diff --git a/plugins/guardrails/.claude-plugin/plugin.json b/plugins/guardrails/.claude-plugin/plugin.json index d5596a6e95..ea24d4ad3b 100644 --- a/plugins/guardrails/.claude-plugin/plugin.json +++ b/plugins/guardrails/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "$schema": "https://json.schemastore.org/claude-code-plugin-manifest.json", "name": "guardrails", - "version": "0.22.0", + "version": "0.22.1", "description": "Twelve safety guards that block secret/credential writes, hardcoded machine-specific paths, git hook-bypass attempts, irreversible git operations (force-push, reset --hard, worktree-wide checkout/restore discards), Bash file-write workarounds that circumvent Write/Edit hooks, multi-line `git commit -m` messages (an actual-newline `-m` mangles across shells; single-line `-m` passes), commit subjects and gh pr create titles that violate the repo's tracked team convention (when one is declared in .claude/source-control.md), (advisory) hallucinated CLI flags, (advisory) /plugin:skill references that do not resolve, (advisory) markdown citing a repo path the repo's own history shows was removed, (advisory, opt-in) un-throttled Workflow fan-out that risks burst 529s, and (advisory, opt-in) direct gh pr create calls bypassing this marketplace's own pull-request skill — each independently toggleable.", "author": { "name": "Melodic Software", diff --git a/plugins/guardrails/CHANGELOG.md b/plugins/guardrails/CHANGELOG.md index 189a36a134..6fd07fe04b 100644 --- a/plugins/guardrails/CHANGELOG.md +++ b/plugins/guardrails/CHANGELOG.md @@ -3,6 +3,73 @@ 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.22.1] + +### Fixed + +- **`secret-pattern-detection` and `hardcoded-path-check` — both BLOCKING PreToolUse guards — + produced NO VERDICT AT ALL for a payload of 65536-65663 bytes.** Not slow: deadlocked. Bash + delivers a here-string by filling a pipe ITSELF, before the reader is exec'd, and it appends a + newline — so a payload in that band puts the write 1-128 bytes past the 65536-byte pipe capacity + and blocks forever (at >=129 bytes over, bash spills to a temp file and it works again, which is + why 65535 and 65664 always passed and only the band between them hung). Measured on Git Bash + against the pre-fix hooks: a 65536-byte Write carrying a live-shape AWS access-key id returned + nothing at a 200-second bound, where the same token in a small payload exits 2 immediately. Both + hooks are registered at `timeout: 60`, so the harness cancels the guard and the verdict is lost — + a fail-open reachable by any agent that controls the size of what it writes. Every whole-payload + `<<<` in the plugin now feeds its reader through process substitution instead: the two pre-filter + gates in `lib/path-detection/hardcoded-path-patterns.sh`, the fast-reject and per-pattern + itemization in `secret-pattern-detection.sh`, and the telemetry-label grep in + `hardcoded-path-check.sh` — the last of which is payload-sized too, because `$VIOLATIONS` embeds + each MATCHED LINE verbatim and the lib's `head -3` bounds the line count, not the byte count, so + one 65KB minified line carrying a hardcoded path deadlocked on the blocked path after the stderr + message but before `exit 2`. Same class as #1587, which fixed `hook-utils.sh`'s JSON path and + stopped there. + + `printf … | grep -q` is NOT the alternative, and the comment that previously justified the + here-string was half right about why: `grep -q` exits at the first match and SIGPIPEs `printf`, so + under the `set -uo pipefail` these hooks run with, the pipeline reports printf's 141 — and + `if ! grep -q …` reads any non-zero status as "no match" and early-returns clean, inverting a + real detection into a fail-open. Process substitution keeps the writer OUT of the pipeline, so + `pipefail` can never see its SIGPIPE, while preserving the early exit the gate exists for. + Verified empirically at every boundary size under `set -o pipefail`, in both the match and + no-match directions. This also resolves a contradiction inside the plugin: the pattern lib told + readers to PREFER a here-string over `printf | grep`, while `hook-utils.sh` told them a whole + payload must never go through `<<<` because it blocks at the pipe capacity. The lib now states the + same rule as `hook-utils.sh` and cites it — a pipe when the reader drains its input (`jq`), process + substitution when the reader may exit early (`grep -q`). `hook-utils.sh` itself is left byte-identical + to `main`: its guidance was already correct, and the sync gate would require a version bump plus a + changelog entry for all fourteen other plugins that carry the shared lib in exchange for a + comment-only edit. + +- **The same deadlock in six command-scanning guards.** `block-convention-violation`, + `block-hook-bypass`, `flag-commit-pr-skill-bypass`, and the shared PowerShell command lib fed the + whole Bash/PowerShell command — or segments derived from it — through `while … done <<<"$cmd"`, + which deadlocks identically at 65536-65663 bytes. `workflow-resilience-check` did the same with an + inline Workflow `script:`. All now use `< <(printf '%s\n' …)`, which is byte-identical to the + here-string it replaces (`<<<` appends a newline unconditionally) and so cannot drop a final line. + +### Changed + +- Boundary regression cases at 65535 / 65536 / 65600 / 65663 / 65664 bytes in both + `secret-pattern-detection.test.sh` and `hardcoded-path-check.test.sh`, including payloads where a + real detectable secret / hardcoded path sits INSIDE the hang window and must still exit 2. Neither + suite previously had a single payload-size case. Every case is bounded by `timeout` and asserts + the EXACT expected code, with 124 reported as its own loud failure — a "non-zero means blocked" + assertion would have accepted the hang and would not have caught this defect. The payload is piped, + never fed to the hook with `<<<`, which would hang the test itself at exactly these sizes. + +- README hook table: the six guards registered under the `Bash|PowerShell` matcher were all listed + as `PreToolUse · Bash`; no row named PowerShell at all. + +### Note on the version bump + +Patch, deliberately. Payloads in the 65536-65663 band that previously slipped through on a cancelled +hook are now blocked, but nothing LEGITIMATE becomes refused that these guards did not already intend +to refuse — the fix restores the documented contract rather than widening it. (The 0.21.0 minor was +called out for an *acceptance* change that could refuse previously-allowed legitimate work; this is +not that.) + ## [0.22.0] ### Removed diff --git a/plugins/guardrails/README.md b/plugins/guardrails/README.md index e612fb277c..b03c5615ce 100644 --- a/plugins/guardrails/README.md +++ b/plugins/guardrails/README.md @@ -10,14 +10,14 @@ Each guard is independently toggleable, so you run exactly the subset you want. |-------|-----------------|----------|-----------------| | **secret-pattern-detection** | PreToolUse · Write \| Edit \| NotebookEdit | **Blocks** (exit 2) | High-confidence secret/credential patterns (AWS/GitHub/GitLab/Slack/Stripe/OpenAI keys, PEM private keys) in new file content. | | **hardcoded-path-check** | PreToolUse · Write \| Edit \| NotebookEdit | **Blocks** (exit 2) | Hardcoded machine-specific paths — Windows drive-letter homes, macOS/Linux user homes, machine-specific repo checkout roots. | -| **block-no-verify** | PreToolUse · Bash | **Blocks** (exit 2) | Git hook-bypass attempts on `git commit` / `git push`: `--no-verify` / `-n`, `core.hooksPath=` assignment, and hook-manager disable env vars — a configurable prefix set defaulting to `lefthook`, `husky`, `pre_commit`, `simple_git_hooks` (e.g. `LEFTHOOK=0`, `HUSKY=0`, `PRE_COMMIT_*=false`), tunable via `block_no_verify_hook_manager_prefixes`, including inside compound `cd … && …` commands. | -| **block-dangerous-git** | PreToolUse · Bash | **Blocks** (exit 2) | Irreversible git operations: `push --force`/`-f` plus the equivalent leading-`+` refspec and `--mirror` forms, and the unsafe `--force-with-lease` spellings, in the two kinds git itself treats differently. **No expected value** (bare `--force-with-lease` or `=`) leases against the remote-tracking ref, which git documents as "trivially defeated" by a background fetch — blocked unless `--force-if-includes` is present, which git documents as the mitigation for exactly this form. **A movable `=:`** — `origin/main`, `HEAD`, a tag, an *abbreviated* object id, or hex of the wrong width for this repository's hash format, all of which git resolves at push time, and gitrevisions resolves a short hex word as a ref before trying it as an object-id prefix — is blocked unconditionally, because git declares `--force-if-includes` a no-op alongside an explicit `:`. A lease passes only when `` is immutable: a **literal** object id of the pushed repository's own hash width (detection never evaluates substitutions, so resolve it with `git rev-parse` as a separate step and pass the result) (40 hex under SHA-1, 64 under SHA-256, read from `git rev-parse --show-object-format` with the command's own `-C`/`--git-dir`/`--work-tree`/`--namespace` replayed onto it; undeterminable fails closed) or the empty string asserting the ref must not exist. The other width is a ref name there, not an object id — git ignores a ref whose name is full-width hex for its own format, but resolves one of the other width like any name. git scopes a pin to its own ref, so a bare fallback alongside a pinned entry still governs every other ref being updated; where the same ref carries several lease entries, git consults the first, and so does this guard. A trailing `--no-force-with-lease` cancels every previous lease, and a push dry-run disarms the check. Also blocked: `reset --hard`, `clean` with a force flag (any dry-run flag disarms), worktree-wide `checkout`/`restore` pathspecs (`.`, `:/`, `:(top…)` — path-scoped forms and `restore --staged .` pass), and forced `checkout -f` / `switch --discard-changes`. Accepted unique-prefix abbreviations of the blocked long options match too. `branch -D` is deliberately not blocked (reflog-recoverable; sanctioned skill flows issue it). Per-repo/per-user allow-list via the `block_dangerous_git_allow` userConfig option (comma list, any subset of `push-force,push-lease-unsafe,reset-hard,clean-force,checkout-dot,restore-dot,checkout-force`). | -| **block-hook-bypass** | PreToolUse · Bash | **Blocks** (exit 2) | Bash file-write workarounds that circumvent the Write/Edit hook gates — `cat > file`, `echo … > file`, and `python3 -c` with file-write indicators. Executable-token detection ignores quoted prose/commit text that merely mentions the pattern. | +| **block-no-verify** | PreToolUse · Bash \| PowerShell | **Blocks** (exit 2) | Git hook-bypass attempts on `git commit` / `git push`: `--no-verify` / `-n`, `core.hooksPath=` assignment, and hook-manager disable env vars — a configurable prefix set defaulting to `lefthook`, `husky`, `pre_commit`, `simple_git_hooks` (e.g. `LEFTHOOK=0`, `HUSKY=0`, `PRE_COMMIT_*=false`), tunable via `block_no_verify_hook_manager_prefixes`, including inside compound `cd … && …` commands. | +| **block-dangerous-git** | PreToolUse · Bash \| PowerShell | **Blocks** (exit 2) | Irreversible git operations: `push --force`/`-f` plus the equivalent leading-`+` refspec and `--mirror` forms, and the unsafe `--force-with-lease` spellings, in the two kinds git itself treats differently. **No expected value** (bare `--force-with-lease` or `=`) leases against the remote-tracking ref, which git documents as "trivially defeated" by a background fetch — blocked unless `--force-if-includes` is present, which git documents as the mitigation for exactly this form. **A movable `=:`** — `origin/main`, `HEAD`, a tag, an *abbreviated* object id, or hex of the wrong width for this repository's hash format, all of which git resolves at push time, and gitrevisions resolves a short hex word as a ref before trying it as an object-id prefix — is blocked unconditionally, because git declares `--force-if-includes` a no-op alongside an explicit `:`. A lease passes only when `` is immutable: a **literal** object id of the pushed repository's own hash width (detection never evaluates substitutions, so resolve it with `git rev-parse` as a separate step and pass the result) (40 hex under SHA-1, 64 under SHA-256, read from `git rev-parse --show-object-format` with the command's own `-C`/`--git-dir`/`--work-tree`/`--namespace` replayed onto it; undeterminable fails closed) or the empty string asserting the ref must not exist. The other width is a ref name there, not an object id — git ignores a ref whose name is full-width hex for its own format, but resolves one of the other width like any name. git scopes a pin to its own ref, so a bare fallback alongside a pinned entry still governs every other ref being updated; where the same ref carries several lease entries, git consults the first, and so does this guard. A trailing `--no-force-with-lease` cancels every previous lease, and a push dry-run disarms the check. Also blocked: `reset --hard`, `clean` with a force flag (any dry-run flag disarms), worktree-wide `checkout`/`restore` pathspecs (`.`, `:/`, `:(top…)` — path-scoped forms and `restore --staged .` pass), and forced `checkout -f` / `switch --discard-changes`. Accepted unique-prefix abbreviations of the blocked long options match too. `branch -D` is deliberately not blocked (reflog-recoverable; sanctioned skill flows issue it). Per-repo/per-user allow-list via the `block_dangerous_git_allow` userConfig option (comma list, any subset of `push-force,push-lease-unsafe,reset-hard,clean-force,checkout-dot,restore-dot,checkout-force`). | +| **block-hook-bypass** | PreToolUse · Bash \| PowerShell | **Blocks** (exit 2) | Bash file-write workarounds that circumvent the Write/Edit hook gates — `cat > file`, `echo … > file`, and `python3 -c` with file-write indicators. Executable-token detection ignores quoted prose/commit text that merely mentions the pattern. | | **cli-flag-verify** | PostToolUse · Write \| Edit | **Advisory** (exit 0) | Hallucinated CLI flags — a `--flag` written as a command that does not exist in the binary's actual `--help` output. Surfaces via `additionalContext`, never blocks. | | **workflow-resilience-check** | PreToolUse · Workflow | **Advisory** (exit 0) | Un-throttled Workflow fan-out — a script calling `parallel()` / `pipeline()` with no wave-cap throttle (`inWaves` / `inWavesPipeline`) and no retry wrapper (`agentRetry`), which risks a burst 529 under wide Opus fan-out. Surfaces a resilience checklist via `additionalContext`, never blocks. **Opt-in — default off since 0.20.0** (behavioral-class injector config-disabled per #2021; set `workflow_resilience_check_enabled=true` to enable). | -| **block-noncanonical-commit** | PreToolUse · Bash | **Blocks** (exit 2) | `git commit -m` whose message actually contains a newline — a multi-line `-m` flattens newlines unpredictably across shells; pipe it via `-F -` / `--file -` instead (narrowed in 0.20.0 per #2021: single-line `-m`, bare `git commit`, and repeated single-line `-m` paragraphs all pass). On the PowerShell tool a here-string `-m` value blocks too — its content is uninspectable and multi-line by construction of the form. Exempt: `--amend`, `-C`/`-c`/`--reuse-message`/`--reedit-message`, `--fixup`/`--squash`, `-F `, and any commit taken while a merge/rebase/cherry-pick/revert is in progress. Resolves `bash -lc` wrappers and git aliases (inline `-c` and persisted config alike). | -| **block-convention-violation** | PreToolUse · Bash | **Blocks** (exit 2) | A commit subject or `gh pr create --title` that violates the team-tracked convention pattern declared in `.claude/source-control.md`. No tracked pattern means no enforcement. Same exemptions as `block-noncanonical-commit`. | -| **flag-commit-pr-skill-bypass** | PreToolUse · Bash | **Advisory** (exit 0) | Any `gh pr create`, bypassing this marketplace's own `/pull-request create` skill. Only fires when the consuming project's own `.claude/settings.json` enables the `source-control` plugin — silent otherwise. Surfaces via `additionalContext`, never blocks. **Opt-in — default off since 0.20.0** (behavioral-class injector config-disabled per #2021; set `flag_commit_pr_skill_bypass_enabled=true` to enable). | +| **block-noncanonical-commit** | PreToolUse · Bash \| PowerShell | **Blocks** (exit 2) | `git commit -m` whose message actually contains a newline — a multi-line `-m` flattens newlines unpredictably across shells; pipe it via `-F -` / `--file -` instead (narrowed in 0.20.0 per #2021: single-line `-m`, bare `git commit`, and repeated single-line `-m` paragraphs all pass). On the PowerShell tool a here-string `-m` value blocks too — its content is uninspectable and multi-line by construction of the form. Exempt: `--amend`, `-C`/`-c`/`--reuse-message`/`--reedit-message`, `--fixup`/`--squash`, `-F `, and any commit taken while a merge/rebase/cherry-pick/revert is in progress. Resolves `bash -lc` wrappers and git aliases (inline `-c` and persisted config alike). | +| **block-convention-violation** | PreToolUse · Bash \| PowerShell | **Blocks** (exit 2) | A commit subject or `gh pr create --title` that violates the team-tracked convention pattern declared in `.claude/source-control.md`. No tracked pattern means no enforcement. Same exemptions as `block-noncanonical-commit`. | +| **flag-commit-pr-skill-bypass** | PreToolUse · Bash \| PowerShell | **Advisory** (exit 0) | Any `gh pr create`, bypassing this marketplace's own `/pull-request create` skill. Only fires when the consuming project's own `.claude/settings.json` enables the `source-control` plugin — silent otherwise. Surfaces via `additionalContext`, never blocks. **Opt-in — default off since 0.20.0** (behavioral-class injector config-disabled per #2021; set `flag_commit_pr_skill_bypass_enabled=true` to enable). | | **skill-reference-verify** | PostToolUse · Write \| Edit | **Advisory** (exit 0) | A `` `/plugin:skill` `` reference in markdown that does not resolve. Only fires inside a marketplace repo, and only for a plugin that repo's own manifests own — a reference to another marketplace is left alone. Resolves through manifest and frontmatter `name`, so a renamed directory still matches. Surfaces via `additionalContext`, never blocks. | | **stale-path-verify** | PostToolUse · Write \| Edit | **Advisory** (exit 0) | A repo-relative path cited in a markdown inline code span that this repo's own history shows was **deleted** and that is gone from the working tree. The gate is provenance, not absence: the exact path must appear in `git log HEAD --no-renames --diff-filter=D --name-only`, so a path belonging to a consuming project's tree, an example, or a plan is never adjudicated. Names the surviving file when exactly one tracked path now carries that basename. Link destinations are out of scope. Surfaces via `additionalContext`, never blocks. | diff --git a/plugins/guardrails/hooks/block-convention-violation.sh b/plugins/guardrails/hooks/block-convention-violation.sh index f4fcb967d1..05896b3db6 100755 --- a/plugins/guardrails/hooks/block-convention-violation.sh +++ b/plugins/guardrails/hooks/block-convention-violation.sh @@ -129,7 +129,7 @@ first_heredoc_subject() { delim="${delim%\"}" in_hd=1 fi - done <<<"$cmd" + done < <(printf '%s\n' "$cmd") # not <<<: a >=64KiB here-string deadlocks (see hardcoded-path-patterns.sh) return 0 } @@ -155,7 +155,7 @@ first_herestring_subject() { hs_quote="${line: -1}" in_hs=1 fi - done <<<"$cmd" + done < <(printf '%s\n' "$cmd") # not <<<: a >=64KiB here-string deadlocks (see hardcoded-path-patterns.sh) return 0 } diff --git a/plugins/guardrails/hooks/block-hook-bypass.sh b/plugins/guardrails/hooks/block-hook-bypass.sh index b2674f3638..3eabf0d001 100755 --- a/plugins/guardrails/hooks/block-hook-bypass.sh +++ b/plugins/guardrails/hooks/block-hook-bypass.sh @@ -245,7 +245,7 @@ strip_literals() { fi done result+="${out}"$'\n' - done <<<"$cmd" + done < <(printf '%s\n' "$cmd") # not <<<: a >=64KiB here-string deadlocks (see hardcoded-path-patterns.sh) printf '%s' "${result%$'\n'}" } @@ -494,7 +494,7 @@ cat_redirect_bypass() { [[ -n "$LAST_STDOUT_TARGET" ]] || continue [[ "$LAST_STDOUT_TARGET" == "/dev/null" ]] && continue return 0 - done <<<"$NORMALIZED_SEGMENTS" + done < <(printf '%s\n' "$NORMALIZED_SEGMENTS") # not <<<: a >=64KiB here-string deadlocks (see hardcoded-path-patterns.sh) return 1 } @@ -563,7 +563,7 @@ producer_redirect_bypass() { [[ -n "$LAST_STDOUT_TARGET" ]] || continue [[ "$LAST_STDOUT_TARGET" == "/dev/null" ]] && continue return 0 - done <<<"$NORMALIZED_SEGMENTS" + done < <(printf '%s\n' "$NORMALIZED_SEGMENTS") # not <<<: a >=64KiB here-string deadlocks (see hardcoded-path-patterns.sh) return 1 } diff --git a/plugins/guardrails/hooks/flag-commit-pr-skill-bypass.sh b/plugins/guardrails/hooks/flag-commit-pr-skill-bypass.sh index 83258a98ec..352bb3cd93 100755 --- a/plugins/guardrails/hooks/flag-commit-pr-skill-bypass.sh +++ b/plugins/guardrails/hooks/flag-commit-pr-skill-bypass.sh @@ -226,7 +226,7 @@ strip_literals() { fi line=$(printf '%s' "$line" | sed "s/'[^']*'//g" | sed -E 's/"([^"\\]|\\.)*"//g') result+="${line}"$'\n' - done <<<"$cmd" + done < <(printf '%s\n' "$cmd") # not <<<: a >=64KiB here-string deadlocks (see hardcoded-path-patterns.sh) printf '%s' "${result%$'\n'}" } diff --git a/plugins/guardrails/hooks/hardcoded-path-check.sh b/plugins/guardrails/hooks/hardcoded-path-check.sh index 5f90f96b71..19799f7232 100755 --- a/plugins/guardrails/hooks/hardcoded-path-check.sh +++ b/plugins/guardrails/hooks/hardcoded-path-check.sh @@ -216,7 +216,14 @@ if [[ -n "$VIOLATIONS" ]]; then } >&2 # Telemetry labels = the block headers only (e.g. "Linux user path detected"), # never the matched lines — those carry the actual machine-specific path. - labels_json=$(grep -E 'detected:$' <<<"$VIOLATIONS" 2>/dev/null | sed 's/:$//' | jq -R . | jq -s . 2>/dev/null) || labels_json='[]' + # Process substitution, not `<<<`. $VIOLATIONS is NOT small: each block embeds + # up to three MATCHED LINES verbatim, and the lib's `head -3` bounds the line + # COUNT, not the byte count — one 65KB minified line carrying a hardcoded path + # makes $VIOLATIONS payload-sized. A here-string of 65536-65663 bytes deadlocks + # (see lib/path-detection/hardcoded-path-patterns.sh), and it would deadlock + # HERE, on the blocked path, after the stderr message but before `exit 2` — + # turning a detected violation into a hook the harness cancels at its timeout. + labels_json=$(grep -E 'detected:$' < <(printf '%s' "$VIOLATIONS") 2>/dev/null | sed 's/:$//' | jq -R . | jq -s . 2>/dev/null) || labels_json='[]' emit_tel "blocked" "$labels_json" exit 2 fi diff --git a/plugins/guardrails/hooks/hardcoded-path-check.test.sh b/plugins/guardrails/hooks/hardcoded-path-check.test.sh index bee856c644..3e3ac4bb9a 100755 --- a/plugins/guardrails/hooks/hardcoded-path-check.test.sh +++ b/plugins/guardrails/hooks/hardcoded-path-check.test.sh @@ -487,4 +487,95 @@ else bad "telemetry: no envelope written on block" fi +# ===================== PAYLOAD-SIZE BOUNDARY (regression) ==================== +# Guards the here-string deadlock. Bash delivers `<<<` through a pipe it fills +# ITSELF before the reader is exec'd, and it appends a newline — so a payload of +# 65536-65663 bytes puts the write 1-128 bytes past the 65536-byte pipe capacity +# and bash blocks FOREVER. At >=129 bytes over, bash spills to a temp file, so +# the window is closed on BOTH sides: 65535 and 65664 always worked and only the +# band between them hung. That shape is why no ordinary size ever caught it. +# +# This hook reached the deadlock through its bundled pattern lib's pre-filter +# gate (lib/path-detection/hardcoded-path-patterns.sh), which scans EVERY write. +# Registered at `timeout: 60`, so in production the harness cancels the guard +# and the verdict is lost. Sibling fix for the same class in hook-utils.sh's +# JSON path: #1587. +# +# The payload is PIPED here, never `bash "$HOOK" <<<"$json"` — a here-string +# would hang THIS FILE at exactly these sizes and read as the bug under test. + +# Content of EXACTLY $1 bytes, ending in " $2" when $2 is given. jq reads the +# content on STDIN (`-Rs`): a 65KB `--arg` blows the Win32 32767-byte argv limit +# and jq would never run. The separating space is load-bearing here — the +# slash-rooted macOS/Linux bodies require a left boundary, so a home path glued +# straight onto filler bytes would legitimately NOT match and the "must block" +# case would pass for the wrong reason. +size_filler() { head -c "$1" /dev/zero | tr '\0' b; } +sized_write_json() { + local n="$1" tail="${2:-}" + [[ -n "$tail" ]] && tail=" $tail" + printf '%s%s' "$(size_filler $((n - ${#tail})))" "$tail" | + MSYS_NO_PATHCONV=1 jq -Rs --arg fp "$FIXTURE" \ + '{tool_name:"Write",tool_input:{file_path:$fp,content:.}}' +} + +# Bound every case so a regression FAILS LOUDLY instead of hanging CI. 150s is +# generous on purpose: the legitimate large-payload scan measured tens of +# seconds on Git Bash under Defender, while a deadlock never returns at any +# bound — so 150 separates the two without making the case flaky on a slow host. +run_bounded() { + local rc=0 + printf '%s' "$1" | CLAUDE_PROJECT_DIR="$TEST_TMPDIR" timeout 150 bash "$HOOK" >/dev/null 2>&1 || rc=$? + printf '%s' "$rc" +} + +# Asserts the EXACT code, and names 124 as its own failure. A "non-zero means +# blocked" assertion would have ACCEPTED the hang and would not have caught this +# defect — the whole point is that no verdict is not a blocking verdict. +assert_bounded_exit() { + if [[ "$3" == "124" ]]; then + bad "$1: HUNG (exit 124 at the 150s bound) — here-string deadlock regression" + elif [[ "$3" == "$2" ]]; then + ok "$1 (exit $3)" + else + bad "$1: expected exit $2, got $3" + fi +} + +# Clean payloads across the window and both shoulders. Filler is all `b`, which +# trips neither the gate's OS-path alternation nor its project-root segment +# test, so these exercise BOTH pre-filter gates and early-return clean. +for SZ in 65535 65536 65600 65663 65664; do + assert_bounded_exit "boundary: clean ${SZ}-byte payload → exit 0" \ + 0 "$(run_bounded "$(sized_write_json "$SZ")")" +done + +# The security case: a REAL hardcoded home path sitting inside the hang window +# must still BLOCK. Pre-fix this exact payload produced no verdict at all. These +# also drive the full scan AND the telemetry-label grep on the blocked path, +# where $VIOLATIONS embeds the matched 65KB line verbatim — `head -3` bounds the +# line COUNT, not the byte count, so that site was payload-sized too. +for SZ in 65536 65600; do + assert_bounded_exit "boundary: linux home path in ${SZ}-byte payload → exit 2" \ + 2 "$(run_bounded "$(sized_write_json "$SZ" "$LINUX_HOME")")" +done + +# Process substitution must not leak writer noise onto stderr. `grep -q` +# early-exits and SIGPIPEs the `printf` feeding it; stderr is this hook's +# user-facing channel, so a stray "write error: Broken pipe" would corrupt the +# blocked message. +BOUND_ERR=$(printf '%s' "$(sized_write_json 65600 "$LINUX_HOME")" | + CLAUDE_PROJECT_DIR="$TEST_TMPDIR" timeout 150 bash "$HOOK" 2>&1 >/dev/null) +assert_contains "boundary: in-window block still reports the label" "$BOUND_ERR" "Linux user path" +assert_absent "boundary: no SIGPIPE noise on stderr" "$BOUND_ERR" "Broken pipe" + +# Empty content. `<<<""` delivered ONE EMPTY LINE; `printf '%s' ""` delivers +# zero bytes. Neither can match a path pattern, and the hook's own empty-content +# guard exits first — pinned so the substitution cannot quietly become a +# behavior change. +RC=0 +printf '%s' "$(sized_write_json 0)" | + CLAUDE_PROJECT_DIR="$TEST_TMPDIR" timeout 30 bash "$HOOK" >/dev/null 2>&1 || RC=$? +assert_exit "boundary: empty content → exit 0" 0 "$RC" + report diff --git a/plugins/guardrails/hooks/secret-pattern-detection.sh b/plugins/guardrails/hooks/secret-pattern-detection.sh index 01048e8e15..f7e4e822b1 100755 --- a/plugins/guardrails/hooks/secret-pattern-detection.sh +++ b/plugins/guardrails/hooks/secret-pattern-detection.sh @@ -153,9 +153,27 @@ emit_tel() { VIOLATIONS="" LABELS=() +# Feeds $CONTENT through PROCESS SUBSTITUTION, never `<<<"$CONTENT"`. Bash +# delivers a here-string through a pipe and appends a newline, so a payload of +# 65536-65663 bytes puts the write 1-128 bytes past the 65536-byte pipe capacity +# and bash blocks FOREVER (at >=129 bytes over it spills to a temp file and +# works again). This guard blocks, so a hang means the harness cancels it at the +# hook timeout and the secret verdict is lost entirely. The deadlock is a +# property of `<<<` alone, NOT of the reader: measured at 65600 bytes, this very +# `grep -nE` (no `-q`, drains its input) hung on a here-string just as `grep -q` +# did, so the here-string had to go here regardless. +# +# Which replacement is required DOES depend on the reader, per the two-shapes +# rule at the gate in lib/path-detection/hardcoded-path-patterns.sh. This reader +# drains and its pipeline status is discarded (only $lines is read), so a plain +# `printf … | grep` would also have been correct here — process substitution is +# used for uniformity with the `-q` gates in this plugin, where a pipe WOULD +# invert the verdict under `pipefail`, so the scan sites all read as one idiom. +# `printf '%s'` matches the no-trailing-newline shape the rest of this hook +# already uses for $INPUT. check_pattern() { local label="$1" pattern="$2" lines - lines=$(grep -nE -- "$pattern" <<<"$CONTENT" 2>/dev/null | head -3 | cut -d: -f1 | tr '\n' ',' | sed 's/,$//') + lines=$(grep -nE -- "$pattern" < <(printf '%s' "$CONTENT") 2>/dev/null | head -3 | cut -d: -f1 | tr '\n' ',' | sed 's/,$//') if [[ -n "$lines" ]]; then VIOLATIONS="${VIOLATIONS}${label} (line ${lines})\n" LABELS+=("$label") @@ -200,11 +218,17 @@ SECRET_PATTERNS=( # MSYS2 + Defender that is ~60 spawns costing 10-25s. grep -qE over all patterns # is logically equivalent to "any pattern matched" — if it finds nothing, no # individual pattern can match, so skipping itemization is sound. +# +# Process substitution, not `<<<` (deadlocks at 65536-65663 bytes) and not +# `printf | grep -q` (`grep -q` early-exits, SIGPIPEs printf, and under the +# `set -uo pipefail` at the top of this file the pipeline reports 141 — which +# this `if !` would read as "no secret" and exit 0 clean, a fail-open on the +# very payload that matched). See check_pattern above. grep_e_args=() for pattern in "${SECRET_PATTERNS[@]}"; do grep_e_args+=(-e "$pattern") done -if ! grep -qE "${grep_e_args[@]}" <<<"$CONTENT" 2>/dev/null; then +if ! grep -qE "${grep_e_args[@]}" < <(printf '%s' "$CONTENT") 2>/dev/null; then emit_tel "ok" '[]' exit 0 fi diff --git a/plugins/guardrails/hooks/secret-pattern-detection.test.sh b/plugins/guardrails/hooks/secret-pattern-detection.test.sh index 4b29a4f293..24a977bc84 100755 --- a/plugins/guardrails/hooks/secret-pattern-detection.test.sh +++ b/plugins/guardrails/hooks/secret-pattern-detection.test.sh @@ -175,4 +175,92 @@ else bad "redaction: no envelope written" fi +# ===================== PAYLOAD-SIZE BOUNDARY (regression) ==================== +# Guards the here-string deadlock. Bash delivers `<<<` through a pipe it fills +# ITSELF before the reader is exec'd, and it appends a newline — so a payload of +# 65536-65663 bytes puts the write 1-128 bytes past the 65536-byte pipe capacity +# and bash blocks FOREVER. At >=129 bytes over, bash spills to a temp file, so +# the window is closed on BOTH sides: 65535 and 65664 always worked and only the +# band between them hung. That shape is why no ordinary size ever caught it. +# +# Measured against the pre-fix hook on Git Bash: a 65536-byte payload carrying a +# live-shape AWS access-key id returned NO verdict at a 200s bound, where the +# same token in a small payload exits 2 immediately. The hook is registered at +# `timeout: 60`, so in production the harness cancels the guard and the secret +# verdict is lost outright. Sibling fix for the same class in hook-utils.sh's +# JSON path: #1587. +# +# The payload is PIPED here, never `bash "$HOOK" <<<"$json"` — a here-string +# would hang THIS FILE at exactly these sizes and read as the bug under test. + +# Content of EXACTLY $1 bytes, ending in " $2" when $2 is given. jq reads the +# content on STDIN (`-Rs`): a 65KB `--arg` blows the Win32 32767-byte argv limit +# and jq would never run. The separating space matters for the sibling +# hardcoded-path suite, whose patterns require a left boundary; keeping one +# builder shape across both suites keeps them comparable. +size_filler() { head -c "$1" /dev/zero | tr '\0' b; } +sized_write_json() { + local n="$1" tail="${2:-}" + [[ -n "$tail" ]] && tail=" $tail" + printf '%s%s' "$(size_filler $((n - ${#tail})))" "$tail" | + MSYS_NO_PATHCONV=1 jq -Rs --arg fp "$FIXTURE" \ + '{tool_name:"Write",tool_input:{file_path:$fp,content:.}}' +} + +# Bound every case so a regression FAILS LOUDLY instead of hanging CI. 150s is +# generous on purpose: the legitimate large-payload itemization measured 41-67s +# on Git Bash under Defender, while a deadlock never returns at any bound — so +# 150 separates the two without making the case flaky on a slow host. +run_bounded() { + local rc=0 + printf '%s' "$1" | timeout 150 bash "$HOOK" >/dev/null 2>&1 || rc=$? + printf '%s' "$rc" +} + +# Asserts the EXACT code, and names 124 as its own failure. A "non-zero means +# blocked" assertion would have ACCEPTED the hang and would not have caught this +# defect — the whole point is that no verdict is not a blocking verdict. +assert_bounded_exit() { + if [[ "$3" == "124" ]]; then + bad "$1: HUNG (exit 124 at the 150s bound) — here-string deadlock regression" + elif [[ "$3" == "$2" ]]; then + ok "$1 (exit $3)" + else + bad "$1: expected exit $2, got $3" + fi +} + +# Clean payloads across the window and both shoulders — exercises the combined +# fast-reject gate, which is the site that scans EVERY write. +for SZ in 65535 65536 65600 65663 65664; do + assert_bounded_exit "boundary: clean ${SZ}-byte payload → exit 0" \ + 0 "$(run_bounded "$(sized_write_json "$SZ")")" +done + +# The security case: a REAL detectable secret sitting inside the hang window +# must still BLOCK. Pre-fix this exact payload produced no verdict at all. +# Two sizes: the exact pipe capacity, and mid-window. These also reach the +# per-pattern itemization (the second patched call site), which a clean payload +# never touches because the fast-reject returns first. +for SZ in 65536 65600; do + assert_bounded_exit "boundary: AWS key in ${SZ}-byte payload → exit 2" \ + 2 "$(run_bounded "$(sized_write_json "$SZ" "$AWS_TOKEN")")" +done + +# Process substitution must not leak writer noise onto stderr. `grep -q` +# early-exits and SIGPIPEs the `printf` feeding it; stderr is this hook's +# user-facing channel, so a stray "write error: Broken pipe" would corrupt the +# blocked message. +BOUND_ERR=$(printf '%s' "$(sized_write_json 65600 "$AWS_TOKEN")" | timeout 150 bash "$HOOK" 2>&1 >/dev/null) +assert_contains "boundary: in-window block still reports the label" "$BOUND_ERR" "AWS Access Key" +assert_absent "boundary: no SIGPIPE noise on stderr" "$BOUND_ERR" "Broken pipe" + +# Empty content. `<<<""` delivered ONE EMPTY LINE; `printf '%s' ""` delivers +# zero bytes. No pattern matches an empty line either way and the hook's own +# `[[ -n "$CONTENT" ]]` guard exits first — pinned so the substitution cannot +# quietly become a behavior change. +RC=0 +printf '%s' "$(sized_write_json 0)" | timeout 30 bash "$HOOK" >/dev/null 2>&1 || RC=$? +assert_exit "boundary: empty content → exit 0" 0 "$RC" + report diff --git a/plugins/guardrails/hooks/workflow-resilience-check.sh b/plugins/guardrails/hooks/workflow-resilience-check.sh index f5f90d8b26..1f164439a7 100755 --- a/plugins/guardrails/hooks/workflow-resilience-check.sh +++ b/plugins/guardrails/hooks/workflow-resilience-check.sh @@ -69,14 +69,24 @@ emit_tel() { } # Does the script fan out at all? No fan-out → no burst risk → checked, clean. -if ! grep -qE 'parallel\(|pipeline\(' <<<"$SCRIPT"; then +# +# Both greps below take $SCRIPT through PROCESS SUBSTITUTION, never `<<<`. An +# inline Workflow `script:` (or a scriptPath file read above) is unbounded +# agent-authored content, and a here-string of 65536-65663 bytes deadlocks bash +# outright — see lib/path-detection/hardcoded-path-patterns.sh for the mechanism. +# This hook is advisory and never blocks, so the cost is not a lost verdict, but +# a hang still wedges the Workflow call until the harness cancels at the hook +# timeout. `printf | grep -q` is not the alternative: `grep -q` early-exits and +# SIGPIPEs printf, and under the `set -uo pipefail` above the pipeline reports +# 141 — which the `if !` here would read as "no fan-out". +if ! grep -qE 'parallel\(|pipeline\(' < <(printf '%s' "$SCRIPT"); then emit_tel '[]' exit 0 fi # Already applies a throttle helper or a retry wrapper → author has the # resilience primitives; checked, clean. -if grep -qE 'inWaves|inWavesPipeline|agentRetry' <<<"$SCRIPT"; then +if grep -qE 'inWaves|inWavesPipeline|agentRetry' < <(printf '%s' "$SCRIPT"); then emit_tel '[]' exit 0 fi diff --git a/plugins/guardrails/lib/path-detection/hardcoded-path-patterns.sh b/plugins/guardrails/lib/path-detection/hardcoded-path-patterns.sh index 937fab49ac..f7298eff45 100644 --- a/plugins/guardrails/lib/path-detection/hardcoded-path-patterns.sh +++ b/plugins/guardrails/lib/path-detection/hardcoded-path-patterns.sh @@ -70,17 +70,34 @@ hpp::scan_text() { # costs a wasted full scan). The gate stays case-sensitive for the OS-path # alternation (matching the detailed patterns' literal "Users") and # case-insensitive for the root segment (matching the detailed `grep -Fi`). - # Uses a here-string, NOT `printf | grep -q`: a pipe + `grep -q` early-exit - # would SIGPIPE printf, and under `pipefail` the pipeline would report printf's - # failure and invert the result. - if ! grep -qE 'Users|/home/|repos|Repos|projects|Projects|dev|Dev' <<<"$content" 2>/dev/null; then + # Feeds the payload through PROCESS SUBSTITUTION — not `<<<`, and not + # `printf | grep -q`. Both alternatives are broken on whole-payload content: + # `<<<"$content"` deadlocks. Bash delivers a here-string through a pipe and + # appends a newline, so a content length of 65536-65663 puts the write + # 1-128 bytes past the 65536-byte pipe capacity and bash blocks forever + # (at >=129 bytes over it spills to a temp file and works again). A + # blocking guard that never answers loses its verdict outright — the + # harness cancels it at the hook timeout. + # `printf … | grep -q` INVERTS. `grep -q` exits at the first match and + # SIGPIPEs printf; under `pipefail` the pipeline reports printf's 141, and + # `if ! …` reads a non-zero status as "no match" and early-returns clean — + # a fail-open on exactly the content that matched. + # `grep -q … < <(printf …)` keeps the early exit, keeps the writer OUT of the + # pipeline (so `pipefail` never sees its SIGPIPE), and never blocks. Same rule + # as lib/hook-utils.sh's `printf | jq`: never feed a whole payload through + # `<<<`; use a pipe when the reader drains its input (jq), and process + # substitution when the reader may exit early (`grep -q`). + # `printf '%s'` (no trailing newline) also matches the detailed blocks below, + # so the gate and the scan see byte-identical input. + if ! grep -qE 'Users|/home/|repos|Repos|projects|Projects|dev|Dev' < <(printf '%s' "$content") 2>/dev/null; then local gate_root="" if [[ -n "$project_root" ]]; then gate_root="${project_root//\\//}" gate_root="${gate_root%/}" gate_root="${gate_root##*/}" fi - if [[ -z "$gate_root" ]] || ! grep -qFi "$gate_root" <<<"$content" 2>/dev/null; then + # Process substitution for the same two reasons as the gate above. + if [[ -z "$gate_root" ]] || ! grep -qFi "$gate_root" < <(printf '%s' "$content") 2>/dev/null; then return 0 fi fi diff --git a/plugins/guardrails/lib/powershell/ps-command.sh b/plugins/guardrails/lib/powershell/ps-command.sh index bc69957b77..96fab3d514 100644 --- a/plugins/guardrails/lib/powershell/ps-command.sh +++ b/plugins/guardrails/lib/powershell/ps-command.sh @@ -140,7 +140,7 @@ ps::blank_herestrings() { continue fi out+="${line}"$'\n' - done <<<"$cmd" + done < <(printf '%s\n' "$cmd") # not <<<: a >=64KiB here-string deadlocks (see hardcoded-path-patterns.sh) if ((in_hs)); then # Opener with no column-zero closer: ambiguous extent. Blanking to end could @@ -668,6 +668,6 @@ ps::write_bypass() { # Only the SPACED form — an attached digit prefix (`2>err.txt`, `2>&1`) is a # stream redirect whose producer is the preceding tool, not a value. [[ "$head" =~ ^[0-9]+([.][0-9]+)?$ ]] && return 0 - done <<<"$norm" + done < <(printf '%s\n' "$norm") # not <<<: a >=64KiB here-string deadlocks (see hardcoded-path-patterns.sh) return 1 } diff --git a/plugins/source-control/.claude-plugin/plugin.json b/plugins/source-control/.claude-plugin/plugin.json index cc8f809e1e..9f229565f4 100644 --- a/plugins/source-control/.claude-plugin/plugin.json +++ b/plugins/source-control/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "$schema": "https://json.schemastore.org/claude-code-plugin-manifest.json", "name": "source-control", - "version": "0.51.0", + "version": "0.51.1", "description": "Git and GitHub delivery workflow: /commit (Conventional Commits + Co-Authored-By trailer via safe heredoc mechanics), /pull-request (prep, create, CI monitoring, review-comment triage, merge, CI-log fetch), /babysit-prs (self-pacing fleet loop — safe by default; opt-in worker/autopilot tiers add gate-checked merge and thread resolution behind a deterministic Python engine), /babysit-loop (the loop-lane merge lane: a standing or drain loop that invokes babysit-prs per cycle, configured through repo-scoped babysit_loop_* keys on the layered source-control.md seam, with merge authority human-only until the target repo's tracked config adopts the lane, a gate-proven C2-mechanical baseline once adopted, and standing merge-rung raises binding from the team-tracked layer only — with one named exception, where an invocation line explicitly typing both the autopilot tier keyword and the dedicated raise argument --merge c3-this-run widens that single invocation's merge authority up to C3 behind a fresh independent frontier-tier resolver, while C4-structural and C5-untrusted-provenance stay unconditionally human-merge), /worktree (create, status, cleanup, audit for parallel-session isolation), /setup (check the effective commit-subject / PR-title convention merged across its config layers and the babysit-prs config, or apply — interview the repo and write the convention config to a chosen layer), and /resolve-conflicts (intent-first merge/rebase conflict resolution with a semantic-conflict sweep — never --abort). The commit-subject / PR-title convention is configurable via a source-control.md config written by a re-runnable setup skill, layered across a ~/.claude user-global file, the tracked team file, and a gitignored .claude/source-control.local.md personal overlay merged per key; Conventional Commits is the default when no convention is declared.", "author": { "name": "Melodic Software", diff --git a/plugins/source-control/CHANGELOG.md b/plugins/source-control/CHANGELOG.md index ecc9ff60d9..08bf023a5d 100644 --- a/plugins/source-control/CHANGELOG.md +++ b/plugins/source-control/CHANGELOG.md @@ -3,6 +3,21 @@ All notable changes to the `source-control` plugin are documented here. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this plugin uses semantic versioning. +## [0.51.1] + +### Fixed + +- **The PR-body linkage gate and validator deadlocked on a maximum-length PR body.** Both walked the + body with `while … done <<<"$body"`. Bash delivers a here-string by filling a pipe ITSELF, before + the reader is exec'd, and appends a newline, so a body of 65536-65663 bytes puts the write 1-128 + bytes past the 65536-byte pipe capacity and blocks forever. GitHub caps a PR body at exactly + 65536 characters, which lands INSIDE that window — so the worst case is not exotic, it is the + documented maximum. `pr-body-linkage-gate` is a blocking PreToolUse gate, so a hang means the + harness cancels it at its timeout and the linkage contract goes unenforced. Both now read through + `< <(printf '%s\n' "$body")`, which is byte-identical to the here-string it replaces and so + cannot drop a final line. Same class as #1587 (`hook-utils.sh`) and the guardrails scan gates fixed + alongside this. + ## [0.51.0] ### Removed diff --git a/plugins/source-control/hooks/pr-body-linkage-gate.sh b/plugins/source-control/hooks/pr-body-linkage-gate.sh index 38123e14a1..3fda4f1ba3 100755 --- a/plugins/source-control/hooks/pr-body-linkage-gate.sh +++ b/plugins/source-control/hooks/pr-body-linkage-gate.sh @@ -191,7 +191,7 @@ sole_heredoc_body() { delim="${delim%\"}" in_hd=1 fi - done <<<"$text" + done < <(printf '%s\n' "$text") # not <<<: a >=64KiB here-string deadlocks (see hardcoded-path-patterns.sh) # Still inside the body at end of text: the delimiter never appeared. ((count == 1 && in_hd == 0)) || return 1 printf '%s' "$out" diff --git a/plugins/source-control/hooks/pr-linkage-validator.sh b/plugins/source-control/hooks/pr-linkage-validator.sh index f65e0157cc..ca8ff8441e 100644 --- a/plugins/source-control/hooks/pr-linkage-validator.sh +++ b/plugins/source-control/hooks/pr-linkage-validator.sh @@ -73,7 +73,7 @@ strip_html_comments() { fi done out+="$kept"$'\n' - done <<<"$body" + done < <(printf '%s\n' "$body") # not <<<: a >=64KiB here-string deadlocks (see hardcoded-path-patterns.sh) printf '%s' "$out" } @@ -110,7 +110,8 @@ has_linkage() { related_section() { local body="$1" line t start=0 lvl i=0 out="" local -a lines=() - while IFS= read -r line || [[ -n "$line" ]]; do lines+=("$line"); done <<<"$body" + # not <<<: a >=64KiB here-string deadlocks (see hardcoded-path-patterns.sh) + while IFS= read -r line || [[ -n "$line" ]]; do lines+=("$line"); done < <(printf '%s\n' "$body") for ((i = 0; i < ${#lines[@]}; i++)); do t="${lines[i]}" t="${t#"${t%%[![:space:]]*}"}"