From 34db7e55638abde667a9443ebc8a2297acf6338c Mon Sep 17 00:00:00 2001 From: iceweasel-oai Date: Tue, 11 Aug 2026 17:37:00 +0000 Subject: [PATCH] Sandbox remote apply_patch operations (#38043) ## Why Cross-platform remote `apply_patch` calls were rejected when filesystem writes were restricted because patch verification and writes could not be safely performed against executor files. ## What changed - Route intercepted and direct remote patches through the executor-managed filesystem sandbox, including the configured workspace roots. - Select the restricted-token sandbox for Windows executor paths when no Windows sandbox level was configured. - Fail closed when an executor cannot enforce the requested sandbox, and treat executor-managed access failures as sandbox denials so approval can retry the patch without sandboxing. ## Testing - Cover sandboxed remote patches, denied writes, approval retries, Windows sandbox selection, and executor filesystem enforcement. GitOrigin-RevId: caddeed0b266c456a689080a14a3a58e2bd7887c --- codex-rs/core/src/session/mcp.rs | 4 +- codex-rs/core/src/session/turn_context.rs | 6 +- .../handlers/unified_exec/exec_command.rs | 28 +- .../core/src/tools/runtimes/apply_patch.rs | 27 +- .../src/tools/runtimes/apply_patch_tests.rs | 26 +- codex-rs/core/src/tools/sandboxing.rs | 31 +- codex-rs/core/src/tools/sandboxing_tests.rs | 6 +- codex-rs/core/tests/suite/apply_patch_cli.rs | 8 + .../suite/unified_exec_process_events.rs | 266 +++++++++++++++--- codex-rs/exec-server/src/fs_sandbox.rs | 37 ++- .../exec-server/tests/file_system/shared.rs | 4 + .../exec-server/tests/file_system/support.rs | 28 +- codex-rs/sandboxing/src/denial.rs | 59 ++-- codex-rs/sandboxing/src/lib.rs | 1 + 14 files changed, 408 insertions(+), 123 deletions(-) diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index ab71d9aa9c..77f95df876 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -1,5 +1,6 @@ use super::mcp_refresh::McpRefreshInvalidationGuard; use super::*; +use crate::tools::sandboxing::executor_windows_sandbox_level; use codex_exec_server::ExecutorCapabilityDiscoveryCache; use codex_exec_server::ExecutorCapabilityDiscoverySnapshot; use codex_exec_server::FileSystemSandboxContext; @@ -386,7 +387,8 @@ impl Session { environment.cwd().clone(), ); sandbox.workspace_roots = environment.workspace_roots().to_vec(); - sandbox.windows_sandbox_level = windows_sandbox_level; + sandbox.windows_sandbox_level = + executor_windows_sandbox_level(windows_sandbox_level, environment.cwd()); sandbox.windows_sandbox_private_desktop = config.permissions.windows_sandbox_private_desktop; sandbox.use_legacy_landlock = config.features.use_legacy_landlock(); diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index bd2d09292e..3368847aa8 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -2,6 +2,7 @@ use super::*; use crate::environment_selection::TurnEnvironmentSnapshot; use crate::exec_policy::AllowPrefixRules; use crate::shell_snapshot::ShellSnapshotFile; +use crate::tools::sandboxing::executor_windows_sandbox_level; use codex_core_plugins::PluginCommandAttribution; use codex_core_plugins::TrustedPluginRoots; use codex_file_system::FileSystemSandboxContext; @@ -378,7 +379,10 @@ impl TurnContext { permissions: permissions.into(), cwd: Some(environment.cwd().clone()), workspace_roots: environment.workspace_roots().to_vec(), - windows_sandbox_level: self.windows_sandbox_level, + windows_sandbox_level: executor_windows_sandbox_level( + self.windows_sandbox_level, + environment.cwd(), + ), windows_sandbox_private_desktop: self .config .permissions diff --git a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs index e89afc8d79..0829e5f07e 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs @@ -238,23 +238,6 @@ impl ExecCommandHandler { ) .map_err(FunctionCallError::RespondToModel)?; let command = resolved_command.command; - if environment.is_remote() - && !cwd_uses_native_convention - && !turn_environment - .permission_profile() - .file_system_sandbox_policy() - .has_full_disk_write_access() - && matches!( - codex_apply_patch::maybe_parse_apply_patch(&command, &cwd), - codex_apply_patch::MaybeApplyPatch::Body(_) - ) - { - // CA-781: patch verification reads executor files before process sandboxing applies. - manager.release_process_id(process_id).await; - return Err(FunctionCallError::RespondToModel( - "cross-platform remote apply_patch is unavailable until executor-side filesystem sandboxing is supported".to_string(), - )); - } let shell_type = resolved_command.shell_type; let command_for_display = codex_shell_command::parse_command::shlex_join(&command); @@ -329,7 +312,7 @@ impl ExecCommandHandler { } }; - if let Some(output) = intercept_apply_patch( + let intercepted_patch = intercept_apply_patch( &command, &cwd, fs.as_ref(), @@ -340,8 +323,13 @@ impl ExecCommandHandler { &context.call_id, "exec_command", ) - .await? - { + .await; + // Keep the reservation when interception returns `Ok(None)`: the normal command below + // still needs this process ID. + if intercepted_patch.is_err() { + manager.release_process_id(process_id).await; + } + if let Some(output) = intercepted_patch? { manager.release_process_id(process_id).await; return Ok(boxed_tool_output(ExecCommandToolOutput { event_call_id: String::new(), diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index 473cc650d6..ab4a5c7b07 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -13,6 +13,7 @@ use crate::tools::sandboxing::Sandboxable; use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; +use crate::tools::sandboxing::executor_windows_sandbox_level; use codex_apply_patch::AppliedPatchDelta; use codex_apply_patch::ApplyPatchAction; use codex_exec_server::FileSystemSandboxContext; @@ -25,6 +26,7 @@ use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::FileChange; use codex_sandboxing::SandboxType; use codex_sandboxing::SandboxablePreference; +use codex_sandboxing::is_likely_executor_managed_sandbox_denied; use codex_sandboxing::policy_transforms::effective_permission_profile; use codex_sandboxing::record_filesystem_sandbox_violation; use codex_utils_path_uri::PathUri; @@ -85,7 +87,7 @@ impl ApplyPatchRuntime { req: &ApplyPatchRequest, attempt: &SandboxAttempt<'_>, ) -> Option { - if attempt.sandbox == SandboxType::None { + if !attempt.sandbox_requested { return None; } @@ -97,7 +99,10 @@ impl ApplyPatchRuntime { permissions: permissions.into(), cwd: Some(attempt.sandbox_cwd.clone()), workspace_roots: attempt.workspace_roots.to_vec(), - windows_sandbox_level: attempt.windows_sandbox_level, + windows_sandbox_level: executor_windows_sandbox_level( + attempt.windows_sandbox_level, + attempt.sandbox_cwd, + ), windows_sandbox_private_desktop: attempt.windows_sandbox_private_desktop, windows_sandbox_proxy_settings_mode: None, use_legacy_landlock: attempt.use_legacy_landlock, @@ -149,6 +154,10 @@ impl ToolRuntime for ApplyPatchRunti &req.turn_environment } + fn uses_executor_managed_process_sandbox(&self, req: &ApplyPatchRequest) -> bool { + req.turn_environment.environment.is_remote() + } + fn sandbox_cwd<'a>(&self, req: &'a ApplyPatchRequest) -> Option<&'a PathUri> { Some(&req.action.cwd) } @@ -191,8 +200,18 @@ impl ToolRuntime for ApplyPatchRunti duration: started_at.elapsed(), timed_out: false, }; - if failed && is_likely_sandbox_denied(attempt.sandbox, &output) { - record_filesystem_sandbox_violation(attempt.sandbox, &output); + let sandbox_denied = failed + && if attempt.sandbox == SandboxType::None { + attempt.sandbox_requested && is_likely_executor_managed_sandbox_denied(&output) + } else { + is_likely_sandbox_denied(attempt.sandbox, &output) + }; + if sandbox_denied { + // TODO(iceweasel): Report executor filesystem sandbox backends like process/start so + // executor-managed apply_patch denials can emit backend-specific violation telemetry. + if attempt.sandbox != SandboxType::None { + record_filesystem_sandbox_violation(attempt.sandbox, &output); + } return Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied { output: Box::new(output), network_policy_decision: None, diff --git a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs index e3e10aa16d..8be5183e67 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs @@ -267,7 +267,7 @@ async fn file_system_sandbox_context_preserves_executor_workspace_permissions() } #[tokio::test] -async fn no_sandbox_attempt_has_no_file_system_context() { +async fn file_system_sandbox_context_respects_sandbox_request() { let path = std::env::temp_dir() .join("apply-patch-runtime-none.txt") .abs(); @@ -310,4 +310,28 @@ async fn no_sandbox_attempt_has_no_file_system_context() { ApplyPatchRuntime::file_system_sandbox_context_for_attempt(&req, &attempt), None ); + + let cwd = PathUri::parse("file:///C:/workspace").expect("Windows workspace URI"); + let permissions = PermissionProfile::workspace_write(); + let attempt = SandboxAttempt { + sandbox_requested: true, + permissions: &permissions, + exec_server_permissions: &permissions, + sandbox_cwd: &cwd, + workspace_roots: std::slice::from_ref(&cwd), + ..attempt + }; + + assert_eq!( + ApplyPatchRuntime::file_system_sandbox_context_for_attempt(&req, &attempt), + Some(FileSystemSandboxContext { + permissions: permissions.into(), + cwd: Some(cwd.clone()), + workspace_roots: vec![cwd], + windows_sandbox_level: WindowsSandboxLevel::RestrictedToken, + windows_sandbox_private_desktop: false, + windows_sandbox_proxy_settings_mode: None, + use_legacy_landlock: false, + }) + ); } diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index cbc9bc73b0..2d20334f64 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -401,6 +401,19 @@ pub(crate) struct SandboxAttempt<'a> { pub(crate) network_proxy: Option<&'a NetworkProxy>, } +pub(crate) fn executor_windows_sandbox_level( + windows_sandbox_level: WindowsSandboxLevel, + cwd: &PathUri, +) -> WindowsSandboxLevel { + if windows_sandbox_level == WindowsSandboxLevel::Disabled + && cwd.infer_path_convention() == Some(PathConvention::Windows) + { + WindowsSandboxLevel::RestrictedToken + } else { + windows_sandbox_level + } +} + impl<'a> SandboxAttempt<'a> { pub(crate) fn network_proxy<'b>( &'b self, @@ -478,24 +491,14 @@ impl<'a> SandboxAttempt<'a> { crate::sandboxing::ExecRequest::from_sandbox_exec_request(request, options, Vec::new()); exec_request.exec_server_managed_network = managed_network; if self.sandbox_requested { - // This level comes from the orchestrator's config, so `Disabled` means Windows - // sandboxing is irrelevant on a non-Windows host. A Windows executor would instead - // treat it as unable to enforce the requested sandbox. Select its baseline restricted - // token backend while preserving explicitly configured levels and same-OS behavior. - let windows_sandbox_level = if self.windows_sandbox_level - == WindowsSandboxLevel::Disabled - && self.sandbox_cwd.infer_path_convention() == Some(PathConvention::Windows) - && PathConvention::native() != PathConvention::Windows - { - WindowsSandboxLevel::RestrictedToken - } else { - self.windows_sandbox_level - }; exec_request.exec_server_sandbox = Some(FileSystemSandboxContext { permissions: exec_server_permissions.into(), cwd: Some(exec_request.windows_sandbox_policy_cwd.clone()), workspace_roots: self.workspace_roots.to_vec(), - windows_sandbox_level, + windows_sandbox_level: executor_windows_sandbox_level( + self.windows_sandbox_level, + self.sandbox_cwd, + ), windows_sandbox_private_desktop: self.windows_sandbox_private_desktop, windows_sandbox_proxy_settings_mode: None, use_legacy_landlock: self.use_legacy_landlock, diff --git a/codex-rs/core/src/tools/sandboxing_tests.rs b/codex-rs/core/src/tools/sandboxing_tests.rs index d7fed8ba64..2cfdec5b8e 100644 --- a/codex-rs/core/src/tools/sandboxing_tests.rs +++ b/codex-rs/core/src/tools/sandboxing_tests.rs @@ -266,7 +266,11 @@ fn exec_server_env_keeps_command_native_and_carries_sandbox_context() { permissions: exec_server_permissions.clone().into(), cwd: Some(cwd_uri.clone()), workspace_roots: vec![cwd_uri.clone()], - windows_sandbox_level: codex_protocol::config_types::WindowsSandboxLevel::Disabled, + windows_sandbox_level: if cfg!(windows) { + codex_protocol::config_types::WindowsSandboxLevel::RestrictedToken + } else { + codex_protocol::config_types::WindowsSandboxLevel::Disabled + }, windows_sandbox_private_desktop: false, windows_sandbox_proxy_settings_mode: None, use_legacy_landlock: false, diff --git a/codex-rs/core/tests/suite/apply_patch_cli.rs b/codex-rs/core/tests/suite/apply_patch_cli.rs index db4635e6ca..34e96f5099 100644 --- a/codex-rs/core/tests/suite/apply_patch_cli.rs +++ b/codex-rs/core/tests/suite/apply_patch_cli.rs @@ -136,6 +136,10 @@ fn restrictive_workspace_write_profile() -> PermissionProfile { } fn workspace_write_with_read_only_root(read_only_root: AbsolutePathBuf) -> PermissionProfile { + if cfg!(windows) { + return restrictive_workspace_write_profile(); + } + let file_system_sandbox_policy = FileSystemSandboxPolicy::restricted(vec![ FileSystemSandboxEntry { path: FileSystemPath::Path { @@ -996,6 +1000,10 @@ async fn apply_patch_cli_preserves_existing_hard_link_outside_workspace() -> Res let harness_work_dir = work_dir.clone(); let harness = apply_patch_harness_with(move |builder| { builder.with_config(move |config| { + config.workspace_roots = vec![harness_work_dir.clone()]; + config + .permissions + .set_workspace_roots(config.workspace_roots.clone()); config.cwd = harness_work_dir; }) }) diff --git a/codex-rs/core/tests/suite/unified_exec_process_events.rs b/codex-rs/core/tests/suite/unified_exec_process_events.rs index 99bb5ca806..46aea1ee2b 100644 --- a/codex-rs/core/tests/suite/unified_exec_process_events.rs +++ b/codex-rs/core/tests/suite/unified_exec_process_events.rs @@ -15,11 +15,13 @@ use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; +use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::TurnEnvironmentSelection; use codex_protocol::protocol::TurnEnvironmentSelections; use codex_protocol::user_input::UserInput; use codex_utils_path_uri::PathUri; use core_test_support::managed_network_requirements_loader; +use core_test_support::responses::ev_apply_patch_custom_tool_call; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; use core_test_support::responses::ev_function_call; @@ -57,7 +59,10 @@ enum PushedExecScenario { Complete, DirectDenied, ElevatedPowerShell, - InterceptedPatch, + SandboxedInterceptedPatch, + SandboxedDirectPatch, + SandboxedDirectPatchDenied, + SandboxedDirectPatchRetry, UnsandboxedInterceptedPatch, FullDiskInterceptedPatch, LegacyExit, @@ -178,6 +183,35 @@ async fn serve_exec_with_pushed_events( ) .await; } + Some("fs/readFile") + if matches!( + scenario, + PushedExecScenario::SandboxedInterceptedPatch + | PushedExecScenario::SandboxedDirectPatch + | PushedExecScenario::SandboxedDirectPatchDenied + | PushedExecScenario::SandboxedDirectPatchRetry + ) => + { + if !request["params"]["sandbox"].is_null() { + assert_eq!(request["params"]["sandbox"]["cwd"], "file:///C:/workspace"); + assert_eq!( + request["params"]["sandbox"]["workspaceRoots"], + json!(["file:///C:/workspace", "file:///D:/other-workspace"]) + ); + assert_eq!( + request["params"]["sandbox"]["windowsSandboxLevel"], + "restricted-token" + ); + } + send_exec_server_json( + &mut websocket, + json!({ + "id": request["id"], + "result": { "dataBase64": BASE64_STANDARD.encode("old\n") } + }), + ) + .await; + } Some("fs/readFile") if unrestricted_patch => { send_exec_server_json( &mut websocket, @@ -188,7 +222,38 @@ async fn serve_exec_with_pushed_events( ) .await; } - Some("fs/writeFile") if unrestricted_patch => { + Some("fs/writeFile") + if matches!( + scenario, + PushedExecScenario::SandboxedDirectPatchDenied + | PushedExecScenario::SandboxedDirectPatchRetry + ) && !request["params"]["sandbox"].is_null() => + { + send_exec_server_json( + &mut websocket, + json!({ + "id": request["id"], + "error": { "code": -32600, "message": "Access is denied. (os error 5)" } + }), + ) + .await; + if matches!(scenario, PushedExecScenario::SandboxedDirectPatchDenied) { + return PushedExecServerResult { + process_read_requests: 0, + process_start: request, + }; + } + } + Some("fs/writeFile") + if matches!( + scenario, + PushedExecScenario::SandboxedInterceptedPatch + | PushedExecScenario::SandboxedDirectPatch + | PushedExecScenario::SandboxedDirectPatchRetry + | PushedExecScenario::UnsandboxedInterceptedPatch + | PushedExecScenario::FullDiskInterceptedPatch + ) => + { send_exec_server_json(&mut websocket, json!({ "id": request["id"], "result": {} })) .await; return PushedExecServerResult { @@ -322,8 +387,11 @@ async fn serve_exec_with_pushed_events( ) .await; } - PushedExecScenario::InterceptedPatch => { - panic!("cross-platform intercepted patches must not start a remote process") + PushedExecScenario::SandboxedInterceptedPatch + | PushedExecScenario::SandboxedDirectPatch + | PushedExecScenario::SandboxedDirectPatchDenied + | PushedExecScenario::SandboxedDirectPatchRetry => { + panic!("cross-platform sandboxed patches must use the remote filesystem") } PushedExecScenario::UnsandboxedInterceptedPatch | PushedExecScenario::FullDiskInterceptedPatch => { @@ -378,8 +446,11 @@ async fn serve_exec_with_pushed_events( PushedExecScenario::ElevatedPowerShell => { panic!("elevated remote PowerShell must not read a remote process") } - PushedExecScenario::InterceptedPatch => { - panic!("cross-platform intercepted patches must not read a remote process") + PushedExecScenario::SandboxedInterceptedPatch + | PushedExecScenario::SandboxedDirectPatch + | PushedExecScenario::SandboxedDirectPatchDenied + | PushedExecScenario::SandboxedDirectPatchRetry => { + panic!("cross-platform sandboxed patches must not read a remote process") } PushedExecScenario::UnsandboxedInterceptedPatch | PushedExecScenario::FullDiskInterceptedPatch => { @@ -464,7 +535,10 @@ async fn serve_exec_with_pushed_events( #[test_case(PushedExecScenario::Complete, true, false, false ; "strict_managed_allowlist_omits_policy_callbacks")] #[cfg_attr(not(windows), test_case(PushedExecScenario::Complete, false, false, true ; "foreign_windows_workspace_sandbox"))] #[test_case(PushedExecScenario::ElevatedPowerShell, false, false, true ; "windows_elevated_powershell_disables_profile")] -#[cfg_attr(not(windows), test_case(PushedExecScenario::InterceptedPatch, false, false, true ; "foreign_windows_intercepted_patch_fails_closed"))] +#[cfg_attr(not(windows), test_case(PushedExecScenario::SandboxedInterceptedPatch, false, false, true ; "foreign_windows_intercepted_patch_is_sandboxed"))] +#[cfg_attr(not(windows), test_case(PushedExecScenario::SandboxedDirectPatch, false, false, true ; "foreign_windows_direct_patch_is_sandboxed"))] +#[cfg_attr(not(windows), test_case(PushedExecScenario::SandboxedDirectPatchDenied, false, false, true ; "foreign_windows_direct_patch_denial_requests_approval"))] +#[cfg_attr(not(windows), test_case(PushedExecScenario::SandboxedDirectPatchRetry, false, false, true ; "foreign_windows_direct_patch_denial_approval_retries_unsandboxed"))] #[cfg_attr(not(windows), test_case(PushedExecScenario::UnsandboxedInterceptedPatch, false, false, true ; "foreign_windows_unsandboxed_intercepted_patch_succeeds"))] #[cfg_attr(not(windows), test_case(PushedExecScenario::FullDiskInterceptedPatch, false, false, true ; "foreign_windows_full_disk_intercepted_patch_succeeds"))] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -476,29 +550,38 @@ async fn exec_command_consumes_pushed_remote_process_events( ) -> Result<()> { let listener = TcpListener::bind("127.0.0.1:0").await?; let server = start_mock_server().await; + let tool_call = match scenario { + PushedExecScenario::SandboxedDirectPatch + | PushedExecScenario::SandboxedDirectPatchDenied + | PushedExecScenario::SandboxedDirectPatchRetry => ev_apply_patch_custom_tool_call( + CALL_ID, + "*** Begin Patch\n*** Update File: secret.txt\n@@\n-old\n+new\n*** End Patch", + ), + _ => ev_function_call( + CALL_ID, + "exec_command", + &json!({ + "cmd": match scenario { + PushedExecScenario::SandboxedInterceptedPatch => { + "apply_patch <<'PATCH'\n*** Begin Patch\n*** Update File: secret.txt\n@@\n-old\n+new\n*** End Patch\nPATCH" + } + PushedExecScenario::UnsandboxedInterceptedPatch + | PushedExecScenario::FullDiskInterceptedPatch => { + "apply_patch <<'PATCH'\n*** Begin Patch\n*** Add File: allowed.txt\n+allowed\n*** End Patch\nPATCH" + } + _ => "pwd", + }, + "yield_time_ms": 1_000, + }) + .to_string(), + ), + }; let response_mock = mount_sse_sequence( &server, vec![ sse(vec![ ev_response_created("resp-1"), - ev_function_call( - CALL_ID, - "exec_command", - &json!({ - "cmd": match scenario { - PushedExecScenario::InterceptedPatch => { - "apply_patch <<'PATCH'\n*** Begin Patch\n*** Update File: secret.txt\n@@\n-old\n+new\n*** End Patch\nPATCH" - } - PushedExecScenario::UnsandboxedInterceptedPatch - | PushedExecScenario::FullDiskInterceptedPatch => { - "apply_patch <<'PATCH'\n*** Begin Patch\n*** Add File: allowed.txt\n+allowed\n*** End Patch\nPATCH" - } - _ => "pwd", - }, - "yield_time_ms": 1_000, - }) - .to_string(), - ), + tool_call, ev_completed("resp-1"), ]), sse(vec![ @@ -622,11 +705,19 @@ timeout = 900 }], ) }), - approval_policy: Some(if managed_network { - AskForApproval::OnRequest - } else { - AskForApproval::Never - }), + approval_policy: Some( + if managed_network + || matches!( + scenario, + PushedExecScenario::SandboxedDirectPatchDenied + | PushedExecScenario::SandboxedDirectPatchRetry + ) + { + AskForApproval::OnRequest + } else { + AskForApproval::Never + }, + ), sandbox_policy: Some(sandbox_policy), permission_profile, collaboration_mode: Some(codex_protocol::config_types::CollaborationMode { @@ -642,6 +733,7 @@ timeout = 900 }) .await?; let mut saw_exec_command_begin = false; + let mut saw_patch_denial_approval = false; if !managed_network { loop { let event = timeout(Duration::from_secs(5), test.codex.next_event()) @@ -652,30 +744,113 @@ timeout = 900 EventMsg::ExecCommandBegin(event) if event.call_id == CALL_ID => { saw_exec_command_begin = true; } + EventMsg::ApplyPatchApprovalRequest(approval) + if matches!( + scenario, + PushedExecScenario::SandboxedDirectPatchDenied + | PushedExecScenario::SandboxedDirectPatchRetry + ) => + { + saw_patch_denial_approval = true; + test.codex + .submit(Op::PatchApproval { + id: approval.call_id, + decision: if matches!( + scenario, + PushedExecScenario::SandboxedDirectPatchRetry + ) { + ReviewDecision::Approved + } else { + ReviewDecision::Denied { + rejection: "denied by test".to_string(), + } + }, + }) + .await?; + } EventMsg::TurnComplete(_) => break, _ => {} } } } - if matches!(scenario, PushedExecScenario::InterceptedPatch) { + if matches!( + scenario, + PushedExecScenario::SandboxedDirectPatchDenied + | PushedExecScenario::SandboxedDirectPatchRetry + ) { + assert!( + saw_patch_denial_approval, + "executor-managed sandbox denial should request patch approval" + ); + let exec_server_result = timeout(Duration::from_secs(5), exec_server) + .await + .context("fake exec-server should observe the denied patch write")??; + assert_eq!(exec_server_result.process_start["method"], "fs/writeFile"); + if matches!(scenario, PushedExecScenario::SandboxedDirectPatchRetry) { + assert_eq!( + exec_server_result.process_start["params"]["sandbox"], + Value::Null + ); + let request = response_mock + .last_request() + .context("model should receive the approved patch result")?; + let (output, success) = request + .custom_tool_call_output_content_and_success(CALL_ID) + .context("approved patch result should be model visible")?; + assert_ne!(success, Some(false)); + assert!( + output + .context("approved patch result should contain text")? + .contains("Success. Updated the following files:") + ); + } + return Ok(()); + } + if matches!( + scenario, + PushedExecScenario::SandboxedInterceptedPatch | PushedExecScenario::SandboxedDirectPatch + ) { assert!(!saw_exec_command_begin); let request = response_mock .last_request() - .context("model should receive the intercepted patch rejection")?; - let (output, success) = request - .function_call_output_content_and_success(CALL_ID) - .context("intercepted patch rejection should be model visible")?; - assert_ne!(success, Some(true)); + .context("model should receive the sandboxed patch result")?; + let (output, success) = if matches!(scenario, PushedExecScenario::SandboxedDirectPatch) { + request.custom_tool_call_output_content_and_success(CALL_ID) + } else { + request.function_call_output_content_and_success(CALL_ID) + } + .context("sandboxed patch result should be model visible")?; + assert_ne!(success, Some(false)); assert!( output - .context("intercepted patch rejection should contain text")? - .contains("cross-platform remote apply_patch is unavailable") + .context("sandboxed patch result should contain text")? + .contains("Success. Updated the following files:") ); - assert!( - !exec_server.is_finished(), - "intercepted patch must not access the executor filesystem" + let exec_server_result = timeout(Duration::from_secs(5), exec_server) + .await + .context("fake exec-server should observe the sandboxed patch write")??; + let write_request = exec_server_result.process_start; + assert_eq!(write_request["method"], "fs/writeFile"); + assert_eq!( + write_request["params"]["path"], + "file:///C:/workspace/secret.txt" + ); + assert_eq!( + write_request["params"]["sandbox"]["windowsSandboxLevel"], + "restricted-token" + ); + assert_eq!( + write_request["params"]["sandbox"]["workspaceRoots"], + json!(["file:///C:/workspace", "file:///D:/other-workspace"]) + ); + assert_eq!( + BASE64_STANDARD.decode( + write_request["params"]["dataBase64"] + .as_str() + .expect("filesystem write should include encoded contents") + )?, + b"new\n" ); - exec_server.abort(); return Ok(()); } if matches!( @@ -786,7 +961,12 @@ timeout = 900 assert!(output.contains("Process exited with code 1")); assert_eq!(process_read_requests, 0, "unexpected compatibility read"); } - PushedExecScenario::InterceptedPatch => unreachable!("intercepted patch returned early"), + PushedExecScenario::SandboxedInterceptedPatch + | PushedExecScenario::SandboxedDirectPatch + | PushedExecScenario::SandboxedDirectPatchDenied + | PushedExecScenario::SandboxedDirectPatchRetry => { + unreachable!("sandboxed patch returned early") + } PushedExecScenario::UnsandboxedInterceptedPatch | PushedExecScenario::FullDiskInterceptedPatch => { unreachable!("unsandboxed intercepted patch returned early") diff --git a/codex-rs/exec-server/src/fs_sandbox.rs b/codex-rs/exec-server/src/fs_sandbox.rs index 303d86a0d5..6417bff1eb 100644 --- a/codex-rs/exec-server/src/fs_sandbox.rs +++ b/codex-rs/exec-server/src/fs_sandbox.rs @@ -13,6 +13,7 @@ use codex_sandboxing::SandboxDirectSpawnTransformRequest; use codex_sandboxing::SandboxExecRequest; use codex_sandboxing::SandboxManager; use codex_sandboxing::SandboxTransformRequest; +use codex_sandboxing::SandboxType; use codex_sandboxing::SandboxablePreference; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::canonicalize_preserving_symlinks; @@ -114,10 +115,15 @@ impl FileSystemSandboxRunner { let sandbox_manager = SandboxManager::new(); let sandbox = sandbox_manager.select_initial( permission_profile, - SandboxablePreference::Auto, + SandboxablePreference::Require, sandbox_context.windows_sandbox_level, /*has_managed_network_requirements*/ false, ); + if sandbox == SandboxType::None { + return Err(invalid_request( + "filesystem sandbox cannot be enforced on this executor".to_string(), + )); + } let command = SandboxCommand { program: helper.as_path().as_os_str().to_owned(), args: vec![CODEX_FS_HELPER_ARG1.to_string()], @@ -545,10 +551,11 @@ mod tests { let runner = FileSystemSandboxRunner::new(runtime_paths); let native_cwd = AbsolutePathBuf::current_dir().expect("cwd"); let cwd = PathUri::from_abs_path(&native_cwd); - let file_system_policy = restricted_policy(vec![path_entry( - native_cwd.clone(), - FileSystemAccessMode::Write, - )]); + let file_system_policy = restricted_policy(vec![ + #[cfg(windows)] + special_entry(FileSystemSpecialPath::Root, FileSystemAccessMode::Read), + path_entry(native_cwd.clone(), FileSystemAccessMode::Write), + ]); let network_policy = NetworkSandboxPolicy::Restricted; let permission_profile = PermissionProfile::from_runtime_permissions(&file_system_policy, network_policy); @@ -557,6 +564,26 @@ mod tests { uri: cwd, native: native_cwd, }; + #[cfg(windows)] + let sandbox_context = { + let error = runner + .sandbox_exec_request( + &permission_profile, + &sandbox_cwd, + std::slice::from_ref(&sandbox_cwd.native), + &sandbox_context, + ) + .expect_err("disabled Windows sandbox must not run the helper unsandboxed"); + assert_eq!( + error.message, + "filesystem sandbox cannot be enforced on this executor" + ); + crate::FileSystemSandboxContext { + windows_sandbox_level: + codex_protocol::config_types::WindowsSandboxLevel::RestrictedToken, + ..sandbox_context + } + }; let request = runner .sandbox_exec_request( diff --git a/codex-rs/exec-server/tests/file_system/shared.rs b/codex-rs/exec-server/tests/file_system/shared.rs index 236a9623ee..7d4e943802 100644 --- a/codex-rs/exec-server/tests/file_system/shared.rs +++ b/codex-rs/exec-server/tests/file_system/shared.rs @@ -756,6 +756,10 @@ pub(crate) async fn assert_sandboxed_canonicalize_resolves_directory_alias( /// Verifies that effective additional permissions extend a read-only sandbox with a writable root. #[test_case(FileSystemImplementation::Local ; "local")] #[test_case(FileSystemImplementation::Remote ; "remote")] +#[cfg_attr( + windows, + ignore = "Windows restricted-token sandbox cannot enforce split writable roots" +)] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn file_system_sandboxed_write_allows_additional_write_root( implementation: FileSystemImplementation, diff --git a/codex-rs/exec-server/tests/file_system/support.rs b/codex-rs/exec-server/tests/file_system/support.rs index 712b50f1f3..b5f13f549c 100644 --- a/codex-rs/exec-server/tests/file_system/support.rs +++ b/codex-rs/exec-server/tests/file_system/support.rs @@ -7,11 +7,13 @@ use codex_exec_server::ExecServerRuntimePaths; use codex_exec_server::ExecutorFileSystem; use codex_exec_server::FileSystemSandboxContext; use codex_exec_server::LocalFileSystem; +use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxEntry; use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::FileSystemSpecialPath; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; @@ -103,9 +105,25 @@ pub(crate) fn workspace_write_sandbox( }]) } -fn sandbox_context(entries: Vec) -> FileSystemSandboxContext { - FileSystemSandboxContext::from_permission_profile(PermissionProfile::from_runtime_permissions( - &FileSystemSandboxPolicy::restricted(entries), - NetworkSandboxPolicy::Restricted, - )) +fn sandbox_context(mut entries: Vec) -> FileSystemSandboxContext { + if cfg!(windows) { + // Restricted-token sandboxing cannot enforce read restrictions, so leave the root + // readable while exercising the requested write restrictions. + entries.push(FileSystemSandboxEntry::new( + FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + FileSystemAccessMode::Read, + )); + } + let mut sandbox = FileSystemSandboxContext::from_permission_profile( + PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::restricted(entries), + NetworkSandboxPolicy::Restricted, + ), + ); + if cfg!(windows) { + sandbox.windows_sandbox_level = WindowsSandboxLevel::RestrictedToken; + } + sandbox } diff --git a/codex-rs/sandboxing/src/denial.rs b/codex-rs/sandboxing/src/denial.rs index f9dfbb34da..507bf4de5e 100644 --- a/codex-rs/sandboxing/src/denial.rs +++ b/codex-rs/sandboxing/src/denial.rs @@ -18,34 +18,7 @@ pub fn is_likely_sandbox_denied( return false; } - // Quick rejects: well-known non-sandbox shell exit codes - // 2: misuse of shell builtins - // 126: permission denied - // 127: command not found - const SANDBOX_DENIED_KEYWORDS: [&str; 7] = [ - "operation not permitted", - "permission denied", - "read-only file system", - "seccomp", - "sandbox", - "landlock", - "failed to write file", - ]; - - let has_sandbox_keyword = [ - &exec_output.stderr.text, - &exec_output.stdout.text, - &exec_output.aggregated_output.text, - ] - .into_iter() - .any(|section| { - let lower = section.to_lowercase(); - SANDBOX_DENIED_KEYWORDS - .iter() - .any(|needle| lower.contains(needle)) - }); - - if has_sandbox_keyword { + if is_likely_executor_managed_sandbox_denied(exec_output) { return true; } @@ -67,3 +40,33 @@ pub fn is_likely_sandbox_denied( false } + +/// Detect executor-managed sandbox denials when its concrete backend is unknown. +pub fn is_likely_executor_managed_sandbox_denied(exec_output: &ExecToolCallOutput) -> bool { + if exec_output.exit_code == 0 { + return false; + } + + const SANDBOX_DENIED_KEYWORDS: [&str; 7] = [ + "operation not permitted", + "permission denied", + "read-only file system", + "seccomp", + "sandbox", + "landlock", + "failed to write file", + ]; + + [ + &exec_output.stderr.text, + &exec_output.stdout.text, + &exec_output.aggregated_output.text, + ] + .into_iter() + .any(|section| { + let lower = section.to_lowercase(); + SANDBOX_DENIED_KEYWORDS + .iter() + .any(|needle| lower.contains(needle)) + }) +} diff --git a/codex-rs/sandboxing/src/lib.rs b/codex-rs/sandboxing/src/lib.rs index 95b143c0ec..3604b196d5 100644 --- a/codex-rs/sandboxing/src/lib.rs +++ b/codex-rs/sandboxing/src/lib.rs @@ -15,6 +15,7 @@ pub use bwrap::find_system_bwrap_in_path; #[cfg(target_os = "linux")] pub use bwrap::system_bwrap_warning; pub use codex_windows_sandbox::WindowsSandboxProxySettingsMode; +pub use denial::is_likely_executor_managed_sandbox_denied; pub use denial::is_likely_sandbox_denied; pub use manager::SandboxCommand; pub use manager::SandboxDirectSpawnTransformRequest;