feedback

Author: aibrahim-oai

codex-rs/core/src/codex.rs

@@ -2689,6 +2689,20 @@ async fn handle_container_exec_with_params(
     sub_id: String,
     call_id: String,
 ) -> ResponseInputItem {
+    if params.with_escalated_permissions.unwrap_or(false)
+        && !matches!(turn_context.approval_policy, AskForApproval::OnRequest)
+    {
+        return ResponseInputItem::FunctionCallOutput {
+            call_id,
+            output: FunctionCallOutputPayload {
+                content: 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
+                ),
+                success: None,
+            },
+        };
+    }
     // check if this was a patch, and apply it if so
     let apply_patch_exec = match maybe_parse_apply_patch_verified(&params.command, &params.cwd) {
         MaybeApplyPatchVerified::Body(changes) => {
@@ -3650,4 +3664,94 @@ mod tests {
 
         (rollout_items, live_history.contents())
     }
+
+    #[tokio::test]
+    async fn rejects_escalated_permissions_when_policy_not_on_request() {
+        use crate::exec::ExecParams;
+        use crate::protocol::AskForApproval;
+        use crate::protocol::SandboxPolicy;
+        use crate::turn_diff_tracker::TurnDiffTracker;
+        use std::collections::HashMap;
+
+        let (session, mut turn_context) = make_session_and_context();
+        // Ensure policy is NOT OnRequest so the early rejection path triggers
+        turn_context.approval_policy = AskForApproval::OnFailure;
+
+        let params = ExecParams {
+            command: vec![
+                "/bin/sh".to_string(),
+                "-c".to_string(),
+                "echo hi".to_string(),
+            ],
+            cwd: turn_context.cwd.clone(),
+            timeout_ms: Some(1000),
+            env: HashMap::new(),
+            with_escalated_permissions: Some(true),
+            justification: Some("test".to_string()),
+        };
+
+        let mut turn_diff_tracker = TurnDiffTracker::new();
+
+        let sub_id = "test-sub".to_string();
+        let call_id = "test-call".to_string();
+
+        let resp = handle_container_exec_with_params(
+            params,
+            &session,
+            &turn_context,
+            &mut turn_diff_tracker,
+            sub_id,
+            call_id,
+        )
+        .await;
+
+        let ResponseInputItem::FunctionCallOutput { output, .. } = resp else {
+            panic!("expected FunctionCallOutput");
+        };
+
+        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
+        );
+
+        pretty_assertions::assert_eq!(output.content, expected);
+
+        // Now retry the same command WITHOUT escalated permissions; should succeed.
+        // Force DangerFullAccess to avoid platform sandbox dependencies in tests.
+        turn_context.sandbox_policy = SandboxPolicy::DangerFullAccess;
+
+        let params2 = ExecParams {
+            command: vec![
+                "/bin/sh".to_string(),
+                "-c".to_string(),
+                "echo hi".to_string(),
+            ],
+            cwd: turn_context.cwd.clone(),
+            timeout_ms: Some(1000),
+            env: HashMap::new(),
+            with_escalated_permissions: Some(false),
+            justification: Some("test".to_string()),
+        };
+
+        let resp2 = handle_container_exec_with_params(
+            params2,
+            &session,
+            &turn_context,
+            &mut turn_diff_tracker,
+            "test-sub".to_string(),
+            "test-call-2".to_string(),
+        )
+        .await;
+
+        let ResponseInputItem::FunctionCallOutput { output, .. } = resp2 else {
+            panic!("expected FunctionCallOutput on retry");
+        };
+
+        // Parse the structured exec output and assert success without new structs
+        let v: serde_json::Value =
+            serde_json::from_str(&output.content).expect("valid exec output json");
+        assert_eq!(v["metadata"]["exit_code"].as_i64(), Some(0));
+        assert!(v["output"].as_str().unwrap_or("").contains("hi"));
+        assert_eq!(output.success, Some(true));
+    }
 }

codex-rs/core/src/safety.rs

@@ -3,7 +3,6 @@ use std::path::Component;
 use std::path::Path;
 use std::path::PathBuf;
 
-use AskForApproval::*;
 use codex_apply_patch::ApplyPatchAction;
 use codex_apply_patch::ApplyPatchFileChange;
 
@@ -99,18 +98,7 @@ pub fn assess_command_safety(
     // would probably be fine to run the command in a sandbox, but when
     // `approved.contains(command)` is `true`, the user may have approved it for
     // the session _because_ they know it needs to run outside a sandbox.
-    let command_is_trusted = is_known_safe_command(command) || approved.contains(command);
-
-    // reject function calls when the model asks for escalated permissions when it should not have to
-    if let Some(decision) = reject_forbidden_escalation(
-        approval_policy,
-        with_escalated_permissions,
-        command_is_trusted,
-    ) {
-        return decision;
-    }
-
-    if command_is_trusted {
+    if is_known_safe_command(command) || approved.contains(command) {
         return SafetyCheck::AutoApprove {
             sandbox_type: SandboxType::None,
         };
@@ -127,12 +115,6 @@ pub(crate) fn assess_safety_for_untrusted_command(
     use AskForApproval::*;
     use SandboxPolicy::*;
 
-    if let Some(decision) =
-        reject_forbidden_escalation(approval_policy, with_escalated_permissions, false)
-    {
-        return decision;
-    }
-
     match (approval_policy, sandbox_policy) {
         (UnlessTrusted, _) => {
             // Even though the user may have opted into DangerFullAccess,
@@ -194,38 +176,6 @@ pub fn get_platform_sandbox() -> Option<SandboxType> {
     }
 }
 
-/// Forbidden escalation is when the model asks for escalated permissions when it should not have to
-/// Rules:
-/// The model shouldn't ask for escalated permissions if the command is trusted
-/// The model shouldn't ask for escalated permissions if the approval policy is Never
-/// The model shouldn't ask for escalated permissions if the approval policy is OnFailure and it hasn't failed
-fn reject_forbidden_escalation(
-    approval_policy: AskForApproval,
-    with_escalated_permissions: bool,
-    command_is_trusted: bool,
-) -> Option<SafetyCheck> {
-    if !with_escalated_permissions {
-        return None;
-    }
-
-    let reason = match approval_policy {
-        Never => Some(
-            "auto-rejected. You should not ask for escalated permissions if the approval policy is Never".to_string(),
-        ),
-        OnFailure => Some(
-            "auto-rejected. You should not ask for escalated permissions if the approval policy is OnFailure and it hasn't failed"
-                .to_string(),
-        ),
-        UnlessTrusted if command_is_trusted => Some(
-            "auto-rejected. The command is already trusted under the UnlessTrusted approval policy. You do not need to ask for escalated permissions"
-                .to_string(),
-        ),
-        OnRequest | UnlessTrusted => None,
-    }?;
-
-    Some(SafetyCheck::Reject { reason })
-}
-
 fn is_write_patch_constrained_to_writable_paths(
     action: &ApplyPatchAction,
     sandbox_policy: &SandboxPolicy,
@@ -397,62 +347,4 @@ mod tests {
         };
         assert_eq!(safety_check, expected);
     }
-
-    #[test]
-    fn test_escalation_rejected_when_policy_is_never() {
-        let command = vec!["git".to_string(), "status".to_string()];
-        let approval_policy = AskForApproval::Never;
-        let sandbox_policy = SandboxPolicy::ReadOnly;
-        let approved = HashSet::new();
-
-        let safety_check =
-            assess_command_safety(&command, approval_policy, &sandbox_policy, &approved, true);
-
-        assert_eq!(
-            safety_check,
-            SafetyCheck::Reject {
-                reason: "auto-rejected. You should not ask for escalated permissions if the approval policy is Never"
-                    .to_string(),
-            }
-        );
-    }
-
-    #[test]
-    fn test_escalation_rejected_for_on_failure_policy() {
-        let command = vec!["git".to_string(), "status".to_string()];
-        let approval_policy = AskForApproval::OnFailure;
-        let sandbox_policy = SandboxPolicy::ReadOnly;
-        let approved = HashSet::new();
-
-        let safety_check =
-            assess_command_safety(&command, approval_policy, &sandbox_policy, &approved, true);
-
-        assert_eq!(
-            safety_check,
-            SafetyCheck::Reject {
-                reason:
-                    "auto-rejected. You should not ask for escalated permissions if the approval policy is OnFailure and it hasn't failed"
-                        .to_string(),
-            }
-        );
-    }
-
-    #[test]
-    fn test_escalation_rejected_when_trusted_under_unless_trusted() {
-        let command = vec!["just".to_string(), "fmt".to_string()];
-        let approval_policy = AskForApproval::UnlessTrusted;
-        let sandbox_policy = SandboxPolicy::ReadOnly;
-        let approved = HashSet::from([command.clone()]);
-
-        let safety_check =
-            assess_command_safety(&command, approval_policy, &sandbox_policy, &approved, true);
-
-        assert_eq!(
-            safety_check,
-            SafetyCheck::Reject {
-                reason: "auto-rejected. The command is already trusted under the UnlessTrusted approval policy. You do not need to ask for escalated permissions"
-                    .to_string(),
-            }
-        );
-    }
 }