diff --git a/CHANGELOG.md b/CHANGELOG.md index c8b6665..23572b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.17.1] - 2026-10-09 + +### Fixed + +- **Hardcoded secrets in Go short declarations, YAML/JSON keys and SQL `PASSWORD` literals are now detected.** Go `password := "..."` (with or without a trailing comment) and `var apiKey string = "..."` (#618); unquoted YAML `password: hunter2hunter2` and JSON quoted keys `"password": "..."` / `"apiKey": "..."` (#619); SQL `CREATE USER app WITH PASSWORD ''` and `IDENTIFIED BY ''` (#620). References stay quiet: `${VAR}`/`$VAR`, `!tag`, `*alias`, `&anchor`, paths, `null`/`required`-style bare words, and SQL placeholders (`PASSWORD '%s'`). The LLM secret cross-check uses the same pattern, so LLM findings on these shapes are no longer dropped. + +### Changed + +- **One shared post-processing module for `cora review` and `cora scan`.** The finding filter pipeline (LLM secret false-positive cross-check, Markdown code blocks, `ignore.rules`, inline `cora-ignore:`, context-line filter, scanner merge for scans) now lives in `engine::postprocess`, so a fix lands in one place. Internal refactor with no CLI or output change (#610). + +- **`ReviewIssue::new` constructor and table-driven hardcoded-secret tests.** `ReviewIssue` is now built through `ReviewIssue::new(..)` plus `with_*` setters, so adding a field touches one place instead of every construction site. One end-to-end table covers the hardcoded-secret heuristics across languages; known false positives/negatives are pinned in a separate table. Internal change with no CLI or output change (#610). + ## [0.17.0] - 2026-10-09 ### Fixed @@ -968,7 +980,8 @@ Benchmarked on the cora-code repository (1,864 symbols, 115 Rust files, x86_64): - **Cross-platform** — Linux (x86_64, ARM64), macOS (Apple Silicon), Windows (x86_64) - **MIT License** — fully open source -[Unreleased]: https://github.com/codecoradev/cora-code/compare/v0.17.0...develop +[Unreleased]: https://github.com/codecoradev/cora-code/compare/v0.17.1...develop +[0.17.1]: https://github.com/codecoradev/cora-code/compare/v0.17.0...v0.17.1 [0.17.0]: https://github.com/codecoradev/cora-code/compare/v0.16.1...v0.17.0 [0.16.1]: https://github.com/codecoradev/cora-code/compare/v0.16.0...v0.16.1 [0.16.0]: https://github.com/codecoradev/cora-code/compare/v0.15.0...v0.16.0 diff --git a/Cargo.lock b/Cargo.lock index d69f9aa..afb8ad8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -303,7 +303,7 @@ dependencies = [ [[package]] name = "cora-code" -version = "0.17.0" +version = "0.17.1" dependencies = [ "anyhow", "assert_cmd", diff --git a/Cargo.toml b/Cargo.toml index 921fe04..0b81246 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cora-code" -version = "0.17.0" +version = "0.17.1" edition = "2024" description = "CLI-first AI code review — BYOK, diff/scan/branch, pre-commit hooks" license = "Apache-2.0" diff --git a/src/commands/review.rs b/src/commands/review.rs index b65269a..7a76bbe 100644 --- a/src/commands/review.rs +++ b/src/commands/review.rs @@ -792,17 +792,9 @@ mod tests { use crate::engine::types::ReviewIssue; fn issue(severity: Severity) -> ReviewIssue { - ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "src/main.rs".to_string(), - line: Some(1), - severity, - issue_type: Some("bug".to_string()), - title: "test".to_string(), - body: "body".to_string(), - suggested_fix: None, - } + ReviewIssue::new("src/main.rs", Some(1), severity, "test") + .with_type("bug") + .with_body("body") } fn response(issues: Vec, should_block: bool) -> ReviewResponse { diff --git a/src/commands/scan.rs b/src/commands/scan.rs index 40aba93..b0e5d49 100644 --- a/src/commands/scan.rs +++ b/src/commands/scan.rs @@ -5,13 +5,11 @@ use colored::Colorize; use tracing::debug; use crate::config::schema::Config; -use crate::engine::review::apply_ignore_rules; +use crate::engine::ReviewIssue; +use crate::engine::postprocess::{Source, postprocess}; use crate::engine::review_store; -use crate::engine::scanner::{ - FileEntry, batch_files, files_as_chunks, format_batch_for_prompt, walk_project, -}; +use crate::engine::scanner::{FileEntry, batch_files, format_batch_for_prompt, walk_project}; use crate::engine::types::TokenUsage; -use crate::engine::{ReviewIssue, inline_suppress, rules, secrets_scanner, security_scanner}; use crate::formatters::{OutputFormat, formatter_for}; /// Default maximum files per LLM batch when `--batch-files` is not specified. @@ -345,19 +343,7 @@ fn finalize_issues( files: &[FileEntry], issues: Vec, ) -> Vec { - let chunks = files_as_chunks(files); - let max = config.rules_config.max_findings; - let mut merged = issues; - for family in [ - secrets_scanner::scan_secrets(&chunks, max), - security_scanner::scan_security(&chunks, max), - ] { - if !family.is_empty() { - merged = rules::merge_rule_findings(merged, family); - } - } - let merged = apply_ignore_rules(merged, &config.ignore.rules); - inline_suppress::apply(merged, &chunks) + postprocess(issues, &Source::Files(files), config) } /// Compute a short SHA256 hash of a file's content for incremental scanning. diff --git a/src/engine/cache.rs b/src/engine/cache.rs index 222ab51..042e6cd 100644 --- a/src/engine/cache.rs +++ b/src/engine/cache.rs @@ -186,17 +186,12 @@ mod tests { fn make_response() -> ReviewResponse { ReviewResponse { - issues: vec![ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "src/main.rs".to_string(), - line: Some(10), - severity: Severity::Major, - issue_type: Some("bugs".to_string()), - title: "Null pointer".to_string(), - body: "Could be null here".to_string(), - suggested_fix: Some("Add a check".to_string()), - }], + issues: vec![ + ReviewIssue::new("src/main.rs", Some(10), Severity::Major, "Null pointer") + .with_type("bugs") + .with_body("Could be null here") + .with_fix("Add a check"), + ], summary: "Found 1 issue.".to_string(), tokens_used: None, should_block: false, diff --git a/src/engine/debt_tracker.rs b/src/engine/debt_tracker.rs index e09871e..b6c04e6 100644 --- a/src/engine/debt_tracker.rs +++ b/src/engine/debt_tracker.rs @@ -703,17 +703,8 @@ mod tests { use super::*; fn make_issue(severity: Severity, issue_type: &str) -> ReviewIssue { - ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "test.rs".to_string(), - line: Some(1), - severity, - issue_type: Some(issue_type.to_string()), - title: "test issue".to_string(), - body: String::new(), - suggested_fix: None, - } + ReviewIssue::new("test.rs", Some(1), severity, "test issue") + .with_type(issue_type.to_string()) } // ─── normalize_category ─── diff --git a/src/engine/deterministic.rs b/src/engine/deterministic.rs index b0b48b3..d75bf4e 100644 --- a/src/engine/deterministic.rs +++ b/src/engine/deterministic.rs @@ -279,17 +279,12 @@ diff --git a/src/api.js b/src/api.js let report = run(&chunks, &Config::default(), &fixture_bridge()); let total = report.len(); let first = report.secrets[0].clone(); - let llm = vec![ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: first.file.clone(), - line: Some(first.line), - severity: crate::engine::Severity::Major, - issue_type: None, - title: "Hardcoded secret committed to the repository".into(), - body: String::new(), - suggested_fix: None, - }]; + let llm = vec![ReviewIssue::new( + first.file.clone(), + Some(first.line), + crate::engine::Severity::Major, + "Hardcoded secret committed to the repository", + )]; let merged = report.merge_into(llm); assert_eq!( merged[0].title, diff --git a/src/engine/index_scanner.rs b/src/engine/index_scanner.rs index 3987412..74760d3 100644 --- a/src/engine/index_scanner.rs +++ b/src/engine/index_scanner.rs @@ -390,21 +390,21 @@ pub fn scan_project_index( match graph::find_unused_imports(conn, &entry.path, project_id) { Ok(unused) => { for u in &unused { - findings.push(ReviewIssue { - rule_id: Some("index-unused-import".into()), - also_matches: Vec::new(), - file: u.file.clone(), - line: Some(u.line), - severity: crate::engine::Severity::Minor, - issue_type: Some("index".into()), - title: format!("[index-unused-import] Unused import: {}", u.target), - body: format!( + findings.push( + ReviewIssue::new( + u.file.clone(), + Some(u.line), + crate::engine::Severity::Minor, + format!("[index-unused-import] Unused import: {}", u.target), + ) + .with_type("index") + .with_body(format!( "Import `{}` is never used in this file. \ Consider removing it to keep imports clean.", u.target - ), - suggested_fix: None, - }); + )) + .with_rule_id("index-unused-import"), + ); } } Err(e) => { @@ -426,22 +426,22 @@ pub fn scan_project_index( match graph::find_dead_code(conn, project_id, &opts) { Ok(dead) => { for func in dead.into_iter().take(max_findings - findings.len()) { - findings.push(ReviewIssue { - rule_id: Some("index-dead-code".into()), - also_matches: Vec::new(), - file: func.file.clone(), - line: Some(func.line), - severity: crate::engine::Severity::Info, - issue_type: Some("index".into()), - title: format!("[index-dead-code] Potentially dead code: {}", func.name), - body: format!( + findings.push( + ReviewIssue::new( + func.file.clone(), + Some(func.line), + crate::engine::Severity::Info, + format!("[index-dead-code] Potentially dead code: {}", func.name), + ) + .with_type("index") + .with_body(format!( "Function `{}` ({}) has no callers in the \ project. Verify it's not called via reflection, \ trait dispatch, or external entry points.", func.name, func.kind - ), - suggested_fix: None, - }); + )) + .with_rule_id("index-dead-code"), + ); } } Err(e) => { diff --git a/src/engine/inline_suppress.rs b/src/engine/inline_suppress.rs index 86ad044..a7b46ee 100644 --- a/src/engine/inline_suppress.rs +++ b/src/engine/inline_suppress.rs @@ -140,17 +140,13 @@ mod tests { const RULE: &str = "Hardcoded password or secret in variable"; fn issue(file: &str, line: u32, title: &str) -> ReviewIssue { - ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: file.to_string(), - line: Some(line), - severity: Severity::Major, - issue_type: Some("rule".to_string()), - title: title.to_string(), - body: String::new(), - suggested_fix: None, - } + ReviewIssue::new( + file.to_string(), + Some(line), + Severity::Major, + title.to_string(), + ) + .with_type("rule") } /// Diff adding `lines` to `f.rs` starting at line 1. diff --git a/src/engine/mod.rs b/src/engine/mod.rs index 1e12326..63ea8e5 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -15,12 +15,15 @@ pub mod llm; pub mod markdown; pub mod memory; pub mod path_match; +pub mod postprocess; pub mod profiles; pub mod quality_gate; pub mod review; pub mod review_store; pub mod rules; pub mod scanner; +#[cfg(test)] +mod secret_heuristics_tests; pub mod secret_patterns; pub mod secrets_scanner; pub mod security_scanner; diff --git a/src/engine/postprocess.rs b/src/engine/postprocess.rs new file mode 100644 index 0000000..7489822 --- /dev/null +++ b/src/engine/postprocess.rs @@ -0,0 +1,1062 @@ +//! Finding post-processing shared by `cora review` and `cora scan` (#610). +//! +//! [`postprocess`] is the single place where raw findings (LLM issues plus +//! deterministic scanner findings) become the final, user-visible list. Fixing +//! a false-positive filter here fixes it for every command. +//! +//! The [`Source`] says what the findings were produced from: +//! +//! - [`Source::Diff`]: a parsed diff (`cora review`, including its +//! LLM-failure fallback). Callers merge the deterministic findings +//! (`Deterministic::merge_into`) before calling, because that step needs the +//! index bridge and runs before the file-path hallucination guard. +//! - [`Source::Files`]: whole files (`cora scan`). Every line counts as added. +//! Postprocessing itself runs the secrets and security scanners over the +//! files and merges them into the issues (same-topic alias merge). +//! +//! Filter order and applicability: +//! +//! | # | Filter | Diff | Files | +//! |---|-----------------------------------|--------------------|-------| +//! | 1 | scanner merge (secrets, security) | no (caller merges) | yes | +//! | 2 | LLM hardcoded-secret FP check | yes | no | +//! | 3 | Markdown fenced code block | yes | no | +//! | 4 | `ignore.rules` | yes | yes | +//! | 5 | inline `cora-ignore:` markers | yes | yes | +//! | 6 | context-line severity filter | yes | no | +//! +//! Filters 2, 3 and 6 reason about added vs unchanged diff lines (or LLM +//! mistakes about added lines), which a full-file scan does not have: every +//! line of a scanned file is "added", and scan findings come from the +//! deterministic scanners rather than from an LLM judging a diff. + +use tracing::debug; + +use crate::config::schema::Config; +use crate::engine::diff_parser::FileChunk; +use crate::engine::scanner::{FileEntry, files_as_chunks}; +use crate::engine::types::{ReviewIssue, Severity}; +use crate::engine::{inline_suppress, rules, secrets_scanner, security_scanner}; + +/// What the findings being post-processed were produced from. +pub enum Source<'a> { + /// A parsed diff (`cora review`). + Diff(&'a [FileChunk]), + /// Whole files (`cora scan`). + Files(&'a [FileEntry]), +} + +/// Run the shared post-processing pipeline over `issues`. See the module docs +/// for the per-source filter table. +pub fn postprocess( + issues: Vec, + source: &Source<'_>, + config: &Config, +) -> Vec { + match source { + Source::Diff(chunks) => { + let issues = apply_llm_secret_fp_filter(issues, chunks); + let issues = apply_markdown_code_block_filter(issues, chunks); + let issues = apply_ignore_rules(issues, &config.ignore.rules); + // Rule-scoped inline suppression (`cora-ignore: `, #554), + // shared by deterministic and LLM findings. + let issues = inline_suppress::apply(issues, chunks); + // Drop low-severity findings on unchanged (context) lines (#507). + apply_context_line_filter(issues, chunks) + } + Source::Files(files) => { + let chunks = files_as_chunks(files); + let max = config.rules_config.max_findings; + let mut merged = issues; + for family in [ + secrets_scanner::scan_secrets(&chunks, max), + security_scanner::scan_security(&chunks, max), + ] { + if !family.is_empty() { + merged = rules::merge_rule_findings(merged, family); + } + } + let merged = apply_ignore_rules(merged, &config.ignore.rules); + inline_suppress::apply(merged, &chunks) + } + } +} + +/// Filter out LLM findings about hardcoded secrets/passwords that point to +/// diff lines which don't actually contain a literal string assignment. +/// +/// The LLM sometimes flags struct field declarations like `api_key: String` +/// or `api_key: extract_api_key.clone()` as "Hardcoded password or secret in +/// variable". These are identifiers, not hardcoded values. +/// +/// This function cross-validates each security finding against the actual +/// added line in the diff. If the line doesn't match the `sec-hardcoded-secret` +/// regex (i.e. no `password/key/secret = "literal"` pattern), the finding is +/// removed as a false positive. +fn apply_llm_secret_fp_filter( + mut issues: Vec, + diff_chunks: &[FileChunk], +) -> Vec { + use crate::engine::diff_parser::DiffLineType; + + // Keywords that indicate an LLM finding is about hardcoded secrets. + static SECRET_KEYWORDS: &[&str] = &[ + "hardcoded password", + "hardcoded secret", + "hardcoded credential", + "hardcoded token", + "hardcoded api key", + "hardcoded api_key", + ]; + + // Pre-compute a lookup: (file_path, new_line_no) -> line content + let added_lines: std::collections::HashMap<(String, u32), &str> = diff_chunks + .iter() + .flat_map(|chunk| { + let path = chunk + .new_path + .as_deref() + .or(chunk.old_path.as_deref()) + .unwrap_or("unknown"); + chunk.chunks.iter().flat_map(|hunk| { + hunk.lines + .iter() + .filter(|l| l.line_type == DiffLineType::Add) + .filter_map(|l| { + l.new_line_no + .map(|ln| ((path.to_string(), ln), l.content.as_str())) + }) + }) + }) + .collect(); + + let before = issues.len(); + issues.retain(|issue| { + // Only check security-type findings about secrets + let issue_type = issue.issue_type.as_deref().unwrap_or(""); + let title_lower = issue.title.to_lowercase(); + + if issue_type != "security" { + return true; + } + + let is_secret_finding = SECRET_KEYWORDS.iter().any(|kw| title_lower.contains(kw)); + if !is_secret_finding { + return true; + } + + // Look up the actual diff line + let line_num = issue.line.unwrap_or(0); + let key = (issue.file.clone(), line_num); + if let Some(actual_line) = added_lines.get(&key) { + if !crate::engine::rules::builtin::has_secret_literal(actual_line) { + debug!( + file = %issue.file, + line = line_num, + title = %issue.title, + "suppressed LLM false positive: line has no hardcoded secret literal" + ); + return false; // Remove this finding + } + } + + // If we can't find the line (hallucinated path/line or context line), + // keep the finding — better safe than sorry. + true + }); + + let filtered = before - issues.len(); + if filtered > 0 { + debug!( + filtered, + remaining = issues.len(), + "filtered LLM false positives for hardcoded secret findings" + ); + } + + issues +} + +/// Drop findings located inside Markdown fenced code blocks (#329). +/// +/// Code blocks in `.md`/`.mdx`/`.markdown` files are documentation examples, +/// not executable code — e.g. a `git push` inside a ```bash block must not be +/// flagged as SQL injection. Findings without a resolvable line, or in files +/// without any code block, are kept unchanged (safe default). +fn apply_markdown_code_block_filter( + mut issues: Vec, + diff_chunks: &[FileChunk], +) -> Vec { + use crate::engine::markdown::{is_markdown, lines_inside_code_blocks}; + use std::collections::HashSet; + + // Build file path -> set of code-block line numbers, for markdown files only. + let mut code_block_lines: std::collections::HashMap> = + std::collections::HashMap::new(); + for chunk in diff_chunks { + let path = chunk + .new_path + .as_deref() + .or(chunk.old_path.as_deref()) + .unwrap_or(""); + if !is_markdown(path) { + continue; + } + let set = lines_inside_code_blocks(chunk); + if !set.is_empty() { + code_block_lines + .entry(path.to_string()) + .or_default() + .extend(set); + } + } + + if code_block_lines.is_empty() { + return issues; // no markdown code blocks in this diff — fast path + } + + let before = issues.len(); + issues.retain(|issue| { + let Some(ln) = issue.line else { + return true; // keep findings without a concrete line number + }; + match code_block_lines.get(&issue.file) { + Some(lines) => !lines.contains(&ln), // drop if inside a code block + None => true, + } + }); + + let dropped = before - issues.len(); + if dropped > 0 { + debug!( + dropped, + remaining = issues.len(), + "removed markdown code-block false positives" + ); + } + + issues +} + +/// Filter out issues whose `issue_type` matches any ignored rule pattern. +fn apply_ignore_rules(mut issues: Vec, ignore_rules: &[String]) -> Vec { + if ignore_rules.is_empty() { + return issues; + } + + let before = issues.len(); + issues.retain(|issue| { + !ignore_rules.iter().any(|pattern| { + let pattern_lower = pattern.to_lowercase(); + let issue_type_lower = issue.issue_type.clone().unwrap_or_default().to_lowercase(); + let exact = pattern.trim().to_lowercase(); + let id_hit = |s: &str| s.trim().to_lowercase() == exact; + issue_type_lower.contains(&pattern_lower) + || issue.title.to_lowercase().contains(&pattern_lower) + // Rule ids match exactly, including ids of scanner findings + // merged into this issue (#597). + || issue.rule_id.as_deref().is_some_and(id_hit) + || issue.also_matches.iter().any(|a| id_hit(a)) + }) + }); + let filtered = before - issues.len(); + if filtered > 0 { + debug!( + filtered, + remaining = issues.len(), + rules = ignore_rules.len(), + "filtered issues via ignore rules" + ); + } + + issues +} + +/// Drop findings on unchanged (context) or removed lines (#507 Pattern #3). +/// +/// The LLM sometimes flags pre-existing code that appears in the diff purely +/// because surrounding lines changed. These findings are not about code the PR +/// introduces — they are noise. +/// +/// **Policy:** Only drop `Minor` and `Info` severity findings on context/removed +/// lines. `Critical` and `Major` findings are kept regardless, because they may +/// represent real risks worth surfacing even in pre-existing code. +fn apply_context_line_filter( + mut issues: Vec, + diff_chunks: &[FileChunk], +) -> Vec { + use crate::engine::diff_parser::DiffLineType; + + // Build lookup: (file, new_line_no) -> is_added + // Only includes lines present in the diff (Add or Context). Lines not in + // the diff at all are left alone (LLM line numbers can be imprecise). + let mut line_kinds: std::collections::HashMap<(String, u32), DiffLineType> = + std::collections::HashMap::new(); + for chunk in diff_chunks { + let path = chunk + .new_path + .as_deref() + .or(chunk.old_path.as_deref()) + .unwrap_or(""); + for hunk in &chunk.chunks { + for line in &hunk.lines { + if let Some(ln) = line.new_line_no { + line_kinds.insert((path.to_string(), ln), line.line_type); + } + } + } + } + + let before = issues.len(); + issues.retain(|issue| { + // Keep findings without a concrete line number + let Some(ln) = issue.line else { + return true; + }; + + // Only filter if we can resolve this (file, line) to a diff line + let Some(kind) = line_kinds.get(&(issue.file.clone(), ln)) else { + return true; // not in diff — can't determine, keep + }; + + match kind { + DiffLineType::Add => true, // genuinely new code — always keep + DiffLineType::Context | DiffLineType::Remove => { + // Pre-existing code — only keep if severity is high enough + // Ord: Critical(0) < Major(1) < Minor(2) < Info(3) + issue.severity <= Severity::Major + } + } + }); + + let dropped = before - issues.len(); + if dropped > 0 { + debug!( + dropped, + remaining = issues.len(), + "removed low-severity findings on unchanged diff context lines (#507)" + ); + } + + issues +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn secret_fp_filter_removes_struct_field_declarations() { + use crate::engine::diff_parser::*; + + // Simulate a diff with a struct field declaration (not a hardcoded secret) + let diff_chunks = vec![FileChunk { + old_path: None, + new_path: Some("crates/uteke-cli/src/cli.rs".to_string()), + language: "rs".to_string(), + chunks: vec![DiffHunk { + old_start: 230, + old_count: 0, + new_start: 234, + new_count: 2, + header: "".to_string(), + lines: vec![ + DiffLine { + line_type: DiffLineType::Add, + content: " extract_api_key: Option,".to_string(), + old_line_no: None, + new_line_no: Some(236), + }, + DiffLine { + line_type: DiffLineType::Add, + content: " extract_base_url: Option,".to_string(), + old_line_no: None, + new_line_no: Some(237), + }, + ], + }], + is_binary: false, + is_deleted: false, + is_new: false, + }]; + + let issues = vec![ + ReviewIssue::new( + "crates/uteke-cli/src/cli.rs", + Some(236), + Severity::Critical, + "Hardcoded password or secret in variable", + ) + .with_type("security") + .with_body("Static security scanner detected..."), + ]; + + let result = apply_llm_secret_fp_filter(issues, &diff_chunks); + assert!( + result.is_empty(), + "struct field declaration should be filtered out" + ); + } + + #[test] + fn secret_fp_filter_keeps_actual_hardcoded_secrets() { + use crate::engine::diff_parser::*; + + let diff_chunks = vec![FileChunk { + old_path: None, + new_path: Some("src/config.rs".to_string()), + language: "rs".to_string(), + chunks: vec![DiffHunk { + old_start: 10, + old_count: 0, + new_start: 15, + new_count: 1, + header: "".to_string(), + lines: vec![DiffLine { + line_type: DiffLineType::Add, + content: " let api_key = \"sk-12345abcdef\";".to_string(), + old_line_no: None, + new_line_no: Some(15), + }], + }], + is_binary: false, + is_deleted: false, + is_new: false, + }]; + + let issues = vec![ + ReviewIssue::new( + "src/config.rs", + Some(15), + Severity::Critical, + "Hardcoded password or secret in variable", + ) + .with_type("security") + .with_body("API key hardcoded..."), + ]; + + let result = apply_llm_secret_fp_filter(issues, &diff_chunks); + assert_eq!(result.len(), 1, "actual hardcoded secret should be kept"); + } + + /// LLM findings on the shapes added in #618-#620 are kept; SQL placeholders + /// and references are still dropped. + #[test] + fn secret_fp_filter_matches_go_yaml_json_sql_shapes() { + use crate::engine::diff_parser::*; + + let cases: &[(&str, bool)] = &[ + (r#"password := "hunter2hunter2""#, true), + (r#"var apiKey string = "abcd1234efgh5678""#, true), + ("password: hunter2hunter2", true), + (r#" "password": "hunter2hunter2","#, true), + ("CREATE USER app WITH PASSWORD 'hunter2hunter2';", true), + ("CREATE USER app WITH PASSWORD '%s';", false), + ("password: required", false), + (r#" "password": "${DB_PASSWORD}","#, false), + ]; + for (line, keep) in cases { + let chunks = vec![FileChunk { + old_path: None, + new_path: Some("src/x.txt".to_string()), + language: "txt".to_string(), + chunks: vec![DiffHunk { + old_start: 0, + old_count: 0, + new_start: 1, + new_count: 1, + header: "".to_string(), + lines: vec![DiffLine { + line_type: DiffLineType::Add, + content: line.to_string(), + old_line_no: None, + new_line_no: Some(1), + }], + }], + is_binary: false, + is_deleted: false, + is_new: false, + }]; + let issues = vec![ + ReviewIssue::new( + "src/x.txt", + Some(1), + Severity::Critical, + "Hardcoded password in config", + ) + .with_type("security"), + ]; + let kept = apply_llm_secret_fp_filter(issues, &chunks).len() == 1; + assert_eq!(kept, *keep, "{line}"); + } + } + + #[test] + fn secret_fp_filter_keeps_non_security_findings() { + use crate::engine::diff_parser::*; + + let diff_chunks = vec![FileChunk { + old_path: None, + new_path: Some("src/main.rs".to_string()), + language: "rs".to_string(), + chunks: vec![DiffHunk { + old_start: 1, + old_count: 0, + new_start: 1, + new_count: 1, + header: "".to_string(), + lines: vec![DiffLine { + line_type: DiffLineType::Add, + content: " api_key: String,".to_string(), + old_line_no: None, + new_line_no: Some(1), + }], + }], + is_binary: false, + is_deleted: false, + is_new: false, + }]; + + let issues = vec![ + ReviewIssue::new("src/main.rs", Some(1), Severity::Minor, "Use of unwrap()") + .with_type("bugs") + .with_body("This can panic"), + ]; + + let result = apply_llm_secret_fp_filter(issues, &diff_chunks); + assert_eq!(result.len(), 1, "non-security findings should pass through"); + } + + #[test] + fn secret_fp_filter_keeps_findings_with_unknown_lines() { + use crate::engine::diff_parser::*; + + // Empty diff — finding references a line not in the diff + let diff_chunks: Vec = vec![]; + + let issues = vec![ + ReviewIssue::new( + "src/config.rs", + Some(999), + Severity::Critical, + "Hardcoded password or secret in variable", + ) + .with_type("security") + .with_body("..."), + ]; + + let result = apply_llm_secret_fp_filter(issues, &diff_chunks); + assert_eq!( + result.len(), + 1, + "unknown lines should be kept (better safe than sorry)" + ); + } + + #[test] + fn ignore_rules_filters_by_title_match() { + let issues = vec![ + ReviewIssue::new( + "cli.rs", + Some(236), + Severity::Critical, + "Command injection via exec/system with dynamic input", + ) + .with_type("rule") + .with_body("Static security scanner detected..."), + ReviewIssue::new( + "main.rs", + Some(10), + Severity::Major, + "SQL injection via string concatenation", + ) + .with_type("security") + .with_body("..."), + ]; + + 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::new("test.py", Some(50), Severity::Minor, "Some style issue") + .with_type("style") + .with_body("..."), + ]; + + 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::new("f.rs", Some(1), Severity::Critical, "Any finding") + .with_type("rule") + .with_body("..."), + ]; + + let result = apply_ignore_rules(issues, &[]); + assert_eq!(result.len(), 1); + } + + #[test] + fn ignore_rules_case_insensitive() { + let issues = vec![ + ReviewIssue::new( + "f.rs", + Some(1), + Severity::Critical, + "HARDCODED password or SECRET in variable", + ) + .with_type("rule") + .with_body("..."), + ]; + + let rules = vec!["Hardcoded Password Or Secret".to_string()]; + let result = apply_ignore_rules(issues, &rules); + assert!(result.is_empty()); + } + + // ─── #329: markdown fenced code-block false positives ─── + + #[test] + fn markdown_fp_filter_drops_finding_inside_code_block() { + use crate::engine::diff_parser::*; + + // The exact #329 scenario: a `git push` inside a ```bash block in a + // markdown doc, flagged as SQL injection. + let diff_chunks = vec![FileChunk { + old_path: None, + new_path: Some("AGENT.md".to_string()), + language: "markdown".to_string(), + chunks: vec![DiffHunk { + old_start: 1, + old_count: 1, + new_start: 1, + new_count: 4, + header: String::new(), + lines: vec![ + DiffLine { + line_type: DiffLineType::Add, + content: "```bash".to_string(), + old_line_no: None, + new_line_no: Some(167), + }, + DiffLine { + line_type: DiffLineType::Add, + content: "git push origin vX.Y.Z".to_string(), + old_line_no: None, + new_line_no: Some(168), + }, + DiffLine { + line_type: DiffLineType::Add, + content: "```".to_string(), + old_line_no: None, + new_line_no: Some(169), + }, + ], + }], + is_binary: false, + is_deleted: false, + is_new: false, + }]; + + let issues = vec![ + ReviewIssue::new( + "AGENT.md", + Some(168), + Severity::Critical, + "SQL injection via string concatenation", + ) + .with_type("security") + .with_body("..."), + ]; + + let result = apply_markdown_code_block_filter(issues, &diff_chunks); + assert!( + result.is_empty(), + "finding inside a markdown code block must be dropped" + ); + } + + #[test] + fn markdown_fp_filter_keeps_finding_outside_code_block() { + use crate::engine::diff_parser::*; + + let diff_chunks = vec![FileChunk { + old_path: None, + new_path: Some("doc.md".to_string()), + language: "markdown".to_string(), + chunks: vec![DiffHunk { + old_start: 1, + old_count: 1, + new_start: 1, + new_count: 3, + header: String::new(), + lines: vec![ + DiffLine { + line_type: DiffLineType::Add, + content: "```bash".to_string(), + old_line_no: None, + new_line_no: Some(1), + }, + DiffLine { + line_type: DiffLineType::Add, + content: "echo hi".to_string(), + old_line_no: None, + new_line_no: Some(2), + }, + DiffLine { + line_type: DiffLineType::Add, + content: "```".to_string(), + old_line_no: None, + new_line_no: Some(3), + }, + ], + }], + is_binary: false, + is_deleted: false, + is_new: false, + }]; + + // Finding on line 5 (outside the block, in prose) must survive. + let issues = vec![ + ReviewIssue::new("doc.md", Some(5), Severity::Minor, "typo") + .with_type("style") + .with_body("..."), + ]; + + let result = apply_markdown_code_block_filter(issues, &diff_chunks); + assert_eq!(result.len(), 1, "finding outside a code block must be kept"); + } + + #[test] + fn markdown_fp_filter_keeps_findings_in_non_markdown_files() { + use crate::engine::diff_parser::*; + + // A real .py file is never treated as markdown, even if it has ``` text. + let diff_chunks = vec![FileChunk { + old_path: None, + new_path: Some("src/app.py".to_string()), + language: "python".to_string(), + chunks: vec![DiffHunk { + old_start: 1, + old_count: 1, + new_start: 1, + new_count: 2, + header: String::new(), + lines: vec![DiffLine { + line_type: DiffLineType::Add, + content: "eval(request.body.code)".to_string(), + old_line_no: None, + new_line_no: Some(42), + }], + }], + is_binary: false, + is_deleted: false, + is_new: false, + }]; + + let issues = vec![ + ReviewIssue::new("src/app.py", Some(42), Severity::Critical, "eval injection") + .with_type("security") + .with_body("..."), + ]; + + let result = apply_markdown_code_block_filter(issues, &diff_chunks); + assert_eq!(result.len(), 1, "non-markdown files are unaffected"); + } + + #[test] + fn llm_secret_filter_keeps_typed_and_single_quoted_literals() { + use crate::engine::diff_parser::{DiffHunk, DiffLine, DiffLineType, FileChunk}; + for (n, line) in [ + "const password: string = \"hunter2hunter2xx\";", + "password: str = 'hunter2hunter2xx'", + "let api_key = 'sk-hunter2hunter2xx';", + ] + .iter() + .enumerate() + { + let chunks = vec![FileChunk { + old_path: None, + new_path: Some("src/a.ts".to_string()), + language: "ts".to_string(), + chunks: vec![DiffHunk { + old_start: 0, + old_count: 0, + new_start: 1, + new_count: 1, + header: String::new(), + lines: vec![DiffLine { + line_type: DiffLineType::Add, + content: line.to_string(), + old_line_no: None, + new_line_no: Some(1), + }], + }], + is_binary: false, + is_deleted: false, + is_new: true, + }]; + let issues = vec![ + ReviewIssue::new( + "src/a.ts", + Some(1), + Severity::Critical, + "Hardcoded password in source code", + ) + .with_type("security"), + ]; + assert_eq!( + apply_llm_secret_fp_filter(issues, &chunks).len(), + 1, + "case {n}: {line}" + ); + } + } + + #[test] + fn ignore_rules_matches_rule_id_exactly() { + let mut a = ReviewIssue::new("a.rs", Some(1), Severity::Major, "Plain title") + .with_type("rule") + .with_rule_id("sec-hardcoded-secret"); + let kept = apply_ignore_rules(vec![a.clone()], &["SEC-Hardcoded-Secret".to_string()]); + assert!(kept.is_empty()); + // ids are exact, not substring + let kept = apply_ignore_rules(vec![a.clone()], &["hardcoded".to_string()]); + assert_eq!(kept.len(), 1); + // merged-away scanner id + a.rule_id = None; + a.also_matches = vec!["sec-hardcoded-secret".to_string()]; + let kept = apply_ignore_rules(vec![a], &["sec-hardcoded-secret".to_string()]); + assert!(kept.is_empty()); + } + + #[test] + fn markdown_fp_filter_keeps_findings_without_line_number() { + // Findings with no resolvable line are kept (safe default). + let issues = + vec![ReviewIssue::new("doc.md", None, Severity::Info, "vague").with_body("...")]; + + let result = apply_markdown_code_block_filter(issues, &[]); + assert_eq!(result.len(), 1); + } + + // ─── postprocess() at the shared interface (#610) ─── + + use crate::engine::diff_parser::parse_diff; + + fn issue(file: &str, line: Option, sev: Severity, ty: &str, title: &str) -> ReviewIssue { + ReviewIssue::new(file.to_string(), line, sev, title.to_string()).with_type(ty.to_string()) + } + + fn file_entry(path: &str, content: &str) -> FileEntry { + FileEntry { + path: path.to_string(), + content: content.to_string(), + lines: content.lines().count(), + } + } + + fn py_diff(extra: &str) -> Vec { + parse_diff(&format!( + "diff --git a/src/app.py b/src/app.py\n--- a/src/app.py\n+++ b/src/app.py\n@@ -1,2 +1,3 @@\n import os\n+password = \"hunter2hunter2\"{extra}\n print(1)\n" + )) + } + + fn py_file(extra: &str) -> FileEntry { + file_entry( + "src/app.py", + &format!("import os\npassword = \"hunter2hunter2\"{extra}\nprint(1)\n"), + ) + } + + fn scanner_title(config: &Config) -> String { + let files = [py_file("")]; + let out = postprocess(Vec::new(), &Source::Files(&files), config); + out.iter() + .find(|i| i.line == Some(2)) + .expect("scanner finding on line 2") + .title + .clone() + } + + #[test] + fn files_source_runs_deterministic_scanners() { + let files = [py_file("")]; + let out = postprocess(Vec::new(), &Source::Files(&files), &Config::default()); + assert!( + out.iter() + .any(|i| i.file == "src/app.py" && i.line == Some(2)) + ); + } + + #[test] + fn files_source_honors_cora_ignore_marker() { + let title = scanner_title(&Config::default()); + let files = [py_file(&format!(" # cora-ignore: {title}"))]; + let out = postprocess(Vec::new(), &Source::Files(&files), &Config::default()); + assert!(!out.iter().any(|i| i.line == Some(2)), "got {out:?}"); + } + + #[test] + fn files_source_honors_ignore_rules() { + let mut config = Config::default(); + config.ignore.rules = vec![scanner_title(&config)]; + let files = [py_file("")]; + let out = postprocess(Vec::new(), &Source::Files(&files), &config); + assert!(!out.iter().any(|i| i.line == Some(2)), "got {out:?}"); + } + + #[test] + fn files_source_aliases_scanner_finding_onto_same_topic_llm_issue() { + let files = [py_file("")]; + let llm = vec![issue( + "src/app.py", + Some(2), + Severity::Major, + "security", + "Hardcoded password in source", + )]; + let out = postprocess(llm, &Source::Files(&files), &Config::default()); + let on_line: Vec<_> = out.iter().filter(|i| i.line == Some(2)).collect(); + assert_eq!(on_line.len(), 1, "got {out:?}"); + assert_eq!(on_line[0].title, "Hardcoded password in source"); + assert!(!on_line[0].also_matches.is_empty()); + } + + #[test] + fn files_source_does_not_apply_diff_only_filters() { + // Markdown code block: the diff filter would drop findings inside the + // fence; a file scan keeps them. + let md = file_entry( + "README.md", + "# Doc\n```py\npassword = \"hunter2hunter2\"\n```\n", + ); + // LLM secret cross-check: a "hardcoded password" finding on a line + // without a literal is dropped for diffs but kept for file scans. + let src = file_entry("src/a.rs", "struct S {\n api_key: String,\n}\n"); + let llm = vec![ + issue("README.md", Some(3), Severity::Major, "bug", "Doc example"), + issue( + "src/a.rs", + Some(2), + Severity::Major, + "security", + "Hardcoded password or secret", + ), + ]; + let files = [md, src]; + let out = postprocess(llm, &Source::Files(&files), &Config::default()); + assert!(out.iter().any(|i| i.title == "Doc example"), "got {out:?}"); + assert!( + out.iter() + .any(|i| i.title == "Hardcoded password or secret"), + "got {out:?}" + ); + } + + #[test] + fn diff_source_honors_cora_ignore_marker() { + let files = [py_file("")]; + let scanned = postprocess(Vec::new(), &Source::Files(&files), &Config::default()); + let title = scanned + .iter() + .find(|i| i.line == Some(2)) + .unwrap() + .title + .clone(); + let chunks = py_diff(&format!(" # cora-ignore: {title}")); + let issues = vec![issue( + "src/app.py", + Some(2), + Severity::Major, + "rule", + &title, + )]; + let out = postprocess(issues, &Source::Diff(&chunks), &Config::default()); + assert!(out.is_empty(), "got {out:?}"); + } + + #[test] + fn diff_source_honors_ignore_rules() { + let chunks = py_diff(""); + let mut config = Config::default(); + config.ignore.rules = vec!["noisy".to_string()]; + let issues = vec![ + issue( + "src/app.py", + Some(2), + Severity::Major, + "style", + "Noisy finding", + ), + issue( + "src/app.py", + Some(2), + Severity::Major, + "bug", + "Real finding", + ), + ]; + let out = postprocess(issues, &Source::Diff(&chunks), &config); + assert_eq!(out.len(), 1); + assert_eq!(out[0].title, "Real finding"); + } + + #[test] + fn diff_source_applies_diff_only_filters() { + let chunks = py_diff(""); + let issues = vec![ + // line 1 is an unchanged context line: Minor dropped, Major kept + issue( + "src/app.py", + Some(1), + Severity::Minor, + "style", + "Minor on context", + ), + issue( + "src/app.py", + Some(1), + Severity::Major, + "bug", + "Major on context", + ), + // line 2 is added and holds a literal: secret finding kept + issue( + "src/app.py", + Some(2), + Severity::Critical, + "security", + "Hardcoded password", + ), + ]; + let out = postprocess(issues, &Source::Diff(&chunks), &Config::default()); + let titles: Vec<_> = out.iter().map(|i| i.title.as_str()).collect(); + assert_eq!(titles, ["Major on context", "Hardcoded password"]); + } + + #[test] + fn diff_source_drops_llm_secret_fp_on_added_line_without_literal() { + let chunks = parse_diff( + "diff --git a/src/a.rs b/src/a.rs\n--- a/src/a.rs\n+++ b/src/a.rs\n@@ -1,1 +1,2 @@\n struct S {\n+ api_key: String,\n", + ); + let issues = vec![issue( + "src/a.rs", + Some(2), + Severity::Major, + "security", + "Hardcoded password or secret", + )]; + let out = postprocess(issues, &Source::Diff(&chunks), &Config::default()); + assert!(out.is_empty(), "got {out:?}"); + } +} diff --git a/src/engine/quality_gate.rs b/src/engine/quality_gate.rs index eab3bf8..fc423cc 100644 --- a/src/engine/quality_gate.rs +++ b/src/engine/quality_gate.rs @@ -362,17 +362,8 @@ mod tests { use super::*; fn make_issue(severity: Severity, issue_type: &str) -> ReviewIssue { - ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "test.rs".to_string(), - line: Some(1), - severity, - issue_type: Some(issue_type.to_string()), - title: "test issue".to_string(), - body: String::new(), - suggested_fix: None, - } + ReviewIssue::new("test.rs", Some(1), severity, "test issue") + .with_type(issue_type.to_string()) } fn default_config() -> QualityGateConfig { diff --git a/src/engine/review.rs b/src/engine/review.rs index 4d95ffc..d6f4045 100644 --- a/src/engine/review.rs +++ b/src/engine/review.rs @@ -3,7 +3,8 @@ use tracing::{debug, instrument}; use crate::config::schema::Config; use crate::engine::llm; -use crate::engine::types::{LLMConfig, ReviewIssue, ReviewResponse, Severity}; +use crate::engine::postprocess::{Source, postprocess}; +use crate::engine::types::{LLMConfig, ReviewResponse}; /// Load a custom system prompt from a file path. /// Returns the file content, or None if the file doesn't exist, can't be read, @@ -308,10 +309,7 @@ async fn review_diff_inner( tokens_used: None, should_block: false, }; - fallback.issues = apply_markdown_code_block_filter(fallback.issues, &diff_chunks); - fallback.issues = apply_ignore_rules(fallback.issues, &config.ignore.rules); - fallback.issues = - crate::engine::inline_suppress::apply(fallback.issues, &diff_chunks); + fallback.issues = postprocess(fallback.issues, &Source::Diff(&diff_chunks), config); let min_sev = config.hook.min_severity_level(); fallback.should_block = fallback .issues @@ -342,30 +340,10 @@ async fn review_diff_inner( } } - // Cross-validate LLM security findings about hardcoded secrets against - // actual diff lines. The LLM sometimes flags struct field declarations - // (e.g. `api_key: String`) as "hardcoded secret" even when no literal - // value is present. This filter removes such false positives by checking - // the added line at the reported file:line against the built-in - // sec-hardcoded-secret regex. - response.issues = apply_llm_secret_fp_filter(response.issues, &diff_chunks); - - // Drop findings inside Markdown fenced code blocks (#329). Code blocks in - // `.md` files are documentation examples, not executable code — a `git push` - // inside a ```bash block is not SQL injection. - response.issues = apply_markdown_code_block_filter(response.issues, &diff_chunks); - - // Apply ignore rules: filter out issues matching ignored patterns - response.issues = apply_ignore_rules(response.issues, &config.ignore.rules); - - // Rule-scoped inline suppression (`cora-ignore: `, #554), shared by - // deterministic and LLM findings. - response.issues = crate::engine::inline_suppress::apply(response.issues, &diff_chunks); - - // Drop low-severity findings on unchanged (context) lines — these are - // pre-existing code that appeared in the diff due to surrounding changes, - // not new code introduced by the PR (#507 Pattern #3). - response.issues = apply_context_line_filter(response.issues, &diff_chunks); + // Shared post-processing (LLM secret FP cross-check, Markdown code blocks, + // ignore.rules, inline `cora-ignore:`, context-line filter), see + // `engine::postprocess`. + response.issues = postprocess(response.issues, &Source::Diff(&diff_chunks), config); // Calculate should_block based on min_severity let min_severity = config.hook.min_severity_level(); @@ -399,275 +377,6 @@ async fn review_diff_inner( Ok(response) } -/// Filter out LLM findings about hardcoded secrets/passwords that point to -/// diff lines which don't actually contain a literal string assignment. -/// -/// The LLM sometimes flags struct field declarations like `api_key: String` -/// or `api_key: extract_api_key.clone()` as "Hardcoded password or secret in -/// variable". These are identifiers, not hardcoded values. -/// -/// This function cross-validates each security finding against the actual -/// added line in the diff. If the line doesn't match the `sec-hardcoded-secret` -/// regex (i.e. no `password/key/secret = "literal"` pattern), the finding is -/// removed as a false positive. -fn apply_llm_secret_fp_filter( - mut issues: Vec, - diff_chunks: &[crate::engine::diff_parser::FileChunk], -) -> Vec { - use crate::engine::diff_parser::DiffLineType; - - // Lazy-compiled regex matching the built-in sec-hardcoded-secret pattern. - // Only triggers for actual value assignments like `api_key = "sk-..."`. - static RE_SECRET_LITERAL: std::sync::LazyLock = std::sync::LazyLock::new(|| { - regex::Regex::new(r#"(?i)(?:password|api_?key|token|secret)(?:\s*:\s*[&\w<>\[\].?|]+)?\s*=\s*(?:"[^"]+"|'[^']+')"#) - .expect("hardcoded secret regex must compile") - }); - - // Keywords that indicate an LLM finding is about hardcoded secrets. - static SECRET_KEYWORDS: &[&str] = &[ - "hardcoded password", - "hardcoded secret", - "hardcoded credential", - "hardcoded token", - "hardcoded api key", - "hardcoded api_key", - ]; - - // Pre-compute a lookup: (file_path, new_line_no) -> line content - let added_lines: std::collections::HashMap<(String, u32), &str> = diff_chunks - .iter() - .flat_map(|chunk| { - let path = chunk - .new_path - .as_deref() - .or(chunk.old_path.as_deref()) - .unwrap_or("unknown"); - chunk.chunks.iter().flat_map(|hunk| { - hunk.lines - .iter() - .filter(|l| l.line_type == DiffLineType::Add) - .filter_map(|l| { - l.new_line_no - .map(|ln| ((path.to_string(), ln), l.content.as_str())) - }) - }) - }) - .collect(); - - let before = issues.len(); - issues.retain(|issue| { - // Only check security-type findings about secrets - let issue_type = issue.issue_type.as_deref().unwrap_or(""); - let title_lower = issue.title.to_lowercase(); - - if issue_type != "security" { - return true; - } - - let is_secret_finding = SECRET_KEYWORDS.iter().any(|kw| title_lower.contains(kw)); - if !is_secret_finding { - return true; - } - - // Look up the actual diff line - let line_num = issue.line.unwrap_or(0); - let key = (issue.file.clone(), line_num); - if let Some(actual_line) = added_lines.get(&key) { - if !RE_SECRET_LITERAL.is_match(actual_line) { - debug!( - file = %issue.file, - line = line_num, - title = %issue.title, - "suppressed LLM false positive: line has no hardcoded secret literal" - ); - return false; // Remove this finding - } - } - - // If we can't find the line (hallucinated path/line or context line), - // keep the finding — better safe than sorry. - true - }); - - let filtered = before - issues.len(); - if filtered > 0 { - debug!( - filtered, - remaining = issues.len(), - "filtered LLM false positives for hardcoded secret findings" - ); - } - - issues -} - -/// Drop findings located inside Markdown fenced code blocks (#329). -/// -/// Code blocks in `.md`/`.mdx`/`.markdown` files are documentation examples, -/// not executable code — e.g. a `git push` inside a ```bash block must not be -/// flagged as SQL injection. Findings without a resolvable line, or in files -/// without any code block, are kept unchanged (safe default). -fn apply_markdown_code_block_filter( - mut issues: Vec, - diff_chunks: &[crate::engine::diff_parser::FileChunk], -) -> Vec { - use crate::engine::markdown::{is_markdown, lines_inside_code_blocks}; - use std::collections::HashSet; - - // Build file path -> set of code-block line numbers, for markdown files only. - let mut code_block_lines: std::collections::HashMap> = - std::collections::HashMap::new(); - for chunk in diff_chunks { - let path = chunk - .new_path - .as_deref() - .or(chunk.old_path.as_deref()) - .unwrap_or(""); - if !is_markdown(path) { - continue; - } - let set = lines_inside_code_blocks(chunk); - if !set.is_empty() { - code_block_lines - .entry(path.to_string()) - .or_default() - .extend(set); - } - } - - if code_block_lines.is_empty() { - return issues; // no markdown code blocks in this diff — fast path - } - - let before = issues.len(); - issues.retain(|issue| { - let Some(ln) = issue.line else { - return true; // keep findings without a concrete line number - }; - match code_block_lines.get(&issue.file) { - Some(lines) => !lines.contains(&ln), // drop if inside a code block - None => true, - } - }); - - let dropped = before - issues.len(); - if dropped > 0 { - debug!( - dropped, - remaining = issues.len(), - "removed markdown code-block false positives" - ); - } - - issues -} - -/// Filter out issues whose `issue_type` matches any ignored rule pattern. -pub(crate) fn apply_ignore_rules( - mut issues: Vec, - ignore_rules: &[String], -) -> Vec { - if ignore_rules.is_empty() { - return issues; - } - - let before = issues.len(); - issues.retain(|issue| { - !ignore_rules.iter().any(|pattern| { - let pattern_lower = pattern.to_lowercase(); - let issue_type_lower = issue.issue_type.clone().unwrap_or_default().to_lowercase(); - let exact = pattern.trim().to_lowercase(); - let id_hit = |s: &str| s.trim().to_lowercase() == exact; - issue_type_lower.contains(&pattern_lower) - || issue.title.to_lowercase().contains(&pattern_lower) - // Rule ids match exactly, including ids of scanner findings - // merged into this issue (#597). - || issue.rule_id.as_deref().is_some_and(id_hit) - || issue.also_matches.iter().any(|a| id_hit(a)) - }) - }); - let filtered = before - issues.len(); - if filtered > 0 { - debug!( - filtered, - remaining = issues.len(), - rules = ignore_rules.len(), - "filtered issues via ignore rules" - ); - } - - issues -} - -/// Drop findings on unchanged (context) or removed lines (#507 Pattern #3). -/// -/// The LLM sometimes flags pre-existing code that appears in the diff purely -/// because surrounding lines changed. These findings are not about code the PR -/// introduces — they are noise. -/// -/// **Policy:** Only drop `Minor` and `Info` severity findings on context/removed -/// lines. `Critical` and `Major` findings are kept regardless, because they may -/// represent real risks worth surfacing even in pre-existing code. -fn apply_context_line_filter( - mut issues: Vec, - diff_chunks: &[crate::engine::diff_parser::FileChunk], -) -> Vec { - use crate::engine::diff_parser::DiffLineType; - - // Build lookup: (file, new_line_no) -> is_added - // Only includes lines present in the diff (Add or Context). Lines not in - // the diff at all are left alone (LLM line numbers can be imprecise). - let mut line_kinds: std::collections::HashMap<(String, u32), DiffLineType> = - std::collections::HashMap::new(); - for chunk in diff_chunks { - let path = chunk - .new_path - .as_deref() - .or(chunk.old_path.as_deref()) - .unwrap_or(""); - for hunk in &chunk.chunks { - for line in &hunk.lines { - if let Some(ln) = line.new_line_no { - line_kinds.insert((path.to_string(), ln), line.line_type); - } - } - } - } - - let before = issues.len(); - issues.retain(|issue| { - // Keep findings without a concrete line number - let Some(ln) = issue.line else { - return true; - }; - - // Only filter if we can resolve this (file, line) to a diff line - let Some(kind) = line_kinds.get(&(issue.file.clone(), ln)) else { - return true; // not in diff — can't determine, keep - }; - - match kind { - DiffLineType::Add => true, // genuinely new code — always keep - DiffLineType::Context | DiffLineType::Remove => { - // Pre-existing code — only keep if severity is high enough - // Ord: Critical(0) < Major(1) < Minor(2) < Info(3) - issue.severity <= Severity::Major - } - } - }); - - let dropped = before - issues.len(); - if dropped > 0 { - debug!( - dropped, - remaining = issues.len(), - "removed low-severity findings on unchanged diff context lines (#507)" - ); - } - - issues -} - /// Check if a file path from an LLM issue matches any of the valid diff file paths. /// Uses exact match only — the LLM should report paths exactly as they appear in the diff. fn is_valid_file_path(issue_file: &str, valid_files: &[String]) -> bool { @@ -915,7 +624,6 @@ pub(crate) fn build_scan_brain_context( #[cfg(test)] mod tests { use super::*; - use crate::engine::Severity; #[test] fn resolve_prompt_inline_takes_priority() { @@ -954,516 +662,4 @@ mod tests { "system_prompt_file outside project root should be rejected" ); } - - #[test] - fn secret_fp_filter_removes_struct_field_declarations() { - use crate::engine::diff_parser::*; - - // Simulate a diff with a struct field declaration (not a hardcoded secret) - let diff_chunks = vec![FileChunk { - old_path: None, - new_path: Some("crates/uteke-cli/src/cli.rs".to_string()), - language: "rs".to_string(), - chunks: vec![DiffHunk { - old_start: 230, - old_count: 0, - new_start: 234, - new_count: 2, - header: "".to_string(), - lines: vec![ - DiffLine { - line_type: DiffLineType::Add, - content: " extract_api_key: Option,".to_string(), - old_line_no: None, - new_line_no: Some(236), - }, - DiffLine { - line_type: DiffLineType::Add, - content: " extract_base_url: Option,".to_string(), - old_line_no: None, - new_line_no: Some(237), - }, - ], - }], - is_binary: false, - is_deleted: false, - is_new: false, - }]; - - let issues = vec![ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "crates/uteke-cli/src/cli.rs".to_string(), - line: Some(236), - severity: Severity::Critical, - issue_type: Some("security".to_string()), - title: "Hardcoded password or secret in variable".to_string(), - body: "Static security scanner detected...".to_string(), - suggested_fix: None, - }]; - - let result = apply_llm_secret_fp_filter(issues, &diff_chunks); - assert!( - result.is_empty(), - "struct field declaration should be filtered out" - ); - } - - #[test] - fn secret_fp_filter_keeps_actual_hardcoded_secrets() { - use crate::engine::diff_parser::*; - - let diff_chunks = vec![FileChunk { - old_path: None, - new_path: Some("src/config.rs".to_string()), - language: "rs".to_string(), - chunks: vec![DiffHunk { - old_start: 10, - old_count: 0, - new_start: 15, - new_count: 1, - header: "".to_string(), - lines: vec![DiffLine { - line_type: DiffLineType::Add, - content: " let api_key = \"sk-12345abcdef\";".to_string(), - old_line_no: None, - new_line_no: Some(15), - }], - }], - is_binary: false, - is_deleted: false, - is_new: false, - }]; - - let issues = vec![ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "src/config.rs".to_string(), - line: Some(15), - severity: Severity::Critical, - issue_type: Some("security".to_string()), - title: "Hardcoded password or secret in variable".to_string(), - body: "API key hardcoded...".to_string(), - suggested_fix: None, - }]; - - let result = apply_llm_secret_fp_filter(issues, &diff_chunks); - assert_eq!(result.len(), 1, "actual hardcoded secret should be kept"); - } - - #[test] - fn secret_fp_filter_keeps_non_security_findings() { - use crate::engine::diff_parser::*; - - let diff_chunks = vec![FileChunk { - old_path: None, - new_path: Some("src/main.rs".to_string()), - language: "rs".to_string(), - chunks: vec![DiffHunk { - old_start: 1, - old_count: 0, - new_start: 1, - new_count: 1, - header: "".to_string(), - lines: vec![DiffLine { - line_type: DiffLineType::Add, - content: " api_key: String,".to_string(), - old_line_no: None, - new_line_no: Some(1), - }], - }], - is_binary: false, - is_deleted: false, - is_new: false, - }]; - - let issues = vec![ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "src/main.rs".to_string(), - line: Some(1), - severity: Severity::Minor, - issue_type: Some("bugs".to_string()), - title: "Use of unwrap()".to_string(), - body: "This can panic".to_string(), - suggested_fix: None, - }]; - - let result = apply_llm_secret_fp_filter(issues, &diff_chunks); - assert_eq!(result.len(), 1, "non-security findings should pass through"); - } - - #[test] - fn secret_fp_filter_keeps_findings_with_unknown_lines() { - use crate::engine::diff_parser::*; - - // Empty diff — finding references a line not in the diff - let diff_chunks: Vec = vec![]; - - let issues = vec![ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "src/config.rs".to_string(), - line: Some(999), - severity: Severity::Critical, - issue_type: Some("security".to_string()), - title: "Hardcoded password or secret in variable".to_string(), - body: "...".to_string(), - suggested_fix: None, - }]; - - let result = apply_llm_secret_fp_filter(issues, &diff_chunks); - assert_eq!( - result.len(), - 1, - "unknown lines should be kept (better safe than sorry)" - ); - } - - #[test] - fn ignore_rules_filters_by_title_match() { - let issues = vec![ - ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - 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 { - rule_id: None, - also_matches: Vec::new(), - 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 { - rule_id: None, - also_matches: Vec::new(), - 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 { - rule_id: None, - also_matches: Vec::new(), - 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 { - rule_id: None, - also_matches: Vec::new(), - 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()); - } - - // ─── #329: markdown fenced code-block false positives ─── - - #[test] - fn markdown_fp_filter_drops_finding_inside_code_block() { - use crate::engine::diff_parser::*; - - // The exact #329 scenario: a `git push` inside a ```bash block in a - // markdown doc, flagged as SQL injection. - let diff_chunks = vec![FileChunk { - old_path: None, - new_path: Some("AGENT.md".to_string()), - language: "markdown".to_string(), - chunks: vec![DiffHunk { - old_start: 1, - old_count: 1, - new_start: 1, - new_count: 4, - header: String::new(), - lines: vec![ - DiffLine { - line_type: DiffLineType::Add, - content: "```bash".to_string(), - old_line_no: None, - new_line_no: Some(167), - }, - DiffLine { - line_type: DiffLineType::Add, - content: "git push origin vX.Y.Z".to_string(), - old_line_no: None, - new_line_no: Some(168), - }, - DiffLine { - line_type: DiffLineType::Add, - content: "```".to_string(), - old_line_no: None, - new_line_no: Some(169), - }, - ], - }], - is_binary: false, - is_deleted: false, - is_new: false, - }]; - - let issues = vec![ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "AGENT.md".to_string(), - line: Some(168), - severity: Severity::Critical, - issue_type: Some("security".to_string()), - title: "SQL injection via string concatenation".to_string(), - body: "...".to_string(), - suggested_fix: None, - }]; - - let result = apply_markdown_code_block_filter(issues, &diff_chunks); - assert!( - result.is_empty(), - "finding inside a markdown code block must be dropped" - ); - } - - #[test] - fn markdown_fp_filter_keeps_finding_outside_code_block() { - use crate::engine::diff_parser::*; - - let diff_chunks = vec![FileChunk { - old_path: None, - new_path: Some("doc.md".to_string()), - language: "markdown".to_string(), - chunks: vec![DiffHunk { - old_start: 1, - old_count: 1, - new_start: 1, - new_count: 3, - header: String::new(), - lines: vec![ - DiffLine { - line_type: DiffLineType::Add, - content: "```bash".to_string(), - old_line_no: None, - new_line_no: Some(1), - }, - DiffLine { - line_type: DiffLineType::Add, - content: "echo hi".to_string(), - old_line_no: None, - new_line_no: Some(2), - }, - DiffLine { - line_type: DiffLineType::Add, - content: "```".to_string(), - old_line_no: None, - new_line_no: Some(3), - }, - ], - }], - is_binary: false, - is_deleted: false, - is_new: false, - }]; - - // Finding on line 5 (outside the block, in prose) must survive. - let issues = vec![ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "doc.md".to_string(), - line: Some(5), - severity: Severity::Minor, - issue_type: Some("style".to_string()), - title: "typo".to_string(), - body: "...".to_string(), - suggested_fix: None, - }]; - - let result = apply_markdown_code_block_filter(issues, &diff_chunks); - assert_eq!(result.len(), 1, "finding outside a code block must be kept"); - } - - #[test] - fn markdown_fp_filter_keeps_findings_in_non_markdown_files() { - use crate::engine::diff_parser::*; - - // A real .py file is never treated as markdown, even if it has ``` text. - let diff_chunks = vec![FileChunk { - old_path: None, - new_path: Some("src/app.py".to_string()), - language: "python".to_string(), - chunks: vec![DiffHunk { - old_start: 1, - old_count: 1, - new_start: 1, - new_count: 2, - header: String::new(), - lines: vec![DiffLine { - line_type: DiffLineType::Add, - content: "eval(request.body.code)".to_string(), - old_line_no: None, - new_line_no: Some(42), - }], - }], - is_binary: false, - is_deleted: false, - is_new: false, - }]; - - let issues = vec![ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "src/app.py".to_string(), - line: Some(42), - severity: Severity::Critical, - issue_type: Some("security".to_string()), - title: "eval injection".to_string(), - body: "...".to_string(), - suggested_fix: None, - }]; - - let result = apply_markdown_code_block_filter(issues, &diff_chunks); - assert_eq!(result.len(), 1, "non-markdown files are unaffected"); - } - - #[test] - fn llm_secret_filter_keeps_typed_and_single_quoted_literals() { - use crate::engine::diff_parser::{DiffHunk, DiffLine, DiffLineType, FileChunk}; - for (n, line) in [ - "const password: string = \"hunter2hunter2xx\";", - "password: str = 'hunter2hunter2xx'", - "let api_key = 'sk-hunter2hunter2xx';", - ] - .iter() - .enumerate() - { - let chunks = vec![FileChunk { - old_path: None, - new_path: Some("src/a.ts".to_string()), - language: "ts".to_string(), - chunks: vec![DiffHunk { - old_start: 0, - old_count: 0, - new_start: 1, - new_count: 1, - header: String::new(), - lines: vec![DiffLine { - line_type: DiffLineType::Add, - content: line.to_string(), - old_line_no: None, - new_line_no: Some(1), - }], - }], - is_binary: false, - is_deleted: false, - is_new: true, - }]; - let issues = vec![ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "src/a.ts".to_string(), - line: Some(1), - severity: Severity::Critical, - issue_type: Some("security".to_string()), - title: "Hardcoded password in source code".to_string(), - body: String::new(), - suggested_fix: None, - }]; - assert_eq!( - apply_llm_secret_fp_filter(issues, &chunks).len(), - 1, - "case {n}: {line}" - ); - } - } - - #[test] - fn ignore_rules_matches_rule_id_exactly() { - let mut a = ReviewIssue { - rule_id: Some("sec-hardcoded-secret".to_string()), - also_matches: Vec::new(), - file: "a.rs".to_string(), - line: Some(1), - severity: Severity::Major, - issue_type: Some("rule".to_string()), - title: "Plain title".to_string(), - body: String::new(), - suggested_fix: None, - }; - let kept = apply_ignore_rules(vec![a.clone()], &["SEC-Hardcoded-Secret".to_string()]); - assert!(kept.is_empty()); - // ids are exact, not substring - let kept = apply_ignore_rules(vec![a.clone()], &["hardcoded".to_string()]); - assert_eq!(kept.len(), 1); - // merged-away scanner id - a.rule_id = None; - a.also_matches = vec!["sec-hardcoded-secret".to_string()]; - let kept = apply_ignore_rules(vec![a], &["sec-hardcoded-secret".to_string()]); - assert!(kept.is_empty()); - } - - #[test] - fn markdown_fp_filter_keeps_findings_without_line_number() { - // Findings with no resolvable line are kept (safe default). - let issues = vec![ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "doc.md".to_string(), - line: None, - severity: Severity::Info, - issue_type: None, - title: "vague".to_string(), - body: "...".to_string(), - suggested_fix: None, - }]; - - let result = apply_markdown_code_block_filter(issues, &[]); - assert_eq!(result.len(), 1); - } } diff --git a/src/engine/review_store.rs b/src/engine/review_store.rs index bcbaeba..11ffde5 100644 --- a/src/engine/review_store.rs +++ b/src/engine/review_store.rs @@ -567,17 +567,10 @@ mod tests { } fn make_issue(file: &str, line: u32, severity: Severity, title: &str) -> ReviewIssue { - ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: file.to_string(), - line: Some(line), - severity, - issue_type: Some("security".to_string()), - title: title.to_string(), - body: "test body".to_string(), - suggested_fix: Some("fix it".to_string()), - } + ReviewIssue::new(file.to_string(), Some(line), severity, title.to_string()) + .with_type("security") + .with_body("test body") + .with_fix("fix it") } fn record<'a>(root: &'a str, issues: &'a [ReviewIssue]) -> ReviewRecord<'a> { diff --git a/src/engine/rules/builtin.rs b/src/engine/rules/builtin.rs index e584307..7de4952 100644 --- a/src/engine/rules/builtin.rs +++ b/src/engine/rules/builtin.rs @@ -11,8 +11,7 @@ pub fn builtin_rules() -> Vec { // --- Security --- CustomRule { id: "sec-hardcoded-secret".to_string(), - pattern: r#"(?i)(?:password|api_?key|token|secret)(?:\s*:\s*[&\w<>\[\].?|]+)?\s*=\s*(?:"[^"]+"|'[^']+')"# - .to_string(), + pattern: secret_literal_pattern(), severity: Severity::Critical, message: "Possible hardcoded secret/credential detected. Use environment variables or a secrets manager.".to_string(), languages: vec!["all".to_string()], @@ -126,6 +125,134 @@ pub fn builtin_rules() -> Vec { ] } +/// Regex source shared by the `sec-hardcoded-secret` rule and the LLM secret +/// cross-check in `postprocess`, so a shape detected here is never dropped +/// there as a false positive (#607, #618-#620). Alternatives: +/// +/// 1. `name [: type] = "lit"` / `name := "lit"` (Go) / `"name": "lit"` (JSON +/// quoted key); the key may be quoted. +/// 2. Go `var name type = "lit"` (colon-free type between name and `=`). +/// 3. SQL `PASSWORD ''` / `IDENTIFIED BY ''` (no `=`). +pub(crate) fn secret_literal_pattern() -> String { + let kw = r"(?:password|api_?key|token|secret)"; + let ty = r"[&*\w<>\[\].?|]+"; + let lit = r#"(?:"[^"]+"|'[^']+')"#; + format!( + r#"(?i){kw}["']?(?:\s*:\s*{ty})?\s*(?::=|=|["']\s*:)\s*{lit}|(?i){kw}["']?\s+{ty}\s*=\s*{lit}|{SQL_PASSWORD_PATTERN}"# + ) +} + +/// SQL password literal without `=`: `... WITH PASSWORD 'x'`, `IDENTIFIED BY 'x'`. +const SQL_PASSWORD_PATTERN: &str = r"(?i)\b(?:password|identified\s+by)\s+'([^']+)'"; + +static SQL_PASSWORD_RE: LazyLock = + LazyLock::new(|| Regex::new(SQL_PASSWORD_PATTERN).expect("sql password regex must compile")); + +static SECRET_LITERAL_RE: LazyLock = LazyLock::new(|| { + Regex::new(&secret_literal_pattern()).expect("hardcoded secret regex must compile") +}); + +/// Does `line` show a hardcoded secret literal the deterministic detectors +/// report? Used by the LLM cross-check so LLM findings on the same shapes are +/// kept. A SQL placeholder (`PASSWORD '%s'`) is not a literal. +pub(crate) fn has_secret_literal(line: &str) -> bool { + if let Some(c) = SQL_PASSWORD_RE.captures(line) + && is_sql_placeholder(&c[1]) + { + return false; + } + if quoted_placeholder_value(line) { + return false; + } + SECRET_LITERAL_RE.is_match(line) || plain_scalar_secret(strip_trailing_comment(line)) +} + +/// A SQL literal that is a bind/format placeholder, not a password: +/// `%s`, `%v`, `$1`, `?`, `:name`, `@name`, `{}` / `{pw}` / `${PW}`, ``. +fn is_sql_placeholder(lit: &str) -> bool { + let lit = lit.trim(); + lit.starts_with(['$', ':', '@', '?']) + || lit.contains('%') + || (lit.contains('{') && lit.contains('}')) + || (lit.starts_with('<') && lit.ends_with('>')) +} + +/// Value of a whole-line `key: value` entry (YAML, JSON, `- key: value`) whose +/// key ends in a secret word. The line must be nothing but the entry (an +/// optional `- ` bullet, an optional trailing comma), so object shorthand inside +/// code (`{ password: formPassword }`, `pub password: String,`) never qualifies. +fn secret_kv_value(line: &str) -> Option<&str> { + static KV_RE: LazyLock = LazyLock::new(|| { + Regex::new( + r#"(?i)^\s*(?:-\s+)?["']?[\w.\-]*(?:password|passwd|pwd|secret|api_?key|token)["']?\s*:\s+(\S.*?)\s*,?\s*$"#, + ) + .expect("kv regex must compile") + }); + KV_RE + .captures(line) + .and_then(|c| c.get(1)) + .map(|m| m.as_str()) +} + +/// Unquoted YAML scalar that looks like a real secret (#619). Conservative: +/// a single token of >= 8 chars with a digit or symbol in it, so bare words +/// (`required`, `formPassword`, `vault_db_password`) stay "references". Never +/// a YAML tag/anchor/alias/block indicator, `${..}`/`$VAR`, a path, or a +/// keyword (`true`, `null`, ...). +fn plain_scalar_secret(line: &str) -> bool { + let Some(v) = secret_kv_value(line) else { + return false; + }; + let lower = v.to_ascii_lowercase(); + v.len() >= 8 + && !v.contains(char::is_whitespace) + && !v.starts_with([ + '"', '\'', '`', '$', '!', '&', '*', '|', '>', '%', '@', '{', '[', '/', '~', '<', '.', + '?', '-', + ]) + && !v.contains("://") + && !v.contains(['{', '}', '(', ')', ';', '#']) + && !matches!( + lower.as_str(), + "true" | "false" | "null" | "none" | "nil" | "yes" | "no" + ) + && v.chars() + .any(|c| c.is_ascii_digit() || "!@#%^&*+=".contains(c)) +} + +/// Quoted `key: "value"` entry whose value is empty or an interpolation / +/// template placeholder (`${X}`, `$X`, `{{ x }}`): a reference, not a secret. +fn quoted_placeholder_value(line: &str) -> bool { + // Unanchored on purpose: inline JSON (`{"password": "${X}", "apiKey": ""}`) + // and call arguments carry the pair in the middle of the line. Every quoted + // secret pair on the line must be a placeholder to suppress the finding, so a + // real literal next to a placeholder is still reported. + static PAIR_RE: LazyLock = LazyLock::new(|| { + Regex::new( + r#"(?i)["']?[\w.\-]*(?:password|passwd|pwd|secret|api_?key|token)["']?\s*:\s*("[^"]*"|'[^']*')"#, + ) + .expect("pair regex must compile") + }); + let mut seen = false; + for cap in PAIR_RE.captures_iter(line) { + let v = &cap[1]; + let inner = &v[1..v.len() - 1]; + let placeholder = inner.is_empty() + || (inner.starts_with("${") && inner.ends_with('}')) + || (inner.starts_with("{{") && inner.ends_with("}}")) + || (inner.starts_with('$') + && inner.len() > 1 + && inner[1..] + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_')); + if !placeholder { + return false; + } + seen = true; + } + seen +} + /// Post-match filter for rules that need additional validation after regex match. /// Returns `true` to suppress a finding that the regex matched but should be ignored. pub fn post_match_filter(rule_id: &str, line: &str) -> bool { @@ -285,6 +412,29 @@ fn is_false_positive_secret(line: &str) -> bool { return true; } + // SQL placeholders in format strings: PASSWORD '%s', IDENTIFIED BY '$1' (#620) + if let Some(c) = SQL_PASSWORD_RE.captures(line) + && is_sql_placeholder(&c[1]) + { + return true; + } + + // `"password": "${X}"` / `password: "{{ x }}"`: a reference, not a secret (#619) + if quoted_placeholder_value(line) { + return true; + } + + // YAML alias / anchor / tag / path / `~` reference (`password: *dbpass`, + // `password: /run/secrets/db`): points at another value, not a secret (#619) + if secret_kv_value(line).is_some_and(|v| v.starts_with(['*', '&', '!', '/', '~'])) { + return true; + } + + // Unquoted YAML `password: hunter2hunter2` is a real secret, not shorthand (#619) + if plain_scalar_secret(line) { + return false; + } + // Object shorthand where the value is a variable reference, not a literal // e.g., { app_secret: formAppSecret } — RHS is a variable name, not a secret value // Variable references: no quotes, no digits mixed with special chars diff --git a/src/engine/rules/mod.rs b/src/engine/rules/mod.rs index 5ab7a1d..9c16d3c 100644 --- a/src/engine/rules/mod.rs +++ b/src/engine/rules/mod.rs @@ -242,17 +242,17 @@ pub fn merge_rule_findings( continue; } - result.push(ReviewIssue { - rule_id: Some(finding.rule_id), - also_matches: Vec::new(), - file: finding.file, - line: Some(finding.line), - severity: finding.severity, - issue_type: Some("rule".to_string()), - title: finding.title, - body: finding.body, - suggested_fix: None, - }); + result.push( + ReviewIssue::new( + finding.file, + Some(finding.line), + finding.severity, + finding.title, + ) + .with_type("rule") + .with_body(finding.body) + .with_rule_id(finding.rule_id), + ); } result @@ -452,16 +452,10 @@ mod tests { } fn llm_issue(line: u32, title: &str, kind: Option<&str>) -> ReviewIssue { - ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "src/a.rs".to_string(), - line: Some(line), - severity: Severity::Major, - issue_type: kind.map(str::to_string), - title: title.to_string(), - body: String::new(), - suggested_fix: None, + let issue = ReviewIssue::new("src/a.rs", Some(line), Severity::Major, title); + match kind { + Some(k) => issue.with_type(k), + None => issue, } } @@ -531,17 +525,16 @@ mod tests { #[test] fn merge_skips_duplicates() { - let llm = vec![ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "src/main.rs".to_string(), - line: Some(5), - severity: Severity::Minor, - issue_type: Some("bug".to_string()), - title: "Unnecessary unwrap".to_string(), - body: "Use proper error handling".to_string(), - suggested_fix: None, - }]; + let llm = vec![ + ReviewIssue::new( + "src/main.rs", + Some(5), + Severity::Minor, + "Unnecessary unwrap", + ) + .with_type("bug") + .with_body("Use proper error handling"), + ]; let rules = vec![RuleFinding { rule_id: "bug-unwrap".to_string(), @@ -567,17 +560,11 @@ mod tests { #[test] fn merge_appends_unique_rule_findings() { - let llm = vec![ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "src/main.rs".to_string(), - line: Some(5), - severity: Severity::Minor, - issue_type: Some("bug".to_string()), - title: "Some issue".to_string(), - body: "Details".to_string(), - suggested_fix: None, - }]; + let llm = vec![ + ReviewIssue::new("src/main.rs", Some(5), Severity::Minor, "Some issue") + .with_type("bug") + .with_body("Details"), + ]; let rules = vec![RuleFinding { rule_id: "bug-todo".to_string(), diff --git a/src/engine/secret_heuristics_tests.rs b/src/engine/secret_heuristics_tests.rs new file mode 100644 index 0000000..4ee588c --- /dev/null +++ b/src/engine/secret_heuristics_tests.rs @@ -0,0 +1,322 @@ +//! Table-driven, end-to-end tests for the hardcoded-secret heuristics (#610). +//! +//! Every row is one added line in a one-line diff, run through the real review +//! path: all deterministic scanners (`deterministic::run`: rule engine, secrets +//! scanner, security scanner), merged into issues, then `postprocess` (ignore +//! rules and inline `cora-ignore:` markers). A row says whether a hardcoded +//! secret finding survives. A failure prints every mismatching row. +//! +//! Rows that expose a real false positive/negative which is not fixed yet live +//! in [`KNOWN_GAPS`], asserting the CURRENT behaviour so a fix flips the test +//! and forces the row to move into [`CASES`]. + +use crate::config::schema::Config; +use crate::engine::deterministic; +use crate::engine::diff_parser::parse_diff; +use crate::engine::index_bridge::IndexBridge; +use crate::engine::postprocess::{Source, postprocess}; + +/// (file extension, added line, expect a hardcoded-secret finding) +type Row = (&'static str, &'static str, bool); + +/// Is a hardcoded-secret finding reported for `line` in a `.{ext}` file? +fn flagged(ext: &str, line: &str) -> bool { + let diff = format!( + "diff --git a/src/app.{ext} b/src/app.{ext}\n--- a/src/app.{ext}\n+++ b/src/app.{ext}\n@@ -0,0 +1 @@\n+{line}\n" + ); + let chunks = parse_diff(&diff); + let report = deterministic::run(&chunks, &Config::default(), &IndexBridge::unavailable()); + let issues = report.merge_into(Vec::new()); + postprocess(issues, &Source::Diff(&chunks), &Config::default()) + .iter() + .any(|i| { + let id = i.rule_id.as_deref().unwrap_or(""); + id.contains("hardcoded-secret") || id.starts_with("secrets/") + }) +} + +fn mismatches(rows: &[Row]) -> Vec { + rows.iter() + .filter(|(ext, line, expect)| flagged(ext, line) != *expect) + .map(|(ext, line, expect)| { + format!( + " ({ext:?}, {line:?}): expected flagged={expect}, got {}", + !expect + ) + }) + .collect() +} + +const CASES: &[Row] = &[ + // ── js / ts: quotes ── + ("js", r#"const password = "hunter2hunter2";"#, true), + ("js", "const apiKey = 'abcd1234efgh5678';", true), + ("js", "const token = `abcd1234efgh5678`;", true), + ("ts", r#"const password: string = "hunter2hunter2";"#, true), + ( + "ts", + r#" readonly secret: string = "hunter2hunter2";"#, + true, + ), + ("ts", " secret: string;", false), + // ── js / ts: trailing comments, with and without a colon ── + ( + "js", + r#"const password = "hunter2hunter2"; // rotate later"#, + true, + ), + ( + "js", + r#"const password = "hunter2hunter2"; // note: rotate later"#, + true, + ), + ( + "ts", + r#"const password: string = "hunter2hunter2"; // TODO: move to vault"#, + true, + ), + // ── cora-ignore markers ── + ( + "js", + r#"const password = "hunter2hunter2"; // cora-ignore: crypto/hardcoded-secret"#, + false, + ), + ( + "js", + r#"const password = "hunter2hunter2"; // cora-ignore: sec-hardcoded-secret"#, + false, + ), + ( + "py", + r#"password = "hunter2hunter2" # cora-ignore: crypto/hardcoded-secret, sec-hardcoded-secret"#, + false, + ), + // a bare marker (no rule list) never suppresses + ( + "js", + r#"const password = "hunter2hunter2"; // cora-ignore"#, + true, + ), + // ── js / ts: object shorthand and variable references ── + ("ts", " { app_secret: formAppSecret }", false), + ("ts", "const body = { password: formPassword };", false), + // ── js / ts: empty strings and UI bindings ── + ("js", r#"let password = "";"#, false), + ("js", "let password = '';", false), + ("ts", r#"let password = $state('');"#, false), + ( + "svelte", + "", + false, + ), + ("ts", "let secret = $state(initialSecretValue);", false), + // ── placeholder / real-looking values ── + ("js", r#"const password = "changeme-please-123";"#, true), + // ── python ── + ("py", r#"password = "hunter2hunter2""#, true), + ("py", "api_key = 'abcd1234efgh5678'", true), + ("py", r#"password: str = "hunter2hunter2""#, true), + ("py", r#"password = "hunter2hunter2" # note: temp"#, true), + ("py", r#"password = """#, false), + ("py", "password = ''", false), + // ── rust ── + ("rs", r#"let password = "hunter2hunter2";"#, true), + ("rs", r#"let password: &str = "hunter2hunter2";"#, true), + ("rs", r#"const API_KEY: &str = "abcd1234efgh5678";"#, true), + ( + "rs", + r#"let password = "hunter2hunter2"; // note: test only"#, + true, + ), + ( + "rs", + r#"let password = "hunter2hunter2"; // cora-ignore: crypto/hardcoded-secret"#, + false, + ), + ("rs", r#"let password = "";"#, false), + ("rs", " pub password: String,", false), + ("rs", " api_key: extract_api_key.clone(),", false), + // ── go ── + ("go", r#"var password = "hunter2hunter2""#, true), + ("go", "const token = `abcd1234efgh5678`", true), + ("go", r#"password := os.Getenv("DB_PASSWORD")"#, false), + ("go", r#"password := """#, false), + // #618: short declarations and colon-free types + ("go", r#"password := "hunter2hunter2""#, true), + ("go", r#"var apiKey string = "abcd1234efgh5678""#, true), + ("go", r#"password := "hunter2hunter2" // note: temp"#, true), + ( + "go", + r#"password := "hunter2hunter2" // cora-ignore: crypto/hardcoded-secret"#, + false, + ), + ("go", r#"var apiKey string = """#, false), + ("go", r#"var apiKey string = os.Getenv("API_KEY")"#, false), + // ── java / kotlin ── + ( + "java", + r#"private static final String PASSWORD = "hunter2hunter2";"#, + true, + ), + ("java", r#"String apiKey = "abcd1234efgh5678";"#, true), + ("java", r#"String password = "";"#, false), + ( + "java", + r#"String password = "hunter2hunter2"; // cora-ignore: crypto/hardcoded-secret"#, + false, + ), + ("kt", r#"val password = "hunter2hunter2""#, true), + ("kt", r#"val apiKey: String = "abcd1234efgh5678""#, true), + ("kt", r#"val password = """#, false), + // ── yaml / json ── + ("yaml", r#"password: "hunter2hunter2""#, true), + ("yaml", "api_key: 'abcd1234efgh5678'", true), + ("yaml", "password: ${DB_PASSWORD}", false), + ("yaml", "password: \"\"", false), + // #619: unquoted YAML scalars + ("yaml", "password: hunter2hunter2", true), + ("yaml", " db_password: hunter2hunter2", true), + ("yaml", "- token: abcd1234efgh5678", true), + ("yaml", "password: hunter2hunter2 # prod db", true), + ("yaml", "password: !secret db", false), + ("yaml", "password: !vault db_password_1", false), + ("yaml", "password: null", false), + ("yaml", "password: ~", false), + ("yaml", "password: required", false), + ("yaml", "password: *dbpass1", false), + ("yaml", "password: &dbpass1 hunter2", false), + ("yaml", "password: $DB_PASSWORD_1", false), + ("yaml", "password: \"${DB_PASSWORD}\"", false), + ("yaml", "password_file: /run/secrets/db_password", false), + ("yaml", "password: /run/secrets/db_password", false), + ("yaml", "secretName: db-credentials-secret", false), + ("ts", " secret: Secret1Type", true), + // #619: quoted JSON keys + ("json", r#" "password": "hunter2hunter2","#, true), + ("json", r#" "apiKey": "abcd1234efgh5678""#, true), + ("json", r#" "password": "hunter2hunter2" "#, true), + ("json", r#" "apiKey": "${API_KEY}""#, false), + ("json", r#" "password": "$DB_PASSWORD","#, false), + ("json", r#" "password": "{{ db_password }}","#, false), + ("json", r#" "password": null,"#, false), + ("json", r#" "password": "","#, false), + ("json", r#" "password": "${DB_PASSWORD}","#, false), + // inline JSON: placeholders stay quiet, a real literal beside one is still flagged + ("json", r#"{"password": "${X}"}"#, false), + ("json", r#"{"password": "${X}", "apiKey": ""}"#, false), + ("json", r#"{"password": "", "token": "${T}"}"#, false), + ( + "json", + r#"{"password": "${X}", "token": "hunter2hunter2xx"}"#, + true, + ), + ("json", r#"{"password": "hunter2hunter2xx"}"#, true), + // ── env / .properties ── + ("env", "DB_PASSWORD=hunter2hunter2", true), + ("env", "API_KEY=abcd1234efgh5678", true), + ("env", "DB_PASSWORD=", false), + ("properties", "db.password=hunter2hunter2", true), + ("properties", "db.password=", false), + // ── shell ── + ("sh", r#"export DB_PASSWORD="hunter2hunter2""#, true), + ("sh", "API_KEY='abcd1234efgh5678'", true), + ("sh", r#"password="""#, false), + ( + "sh", + r#"export DB_PASSWORD="hunter2hunter2" # note: dev"#, + true, + ), + // ── sql ── + ( + "sql", + "ALTER ROLE app SET password = 'hunter2hunter2';", + true, + ), + ("sql", "ALTER ROLE app SET password = '';", false), + // #620: PASSWORD '' without `=` + ( + "sql", + "CREATE USER app WITH PASSWORD 'hunter2hunter2';", + true, + ), + ("sql", "ALTER USER app PASSWORD 'hunter2hunter2';", true), + ( + "sql", + "CREATE USER app IDENTIFIED BY 'hunter2hunter2';", + true, + ), + ( + "sql", + "CREATE USER app WITH PASSWORD 'hunter2hunter2'; -- note: temp", + true, + ), + ("sql", "CREATE USER app WITH PASSWORD '';", false), + ("sql", "CREATE USER app WITH PASSWORD '%s';", false), + ("sql", "CREATE USER app WITH PASSWORD '$1';", false), + ("sql", "CREATE USER app WITH PASSWORD ':pw';", false), + ("sql", "CREATE USER app WITH PASSWORD '{password}';", false), + ("sql", "CREATE USER app IDENTIFIED BY '?';", false), + ("sql", "CREATE USER app WITH PASSWORD NULL;", false), + ("go", r#"q := "CREATE USER app WITH PASSWORD '%s'""#, false), + ( + "py", + r#"q = f"CREATE USER app WITH PASSWORD '{pw}'""#, + false, + ), +]; + +/// Rows exposing a real false negative/positive that is not fixed yet. Each +/// asserts the CURRENT behaviour (`expect` = what the scanner does today, with +/// the ideal result in the trailing comment). Move a row into [`CASES`] with +/// the ideal expectation when its issue is fixed. +const KNOWN_GAPS: &[Row] = &[ + // KNOWN GAP #616 - FP: env-var reads and call expressions on the RHS are reported as hardcoded secrets (ideal: not flagged) + ("js", "const token = process.env.API_TOKEN;", true), + ( + "js", + "const secret = process.env.SESSION_SECRET || fallbackValue;", + true, + ), + ("py", r#"password = os.environ["DB_PASSWORD"]"#, true), + ("py", r#"password = os.getenv("DB_PASSWORD")"#, true), + ("py", "password = get_password_from_vault()", true), + ("rs", r#"let token = std::env::var("API_TOKEN")?;"#, true), + ( + "rs", + r#"let token = std::env::var("API_TOKEN").unwrap_or_default();"#, + true, + ), + ("rs", r#"let password = String::new();"#, true), + ( + "java", + r#"String password = System.getenv("DB_PASSWORD");"#, + true, + ), + ("kt", r#"val password = System.getenv("DB_PASSWORD")"#, true), + // KNOWN GAP #617 - FP: shell-style `${VAR}`/`$VAR` interpolation in env, .properties and shell files (ideal: not flagged) + ("env", "DB_PASSWORD=${DB_PASSWORD_FROM_VAULT}", true), + ("properties", "db.password=${DB_PASSWORD}", true), + ("sh", r#"export DB_PASSWORD="$VAULT_DB_PASSWORD""#, true), +]; + +#[test] +fn hardcoded_secret_table() { + let bad = mismatches(CASES); + assert!( + bad.is_empty(), + "{} of {} rows mismatched:\n{}", + bad.len(), + CASES.len(), + bad.join("\n") + ); +} + +#[test] +fn hardcoded_secret_known_gaps_keep_current_behaviour() { + let bad = mismatches(KNOWN_GAPS); + assert!( + bad.is_empty(), + "a KNOWN_GAPS row changed behaviour (fixed? move it into CASES):\n{}", + bad.join("\n") + ); +} diff --git a/src/engine/security_scanner.rs b/src/engine/security_scanner.rs index ebf4e65..80cfd17 100644 --- a/src/engine/security_scanner.rs +++ b/src/engine/security_scanner.rs @@ -45,7 +45,11 @@ pub static PATTERNS: &[SecurityPattern] = &[ 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*[&\w<>\[\].?|]+)?\s*[=:]\s*\S{8,}", + // Alternatives: (1) `name [: type] =|:` value, `:=` only before a quoted + // literal (Go `password := "..."`, never `:= os.Getenv(..)`, #618), key + // may be quoted (JSON, #619); (2) Go `var name type = value`; (3) SQL + // `PASSWORD ''` / `IDENTIFIED BY ''` (#620). + regex: r#"(?i)(?:password|passwd|pwd|secret|api_key|apikey|token)["']?(?:\s*:\s*[&\w<>\[\].?|]+)?\s*(?:[=:]|:=\s*["'`])\s*\S{8,}|(?:password|passwd|pwd|secret|api_key|apikey|token)["']?\s+[&*\w<>\[\].?|]+\s*=\s*["'`]\S{7,}|\b(?:password|identified\s+by)\s+'[^']+'"#, severity: Severity::Critical, }, // ── Injection ── diff --git a/src/engine/types.rs b/src/engine/types.rs index 76f6e83..db9e299 100644 --- a/src/engine/types.rs +++ b/src/engine/types.rs @@ -135,6 +135,55 @@ pub struct ReviewIssue { pub also_matches: Vec, } +impl ReviewIssue { + /// Minimal issue: no type, empty body, no fix, no rule id. Chain the + /// `with_*` setters for the rest. Construction sites go through this so a + /// new field is added in one place (#610). + pub fn new( + file: impl Into, + line: Option, + severity: Severity, + title: impl Into, + ) -> Self { + Self { + file: file.into(), + line, + severity, + issue_type: None, + title: title.into(), + body: String::new(), + suggested_fix: None, + rule_id: None, + also_matches: Vec::new(), + } + } + + #[must_use] + pub fn with_type(mut self, issue_type: impl Into) -> Self { + self.issue_type = Some(issue_type.into()); + self + } + + #[must_use] + pub fn with_body(mut self, body: impl Into) -> Self { + self.body = body.into(); + self + } + + #[cfg(test)] + #[must_use] + pub fn with_fix(mut self, fix: impl Into) -> Self { + self.suggested_fix = Some(fix.into()); + self + } + + #[must_use] + pub fn with_rule_id(mut self, id: impl Into) -> Self { + self.rule_id = Some(id.into()); + self + } +} + #[cfg(test)] #[allow(clippy::float_cmp)] mod tests { @@ -391,17 +440,10 @@ mod tests { #[test] fn review_issue_roundtrip() { - let issue = ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "src/main.rs".to_string(), - line: Some(42), - severity: Severity::Critical, - issue_type: Some("security".to_string()), - title: "SQL Injection".to_string(), - body: "Details here".to_string(), - suggested_fix: Some("Use params".to_string()), - }; + let issue = ReviewIssue::new("src/main.rs", Some(42), Severity::Critical, "SQL Injection") + .with_type("security") + .with_body("Details here") + .with_fix("Use params"); let json = serde_json::to_string(&issue).unwrap(); let back: ReviewIssue = serde_json::from_str(&json).unwrap(); assert_eq!(back.file, issue.file); diff --git a/src/formatters/compact.rs b/src/formatters/compact.rs index cc2f74e..ccacb59 100644 --- a/src/formatters/compact.rs +++ b/src/formatters/compact.rs @@ -88,17 +88,9 @@ mod tests { use crate::engine::{ReviewIssue, ScanResponse, Severity}; fn sample_issue() -> ReviewIssue { - ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "src/main.rs".to_string(), - line: Some(42), - severity: Severity::Critical, - issue_type: Some("security".to_string()), - title: "SQL Injection".to_string(), - body: "Details here.".to_string(), - suggested_fix: None, - } + ReviewIssue::new("src/main.rs", Some(42), Severity::Critical, "SQL Injection") + .with_type("security") + .with_body("Details here.") } fn sample_response() -> ReviewResponse { @@ -183,17 +175,7 @@ mod tests { #[test] fn format_issue_compact_no_line() { - let issue = ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "README.md".to_string(), - line: None, - severity: Severity::Info, - issue_type: None, - title: "Typo".to_string(), - body: String::new(), - suggested_fix: None, - }; + let issue = ReviewIssue::new("README.md", None, Severity::Info, "Typo"); let line = format_issue_compact(&issue); assert!(line.contains("README.md")); // When no line, file is directly followed by ": title" not "file:N: title" diff --git a/src/formatters/json_fmt.rs b/src/formatters/json_fmt.rs index a0a3bad..d5c3546 100644 --- a/src/formatters/json_fmt.rs +++ b/src/formatters/json_fmt.rs @@ -49,17 +49,10 @@ mod tests { use crate::engine::{ReviewIssue, Severity, TokenUsage}; fn sample_issue() -> ReviewIssue { - ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "src/main.rs".to_string(), - line: Some(42), - severity: Severity::Critical, - issue_type: Some("security".to_string()), - title: "SQL Injection".to_string(), - body: "User input concatenated into query.".to_string(), - suggested_fix: Some("Use parameterized queries.".to_string()), - } + ReviewIssue::new("src/main.rs", Some(42), Severity::Critical, "SQL Injection") + .with_type("security") + .with_body("User input concatenated into query.") + .with_fix("Use parameterized queries.") } fn sample_response() -> ReviewResponse { @@ -147,17 +140,11 @@ mod tests { #[test] fn format_review_multiple_issues() { let mut response = sample_response(); - response.issues.push(ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "src/lib.rs".to_string(), - line: Some(10), - severity: Severity::Minor, - issue_type: Some("style".to_string()), - title: "Naming".to_string(), - body: "Use snake_case.".to_string(), - suggested_fix: None, - }); + response.issues.push( + ReviewIssue::new("src/lib.rs", Some(10), Severity::Minor, "Naming") + .with_type("style") + .with_body("Use snake_case."), + ); let fmt = JsonFormatter; let output = fmt.format_review(&response).unwrap(); let parsed: serde_json::Value = serde_json::from_str(&output).unwrap(); diff --git a/src/formatters/pretty.rs b/src/formatters/pretty.rs index 5e5a568..a3a5734 100644 --- a/src/formatters/pretty.rs +++ b/src/formatters/pretty.rs @@ -242,17 +242,10 @@ mod tests { use crate::engine::{ReviewIssue, ScanResponse, Severity}; fn sample_issue() -> ReviewIssue { - ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "src/main.rs".to_string(), - line: Some(42), - severity: Severity::Critical, - issue_type: Some("security".to_string()), - title: "SQL Injection".to_string(), - body: "User input is concatenated directly into SQL query.".to_string(), - suggested_fix: Some("Use parameterized queries.".to_string()), - } + ReviewIssue::new("src/main.rs", Some(42), Severity::Critical, "SQL Injection") + .with_type("security") + .with_body("User input is concatenated directly into SQL query.") + .with_fix("Use parameterized queries.") } fn sample_response() -> ReviewResponse { @@ -382,17 +375,7 @@ mod tests { #[test] fn format_issue_pretty_no_line() { - let issue = ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "README.md".to_string(), - line: None, - severity: Severity::Info, - issue_type: None, - title: "Typo".to_string(), - body: String::new(), - suggested_fix: None, - }; + let issue = ReviewIssue::new("README.md", None, Severity::Info, "Typo"); let out = format_issue_pretty(&issue, 1); assert!(out.contains("README.md")); assert!(!out.contains("README.md:")); diff --git a/src/formatters/sarif.rs b/src/formatters/sarif.rs index b98eb2a..0fa7570 100644 --- a/src/formatters/sarif.rs +++ b/src/formatters/sarif.rs @@ -154,17 +154,10 @@ mod tests { use crate::engine::{ReviewIssue, Severity}; fn sample_issue() -> ReviewIssue { - ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "src/main.rs".to_string(), - line: Some(42), - severity: Severity::Critical, - issue_type: Some("security".to_string()), - title: "SQL Injection".to_string(), - body: "User input concatenated into query.".to_string(), - suggested_fix: Some("Use parameterized queries.".to_string()), - } + ReviewIssue::new("src/main.rs", Some(42), Severity::Critical, "SQL Injection") + .with_type("security") + .with_body("User input concatenated into query.") + .with_fix("Use parameterized queries.") } fn sample_response() -> ReviewResponse { @@ -327,17 +320,8 @@ mod tests { #[test] fn sarif_issue_without_line_has_no_region() { - let issue = ReviewIssue { - rule_id: None, - also_matches: Vec::new(), - file: "src/lib.rs".to_string(), - line: None, - severity: Severity::Info, - issue_type: None, - title: "No line".to_string(), - body: "No line info.".to_string(), - suggested_fix: None, - }; + let issue = ReviewIssue::new("src/lib.rs", None, Severity::Info, "No line") + .with_body("No line info."); let response = ReviewResponse { issues: vec![issue], summary: String::new(),