diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index d943182790..683cbbc6d7 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -414,8 +414,12 @@ pub use unified_exec::spawn_windows_sandbox_session_for_level; #[cfg(target_os = "windows")] pub use unified_exec::spawn_windows_sandbox_session_legacy; #[cfg(target_os = "windows")] +pub use uninstall_windows::PreparedWindowsSandboxCleanup; +#[cfg(target_os = "windows")] pub use uninstall_windows::clean_up_packaged_windows_sandbox; #[cfg(target_os = "windows")] +pub use uninstall_windows::prepare_packaged_windows_sandbox_cleanup; +#[cfg(target_os = "windows")] pub use wfp::install_wfp_filters_for_account; #[cfg(target_os = "windows")] pub use wfp_setup::install_wfp_filters; diff --git a/codex-rs/windows-sandbox-rs/src/uninstall_windows.rs b/codex-rs/windows-sandbox-rs/src/uninstall_windows.rs index ff5b7797c2..885227f624 100644 --- a/codex-rs/windows-sandbox-rs/src/uninstall_windows.rs +++ b/codex-rs/windows-sandbox-rs/src/uninstall_windows.rs @@ -1,4 +1,4 @@ -//! Stops sandbox work before removing its protections, then continues independent cleanup. +//! Stops sandbox work under the setup lock before removing its resources and protections. use std::path::Path; @@ -19,7 +19,30 @@ pub fn clean_up_packaged_windows_sandbox( codex_home: Option<&Path>, clean_up_desktop: impl FnOnce() -> Result<()>, ) -> Result<()> { - let _setup_lock = crate::setup_mutex::acquire_sandbox_setup_lock(/*timeout_ms*/ 5_000)?; + prepare_packaged_windows_sandbox_cleanup()?.finish(codex_home, clean_up_desktop) +} + +/// Holds the setup lock after sandbox accounts are disabled, their processes +/// have exited, and their logon tokens have been released. +/// +/// Keep this guard on the thread that acquired the setup lock. Callers must not +/// re-enable the accounts or create new sandbox logons before `finish`. +/// +/// Dropping the guard only releases the lock. It does not re-enable accounts, +/// remove protections, or undo work performed between the two phases. +#[must_use = "finish cleanup or leave the sandbox accounts disabled with their protections intact"] +pub struct PreparedWindowsSandboxCleanup { + _setup_lock: crate::setup_mutex::SandboxSetupLock, +} + +/// Disables sandbox accounts, stops their processes, and waits for their logon +/// tokens to be released. +/// +/// No resources or protections are removed here. On failure, restoration of the +/// original account flags is attempted before the setup lock is released; any +/// restoration failure is included in the returned error. +pub fn prepare_packaged_windows_sandbox_cleanup() -> Result { + let setup_lock = crate::setup_mutex::acquire_sandbox_setup_lock(/*timeout_ms*/ 5_000)?; let mut errors = Vec::new(); let mut users = principals::DisabledSandboxUsers::default(); if let Err(error) = users.disable().and_then(|()| processes::stop(&users)) { @@ -31,42 +54,62 @@ pub fn clean_up_packaged_windows_sandbox( return Err(anyhow!(errors.join("; "))); } - if let Some(codex_home) = codex_home { - for directory in [ - crate::setup::sandbox_dir(codex_home), - crate::setup::sandbox_secrets_dir(codex_home), - crate::setup::sandbox_bin_dir(codex_home), - ] { - match std::fs::remove_dir_all(&directory) { - Ok(()) => {} - Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} - Err(error) => { - errors.push(format!("remove {}: {error}", directory.display())); + Ok(PreparedWindowsSandboxCleanup { + _setup_lock: setup_lock, + }) +} + +impl PreparedWindowsSandboxCleanup { + /// Removes resources while retaining the setup lock and disabled accounts. + /// + /// Keep a supplied home and its ancestors pinned until `clean_up_desktop` + /// starts. Independent cleanup steps continue after an error, as in + /// `clean_up_packaged_windows_sandbox`. + pub fn finish( + self, + codex_home: Option<&Path>, + clean_up_desktop: impl FnOnce() -> Result<()>, + ) -> Result<()> { + let _prepared = self; + let mut errors = Vec::new(); + + if let Some(codex_home) = codex_home { + for directory in [ + crate::setup::sandbox_dir(codex_home), + crate::setup::sandbox_secrets_dir(codex_home), + crate::setup::sandbox_bin_dir(codex_home), + ] { + match std::fs::remove_dir_all(&directory) { + Ok(()) => {} + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + Err(error) => { + errors.push(format!("remove {}: {error}", directory.display())); + } } } } - } - if let Err(error) = clean_up_desktop() { - errors.push(format!("{error:#}")); - } - - for result in [ - crate::wfp::remove_wfp_filters(), - firewall::cleanup_firewall_rules(), - principals::remove_sandbox_principal("CodexSandboxUsers"), - crate::hide_users::unhide_sandbox_users(&[OFFLINE_USERNAME, ONLINE_USERNAME]), - // Keep accounts disabled and setup locked until shared cleanup and account deletion finish. - principals::remove_sandbox_principal(OFFLINE_USERNAME), - principals::remove_sandbox_principal(ONLINE_USERNAME), - ] { - if let Err(error) = result { + if let Err(error) = clean_up_desktop() { errors.push(format!("{error:#}")); } - } - if errors.is_empty() { - Ok(()) - } else { - Err(anyhow!(errors.join("; "))) + + for result in [ + crate::wfp::remove_wfp_filters(), + firewall::cleanup_firewall_rules(), + principals::remove_sandbox_principal("CodexSandboxUsers"), + crate::hide_users::unhide_sandbox_users(&[OFFLINE_USERNAME, ONLINE_USERNAME]), + // Keep accounts disabled and setup locked until shared cleanup and account deletion finish. + principals::remove_sandbox_principal(OFFLINE_USERNAME), + principals::remove_sandbox_principal(ONLINE_USERNAME), + ] { + if let Err(error) = result { + errors.push(format!("{error:#}")); + } + } + if errors.is_empty() { + Ok(()) + } else { + Err(anyhow!(errors.join("; "))) + } } } diff --git a/codex-rs/windows-sandbox-service/src/package_lifecycle.rs b/codex-rs/windows-sandbox-service/src/package_lifecycle.rs index be3b6b8b36..544567e3fa 100644 --- a/codex-rs/windows-sandbox-service/src/package_lifecycle.rs +++ b/codex-rs/windows-sandbox-service/src/package_lifecycle.rs @@ -10,7 +10,6 @@ use std::sync::atomic::Ordering; use anyhow::Context; use anyhow::Result; use anyhow::ensure; -use codex_windows_sandbox::clean_up_packaged_windows_sandbox; use codex_windows_sandbox::string_from_sid_bytes; use windows::ApplicationModel::Package; use windows::ApplicationModel::PackageCatalog; @@ -32,6 +31,8 @@ use crate::installation_record::DesktopInstallation; use crate::installation_record::InstallationRecord; use crate::ipc::OwnedHandle; +mod cleanup; + struct UserInstallation { codex_home: Option, directory_handles: Vec, @@ -187,61 +188,7 @@ impl PackageLifecycle { let installation = installation .as_mut() .context("the authenticated package installation was not recorded")?; - crate::service::log_information( - crate::service::EVENT_CLEANUP_STARTED, - "sandbox uninstall cleanup started", - ); - // A partial uninstall must not let a later install inherit stale directory ownership. - crate::installation_record::remove()?; - let codex_home = installation.codex_home.clone(); - clean_up_packaged_windows_sandbox(codex_home.as_deref(), || { - let Some(desktop) = &installation.desktop_installation else { - return Ok(()); - }; - // The marker is user-writable. It must never authorize deletion as LocalSystem. - with_owner_impersonation(installation.user_token.0, || { - let mut errors = Vec::new(); - let mut record_error = |result: io::Result<()>| { - if let Err(error) = result - && error.kind() != io::ErrorKind::NotFound - { - errors.push(error.to_string()); - } - }; - if let Some(home) = &codex_home - && desktop.created_codex_home - { - // Release the home itself so it can be deleted; keep its ancestors pinned. - installation.directory_handles.pop(); - record_error(std::fs::remove_dir_all(home)); - } - // The cache may have been created after provisioning. Pin it only for cleanup. - let mut cache_directory_handles = Vec::new(); - if desktop.cache_home.is_dir() { - match crate::ipc::pin_existing_ancestors( - &desktop.cache_home, - &mut cache_directory_handles, - ) { - Ok(()) => record_error(std::fs::remove_dir_all( - desktop.cache_home.join("codex-runtimes"), - )), - Err(error) => errors.push(error.to_string()), - } - } - ensure!( - errors.is_empty(), - "remove desktop directories: {}", - errors.join("; ") - ); - Ok(()) - }) - }) - .context("remove packaged Windows sandbox resources")?; - crate::service::log_information( - crate::service::EVENT_CLEANUP_FINISHED, - "sandbox uninstall cleanup finished", - ); - Ok(()) + cleanup::clean_up(installation) } } diff --git a/codex-rs/windows-sandbox-service/src/package_lifecycle/cleanup.rs b/codex-rs/windows-sandbox-service/src/package_lifecycle/cleanup.rs new file mode 100644 index 0000000000..d52e386de2 --- /dev/null +++ b/codex-rs/windows-sandbox-service/src/package_lifecycle/cleanup.rs @@ -0,0 +1,70 @@ +//! Removes one authenticated owner's sandbox resources during package uninstall. +//! Desktop paths are deleted only while impersonating the owner; native cleanup keeps its order. + +use std::io; + +use anyhow::Context; +use anyhow::Result; +use anyhow::ensure; +use codex_windows_sandbox::clean_up_packaged_windows_sandbox; + +use super::UserInstallation; +use super::with_owner_impersonation; + +pub(super) fn clean_up(installation: &mut UserInstallation) -> Result<()> { + crate::service::log_information( + crate::service::EVENT_CLEANUP_STARTED, + "sandbox uninstall cleanup started", + ); + // A partial uninstall must not let a later install inherit stale directory ownership. + crate::installation_record::remove()?; + let codex_home = installation.codex_home.clone(); + clean_up_packaged_windows_sandbox(codex_home.as_deref(), || { + let Some(desktop) = &installation.desktop_installation else { + return Ok(()); + }; + // The marker is user-writable. It must never authorize deletion as LocalSystem. + with_owner_impersonation(installation.user_token.0, || { + let mut errors = Vec::new(); + let mut record_error = |result: io::Result<()>| { + if let Err(error) = result + && error.kind() != io::ErrorKind::NotFound + { + errors.push(error.to_string()); + } + }; + if let Some(home) = &codex_home + && desktop.created_codex_home + { + // Release the home itself so it can be deleted; keep its ancestors pinned. + installation.directory_handles.pop(); + record_error(std::fs::remove_dir_all(home)); + } + // The cache may have been created after provisioning. Pin it only for cleanup. + let mut cache_directory_handles = Vec::new(); + if desktop.cache_home.is_dir() { + match crate::ipc::pin_existing_ancestors( + &desktop.cache_home, + &mut cache_directory_handles, + ) { + Ok(()) => record_error(std::fs::remove_dir_all( + desktop.cache_home.join("codex-runtimes"), + )), + Err(error) => errors.push(error.to_string()), + } + } + ensure!( + errors.is_empty(), + "remove desktop directories: {}", + errors.join("; ") + ); + Ok(()) + }) + }) + .context("remove packaged Windows sandbox resources")?; + crate::service::log_information( + crate::service::EVENT_CLEANUP_FINISHED, + "sandbox uninstall cleanup finished", + ); + Ok(()) +}