From 7e31d1883dbf96e47a0d131ffaa2baf12605be75 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 5 Jun 2026 09:01:10 -0700 Subject: [PATCH] fix: honor parent approvals for intercepted execs --- .../tools/runtimes/shell/unix_escalation.rs | 38 ++- .../runtimes/shell/unix_escalation_tests.rs | 127 +++++++++ codex-rs/core/tests/suite/approvals.rs | 268 ++++++++++++++++++ 3 files changed, 432 insertions(+), 1 deletion(-) diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index 9bc8d117f7..3530174877 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -17,6 +17,7 @@ use crate::shell::ShellType; use crate::tools::runtimes::build_sandbox_command; use crate::tools::runtimes::exec_env_for_sandbox_permissions; use crate::tools::runtimes::prepend_zsh_fork_bin_to_path; +use crate::tools::sandboxing::ExecApprovalRequirement; use crate::tools::sandboxing::PermissionRequestPayload; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::ToolCtx; @@ -100,6 +101,19 @@ fn approval_sandbox_permissions( } } +fn parent_approved_sandbox_override( + sandbox_permissions: SandboxPermissions, + additional_permissions_preapproved: bool, + exec_approval_requirement: &ExecApprovalRequirement, +) -> bool { + sandbox_permissions.requests_sandbox_override() + && (additional_permissions_preapproved + || matches!( + exec_approval_requirement, + ExecApprovalRequirement::NeedsApproval { .. } + )) +} + pub(super) async fn try_run_zsh_fork( req: &ShellRequest, attempt: &SandboxAttempt<'_>, @@ -228,6 +242,11 @@ pub(super) async fn try_run_zsh_fork( sandbox_permissions: req.sandbox_permissions, approval_sandbox_permissions, prompt_permissions: req.additional_permissions.clone(), + parent_sandbox_override_approved: parent_approved_sandbox_override( + req.sandbox_permissions, + req.additional_permissions_preapproved, + &req.exec_approval_requirement, + ), stopwatch: stopwatch.clone(), }; @@ -304,6 +323,11 @@ pub(crate) async fn prepare_unified_exec_zsh_fork( req.additional_permissions_preapproved, ), prompt_permissions: req.additional_permissions.clone(), + parent_sandbox_override_approved: parent_approved_sandbox_override( + req.sandbox_permissions, + req.additional_permissions_preapproved, + &req.exec_approval_requirement, + ), stopwatch: Stopwatch::unlimited(), }; @@ -336,6 +360,7 @@ struct CoreShellActionProvider { sandbox_permissions: SandboxPermissions, approval_sandbox_permissions: SandboxPermissions, prompt_permissions: Option, + parent_sandbox_override_approved: bool, stopwatch: Stopwatch, } @@ -639,6 +664,17 @@ impl EscalationPolicy for CoreShellActionProvider { SandboxPermissions::RequireEscalated => unsandboxed_allowed, SandboxPermissions::WithAdditionalPermissions => true, }; + // The parent shell/unified-exec approval already covered unmatched + // prompts caused by its sandbox override. Explicit exec-policy rules + // still keep their own decisions. + let decision = if self.parent_sandbox_override_approved + && evaluation.decision == Decision::Prompt + && !decision_driven_by_policy + { + Decision::Allow + } else { + evaluation.decision + }; let decision_source = if decision_driven_by_policy { DecisionSource::PrefixRule @@ -656,7 +692,7 @@ impl EscalationPolicy for CoreShellActionProvider { ), }; self.process_decision( - evaluation.decision, + decision, needs_escalation, program, argv, diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs index 87e0854176..84066e0928 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs @@ -153,6 +153,42 @@ fn approval_sandbox_permissions_only_downgrades_preapproved_additional_permissio ); } +#[test] +fn parent_approved_sandbox_override_tracks_parent_approval_sources() { + assert!(super::parent_approved_sandbox_override( + SandboxPermissions::RequireEscalated, + /*additional_permissions_preapproved*/ false, + &crate::tools::sandboxing::ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: None, + }, + )); + assert!(super::parent_approved_sandbox_override( + SandboxPermissions::WithAdditionalPermissions, + /*additional_permissions_preapproved*/ true, + &crate::tools::sandboxing::ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, + }, + )); + assert!(!super::parent_approved_sandbox_override( + SandboxPermissions::UseDefault, + /*additional_permissions_preapproved*/ false, + &crate::tools::sandboxing::ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: None, + }, + )); + assert!(!super::parent_approved_sandbox_override( + SandboxPermissions::RequireEscalated, + /*additional_permissions_preapproved*/ false, + &crate::tools::sandboxing::ExecApprovalRequirement::Skip { + bypass_sandbox: true, + proposed_execpolicy_amendment: None, + }, + )); +} + #[test] fn extract_shell_script_preserves_login_flag() { assert_eq!( @@ -428,6 +464,7 @@ async fn preapproved_additional_permissions_escalate_intercepted_exec() -> anyho sandbox_permissions: SandboxPermissions::WithAdditionalPermissions, approval_sandbox_permissions: SandboxPermissions::UseDefault, prompt_permissions: Some(requested_permissions), + parent_sandbox_override_approved: true, stopwatch: codex_shell_escalation::Stopwatch::new(Duration::from_secs(1)), }; @@ -449,6 +486,93 @@ async fn preapproved_additional_permissions_escalate_intercepted_exec() -> anyho Ok(()) } +#[tokio::test] +async fn parent_require_escalated_approval_escalates_intercepted_exec() -> anyhow::Result<()> { + let (session, turn_context) = make_session_and_context().await; + let workdir = test_sandbox_cwd(); + let provider = CoreShellActionProvider { + policy: Arc::new(RwLock::new(codex_execpolicy::Policy::empty())), + session: Arc::new(session), + turn: Arc::new(turn_context), + call_id: "parent-require-escalated".to_string(), + tool_name: GuardianCommandSource::Shell, + approval_policy: AskForApproval::OnRequest, + permission_profile: PermissionProfile::workspace_write(), + file_system_sandbox_policy: read_only_file_system_sandbox_policy(), + sandbox_policy_cwd: workdir.clone(), + sandbox_permissions: SandboxPermissions::RequireEscalated, + approval_sandbox_permissions: SandboxPermissions::RequireEscalated, + prompt_permissions: None, + parent_sandbox_override_approved: true, + stopwatch: codex_shell_escalation::Stopwatch::new(Duration::from_secs(1)), + }; + + let action = codex_shell_escalation::EscalationPolicy::determine_action( + &provider, + &AbsolutePathBuf::from_absolute_path("/usr/bin/curl")?, + &["curl".to_string(), "example.com".to_string()], + &workdir, + ) + .await?; + + assert_eq!( + action, + codex_shell_escalation::EscalationDecision::Escalate(EscalationExecution::Unsandboxed) + ); + + Ok(()) +} + +#[tokio::test] +async fn parent_approval_does_not_override_intercepted_exec_policy_prompt() -> anyhow::Result<()> { + let (session, turn_context) = make_session_and_context().await; + let mut parser = PolicyParser::new(); + parser.parse( + "test.rules", + r#"prefix_rule(pattern = ["curl"], decision = "prompt")"#, + )?; + let workdir = test_sandbox_cwd(); + let provider = CoreShellActionProvider { + policy: Arc::new(RwLock::new(parser.build())), + session: Arc::new(session), + turn: Arc::new(turn_context), + call_id: "parent-policy-prompt".to_string(), + tool_name: GuardianCommandSource::Shell, + approval_policy: AskForApproval::Granular(GranularApprovalConfig { + sandbox_approval: true, + rules: false, + skill_approval: true, + request_permissions: true, + mcp_elicitations: true, + }), + permission_profile: PermissionProfile::workspace_write(), + file_system_sandbox_policy: read_only_file_system_sandbox_policy(), + sandbox_policy_cwd: workdir.clone(), + sandbox_permissions: SandboxPermissions::RequireEscalated, + approval_sandbox_permissions: SandboxPermissions::RequireEscalated, + prompt_permissions: None, + parent_sandbox_override_approved: true, + stopwatch: codex_shell_escalation::Stopwatch::new(Duration::from_secs(1)), + }; + + let action = codex_shell_escalation::EscalationPolicy::determine_action( + &provider, + &AbsolutePathBuf::from_absolute_path("/usr/bin/curl")?, + &["curl".to_string(), "example.com".to_string()], + &workdir, + ) + .await?; + + assert_eq!( + action, + codex_shell_escalation::EscalationDecision::Deny { + reason: Some("Execution forbidden by policy".to_string()), + } + ); + + Ok(()) +} + #[tokio::test(flavor = "current_thread")] async fn execve_permission_request_hook_short_circuits_prompt() -> anyhow::Result<()> { let (session, mut turn_context) = make_session_and_context().await; @@ -561,6 +685,7 @@ async fn execve_permission_request_hook_short_circuits_prompt() -> anyhow::Resul sandbox_permissions: SandboxPermissions::RequireEscalated, approval_sandbox_permissions: SandboxPermissions::RequireEscalated, prompt_permissions: None, + parent_sandbox_override_approved: false, stopwatch: codex_shell_escalation::Stopwatch::new(Duration::from_secs(1)), }; @@ -775,6 +900,7 @@ prefix_rule(pattern = ["{cat_path_literal}"], decision = "allow") sandbox_permissions: SandboxPermissions::UseDefault, approval_sandbox_permissions: SandboxPermissions::UseDefault, prompt_permissions: None, + parent_sandbox_override_approved: false, stopwatch: codex_shell_escalation::Stopwatch::new(Duration::from_secs(1)), }; @@ -818,6 +944,7 @@ async fn denied_reads_keep_granular_sandbox_rejection_for_escalation() -> anyhow sandbox_permissions: SandboxPermissions::RequireEscalated, approval_sandbox_permissions: SandboxPermissions::RequireEscalated, prompt_permissions: None, + parent_sandbox_override_approved: false, stopwatch: codex_shell_escalation::Stopwatch::new(Duration::from_secs(1)), }; diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index 96ac47cf45..0221d3ccdb 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -2942,6 +2942,274 @@ async fn unified_exec_zsh_fork_parent_approval_preserves_denied_reads() -> Resul Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[cfg(unix)] +async fn unified_exec_zsh_fork_parent_approval_escalates_intercepted_exec() -> Result<()> { + skip_if_no_network!(Ok(())); + + let Some(runtime) = zsh_fork_runtime("unified-exec zsh-fork parent approval test")? else { + return Ok(()); + }; + + let approval_policy = AskForApproval::OnRequest; + let permission_profile = restrictive_workspace_write_profile(); + let outside_dir = tempfile::tempdir_in(std::env::current_dir()?)?; + let outside_path = outside_dir + .path() + .join("unified-exec-zsh-fork-parent-approval.txt"); + let command = format!("printf hi > {outside_path:?}"); + + let server = start_mock_server().await; + let outside_path_for_hook = outside_path.clone(); + let test = build_unified_exec_zsh_fork_test( + &server, + runtime, + approval_policy, + permission_profile, + move |_home| { + let _ = fs::remove_file(&outside_path_for_hook); + }, + ) + .await?; + + let call_id = "uexec-zsh-fork-parent-approval"; + let event = exec_command_event( + call_id, + &command, + Some(30_000), + SandboxPermissions::RequireEscalated, + Some("write outside the workspace for the test"), + )?; + let _ = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-uexec-zsh-fork-parent-approval-1"), + event, + ev_completed("resp-uexec-zsh-fork-parent-approval-1"), + ]), + ) + .await; + let results = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-uexec-zsh-fork-parent-approval-1", "done"), + ev_completed("resp-uexec-zsh-fork-parent-approval-2"), + ]), + ) + .await; + + let session_model = test.session_configured.model.clone(); + let (sandbox_policy, permission_profile) = turn_permission_fields( + test.session_configured.permission_profile.clone(), + test.cwd.path(), + ); + test.codex + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: "run approved unified exec through zsh fork".into(), + text_elements: Vec::new(), + }], + environments: None, + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: codex_protocol::protocol::ThreadSettingsOverrides { + cwd: Some(test.cwd.path().to_path_buf()), + approval_policy: Some(approval_policy), + approvals_reviewer: Some(ApprovalsReviewer::User), + sandbox_policy: Some(sandbox_policy), + permission_profile, + collaboration_mode: Some(codex_protocol::config_types::CollaborationMode { + mode: codex_protocol::config_types::ModeKind::Default, + settings: codex_protocol::config_types::Settings { + model: session_model, + reasoning_effort: None, + developer_instructions: None, + }, + }), + ..Default::default() + }, + }) + .await?; + + let approval = expect_exec_approval(&test, &command).await; + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::Approved, + }) + .await?; + wait_for_completion_without_approval(&test).await; + + let result = parse_result(&results.single_request().function_call_output(call_id)); + assert_eq!( + result.exit_code.unwrap_or(0), + 0, + "approved unified exec zsh-fork command should complete: {}", + result.stdout + ); + let contents = fs::read_to_string(&outside_path) + .with_context(|| format!("read {}", outside_path.display()))?; + assert_eq!(contents, "hi"); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[cfg(unix)] +async fn unified_exec_zsh_fork_parent_approval_keeps_explicit_prompt_rule() -> Result<()> { + skip_if_no_network!(Ok(())); + + let Some(runtime) = zsh_fork_runtime("unified-exec zsh-fork prompt rule approval test")? else { + return Ok(()); + }; + + let approval_policy = AskForApproval::OnRequest; + let permission_profile = restrictive_workspace_write_profile(); + let outside_dir = tempfile::tempdir_in(std::env::current_dir()?)?; + let outside_path = outside_dir + .path() + .join("unified-exec-zsh-fork-explicit-prompt-rule.txt"); + let command = format!("touch {outside_path:?}"); + let rules = r#"prefix_rule(pattern=["touch"], decision="prompt")"#.to_string(); + + let server = start_mock_server().await; + let outside_path_for_hook = outside_path.clone(); + let test = build_unified_exec_zsh_fork_test( + &server, + runtime, + approval_policy, + permission_profile, + move |home| { + let _ = fs::remove_file(&outside_path_for_hook); + let rules_dir = home.join("rules"); + fs::create_dir_all(&rules_dir).unwrap(); + fs::write(rules_dir.join("default.rules"), &rules).unwrap(); + }, + ) + .await?; + + let call_id = "uexec-zsh-fork-parent-approval-explicit-prompt-rule"; + let event = exec_command_event( + call_id, + &command, + Some(30_000), + SandboxPermissions::RequireEscalated, + Some("write outside the workspace for the test"), + )?; + let _ = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-uexec-zsh-fork-prompt-rule-1"), + event, + ev_completed("resp-uexec-zsh-fork-prompt-rule-1"), + ]), + ) + .await; + let results = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-uexec-zsh-fork-prompt-rule-1", "done"), + ev_completed("resp-uexec-zsh-fork-prompt-rule-2"), + ]), + ) + .await; + + let session_model = test.session_configured.model.clone(); + let (sandbox_policy, permission_profile) = turn_permission_fields( + test.session_configured.permission_profile.clone(), + test.cwd.path(), + ); + test.codex + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: "run approved unified exec prompt rule through zsh fork".into(), + text_elements: Vec::new(), + }], + environments: None, + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: codex_protocol::protocol::ThreadSettingsOverrides { + cwd: Some(test.cwd.path().to_path_buf()), + approval_policy: Some(approval_policy), + approvals_reviewer: Some(ApprovalsReviewer::User), + sandbox_policy: Some(sandbox_policy), + permission_profile, + collaboration_mode: Some(codex_protocol::config_types::CollaborationMode { + mode: codex_protocol::config_types::ModeKind::Default, + settings: codex_protocol::config_types::Settings { + model: session_model, + reasoning_effort: None, + developer_instructions: None, + }, + }), + ..Default::default() + }, + }) + .await?; + + let parent_approval = expect_exec_approval(&test, &command).await; + test.codex + .submit(Op::ExecApproval { + id: parent_approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::Approved, + }) + .await?; + + let approval_event = wait_for_event_with_timeout( + &test.codex, + |event| { + matches!( + event, + EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_) + ) + }, + Duration::from_secs(10), + ) + .await; + let EventMsg::ExecApprovalRequest(inner_approval) = approval_event else { + panic!("expected explicit prompt rule approval before completion"); + }; + assert!( + inner_approval + .command + .iter() + .any(|arg| arg.ends_with("/touch")) + && inner_approval + .command + .iter() + .any(|arg| arg == outside_path.to_string_lossy().as_ref()), + "expected explicit prompt rule approval for intercepted touch, got: {:?}", + inner_approval.command + ); + + test.codex + .submit(Op::ExecApproval { + id: inner_approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::Approved, + }) + .await?; + wait_for_completion(&test).await; + + let result = parse_result(&results.single_request().function_call_output(call_id)); + assert_eq!( + result.exit_code.unwrap_or(0), + 0, + "approved unified exec zsh-fork prompt-rule command should complete: {}", + result.stdout + ); + assert!( + outside_path.exists(), + "approved intercepted touch should create the out-of-workspace file" + ); + + Ok(()) +} + #[tokio::test(flavor = "current_thread")] #[cfg(unix)] async fn invalid_requested_prefix_rule_falls_back_for_compound_command() -> Result<()> {