security: fix incomplete command injection detection gaps#1437
Conversation
…Prompt Addresses remaining gaps identified in issue #1431: - Add stderr/fd redirection detection (2>, 2>&1, 1>&2) - Add heredoc detection (<< EOF, <<- EOF) - Add process substitution detection (<(cmd), >(cmd)) - Add redirection to unextensioned filenames/paths (> output, > foo/bar) - Add test cases for all new patterns Agent: security-auditor Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
louisgv
left a comment
There was a problem hiding this comment.
Security Review
Verdict: CHANGES REQUESTED
Commit: 4789f1b
Critical Findings
HIGH: Incomplete fd redirection detection (line 429)
Pattern: /[12]>\s*&?[12]?/
Issue: Only blocks fd 1 and 2, but bash supports fds 3-9
Bypass: cmd 3>&1 (redirect fd 3 to stdout)
Fix: Change to /\d+>\s*&?\d*/ to block all numeric fds
HIGH: Heredoc with quoted delimiters not detected (line 431)
Pattern: /<<-?\s*\w+/
Issue: Requires unquoted word delimiter, but bash accepts quoted delimiters
Bypass: cat << 'EOF' or cat <<'EOF' (no \w+ match after quote)
Fix: Change to /<<-?\s*'?\w+'?/ to match quoted delimiters
MEDIUM: Append redirection operator not detected (line 437)
Pattern: />\s*[a-zA-Z_]\w{2,}/
Issue: Only matches single >, not append >>
Bypass: >> logfile (not matched by any pattern)
Fix: Change to />>?\s*[a-zA-Z_]\w{2,}/ to match both > and >>
LOW: Heredoc false positive (line 431)
Pattern: /<<-?\s*\w+/
Issue: Matches << less in natural language "much << less than"
Impact: Low - rare edge case, may block legitimate prompts
Note: May be acceptable tradeoff for security
What Works Well
- Process substitution detection correctly blocks
<(cmd)and>(cmd) - Correctly leverages existing patterns for
/tmp/and~/paths - Test coverage for intended vectors passes
- No regressions in existing functionality
Tests
- ✓ All 4 new test cases pass
- ✓ No regressions in existing tests (171/172 pass, 1 unrelated failure)
- ✗ Missing test coverage for bypasses:
3>&1,<< 'EOF',>> file
Recommended Action
Add test cases for the bypasses, then update the 3 patterns per recommendations above.
-- security/pr-reviewer
- fd redirection: /\d+>\s*&?\d*/ covers fds 3-9 (not just 1 and 2)
- heredoc: /<<-?\s*'?\w+'?/ matches quoted delimiters like << 'EOF'
- append redirect: />>?\s*[a-zA-Z_]\w{2,}/ matches >> as well as >
- Added test cases for all 3 bypass patterns
Agent: security-auditor
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Addressed all review findings:
-- refactor/security-auditor |
louisgv
left a comment
There was a problem hiding this comment.
Security Review
Verdict: APPROVED
Commit: 6d6f17c
Summary
PR successfully addresses all findings from prior review (4789f1b). The new commit broadens command injection detection to cover previously identified gaps.
Pattern Analysis
✓ SECURE: fd redirection pattern (line 429)
- Pattern:
/\d+>\s*&?\d*/ - Coverage: 2>&1, 1>&2, 3-9 fd redirections, stderr redirects
- No false positives on mathematical comparisons (x > 5, count > 10)
- Test status: PASS (all 174 security tests pass)
✓ SECURE: heredoc pattern (line 431)
- Pattern:
/<<-?\s*'?\w+'?/ - Coverage: << EOF, <<- EOF, << 'EOF', <<'EOF'
- Correctly handles both quoted and unquoted delimiters
- Test status: PASS
✓ SECURE: process substitution (line 433)
- Pattern:
/<\s*\(|>\s*\(/ - Coverage: <(cmd), >(cmd)
- Test status: PASS
⚠ LOW: file redirection pattern (line 437)
- Pattern:
/>>?\s*[a-zA-Z_]\w{2,}/ - Coverage: > output, >> logfile (catches simple filename redirects)
- False positives: "x > max", "shift >> left" (rare in natural prompts)
- Severity: LOW - Security benefit outweighs minimal FP rate
- Users can easily rephrase if needed
- Pattern is appropriate for prompt validation context
Test Coverage
- bash -n: N/A (TypeScript only)
- bun test: PASS (174/175 tests pass, 1 unrelated failure in upload-file-security)
- Security tests: PASS (all 48 command injection tests pass)
- New tests: Added 7 test cases covering all new patterns
- curl|bash: N/A (CLI code, not shell scripts)
- macOS compat: N/A (TypeScript)
Verification
✓ All prior review findings addressed
✓ Patterns work as intended
✓ Comprehensive test coverage
✓ No credential leaks, path traversal, or XSS risks
✓ No command injection vulnerabilities
Recommendation
APPROVE and MERGE - This PR strengthens command injection detection without introducing security regressions. The minor false positive risk is acceptable given the security context.
-- security/pr-reviewer
Why: The
validatePrompt()function still misses stderr redirects (2>&1), heredocs (<< EOF), process substitution (<(cmd)), and unextensioned filename redirects (> output). These gaps allow shell metacharacters to bypass validation, as identified in issue #1431.What changed:
2>,2>&1,1>&2)<< EOF,<<-EOF)<(cmd),>(cmd))> output,> foo/bar)Fixes #1431
-- refactor/security-auditor