From db45ef91863e74c09880a18753153a52bb12eead Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Mon, 13 Apr 2026 21:01:37 -0700 Subject: [PATCH] Gate hook permission suggestions on available approval decisions Co-authored-by: Codex --- codex-rs/core/src/tools/runtimes/shell.rs | 8 +-- .../core/src/tools/runtimes/unified_exec.rs | 8 +-- codex-rs/core/src/tools/sandboxing.rs | 25 +++++++++ codex-rs/core/src/tools/sandboxing_tests.rs | 51 ++++++++++++++++++- 4 files changed, 85 insertions(+), 7 deletions(-) diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 155a4ff766..00de58d3d7 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -30,7 +30,7 @@ use crate::tools::sandboxing::Sandboxable; use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; -use crate::tools::sandboxing::exec_policy_permission_suggestions; +use crate::tools::sandboxing::approval_permission_suggestions; use crate::tools::sandboxing::sandbox_override_for_first_attempt; use crate::tools::sandboxing::with_cached_approval; use codex_hooks::PermissionSuggestionDestination; @@ -204,11 +204,13 @@ impl Approvable for ShellRuntime { fn permission_request_payload( &self, req: &ShellRequest, - _approval_ctx: &ApprovalCtx<'_>, + approval_ctx: &ApprovalCtx<'_>, ) -> Option { - let permission_suggestions = exec_policy_permission_suggestions( + let permission_suggestions = approval_permission_suggestions( + approval_ctx.network_approval_context.as_ref(), req.exec_approval_requirement .proposed_execpolicy_amendment(), + req.additional_permissions.as_ref(), &[PermissionSuggestionDestination::UserSettings], ); Some(PermissionRequestPayload { diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 99ea14a5a5..0b936fb40a 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -27,7 +27,7 @@ use crate::tools::sandboxing::Sandboxable; use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; -use crate::tools::sandboxing::exec_policy_permission_suggestions; +use crate::tools::sandboxing::approval_permission_suggestions; use crate::tools::sandboxing::sandbox_override_for_first_attempt; use crate::tools::sandboxing::with_cached_approval; use crate::unified_exec::NoopSpawnLifecycle; @@ -184,11 +184,13 @@ impl Approvable for UnifiedExecRuntime<'_> { fn permission_request_payload( &self, req: &UnifiedExecRequest, - _approval_ctx: &ApprovalCtx<'_>, + approval_ctx: &ApprovalCtx<'_>, ) -> Option { - let permission_suggestions = exec_policy_permission_suggestions( + let permission_suggestions = approval_permission_suggestions( + approval_ctx.network_approval_context.as_ref(), req.exec_approval_requirement .proposed_execpolicy_amendment(), + req.additional_permissions.as_ref(), &[PermissionSuggestionDestination::UserSettings], ); Some(PermissionRequestPayload { diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 2e5e37f95b..920c6514fd 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -19,10 +19,12 @@ use codex_network_proxy::NetworkProxy; use codex_protocol::approvals::ExecPolicyAmendment; use codex_protocol::approvals::NetworkApprovalContext; use codex_protocol::error::CodexErr; +use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::FileSystemSandboxKind; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::ExecApprovalRequestEvent; use codex_protocol::protocol::ReviewDecision; #[cfg(test)] use codex_protocol::protocol::SandboxPolicy; @@ -166,6 +168,29 @@ pub(crate) fn exec_policy_permission_suggestions( .collect() } +pub(crate) fn approval_permission_suggestions( + network_approval_context: Option<&NetworkApprovalContext>, + proposed_execpolicy_amendment: Option<&ExecPolicyAmendment>, + additional_permissions: Option<&PermissionProfile>, + destinations: &[PermissionSuggestionDestination], +) -> Vec { + let available_decisions = ExecApprovalRequestEvent::default_available_decisions( + network_approval_context, + proposed_execpolicy_amendment, + /*proposed_network_policy_amendments*/ None, + additional_permissions, + ); + let allows_execpolicy_amendment = available_decisions + .iter() + .any(|decision| matches!(decision, ReviewDecision::ApprovedExecpolicyAmendment { .. })); + + if allows_execpolicy_amendment { + exec_policy_permission_suggestions(proposed_execpolicy_amendment, destinations) + } else { + Vec::new() + } +} + // Specifies what tool orchestrator should do with a given tool call. #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) enum ExecApprovalRequirement { diff --git a/codex-rs/core/src/tools/sandboxing_tests.rs b/codex-rs/core/src/tools/sandboxing_tests.rs index f5ed4d1850..24094bd4c6 100644 --- a/codex-rs/core/src/tools/sandboxing_tests.rs +++ b/codex-rs/core/src/tools/sandboxing_tests.rs @@ -5,8 +5,13 @@ use codex_hooks::PermissionSuggestionDestination; use codex_hooks::PermissionSuggestionRule; use codex_hooks::PermissionSuggestionType; use codex_protocol::approvals::ExecPolicyAmendment; +use codex_protocol::approvals::NetworkApprovalContext; +use codex_protocol::approvals::NetworkApprovalProtocol; +use codex_protocol::models::FileSystemPermissions; +use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::GranularApprovalConfig; use codex_protocol::protocol::NetworkAccess; +use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; #[test] @@ -116,12 +121,14 @@ fn guardian_bypasses_sandbox_for_explicit_escalation_on_first_attempt() { #[test] fn command_approval_execpolicy_amendment_maps_to_user_settings_suggestion() { - let suggestions = exec_policy_permission_suggestions( + let suggestions = approval_permission_suggestions( + /*network_approval_context*/ None, Some(&ExecPolicyAmendment::new(vec![ "rm".to_string(), "-rf".to_string(), "node_modules".to_string(), ])), + /*additional_permissions*/ None, &[PermissionSuggestionDestination::UserSettings], ); @@ -141,3 +148,45 @@ fn command_approval_execpolicy_amendment_maps_to_user_settings_suggestion() { }] ); } + +#[test] +fn command_approval_with_additional_permissions_has_no_persistent_suggestions() { + let suggestions = approval_permission_suggestions( + /*network_approval_context*/ None, + Some(&ExecPolicyAmendment::new(vec![ + "cat".to_string(), + "/tmp/secret".to_string(), + ])), + Some(&PermissionProfile { + network: None, + file_system: Some(FileSystemPermissions { + read: Some(vec![ + AbsolutePathBuf::from_absolute_path("/tmp/secret") + .expect("/tmp/secret should be an absolute path"), + ]), + write: None, + }), + }), + &[PermissionSuggestionDestination::UserSettings], + ); + + assert_eq!(suggestions, Vec::::new()); +} + +#[test] +fn network_approval_with_execpolicy_amendment_has_no_persistent_suggestions() { + let suggestions = approval_permission_suggestions( + Some(&NetworkApprovalContext { + host: "example.com".to_string(), + protocol: NetworkApprovalProtocol::Https, + }), + Some(&ExecPolicyAmendment::new(vec![ + "curl".to_string(), + "https://example.com".to_string(), + ])), + /*additional_permissions*/ None, + &[PermissionSuggestionDestination::UserSettings], + ); + + assert_eq!(suggestions, Vec::::new()); +}