Propagate Windows sandbox ACL update failures (#39279)

## Why

Windows sandbox preflight could report success when applying a deny ACE failed,
leaving a detected world-writable path without the intended capability
restriction.

## What changed

- Return errors from `SetEntriesInAclW` and `SetNamedSecurityInfoW`, including the
  affected path in ACL API errors.
- Attempt every flagged path, aggregate deny-ACE failures, and fail preflight
  after logging them.
- Preserve cleanup of security descriptors and newly allocated ACLs on failure.

## Testing

Added tests that verify ACL API failures are returned and that preflight keeps
processing remaining paths before propagating an error.

GitOrigin-RevId: e2be1c70f72840046dc55760364de7bcf3b1bdc9

codex-rs/windows-sandbox-rs/src/acl.rs

@@ -56,6 +56,14 @@ const GENERIC_READ_MASK: u32 = 0x8000_0000;
 const GENERIC_WRITE_MASK: u32 = 0x4000_0000;
 const DENY_ACCESS: i32 = 3;
 
+fn acl_api_result(path: &Path, operation: &str, code: u32) -> Result<()> {
+    if code == ERROR_SUCCESS {
+        Ok(())
+    } else {
+        Err(anyhow!("{operation} failed for {}: {code}", path.display()))
+    }
+}
+
 /// Fetch DACL via handle-based query; caller must LocalFree the returned SD.
 ///
 /// # Safety
@@ -625,10 +633,17 @@ unsafe fn add_deny_ace(path: &Path, psid: *mut c_void, kind: DenyAceKind) -> Res
         &mut p_sd,
     );
     if code != ERROR_SUCCESS {
-        return Err(anyhow!("GetNamedSecurityInfoW failed: {code}"));
+        if !p_sd.is_null() {
+            LocalFree(p_sd as HLOCAL);
+        }
+        return Err(anyhow!(
+            "GetNamedSecurityInfoW failed for {}: {code}",
+            path.display()
+        ));
     }
-    let mut added = false;
-    if !kind.already_present(p_dacl, psid) {
+    let result = if kind.already_present(p_dacl, psid) {
+        Ok(false)
+    } else {
         let trustee = TRUSTEE_W {
             pMultipleTrustee: std::ptr::null_mut(),
             MultipleTrusteeOperation: 0,
@@ -643,7 +658,9 @@ unsafe fn add_deny_ace(path: &Path, psid: *mut c_void, kind: DenyAceKind) -> Res
         explicit.Trustee = trustee;
         let mut p_new_dacl: *mut ACL = std::ptr::null_mut();
         let code2 = SetEntriesInAclW(1, &explicit, p_dacl, &mut p_new_dacl);
-        if code2 == ERROR_SUCCESS {
+        let result = if let Err(err) = acl_api_result(path, "SetEntriesInAclW", code2) {
+            Err(err)
+        } else {
             let code3 = SetNamedSecurityInfoW(
                 to_wide(path).as_ptr() as *mut u16,
                 1,
@@ -653,18 +670,17 @@ unsafe fn add_deny_ace(path: &Path, psid: *mut c_void, kind: DenyAceKind) -> Res
                 p_new_dacl,
                 std::ptr::null_mut(),
             );
-            if code3 == ERROR_SUCCESS {
-                added = true;
-            }
-            if !p_new_dacl.is_null() {
-                LocalFree(p_new_dacl as HLOCAL);
-            }
+            acl_api_result(path, "SetNamedSecurityInfoW", code3).map(|()| true)
+        };
+        if !p_new_dacl.is_null() {
+            LocalFree(p_new_dacl as HLOCAL);
         }
-    }
+        result
+    };
     if !p_sd.is_null() {
         LocalFree(p_sd as HLOCAL);
     }
-    Ok(added)
+    result
 }
 
 /// Adds a deny ACE to prevent reads for the given SID on the target path.
@@ -680,6 +696,10 @@ pub unsafe fn add_deny_read_ace(path: &Path, psid: *mut c_void) -> Result<bool>
     add_deny_ace(path, psid, DenyAceKind::Read)
 }
 
+#[cfg(test)]
+#[path = "acl_tests.rs"]
+mod tests;
+
 pub unsafe fn revoke_ace(path: &Path, psid: *mut c_void) {
     let mut p_sd: *mut c_void = std::ptr::null_mut();
     let mut p_dacl: *mut ACL = std::ptr::null_mut();

codex-rs/windows-sandbox-rs/src/acl_tests.rs

@@ -0,0 +1,15 @@
+use super::acl_api_result;
+use pretty_assertions::assert_eq;
+use windows_sys::Win32::Foundation::ERROR_ACCESS_DENIED;
+
+#[test]
+fn deny_ace_update_failure_is_an_error() {
+    let path = std::path::Path::new(r"C:\world-writable");
+    let error = acl_api_result(path, "SetNamedSecurityInfoW", ERROR_ACCESS_DENIED)
+        .expect_err("access denied must not look like an already-present ACE");
+
+    assert_eq!(
+        error.to_string(),
+        r"SetNamedSecurityInfoW failed for C:\world-writable: 5"
+    );
+}

codex-rs/windows-sandbox-rs/src/audit.rs

@@ -12,6 +12,7 @@ use crate::setup::effective_write_roots_for_permissions;
 use crate::token::LocalSid;
 use crate::token::world_sid;
 use anyhow::Result;
+use anyhow::anyhow;
 use std::collections::HashSet;
 use std::ffi::OsStr;
 use std::ffi::c_void;
@@ -228,20 +229,21 @@ pub fn apply_world_writable_scan_and_denies_for_permissions(
     if flagged.is_empty() {
         return Ok(());
     }
-    if let Err(err) = apply_capability_denies_for_world_writable_for_permissions(
+    let result = apply_capability_denies_for_world_writable_for_permissions(
         codex_home,
         &flagged,
         permissions,
         cwd,
         env_map,
         logs_base_dir,
-    ) {
+    );
+    if let Err(err) = &result {
         log_note(
             &format!("AUDIT: failed to apply capability deny ACEs: {err}"),
             logs_base_dir,
         );
     }
-    Ok(())
+    result
 }
 
 fn apply_capability_denies_for_world_writable_for_permissions(
@@ -282,6 +284,23 @@ fn apply_capability_denies_for_world_writable_for_permissions(
         } else {
             (vec![LocalSid::from_string(&caps.readonly)?], Vec::new())
         };
+    apply_capability_denies_to_paths(
+        flagged,
+        &workspace_roots,
+        &active_sids,
+        logs_base_dir,
+        |path, sid| unsafe { add_deny_write_ace(path, sid) },
+    )
+}
+
+fn apply_capability_denies_to_paths(
+    flagged: &[PathBuf],
+    workspace_roots: &[PathBuf],
+    active_sids: &[LocalSid],
+    logs_base_dir: Option<&Path>,
+    mut apply_deny: impl FnMut(&Path, *mut c_void) -> Result<bool>,
+) -> Result<()> {
+    let mut errors = Vec::new();
     for path in flagged {
         if workspace_roots
             .iter()
@@ -289,31 +308,39 @@ fn apply_capability_denies_for_world_writable_for_permissions(
         {
             continue;
         }
-        for active_sid in &active_sids {
-            let res = unsafe { add_deny_write_ace(path, active_sid.as_ptr()) };
-            match res {
+        for active_sid in active_sids {
+            match apply_deny(path, active_sid.as_ptr()) {
                 Ok(true) => log_note(
                     &format!("AUDIT: applied capability deny ACE to {}", path.display()),
                     logs_base_dir,
                 ),
                 Ok(false) => {}
-                Err(err) => log_note(
-                    &format!(
+                Err(err) => {
+                    let error = format!(
                         "AUDIT: failed to apply capability deny ACE to {}: {}",
                         path.display(),
                         err
-                    ),
-                    logs_base_dir,
-                ),
+                    );
+                    log_note(&error, logs_base_dir);
+                    errors.push(error);
+                }
             }
         }
     }
-    Ok(())
+    if errors.is_empty() {
+        Ok(())
+    } else {
+        Err(anyhow!(errors.join("; ")))
+    }
 }
 
 #[cfg(test)]
 mod tests {
+    use super::apply_capability_denies_to_paths;
     use super::gather_candidates;
+    use crate::token::LocalSid;
+    use anyhow::anyhow;
+    use pretty_assertions::assert_eq;
     use std::collections::HashMap;
     use std::fs;
 
@@ -347,4 +374,27 @@ mod tests {
         assert!(candidates.contains(&canon_b));
         assert!(candidates.contains(&canon_space));
     }
+
+    #[test]
+    fn deny_ace_failure_is_propagated_after_other_paths_are_attempted() {
+        let failed_path = std::path::PathBuf::from(r"C:\failed");
+        let successful_path = std::path::PathBuf::from(r"C:\protected");
+        let flagged = vec![failed_path.clone(), successful_path];
+        let active_sids = vec![LocalSid::from_string("S-1-1-0").expect("world SID")];
+        let mut attempted = Vec::new();
+
+        let error =
+            apply_capability_denies_to_paths(&flagged, &[], &active_sids, None, |path, _sid| {
+                attempted.push(path.to_path_buf());
+                if path == failed_path {
+                    Err(anyhow!("access denied"))
+                } else {
+                    Ok(true)
+                }
+            })
+            .expect_err("a failed deny ACE must fail preflight");
+
+        assert_eq!(attempted, flagged);
+        assert!(error.to_string().contains(r"C:\failed"));
+    }
 }