diff --git a/CHANGELOG.md b/CHANGELOG.md index e9ebcd5..57a2773 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **Custom rules are validated and the docs match the schema (#684, #678).** `cora config validate` and config load now reject an invalid regex, an empty or duplicate id, an id that shadows a built-in rule, an empty message and an unknown `languages` value, naming the rule. `languages` accepts language names (`"rust"`) as well as extensions (`"rs"`); `languages` and `exclude` are optional. `cora config validate --dry-run [--path DIR]` runs each custom rule and prints how many lines it matched. `docs/configuration.md` now shows the real schema (`rules_engine.custom`); the 0.19.0 entry no longer says `custom_rules`. +- **`cora-ignore` no longer fails silently (#679).** A marker on a line with findings that names none of them (a typo, an unknown rule, or explanation text that became part of the rule name) now prints a warning on stderr naming the marker and the rule ids on that line. ` -- ` ends the rule list, so `cora-ignore: sec-hardcoded-secret -- env var name` suppresses the finding and keeps the reason in the source. - The `Cora Review` workflow now fails with an error naming each empty secret (`CORA_API_KEY`, `CORA_BASE_URL`, `CORA_MODEL`) instead of running without them. - The `Cora Review` job no longer gets cancelled on larger pull requests: its time limit is 30 minutes (the review action allows up to three LLM attempts of 10 minutes each). diff --git a/docs/configuration.md b/docs/configuration.md index 1250caa..b8cc097 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -562,6 +562,8 @@ const next = compute(); - If an LLM finding and a scanner finding on the same line describe the same problem (they share a topic word, e.g. both are about a hardcoded password/secret), only the LLM finding is reported, and a marker (or `ignore.rules` entry) naming the scanner rule's id or title still suppresses it. If the LLM finding is about something else (say SQL injection next to a hardcoded secret), both are reported and a marker only suppresses the one it names. - Scope: the same line; plus the next line when the marker line contains only a comment. - A bare `cora-ignore` (no `:` or empty rule list) suppresses nothing, so every suppression names its rule. +- The rule list runs to the end of the line (or the comment terminator). Put an explanation after ` -- `: `// cora-ignore: sec-hardcoded-secret -- env var name, not a value`. Text without the separator becomes part of the rule name. +- A marker on a line that has findings but names none of them (a typo, an unknown rule, trailing text) prints `warning: cora-ignore at FILE:LINE matched no finding ...` on stderr, listing the rule ids that are on that line. - Applies to both deterministic scanner and LLM findings, in both `cora review` and `cora scan`. Other rules on the same line and the rest of the project stay visible. Works alongside `ignore.rules` and `ignore.files`. - `cora review` reads markers from the diff (added and context lines); a marker outside the diff's context window is not seen. `cora scan` reads markers from the full file contents. - `cora scan` also runs the deterministic secrets and security scanners on every scanned file and applies `ignore.rules`, so those findings are reported even when the LLM call fails. diff --git a/src/engine/inline_suppress.rs b/src/engine/inline_suppress.rs index a98b00f..c2e1b83 100644 --- a/src/engine/inline_suppress.rs +++ b/src/engine/inline_suppress.rs @@ -13,6 +13,11 @@ //! * The marker is matched as the substring `cora-ignore:` anywhere in a line, //! so it works after `//`, `#`, `--`, `/* */`, ``, and so on. //! * A bare `cora-ignore` with no `:` and rule list never suppresses anything. +//! * The rule list runs to the end of the line (or the comment terminator), so +//! explanatory text goes after a ` -- ` separator: +//! `cora-ignore: sec-hardcoded-secret -- env var name, not a value`. +//! * A marker on a line that has findings but names none of them is reported +//! (see [`Unmatched`]) instead of silently doing nothing (#679). //! //! One filter ([`apply`]) is applied to the merged deterministic + LLM findings. //! It reads the post-change lines present in the parsed diff (added and @@ -42,6 +47,13 @@ fn parse_rules(line: &str) -> Vec { rest = &rest[..i]; } } + // `-- reason text` ends the rule list; the reason is documentation only. + if let Some(i) = rest + .find(" -- ") + .or_else(|| rest.strip_suffix(" --").map(str::len)) + { + rest = &rest[..i]; + } rest.split(',') .map(|r| r.trim().to_lowercase()) .filter(|r| !r.is_empty()) @@ -73,18 +85,51 @@ fn matches_any(issue: &ReviewIssue, rules: &HashSet) -> bool { || issue.also_matches.iter().any(|a| hit(a)) } +/// A marker that sits on a line with findings but names none of them (a typo, +/// an unknown rule, or trailing text that became part of the rule name). +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Unmatched { + pub file: String, + pub line: u32, + /// What the marker named (lowercased, as parsed). + pub named: Vec, + /// Rule ids (or titles) of the findings on the marker's target line(s). + pub findings: Vec, +} + +impl Unmatched { + pub fn message(&self) -> String { + format!( + "cora-ignore at {}:{} matched no finding (named: {}); findings there: {}. Name a rule id exactly and put any explanation after ` -- `.", + self.file, + self.line, + self.named + .iter() + .map(|n| format!("\"{n}\"")) + .collect::>() + .join(", "), + self.findings.join(", ") + ) + } +} + /// Drop findings suppressed by an inline `cora-ignore:` marker, reading the /// post-change lines of a diff (diff adapter over [`apply_lines`]). -pub fn apply(issues: Vec, chunks: &[FileChunk]) -> Vec { +pub fn apply(issues: Vec, chunks: &[FileChunk]) -> (Vec, Vec) { let files: Vec> = chunks.iter().map(ScanFile::post_image).collect(); apply_lines(issues, &files) } /// Drop findings suppressed by an inline `cora-ignore:` marker found in /// `files`' lines. -pub fn apply_lines(mut issues: Vec, files: &[ScanFile<'_>]) -> Vec { +pub fn apply_lines( + mut issues: Vec, + files: &[ScanFile<'_>], +) -> (Vec, Vec) { // (file, line) -> lowercased rules suppressed there. let mut suppressed: HashMap<(String, u32), HashSet> = HashMap::new(); + // Every marker: (file, marker line, target lines, rules). + let mut markers: Vec<(String, u32, Vec, Vec)> = Vec::new(); for file in files { let path = file.path; for &(ln, content) in &file.lines { @@ -103,16 +148,40 @@ pub fn apply_lines(mut issues: Vec, files: &[ScanFile<'_>]) -> Vec< .entry((path.to_string(), ln)) .or_default() .extend(rules.iter().cloned()); + let mut targets = vec![ln]; if is_comment_only(content) { suppressed .entry((path.to_string(), ln + 1)) .or_default() - .extend(rules); + .extend(rules.iter().cloned()); + targets.push(ln + 1); } + markers.push((path.to_string(), ln, targets, rules)); } } if suppressed.is_empty() { - return issues; + return (issues, Vec::new()); + } + + // Markers that sit on findings yet name none of them. + let mut unmatched = Vec::new(); + for (file, ln, targets, rules) in &markers { + let set: HashSet = rules.iter().cloned().collect(); + let on_target: Vec<&ReviewIssue> = issues + .iter() + .filter(|i| i.file == *file && i.line.is_some_and(|l| targets.contains(&l))) + .collect(); + if !on_target.is_empty() && !on_target.iter().any(|i| matches_any(i, &set)) { + unmatched.push(Unmatched { + file: file.clone(), + line: *ln, + named: rules.clone(), + findings: on_target + .iter() + .map(|i| i.rule_id.clone().unwrap_or_else(|| i.title.clone())) + .collect(), + }); + } } let before = issues.len(); @@ -127,7 +196,7 @@ pub fn apply_lines(mut issues: Vec, files: &[ScanFile<'_>]) -> Vec< if dropped > 0 { debug!(dropped, "suppressed findings via inline cora-ignore"); } - issues + (issues, unmatched) } #[cfg(test)] @@ -136,6 +205,14 @@ mod tests { use crate::engine::Severity; use crate::engine::diff_parser::parse_diff; + fn apply(issues: Vec, chunks: &[FileChunk]) -> Vec { + super::apply(issues, chunks).0 + } + + fn unmatched(issues: Vec, chunks: &[FileChunk]) -> Vec { + super::apply(issues, chunks).1 + } + const RULE: &str = "Hardcoded password or secret in variable"; fn issue(file: &str, line: u32, title: &str) -> ReviewIssue { @@ -380,4 +457,38 @@ mod tests { // Without the marker the finding stays. assert!(!apply(issues, &plain).is_empty()); } + + #[test] + fn a_reason_after_the_separator_is_not_part_of_the_rule_list() { + let c = diff_of(&["x(); // cora-ignore: Rule A -- env var name, not a value"]); + assert!(apply(vec![issue("f.rs", 1, "Rule A")], &c).is_empty()); + let c = diff_of(&["x(); /* cora-ignore: Rule A, Rule B -- why */"]); + assert!(apply(vec![issue("f.rs", 1, "Rule B")], &c).is_empty()); + } + + #[test] + fn trailing_text_without_a_separator_is_reported_not_silent() { + let c = diff_of(&["x(); // cora-ignore: Rule A (an env var name, not a value)"]); + let issues = vec![issue("f.rs", 1, "Rule A")]; + assert_eq!(apply(issues.clone(), &c).len(), 1, "still reported"); + let u = unmatched(issues, &c); + assert_eq!(u.len(), 1); + assert_eq!((u[0].file.as_str(), u[0].line), ("f.rs", 1)); + assert!(u[0].message().contains("--"), "{}", u[0].message()); + } + + #[test] + fn an_unknown_rule_name_is_reported() { + let c = diff_of(&["x(); // cora-ignore: no-such-rule"]); + let u = unmatched(vec![issue("f.rs", 1, "Rule A")], &c); + assert_eq!(u.len(), 1); + assert_eq!(u[0].named, vec!["no-such-rule".to_string()]); + assert_eq!(u[0].findings, vec!["Rule A".to_string()]); + } + + #[test] + fn matching_markers_and_markers_on_clean_lines_are_not_reported() { + let c = diff_of(&["x(); // cora-ignore: Rule A", "y(); // cora-ignore: stale"]); + assert!(unmatched(vec![issue("f.rs", 1, "Rule A")], &c).is_empty()); + } } diff --git a/src/engine/postprocess.rs b/src/engine/postprocess.rs index 9de3e5b..c02609c 100644 --- a/src/engine/postprocess.rs +++ b/src/engine/postprocess.rs @@ -87,7 +87,8 @@ pub fn postprocess_report( 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); + let (issues, unmatched) = inline_suppress::apply(issues, chunks); + warn_unmatched(&unmatched); // Drop low-severity findings on unchanged (context) lines (#507). apply_context_line_filter(issues, chunks) } @@ -97,13 +98,23 @@ pub fn postprocess_report( let merged = deterministic::run_files(files, config).merge_into(issues); let scan_files = scan_input::from_entries(files); let merged = apply_ignore_rules(merged, &config.ignore.rules); - inline_suppress::apply_lines(merged, &scan_files) + let (kept, unmatched) = inline_suppress::apply_lines(merged, &scan_files); + warn_unmatched(&unmatched); + kept } }; let (issues, dropped) = cap_deterministic(issues, config.rules_config.finding_cap()); PostprocessReport { issues, dropped } } +/// Tell the user about `cora-ignore` markers that suppressed nothing (#679). +/// Goes to stderr, so JSON / SARIF on stdout stays clean. +fn warn_unmatched(unmatched: &[inline_suppress::Unmatched]) { + for u in unmatched { + eprintln!("warning: {}", u.message()); + } +} + /// Keep at most `cap` deterministic findings (those carrying a `rule_id`), /// highest severity first, but never fewer than every Critical and Major one; ties keep their existing order (stable). LLM issues /// are never counted or dropped, and every kept issue keeps its position. diff --git a/tests/inline_ignore.rs b/tests/inline_ignore.rs new file mode 100644 index 0000000..ec57fe4 --- /dev/null +++ b/tests/inline_ignore.rs @@ -0,0 +1,45 @@ +//! `cora-ignore` markers must not fail silently (#679). +use std::process::Command; + +fn scan(src: &str) -> String { + let d = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(d.path().join("src")).unwrap(); + std::fs::write(d.path().join("src/a.rs"), src).unwrap(); + let out = Command::new(env!("CARGO_BIN_EXE_cora")) + .args(["scan", "--no-llm", "--format", "compact"]) + .current_dir(d.path()) + .env_clear() + .env("PATH", "/usr/bin:/bin") + .env("HOME", d.path()) + .output() + .unwrap(); + String::from_utf8_lossy(&out.stdout).to_string() + &String::from_utf8_lossy(&out.stderr) +} + +const LINE: &str = + "pub const ENV_API_KEY: &str = \"TOLE_API_KEY\"; // cora-ignore: sec-hardcoded-secret"; + +#[test] +fn trailing_text_without_a_separator_warns_and_keeps_the_finding() { + let text = scan(&format!("{LINE} (an env var name)\n")); + assert!(text.contains("matched no finding"), "{text}"); + assert!(text.contains("sec-hardcoded-secret"), "{text}"); + assert!(text.contains("Found 1 issue"), "{text}"); +} + +#[test] +fn an_unknown_rule_warns() { + let text = + scan("pub const ENV_API_KEY: &str = \"TOLE_API_KEY\"; // cora-ignore: no-such-rule\n"); + assert!( + text.contains("no-such-rule") && text.contains("matched no finding"), + "{text}" + ); +} + +#[test] +fn the_separator_suppresses_without_a_warning() { + let text = scan(&format!("{LINE} -- env var name\n")); + assert!(text.contains("Found 0 issues"), "{text}"); + assert!(!text.contains("matched no finding"), "{text}"); +}