core/core-agent-ide
8
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

bug fixes and perf improvements for elevated sandbox setup (#8094)

Browse source 
a few fixes based on testing feedback:
* ensure cap_sid file is always written by elevated setup.
* always log to same file whether using elevated sandbox or not
* process potentially slow ACE write operations in parallel
* dedupe write roots so we don't double process any
* don't try to create read/write ACEs on the same directories, due to
race condition
This commit is contained in:
iceweasel-oai 2025-12-16 09:48:29 -08:00 • committed by GitHub
parent b53889aed5
commit 3d14da9728
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 186 additions and 120 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
codex-rs/windows-sandbox-rs/src/audit.rs
View file

@ -251,7 +251,7 @@ pub fn apply_capability_denies_for_world_writable(
}
std::fs::create_dir_all(codex_home)?;
let cap_path = cap_sid_file(codex_home);
let caps = load_or_create_cap_sids(codex_home);
let caps = load_or_create_cap_sids(codex_home)?;
std::fs::write(&cap_path, serde_json::to_string(&caps)?)?;
let (active_sid, workspace_roots): (*mut c_void, Vec<PathBuf>) = match sandbox_policy {
SandboxPolicy::WorkspaceWrite { writable_roots, .. } => {

44
codex-rs/windows-sandbox-rs/src/cap.rs
View file

@ -1,3 +1,5 @@
use anyhow::Context;
use anyhow::Result;
use rand::rngs::SmallRng;
use rand::RngCore;
use rand::SeedableRng;
@ -26,25 +28,39 @@ fn make_random_cap_sid_string() -> String {
format!("S-1-5-21-{}-{}-{}-{}", a, b, c, d)
}
pub fn load_or_create_cap_sids(codex_home: &Path) -> CapSids {
fn persist_caps(path: &Path, caps: &CapSids) -> Result<()> {
if let Some(dir) = path.parent() {
fs::create_dir_all(dir)
.with_context(|| format!("create cap sid dir {}", dir.display()))?;
}
let json = serde_json::to_string(caps)?;
fs::write(path, json).with_context(|| format!("write cap sid file {}", path.display()))?;
Ok(())
}
pub fn load_or_create_cap_sids(codex_home: &Path) -> Result<CapSids> {
let path = cap_sid_file(codex_home);
if path.exists() {
if let Ok(txt) = fs::read_to_string(&path) {
let t = txt.trim();
if t.starts_with('{') && t.ends_with('}') {
if let Ok(obj) = serde_json::from_str::<CapSids>(t) {
return obj;
}
} else if !t.is_empty() {
return CapSids {
workspace: t.to_string(),
readonly: make_random_cap_sid_string(),
};
let txt = fs::read_to_string(&path)
.with_context(|| format!("read cap sid file {}", path.display()))?;
let t = txt.trim();
if t.starts_with('{') && t.ends_with('}') {
if let Ok(obj) = serde_json::from_str::<CapSids>(t) {
return Ok(obj);
}
} else if !t.is_empty() {
let caps = CapSids {
workspace: t.to_string(),
readonly: make_random_cap_sid_string(),
};
persist_caps(&path, &caps)?;
return Ok(caps);
}
}
CapSids {
let caps = CapSids {
workspace: make_random_cap_sid_string(),
readonly: make_random_cap_sid_string(),
}
};
persist_caps(&path, &caps)?;
Ok(caps)
}

54
codex-rs/windows-sandbox-rs/src/elevated_impl.rs
View file

@ -2,7 +2,6 @@ mod windows_impl {
use crate::acl::allow_null_device;
use crate::allow::compute_allow_paths;
use crate::allow::AllowDenyPaths;
use crate::cap::cap_sid_file;
use crate::cap::load_or_create_cap_sids;
use crate::env::ensure_non_interactive_pager;
use crate::env::inherit_path_env;
@ -53,13 +52,6 @@ mod windows_impl {
use windows_sys::Win32::System::Threading::STARTUPINFOW;
/// Ensures the parent directory of a path exists before writing to it.
fn ensure_dir(p: &Path) -> Result<()> {
if let Some(d) = p.parent() {
std::fs::create_dir_all(d)?;
}
Ok(())
}
/// Walks upward from `start` to locate the git worktree root, following gitfile redirects.
fn find_git_root(start: &Path) -> Option<PathBuf> {
let mut cur = dunce::canonicalize(start).ok()?;
@ -246,44 +238,26 @@ mod windows_impl {
let sandbox_creds =
require_logon_sandbox_creds(&policy, sandbox_policy_cwd, cwd, &env_map, codex_home)?;
log_note("cli creds ready", logs_base_dir);
let cap_sid_path = cap_sid_file(codex_home);
// Build capability SID for ACL grants.
if matches!(&policy, SandboxPolicy::DangerFullAccess) {
anyhow::bail!("DangerFullAccess is not supported for sandboxing")
}
let caps = load_or_create_cap_sids(codex_home)?;
let (psid_to_use, cap_sid_str) = match &policy {
SandboxPolicy::ReadOnly => {
let caps = load_or_create_cap_sids(codex_home);
ensure_dir(&cap_sid_path)?;
fs::write(&cap_sid_path, serde_json::to_string(&caps)?)?;
(
unsafe { convert_string_sid_to_sid(&caps.readonly).unwrap() },
caps.readonly.clone(),
)
}
SandboxPolicy::WorkspaceWrite { .. } => {
let caps = load_or_create_cap_sids(codex_home);
ensure_dir(&cap_sid_path)?;
fs::write(&cap_sid_path, serde_json::to_string(&caps)?)?;
(
unsafe { convert_string_sid_to_sid(&caps.workspace).unwrap() },
caps.workspace.clone(),
)
}
SandboxPolicy::DangerFullAccess => {
anyhow::bail!("DangerFullAccess is not supported for sandboxing")
}
SandboxPolicy::ReadOnly => (
unsafe { convert_string_sid_to_sid(&caps.readonly).unwrap() },
caps.readonly.clone(),
),
SandboxPolicy::WorkspaceWrite { .. } => (
unsafe { convert_string_sid_to_sid(&caps.workspace).unwrap() },
caps.workspace.clone(),
),
SandboxPolicy::DangerFullAccess => unreachable!("DangerFullAccess handled above"),
};
let AllowDenyPaths { allow, deny } =
let AllowDenyPaths { allow: _, deny: _ } =
compute_allow_paths(&policy, sandbox_policy_cwd, &current_dir, &env_map);
// Deny/allow ACEs are now applied during setup; avoid per-command churn.
log_note(
&format!(
"cli skipping per-command ACL grants (allow_count={} deny_count={})",
allow.len(),
deny.len()
),
logs_base_dir,
);
unsafe {
allow_null_device(psid_to_use);
}

29
codex-rs/windows-sandbox-rs/src/lib.rs
View file

@ -85,7 +85,6 @@ mod windows_impl {
use super::acl::revoke_ace;
use super::allow::compute_allow_paths;
use super::allow::AllowDenyPaths;
use super::cap::cap_sid_file;
use super::cap::load_or_create_cap_sids;
use super::env::apply_no_network_to_env;
use super::env::ensure_non_interactive_pager;
@ -104,7 +103,6 @@ mod windows_impl {
use anyhow::Result;
use std::collections::HashMap;
use std::ffi::c_void;
use std::fs;
use std::io;
use std::path::Path;
use std::path::PathBuf;
@ -130,13 +128,6 @@ mod windows_impl {
!policy.has_full_network_access()
}
fn ensure_dir(p: &Path) -> Result<()> {
if let Some(d) = p.parent() {
std::fs::create_dir_all(d)?;
}
Ok(())
}
fn ensure_codex_home_exists(p: &Path) -> Result<()> {
std::fs::create_dir_all(p)?;
Ok(())
@ -194,32 +185,28 @@ mod windows_impl {
apply_no_network_to_env(&mut env_map)?;
}
ensure_codex_home_exists(codex_home)?;
let current_dir = cwd.to_path_buf();
let logs_base_dir = Some(codex_home);
let sandbox_base = codex_home.join(".sandbox");
std::fs::create_dir_all(&sandbox_base)?;
let logs_base_dir = Some(sandbox_base.as_path());
log_start(&command, logs_base_dir);
let cap_sid_path = cap_sid_file(codex_home);
let is_workspace_write = matches!(&policy, SandboxPolicy::WorkspaceWrite { .. });
if matches!(&policy, SandboxPolicy::DangerFullAccess) {
anyhow::bail!("DangerFullAccess is not supported for sandboxing")
}
let caps = load_or_create_cap_sids(codex_home)?;
let (h_token, psid_to_use): (HANDLE, *mut c_void) = unsafe {
match &policy {
SandboxPolicy::ReadOnly => {
let caps = load_or_create_cap_sids(codex_home);
ensure_dir(&cap_sid_path)?;
fs::write(&cap_sid_path, serde_json::to_string(&caps)?)?;
let psid = convert_string_sid_to_sid(&caps.readonly).unwrap();
super::token::create_readonly_token_with_cap(psid)?
}
SandboxPolicy::WorkspaceWrite { .. } => {
let caps = load_or_create_cap_sids(codex_home);
ensure_dir(&cap_sid_path)?;
fs::write(&cap_sid_path, serde_json::to_string(&caps)?)?;
let psid = convert_string_sid_to_sid(&caps.workspace).unwrap();
super::token::create_workspace_write_token_with_cap(psid)?
}
SandboxPolicy::DangerFullAccess => {
anyhow::bail!("DangerFullAccess is not supported for sandboxing")
}
SandboxPolicy::DangerFullAccess => unreachable!("DangerFullAccess handled above"),
}
};

104
codex-rs/windows-sandbox-rs/src/setup_main_win.rs
View file

@ -20,6 +20,7 @@ use rand::RngCore;
use rand::SeedableRng;
use serde::Deserialize;
use serde::Serialize;
use std::collections::HashSet;
use std::ffi::c_void;
use std::ffi::OsStr;
use std::fs::File;
@ -392,8 +393,8 @@ fn run_netsh_firewall(sid: &str, log: &mut File) -> Result<()> {
log_line(
log,
&format!(
"firewall rule configured via COM with LocalUserAuthorizedList={local_user_spec}"
),
"firewall rule configured via COM with LocalUserAuthorizedList={local_user_spec}"
),
)?;
Ok(())
})()
@ -647,7 +648,7 @@ fn run_setup(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<()> {
string_from_sid_bytes(&online_sid).map_err(anyhow::Error::msg)?
),
)?;
let caps = load_or_create_cap_sids(&payload.codex_home);
let caps = load_or_create_cap_sids(&payload.codex_home)?;
let cap_psid = unsafe {
convert_string_sid_to_sid(&caps.workspace)
.ok_or_else(|| anyhow::anyhow!("convert capability SID failed"))?
@ -758,7 +759,19 @@ fn run_setup(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<()> {
}
}
let cap_sid_str = caps.workspace.clone();
let online_sid_str = string_from_sid_bytes(&online_sid).map_err(anyhow::Error::msg)?;
let sid_strings = vec![offline_sid_str.clone(), online_sid_str, cap_sid_str];
let write_mask =
FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE | FILE_DELETE_CHILD;
let mut grant_tasks: Vec<PathBuf> = Vec::new();
let mut seen_write_roots: HashSet<PathBuf> = HashSet::new();
for root in &payload.write_roots {
if !seen_write_roots.insert(root.clone()) {
continue;
}
if !root.exists() {
log_line(
log,
@ -766,12 +779,6 @@ fn run_setup(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<()> {
)?;
continue;
}
let sids = vec![offline_psid, online_psid, cap_psid];
let write_mask = FILE_GENERIC_READ
| FILE_GENERIC_WRITE
| FILE_GENERIC_EXECUTE
| DELETE
| FILE_DELETE_CHILD;
let mut need_grant = false;
for (label, psid) in [
("offline", offline_psid),
@ -817,25 +824,7 @@ fn run_setup(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<()> {
root.display()
),
)?;
match unsafe { ensure_allow_write_aces(root, &sids) } {
Ok(res) => {
log_line(
log,
&format!(
"write ACE {} on {}",
if res { "added" } else { "already present" },
root.display()
),
)?;
}
Err(e) => {
refresh_errors.push(format!("write ACE failed on {}: {}", root.display(), e));
log_line(
log,
&format!("write ACE grant failed on {}: {}", root.display(), e),
)?;
}
}
grant_tasks.push(root.clone());
} else {
log_line(
log,
@ -847,6 +836,65 @@ fn run_setup(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 sid_strings = sid_strings.clone();
let tx = tx.clone();
scope.spawn(move || {
// Convert SID strings to psids locally in this thread.
let mut psids: Vec<*mut c_void> = Vec::new();
for sid_str in &sid_strings {
if let Some(psid) = unsafe { convert_string_sid_to_sid(sid_str) } {
psids.push(psid);
} else {
let _ = tx.send((root.clone(), Err(anyhow::anyhow!("convert SID failed"))));
return;
}
}
let res = unsafe { ensure_allow_write_aces(&root, &psids) };
for psid in psids {
unsafe {
LocalFree(psid as HLOCAL);
}
}
let _ = tx.send((root, res));
});
}
drop(tx);
for (root, res) in rx {
match res {
Ok(added) => {
if log_line(
log,
&format!(
"write ACE {} on {}",
if added { "added" } else { "already present" },
root.display()
),
)
.is_err()
{
// ignore log errors inside scoped thread
}
}
Err(e) => {
refresh_errors.push(format!("write ACE failed on {}: {}", root.display(), e));
if log_line(
log,
&format!("write ACE grant failed on {}: {}", root.display(), e),
)
.is_err()
{
// ignore log errors inside scoped thread
}
}
}
}
});
if refresh_only {
log_line(
log,

73
codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs
View file

@ -1,6 +1,7 @@
use serde::Deserialize;
use serde::Serialize;
use std::collections::HashMap;
use std::collections::HashSet;
use std::ffi::c_void;
use std::os::windows::process::CommandExt;
use std::path::Path;
@ -54,13 +55,22 @@ pub fn run_setup_refresh(
if matches!(policy, SandboxPolicy::DangerFullAccess) {
return Ok(());
}
let (read_roots, write_roots) = build_payload_roots(
policy,
policy_cwd,
command_cwd,
env_map,
codex_home,
None,
None,
);
let payload = ElevationPayload {
version: SETUP_VERSION,
offline_username: OFFLINE_USERNAME.to_string(),
online_username: ONLINE_USERNAME.to_string(),
codex_home: codex_home.to_path_buf(),
read_roots: gather_read_roots(command_cwd, policy),
write_roots: gather_write_roots(policy, policy_cwd, command_cwd, env_map),
read_roots,
write_roots,
real_user: std::env::var("USERNAME").unwrap_or_else(|_| "Administrators".to_string()),
refresh_only: true,
};
@ -219,7 +229,14 @@ pub(crate) fn gather_write_roots(
let AllowDenyPaths { allow, .. } =
compute_allow_paths(policy, policy_cwd, command_cwd, env_map);
roots.extend(allow);
canonical_existing(&roots)
let mut dedup: HashSet<PathBuf> = HashSet::new();
let mut out: Vec<PathBuf> = Vec::new();
for r in canonical_existing(&roots) {
if dedup.insert(r.clone()) {
out.push(r);
}
}
out
}
#[derive(Serialize)]
@ -355,19 +372,15 @@ pub fn run_elevated_setup(
// Ensure the shared sandbox directory exists before we send it to the elevated helper.
let sbx_dir = sandbox_dir(codex_home);
std::fs::create_dir_all(&sbx_dir)?;
let mut write_roots = if let Some(roots) = write_roots_override {
roots
} else {
gather_write_roots(policy, policy_cwd, command_cwd, env_map)
};
if !write_roots.contains(&sbx_dir) {
write_roots.push(sbx_dir.clone());
}
let read_roots = if let Some(roots) = read_roots_override {
roots
} else {
gather_read_roots(command_cwd, policy)
};
let (read_roots, write_roots) = build_payload_roots(
policy,
policy_cwd,
command_cwd,
env_map,
codex_home,
read_roots_override,
write_roots_override,
);
let payload = ElevationPayload {
version: SETUP_VERSION,
offline_username: OFFLINE_USERNAME.to_string(),
@ -381,3 +394,31 @@ pub fn run_elevated_setup(
let needs_elevation = !is_elevated()?;
run_setup_exe(&payload, needs_elevation)
}
fn build_payload_roots(
policy: &SandboxPolicy,
policy_cwd: &Path,
command_cwd: &Path,
env_map: &HashMap<String, String>,
codex_home: &Path,
read_roots_override: Option<Vec<PathBuf>>,
write_roots_override: Option<Vec<PathBuf>>,
) -> (Vec<PathBuf>, Vec<PathBuf>) {
let sbx_dir = sandbox_dir(codex_home);
let mut write_roots = if let Some(roots) = write_roots_override {
canonical_existing(&roots)
} else {
gather_write_roots(policy, policy_cwd, command_cwd, env_map)
};
if !write_roots.contains(&sbx_dir) {
write_roots.push(sbx_dir.clone());
}
let mut read_roots = if let Some(roots) = read_roots_override {
canonical_existing(&roots)
} else {
gather_read_roots(command_cwd, policy)
};
let write_root_set: HashSet<PathBuf> = write_roots.iter().cloned().collect();
read_roots.retain(|root| !write_root_set.contains(root));
(read_roots, write_roots)
}