From 65206aecbed4daadf260d8c1d712adefba1924fd Mon Sep 17 00:00:00 2001 From: ajianaz Date: Thu, 15 Oct 2026 16:04:09 +0700 Subject: [PATCH 1/4] feat(rules): test-code awareness for the built-in rules (#663) Whole test files and Rust #[cfg(test)] modules / #[test] functions (also a diff hunk inside mod tests) are exempt from the noisy rules; secret scanners keep only their high-confidence patterns there. bug-println is skipped in program entry points. rules_engine.include_tests turns it off. Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: ajianaz --- CHANGELOG.md | 2 + docs/configuration.md | 11 ++ src/config/schema.rs | 6 + src/engine/deterministic.rs | 24 ++- src/engine/mod.rs | 1 + src/engine/rules/builtin.rs | 12 ++ src/engine/rules/mod.rs | 134 +++++++++++++++ src/engine/rules/types.rs | 8 + src/engine/scan_input.rs | 136 ++++++++++++--- src/engine/secrets_scanner.rs | 4 +- src/engine/security_scanner.rs | 108 ++++++++---- src/engine/test_context.rs | 303 +++++++++++++++++++++++++++++++++ 12 files changed, 687 insertions(+), 62 deletions(-) create mode 100644 src/engine/test_context.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index f72ad71..f487415 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. `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..8f1df12 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 everything else. +- Set `rules_engine.include_tests: true` to scan test code like any other code. Detection is conservative: when it cannot tell (for example a diff that does not show the `mod tests` line), the line stays scanned. + #### 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..3cdc774 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)); + 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) + } 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,6 +202,17 @@ 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; + } + continue; + } + for (rule_id, name, regex, severity) in COMPILED_PATTERNS.iter() { if regex.is_match(content) { // Apply post-match filter to suppress known false positives @@ -257,55 +269,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 +1374,42 @@ 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" + ); + } } diff --git a/src/engine/test_context.rs b/src/engine/test_context.rs new file mode 100644 index 0000000..622d597 --- /dev/null +++ b/src/engine/test_context.rs @@ -0,0 +1,303 @@ +//! 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) -> i32 { + let mut delta = 0; + let mut chars = line.chars().peekable(); + let mut in_str = false; + while let Some(c) = chars.next() { + if in_str { + match c { + '\\' => { + chars.next(); + } + '"' => in_str = false, + _ => {} + } + continue; + } + match c { + '"' => in_str = 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 +} + +/// 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. +pub fn rust_test_lines(lines: &[(u32, &str)]) -> HashSet { + let mut out = HashSet::new(); + // (lines of the region so far, brace depth, seen the opening brace) + let mut region: Option<(Vec, i32, bool)> = None; + 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(); + } + prev = Some(n); + let trimmed = text.trim(); + + if let Some((seen, depth, opened)) = region.as_mut() { + seen.push(n); + *depth += brace_delta(text); + if text.contains('{') { + *opened = true; + } + let done = (*opened && *depth <= 0) || (!*opened && trimmed.ends_with(';')); + if done { + let (seen, _, _) = region.take().expect("region present"); + out.extend(seen); + } + continue; + } + + if is_test_attribute(trimmed) { + pending.push(n); + continue; + } + if !pending.is_empty() { + // Other attributes and blank lines between the test attribute and + // the item keep the region pending. + if trimmed.starts_with("#[") || trimmed.is_empty() || trimmed.starts_with("///") { + pending.push(n); + continue; + } + // The decorated item starts here. + let mut seen = std::mem::take(&mut pending); + seen.push(n); + let depth = brace_delta(text); + let opened = text.contains('{'); + if (opened && depth <= 0) || (!opened && trimmed.ends_with(';')) { + out.extend(seen); + } else { + region = Some((seen, depth, opened)); + } + } + } + if let Some((seen, _, _)) = region { + out.extend(seen); + } + out +} + +#[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)); + 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)); + 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)); + 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)); + 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)); + 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); + 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}")); + 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("")); + } +} From ebf941289e9d945c86321ca89b4aa0c63dd9c607 Mon Sep 17 00:00:00 2001 From: ajianaz Date: Thu, 15 Oct 2026 16:13:01 +0700 Subject: [PATCH 2/4] fix(security): test awareness cannot hide code (unclosed regions, injection in test blocks) An unclosed #[cfg(test)] region in a whole file is dropped instead of exempting the rest of the file, single-line raw strings no longer confuse brace matching, and inline test blocks keep every security pattern except the generic hardcoded-secret one. Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: ajianaz --- CHANGELOG.md | 2 +- docs/configuration.md | 4 +- src/engine/scan_input.rs | 4 +- src/engine/security_scanner.rs | 40 +++++++++++++++++-- src/engine/test_context.rs | 73 +++++++++++++++++++++++++++++----- 5 files changed, 104 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f487415..8b6ec76 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,7 +33,7 @@ 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. `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 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 whose braces never close is dropped instead of exempting the rest of the file). `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. diff --git a/docs/configuration.md b/docs/configuration.md index 8f1df12..e58d01b 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -472,8 +472,8 @@ Tests legitimately `unwrap()`, `println!`, clone, use `http://` fixtures and fak - **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 everything else. -- Set `rules_engine.include_tests: true` to scan test code like any other code. Detection is conservative: when it cannot tell (for example a diff that does not show the `mod tests` line), the line stays scanned. +- **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 whose braces never close in a whole file is ignored rather than extended to the end of the file, and a diff that does not show the `mod tests` line is scanned normally. #### What `max_findings` caps diff --git a/src/engine/scan_input.rs b/src/engine/scan_input.rs index 3cdc774..f9e2ac7 100644 --- a/src/engine/scan_input.rs +++ b/src/engine/scan_input.rs @@ -69,7 +69,7 @@ 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(); - set.extend(test_context::rust_test_lines(&post)); + 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)); @@ -141,7 +141,7 @@ impl<'a> ScanFile<'a> { .map(|(t, n)| (n, t)) .collect(); let test_lines = if language == "rs" { - test_context::rust_test_lines(&lines) + test_context::rust_test_lines(&lines, false) } else { HashSet::new() }; diff --git a/src/engine/security_scanner.rs b/src/engine/security_scanner.rs index a45cba6..334fca5 100644 --- a/src/engine/security_scanner.rs +++ b/src/engine/security_scanner.rs @@ -202,18 +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; } - continue; } 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) { @@ -1412,4 +1418,30 @@ mod tests { "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 index 622d597..18c6e2f 100644 --- a/src/engine/test_context.rs +++ b/src/engine/test_context.rs @@ -78,6 +78,22 @@ fn brace_delta(line: &str) -> i32 { 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(&'/') => break, '\'' => { @@ -118,7 +134,13 @@ fn is_test_attribute(trimmed: &str) -> bool { /// `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. -pub fn rust_test_lines(lines: &[(u32, &str)]) -> HashSet { +/// +/// `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) let mut region: Option<(Vec, i32, bool)> = None; @@ -174,8 +196,10 @@ pub fn rust_test_lines(lines: &[(u32, &str)]) -> HashSet { } } } - if let Some((seen, _, _)) = region { - out.extend(seen); + if partial_view { + if let Some((seen, _, _)) = region { + out.extend(seen); + } } out } @@ -222,7 +246,7 @@ mod tests { } } fn after() { z.unwrap(); }"; - let set = rust_test_lines(&numbered(src)); + 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"); @@ -239,7 +263,7 @@ mod tests { fn b() {} } fn after() {}"; - let set = rust_test_lines(&numbered(src)); + let set = rust_test_lines(&numbered(src), false); assert!(set.contains(&4) && set.contains(&5)); assert!(!set.contains(&6)); } @@ -253,7 +277,7 @@ fn only_this() { a.unwrap(); } fn keep_too() { b.unwrap(); }"; - let set = rust_test_lines(&numbered(src)); + let set = rust_test_lines(&numbered(src), false); assert!((2..=5).all(|n| set.contains(&n))); assert!(!set.contains(&1) && !set.contains(&6)); } @@ -261,14 +285,14 @@ fn keep_too() { b.unwrap(); }"; #[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)); + 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)); + let set = rust_test_lines(&numbered(src), false); assert!((1..=5).all(|n| set.contains(&n)) && !set.contains(&6)); } @@ -281,14 +305,14 @@ fn keep_too() { b.unwrap(); }"; (3, " fn a() {"), (40, " x.unwrap();"), ]; - let set = rust_test_lines(&lines); + 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}")); + let set = rust_test_lines(&numbered("fn main() {\n println!(\"hi\");\n}"), false); assert!(set.is_empty()); } @@ -300,4 +324,33 @@ fn keep_too() { b.unwrap(); }"; 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): what was seen inside the region stays marked. + assert!(rust_test_lines(&lines, true).contains(&2)); + } + + #[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:?}" + ); + } } From 45dd8b5de02046e0f5fd39a3bcc445207d327b98 Mon Sep 17 00:00:00 2001 From: ajianaz Date: Thu, 15 Oct 2026 16:24:51 +0700 Subject: [PATCH 3/4] fix(security): test-region detection cannot be steered by comments or strings Block comments are skipped when counting braces, and a test region is dropped when an item starts at or left of its own indentation or its closing brace does not balance the count, so a parser differential cannot exempt production code. Column-0 text that is not a Rust item (TOML fixtures in raw strings) does not abandon a region. Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: ajianaz --- CHANGELOG.md | 2 +- docs/configuration.md | 2 +- src/engine/test_context.rs | 182 ++++++++++++++++++++++++++++++++++--- 3 files changed, 172 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b6ec76..6620aa0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,7 +33,7 @@ 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 whose braces never close is dropped instead of exempting the rest of the file). `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 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). `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. diff --git a/docs/configuration.md b/docs/configuration.md index e58d01b..6f306fd 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -473,7 +473,7 @@ Tests legitimately `unwrap()`, `println!`, clone, use `http://` fixtures and fak - **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 whose braces never close in a whole file is ignored rather than extended to the end of the file, and a diff that does not show the `mod tests` line is scanned normally. +- 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 diff --git a/src/engine/test_context.rs b/src/engine/test_context.rs index 18c6e2f..6527947 100644 --- a/src/engine/test_context.rs +++ b/src/engine/test_context.rs @@ -62,11 +62,19 @@ pub fn hunk_header_is_test_module(header: &str) -> bool { /// 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) -> i32 { +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 { '\\' => { @@ -95,6 +103,10 @@ fn brace_delta(line: &str) -> i32 { } } '"' => 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. @@ -118,6 +130,22 @@ fn brace_delta(line: &str) -> i32 { 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)]") @@ -142,8 +170,10 @@ fn is_test_attribute(trimmed: &str) -> bool { /// 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) - let mut region: Option<(Vec, i32, bool)> = None; + // (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; @@ -151,23 +181,41 @@ pub fn rust_test_lines(lines: &[(u32, &str)], partial_view: bool) -> HashSet 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"); + let (seen, _, _, _) = region.take().expect("region present"); out.extend(seen); } continue; @@ -187,17 +235,18 @@ pub fn rust_test_lines(lines: &[(u32, &str)], partial_view: bool) -> HashSet 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}"); + } + } } From fa43ea8892b678c5848aca1c88039b9b5980a7b1 Mon Sep 17 00:00:00 2001 From: ajianaz Date: Thu, 15 Oct 2026 16:34:16 +0700 Subject: [PATCH 4/4] fix(security): same-line attribute items and block-comment attributes cannot exempt code #[cfg(test)] fn helper() {} decorated the next line instead of its own item, and an attribute inside a block comment counted as one. Comment state is now tracked on every line so it cannot go stale. Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: ajianaz --- CHANGELOG.md | 2 +- src/engine/test_context.rs | 144 +++++++++++++++++++++++++++++++------ 2 files changed, 123 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6620aa0..359580b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,7 +33,7 @@ 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). `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 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. diff --git a/src/engine/test_context.rs b/src/engine/test_context.rs index 6527947..696d69c 100644 --- a/src/engine/test_context.rs +++ b/src/engine/test_context.rs @@ -190,6 +190,12 @@ pub fn rust_test_lines(lines: &[(u32, &str)], partial_view: bool) -> HashSet HashSet 0 { + if closes && indent_of_line <= *indent && *depth > 0 { region = None; // the item's own `}` must balance the count continue; } @@ -221,28 +230,39 @@ pub fn rust_test_lines(lines: &[(u32, &str)], partial_view: bool) -> HashSet HashSet 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::*; @@ -511,4 +555,60 @@ fn production() {}"; 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:?}"); + } }