Skip to content

fix(guardrails): close three block-hook-bypass inline-write forms, one a reopened residual - #2367

Merged
kyle-sexton merged 4 commits into
mainfrom
fix/2217-block-hook-bypass-inline-write-forms
Aug 12, 2026
Merged

fix(guardrails): close three block-hook-bypass inline-write forms, one a reopened residual#2367
kyle-sexton merged 4 commits into
mainfrom
fix/2217-block-hook-bypass-inline-write-forms

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Summary

Three inline-write forms reached a real file through block-hook-bypass.sh without matching any
detector (#2217). All three sit inside the guard's own declared threat model. One of them — the
python3 - <<PY stdin heredoc — is a documented accepted residual, and this PR treats it as
reopening an accepted residual on new reachability evidence, not as an oversight: the acceptance
comment is updated in both files that carry it rather than contradicted.

Anchors are re-read at 4c90b454. The issue was filed before PR #2287 landed and cites
block-hook-bypass.sh:635-636 / :670; at the base of this branch those lines are :968-969 and
:1005. #2287's mark-based strip_literals rewrite is untouched by cause 1 and cause 3; cause 2
changes one join in the same function, in the direction #2287 deliberately left alone. PR #2325 is
already merged into this base, so none of its renumbered lines are touched here.

Cause 1 — the interpreter detector was a spelling floor, not a rule

Both lanes required the literal python3: the Bash lane's EXEC_LC scan and the PowerShell
lane's ps::might_write_via_python3 token test. So the identical inline write spelled python -c,
py -c, py3 -c, python2 -c or python3.11 -c ran unseen. The guard's own scope message
advertised python -c as its example — naming the one spelling the regex did not match.

The command word is now the python family (py/python/pypy + optional version suffix +
optional .exe), still separator-anchored. py -3 -c is admitted because a -<digits> token cannot
be a script path; no other gap between interpreter and flag is admitted, so python3 build.py and
python3 -m tool … still do not block.

Cause 2 — a physical newline inside a quoted span split a producer from its own redirect

A newline reached with a quote span still open is not a separator: bash is inside a quoted word,
so the text either side of the span is one word. strip_literals re-emitted it anyway,
normalize_segments split there, and producer_redirect_bypass requires producer and redirect in
one segment.

The join is now empty. Not a space — and that is the one place this PR's mechanism differs from
the one the issue suggested. ec"<newline>"ho x > f is echo x > f to bash; a space join leaves
ec ho, which _producer_head does not match, so the write is still missed. That case ships as an
assertion so the choice is evidenced rather than argued. Joining empty cannot manufacture a token
bash does not also form, because an open quote is precisely what makes the two sides one word.

Cause 3 — REOPENED ACCEPTED RESIDUAL (RECONCILE.md AD-12)

python3 - <<PY … PY (no -c) was recorded as uncovered and accepted in the PowerShell lane's
comment. Reachability evidence established before changing it, by grep over the repo:

  • .work/handoffs/20260809T082720Z-handoff-post-2008-followups.md:211 — a prior session in this
    repository reached for exactly that form (python - <<'PY') to patch a file. Note the
    spelling is python, not python3, so it is a cause-1 datum too.
  • The reporting session hit it while trying to comply with this guard's own remediation.
  • Cause 1 raises the pressure toward it: a refused python -c write reroutes most naturally to the
    heredoc.

The - is what makes it inline — the code sits in the command string the hook reads, not in an
opaque script file. strip_literals drops the heredoc operator and body, so EXEC_LC retains
python3 - while the body's write indicators stay visible in COMMAND_LC.

Narrowed residual, restated at its real width: python3 <<PY with no - stays uncovered.
Matching a bare trailing interpreter token would flip echo "pathlib" | python3 and
cat script.py | python3 to blocked. Both floors are asserted.

Direction of every behavior change

29 granted → refused, 1 refused → granted. Measured two ways, not asserted. Deliberately not
phrased as "every change is one direction" — that was the first version of this section and it was
wrong; see the correction on the review thread.

Tier 1 — the shipped suite, run against the PRE-change hook. The new assertions were copied into
a worktree at the merge base and the suite executed there, so the direction of every assertion is
read off failures rather than argued. Exact counts are in the "Suite counts" comment below; the
split is 23 expected 2, got 0 (granted → refused) and 1 expected 0, got 2 (refused →
granted — the row named below).

Tier 2 — probes beyond the suite, run against both hooks and pasted here rather than promoted to
assertions (the six below are same-shape duplicates of rows the suite already pins):

row tool before after direction
/usr/bin/python -c "open('f','w')…" Bash 0 2 granted → refused
C:\Python313\python.exe -c … PowerShell 0 2 granted → refused
& 'python' -c … PowerShell 0 2 granted → refused
py3 -c … PowerShell 0 2 granted → refused
pypy3 -c … PowerShell 0 2 granted → refused
python2 -c … PowerShell 0 2 granted → refused

The one refused → granted, named rather than buried. Fusing the two sides of a span back into
one segment also puts whatever preceded the span at the segment start, where _producer_head's ^
anchor sees it. In foo "a<newline>" echo x > f, bash's command word is foo and echo is one of
its arguments, so the redirect's producer is another program — and this guard is producer-scoped by
design (see the README's producer-scoping note). The newline previously split it into a bogus
echo x > f segment and blocked it. The single-line spelling foo "a" echo x > f is rc=0 on
main today
, so this makes the multi-line form agree with shipped behavior rather than inventing
an exemption. Both are asserted.

The mirror case is asserted next to it: echo "a<newline>" x > f, where the command word really is
the producer, moves the other way (0 → 2).

It is one row, not a class — verified, not reasoned. The obvious escalation is a legitimate
command prefix in front of the span hiding a real producer from the ^ anchor. Every prefix the
file already models was probed against both hooks and all of them still block, because
_cmd_prefix / _modifier_opt_arg / _leading_redir peel on the fused segment: env assignments,
env, if…then, !, exec -a NAME, and a leading redirect (rows 5–8 above, each paired with its
single-line control). The only text that survives to the segment start is a genuine command word,
which is exactly the case where the producer is not echo.

Every remaining floor keeps rc=0: the name anchor, the #1601/#2148 over-block repros re-run for
each new spelling, the multi-line prose/--body floor, the /dev/null discard floor and the stdin
floor.

Test plan

Hook invoked as a decision function on PreToolUse Bash payloads built with jq -n --argrc=2
blocked, rc=0 allowed. Adversarial-first: every row below was written and run against the
pre-change hook first.

Before (4c90b454) → after (this branch):

                                                    BEFORE  AFTER
### CONTROL (guard live)
echo "*" > .gitignore                                 rc=2   rc=2
git status                                            rc=0   rc=0

### C-H1 interpreter spelling floor      (granted -> refused)
python3 -c open-write                                 rc=2   rc=2
python  -c open-write                                 rc=0   rc=2
py      -c open-write                                 rc=0   rc=2
python3.11 -c open-write                              rc=0   rc=2
python2 -c open-write                                 rc=0   rc=2
py3 -c open-write                                     rc=0   rc=2
py -3 -c open-write                                   rc=0   rc=2
/usr/bin/python -c open-write                         rc=0   rc=2
/c/Python313/python.exe -c open-write                 rc=0   rc=2
pypy3 -c open-write                                   rc=0   rc=2

### C-H1 name-anchor floor                    (unchanged)
notpython3 -c write                                   rc=0   rc=0
mypython3 -c write                                    rc=0   rc=0
pythonx -c write                                      rc=0   rc=0
mypy -c write                                         rc=0   rc=0
happy -c write                                        rc=0   rc=0
spy -c write                                          rc=0   rc=0
pytest -c write                                       rc=0   rc=0

### over-block floor #1601 / #2148            (unchanged)
#1601 read-only json.load(open(p))                    rc=0   rc=0
#2148 print-only                                      rc=0   rc=0
python -c read-only open                              rc=0   rc=0
py -c print only                                      rc=0   rc=0
python3.11 -c os.path.normpath                        rc=0   rc=0
python -m tool                                        rc=0   rc=0
python build.py                                       rc=0   rc=0
python --version                                      rc=0   rc=0
py --list                                             rc=0   rc=0

### C-H3 newline-split producer          (granted -> refused)
printf 'a<NL>b<NL>' > notes.md                        rc=0   rc=2
echo "a<NL>b" > notes.md                              rc=0   rc=2
ec"<NL>"ho x > f   (one bash word = echo)             rc=0   rc=2
printf "a\nb\n" > notes.md   (escaped control)        rc=2   rc=2
cat > f with an earlier multi-line quote              rc=2   rc=2

### C-H3 blast-radius floor                   (unchanged)
gh pr --body multiline mentioning echo > f            rc=0   rc=0
git commit -m multiline prose mentioning cat > f      rc=0   rc=0
grep "foo<NL>bar" file | wc -l                        rc=0   rc=0
multi-line span discarded to /dev/null                rc=0   rc=0
multi-line span then a real producer+redirect         rc=2   rc=2
multi-line span piped to wc                           rc=0   rc=0
multi-line sq span in --body, no redirect             rc=0   rc=0
unterminated quote at end of command                  rc=0   rc=0

### G1 reopened residual                 (granted -> refused)
python3 - <<PY open-write                             rc=0   rc=2
python  - <<PY open-write                             rc=0   rc=2

### G1 stdin floor                            (unchanged)
python3 <<PY open-write (no dash)  [residual]         rc=0   rc=0
python3 - <<PY read-only                              rc=0   rc=0
cat s.py | python3 -                                  rc=0   rc=0
cat s.py | python3                                    rc=0   rc=0
echo "pathlib" | python3                              rc=0   rc=0
python3 - </dev/null                                  rc=0   rc=0
commit message quoting a heredoc write                rc=0   rc=0
cat <<EOF > file (cat lane control)                   rc=2   rc=2

Shipped contract suitebash plugins/guardrails/hooks/block-hook-bypass.test.sh. Counts are
pasted from the runs in the "Suite counts" comment below; no existing assertion changed.

Lint / repo checks run locally:

$ shellcheck -x plugins/guardrails/hooks/block-hook-bypass.sh \
    plugins/guardrails/hooks/block-hook-bypass.test.sh \
    plugins/guardrails/lib/powershell/ps-command.sh
(no output)
$ shfmt -d <same three files>
(no output)
$ scripts/check-changelog-parity.sh --check
Every versioned plugin has a CHANGELOG.md (or a stale-guarded baseline entry), and none documents a version above its manifest.
$ scripts/check-changelog-parity.sh --check-order
All 76 changelog(s) read newest-first with no duplicate versions.
$ scripts/check-changelog-parity.sh --check-bump origin/main
Every plugin whose version changed vs origin/main has a '## [<version>]' CHANGELOG.md entry.

CI also runs this suite. plugin-gate executes every plugins/**/*.test.sh (ci.yml:677) and
passed on the head commit in 5m33s, so the suite is green independently of the local runs.

Not a sign-off

Per OUTCOME.md, the required security-review check has been observed reporting pass in 16s on
a ~700-line change to this same hook
(run 31558511903) while the reviewer inside it did not run.
A green security-review on this PR should not be read as a security review of it. This is a change
to a guard whose whole job is refusing bypasses and it wants human eyes on the diff.

Related

Closes #2217.

Inbox items: 2026-08-10-plugin-quality-audit-four-components (C-H1, C-H3) and the
20260811-021645-routed audit-pass report-path item (G1).
Ledgers: .work/handoff-inbox-batch-4/ledgers/I7-four-components-023241Z.md §§ C-H1, C-H3;
.work/handoff-inbox-batch-4/ledgers/I8-audit-pass-report-path.md § G1. Adjudication:
RECONCILE.md AD-12.

Adjacent and deliberately not closed by this PR: #1601 and #2148 report this same arm
over-blocking. Both were re-verified rc=0 at the base of this branch (the write-mode
discrimination already fixed the mechanism #1601 names), and both repros are pinned as floors here —
for the new spellings as well — so this widening does not reopen them. They stay open on their own
terms.

#2227 needs no work: already shipped. repo_oid_width at origin/main captures the git error
via 2>&1, caches only a successful width (_repo_oid_width_key="" on failure), and blocks with a
distinct message via _lease_oid_width_unknown — PR #2288, commit f3336eab,
plugins/guardrails/CHANGELOG.md:134-135. The issue is CLOSED/COMPLETED.

kyle-sexton and others added 2 commits August 12, 2026 01:35
…e a reopened residual

Three inline-write forms reached a real file without matching any detector
(#2217). All three moved allowed -> blocked; nothing moved the other way.

1. The interpreter detector required the literal `python3` in both lanes, so
   `python -c`, `py -c`, `py3 -c`, `python2 -c` and `python3.11 -c` ran the same
   inline write unseen. The command word is now the python family, still
   separator-anchored so `notpython3`, `mypy`, `spy`, `happy` and `pytest` stay
   inert.

2. A physical newline inside an open quote span is not a separator — bash is
   inside a quoted word — but strip_literals re-emitted it, splitting a producer
   from its own redirect. The join is now empty, not a space: `ec"<nl>"ho x > f`
   is `echo x > f` to bash and a space join leaves `ec ho`.

3. REOPENED ACCEPTED RESIDUAL: the `python3 - <<PY` stdin heredoc, documented as
   accepted, is reopened on new reachability evidence (this repo's own session
   record shows an agent reaching for that form to patch a file). The acceptance
   comment is updated in both files rather than contradicted. `python3 <<PY` with
   no `-` stays an accepted residual, restated at its narrowed width.

Suite: PASS=345 FAIL=0 before, extended after, no existing assertion changed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@cursor

cursor Bot commented Aug 12, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 4m 8s —— View job


Security review — PR #2367

  • Gather context (PR diff, changed files)
  • Understand the request (security review only)
  • Review block-hook-bypass.sh changes
  • Review ps-command.sh changes
  • Review test/config/docs changes for security relevance
  • Post final findings

Scope: the 6 files changed in this PR (plugin.json, CHANGELOG.md, README.md, block-hook-bypass.sh, block-hook-bypass.test.sh, lib/powershell/ps-command.sh), via gh pr diff 2367. No workflow files are touched, so no pull_request_target/workflow_run/permissions/pin surface applies here — deferring to zizmor's lane as instructed for anything in that category anyway (moot, since it's a no-op on this diff).

Findings: none

This PR is itself a security-hardening change to a PreToolUse guard (block-hook-bypass.sh / ps-command.sh), and every change I traced widens (tightens) detection rather than narrowing it:

  1. block-hook-bypass.sh#L222 / #L241 — the interpreter regex changed from a literal python3 anchor to a (pypy|python|py)[0-9]*(\.[0-9]+)*(\.exe)? family match. I hand-traced the alternation order and boundary classes for the cases the PR calls out (python3, python3.11, py3, pypy3, py -3, notpython3, mypy) — the new pattern is a strict superset of what the old literal matched; I could not construct an input the old regex caught that the new one misses, nor a boundary-class gap that lets an unrelated command word (e.g. xpython3, pypython3) falsely anchor.
  2. block-hook-bypass.sh#L241 — the new _py_stdin_code pattern only adds a detection branch (OR'd into the existing condition alongside the unchanged py_write_indicator gate); it can't remove coverage that existed before.
  3. block-hook-bypass.sh#L153-L159strip_literals's line-join now emits nothing (instead of a newline) when a dropped quote span is still open at end-of-line. I checked this against bash's actual quoting semantics: an open quote span means bash has not reached a word boundary, so the two sides genuinely are one word once the quoted content is dropped — joining with nothing reconstructs exactly what bash would parse, never something bash wouldn't. The previous newline-join was the bug (it fabricated a normalize_segments boundary that doesn't exist in bash), and this only tightens the producer/redirect check, not loosens it. I could not find an adversarial input where the empty join causes two independently-separated commands to falsely merge (it only fires while a quote is still open, i.e., never at a real bash separator).
  4. ps-command.sh#L345 — mirrors change 1 for the PowerShell lane; same superset relationship holds.
  5. Version bump (0.27.20.28.0) and CHANGELOG match; no permission, config-schema, or metadata widening.

No injection surfaces, credential/token handling, or authorization logic are touched — this is pure command-string pattern matching against an untrusted Bash/PowerShell payload the guard was already parsing, and the trust boundary (LLM-agent-issued shell commands vs. the hook that vets them) is unchanged.

Caveat on this review's depth: I was not able to execute block-hook-bypass.test.sh in this sandbox (script execution required an approval that wasn't available in this run) to empirically confirm the claimed PASS=392 FAIL=0, so this is a static/manual trace review of the regex and join-logic changes rather than a test-verified one. If tighter confidence is wanted, running the suite directly (as the PR body already documents doing) is the way to close that gap. Per the task instructions, this is a security-only review — the guard's own author already flags in the PR body ("Not a sign-off") that this class of change wants human eyes regardless, which I'd echo.

Comment thread plugins/guardrails/hooks/block-hook-bypass.sh

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3ebdb46a9b

ℹ️ 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".

Comment thread plugins/guardrails/hooks/block-hook-bypass.sh
Comment thread plugins/guardrails/hooks/block-hook-bypass.sh
…ndary

Adversarial probing after the suite went green found one row moving the OTHER
way: `foo "a<NL>" echo x > f`, 2 -> 0. Fusing a carried-open span back into one
segment puts whatever preceded it at the segment start, and there bash's command
word is `foo` with `echo` as an argument — a producer this guard does not model,
by design. Its single-line spelling was already allowed on main, so this makes
the multi-line form agree with shipped behavior rather than granting a new
exemption. Pinned, with that control and with the mirror case.

Verified it is a row and not a class: every command prefix the file models
(env assignments, env, if…then, !, exec -a NAME, a leading redirect) still
blocks in front of a multi-line span, because the peels run on the fused
segment. Each pinned with its single-line control.

Direction count corrected to 23 granted->refused / 1 refused->granted in the
CHANGELOG entry; the PR body carries the measurement.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 3m 29s —— View job


Security review — PR #2367

  • Gather context (PR diff via gh pr diff 2367, changed files)
  • Understand the request (security review only)
  • Review block-hook-bypass.sh changes
  • Review ps-command.sh changes
  • Review test/config/docs changes for security relevance
  • Post final findings

Scope: all 6 files changed in this PR (plugin.json, CHANGELOG.md, README.md, block-hook-bypass.sh, block-hook-bypass.test.sh, lib/powershell/ps-command.sh), diffed against origin/main. No workflow files are touched, so no pull_request_target/workflow_run/permissions/pin surface applies — zizmor's lane is a no-op on this diff.

Findings: none

This is itself a hardening change to a defensive PreToolUse guard, and I traced every behavioral change independently (not just re-reading the two prior reviews already on the thread):

  1. block-hook-bypass.sh#L1017 (_py_inline_c) / #L1035 (_py_stdin_code) — literal python3 widened to the family (pypy|python|py)[0-9]*(\.[0-9]+)*(\.exe)?. I hand-traced POSIX ERE leftmost-longest matching for python3, python3.11, py3, py -3, pypy3, and the anchor cases notpython3/mypython3/mypy/pytest — the new pattern strictly adds coverage; I couldn't construct an input the old literal caught that the new alternation misses, nor a boundary gap letting an unrelated word falsely anchor. [0-9]*(\.[0-9]+)* is a sequential (not nested) quantifier pair separated by a literal ., so it doesn't have the (a+)+-shape that causes catastrophic backtracking — no ReDoS concern.
  2. ps-command.sh#L306 — mirrors change 1 for PowerShell. Notably the trailing boundary class also gained a . exclusion ([^[:alnum:]_.] vs the old [^[:alnum:]_]) — checked this is required precision for the new dotted-version group (prevents a stray . after an unconsumed non-digit suffix from being treated as a satisfied boundary) and only removes a prior false-anchor edge case; it doesn't create a gap in real interpreter spellings.
  3. block-hook-bypass.sh#L358-L379strip_literals's line-join now emits nothing (was: newline) when a dropped quote span is still open at end-of-line. Verified against bash's own quoting semantics: an open quote span means bash hasn't reached a word boundary, so joining empty reconstructs exactly the one word bash would parse — it can't fabricate a token bash wouldn't also form, and it only fires while a quote is genuinely still open (never at a real separator), so I can't find an input where this causes two independently-separated commands to falsely fuse into one. The one behavior flip this produces (foo "a\n" echo x > f: blocked → allowed) is a false-positive correction, not a new exemption — confirmed foo "a" echo x > f is already rc=0 on main (single-line control, same producer-scoping rule), and the PR's own prefix-boundary sweep (env assignment, env, if…then, !, exec -a, leading redirect) correctly still blocks when a real producer precedes the fused span.
  4. Test harness (block-hook-bypass.test.sh)run() passes the command string through command_json into JSON piped on stdin to the hook subprocess; it does not eval attacker-shaped strings in the test runner's own shell context. Unaffected by this PR beyond appended assertions.
  5. Version bump (0.27.20.28.0) and CHANGELOG/README updates carry no permission, config-schema, or metadata widening.

No injection surfaces, credential/token handling, or authorization logic are touched — this is pattern matching against an untrusted Bash/PowerShell command string the guard already parsed, and every change either adds a detection branch (can only tighten) or fixes a join that was manufacturing a segment boundary bash doesn't have (also tightens, with one verified false-positive removal). The trust boundary (LLM-agent-issued shell commands vs. the hook vetting them) is unchanged.

Caveat on this review's depth: like the prior automated review on this PR, I could not execute block-hook-bypass.test.sh in this sandbox (bash <script> requires an approval this run doesn't have — plain read-only git/gh/ls commands work, but subprocess script execution is blocked), so this is a static/manual regex-and-logic trace, not a test-verified run. The PR body's own Tier 1/Tier 2 measured tables are the empirical version of this; I found no daylight between them and my independent trace. As the PR author's own review comment on this thread states — a green security-review check is not evidence a reviewer executed, and this is a guard whose whole job is refusing bypasses, so it still wants a human read of the diff regardless of this check's result.

… floors

Probing the new stdin arm turned up `python3 -O - <<PY` — an interpreter option
between the name and the `-` — as uncovered, which is the same width the `-c`
arm already states: no gap beyond a `-<digits>` version selector, because
admitting an arbitrary option-shaped token is what would let a script path
through as one. Said so where the pattern is defined, and pinned both the
uncovered spelling and the glued `python3 -<<PY` that IS covered.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 4m 50s —— View job


Security review — PR #2367

  • Gather context (PR diff, changed files)
  • Understand the request (security review only)
  • Review block-hook-bypass.sh changes
  • Review ps-command.sh changes
  • Review test/config/docs changes for security relevance
  • Post final findings

Scope: all 6 changed files via gh pr diff 2367 (plugin.json, CHANGELOG.md, README.md, block-hook-bypass.sh, block-hook-bypass.test.sh, lib/powershell/ps-command.sh). No workflow files are touched, so no pull_request_target/workflow_run/permissions/pin surface applies — zizmor's lane is a no-op here regardless.

Findings: none

This is itself a hardening change to a PreToolUse command-string guard, so the relevant question is whether any change narrows detection or opens an injection/trust-boundary gap rather than tightening it. I traced each mechanism independently rather than taking the PR body's claims at face value:

  1. block-hook-bypass.sh#L240 / #L262 / ps-command.sh#L517 — the python3-literal anchors became a (pypy|python|py)[0-9]*(\.[0-9]+)*(\.exe)? family match. I hand-checked the trailing boundary class specifically, since it changed shape (ps-command.sh line 517 now excludes . from the boundary set, relying on the new ([.][0-9]+)* group to consume a version suffix instead) — this is a strict tightening, not a loosening: I could not construct an input the old literal matched that the new alternation misses, and the narrowed boundary class closes rather than opens a gap (previously python3.11 could only satisfy the boundary by accident via . counting as a terminator; now it's consumed explicitly). No ReDoS concern either — [0-9]*(\.[0-9]+)* has no overlapping-quantifier ambiguity (the literal . between groups bounds the backtracking), and bash's [[ =~ ]] uses glibc's POSIX engine, not PCRE-style backtracking, over an attacker-supplied string that's bounded by normal tool-call size.
  2. block-hook-bypass.sh#L358-L379strip_literals's line-join now emits nothing (instead of a newline) for a dropped quote span still open at end-of-line. I checked the two possible failure directions: (a) could this merge two genuinely separate bash commands into one, hiding a real producer/redirect pair from a later exemption check? No — it only fires while a quote is still open, which by definition is never a real bash separator. (b) Could it cause an under-detection elsewhere (the dangerous direction) by wrongly modeling bash's quoting? I traced the ec"<NL>"ho case Codex flagged and confirmed it's a false-positive (over-block: bash actually keeps the embedded newline literally in the word, so the "attack" command doesn't even resolve to echo) — that's a correctness issue already called out by Codex, not a security bypass, since a false positive can only refuse an exemption, never grant one. I could not find a converse case where this join causes a real producer+redirect pair to be missed.
  3. block-hook-bypass.sh#L918-L931 (block_bypass) — confirmed the emitted block messages (including the now-slightly-stale scope note Codex also flagged) interpolate only static strings, never the raw $COMMAND/$reason from attacker-controlled input, into echo/printf — no command or format-string injection surface here, unchanged by this PR.
  4. No eval, no unsafe interpolation of the untrusted command string into a further shell context, no secrets/token handling touched, no permission or config-schema widening beyond the version bump (0.27.20.28.0) and matching CHANGELOG.md/README.md prose.

No injection surfaces, credential/token handling, or authorization logic are implicated — this remains pure command-string pattern matching against an untrusted Bash/PowerShell payload, and every mechanism I could trace only adds detection coverage.

Caveat: as with the prior automated pass on this PR, I was not able to execute block-hook-bypass.test.sh in this sandbox (running it requires an approval not available in this run), so this is a static/manual trace of the regex and join-logic changes, not a test-verified confirmation of the PR's own PASS=392 FAIL=0 claim.

Per the PR author's own "Not a sign-off" note and the unresolved review thread on this same file: this is a change to a guard whose job is refusing bypasses, and it explicitly wants a human read of the diff regardless of green automated checks — I'd echo that.

@kyle-sexton
kyle-sexton merged commit eb9f338 into main Aug 12, 2026
35 checks passed
@kyle-sexton
kyle-sexton deleted the fix/2217-block-hook-bypass-inline-write-forms branch August 12, 2026 06:16
@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 6m 56s —— View job


Code review — PR #2367

  • Gather context (PR diff, changed files, CLAUDE.md/REVIEW.md)
  • Understand the request (correctness/architecture/error handling/test coverage/maintainability review; security is a separate lane per REVIEW.md and is already covered by the claude-security-review comment above)
  • Review block-hook-bypass.sh changes
  • Review ps-command.sh changes
  • Review block-hook-bypass.test.sh changes
  • Review README.md / CHANGELOG.md / plugin.json changes
  • Post inline findings
  • Post final summary

Summary

Scope: the 6 changed files (plugin.json, CHANGELOG.md, README.md, block-hook-bypass.sh, block-hook-bypass.test.sh, lib/powershell/ps-command.sh), reviewed via gh pr diff 2367 and direct reads of the merged files at 4b19140. Per REVIEW.md's lane split, security findings (injection, auth, secrets) are out of scope here — this repo has a claude-security-review workflow, whose review already ran and reported no findings.

1 inline finding posted — see the comment on block-hook-bypass.sh:1057. Summary: the per-call block_bypass message was correctly widened from "python3 -c …" to "python …", but the scope note it appends right after (_BYPASS_SCOPE_NOTE_BASH/_BYPASS_SCOPE_NOTE_PWSH, both lanes) and two nearby internal SCOPE comments were left saying "inline python3 -c only" — stale relative to this PR's own widening. It's also pinned by an unchanged test assertion (block-hook-bypass.test.sh:756-757) that will need updating alongside the message. I verified this by reading the merged file directly (not just trusting Codex's earlier comment flagging the same spot) — confirmed at block-hook-bypass.sh:907-916, :990-991, :1007-1009.

On the two Codex findings already posted on this PR:

  • The "stale scope note" finding — I independently verified it (see above) and found two additional stale locations beyond the one Codex cited.
  • The "preserve quoted newlines" finding (ec"x<NL>y"ho reconstructing as echo when the actual bash word is ecx<NL>yho) — I traced this against strip_literals's character loop (block-hook-bypass.sh:255-267): a dropped (non-redirect-operand) quoted span already collapses to nothing within a single physical line, pre-existing this PR — e.g. ec"x"ho hello > out.txt on main today already reconstructs to echo hello > out.txt for matching purposes, newline or no newline. This PR only extends that same "dropped content joins with nothing" rule across a line boundary. So Codex's counterexample is real but describes a pre-existing design property of the guard (already-accepted over-block risk, not a new regression), and it fails in the guard's safe direction (over-block, rc=2) rather than the unsafe one (missed write). I'd concur with Codex's P2/low-severity tag rather than raise its severity.

Not independently verified: I was not able to execute block-hook-bypass.test.sh or invoke the hook directly in this sandbox (running the hook script required an approval that wasn't available), so the PR body's PASS=392 FAIL=0 counts, the PASS=377 FAIL=15 pre-change run, and the direction table (23 granted→refused, 1 refused→granted) are author-claimed and unverified by me — this matches the security-review comment's same caveat. The regex/logic changes I did hand-trace (interpreter family alternation boundaries for python/py/pypy/mypy/pytest/notpython3, the py -3 -c version-selector gap, the empty-join rationale in strip_literals) are internally consistent with the PR's stated behavior and its own extensive test additions.

No other correctness, error-handling, or test-coverage issues found in ps-command.sh, CHANGELOG.md, plugin.json, or README.md — the README's guard-scope table and prose were updated accurately (in contrast to the hook's own internal messages/comments, which is what makes the inline finding above an omission rather than a deliberate choice).
· branch: claude/pr-2367-20260812-0617

if [[ "$EXEC_LC" =~ $_py_inline_c || "$EXEC_LC" =~ $_py_stdin_code ]] &&
py_write_indicator "$COMMAND_LC"; then
block_bypass "python-write" "python3 -c file write bypasses Write/Edit hooks"
block_bypass "python-write" "python inline-code file write bypasses Write/Edit hooks"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The per-call block message was generalized here, but the scope note block_bypass() appends right after it wasn't — so the remediation text still under-states coverage.

This line (and its PowerShell-lane counterpart at block-hook-bypass.sh:978) was correctly widened from "python3 -c inline-code file write…" to the generic "python inline-code file write…". But block_bypass() unconditionally appends _BYPASS_SCOPE_NOTE_BASH / _BYPASS_SCOPE_NOTE_PWSH right after, and neither was touched by this PR — they still read (block-hook-bypass.sh:907-908 and :912-914):

"known shell file-write forms plus inline python3 -c only"

after this PR deliberately widened coverage to the whole interpreter family (py/python/pypy + version suffix) and added the stdin-heredoc form (python3 - <<PY). Two more internal SCOPE (documented residual) comments a few lines below this one — :990-991 and :1007-1009 — say the same stale "python3 -c" thing.

This isn't cosmetic: the file's own rationale for carrying this note at all (:898-906) is that an inaccurate scope claim gets a human/agent to "credit the guard with coverage it never claimed" — here it's the opposite failure, understating coverage to the exact audience (a blocked agent reading its own remediation text) most likely to act on it, e.g. concluding py -c or a stdin heredoc is still an available workaround when both are now blocked.

It's also pinned by an existing regression test that this PR left unchanged — block-hook-bypass.test.sh:756-757:

assert_contains "bash block limits interpreter coverage to python3 -c" "$scopeout" \
  "inline python3 -c only"

So fixing the message requires updating that assertion's expected substring too (and adding an equivalent one for the PowerShell note, which isn't currently pinned at all).

Fix this →

@github-actions

Copy link
Copy Markdown

Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Post-merge: final measured numbers, and two reviewer findings this PR merged past.

This merged at 06:16Z while review / review was still pending; it later passed (7m30s), as did all
35 checks. Recording what landed after the merge, since the PR body could no longer be the record.

The direction count is confirmed at the figure the body claims. The full suite was run against
the pre-change hook at the merge base:

branch (this PR):    PASS=412 FAIL=0
base   (4c90b454):   PASS=388 FAIL=24
  of the 24:  23 x "expected exit 2, got 0"   (granted -> refused)
               1 x "expected exit 0, got 2"   (refused -> granted)

23 / 1 in the shipped suite, plus 6 probe-verified rows in the granted → refused direction = 29
granted → refused, 1 refused → granted
. The one is foo "a<newline>" echo x > f, named and
justified in the body.

Two P2s from the automated reviewer arrived and were resolved without a fix. Both are real:

  1. Scope note stale (block-hook-bypass.sh:1057). Correct, and it is the worse of the two: this
    PR widened the python lane but both emitted notes still said inline python3 -c only — a safety
    guard understating its own enforced surface, which is what invites the contortion the note's own
    preamble warns about. The contract test pinned the literal obsolete string, so it kept passing.
    Fixed in fix(guardrails): restate block-hook-bypass's emitted scope note at its shipped width #2384, which also repins the assertion at the family, the stdin form and the residual
    so it cannot drift again.

  2. Command-word splice (block-hook-bypass.sh:376). Symptom real, stated mechanism incomplete —
    and the difference changes what the fix must be. The reviewer says the empty join reconstructs
    echo from ec"x<newline>y"ho, so the newline should be marked opaque. Measured:

                                                    base(4c90b454)  after this PR
    ec"xy"ho hello > out.txt      (single, non-empty)     rc=2          rc=2
    ec"x<NL>y"ho hello > out.txt  (multi,  non-empty)     rc=0          rc=2
    ec""ho x > f                  (single, empty span)    rc=2          rc=2
    ec"<NL>"ho x > f              (multi,  empty span)    rc=0          rc=2
    

    The single-line form was already rc=2 on mainstrip_literals has always dropped a
    quoted span's content unmarked outside a redirect operand, so the splice predates this work.
    This PR made the multi-line form agree with it. Marking the join opaque fixes only the
    multi-line half, puts the two spellings back in disagreement, and loses the empty-span cases
    (ec"<newline>"ho x > f genuinely is echo to bash). Filed as fix(guardrails): strip_literals drops a quoted span's content unmarked, splicing a command word into a false echo #2385 with the corrected
    mechanism, so a future fix is not written against the wrong one. LOW — a false positive, not a
    bypass.

The claude review thread on this PR is left unresolved deliberately: security-review passing
(5m23s here, a real duration rather than the 16s pattern from #2287) is still not evidence a human
read this diff, and this is a change to a guard whose job is refusing bypasses.

kyle-sexton added a commit that referenced this pull request Aug 12, 2026
…s shipped width (#2384)

## Summary

Follow-up to #2367, which merged while its automated review was still
landing. The reviewer's second
P2 is correct and this fixes it.

#2367 widened `block-hook-bypass`'s python lane from the literal
`python3 -c` to the interpreter
family plus a `python3 - <<PY` stdin heredoc — and left both **emitted
scope notes** saying
`inline python3 -c only`.

That note is not commentary. It is the guard's contract with whoever it
just blocked, and the
paragraph directly above it in the source says why: understating the
surface makes an agent contort
around a restriction that isn't there, and overstating it makes a human
credit coverage the guard
never had. Shipping a widened detector behind a narrower claim is the
same false-account defect as
the reverse, and it points the blocked agent at a form it is told the
guard cannot see — which, after
#2367, it can.

**The contract test is the part that should have caught this and
didn't.** Its assertion pinned the
literal string `inline python3 -c only`, so it kept passing while the
claim it pinned went stale. It
now pins the interpreter family, the stdin form, **and** the no-dash
residual, so the note cannot
drift from the detector again without a failure.

**No detector behaviour changes: 0 granted → refused, 0 refused →
granted.** Only the two note
strings and the assertions over them.

## Test plan

The notes as actually emitted, by invoking the hook on a blocked payload
(not read off the source):

```
$ bash plugins/guardrails/hooks/block-hook-bypass.sh <<< '{"hook_event_name":"PreToolUse","tool_name":"Bash","tool_input":{"command":"cat > f.txt"},...}'
Scope: only this command string is inspected — known shell file-write forms plus inline python
code (python/python3/py/pypy with -c, or a program read from stdin as python3 - <<PY) only. POSIX
tee pipe writes, other inline-interpreter writes (e.g. node -e, sed -i), a stdin heredoc with no -
argument (python3 <<PY), writes inside an invoked script file or a program's own opaque code, and
redirects produced by another program, are not seen.

$ ... tool_name PowerShell, command "Set-Content f.txt x"
Scope: only this command string is inspected — known PowerShell file-write cmdlets and
content-producer redirects (including Tee-Object and the tee alias) plus inline python code
(python/python3/py/pypy with -c) only. Other inline-interpreter writes (e.g. node -e), writes inside
an invoked script file or a program's own opaque code, and redirects produced by another program,
are not seen.
```

Each clause is checked against the detector it describes, all of which
are already asserted in the
suite from #2367: the family (`python -c`, `py -c`, `py3 -c`, `python2
-c`, `python3.11 -c`,
`pypy3 -c` all `rc=2`), the stdin form (`python3 - <<PY` `rc=2`), and
the residual
(`python3 <<PY` with no `-` `rc=0`).

**Lint:**

```
$ shellcheck -x plugins/guardrails/hooks/block-hook-bypass.sh plugins/guardrails/hooks/block-hook-bypass.test.sh
(no output)
$ shfmt -d <same two files>
(no output)
```

The suite count moves from 412 to 414 (two assertions replaced by five,
plus one on the PowerShell
note). `plugin-gate` runs `plugins/**/*.test.sh` and is the gate on it.

## Related

Follow-up to #2367 (closes #2217). Raised by the automated reviewer on
that PR at
`block-hook-bypass.sh:1057`, resolved there without a fix because the PR
had already merged.

**Also raised on #2367 and deliberately NOT fixed here — filed
separately instead.** The reviewer's
first P2 says the empty line-join reconstructs `echo` from
`ec"x<newline>y"ho`, which bash would run
as `ecx<newline>yho`. The symptom is real; the stated mechanism is
incomplete, and measuring it
changes what the fix has to be:

```
                                                        base(4c90b45)  after #2367
ec"xy"ho hello > out.txt      (single line, non-empty)       rc=2           rc=2
ec"x<NL>y"ho hello > out.txt  (multi-line, non-empty)        rc=0           rc=2
ec""ho x > f                  (single line, empty span)      rc=2           rc=2
ec"<NL>"ho x > f              (multi-line, empty span)       rc=0           rc=2
```

The single-line row was **already** `rc=2` before #2367 —
`strip_literals` has always dropped a
quoted span's content without marking it, so the command-word splice
predates this work by a long
way. #2367 made the multi-line form agree with the shipped single-line
form. Marking the *newline
join* opaque, as suggested, would fix only the multi-line half and put
the two spellings back in
disagreement, while also losing the empty-span case (`ec"<newline>"ho x
> f` genuinely *is* `echo` to
bash). A coherent fix belongs at the dropped-span emit site for both
line shapes and needs to
distinguish an empty span from a non-empty one — bigger than a note fix
and its own change.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(guardrails): block-hook-bypass producer detection misses three inline-write forms (one a documented accepted residual)

1 participant