From 162e179872abac35c3a53f55e7c54c3819e55cd8 Mon Sep 17 00:00:00 2001 From: wtamminga Date: Fri, 31 Jul 2026 21:08:04 -0500 Subject: [PATCH 1/3] fix(careful): parse the tool payload as JSON so quotes cannot hide a command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit check-careful.sh extracted the command with grep -o '"command"[[:space:]]*:[[:space:]]*"[^"]*"' and [^"]* stops at the first escaped quote inside the JSON string value. Any destructive command preceded by a quoted argument was truncated away before the pattern checks ran: git commit -m "wip" && rm -rf / -> CMD='git commit -m \' -> allowed bash -c "rm -rf /" -> CMD='bash -c \' -> allowed echo "x"; rm -rf ~ -> CMD='echo \' -> allowed The python3 fallback could not rescue these: it only ran when CMD was empty, and truncation leaves CMD non-empty. So the hook returned {} and the command went through unwarned. This also silently defeated the SQL checks, which is visible in this repo's own tests — the SQL block carried a note that `psql -c "DROP TABLE"` could not be used because the extractor truncated it, so those cases were written without quotes, in a shape nobody actually types. Parse the payload with python3 (falling back to node) and fail CLOSED when it cannot be parsed at all: a hook whose job is gating destructive commands should not allow-by-default on unreadable input. A well-formed payload with no command field, or a non-string command, still allows. Tests: the four quoted-bypass shapes now warn; quoted SQL statements are inspected; unparseable input asks; no-command and non-string-command payloads still allow; `echo "hello world"` still allows. --- careful/bin/check-careful.sh | 47 +++++++++++++++++++++++----- test/hook-scripts.test.ts | 60 +++++++++++++++++++++++++++++++++--- 2 files changed, 95 insertions(+), 12 deletions(-) diff --git a/careful/bin/check-careful.sh b/careful/bin/check-careful.sh index 22bf8b9226..c932bf3226 100755 --- a/careful/bin/check-careful.sh +++ b/careful/bin/check-careful.sh @@ -7,16 +7,47 @@ set -euo pipefail # Read stdin (JSON with tool_input) INPUT=$(cat) -# Extract the "command" field value from tool_input -# Try grep/sed first (handles 99% of cases), fall back to Python for escaped quotes -CMD=$(printf '%s' "$INPUT" | grep -o '"command"[[:space:]]*:[[:space:]]*"[^"]*"' | head -1 | sed 's/.*:[[:space:]]*"//;s/"$//' || true) - -# Python fallback if grep returned empty (e.g., escaped quotes in command) -if [ -z "$CMD" ]; then - CMD=$(printf '%s' "$INPUT" | python3 -c 'import sys,json; print(json.loads(sys.stdin.read()).get("tool_input",{}).get("command",""))' 2>/dev/null || true) +# Extract the "command" field value from tool_input with a real JSON parser. +# +# The previous extractor was +# grep -o '"command"[[:space:]]*:[[:space:]]*"[^"]*"' +# whose [^"]* stops at the first escaped quote in the JSON string value. Any +# destructive command preceded by a quoted argument was therefore truncated +# away before the pattern checks ever ran: +# +# git commit -m "wip" && rm -rf / -> CMD='git commit -m \' -> allowed +# 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 +} + +set +e +CMD=$(extract_cmd) +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 '{"permissionDecision":"ask","message":"[careful] Could not parse the tool payload to safety-check this command. Approve only if you know what it does."}\n' + exit 0 fi -# If we still couldn't extract a command, allow +# Parsed fine, but there is genuinely no command field (non-Bash payload) — allow. if [ -z "$CMD" ]; then echo '{}' exit 0 diff --git a/test/hook-scripts.test.ts b/test/hook-scripts.test.ts index db2e7629f3..10a8de6c43 100644 --- a/test/hook-scripts.test.ts +++ b/test/hook-scripts.test.ts @@ -111,13 +111,55 @@ describe('check-careful.sh', () => { expect(output.permissionDecision).toBe('ask'); expect(output.message).toContain('recursive delete'); }); + + test.each([ + 'git commit -m "wip" && rm -rf /', + 'bash -c "rm -rf /"', + 'echo "x"; rm -rf ~', + 'npm run build --msg "done" && rm -rf /', + ])('a quoted argument cannot hide a later destructive command: %s', (command) => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput(command)); + expect(exitCode).toBe(0); + expect(output.permissionDecision).toBe('ask'); + expect(output.message).toContain('recursive delete'); + }); + }); + + // --- JSON payload extraction --- + + describe('command extraction', () => { + test('fails closed when the payload is not valid JSON', () => { + const { exitCode, output } = runHookRaw(CAREFUL_SCRIPT, 'this is not json'); + expect(exitCode).toBe(0); + expect(output.permissionDecision).toBe('ask'); + expect(output.message).toContain('parse'); + }); + + test('allows a well-formed payload with no command field', () => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, { tool_input: { file_path: '/tmp/x' } }); + expect(exitCode).toBe(0); + expect(output.permissionDecision).toBeUndefined(); + }); + + test('allows when command is present but not a string', () => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, { tool_input: { command: 42 } }); + expect(exitCode).toBe(0); + expect(output.permissionDecision).toBeUndefined(); + }); + + test('preserves escaped quotes in the extracted command', () => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('echo "hello world"')); + expect(exitCode).toBe(0); + expect(output.permissionDecision).toBeUndefined(); + }); }); // --- SQL destructive commands --- - // Note: SQL commands that contain embedded double quotes (e.g., psql -c "DROP TABLE") - // get their command value truncated by the grep-based JSON extractor because \" - // terminates the [^"]* match. We use commands WITHOUT embedded quotes so the grep - // extraction works and the SQL keywords are visible to the pattern matcher. + // Embedded double quotes are now safe to use here. They previously truncated the + // extracted command (the grep-based extractor stopped at the first \"), which hid + // the SQL keyword from the pattern matcher — so these tests had to be written + // without quotes, in a shape no one actually types. The JSON-parser extraction + // fixed that, and the quoted forms below are the realistic ones. describe('SQL destructive commands', () => { test('psql DROP TABLE warns with DROP in message', () => { @@ -127,6 +169,16 @@ describe('check-careful.sh', () => { expect(output.message).toContain('DROP'); }); + test.each([ + 'psql -c "DROP TABLE users"', + 'psql -c "TRUNCATE orders"', + 'mysql -e "DROP DATABASE prod"', + ])('a quoted SQL statement is still inspected: %s', (command) => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput(command)); + expect(exitCode).toBe(0); + expect(output.permissionDecision).toBe('ask'); + }); + test('mysql drop database warns (case insensitive)', () => { const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('mysql -e drop database mydb')); expect(exitCode).toBe(0); From 2a033fd094a5449dc809b176c0e184ab4a895934 Mon Sep 17 00:00:00 2001 From: wtamminga Date: Fri, 31 Jul 2026 21:08:41 -0500 Subject: [PATCH 2/3] fix(careful): treat rm -R as recursive, not just -r MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit rm accepts -R as an exact synonym for -r, so `rm -R /`, `rm -Rf ~` and `rm -fR /` are full recursive deletes. The destructive check matched `-[a-zA-Z]*r` — lowercase only — so all three classified as non-recursive and were allowed without a warning. Widen the flag class to [rR] in both the destructive check and the build-artifact safe exception. Doing both keeps `rm -Rf node_modules` allowed rather than turning it into a false positive. Tests: the three uppercase forms warn; `rm -Rf node_modules` still allows. --- careful/bin/check-careful.sh | 6 ++++-- test/hook-scripts.test.ts | 17 +++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/careful/bin/check-careful.sh b/careful/bin/check-careful.sh index c932bf3226..db6857ec6b 100755 --- a/careful/bin/check-careful.sh +++ b/careful/bin/check-careful.sh @@ -61,7 +61,7 @@ CMD_LOWER=$(printf '%s' "$CMD" | tr '[:upper:]' '[:lower:]') # syntax or comments can hide an earlier destructive command, for example: # rm -rf / # rm -rf node_modules # Unknown syntax fails closed and falls through to the destructive checks. -if printf '%s' "$CMD" | grep -qE '^[[:space:]]*rm[[:space:]]+(-[a-zA-Z]*r[a-zA-Z]*[[:space:]]+|--recursive[[:space:]]+)(([^[:space:];&|#]*/)?(node_modules|\.next|dist|__pycache__|\.cache|build|\.turbo|coverage)[[:space:]]*)+$' 2>/dev/null; then +if printf '%s' "$CMD" | grep -qE '^[[:space:]]*rm[[:space:]]+(-[a-zA-Z]*[rR][a-zA-Z]*[[:space:]]+|--recursive[[:space:]]+)(([^[:space:];&|#]*/)?(node_modules|\.next|dist|__pycache__|\.cache|build|\.turbo|coverage)[[:space:]]*)+$' 2>/dev/null; then echo '{}' exit 0 fi @@ -71,7 +71,9 @@ WARN="" PATTERN="" # rm -rf / rm -r / rm --recursive -if printf '%s' "$CMD" | grep -qE 'rm\s+(-[a-zA-Z]*r|--recursive)' 2>/dev/null; then +# [rR] because rm accepts -R as an equal synonym for -r (and -fR, -Rf); matching +# only lowercase let `rm -R /` through as a non-recursive command. +if printf '%s' "$CMD" | grep -qE 'rm\s+(-[a-zA-Z]*[rR]|--recursive)' 2>/dev/null; then WARN="Destructive: recursive delete (rm -r). This permanently removes files." PATTERN="rm_recursive" fi diff --git a/test/hook-scripts.test.ts b/test/hook-scripts.test.ts index 10a8de6c43..1eef42082d 100644 --- a/test/hook-scripts.test.ts +++ b/test/hook-scripts.test.ts @@ -112,6 +112,23 @@ describe('check-careful.sh', () => { expect(output.message).toContain('recursive delete'); }); + test.each([ + 'rm -R /', + 'rm -Rf ~', + 'rm -fR /var/data', + ])('uppercase -R is recursive too: %s', (command) => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput(command)); + expect(exitCode).toBe(0); + expect(output.permissionDecision).toBe('ask'); + expect(output.message).toContain('recursive delete'); + }); + + test('rm -Rf node_modules still allows (safe exception accepts -R)', () => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('rm -Rf node_modules')); + expect(exitCode).toBe(0); + expect(output.permissionDecision).toBeUndefined(); + }); + test.each([ 'git commit -m "wip" && rm -rf /', 'bash -c "rm -rf /"', From a6fedde9e25b219371c4a9b6683453fa871cdc7d Mon Sep 17 00:00:00 2001 From: wtamminga Date: Fri, 31 Jul 2026 21:09:34 -0500 Subject: [PATCH 3/3] fix(careful): ask when a command hides its shape behind shell expansion Every check in this hook inspects the command as a string, but bash executes what the string means after expansion. ${IFS} holds the default field separator and contains no literal whitespace, so rm${IFS}-rf${IFS}/ matches none of the `rm\s+` patterns while executing as a full recursive delete. A command assembled by a base64 decode piped to a shell has the same property. Rather than try to out-parse bash, treat these splitting and decoding primitives as a reason to ask. They are rare in commands a human means to run unattended, so the false-positive cost is low, and the alternative is a pattern list that can always be re-encoded around. Tests: the ${IFS} and $IFS forms and a base64-to-shell pipeline all ask; `cat file.b64 | base64 -d > out.bin` (a decode that does not feed a shell) still allows. --- careful/bin/check-careful.sh | 17 +++++++++++++++++ test/hook-scripts.test.ts | 21 +++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/careful/bin/check-careful.sh b/careful/bin/check-careful.sh index db6857ec6b..dc39c4b33f 100755 --- a/careful/bin/check-careful.sh +++ b/careful/bin/check-careful.sh @@ -56,6 +56,23 @@ fi # Normalize: lowercase for case-insensitive SQL matching CMD_LOWER=$(printf '%s' "$CMD" | tr '[:upper:]' '[:lower:]') +# --- Shell-obfuscation tripwire --- +# Every check below inspects the command as a STRING, but bash executes what the +# string MEANS after expansion. ${IFS} holds the default field separator and +# contains no literal whitespace, so +# +# rm${IFS}-rf${IFS}/ +# +# matches none of the `rm\s+` patterns while executing as a full recursive +# delete. The same holds for a command assembled by a base64 decode piped to a +# shell. Rather than try to out-parse bash, treat these splitting/decoding +# 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 '{"permissionDecision":"ask","message":"[careful] Shell obfuscation detected (IFS word-splitting or base64-to-shell). Read the command carefully before approving."}\n' + exit 0 +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: diff --git a/test/hook-scripts.test.ts b/test/hook-scripts.test.ts index 1eef42082d..d14193f765 100644 --- a/test/hook-scripts.test.ts +++ b/test/hook-scripts.test.ts @@ -142,6 +142,27 @@ describe('check-careful.sh', () => { }); }); + // --- Shell obfuscation --- + + describe('shell obfuscation', () => { + test.each([ + 'rm${IFS}-rf${IFS}/', + 'rm$IFS-rf$IFS/', + 'echo cm0gLXJmIC8= | base64 -d | sh', + ])('asks when the command hides its shape behind expansion: %s', (command) => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput(command)); + expect(exitCode).toBe(0); + expect(output.permissionDecision).toBe('ask'); + expect(output.message).toContain('obfuscation'); + }); + + test('ordinary commands are unaffected', () => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('cat file.b64 | base64 -d > out.bin')); + expect(exitCode).toBe(0); + expect(output.permissionDecision).toBeUndefined(); + }); + }); + // --- JSON payload extraction --- describe('command extraction', () => {