safety: honor filesystem policy carveouts in apply_patch

codex-rs/core/src/apply_patch.rs

@@ -40,6 +40,7 @@ pub(crate) async fn apply_patch(
         &action,
         turn_context.approval_policy.value(),
         turn_context.sandbox_policy.get(),
+        &turn_context.file_system_sandbox_policy,
         &turn_context.cwd,
         turn_context.windows_sandbox_level,
     ) {

codex-rs/core/src/safety.rs

@@ -9,6 +9,7 @@ use crate::exec::SandboxType;
 use crate::util::resolve_path;
 
 use crate::protocol::AskForApproval;
+use crate::protocol::FileSystemSandboxPolicy;
 use crate::protocol::SandboxPolicy;
 use codex_protocol::config_types::WindowsSandboxLevel;
 
@@ -28,6 +29,7 @@ pub fn assess_patch_safety(
     action: &ApplyPatchAction,
     policy: AskForApproval,
     sandbox_policy: &SandboxPolicy,
+    file_system_sandbox_policy: &FileSystemSandboxPolicy,
     cwd: &Path,
     windows_sandbox_level: WindowsSandboxLevel,
 ) -> SafetyCheck {
@@ -60,7 +62,7 @@ pub fn assess_patch_safety(
     // 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, sandbox_policy, cwd)
+    if is_write_patch_constrained_to_writable_paths(action, file_system_sandbox_policy, cwd)
         || matches!(policy, AskForApproval::OnFailure)
     {
         if matches!(
@@ -122,20 +124,9 @@ pub fn get_platform_sandbox(windows_sandbox_enabled: bool) -> Option<SandboxType
 
 fn is_write_patch_constrained_to_writable_paths(
     action: &ApplyPatchAction,
-    sandbox_policy: &SandboxPolicy,
+    file_system_sandbox_policy: &FileSystemSandboxPolicy,
     cwd: &Path,
 ) -> bool {
-    // Early‑exit if there are no declared writable roots.
-    let writable_roots = match sandbox_policy {
-        SandboxPolicy::ReadOnly { .. } => {
-            return false;
-        }
-        SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => {
-            return true;
-        }
-        SandboxPolicy::WorkspaceWrite { .. } => sandbox_policy.get_writable_roots_with_cwd(cwd),
-    };
-
     // Normalize a path by removing `.` and resolving `..` without touching the
     // filesystem (works even if the file does not exist).
     fn normalize(path: &Path) -> Option<PathBuf> {
@@ -152,6 +143,9 @@ 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.
@@ -162,6 +156,17 @@ 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))
@@ -193,6 +198,10 @@ fn is_write_patch_constrained_to_writable_paths(
 #[cfg(test)]
 mod tests {
     use super::*;
+    use codex_protocol::protocol::FileSystemAccessMode;
+    use codex_protocol::protocol::FileSystemPath;
+    use codex_protocol::protocol::FileSystemSandboxEntry;
+    use codex_protocol::protocol::FileSystemSpecialPath;
     use codex_protocol::protocol::RejectConfig;
     use codex_utils_absolute_path::AbsolutePathBuf;
     use tempfile::TempDir;
@@ -223,13 +232,13 @@ mod tests {
 
         assert!(is_write_patch_constrained_to_writable_paths(
             &add_inside,
-            &policy_workspace_only,
+            &FileSystemSandboxPolicy::from(&policy_workspace_only),
             &cwd,
         ));
 
         assert!(!is_write_patch_constrained_to_writable_paths(
             &add_outside,
-            &policy_workspace_only,
+            &FileSystemSandboxPolicy::from(&policy_workspace_only),
             &cwd,
         ));
 
@@ -244,7 +253,7 @@ mod tests {
         };
         assert!(is_write_patch_constrained_to_writable_paths(
             &add_outside,
-            &policy_with_parent,
+            &FileSystemSandboxPolicy::from(&policy_with_parent),
             &cwd,
         ));
     }
@@ -264,6 +273,7 @@ mod tests {
                 &add_inside,
                 AskForApproval::OnRequest,
                 &policy,
+                &FileSystemSandboxPolicy::from(&policy),
                 &cwd,
                 WindowsSandboxLevel::Disabled
             ),
@@ -294,6 +304,7 @@ mod tests {
                 &add_outside,
                 AskForApproval::OnRequest,
                 &policy_workspace_only,
+                &FileSystemSandboxPolicy::from(&policy_workspace_only),
                 &cwd,
                 WindowsSandboxLevel::Disabled,
             ),
@@ -308,6 +319,7 @@ mod tests {
                     mcp_elicitations: false,
                 }),
                 &policy_workspace_only,
+                &FileSystemSandboxPolicy::from(&policy_workspace_only),
                 &cwd,
                 WindowsSandboxLevel::Disabled,
             ),
@@ -339,6 +351,7 @@ mod tests {
                     mcp_elicitations: false,
                 }),
                 &policy_workspace_only,
+                &FileSystemSandboxPolicy::from(&policy_workspace_only),
                 &cwd,
                 WindowsSandboxLevel::Disabled,
             ),
@@ -348,4 +361,47 @@ mod tests {
             },
         );
     }
+
+    #[test]
+    fn explicit_unreadable_paths_prevent_auto_approval_for_external_sandbox() {
+        let tmp = TempDir::new().unwrap();
+        let cwd = tmp.path().to_path_buf();
+        let blocked_path = cwd.join("blocked.txt");
+        let blocked_absolute = AbsolutePathBuf::from_absolute_path(blocked_path.clone()).unwrap();
+        let action = ApplyPatchAction::new_add_for_test(&blocked_path, "".to_string());
+        let sandbox_policy = SandboxPolicy::ExternalSandbox {
+            network_access: codex_protocol::protocol::NetworkAccess::Restricted,
+        };
+        let file_system_sandbox_policy = FileSystemSandboxPolicy::restricted(vec![
+            FileSystemSandboxEntry {
+                path: FileSystemPath::Special {
+                    value: FileSystemSpecialPath::Root,
+                },
+                access: FileSystemAccessMode::Write,
+            },
+            FileSystemSandboxEntry {
+                path: FileSystemPath::Path {
+                    path: blocked_absolute,
+                },
+                access: FileSystemAccessMode::None,
+            },
+        ]);
+
+        assert!(!is_write_patch_constrained_to_writable_paths(
+            &action,
+            &file_system_sandbox_policy,
+            &cwd,
+        ));
+        assert_eq!(
+            assess_patch_safety(
+                &action,
+                AskForApproval::OnRequest,
+                &sandbox_policy,
+                &file_system_sandbox_policy,
+                &cwd,
+                WindowsSandboxLevel::Disabled,
+            ),
+            SafetyCheck::AskUser,
+        );
+    }
 }