From 455c805125374e8a193d6a25daa923ca2d33095e Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sat, 15 Aug 2026 23:12:19 -0700 Subject: [PATCH] fix(hooks): fail-closed freeze + shared extractor + careful HIGH tier Freeze boundary hook had four verified bugs: the grep-first JSON extractor truncated at escaped quotes and failed OPEN on unparseable payloads; the deny JSON was printf-interpolated so a quote- or newline-bearing path silently no-oped the block; the freeze path read stripped INTERNAL spaces (a boundary like ~/My Project could never match); and the path resolver skipped the final component, letting an in-boundary symlink write through to an out-of-boundary target. Fixes, structurally: one shared sourced helper (careful/bin/hook-extract.sh) now owns JSON extraction and JSON-encoded decision envelopes for BOTH hooks -- the two-copy drift is how freeze kept a broken extractor after careful's was fixed. Freeze is now deny-tier fail-closed (unparseable payload denies, parsed-but-no-file_path still allows), trims only leading/trailing whitespace, and resolves symlinks through the final path component. Careful gains a HIGH tier (hard deny, simple commands only): recursive delete of /, ~, or $HOME, and force-push to the repo's default branch. Compound commands always fall through to the MEDIUM ask; --force-with-lease is never HIGH. Documented as a best-effort advisory hard-stop, not a policy boundary. Plus additive-only project patterns (~/.gstack/careful-patterns.txt + per-project file): config can only ADD warn rules, never suppress a baseline family. test/hook-scripts.test.ts: 89 tests incl. malformed-payload deny, parseable deny JSON for hostile paths, space-bearing boundaries, symlink escape, HIGH tier splits, additive invariant, invalid-regex resilience. Co-Authored-By: Claude Fable 5 --- careful/SKILL.md | 30 +++- careful/SKILL.md.tmpl | 30 +++- careful/bin/check-careful.sh | 134 +++++++++++++---- careful/bin/hook-extract.sh | 64 ++++++++ freeze/SKILL.md | 14 +- freeze/SKILL.md.tmpl | 14 +- freeze/bin/check-freeze.sh | 65 ++++++-- test/hook-scripts.test.ts | 280 +++++++++++++++++++++++++++++++++-- 8 files changed, 565 insertions(+), 66 deletions(-) create mode 100644 careful/bin/hook-extract.sh diff --git a/careful/SKILL.md b/careful/SKILL.md index 79571df99..a200ea308 100644 --- a/careful/SKILL.md +++ b/careful/SKILL.md @@ -64,6 +64,34 @@ The hook reads the command from the tool input JSON, checks it against the patterns above, and returns a `hookSpecificOutput` payload with `permissionDecision: "ask"` and a warning reason if a match is found (the decision must be nested under `hookSpecificOutput` — Claude Code ignores a -top-level `permissionDecision`). You can always override the warning and proceed. +top-level `permissionDecision`). You can always override a MEDIUM warning and +proceed. + +## HIGH tier (hard deny) + +A tiny set of catastrophic commands is **denied outright** while /careful is +active, not just warned: + +- `rm -r`/`-R` targeting exactly `/`, `~`, or `$HOME` +- `git push --force` / `-f` to the repo's **default branch** + +HIGH only fires on SIMPLE commands (no `;`, `&&`, `||`, `|`, newline) — string +matching cannot resolve what a compound command does, so compound shapes fall +through to the ordinary MEDIUM ask. `--force-with-lease` is deliberately not +matched (it's the safe force variant). This is a best-effort advisory +hard-stop, not a policy boundary: /careful is opt-in and session-scoped, so +the escape hatch is ending the /careful session. + +## Project patterns (additive only) + +Add your own warn rules — one POSIX ERE per line, `#` comments allowed — in: + +- `~/.gstack/careful-patterns.txt` (all projects) +- `~/.gstack/projects//careful-patterns.txt` (this project) + +Matching lines warn with `[careful] Project rule matched: `. Config +can only ADD rules: the files are consulted after the built-in families, so no +file content can suppress or weaken a baseline warning. Invalid regex lines +are skipped. To deactivate, end the conversation or start a new one. Hooks are session-scoped. diff --git a/careful/SKILL.md.tmpl b/careful/SKILL.md.tmpl index 88b4eeab8..5994979e6 100644 --- a/careful/SKILL.md.tmpl +++ b/careful/SKILL.md.tmpl @@ -59,6 +59,34 @@ The hook reads the command from the tool input JSON, checks it against the patterns above, and returns a `hookSpecificOutput` payload with `permissionDecision: "ask"` and a warning reason if a match is found (the decision must be nested under `hookSpecificOutput` — Claude Code ignores a -top-level `permissionDecision`). You can always override the warning and proceed. +top-level `permissionDecision`). You can always override a MEDIUM warning and +proceed. + +## HIGH tier (hard deny) + +A tiny set of catastrophic commands is **denied outright** while /careful is +active, not just warned: + +- `rm -r`/`-R` targeting exactly `/`, `~`, or `$HOME` +- `git push --force` / `-f` to the repo's **default branch** + +HIGH only fires on SIMPLE commands (no `;`, `&&`, `||`, `|`, newline) — string +matching cannot resolve what a compound command does, so compound shapes fall +through to the ordinary MEDIUM ask. `--force-with-lease` is deliberately not +matched (it's the safe force variant). This is a best-effort advisory +hard-stop, not a policy boundary: /careful is opt-in and session-scoped, so +the escape hatch is ending the /careful session. + +## Project patterns (additive only) + +Add your own warn rules — one POSIX ERE per line, `#` comments allowed — in: + +- `~/.gstack/careful-patterns.txt` (all projects) +- `~/.gstack/projects//careful-patterns.txt` (this project) + +Matching lines warn with `[careful] Project rule matched: `. Config +can only ADD rules: the files are consulted after the built-in families, so no +file content can suppress or weaken a baseline warning. Invalid regex lines +are skipped. To deactivate, end the conversation or start a new one. Hooks are session-scoped. diff --git a/careful/bin/check-careful.sh b/careful/bin/check-careful.sh index 3a7110619..abaad5bea 100755 --- a/careful/bin/check-careful.sh +++ b/careful/bin/check-careful.sh @@ -1,14 +1,23 @@ #!/usr/bin/env bash # check-careful.sh — PreToolUse hook for /careful skill # Reads JSON from stdin, checks Bash command for destructive patterns. -# Returns a PreToolUse hookSpecificOutput with permissionDecision "ask" to warn, -# or {} to allow. The decision MUST be nested under hookSpecificOutput — Claude -# Code ignores a top-level permissionDecision, which silently no-ops the warning. +# Two tiers: +# HIGH — a tiny set of catastrophic SIMPLE commands returns "deny" +# (best-effort advisory hard-stop, not a policy boundary). +# MEDIUM — the destructive families below return "ask" (always overridable). +# The decision MUST be nested under hookSpecificOutput — Claude Code ignores a +# top-level permissionDecision, which silently no-ops the warning. set -euo pipefail # Read stdin (JSON with tool_input) INPUT=$(cat) +# Shared JSON helpers (extractor + encoder) — one copy for careful AND freeze. +# See hook-extract.sh for the drift history that motivated the shared file. +_HOOK_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=careful/bin/hook-extract.sh +. "$_HOOK_DIR/hook-extract.sh" + # Extract the "command" field value from tool_input with a real JSON parser. # # The previous extractor was @@ -21,31 +30,17 @@ INPUT=$(cat) # bash -c "rm -rf /" -> CMD='bash -c \' -> allowed # echo "x"; rm -rf ~ -> CMD='echo \' -> allowed # -# The python3 fallback never rescued these because CMD was non-empty, so the -# `[ -z "$CMD" ]` guard did not fire. Parse the payload properly instead, and -# fail CLOSED when it cannot be parsed at all — a hook that gates destructive -# commands must not allow-by-default on unreadable input. -# -# python3 is tried first because it ships with macOS and most Linux distros and -# is reliably on PATH in a hook environment; node is the fallback. -extract_cmd() { - if command -v python3 >/dev/null 2>&1; then - printf '%s' "$INPUT" | python3 -c 'import sys,json; d=json.loads(sys.stdin.read()); c=d.get("tool_input",{}).get("command",""); sys.stdout.write(c if isinstance(c,str) else "")' 2>/dev/null && return 0 - fi - if command -v node >/dev/null 2>&1; then - printf '%s' "$INPUT" | node -e 'let s="";process.stdin.on("data",d=>s+=d).on("end",()=>{try{const j=JSON.parse(s);const c=(j&&j.tool_input&&j.tool_input.command)||"";process.stdout.write(typeof c==="string"?c:"")}catch(e){process.exit(3)}})' 2>/dev/null && return 0 - fi - return 1 -} - +# Parse the payload properly instead, and fail CLOSED when it cannot be parsed +# at all — a hook that gates destructive commands must not allow-by-default on +# unreadable input. set +e -CMD=$(extract_cmd) +CMD=$(gstack_hook_extract_field "$INPUT" command) EXTRACT_RC=$? set -e # No parser available, or the payload is not parseable JSON. Fail closed. if [ "$EXTRACT_RC" -ne 0 ] && [ -n "$INPUT" ]; then - printf '{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"ask","permissionDecisionReason":"[careful] Could not parse the tool payload to safety-check this command. Approve only if you know what it does."}}\n' + gstack_hook_decision ask "[careful] Could not parse the tool payload to safety-check this command. Approve only if you know what it does." exit 0 fi @@ -55,6 +50,12 @@ if [ -z "$CMD" ]; then exit 0 fi +# Log a hook fire event (pattern name only, never command content). +_careful_log_fire() { + mkdir -p ~/.gstack/analytics 2>/dev/null || true + echo '{"event":"hook_fire","skill":"careful","pattern":"'"$1"'","ts":"'$(date -u +%Y-%m-%dT%H:%M:%SZ)'","repo":"'$(basename "$(git rev-parse --show-toplevel 2>/dev/null)" 2>/dev/null || echo "unknown")'"}' >> ~/.gstack/analytics/skill-usage.jsonl 2>/dev/null || true +} + # Normalize: lowercase for case-insensitive SQL matching CMD_LOWER=$(printf '%s' "$CMD" | tr '[:upper:]' '[:lower:]') @@ -71,10 +72,54 @@ CMD_LOWER=$(printf '%s' "$CMD" | tr '[:upper:]' '[:lower:]') # primitives as a reason to ask: they are vanishingly rare in commands a human # actually means to run unattended. if printf '%s' "$CMD" | grep -qE '\$\{IFS\}|\$IFS|\$\(echo[^)]*base64[^)]*\)|base64[[:space:]]+(-d|--decode)[^|]*\|[[:space:]]*(sh|bash)' 2>/dev/null; then - printf '{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"ask","permissionDecisionReason":"[careful] Shell obfuscation detected (IFS word-splitting or base64-to-shell). Read the command carefully before approving."}}\n' + gstack_hook_decision ask "[careful] Shell obfuscation detected (IFS word-splitting or base64-to-shell). Read the command carefully before approving." exit 0 fi +# --- HIGH tier: hard deny (best-effort advisory hard-stop, NOT a policy boundary) --- +# Only SIMPLE commands are eligible: string matching cannot resolve what a +# compound command does (`cd X && git push --force` — whose cwd? which repo?), +# so anything containing ; && || | or a newline falls through to the MEDIUM ask +# families below — conservative failure = ask, never guess. +# --force-with-lease is deliberately NOT matched here (it is the safe variant). +# curl|sh stays MEDIUM/allow territory: hard-denying it would block legitimate +# installer flows, including gstack's own setup pattern. +_IS_SIMPLE=1 +case "$CMD" in + *';'*|*'&&'*|*'||'*|*'|'*|*$'\n'*) _IS_SIMPLE=0 ;; +esac +if [ "$_IS_SIMPLE" -eq 1 ]; then + # Recursive delete aimed at the filesystem root or the whole home directory. + if printf '%s' "$CMD" | grep -qE '^[[:space:]]*(sudo[[:space:]]+)?rm[[:space:]]+(-[a-zA-Z]*[rR][a-zA-Z]*[[:space:]]+)+(/|~|\$HOME)/?[[:space:]]*$' 2>/dev/null; then + _careful_log_fire "high_rm_root" + gstack_hook_decision deny "[careful][HIGH] Recursive delete of / or the home directory is blocked while /careful is active. If you truly mean it, end the /careful session first." + exit 0 + fi + # Force-push to the repo's default branch (the shared history everyone pulls). + if printf '%s' "$CMD" | grep -qE '^[[:space:]]*git[[:space:]]+push([[:space:]]|$)' 2>/dev/null \ + && printf '%s' "$CMD" | grep -qE '(^|[[:space:]])(-f|--force)($|[[:space:]])' 2>/dev/null; then + _DEFAULT_BRANCH=$(git symbolic-ref refs/remotes/origin/HEAD 2>/dev/null | sed 's|.*/||' || true) + if [ -n "$_DEFAULT_BRANCH" ]; then + _TARGETS_DEFAULT=0 + if printf '%s' "$CMD" | grep -qE "(^|[[:space:]])${_DEFAULT_BRANCH}([[:space:]]|$)" 2>/dev/null; then + _TARGETS_DEFAULT=1 + elif printf '%s' "$CMD" | grep -qE '^[[:space:]]*git[[:space:]]+push([[:space:]]+(-f|--force))*[[:space:]]*$' 2>/dev/null; then + # Bare `git push --force` (force flags only, no remote/ref): it targets + # the current branch's upstream, which is the default branch only when + # we are ON it. Any other arg shape (explicit remote + feature ref, + # exotic refspecs) falls through to the MEDIUM ask below. + _CURRENT_BRANCH=$(git branch --show-current 2>/dev/null || true) + [ -n "$_CURRENT_BRANCH" ] && [ "$_CURRENT_BRANCH" = "$_DEFAULT_BRANCH" ] && _TARGETS_DEFAULT=1 + fi + if [ "$_TARGETS_DEFAULT" -eq 1 ]; then + _careful_log_fire "high_force_push_default" + gstack_hook_decision deny "[careful][HIGH] Force-push to the default branch ($_DEFAULT_BRANCH) is blocked while /careful is active. Use --force-with-lease on a feature branch, or end the /careful session if you truly mean it." + exit 0 + fi + fi + fi +fi + # --- Check for safe exceptions (one standalone rm of build artifacts) --- # Match the complete command. Parsing only the last rm is unsafe because shell # syntax or comments can hide an earlier destructive command, for example: @@ -102,7 +147,7 @@ case "$CMD" in ;; esac -# --- Destructive pattern checks --- +# --- Destructive pattern checks (MEDIUM tier — always overridable) --- WARN="" PATTERN="" @@ -154,14 +199,43 @@ if [ -z "$WARN" ] && printf '%s' "$CMD" | grep -qE 'docker\s+(rm\s+-f|system\s+p PATTERN="docker_destructive" fi +# --- Additive project patterns --- +# Config can only ADD warn rules, never remove or weaken a baseline family: +# these files are consulted AFTER the hardcoded checks and only when none of +# them matched, so no file content can suppress a baseline warning. One POSIX +# ERE per line; blank lines and #-comments skipped; an invalid regex is +# skipped (never fatal — the hook must not break on a typo in config). +if [ -z "$WARN" ]; then + _GSTACK_HOME_DIR="${GSTACK_HOME:-$HOME/.gstack}" + _PATTERN_FILES="$_GSTACK_HOME_DIR/careful-patterns.txt" + eval "$("$_HOOK_DIR/../../bin/gstack-slug" 2>/dev/null)" 2>/dev/null || true + if [ -n "${SLUG:-}" ]; then + _PATTERN_FILES="$_PATTERN_FILES +$_GSTACK_HOME_DIR/projects/$SLUG/careful-patterns.txt" + fi + while IFS= read -r _PF; do + [ -f "$_PF" ] || continue + while IFS= read -r _PAT || [ -n "$_PAT" ]; do + case "$_PAT" in ''|'#'*) continue ;; esac + _PAT_RC=0 + printf '' | grep -qE "$_PAT" 2>/dev/null || _PAT_RC=$? + [ "$_PAT_RC" -eq 2 ] && continue # invalid ERE — skip the line + if printf '%s' "$CMD" | grep -qE "$_PAT" 2>/dev/null; then + WARN="Project rule matched: $_PAT" + PATTERN="project_rule" + break + fi + done < "$_PF" + [ -n "$WARN" ] && break + done </dev/null || true - echo '{"event":"hook_fire","skill":"careful","pattern":"'"$PATTERN"'","ts":"'$(date -u +%Y-%m-%dT%H:%M:%SZ)'","repo":"'$(basename "$(git rev-parse --show-toplevel 2>/dev/null)" 2>/dev/null || echo "unknown")'"}' >> ~/.gstack/analytics/skill-usage.jsonl 2>/dev/null || true - - WARN_ESCAPED=$(printf '%s' "$WARN" | sed 's/"/\\"/g') - printf '{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"ask","permissionDecisionReason":"[careful] %s"}}\n' "$WARN_ESCAPED" + _careful_log_fire "$PATTERN" + gstack_hook_decision ask "[careful] $WARN" else echo '{}' fi diff --git a/careful/bin/hook-extract.sh b/careful/bin/hook-extract.sh new file mode 100644 index 000000000..fd71a446b --- /dev/null +++ b/careful/bin/hook-extract.sh @@ -0,0 +1,64 @@ +#!/usr/bin/env bash +# hook-extract.sh — SHARED JSON 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. +# +# ONE copy on purpose. These two hooks previously carried separate extractor +# copies; the escaped-quote truncation bug got fixed in careful's copy while +# freeze silently kept the broken one. Any future parsing fix lands here and +# reaches both hooks by construction. + +# gstack_hook_extract_field PAYLOAD FIELD +# Prints tool_input.FIELD when PAYLOAD is valid JSON and the field is a +# string ("" when absent or non-string). Returns 1 when no parser is +# available or the payload is not parseable JSON — the CALLER decides the +# polarity for that case (careful asks, freeze denies). +# +# python3 is tried first because it ships with macOS and most Linux distros +# and is reliably on PATH in a hook environment; node is the fallback. +gstack_hook_extract_field() { + _ghef_payload="$1" + _ghef_field="$2" + if command -v python3 >/dev/null 2>&1; then + printf '%s' "$_ghef_payload" | python3 -c 'import sys,json +field = sys.argv[1] +d = json.loads(sys.stdin.read()) +c = d.get("tool_input", {}).get(field, "") +sys.stdout.write(c if isinstance(c, str) else "")' "$_ghef_field" 2>/dev/null && return 0 + fi + if command -v node >/dev/null 2>&1; then + printf '%s' "$_ghef_payload" | node -e 'let s="";process.stdin.on("data",d=>s+=d).on("end",()=>{try{const j=JSON.parse(s);const c=(j&&j.tool_input&&j.tool_input[process.argv[1]])||"";process.stdout.write(typeof c==="string"?c:"")}catch(e){process.exit(3)}})' "$_ghef_field" 2>/dev/null && return 0 + fi + return 1 +} + +# gstack_hook_json_string TEXT +# Prints TEXT as a JSON string literal (surrounding quotes included), +# encoding quotes, backslashes, control characters and newlines. Never build +# hook JSON with printf/sed interpolation: a path containing a quote or a +# newline produces malformed JSON, and Claude Code silently ignores the +# whole decision — a deny that no-ops exactly when it matters. +gstack_hook_json_string() { + _ghjs_text="$1" + if command -v python3 >/dev/null 2>&1; then + printf '%s' "$_ghjs_text" | python3 -c 'import sys,json; sys.stdout.write(json.dumps(sys.stdin.read()))' 2>/dev/null && return 0 + fi + if command -v node >/dev/null 2>&1; then + printf '%s' "$_ghjs_text" | node -e 'let s="";process.stdin.on("data",d=>s+=d).on("end",()=>process.stdout.write(JSON.stringify(s)))' 2>/dev/null && return 0 + fi + # Last-resort fallback (no parser on PATH): strip to a safe charset so the + # envelope stays valid JSON even if the message loses characters. + printf '"%s"' "$(printf '%s' "$_ghjs_text" | tr -cd 'a-zA-Z0-9 ._/:@=+-' )" +} + +# gstack_hook_decision DECISION REASON +# Emits the full PreToolUse hookSpecificOutput envelope with REASON safely +# JSON-encoded. DECISION is "ask" or "deny". The decision MUST be nested +# under hookSpecificOutput — Claude Code ignores a top-level +# permissionDecision, which silently no-ops the block. +gstack_hook_decision() { + _ghd_decision="$1" + _ghd_reason="$2" + _ghd_encoded=$(gstack_hook_json_string "$_ghd_reason") + printf '{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"%s","permissionDecisionReason":%s}}\n' "$_ghd_decision" "$_ghd_encoded" +} diff --git a/freeze/SKILL.md b/freeze/SKILL.md index c00006236..f671cb48e 100644 --- a/freeze/SKILL.md +++ b/freeze/SKILL.md @@ -76,14 +76,22 @@ again. To remove it, run `/unfreeze` or end the session." ## How it works -The hook reads `file_path` from the Edit/Write tool input JSON, then checks -whether the path starts with the freeze directory. If not, it returns a +The hook reads `file_path` from the Edit/Write tool input JSON (shared +real-JSON extractor with /careful — one copy, sourced by both hooks), then +checks whether the path starts with the freeze directory. If not, it returns a `hookSpecificOutput` payload with `permissionDecision: "deny"` to block the operation (nested under `hookSpecificOutput` — Claude Code ignores a top-level `permissionDecision`). +Polarity is fail-closed: a tool payload the hook cannot parse is DENIED, not +allowed — a boundary that fails open is not a boundary. A payload that parses +but has no `file_path` (a non-file tool) is allowed. Symlinks are resolved +through their FINAL component, so an in-boundary symlink pointing outside the +boundary is checked against its target. + The freeze boundary persists for the session via the state file. The hook -script reads it on every Edit/Write invocation. +script reads it on every Edit/Write invocation. Boundaries containing spaces +are supported. ## Notes diff --git a/freeze/SKILL.md.tmpl b/freeze/SKILL.md.tmpl index 7a38878c8..e6804d91c 100644 --- a/freeze/SKILL.md.tmpl +++ b/freeze/SKILL.md.tmpl @@ -71,14 +71,22 @@ again. To remove it, run `/unfreeze` or end the session." ## How it works -The hook reads `file_path` from the Edit/Write tool input JSON, then checks -whether the path starts with the freeze directory. If not, it returns a +The hook reads `file_path` from the Edit/Write tool input JSON (shared +real-JSON extractor with /careful — one copy, sourced by both hooks), then +checks whether the path starts with the freeze directory. If not, it returns a `hookSpecificOutput` payload with `permissionDecision: "deny"` to block the operation (nested under `hookSpecificOutput` — Claude Code ignores a top-level `permissionDecision`). +Polarity is fail-closed: a tool payload the hook cannot parse is DENIED, not +allowed — a boundary that fails open is not a boundary. A payload that parses +but has no `file_path` (a non-file tool) is allowed. Symlinks are resolved +through their FINAL component, so an in-boundary symlink pointing outside the +boundary is checked against its target. + The freeze boundary persists for the session via the state file. The hook -script reads it on every Edit/Write invocation. +script reads it on every Edit/Write invocation. Boundaries containing spaces +are supported. ## Notes diff --git a/freeze/bin/check-freeze.sh b/freeze/bin/check-freeze.sh index f26c07578..375f3f596 100755 --- a/freeze/bin/check-freeze.sh +++ b/freeze/bin/check-freeze.sh @@ -4,11 +4,24 @@ # Returns a PreToolUse hookSpecificOutput with permissionDecision "deny" to block, # or {} to allow. The decision MUST be nested under hookSpecificOutput — Claude # Code ignores a top-level permissionDecision, which silently no-ops the block. +# +# Polarity: freeze is a DENY-tier hook, so an unreadable payload DENIES +# (fail closed). A payload that parses but has no file_path is a non-file +# tool — allow. This is the opposite edge-handling from careful's ask-tier +# and intentionally so: /guard runs both, and a boundary that fails open is +# not a boundary. set -euo pipefail # Read stdin INPUT=$(cat) +# Shared JSON helpers (extractor + encoder) — one copy for careful AND freeze. +# freeze previously carried its own grep-first extractor which truncated at +# escaped quotes and failed OPEN; the shared file kills that drift class. +_HOOK_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=careful/bin/hook-extract.sh +. "$_HOOK_DIR/../../careful/bin/hook-extract.sh" + # Locate the freeze directory state file STATE_DIR="${CLAUDE_PLUGIN_DATA:-$HOME/.gstack}" FREEZE_FILE="$STATE_DIR/freeze-dir.txt" @@ -19,7 +32,11 @@ if [ ! -f "$FREEZE_FILE" ]; then exit 0 fi -FREEZE_DIR=$(tr -d '[:space:]' < "$FREEZE_FILE") +# First line, trimmed of LEADING/TRAILING whitespace only. The previous +# `tr -d '[:space:]'` deleted INTERNAL spaces too, so a boundary like +# "~/My Project/src" could never match anything — every edit denied (or the +# mangled path accidentally allowed the wrong tree). +FREEZE_DIR=$(head -n 1 "$FREEZE_FILE" 2>/dev/null | sed 's/^[[:space:]]*//;s/[[:space:]]*$//') # If freeze dir is empty, allow if [ -z "$FREEZE_DIR" ]; then @@ -27,16 +44,20 @@ if [ -z "$FREEZE_DIR" ]; then exit 0 fi -# Extract file_path from tool_input JSON -# Try grep/sed first, fall back to Python for escaped quotes -FILE_PATH=$(printf '%s' "$INPUT" | grep -o '"file_path"[[:space:]]*:[[:space:]]*"[^"]*"' | head -1 | sed 's/.*:[[:space:]]*"//;s/"$//' || true) +# Extract file_path from tool_input with the shared real-JSON parser. +set +e +FILE_PATH=$(gstack_hook_extract_field "$INPUT" file_path) +EXTRACT_RC=$? +set -e -# Python fallback if grep returned empty -if [ -z "$FILE_PATH" ]; then - FILE_PATH=$(printf '%s' "$INPUT" | python3 -c 'import sys,json; print(json.loads(sys.stdin.read()).get("tool_input",{}).get("file_path",""))' 2>/dev/null || true) +# Unparseable payload (or no parser available): DENY. A boundary hook that +# allows what it cannot read is not a boundary. +if [ "$EXTRACT_RC" -ne 0 ] && [ -n "$INPUT" ]; then + gstack_hook_decision deny "[freeze] Could not parse the tool payload to check the freeze boundary. Blocked (fail closed). Freeze boundary: $FREEZE_DIR" + exit 0 fi -# If we couldn't extract a file path, allow (don't block on parse failure) +# Parsed fine but no file_path field: a non-file tool payload — allow. if [ -z "$FILE_PATH" ]; then echo '{}' exit 0 @@ -53,11 +74,26 @@ esac # Normalize: remove double slashes and trailing slash FILE_PATH=$(printf '%s' "$FILE_PATH" | sed 's|/\+|/|g;s|/$||') -# Resolve symlinks and .. sequences (POSIX-portable, works on macOS) +# Resolve symlinks and .. sequences (POSIX-portable, works on macOS). +# The FULL path is resolved, including the FINAL component: the previous +# version resolved only the parent directory, so an in-boundary symlink +# pointing at an out-of-boundary target sailed through the check while the +# actual write landed outside the boundary. A final component that is a +# symlink is followed (bounded, cycle-safe) so the TARGET gets checked; a +# final component that does not exist yet (new file) has nothing to follow +# and parent resolution is the correct behavior. _resolve_path() { - local _dir _base - _dir="$(dirname "$1")" - _base="$(basename "$1")" + local _p="$1" _dir _base _tgt _i=0 + while [ -L "$_p" ] && [ "$_i" -lt 40 ]; do + _tgt=$(readlink "$_p" 2>/dev/null) || break + case "$_tgt" in + /*) _p="$_tgt" ;; + *) _p="$(dirname "$_p")/$_tgt" ;; + esac + _i=$((_i + 1)) + done + _dir="$(dirname "$_p")" + _base="$(basename "$_p")" _dir="$(cd "$_dir" 2>/dev/null && pwd -P || printf '%s' "$_dir")" printf '%s/%s' "$_dir" "$_base" } @@ -76,6 +112,9 @@ case "$FILE_PATH" in mkdir -p ~/.gstack/analytics 2>/dev/null || true echo '{"event":"hook_fire","skill":"freeze","pattern":"boundary_deny","ts":"'$(date -u +%Y-%m-%dT%H:%M:%SZ)'","repo":"'$(basename "$(git rev-parse --show-toplevel 2>/dev/null)" 2>/dev/null || echo "unknown")'"}' >> ~/.gstack/analytics/skill-usage.jsonl 2>/dev/null || true - printf '{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"deny","permissionDecisionReason":"[freeze] Blocked: %s is outside the freeze boundary (%s). Only edits within the frozen directory are allowed."}}\n' "$FILE_PATH" "$FREEZE_DIR" + # The reason is JSON-encoded by the shared helper. Never interpolate paths + # into hand-built JSON: a path containing a quote or newline produced + # malformed JSON here, and the deny silently no-oped. + gstack_hook_decision deny "[freeze] Blocked: $FILE_PATH is outside the freeze boundary ($FREEZE_DIR). Only edits within the frozen directory are allowed." ;; esac diff --git a/test/hook-scripts.test.ts b/test/hook-scripts.test.ts index 96db60e5a..25191b63e 100644 --- a/test/hook-scripts.test.ts +++ b/test/hook-scripts.test.ts @@ -8,11 +8,12 @@ const ROOT = path.resolve(import.meta.dir, '..'); const CAREFUL_SCRIPT = path.join(ROOT, 'careful', 'bin', 'check-careful.sh'); const FREEZE_SCRIPT = path.join(ROOT, 'freeze', 'bin', 'check-freeze.sh'); -function runHook(scriptPath: string, input: object, env?: Record): { exitCode: number; output: any; raw: string } { +function runHook(scriptPath: string, input: object, env?: Record, cwd?: string): { exitCode: number; output: any; raw: string } { const result = spawnSync('bash', [scriptPath], { input: JSON.stringify(input), stdio: ['pipe', 'pipe', 'pipe'], env: { ...process.env, ...env }, + cwd, timeout: 5000, }); const raw = result.stdout.toString().trim(); @@ -23,6 +24,25 @@ function runHook(scriptPath: string, input: object, env?: Record return { exitCode: result.status ?? 1, output, raw }; } +// Scratch git repo with a resolvable origin default branch — the HIGH-tier +// force-push check reads `git symbolic-ref refs/remotes/origin/HEAD` from the +// hook's cwd, and Conductor worktrees don't reliably carry that ref. +function withGitRepo(defaultBranch: string, currentBranch: string, fn: (repoDir: string) => void) { + const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-careful-git-')); + try { + const git = (args: string[]) => + spawnSync('git', ['-c', 'user.email=t@test', '-c', 'user.name=t', ...args], { cwd: repoDir, timeout: 5000 }); + git(['init', '-q', '-b', defaultBranch]); + git(['commit', '--allow-empty', '-q', '-m', 'init']); + // A symbolic ref may dangle; the hook only reads its NAME. + git(['symbolic-ref', 'refs/remotes/origin/HEAD', `refs/remotes/origin/${defaultBranch}`]); + if (currentBranch !== defaultBranch) git(['checkout', '-q', '-b', currentBranch]); + fn(repoDir); + } finally { + fs.rmSync(repoDir, { recursive: true, force: true }); + } +} + function runHookRaw(scriptPath: string, rawInput: string, env?: Record): { exitCode: number; output: any; raw: string } { const result = spawnSync('bash', [scriptPath], { input: rawInput, @@ -161,12 +181,13 @@ describe('check-careful.sh', () => { // Capital -R is the documented recursive flag on BSD rm (macOS) and accepted // by GNU rm. Both greps previously required a lowercase r, so `rm -R /` - // silently allowed. - test('rm -R / warns (capital -R recursive)', () => { + // silently allowed. A bare recursive delete of / is now HIGH-tier: denied, + // not asked. + test('rm -R / denies (HIGH tier: recursive delete of root)', () => { const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('rm -R /')); expect(exitCode).toBe(0); - expect(output.hookSpecificOutput?.permissionDecision).toBe('ask'); - expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('recursive delete'); + expect(output.hookSpecificOutput?.permissionDecision).toBe('deny'); + expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('HIGH'); }); test('rm -fR /home/user warns (capital R in flag cluster)', () => { @@ -326,18 +347,26 @@ describe('check-careful.sh', () => { // --- Git destructive commands --- describe('git destructive commands', () => { - test('git push --force warns with force-push', () => { - const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('git push --force origin main')); - expect(exitCode).toBe(0); - expect(output.hookSpecificOutput?.permissionDecision).toBe('ask'); - expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('force-push'); + // Force-push to a NON-default branch is MEDIUM (ask). Force-push to the + // default branch is HIGH (deny) — covered in the HIGH tier describe. The + // fixture repo pins the default branch so the split is deterministic + // regardless of the host repo's origin/HEAD. + test('git push --force warns with force-push (non-default target)', () => { + withGitRepo('trunk', 'trunk', (repoDir) => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('git push --force origin main'), undefined, repoDir); + expect(exitCode).toBe(0); + expect(output.hookSpecificOutput?.permissionDecision).toBe('ask'); + expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('force-push'); + }); }); - test('git push -f warns', () => { - const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('git push -f origin main')); - expect(exitCode).toBe(0); - expect(output.hookSpecificOutput?.permissionDecision).toBe('ask'); - expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('force-push'); + test('git push -f warns (non-default target)', () => { + withGitRepo('trunk', 'trunk', (repoDir) => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('git push -f origin main'), undefined, repoDir); + expect(exitCode).toBe(0); + expect(output.hookSpecificOutput?.permissionDecision).toBe('ask'); + expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('force-push'); + }); }); test('git reset --hard warns with uncommitted', () => { @@ -443,6 +472,132 @@ describe('check-careful.sh', () => { expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('recursive delete'); }); }); + + // --- HIGH tier (hard deny) --- + // A tiny set of catastrophic SIMPLE commands is denied outright while + // /careful is active. Best-effort advisory hard-stop, not a policy boundary: + // compound commands always fall through to the MEDIUM ask. + + describe('HIGH tier (hard deny)', () => { + test.each(['rm -rf /', 'rm -rf ~', 'rm -rf $HOME', 'sudo rm -rf /', 'rm -Rf ~/'])( + 'denies catastrophic recursive delete: %s', + (command) => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput(command)); + expect(exitCode).toBe(0); + expect(output.hookSpecificOutput?.permissionDecision).toBe('deny'); + expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('HIGH'); + }, + ); + + test('rm -rf ~/subdir stays MEDIUM ask (not the whole home dir)', () => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('rm -rf ~/subdir')); + expect(exitCode).toBe(0); + expect(output.hookSpecificOutput?.permissionDecision).toBe('ask'); + }); + + test('git push --force origin denies', () => { + withGitRepo('main', 'feature', (repoDir) => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('git push --force origin main'), undefined, repoDir); + expect(exitCode).toBe(0); + expect(output.hookSpecificOutput?.permissionDecision).toBe('deny'); + expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('default branch'); + }); + }); + + test('bare git push --force while ON the default branch denies', () => { + withGitRepo('main', 'main', (repoDir) => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('git push --force'), undefined, repoDir); + expect(exitCode).toBe(0); + expect(output.hookSpecificOutput?.permissionDecision).toBe('deny'); + expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('HIGH'); + }); + }); + + test('bare git push --force on a feature branch asks (MEDIUM)', () => { + withGitRepo('main', 'feature', (repoDir) => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('git push --force'), undefined, repoDir); + expect(exitCode).toBe(0); + expect(output.hookSpecificOutput?.permissionDecision).toBe('ask'); + expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('force-push'); + }); + }); + + test('git push -f origin feature asks (MEDIUM — not the default branch)', () => { + withGitRepo('main', 'main', (repoDir) => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('git push -f origin feature'), undefined, repoDir); + expect(exitCode).toBe(0); + expect(output.hookSpecificOutput?.permissionDecision).toBe('ask'); + expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('force-push'); + }); + }); + + test('compound force-push falls through to ask, never deny (cannot resolve cwd)', () => { + withGitRepo('main', 'main', (repoDir) => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('cd elsewhere && git push --force origin main'), undefined, repoDir); + expect(exitCode).toBe(0); + expect(output.hookSpecificOutput?.permissionDecision).toBe('ask'); + }); + }); + + test('--force-with-lease is never HIGH (the safe force variant)', () => { + withGitRepo('main', 'main', (repoDir) => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('git push --force-with-lease origin main'), undefined, repoDir); + expect(exitCode).toBe(0); + expect(output.hookSpecificOutput?.permissionDecision).not.toBe('deny'); + }); + }); + }); + + // --- Additive project patterns --- + // Config can only ADD warn rules. The files are consulted after the baseline + // families, so no file content can suppress a baseline match. + + describe('additive project patterns', () => { + function withPatternFile(content: string, fn: (gstackHome: string) => void) { + const gstackHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-careful-pat-')); + fs.writeFileSync(path.join(gstackHome, 'careful-patterns.txt'), content); + try { + fn(gstackHome); + } finally { + fs.rmSync(gstackHome, { recursive: true, force: true }); + } + } + + test('a project pattern adds an ask rule', () => { + withPatternFile('# infra safety\nterraform\\s+destroy\n', (gstackHome) => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('terraform destroy -auto-approve'), { GSTACK_HOME: gstackHome }); + expect(exitCode).toBe(0); + expect(output.hookSpecificOutput?.permissionDecision).toBe('ask'); + expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('Project rule'); + }); + }); + + test('a garbage pattern file cannot suppress a baseline match (additive invariant)', () => { + withPatternFile('# override: allow everything\nallow-everything\nignore baseline\n', (gstackHome) => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('rm -rf /var/data'), { GSTACK_HOME: gstackHome }); + expect(exitCode).toBe(0); + expect(output.hookSpecificOutput?.permissionDecision).toBe('ask'); + expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('recursive delete'); + }); + }); + + test('an invalid regex line is skipped without breaking the hook', () => { + withPatternFile('([unclosed\nterraform\\s+destroy\n', (gstackHome) => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('terraform destroy'), { GSTACK_HOME: gstackHome }); + expect(exitCode).toBe(0); + expect(output.hookSpecificOutput?.permissionDecision).toBe('ask'); + expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('Project rule'); + }); + }); + + test('safe commands still allow with a pattern file present', () => { + withPatternFile('terraform\\s+destroy\n', (gstackHome) => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('ls -la'), { GSTACK_HOME: gstackHome }); + expect(exitCode).toBe(0); + expect(output.hookSpecificOutput?.permissionDecision).toBeUndefined(); + }); + }); + }); }); // ============================================================ @@ -550,5 +705,100 @@ describe('check-freeze.sh', () => { expect(output.hookSpecificOutput?.permissionDecision).toBeUndefined(); }); }); + + test('malformed JSON payload DENIES (fail closed — freeze is a deny-tier hook)', () => { + withFreezeDir('/Users/dev/project/src/', (stateDir) => { + const { exitCode, output } = runHookRaw( + FREEZE_SCRIPT, + 'not json at all {{{{', + { CLAUDE_PLUGIN_DATA: stateDir }, + ); + expect(exitCode).toBe(0); + expect(output.hookSpecificOutput?.permissionDecision).toBe('deny'); + expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('fail closed'); + }); + }); + + test('a quote-bearing path outside the boundary emits PARSEABLE deny JSON', () => { + // The old printf-interpolated deny emitted malformed JSON for paths + // containing quotes — Claude Code silently ignored the whole decision, + // so the deny no-oped exactly when the path was hostile. + withFreezeDir('/Users/dev/project/src/', (stateDir) => { + const { exitCode, output, raw } = runHook( + FREEZE_SCRIPT, + freezeInput('/tmp/evil"quoted/x.ts'), + { CLAUDE_PLUGIN_DATA: stateDir }, + ); + expect(exitCode).toBe(0); + expect(() => JSON.parse(raw)).not.toThrow(); + expect(output.hookSpecificOutput?.permissionDecision).toBe('deny'); + }); + }); + + test('a newline-bearing path outside the boundary emits PARSEABLE deny JSON', () => { + withFreezeDir('/Users/dev/project/src/', (stateDir) => { + const { exitCode, output, raw } = runHook( + FREEZE_SCRIPT, + freezeInput('/tmp/evil\npath.ts'), + { CLAUDE_PLUGIN_DATA: stateDir }, + ); + expect(exitCode).toBe(0); + expect(() => JSON.parse(raw)).not.toThrow(); + expect(output.hookSpecificOutput?.permissionDecision).toBe('deny'); + }); + }); + }); + + describe('space-bearing freeze boundary', () => { + // The old `tr -d '[:space:]'` stripped INTERNAL spaces from the freeze + // path, so a boundary like ".../My Project/src" never matched anything. + test('a boundary containing spaces allows edits inside it', () => { + const base = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-freeze-space-')); + const boundary = path.join(base, 'My Project', 'src'); + fs.mkdirSync(boundary, { recursive: true }); + try { + withFreezeDir(boundary + '/', (stateDir) => { + const inside = runHook(FREEZE_SCRIPT, freezeInput(path.join(boundary, 'index.ts')), { CLAUDE_PLUGIN_DATA: 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 }); + expect(outside.exitCode).toBe(0); + expect(outside.output.hookSpecificOutput?.permissionDecision).toBe('deny'); + }); + } finally { + fs.rmSync(base, { recursive: true, force: true }); + } + }); + }); + + describe('symlink boundary escape', () => { + // The old resolver followed the parent directory but NOT the final path + // component, so an in-boundary symlink pointing outside the boundary was + // allowed while the write landed outside. + test('an in-boundary symlink to an outside target denies', () => { + const base = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-freeze-link-')); + const boundary = path.join(base, 'boundary'); + const outside = path.join(base, 'outside'); + fs.mkdirSync(boundary, { recursive: true }); + fs.mkdirSync(outside, { recursive: true }); + fs.writeFileSync(path.join(outside, 'secret.txt'), 'x'); + 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 }); + 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 }); + expect(real.exitCode).toBe(0); + expect(real.output.hookSpecificOutput?.permissionDecision).toBeUndefined(); + }); + } finally { + fs.rmSync(base, { recursive: true, force: true }); + } + }); }); });