docs(guardrails): state the true scope of block-hook-bypass's quoted-operand fail-close, and pin it - #2235
Merged
Merged
Conversation
…se, and pin it Follow-up to the #2224 review, which landed after that PR merged. The reviewer is right that the check is broader than every doc surface claimed: it reads the whole raw tail after the first `>`, not the segment being evaluated and not the target word, so a quote in an unrelated LATER segment cancels the exemption for an earlier, unambiguous write. rc=2 :: echo x > /tmp/scratch/f && grep foo "notes.txt" rc=0 :: echo x > /tmp/scratch/f && grep foo notes.txt The behaviour stays. It is one-directional — the test can only ever refuse an exemption, never grant one — so the failure mode is lost convenience, not a bypass. Narrowing it means knowing which quotes belonged to the operand, which is exactly the association strip_literals destroys: #2226's root cause rather than a separate fix. What changes is accuracy and coverage. The hook comment, 0.25.0's CHANGELOG entry, the README and the manifest option description all said "a quoted or escaped operand"; all four now state that the test spans the whole command. Two regression tests pin both sides of the boundary, which the existing cross-segment test did not cover (it used an unquoted second segment with a genuinely non-exempt target). guardrails 0.25.0 -> 0.25.1: docs and tests only, no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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. |
Contributor
|
Claude Code is working… I'll analyze this and get back to you. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82944f012f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
|
Claude finished @kyle-sexton's task in 3m 43s —— View job Review complete
This is a docs/comments/tests-only correction (guardrails 0.25.0 → 0.25.1) — no code-logic line changed in Independently verified (by tracing the code, not by running the test suite —
One nit posted inline on No correctness, error-handling, or test-coverage issues found under this lane's scope (security is out of scope here — |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
… the operator
Review found the same defect one layer down, in the PR whose whole purpose is
scope accuracy. My correction said "after the first redirect operator". It is not
keyed on the operator at all: `${COMMAND#*>}` splits at the first literal `>`
CHARACTER and never decides whether that `>` is syntax. So a `>` inside quoted
content starts the scanned tail early and that content's own closing quote lands
inside it:
rc=2 :: echo "a > b" > /tmp/scratch/f
rc=2 :: echo 'x > y' > /tmp/scratch/f
rc=0 :: echo "hello world" > /tmp/scratch/f
That also made 0.25.0's "quotes before the redirect keep the exemption" claim
false as written — it holds only while the quoted content contains no `>`. Two
surfaces in this PR were contradicting each other, which is exactly the shape
#2236 was filed for.
The behaviour stays. Keying on the real operator and narrowing to the operand
need the same missing thing — knowing which `>` and which quotes are syntax
rather than content — which is what strip_literals destroys before this runs.
Same root cause as #2226, recorded there as further evidence, not attempted here.
All four surfaces now say "any quote or backslash after the first `>` character,
operator or not, anywhere in the command", and two more regression tests pin the
quoted-content case in both quote styles.
Also per review: 0.25.0's shipped entry is restored verbatim and carries a short
erratum pointer to 0.25.1 instead of being rewritten in place, so the changelog
still records what shipped at that version.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closed
1 task
Contributor
Security review complete
No security issues found. This PR is docs/tests-only, as the description states. Verified directly (not taken on trust): diffed
Out of scope per this lane's brief (and correctly so, since nothing here touches it): GitHub Actions trigger/permissions patterns, supply-chain/action pins — none of the changed files are workflows or dependency manifests. |
Contributor
|
Claude finished @kyle-sexton's task in 3m 6s —— View job Review complete
This lane reviews the docs-only correction as it stands after the latest push. Traced the logic myself (not relying on the PR's Test Plan claims) by hand-evaluating The latest commit ( The same commit also resolves the earlier Claude-review nit about rewriting the shipped One doc-only nit, not blocking: the PR description text (" No correctness, error-handling, or test-coverage issues found in the current diff. Security is out of scope for this lane. |
cursor Bot
pushed a commit
that referenced
this pull request
Aug 12, 2026
…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>
cursor Bot
pushed a commit
that referenced
this pull request
Aug 12, 2026
…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>
kyle-sexton
added a commit
that referenced
this pull request
Aug 12, 2026
…edirect operand (#2287) ## Summary A quoted redirect operand is **one pathname** to bash. `block-hook-bypass` decided its target-based exemptions 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_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 the `/dev/null` discard — the only target exemption that is on by default — fired for writes whose destination was not `/dev/null`. Measured at `56f5cd21` (0.25.3), hook invoked as a decision function on a `PreToolUse` Bash payload: ``` rc=0 :: echo x > "/dev/null ../../etc/pw" # the reported bypass — exempted on the word /dev/null rc=0 :: echo x > "/dev/null;/../../etc/passwd" rc=0 :: echo x > "/dev/null|/../../etc/passwd" rc=0 :: echo x > '/dev/null ../../etc/pw' rc=0 :: echo x > /dev/"null ../../etc/pw" rc=0 :: echo x > /dev/null\;/../../etc/passwd # the UNQUOTED escaped spelling, via \x02 to space rc=0 :: cat > "/dev/null ../../etc/pw" # the cat lane rc=2 :: echo x > "/tmp/scratch/a ../../etc/pw" # control: the scratch axis already fails closed ``` 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_literals` 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 `$`, a backtick, `"`, `\` or a newline — the old code dropped it and kept 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**. - `\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 arriving in the command text is mapped to OPAQUE, so a forged sentinel can only ever *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 load-bearing, not tidiness: `normalize_segments`, `_producer_head`, `_cat_redir` and every whitespace trim in the 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. ### Both consumers, as the issue asks 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 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: the hook 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: ``` $ … | grep '^FAIL' | awk -F'expected exit ' '{print $2}' | sort | uniq -c 4 0, got 2 <- moves to GRANTED 15 2, got 0 <- moves to REFUSED ``` - **REFUSES (security-positive), 15 shapes.** The `/dev/null` family above in its quoted, single-quoted, partially-quoted, escaped, fd-numbered (`1>`), `cat`-lane and real-file-then-operand spellings; a multi-line quoted operand; an operand continued by a backslash-newline; an empty quoted target (`> ""`, previously allowed, now blocks). - **GRANTS (the entire grant surface), 4 shapes.** `echo x > /tmp/scratch/f && grep foo "notes.txt"`, `echo x > /tmp/scratch/f; cat "notes.txt"`, `echo "a > b" > /tmp/scratch/f`, and `echo 'x > y' > /tmp/scratch/f`. Each lands only on a target the marks **prove** was bare — no quote 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 #2235 documented as lost convenience. - **In neither set:** a forged sentinel byte and an escaped-space operand (`> /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. - **Deliberately NOT widened.** The scratch axis still refuses a merely quoted operand (`> "/tmp/scratch/f"`) even though the pathname is now known precisely. Widening it would be a grant with no reported need, and keeping it leaves #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 are corrected 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: ``` rc=0 :: echo x >&2 rc=0 :: echo x 2>&1 rc=0 :: printf '%s' x >&2 rc=0 :: echo x >&2>file rc=0 :: echo x &>realfile.txt rc=0 :: cat 1>&2 rc=0 :: echo x 1>&2 rc=0 :: cat 1>&- ``` Neither issue moves in either direction; neither is fixed by this PR. `guardrails` 0.25.3 → **0.26.0** (minor: what is exempted changes in both directions). The 0.25.0 and 0.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 the deliberate assertion flips in their own commit (`f775974b`), then the fix (`eba2ee6f`). The new suite run against **`origin/main`'s hook at `56f5cd21`** (same test file, main's `plugins/guardrails` extracted with `git archive`) — 19 real failures: ``` FAIL: scratch: quote in an unrelated later segment keeps it (allowed): expected exit 0, got 2 FAIL: scratch: quote in a later ;-segment keeps it (allowed): expected exit 0, got 2 FAIL: scratch: > inside double-quoted content keeps it (allowed): expected exit 0, got 2 FAIL: scratch: > inside single-quoted content keeps it (allowed): expected exit 0, got 2 FAIL: #2226: quoted /dev/null + space fragment (blocked): expected exit 2, got 0 FAIL: #2226: quoted /dev/null + ; fragment (blocked): expected exit 2, got 0 FAIL: #2226: quoted /dev/null + | fragment (blocked): expected exit 2, got 0 FAIL: #2226: quoted /dev/null + & fragment (blocked): expected exit 2, got 0 FAIL: #2226: quoted /dev/null + parens (blocked): expected exit 2, got 0 FAIL: #2226: double-quoted /dev/null + backslash escape (blocked): expected exit 2, got 0 FAIL: #2226: single-quoted /dev/null + space fragment (blocked): expected exit 2, got 0 FAIL: #2226: partially-quoted /dev/null + space fragment (blocked): expected exit 2, got 0 FAIL: #2226: unquoted escaped ; in a /dev/null operand (blocked): expected exit 2, got 0 FAIL: #2226: 1> fd-numbered quoted operand (blocked): expected exit 2, got 0 FAIL: #2226: cat lane, quoted operand (blocked): expected exit 2, got 0 FAIL: #2226: real file then quoted operand (blocked): expected exit 2, got 0 FAIL: #2226: multi-line quoted operand (blocked): expected exit 2, got 0 FAIL: #2226: operand continued by backslash-newline (blocked): expected exit 2, got 0 FAIL: #2226: empty quoted target (blocked): expected exit 2, got 0 PASS=314 FAIL=19 ``` The same suite on this branch: ``` $ bash plugins/guardrails/hooks/block-hook-bypass.test.sh PASS=333 FAIL=0 ``` **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: - **Three of #2235's four boundary tests flipped** 2 → 0 (the two quote-in-a-later-segment shapes and the two `>`-inside-quoted-content shapes; the unquoted compound is unchanged and stays as the control). They are re-pinned in `f775974b`, on their own, with the reasoning in the message — a fourth grant, the `;`-separated compound #2236 reported and nothing covered, is added there too. - **One of #2224's 36 is RETIRED, not flipped:** `control: /dev/null still shows the inherited truncation (#2226, allowed)`. #2224 wrote that assertion 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/null` assertions covering the whole family rather than just its `;` spelling. It went out in `bb937fec` alongside 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: ``` $ 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 ``` 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_operand` is the same `${out%…}` pair the quoted-operand keep already ran per quote character, but it now also runs per **unquoted backslash**, and it scans all of `out`. A Windows-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-bypass` exempts 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/null` half with a control test that this PR flips) and #2235 / #2236 (merged, 0.25.1 — the scope-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. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to a review comment on #2224 that arrived after that PR had already merged. No
behaviour changes here — four documents become accurate, and two tests pin a boundary that was
unpinned.
0.25.0 shipped the scratch-root exemption with a fail-close on quoted or escaped redirect operands.
Every surface described it as refusing "a quoted or escaped operand". It is not operand-scoped:
the check reads the whole raw command tail after the first
>— not the segment beingevaluated, and not the target word. So a quote in an unrelated later segment cancels the exemption
for an earlier, unambiguous write.
Reproduced on
origin/mainatfd7ffc42(i.e. 0.25.0 as merged), root/tmp/scratch:Segment 1's target is a plain, unambiguous path under the configured root, and by the containment
rule alone it should be exempt — but the unrelated
grepargument's quote refuses it, so theordinary "write to scratch, then read something" pattern blocks entirely.
The behaviour stays, deliberately. The check is one-directional — it can only ever refuse an
exemption, never grant one — so the failure mode is lost convenience, not a bypass. Narrowing it
means knowing which quotes belonged to the operand, which is precisely the association
strip_literalsdestroys. That is #2226's root cause, not a separate fix, and it is notattempted here: #2226 is where the shared
strip_literalswork belongs, alongside #1680 and #1667.What changes:
block-hook-bypass.sh— the comment above the check now states the true scope explicitly, includingthe compound-command consequence and why narrowing it is fix(guardrails): block-hook-bypass exempts a quoted redirect target on its first word only #2226's problem.
CHANGELOG.md— a0.25.1entry recording the correction, and 0.25.0's own entry corrected so thetwo do not contradict each other.
README.mdand.claude-plugin/plugin.json— the option description and the guard's caveatparagraph both replace the "operand" framing with the whole-command one.
block-hook-bypass.test.sh— two regression tests pinning both sides of the boundary. Theexisting cross-segment test did not cover this shape: it used an unquoted second segment with a
genuinely non-exempt target, so it would pass either way.
guardrails0.25.0 → 0.25.1 (patch: docs and tests only).Test plan
The two new assertions, and the six existing quoted-operand ones they sit beside, verified directly
against the branch:
The full
block-hook-bypass.test.shcontract suite runs in theplugin-gateCI job on this branch.Gates, run locally in the worktree:
Related
Closes #2236 — the scope-accuracy defect this PR fixes: four surfaces shipped in 0.25.0 describe the
quoted-operand fail-close as operand-scoped when it spans the whole command, and no test pinned the
compound-command boundary.
Follows up #2224 (merged) and its issue #2210.
Related, and deliberately NOT closed by this PR: #2226 — the shared
strip_literalsoperand/quote association whose absence is why this check must be command-wide. Correcting the
documentation does not fix that root cause, and this PR does not attempt it; #2226 stays open
alongside its adjacent machinery issues #1680 and #1667.
Origin: handoff-inbox batch 4, lane C. A review finding on #2224 that arrived post-merge, not a
ledger row.