fix(guardrails): cap Bash substitutions before sourcing the PreToolUse chain (#4684) - #4714
Merged
Merged
Conversation
…any guard runs (#4684) The Bash/PowerShell row's cost grows with the number of command and process substitutions while every per-command cap still holds: 2,339 sibling $(: rm) in 16,378 characters took 65 s through the row on Windows, past the 60 s timeout at which a PreToolUse command hook is cancelled without blocking. run-guards.sh now counts $(, <(, >( and backtick pairs as text before it sources the first guard and exits 2 past --max-substitutions, which the row passes as 256. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Contributor
|
PR body contract — issue linkage This PR body does not yet satisfy the issue-linkage contract:
Edit the body and this comment updates itself on the next run. |
Keep the substitution cap and the command-length short-circuit on the same Bash row. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Re-bump guardrails to 0.38.2 after 0.38.1 landed. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Re-bump guardrails to 0.38.3 after 0.38.2 landed. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
This was referenced Sep 29, 2026
kyle-sexton
added a commit
that referenced
this pull request
Oct 1, 2026
…ion cap (#5689) Closes #4684 ## Summary The substitution cap in `run-guards.sh` (`--max-substitutions 256`) still refused a command whose quoted-delimiter heredoc body held more than 256 substitution spellings, for example a PR body written through `cat <<'EOF'` with many inline-code spans. Bash expands nothing in such a body, so the refusal was a false positive. This is option A of the owner decision of 2026-10-01 on #4684. `block-root-delete-target.sh` is unchanged. It still reads quoted heredoc bodies as commands, so the root-delete judgment is exactly what it was. ## Fix - `run_guards::counted_text` in `plugins/guardrails/hooks/run-guards.sh` drops the body of a heredoc whose delimiter is quoted (`<<'EOF'`, `<<"EOF"`, `<<\EOF`, `<<E'O'F`, `<<-'EOF'` with tab-indented terminator) before the recount. The operator, the delimiter word and the rest of its line stay in the counted text. - The recount now also runs when the command holds `<<`, not only a single quote. - The body is dropped only where bash and `hook::bash_parse_segments` end it on the same line. Everything else counts whole: an unquoted or expanding heredoc, an empty or non-`[A-Za-z0-9_]` delimiter, a quote, `\`, `$`, `#`, `<`, `(` or `)` on the delimiter line, a second heredoc, an enclosing `$(`, `<(`, `>(`, backtick or arithmetic, no terminator line, a single-quoted span after a dropped body, and any command that names a shell, `eval`, `su`, `env`, `source`, `. file` or `alias` (`bash <<'EOF'` and `source /dev/stdin <<'EOF'` count whole). - Header comment and README updated. Security review of the branch diff (focused on the cap change): - Substitutions bash will expand cannot escape the count. Bash expands a heredoc body only when no part of the delimiter is quoted, and the discount requires a quoted part. Checked with 37 spellings against real `run-guards.sh`: unquoted, `<<-` unquoted, empty `''`, delimiter with a space, `$'EOF'`, trailing-space or missing terminator, two heredocs on a line, heredoc inside `$(`, backticks, `<(`, double quotes and arithmetic all still count (exit 2); only the quoted-delimiter forms and pre-existing single-quoted spans are discounted. - Root-delete coverage is unchanged: the file is not in the diff, and `bash <<'EOF'` with a `$(rm -rf /)` body is refused by it (test below). The guards still read every byte; the cap only counts. - Guards and the cap read one text (carriage returns are stripped before both), and the guard library ends a heredoc at the same line as the cap, so the cap cannot drop text a guard would treat as commands. - Worst-case cost with the cap no longer protecting a quoted body, full 10-guard row, 16,384-char quoted heredoc, WSL2: `$(: rm)`, backtick pairs, `<(: rm)`, `"$(: rm)"`, `$(rm -rf /tmp/x)` and 390 small heredocs each finished in 0.1 to 3.0 s, none near the 60 s hook timeout. For a no-work body such as `$(: rm)`, root-delete's 25 s deadline never starts; `--max-command-len` (16,384 characters, about 1.1 ms per spelling, 2.6 to 3.7 s measured) is what bounds the cost, and the `run-guards.sh` comment and README now say so. - Found, not changed: `bash <<'EOF'` with a plain `rm -rf /` body (no substitution) is allowed by every guard on `origin/main` too, because root-delete reads only substitutions in a heredoc body. Pre-existing and outside this change. ## Verification - `plugins/guardrails/hooks/run-guards.test.sh`: PASS=641 FAIL=0. With the heredoc arm of `counted_text` reverted, 25 cases fail (every exit-0 quoted-heredoc case); with the `source` and `. file` arm reverted, the sourcing cases fail; restored. - `plugins/guardrails/hooks/abort-boundary.test.sh`: PASS=260 FAIL=0. Every other `plugins/guardrails/hooks/*.test.sh`, `shellcheck`, `typos` and `check-shell-portability` are clean. - New and updated cases cover: quote forms and `<<-` tab terminator no longer counted; `gh --body-file -` form; unquoted, expanding, after-terminator, missing-terminator and two-heredoc commands still exit 2; `bash <<'EOF'` with a `$(rm -rf /)` body refused through the full row; the `$(: x)` spelling. - Full-row timing (owner condition), WSL2 Linux, `cat <<'EOF'` + 2,338 x `$(: rm)` + `EOF` (16,382 chars), median of 5, recorded on the issue (comment 5932518425): 2,663 ms idle and 3,260 ms with 8 busy loops, rc 0, against 1,441 ms and 1,739 ms for the same text without `$`. About 3.3 s worst case, far inside 60 s, so option B is not needed. Windows rate (about 1.1 ms per spelling, about 2.6 s at the ceiling) is in comment 5922576224. - #4520 headroom, Windows Git Bash (melo-desk-001, GNU bash 5.3.15, 24 logical CPUs), measured on the shipped guard code (guardrails 0.43.2) with the `block-root-delete-target` input at the cap, `echo ` + 256 x `$(: rm)`: the guard used 656-676 ms idle and 630-644 ms under 8 busy loops, against its 25 s `RDT_DEADLINE` (about 37x headroom). The full 10-guard row took 2.3-2.5 s of the 60 s hook timeout. Source: issue comment 5922576224. - Over-cap payloads on Windows, same host and comment: `echo ` + 2339 x `$(: rm)` (p1) and 2330 x `$(: rm)` + `; rm -rf /` (p2) both exit 2 at the substitution cap through `run-guards.sh` before any guard is sourced. p1 took 81 ms idle and 74 ms busy; p2 took 82 ms idle and 77 ms busy. This change does not touch that path: the recount runs only on a command holding a quote or `<<`. - Version 0.46.2 and a CHANGELOG entry, because `ci-status` runs `check-changelog-parity.sh --check-bump` and fails a guardrails change without them. ## Related - #5543 shipped the single-quoted half of (c). - #4714 put the cap before the guard chain. - #4520 root-delete deadline headroom: answered under Verification. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4684
Summary
Bash/PowerShell PreToolUse refuses (exit 2) commands with more than 256 command/process substitutions before any guard is sourced.
run-guards.sh --max-substitutions 256(text-only count). Timed-out hooks fail open (hooks timeouts).guardrails→ 0.37.4 (serialize above #4699 0.37.3; series 4684→4682→4685→4678).Test plan