From 0119f3bfc0942d2565a034cc43fdfca95a006e85 Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Sat, 4 Jul 2026 21:47:24 -0300 Subject: [PATCH] fix(windows-sandbox): grant delete rights to writable roots --- codex-rs/windows-sandbox-rs/src/spawn_prep.rs | 3 +- .../src/unified_exec/tests.rs | 102 ++++++++++++++++++ 2 files changed, 104 insertions(+), 1 deletion(-) diff --git a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs index 382a6a4531..9479c0b8d7 100644 --- a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs +++ b/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 { diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs index 96f5c39e92..461e1daf2f 100644 --- a/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs @@ -454,6 +454,108 @@ 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 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", + "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(), + ), + ]); + + 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(), + ), + (0, false, false, false, Some("outside".to_string())), + "stdout={stdout:?}\n{}", + sandbox_log(codex_home.path()) + ); + }); +} + #[test] fn legacy_capture_cancellation_is_not_reported_as_timeout() { let Some(pwsh) = pwsh_path() else {