fix(windows-sandbox): allow deletion in writable roots

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

@@ -301,19 +301,30 @@ pub unsafe fn dacl_has_read_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) ->
     false
 }
 
+// Grant DELETE on each inheriting descendant instead of FILE_DELETE_CHILD on
+// its parent. A parent delete-child grant would bypass a direct deny-write ACE
+// on protected children such as `.git` or an explicit read-only subpath.
 const WRITE_ALLOW_MASK: u32 =
-    FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE | FILE_DELETE_CHILD;
+    FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE;
 
 unsafe fn ensure_allow_mask_aces_with_inheritance_impl(
     path: &Path,
     sids: &[*mut c_void],
     allow_mask: u32,
+    disallow_mask: u32,
     inheritance: u32,
 ) -> Result<bool> {
     let (p_dacl, p_sd) = fetch_dacl_handle(path)?;
     let mut entries: Vec<EXPLICIT_ACCESS_W> = Vec::new();
     for sid in sids {
-        if dacl_mask_allows(p_dacl, &[*sid], allow_mask, /*require_all_bits*/ true) {
+        if dacl_mask_allows(p_dacl, &[*sid], allow_mask, /*require_all_bits*/ true)
+            && !dacl_mask_allows(
+                p_dacl,
+                &[*sid],
+                disallow_mask,
+                /*require_all_bits*/ false,
+            )
+        {
             continue;
         }
         entries.push(EXPLICIT_ACCESS_W {
@@ -386,7 +397,13 @@ pub unsafe fn ensure_allow_mask_aces_with_inheritance(
     allow_mask: u32,
     inheritance: u32,
 ) -> Result<bool> {
-    ensure_allow_mask_aces_with_inheritance_impl(path, sids, allow_mask, inheritance)
+    ensure_allow_mask_aces_with_inheritance_impl(
+        path,
+        sids,
+        allow_mask,
+        /*disallow_mask*/ 0,
+        inheritance,
+    )
 }
 
 /// Ensure all provided SIDs have an allow ACE with the requested mask on the path.
@@ -413,7 +430,13 @@ pub unsafe fn ensure_allow_mask_aces(
 /// # Safety
 /// Caller must pass valid SID pointers and an existing path; free the returned security descriptor with `LocalFree`.
 pub unsafe fn ensure_allow_write_aces(path: &Path, sids: &[*mut c_void]) -> Result<bool> {
-    ensure_allow_mask_aces(path, sids, WRITE_ALLOW_MASK)
+    ensure_allow_mask_aces_with_inheritance_impl(
+        path,
+        sids,
+        WRITE_ALLOW_MASK,
+        FILE_DELETE_CHILD,
+        CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE,
+    )
 }
 
 /// Adds an allow ACE granting read/write/execute to the given SID on the target path.

codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs

@@ -65,6 +65,8 @@ use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_READ;
 use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_WRITE;
 
 const DENY_ACCESS: i32 = 3;
+const WRITE_ROOT_ALLOW_MASK: u32 =
+    FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE;
 
 mod sandbox_users;
 mod setup_runtime_bin;
@@ -158,6 +160,23 @@ fn workspace_write_cap_sids_for_path(
     Ok(sid_strs)
 }
 
+fn write_root_needs_refresh(root: &Path, psid: *mut c_void) -> Result<bool> {
+    if !path_mask_allows(
+        root,
+        &[psid],
+        WRITE_ROOT_ALLOW_MASK,
+        /*require_all_bits*/ true,
+    )? {
+        return Ok(true);
+    }
+    path_mask_allows(
+        root,
+        &[psid],
+        FILE_DELETE_CHILD,
+        /*require_all_bits*/ false,
+    )
+}
+
 fn spawn_read_acl_helper(payload: &Payload, _log: &mut dyn Write) -> Result<()> {
     let mut read_payload = payload.clone();
     read_payload.mode = SetupMode::ReadAclsOnly;
@@ -826,8 +845,6 @@ fn run_setup_full(payload: &Payload, log: &mut dyn Write, sbx_dir: &Path) -> Res
         )?;
     }
 
-    let write_mask =
-        FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE | FILE_DELETE_CHILD;
     let mut grant_tasks: Vec<(PathBuf, String)> = Vec::new();
 
     let mut seen_deny_paths: HashSet<PathBuf> = HashSet::new();
@@ -862,27 +879,26 @@ fn run_setup_full(payload: &Payload, log: &mut dyn Write, sbx_dir: &Path) -> Res
             ("sandbox_group", sandbox_group_psid),
             (cap_label, root_cap_psid),
         ] {
-            let has =
-                match path_mask_allows(root, &[psid], write_mask, /*require_all_bits*/ true) {
-                    Ok(h) => h,
-                    Err(e) => {
-                        refresh_errors.push(format!(
-                            "write mask check failed on {} for {label}: {}",
+            let needs_refresh = match write_root_needs_refresh(root, psid) {
+                Ok(needs_refresh) => needs_refresh,
+                Err(e) => {
+                    refresh_errors.push(format!(
+                        "write ACE check failed on {} for {label}: {}",
+                        root.display(),
+                        e
+                    ));
+                    log_line(
+                        log,
+                        &format!(
+                            "write ACE check failed on {} for {label}: {}; continuing",
                             root.display(),
                             e
-                        ));
-                        log_line(
-                            log,
-                            &format!(
-                                "write mask check failed on {} for {label}: {}; continuing",
-                                root.display(),
-                                e
-                            ),
-                        )?;
-                        false
-                    }
-                };
-            if !has {
+                        ),
+                    )?;
+                    true
+                }
+            };
+            if needs_refresh {
                 need_grant = true;
             }
         }
@@ -1036,13 +1052,21 @@ fn run_setup_full(payload: &Payload, log: &mut dyn Write, sbx_dir: &Path) -> Res
 mod tests {
     use super::Payload;
     use super::SETUP_VERSION;
+    use super::WRITE_ROOT_ALLOW_MASK;
+    use super::convert_string_sid_to_sid;
     use super::workspace_write_cap_sids_for_path;
+    use super::write_root_needs_refresh;
     use codex_otel::StatsigMetricsSettings;
+    use codex_windows_sandbox::ensure_allow_mask_aces;
+    use codex_windows_sandbox::ensure_allow_write_aces;
     use codex_windows_sandbox::load_or_create_cap_sids;
     use codex_windows_sandbox::workspace_write_cap_sid_for_root;
     use pretty_assertions::assert_eq;
     use serde_json::json;
     use std::fs;
+    use windows_sys::Win32::Foundation::HLOCAL;
+    use windows_sys::Win32::Foundation::LocalFree;
+    use windows_sys::Win32::Storage::FileSystem::FILE_DELETE_CHILD;
 
     fn payload_json() -> serde_json::Value {
         json!({
@@ -1090,6 +1114,36 @@ mod tests {
         );
     }
 
+    #[test]
+    fn write_root_refresh_replaces_stale_delete_child_grant() {
+        let temp = tempfile::tempdir().expect("tempdir");
+        let codex_home = temp.path().join("codex-home");
+        let workspace = temp.path().join("workspace");
+        fs::create_dir_all(&codex_home).expect("create codex home");
+        fs::create_dir_all(&workspace).expect("create workspace");
+
+        let sid = workspace_write_cap_sid_for_root(&codex_home, &workspace, &workspace)
+            .expect("workspace sid");
+        let psid = unsafe { convert_string_sid_to_sid(&sid).expect("convert workspace sid") };
+        let stale_write_mask = WRITE_ROOT_ALLOW_MASK | FILE_DELETE_CHILD;
+        let seeded = unsafe { ensure_allow_mask_aces(&workspace, &[psid], stale_write_mask) }
+            .expect("seed stale write ACE");
+        let needs_refresh_before =
+            write_root_needs_refresh(&workspace, psid).expect("check stale write ACE");
+        let replaced = unsafe { ensure_allow_write_aces(&workspace, &[psid]) }
+            .expect("replace stale write ACE");
+        let needs_refresh_after =
+            write_root_needs_refresh(&workspace, psid).expect("check refreshed write ACE");
+        unsafe {
+            LocalFree(psid as HLOCAL);
+        }
+
+        assert_eq!(
+            (seeded, needs_refresh_before, replaced, needs_refresh_after),
+            (true, true, true, false)
+        );
+    }
+
     #[test]
     fn deny_path_under_active_root_uses_only_matching_root_sid() {
         let temp = tempfile::tempdir().expect("tempdir");

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

@@ -1,6 +1,7 @@
 use crate::acl::add_allow_ace;
 use crate::acl::add_deny_write_ace;
 use crate::acl::allow_null_device;
+use crate::acl::ensure_allow_write_aces;
 use crate::allow::AllowDenyPaths;
 use crate::allow::compute_allow_paths_for_permissions;
 use crate::cap::load_or_create_cap_sids;
@@ -294,7 +295,7 @@ pub(crate) fn apply_legacy_session_acl_rules(
                 let Some(root_sid) = matching_root_capability(p, acl_sids.write_root_sids) else {
                     continue;
                 };
-                let _ = add_allow_ace(p, root_sid.sid.as_ptr());
+                let _ = ensure_allow_write_aces(p, &[root_sid.sid.as_ptr()]);
             }
         }
         for p in &deny {

codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs

@@ -454,6 +454,116 @@ fn legacy_capture_powershell_emits_output() {
     );
 }
 
+#[test]
+fn legacy_workspace_write_delete_is_limited_to_writable_roots() {
+    let _guard = legacy_process_test_guard();
+    let runtime = current_thread_runtime();
+    runtime.block_on(async move {
+        // Keep writable roots out of USERPROFILE exclusions such as AppData.
+        let test_root = TempDir::new_in(sandbox_cwd()).expect("create legacy delete test root");
+        let codex_home = sandbox_home("legacy-delete-writable-roots");
+        let workspace = test_root.path().join("workspace");
+        let temp_root = test_root.path().join("temp");
+        let tmp_root = test_root.path().join("tmp");
+        let outside_root = test_root.path().join("outside");
+        for directory in [&workspace, &temp_root, &tmp_root, &outside_root] {
+            fs::create_dir_all(directory).expect("create legacy delete test directory");
+        }
+        let protected_git_dir = workspace.join(".git");
+        fs::create_dir(&protected_git_dir).expect("create protected .git directory");
+
+        let workspace_file = workspace.join("workspace-delete.txt");
+        let temp_file = temp_root.join("temp-delete.txt");
+        let tmp_file = tmp_root.join("tmp-delete.txt");
+        let outside_file = outside_root.join("outside-delete.txt");
+        fs::write(&workspace_file, "workspace").expect("seed workspace file");
+        fs::write(&temp_file, "temp").expect("seed TEMP file");
+        fs::write(&tmp_file, "tmp").expect("seed TMP file");
+        fs::write(&outside_file, "outside").expect("seed outside file");
+
+        let script = workspace.join("delete-fixtures.cmd");
+        fs::write(
+            &script,
+            concat!(
+                "@echo off\r\n",
+                "del /f /q \"%WORKSPACE_DELETE%\"\r\n",
+                "del /f /q \"%TEMP_DELETE%\"\r\n",
+                "del /f /q \"%TMP_DELETE%\"\r\n",
+                "del /f /q \"%OUTSIDE_DELETE%\"\r\n",
+                "rmdir \"%PROTECTED_GIT_DIR%\"\r\n",
+                "exit /b 0\r\n",
+            ),
+        )
+        .expect("write delete script");
+
+        let env_map = HashMap::from([
+            ("TEMP".to_string(), temp_root.to_string_lossy().into_owned()),
+            ("TMP".to_string(), tmp_root.to_string_lossy().into_owned()),
+            (
+                "WORKSPACE_DELETE".to_string(),
+                workspace_file.to_string_lossy().into_owned(),
+            ),
+            (
+                "TEMP_DELETE".to_string(),
+                temp_file.to_string_lossy().into_owned(),
+            ),
+            (
+                "TMP_DELETE".to_string(),
+                tmp_file.to_string_lossy().into_owned(),
+            ),
+            (
+                "OUTSIDE_DELETE".to_string(),
+                outside_file.to_string_lossy().into_owned(),
+            ),
+            (
+                "PROTECTED_GIT_DIR".to_string(),
+                protected_git_dir.to_string_lossy().into_owned(),
+            ),
+        ]);
+
+        let permission_profile = PermissionProfile::workspace_write();
+        let spawned = spawn_windows_sandbox_session_legacy(
+            &permission_profile,
+            workspace_roots_for(workspace.as_path()).as_slice(),
+            codex_home.path(),
+            vec![
+                "C:\\Windows\\System32\\cmd.exe".to_string(),
+                "/d".to_string(),
+                "/c".to_string(),
+                script.display().to_string(),
+            ],
+            workspace.as_path(),
+            env_map,
+            /*timeout_ms*/ Some(5_000),
+            &[],
+            &[],
+            /*tty*/ false,
+            /*stdin_open*/ false,
+            /*use_private_desktop*/ true,
+        )
+        .await
+        .expect("spawn legacy delete session");
+        let (stdout, exit_code) =
+            collect_stdout_and_exit(spawned, codex_home.path(), Duration::from_secs(/*secs*/ 10))
+                .await;
+        let stdout = String::from_utf8_lossy(&stdout);
+
+        assert_eq!(
+            (
+                exit_code,
+                workspace_file.exists(),
+                temp_file.exists(),
+                tmp_file.exists(),
+                fs::read_to_string(&outside_file).ok(),
+                protected_git_dir.is_dir(),
+            ),
+            (0, false, false, false, Some("outside".to_string()), true),
+            "stdout={stdout:?}\n{}",
+            sandbox_log(codex_home.path())
+        );
+    });
+}
+
 #[test]
 fn legacy_capture_cancellation_is_not_reported_as_timeout() {
     let Some(pwsh) = pwsh_path() else {