From c44724c1f9ba5192f4e59d01545557adc576648c Mon Sep 17 00:00:00 2001 From: ajianaz Date: Tue, 4 Aug 2026 16:47:31 +0700 Subject: [PATCH] fix(rules+cache): align builtin rule ID in post_match_filter and add provider/base_url to cache key Bug 1: post_match_filter ID mismatch (sec-hardcoded-secret vs crypto/hardcoded-secret) - Builtin rule 'sec-hardcoded-secret' was never filtered by post_match_filter because the match arm only checked 'crypto/hardcoded-secret' (security_scanner.rs ID) - Added 'sec-hardcoded-secret' as an alias in the match arm - Added regression tests for builtin rule ID Bug 2: Cache key missing provider and base_url - cache_key() only hashed diff+model+temperature - Switching providers with same model name returned stale cache from previous provider - Added provider and base_url to cache_key, get_cached_review, save_cached_review - Added regression tests for provider/base_url cache isolation Tests: 781 pass (5 new), clippy clean, 6 integration pass --- src/engine/cache.rs | 99 +++++++++++++++++++++++++++++++------ src/engine/review.rs | 4 ++ src/engine/rules/builtin.rs | 34 ++++++++++++- 3 files changed, 120 insertions(+), 17 deletions(-) diff --git a/src/engine/cache.rs b/src/engine/cache.rs index 8f0926e..bbb3ce8 100644 --- a/src/engine/cache.rs +++ b/src/engine/cache.rs @@ -14,12 +14,15 @@ fn cache_dir() -> std::result::Result { } /// Compute SHA-256 hex digest of the diff content + config parameters. -/// Includes model and temperature so config changes invalidate the cache. +/// Includes model, provider, base_url, and temperature so config changes +/// invalidate the cache (e.g., switching providers with the same model name). #[allow(clippy::format_collect)] -fn cache_key(diff: &str, model: &str, temperature: f32) -> String { +fn cache_key(diff: &str, model: &str, temperature: f32, provider: &str, base_url: &str) -> String { let mut hasher = Sha256::new(); hasher.update(diff.as_bytes()); hasher.update(model.as_bytes()); + hasher.update(provider.as_bytes()); + hasher.update(base_url.as_bytes()); hasher.update(temperature.to_le_bytes()); let result = hasher.finalize(); result.iter().map(|b| format!("{b:02x}")).collect() @@ -34,8 +37,10 @@ pub fn get_cached_review( model: &str, temperature: f32, ttl: u64, + provider: &str, + base_url: &str, ) -> Option { - let hash = cache_key(diff, model, temperature); + let hash = cache_key(diff, model, temperature, provider, base_url); let dir = cache_dir().ok()?; let path = dir.join(format!("{hash}.json")); @@ -78,11 +83,13 @@ pub fn save_cached_review( model: &str, temperature: f32, response: &ReviewResponse, + provider: &str, + base_url: &str, ) -> std::result::Result<(), CoraError> { let dir = cache_dir()?; std::fs::create_dir_all(&dir).map_err(CoraError::CacheIo)?; - let hash = cache_key(diff, model, temperature); + let hash = cache_key(diff, model, temperature, provider, base_url); let path = dir.join(format!("{hash}.json")); let now = SystemTime::now() @@ -136,24 +143,54 @@ mod tests { #[test] fn cache_key_is_deterministic() { - let hash1 = cache_key("hello world", "gpt-4", 0.0); - let hash2 = cache_key("hello world", "gpt-4", 0.0); + let hash1 = cache_key( + "hello world", + "gpt-4", + 0.0, + "openai", + "https://api.openai.com/v1", + ); + let hash2 = cache_key( + "hello world", + "gpt-4", + 0.0, + "openai", + "https://api.openai.com/v1", + ); assert_eq!(hash1, hash2); assert_eq!(hash1.len(), 64); // SHA-256 hex = 64 chars } #[test] fn cache_key_differs_for_different_inputs() { - let hash1 = cache_key("hello world", "gpt-4", 0.0); - let hash2 = cache_key("hello earth", "gpt-4", 0.0); + let hash1 = cache_key( + "hello world", + "gpt-4", + 0.0, + "openai", + "https://api.openai.com/v1", + ); + let hash2 = cache_key( + "hello earth", + "gpt-4", + 0.0, + "openai", + "https://api.openai.com/v1", + ); assert_ne!(hash1, hash2); } #[test] fn cache_key_includes_model_and_temperature() { - let h1 = cache_key("diff", "gpt-4", 0.0); - let h2 = cache_key("diff", "gpt-3.5", 0.0); - let h3 = cache_key("diff", "gpt-4", 0.7); + let h1 = cache_key("diff", "gpt-4", 0.0, "openai", "https://api.openai.com/v1"); + let h2 = cache_key( + "diff", + "gpt-3.5", + 0.0, + "openai", + "https://api.openai.com/v1", + ); + let h3 = cache_key("diff", "gpt-4", 0.7, "openai", "https://api.openai.com/v1"); assert_ne!(h1, h2, "different models should differ"); assert_ne!(h1, h3, "different temperatures should differ"); } @@ -161,7 +198,7 @@ mod tests { #[test] fn cache_key_len_is_64() { let diff = "diff --git a/file.txt b/file.txt\n+ hello"; - let hash = cache_key(diff, "model", 0.0); + let hash = cache_key(diff, "model", 0.0, "openai", "https://api.openai.com/v1"); assert_eq!(hash.len(), 64); } @@ -169,11 +206,43 @@ mod tests { fn cache_miss_on_different_diff() { let diff1 = "diff --git a/a.txt b/a.txt\n+ hello"; let diff2 = "diff --git a/b.txt b/b.txt\n+ world"; - let hash1 = cache_key(diff1, "model", 0.0); - let hash2 = cache_key(diff2, "model", 0.0); + let hash1 = cache_key(diff1, "model", 0.0, "openai", "https://api.openai.com/v1"); + let hash2 = cache_key(diff2, "model", 0.0, "openai", "https://api.openai.com/v1"); assert_ne!(hash1, hash2); } + #[test] + fn cache_key_differs_for_different_providers() { + let h1 = cache_key("diff", "gpt-4", 0.0, "openai", "https://api.openai.com/v1"); + let h2 = cache_key( + "diff", + "gpt-4", + 0.0, + "azure", + "https://my-azure.openai.azure.com", + ); + assert_ne!( + h1, h2, + "different providers should produce different cache keys" + ); + } + + #[test] + fn cache_key_differs_for_different_base_urls() { + let h1 = cache_key("diff", "gpt-4", 0.0, "openai", "https://api.openai.com/v1"); + let h2 = cache_key( + "diff", + "gpt-4", + 0.0, + "openai", + "https://proxy.example.com/v1", + ); + assert_ne!( + h1, h2, + "different base_urls should produce different cache keys" + ); + } + #[test] fn cached_review_serialization_roundtrip() { let response = make_response(); @@ -246,7 +315,7 @@ mod tests { // Manually set up a cache entry in the temp dir let diff = "test diff content"; - let hash = cache_key(diff, "model", 0.0); + let hash = cache_key(diff, "model", 0.0, "openai", "https://api.openai.com/v1"); let path = dir.join(format!("{hash}.json")); let response = make_response(); diff --git a/src/engine/review.rs b/src/engine/review.rs index bf732ec..cb83f7f 100644 --- a/src/engine/review.rs +++ b/src/engine/review.rs @@ -106,6 +106,8 @@ async fn review_diff_inner( &llm_config.model, llm_config.temperature, config.cache_ttl, + &llm_config.provider, + &llm_config.base_url, ) { debug!("returning cached review response"); return Ok(cached); @@ -453,6 +455,8 @@ async fn review_diff_inner( &llm_config.model, llm_config.temperature, &response, + &llm_config.provider, + &llm_config.base_url, ) { debug!("failed to save review to cache: {}", e); } diff --git a/src/engine/rules/builtin.rs b/src/engine/rules/builtin.rs index bc63410..c77f211 100644 --- a/src/engine/rules/builtin.rs +++ b/src/engine/rules/builtin.rs @@ -127,8 +127,8 @@ pub fn builtin_rules() -> Vec { /// 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 { match rule_id { + "sec-hardcoded-secret" | "crypto/hardcoded-secret" => is_false_positive_secret(line), "sec-hardcoded-url" => is_false_positive_url(line), - "crypto/hardcoded-secret" => is_false_positive_secret(line), _ => false, } } @@ -403,7 +403,37 @@ mod tests { )); assert!(!post_match_filter( "crypto/hardcoded-secret", - "const API_KEY = \"sk-abc123def456gh\"" + "const API_KEY = \"***\"" + )); + } + + // ─── sec-hardcoded-secret (builtin rule ID) false positive tests ─── + + #[test] + fn builtin_rule_id_secret_empty_string_is_false_positive() { + assert!(post_match_filter( + "sec-hardcoded-secret", + "let formAppSecret = $state('');" + )); + assert!(post_match_filter( + "sec-hardcoded-secret", + "let password = '';" + )); + } + + #[test] + fn builtin_rule_id_secret_svelte_state_is_false_positive() { + assert!(post_match_filter( + "sec-hardcoded-secret", + "let formPassword = $state('default12345678');" + )); + } + + #[test] + fn builtin_rule_id_secret_actual_hardcoded_is_real_finding() { + assert!(!post_match_filter( + "sec-hardcoded-secret", + "let password = supersecret12345" )); } }