diff --git a/codex-rs/core/src/exec_policy_tests.rs b/codex-rs/core/src/exec_policy_tests.rs index f15c0280b0..776aa8e54f 100644 --- a/codex-rs/core/src/exec_policy_tests.rs +++ b/codex-rs/core/src/exec_policy_tests.rs @@ -1777,8 +1777,12 @@ async fn proposed_execpolicy_amendment_is_present_when_heuristics_allow() { async fn proposed_execpolicy_amendment_is_suppressed_when_policy_matches_allow() { assert_exec_approval_requirement_for_command( ExecApprovalRequirementScenario { - policy_src: Some(r#"prefix_rule(pattern=["echo"], decision="allow")"#.to_string()), - command: vec!["echo".to_string(), "safe".to_string()], + policy_src: Some(r#"prefix_rule(pattern=["python3"], decision="allow")"#.to_string()), + command: vec![ + "python3".to_string(), + "-c".to_string(), + "print(1)".to_string(), + ], approval_policy: AskForApproval::OnRequest, sandbox_policy: SandboxPolicy::new_read_only_policy(), file_system_sandbox_policy: read_only_file_system_sandbox_policy(), diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index 4fd1ea17f4..648d3f79e8 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -27,6 +27,7 @@ use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; use crate::tools::sandboxing::default_exec_approval_requirement; +use crate::tools::sandboxing::sandbox_override_for_first_attempt; use codex_hooks::PermissionRequestDecision; use codex_otel::ToolDecisionSource; use codex_protocol::error::CodexErr; @@ -149,7 +150,7 @@ impl ToolOrchestrator { let requirement = tool.exec_approval_requirement(req).unwrap_or_else(|| { default_exec_approval_requirement(approval_policy, &file_system_sandbox_policy) }); - match requirement { + match &requirement { ExecApprovalRequirement::Skip { .. } => { if strict_auto_review { let guardian_review_id = Some(new_guardian_review_id()); @@ -184,7 +185,7 @@ impl ToolOrchestrator { } } ExecApprovalRequirement::Forbidden { reason } => { - return Err(ToolError::Rejected(reason)); + return Err(ToolError::Rejected(reason.clone())); } ExecApprovalRequirement::NeedsApproval { reason, .. } => { let guardian_review_id = use_guardian.then(new_guardian_review_id); @@ -193,7 +194,7 @@ impl ToolOrchestrator { turn: &tool_ctx.turn, call_id: &tool_ctx.call_id, guardian_review_id: guardian_review_id.clone(), - retry_reason: reason, + retry_reason: reason.clone(), network_approval_context: None, }; let decision = Self::request_approval( @@ -214,8 +215,13 @@ impl ToolOrchestrator { } // 2) First attempt under the selected sandbox. + let sandbox_override = sandbox_override_for_first_attempt( + tool.sandbox_permissions(req), + &requirement, + &file_system_sandbox_policy, + ); let managed_network_active = turn_ctx.network.is_some(); - let initial_sandbox = match tool.sandbox_mode_for_first_attempt(req) { + let initial_sandbox = match sandbox_override { SandboxOverride::BypassSandboxFirstAttempt => SandboxType::None, SandboxOverride::NoOverride => self.sandbox.select_initial( &file_system_sandbox_policy, diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 4aa58552a4..4f6eb620aa 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -28,13 +28,11 @@ use crate::tools::sandboxing::ApprovalCtx; use crate::tools::sandboxing::ExecApprovalRequirement; use crate::tools::sandboxing::PermissionRequestPayload; use crate::tools::sandboxing::SandboxAttempt; -use crate::tools::sandboxing::SandboxOverride; use crate::tools::sandboxing::Sandboxable; use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; use crate::tools::sandboxing::managed_network_for_sandbox_permissions; -use crate::tools::sandboxing::sandbox_override_for_first_attempt; use crate::tools::sandboxing::with_cached_approval; use codex_network_proxy::NetworkProxy; use codex_protocol::exec_output::ExecToolCallOutput; @@ -210,8 +208,8 @@ impl Approvable for ShellRuntime { )) } - fn sandbox_mode_for_first_attempt(&self, req: &ShellRequest) -> SandboxOverride { - sandbox_override_for_first_attempt(req.sandbox_permissions, &req.exec_approval_requirement) + fn sandbox_permissions(&self, req: &ShellRequest) -> SandboxPermissions { + req.sandbox_permissions } } diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index ae080e0388..5bc6687c37 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -26,13 +26,11 @@ use crate::tools::sandboxing::ApprovalCtx; use crate::tools::sandboxing::ExecApprovalRequirement; use crate::tools::sandboxing::PermissionRequestPayload; use crate::tools::sandboxing::SandboxAttempt; -use crate::tools::sandboxing::SandboxOverride; use crate::tools::sandboxing::Sandboxable; use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; use crate::tools::sandboxing::managed_network_for_sandbox_permissions; -use crate::tools::sandboxing::sandbox_override_for_first_attempt; use crate::tools::sandboxing::with_cached_approval; use crate::unified_exec::NoopSpawnLifecycle; use crate::unified_exec::UnifiedExecError; @@ -212,8 +210,8 @@ impl Approvable for UnifiedExecRuntime<'_> { )) } - fn sandbox_mode_for_first_attempt(&self, req: &UnifiedExecRequest) -> SandboxOverride { - sandbox_override_for_first_attempt(req.sandbox_permissions, &req.exec_approval_requirement) + fn sandbox_permissions(&self, req: &UnifiedExecRequest) -> SandboxPermissions { + req.sandbox_permissions } } diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 6bb78632f7..656615d4d1 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -248,18 +248,27 @@ pub(crate) enum SandboxOverride { pub(crate) fn sandbox_override_for_first_attempt( sandbox_permissions: SandboxPermissions, exec_approval_requirement: &ExecApprovalRequirement, + file_system_sandbox_policy: &FileSystemSandboxPolicy, ) -> SandboxOverride { // ExecPolicy `Allow` can intentionally imply full trust (Skip + bypass_sandbox=true), // which supersedes `with_additional_permissions` sandboxed execution hints. - if sandbox_permissions.requires_escalated_permissions() - || matches!( - exec_approval_requirement, - ExecApprovalRequirement::Skip { - bypass_sandbox: true, - .. - } - ) - { + if matches!( + exec_approval_requirement, + ExecApprovalRequirement::Skip { + bypass_sandbox: true, + .. + } + ) { + return SandboxOverride::BypassSandboxFirstAttempt; + } + + // Deny-read restrictions suppress explicit escalation because that path + // would otherwise discard the filesystem policy entirely. + if file_system_sandbox_policy.has_denied_read_restrictions() { + return SandboxOverride::NoOverride; + } + + if sandbox_permissions.requires_escalated_permissions() { SandboxOverride::BypassSandboxFirstAttempt } else { SandboxOverride::NoOverride @@ -289,11 +298,10 @@ pub(crate) trait Approvable { // requests touching a subset can be auto-approved. fn approval_keys(&self, req: &Req) -> Vec; - /// Some tools may request to skip the sandbox on the first attempt - /// (e.g., when the request explicitly asks for escalated permissions). - /// Defaults to `NoOverride`. - fn sandbox_mode_for_first_attempt(&self, _req: &Req) -> SandboxOverride { - SandboxOverride::NoOverride + /// Return per-request sandbox permissions for first-attempt sandbox + /// selection. Most tools use the ambient sandbox policy unchanged. + fn sandbox_permissions(&self, _req: &Req) -> SandboxPermissions { + SandboxPermissions::UseDefault } fn should_bypass_approval(&self, policy: AskForApproval, already_approved: bool) -> bool { diff --git a/codex-rs/core/src/tools/sandboxing_tests.rs b/codex-rs/core/src/tools/sandboxing_tests.rs index 19eb7a67d9..3989862ab5 100644 --- a/codex-rs/core/src/tools/sandboxing_tests.rs +++ b/codex-rs/core/src/tools/sandboxing_tests.rs @@ -1,6 +1,9 @@ use super::*; use crate::sandboxing::SandboxPermissions; use crate::tools::hook_names::HookToolName; +use codex_protocol::permissions::FileSystemAccessMode; +use codex_protocol::permissions::FileSystemPath; +use codex_protocol::permissions::FileSystemSandboxEntry; use codex_protocol::protocol::GranularApprovalConfig; use codex_protocol::protocol::NetworkAccess; use pretty_assertions::assert_eq; @@ -120,6 +123,7 @@ fn additional_permissions_allow_bypass_sandbox_first_attempt_when_execpolicy_ski bypass_sandbox: true, proposed_execpolicy_amendment: None, }, + &FileSystemSandboxPolicy::default(), ), SandboxOverride::BypassSandboxFirstAttempt ); @@ -134,7 +138,43 @@ fn guardian_bypasses_sandbox_for_explicit_escalation_on_first_attempt() { bypass_sandbox: false, proposed_execpolicy_amendment: None, }, + &FileSystemSandboxPolicy::default(), ), SandboxOverride::BypassSandboxFirstAttempt ); } + +#[test] +fn deny_read_blocks_explicit_escalation_but_preserves_policy_bypass() { + let file_system_policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { + pattern: "**/*.env".to_string(), + }, + access: FileSystemAccessMode::None, + }]); + + assert_eq!( + sandbox_override_for_first_attempt( + SandboxPermissions::RequireEscalated, + &ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, + }, + &file_system_policy, + ), + SandboxOverride::NoOverride, + "explicit escalation would drop deny-read filesystem policy, so keep the first attempt sandboxed", + ); + assert_eq!( + sandbox_override_for_first_attempt( + SandboxPermissions::WithAdditionalPermissions, + &ExecApprovalRequirement::Skip { + bypass_sandbox: true, + proposed_execpolicy_amendment: None, + }, + &file_system_policy, + ), + SandboxOverride::BypassSandboxFirstAttempt, + "exec-policy allow rules intentionally bypass sandbox even when deny-read entries exist", + ); +}