feat(sandbox): harden Bash write-path detection + sandbox enforcement#24
Open
feat(sandbox): harden Bash write-path detection + sandbox enforcement#24
Conversation
Greptile SummaryThis PR hardens the Bash write-path detection layer in muzzle and adds OS-level sandbox awareness. It introduces 10 new regex patterns to catch in-place file editors ( Key changes:
Open finding: Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Bash tool call] --> B{session active?}
B -- No --> Z[Allow]
B -- Yes --> C[check_worktree_enforcement\ngit -C / cd path checks]
C -- Blocked --> DENY1[Deny: git op on main checkout]
C -- OK --> D[check_bash_write_paths]
D --> E[RE_REDIRECT / RE_TEE\nabsolute + relative paths]
D --> F[RE_SED_INPLACE / RE_PERL / RE_RUBY\nextract_file_args multi-file]
D --> G[RE_CP / RE_MV / RE_INSTALL / RE_RSYNC\nextract_copy_dest last-arg dest]
D --> H[RE_DD_OF / RE_PATCH\ncapture group / extract_file_args]
E & F & G & H --> I[push_write_path\nskip SCP, rsync://, rel: prefix]
I --> J{path prefix?}
J -- absolute --> K[check_path_with_context\nFileTool context]
J -- rel: prefix --> L{worktree active?}
J -- gitc: prefix --> M[is_system_path check only]
L -- Yes --> N[resolve CWD + path\ncheck_path_with_context]
L -- No --> Z
K -- Deny --> DENY2[Deny: main checkout path]
N -- Deny --> DENY3[Deny: relative path in worktree]
K & N -- Allow --> Z
style DENY1 fill:#f66
style DENY2 fill:#f66
style DENY3 fill:#f66
style Z fill:#6f6
Reviews (24): Last reviewed commit: "feat(sandbox): harden Bash write-path de..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
frits-v
force-pushed
the
worktree-e5961ded
branch
from
a5f97f3 to
b71c209
Compare
Owner
Author
|
@greptile review |
1 similar comment
Owner
Author
|
@greptile review |
Owner
Author
|
@greptile review |
Owner
Author
|
@greptile review |
Owner
Author
|
@greptile review |
Owner
Author
|
@greptile review |
Owner
Author
|
@greptile review |
Owner
Author
|
@greptile review |
Owner
Author
|
@greptile review |
frits-v
force-pushed
the
worktree-e5961ded
branch
from
b100fbb to
b149144
Compare
Owner
Author
|
@greptile review |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds sandbox documentation and settings guidance; expands Bash write-path detection to many tools and relative paths; enforces Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Bash)
participant GitCheck as gitcheck parser
participant Permissions as Permissions Hook
participant Sandbox as OS Sandbox / sandbox.rs
participant Output as Context/Session Output
User->>GitCheck: Run bash command (possible write ops)
GitCheck->>GitCheck: Extract write-targets (abs, rel:, cp/mv/sed/dd/patch, etc.)
GitCheck->>Permissions: Push normalized write-paths (`abs` / `rel:`)
Permissions->>Permissions: If `rel:` and worktree_active -> resolve CWD
alt path resolved
Permissions->>Sandbox: check_path_with_context (ToolContext::Bash)
Sandbox-->>Permissions: allow / deny / ask
Permissions->>Output: Emit deny or prompt if deny/ask
else resolution failed
Permissions->>Output: Deny and emit worktree-missing message
end
sequenceDiagram
participant SessionHook as Session Start Hook
participant FS as Settings Files (user/global, local, project)
participant Output as Context/Session Output
SessionHook->>FS: Read/parse settings.json files (best-effort)
FS-->>SessionHook: Parsed sandbox.enabled / allowUnsandboxedCommands (if present)
alt sandbox.enabled == true
alt allowUnsandboxedCommands == true
SessionHook->>Output: Emit warning about unsandboxed commands enabled
else
SessionHook->>Output: Emit sandbox-enabled context
end
else
SessionHook->>Output: Emit "SANDBOX NOT ENABLED" guidance (regex fallback)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
frits-v
force-pushed
the
worktree-e5961ded
branch
from
d3e43c3 to
e7202ed
Compare
Owner
Author
|
@greptile review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/src/bin/permissions.rs`:
- Around line 217-231: The current is_rel branch short-circuits sandbox checks
and directly returns Decision::Deny under an active worktree; instead resolve
the effective absolute path for the relative target and pass that resolved path
into sandbox::check_path_with_context so allowlisted relative targets (e.g.
.agents, .claude-tmp) are evaluated by the same logic as absolute paths.
Specifically, inside the if is_rel block (referencing is_rel,
sess.has_session(), sess.worktree_active, actual_path, and
muzzle::worktree_missing_msg) compute the base/resolved path for actual_path,
then call sandbox::check_path_with_context(...) with the resolved path and
return its Decision result rather than unconditionally returning Decision::Deny.
Ensure the muzzle::worktree_missing_msg branch is preserved for error messaging
when resolution fails.
In `@hooks/src/bin/session_start.rs`:
- Around line 179-180: The sandbox guidance is skipped when handle_startup()
returns early on worktree-creation failure; move the call to
check_sandbox_enabled() so it runs before the early-return branch (i.e., call
check_sandbox_enabled() immediately after handle_startup() returns and before
inspecting result.failed), or alternatively incorporate the sandbox guidance
text into the same failure handling block where result.failed is processed;
update the logic around handle_startup(), result.failed, and
check_sandbox_enabled() accordingly so the sandbox message is always emitted on
that failure path.
- Around line 650-657: The project-level settings entries in the settings_paths
array (currently PathBuf::from(".claude/settings.json") and
PathBuf::from(".claude/settings.local.json") in session_start.rs) are resolved
relative to the current working directory and will miss repo/worktree-local
configs when the hook runs from a nested directory; update the logic that builds
settings_paths (or the function that computes them) to locate project settings
by resolving the repository/workspace root (e.g., via git worktree/repo
discovery) or by walking parent directories up to filesystem root and adding the
first-found .claude/settings*.json paths, replacing the direct
PathBuf::from(".claude/...") entries so project-level settings are always found
regardless of the current CWD.
In `@hooks/src/gitcheck.rs`:
- Around line 326-385: The redirect/tee handling misses relative writes because
RE_REDIRECT and RE_TEE only match absolute paths and matched redirect/tee
targets are pushed directly (no "rel:" prefix), so update the regexes
RE_REDIRECT and RE_TEE to allow relative paths (e.g., optional leading / or
relative patterns like [./]?), and change the redirect/tee capture handling to
route targets through push_write_path (or, if keeping direct pushes, prefix
relative targets with "rel:") so permissions.rs will recognize them; use
existing helpers like extract_file_args or the same logic used for
sed/perl/ruby/patch to extract targets and call push_write_path for each match.
- Around line 428-435: The current truncation happens because after_tool is
split on raw separators (['|',';','<','>'] and "&&") without regard for quoted
strings; replace the two-step .split(...) calls with a quote-aware boundary
detection helper (e.g., a new function like find_first_unquoted_separator or
split_unquoted_segment) that scans after_tool char-by-char tracking
single/double-quote state and backslashes and returns the first separator index
not inside quotes or escapes, then use that index to produce segment; update
usages replacing the existing segment assignment (the two lines that create
segment from after_tool) to call this helper so sed/perl commands keep their
quoted parts and the real write target is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc02a1ac-d654-45dc-9ac5-2d66871a05f4
📒 Files selected for processing (6)
CLAUDE.mddocs/sandbox.mdhooks/src/bin/permissions.rshooks/src/bin/session_start.rshooks/src/gitcheck.rshooks/src/lib.rs
frits-v
force-pushed
the
worktree-e5961ded
branch
from
e7202ed to
f405158
Compare
Owner
Author
|
@greptile review |
Owner
Author
|
@coderabbitai review |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
hooks/src/gitcheck.rs (2)
424-431:⚠️ Potential issue | 🟠 MajorQuote-unaware splitting truncates commands with
|or;in patterns.The split on
['|', ';', '<', '>']doesn't respect quoted strings, sosed -i 's|old|new|' file.rsgets truncated to-i 'sbefore the filename can be extracted. The actual write target is lost.This was flagged in a previous review. A quote-aware boundary finder is needed to avoid splitting inside quoted sed/perl expressions.
,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/gitcheck.rs` around lines 424 - 431, The current naive split of after_tool (used to compute segment) cuts inside quoted expressions like sed -i 's|old|new|' and loses the target filename; replace the split logic in gitcheck.rs that builds segment by implementing a quote-aware boundary finder that scans after_tool char-by-char, tracking single- and double-quote state and escaping, and only treats |, ;, <, > and the && sequence as separators when not inside quotes or escaped (then return the substring up to that boundary). Update the subsequent "&&" handling to use the same quote-aware logic (or incorporate && into the scanner) so commands containing quoted operators are preserved; keep variable names segment and after_tool so callers remain recognizable and add unit tests for sed/perl-style expressions to verify correct filename extraction.
67-70:⚠️ Potential issue | 🟠 MajorRedirect and tee regexes still only capture absolute paths.
The
RE_REDIRECTandRE_TEEpatterns require a leading/, so relative redirect targets likeecho x > src/lib.rsortee src/lib.rsare not captured. These paths bypasspush_write_path()entirely and won't receive therel:prefix thatpermissions.rsexpects.This appears to be the same gap flagged in a previous review. Consider updating the capture groups to allow relative paths:
Proposed regex changes
static RE_REDIRECT: LazyLock<Regex> = - LazyLock::new(|| Regex::new(r"[12]?>>?\s*(/[^\s;|&)]+)").unwrap()); + LazyLock::new(|| Regex::new(r"[12]?>>?\s*([^\s;|&)]+)").unwrap()); static RE_TEE: LazyLock<Regex> = - LazyLock::new(|| Regex::new(r"\btee\s+(?:-a\s+)?(/[^\s]+)").unwrap()); + LazyLock::new(|| Regex::new(r"\btee\s+(?:-a\s+)?([^\s;|&]+)").unwrap());,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/gitcheck.rs` around lines 67 - 70, RE_REDIRECT and RE_TEE only match absolute paths (leading "/") so relative targets like "echo x > src/lib.rs" or "tee src/lib.rs" are missed; update the Regex constructors for RE_REDIRECT and RE_TEE to accept relative paths (allow optional "./" or "../" or no leading slash) while still excluding shell delimiters (;|&) and whitespace, so push_write_path() sees those targets and permissions.rs gets the rel: prefix; locate RE_REDIRECT and RE_TEE in gitcheck.rs and modify their capture groups to permit relative path forms (and preserve existing exclusion of metacharacters).hooks/src/bin/session_start.rs (1)
653-658:⚠️ Potential issue | 🟡 MinorProject-level settings still resolved from CWD, not repo root.
The paths
.claude/settings.jsonand.claude/settings.local.json(lines 656-657) are relative to the current working directory. When the hook runs from a nested directory (e.g.,src/submodule/), these project-local settings won't be found, potentially causing a false "SANDBOX NOT ENABLED" warning.Consider discovering the repository root (e.g., via
git rev-parse --show-toplevel) or walking parent directories to locate project-level settings.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/bin/session_start.rs` around lines 653 - 658, The project-local settings in the settings_paths array are constructed relative to the current working directory, so when the hook runs from a nested folder it misses repo-level files; update the code that builds settings_paths (the settings_paths variable) to resolve project-local entries against the repository root instead of CWD—implement a repo root discovery (e.g., run "git rev-parse --show-toplevel" or walk parent directories in a helper like find_repo_root()) and then construct the two project paths as repo_root.join(".claude/settings.json") and repo_root.join(".claude/settings.local.json"); leave the home-based entries as-is and use the discovered repo root if available, falling back to CWD only if repo root cannot be determined.
🧹 Nitpick comments (1)
hooks/src/gitcheck.rs (1)
467-480: Sed/perl expression filtering heuristic looks reasonable but has edge cases.The 3-slash threshold distinguishes sed expressions like
s/foo/bar/from paths likeb/src/lib.rs(2 slashes). However, paths with 3+ directory levels that start with a single letter (e.g.,a/b/c/file.rs) could be incorrectly filtered.The test at lines 1629-1643 validates 1-2 slash paths work, but consider adding a test for deeper relative paths like
a/b/c/d.rsto confirm they're not misidentified as sed expressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/gitcheck.rs` around lines 467 - 480, Add a unit test in hooks/src/gitcheck.rs that exercises the sed/perl filtering branch (the cleaned.starts_with('/') && cleaned.ends_with('/') check and the single-letter-prefix heuristic that uses cleaned.as_bytes()[0] and slash_count) with deeper relative paths such as "a/b/c/d.rs" and "x/y/z/file.rs" to ensure they are not treated as sed expressions and thus are allowed; reference the same logic block around the cleaned variable and the single-letter-prefix + slash_count check so the test fails if the heuristic incorrectly filters 3+ level relative paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@hooks/src/bin/session_start.rs`:
- Around line 653-658: The project-local settings in the settings_paths array
are constructed relative to the current working directory, so when the hook runs
from a nested folder it misses repo-level files; update the code that builds
settings_paths (the settings_paths variable) to resolve project-local entries
against the repository root instead of CWD—implement a repo root discovery
(e.g., run "git rev-parse --show-toplevel" or walk parent directories in a
helper like find_repo_root()) and then construct the two project paths as
repo_root.join(".claude/settings.json") and
repo_root.join(".claude/settings.local.json"); leave the home-based entries
as-is and use the discovered repo root if available, falling back to CWD only if
repo root cannot be determined.
In `@hooks/src/gitcheck.rs`:
- Around line 424-431: The current naive split of after_tool (used to compute
segment) cuts inside quoted expressions like sed -i 's|old|new|' and loses the
target filename; replace the split logic in gitcheck.rs that builds segment by
implementing a quote-aware boundary finder that scans after_tool char-by-char,
tracking single- and double-quote state and escaping, and only treats |, ;, <, >
and the && sequence as separators when not inside quotes or escaped (then return
the substring up to that boundary). Update the subsequent "&&" handling to use
the same quote-aware logic (or incorporate && into the scanner) so commands
containing quoted operators are preserved; keep variable names segment and
after_tool so callers remain recognizable and add unit tests for sed/perl-style
expressions to verify correct filename extraction.
- Around line 67-70: RE_REDIRECT and RE_TEE only match absolute paths (leading
"/") so relative targets like "echo x > src/lib.rs" or "tee src/lib.rs" are
missed; update the Regex constructors for RE_REDIRECT and RE_TEE to accept
relative paths (allow optional "./" or "../" or no leading slash) while still
excluding shell delimiters (;|&) and whitespace, so push_write_path() sees those
targets and permissions.rs gets the rel: prefix; locate RE_REDIRECT and RE_TEE
in gitcheck.rs and modify their capture groups to permit relative path forms
(and preserve existing exclusion of metacharacters).
---
Nitpick comments:
In `@hooks/src/gitcheck.rs`:
- Around line 467-480: Add a unit test in hooks/src/gitcheck.rs that exercises
the sed/perl filtering branch (the cleaned.starts_with('/') &&
cleaned.ends_with('/') check and the single-letter-prefix heuristic that uses
cleaned.as_bytes()[0] and slash_count) with deeper relative paths such as
"a/b/c/d.rs" and "x/y/z/file.rs" to ensure they are not treated as sed
expressions and thus are allowed; reference the same logic block around the
cleaned variable and the single-letter-prefix + slash_count check so the test
fails if the heuristic incorrectly filters 3+ level relative paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a283bc9b-25a3-49c3-bb93-0a7258e0f259
📒 Files selected for processing (6)
CLAUDE.mddocs/sandbox.mdhooks/src/bin/permissions.rshooks/src/bin/session_start.rshooks/src/gitcheck.rshooks/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (2)
- hooks/src/lib.rs
- docs/sandbox.md
frits-v
force-pushed
the
worktree-e5961ded
branch
from
f405158 to
5d812a6
Compare
Owner
Author
|
@greptile review |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
hooks/src/gitcheck.rs (2)
294-307:⚠️ Potential issue | 🔴 CriticalRelative
>/teewrites still never reachpush_write_path().These loops now support
rel:targets, butRE_REDIRECTandRE_TEEstill only match unquoted absolute paths. Commands likeecho hi > src/lib.rs,tee src/lib.rs, or quoted targets never enter this block, so the main-checkout write is missed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/gitcheck.rs` around lines 294 - 307, The redirect and tee capture loops using RE_REDIRECT and RE_TEE only match unquoted absolute paths so relative or quoted targets (e.g., "src/lib.rs" or > src/lib.rs) never reach push_write_path; update the matching so those loops also consider relative and quoted operands by either extending RE_REDIRECT and RE_TEE to include relative-path and quoted-path alternatives or add companion regexes (e.g., RE_REDIRECT_REL / RE_TEE_REL) and run the same captures over cmd, then call push_write_path(&mut paths, p) with the trimmed capture; keep the same variable names (cmd, caps, m, p) and behavior to ensure all write targets are collected.
424-431:⚠️ Potential issue | 🔴 CriticalDon't split on raw
|/;inside quoted edit programs.
extract_file_args()still truncatessed -i 's|old|new|' file.rsandperl -i -pe 's/a/b/; s/c/d/' file.rsat the separator inside the quoted script, so the real filename is never parsed. This leaves a straightforward in-place-edit bypass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/gitcheck.rs` around lines 424 - 431, The current truncation happens because extract_file_args() splits the command stored in after_tool using .split(['|',';','<','>']) and .split("&&") without regard for quotes, so separators inside single- or double-quoted scripts (e.g., sed -i 's|old|new|' file.rs) are treated as command boundaries; fix by replacing that naive split with a quote-aware splitter: implement or call a helper (e.g., split_outside_quotes or parse_until_unquoted_separator) that scans after_tool, tracking single- and double-quote state and only treating |, ;, <, > and && as separators when they occur outside quotes, then assign the first returned segment to segment (preserve the existing fallback behavior using unwrap_or).hooks/src/bin/session_start.rs (1)
653-657:⚠️ Potential issue | 🟠 MajorResolve project settings from the workspace root, not
cwd.
PathBuf::from(".claude/settings*.json")is still relative to the process working directory. Starting the hook from a nested directory will miss project-local sandbox settings and emit a falseSANDBOX NOT ENABLEDwarning. Walk parents or resolve the repo/worktree root before adding these paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/bin/session_start.rs` around lines 653 - 657, The settings_paths array is building project-local paths relative to the process cwd (PathBuf::from(".claude/...")) which misses sandbox settings when the hook runs from a nested dir; change the code that constructs settings_paths (the settings_paths variable in session_start.rs) to resolve a repository/worktree or workspace root first (e.g., via git2::Repository::discover or by walking parents for a marker like .git/Cargo.toml) and then use workspace_root.join(".claude/settings.json") and workspace_root.join(".claude/settings.local.json") instead of PathBuf::from("..."); ensure home-based paths (home.join(...)) remain and add the repo-root-based paths so project-local settings are found regardless of cwd.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/sandbox.md`:
- Around line 30-52: The example settings.json's filesystem.allowWrite entries
conflict with the documented path-resolution rule (unprefixed paths resolve
relative to the settings file), so update the snippet and emitted template in
session_start.rs to use explicit workspace-root or absolute paths (or else
prefix with the correct relative path) — specifically change entries like
".worktrees", ".agents", ".claude-tmp", "CLAUDE.md" in the sample to a clear
absolute or workspace-root form (e.g., "/path/to/workspace/.worktrees" or
"<workspace>/.worktrees") and/or add a clarifying note in the docs to avoid
users creating a broken allowlist; also mirror this fix in the template emitted
by hooks/src/bin/session_start.rs where the same snippet is produced.
In `@hooks/src/gitcheck.rs`:
- Around line 360-363: The current loop using RE_RSYNC calls
extract_copy_dest(cmd, m.end()) which assumes cp/mv-style flags (treating -t as
destination and requiring ≥2 non-option args), causing rsync to misidentify -t
and install to miss single-arg -d invocations; update the parsing so
extract_copy_dest can distinguish tool kind or add separate parsers for
rsync/install: change calls in the RE_RSYNC handling to invoke a new
rsync-specific extractor (or pass a ToolKind enum) that ignores -t/--times and
treats the final non-option arg as the destination, and for install handle -d as
a mode that accepts a single path argument; ensure push_write_path(&mut paths,
&dest) is still called with the corrected dest from the specialized extractor.
---
Duplicate comments:
In `@hooks/src/bin/session_start.rs`:
- Around line 653-657: The settings_paths array is building project-local paths
relative to the process cwd (PathBuf::from(".claude/...")) which misses sandbox
settings when the hook runs from a nested dir; change the code that constructs
settings_paths (the settings_paths variable in session_start.rs) to resolve a
repository/worktree or workspace root first (e.g., via
git2::Repository::discover or by walking parents for a marker like
.git/Cargo.toml) and then use workspace_root.join(".claude/settings.json") and
workspace_root.join(".claude/settings.local.json") instead of
PathBuf::from("..."); ensure home-based paths (home.join(...)) remain and add
the repo-root-based paths so project-local settings are found regardless of cwd.
In `@hooks/src/gitcheck.rs`:
- Around line 294-307: The redirect and tee capture loops using RE_REDIRECT and
RE_TEE only match unquoted absolute paths so relative or quoted targets (e.g.,
"src/lib.rs" or > src/lib.rs) never reach push_write_path; update the matching
so those loops also consider relative and quoted operands by either extending
RE_REDIRECT and RE_TEE to include relative-path and quoted-path alternatives or
add companion regexes (e.g., RE_REDIRECT_REL / RE_TEE_REL) and run the same
captures over cmd, then call push_write_path(&mut paths, p) with the trimmed
capture; keep the same variable names (cmd, caps, m, p) and behavior to ensure
all write targets are collected.
- Around line 424-431: The current truncation happens because
extract_file_args() splits the command stored in after_tool using
.split(['|',';','<','>']) and .split("&&") without regard for quotes, so
separators inside single- or double-quoted scripts (e.g., sed -i 's|old|new|'
file.rs) are treated as command boundaries; fix by replacing that naive split
with a quote-aware splitter: implement or call a helper (e.g.,
split_outside_quotes or parse_until_unquoted_separator) that scans after_tool,
tracking single- and double-quote state and only treating |, ;, <, > and && as
separators when they occur outside quotes, then assign the first returned
segment to segment (preserve the existing fallback behavior using unwrap_or).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4aecff38-f0f8-487c-b63f-9b05ee7b3f3f
📒 Files selected for processing (6)
CLAUDE.mddocs/sandbox.mdhooks/src/bin/permissions.rshooks/src/bin/session_start.rshooks/src/gitcheck.rshooks/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (2)
- hooks/src/bin/permissions.rs
- hooks/src/lib.rs
frits-v
force-pushed
the
worktree-e5961ded
branch
from
5d812a6 to
14da595
Compare
Owner
Author
|
@greptile review |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
hooks/src/bin/session_start.rs (1)
653-658:⚠️ Potential issue | 🟠 MajorProject-level sandbox settings are still CWD-relative.
PathBuf::from(".claude/settings.json")only finds project settings when the hook starts at the repo/worktree root. If startup happens from a nested directory, this misses project-local sandbox config and can emit a falseSANDBOX NOT ENABLEDwarning. Please resolve these paths from the repo/worktree root or walk parent directories first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/bin/session_start.rs` around lines 653 - 658, The array assigned to settings_paths uses PathBuf::from(".claude/...") which is CWD-relative and will miss project-level settings when started from a nested directory; update the code that builds settings_paths (the settings_paths variable in session_start.rs) to resolve those project-local entries from the repository/worktree root (e.g., repo_root.join(".claude/settings.json") and repo_root.join(".claude/settings.local.json")) or implement a parent-walk that searches upward from the current working dir to find the repo/worktree root (stop at a .git or workspace marker) and use the discovered root to construct the PathBufs so project sandbox settings are found regardless of startup directory.hooks/src/bin/permissions.rs (1)
217-249:⚠️ Potential issue | 🔴 CriticalRelative write targets still bypass the sandbox outside the active-worktree case.
This branch only resolves
rel:paths whensess.worktree_activeis true. In no-worktree sessions—and with no session at all—it hitscontinuebefore the sharedcheck_path_with_context(...)call, so relative targets skip FR-WE-2 plus the normal system/home/workspace checks. A command likesed -i README.mdafter worktree creation fails can still write into the main checkout.Proposed fix
if is_rel { - // Relative path from a file-mutating command (sed -i, cp, mv, etc.). - // When worktrees are active, resolve against CWD (main checkout) and - // run through sandbox checks so allowlisted paths (.agents/, .claude-tmp/) - // are permitted while other main-checkout writes are blocked. - if sess.has_session() && sess.worktree_active { - // Resolve relative path against CWD to get an absolute path - if let Ok(cwd) = std::env::current_dir() { - let resolved = cwd.join(actual_path); - let resolved_str = resolved.to_string_lossy(); - let result = sandbox::check_path_with_context( - &resolved_str, - Some(&sess), - sandbox::ToolContext::Bash, - ); - match result { - sandbox::PathDecision::Deny(reason) => return Decision::Deny(reason), - sandbox::PathDecision::Ask(reason) => return Decision::Ask(reason), - sandbox::PathDecision::Allow => {} - } - } else { - // Can't resolve CWD — deny to be safe - return Decision::Deny(format!( - "BLOCKED: File-mutating Bash command targets main checkout \ - path '{}'. {}", - actual_path, - muzzle::worktree_missing_msg("(detected from Bash)") - )); - } - continue; - } - // No worktree active — can't resolve relative path, allow through + // Resolve relative targets against CWD so they go through the same + // sandbox policy as absolute paths. + if let Ok(cwd) = std::env::current_dir() { + let resolved = cwd.join(actual_path); + let resolved_str = resolved.to_string_lossy(); + let result = sandbox::check_path_with_context( + &resolved_str, + Some(&sess), + sandbox::ToolContext::Bash, + ); + match result { + sandbox::PathDecision::Deny(reason) => return Decision::Deny(reason), + sandbox::PathDecision::Ask(reason) => return Decision::Ask(reason), + sandbox::PathDecision::Allow => {} + } + } else if sess.has_session() { + return Decision::Deny(format!( + "BLOCKED: Cannot resolve current directory for relative write target '{}'.", + actual_path + )); + } continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/bin/permissions.rs` around lines 217 - 249, For is_rel paths, ensure we always run sandbox::check_path_with_context against the resolved CWD path instead of skipping the check when sess.worktree_active is false or when there is no session; change the logic in permissions.rs so: attempt to resolve std::env::current_dir() for all is_rel cases, call sandbox::check_path_with_context(&resolved_str, Some(&sess), sandbox::ToolContext::Bash) and handle PathDecision::{Deny,Ask,Allow} the same way, and only on failure to resolve CWD return a denial (use the existing muzzle::worktree_missing_msg only when sess.has_session() && sess.worktree_active, otherwise return a generic deny mentioning actual_path); keep references to is_rel, actual_path, sess.has_session(), sess.worktree_active, sandbox::check_path_with_context, and Decision::{Deny,Ask,Allow}.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/src/bin/session_start.rs`:
- Around line 143-145: check_sandbox_enabled() currently writes a JSON object to
stdout separately from handle_startup()/output_worktree_paths(), violating the
single-JSON SessionStart contract; change check_sandbox_enabled() to return its
JSON payload (or an Option/Result containing the JSON) instead of printing,
update handle_startup() and the worktree-creation error path to collect that
returned sandbox JSON together with the worktree info from
output_worktree_paths(), and finally emit one combined JSON object into
hookSpecificOutput.additionalContext (ensuring this happens even when
early-returning on worktree failure).
---
Duplicate comments:
In `@hooks/src/bin/permissions.rs`:
- Around line 217-249: For is_rel paths, ensure we always run
sandbox::check_path_with_context against the resolved CWD path instead of
skipping the check when sess.worktree_active is false or when there is no
session; change the logic in permissions.rs so: attempt to resolve
std::env::current_dir() for all is_rel cases, call
sandbox::check_path_with_context(&resolved_str, Some(&sess),
sandbox::ToolContext::Bash) and handle PathDecision::{Deny,Ask,Allow} the same
way, and only on failure to resolve CWD return a denial (use the existing
muzzle::worktree_missing_msg only when sess.has_session() &&
sess.worktree_active, otherwise return a generic deny mentioning actual_path);
keep references to is_rel, actual_path, sess.has_session(),
sess.worktree_active, sandbox::check_path_with_context, and
Decision::{Deny,Ask,Allow}.
In `@hooks/src/bin/session_start.rs`:
- Around line 653-658: The array assigned to settings_paths uses
PathBuf::from(".claude/...") which is CWD-relative and will miss project-level
settings when started from a nested directory; update the code that builds
settings_paths (the settings_paths variable in session_start.rs) to resolve
those project-local entries from the repository/worktree root (e.g.,
repo_root.join(".claude/settings.json") and
repo_root.join(".claude/settings.local.json")) or implement a parent-walk that
searches upward from the current working dir to find the repo/worktree root
(stop at a .git or workspace marker) and use the discovered root to construct
the PathBufs so project sandbox settings are found regardless of startup
directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1cfba6b0-a42c-4198-a7ad-f5e3c16068c2
📒 Files selected for processing (6)
CLAUDE.mddocs/sandbox.mdhooks/src/bin/permissions.rshooks/src/bin/session_start.rshooks/src/gitcheck.rshooks/src/lib.rs
✅ Files skipped from review due to trivial changes (3)
- docs/sandbox.md
- CLAUDE.md
- hooks/src/gitcheck.rs
frits-v
force-pushed
the
worktree-e5961ded
branch
from
14da595 to
e726fe9
Compare
Owner
Author
|
@greptile review |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hooks/src/bin/session_start.rs (1)
275-283:⚠️ Potential issue | 🟠 MajorMultiple
emit_context()calls produce multiple JSON objects.The function calls
emit_context()from multiple execution paths:
check_sandbox_enabled()(lines 693–697 or 703–729) — always executeshandle_startup()failure path (lines 159–162)output_worktree_paths()(line 270)According to Claude Code's SessionStart hook protocol, stdout must contain only a single JSON object. Multiple JSON objects cannot be parsed correctly by Claude Code.
In a successful startup,
check_sandbox_enabled()emits one JSON object, thenoutput_worktree_paths()emits a second. In a failed startup, two are emitted before returning. Both scenarios violate the protocol.Collect all context strings and emit a single combined JSON object at the end of
handle_startup().Also applies to: 693–699, 703–729
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/bin/session_start.rs` around lines 275 - 283, Multiple calls to emit_context produce multiple JSON objects on stdout; change the flow so context is collected and emitted once at the end of handle_startup. Update emit_context (or replace it) to return a String (or push to a central Vec<String>) instead of printing; modify callers check_sandbox_enabled, output_worktree_paths, and the handle_startup error path to append their context messages to that accumulator; finally, after handle_startup completes (success or failure), build a single JSON object (same shape currently produced by emit_context) combining all collected context strings (e.g., join or put them in an array) and print it exactly once. Ensure all existing call sites (emit_context calls in check_sandbox_enabled, output_worktree_paths, and handle_startup) are updated to use the new accumulator API so no direct prints occur before the final emit.
🧹 Nitpick comments (1)
hooks/src/bin/permissions.rs (1)
239-244: Consider simplifying the error message for CWD resolution failure.The error message combines a custom prefix with
worktree_missing_msg("(detected from Bash)"). The resulting output will have two separate messages concatenated, which may be verbose. However, this provides maximum context for debugging, so it's acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/bin/permissions.rs` around lines 239 - 244, The Deny message in Decision::Deny currently concatenates a custom prefix with muzzle::worktree_missing_msg("(detected from Bash)"), producing a verbose two-part message; update the formatted string in the Decision::Deny call to produce a single concise message that still includes actual_path and the worktree context (call muzzle::worktree_missing_msg("(detected from Bash)") inline or substitute a shorter phrase), e.g., combine into one format string so the output is not duplicated or overly verbose while retaining the same contextual symbols (Decision::Deny, actual_path, muzzle::worktree_missing_msg).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@hooks/src/bin/session_start.rs`:
- Around line 275-283: Multiple calls to emit_context produce multiple JSON
objects on stdout; change the flow so context is collected and emitted once at
the end of handle_startup. Update emit_context (or replace it) to return a
String (or push to a central Vec<String>) instead of printing; modify callers
check_sandbox_enabled, output_worktree_paths, and the handle_startup error path
to append their context messages to that accumulator; finally, after
handle_startup completes (success or failure), build a single JSON object (same
shape currently produced by emit_context) combining all collected context
strings (e.g., join or put them in an array) and print it exactly once. Ensure
all existing call sites (emit_context calls in check_sandbox_enabled,
output_worktree_paths, and handle_startup) are updated to use the new
accumulator API so no direct prints occur before the final emit.
---
Nitpick comments:
In `@hooks/src/bin/permissions.rs`:
- Around line 239-244: The Deny message in Decision::Deny currently concatenates
a custom prefix with muzzle::worktree_missing_msg("(detected from Bash)"),
producing a verbose two-part message; update the formatted string in the
Decision::Deny call to produce a single concise message that still includes
actual_path and the worktree context (call
muzzle::worktree_missing_msg("(detected from Bash)") inline or substitute a
shorter phrase), e.g., combine into one format string so the output is not
duplicated or overly verbose while retaining the same contextual symbols
(Decision::Deny, actual_path, muzzle::worktree_missing_msg).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 423bf632-2345-44f3-95b7-7b7c590f84a8
📒 Files selected for processing (6)
CLAUDE.mddocs/sandbox.mdhooks/src/bin/permissions.rshooks/src/bin/session_start.rshooks/src/gitcheck.rshooks/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- hooks/src/gitcheck.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- CLAUDE.md
Close the Edit-hook bypass vector where Claude used sed -i/cp/mv via Bash to modify main checkout files after the Edit tool was denied. Write-path detection: - 10 new regex patterns for in-place editors (sed -i, perl -i, ruby -i) and file copy/move commands (cp, mv, install, rsync, dd of=, patch) - Detect relative path writes with rel: prefix for worktree enforcement - Handle combined flags (-ni.bak, -pie, -w -i), backup suffixes (-i.bak) - Handle --target-directory=value, --in-place long form, sudo/env prefix - extract_file_args returns all file targets, not just the last one - find_iter scans all command occurrences in multi-stage pipelines - Pass RE_* match offsets to extractors to avoid tool-in-filename mismatch - Split on > redirect to prevent masking of real write targets - Skip SCP remote destinations (user@host:path) and rsync daemon URLs - Skip bare numeric tokens from fd redirects (2>/dev/null) - Distinguish single-char dir paths (b/src/lib.rs) from sed expressions - Fix quoted absolute path guard to only skip sed address expressions Sandbox enforcement: - Check sandbox.enabled on session start with last-write-wins semantics - Warn when allowUnsandboxedCommands is true (default) - Prescriptive WORKTREE_MISSING deny messages naming bypass vectors - Defense-in-depth documentation in docs/sandbox.md Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
frits-v
force-pushed
the
worktree-e5961ded
branch
from
e726fe9 to
e5a741f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
hooks/src/gitcheck.rs (2)
356-364:⚠️ Potential issue | 🔴 CriticalUse tool-specific destination parsing for
rsyncandinstall.The shared
extract_copy_dest()logic still appliescp/mvrules to every caller. That breaksrsync -t src/ dest/(-tis preserve-times, not target-directory) and it completely missesinstall -d hooks/newdir, which is a write with only one path operand.Also applies to: 499-561
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/gitcheck.rs` around lines 356 - 364, The current loop uses extract_copy_dest() (which applies cp/mv rules) for RE_INSTALL and RE_RSYNC matches, causing rsync flags like -t to be mis-parsed and missing single-operand install -d semantics; update the handling so RE_RSYNC matches call a new rsync-specific dest parser (e.g., parse_rsync_dest(cmd, m.end())) that understands rsync flags and trailing-slash semantics before calling push_write_path, and make RE_INSTALL matches use an install-specific parser (e.g., parse_install_dest(cmd, m.end())) that treats a single operand with -d as a write target; replace calls to extract_copy_dest in the loops around RE_RSYNC and RE_INSTALL (and the similar block at lines ~499-561) with these tool-specific parsers and keep push_write_path usage unchanged.
431-438:⚠️ Potential issue | 🔴 CriticalMake command-boundary detection quote-aware again.
Lines 435-438 and Line 504 still split on raw
|,;,<,>, and&&without respecting quotes. Commands likesed -i 's|old|new|' hooks/src/gitcheck.rsorperl -i -pe 's/a/b/; s/c/d/' hooks/src/gitcheck.rsget truncated before the filename, so no write target is recorded.Also applies to: 503-507
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/gitcheck.rs` around lines 431 - 438, The current logic that computes segment from after_tool (the two chained .split calls that split on ['|',';','<','>'] and "&&") is not quote-aware and will truncate commands containing those characters inside quotes; replace these raw .split calls with a quote-aware scan that finds the first unquoted pipeline/sequence/redirection operator and returns the slice up to that operator (implement a helper like find_first_unquoted_delim or split_unquoted that walks after_tool, tracks single/double-quote state and escapes, and returns the substring before the first unquoted '|', ';', '<', '>', or the two-char '&&'); update both uses around the segment computation (the code that uses after_tool -> segment and the similar occurrence later) to call that helper so write-targets inside quoted sed/perl expressions are not lost.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/src/gitcheck.rs`:
- Around line 67-70: RE_REDIRECT currently only matches >, >>, 1>, 2> and misses
&>, &>>, and >|; update the static RE_REDIRECT regex to match all Bash redirect
forms (numeric or ampersand targets, single or double >, and the >| operator)
and continue to capture the filename/target group (e.g., change to a pattern
that allows an optional digit or '&' prefix and accounts for >> and >|). RE_TEE
only captures a single operand; update the static RE_TEE regex to match the tee
command with optional flags and capture all subsequent non-shell-token targets
(e.g., capture a space-separated list of targets so callers can split them and
inspect each file). Ensure you update the LazyLock initializers for RE_REDIRECT
and RE_TEE accordingly (preserve names RE_REDIRECT and RE_TEE) so all redirect
forms like "&>", "&>>", ">|" and multiple tee targets are detected.
- Around line 91-106: The anchored command-start regexes RE_CP, RE_MV,
RE_INSTALL, RE_RSYNC, and RE_PATCH only treat ^, |, ||, &&, and ; as command
separators and thus miss commands after a newline or a single ampersand; update
each pattern's left-side alternation from (?:^|\|{1,2}|&&|;\s*) to include
newline and single & as separators (e.g., (?:^|\n|\|{1,2}|&&|&|;\s*)) so the
regex will match commands that start after a newline or after a backgrounded
single &; ensure you include the escaped '\n' in the string literals and
preserve the trailing \s* grouping and other existing prefixes like
(?:sudo\s+|env\s+)?.
---
Duplicate comments:
In `@hooks/src/gitcheck.rs`:
- Around line 356-364: The current loop uses extract_copy_dest() (which applies
cp/mv rules) for RE_INSTALL and RE_RSYNC matches, causing rsync flags like -t to
be mis-parsed and missing single-operand install -d semantics; update the
handling so RE_RSYNC matches call a new rsync-specific dest parser (e.g.,
parse_rsync_dest(cmd, m.end())) that understands rsync flags and trailing-slash
semantics before calling push_write_path, and make RE_INSTALL matches use an
install-specific parser (e.g., parse_install_dest(cmd, m.end())) that treats a
single operand with -d as a write target; replace calls to extract_copy_dest in
the loops around RE_RSYNC and RE_INSTALL (and the similar block at lines
~499-561) with these tool-specific parsers and keep push_write_path usage
unchanged.
- Around line 431-438: The current logic that computes segment from after_tool
(the two chained .split calls that split on ['|',';','<','>'] and "&&") is not
quote-aware and will truncate commands containing those characters inside
quotes; replace these raw .split calls with a quote-aware scan that finds the
first unquoted pipeline/sequence/redirection operator and returns the slice up
to that operator (implement a helper like find_first_unquoted_delim or
split_unquoted that walks after_tool, tracks single/double-quote state and
escapes, and returns the substring before the first unquoted '|', ';', '<', '>',
or the two-char '&&'); update both uses around the segment computation (the code
that uses after_tool -> segment and the similar occurrence later) to call that
helper so write-targets inside quoted sed/perl expressions are not lost.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8acd0e46-f3af-4010-a756-2055c98e79f2
📒 Files selected for processing (6)
CLAUDE.mddocs/sandbox.mdhooks/src/bin/permissions.rshooks/src/bin/session_start.rshooks/src/gitcheck.rshooks/src/lib.rs
✅ Files skipped from review due to trivial changes (2)
- CLAUDE.md
- docs/sandbox.md
🚧 Files skipped from review as they are similar to previous changes (2)
- hooks/src/lib.rs
- hooks/src/bin/permissions.rs
Comment on lines
67
to
+70
| static RE_REDIRECT: LazyLock<Regex> = | ||
| LazyLock::new(|| Regex::new(r"[12]?>>?\s*(/[^\s;|&)]+)").unwrap()); | ||
| LazyLock::new(|| Regex::new(r"[12]?>>?\s*([^\s;|&)]+)").unwrap()); | ||
| static RE_TEE: LazyLock<Regex> = | ||
| LazyLock::new(|| Regex::new(r"\btee\s+(?:-a\s+)?(/[^\s]+)").unwrap()); | ||
| LazyLock::new(|| Regex::new(r"\btee\s+(?:-a\s+)?([^\s;|&]+)").unwrap()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the gitcheck.rs file
cd hooks && head -100 src/gitcheck.rs | tail -40Repository: frits-v/muzzle
Length of output: 2445
🏁 Script executed:
# Let's also see the full context around lines 67-70 and 295-308
cd hooks && sed -n '60,80p' src/gitcheck.rsRepository: frits-v/muzzle
Length of output: 1311
🏁 Script executed:
# Check the tee processing logic
cd hooks && sed -n '295,320p' src/gitcheck.rsRepository: frits-v/muzzle
Length of output: 892
🏁 Script executed:
# Also look for the check_bash_write_paths function to understand how tee results are used
cd hooks && grep -n "check_bash_write_paths\|push_write_path" src/gitcheck.rs | head -20Repository: frits-v/muzzle
Length of output: 1368
Detect all Bash redirect operators and all tee output targets.
The RE_REDIRECT regex only matches >, >>, 1>, and 2>, missing &>, &>>, and >|. The RE_TEE regex captures only the first operand, skipping additional files. Examples: printf x &> hooks/src/gitcheck.rs, printf x >| hooks/src/gitcheck.rs, and echo x | tee /dev/null hooks/src/gitcheck.rs all escape detection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/src/gitcheck.rs` around lines 67 - 70, RE_REDIRECT currently only
matches >, >>, 1>, 2> and misses &>, &>>, and >|; update the static RE_REDIRECT
regex to match all Bash redirect forms (numeric or ampersand targets, single or
double >, and the >| operator) and continue to capture the filename/target group
(e.g., change to a pattern that allows an optional digit or '&' prefix and
accounts for >> and >|). RE_TEE only captures a single operand; update the
static RE_TEE regex to match the tee command with optional flags and capture all
subsequent non-shell-token targets (e.g., capture a space-separated list of
targets so callers can split them and inspect each file). Ensure you update the
LazyLock initializers for RE_REDIRECT and RE_TEE accordingly (preserve names
RE_REDIRECT and RE_TEE) so all redirect forms like "&>", "&>>", ">|" and
multiple tee targets are detected.
Comment on lines
+91
to
+106
| static RE_CP: LazyLock<Regex> = | ||
| LazyLock::new(|| Regex::new(r"(?:^|\|{1,2}|&&|;\s*)\s*(?:sudo\s+|env\s+)?cp\b").unwrap()); | ||
| static RE_MV: LazyLock<Regex> = | ||
| LazyLock::new(|| Regex::new(r"(?:^|\|{1,2}|&&|;\s*)\s*(?:sudo\s+|env\s+)?mv\b").unwrap()); | ||
| // Match standalone `install` utility only, not package managers (npm install, pip install, etc.). | ||
| // Require `install` at the start of a command segment (after |, &&, ;, or line start). | ||
| // Also match `sudo install` and `env install` for elevated-privilege invocations. | ||
| static RE_INSTALL: LazyLock<Regex> = | ||
| LazyLock::new(|| Regex::new(r"(?:^|\|{1,2}|&&|;\s*)\s*(?:sudo\s+|env\s+)?install\b").unwrap()); | ||
| static RE_RSYNC: LazyLock<Regex> = | ||
| LazyLock::new(|| Regex::new(r"(?:^|\|{1,2}|&&|;\s*)\s*(?:sudo\s+|env\s+)?rsync\b").unwrap()); | ||
| static RE_DD_OF: LazyLock<Regex> = | ||
| LazyLock::new(|| Regex::new(r"\bdd\b[^;|&]*\bof=([^\s;|&]+)").unwrap()); | ||
| // Anchor to command-start position to avoid matching inside git format-patch / --patch | ||
| static RE_PATCH: LazyLock<Regex> = | ||
| LazyLock::new(|| Regex::new(r"(?:^|\|{1,2}|&&|;\s*)\s*patch\b").unwrap()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n hooks/src/gitcheck.rs | sed -n '85,115p'Repository: frits-v/muzzle
Length of output: 2134
🏁 Script executed:
rg -A 10 -B 5 "RE_CP|RE_MV|RE_INSTALL|RE_RSYNC|RE_PATCH" hooks/src/gitcheck.rs | head -80Repository: frits-v/muzzle
Length of output: 3204
🏁 Script executed:
rg -l "test" hooks/src/gitcheck.rsRepository: frits-v/muzzle
Length of output: 79
🏁 Script executed:
find . -name "*.rs" -type f | xargs grep -l "test.*RE_CP\|test.*RE_MV\|test.*gitcheck" | head -5Repository: frits-v/muzzle
Length of output: 91
🏁 Script executed:
rg -A 5 -B 5 "multiline\|command.*separator\|newline.*command" hooks/src/gitcheck.rsRepository: frits-v/muzzle
Length of output: 40
🏁 Script executed:
rg -B 20 "pub fn check_git_safety" hooks/src/gitcheck.rs | head -30Repository: frits-v/muzzle
Length of output: 1384
🏁 Script executed:
cat -n hooks/tests/proptest_sandbox.rs | head -100Repository: frits-v/muzzle
Length of output: 4146
🏁 Script executed:
rg "check_git_safety" hooks/ -A 3 -B 3 | head -50Repository: frits-v/muzzle
Length of output: 3031
🏁 Script executed:
rg "GitMessage|git_command" hooks/ -A 2 -B 2 | head -80Repository: frits-v/muzzle
Length of output: 546
🏁 Script executed:
rg "check_git_safety" hooks/ --type rust -B 10 | grep -A 10 "fn\|cmd\|command"Repository: frits-v/muzzle
Length of output: 10932
🏁 Script executed:
find hooks -name "*.rs" -type f | xargs grep -l "check_git_safety\|GitMessage" | head -5Repository: frits-v/muzzle
Length of output: 140
🏁 Script executed:
rg -B 30 "check_git_safety" hooks/src/main.rs 2>/dev/null || echo "Not in main.rs"Repository: frits-v/muzzle
Length of output: 72
🏁 Script executed:
find hooks -name "*.rs" -type f -exec grep -l "main\|entry" {} \;Repository: frits-v/muzzle
Length of output: 482
🏁 Script executed:
rg "struct BashInput\|command:" hooks/src/bin/permissions.rs -A 5 -B 2Repository: frits-v/muzzle
Length of output: 40
🏁 Script executed:
rg "BashInput" hooks/ -B 5 -A 10 | head -80Repository: frits-v/muzzle
Length of output: 1804
🏁 Script executed:
cat > /tmp/test_bash.sh << 'EOF'
#!/bin/bash
# Test if newline and single & are command separators in Bash
# Test 1: Newline separator
echo "Test 1: newline separator"
echo "before"
cp /tmp/nonexistent1 /tmp/dest1
# Test 2: Single & (background)
echo "Test 2: single & (background)"
sleep 0 & cp /tmp/nonexistent2 /tmp/dest2
# Test 3: Combined with other separators
echo "Test 3: semicolon separator"
echo "before" ; cp /tmp/nonexistent3 /tmp/dest3
echo "Test 4: && separator"
echo "before" && cp /tmp/nonexistent4 /tmp/dest4
EOF
cat /tmp/test_bash.shRepository: frits-v/muzzle
Length of output: 555
🏁 Script executed:
python3 << 'PYEOF'
import re
# The regexes from gitcheck.rs
RE_CP = re.compile(r"(?:^|\|{1,2}|&&|;\s*)\s*(?:sudo\s+|env\s+)?cp\b")
RE_MV = re.compile(r"(?:^|\|{1,2}|&&|;\s*)\s*(?:sudo\s+|env\s+)?mv\b")
# Test cases
test_cases = [
("cp /tmp/a /tmp/b", True, "simple cp at start"),
("echo ok\ncp /tmp/a /tmp/b", False, "cp after newline (not matched!)"),
("sleep 0 & cp /tmp/a /tmp/b", False, "cp after single & (not matched!)"),
("echo ok; cp /tmp/a /tmp/b", True, "cp after semicolon"),
("echo ok && cp /tmp/a /tmp/b", True, "cp after &&"),
("echo ok | cp /tmp/a /tmp/b", True, "cp after |"),
("echo ok || cp /tmp/a /tmp/b", True, "cp after ||"),
]
print("Testing RE_CP regex:")
print("=" * 60)
for cmd, should_match, description in test_cases:
match = RE_CP.search(cmd)
matched = match is not None
status = "✓" if matched == should_match else "✗"
print(f"{status} {description}")
print(f" Command: {repr(cmd)}")
print(f" Expected: {should_match}, Got: {matched}")
if match:
print(f" Match: {repr(match.group())}")
print()
PYEOFRepository: frits-v/muzzle
Length of output: 848
🏁 Script executed:
# Check how BashInput.command is created - look for where bash commands come from
rg "BashInput\|tool_input" hooks/src/bin/permissions.rs -B 10 -A 10Repository: frits-v/muzzle
Length of output: 40
🏁 Script executed:
python3 << 'PYEOF'
# Verify Bash command separator semantics
print("Bash Command Separators (from POSIX/Bash spec):")
print("=" * 70)
print()
print("1. SEMICOLON (;)")
print(" - Separates commands sequentially")
print(" - Next command runs regardless of previous exit status")
print(" - Example: command1 ; command2")
print()
print("2. NEWLINE")
print(" - Acts as command separator in Bash")
print(" - Equivalent to semicolon in most contexts")
print(" - Example:")
print(" command1")
print(" command2")
print()
print("3. AMPERSAND (&)")
print(" - Single & backgrounds the command")
print(" - Command runs asynchronously, shell continues immediately")
print(" - Example: command1 & command2")
print()
print("4. AND (&&)")
print(" - Logical AND - runs next command only if previous succeeds")
print(" - Example: command1 && command2")
print()
print("5. OR (||)")
print(" - Logical OR - runs next command only if previous fails")
print(" - Example: command1 || command2")
print()
print("6. PIPE (|)")
print(" - Connects stdout of previous to stdin of next")
print(" - Example: command1 | command2")
print()
print("Conclusion: Newline and single & ARE valid Bash command separators.")
print("Commands after these separators can execute independently.")
PYEOFRepository: frits-v/muzzle
Length of output: 1056
🏁 Script executed:
rg "\\\\n|newline" hooks/src/gitcheck.rsRepository: frits-v/muzzle
Length of output: 347
🏁 Script executed:
cat -n hooks/src/gitcheck.rs | sed -n '60,90p'Repository: frits-v/muzzle
Length of output: 2073
Include newline and single & as command separators in the anchored tool patterns.
The regexes for cp, mv, install, rsync, and patch only anchor to ^, |, ||, &&, and ;. A multiline command like printf ok\ncp /tmp/fixed.rs hooks/src/gitcheck.rs or a backgrounded command like sleep 0 & mv /tmp/fixed.rs src/lib.rs will skip detection of the second command. This is inconsistent with other patterns in the file (e.g., RE_SED_INPLACE, RE_PERL_INPLACE) which already account for \n as a separator.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/src/gitcheck.rs` around lines 91 - 106, The anchored command-start
regexes RE_CP, RE_MV, RE_INSTALL, RE_RSYNC, and RE_PATCH only treat ^, |, ||,
&&, and ; as command separators and thus miss commands after a newline or a
single ampersand; update each pattern's left-side alternation from
(?:^|\|{1,2}|&&|;\s*) to include newline and single & as separators (e.g.,
(?:^|\n|\|{1,2}|&&|&|;\s*)) so the regex will match commands that start after a
newline or after a backgrounded single &; ensure you include the escaped '\n' in
the string literals and preserve the trailing \s* grouping and other existing
prefixes like (?:sudo\s+|env\s+)?.
Comment on lines
69
to
+70
| static RE_TEE: LazyLock<Regex> = | ||
| LazyLock::new(|| Regex::new(r"\btee\s+(?:-a\s+)?(/[^\s]+)").unwrap()); | ||
| LazyLock::new(|| Regex::new(r"\btee\s+(?:-a\s+)?([^\s;|&]+)").unwrap()); |
There was a problem hiding this comment.
tee only captures the first output file — subsequent targets evade detection
RE_TEE is \btee\s+(?:-a\s+)?([^\s;|&]+), which captures exactly one filename per match. The tee utility accepts any number of output files, and only the first is ever inspected.
A concrete bypass during a worktree session:
command | tee /safe/allowed/path.txt /main/checkout/src/lib.rsTrace:
RE_TEE.captures_iterfinds one match;caps.get(1)="/safe/allowed/path.txt"./safe/allowed/path.txtpassescheck_path_with_context→Allow./main/checkout/src/lib.rsis never inspected — the write proceeds undetected.
The same applies with -a (append mode):
cmd | tee -a .claude-tmp/log.txt /main/checkout/MakefileA fix would be to switch from a single capture group to a multi-file extraction loop, similar to extract_file_args:
// Instead of RE_TEE / captures_iter with a single group:
fn extract_tee_paths(cmd: &str) -> Vec<String> {
// find each `tee` invocation, collect all positional args
// (skip -a and any other option flags)
}No existing test covers the multi-file tee form, so this gap is currently invisible in the test suite.
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
sed -i/cp/mvvia Bash to modify main checkout files after the Edit tool was correctly deniedsed -i,perl -i,ruby -i) and file copy/move commands (cp,mv,install,rsync,dd of=,patch)rel:prefix — when worktrees are active, relative paths in file-mutating Bash commands are deniedcp $(which sed) .bin/zet)Defense in depth
Review rounds addressed
feat(sandbox)fix: cp -t bypass-t/--target-directorycapture, install false positives, dead codefix: false positivesfix: git mv, perl -w -ifix: vibe findings--target-directory=value, sandbox config last-write-wins, sed transliterate, sed -ni combined flagsfix: rsync remote, redirect bypass>redirect split, multi-file arg extraction,--in-placesed anchoringTest plan
cargo fmt --checkcleancargo clippy -- -D warningsclean🤖 Generated with Claude Code