From a47624e9581760bead901b9f2a6215879d66ffe9 Mon Sep 17 00:00:00 2001 From: Xavier876 <65731369+Xavier876@users.noreply.github.com> Date: Thu, 10 Sep 2026 18:38:51 -0500 Subject: [PATCH] fix(hooks): only guard a verb in command position MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pre-bash-guard.sh matched its guarded phrases anywhere in the command text, so it blocked commands that read or write *about* the thing it guards: grep -n 'Remove-Item\|rm -rf\|BLOCKED' hooks/pre-bash-guard.sh grep -rn 'git reset --hard' docs/ git commit -m "docs: explain why rm -rf is blocked" confirm -rf task None of these delete or discard anything. The first is a command that reads the guard's own source; the last is any word ending in the letters r and m followed by a flag. A guard that blocks reading and documenting itself teaches people to route around it, which is worse than not guarding. This is the same class of defect #23 fixed in pre-commit-antipattern.sh — a naive grep over text that holds both code and prose about code. The same answer applies here. Four passes, in order: 1. Heredoc bodies collapse to a placeholder. A commit message written with `cat >msg <<'EOF'` is prose, and prose about a guard quotes what it guards. Delimiter lines stay, so the shape of the command is still readable. 2. Quoted spans collapse too. That is what separates a commit message carrying a phrase from a command that is one, and it reduces a grep alternation, escapes and all, to a single harmless token. 3. A shell wrapper hands its quoted argument back to a shell, so when one is present the unmasked text is scanned instead. The wrapper is itself looked for in the masked text and in command position, so prose that merely names one does not switch the scan back on. 4. A verb only counts in command position: the start of the command, or after something that begins one. This is what stops "confirm -rf". The rm allowlist reads the unmasked text, being the one check that needs the target path, and now also accepts a quoted target — "node_modules" is the same target as node_modules, and quoting is what anybody does once a path has a space. The no-jq fallback now undoes JSON string escapes. Without that a multi-line command arrives as a single line carrying literal \n, so nothing that reads the command line by line can see its shape, and the guard behaved differently with and without jq installed. Six tests added to .github/scripts/test-hooks.sh, continuing from the existing number 5. Where 5 covers a guarded phrase in another JSON field, these cover it as data inside the command itself. Five fail against the current guard. The last two are the guard rails: a recursive force delete of a project directory, and one wrapped in a shell wrapper, both still blocked. Co-Authored-By: Claude Opus 5 --- .github/scripts/test-hooks.sh | 53 ++++++++++++++++++++ hooks/pre-bash-guard.sh | 92 ++++++++++++++++++++++++++++++++--- 2 files changed, 138 insertions(+), 7 deletions(-) diff --git a/.github/scripts/test-hooks.sh b/.github/scripts/test-hooks.sh index 4554049..2828725 100644 --- a/.github/scripts/test-hooks.sh +++ b/.github/scripts/test-hooks.sh @@ -142,6 +142,59 @@ else fail "pre-bash-guard exited $rc (expected 0) — over-blocking regression: payload text matched instead of the parsed command" fi +# 6-9. Regression: a guarded phrase appearing as DATA inside the command must +# not block. Test 5 covers the phrase in another JSON field; these cover it +# in the command itself — a grep pattern, a commit message, and a word that +# merely ends in the letters "rm". None of them delete or discard anything, +# and the first is a command that reads the guard's own source. +allow_command() { + local label="$1" command_json="$2" n="$3" + printf '{"session_id":"ci","hook_event_name":"PreToolUse","tool_name":"Bash","tool_input":{"command":"%s"}}' \ + "$command_json" >"$WORKDIR/p$n.json" + run_hook 10 hooks/pre-bash-guard.sh "$WORKDIR/p$n.json" "$WORKDIR/o$n" "$WORKDIR/e$n" + local rc=$? + if [ "$rc" -eq 0 ]; then + pass "pre-bash-guard allows $label" + else + echo " output was: [$(cat "$WORKDIR/o$n" "$WORKDIR/e$n")]" + fail "pre-bash-guard exited $rc (expected 0) — over-blocking: $label" + fi +} + +allow_command "a grep pattern containing the guarded phrase" \ + "grep -n 'Remove-Item\\\\|rm -rf\\\\|BLOCKED' hooks/pre-bash-guard.sh" 6 +allow_command "a commit message documenting the guard" \ + "git commit -m \\\"docs: explain why rm -rf is blocked\\\"" 7 +allow_command "a word that merely ends in r-m" \ + "confirm -rf task" 8 +allow_command "grepping the docs for a guarded git phrase" \ + "grep -rn 'git reset --hard' docs/" 9 +allow_command "a commit message that merely names a shell wrapper" \ + "git commit -m \\\"use bash -c to run it, never rm -rf /tmp/x\\\"" 12 +allow_command "a heredoc commit message about the guard" \ + "cat > /tmp/m.txt <<'EOF'\\ndocs: explain why rm -rf is blocked\\nand why bash -c is treated as a command\\nEOF\\ngit commit -F /tmp/m.txt" 13 + +# 10-11. The other half of the same change: masking quoted text must not let a +# real command through. A shell wrapper hands its quoted argument back to a +# shell, so that argument is a command again and still has to be blocked. +block_command() { + local label="$1" command_json="$2" n="$3" + printf '{"session_id":"ci","hook_event_name":"PreToolUse","tool_name":"Bash","tool_input":{"command":"%s"}}' \ + "$command_json" >"$WORKDIR/p$n.json" + run_hook 10 hooks/pre-bash-guard.sh "$WORKDIR/p$n.json" "$WORKDIR/o$n" "$WORKDIR/e$n" + local rc=$? + if [ "$rc" -eq 2 ]; then + pass "pre-bash-guard still blocks $label" + else + fail "pre-bash-guard exited $rc (expected 2) — under-blocking: $label" + fi +} + +block_command "a recursive force delete of a project directory" \ + "rm -rf src/MyApp/Controllers" 10 +block_command "a recursive force delete wrapped in bash -c" \ + "bash -c \\\"rm -rf /\\\"" 11 + echo "" if [ "$FAILURES" -gt 0 ]; then echo "$FAILURES hook test(s) failed" diff --git a/hooks/pre-bash-guard.sh b/hooks/pre-bash-guard.sh index 227cb92..40f67ce 100644 --- a/hooks/pre-bash-guard.sh +++ b/hooks/pre-bash-guard.sh @@ -25,40 +25,118 @@ if [[ -z "$COMMAND" ]] && [[ ! -t 0 ]]; then # patterns match on prefixes and truncation only risks under-matching the # tail of unusual commands, not blocking on unrelated payload fields. COMMAND=$(printf '%s' "$STDIN" | sed -n 's/.*"command"[[:space:]]*:[[:space:]]*"\(\(\\.\|[^"\\]\)*\)".*/\1/p' | head -1) || true + # Undo the JSON string escapes jq would have undone. Without this a + # multi-line command arrives as one line carrying literal \n, and anything + # that reads the command line by line — heredoc bodies, below — cannot see + # its shape. Backslash last, so an escaped backslash is not re-expanded. + COMMAND=$(printf '%s' "$COMMAND" | sed -e 's/\\n/\ +/g' -e 's/\\t/\t/g' -e 's/\\"/"/g' -e 's/\\\\/\\/g') fi [[ -z "$COMMAND" ]] && COMMAND="$STDIN" fi +# ── Where a guarded verb has to appear ────────────────────────────── +# +# Extracting the command field stopped the guard firing on *other* JSON fields, +# but not on the guarded words appearing as data inside the command itself. +# Real false positives, all from one session: +# +# grep -n 'Remove-Item\|rm -rf\|BLOCKED' hooks/pre-bash-guard.sh +# grep -rn 'git reset --hard' docs/ +# git commit -m "document why rm -rf is guarded" +# confirm -rf task # "confirm" ends in the letters r and m +# +# None of them delete anything. The first is reading this very file. A guard +# that blocks reading itself, or writing about itself, teaches the reader to +# route around it — which is worse than not guarding. +# +# Two passes fix it. + +# 1. Heredoc bodies are data. A commit message written with `cat >msg <<'EOF'` +# is prose, and prose about a guard quotes the thing it guards. Each body +# line collapses to a placeholder; the delimiter lines stay so the shape of +# the command is still readable to the checks below. +SCAN_RAW="$COMMAND" +NO_HEREDOC=$(printf '%s' "$COMMAND" | awk -v sq="'" -v dq='"' ' + inhd { + line = $0 + sub(/^[ \t]+/, "", line) + if (line == marker) { inhd = 0; print } else { print "Q" } + next + } + { + if (match($0, "<<-?[ \t]*[" sq dq "]?[A-Za-z_][A-Za-z0-9_]*")) { + m = substr($0, RSTART, RLENGTH) + sub(/^<<-?[ \t]*/, "", m) + gsub(sq, "", m); gsub(dq, "", m) + marker = m; inhd = 1 + } + print + } +') + +# 2. Quoted spans are data too. That is what separates +# `git commit -m "... rm -rf ..."` from `rm -rf`: one is an argument +# somebody is writing, the other is a command. It also collapses a grep +# alternation, escapes and all, into a single harmless token. +MASKED=$(printf '%s' "$NO_HEREDOC" | sed -e "s/'[^']*'/Q/g" -e 's/"[^"]*"/Q/g') +SCAN="$MASKED" + +# 3. Unless a shell wrapper hands that quoted text straight back to a shell, in +# which case it is a command again and has to be read as one. The wrapper +# itself is looked for in the *masked* text and in command position, so that +# prose merely naming one — this comment, or a commit message about it — +# does not switch the scan back on. +if printf '%s' "$MASKED" | grep -qE '(^|[[:space:];&|(){}])((ba|z|k|da)?sh[[:space:]]+(-[a-zA-Z]+[[:space:]]+)*-c|eval)([[:space:]]|$)'; then + SCAN="$NO_HEREDOC" +fi + +# 4. A verb only counts in *command position*: the start of the command, or +# straight after something that begins one. This is what stops "confirm -rf" +# — the letters r and m at the end of an ordinary word are not a command. +SEP=$'(^|[[:space:];&|(){}<>"\'`])[[:space:]]*' + +# Whether a guarded verb appears in command position. Takes an ERE for the verb. +in_command_position() { + printf '%s' "$SCAN" | grep -qE "${SEP}$1" +} + # ── Destructive git operations ────────────────────────────────────── -if echo "$COMMAND" | grep -qE 'git\s+push\s+.*--force|git\s+push\s+-f\b'; then +if in_command_position 'git[[:space:]]+push[[:space:]]+.*--force' \ + || in_command_position 'git[[:space:]]+push[[:space:]]+-f\b'; then echo "BLOCKED: Force push detected. Use regular push or discuss with the user first." >&2 exit 2 fi -if echo "$COMMAND" | grep -qE 'git\s+reset\s+--hard'; then +if in_command_position 'git[[:space:]]+reset[[:space:]]+--hard'; then echo "BLOCKED: git reset --hard will discard all uncommitted changes. Discuss with the user first." >&2 exit 2 fi -if echo "$COMMAND" | grep -qE 'git\s+clean\s+-[a-zA-Z]*f'; then +if in_command_position 'git[[:space:]]+clean[[:space:]]+-[a-zA-Z]*f'; then echo "BLOCKED: git clean -f will permanently delete untracked files. Discuss with the user first." >&2 exit 2 fi -if echo "$COMMAND" | grep -qE 'git\s+checkout\s+\.'; then +if in_command_position 'git[[:space:]]+checkout[[:space:]]+\.'; then echo "BLOCKED: git checkout . will discard all unstaged changes. Discuss with the user first." >&2 exit 2 fi # ── Dangerous file operations ─────────────────────────────────────── -if echo "$COMMAND" | grep -qE 'rm\s+-[a-zA-Z]*r[a-zA-Z]*f|rm\s+-[a-zA-Z]*f[a-zA-Z]*r'; then +if in_command_position 'rm[[:space:]]+-[a-zA-Z]*r[a-zA-Z]*f' \ + || in_command_position 'rm[[:space:]]+-[a-zA-Z]*f[a-zA-Z]*r'; then # Allow recursive-force rm on safe targets (node_modules, bin, obj, TestResults, # .vs, /tmp). Matches any flag spelling the detector matches (-rf, -fr, -rfv, ...) # and allows ./-prefixed or relative-path-prefixed targets (e.g. ./bin, # src/MyApp/obj). Absolute paths stay blocked (so /bin is NOT allowed) except /tmp. - if echo "$COMMAND" | grep -qE 'rm\s+-[a-zA-Z]+\s+(\./)?([^ /][^ ]*/)?(node_modules|bin|obj|TestResults|\.vs)/?(\s|$)|rm\s+-[a-zA-Z]+\s+/tmp(/[^ ]*)?(\s|$)'; then + # A quoted target is allowed too — "node_modules" is the same target as + # node_modules, and quoting it is what anybody does once a path has a space. + # Read against the unmasked text: masking replaced the quoted target with a + # placeholder, and the allowlist is the one check that needs the path itself. + if echo "$SCAN_RAW" | grep -qE 'rm[[:space:]]+-[a-zA-Z]+[[:space:]]+["'"'"']?(\./)?([^ /"'"'"'][^ "'"'"']*/)?(node_modules|bin|obj|TestResults|\.vs)/?["'"'"']?([[:space:]]|$)|rm[[:space:]]+-[a-zA-Z]+[[:space:]]+["'"'"']?/tmp(/[^ "'"'"']*)?["'"'"']?([[:space:]]|$)'; then : # safe, fall through to exit 0 else echo "BLOCKED: rm -rf detected in a project directory. Verify the target path is intentional." >&2 @@ -68,7 +146,7 @@ fi # ── dotnet run without launch settings check ──────────────────────── -if echo "$COMMAND" | grep -qE 'dotnet\s+run\b'; then +if in_command_position 'dotnet[[:space:]]+run\b'; then echo "WARNING: dotnet run detected. Ensure launchSettings.json exists and the correct profile is selected." # Allow but warn — exit 0 so it proceeds fi