fix(guardrails): diagnose repo_oid_width probe failures (#2227) - #2288
Conversation
…ching width 0 When git cannot read --show-object-format, fail closed with the git error and a distinct block message instead of treating every full-width SHA as a movable lease. Do not cache probe failures for the rest of the invocation. Fixes #2227 Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
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 finished @kyle-sexton's task in 2m 11s —— View job Security Review — PR #2288
Scope: All 4 changed files reviewed. No GitHub Actions workflow files are touched by this PR, so the zizmor-covered lanes (unpinned actions, dangerous triggers, excessive Findings1. SUGGESTION — Raw git stderr is now reflected verbatim into the agent-facing block message Failure scenario: an agent operating in an attacker-supplied/cloned repository containing a Why this is low severity in practice: the guard still fails closed in every case (exit 2, action denied) regardless of message content, and the agent already has direct read access to Confidence: LOW–MEDIUM (plausible based on documented git config-parsing error behavior; not confirmed exploitable and not a bypass of the guard's own fail-closed logic). No other issues found
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f82790d224
ℹ️ 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".
|
Claude finished @kyle-sexton's task in 4m 44s —— View job Code review complete
SummaryThe fix is sound: One inline finding posted on the new block message wording (minor). Stale doc comment this PR's own nearby edit highlights but doesn't fix —
This is now inaccurate: after this PR, a failed probe leaves Notes
|
…ease Trailing lease negation must reset lease_width_unknown so a cancelled pinned lease whose width probe failed does not still block the push. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Warning Automated security 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."
The check is green on purpose, and it is not evidence. It certifies that a security pass ran, and this one did not complete — but the cause is outside this PR's control, so merging is deliberately left unblocked rather than locking every merge for the length of the outage. Nothing was reviewed at this head. Where this check is required, it is satisfied without that evidence; a human should review security-sensitive changes here before merging. Re-run the job to retry the review; a new push also retries it only if the caller's Re-running does NOT help for every class:
|
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…idth-2227-63e1 Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
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 re-triggers this lane only if the caller's |
…e a reopened residual (#2367) ## 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 **23 granted → refused, 1 refused → granted.** Measured two ways, not asserted. **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: ``` $ bash plugins/guardrails/hooks/block-hook-bypass.test.sh # pre-change hook, new assertions FAIL: #2217: python -c open write (blocked): expected exit 2, got 0 FAIL: #2217: py -c open write (blocked): expected exit 2, got 0 FAIL: #2217: py3 -c open write (blocked): expected exit 2, got 0 FAIL: #2217: python2 -c open write (blocked): expected exit 2, got 0 FAIL: #2217: python3.11 -c open write (blocked): expected exit 2, got 0 FAIL: #2217: pypy3 -c open write (blocked): expected exit 2, got 0 FAIL: #2217: py -3 -c open write (blocked): expected exit 2, got 0 FAIL: #2217: path-qualified python.exe -c open write (blocked): expected exit 2, got 0 FAIL: #2217: printf, physical newline in a single-quoted arg (blocked): expected exit 2, got 0 FAIL: #2217: echo, physical newline in a double-quoted arg (blocked): expected exit 2, got 0 FAIL: #2217: quote span splicing a command word (blocked): expected exit 2, got 0 FAIL: #2217: python3 - <<PY heredoc write (blocked): expected exit 2, got 0 FAIL: #2217: python - <<PY heredoc write, family spelling (blocked): expected exit 2, got 0 FAIL: #2217: PS python -c open write (blocked): expected exit 2, got 0 FAIL: #2217: PS py -c open write (blocked): expected exit 2, got 0 PASS=377 FAIL=15 ``` Every failure is `expected 2, got 0` — granted → refused. None is `expected 0, got 2`. **Tier 2 — adversarial probes written after the suite was green, specifically hunting the other direction.** Eight more rows move; all eight are now assertions too: | row | before | after | direction | |---|---|---|---| | `/usr/bin/python -c "open('f','w')…"` | 0 | 2 | granted → refused | | `echo "a<NL>" x > f` | 0 | 2 | granted → refused | | `cat "a<NL>" > f` | 0 | 2 | granted → refused | | `echo "a<NL>b" "c<NL>d" > f` | 0 | 2 | granted → refused | | `if true ; then echo "a<NL>b" > f ; fi` | 0 | 2 | granted → refused | | `! echo "a<NL>b" > f` | 0 | 2 | granted → refused | | `exec -a n echo "a<NL>b" > f` | 0 | 2 | granted → refused | | `FOO=1 printf "a<NL>b" > f` | 0 | 2 | granted → refused | | **`foo "a<NL>" echo x > f`** | **2** | **0** | **refused → granted** | **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. **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 --arg` — `rc=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 suite** — `bash 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. ``` ### 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. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Summary
Fixes #2227. When
git rev-parse --show-object-formatfails,repo_oid_widthno longer falls back to a cached width of 0 that makes every literal full-width SHA look like a movable lease.Verification
bash plugins/guardrails/hooks/block-dangerous-git.test.sh— PASS=383 FAIL=0Related