From c2d5458d67714b9d92dfe18d8ebbb512001252d9 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Wed, 11 Mar 2026 19:23:22 -0700 Subject: [PATCH] fix: align core approvals with split sandbox policies (#14171) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- codex-rs/core/src/config/config_tests.rs | 72 +++++++++++++++ codex-rs/core/src/exec_policy.rs | 89 +++++++++++++++++-- codex-rs/core/src/safety.rs | 18 +--- codex-rs/core/src/tools/handlers/shell.rs | 1 + codex-rs/core/src/tools/orchestrator.rs | 4 +- .../tools/runtimes/shell/unix_escalation.rs | 36 ++++++-- .../runtimes/shell/unix_escalation_tests.rs | 72 ++++++++++----- codex-rs/core/src/tools/sandboxing.rs | 42 +++++---- .../core/src/unified_exec/process_manager.rs | 1 + 9 files changed, 259 insertions(+), 76 deletions(-) diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 5549a3414..c6d92e74b 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -17,7 +17,9 @@ use codex_config::CONFIG_TOML_FILE; 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 serde::Deserialize; use tempfile::tempdir; @@ -1044,6 +1046,76 @@ trust_level = "trusted" } } +#[test] +fn legacy_sandbox_mode_config_builds_split_policies_without_drift() -> std::io::Result<()> { + let codex_home = TempDir::new()?; + let cwd = TempDir::new()?; + let extra_root = test_absolute_path("/tmp/legacy-extra-root"); + let cases = vec![ + ( + "danger-full-access".to_string(), + r#"sandbox_mode = "danger-full-access" +"# + .to_string(), + ), + ( + "read-only".to_string(), + r#"sandbox_mode = "read-only" +"# + .to_string(), + ), + ( + "workspace-write".to_string(), + format!( + r#"sandbox_mode = "workspace-write" + +[sandbox_workspace_write] +writable_roots = [{}] +exclude_tmpdir_env_var = true +exclude_slash_tmp = true +"#, + serde_json::json!(extra_root) + ), + ), + ]; + + for (name, config_toml) in cases { + let cfg = toml::from_str::(&config_toml) + .unwrap_or_else(|err| panic!("case `{name}` should parse: {err}")); + let config = Config::load_from_base_config_with_overrides( + cfg, + ConfigOverrides { + cwd: Some(cwd.path().to_path_buf()), + ..Default::default() + }, + codex_home.path().to_path_buf(), + )?; + + let sandbox_policy = config.permissions.sandbox_policy.get(); + assert_eq!( + config.permissions.file_system_sandbox_policy, + FileSystemSandboxPolicy::from_legacy_sandbox_policy(sandbox_policy, cwd.path()), + "case `{name}` should preserve filesystem semantics from legacy config" + ); + assert_eq!( + config.permissions.network_sandbox_policy, + NetworkSandboxPolicy::from(sandbox_policy), + "case `{name}` should preserve network semantics from legacy config" + ); + assert_eq!( + config + .permissions + .file_system_sandbox_policy + .to_legacy_sandbox_policy(config.permissions.network_sandbox_policy, cwd.path()) + .unwrap_or_else(|err| panic!("case `{name}` should round-trip: {err}")), + sandbox_policy.clone(), + "case `{name}` should round-trip through split policies without drift" + ); + } + + Ok(()) +} + #[test] fn filter_mcp_servers_by_allowlist_enforces_identity_rules() { const MISMATCHED_COMMAND_SERVER: &str = "mismatched-command-should-disable"; diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index fc136fba0..830fbb2ce 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -21,6 +21,8 @@ use codex_execpolicy::RuleMatch; use codex_execpolicy::blocking_append_allow_prefix_rule; use codex_execpolicy::blocking_append_network_rule; use codex_protocol::approvals::ExecPolicyAmendment; +use codex_protocol::permissions::FileSystemSandboxKind; +use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::SandboxPolicy; use thiserror::Error; @@ -173,6 +175,7 @@ pub(crate) struct ExecApprovalRequest<'a> { pub(crate) command: &'a [String], pub(crate) approval_policy: AskForApproval, pub(crate) sandbox_policy: &'a SandboxPolicy, + pub(crate) file_system_sandbox_policy: &'a FileSystemSandboxPolicy, pub(crate) sandbox_permissions: SandboxPermissions, pub(crate) prefix_rule: Option>, } @@ -204,6 +207,7 @@ impl ExecPolicyManager { command, approval_policy, sandbox_policy, + file_system_sandbox_policy, sandbox_permissions, prefix_rule, } = req; @@ -217,6 +221,7 @@ impl ExecPolicyManager { render_decision_for_unmatched_command( approval_policy, sandbox_policy, + file_system_sandbox_policy, cmd, sandbox_permissions, used_complex_parsing, @@ -488,6 +493,7 @@ pub async fn load_exec_policy(config_stack: &ConfigLayerStack) -> Result { - match sandbox_policy { - SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => { + match file_system_sandbox_policy.kind { + FileSystemSandboxKind::Unrestricted | FileSystemSandboxKind::ExternalSandbox => { // The user has indicated we should "just run" commands // in their unrestricted environment, so we do so since the // command has not been flagged as dangerous. Decision::Allow } - SandboxPolicy::ReadOnly { .. } | SandboxPolicy::WorkspaceWrite { .. } => { - // In restricted sandboxes (ReadOnly/WorkspaceWrite), do not prompt for - // non‑escalated, non‑dangerous commands — let the sandbox enforce - // restrictions (e.g., block network/write) without a user prompt. + FileSystemSandboxKind::Restricted => { + // In restricted sandboxes, do not prompt for non-escalated, + // non-dangerous commands; let the sandbox enforce + // restrictions without a user prompt. if sandbox_permissions.requests_sandbox_override() { Decision::Prompt } else { @@ -548,13 +554,13 @@ pub fn render_decision_for_unmatched_command( } } } - AskForApproval::Reject(_) => match sandbox_policy { - SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => { + AskForApproval::Reject(_) => match file_system_sandbox_policy.kind { + FileSystemSandboxKind::Unrestricted | FileSystemSandboxKind::ExternalSandbox => { // Mirror on-request behavior for unmatched commands; prompt-vs-reject is handled // by `prompt_is_rejected_by_policy`. Decision::Allow } - SandboxPolicy::ReadOnly { .. } | SandboxPolicy::WorkspaceWrite { .. } => { + FileSystemSandboxKind::Restricted => { if sandbox_permissions.requests_sandbox_override() { Decision::Prompt } else { @@ -824,6 +830,10 @@ mod tests { use crate::config_loader::ConfigRequirements; use crate::config_loader::ConfigRequirementsToml; use codex_app_server_protocol::ConfigLayerSource; + use codex_protocol::permissions::FileSystemAccessMode; + use codex_protocol::permissions::FileSystemPath; + use codex_protocol::permissions::FileSystemSandboxEntry; + use codex_protocol::permissions::FileSystemSpecialPath; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::RejectConfig; use codex_protocol::protocol::SandboxPolicy; @@ -876,6 +886,19 @@ mod tests { value.replace('\\', "\\\\").replace('"', "\\\"") } + fn read_only_file_system_sandbox_policy() -> FileSystemSandboxPolicy { + FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, + }]) + } + + fn unrestricted_file_system_sandbox_policy() -> FileSystemSandboxPolicy { + FileSystemSandboxPolicy::unrestricted() + } + #[tokio::test] async fn returns_empty_policy_when_no_policy_files_exist() { let temp_dir = tempdir().expect("create temp dir"); @@ -1234,6 +1257,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") command: &forbidden_script, approval_policy: AskForApproval::OnRequest, sandbox_policy: &SandboxPolicy::DangerFullAccess, + file_system_sandbox_policy: &unrestricted_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }) @@ -1284,6 +1308,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") command: &command, 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, prefix_rule: None, }) @@ -1311,6 +1336,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") command: &command, approval_policy: AskForApproval::UnlessTrusted, sandbox_policy: &SandboxPolicy::new_read_only_policy(), + file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }) @@ -1339,6 +1365,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") command: &command, approval_policy: AskForApproval::UnlessTrusted, sandbox_policy: &SandboxPolicy::new_read_only_policy(), + file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: Some(requested_prefix.clone()), }) @@ -1378,6 +1405,7 @@ prefix_rule( ], approval_policy: AskForApproval::OnRequest, sandbox_policy: &SandboxPolicy::DangerFullAccess, + file_system_sandbox_policy: &unrestricted_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }) @@ -1407,6 +1435,7 @@ prefix_rule( command: &command, approval_policy: AskForApproval::OnRequest, sandbox_policy: &SandboxPolicy::DangerFullAccess, + file_system_sandbox_policy: &unrestricted_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }) @@ -1443,6 +1472,7 @@ prefix_rule(pattern=["git"], decision="allow") command: &command, approval_policy: AskForApproval::UnlessTrusted, sandbox_policy: &SandboxPolicy::new_read_only_policy(), + file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }) @@ -1485,6 +1515,7 @@ prefix_rule(pattern=["git"], decision="prompt") command: &command, approval_policy: AskForApproval::UnlessTrusted, sandbox_policy: &SandboxPolicy::new_read_only_policy(), + file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }) @@ -1513,6 +1544,7 @@ prefix_rule(pattern=["git"], decision="prompt") command: &command, approval_policy: AskForApproval::UnlessTrusted, sandbox_policy: &SandboxPolicy::new_read_only_policy(), + file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: Some(vec!["cargo".to_string(), "install".to_string()]), }) @@ -1546,6 +1578,7 @@ prefix_rule(pattern=["git"], decision="prompt") command: &command, approval_policy: AskForApproval::Never, sandbox_policy: &SandboxPolicy::DangerFullAccess, + file_system_sandbox_policy: &unrestricted_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }) @@ -1574,6 +1607,25 @@ prefix_rule(pattern=["git"], decision="prompt") mcp_elicitations: false, }), &SandboxPolicy::new_read_only_policy(), + &read_only_file_system_sandbox_policy(), + &command, + SandboxPermissions::RequireEscalated, + false, + ) + ); + } + + #[test] + fn unmatched_on_request_uses_split_filesystem_policy_for_escalation_prompts() { + let command = vec!["madeup-cmd".to_string()]; + let restricted_file_system_policy = FileSystemSandboxPolicy::restricted(vec![]); + + assert_eq!( + Decision::Prompt, + render_decision_for_unmatched_command( + AskForApproval::OnRequest, + &SandboxPolicy::DangerFullAccess, + &restricted_file_system_policy, &command, SandboxPermissions::RequireEscalated, false, @@ -1597,6 +1649,7 @@ prefix_rule(pattern=["git"], decision="prompt") mcp_elicitations: false, }), sandbox_policy: &SandboxPolicy::new_read_only_policy(), + file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::RequireEscalated, prefix_rule: None, }) @@ -1635,6 +1688,7 @@ prefix_rule(pattern=["git"], decision="prompt") mcp_elicitations: false, }), sandbox_policy: &SandboxPolicy::new_read_only_policy(), + file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::RequireEscalated, prefix_rule: None, }) @@ -1671,6 +1725,7 @@ prefix_rule(pattern=["git"], decision="prompt") mcp_elicitations: false, }), sandbox_policy: &SandboxPolicy::new_read_only_policy(), + file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::RequireEscalated, prefix_rule: None, }) @@ -1694,6 +1749,7 @@ prefix_rule(pattern=["git"], decision="prompt") command: &command, approval_policy: AskForApproval::UnlessTrusted, sandbox_policy: &SandboxPolicy::new_read_only_policy(), + file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }) @@ -1718,6 +1774,7 @@ prefix_rule(pattern=["git"], decision="prompt") command: &command, approval_policy: AskForApproval::UnlessTrusted, sandbox_policy: &SandboxPolicy::new_read_only_policy(), + file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }) @@ -1746,6 +1803,7 @@ prefix_rule(pattern=["git"], decision="prompt") command: &command, approval_policy: AskForApproval::UnlessTrusted, sandbox_policy: &SandboxPolicy::new_read_only_policy(), + file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }) @@ -1774,6 +1832,7 @@ prefix_rule(pattern=["git"], decision="prompt") command: &command, approval_policy: AskForApproval::OnRequest, sandbox_policy: &SandboxPolicy::new_read_only_policy(), + file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::RequireEscalated, prefix_rule: Some(vec!["cargo".to_string(), "install".to_string()]), }) @@ -1805,6 +1864,7 @@ prefix_rule(pattern=["git"], decision="prompt") command: &command, approval_policy: AskForApproval::OnRequest, sandbox_policy: &SandboxPolicy::DangerFullAccess, + file_system_sandbox_policy: &unrestricted_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::RequireEscalated, prefix_rule: Some(vec!["cargo".to_string(), "install".to_string()]), }) @@ -1843,6 +1903,7 @@ prefix_rule(pattern=["git"], decision="prompt") command: &command, approval_policy: AskForApproval::UnlessTrusted, sandbox_policy: &SandboxPolicy::DangerFullAccess, + file_system_sandbox_policy: &unrestricted_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }) @@ -1917,6 +1978,7 @@ prefix_rule(pattern=["git"], decision="prompt") command: &command, approval_policy: AskForApproval::UnlessTrusted, sandbox_policy: &SandboxPolicy::new_read_only_policy(), + file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }) @@ -1947,6 +2009,7 @@ prefix_rule(pattern=["git"], decision="prompt") command: &command, approval_policy: AskForApproval::OnRequest, sandbox_policy: &SandboxPolicy::DangerFullAccess, + file_system_sandbox_policy: &unrestricted_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }) @@ -1974,6 +2037,7 @@ prefix_rule(pattern=["git"], decision="prompt") command: &command, approval_policy: AskForApproval::UnlessTrusted, sandbox_policy: &SandboxPolicy::new_read_only_policy(), + file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }) @@ -2012,6 +2076,7 @@ prefix_rule(pattern=["git"], decision="prompt") command: &command, approval_policy: AskForApproval::UnlessTrusted, sandbox_policy: &SandboxPolicy::new_read_only_policy(), + file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }) @@ -2035,6 +2100,7 @@ prefix_rule(pattern=["git"], decision="prompt") command: &command, 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, prefix_rule: None, }) @@ -2065,6 +2131,7 @@ prefix_rule(pattern=["git"], decision="prompt") command: &command, 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, prefix_rule: None, }) @@ -2238,6 +2305,7 @@ prefix_rule(pattern=["git"], decision="prompt") command: &command, approval_policy: AskForApproval::OnRequest, sandbox_policy: &SandboxPolicy::DangerFullAccess, + file_system_sandbox_policy: &unrestricted_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }) @@ -2304,6 +2372,7 @@ prefix_rule(pattern=["git"], decision="prompt") command: &sneaky_command, approval_policy: AskForApproval::OnRequest, sandbox_policy: &SandboxPolicy::new_read_only_policy(), + file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_permissions: permissions, prefix_rule: None, }) @@ -2327,6 +2396,7 @@ prefix_rule(pattern=["git"], decision="prompt") command: &dangerous_command, approval_policy: AskForApproval::OnRequest, sandbox_policy: &SandboxPolicy::new_read_only_policy(), + file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_permissions: permissions, prefix_rule: None, }) @@ -2346,6 +2416,7 @@ prefix_rule(pattern=["git"], decision="prompt") command: &dangerous_command, approval_policy: AskForApproval::Never, sandbox_policy: &SandboxPolicy::new_read_only_policy(), + file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_permissions: permissions, prefix_rule: None, }) diff --git a/codex-rs/core/src/safety.rs b/codex-rs/core/src/safety.rs index 6d97e7cd6..d9b5368fc 100644 --- a/codex-rs/core/src/safety.rs +++ b/codex-rs/core/src/safety.rs @@ -143,9 +143,6 @@ fn is_write_patch_constrained_to_writable_paths( Some(out) } - let unreadable_roots = file_system_sandbox_policy.get_unreadable_roots_with_cwd(cwd); - let writable_roots = file_system_sandbox_policy.get_writable_roots_with_cwd(cwd); - // Determine whether `path` is inside **any** writable root. Both `path` // and roots are converted to absolute, normalized forms before the // prefix check. @@ -156,20 +153,7 @@ fn is_write_patch_constrained_to_writable_paths( None => return false, }; - if unreadable_roots - .iter() - .any(|root| abs.starts_with(root.as_path())) - { - return false; - } - - if file_system_sandbox_policy.has_full_disk_write_access() { - return true; - } - - writable_roots - .iter() - .any(|writable_root| writable_root.is_path_writable(&abs)) + file_system_sandbox_policy.can_write_path_with_cwd(&abs, cwd) }; for (path, change) in action.changes() { diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 8c8d668f8..26b04af9e 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -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 { diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index 73c2da3bd..b26ea0bee 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -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 { .. } ); diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index 35a4e332e..f0be71210 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -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>, used_complex_parsing: bool, diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs index 02779650f..4f9f31a78 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs @@ -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) -> 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, diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index fef4fa373..922544471 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -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, diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 29311b1ff..f2c0f7d31 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -585,6 +585,7 @@ impl UnifiedExecProcessManager { command: &request.command, approval_policy: context.turn.approval_policy.value(), sandbox_policy: context.turn.sandbox_policy.get(), + file_system_sandbox_policy: &context.turn.file_system_sandbox_policy, sandbox_permissions: if request.additional_permissions_preapproved { crate::sandboxing::SandboxPermissions::UseDefault } else {