fix(guardrails): an unspaced assignment no longer hides a PowerShell dynamic invocation or launcher from the fail-closed sink - #2992
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 50s —— View job Security review
The two regex widenings in However, review of the surrounding file turned up an unaddressed instance of the identical defect class, in the same file, in a sibling sink-trigger function this PR does not touch:
Failure scenario:
None of the gates match, so This is pre-existing (not introduced by this diff), but it's the same fail-open boundary-disagreement class the PR title and changelog explicitly claim to close ("entry and measurement have to agree on what a token boundary is, in both directions") — left open one function away. Given the PR is already touching this exact file for this exact defect shape, worth closing in the same pass or a fast follow-up: add No other security-relevant issues found. |
![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: fa1c06bee4
ℹ️ 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: |
|
Claude finished @kyle-sexton's task in 5m 43s —— View job Code review complete
Note on process: the SummaryThe fix itself — spelling One likely regression, posted as an inline comment on No other correctness or maintainability issues found; the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 5 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
|
Claude finished @kyle-sexton's task in 2m 53s —— View job Security review
No security vulnerabilities found. Scope of this passThe last security review on this PR covered head What that commit doesIt re-pins the The previously-open |
|
Claude finished @kyle-sexton's task in 3m 25s —— View job
|
Session stopped here — state and remaining work, for whoever picks this upThe lane working this PR was stopped mid-flight. Nothing is lost; the worktree was clean and both commits are pushed. This comment carries what would otherwise have died with the session. Where this standsHead is The defect, measured independently against The only difference in each pair is whitespace around What is NOT yet done
Why the verifier is not a formality hereThe sibling lane's PR 2983 had fully green CI — 47 success, 1 skipped — and 25 pinned tests, and still introduced a new fail-open while closing one: a command that merged main blocks at 2/2 was waved through at 0/0 by that branch. Green CI is not evidence of correctness in this file; only an adversarial probe is. This change widens a sink trigger, which is the over-block direction — but a regex edit that silently stops matching fails OPEN, and this file's own block comment says exactly that about variable-in-pattern-position constructs. So probe each edited pattern individually; one probe for the family proves nothing about which pattern actually fired. Pin the allow side too, so a future widening cannot silently over-block ordinary assignment idiom. Standing constraints for this repoVerify merged content by executing probes, never by reading PR or issue state. Diff against the merge commit's true parent, not the recorded base. Check base staleness by file overlap immediately before merging. Squash is the only permitted merge method. Never use A sibling lane is live on the same file at roughly line 300 (PR 2983). Whoever merges second rebases. Related
|
9af6550 to
598aae8
Compare
|
Claude finished @cursor[bot]'s task in 5m 41s —— View job
|
…dynamic invocation or launcher from the fail-closed sink PR 2966 added = to the CALL-TARGET separator classes. The SINK-TRIGGER classes one layer up -- ps::has_dynamic_invocation and ps::has_launcher -- still lacked it, so the two lanes disagreed about what a token boundary is. This is the mirror image of issues 2922/2924: there gate ENTRY was BROADER than every measuring predicate, so the gate was entered and no arm fired; here entry was NARROWER than measurement, so the sink was never entered and the measuring predicates never ran. Both directions fail OPEN. Closes #2984 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Q4QNEensQgkYm8vByTydJ
git -c core.pager=cmd log fails closed after the widening. That is parity: the identical collision already blocked on the pre-fix base under a space or pipe separator, and only the =-glued spelling did not. Benign neighbours (core.pager=cat, plain git log, a =-glued launcher token with no git) are pinned alongside it to show the trigger is the launcher word, not the =. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Q4QNEensQgkYm8vByTydJ
…ot a generic separator The first pass put `=` in the same class as `;` `|` `&`. That closed #2984 (`$out=pwsh`, `$a=& "…"`) but also matched data inside quotes (about_Quoting_Rules) and git(1) `-c <name>=<value>` (`git -c section.key=cmd`). `=` is now a separate `$name=` / `$scope:name=` arm, scanned quote-blanked. The #2984 unspaced-assignment cases still enter the sink. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
…terals The new quoted-equals and classifier pins keep `$script` / `$tool` / `$out` inside single quotes so bash does not expand them. Add the same per-command SC2016 disable the rest of this file already uses. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
25d396d to
80739f9
Compare
|
Claude finished @cursor[bot]'s task in 5m 23s —— View job /review:code-review — CI code-review lane
Scope noteThis lane covers correctness/maintainability; security-relevant findings are left to SummaryNo blocking correctness or maintainability issues found. The design settled on in the latest fix (
Consistency check: The regex classes remain spelled out literally in each predicate rather than shared through a variable, consistent with the file's own quote-removal convention. Verification noteI could not execute Non-blocking observationThe stopped-session tracking comment mentions a planned Latin-1/accented-name probe row that was never added to the test file. |
…ng is exercised The Codex `pattern=&` row fails the `$name=` structural check even unquoted. `Write-Host '$a=& "$tool" reset --hard'` is the assignment-shaped call inside quotes, so it reaches the quote-blanked confirmation the way the launcher `$out=pwsh` pin already does. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Claude finished @cursor[bot]'s task in 4m 4s —— View job
|
…ctually runs `$a` glued to the opening quote fails the quote-intact predecessor class before blanking runs. `Write-Host "x; $a=& 'ls'"` puts `$name=` after `;` inside a double-quoted span, so the structural check matches and quote-blanking is what rejects it. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
2 participants
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 andgit -c <name>=<value>config overrides stay out of that sink.Fix
ps::has_dynamic_invocationandps::has_launcherkeep their original separators (space,;,|,&,(). The#2984unspaced-assignment hole is a separate$name=/$scope:name=arm (about_Assignment_Operators), scanned on quote-blanked text so quoted spans stay data (about_Quoting_Rules). git(1)-c <name>=<value>(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.shonadfd2d9a556fda0f2a3f7afc88305f49dd957b97: PASS=458 FAIL=0.Hook rc (PowerShell
PreToolUseenvelopes):$out=pwsh $script$a=& "$tool" reset --hard$a=& 'git reset --hard'Write-Host "shell=pwsh $script"Write-Host 'pattern=& "$tool"'git -c section.key=cmd log --oneline -n 1git -c core.pager=cmd log --oneline -n 1$a=Get-Content f.txtDirect classification pins:
ps::has_launcher: quoted shell=pwsh is not a launcherps::has_dynamic_invocation: quoted pattern=& "$tool" is not a callps::has_launcher: git -c section.key=cmd is not a launcher assignmentps::has_launcher: $out=pwsh $script still is a launcher assignmentclassify: quoted =pwsh does not enter launcher sinkclassify: git -c section.key=cmd does not enter launcher sinkclassify: $out=pwsh $script still enters launcher sinkWrite-Host '$a=& "$tool" reset --hard'ps::has_dynamic_invocation: quoted $a=&classify: quoted $a=& does not enter dynamic-invocation sinkAlso:
block-no-verify.test.shPASS=230 FAIL=0;check-changelog-parity.test.shPASS=84 FAIL=0.Related
$a=& $w …, U+00A0) #2928 / PR fix(guardrails): fd-dup merges and PowerShell token separators no longer hide computed writer calls #2966 — added=to the call-target classes; this is the sink-trigger half of the same boundary defect.