fix(guardrails): fd-dup merges and PowerShell token separators no longer hide computed writer calls - #2966
Conversation
…ger hide computed writer calls Two proven fail-opens in the PowerShell lane of the hook-bypass guard, each a working file write verified under pwsh. #2927 — `ps::call_site_operand_region` ends a call's operand region at a depth-zero `;` `|` `&`, and the `&` inside `2>&1` is at depth zero. The region of `& $w 2>&1 f.txt x` was truncated to `" 2>"`, both measuring probes went silent, and the command fell through allowed. `ps::write_bypass` already stripped fd-dup merges, but only into a separate `gate` variable feeding the `>` probe alone; the measuring probes got the unstripped text. The strip now runs once, before every probe in the branch. #2928 — the call-site separator classes were derived from bash character classes rather than from PowerShell's tokenizer. `=` is now in every one of them (entry AND measurement together, so widening does not repeat the #2922 / #2924 gate-admits-probes-cannot-see mechanism), and the Unicode gap is closed by NORMALIZING token-separating code points to an ASCII space at intake rather than by putting multi-byte sequences into a bracket expression, which would decompose into independent byte members under a single-byte locale and over-block accented paths. Accepted behavior change: `& $py a.py 2>&1 b.txt` moves 0 -> 2, consistent with `& $py script.py arg`, which already blocked. The six #2848 must-allow cases stay 0 on all three blocking hooks. Closes #2927 Closes #2928 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Q4QNEensQgkYm8vByTydJ
|
I'll analyze this and get back to you. |
|
Claude finished @kyle-sexton's task in 5m 0s —— View job Security review completeNo security issues found. See summary below.
This PR modifies #2927 fix: the fd-dup strip ( #2928 fix: Unicode normalization: applied once at intake in Test cases in all three Pre-existing, already-disclosed gap, correctly left alone: No injection, secrets-handling, or GitHub Actions concerns — only the bash guard library, tests, and changelog/version were touched. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 937088f161
ℹ️ 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".
|
Last security-reviewed head: |
…dynamic invocation or launcher from the fail-closed sink (#2992) Closes #2984 ## Summary An unspaced PowerShell assignment (`$out=pwsh $script`, `$a=& "$tool" …`) no longer hides a launcher or string-literal call from the fail-closed sink. `=` is matched as a PowerShell assignment operator, not as a generic token separator, so quoted text and `git -c <name>=<value>` config overrides stay out of that sink. ## Fix `ps::has_dynamic_invocation` and `ps::has_launcher` keep their original separators (space, `;`, `|`, `&`, `(`). The `#2984` unspaced-assignment hole is a separate `$name=` / `$scope:name=` arm ([about_Assignment_Operators](https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_assignment_operators)), scanned on quote-blanked text so quoted spans stay data ([about_Quoting_Rules](https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_quoting_rules)). git(1) [`-c <name>=<value>`](https://git-scm.com/docs/git#Documentation/git.txt--cltnamegtltvaluegt) (`git -c section.key=cmd`) has no `$name=` LHS and is not classified as a launcher assignment. Each regex is spelled out literally, never shared through a variable — per the file's quote-removal note on pattern position. ## Verification `block-dangerous-git.test.sh` on `adfd2d9a556fda0f2a3f7afc88305f49dd957b97`: **PASS=458 FAIL=0**. Hook rc (PowerShell `PreToolUse` envelopes): | command | expected | measured | |---|---|---| | `$out=pwsh $script` | 2 | 2 | | `$a=& "$tool" reset --hard` | 2 | 2 | | `$a=& 'git reset --hard'` | 2 | 2 | | `Write-Host "shell=pwsh $script"` | 0 | 0 | | `Write-Host 'pattern=& "$tool"'` | 0 | 0 | | `git -c section.key=cmd log --oneline -n 1` | 0 | 0 | | `git -c core.pager=cmd log --oneline -n 1` | 0 | 0 | | `$a=Get-Content f.txt` | 0 | 0 | Direct classification pins: | pin | result | |---|---| | `ps::has_launcher: quoted shell=pwsh is not a launcher` | rc=1 | | `ps::has_dynamic_invocation: quoted pattern=& "$tool" is not a call` | rc=1 | | `ps::has_launcher: git -c section.key=cmd is not a launcher assignment` | rc=1 | | `ps::has_launcher: $out=pwsh $script still is a launcher assignment` | rc=0 | | `classify: quoted =pwsh does not enter launcher sink` | trigger empty | | `classify: git -c section.key=cmd does not enter launcher sink` | trigger empty | | `classify: $out=pwsh $script still enters launcher sink` | trigger=launcher | | `Write-Host '$a=& "$tool" reset --hard'` | 0 | 0 | | `ps::has_dynamic_invocation: quoted $a=&` | rc=1 | | `classify: quoted $a=& does not enter dynamic-invocation sink` | trigger empty | Also: `block-no-verify.test.sh` PASS=230 FAIL=0; `check-changelog-parity.test.sh` PASS=84 FAIL=0. ## Related - Refs #2928 / PR #2966 — added `=` to the call-target classes; this is the sink-trigger half of the same boundary defect. - Refs #2922 and #2924 — the entry-broader-than-measurement failures; this is the inverse. - Refs #2848 — the computed-writer over-block removal whose six acceptance cases guard this change. - Refs #2965 — a different entry-side defect in the same sink-trigger path. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>

Closes #2927
Closes #2928
Two proven fail-opens in the PowerShell lane of the guardrails hook-bypass guard. Both let a real file write through — each shape parses clean under
[System.Management.Automation.Language.Parser]::ParseInputand writes the file under a realpwsh. Both are pre-existing rather than 0.28.x regressions: every bypass row measures 0 on the pre-0.28.33 base as well.Issue 2927 — an fd-dup merge truncated the operand region
ps::call_site_operand_regionends a call's operand region at a statement or pipeline separator (;|&) at bracket depth ZERO. The&inside2>&1sits at depth zero, so the region of& $w 2>&1 f.txt xwas truncated to" 2>"and both measuring probes went silent.ps::write_bypassalready knew fd-dup merges are plumbing rather than writes and stripped them — but only into a separategatevariable that fed the>redirect probe alone, whileps::computed_call_has_splat_operandandps::computed_call_has_positional_write_signalwere handed the unstripped text. That divergence between what the gate stripped and what the probes measured WAS the defect. The strip now runs once, before every probe in the branch, and the separategatevariable is gone.Issue 2928 — call-site boundaries came from bash character classes, not PowerShell's tokenizer
=is now in the separator class of every call-target predicate: gate entry (ps::call_target_is_bare_computed), the subexpression half (ps::call_target_is_bare_subexpression), the interpolating-string half, there_varof both measuring probes, the quoted-writer regex inps::write_bypass, andps::might_invoke_git's launcher class. Entry and measurement move together on purpose — widening entry alone is the exact "gate admits, probes cannot see" mechanism behind issue 2922 and issue 2924, so widening one without the others would have manufactured a third instance of it.Unicode whitespace is closed by NORMALIZING at intake, not by widening the classes. Under a single-byte locale a multi-byte sequence inside a bracket expression decomposes into independent byte members, and
\xa0is the second byte ofà— so a class-widening fix would have split ordinary accented paths into extra operands, an over-block of exactly the class issue 2848 exists to keep closed.ps::blank_herestringsnow maps every token-separating code point to an ASCII space, spelled as raw UTF-8 byte sequences ($'\xc2\xa0') so the substitution is byte-identical under the C and UTF-8 locales. The guards pin no locale, so a fix that only worked under one would fail open under the other.The separator set was derived by MEASUREMENT, not from a Unicode category table: each candidate was parsed and kept only when
& $w<CH>f.txt xproduced ONE command of THREE elements. U+200B and U+FEFF measured two elements — they sit inside the token, the target never resolves, and there is nothing to hide — so they are excluded and pinned as such.,& $w f.txt x, raised in the issue as a possible third member, was ruled out the same way: it does not parse ("Missing expression after unary operator ','.").Probe table
rc from each hook, payloads built with
jqas realPreToolUseenvelopes.base=6dc12e45a. Attribution measured on two extra trees, each carrying exactly one of the two fixes.The two fail-opens, with per-fix attribution
& $w 2>&1 f.txt x& $w 2>&1 @p& $env:w 2>&1 f.txt x'x' | & $w 2>&1 f.txt& $w; & $w2 2>&1 f.txt x$a=& $w f.txt x=)& $w<U+00A0>f.txt x& $w<U+2003>f.txt x$a=& $w<U+00A0>f.txt x& $w<U+00A0>@p$a=& 'Set-Content' f.txt x=, quoted-writer regex)$a=& ($w) f.txt x=, subexpression predicate)Neither fix closes any of the other's rows. An all-rc=2 union would not have distinguished them.
Bonus closes on the git lanes (the normalization sits at intake)
git commit --no-verify<U+00A0>-m xblock-no-verifygit<U+00A0>commit --no-verify -m xblock-no-verifygit push --force<U+00A0>origin mainblock-dangerous-gitgit<U+00A0>reset --hardblock-dangerous-git$p=Start-Process ('g'+'it') resetblock-dangerous-git,block-no-verifyACCEPTED behavior change
& $py a.py 2>&1 b.txt& $py script.py arg, which already blocked. The class is narrow: it needs a positional on BOTH sides of the merge —& $npm run build 2>&1 log.txtwas already 2 at base.The six issue-2848 must-allow cases — rc=0 on all three hooks, base AND merged
$py = "..."; if (-not (Test-Path $py)) { $py = (Get-Command python).Source }; & $py run.py --flag& $py $script (Join-Path $dir "$id.jsonl")& $py -m unittest discover. $PROFILE& $py script.pyforeach ($x in @('a')) { & $w f.txt }Negative controls — the hooks are not silently no-opping
git commit --no-verify -m xgit push --force origin maincat > somefile.txtSet-Content -Path out.txt -Value higit commit --no-verify -m xgit push --force origin mainOver-block guards — 0 at base and 0 merged
& $tool 2>&1git status 2>&1 > out.txt& $py café.py\xa0must not act as a separator member& $py -m café& $w café.txt& $py -m pip install x 2>&1& $w<U+200B>f.txt x$out=pwsh $script=widening must not over-block$p=Start-Process $appgit log --oneline<U+00A0>-n 5Tests
Every new case asserts an EXACT rc, never
RC -ne 0. The Unicode separators are built from byte escapes ($'\xc2\xa0'), never pasted as literal characters — a formatter or.gitattributesrule that normalized a raw U+00A0 to a plain space would degrade the row to& $w f.txt x, which blocks anyway, leaving a case that passes while pinning nothing.Mutation-checked per case against a tree with the fix reverted: every "expect 2" case measures 0 without the fix (RED) and 2 with it (GREEN); every "expect 0" case measures 0 in both.
Residual noticed, not fixed here
$out = pwsh $scriptfails closed onblock-dangerous-gitwhile$out=pwsh $scriptstays allowed, because the sink TRIGGERps::has_launchercarries its own separator class that still lacks=. Widening that one is the over-block direction (it would flip ordinary launcher assignments from 0 to 2), so it is left alone and flagged rather than folded in.Related
$( … )call-target fail-open; the$()spelling of the 2927 shape is refused by that shape arm, the bare-variable spelling was not.ps::call_site_operand_region. Its six must-allow cases are pinned unchanged above.🤖 Generated with Claude Code
https://claude.ai/code/session_018Q4QNEensQgkYm8vByTydJ