fix(windows-sandbox): harden deny-read setup

Co-authored-by: Codex noreply@openai.com

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

@@ -496,19 +496,6 @@ fn run_read_acl_only(payload: &Payload, log: &mut File) -> Result<()> {
             }
         }
     }
-    // Deny-read ACEs are applied after read grants so the DACL ends with
-    // explicit deny entries that take precedence over the broad read allowlist.
-    match unsafe { apply_deny_read_acls(&payload.deny_read_paths, sandbox_group_psid) } {
-        Ok(applied) => {
-            if !applied.is_empty() {
-                log_line(log, &format!("applied {} deny-read ACLs", applied.len()))?;
-            }
-        }
-        Err(err) => {
-            refresh_errors.push(format!("apply deny-read ACLs failed: {err}"));
-            log_line(log, &format!("apply deny-read ACLs failed: {err}"))?;
-        }
-    }
     unsafe {
         if !sandbox_group_psid.is_null() {
             LocalFree(sandbox_group_psid as HLOCAL);
@@ -633,13 +620,21 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<(
         );
     }
 
-    // The read ACL helper also applies deny-read ACEs, so it must run whenever
-    // deny-read paths are present even if no new read roots need to be granted.
-    if payload.read_roots.is_empty() && payload.deny_read_paths.is_empty() {
+    // Deny-read ACEs must be present before the sandboxed command starts. Apply
+    // them synchronously here instead of delegating them to the background
+    // helper used for read grants.
+    let applied_deny_read_paths =
+        unsafe { apply_deny_read_acls(&payload.deny_read_paths, sandbox_group_psid) }
+            .context("apply deny-read ACLs")?;
+    if !applied_deny_read_paths.is_empty() {
         log_line(
             log,
-            "no read roots or deny-read paths; skipping read ACL helper",
+            &format!("applied {} deny-read ACLs", applied_deny_read_paths.len()),
         )?;
+    }
+
+    if payload.read_roots.is_empty() {
+        log_line(log, "no read roots to grant; skipping read ACL helper")?;
     } else {
         match read_acl_mutex_exists() {
             Ok(true) => {

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

@@ -51,8 +51,8 @@ pub fn resolve_windows_deny_read_paths(
         return Ok(paths);
     };
 
-    let mut seen_scan_dirs = HashSet::new();
     for pattern in unreadable_globs {
+        let mut seen_scan_dirs = HashSet::new();
         let scan_plan = glob_scan_plan(&pattern);
         collect_existing_glob_matches(
             &scan_plan.root,
@@ -194,6 +194,9 @@ mod tests {
     use std::path::PathBuf;
     use tempfile::TempDir;
 
+    #[cfg(unix)]
+    use std::os::unix::fs::symlink;
+
     fn unreadable_glob_entry(pattern: String) -> FileSystemSandboxEntry {
         FileSystemSandboxEntry {
             path: FileSystemPath::GlobPattern { pattern },
@@ -312,4 +315,34 @@ mod tests {
             vec![AbsolutePathBuf::from_absolute_path(root_env).expect("absolute root env")]
         );
     }
+
+    #[cfg(unix)]
+    #[test]
+    fn aliased_glob_roots_each_preserve_their_lexical_matches() {
+        let tmp = TempDir::new().expect("tempdir");
+        let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd");
+        let target = tmp.path().join("target");
+        let alias_a = tmp.path().join("alias-a");
+        let alias_b = tmp.path().join("alias-b");
+        let secret = target.join("secret.env");
+        std::fs::create_dir_all(&target).expect("create target");
+        std::fs::write(&secret, "secret").expect("write secret");
+        symlink(&target, &alias_a).expect("create alias a");
+        symlink(&target, &alias_b).expect("create alias b");
+        let policy = FileSystemSandboxPolicy::restricted(vec![
+            unreadable_glob_entry(format!("{}/**/*.env", alias_a.display())),
+            unreadable_glob_entry(format!("{}/**/*.env", alias_b.display())),
+        ]);
+
+        let actual: HashSet<PathBuf> = resolve_windows_deny_read_paths(&policy, &cwd)
+            .expect("resolve")
+            .into_iter()
+            .map(AbsolutePathBuf::into_path_buf)
+            .collect();
+        let expected = [alias_a.join("secret.env"), alias_b.join("secret.env")]
+            .into_iter()
+            .collect();
+
+        assert_eq!(actual, expected);
+    }
 }

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

@@ -821,14 +821,9 @@ fn build_payload_deny_write_paths(
 }
 
 fn build_payload_deny_read_paths(explicit_deny_read_paths: Option<Vec<PathBuf>>) -> Vec<PathBuf> {
-    // Preserve missing exact deny paths so the Windows helper can materialize
-    // and deny them before the sandboxed process runs. Existing paths are
-    // canonicalized here to make the elevated helper operate on stable targets.
-    explicit_deny_read_paths
-        .unwrap_or_default()
-        .into_iter()
-        .map(|path| canonicalize_path(&path))
-        .collect()
+    // Keep the configured spelling here so the ACL layer can plan both the
+    // lexical path and any existing canonical target for reparse-point aliases.
+    explicit_deny_read_paths.unwrap_or_default()
 }
 
 fn expand_user_profile_root(roots: Vec<PathBuf>) -> Vec<PathBuf> {
@@ -1472,7 +1467,7 @@ mod tests {
     }
 
     #[test]
-    fn build_payload_deny_read_paths_keeps_missing_paths_and_canonicalizes_existing_paths() {
+    fn build_payload_deny_read_paths_preserves_explicit_paths() {
         let tmp = TempDir::new().expect("tempdir");
         let existing = tmp.path().join("secret.env");
         let missing = tmp.path().join("future.env");
@@ -1480,10 +1475,7 @@ mod tests {
 
         assert_eq!(
             super::build_payload_deny_read_paths(Some(vec![existing.clone(), missing.clone()])),
-            vec![
-                dunce::canonicalize(existing).expect("canonical existing"),
-                missing
-            ]
+            vec![existing, missing]
         );
     }
 }