diff --git a/plugins/guardrails/.claude-plugin/plugin.json b/plugins/guardrails/.claude-plugin/plugin.json index b7d00beb4a..28ea8c4e86 100644 --- a/plugins/guardrails/.claude-plugin/plugin.json +++ b/plugins/guardrails/.claude-plugin/plugin.json @@ -147,5 +147,5 @@ "min": 1 } }, - "version": "0.29.3" + "version": "0.29.4" } diff --git a/plugins/guardrails/CHANGELOG.md b/plugins/guardrails/CHANGELOG.md index 90da821d70..b140d0ffd5 100644 --- a/plugins/guardrails/CHANGELOG.md +++ b/plugins/guardrails/CHANGELOG.md @@ -3,6 +3,62 @@ All notable changes to the `guardrails` plugin are documented here. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this plugin uses semantic versioning. +## [0.29.4] + +### Fixed + +- **PowerShell guards: an apostrophe inside a double-quoted string no longer + deletes the command between two such strings + ([#2965](https://github.com/melodic-software/claude-code-plugins/issues/2965)).** + `Write-Host "a'b"; & ('g'+'it') push --force; Write-Host "c'd"` exited 0 from + `block-dangerous-git` and `block-no-verify`, while the bare + `& ('g'+'it') push --force` exited 2. The natural-prose spelling — + `Write-Host "Kyle's build"; & ($tool) push --force; Write-Host "that's all"` — + did the same, and `& ('set-'+'content') f.txt x` flanked the same way exited 0 + from `block-hook-bypass`. Long-standing shipped behavior, not a 0.28.x/0.29.x + regression. + + `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. The whole command above reduced to the single token + `Write-Host`. This is an ENTRY-side failure, not a measurement error: with the + `(` deleted, `ps::has_special_constructs` saw no construct, + `ps::has_dynamic_invocation` saw no call and `ps::has_launcher` saw no + launcher, so `ps::classify_git_command` never entered the fail-closed sink and + none of the downstream probes ran at all. + + The two expressions are replaced by one LEFT-TO-RIGHT walk in which 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. The reversed spelling + (`'a"b'; & ('g'+'it') push --force; 'c"d'`) closes by the same walk. + + 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. An unterminated opener emits the rest of its line verbatim; a span never + crosses a newline; and smart quotes are not treated as delimiters. + + Two escape spellings make pairing itself ambiguous, so the walk refuses the + question and emits the rest of the line verbatim rather than picking a closer. + PowerShell's doubled-quote escape (`'it''s'`, `"say ""hi"""`) is one: a doubled + candidate closer is treated as ambiguous, not naively paired. A backtick inside + a would-be double-quoted span is the other, and it has two failure modes — + honoring the escape (`ps::_skip_double_quote`) extends a span past `` "a`" `` + to the next real quote and reopens this same bypass; refusing it ends the span + at the backticked quote and leaves the string's real closer as a stray opener + that re-pairs far to the right. Review measured + ``"a`""; & ('g'+'it') push --force; 'b"c'`` at 0 on all three hooks before the + walk started deleting nothing once a backtick is seen. A lone empty string + (`""`, `''`) is not doubled and still blanks normally. + + No behavior change to the [#2848](https://github.com/melodic-software/claude-code-plugins/issues/2848) + must-allow shapes: all six stay 0 on all three blocking hooks, including when + contaminated with apostrophes (`& $py $script (Join-Path $dir "that's.jsonl")`). + Prose that merely mentions a write or a git command stays inert. + ## [0.29.3] ### Fixed diff --git a/plugins/guardrails/hooks/block-dangerous-git.test.sh b/plugins/guardrails/hooks/block-dangerous-git.test.sh index 929bbc89d3..b6f76373a1 100755 --- a/plugins/guardrails/hooks/block-dangerous-git.test.sh +++ b/plugins/guardrails/hooks/block-dangerous-git.test.sh @@ -1107,4 +1107,74 @@ assert_contains "NUL msg: all-NUL command refused by the flag, not skipped" \ "$(nul_stderr '' '')" "NUL byte" run "empty command, no NUL (allowed)" "" 0 +# --- #2965: an apostrophe in a DOUBLE-quoted string is not a span delimiter ----- +# ps::blank_quoted_spans used to pair quotes with two independent `sed` +# expressions, neither aware of which style opened first. The single-quote +# expression matched from the apostrophe inside one double-quoted string to the +# apostrophe inside the next and DELETED everything between them: +# +# in: Write-Host "a'b"; & ('g'+'it') push --force; Write-Host "c'd" +# out: Write-Host +# +# With the `(` gone, has_special_constructs saw no construct and the fail-closed +# sink was never ENTERED — so every downstream measuring probe was moot. Each +# command below blocks on its own; adding two ordinary apostrophe-bearing strings +# is what made it vanish. The controls are load-bearing: without them an +# all-blocked column is equally consistent with a guard that refuses every +# command containing an apostrophe. +# shellcheck disable=SC2016 +run_pwsh "PS: computed git push --force (control, blocked)" \ + "& ('g'+'it') push --force" 2 +# shellcheck disable=SC2016 +run_pwsh "PS: same call straddled by apostrophe-bearing strings (blocked — #2965)" \ + "Write-Host \"a'b\"; & ('g'+'it') push --force; Write-Host \"c'd\"" 2 +# shellcheck disable=SC2016 +run_pwsh "PS: natural-prose spelling, variable target (blocked — #2965)" \ + "Write-Host \"Kyle's build\"; & (\$tool) push --force; Write-Host \"that's all\"" 2 +# The REVERSED pairing — a double quote inside SINGLE-quoted strings — is the same +# defect with the roles swapped, and must stay closed by the same walk. +# shellcheck disable=SC2016 +run_pwsh "PS: reversed straddle, quotes inside single-quoted strings (blocked — #2965)" \ + "Write-Host 'a\"b'; & ('g'+'it') push --force; Write-Host 'c\"d'" 2 +# AMBIGUITY RESOLVES TOWARD NOT DELETING. An UNTERMINATED opener must not swallow +# the rest of the line, a BACKTICK-escaped quote must not extend the span to the +# next real one (honoring the escape here is what would reopen this bug in a new +# spelling), and PowerShell's DOUBLED-quote escape is deliberately over-blocked. +# shellcheck disable=SC2016 +run_pwsh "PS: unterminated opener does not swallow the command (blocked — #2965)" \ + "Write-Host \"oops; & ('g'+'it') push --force" 2 +# shellcheck disable=SC2016 +run_pwsh "PS: backtick-escaped quote does not extend the span (blocked — #2965)" \ + "Write-Host \"a\`\"; & ('g'+'it') push --force; Write-Host \"b\"" 2 +# shellcheck disable=SC2016 +run_pwsh "PS: doubled-quote escape over-blocks rather than deletes (blocked — #2965)" \ + "Write-Host 'it''s'; & ('g'+'it') push --force" 2 +# The escape cases have a SECOND failure mode that the first cut of this fix +# shipped and review caught: ending a span AT the backticked quote leaves the +# string's REAL closer behind as a stray opener, which then pairs with a quote +# far to the right and deletes the command anyway. Both escapes therefore delete +# NOTHING on their line. Pinned on the exact reviewed spelling. +# shellcheck disable=SC2016 +run_pwsh "PS: escaped quote's real closer must not re-pair rightward (blocked — #2965)" \ + "\"a\`\"\"; & ('g'+'it') push --force; 'b\"c'" 2 +# A lone EMPTY string is not a doubled quote — its closer is followed by +# something other than the same quote — so it must still blank normally rather +# than fall into the delete-nothing branch and start over-blocking. +# shellcheck disable=SC2016 +run_pwsh "PS: empty string still blanks normally (allowed — #2965)" \ + "Write-Host \"\"; & \$py script.py" 0 +# Over-block rails. Message text must stay inert, and the #2848 must-allow shapes +# must survive an apostrophe appearing beside them — more text is now VISIBLE to +# every probe, so this is exactly where a new over-block would surface. +run_pwsh "PS: apostrophe in a commit message stays inert (allowed — #2965)" \ + "git commit -m \"it's a fix\"" 0 +run_pwsh "PS: two apostrophe-bearing strings, no command between (allowed — #2965)" \ + "Write-Host \"Kyle's build\"; Write-Host \"that's all\"" 0 +# shellcheck disable=SC2016 +run_pwsh "PS: #2848 bare-computed call target flanked by an apostrophe (allowed — #2965)" \ + "Write-Host \"Kyle's build\"; & \$py \$script (Join-Path \$dir \"\$id.jsonl\")" 0 +# shellcheck disable=SC2016 +run_pwsh "PS: #2848 apostrophe inside the grouped operand itself (allowed — #2965)" \ + "& \$py \$script (Join-Path \$dir \"that's.jsonl\")" 0 + report diff --git a/plugins/guardrails/hooks/block-hook-bypass.test.sh b/plugins/guardrails/hooks/block-hook-bypass.test.sh index 664e5978b0..7c16569fc9 100755 --- a/plugins/guardrails/hooks/block-hook-bypass.test.sh +++ b/plugins/guardrails/hooks/block-hook-bypass.test.sh @@ -1745,4 +1745,57 @@ nul_rc=0 bash "$HOOK" <<<"$(jq -n '{tool_name:"Bash",tool_input:{command:("git status" + ([0]|implode))}}')" >/dev/null 2>&1 || nul_rc=$? assert_exit "NUL in command (blocked)" 2 "$nul_rc" +# --- #2965: an apostrophe in a DOUBLE-quoted string is not a span delimiter ----- +# ps::blank_quoted_spans used to pair quotes with two independent `sed` +# expressions, neither aware of which style opened first. The single-quote +# expression matched from the apostrophe inside one double-quoted string to the +# apostrophe inside the next and DELETED everything between them, so a computed +# writer call flanked by two ordinary strings was ALLOWED even though the same +# call blocks on its own. The controls are load-bearing: without them an +# all-blocked column is equally consistent with a guard that refuses every +# command containing an apostrophe. +# shellcheck disable=SC2016 +run_pwsh "PS: computed Set-Content call (control, blocked)" \ + "& ('set-'+'content') f.txt x" 2 +# shellcheck disable=SC2016 +run_pwsh "PS: same call straddled by apostrophe-bearing strings (blocked — #2965)" \ + "Write-Host \"a'b\"; & ('set-'+'content') f.txt x; Write-Host \"c'd\"" 2 +# shellcheck disable=SC2016 +run_pwsh "PS: natural-prose spelling of the straddle (blocked — #2965)" \ + "Write-Host \"Kyle's build\"; & ('set-'+'content') f.txt x; Write-Host \"that's all\"" 2 +# A bare-computed writer target with a -Value write signal is the other shape the +# deletion hid — it reaches write_bypass through a different gate than the +# subexpression target above, so it is pinned separately. +# shellcheck disable=SC2016 +run_pwsh "PS: bare-computed writer with -Value, straddled (blocked — #2965)" \ + "Write-Host \"it's\"; & \$w f.txt -Value x; Write-Host \"won't\"" 2 +# The escape cases have a SECOND failure mode that the first cut of this fix +# shipped and review caught: ending a span AT the backticked quote leaves the +# string's REAL closer behind as a stray opener, which then pairs with a quote +# far to the right and deletes the writer call anyway. Both the backtick and the +# doubled-quote escape therefore delete NOTHING on their line. This spelling +# reaches write_bypass through `lcq_bt` — the backtick-intact copy built before +# backticks are stripped from `lcq` — so `ps::blank_quoted_spans` sees the +# backtick and the backtick-ambiguity branch emits the line verbatim. The +# doubled-quote arm is not what catches this pinned case. +# shellcheck disable=SC2016 +run_pwsh "PS: escaped quote's real closer must not re-pair rightward (blocked — #2965)" \ + "\"a\`\"\"; & ('set-'+'content') f.txt x; 'b\"c'" 2 +# A lone EMPTY string is not a doubled quote, so it must still blank normally +# rather than fall into the delete-nothing branch and start over-blocking. +# shellcheck disable=SC2016 +run_pwsh "PS: empty string still blanks normally (allowed — #2965)" \ + "& \$py \$script \"\"" 0 +# Over-block rails. Message text naming a write must stay inert — quote blanking +# is what makes it inert, and this change makes MORE text visible to every probe. +run_pwsh "PS: redirect inside message text stays inert (allowed — #2965)" \ + "Write-Host 'example > out.txt'" 0 +run_pwsh "PS: -Value inside a commit message stays inert (allowed — #2965)" \ + "git commit -m 'use -Value x'" 0 +run_pwsh "PS: two apostrophe-bearing strings, no command between (allowed — #2965)" \ + "Write-Host \"Kyle's build\"; Write-Host \"that's all\"" 0 +# shellcheck disable=SC2016 +run_pwsh "PS: #2848 bare-computed call target flanked by an apostrophe (allowed — #2965)" \ + "Write-Host \"Kyle's build\"; & \$py \$script (Join-Path \$dir \"\$id.jsonl\")" 0 + report diff --git a/plugins/guardrails/hooks/block-no-verify.test.sh b/plugins/guardrails/hooks/block-no-verify.test.sh index a0e7b968f8..a94b2ecb25 100755 --- a/plugins/guardrails/hooks/block-no-verify.test.sh +++ b/plugins/guardrails/hooks/block-no-verify.test.sh @@ -579,4 +579,33 @@ assert_contains "NUL msg: all-NUL command refused by the flag, not skipped" \ "$(nul_stderr '' '')" "NUL byte" run "empty command, no NUL (allowed)" "" 0 +# --- #2965: an apostrophe in a DOUBLE-quoted string is not a span delimiter ----- +# ps::blank_quoted_spans used to pair quotes with two independent `sed` +# expressions, neither aware of which style opened first. The single-quote +# expression matched from the apostrophe inside one double-quoted string to the +# apostrophe inside the next and DELETED everything between them — including the +# command. With the `(` gone the sink was never entered, so this blocked on its +# own and was ALLOWED once flanked by two ordinary strings. +# +# The controls are load-bearing: without them an all-blocked column is equally +# consistent with a guard that refuses everything containing an apostrophe. +# shellcheck disable=SC2016 +run_pwsh "PS: computed git commit --no-verify (control, blocked)" \ + "& ('g'+'it') commit --no-verify -m x" 2 +# shellcheck disable=SC2016 +run_pwsh "PS: same call straddled by apostrophe-bearing strings (blocked — #2965)" \ + "Write-Host \"a'b\"; & ('g'+'it') commit --no-verify -m x; Write-Host \"c'd\"" 2 +# shellcheck disable=SC2016 +run_pwsh "PS: natural-prose spelling of the straddle (blocked — #2965)" \ + "Write-Host \"Kyle's build\"; & ('g'+'it') commit --no-verify -m x; Write-Host \"that's all\"" 2 +# Over-block rails. An apostrophe inside message text must stay inert, and the +# #2848 must-allow shape must survive an apostrophe appearing next to it. +run_pwsh "PS: apostrophe in a commit message stays inert (allowed — #2965)" \ + "git commit -m \"it's a fix\"" 0 +run_pwsh "PS: two apostrophe-bearing strings, no command between (allowed — #2965)" \ + "Write-Host \"Kyle's build\"; Write-Host \"that's all\"" 0 +# shellcheck disable=SC2016 +run_pwsh "PS: #2848 bare-computed call target flanked by an apostrophe (allowed — #2965)" \ + "Write-Host \"Kyle's build\"; & \$py \$script (Join-Path \$dir \"\$id.jsonl\")" 0 + report diff --git a/plugins/guardrails/lib/powershell/ps-command.sh b/plugins/guardrails/lib/powershell/ps-command.sh index 0992ecb9fb..3dadafbe2f 100644 --- a/plugins/guardrails/lib/powershell/ps-command.sh +++ b/plugins/guardrails/lib/powershell/ps-command.sh @@ -240,15 +240,111 @@ ps::blank_herestrings() { # Crude, SCAN-ONLY strip of single- and double-quoted spans, so that structural # detection and commit/push shaping ignore characters inside message text. Never -# fed to a parser. Backtick-escaped quotes are not honored — a backtick anywhere -# already forces the fail-closed branch, so this crudeness cannot open a gap. +# fed to a parser. +# +# LEFT TO RIGHT, FIRST OPENER OWNS ITS SPAN. This used to be a `sed` with two +# independent expressions — `s/'[^']*'//g` then `s/"[^"]*"//g` — neither of which +# had any notion 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, so the span it deleted ran from the +# apostrophe in one double-quoted string to the apostrophe in the next — taking +# everything between them with it: +# +# in: Write-Host "a'b"; & ('g'+'it') push --force; Write-Host "c'd" +# out: Write-Host +# +# That is not a measurement error, it is the ERASURE of the command every +# sink-trigger scan is about to look at. With the `(` gone, has_special_constructs +# saw no construct, has_dynamic_invocation saw no call, has_launcher saw no +# launcher — so classify_git_command never entered the fail-closed sink at all, +# and the same deletion hid a computed writer call from write_bypass. Two +# ordinary strings that happen to contain apostrophes ("Kyle's build") were a +# general bypass of all three blocking hooks (#2965). +# +# A single walk fixes the pairing: 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. This is an ENTRY scan: leaving text in +# view can only cause an over-block, while deleting it is the fail-open above. So +# - an UNTERMINATED opener emits the rest of its line verbatim rather than +# swallowing it; +# - a span never crosses a NEWLINE (matching the old per-line `sed` semantics), +# because a multi-line double-quoted string would otherwise let +# `"a\n& ('g'+'it') push --force\n"` blank the middle line; +# - PowerShell's DOUBLED-quote escape (`'it''s'`, `"say ""hi"""`) makes a +# candidate closer ambiguous, so it too resolves to deleting nothing on the +# line. A lone empty string (`""`, `''`) is NOT doubled — its closer is +# followed by something other than the same quote — so it still blanks +# normally and `& $py $script ""` is unaffected. +# - SMART quotes (U+2018/U+2019/U+201C/U+201D), which PowerShell's tokenizer +# does accept as delimiters, are NOT treated as delimiters. Not deleting +# leaves their contents in view: over-block, not bypass. +# +# A BACKTICK inside a would-be DOUBLE-quoted span makes the pairing ambiguous, +# and ambiguity here means DELETE NOTHING ON THIS LINE. Neither available answer +# is safe on its own, which is why the resolution is to refuse the question: +# +# - HONORING the escape (what `ps::_skip_double_quote` does, correctly, in the +# sink BLANKING path that runs after the entry decision) extends the span +# past `` `" `` to the next real quote, so +# `Write-Host "a`"; & ('g'+'it') push --force; Write-Host "b"` becomes one +# span and blanks the git command — this issue's own bug in a new spelling. +# - NOT honoring it ends the span at the backticked quote, which leaves the +# string's REAL closer behind as a stray opener that pairs with a quote far +# to the right. ``"a`""; & ('g'+'it') push --force; 'b"c'`` then reduces to +# `c'` and all three hooks returned 0 (caught in review of this change). +# +# Emitting the rest of the line verbatim costs nothing that is not already paid: +# a surviving backtick trips has_special_constructs' backtick arm and routes to +# the fail-closed sink anyway. It cannot do that if the span containing it is +# deleted, which is the deeper reason this branch exists. SINGLE-quoted spans are +# exempt: PowerShell gives them no escape at all, so a backtick inside one is an +# ordinary character and the pairing is genuinely unambiguous. ps::blank_quoted_spans() { - local text="$1" - # One `sed` for both spans: expressions apply in order per line, so the - # double-quote strip still sees the single-quote-stripped text — same result - # as the two chained passes, at half the process cost on a hot classifier path. - text=$(printf '%s' "$text" | sed -e "s/'[^']*'//g" -e 's/"[^"]*"//g') - printf '%s' "$text" + local text="$1" out="" i=0 n j q found c + n=${#text} + while ((i < n)); do + q="${text:i:1}" + if [[ "$q" == "'" || "$q" == '"' ]]; then + found=0 + for ((j = i + 1; j < n; j++)); do + c="${text:j:1}" + [[ "$c" == $'\n' ]] && break + # Ambiguous escape context in a double-quoted span — stop looking and + # fall through to the delete-nothing branch below. + if [[ "$q" == '"' && "$c" == '`' ]]; then + for ((; j < n; j++)); do [[ "${text:j:1}" == $'\n' ]] && break; done + break + fi + if [[ "$c" == "$q" ]]; then + # A DOUBLED quote is PowerShell's other escape for a delimiter + # (`'it''s'`, `"say ""hi"""`), so this candidate closer may not be one. + # Same resolution as the backtick: refuse the question, delete nothing. + if [[ "${text:j+1:1}" == "$q" ]]; then + for ((; j < n; j++)); do [[ "${text:j:1}" == $'\n' ]] && break; done + break + fi + found=1 + break + fi + done + if ((found)); then + i=$((j + 1)) + continue + fi + # Unterminated (or escape-ambiguous) on this line: extent is ambiguous, so + # delete nothing. `j` already sits on the newline (or at the end), so copy + # the rest verbatim in one slice — this also keeps the walk linear. + out+="${text:i:j-i}" + i=$j + continue + fi + out+="$q" + i=$((i + 1)) + done + printf '%s' "$out" } # Fold a BACKTICK-ESCAPED closing brace to `_`, left to right, BEFORE any caller @@ -1497,7 +1593,7 @@ ps::print_unparsable_git_block_message() { # CONTENT scanning of PowerShell writes stays on the Write|Edit-matched guards; # scanning PowerShell write content is deferred to A2b. ps::write_bypass() { - local cmd="$1" scan lcs seg lc head lcq q="\"'" blanked_gate + local cmd="$1" scan lcs seg lc head lcq lcq_bt q="\"'" blanked_gate ps::blank_herestrings "$cmd" # A call `&` / dot-source `.` of a QUOTED writer name runs that string as the @@ -1512,6 +1608,16 @@ ps::write_bypass() { # the load-bearing position: the probes cannot do it themselves, because by the # time they are called the backticks are already gone. lcq=$(ps::fold_escaped_brace_closers "$PS_BLANKED") + # Keep a backtick-INTACT copy for the quote-blanking below, for the same reason + # the brace fold has to run before the deletion: a backtick-escaped QUOTE is an + # escape context that the deletion destroys. `"say `"hi"` is one string, but + # once the backtick is gone it reads as `"say "` + `hi` + a dangling `"` whose + # pairing runs forward to the next literal quote anywhere on the line — which + # swallowed `& ('set-'+'content') f.txt x` and returned 0 (review of #2965). + # ps::blank_quoted_spans can only resolve that toward NOT deleting while the + # backtick still exists, so it must see this copy; the result is stripped + # afterwards, which still recovers an obfuscated `Set``-Content` name. + lcq_bt="${lcq,,}" lcq="${lcq//\`/}" lcq="${lcq,,}" if [[ "$lcq" =~ (^|[[:space:]\;\{\}\(\|\&=])[.\&][[:space:]]*[$q]([a-z.]+\\)?(set-content|add-content|out-file|tee-object|ac|tee|iex|invoke-expression|new-item|ni|epcsv|export-[a-z]+) ]]; then @@ -1528,7 +1634,8 @@ ps::write_bypass() { # Both this and the quoted-writer check above accept a statement/block separator # boundary (`;& …`), not only whitespace (review round 6). if ps::call_target_is_bare_computed "$lcq"; then - blanked_gate=$(ps::blank_quoted_spans "$lcq") + blanked_gate=$(ps::blank_quoted_spans "$lcq_bt") + blanked_gate="${blanked_gate//\`/}" # fd-dup merges (`2>&1`) are plumbing, not file writes — strip them before # ANY probe in this branch runs, so `& $tool 2>&1` does not look like a # producer redirect AND the `&` of the merge is not read as a statement