From e54db2c1de2e6b08da3d1d8909afeee17bdc508b Mon Sep 17 00:00:00 2001 From: Ross Churchill Date: Fri, 22 May 2026 14:09:58 +0100 Subject: [PATCH 1/2] fix(C-01): read filter file once; hash+parse same buffer; harden CI trust bypass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C-01 (TOCTOU): toml_filter.rs previously called check_trust() (which reads+hashes the file) then fs::read_to_string() a second time. A race-replacement between the two reads could inject arbitrary filter rules. Fix: read once into Vec, compute SHA-256 in-process, call new check_trust_from_hash() which never re-reads the file. CI bypass hardening (Chain A): removed bare CI=true from the allowed list for RTK_TRUST_PROJECT_FILTERS=1. Only verified platform variables (GITHUB_ACTIONS, GITLAB_CI, JENKINS_URL, BUILDKITE) now grant EnvOverride. CI=true is trivially settable in .envrc and was a local-bypass vector. Chain A risk gate: compile_filter() now calls validate_filter_safety() before adding a filter to the registry. Rejects on_empty="pass", head_lines=1, and match_command=".*"/".*+" — the three patterns identified in the Chain A exploit. Also gates all eprintln! sites in trust.rs (2) and toml_filter.rs (13) behind in_hook_mode() to fix C-07 in these files (C-07 helper landed in prior commit). Adds TOCTOU regression test and CI-bypass test in trust.rs test module. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/core/toml_filter.rs | 137 ++++++++++++++++++++++++++++-------- src/hooks/trust.rs | 151 +++++++++++++++++++++++++++++++++++----- 2 files changed, 241 insertions(+), 47 deletions(-) diff --git a/src/core/toml_filter.rs b/src/core/toml_filter.rs index 06060d22d..aa5a6fc37 100644 --- a/src/core/toml_filter.rs +++ b/src/core/toml_filter.rs @@ -189,28 +189,52 @@ impl TomlFilterRegistry { let mut filters = Vec::new(); // Priority 1: project-local .rtk/filters.toml (trust-gated) + // C-01 fix: read once into raw bytes, hash in-process, parse same buffer. + // This closes the TOCTOU race: no second read between hash and parse. let project_filter_path = std::path::Path::new(".rtk/filters.toml"); if project_filter_path.exists() { - let trust_status = crate::hooks::trust::check_trust(project_filter_path) + if let Ok(raw) = std::fs::read(project_filter_path) { + let hash = { + use sha2::{Digest, Sha256}; + let mut h = Sha256::new(); + h.update(&raw); + format!("{:x}", h.finalize()) + }; + let trust_status = crate::hooks::trust::check_trust_from_hash( + project_filter_path, + &hash, + ) .unwrap_or(crate::hooks::trust::TrustStatus::Untrusted); - match trust_status { - crate::hooks::trust::TrustStatus::Trusted - | crate::hooks::trust::TrustStatus::EnvOverride => { - if let Ok(content) = std::fs::read_to_string(project_filter_path) { + match trust_status { + crate::hooks::trust::TrustStatus::Trusted + | crate::hooks::trust::TrustStatus::EnvOverride => { + let content = String::from_utf8_lossy(&raw); match Self::parse_and_compile(&content, "project") { Ok(f) => filters.extend(f), - Err(e) => eprintln!("[rtk] warning: .rtk/filters.toml: {}", e), + Err(e) => { + if !crate::core::utils::in_hook_mode() { + eprintln!("[rtk] warning: .rtk/filters.toml: {}", e); + } + } + } + } + crate::hooks::trust::TrustStatus::Untrusted => { + if !crate::core::utils::in_hook_mode() { + eprintln!( + "[rtk] WARNING: untrusted project filters (.rtk/filters.toml)" + ); + eprintln!( + "[rtk] Filters NOT applied. Run `rtk trust` to review and enable." + ); + } + } + crate::hooks::trust::TrustStatus::ContentChanged { .. } => { + if !crate::core::utils::in_hook_mode() { + eprintln!("[rtk] WARNING: .rtk/filters.toml changed since trusted."); + eprintln!("[rtk] Filters NOT applied. Run `rtk trust` to re-review."); } } - } - crate::hooks::trust::TrustStatus::Untrusted => { - eprintln!("[rtk] WARNING: untrusted project filters (.rtk/filters.toml)"); - eprintln!("[rtk] Filters NOT applied. Run `rtk trust` to review and enable."); - } - crate::hooks::trust::TrustStatus::ContentChanged { .. } => { - eprintln!("[rtk] WARNING: .rtk/filters.toml changed since trusted."); - eprintln!("[rtk] Filters NOT applied. Run `rtk trust` to re-review."); } } } @@ -221,7 +245,11 @@ impl TomlFilterRegistry { if let Ok(content) = std::fs::read_to_string(&global_path) { match Self::parse_and_compile(&content, "user-global") { Ok(f) => filters.extend(f), - Err(e) => eprintln!("[rtk] warning: {}: {}", global_path.display(), e), + Err(e) => { + if !crate::core::utils::in_hook_mode() { + eprintln!("[rtk] warning: {}: {}", global_path.display(), e); + } + } } } } @@ -230,7 +258,11 @@ impl TomlFilterRegistry { let builtin = BUILTIN_TOML; match Self::parse_and_compile(builtin, "builtin") { Ok(f) => filters.extend(f), - Err(e) => eprintln!("[rtk] warning: builtin filters: {}", e), + Err(e) => { + if !crate::core::utils::in_hook_mode() { + eprintln!("[rtk] warning: builtin filters: {}", e); + } + } } TomlFilterRegistry { filters } @@ -251,7 +283,11 @@ impl TomlFilterRegistry { for (name, def) in file.filters { match compile_filter(name.clone(), def) { Ok(f) => compiled.push(f), - Err(e) => eprintln!("[rtk] warning: filter '{}' in {}: {}", name, source, e), + Err(e) => { + if !crate::core::utils::in_hook_mode() { + eprintln!("[rtk] warning: filter '{}' in {}: {}", name, source, e); + } + } } } Ok(compiled) @@ -313,12 +349,43 @@ const RUST_HANDLED_COMMANDS: &[&str] = &[ "learn", ]; +/// Reject filter definitions that are known to produce trivially abusable behaviour. +/// Called before the filter enters the registry (Chain A risk gate). +fn validate_filter_safety(name: &str, def: &TomlFilterDef) -> Result<(), String> { + if def.on_empty.as_deref() == Some("pass") { + return Err(format!( + "filter '{}': on_empty=\"pass\" is unsafe — an empty output silently passes through \ + all content; use on_empty=\"message\" or omit on_empty", + name + )); + } + if def.head_lines == Some(1) { + return Err(format!( + "filter '{}': head_lines=1 is unsafe — a single-line summary can hide all remaining \ + output from the LLM; use head_lines >= 2 or a different truncation strategy", + name + )); + } + if matches!(def.match_command.as_str(), ".*" | ".+" | "^.*$" | "^.+$") { + return Err(format!( + "filter '{}': match_command=\"{}\" matches every command — this filter applies \ + globally and may suppress output for commands it was not designed for; use a \ + specific command prefix or name", + name, def.match_command + )); + } + Ok(()) +} + fn compile_filter(name: String, def: TomlFilterDef) -> Result { // Mutual exclusion: strip and keep cannot both be set if !def.strip_lines_matching.is_empty() && !def.keep_lines_matching.is_empty() { return Err("strip_lines_matching and keep_lines_matching are mutually exclusive".into()); } + // Chain A risk gate: reject known-dangerous filter patterns before compilation. + validate_filter_safety(&name, &def)?; + let match_regex = Regex::new(&def.match_command) .map_err(|e| format!("invalid match_command regex: {}", e))?; @@ -326,11 +393,13 @@ fn compile_filter(name: String, def: TomlFilterDef) -> Result) -> VerifyResults { } } _ => { - eprintln!("[rtk] WARNING: untrusted project filters skipped in verify"); + if !crate::core::utils::in_hook_mode() { + eprintln!("[rtk] WARNING: untrusted project filters skipped in verify"); + } } } } @@ -605,7 +676,9 @@ fn collect_test_outcomes( let file: TomlFilterFile = match toml::from_str(content) { Ok(f) => f, Err(e) => { - eprintln!("[rtk] warning: TOML parse error during verify: {}", e); + if !crate::core::utils::in_hook_mode() { + eprintln!("[rtk] warning: TOML parse error during verify: {}", e); + } return; } }; @@ -618,7 +691,11 @@ fn collect_test_outcomes( Ok(f) => { compiled_filters.insert(name, f); } - Err(e) => eprintln!("[rtk] warning: filter '{}' compilation error: {}", name, e), + Err(e) => { + if !crate::core::utils::in_hook_mode() { + eprintln!("[rtk] warning: filter '{}' compilation error: {}", name, e); + } + } } } @@ -635,10 +712,12 @@ fn collect_test_outcomes( let compiled = match compiled_filters.get(&filter_name) { Some(f) => f, None => { - eprintln!( - "[rtk] warning: [[tests.{}]] references unknown filter", - filter_name - ); + if !crate::core::utils::in_hook_mode() { + eprintln!( + "[rtk] warning: [[tests.{}]] references unknown filter", + filter_name + ); + } continue; } }; diff --git a/src/hooks/trust.rs b/src/hooks/trust.rs index f93fe6b73..2fbfe3b65 100644 --- a/src/hooks/trust.rs +++ b/src/hooks/trust.rs @@ -89,35 +89,43 @@ fn canonical_key(filter_path: &Path) -> Result { // Public API // --------------------------------------------------------------------------- -/// Check if a project-local filter file is trusted. +/// Check if a project-local filter file is trusted using a pre-computed hash. +/// +/// Fixes C-01 (TOCTOU): the caller reads the file once, hashes the bytes +/// in-process, then passes the hash here. We never re-read the file, so +/// a race-replacement between hash and parse is impossible. /// /// Priority: env var > hash match > untrusted. /// All errors are soft — if anything fails, returns Untrusted (fail-secure). -pub fn check_trust(filter_path: &Path) -> Result { +pub fn check_trust_from_hash(filter_path: &Path, pre_computed_hash: &str) -> Result { // Fast path: env var override for CI pipelines only. - // Requires a known CI env var to be set to prevent .envrc injection attacks. + // Requires a platform-specific CI var — the generic CI=true is intentionally + // excluded because it is trivial to set locally (.envrc injection). if std::env::var("RTK_TRUST_PROJECT_FILTERS").as_deref() == Ok("1") { - let in_ci = std::env::var("CI").is_ok() - || std::env::var("GITHUB_ACTIONS").is_ok() + let in_verified_ci = std::env::var("GITHUB_ACTIONS").is_ok() || std::env::var("GITLAB_CI").is_ok() || std::env::var("JENKINS_URL").is_ok() || std::env::var("BUILDKITE").is_ok(); - if in_ci { + if in_verified_ci { return Ok(TrustStatus::EnvOverride); } - eprintln!( - "[rtk] WARNING: RTK_TRUST_PROJECT_FILTERS=1 ignored (CI environment not detected)" - ); + if !crate::core::utils::in_hook_mode() { + eprintln!( + "[rtk] WARNING: RTK_TRUST_PROJECT_FILTERS=1 ignored (no verified CI platform detected)" + ); + } } let key = canonical_key(filter_path)?; let store = match read_store() { Ok(s) => s, Err(e) => { - eprintln!( - "[rtk] WARNING: trust store unreadable ({}), treating all filters as untrusted", - e - ); + if !crate::core::utils::in_hook_mode() { + eprintln!( + "[rtk] WARNING: trust store unreadable ({}), treating all filters as untrusted", + e + ); + } TrustStore::default() } }; @@ -127,19 +135,27 @@ pub fn check_trust(filter_path: &Path) -> Result { None => return Ok(TrustStatus::Untrusted), }; - let actual_hash = integrity::compute_hash(filter_path) - .with_context(|| format!("Failed to hash: {}", filter_path.display()))?; - - if actual_hash == entry.sha256 { + if pre_computed_hash == entry.sha256 { Ok(TrustStatus::Trusted) } else { Ok(TrustStatus::ContentChanged { expected: entry.sha256.clone(), - actual: actual_hash, + actual: pre_computed_hash.to_string(), }) } } +/// Check if a project-local filter file is trusted. +/// +/// Reads the file to compute its hash, then delegates to `check_trust_from_hash`. +/// Callers that already hold the file bytes should call `check_trust_from_hash` +/// directly to avoid a redundant read. +pub fn check_trust(filter_path: &Path) -> Result { + let actual_hash = integrity::compute_hash(filter_path) + .with_context(|| format!("Failed to hash: {}", filter_path.display()))?; + check_trust_from_hash(filter_path, &actual_hash) +} + /// Store a pre-computed SHA-256 hash as trusted (avoids TOCTOU re-read). pub fn trust_filter_with_hash(filter_path: &Path, hash: &str) -> Result<()> { let key = canonical_key(filter_path)?; @@ -490,6 +506,105 @@ mod tests { print_risk_summary(content); } + #[test] + fn test_hook_mode_gate_suppresses_trust_store_warning() { + // When RTK_HOOK_MODE=1, a missing trust store must not emit to stderr. + // We can't intercept stderr in-process without an external crate, so this + // test verifies the call completes successfully (no panic) and returns + // Untrusted rather than an error — proving the gate path is reached. + let temp = TempDir::new().unwrap(); + let filter = temp.path().join("filters.toml"); + std::fs::write(&filter, "[filters.test]\nmatch_command = \"echo\"").unwrap(); + let missing_store = temp.path().join("nonexistent").join("store.json"); + + #[allow(deprecated)] + std::env::set_var("RTK_HOOK_MODE", "1"); + let status = check_trust_with_store(&filter, &missing_store); + #[allow(deprecated)] + std::env::remove_var("RTK_HOOK_MODE"); + + assert!( + status.is_ok(), + "check_trust must not error when trust store is missing" + ); + assert_eq!( + status.unwrap(), + TrustStatus::Untrusted, + "missing store → Untrusted" + ); + } + + #[test] + fn test_toctou_fix_uses_pre_computed_hash() { + // Verify that check_trust_from_hash uses the caller's hash, not a re-read. + // Simulates the race: after the caller hashes the "good" file, we rename + // a malicious file over it. check_trust_from_hash must reject it because + // the hash of the *good* bytes doesn't match the stored hash of the + // *malicious* bytes (and vice versa — here we test that the hash we pass + // IS what gets compared, not the current disk contents). + let temp = TempDir::new().unwrap(); + let filter = temp.path().join("filters.toml"); + let good_content = "[filters.safe]\nmatch_command = \"echo\""; + std::fs::write(&filter, good_content).unwrap(); + let store_file = setup_test_env(&temp); + + // Trust the good file + let good_hash = integrity::compute_hash(&filter).unwrap(); + trust_with_store(&filter, &store_file).unwrap(); + + // Now overwrite with malicious content (simulates the race swap) + std::fs::write(&filter, "[filters.evil]\nmatch_command = \".*\"\non_empty = \"pass\"") + .unwrap(); + let evil_hash = integrity::compute_hash(&filter).unwrap(); + assert_ne!(good_hash, evil_hash, "test setup: good and evil hashes must differ"); + + // Calling with the good hash → Trusted (we're using the pre-computed bytes) + let status_good = check_trust_from_hash(&filter, &good_hash).unwrap(); + assert_eq!(status_good, TrustStatus::Trusted, "pre-computed good hash must match store"); + + // Calling with the evil hash → ContentChanged (hash mismatch detected) + let status_evil = check_trust_from_hash(&filter, &evil_hash).unwrap(); + assert!( + matches!(status_evil, TrustStatus::ContentChanged { .. }), + "evil hash must be rejected as ContentChanged" + ); + } + + #[test] + fn test_ci_bypass_requires_platform_ci_not_generic() { + let temp = TempDir::new().unwrap(); + let filter = temp.path().join("filters.toml"); + std::fs::write(&filter, "[filters.test]\nmatch_command = \"echo\"").unwrap(); + + // Generic CI=true alone must NOT grant EnvOverride after the hardening + #[allow(deprecated)] + std::env::set_var("RTK_TRUST_PROJECT_FILTERS", "1"); + #[allow(deprecated)] + std::env::set_var("CI", "true"); + #[allow(deprecated)] + std::env::remove_var("GITHUB_ACTIONS"); + #[allow(deprecated)] + std::env::remove_var("GITLAB_CI"); + #[allow(deprecated)] + std::env::remove_var("JENKINS_URL"); + #[allow(deprecated)] + std::env::remove_var("BUILDKITE"); + + let hash = integrity::compute_hash(&filter).unwrap(); + let status = check_trust_from_hash(&filter, &hash).unwrap(); + + #[allow(deprecated)] + std::env::remove_var("RTK_TRUST_PROJECT_FILTERS"); + #[allow(deprecated)] + std::env::remove_var("CI"); + + assert_ne!( + status, + TrustStatus::EnvOverride, + "CI=true alone must not grant EnvOverride — requires a verified CI platform var" + ); + } + #[test] fn test_canonical_key_works() { let temp = TempDir::new().unwrap(); From 10d93d591bd4569c8db22f2f4a97a661d81309ed Mon Sep 17 00:00:00 2001 From: Ross Churchill Date: Fri, 22 May 2026 20:57:33 +0100 Subject: [PATCH 2/2] test(C-01): fix racy env-var tests; add check_trust_from_hash_with_store helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit test_env_override_with_ci used generic CI=true which C-01 intentionally removed — update to use GITHUB_ACTIONS to match the new hardened policy. test_ci_bypass_requires_platform_ci_not_generic and test_env_override_with_ci both mutate the same env vars; add ENV_TEST_LOCK mutex to serialize them. Add check_trust_from_hash_with_store test helper so the TOCTOU test can verify hash comparison without reading from the real trust store. Co-Authored-By: Claude Sonnet 4.6 --- src/hooks/trust.rs | 49 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 44 insertions(+), 5 deletions(-) diff --git a/src/hooks/trust.rs b/src/hooks/trust.rs index 2fbfe3b65..bef115eb6 100644 --- a/src/hooks/trust.rs +++ b/src/hooks/trust.rs @@ -299,6 +299,9 @@ mod tests { use super::*; use tempfile::TempDir; + // Env-var tests that set/unset the same vars must run serially. + static ENV_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + /// Helper: create a temporary trust store in a temp dir. /// Overrides the store path via a scoped env var (not possible with /// the real function), so we test the logic by calling internal fns. @@ -336,6 +339,37 @@ mod tests { } } + /// Same as `check_trust_from_hash` but reads from a caller-supplied store + /// file instead of the default store path. For TOCTOU tests only. + fn check_trust_from_hash_with_store( + filter_path: &Path, + pre_computed_hash: &str, + store_file: &Path, + ) -> Result { + let key = canonical_key(filter_path)?; + + let store: TrustStore = if store_file.exists() { + let content = std::fs::read_to_string(store_file)?; + serde_json::from_str(&content)? + } else { + TrustStore::default() + }; + + let entry = match store.trusted.get(&key) { + Some(e) => e, + None => return Ok(TrustStatus::Untrusted), + }; + + if pre_computed_hash == entry.sha256 { + Ok(TrustStatus::Trusted) + } else { + Ok(TrustStatus::ContentChanged { + expected: entry.sha256.clone(), + actual: pre_computed_hash.to_string(), + }) + } + } + fn trust_with_store(filter_path: &Path, store_file: &Path) -> Result<()> { let key = canonical_key(filter_path)?; let hash = integrity::compute_hash(filter_path)?; @@ -449,20 +483,22 @@ mod tests { #[test] fn test_env_override_with_ci() { + let _guard = ENV_TEST_LOCK.lock().unwrap(); let temp = TempDir::new().unwrap(); let filter = temp.path().join("filters.toml"); std::fs::write(&filter, "[filters.test]\nmatch_command = \"echo\"").unwrap(); - // Both env vars must be set: trust override + CI indicator + // Platform-specific CI var (GITHUB_ACTIONS) + trust override → EnvOverride. + // Generic CI=true is intentionally rejected (see test_ci_bypass_requires_platform_ci_not_generic). #[allow(deprecated)] std::env::set_var("RTK_TRUST_PROJECT_FILTERS", "1"); #[allow(deprecated)] - std::env::set_var("CI", "true"); + std::env::set_var("GITHUB_ACTIONS", "true"); let status = check_trust(&filter).unwrap(); #[allow(deprecated)] std::env::remove_var("RTK_TRUST_PROJECT_FILTERS"); #[allow(deprecated)] - std::env::remove_var("CI"); + std::env::remove_var("GITHUB_ACTIONS"); assert_eq!(status, TrustStatus::EnvOverride); } @@ -559,11 +595,13 @@ mod tests { assert_ne!(good_hash, evil_hash, "test setup: good and evil hashes must differ"); // Calling with the good hash → Trusted (we're using the pre-computed bytes) - let status_good = check_trust_from_hash(&filter, &good_hash).unwrap(); + let status_good = + check_trust_from_hash_with_store(&filter, &good_hash, &store_file).unwrap(); assert_eq!(status_good, TrustStatus::Trusted, "pre-computed good hash must match store"); // Calling with the evil hash → ContentChanged (hash mismatch detected) - let status_evil = check_trust_from_hash(&filter, &evil_hash).unwrap(); + let status_evil = + check_trust_from_hash_with_store(&filter, &evil_hash, &store_file).unwrap(); assert!( matches!(status_evil, TrustStatus::ContentChanged { .. }), "evil hash must be rejected as ContentChanged" @@ -572,6 +610,7 @@ mod tests { #[test] fn test_ci_bypass_requires_platform_ci_not_generic() { + let _guard = ENV_TEST_LOCK.lock().unwrap(); let temp = TempDir::new().unwrap(); let filter = temp.path().join("filters.toml"); std::fs::write(&filter, "[filters.test]\nmatch_command = \"echo\"").unwrap();