fix(guardrails): decide block-hook-bypass's exemptions on the whole redirect operand - #2287
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
… at 0.25.3 block-hook-bypass decides its target-based exemptions on the first whitespace- or separator-delimited fragment of a quoted redirect operand rather than on the operand. strip_literals keeps a quoted write target as literal content (dropping the quotes so it still reads as a write) and _redir_scan's target class ends at whitespace, so a quoted operand carrying a space, `;`, `|`, `&`, `(`, `)` or a newline resolves, for exemption purposes, to its first fragment. These assertions reproduce that against the `/dev/null` discard — the only target-based exemption that is on by default — plus the unquoted escaped forms that reach the same truncation through normalize_segments' escaped- separator sentinel, the fd-numbered and cat spellings, both redirect orderings, a multi-line operand, an operand continued by a backslash-newline, and a forged sentinel byte. Nineteen of them fail against the hook as it stands; the fix lands two commits later. Refs #2226 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tely flips Four assertions added by #2224/#2235 encode behaviour the #2226 fix changes, so they are moved here on their own rather than folded into the fix. 0.25.0's fail-close could not tell an operand's quotes from a content quote, so it read `${COMMAND#*>}` — any quote or backslash after the first literal `>` CHARACTER, anywhere in the command. #2236 documented the two resulting blunt edges and pinned both: a quote in an unrelated later segment cancelled an earlier unambiguous write's exemption, and a `>` inside quoted content started the scanned tail early so that content's own closing quote landed inside it. Marking the operand supplies exactly the association those two lacked, so the test becomes operand-keyed and both shapes are exempt again: scratch: quote in an unrelated later segment ... 2 -> 0 scratch: > inside double-quoted content ... 2 -> 0 scratch: > inside single-quoted content ... 2 -> 0 A fourth is added rather than flipped — the `;`-separated spelling of the same compound, which #2236 reported and no assertion covered. These three are the only GRANT in the change: each lands on a target the marks prove was bare — no quote mark, no opaque mark, no backslash. Every other verdict this fix moves goes the other way, toward refusing an exemption. The scratch axis's own floor is untouched: a quoted operand, benign or not, is still never scratch-exempt, and that assertion stays as written. Refs #2226, #2236 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…edirect operand
A quoted redirect operand is ONE pathname to bash. block-hook-bypass decided
its target-based exemptions on the operand's first whitespace- or separator-
delimited fragment instead, because the two pieces of machinery either side of
the decision disagree about what a kept operand is: strip_literals KEEPS a
quoted write target as literal content (dropping the quotes, so a quoted target
still reads as a write) while normalize_segments then reads a `;`, `|`, `&`,
`(`, `)` or newline inside it as a segment boundary and _redir_scan's target
class ends at whitespace.
So `echo x > "/dev/null ../../etc/pw"` was exempted on the word `/dev/null`,
though nothing named `/dev/null` is the destination — and the same truncation
reached the unquoted escaped spellings, where normalize_segments' escaped-
separator sentinel restored `\;` to a space mid-operand.
strip_literals now marks a kept operand's literal content with two sentinels:
\x03 OPAQUE one character whose literal value would read as syntax
downstream, or a backslash escape this strip cannot reproduce
faithfully (inside double quotes bash RETAINS the backslash
unless it escapes $ ` " \ or a newline). Inert to every scan, so
the operand survives as ONE token, and its presence means the
pathname is not recoverable here — no exemption may be granted.
\x04 QUOTED emitted where a kept span opens. The discard compare strips it,
so `> "/dev/null"` is still a discard; the scratch axis keeps its
shipped floor of never exempting a quoted operand.
A raw \x01-\x04 byte in the command is mapped to OPAQUE, so a forged sentinel
can only cost an exemption, never manufacture one.
Every mark is gated on _in_redirect_operand — the same "this word began right
after a `>`" test the quoted-operand keep already used, now factored out and
also applied to the unquoted backslash branch. That gating is deliberate and
load-bearing: normalize_segments, _producer_head, _cat_redir and every
whitespace trim in this file are byte-for-byte as shipped, so an escaped
separator BETWEEN commands (`echo x \; > f`) still travels the unchanged
`\x02`-to-space path and a backslash in a command word
(`/c/Python313/python3.exe -c`) is untouched.
With the association restored, scratch_target_exempt no longer has to infer it
from `${COMMAND#*>}`. The fail-close is keyed on the operand's own marks, which
retires both blunt edges #2236 documented.
Direction of every verdict this moves, on the axis that matters: refusing an
exemption is friction, granting one is a bypass. Nineteen shapes move from
GRANTED to REFUSED. Three move the other way — a quote in an unrelated later
segment, and a `>` inside double- or single-quoted content — and each lands on
a target the marks prove was bare.
#1680 and #1667 read the same file's normalization. Neither moves: their shapes
(`echo x >&2`, `printf … >&2`, `echo x &>file`, `1>&2`, `2>&1`, `>&2>file`,
`cat 1>&2`, `cat 1>&-`) carry no quotes and no backslashes, so no mark is ever
emitted for them, and the machinery they concern is unchanged.
Closes #2226
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…k-hook-bypass Minor, not patch: what the guard exempts changes in both directions. The CHANGELOG entry grades every moved verdict on the axis that matters — nineteen shapes move from GRANTED to REFUSED (the #2226 family), three move the other way (the #2236 frictions), and the grant surface is named explicitly. Four surfaces move together, the same four #2235 corrected: the hook comment (committed with the fix), this CHANGELOG, the README caveat paragraph, and the manifest's block_hook_bypass_scratch_roots description. All three of the latter carried the "any quote or backslash after the first `>` CHARACTER in the command" wording that this release retires; the README options table is regenerated from the manifest by sync-plugin-options-docs.py. 0.25.0's and 0.25.1's entries are left exactly as they shipped. 0.25.1 gains an erratum pointer inside it — its claim that the fail-close's breadth stays, and that narrowing it needs #2226, was true when written and is not now. That mirrors the pointer 0.25.0 already carries to 0.25.1. Refs #2226, #2236 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…red control assertion Two corrections to text shipped earlier on this branch, both found by review before merge. No behaviour change; the assertions themselves are unmoved. 1. The direction split was miscounted. Counted mechanically from the new suite run against 0.25.3's hook — 19 failures, `expected exit 2, got 0` fifteen times and `expected exit 0, got 2` four times — the change moves FIFTEEN shapes from GRANTED to REFUSED and FOUR the other way, not nineteen and three. The `;`-separated compound is a fourth GRANT, not merely a newly-pinned case, and eba2ee6/f775974b say otherwise. A forged sentinel byte and an escaped-space operand were listed among the moved-to-REFUSED set and belong in neither: both already blocked at 0.25.3 and are regression guards here. 2. One of #2224's 36 assertions is RETIRED rather than kept — `control: /dev/null still shows the inherited truncation (#2226, allowed)`. #2224 wrote it so it would "flip visibly" when this issue was fixed, so retiring it is that PR's own instruction; six /dev/null assertions covering the whole family replace it. It went out in bb937fe with the new tests instead of in its own commit, which the constraint asked for. Recorded here and in the PR body rather than by re-cutting history. Refs #2226 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
16f618c to
d1c286b
Compare
|
Warning Automated security review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
The check is green on purpose, and it is not evidence. It certifies that a security pass ran, and this one did not complete — but the cause is outside this PR's control, so merging is deliberately left unblocked rather than locking every merge for the length of the outage. Nothing was reviewed at this head. Where this check is required, it is satisfied without that evidence; a human should review security-sensitive changes here before merging. Re-run the job to retry the review; a new push also retries it only if the caller's Re-running does NOT help for every class:
|
|
Warning Automated review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-run the job to retry the review. A new push re-triggers this lane only if the caller's |
…lock-hook-bypass operand-keyed Follow-up to #2287 (merged). That work was written against 0.26.0 and renumbered to 0.27.0 when main took 0.26.0 for the block-dangerous-git / block-no-verify jq fail-closed change (#2146) mid-flight. The renumber reached the CHANGELOG heading, the manifest version and the entry's own comparison table; it did not reach the narrative around them, so eight prose references still send a reader to a release that documents something else: README.md the "Since 0.26.0" / "Before 0.26.0" pair block-hook-bypass.sh the note above scratch_target_exempt's fail-close the test file three section comments CHANGELOG.md the erratum inside 0.25.1, both mentions 0.26.0's own heading is untouched — it is the one correct 0.26.0 reference in this plugin, and re-grepping against current main confirms it is the only one left afterwards. Rebased onto current main, which moved a long way and took 0.27.1 for the stale-path-verify comment fix (#1555) while this branch sat. This release is therefore 0.27.2, not 0.27.1 as the pre-rebase branch had it. Comments and prose only. Verified mechanically rather than asserted: the diff of the two shell files contains ZERO non-comment changed lines. Refs #2226 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lock-hook-bypass operand-keyed Follow-up to #2287 (merged). That work was written against 0.26.0 and renumbered to 0.27.0 when main took 0.26.0 for the block-dangerous-git / block-no-verify jq fail-closed change (#2146) mid-flight. The renumber reached the CHANGELOG heading, the manifest version and the entry's own comparison table; it did not reach the narrative around them, so eight prose references still send a reader to a release that documents something else: README.md the "Since 0.26.0" / "Before 0.26.0" pair block-hook-bypass.sh the note above scratch_target_exempt's fail-close the test file three section comments CHANGELOG.md the erratum inside 0.25.1, both mentions 0.26.0's own heading is untouched — it is the one correct 0.26.0 reference in this plugin, and re-grepping against current main confirms it is the only one left afterwards. Rebased onto current main, which moved a long way and took 0.27.1 for the stale-path-verify comment fix (#1555) while this branch sat. This release is therefore 0.27.2, not 0.27.1 as the pre-rebase branch had it. Comments and prose only. Verified mechanically rather than asserted: the diff of the two shell files contains ZERO non-comment changed lines. Refs #2226 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lock-hook-bypass operand-keyed (#2325) ## Summary Follow-up to **#2287** (merged), which fixed #2226 by making `block-hook-bypass`'s exemptions operand-keyed. That work was written against **0.26.0** and renumbered to **0.27.0** mid-flight, because `main` took 0.26.0 for the `block-dangerous-git` / `block-no-verify` `jq` fail-closed change (#2146) while the branch was open. The renumber reached the CHANGELOG heading, the manifest version and the entry's own comparison table. It did not reach the narrative around them, so **eight** prose references shipped on `main` still send a reader to a release that documents something else entirely: | file | count | reference | | --- | --- | --- | | `plugins/guardrails/README.md` | 2 | the "Since **0.26.0**" / "Before 0.26.0" pair in the scratch-root caveat paragraph | | `plugins/guardrails/hooks/block-hook-bypass.sh` | 1 | the note above `scratch_target_exempt`'s fail-close | | `plugins/guardrails/hooks/block-hook-bypass.test.sh` | 3 | three section comments | | `plugins/guardrails/CHANGELOG.md` | 2 | the erratum inside the 0.25.1 entry, both mentions | `main`'s own **0.26.0** entry — the `jq` fail-closed release — is untouched. It is the one 0.26.0 reference in this plugin that is correct, and after this change it is the only one left. Comments and prose only. No behaviour change, no assertion moved, no gate output changed. ## Rebased, and renumbered again This branch was `CONFLICTING` against `main` and has been rebased onto current `origin/main` (`900d33a1` at the time of the rebase). `main` took **0.27.1** for the `stale-path-verify.test.sh` comment fix (#1555) while this branch sat, so **this release is 0.27.2, not 0.27.1** as the pre-rebase branch had it. Patch, docs only — the precedent 0.25.1 set for a docs-accuracy correction against a shipped entry. **The sweep's target set was re-grepped against current `main` rather than trusted from the branch**, since a further version move could have changed which references are stale. It did not: the same eight, in the same four files. Enumerated on `origin/main` before the rebase: ``` CHANGELOG.md:115:## [0.26.0] <- correct, left alone CHANGELOG.md:255:> ... #2226 is fixed in 0.26.0 and the breadth is gone ... CHANGELOG.md:257:> 0.26.0 above for what replaced it. ... README.md:118: ... would be judged on `/tmp/scratch/a`. Since **0.26.0** README.md:124: Before 0.26.0 this test read the whole raw command tail ... block-hook-bypass.sh:677:# ... Since 0.26.0 that decision is made block-hook-bypass.test.sh:876:# drops its quotes, and before 0.26.0 normalize_segments ... block-hook-bypass.test.sh:900:# 0.26.0 and these four cases are where that is visible. ... block-hook-bypass.test.sh:928:# to bash. Until 0.26.0 the exemptions were decided ... ``` ## Test plan The claim is that nothing executable moved, so that is what is verified — mechanically, not asserted. Every changed line in both shell files is a comment line: ``` $ git diff -U0 -- plugins/guardrails/hooks/block-hook-bypass.sh \ plugins/guardrails/hooks/block-hook-bypass.test.sh \ | grep -E '^[+-][^+-]' | grep -vE '^[+-][[:space:]]*#' | wc -l 0 ``` Only `0.26.0` remains where it should: ``` $ grep -rn "0\.26\.0" plugins/guardrails/CHANGELOG.md plugins/guardrails/README.md \ plugins/guardrails/hooks/block-hook-bypass.sh plugins/guardrails/hooks/block-hook-bypass.test.sh plugins/guardrails/CHANGELOG.md:115:## [0.26.0] ``` Gates, run locally in the worktree after the rebase: ``` $ shellcheck --rcfile=.shellcheckrc -x plugins/guardrails/hooks/block-hook-bypass.sh \ plugins/guardrails/hooks/block-hook-bypass.test.sh shellcheck CLEAN $ bash scripts/check-shell-portability.sh --paths \ plugins/guardrails/hooks/block-hook-bypass.sh plugins/guardrails/hooks/block-hook-bypass.test.sh No unexcused GNU-only constructs in 2 shell file(s). $ python3 scripts/sync-plugin-options-docs.py --check plugin options docs: up to date $ bash scripts/check-changelog-parity.sh --check Every versioned plugin has a CHANGELOG.md (or a stale-guarded baseline entry), and none documents a version above its manifest. $ bash scripts/check-changelog-parity.sh --check-order All 75 changelog(s) read newest-first with no duplicate versions. $ bash scripts/check-changelog-parity.sh --check-bump origin/main Every plugin whose version changed vs origin/main has a '## [<version>]' CHANGELOG.md entry. $ npx markdownlint-cli2 plugins/guardrails/CHANGELOG.md plugins/guardrails/README.md Summary: 0 issues in 0 files ``` The `block-hook-bypass` contract suite was green at **345 / 345** on the tree #2287 merged, and `plugin-gate` confirmed that on ubuntu-24.04 / bash 5.2.21. This branch changes no executable line of either file, so `ci` runs it again unchanged here. ## Related Follows up #2287 (merged) and its issue #2226 (closed) — the quoted-redirect-operand fix whose release number this corrects. Version-collision context: #2146 landed as `guardrails` 0.26.0 while #2287 was open, and #1555 landed as 0.27.1 while this branch was open. Origin: handoff-inbox batch 4, lane owning `guardrails`. Filed as a follow-up because #2287 was merged before this sweep could be pushed to its branch. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e a reopened residual (#2367) ## Summary Three inline-write forms reached a real file through `block-hook-bypass.sh` without matching any detector (#2217). All three sit inside the guard's own declared threat model. One of them — the `python3 - <<PY` stdin heredoc — is a **documented accepted residual**, and this PR treats it as **reopening an accepted residual on new reachability evidence**, not as an oversight: the acceptance comment is updated in both files that carry it rather than contradicted. **Anchors are re-read at `4c90b454`.** The issue was filed before PR #2287 landed and cites `block-hook-bypass.sh:635-636` / `:670`; at the base of this branch those lines are `:968-969` and `:1005`. #2287's mark-based `strip_literals` rewrite is untouched by cause 1 and cause 3; cause 2 changes one join in the same function, in the direction #2287 deliberately left alone. PR #2325 is already merged into this base, so none of its renumbered lines are touched here. ### Cause 1 — the interpreter detector was a spelling floor, not a rule Both lanes required the **literal** `python3`: the Bash lane's `EXEC_LC` scan and the PowerShell lane's `ps::might_write_via_python3` token test. So the identical inline write spelled `python -c`, `py -c`, `py3 -c`, `python2 -c` or `python3.11 -c` ran unseen. The guard's own scope message advertised `python -c` as its example — naming the one spelling the regex did not match. The command word is now the python **family** (`py`/`python`/`pypy` + optional version suffix + optional `.exe`), still separator-anchored. `py -3 -c` is admitted because a `-<digits>` token cannot be a script path; no other gap between interpreter and flag is admitted, so `python3 build.py` and `python3 -m tool …` still do not block. ### Cause 2 — a physical newline inside a quoted span split a producer from its own redirect A newline reached with a quote span still **open** is not a separator: bash is inside a quoted word, so the text either side of the span is **one word**. `strip_literals` re-emitted it anyway, `normalize_segments` split there, and `producer_redirect_bypass` requires producer and redirect in one segment. The join is now **empty**. Not a space — and that is the one place this PR's mechanism differs from the one the issue suggested. `ec"<newline>"ho x > f` is `echo x > f` to bash; a space join leaves `ec ho`, which `_producer_head` does not match, so the write is still missed. That case ships as an assertion so the choice is evidenced rather than argued. Joining empty cannot manufacture a token bash does not also form, because an open quote is precisely what makes the two sides one word. ### Cause 3 — REOPENED ACCEPTED RESIDUAL (`RECONCILE.md` AD-12) `python3 - <<PY … PY` (no `-c`) was recorded as uncovered and accepted in the PowerShell lane's comment. Reachability evidence established before changing it, by grep over the repo: - `.work/handoffs/20260809T082720Z-handoff-post-2008-followups.md:211` — a prior session in this repository reached for exactly that form (`python - <<'PY'`) **to patch a file**. Note the spelling is `python`, not `python3`, so it is a cause-1 datum too. - The reporting session hit it while trying to comply with this guard's own remediation. - Cause 1 raises the pressure toward it: a refused `python -c` write reroutes most naturally to the heredoc. The `-` is what makes it inline — the code sits in the command string the hook reads, not in an opaque script file. `strip_literals` drops the heredoc operator and body, so `EXEC_LC` retains `python3 -` while the body's write indicators stay visible in `COMMAND_LC`. **Narrowed residual, restated at its real width:** `python3 <<PY` with **no** `-` stays uncovered. Matching a bare trailing interpreter token would flip `echo "pathlib" | python3` and `cat script.py | python3` to blocked. Both floors are asserted. ### Direction of every behavior change **23 granted → refused, 1 refused → granted.** Measured two ways, not asserted. **Tier 1 — the shipped suite, run against the PRE-change hook.** The new assertions were copied into a worktree at the merge base and the suite executed there: ``` $ bash plugins/guardrails/hooks/block-hook-bypass.test.sh # pre-change hook, new assertions FAIL: #2217: python -c open write (blocked): expected exit 2, got 0 FAIL: #2217: py -c open write (blocked): expected exit 2, got 0 FAIL: #2217: py3 -c open write (blocked): expected exit 2, got 0 FAIL: #2217: python2 -c open write (blocked): expected exit 2, got 0 FAIL: #2217: python3.11 -c open write (blocked): expected exit 2, got 0 FAIL: #2217: pypy3 -c open write (blocked): expected exit 2, got 0 FAIL: #2217: py -3 -c open write (blocked): expected exit 2, got 0 FAIL: #2217: path-qualified python.exe -c open write (blocked): expected exit 2, got 0 FAIL: #2217: printf, physical newline in a single-quoted arg (blocked): expected exit 2, got 0 FAIL: #2217: echo, physical newline in a double-quoted arg (blocked): expected exit 2, got 0 FAIL: #2217: quote span splicing a command word (blocked): expected exit 2, got 0 FAIL: #2217: python3 - <<PY heredoc write (blocked): expected exit 2, got 0 FAIL: #2217: python - <<PY heredoc write, family spelling (blocked): expected exit 2, got 0 FAIL: #2217: PS python -c open write (blocked): expected exit 2, got 0 FAIL: #2217: PS py -c open write (blocked): expected exit 2, got 0 PASS=377 FAIL=15 ``` Every failure is `expected 2, got 0` — granted → refused. None is `expected 0, got 2`. **Tier 2 — adversarial probes written after the suite was green, specifically hunting the other direction.** Eight more rows move; all eight are now assertions too: | row | before | after | direction | |---|---|---|---| | `/usr/bin/python -c "open('f','w')…"` | 0 | 2 | granted → refused | | `echo "a<NL>" x > f` | 0 | 2 | granted → refused | | `cat "a<NL>" > f` | 0 | 2 | granted → refused | | `echo "a<NL>b" "c<NL>d" > f` | 0 | 2 | granted → refused | | `if true ; then echo "a<NL>b" > f ; fi` | 0 | 2 | granted → refused | | `! echo "a<NL>b" > f` | 0 | 2 | granted → refused | | `exec -a n echo "a<NL>b" > f` | 0 | 2 | granted → refused | | `FOO=1 printf "a<NL>b" > f` | 0 | 2 | granted → refused | | **`foo "a<NL>" echo x > f`** | **2** | **0** | **refused → granted** | **The one refused → granted, named rather than buried.** Fusing the two sides of a span back into one segment also puts whatever preceded the span at the segment start, where `_producer_head`'s `^` anchor sees it. In `foo "a<newline>" echo x > f`, bash's command word is `foo` and `echo` is one of its *arguments*, so the redirect's producer is another program — and this guard is producer-scoped by design (see the README's producer-scoping note). The newline previously split it into a bogus `echo x > f` segment and blocked it. The single-line spelling `foo "a" echo x > f` is **rc=0 on `main` today**, so this makes the multi-line form agree with shipped behavior rather than inventing an exemption. Both are asserted. **It is one row, not a class — verified, not reasoned.** The obvious escalation is a *legitimate* command prefix in front of the span hiding a real producer from the `^` anchor. Every prefix the file already models was probed against both hooks and all of them still block, because `_cmd_prefix` / `_modifier_opt_arg` / `_leading_redir` peel on the fused segment: env assignments, `env`, `if…then`, `!`, `exec -a NAME`, and a leading redirect (rows 5–8 above, each paired with its single-line control). The only text that survives to the segment start is a genuine command word, which is exactly the case where the producer is not `echo`. Every remaining floor keeps `rc=0`: the name anchor, the #1601/#2148 over-block repros re-run for each new spelling, the multi-line prose/`--body` floor, the `/dev/null` discard floor and the stdin floor. ## Test plan Hook invoked as a decision function on `PreToolUse` Bash payloads built with `jq -n --arg` — `rc=2` blocked, `rc=0` allowed. Adversarial-first: every row below was written and run against the **pre-change** hook first. **Before (`4c90b454`) → after (this branch):** ``` BEFORE AFTER ### CONTROL (guard live) echo "*" > .gitignore rc=2 rc=2 git status rc=0 rc=0 ### C-H1 interpreter spelling floor (granted -> refused) python3 -c open-write rc=2 rc=2 python -c open-write rc=0 rc=2 py -c open-write rc=0 rc=2 python3.11 -c open-write rc=0 rc=2 python2 -c open-write rc=0 rc=2 py3 -c open-write rc=0 rc=2 py -3 -c open-write rc=0 rc=2 /usr/bin/python -c open-write rc=0 rc=2 /c/Python313/python.exe -c open-write rc=0 rc=2 pypy3 -c open-write rc=0 rc=2 ### C-H1 name-anchor floor (unchanged) notpython3 -c write rc=0 rc=0 mypython3 -c write rc=0 rc=0 pythonx -c write rc=0 rc=0 mypy -c write rc=0 rc=0 happy -c write rc=0 rc=0 spy -c write rc=0 rc=0 pytest -c write rc=0 rc=0 ### over-block floor #1601 / #2148 (unchanged) #1601 read-only json.load(open(p)) rc=0 rc=0 #2148 print-only rc=0 rc=0 python -c read-only open rc=0 rc=0 py -c print only rc=0 rc=0 python3.11 -c os.path.normpath rc=0 rc=0 python -m tool rc=0 rc=0 python build.py rc=0 rc=0 python --version rc=0 rc=0 py --list rc=0 rc=0 ### C-H3 newline-split producer (granted -> refused) printf 'a<NL>b<NL>' > notes.md rc=0 rc=2 echo "a<NL>b" > notes.md rc=0 rc=2 ec"<NL>"ho x > f (one bash word = echo) rc=0 rc=2 printf "a\nb\n" > notes.md (escaped control) rc=2 rc=2 cat > f with an earlier multi-line quote rc=2 rc=2 ### C-H3 blast-radius floor (unchanged) gh pr --body multiline mentioning echo > f rc=0 rc=0 git commit -m multiline prose mentioning cat > f rc=0 rc=0 grep "foo<NL>bar" file | wc -l rc=0 rc=0 multi-line span discarded to /dev/null rc=0 rc=0 multi-line span then a real producer+redirect rc=2 rc=2 multi-line span piped to wc rc=0 rc=0 multi-line sq span in --body, no redirect rc=0 rc=0 unterminated quote at end of command rc=0 rc=0 ### G1 reopened residual (granted -> refused) python3 - <<PY open-write rc=0 rc=2 python - <<PY open-write rc=0 rc=2 ### G1 stdin floor (unchanged) python3 <<PY open-write (no dash) [residual] rc=0 rc=0 python3 - <<PY read-only rc=0 rc=0 cat s.py | python3 - rc=0 rc=0 cat s.py | python3 rc=0 rc=0 echo "pathlib" | python3 rc=0 rc=0 python3 - </dev/null rc=0 rc=0 commit message quoting a heredoc write rc=0 rc=0 cat <<EOF > file (cat lane control) rc=2 rc=2 ``` **Shipped contract suite** — `bash plugins/guardrails/hooks/block-hook-bypass.test.sh`. Counts are pasted from the runs in the "Suite counts" comment below; no existing assertion changed. **Lint / repo checks run locally:** ``` $ shellcheck -x plugins/guardrails/hooks/block-hook-bypass.sh \ plugins/guardrails/hooks/block-hook-bypass.test.sh \ plugins/guardrails/lib/powershell/ps-command.sh (no output) $ shfmt -d <same three files> (no output) $ scripts/check-changelog-parity.sh --check Every versioned plugin has a CHANGELOG.md (or a stale-guarded baseline entry), and none documents a version above its manifest. $ scripts/check-changelog-parity.sh --check-order All 76 changelog(s) read newest-first with no duplicate versions. $ scripts/check-changelog-parity.sh --check-bump origin/main Every plugin whose version changed vs origin/main has a '## [<version>]' CHANGELOG.md entry. ``` ### Not a sign-off Per `OUTCOME.md`, the required `security-review` check has been observed reporting **pass in 16s on a ~700-line change to this same hook** (run `31558511903`) while the reviewer inside it did not run. A green `security-review` on this PR should not be read as a security review of it. This is a change to a guard whose whole job is refusing bypasses and it wants human eyes on the diff. ## Related Closes #2217. Inbox items: `2026-08-10-plugin-quality-audit-four-components` (C-H1, C-H3) and the `20260811-021645`-routed `audit-pass` report-path item (G1). Ledgers: `.work/handoff-inbox-batch-4/ledgers/I7-four-components-023241Z.md` §§ C-H1, C-H3; `.work/handoff-inbox-batch-4/ledgers/I8-audit-pass-report-path.md` § G1. Adjudication: `RECONCILE.md` AD-12. Adjacent and deliberately **not** closed by this PR: **#1601** and **#2148** report this same arm *over*-blocking. Both were re-verified `rc=0` at the base of this branch (the write-mode discrimination already fixed the mechanism #1601 names), and both repros are pinned as floors here — for the new spellings as well — so this widening does not reopen them. They stay open on their own terms. **#2227 needs no work: already shipped.** `repo_oid_width` at `origin/main` captures the git error via `2>&1`, caches only a successful width (`_repo_oid_width_key=""` on failure), and blocks with a distinct message via `_lease_oid_width_unknown` — PR #2288, commit `f3336eab`, `plugins/guardrails/CHANGELOG.md:134-135`. The issue is CLOSED/COMPLETED. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixes #2337 (in-repo slice) Adds \`scripts/verify-security-review-evidence.sh\` and a \`security-review-evidence\` sibling job in \`claude-security-review.yml\` that: - Applies only to in-scope PRs (paths from \`.github/claude-security-paths\`) - Skips exempt actors and skipped/out-of-scope runs - **Fails closed** when \`security-review\` succeeded in <45s or logs contain a validation skip / \`review-ran=false\` The reusable workflow's advisory infra-failure posture is unchanged; this guard targets the false-pass shape observed on #2287 (16s green with no review). ## Related - Refs #2287 (the false-pass shape this guard targets). --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Summary
A quoted redirect operand is one pathname to bash.
block-hook-bypassdecided its target-basedexemptions on that operand's first whitespace- or separator-delimited fragment, because the two
pieces of machinery either side of the decision disagree about what a kept operand is:
strip_literalskeeps a quoted write target as literal content (dropping the quotes, so a quotedtarget still reads as a write) while
normalize_segmentsthen reads a;,|,&,(,)ornewline inside it as a segment boundary, and
_redir_scan's target class ends at whitespace.So the
/dev/nulldiscard — the only target exemption that is on by default — fired for writes whosedestination was not
/dev/null. Measured at56f5cd21(0.25.3), hook invoked as a decision functionon a
PreToolUseBash payload:Reaching a chosen file this way needs a directory whose name ends in the whitespace-bearing
fragment to already exist, so this is correctness and defence-in-depth rather than a demonstrated
escape. It matters because it is the exact assumption every target-based exemption rests on, and this
guard now has two.
The mechanism
strip_literalsmarks a kept operand's literal content with two sentinels:\x03OPAQUE — one character whose literal value would read as syntax downstream, or abackslash escape this strip cannot reproduce faithfully (inside double quotes bash retains the
backslash unless it escapes
$, a backtick,",\or a newline — the old code dropped it andkept the escaped char, which is wrong). Inert to every scan, so the operand survives as one
token; its presence means the pathname is not recoverable here, so no exemption of any kind may
be granted.
\x04QUOTED — emitted where a kept span opens. The discard compare strips it, so> "/dev/null"is still a discard; the scratch axis keeps its shipped floor of never exempting aquoted operand.
A raw
\x01–\x04byte arriving in the command text is mapped to OPAQUE, so a forged sentinel canonly ever cost an exemption, never manufacture one.
Every mark is gated on
_in_redirect_operand— the same "this word began right after a>" testthe quoted-operand keep already used, now factored out and also applied to the unquoted backslash
branch. That gating is load-bearing, not tidiness:
normalize_segments,_producer_head,_cat_redirand every whitespace trim in the file are byte-for-byte as shipped, so an escapedseparator between commands (
echo x \; > f) still travels the unchanged\x02-to-space path and abackslash in a command word (
/c/Python313/python3.exe -c) is untouched.Both consumers, as the issue asks
With the association restored,
scratch_target_exemptno longer has to infer it from${COMMAND#*>}. The fail-close is keyed on the operand's own marks, which retires both blunt edges#2236 documented and #2235 pinned — it is segment-scoped now, and it is keyed on the operand rather
than on the first literal
>character. All four surfaces #2235 corrected move together again: thehook comment, the CHANGELOG, the README caveat paragraph, and the manifest's option description
(README options table regenerated by
sync-plugin-options-docs.py).Constraint 5 — direction of every moved verdict
Refusing an exemption is friction; granting one is a bypass. Counted mechanically from the new suite
run against
origin/main's hook below, which reports 19 failures splitting 15 / 4 by direction:/dev/nullfamily above in its quoted,single-quoted, partially-quoted, escaped, fd-numbered (
1>),cat-lane andreal-file-then-operand spellings; a multi-line quoted operand; an operand continued by a
backslash-newline; an empty quoted target (
> "", previously allowed, now blocks).echo x > /tmp/scratch/f && grep foo "notes.txt",echo x > /tmp/scratch/f; cat "notes.txt",echo "a > b" > /tmp/scratch/f, andecho 'x > y' > /tmp/scratch/f. Each lands only on a target the marks prove was bare — noquote mark, no opaque mark, no backslash — using the same quote tracking every other lane of this
guard already relies on to keep quoted prose inert. These are exactly the frictions docs(guardrails): state the true scope of block-hook-bypass's quoted-operand fail-close, and pin it #2235
documented as lost convenience.
(
> /dev/null\ ../../etc/pw) already blocked at 0.25.3. They are pinned here as regression guards,not flips — the marking must not stop them blocking.
(
> "/tmp/scratch/f") even though the pathname is now known precisely. Widening it would be agrant with no reported need, and keeping it leaves feat(guardrails): opt-in scratch-root exemption for block-hook-bypass write targets #2224's "even a benign quoted target is not
exempted" assertion untouched.
Two commit messages on this branch (
eba2ee6f,f775974b) state these as 19/3 and describe the;-compound as "added rather than flipped". Both were miscounted against the run above and arecorrected here and in the CHANGELOG; the numbers in this section are the ones that hold.
Shared machinery — #1680 and #1667 (constraint 3)
Both read this file's normalization. Measured, not reasoned: their shapes carry no quotes and no
backslashes, so no mark is ever emitted for them, and the code they concern (
normalize_segments's>&/<&/&>sentinel and its\&restore,_echo_file_out's leading class) is unchanged.Before and after on this branch, identical in both columns:
Neither issue moves in either direction; neither is fixed by this PR.
guardrails0.25.3 → 0.26.0 (minor: what is exempted changes in both directions). The 0.25.0 and0.25.1 entries are left as they shipped; 0.25.1 gains an erratum pointer inside it, mirroring the one
0.25.0 already carries.
Test plan
Adversarial-first: the tests were written and committed before the fix (
bb937fec), then thedeliberate assertion flips in their own commit (
f775974b), then the fix (eba2ee6f).The new suite run against
origin/main's hook at56f5cd21(same test file, main'splugins/guardrailsextracted withgit archive) — 19 real failures:The same suite on this branch:
What moved in the inherited assertions, stated exactly. #2224's 36 scratch-axis assertions and
#2235's boundary tests all still pass unmodified, with two exceptions:
the two
>-inside-quoted-content shapes; the unquoted compound is unchanged and stays as thecontrol). They are re-pinned in
f775974b, on their own, with the reasoning in the message — afourth grant, the
;-separated compound docs(guardrails): 0.25.0 describes block-hook-bypass's quoted-operand fail-close as operand-scoped, but it spans the whole command #2236 reported and nothing covered, is added there too.control: /dev/null still shows the inherited truncation (#2226, allowed). feat(guardrails): opt-in scratch-root exemption for block-hook-bypass write targets #2224 wrote thatassertion for exactly this moment — its PR body says it is "pinned here by a control test that
flips visibly when it is fixed" — so retiring it is that PR's own instruction, not a silent
loosening. It is replaced by six
/dev/nullassertions covering the whole family rather than justits
;spelling. It went out inbb937fecalongside the new tests rather than in its own commit;that is a process miss on my part and is called out here so a reviewer does not have to find it.
Gates, run locally in the worktree:
Security-review note: this change narrows the guard's trust surface on net. It adds no hook, no
grant, and no external read or write. Its grant direction is four shapes, each on a target proven
bare, listed above.
Cost note, since this file deliberately refuses forks and calls out a quadratic term 0.21.0 removed:
_in_redirect_operandis the same${out%…}pair the quoted-operand keep already ran per quotecharacter, but it now also runs per unquoted backslash, and it scans all of
out. AWindows-path-heavy command has more backslashes than quotes, so that is a real (small, string-op,
fork-free) addition to the per-call cost rather than a pure refactor.
Related
Closes #2226 —
block-hook-bypassexempts a quoted redirect target on its first word only.Follows up #2224 (merged, 0.25.0 — the scratch-root axis that surfaced this and pinned the
/dev/nullhalf with a control test that this PR flips) and #2235 / #2236 (merged, 0.25.1 — thescope-accuracy correction whose two documented blunt edges this PR retires).
Related, and deliberately not moved: #1680 and #1667, the other defects in this file's
normalization machinery. Measured before and after with no delta, evidence above.
Adjacent, not addressed: #547 (the guard-precision class issue).
Origin: handoff-inbox batch 4, lane owning
guardrails. #2226 was filed from implementation work on#2210 / #2224, not from a ledger row.