From fb4d2983e9a3e8406fd85fa208146f2dc61da481 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 3 Oct 2026 20:52:02 +0000 Subject: [PATCH] E1: freeze normalizes boundary and target the same way on Windows; hooks spawn through cmd.exe (#2876, #2354) gstack_hook_normalize_path (careful/bin/hook-extract.sh) maps C:\..., c:/..., UNC \\server\share (leading // kept) and /cygdrive/c to one form; check-freeze runs both the saved boundary and file_path through it before the absolute test, after symlink resolution, and compares case-insensitively on Windows bash. freeze, guard and careful register hooks as bash -c "exec bash \"$HOME/...\"" so cmd.exe, which never expands $HOME, no longer turns the hook into a silent exit 127. Co-authored-by: Kevin Grasso Co-authored-by: Xuan-Ce Wang <261591073+xwang4-svg@users.noreply.github.com> --- careful/SKILL.md.tmpl | 2 +- careful/bin/hook-extract.sh | 44 ++++++- freeze/SKILL.md.tmpl | 4 +- freeze/bin/check-freeze.sh | 29 ++++- guard/SKILL.md.tmpl | 6 +- test/investigate-freeze-path.test.ts | 176 +++++++++++++++++++++++++++ 6 files changed, 250 insertions(+), 11 deletions(-) diff --git a/careful/SKILL.md.tmpl b/careful/SKILL.md.tmpl index e77dc2930..33c1b8286 100644 --- a/careful/SKILL.md.tmpl +++ b/careful/SKILL.md.tmpl @@ -19,7 +19,7 @@ hooks: - matcher: "Bash" hooks: - type: command - command: "bash $HOME/.claude/skills/gstack/careful/bin/check-careful.sh" + command: 'bash -c "exec bash \"$HOME/.claude/skills/gstack/careful/bin/check-careful.sh\""' statusMessage: "Checking for destructive commands..." sensitive: true --- diff --git a/careful/bin/hook-extract.sh b/careful/bin/hook-extract.sh index 2934beb2e..09e0cd26f 100644 --- a/careful/bin/hook-extract.sh +++ b/careful/bin/hook-extract.sh @@ -1,5 +1,5 @@ #!/usr/bin/env bash -# hook-extract.sh — SHARED JSON helpers for gstack PreToolUse hooks. +# hook-extract.sh — SHARED JSON and path helpers for gstack PreToolUse hooks. # Sourced (never executed) by careful/bin/check-careful.sh and # freeze/bin/check-freeze.sh via a path relative to each hook script. # @@ -32,6 +32,48 @@ sys.stdout.write(c if isinstance(c, str) else "")' "$_ghef_field" 2>/dev/null && return 1 } +# Windows bash (Git Bash / MSYS / Cygwin), read from bash's own OSTYPE so the +# check never costs a fork: careful sources this file on every Bash call. +case "${OSTYPE:-}" in + msys*|cygwin*|win32*) GSTACK_HOOK_IS_WINDOWS=1 ;; + *) GSTACK_HOOK_IS_WINDOWS=0 ;; +esac +_GSTACK_HOOK_AZ_UPPER=ABCDEFGHIJKLMNOPQRSTUVWXYZ +_GSTACK_HOOK_AZ_LOWER=abcdefghijklmnopqrstuvwxyz +GSTACK_HOOK_PATH="" + +# gstack_hook_normalize_path PATH +# The one path canonicalization for freeze's boundary AND the tool's +# file_path (#2876). Leaves the result in GSTACK_HOOK_PATH (stdout is the +# hook's decision channel, and $(...) would cost a fork): +# C:\dev\x c:/dev/x -> /c/dev/x (separators, drive-letter case) +# \\srv\share\x -> //srv/share/x (UNC: the leading // is kept) +# /cygdrive/c/x -> /c/x (Windows bash only) +# Drive-letter and UNC shapes are recognised lexically on every platform. +# A path without those shapes is rewritten only on Windows bash: on POSIX +# '\' is a legal filename character, and rewriting it would let a name like +# 'a\..\..\etc\x' change which directory the check sees. Remaining case +# differences on Windows are handled by comparing under nocasematch. +# Builtins only, bash 3.2 compatible. +gstack_hook_normalize_path() { + GSTACK_HOOK_PATH="$1" + case "$GSTACK_HOOK_PATH" in + [A-Za-z]:[\\/]*|[A-Za-z]:|\\\\[!\\]*) ;; + *) [ "$GSTACK_HOOK_IS_WINDOWS" = 1 ] || return 0 ;; + esac + GSTACK_HOOK_PATH="${GSTACK_HOOK_PATH//\\//}" + case "$GSTACK_HOOK_PATH" in + [A-Za-z]:/*|[A-Za-z]:) + _ghnp_d="${GSTACK_HOOK_PATH%%:*}" + _ghnp_pre="${_GSTACK_HOOK_AZ_UPPER%%"$_ghnp_d"*}" + [ "$_ghnp_pre" = "$_GSTACK_HOOK_AZ_UPPER" ] || _ghnp_d="${_GSTACK_HOOK_AZ_LOWER:${#_ghnp_pre}:1}" + GSTACK_HOOK_PATH="/$_ghnp_d${GSTACK_HOOK_PATH#?:}" + ;; + /cygdrive/[A-Za-z]/*|/cygdrive/[A-Za-z]) GSTACK_HOOK_PATH="${GSTACK_HOOK_PATH#/cygdrive}" ;; + esac + return 0 +} + # gstack_hook_json_string TEXT # Prints TEXT as a JSON string literal (surrounding quotes included), # encoding quotes, backslashes, control characters and newlines. Never build diff --git a/freeze/SKILL.md.tmpl b/freeze/SKILL.md.tmpl index 470f713a1..b86711c62 100644 --- a/freeze/SKILL.md.tmpl +++ b/freeze/SKILL.md.tmpl @@ -20,12 +20,12 @@ hooks: - matcher: "Edit" hooks: - type: command - command: "bash $HOME/.claude/skills/gstack/freeze/bin/check-freeze.sh" + command: 'bash -c "exec bash \"$HOME/.claude/skills/gstack/freeze/bin/check-freeze.sh\""' statusMessage: "Checking freeze boundary..." - matcher: "Write" hooks: - type: command - command: "bash $HOME/.claude/skills/gstack/freeze/bin/check-freeze.sh" + command: 'bash -c "exec bash \"$HOME/.claude/skills/gstack/freeze/bin/check-freeze.sh\""' statusMessage: "Checking freeze boundary..." sensitive: true --- diff --git a/freeze/bin/check-freeze.sh b/freeze/bin/check-freeze.sh index 0c092dbc1..4b67dae03 100755 --- a/freeze/bin/check-freeze.sh +++ b/freeze/bin/check-freeze.sh @@ -56,7 +56,7 @@ fi # A helper from an older install that lacks the function must fail CLOSED # (the existence check above only proves the file sourced), never exit 127 # with no JSON — Claude Code treats that as non-blocking. -if ! command -v gstack_hook_state_root >/dev/null 2>&1; then +if ! command -v gstack_hook_state_root >/dev/null 2>&1 || ! command -v gstack_hook_normalize_path >/dev/null 2>&1; then _FREEZE_DECIDED=1 printf '{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"deny","permissionDecisionReason":"[freeze] Hook helpers out of date (partial upgrade?) - blocked, fail closed. Re-run ./setup or /unfreeze."}}\n' exit 0 @@ -93,6 +93,12 @@ if [ -z "$FREEZE_DIR" ]; then exit 0 fi +# Both sides of the comparison go through one canonicalization: the state +# file may hold C:\dev\proj\ or the /c/dev/proj/ that `pwd` prints in Git +# Bash, and Claude Code on Windows reports file_path as C:\... (#2876). +gstack_hook_normalize_path "$FREEZE_DIR" +FREEZE_DIR="$GSTACK_HOOK_PATH" + case "$FREEZE_DIR" in /*) ;; *) @@ -123,6 +129,11 @@ if [ -z "$FILE_PATH" ]; then exit 0 fi +# Canonicalize before the absolute test: a drive-letter path does not start +# with '/', so it used to be joined onto cwd and every edit was denied. +gstack_hook_normalize_path "$FILE_PATH" +FILE_PATH="$GSTACK_HOOK_PATH" + # Resolve file_path to absolute if it isn't already case "$FILE_PATH" in /*) ;; # already absolute @@ -131,8 +142,14 @@ case "$FILE_PATH" in ;; esac -# Normalize: remove double slashes and trailing slash +# Normalize: remove double slashes and trailing slash. A leading '//' is a UNC +# host (//server/share) and is kept, or the check would compare a local path. +_FP_LEAD="" +case "$FILE_PATH" in + //[!/]*) _FP_LEAD="/" ;; +esac FILE_PATH=$(printf '%s' "$FILE_PATH" | sed 's|/\+|/|g;s|/$||') +FILE_PATH="$_FP_LEAD$FILE_PATH" [ -n "$FILE_PATH" ] || FILE_PATH="/" # Resolve symlinks and .. sequences (POSIX-portable, works on macOS). @@ -157,12 +174,16 @@ _resolve_path() { _base="$(basename "$_p")" if [ "$_base" = / ]; then printf '/'; return; fi _dir="$(cd "$_dir" 2>/dev/null && pwd -P || printf '%s' "$_dir")" - printf '%s/%s' "${_dir%/}" "$_base" + # pwd -P answers in the platform's own dialect (/cygdrive/c/... on Cygwin). + gstack_hook_normalize_path "${_dir%/}/$_base" + printf '%s' "$GSTACK_HOOK_PATH" } FILE_PATH=$(_resolve_path "$FILE_PATH") FREEZE_DIR=$(_resolve_path "$FREEZE_DIR") -# Check: does the file path start with the freeze directory? +# Check: does the file path start with the freeze directory? Windows paths +# are case-insensitive, so compare that way there. +[ "$GSTACK_HOOK_IS_WINDOWS" = 1 ] && shopt -s nocasematch case "$FILE_PATH" in "${FREEZE_DIR%/}/"*|"${FREEZE_DIR}") # Inside freeze boundary — allow diff --git a/guard/SKILL.md.tmpl b/guard/SKILL.md.tmpl index cf0b157e9..f2f194920 100644 --- a/guard/SKILL.md.tmpl +++ b/guard/SKILL.md.tmpl @@ -20,17 +20,17 @@ hooks: - matcher: "Bash" hooks: - type: command - command: "bash $HOME/.claude/skills/gstack/careful/bin/check-careful.sh" + command: 'bash -c "exec bash \"$HOME/.claude/skills/gstack/careful/bin/check-careful.sh\""' statusMessage: "Checking for destructive commands..." - matcher: "Edit" hooks: - type: command - command: "bash $HOME/.claude/skills/gstack/freeze/bin/check-freeze.sh" + command: 'bash -c "exec bash \"$HOME/.claude/skills/gstack/freeze/bin/check-freeze.sh\""' statusMessage: "Checking freeze boundary..." - matcher: "Write" hooks: - type: command - command: "bash $HOME/.claude/skills/gstack/freeze/bin/check-freeze.sh" + command: 'bash -c "exec bash \"$HOME/.claude/skills/gstack/freeze/bin/check-freeze.sh\""' statusMessage: "Checking freeze boundary..." sensitive: true --- diff --git a/test/investigate-freeze-path.test.ts b/test/investigate-freeze-path.test.ts index 7b9ef0cae..0dbbdb16a 100644 --- a/test/investigate-freeze-path.test.ts +++ b/test/investigate-freeze-path.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from 'bun:test'; import * as fs from 'fs'; import * as path from 'path'; +import * as os from 'os'; const ROOT = path.resolve(import.meta.dir, '..'); const FILES = ['investigate/SKILL.md.tmpl', 'investigate/SKILL.md']; @@ -32,3 +33,178 @@ describe('investigate freeze path resolution', () => { }); } }); + +// E1 (#2876, #2354 residual). One canonicalization for the saved boundary and +// the tool's file_path, exercised through the real hook script. Hooks run via +// explicit `bash` argv with template-literal paths so the file stays in the +// windows-free lane (scripts/test-free-shards.ts --windows-only --list). +const IS_WIN = process.platform === 'win32'; +const FREEZE_HOOK = `${ROOT}/freeze/bin/check-freeze.sh`; + +function decision(stdout: string): string { + const raw = stdout.trim(); + if (raw === '{}') return 'allow'; + return JSON.parse(raw)?.hookSpecificOutput?.permissionDecision ?? `unparsed: ${raw}`; +} + +function withState(boundary: string, fn: (env: Record) => T): T { + const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-e1-freeze-')); + try { + fs.writeFileSync(path.join(stateDir, 'freeze-dir.txt'), `${boundary}\n`); + return fn({ ...process.env, GSTACK_STATE_ROOT: stateDir } as Record); + } finally { + fs.rmSync(stateDir, { recursive: true, force: true }); + } +} + +function freezeDecision(boundary: string, filePath: string): string { + return withState(boundary, (env) => { + const r = Bun.spawnSync(['bash', FREEZE_HOOK], { + stdin: Buffer.from(JSON.stringify({ tool_input: { file_path: filePath } })), + env, + timeout: 15_000, + }); + expect(r.exitCode).toBe(0); + return decision(r.stdout.toString()); + }); +} + +describe('E1: freeze boundary and target share one platform-aware normalization (#2876)', () => { + test.each([ + ['C:\\dev\\proj\\src\\', 'C:\\dev\\proj\\src\\a.py', 'allow'], + ['/c/dev/proj/src/', 'C:\\dev\\proj\\src\\a.py', 'allow'], + ['C:\\dev\\proj\\src\\', '/c/dev/proj/src/a.py', 'allow'], + ['c:/dev/proj/src/', 'C:\\dev\\proj\\src\\a.py', 'allow'], + ['C:/dev/proj/src', 'c:\\dev\\proj\\src\\deep\\new\\file.py', 'allow'], + ['C:\\dev\\proj\\src\\', 'C:\\dev\\proj\\src-old\\a.py', 'deny'], + ['C:\\dev\\proj\\src\\', 'D:\\dev\\proj\\src\\a.py', 'deny'], + ['C:\\dev\\proj\\src\\', 'C:\\dev\\proj\\other\\missing\\a.py', 'deny'], + ['\\\\server\\share\\proj\\', '\\\\server\\share\\proj\\a.py', 'allow'], + ['\\\\server\\share\\proj\\', '\\\\server\\other\\proj\\a.py', 'deny'], + ['\\\\server\\share\\proj\\', '/server/share/proj/a.py', 'deny'], + ])('boundary %s, target %s → %s', (boundary, target, expected) => { + expect(freezeDecision(boundary, target)).toBe(expected); + }); + + test.skipIf(!IS_WIN)('Windows: drive and path case differences do not decide the outcome', () => { + expect(freezeDecision('C:\\Dev\\Proj\\Src\\', 'c:\\dev\\proj\\src\\a.py')).toBe('allow'); + expect(freezeDecision('c:\\dev\\proj\\src\\', 'C:\\DEV\\PROJ\\SRC-OLD\\a.py')).toBe('deny'); + }); + + test.skipIf(!IS_WIN)('Windows: a junction inside the boundary that points outside is denied', () => { + const base = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-e1-junction-'))); + try { + fs.mkdirSync(path.join(base, 'boundary')); + fs.mkdirSync(path.join(base, 'outside')); + fs.symlinkSync(path.join(base, 'outside'), path.join(base, 'boundary', 'escape'), 'junction'); + expect(freezeDecision(`${path.join(base, 'boundary')}\\`, path.join(base, 'boundary', 'escape', 'a.py'))).toBe('deny'); + expect(freezeDecision(`${path.join(base, 'boundary')}\\`, path.join(base, 'boundary', 'a.py'))).toBe('allow'); + } finally { + fs.rmSync(base, { recursive: true, force: true }); + } + }); + + test.skipIf(IS_WIN)('POSIX: a backslash in a filename stays a filename character', () => { + const base = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-e1-bslash-'))); + try { + const boundary = path.join(base, 'boundary'); + fs.mkdirSync(boundary); + expect(freezeDecision(`${boundary}/`, `${boundary}/a\\..\\..\\etc\\x`)).toBe('allow'); + } finally { + fs.rmSync(base, { recursive: true, force: true }); + } + }); +}); + +// #2354 residual: on Windows Claude Code spawns hook commands through +// `cmd.exe /d /s /c`, which never expands $HOME, so `bash $HOME/...` handed +// bash a literal "$HOME/..." path, exited 127, and the hook silently failed +// open. Run every registered freeze/guard/careful command the way cmd.exe +// does (no $ expansion, MSVC argv quoting) and the way POSIX sh does. +function registeredHookCommands(rel: string): string[] { + const fm = fs.readFileSync(path.join(ROOT, rel), 'utf-8').split('\n---')[0]; + return [...fm.matchAll(/^\s*command:\s*(.+)$/gm)].map((m) => { + const v = m[1].trim(); + if (v.startsWith("'")) return v.slice(1, -1).replaceAll("''", "'"); + if (v.startsWith('"')) return JSON.parse(v); + return v; + }); +} + +/** argv as a Windows program (bash.exe) parses its command line from cmd.exe. */ +function msvcArgv(command: string): string[] { + const args: string[] = []; + let cur = ''; + let inQuotes = false; + let started = false; + for (let i = 0; i < command.length; i++) { + const c = command[i]; + if (c === '\\') { + let n = 0; + while (command[i] === '\\') { n++; i++; } + if (command[i] === '"') { + cur += '\\'.repeat(Math.floor(n / 2)); + if (n % 2) cur += '"'; else { inQuotes = !inQuotes; } + } else { + cur += '\\'.repeat(n); + i--; + } + started = true; + } else if (c === '"') { + inQuotes = !inQuotes; + started = true; + } else if ((c === ' ' || c === '\t') && !inQuotes) { + if (started) args.push(cur); + cur = ''; + started = false; + } else { + cur += c; + started = true; + } + } + if (started) args.push(cur); + return args; +} + +function withInstalledHome(fn: (home: string) => T): T { + const home = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-e1-home-')); + try { + fs.mkdirSync(path.join(home, '.claude', 'skills'), { recursive: true }); + fs.symlinkSync(ROOT, path.join(home, '.claude', 'skills', 'gstack'), 'junction'); + return fn(home); + } finally { + fs.rmSync(home, { recursive: true, force: true }); + } +} + +const HOOKED = ['freeze/SKILL.md.tmpl', 'guard/SKILL.md.tmpl', 'careful/SKILL.md.tmpl']; + +describe('E1: registered hook commands run under cmd.exe and POSIX sh (#2354)', () => { + for (const rel of HOOKED) { + const commands = registeredHookCommands(rel); + test(`${rel} registers $HOME-anchored hooks`, () => { + expect(commands.length).toBeGreaterThan(0); + }); + for (const [i, command] of commands.entries()) { + const isFreeze = command.includes('check-freeze.sh'); + const input = isFreeze ? { tool_input: { file_path: `${os.tmpdir()}${path.sep}outside-boundary.txt` } } : { tool_input: { command: 'rm -rf /' } }; + const expected = 'deny'; + const shells: Array<[string, (home: string) => string[]]> = IS_WIN + ? [['cmd.exe', () => ['cmd.exe', '/d', '/s', '/c', `"${command}"`]]] + : [['cmd.exe argv', () => msvcArgv(command)], ['POSIX shell', () => ['sh', '-c', command]]]; + for (const [label, argv] of shells) { + test(`${rel} hook #${i + 1} reaches a decision via ${label}`, () => { + withInstalledHome((home) => withState(path.join(home, 'frozen'), (env) => { + const r = Bun.spawnSync(argv(home), { + stdin: Buffer.from(JSON.stringify(input)), + env: { ...env, HOME: home }, + timeout: 15_000, + }); + expect(r.exitCode, r.stderr.toString()).toBe(0); + expect(decision(r.stdout.toString())).toBe(expected); + })); + }); + } + } + } +});