From 9f2c3cb5de408911ca3672f8b5f2512d27b4c454 Mon Sep 17 00:00:00 2001 From: Charles Cunningham Date: Fri, 6 Mar 2026 20:39:11 -0800 Subject: [PATCH] split sandbox permission validation modes Co-authored-by: Codex --- codex-rs/core/src/codex_tests.rs | 186 +++++++++++++++++- codex-rs/core/src/tools/handlers/mod.rs | 10 +- codex-rs/core/src/tools/handlers/shell.rs | 13 +- .../core/src/tools/handlers/unified_exec.rs | 10 +- 4 files changed, 197 insertions(+), 22 deletions(-) diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index b5f2ea2ef4..c94b2ebdfd 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -3759,25 +3759,30 @@ async fn sample_rollout( } #[tokio::test] -async fn rejects_escalated_permissions_when_policy_not_on_request() { +async fn rejects_additional_permissions_when_policy_not_on_request() { use crate::exec::ExecParams; + use crate::features::Feature; use crate::protocol::AskForApproval; use crate::protocol::SandboxPolicy; use crate::sandboxing::SandboxPermissions; use crate::turn_diff_tracker::TurnDiffTracker; use std::collections::HashMap; - let (session, mut turn_context_raw) = make_session_and_context().await; + let (mut session, mut turn_context_raw) = make_session_and_context().await; // Ensure policy is NOT OnRequest so the early rejection path triggers turn_context_raw .approval_policy .set(AskForApproval::OnFailure) .expect("test setup should allow updating approval policy"); + session + .features + .enable(Feature::RequestPermissions) + .expect("test setup should allow enabling request permissions"); let session = Arc::new(session); let mut turn_context = Arc::new(turn_context_raw); let timeout_ms = 1000; - let sandbox_permissions = SandboxPermissions::RequireEscalated; + let sandbox_permissions = SandboxPermissions::WithAdditionalPermissions; let params = ExecParams { command: if cfg!(windows) { vec![ @@ -3845,8 +3850,8 @@ async fn rejects_escalated_permissions_when_policy_not_on_request() { }; let expected = format!( - "approval policy is {policy:?}; reject command — you should not ask for escalated permissions if the approval policy is {policy:?}", - policy = turn_context.approval_policy.value() + "approval policy is {policy:?}; reject command — you cannot request additional permissions unless the approval policy is OnRequest", + policy = turn_context.approval_policy.value(), ); pretty_assertions::assert_eq!(output, expected); @@ -3909,7 +3914,126 @@ async fn rejects_escalated_permissions_when_policy_not_on_request() { assert!(exec_output.output.contains("hi")); } #[tokio::test] -async fn unified_exec_rejects_escalated_permissions_when_policy_not_on_request() { +async fn unified_exec_rejects_additional_permissions_when_policy_not_on_request() { + use crate::protocol::AskForApproval; + use crate::sandboxing::SandboxPermissions; + use crate::turn_diff_tracker::TurnDiffTracker; + + let (session, mut turn_context_raw) = make_session_and_context().await; + turn_context_raw + .approval_policy + .set(AskForApproval::OnFailure) + .expect("test setup should allow updating approval policy"); + let session = Arc::new(session); + let turn_context = Arc::new(turn_context_raw); + let tracker = Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new())); + + let handler = UnifiedExecHandler; + let resp = handler + .handle(ToolInvocation { + session: Arc::clone(&session), + turn: Arc::clone(&turn_context), + tracker: Arc::clone(&tracker), + call_id: "exec-call".to_string(), + tool_name: "exec_command".to_string(), + payload: ToolPayload::Function { + arguments: serde_json::json!({ + "cmd": "echo hi", + "sandbox_permissions": SandboxPermissions::WithAdditionalPermissions, + "justification": "need additional sandbox permissions", + }) + .to_string(), + }, + }) + .await; + + let Err(FunctionCallError::RespondToModel(output)) = resp else { + panic!("expected error result"); + }; + + let expected = format!( + "approval policy is {policy:?}; reject command — you cannot ask for additional sandbox permissions if the approval policy is not OnRequest", + policy = turn_context.approval_policy.value() + ); + + pretty_assertions::assert_eq!(output, expected); +} + +#[tokio::test] +async fn rejects_escalated_permissions_when_policy_is_on_failure() { + use crate::exec::ExecParams; + use crate::protocol::AskForApproval; + use crate::sandboxing::SandboxPermissions; + use crate::turn_diff_tracker::TurnDiffTracker; + use std::collections::HashMap; + + let (session, mut turn_context_raw) = make_session_and_context().await; + turn_context_raw + .approval_policy + .set(AskForApproval::OnFailure) + .expect("test setup should allow updating approval policy"); + let session = Arc::new(session); + let turn_context = Arc::new(turn_context_raw); + + let params = ExecParams { + command: if cfg!(windows) { + vec![ + "cmd.exe".to_string(), + "/C".to_string(), + "echo hi".to_string(), + ] + } else { + vec![ + "/bin/sh".to_string(), + "-c".to_string(), + "echo hi".to_string(), + ] + }, + cwd: turn_context.cwd.clone(), + expiration: 1000.into(), + env: HashMap::new(), + network: None, + sandbox_permissions: SandboxPermissions::RequireEscalated, + windows_sandbox_level: turn_context.windows_sandbox_level, + justification: Some("test".to_string()), + arg0: None, + }; + + let handler = ShellHandler; + let resp = handler + .handle(ToolInvocation { + session: Arc::clone(&session), + turn: Arc::clone(&turn_context), + tracker: Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new())), + call_id: "test-call".to_string(), + tool_name: "shell".to_string(), + payload: ToolPayload::Function { + arguments: serde_json::json!({ + "command": params.command.clone(), + "workdir": Some(turn_context.cwd.to_string_lossy().to_string()), + "timeout_ms": params.expiration.timeout_ms(), + "sandbox_permissions": params.sandbox_permissions, + "justification": params.justification.clone(), + }) + .to_string(), + }, + }) + .await; + + let Err(FunctionCallError::RespondToModel(output)) = resp else { + panic!("expected error result"); + }; + + let expected = format!( + "approval policy is {policy:?}; reject command — you should not ask for escalated permissions if the approval policy is {policy:?}", + policy = turn_context.approval_policy.value() + ); + + pretty_assertions::assert_eq!(output, expected); +} + +#[tokio::test] +async fn unified_exec_rejects_escalated_permissions_when_policy_is_on_failure() { use crate::protocol::AskForApproval; use crate::sandboxing::SandboxPermissions; use crate::turn_diff_tracker::TurnDiffTracker; @@ -3935,7 +4059,7 @@ async fn unified_exec_rejects_escalated_permissions_when_policy_not_on_request() arguments: serde_json::json!({ "cmd": "echo hi", "sandbox_permissions": SandboxPermissions::RequireEscalated, - "justification": "need unsandboxed execution", + "justification": "need escalated permissions", }) .to_string(), }, @@ -3947,7 +4071,53 @@ async fn unified_exec_rejects_escalated_permissions_when_policy_not_on_request() }; let expected = format!( - "approval policy is {policy:?}; reject command — you cannot ask for escalated permissions if the approval policy is {policy:?}", + "approval policy is {policy:?}; reject command — you should not ask for escalated permissions if the approval policy is {policy:?}", + policy = turn_context.approval_policy.value() + ); + + pretty_assertions::assert_eq!(output, expected); +} + +#[tokio::test] +async fn unified_exec_rejects_escalated_permissions_when_policy_is_never() { + use crate::protocol::AskForApproval; + use crate::sandboxing::SandboxPermissions; + use crate::turn_diff_tracker::TurnDiffTracker; + + let (session, mut turn_context_raw) = make_session_and_context().await; + turn_context_raw + .approval_policy + .set(AskForApproval::Never) + .expect("test setup should allow updating approval policy"); + let session = Arc::new(session); + let turn_context = Arc::new(turn_context_raw); + let tracker = Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new())); + + let handler = UnifiedExecHandler; + let resp = handler + .handle(ToolInvocation { + session: Arc::clone(&session), + turn: Arc::clone(&turn_context), + tracker: Arc::clone(&tracker), + call_id: "exec-call".to_string(), + tool_name: "exec_command".to_string(), + payload: ToolPayload::Function { + arguments: serde_json::json!({ + "cmd": "echo hi", + "sandbox_permissions": SandboxPermissions::RequireEscalated, + "justification": "need escalated permissions", + }) + .to_string(), + }, + }) + .await; + + let Err(FunctionCallError::RespondToModel(output)) = resp else { + panic!("expected error result"); + }; + + let expected = format!( + "approval policy is {policy:?}; reject command — you should not ask for escalated permissions if the approval policy is {policy:?}", policy = turn_context.approval_policy.value() ); diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index 086701d9c2..c9e9523753 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -102,6 +102,14 @@ pub(super) fn normalize_and_validate_additional_permissions( SandboxPermissions::WithAdditionalPermissions ); + if sandbox_permissions.requires_escalated_permissions() + && approval_policy != AskForApproval::OnRequest + { + return Err(format!( + "approval policy is {approval_policy:?}; reject command — you should not ask for escalated permissions if the approval policy is {approval_policy:?}" + )); + } + if !request_permission_enabled && (uses_additional_permissions || additional_permissions.is_some()) { @@ -112,7 +120,7 @@ pub(super) fn normalize_and_validate_additional_permissions( } if uses_additional_permissions { - if !matches!(approval_policy, AskForApproval::OnRequest) { + if approval_policy != AskForApproval::OnRequest { return Err(format!( "approval policy is {approval_policy:?}; reject command — you cannot request additional permissions unless the approval policy is OnRequest" )); diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index cea0b9dd33..610bbbb277 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -341,16 +341,15 @@ impl ShellHandler { ) .map_err(FunctionCallError::RespondToModel)?; - // Approval policy guard for explicit escalation in non-OnRequest modes. - if exec_params.sandbox_permissions.requests_sandbox_override() - && !matches!( - turn.approval_policy.value(), - codex_protocol::protocol::AskForApproval::OnRequest - ) + // Approval policy guard for the sandboxed additional-permissions flow. + if exec_params + .sandbox_permissions + .uses_additional_permissions() + && turn.approval_policy.value() != codex_protocol::protocol::AskForApproval::OnRequest { let approval_policy = turn.approval_policy.value(); return Err(FunctionCallError::RespondToModel(format!( - "approval policy is {approval_policy:?}; reject command — you should not ask for escalated permissions if the approval policy is {approval_policy:?}" + "approval policy is {approval_policy:?}; reject command — you should not ask for additional sandbox permissions if the approval policy is not OnRequest" ))); } diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 915c1734c5..3434929c19 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -171,16 +171,14 @@ impl ToolHandler for UnifiedExecHandler { let request_permission_enabled = session.features().enabled(Feature::RequestPermissions); - if sandbox_permissions.requests_sandbox_override() - && !matches!( - context.turn.approval_policy.value(), - codex_protocol::protocol::AskForApproval::OnRequest - ) + if sandbox_permissions.uses_additional_permissions() + && context.turn.approval_policy.value() + != codex_protocol::protocol::AskForApproval::OnRequest { let approval_policy = context.turn.approval_policy.value(); manager.release_process_id(&process_id).await; return Err(FunctionCallError::RespondToModel(format!( - "approval policy is {approval_policy:?}; reject command — you cannot ask for escalated permissions if the approval policy is {approval_policy:?}" + "approval policy is {approval_policy:?}; reject command — you cannot ask for additional sandbox permissions if the approval policy is not OnRequest" ))); }