fix(safety): block destructive git operations in shell blocklist#195
Merged
fix(safety): block destructive git operations in shell blocklist#195
Conversation
Sub-agents via DelegateTool get shell access by default. While the git tool only exposes safe operations (no push), agents could bypass it by running destructive git commands through shell. Add regex patterns blocking: force push, reset --hard, clean -f, checkout -- ., branch -D. Safe operations (push, pull, branch -d, reset --soft, merge) remain allowed. Closes #194 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded regex blocklist entries in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/security/shell.rs (1)
580-647: Add regression tests for option-token and case edge cases.Nice coverage overall, but add cases that lock in the intended behavior for the new patterns: bundled
-fu,git clean --force, mixed-caseGIT branch -D, and a safe branch name ending in-f.Suggested test additions
fn test_git_force_push_blocked() { let config = ShellSecurityConfig::new(); @@ assert!(config.validate_command("git push origin feat -f").is_err()); + assert!(config.validate_command("git push -fu origin main").is_err()); @@ fn test_git_clean_blocked() { let config = ShellSecurityConfig::new(); @@ assert!(config.validate_command("git clean -df").is_err()); + assert!(config.validate_command("git clean --force -d").is_err()); @@ fn test_git_branch_force_delete_blocked() { let config = ShellSecurityConfig::new(); assert!(config .validate_command("git branch -D feature-branch") .is_err()); + assert!(config + .validate_command("GIT branch -D feature-branch") + .is_err()); @@ fn test_safe_git_operations_allowed() { let config = ShellSecurityConfig::new(); @@ assert!(config.validate_command("git push origin main").is_ok()); + assert!(config.validate_command("git push origin release-f").is_ok());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/security/shell.rs` around lines 580 - 647, Add regression tests to cover option-token and case edge cases for ShellSecurityConfig::validate_command: add assertions that a bundled short-opts case like "git clean -fu" is rejected, that the long form "git clean --force" is rejected, that command name case-insensitivity is handled (e.g., "GIT branch -D feature-branch" is rejected), and that branch names ending in "-f" are allowed (e.g., "git branch -d feature-f" is ok); place these assertions alongside the existing tests (e.g., in test_git_clean_blocked, test_git_branch_force_delete_blocked, and test_safe_git_operations_allowed) so the new patterns are covered as regression checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/security/shell.rs`:
- Line 59: Remove the inline flag from the regex entry
r"(?-i)git\s+branch\s+.*-D\b" so it no longer disables case-insensitive
matching; update the blocklist pattern to r"git\s+branch\s+.*-D\b" (so it will
inherit the module's (?i) wrapping used when compiling with Regex::new) to
ensure commands like GIT branch -D are blocked consistently with the other git
patterns in the same blocklist.
- Around line 54-55: Update the regexes used to detect force-push and
branch-delete in src/security/shell.rs so they require an option token boundary
(a whitespace before the flag) and retain case-insensitive matching;
specifically, change the "git\s+push\s+.*-f\b" pattern to require a whitespace
before the flag (e.g., use "\s+-f(?:\s|$)" or "\s+--force(?:\s|$)" style) and
remove any (?-i) that disables case-insensitivity so the overall matcher remains
case-insensitive, and adjust the branch-delete pattern similarly to require
whitespace before "-D" or "--delete" and anchor or follow with space/end to
avoid matching branch-name suffixes.
---
Nitpick comments:
In `@src/security/shell.rs`:
- Around line 580-647: Add regression tests to cover option-token and case edge
cases for ShellSecurityConfig::validate_command: add assertions that a bundled
short-opts case like "git clean -fu" is rejected, that the long form "git clean
--force" is rejected, that command name case-insensitivity is handled (e.g.,
"GIT branch -D feature-branch" is rejected), and that branch names ending in
"-f" are allowed (e.g., "git branch -d feature-f" is ok); place these assertions
alongside the existing tests (e.g., in test_git_clean_blocked,
test_git_branch_force_delete_blocked, and test_safe_git_operations_allowed) so
the new patterns are covered as regression checks.
| r"git\s+reset\s+--hard", | ||
| r"git\s+clean\s+.*-[a-zA-Z]*f", | ||
| r"git\s+checkout\s+--\s+\.", | ||
| r"(?-i)git\s+branch\s+.*-D\b", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file structure and the specific line in question
head -n 70 src/security/shell.rs | tail -n 20Repository: qhkm/zeptoclaw
Length of output: 636
🏁 Script executed:
# Let's get more context around line 59
sed -n '40,80p' src/security/shell.rsRepository: qhkm/zeptoclaw
Length of output: 1245
🏁 Script executed:
# Let's understand the blocklist structure better
sed -n '1,100p' src/security/shell.rs | cat -nRepository: qhkm/zeptoclaw
Length of output: 4233
🏁 Script executed:
# Look for Regex compilation and usage
sed -n '100,200p' src/security/shell.rsRepository: qhkm/zeptoclaw
Length of output: 3437
🏁 Script executed:
# Search for how regexes are created/compiled
rg -A 5 -B 5 "Regex::new|RegexBuilder" src/security/shell.rsRepository: qhkm/zeptoclaw
Length of output: 763
🏁 Script executed:
# Look for tests that validate the shell security patterns
fd -e rs -type f src/security | xargs grep -l "test\|Test" | head -5Repository: qhkm/zeptoclaw
Length of output: 228
🏁 Script executed:
# Search for any tests related to shell security or the git branch pattern
rg -A 10 "git.*branch|GIT.*BRANCH" --type rs src/Repository: qhkm/zeptoclaw
Length of output: 84
🏁 Script executed:
# Let me check if there are integration tests or examples that show expected behavior
find . -type f -name "*.rs" | xargs rg -l "validate_command|ShellSecurityConfig" | head -10Repository: qhkm/zeptoclaw
Length of output: 229
🏁 Script executed:
# Check integration tests for shell security tests
rg -A 20 "shell|validate_command|git.*branch" tests/integration.rs | head -100Repository: qhkm/zeptoclaw
Length of output: 2773
🏁 Script executed:
# Let's verify the regex behavior manually by understanding how (?-i) works
# Create a simple test to understand the pattern behavior
python3 << 'EOF'
import re
# Simulate what happens with the current pattern
# The pattern in the code has (?-i) which disables case-insensitive mode
pattern_with_disable = r"(?i)(?-i)git\s+branch\s+.*-D\b" # Simulating how it's compiled with (?i) prefix
pattern_without_disable = r"(?i)git\s+branch\s+.*-D\b" # Without the (?-i)
suggested_fix = r"git\s+branch\b.*\s-(?-i:D)(?:\s|$)" # The suggested fix
# Test cases
test_commands = [
"git branch -D feature", # lowercase - should match
"GIT branch -D feature", # uppercase - should match if case-insensitive
"Git branch -D feature", # mixed case - should match if case-insensitive
"git BRANCH -D feature", # uppercase BRANCH - should match if case-insensitive
"git branch -d feature", # lowercase -d instead of -D - should NOT match
]
print("=== Testing current pattern WITH (?-i) in raw string ===")
current_pattern = re.compile(r"(?i)(?-i)git\s+branch\s+.*-D\b")
for cmd in test_commands:
match = current_pattern.search(cmd)
print(f"{cmd:30} -> {'MATCH' if match else 'NO MATCH'}")
print("\n=== Testing pattern WITHOUT (?-i) (all other patterns) ===")
standard_pattern = re.compile(r"(?i)git\s+branch\s+.*-D\b")
for cmd in test_commands:
match = standard_pattern.search(cmd)
print(f"{cmd:30} -> {'MATCH' if match else 'NO MATCH'}")
print("\n=== Testing suggested fix ===")
try:
suggested = re.compile(r"git\s+branch\b.*\s-(?-i:D)(?:\s|$)")
for cmd in test_commands:
match = suggested.search(cmd)
print(f"{cmd:30} -> {'MATCH' if match else 'NO MATCH'}")
except re.error as e:
print(f"ERROR compiling suggested fix: {e}")
EOFRepository: qhkm/zeptoclaw
Length of output: 1299
🏁 Script executed:
# Let me test with Rust's regex syntax which is what the code actually uses
# First, let's check the Rust regex crate version
grep -A 5 "regex" Cargo.toml | head -20Repository: qhkm/zeptoclaw
Length of output: 295
🏁 Script executed:
# Let's create a test file to verify the actual Rust regex behavior
cat > /tmp/test_regex.rs << 'EOF'
// Test to understand Rust regex inline flag behavior
fn main() {
use regex::Regex;
// Simulate what happens in ShellSecurityConfig::new()
let pattern_from_const = r"(?-i)git\s+branch\s+.*-D\b";
let compiled_pattern = format!("(?i){}", pattern_from_const); // This is what the code does
println!("Original pattern in const: {}", pattern_from_const);
println!("Compiled pattern: {}", compiled_pattern);
let regex = Regex::new(&compiled_pattern).expect("Invalid regex");
// Test cases
let test_cases = vec![
"git branch -D feature",
"GIT branch -D feature",
"Git Branch -D feature",
"git branch -d feature",
];
println!("\nMatching results:");
for test in test_cases {
let matches = regex.is_match(test);
println!("{:35} -> {}", test, if matches { "MATCH" } else { "NO MATCH" });
}
}
EOF
# Check if we can compile and run this
cd /tmp && rustc --edition 2021 test_regex.rs -L /root/.cargo 2>&1 | head -20Repository: qhkm/zeptoclaw
Length of output: 304
🏁 Script executed:
# Let me search for actual test cases that test the git branch pattern
rg "git.*branch|branch.*-D" tests/ --type rust -B 3 -A 3Repository: qhkm/zeptoclaw
Length of output: 40
🏁 Script executed:
# Let's check the Rust regex documentation/behavior for inline flags
# Create a simpler test to understand the pattern behavior without needing to compile
python3 << 'EOF'
# Let me document what happens with the pattern in Rust regex
# Based on Rust regex crate documentation:
# - (?i) enables case-insensitive mode for the rest of the pattern
# - (?-i) disables case-insensitive mode for the rest of the pattern
# - (?i:...) enables case-insensitive for just the content in parentheses
print("=== Analysis of Rust regex inline flags ===\n")
print("Pattern: r\"(?-i)git\\s+branch\\s+.*-D\\b\"")
print("As compiled by code: \"(?i)(?-i)git\\s+branch\\s+.*-D\\b\"\n")
print("How this works in Rust regex:")
print("1. (?i) at start -> enables case-insensitive mode")
print("2. (?-i) immediately after -> DISABLES case-insensitive mode")
print("3. Everything after (?-i) is now CASE-SENSITIVE")
print("4. So 'git' will NOT match 'GIT' or 'Git'")
print("5. And 'branch' will NOT match 'BRANCH' or 'Branch'")
print("\nResult: Patterns like 'GIT branch -D' will NOT be blocked\n")
print("=== Other patterns in the file (lines 54-58) ===")
other_patterns = [
r'git\s+push\s+.*--force',
r'git\s+push\s+.*-f\b',
r'git\s+reset\s+--hard',
r'git\s+clean\s+.*-[a-zA-Z]*f',
r'git\s+checkout\s+--\s+\.',
]
print("All other git patterns:")
for p in other_patterns:
print(f" {p}")
print("\nNone of these have (?-i), so they inherit (?i) from compilation")
print("Therefore they ARE case-insensitive and will match 'GIT', 'BRANCH', etc.\n")
print("=== The Suggested Fix Problem ===")
print("Suggested: r\"git\\s+branch\\b.*\\s-(?-i:D)(?:\\s|$)\"")
print("Issues:")
print("1. No (?i) or (?-i), so pattern is CASE-SENSITIVE")
print("2. Will NOT match 'GIT branch -D' (uppercase GIT)")
print("3. Will NOT match 'git BRANCH -D' (uppercase BRANCH)")
print("4. The (?-i:D) scoped flag only affects one character, not useful")
print("5. More restrictive with space requirement before - and after D\n")
print("=== The Real Issue ===")
print("The pattern at line 59 is INCONSISTENT with all other patterns")
print("All others use (?i) (case-insensitive), but this one uses (?-i) (case-sensitive)")
print("This is likely a BUG - either:")
print(" A) The (?-i) was added by mistake")
print(" B) Someone tried to fix something but broke it instead")
EOFRepository: qhkm/zeptoclaw
Length of output: 1544
🏁 Script executed:
# Let's verify there are no tests specifically for case variations of git branch -D
rg -i "git.*branch" tests/ -A 5 -B 5Repository: qhkm/zeptoclaw
Length of output: 40
🏁 Script executed:
# Let's check if there are any comments or issues explaining why (?-i) was added
rg -B 10 "(?-i)" src/security/shell.rsRepository: qhkm/zeptoclaw
Length of output: 23984
Remove the (?-i) inline flag that breaks case-insensitive matching.
The pattern at line 59 contains (?-i) which disables case-insensitive mode for the entire rest of the regex. Since Regex::new() wraps each pattern with (?i) (see line 117), the compiled pattern becomes (?i)(?-i)git\s+branch\s+.*-D\b, rendering it case-sensitive. This means commands like GIT branch -D feature will not be blocked, unlike all other destructive git patterns (lines 54–58) which remain case-insensitive.
Fix: Remove the (?-i) to make the pattern consistent with the rest of the blocklist:
Suggested fix
- r"(?-i)git\s+branch\s+.*-D\b",
+ r"git\s+branch\s+.*-D\b",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| r"(?-i)git\s+branch\s+.*-D\b", | |
| r"git\s+branch\s+.*-D\b", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/security/shell.rs` at line 59, Remove the inline flag from the regex
entry r"(?-i)git\s+branch\s+.*-D\b" so it no longer disables case-insensitive
matching; update the blocklist pattern to r"git\s+branch\s+.*-D\b" (so it will
inherit the module's (?i) wrapping used when compiling with Regex::new) to
ensure commands like GIT branch -D are blocked consistently with the other git
patterns in the same blocklist.
This was referenced Mar 1, 2026
- Force-push: require whitespace before flag to avoid false positives on branch names like `release-f` - Branch delete: remove `(?-i)` that broke case-insensitive matching, use scoped `(?-i:D)` to keep `-D` case-sensitive while allowing `GIT branch -D` to be caught - Clean: add `--force` long-form pattern - Add regression tests: bundled `-fu`, `git clean --force`, uppercase `GIT branch -D`, safe branch name `release-f` Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/security/shell.rs`:
- Around line 54-60: The blocklist regexes (e.g.
r"git\s+push\b.*\s--force(?:-with-lease)?(?:\s|$)",
r"git\s+push\b.*\s-[A-Za-z]*f[A-Za-z]*(?:\s|$)", r"git\s+reset\s+--hard",
r"git\s+clean\s+.*-[a-zA-Z]*f", r"git\s+clean\s+.*--force",
r"git\s+checkout\s+--\s+\.") only match when the subcommand immediately follows
"git", so first normalize the incoming git argv by consuming global git options
(e.g. -c, -C, --git-dir, --work-tree, -p, --namespace, etc.) before applying
destructive-command checks: implement a small parser that strips leading global
options and returns the real subcommand and its args, then run the existing
destructive regexes against the reconstructed "git <subcommand> <args>" string;
also tighten the checkout rule by changing the pattern
r"git\s+checkout\s+--\s+\." to require end-of-string or word-boundary (e.g.
r"git\s+checkout\s+--\s+\.$" or r"git\s+checkout\s+--\s+\.\b") so it doesn’t
match filenames like ".env".
- Line 59: The regex used to block "git checkout -- ." is too broad and blocks
dotfile restores; update the pattern used in the shell security check (the regex
literal currently matching git\s+checkout\s+--\s+\.) to instead only match the
all-files target by requiring the dot be end-of-token or followed by a separator
(use the suggested pattern that enforces \.($|[\s;|&/]) ); locate the regex in
src/security/shell.rs and replace the existing literal with the new stricter
pattern so commands like "git checkout -- .gitignore" are allowed while "git
checkout -- ." and "git checkout -- ./" remain blocked.
- Add normalize_git_command() that strips global git options (-C, -c, --git-dir, --work-tree, etc.) before pattern matching, preventing bypasses like `git -C /tmp push --force` - Tighten checkout pattern to require end-of-token after `.`, so `git checkout -- .gitignore` is allowed while `git checkout -- .` remains blocked - Add regression tests for both fixes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 1, 2026
This was referenced Mar 4, 2026
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
push --force/-f,reset --hard,clean -f,checkout -- .,branch -Dsrc/tools/git.rs) already blocks push/force-push, but sub-agents could bypass it through the shell toolTest plan
cargo fmt -- --checkcleancargo clippy -- -D warningscleanCloses #194
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests