From b1c13b6fe535aeda00ed95b52c80f02e7d6ec745 Mon Sep 17 00:00:00 2001 From: iceweasel-oai Date: Mon, 18 May 2026 11:00:03 -0700 Subject: [PATCH] Simplify legacy Windows sandbox ACL persistence (#22569) ## Why The legacy Windows sandbox still carried a `persist_aces` mode switch, even though the only path that meaningfully applies filesystem ACEs today is `workspace-write`, which already uses the persistent behavior. Legacy read-only sessions rely on the read-only capability SID rather than per-command filesystem ACE mutation, so the temporary cleanup branch had become conceptual overhead without a corresponding behavioral need. Removing that split makes the ACL lifecycle match the current sandbox model more directly and trims the guard/revocation plumbing from the legacy launcher paths. ## What changed - Removed the `persist_aces` parameter from legacy ACL preparation. - Made legacy deny-read handling always use the persistent reconciliation path. - Dropped guard tracking and post-exit ACE revocation from both capture and unified-exec legacy flows. - Kept workspace `.codex` / `.agents` protection tied directly to `WorkspaceWrite` instead of an intermediate persistence flag. ## Verification - `cargo fmt -p codex-windows-sandbox` - `git diff --check` - `cargo test -p codex-windows-sandbox` - 85 passed, 2 ignored, 2 (unrelated) failed locally. --- codex-rs/windows-sandbox-rs/src/lib.rs | 25 +------ codex-rs/windows-sandbox-rs/src/spawn_prep.rs | 74 +++++-------------- .../src/unified_exec/backends/legacy.rs | 25 +------ 3 files changed, 21 insertions(+), 103 deletions(-) diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index db958c9975..da2a2d3634 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -279,7 +279,6 @@ pub use stub::run_windows_sandbox_legacy_preflight; #[cfg(target_os = "windows")] mod windows_impl { - use super::acl::revoke_ace; use super::logging::log_failure; use super::logging::log_success; use super::policy::SandboxPolicy; @@ -292,7 +291,6 @@ mod windows_impl { use super::spawn_prep::prepare_legacy_session_security; use super::spawn_prep::prepare_legacy_spawn_context; use super::spawn_prep::root_capability_sids; - use super::token::LocalSid; use anyhow::Result; use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::HashMap; @@ -424,8 +422,7 @@ mod windows_impl { ); let security = prepare_legacy_session_security(&policy, codex_home, cwd, capability_roots)?; allow_null_device_for_workspace_write(is_workspace_write); - let persist_aces = is_workspace_write; - let guards = apply_legacy_session_acl_rules( + apply_legacy_session_acl_rules( &policy, sandbox_policy_cwd, codex_home, @@ -438,7 +435,6 @@ mod windows_impl { readonly_sid_str: security.readonly_sid_str.as_deref(), write_root_sids: &security.write_root_sids, }, - persist_aces, )?; let (stdin_pair, stdout_pair, stderr_pair) = unsafe { setup_stdio_pipes()? }; let ((in_r, in_w), (out_r, out_w), (err_r, err_w)) = (stdin_pair, stdout_pair, stderr_pair); @@ -463,13 +459,6 @@ mod windows_impl { CloseHandle(out_w); CloseHandle(err_r); CloseHandle(err_w); - if !persist_aces { - for (p, sid_str) in &guards { - if let Ok(sid) = LocalSid::from_string(sid_str) { - revoke_ace(p, sid.as_ptr()); - } - } - } CloseHandle(security.h_token); } return Err(err); @@ -570,15 +559,6 @@ mod windows_impl { log_failure(&command, &format!("exit code {exit_code}"), logs_base_dir); } - if !persist_aces { - unsafe { - for (p, sid_str) in guards { - if let Ok(sid) = LocalSid::from_string(&sid_str) { - revoke_ace(&p, sid.as_ptr()); - } - } - } - } Ok(CaptureResult { exit_code, stdout, @@ -609,7 +589,7 @@ mod windows_impl { codex_home, ); let write_root_sids = root_capability_sids(codex_home, cwd, capability_roots)?; - let _guards = apply_legacy_session_acl_rules( + apply_legacy_session_acl_rules( sandbox_policy, sandbox_policy_cwd, codex_home, @@ -622,7 +602,6 @@ mod windows_impl { readonly_sid_str: None, write_root_sids: &write_root_sids, }, - /*persist_aces*/ true, )?; Ok(()) diff --git a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs index 331df4f438..b27fa822fa 100644 --- a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs +++ b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs @@ -8,7 +8,6 @@ use crate::cap::workspace_write_cap_sid_for_root; use crate::cap::workspace_write_root_contains_path; use crate::cap::workspace_write_root_overlaps_path; use crate::cap::workspace_write_root_specificity; -use crate::deny_read_acl::apply_deny_read_acls; use crate::deny_read_state::sync_persistent_deny_read_acls; use crate::env::apply_no_network_to_env; use crate::env::ensure_non_interactive_pager; @@ -283,11 +282,9 @@ pub(crate) fn apply_legacy_session_acl_rules( additional_deny_read_paths: &[PathBuf], additional_deny_write_paths: &[PathBuf], acl_sids: LegacyAclSids<'_>, - persist_aces: bool, -) -> Result> { +) -> Result<()> { let AllowDenyPaths { allow, mut deny } = compute_allow_paths(policy, sandbox_policy_cwd, current_dir, env_map); - let mut guards: Vec<(PathBuf, String)> = Vec::new(); unsafe { for path in additional_deny_write_paths { // Explicit carveouts must exist before the command starts so the @@ -300,31 +297,19 @@ pub(crate) fn apply_legacy_session_acl_rules( } if let Some(readonly_sid) = acl_sids.readonly_sid { for p in &allow { - if matches!(add_allow_ace(p, readonly_sid.as_ptr()), Ok(true)) - && !persist_aces - && let Some(readonly_sid_str) = acl_sids.readonly_sid_str - { - guards.push((p.clone(), readonly_sid_str.to_string())); - } + let _ = add_allow_ace(p, readonly_sid.as_ptr()); } } else { for p in &allow { let Some(root_sid) = matching_root_capability(p, acl_sids.write_root_sids) else { continue; }; - if matches!(add_allow_ace(p, root_sid.sid.as_ptr()), Ok(true)) && !persist_aces { - guards.push((p.clone(), root_sid.sid_str.clone())); - } + let _ = add_allow_ace(p, root_sid.sid.as_ptr()); } } for p in &deny { for root_sid in deny_root_capabilities_for_path(p, acl_sids.write_root_sids) { - if let Ok(added) = add_deny_write_ace(p, root_sid.sid.as_ptr()) - && added - && !persist_aces - { - guards.push((p.clone(), root_sid.sid_str.clone())); - } + let _ = add_deny_write_ace(p, root_sid.sid.as_ptr()); } } if !additional_deny_read_paths.is_empty() { @@ -332,42 +317,20 @@ pub(crate) fn apply_legacy_session_acl_rules( let Some(readonly_sid_str) = acl_sids.readonly_sid_str else { anyhow::bail!("readonly capability SID string missing"); }; - let applied_deny_read_paths = if persist_aces { - sync_persistent_deny_read_acls( - codex_home, - readonly_sid_str, - additional_deny_read_paths, - readonly_sid.as_ptr(), - )? - } else { - apply_deny_read_acls(additional_deny_read_paths, readonly_sid.as_ptr())? - }; - if !persist_aces { - guards.extend( - applied_deny_read_paths - .into_iter() - .map(|path| (path, readonly_sid_str.to_string())), - ); - } + sync_persistent_deny_read_acls( + codex_home, + readonly_sid_str, + additional_deny_read_paths, + readonly_sid.as_ptr(), + )?; } else { for root_sid in acl_sids.write_root_sids { - let applied_deny_read_paths = if persist_aces { - sync_persistent_deny_read_acls( - codex_home, - &root_sid.sid_str, - additional_deny_read_paths, - root_sid.sid.as_ptr(), - )? - } else { - apply_deny_read_acls(additional_deny_read_paths, root_sid.sid.as_ptr())? - }; - if !persist_aces { - guards.extend( - applied_deny_read_paths - .into_iter() - .map(|path| (path, root_sid.sid_str.clone())), - ); - } + sync_persistent_deny_read_acls( + codex_home, + &root_sid.sid_str, + additional_deny_read_paths, + root_sid.sid.as_ptr(), + )?; } } } @@ -377,8 +340,7 @@ pub(crate) fn apply_legacy_session_acl_rules( if let Some(readonly_sid) = acl_sids.readonly_sid { allow_null_device(readonly_sid.as_ptr()); } - if persist_aces - && matches!(policy, SandboxPolicy::WorkspaceWrite { .. }) + if matches!(policy, SandboxPolicy::WorkspaceWrite { .. }) && let Some(workspace_sid) = matching_root_capability(current_dir, acl_sids.write_root_sids) { @@ -389,7 +351,7 @@ pub(crate) fn apply_legacy_session_acl_rules( } } } - Ok(guards) + Ok(()) } #[allow(clippy::too_many_arguments)] diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs index 2f76147ce9..2cb12bfd71 100644 --- a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs @@ -1,6 +1,5 @@ use super::windows_common::finish_driver_spawn; use super::windows_common::normalize_windows_tty_input; -use crate::acl::revoke_ace; use crate::conpty::ConptyInstance; use crate::conpty::spawn_conpty_process_as_user; use crate::desktop::LaunchDesktop; @@ -16,7 +15,6 @@ use crate::spawn_prep::apply_legacy_session_acl_rules; use crate::spawn_prep::legacy_session_capability_roots; use crate::spawn_prep::prepare_legacy_session_security; use crate::spawn_prep::prepare_legacy_spawn_context; -use crate::token::LocalSid; use anyhow::Result; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_pty::ProcessDriver; @@ -24,7 +22,6 @@ use codex_utils_pty::SpawnedProcess; use codex_utils_pty::TerminalSize; use std::collections::HashMap; use std::path::Path; -use std::path::PathBuf; use std::ptr; use std::sync::Arc; use std::sync::Mutex as StdMutex; @@ -206,7 +203,6 @@ fn finalize_exit( process_handle: Arc>>, thread_handle: HANDLE, output_join: std::thread::JoinHandle<()>, - guards: Vec<(PathBuf, String)>, logs_base_dir: Option<&Path>, command: Vec, ) { @@ -242,14 +238,6 @@ fn finalize_exit( } else { log_failure(&command, &format!("exit code {exit_code}"), logs_base_dir); } - - unsafe { - for (path, cap_sid) in guards { - if let Ok(sid) = LocalSid::from_string(&cap_sid) { - revoke_ace(&path, sid.as_ptr()); - } - } - } } fn resize_conpty_handle(hpc: &Arc>>, size: TerminalSize) -> Result<()> { @@ -325,8 +313,7 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( prepare_legacy_session_security(&common.policy, codex_home, cwd, capability_roots)?; allow_null_device_for_workspace_write(common.is_workspace_write); - let persist_aces = common.is_workspace_write; - let guards = apply_legacy_session_acl_rules( + apply_legacy_session_acl_rules( &common.policy, sandbox_policy_cwd, codex_home, @@ -339,7 +326,6 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( readonly_sid_str: security.readonly_sid_str.as_deref(), write_root_sids: &security.write_root_sids, }, - persist_aces, )?; let (writer_tx, writer_rx) = mpsc::channel::>(128); @@ -375,13 +361,6 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( Ok(handles) => handles, Err(err) => { unsafe { - if !persist_aces { - for (path, cap_sid) in &guards { - if let Ok(sid) = LocalSid::from_string(cap_sid) { - revoke_ace(path, sid.as_ptr()); - } - } - } CloseHandle(security.h_token); } return Err(err); @@ -392,7 +371,6 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( let process_handle = Arc::new(StdMutex::new(Some(pi.hProcess))); let wait_handle = Arc::clone(&process_handle); let command_for_wait = command.clone(); - let guards_for_wait = if persist_aces { Vec::new() } else { guards }; let hpc_for_wait = hpc_handle.clone(); std::thread::spawn(move || { let _desktop = desktop; @@ -423,7 +401,6 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( wait_handle, pi.hThread, output_join, - guards_for_wait, common.logs_base_dir.as_deref(), command_for_wait, );