clone/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-05-19 02:33:10 +00:00
Code Issues Packages Projects Releases Wiki Activity

Simplify legacy Windows sandbox ACL persistence (#22569)

Browse Source 
## Why

The legacy Windows sandbox still carried a `persist_aces` mode switch,
even though the only path that meaningfully applies filesystem ACEs
today is `workspace-write`, which already uses the persistent behavior.
Legacy read-only sessions rely on the read-only capability SID rather
than per-command filesystem ACE mutation, so the temporary cleanup
branch had become conceptual overhead without a corresponding behavioral
need.

Removing that split makes the ACL lifecycle match the current sandbox
model more directly and trims the guard/revocation plumbing from the
legacy launcher paths.

## What changed

- Removed the `persist_aces` parameter from legacy ACL preparation.
- Made legacy deny-read handling always use the persistent
reconciliation path.
- Dropped guard tracking and post-exit ACE revocation from both capture
and unified-exec legacy flows.
- Kept workspace `.codex` / `.agents` protection tied directly to
`WorkspaceWrite` instead of an intermediate persistence flag.

## Verification

- `cargo fmt -p codex-windows-sandbox`
- `git diff --check`
- `cargo test -p codex-windows-sandbox`
  - 85 passed, 2 ignored, 2 (unrelated) failed locally.
This commit is contained in:
iceweasel-oai
2026-05-18 11:00:03 -07:00
committed by GitHub
parent 9286ff2805
commit b1c13b6fe5
3 changed files with 21 additions and 103 deletions
Download Patch File Download Diff File Expand all files Collapse all files

25
codex-rs/windows-sandbox-rs/src/lib.rs
View File

@@ -279,7 +279,6 @@ pub use stub::run_windows_sandbox_legacy_preflight;
#[cfg(target_os = "windows")]
mod windows_impl {
use super::acl::revoke_ace;
use super::logging::log_failure;
use super::logging::log_success;
use super::policy::SandboxPolicy;
@@ -292,7 +291,6 @@ mod windows_impl {
use super::spawn_prep::prepare_legacy_session_security;
use super::spawn_prep::prepare_legacy_spawn_context;
use super::spawn_prep::root_capability_sids;
use super::token::LocalSid;
use anyhow::Result;
use codex_utils_absolute_path::AbsolutePathBuf;
use std::collections::HashMap;
@@ -424,8 +422,7 @@ mod windows_impl {
);
let security = prepare_legacy_session_security(&policy, codex_home, cwd, capability_roots)?;
allow_null_device_for_workspace_write(is_workspace_write);
let persist_aces = is_workspace_write;
let guards = apply_legacy_session_acl_rules(
apply_legacy_session_acl_rules(
&policy,
sandbox_policy_cwd,
codex_home,
@@ -438,7 +435,6 @@ mod windows_impl {
readonly_sid_str: security.readonly_sid_str.as_deref(),
write_root_sids: &security.write_root_sids,
},
persist_aces,
)?;
let (stdin_pair, stdout_pair, stderr_pair) = unsafe { setup_stdio_pipes()? };
let ((in_r, in_w), (out_r, out_w), (err_r, err_w)) = (stdin_pair, stdout_pair, stderr_pair);
@@ -463,13 +459,6 @@ mod windows_impl {
CloseHandle(out_w);
CloseHandle(err_r);
CloseHandle(err_w);
if !persist_aces {
for (p, sid_str) in &guards {
if let Ok(sid) = LocalSid::from_string(sid_str) {
revoke_ace(p, sid.as_ptr());
}
}
}
CloseHandle(security.h_token);
}
return Err(err);
@@ -570,15 +559,6 @@ mod windows_impl {
log_failure(&command, &format!("exit code {exit_code}"), logs_base_dir);
}
if !persist_aces {
unsafe {
for (p, sid_str) in guards {
if let Ok(sid) = LocalSid::from_string(&sid_str) {
revoke_ace(&p, sid.as_ptr());
}
}
}
}
Ok(CaptureResult {
exit_code,
stdout,
@@ -609,7 +589,7 @@ mod windows_impl {
codex_home,
);
let write_root_sids = root_capability_sids(codex_home, cwd, capability_roots)?;
let _guards = apply_legacy_session_acl_rules(
apply_legacy_session_acl_rules(
sandbox_policy,
sandbox_policy_cwd,
codex_home,
@@ -622,7 +602,6 @@ mod windows_impl {
readonly_sid_str: None,
write_root_sids: &write_root_sids,
},
/*persist_aces*/ true,
)?;
Ok(())

74
codex-rs/windows-sandbox-rs/src/spawn_prep.rs
View File

@@ -8,7 +8,6 @@ use crate::cap::workspace_write_cap_sid_for_root;
use crate::cap::workspace_write_root_contains_path;
use crate::cap::workspace_write_root_overlaps_path;
use crate::cap::workspace_write_root_specificity;
use crate::deny_read_acl::apply_deny_read_acls;
use crate::deny_read_state::sync_persistent_deny_read_acls;
use crate::env::apply_no_network_to_env;
use crate::env::ensure_non_interactive_pager;
@@ -283,11 +282,9 @@ pub(crate) fn apply_legacy_session_acl_rules(
additional_deny_read_paths: &[PathBuf],
additional_deny_write_paths: &[PathBuf],
acl_sids: LegacyAclSids<'_>,
persist_aces: bool,
) -> Result<Vec<(PathBuf, String)>> {
) -> Result<()> {
let AllowDenyPaths { allow, mut deny } =
compute_allow_paths(policy, sandbox_policy_cwd, current_dir, env_map);
let mut guards: Vec<(PathBuf, String)> = Vec::new();
unsafe {
for path in additional_deny_write_paths {
// Explicit carveouts must exist before the command starts so the
@@ -300,31 +297,19 @@ pub(crate) fn apply_legacy_session_acl_rules(
}
if let Some(readonly_sid) = acl_sids.readonly_sid {
for p in &allow {
if matches!(add_allow_ace(p, readonly_sid.as_ptr()), Ok(true))
&& !persist_aces
&& let Some(readonly_sid_str) = acl_sids.readonly_sid_str
{
guards.push((p.clone(), readonly_sid_str.to_string()));
}
let _ = add_allow_ace(p, readonly_sid.as_ptr());
}
} else {
for p in &allow {
let Some(root_sid) = matching_root_capability(p, acl_sids.write_root_sids) else {
continue;
};
if matches!(add_allow_ace(p, root_sid.sid.as_ptr()), Ok(true)) && !persist_aces {
guards.push((p.clone(), root_sid.sid_str.clone()));
}
let _ = add_allow_ace(p, root_sid.sid.as_ptr());
}
}
for p in &deny {
for root_sid in deny_root_capabilities_for_path(p, acl_sids.write_root_sids) {
if let Ok(added) = add_deny_write_ace(p, root_sid.sid.as_ptr())
&& added
&& !persist_aces
{
guards.push((p.clone(), root_sid.sid_str.clone()));
}
let _ = add_deny_write_ace(p, root_sid.sid.as_ptr());
}
}
if !additional_deny_read_paths.is_empty() {
@@ -332,42 +317,20 @@ pub(crate) fn apply_legacy_session_acl_rules(
let Some(readonly_sid_str) = acl_sids.readonly_sid_str else {
anyhow::bail!("readonly capability SID string missing");
};
let applied_deny_read_paths = if persist_aces {
sync_persistent_deny_read_acls(
codex_home,
readonly_sid_str,
additional_deny_read_paths,
readonly_sid.as_ptr(),
)?
} else {
apply_deny_read_acls(additional_deny_read_paths, readonly_sid.as_ptr())?
};
if !persist_aces {
guards.extend(
applied_deny_read_paths
.into_iter()
.map(|path| (path, readonly_sid_str.to_string())),
);
}
sync_persistent_deny_read_acls(
codex_home,
readonly_sid_str,
additional_deny_read_paths,
readonly_sid.as_ptr(),
)?;
} else {
for root_sid in acl_sids.write_root_sids {
let applied_deny_read_paths = if persist_aces {
sync_persistent_deny_read_acls(
codex_home,
&root_sid.sid_str,
additional_deny_read_paths,
root_sid.sid.as_ptr(),
)?
} else {
apply_deny_read_acls(additional_deny_read_paths, root_sid.sid.as_ptr())?
};
if !persist_aces {
guards.extend(
applied_deny_read_paths
.into_iter()
.map(|path| (path, root_sid.sid_str.clone())),
);
}
sync_persistent_deny_read_acls(
codex_home,
&root_sid.sid_str,
additional_deny_read_paths,
root_sid.sid.as_ptr(),
)?;
}
}
}
@@ -377,8 +340,7 @@ pub(crate) fn apply_legacy_session_acl_rules(
if let Some(readonly_sid) = acl_sids.readonly_sid {
allow_null_device(readonly_sid.as_ptr());
}
if persist_aces
&& matches!(policy, SandboxPolicy::WorkspaceWrite { .. })
if matches!(policy, SandboxPolicy::WorkspaceWrite { .. })
&& let Some(workspace_sid) =
matching_root_capability(current_dir, acl_sids.write_root_sids)
{
@@ -389,7 +351,7 @@ pub(crate) fn apply_legacy_session_acl_rules(
}
}
}
Ok(guards)
Ok(())
}
#[allow(clippy::too_many_arguments)]

25
codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs
View File

@@ -1,6 +1,5 @@
use super::windows_common::finish_driver_spawn;
use super::windows_common::normalize_windows_tty_input;
use crate::acl::revoke_ace;
use crate::conpty::ConptyInstance;
use crate::conpty::spawn_conpty_process_as_user;
use crate::desktop::LaunchDesktop;
@@ -16,7 +15,6 @@ use crate::spawn_prep::apply_legacy_session_acl_rules;
use crate::spawn_prep::legacy_session_capability_roots;
use crate::spawn_prep::prepare_legacy_session_security;
use crate::spawn_prep::prepare_legacy_spawn_context;
use crate::token::LocalSid;
use anyhow::Result;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_pty::ProcessDriver;
@@ -24,7 +22,6 @@ use codex_utils_pty::SpawnedProcess;
use codex_utils_pty::TerminalSize;
use std::collections::HashMap;
use std::path::Path;
use std::path::PathBuf;
use std::ptr;
use std::sync::Arc;
use std::sync::Mutex as StdMutex;
@@ -206,7 +203,6 @@ fn finalize_exit(
process_handle: Arc<StdMutex<Option<HANDLE>>>,
thread_handle: HANDLE,
output_join: std::thread::JoinHandle<()>,
guards: Vec<(PathBuf, String)>,
logs_base_dir: Option<&Path>,
command: Vec<String>,
) {
@@ -242,14 +238,6 @@ fn finalize_exit(
} else {
log_failure(&command, &format!("exit code {exit_code}"), logs_base_dir);
}
unsafe {
for (path, cap_sid) in guards {
if let Ok(sid) = LocalSid::from_string(&cap_sid) {
revoke_ace(&path, sid.as_ptr());
}
}
}
}
fn resize_conpty_handle(hpc: &Arc<StdMutex<Option<HANDLE>>>, size: TerminalSize) -> Result<()> {
@@ -325,8 +313,7 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy(
prepare_legacy_session_security(&common.policy, codex_home, cwd, capability_roots)?;
allow_null_device_for_workspace_write(common.is_workspace_write);
let persist_aces = common.is_workspace_write;
let guards = apply_legacy_session_acl_rules(
apply_legacy_session_acl_rules(
&common.policy,
sandbox_policy_cwd,
codex_home,
@@ -339,7 +326,6 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy(
readonly_sid_str: security.readonly_sid_str.as_deref(),
write_root_sids: &security.write_root_sids,
},
persist_aces,
)?;
let (writer_tx, writer_rx) = mpsc::channel::<Vec<u8>>(128);
@@ -375,13 +361,6 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy(
Ok(handles) => handles,
Err(err) => {
unsafe {
if !persist_aces {
for (path, cap_sid) in &guards {
if let Ok(sid) = LocalSid::from_string(cap_sid) {
revoke_ace(path, sid.as_ptr());
}
}
}
CloseHandle(security.h_token);
}
return Err(err);
@@ -392,7 +371,6 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy(
let process_handle = Arc::new(StdMutex::new(Some(pi.hProcess)));
let wait_handle = Arc::clone(&process_handle);
let command_for_wait = command.clone();
let guards_for_wait = if persist_aces { Vec::new() } else { guards };
let hpc_for_wait = hpc_handle.clone();
std::thread::spawn(move || {
let _desktop = desktop;
@@ -423,7 +401,6 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy(
wait_handle,
pi.hThread,
output_join,
guards_for_wait,
common.logs_base_dir.as_deref(),
command_for_wait,
);