From 4caa5d615dfb68eab0f391837badbd53fed1b32e Mon Sep 17 00:00:00 2001 From: chess Date: Thu, 10 Sep 2026 21:52:36 +0000 Subject: [PATCH] Keep Windows sandbox private desktops alive across helper exits (#44658) ## Why Private desktops owned by short-lived sandbox wrappers disappear when those wrappers exit, preventing reuse across filesystem helper requests. ## What changed - Select and cache private desktops in the calling process, keeping desktops separate for different sandbox permissions. - Pass the desktop name through the wrapper to the restricted-token and elevated backends so helpers reuse the selected desktop. - Separate sandbox account preparation from filesystem ACL refresh so desktop selection does not perform the wrapper's refresh. - Propagate desktop preparation errors and require a desktop name when the wrapper's private desktop flag is set. ## Testing Add a Windows filesystem regression test covering desktop survival and reuse across reads, writes, metadata queries, and streaming reads, plus separate read-only permissions and rejected writes. Extend wrapper argument tests to cover named desktops and rejection of a missing desktop name. GitOrigin-RevId: 05a1cb829a902732248bfa7f4ad7470d911fa9f6 --- .../exec-server/tests/file_system/support.rs | 8 +- .../exec-server/tests/file_system_windows.rs | 168 ++++++++++++++++++ codex-rs/sandboxing/src/manager.rs | 3 +- codex-rs/windows-sandbox-rs/src/desktop.rs | 20 ++- .../src/elevated/runner_client.rs | 10 +- codex-rs/windows-sandbox-rs/src/identity.rs | 128 +++++++------ .../src/identity_integration_tests.rs | 65 +++---- codex-rs/windows-sandbox-rs/src/spawn_prep.rs | 2 +- .../src/unified_exec/backends/elevated.rs | 58 +++--- .../src/unified_exec/backends/legacy.rs | 24 ++- .../src/unified_exec/mod.rs | 13 +- codex-rs/windows-sandbox-rs/src/wrapper.rs | 149 +++++++++++++++- .../windows-sandbox-rs/src/wrapper_tests.rs | 21 ++- 13 files changed, 504 insertions(+), 165 deletions(-) diff --git a/codex-rs/exec-server/tests/file_system/support.rs b/codex-rs/exec-server/tests/file_system/support.rs index aa7122810a..30b38bdb36 100644 --- a/codex-rs/exec-server/tests/file_system/support.rs +++ b/codex-rs/exec-server/tests/file_system/support.rs @@ -76,10 +76,10 @@ pub(crate) async fn create_file_system_context( #[cfg(windows)] pub(crate) fn is_unsupported_restricted_token_host(result: &std::io::Result) -> bool { - result.as_ref().err().is_some_and(|err| { - err.to_string() - .contains("windows sandbox failed: CreateRestrictedToken failed: 87") - }) + result + .as_ref() + .err() + .is_some_and(|err| err.to_string().contains("CreateRestrictedToken failed: 87")) } pub(crate) fn absolute_path(path: std::path::PathBuf) -> AbsolutePathBuf { diff --git a/codex-rs/exec-server/tests/file_system_windows.rs b/codex-rs/exec-server/tests/file_system_windows.rs index 41916edbfc..e80b8eb1a7 100644 --- a/codex-rs/exec-server/tests/file_system_windows.rs +++ b/codex-rs/exec-server/tests/file_system_windows.rs @@ -8,6 +8,8 @@ mod shared; #[path = "file_system/support.rs"] mod support; +use std::collections::BTreeSet; +use std::ffi::c_void; use std::path::Path; use std::process::Command; use std::time::Duration; @@ -22,10 +24,13 @@ use codex_exec_server::WriteFileOptions; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::protocol::SandboxPolicy; use codex_utils_path_uri::PathUri; +use futures::TryStreamExt; +use pretty_assertions::assert_eq; use test_case::test_case; use tokio::net::windows::named_pipe::ServerOptions; use tokio::time::timeout; use uuid::Uuid; +use windows_sys::Win32::System::Threading::GetCurrentProcess; use crate::support::FileSystemImplementation; use crate::support::create_file_system_context; @@ -387,6 +392,169 @@ async fn file_system_remote_fs_helper_respects_windows_sandbox_write_policy() -> Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn file_system_private_desktop_survives_helper_exits_and_separates_permissions() -> Result<()> +{ + let context = create_file_system_context(FileSystemImplementation::Local).await?; + let file_system = context.file_system; + let tmp = tempfile::TempDir::new()?; + let path = tmp.path().join("contents.txt"); + std::fs::write(&path, b"initial")?; + let uri = PathUri::from_host_native_path(&path)?; + let mut sandbox = workspace_write_sandbox(tmp.path().to_path_buf()); + sandbox.windows_sandbox_private_desktop = true; + let before = process_private_desktops()?; + let read = file_system + .read_file(&uri, ReadFileOptions::default(), Some(&sandbox)) + .await; + if is_unsupported_restricted_token_host(&read) { + eprintln!("Skipping private desktop reuse: this host cannot create restricted tokens"); + return Ok(()); + } + assert_eq!(read?, b"initial"); + + // Ownership must outlive each helper; a desktop held only by the helper disappears here. + let warmed = process_private_desktops()?; + assert_eq!(warmed.difference(&before).count(), 1); + for contents in ["updated", "updated again"] { + file_system + .write_file( + &uri, + contents.as_bytes().to_vec(), + WriteFileOptions::default(), + Some(&sandbox), + ) + .await?; + assert_eq!(process_private_desktops()?, warmed); + assert_eq!( + file_system + .read_file(&uri, ReadFileOptions::default(), Some(&sandbox)) + .await?, + contents.as_bytes() + ); + assert_eq!(process_private_desktops()?, warmed); + assert!( + file_system + .get_metadata(&uri, GetMetadataOptions::default(), Some(&sandbox)) + .await? + .is_file + ); + assert_eq!(process_private_desktops()?, warmed); + let chunks = file_system + .read_file_stream(&uri, Some(&sandbox)) + .await? + .try_collect::>() + .await?; + assert_eq!(chunks.concat(), contents.as_bytes()); + assert_eq!(process_private_desktops()?, warmed); + } + + let mut readonly = read_only_sandbox_for_cwd(tmp.path().to_path_buf())?; + readonly.windows_sandbox_level = WindowsSandboxLevel::RestrictedToken; + readonly.windows_sandbox_private_desktop = true; + assert_eq!( + file_system + .read_file(&uri, ReadFileOptions::default(), Some(&readonly)) + .await?, + b"updated again" + ); + let separated = process_private_desktops()?; + assert!(warmed.is_subset(&separated)); + assert_eq!(separated.difference(&warmed).count(), 1); + file_system + .write_file( + &uri, + b"blocked".to_vec(), + WriteFileOptions::default(), + Some(&readonly), + ) + .await + .expect_err("read-only filesystem requests must reject writes"); + assert_eq!(std::fs::read(&path)?, b"updated again"); + assert_eq!(process_private_desktops()?, separated); + Ok(()) +} + +fn process_private_desktops() -> Result> { + // Query this process so other tests' private desktops cannot affect the assertions. + // Native layout: https://github.com/winsiderss/phnt/blob/master/ntpsapi.h + #[repr(C)] + struct HandleEntry { + handle: isize, + _handle_count: usize, + _pointer_count: usize, + _granted_access: u32, + _object_type_index: u32, + _handle_attributes: u32, + _reserved: u32, + } + #[link(name = "ntdll")] + unsafe extern "system" { + fn NtQueryInformationProcess( + process: isize, + class: u32, + information: *mut c_void, + length: u32, + return_length: *mut u32, + ) -> i32; + } + #[link(name = "user32")] + unsafe extern "system" { + fn GetUserObjectInformationW( + object: isize, + index: i32, + information: *mut c_void, + length: u32, + length_needed: *mut u32, + ) -> i32; + } + let mut snapshot = vec![0usize; 8192]; + let bytes = std::mem::size_of_val(snapshot.as_slice()); + let status = unsafe { + NtQueryInformationProcess( + GetCurrentProcess(), + /*class*/ 51, + snapshot.as_mut_ptr().cast(), + bytes as u32, + std::ptr::null_mut(), + ) + }; + anyhow::ensure!(status >= 0, "process handle query failed: {status:#x}"); + let count = snapshot[0]; + anyhow::ensure!( + count <= (bytes - 2 * size_of::()) / size_of::(), + "process handle snapshot exceeds its buffer" + ); + let entries = unsafe { + std::slice::from_raw_parts(snapshot.as_ptr().add(2).cast::(), count) + }; + let mut desktops = BTreeSet::new(); + for entry in entries { + let mut name = [0u16; 64]; + let mut length_needed = 0; + if unsafe { + GetUserObjectInformationW( + entry.handle, + /*index*/ 2, + name.as_mut_ptr().cast(), + std::mem::size_of_val(&name) as u32, + &mut length_needed, + ) + } != 0 + { + let end = name + .iter() + .position(|&unit| unit == 0) + .unwrap_or(name.len()); + let name = String::from_utf16(&name[..end])?; + if name.starts_with("CodexSandboxDesktop-") { + desktops.insert(name); + } + } + } + Ok(desktops) +} + fn read_only_sandbox_for_cwd(cwd: std::path::PathBuf) -> Result { Ok(FileSystemSandboxContext::from_legacy_sandbox_policy( SandboxPolicy::new_read_only_policy(), diff --git a/codex-rs/sandboxing/src/manager.rs b/codex-rs/sandboxing/src/manager.rs index b633e8f779..2b16db917e 100644 --- a/codex-rs/sandboxing/src/manager.rs +++ b/codex-rs/sandboxing/src/manager.rs @@ -655,7 +655,8 @@ fn wrap_windows_sandbox_exec_request_for_direct_spawn( deny_read_paths_override, deny_write_paths_override, codex_home, - ); + ) + .map_err(|err| SandboxTransformError::WindowsSandboxPreparation(err.to_string()))?; request.command = Vec::with_capacity(1 + wrapper_args.len()); request.command.push(source.to_string_lossy().into_owned()); diff --git a/codex-rs/windows-sandbox-rs/src/desktop.rs b/codex-rs/windows-sandbox-rs/src/desktop.rs index d8e7d19adf..0c23e8d146 100644 --- a/codex-rs/windows-sandbox-rs/src/desktop.rs +++ b/codex-rs/windows-sandbox-rs/src/desktop.rs @@ -160,6 +160,24 @@ impl LaunchDesktop { if !use_private_desktop { return Self::prepare(/*use_private_desktop*/ false, logs_base_dir); } + Self::open_private(&Self::shared_legacy_name( + permissions, + cwd, + env, + security, + additional_deny_write_paths, + logs_base_dir, + )?) + } + + pub(crate) fn shared_legacy_name( + permissions: &ResolvedWindowsSandboxPermissions, + cwd: &Path, + env: &HashMap, + security: &LegacySessionSecurity, + additional_deny_write_paths: &[PathBuf], + logs_base_dir: Option<&Path>, + ) -> Result { let sandbox_sid = unsafe { get_user_sid_bytes(security.h_token)? }; let sandbox_sid = string_from_sid_bytes(&sandbox_sid).map_err(anyhow::Error::msg)?; let paths = compute_allow_paths_for_permissions(permissions, cwd, env); @@ -193,7 +211,7 @@ impl LaunchDesktop { entry.insert(PrivateDesktop::create(logs_base_dir)?) } }; - Self::open_private(&desktop.name) + Ok(desktop.name.clone()) } pub fn prepare(use_private_desktop: bool, logs_base_dir: Option<&Path>) -> Result { diff --git a/codex-rs/windows-sandbox-rs/src/elevated/runner_client.rs b/codex-rs/windows-sandbox-rs/src/elevated/runner_client.rs index 697ad33919..d7430123fc 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated/runner_client.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated/runner_client.rs @@ -323,9 +323,13 @@ pub(crate) fn spawn_runner_transport( mut spawn_request: SpawnRequest, desktop_policy: Option<&DesktopPolicy>, ) -> Result { - spawn_request.private_desktop_name = desktop_policy - .map(|policy| shared_private_desktop_for_user(&sandbox_creds.username, policy, log_dir)) - .transpose()?; + if let Some(policy) = desktop_policy { + spawn_request.private_desktop_name = Some(shared_private_desktop_for_user( + &sandbox_creds.username, + policy, + log_dir, + )?); + } let (pipe_in_name, pipe_out_name) = pipe_pair(); let h_pipe_in = create_named_pipe(&pipe_in_name, PIPE_ACCESS_OUTBOUND, &sandbox_creds.username)?; diff --git a/codex-rs/windows-sandbox-rs/src/identity.rs b/codex-rs/windows-sandbox-rs/src/identity.rs index 6f3a66d2fd..8379f469af 100644 --- a/codex-rs/windows-sandbox-rs/src/identity.rs +++ b/codex-rs/windows-sandbox-rs/src/identity.rs @@ -3,7 +3,9 @@ use crate::logging::debug_log; use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; use crate::setup::OFFLINE_USERNAME; use crate::setup::ONLINE_USERNAME; +use crate::setup::OfflineProxySettings; use crate::setup::SandboxNetworkIdentity; +use crate::setup::SandboxSetupRequest; use crate::setup::SandboxUserRecord; use crate::setup::SandboxUsersFile; use crate::setup::SetupMarker; @@ -182,55 +184,63 @@ pub fn require_logon_sandbox_creds( proxy_enforced: bool, proxy_settings_mode: crate::WindowsSandboxProxySettingsMode, ) -> Result { - require_logon_sandbox_creds_with_setup( - permissions, - command_cwd, - env_map, - codex_home, - read_roots_override, - read_roots_include_platform_defaults, - write_roots_override, - deny_read_paths_override, - deny_write_paths_override, - proxy_enforced, - proxy_settings_mode, - run_elevated_setup_with_proxy_settings, - run_setup_refresh_with_overrides_and_proxy_settings, - local_user_flags, - ) -} - -#[allow(clippy::too_many_arguments)] -fn require_logon_sandbox_creds_with_setup( - permissions: &ResolvedWindowsSandboxPermissions, - command_cwd: &Path, - env_map: &HashMap, - codex_home: &Path, - read_roots_override: Option<&[PathBuf]>, - read_roots_include_platform_defaults: bool, - write_roots_override: Option<&[PathBuf]>, - deny_read_paths_override: &[PathBuf], - deny_write_paths_override: &[PathBuf], - proxy_enforced: bool, - proxy_settings_mode: crate::WindowsSandboxProxySettingsMode, - run_full_setup: impl FnOnce( - crate::setup::SandboxSetupRequest<'_>, - &crate::setup::OfflineProxySettings, - ) -> Result<()>, - run_refresh_setup: impl FnOnce( - crate::setup::SandboxSetupRequest<'_>, - crate::setup::SetupRootOverrides, - &crate::setup::OfflineProxySettings, - ) -> Result<()>, - read_local_user_flags: impl Fn(&str) -> Result>, -) -> Result { - let sandbox_dir = crate::setup::sandbox_dir(codex_home); let needed_read = read_roots_override .map(<[PathBuf]>::to_vec) .unwrap_or_else(|| gather_read_roots(command_cwd, permissions, env_map, codex_home)); let needed_write = write_roots_override .map(<[PathBuf]>::to_vec) .unwrap_or_else(|| gather_write_roots_for_permissions(permissions, command_cwd, env_map)); + // Do not grant the capability token write access to CODEX_HOME/.sandbox; the setup helper + // grants the sandbox group access separately through lock_sandbox_dir. + let request = SandboxSetupRequest { + permissions, + command_cwd, + env_map, + codex_home, + proxy_enforced, + }; + let (creds, offline_proxy_settings) = require_sandbox_account(&request, proxy_settings_mode)?; + run_setup_refresh_with_overrides_and_proxy_settings( + request, + crate::setup::SetupRootOverrides { + read_roots: Some(needed_read), + read_roots_include_platform_defaults, + write_roots: Some(needed_write), + deny_read_paths: Some(deny_read_paths_override.to_vec()), + deny_write_paths: Some(deny_write_paths_override.to_vec()), + }, + &offline_proxy_settings, + )?; + Ok(creds) +} + +/// Ensures the selected account is ready; launchers must refresh filesystem ACLs separately. +pub(crate) fn require_sandbox_account( + request: &SandboxSetupRequest<'_>, + proxy_settings_mode: crate::WindowsSandboxProxySettingsMode, +) -> Result<(SandboxCreds, OfflineProxySettings)> { + require_sandbox_account_with_setup( + request, + proxy_settings_mode, + run_elevated_setup_with_proxy_settings, + local_user_flags, + ) +} + +fn require_sandbox_account_with_setup( + request: &SandboxSetupRequest<'_>, + proxy_settings_mode: crate::WindowsSandboxProxySettingsMode, + run_full_setup: impl FnOnce(SandboxSetupRequest<'_>, &OfflineProxySettings) -> Result<()>, + read_local_user_flags: impl Fn(&str) -> Result>, +) -> Result<(SandboxCreds, OfflineProxySettings)> { + let &SandboxSetupRequest { + permissions, + command_cwd, + env_map, + codex_home, + proxy_enforced, + } = request; + let sandbox_dir = crate::setup::sandbox_dir(codex_home); let network_identity = SandboxNetworkIdentity::from_permissions(permissions, proxy_enforced); let marker = load_marker(codex_home)?; let desired_offline_proxy_settings = desired_offline_proxy_settings( @@ -239,9 +249,6 @@ fn require_logon_sandbox_creds_with_setup( env_map, network_identity, ); - // NOTE: Do not add CODEX_HOME/.sandbox to `needed_write`; it must remain non-writable by the - // restricted capability token. The setup helper's `lock_sandbox_dir` is responsible for - // granting the sandbox group access to this directory without granting the capability SID. let mut setup_reason: Option = None; let mut identity = match marker { @@ -305,33 +312,18 @@ fn require_logon_sandbox_creds_with_setup( )?; identity = select_identity(network_identity, codex_home)?; } - // Always refresh ACLs (non-elevated) for current roots via the setup binary. - run_refresh_setup( - crate::setup::SandboxSetupRequest { - permissions, - command_cwd, - env_map, - codex_home, - proxy_enforced, - }, - crate::setup::SetupRootOverrides { - read_roots: Some(needed_read), - read_roots_include_platform_defaults, - write_roots: Some(needed_write), - deny_read_paths: Some(deny_read_paths_override.to_vec()), - deny_write_paths: Some(deny_write_paths_override.to_vec()), - }, - &desired_offline_proxy_settings, - )?; let identity = identity.ok_or_else(|| { anyhow!( "Windows sandbox setup is missing or out of date; rerun the sandbox setup with elevation" ) })?; - Ok(SandboxCreds { - username: identity.username, - password: identity.password, - }) + Ok(( + SandboxCreds { + username: identity.username, + password: identity.password, + }, + desired_offline_proxy_settings, + )) } fn desired_offline_proxy_settings( diff --git a/codex-rs/windows-sandbox-rs/src/identity_integration_tests.rs b/codex-rs/windows-sandbox-rs/src/identity_integration_tests.rs index 6f1926ed9e..06f1340cbb 100644 --- a/codex-rs/windows-sandbox-rs/src/identity_integration_tests.rs +++ b/codex-rs/windows-sandbox-rs/src/identity_integration_tests.rs @@ -1,12 +1,13 @@ -//! Exercises credential orchestration with real marker files and DPAPI credentials. +//! Exercises account preparation with real marker files and DPAPI credentials. //! Setup and account lookup callbacks isolate machine state. This does not exercise //! the public wrapper, native account queries, payload serialization, singleflight, //! or helper launches. -use super::require_logon_sandbox_creds_with_setup; +use super::require_sandbox_account_with_setup; use crate::WindowsSandboxProxySettingsMode; use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; use crate::setup::SETUP_VERSION; +use crate::setup::SandboxSetupRequest; use crate::setup::SandboxUserRecord; use crate::setup::SandboxUsersFile; use crate::setup::SetupMarker; @@ -17,31 +18,25 @@ use base64::Engine; use base64::engine::general_purpose::STANDARD as BASE64; use codex_protocol::models::PermissionProfile; use pretty_assertions::assert_eq; -use std::cell::RefCell; +use std::cell::Cell; use std::collections::HashMap; use std::fs; use windows_sys::Win32::NetworkManagement::NetManagement::UF_NORMAL_ACCOUNT; -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum SetupLaunch { - Full, - Refresh, -} - #[test] fn credential_setup_reconciles_effective_firewall_policy() -> Result<()> { let permissions = ResolvedWindowsSandboxPermissions::try_from_permission_profile( &PermissionProfile::read_only(), )?; - for (stored_binding, desired_binding, ports, expected_launch) in [ - (true, true, "8080", SetupLaunch::Refresh), - (true, true, "", SetupLaunch::Refresh), - (true, true, "8080,3129", SetupLaunch::Refresh), - (false, false, "8080", SetupLaunch::Full), - (false, true, "3128", SetupLaunch::Full), - (true, false, "3128", SetupLaunch::Full), + for (stored_binding, desired_binding, ports, expected_full_setups) in [ + (true, true, "8080", 0), + (true, true, "", 0), + (true, true, "8080,3129", 0), + (false, false, "8080", 1), + (false, true, "3128", 1), + (true, false, "3128", 1), ] { - let launches = RefCell::new(Vec::new()); + let full_setups = Cell::new(/*value*/ 0); let home = tempfile::tempdir()?; let marker = SetupMarker { version: SETUP_VERSION, @@ -78,36 +73,30 @@ fn credential_setup_reconciles_effective_firewall_policy() -> Result<()> { u8::from(desired_binding).to_string(), ), ]); - let result = require_logon_sandbox_creds_with_setup( - &permissions, - home.path(), - &env, - home.path(), - Some(&[]), - /*read_roots_include_platform_defaults*/ false, - Some(&[]), - &[], - &[], - /*proxy_enforced*/ true, + let result = require_sandbox_account_with_setup( + &SandboxSetupRequest { + permissions: &permissions, + command_cwd: home.path(), + env_map: &env, + codex_home: home.path(), + proxy_enforced: true, + }, WindowsSandboxProxySettingsMode::Reconcile, |_, _| { - launches.borrow_mut().push(SetupLaunch::Full); + full_setups.set(full_setups.get() + 1); Err(anyhow::anyhow!("full setup intercepted")) }, - |_, _, _| { - launches.borrow_mut().push(SetupLaunch::Refresh); - Ok(()) - }, |_| Ok(Some(UF_NORMAL_ACCOUNT)), ); - assert_eq!(launches.into_inner(), [expected_launch]); + assert_eq!(full_setups.get(), expected_full_setups); assert_eq!( result - .map(|creds| (creds.username, creds.password)) + .map(|(creds, _)| (creds.username, creds.password)) .map_err(|err| err.to_string()), - match expected_launch { - SetupLaunch::Refresh => Ok(("offline".into(), "test-password".into())), - SetupLaunch::Full => Err("full setup intercepted".into()), + if expected_full_setups == 0 { + Ok(("offline".into(), "test-password".into())) + } else { + Err("full setup intercepted".into()) } ); assert_eq!(fs::read(setup_marker_path(home.path()))?, marker_bytes); diff --git a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs index 9479c0b8d7..a19f0eeaac 100644 --- a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs +++ b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs @@ -80,7 +80,7 @@ pub(crate) struct LegacyAclSids<'a> { pub(crate) write_root_sids: &'a [RootCapabilitySid], } -fn prepare_spawn_context_common( +pub(crate) fn prepare_spawn_context_common( permission_profile: &PermissionProfile, workspace_roots: &[AbsolutePathBuf], codex_home: &Path, diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs index 0d7d2ca19a..24a5494b71 100644 --- a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs @@ -97,34 +97,33 @@ async fn spawn_runner_transport_task( request: RunnerTransportRequest, ) -> Result { tokio::task::spawn_blocking(move || -> Result<_> { - let desktop_policy = request - .spawn_request - .use_private_desktop - .then(|| { - DesktopPolicy::elevated( - crate::setup::SandboxSetupRequest { - permissions: &request.permissions, - command_cwd: &request.cwd, - env_map: &request.env_map, - codex_home: &request.codex_home, - proxy_enforced: request.proxy_enforced, - }, - crate::setup::SetupRootOverrides { - read_roots: request.read_roots_override.clone(), - read_roots_include_platform_defaults: request - .read_roots_include_platform_defaults, - write_roots: request.write_roots_override.clone(), - deny_read_paths: Some(request.deny_read_paths_override.clone()), - deny_write_paths: Some(request.deny_write_paths_override.clone()), - }, - &request.spawn_request.cap_sids, - request - .spawn_request - .network_proxy_restricting_sid - .as_deref(), - ) - }) - .transpose()?; + let desktop_policy = (request.spawn_request.use_private_desktop + && request.spawn_request.private_desktop_name.is_none()) + .then(|| { + DesktopPolicy::elevated( + crate::setup::SandboxSetupRequest { + permissions: &request.permissions, + command_cwd: &request.cwd, + env_map: &request.env_map, + codex_home: &request.codex_home, + proxy_enforced: request.proxy_enforced, + }, + crate::setup::SetupRootOverrides { + read_roots: request.read_roots_override.clone(), + read_roots_include_platform_defaults: request + .read_roots_include_platform_defaults, + write_roots: request.write_roots_override.clone(), + deny_read_paths: Some(request.deny_read_paths_override.clone()), + deny_write_paths: Some(request.deny_write_paths_override.clone()), + }, + &request.spawn_request.cap_sids, + request + .spawn_request + .network_proxy_restricting_sid + .as_deref(), + ) + }) + .transpose()?; spawn_runner_transport_with_retry( sandbox_creds, &request, @@ -165,6 +164,7 @@ pub(crate) async fn spawn_windows_sandbox_session_elevated_for_permission_profil tty: bool, stdin_open: bool, use_private_desktop: bool, + private_desktop_name: Option, ) -> Result { let deny_read_paths_override = deny_read_paths_override .iter() @@ -215,7 +215,7 @@ pub(crate) async fn spawn_windows_sandbox_session_elevated_for_permission_profil tty, stdin_open, use_private_desktop, - private_desktop_name: None, + private_desktop_name, }, read_roots_override: read_roots_override.map(<[PathBuf]>::to_vec), read_roots_include_platform_defaults, diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs index 08ede4a2b8..ca13e19106 100644 --- a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs @@ -70,6 +70,7 @@ fn spawn_legacy_process( cwd: &Path, env_map: &HashMap, use_private_desktop: bool, + private_desktop_name: Option<&str>, tty: bool, stdin_open: bool, stdout_tx: broadcast::Sender>, @@ -78,15 +79,18 @@ fn spawn_legacy_process( logs_base_dir: Option<&Path>, ) -> Result { let h_token = security.h_token; - let launch_desktop = LaunchDesktop::prepare_legacy( - use_private_desktop, - permissions, - cwd, - env_map, - security, - additional_deny_write_paths, - logs_base_dir, - )?; + let launch_desktop = match private_desktop_name { + Some(name) => LaunchDesktop::open_private(name)?, + None => LaunchDesktop::prepare_legacy( + use_private_desktop, + permissions, + cwd, + env_map, + security, + additional_deny_write_paths, + logs_base_dir, + )?, + }; let (pi, job, output_join, writer_handle, hpc, conpty_owner, desktop) = if tty { let (pi, mut conpty) = spawn_conpty_process_as_user(h_token, command, cwd, env_map, launch_desktop)?; @@ -325,6 +329,7 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( tty: bool, stdin_open: bool, use_private_desktop: bool, + private_desktop_name: Option, ) -> Result { let common = prepare_legacy_spawn_context( permission_profile, @@ -404,6 +409,7 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( cwd, &env_map, use_private_desktop, + private_desktop_name.as_deref(), tty, stdin_open, stdout_tx, diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/mod.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/mod.rs index 4e6020a8f8..540baf0c23 100644 --- a/codex-rs/windows-sandbox-rs/src/unified_exec/mod.rs +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/mod.rs @@ -49,6 +49,13 @@ pub struct WindowsSandboxSessionRequest<'a> { pub async fn spawn_windows_sandbox_session_for_level( request: WindowsSandboxSessionRequest<'_>, +) -> Result { + spawn_windows_sandbox_session_with_desktop(request, /*private_desktop_name*/ None).await +} + +pub(crate) async fn spawn_windows_sandbox_session_with_desktop( + request: WindowsSandboxSessionRequest<'_>, + private_desktop_name: Option, ) -> Result { if matches!(request.windows_sandbox_level, WindowsSandboxLevel::Elevated) { backends::elevated::spawn_windows_sandbox_session_elevated_for_permission_profile( @@ -70,6 +77,7 @@ pub async fn spawn_windows_sandbox_session_for_level( request.tty, request.stdin_open, request.use_private_desktop, + private_desktop_name, ) .await } else { @@ -79,7 +87,7 @@ pub async fn spawn_windows_sandbox_session_for_level( if request.network_proxy_restricting_sid.is_some() { bail!("network proxy restricting SID requires the elevated Windows sandbox backend"); } - spawn_windows_sandbox_session_legacy( + backends::legacy::spawn_windows_sandbox_session_legacy( request.permission_profile, request.workspace_roots, request.codex_home, @@ -92,6 +100,7 @@ pub async fn spawn_windows_sandbox_session_for_level( request.tty, request.stdin_open, request.use_private_desktop, + private_desktop_name, ) .await } @@ -125,6 +134,7 @@ pub async fn spawn_windows_sandbox_session_legacy( tty, stdin_open, use_private_desktop, + /*private_desktop_name*/ None, ) .await } @@ -168,6 +178,7 @@ pub async fn spawn_windows_sandbox_session_elevated_for_permission_profile( tty, stdin_open, use_private_desktop, + /*private_desktop_name*/ None, ) .await } diff --git a/codex-rs/windows-sandbox-rs/src/wrapper.rs b/codex-rs/windows-sandbox-rs/src/wrapper.rs index 9729db1b84..eb410216d8 100644 --- a/codex-rs/windows-sandbox-rs/src/wrapper.rs +++ b/codex-rs/windows-sandbox-rs/src/wrapper.rs @@ -9,6 +9,18 @@ use std::collections::HashMap; use std::path::Path; use std::path::PathBuf; +use crate::cap::load_or_create_cap_sids; +use crate::desktop::DesktopPolicy; +use crate::desktop::LaunchDesktop; +use crate::desktop::shared_private_desktop_for_user; +use crate::identity::require_sandbox_account; +use crate::setup::effective_write_roots_for_permissions; +use crate::spawn_prep::SpawnPrepOptions; +use crate::spawn_prep::legacy_session_capability_roots; +use crate::spawn_prep::prepare_legacy_session_security; +use crate::spawn_prep::prepare_legacy_spawn_context; +use crate::spawn_prep::prepare_spawn_context_common; +use crate::spawn_prep::root_capability_sids; use anyhow::Context; use anyhow::Result; use anyhow::anyhow; @@ -16,6 +28,7 @@ use anyhow::bail; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::PermissionProfile; use codex_utils_absolute_path::AbsolutePathBuf; +use windows_sys::Win32::Foundation::CloseHandle; pub const CODEX_WINDOWS_SANDBOX_ARG1: &str = "--run-as-windows-sandbox"; @@ -27,6 +40,7 @@ const ENV_JSON_FLAG: &str = "--env-json"; const NETWORK_PROXY_RESTRICTING_SID_FLAG: &str = "--network-proxy-restricting-sid"; const PERMISSION_PROFILE_FLAG: &str = "--permission-profile"; const PRIVATE_DESKTOP_FLAG: &str = "--windows-sandbox-private-desktop"; +const PRIVATE_DESKTOP_NAME_FLAG: &str = "--windows-sandbox-private-desktop-name"; const PRESERVE_PROXY_SETTINGS_FLAG: &str = "--preserve-proxy-settings"; const PROXY_ENFORCED_FLAG: &str = "--proxy-enforced"; const READ_ROOTS_INCLUDE_PLATFORM_DEFAULTS_FLAG: &str = "--read-roots-include-platform-defaults"; @@ -53,7 +67,7 @@ pub fn create_windows_sandbox_command_args_for_permission_profile( deny_read_paths_override: &[AbsolutePathBuf], deny_write_paths_override: &[AbsolutePathBuf], codex_home: &Path, -) -> Vec { +) -> Result> { let permission_profile_json = serde_json::to_string(permission_profile) .unwrap_or_else(|err| panic!("failed to serialize permission profile: {err}")); let env_json = serde_json::to_string(env_map) @@ -81,7 +95,119 @@ pub fn create_windows_sandbox_command_args_for_permission_profile( args.push(root.as_path().to_string_lossy().into_owned()); } if windows_sandbox_private_desktop { + // The caller owns the cache so the desktop survives this short-lived wrapper. + let mut desktop_env = env_map.clone(); + let deny_write_paths = deny_write_paths_override + .iter() + .map(AbsolutePathBuf::to_path_buf) + .collect::>(); + let desktop_name = if windows_sandbox_level == WindowsSandboxLevel::Elevated { + let common = prepare_spawn_context_common( + permission_profile, + workspace_roots, + codex_home, + command_cwd.as_path(), + &mut desktop_env, + &command, + SpawnPrepOptions { + inherit_path: true, + add_git_safe_directory: true, + }, + )?; + let request = crate::setup::SandboxSetupRequest { + permissions: &common.permissions, + command_cwd: command_cwd.as_path(), + env_map: &desktop_env, + codex_home, + proxy_enforced, + }; + // Desktop selection needs the account and capabilities, not the wrapper's ACL refresh. + let (sandbox_creds, _) = require_sandbox_account(&request, proxy_settings_mode)?; + let caps = load_or_create_cap_sids(codex_home)?; + let cap_sids = if common.uses_write_capabilities { + root_capability_sids( + codex_home, + command_cwd.as_path(), + effective_write_roots_for_permissions( + &common.permissions, + command_cwd.as_path(), + &desktop_env, + codex_home, + write_roots_override, + ), + )? + .into_iter() + .map(|root| root.sid_str) + .collect::>() + } else { + vec![caps.readonly] + }; + if cap_sids.is_empty() { + bail!("workspace-write sandbox has no writable root capability SIDs"); + } + let policy = DesktopPolicy::elevated( + request, + crate::setup::SetupRootOverrides { + read_roots: read_roots_override.map(<[PathBuf]>::to_vec), + read_roots_include_platform_defaults, + write_roots: write_roots_override.map(<[PathBuf]>::to_vec), + deny_read_paths: Some( + deny_read_paths_override + .iter() + .map(AbsolutePathBuf::to_path_buf) + .collect(), + ), + deny_write_paths: Some(deny_write_paths), + }, + &cap_sids, + network_proxy_restricting_sid, + )?; + shared_private_desktop_for_user( + &sandbox_creds.username, + &policy, + common.logs_base_dir.as_deref(), + )? + } else { + let common = prepare_legacy_spawn_context( + permission_profile, + workspace_roots, + codex_home, + command_cwd.as_path(), + &mut desktop_env, + &command, + SpawnPrepOptions { + inherit_path: false, + add_git_safe_directory: false, + }, + )?; + let capability_roots = legacy_session_capability_roots( + &common.permissions, + &common.current_dir, + &desktop_env, + codex_home, + ); + let security = prepare_legacy_session_security( + common.uses_write_capabilities, + codex_home, + command_cwd.as_path(), + capability_roots, + )?; + let desktop_name = LaunchDesktop::shared_legacy_name( + &common.permissions, + &common.current_dir, + &desktop_env, + &security, + &deny_write_paths, + common.logs_base_dir.as_deref(), + ); + unsafe { + CloseHandle(security.h_token); + } + desktop_name? + }; args.push(PRIVATE_DESKTOP_FLAG.to_string()); + args.push(PRIVATE_DESKTOP_NAME_FLAG.to_string()); + args.push(desktop_name); } if proxy_enforced { args.push(PROXY_ENFORCED_FLAG.to_string()); @@ -118,7 +244,7 @@ pub fn create_windows_sandbox_command_args_for_permission_profile( } args.push("--".to_string()); args.extend(command); - args + Ok(args) } fn push_json_arg(args: &mut Vec, flag: &str, value: &T) { @@ -164,6 +290,7 @@ struct WindowsSandboxWrapperRequest { permission_profile: PermissionProfile, windows_sandbox_level: WindowsSandboxLevel, windows_sandbox_private_desktop: bool, + private_desktop_name: Option, proxy_enforced: bool, network_proxy_restricting_sid: Option, proxy_settings_mode: crate::WindowsSandboxProxySettingsMode, @@ -179,8 +306,8 @@ async fn run_windows_sandbox_wrapper_request(request: WindowsSandboxWrapperReque if request.command.is_empty() { bail!("missing sandboxed command in windows sandbox wrapper request"); } - let spawned = - crate::spawn_windows_sandbox_session_for_level(crate::WindowsSandboxSessionRequest { + let spawned = crate::unified_exec::spawn_windows_sandbox_session_with_desktop( + crate::WindowsSandboxSessionRequest { permission_profile: &request.permission_profile, workspace_roots: request.workspace_roots.as_slice(), codex_home: request.codex_home.as_path(), @@ -200,8 +327,10 @@ async fn run_windows_sandbox_wrapper_request(request: WindowsSandboxWrapperReque tty: false, stdin_open: true, use_private_desktop: request.windows_sandbox_private_desktop, - }) - .await?; + }, + request.private_desktop_name, + ) + .await?; Ok(crate::forward_sandbox_session_stdio(spawned).await) } @@ -215,6 +344,7 @@ fn parse_windows_sandbox_wrapper_args(args: Vec) -> Result) -> Result windows_sandbox_private_desktop = true, + PRIVATE_DESKTOP_NAME_FLAG => { + private_desktop_name = Some(next_flag_value(&mut args, &arg)?); + } PRESERVE_PROXY_SETTINGS_FLAG => { proxy_settings_mode = crate::WindowsSandboxProxySettingsMode::Preserve; } @@ -291,6 +424,9 @@ fn parse_windows_sandbox_wrapper_args(args: Vec) -> Result) -> Result