From d2a1941ab28ced1872376bd82245ff9e040d0f34 Mon Sep 17 00:00:00 2001 From: starr-openai Date: Tue, 7 Apr 2026 15:06:53 -0700 Subject: [PATCH] codex: fix sandbox launch config callsites Co-authored-by: Codex --- codex-rs/core/src/exec.rs | 23 ++-- codex-rs/core/src/tools/js_repl/mod.rs | 26 ++-- .../tools/runtimes/shell/unix_escalation.rs | 23 ++-- codex-rs/core/src/tools/sandboxing.rs | 23 ++-- codex-rs/exec-server/tests/exec_process.rs | 28 ++-- codex-rs/sandboxing/src/manager.rs | 53 +++----- codex-rs/sandboxing/src/manager_tests.rs | 123 ++++++++++-------- 7 files changed, 158 insertions(+), 141 deletions(-) diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index 220248910b..fc12a6255a 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -39,6 +39,7 @@ use codex_protocol::protocol::ExecCommandOutputDeltaEvent; use codex_protocol::protocol::ExecOutputStream; use codex_protocol::protocol::SandboxPolicy; use codex_sandboxing::SandboxCommand; +use codex_sandboxing::SandboxLaunchConfig; use codex_sandboxing::SandboxManager; use codex_sandboxing::SandboxType; use codex_sandboxing::SandboxablePreference; @@ -282,20 +283,24 @@ pub fn build_exec_request( expiration, capture_policy, }; + let sandbox_launch_config = SandboxLaunchConfig { + sandbox: sandbox_type, + policy: sandbox_policy.clone(), + file_system_policy: file_system_sandbox_policy.clone(), + network_policy: network_sandbox_policy, + sandbox_policy_cwd: sandbox_cwd.to_path_buf(), + additional_permissions: None, + enforce_managed_network, + windows_sandbox_level, + windows_sandbox_private_desktop, + use_legacy_landlock, + }; let mut exec_req = manager .transform( command, - sandbox_policy, - file_system_sandbox_policy, - network_sandbox_policy, - sandbox_type, - enforce_managed_network, + &sandbox_launch_config, network.as_ref(), - sandbox_cwd, codex_linux_sandbox_exe.as_deref(), - use_legacy_landlock, - windows_sandbox_level, - windows_sandbox_private_desktop, ) .map(|request| ExecRequest::from_sandbox_exec_request(request, options)) .map_err(CodexErr::from)?; diff --git a/codex-rs/core/src/tools/js_repl/mod.rs b/codex-rs/core/src/tools/js_repl/mod.rs index 5d08bc5c5c..a561c5015c 100644 --- a/codex-rs/core/src/tools/js_repl/mod.rs +++ b/codex-rs/core/src/tools/js_repl/mod.rs @@ -42,6 +42,7 @@ use crate::sandboxing::ExecOptions; use crate::tools::ToolRouter; use crate::tools::context::SharedTurnDiffTracker; use codex_sandboxing::SandboxCommand; +use codex_sandboxing::SandboxLaunchConfig; use codex_sandboxing::SandboxManager; use codex_sandboxing::SandboxablePreference; use codex_tools::ToolSpec; @@ -1057,20 +1058,27 @@ impl JsReplManager { expiration: ExecExpiration::DefaultTimeout, capture_policy: ExecCapturePolicy::ShellTool, }; + let sandbox_launch_config = SandboxLaunchConfig { + sandbox: sandbox_type, + policy: turn.sandbox_policy.get().clone(), + file_system_policy: turn.file_system_sandbox_policy.clone(), + network_policy: turn.network_sandbox_policy, + sandbox_policy_cwd: turn.cwd.to_path_buf(), + additional_permissions: None, + enforce_managed_network: has_managed_network_requirements, + windows_sandbox_level: turn.windows_sandbox_level, + windows_sandbox_private_desktop: turn + .config + .permissions + .windows_sandbox_private_desktop, + use_legacy_landlock: turn.features.use_legacy_landlock(), + }; let exec_env = sandbox .transform( command, - &turn.sandbox_policy, - &turn.file_system_sandbox_policy, - turn.network_sandbox_policy, - sandbox_type, - has_managed_network_requirements, + &sandbox_launch_config, None, - &turn.cwd, turn.codex_linux_sandbox_exe.as_deref(), - turn.features.use_legacy_landlock(), - turn.windows_sandbox_level, - turn.config.permissions.windows_sandbox_private_desktop, ) .map(|request| { crate::sandboxing::ExecRequest::from_sandbox_exec_request(request, options) diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index 16858fb098..0010f093f9 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -33,6 +33,7 @@ use codex_protocol::protocol::NetworkPolicyRuleAction; use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::SandboxPolicy; use codex_sandboxing::SandboxCommand; +use codex_sandboxing::SandboxLaunchConfig; use codex_sandboxing::SandboxManager; use codex_sandboxing::SandboxType; use codex_sandboxing::SandboxablePreference; @@ -834,19 +835,23 @@ impl CoreShellCommandExecutor { expiration: ExecExpiration::DefaultTimeout, capture_policy: ExecCapturePolicy::ShellTool, }; + let sandbox_launch_config = SandboxLaunchConfig { + sandbox, + policy: sandbox_policy.clone(), + file_system_policy: file_system_sandbox_policy.clone(), + network_policy: network_sandbox_policy, + sandbox_policy_cwd: self.sandbox_policy_cwd.clone(), + additional_permissions: None, + enforce_managed_network: self.network.is_some(), + windows_sandbox_level: self.windows_sandbox_level, + windows_sandbox_private_desktop: false, + use_legacy_landlock: self.use_legacy_landlock, + }; let exec_request = sandbox_manager.transform( command, - sandbox_policy, - file_system_sandbox_policy, - network_sandbox_policy, - sandbox, - self.network.is_some(), + &sandbox_launch_config, self.network.as_ref(), - &self.sandbox_policy_cwd, self.codex_linux_sandbox_exe.as_deref(), - self.use_legacy_landlock, - self.windows_sandbox_level, - false, )?; let mut exec_request = crate::sandboxing::ExecRequest::from_sandbox_exec_request(exec_request, options); diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 88106169e1..9d86a823b1 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -22,6 +22,7 @@ use codex_protocol::protocol::ReviewDecision; #[cfg(test)] use codex_protocol::protocol::SandboxPolicy; use codex_sandboxing::SandboxCommand; +use codex_sandboxing::SandboxLaunchConfig; use codex_sandboxing::SandboxManager; use codex_sandboxing::SandboxTransformError; use codex_sandboxing::SandboxType; @@ -337,21 +338,25 @@ impl<'a> SandboxAttempt<'a> { options: ExecOptions, network: Option<&NetworkProxy>, ) -> Result { + let sandbox_launch_config = SandboxLaunchConfig { + sandbox: self.sandbox, + policy: self.policy.clone(), + file_system_policy: self.file_system_policy.clone(), + network_policy: self.network_policy, + sandbox_policy_cwd: self.sandbox_cwd.to_path_buf(), + additional_permissions: None, + enforce_managed_network: self.enforce_managed_network, + windows_sandbox_level: self.windows_sandbox_level, + windows_sandbox_private_desktop: self.windows_sandbox_private_desktop, + use_legacy_landlock: self.use_legacy_landlock, + }; self.manager .transform( command, - self.policy, - self.file_system_policy, - self.network_policy, - self.sandbox, - self.enforce_managed_network, + &sandbox_launch_config, network, - self.sandbox_cwd, self.codex_linux_sandbox_exe .map(std::path::PathBuf::as_path), - self.use_legacy_landlock, - self.windows_sandbox_level, - self.windows_sandbox_private_desktop, ) .map(|request| { crate::sandboxing::ExecRequest::from_sandbox_exec_request(request, options) diff --git a/codex-rs/exec-server/tests/exec_process.rs b/codex-rs/exec-server/tests/exec_process.rs index e35f149c28..eb7ba91523 100644 --- a/codex-rs/exec-server/tests/exec_process.rs +++ b/codex-rs/exec-server/tests/exec_process.rs @@ -52,18 +52,17 @@ async fn create_process_context(use_remote: bool) -> Result { async fn assert_exec_process_starts_and_exits(use_remote: bool) -> Result<()> { let context = create_process_context(use_remote).await?; + let cwd = std::env::current_dir()?; let session = context .backend .start(ExecParams { process_id: ProcessId::from("proc-1"), argv: vec!["true".to_string()], - cwd: std::env::current_dir()?, + cwd: cwd.clone(), env: Default::default(), tty: false, arg0: None, - sandbox: SandboxLaunchConfig::no_sandbox( - std::env::current_dir().expect("read current dir"), - ), + sandbox: SandboxLaunchConfig::no_sandbox(cwd), }) .await?; assert_eq!(session.process.process_id().as_str(), "proc-1"); @@ -126,6 +125,7 @@ async fn collect_process_output_from_reads( async fn assert_exec_process_streams_output(use_remote: bool) -> Result<()> { let context = create_process_context(use_remote).await?; + let cwd = std::env::current_dir()?; let process_id = "proc-stream".to_string(); let session = context .backend @@ -136,13 +136,11 @@ async fn assert_exec_process_streams_output(use_remote: bool) -> Result<()> { "-c".to_string(), "sleep 0.05; printf 'session output\\n'".to_string(), ], - cwd: std::env::current_dir()?, + cwd: cwd.clone(), env: Default::default(), tty: false, arg0: None, - sandbox: SandboxLaunchConfig::no_sandbox( - std::env::current_dir().expect("read current dir"), - ), + sandbox: SandboxLaunchConfig::no_sandbox(cwd), }) .await?; assert_eq!(session.process.process_id().as_str(), process_id); @@ -158,6 +156,7 @@ async fn assert_exec_process_streams_output(use_remote: bool) -> Result<()> { async fn assert_exec_process_write_then_read(use_remote: bool) -> Result<()> { let context = create_process_context(use_remote).await?; + let cwd = std::env::current_dir()?; let process_id = "proc-stdin".to_string(); let session = context .backend @@ -168,11 +167,11 @@ async fn assert_exec_process_write_then_read(use_remote: bool) -> Result<()> { "-c".to_string(), "import sys; line = sys.stdin.readline(); sys.stdout.write(f'from-stdin:{line}'); sys.stdout.flush()".to_string(), ], - cwd: std::env::current_dir()?, + cwd: cwd.clone(), env: Default::default(), tty: true, arg0: None, - sandbox: SandboxLaunchConfig::no_sandbox(std::env::current_dir().expect("read current dir")), + sandbox: SandboxLaunchConfig::no_sandbox(cwd), }) .await?; assert_eq!(session.process.process_id().as_str(), process_id); @@ -196,6 +195,7 @@ async fn assert_exec_process_preserves_queued_events_before_subscribe( use_remote: bool, ) -> Result<()> { let context = create_process_context(use_remote).await?; + let cwd = std::env::current_dir()?; let session = context .backend .start(ExecParams { @@ -205,13 +205,11 @@ async fn assert_exec_process_preserves_queued_events_before_subscribe( "-c".to_string(), "printf 'queued output\\n'".to_string(), ], - cwd: std::env::current_dir()?, + cwd: cwd.clone(), env: Default::default(), tty: false, arg0: None, - sandbox: SandboxLaunchConfig::no_sandbox( - std::env::current_dir().expect("read current dir"), - ), + sandbox: SandboxLaunchConfig::no_sandbox(cwd), }) .await?; @@ -278,7 +276,7 @@ async fn assert_exec_process_sandbox_denies_write_outside_workspace( env: Default::default(), tty: false, arg0: None, - sandbox: Some(write_outside_workspace_sandbox(&workspace_root)), + sandbox: write_outside_workspace_sandbox(&workspace_root), }) .await?; diff --git a/codex-rs/sandboxing/src/manager.rs b/codex-rs/sandboxing/src/manager.rs index 40016cb66b..e3a5490fcc 100644 --- a/codex-rs/sandboxing/src/manager.rs +++ b/codex-rs/sandboxing/src/manager.rs @@ -87,20 +87,7 @@ impl SandboxLaunchConfig { network: Option<&NetworkProxy>, codex_linux_sandbox_exe: Option<&Path>, ) -> Result { - SandboxManager::new().transform( - command, - &self.policy, - &self.file_system_policy, - self.network_policy, - self.sandbox, - self.enforce_managed_network, - network, - self.sandbox_policy_cwd.as_path(), - codex_linux_sandbox_exe, - self.use_legacy_landlock, - self.windows_sandbox_level, - self.windows_sandbox_private_desktop, - ) + SandboxManager::new().transform(command, self, network, codex_linux_sandbox_exe) } } @@ -205,33 +192,27 @@ impl SandboxManager { pub fn transform( &self, mut command: SandboxCommand, - policy: &SandboxPolicy, - file_system_policy: &FileSystemSandboxPolicy, - network_policy: NetworkSandboxPolicy, - sandbox: SandboxType, - enforce_managed_network: bool, + launch: &SandboxLaunchConfig, network: Option<&NetworkProxy>, - sandbox_policy_cwd: &Path, codex_linux_sandbox_exe: Option<&Path>, - use_legacy_landlock: bool, - windows_sandbox_level: WindowsSandboxLevel, - windows_sandbox_private_desktop: bool, ) -> Result { let additional_permissions = command.additional_permissions.take(); let EffectiveSandboxPermissions { sandbox_policy: effective_policy, - } = EffectiveSandboxPermissions::new(policy, additional_permissions.as_ref()); + } = EffectiveSandboxPermissions::new(&launch.policy, additional_permissions.as_ref()); let effective_file_system_policy = effective_file_system_sandbox_policy( - file_system_policy, + &launch.file_system_policy, + additional_permissions.as_ref(), + ); + let effective_network_policy = effective_network_sandbox_policy( + launch.network_policy, additional_permissions.as_ref(), ); - let effective_network_policy = - effective_network_sandbox_policy(network_policy, additional_permissions.as_ref()); let mut argv = Vec::with_capacity(1 + command.args.len()); argv.push(command.program); argv.extend(command.args.into_iter().map(OsString::from)); - let (argv, arg0_override) = match sandbox { + let (argv, arg0_override) = match launch.sandbox { SandboxType::None => (os_argv_to_strings(argv), None), #[cfg(target_os = "macos")] SandboxType::MacosSeatbelt => { @@ -239,8 +220,8 @@ impl SandboxManager { os_argv_to_strings(argv), &effective_file_system_policy, effective_network_policy, - sandbox_policy_cwd, - enforce_managed_network, + launch.sandbox_policy_cwd.as_path(), + launch.enforce_managed_network, network, ); let mut full_command = Vec::with_capacity(1 + args.len()); @@ -253,15 +234,15 @@ impl SandboxManager { SandboxType::LinuxSeccomp => { let exe = codex_linux_sandbox_exe .ok_or(SandboxTransformError::MissingLinuxSandboxExecutable)?; - let allow_proxy_network = allow_network_for_proxy(enforce_managed_network); + let allow_proxy_network = allow_network_for_proxy(launch.enforce_managed_network); let mut args = create_linux_sandbox_command_args_for_policies( os_argv_to_strings(argv), command.cwd.as_path(), &effective_policy, &effective_file_system_policy, effective_network_policy, - sandbox_policy_cwd, - use_legacy_landlock, + launch.sandbox_policy_cwd.as_path(), + launch.use_legacy_landlock, allow_proxy_network, ); let mut full_command = Vec::with_capacity(1 + args.len()); @@ -280,9 +261,9 @@ impl SandboxManager { cwd: command.cwd, env: command.env, network: network.cloned(), - sandbox, - windows_sandbox_level, - windows_sandbox_private_desktop, + sandbox: launch.sandbox, + windows_sandbox_level: launch.windows_sandbox_level, + windows_sandbox_private_desktop: launch.windows_sandbox_private_desktop, sandbox_policy: effective_policy, file_system_sandbox_policy: effective_file_system_policy, network_sandbox_policy: effective_network_policy, diff --git a/codex-rs/sandboxing/src/manager_tests.rs b/codex-rs/sandboxing/src/manager_tests.rs index dbbf50b087..e94545fef2 100644 --- a/codex-rs/sandboxing/src/manager_tests.rs +++ b/codex-rs/sandboxing/src/manager_tests.rs @@ -1,4 +1,5 @@ use super::SandboxCommand; +use super::SandboxLaunchConfig; use super::SandboxManager; use super::SandboxType; use super::SandboxablePreference; @@ -20,8 +21,30 @@ use codex_utils_absolute_path::AbsolutePathBuf; use dunce::canonicalize; use pretty_assertions::assert_eq; use std::collections::HashMap; +use std::path::Path; use tempfile::TempDir; +fn test_launch_config( + cwd: &Path, + policy: SandboxPolicy, + file_system_policy: FileSystemSandboxPolicy, + network_policy: NetworkSandboxPolicy, + sandbox: SandboxType, +) -> SandboxLaunchConfig { + SandboxLaunchConfig { + sandbox, + policy, + file_system_policy, + network_policy, + sandbox_policy_cwd: cwd.to_path_buf(), + additional_permissions: None, + enforce_managed_network: false, + windows_sandbox_level: WindowsSandboxLevel::Disabled, + windows_sandbox_private_desktop: false, + use_legacy_landlock: false, + } +} + #[test] fn danger_full_access_defaults_to_no_sandbox_without_network_requirements() { let manager = SandboxManager::new(); @@ -83,19 +106,17 @@ fn transform_preserves_unrestricted_file_system_policy_for_restricted_network() env: HashMap::new(), additional_permissions: None, }, - &SandboxPolicy::ExternalSandbox { - network_access: NetworkAccess::Restricted, - }, - &FileSystemSandboxPolicy::unrestricted(), - NetworkSandboxPolicy::Restricted, - SandboxType::None, - false, + &test_launch_config( + cwd.as_path(), + SandboxPolicy::ExternalSandbox { + network_access: NetworkAccess::Restricted, + }, + FileSystemSandboxPolicy::unrestricted(), + NetworkSandboxPolicy::Restricted, + SandboxType::None, + ), None, - cwd.as_path(), None, - false, - WindowsSandboxLevel::Disabled, - false, ) .expect("transform"); @@ -135,19 +156,17 @@ fn transform_additional_permissions_enable_network_for_external_sandbox() { }), }), }, - &SandboxPolicy::ExternalSandbox { - network_access: NetworkAccess::Restricted, - }, - &FileSystemSandboxPolicy::unrestricted(), - NetworkSandboxPolicy::Restricted, - SandboxType::None, - false, + &test_launch_config( + cwd.as_path(), + SandboxPolicy::ExternalSandbox { + network_access: NetworkAccess::Restricted, + }, + FileSystemSandboxPolicy::unrestricted(), + NetworkSandboxPolicy::Restricted, + SandboxType::None, + ), None, - cwd.as_path(), None, - false, - WindowsSandboxLevel::Disabled, - false, ) .expect("transform"); @@ -189,33 +208,31 @@ fn transform_additional_permissions_preserves_denied_entries() { ..Default::default() }), }, - &SandboxPolicy::ReadOnly { - access: ReadOnlyAccess::FullAccess, - network_access: false, - }, - &FileSystemSandboxPolicy::restricted(vec![ - FileSystemSandboxEntry { - path: FileSystemPath::Special { - value: FileSystemSpecialPath::Root, - }, - access: FileSystemAccessMode::Read, + &test_launch_config( + cwd.as_path(), + SandboxPolicy::ReadOnly { + access: ReadOnlyAccess::FullAccess, + network_access: false, }, - FileSystemSandboxEntry { - path: FileSystemPath::Path { - path: denied_path.clone(), + FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, }, - access: FileSystemAccessMode::None, - }, - ]), - NetworkSandboxPolicy::Restricted, - SandboxType::None, - false, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: denied_path.clone(), + }, + access: FileSystemAccessMode::None, + }, + ]), + NetworkSandboxPolicy::Restricted, + SandboxType::None, + ), None, - cwd.as_path(), None, - false, - WindowsSandboxLevel::Disabled, - false, ) .expect("transform"); @@ -259,17 +276,15 @@ fn transform_linux_seccomp_request( env: HashMap::new(), additional_permissions: None, }, - &SandboxPolicy::DangerFullAccess, - &FileSystemSandboxPolicy::unrestricted(), - NetworkSandboxPolicy::Enabled, - SandboxType::LinuxSeccomp, - false, + &test_launch_config( + cwd.as_path(), + SandboxPolicy::DangerFullAccess, + FileSystemSandboxPolicy::unrestricted(), + NetworkSandboxPolicy::Enabled, + SandboxType::LinuxSeccomp, + ), None, - cwd.as_path(), Some(codex_linux_sandbox_exe), - false, - WindowsSandboxLevel::Disabled, - false, ) .expect("transform") }