mirror of
https://github.com/openai/codex.git
synced 2026-05-01 01:47:18 +00:00
## Why `#13434` introduces split `FileSystemSandboxPolicy` and `NetworkSandboxPolicy`, but the runtime still made most execution-time sandbox decisions from the legacy `SandboxPolicy` projection. That projection loses information about combinations like unrestricted filesystem access with restricted network access. In practice, that means the runtime can choose the wrong platform sandbox behavior or set the wrong network-restriction environment for a command even when config has already separated those concerns. This PR carries the split policies through the runtime so sandbox selection, process spawning, and exec handling can consult the policy that actually matters. ## What changed - threaded `FileSystemSandboxPolicy` and `NetworkSandboxPolicy` through `TurnContext`, `ExecRequest`, sandbox attempts, shell escalation state, unified exec, and app-server exec overrides - updated sandbox selection in `core/src/sandboxing/mod.rs` and `core/src/exec.rs` to key off `FileSystemSandboxPolicy.kind` plus `NetworkSandboxPolicy`, rather than inferring behavior only from the legacy `SandboxPolicy` - updated process spawning in `core/src/spawn.rs` and the platform wrappers to use `NetworkSandboxPolicy` when deciding whether to set `CODEX_SANDBOX_NETWORK_DISABLED` - kept additional-permissions handling and legacy `ExternalSandbox` compatibility projections aligned with the split policies, including explicit user-shell execution and Windows restricted-token routing - updated callers across `core`, `app-server`, and `linux-sandbox` to pass the split policies explicitly ## Verification - added regression coverage in `core/tests/suite/user_shell_cmd.rs` to verify `RunUserShellCommand` does not inherit `CODEX_SANDBOX_NETWORK_DISABLED` from the active turn - added coverage in `core/src/exec.rs` for Windows restricted-token sandbox selection when the legacy projection is `ExternalSandbox` - updated Linux sandbox coverage in `linux-sandbox/tests/suite/landlock.rs` to exercise the split-policy exec path - verified the current PR state with `just clippy` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/13439). * #13453 * #13452 * #13451 * #13449 * #13448 * #13445 * #13440 * __->__ #13439 --------- Co-authored-by: viyatb-oai <viyatb@openai.com>
126 lines
4.3 KiB
Rust
126 lines
4.3 KiB
Rust
use codex_network_proxy::NetworkProxy;
|
|
use std::collections::HashMap;
|
|
use std::path::PathBuf;
|
|
use std::process::Stdio;
|
|
use tokio::process::Child;
|
|
use tokio::process::Command;
|
|
use tracing::trace;
|
|
|
|
use codex_protocol::permissions::NetworkSandboxPolicy;
|
|
|
|
/// Experimental environment variable that will be set to some non-empty value
|
|
/// if both of the following are true:
|
|
///
|
|
/// 1. The process was spawned by Codex as part of a shell tool call.
|
|
/// 2. NetworkSandboxPolicy is restricted for the tool call.
|
|
///
|
|
/// We may try to have just one environment variable for all sandboxing
|
|
/// attributes, so this may change in the future.
|
|
pub const CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR: &str = "CODEX_SANDBOX_NETWORK_DISABLED";
|
|
|
|
/// Should be set when the process is spawned under a sandbox. Currently, the
|
|
/// value is "seatbelt" for macOS, but it may change in the future to
|
|
/// accommodate sandboxing configuration and other sandboxing mechanisms.
|
|
pub const CODEX_SANDBOX_ENV_VAR: &str = "CODEX_SANDBOX";
|
|
|
|
#[derive(Debug, Clone, Copy)]
|
|
pub enum StdioPolicy {
|
|
RedirectForShellTool,
|
|
Inherit,
|
|
}
|
|
|
|
/// Spawns the appropriate child process for the ExecParams and SandboxPolicy,
|
|
/// ensuring the args and environment variables used to create the `Command`
|
|
/// (and `Child`) honor the configuration.
|
|
///
|
|
/// For now, we take `NetworkSandboxPolicy` as a parameter to spawn_child()
|
|
/// because we need to determine whether to set the
|
|
/// `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR` environment variable.
|
|
pub(crate) struct SpawnChildRequest<'a> {
|
|
pub program: PathBuf,
|
|
pub args: Vec<String>,
|
|
pub arg0: Option<&'a str>,
|
|
pub cwd: PathBuf,
|
|
pub network_sandbox_policy: NetworkSandboxPolicy,
|
|
pub network: Option<&'a NetworkProxy>,
|
|
pub stdio_policy: StdioPolicy,
|
|
pub env: HashMap<String, String>,
|
|
}
|
|
|
|
pub(crate) async fn spawn_child_async(request: SpawnChildRequest<'_>) -> std::io::Result<Child> {
|
|
let SpawnChildRequest {
|
|
program,
|
|
args,
|
|
arg0,
|
|
cwd,
|
|
network_sandbox_policy,
|
|
network,
|
|
stdio_policy,
|
|
mut env,
|
|
} = request;
|
|
|
|
trace!(
|
|
"spawn_child_async: {program:?} {args:?} {arg0:?} {cwd:?} {network_sandbox_policy:?} {stdio_policy:?} {env:?}"
|
|
);
|
|
|
|
let mut cmd = Command::new(&program);
|
|
#[cfg(unix)]
|
|
cmd.arg0(arg0.map_or_else(|| program.to_string_lossy().to_string(), String::from));
|
|
cmd.args(args);
|
|
cmd.current_dir(cwd);
|
|
if let Some(network) = network {
|
|
network.apply_to_env(&mut env);
|
|
}
|
|
cmd.env_clear();
|
|
cmd.envs(env);
|
|
|
|
if !network_sandbox_policy.is_enabled() {
|
|
cmd.env(CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR, "1");
|
|
}
|
|
|
|
// If this Codex process dies (including being killed via SIGKILL), we want
|
|
// any child processes that were spawned as part of a `"shell"` tool call
|
|
// to also be terminated.
|
|
|
|
#[cfg(unix)]
|
|
unsafe {
|
|
let detach_from_tty = matches!(stdio_policy, StdioPolicy::RedirectForShellTool);
|
|
#[cfg(target_os = "linux")]
|
|
let parent_pid = libc::getpid();
|
|
cmd.pre_exec(move || {
|
|
if detach_from_tty {
|
|
codex_utils_pty::process_group::detach_from_tty()?;
|
|
}
|
|
|
|
// This relies on prctl(2), so it only works on Linux.
|
|
#[cfg(target_os = "linux")]
|
|
{
|
|
// This prctl call effectively requests, "deliver SIGTERM when my
|
|
// current parent dies."
|
|
codex_utils_pty::process_group::set_parent_death_signal(parent_pid)?;
|
|
}
|
|
Ok(())
|
|
});
|
|
}
|
|
|
|
match stdio_policy {
|
|
StdioPolicy::RedirectForShellTool => {
|
|
// Do not create a file descriptor for stdin because otherwise some
|
|
// commands may hang forever waiting for input. For example, ripgrep has
|
|
// a heuristic where it may try to read from stdin as explained here:
|
|
// https://github.com/BurntSushi/ripgrep/blob/e2362d4d5185d02fa857bf381e7bd52e66fafc73/crates/core/flags/hiargs.rs#L1101-L1103
|
|
cmd.stdin(Stdio::null());
|
|
|
|
cmd.stdout(Stdio::piped()).stderr(Stdio::piped());
|
|
}
|
|
StdioPolicy::Inherit => {
|
|
// Inherit stdin, stdout, and stderr from the parent process.
|
|
cmd.stdin(Stdio::inherit())
|
|
.stdout(Stdio::inherit())
|
|
.stderr(Stdio::inherit());
|
|
}
|
|
}
|
|
|
|
cmd.kill_on_drop(true).spawn()
|
|
}
|