diff --git a/careful/bin/hook-extract.sh b/careful/bin/hook-extract.sh index 6ab8d611e..592e8fbd7 100644 --- a/careful/bin/hook-extract.sh +++ b/careful/bin/hook-extract.sh @@ -63,12 +63,35 @@ gstack_hook_decision() { printf '{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"%s","permissionDecisionReason":%s}}\n' "$_ghd_decision" "$_ghd_encoded" } +# gstack_hook_state_root +# Print the gstack state root, resolved with EXACTLY the chain bin/gstack-paths +# uses (GSTACK_STATE_ROOT): GSTACK_HOME, then CLAUDE_PLUGIN_DATA only when +# CLAUDE_PLUGIN_ROOT names gstack (a CLAUDE_PLUGIN_DATA leaked from another +# plugin via CLAUDE_ENV_FILE must not redirect our state), then $HOME/.gstack, +# then a project-local .gstack. Hooks run on every Edit/Bash call, so this is +# pure bash — never spawn gstack-paths from a hook. The writers (/freeze, +# /guard, /unfreeze, /investigate) resolve through gstack-paths; a reader that +# used a different chain failed OPEN whenever GSTACK_HOME was set (#1459). +# test/hook-scripts.test.ts pins parity against gstack-paths. +gstack_hook_state_root() { + if [ -n "${GSTACK_HOME:-}" ]; then + printf '%s\n' "$GSTACK_HOME" + elif [ -n "${CLAUDE_PLUGIN_DATA:-}" ] && printf '%s' "${CLAUDE_PLUGIN_ROOT:-}" | grep -qi "gstack"; then + printf '%s\n' "$CLAUDE_PLUGIN_DATA" + elif [ -n "${HOME:-}" ]; then + printf '%s\n' "$HOME/.gstack" + else + printf '%s\n' ".gstack" + fi +} + # gstack_hook_log_fire SKILL PATTERN # Append a hook_fire analytics record (pattern name only, never command -# content). Respects GSTACK_HOME so tests never pollute the operator's real -# analytics file. Best-effort: failures never affect the hook decision. +# content). Resolves the state root through gstack_hook_state_root so tests +# never pollute the operator's real analytics file. Best-effort: failures +# never affect the hook decision. gstack_hook_log_fire() { - _ghlf_dir="${GSTACK_HOME:-$HOME/.gstack}/analytics" + _ghlf_dir="$(gstack_hook_state_root)/analytics" mkdir -p "$_ghlf_dir" 2>/dev/null || true # Fields are JSON-encoded (a repo basename can carry quotes/backslashes) — # same rule this file states for decisions: never raw-interpolate into JSON. diff --git a/freeze/bin/check-freeze.sh b/freeze/bin/check-freeze.sh index 6c6e62e76..6e76da78e 100755 --- a/freeze/bin/check-freeze.sh +++ b/freeze/bin/check-freeze.sh @@ -31,8 +31,12 @@ if [ ! -f "$_HOOK_HELPER" ] || ! . "$_HOOK_HELPER" 2>/dev/null; then exit 0 fi -# Locate the freeze directory state file -STATE_DIR="${CLAUDE_PLUGIN_DATA:-$HOME/.gstack}" +# Locate the freeze directory state file. The writer (/freeze via +# gstack-paths) and this reader MUST resolve the same root or the boundary +# fails open: with GSTACK_HOME set, /freeze wrote freeze-dir.txt under +# GSTACK_HOME while this hook read $HOME/.gstack, found nothing, and allowed +# everything (#1459, #1509). gstack_hook_state_root mirrors gstack-paths. +STATE_DIR="$(gstack_hook_state_root)" FREEZE_FILE="$STATE_DIR/freeze-dir.txt" # If no freeze file exists, allow everything (not yet configured) diff --git a/test/hook-scripts.test.ts b/test/hook-scripts.test.ts index 55817099a..a274b9c00 100644 --- a/test/hook-scripts.test.ts +++ b/test/hook-scripts.test.ts @@ -76,6 +76,34 @@ function withFreezeDir(freezePath: string, fn: (stateDir: string) => void) { } } +// The freeze WRITER resolves its state root through bin/gstack-paths, which +// trusts CLAUDE_PLUGIN_DATA only when CLAUDE_PLUGIN_ROOT names gstack; the +// reader mirrors that exact chain (#1459 / #1509). A test standing in for a +// plugin install must supply both, and must neutralize a GSTACK_HOME inherited +// from the shard's process.env (an empty value reads as unset in ${VAR:-}). +function freezeEnv(stateDir: string, extra: Record = {}): Record { + return { GSTACK_HOME: '', CLAUDE_PLUGIN_DATA: stateDir, CLAUDE_PLUGIN_ROOT: '/plugins/gstack', ...extra }; +} + +const HOOK_EXTRACT = path.join(ROOT, 'careful', 'bin', 'hook-extract.sh'); +const GSTACK_PATHS = path.join(ROOT, 'bin', 'gstack-paths'); + +/** What the hook helper resolves as the state root under a given env. */ +function hookStateRoot(env: Record): string { + const r = spawnSync('bash', ['-c', `. "${HOOK_EXTRACT}" && gstack_hook_state_root`], { + env: { PATH: process.env.PATH ?? '', ...env }, encoding: 'utf-8', timeout: 5000, + }); + return r.stdout.trim(); +} + +/** What bin/gstack-paths resolves as GSTACK_STATE_ROOT under the same env. */ +function pathsStateRoot(env: Record): string { + const r = spawnSync('bash', ['-c', `eval "$("${GSTACK_PATHS}")" && printf '%s' "$GSTACK_STATE_ROOT"`], { + env: { PATH: process.env.PATH ?? '', ...env }, encoding: 'utf-8', timeout: 5000, + }); + return r.stdout.trim(); +} + // ============================================================ // Frontmatter hook wiring (#2469 / #1871) // ============================================================ @@ -687,7 +715,7 @@ describe('check-freeze.sh', () => { const { exitCode, output } = runHook( FREEZE_SCRIPT, freezeInput('/Users/dev/project/src/index.ts'), - { CLAUDE_PLUGIN_DATA: stateDir }, + freezeEnv(stateDir), ); expect(exitCode).toBe(0); expect(output.hookSpecificOutput?.permissionDecision).toBeUndefined(); @@ -699,7 +727,7 @@ describe('check-freeze.sh', () => { const { exitCode, output } = runHook( FREEZE_SCRIPT, freezeInput('/Users/dev/project/src/components/Button.tsx'), - { CLAUDE_PLUGIN_DATA: stateDir }, + freezeEnv(stateDir), ); expect(exitCode).toBe(0); expect(output.hookSpecificOutput?.permissionDecision).toBeUndefined(); @@ -713,7 +741,7 @@ describe('check-freeze.sh', () => { const { exitCode, output } = runHook( FREEZE_SCRIPT, freezeInput('/Users/dev/other-project/index.ts'), - { CLAUDE_PLUGIN_DATA: stateDir }, + freezeEnv(stateDir), ); expect(exitCode).toBe(0); expect(output.hookSpecificOutput?.permissionDecision).toBe('deny'); @@ -727,7 +755,7 @@ describe('check-freeze.sh', () => { const { exitCode, output } = runHook( FREEZE_SCRIPT, freezeInput('/etc/hosts'), - { CLAUDE_PLUGIN_DATA: stateDir }, + freezeEnv(stateDir), ); expect(exitCode).toBe(0); expect(output.hookSpecificOutput?.permissionDecision).toBe('deny'); @@ -743,7 +771,7 @@ describe('check-freeze.sh', () => { const { exitCode, output } = runHook( FREEZE_SCRIPT, freezeInput('/Users/dev/project/src-old/index.ts'), - { CLAUDE_PLUGIN_DATA: stateDir }, + freezeEnv(stateDir), ); expect(exitCode).toBe(0); expect(output.hookSpecificOutput?.permissionDecision).toBe('deny'); @@ -759,7 +787,7 @@ describe('check-freeze.sh', () => { const { exitCode, output } = runHook( FREEZE_SCRIPT, freezeInput('/anywhere/at/all.ts'), - { CLAUDE_PLUGIN_DATA: stateDir }, + freezeEnv(stateDir), ); expect(exitCode).toBe(0); expect(output.hookSpecificOutput?.permissionDecision).toBeUndefined(); @@ -775,7 +803,7 @@ describe('check-freeze.sh', () => { const { exitCode, output } = runHook( FREEZE_SCRIPT, { tool_input: {} }, - { CLAUDE_PLUGIN_DATA: stateDir }, + freezeEnv(stateDir), ); expect(exitCode).toBe(0); expect(output.hookSpecificOutput?.permissionDecision).toBeUndefined(); @@ -787,7 +815,7 @@ describe('check-freeze.sh', () => { const { exitCode, output } = runHookRaw( FREEZE_SCRIPT, 'not json at all {{{{', - { CLAUDE_PLUGIN_DATA: stateDir }, + freezeEnv(stateDir), ); expect(exitCode).toBe(0); expect(output.hookSpecificOutput?.permissionDecision).toBe('deny'); @@ -803,7 +831,7 @@ describe('check-freeze.sh', () => { const { exitCode, output, raw } = runHook( FREEZE_SCRIPT, freezeInput('/tmp/evil"quoted/x.ts'), - { CLAUDE_PLUGIN_DATA: stateDir }, + freezeEnv(stateDir), ); expect(exitCode).toBe(0); expect(() => JSON.parse(raw)).not.toThrow(); @@ -816,7 +844,7 @@ describe('check-freeze.sh', () => { const { exitCode, output, raw } = runHook( FREEZE_SCRIPT, freezeInput('/tmp/evil\npath.ts'), - { CLAUDE_PLUGIN_DATA: stateDir }, + freezeEnv(stateDir), ); expect(exitCode).toBe(0); expect(() => JSON.parse(raw)).not.toThrow(); @@ -834,11 +862,11 @@ describe('check-freeze.sh', () => { fs.mkdirSync(boundary, { recursive: true }); try { withFreezeDir(boundary + '/', (stateDir) => { - const inside = runHook(FREEZE_SCRIPT, freezeInput(path.join(boundary, 'index.ts')), { CLAUDE_PLUGIN_DATA: stateDir }); + const inside = runHook(FREEZE_SCRIPT, freezeInput(path.join(boundary, 'index.ts')), freezeEnv(stateDir)); expect(inside.exitCode).toBe(0); expect(inside.output.hookSpecificOutput?.permissionDecision).toBeUndefined(); - const outside = runHook(FREEZE_SCRIPT, freezeInput(path.join(base, 'elsewhere.ts')), { CLAUDE_PLUGIN_DATA: stateDir }); + const outside = runHook(FREEZE_SCRIPT, freezeInput(path.join(base, 'elsewhere.ts')), freezeEnv(stateDir)); expect(outside.exitCode).toBe(0); expect(outside.output.hookSpecificOutput?.permissionDecision).toBe('deny'); }); @@ -859,7 +887,7 @@ describe('check-freeze.sh', () => { fs.copyFileSync(FREEZE_SCRIPT, script); try { withFreezeDir('/Users/dev/project/src/', (stateDir) => { - const { exitCode, output } = runHook(script, freezeInput('/Users/dev/project/src/x.ts'), { CLAUDE_PLUGIN_DATA: stateDir }); + const { exitCode, output } = runHook(script, freezeInput('/Users/dev/project/src/x.ts'), freezeEnv(stateDir)); expect(exitCode).toBe(0); expect(output.hookSpecificOutput?.permissionDecision).toBe('deny'); expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('fail closed'); @@ -884,13 +912,13 @@ describe('check-freeze.sh', () => { fs.symlinkSync(path.join(outside, 'secret.txt'), path.join(boundary, 'link.txt')); try { withFreezeDir(boundary + '/', (stateDir) => { - const viaLink = runHook(FREEZE_SCRIPT, freezeInput(path.join(boundary, 'link.txt')), { CLAUDE_PLUGIN_DATA: stateDir }); + const viaLink = runHook(FREEZE_SCRIPT, freezeInput(path.join(boundary, 'link.txt')), freezeEnv(stateDir)); expect(viaLink.exitCode).toBe(0); expect(viaLink.output.hookSpecificOutput?.permissionDecision).toBe('deny'); // A real in-boundary file is unaffected. fs.writeFileSync(path.join(boundary, 'real.txt'), 'y'); - const real = runHook(FREEZE_SCRIPT, freezeInput(path.join(boundary, 'real.txt')), { CLAUDE_PLUGIN_DATA: stateDir }); + const real = runHook(FREEZE_SCRIPT, freezeInput(path.join(boundary, 'real.txt')), freezeEnv(stateDir)); expect(real.exitCode).toBe(0); expect(real.output.hookSpecificOutput?.permissionDecision).toBeUndefined(); }); @@ -900,3 +928,79 @@ describe('check-freeze.sh', () => { }); }); }); + +// ============================================================ +// check-freeze.sh state-root resolution (#1459 / #1509) +// ============================================================ +// /freeze writes freeze-dir.txt under the root gstack-paths resolves +// (GSTACK_HOME first). The reader used to read ${CLAUDE_PLUGIN_DATA:-$HOME/.gstack} +// — so with GSTACK_HOME set it found no file and ALLOWED everything. A deny-tier +// boundary that fails open is not a boundary; writer and reader now share one +// chain (gstack_hook_state_root in careful/bin/hook-extract.sh). +describe('check-freeze.sh state-root resolution (#1459 / #1509)', () => { + const BOUNDARY = '/Users/dev/project/src/'; + const OUTSIDE = '/Users/dev/other-project/index.ts'; + + function withEmptyDir(fn: (dir: string) => void) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-freeze-empty-')); + try { fn(dir); } finally { fs.rmSync(dir, { recursive: true, force: true }); } + } + + test('REGRESSION: freeze file under GSTACK_HOME (HOME has none) denies an outside edit', () => { + withFreezeDir(BOUNDARY, (gstackHome) => { + withEmptyDir((fakeHome) => { + const { exitCode, output } = runHook(FREEZE_SCRIPT, freezeInput(OUTSIDE), { + GSTACK_HOME: gstackHome, HOME: fakeHome, CLAUDE_PLUGIN_DATA: '', CLAUDE_PLUGIN_ROOT: '', + }); + expect(exitCode).toBe(0); + expect(output.hookSpecificOutput?.permissionDecision).toBe('deny'); + }); + }); + }); + + test('GSTACK_HOME wins over CLAUDE_PLUGIN_DATA (matches gstack-paths precedence)', () => { + withFreezeDir(BOUNDARY, (pluginData) => { + withEmptyDir((gstackHome) => { + // The freeze file lives under CLAUDE_PLUGIN_DATA, but GSTACK_HOME is set and + // has none — the writer would have written there, so the reader must look there. + const { output } = runHook(FREEZE_SCRIPT, freezeInput(OUTSIDE), + freezeEnv(pluginData, { GSTACK_HOME: gstackHome })); + expect(output.hookSpecificOutput?.permissionDecision).toBeUndefined(); + }); + }); + }); + + test('CLAUDE_PLUGIN_DATA is ignored when CLAUDE_PLUGIN_ROOT is another plugin', () => { + withFreezeDir(BOUNDARY, (pluginData) => { + withEmptyDir((fakeHome) => { + const { output } = runHook(FREEZE_SCRIPT, freezeInput(OUTSIDE), + freezeEnv(pluginData, { CLAUDE_PLUGIN_ROOT: '/plugins/codex', HOME: fakeHome })); + // Falls through to $HOME/.gstack, which has no freeze file → allow. + expect(output.hookSpecificOutput?.permissionDecision).toBeUndefined(); + }); + }); + }); + + test('CLAUDE_PLUGIN_DATA is honoured when CLAUDE_PLUGIN_ROOT names gstack', () => { + withFreezeDir(BOUNDARY, (pluginData) => { + withEmptyDir((fakeHome) => { + const { output } = runHook(FREEZE_SCRIPT, freezeInput(OUTSIDE), freezeEnv(pluginData, { HOME: fakeHome })); + expect(output.hookSpecificOutput?.permissionDecision).toBe('deny'); + }); + }); + }); + + test('gstack_hook_state_root is byte-identical to gstack-paths GSTACK_STATE_ROOT', () => { + const combos: Record[] = [ + { HOME: '/home/u', GSTACK_HOME: '/state/x', CLAUDE_PLUGIN_DATA: '/plug/data', CLAUDE_PLUGIN_ROOT: '/plugins/gstack' }, + { HOME: '/home/u', GSTACK_HOME: '', CLAUDE_PLUGIN_DATA: '/plug/data', CLAUDE_PLUGIN_ROOT: '/plugins/gstack' }, + { HOME: '/home/u', GSTACK_HOME: '', CLAUDE_PLUGIN_DATA: '/plug/data', CLAUDE_PLUGIN_ROOT: '/plugins/codex' }, + { HOME: '/home/u', GSTACK_HOME: '', CLAUDE_PLUGIN_DATA: '/plug/data', CLAUDE_PLUGIN_ROOT: '' }, + { HOME: '/home/u', GSTACK_HOME: '', CLAUDE_PLUGIN_DATA: '', CLAUDE_PLUGIN_ROOT: '' }, + { HOME: '', GSTACK_HOME: '', CLAUDE_PLUGIN_DATA: '', CLAUDE_PLUGIN_ROOT: '' }, + ]; + for (const env of combos) { + expect(hookStateRoot(env)).toBe(pathsStateRoot(env)); + } + }); +});