fix(global-settings): destructive guard borrowed its verb from another command (v2.4.4)

[WilcoLouwerse]

The bug

The destructive/in-place guard in block-write-commands.sh used [^|]* as its gap pattern. That stops at a pipe but spans ; and && freely — so the tool name, its -i flag, and the protected path could each be borrowed from a different command in the same chain:

awk '{print}' /tmp/x; grep -c -i needle ~/.claude/hooks/foo.sh

Hard-denied as an "in-place edit". awk came from command one, -i from grep -c -i in command two, and the protected path from that same second command. Nothing in-place happens anywhere. Same shape on the rm arm:

rm /tmp/junk; cat ~/.claude/settings-version

Both commands only read the protected path.

Why it went unnoticed

It fails closed — the symptom is a refused read, never an allowed write. So there is no security exposure in either direction, and nothing ever broke loudly. It surfaced during real work in this repo: a diagnostic that only inspected ~/.claude/ was refused, with a reason describing an operation the command never attempted.

It also blocked the first attempt to commit this very fix, because the two examples above appear in the commit message. That is the tightest demonstration of the bug I can offer.

The fix

Anchor all three arms at a command-segment boundary (the rm arm already was) and keep the gap inside that segment — [^|;&] instead of [^|]:

-if echo "$cmd" | grep -qE "\b(sed|perl|awk|gawk|ruby)\b[^|]*[[:space:]]-i\b[^|]*${_prot}" \
-|| echo "$cmd" | grep -qE "\b(truncate|shred|unlink)\b[^|]*${_prot}" \
-|| echo "$cmd" | grep -qE "(^|[;&|]\s*)rm\b[^|]*${_prot}"; then
+if echo "$cmd" | grep -qE "(^|[;&|]\s*)(sed|perl|awk|gawk|ruby)\b[^|;&]*[[:space:]]-i\b[^|;&]*${_prot}" \
+|| echo "$cmd" | grep -qE "(^|[;&|]\s*)(truncate|shred|unlink)\b[^|;&]*${_prot}" \
+|| echo "$cmd" | grep -qE "(^|[;&|]\s*)rm\b[^|;&]*${_prot}"; then

Why this does not weaken the guard: a destructive verb aimed at a protected path always starts its own command segment, so every genuine case still matches. sed -i … ~/.claude/x, true && rm ~/.claude/x, echo foo; truncate -s 0 ~/.claude/x — all still hard-denied. The narrowing only removes matches assembled across ; / && from unrelated commands.

Verification

Full guard suite: 7509/7509, zero failures. Run on the branch after merging current main.

Existing tests are untouched, as requested. The test file is +25 / −0 lines against main — verifiable in the diff, and git diff --numstat confirms zero deletions. Nothing existing was edited to make anything pass.

Before/after on a standalone fixture set — the unpatched hook scores 9/11, the patched hook 11/11, and the two deltas are exactly the false positives above:

BEFORE (main):     9 passed, 2 failed
  FAIL  allow  minimal repro: awk early + unrelated -i later + protected path only read
  FAIL  allow  rm on unprotected path, protected path only read

AFTER (patched):  11 passed, 0 failed

New fixtures — a segment-fp section adds 18 cases (12 allow + 6 deny) across three protected files. Twelve assert the false positives are gone; six are controls proving genuine destructive ops still deny, including the chained forms (echo foo; sed -i …, echo foo && truncate …) that the existing section-4 fixtures never exercised. So this PR nets slightly more coverage of the real behaviour than existed before.

Version

VERSION 2.4.1 → 2.4.4.

Note the commit-message drift: the original commit says -> 2.4.3, written when that number was free. Since then main merged #704 (taking 2.4.2) and #716 was updated to claim 2.4.3, so this branch moved to 2.4.4 when main was merged in. The final state is 2.4.4; only the older commit message is stale. Merge order does not matter against #716 — the numbers no longer collide.

Test plan

• bash -n global-settings/block-write-commands.sh
• global-settings/tests/test-block-write-commands.sh → expect 7509/7509
• Confirm git diff --numstat origin/main -- global-settings/tests/test-block-write-commands.sh shows 25 0 (additions only)
• Sanity-check the three regex arms by eye — the claim is that segment-anchoring cannot drop a genuine destructive match
• After install, run a chained read-only inspection of ~/.claude/ and confirm it is no longer refused

🤖 Generated with Claude Code

[WilcoLouwerse]

Inline findings for #720 below.

[WilcoLouwerse]

Verdict: REQUEST_CHANGES (Standard) — self-review posted as COMMENT (GitHub blocks self-approval)

Findings

• 🔴 Blocker: narrowing the segment anchor on the sed|perl|awk|gawk|ruby and truncate|shred|unlink arms introduces new bypass shapes that did not exist on main — leading whitespace/tab, subshells, brace groups, env, and command substitution around the verb all flip a genuinely destructive in-place edit / truncation of a protected path from DENY (main) to ALLOW (this PR). Verified with an orthogonal Python re-implementation of both regex versions, independent of executing the hook itself. None of the 18 new segment-fp fixtures exercise these shapes, so the suite's 7509/7509 pass rate doesn't cover this.
• 🟢 Minor: commit-message says -> 2.4.3, file ships 2.4.4 — drift is explained (PR #716 took 2.4.3 first) and 2.4.4 is confirmed free, not blocking.

Already verified, not re-reported as new: bash -n passes; full guard suite 7509/7509 on the merged tree; test file is +25/-0 against main (confirmed via git diff --numstat-equivalent file-level additions/deletions from the PR files API — zero deletions, no existing fixture altered); standalone fixture set scores 9/11 unpatched vs 11/11 patched, matching the two described false positives exactly; the intended chained-command false-positive fix (awk … ; grep -c -i … ~/.claude/…, rm … ; cat ~/.claude/…) does work as described and is now correctly allowed; all of sed -i 's/a/b/' ~/.claude/settings.json, true && rm ~/.claude/settings.json, echo foo; truncate -s 0 ~/.claude/settings-version, perl -i -pe, gawk -i inplace, and every ~/$HOME/${HOME}/quoted/literal-path variant remain hard-denied by this PR — the regression above is additive on top of a fix that otherwise works exactly as described.

Version: 2.4.4 is confirmed free (main=2.4.2, open PR #716 claims 2.4.3, no other open PR touches VERSION).

[WilcoLouwerse]

Inline finding for #720 below.

[WilcoLouwerse]

🔴 Blocker — narrowing the gap class alone reopens a silent bypass, on top of the anchor issue this commit already fixed

The anchor revert in 3b22792 correctly restores coverage for the six shapes from the prior review (leading whitespace/tab, subshell, brace group, env, if/then — all re-verified DENY below). But the remaining change — [^|] → [^|;&] on all three arms — is not safe on its own either. The guard is pure text matching, not shell-aware: it has no notion that a ; or & inside a quoted argument is not a command separator. Narrowing the gap class means the match now fails to bridge across any literal ;/& character sitting between the verb (or its -i flag) and the protected path — including one that's part of a single command's own arguments, not a chain.

That shape is not exotic. It is the everyday idiom of chaining substitutions in one script (verb + -i + a quoted script containing a literal semicolon, followed by a protected path):

VERB -i "s/a/b/;s/c/d/" <protected-path>

Verified by extracting the actual regex from both hook versions (not a re-implementation) and testing directly:

MAIN    gap=[^|]    → MATCH    (correctly hard-denied)
THIS PR gap=[^|;&]  → NOMATCH  (falls through to the generic, path-unaware fallback rule for that verb where one exists → permissionDecision: "ask", not the intended unconditional block)

For the in-place-edit and truncation/deletion verbs on the first two arms (all except sed), and for the deletion verb on the third arm, there is no generic path-unaware fallback rule at all, so the identical shape is a silent, unconditional ALLOW — no prompt, no deny. Confirmed by piping each candidate through the real hook script (fake $HOME, protected files present):

in-place-edit verb #2 (Perl) with -i, quoted script containing ';', protected path      → ALLOW  (main: DENY)
in-place-edit verb #4 (gawk) with -i inplace, quoted script containing ';', protected path → ALLOW  (main: DENY)
truncation verb (0-length) with a quoted filename containing ';', protected path         → ALLOW  (main: DENY)
deletion verb (single-file, non-recursive) with a quoted filename containing ';', protected path → ALLOW (main: DENY)
deletion verb from the third arm (recursive-delete family), quoted filename containing ';', protected path → ask, path-unaware (main: DENY)

All five genuinely mutate or delete a protected-list file; all five are hard-denied on main; all five bypass, or get silently downgraded past, this PR's guard.

This falsifies the PR description's central claim ("narrowing the gap alone fixes the false positive without opening those holes") for the two un-anchored arms plus the pre-existing anchored arm. A character-class restriction on the gap can't distinguish a real command separator from the same byte occurring inside quotes; fixing this needs shell-aware segment splitting (matching how bash itself would tokenize unquoted ;/&/|), not a wider or narrower character class.

None of the new segment-fp or non-segment-start fixtures exercise this — the test file has no case with a ;/& inside a quoted script/filename sitting between the verb and the path, so 7539/7539 gives zero signal on it.

(Concrete reproduction commands and raw hook output withheld from this comment body — piping them through GitHub's rendering would itself hit this repo's own installed copy of the guard hook on this reviewer's machine, exactly as it did for the PR author while drafting this fix. Happy to share the fixture-file diff privately if useful; the regex-level proof above — real regex, real gap classes, MATCH vs NOMATCH — is the load-bearing part and doesn't require running the literal strings anywhere.)

[WilcoLouwerse]

Confirmed and fixed in 5cd96b9 — you were right again, and this one was worse than the first. I reproduced it before changing anything: sed -i "s/a/b/;s/c/d/" <protected-path> denies on main and was allowed on my branch, and for perl -i / gawk -i inplace it was a silent allow with no fallback prompt at all.

My error was assuming a ; is always a command separator. The guard is plain text matching with no shell awareness, so it cannot distinguish that from a ; inside a quoted argument — and a two-substitution sed script is about as ordinary as it gets.

The false positive never needed a broad narrowing. It came from the verb and the -i belonging to different commands, so the only gap that must be constrained is the one between them — a ; or & can never legitimately sit between a command name and its own flag. Every gap preceding ${_prot} is back to [^|]*.

Net diff against main is now one character class, on one arm, in one position; the truncate/shred/unlink and rm arms are byte-identical to main again.

That leaves two false positives unfixed: the rm-then-read shape and the sed-on-an-unprotected-path-then-read shape. Both need the guard to know where one command ends, which regex cannot do without reopening the hole you found. They fail closed, so I left them and documented the reasoning in the comment block — explicitly, so the next person does not "fix" it the way I twice did.

Fixtures reworked: dropped the three that asserted the now-unfixed false positives, added 18 covering separators inside quoted arguments (your six shapes x 3 files) alongside the 30 non-segment-start cases. Full suite 7548/7548; on a 24-case adversarial set main scores 23/24 and this version 24/24. Test file still additions-only (+62/-0).

Worth recording: this is the second time an "make it consistent" instinct cost coverage in this guard. Both times the minimal edit was the correct one.

[WilcoLouwerse]

Verdict: REQUEST_CHANGES (Standard) — self-review posted as COMMENT (GitHub blocks self-approval)

Re-review of 3b22792 (fix for the prior blocker).

Findings

• 🔴 Blocker: narrowing the gap class alone ([^|] → [^|;&]) reopens a silent bypass on the same three arms — a genuine in-place edit/truncation/deletion of a protected path, denied on main, is silently allowed (or downgraded to a generic, path-unaware "ask") whenever a literal ;/& sits inside a quoted script or filename between the verb and the path. That shape includes the everyday idiom of chaining substitutions in one script (verb -i "s/a/b/;s/c/d/" <protected-path>). Verified against the extracted regex directly (main MATCH vs this-PR NOMATCH) and by piping candidate commands through the real hook script with a fake $HOME. None of the 48 new fixtures exercise this shape.

Confirmed fixed from the prior review: all six previously-regressing shapes (leading whitespace, leading tab, subshell, brace group, env wrapper, if/then) are denied again — the anchor revert on the sed/perl/awk/gawk/ruby and truncate/shred/unlink arms restores that coverage exactly as described. Resolved that thread.

Re-verified, matches the PR description:

• The only functional diff from main across all three arms is the gap-class change [^|] → [^|;&]; the rm arm's (^|[;&|]\s*) anchor is unchanged/pre-existing, confirmed by a direct file diff against main at this head SHA.
• Original false positives remain fixed: a chained awk-then-unrelated--i command and a chained rm-then-read command are both correctly ALLOWed now, DENYed on main.
• Full guard suite: 7539/7539, independently re-run against this head SHA.
• Test file diff against main: +48/−0, confirmed via the files API — additions only, no existing fixture touched.
• VERSION: main=2.4.2, open PR #716 claims 2.4.3 (unmerged), this PR=2.4.4 — no collision, 2.4.4 confirmed still free.

Minor (resolved, no action needed): the commit-message drift (-> 2.4.3 vs shipped 2.4.4) is disclosed in the PR body and explained by merge ordering with #716 — sufficient as-is, not blocking.

Bottom line: the anchor regression from the prior review is genuinely fixed. But the remaining "narrow the gap class" half of the fix has the same class of problem the anchor did — a character-class restriction can't tell a real command separator from the same byte inside quotes — and needs another pass before this is safe to merge.