From e27b47d1c2697be74a421c3ab54276c2536718f4 Mon Sep 17 00:00:00 2001 From: mrhard9090 <74858909+mrhard9090@users.noreply.github.com> Date: Sat, 26 Sep 2026 22:34:18 +0300 Subject: [PATCH 1/2] fix(permissions): fail closed when a settings file cannot be read or parsed A Claude Code settings file that exists but cannot be read or parsed adds a catch-all Ask rule and a warning instead of being skipped. Refs #4282. --- src/hooks/permissions.rs | 121 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 117 insertions(+), 4 deletions(-) diff --git a/src/hooks/permissions.rs b/src/hooks/permissions.rs index 840d871932..38abed0ef9 100644 --- a/src/hooks/permissions.rs +++ b/src/hooks/permissions.rs @@ -146,21 +146,41 @@ pub(crate) fn check_command_with_rules( /// 3. `~/.claude/settings.json` /// 4. `~/.claude/settings.local.json` /// -/// Missing files and malformed JSON are silently skipped. +/// Missing and empty files are skipped. A file that exists but cannot be read +/// or parsed may hold deny rules that can no longer be applied, so it fails +/// closed: a warning is printed and every command gets at least `Ask`. fn load_permission_rules() -> (Vec, Vec, Vec) { + load_permission_rules_from(&get_settings_paths()) +} + +fn load_permission_rules_from(paths: &[PathBuf]) -> (Vec, Vec, Vec) { let mut deny_rules = Vec::new(); let mut ask_rules = Vec::new(); let mut allow_rules = Vec::new(); - for path in get_settings_paths() { - let Ok(content) = std::fs::read_to_string(&path) else { - continue; + for path in paths { + let content = match std::fs::read_to_string(path) { + Ok(content) => content, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => continue, + Err(e) => { + eprintln!( + "[rtk] warning: failed to read permissions from {}: {}", + path.display(), + e + ); + ask_rules.push("*".to_string()); + continue; + } }; + if content.trim().is_empty() { + continue; + } let Ok(json) = crate::core::utils::from_json_str::(&content) else { eprintln!( "[rtk] warning: failed to parse permissions from {}", path.display() ); + ask_rules.push("*".to_string()); continue; }; let Some(permissions) = json.get("permissions") else { @@ -545,6 +565,99 @@ mod tests { ); } + fn rules_from_files(files: &[(&str, Option<&str>)]) -> (Vec, Vec, Vec) { + let tmp = tempfile::tempdir().unwrap(); + let paths: Vec = files + .iter() + .map(|(name, content)| { + let path = tmp.path().join(name); + match content { + Some(content) => std::fs::write(&path, content).unwrap(), + // A directory in place of the file: it exists but cannot be read. + None => std::fs::create_dir(&path).unwrap(), + } + path + }) + .collect(); + load_permission_rules_from(&paths) + } + + #[test] + fn test_valid_settings_file_rules_apply() { + let (deny, ask, allow) = rules_from_files(&[( + "settings.json", + Some(r#"{"permissions":{"deny":["Bash(git push *)"]}}"#), + )]); + assert_eq!( + check_command_with_rules("git push origin main", &deny, &ask, &allow), + PermissionVerdict::Deny + ); + assert_eq!( + check_command_with_rules("git status", &deny, &ask, &allow), + PermissionVerdict::Default + ); + } + + #[test] + fn test_unparseable_settings_file_fails_closed_to_ask() { + let (deny, ask, allow) = rules_from_files(&[ + ( + "settings.json", + Some(r#"{"permissions":{"deny":["Bash(git push *)"],}}"#), + ), + ( + "settings.local.json", + Some(r#"{"permissions":{"allow":["Bash(git status)"]}}"#), + ), + ]); + assert_eq!( + check_command_with_rules("git push origin main", &deny, &ask, &allow), + PermissionVerdict::Ask + ); + assert_eq!( + check_command_with_rules("git status", &deny, &ask, &allow), + PermissionVerdict::Ask + ); + } + + #[test] + fn test_unreadable_settings_file_fails_closed_to_ask() { + let (deny, ask, allow) = rules_from_files(&[("settings.json", None)]); + assert_eq!( + check_command_with_rules("git push origin main", &deny, &ask, &allow), + PermissionVerdict::Ask + ); + } + + #[test] + fn test_unreadable_settings_file_keeps_other_files_deny_rules() { + let (deny, ask, allow) = rules_from_files(&[ + ("settings.json", None), + ( + "settings.local.json", + Some(r#"{"permissions":{"deny":["Bash(rm *)"]}}"#), + ), + ]); + assert_eq!( + check_command_with_rules("rm -rf build", &deny, &ask, &allow), + PermissionVerdict::Deny + ); + } + + #[test] + fn test_missing_and_empty_settings_files_add_no_rules() { + let tmp = tempfile::tempdir().unwrap(); + let empty = tmp.path().join("settings.local.json"); + std::fs::write( + &empty, " +", + ) + .unwrap(); + let (deny, ask, allow) = + load_permission_rules_from(&[tmp.path().join("settings.json"), empty]); + assert!(deny.is_empty() && ask.is_empty() && allow.is_empty()); + } + #[test] fn test_parse_bash_pattern() { assert_eq!( From cd9dff3c925ca2787efe33a01333f30cb5666778 Mon Sep 17 00:00:00 2001 From: Nicolas Le Cam Date: Mon, 19 Oct 2026 00:50:46 +0200 Subject: [PATCH 2/2] test(permissions): pin the load-failure warning and deny rules past a parse failure The read-failure warning and the delegate exit code are checked end to end through `rtk rewrite` with `RTK_REWRITE_HOST=openclaw`, for a settings file that cannot be read and one that cannot be parsed. A unit test checks that deny rules from the other settings files still apply after an unparseable one, as they already did after an unreadable one. The new test helpers say why they would panic. Co-Authored-By: Claude Opus 5.5 --- src/hooks/permissions.rs | 32 ++++++++++++++--- tests/hook_decision_protocol_test.rs | 51 ++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 5 deletions(-) diff --git a/src/hooks/permissions.rs b/src/hooks/permissions.rs index 38abed0ef9..5af19c570f 100644 --- a/src/hooks/permissions.rs +++ b/src/hooks/permissions.rs @@ -566,15 +566,15 @@ mod tests { } fn rules_from_files(files: &[(&str, Option<&str>)]) -> (Vec, Vec, Vec) { - let tmp = tempfile::tempdir().unwrap(); + let tmp = tempfile::tempdir().expect("tempdir"); let paths: Vec = files .iter() .map(|(name, content)| { let path = tmp.path().join(name); match content { - Some(content) => std::fs::write(&path, content).unwrap(), + Some(content) => std::fs::write(&path, content).expect("write settings"), // A directory in place of the file: it exists but cannot be read. - None => std::fs::create_dir(&path).unwrap(), + None => std::fs::create_dir(&path).expect("mkdir settings"), } path }) @@ -644,15 +644,37 @@ mod tests { ); } + #[test] + fn test_unparseable_settings_file_keeps_other_files_deny_rules() { + let (deny, ask, allow) = rules_from_files(&[ + ( + "settings.json", + Some(r#"{"permissions":{"deny":["Bash(git push *)"],}}"#), + ), + ( + "settings.local.json", + Some(r#"{"permissions":{"deny":["Bash(rm *)"]}}"#), + ), + ]); + assert_eq!( + check_command_with_rules("rm -rf build", &deny, &ask, &allow), + PermissionVerdict::Deny + ); + assert_eq!( + check_command_with_rules("git push origin main", &deny, &ask, &allow), + PermissionVerdict::Ask + ); + } + #[test] fn test_missing_and_empty_settings_files_add_no_rules() { - let tmp = tempfile::tempdir().unwrap(); + let tmp = tempfile::tempdir().expect("tempdir"); let empty = tmp.path().join("settings.local.json"); std::fs::write( &empty, " ", ) - .unwrap(); + .expect("write settings"); let (deny, ask, allow) = load_permission_rules_from(&[tmp.path().join("settings.json"), empty]); assert!(deny.is_empty() && ask.is_empty() && allow.is_empty()); diff --git a/tests/hook_decision_protocol_test.rs b/tests/hook_decision_protocol_test.rs index 935d2beed4..e50dfbc73b 100644 --- a/tests/hook_decision_protocol_test.rs +++ b/tests/hook_decision_protocol_test.rs @@ -701,6 +701,57 @@ mod rewrite_host_scoping { ); } + /// A settings file that exists but cannot be read or parsed may hold deny + /// rules RTK can no longer apply, so it asks: `git push origin main`, which + /// the unparseable file denies, exits 3 in both cases even for a delegate + /// that relaxes the default ask, with a warning naming the file. + #[test] + fn openclaw_asks_when_a_settings_file_cannot_be_read_or_parsed() { + let unreadable = Sandbox::bare(); + let settings = unreadable.project.join(".claude/settings.json"); + std::fs::remove_file(&settings).expect("remove settings"); + std::fs::create_dir(&settings).expect("a directory in place of settings.json"); + + let unparseable = Sandbox::bare(); + std::fs::write( + unparseable.project.join(".claude/settings.json"), + r#"{"permissions":{"deny":["Bash(git push *)"],}}"#, + ) + .expect("write settings"); + + let file = std::path::Path::new("project") + .join(".claude") + .join("settings.json") + .display() + .to_string(); + for (sandbox, warning) in [ + ( + &unreadable, + "[rtk] warning: failed to read permissions from ", + ), + ( + &unparseable, + "[rtk] warning: failed to parse permissions from ", + ), + ] { + let (code, stdout, stderr) = sandbox.run_with_env( + &["rewrite", "git push origin main"], + &[("RTK_REWRITE_HOST", "openclaw")], + ); + assert_eq!( + (code, stdout.as_str()), + (3, "rtk git push origin main"), + "{warning}: {stderr}" + ); + assert!( + stderr + .lines() + .any(|line| line.starts_with(warning) && line.contains(&file)), + "{warning}: {stderr}" + ); + } + } + /// The suite is hermetic against the variable: `run_with_env` scrubs it, so /// exporting `RTK_REWRITE_HOST=openclaw` in a developer's shell cannot turn /// `default_verdict_exits_three_never_zero` red on that machine and green