diff --git a/CHANGELOG.md b/CHANGELOG.md index f72ad71..359580b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **Rules are test-code aware (#663).** Whole test files (`*_test.*`, `*.test.ts`, `spec/`, `fixtures/`, ... not just `tests/`) and Rust `#[cfg(test)]` modules / `#[test]` functions (also a diff hunk inside `mod tests`) are exempt from `bug-unwrap`, `bug-expect`, `bug-println`, `qual-clone`, `qual-error-ignore`, `sec-hardcoded-url` and `sec-hardcoded-secret`; the secret scanners only run their high-confidence patterns there, so a real AWS key or token in a test is still reported (injection, `eval` and other security patterns still apply inside test blocks, and a test region is only trusted when our brace matching agrees with the code's own indentation: `/* { */` comments are ignored, and a region that never closes, or that contains an item starting at or left of its own indentation, is dropped instead of exempting what follows). An attribute that carries its item on the same line (`#[cfg(test)] fn helper() {}`) no longer exempts the NEXT line, and an attribute inside a block comment is not an attribute. `bug-println` is also skipped in `main.rs`, `src/bin/`, `examples/` and `benches/`. New setting `rules_engine.include_tests: true` turns the exemptions off. On `uteke-core` (Rust) the findings drop from 2400 to 235 and the 6 Critical ones disappear; on `uteke-cloud` the 3 Major URL findings are gone. New module `engine::test_context`. + - **`rules_engine.max_findings` now defaults to 50, and Critical/Major findings are never hidden.** The old default of 5 could hide serious findings (a module with 7 critical findings showed 5) and made agents and users misread a capped list as the full picture. The cap now limits only the noise: it keeps every Critical and Major finding and fills the remaining slots with the most severe Minor/Info ones, still reporting how many were not shown. On a real Rust codebase the unfiltered list was up to 2400 findings, 88% `bug-unwrap`, so unlimited is not the default. Set `max_findings: 0` (or `null`) for no cap. - **Review is now prepare / judge / finalize with the LLM as the seam (#649).** `review::prepare` turns a diff into a `judge::Packet` (judge-visible text, assembled context, system prompt override, `valid_files`, diff chunks, deterministic findings); a `judge::Judge` returns findings (`HttpJudge` = today's LLM call, `NoJudge` = deterministic only); `review::review_with` merges, guards file paths, post-processes and decides `should_block`. Output of `cora review` is unchanged. The review pipeline now has an end-to-end test with a fake judge (no network), and the `llm` module no longer prints: spinners moved to `progress::Spinner` and `llm::run_review_prompts` / `llm::scan_files` take an events observer (the old `llm::review_diff` / `llm::review_diff_stream` are replaced by `run_review_prompts`). diff --git a/docs/configuration.md b/docs/configuration.md index 09d23e7..6f306fd 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -462,8 +462,19 @@ rules_engine: |-------|------|---------|-------------| | `enabled` | `bool` | `true` | Enable/disable the rule engine | | `max_findings` | `int` | `50` | Max deterministic findings shown per run; Critical and Major are never hidden (`0` or `null` = unlimited). See below | +| `include_tests` | `bool` | `false` | Scan test code like any other code (see "Test code" below) | | `index_skip_files` | `[string]` | *(see below)* | Glob patterns for files to skip during index scanning | +#### Test code + +Tests legitimately `unwrap()`, `println!`, clone, use `http://` fixtures and fake credentials, so by default the rules treat test code differently: + +- **What counts as test code:** whole files under test paths (`tests/`, `*_test.*`, `*.test.ts`, `spec/`, `__tests__/`, `fixtures/`, `examples/`), Rust `#[cfg(test)]` modules and `#[test]` functions (found by brace matching), and, in a diff, a hunk inside `mod tests`. +- **What is skipped there:** the built-in rules `bug-unwrap`, `bug-expect`, `bug-println`, `qual-clone`, `qual-error-ignore`, `sec-hardcoded-url` and `sec-hardcoded-secret`. The secret scanners only run their high-confidence patterns (AWS keys, GitHub tokens, private keys, ...) on test code, so a real credential committed in a test is still found. +- `bug-println` is also skipped in program entry points (`main.rs`, `src/bin/`, `examples/`, `benches/`). Other CLI modules keep the rule; use `ignore.rules` to silence it there. +- **Not exempt:** custom rules, `bug-todo`, and every other pattern. In particular, injection, `eval`, weak-crypto and similar security patterns still apply inside inline test blocks, so wrapping code in a test-looking block does not hide it. (Whole test *files* by path keep the older behavior: only high-confidence secrets are scanned.) +- Set `rules_engine.include_tests: true` to scan test code like any other code. Detection is conservative: when it cannot tell, the line stays scanned. A `#[cfg(test)]` region is only trusted when brace matching and indentation agree (block comments are skipped; a region that never closes, or has an item starting at or left of its own indentation, is ignored rather than extended over the code after it), and a diff that does not show the `mod tests` line is scanned normally. + #### What `max_findings` caps `max_findings` caps the **deterministic** findings only (rule engine, secrets, security, and index scanners; the findings that carry a `rule_id`). LLM issues are never capped. diff --git a/src/config/schema.rs b/src/config/schema.rs index 69ac767..e59df2b 100644 --- a/src/config/schema.rs +++ b/src/config/schema.rs @@ -644,6 +644,10 @@ pub struct RulesSection { pub custom: Vec, #[serde(default, skip_serializing_if = "Vec::is_empty")] pub index_skip_files: Vec, + /// Scan test code like any other code (default `false`, see + /// `RulesConfig::include_tests`, #663). + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + pub include_tests: bool, } /// File bundling configuration section for `.cora.yaml`. @@ -800,6 +804,7 @@ impl CoraFile { if let Some(re) = &self.rules_engine { config.rules_config.enabled = re.enabled; config.rules_config.max_findings = re.max_findings; + config.rules_config.include_tests = re.include_tests; if !re.custom.is_empty() { config.rules_config.custom_rules = re.custom.clone(); } @@ -1178,6 +1183,7 @@ provider: zai max_findings: 10, custom: Vec::new(), index_skip_files: vec!["my-generated/**".to_string()], + include_tests: false, }), ..Default::default() }; diff --git a/src/engine/deterministic.rs b/src/engine/deterministic.rs index ecccae6..a32f896 100644 --- a/src/engine/deterministic.rs +++ b/src/engine/deterministic.rs @@ -46,6 +46,21 @@ pub fn skip_patterns(config: &Config) -> Vec { crate::index::skip_patterns_from_config(Some(config)).unwrap_or_default() } +/// Switch test awareness off when `rules_engine.include_tests` is set (#663). +fn scan_files_for<'a>( + files: Vec>, + config: &Config, +) -> Vec> { + if config.rules_config.include_tests { + files + .into_iter() + .map(crate::engine::scan_input::ScanFile::ignoring_tests) + .collect() + } else { + files + } +} + /// Run every deterministic check on a parsed diff. pub fn run(chunks: &[FileChunk], config: &Config, bridge: &IndexBridge) -> DeterministicReport { // Scanners run uncapped: `rules_engine.max_findings` is enforced by @@ -57,11 +72,12 @@ pub fn run(chunks: &[FileChunk], config: &Config, bridge: &IndexBridge) -> Deter max_findings: 0, ..config.rules_config.clone() }; + let files = scan_files_for(crate::engine::scan_input::from_chunks(chunks), config); DeterministicReport { - rules: rules::run_rules(chunks, &uncapped_rules), - secrets: secrets_scanner::scan_secrets(chunks, max), - security: security_scanner::scan_security(chunks, max), + rules: rules::run_rules_in(&files, &uncapped_rules), + secrets: secrets_scanner::scan_secrets_in(&files, max), + security: security_scanner::scan_security_in(&files, max), index_unused: index_scanner::scan_unused_imports(bridge, chunks, max, &skip), index_dead: index_scanner::scan_dead_code_in_review(bridge, chunks, max, &skip), index_breaking: index_scanner::scan_breaking_changes(bridge, chunks, max, &skip), @@ -78,7 +94,7 @@ pub fn run_files( files: &[crate::engine::scanner::FileEntry], config: &Config, ) -> DeterministicReport { - let scan_files = crate::engine::scan_input::from_entries(files); + let scan_files = scan_files_for(crate::engine::scan_input::from_entries(files), config); let uncapped_rules = crate::engine::rules::types::RulesConfig { max_findings: 0, ..config.rules_config.clone() diff --git a/src/engine/mod.rs b/src/engine/mod.rs index f76854c..486b544 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -31,6 +31,7 @@ pub mod secret_patterns; pub mod secrets_scanner; pub mod security_scanner; pub mod static_analysis; +pub mod test_context; pub mod types; // Re-export commonly used types from other modules for convenience diff --git a/src/engine/rules/builtin.rs b/src/engine/rules/builtin.rs index d486354..7866c3e 100644 --- a/src/engine/rules/builtin.rs +++ b/src/engine/rules/builtin.rs @@ -566,6 +566,7 @@ pub fn post_match_filter(rule_id: &str, line: &str) -> bool { pub fn post_match_filter_for_path(rule_id: &str, line: &str, path: &str) -> bool { match rule_id { "sec-hardcoded-secret" | "crypto/hardcoded-secret" => is_false_positive_secret(line, path), + "bug-println" => is_cli_output_path(path), "sec-hardcoded-url" => is_false_positive_url(line), "config/cors-wildcard" => is_false_positive_cors(line), "injection/sql-concat" | "sec-sql-concat" => is_false_positive_sql_concat(line), @@ -577,6 +578,17 @@ pub fn post_match_filter_for_path(rule_id: &str, line: &str, path: &str) -> bool } } +/// `println!` is the intended output of a program's entry point: `main.rs`, +/// `src/bin/`, `examples/`, `benches/` (#663). Other files keep the rule; a +/// crate's own `commands/` modules can be silenced with `ignore.rules` or +/// `bug-println` in a profile. +fn is_cli_output_path(path: &str) -> bool { + let p = path.replace('\\', "/"); + p.rsplit('/').next() == Some("main.rs") + || p.split('/') + .any(|seg| matches!(seg, "bin" | "examples" | "benches")) +} + /// Check if an `http://` URL match is a false positive. /// /// Suppresses: XML/SVG namespaces, Docker internal hostnames, comment/docstring lines, diff --git a/src/engine/rules/mod.rs b/src/engine/rules/mod.rs index ef19ac0..74a29d6 100644 --- a/src/engine/rules/mod.rs +++ b/src/engine/rules/mod.rs @@ -31,6 +31,20 @@ pub fn run_rules(chunks: &[FileChunk], config: &RulesConfig) -> Vec /// Run all rules (built-in + custom) against file lines. The one implementation /// behind both the diff and the whole-file entry points. +/// Built-in rules that skip test code (files under test paths and inline Rust +/// test blocks) unless `rules_engine.include_tests` is set (#663): tests +/// legitimately unwrap, print, clone, use `http://` fixtures and fake secrets. +/// Real secrets in tests are still found by the high-confidence scanners. +const TEST_EXEMPT_RULES: &[&str] = &[ + "bug-unwrap", + "bug-expect", + "bug-println", + "qual-clone", + "qual-error-ignore", + "sec-hardcoded-url", + "sec-hardcoded-secret", +]; + pub fn run_rules_in(files: &[ScanFile<'_>], config: &RulesConfig) -> Vec { if !config.enabled { debug!("rule engine is disabled"); @@ -67,6 +81,14 @@ pub fn run_rules_in(files: &[ScanFile<'_>], config: &RulesConfig) -> Vec crate::engine::scanner::FileEntry { + crate::engine::scanner::FileEntry { + path: path.to_string(), + lines: content.lines().count(), + content: content.to_string(), + } + } + + fn rule_ids(entry: &crate::engine::scanner::FileEntry, cfg: &RulesConfig) -> Vec { + let files = crate::engine::scan_input::from_entries(std::slice::from_ref(entry)); + run_rules_in(&files, cfg) + .into_iter() + .map(|f| format!("{}@{}", f.rule_id, f.line)) + .collect() + } + + const RUST_WITH_TESTS: &str = "\ +fn real() { + let a = x.unwrap(); +} +#[cfg(test)] +mod tests { + #[test] + fn t() { + let b = y.unwrap(); + println!(\"debug\"); + let u = \"http://example.test/x\"; + } +} +"; + + #[test] + fn inline_rust_test_blocks_skip_the_noisy_rules() { + let entry = file_entry("src/lib.rs", RUST_WITH_TESTS); + let ids = rule_ids(&entry, &default_config()); + assert!(ids.contains(&"bug-unwrap@2".to_string()), "{ids:?}"); + assert!( + !ids.iter() + .any(|i| i.ends_with("@8") || i.ends_with("@9") || i.ends_with("@10")), + "test block lines must be exempt: {ids:?}" + ); + } + + #[test] + fn include_tests_scans_test_blocks_again() { + let entry = file_entry("src/lib.rs", RUST_WITH_TESTS); + let cfg = RulesConfig { + include_tests: true, + ..default_config() + }; + let ids = rule_ids(&entry, &cfg); + assert!(ids.contains(&"bug-unwrap@8".to_string()), "{ids:?}"); + assert!(ids.contains(&"bug-println@9".to_string()), "{ids:?}"); + } + + #[test] + fn test_paths_are_exempt_beyond_the_tests_directory() { + // `foo_test.rs` / `app.test.ts` are not under tests/, but are test files. + for path in ["src/foo_test.rs", "src/app.test.ts", "src/__tests__/a.rs"] { + let entry = file_entry(path, "fn a() { let x = y.unwrap(); }\n"); + let ids = rule_ids(&entry, &default_config()); + assert!( + !ids.iter().any(|i| i.starts_with("bug-unwrap")), + "{path}: {ids:?}" + ); + } + let entry = file_entry("src/prod.rs", "fn a() { let x = y.unwrap(); }\n"); + assert!( + rule_ids(&entry, &default_config()) + .iter() + .any(|i| i.starts_with("bug-unwrap")) + ); + } + + #[test] + fn rules_that_are_not_noise_still_fire_in_tests() { + // bug-todo is not exempt: a TODO in a test is still a TODO. + let entry = file_entry("src/foo_test.rs", "// TODO: finish\n"); + assert!( + rule_ids(&entry, &default_config()) + .iter() + .any(|i| i.starts_with("bug-todo")) + ); + } + + #[test] + fn println_is_fine_in_program_entry_points_only() { + let src = "fn main() { println!(\"hi\"); }\n"; + for ok in [ + "src/main.rs", + "crates/x/src/bin/tool.rs", + "examples/demo.rs", + ] { + let entry = file_entry(ok, src); + assert!( + !rule_ids(&entry, &default_config()) + .iter() + .any(|i| i.starts_with("bug-println")), + "{ok}" + ); + } + let entry = file_entry("src/lib.rs", "pub fn f() { println!(\"hi\"); }\n"); + assert!( + rule_ids(&entry, &default_config()) + .iter() + .any(|i| i.starts_with("bug-println")) + ); + } } diff --git a/src/engine/rules/types.rs b/src/engine/rules/types.rs index 1ccae81..2827db7 100644 --- a/src/engine/rules/types.rs +++ b/src/engine/rules/types.rs @@ -19,6 +19,13 @@ pub struct RulesConfig { /// Matches against the file path relative to the project root. #[serde(default = "default_index_skip_files")] pub index_skip_files: Vec, + /// Scan test code like any other code. By default noisy rules + /// (`bug-unwrap`, `bug-expect`, `bug-println`, `qual-clone`, + /// `sec-hardcoded-url`, `sec-hardcoded-secret`) skip test files and inline + /// Rust test blocks, and the secret/security scanners only run their + /// high-confidence patterns on inline test blocks (#663). + #[serde(default)] + pub include_tests: bool, } /// Default skip patterns for index scanners — common false-positive sources. @@ -67,6 +74,7 @@ impl Default for RulesConfig { max_findings: 50, custom_rules: Vec::new(), index_skip_files: default_index_skip_files(), + include_tests: false, } } } diff --git a/src/engine/scan_input.rs b/src/engine/scan_input.rs index c6700e1..f9e2ac7 100644 --- a/src/engine/scan_input.rs +++ b/src/engine/scan_input.rs @@ -13,10 +13,12 @@ //! - [`ScanFile::from_entry`]: every line of a scanned file (`cora scan`, MCP //! snippet check). +use std::collections::HashSet; use std::path::Path; use crate::engine::diff_parser::{DiffLineType, FileChunk}; use crate::engine::scanner::FileEntry; +use crate::engine::test_context; /// One file's worth of lines to scan. #[derive(Debug, Clone)] @@ -27,9 +29,55 @@ pub struct ScanFile<'a> { pub language: &'a str, /// `(line number, text)`. Diff lines without a number carry `0`. pub lines: Vec<(u32, &'a str)>, + /// The whole file is test code (by path: `tests/`, `*_test.*`, ...). + pub test_path: bool, + /// Lines inside inline test code (Rust `#[cfg(test)]` modules, `#[test]` + /// functions), see [`test_context`]. + pub test_lines: HashSet, } impl<'a> ScanFile<'a> { + /// Is `line` test code (whole-file test path or an inline test block)? + pub fn in_test(&self, line: u32) -> bool { + self.test_path || self.test_lines.contains(&line) + } + + /// This file with test awareness switched off + /// (`rules_engine.include_tests: true`). + pub fn ignoring_tests(mut self) -> Self { + self.test_path = false; + self.test_lines.clear(); + self + } + + /// Inline test lines of a diff chunk: Rust test blocks visible in its + /// post-image, plus every line of a hunk that sits inside `mod tests`. + fn chunk_test_lines(chunk: &FileChunk) -> HashSet { + let mut set = HashSet::new(); + let is_rust = chunk + .new_path + .as_deref() + .or(chunk.old_path.as_deref()) + .is_some_and(|p| p.ends_with(".rs")); + if !is_rust { + return set; + } + let post: Vec<(u32, &str)> = chunk + .chunks + .iter() + .flat_map(|h| &h.lines) + .filter(|l| l.line_type != DiffLineType::Remove) + .filter_map(|l| l.new_line_no.map(|n| (n, l.content.as_str()))) + .collect(); + set.extend(test_context::rust_test_lines(&post, true)); + for hunk in &chunk.chunks { + if test_context::hunk_header_is_test_module(&hunk.header) { + set.extend(hunk.lines.iter().filter_map(|l| l.new_line_no)); + } + } + set + } + /// Added lines of a diff chunk, in diff order. A missing path becomes /// `"unknown"` and a missing new-side line number becomes `0`, exactly as /// the scanners treated them before this seam existed. @@ -41,14 +89,17 @@ impl<'a> ScanFile<'a> { .filter(|l| l.line_type == DiffLineType::Add) .map(|l| (l.new_line_no.unwrap_or(0), l.content.as_str())) .collect(); + let path = chunk + .new_path + .as_deref() + .or(chunk.old_path.as_deref()) + .unwrap_or("unknown"); Self { - path: chunk - .new_path - .as_deref() - .or(chunk.old_path.as_deref()) - .unwrap_or("unknown"), + path, language: chunk.language.as_str(), lines, + test_path: test_context::is_test_path(path), + test_lines: Self::chunk_test_lines(chunk), } } @@ -62,32 +113,44 @@ impl<'a> ScanFile<'a> { .filter(|l| l.line_type != DiffLineType::Remove) .filter_map(|l| l.new_line_no.map(|n| (n, l.content.as_str()))) .collect(); + let path = chunk + .new_path + .as_deref() + .or(chunk.old_path.as_deref()) + .unwrap_or(""); Self { - path: chunk - .new_path - .as_deref() - .or(chunk.old_path.as_deref()) - .unwrap_or(""), + path, language: chunk.language.as_str(), lines, + test_path: test_context::is_test_path(path), + test_lines: Self::chunk_test_lines(chunk), } } /// Every line of a whole file, numbered from 1. The language is the file /// extension (`""` when there is none). pub fn from_entry(entry: &'a FileEntry) -> Self { + let language = Path::new(&entry.path) + .extension() + .and_then(|e| e.to_str()) + .unwrap_or(""); + let lines: Vec<(u32, &str)> = entry + .content + .lines() + .zip(1u32..) + .map(|(t, n)| (n, t)) + .collect(); + let test_lines = if language == "rs" { + test_context::rust_test_lines(&lines, false) + } else { + HashSet::new() + }; Self { path: entry.path.as_str(), - language: Path::new(&entry.path) - .extension() - .and_then(|e| e.to_str()) - .unwrap_or(""), - lines: entry - .content - .lines() - .zip(1u32..) - .map(|(t, n)| (n, t)) - .collect(), + language, + lines, + test_path: test_context::is_test_path(&entry.path), + test_lines, } } } @@ -129,4 +192,37 @@ mod tests { let post = ScanFile::post_image(&chunks[0]); assert_eq!(post.lines, vec![(1, "ctx"), (2, "added"), (3, "tail")]); } + + #[test] + fn scan_file_knows_inline_test_lines_and_test_paths() { + let e = FileEntry { + path: "src/a.rs".into(), + content: "fn a() {}\n#[cfg(test)]\nmod tests {\n fn t() {}\n}\n".into(), + lines: 5, + }; + let f = ScanFile::from_entry(&e); + assert!(!f.in_test(1)); + assert!((2..=5).all(|n| f.in_test(n))); + assert!(!f.clone().ignoring_tests().in_test(3)); + + let t = FileEntry { + path: "tests/it.rs".into(), + content: "fn a() {}\n".into(), + lines: 1, + }; + assert!( + ScanFile::from_entry(&t).in_test(1), + "whole file is test code" + ); + } + + #[test] + fn diff_hunk_inside_mod_tests_is_test_code() { + // git prints the enclosing `mod tests {` as the hunk header. + let diff = "diff --git a/a.rs b/a.rs\n--- a/a.rs\n+++ b/a.rs\n@@ -40,2 +40,3 @@ mod tests {\n fn t() {\n+ x.unwrap();\n }\n"; + let chunks = parse_diff(diff); + let added = ScanFile::added_lines(&chunks[0]); + assert_eq!(added.lines, vec![(41, " x.unwrap();")]); + assert!(added.in_test(41)); + } } diff --git a/src/engine/secrets_scanner.rs b/src/engine/secrets_scanner.rs index c29d718..968a336 100644 --- a/src/engine/secrets_scanner.rs +++ b/src/engine/secrets_scanner.rs @@ -113,6 +113,7 @@ static TEST_FIXTURE_RE: LazyLock = LazyLock::new(|| { /// /// Returns findings capped at `max_findings`. Secret values are masked in the /// message body. +#[cfg_attr(not(test), allow(dead_code))] // callers use scan_secrets_in; kept for tests/API pub fn scan_secrets(chunks: &[FileChunk], max_findings: usize) -> Vec { scan_secrets_in(&scan_input::from_chunks(chunks), max_findings) } @@ -131,7 +132,8 @@ pub fn scan_secrets_in(files: &[ScanFile<'_>], max_findings: usize) -> Vec> = Laz /// /// Only scans added lines (not removed/context) to reduce false positives. /// Returns findings sorted by severity (worst first). +#[cfg_attr(not(test), allow(dead_code))] // callers use scan_security_in; kept for tests/API pub fn scan_security(chunks: &[FileChunk], max_findings: usize) -> Vec { scan_security_in(&scan_input::from_chunks(chunks), max_findings) } @@ -201,7 +202,24 @@ pub fn scan_security_in(files: &[ScanFile<'_>], max_findings: usize) -> Vec= max_findings { + findings.sort_by_key(|f| std::cmp::Reverse(f.severity)); + return findings; + } + } + for (rule_id, name, regex, severity) in COMPILED_PATTERNS.iter() { + if in_test_block && rule_id == "crypto/hardcoded-secret" { + continue; + } if regex.is_match(content) { // Apply post-match filter to suppress known false positives if post_match_filter_for_path(rule_id, content, path) { @@ -257,55 +275,39 @@ fn scan_high_confidence_secrets( if line_no == 0 { continue; } - // Placeholder keys (e.g. AKIAIOSFODNN7EXAMPLE) are ignored inside. - if crate::engine::secret_patterns::find_high_confidence(content).is_none() { - continue; - } - findings.push(RuleFinding { - rule_id: "secrets/high-confidence-in-test-or-doc".to_string(), - file: path.to_string(), - line: line_no, - severity: Severity::Major, - title: "Credential-like secret in test/doc file".to_string(), - body: format!( - "Static security scanner detected a credential-like value in {path}:{line_no}. \ - Test/doc paths are not exempt from secret scanning; verify it is a fake." - ), - }); + high_confidence_line(path, line_no, content, findings); if findings.len() >= max_findings { return; } } } +/// The high-confidence secret check for one line of test/doc code. +fn high_confidence_line(path: &str, line_no: u32, content: &str, findings: &mut Vec) { + // Placeholder keys (e.g. AKIAIOSFODNN7EXAMPLE) are ignored inside. + if crate::engine::secret_patterns::find_high_confidence(content).is_none() { + return; + } + findings.push(RuleFinding { + rule_id: "secrets/high-confidence-in-test-or-doc".to_string(), + file: path.to_string(), + line: line_no, + severity: Severity::Major, + title: "Credential-like secret in test/doc file".to_string(), + body: format!( + "Static security scanner detected a credential-like value in {path}:{line_no}. \ + Test/doc paths are not exempt from secret scanning; verify it is a fake." + ), + }); +} + /// Check if a file path looks like a test/spec/fixture/mock/example file. /// /// Uses path-segment awareness so common words like `latest`, `aspect`, /// `attestation`, `protest` are not mistaken for test files (#87). A segment /// is any run between `/`, `_`, `-`, and `.` separators. fn is_test_file(path: &str) -> bool { - let lower = path.to_lowercase(); - for seg in lower.split(['/', '_', '-', '.']) { - if matches!( - seg, - "test" - | "tests" - | "testing" - | "tested" - | "__tests__" - | "spec" - | "specs" - | "fixture" - | "fixtures" - | "mock" - | "mocks" - | "example" - | "examples" - ) { - return true; - } - } - false + crate::engine::test_context::is_test_path(path) } /// Check if a file path is a documentation or non-code file. @@ -1378,4 +1380,68 @@ mod tests { "verify=False in docstring should be suppressed" ); } + + #[test] + fn inline_test_blocks_only_report_high_confidence_secrets() { + use crate::engine::scanner::FileEntry; + let content = "\ +fn real() {} +#[cfg(test)] +mod tests { + fn t() { + let c = Cfg { api_key: \"sk-test-123\".into() }; + let aws = \"AKIAABCDEFGHIJKLMNOP\"; + } +} +"; + let entry = FileEntry { + path: "src/lib.rs".to_string(), + lines: content.lines().count(), + content: content.to_string(), + }; + let files = crate::engine::scan_input::from_entries(std::slice::from_ref(&entry)); + let findings = scan_security_in(&files, 100); + assert!( + !findings + .iter() + .any(|f| f.rule_id == "crypto/hardcoded-secret"), + "fixture secret in a test block: {:?}", + findings + .iter() + .map(|f| (&f.rule_id, f.line)) + .collect::>() + ); + assert!( + findings + .iter() + .any(|f| f.rule_id == "secrets/high-confidence-in-test-or-doc" && f.line == 6), + "a real-looking AWS key in a test block must still be reported" + ); + } + + #[test] + fn test_looking_blocks_do_not_hide_injection_or_eval() { + use crate::engine::scanner::FileEntry; + let content = "\ +#[cfg(test)] +mod tests { + fn t() { + let q = \"SELECT * FROM t WHERE id = \" + id; + eval(user_input); + } +} +"; + let entry = FileEntry { + path: "src/lib.rs".to_string(), + lines: content.lines().count(), + content: content.to_string(), + }; + let files = crate::engine::scan_input::from_entries(std::slice::from_ref(&entry)); + let ids: Vec = scan_security_in(&files, 100) + .into_iter() + .map(|f| f.rule_id) + .collect(); + assert!(ids.contains(&"injection/sql-concat".to_string()), "{ids:?}"); + assert!(ids.contains(&"injection/eval".to_string()), "{ids:?}"); + } } diff --git a/src/engine/test_context.rs b/src/engine/test_context.rs new file mode 100644 index 0000000..696d69c --- /dev/null +++ b/src/engine/test_context.rs @@ -0,0 +1,614 @@ +//! Test-code awareness for the deterministic rules (#663). +//! +//! Tests legitimately do what rules flag: `unwrap()`, `println!`, fake +//! credentials, `http://` URLs. This module says which lines of a file are test +//! code, so noisy rules can skip them: +//! +//! - **path**: `tests/`, `*_test.*`, `*.test.ts`, `spec/`, `fixtures/`, ... a +//! whole file ([`is_test_path`]); +//! - **inline Rust tests**: `#[cfg(test)]` modules and `#[test]` functions, +//! found by brace matching ([`rust_test_lines`]); +//! - **diffs**: a hunk whose header (the nearest enclosing item git prints) is +//! `mod tests` ([`hunk_header_is_test_module`]), since the `#[cfg(test)]` +//! line itself is usually outside the hunk. +//! +//! The detection is deliberately conservative: when it cannot tell, a line is +//! NOT test code and stays scanned. `rules_engine.include_tests: true` turns the +//! exemptions off. + +use std::collections::HashSet; + +/// Does `path` look like a test/spec/fixture/mock/example file? +/// +/// Uses path-segment awareness so common words like `latest`, `aspect`, +/// `attestation`, `protest` are not mistaken for test files (#87). A segment is +/// any run between `/`, `_`, `-`, and `.` separators. +pub fn is_test_path(path: &str) -> bool { + let lower = path.to_lowercase(); + lower.split(['/', '_', '-', '.']).any(|seg| { + matches!( + seg, + "test" + | "tests" + | "testing" + | "tested" + | "__tests__" + | "spec" + | "specs" + | "fixture" + | "fixtures" + | "mock" + | "mocks" + | "example" + | "examples" + ) + }) +} + +/// Is the `@@ ... @@
` text of a hunk a test module (`mod tests {`)? +pub fn hunk_header_is_test_module(header: &str) -> bool { + let h = header.trim(); + let h = h.strip_prefix("pub ").unwrap_or(h); + h.strip_prefix("mod ").is_some_and(|rest| { + let name = rest + .split(|c: char| !(c.is_alphanumeric() || c == '_')) + .next() + .unwrap_or(""); + matches!(name, "tests" | "test" | "unit_tests" | "integration_tests") + }) +} + +/// Net `{` minus `}` on one line of Rust, ignoring braces inside string and +/// char literals and after `//`. Approximate on purpose (raw strings and +/// multi-line literals are not tracked); an error only shrinks or grows the +/// exempt region, never invents findings. +fn brace_delta(line: &str, in_block: &mut bool) -> i32 { + let mut delta = 0; + let mut chars = line.chars().peekable(); + let mut in_str = false; + while let Some(c) = chars.next() { + // `/* ... */` (may span lines): braces inside are not code. + if *in_block { + if c == '*' && chars.peek() == Some(&'/') { + chars.next(); + *in_block = false; + } + continue; + } + if in_str { + match c { + '\\' => { + chars.next(); + } + '"' => in_str = false, + _ => {} + } + continue; + } + match c { + // Single-line raw string `r"..."` / `r#"..."#`: skip its content. + 'r' if matches!(chars.peek(), Some('"') | Some('#')) => { + let rest: String = chars.clone().collect(); + let hashes = rest.chars().take_while(|&h| h == '#').count(); + if rest[hashes..].starts_with('"') { + let close = format!("\"{}", "#".repeat(hashes)); + if let Some(end) = rest[hashes + 1..].find(&close) { + // Consume through the closing delimiter. + for _ in 0..(hashes + 1 + end + close.len()) { + chars.next(); + } + } else { + break; // unterminated on this line: ignore the rest + } + } + } + '"' => in_str = true, + '/' if chars.peek() == Some(&'*') => { + chars.next(); + *in_block = true; + } + '/' if chars.peek() == Some(&'/') => break, + '\'' => { + // Char literal ('{', '\'') vs lifetime ('a): only skip literals. + let rest: String = chars.clone().take(3).collect(); + if rest.starts_with('\\') { + chars.next(); + chars.next(); + if chars.peek() == Some(&'\'') { + chars.next(); + } + } else if rest.chars().nth(1) == Some('\'') { + chars.next(); + chars.next(); + } + } + '{' => delta += 1, + '}' => delta -= 1, + _ => {} + } + } + delta +} + +/// Does this (trimmed) line start a Rust ITEM or macro item (`fn`, `impl`, +/// `pub struct`, `use`, `#[attr]`, `thread_local! {`, ...)? Used by the +/// indentation safety net: content of a multi-line string at column 0 (a TOML +/// or JSON fixture) is not an item and must not abandon a test region, but +/// code that follows the real end of the region is always an item. +fn looks_like_item_start(trimmed: &str) -> bool { + use std::sync::LazyLock; + static ITEM_RE: LazyLock = LazyLock::new(|| { + regex::Regex::new( + r#"^(?:#\[|#!\[|(?:pub(?:\([^)]*\))?\s+)?(?:(?:async|unsafe|const|default|extern(?:\s+"[^"]*")?)\s+)*(?:fn|struct|enum|union|trait|impl|mod|use|type|static|const|extern)\b|macro_rules!\s|[A-Za-z_][\w:]*!\s*[(\[{])"#, + ) + .expect("item regex must compile") + }); + ITEM_RE.is_match(trimmed) +} + +/// Is this attribute line one that marks the next item as test code? +fn is_test_attribute(trimmed: &str) -> bool { + trimmed.starts_with("#[cfg(test)]") + || trimmed.starts_with("#[cfg(all(test") + || trimmed == "#[test]" + || (trimmed.starts_with("#[") && trimmed.trim_end().ends_with("::test]")) + || trimmed.starts_with("#[tokio::test") + || trimmed.starts_with("#[rstest") +} + +/// Line numbers inside Rust test code: from a test attribute through the end of +/// the item it decorates (`#[cfg(test)] mod tests { ... }`, `#[test] fn ...`). +/// +/// `lines` are `(line number, text)` in file order. A gap in the numbering (a +/// diff skipping unchanged code) abandons the region being tracked, so a +/// partial view never extends a region it cannot see the end of. +/// +/// `partial_view` is true for a diff (the view may end inside a region, whose +/// lines seen so far are still test code). For a WHOLE file it is false: a +/// region that never closes means the brace matching lost track (a raw string, +/// a macro, or a hostile `{`), and exempting everything to the end of the file +/// would hide real code, so the unfinished region is dropped instead. +pub fn rust_test_lines(lines: &[(u32, &str)], partial_view: bool) -> HashSet { + let mut out = HashSet::new(); + // (lines of the region so far, brace depth, seen the opening brace, + // indentation of the decorated item) + let mut region: Option<(Vec, i32, bool, usize)> = None; + let mut in_block = false; + let mut pending: Vec = Vec::new(); + let mut prev: Option = None; + + for &(n, text) in lines { + if prev.is_some_and(|p| n != p + 1) { + // Discontinuous: drop what we cannot finish (the lines already + // collected stay marked: they were inside the region). + if let Some((seen, _, _, _)) = region.take() { + out.extend(seen); + } + pending.clear(); + in_block = false; + } + prev = Some(n); + let trimmed = text.trim(); + + // One comment/brace pass per line, so block-comment state is tracked + // everywhere (not only inside a region) and cannot go stale. + let started_in_block = in_block; + let delta = brace_delta(text, &mut in_block); + let indent_of_line = text.len() - text.trim_start().len(); + + if let Some((seen, depth, opened, indent)) = region.as_mut() { + // Safety net against a parser differential: inside the region, + // an ITEM may not start at or left of the decorated item's + // indentation, and the closing `}` must balance the count. If one + // does, our brace matching disagrees with the compiler (a block + // comment, a multi-line string, a hostile `{`), so vouch for + // nothing. Non-item text at column 0 (a TOML fixture inside a raw + // string) is fine. + let closes = !started_in_block && trimmed.starts_with('}'); + if !started_in_block + && indent_of_line <= *indent + && !closes + && looks_like_item_start(trimmed) + { + region = None; + continue; + } + seen.push(n); + *depth += delta; + if text.contains('{') && !started_in_block { + *opened = true; + } + if closes && indent_of_line <= *indent && *depth > 0 { + region = None; // the item's own `}` must balance the count + continue; + } + let done = (*opened && *depth <= 0) || (!*opened && trimmed.ends_with(';')); + if done { + let (seen, _, _, _) = region.take().expect("region present"); + out.extend(seen); + } + continue; + } + + // Outside a region, a line that begins inside a block comment is + // comment text: never an attribute, never the decorated item. + if started_in_block { + continue; + } + + let is_attr_line = is_test_attribute(trimmed); + if is_attr_line || (!pending.is_empty() && trimmed.starts_with("#[")) { + // `#[cfg(test)] fn helper() {}` carries its item on the same line: + // the attribute decorates THAT item, not the next line. + if attribute_rest(trimmed).is_none_or(is_line_comment_or_empty) { + pending.push(n); + continue; + } + } else if !pending.is_empty() { + // Blank lines and doc comments between the test attribute and the + // item keep the region pending. + if trimmed.is_empty() || trimmed.starts_with("///") { + pending.push(n); + continue; + } + } else { + continue; + } + + // The decorated item starts on this line. + let mut seen = std::mem::take(&mut pending); + seen.push(n); + let opened = text.contains('{'); + if (opened && delta <= 0) || (!opened && trimmed.ends_with(';')) { + out.extend(seen); + } else { + region = Some((seen, delta, opened, indent_of_line)); + } + } + if partial_view { + if let Some((seen, _, _, _)) = region { + out.extend(seen); + } + } + out +} + +/// The text after the first complete `#[...]` attribute on `trimmed`, or +/// `None` when the attribute does not close on this line. +fn attribute_rest(trimmed: &str) -> Option<&str> { + let bytes = trimmed.as_bytes(); + let mut depth = 0i32; + for (i, &b) in bytes.iter().enumerate() { + match b { + b'[' => depth += 1, + b']' => { + depth -= 1; + if depth == 0 { + return Some(trimmed[i + 1..].trim()); + } + } + _ => {} + } + } + None +} + +fn is_line_comment_or_empty(rest: &str) -> bool { + rest.is_empty() || rest.starts_with("//") +} + +#[cfg(test)] +mod tests { + use super::*; + + fn numbered(src: &str) -> Vec<(u32, &str)> { + src.lines().zip(1u32..).map(|(t, n)| (n, t)).collect() + } + + #[test] + fn path_segments_decide_test_files() { + for p in [ + "tests/auth_test.py", + "src/app.test.ts", + "src/__tests__/setup.rs", + "spec/models/user_spec.rb", + "fixtures/a.json", + ] { + assert!(is_test_path(p), "{p}"); + } + for p in [ + "src/latest_config.rs", + "src/models/attestation.rs", + "src/protest.rs", + ] { + assert!(!is_test_path(p), "{p}"); + } + } + + #[test] + fn cfg_test_module_is_marked_to_its_closing_brace() { + let src = "\ +fn real() { x.unwrap(); } +#[cfg(test)] +mod tests { + use super::*; + #[test] + fn t() { + let s = \"}{\"; + y.unwrap(); + } +} +fn after() { z.unwrap(); }"; + let set = rust_test_lines(&numbered(src), false); + assert!(!set.contains(&1), "code before is not test code"); + for n in 2..=10 { + assert!(set.contains(&n), "line {n} is inside the test module"); + } + assert!(!set.contains(&11), "code after the module is not test code"); + } + + #[test] + fn braces_in_strings_chars_and_comments_do_not_end_the_region() { + let src = "\ +#[cfg(test)] +mod tests { + fn a() { let c = '}'; let s = \"}}}\"; } // }}} + fn b() {} +} +fn after() {}"; + let set = rust_test_lines(&numbered(src), false); + assert!(set.contains(&4) && set.contains(&5)); + assert!(!set.contains(&6)); + } + + #[test] + fn test_attribute_on_a_function_marks_only_that_function() { + let src = "\ +fn keep() {} +#[test] +fn only_this() { + a.unwrap(); +} +fn keep_too() { b.unwrap(); }"; + let set = rust_test_lines(&numbered(src), false); + assert!((2..=5).all(|n| set.contains(&n))); + assert!(!set.contains(&1) && !set.contains(&6)); + } + + #[test] + fn test_attribute_on_a_statement_item_marks_one_line() { + let src = "#[cfg(test)]\nuse helper::x;\nfn real() {}"; + let set = rust_test_lines(&numbered(src), false); + assert!(set.contains(&1) && set.contains(&2) && !set.contains(&3)); + } + + #[test] + fn stacked_attributes_and_doc_comments_stay_in_the_region() { + let src = "#[cfg(test)]\n#[allow(dead_code)]\n/// helper\nmod tests {\n}\nfn real() {}"; + let set = rust_test_lines(&numbered(src), false); + assert!((1..=5).all(|n| set.contains(&n)) && !set.contains(&6)); + } + + #[test] + fn a_gap_in_numbering_abandons_an_unfinished_region_but_keeps_what_was_seen() { + // Diff view: the module is open at line 3 and the next shown line is 40. + let lines = vec![ + (1, "#[cfg(test)]"), + (2, "mod tests {"), + (3, " fn a() {"), + (40, " x.unwrap();"), + ]; + let set = rust_test_lines(&lines, true); + assert!(set.contains(&1) && set.contains(&2) && set.contains(&3)); + assert!(!set.contains(&40), "cannot vouch for lines after a gap"); + } + + #[test] + fn plain_code_has_no_test_lines() { + let set = rust_test_lines(&numbered("fn main() {\n println!(\"hi\");\n}"), false); + assert!(set.is_empty()); + } + + #[test] + fn test_module_hunk_headers() { + assert!(hunk_header_is_test_module("mod tests {")); + assert!(hunk_header_is_test_module("pub mod test {")); + assert!(!hunk_header_is_test_module("mod testing_utils {")); + assert!(!hunk_header_is_test_module("fn tests() {")); + assert!(!hunk_header_is_test_module("")); + } + + #[test] + fn an_unclosed_region_in_a_whole_file_is_dropped_not_extended_to_eof() { + // A hostile or unparseable `{` must not exempt the rest of the file. + let src = "\ +#[cfg(test)] +fn helper() { + let weird = '\\u{7b}'; let open = r#\"{{{\"#; let more = \"{\"; // tricks + { +} +fn production() { danger.unwrap(); } +fn more_production() { x.unwrap(); }"; + let lines = numbered(src); + // Whole-file view: no region is vouched for at all. + let whole = rust_test_lines(&lines, false); + assert!(!whole.contains(&6) && !whole.contains(&7), "{whole:?}"); + // Partial view (a diff): the column-0 `fn production` inside the + // still-open region is a disagreement, so nothing is vouched for. + let partial = rust_test_lines(&lines, true); + assert!( + !partial.contains(&6) && !partial.contains(&7), + "{partial:?}" + ); + } + + #[test] + fn single_line_raw_strings_do_not_confuse_brace_matching() { + let src = "#[cfg(test)]\nmod tests {\n fn a() { let s = r#\"}}}\"#; }\n}\nfn real() {}"; + let set = rust_test_lines(&numbered(src), false); + assert!( + (1..=4).all(|n| set.contains(&n)) && !set.contains(&5), + "{set:?}" + ); + } + + #[test] + fn a_block_comment_brace_cannot_extend_the_test_region() { + // `/* { */` is a comment: the test fn ends on line 2, `production` + // is NOT test code. (Counting the comment brace used to extend the + // region over it.) + let src = "\ +#[cfg(test)] +fn t() { /* { */ } +fn production() { danger.unwrap(); } +/* } */"; + let set = rust_test_lines(&numbered(src), false); + assert!(set.contains(&2)); + assert!(!set.contains(&3), "{set:?}"); + } + + #[test] + fn a_new_top_level_item_inside_an_open_region_abandons_it() { + // Multi-line raw string the matcher cannot see: the open brace inside + // makes the region look unfinished, and a column-0 item follows. Vouch + // for nothing rather than exempt `production`. + let src = "\ +#[cfg(test)] +fn t() { + let s = r#\" +{ +\"#; +} +fn production() { danger.unwrap(); } +/* } */"; + let set = rust_test_lines(&numbered(src), false); + assert!(!set.contains(&7), "{set:?}"); + let partial = rust_test_lines(&numbered(src), true); + assert!(!partial.contains(&7), "{partial:?}"); + } + + #[test] + fn multi_line_block_comments_hide_their_braces() { + let src = + "#[cfg(test)]\nmod tests {\n /* open {\n { */\n fn a() {}\n}\nfn real() {}"; + let set = rust_test_lines(&numbered(src), false); + assert!( + (1..=6).all(|n| set.contains(&n)) && !set.contains(&7), + "{set:?}" + ); + } + + #[test] + fn indented_modules_close_at_their_own_indentation() { + let src = "mod outer {\n #[cfg(test)]\n mod tests {\n fn a() {}\n }\n fn prod() {}\n}"; + let set = rust_test_lines(&numbered(src), false); + assert!( + (2..=5).all(|n| set.contains(&n)) && !set.contains(&6), + "{set:?}" + ); + } + + #[test] + fn column_zero_fixture_text_inside_a_test_module_does_not_abandon_it() { + // A TOML fixture in a raw string, flush left: not Rust items. + let src = "\ +#[cfg(test)] +mod tests { + fn t() { + let toml = r#\" +[embed] +api_key = \"sk-test\" +\"#; + } +} +fn production() {}"; + let set = rust_test_lines(&numbered(src), false); + assert!((1..=9).all(|n| set.contains(&n)), "{set:?}"); + assert!(!set.contains(&10)); + } + + #[test] + fn item_start_detection() { + for yes in [ + "fn a() {", + "pub fn a() {", + "pub(crate) async fn a() {", + "unsafe impl X for Y {", + "impl Z for T {", + "struct S;", + "use std::fs;", + "#[derive(Debug)]", + "thread_local! {", + "macro_rules! m {", + "extern \"C\" fn f() {", + ] { + assert!(looks_like_item_start(yes), "{yes}"); + } + for no in [ + "api_key = \"x\"", + "[embed]", + "{", + "\"text\"", + "foo.bar();", + "}", + ] { + assert!(!looks_like_item_start(no), "{no}"); + } + } + + #[test] + fn an_attribute_with_its_item_on_the_same_line_does_not_exempt_the_next_line() { + let src = "\ +#[cfg(test)] fn helper() {} +fn production() { danger.unwrap(); } +#[cfg(test)] +#[allow(dead_code)] fn other() {} +fn production2() { danger.unwrap(); }"; + let set = rust_test_lines(&numbered(src), false); + assert!( + set.contains(&1) && set.contains(&3) && set.contains(&4), + "{set:?}" + ); + assert!(!set.contains(&2) && !set.contains(&5), "{set:?}"); + } + + #[test] + fn a_same_line_item_with_a_body_that_spans_lines_is_one_region() { + let src = "#[cfg(test)] mod tests {\n fn a() {}\n}\nfn production() {}"; + let set = rust_test_lines(&numbered(src), false); + assert!( + (1..=3).all(|n| set.contains(&n)) && !set.contains(&4), + "{set:?}" + ); + } + + #[test] + fn an_attribute_inside_a_block_comment_is_not_an_attribute() { + let src = "/*\n#[cfg(test)]\n*/\nfn production() { danger.unwrap(); }\n/* #[test] */\nfn also_production() {}"; + let set = rust_test_lines(&numbered(src), false); + assert!(set.is_empty(), "{set:?}"); + } + + #[test] + fn block_comment_state_does_not_go_stale_between_regions() { + // An earlier multi-line comment must not leak into brace counting of + // the next test module, and a closed region must not leave state behind. + let src = "\ +/* a comment + { with a brace +*/ +#[cfg(test)] +mod tests { + fn a() {} +} +fn production() { danger.unwrap(); } +#[cfg(test)] +mod more { +} +"; + let set = rust_test_lines(&numbered(src), false); + assert!((4..=7).all(|n| set.contains(&n)), "{set:?}"); + assert!(!set.contains(&8), "{set:?}"); + assert!((9..=11).all(|n| set.contains(&n)), "{set:?}"); + } +}