windows sandbox: stamp workspace SID on extra roots

codex-rs/windows-sandbox-rs/sandbox_smoketests.py

@@ -328,6 +328,43 @@ def main() -> int:
     rc, out, err = run_sbx("workspace-write", ["cmd", "/c", "del /q delme.txt"], WS_ROOT)
     add("WS: delete succeeds (expected on this host)", rc == 0 and not target.exists(), f"rc={rc}, err={err}")
 
+    # 12b. WS: atomic replace inside workspace succeeds
+    replace_target = WS_ROOT / "replace.txt"
+    write_file(replace_target, "old")
+    rc, out, err = run_sbx(
+        "workspace-write",
+        [
+            "python",
+            "-c",
+            "from pathlib import Path; import os; p=Path('replace.txt'); t=Path('replace.txt.tmp'); t.write_text('new', encoding='utf-8'); os.replace(t, p); print(p.read_text(encoding='utf-8'))",
+        ],
+        WS_ROOT,
+    )
+    add(
+        "WS: atomic replace in workspace allowed",
+        rc == 0 and replace_target.read_text(encoding='utf-8').strip() == "new",
+        f"rc={rc}, err={err}",
+    )
+
+    # 12c. WS: atomic replace in additional root succeeds
+    extra_replace_target = EXTRA_ROOT / "replace-extra.txt"
+    write_file(extra_replace_target, "old")
+    rc, out, err = run_sbx(
+        "workspace-write",
+        [
+            "python",
+            "-c",
+            f"from pathlib import Path; import os; p=Path(r'{extra_replace_target}'); t=Path(r'{extra_replace_target}.tmp'); t.write_text('new', encoding='utf-8'); os.replace(t, p); print(p.read_text(encoding='utf-8'))",
+        ],
+        WS_ROOT,
+        additional_root=EXTRA_ROOT,
+    )
+    add(
+        "WS: atomic replace in additional root allowed",
+        rc == 0 and extra_replace_target.read_text(encoding='utf-8').strip() == "new",
+        f"rc={rc}, err={err}",
+    )
+
     # 13. RO: python tries to write (denied)
     pyfile = WS_ROOT / "py_should_fail.txt"; remove_if_exists(pyfile)
     rc, out, err = run_sbx("read-only", ["python", "-c", "open('py_should_fail.txt','w').write('x')"], WS_ROOT)

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

@@ -411,7 +411,9 @@ pub unsafe fn add_allow_ace(path: &Path, psid: *mut c_void) -> Result<bool> {
         ptstrName: psid as *mut u16,
     };
     let mut explicit: EXPLICIT_ACCESS_W = std::mem::zeroed();
-    explicit.grfAccessPermissions = FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE;
+    // Keep legacy one-shot ACL grants aligned with the stronger write mask used by
+    // setup refresh so atomic replace/delete patterns work on stamped paths too.
+    explicit.grfAccessPermissions = WRITE_ALLOW_MASK;
     explicit.grfAccessMode = 2; // SET_ACCESS
     explicit.grfInheritance = CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE;
     explicit.Trustee = trustee;

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

@@ -117,6 +117,32 @@ fn log_line(log: &mut File, msg: &str) -> Result<()> {
     Ok(())
 }
 
+fn write_capability_labels_and_psids_for_root(
+    root: &Path,
+    canonical_command_cwd: &Path,
+    cap_psid: *mut c_void,
+    workspace_psid: *mut c_void,
+) -> Vec<(&'static str, *mut c_void)> {
+    if is_command_cwd_root(root, canonical_command_cwd) {
+        vec![("workspace_cap", workspace_psid)]
+    } else {
+        vec![("cap", cap_psid), ("workspace_cap", workspace_psid)]
+    }
+}
+
+fn write_capability_sid_strings_for_root(
+    root: &Path,
+    canonical_command_cwd: &Path,
+    cap_sid_str: &str,
+    workspace_sid_str: &str,
+) -> Vec<String> {
+    if is_command_cwd_root(root, canonical_command_cwd) {
+        vec![workspace_sid_str.to_string()]
+    } else {
+        vec![cap_sid_str.to_string(), workspace_sid_str.to_string()]
+    }
+}
+
 fn spawn_read_acl_helper(payload: &Payload, _log: &mut File) -> Result<()> {
     let mut read_payload = payload.clone();
     read_payload.mode = SetupMode::ReadAclsOnly;
@@ -676,21 +702,15 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<(
             continue;
         }
         let mut need_grant = false;
-        let is_command_cwd = is_command_cwd_root(root, &canonical_command_cwd);
-        let cap_label = if is_command_cwd {
-            "workspace_cap"
-        } else {
-            "cap"
-        };
-        let cap_psid_for_root = if is_command_cwd {
-            workspace_psid
-        } else {
-            cap_psid
-        };
-        for (label, psid) in [
-            ("sandbox_group", sandbox_group_psid),
-            (cap_label, cap_psid_for_root),
-        ] {
+        for (label, psid) in [("sandbox_group", sandbox_group_psid)].into_iter().chain(
+            write_capability_labels_and_psids_for_root(
+                root,
+                &canonical_command_cwd,
+                cap_psid,
+                workspace_psid,
+            )
+            .into_iter(),
+        ) {
             let has =
                 match path_mask_allows(root, &[psid], write_mask, /*require_all_bits*/ true) {
                     Ok(h) => h,
@@ -719,7 +739,7 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<(
             log_line(
                 log,
                 &format!(
-                    "granting write ACE to {} for sandbox group and capability SID",
+                    "granting write ACE to {} for sandbox group and capability SID set",
                     root.display()
                 ),
             )?;
@@ -730,12 +750,17 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<(
     let (tx, rx) = mpsc::channel::<(PathBuf, Result<bool>)>();
     std::thread::scope(|scope| {
         for root in grant_tasks {
-            let is_command_cwd = is_command_cwd_root(&root, &canonical_command_cwd);
-            let sid_strings = if is_command_cwd {
-                vec![sandbox_group_sid_str.clone(), workspace_sid_str.clone()]
-            } else {
-                vec![sandbox_group_sid_str.clone(), cap_sid_str.clone()]
-            };
+            let sid_strings = std::iter::once(sandbox_group_sid_str.clone())
+                .chain(
+                    write_capability_sid_strings_for_root(
+                        &root,
+                        &canonical_command_cwd,
+                        &cap_sid_str,
+                        &workspace_sid_str,
+                    )
+                    .into_iter(),
+                )
+                .collect::<Vec<_>>();
             let tx = tx.clone();
             scope.spawn(move || {
                 // Convert SID strings to psids locally in this thread.
@@ -798,28 +823,34 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<(
         }
 
         let canonical_path = canonicalize_path(path);
-        let deny_psid = if canonical_path.starts_with(&canonical_command_cwd) {
-            workspace_psid
+        let deny_psids = if canonical_path.starts_with(&canonical_command_cwd) {
+            vec![workspace_psid]
         } else {
-            cap_psid
+            vec![cap_psid, workspace_psid]
         };
 
-        match unsafe { add_deny_write_ace(path, deny_psid) } {
-            Ok(true) => {
-                log_line(
-                    log,
-                    &format!("applied deny ACE to protect {}", path.display()),
-                )?;
-            }
-            Ok(false) => {}
-            Err(err) => {
-                refresh_errors.push(format!("deny ACE failed on {}: {err}", path.display()));
-                log_line(
-                    log,
-                    &format!("deny ACE failed on {}: {err}", path.display()),
-                )?;
+        let mut applied = false;
+        for deny_psid in deny_psids {
+            match unsafe { add_deny_write_ace(path, deny_psid) } {
+                Ok(true) => {
+                    applied = true;
+                }
+                Ok(false) => {}
+                Err(err) => {
+                    refresh_errors.push(format!("deny ACE failed on {}: {err}", path.display()));
+                    log_line(
+                        log,
+                        &format!("deny ACE failed on {}: {err}", path.display()),
+                    )?;
+                }
             }
         }
+        if applied {
+            log_line(
+                log,
+                &format!("applied deny ACE to protect {}", path.display()),
+            )?;
+        }
     }
 
     lock_sandbox_dir(

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

@@ -56,6 +56,24 @@ pub(crate) struct LegacySessionSecurity {
     pub(crate) cap_sid_str: String,
 }
 
+fn allow_psids_for_path<'a>(
+    policy: &SandboxPolicy,
+    path: &Path,
+    canonical_cwd: &Path,
+    psid_generic: &'a LocalSid,
+    psid_workspace: Option<&'a LocalSid>,
+) -> Vec<&'a LocalSid> {
+    if !matches!(policy, SandboxPolicy::WorkspaceWrite { .. }) {
+        return vec![psid_generic];
+    }
+
+    match psid_workspace {
+        Some(workspace_sid) if is_command_cwd_root(path, canonical_cwd) => vec![workspace_sid],
+        Some(workspace_sid) => vec![psid_generic, workspace_sid],
+        None => vec![psid_generic],
+    }
+}
+
 /// Owns a SID allocated by `ConvertStringSidToSidW` and releases it with `LocalFree`.
 pub struct LocalSid {
     psid: *mut c_void,
@@ -226,22 +244,28 @@ pub(crate) fn apply_legacy_session_acl_rules(
     let canonical_cwd = canonicalize_path(current_dir);
     unsafe {
         for p in &allow {
-            let psid = if matches!(policy, SandboxPolicy::WorkspaceWrite { .. })
-                && is_command_cwd_root(p, &canonical_cwd)
+            let mut added_any = false;
+            for sid in allow_psids_for_path(policy, p, &canonical_cwd, psid_generic, psid_workspace)
             {
-                psid_workspace.unwrap_or(psid_generic).as_ptr()
-            } else {
-                psid_generic.as_ptr()
-            };
-            if matches!(add_allow_ace(p, psid), Ok(true)) && !persist_aces {
+                if matches!(add_allow_ace(p, sid.as_ptr()), Ok(true)) {
+                    added_any = true;
+                }
+            }
+            if added_any && !persist_aces {
                 guards.push(p.clone());
             }
         }
         for p in &deny {
-            if let Ok(added) = add_deny_write_ace(p, psid_generic.as_ptr())
-                && added
-                && !persist_aces
+            let mut added_any = false;
+            for sid in allow_psids_for_path(policy, p, &canonical_cwd, psid_generic, psid_workspace)
             {
+                if let Ok(added) = add_deny_write_ace(p, sid.as_ptr())
+                    && added
+                {
+                    added_any = true;
+                }
+            }
+            if added_any && !persist_aces {
                 guards.push(p.clone());
             }
         }