fix(hooks): examine sensitive files reached via Bash; scope fixture exceptions (S4) - #893
Merged
Merged
Conversation
…xceptions (S4) check_sensitive_file ran only in the Write/Edit/Read tool branches, so the identical acts spelled as a shell command were never examined at all. Verified allowed before this change: `cat ~/.ssh/id_ed25519`, `cp ~/.env /var/www/html/`, `cat > ~/.env`, while the Write-tool equivalents were denied. Bash commands are now scanned per segment. The posture mirrors the tool branches rather than inventing a stricter one for Bash: copy/write verbs (cp, mv, scp, rsync, tee, dd, install, rclone) and output-redirect targets DENY, because they duplicate a secret to a new location or overwrite a credential file, which is exactly what Write/Edit deny; read verbs (cat, less, head, base64, ...) WARN only, matching the Read tool and the reason its docstring gives -- agents routinely read a project's own .env while debugging and blocking that breaks common legitimate work. _SENSITIVE_EXCEPTIONS matched `/fixtures/` anywhere in a path, so a real credential file at /home/feedgen/fixtures/.env was excused from every check. Directory exceptions (fixtures, testdata, __fixtures__, test_data) now apply only when the path resolves inside the repo worktree; suffix exceptions (.env.example and friends) still apply anywhere. Three existing tests asserted the unscoped behavior using out-of-repo paths; they now use in-repo paths and gained paired out-of-repo deny rows. Adds the credential stores the home CLAUDE.md names but the pattern list missed: .git-credentials, .netrc, ~/.config/gh/hosts.yml, .envrc. This extends the list S1 modified rather than replacing it. Adds 44 table-driven rows: every closed bypass, warn-vs-deny posture rows asserting the advisory actually fires, exception-scoping rows in both directions, and false-positive rows for ordinary work (cat README.md, cp README.md /tmp/r, echo hi > /tmp/out.txt, grep -r token .).
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.
Problem
Track S security audit, finding S4.
1. The sensitive-file guard never looked at Bash.
check_sensitive_filewas called only in theWrite/Edit/Readtool branches ofmain(). The identical acts spelled as a shell command were never examined. Verified ALLOWED before this change, while the Write-tool equivalents were denied:cat ~/.ssh/id_ed25519cp ~/.env /var/www/html/cat > ~/.env2. Fixture exceptions were unscoped.
_SENSITIVE_EXCEPTIONSmatched/fixtures/anywhere in a path, so a Write to/home/feedgen/fixtures/.env— a real credential file in the home directory — was excused from every check.3. Missing patterns. The home CLAUDE.md names
.git-credentials,.netrc,~/.config/gh/hosts.yml, and.envrcas protected; the pattern list had none of them.Fix
Bash scanning (
check_sensitive_bash), per segment, per non-heredoc line. The posture deliberately mirrors the existing tool branches rather than inventing a stricter one for Bash:cp,mv,scp,rsync,tee,dd,install,rclone) and output-redirect targets (>,>>). These duplicate a secret to a new location or overwrite a credential file, which is exactly whatWrite/Editdeny.cat,less,head,base64, …). Same as theReadtool, for the reason its own docstring gives: agents routinely read a project's own.envwhile debugging, and blocking that breaks common legitimate work for less benefit than the write block gives. Making Bashcatdeny while theReadtool warns would be incoherent.An unrecognized command's arguments are not assumed to be file paths, so this cannot spray false positives across ordinary commands.
~is expanded before matching — the patterns are rooted at/, so an un-expanded~/.envnever matched anything.Exception scoping. Split into suffix exceptions (
.env.example,.env.sample,.env.template— safe anywhere, they name placeholder files) and directory exceptions (fixtures,testdata,__fixtures__,test_data— honored only when the path resolves inside the repo worktree). A "this is test data" claim is only meaningful about a project's own tree. Repo root is found by walking up for.gitrather than shelling out to git, to stay inside the latency budget.Patterns. Extends
_SENSITIVE_PATTERNS(which S1 also modified) with the four missing credential stores.Existing tests changed
Three tests asserted the unscoped exception behavior using out-of-repo paths (
/project/fixtures/credentials.json). They now use in-repo paths and gained paired out-of-repo deny rows. This is the finding being fixed, not a weakened test.Tests
44 new table-driven rows in
hooks/tests/test_pretool_unified_gate_security.py: every closed bypass; warn-vs-deny rows that assert the advisory text actually fires (a WARN row would otherwise be indistinguishable from no coverage at all); exception scoping in both directions; the new patterns plus near-misses (.envrc.example,gh/config.yml) that must not match; and false-positive rows for ordinary work (cat README.md,cp README.md /tmp/r,echo hi > /tmp/out.txt,grep -r token .,ls -la ~/.ssh). A regression row confirms S1's.guard-whitelistpattern now also fires through the new Bash path.Gates
ruff checkclean,ruff format --checkclean (0.15.12),pytest hooks/tests/2204 passed,smoke-test-hooks --ci74/74 PASS,benchmark-hooks100.6ms (threshold 200ms).The fail-open error contract is unchanged.