mirror of
https://github.com/openai/codex.git
synced 2026-11-13 06:45:07 +00:00
fix(windows-sandbox): allow deletion in writable roots (#31138)
## Why The legacy unelevated Windows sandbox allowed tools to create and update files in workspace-write roots, but it could not delete files that already existed there. This breaks operations such as `apply_patch` file deletion and replacement in the workspace, `TEMP`, and `TMP`. The delete grant must also preserve deny-write carveouts. Granting `FILE_DELETE_CHILD` on a writable parent would let the sandbox remove protected children such as `.git` or an explicit read-only subpath even when those children have direct deny ACEs. This addresses the delete-failure variant reported in #30009 and #30712. It does not claim to fix their separate split-root setup, elevated-helper, or proxy-related failure modes. ## What Changed - Give writable-root capability ACEs inheritable `DELETE` rights without granting parent-level `FILE_DELETE_CHILD`, so descendants can be removed while protected children remain protected. - Replace stale write ACEs that still contain `FILE_DELETE_CHILD`, and make elevated setup detect and refresh that unsafe legacy state. - Keep read-only capability handling unchanged. - Add Windows regressions covering pre-existing files in the workspace, `TEMP`, and `TMP`, plus protected `.git` and outside-root controls. The core ACL behavior is in [`acl.rs`](https://github.com/openai/codex/blob/767540eec35cc88dafc34d87fcec7e58087413fb/codex-rs/windows-sandbox-rs/src/acl.rs#L303-L438), stale-ACE detection is in [`setup_main/win.rs`](https://github.com/openai/codex/blob/767540eec35cc88dafc34d87fcec7e58087413fb/codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs#L163-L179), and the end-to-end regression is in [`unified_exec/tests.rs`](https://github.com/openai/codex/blob/767540eec35cc88dafc34d87fcec7e58087413fb/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs#L458-L568). ## How to Test On Windows: 1. Start Codex with the legacy unelevated Windows sandbox and a workspace-write permission profile. 2. Seed pre-existing files in the workspace, `TEMP`, and `TMP`; also create a sibling file outside the writable roots and a protected `.git` directory. 3. Delete the three files inside writable roots through a sandboxed command or `apply_patch`. 4. Confirm the writable-root files are deleted, while the outside-root file and protected `.git` directory remain intact. Targeted tests: - `just test -p codex-windows-sandbox` - Windows-only `legacy_workspace_write_delete_is_limited_to_writable_roots` - Windows-only `write_root_refresh_replaces_stale_delete_child_grant` The final SHA passed all 31 required checks, including the Windows Bazel test matrix, in [run 28886245161](https://github.com/openai/codex/actions/runs/28886245161).
This commit is contained in:
1 parent
48cf582331
commit
166534fc22
4 files changed
+214
-26
No files matched your search
@@ -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 {
|
||||
|
||||
Reference in new issue
Block a user