-
Notifications
You must be signed in to change notification settings - Fork 2
Add command allowlist for AI remediation pipeline #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
73808c9
a747705
a84351a
a9d3214
e286de0
df60b6f
69c2412
f91fa62
b39d86b
12990c7
7e36bc8
1c3ee3e
b3aead7
632a431
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -287,10 +287,25 @@ else | |||||||||||||||||||||||||||||||||||||||||||||
| _non_ok_count="$(echo "$_non_ok_findings" | jq 'length')" | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| if (( _non_ok_count > 0 )); then | ||||||||||||||||||||||||||||||||||||||||||||||
| log_info "Piping $_non_ok_count findings to Claude for remediation..." | ||||||||||||||||||||||||||||||||||||||||||||||
| echo "$_non_ok_findings" | "$_claude_bin" -p \ | ||||||||||||||||||||||||||||||||||||||||||||||
| --allowedTools "Bash,Read,Write,Edit,Glob,Grep" \ | ||||||||||||||||||||||||||||||||||||||||||||||
| "You are a security remediation agent. You have been given ClawPinch security scan findings as JSON. For each finding: 1) Read the evidence to understand the issue 2) Apply the auto_fix command if available, otherwise implement the remediation manually 3) Verify the fix. Work through findings in order (critical first). Be precise and minimal in your changes." | ||||||||||||||||||||||||||||||||||||||||||||||
| # Pre-validate auto_fix commands: strip any that fail the allowlist | ||||||||||||||||||||||||||||||||||||||||||||||
| # so the AI agent only receives pre-approved commands | ||||||||||||||||||||||||||||||||||||||||||||||
| _validated_findings="[]" | ||||||||||||||||||||||||||||||||||||||||||||||
| for _idx in $(seq 0 $(( _non_ok_count - 1 ))); do | ||||||||||||||||||||||||||||||||||||||||||||||
| _finding="$(echo "$_non_ok_findings" | jq -c ".[$_idx]")" | ||||||||||||||||||||||||||||||||||||||||||||||
| _fix_cmd="$(echo "$_finding" | jq -r '.auto_fix // ""')" | ||||||||||||||||||||||||||||||||||||||||||||||
| if [[ -n "$_fix_cmd" ]] && ! validate_command "$_fix_cmd" 2>/dev/null; then | ||||||||||||||||||||||||||||||||||||||||||||||
| # Strip the disallowed auto_fix, keep finding for manual review | ||||||||||||||||||||||||||||||||||||||||||||||
| _finding="$(echo "$_finding" | jq -c '.auto_fix = "" | .remediation = (.remediation + " [auto_fix removed: command not in allowlist]")')" | ||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Validation errors suppressed. The Remove the stderr redirect so users can see validation failure reasons, or capture the error message and include it in the warning. Prompt To Fix With AIThis is a comment left during a code review.
Path: clawpinch.sh
Line: 305:305
Comment:
Validation errors suppressed. The `validate_command` call uses `2>/dev/null` to silence stderr, which hides the root cause when validation fails (missing config file, python3 not found, JSON parse errors). Users will only see the warning on line 306 that auto_fix was stripped, but won't understand why.
Remove the stderr redirect so users can see validation failure reasons, or capture the error message and include it in the warning.
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||||||||||||||||||||
| log_warn "Stripped disallowed auto_fix from finding $(echo "$_finding" | jq -r '.id')" | ||||||||||||||||||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||||||||||||||||||
| _validated_findings="$(echo "$_validated_findings" "[$_finding]" | jq -s '.[0] + .[1]')" | ||||||||||||||||||||||||||||||||||||||||||||||
| done | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
| _validated_findings="[]" | |
| for _idx in $(seq 0 $(( _non_ok_count - 1 ))); do | |
| _finding="$(echo "$_non_ok_findings" | jq -c ".[$_idx]")" | |
| _fix_cmd="$(echo "$_finding" | jq -r '.auto_fix // ""')" | |
| if [[ -n "$_fix_cmd" ]] && ! validate_command "$_fix_cmd" 2>/dev/null; then | |
| # Strip the disallowed auto_fix, keep finding for manual review | |
| _finding="$(echo "$_finding" | jq -c '.auto_fix = "" | .remediation = (.remediation + " [auto_fix removed: command not in allowlist]")')" | |
| log_warn "Stripped disallowed auto_fix from finding $(echo "$_finding" | jq -r '.id')" | |
| fi | |
| _validated_findings="$(echo "$_validated_findings" "[$_finding]" | jq -s '.[0] + .[1]')" | |
| done | |
| _validated_findings_array=() | |
| while IFS= read -r _finding; do | |
| _fix_cmd="$(echo "$_finding" | jq -r '.auto_fix // ""')" | |
| if [[ -n "$_fix_cmd" ]] && ! validate_command "$_fix_cmd" 2>/dev/null; then | |
| # Strip the disallowed auto_fix, keep finding for manual review | |
| _finding="$(echo "$_finding" | jq -c '.auto_fix = "" | .remediation = (.remediation + " [auto_fix removed: command not in allowlist]")')" | |
| log_warn "Stripped disallowed auto_fix from finding $(echo "$_finding" | jq -r '.id')" | |
| fi | |
| _validated_findings_array+=("$_finding") | |
| done < <(echo "$_non_ok_findings" | jq -c '.[]') | |
| _validated_findings="$(IFS=,; echo "[${_validated_findings_array[*]}]")" |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instructions tell Claude not to use Bash, but don't explain how to handle auto_fix commands that are shell scripts. Consider clarifying the expected workflow.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: clawpinch.sh
Line: 307:308
Comment:
Instructions tell Claude not to use Bash, but don't explain how to handle `auto_fix` commands that are shell scripts. Consider clarifying the expected workflow.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -121,6 +121,118 @@ require_cmd() { | |
| fi | ||
| } | ||
|
|
||
| # ─── Command validation (allowlist) ───────────────────────────────────────── | ||
|
|
||
| validate_command() { | ||
| # Usage: validate_command <command_string> | ||
| # Returns 0 if ALL commands in the string are in allowlist, 1 otherwise | ||
| local cmd_string="$1" | ||
|
|
||
| if [[ -z "$cmd_string" ]]; then | ||
| log_error "validate_command: empty command string" | ||
| return 1 | ||
| fi | ||
|
|
||
| # Find security config file (walk up from cwd to root) | ||
| local security_file="" | ||
| local dir | ||
| dir="$(pwd)" | ||
| while true; do | ||
| if [[ -f "$dir/.auto-claude-security.json" ]]; then | ||
| security_file="$dir/.auto-claude-security.json" | ||
| break | ||
| fi | ||
| if [[ "$dir" == "/" ]]; then | ||
| break | ||
| fi | ||
| dir="$(dirname "$dir")" | ||
| done | ||
|
|
||
| if [[ -z "$security_file" ]]; then | ||
| log_error "validate_command: .auto-claude-security.json not found (searched from $(pwd) to /). Create one with allowed command lists to enable auto-fix execution." | ||
|
||
| return 1 | ||
| fi | ||
|
|
||
| # Check if jq is available | ||
| if ! has_cmd jq; then | ||
| log_error "validate_command: jq is required but not installed" | ||
| return 1 | ||
| fi | ||
|
|
||
| # Validate the security config is valid JSON first | ||
| if ! jq '.' "$security_file" >/dev/null 2>&1; then | ||
| log_error "validate_command: $security_file is not valid JSON" | ||
| return 1 | ||
|
Comment on lines
+228
to
+286
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function walks from current directory to root searching for Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: scripts/helpers/common.sh
Line: 215:229
Comment:
The function walks from current directory to root searching for `.auto-claude-security.json`. This is good for monorepo support, but the search behavior isn't documented. If a user has the file in the wrong location (e.g., their home directory instead of project root), validation will find it but use potentially wrong allowlist. Consider adding a log_info message when the file is found to show which config is being used.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+284
to
+286
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing config file causes validation to fail silently. When Check for config file existence once at startup and show a clear setup message if missing, rather than failing on every command validation. Prompt To Fix With AIThis is a comment left during a code review.
Path: scripts/helpers/common.sh
Line: 248:250
Comment:
Missing config file causes validation to fail silently. When `.auto-claude-security.json` is not found, `validate_command()` logs an error and returns 1, but the calling code in `clawpinch.sh:305` suppresses stderr with `2>/dev/null`. Users won't know their commands are being rejected due to missing config — they'll just see auto_fix fields stripped with a warning.
Check for config file existence once at startup and show a clear setup message if missing, rather than failing on every command validation.
How can I resolve this? If you propose a fix, please make it concise. |
||
| fi | ||
|
|
||
| # Get all allowed commands from security config | ||
| local allowed_commands | ||
| allowed_commands="$(jq -r ' | ||
| (.base_commands // []) + | ||
| (.stack_commands // []) + | ||
| (.script_commands // []) + | ||
| (.custom_commands // []) | | ||
| .[] | ||
| ' "$security_file" 2>/dev/null)" | ||
|
|
||
| if [[ -z "$allowed_commands" ]]; then | ||
| log_error "validate_command: allowlist is empty in $security_file — no commands are permitted" | ||
| return 1 | ||
| fi | ||
|
|
||
| # Extract ALL commands from the string (split by |, &&, ||, ;) | ||
| # This ensures we validate every command in a chain | ||
| # Try to use Python script for proper quote-aware parsing | ||
| local script_dir | ||
| script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| local parse_script="$script_dir/parse_commands.py" | ||
|
|
||
| # Require Python parser — fail closed if unavailable (no insecure fallback) | ||
| if ! [[ -f "$parse_script" ]] || ! has_cmd python3; then | ||
| log_error "validate_command: python3 or parse_commands.py not available. Cannot securely validate command." | ||
| return 1 | ||
| fi | ||
|
|
||
| local base_commands_list | ||
| base_commands_list="$(python3 "$parse_script" "$cmd_string" 2>/dev/null)" | ||
| if [[ $? -ne 0 || -z "$base_commands_list" ]]; then | ||
| log_error "validate_command: Python helper failed to parse command string." | ||
| return 1 | ||
| fi | ||
|
|
||
| # Check each base command | ||
| while IFS= read -r base_cmd; do | ||
| # Skip empty lines | ||
| [[ -z "$base_cmd" ]] && continue | ||
|
|
||
| # Skip flags/options (start with -) | ||
| [[ "$base_cmd" =~ ^- ]] && continue | ||
|
|
||
| # Skip quoted strings (they're arguments, not commands) | ||
| [[ "$base_cmd" =~ ^[\'\"] ]] && continue | ||
|
||
|
|
||
| # Block path-based commands (/bin/rm, ./malicious, ~/script) — prevents allowlist bypass | ||
| if [[ "$base_cmd" =~ ^[/~\.] ]]; then | ||
| log_error "validate_command: path-based command '$base_cmd' is not allowed (use bare command names)" | ||
| return 1 | ||
| fi | ||
|
|
||
| # Skip redirection operators | ||
| case "$base_cmd" in | ||
| '>'|'>>'|'<'|'2>'|'&>'|'2>&1') continue ;; | ||
| esac | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The validator explicitly skips redirection operators ( |
||
|
|
||
| # Check if this command is in the allowlist (exact match) | ||
| if ! grep -Fxq -- "$base_cmd" <<< "$allowed_commands"; then | ||
| log_error "validate_command: '$base_cmd' is not in the allowlist" | ||
| return 1 | ||
| fi | ||
| done <<< "$base_commands_list" | ||
|
|
||
| # All commands validated successfully | ||
| return 0 | ||
| } | ||
|
|
||
| # ─── OS detection ─────────────────────────────────────────────────────────── | ||
|
|
||
| detect_os() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,150 @@ | ||
| #!/usr/bin/env python3 | ||
| """Parse shell command string and extract base commands while respecting quotes. | ||
|
|
||
| Security: rejects commands containing dangerous shell constructs that could | ||
| hide malicious commands (command substitution, process substitution, backticks). | ||
| """ | ||
| import sys | ||
| import shlex | ||
| import re | ||
|
|
||
|
|
||
| # Patterns that indicate hidden command execution — reject the entire string | ||
| _DANGEROUS_PATTERNS = [ | ||
| r'\$\(', # command substitution: $(...) | ||
| r'`', # backtick command substitution: `...` | ||
| r'<\(', # process substitution: <(...) | ||
| r'>\(', # process substitution: >(...) | ||
| ] | ||
|
|
||
| _DANGEROUS_RE = re.compile('|'.join(_DANGEROUS_PATTERNS)) | ||
|
|
||
|
|
||
| def _check_dangerous_outside_single_quotes(cmd_string): | ||
| """Check for dangerous patterns outside single-quoted regions. | ||
|
|
||
| Single quotes in shell prevent all expansion, so $() inside single | ||
| quotes is literal text (e.g. sed 's/$(pwd)/path/g' is safe). | ||
| Returns True if a dangerous pattern is found outside single quotes. | ||
| """ | ||
| in_single = False | ||
| i = 0 | ||
| while i < len(cmd_string): | ||
| c = cmd_string[i] | ||
| if c == "'" and not in_single: | ||
| # Entering single-quoted region — skip to closing quote | ||
| in_single = True | ||
| i += 1 | ||
| continue | ||
| elif c == "'" and in_single: | ||
| in_single = False | ||
| i += 1 | ||
| continue | ||
|
Comment on lines
+38
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Escaped single quotes ( Prompt To Fix With AIThis is a comment left during a code review.
Path: scripts/helpers/parse_commands.py
Line: 33:42
Comment:
Escaped single quotes (`\'`) aren't handled. In shell, `echo 'can'\''t'` is valid and produces "can't" as output. This parser won't track quote state correctly across escaped quotes.
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| if not in_single: | ||
| # Check if a dangerous pattern starts at this position | ||
| remaining = cmd_string[i:] | ||
| if _DANGEROUS_RE.match(remaining): | ||
| return True | ||
|
|
||
| i += 1 | ||
| return False | ||
|
Comment on lines
+26
to
+66
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
|
|
||
|
|
||
| def extract_commands(cmd_string): | ||
| """Extract all base commands from a shell command string. | ||
|
|
||
| Raises ValueError if the command string contains dangerous shell | ||
| constructs that could hide commands from validation. | ||
| """ | ||
| # Reject strings containing command/process substitution or backticks | ||
| # Only check outside single-quoted regions (single quotes prevent expansion) | ||
|
Comment on lines
+74
to
+76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The check for dangerous patterns correctly ignores them inside single-quoted strings. However, the implementation in Prompt To Fix With AIThis is a comment left during a code review.
Path: scripts/helpers/parse_commands.py
Line: 66:68
Comment:
The check for dangerous patterns correctly ignores them inside single-quoted strings. However, the implementation in `_check_dangerous_outside_single_quotes()` doesn't handle the shell construct `'can'\''t'` (escaped single quote within single-quoted context) perfectly - when encountering `\'` outside quotes (line 38-40), it skips both characters, but this could miss checking the character after the backslash for dangerous patterns if that character happens to start a pattern.
How can I resolve this? If you propose a fix, please make it concise. |
||
| if _check_dangerous_outside_single_quotes(cmd_string): | ||
| raise ValueError( | ||
| f"Command string contains dangerous shell construct: {cmd_string!r}" | ||
| ) | ||
|
|
||
| commands = [] | ||
|
|
||
| # Split by command separators: |, &&, ||, ;, & | ||
| # Use a simple state machine to handle quotes | ||
| in_single = False | ||
| in_double = False | ||
| current = "" | ||
| i = 0 | ||
|
|
||
| while i < len(cmd_string): | ||
| c = cmd_string[i] | ||
|
|
||
| # Handle backslash escape (only outside single quotes) | ||
| if c == "\\" and not in_single and i + 1 < len(cmd_string): | ||
| current += c + cmd_string[i + 1] | ||
| i += 2 | ||
| continue | ||
|
|
||
| # Track quote state | ||
| if c == "'" and not in_double: | ||
| in_single = not in_single | ||
| current += c | ||
| elif c == '"' and not in_single: | ||
| in_double = not in_double | ||
| current += c | ||
| # Check for separators outside quotes | ||
| elif not in_single and not in_double: | ||
| if i < len(cmd_string) - 1 and cmd_string[i:i+2] in ['&&', '||']: | ||
| if current.strip(): | ||
| commands.append(current.strip()) | ||
| current = "" | ||
| i += 1 # skip second char | ||
| elif c in ['|', ';']: | ||
| if current.strip(): | ||
| commands.append(current.strip()) | ||
| current = "" | ||
| elif c == '&': | ||
| # Background operator — treat as separator | ||
| if current.strip(): | ||
| commands.append(current.strip()) | ||
| current = "" | ||
| elif c == '\n': | ||
| # Newline — treat as separator | ||
| if current.strip(): | ||
| commands.append(current.strip()) | ||
| current = "" | ||
| else: | ||
| current += c | ||
| else: | ||
| current += c | ||
|
|
||
| i += 1 | ||
|
|
||
| if current.strip(): | ||
| commands.append(current.strip()) | ||
|
|
||
| # Extract base command from each segment | ||
| base_commands = [] | ||
| for cmd in commands: | ||
| try: | ||
| # Use shlex to properly parse the command | ||
| tokens = shlex.split(cmd) | ||
| if tokens: | ||
| base_commands.append(tokens[0]) | ||
| except ValueError: | ||
| # If shlex fails, reject — don't fall back to insecure parsing | ||
| raise ValueError(f"Failed to parse command segment: {cmd!r}") | ||
|
|
||
| return base_commands | ||
|
|
||
| if __name__ == "__main__": | ||
| if len(sys.argv) > 1: | ||
| cmd = " ".join(sys.argv[1:]) | ||
| else: | ||
| cmd = sys.stdin.read().strip() | ||
|
|
||
| try: | ||
| commands = extract_commands(cmd) | ||
| except ValueError as e: | ||
| print(f"ERROR: {e}", file=sys.stderr) | ||
| sys.exit(1) | ||
|
|
||
| for c in commands: | ||
| print(c) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validation errors are silenced with
2>/dev/null. If.auto-claude-security.jsonis missing, users won't see why auto_fix commands are being stripped.Prompt To Fix With AI