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

fix: align core approvals with split sandbox policies (#14171)

Browse Source 
## Stack

   fix: fail closed for unsupported split windows sandboxing #14172
   fix: preserve split filesystem semantics in linux sandbox #14173
-> fix: align core approvals with split sandbox policies #14171
   refactor: centralize filesystem permissions precedence #14174

## Why This PR Exists

This PR is intentionally narrower than the title may suggest.

Most of the original split-permissions migration already landed in the
earlier `#13434 -> #13453` stack. In particular:

- `#13439` already did the broad runtime plumbing for split filesystem
and network policies.
- `#13445` already moved `apply_patch` safety onto filesystem-policy
semantics.
- `#13448` already switched macOS Seatbelt generation to split policies.
- `#13449` and `#13453` already handled Linux helper and bubblewrap
enforcement.
- `#13440` already introduced the first protocol-side helpers for
deriving effective filesystem access.

The reason this PR still exists is that after the follow-on
`[permissions]` work and the new shared precedence helper in `#14174`, a
few core approval paths were still deciding behavior from the legacy
`SandboxPolicy` projection instead of the split filesystem policy that
actually carries the carveouts.

That means this PR is mostly a cleanup and alignment pass over the
remaining core consumers, not a fresh sandbox backend migration.

## What Is Actually New Here

- make unmatched-command fallback decisions consult
`FileSystemSandboxPolicy` instead of only legacy `DangerFullAccess` /
`ReadOnly` / `WorkspaceWrite` categories
- thread `file_system_sandbox_policy` into the shell, unified-exec, and
intercepted-exec approval paths so they all use the same split-policy
semantics
- keep `apply_patch` safety on the same effective-access rules as the
shared protocol helper, rather than letting it drift through
compatibility projections
- add loader-level regression coverage proving legacy `sandbox_mode`
config still builds split policies and round-trips back without semantic
drift

## What This PR Does Not Do

This PR does not introduce new platform backend enforcement on its own.

- Linux backend parity remains in `#14173`.
- Windows fail-closed handling remains in `#14172`.
- The shared precedence/model changes live in `#14174`.

## Files To Focus On

- `core/src/exec_policy.rs`: unmatched-command fallback and approval
rendering now read the split filesystem policy directly
- `core/src/tools/sandboxing.rs`: default exec-approval requirement keys
off `FileSystemSandboxPolicy.kind`
- `core/src/tools/handlers/shell.rs`: shell approval requests now carry
the split filesystem policy
- `core/src/unified_exec/process_manager.rs`: unified-exec approval
requests now carry the split filesystem policy
- `core/src/tools/runtimes/shell/unix_escalation.rs`: intercepted exec
fallback now uses the same split-policy approval semantics
- `core/src/safety.rs`: `apply_patch` safety keeps using effective
filesystem access rather than legacy sandbox categories
- `core/src/config/config_tests.rs`: new regression coverage for legacy
`sandbox_mode` no-drift behavior through the split-policy loader

## Notes

- `core/src/codex.rs` and `core/src/codex_tests.rs` are just small
fallout updates for `RequestPermissionsResponse.scope`; they are not the
point of the PR.
- If you reviewed the earlier `#13439` / `#13445` stack, the main review
question here is simply: “are there any remaining approval or
patch-safety paths that still reconstruct semantics from legacy
`SandboxPolicy` instead of consuming the split filesystem policy
directly?”

## Testing
- cargo test -p codex-core
legacy_sandbox_mode_config_builds_split_policies_without_drift
- cargo test -p codex-core request_permissions
- cargo test -p codex-core intercepted_exec_policy
- cargo test -p codex-core
restricted_sandbox_requires_exec_approval_on_request
- cargo test -p codex-core
unmatched_on_request_uses_split_filesystem_policy_for_escalation_prompts
- cargo test -p codex-core explicit_
- cargo clippy -p codex-core --tests -- -D warnings
This commit is contained in:
viyatb-oai
2026-03-11 19:23:22 -07:00
committed by GitHub
Unverified
parent c1ea3f95d1
commit c2d5458d67
9 changed files with 259 additions and 76 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/tools/handlers/shell.rs
+1
View File
@@ -403,6 +403,7 @@ impl ShellHandler {
command: &exec_params.command,
approval_policy: turn.approval_policy.value(),
sandbox_policy: turn.sandbox_policy.get(),
file_system_sandbox_policy: &turn.file_system_sandbox_policy,
sandbox_permissions: if effective_additional_permissions.permissions_preapproved {
codex_protocol::models::SandboxPermissions::UseDefault
} else {
codex-rs/core/src/tools/orchestrator.rs
+2 -2
View File
@@ -120,7 +120,7 @@ impl ToolOrchestrator {
let mut already_approved = false;
let requirement = tool.exec_approval_requirement(req).unwrap_or_else(|| {
default_exec_approval_requirement(approval_policy, &turn_ctx.sandbox_policy)
default_exec_approval_requirement(approval_policy, &turn_ctx.file_system_sandbox_policy)
});
match requirement {
ExecApprovalRequirement::Skip { .. } => {
@@ -249,7 +249,7 @@ impl ToolOrchestrator {
&& matches!(
default_exec_approval_requirement(
approval_policy,
&turn_ctx.sandbox_policy
&turn_ctx.file_system_sandbox_policy
),
ExecApprovalRequirement::NeedsApproval { .. }
);
codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs
+27 -9
View File
@@ -692,10 +692,14 @@ impl EscalationPolicy for CoreShellActionProvider {
&policy,
program,
argv,
self.approval_policy,
&self.sandbox_policy,
self.sandbox_permissions,
ENABLE_INTERCEPTED_EXEC_POLICY_SHELL_WRAPPER_PARSING,
InterceptedExecPolicyContext {
approval_policy: self.approval_policy,
sandbox_policy: &self.sandbox_policy,
file_system_sandbox_policy: &self.file_system_sandbox_policy,
sandbox_permissions: self.sandbox_permissions,
enable_shell_wrapper_parsing:
ENABLE_INTERCEPTED_EXEC_POLICY_SHELL_WRAPPER_PARSING,
},
)
};
// When true, means the Evaluation was due to *.rules, not the
@@ -744,15 +748,19 @@ fn evaluate_intercepted_exec_policy(
policy: &Policy,
program: &AbsolutePathBuf,
argv: &[String],
approval_policy: AskForApproval,
sandbox_policy: &SandboxPolicy,
sandbox_permissions: SandboxPermissions,
enable_intercepted_exec_policy_shell_wrapper_parsing: bool,
context: InterceptedExecPolicyContext<'_>,
) -> Evaluation {
let InterceptedExecPolicyContext {
approval_policy,
sandbox_policy,
file_system_sandbox_policy,
sandbox_permissions,
enable_shell_wrapper_parsing,
} = context;
let CandidateCommands {
commands,
used_complex_parsing,
} = if enable_intercepted_exec_policy_shell_wrapper_parsing {
} = if enable_shell_wrapper_parsing {
// In this codepath, the first argument in `commands` could be a bare
// name like `find` instead of an absolute path like `/usr/bin/find`.
// It could also be a shell built-in like `echo`.
@@ -770,6 +778,7 @@ fn evaluate_intercepted_exec_policy(
crate::exec_policy::render_decision_for_unmatched_command(
approval_policy,
sandbox_policy,
file_system_sandbox_policy,
cmd,
sandbox_permissions,
used_complex_parsing,
@@ -785,6 +794,15 @@ fn evaluate_intercepted_exec_policy(
)
}
#[derive(Clone, Copy)]
struct InterceptedExecPolicyContext<'a> {
approval_policy: AskForApproval,
sandbox_policy: &'a SandboxPolicy,
file_system_sandbox_policy: &'a FileSystemSandboxPolicy,
sandbox_permissions: SandboxPermissions,
enable_shell_wrapper_parsing: bool,
}
struct CandidateCommands {
commands: Vec<Vec<String>>,
used_complex_parsing: bool,
codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs
+48 -24
View File
@@ -1,6 +1,7 @@
use super::CoreShellActionProvider;
#[cfg(target_os = "macos")]
use super::CoreShellCommandExecutor;
use super::InterceptedExecPolicyContext;
use super::ParsedShellCommand;
use super::commands_for_intercepted_exec_policy;
use super::evaluate_intercepted_exec_policy;
@@ -36,6 +37,7 @@ use codex_protocol::permissions::FileSystemAccessMode;
use codex_protocol::permissions::FileSystemPath;
use codex_protocol::permissions::FileSystemSandboxEntry;
use codex_protocol::permissions::FileSystemSandboxPolicy;
use codex_protocol::permissions::FileSystemSpecialPath;
use codex_protocol::permissions::NetworkSandboxPolicy;
use codex_protocol::protocol::SkillScope;
use codex_shell_escalation::EscalationExecution;
@@ -67,6 +69,20 @@ fn starlark_string(value: &str) -> String {
value.replace('\\', "\\\\").replace('"', "\\\"")
}
fn read_only_file_system_sandbox_policy() -> FileSystemSandboxPolicy {
FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry {
path: FileSystemPath::Special {
value: FileSystemSpecialPath::Root,
},
access: FileSystemAccessMode::Read,
}])
}
#[cfg(target_os = "macos")]
fn unrestricted_file_system_sandbox_policy() -> FileSystemSandboxPolicy {
FileSystemSandboxPolicy::unrestricted()
}
fn test_skill_metadata(permission_profile: Option<PermissionProfile>) -> SkillMetadata {
SkillMetadata {
name: "skill".to_string(),
@@ -412,10 +428,13 @@ fn evaluate_intercepted_exec_policy_uses_wrapper_command_when_shell_wrapper_pars
"-lc".to_string(),
"npm publish".to_string(),
],
AskForApproval::OnRequest,
&SandboxPolicy::new_read_only_policy(),
SandboxPermissions::UseDefault,
enable_intercepted_exec_policy_shell_wrapper_parsing,
InterceptedExecPolicyContext {
approval_policy: AskForApproval::OnRequest,
sandbox_policy: &SandboxPolicy::new_read_only_policy(),
file_system_sandbox_policy: &read_only_file_system_sandbox_policy(),
sandbox_permissions: SandboxPermissions::UseDefault,
enable_shell_wrapper_parsing: enable_intercepted_exec_policy_shell_wrapper_parsing,
},
);
assert!(
@@ -460,10 +479,13 @@ fn evaluate_intercepted_exec_policy_matches_inner_shell_commands_when_enabled()
"-lc".to_string(),
"npm publish".to_string(),
],
AskForApproval::OnRequest,
&SandboxPolicy::new_read_only_policy(),
SandboxPermissions::UseDefault,
enable_intercepted_exec_policy_shell_wrapper_parsing,
InterceptedExecPolicyContext {
approval_policy: AskForApproval::OnRequest,
sandbox_policy: &SandboxPolicy::new_read_only_policy(),
file_system_sandbox_policy: &read_only_file_system_sandbox_policy(),
sandbox_permissions: SandboxPermissions::UseDefault,
enable_shell_wrapper_parsing: enable_intercepted_exec_policy_shell_wrapper_parsing,
},
);
assert_eq!(
@@ -499,10 +521,13 @@ host_executable(name = "git", paths = ["{git_path_literal}"])
&policy,
&program,
&["git".to_string(), "status".to_string()],
AskForApproval::OnRequest,
&SandboxPolicy::new_read_only_policy(),
SandboxPermissions::UseDefault,
false,
InterceptedExecPolicyContext {
approval_policy: AskForApproval::OnRequest,
sandbox_policy: &SandboxPolicy::new_read_only_policy(),
file_system_sandbox_policy: &read_only_file_system_sandbox_policy(),
sandbox_permissions: SandboxPermissions::UseDefault,
enable_shell_wrapper_parsing: false,
},
);
assert_eq!(
@@ -543,10 +568,13 @@ host_executable(name = "git", paths = ["{allowed_git_literal}"])
&policy,
&program,
&["git".to_string(), "status".to_string()],
AskForApproval::OnRequest,
&SandboxPolicy::new_read_only_policy(),
SandboxPermissions::UseDefault,
false,
InterceptedExecPolicyContext {
approval_policy: AskForApproval::OnRequest,
sandbox_policy: &SandboxPolicy::new_read_only_policy(),
file_system_sandbox_policy: &read_only_file_system_sandbox_policy(),
sandbox_permissions: SandboxPermissions::UseDefault,
enable_shell_wrapper_parsing: false,
},
);
assert!(matches!(
@@ -571,9 +599,7 @@ async fn prepare_escalated_exec_turn_default_preserves_macos_seatbelt_extensions
network: None,
sandbox: SandboxType::None,
sandbox_policy: SandboxPolicy::new_read_only_policy(),
file_system_sandbox_policy: FileSystemSandboxPolicy::from(
&SandboxPolicy::new_read_only_policy(),
),
file_system_sandbox_policy: read_only_file_system_sandbox_policy(),
network_sandbox_policy: NetworkSandboxPolicy::Restricted,
windows_sandbox_level: WindowsSandboxLevel::Disabled,
sandbox_permissions: SandboxPermissions::UseDefault,
@@ -625,7 +651,7 @@ async fn prepare_escalated_exec_permissions_preserve_macos_seatbelt_extensions()
network: None,
sandbox: SandboxType::None,
sandbox_policy: SandboxPolicy::DangerFullAccess,
file_system_sandbox_policy: FileSystemSandboxPolicy::from(&SandboxPolicy::DangerFullAccess),
file_system_sandbox_policy: unrestricted_file_system_sandbox_policy(),
network_sandbox_policy: NetworkSandboxPolicy::Enabled,
windows_sandbox_level: WindowsSandboxLevel::Disabled,
sandbox_permissions: SandboxPermissions::UseDefault,
@@ -640,9 +666,7 @@ async fn prepare_escalated_exec_permissions_preserve_macos_seatbelt_extensions()
let permissions = Permissions {
approval_policy: Constrained::allow_any(AskForApproval::Never),
sandbox_policy: Constrained::allow_any(SandboxPolicy::new_read_only_policy()),
file_system_sandbox_policy: codex_protocol::permissions::FileSystemSandboxPolicy::from(
&SandboxPolicy::new_read_only_policy(),
),
file_system_sandbox_policy: read_only_file_system_sandbox_policy(),
network_sandbox_policy: codex_protocol::permissions::NetworkSandboxPolicy::Restricted,
network: None,
allow_login_shell: true,
@@ -701,7 +725,7 @@ async fn prepare_escalated_exec_permission_profile_unions_turn_and_requested_mac
network: None,
sandbox: SandboxType::None,
sandbox_policy: sandbox_policy.clone(),
file_system_sandbox_policy: FileSystemSandboxPolicy::from(&sandbox_policy),
file_system_sandbox_policy: read_only_file_system_sandbox_policy(),
network_sandbox_policy: NetworkSandboxPolicy::from(&sandbox_policy),
windows_sandbox_level: WindowsSandboxLevel::Disabled,
sandbox_permissions: SandboxPermissions::UseDefault,
codex-rs/core/src/tools/sandboxing.rs
+27 -15
View File
@@ -7,6 +7,7 @@
use crate::codex::Session;
use crate::codex::TurnContext;
use crate::error::CodexErr;
#[cfg(test)]
use crate::protocol::SandboxPolicy;
use crate::sandboxing::CommandSpec;
use crate::sandboxing::SandboxManager;
@@ -17,6 +18,7 @@ use crate::tools::network_approval::NetworkApprovalSpec;
use codex_network_proxy::NetworkProxy;
use codex_protocol::approvals::ExecPolicyAmendment;
use codex_protocol::approvals::NetworkApprovalContext;
use codex_protocol::permissions::FileSystemSandboxKind;
use codex_protocol::permissions::FileSystemSandboxPolicy;
use codex_protocol::permissions::NetworkSandboxPolicy;
use codex_protocol::protocol::AskForApproval;
@@ -158,20 +160,22 @@ impl ExecApprovalRequirement {
}
/// - Never, OnFailure: do not ask
/// - OnRequest: ask unless sandbox policy is DangerFullAccess
/// - Reject: ask unless sandbox policy is DangerFullAccess, but auto-reject
/// - OnRequest: ask unless filesystem access is unrestricted
/// - Reject: ask unless filesystem access is unrestricted, but auto-reject
/// when `sandbox_approval` rejection is enabled.
/// - UnlessTrusted: always ask
pub(crate) fn default_exec_approval_requirement(
policy: AskForApproval,
sandbox_policy: &SandboxPolicy,
file_system_sandbox_policy: &FileSystemSandboxPolicy,
) -> ExecApprovalRequirement {
let needs_approval = match policy {
AskForApproval::Never | AskForApproval::OnFailure => false,
AskForApproval::OnRequest | AskForApproval::Reject(_) => !matches!(
sandbox_policy,
SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. }
),
AskForApproval::OnRequest | AskForApproval::Reject(_) => {
matches!(
file_system_sandbox_policy.kind,
FileSystemSandboxKind::Restricted
)
}
AskForApproval::UnlessTrusted => true,
};
@@ -365,12 +369,13 @@ mod tests {
#[test]
fn external_sandbox_skips_exec_approval_on_request() {
let sandbox_policy = SandboxPolicy::ExternalSandbox {
network_access: NetworkAccess::Restricted,
};
assert_eq!(
default_exec_approval_requirement(
AskForApproval::OnRequest,
&SandboxPolicy::ExternalSandbox {
network_access: NetworkAccess::Restricted,
},
&FileSystemSandboxPolicy::from(&sandbox_policy),
),
ExecApprovalRequirement::Skip {
bypass_sandbox: false,
@@ -381,10 +386,11 @@ mod tests {
#[test]
fn restricted_sandbox_requires_exec_approval_on_request() {
let sandbox_policy = SandboxPolicy::new_read_only_policy();
assert_eq!(
default_exec_approval_requirement(
AskForApproval::OnRequest,
&SandboxPolicy::new_read_only_policy()
&FileSystemSandboxPolicy::from(&sandbox_policy)
),
ExecApprovalRequirement::NeedsApproval {
reason: None,
@@ -403,8 +409,11 @@ mod tests {
mcp_elicitations: false,
});
let requirement =
default_exec_approval_requirement(policy, &SandboxPolicy::new_read_only_policy());
let sandbox_policy = SandboxPolicy::new_read_only_policy();
let requirement = default_exec_approval_requirement(
policy,
&FileSystemSandboxPolicy::from(&sandbox_policy),
);
assert_eq!(
requirement,
@@ -424,8 +433,11 @@ mod tests {
mcp_elicitations: true,
});
let requirement =
default_exec_approval_requirement(policy, &SandboxPolicy::new_read_only_policy());
let sandbox_policy = SandboxPolicy::new_read_only_policy();
let requirement = default_exec_approval_requirement(
policy,
&FileSystemSandboxPolicy::from(&sandbox_policy),
);
assert_eq!(
requirement,