diff --git a/CHANGELOG.md b/CHANGELOG.md index 23572b8..c30cede 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.17.2] - 2026-10-09 + +### Fixed + +- **Env reads, call expressions and shell interpolation are no longer reported as hardcoded secrets.** `process.env.X`, `os.environ["X"]`, `os.getenv("X")`, `std::env::var("X")?`, `System.getenv("X")`, `get_password_from_vault()`, `String::new()` and bare identifiers/member accesses (`const password = userInput;`) are expressions, not literals, in source files (#616). `DB_PASSWORD=${VAR}`, `db.password=${DB_PASSWORD}`, `export X="$VAULT_X"` and `$(cmd)` are references (#617). Real values stay flagged: quoted or concatenated literals in code, and bare tokens in `.env`/`.properties`/`.ini`/`.toml`/shell/YAML files (`DB_PASSWORD=hunter2hunter2xx`). The LLM secret cross-check uses the same classification. + ## [0.17.1] - 2026-10-09 ### Fixed @@ -980,7 +986,8 @@ Benchmarked on the cora-code repository (1,864 symbols, 115 Rust files, x86_64): - **Cross-platform** — Linux (x86_64, ARM64), macOS (Apple Silicon), Windows (x86_64) - **MIT License** — fully open source -[Unreleased]: https://github.com/codecoradev/cora-code/compare/v0.17.1...develop +[Unreleased]: https://github.com/codecoradev/cora-code/compare/v0.17.2...develop +[0.17.2]: https://github.com/codecoradev/cora-code/compare/v0.17.1...v0.17.2 [0.17.1]: https://github.com/codecoradev/cora-code/compare/v0.17.0...v0.17.1 [0.17.0]: https://github.com/codecoradev/cora-code/compare/v0.16.1...v0.17.0 [0.16.1]: https://github.com/codecoradev/cora-code/compare/v0.16.0...v0.16.1 diff --git a/Cargo.lock b/Cargo.lock index afb8ad8..9287b14 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -303,7 +303,7 @@ dependencies = [ [[package]] name = "cora-code" -version = "0.17.1" +version = "0.17.2" dependencies = [ "anyhow", "assert_cmd", diff --git a/Cargo.toml b/Cargo.toml index 0b81246..c6048c9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cora-code" -version = "0.17.1" +version = "0.17.2" edition = "2024" description = "CLI-first AI code review — BYOK, diff/scan/branch, pre-commit hooks" license = "Apache-2.0" diff --git a/src/engine/postprocess.rs b/src/engine/postprocess.rs index 7489822..85d44d7 100644 --- a/src/engine/postprocess.rs +++ b/src/engine/postprocess.rs @@ -149,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 !crate::engine::rules::builtin::has_secret_literal(actual_line) { + if !crate::engine::rules::builtin::has_secret_literal(actual_line, &issue.file) { debug!( file = %issue.file, line = line_num, diff --git a/src/engine/rules/builtin.rs b/src/engine/rules/builtin.rs index 7de4952..c722962 100644 --- a/src/engine/rules/builtin.rs +++ b/src/engine/rules/builtin.rs @@ -155,13 +155,13 @@ static SECRET_LITERAL_RE: LazyLock = LazyLock::new(|| { /// 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 { +pub(crate) fn has_secret_literal(line: &str, path: &str) -> bool { if let Some(c) = SQL_PASSWORD_RE.captures(line) && is_sql_placeholder(&c[1]) { return false; } - if quoted_placeholder_value(line) { + if quoted_placeholder_value(line) || non_literal_rhs(strip_trailing_comment(line), path) { return false; } SECRET_LITERAL_RE.is_match(line) || plain_scalar_secret(strip_trailing_comment(line)) @@ -253,11 +253,139 @@ fn quoted_placeholder_value(line: &str) -> bool { seen } +/// How a bare (unquoted) right-hand side of `name = value` reads in a file. +#[derive(Clone, Copy, PartialEq, Eq)] +enum RhsStyle { + /// `.env`, `.properties`, `.ini`, `.toml`, shell, YAML...: a bare token IS + /// the literal value (`DB_PASSWORD=hunter2hunter2`). + Assignment, + /// Source code: a bare RHS is an expression (identifier, member access, + /// call, env read), never a literal. + Code, + /// Path unknown or not classified: keep the legacy behaviour (flag). + Unknown, +} + +fn rhs_style(path: &str) -> RhsStyle { + let file = path + .rsplit(['/', '\\']) + .next() + .unwrap_or(path) + .to_ascii_lowercase(); + if file == ".env" + || file.starts_with(".env.") + || matches!( + file.as_str(), + "dockerfile" | "makefile" | ".npmrc" | ".envrc" + ) + { + return RhsStyle::Assignment; + } + let Some((_, ext)) = file.rsplit_once('.') else { + return RhsStyle::Unknown; + }; + match ext { + "env" | "properties" | "ini" | "cfg" | "conf" | "toml" | "sh" | "bash" | "zsh" | "ksh" + | "envrc" | "yaml" | "yml" | "tfvars" | "mk" | "service" | "dockerfile" => { + RhsStyle::Assignment + } + "js" | "jsx" | "mjs" | "cjs" | "ts" | "tsx" | "mts" | "cts" | "svelte" | "vue" | "py" + | "rs" | "go" | "java" | "kt" | "kts" | "scala" | "groovy" | "gradle" | "c" | "h" + | "cc" | "cpp" | "hpp" | "cs" | "php" | "rb" | "swift" | "dart" | "lua" | "pl" | "ex" + | "exs" | "erl" | "hs" | "clj" | "jl" | "r" | "sql" | "zig" | "nim" | "m" | "mm" => { + RhsStyle::Code + } + _ => RhsStyle::Unknown, + } +} + +/// Right-hand side of the first ` [: type] =|:= ` on the line +/// (`==` comparisons are not assignments). +fn secret_assign_rhs(line: &str) -> Option<&str> { + static ASSIGN_RE: LazyLock = LazyLock::new(|| { + Regex::new( + r#"(?i)(?:password|passwd|pwd|secret|api_?key|token)["']?(?:\s*:\s*[&*\w<>\[\].?|]+|\s+[&*\w<>\[\].?|]+)?\s*(?::=|=)"#, + ) + .expect("assign regex must compile") + }); + let rest = &line[ASSIGN_RE.find(line)?.end()..]; + if rest.starts_with('=') { + return None; + } + Some(rest.trim()) +} + +/// A quoted value that is nothing but a reference: `${X}`, `$X`, `$(cmd)`. +fn is_pure_reference(inner: &str) -> bool { + (inner.starts_with("${") && inner.ends_with('}') && inner.matches("${").count() == 1) + || (inner.starts_with("$(") && inner.ends_with(')')) + || (inner.starts_with('$') + && inner.len() > 1 + && inner[1..] + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_')) +} + +/// Does the RHS start with a quoted literal (`"x"`, `'x'`, `` `x` ``, `b"x"`, +/// `r#"x"#`, `f'x'`) or a literal wrapped by a constructor (`String::from("x")`)? +fn starts_with_literal(rhs: &str) -> bool { + static LIT_RE: LazyLock = LazyLock::new(|| { + Regex::new( + r#"^(?:[A-Za-z]{0,2}#*["'`]|(?:String::from|Some|Secret(?:String)?::(?:new|from)|new\s+String)\s*\(\s*[A-Za-z]{0,2}#*["'`])"#, + ) + .expect("literal-start regex must compile") + }); + LIT_RE.is_match(rhs) +} + +/// Is the right-hand side of a secret-named assignment provably NOT a literal +/// value (#616, #617)? +/// +/// - Quoted RHS: a literal, unless the whole value is a reference (`"${X}"`, +/// `"$VAULT_X"`, `"$(cmd)"`). Concatenations (`"a" + "b"`) stay literals. +/// - Unquoted RHS starting with `$` or containing `${..}` / `$(..)` / a backtick +/// is shell-style interpolation or command substitution, in any file. +/// - Other unquoted RHS: in source code it is an expression (`process.env.X`, +/// `os.getenv("X")`, `get_pw()`, `std::env::var("X")?`, `cfg.token`, `userInput`) +/// so not a secret; in assignment-style files (.env, .properties, shell, YAML) +/// a bare token is the literal value, so it is kept as a finding; in unknown +/// files nothing changes. +fn non_literal_rhs(line: &str, path: &str) -> bool { + let Some(rhs) = secret_assign_rhs(line) else { + return false; + }; + let rhs = rhs.trim_end_matches([';', ',']).trim_end(); + if rhs.is_empty() { + return false; + } + if let Some(q) = rhs.chars().next().filter(|c| matches!(c, '"' | '\'')) { + // Whole-value quote: `"..."` with nothing after the closing quote. + return rhs.len() >= 2 + && rhs.ends_with(q) + && !rhs[1..rhs.len() - 1].contains(q) + && is_pure_reference(&rhs[1..rhs.len() - 1]); + } + if starts_with_literal(rhs) { + return false; + } + if rhs.starts_with('$') || rhs.contains("${") || rhs.contains("$(") || rhs.contains('`') { + return true; + } + rhs_style(path) == RhsStyle::Code +} + /// 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. +#[cfg(test)] pub fn post_match_filter(rule_id: &str, line: &str) -> bool { + post_match_filter_for_path(rule_id, line, "") +} + +/// [`post_match_filter`] with the file path, which lets the hardcoded-secret +/// filter tell source code from assignment-style config files (#616, #617). +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), + "sec-hardcoded-secret" | "crypto/hardcoded-secret" => is_false_positive_secret(line, path), "sec-hardcoded-url" => is_false_positive_url(line), "config/cors-wildcard" => is_false_positive_cors(line), "injection/sql-concat" => is_false_positive_sql_concat(line), @@ -395,12 +523,18 @@ fn first_unquoted_colon(line: &str) -> Option { /// /// Suppresses: empty string values, variable references (no literal secret), /// and UI binding patterns (Svelte $state, bind:value, form fields). -fn is_false_positive_secret(line: &str) -> bool { +fn is_false_positive_secret(line: &str, path: &str) -> bool { // Judge the code only: a trailing comment ("// note: fix later") must not // turn a real secret into an "object shorthand" false positive (#603). let line = strip_trailing_comment(line); let lower = line.to_lowercase(); + // Env reads, call/member expressions, bare identifiers and `${VAR}` + // interpolation on the RHS are not literals (#616, #617). + if non_literal_rhs(line, path) { + return true; + } + // Empty string or empty-ish values: = '' = "" = $state('') if lower.contains("= ''") || lower.contains("= \"\"") || lower.contains("= $state('')") { return true; diff --git a/src/engine/rules/mod.rs b/src/engine/rules/mod.rs index 9c16d3c..d2898d6 100644 --- a/src/engine/rules/mod.rs +++ b/src/engine/rules/mod.rs @@ -75,7 +75,7 @@ pub fn run_rules(chunks: &[FileChunk], config: &RulesConfig) -> Vec } // Post-match filter (e.g., allow localhost URLs) - if builtin::post_match_filter(&rule.id, &line.content) { + if builtin::post_match_filter_for_path(&rule.id, &line.content, file_path) { continue; } diff --git a/src/engine/secret_heuristics_tests.rs b/src/engine/secret_heuristics_tests.rs index 4ee588c..8476620 100644 --- a/src/engine/secret_heuristics_tests.rs +++ b/src/engine/secret_heuristics_tests.rs @@ -263,6 +263,95 @@ const CASES: &[Row] = &[ r#"q = f"CREATE USER app WITH PASSWORD '{pw}'""#, false, ), + // ── #616: env reads / call / member expressions / identifiers are not literals ── + ("js", "const token = process.env.API_TOKEN;", false), + ( + "js", + "const secret = process.env.SESSION_SECRET || fallbackValue;", + false, + ), + ("py", r#"password = os.environ["DB_PASSWORD"]"#, false), + ("py", r#"password = os.getenv("DB_PASSWORD")"#, false), + ("py", "password = get_password_from_vault()", false), + ("rs", r#"let token = std::env::var("API_TOKEN")?;"#, false), + ( + "rs", + r#"let token = std::env::var("API_TOKEN").unwrap_or_default();"#, + false, + ), + ("rs", r#"let password = String::new();"#, false), + ( + "java", + r#"String password = System.getenv("DB_PASSWORD");"#, + false, + ), + ( + "kt", + r#"val password = System.getenv("DB_PASSWORD")"#, + false, + ), + ("js", "const password = userInput;", false), + ("rs", "let token = cfg.token_value_here;", false), + ("ts", "const apiKey: string = config.apiKeyValue;", false), + ("rb", r#"password = ENV["DB_PASSWORD"]"#, false), + ("php", r#"$password = $_ENV['DB_PASSWORD'];"#, false), + ( + "cs", + "var token = Environment.GetEnvironmentVariable(name);", + false, + ), + // a quoted or concatenated literal in code is still a secret + ("js", r#"password = "abc" + "defgh12345";"#, true), + ("py", r#"password = b"hunter2hunter2xx""#, true), + ("py", r#"password = f"hunter2hunter2xx""#, true), + ("rs", r##"let password = r#"hunter2hunter2xx"#;"##, true), + ( + "rs", + r#"let password = String::from("hunter2hunter2xx");"#, + true, + ), + ( + "rs", + r#"let password = "hunter2hunter2xx".to_string();"#, + true, + ), + // ── #617: shell-style interpolation is a reference, a real value is not ── + ("env", "DB_PASSWORD=${DB_PASSWORD_FROM_VAULT}", false), + ("properties", "db.password=${DB_PASSWORD}", false), + ("sh", r#"export DB_PASSWORD="$VAULT_DB_PASSWORD""#, false), + ("env", "DB_PASSWORD=$VAULT_DB_PASSWORD", false), + ( + "sh", + "export API_KEY=$(vault read -field=key secret/api)", + false, + ), + ( + "sh", + r#"export API_KEY="$(vault read -field=key secret/api)""#, + false, + ), + ( + "sh", + r#"export DB_PASSWORD="${DB_PASSWORD_FROM_VAULT}""#, + false, + ), + ("ini", "password=${DB_PASSWORD}", false), + ("yaml", " - DB_PASSWORD=${DB_PASSWORD}", false), + // real values in assignment-style files stay flagged + ("env", "DB_PASSWORD=hunter2hunter2xx", true), + ("sh", "export API_KEY=abcd1234efgh5678", true), + ("properties", "password=hunter2hunter2xx", true), + ("sh", r#"export X_SECRET="hunter2hunter2xx""#, true), + ("env", "DB_PASSWORD=hunter2hunter2xx # prod", true), + ("toml", "api_key = abcd1234efgh5678", true), + ("ini", "password = hunter2hunter2xx", true), + ("yaml", " - DB_PASSWORD=hunter2hunter2xx", true), + // a literal mixed into an interpolation is not a pure reference + ( + "sh", + r#"export DB_PASSWORD="${PREFIX}hunter2hunter2xx""#, + true, + ), ]; /// Rows exposing a real false negative/positive that is not fixed yet. Each @@ -270,33 +359,30 @@ const CASES: &[Row] = &[ /// the ideal result in the trailing comment). Move a row into [`CASES`] with /// the ideal expectation when its issue is fixed. const KNOWN_GAPS: &[Row] = &[ - // KNOWN GAP #616 - FP: env-var reads and call expressions on the RHS are reported as hardcoded secrets (ideal: not flagged) - ("js", "const token = process.env.API_TOKEN;", true), + // KNOWN GAP #628 - FN: an unquoted RHS in source code is treated as an expression, so a real literal default inside a call is missed (ideal: flagged) ( - "js", - "const secret = process.env.SESSION_SECRET || fallbackValue;", - true, + "py", + r#"password = os.environ.get("DB_PASSWORD", "hunter2hunter2xx")"#, + false, ), - ("py", r#"password = os.environ["DB_PASSWORD"]"#, true), - ("py", r#"password = os.getenv("DB_PASSWORD")"#, true), - ("py", "password = get_password_from_vault()", true), - ("rs", r#"let token = std::env::var("API_TOKEN")?;"#, true), ( "rs", - r#"let token = std::env::var("API_TOKEN").unwrap_or_default();"#, - true, + r#"let password = std::env::var("DB_PASSWORD").unwrap_or("hunter2hunter2xx".into());"#, + false, ), - ("rs", r#"let password = String::new();"#, true), + ( + "js", + r#"const password = process.env.DB_PASSWORD || "hunter2hunter2xx";"#, + false, + ), + // KNOWN GAP #630 - FN: the scanner regex needs a >= 8 char token after `=`, so `new String("lit")` and other short-prefixed constructors are never matched (ideal: flagged) ( "java", - r#"String password = System.getenv("DB_PASSWORD");"#, - true, + r#"String password = new String("hunter2hunter2xx");"#, + false, ), - ("kt", r#"val password = System.getenv("DB_PASSWORD")"#, true), - // KNOWN GAP #617 - FP: shell-style `${VAR}`/`$VAR` interpolation in env, .properties and shell files (ideal: not flagged) - ("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 #629 - FN: shell `${VAR:-literal}` default with a real literal is treated as interpolation (ideal: flagged) + ("sh", "DB_PASSWORD=${DB_PASSWORD:-hunter2hunter2xx}", false), ]; #[test] diff --git a/src/engine/security_scanner.rs b/src/engine/security_scanner.rs index 80cfd17..42170e3 100644 --- a/src/engine/security_scanner.rs +++ b/src/engine/security_scanner.rs @@ -10,7 +10,7 @@ use tracing::debug; use crate::engine::Severity; use crate::engine::diff_parser::{DiffLineType, FileChunk}; -use crate::engine::rules::builtin::post_match_filter; +use crate::engine::rules::builtin::post_match_filter_for_path; use crate::engine::rules::types::RuleFinding; // ─── Security patterns ─── @@ -203,7 +203,7 @@ pub fn scan_security(chunks: &[FileChunk], max_findings: usize) -> Vec