From fe140d4c8e7d47950d4d2e35ff7c58e55b744f65 Mon Sep 17 00:00:00 2001 From: iceweasel-oai Date: Wed, 2 Sep 2026 22:33:13 +0000 Subject: [PATCH] Authorize `apply_patch` in the executor path context (#42391) ## Why Patch targets can use a different path convention from the Codex host, so host-native path conversion can misclassify writable roots and requested permissions. ## What changed - Evaluate patch targets as `PathUri` values with the active filesystem policy context, including workspace roots and the executor's path convention. - Distinguish executor-managed sandboxing from local platform sandboxing when deciding whether a patch can be auto-approved and how to normalize additional write permissions. - Make full-disk and special-path policy checks honor the selected executor's Windows or POSIX convention. ## Testing Add coverage for Windows executor URIs, full-disk policy aliases, remote patch permission requests, sandbox availability, and owner-provided workspace roots. GitOrigin-RevId: 1a054ea443efd342623c67432762f85c53d20c15 --- codex-rs/core/src/apply_patch.rs | 8 +- codex-rs/core/src/apply_patch_tests.rs | 13 +- codex-rs/core/src/safety.rs | 107 ++++--------- codex-rs/core/src/safety_tests.rs | 150 +++++++++++++++--- .../core/src/tools/handlers/apply_patch.rs | 122 +++++++------- .../src/tools/handlers/apply_patch_tests.rs | 71 ++++++++- codex-rs/core/tests/suite/workspace_roots.rs | 6 +- codex-rs/protocol/src/permissions.rs | 114 +++++++++---- 8 files changed, 389 insertions(+), 202 deletions(-) diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index 1283d0cd9c..9b3c7a2d23 100644 --- a/codex-rs/core/src/apply_patch.rs +++ b/codex-rs/core/src/apply_patch.rs @@ -1,4 +1,5 @@ use crate::function_tool::FunctionCallError; +use crate::safety::PatchSandboxRoute; use crate::safety::SafetyCheck; use crate::safety::assess_patch_safety; use crate::session::step_context::StepContext; @@ -6,6 +7,7 @@ use crate::session::turn_context::TurnEnvironment; use crate::tools::sandboxing::ExecApprovalRequirement; use codex_apply_patch::ApplyPatchAction; use codex_apply_patch::ApplyPatchFileChange; +use codex_protocol::permissions::FileSystemSandboxPolicyContext; use codex_protocol::protocol::FileChange; use codex_protocol::protocol::FileSystemSandboxPolicy; use codex_utils_path_uri::PathUri; @@ -23,6 +25,8 @@ pub(crate) fn prepare_apply_patch( step_context: &StepContext, turn_environment: &TurnEnvironment, file_system_sandbox_policy: &FileSystemSandboxPolicy, + context: &FileSystemSandboxPolicyContext<'_>, + sandbox_route: PatchSandboxRoute, action: ApplyPatchAction, ) -> Result { match assess_patch_safety( @@ -30,8 +34,8 @@ pub(crate) fn prepare_apply_patch( step_context.settings.approval_policy(), turn_environment.permission_profile(), file_system_sandbox_policy, - &action.cwd, - turn_environment.config().windows_sandbox_level, + context, + sandbox_route, ) { SafetyCheck::AutoApprove => Ok(ApplyPatchRuntimeInvocation { action, diff --git a/codex-rs/core/src/apply_patch_tests.rs b/codex-rs/core/src/apply_patch_tests.rs index 578873d3c6..275b2a2da7 100644 --- a/codex-rs/core/src/apply_patch_tests.rs +++ b/codex-rs/core/src/apply_patch_tests.rs @@ -58,9 +58,18 @@ async fn prepare_apply_patch_uses_action_policy_before_turn_policy() { .clone(); environment.config_mut().permission_profile = PermissionProfileSnapshot::legacy(permission_profile); + let sandbox = environment.sandbox_context(/*additional_permissions*/ None); + let context = sandbox.policy_context().expect("local sandbox context"); - let prepared = prepare_apply_patch(&step, &environment, &file_system_policy, action) - .expect("issuing action policy should request approval"); + let prepared = prepare_apply_patch( + &step, + &environment, + &file_system_policy, + &context, + PatchSandboxRoute::Platform(codex_protocol::config_types::WindowsSandboxLevel::Disabled), + action, + ) + .expect("issuing action policy should request approval"); assert!(!prepared.auto_approved); assert!(matches!( diff --git a/codex-rs/core/src/safety.rs b/codex-rs/core/src/safety.rs index 7c93938c71..bf16a9c247 100644 --- a/codex-rs/core/src/safety.rs +++ b/codex-rs/core/src/safety.rs @@ -1,12 +1,9 @@ -use std::path::Component; -use std::path::Path; -use std::path::PathBuf; - use codex_apply_patch::ApplyPatchAction; use codex_apply_patch::ApplyPatchFileChange; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::FileSystemSandboxPolicyContext; use codex_protocol::protocol::AskForApproval; use codex_sandboxing::get_platform_sandbox; use codex_utils_path_uri::PathUri; @@ -23,13 +20,19 @@ pub enum SafetyCheck { Reject { reason: String }, } +#[derive(Debug, Clone, Copy)] +pub(crate) enum PatchSandboxRoute { + ExecutorManaged, + Platform(WindowsSandboxLevel), +} + pub fn assess_patch_safety( action: &ApplyPatchAction, policy: AskForApproval, permission_profile: &PermissionProfile, file_system_sandbox_policy: &FileSystemSandboxPolicy, - cwd: &PathUri, - windows_sandbox_level: WindowsSandboxLevel, + context: &FileSystemSandboxPolicyContext<'_>, + sandbox_route: PatchSandboxRoute, ) -> SafetyCheck { if action.is_empty() { return SafetyCheck::Reject { @@ -53,43 +56,27 @@ pub fn assess_patch_safety( policy, AskForApproval::Granular(granular_config) if !granular_config.sandbox_approval ); + let sandbox_available = match sandbox_route { + PatchSandboxRoute::ExecutorManaged => true, + PatchSandboxRoute::Platform(windows_sandbox_level) => { + get_platform_sandbox(windows_sandbox_level != WindowsSandboxLevel::Disabled).is_some() + } + }; // Even though the patch appears to be constrained to writable paths, it is // possible that paths in the patch are hard links to files outside the // writable roots, so we should still run `apply_patch` in a sandbox in that case. - if is_write_patch_constrained_to_writable_paths(action, file_system_sandbox_policy, cwd) { - if matches!( + // Disabled and External profiles intentionally do not apply an outer sandbox. + if is_write_patch_constrained_to_writable_paths(action, file_system_sandbox_policy, context) + && (matches!( permission_profile, PermissionProfile::Disabled | PermissionProfile::External { .. } - ) { - // Disabled and External profiles intentionally do not apply an - // outer Codex filesystem sandbox. - SafetyCheck::AutoApprove - } else { - // Only auto‑approve when we can actually enforce a sandbox. Otherwise - // fall back to asking the user because the patch may touch arbitrary - // paths outside the project. - match get_platform_sandbox(windows_sandbox_level != WindowsSandboxLevel::Disabled) { - Some(_) => SafetyCheck::AutoApprove, - None => { - if rejects_sandbox_approval { - SafetyCheck::Reject { - reason: patch_rejection_reason( - permission_profile, - file_system_sandbox_policy, - cwd, - ) - .to_string(), - } - } else { - SafetyCheck::AskUser - } - } - } - } + ) || sandbox_available) + { + SafetyCheck::AutoApprove } else if rejects_sandbox_approval { SafetyCheck::Reject { - reason: patch_rejection_reason(permission_profile, file_system_sandbox_policy, cwd) + reason: patch_rejection_reason(permission_profile, file_system_sandbox_policy, context) .to_string(), } } else { @@ -100,14 +87,12 @@ pub fn assess_patch_safety( fn patch_rejection_reason( permission_profile: &PermissionProfile, file_system_sandbox_policy: &FileSystemSandboxPolicy, - cwd: &PathUri, + context: &FileSystemSandboxPolicyContext<'_>, ) -> &'static str { - let has_no_writable_roots = cwd - .to_abs_path() - .is_ok_and(|cwd| !file_system_sandbox_policy.has_writable_roots_with_cwd(cwd.as_path())); + let has_no_writable_roots = !file_system_sandbox_policy.has_configured_writable_roots(context); match permission_profile { PermissionProfile::Managed { .. } - if !file_system_sandbox_policy.has_full_disk_write_access() + if !file_system_sandbox_policy.has_full_disk_write_access_with_context(context) && has_no_writable_roots => { PATCH_REJECTED_READ_ONLY_REASON @@ -121,49 +106,15 @@ fn patch_rejection_reason( fn is_write_patch_constrained_to_writable_paths( action: &ApplyPatchAction, file_system_sandbox_policy: &FileSystemSandboxPolicy, - cwd: &PathUri, + context: &FileSystemSandboxPolicyContext<'_>, ) -> bool { // A full-disk policy permits every patch target, so no per-path writable-root check can // further constrain the result. - if file_system_sandbox_policy.has_full_disk_write_access() { + if file_system_sandbox_policy.has_full_disk_write_access_with_context(context) { return true; } - // TODO(anp): Make filesystem sandbox policies operate on PathUri. - let Ok(native_cwd) = cwd.to_abs_path() else { - return false; - }; - // Normalize a path by removing `.` and resolving `..` without touching the - // filesystem (works even if the file does not exist). - fn normalize(path: &Path) -> Option { - let mut out = PathBuf::new(); - for comp in path.components() { - match comp { - Component::ParentDir => { - out.pop(); - } - Component::CurDir => { /* skip */ } - other => out.push(other.as_os_str()), - } - } - Some(out) - } - - // Determine whether `path` is inside **any** writable root. Both `path` - // and roots are converted to absolute, normalized forms before the - // prefix check. - let is_path_writable = |path: &PathUri| { - // TODO(anp): Make sandbox policy path checks accept PathUri without host projection. - let Ok(path) = path.to_abs_path() else { - return false; - }; - let abs = path.into_path_buf(); - let abs = match normalize(&abs) { - Some(v) => v, - None => return false, - }; - - file_system_sandbox_policy.can_write_path_with_cwd(&abs, &native_cwd) - }; + let is_path_writable = + |path: &PathUri| file_system_sandbox_policy.can_write_path(path, context); for (path, change) in action.changes() { match change { diff --git a/codex-rs/core/src/safety_tests.rs b/codex-rs/core/src/safety_tests.rs index cf947d8d2b..6539543499 100644 --- a/codex-rs/core/src/safety_tests.rs +++ b/codex-rs/core/src/safety_tests.rs @@ -1,5 +1,6 @@ use super::*; use codex_protocol::models::PermissionProfile; +use codex_protocol::permissions::FileSystemSandboxPolicyContext; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::FileSystemAccessMode; use codex_protocol::protocol::FileSystemPath; @@ -12,6 +13,109 @@ use core_test_support::PathExt; use pretty_assertions::assert_eq; use tempfile::TempDir; +fn local_context(cwd: &PathUri) -> FileSystemSandboxPolicyContext<'_> { + FileSystemSandboxPolicyContext { + cwd, + workspace_roots: std::slice::from_ref(cwd), + user_home_dir: None, + temporary_directories: None, + } +} + +#[test] +fn windows_patch_matching_is_uri_native() { + let cwd = PathUri::parse("file:///C:/workspace").expect("Windows cwd"); + let context = local_context(&cwd); + let permission_profile = PermissionProfile::workspace_write(); + let policy = permission_profile.file_system_sandbox_policy(); + let inside = ApplyPatchAction::new_add_for_test( + &PathUri::parse("file:///C:/workspace/in.txt").expect("inside"), + String::new(), + ); + let outside = ApplyPatchAction::new_add_for_test( + &PathUri::parse("file:///C:/outside.txt").expect("outside"), + String::new(), + ); + + assert!(!is_write_patch_constrained_to_writable_paths( + &outside, &policy, &context, + )); + assert_eq!( + assess_patch_safety( + &inside, + AskForApproval::Never, + &permission_profile, + &policy, + &context, + PatchSandboxRoute::ExecutorManaged, + ), + SafetyCheck::AutoApprove, + ); +} + +#[test] +fn full_disk_write_uses_executor_path_convention() { + use FileSystemAccessMode::Deny; + use FileSystemAccessMode::Read; + use FileSystemAccessMode::Write; + use FileSystemSpecialPath::Root; + use FileSystemSpecialPath::SlashTmp; + + for (root, full_disk_write) in [("file:///", false), ("file:///C:/", true)] { + let cwd = PathUri::parse(root).expect("executor root"); + let context = local_context(&cwd); + let mut policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry::new(FileSystemPath::Special { value: Root }, Write), + FileSystemSandboxEntry::new(FileSystemPath::Special { value: SlashTmp }, Deny), + ]); + let action = ApplyPatchAction::new_add_for_test( + &cwd.join("tmp/blocked.txt").expect("patch target"), + String::new(), + ); + + assert_eq!( + policy.has_full_disk_write_access_with_context(&context), + full_disk_write + ); + assert_eq!( + assess_patch_safety( + &action, + AskForApproval::OnRequest, + &PermissionProfile::from_runtime_permissions( + &policy, + NetworkSandboxPolicy::Restricted + ), + &policy, + &context, + PatchSandboxRoute::ExecutorManaged, + ), + if full_disk_write { + SafetyCheck::AutoApprove + } else { + SafetyCheck::AskUser + }, + ); + + // Literal aliases can override equally specific read grants, but never denies. + policy.entries[1].access = Read; + policy.entries.push(FileSystemSandboxEntry::new( + cwd.join("tmp").expect("tmp alias").into(), + Write, + )); + assert!(policy.has_full_disk_write_access_with_context(&context)); + policy.entries[1].access = Deny; + assert_eq!( + policy.has_full_disk_write_access_with_context(&context), + full_disk_write + ); + policy.entries[1].path = cwd.clone().into(); + policy.entries[1].access = Read; + assert!(policy.has_full_disk_write_access_with_context(&context)); + policy.entries[1].access = Deny; + assert!(!policy.has_full_disk_write_access_with_context(&context)); + } +} + #[test] fn test_writable_roots_constraint() { // Use a temporary directory as our workspace to avoid touching @@ -40,13 +144,13 @@ fn test_writable_roots_constraint() { assert!(is_write_patch_constrained_to_writable_paths( &add_inside, &workspace_only_file_system_policy, - &cwd_uri, + &local_context(&cwd_uri), )); assert!(!is_write_patch_constrained_to_writable_paths( &add_outside, &workspace_only_file_system_policy, - &cwd_uri, + &local_context(&cwd_uri), )); // With the parent dir explicitly added as a writable root, the @@ -59,7 +163,7 @@ fn test_writable_roots_constraint() { assert!(is_write_patch_constrained_to_writable_paths( &add_outside, &file_system_policy_with_parent, - &cwd_uri, + &local_context(&cwd_uri), )); } @@ -85,8 +189,8 @@ fn external_sandbox_auto_approves_in_on_request() { AskForApproval::OnRequest, &permission_profile, &file_system_sandbox_policy, - &cwd_uri, - WindowsSandboxLevel::Disabled + &local_context(&cwd_uri), + PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled) ), SafetyCheck::AutoApprove ); @@ -115,8 +219,8 @@ fn granular_with_all_flags_true_matches_on_request_for_out_of_root_patch() { AskForApproval::OnRequest, &permission_profile, &file_system_sandbox_policy, - &cwd_uri, - WindowsSandboxLevel::Disabled, + &local_context(&cwd_uri), + PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled), ), SafetyCheck::AskUser, ); @@ -132,8 +236,8 @@ fn granular_with_all_flags_true_matches_on_request_for_out_of_root_patch() { }), &permission_profile, &file_system_sandbox_policy, - &cwd_uri, - WindowsSandboxLevel::Disabled, + &local_context(&cwd_uri), + PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled), ), SafetyCheck::AskUser, ); @@ -168,8 +272,8 @@ fn granular_sandbox_approval_false_rejects_out_of_root_patch() { }), &permission_profile, &file_system_sandbox_policy, - &cwd_uri, - WindowsSandboxLevel::Disabled, + &local_context(&cwd_uri), + PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled), ), SafetyCheck::Reject { reason: PATCH_REJECTED_OUTSIDE_PROJECT_REASON.to_string(), @@ -191,7 +295,7 @@ fn read_only_policy_rejects_patch_with_read_only_reason() { assert!(!is_write_patch_constrained_to_writable_paths( &action, &file_system_sandbox_policy, - &cwd_uri, + &local_context(&cwd_uri), )); assert_eq!( assess_patch_safety( @@ -199,8 +303,8 @@ fn read_only_policy_rejects_patch_with_read_only_reason() { AskForApproval::Never, &permission_profile, &file_system_sandbox_policy, - &cwd_uri, - WindowsSandboxLevel::Disabled, + &local_context(&cwd_uri), + PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled), ), SafetyCheck::Reject { reason: PATCH_REJECTED_READ_ONLY_REASON.to_string(), @@ -241,7 +345,7 @@ fn explicit_unreadable_paths_prevent_auto_approval_for_external_sandbox() { assert!(!is_write_patch_constrained_to_writable_paths( &action, &file_system_sandbox_policy, - &cwd_uri, + &local_context(&cwd_uri), )); assert_eq!( assess_patch_safety( @@ -249,8 +353,8 @@ fn explicit_unreadable_paths_prevent_auto_approval_for_external_sandbox() { AskForApproval::OnRequest, &permission_profile, &file_system_sandbox_policy, - &cwd_uri, - WindowsSandboxLevel::Disabled, + &local_context(&cwd_uri), + PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled), ), SafetyCheck::AskUser, ); @@ -291,7 +395,7 @@ fn explicit_read_only_subpaths_prevent_auto_approval_for_external_sandbox() { assert!(!is_write_patch_constrained_to_writable_paths( &action, &file_system_sandbox_policy, - &cwd_uri, + &local_context(&cwd_uri), )); assert_eq!( assess_patch_safety( @@ -299,8 +403,8 @@ fn explicit_read_only_subpaths_prevent_auto_approval_for_external_sandbox() { AskForApproval::OnRequest, &permission_profile, &file_system_sandbox_policy, - &cwd_uri, - WindowsSandboxLevel::Disabled, + &local_context(&cwd_uri), + PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled), ), SafetyCheck::AskUser, ); @@ -334,7 +438,7 @@ fn missing_project_dot_codex_config_requires_approval() { assert!(!is_write_patch_constrained_to_writable_paths( &action, &file_system_sandbox_policy, - &cwd_uri, + &local_context(&cwd_uri), )); assert_eq!( assess_patch_safety( @@ -342,8 +446,8 @@ fn missing_project_dot_codex_config_requires_approval() { AskForApproval::OnRequest, &permission_profile, &file_system_sandbox_policy, - &cwd_uri, - WindowsSandboxLevel::Disabled, + &local_context(&cwd_uri), + PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled), ), SafetyCheck::AskUser, ); diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index b945bf6fbd..0f5288995b 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -1,4 +1,3 @@ -use std::collections::BTreeSet; use std::collections::HashMap; use std::path::Path; use std::path::PathBuf; @@ -10,6 +9,7 @@ use tokio_util::sync::CancellationToken; use crate::apply_patch; use crate::apply_patch::convert_apply_patch_to_protocol; use crate::function_tool::FunctionCallError; +use crate::safety::PatchSandboxRoute; use crate::session::session::Session; use crate::session::step_context::StepContext; use crate::session::turn_context::TurnContext; @@ -24,6 +24,7 @@ use crate::tools::events::ToolEmitter; use crate::tools::events::ToolEventCtx; use crate::tools::handlers::apply_granted_turn_permissions; use crate::tools::handlers::apply_patch_spec::create_apply_patch_freeform_tool; +use crate::tools::handlers::file_system_sandbox_policy_context_for_cwd; use crate::tools::handlers::resolve_tool_environment; use crate::tools::handlers::updated_hook_command; use crate::tools::hook_names::HookToolName; @@ -45,15 +46,16 @@ use codex_exec_server::ExecutorFileSystem; use codex_features::Feature; use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::models::FileSystemPermissions; +use codex_protocol::permissions::FileSystemSandboxPolicyContext; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::FileChange; use codex_protocol::protocol::PatchApplyUpdatedEvent; use codex_sandboxing::policy_transforms::effective_file_system_sandbox_policy; use codex_sandboxing::policy_transforms::merge_permission_profiles; use codex_sandboxing::policy_transforms::normalize_additional_permissions; +use codex_sandboxing::policy_transforms::normalize_additional_permissions_with_context; use codex_tools::ToolName; use codex_tools::ToolSpec; -use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; const APPLY_PATCH_ARGUMENT_DIFF_BUFFER_INTERVAL: Duration = Duration::from_millis(500); @@ -234,40 +236,47 @@ fn file_paths_for_action(action: &ApplyPatchAction) -> Vec { } fn write_permissions_for_paths( - file_paths: &[AbsolutePathBuf], + file_paths: &[PathUri], file_system_sandbox_policy: &codex_protocol::permissions::FileSystemSandboxPolicy, - cwd: &AbsolutePathBuf, + context: &FileSystemSandboxPolicyContext<'_>, + sandbox_route: PatchSandboxRoute, ) -> Option { - let write_paths = file_paths + let mut write_paths = file_paths .iter() // Skip already-writable targets before deriving parent permissions. // Otherwise, a writable directory could grant access to its parent. - .filter(|path| { - !file_system_sandbox_policy.can_write_path_with_cwd(path.as_path(), cwd.as_path()) - }) + .filter(|path| !file_system_sandbox_policy.can_write_path(path, context)) .map(|path| { path.parent() + .or_else(|| match sandbox_route { + PatchSandboxRoute::Platform(_) => { + // Host path rules can recover parents of opaque local paths. + // `to_abs_path` verifies that the target round-trips losslessly. + path.to_abs_path().ok()?.parent().map(PathUri::from) + } + PatchSandboxRoute::ExecutorManaged => None, + }) .unwrap_or_else(|| path.clone()) - .into_path_buf() }) - .filter(|path| { - !file_system_sandbox_policy.can_write_path_with_cwd(path.as_path(), cwd.as_path()) - }) - .collect::>() - .into_iter() - .map(AbsolutePathBuf::from_absolute_path) - .collect::, _>>() - .ok()?; + .filter(|path| !file_system_sandbox_policy.can_write_path(path, context)) + .collect::>(); + write_paths.sort_by_key(PathUri::to_string); + write_paths.dedup(); let permissions = (!write_paths.is_empty()).then_some(AdditionalPermissionProfile { - file_system: Some(FileSystemPermissions::from_read_write_roots( + file_system: Some(FileSystemPermissions::from_read_write_path_uris( Some(vec![]), Some(write_paths), )), ..Default::default() })?; - normalize_additional_permissions(permissions).ok() + match sandbox_route { + PatchSandboxRoute::Platform(_) => normalize_additional_permissions(permissions).ok(), + PatchSandboxRoute::ExecutorManaged => { + normalize_additional_permissions_with_context(permissions, context).ok() + } + } } /// Extracts the raw patch text used as the command-shaped hook input for apply_patch. @@ -282,15 +291,15 @@ async fn effective_patch_permissions( session: &Session, environment: &TurnEnvironment, action: &ApplyPatchAction, - cwd: &PathUri, -) -> std::io::Result<( + context: &FileSystemSandboxPolicyContext<'_>, + sandbox_route: PatchSandboxRoute, +) -> ( Vec, crate::tools::handlers::EffectiveAdditionalPermissions, codex_protocol::permissions::FileSystemSandboxPolicy, -)> { +) { let environment_id = environment.selection.environment_id.as_str(); let file_paths = file_paths_for_action(action); - let native_cwd = cwd.to_abs_path()?; let granted_permissions = merge_permission_profiles( session .granted_session_permissions(environment_id) @@ -302,49 +311,30 @@ async fn effective_patch_permissions( .as_ref(), ); let base_file_system_sandbox_policy = environment - .permission_profile_with_workspace_roots() + .permission_profile() .file_system_sandbox_policy(); let file_system_sandbox_policy = effective_file_system_sandbox_policy( &base_file_system_sandbox_policy, granted_permissions.as_ref(), ); - let native_file_paths = file_paths - .iter() - .map(PathUri::to_abs_path) - .collect::, _>>()?; let effective_additional_permissions = apply_granted_turn_permissions( session, environment, - cwd, + context.cwd, crate::sandboxing::SandboxPermissions::UseDefault, - write_permissions_for_paths(&native_file_paths, &file_system_sandbox_policy, &native_cwd), + write_permissions_for_paths( + &file_paths, + &file_system_sandbox_policy, + context, + sandbox_route, + ), ) .await; - Ok(( + ( file_paths, effective_additional_permissions, file_system_sandbox_policy, - )) -} - -fn patch_permissions_without_path_matching( - action: &ApplyPatchAction, -) -> ( - Vec, - crate::tools::handlers::EffectiveAdditionalPermissions, - codex_protocol::permissions::FileSystemSandboxPolicy, -) { - // TODO(anp): Make permission matching operate on PathUri. Until then, foreign paths skip - // permission matching; a managed turn still fails closed at the platform sandbox boundary. - ( - file_paths_for_action(action), - crate::tools::handlers::EffectiveAdditionalPermissions { - sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, - additional_permissions: None, - permissions_preapproved: false, - }, - codex_protocol::permissions::FileSystemSandboxPolicy::unrestricted(), ) } @@ -429,7 +419,6 @@ impl ApplyPatchHandler { }; let content = execute_verified_patch( changes, - turn_environment.cwd(), turn_environment.clone(), Some(&tracker), tool_ctx, @@ -539,7 +528,7 @@ pub(crate) async fn intercept_apply_patch( tool_name: ToolName::plain(tool_name), }; let content = - execute_verified_patch(changes, cwd, turn_environment, tracker, tool_ctx).await?; + execute_verified_patch(changes, turn_environment, tracker, tool_ctx).await?; Ok(Some(FunctionToolOutput::from_text(content, Some(true)))) } codex_apply_patch::MaybeApplyPatchVerified::CorrectnessError(parse_error) => { @@ -557,19 +546,38 @@ pub(crate) async fn intercept_apply_patch( async fn execute_verified_patch( action: ApplyPatchAction, - cwd: &PathUri, turn_environment: TurnEnvironment, tracker: Option<&SharedTurnDiffTracker>, tool_ctx: ToolCtx, ) -> Result { + let cwd = action.cwd.clone(); + let sandbox_context = turn_environment.sandbox_context(/*additional_permissions*/ None); + let Some(policy_context) = file_system_sandbox_policy_context_for_cwd(&sandbox_context, &cwd) + else { + return Err(FunctionCallError::RespondToModel( + "apply_patch requires an executor cwd".to_string(), + )); + }; + let sandbox_route = if turn_environment.environment.is_remote() { + PatchSandboxRoute::ExecutorManaged + } else { + PatchSandboxRoute::Platform(turn_environment.config().windows_sandbox_level) + }; let (file_paths, effective_additional_permissions, file_system_sandbox_policy) = - effective_patch_permissions(tool_ctx.session.as_ref(), &turn_environment, &action, cwd) - .await - .unwrap_or_else(|_| patch_permissions_without_path_matching(&action)); + effective_patch_permissions( + tool_ctx.session.as_ref(), + &turn_environment, + &action, + &policy_context, + sandbox_route, + ) + .await; let apply = apply_patch::prepare_apply_patch( &tool_ctx.step_context, &turn_environment, &file_system_sandbox_policy, + &policy_context, + sandbox_route, action, )?; let changes = convert_apply_patch_to_protocol(&apply.action); diff --git a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs index 24d6f6ea4a..f2c07293cd 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs @@ -1,10 +1,13 @@ use super::*; use codex_apply_patch::MaybeApplyPatchVerified; use codex_exec_server::LOCAL_FS; +use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemSandboxEntry; use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::FileSystemSandboxPolicyContext; use codex_protocol::protocol::FileChange; +use codex_utils_absolute_path::AbsolutePathBuf; use core_test_support::PathBufExt; use core_test_support::PathExt; use pretty_assertions::assert_eq; @@ -15,6 +18,15 @@ use std::sync::Arc; use tempfile::TempDir; use tokio::sync::Mutex; +fn local_context(cwd: &PathUri) -> FileSystemSandboxPolicyContext<'_> { + FileSystemSandboxPolicyContext { + cwd, + workspace_roots: std::slice::from_ref(cwd), + user_home_dir: None, + temporary_directories: None, + } +} + use crate::session::step_context::StepContext; use crate::session::tests::make_session_and_context; use crate::tools::context::ToolInvocation; @@ -281,7 +293,12 @@ fn write_permissions_for_paths_skip_dirs_already_writable_under_workspace_root() /*exclude_slash_tmp*/ false, ); - let permissions = write_permissions_for_paths(&[file_path], &sandbox_policy, &cwd); + let permissions = write_permissions_for_paths( + &[file_path.into()], + &sandbox_policy, + &local_context(&cwd.into()), + PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled), + ); assert_eq!(permissions, None); } @@ -302,7 +319,12 @@ fn write_permissions_for_paths_keep_dirs_outside_workspace_root() { /*exclude_slash_tmp*/ true, ); - let permissions = write_permissions_for_paths(&[file_path], &sandbox_policy, &cwd_abs); + let permissions = write_permissions_for_paths( + &[file_path.into()], + &sandbox_policy, + &local_context(&cwd_abs.into()), + PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled), + ); let expected_outside = outside.abs(); assert_eq!( @@ -324,9 +346,12 @@ fn write_permissions_for_paths_do_not_widen_workspace_root_target() { /*exclude_tmpdir_env_var*/ true, /*exclude_slash_tmp*/ true, ); - - let permissions = - write_permissions_for_paths(std::slice::from_ref(&cwd), &sandbox_policy, &cwd); + let permissions = write_permissions_for_paths( + &[cwd.clone().into()], + &sandbox_policy, + &local_context(&cwd.into()), + PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled), + ); assert_eq!(permissions, None); } @@ -341,7 +366,41 @@ fn write_permissions_for_paths_do_not_regrant_an_already_writable_parent() { FileSystemSandboxEntry::new(file_path.clone().into(), FileSystemAccessMode::Read), ]); - let permissions = write_permissions_for_paths(&[file_path], &sandbox_policy, &cwd); + let permissions = write_permissions_for_paths( + &[file_path.into()], + &sandbox_policy, + &local_context(&cwd.into()), + PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled), + ); assert_eq!(permissions, None); } + +#[test] +fn write_permissions_for_windows_paths_uses_executor_uris() { + let cwd = PathUri::parse("file:///C:/workspace").expect("Windows cwd"); + let context = local_context(&cwd); + let policy = FileSystemSandboxPolicy::workspace_write( + &[], + /*exclude_tmpdir_env_var*/ true, + /*exclude_slash_tmp*/ true, + ); + let outside = PathUri::parse("file:///C:/outside/out.txt").expect("outside"); + + assert_eq!( + write_permissions_for_paths( + &[outside], + &policy, + &context, + PatchSandboxRoute::ExecutorManaged, + ) + .and_then(|profile| profile.file_system) + .map(|permissions| permissions.entries), + Some(vec![FileSystemSandboxEntry::new( + PathUri::parse("file:///C:/outside") + .expect("outside parent") + .into(), + FileSystemAccessMode::Write, + )]), + ); +} diff --git a/codex-rs/core/tests/suite/workspace_roots.rs b/codex-rs/core/tests/suite/workspace_roots.rs index a3ed21581b..5dd6c5c367 100644 --- a/codex-rs/core/tests/suite/workspace_roots.rs +++ b/codex-rs/core/tests/suite/workspace_roots.rs @@ -1,7 +1,6 @@ use anyhow::Context; use anyhow::Result; use codex_core::EnvironmentConfig; -use codex_core::windows_sandbox::WindowsSandboxLevelExt; use codex_exec_server::CreateDirectoryOptions; use codex_exec_server::RemoveOptions; use codex_protocol::config_types::WindowsSandboxLevel; @@ -234,7 +233,8 @@ async fn workspace_roots_allow_file_and_command_writes_in_secondary_root( if !owner_resolved_roots { config.workspace_roots.push(secondary_root); } - config.set_windows_sandbox_enabled(/*value*/ true); + // Owner-provided sandbox settings must work independently of thread defaults. + config.set_windows_sandbox_enabled(!owner_resolved_roots); }) .with_workspace_setup(|cwd, fs| async move { let secondary_root = cwd @@ -276,7 +276,7 @@ async fn workspace_roots_allow_file_and_command_writes_in_secondary_root( workspace_roots_profile(), ), shell_environment_policy: Default::default(), - windows_sandbox_level: WindowsSandboxLevel::from_config(&test.config), + windows_sandbox_level: WindowsSandboxLevel::RestrictedToken, windows_sandbox_private_desktop: test .config .permissions diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index 7327a5177d..2e02b851cd 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -743,7 +743,7 @@ impl FileSystemSandboxPolicy { /// entry for the same target wins under the normal precedence rules, so a /// shadowed `read` entry must not downgrade the policy out of full-disk /// write mode. - fn has_write_narrowing_entries(&self) -> bool { + fn has_write_narrowing_entries(&self, convention: PathConvention) -> bool { matches!(self.kind, FileSystemSandboxKind::Restricted) && self.entries.iter().any(|entry| { if entry.access.can_write() { @@ -751,15 +751,21 @@ impl FileSystemSandboxPolicy { } match &entry.path { - FileSystemPath::Path { .. } => !self.has_same_target_write_override(entry), + FileSystemPath::Path { .. } => { + !self.has_same_target_write_override(entry, convention) + } FileSystemPath::GlobPattern { .. } => true, FileSystemPath::Special { value } => match value { FileSystemSpecialPath::Root => entry.access == FileSystemAccessMode::Deny, - FileSystemSpecialPath::SlashTmp if !cfg!(unix) => false, + FileSystemSpecialPath::SlashTmp + if convention == PathConvention::Windows => + { + false + } FileSystemSpecialPath::Minimal | FileSystemSpecialPath::Unknown { .. } => { false } - _ => !self.has_same_target_write_override(entry), + _ => !self.has_same_target_write_override(entry, convention), }, } }) @@ -767,11 +773,15 @@ impl FileSystemSandboxPolicy { /// Returns true when a higher-priority `write` entry targets the same /// location as `entry`, so `entry` cannot narrow effective write access. - fn has_same_target_write_override(&self, entry: &FileSystemSandboxEntry) -> bool { + fn has_same_target_write_override( + &self, + entry: &FileSystemSandboxEntry, + convention: PathConvention, + ) -> bool { self.entries.iter().any(|candidate| { candidate.access.can_write() && candidate.access > entry.access - && file_system_paths_share_target(&candidate.path, &entry.path) + && file_system_paths_share_target(&candidate.path, &entry.path, convention) }) } @@ -880,14 +890,29 @@ impl FileSystemSandboxPolicy { } } - /// Returns true when filesystem writes are unrestricted. + /// Returns true when filesystem writes are unrestricted on this host. pub fn has_full_disk_write_access(&self) -> bool { + self.has_full_disk_write_access_for_convention(Some(PathConvention::native())) + } + + /// Returns true when filesystem writes are unrestricted for the selected executor. + pub fn has_full_disk_write_access_with_context( + &self, + context: &FileSystemSandboxPolicyContext<'_>, + ) -> bool { + self.has_full_disk_write_access_for_convention(context.cwd.infer_path_convention()) + } + + fn has_full_disk_write_access_for_convention( + &self, + convention: Option, + ) -> bool { match self.kind { FileSystemSandboxKind::Unrestricted | FileSystemSandboxKind::ExternalSandbox => true, - FileSystemSandboxKind::Restricted => { + FileSystemSandboxKind::Restricted => convention.is_some_and(|convention| { self.has_root_access(FileSystemAccessMode::can_write) - && !self.has_write_narrowing_entries() - } + && !self.has_write_narrowing_entries(convention) + }), } } @@ -967,11 +992,16 @@ impl FileSystemSandboxPolicy { .unwrap_or(FileSystemAccessMode::Deny) } - fn can_write_path(&self, path: &PathUri, context: &FileSystemSandboxPolicyContext<'_>) -> bool { + pub fn can_write_path( + &self, + path: &PathUri, + context: &FileSystemSandboxPolicyContext<'_>, + ) -> bool { if !self.resolve_access(path, context).can_write() { return false; } - self.has_full_disk_write_access() || self.metadata_write_denial(path, context).is_none() + self.has_full_disk_write_access_with_context(context) + || self.metadata_write_denial(path, context).is_none() } fn metadata_write_denial( @@ -1453,13 +1483,23 @@ impl FileSystemSandboxPolicy { /// do not currently exist (including `/tmp`). Do not use this result to authorize /// filesystem access or replace the resolution needed for sandbox enforcement. pub fn has_configured_writable_roots_with_cwd(&self, cwd: &Path) -> bool { - !self.has_full_disk_write_access() - && with_local_policy_context(cwd, cwd, |_, context| { - self.resolved_entries(context) - .into_iter() - .any(|(path, access)| access.can_write() && self.can_write_path(&path, context)) - }) - .unwrap_or(false) + with_local_policy_context(cwd, cwd, |_, context| { + self.has_configured_writable_roots(context) + }) + .unwrap_or(false) + } + + /// Reports configured writable roots for executor-context diagnostics, excluding + /// full-disk policies and without inspecting the filesystem. + pub fn has_configured_writable_roots( + &self, + context: &FileSystemSandboxPolicyContext<'_>, + ) -> bool { + !self.has_full_disk_write_access_with_context(context) + && self + .resolved_entries(context) + .into_iter() + .any(|(path, access)| access.can_write() && self.can_write_path(&path, context)) } /// Returns writable roots without following attacker-mutable path components. @@ -1979,7 +2019,11 @@ fn local_temporary_directories() -> Vec { /// This is intentionally narrower than full path resolution: it only answers /// the "can one entry shadow another at the same specificity?" question used /// by `has_write_narrowing_entries`. -fn file_system_paths_share_target(left: &FileSystemPath, right: &FileSystemPath) -> bool { +fn file_system_paths_share_target( + left: &FileSystemPath, + right: &FileSystemPath, + convention: PathConvention, +) -> bool { match (left, right) { (FileSystemPath::Path { path: left }, FileSystemPath::Path { path: right }) => { left == right @@ -1988,9 +2032,10 @@ fn file_system_paths_share_target(left: &FileSystemPath, right: &FileSystemPath) special_paths_share_target(left, right) } (FileSystemPath::Path { path }, FileSystemPath::Special { value }) - | (FileSystemPath::Special { value }, FileSystemPath::Path { path }) => path - .to_abs_path() - .is_ok_and(|path| special_path_matches_absolute_path(value, &path)), + | (FileSystemPath::Special { value }, FileSystemPath::Path { path }) => { + path.infer_path_convention() == Some(convention) + && special_path_matches_path_uri(value, path) + } ( FileSystemPath::GlobPattern { pattern: left }, FileSystemPath::GlobPattern { pattern: right }, @@ -2025,18 +2070,19 @@ fn special_paths_share_target(left: &FileSystemSpecialPath, right: &FileSystemSp } } -/// Matches cwd-independent special paths against absolute `Path` entries when +/// Matches cwd-independent special paths against `PathUri` entries when /// they name the same location. /// /// We intentionally only fold the special paths whose concrete meaning is /// stable without a cwd, such as `/` and `/tmp`. -fn special_path_matches_absolute_path( - value: &FileSystemSpecialPath, - path: &AbsolutePathBuf, -) -> bool { +fn special_path_matches_path_uri(value: &FileSystemSpecialPath, path: &PathUri) -> bool { match value { - FileSystemSpecialPath::Root => path.as_path().parent().is_none(), - FileSystemSpecialPath::SlashTmp => path.as_path() == Path::new("/tmp"), + FileSystemSpecialPath::Root => path.lexical_depth().is_some() && path.parent().is_none(), + FileSystemSpecialPath::SlashTmp => { + path.infer_path_convention() == Some(PathConvention::Posix) + && path.lexical_depth() == Some(1) + && path.basename().as_deref() == Some("tmp") + } _ => false, } } @@ -2329,7 +2375,7 @@ fn append_default_read_only_entry_if_no_explicit_rule( ) { if entries .iter() - .any(|entry| file_system_paths_share_target(&entry.path, &path)) + .any(|entry| file_system_paths_share_target(&entry.path, &path, PathConvention::native())) { return; } @@ -2509,6 +2555,12 @@ mod tests { FileSystemSandboxPolicy::read_only(), FileSystemSandboxPolicy::unrestricted(), FileSystemSandboxPolicy::external_sandbox(), + FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry::new( + FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + FileSystemAccessMode::Write, + )]), FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry::new( writable_root.clone().into(), FileSystemAccessMode::Write,