diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c04ab6..48f02f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **`crypto/hardcoded-secret` / `sec-hardcoded-secret` no longer flag `name: value` entries whose value is not a literal (#661).** In source files, `webhook_secret: env::var("X")`, `refresh_token: prev?.refresh_token`, `token: Option<&'static str>`, `"password": ACCOUNT_PW` and `"clientSecret": j["clientSecret"]` are references or types, and format placeholders (`api_key = "{}"`, `"%s"`) are not values. Quoted literals (`token: "ghp_..."`) and `=` assignments are judged as before. On two real codebases the Critical/Major findings dropped from 38 to 20 (all remaining ones are test fixtures, SQL and test URLs, tracked in #662/#663). + - **Agent-mode review no longer hides capped findings without saying so.** `cora.review_prepare` listed every deterministic finding but `cora.review_finalize` showed only `rules_engine.max_findings` (default 5) of them with no explanation; found by running a real agent on a real project. `review_prepare` now reports `deterministic_findings_total` and the cap, and `review_finalize` / `scan_finalize` / `cora.review_diff` report `not_shown` (with a note). - **The MCP `cora.review_diff` tool no longer crashes the server (#647).** Every call panicked with "Cannot start a runtime from within a runtime" because the handler blocked on a second runtime from a tokio runtime thread. Handlers now go through `block_in_place`. diff --git a/src/engine/rules/builtin.rs b/src/engine/rules/builtin.rs index 0208efa..8df6504 100644 --- a/src/engine/rules/builtin.rs +++ b/src/engine/rules/builtin.rs @@ -284,6 +284,7 @@ fn quoted_placeholder_value(line: &str) -> bool { let v = &cap[1]; let inner = &v[1..v.len() - 1]; let placeholder = inner.is_empty() + || is_format_placeholder(inner) || (inner.starts_with("${") && inner.ends_with('}') && !shell_secret_default(inner)) || (inner.starts_with("{{") && inner.ends_with("}}")) || (inner.starts_with('$') @@ -361,12 +362,64 @@ fn secret_assign_rhs(line: &str) -> Option<&str> { Some(rest.trim()) } -/// A quoted value that is nothing but a reference: `${X}`, `$X`, `$(cmd)`. +/// A format-string placeholder standing in for a value: `{}`, `{name}`, +/// `{0}`, `{:?}`, `%s`, `%v`. Not a literal secret. +fn is_format_placeholder(inner: &str) -> bool { + static RE: LazyLock = LazyLock::new(|| { + // Digits only as a positional index (`{0}`): a name with digits inside + // braces (`{hunter2hunter2}`) is more likely a value than a template. + Regex::new(r"^(?:\{(?:\d+|[A-Za-z_][A-Za-z_]{0,31})?(?::[^{}]*)?\}|%[sdvq])$") + .expect("placeholder regex must compile") + }); + RE.is_match(inner) +} + +/// `name: value` entries (struct-literal fields, object/dict entries, type +/// annotations) in SOURCE CODE whose value is not a quoted literal: +/// `webhook_secret: env::var("X")`, `refresh_token: prev?.refresh_token`, +/// `token: Option<&'static str>`, `"password": ACCOUNT_PW`. In code a bare +/// expression after the colon is a reference or a type, never a secret value; a +/// quoted literal (`token: "abc12345"`) is not covered here and stays flagged. +/// Config-style files (YAML, TOML, ...) keep their own judgement. +fn colon_value_is_not_a_literal(line: &str, path: &str) -> bool { + if rhs_style(path) != RhsStyle::Code { + return false; + } + static COLON_RE: LazyLock = LazyLock::new(|| { + Regex::new(r#"(?i)(?:password|passwd|pwd|secret|api_?key|token)["']?\s*:"#) + .expect("colon regex must compile") + }); + // EVERY entry on the line must be a non-literal: `{ token: id, password: + // "hunter2hunter2" }` has a real secret after a harmless first entry. + let mut entries = 0; + for m in COLON_RE.find_iter(line) { + let rest = &line[m.end()..]; + // `a::token`, `x := ...` and `token:=` are not `name: value` entries. + if rest.starts_with([':', '=']) { + continue; + } + let rhs = rest.trim().trim_end_matches([';', ',']).trim_end(); + if rhs.is_empty() || starts_with_literal(rhs) { + return false; + } + // An env read / wrapper with a literal fallback is judged on the + // literal: `secret: env::var("X").unwrap_or("hunter2hunter2".into())`. + if embedded_literal(rhs) == Some(true) { + return false; + } + entries += 1; + } + entries > 0 +} + +/// A quoted value that is nothing but a reference: `${X}`, `$X`, `$(cmd)`, +/// or a format placeholder. fn is_pure_reference(inner: &str) -> bool { - (inner.starts_with("${") - && inner.ends_with('}') - && inner.matches("${").count() == 1 - && !shell_secret_default(inner)) + is_format_placeholder(inner) + || (inner.starts_with("${") + && inner.ends_with('}') + && inner.matches("${").count() == 1 + && !shell_secret_default(inner)) || (inner.starts_with("$(") && inner.ends_with(')')) || (inner.starts_with('$') && inner.len() > 1 @@ -474,7 +527,7 @@ fn embedded_literal(rhs: &str) -> Option { /// files nothing changes. fn non_literal_rhs(line: &str, path: &str) -> bool { let Some(rhs) = secret_assign_rhs(line) else { - return false; + return colon_value_is_not_a_literal(line, path); }; let rhs = rhs.trim_end_matches([';', ',']).trim_end(); if rhs.is_empty() { @@ -1387,4 +1440,148 @@ mod tests { assert!(!has_secret_literal(line, path), "{line}"); } } + + // ─── #661: `name: value` entries in code that are not secret literals ─── + + fn secret_fp(line: &str, path: &str) -> bool { + post_match_filter_for_path("crypto/hardcoded-secret", line, path) + && post_match_filter_for_path("sec-hardcoded-secret", line, path) + } + + #[test] + fn colon_entries_with_non_literal_values_are_not_secrets() { + for (line, path) in [ + ( + r#"webhook_secret: env::var("POLAR_WEBHOOK_SECRET")"#, + "config.rs", + ), + ( + "access_token: env::var(\"POLAR_ACCESS_TOKEN\")", + "config.rs", + ), + ( + "refresh_token: typeof b.refresh_token === 'string' ? b.refresh_token : prev?.refresh_token,", + "oidc.ts", + ), + ( + "fn with_auth(token: Option<&'static str>) -> Self {", + "mod.rs", + ), + ("admin_token: Option<&'static str>,", "mod.rs"), + ( + "api_key: pick(\"UTEKE_EMBED_FALLBACK_API_KEY\", self.api_key),", + "lib.rs", + ), + ("\"RS_CLIENT_SECRET\": rs[\"clientSecret\"],", "gh_setup.py"), + ( + "json_body={\"email\": ADMIN, \"password\": ACCOUNT_PW})", + "lib.py", + ), + ( + "let ent = {\"clientSecret\": j[\"clientSecret\"]}", + "gh_setup.py", + ), + ] { + assert!(secret_fp(line, path), "should be suppressed: {line}"); + } + } + + #[test] + fn format_placeholders_are_not_secret_values() { + for line in [ + "api_key = \"{}\"", + "api_key = \"{key}\"", + "token = \"%s\"", + "password = \"{0}\"", + "api_key: \"{}\",", + ] { + assert!( + secret_fp(line, "onboard.rs"), + "should be suppressed: {line}" + ); + } + } + + #[test] + fn real_secret_literals_in_colon_entries_are_still_flagged() { + for (line, path) in [ + ("password: \"hunter2hunter2\",", "config.ts"), + ("api_key: 'sk_live_abcdefghijkl',", "client.js"), + ("const cfg = { token: \"ghp_abcdefghijklmnop\" };", "a.ts"), + ("password = \"hunter2hunter2\"", "app.py"), + ("let token: &str = \"abcdefgh12345678\";", "main.rs"), + ("api_key: \"k\".into(),", "lib.rs"), // short literal: left to test-code awareness (#663) + ] { + assert!( + !post_match_filter_for_path("crypto/hardcoded-secret", line, path), + "must stay flagged: {line}" + ); + } + } + + #[test] + fn rust_path_segments_named_like_secrets_are_not_entries() { + // `Token::secret: ...` style paths and `:=` are not `name: value` entries. + assert!(!colon_value_is_not_a_literal( + "let s = a::token::new();", + "x.rs" + )); + assert!(!colon_value_is_not_a_literal( + "token := \"abcdefgh12345678\"", + "main.go" + )); + } + + #[test] + fn config_style_files_keep_their_own_judgement() { + // YAML: an unquoted value after the colon is still judged by the + // plain-scalar rule, not waved through by the code-only rule. + assert!(!colon_value_is_not_a_literal( + "password: hunter2hunter2", + "app.yaml" + )); + } + + #[test] + fn a_harmless_first_entry_does_not_hide_a_later_real_secret() { + for (line, path) in [ + ( + "const c = { token: ident, password: \"hunter2hunter2\" };", + "a.ts", + ), + ( + "Cfg { user: name, api_key: \"sk_live_abcdefghijkl\" }", + "a.rs", + ), + ] { + assert!( + !post_match_filter_for_path("crypto/hardcoded-secret", line, path), + "must stay flagged: {line}" + ); + } + } + + #[test] + fn env_reads_with_a_literal_fallback_stay_flagged_in_colon_entries() { + let line = "webhook_secret: env::var(\"X\").unwrap_or(\"hunter2hunter2\".into()),"; + assert!(!post_match_filter_for_path( + "crypto/hardcoded-secret", + line, + "config.rs" + )); + } + + #[test] + fn braced_values_with_digits_are_not_placeholders() { + assert!(!is_format_placeholder("{hunter2hunter2}")); + assert!(!is_format_placeholder("{Passw0rd!}")); + for ok in ["{}", "{0}", "{name}", "{api_key}", "{:?}", "{0:>8}", "%s"] { + assert!(is_format_placeholder(ok), "{ok}"); + } + assert!(!post_match_filter_for_path( + "crypto/hardcoded-secret", + "password = \"{hunter2hunter2}\"", + "app.py" + )); + } }