fix(policy): normalize paths + scan Bash commands in policy guards - #28
Merged
Merged
Conversation
July 2026 audit found the policy guard layer shallower than the product
story. Three findings addressed:
1. PathRestriction bypass — the pre-tool guard did a raw startsWith on
the unnormalized tool-input path, and only for Edit/Write. Both the
guard and the post-hoc evaluator now compare lexically-normalized
paths (./ and ../ segments resolved with posix-normalize semantics,
no fs access so not-yet-existing files still work) case-insensitively
(macOS filesystems are case-insensitive by default), and NotebookEdit
is inspected via notebook_path wherever file_path was handled
(PathRestriction, FileLimitCount, BranchProtection, SecretDetection,
and the hook's tool-to-action mapping). Bash-mediated writes (tee,
> redirects) remain out of scope and are now documented as such at
the guard.
2. SecretDetection never saw Bash commands — the pre-tool guard now
scans the Bash command string (and NotebookEdit new_source); the
post-hoc evaluator scans cmd.command in addition to cmd.stdout; and
the hook now populates FileEdit.diff with the written content
(new_string / content / new_source, capped at 10000 chars) so
post-hoc re.test(edit.diff) can actually match on hook-produced
runs instead of always testing the empty string.
3. minCoverage removed from TestEnforcementConfig — it was declared,
persisted, and rendered in the dashboard ("min 80% coverage") but
never enforced, and no coverage number exists anywhere in the
domain model to enforce it against. Removed from the config type
and the dashboard summary so the UI stops claiming enforcement
that cannot happen; legacy rows carrying the key stay inert.
Verified end-to-end through the real CLI hook: traversal Write,
case-variant NotebookEdit, and secret-bearing Bash command all block
with exit 2; benign writes pass; post-tool-use persists the diff.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
Summary
Deepens the policy guard layer, which the July audit found bypassable in practice:
normalizePathForPolicy()(resolves..//./, collapses//, case-insensitive, no fs access so not-yet-existing files still match). Closes the/repo/src/../secrets/key.pemtraversal and macOS case bypasses.NotebookEdit(notebook_path) is now covered by PathRestriction, FileLimitCount, and BranchProtection guards and the hook's action mapping — it was previously a blanket bypass. Prefix-from-start semantics preserved and pinned by tests (.envmatches.env.local, notsrc/.env).Bash.commandandNotebookEdit.new_sourcein addition to Write/Edit content; the post-hoc evaluator scanscmd.commandas well as stdout. The hook now records the written content as the FileEdit diff (10k-char cap), so post-hoc secret detection can actually match on hook-produced runs (it previously always tested an empty string). Block messages never echo the matched secret (asserted).minCoverageremoved — no coverage number exists anywhere in the domain model (TestResult carries name/passed/duration/message only) and hooks always finalize with empty testResults, so the dashboard's "min N% coverage" summary advertised enforcement that could never happen. Removed from the config type and the policy summary; legacy DB rows carrying the key stay inert (pinned by tests in core and web).tee,>redirects,cp, heredocs) are not intercepted by PathRestriction; a code comment states this and points at RiskyOpFlag patterns as the mitigation.Tests
~42 new/reworked, including the first direct
evaluatePreToolPoliciessuite in core (traversal,./, case, NotebookEdit across four policy types, Bash secret scanning, disabled-policy skip). Verified end-to-end through the real hook binary with an isolated HOME: traversal Write, case-variant NotebookEdit, and secret-bearing Bash all block with exit 2; benign writes allowed. Full workspace suite green, web lint clean.🤖 Generated with Claude Code