diff --git a/src/hooks-config.ts b/src/hooks-config.ts index 9c0048bd..634edbb1 100644 --- a/src/hooks-config.ts +++ b/src/hooks-config.ts @@ -287,6 +287,64 @@ function withSettingsLock(path: string, fn: () => Promise): Promise { return run; } +/** + * Why writing into `/.claude/settings.local.json` must NOT proceed, + * or null when it is safe. + * + * Case contents can be FOREIGN (a freshly cloned repository, an imported + * tree): `.claude` or the settings file itself can arrive as a symlink + * pointing anywhere on this machine, and `writeFile` follows links, so a + * scaffold write would land outside the case, up to and including replacing + * the user's own `~/.claude/settings.json` (#251 review). Any symlink in the + * chain, or a `.claude` that resolves outside the case, refuses the write. + * A missing `.claude` is fine (the writer creates it). + */ +export async function settingsWriteBlocker(casePath: string): Promise { + const claudeDir = join(casePath, '.claude'); + try { + const dirStat = await lstat(claudeDir).catch(() => null); + if (dirStat?.isSymbolicLink()) return 'its .claude is a symlink'; + if (dirStat && !dirStat.isDirectory()) return 'its .claude is a file, not a directory'; + if (dirStat && (await realpath(claudeDir)) !== join(await realpath(casePath), '.claude')) { + return 'its .claude directory resolves outside the case'; + } + const settingsStat = await lstat(join(claudeDir, 'settings.local.json')).catch(() => null); + if (settingsStat?.isSymbolicLink()) return 'its .claude/settings.local.json is a symlink'; + } catch (err) { + return `its .claude paths could not be verified (${String(err)})`; + } + return null; +} + +/** + * The ONE gate for writing `/.claude/settings.local.json`. + * + * Serializes writers per path (withSettingsLock) and, INSIDE the lock, refuses + * the write when `settingsWriteBlocker` reports the target unsafe. Every + * settings writer in this module must go through here rather than calling + * `writeFile` on the settings path itself, so a repository-controlled symlink + * can never redirect ANY of them outside the case (#251 review: the guard + * originally covered only two writers, and applyStatusLineConfig was shown + * writing through a symlinked settings file). Refusal is a console.warn, not + * a throw: hooks/statusline degrade gracefully and the session still runs. + */ +async function withSafeSettingsWrite( + casePath: string, + purpose: string, + fn: (claudeDir: string, settingsPath: string) => Promise +): Promise { + const claudeDir = join(casePath, '.claude'); + const settingsPath = join(claudeDir, 'settings.local.json'); + await withSettingsLock(settingsPath, async () => { + const blocker = await settingsWriteBlocker(casePath); + if (blocker) { + console.warn(`[hooks-config] Refusing to write ${purpose} for ${casePath}: ${blocker}`); + return; + } + await fn(claudeDir, settingsPath); + }); +} + /** * Generates the hooks section for .claude/settings.local.json * @@ -460,8 +518,7 @@ function mergeCodemanHooks(existingValue: unknown, generated: Record { if (keysToRemove.length === 0) return; - const settingsPath = join(casePath, '.claude', 'settings.local.json'); - await withSettingsLock(settingsPath, async () => { + await withSafeSettingsWrite(casePath, 'env-key removal', async (_claudeDir, settingsPath) => { if (!existsSync(settingsPath)) return; let existing: Record; @@ -493,9 +550,7 @@ export async function stripCaseEnvKeys(casePath: string, keysToRemove: readonly * Merges with existing env field; removes vars set to empty string. */ export async function updateCaseEnvVars(casePath: string, envVars: Record): Promise { - const claudeDir = join(casePath, '.claude'); - const settingsPath = join(claudeDir, 'settings.local.json'); - await withSettingsLock(settingsPath, async () => { + await withSafeSettingsWrite(casePath, 'env vars', async (claudeDir, settingsPath) => { if (!existsSync(claudeDir)) { await mkdir(claudeDir, { recursive: true }); } @@ -526,16 +581,7 @@ export async function updateCaseEnvVars(casePath: string, envVars: Record { - // Same symlink refusal as writeHooksConfig: this writer runs for cloned - // cases too (modelOverride at quick-start), against foreign tree contents. - const blocker = await settingsWriteBlocker(casePath); - if (blocker) { - console.warn(`[hooks-config] Refusing to write model for ${casePath}: ${blocker}`); - return; - } - const claudeDir = join(casePath, '.claude'); - const settingsPath = join(claudeDir, 'settings.local.json'); - await withSettingsLock(settingsPath, async () => { + await withSafeSettingsWrite(casePath, 'model', async (claudeDir, settingsPath) => { if (!existsSync(claudeDir)) { await mkdir(claudeDir, { recursive: true }); } @@ -557,35 +603,6 @@ export async function updateCaseModel(casePath: string, model: string | null): P }); } -/** - * Why writing into `/.claude/settings.local.json` must NOT proceed, - * or null when it is safe. - * - * Case contents can be FOREIGN (a freshly cloned repository, an imported - * tree): `.claude` or the settings file itself can arrive as a symlink - * pointing anywhere on this machine, and `writeFile` follows links, so a - * scaffold write would land outside the case, up to and including replacing - * the user's own `~/.claude/settings.json` (#251 review). Any symlink in the - * chain, or a `.claude` that resolves outside the case, refuses the write. - * A missing `.claude` is fine (the writer creates it). - */ -export async function settingsWriteBlocker(casePath: string): Promise { - const claudeDir = join(casePath, '.claude'); - try { - const dirStat = await lstat(claudeDir).catch(() => null); - if (dirStat?.isSymbolicLink()) return 'its .claude is a symlink'; - if (dirStat && !dirStat.isDirectory()) return 'its .claude is a file, not a directory'; - if (dirStat && (await realpath(claudeDir)) !== join(await realpath(casePath), '.claude')) { - return 'its .claude directory resolves outside the case'; - } - const settingsStat = await lstat(join(claudeDir, 'settings.local.json')).catch(() => null); - if (settingsStat?.isSymbolicLink()) return 'its .claude/settings.local.json is a symlink'; - } catch (err) { - return `its .claude paths could not be verified (${String(err)})`; - } - return null; -} - /** * Writes hooks config to .claude/settings.local.json in the given case path. * Merges with existing file content, only touching the `hooks` key. @@ -593,14 +610,7 @@ export async function settingsWriteBlocker(casePath: string): Promise { - const blocker = await settingsWriteBlocker(casePath); - if (blocker) { - console.warn(`[hooks-config] Refusing to write hooks for ${casePath}: ${blocker}`); - return; - } - const claudeDir = join(casePath, '.claude'); - const settingsPath = join(claudeDir, 'settings.local.json'); - await withSettingsLock(settingsPath, async () => { + await withSafeSettingsWrite(casePath, 'hooks', async (claudeDir, settingsPath) => { if (!existsSync(claudeDir)) { await mkdir(claudeDir, { recursive: true }); } @@ -642,9 +652,7 @@ export async function writeHooksConfig(casePath: string): Promise { * block they have never had), so that call is left to the owner rather than made here. */ export async function ensureCodemanHooks(casePath: string): Promise { - const claudeDir = join(casePath, '.claude'); - const settingsPath = join(claudeDir, 'settings.local.json'); - await withSettingsLock(settingsPath, async () => { + await withSafeSettingsWrite(casePath, 'hooks (ensure)', async (claudeDir, settingsPath) => { if (!existsSync(claudeDir)) { await mkdir(claudeDir, { recursive: true }); } @@ -685,9 +693,8 @@ export async function ensureCodemanHooks(casePath: string): Promise { * when the hooks aren't ours, so it is cheap enough to call on every Claude spawn. */ export async function refreshStaleCodemanHooks(casePath: string): Promise { - const settingsPath = join(casePath, '.claude', 'settings.local.json'); - if (!existsSync(settingsPath)) return; - await withSettingsLock(settingsPath, async () => { + if (!existsSync(join(casePath, '.claude', 'settings.local.json'))) return; + await withSafeSettingsWrite(casePath, 'hooks (refresh)', async (_claudeDir, settingsPath) => { let existing: Record; try { existing = JSON.parse(await readFile(settingsPath, 'utf-8')); @@ -749,10 +756,7 @@ export function generateStatusLineCommand(): string { * Claude mode. Merges, preserving all other keys (hooks, env, model). */ export async function applyStatusLineConfig(casePath: string, enabled: boolean): Promise { - const claudeDir = join(casePath, '.claude'); - const settingsPath = join(claudeDir, 'settings.local.json'); - - await withSettingsLock(settingsPath, async () => { + await withSafeSettingsWrite(casePath, 'statusLine', async (claudeDir, settingsPath) => { let existing: Record = {}; if (existsSync(settingsPath)) { try { diff --git a/test/hooks-config.test.ts b/test/hooks-config.test.ts index 500fa6c9..b729b6da 100644 --- a/test/hooks-config.test.ts +++ b/test/hooks-config.test.ts @@ -11,12 +11,16 @@ import { join } from 'node:path'; import { tmpdir } from 'node:os'; import { spawn } from 'node:child_process'; import { + applyStatusLineConfig, ensureCodemanHooks, generateBackgroundWakeScript, generateHooksConfig, generateSubagentStopGuardScript, refreshStaleCodemanHooks, settingsWriteBlocker, + stripCaseEnvKeys, + updateCaseEnvVars, + updateCaseModel, writeHooksConfig, } from '../src/hooks-config.js'; @@ -230,6 +234,31 @@ describe('writeHooksConfig', () => { expect(await settingsWriteBlocker(caseDir)).toBeNull(); }); + it('EVERY settings writer refuses a symlinked settings.local.json (#251 review round 2)', async () => { + // Round 1 guarded only writeHooksConfig/updateCaseModel; the reviewer + // demonstrated applyStatusLineConfig writing through the link. All + // writers now share one safe-write gate, so pin all of them at once. + const outsideFile = join(testDir, 'victim-all-writers.json'); + const precious = + '{"env":{"CLAUDE_CODE_KEEP":"me"},"hooks":{"Stop":[{"hooks":[{"command":"curl /api/hook-event"}]}]}}\n'; + writeFileSync(outsideFile, precious); + const caseDir = join(testDir, 'case-writers'); + mkdirSync(join(caseDir, '.claude'), { recursive: true }); + symlinkSync(outsideFile, join(caseDir, '.claude', 'settings.local.json')); + + await writeHooksConfig(caseDir); + await ensureCodemanHooks(caseDir); + await refreshStaleCodemanHooks(caseDir); + await updateCaseModel(caseDir, 'opus'); + await updateCaseEnvVars(caseDir, { CLAUDE_CODE_NEW: 'value' }); + await stripCaseEnvKeys(caseDir, ['CLAUDE_CODE_KEEP']); + await applyStatusLineConfig(caseDir, true); + await applyStatusLineConfig(caseDir, false); + + // The link target is byte-identical: none of the writers went through it. + expect(readFileSync(outsideFile, 'utf-8')).toBe(precious); + }); + it('should merge with existing settings.local.json', async () => { const claudeDir = join(testDir, '.claude'); mkdirSync(claudeDir, { recursive: true });