diff --git a/CHANGELOG.md b/CHANGELOG.md index fba720c..44c4530 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **Hardcoded secrets in Go short declarations, YAML/JSON keys and SQL `PASSWORD` literals are now detected.** Go `password := "..."` (with or without a trailing comment) and `var apiKey string = "..."` (#618); unquoted YAML `password: hunter2hunter2` and JSON quoted keys `"password": "..."` / `"apiKey": "..."` (#619); SQL `CREATE USER app WITH PASSWORD ''` and `IDENTIFIED BY ''` (#620). References stay quiet: `${VAR}`/`$VAR`, `!tag`, `*alias`, `&anchor`, paths, `null`/`required`-style bare words, and SQL placeholders (`PASSWORD '%s'`). The LLM secret cross-check uses the same pattern, so LLM findings on these shapes are no longer dropped. + ### Changed - **One shared post-processing module for `cora review` and `cora scan`.** The finding filter pipeline (LLM secret false-positive cross-check, Markdown code blocks, `ignore.rules`, inline `cora-ignore:`, context-line filter, scanner merge for scans) now lives in `engine::postprocess`, so a fix lands in one place. Internal refactor with no CLI or output change (#610). diff --git a/src/engine/postprocess.rs b/src/engine/postprocess.rs index 74673ea..7489822 100644 --- a/src/engine/postprocess.rs +++ b/src/engine/postprocess.rs @@ -99,13 +99,6 @@ fn apply_llm_secret_fp_filter( ) -> Vec { use crate::engine::diff_parser::DiffLineType; - // Lazy-compiled regex matching the built-in sec-hardcoded-secret pattern. - // Only triggers for actual value assignments like `api_key = "sk-..."`. - static RE_SECRET_LITERAL: std::sync::LazyLock = std::sync::LazyLock::new(|| { - regex::Regex::new(r#"(?i)(?:password|api_?key|token|secret)(?:\s*:\s*[&\w<>\[\].?|]+)?\s*=\s*(?:"[^"]+"|'[^']+')"#) - .expect("hardcoded secret regex must compile") - }); - // Keywords that indicate an LLM finding is about hardcoded secrets. static SECRET_KEYWORDS: &[&str] = &[ "hardcoded password", @@ -156,7 +149,7 @@ fn apply_llm_secret_fp_filter( let line_num = issue.line.unwrap_or(0); let key = (issue.file.clone(), line_num); if let Some(actual_line) = added_lines.get(&key) { - if !RE_SECRET_LITERAL.is_match(actual_line) { + if !crate::engine::rules::builtin::has_secret_literal(actual_line) { debug!( file = %issue.file, line = line_num, @@ -446,6 +439,58 @@ mod tests { assert_eq!(result.len(), 1, "actual hardcoded secret should be kept"); } + /// LLM findings on the shapes added in #618-#620 are kept; SQL placeholders + /// and references are still dropped. + #[test] + fn secret_fp_filter_matches_go_yaml_json_sql_shapes() { + use crate::engine::diff_parser::*; + + let cases: &[(&str, bool)] = &[ + (r#"password := "hunter2hunter2""#, true), + (r#"var apiKey string = "abcd1234efgh5678""#, true), + ("password: hunter2hunter2", true), + (r#" "password": "hunter2hunter2","#, true), + ("CREATE USER app WITH PASSWORD 'hunter2hunter2';", true), + ("CREATE USER app WITH PASSWORD '%s';", false), + ("password: required", false), + (r#" "password": "${DB_PASSWORD}","#, false), + ]; + for (line, keep) in cases { + let chunks = vec![FileChunk { + old_path: None, + new_path: Some("src/x.txt".to_string()), + language: "txt".to_string(), + chunks: vec![DiffHunk { + old_start: 0, + old_count: 0, + new_start: 1, + new_count: 1, + header: "".to_string(), + lines: vec![DiffLine { + line_type: DiffLineType::Add, + content: line.to_string(), + old_line_no: None, + new_line_no: Some(1), + }], + }], + is_binary: false, + is_deleted: false, + is_new: false, + }]; + let issues = vec![ + ReviewIssue::new( + "src/x.txt", + Some(1), + Severity::Critical, + "Hardcoded password in config", + ) + .with_type("security"), + ]; + let kept = apply_llm_secret_fp_filter(issues, &chunks).len() == 1; + assert_eq!(kept, *keep, "{line}"); + } + } + #[test] fn secret_fp_filter_keeps_non_security_findings() { use crate::engine::diff_parser::*; diff --git a/src/engine/rules/builtin.rs b/src/engine/rules/builtin.rs index e584307..7de4952 100644 --- a/src/engine/rules/builtin.rs +++ b/src/engine/rules/builtin.rs @@ -11,8 +11,7 @@ pub fn builtin_rules() -> Vec { // --- Security --- CustomRule { id: "sec-hardcoded-secret".to_string(), - pattern: r#"(?i)(?:password|api_?key|token|secret)(?:\s*:\s*[&\w<>\[\].?|]+)?\s*=\s*(?:"[^"]+"|'[^']+')"# - .to_string(), + pattern: secret_literal_pattern(), severity: Severity::Critical, message: "Possible hardcoded secret/credential detected. Use environment variables or a secrets manager.".to_string(), languages: vec!["all".to_string()], @@ -126,6 +125,134 @@ pub fn builtin_rules() -> Vec { ] } +/// Regex source shared by the `sec-hardcoded-secret` rule and the LLM secret +/// cross-check in `postprocess`, so a shape detected here is never dropped +/// there as a false positive (#607, #618-#620). Alternatives: +/// +/// 1. `name [: type] = "lit"` / `name := "lit"` (Go) / `"name": "lit"` (JSON +/// quoted key); the key may be quoted. +/// 2. Go `var name type = "lit"` (colon-free type between name and `=`). +/// 3. SQL `PASSWORD ''` / `IDENTIFIED BY ''` (no `=`). +pub(crate) fn secret_literal_pattern() -> String { + let kw = r"(?:password|api_?key|token|secret)"; + let ty = r"[&*\w<>\[\].?|]+"; + let lit = r#"(?:"[^"]+"|'[^']+')"#; + format!( + r#"(?i){kw}["']?(?:\s*:\s*{ty})?\s*(?::=|=|["']\s*:)\s*{lit}|(?i){kw}["']?\s+{ty}\s*=\s*{lit}|{SQL_PASSWORD_PATTERN}"# + ) +} + +/// SQL password literal without `=`: `... WITH PASSWORD 'x'`, `IDENTIFIED BY 'x'`. +const SQL_PASSWORD_PATTERN: &str = r"(?i)\b(?:password|identified\s+by)\s+'([^']+)'"; + +static SQL_PASSWORD_RE: LazyLock = + LazyLock::new(|| Regex::new(SQL_PASSWORD_PATTERN).expect("sql password regex must compile")); + +static SECRET_LITERAL_RE: LazyLock = LazyLock::new(|| { + Regex::new(&secret_literal_pattern()).expect("hardcoded secret regex must compile") +}); + +/// Does `line` show a hardcoded secret literal the deterministic detectors +/// report? Used by the LLM cross-check so LLM findings on the same shapes are +/// kept. A SQL placeholder (`PASSWORD '%s'`) is not a literal. +pub(crate) fn has_secret_literal(line: &str) -> bool { + if let Some(c) = SQL_PASSWORD_RE.captures(line) + && is_sql_placeholder(&c[1]) + { + return false; + } + if quoted_placeholder_value(line) { + return false; + } + SECRET_LITERAL_RE.is_match(line) || plain_scalar_secret(strip_trailing_comment(line)) +} + +/// A SQL literal that is a bind/format placeholder, not a password: +/// `%s`, `%v`, `$1`, `?`, `:name`, `@name`, `{}` / `{pw}` / `${PW}`, ``. +fn is_sql_placeholder(lit: &str) -> bool { + let lit = lit.trim(); + lit.starts_with(['$', ':', '@', '?']) + || lit.contains('%') + || (lit.contains('{') && lit.contains('}')) + || (lit.starts_with('<') && lit.ends_with('>')) +} + +/// Value of a whole-line `key: value` entry (YAML, JSON, `- key: value`) whose +/// key ends in a secret word. The line must be nothing but the entry (an +/// optional `- ` bullet, an optional trailing comma), so object shorthand inside +/// code (`{ password: formPassword }`, `pub password: String,`) never qualifies. +fn secret_kv_value(line: &str) -> Option<&str> { + static KV_RE: LazyLock = LazyLock::new(|| { + Regex::new( + r#"(?i)^\s*(?:-\s+)?["']?[\w.\-]*(?:password|passwd|pwd|secret|api_?key|token)["']?\s*:\s+(\S.*?)\s*,?\s*$"#, + ) + .expect("kv regex must compile") + }); + KV_RE + .captures(line) + .and_then(|c| c.get(1)) + .map(|m| m.as_str()) +} + +/// Unquoted YAML scalar that looks like a real secret (#619). Conservative: +/// a single token of >= 8 chars with a digit or symbol in it, so bare words +/// (`required`, `formPassword`, `vault_db_password`) stay "references". Never +/// a YAML tag/anchor/alias/block indicator, `${..}`/`$VAR`, a path, or a +/// keyword (`true`, `null`, ...). +fn plain_scalar_secret(line: &str) -> bool { + let Some(v) = secret_kv_value(line) else { + return false; + }; + let lower = v.to_ascii_lowercase(); + v.len() >= 8 + && !v.contains(char::is_whitespace) + && !v.starts_with([ + '"', '\'', '`', '$', '!', '&', '*', '|', '>', '%', '@', '{', '[', '/', '~', '<', '.', + '?', '-', + ]) + && !v.contains("://") + && !v.contains(['{', '}', '(', ')', ';', '#']) + && !matches!( + lower.as_str(), + "true" | "false" | "null" | "none" | "nil" | "yes" | "no" + ) + && v.chars() + .any(|c| c.is_ascii_digit() || "!@#%^&*+=".contains(c)) +} + +/// Quoted `key: "value"` entry whose value is empty or an interpolation / +/// template placeholder (`${X}`, `$X`, `{{ x }}`): a reference, not a secret. +fn quoted_placeholder_value(line: &str) -> bool { + // Unanchored on purpose: inline JSON (`{"password": "${X}", "apiKey": ""}`) + // and call arguments carry the pair in the middle of the line. Every quoted + // secret pair on the line must be a placeholder to suppress the finding, so a + // real literal next to a placeholder is still reported. + static PAIR_RE: LazyLock = LazyLock::new(|| { + Regex::new( + r#"(?i)["']?[\w.\-]*(?:password|passwd|pwd|secret|api_?key|token)["']?\s*:\s*("[^"]*"|'[^']*')"#, + ) + .expect("pair regex must compile") + }); + let mut seen = false; + for cap in PAIR_RE.captures_iter(line) { + let v = &cap[1]; + let inner = &v[1..v.len() - 1]; + let placeholder = inner.is_empty() + || (inner.starts_with("${") && inner.ends_with('}')) + || (inner.starts_with("{{") && inner.ends_with("}}")) + || (inner.starts_with('$') + && inner.len() > 1 + && inner[1..] + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_')); + if !placeholder { + return false; + } + seen = true; + } + seen +} + /// Post-match filter for rules that need additional validation after regex match. /// Returns `true` to suppress a finding that the regex matched but should be ignored. pub fn post_match_filter(rule_id: &str, line: &str) -> bool { @@ -285,6 +412,29 @@ fn is_false_positive_secret(line: &str) -> bool { return true; } + // SQL placeholders in format strings: PASSWORD '%s', IDENTIFIED BY '$1' (#620) + if let Some(c) = SQL_PASSWORD_RE.captures(line) + && is_sql_placeholder(&c[1]) + { + return true; + } + + // `"password": "${X}"` / `password: "{{ x }}"`: a reference, not a secret (#619) + if quoted_placeholder_value(line) { + return true; + } + + // YAML alias / anchor / tag / path / `~` reference (`password: *dbpass`, + // `password: /run/secrets/db`): points at another value, not a secret (#619) + if secret_kv_value(line).is_some_and(|v| v.starts_with(['*', '&', '!', '/', '~'])) { + return true; + } + + // Unquoted YAML `password: hunter2hunter2` is a real secret, not shorthand (#619) + if plain_scalar_secret(line) { + return false; + } + // Object shorthand where the value is a variable reference, not a literal // e.g., { app_secret: formAppSecret } — RHS is a variable name, not a secret value // Variable references: no quotes, no digits mixed with special chars diff --git a/src/engine/secret_heuristics_tests.rs b/src/engine/secret_heuristics_tests.rs index 4107dcf..4ee588c 100644 --- a/src/engine/secret_heuristics_tests.rs +++ b/src/engine/secret_heuristics_tests.rs @@ -141,6 +141,17 @@ const CASES: &[Row] = &[ ("go", "const token = `abcd1234efgh5678`", true), ("go", r#"password := os.Getenv("DB_PASSWORD")"#, false), ("go", r#"password := """#, false), + // #618: short declarations and colon-free types + ("go", r#"password := "hunter2hunter2""#, true), + ("go", r#"var apiKey string = "abcd1234efgh5678""#, true), + ("go", r#"password := "hunter2hunter2" // note: temp"#, true), + ( + "go", + r#"password := "hunter2hunter2" // cora-ignore: crypto/hardcoded-secret"#, + false, + ), + ("go", r#"var apiKey string = """#, false), + ("go", r#"var apiKey string = os.Getenv("API_KEY")"#, false), // ── java / kotlin ── ( "java", @@ -162,8 +173,44 @@ const CASES: &[Row] = &[ ("yaml", "api_key: 'abcd1234efgh5678'", true), ("yaml", "password: ${DB_PASSWORD}", false), ("yaml", "password: \"\"", false), + // #619: unquoted YAML scalars + ("yaml", "password: hunter2hunter2", true), + ("yaml", " db_password: hunter2hunter2", true), + ("yaml", "- token: abcd1234efgh5678", true), + ("yaml", "password: hunter2hunter2 # prod db", true), + ("yaml", "password: !secret db", false), + ("yaml", "password: !vault db_password_1", false), + ("yaml", "password: null", false), + ("yaml", "password: ~", false), + ("yaml", "password: required", false), + ("yaml", "password: *dbpass1", false), + ("yaml", "password: &dbpass1 hunter2", false), + ("yaml", "password: $DB_PASSWORD_1", false), + ("yaml", "password: \"${DB_PASSWORD}\"", false), + ("yaml", "password_file: /run/secrets/db_password", false), + ("yaml", "password: /run/secrets/db_password", false), + ("yaml", "secretName: db-credentials-secret", false), + ("ts", " secret: Secret1Type", true), + // #619: quoted JSON keys + ("json", r#" "password": "hunter2hunter2","#, true), + ("json", r#" "apiKey": "abcd1234efgh5678""#, true), + ("json", r#" "password": "hunter2hunter2" "#, true), + ("json", r#" "apiKey": "${API_KEY}""#, false), + ("json", r#" "password": "$DB_PASSWORD","#, false), + ("json", r#" "password": "{{ db_password }}","#, false), + ("json", r#" "password": null,"#, false), ("json", r#" "password": "","#, false), ("json", r#" "password": "${DB_PASSWORD}","#, false), + // inline JSON: placeholders stay quiet, a real literal beside one is still flagged + ("json", r#"{"password": "${X}"}"#, false), + ("json", r#"{"password": "${X}", "apiKey": ""}"#, false), + ("json", r#"{"password": "", "token": "${T}"}"#, false), + ( + "json", + r#"{"password": "${X}", "token": "hunter2hunter2xx"}"#, + true, + ), + ("json", r#"{"password": "hunter2hunter2xx"}"#, true), // ── env / .properties ── ("env", "DB_PASSWORD=hunter2hunter2", true), ("env", "API_KEY=abcd1234efgh5678", true), @@ -186,6 +233,36 @@ const CASES: &[Row] = &[ true, ), ("sql", "ALTER ROLE app SET password = '';", false), + // #620: PASSWORD '' without `=` + ( + "sql", + "CREATE USER app WITH PASSWORD 'hunter2hunter2';", + true, + ), + ("sql", "ALTER USER app PASSWORD 'hunter2hunter2';", true), + ( + "sql", + "CREATE USER app IDENTIFIED BY 'hunter2hunter2';", + true, + ), + ( + "sql", + "CREATE USER app WITH PASSWORD 'hunter2hunter2'; -- note: temp", + true, + ), + ("sql", "CREATE USER app WITH PASSWORD '';", false), + ("sql", "CREATE USER app WITH PASSWORD '%s';", false), + ("sql", "CREATE USER app WITH PASSWORD '$1';", false), + ("sql", "CREATE USER app WITH PASSWORD ':pw';", false), + ("sql", "CREATE USER app WITH PASSWORD '{password}';", false), + ("sql", "CREATE USER app IDENTIFIED BY '?';", false), + ("sql", "CREATE USER app WITH PASSWORD NULL;", false), + ("go", r#"q := "CREATE USER app WITH PASSWORD '%s'""#, false), + ( + "py", + r#"q = f"CREATE USER app WITH PASSWORD '{pw}'""#, + false, + ), ]; /// Rows exposing a real false negative/positive that is not fixed yet. Each @@ -220,20 +297,6 @@ const KNOWN_GAPS: &[Row] = &[ ("env", "DB_PASSWORD=${DB_PASSWORD_FROM_VAULT}", true), ("properties", "db.password=${DB_PASSWORD}", true), ("sh", r#"export DB_PASSWORD="$VAULT_DB_PASSWORD""#, true), - // KNOWN GAP #618 - FN: Go `:=` short declarations are never matched by the regexes (ideal: flagged) - ("go", r#"password := "hunter2hunter2""#, false), - ("go", r#"var apiKey string = "abcd1234efgh5678""#, false), - ("go", r#"password := "hunter2hunter2" // note: temp"#, false), - // KNOWN GAP #619 - FN: unquoted YAML scalar is mistaken for object shorthand; quoted JSON keys never match (ideal: flagged) - ("yaml", "password: hunter2hunter2", false), - ("json", r#" "password": "hunter2hunter2","#, false), - ("json", r#" "apiKey": "abcd1234efgh5678""#, false), - // KNOWN GAP #620 - FN: SQL `PASSWORD ''` (no `=`) is not matched (ideal: flagged) - ( - "sql", - "CREATE USER app WITH PASSWORD 'hunter2hunter2';", - false, - ), ]; #[test] diff --git a/src/engine/security_scanner.rs b/src/engine/security_scanner.rs index ebf4e65..80cfd17 100644 --- a/src/engine/security_scanner.rs +++ b/src/engine/security_scanner.rs @@ -45,7 +45,11 @@ pub static PATTERNS: &[SecurityPattern] = &[ SecurityPattern { id: "crypto/hardcoded-secret", name: "Hardcoded password or secret in variable", - regex: r"(?i)(?:password|passwd|pwd|secret|api_key|apikey|token)(?:\s*:\s*[&\w<>\[\].?|]+)?\s*[=:]\s*\S{8,}", + // Alternatives: (1) `name [: type] =|:` value, `:=` only before a quoted + // literal (Go `password := "..."`, never `:= os.Getenv(..)`, #618), key + // may be quoted (JSON, #619); (2) Go `var name type = value`; (3) SQL + // `PASSWORD ''` / `IDENTIFIED BY ''` (#620). + regex: r#"(?i)(?:password|passwd|pwd|secret|api_key|apikey|token)["']?(?:\s*:\s*[&\w<>\[\].?|]+)?\s*(?:[=:]|:=\s*["'`])\s*\S{8,}|(?:password|passwd|pwd|secret|api_key|apikey|token)["']?\s+[&*\w<>\[\].?|]+\s*=\s*["'`]\S{7,}|\b(?:password|identified\s+by)\s+'[^']+'"#, severity: Severity::Critical, }, // ── Injection ──