From 09731036ac048dd01480e3a535db51f00981d160 Mon Sep 17 00:00:00 2001 From: gaebal-gajae Date: Sun, 11 Oct 2026 20:12:25 +0000 Subject: [PATCH] Fix untrusted project hooks in read-only mode --- rust/crates/runtime/src/config.rs | 186 +++++++++++++++++------- rust/crates/runtime/src/conversation.rs | 85 ++++++++++- 2 files changed, 216 insertions(+), 55 deletions(-) diff --git a/rust/crates/runtime/src/config.rs b/rust/crates/runtime/src/config.rs index 95057563..964e1a34 100644 --- a/rust/crates/runtime/src/config.rs +++ b/rust/crates/runtime/src/config.rs @@ -466,31 +466,39 @@ impl ConfigLoader { let mut merged = BTreeMap::new(); let mut loaded_entries = Vec::new(); let mut mcp = McpConfigCollection::default(); - let mut all_warnings = Vec::new(); + let mut all_warnings: Vec = Vec::new(); for entry in self.discover() { crate::config_validate::check_unsupported_format(&entry.path)?; let OptionalConfigFile::Loaded(parsed) = read_optional_json_object(&entry.path)? else { continue; }; - let validation = crate::config_validate::validate_config_file( - &parsed.object, - &parsed.source, - &entry.path, - ); + let ParsedConfigFile { mut object, source } = parsed; + if let Some(warning) = + strip_untrusted_project_hooks(entry.source, &mut object, &entry.path) + { + all_warnings.push(warning); + } + let validation = + crate::config_validate::validate_config_file(&object, &source, &entry.path); if !validation.is_ok() { let first_error = &validation.errors[0]; return Err(ConfigError::Parse(first_error.to_string())); } - all_warnings.extend(validation.warnings); - validate_optional_hooks_config(&parsed.object, &entry.path)?; - merge_mcp_servers(&mut mcp, entry.source, &parsed.object, &entry.path)?; - deep_merge_objects(&mut merged, &parsed.object); + all_warnings.extend( + validation + .warnings + .into_iter() + .map(|warning| warning.to_string()), + ); + validate_optional_hooks_config(&object, &entry.path)?; + merge_mcp_servers(&mut mcp, entry.source, &object, &entry.path)?; + deep_merge_objects(&mut merged, &object); loaded_entries.push(entry); } for warning in &all_warnings { - emit_config_warning_once(&warning.to_string()); + emit_config_warning_once(warning); } build_runtime_config(merged, loaded_entries, mcp) @@ -512,19 +520,22 @@ impl ConfigLoader { let OptionalConfigFile::Loaded(parsed) = read_optional_json_object(&entry.path)? else { continue; }; - let validation = crate::config_validate::validate_config_file( - &parsed.object, - &parsed.source, - &entry.path, - ); + let ParsedConfigFile { mut object, source } = parsed; + if let Some(warning) = + strip_untrusted_project_hooks(entry.source, &mut object, &entry.path) + { + all_warnings.push(warning); + } + let validation = + crate::config_validate::validate_config_file(&object, &source, &entry.path); if !validation.is_ok() { let first_error = &validation.errors[0]; return Err(ConfigError::Parse(first_error.to_string())); } all_warnings.extend(validation.warnings.iter().map(|w| w.to_string())); - validate_optional_hooks_config(&parsed.object, &entry.path)?; - merge_mcp_servers(&mut mcp, entry.source, &parsed.object, &entry.path)?; - deep_merge_objects(&mut merged, &parsed.object); + validate_optional_hooks_config(&object, &entry.path)?; + merge_mcp_servers(&mut mcp, entry.source, &object, &entry.path)?; + deep_merge_objects(&mut merged, &object); loaded_entries.push(entry); } @@ -589,11 +600,14 @@ impl ConfigLoader { } }; - let validation = crate::config_validate::validate_config_file( - &parsed.object, - &parsed.source, - &entry.path, - ); + let ParsedConfigFile { mut object, source } = parsed; + if let Some(warning) = + strip_untrusted_project_hooks(entry.source, &mut object, &entry.path) + { + warnings.push(warning); + } + let validation = + crate::config_validate::validate_config_file(&object, &source, &entry.path); if !validation.is_ok() { let detail = validation.errors[0].to_string(); load_error.get_or_insert_with(|| detail.clone()); @@ -612,7 +626,7 @@ impl ConfigLoader { .map(|warning| warning.to_string()), ); - if let Err(error) = validate_optional_hooks_config(&parsed.object, &entry.path) { + if let Err(error) = validate_optional_hooks_config(&object, &entry.path) { let detail = error.to_string(); load_error.get_or_insert_with(|| detail.clone()); files.push(ConfigFileReport::load_error( @@ -624,9 +638,7 @@ impl ConfigLoader { continue; } - if let Err(error) = - merge_mcp_servers(&mut mcp, entry.source, &parsed.object, &entry.path) - { + if let Err(error) = merge_mcp_servers(&mut mcp, entry.source, &object, &entry.path) { let detail = error.to_string(); load_error.get_or_insert_with(|| detail.clone()); files.push(ConfigFileReport::load_error( @@ -638,8 +650,8 @@ impl ConfigLoader { continue; } - let key_paths = collect_config_key_paths(&parsed.object); - deep_merge_objects(&mut merged, &parsed.object); + let key_paths = collect_config_key_paths(&object); + deep_merge_objects(&mut merged, &object); loaded_entries.push(entry.clone()); files.push(ConfigFileReport::loaded(entry, precedence_rank, key_paths)); } @@ -663,6 +675,23 @@ impl ConfigLoader { } } +fn strip_untrusted_project_hooks( + source: ConfigSource, + object: &mut BTreeMap, + path: &Path, +) -> Option { + // Project settings come from the repository and there is no project-hook + // trust approval flow yet. User and local settings remain eligible. + if source == ConfigSource::Project && object.remove("hooks").is_some() { + Some(format!( + "Ignoring project hooks in {} because project-hook trust approval is not supported.", + path.display() + )) + } else { + None + } +} + impl ConfigFileReport { fn loaded(entry: ConfigEntry, precedence_rank: usize, key_paths: Vec) -> Self { Self { @@ -2590,6 +2619,7 @@ mod tests { }; use crate::json::JsonValue; use crate::sandbox::FilesystemIsolationMode; + use crate::HookRunner; use std::fs; use std::time::{SystemTime, UNIX_EPOCH}; @@ -2695,17 +2725,14 @@ mod tests { .and_then(JsonValue::as_object) .expect("hooks object") .contains_key("PreToolUse")); - assert!(loaded + assert!(!loaded .get("hooks") .and_then(JsonValue::as_object) .expect("hooks object") .contains_key("PostToolUse")); assert_eq!(loaded.hooks().pre_tool_use(), &["base".to_string()]); - assert_eq!(loaded.hooks().post_tool_use(), &["project".to_string()]); - assert_eq!( - loaded.hooks().post_tool_use_failure(), - &["project-failure".to_string()] - ); + assert!(loaded.hooks().post_tool_use().is_empty()); + assert!(loaded.hooks().post_tool_use_failure().is_empty()); assert_eq!(loaded.permission_rules().allow(), &["Read".to_string()]); assert_eq!( loaded.permission_rules().deny(), @@ -2718,6 +2745,73 @@ mod tests { fs::remove_dir_all(root).expect("cleanup temp dir"); } + #[test] + fn project_config_hooks_cannot_execute_commands() { + let root = temp_dir(); + let cwd = root.join("project"); + let home = root.join("home").join(".claw"); + let project_settings = cwd.join(".claw").join("settings.json"); + let project_legacy_settings = cwd.join(".claw.json"); + let marker = root.join("project-hook-executed"); + fs::create_dir_all(project_settings.parent().expect("project config dir")) + .expect("create project config dir"); + fs::create_dir_all(&home).expect("home config dir"); + let command = format!("echo compromised >> {}", marker.display()); + fs::write( + &project_legacy_settings, + format!(r#"{{"hooks":{{"PreToolUse":["{command}"]}}}}"#), + ) + .expect("write untrusted legacy project hook"); + fs::write( + &project_settings, + format!(r#"{{"hooks":{{"PreToolUse":["{command}"]}}}}"#), + ) + .expect("write untrusted project hook"); + + let loader = ConfigLoader::new(&cwd, &home); + let loaded = loader.load().expect("config should load"); + let _ = + HookRunner::from_feature_config(loaded.feature_config()).run_pre_tool_use("Read", "{}"); + + let hook_executed = marker.exists(); + if hook_executed { + fs::remove_file(&marker).expect("remove hook marker"); + } + assert!( + !hook_executed, + "untrusted project hook executed arbitrary shell command" + ); + + let (collected, warnings) = loader + .load_collecting_warnings() + .expect("collecting config should load"); + assert!(collected.hooks().pre_tool_use().is_empty()); + assert_eq!( + warnings + .iter() + .filter(|warning| warning.contains("Ignoring project hooks")) + .count(), + 2 + ); + let inspected = loader.inspect_collecting_warnings(); + assert_eq!( + inspected + .warnings + .iter() + .filter(|warning| warning.contains("Ignoring project hooks")) + .count(), + 2 + ); + assert!(inspected + .runtime_config + .expect("inspection should produce runtime config") + .hooks() + .pre_tool_use() + .is_empty()); + + fs::remove_dir_all(root).expect("cleanup temp dir"); + } + #[test] fn parses_object_style_hook_entries_with_matchers() { let root = temp_dir(); @@ -3523,7 +3617,7 @@ mod tests { } #[test] - fn loads_valid_hook_entries_and_records_invalid_siblings_441() { + fn ignores_project_hook_entries_and_keeps_user_hooks_441() { // given let root = temp_dir(); let cwd = root.join("project"); @@ -3547,22 +3641,8 @@ mod tests { .load() .expect("config should load valid hook entries and record invalid siblings"); - // #106: arrays now concatenate across config layers, so both "base" and "project" are present - assert_eq!( - loaded.hooks().pre_tool_use(), - &["base".to_string(), "project".to_string()] - ); - assert_eq!(loaded.hooks().invalid_count(), 1); - assert_eq!(loaded.hooks().invalid_hooks()[0].event, "PreToolUse"); - assert_eq!( - loaded.hooks().invalid_hooks()[0].kind, - "invalid_hooks_config" - ); - // #106: invalid entry at index 2 after array concatenation - assert_eq!(loaded.hooks().invalid_hooks()[0].index, Some(2)); - assert!(loaded.hooks().invalid_hooks()[0] - .reason - .contains("must be a string or hook object")); + assert_eq!(loaded.hooks().pre_tool_use(), &["base".to_string()]); + assert!(loaded.hooks().invalid_hooks().is_empty()); fs::remove_dir_all(root).expect("cleanup temp dir"); } diff --git a/rust/crates/runtime/src/conversation.rs b/rust/crates/runtime/src/conversation.rs index 9c36329a..5a0ebc72 100644 --- a/rust/crates/runtime/src/conversation.rs +++ b/rust/crates/runtime/src/conversation.rs @@ -10,7 +10,7 @@ use crate::compact::{ use crate::config::RuntimeFeatureConfig; use crate::hooks::{HookAbortSignal, HookProgressReporter, HookRunResult, HookRunner}; use crate::permissions::{ - PermissionContext, PermissionOutcome, PermissionPolicy, PermissionPrompter, + PermissionContext, PermissionMode, PermissionOutcome, PermissionPolicy, PermissionPrompter, }; use crate::session::{ContentBlock, ConversationMessage, Session}; use crate::usage::{TokenUsage, UsageTracker}; @@ -176,6 +176,11 @@ where feature_config: &RuntimeFeatureConfig, ) -> Self { let usage_tracker = UsageTracker::from_session(&session); + let hook_runner = if permission_policy.active_mode() == PermissionMode::ReadOnly { + HookRunner::default() + } else { + HookRunner::from_feature_config(feature_config) + }; Self { session, api_client, @@ -184,7 +189,7 @@ where system_prompt, max_iterations: usize::MAX, usage_tracker, - hook_runner: HookRunner::from_feature_config(feature_config), + hook_runner, auto_compaction_input_tokens_threshold: auto_compaction_threshold_from_env(), hook_abort_signal: HookAbortSignal::default(), hook_progress_reporter: None, @@ -991,6 +996,82 @@ mod tests { )); } + #[test] + fn read_only_mode_does_not_execute_hooks() { + struct SingleCallApiClient { + calls: usize, + } + + impl ApiClient for SingleCallApiClient { + fn stream( + &mut self, + _request: ApiRequest, + ) -> Result, RuntimeError> { + self.calls += 1; + if self.calls == 1 { + Ok(vec![ + AssistantEvent::ToolUse { + id: "tool-1".to_string(), + name: "Read".to_string(), + input: "{}".to_string(), + }, + AssistantEvent::ToolUse { + id: "tool-2".to_string(), + name: "ReadError".to_string(), + input: "{}".to_string(), + }, + AssistantEvent::MessageStop, + ]) + } else { + Ok(vec![ + AssistantEvent::TextDelta("done".to_string()), + AssistantEvent::MessageStop, + ]) + } + } + } + + let nanos = SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("system time should be after epoch") + .as_nanos(); + let marker = std::env::temp_dir().join(format!( + "claw-read-only-hook-{}-{nanos}", + std::process::id() + )); + let hook_command = format!("echo compromised >> \"{}\"", marker.display()); + let features = RuntimeFeatureConfig::default().with_hooks(RuntimeHookConfig::new( + vec![shell_snippet(&hook_command)], + vec![shell_snippet(&hook_command)], + vec![shell_snippet(&hook_command)], + )); + let mut runtime = ConversationRuntime::new_with_features( + Session::new(), + SingleCallApiClient { calls: 0 }, + StaticToolExecutor::new() + .register("Read", |_input| Ok("read result".to_string())) + .register("ReadError", |_input| Err(ToolError::new("read failed"))), + PermissionPolicy::new(PermissionMode::ReadOnly) + .with_tool_requirement("Read", PermissionMode::ReadOnly) + .with_tool_requirement("ReadError", PermissionMode::ReadOnly), + vec!["system".to_string()], + &features, + ); + + runtime + .run_turn("read a file", None) + .expect("read-only tool call should succeed"); + + let hook_executed = marker.exists(); + if hook_executed { + fs::remove_file(&marker).expect("remove hook marker"); + } + assert!( + !hook_executed, + "read-only mode must not execute pre-, post-, or failure hooks" + ); + } + #[test] fn records_runtime_session_trace_events() { let sink = Arc::new(MemoryTelemetrySink::default());