Repository navigation
fix(guardrails): narrow block-hook-bypass to a single plain shell write - #6708
Closed
kyle-sexton wants to merge 9 commits into
Closed
kyle-sexton wants to merge 9 commits into
kyle-sexton wants to merge 9 commits into
Conversation
…ry targets A cat, echo, printf or staged-move write to a non-ignored path in the cwd's repository is checked after the fact by check-bash-file-changes, so blocking it up front only duplicates that check. These stay-quiet cases fail against the current hook; the must-fire cases beside them pin what the shell check cannot see (ignored and .git targets, targets outside the repository or in a nested one, unplaceable targets, the python and PowerShell lanes). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EFqTQRnCGywRxhv4nw4o4V
…ge check cannot see check-bash-file-changes runs the Write/Edit content guards on every non-ignored file a shell command changed in the cwd's repository, so the cat, echo/printf and staged-move lanes blocking such a target only duplicated it. Those lanes now leave an unquoted literal target to that check when it resolves, through symlinks, inside the cwd's repository, outside .git and any nested repository, and git does not ignore it. An ignored, .git, outside or unplaceable target still blocks, as does every write while the check is off or has no data directory. The python and PowerShell lanes are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EFqTQRnCGywRxhv4nw4o4V
Member
Author
|
Security review (fresh-context reviewer; each command below exits 0 on this branch and 2 on main, and F2-F4 were also run end to end through check-bash-file-changes): block. Held for the owner.
|
…ty review broke Each command from the security review (F1-F7) exits 0 against the branch's repository axis and must block: a write followed by git add/commit/push, a redirect form other than plain > or >> (&>, 1<>, an fd-numbered write), a symlink or directory made in the same command, a .gitignore write or git init alongside the write, the shell check's file cap, a background run, a call with no pre-command snapshot, and the .git. alias. The staged-move stay-quiet case becomes a must-fire case, since a staged move is two commands. Stay-quiet cases for the single plain write, including here-docs and 2>&1 / 2>/dev/null, and a check that a granted write leaves the snapshot on disk, are added beside them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EFqTQRnCGywRxhv4nw4o4V
…ange check check-bash-file-changes runs after the whole command and only reports, so block-hook-bypass now leaves a write to it only when the command is that one write: a lone cat, echo or printf segment with one plain > or >> redirect (plus 2>&1, 2>/dev/null, a here-string or one here-doc whose unquoted body runs no substitution), no git, ln, mkdir, mv or cp word, and not run_in_background. The target must also not be a .gitignore or under a .git component, counting the .git. and trailing-space aliases Windows folds to .git, and the check's pre-command snapshot must be on disk; the guard takes it through the same script when the parallel snapshot hook has not yet, and refuses when no file results. Staged moves are two commands and are never granted, so that lane returns to main's form. The three payload fields are read in their own call on the grant path only and declared unprimed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EFqTQRnCGywRxhv4nw4o4V
The repository axis now grants only a target made of [A-Za-z0-9._/@+-], so brace expansion, commas, NTFS streams (`.git:stream`) and backslashes keep the block. An unquoted here-doc whose body has a line ending in `\` is refused: bash joins it into the terminator and runs the lines after it. The cat and echo/printf lanes now find stdout writes that never use a plain `>`: `&>`, `&>>`, `>& f`, `1<> f`, `3> f 1>&3` and `2> f 1>&2`, resolving redirects left to right; a discard that a dup overrides no longer exempts. Must-fire tests pin every category the reviewer could not test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EFqTQRnCGywRxhv4nw4o4V
…-hook-bypass # Conflicts: # plugins/guardrails/README.md
…reach Security re-review findings on the narrowed grant: - check-bash-file-changes judges a tracked file by its added lines whatever its size, diffs with --text so a .gitattributes -diff or NUL byte cannot hide them, and reports a new file it cannot read (over 1 MiB or NUL). - block-hook-bypass refuses .gitattributes, agent/editor/hook/workflow config targets (.claude/, .husky/, .vscode/, .idea/, .github/workflows/, .mcp.json, .envrc), and an existing target with more than one hard link. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EFqTQRnCGywRxhv4nw4o4V
…st-check misses The third security review of the narrowed block-hook-bypass found three gaps: - X1: an earlier call can hide a tracked file's change from the shell file-change check with an assume-unchanged or skip-worktree bit or a git filter attribute. The repository axis now refuses such a target, read from git ls-files -v and git check-attr filter. - X2: the config list now covers .github/actions/, .gitmodules, .devcontainer/ and .cursor/, matching each component with trailing dots and spaces stripped so Windows aliases such as .claude./ count. - X3: check-bash-file-changes reads a new file holding a NUL byte with each NUL as a line break, so its content is checked rather than only noted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EFqTQRnCGywRxhv4nw4o4V
Member
Author
|
Closing per owner decision (2026-10-10, option A): drop the narrowing and keep main's block-hook-bypass. Four security review rounds each found a new P2+ bypass of the same class: earlier-command git state (attributes, config, index bits, working-tree-encoding) changes what git status/diff report to the post-check, so a detective post-check does not converge. The reporting improvements landed separately in #6710. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #6689
Summary
check-bash-file-changes(#6690) runs guardrails' Write/Edit content guards on every non-ignored file a Bash or PowerShell command changed in the cwd's repository. It runs after the whole command and only reports. The security review on this PR showed that leaving any command with a matching target to that check let the rest of the command act before the report arrived: it could push the file, redirect the write somewhere else, hide it, or push it past the check's limits. Owner decision (option b): narrowblock-hook-bypassonly for a single-segment shell file write, and refuse every bypass the review found.Fix
The repository axis (
repo_target_seen) now leaves a write to the shell check only when all of these hold. Anything else keeps today's block._bbh_single_write, decided once per hook run):cat,echoorprintf(no prefix word, so noexec,envorcoproc);;,&,|,(,)or backtick on the command line once2>&1is removed. That rules out&&,||, pipes,&>, subshells, and command or process substitution. Quoted text containing these characters refuses too, which keeps today's block;$(or a backtick;>/>>, plus optionally2>&1,2>/dev/null, a here-string, or one here-doc.&>,1<>, any fd-numbered write and any dup onto stdout clear the segment (F2);git,ln,mkdir,mvorcp(F1, F3, F4);tool_input.run_in_backgroundis nottrue(F5).git check-ignoretests, plus:.gitafter case folding and stripping trailing dots and spaces, the way Windows does. So.git.,.GIT..and a.gitwith a trailing space all count (F7);.gitignoretarget refuses (F4).check-bash-file-changes.mjs snapshotwhen the file is not already on disk. It refuses when no non-empty, non-symlink snapshot file results, for example when there is notool_use_idor the data directory cannot be written.parse_mv_cp_operandsand the segment model go back to main's form (SEG_WR,MOVE_DEST_RAWandMOVE_DEST_DIRare removed)..tool_input.run_in_background,.session_idand.tool_use_idare read in their ownhook::jq_fieldscall, only on the grant path, and are declared inGUARD_FIELDS_UNPRIMED..gitignore, no snapshot. The header comment, README row and Scope notes, and the changelog fragment (patch) describe the single-write design. They also say that a granted write skips other plugins' and the user's own Write/Edit hooks, which the check does not re-run.Verification
Each review example is now a must-fire test. Against the pre-fix branch head (476a03e) every one of them exits 0 and fails the test:
cat > config.py <<'EOF' && git add config.py && git commit -m x && git push+ body/EOF(the review's one-line form as bash runs it), the multi-line variant,cat > tmp && mv tmp src/f(the old stay-quiet case, now must-fire),gitas an argument word, an unquoted here-doc running$(git push), a command after the terminatorcat > ok.txt &> ~/.bashrc,echo x > ok.txt 1<>~/.bashrc,cat 3> ~/.bashrc > ok.txt 1>&3, two stdout targetsln -s ~/.bashrc lnk && cat > lnk,mkdir -p newdir && echo x > newdir/fecho evil.sh >> .gitignore && cat > evil.sh,echo evil.sh >> .gitignorealone,git init -q sub && echo x > sub/ftouch a{01..20} && echo P > zz.sh,echo x > newfile.txt && rm newfile.txt,run_in_background: truetool_use_id,CLAUDE_PLUGIN_DATApointing at a filecat > .git./hooks/pre-commit,echo x > .GIT../config(plus.git\ /…, which was already blocked)Pre-fix head run:
PASS=856 FAIL=28(every F test, the snapshot-written check, and the new reason assertions). After the fix:block-hook-bypass.test.shreportsPASS=884 FAIL=0.The stay-quiet cases still pass:
cat > docs/x.md,echo x > newfile.txt,printf a >> README.md, an absolute in-repo target, and direct/dispatched parity. New stay-quiet cases also pass: a quoted here-doc containing$(date), a<<-here-doc with a trailing newline,2>/dev/null, and2>&1. One more test asserts that a granted call leaves its snapshot file on disk.scripts/affected-tests.sh --run --jobs 8: every guardrails suite passes, includingrun-guards.test.sh(PASS=652),check-bash-file-changes.test.sh, the PS differential and the killswitch hoist. The two selected Node suites pass.scripts/check-script-contract.test.shfails 2check-html-assets.shcases becausehtmlhintis not innode_modulesin a fresh worktree; this change does not touch that. shellcheck is clean,shfmt -i 2is clean, and markdownlint is clean. These checks are also clean: check-hook-slow-shapes, check-silent-skips, check-discriminating-test-skips, check-pipefail-grep-q, check-fixture-git-isolation, check-shell-portability, check-changelog-fragments.Residuals:
Related
check-bash-file-changes🤖 Generated with Claude Code
https://claude.ai/code/session_01EFqTQRnCGywRxhv4nw4o4V