split sandbox permission validation modes

Co-authored-by: Codex <noreply@openai.com>
2026-11-11 21:10:39 +00:00 · 2026-03-06 20:39:11 -08:00
parent 22ac6b9aaa
commit 9f2c3cb5de
4 changed files with 197 additions and 22 deletions
@@ -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()
    );

@@ -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"
            ));
@@ -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"
            )));
        }

@@ -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"
                    )));
                }