apply-patch: avoid sandbox link write-through

codex-rs/core/tests/suite/apply_patch_cli.rs

@@ -15,6 +15,11 @@ use std::time::Duration;
 use codex_exec_server::CreateDirectoryOptions;
 use codex_features::Feature;
 use codex_protocol::models::PermissionProfile;
+use codex_protocol::permissions::FileSystemAccessMode;
+use codex_protocol::permissions::FileSystemPath;
+use codex_protocol::permissions::FileSystemSandboxEntry;
+use codex_protocol::permissions::FileSystemSandboxPolicy;
+use codex_protocol::permissions::FileSystemSpecialPath;
 use codex_protocol::permissions::NetworkSandboxPolicy;
 use codex_protocol::protocol::AskForApproval;
 use codex_protocol::protocol::EventMsg;
@@ -23,6 +28,7 @@ use codex_protocol::protocol::SandboxPolicy;
 use codex_protocol::user_input::UserInput;
 #[cfg(target_os = "linux")]
 use codex_sandboxing::landlock::CODEX_LINUX_SANDBOX_ARG0;
+use codex_utils_absolute_path::AbsolutePathBuf;
 use core_test_support::assert_regex_match;
 use core_test_support::responses::ev_apply_patch_function_call;
 use core_test_support::responses::ev_assistant_message;
@@ -113,6 +119,67 @@ fn restrictive_workspace_write_profile() -> PermissionProfile {
     )
 }
 
+fn workspace_write_with_read_only_root(read_only_root: AbsolutePathBuf) -> PermissionProfile {
+    let file_system_sandbox_policy = FileSystemSandboxPolicy::restricted(vec![
+        FileSystemSandboxEntry {
+            path: FileSystemPath::Path {
+                path: read_only_root,
+            },
+            access: FileSystemAccessMode::Read,
+        },
+        FileSystemSandboxEntry {
+            path: FileSystemPath::Special {
+                value: FileSystemSpecialPath::project_roots(/*subpath*/ None),
+            },
+            access: FileSystemAccessMode::Write,
+        },
+    ]);
+    PermissionProfile::from_runtime_permissions(
+        &file_system_sandbox_policy,
+        NetworkSandboxPolicy::Restricted,
+    )
+}
+
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+enum WorkspaceLinkKind {
+    Soft,
+    Hard,
+}
+
+impl WorkspaceLinkKind {
+    fn name(self) -> &'static str {
+        match self {
+            WorkspaceLinkKind::Soft => "soft",
+            WorkspaceLinkKind::Hard => "hard",
+        }
+    }
+
+    fn create(self, source: &std::path::Path, link: &std::path::Path) -> std::io::Result<()> {
+        match self {
+            WorkspaceLinkKind::Soft => create_file_symlink(source, link),
+            WorkspaceLinkKind::Hard => std::fs::hard_link(source, link),
+        }
+    }
+}
+
+#[cfg(unix)]
+fn create_file_symlink(source: &std::path::Path, link: &std::path::Path) -> std::io::Result<()> {
+    std::os::unix::fs::symlink(source, link)
+}
+
+#[cfg(windows)]
+fn create_file_symlink(source: &std::path::Path, link: &std::path::Path) -> std::io::Result<()> {
+    std::os::windows::fs::symlink_file(source, link)
+}
+
+#[cfg(not(any(unix, windows)))]
+fn create_file_symlink(_source: &std::path::Path, _link: &std::path::Path) -> std::io::Result<()> {
+    Err(std::io::Error::new(
+        std::io::ErrorKind::Unsupported,
+        "file symlinks are unsupported on this platform",
+    ))
+}
+
 pub async fn mount_apply_patch(
     harness: &TestCodexHarness,
     call_id: &str,
@@ -691,6 +758,76 @@ async fn apply_patch_cli_rejects_path_traversal_outside_workspace(
     Ok(())
 }
 
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+#[test_case(ApplyPatchModelOutput::Freeform, WorkspaceLinkKind::Soft ; "freeform_soft_link")]
+#[test_case(ApplyPatchModelOutput::Function, WorkspaceLinkKind::Soft ; "function_soft_link")]
+#[test_case(ApplyPatchModelOutput::Shell, WorkspaceLinkKind::Soft ; "shell_soft_link")]
+#[test_case(ApplyPatchModelOutput::ShellViaHeredoc, WorkspaceLinkKind::Soft ; "shell_heredoc_soft_link")]
+#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc, WorkspaceLinkKind::Soft ; "shell_command_heredoc_soft_link")]
+#[test_case(ApplyPatchModelOutput::Freeform, WorkspaceLinkKind::Hard ; "freeform_hard_link")]
+#[test_case(ApplyPatchModelOutput::Function, WorkspaceLinkKind::Hard ; "function_hard_link")]
+#[test_case(ApplyPatchModelOutput::Shell, WorkspaceLinkKind::Hard ; "shell_hard_link")]
+#[test_case(ApplyPatchModelOutput::ShellViaHeredoc, WorkspaceLinkKind::Hard ; "shell_heredoc_hard_link")]
+#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc, WorkspaceLinkKind::Hard ; "shell_command_heredoc_hard_link")]
+async fn apply_patch_cli_does_not_write_through_link_escape_outside_workspace(
+    model_output: ApplyPatchModelOutput,
+    link_kind: WorkspaceLinkKind,
+) -> Result<()> {
+    skip_if_no_network!(Ok(()));
+    skip_if_remote!(
+        Ok(()),
+        "link escape setup needs local filesystem link creation"
+    );
+
+    let test_root = tempfile::tempdir_in(std::env::current_dir()?)?;
+    let work_dir = AbsolutePathBuf::try_from(test_root.path().join("work"))?;
+    let outside_dir = AbsolutePathBuf::try_from(test_root.path().join("outside"))?;
+    std::fs::create_dir_all(work_dir.as_path())?;
+    std::fs::create_dir_all(outside_dir.as_path())?;
+
+    let harness_work_dir = work_dir.clone();
+    let harness = apply_patch_harness_with(move |builder| {
+        builder.with_config(move |config| {
+            config.cwd = harness_work_dir;
+        })
+    })
+    .await?;
+    let original_contents = "original outside content\n";
+    let outside_file = outside_dir.join("victim.txt");
+    std::fs::write(&outside_file, original_contents)?;
+
+    let link_rel = format!("{}-link.txt", link_kind.name());
+    let link_path = harness.path(&link_rel);
+    match link_kind.create(&outside_file, &link_path) {
+        Ok(()) => {}
+        Err(error) if link_kind == WorkspaceLinkKind::Soft && cfg!(windows) => {
+            eprintln!("Skipping Windows symlink apply_patch sandbox test: {error}");
+            return Ok(());
+        }
+        Err(error) => return Err(error.into()),
+    }
+
+    let patch = format!("*** Begin Patch\n*** Add File: {link_rel}\n+pwned\n*** End Patch");
+    let call_id = format!("apply-{}-link-escape", link_kind.name());
+    mount_apply_patch(&harness, &call_id, &patch, "fail", model_output).await;
+
+    harness
+        .submit_with_permission_profile(
+            "attempt to escape workspace via apply_patch link",
+            workspace_write_with_read_only_root(outside_dir.clone()),
+        )
+        .await?;
+
+    let out = harness.apply_patch_output(&call_id, model_output).await;
+    assert_eq!(
+        std::fs::read_to_string(&outside_file)?,
+        original_contents,
+        "{:?} link escape should not modify the outside victim; tool output: {out}",
+        link_kind
+    );
+    Ok(())
+}
+
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 #[test_case(ApplyPatchModelOutput::Freeform)]
 #[test_case(ApplyPatchModelOutput::Function)]

codex-rs/exec-server/src/fs_helper.rs

@@ -3,7 +3,11 @@ use base64::engine::general_purpose::STANDARD;
 use codex_app_server_protocol::JSONRPCErrorError;
 use serde::Deserialize;
 use serde::Serialize;
+use std::path::PathBuf;
+use std::sync::atomic::AtomicU64;
+use std::sync::atomic::Ordering;
 use tokio::io;
+use tokio::io::AsyncWriteExt;
 
 use crate::CopyOptions;
 use crate::CreateDirectoryOptions;
@@ -37,6 +41,7 @@ use crate::rpc::invalid_request;
 use crate::rpc::not_found;
 
 pub const CODEX_FS_HELPER_ARG1: &str = "--codex-run-as-fs-helper";
+static TEMP_FILE_COUNTER: AtomicU64 = AtomicU64::new(0);
 
 #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
 #[serde(tag = "operation", content = "params")]
@@ -185,8 +190,7 @@ pub(crate) async fn run_direct_request(
                     "{FS_WRITE_FILE_METHOD} requires valid base64 dataBase64: {err}"
                 ))
             })?;
-            file_system
-                .write_file(&params.path, bytes, /*sandbox*/ None)
+            write_file_by_replacing_path(&params.path, bytes)
                 .await
                 .map_err(map_fs_error)?;
             Ok(FsHelperPayload::WriteFile(FsWriteFileResponse {}))
@@ -266,6 +270,84 @@ pub(crate) async fn run_direct_request(
     }
 }
 
+/// The helper is already running inside the platform sandbox. Write through a
+/// fresh file and rename it into place so an existing final-path symlink or
+/// hard link is replaced rather than used as the write target.
+async fn write_file_by_replacing_path(
+    path: &codex_utils_absolute_path::AbsolutePathBuf,
+    bytes: Vec<u8>,
+) -> io::Result<()> {
+    let parent = path
+        .parent()
+        .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, "path has no parent"))?;
+    if path.as_path().file_name().is_none() {
+        return Err(io::Error::new(
+            io::ErrorKind::InvalidInput,
+            "path has no file name",
+        ));
+    }
+
+    let (temp_path, mut temp_file) = loop {
+        let counter = TEMP_FILE_COUNTER.fetch_add(1, Ordering::Relaxed);
+        let temp_name = format!(".codex-write-{}-{counter}.tmp", std::process::id());
+        let temp_path = parent.join(PathBuf::from(temp_name));
+        match tokio::fs::OpenOptions::new()
+            .write(true)
+            .create_new(true)
+            .open(temp_path.as_path())
+            .await
+        {
+            Ok(file) => break (temp_path, file),
+            Err(err) if err.kind() == io::ErrorKind::AlreadyExists => continue,
+            Err(err) => return Err(err),
+        }
+    };
+
+    let write_result = async {
+        temp_file.write_all(&bytes).await?;
+        temp_file.flush().await
+    }
+    .await;
+    drop(temp_file);
+    if let Err(err) = write_result {
+        let _ = tokio::fs::remove_file(temp_path.as_path()).await;
+        return Err(err);
+    }
+
+    if let Ok(metadata) = tokio::fs::metadata(path.as_path()).await {
+        let _ = tokio::fs::set_permissions(temp_path.as_path(), metadata.permissions()).await;
+    }
+
+    if let Err(err) = replace_with_temp_file(&temp_path, path).await {
+        let _ = tokio::fs::remove_file(temp_path.as_path()).await;
+        return Err(err);
+    }
+    Ok(())
+}
+
+#[cfg(not(windows))]
+async fn replace_with_temp_file(
+    temp_path: &codex_utils_absolute_path::AbsolutePathBuf,
+    path: &codex_utils_absolute_path::AbsolutePathBuf,
+) -> io::Result<()> {
+    tokio::fs::rename(temp_path.as_path(), path.as_path()).await
+}
+
+#[cfg(windows)]
+async fn replace_with_temp_file(
+    temp_path: &codex_utils_absolute_path::AbsolutePathBuf,
+    path: &codex_utils_absolute_path::AbsolutePathBuf,
+) -> io::Result<()> {
+    match tokio::fs::rename(temp_path.as_path(), path.as_path()).await {
+        Ok(()) => Ok(()),
+        Err(err) if err.kind() == io::ErrorKind::AlreadyExists => {
+            tokio::fs::remove_file(path.as_path()).await?;
+            tokio::fs::rename(temp_path.as_path(), path.as_path()).await
+        }
+        Err(err) => Err(err),
+    }
+}
+
 fn map_fs_error(err: io::Error) -> JSONRPCErrorError {
     match err.kind() {
         io::ErrorKind::NotFound => not_found(err.to_string()),