Skip to content

fix(guardrails): sharpen delete and lease hints, block jq credential reads - #6654

Merged
kyle-sexton merged 3 commits into
mainfrom
fix/guardrails-g1-hints-credential-read
Oct 9, 2026
Merged

kyle-sexton merged 3 commits into
mainfrom
fix/guardrails-g1-hints-credential-read

Conversation

@kyle-sexton

Copy link
Copy Markdown
Member

No related issue: items B3m, B13, A13 and the B3c test from an approved guardrails performance-audit change set.

Summary

  • B3m block-root-delete-target: the outside-tree block names the allowed roots (block_root_delete_target_allowed_roots) and the session scratchpad first, says "do not retry the delete with another tool (find -delete, rmtree, git clean)", and keeps the user hand-off ("only the user can") as the last resort.
  • B3c test: a §2d case with a fake HOME and the key <H>/worktrees/.tmp-*,<H>/.work. <H>/worktrees/.tmp-x and <H>/.work/x are allowed. <H>/worktrees, <H>/worktrees/other, <H>/.work and <H> are refused. The test only; the guard already handled this (name-prefix entries shipped in feat(guardrails): allow name-prefix entries in block_root_delete_target_allowed_roots #6624).
  • B13 block-dangerous-git: the lease-width hint (unreadable hash format) is now git -C <repo> push --force-with-lease=<ref>:<full-sha> instead of "Run the push from inside the repository". The guard refuses that cd <repo> && git push form from a session root that is not a repository. The header comment that called it "the very form the block messages above prescribe" now names git -C instead. No parser change.
  • A13 block-credential-read: blocks cat/Get-Content/gc/type of .credentials.json and .docker/config.json, jq of either file, and jq env / jq '$ENV'. The pre-filter at the old line 128 now admits env (which covers the old \.env) and docker, so these shapes reach the matcher. README row updated.

Fix

Approved text where the plan corrected it:

  • A13 "jq readers": jq reads only the two JSON credential files, not every credential file. Matching jq against the whole list would block jq '.env' package.json, because a .env jq filter looks like a .env path. A stay-quiet test covers this.
  • jq env family: it goes under the existing env-echo allow-list token. A new token would need a plugin.json userConfig change, which is outside this PR's scope.

Test edits in .sh files went in through git apply. The edit-time shfmt hook re-indented every case arm in block-root-delete-target.sh (about 1,900 lines) to match .editorconfig switch_case_indent, and that churn was dropped. The diff holds only the intended lines.

Verification

  • block-credential-read.test.sh PASS=336 FAIL=0; block-dangerous-git.test.sh PASS=835 FAIL=0; block-root-delete-target.test.sh PASS=2213 FAIL=0.
  • scripts/affected-tests.sh --run: 29 shell suites passed. Its 2 node suites (lib/exec-bash.gates.test.mjs, lib/exec-bash.resolver.test.mjs) passed under node.
  • ShellCheck on the six changed .sh files: clean. check-hook-slow-shapes.sh: no slow shapes. check-purged-em-dashes.sh: none.
  • markdownlint-cli2, typos and editorconfig-checker on the changed files: clean. No TODO/FIXME markers were added.
  • check-changelog-fragments.sh --check and --check-required origin/main, check-changelog-parity.sh --check-bump origin/main, validate-plugins.sh: pass.

Related

  • Fragment: .changes/guardrails/fix-guardrails-g1-hints-credential-read-4ea448bd.md (bump: patch).
  • Per-machine block_root_delete_target_allowed_roots values live in dotfiles, not here.
  • B2 (block-hook-bypass) is a separate PR that does not touch these files.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VxGrZW26Qs1qeo3RFgmbvp

…reads

block-root-delete-target names the allowed roots and the scratchpad before
the user hand-off and says not to retry with another tool. block-dangerous-git
hints git -C <repo> for an unreadable hash format. block-credential-read
blocks jq and cat of .credentials.json and .docker/config.json and jq env.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VxGrZW26Qs1qeo3RFgmbvp
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 9, 2026 12:44
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-09T12:47:50.281961Z 4d07db1 Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Code review in progress

  • Gather context (PR diff, changed files, CLAUDE.md/REVIEW.md)
  • Review block-credential-read.sh/.test.sh (A13)
  • Review block-dangerous-git.sh/.test.sh (B13)
  • Review block-root-delete-target.sh/.test.sh (B3m/B3c)
  • Review README/changelog fragment
  • Adversarial verification of candidate findings
  • Post findings and summary

View job run

@claude

claude Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review in progress

  • Gather context (PR diff, changed files)
  • Review block-credential-read.sh, block-dangerous-git.sh, block-root-delete-target.sh diffs
  • Check instruction-exception-register for weakened guardrail text (none found — the diff only widens/strengthens messages)
  • Adversarially verify a candidate jq bypass finding
  • Post final findings

@claude

claude Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review in progress

  • Gather context (PR diff, changed files, CLAUDE.md/REVIEW.md)
  • Review block-credential-read.sh/.test.sh (A13) — static trace of new jq regexes
  • Review block-dangerous-git.sh/.test.sh (B13) — hint text and probe logic
  • Review block-root-delete-target.sh/.test.sh (B3m/B3c) — message wording, confirmed name-prefix logic predates this PR
  • Review README/changelog fragment
  • Adversarial verification of candidate findings (two hunter agents dispatched)
  • Post findings and summary

@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: 4d07db1c0e

ℹ️ 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-credential-read.sh Outdated
@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

claude-security-review has reviewed this pull request through b2fb143; a later push is reviewed from there.

@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

claude-review has reviewed this pull request through b2fb143; a later push is reviewed from there.

…ential guard

Strip --arg/--argjson operands and tell jq's case-sensitive $ENV builtin
from a $env variable before the jq shapes match, with stay-quiet tests for
both. Reorder the pre-filter so the prerequisite gate no longer reads
`docker ]]` as a docker run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VxGrZW26Qs1qeo3RFgmbvp
@claude

claude Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review complete

  • Gather context (incremental diff, REVIEW.md/AGENTS.md)
  • Review block-credential-read.sh (A13 --arg/--argjson stripping)
  • Review block-credential-read.test.sh (new cases)
  • Review the changelog fragment update
  • Adversarial verification (manual regex trace of the new sj stripping logic)
  • Post findings and summary

Scope

Incremental lane — reviewed only what changed since 4dc531d, confined to:

  • .changes/guardrails/fix-guardrails-g1-hints-credential-read-4ea448bd.md
  • plugins/guardrails/hooks/block-credential-read.sh
  • plugins/guardrails/hooks/block-credential-read.test.sh

Summary

This commit (b2fb143) fixes exactly the P2 false positive Codex flagged against the prior commit in its review comment on block-credential-read.sh: jq --arg env production '.environment = $env' package.json and jq --arg path ~/.claude/.credentials.json '.foo' package.json were being blocked because the matcher didn't distinguish --arg/--argjson operand values from actual jq filters/files.

I traced the new logic in block-credential-read.sh:188-197 by hand against both of Codex's repro commands plus several adversarial variants I constructed (an arg named env with a literal filter that still invokes the real env builtin afterward; $ENV smuggled inside a quoted --arg value; a credential path passed through --slurpfile/--rawfile, which the comment says intentionally stay matched since those genuinely read the named file):

  • jq --arg env production '.environment = $env' package.json → the stripping loop consumes exactly --arg env production (the NAME/VALUE pair), leaving .environment = $env as filter text, which correctly doesn't match re_jqenv (no bypass, no new false positive).
  • jq -n --arg x y '$ENV' (new test, line 126) → stripping only removes --arg x y, leaving the real $ENV builtin reference intact, so it still blocks. Confirms the fix doesn't let an unrelated --arg launder an actual env dump.
  • jq --arg x 1 env (constructed, not in the test file) → stripping removes only the NAME/VALUE pair (x 1), leaving the trailing env filter token, which still matches re_jqenv. No bypass via argument-count confusion.
  • jq --slurpfile c ~/.claude/.credentials.json '$c' (new test, line 122) → correctly still blocks, since --slurpfile/--rawfile are deliberately excluded from the strip pattern (--arg(json)? only) and jq genuinely opens that file.

The $ENV→$jq_env_builtin substitution is a plain substring replace done before lower-casing, so it only retags the literal uppercase builtin and leaves a real $env variable (lower-case) alone — consistent with jq's own case sensitivity and the updated changelog wording.

Tests added (block-credential-read.test.sh:123-124 for the two new blocked cases, :168-169 for the two new allowed cases) land in the right expected-exit buckets and match the PR body's claimed PASS=336 FAIL=0. I could not execute the shell test suite myself in this sandboxed lane (no shell-execution tool is in this lane's allowlist), so that pass count is author-claimed and unverified by me directly — my check was a manual trace of the regex/stripping logic against the new and constructed cases above, not a test run.

The changelog fragment's added sentence ("a $env jq variable and an --arg/--argjson value still pass") accurately describes the new behavior.

No correctness issues found in this incremental slice. No inline comments to post.

@claude

claude Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review complete

Incremental lane — scope was only the three files that changed since 4dc531d:
.changes/guardrails/fix-guardrails-g1-hints-credential-read-4ea448bd.md,
plugins/guardrails/hooks/block-credential-read.sh, and
plugins/guardrails/hooks/block-credential-read.test.sh. Every other file in
this PR was already reviewed at that commit.

  • Read .claude-lane/incremental.diff
  • Review block-credential-read.sh changes (jq --arg/--argjson operand stripping)
  • Review block-credential-read.test.sh changes
  • Review changelog fragment
  • Adversarial verification of candidate findings
  • Post final findings

No security issues found.

This increment is the fix for Codex's prior P2 finding (jq --arg env production '.environment = $env' package.json / jq --arg path ~/.claude/.credentials.json '.foo' package.json being over-blocked). The change builds a second derived string sj (block-credential-read.sh#L188-L197) that marks jq's $ENV builtin before lower-casing (so it stays distinguishable from a $env variable) and strips exactly the two tokens following --arg/--argjson before the jq-specific regexes run against it (#L207-L208, used at #L226 and #L231).

I traced it adversarially for a bypass (a command that genuinely reads one of the two credential files or dumps $ENV, but gets mis-stripped into passing):

  • A real positional jq --arg a b '.filter' ~/.claude/.credentials.json keeps the trailing file argument intact — the strip regex only consumes exactly --arg NAME VALUE (two tokens), not whatever follows — so re_jqfile still matches and it's still blocked.
  • --slurpfile/--rawfile, which do read a file, aren't matched by the strip pattern (--arg(json)? only) and stay subject to re_jqfile; the new test for --slurpfile confirms this still blocks.
  • A credential file reached through a nested shell ($(cat ~/.claude/.credentials.json) as an --arg value) is still caught independently by the pre-existing re_file match against $s, which this diff doesn't touch.
  • Chaining multiple --arg pairs can't smuggle a real file argument past the strip, because jq's own semantics mean a token immediately following a second --arg NAME is consumed as that arg's value, not opened as a file — so there's no case where "stripped as an arg value" and "actually opened by jq" are simultaneously true.
  • The one theoretical edge I found — a literal --arg x y substring embedded inside a quoted path segment could, in principle, get taken for an option by the flattened-string matcher — requires the attacker to control the literal name of a directory that happens to be an ancestor of ~/.claude/.credentials.json or ~/.docker/config.json on the target machine. That's not something a command string alone can arrange, so I'm not raising it as a finding.

Checked this against the instruction-exception register's secret-handling class: this narrows a hook's match precision, not the mechanism itself — the guard still blocks every genuine credential-file read and $ENV/jq env dump I could construct, and the new "stay-quiet" tests (jq --arg env production ... / jq --arg path ~/.claude/.credentials.json ... → 0) are exactly the discipline docs/conventions/hook-precision/README.md rule 2 asks for (ignore tokens inside quoted/option-operand positions rather than raw co-occurrence). No deletion of a protected rule, just a false-positive fix with regression coverage.

Changelog fragment text matches the behavior change.

@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 9, 2026
Merged via the queue into main with commit cab2e62 Oct 9, 2026
26 checks passed
@kyle-sexton
kyle-sexton deleted the fix/guardrails-g1-hints-credential-read branch October 9, 2026 13:24
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.

1 participant