diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 194660d536..7272adf2fa 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3712,6 +3712,7 @@ dependencies = [ "schemars 0.8.22", "serde", "serde_json", + "sha2 0.10.9", "tempfile", "tokio", "tracing", diff --git a/codex-rs/app-server/src/config_manager_service.rs b/codex-rs/app-server/src/config_manager_service.rs index f7377c5b76..df807ba4b5 100644 --- a/codex-rs/app-server/src/config_manager_service.rs +++ b/codex-rs/app-server/src/config_manager_service.rs @@ -24,6 +24,7 @@ use codex_config::merge_toml_values; use codex_config::shell_environment_filter_entry; use codex_config::validate_shell_environment_policy_filter_config; use codex_core::config::deserialize_config_toml_with_base; +use codex_core::config::edit::ConfigEdit; use codex_core::config::edit::ConfigEditsBuilder; use codex_core::config::validate_feature_requirements_for_config_toml; use codex_core::path_utils; @@ -360,6 +361,38 @@ impl ConfigManager { } config_edits.extend(credential_provider_edits.remapping_edits(&user_config)?); + // hooks/list returns an opaque declaration/content approval value. Split it + // on persistence so older clients still recognize the declaration approval. + if let Some(states) = user_config + .get_mut("hooks") + .and_then(|hooks| hooks.get_mut("state")) + .and_then(TomlValue::as_table_mut) + { + for (key, state) in states { + let Some(state) = state.as_table_mut() else { + continue; + }; + let Some((declaration, content)) = state + .get("trusted_hash") + .and_then(TomlValue::as_str) + .and_then(|hash| hash.split_once('/')) + else { + continue; + }; + let hashes = [ + ("trusted_hash", declaration.to_owned()), + ("trusted_content_hash", content.to_owned()), + ]; + for (field, hash) in hashes { + state.insert(field.to_string(), TomlValue::String(hash.clone())); + config_edits.push(ConfigEdit::SetPath { + segments: vec!["hooks".into(), "state".into(), key.clone(), field.into()], + value: toml_edit::value(hash), + }); + } + } + } + validate_config(&user_config).map_err(|err| { ConfigManagerError::write( ConfigWriteErrorCode::ConfigValidationError, diff --git a/codex-rs/app-server/src/effective_plugin_change.rs b/codex-rs/app-server/src/effective_plugin_change.rs index 75638b4192..8c07c55dd7 100644 --- a/codex-rs/app-server/src/effective_plugin_change.rs +++ b/codex-rs/app-server/src/effective_plugin_change.rs @@ -119,14 +119,17 @@ pub(crate) async fn trust_materialized_plugin_hooks( .plugins_manager() .plugins_for_config(&config.plugins_config_input()) .await; - let hooks = codex_hooks::list_hooks(codex_hooks::HooksConfig { + let hooks_config = codex_hooks::HooksConfig { feature_enabled: true, bypass_hook_trust: config.bypass_hook_trust, config_layer_stack: Some(config.config_layer_stack), plugin_hook_sources: plugin_outcome.effective_plugin_hook_sources(), plugin_hook_load_warnings: plugin_outcome.effective_plugin_hook_warnings(), ..Default::default() - }); + }; + let hooks = tokio::task::spawn_blocking(move || codex_hooks::list_hooks(hooks_config)) + .await + .map_err(|err| format!("hook discovery task failed: {err}"))?; if !hooks.warnings.is_empty() { warn!( warnings = ?hooks.warnings, diff --git a/codex-rs/app-server/src/request_processors/catalog_processor.rs b/codex-rs/app-server/src/request_processors/catalog_processor.rs index efbed26c5c..020b1cb762 100644 --- a/codex-rs/app-server/src/request_processors/catalog_processor.rs +++ b/codex-rs/app-server/src/request_processors/catalog_processor.rs @@ -671,14 +671,17 @@ impl CatalogRequestProcessor { } else { codex_core_plugins::PluginHookLoadOutcome::default() }; - let hooks = codex_hooks::list_hooks(codex_hooks::HooksConfig { + let hooks_config = codex_hooks::HooksConfig { feature_enabled: hooks_enabled, bypass_hook_trust: config.bypass_hook_trust, config_layer_stack: Some(config.config_layer_stack), plugin_hook_sources: plugin_hooks.hook_sources, plugin_hook_load_warnings: plugin_hooks.hook_load_warnings, ..Default::default() - }); + }; + let hooks = tokio::task::spawn_blocking(move || codex_hooks::list_hooks(hooks_config)) + .await + .map_err(|err| internal_error(format!("hook discovery task failed: {err}")))?; data.push(codex_app_server_protocol::HooksListEntry { cwd, hooks: hooks_to_info(&hooks.hooks), diff --git a/codex-rs/app-server/tests/suite/v2/hooks_list.rs b/codex-rs/app-server/tests/suite/v2/hooks_list.rs index beebe306a1..6462f32d88 100644 --- a/codex-rs/app-server/tests/suite/v2/hooks_list.rs +++ b/codex-rs/app-server/tests/suite/v2/hooks_list.rs @@ -289,6 +289,7 @@ async fn hooks_list_shows_discovered_plugin_hook() -> Result<()> { .path() .join("plugins/cache/test/demo/local/hooks/hooks.json"), )?)?; + let current_hash = data[0].hooks[0].current_hash.clone(); assert_eq!( data, vec![HooksListEntry { @@ -310,15 +311,7 @@ async fn hooks_list_shows_discovered_plugin_hook() -> Result<()> { display_order: 0, enabled: true, is_managed: false, - current_hash: command_hook_hash( - "pre_tool_use", - Some("Bash"), - "echo plugin hook", - /*timeout_sec*/ 7, - /*async*/ false, - Some("running plugin hook"), - /*additional_context_limit*/ None, - ), + current_hash, trust_status: HookTrustStatus::Untrusted, }], warnings: Vec::new(), @@ -419,6 +412,11 @@ async fn plugin_upgrade_refreshes_hook_runtime_for_loaded_session() -> Result<() let thread_start_id = mcp .send_thread_start_request_with_auto_env(ThreadStartParams { model: Some("mock-model".to_string()), + // Keep this refresh/disablement test independent of approval changes. + config: Some(std::collections::HashMap::from([( + "bypass_hook_trust".to_string(), + serde_json::json!(true), + )])), ..Default::default() }) .await?; @@ -559,7 +557,9 @@ async fn loaded_session_refreshes_externally_updated_plugin_hooks() -> Result<() let server = create_mock_responses_server_sequence_unchecked(vec![ create_final_assistant_message_sse_response("Version 1")?, + create_final_assistant_message_sse_response("Version 2 needs approval")?, create_final_assistant_message_sse_response("Version 2")?, + create_final_assistant_message_sse_response("Rollback needs approval")?, create_final_assistant_message_sse_response("Rollback")?, ]) .await; @@ -622,7 +622,13 @@ async fn loaded_session_refreshes_externally_updated_plugin_hooks() -> Result<() timeout(DEFAULT_TIMEOUT, mcp.read_response(start_id)).await??; let mut expected_versions = String::new(); - for version in ["1.0.0", "2.0.0", "0.9.0"] { + for (version, expected_trust) in [ + ("1.0.0", HookTrustStatus::Trusted), + ("2.0.0", HookTrustStatus::Modified), + ("2.0.0", HookTrustStatus::Trusted), + ("0.9.0", HookTrustStatus::Modified), + ("0.9.0", HookTrustStatus::Trusted), + ] { // This test process updates the shared store without notifying app-server. write_versioned_plugin_hook(source.path(), version, &hook_log_path)?; store.install(AbsolutePathBuf::try_from(source.path())?, plugin_id.clone())?; @@ -642,8 +648,66 @@ async fn loaded_session_refreshes_externally_updated_plugin_hooks() -> Result<() mcp.read_stream_until_notification_message("turn/completed"), ) .await??; - expected_versions.push_str(&format!("{version}\n")); + if expected_trust == HookTrustStatus::Trusted { + expected_versions.push_str(&format!("{version}\n")); + } assert_eq!(std::fs::read_to_string(&hook_log_path)?, expected_versions); + let list_id = mcp + .send_hooks_list_request(HooksListParams { + cwds: vec![codex_home.path().to_path_buf()], + }) + .await?; + let HooksListResponse { data } = + timeout(DEFAULT_TIMEOUT, mcp.read_response(list_id)).await??; + let hook = data[0] + .hooks + .iter() + .find(|hook| hook.event_name == HookEventName::UserPromptSubmit) + .expect("plugin should register a user-prompt hook"); + assert_eq!(hook.trust_status, expected_trust); + if expected_trust == HookTrustStatus::Trusted { + let saved: toml::Value = toml::from_str(&std::fs::read_to_string( + codex_home.path().join("config.toml"), + )?)?; + // Older clients compare only the declaration hash; keep that approval valid. + let declaration_hash = command_hook_hash( + "user_prompt_submit", + /*matcher*/ None, + "python3 ${PLUGIN_ROOT}/hooks/log_version.py", + hook.timeout_sec, + /*async*/ false, + /*status_message*/ None, + /*additional_context_limit*/ None, + ); + assert_eq!( + saved["hooks"]["state"][&hook.key] + .clone() + .try_into::()?, + codex_config::HookStateToml { + enabled: None, + trusted_hash: Some(declaration_hash), + trusted_content_hash: Some( + hook.current_hash.split_once('/').unwrap().1.to_string() + ), + }, + ); + } + if expected_trust == HookTrustStatus::Modified { + let trust_id = mcp + .send_config_batch_write_request(ConfigBatchWriteParams { + edits: vec![ConfigEdit { + key_path: format!("hooks.state.\"{}\".trusted_hash", hook.key), + value: serde_json::json!(hook.current_hash), + merge_strategy: MergeStrategy::Upsert, + }], + file_path: None, + expected_version: None, + reload_user_config: true, + }) + .await?; + let _: codex_app_server_protocol::ConfigWriteResponse = + timeout(DEFAULT_TIMEOUT, mcp.read_response(trust_id)).await??; + } } Ok(()) } @@ -861,22 +925,7 @@ async fn hooks_list_shows_discovered_plugin_mcp_tool_hook() -> Result<()> { .path() .join("plugins/cache/test/demo/local/hooks/hooks.json"), )?)?; - let identity = NormalizedHookIdentity { - event_name: "pre_tool_use", - group: codex_config::MatcherGroup { - matcher: Some("Bash".to_string()), - hooks: vec![codex_config::HookHandlerConfig::McpTool { - server: "security".to_string(), - tool: "inspect".to_string(), - input: serde_json::from_value(serde_json::json!({ - "path": "${tool_input.path}", - }))?, - timeout_sec: Some(9), - status_message: Some("checking security policy".to_string()), - }], - }, - }; - let identity = codex_config::TomlValue::try_from(identity)?; + let current_hash = data[0].hooks[0].current_hash.clone(); assert_eq!( data, @@ -899,7 +948,7 @@ async fn hooks_list_shows_discovered_plugin_mcp_tool_hook() -> Result<()> { display_order: 0, enabled: true, is_managed: false, - current_hash: codex_config::version_for_toml(&identity), + current_hash, trust_status: HookTrustStatus::Untrusted, }], warnings: Vec::new(), @@ -969,6 +1018,7 @@ enabled = false let source_path = AbsolutePathBuf::from_absolute_path(std::fs::canonicalize( plugin_root.join("hooks/hooks.json"), )?)?; + let current_hash = data[0].hooks[0].current_hash.clone(); assert_eq!( data, vec![HooksListEntry { @@ -990,15 +1040,7 @@ enabled = false display_order: 1, enabled: false, is_managed: false, - current_hash: command_hook_hash( - "stop", - /*matcher*/ None, - ordinary_command, - /*timeout_sec*/ 5, - /*async*/ false, - /*status_message*/ None, - /*additional_context_limit*/ None, - ), + current_hash, trust_status: HookTrustStatus::Untrusted, }], warnings: Vec::new(), diff --git a/codex-rs/app-server/tests/suite/v2/plugin_list.rs b/codex-rs/app-server/tests/suite/v2/plugin_list.rs index 075ab81845..9192a27d07 100644 --- a/codex-rs/app-server/tests/suite/v2/plugin_list.rs +++ b/codex-rs/app-server/tests/suite/v2/plugin_list.rs @@ -3915,9 +3915,16 @@ trusted_hash = "sha256:unrelated" )?)?; let hook_states = config["hooks"]["state"].as_table().expect("hook states"); for hook in &hooks { + let (declaration_hash, content_hash) = hook + .current_hash + .split_once('/') + .expect("declaration and content hashes"); assert_eq!( - hook_states[hook.key.as_str()]["trusted_hash"].as_str(), - Some(hook.current_hash.as_str()) + ( + hook_states[hook.key.as_str()]["trusted_hash"].as_str(), + hook_states[hook.key.as_str()]["trusted_content_hash"].as_str(), + ), + (Some(declaration_hash), Some(content_hash)) ); } assert!( diff --git a/codex-rs/config/src/hook_config.rs b/codex-rs/config/src/hook_config.rs index 7575fa5fcb..31f82cb829 100644 --- a/codex-rs/config/src/hook_config.rs +++ b/codex-rs/config/src/hook_config.rs @@ -30,6 +30,9 @@ pub struct HookStateToml { pub enabled: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub trusted_hash: Option, + /// Approved plugin contents; kept separate for declaration-only clients. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub trusted_content_hash: Option, } #[derive(Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] diff --git a/codex-rs/config/src/hooks_tests.rs b/codex-rs/config/src/hooks_tests.rs index bd71a7f0e3..fbf1a5b752 100644 --- a/codex-rs/config/src/hooks_tests.rs +++ b/codex-rs/config/src/hooks_tests.rs @@ -229,6 +229,7 @@ command = "python3 /tmp/pre.py" super::HookStateToml { enabled: Some(false), trusted_hash: Some("sha256:abc123".to_string()), + trusted_content_hash: None, }, )]), } diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 8ddfa9b558..44f9a381b8 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -2262,6 +2262,10 @@ "enabled": { "type": "boolean" }, + "trusted_content_hash": { + "description": "Approved plugin contents; kept separate for declaration-only clients.", + "type": "string" + }, "trusted_hash": { "type": "string" } diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index 274874c1af..0be828930f 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -11,7 +11,6 @@ use codex_core::StartThreadOptions; use codex_core::TurnInput; use codex_core::TurnInputRequest; use codex_core::TurnStartOptions; -use codex_core::config::Config; use codex_core::config::Constrained; use codex_core::config::ThreadStoreConfig; use codex_features::Feature; @@ -20,8 +19,6 @@ use codex_history::RolloutItem; use codex_model_provider_info::ModelProviderInfo; use codex_model_provider_info::built_in_model_providers; use codex_models_manager::bundled_models_response; -use codex_plugin::PluginHookSource; -use codex_plugin::PluginId; use codex_protocol::items::parse_hook_prompt_fragment; use codex_protocol::mcp::ClientMcpExtensions; use codex_protocol::models::ContentItem; @@ -47,11 +44,9 @@ use codex_protocol::request_user_input::RequestUserInputAnswer; use codex_protocol::request_user_input::RequestUserInputResponse; use codex_protocol::user_input::UserInput; use codex_thread_store::InMemoryThreadStore; -use codex_utils_absolute_path::AbsolutePathBuf; use core_test_support::TestTargetOs; use core_test_support::fs_wait; use core_test_support::hooks::trust_discovered_hooks; -use core_test_support::hooks::trust_hooks; use core_test_support::managed_network_requirements_loader; use core_test_support::responses; use core_test_support::responses::ev_apply_patch_custom_tool_call; @@ -170,24 +165,6 @@ fn non_openai_model_provider(server: &wiremock::MockServer) -> ModelProviderInfo provider } -fn trust_plugin_hooks(config: &mut Config, plugin_hook_sources: Vec) { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); - let listed = codex_hooks::list_hooks(codex_hooks::HooksConfig { - feature_enabled: true, - config_layer_stack: Some(config.config_layer_stack.clone()), - plugin_hook_sources, - ..codex_hooks::HooksConfig::default() - }); - assert!( - !listed.hooks.is_empty(), - "trusted plugin hook fixture should discover at least one hook" - ); - trust_hooks(config, listed.hooks); -} - fn write_stop_hook(home: &Path, block_prompts: &[&str]) -> Result<()> { let script_path = home.join("stop_hook.py"); let log_path = home.join("stop_hook_log.jsonl"); @@ -4569,6 +4546,7 @@ async fn local_bundled_cleanup_hook_runs_without_saved_trust( HookStateToml { enabled: Some(false), trusted_hash: None, + trusted_content_hash: None, }, )]); if !hooks_enabled { @@ -4584,6 +4562,7 @@ async fn local_bundled_cleanup_hook_runs_without_saved_trust( HookStateToml { enabled: None, trusted_hash: Some(hook.current_hash), + trusted_content_hash: None, }, ); } @@ -4651,8 +4630,11 @@ async fn plugin_pre_tool_use_blocks_exec_command_before_execution() -> Result<() skip_if_no_network!(Ok(())); let server = start_mock_server().await; + let home = Arc::new(TempDir::new()?); let call_id = "plugin-pretooluse-exec-command"; - let marker = std::env::temp_dir().join("plugin-pretooluse-exec-command-marker"); + let same_turn_call_id = "plugin-pretooluse-exec-command-same-turn"; + let changed_call_id = "plugin-pretooluse-changed-script"; + let marker = home.path().join("plugin-pretooluse-exec-command-marker"); let command = format!("printf blocked > {}", marker.display()); let args = serde_json::json!({ "cmd": command }); let responses = mount_sse_sequence( @@ -4667,6 +4649,24 @@ async fn plugin_pre_tool_use_blocks_exec_command_before_execution() -> Result<() ), ev_completed("resp-1"), ]), + sse(vec![ + ev_response_created("resp-same-turn"), + ev_function_call( + same_turn_call_id, + "exec_command", + &serde_json::to_string(&args)?, + ), + ev_completed("resp-same-turn"), + ]), + sse(vec![ + ev_response_created("resp-changed"), + ev_function_call( + changed_call_id, + "exec_command", + &serde_json::to_string(&args)?, + ), + ev_completed("resp-changed"), + ]), sse(vec![ ev_response_created("resp-2"), ev_assistant_message("msg-1", "plugin hook blocked it"), @@ -4676,7 +4676,6 @@ async fn plugin_pre_tool_use_blocks_exec_command_before_execution() -> Result<() ) .await; - let home = Arc::new(TempDir::new()?); let plugin_root = home.path().join("plugins/cache/test/sample/local"); let hooks_dir = plugin_root.join("hooks"); fs::create_dir_all(plugin_root.join(".codex-plugin")) @@ -4691,18 +4690,40 @@ async fn plugin_pre_tool_use_blocks_exec_command_before_execution() -> Result<() home.path().join("config.toml"), r#"[plugins."sample@test"] enabled = true + +# Declaration-only approvals from before content fingerprinting. +[hooks.state."sample@test:hooks/hooks.json:pre_tool_use:0:0"] +trusted_hash = "sha256:c9ee1aca1b5d8c72b99df6571c87d61350b09f7f4594ae30e7250a2027e9d76b" +[hooks.state."sample@test:hooks/hooks.json:post_tool_use:0:0"] +trusted_hash = "sha256:9eef43ad6f26b1f9b94f318a50445f8bb62c363da08152a2966b73d308bf42ba" "#, ) .context("write plugin config")?; let script_path = hooks_dir.join("pre_tool_use_hook.py"); - let log_path = hooks_dir.join("pre_tool_use_hook_log.jsonl"); + let log_path = home.path().join("pre_tool_use_hook_log.jsonl"); + fs::write( + hooks_dir.join("hook_support.py"), + "DENIAL_REASON = 'blocked by plugin hook'\n", + ) + .context("write bundled hook module")?; fs::write( &script_path, format!( r#"import json from pathlib import Path import sys +sys.path.insert(0, str(Path(__file__).parent)) +from hook_support import DENIAL_REASON + +root = Path(__file__).parents[1] +for relative in (".venv/pyvenv.cfg", "sample.egg-info/PKG-INFO"): + artifact = root / relative + artifact.parent.mkdir(exist_ok=True) + artifact.write_text("generated") +python_link = root / ".venv/python" +if sys.platform != "win32" and not python_link.exists(): + python_link.symlink_to(sys.executable) payload = json.load(sys.stdin) with Path(r"{log_path}").open("a", encoding="utf-8") as handle: @@ -4712,9 +4733,12 @@ print(json.dumps({{ "hookSpecificOutput": {{ "hookEventName": "PreToolUse", "permissionDecision": "deny", - "permissionDecisionReason": "blocked by plugin hook" + "permissionDecisionReason": DENIAL_REASON }} }})) +if len(Path(r"{log_path}").read_text().splitlines()) == 2: + with Path(__file__).open("a", encoding="utf-8") as script: + script.write("\n# updated plugin bundle\n") "#, log_path = log_path.display(), ), @@ -4726,30 +4750,20 @@ print(json.dumps({{ "matcher": "^Bash$", "hooks": [{ "type": "command", - "command": "python3 ${PLUGIN_ROOT}/hooks/pre_tool_use_hook.py" + "command": "python3 -I -S ${PLUGIN_ROOT}/hooks/pre_tool_use_hook.py" + }] + }], + "PostToolUse": [{ + "matcher": "^Bash$", + "hooks": [{ + "type": "command", + "command": "python3 -I -S ${PLUGIN_ROOT}/hooks/pre_tool_use_hook.py" }] }] } }"#; let plugin_hooks_path = hooks_dir.join("hooks.json"); fs::write(&plugin_hooks_path, plugin_hooks_json).context("write plugin hooks config")?; - let plugin_root_abs = - AbsolutePathBuf::try_from(plugin_root.clone()).context("absolute plugin root")?; - let plugin_hooks_path_abs = - AbsolutePathBuf::try_from(plugin_hooks_path).context("absolute plugin hooks path")?; - let plugin_data_root = - AbsolutePathBuf::try_from(plugin_root.join("data")).context("absolute plugin data root")?; - let plugin_hook_sources = vec![PluginHookSource { - plugin_id: PluginId::parse("sample@test").context("plugin id")?, - plugin_root: plugin_root_abs, - plugin_data_root, - source_path: plugin_hooks_path_abs, - source_relative_path: "hooks/hooks.json".to_string(), - hooks: serde_json::from_str::(plugin_hooks_json) - .context("parse plugin hooks")? - .hooks, - }]; - let mut builder = test_codex() .with_home(Arc::clone(&home)) .with_config(move |config| { @@ -4757,13 +4771,11 @@ print(json.dumps({{ .features .enable(Feature::Plugins) .expect("test config should allow feature update"); - trust_plugin_hooks(config, plugin_hook_sources); + config.features.enable(Feature::CodexHooks).unwrap(); }); let test = builder.build(&server).await?; - - if marker.exists() { - fs::remove_file(&marker).context("remove leftover plugin pre tool use marker")?; - } + assert!(!home.path().join(".hook-trust").exists()); + let legacy_config = fs::read_to_string(home.path().join("config.toml"))?; test.submit_turn_with_policy( "run the shell command blocked by a plugin hook", @@ -4772,7 +4784,7 @@ print(json.dumps({{ .await?; let requests = responses.requests(); - assert_eq!(requests.len(), 2); + assert_eq!(requests.len(), 4); let output_item = requests[1].function_call_output(call_id); let output = output_item .get("output") @@ -4782,17 +4794,32 @@ print(json.dumps({{ output.contains("Command blocked by PreToolUse hook: blocked by plugin hook"), "blocked tool output should surface the plugin hook reason", ); + assert!(marker.exists(), "the modified hook must not execute again"); assert!( - !marker.exists(), - "plugin hook should block shell command execution" + requests[2].function_call_output(same_turn_call_id)["output"] + .as_str() + .unwrap() + .contains("Command blocked by PreToolUse hook") ); let hook_inputs = read_hook_inputs_from_log(&log_path)?; - assert_eq!(hook_inputs.len(), 1); + // A legacy hook first used after a bundle change must share the earlier baseline. + assert_eq!(hook_inputs.len(), 2); assert_eq!(hook_inputs[0]["hook_event_name"], "PreToolUse"); assert_eq!(hook_inputs[0]["tool_name"], "Bash"); assert_eq!(hook_inputs[0]["tool_use_id"], call_id); assert_eq!(hook_inputs[0]["tool_input"]["command"], command); + assert!( + hooks_dir.join("__pycache__").exists(), + "isolated Python should create bytecode without invalidating hook trust" + ); + assert_eq!(hook_inputs[1]["tool_use_id"], same_turn_call_id); + // Migration keeps declaration-only approvals readable by older clients. + assert_eq!( + fs::read_to_string(home.path().join("config.toml"))?, + legacy_config + ); + assert_eq!(fs::read_dir(home.path().join(".hook-trust"))?.count(), 1); Ok(()) } diff --git a/codex-rs/hooks/Cargo.toml b/codex-rs/hooks/Cargo.toml index 4ad0b1ce97..d519e3b3d0 100644 --- a/codex-rs/hooks/Cargo.toml +++ b/codex-rs/hooks/Cargo.toml @@ -28,6 +28,8 @@ regex = { workspace = true } schemars = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } +sha2 = { workspace = true } +tempfile = { workspace = true } tokio = { workspace = true, features = ["fs", "io-util", "process", "sync", "time"] } tracing = { workspace = true } uuid = { workspace = true, features = ["v4"] } @@ -35,7 +37,6 @@ uuid = { workspace = true, features = ["v4"] } [dev-dependencies] libc = { workspace = true } pretty_assertions = { workspace = true } -tempfile = { workspace = true } tokio = { workspace = true, features = ["macros", "rt-multi-thread", "time"] } [target.'cfg(any(unix, windows))'.dependencies] diff --git a/codex-rs/hooks/src/config_rules.rs b/codex-rs/hooks/src/config_rules.rs index 050932ecbb..99ccfdbe9c 100644 --- a/codex-rs/hooks/src/config_rules.rs +++ b/codex-rs/hooks/src/config_rules.rs @@ -59,6 +59,9 @@ pub fn hook_states_from_stack( if let Some(trusted_hash) = state.trusted_hash { effective_state.trusted_hash = Some(trusted_hash); } + if let Some(trusted_content_hash) = state.trusted_content_hash { + effective_state.trusted_content_hash = Some(trusted_content_hash); + } } } @@ -104,6 +107,7 @@ mod tests { HookStateToml { enabled: Some(true), trusted_hash: None, + trusted_content_hash: None, }, )]) ); @@ -124,6 +128,7 @@ mod tests { HookStateToml { enabled: Some(/*enabled*/ false), trusted_hash: None, + trusted_content_hash: None, }, ), ), @@ -134,6 +139,7 @@ mod tests { HookStateToml { enabled: None, trusted_hash: Some("sha256:trusted".to_string()), + trusted_content_hash: None, }, ), ), @@ -150,6 +156,7 @@ mod tests { HookStateToml { enabled: Some(false), trusted_hash: Some("sha256:trusted".to_string()), + trusted_content_hash: None, }, )]) ); @@ -189,6 +196,7 @@ mod tests { HookStateToml { enabled: Some(false), trusted_hash: None, + trusted_content_hash: None, }, )]) ); @@ -230,6 +238,7 @@ mod tests { HookStateToml { enabled: Some(false), trusted_hash: None, + trusted_content_hash: None, }, )]) ); @@ -241,6 +250,7 @@ mod tests { HookStateToml { enabled, trusted_hash: None, + trusted_content_hash: None, }, ) } diff --git a/codex-rs/hooks/src/engine/command_runner.rs b/codex-rs/hooks/src/engine/command_runner.rs index d323c2131e..cd43860ad9 100644 --- a/codex-rs/hooks/src/engine/command_runner.rs +++ b/codex-rs/hooks/src/engine/command_runner.rs @@ -216,6 +216,28 @@ pub(crate) async fn run_command( ) -> HandlerRunResult { let started_at = chrono::Utc::now().timestamp(); let started = Instant::now(); + let timeout_started = tokio::time::Instant::now(); + let duration = Duration::from_secs(handler.timeout_sec); + if let Some(trust) = &handler.plugin_trust { + let error = match timeout(duration, trust.verify()).await { + Ok(Ok(())) => None, + Ok(Err(err)) => Some(err.to_string()), + Err(_) => Some(format!("hook timed out after {}s", handler.timeout_sec)), + }; + if let Some(error) = error { + return finish_command_run( + started_at, + started, + CommandRunCompletion { + exit_code: None, + stdout: String::new(), + stderr: String::new(), + error: Some(error), + outcome: "trust_error", + }, + ); + } + } let mut command = build_command(&runtime.shell, command_line, &runtime.environment, env); command.current_dir(cwd); @@ -268,10 +290,14 @@ pub(crate) async fn run_command( .map_err(|err| ("wait_error", err.to_string())) }; - let timeout_duration = Duration::from_secs(handler.timeout_sec); // Drain output while sending input so neither pipe can block the other, and // include stdin writes in the deadline even when the hook never reads them. - match timeout(timeout_duration, try_join(write_stdin, wait_with_output)).await { + match timeout( + duration.saturating_sub(timeout_started.elapsed()), + try_join(write_stdin, wait_with_output), + ) + .await + { Ok(Ok(((), output))) => { // Successfully completed hooks may intentionally leave detached helpers running. #[cfg(windows)] diff --git a/codex-rs/hooks/src/engine/command_runner_tests.rs b/codex-rs/hooks/src/engine/command_runner_tests.rs index e8e6577a5a..ac5a3fa5b2 100644 --- a/codex-rs/hooks/src/engine/command_runner_tests.rs +++ b/codex-rs/hooks/src/engine/command_runner_tests.rs @@ -112,6 +112,7 @@ async fn hook_shell_startup_does_not_stop_on_controlling_terminal() { )]); let handler = ConfiguredHandler { builtin: false, + plugin_trust: None, event_name: HookEventName::SessionStart, matcher: None, timeout_sec: 2, @@ -216,6 +217,7 @@ async fn cmd_shell_runs_quoted_hook_command_path() { let env = HashMap::new(); let handler = ConfiguredHandler { builtin: false, + plugin_trust: None, event_name: HookEventName::SessionStart, matcher: None, timeout_sec: 10, @@ -266,6 +268,7 @@ async fn fast_exiting_hook_preserves_stdout_when_stdin_is_not_consumed() { let env = HashMap::new(); let handler = ConfiguredHandler { builtin: false, + plugin_trust: None, event_name: HookEventName::SessionStart, matcher: None, timeout_sec: 10, @@ -354,6 +357,7 @@ async fn command_hook_does_not_expose_configured_noise_auth_token() { ]); let handler = ConfiguredHandler { builtin: false, + plugin_trust: None, event_name: HookEventName::SessionStart, matcher: None, timeout_sec: 10, @@ -492,6 +496,7 @@ fn write_handler(temp: &TempDir, source: &str) -> ConfiguredHandler { std::fs::write(&script_path, source).expect("write async test hook"); ConfiguredHandler { builtin: false, + plugin_trust: None, event_name: HookEventName::UserPromptSubmit, matcher: None, timeout_sec: 10, diff --git a/codex-rs/hooks/src/engine/discovery.rs b/codex-rs/hooks/src/engine/discovery.rs index 0c5a28ef35..1986462a4a 100644 --- a/codex-rs/hooks/src/engine/discovery.rs +++ b/codex-rs/hooks/src/engine/discovery.rs @@ -28,6 +28,7 @@ use super::ConfiguredHandlerKind; use super::HookListEntry; use super::HookListEntryHandler; use super::dispatcher::hook_event_name_label; +use super::plugin_trust::PluginHookTrust; use crate::config_rules::hook_states_from_stack; use crate::engine::HookMatcher; use crate::events::common::matcher_pattern_for_event; @@ -55,6 +56,9 @@ struct HookHandlerSource<'a> { hook_states: &'a HashMap, env: HashMap, plugin_id: Option, + plugin_content_hash: Option<&'a std::io::Result>, + config_layer_stack: Option<&'a ConfigLayerStack>, + plugin_root: Option<&'a AbsolutePathBuf>, } enum HookRequirement<'a> { @@ -79,13 +83,21 @@ struct NormalizedHandler { additional_context_limit: Option, } -#[derive(Clone, Copy)] -struct HookDiscoveryPolicy { - allow_managed_hooks_only: bool, - bypass_hook_trust: bool, +#[derive(Clone, Copy, PartialEq, Eq)] +pub(crate) enum DiscoveryMode { + Runtime, + Listing, } -impl HookDiscoveryPolicy { +#[derive(Clone, Copy)] +struct HookDiscoveryPolicy<'a> { + config_layer_stack: Option<&'a ConfigLayerStack>, + allow_managed_hooks_only: bool, + bypass_hook_trust: bool, + mode: DiscoveryMode, +} + +impl HookDiscoveryPolicy<'_> { fn allows(self, source: &HookHandlerSource<'_>) -> bool { !self.allow_managed_hooks_only || source.is_managed } @@ -96,6 +108,7 @@ pub(crate) fn discover_handlers( plugin_hook_sources: Vec, plugin_hook_load_warnings: Vec, bypass_hook_trust: bool, + mode: DiscoveryMode, ) -> DiscoveryResult { let mut handlers = Vec::new(); let mut hook_entries = Vec::new(); @@ -105,6 +118,7 @@ pub(crate) fn discover_handlers( let mut visited_json_hook_folders = HashSet::new(); let hook_states = hook_states_from_stack(config_layer_stack); let policy = HookDiscoveryPolicy { + config_layer_stack, allow_managed_hooks_only: config_layer_stack.is_some_and(|config_layer_stack| { config_layer_stack .requirements() @@ -113,6 +127,7 @@ pub(crate) fn discover_handlers( .is_some_and(|requirement| requirement.value) }), bypass_hook_trust, + mode, }; if let Some(config_layer_stack) = config_layer_stack { @@ -139,6 +154,9 @@ pub(crate) fn discover_handlers( hook_states: &hook_states, env: HashMap::new(), plugin_id: None, + plugin_content_hash: None, + config_layer_stack: None, + plugin_root: None, }; if !policy.allows(&policy_source) { continue; @@ -179,6 +197,9 @@ pub(crate) fn discover_handlers( hook_states: &hook_states, env: HashMap::new(), plugin_id: None, + plugin_content_hash: None, + config_layer_stack: None, + plugin_root: None, }, hook_events, policy, @@ -212,7 +233,7 @@ fn append_managed_requirement_handlers( display_order: &mut i64, config_layer_stack: &ConfigLayerStack, hook_states: &HashMap, - policy: HookDiscoveryPolicy, + policy: HookDiscoveryPolicy<'_>, ) -> Vec { let Some(managed_hooks) = config_layer_stack.requirements().managed_hooks.as_ref() else { return Vec::new(); @@ -234,6 +255,9 @@ fn append_managed_requirement_handlers( hook_states, env: HashMap::new(), plugin_id: None, + plugin_content_hash: None, + config_layer_stack: None, + plugin_root: None, }, managed_hooks.get().hooks.clone(), policy, @@ -248,8 +272,13 @@ fn append_plugin_hook_sources( display_order: &mut i64, plugin_hook_sources: Vec, hook_states: &HashMap, - policy: HookDiscoveryPolicy, + policy: HookDiscoveryPolicy<'_>, ) { + if policy.allow_managed_hooks_only { + return; + } + // Listing inspects current contents; runtime discovery defers hashing until execution. + let mut content_hashes = HashMap::new(); for source in plugin_hook_sources { let PluginHookSource { plugin_root, @@ -259,6 +288,14 @@ fn append_plugin_hook_sources( source_relative_path, hooks, } = source; + let plugin_id = plugin_id.as_key(); + let content_hash = (policy.mode == DiscoveryMode::Listing).then(|| { + content_hashes + .entry(plugin_root.clone()) + .or_insert_with(|| { + super::plugin_fingerprint::plugin_content_hash(&plugin_root, &|| false) + }) + }); let mut env = HashMap::new(); let plugin_root_value = plugin_root.display().to_string(); let plugin_data_root_value = plugin_data_root.display().to_string(); @@ -268,7 +305,6 @@ fn append_plugin_hook_sources( env.insert("PLUGIN_DATA".to_string(), plugin_data_root_value.clone()); // For OOTB compat with existing plugins that use this env var. env.insert("CLAUDE_PLUGIN_DATA".to_string(), plugin_data_root_value); - let plugin_id = plugin_id.as_key(); append_hook_events( handlers, hook_entries, @@ -287,6 +323,9 @@ fn append_plugin_hook_sources( hook_states, env, plugin_id: Some(plugin_id), + plugin_content_hash: content_hash.as_deref(), + config_layer_stack: policy.config_layer_stack, + plugin_root: Some(&plugin_root), }, hooks, policy, @@ -456,7 +495,7 @@ fn append_hook_events( display_order: &mut i64, mut source: HookHandlerSource<'_>, hook_events: HookEventsToml, - policy: HookDiscoveryPolicy, + policy: HookDiscoveryPolicy<'_>, ) { if !policy.allows(&source) { return; @@ -663,7 +702,6 @@ fn append_matcher_groups( status_message, additional_context_limit, } = normalized; - let current_hash = hook_hash(event_name, matcher, &group, &config); let key = crate::hook_key(&source.key_source, event_name, group_index, handler_index); let state = source.hook_states.get(&key); let builtin = source.plugin_id.as_deref().is_some_and(|plugin_id| { @@ -675,10 +713,67 @@ fn append_matcher_groups( /*app_connector_id*/ None, ) }); + let plugin_content_hash = match source.plugin_content_hash { + Some(Ok(hash)) => Some(hash.as_str()), + Some(Err(err)) if !builtin && !source.bypass_hook_trust => { + source.record_load_failure( + format!( + "skipping plugin hook in {}: failed to fingerprint plugin contents: {err}", + source.path.display() + ), + warnings, + ); + continue; + } + Some(Err(_)) | None => None, + }; + let declaration_hash = hook_hash(event_name, matcher, &group, &config); + // The config writer persists these two approval hashes separately. + let current_hash = plugin_content_hash.map_or_else( + || declaration_hash.clone(), + |hash| format!("{declaration_hash}/{hash}"), + ); let enabled = hook_enabled(source.is_managed, builtin, state); let trusted_hash = hook_trusted_hash(source.is_managed, state); - let trust_status = - hook_trust_status(source.is_managed, builtin, ¤t_hash, trusted_hash); + let plugin_trust = if !builtin + && (!source.bypass_hook_trust || source.plugin_content_hash.is_some()) + && let Some(root) = source.plugin_root + && let Some(plugin_id) = source.plugin_id.as_deref() + && let Some(trusted_hash) = trusted_hash + { + Some(PluginHookTrust { + root: root.clone(), + legacy_path: source + .config_layer_stack + .and_then(|stack| super::plugin_trust::legacy_trust_path(stack, plugin_id)), + declaration_hash, + trusted_hash: trusted_hash.to_owned(), + trusted_content_hash: state + .and_then(|state| state.trusted_content_hash.clone()), + }) + } else { + None + }; + let trust_status = if let Some(content_hash) = plugin_content_hash + && let Some(trust) = plugin_trust.as_ref() + { + match trust.matches_hash(content_hash) { + Ok(true) => HookTrustStatus::Trusted, + Ok(false) => HookTrustStatus::Modified, + Err(err) => { + source.record_load_failure( + format!( + "skipping plugin hook in {}: failed to migrate hook trust: {err}", + source.path.display() + ), + warnings, + ); + continue; + } + } + } else { + hook_trust_status(source.is_managed, builtin, ¤t_hash, trusted_hash) + }; let handler = match &kind { ConfiguredHandlerKind::Command { command, r#async, .. @@ -714,6 +809,7 @@ fn append_matcher_groups( }); if enabled && (source.bypass_hook_trust + || (source.plugin_content_hash.is_none() && plugin_trust.is_some()) || matches!( trust_status, HookTrustStatus::Managed | HookTrustStatus::Trusted @@ -721,6 +817,7 @@ fn append_matcher_groups( { handlers.push(ConfiguredHandler { builtin, + plugin_trust, event_name, matcher: compiled_matcher.clone(), timeout_sec, @@ -913,6 +1010,9 @@ mod tests { hook_states, env: std::collections::HashMap::new(), plugin_id: None, + plugin_content_hash: None, + config_layer_stack: None, + plugin_root: None, } } @@ -931,6 +1031,9 @@ mod tests { hook_states, env: std::collections::HashMap::new(), plugin_id: None, + plugin_content_hash: None, + config_layer_stack: None, + plugin_root: None, } } @@ -1258,6 +1361,7 @@ mod tests { handlers, vec![ConfiguredHandler { builtin: false, + plugin_trust: None, event_name: HookEventName::UserPromptSubmit, matcher: None, timeout_sec: 600, @@ -1298,6 +1402,7 @@ mod tests { handlers, vec![ConfiguredHandler { builtin: false, + plugin_trust: None, event_name: HookEventName::PreToolUse, matcher: Some(HookMatcher::new("^Bash$").expect("valid matcher")), timeout_sec: 600, @@ -1517,6 +1622,7 @@ mod tests { HookStateToml { enabled: Some(false), trusted_hash: None, + trusted_content_hash: None, }, )]); diff --git a/codex-rs/hooks/src/engine/dispatcher.rs b/codex-rs/hooks/src/engine/dispatcher.rs index 30c0c34961..8f17b6714b 100644 --- a/codex-rs/hooks/src/engine/dispatcher.rs +++ b/codex-rs/hooks/src/engine/dispatcher.rs @@ -352,6 +352,7 @@ mod tests { ) -> ConfiguredHandler { ConfiguredHandler { builtin: false, + plugin_trust: None, event_name, matcher: matcher .map(|pattern| crate::engine::HookMatcher::new(pattern).expect("valid matcher")), diff --git a/codex-rs/hooks/src/engine/mcp_runner.rs b/codex-rs/hooks/src/engine/mcp_runner.rs index 18cb81e185..2c3f26f854 100644 --- a/codex-rs/hooks/src/engine/mcp_runner.rs +++ b/codex-rs/hooks/src/engine/mcp_runner.rs @@ -37,7 +37,14 @@ pub(crate) async fn run_mcp_tool( ) -> HandlerRunResult { let started_at = chrono::Utc::now().timestamp(); let started = Instant::now(); + let timeout_started = tokio::time::Instant::now(); + let duration = Duration::from_secs(handler.timeout_sec); let result = async { + if let Some(trust) = &handler.plugin_trust { + tokio::time::timeout(duration, trust.verify()) + .await + .context("hook trust verification timed out")??; + } let hook_event: Value = serde_json::from_str(hook_event_json).context("failed to parse hook event input")?; let input = expand_mcp_argument_template(argument_template, &hook_event)?; @@ -70,7 +77,11 @@ pub(crate) async fn run_mcp_tool( environment_id, metadata: call_metadata, input, - timeout: Duration::from_secs(handler.timeout_sec), + timeout: if handler.plugin_trust.is_some() { + duration.saturating_sub(timeout_started.elapsed()) + } else { + Duration::from_secs(handler.timeout_sec) + }, }) .await } diff --git a/codex-rs/hooks/src/engine/mcp_runner_tests.rs b/codex-rs/hooks/src/engine/mcp_runner_tests.rs index 4c4726d284..8a37d8d238 100644 --- a/codex-rs/hooks/src/engine/mcp_runner_tests.rs +++ b/codex-rs/hooks/src/engine/mcp_runner_tests.rs @@ -96,6 +96,7 @@ async fn mcp_tool_results_use_command_hook_output_contract() { .expect("object input"); let handler = ConfiguredHandler { builtin: false, + plugin_trust: None, event_name: HookEventName::PostToolUse, matcher: None, timeout_sec: 30, diff --git a/codex-rs/hooks/src/engine/mod.rs b/codex-rs/hooks/src/engine/mod.rs index 75110866e1..f1d46adc26 100644 --- a/codex-rs/hooks/src/engine/mod.rs +++ b/codex-rs/hooks/src/engine/mod.rs @@ -4,6 +4,8 @@ pub(crate) mod dispatcher; mod matcher; pub(crate) mod mcp_runner; pub(crate) mod output_parser; +mod plugin_fingerprint; +mod plugin_trust; pub(crate) mod schema_loader; pub(crate) use matcher::HookMatcher; @@ -62,6 +64,7 @@ pub(crate) struct CommandShell { pub(crate) struct ConfiguredHandler { /// Internally admitted cleanup hook, enabled independently of per-hook state. pub builtin: bool, + pub plugin_trust: Option, pub event_name: codex_protocol::protocol::HookEventName, pub matcher: Option, pub timeout_sec: u64, @@ -256,6 +259,7 @@ impl ClaudeHooksEngine { plugin_hook_sources, plugin_hook_load_warnings, bypass_hook_trust, + discovery::DiscoveryMode::Runtime, ); if !enabled { discovered.handlers.retain(|handler| handler.builtin); @@ -335,6 +339,7 @@ impl ClaudeHooksEngine { } handlers.push(ConfiguredHandler { builtin: true, + plugin_trust: None, event_name, matcher: None, timeout_sec: timeout_sec.unwrap_or(5).max(1), diff --git a/codex-rs/hooks/src/engine/mod_tests.rs b/codex-rs/hooks/src/engine/mod_tests.rs index 9c5f46fdfa..f082dbf144 100644 --- a/codex-rs/hooks/src/engine/mod_tests.rs +++ b/codex-rs/hooks/src/engine/mod_tests.rs @@ -45,6 +45,7 @@ use super::ConfiguredHandler; use super::ConfiguredHandlerKind; use super::HandlerSourcePath; use super::HookListEntryHandler; +use super::discovery::DiscoveryMode; use crate::events::interrupt::InterruptRequest; use crate::events::pre_tool_use::PreToolUseRequest; use crate::events::stop::StopHookTarget; @@ -92,6 +93,7 @@ fn permission_request_timeout_only_counts_synchronous_handlers() { let command = "echo synchronous permission hook"; let synchronous_handler = ConfiguredHandler { builtin: false, + plugin_trust: None, event_name: HookEventName::PermissionRequest, matcher: None, timeout_sec: 5, @@ -815,6 +817,7 @@ fn unknown_requirement_source_hooks_stay_managed() { Vec::new(), Vec::new(), /*bypass_hook_trust*/ false, + DiscoveryMode::Listing, ); assert_eq!(discovered.hook_entries.len(), 1); assert_eq!(discovered.hook_entries[0].source, HookSource::Unknown); @@ -902,6 +905,7 @@ fn user_disablement_filters_non_managed_hooks_but_not_managed_hooks() { Vec::new(), Vec::new(), /*bypass_hook_trust*/ false, + DiscoveryMode::Listing, ); assert_eq!(discovered.hook_entries.len(), 2); assert_eq!(discovered.hook_entries[0].key, managed_disabled_key); @@ -969,6 +973,7 @@ fn user_disablement_does_not_filter_managed_layer_hooks() { Vec::new(), Vec::new(), /*bypass_hook_trust*/ false, + DiscoveryMode::Listing, ); assert_eq!(discovered.hook_entries.len(), 1); assert_eq!(discovered.hook_entries[0].key, managed_key); @@ -1038,15 +1043,18 @@ fn trusted_plugin_hook_stack( plugin_hook_sources.to_vec(), Vec::new(), /*bypass_hook_trust*/ false, + DiscoveryMode::Listing, ); let state = discovered .hook_entries .into_iter() .map(|entry| { + let (declaration, content) = entry.current_hash.split_once('/').unwrap(); ( entry.key, serde_json::json!({ - "trusted_hash": entry.current_hash, + "trusted_hash": declaration, + "trusted_content_hash": content, }), ) }) @@ -1195,6 +1203,7 @@ fn allow_managed_hooks_only_false_keeps_unmanaged_hooks() { Vec::new(), Vec::new(), /*bypass_hook_trust*/ false, + DiscoveryMode::Listing, ); assert_eq!(discovered.hook_entries.len(), 1); assert!(!discovered.hook_entries[0].is_managed); @@ -1253,6 +1262,7 @@ fn allow_managed_hooks_only_in_config_toml_does_not_enable_policy() { Vec::new(), Vec::new(), /*bypass_hook_trust*/ false, + DiscoveryMode::Listing, ); assert_eq!(discovered.hook_entries.len(), 1); assert!(!discovered.hook_entries[0].is_managed); @@ -1456,6 +1466,7 @@ fn allow_managed_hooks_only_keeps_managed_requirement_and_config_layer_hooks() { Vec::new(), Vec::new(), /*bypass_hook_trust*/ false, + DiscoveryMode::Listing, ); assert!(discovered.hook_entries.iter().all(|entry| entry.is_managed)); } @@ -1765,6 +1776,7 @@ fn bundled_cleanup_hooks_are_trusted_without_saved_hashes() { vec![bundled_cleanup_source(plugin_id, server, event)], Vec::new(), /*bypass_hook_trust*/ false, + DiscoveryMode::Listing, ); assert_eq!(discovered.handlers.len(), 1, "{plugin_id} {event}"); assert_eq!( @@ -1789,7 +1801,9 @@ fn bundled_cleanup_hooks_are_trusted_without_saved_hashes() { #[test] fn bundled_cleanup_trust_does_not_extend_to_other_handlers() { + let temp = tempdir().expect("create plugin directory"); let mut source = bundled_cleanup_source("browser@openai-bundled", "node_repl", "Stop"); + source.plugin_root = AbsolutePathBuf::try_from(temp.path()).expect("plugin root"); let cleanup = source.hooks.stop[0].hooks[0].clone(); source.hooks.stop[0].hooks.extend([ HookHandlerConfig::McpTool { @@ -1824,6 +1838,7 @@ fn bundled_cleanup_trust_does_not_extend_to_other_handlers() { vec![source], Vec::new(), /*bypass_hook_trust*/ false, + DiscoveryMode::Listing, ); assert_eq!(discovered.handlers.len(), 1); assert_eq!( @@ -1844,6 +1859,7 @@ fn bundled_cleanup_trust_does_not_extend_to_other_handlers() { #[test] fn bundled_cleanup_trust_requires_matching_plugin_server_and_event() { + let temp = tempdir().expect("create plugin directory"); for (plugin_id, server, event) in [ ("other@openai-bundled", "node_repl", "Stop"), ("browser@other", "node_repl", "Stop"), @@ -1858,11 +1874,14 @@ fn bundled_cleanup_trust_requires_matching_plugin_server_and_event() { "PreToolUse", ), ] { + let mut source = bundled_cleanup_source(plugin_id, server, event); + source.plugin_root = AbsolutePathBuf::try_from(temp.path()).expect("plugin root"); let discovered = super::discovery::discover_handlers( /*config_layer_stack*/ None, - vec![bundled_cleanup_source(plugin_id, server, event)], + vec![source], Vec::new(), /*bypass_hook_trust*/ false, + DiscoveryMode::Listing, ); assert!(discovered.handlers.is_empty(), "{plugin_id} {event}"); assert_eq!(discovered.hook_entries.len(), 1); @@ -1875,7 +1894,9 @@ fn bundled_cleanup_trust_requires_matching_plugin_server_and_event() { #[test] fn local_hosted_app_cleanup_hooks_require_saved_trust() { + let temp = tempdir().expect("create plugin directory"); let mut source = bundled_cleanup_source("browser@openai-curated-remote", "codex_apps", "Stop"); + source.plugin_root = AbsolutePathBuf::try_from(temp.path()).expect("plugin root"); source.hooks.stop[0].hooks[0] = HookHandlerConfig::McpTool { server: "codex_apps".to_string(), tool: "browser.turn_ended".to_string(), @@ -1888,6 +1909,7 @@ fn local_hosted_app_cleanup_hooks_require_saved_trust() { vec![source], Vec::new(), /*bypass_hook_trust*/ false, + DiscoveryMode::Listing, ); assert!(discovered.handlers.is_empty()); assert_eq!( @@ -1908,6 +1930,7 @@ fn builtin_cleanup_ignores_disablement_but_preserves_managed_only_policy() { vec![source.clone()], Vec::new(), /*bypass_hook_trust*/ false, + DiscoveryMode::Listing, ); let expected_entry = discovered.hook_entries[0].clone(); let disabled_stack = ConfigLayerStack::new( @@ -1927,6 +1950,7 @@ fn builtin_cleanup_ignores_disablement_but_preserves_managed_only_policy() { vec![source.clone()], Vec::new(), /*bypass_hook_trust*/ false, + DiscoveryMode::Listing, ); assert_eq!(disabled.handlers, discovered.handlers); assert_eq!(disabled.hook_entries, vec![expected_entry]); @@ -1941,6 +1965,7 @@ fn builtin_cleanup_ignores_disablement_but_preserves_managed_only_policy() { vec![source.clone()], Vec::new(), /*bypass_hook_trust*/ false, + DiscoveryMode::Listing, ); assert!(managed_only.handlers.is_empty()); assert!(managed_only.hook_entries.is_empty()); @@ -1967,7 +1992,9 @@ fn builtin_cleanup_ignores_disablement_but_preserves_managed_only_policy() { #[test] fn disabled_hooks_feature_keeps_builtin_cleanup_but_not_trusted_plugin_hooks() { + let temp = tempdir().expect("create plugin directory"); let mut source = bundled_cleanup_source("browser@openai-bundled", "node_repl", "Stop"); + source.plugin_root = AbsolutePathBuf::try_from(temp.path()).expect("plugin root"); source.hooks.stop[0].hooks.push(HookHandlerConfig::Command { command: "echo ordinary hook".to_string(), command_windows: None, @@ -1982,6 +2009,7 @@ fn disabled_hooks_feature_keeps_builtin_cleanup_but_not_trusted_plugin_hooks() { vec![source.clone()], Vec::new(), /*bypass_hook_trust*/ false, + DiscoveryMode::Runtime, ); assert_eq!(discovered.handlers.len(), 2); @@ -2144,6 +2172,7 @@ fn plugin_hook_sources_expand_plugin_placeholders() { AbsolutePathBuf::try_from(temp.path().join("demo-plugin")).expect("plugin root"); let plugin_data_root = AbsolutePathBuf::try_from(temp.path().join("plugin-data")).expect("plugin data root"); + fs::create_dir_all(&plugin_root).expect("create plugin directory"); let source_path = plugin_root.join("hooks/hooks.json"); let plugin_id = PluginId::parse("demo-plugin@test-marketplace").expect("plugin id"); let plugin_hook_sources = vec![PluginHookSource { @@ -2320,6 +2349,7 @@ fn executor_stop_hook_fixture() -> ( engine.handlers.as_slice(), &[ConfiguredHandler { builtin: true, + plugin_trust: None, event_name: HookEventName::Stop, matcher: None, timeout_sec: 5, @@ -2409,6 +2439,7 @@ async fn executor_stop_hooks_run_unless_regular_hooks_block_without_stopping() { Arc::make_mut(&mut engine.handlers).push(ConfiguredHandler { builtin: false, + plugin_trust: None, event_name: HookEventName::Stop, matcher: None, timeout_sec: 30, @@ -2444,6 +2475,7 @@ async fn executor_stop_hooks_run_unless_regular_hooks_block_without_stopping() { Arc::make_mut(&mut engine.handlers).push(ConfiguredHandler { builtin: false, + plugin_trust: None, event_name: HookEventName::Stop, matcher: None, timeout_sec: 30, diff --git a/codex-rs/hooks/src/engine/plugin_fingerprint.rs b/codex-rs/hooks/src/engine/plugin_fingerprint.rs new file mode 100644 index 0000000000..6b85cd9ac1 --- /dev/null +++ b/codex-rs/hooks/src/engine/plugin_fingerprint.rs @@ -0,0 +1,91 @@ +//! Best-effort source identity for installed plugins used by hook trust. +//! +//! Hash bundle files, excluding generated Python bytecode, virtual environments, +//! and package metadata. This detects source changes, not tampering inside those +//! runtime artifacts. Cancellation is checked between entries and bounded reads. + +use std::fs; +use std::io; +use std::io::Read; + +use codex_utils_absolute_path::AbsolutePathBuf; +use sha2::Digest; +use sha2::Sha256; + +pub(super) fn plugin_content_hash( + root: &AbsolutePathBuf, + cancelled: &dyn Fn() -> bool, +) -> io::Result { + let check_cancelled = || { + if cancelled() { + Err(io::Error::from(io::ErrorKind::Interrupted)) + } else { + Ok(()) + } + }; + check_cancelled()?; + if !fs::symlink_metadata(root)?.is_dir() { + return Err(io::Error::other("plugin root is not a directory")); + } + let mut pending = vec![root.to_path_buf()]; + let mut hash = Sha256::new(); + hash.update(b"codex-plugin-bundle-v1"); + let mut buffer = [0_u8; 64 * 1024]; + while let Some(path) = pending.pop() { + check_cancelled()?; + let relative_path = path.strip_prefix(root).map_err(io::Error::other)?; + if relative_path + .file_name() + .and_then(std::ffi::OsStr::to_str) + .is_some_and(|name| { + matches!(name, "__pycache__" | ".venv") + || name.ends_with(".pyc") + || name.ends_with(".egg-info") + }) + { + continue; + } + let name = relative_path.as_os_str().as_encoded_bytes(); + hash.update((name.len() as u64).to_le_bytes()); + hash.update(name); + let metadata = fs::symlink_metadata(&path)?; + if metadata.is_dir() { + hash.update(b"directory"); + let mut entries = Vec::new(); + for entry in fs::read_dir(&path)? { + check_cancelled()?; + entries.push(entry?); + } + entries.sort_by_key(fs::DirEntry::file_name); + pending.extend(entries.iter().rev().map(fs::DirEntry::path)); + } else if metadata.is_file() { + hash.update(b"file"); + let length = metadata.len(); + hash.update(length.to_le_bytes()); + // Bound each read to the observed size, detecting growth or truncation + // instead of trusting a partial fingerprint of a changing file. + check_cancelled()?; + let mut file = fs::File::open(&path)?.take(length.saturating_add(1)); + let mut bytes_read = 0_u64; + loop { + check_cancelled()?; + let count = file.read(&mut buffer)?; + if count == 0 { + break; + } + bytes_read += count as u64; + hash.update(&buffer[..count]); + } + if bytes_read != length { + return Err(io::Error::other("plugin file changed while fingerprinting")); + } + } else { + return Err(io::Error::other(format!( + "unsupported plugin entry type: {}", + path.display() + ))); + } + } + check_cancelled()?; + Ok(format!("sha256:{:x}", hash.finalize())) +} diff --git a/codex-rs/hooks/src/engine/plugin_trust.rs b/codex-rs/hooks/src/engine/plugin_trust.rs new file mode 100644 index 0000000000..8fe88effa1 --- /dev/null +++ b/codex-rs/hooks/src/engine/plugin_trust.rs @@ -0,0 +1,115 @@ +//! Verifies plugin contents at execution, including one-time legacy approval migration. +//! +//! Runtime discovery stores declarations and approvals without scanning bundles. Each +//! execution verifies fresh contents; dropping its future cancels the scan. Legacy +//! approvals share one immutable bundle baseline per plugin and config home, across +//! hooks, profiles, versions, and sessions. + +use codex_utils_absolute_path::AbsolutePathBuf; +use std::fs; +use std::io; +use std::io::Write; + +use codex_config::ConfigLayerSource; +use codex_config::ConfigLayerStack; +use sha2::Digest; +use sha2::Sha256; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct PluginHookTrust { + pub(super) root: AbsolutePathBuf, + pub(super) declaration_hash: String, + pub(super) trusted_hash: String, + pub(super) trusted_content_hash: Option, + pub(super) legacy_path: Option, +} + +impl PluginHookTrust { + pub(super) async fn verify(&self) -> io::Result<()> { + let trust = self.clone(); + // Receiver lifetime propagates cancellation even to a queued blocking job. + let (cancelled, _cancel_on_drop) = tokio::sync::oneshot::channel::<()>(); + tokio::task::spawn_blocking(move || { + let content_hash = + super::plugin_fingerprint::plugin_content_hash(&trust.root, &|| { + cancelled.is_closed() + })?; + if cancelled.is_closed() { + return Err(io::ErrorKind::Interrupted.into()); + } + if !trust.matches_hash(&content_hash)? { + return Err(io::Error::other( + "plugin contents changed; hook approval required", + )); + } + Ok(()) + }) + .await + .map_err(io::Error::other)? + } + + pub(super) fn matches_hash(&self, content_hash: &str) -> io::Result { + if self.trusted_hash != self.declaration_hash { + return Ok(false); + } + if let Some(expected) = &self.trusted_content_hash { + if expected != content_hash { + return Ok(false); + } + if let Some(path) = &self.legacy_path { + // A modern approval also pins the baseline for legacy siblings. + migrate_legacy_trust(path, content_hash)?; + } + return Ok(true); + } + if let Some(path) = &self.legacy_path { + return Ok(migrate_legacy_trust(path, content_hash)? == content_hash); + } + Ok(false) + } +} + +pub(super) fn legacy_trust_path( + stack: &ConfigLayerStack, + plugin_id: &str, +) -> Option { + let config_file = stack.all_layers_low_to_high().find_map(|layer| { + if let ConfigLayerSource::User { file, .. } = &layer.name { + Some(file) + } else { + None + } + }); + let config_file = config_file?; + let parent = config_file.parent()?; + let mut identity = Sha256::new(); + for part in [ + config_file.as_os_str().as_encoded_bytes(), + plugin_id.as_bytes(), + ] { + identity.update((part.len() as u64).to_le_bytes()); + identity.update(part); + } + let directory = parent.join(".hook-trust"); + Some(directory.join(format!("{:x}", identity.finalize()))) +} + +fn migrate_legacy_trust(path: &AbsolutePathBuf, current_hash: &str) -> io::Result { + match fs::read_to_string(path) { + Ok(baseline) => return Ok(baseline), + Err(err) if err.kind() == io::ErrorKind::NotFound => {} + Err(err) => return Err(err), + } + let directory = path + .parent() + .ok_or_else(|| io::Error::other("hook trust baseline has no parent"))?; + fs::create_dir_all(directory.as_path())?; + let mut temporary = tempfile::NamedTempFile::new_in(directory)?; + temporary.write_all(current_hash.as_bytes())?; + temporary.as_file().sync_all()?; + match temporary.persist_noclobber(path) { + Ok(_) => Ok(current_hash.to_owned()), + Err(err) if err.error.kind() == io::ErrorKind::AlreadyExists => fs::read_to_string(path), + Err(err) => Err(err.error), + } +} diff --git a/codex-rs/hooks/src/events/compact.rs b/codex-rs/hooks/src/events/compact.rs index 2509baae46..cdc9846e7d 100644 --- a/codex-rs/hooks/src/events/compact.rs +++ b/codex-rs/hooks/src/events/compact.rs @@ -524,6 +524,7 @@ mod tests { fn handler(event_name: HookEventName) -> ConfiguredHandler { ConfiguredHandler { builtin: false, + plugin_trust: None, event_name, matcher: None, timeout_sec: 5, diff --git a/codex-rs/hooks/src/events/interrupt_tests.rs b/codex-rs/hooks/src/events/interrupt_tests.rs index 7ce3822126..9c1b450fdd 100644 --- a/codex-rs/hooks/src/events/interrupt_tests.rs +++ b/codex-rs/hooks/src/events/interrupt_tests.rs @@ -99,6 +99,7 @@ fn error(text: &str) -> HookOutputEntry { fn handler() -> ConfiguredHandler { ConfiguredHandler { builtin: false, + plugin_trust: None, event_name: HookEventName::Interrupt, matcher: None, timeout_sec: 600, diff --git a/codex-rs/hooks/src/events/post_tool_use.rs b/codex-rs/hooks/src/events/post_tool_use.rs index 1c62d364d1..820659211c 100644 --- a/codex-rs/hooks/src/events/post_tool_use.rs +++ b/codex-rs/hooks/src/events/post_tool_use.rs @@ -593,6 +593,7 @@ mod tests { fn handler_with_async(r#async: bool) -> ConfiguredHandler { ConfiguredHandler { builtin: false, + plugin_trust: None, event_name: HookEventName::PostToolUse, matcher: Some(crate::engine::HookMatcher::new("^Bash$").expect("valid matcher")), timeout_sec: 5, diff --git a/codex-rs/hooks/src/events/pre_tool_use.rs b/codex-rs/hooks/src/events/pre_tool_use.rs index a47b1ff2b0..7613c15b87 100644 --- a/codex-rs/hooks/src/events/pre_tool_use.rs +++ b/codex-rs/hooks/src/events/pre_tool_use.rs @@ -775,6 +775,7 @@ mod tests { fn handler_with_async(r#async: bool) -> ConfiguredHandler { ConfiguredHandler { builtin: false, + plugin_trust: None, event_name: HookEventName::PreToolUse, matcher: Some(crate::engine::HookMatcher::new("^Bash$").expect("valid matcher")), timeout_sec: 5, diff --git a/codex-rs/hooks/src/events/session_end_tests.rs b/codex-rs/hooks/src/events/session_end_tests.rs index 2a1ada4a78..b0e8267c58 100644 --- a/codex-rs/hooks/src/events/session_end_tests.rs +++ b/codex-rs/hooks/src/events/session_end_tests.rs @@ -62,6 +62,7 @@ fn session_end_ignores_successful_output() { fn handler(matcher: Option<&str>) -> ConfiguredHandler { ConfiguredHandler { builtin: false, + plugin_trust: None, event_name: HookEventName::SessionEnd, matcher: matcher .map(|pattern| crate::engine::HookMatcher::new(pattern).expect("valid matcher")), diff --git a/codex-rs/hooks/src/events/session_start.rs b/codex-rs/hooks/src/events/session_start.rs index 79ea63f335..df5ab4f923 100644 --- a/codex-rs/hooks/src/events/session_start.rs +++ b/codex-rs/hooks/src/events/session_start.rs @@ -545,6 +545,7 @@ mod tests { fn handler_for(event_name: HookEventName) -> ConfiguredHandler { ConfiguredHandler { builtin: false, + plugin_trust: None, event_name, matcher: None, timeout_sec: 600, diff --git a/codex-rs/hooks/src/events/stop.rs b/codex-rs/hooks/src/events/stop.rs index 5e2b300153..cf981707b9 100644 --- a/codex-rs/hooks/src/events/stop.rs +++ b/codex-rs/hooks/src/events/stop.rs @@ -696,6 +696,7 @@ mod tests { fn handler_with_async(r#async: bool) -> ConfiguredHandler { ConfiguredHandler { builtin: false, + plugin_trust: None, event_name: HookEventName::Stop, matcher: None, timeout_sec: 600, diff --git a/codex-rs/hooks/src/events/user_prompt_submit.rs b/codex-rs/hooks/src/events/user_prompt_submit.rs index bc856b251a..3a44a03457 100644 --- a/codex-rs/hooks/src/events/user_prompt_submit.rs +++ b/codex-rs/hooks/src/events/user_prompt_submit.rs @@ -464,6 +464,7 @@ mod tests { fn handler_with_async(r#async: bool) -> ConfiguredHandler { ConfiguredHandler { builtin: false, + plugin_trust: None, event_name: HookEventName::UserPromptSubmit, matcher: None, timeout_sec: 5, diff --git a/codex-rs/hooks/src/registry.rs b/codex-rs/hooks/src/registry.rs index b8703bf9c9..2a5e1ec1d1 100644 --- a/codex-rs/hooks/src/registry.rs +++ b/codex-rs/hooks/src/registry.rs @@ -311,6 +311,7 @@ pub fn list_hooks(config: HooksConfig) -> HookListOutcome { config.plugin_hook_sources, config.plugin_hook_load_warnings, config.bypass_hook_trust, + crate::engine::discovery::DiscoveryMode::Listing, ); HookListOutcome { hooks: discovered.hook_entries,