diff --git a/src/core/runner.rs b/src/core/runner.rs index 3a42b89c4f..dfea513f49 100644 --- a/src/core/runner.rs +++ b/src/core/runner.rs @@ -15,6 +15,7 @@ use crate::core::truncate::{CAP_LIST, CAP_WARNINGS}; /// emitted so the caller tracks exactly that. pub fn emit_guarded(filtered: &str, hint: Option<&str>, raw: &str) -> String { let body = match hint { + Some(h) if filtered.is_empty() => h.to_string(), Some(h) => format!("{}\n{}", filtered, h), None => filtered.to_string(), }; @@ -1192,6 +1193,51 @@ mod forwarded_stderr_tests { } } +#[cfg(test)] +mod emit_guarded_tests { + use super::*; + + #[test] + fn test_emit_guarded_empty_filtered_with_hint_no_leading_blank() { + // Use a long raw matching the repro (600 newlines) so never_worse prefers the hint + let long_raw = "x".repeat(600); + let result = emit_guarded( + "", + Some("[full output: ~/.cache/rtk/tee/1_uv.log]"), + &long_raw, + ); + assert!( + !result.starts_with('\n'), + "should not start with blank line: {:?}", + result + ); + assert_eq!(result, "[full output: ~/.cache/rtk/tee/1_uv.log]"); + } + + #[test] + fn test_emit_guarded_non_empty_filtered_with_hint_has_separator() { + // Use a long raw matching the repro so never_worse prefers the filtered+body + let long_raw = "x".repeat(600); + let result = emit_guarded("body content", Some("[hint]"), &long_raw); + assert!( + result.starts_with("body content"), + "should start with body: {:?}", + result + ); + assert!( + result.contains("\n[hint]"), + "should have separator: {:?}", + result + ); + } + + #[test] + fn test_emit_guarded_no_hint_returns_filtered() { + let result = emit_guarded("body content", None, "raw raw raw"); + assert_eq!(result, "body content"); + } +} + #[cfg(test)] mod err_test_runner_tests { use super::*; diff --git a/src/discover/mod.rs b/src/discover/mod.rs index 859dbbcfa7..a5e6da4107 100644 --- a/src/discover/mod.rs +++ b/src/discover/mod.rs @@ -301,7 +301,7 @@ pub fn run( registry::normalize_transparent_prefixes(&transparent_prefixes); // Loaded once up front (see `PermissionRules`), not once per command. - let (deny, ask, allow) = permissions::load_rules_for(permissions::Host::Claude); + let (deny, ask, allow, _config_error) = permissions::load_rules_for(permissions::Host::Claude); let rules = PermissionRules { deny, ask, allow }; let cutoff = crate::core::utils::days_ago_cutoff(since_days); diff --git a/src/hooks/decision.rs b/src/hooks/decision.rs index 83d7fde24f..4d3994ef72 100644 --- a/src/hooks/decision.rs +++ b/src/hooks/decision.rs @@ -79,7 +79,8 @@ pub(crate) fn decide_with_params( excluded: &[String], transparent_prefixes: &[String], ) -> HookDecision { - if verdict == PermissionVerdict::Deny { + // ConfigError means a settings file exists but couldn't be read/parsed — fail closed. + if verdict == PermissionVerdict::ConfigError || verdict == PermissionVerdict::Deny { return HookDecision::Deny; } @@ -419,7 +420,8 @@ mod tests { check_command_for("git status", Host::Codex), PermissionVerdict::Default ); - let (deny, ask, allow) = super::super::permissions::load_rules_for(Host::Codex); + let (deny, ask, allow, _config_error) = + super::super::permissions::load_rules_for(Host::Codex); assert!(deny.is_empty() && ask.is_empty() && allow.is_empty()); } diff --git a/src/hooks/permissions.rs b/src/hooks/permissions.rs index 4bfde316ea..aba7e37d9f 100644 --- a/src/hooks/permissions.rs +++ b/src/hooks/permissions.rs @@ -19,6 +19,8 @@ pub enum PermissionVerdict { Ask, /// No rule matched — default to ask (matches Claude Code's least-privilege default). Default, + /// A settings file exists but could not be read or parsed — fail closed. + ConfigError, } /// Check `cmd` against Claude Code's deny/ask/allow permission rules. @@ -43,7 +45,10 @@ pub enum Host { } pub fn check_command_for(cmd: &str, host: Host) -> PermissionVerdict { - let (deny_rules, ask_rules, allow_rules) = load_rules_for(host); + let (deny_rules, ask_rules, allow_rules, config_error) = load_rules_for(host); + if config_error { + return PermissionVerdict::ConfigError; + } check_command_with_rules(cmd, &deny_rules, &ask_rules, &allow_rules) } @@ -53,17 +58,29 @@ pub fn check_command_for(cmd: &str, host: Host) -> PermissionVerdict { /// can load once up front and reuse `check_command_with_rules` per command instead /// of going through `check_command_for` and re-reading every settings file from /// disk on every single call. -pub(crate) fn load_rules_for(host: Host) -> (Vec, Vec, Vec) { +pub(crate) fn load_rules_for(host: Host) -> (Vec, Vec, Vec, bool) { match host { - Host::Claude => load_permission_rules(), - Host::Cursor => load_cursor_rules(), - Host::Gemini => load_gemini_rules(), - Host::Droid => load_droid_rules(), + Host::Claude => { + let (deny, ask, allow, config_error) = load_permission_rules(); + (deny, ask, allow, config_error) + } + Host::Cursor => { + let (deny, ask, allow) = load_cursor_rules(); + (deny, ask, allow, false) + } + Host::Gemini => { + let (deny, ask, allow) = load_gemini_rules(); + (deny, ask, allow, false) + } + Host::Droid => { + let (deny, ask, allow) = load_droid_rules(); + (deny, ask, allow, false) + } // Hosts with no RTK-side rule source. Codex enforces its native // execution rules after updatedInput. Do not interpret these hosts' // rules as Claude Bash patterns or borrow another host's settings. // No RTK-side match means Default, not an explicit Allow. - Host::Codex | Host::Trae | Host::Vibe => (Vec::new(), Vec::new(), Vec::new()), + Host::Codex | Host::Trae | Host::Vibe => (Vec::new(), Vec::new(), Vec::new(), false), } } @@ -146,22 +163,39 @@ pub(crate) fn check_command_with_rules( /// 3. `~/.claude/settings.json` /// 4. `~/.claude/settings.local.json` /// -/// Missing files and malformed JSON are silently skipped. -fn load_permission_rules() -> (Vec, Vec, Vec) { +/// If any settings file exists but cannot be read or parsed, returns config_error=true +/// to trigger fail-closed behavior (ConfigError verdict). +fn load_permission_rules() -> (Vec, Vec, Vec, bool) { let mut deny_rules = Vec::new(); let mut ask_rules = Vec::new(); let mut allow_rules = Vec::new(); + let mut config_error = false; for path in get_settings_paths() { - let Ok(content) = std::fs::read_to_string(&path) else { + if !path.exists() { continue; + } + let content = match std::fs::read_to_string(&path) { + Ok(c) => c, + Err(_) => { + eprintln!( + "[rtk] error: failed to read permissions from {}", + path.display() + ); + config_error = true; + continue; + } }; - let Ok(json) = crate::core::utils::from_json_str::(&content) else { - eprintln!( - "[rtk] warning: failed to parse permissions from {}", - path.display() - ); - continue; + let json = match crate::core::utils::from_json_str::(&content) { + Ok(j) => j, + Err(_) => { + eprintln!( + "[rtk] error: failed to parse permissions from {}", + path.display() + ); + config_error = true; + continue; + } }; let Some(permissions) = json.get("permissions") else { continue; @@ -172,7 +206,7 @@ fn load_permission_rules() -> (Vec, Vec, Vec) { append_bash_rules(permissions.get("allow"), &mut allow_rules); } - (deny_rules, ask_rules, allow_rules) + (deny_rules, ask_rules, allow_rules, config_error) } /// Extract Bash-scoped patterns from a JSON array and append them to `target`. diff --git a/src/hooks/rewrite_cmd.rs b/src/hooks/rewrite_cmd.rs index 9ed32383b3..c901985701 100644 --- a/src/hooks/rewrite_cmd.rs +++ b/src/hooks/rewrite_cmd.rs @@ -211,10 +211,12 @@ mod tests { /// Ask → 3 (process::exit(3)) /// Default→ 3 (process::exit(3)) — grouped with Ask /// Deny → 2 (process::exit(2)) — handled before rewrite match + /// ConfigError → 2 (fail closed, same as Deny) fn expected_exit_code(verdict: &PermissionVerdict) -> i32 { match verdict { PermissionVerdict::Allow => 0, PermissionVerdict::Deny => 2, + PermissionVerdict::ConfigError => 2, PermissionVerdict::Ask => 3, PermissionVerdict::Default => 3, // MUST be 3, not 0! } @@ -256,6 +258,14 @@ mod tests { assert_eq!(expected_exit_code(&verdict), 2); } + #[test] + fn test_config_error_verdict_maps_to_deny_exit_code() { + // ConfigError (unreadable/unparseable settings file) maps to Deny (exit 2) — fail closed. + // This is tested indirectly by ensuring the enum variant exists and maps to 2. + // A direct test would require triggering a config error, which requires file I/O. + assert_eq!(expected_exit_code(&PermissionVerdict::ConfigError), 2); + } + #[test] fn test_no_auto_allow_bypass_for_unrecognized_commands() { // SECURITY: A command with no permission rules and no matching allow rule