diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index dc29a45d4c..7356d60bde 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -2408,6 +2408,69 @@ fn map_file_change_approval_decision( } } +fn map_command_execution_approval_decision( + decision: CommandExecutionApprovalDecision, + allow_override_command: bool, +) -> (ReviewDecision, Option) { + match decision { + CommandExecutionApprovalDecision::Accept => (ReviewDecision::Approved, None), + CommandExecutionApprovalDecision::AcceptWithOverrideCommand { command } => { + if !allow_override_command { + error!( + "failed to deserialize CommandExecutionRequestApprovalResponse: override command is not allowed for network-only approvals" + ); + ( + ReviewDecision::Denied, + Some(CommandExecutionStatus::Declined), + ) + } else if command.is_empty() { + error!( + "failed to deserialize CommandExecutionRequestApprovalResponse: override command cannot be empty" + ); + ( + ReviewDecision::Denied, + Some(CommandExecutionStatus::Declined), + ) + } else { + (ReviewDecision::ApprovedOverrideCommand { command }, None) + } + } + CommandExecutionApprovalDecision::AcceptForSession => { + (ReviewDecision::ApprovedForSession, None) + } + CommandExecutionApprovalDecision::AcceptWithExecpolicyAmendment { + execpolicy_amendment, + } => ( + ReviewDecision::ApprovedExecpolicyAmendment { + proposed_execpolicy_amendment: execpolicy_amendment.into_core(), + }, + None, + ), + CommandExecutionApprovalDecision::ApplyNetworkPolicyAmendment { + network_policy_amendment, + } => { + let completion_status = match network_policy_amendment.action { + V2NetworkPolicyRuleAction::Allow => None, + V2NetworkPolicyRuleAction::Deny => Some(CommandExecutionStatus::Declined), + }; + ( + ReviewDecision::NetworkPolicyAmendment { + network_policy_amendment: network_policy_amendment.into_core(), + }, + completion_status, + ) + } + CommandExecutionApprovalDecision::Decline => ( + ReviewDecision::Denied, + Some(CommandExecutionStatus::Declined), + ), + CommandExecutionApprovalDecision::Cancel => ( + ReviewDecision::Abort, + Some(CommandExecutionStatus::Declined), + ), + } +} + #[allow(clippy::too_many_arguments)] async fn on_file_change_request_approval_response( event_turn_id: String, @@ -2502,67 +2565,12 @@ async fn on_command_execution_request_approval_response( } }); - let decision = response.decision; - - let (decision, completion_status) = match decision { - CommandExecutionApprovalDecision::Accept => (ReviewDecision::Approved, None), - CommandExecutionApprovalDecision::AcceptWithOverrideCommand { command } => { - // Network-only approval prompts omit the command, so they - // must not allow clients to steer execution via override. - if completion_item.is_none() { - error!( - "failed to deserialize CommandExecutionRequestApprovalResponse: override command is not allowed for network-only approvals" - ); - ( - ReviewDecision::Denied, - Some(CommandExecutionStatus::Declined), - ) - } else if command.is_empty() { - error!( - "failed to deserialize CommandExecutionRequestApprovalResponse: override command cannot be empty" - ); - ( - ReviewDecision::Denied, - Some(CommandExecutionStatus::Declined), - ) - } else { - (ReviewDecision::ApprovedOverrideCommand { command }, None) - } - } - CommandExecutionApprovalDecision::AcceptForSession => { - (ReviewDecision::ApprovedForSession, None) - } - CommandExecutionApprovalDecision::AcceptWithExecpolicyAmendment { - execpolicy_amendment, - } => ( - ReviewDecision::ApprovedExecpolicyAmendment { - proposed_execpolicy_amendment: execpolicy_amendment.into_core(), - }, - None, - ), - CommandExecutionApprovalDecision::ApplyNetworkPolicyAmendment { - network_policy_amendment, - } => { - let completion_status = match network_policy_amendment.action { - V2NetworkPolicyRuleAction::Allow => None, - V2NetworkPolicyRuleAction::Deny => Some(CommandExecutionStatus::Declined), - }; - ( - ReviewDecision::NetworkPolicyAmendment { - network_policy_amendment: network_policy_amendment.into_core(), - }, - completion_status, - ) - } - CommandExecutionApprovalDecision::Decline => ( - ReviewDecision::Denied, - Some(CommandExecutionStatus::Declined), - ), - CommandExecutionApprovalDecision::Cancel => ( - ReviewDecision::Abort, - Some(CommandExecutionStatus::Declined), - ), - }; + let (decision, completion_status) = map_command_execution_approval_decision( + response.decision, + // Network-only prompts omit the completion item, and they + // must not allow command override responses. + completion_item.is_some(), + ); (decision, completion_status) } Ok(Err(err)) if is_turn_transition_server_request_error(&err) => return, @@ -2907,6 +2915,19 @@ mod tests { assert_eq!(completion_status, None); } + #[test] + fn command_execution_override_is_rejected_for_network_only_prompts() { + let (decision, completion_status) = map_command_execution_approval_decision( + CommandExecutionApprovalDecision::AcceptWithOverrideCommand { + command: vec!["echo".to_string(), "hi".to_string()], + }, + false, + ); + + assert_eq!(decision, ReviewDecision::Denied); + assert_eq!(completion_status, Some(CommandExecutionStatus::Declined)); + } + #[test] fn mcp_server_elicitation_turn_transition_error_maps_to_cancel() { let error = JSONRPCErrorError { diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 2193cb7ebf..aca77680bf 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -36,6 +36,7 @@ use crate::tools::sandboxing::sandbox_override_for_first_attempt; use crate::tools::sandboxing::with_cached_approval; use codex_network_proxy::NetworkProxy; use codex_protocol::models::PermissionProfile; +use codex_protocol::protocol::ExecCommandSource; use codex_protocol::protocol::ReviewDecision; use futures::future::BoxFuture; use std::collections::HashMap; diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index 7a9dba038e..fe38a5f9f9 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -34,6 +34,7 @@ use core_test_support::skip_if_no_network; use core_test_support::test_codex::TestCodex; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_event; +use core_test_support::wait_for_event_match; use core_test_support::wait_for_event_with_timeout; use core_test_support::zsh_fork::build_zsh_fork_test; use core_test_support::zsh_fork::restrictive_workspace_write_policy; @@ -1815,6 +1816,102 @@ async fn approving_apply_patch_for_session_skips_future_prompts_for_same_file() Ok(()) } +#[tokio::test(flavor = "current_thread")] +#[cfg(unix)] +async fn approved_override_command_updates_exec_events() -> Result<()> { + let server = start_mock_server().await; + let approval_policy = AskForApproval::OnRequest; + let sandbox_policy = SandboxPolicy::new_read_only_policy(); + let sandbox_policy_for_config = sandbox_policy.clone(); + let mut builder = test_codex().with_config(move |config| { + config.permissions.approval_policy = Constrained::allow_any(approval_policy); + config.permissions.sandbox_policy = Constrained::allow_any(sandbox_policy_for_config); + }); + let test = builder.build(&server).await?; + + let call_id = "override-command-events"; + let original_command = r#"cat < "hello" > /var/test.txt"#; + let override_payload = "printf 'override-output'"; + let (event, expected_command) = ActionKind::RunCommand { + command: original_command, + } + .prepare( + &test, + &server, + call_id, + SandboxPermissions::RequireEscalated, + ) + .await?; + let expected_command = + expected_command.expect("override command scenario should produce a shell command"); + + let _ = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-override-command-1"), + event, + ev_completed("resp-override-command-1"), + ]), + ) + .await; + let results_mock = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-override-command-1", "done"), + ev_completed("resp-override-command-2"), + ]), + ) + .await; + + submit_turn( + &test, + "override-command-events", + approval_policy, + sandbox_policy.clone(), + ) + .await?; + + let approval = expect_exec_approval(&test, expected_command.as_str()).await; + let mut override_command = approval.command.clone(); + *override_command + .last_mut() + .expect("shell approval command should include the shell payload") = + override_payload.to_string(); + + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::ApprovedOverrideCommand { + command: override_command.clone(), + }, + }) + .await?; + + let begin_event = wait_for_event_match(&test.codex, |event| match event { + EventMsg::ExecCommandBegin(ev) if ev.call_id == call_id => Some(ev.clone()), + _ => None, + }) + .await; + let end_event = wait_for_event_match(&test.codex, |event| match event { + EventMsg::ExecCommandEnd(ev) if ev.call_id == call_id => Some(ev.clone()), + _ => None, + }) + .await; + + wait_for_completion(&test).await; + + assert_eq!(begin_event.command, override_command); + assert_eq!(end_event.command, override_command); + assert_eq!(end_event.stdout, "override-output"); + + let output_item = results_mock.single_request().function_call_output(call_id); + let result = parse_result(&output_item); + assert_eq!(result.stdout, "override-output"); + + Ok(()) +} + #[tokio::test(flavor = "current_thread")] #[cfg(unix)] async fn approving_execpolicy_amendment_persists_policy_and_skips_future_prompts() -> Result<()> { diff --git a/codex-rs/protocol/src/approvals.rs b/codex-rs/protocol/src/approvals.rs index 9a379a82c4..82f5036c58 100644 --- a/codex-rs/protocol/src/approvals.rs +++ b/codex-rs/protocol/src/approvals.rs @@ -330,3 +330,66 @@ pub struct ApplyPatchApprovalRequestEvent { #[serde(skip_serializing_if = "Option::is_none")] pub grant_root: Option, } + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + #[test] + fn command_prompts_advertise_override_decision() { + let command = vec!["echo".to_string(), "hi".to_string()]; + + let decisions = + ExecApprovalRequestEvent::default_available_decisions(&command, None, None, None, None); + + assert_eq!( + decisions, + vec![ + ReviewDecision::Approved, + ReviewDecision::ApprovedOverrideCommand { + command: command.clone(), + }, + ReviewDecision::Abort, + ] + ); + } + + #[test] + fn network_only_prompts_do_not_advertise_override_decision() { + let decisions = ExecApprovalRequestEvent::default_available_decisions( + &["echo".to_string(), "hi".to_string()], + Some(&NetworkApprovalContext { + host: "example.com".to_string(), + protocol: NetworkApprovalProtocol::Https, + }), + None, + Some(&[ + NetworkPolicyAmendment { + host: "example.com".to_string(), + action: NetworkPolicyRuleAction::Allow, + }, + NetworkPolicyAmendment { + host: "example.com".to_string(), + action: NetworkPolicyRuleAction::Deny, + }, + ]), + None, + ); + + assert_eq!( + decisions, + vec![ + ReviewDecision::Approved, + ReviewDecision::ApprovedForSession, + ReviewDecision::NetworkPolicyAmendment { + network_policy_amendment: NetworkPolicyAmendment { + host: "example.com".to_string(), + action: NetworkPolicyRuleAction::Allow, + }, + }, + ReviewDecision::Abort, + ] + ); + } +}