Skip to content

feat(sandbox): add Windows deny-read parity #18202

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
viyatb-oai wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat(sandbox): add Windows deny-read parity #18202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7bb835a
feat(sandbox): add Windows deny-read parity
viyatb-oai Apr 16, 2026
1013499
refactor(sandbox): simplify Windows deny-read parity
viyatb-oai Apr 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions codex-rs/core/src/config/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2023,12 +2023,6 @@ impl Config {
}
})
.map_err(std::io::Error::from)?;

if cfg!(target_os = "windows") {
startup_warnings.push(format!(
"managed filesystem deny_read from {filesystem_requirements_source} is only enforced for direct file tools on Windows; shell subprocess reads are not sandboxed"
));
}
}
apply_requirement_constrained_value(
"approvals_reviewer",
Expand Down
114 changes: 59 additions & 55 deletions codex-rs/core/src/exec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,15 +96,17 @@ pub struct ExecParams {

/// Resolved filesystem overrides for the Windows sandbox backends.
///
/// The unelevated restricted-token backend only consumes extra deny-write
/// carveouts on top of the legacy `WorkspaceWrite` allow set. The elevated
/// backend can also consume explicit read and write roots during setup/refresh.
/// Read-root overrides are layered on top of the baseline helper/platform roots
/// that the elevated setup path needs to launch the sandboxed command.
/// Both Windows backends consume extra deny-read paths, and the unelevated
/// restricted-token backend also consumes extra deny-write carveouts on top of
/// the legacy `WorkspaceWrite` allow set. The elevated backend can also consume
/// explicit read and write roots during setup/refresh. Read-root overrides are
/// layered on top of the baseline helper/platform roots that the elevated setup
/// path needs to launch the sandboxed command.
#[derive(Debug, Clone, PartialEq, Eq)]
pub(crate) struct WindowsSandboxFilesystemOverrides {
pub(crate) read_roots_override: Option<Vec<PathBuf>>,
pub(crate) write_roots_override: Option<Vec<PathBuf>>,
pub(crate) additional_deny_read_paths: Vec<AbsolutePathBuf>,
pub(crate) additional_deny_write_paths: Vec<AbsolutePathBuf>,
}

Expand Down Expand Up @@ -490,7 +492,7 @@ async fn exec_windows_sandbox(
) -> Result<RawExecToolCallOutput> {
use crate::config::find_codex_home;
use codex_windows_sandbox::run_windows_sandbox_capture_elevated;
use codex_windows_sandbox::run_windows_sandbox_capture_with_extra_deny_write_paths;
use codex_windows_sandbox::run_windows_sandbox_capture_with_filesystem_overrides;

let ExecParams {
command,
Expand Down Expand Up @@ -531,27 +533,21 @@ 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.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_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 spawn_res = tokio::task::spawn_blocking(move || {
if use_elevated {
run_windows_sandbox_capture_elevated(
Expand All @@ -567,18 +563,20 @@ async fn exec_windows_sandbox(
proxy_enforced,
read_roots_override: elevated_read_roots_override.as_deref(),
write_roots_override: elevated_write_roots_override.as_deref(),
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 {
run_windows_sandbox_capture_with_extra_deny_write_paths(
run_windows_sandbox_capture_with_filesystem_overrides(
policy_str.as_str(),
&sandbox_cwd,
codex_home.as_ref(),
command,
&cwd,
env,
timeout_ms,
&additional_deny_read_paths,
&additional_deny_write_paths,
windows_sandbox_private_desktop,
)
Expand Down Expand Up @@ -971,25 +969,21 @@ pub(crate) fn resolve_windows_restricted_token_filesystem_overrides(
));
}

if !file_system_sandbox_policy.has_full_disk_read_access() {
// The restricted-token backend can add deny ACL overlays, but it cannot
// synthesize a smaller read allowlist. A policy that leaves the filesystem
// root readable and only adds deny-read carveouts is therefore enforceable;
// a policy that removes root read access is not.
if !windows_policy_has_root_read_access(file_system_sandbox_policy, sandbox_policy_cwd) {
return Err(
"windows unelevated restricted-token sandbox cannot enforce split filesystem read restrictions directly; refusing to run unsandboxed"
.to_string(),
);
}

if !file_system_sandbox_policy
.get_unreadable_roots_with_cwd(sandbox_policy_cwd)
.is_empty()
|| !file_system_sandbox_policy
.get_unreadable_globs_with_cwd(sandbox_policy_cwd)
.is_empty()
{
return Err(
"windows unelevated restricted-token sandbox cannot enforce unreadable split filesystem carveouts directly; refusing to run unsandboxed"
.to_string(),
);
}
let additional_deny_read_paths = codex_windows_sandbox::resolve_windows_deny_read_paths(
file_system_sandbox_policy,
sandbox_policy_cwd,
)?;

let legacy_writable_roots = sandbox_policy.get_writable_roots_with_cwd(sandbox_policy_cwd);
let split_writable_roots =
Expand Down Expand Up @@ -1053,13 +1047,14 @@ pub(crate) fn resolve_windows_restricted_token_filesystem_overrides(
}
}

if additional_deny_write_paths.is_empty() {
if additional_deny_read_paths.is_empty() && additional_deny_write_paths.is_empty() {
return Ok(None);
}

Ok(Some(WindowsSandboxFilesystemOverrides {
read_roots_override: None,
write_roots_override: None,
additional_deny_read_paths,
additional_deny_write_paths: additional_deny_write_paths
.into_iter()
.map(|path| AbsolutePathBuf::from_absolute_path(path).map_err(|err| err.to_string()))
Expand All @@ -1073,6 +1068,16 @@ fn normalize_windows_override_path(path: &Path) -> std::result::Result<PathBuf,
.map_err(|err| err.to_string())
}

fn windows_policy_has_root_read_access(
file_system_sandbox_policy: &FileSystemSandboxPolicy,
cwd: &AbsolutePathBuf,
) -> bool {
let Some(root) = cwd.as_path().ancestors().last() else {
return false;
};
file_system_sandbox_policy.can_read_path_with_cwd(root, cwd.as_path())
}

pub(crate) fn resolve_windows_elevated_filesystem_overrides(
sandbox: SandboxType,
sandbox_policy: &SandboxPolicy,
Expand All @@ -1096,18 +1101,10 @@ pub(crate) fn resolve_windows_elevated_filesystem_overrides(
));
}

if !file_system_sandbox_policy
.get_unreadable_roots_with_cwd(sandbox_policy_cwd)
.is_empty()
|| !file_system_sandbox_policy
.get_unreadable_globs_with_cwd(sandbox_policy_cwd)
.is_empty()
{
return Err(
"windows elevated sandbox cannot enforce unreadable split filesystem carveouts directly; refusing to run unsandboxed"
.to_string(),
);
}
let additional_deny_read_paths = codex_windows_sandbox::resolve_windows_deny_read_paths(
file_system_sandbox_policy,
sandbox_policy_cwd,
)?;

let split_writable_roots =
file_system_sandbox_policy.get_writable_roots_with_cwd(sandbox_policy_cwd);
Expand Down Expand Up @@ -1145,11 +1142,16 @@ pub(crate) fn resolve_windows_elevated_filesystem_overrides(
.collect();
let split_root_path_set: BTreeSet<PathBuf> = split_root_paths.iter().cloned().collect();

let matches_legacy_read_access = file_system_sandbox_policy.has_full_disk_read_access()
== sandbox_policy.has_full_disk_read_access();
// `has_full_disk_read_access()` is intentionally false when deny-read
// entries exist. For Windows setup overrides, the important question is
// whether the baseline still reads from the filesystem root and only needs
// additional deny ACLs layered on top.
let split_has_root_read_access =
windows_policy_has_root_read_access(file_system_sandbox_policy, sandbox_policy_cwd);
let matches_legacy_read_access =
split_has_root_read_access == sandbox_policy.has_full_disk_read_access();
let read_roots_override = if matches_legacy_read_access
&& (file_system_sandbox_policy.has_full_disk_read_access()
|| split_readable_root_set == legacy_readable_root_set)
&& (split_has_root_read_access || split_readable_root_set == legacy_readable_root_set)
{
None
} else {
Expand Down Expand Up @@ -1198,6 +1200,7 @@ pub(crate) fn resolve_windows_elevated_filesystem_overrides(

if read_roots_override.is_none()
&& write_roots_override.is_none()
&& additional_deny_read_paths.is_empty()
&& additional_deny_write_paths.is_empty()
{
return Ok(None);
Expand All @@ -1206,6 +1209,7 @@ pub(crate) fn resolve_windows_elevated_filesystem_overrides(
Ok(Some(WindowsSandboxFilesystemOverrides {
read_roots_override,
write_roots_override,
additional_deny_read_paths,
additional_deny_write_paths,
}))
}
Expand Down
Loading
Loading