fix(guardrails): state block-hook-bypass's enforcement scope in its block message - #1821
Conversation
…lock message The message said a write was prevented and that Write/Edit is the sanctioned path, with nothing about scope, so it read as "shell file writes are blocked". The guard is deliberately producer-scoped over a single command string, and the gap runs in both directions: an agent concludes shell file writes are unavailable and contorts around a restriction a script file does not have, while a human credits the guard with coverage it never claimed — the more expensive error where the guard is load-bearing in someone's threat model. Verified against the hook with fixture input. printf 'x' > out.log blocks, while bash execute.sh — whose script may write freely — is allowed, as reported. Two shapes the report did not name are allowed too, and they decide the wording: bash execute.sh >> run.log and sort data.txt > out.txt are direct redirects in the command string and are allowed by the producer-scoped design, as is cat a.txt b.txt > c.txt, since only the stdin-consuming cat > f form is a write workaround. The report's suggested line, "direct redirects in this command only", would therefore have overstated coverage in the other direction. The shipped note says instead that only this command string is inspected, only known file-write forms in it are matched, and neither a write inside an invoked script nor a redirect produced by another program is seen. The README's residuals section states the same scope beside the existing quoted-span residual, so the limit is documented where consumers read the guard's guarantees rather than only at the moment of a block. No hook logic changes. The behaviour the note describes is pinned by tests beside the message-content assertions, so the two move together. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C
|
Claude finished @kyle-sexton's task in 2m 4s —— View job PR Review
Summary: Clean, well-scoped disclosure fix. No hook logic changes. All five blocking lanes (
|
|
Claude finished @kyle-sexton's task in 1m 18s —— View job Security Review — PR #1821
FindingsNo security issues found. This PR is a disclosure-only fix. The only functional code change is a static string constant and one
_BYPASS_SCOPE_NOTE="Scope: only this command string is inspected, and only known \
file-write forms in it. Writes performed inside a script or program this command \
invokes, and redirects produced by another program, are not seen."The constant is a pure string literal — no variable interpolation, no shell-special characters that would expand. It reaches the caller exclusively via Detection logic — unchanged. Nothing that is blocked or allowed changes in this PR. No trust boundary, permission scope, or access-control decision is altered. Information disclosure concern (considered and rejected): Surfacing the guard's gaps in the block message — e.g., "writes inside an invoked script are not seen" — could theoretically help an adversarial agent route around it deliberately. However, (a) the gaps already existed in the code and were already documented in the file header comments and README's Scope section, so this is not new information; (b) the guard is explicitly designed and documented as a speed bump against accidental write workarounds, not a hard security boundary; and (c) over-trust of the guard's coverage (the error this fixes) is the more expensive failure mode the PR author identified, not disclosure of the limit. Tests and documentation — new assertions in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 776052cf94
ℹ️ 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".
…le gate surfaced shell-portability-lint gates CHANGED files, so editing block-hook-bypass.sh surfaced a pre-existing hit at an untouched line: the linter reads the `\<` and `\>` inside a Bash bracket PATTERN as GNU grep/sed word-boundary operators. They are backslash-escaped literals, and Bash pattern matching behaves identically on BSD userland, so the construct is portable as written. Annotated with the repo's sanctioned portability-ok escape and the reason at the site, rather than rewriting a correct predicate to satisfy a text scan. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C
|
Claude finished @kyle-sexton's task in 1m 53s —— View job PR Review — #1821
Codex P2 Finding — AssessedThe finding claims the scope note is "internally contradictory" when emitted by the Why it's not contradictory. The scope note has two clauses:
The "A script or program this command invokes" refers to the case where the write lives in an external file — The Codex conflates two distinct cases:
The note is accurate for both cases. No change needed. One minor wording observation (no action needed): a reader who only skims the second clause could misread "a program this command invokes" as covering the Python interpreter for the Verdict: The Codex P2 finding is a false positive. The scope note is not contradictory — it correctly applies to external-file invocations, not inline |
|
Claude finished @kyle-sexton's task in 1m 22s —— View job Security Review — PR #1821
FindingsNo security issues found. This PR is a documentation/disclosure fix. The only functional code additions are a static string constant and one
_BYPASS_SCOPE_NOTE="Scope: only this command string is inspected, and only known \
file-write forms in it. Writes performed inside a script or program this command \
invokes, and redirects produced by another program, are not seen."( The constant is a pure string literal — no variable interpolation, no
(L226–228) A comment explaining that Detection logic — unchanged. Nothing that is blocked or allowed changes in this PR. No access-control decision, permission scope, or trust boundary is altered. Information disclosure (considered and rejected as a finding): Codex flagged that naming the invoked-program blind spot in the block message could help an adversarial agent route around the guard deliberately. However: (a) the blind spot is already declared in the file's own header comments and in the README's Scope section, so this is not new information; (b) the guard is explicitly designed and documented as a speed bump against accidental write-workarounds, not a hard security boundary; and (c) over-crediting the guard's coverage — the failure mode this PR fixes — is the more dangerous error in a threat model where someone relies on this guard as load-bearing. Disclosing accurate scope is a net security improvement, not a disclosure vulnerability. No GitHub Actions files changed. No new dependencies, action pins, or permission changes. No secrets or credential handling touched. |
…ind-spot note The scope disclosure claimed writes inside an invoked program are not seen immediately after _py_write blocked exactly such a write. The blind spot now claims only an invoked script file or a program's own opaque code, with the inline-code exemption pinned by the tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STASxNiqEm7doy2Z65un4H
…ypass-scope # Conflicts: # plugins/guardrails/.claude-plugin/plugin.json # plugins/guardrails/CHANGELOG.md
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
|
Warning Automated review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-run the job to retry the review. A new push does not re-trigger this lane. |
|
Claude finished @kyle-sexton's task in 1m 27s —— View job Security Review — PR #1821
FindingsNo security issues found. This PR remains a disclosure/documentation-only change at the current HEAD (52d79ec). Since the last automated pass, the only substantive update is the wording fix (
Information disclosure (reconfirmed, not a new concern) — stating the guard's blind spot (invoked-script contents, external-program redirects) in the block message is not new information: it was already documented in the file's header comment (
No GitHub Actions files are touched by this PR, so no CI trigger, permissions, or supply-chain surface to review here. |
#1840) ## Summary `hpp::scan_text`'s macOS block defanged `Shared` tokens inside a per-candidate `while read` loop, spawning a `sed` and a `grep` for every candidate line. The loop's only escape was the trailing `head -3`, and that short-circuit fires when candidates **survive** the defang. On a block where every candidate is a legitimate `Users/Shared` reference, none survives, nothing is ever written, `head -3` never closes the pipe, and the loop runs to completion. So the guard was slowest on precisely the innocent content the exclusion exists to serve, and fastest on violations — the wrong way round for something with a hook timeout. A `PreToolUse` guard killed at its timeout **fails open**, which makes this a correctness bug, not a speed one. This plugin has been bitten by that before (#1345). The defang now runs **once over the whole candidate block**: - One `sed` over the candidate block. `sed` is line-oriented in this pipeline — no `N`/`H` multiline commands, and `$` anchors per line in both shapes — so hoisting cannot change any individual line's result. - One `grep -nE` over the defanged block yields the block-relative indices of the survivors; `awk` then selects those lines from the **original** block by `NR`. The reported entry therefore still carries its original line number and original un-defanged text, never the defanged copy. - A block containing no `Shared` token at all skips the pipeline entirely via a bash-builtin substring test. The defang is a provable no-op there, so the common case costs nothing. `grep -E` remains the sole matcher and `awk` does no regex work, so no second regex dialect enters and the shared `HPP_*` bodies stay the single source of truth. The survivor re-test also strips `grep -n`'s `<n>:` line-number prefix before matching. Hoisting made that necessary and it is easy to miss: the re-test runs over the **numbered** candidate lines, so a violation at **column 0** arrives as `<n>:/Users/…` and can no longer satisfy the left boundary's `^` alternative. It matched anyway only because that class also accepts `:` — a member added for yaml/docker value position, which owes this pipeline nothing. Narrowing the class for its own stated purpose would therefore have silently dropped a violation the first pass had already flagged. Verified by rerunning the pipeline with `:` removed from the class: the column-0 survivor set goes from `[2]` to `[]` unstripped, and stays `[2]` stripped. The strip is one more expression on the `sed` the defang already runs, so it adds no process, and a column-0 case now pins it. Detection semantics are otherwise unchanged. Only the hoisting was ported — this repo's `_posix_boundary` and its `[^A-Za-z0-9._-]` defang boundary class are untouched. One adjacent fail-open fix: the candidate assignment gains an explicit `|| true`. Its trailing `grep -v` exits non-zero whenever nothing survives the Windows exclusion (the common clean case), and this library is sourced by commit-time hooks whose shell options it does not control — aborting there under `set -e` would fail open in the same way. ## Measured A matched pair: one machine, one harness, the same two corpora driven **through the hook**, with only `lib/path-detection/hardcoded-path-patterns.sh` swapped between the sides. Spawn counts come from `grep`/`sed` shims on `PATH`; wall clock is `EPOCHREALTIME` around the hook invocation with the payload precomputed outside the timed region. | Corpus — 100 vs 400 Shared-only lines | Per-candidate loop | Hoisted | | --- | --- | --- | | `grep`/`sed` spawns at 100 | 210 (100 `sed` + 110 `grep`) | 12 | | `grep`/`sed` spawns at 400 | proportional | 12 | | Wall clock at 100 | 22s (median of 3: 21.2 / 22.0 / 23.5) | 11s (median of 5, range 9.5–13.9) | | Wall clock at 400 | 288s | 10s (median of 5, range 5.0–15.2) | Spawn count is the exact figure; wall clock is its consequence. The per-candidate shape grew **13x for a 4x input increase** — super-linear, because fork pressure compounds — while the hoisted shape is flat and its spread is machine noise. A control corpus of the same size carrying no `Shared` token cost 1.9–4.0s on **both** sides, which places the delta in the defang rather than in payload size. The figures in the issue (108s per-line against 0.92s hoisted) are #1792's own measurement on a different machine, not this pair. ## Test plan Regression cases in `hardcoded-path-check.test.sh`, plus the existing suite: - **Bounded, not per-candidate** — the headline pin. It counts **subprocesses**, not seconds. `grep`/`sed` shims on `PATH` tally every spawn the hook makes, and the tally must not move when the input quadruples. A companion assertion fails if the shims never fire, so the equality cannot pass vacuously. This replaces a wall-clock ratio assertion, and the reason is the most reviewer-relevant fact in the PR: **the timing assertion failed on unchanged code.** Repeats of the identical 400-line corpus measured 5.0s and 15.2s, so the noise floor was wider than the 4x signal the ratio existed to detect, and the gate reported `FAIL: Shared defang scales with candidate count: 4s at 100 lines, 18s at 400` on a green branch. Widening the tolerance would have left a gate that cannot discriminate the bug it guards. A count is exact, load-invariant, and pins the property the fix actually establishes — a constant subprocess count. - **Shared-padded block** — four Shared-only lines then a real user path at line 5, so the violation sits beyond the first three candidates. Asserts the reported entry carries the **original file line number** (`5:cd …`) and that the defanged spelling never appears in output. This is exactly where a block-relative index would leak through in place of the file-relative one. - **Violation at column 0 inside a Shared block** — pins the prefix strip described above. It passes both with and without the strip *today*, which is the point: it guards the coupling rather than a live defect, and it fails the moment `:` leaves the boundary class if the strip is ever reverted. - **All candidates excluded as Windows paths** — the candidate block is empty after the `-v` stage, which under `pipefail` reports failure. Asserts exit 0 and silence, distinguishing a clean pass from a silent abort. - Existing Shared cases (bare `Shared` at EOL, `Shared` + user path on one line, `SharedStuff`, trailing punctuation, quoted forms) all still pass, pinning that the exclusion stays match-level. Results on this branch: ``` ok: spawn shims active (12 grep/sed spawns at 100 lines) ok: Shared defang is bounded, not per-candidate (12 spawns at 100 lines, 12 at 400) PASS=81 FAIL=0 ``` Version bumped 0.18.4 → 0.18.5 with a matching `## [0.18.5]` CHANGELOG entry. (The branch originally claimed 0.18.4; #1821 released that version on `main` while this was open, so it was renumbered when this branch rebased rather than co-owning a released version.) Gates, all run from the worktree root after the rebase: | Gate | Result | | --- | --- | | `bash plugins/guardrails/hooks/hardcoded-path-check.test.sh` | PASS=81 FAIL=0 | | `shellcheck` (lib + test) | clean | | `shfmt -d` (lib + test) | clean | | `scripts/check-shell-portability.sh origin/main` | PASS — 2 files, no unexcused GNU-only constructs | | `scripts/check-changed-skills.sh origin/main` | PASS — no changed skills to gate | | `scripts/check-changelog-parity.sh --check` | PASS | | `scripts/check-changelog-parity.sh --check-bump origin/main` | PASS | | `scripts/check-changelog-parity.sh --check-order` | PASS — 71 changelogs, newest-first, no duplicates | | `scripts/check-cross-plugin-source-drift.sh --check` | PASS — no unregistered or drifted clusters | | `scripts/check-silent-skips.sh` | PASS | | `scripts/validate-plugins.sh` | PASS — manifests + catalog | | `markdownlint-cli2 "plugins/guardrails/**/*.md"` | PASS — 0 issues, 3 files | ## Related - Fixes #1792 - Refs #1095 — the match-level Shared carve-out this reports on - Refs #1345 — prior incident: guards killed at their timeout - Refs melodic-software/medley#1685 — the same block-hoisted shape over the same `machine-path-patterns.sh` bodies. Porting it here reconverges the two drivers rather than leaving them forked. That PR also carries a derived `_seg_end` and a `:-` boundary addition which are **not** included here — those are separate semantic changes belonging to their own issues. ### Overlap with concurrent guardrails work Sibling lanes are working #1814/#1811/#1810 (git wrapper argv parsing) on other branches. Different code path — those touch the commit-guard argv helpers, this touches the path-detection driver — so no functional overlap is expected. Both land in the same plugin, so `CHANGELOG.md` and `plugin.json`'s version are the likely textual conflict points; whichever merges second rebases the version bump. Rebased onto current `origin/main`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01FVoZoMYXqf8ZVbQYixPVPW Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Summary
block-hook-bypass's block message asserted that a write was prevented and that Write/Edit is the sanctioned path, with nothing about scope — so it read as "shell file writes are blocked". The guard is deliberately producer-scoped over a single command string, and the gap runs in both directions: an agent concludes shell file writes are unavailable and contorts around a restriction a script file does not have, while a human credits the guard with coverage it never claimed — the more expensive error where the guard is load-bearing in someone's threat model.No hook logic changes. This is a disclosure fix plus the tests that keep the disclosure true.
Fix
block_bypass()gains a third line, so every lane that blocks carries it:The issue's alternative suggestion is taken as well, not instead: the
READMEresiduals section — which already documents this guard's quoted-span residual — now states the same scope, so the limit sits where consumers read the guard's guarantees and not only at the moment of a block.Verification
Driven against the hook with fixture PreToolUse JSON, never by live-triggering it.
The reported behaviour reproduces:
printf 'x' > out.logexits 2, whilebash execute.sh,./verify.sh, andpython3 build.pyall exit 0 — the identical redirect inside an invoked script is not inspected.The issue's suggested wording would have overstated coverage in the other direction, which is why the shipped line differs from it. The suggestion was "direct redirects in this command only", but these are all direct redirects in the command string and all exit 0 by design:
bash execute.sh >> run.logsort data.txt > out.txtcurl https://example.com > page.htmljq . in.json > out.jsonecho hi | tee out.txtcat a.txt b.txt > c.txtThe last row matters: only the stdin-consuming
cat > fform is a write workaround, so evencatwith input files is ordinary data processing and is allowed. The real scope is one command string and only the specific content-producer write forms —cat > fconsuming stdin,echo/printf > f, inlinepython3 -cwrites, and the PowerShell write cmdlets — which is what the shipped note says.Gates:
bash plugins/guardrails/hooks/block-hook-bypass.test.sh— PASS=210, FAIL=0. Seven new assertions: the scope line present on both the Bash and PowerShell lanes, and the four allowed shapes above pinned beside them, so a future change to what the guard blocks cannot silently make its message wrong again.shellcheckon the hook — clean;markdownlint-cli2on both changed markdown files — 0 errors;check-changelog-parity.sh --check-order— pass.Related
disk-hygiene; filed and fixed separately, no shared code with this change.Fixes #1802
🤖 Generated with Claude Code
https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C