From dfb36573cd0c6aed5a61d6e11ed6d34a34e160ff Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 26 Mar 2026 13:38:33 -0700 Subject: [PATCH] sandboxing: use OsString for SandboxCommand.program (#15897) ## Why `SandboxCommand.program` represents an executable path, but keeping it as `String` forced path-backed callers to run `to_string_lossy()` before the sandbox layer ever touched the command. That loses fidelity earlier than necessary and adds avoidable conversions in runtimes that already have a `PathBuf`. ## What changed - Changed `SandboxCommand.program` to `OsString`. - Updated `SandboxManager::transform` to keep the program and argv in `OsString` form until the `SandboxExecRequest` conversion boundary. - Switched the path-backed `apply_patch` and `js_repl` runtimes to pass `into_os_string()` instead of `to_string_lossy()`. - Updated the remaining string-backed builders and tests to match the new type while preserving the existing Linux helper `arg0` behavior. ## Verification - `cargo test -p codex-sandboxing` - `just argument-comment-lint -p codex-core -p codex-sandboxing` - `cargo test -p codex-core` currently fails in unrelated existing config tests: `config::tests::approvals_reviewer_*` and `config::tests::smart_approvals_alias_*` --- codex-rs/core/src/exec.rs | 2 +- codex-rs/core/src/tools/js_repl/mod.rs | 2 +- .../core/src/tools/runtimes/apply_patch.rs | 2 +- .../src/tools/runtimes/apply_patch_tests.rs | 8 ++--- codex-rs/core/src/tools/runtimes/mod.rs | 2 +- .../tools/runtimes/shell/unix_escalation.rs | 2 +- codex-rs/sandboxing/src/manager.rs | 31 +++++++++++++------ codex-rs/sandboxing/src/manager_tests.rs | 8 ++--- 8 files changed, 33 insertions(+), 24 deletions(-) diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index a4399ddc4..4291b6cca 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -272,7 +272,7 @@ pub fn build_exec_request( let manager = SandboxManager::new(); let command = SandboxCommand { - program: program.clone(), + program: program.clone().into(), args: args.to_vec(), cwd, env, diff --git a/codex-rs/core/src/tools/js_repl/mod.rs b/codex-rs/core/src/tools/js_repl/mod.rs index 266aa2a1c..a9e3aa0f2 100644 --- a/codex-rs/core/src/tools/js_repl/mod.rs +++ b/codex-rs/core/src/tools/js_repl/mod.rs @@ -1045,7 +1045,7 @@ impl JsReplManager { has_managed_network_requirements, ); let command = SandboxCommand { - program: node_path.to_string_lossy().to_string(), + program: node_path.into_os_string(), args: vec![ "--experimental-vm-modules".to_string(), kernel_path.to_string_lossy().to_string(), diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index c476d4773..65a3981cc 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -97,7 +97,7 @@ impl ApplyPatchRuntime { fn build_sandbox_command_with_program(req: &ApplyPatchRequest, exe: PathBuf) -> SandboxCommand { SandboxCommand { - program: exe.to_string_lossy().to_string(), + program: exe.into_os_string(), args: vec![ CODEX_CORE_APPLY_PATCH_ARG1.to_string(), req.action.patch.clone(), 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 e38bf5bb0..589e5216d 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs @@ -98,10 +98,7 @@ fn build_sandbox_command_prefers_configured_codex_self_exe_for_apply_patch() { let command = ApplyPatchRuntime::build_sandbox_command(&request, Some(&codex_self_exe)) .expect("build sandbox command"); - assert_eq!( - command.program, - codex_self_exe.to_string_lossy().to_string() - ); + assert_eq!(command.program, codex_self_exe.into_os_string()); } #[cfg(not(target_os = "windows"))] @@ -136,7 +133,6 @@ fn build_sandbox_command_falls_back_to_current_exe_for_apply_patch() { command.program, std::env::current_exe() .expect("current exe") - .to_string_lossy() - .to_string() + .into_os_string() ); } diff --git a/codex-rs/core/src/tools/runtimes/mod.rs b/codex-rs/core/src/tools/runtimes/mod.rs index c1a5ad56f..53890a89b 100644 --- a/codex-rs/core/src/tools/runtimes/mod.rs +++ b/codex-rs/core/src/tools/runtimes/mod.rs @@ -28,7 +28,7 @@ pub(crate) fn build_sandbox_command( .split_first() .ok_or_else(|| ToolError::Rejected("command args are empty".to_string()))?; Ok(SandboxCommand { - program: program.clone(), + program: program.clone().into(), args: args.to_vec(), cwd: cwd.to_path_buf(), env: env.clone(), 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 baef00d33..b2eaa7a03 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -864,7 +864,7 @@ impl CoreShellCommandExecutor { self.network.is_some(), ); let command = SandboxCommand { - program: program.clone(), + program: program.clone().into(), args: args.to_vec(), cwd: workdir.to_path_buf(), env, diff --git a/codex-rs/sandboxing/src/manager.rs b/codex-rs/sandboxing/src/manager.rs index 3d286eb64..3f121ba41 100644 --- a/codex-rs/sandboxing/src/manager.rs +++ b/codex-rs/sandboxing/src/manager.rs @@ -18,6 +18,7 @@ use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::SandboxPolicy; use std::collections::HashMap; +use std::ffi::OsString; use std::path::Path; use std::path::PathBuf; @@ -65,7 +66,7 @@ pub fn get_platform_sandbox(windows_sandbox_enabled: bool) -> Option, pub cwd: PathBuf, pub env: HashMap, @@ -209,14 +210,14 @@ impl SandboxManager { 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.append(&mut command.args); + argv.extend(command.args.into_iter().map(OsString::from)); let (argv, arg0_override) = match sandbox { - SandboxType::None => (argv, None), + SandboxType::None => (os_argv_to_strings(argv), None), #[cfg(target_os = "macos")] SandboxType::MacosSeatbelt => { let mut args = create_seatbelt_command_args_for_policies_with_extensions( - argv.clone(), + os_argv_to_strings(argv), &effective_file_system_policy, effective_network_policy, sandbox_policy_cwd, @@ -236,7 +237,7 @@ impl SandboxManager { .ok_or(SandboxTransformError::MissingLinuxSandboxExecutable)?; let allow_proxy_network = allow_network_for_proxy(enforce_managed_network); let mut args = create_linux_sandbox_command_args_for_policies( - argv.clone(), + os_argv_to_strings(argv), command.cwd.as_path(), &effective_policy, &effective_file_system_policy, @@ -246,7 +247,7 @@ impl SandboxManager { allow_proxy_network, ); let mut full_command = Vec::with_capacity(1 + args.len()); - full_command.push(exe.to_string_lossy().to_string()); + full_command.push(os_string_to_command_component(exe.as_os_str().to_owned())); full_command.append(&mut args); ( full_command, @@ -254,9 +255,9 @@ impl SandboxManager { ) } #[cfg(target_os = "windows")] - SandboxType::WindowsRestrictedToken => (argv, None), + SandboxType::WindowsRestrictedToken => (os_argv_to_strings(argv), None), #[cfg(not(target_os = "windows"))] - SandboxType::WindowsRestrictedToken => (argv, None), + SandboxType::WindowsRestrictedToken => (os_argv_to_strings(argv), None), }; Ok(SandboxExecRequest { @@ -275,9 +276,21 @@ impl SandboxManager { } } +fn os_argv_to_strings(argv: Vec) -> Vec { + argv.into_iter() + .map(os_string_to_command_component) + .collect() +} + +fn os_string_to_command_component(value: OsString) -> String { + value + .into_string() + .unwrap_or_else(|value| value.to_string_lossy().into_owned()) +} + fn linux_sandbox_arg0_override(exe: &Path) -> String { if exe.file_name().and_then(|name| name.to_str()) == Some(CODEX_LINUX_SANDBOX_ARG0) { - exe.to_string_lossy().into_owned() + os_string_to_command_component(exe.as_os_str().to_owned()) } else { CODEX_LINUX_SANDBOX_ARG0.to_string() } diff --git a/codex-rs/sandboxing/src/manager_tests.rs b/codex-rs/sandboxing/src/manager_tests.rs index 690aec89d..67911bff6 100644 --- a/codex-rs/sandboxing/src/manager_tests.rs +++ b/codex-rs/sandboxing/src/manager_tests.rs @@ -76,7 +76,7 @@ fn transform_preserves_unrestricted_file_system_policy_for_restricted_network() let exec_request = manager .transform(SandboxTransformRequest { command: SandboxCommand { - program: "true".to_string(), + program: "true".into(), args: Vec::new(), cwd: cwd.clone(), env: HashMap::new(), @@ -122,7 +122,7 @@ fn transform_additional_permissions_enable_network_for_external_sandbox() { let exec_request = manager .transform(SandboxTransformRequest { command: SandboxCommand { - program: "true".to_string(), + program: "true".into(), args: Vec::new(), cwd: cwd.clone(), env: HashMap::new(), @@ -181,7 +181,7 @@ fn transform_additional_permissions_preserves_denied_entries() { let exec_request = manager .transform(SandboxTransformRequest { command: SandboxCommand { - program: "true".to_string(), + program: "true".into(), args: Vec::new(), cwd: cwd.clone(), env: HashMap::new(), @@ -259,7 +259,7 @@ fn transform_linux_seccomp_request( manager .transform(SandboxTransformRequest { command: SandboxCommand { - program: "true".to_string(), + program: "true".into(), args: Vec::new(), cwd: cwd.clone(), env: HashMap::new(),