diff --git a/src/engine/review.rs b/src/engine/review.rs index db0ae5f..185c3d3 100644 --- a/src/engine/review.rs +++ b/src/engine/review.rs @@ -478,6 +478,7 @@ fn apply_ignore_rules(mut issues: Vec, ignore_rules: &[String]) -> return issues; } + let before = issues.len(); issues.retain(|issue| { !ignore_rules.iter().any(|pattern| { let pattern_lower = pattern.to_lowercase(); @@ -486,6 +487,15 @@ fn apply_ignore_rules(mut issues: Vec, ignore_rules: &[String]) -> || issue.title.to_lowercase().contains(&pattern_lower) }) }); + let filtered = before - issues.len(); + if filtered > 0 { + debug!( + filtered, + remaining = issues.len(), + rules = ignore_rules.len(), + "filtered issues via ignore rules" + ); + } issues } @@ -695,4 +705,83 @@ mod tests { "unknown lines should be kept (better safe than sorry)" ); } + + #[test] + fn ignore_rules_filters_by_title_match() { + let issues = vec![ + ReviewIssue { + file: "cli.rs".to_string(), + line: Some(236), + severity: Severity::Critical, + issue_type: Some("rule".to_string()), + title: "Command injection via exec/system with dynamic input".to_string(), + body: "Static security scanner detected...".to_string(), + suggested_fix: None, + }, + ReviewIssue { + file: "main.rs".to_string(), + line: Some(10), + severity: Severity::Major, + issue_type: Some("security".to_string()), + title: "SQL injection via string concatenation".to_string(), + body: "...".to_string(), + suggested_fix: None, + }, + ]; + + let rules = vec!["Command injection via exec/system with dynamic input".to_string()]; + let result = apply_ignore_rules(issues, &rules); + assert_eq!(result.len(), 1); + assert_eq!(result[0].title, "SQL injection via string concatenation"); + } + + #[test] + fn ignore_rules_filters_by_issue_type_match() { + let issues = vec![ReviewIssue { + file: "test.py".to_string(), + line: Some(50), + severity: Severity::Minor, + issue_type: Some("style".to_string()), + title: "Some style issue".to_string(), + body: "...".to_string(), + suggested_fix: None, + }]; + + let rules = vec!["style".to_string()]; + let result = apply_ignore_rules(issues, &rules); + assert!(result.is_empty()); + } + + #[test] + fn ignore_rules_empty_keeps_all() { + let issues = vec![ReviewIssue { + file: "f.rs".to_string(), + line: Some(1), + severity: Severity::Critical, + issue_type: Some("rule".to_string()), + title: "Any finding".to_string(), + body: "...".to_string(), + suggested_fix: None, + }]; + + let result = apply_ignore_rules(issues, &[]); + assert_eq!(result.len(), 1); + } + + #[test] + fn ignore_rules_case_insensitive() { + let issues = vec![ReviewIssue { + file: "f.rs".to_string(), + line: Some(1), + severity: Severity::Critical, + issue_type: Some("rule".to_string()), + title: "HARDCODED password or SECRET in variable".to_string(), + body: "...".to_string(), + suggested_fix: None, + }]; + + let rules = vec!["Hardcoded Password Or Secret".to_string()]; + let result = apply_ignore_rules(issues, &rules); + assert!(result.is_empty()); + } } diff --git a/src/engine/security_scanner.rs b/src/engine/security_scanner.rs index 3bf5fa9..8f6ed95 100644 --- a/src/engine/security_scanner.rs +++ b/src/engine/security_scanner.rs @@ -63,7 +63,10 @@ pub static PATTERNS: &[SecurityPattern] = &[ SecurityPattern { id: "injection/exec", name: "Command injection via exec/system with dynamic input", - regex: r"(?i)(?:exec|system|popen|subprocess\.(?:call|run))\s*\(", + // Only flag when there's a dynamic input signal on the same line + // (f-string, format(), string concat with +, shell=True, or raw user input). + // subprocess.run(["cmd", "arg"]) with literal list is safe and should not trigger. + regex: r#"(?i)(?:exec|system|popen|subprocess\.(?:call|run))\s*\((?:[^)]*(?:f"|f'|format\(|\.format\(|shell\s*=\s*True|\+\s*(?:req|request|input|params|data|user|query)|%s|%\(.*\)))"#, severity: Severity::Critical, }, // ── Auth issues ── @@ -312,4 +315,57 @@ mod tests { // Critical (hardcoded-secret) should come before Minor (debug) assert!(findings[0].severity >= findings[findings.len() - 1].severity); } + + #[test] + fn subprocess_run_with_literal_list_no_false_positive() { + let chunks = vec![make_chunk( + "src/provider.py", + &["cmd = [self._bin, 'recall', query]", "subprocess.run(cmd)"], + )]; + let findings = scan_security(&chunks, 10); + let exec_findings: Vec<_> = findings + .iter() + .filter(|f| f.rule_id == "injection/exec") + .collect(); + assert!( + exec_findings.is_empty(), + "subprocess.run(cmd) with controlled list should not trigger" + ); + } + + #[test] + fn subprocess_run_with_fstring_triggers() { + let chunks = vec![make_chunk( + "src/handler.py", + &["subprocess.run(f'cat {user_input}')"], + )]; + let findings = scan_security(&chunks, 10); + let exec_findings: Vec<_> = findings + .iter() + .filter(|f| f.rule_id == "injection/exec") + .collect(); + assert_eq!( + exec_findings.len(), + 1, + "subprocess.run with f-string should trigger" + ); + } + + #[test] + fn subprocess_run_with_shell_true_triggers() { + let chunks = vec![make_chunk( + "src/handler.py", + &["subprocess.run(cmd, shell=True)"], + )]; + let findings = scan_security(&chunks, 10); + let exec_findings: Vec<_> = findings + .iter() + .filter(|f| f.rule_id == "injection/exec") + .collect(); + assert_eq!( + exec_findings.len(), + 1, + "subprocess.run with shell=True should trigger" + ); + } }