fix(guardrails): apostrophe in a double-quoted string no longer deletes the command between two such strings - #2983
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 3m 48s —— View job Code review complete
I traced the new The 25 added test cases across the three hook test files line up with the PR's bypass-closure and over-block claims, and the One minor, non-blocking finding posted inline:
No correctness or maintainability issues found beyond that. Nice work — this is a thorough, well-tested fix. |
|
Claude finished @kyle-sexton's task in 7m 17s —— View job Security review — PR #2983
|
|
Claude has reviewed this PR 3 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40f2081493
ℹ️ 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 4m 40s —— View job Code review complete
FindingsConfirmed, high-signal (posted inline on A backtick-escaped quote that is immediately followed by the string's real terminator (a doubled quote right after the escape) closes the span one character early, same as the pinned-safe case at This is also a real test-coverage gap: none of the 25 pinned cases exercise a backtick-escaped quote immediately followed by its own terminator — only the safe "backtick then later real quote" shape is pinned. Details and a fix pointer are in the inline comment. This should block merge until either the walk is fixed to not leave a dangling opener in this shape, or pinned tests + a documented residual establish the actual (narrower) safety boundary. Not re-flagged: the pre-existing non-blocking nit about the redundant trailing-newline-strip loop/comment at Everything else (test structure, |
|
Claude finished @kyle-sexton's task in 4m 53s —— View job Security review (
|
|
Claude finished @kyle-sexton's task in 3m 2s —— View job Security review (
|
|
Claude finished @kyle-sexton's task in 10m 29s —— View job Code review complete
This is the code-review lane (correctness/maintainability); security findings are scoped to The critical fix (
|
Session stopped here — state and remaining work, for whoever picks this upThe lane working this PR was stopped mid-flight. Nothing is lost; everything is pushed. This comment carries what would otherwise have died with the session. Where this standsHead is The regression: the left-to-right blanker refused the backtick escape and closed a quoted span one quote-character early, leaving the string's true closing quote as an unpaired opener. That opener then scanned forward to the next literal quote anywhere on the line — including one embedded in an unrelated single-quoted string — and blanked everything between, swallowing a dangerous construct. Measured independently, not taken from a report:
Also re-measured at The original issue-2965 rows still block and the acceptance cases still pass. What is NOT yet done
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 lines 910 to 950 (PR 2992). Whoever merges second rebases. Related
|
…es the command between two such strings
`ps::blank_quoted_spans` paired quote characters with two independent `sed`
expressions, neither aware of which quote style opened first. An apostrophe
inside a double-quoted string is a literal character to PowerShell, but the
single-quote expression treated it as a delimiter and matched from the
apostrophe in one string to the apostrophe in the next, deleting everything
between them.
in: Write-Host "a'b"; & ('g'+'it') push --force; Write-Host "c'd"
out: Write-Host
This is an ENTRY-side failure. With the `(` deleted, has_special_constructs saw
no construct, has_dynamic_invocation saw no call and has_launcher saw no
launcher, so classify_git_command never entered the fail-closed sink and no
downstream probe ran at all. Two ordinary apostrophe-bearing strings were a
general bypass of all three blocking hooks.
Replaced by one left-to-right walk in which whichever quote character opens
first owns everything up to its own next occurrence. Ambiguity resolves toward
NOT deleting, since this is an entry scan where leaving text in view can only
over-block: an unterminated opener emits the rest of its line verbatim, a span
never crosses a newline, the doubled-quote escape is over-blocked rather than
modeled, and smart quotes are not delimiters. The walk deliberately does not
honor the backtick escape and so does not reuse ps::_skip_double_quote --
honoring it extends a span past `"a`"` to the next real quote and reopens this
same bypass, measured at rc 0 on both git hooks.
Closes #2965
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Q4QNEensQgkYm8vByTydJ
`mis-measurement` is flagged by typos-cli as `mis` -> `miss`/`mist`. Same meaning, no vocabulary change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Q4QNEensQgkYm8vByTydJ
… past its own string
The left-to-right blanker refused the backtick escape and closed the span
one quote-character early, leaving the string's true closing quote as an
unpaired opener. That opener then scanned forward to the next literal
quote anywhere on the line -- including one embedded in an unrelated
single-quoted string -- and blanked everything between, swallowing a
dangerous construct.
Reported on this PR by two independent reviewers and confirmed by
measurement: `Write-Host "a`""; & ('g'+'it') push --force; Write-Host
'b"c'` measured rc=2 on block-dangerous-git and block-no-verify on
merged main and rc=0 on this branch before this commit.
The disturbance is now contained to the string it originates in, so
ambiguity still resolves toward not deleting rather than toward
extending a span, which is the direction that would reopen the original
issue.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Q4QNEensQgkYm8vByTydJ
…pans All five callers capture via command substitution, which already strips trailing newlines. The loop and its "callers see that shape" comment never changed what any current caller observed. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
…iguity arm The pinned case reaches write_bypass through lcq_bt, not the backtick-stripped lcq, so the backtick-ambiguity branch fires first. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
…ty boundary The 0.29.4 entry still described naive pairing for the doubled-quote escape and only the first backtick failure mode. Match the walk that deletes nothing on the line once either escape is seen. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
4595089 to
e7c776e
Compare
|
Claude finished @cursor[bot]'s task in 3m 12s —— View job Code review complete
This is the code-review lane (correctness/maintainability). The critical bypass flagged in earlier review rounds on this PR (a backtick-escaped quote leaving a string's real closer as a stray opener that re-pairs far to the right, e.g. Independently re-traced Each commit since maps cleanly to a specific prior finding:
No new findings. Everything flagged in prior review rounds on this PR has been addressed by a targeted fix rather than a suppression, and I did not find anything new to raise in the current diff. |

Closes #2965
Summary
Close the PowerShell apostrophe-in-double-quoted-string fail-open:
ps::blank_quoted_spansno longer deletes the command between two apostrophe-bearing double-quoted strings, so all three blocking hooks reach the fail-closed sink.ps::blank_quoted_spanspaired quote characters with two independentsedexpressions, neither aware of which quote style opened first. An apostrophe inside a double-quoted string is a literal character to PowerShell, but the single-quote expression treated it as a span delimiter and matched from the apostrophe in one double-quoted string to the apostrophe in the next — deleting everything between them.This is an ENTRY-side failure, not a mis-measurement. With the
(deleted,ps::has_special_constructssaw no construct,ps::has_dynamic_invocationsaw no call andps::has_launchersaw no launcher — sops::classify_git_commandnever entered the fail-closed sink and none of the downstream measuring probes ran at all. Two ordinary apostrophe-bearing strings ("Kyle's build","that's all") were a general bypass of all three blocking hooks.Fix
One left-to-right walk replaces the two expressions: whichever quote character opens first owns everything up to its own next occurrence, so an apostrophe inside a double-quoted span is ordinary text and a quote character inside a single-quoted span is ordinary text.
Ambiguity resolves toward NOT deleting, because this is an entry scan where leaving text in view can only over-block while deleting it is the fail-open above:
Write-Host "oops; & ('g'+'it') push --force→ 2sed)'it''s')Write-Host 'it''s'; & ('g'+'it') push --force→ 2Write-Host "a’b"; & ('g'+'it') push --force; Write-Host "c’d"→ 2Write-Host "a\"; & ('g'+'it') push --force; Write-Host "b"` → 2Why this does not reuse
ps::_skip_double_quoteThe issue suggested reusing it. It honors the backtick escape, which is correct where it is used — the sink BLANKING path, already past the entry decision. Honoring it here extends a span past
"a`"to the next real quote and reopens this very bug in a new spelling. Measured: with a backtick-honoring mutant of this walk,Write-Host "a\"; & ('g'+'it') push --force; Write-Host "b"drops to **rc 0** onblock-dangerous-gitandblock-no-verify. Refusing the escape ends the span at the backticked quote, leaves more text in view, preserves the oldsed's behavior, and lets a surviving backtick still triphas_special_constructs' backtick arm — which it cannot do if the span containing it is deleted.ps::_skip_double_quote/ps::_skip_single_quote` are left untouched.Bypass closure
tool_name: "PowerShell", payloads built withjqas realPreToolUseenvelopes. Before = merge base, after = this branch.& ('g'+'it') push --force(control)Write-Host "a'b"; & ('g'+'it') push --force; Write-Host "c'd"Write-Host "Kyle's build"; & ($tool) push --force; Write-Host "that's all"& ('set-'+'content') f.txt x(control)Write-Host "a'b"; & ('set-'+'content') f.txt x; Write-Host "c'd"Three further straddle spellings found and closed while probing:
& ('set-'+'content')in the natural-prose spelling, the--no-verifystraddle, and a bare-computed writer with-Value(Write-Host "it's"; & $w f.txt -Value x; Write-Host "won't") — each 0 → 2.Over-block rails
Prose-inert rows, all three hooks, 0 before and 0 after — none changed:
Write-Host 'example > out.txt'·git commit -m 'use -Value x'·echo 'run & $($w) f.txt x later'·Write-Host 'example & $(Get-Date) f.txt x'·git commit -m "it's a fix"·Write-Host "Kyle's build"The issue-2848 must-allow six are 0 before and 0 after on all three hooks, and stay 0 when contaminated with the apostrophes this change makes visible:
& $py $script (Join-Path $dir "that's.jsonl")— 0Write-Host "Kyle's build"; & $py $script (Join-Path $dir "$id.jsonl")— 0Start-Process notepad -ArgumentList "Kyle's", (Join-Path $d "that's")— 0Write-Host "Kyle's"; & $py -m unittest discover— 0Negative controls all rc=2 (Bash:
git commit --no-verify -m x,git push --force origin main,cat > somefile.txt; PowerShell:Set-Content -Path out.txt -Value hi,git commit --no-verify -m x,git push --force origin main) — so the zero columns above are not silently no-opping hooks.Callers
All five call sites of
ps::blank_quoted_spanswere probed, not just reasoned about:ps::might_write_via_python3(launcher + computed construct)$/(visible; probedStart-Process notepad -ArgumentList "Kyle's", (Join-Path $d "that's")→ still 0ps::might_write_via_python3(has_special_constructsarm)Write-Host "Kyle's"; & $py -m unittest discover→ still 0ps::classify_git_commandsink triggerps::write_bypasscomputed-target gateps::write_bypasswriter-name scan& ('set-'+'content')straddle 0 → 2;git commit -m 'use -Value x'still 0Tests
25 pinned cases across the three hook test files, each asserting an exact rc. Mutation-checked by reverting the fix: 7 go RED (the straddle rows on all three hooks), the rest are ambiguity-policy and over-block rails that are green both ways by design, plus the backtick pin which goes RED under its own targeted backtick-honoring mutant.
Related
🤖 Generated with Claude Code
https://claude.ai/code/session_018Q4QNEensQgkYm8vByTydJ
Verification
25 pinned cases across the three hook test files, each asserting an exact rc. Mutation-checked by reverting the fix: 7 go RED (the straddle rows on all three hooks). CI
ci-statusis green on this head; remaining babysit work is review-thread resolution.