fix(guardrails): measure a braced call target like its bare twin - #2908
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. |
|
Claude finished @kyle-sexton's task in 4m 25s —— View job Code review: PR #2908Scope: Verdict: no blocking findings. The fix is correct and well-isolated. What I checked
One non-blocking documentation noteIn both new comment blocks
Bash's |
|
Claude finished @kyle-sexton's task in 2m 27s —— View job Security review complete ✅
No security issues found. This PR is itself a security hardening fix — it closes a fail-open regression in the PowerShell write-bypass guard (
I could not execute the test suite or The pre-existing |
|
Last security-reviewed head: |
|
Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
![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: 62c5768066
ℹ️ 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".
`& ${env:writer} f.txt x` and `& ${env:writer} @p` exited 0 from
block-hook-bypass while the identical `& $env:writer …` exited 2 — a
fail-open on a working file write, introduced by 0.28.33 and found by a
fresh-context verifier running against merged content.
`${env:w}` and `$env:w` are the same reference (about_Variables;
`${env:t} -eq $env:t` is True), and ps::call_target_is_bare_computed
admits both because it only looks for the `$`. But the two probes that
MEASURE a call site — ps::computed_call_has_splat_operand and
ps::computed_call_has_positional_write_signal — keyed on the bare
spelling alone. A braced target therefore entered the computed-target
gate and then matched no call site at all: every arm stayed silent and
the command fell through allowed. Before 0.28.33 the blanket
ps::has_special_constructs arm covered the shape incidentally, via the
braces themselves.
Both probes now accept `${…}` alongside the bare form, listed first so
it wins on a braced target. The gate ENTRY predicate is deliberately
unchanged: teaching the measuring probes closes the hole, narrowing
entry to match would open a second one.
Pinned by seven tests, each braced row paired with its bare twin so the
two spellings cannot drift apart again, plus `& ${env:py} script.py`
holding the allowed side.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Q4QNEensQgkYm8vByTydJ
…ng backticks
A braced PowerShell variable name may contain a closing brace by escaping it:
`${my`}writer}` names the variable `my}writer` (about_Variables). The library
deletes backticks before matching so a cmdlet name obfuscated with PowerShell's
escape character resolves to its real form — but that deletion runs FIRST, and it
turns the braced name into `${my}writer}`, text genuinely indistinguishable from
a `${my}` reference followed by the literal `writer}`.
No rule applied after the deletion can tell those apart. The braced-target
alternative's `[^}]*` stopped at the injected brace, the whitespace boundary
never matched, and the call site disappeared: both measuring probes returned
false, every arm of the computed-target gate stayed silent, and
`& ${my`}writer} f.txt x` / `& ${my`}writer} @p` fell through ALLOWED — the same
computed-writer fail-open the braced-target change exists to close.
Consume the escape while it still exists. `ps::fold_escaped_brace_closers` folds
each backtick-escaped closer to one ordinary name character, left to right,
before `ps::write_bypass` deletes backticks. An escaped backtick is consumed as a
unit and emitted unchanged, so it cannot lend its second backtick to a following
brace and the obfuscation recovery is untouched. An escaped OPENING brace needs
no handling — deleting its backtick leaves a brace `[^}]*` matches and the real
closer still terminates.
The target token may also carry non-space text glued after its closing brace,
which PowerShell concatenates (`& ${env:w}riter f.txt x`, and the `writer}`
trailing `${my``}`). Requiring whitespace immediately after `}` made those call
sites unmatchable, which is the same fail-open in another spelling. Operands are
measured exactly as before once a site is found, so widening the target token
decides only where measuring starts, never the verdict.
Eight further tests pin the shapes, including two escaped closers in one name,
an escaped backtick followed by a real closer, and the allowed one-positional
twin so consuming the escape does not itself become a write signal.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Q4QNEensQgkYm8vByTydJ
62c5768 to
901a481
Compare
|
Claude finished @kyle-sexton's task in 3m 57s —— View job Code review: PR #2908 (head
|
|
Claude finished @kyle-sexton's task in 2m 33s —— View job Security review complete ✅
No security issues found. Since the last security review (head I traced the fix rather than taking the commit message at face value:
Nothing else in this PR's surface (all four changed files, entire diff vs |
Closes #2924 ## What was open `block-hook-bypass` allowed a computed writer call whose target is spelled `$( … )` while blocking the `( … )` spelling of the same construct. `block-dangerous-git` carried the identical hole. Measured with `jq`-built `PreToolUse` envelopes (no shell quoting touched the command string), `base` = `10bdd7c51` (pre-#2890), `main` = `a456023af`: | hook | command | base | main | this PR | |---|---|---|---|---| | block-hook-bypass | `& $($w) f.txt x` | 2 | **0** | 2 | | block-hook-bypass | `& $($w) @p` | 2 | **0** | 2 | | block-hook-bypass | `& ($w) f.txt x` (paren twin) | 2 | 2 | 2 | | block-dangerous-git | `& $($g) reset --hard` | 2 | **0** | 2 | | block-dangerous-git | `& ($g) reset --hard` (paren twin) | 2 | 2 | 2 | The `0` cells are a working `Set-Content <path> <value>`, and a working `git reset --hard`, sailing past their guards. ## Cause The same "gate admits, probes cannot see" mechanism as #2922, one construct over. - `ps::call_target_is_bare_computed` — the gate ENTRY predicate — matches `[.&][[:space:]]*[$(]`, so `& $(` enters the computed-target branch of `ps::write_bypass` on the `$` alone. - `ps::call_target_is_bare_subexpression` required `(` **immediately** after the call operator, so it never fired on `$(`. - `ps::computed_call_has_positional_write_signal` and `ps::computed_call_has_splat_operand` both require a `$name` or `${name}` target; `$(` is neither, so neither probe located a call site at all. Gate entered, zero arms fired, command fell through allowed. Before 0.28.33 the blanket `ps::has_special_constructs` arm covered the shape incidentally, via the parentheses inside `$(`. ## Fix `ps::call_target_is_bare_subexpression` now accepts an OPTIONAL `$` before the opening paren, so `$( … )` and `( … )` — one construct, two spellings — reach one verdict on both lanes. The `$` is ESCAPED in the pattern. An unescaped `$?` in that position is a parameter expansion of the last exit status, which would silently rewrite the pattern and stop it matching the paren spelling too: a fail-OPEN on `& ('Set-'+'Content') f.txt x`, the very shape the predicate exists for. That row is kept in the test set as the tripwire. Unlike #2908 this is deliberately NOT a pre-deletion transform. Nothing destroys the evidence upstream here — `$(` survives backtick deletion, quote blanking, and lowercasing intact — so the fix belongs in the predicate, not in a pass ahead of it. ## The one rc change that is not a bypass row `& $($py) script.py` goes 0 to 2. That is intended, and it is not a reintroduced over-block: a SUBEXPRESSION target is refused BY SHAPE regardless of its operands, which is why `& ($py) script.py` is rc=2 on `main` today and on the pre-#2890 base. #2848 dropped grouping ANYWHERE ELSE in the command as a write signal and deliberately KEPT the target-is-subexpression arm. Allowing the dollar spelling while the paren spelling blocks would be a fresh instance of the defect class this PR closes. `ps::write_bypass` still contains zero `ps::has_special_constructs` calls — the #2848 fix is untouched. ## Over-block regression set (all still rc=0 on all three hooks) `REPRO_2592`, `BLOCK_A`, `BLOCK_C`, `GROUPING_ONLY`, `COMPUTED_ONLY`, `CONTROL_1973`, and the splat pair `Write-Output @Args; & $py script.py` / `Write-Output; & $py script.py`. ## Siblings probed and NOT fixed here Confirmed rc=0 on the pre-#2890 base as well, so pre-existing rather than a #2890 regression, and already enumerated in #2924's own body — no new issues filed: - interpolating quoted targets: `& "$env:writer" f.txt x` - index and member targets: `& $tools[0] f.txt x`, `& $tools.writer f.txt x` - quoted operands erasing the positional write signal (#2906) ## Tests Twenty rows across `block-hook-bypass.test.sh` and `block-dangerous-git.test.sh`, each `$( … )` row PAIRED with its `( … )` twin so the two spellings are pinned to one verdict and cannot drift apart again. Covers positional Path+Value, splat, dot-source, glued and extra-whitespace operators, nested `$($())`, `$(& $w)`, `$(Get-Command x).Source`, a braced reference inside the subexpression, a scoped variable, and the assembled-name tripwire in both spellings. ## Related - PR #2890 — narrowed the computed-target gate and introduced this fail-open family (guardrails 0.28.33). - PR #2908 — closed the BRACED spelling (#2922) with the same "teach the measuring side, do not narrow entry" shape. - Issue #2922 — the braced spelling of this class. - Issue #2906 — quoted operands erase the positional write signal. Genuinely pre-existing; deliberately NOT addressed here, since its fix changes deletion-to-placeholder semantics with real over-block risk. - Issue #2848 — the over-block fix whose narrowing this PR must not and does not reverse. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_018Q4QNEensQgkYm8vByTydJ Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 participant
Follow-up to PR 2890 (
4e6249ae7). Fixes a fail-open regression that PR introduced, found by a fresh-context verifier agent probing merged content adversarially.Closes #2922.
The defect
& ${env:writer} f.txt xexited 0 fromblock-hook-bypassonorigin/main, while the identical& $env:writer f.txt xexited 2. Both are a workingSet-Content <path> <value>through a computed call target.${env:w}and$env:ware the same reference —about_Variables;${env:t} -eq $env:tevaluates True on PowerShell 7. The two spellings were getting opposite verdicts:ps::call_target_is_bare_computed(the gate ENTRY predicate) matches[.&][[:space:]]*[$(]— it only looks for the$, so a braced target enters the computed-target branch ofps::write_bypass.ps::computed_call_has_splat_operandandps::computed_call_has_positional_write_signal(the probes that MEASURE a call site) both keyed onre_varrequiring\$[a-z0-9_:?]+.{is not in that class, so neither located a call site at all.Before 0.28.33 the blanket
ps::has_special_constructsarm covered the shape incidentally, via the braces themselves. Removing that arm is the point of issue 2848; this shape was the one it had been carrying that the replacement did not name.Evidence
Every rc below was OBSERVED, not asserted. Payloads built with
jqas realPreToolUseenvelopes (no shell quoting touched the command string).base=10bdd7c51(pre-2890),main=9cf4587f7(current),fix= this branch. rc fromblock-hook-bypass.sh:& ${env:writer} f.txt x& ${env:writer} @p. ${env:w} @p& ${script:w} f.txt x& ${global:w} @p& ${my writer} f.txt x& ${my`}writer} f.txt x& ${my`}writer} @p. ${my`}writer} @p& ${my`}w`}x} f.txt x& ${my}writer} f.txt x``& ${my`{writer} f.txt x& ${env:w}riter f.txt x& $env:writer f.txt x(bare twin)& ${env:py} script.py(allowed side)& ${my`}py} script.py(allowed side)The two allowed rows read
base= 2 because a braced target is itself a brace, so the blanketps::has_special_constructsarm blocked an ordinary interpreter call before 0.28.33. That over-block is exactly what issue 2848 set out to remove, and this PR keeps it removed — the allowed rows stay rc=0. An earlier revision of this body listed those two rows asbase= 0; that was asserted rather than measured, and is corrected here.The fix
1. Both measuring probes accept
${…}. The braced alternative is listed first so it wins on a braced target, and the target token may carry non-space text glued after its closing brace — PowerShell concatenates& ${env:w}riter, and requiring whitespace immediately after}made that whole call site unmatchable, which is the same fail-open in another spelling:[^}]*is what reaches& ${my writer} f.txt x— a braced name may contain a space, which no bare-name character class can express. That row is the isolating pin for the braced alternative.The gate ENTRY predicate is deliberately left unchanged. Teaching the measuring probes closes the hole; narrowing entry to match the probes would have opened a second fail-open instead.
2. A braced name's ESCAPED closing brace is consumed before backticks are deleted (review round on this PR).
${my`}writer}names the variablemy}writer, butps::write_bypassdeletes backticks first so a cmdlet name obfuscated with PowerShell's escape character resolves to its real form. That deletion rendered the text${my}writer}— genuinely indistinguishable from a${my}reference followed by a literalwriter}. No rule applied after the deletion can tell those apart, which is why the fix has to run before it. Newps::fold_escaped_brace_closersfolds each escaped closer to one ordinary name character, left to right, at the pointlcqis built. An escaped backtick is consumed as a unit and emitted unchanged, so it cannot lend its second backtick to a following brace and theSet-Content`` obfuscation recovery is untouched. An escaped OPENING brace needs no handling — deleting its backtick leaves a brace[^}]*matches and the real closer still terminates.Operands are measured exactly as before once a call site is found, so widening the target token decides only where measuring starts, never the verdict.
Tests
Fifteen new rows in
plugins/guardrails/hooks/block-hook-bypass.test.sh, each blocked braced row paired with its bare twin or its allowed one-positional twin so the spellings are pinned to the same verdict and cannot drift apart again — braced target with positional Path+Value, with a splat, dot-sourced, script-scoped, a name containing a space, an escaped closer in each of those positions, two escaped closers in one name, an escaped backtick followed by a real closer, an escaped opening brace, a glued trailing target token, and both allowed twins.Verification
REPRO_2592,BLOCK_A,BLOCK_C,GROUPING_ONLY,COMPUTED_ONLY,CONTROL_1973are rc=0 on all three hooks.shellcheck --rcfile .shellcheckrcclean;markdownlint-cli2clean; all fourcheck-changelog-parity.shmodes pass, including--check-preservedconfirming all 106 headings main carried survive byte-identical.Not included, filed separately
The verifier also surfaced that
ps::blank_quoted_spansdeletes quoted spans, so& $w 'f.txt' 'x'evades the positional write signal. That is pre-existing — rc=0 on10bdd7c51as well as on4e6249ae7— and fixing it changes deletion-to-placeholder semantics several probes depend on, with real over-block risk. Filed as issue 2906 with its own probe table rather than bolted onto this hotfix.Related
4e6249ae7) — the change that introduced this fail-open. Not closed here; its own issue 2848 stays closed and its acceptance cases are re-pinned above.ps::blank_quoted_spansdeletes quoted operands, so quoting evades the same positional write signal. Pre-existing, deliberately out of scope for this hotfix.302a6fa95) — the command-position fix;REPRO_2592stays allowed and pinned.CONTROL_1973is that fix holding.ps::write_bypassthat established the bare-computed-target half; its four write-signal pins are unchanged.🤖 Generated with Claude Code
https://claude.ai/code/session_018Q4QNEensQgkYm8vByTydJ