From b9775a0d57b09b43e5bab17a245ec053cc6b4426 Mon Sep 17 00:00:00 2001 From: chess Date: Fri, 16 Oct 2026 20:53:20 +0000 Subject: [PATCH] Honor home deletion preferences during Windows uninstall (#54708) ## What changed Read `desktop.uninstall_codex_home` from the owner's current `config.toml` before recursively deleting a desktop-created Codex home during Windows sandbox uninstall. - Preserve the home by default for ordinary installations; retain the deletion default for OEM preinstalls when no preference is set. - Let an explicit boolean preference override either default. Preserve the home when configuration is unreadable, malformed, or contains an invalid preference. - Capture OEM metadata before Windows removes it during uninstall, and revoke sandbox permissions on retained homes. ## Testing Add unit tests for missing preferences, explicit overrides, malformed or invalid configuration, and configuration read failures across both installation defaults. GitOrigin-RevId: 46afc2a7b0f4ecf7f447e46b515e50d0d278e4a8 --- codex-rs/Cargo.lock | 1 + codex-rs/windows-sandbox-service/Cargo.toml | 2 + codex-rs/windows-sandbox-service/src/lib.rs | 2 + .../src/package_lifecycle.rs | 23 +++++- .../src/package_lifecycle/cleanup.rs | 9 ++- .../src/uninstall_policy.rs | 28 +++++++ .../src/uninstall_policy_tests.rs | 74 +++++++++++++++++++ 7 files changed, 134 insertions(+), 5 deletions(-) create mode 100644 codex-rs/windows-sandbox-service/src/uninstall_policy.rs create mode 100644 codex-rs/windows-sandbox-service/src/uninstall_policy_tests.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index aa68d67eb6..df6766fda2 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -5398,6 +5398,7 @@ dependencies = [ "codex-windows-sandbox", "pretty_assertions", "serde_json", + "tempfile", "tokio", "toml 0.9.11+spec-1.1.0", "windows 0.58.0", diff --git a/codex-rs/windows-sandbox-service/Cargo.toml b/codex-rs/windows-sandbox-service/Cargo.toml index 381fec9888..b4652f858f 100644 --- a/codex-rs/windows-sandbox-service/Cargo.toml +++ b/codex-rs/windows-sandbox-service/Cargo.toml @@ -28,6 +28,7 @@ toml = { workspace = true } [dev-dependencies] pretty_assertions = { workspace = true } +tempfile = { workspace = true } [target.'cfg(windows)'.dependencies.windows] version = "0.58" @@ -35,6 +36,7 @@ features = [ "ApplicationModel", "Foundation_Collections", "Management_Deployment", + "Storage", "Win32_System_WinRT", ] diff --git a/codex-rs/windows-sandbox-service/src/lib.rs b/codex-rs/windows-sandbox-service/src/lib.rs index 4924e7acca..aef9ed77c2 100644 --- a/codex-rs/windows-sandbox-service/src/lib.rs +++ b/codex-rs/windows-sandbox-service/src/lib.rs @@ -17,6 +17,8 @@ mod provisioning; mod registered_runtime; #[cfg(windows)] mod service; +#[cfg(any(windows, test))] +mod uninstall_policy; #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum RunMode { diff --git a/codex-rs/windows-sandbox-service/src/package_lifecycle.rs b/codex-rs/windows-sandbox-service/src/package_lifecycle.rs index aa58215027..42e6adc066 100644 --- a/codex-rs/windows-sandbox-service/src/package_lifecycle.rs +++ b/codex-rs/windows-sandbox-service/src/package_lifecycle.rs @@ -56,6 +56,7 @@ struct UserInstallation { pub(crate) struct PackageLifecycle { package_name: HSTRING, + preinstalled: bool, uninstalling: Arc, installation: RefCell>, } @@ -64,8 +65,20 @@ impl PackageLifecycle { pub(crate) fn new(uninstalling: Arc) -> Result { unsafe { RoInitialize(RO_INIT_MULTITHREADED) } .context("initialize the sandbox service Windows Runtime apartment")?; + let package = Package::Current()?; + // Windows removes OEM metadata on uninstall; capture it while the package is installed. + let preinstalled = package + .InstalledLocation() + .and_then(|location| location.Path()) + .is_ok_and(|path| { + PathBuf::from(path.to_os_string()) + .join("microsoft.system.package.metadata") + .join("Custom.data") + .is_file() + }); Ok(Self { - package_name: Package::Current()?.Id()?.FullName()?, + package_name: package.Id()?.FullName()?, + preinstalled, uninstalling, installation: RefCell::default(), }) @@ -265,7 +278,13 @@ impl PackageLifecycle { let installation = installation .as_mut() .context("the authenticated package installation was not recorded")?; - cleanup::clean_up(installation, prepared, runtime, &self.uninstalling) + cleanup::clean_up( + installation, + prepared, + runtime, + &self.uninstalling, + /*preinstalled*/ self.preinstalled, + ) } } diff --git a/codex-rs/windows-sandbox-service/src/package_lifecycle/cleanup.rs b/codex-rs/windows-sandbox-service/src/package_lifecycle/cleanup.rs index 03fb2b8fd8..3d277159c8 100644 --- a/codex-rs/windows-sandbox-service/src/package_lifecycle/cleanup.rs +++ b/codex-rs/windows-sandbox-service/src/package_lifecycle/cleanup.rs @@ -25,6 +25,7 @@ pub(super) fn clean_up( prepared: &PreparedWindowsSandboxCleanup, runtime: Option<&InstallationRecord>, uninstalling: &AtomicBool, + preinstalled: bool, ) -> Result<()> { crate::service::log_information( crate::service::EVENT_CLEANUP_STARTED, @@ -73,7 +74,9 @@ pub(super) fn clean_up( } }; if let Some(home) = &codex_home { - if desktop.created_codex_home { + if desktop.created_codex_home + && crate::uninstall_policy::clean_uninstall_enabled(home, preinstalled) + { // Release the home itself so it can be deleted; keep its ancestors pinned. if installation.directory_guard.take().is_some() { installation.directory_handles.pop(); @@ -84,8 +87,8 @@ pub(super) fn clean_up( ); } else { prune_codex_home = true; - log_cleanup("skipping recursive codex home removal: existing CLI home"); - // Preserve CLI data without leaving inherited permissions for the deleted group. + log_cleanup("preserving codex home"); + // Retained data must not keep permissions for the deleted sandbox group. record_result( "remove codex home sandbox permissions", resolve_sid("CodexSandboxUsers") diff --git a/codex-rs/windows-sandbox-service/src/uninstall_policy.rs b/codex-rs/windows-sandbox-service/src/uninstall_policy.rs new file mode 100644 index 0000000000..f452e71a08 --- /dev/null +++ b/codex-rs/windows-sandbox-service/src/uninstall_policy.rs @@ -0,0 +1,28 @@ +//! Reads the owner's current home-deletion preference; existing OEM preinstalls retain their default. + +use std::io; +use std::path::Path; + +use codex_config::AbsolutePathBufGuard; +use codex_config::config_toml::ConfigToml; + +pub(crate) fn clean_uninstall_enabled(home: &Path, preinstalled: bool) -> bool { + let contents = match std::fs::read_to_string(home.join("config.toml")) { + Ok(contents) => contents, + Err(error) if error.kind() == io::ErrorKind::NotFound => return preinstalled, + Err(_) => return false, + }; + let _guard = AbsolutePathBufGuard::new(home); + let Ok(config) = toml::from_str::(&contents) else { + return false; + }; + config + .desktop + .as_ref() + .and_then(|desktop| desktop.get("uninstall_codex_home")) + .map_or(preinstalled, |value| value.as_bool().unwrap_or(false)) +} + +#[cfg(test)] +#[path = "uninstall_policy_tests.rs"] +mod tests; diff --git a/codex-rs/windows-sandbox-service/src/uninstall_policy_tests.rs b/codex-rs/windows-sandbox-service/src/uninstall_policy_tests.rs new file mode 100644 index 0000000000..cb4708913e --- /dev/null +++ b/codex-rs/windows-sandbox-service/src/uninstall_policy_tests.rs @@ -0,0 +1,74 @@ +//! Verifies local configuration precedence over the preinstalled cleanup default. + +use anyhow::Result; +use pretty_assertions::assert_eq; + +use super::clean_uninstall_enabled; + +/// Existing preinstalls keep clean uninstall when no user preference exists. +#[test] +fn missing_preference_uses_preinstall_default() -> Result<()> { + let home = tempfile::tempdir()?; + for preinstalled in [false, true] { + assert_eq!( + clean_uninstall_enabled(home.path(), preinstalled), + preinstalled + ); + } + for contents in ["", "log_dir = 'logs'", "[desktop]\ntheme = 'dark'"] { + std::fs::write(home.path().join("config.toml"), contents)?; + for preinstalled in [false, true] { + assert_eq!( + clean_uninstall_enabled(home.path(), preinstalled), + preinstalled + ); + } + } + Ok(()) +} + +/// A user's explicit choice overrides both ordinary and preinstalled defaults. +#[test] +fn explicit_preference_overrides_preinstall_default() -> Result<()> { + let home = tempfile::tempdir()?; + for enabled in [false, true] { + std::fs::write( + home.path().join("config.toml"), + format!("log_dir = 'logs'\n[desktop]\nuninstall_codex_home = {enabled}"), + )?; + for preinstalled in [false, true] { + assert_eq!(clean_uninstall_enabled(home.path(), preinstalled), enabled); + } + } + Ok(()) +} + +/// Invalid configuration never falls back to destructive cleanup, even on a preload. +#[test] +fn malformed_or_invalid_preference_preserves_home() -> Result<()> { + let home = tempfile::tempdir()?; + for contents in [ + "[desktop", + "desktop = false", + "[desktop]\nuninstall_codex_home = 'true'", + "[desktop]\nuninstall_codex_home = 1", + "[desktop]\nuninstall_codex_home = []", + ] { + std::fs::write(home.path().join("config.toml"), contents)?; + for preinstalled in [false, true] { + assert!(!clean_uninstall_enabled(home.path(), preinstalled)); + } + } + Ok(()) +} + +/// Failure to read an existing config must not restore the preinstalled deletion default. +#[test] +fn unreadable_config_preserves_home() -> Result<()> { + let home = tempfile::tempdir()?; + std::fs::create_dir(home.path().join("config.toml"))?; + for preinstalled in [false, true] { + assert!(!clean_uninstall_enabled(home.path(), preinstalled)); + } + Ok(()) +}