clone/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-05-23 20:44:50 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor(sandbox): simplify Windows deny-read parity

Browse Source 
Move Windows deny-read path resolution into the Windows sandbox crate, bound non-recursive glob expansion, and avoid unnecessary deny-read setup work when no read roots or persistent records are present.

Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
viyatb-oai
2026-04-16 15:13:21 -07:00
parent c69f7f6f37
commit 5f925ff9b5
7 changed files with 147 additions and 94 deletions
Download Patch File Download Diff File Expand all files Collapse all files

52
codex-rs/core/src/exec.rs
View File

@@ -607,47 +607,23 @@ async fn exec_windows_sandbox(
let proxy_enforced = network.is_some();
let use_elevated = windows_sandbox_uses_elevated_backend(sandbox_level, proxy_enforced);
let additional_deny_write_paths = windows_sandbox_filesystem_overrides
.map(|overrides| {
overrides
.additional_deny_write_paths
.iter()
.map(AbsolutePathBuf::to_path_buf)
.collect::<Vec<_>>()
})
.unwrap_or_default();
.map(|overrides| overrides.additional_deny_write_paths.clone())
.unwrap_or_default()
.into_iter()
.map(AbsolutePathBuf::into_path_buf)
.collect::<Vec<_>>();
let additional_deny_read_paths = windows_sandbox_filesystem_overrides
.map(|overrides| {
overrides
.additional_deny_read_paths
.iter()
.map(AbsolutePathBuf::to_path_buf)
.collect::<Vec<_>>()
})
.unwrap_or_default();
.map(|overrides| overrides.additional_deny_read_paths.clone())
.unwrap_or_default()
.into_iter()
.map(AbsolutePathBuf::into_path_buf)
.collect::<Vec<_>>();
let elevated_read_roots_override = windows_sandbox_filesystem_overrides
.and_then(|overrides| overrides.read_roots_override.clone());
let elevated_read_roots_include_platform_defaults = windows_sandbox_filesystem_overrides
.is_some_and(|overrides| overrides.read_roots_include_platform_defaults);
let elevated_write_roots_override = windows_sandbox_filesystem_overrides
.and_then(|overrides| overrides.write_roots_override.clone());
let elevated_deny_write_paths = windows_sandbox_filesystem_overrides
.map(|overrides| {
overrides
.additional_deny_write_paths
.iter()
.map(AbsolutePathBuf::to_path_buf)
.collect::<Vec<_>>()
})
.unwrap_or_default();
let elevated_deny_read_paths = windows_sandbox_filesystem_overrides
.map(|overrides| {
overrides
.additional_deny_read_paths
.iter()
.map(AbsolutePathBuf::to_path_buf)
.collect::<Vec<_>>()
})
.unwrap_or_default();
let spawn_res = tokio::task::spawn_blocking(move || {
if use_elevated {
run_windows_sandbox_capture_elevated(
@@ -665,8 +641,8 @@ async fn exec_windows_sandbox(
read_roots_include_platform_defaults:
elevated_read_roots_include_platform_defaults,
write_roots_override: elevated_write_roots_override.as_deref(),
deny_read_paths_override: &elevated_deny_read_paths,
deny_write_paths_override: &elevated_deny_write_paths,
deny_read_paths_override: &additional_deny_read_paths,
deny_write_paths_override: &additional_deny_write_paths,
},
)
} else {
@@ -1082,7 +1058,7 @@ pub(crate) fn resolve_windows_restricted_token_filesystem_overrides(
);
}
let additional_deny_read_paths = crate::windows_deny_read::resolve_windows_deny_read_paths(
let additional_deny_read_paths = codex_windows_sandbox::resolve_windows_deny_read_paths(
file_system_sandbox_policy,
sandbox_policy_cwd,
)?;
@@ -1204,7 +1180,7 @@ pub(crate) fn resolve_windows_elevated_filesystem_overrides(
));
}
let additional_deny_read_paths = crate::windows_deny_read::resolve_windows_deny_read_paths(
let additional_deny_read_paths = codex_windows_sandbox::resolve_windows_deny_read_paths(
file_system_sandbox_policy,
sandbox_policy_cwd,
)?;

1
codex-rs/core/src/lib.rs
View File

@@ -111,7 +111,6 @@ pub mod review_format;
pub mod review_prompts;
mod thread_manager;
pub(crate) mod web_search;
pub(crate) mod windows_deny_read;
pub(crate) mod windows_sandbox_read_grants;
pub use thread_manager::ForkSnapshot;
pub use thread_manager::NewThread;

14
codex-rs/windows-sandbox-rs/src/deny_read_acl.rs
View File

@@ -89,12 +89,24 @@ pub fn write_persistent_deny_read_acl_record(
paths: &[PathBuf],
) -> Result<()> {
let record_path = deny_read_acl_record_path(codex_home, kind);
let planned_paths = plan_deny_read_acl_paths(paths);
if planned_paths.is_empty() {
match std::fs::remove_file(&record_path) {
Ok(()) => return Ok(()),
Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(()),
Err(err) => {
return Err(err).with_context(|| {
format!("remove deny-read ACL record {}", record_path.display())
});
}
}
}
if let Some(parent) = record_path.parent() {
std::fs::create_dir_all(parent)
.with_context(|| format!("create deny-read ACL record dir {}", parent.display()))?;
}
let record = DenyReadAclRecord {
paths: plan_deny_read_acl_paths(paths),
paths: planned_paths,
};
let contents = serde_json::to_vec_pretty(&record)
.with_context(|| format!("serialize deny-read ACL record {}", record_path.display()))?;

92
codex-rs/core/src/windows_deny_read.rs → codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs
View File

@@ -8,6 +8,11 @@ use std::collections::HashSet;
use std::path::Path;
use std::path::PathBuf;
struct GlobScanPlan {
root: PathBuf,
max_depth: Option<usize>,
}
/// Resolve split filesystem `None` read entries into concrete Windows ACL targets.
///
/// Windows ACLs do not understand Codex filesystem glob patterns directly. Exact
@@ -15,7 +20,7 @@ use std::path::PathBuf;
/// exist yet. Glob entries are snapshot-expanded to the files/directories that
/// already exist under their literal scan root; future exact paths are handled
/// later by materializing them before the deny ACE is applied.
pub(crate) fn resolve_windows_deny_read_paths(
pub fn resolve_windows_deny_read_paths(
file_system_sandbox_policy: &FileSystemSandboxPolicy,
cwd: &AbsolutePathBuf,
) -> Result<Vec<AbsolutePathBuf>, String> {
@@ -48,12 +53,15 @@ pub(crate) fn resolve_windows_deny_read_paths(
let mut seen_scan_dirs = HashSet::new();
for pattern in unreadable_globs {
let scan_plan = glob_scan_plan(&pattern);
collect_existing_glob_matches(
&glob_scan_root(&pattern),
&scan_plan.root,
&matcher,
&mut paths,
&mut seen,
&mut seen_scan_dirs,
scan_plan.max_depth,
0,
)?;
}
@@ -66,6 +74,8 @@ fn collect_existing_glob_matches(
paths: &mut Vec<AbsolutePathBuf>,
seen_paths: &mut HashSet<PathBuf>,
seen_scan_dirs: &mut HashSet<PathBuf>,
max_depth: Option<usize>,
depth: usize,
) -> Result<(), String> {
if !path.exists() {
return Ok(());
@@ -90,11 +100,23 @@ fn collect_existing_glob_matches(
return Ok(());
}
if max_depth.is_some_and(|max_depth| depth >= max_depth) {
return Ok(());
}
let Ok(entries) = std::fs::read_dir(path) else {
return Ok(());
};
for entry in entries.flatten() {
collect_existing_glob_matches(&entry.path(), matcher, paths, seen_paths, seen_scan_dirs)?;
collect_existing_glob_matches(
&entry.path(),
matcher,
paths,
seen_paths,
seen_scan_dirs,
max_depth,
depth + 1,
)?;
}
Ok(())
@@ -113,7 +135,7 @@ fn push_absolute_path(
Ok(())
}
fn glob_scan_root(pattern: &str) -> PathBuf {
fn glob_scan_plan(pattern: &str) -> GlobScanPlan {
// Start scanning at the deepest literal directory prefix before the first
// glob metacharacter. For example, `C:\repo\**\*.env` only scans `C:\repo`
// instead of the current directory or drive root.
@@ -124,22 +146,43 @@ fn glob_scan_root(pattern: &str) -> PathBuf {
.unwrap_or(pattern.len());
let literal_prefix = &pattern[..first_glob];
let Some(separator_index) = literal_prefix.rfind(['/', '\\']) else {
return PathBuf::from(".");
return GlobScanPlan {
root: PathBuf::from("."),
max_depth: glob_scan_max_depth(pattern),
};
};
let pattern_suffix = &pattern[separator_index + 1..];
let is_drive_root_separator = separator_index > 0
&& literal_prefix
.as_bytes()
.get(separator_index - 1)
.is_some_and(|ch| *ch == b':');
if separator_index == 0 || is_drive_root_separator {
return PathBuf::from(&literal_prefix[..=separator_index]);
return GlobScanPlan {
root: PathBuf::from(&literal_prefix[..=separator_index]),
max_depth: glob_scan_max_depth(pattern_suffix),
};
}
PathBuf::from(literal_prefix[..separator_index].to_string())
GlobScanPlan {
root: PathBuf::from(literal_prefix[..separator_index].to_string()),
max_depth: glob_scan_max_depth(pattern_suffix),
}
}
fn glob_scan_max_depth(pattern_suffix: &str) -> Option<usize> {
let components = pattern_suffix
.split(['/', '\\'])
.filter(|component| !component.is_empty())
.collect::<Vec<_>>();
if components.contains(&"**") {
return None;
}
Some(components.len())
}
#[cfg(test)]
mod tests {
use super::glob_scan_root;
use super::glob_scan_plan;
use super::resolve_windows_deny_read_paths;
use codex_protocol::permissions::FileSystemAccessMode;
use codex_protocol::permissions::FileSystemPath;
@@ -170,14 +213,21 @@ mod tests {
#[test]
fn scan_root_uses_literal_prefix_before_glob() {
assert_eq!(
glob_scan_root("/tmp/work/**/*.env"),
glob_scan_plan("/tmp/work/**/*.env").root,
PathBuf::from("/tmp/work")
);
assert_eq!(
glob_scan_root(r"C:\Users\dev\repo\**\*.env"),
glob_scan_plan(r"C:\Users\dev\repo\**\*.env").root,
PathBuf::from(r"C:\Users\dev\repo")
);
assert_eq!(glob_scan_root(r"C:\*.env"), PathBuf::from(r"C:\"));
assert_eq!(glob_scan_plan(r"C:\*.env").root, PathBuf::from(r"C:\"));
}
#[test]
fn scan_depth_is_bounded_for_non_recursive_globs() {
assert_eq!(glob_scan_plan("/tmp/work/*.env").max_depth, Some(1));
assert_eq!(glob_scan_plan("/tmp/work/*/*.env").max_depth, Some(2));
assert_eq!(glob_scan_plan("/tmp/work/**/*.env").max_depth, None);
}
#[test]
@@ -225,4 +275,24 @@ mod tests {
assert_eq!(actual, expected);
}
#[test]
fn non_recursive_globs_do_not_expand_nested_matches() {
let tmp = TempDir::new().expect("tempdir");
let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd");
let root_env = tmp.path().join(".env");
let nested_env = tmp.path().join("app").join(".env");
std::fs::create_dir_all(nested_env.parent().expect("parent")).expect("create parent");
std::fs::write(&root_env, "secret").expect("write root env");
std::fs::write(&nested_env, "secret").expect("write nested env");
let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry(format!(
"{}/*.env",
tmp.path().display()
))]);
assert_eq!(
resolve_windows_deny_read_paths(&policy, &cwd).expect("resolve"),
vec![AbsolutePathBuf::from_absolute_path(root_env).expect("absolute root env")]
);
}
}

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

@@ -34,6 +34,8 @@ windows_modules!(
workspace_acl
);
mod deny_read_resolver;
#[cfg(target_os = "windows")]
#[path = "conpty/mod.rs"]
mod conpty;
@@ -115,6 +117,7 @@ pub use deny_read_acl::cleanup_stale_persistent_deny_read_acls;
pub use deny_read_acl::plan_deny_read_acl_paths;
#[cfg(target_os = "windows")]
pub use deny_read_acl::write_persistent_deny_read_acl_record;
pub use deny_read_resolver::resolve_windows_deny_read_paths;
#[cfg(target_os = "windows")]
pub use dpapi::protect as dpapi_protect;
#[cfg(target_os = "windows")]

62
codex-rs/windows-sandbox-rs/src/setup_main_win.rs
View File

@@ -469,17 +469,6 @@ fn run_read_acl_only(payload: &Payload, log: &mut File) -> Result<()> {
let sandbox_group_sid = resolve_sandbox_users_group_sid()?;
let sandbox_group_psid = sid_bytes_to_psid(&sandbox_group_sid)?;
let mut refresh_errors: Vec<String> = Vec::new();
let users_sid = resolve_sid("Users")?;
let users_psid = sid_bytes_to_psid(&users_sid)?;
let auth_sid = resolve_sid("Authenticated Users")?;
let auth_psid = sid_bytes_to_psid(&auth_sid)?;
let everyone_sid = resolve_sid("Everyone")?;
let everyone_psid = sid_bytes_to_psid(&everyone_sid)?;
let rx_psids = vec![users_psid, auth_psid, everyone_psid];
let subjects = ReadAclSubjects {
sandbox_group_psid,
rx_psids: &rx_psids,
};
// Stale cleanup must happen before the helper re-grants read ACLs because
// the cleanup primitive revokes all ACEs for the sandbox group SID.
match unsafe {
@@ -503,15 +492,39 @@ fn run_read_acl_only(payload: &Payload, log: &mut File) -> Result<()> {
log_line(log, &format!("cleanup stale deny-read ACLs failed: {err}"))?;
}
}
apply_read_acls(
&payload.read_roots,
&subjects,
log,
&mut refresh_errors,
FILE_GENERIC_READ | FILE_GENERIC_EXECUTE,
"read",
OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE,
)?;
if !payload.read_roots.is_empty() {
let users_sid = resolve_sid("Users")?;
let users_psid = sid_bytes_to_psid(&users_sid)?;
let auth_sid = resolve_sid("Authenticated Users")?;
let auth_psid = sid_bytes_to_psid(&auth_sid)?;
let everyone_sid = resolve_sid("Everyone")?;
let everyone_psid = sid_bytes_to_psid(&everyone_sid)?;
let rx_psids = vec![users_psid, auth_psid, everyone_psid];
let subjects = ReadAclSubjects {
sandbox_group_psid,
rx_psids: &rx_psids,
};
apply_read_acls(
&payload.read_roots,
&subjects,
log,
&mut refresh_errors,
FILE_GENERIC_READ | FILE_GENERIC_EXECUTE,
"read",
OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE,
)?;
unsafe {
if !users_psid.is_null() {
LocalFree(users_psid as HLOCAL);
}
if !auth_psid.is_null() {
LocalFree(auth_psid as HLOCAL);
}
if !everyone_psid.is_null() {
LocalFree(everyone_psid as HLOCAL);
}
}
}
// 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) } {
@@ -534,15 +547,6 @@ fn run_read_acl_only(payload: &Payload, log: &mut File) -> Result<()> {
if !sandbox_group_psid.is_null() {
LocalFree(sandbox_group_psid as HLOCAL);
}
if !users_psid.is_null() {
LocalFree(users_psid as HLOCAL);
}
if !auth_psid.is_null() {
LocalFree(auth_psid as HLOCAL);
}
if !everyone_psid.is_null() {
LocalFree(everyone_psid as HLOCAL);
}
}
if !refresh_errors.is_empty() {
log_line(

17
codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs
View File

@@ -15,6 +15,7 @@ use crate::allow::compute_allow_paths;
use crate::helper_materialization::helper_bin_dir;
use crate::logging::log_note;
use crate::path_normalization::canonical_path_key;
use crate::path_normalization::canonicalize_path;
use crate::policy::SandboxPolicy;
use crate::setup_error::SetupErrorCode;
use crate::setup_error::SetupFailure;
@@ -813,13 +814,7 @@ fn build_payload_deny_write_paths(
let mut deny_write_paths: Vec<PathBuf> = explicit_deny_write_paths
.unwrap_or_default()
.into_iter()
.map(|path| {
if path.exists() {
dunce::canonicalize(&path).unwrap_or(path)
} else {
path
}
})
.map(|path| canonicalize_path(&path))
.collect();
deny_write_paths.extend(allow_deny_paths.deny);
deny_write_paths
@@ -832,13 +827,7 @@ fn build_payload_deny_read_paths(explicit_deny_read_paths: Option<Vec<PathBuf>>)
explicit_deny_read_paths
.unwrap_or_default()
.into_iter()
.map(|path| {
if path.exists() {
dunce::canonicalize(&path).unwrap_or(path)
} else {
path
}
})
.map(|path| canonicalize_path(&path))
.collect()
}