mirror of
https://github.com/openai/codex.git
synced 2026-04-24 06:35:50 +00:00
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_*`
This commit is contained in:
@@ -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,
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -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()
|
||||
);
|
||||
}
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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<SandboxType
|
||||
|
||||
#[derive(Debug)]
|
||||
pub struct SandboxCommand {
|
||||
pub program: String,
|
||||
pub program: OsString,
|
||||
pub args: Vec<String>,
|
||||
pub cwd: PathBuf,
|
||||
pub env: HashMap<String, String>,
|
||||
@@ -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<OsString>) -> Vec<String> {
|
||||
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()
|
||||
}
|
||||
|
||||
@@ -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(),
|
||||
|
||||
Reference in New Issue
Block a user