diff --git a/codex-rs/app-server/src/lib.rs b/codex-rs/app-server/src/lib.rs index aa565b6b34..36dd9dbca8 100644 --- a/codex-rs/app-server/src/lib.rs +++ b/codex-rs/app-server/src/lib.rs @@ -497,6 +497,8 @@ pub async fn run_main_with_transport_options( auth: AppServerWebsocketAuthSettings, runtime_options: AppServerRuntimeOptions, ) -> IoResult { + #[cfg(target_os = "windows")] + let _registered_core = codex_windows_sandbox::registered_core_requested(); let loader_overrides = loader_overrides_with_test_user_config_file( loader_overrides, test_user_config_file_from_env(), diff --git a/codex-rs/app-server/src/message_processor.rs b/codex-rs/app-server/src/message_processor.rs index 9c7c11281f..2c5bba1729 100644 --- a/codex-rs/app-server/src/message_processor.rs +++ b/codex-rs/app-server/src/message_processor.rs @@ -1120,11 +1120,11 @@ impl MessageProcessor { .read(params) .await .map(|response| Some(response.into())), - ClientRequest::WindowsSandboxReadiness { .. } => self - .windows_sandbox_processor - .windows_sandbox_readiness() - .await - .map(|response| Some(response.into())), + ClientRequest::WindowsSandboxReadiness { .. } => { + self.windows_sandbox_processor + .windows_sandbox_readiness(&request_id) + .await + } ClientRequest::ExternalAgentConfigDetect { params, .. } => self .external_agent_config_processor .detect(params) diff --git a/codex-rs/app-server/src/request_processors/windows_sandbox_processor.rs b/codex-rs/app-server/src/request_processors/windows_sandbox_processor.rs index dc84430ca5..41a6233d0a 100644 --- a/codex-rs/app-server/src/request_processors/windows_sandbox_processor.rs +++ b/codex-rs/app-server/src/request_processors/windows_sandbox_processor.rs @@ -1,10 +1,14 @@ use super::*; +#[cfg(target_os = "windows")] +use anyhow::Context as _; #[derive(Clone)] pub(crate) struct WindowsSandboxRequestProcessor { outgoing: Arc, config: Arc, config_manager: ConfigManager, + #[cfg(target_os = "windows")] + registration_refresh: Arc>, } impl WindowsSandboxRequestProcessor { @@ -17,13 +21,67 @@ impl WindowsSandboxRequestProcessor { outgoing, config, config_manager, + #[cfg(target_os = "windows")] + registration_refresh: Arc::default(), } } pub(crate) async fn windows_sandbox_readiness( &self, - ) -> Result { - Ok(determine_windows_sandbox_readiness(&self.config)) + request_id: &ConnectionRequestId, + ) -> Result, JSONRPCErrorError> { + #[cfg(target_os = "windows")] + if codex_windows_sandbox::registered_core_requested() + && matches!( + WindowsSandboxLevel::from_config(&self.config), + WindowsSandboxLevel::Elevated + ) + && !codex_login::is_workload_identity_selected() + { + let processor = self.clone(); + let request_id = request_id.clone(); + // Deployment must not hold up unrelated RPCs in the serial dispatcher. + tokio::spawn(async move { + // Coalesce readiness requests. A failed refresh leaves manual setup available. + processor.registration_refresh.get_or_init(|| async { + let config = Arc::clone(&processor.config); + let result = tokio::task::spawn_blocking(move || -> anyhow::Result<()> { + if !codex_windows_sandbox::registered_core_needs_refresh(&config.codex_home)? { + return Ok(()); + } + let env_map = std::env::vars().collect(); + let policy = config.permissions.effective_permission_profile(); + let (settings, listeners) = match config.permissions.network.as_ref() { + Some(network) => network.windows_sandbox_proxy_listeners()?, + None => ( + codex_windows_sandbox::WindowsSandboxProvisioningSettings::from_environment(&policy, &env_map), + codex_windows_sandbox::WindowsSandboxProxyListeners::from_environment(&policy, &env_map), + ), + }; + codex_windows_sandbox::refresh_registered_core_via_service( + &config.codex_home, settings, listeners, + )?; + Ok(()) + }).await.map_err(anyhow::Error::from).and_then(std::convert::identity); + if let Err(error) = result { + warn!("Registered Core startup refresh requires manual setup: {error:#}"); + } + }).await; + processor + .outgoing + .send_response( + request_id, + determine_windows_sandbox_readiness(&processor.config), + ) + .await; + }); + return Ok(None); + } + #[cfg(not(target_os = "windows"))] + let _ = request_id; + Ok(Some( + determine_windows_sandbox_readiness(&self.config).into(), + )) } pub(crate) async fn windows_sandbox_setup_start( @@ -79,7 +137,8 @@ impl WindowsSandboxRequestProcessor { // the caller's resolved configuration instead of loading auth in the service. #[cfg(target_os = "windows")] if setup_mode == CoreWindowsSandboxSetupMode::Elevated - && config.features.enabled(Feature::WindowsSandboxService) + && (codex_windows_sandbox::registered_core_requested() + || config.features.enabled(Feature::WindowsSandboxService)) && !codex_login::is_workload_identity_selected() { let provisioning = match config @@ -104,6 +163,9 @@ impl WindowsSandboxRequestProcessor { { Ok(provisioning) => Some(provisioning), Err(error) => { + if codex_windows_sandbox::registered_core_requested() { + return Err(error).context("registered Core requires service-compatible proxy settings"); + } warn!( "Windows sandbox service does not support the configured proxy listeners; falling back to elevated setup: {error}" ); @@ -118,6 +180,8 @@ impl WindowsSandboxRequestProcessor { &service_setup_request.permission_profile, &service_setup_request.workspace_roots, ) else { + anyhow::ensure!(!codex_windows_sandbox::registered_core_requested(), + "registered Core requires a service-compatible sandbox policy"); // The existing setup path can still succeed for completed // provisioning without resolving the current profile. return Ok(()); @@ -177,12 +241,15 @@ async fn load_setup_config( fallback_cwd: &std::path::Path, requested_cwd: Option, ) -> std::io::Result<(Config, PathBuf)> { + // Setup without a project must not grant writes to the app's install directory. + let workspace_roots = requested_cwd.is_none().then(Vec::new); let cwd = requested_cwd.unwrap_or_else(|| fallback_cwd.to_path_buf()); let config = manager .load_for_cwd( /*request_overrides*/ None, ConfigOverrides { cwd: Some(cwd.clone()), + workspace_roots, ..Default::default() }, Some(cwd.clone()), diff --git a/codex-rs/app-server/src/request_processors/windows_sandbox_setup_config_tests.rs b/codex-rs/app-server/src/request_processors/windows_sandbox_setup_config_tests.rs index a9683e6341..073c947af9 100644 --- a/codex-rs/app-server/src/request_processors/windows_sandbox_setup_config_tests.rs +++ b/codex-rs/app-server/src/request_processors/windows_sandbox_setup_config_tests.rs @@ -5,6 +5,21 @@ use super::load_setup_config; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; +#[tokio::test] +async fn omitted_cwd_does_not_make_the_server_directory_a_workspace() -> anyhow::Result<()> { + let home = tempfile::tempdir()?; + let install = tempfile::tempdir()?; + let manager = ConfigManager::without_managed_config_for_tests(home.path().to_path_buf()); + let (config, command_cwd) = + load_setup_config(&manager, install.path(), /*requested_cwd*/ None).await?; + + assert_eq!( + (command_cwd, config.effective_workspace_roots()), + (install.path().to_path_buf(), Vec::new()), + ); + Ok(()) +} + #[tokio::test] async fn explicit_cwd_remains_the_setup_workspace() -> anyhow::Result<()> { let home = tempfile::tempdir()?; diff --git a/codex-rs/sandboxing/src/manager.rs b/codex-rs/sandboxing/src/manager.rs index f14f4851d5..9f9fef3775 100644 --- a/codex-rs/sandboxing/src/manager.rs +++ b/codex-rs/sandboxing/src/manager.rs @@ -736,14 +736,24 @@ fn wrap_windows_sandbox_exec_request_for_direct_spawn( #[cfg(target_os = "windows")] fn add_windows_sandbox_wrapper_setup_env(env: &mut HashMap) { - add_windows_sandbox_wrapper_setup_env_from_vars(env, std::env::vars_os()); + add_windows_sandbox_wrapper_setup_env_from_vars( + env, + std::env::vars_os(), + codex_windows_sandbox::registered_core_requested(), + ); } #[cfg(target_os = "windows")] fn add_windows_sandbox_wrapper_setup_env_from_vars( env: &mut HashMap, vars: impl IntoIterator, + registered_core: bool, ) { + // This outer helper must use the parent's runtime selection, not shell-policy overrides. + env.retain(|key, _| !key.eq_ignore_ascii_case("CODEX_WINDOWS_REGISTERED_CORE")); + if registered_core { + env.insert("CODEX_WINDOWS_REGISTERED_CORE".into(), "1".into()); + } for (key, value) in vars { let key = key.to_string_lossy().into_owned(); if !WINDOWS_SANDBOX_WRAPPER_SETUP_ENV_ALLOWLIST diff --git a/codex-rs/sandboxing/src/manager_tests.rs b/codex-rs/sandboxing/src/manager_tests.rs index c6936ae162..fc24c4691c 100644 --- a/codex-rs/sandboxing/src/manager_tests.rs +++ b/codex-rs/sandboxing/src/manager_tests.rs @@ -680,6 +680,7 @@ fn transform_for_direct_spawn_windows_preserves_only_wrapper_setup_environment() std::ffi::OsString::from(value), ) }), + /*registered_core*/ false, ); assert_eq!( @@ -693,6 +694,27 @@ fn transform_for_direct_spawn_windows_preserves_only_wrapper_setup_environment() ); } +#[cfg(target_os = "windows")] +#[test] +fn wrapper_runtime_selection_uses_the_parent_not_environment_overrides() { + for registered_core in [false, true] { + let mut env = HashMap::from([("codex_windows_registered_core".into(), "1".into())]); + super::add_windows_sandbox_wrapper_setup_env_from_vars( + &mut env, + [("CODEX_WINDOWS_REGISTERED_CORE".into(), "0".into())], + registered_core, + ); + assert_eq!( + env, + if registered_core { + HashMap::from([("CODEX_WINDOWS_REGISTERED_CORE".into(), "1".into())]) + } else { + HashMap::new() + } + ); + } +} + #[cfg(target_os = "windows")] #[test] fn transform_for_direct_spawn_windows_materializes_inner_helper() { diff --git a/codex-rs/windows-sandbox-rs/src/app_package.rs b/codex-rs/windows-sandbox-rs/src/app_package.rs new file mode 100644 index 0000000000..3e4d5d09d3 --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/app_package.rs @@ -0,0 +1,243 @@ +//! Select the requested runtime without inferring authority from directory or manifest text. +//! Registered runners still require OS package identity and the exact staged runner image. + +use anyhow::Context; +use anyhow::Result; +use anyhow::bail; +use anyhow::ensure; +use std::ffi::OsStr; +use std::ffi::OsString; +use std::os::windows::ffi::OsStringExt; +use std::path::Path; +use std::path::PathBuf; +use std::sync::OnceLock; +use windows_sys::Win32::Foundation::APPMODEL_ERROR_NO_PACKAGE; +use windows_sys::Win32::Foundation::ERROR_INSUFFICIENT_BUFFER; +use windows_sys::Win32::Foundation::ERROR_SUCCESS; +use windows_sys::Win32::Foundation::HANDLE; +use windows_sys::Win32::NetworkManagement::NetManagement::UF_ACCOUNTDISABLE; +use windows_sys::Win32::Storage::Packaging::Appx::GetPackageFullName; +use windows_sys::Win32::System::Threading::GetCurrentProcess; +use windows_sys::Win32::System::Threading::QueryFullProcessImageNameW; + +#[link(name = "kernel32")] +unsafe extern "system" { + fn GetStagedPackagePathByFullName( + package_full_name: *const u16, + path_length: *mut u32, + path: *mut u16, + ) -> i32; +} + +/// Capture the startup request once; this flag never authorizes a package or process. +pub fn registered_core_requested() -> bool { + static REQUESTED: OnceLock = OnceLock::new(); + *REQUESTED.get_or_init(|| { + requested_value(std::env::var_os("CODEX_WINDOWS_REGISTERED_CORE").as_deref()) + }) +} + +fn requested_value(value: Option<&OsStr>) -> bool { + value == Some(OsStr::new("1")) +} + +fn current_package_full_name() -> Result> { + process_package_name(unsafe { GetCurrentProcess() }) +} + +fn process_package_name(process: HANDLE) -> Result> { + query_package_name( + |length, buffer| unsafe { GetPackageFullName(process, length, buffer) }, + /*max_length*/ 32768, + ) +} + +pub(crate) fn query_package_name( + mut query: impl FnMut(*mut u32, *mut u16) -> u32, + max_length: u32, +) -> Result> { + let mut length = 0; + let status = query(&mut length, std::ptr::null_mut()); + if status == APPMODEL_ERROR_NO_PACKAGE { + return Ok(None); + } + ensure!( + status == ERROR_INSUFFICIENT_BUFFER && length > 1 && length <= max_length, + "package name query failed: {status}" + ); + let mut family = vec![0u16; length as usize]; + let status = query(&mut length, family.as_mut_ptr()); + ensure!( + status == ERROR_SUCCESS && length > 1 && length as usize <= family.len(), + "package name read failed: {status}" + ); + let family = family[..length as usize] + .strip_suffix(&[0]) + .context("unterminated package name")?; + ensure!(!family.contains(&0), "invalid package name"); + Ok(Some(String::from_utf16(family)?)) +} + +/// Only an OS-verified packaged runner may propagate package context to sandbox children. +pub(crate) fn current_process_is_registered_core_runner() -> Result { + let executable = std::env::current_exe().context("resolve current Core runner image")?; + if !executable + .file_name() + .is_some_and(|name| name.eq_ignore_ascii_case("codex-command-runner.exe")) + { + return Ok(false); + } + let process = unsafe { GetCurrentProcess() }; + if process_package_name(process)?.is_none() { + ensure!( + !registered_core_requested(), + "registered Core runner has no package identity" + ); + return Ok(false); + } + verify_registered_core_runner(process, &executable)?; + Ok(true) +} + +fn staged_package_root(name: &[u16]) -> Result { + let mut staged = vec![0u16; 32768]; + let mut length = staged.len() as u32; + let status = + unsafe { GetStagedPackagePathByFullName(name.as_ptr(), &mut length, staged.as_mut_ptr()) }; + ensure!( + status == 0 && length > 1 && length as usize <= staged.len(), + "registered Core staged package query failed: {status}" + ); + let staged = &staged[..length as usize]; + ensure!( + staged.last() == Some(&0) && !staged[..staged.len() - 1].contains(&0), + "invalid registered Core staged package path" + ); + dunce::canonicalize(OsString::from_wide(&staged[..staged.len() - 1])) + .context("resolve OS-staged package root") +} + +pub(crate) fn verify_registered_core_runner(process: HANDLE, expected_runner: &Path) -> Result<()> { + let name = + process_package_name(process)?.context("registered Core runner has no package identity")?; + let staged = staged_package_root(&crate::winutil::to_wide(name))?; + let expected = + dunce::canonicalize(expected_runner).context("resolve registered Core runner")?; + ensure!( + expected.as_os_str().eq_ignore_ascii_case( + staged + .join("app") + .join("resources") + .join("codex-command-runner.exe") + .as_os_str() + ), + "registered alias selected a different package identity" + ); + let mut image = vec![0u16; 32768]; + let mut length = image.len() as u32; + if unsafe { QueryFullProcessImageNameW(process, 0, image.as_mut_ptr(), &mut length) } == 0 { + return Err(std::io::Error::last_os_error()) + .context("inspect registered Core runner image"); + } + ensure!( + length > 0 && length as usize <= image.len(), + "invalid registered Core image length" + ); + let image = dunce::canonicalize(OsString::from_wide(&image[..length as usize]))?; + ensure!( + image.as_os_str().eq_ignore_ascii_case(expected.as_os_str()), + "registered alias selected a different runner image" + ); + Ok(()) +} + +/// Checks the service's committed receipt without provisioning or mutating either account. +pub(crate) fn registered_setup_is_ready(codex_home: &Path) -> Result { + for account in [ + crate::setup::OFFLINE_USERNAME, + crate::setup::ONLINE_USERNAME, + ] { + if crate::winutil::local_user_flags(account)? + .is_none_or(|flags| flags & UF_ACCOUNTDISABLE != 0) + { + return Ok(false); + } + } + let Some(package) = current_package_full_name()? else { + return Ok(false); + }; + let Some(record) = crate::runtime_ownership::load_installation()? else { + return Ok(false); + }; + Ok(record.codex_home == codex_home.canonicalize()? + && record.runtime()?.ready_for_package(&package)) +} + +/// A startup hint only; the service rechecks ownership and readiness under its setup lock. +#[doc(hidden)] +pub fn registered_core_needs_refresh(codex_home: &Path) -> Result { + let Some(package) = current_package_full_name()? else { + return Ok(false); + }; + let Some(record) = crate::runtime_ownership::load_installation()? else { + return Ok(false); + }; + let runtime = record.runtime()?; + Ok(record.codex_home == codex_home.canonicalize()? + && runtime + .ready_package + .as_deref() + .is_some_and(|ready| ready != package && runtime.ready_for_package(ready))) +} + +/// Consume the service's setup receipt; launching a command never installs a package. +pub(crate) fn registered_runner_alias( + codex_home: &Path, + sandbox_username: &str, +) -> anyhow::Result { + let account = if sandbox_username.eq_ignore_ascii_case(crate::setup::OFFLINE_USERNAME) { + crate::SandboxRuntimeAccount::Offline + } else if sandbox_username.eq_ignore_ascii_case(crate::setup::ONLINE_USERNAME) { + crate::SandboxRuntimeAccount::Online + } else { + bail!("app runtime registration requires a managed sandbox account"); + }; + let record = crate::runtime_ownership::load_installation()? + .context("registered Core setup has not completed")?; + let package = + current_package_full_name()?.context("registered Core launch requires an installed app")?; + let owner = crate::winutil::resolve_sid(&crate::runtime_ownership::current_setup_user()?)?; + anyhow::ensure!( + record.user_sid + == crate::winutil::string_from_sid_bytes(&owner).map_err(anyhow::Error::msg)? + && record.codex_home == codex_home.canonicalize()? + && record.runtime()?.ready_for_package(&package), + "registered Core setup belongs to another owner or is being removed" + ); + let entry = record + .runtime()? + .accounts + .iter() + .find(|entry| entry.account == account) + .context("managed account registration is not ready; retry setup")?; + let sid = crate::winutil::resolve_sid(sandbox_username)?; + anyhow::ensure!( + entry.user_sid + == crate::winutil::string_from_sid_bytes(&sid).map_err(anyhow::Error::msg)?, + "managed runtime account changed; retry setup" + ); + let path = entry + .alias_path + .as_ref() + .context("registered Core alias is not ready; retry setup")?; + anyhow::ensure!( + path.is_absolute() + && path.file_name() == Some(std::ffi::OsStr::new(crate::APP_CORE_RUNNER_ALIAS)), + "registered Core setup contains an invalid alias" + ); + Ok(path.clone()) +} + +#[cfg(test)] +#[path = "app_package_tests.rs"] +mod tests; diff --git a/codex-rs/windows-sandbox-rs/src/app_package_tests.rs b/codex-rs/windows-sandbox-rs/src/app_package_tests.rs new file mode 100644 index 0000000000..015acffcf8 --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/app_package_tests.rs @@ -0,0 +1,98 @@ +//! Runtime opt-in and bounded OS package-name results, without installed-state changes. + +use super::query_package_name; +use super::requested_value; +use pretty_assertions::assert_eq; +use std::ffi::OsStr; +use std::ffi::OsString; +use std::os::windows::ffi::OsStringExt; +use windows_sys::Win32::Foundation::APPMODEL_ERROR_NO_PACKAGE; +use windows_sys::Win32::Foundation::ERROR_ACCESS_DENIED; +use windows_sys::Win32::Foundation::ERROR_INSUFFICIENT_BUFFER; +use windows_sys::Win32::Foundation::ERROR_SUCCESS; + +#[test] +fn only_exact_opt_in_requests_registered_core() { + let values = [ + None, + Some(""), + Some("0"), + Some("1"), + Some("true"), + Some(" 1"), + Some("1 "), + ]; + assert_eq!( + values.map(|value| requested_value(value.map(OsStr::new))), + [false, false, false, true, false, false, false] + ); + assert!(!requested_value(Some(&OsString::from_wide(&[0xd800])))); +} + +#[test] +fn package_query_distinguishes_absence_from_failure_and_bounds_allocation() { + assert_eq!( + query_package_name(|_, _| APPMODEL_ERROR_NO_PACKAGE, /*max_length*/ 256).unwrap(), + None + ); + assert!(query_package_name(|_, _| ERROR_ACCESS_DENIED, /*max_length*/ 256).is_err()); + for max_length in [256, 32768] { + assert!( + query_package_name( + |length, buffer| { + assert!(buffer.is_null()); + unsafe { *length = max_length + 1 }; + ERROR_INSUFFICIENT_BUFFER + }, + max_length + ) + .is_err() + ); + } +} + +#[test] +fn package_query_validates_returned_length_termination_and_utf16() { + for (value, returned, expected) in [ + (vec![65, 0], 2, Some("A")), + (vec![65, 0], 0, None), + (vec![65, 0], 3, None), + (vec![65, 66], 2, None), + (vec![65, 0, 66, 0], 4, None), + (vec![0xd800, 0], 2, None), + ] { + let actual = query_package_name( + |length, buffer| { + if buffer.is_null() { + unsafe { *length = value.len() as u32 }; + ERROR_INSUFFICIENT_BUFFER + } else { + unsafe { + std::ptr::copy_nonoverlapping(value.as_ptr(), buffer, value.len()); + *length = returned; + } + ERROR_SUCCESS + } + }, + /*max_length*/ 256, + ); + match expected { + Some(name) => assert_eq!(actual.unwrap(), Some(name.to_owned())), + None => assert!(actual.is_err()), + } + } + assert!( + query_package_name( + |length, buffer| { + unsafe { *length = 2 }; + if buffer.is_null() { + ERROR_INSUFFICIENT_BUFFER + } else { + ERROR_ACCESS_DENIED + } + }, + /*max_length*/ 256 + ) + .is_err() + ); +} diff --git a/codex-rs/windows-sandbox-rs/src/conpty/mod.rs b/codex-rs/windows-sandbox-rs/src/conpty/mod.rs index a8bb27a13c..363421ec19 100644 --- a/codex-rs/windows-sandbox-rs/src/conpty/mod.rs +++ b/codex-rs/windows-sandbox-rs/src/conpty/mod.rs @@ -134,9 +134,13 @@ pub fn spawn_conpty_process_as_user( job: Some(Arc::clone(&job)), _desktop: Some(desktop), }; - let mut attrs = ProcThreadAttributeList::new(/*attr_count*/ 2)?; + let preserve_app_context = crate::app_package::current_process_is_registered_core_runner()?; + let mut attrs = ProcThreadAttributeList::new(2 + u32::from(preserve_app_context))?; attrs.set_pseudoconsole(hpc)?; attrs.set_job(job.as_raw_handle() as HANDLE)?; + if preserve_app_context { + attrs.preserve_desktop_app_context()?; + } si.lpAttributeList = attrs.as_mut_ptr(); let mut pi: PROCESS_INFORMATION = unsafe { std::mem::zeroed() }; diff --git a/codex-rs/windows-sandbox-rs/src/desktop.rs b/codex-rs/windows-sandbox-rs/src/desktop.rs index 0c23e8d146..4311ba0703 100644 --- a/codex-rs/windows-sandbox-rs/src/desktop.rs +++ b/codex-rs/windows-sandbox-rs/src/desktop.rs @@ -106,15 +106,17 @@ impl DesktopPolicy { network_proxy_restricting_sid: Option<&str>, ) -> Result { // Match the complete read override passed by credential setup to the ACL helper. + let runtime = crate::setup::current_setup_runtime(); overrides.read_roots.get_or_insert_with(|| { gather_read_roots( request.command_cwd, request.permissions, request.env_map, request.codex_home, + runtime, ) }); - let (read_roots, write_roots) = build_payload_roots(&request, &overrides); + let (read_roots, write_roots) = build_payload_roots(&request, &overrides, runtime); Ok(Self { uses_write_capabilities: request .permissions diff --git a/codex-rs/windows-sandbox-rs/src/elevated/mod.rs b/codex-rs/windows-sandbox-rs/src/elevated/mod.rs index 3c8084bb62..70d1ad9ccd 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated/mod.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated/mod.rs @@ -1,3 +1,4 @@ pub(crate) mod ipc_framed; pub(crate) mod runner_client; +pub(crate) mod runner_metrics; pub(crate) mod runner_pipe; 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 d7430123fc..6f688f166d 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated/runner_client.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated/runner_client.rs @@ -1,3 +1,5 @@ +use crate::app_package::registered_core_requested; +use crate::app_package::verify_registered_core_runner; use crate::desktop::DesktopPolicy; use crate::desktop::shared_private_desktop_for_user; use crate::identity::SandboxCreds; @@ -19,9 +21,10 @@ use crate::winutil::quote_windows_arg; use crate::winutil::to_wide; use anyhow::Context; use anyhow::Result; -use std::ffi::c_void; use std::fs::File; +use std::os::windows::io::AsRawHandle; use std::os::windows::io::FromRawHandle; +use std::os::windows::io::OwnedHandle; use std::path::Path; use std::ptr; use std::sync::mpsc; @@ -42,6 +45,7 @@ use windows_sys::Win32::System::IO::CancelSynchronousIo; use windows_sys::Win32::System::Threading::CreateProcessWithLogonW; use windows_sys::Win32::System::Threading::GetCurrentProcess; use windows_sys::Win32::System::Threading::GetCurrentThread; +use windows_sys::Win32::System::Threading::LOGON_WITH_PROFILE; use windows_sys::Win32::System::Threading::PROCESS_INFORMATION; use windows_sys::Win32::System::Threading::STARTF_FORCEOFFFEEDBACK; use windows_sys::Win32::System::Threading::STARTUPINFOW; @@ -142,11 +146,13 @@ pub(crate) fn retry_runner_spawn_once( mut spawn: impl FnMut(SandboxCreds) -> Result, refresh: impl FnOnce() -> Result, ) -> Result { - match spawn(sandbox_creds) { + let result = match spawn(sandbox_creds) { Ok(result) => Ok(result), - Err(err) if is_refreshable_sandbox_creds_error(&err, command) => spawn(refresh()?), + Err(err) if is_refreshable_sandbox_creds_error(&err, command) => refresh().and_then(spawn), Err(err) => Err(err), - } + }; + super::runner_metrics::record("startup", if result.is_ok() { "success" } else { "error" }); + result } impl RunnerTransport { @@ -323,6 +329,15 @@ pub(crate) fn spawn_runner_transport( mut spawn_request: SpawnRequest, desktop_policy: Option<&DesktopPolicy>, ) -> Result { + let runner_exe = find_runner_exe(codex_home, log_dir)?; + let registered_alias = if registered_core_requested() { + Some(crate::app_package::registered_runner_alias( + codex_home, + &sandbox_creds.username, + )?) + } else { + None + }; if let Some(policy) = desktop_policy { spawn_request.private_desktop_name = Some(shared_private_desktop_for_user( &sandbox_creds.username, @@ -331,13 +346,26 @@ pub(crate) fn spawn_runner_transport( )?); } 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)?; - let h_pipe_out = - create_named_pipe(&pipe_out_name, PIPE_ACCESS_INBOUND, &sandbox_creds.username)?; + let pipe_write = unsafe { + File::from_raw_handle(create_named_pipe( + &pipe_in_name, + PIPE_ACCESS_OUTBOUND, + &sandbox_creds.username, + )? as _) + }; + let pipe_read = unsafe { + File::from_raw_handle(create_named_pipe( + &pipe_out_name, + PIPE_ACCESS_INBOUND, + &sandbox_creds.username, + )? as _) + }; + let h_pipe_in = pipe_write.as_raw_handle() as HANDLE; + let h_pipe_out = pipe_read.as_raw_handle() as HANDLE; - let runner_exe = find_runner_exe(codex_home, log_dir); - let runner_cmdline = runner_exe + let runner_cmdline = registered_alias + .as_deref() + .unwrap_or(&runner_exe) .to_str() .map(str::to_owned) .unwrap_or_else(|| "codex-command-runner.exe".to_string()); @@ -357,24 +385,25 @@ pub(crate) fn spawn_runner_transport( si.cb = std::mem::size_of::() as u32; si.dwFlags = STARTF_FORCEOFFFEEDBACK; let mut pi: PROCESS_INFORMATION = unsafe { std::mem::zeroed() }; - let env_block: Option> = None; let previous_error_mode = unsafe { SetErrorMode(RUNNER_ERROR_MODE_FLAGS) }; - // Sandbox users have no profile state that commands should inherit. + // Execution aliases require the registered account's profile. + // Other launches retain their existing profile-free behavior. let spawn_res = unsafe { CreateProcessWithLogonW( user_w.as_ptr(), domain_w.as_ptr(), password_w.as_ptr(), - /*dwlogonflags*/ 0, + if registered_alias.is_some() { + LOGON_WITH_PROFILE + } else { + 0 + }, exe_w.as_ptr(), cmdline_vec.as_mut_ptr(), windows_sys::Win32::System::Threading::CREATE_NO_WINDOW | windows_sys::Win32::System::Threading::CREATE_UNICODE_ENVIRONMENT, - env_block - .as_ref() - .map(|block| block.as_ptr() as *const c_void) - .unwrap_or(ptr::null()), + ptr::null(), cwd_w.as_ptr(), &si, &mut pi, @@ -385,73 +414,43 @@ pub(crate) fn spawn_runner_transport( } if spawn_res == 0 { let err = unsafe { GetLastError() }; - unsafe { - CloseHandle(h_pipe_in); - CloseHandle(h_pipe_out); - } return Err(RunnerLogonError { code: err }.into()); } + // Keep the process pinned through the entire startup handshake. Pipes close + // automatically on every error, including failure to create the second pipe. + let _runner_process = unsafe { OwnedHandle::from_raw_handle(pi.hProcess as _) }; + let _runner_thread = unsafe { OwnedHandle::from_raw_handle(pi.hThread as _) }; let expected_runner_pid = pi.dwProcessId; - - let connect_result = (|| -> Result<()> { - connect_pipe_with_timeout(h_pipe_in, expected_runner_pid, "pipe-in")?; - connect_pipe_with_timeout(h_pipe_out, expected_runner_pid, "pipe-out")?; - Ok(()) - })(); - - unsafe { - if pi.hThread != 0 { - CloseHandle(pi.hThread); - } - } - - if let Err(err) = connect_result { - unsafe { - // Keep the process handle alive until the pipe handshake finishes. If the handshake - // fails after the runner process has already launched, we still need a way to stop - // that child instead of leaking a stray `codex-command-runner.exe`. - if pi.hProcess != 0 { - let _ = TerminateProcess(pi.hProcess, 1); - CloseHandle(pi.hProcess); - } - CloseHandle(h_pipe_in); - CloseHandle(h_pipe_out); - } - return Err(err); - } - let mut transport = RunnerTransport { - // Once the pipe connect phase succeeds we can transfer the raw HANDLEs into `File`s. - // From here on, the `RunnerTransport` owns closing the pipes on every success/error path. - pipe_write: unsafe { File::from_raw_handle(h_pipe_in as _) }, - pipe_read: unsafe { File::from_raw_handle(h_pipe_out as _) }, + pipe_write, + pipe_read, }; let startup_result = (|| -> Result<()> { - // Keep the runner process HANDLE alive until the *entire* startup handshake finishes. - // That way, a later `send_spawn_request` or `spawn_ready` failure can still terminate the - // runner instead of leaving a stray `codex-command-runner.exe` behind. + // An update can retarget the alias after setup. Check the selected image + // and authenticate its pipes before sending it the command. + if registered_alias.is_some() { + verify_registered_core_runner(pi.hProcess, &runner_exe)?; + } + connect_pipe_with_timeout(h_pipe_in, expected_runner_pid, "pipe-in")?; + connect_pipe_with_timeout(h_pipe_out, expected_runner_pid, "pipe-out")?; transport.send_spawn_request(spawn_request)?; transport.read_spawn_ready()?; Ok(()) })(); if let Err(err) = startup_result { unsafe { - if pi.hProcess != 0 { - let _ = TerminateProcess(pi.hProcess, 1); - CloseHandle(pi.hProcess); - } + let _ = TerminateProcess(pi.hProcess, 1); } - drop(transport); return Err(err); } - unsafe { - if pi.hProcess != 0 { - // The runner has now connected both pipes *and* acknowledged the spawn request, so - // startup is complete. At that point the transport pipes become the only lifetime - // anchor we need to keep the session alive. - CloseHandle(pi.hProcess); - } + if registered_alias.is_some() { + crate::logging::debug_log( + &format!( + "registered_core_runner_ready pid={expected_runner_pid} package_verified=true pipes_authenticated=true" + ), + log_dir, + ); } Ok(transport) diff --git a/codex-rs/windows-sandbox-rs/src/elevated/runner_metrics.rs b/codex-rs/windows-sandbox-rs/src/elevated/runner_metrics.rs new file mode 100644 index 0000000000..460e5317ed --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/elevated/runner_metrics.rs @@ -0,0 +1,35 @@ +//! Compare registered and legacy runner outcomes at the actual local execution boundary. +//! Startup is counted once after credential retry; commands are counted on exit or pipe failure. + +pub(crate) fn record(phase: &'static str, outcome: &'static str) { + let runtime = if crate::app_package::registered_core_requested() { + "registered" + } else { + "legacy" + }; + if let Some(metrics) = codex_otel::global() { + // Fixed labels only: never send command text, paths, or arbitrary error messages. + let _ = metrics.counter( + "codex.windows_sandbox.runner_result", + /*inc*/ 1, + &[("runtime", runtime), ("phase", phase), ("outcome", outcome)], + ); + } +} + +pub(crate) fn record_command(exit: Option<(i32, bool)>) { + record("command", command_outcome(exit)); +} + +fn command_outcome(exit: Option<(i32, bool)>) -> &'static str { + match exit { + Some((_, true)) => "timeout", + Some((0, false)) => "success", + Some((_, false)) => "nonzero_exit", + None => "transport_error", + } +} + +#[cfg(test)] +#[path = "runner_metrics_tests.rs"] +mod tests; diff --git a/codex-rs/windows-sandbox-rs/src/elevated/runner_metrics_tests.rs b/codex-rs/windows-sandbox-rs/src/elevated/runner_metrics_tests.rs new file mode 100644 index 0000000000..7d7e17ee49 --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/elevated/runner_metrics_tests.rs @@ -0,0 +1,27 @@ +//! Keep command outcomes distinct without publishing arbitrary exit-code metric labels. + +use super::command_outcome; +use pretty_assertions::assert_eq; + +#[test] +fn command_results_distinguish_timeouts_transport_errors_and_nonzero_exits() { + assert_eq!( + [ + Some((0, false)), + Some((1, false)), + Some((0xc0000135u32 as i32, false)), // Windows DLL-not-found status. + Some((0, true)), + Some((1, true)), + None, + ] + .map(command_outcome), + [ + "success", + "nonzero_exit", + "nonzero_exit", + "timeout", + "timeout", + "transport_error", + ] + ); +} diff --git a/codex-rs/windows-sandbox-rs/src/elevated/runner_pipe.rs b/codex-rs/windows-sandbox-rs/src/elevated/runner_pipe.rs index b23f7fd380..463005ce77 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated/runner_pipe.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated/runner_pipe.rs @@ -37,9 +37,8 @@ pub const PIPE_ACCESS_INBOUND: u32 = 0x0000_0001; /// PIPE_ACCESS_OUTBOUND (win32 constant), not exposed in windows-sys 0.52. pub const PIPE_ACCESS_OUTBOUND: u32 = 0x0000_0002; -/// Resolves the elevated command runner path, preferring the copied helper under -/// `.sandbox-bin` and falling back to the legacy sibling lookup when needed. -pub fn find_runner_exe(codex_home: &Path, log_dir: Option<&Path>) -> PathBuf { +/// Resolve the installed runner for registered Core; otherwise use the legacy copy path. +pub fn find_runner_exe(codex_home: &Path, log_dir: Option<&Path>) -> anyhow::Result { resolve_command_runner(codex_home, log_dir) } diff --git a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs index 702884acbd..01368975c1 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs @@ -292,6 +292,7 @@ mod windows_impl { let _ = cancel_handle.join(); } drop(pipe_write); + crate::elevated::runner_metrics::record_command(result.as_ref().ok().copied()); let (exit_code, timed_out) = result?; if exit_code == 0 { diff --git a/codex-rs/windows-sandbox-rs/src/helper_materialization.rs b/codex-rs/windows-sandbox-rs/src/helper_materialization.rs index 1ada0afaa2..1d219422da 100644 --- a/codex-rs/windows-sandbox-rs/src/helper_materialization.rs +++ b/codex-rs/windows-sandbox-rs/src/helper_materialization.rs @@ -1,4 +1,6 @@ -//! Selects sandbox helper paths; legacy file copying lives in the copy module. +//! Resolve sandbox helpers and materialize legacy executables with inherited sandbox ACLs. +//! An explicit registered-runtime request never falls through to copying or PATH lookup; +//! the service and startup handshake independently verify the installed image. mod copy; use copy::CopyOutcome; @@ -7,31 +9,27 @@ use copy::copy_from_source_if_needed; use anyhow::Context; use anyhow::Result; use anyhow::anyhow; -use std::collections::HashMap; use std::ffi::OsStr; use std::fs; - use std::path::Path; use std::path::PathBuf; -use std::sync::Mutex; -use std::sync::OnceLock; use std::time::UNIX_EPOCH; +use crate::app_package::registered_core_requested; use crate::logging::log_note; use crate::sandbox_bin_dir; +use crate::setup::SetupRuntime; const DEV_BUILD_VERSION_SENTINEL: &str = "0.0.0"; const COMMAND_RUNNER_EXE: &str = "codex-command-runner.exe"; pub(crate) const BIN_DIRNAME: &str = "bin"; pub(crate) const RESOURCES_DIRNAME: &str = "codex-resources"; -static HELPER_PATH_CACHE: OnceLock>> = OnceLock::new(); - pub(crate) fn helper_bin_dir(codex_home: &Path) -> PathBuf { sandbox_bin_dir(codex_home) } -pub(crate) fn legacy_lookup() -> PathBuf { +fn legacy_lookup() -> PathBuf { if let Ok(exe) = std::env::current_exe() && let Some(candidate) = bundled_executable_path_for_exe(&exe, COMMAND_RUNNER_EXE) { @@ -40,8 +38,21 @@ pub(crate) fn legacy_lookup() -> PathBuf { PathBuf::from(COMMAND_RUNNER_EXE) } -pub(crate) fn resolve_command_runner(codex_home: &Path, log_dir: Option<&Path>) -> PathBuf { - match copy_runner_if_needed(codex_home, log_dir) { +pub(crate) fn resolve_command_runner(codex_home: &Path, log_dir: Option<&Path>) -> Result { + if registered_core_requested() { + let exe = std::env::current_exe().context("resolve registered Core helper source")?; + let direct_path = exe.with_file_name(COMMAND_RUNNER_EXE); + log_note( + &format!( + "helper launch resolution: using app-contained command-runner path {}", + direct_path.display() + ), + log_dir, + ); + // Missing packaged helpers must fail rather than search PATH or create a copy. + return Ok(direct_path); + } + Ok(match copy_runner_if_needed(codex_home, log_dir) { Ok(path) => { log_note( &format!( @@ -63,26 +74,44 @@ pub(crate) fn resolve_command_runner(codex_home: &Path, log_dir: Option<&Path>) ); fallback } - } -} - -pub fn resolve_current_exe_for_launch(codex_home: &Path, fallback_executable: &str) -> PathBuf { - let source = match std::env::current_exe() { - Ok(path) => path, - Err(_) => return PathBuf::from(fallback_executable), - }; - resolve_exe_for_launch(&source, codex_home) + }) } pub fn resolve_exe_for_launch(source: &Path, codex_home: &Path) -> PathBuf { + let runtime = crate::setup::current_setup_runtime(); + resolve_exe_for_runtime(source, codex_home, runtime) +} + +fn resolve_exe_for_runtime(source: &Path, codex_home: &Path, runtime: SetupRuntime) -> PathBuf { + let sandbox_log_dir = crate::sandbox_dir(codex_home); + if runtime == SetupRuntime::Registered { + log_note( + &format!( + "helper executable resolution: route=direct source={} selected={}", + source.display(), + source.display() + ), + Some(&sandbox_log_dir), + ); + return source.to_path_buf(); + } let Some(file_name) = source.file_name() else { return source.to_path_buf(); }; let destination = helper_bin_dir(codex_home).join(file_name); match copy_from_source_if_needed(source, &destination) { - Ok(_) => destination, + Ok(_) => { + log_note( + &format!( + "helper executable resolution: route=materialized source={} selected={}", + source.display(), + destination.display() + ), + Some(&sandbox_log_dir), + ); + destination + } Err(err) => { - let sandbox_log_dir = crate::sandbox_dir(codex_home); log_note( &format!( "helper copy failed for executable: {err:#}; falling back to legacy path {}", @@ -96,20 +125,9 @@ pub fn resolve_exe_for_launch(source: &Path, codex_home: &Path) -> PathBuf { } fn copy_runner_if_needed(codex_home: &Path, log_dir: Option<&Path>) -> Result { - let cache_key = format!("{}|{}", COMMAND_RUNNER_EXE, codex_home.display()); - if let Some(path) = cached_helper_path(&cache_key) { - log_note( - &format!( - "helper copy: using in-memory cache for command-runner -> {}", - path.display() - ), - log_dir, - ); - return Ok(path); - } - let source = sibling_source_path()?; - let destination = helper_destination_for_source(codex_home, &source)?; + let suffix = helper_version_suffix(&source)?; + let destination = helper_bin_dir(codex_home).join(materialized_file_name(&suffix)); log_note( &format!( "helper copy: validating command-runner source={} destination={}", @@ -132,23 +150,9 @@ fn copy_runner_if_needed(codex_home: &Path, log_dir: Option<&Path>) -> Result Option { - let cache = HELPER_PATH_CACHE.get_or_init(|| Mutex::new(HashMap::new())); - let guard = cache.lock().ok()?; - guard.get(cache_key).cloned() -} - -fn store_helper_path(cache_key: String, path: PathBuf) { - let cache = HELPER_PATH_CACHE.get_or_init(|| Mutex::new(HashMap::new())); - if let Ok(mut guard) = cache.lock() { - guard.insert(cache_key, path); - } -} - fn sibling_source_path() -> Result { let exe = std::env::current_exe().context("resolve current executable for helper lookup")?; bundled_executable_path_for_exe(&exe, COMMAND_RUNNER_EXE).ok_or_else(|| { @@ -184,11 +188,6 @@ pub(crate) fn bundled_executable_path_for_exe(exe: &Path, file_name: &str) -> Op find(exe).or_else(|| find(&dunce::canonicalize(exe).ok()?)) } -fn helper_destination_for_source(codex_home: &Path, source: &Path) -> Result { - let suffix = helper_version_suffix(source)?; - Ok(helper_bin_dir(codex_home).join(materialized_file_name(&suffix))) -} - fn materialized_file_name(suffix: &str) -> String { format!("codex-command-runner-{suffix}.exe") } @@ -219,15 +218,15 @@ mod tests { use super::BIN_DIRNAME; use super::CopyOutcome; use super::DEV_BUILD_VERSION_SENTINEL; - use super::RESOURCES_DIRNAME; use super::bundled_executable_path_for_exe; use super::copy_from_source_if_needed; - use super::dev_build_suffix; use super::helper_bin_dir; use super::helper_version_suffix; use super::materialized_file_name; + use super::resolve_exe_for_runtime; + use crate::setup::SetupRuntime; use pretty_assertions::assert_eq; use std::fs; use std::path::Path; @@ -244,6 +243,37 @@ mod tests { ); } + #[test] + fn registered_request_does_not_materialize_or_replace_a_missing_source() { + let tmp = TempDir::new().expect("tempdir"); + let executable = tmp.path().join("codex.exe"); + let home = tmp.path().join("home"); + for content in [None, Some(b"fixture".as_slice())] { + if let Some(content) = content { + fs::write(&executable, content).expect("write source"); + } + assert_eq!( + resolve_exe_for_runtime(&executable, &home, SetupRuntime::Registered), + executable + ); + assert!(!helper_bin_dir(&home).exists()); + } + } + + #[test] + fn legacy_request_materializes_the_same_source() { + let tmp = TempDir::new().expect("tempdir"); + let executable = tmp.path().join("codex.exe"); + let home = tmp.path().join("home"); + fs::write(&executable, b"fixture").expect("write source"); + let destination = helper_bin_dir(&home).join("codex.exe"); + assert_eq!( + resolve_exe_for_runtime(&executable, &home, SetupRuntime::Legacy), + destination + ); + assert_eq!(fs::read(destination).expect("read copy"), b"fixture"); + } + #[test] fn copy_runner_into_shared_bin_dir() { let tmp = TempDir::new().expect("tempdir"); diff --git a/codex-rs/windows-sandbox-rs/src/identity.rs b/codex-rs/windows-sandbox-rs/src/identity.rs index 4d5b18556d..14adc5d3ff 100644 --- a/codex-rs/windows-sandbox-rs/src/identity.rs +++ b/codex-rs/windows-sandbox-rs/src/identity.rs @@ -61,6 +61,11 @@ pub fn sandbox_setup_is_complete(codex_home: &Path) -> bool { if !marker_ok { return false; } + if crate::registered_core_requested() + && !crate::app_package::registered_setup_is_ready(codex_home).unwrap_or(false) + { + return false; + } matches!(load_users(codex_home), Ok(Some(users)) if users.version_matches()) } @@ -239,9 +244,12 @@ pub fn require_logon_sandbox_creds( proxy_enforced: bool, proxy_settings_mode: crate::WindowsSandboxProxySettingsMode, ) -> Result { + let runtime = crate::setup::current_setup_runtime(); let needed_read = read_roots_override .map(<[PathBuf]>::to_vec) - .unwrap_or_else(|| gather_read_roots(command_cwd, permissions, env_map, codex_home)); + .unwrap_or_else(|| { + gather_read_roots(command_cwd, permissions, env_map, codex_home, runtime) + }); let needed_write = write_roots_override .map(<[PathBuf]>::to_vec) .unwrap_or_else(|| gather_write_roots_for_permissions(permissions, command_cwd, env_map)); 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 06f1340cbb..96cc83203f 100644 --- a/codex-rs/windows-sandbox-rs/src/identity_integration_tests.rs +++ b/codex-rs/windows-sandbox-rs/src/identity_integration_tests.rs @@ -33,6 +33,7 @@ fn credential_setup_reconciles_effective_firewall_policy() -> Result<()> { (true, true, "", 0), (true, true, "8080,3129", 0), (false, false, "8080", 1), + (false, false, "", 1), (false, true, "3128", 1), (true, false, "3128", 1), ] { @@ -73,33 +74,39 @@ fn credential_setup_reconciles_effective_firewall_policy() -> Result<()> { u8::from(desired_binding).to_string(), ), ]); - 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, - |_, _| { - full_setups.set(full_setups.get() + 1); - Err(anyhow::anyhow!("full setup intercepted")) - }, - |_| Ok(Some(UF_NORMAL_ACCOUNT)), - ); + let prepare = || { + require_sandbox_account_with_setup( + &SandboxSetupRequest { + permissions: &permissions, + command_cwd: home.path(), + env_map: &env, + codex_home: home.path(), + proxy_enforced: true, + }, + WindowsSandboxProxySettingsMode::Reconcile, + |_, desired| { + full_setups.set(full_setups.get() + 1); + let mut reconciled = marker.clone(); + reconciled.proxy_ports = desired.proxy_ports.clone(); + reconciled.allow_local_binding = desired.allow_local_binding; + fs::write( + setup_marker_path(home.path()), + serde_json::to_vec(&reconciled)?, + )?; + Ok(()) + }, + |_| Ok(Some(UF_NORMAL_ACCOUNT)), + ) + }; + for _ in 0..2 { + let (creds, _) = prepare()?; + assert_eq!( + (creds.username, creds.password), + ("offline".into(), "test-password".into()) + ); + } + // The second command reuses reconciled settings, including when filtering removed the proxy. assert_eq!(full_setups.get(), expected_full_setups); - assert_eq!( - result - .map(|(creds, _)| (creds.username, creds.password)) - .map_err(|err| err.to_string()), - 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); } Ok(()) } diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index 1770c68252..4393e9e191 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -55,6 +55,14 @@ mod acl; #[cfg(target_os = "windows")] mod allow; #[cfg(target_os = "windows")] +mod app_package; +#[cfg(target_os = "windows")] +#[doc(hidden)] +pub use app_package::registered_core_needs_refresh; +#[cfg(target_os = "windows")] +#[doc(hidden)] +pub use app_package::registered_core_requested; +#[cfg(target_os = "windows")] mod audit; #[cfg(target_os = "windows")] mod cap; @@ -265,8 +273,6 @@ pub use elevated_impl::run_windows_sandbox_capture_for_permission_profile as run #[cfg(target_os = "windows")] pub use file_write::write_file_atomically; #[cfg(target_os = "windows")] -pub use helper_materialization::resolve_current_exe_for_launch; -#[cfg(target_os = "windows")] pub use helper_materialization::resolve_exe_for_launch; #[cfg(target_os = "windows")] pub use hide_users::hide_current_user_profile_dir; @@ -353,6 +359,9 @@ pub use provisioning_client::WindowsSandboxProvisioningOutcome; #[cfg(target_os = "windows")] pub use provisioning_client::provision_windows_sandbox_via_service; #[cfg(target_os = "windows")] +#[doc(hidden)] +pub use provisioning_client::refresh_registered_core_via_service; +#[cfg(target_os = "windows")] pub use provisioning_client::register_desktop_installation; #[cfg(target_os = "windows")] pub use provisioning_protocol::FramedProvisioningMessage; diff --git a/codex-rs/windows-sandbox-rs/src/proc_thread_attr.rs b/codex-rs/windows-sandbox-rs/src/proc_thread_attr.rs index 38110a6923..12c9b3c864 100644 --- a/codex-rs/windows-sandbox-rs/src/proc_thread_attr.rs +++ b/codex-rs/windows-sandbox-rs/src/proc_thread_attr.rs @@ -5,7 +5,10 @@ use windows_sys::Win32::Foundation::HANDLE; use windows_sys::Win32::System::Threading::DeleteProcThreadAttributeList; use windows_sys::Win32::System::Threading::InitializeProcThreadAttributeList; use windows_sys::Win32::System::Threading::LPPROC_THREAD_ATTRIBUTE_LIST; +use windows_sys::Win32::System::Threading::PROC_THREAD_ATTRIBUTE_DESKTOP_APP_POLICY; use windows_sys::Win32::System::Threading::UpdateProcThreadAttribute; +use windows_sys::Win32::System::WindowsProgramming::PROCESS_CREATION_DESKTOP_APP_BREAKAWAY_DISABLE_PROCESS_TREE; +use windows_sys::Win32::System::WindowsProgramming::PROCESS_CREATION_DESKTOP_APP_BREAKAWAY_OVERRIDE; const PROC_THREAD_ATTRIBUTE_HANDLE_LIST: usize = 0x0002_0002; const PROC_THREAD_ATTRIBUTE_JOB_LIST: usize = 0x0002_000D; @@ -15,6 +18,7 @@ pub struct ProcThreadAttributeList { buffer: Vec, handle_list: Vec, job_list: Vec, + desktop_app_policy: Option>, } impl ProcThreadAttributeList { @@ -40,6 +44,7 @@ impl ProcThreadAttributeList { buffer, handle_list: Vec::new(), job_list: Vec::new(), + desktop_app_policy: None, }) } @@ -80,6 +85,28 @@ impl ProcThreadAttributeList { unsafe { self.update(PROC_THREAD_ATTRIBUTE_JOB_LIST, value, size) } } + pub fn preserve_desktop_app_context(&mut self) -> io::Result<()> { + // System shells otherwise leave the package environment and cannot launch + // children from its protected directory. Keep the initial child and descendants + // inside it, using the caller-provided restricted token and job containment. + // Compare compatibility outcomes via codex.windows_sandbox.runner_result. + // https://learn.microsoft.com/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute + let policy = self.desktop_app_policy.insert(Box::new( + PROCESS_CREATION_DESKTOP_APP_BREAKAWAY_DISABLE_PROCESS_TREE + | PROCESS_CREATION_DESKTOP_APP_BREAKAWAY_OVERRIDE, + )); + let value = std::ptr::from_mut(policy.as_mut()).cast(); + // SAFETY: the boxed DWORD remains at a stable address through process creation, + // even if the attribute list moves. It is released after DeleteProcThreadAttributeList. + unsafe { + self.update( + PROC_THREAD_ATTRIBUTE_DESKTOP_APP_POLICY as usize, + value, + std::mem::size_of::(), + ) + } + } + unsafe fn update( &mut self, attribute: usize, diff --git a/codex-rs/windows-sandbox-rs/src/process.rs b/codex-rs/windows-sandbox-rs/src/process.rs index aa87ddba49..a6d088ef41 100644 --- a/codex-rs/windows-sandbox-rs/src/process.rs +++ b/codex-rs/windows-sandbox-rs/src/process.rs @@ -112,9 +112,13 @@ pub unsafe fn create_process_as_user( | (None, ConsoleMode::Inherit) | (None, ConsoleMode::NoWindow) => 0, }; - let attr_count = if stdio.is_some() { 2 } else { 1 }; + let preserve_app_context = crate::app_package::current_process_is_registered_core_runner()?; + let attr_count = if stdio.is_some() { 2 } else { 1 } + u32::from(preserve_app_context); let mut attrs = ProcThreadAttributeList::new(attr_count)?; attrs.set_job(job.as_raw_handle() as HANDLE)?; + if preserve_app_context { + attrs.preserve_desktop_app_context()?; + } let mut si: STARTUPINFOEXW = std::mem::zeroed(); si.StartupInfo.cb = std::mem::size_of::() as u32; diff --git a/codex-rs/windows-sandbox-rs/src/provisioning_client.rs b/codex-rs/windows-sandbox-rs/src/provisioning_client.rs index 98423b45a0..c602092850 100644 --- a/codex-rs/windows-sandbox-rs/src/provisioning_client.rs +++ b/codex-rs/windows-sandbox-rs/src/provisioning_client.rs @@ -64,6 +64,10 @@ impl WindowsSandboxProxyListeners { if permission_profile.network_sandbox_policy().is_enabled() { return Self::default(); } + Self::from_proxy_environment(env_map) + } + + pub(crate) fn from_proxy_environment(env_map: &HashMap) -> Self { let mut listeners = Self::default(); for value in crate::setup::PROXY_ENV_KEYS .iter() @@ -100,11 +104,44 @@ pub enum WindowsSandboxProvisioningOutcome { Unavailable, } +enum ProvisioningIntent { + Setup, + RefreshRegistration, +} + +/// Refreshes only an already-provisioned gated runtime; no helper or setup fallback. +#[doc(hidden)] +pub fn refresh_registered_core_via_service( + codex_home: &Path, + settings: WindowsSandboxProvisioningSettings, + listeners: WindowsSandboxProxyListeners, +) -> anyhow::Result { + anyhow::ensure!( + crate::registered_core_requested(), + "registered Core is not enabled" + ); + provision( + codex_home, + settings, + listeners, + ProvisioningIntent::RefreshRegistration, + ) +} + /// Provisions the elevated Windows sandbox through the authenticated packaged service. pub fn provision_windows_sandbox_via_service( codex_home: &Path, settings: WindowsSandboxProvisioningSettings, listeners: WindowsSandboxProxyListeners, +) -> anyhow::Result { + provision(codex_home, settings, listeners, ProvisioningIntent::Setup) +} + +fn provision( + codex_home: &Path, + settings: WindowsSandboxProvisioningSettings, + listeners: WindowsSandboxProxyListeners, + intent: ProvisioningIntent, ) -> anyhow::Result { let request = crate::FramedProvisioningMessage { version: crate::PROVISIONING_PROTOCOL_VERSION, @@ -114,8 +151,8 @@ pub fn provision_windows_sandbox_via_service( .to_str() .context("sandbox provisioning home is not valid UTF-8")? .to_owned(), - registered_core: false, - refresh_only: false, + registered_core: crate::registered_core_requested(), + refresh_only: matches!(intent, ProvisioningIntent::RefreshRegistration), settings, listeners, }, @@ -126,13 +163,18 @@ pub fn provision_windows_sandbox_via_service( crate::SandboxProvisioningResponse::Ok => { Ok(WindowsSandboxProvisioningOutcome::Provisioned) } - crate::SandboxProvisioningResponse::Unavailable => { - Ok(WindowsSandboxProvisioningOutcome::Unavailable) - } + crate::SandboxProvisioningResponse::Unavailable => service_unavailable(), crate::SandboxProvisioningResponse::Error { message } => Err(anyhow!(message)), } } +fn service_unavailable() -> anyhow::Result { + if crate::registered_core_requested() { + bail!("app runtime provisioning service is unavailable; refusing helper fallback"); + } + Ok(WindowsSandboxProvisioningOutcome::Unavailable) +} + /// Records desktop uninstall ownership without creating or enabling a sandbox. pub fn register_desktop_installation(codex_home: &Path) -> anyhow::Result<()> { let request = crate::FramedProvisioningMessage { diff --git a/codex-rs/windows-sandbox-rs/src/runtime_ownership.rs b/codex-rs/windows-sandbox-rs/src/runtime_ownership.rs index 9cd6fb7945..7cba7bfd4d 100644 --- a/codex-rs/windows-sandbox-rs/src/runtime_ownership.rs +++ b/codex-rs/windows-sandbox-rs/src/runtime_ownership.rs @@ -129,6 +129,28 @@ pub struct RuntimeAccountRegistration { pub alias_path: Option, } +/// Capture the requesting process identity before elevation, never USERNAME. +pub(crate) fn current_setup_user() -> Result { + use std::os::windows::io::FromRawHandle; + use std::os::windows::io::OwnedHandle; + use windows_sys::Win32::Security as security; + use windows_sys::Win32::System::Threading as threading; + let mut token = 0; + if unsafe { + threading::OpenProcessToken( + threading::GetCurrentProcess(), + security::TOKEN_QUERY, + &mut token, + ) + } == 0 + { + return Err(io::Error::last_os_error()).context("query requesting setup identity"); + } + let _token = unsafe { OwnedHandle::from_raw_handle(token as _) }; + let sid = unsafe { crate::token::get_user_sid_bytes(token)? }; + unsafe { crate::winutil::account_name_from_sid(sid.as_ptr() as _) } + .context("resolve requesting setup identity") +} /// Core owns its complete record; the old parent is only a fallback before/after Core. pub fn load_installation() -> Result> { if let Some(record) = crate::installation_record::load_from(CORE_INSTALLATION_KEY)? { diff --git a/codex-rs/windows-sandbox-rs/src/setup.rs b/codex-rs/windows-sandbox-rs/src/setup.rs index 12ae351177..ad9ed0dcd3 100644 --- a/codex-rs/windows-sandbox-rs/src/setup.rs +++ b/codex-rs/windows-sandbox-rs/src/setup.rs @@ -18,6 +18,7 @@ use std::sync::OnceLock; use crate::allow::AllowDenyPaths; use crate::allow::compute_allow_paths_for_permissions; +use crate::app_package::registered_core_requested; use crate::deny_read_resolver::resolve_windows_deny_read_paths; use crate::helper_materialization::bundled_executable_path_for_exe; use crate::helper_materialization::helper_bin_dir; @@ -252,6 +253,7 @@ pub fn run_setup_refresh( ..SetupRootOverrides::default() }, /*offline_proxy_settings_override*/ None, + current_setup_runtime(), ) } @@ -260,7 +262,12 @@ pub(crate) fn run_setup_refresh_with_overrides_and_proxy_settings( overrides: SetupRootOverrides, offline_proxy_settings: &OfflineProxySettings, ) -> Result<()> { - run_setup_refresh_inner(request, overrides, Some(offline_proxy_settings)) + run_setup_refresh_inner( + request, + overrides, + Some(offline_proxy_settings), + current_setup_runtime(), + ) } pub fn run_setup_refresh_with_extra_read_roots( @@ -283,7 +290,8 @@ pub fn run_setup_refresh_with_extra_read_roots( permissions.validate_elevated_filesystem_policy(command_cwd)?; let deny_read_paths = setup_refresh_deny_read_paths(permission_profile, workspace_roots, command_cwd)?; - let mut read_roots = gather_read_roots(command_cwd, &permissions, env_map, codex_home); + let runtime = current_setup_runtime(); + let mut read_roots = gather_read_roots(command_cwd, &permissions, env_map, codex_home, runtime); read_roots.extend(extra_read_roots); run_setup_refresh_inner( SandboxSetupRequest { @@ -301,6 +309,7 @@ pub fn run_setup_refresh_with_extra_read_roots( deny_write_paths: None, }, /*offline_proxy_settings_override*/ None, + runtime, ) } @@ -327,11 +336,12 @@ fn run_setup_refresh_inner( request: SandboxSetupRequest<'_>, overrides: SetupRootOverrides, offline_proxy_settings_override: Option<&OfflineProxySettings>, + runtime: SetupRuntime, ) -> Result<()> { request .permissions .validate_elevated_filesystem_policy(request.command_cwd)?; - let (read_roots, write_roots) = build_payload_roots(&request, &overrides); + let (read_roots, write_roots) = build_payload_roots(&request, &overrides, runtime); let deny_read_paths = build_payload_deny_read_paths(overrides.deny_read_paths); let deny_write_paths = build_payload_deny_write_paths(&request, overrides.deny_write_paths); let offline_proxy_settings = @@ -349,9 +359,9 @@ fn run_setup_refresh_inner( proxy_ports: offline_proxy_settings.proxy_ports, allow_local_binding: offline_proxy_settings.allow_local_binding, otel: None, - real_user: std::env::var("USERNAME").unwrap_or_else(|_| "Administrators".to_string()), + real_user: crate::runtime_ownership::current_setup_user()?, mode: SetupMode::Full, - runtime: SetupRuntime::Legacy, + runtime, refresh_only: true, }; let json = serde_json::to_vec(&payload)?; @@ -554,7 +564,18 @@ fn profile_read_roots(user_profile: &Path) -> Vec { .collect() } -fn gather_helper_read_roots(codex_home: &Path) -> Vec { +pub(crate) fn current_setup_runtime() -> SetupRuntime { + if registered_core_requested() { + SetupRuntime::Registered + } else { + SetupRuntime::Legacy + } +} + +fn gather_helper_read_roots(codex_home: &Path, runtime: SetupRuntime) -> Vec { + if runtime == SetupRuntime::Registered { + return Vec::new(); + } let helper_dir = helper_bin_dir(codex_home); let _ = std::fs::create_dir_all(&helper_dir); vec![helper_dir] @@ -565,8 +586,9 @@ fn gather_full_read_roots_for_permissions( permissions: &ResolvedWindowsSandboxPermissions, env_map: &HashMap, codex_home: &Path, + runtime: SetupRuntime, ) -> Vec { - let mut roots = gather_helper_read_roots(codex_home); + let mut roots = gather_helper_read_roots(codex_home, runtime); roots.extend( WINDOWS_PLATFORM_DEFAULT_READ_ROOTS .iter() @@ -596,6 +618,7 @@ pub(crate) fn gather_read_roots( permissions: &ResolvedWindowsSandboxPermissions, env_map: &HashMap, codex_home: &Path, + runtime: SetupRuntime, ) -> Vec { if permissions.has_symbolic_root_read_access(command_cwd) { return gather_full_read_roots_for_permissions( @@ -603,10 +626,11 @@ pub(crate) fn gather_read_roots( permissions, env_map, codex_home, + runtime, ); } - let mut roots = gather_helper_read_roots(codex_home); + let mut roots = gather_helper_read_roots(codex_home, runtime); if permissions.include_platform_defaults() { roots.extend( WINDOWS_PLATFORM_DEFAULT_READ_ROOTS @@ -962,11 +986,10 @@ fn run_setup_exe_payload( .stderr(Stdio::null()) .status() } else { - crate::setup_launch::spawn_with_retained_handles( - Command::new(&exe).arg(payload_b64), - retained_handles, - ) - .and_then(|mut child| child.wait()) + let mut command = Command::new(&exe); + command.arg(payload_b64); + crate::setup_launch::spawn_with_retained_handles(&mut command, retained_handles) + .and_then(|mut child| child.wait()) } .map_err(|err| { failure( @@ -1051,6 +1074,35 @@ pub(crate) fn run_elevated_setup_with_proxy_settings( request: SandboxSetupRequest<'_>, offline_proxy_settings: &OfflineProxySettings, ) -> Result<()> { + if registered_core_requested() { + request + .permissions + .validate_elevated_filesystem_policy(request.command_cwd)?; + // Reconcile the effective command settings through the same authenticated service as setup. + // A filtered shell environment may differ from the app's startup environment. + let settings = crate::WindowsSandboxProvisioningSettings { + proxy_ports: offline_proxy_settings.proxy_ports.clone(), + allow_local_binding: offline_proxy_settings.allow_local_binding, + }; + let mut listeners = + crate::WindowsSandboxProxyListeners::from_proxy_environment(request.env_map); + listeners + .http_ports + .retain(|port| settings.proxy_ports.contains(port)); + listeners + .socks_ports + .retain(|port| settings.proxy_ports.contains(port)); + let outcome = crate::provisioning_client::provision_windows_sandbox_via_service( + request.codex_home, + settings, + listeners, + )?; + anyhow::ensure!( + outcome == crate::WindowsSandboxProvisioningOutcome::Provisioned, + "registered Core reconciliation requires the sandbox service" + ); + return Ok(()); + } run_elevated_setup_inner(request, Some(offline_proxy_settings)) } @@ -1058,6 +1110,10 @@ fn run_elevated_setup_inner( request: SandboxSetupRequest<'_>, offline_proxy_settings_override: Option<&OfflineProxySettings>, ) -> Result<()> { + anyhow::ensure!( + !registered_core_requested(), + "registered Core requires the sandbox service; retry service setup" + ); request .permissions .validate_elevated_filesystem_policy(request.command_cwd)?; @@ -1069,7 +1125,11 @@ fn run_elevated_setup_inner( format!("failed to create sandbox dir {}: {err}", sbx_dir.display()), ) })?; - let payload = elevated_provisioning_payload(&request, offline_proxy_settings_override); + let payload = elevated_provisioning_payload( + &request, + offline_proxy_settings_override, + crate::runtime_ownership::current_setup_user()?, + ); let needs_elevation = !is_elevated().map_err(|err| { failure( SetupErrorCode::OrchestratorElevationCheckFailed, @@ -1082,6 +1142,7 @@ fn run_elevated_setup_inner( fn elevated_provisioning_payload( request: &SandboxSetupRequest<'_>, offline_proxy_settings_override: Option<&OfflineProxySettings>, + real_user: String, ) -> ElevationPayload { let offline_proxy_settings = offline_proxy_settings_for_request(request, offline_proxy_settings_override); @@ -1097,7 +1158,7 @@ fn elevated_provisioning_payload( deny_write_paths: Vec::new(), proxy_ports: offline_proxy_settings.proxy_ports, allow_local_binding: offline_proxy_settings.allow_local_binding, - real_user: std::env::var("USERNAME").unwrap_or_else(|_| "Administrators".to_string()), + real_user, otel: codex_otel::global_statsig_metrics_settings(), mode: SetupMode::InteractiveProvision, runtime: SetupRuntime::Legacy, @@ -1110,15 +1171,23 @@ pub fn run_elevated_provisioning_setup( real_user: &str, settings: crate::WindowsSandboxProvisioningSettings, ) -> Result<()> { - run_elevated_provisioning_setup_with_retained_handles(codex_home, real_user, settings, &[]) + run_elevated_provisioning_setup_with_retained_handles( + codex_home, + real_user, + settings, + current_setup_runtime(), + &[], + ) } /// Runs service provisioning with directory protections retained by the helper /// itself, so they survive an unexpected exit of the provisioning service. +/// The runtime must describe the authenticated client, not the shared service image. pub fn run_elevated_provisioning_setup_with_retained_handles( codex_home: &Path, real_user: &str, settings: crate::WindowsSandboxProvisioningSettings, + runtime: SetupRuntime, retained_handles: &[BorrowedHandle<'_>], ) -> Result<()> { if !codex_home.is_absolute() @@ -1172,7 +1241,7 @@ pub fn run_elevated_provisioning_setup_with_retained_handles( otel: codex_otel::global_statsig_metrics_settings(), real_user: real_user.to_string(), mode: SetupMode::ProvisionOnly, - runtime: SetupRuntime::Legacy, + runtime, refresh_only: false, }; run_setup_exe( @@ -1186,6 +1255,7 @@ pub fn run_elevated_provisioning_setup_with_retained_handles( pub(crate) fn build_payload_roots( request: &SandboxSetupRequest<'_>, overrides: &SetupRootOverrides, + runtime: SetupRuntime, ) -> (Vec, Vec) { let write_roots = effective_write_roots_for_setup( request.permissions, @@ -1197,7 +1267,7 @@ pub(crate) fn build_payload_roots( let mut read_roots = if let Some(roots) = overrides.read_roots.as_deref() { // An explicit override is the split policy's complete readable set. Keep only the // helper/platform roots the elevated setup needs; do not re-add legacy cwd/full-read roots. - let mut read_roots = gather_helper_read_roots(request.codex_home); + let mut read_roots = gather_helper_read_roots(request.codex_home, runtime); if overrides.read_roots_include_platform_defaults { read_roots.extend( WINDOWS_PLATFORM_DEFAULT_READ_ROOTS @@ -1213,6 +1283,7 @@ pub(crate) fn build_payload_roots( request.permissions, request.env_map, request.codex_home, + runtime, ) }; read_roots = expand_user_profile_root(read_roots); @@ -1605,9 +1676,13 @@ mod tests { }; let payload = super::elevated_provisioning_payload( - &request, /*offline_proxy_settings_override*/ None, + &request, + /*offline_proxy_settings_override*/ None, + r"DOMAIN\alice".to_string(), ); + assert!(matches!(payload.runtime, super::SetupRuntime::Legacy)); + assert_eq!(payload.real_user, r"DOMAIN\alice"); assert_eq!(payload.command_cwd, codex_home); assert_eq!(payload.read_roots, Vec::::new()); assert_eq!(payload.write_roots, Vec::::new()); @@ -2302,13 +2377,67 @@ mod tests { let workspace_roots = workspace_roots_for(command_cwd.as_path()); let permissions = permissions_for(&permission_profile, workspace_roots.as_slice()); - let roots = gather_read_roots(&command_cwd, &permissions, &HashMap::new(), &codex_home); + let roots = gather_read_roots( + &command_cwd, + &permissions, + &HashMap::new(), + &codex_home, + super::SetupRuntime::Legacy, + ); let expected = dunce::canonicalize(helper_bin_dir(&codex_home)).expect("canonical helper dir"); assert!(roots.contains(&expected)); } + #[test] + fn helper_read_roots_only_create_a_legacy_bin() { + let tmp = TempDir::new().expect("tempdir"); + for runtime in [super::SetupRuntime::Registered, super::SetupRuntime::Legacy] { + let home = tmp.path().join(format!("{runtime:?}")); + let roots = super::gather_helper_read_roots(&home, runtime); + match runtime { + super::SetupRuntime::Registered => { + assert_eq!(roots, Vec::::new()); + assert!(!home.exists()); + } + super::SetupRuntime::Legacy => { + assert_eq!(roots, vec![helper_bin_dir(&home)]); + assert!(helper_bin_dir(&home).is_dir()); + } + } + } + } + + #[test] + fn registered_payload_preserves_explicit_bin_read_root() { + let tmp = TempDir::new().expect("tempdir"); + let home = tmp.path().join("home"); + let bin = helper_bin_dir(&home); + fs::create_dir_all(&bin).expect("create explicit root"); + let workspace_roots = workspace_roots_for(tmp.path()); + let permissions = permissions_for(&PermissionProfile::read_only(), &workspace_roots); + let (read_roots, write_roots) = build_payload_roots( + &super::SandboxSetupRequest { + permissions: &permissions, + command_cwd: tmp.path(), + env_map: &HashMap::new(), + codex_home: &home, + proxy_enforced: false, + }, + &super::SetupRootOverrides { + read_roots: Some(vec![bin.clone()]), + ..super::SetupRootOverrides::default() + }, + super::SetupRuntime::Registered, + ); + assert_eq!( + read_roots, + vec![dunce::canonicalize(bin).expect("canonical bin")] + ); + assert_eq!(write_roots, Vec::::new()); + } + #[test] fn workspace_write_roots_remain_readable() { let tmp = TempDir::new().expect("tempdir"); @@ -2328,7 +2457,13 @@ mod tests { let workspace_roots = workspace_roots_for(command_cwd.as_path()); let permissions = permissions_for(&permission_profile, workspace_roots.as_slice()); - let roots = gather_read_roots(&command_cwd, &permissions, &HashMap::new(), &codex_home); + let roots = gather_read_roots( + &command_cwd, + &permissions, + &HashMap::new(), + &codex_home, + super::SetupRuntime::Legacy, + ); let expected_writable = dunce::canonicalize(&writable_root).expect("canonical writable root"); @@ -2364,6 +2499,7 @@ mod tests { deny_read_paths: None, deny_write_paths: None, }, + super::SetupRuntime::Legacy, ); let expected_helper = dunce::canonicalize(helper_bin_dir(&codex_home)).expect("canonical helper dir"); @@ -2411,6 +2547,7 @@ mod tests { deny_read_paths: None, deny_write_paths: None, }, + super::SetupRuntime::Legacy, ); let expected_helper = dunce::canonicalize(helper_bin_dir(&codex_home)).expect("canonical helper dir"); @@ -2475,7 +2612,8 @@ mod tests { &codex_home, Some(&override_roots), ); - let (_read_roots, payload_write_roots) = build_payload_roots(&request, &overrides); + let (_read_roots, payload_write_roots) = + build_payload_roots(&request, &overrides, super::SetupRuntime::Legacy); let expected_workspace = dunce::canonicalize(&command_cwd).expect("canonical workspace"); let expected_extra = dunce::canonicalize(&extra_root).expect("canonical extra root"); @@ -2578,6 +2716,7 @@ mod tests { &permissions, &HashMap::new(), &codex_home, + super::SetupRuntime::Legacy, ); assert!( diff --git a/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs b/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs index 9d2e1a9bc7..6b003f09d6 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs @@ -542,7 +542,7 @@ fn real_main(setup_mode: &mut Option) -> Result<()> { let _setup_guard = if payload.mode.provisions_accounts(payload.refresh_only) { let guard = acquire_sandbox_setup_lock(INFINITE)?; anyhow::ensure!( - payload.runtime == SetupRuntime::Legacy, + payload.runtime == SetupRuntime::Legacy && !crate::registered_core_requested(), "registered Core requires service-owned provisioning" ); anyhow::ensure!( diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/windows_common.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/windows_common.rs index f37829e950..05a8778589 100644 --- a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/windows_common.rs +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/windows_common.rs @@ -84,7 +84,7 @@ pub(crate) fn start_runner_stdout_reader( exit_tx: oneshot::Sender, ) { std::thread::spawn(move || { - loop { + let exit = loop { let msg = match crate::ipc_framed::read_frame(&mut pipe_read) { Ok(Some(v)) => v, Ok(None) => { @@ -93,8 +93,7 @@ pub(crate) fn start_runner_stdout_reader( &stdout_tx, stderr_tx.as_ref(), ); - let _ = exit_tx.send(-1); - break; + break None; } Err(err) => { send_runner_error( @@ -102,8 +101,7 @@ pub(crate) fn start_runner_stdout_reader( &stdout_tx, stderr_tx.as_ref(), ); - let _ = exit_tx.send(-1); - break; + break None; } }; @@ -125,13 +123,11 @@ pub(crate) fn start_runner_stdout_reader( } } Message::Exit { payload } => { - let _ = exit_tx.send(payload.exit_code); - break; + break Some((payload.exit_code, payload.timed_out)); } Message::Error { payload } => { send_runner_error(&payload.message, &stdout_tx, stderr_tx.as_ref()); - let _ = exit_tx.send(-1); - break; + break None; } Message::SpawnReady { .. } | Message::Stdin { .. } @@ -140,7 +136,9 @@ pub(crate) fn start_runner_stdout_reader( | Message::SpawnRequest { .. } | Message::Terminate { .. } => {} } - } + }; + crate::elevated::runner_metrics::record_command(exit); + let _ = exit_tx.send(exit.map_or(-1, |(code, _)| code)); }); } @@ -174,3 +172,7 @@ fn send_runner_error( let _ = stdout_tx.send(formatted); } } + +#[cfg(test)] +#[path = "windows_common_tests.rs"] +mod tests; diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/windows_common_tests.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/windows_common_tests.rs new file mode 100644 index 0000000000..1a957e1e52 --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/windows_common_tests.rs @@ -0,0 +1,34 @@ +//! The result-metric hook must preserve terminal exit codes and pipe-failure behavior. + +use super::*; +use crate::ipc_framed::ExitPayload; +use crate::ipc_framed::write_frame; +use pretty_assertions::assert_eq; +use std::io::Seek; + +#[test] +fn runner_result_reporting_preserves_exit_and_closed_pipe_results() -> Result<()> { + for exit_code in [Some(0), Some(23), None] { + let mut frames = tempfile::tempfile()?; + if let Some(exit_code) = exit_code { + write_frame( + &mut frames, + &FramedMessage { + version: IPC_PROTOCOL_VERSION, + message: Message::Exit { + payload: ExitPayload { + exit_code, + timed_out: false, + }, + }, + }, + )?; + } + frames.rewind()?; + let (stdout_tx, _stdout_rx) = broadcast::channel(1); + let (exit_tx, exit_rx) = oneshot::channel(); + start_runner_stdout_reader(frames, stdout_tx, /*stderr_tx*/ None, exit_tx); + assert_eq!(exit_rx.blocking_recv()?, exit_code.unwrap_or(-1)); + } + Ok(()) +} diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs index f8432993f5..296b81b0e4 100644 --- a/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs @@ -346,6 +346,61 @@ fn elevated_non_tty_cmd_forwards_env_output_and_exit() { }); } +#[test] +#[ignore = "requires this test binary in an installed test MSIX, launched with package identity, CODEX_WINDOWS_REGISTERED_CORE=1, and CODEX_HOME provisioned by that package's service in a disposable Windows VM"] +fn registered_non_tty_cmd_forwards_env_output_and_exit() { + assert!( + crate::registered_core_requested(), + "registered Core opt-in is required" + ); + let codex_home = PathBuf::from(std::env::var_os("CODEX_HOME").expect("fixture CODEX_HOME")); + assert!( + crate::app_package::registered_setup_is_ready(&codex_home) + .expect("validate the installed package and service receipt"), + "the test process must have package identity and completed service setup", + ); + let cwd = tempfile::tempdir().expect("isolated command workspace"); + current_thread_runtime().block_on(async { + let mut env_map: HashMap = std::env::vars().collect(); + env_map.insert("CODEX_REGISTERED_TEST".into(), "REGISTERED-ENV-OK".into()); + let spawned = spawn_windows_sandbox_session_elevated_for_permission_profile( + &PermissionProfile::workspace_write(), + workspace_roots_for(cwd.path()).as_slice(), + &codex_home, + vec![ + "C:\\Windows\\System32\\cmd.exe".into(), + "/d".into(), + "/c".into(), + "echo %CODEX_REGISTERED_TEST%& exit /b 23".into(), + ], + cwd.path(), + env_map, + /*proxy_enforced*/ false, + /*network_proxy_restricting_sid*/ None, + Some(5_000), + /*read_roots_override*/ None, + /*read_roots_include_platform_defaults*/ true, + /*write_roots_override*/ None, + &[], + &[], + /*tty*/ false, + /*stdin_open*/ false, + /*use_private_desktop*/ true, + ) + .await + .expect("launch through the service-recorded alias and authenticated pipes"); + let (stdout, exit_code) = + collect_stdout_and_exit(spawned, &codex_home, Duration::from_secs(10)).await; + assert_eq!( + ( + String::from_utf8(stdout).expect("command output"), + exit_code + ), + ("REGISTERED-ENV-OK\r\n".to_owned(), 23), + ); + }); +} + #[test] fn legacy_non_tty_cmd_rejects_deny_read_overrides() { let _guard = legacy_process_test_guard(); diff --git a/codex-rs/windows-sandbox-service/src/provisioning.rs b/codex-rs/windows-sandbox-service/src/provisioning.rs index 8adbf19401..cf91b51cde 100644 --- a/codex-rs/windows-sandbox-service/src/provisioning.rs +++ b/codex-rs/windows-sandbox-service/src/provisioning.rs @@ -122,6 +122,7 @@ pub(crate) fn run( &identity.codex_home, &identity.account, settings, + identity.runtime, &retained_handles, ) { Ok(()) => {