diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index f1559f5407..b945bf6fbd 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -314,8 +314,8 @@ async fn effective_patch_permissions( .collect::, _>>()?; let effective_additional_permissions = apply_granted_turn_permissions( session, - environment_id, - native_cwd.as_path(), + environment, + cwd, crate::sandboxing::SandboxPermissions::UseDefault, write_permissions_for_paths(&native_file_paths, &file_system_sandbox_policy, &native_cwd), ) diff --git a/codex-rs/core/src/tools/handlers/extension_tools.rs b/codex-rs/core/src/tools/handlers/extension_tools.rs index cad51e0be7..9a6d56f22e 100644 --- a/codex-rs/core/src/tools/handlers/extension_tools.rs +++ b/codex-rs/core/src/tools/handlers/extension_tools.rs @@ -184,8 +184,8 @@ async fn to_extension_call(invocation: &ToolInvocation) -> ExtensionToolCall<'_> }; let additional_permissions = apply_granted_turn_permissions( invocation.session.as_ref(), - &environment.selection.environment_id, - native_cwd.as_path(), + environment, + environment.cwd(), SandboxPermissions::UseDefault, /*additional_permissions*/ None, ) diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index 1217131be2..35268fe55f 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -35,15 +35,16 @@ mod view_image; pub(crate) mod view_image_spec; mod wait_for_environment; -use codex_sandboxing::policy_transforms::materialize_additional_permissions; +use codex_file_system::FileSystemSandboxContext; +use codex_sandboxing::policy_transforms::materialize_additional_permissions_with_context; 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_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::AbsolutePathBufGuard; +use codex_utils_path_uri::PathUri; use serde::Deserialize; use serde_json::Map; use serde_json::Value; -use std::path::Path; use crate::environment_selection::TurnEnvironmentSnapshot; use crate::function_tool::FunctionCallError; @@ -181,7 +182,7 @@ pub(crate) fn normalize_and_validate_additional_permissions( sandbox_permissions: SandboxPermissions, additional_permissions: Option, permissions_preapproved: bool, - _cwd: &Path, + context: &codex_protocol::permissions::FileSystemSandboxPolicyContext<'_>, ) -> Result, String> { let uses_additional_permissions = matches!( sandbox_permissions, @@ -210,7 +211,8 @@ pub(crate) fn normalize_and_validate_additional_permissions( .to_string(), ); }; - let normalized = normalize_additional_permissions(additional_permissions)?; + let normalized = + normalize_additional_permissions_with_context(additional_permissions, context)?; if normalized.is_empty() { return Err( "`additional_permissions` must include at least one requested permission in `network` or `file_system`" @@ -236,6 +238,15 @@ pub(super) struct EffectiveAdditionalPermissions { pub permissions_preapproved: bool, } +pub(super) fn file_system_sandbox_policy_context_for_cwd<'a>( + sandbox_context: &'a FileSystemSandboxContext, + cwd: &'a PathUri, +) -> Option> { + let mut context = sandbox_context.policy_context()?; + context.cwd = cwd; + Some(context) +} + pub(super) fn implicit_granted_permissions( sandbox_permissions: SandboxPermissions, additional_permissions: Option<&AdditionalPermissionProfile>, @@ -255,8 +266,8 @@ pub(super) fn implicit_granted_permissions( pub(super) async fn apply_granted_turn_permissions( session: &Session, - environment_id: &str, - cwd: &Path, + environment: &TurnEnvironment, + cwd: &PathUri, sandbox_permissions: SandboxPermissions, additional_permissions: Option, ) -> EffectiveAdditionalPermissions { @@ -268,6 +279,7 @@ pub(super) async fn apply_granted_turn_permissions( }; } + let environment_id = &environment.selection.environment_id; let granted_session_permissions = session.granted_session_permissions(environment_id).await; let granted_turn_permissions = session.granted_turn_permissions(environment_id).await; let granted_permissions = merge_permission_profiles( @@ -278,13 +290,15 @@ pub(super) async fn apply_granted_turn_permissions( additional_permissions.as_ref(), granted_permissions.as_ref(), ); + let sandbox_context = environment.sandbox_context(/*additional_permissions*/ None); + let context = file_system_sandbox_policy_context_for_cwd(&sandbox_context, cwd); let preapproved_permissions = granted_permissions.as_ref().and_then(|granted| { if additional_permissions.is_none() { Some(granted.clone()) } else { - effective_permissions - .as_ref() - .and_then(|effective| preapproved_permission_profile(effective, granted, cwd)) + effective_permissions.as_ref().and_then(|effective| { + preapproved_permission_profile(effective, granted, context.as_ref()?) + }) } }); let permissions_preapproved = preapproved_permissions.is_some(); @@ -309,11 +323,35 @@ pub(super) async fn apply_granted_turn_permissions( fn preapproved_permission_profile( effective_permissions: &AdditionalPermissionProfile, granted_permissions: &AdditionalPermissionProfile, - cwd: &Path, + context: &codex_protocol::permissions::FileSystemSandboxPolicyContext<'_>, ) -> Option { + if effective_permissions + .file_system + .as_ref() + .is_some_and(|permissions| { + permissions.entries.iter().any(|entry| { + (matches!( + &entry.path, + codex_protocol::permissions::FileSystemPath::Special { + value: codex_protocol::permissions::FileSystemSpecialPath::Tmpdir, + } + ) && context + .temporary_directories + .is_none_or(<[PathUri]>::is_empty)) + || matches!( + &entry.path, + codex_protocol::permissions::FileSystemPath::Special { + value: codex_protocol::permissions::FileSystemSpecialPath::ProjectRoots { .. }, + } if context.workspace_roots.is_empty() + ) + }) + }) + { + return None; + } let (Ok(effective), Ok(granted)) = ( - materialize_additional_permissions(effective_permissions.clone(), cwd), - materialize_additional_permissions(granted_permissions.clone(), cwd), + materialize_additional_permissions_with_context(effective_permissions.clone(), context), + materialize_additional_permissions_with_context(granted_permissions.clone(), context), ) else { return None; }; @@ -352,12 +390,14 @@ mod tests { use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxEntry; + use codex_protocol::permissions::FileSystemSandboxPolicyContext; use codex_protocol::permissions::FileSystemSpecialPath; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::GranularApprovalConfig; use codex_sandboxing::policy_transforms::intersect_permission_profiles; use codex_sandboxing::policy_transforms::merge_permission_profiles; use codex_utils_absolute_path::AbsolutePathBuf; + use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; use tempfile::tempdir; @@ -382,10 +422,20 @@ mod tests { } } + fn local_context(cwd: &PathUri) -> FileSystemSandboxPolicyContext<'_> { + FileSystemSandboxPolicyContext { + cwd, + workspace_roots: std::slice::from_ref(cwd), + temporary_directories: None, + user_home_dir: None, + } + } + #[test] fn preapproved_permissions_work_when_request_permissions_tool_is_enabled_without_exec_permission_approvals_feature() { let cwd = tempdir().expect("tempdir"); + let cwd = PathUri::from_host_native_path(cwd.path()).expect("cwd URI"); let normalized = normalize_and_validate_additional_permissions( /*additional_permissions_allowed*/ false, @@ -399,7 +449,7 @@ mod tests { SandboxPermissions::WithAdditionalPermissions, Some(network_permissions()), /*permissions_preapproved*/ true, - cwd.path(), + &local_context(&cwd), ) .expect("preapproved permissions should be allowed"); @@ -409,6 +459,7 @@ mod tests { #[test] fn fresh_additional_permissions_still_require_exec_permission_approvals_feature() { let cwd = tempdir().expect("tempdir"); + let cwd = PathUri::from_host_native_path(cwd.path()).expect("cwd URI"); let err = normalize_and_validate_additional_permissions( /*additional_permissions_allowed*/ false, @@ -416,7 +467,7 @@ mod tests { SandboxPermissions::WithAdditionalPermissions, Some(network_permissions()), /*permissions_preapproved*/ false, - cwd.path(), + &local_context(&cwd), ) .expect_err("fresh inline permission requests should remain disabled"); @@ -493,10 +544,71 @@ mod tests { let effective_permissions = merge_permission_profiles(Some(&requested_permissions), Some(&stored_grant)) .expect("merged permissions"); + let cwd = PathUri::from_host_native_path(cwd.path()).expect("cwd URI"); assert_eq!( - preapproved_permission_profile(&effective_permissions, &stored_grant, cwd.path()), + preapproved_permission_profile( + &effective_permissions, + &stored_grant, + &local_context(&cwd), + ), Some(stored_grant) ); } + + #[test] + fn symbolic_tmpdir_preapproval_requires_executor_metadata() { + let cwd = PathUri::parse("file:///C:/workspace").expect("Windows cwd"); + let temporary_directory = PathUri::parse("file:///C:/Temp").expect("Windows temp dir"); + let concrete = FileSystemSandboxEntry::new( + temporary_directory.clone().into(), + FileSystemAccessMode::Write, + ); + let symbolic = FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Tmpdir, + }, + access: FileSystemAccessMode::Write, + missing_path_behavior: None, + }; + let effective = AdditionalPermissionProfile { + file_system: Some(FileSystemPermissions { + entries: vec![concrete.clone(), symbolic], + glob_scan_max_depth: None, + }), + ..Default::default() + }; + let granted = AdditionalPermissionProfile { + file_system: Some(FileSystemPermissions { + entries: vec![concrete], + glob_scan_max_depth: None, + }), + ..Default::default() + }; + + assert_eq!( + preapproved_permission_profile(&effective, &granted, &local_context(&cwd)), + None + ); + + let empty_temporary_directories = []; + let empty_tmpdir_context = FileSystemSandboxPolicyContext { + temporary_directories: Some(&empty_temporary_directories), + ..local_context(&cwd) + }; + assert_eq!( + preapproved_permission_profile(&effective, &granted, &empty_tmpdir_context), + None + ); + + let temporary_directories = [temporary_directory]; + let tmpdir_context = FileSystemSandboxPolicyContext { + temporary_directories: Some(&temporary_directories), + ..local_context(&cwd) + }; + assert_eq!( + preapproved_permission_profile(&effective, &granted, &tmpdir_context), + Some(granted) + ); + } } diff --git a/codex-rs/core/src/tools/handlers/permission_preapproval_tests.rs b/codex-rs/core/src/tools/handlers/permission_preapproval_tests.rs index 13826255b6..4876caa94f 100644 --- a/codex-rs/core/src/tools/handlers/permission_preapproval_tests.rs +++ b/codex-rs/core/src/tools/handlers/permission_preapproval_tests.rs @@ -5,8 +5,10 @@ use codex_protocol::models::NetworkPermissions; use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxEntry; +use codex_protocol::permissions::FileSystemSandboxPolicyContext; use codex_sandboxing::policy_transforms::merge_permission_profiles; use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; use tempfile::tempdir; @@ -20,6 +22,15 @@ fn file_system_permissions(entries: Vec) -> AdditionalPe } } +fn local_context(cwd: &PathUri) -> FileSystemSandboxPolicyContext<'_> { + FileSystemSandboxPolicyContext { + cwd, + workspace_roots: std::slice::from_ref(cwd), + temporary_directories: None, + user_home_dir: None, + } +} + #[test] fn preapproval_accepts_reordered_replay_of_one_accumulated_grant() { let cwd = tempdir().expect("tempdir"); @@ -35,9 +46,10 @@ fn preapproval_accepts_reordered_replay_of_one_accumulated_grant() { }); let replay = file_system_permissions(vec![write]); let effective = merge_permission_profiles(Some(&replay), Some(&granted)).expect("permissions"); + let cwd = PathUri::from_host_native_path(cwd.path()).expect("cwd URI"); assert_eq!( - preapproved_permission_profile(&effective, &granted, cwd.path()), + preapproved_permission_profile(&effective, &granted, &local_context(&cwd)), Some(granted) ); } @@ -50,6 +62,8 @@ fn preapproval_requires_fresh_read_and_write_beneath_a_deny() { FileSystemSandboxEntry::new(root.clone().into(), FileSystemAccessMode::Write), FileSystemSandboxEntry::new(root.join("secrets").into(), FileSystemAccessMode::Deny), ]); + let cwd = PathUri::from_host_native_path(cwd.path()).expect("cwd URI"); + let context = local_context(&cwd); let preapproved = [FileSystemAccessMode::Read, FileSystemAccessMode::Write].map(|access| { let requested = file_system_permissions(vec![FileSystemSandboxEntry::new( @@ -59,7 +73,7 @@ fn preapproval_requires_fresh_read_and_write_beneath_a_deny() { let effective = merge_permission_profiles(Some(&requested), Some(&granted)).expect("permissions"); - preapproved_permission_profile(&effective, &granted, cwd.path()) + preapproved_permission_profile(&effective, &granted, &context) }); assert_eq!(preapproved, [None, None]); @@ -74,9 +88,10 @@ fn preapproval_fails_closed_when_materialization_rejects_both_profiles() { }, FileSystemAccessMode::Write, )]); + let cwd = PathUri::from_host_native_path(cwd.path()).expect("cwd URI"); assert_eq!( - preapproved_permission_profile(&invalid, &invalid, cwd.path()), + preapproved_permission_profile(&invalid, &invalid, &local_context(&cwd)), None ); } diff --git a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs index 6b54a2beb5..2c7b7a2ef8 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs @@ -12,6 +12,7 @@ use crate::tools::context::ToolPayload; use crate::tools::context::boxed_tool_output; use crate::tools::handlers::apply_granted_turn_permissions; use crate::tools::handlers::apply_patch::intercept_apply_patch; +use crate::tools::handlers::file_system_sandbox_policy_context_for_cwd; use crate::tools::handlers::implicit_granted_permissions; use crate::tools::handlers::normalize_and_validate_additional_permissions; use crate::tools::handlers::parse_arguments; @@ -301,12 +302,20 @@ impl ExecCommandHandler { let exec_permission_approvals_enabled = session.features().enabled(Feature::ExecPermissionApprovals); let requested_additional_permissions = additional_permissions.clone(); - // TODO(anp): Make permission matching operate on PathUri for remote environments. - let permission_cwd = native_cwd.as_ref().unwrap_or(&turn.config.cwd); + let sandbox_context = + turn_environment.sandbox_context(/*additional_permissions*/ None); + let Some(permission_context) = + file_system_sandbox_policy_context_for_cwd(&sandbox_context, &cwd) + else { + manager.release_process_id(process_id).await; + return Err(FunctionCallError::RespondToModel( + "selected environment sandbox context is missing cwd".to_string(), + )); + }; let effective_additional_permissions = apply_granted_turn_permissions( context.session.as_ref(), - &turn_environment.selection.environment_id, - permission_cwd.as_path(), + turn_environment, + &cwd, sandbox_permissions, additional_permissions, ) @@ -343,7 +352,7 @@ impl ExecCommandHandler { effective_additional_permissions.sandbox_permissions, effective_additional_permissions.additional_permissions, effective_additional_permissions.permissions_preapproved, - permission_cwd, + &permission_context, ) }, |permissions| Ok(Some(permissions)), diff --git a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs index 775d78cc1f..f75b57d213 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -280,6 +280,99 @@ async fn shell_mode_for_environment_uses_direct_mode_for_remote_environments() - Ok(()) } +#[tokio::test] +#[cfg(not(windows))] +async fn exec_command_reuses_foreign_windows_grant() { + use crate::session::tests::make_session_and_context_with_auth_and_config_and_rx; + use codex_features::Feature; + use codex_protocol::models::AdditionalPermissionProfile; + use codex_protocol::models::FileSystemPermissions; + use codex_utils_path_uri::PathUri; + + let (session, mut turn, _events) = make_session_and_context_with_auth_and_config_and_rx( + codex_login::CodexAuth::from_api_key("Test API Key"), + Vec::new(), + |config| { + config + .features + .enable(Feature::RequestPermissionsTool) + .expect("test setup should allow request permissions"); + }, + ) + .await; + + let cwd = PathUri::parse("file:///C:/workspace").expect("valid Windows cwd"); + let granted_permissions = AdditionalPermissionProfile { + file_system: Some(FileSystemPermissions::from_read_write_path_uris( + /*read*/ Some(Vec::new()), + /*write*/ + Some(vec![ + PathUri::parse("file:///C:/workspace/granted").expect("valid Windows grant"), + ]), + )), + ..Default::default() + }; + *session.active_turn.lock().await = Some(crate::state::ActiveTurn::default()); + let turn_state = { + let active_turn = session.active_turn.lock().await; + Arc::clone(&active_turn.as_ref().expect("active turn").turn_state) + }; + turn_state.lock().await.record_granted_permissions( + codex_exec_server::REMOTE_ENVIRONMENT_ID, + granted_permissions.clone(), + ); + + { + let turn = Arc::get_mut(&mut turn).expect("turn should be uniquely owned"); + let TurnEnvironmentState::Ready(environment) = turn + .environments + .environments + .first_mut() + .expect("primary environment") + else { + panic!("primary environment should be ready"); + }; + environment.selection.environment_id = codex_exec_server::REMOTE_ENVIRONMENT_ID.to_string(); + environment.selection.cwd = cwd.clone(); + environment.selection.workspace_roots = vec![cwd.clone()]; + environment.config_mut().workspace_roots = vec![cwd]; + environment.environment = Arc::new( + Environment::create_for_tests(Some("ws://127.0.0.1:1/remote-exec-server".to_string())) + .expect("remote environment"), + ); + } + + let response = ExecCommandHandler::default() + .handle(ToolInvocation { + session: Arc::clone(&session), + step_context: StepContext::for_test(Arc::clone(&turn)), + turn, + cancellation_token: tokio_util::sync::CancellationToken::new(), + tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), + call_id: "foreign-windows-grant".to_string(), + tool_name: codex_tools::ToolName::plain("exec_command"), + source: ToolCallSource::Direct, + payload: ToolPayload::Function { + arguments: serde_json::json!({ + "cmd": "*** Begin Patch\n*** Add File: granted/file.txt\n+text\n*** End Patch", + "workdir": "nested", + "sandbox_permissions": "with_additional_permissions", + "additional_permissions": granted_permissions, + }) + .to_string(), + }, + }) + .await; + + let Err(FunctionCallError::RespondToModel(message)) = response else { + panic!("raw patch should stop before remote execution"); + }; + assert!( + message.contains("apply_patch verification failed"), + "matching foreign grant should reach patch interception: {message}" + ); +} + #[tokio::test] async fn exec_command_pre_tool_use_payload_uses_raw_command() { let payload = ToolPayload::Function { diff --git a/codex-rs/sandboxing/src/policy_transforms.rs b/codex-rs/sandboxing/src/policy_transforms.rs index ad35730843..0f8918df5c 100644 --- a/codex-rs/sandboxing/src/policy_transforms.rs +++ b/codex-rs/sandboxing/src/policy_transforms.rs @@ -67,7 +67,6 @@ pub fn normalize_additional_permissions_with_context( if path.infer_path_convention().is_none() || path.infer_path_convention() != context.cwd.infer_path_convention() || path.join(".").is_err() - || context.cwd.join(".").is_err() { return Err(format!( "permission path `{path}` does not match executor cwd `{}`", diff --git a/codex-rs/sandboxing/src/policy_transforms_tests.rs b/codex-rs/sandboxing/src/policy_transforms_tests.rs index 97334bfc40..2a13575cdf 100644 --- a/codex-rs/sandboxing/src/policy_transforms_tests.rs +++ b/codex-rs/sandboxing/src/policy_transforms_tests.rs @@ -18,6 +18,8 @@ use codex_protocol::permissions::FileSystemSandboxPolicyContext; use codex_protocol::permissions::FileSystemSpecialPath; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::LegacyAppPathString; +use codex_utils_path_uri::PathConvention; use codex_utils_path_uri::PathUri; use dunce::canonicalize; use pretty_assertions::assert_eq; @@ -162,9 +164,29 @@ fn normalize_additional_permissions_only_checks_convention_with_context() { temporary_directories: None, }; assert!( - normalize_additional_permissions_with_context(permissions, &context).is_err(), + normalize_additional_permissions_with_context(permissions.clone(), &context).is_err(), "context-aware normalization should reject mismatched conventions" ); + + let cwd = LegacyAppPathString::from_string( + r"\\?\Volume{00000000-0000-0000-0000-000000000000}\workspace", + ) + .to_path_uri(PathConvention::Windows) + .expect("opaque Windows cwd"); + assert!(cwd.is_opaque(), "volume GUID cwd should stay opaque"); + let workspace_roots = [cwd.clone()]; + let context = FileSystemSandboxPolicyContext { + cwd: &cwd, + workspace_roots: &workspace_roots, + user_home_dir: None, + temporary_directories: None, + }; + + assert_eq!( + normalize_additional_permissions_with_context(permissions.clone(), &context) + .expect("absolute path with opaque cwd"), + permissions + ); } #[cfg(unix)]