Address exec-server sandbox review comments

- preserve sandbox child environment variables from shared launch requests
- dispatch codex-exec-server through codex-arg0 so helper arg0 is executable
- send sandbox preference to remote servers and report the server-selected sandbox

Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
starr-openai
2026-04-08 10:53:18 -07:00
parent 5824352d53
commit a69385f63f
23 changed files with 140 additions and 110 deletions

View File

@@ -284,7 +284,7 @@ pub fn build_exec_request(
capture_policy,
};
let sandbox_launch_config = SandboxLaunchConfig {
sandbox: sandbox_type,
sandbox_preference: SandboxablePreference::from_selected_sandbox(sandbox_type),
policy: sandbox_policy.clone(),
file_system_policy: file_system_sandbox_policy.clone(),
network_policy: network_sandbox_policy,

View File

@@ -12,9 +12,6 @@ use crate::exec::ExecExpiration;
use crate::exec::StdoutStream;
use crate::exec::WindowsRestrictedTokenFilesystemOverlay;
use crate::exec::execute_exec_request;
#[cfg(target_os = "macos")]
use crate::spawn::CODEX_SANDBOX_ENV_VAR;
use crate::spawn::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR;
use codex_network_proxy::NetworkProxy;
use codex_protocol::config_types::WindowsSandboxLevel;
use codex_protocol::exec_output::ExecToolCallOutput;
@@ -88,13 +85,14 @@ impl ExecRequest {
}
pub(crate) fn from_sandbox_exec_request(
request: SandboxExecRequest,
mut request: SandboxExecRequest,
options: ExecOptions,
) -> Self {
request.prepare_env_for_spawn();
let SandboxExecRequest {
command,
cwd,
mut env,
env,
network,
sandbox,
windows_sandbox_level,
@@ -108,16 +106,6 @@ impl ExecRequest {
expiration,
capture_policy,
} = options;
if !network_sandbox_policy.is_enabled() {
env.insert(
CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR.to_string(),
"1".to_string(),
);
}
#[cfg(target_os = "macos")]
if sandbox == SandboxType::MacosSeatbelt {
env.insert(CODEX_SANDBOX_ENV_VAR.to_string(), "seatbelt".to_string());
}
Self {
command,
cwd,

View File

@@ -1059,7 +1059,7 @@ impl JsReplManager {
capture_policy: ExecCapturePolicy::ShellTool,
};
let sandbox_launch_config = SandboxLaunchConfig {
sandbox: sandbox_type,
sandbox_preference: SandboxablePreference::from_selected_sandbox(sandbox_type),
policy: turn.sandbox_policy.get().clone(),
file_system_policy: turn.file_system_sandbox_policy.clone(),
network_policy: turn.network_sandbox_policy,

View File

@@ -836,7 +836,7 @@ impl CoreShellCommandExecutor {
capture_policy: ExecCapturePolicy::ShellTool,
};
let sandbox_launch_config = SandboxLaunchConfig {
sandbox,
sandbox_preference: SandboxablePreference::from_selected_sandbox(sandbox),
policy: sandbox_policy.clone(),
file_system_policy: file_system_sandbox_policy.clone(),
network_policy: network_sandbox_policy,

View File

@@ -87,12 +87,8 @@ fn build_remote_exec_sandbox_config(
attempt: &SandboxAttempt<'_>,
additional_permissions: Option<PermissionProfile>,
) -> SandboxLaunchConfig {
if matches!(attempt.sandbox, codex_sandboxing::SandboxType::None) {
return SandboxLaunchConfig::no_sandbox(attempt.sandbox_cwd.to_path_buf());
}
SandboxLaunchConfig {
sandbox: attempt.sandbox,
sandbox_preference: SandboxablePreference::from_selected_sandbox(attempt.sandbox),
policy: attempt.policy.clone(),
file_system_policy: attempt.file_system_policy.clone(),
network_policy: attempt.network_policy,

View File

@@ -339,7 +339,7 @@ impl<'a> SandboxAttempt<'a> {
network: Option<&NetworkProxy>,
) -> Result<crate::sandboxing::ExecRequest, SandboxTransformError> {
let sandbox_launch_config = SandboxLaunchConfig {
sandbox: self.sandbox,
sandbox_preference: SandboxablePreference::from_selected_sandbox(self.sandbox),
policy: self.policy.clone(),
file_system_policy: self.file_system_policy.clone(),
network_policy: self.network_policy,

View File

@@ -615,8 +615,8 @@ impl UnifiedExecProcessManager {
})
.await
.map_err(|err| UnifiedExecError::create_process(err.to_string()))?;
return UnifiedExecProcess::from_remote_started(started, request.sandbox).await;
}
return UnifiedExecProcess::from_remote_started(started, request.sandbox).await;
}
let spawn_result = if tty {
codex_utils_pty::pty::spawn_process_with_inherited_fds(
@@ -651,12 +651,12 @@ impl UnifiedExecProcessManager {
params: codex_exec_server::ExecParams,
environment: &codex_exec_server::Environment,
) -> Result<UnifiedExecProcess, UnifiedExecError> {
let sandbox_type = params.sandbox.sandbox;
let started = environment
.get_exec_backend()
.start(params)
.await
.map_err(|err| UnifiedExecError::create_process(err.to_string()))?;
let sandbox_type = started.sandbox_type;
UnifiedExecProcess::from_remote_started(started, sandbox_type).await
}

View File

@@ -20,7 +20,7 @@ arc-swap = { workspace = true }
async-trait = { workspace = true }
base64 = { workspace = true }
clap = { workspace = true, features = ["derive"] }
codex-protocol = { workspace = true }
codex-arg0 = { workspace = true }
codex-sandboxing = { workspace = true }
codex-app-server-protocol = { workspace = true }
codex-protocol = { workspace = true }

View File

@@ -1,9 +1,6 @@
#[cfg(target_os = "linux")]
use std::path::Path;
use clap::Parser;
#[cfg(target_os = "linux")]
use codex_sandboxing::landlock::CODEX_LINUX_SANDBOX_ARG0;
use codex_arg0::Arg0DispatchPaths;
use codex_arg0::arg0_dispatch_or_else;
#[derive(Debug, Parser)]
struct ExecServerArgs {
@@ -17,29 +14,13 @@ struct ExecServerArgs {
}
fn main() -> anyhow::Result<()> {
dispatch_arg0();
let runtime = tokio::runtime::Runtime::new()?;
runtime.block_on(async {
arg0_dispatch_or_else(|arg0_paths: Arg0DispatchPaths| async move {
let args = ExecServerArgs::parse();
codex_exec_server::run_main_with_listen_url(&args.listen)
let runtime =
codex_exec_server::ExecServerRuntimeConfig::new(arg0_paths.codex_linux_sandbox_exe);
codex_exec_server::run_main_with_runtime(&args.listen, runtime)
.await
.map_err(|err| anyhow::Error::msg(err.to_string()))
.map_err(|err| anyhow::Error::msg(err.to_string()))?;
Ok(())
})
}
#[cfg(target_os = "linux")]
fn dispatch_arg0() {
let argv0 = std::env::args_os().next().unwrap_or_default();
let exe_name = Path::new(&argv0)
.file_name()
.and_then(|name| name.to_str())
.unwrap_or_default();
if exe_name == CODEX_LINUX_SANDBOX_ARG0 {
codex_linux_sandbox::run_main();
}
}
#[cfg(not(target_os = "linux"))]
fn dispatch_arg0() {}

View File

@@ -28,6 +28,7 @@ pub use file_system::FileSystemResult;
pub use file_system::ReadDirectoryEntry;
pub use file_system::RemoveOptions;
pub use local_file_system::LOCAL_FS;
pub use local_process::ExecServerRuntimeConfig;
pub use process::ExecBackend;
pub use process::ExecProcess;
pub use process::StartedExecProcess;
@@ -66,3 +67,4 @@ pub use server::DEFAULT_LISTEN_URL;
pub use server::ExecServerListenUrlParseError;
pub use server::run_main;
pub use server::run_main_with_listen_url;
pub use server::run_main_with_runtime;

View File

@@ -98,22 +98,20 @@ struct LocalExecProcess {
}
#[derive(Clone, Debug, Default)]
struct ExecServerRuntimeConfig {
pub struct ExecServerRuntimeConfig {
codex_linux_sandbox_exe: Option<PathBuf>,
}
impl ExecServerRuntimeConfig {
fn detect() -> Self {
pub fn new(codex_linux_sandbox_exe: Option<PathBuf>) -> Self {
Self {
// The Codex CLI and codex-exec-server both dispatch the Linux
// sandbox helper from their own executable via argv[0].
codex_linux_sandbox_exe: if cfg!(target_os = "linux") {
std::env::current_exe().ok()
} else {
None
},
codex_linux_sandbox_exe,
}
}
pub fn detect() -> Self {
Self::default()
}
}
struct StartedProcess {
@@ -133,13 +131,20 @@ impl Default for LocalProcess {
impl LocalProcess {
pub(crate) fn new(notifications: RpcNotificationSender) -> Self {
Self::new_with_runtime(notifications, ExecServerRuntimeConfig::detect())
}
pub(crate) fn new_with_runtime(
notifications: RpcNotificationSender,
runtime: ExecServerRuntimeConfig,
) -> Self {
Self {
inner: Arc::new(Inner {
notifications,
processes: Mutex::new(HashMap::new()),
initialize_requested: AtomicBool::new(false),
initialized: AtomicBool::new(false),
runtime: ExecServerRuntimeConfig::detect(),
runtime,
}),
}
}
@@ -306,6 +311,7 @@ impl LocalProcess {
.await
.map(|started| ExecResponse {
process_id: started.process_id,
sandbox: started.sandbox_type,
})
}
@@ -523,7 +529,7 @@ fn prepare_exec_launch(
&params.env,
params.sandbox.additional_permissions.clone(),
)?;
params
let mut launch = params
.sandbox
.transform(
command,
@@ -533,7 +539,9 @@ fn prepare_exec_launch(
None,
runtime.codex_linux_sandbox_exe.as_ref(),
)
.map_err(|err| internal_error(format!("failed to build sandbox launch: {err}")))
.map_err(|err| internal_error(format!("failed to build sandbox launch: {err}")))?;
launch.prepare_env_for_spawn();
Ok(launch)
}
impl LocalProcess {

View File

@@ -4,6 +4,7 @@ use std::path::PathBuf;
use base64::engine::general_purpose::STANDARD as BASE64_STANDARD;
use codex_protocol::protocol::SandboxPolicy;
use codex_sandboxing::SandboxLaunchConfig;
use codex_sandboxing::SandboxType;
use codex_utils_absolute_path::AbsolutePathBuf;
use serde::Deserialize;
use serde::Serialize;
@@ -71,6 +72,7 @@ pub struct ExecParams {
#[serde(rename_all = "camelCase")]
pub struct ExecResponse {
pub process_id: ProcessId,
pub sandbox: SandboxType,
}
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]

View File

@@ -34,19 +34,18 @@ impl RemoteProcess {
impl ExecBackend for RemoteProcess {
async fn start(&self, params: ExecParams) -> Result<StartedExecProcess, ExecServerError> {
let process_id = params.process_id.clone();
let sandbox_type = params.sandbox.sandbox;
let session = self.client.register_session(&process_id).await?;
match self.client.exec(params).await {
Ok(_) => {}
let response = match self.client.exec(params).await {
Ok(response) => response,
Err(err) => {
session.unregister().await;
return Err(err);
}
}
};
Ok(StartedExecProcess {
process: Arc::new(RemoteExecProcess { session }),
sandbox_type,
sandbox_type: response.sandbox,
})
}
}

View File

@@ -16,5 +16,12 @@ pub async fn run_main() -> Result<(), Box<dyn std::error::Error + Send + Sync>>
pub async fn run_main_with_listen_url(
listen_url: &str,
) -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
transport::run_transport(listen_url).await
transport::run_transport(listen_url, crate::ExecServerRuntimeConfig::detect()).await
}
pub async fn run_main_with_runtime(
listen_url: &str,
runtime: crate::ExecServerRuntimeConfig,
) -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
transport::run_transport(listen_url, runtime).await
}

View File

@@ -1,5 +1,6 @@
use codex_app_server_protocol::JSONRPCErrorError;
use crate::ExecServerRuntimeConfig;
use crate::protocol::ExecParams;
use crate::protocol::ExecResponse;
use crate::protocol::FsCopyParams;
@@ -34,9 +35,12 @@ pub(crate) struct ExecServerHandler {
}
impl ExecServerHandler {
pub(crate) fn new(notifications: RpcNotificationSender) -> Self {
pub(crate) fn new(
notifications: RpcNotificationSender,
runtime: ExecServerRuntimeConfig,
) -> Self {
Self {
process: ProcessHandler::new(notifications),
process: ProcessHandler::new(notifications, runtime),
file_system: FileSystemHandler::default(),
}
}

View File

@@ -6,6 +6,7 @@ use pretty_assertions::assert_eq;
use tokio::sync::mpsc;
use super::ExecServerHandler;
use crate::ExecServerRuntimeConfig;
use crate::ProcessId;
use crate::protocol::ExecParams;
use crate::protocol::InitializeResponse;
@@ -36,9 +37,10 @@ fn exec_params(process_id: &str) -> ExecParams {
async fn initialized_handler() -> Arc<ExecServerHandler> {
let (outgoing_tx, _outgoing_rx) = mpsc::channel(16);
let handler = Arc::new(ExecServerHandler::new(RpcNotificationSender::new(
outgoing_tx,
)));
let handler = Arc::new(ExecServerHandler::new(
RpcNotificationSender::new(outgoing_tx),
ExecServerRuntimeConfig::detect(),
));
assert_eq!(
handler.initialize().expect("initialize"),
InitializeResponse {}

View File

@@ -1,5 +1,6 @@
use codex_app_server_protocol::JSONRPCErrorError;
use crate::ExecServerRuntimeConfig;
use crate::local_process::LocalProcess;
use crate::protocol::ExecParams;
use crate::protocol::ExecResponse;
@@ -18,9 +19,12 @@ pub(crate) struct ProcessHandler {
}
impl ProcessHandler {
pub(crate) fn new(notifications: RpcNotificationSender) -> Self {
pub(crate) fn new(
notifications: RpcNotificationSender,
runtime: ExecServerRuntimeConfig,
) -> Self {
Self {
process: LocalProcess::new(notifications),
process: LocalProcess::new_with_runtime(notifications, runtime),
}
}

View File

@@ -4,6 +4,7 @@ use tokio::sync::mpsc;
use tracing::debug;
use tracing::warn;
use crate::ExecServerRuntimeConfig;
use crate::connection::CHANNEL_CAPACITY;
use crate::connection::JsonRpcConnection;
use crate::connection::JsonRpcConnectionEvent;
@@ -15,13 +16,16 @@ use crate::rpc::method_not_found;
use crate::server::ExecServerHandler;
use crate::server::registry::build_router;
pub(crate) async fn run_connection(connection: JsonRpcConnection) {
pub(crate) async fn run_connection(
connection: JsonRpcConnection,
runtime: ExecServerRuntimeConfig,
) {
let router = Arc::new(build_router());
let (json_outgoing_tx, mut incoming_rx, connection_tasks) = connection.into_parts();
let (outgoing_tx, mut outgoing_rx) =
mpsc::channel::<RpcServerOutboundMessage>(CHANNEL_CAPACITY);
let notifications = RpcNotificationSender::new(outgoing_tx.clone());
let handler = Arc::new(ExecServerHandler::new(notifications));
let handler = Arc::new(ExecServerHandler::new(notifications, runtime));
let outbound_task = tokio::spawn(async move {
while let Some(message) = outgoing_rx.recv().await {

View File

@@ -4,6 +4,7 @@ use tokio::net::TcpListener;
use tokio_tungstenite::accept_async;
use tracing::warn;
use crate::ExecServerRuntimeConfig;
use crate::connection::JsonRpcConnection;
use crate::server::processor::run_connection;
@@ -48,13 +49,15 @@ pub(crate) fn parse_listen_url(
pub(crate) async fn run_transport(
listen_url: &str,
runtime: ExecServerRuntimeConfig,
) -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
let bind_address = parse_listen_url(listen_url)?;
run_websocket_listener(bind_address).await
run_websocket_listener(bind_address, runtime).await
}
async fn run_websocket_listener(
bind_address: SocketAddr,
runtime: ExecServerRuntimeConfig,
) -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
let listener = TcpListener::bind(bind_address).await?;
let local_addr = listener.local_addr()?;
@@ -63,13 +66,17 @@ async fn run_websocket_listener(
loop {
let (stream, peer_addr) = listener.accept().await?;
let runtime = runtime.clone();
tokio::spawn(async move {
match accept_async(stream).await {
Ok(websocket) => {
run_connection(JsonRpcConnection::from_websocket(
websocket,
format!("exec-server websocket {peer_addr}"),
))
run_connection(
JsonRpcConnection::from_websocket(
websocket,
format!("exec-server websocket {peer_addr}"),
),
runtime,
)
.await;
}
Err(err) => {

View File

@@ -17,7 +17,7 @@ use codex_protocol::permissions::FileSystemSandboxPolicy;
use codex_protocol::permissions::NetworkSandboxPolicy;
use codex_protocol::protocol::SandboxPolicy;
use codex_sandboxing::SandboxLaunchConfig;
use codex_sandboxing::SandboxType;
use codex_sandboxing::SandboxablePreference;
use pretty_assertions::assert_eq;
use tempfile::TempDir;
use test_case::test_case;
@@ -224,16 +224,6 @@ async fn assert_exec_process_preserves_queued_events_before_subscribe(
Ok(())
}
fn platform_sandbox_type() -> SandboxType {
if cfg!(target_os = "macos") {
SandboxType::MacosSeatbelt
} else if cfg!(target_os = "linux") {
SandboxType::LinuxSeccomp
} else {
unreachable!("unix exec-server tests only run on macOS and Linux");
}
}
fn write_outside_workspace_sandbox(workspace_root: &std::path::Path) -> SandboxLaunchConfig {
let mut policy = SandboxPolicy::new_workspace_write_policy();
if let SandboxPolicy::WorkspaceWrite {
@@ -246,7 +236,7 @@ fn write_outside_workspace_sandbox(workspace_root: &std::path::Path) -> SandboxL
*exclude_slash_tmp = true;
}
SandboxLaunchConfig {
sandbox: platform_sandbox_type(),
sandbox_preference: SandboxablePreference::Require,
policy: policy.clone(),
file_system_policy: FileSystemSandboxPolicy::from_legacy_sandbox_policy(
&policy,

View File

@@ -75,6 +75,7 @@ async fn exec_server_starts_process_over_websocket() -> anyhow::Result<()> {
process_start_response,
ExecResponse {
process_id: ProcessId::from("proc-1"),
sandbox: codex_sandboxing::SandboxType::None,
}
);

View File

@@ -43,17 +43,29 @@ impl SandboxType {
}
}
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)]
#[serde(rename_all = "kebab-case")]
pub enum SandboxablePreference {
Auto,
Require,
Forbid,
}
impl SandboxablePreference {
pub fn from_selected_sandbox(sandbox: SandboxType) -> Self {
match sandbox {
SandboxType::None => Self::Forbid,
SandboxType::MacosSeatbelt
| SandboxType::LinuxSeccomp
| SandboxType::WindowsRestrictedToken => Self::Require,
}
}
}
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct SandboxLaunchConfig {
pub sandbox: SandboxType,
pub sandbox_preference: SandboxablePreference,
pub policy: SandboxPolicy,
pub file_system_policy: FileSystemSandboxPolicy,
pub network_policy: NetworkSandboxPolicy,
@@ -68,7 +80,7 @@ pub struct SandboxLaunchConfig {
impl SandboxLaunchConfig {
pub fn no_sandbox(sandbox_policy_cwd: PathBuf) -> Self {
Self {
sandbox: SandboxType::None,
sandbox_preference: SandboxablePreference::Forbid,
policy: SandboxPolicy::DangerFullAccess,
file_system_policy: FileSystemSandboxPolicy::unrestricted(),
network_policy: NetworkSandboxPolicy::Enabled,
@@ -131,6 +143,22 @@ pub struct SandboxExecRequest {
pub arg0: Option<String>,
}
impl SandboxExecRequest {
pub fn prepare_env_for_spawn(&mut self) {
if !self.network_sandbox_policy.is_enabled() {
self.env.insert(
"CODEX_SANDBOX_NETWORK_DISABLED".to_string(),
"1".to_string(),
);
}
#[cfg(target_os = "macos")]
if self.sandbox == SandboxType::MacosSeatbelt {
self.env
.insert("CODEX_SANDBOX".to_string(), "seatbelt".to_string());
}
}
}
#[derive(Debug)]
pub enum SandboxTransformError {
MissingLinuxSandboxExecutable,
@@ -208,11 +236,18 @@ impl SandboxManager {
launch.network_policy,
additional_permissions.as_ref(),
);
let sandbox = self.select_initial(
&effective_file_system_policy,
effective_network_policy,
launch.sandbox_preference,
launch.windows_sandbox_level,
launch.enforce_managed_network,
);
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 launch.sandbox {
let (argv, arg0_override) = match sandbox {
SandboxType::None => (os_argv_to_strings(argv), None),
#[cfg(target_os = "macos")]
SandboxType::MacosSeatbelt => {
@@ -261,7 +296,7 @@ impl SandboxManager {
cwd: command.cwd,
env: command.env,
network: network.cloned(),
sandbox: launch.sandbox,
sandbox,
windows_sandbox_level: launch.windows_sandbox_level,
windows_sandbox_private_desktop: launch.windows_sandbox_private_desktop,
sandbox_policy: effective_policy,

View File

@@ -29,10 +29,10 @@ fn test_launch_config(
policy: SandboxPolicy,
file_system_policy: FileSystemSandboxPolicy,
network_policy: NetworkSandboxPolicy,
sandbox: SandboxType,
sandbox_preference: SandboxablePreference,
) -> SandboxLaunchConfig {
SandboxLaunchConfig {
sandbox,
sandbox_preference,
policy,
file_system_policy,
network_policy,
@@ -113,7 +113,7 @@ fn transform_preserves_unrestricted_file_system_policy_for_restricted_network()
},
FileSystemSandboxPolicy::unrestricted(),
NetworkSandboxPolicy::Restricted,
SandboxType::None,
SandboxablePreference::Forbid,
),
/*network*/ None,
/*codex_linux_sandbox_exe*/ None,
@@ -163,7 +163,7 @@ fn transform_additional_permissions_enable_network_for_external_sandbox() {
},
FileSystemSandboxPolicy::unrestricted(),
NetworkSandboxPolicy::Restricted,
SandboxType::None,
SandboxablePreference::Forbid,
),
/*network*/ None,
/*codex_linux_sandbox_exe*/ None,
@@ -229,7 +229,7 @@ fn transform_additional_permissions_preserves_denied_entries() {
},
]),
NetworkSandboxPolicy::Restricted,
SandboxType::None,
SandboxablePreference::Forbid,
),
/*network*/ None,
/*codex_linux_sandbox_exe*/ None,
@@ -281,7 +281,7 @@ fn transform_linux_seccomp_request(
SandboxPolicy::DangerFullAccess,
FileSystemSandboxPolicy::unrestricted(),
NetworkSandboxPolicy::Enabled,
SandboxType::LinuxSeccomp,
SandboxablePreference::Require,
),
/*network*/ None,
Some(codex_linux_sandbox_exe),