diff --git a/src/engine/mod.rs b/src/engine/mod.rs index a29b530e..04a8cb5f 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -10,6 +10,7 @@ pub mod review; pub mod rules; pub mod scanner; pub mod secrets_scanner; +pub mod security_scanner; pub mod static_analysis; pub mod types; diff --git a/src/engine/review.rs b/src/engine/review.rs index ae5f4e3c..457324c1 100644 --- a/src/engine/review.rs +++ b/src/engine/review.rs @@ -123,27 +123,52 @@ async fn review_diff_inner( config.rules_config.max_findings, ); + // Run deterministic security pattern scan (weak crypto, injection, etc.) + let security_findings = crate::engine::security_scanner::scan_security( + &diff_chunks, + config.rules_config.max_findings, + ); + let rule_context = crate::engine::rules::format_rule_context(&rule_findings); let secrets_context = crate::engine::rules::format_rule_context(&secrets_findings); + let security_context = crate::engine::rules::format_rule_context(&security_findings); // Keep a clone for merging after LLM (rule_findings may be consumed in error fallback) let rule_findings_clone = rule_findings.clone(); let secrets_findings_clone = secrets_findings.clone(); + let security_findings_clone = security_findings.clone(); - // Combine static analysis + rule engine + secrets findings for LLM prompt + // Combine static analysis + rule engine + secrets + security findings for LLM prompt let combined_context = match ( static_context.as_deref(), rule_context.as_str(), secrets_context.as_str(), + security_context.as_str(), ) { - (Some(sa), rc, sc) if !rc.is_empty() && !sc.is_empty() => { + (Some(sa), rc, sc, sec) if !rc.is_empty() && !sc.is_empty() && !sec.is_empty() => { + Some(format!("{sa}\n\n{rc}\n\n{sc}\n\n{sec}")) + } + (Some(sa), rc, sc, _) if !rc.is_empty() && !sc.is_empty() => { Some(format!("{sa}\n\n{rc}\n\n{sc}")) } - (Some(sa), rc, _) if !rc.is_empty() => Some(format!("{sa}\n\n{rc}")), - (Some(sa), _, sc) if !sc.is_empty() => Some(format!("{sa}\n\n{sc}")), - (Some(sa), _, _) => Some(sa.to_string()), - (_, rc, sc) if !rc.is_empty() && !sc.is_empty() => Some(format!("{rc}\n\n{sc}")), - (_, rc, _) if !rc.is_empty() => Some(rc.to_string()), - (_, _, sc) if !sc.is_empty() => Some(sc.to_string()), + (Some(sa), rc, _, sec) if !rc.is_empty() && !sec.is_empty() => { + Some(format!("{sa}\n\n{rc}\n\n{sec}")) + } + (Some(sa), _, sc, sec) if !sc.is_empty() && !sec.is_empty() => { + Some(format!("{sa}\n\n{sc}\n\n{sec}")) + } + (Some(sa), rc, _, _) if !rc.is_empty() => Some(format!("{sa}\n\n{rc}")), + (Some(sa), _, sc, _) if !sc.is_empty() => Some(format!("{sa}\n\n{sc}")), + (Some(sa), _, _, sec) if !sec.is_empty() => Some(format!("{sa}\n\n{sec}")), + (Some(sa), _, _, _) => Some(sa.to_string()), + (_, rc, sc, sec) if !rc.is_empty() && !sc.is_empty() && !sec.is_empty() => { + Some(format!("{rc}\n\n{sc}\n\n{sec}")) + } + (_, rc, sc, _) if !rc.is_empty() && !sc.is_empty() => Some(format!("{rc}\n\n{sc}")), + (_, rc, _, sec) if !rc.is_empty() && !sec.is_empty() => Some(format!("{rc}\n\n{sec}")), + (_, _, sc, sec) if !sc.is_empty() && !sec.is_empty() => Some(format!("{sc}\n\n{sec}")), + (_, rc, _, _) if !rc.is_empty() => Some(rc.to_string()), + (_, _, sc, _) if !sc.is_empty() => Some(sc.to_string()), + (_, _, _, sec) if !sec.is_empty() => Some(sec.to_string()), _ => None, }; @@ -223,6 +248,8 @@ async fn review_diff_inner( crate::engine::rules::merge_rule_findings(vec![], rule_findings); all_deterministic = crate::engine::rules::merge_rule_findings(all_deterministic, secrets_findings); + all_deterministic = + crate::engine::rules::merge_rule_findings(all_deterministic, security_findings); let mut fallback = ReviewResponse { issues: all_deterministic, summary: format!( @@ -243,7 +270,7 @@ async fn review_diff_inner( } }; - // Merge rule findings + secrets findings with LLM issues + // Merge rule findings + secrets findings + security findings with LLM issues if !rule_findings_clone.is_empty() { response.issues = crate::engine::rules::merge_rule_findings(response.issues, rule_findings_clone); @@ -252,6 +279,10 @@ async fn review_diff_inner( response.issues = crate::engine::rules::merge_rule_findings(response.issues, secrets_findings_clone); } + if !security_findings_clone.is_empty() { + response.issues = + crate::engine::rules::merge_rule_findings(response.issues, security_findings_clone); + } // Filter out issues with invalid file paths (hallucination guard) if !valid_files.is_empty() { diff --git a/src/engine/security_scanner.rs b/src/engine/security_scanner.rs new file mode 100644 index 00000000..fb742f06 --- /dev/null +++ b/src/engine/security_scanner.rs @@ -0,0 +1,315 @@ +//! Static security scanner — deterministic pattern detection for common vulnerabilities. +//! +//! Runs before LLM review. Catches what's guaranteed to be a finding: +//! weak crypto, SQL injection, code injection, auth issues, debug config. +//! Secrets are handled separately by `secrets_scanner.rs`. + +use regex::Regex; +use std::sync::LazyLock; +use tracing::debug; + +use crate::engine::Severity; +use crate::engine::diff_parser::{DiffLineType, FileChunk}; +use crate::engine::rules::types::RuleFinding; + +// ─── Security patterns ─── + +struct SecurityPattern { + id: &'static str, + name: &'static str, + regex: &'static str, + severity: Severity, +} + +static PATTERNS: &[SecurityPattern] = &[ + // ── Weak crypto ── + SecurityPattern { + id: "crypto/md5-password", + name: "MD5 used for password hashing", + regex: r"(?i)md5\s*\(\s*(?:password|passwd|pwd|secret)", + severity: Severity::Critical, + }, + SecurityPattern { + id: "crypto/sha1-password", + name: "SHA-1 used for password hashing", + regex: r"(?i)sha1\s*\(\s*(?:password|passwd|pwd|secret)", + severity: Severity::Major, + }, + SecurityPattern { + id: "crypto/weak-hash", + name: "Weak hash algorithm (MD5/SHA1) for security-sensitive data", + regex: r"(?i)(?:hashlib\.md5|hashlib\.sha1|MD5Create|SHA1Create|Digest::MD5|Digest::SHA1)", + severity: Severity::Major, + }, + SecurityPattern { + id: "crypto/hardcoded-secret", + name: "Hardcoded password or secret in variable", + regex: r"(?i)(?:password|passwd|pwd|secret|api_key|apikey|token)\s*[=:]\s*\S{8,}", + severity: Severity::Critical, + }, + // ── Injection ── + SecurityPattern { + id: "injection/sql-concat", + name: "SQL injection via string concatenation", + regex: r"(?i)(?:SELECT|INSERT|UPDATE|DELETE)\s+.*\+", + severity: Severity::Critical, + }, + SecurityPattern { + id: "injection/eval", + name: "eval() with dynamic input", + regex: r"(?i)eval\s*\(\s*(?:req|request|input|params|data|user)", + severity: Severity::Critical, + }, + SecurityPattern { + id: "injection/exec", + name: "Command injection via exec/system with dynamic input", + regex: r"(?i)(?:exec|system|popen|subprocess\.(?:call|run))\s*\(", + severity: Severity::Critical, + }, + // ── Auth issues ── + SecurityPattern { + id: "auth/hardcoded-role", + name: "Hardcoded role or permission check", + regex: r"(?i)role\s*==\s*(?:admin|super|root)|is_admin\s*==\s*True", + severity: Severity::Major, + }, + // ── Debug config ── + SecurityPattern { + id: "config/debug-enabled", + name: "Debug mode enabled (production risk)", + regex: r"(?i)(?:DEBUG\s*=\s*True|debug:\s*true|--debug)", + severity: Severity::Minor, + }, + SecurityPattern { + id: "config/cors-wildcard", + name: "CORS wildcard allows all origins", + regex: r"(?i)(?:Access-Control-Allow-Origin|cors).*\*", + severity: Severity::Major, + }, + // ── TLS/SSL ── + SecurityPattern { + id: "crypto/ssl-verify-disabled", + name: "SSL certificate verification disabled", + regex: r"(?i)(?:verify\s*=\s*False|verify:\s*false|rejectUnauthorized:\s*false)", + severity: Severity::Critical, + }, +]; + +static COMPILED_PATTERNS: LazyLock> = LazyLock::new(|| { + PATTERNS + .iter() + .filter_map(|p| { + Regex::new(p.regex) + .ok() + .map(|re| (p.id.to_string(), p.name.to_string(), re, p.severity)) + }) + .collect() +}); + +/// Run static security scan on diff chunks. +/// +/// Only scans added lines (not removed/context) to reduce false positives. +/// Returns findings sorted by severity (worst first). +pub fn scan_security(chunks: &[FileChunk], max_findings: usize) -> Vec { + let mut findings = Vec::new(); + + for chunk in chunks { + let path = chunk + .new_path + .as_deref() + .or(chunk.old_path.as_deref()) + .unwrap_or("unknown"); + + // Skip test/spec/fixture/mock/example files + if is_test_file(path) { + debug!(file = path, "skipping test file in security scan"); + continue; + } + + for hunk in &chunk.chunks { + for line in &hunk.lines { + if line.line_type != DiffLineType::Add { + continue; + } + + let line_no = line.new_line_no.unwrap_or(0); + if line_no == 0 { + continue; + } + + for (rule_id, name, regex, severity) in COMPILED_PATTERNS.iter() { + if regex.is_match(&line.content) { + debug!( + rule = %rule_id, + file = path, + line = line_no, + "security pattern match" + ); + findings.push(RuleFinding { + rule_id: rule_id.clone(), + file: path.to_string(), + line: line_no, + severity: *severity, + title: name.clone(), + body: format!( + "Static security scanner detected: {} in {}:{}", + name, path, line_no + ), + }); + + if findings.len() >= max_findings { + findings.sort_by_key(|f| std::cmp::Reverse(f.severity)); + return findings; + } + } + } + } + } + } + + findings.sort_by_key(|f| std::cmp::Reverse(f.severity)); + findings +} + +/// Check if a file path looks like a test/spec/fixture file. +fn is_test_file(path: &str) -> bool { + let lower = path.to_lowercase(); + lower.contains("test") + || lower.contains("spec") + || lower.contains("fixture") + || lower.contains("mock") + || lower.contains("example") + || lower.contains("__tests__") + || lower.contains(".test.") + || lower.contains(".spec.") +} + +#[cfg(test)] +mod tests { + use super::*; + + fn make_chunk(file: &str, added_lines: &[&str]) -> FileChunk { + use crate::engine::diff_parser::{DiffHunk, DiffLine}; + FileChunk { + old_path: None, + new_path: Some(file.to_string()), + language: "py".to_string(), + chunks: vec![DiffHunk { + old_start: 1, + old_count: 1, + new_start: 1, + new_count: added_lines.len() as u32, + header: String::new(), + lines: added_lines + .iter() + .enumerate() + .map(|(i, content)| DiffLine { + line_type: DiffLineType::Add, + content: content.to_string(), + old_line_no: None, + new_line_no: Some(i as u32 + 1), + }) + .collect(), + }], + is_binary: false, + is_deleted: false, + is_new: false, + } + } + + #[test] + fn detects_hardcoded_password() { + let chunks = vec![make_chunk( + "src/auth.rs", + &["let password = supersecret123;"], + )]; + let findings = scan_security(&chunks, 10); + assert!(!findings.is_empty()); + assert!(findings[0].rule_id.contains("hardcoded-secret")); + } + + #[test] + fn detects_sql_injection() { + let chunks = vec![make_chunk( + "src/db.py", + &["query = \"SELECT * FROM users WHERE id = \" + req.params.id"], + )]; + let findings = scan_security(&chunks, 10); + assert!(!findings.is_empty()); + assert!(findings[0].rule_id.contains("sql")); + } + + #[test] + fn detects_eval_injection() { + let chunks = vec![make_chunk("src/run.js", &["eval(request.body.code)"])]; + let findings = scan_security(&chunks, 10); + assert!(!findings.is_empty()); + assert!(findings[0].rule_id.contains("eval")); + } + + #[test] + fn detects_debug_enabled() { + let chunks = vec![make_chunk("src/config.py", &["DEBUG = True"])]; + let findings = scan_security(&chunks, 10); + assert!(!findings.is_empty()); + assert!(findings[0].rule_id.contains("debug")); + } + + #[test] + fn detects_ssl_verify_disabled() { + let chunks = vec![make_chunk( + "src/client.py", + &["requests.get(url, verify=False)"], + )]; + let findings = scan_security(&chunks, 10); + assert!(!findings.is_empty()); + assert!(findings[0].rule_id.contains("ssl")); + } + + #[test] + fn detects_cors_wildcard() { + let chunks = vec![make_chunk( + "src/server.rs", + &["Access-Control-Allow-Origin: *"], + )]; + let findings = scan_security(&chunks, 10); + assert!(!findings.is_empty()); + } + + #[test] + fn skips_test_files() { + let chunks = vec![make_chunk( + "tests/auth_test.py", + &["let password = test_password_123;"], + )]; + let findings = scan_security(&chunks, 10); + assert!(findings.is_empty()); + } + + #[test] + fn empty_diff_no_findings() { + let findings = scan_security(&[], 10); + assert!(findings.is_empty()); + } + + #[test] + fn respects_max_findings() { + let chunks = vec![ + make_chunk("src/a.py", &["DEBUG = True", "eval(input.data)"]), + make_chunk("src/b.py", &["let password = supersecret"]), + ]; + let findings = scan_security(&chunks, 2); + assert_eq!(findings.len(), 2); + } + + #[test] + fn sorts_by_severity() { + let chunks = vec![make_chunk( + "src/app.py", + &["DEBUG = True", "let password = supersecret"], + )]; + let findings = scan_security(&chunks, 10); + // Critical (hardcoded-secret) should come before Minor (debug) + assert!(findings[0].severity >= findings[findings.len() - 1].severity); + } +}