Use granted permissions when invoking apply_patch (openai#14429)

Author: mousseau-oai

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,
     ) {

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<AbsolutePathBuf>,
@@ -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
+        );
+    }
 }

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<Permiss
     crate::sandboxing::normalize_additional_permissions(permissions).ok()
 }
 
+async fn effective_patch_permissions(
+    session: &Session,
+    turn: &TurnContext,
+    action: &ApplyPatchAction,
+) -> (
+    Vec<AbsolutePathBuf>,
+    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(),