Fix untrusted project hooks in read-only mode

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<String> = 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(
+            let ParsedConfigFile { mut object, source } = parsed;
-                &parsed.object,
+            if let Some(warning) =
-                &parsed.source,
+                strip_untrusted_project_hooks(entry.source, &mut object, &entry.path)
-                &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);
+            all_warnings.extend(
-            validate_optional_hooks_config(&parsed.object, &entry.path)?;
+                validation
-            merge_mcp_servers(&mut mcp, entry.source, &parsed.object, &entry.path)?;
+                    .warnings
-            deep_merge_objects(&mut merged, &parsed.object);
+                    .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(
+            let ParsedConfigFile { mut object, source } = parsed;
-                &parsed.object,
+            if let Some(warning) =
-                &parsed.source,
+                strip_untrusted_project_hooks(entry.source, &mut object, &entry.path)
-                &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)?;
+            validate_optional_hooks_config(&object, &entry.path)?;
-            merge_mcp_servers(&mut mcp, entry.source, &parsed.object, &entry.path)?;
+            merge_mcp_servers(&mut mcp, entry.source, &object, &entry.path)?;
-            deep_merge_objects(&mut merged, &parsed.object);
+            deep_merge_objects(&mut merged, &object);
             loaded_entries.push(entry);
         }
 
@@ -589,11 +600,14 @@ impl ConfigLoader {
                 }
             };
 
-            let validation = crate::config_validate::validate_config_file(
+            let ParsedConfigFile { mut object, source } = parsed;
-                &parsed.object,
+            if let Some(warning) =
-                &parsed.source,
+                strip_untrusted_project_hooks(entry.source, &mut object, &entry.path)
-                &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) =
+            if let Err(error) = merge_mcp_servers(&mut mcp, entry.source, &object, &entry.path) {
-                merge_mcp_servers(&mut mcp, entry.source, &parsed.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);
+            let key_paths = collect_config_key_paths(&object);
-            deep_merge_objects(&mut merged, &parsed.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<String, JsonValue>,
+    path: &Path,
+) -> Option<String> {
+    // 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<String>) -> 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!(loaded.hooks().post_tool_use().is_empty());
-        assert_eq!(
+        assert!(loaded.hooks().post_tool_use_failure().is_empty());
-            loaded.hooks().post_tool_use_failure(),
-            &["project-failure".to_string()]
-        );
         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()]);
-        assert_eq!(
+        assert!(loaded.hooks().invalid_hooks().is_empty());
-            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"));
 
         fs::remove_dir_all(root).expect("cleanup temp dir");
     }

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<Vec<AssistantEvent>, 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());