agent/codex
2
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

core: stop threading SandboxPolicy through exec (#25700)

Browse Source 
## Why

#25450 attempts a broad `SandboxPolicy` removal across several unrelated
surfaces, which makes it hard to review and still leaves new helper code
moving legacy policies around. This PR is a narrower alternative:
migrate only the exec-side Windows sandbox plumbing so the review can
focus on one production path and one compatibility boundary.

The goal is to stop threading `SandboxPolicy` through exec code without
expanding the migration into app-server, protocol, telemetry, config, or
session behavior.

## What changed

- Removed `ExecRequest::compatibility_sandbox_policy()`.
- Changed the Windows restricted-token and elevated filesystem override
helpers to accept `PermissionProfile` plus the split filesystem/network
policies instead of a `SandboxPolicy`.
- Kept the remaining legacy projection local to the writable-root
comparison that still needs to compare split policy behavior against the
legacy Windows backend model.
- Rejected restricted split filesystem policies that still grant
full-disk writes before using the Windows restricted-token backend,
preserving the previous clear-failure behavior for profiles that project
to `ExternalSandbox`.
- Updated the Windows sandbox override tests to exercise the new call
shape and cover the full-write split-profile regression.

## Verification

- `just test -p codex-core windows_restricted_token`
- `just test -p codex-core windows_elevated`
This commit is contained in:
Michael Bolin
2026-06-03 10:41:41 -07:00
committed by GitHub
parent a2a9e767f7
commit 52b359b249
10 changed files with 167 additions and 233 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/exec.rs
+52 -57
View File
@@ -31,19 +31,18 @@ use codex_protocol::error::SandboxErr;
use codex_protocol::exec_output::ExecToolCallOutput;
use codex_protocol::exec_output::StreamOutput;
use codex_protocol::models::PermissionProfile;
use codex_protocol::permissions::FileSystemSandboxKind;
use codex_protocol::permissions::FileSystemSandboxPolicy;
use codex_protocol::permissions::NetworkSandboxPolicy;
use codex_protocol::protocol::Event;
use codex_protocol::protocol::EventMsg;
use codex_protocol::protocol::ExecCommandOutputDeltaEvent;
use codex_protocol::protocol::ExecOutputStream;
use codex_protocol::protocol::SandboxPolicy;
use codex_sandboxing::SandboxCommand;
use codex_sandboxing::SandboxManager;
use codex_sandboxing::SandboxTransformRequest;
use codex_sandboxing::SandboxType;
use codex_sandboxing::SandboxablePreference;
use codex_sandboxing::compatibility_sandbox_policy_for_permission_profile;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_pty::DEFAULT_OUTPUT_BYTES_CAP;
use codex_utils_pty::process_group::kill_child_process_group;
@@ -419,22 +418,17 @@ pub fn build_exec_request(
exec_req.windows_sandbox_level,
exec_req.network.is_some(),
);
let sandbox_policy = exec_req.compatibility_sandbox_policy();
exec_req.windows_sandbox_filesystem_overrides = if use_windows_elevated_backend {
resolve_windows_elevated_filesystem_overrides(
exec_req.sandbox,
&sandbox_policy,
&exec_req.file_system_sandbox_policy,
exec_req.network_sandbox_policy,
&exec_req.permission_profile,
sandbox_cwd,
use_windows_elevated_backend,
)
} else {
resolve_windows_restricted_token_filesystem_overrides(
exec_req.sandbox,
&sandbox_policy,
&exec_req.file_system_sandbox_policy,
exec_req.network_sandbox_policy,
&exec_req.permission_profile,
sandbox_cwd,
exec_req.windows_sandbox_level,
)
@@ -1004,34 +998,28 @@ async fn exec(
}
#[cfg_attr(not(target_os = "windows"), allow(dead_code))]
fn should_use_windows_restricted_token_sandbox(
sandbox: SandboxType,
sandbox_policy: &SandboxPolicy,
file_system_sandbox_policy: &FileSystemSandboxPolicy,
fn permission_profile_supports_windows_restricted_token_sandbox(
permission_profile: &PermissionProfile,
) -> bool {
sandbox == SandboxType::WindowsRestrictedToken
&& file_system_sandbox_policy.kind == FileSystemSandboxKind::Restricted
&& !matches!(
sandbox_policy,
SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. }
)
match permission_profile {
PermissionProfile::Managed { file_system, .. } => {
!file_system.to_sandbox_policy().has_full_disk_write_access()
}
PermissionProfile::Disabled | PermissionProfile::External { .. } => false,
}
}
#[cfg_attr(not(test), allow(dead_code))]
pub(crate) fn unsupported_windows_restricted_token_sandbox_reason(
sandbox: SandboxType,
sandbox_policy: &SandboxPolicy,
file_system_sandbox_policy: &FileSystemSandboxPolicy,
network_sandbox_policy: NetworkSandboxPolicy,
permission_profile: &PermissionProfile,
sandbox_policy_cwd: &AbsolutePathBuf,
windows_sandbox_level: WindowsSandboxLevel,
) -> Option<String> {
if windows_sandbox_level == WindowsSandboxLevel::Elevated {
resolve_windows_elevated_filesystem_overrides(
sandbox,
sandbox_policy,
file_system_sandbox_policy,
network_sandbox_policy,
permission_profile,
sandbox_policy_cwd,
windows_sandbox_level == WindowsSandboxLevel::Elevated,
)
@@ -1039,9 +1027,7 @@ pub(crate) fn unsupported_windows_restricted_token_sandbox_reason(
} else {
resolve_windows_restricted_token_filesystem_overrides(
sandbox,
sandbox_policy,
file_system_sandbox_policy,
network_sandbox_policy,
permission_profile,
sandbox_policy_cwd,
windows_sandbox_level,
)
@@ -1051,9 +1037,7 @@ pub(crate) fn unsupported_windows_restricted_token_sandbox_reason(
pub(crate) fn resolve_windows_restricted_token_filesystem_overrides(
sandbox: SandboxType,
sandbox_policy: &SandboxPolicy,
file_system_sandbox_policy: &FileSystemSandboxPolicy,
network_sandbox_policy: NetworkSandboxPolicy,
permission_profile: &PermissionProfile,
sandbox_policy_cwd: &AbsolutePathBuf,
windows_sandbox_level: WindowsSandboxLevel,
) -> std::result::Result<Option<WindowsSandboxFilesystemOverrides>, String> {
@@ -1063,25 +1047,22 @@ pub(crate) fn resolve_windows_restricted_token_filesystem_overrides(
return Ok(None);
}
let (file_system_sandbox_policy, network_sandbox_policy) =
permission_profile.to_runtime_permissions();
let needs_direct_runtime_enforcement = file_system_sandbox_policy
.needs_direct_runtime_enforcement(network_sandbox_policy, sandbox_policy_cwd);
if should_use_windows_restricted_token_sandbox(
sandbox,
sandbox_policy,
file_system_sandbox_policy,
) && !needs_direct_runtime_enforcement
if permission_profile_supports_windows_restricted_token_sandbox(permission_profile)
&& !needs_direct_runtime_enforcement
{
return Ok(None);
}
if !should_use_windows_restricted_token_sandbox(
sandbox,
sandbox_policy,
file_system_sandbox_policy,
) {
if !permission_profile_supports_windows_restricted_token_sandbox(permission_profile) {
let permission_profile_name = permission_profile_display_name(permission_profile);
return Err(format!(
"windows sandbox backend cannot enforce file_system={:?}, network={network_sandbox_policy:?}, legacy_policy={sandbox_policy:?}; refusing to run unsandboxed",
"windows sandbox backend cannot enforce file_system={:?}, network={network_sandbox_policy:?}, permission_profile={permission_profile_name}; refusing to run unsandboxed",
file_system_sandbox_policy.kind,
));
}
@@ -1090,7 +1071,7 @@ pub(crate) fn resolve_windows_restricted_token_filesystem_overrides(
// but its WRITE_RESTRICTED token does not make capability SID deny-read ACEs
// participate in read access checks. Read restrictions therefore require the
// elevated backend, even when the filesystem root remains readable.
if !windows_policy_has_root_read_access(file_system_sandbox_policy, sandbox_policy_cwd) {
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(),
@@ -1098,7 +1079,7 @@ pub(crate) fn resolve_windows_restricted_token_filesystem_overrides(
}
let additional_deny_read_paths = codex_windows_sandbox::resolve_windows_deny_read_paths(
file_system_sandbox_policy,
&file_system_sandbox_policy,
sandbox_policy_cwd,
)?;
if !additional_deny_read_paths.is_empty() {
@@ -1108,7 +1089,11 @@ pub(crate) fn resolve_windows_restricted_token_filesystem_overrides(
);
}
let legacy_writable_roots = sandbox_policy.get_writable_roots_with_cwd(sandbox_policy_cwd);
let legacy_projection = compatibility_sandbox_policy_for_permission_profile(
permission_profile,
sandbox_policy_cwd.as_path(),
);
let legacy_writable_roots = legacy_projection.get_writable_roots_with_cwd(sandbox_policy_cwd);
let split_writable_roots =
file_system_sandbox_policy.get_writable_roots_with_cwd(sandbox_policy_cwd);
let legacy_root_paths: BTreeSet<PathBuf> = legacy_writable_roots
@@ -1204,9 +1189,7 @@ fn windows_policy_has_root_read_access(
pub(crate) fn resolve_windows_elevated_filesystem_overrides(
sandbox: SandboxType,
sandbox_policy: &SandboxPolicy,
file_system_sandbox_policy: &FileSystemSandboxPolicy,
network_sandbox_policy: NetworkSandboxPolicy,
permission_profile: &PermissionProfile,
sandbox_policy_cwd: &AbsolutePathBuf,
use_windows_elevated_backend: bool,
) -> std::result::Result<Option<WindowsSandboxFilesystemOverrides>, String> {
@@ -1214,19 +1197,19 @@ pub(crate) fn resolve_windows_elevated_filesystem_overrides(
return Ok(None);
}
if !should_use_windows_restricted_token_sandbox(
sandbox,
sandbox_policy,
file_system_sandbox_policy,
) {
let (file_system_sandbox_policy, network_sandbox_policy) =
permission_profile.to_runtime_permissions();
if !permission_profile_supports_windows_restricted_token_sandbox(permission_profile) {
let permission_profile_name = permission_profile_display_name(permission_profile);
return Err(format!(
"windows sandbox backend cannot enforce file_system={:?}, network={network_sandbox_policy:?}, legacy_policy={sandbox_policy:?}; refusing to run unsandboxed",
"windows sandbox backend cannot enforce file_system={:?}, network={network_sandbox_policy:?}, permission_profile={permission_profile_name}; refusing to run unsandboxed",
file_system_sandbox_policy.kind,
));
}
let additional_deny_read_paths = codex_windows_sandbox::resolve_windows_deny_read_paths(
file_system_sandbox_policy,
&file_system_sandbox_policy,
sandbox_policy_cwd,
)?;
@@ -1242,7 +1225,11 @@ pub(crate) fn resolve_windows_elevated_filesystem_overrides(
let needs_direct_runtime_enforcement = file_system_sandbox_policy
.needs_direct_runtime_enforcement(network_sandbox_policy, sandbox_policy_cwd);
let normalize_path = |path: PathBuf| dunce::canonicalize(&path).unwrap_or(path);
let legacy_writable_roots = sandbox_policy.get_writable_roots_with_cwd(sandbox_policy_cwd);
let legacy_projection = compatibility_sandbox_policy_for_permission_profile(
permission_profile,
sandbox_policy_cwd.as_path(),
);
let legacy_writable_roots = legacy_projection.get_writable_roots_with_cwd(sandbox_policy_cwd);
let legacy_root_paths: BTreeSet<PathBuf> = legacy_writable_roots
.iter()
.map(|root| normalize_path(root.root.to_path_buf()))
@@ -1264,7 +1251,7 @@ pub(crate) fn resolve_windows_elevated_filesystem_overrides(
// 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);
windows_policy_has_root_read_access(&file_system_sandbox_policy, sandbox_policy_cwd);
let read_roots_override = if split_has_root_read_access {
None
} else {
@@ -1329,6 +1316,14 @@ pub(crate) fn resolve_windows_elevated_filesystem_overrides(
}))
}
fn permission_profile_display_name(permission_profile: &PermissionProfile) -> &'static str {
match permission_profile {
PermissionProfile::Managed { .. } => "Managed",
PermissionProfile::Disabled => "Disabled",
PermissionProfile::External { .. } => "External",
}
}
fn has_reopened_writable_descendant(
writable_roots: &[codex_protocol::protocol::WritableRoot],
) -> bool {