diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index 1928a95621..0d64934cad 100644 --- a/codex-rs/core/src/apply_patch.rs +++ b/codex-rs/core/src/apply_patch.rs @@ -1,6 +1,7 @@ use crate::codex::TurnContext; use crate::function_tool::FunctionCallError; use crate::protocol::FileChange; +use crate::protocol::FileSystemSandboxPolicy; use crate::safety::SafetyCheck; use crate::safety::assess_patch_safety; use crate::tools::sandboxing::ExecApprovalRequirement; @@ -34,13 +35,14 @@ pub(crate) struct ApplyPatchExec { pub(crate) async fn apply_patch( turn_context: &TurnContext, + file_system_sandbox_policy: &FileSystemSandboxPolicy, action: ApplyPatchAction, ) -> InternalApplyPatchInvocation { match assess_patch_safety( &action, turn_context.approval_policy.value(), turn_context.sandbox_policy.get(), - &turn_context.file_system_sandbox_policy, + file_system_sandbox_policy, &turn_context.cwd, turn_context.windows_sandbox_level, ) { diff --git a/codex-rs/core/src/sandboxing/mod.rs b/codex-rs/core/src/sandboxing/mod.rs index a3a5617af6..9d0912faa1 100644 --- a/codex-rs/core/src/sandboxing/mod.rs +++ b/codex-rs/core/src/sandboxing/mod.rs @@ -388,6 +388,26 @@ fn merge_file_system_policy_with_additional_permissions( } } +pub(crate) fn effective_file_system_sandbox_policy( + file_system_policy: &FileSystemSandboxPolicy, + additional_permissions: Option<&PermissionProfile>, +) -> FileSystemSandboxPolicy { + let Some(additional_permissions) = additional_permissions else { + return file_system_policy.clone(); + }; + + let (extra_reads, extra_writes) = additional_permission_roots(additional_permissions); + if extra_reads.is_empty() && extra_writes.is_empty() { + file_system_policy.clone() + } else { + merge_file_system_policy_with_additional_permissions( + file_system_policy, + extra_reads, + extra_writes, + ) + } +} + fn merge_read_only_access_with_additional_reads( read_only_access: &ReadOnlyAccess, extra_reads: Vec, @@ -587,18 +607,10 @@ impl SandboxManager { ); let (effective_file_system_policy, effective_network_policy) = if let Some(additional_permissions) = additional_permissions { - let (extra_reads, extra_writes) = - additional_permission_roots(&additional_permissions); - let file_system_sandbox_policy = - if extra_reads.is_empty() && extra_writes.is_empty() { - file_system_policy.clone() - } else { - merge_file_system_policy_with_additional_permissions( - file_system_policy, - extra_reads, - extra_writes, - ) - }; + let file_system_sandbox_policy = effective_file_system_sandbox_policy( + file_system_policy, + Some(&additional_permissions), + ); let network_sandbox_policy = if merge_network_access(network_policy.is_enabled(), &additional_permissions) { NetworkSandboxPolicy::Enabled @@ -721,6 +733,7 @@ mod tests { #[cfg(target_os = "macos")] use super::EffectiveSandboxPermissions; use super::SandboxManager; + use super::effective_file_system_sandbox_policy; #[cfg(target_os = "macos")] use super::intersect_permission_profiles; use super::merge_file_system_policy_with_additional_permissions; @@ -1364,4 +1377,80 @@ mod tests { true ); } + + #[test] + fn effective_file_system_sandbox_policy_returns_base_policy_without_additional_permissions() { + let temp_dir = TempDir::new().expect("create temp dir"); + let cwd = AbsolutePathBuf::from_absolute_path( + canonicalize(temp_dir.path()).expect("canonicalize temp dir"), + ) + .expect("absolute temp dir"); + let denied_path = cwd.join("denied").expect("denied path"); + let base_policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: denied_path }, + access: FileSystemAccessMode::None, + }, + ]); + + let effective_policy = effective_file_system_sandbox_policy(&base_policy, None); + + assert_eq!(effective_policy, base_policy); + } + + #[test] + fn effective_file_system_sandbox_policy_merges_additional_write_roots() { + let temp_dir = TempDir::new().expect("create temp dir"); + let cwd = AbsolutePathBuf::from_absolute_path( + canonicalize(temp_dir.path()).expect("canonicalize temp dir"), + ) + .expect("absolute temp dir"); + let allowed_path = cwd.join("allowed").expect("allowed path"); + let denied_path = cwd.join("denied").expect("denied path"); + let base_policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: denied_path.clone(), + }, + access: FileSystemAccessMode::None, + }, + ]); + let additional_permissions = PermissionProfile { + file_system: Some(FileSystemPermissions { + read: Some(vec![]), + write: Some(vec![allowed_path.clone()]), + }), + ..Default::default() + }; + + let effective_policy = + effective_file_system_sandbox_policy(&base_policy, Some(&additional_permissions)); + + assert_eq!( + effective_policy.entries.contains(&FileSystemSandboxEntry { + path: FileSystemPath::Path { path: denied_path }, + access: FileSystemAccessMode::None, + }), + true + ); + assert_eq!( + effective_policy.entries.contains(&FileSystemSandboxEntry { + path: FileSystemPath::Path { path: allowed_path }, + access: FileSystemAccessMode::Write, + }), + true + ); + } } diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 3cbbbd508d..24119cd23b 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -11,6 +11,8 @@ use crate::client_common::tools::ToolSpec; use crate::codex::Session; use crate::codex::TurnContext; use crate::function_tool::FunctionCallError; +use crate::sandboxing::effective_file_system_sandbox_policy; +use crate::sandboxing::merge_permission_profiles; use crate::tools::context::FunctionToolOutput; use crate::tools::context::SharedTurnDiffTracker; use crate::tools::context::ToolInvocation; @@ -89,6 +91,38 @@ fn write_permissions_for_paths(file_paths: &[AbsolutePathBuf]) -> Option ( + Vec, + crate::tools::handlers::EffectiveAdditionalPermissions, + codex_protocol::permissions::FileSystemSandboxPolicy, +) { + let file_paths = file_paths_for_action(action); + let granted_permissions = merge_permission_profiles( + session.granted_session_permissions().await.as_ref(), + session.granted_turn_permissions().await.as_ref(), + ); + let effective_additional_permissions = apply_granted_turn_permissions( + session, + crate::sandboxing::SandboxPermissions::UseDefault, + write_permissions_for_paths(&file_paths), + ) + .await; + let file_system_sandbox_policy = effective_file_system_sandbox_policy( + &turn.file_system_sandbox_policy, + granted_permissions.as_ref(), + ); + + ( + file_paths, + effective_additional_permissions, + file_system_sandbox_policy, + ) +} + #[async_trait] impl ToolHandler for ApplyPatchHandler { type Output = FunctionToolOutput; @@ -138,20 +172,17 @@ impl ToolHandler for ApplyPatchHandler { let command = vec!["apply_patch".to_string(), patch_input.clone()]; match codex_apply_patch::maybe_parse_apply_patch_verified(&command, &cwd) { codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => { - match apply_patch::apply_patch(turn.as_ref(), changes).await { + let (file_paths, effective_additional_permissions, file_system_sandbox_policy) = + effective_patch_permissions(session.as_ref(), turn.as_ref(), &changes).await; + match apply_patch::apply_patch(turn.as_ref(), &file_system_sandbox_policy, changes) + .await + { InternalApplyPatchInvocation::Output(item) => { let content = item?; Ok(FunctionToolOutput::from_text(content, Some(true))) } InternalApplyPatchInvocation::DelegateToExec(apply) => { let changes = convert_apply_patch_to_protocol(&apply.action); - let file_paths = file_paths_for_action(&apply.action); - let effective_additional_permissions = apply_granted_turn_permissions( - session.as_ref(), - crate::sandboxing::SandboxPermissions::UseDefault, - write_permissions_for_paths(&file_paths), - ) - .await; let emitter = ToolEmitter::apply_patch(changes.clone(), apply.auto_approved); let event_ctx = ToolEventCtx::new( @@ -247,20 +278,17 @@ pub(crate) async fn intercept_apply_patch( turn.as_ref(), ) .await; - match apply_patch::apply_patch(turn.as_ref(), changes).await { + let (approval_keys, effective_additional_permissions, file_system_sandbox_policy) = + effective_patch_permissions(session.as_ref(), turn.as_ref(), &changes).await; + match apply_patch::apply_patch(turn.as_ref(), &file_system_sandbox_policy, changes) + .await + { InternalApplyPatchInvocation::Output(item) => { let content = item?; Ok(Some(FunctionToolOutput::from_text(content, Some(true)))) } InternalApplyPatchInvocation::DelegateToExec(apply) => { let changes = convert_apply_patch_to_protocol(&apply.action); - let approval_keys = file_paths_for_action(&apply.action); - let effective_additional_permissions = apply_granted_turn_permissions( - session.as_ref(), - crate::sandboxing::SandboxPermissions::UseDefault, - write_permissions_for_paths(&approval_keys), - ) - .await; let emitter = ToolEmitter::apply_patch(changes.clone(), apply.auto_approved); let event_ctx = ToolEventCtx::new( session.as_ref(),