From 0c70698e24e37cfdb2578cbeec0d981f656dc734 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Sat, 9 May 2026 08:28:15 -0700 Subject: [PATCH] tests: cover sandbox link write behavior (#21819) ## Why [PR #1705](https://github.com/openai/codex/pull/1705) moved `apply_patch` execution under the configured sandbox and called out the need for integration coverage. We already covered textual `../` escapes, but did not have coverage for link aliases that live inside a writable workspace while pointing at, or aliasing, files visible outside it. This PR locks in the current sandbox boundary without changing production write semantics. Symlink escapes into a read-only outside root should fail and leave the outside file unchanged. Existing hard links are characterized separately: if a user-created hard link already exists inside the writable root, sandboxed writes preserve normal hard-link semantics rather than replacing the link and silently breaking that relationship. ## What Changed - Added `apply_patch_cli_does_not_write_through_symlink_escape_outside_workspace` to verify `apply_patch` cannot update a symlink that targets a file outside the writable workspace. - Added `apply_patch_cli_preserves_existing_hard_link_outside_workspace` to verify `apply_patch` intentionally writes through an existing hard link and does not unlink or replace it. - Added `file_system_sandboxed_write_preserves_existing_hard_link` to verify sandboxed `fs/writeFile` preserves an existing hard link and writes the shared inode. ## Testing - `cargo test -p codex-exec-server file_system_sandboxed_write` - `cargo test -p codex-core apply_patch_cli_does_not_write_through_symlink_escape_outside_workspace` - `cargo test -p codex-core apply_patch_cli_preserves_existing_hard_link_outside_workspace` - `just fix -p codex-exec-server -p codex-core` - `just fix -p codex-core` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/21819). * #21845 * __->__ #21819 --- codex-rs/core/tests/common/test_codex.rs | 9 +- codex-rs/core/tests/suite/apply_patch_cli.rs | 220 +++++++++++++++++++ codex-rs/exec-server/tests/file_system.rs | 48 ++++ 3 files changed, 276 insertions(+), 1 deletion(-) diff --git a/codex-rs/core/tests/common/test_codex.rs b/codex-rs/core/tests/common/test_codex.rs index bcc93d7d09..026c84ee2c 100644 --- a/codex-rs/core/tests/common/test_codex.rs +++ b/codex-rs/core/tests/common/test_codex.rs @@ -383,9 +383,16 @@ impl TestCodexBuilder { .exec_server_url .clone() .or_else(|| test_env.exec_server_url.clone()); + #[cfg(target_os = "linux")] + let codex_linux_sandbox_exe = Some( + crate::find_codex_linux_sandbox_exe() + .context("should find binary for codex-linux-sandbox")?, + ); + #[cfg(not(target_os = "linux"))] + let codex_linux_sandbox_exe = None; let local_runtime_paths = codex_exec_server::ExecServerRuntimePaths::new( std::env::current_exe()?, - /*codex_linux_sandbox_exe*/ None, + codex_linux_sandbox_exe, )?; let environment_manager = Arc::new( codex_exec_server::EnvironmentManager::create_for_tests( diff --git a/codex-rs/core/tests/suite/apply_patch_cli.rs b/codex-rs/core/tests/suite/apply_patch_cli.rs index 3f85d31c7c..f95fd1efef 100644 --- a/codex-rs/core/tests/suite/apply_patch_cli.rs +++ b/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_assistant_message; use core_test_support::responses::ev_completed; @@ -112,6 +118,45 @@ 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, + ) +} + +#[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, @@ -675,6 +720,181 @@ async fn apply_patch_cli_rejects_path_traversal_outside_workspace( Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[test_case(ApplyPatchModelOutput::Freeform ; "freeform")] +#[test_case(ApplyPatchModelOutput::Shell ; "shell")] +#[test_case(ApplyPatchModelOutput::ShellViaHeredoc ; "shell_heredoc")] +#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc ; "shell_command_heredoc")] +async fn apply_patch_cli_does_not_write_through_symlink_escape_outside_workspace( + model_output: ApplyPatchModelOutput, +) -> 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 = "soft-link.txt"; + let link_path = harness.path(link_rel); + match create_file_symlink(&outside_file, &link_path) { + Ok(()) => {} + Err(error) if cfg!(windows) => { + eprintln!("Skipping Windows symlink apply_patch sandbox test: {error}"); + return Ok(()); + } + Err(error) => return Err(error.into()), + } + + let patch = format!( + r#"*** Begin Patch +*** Update File: {link_rel} +@@ +-original outside content ++pwned +*** End Patch"# + ); + let call_id = "apply-symlink-escape"; + 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, + "symlink escape should not modify the outside victim; tool output: {out}", + ); + let metadata = std::fs::symlink_metadata(&link_path)?; + assert!(metadata.file_type().is_symlink()); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[test_case(ApplyPatchModelOutput::Freeform ; "freeform")] +#[test_case(ApplyPatchModelOutput::Shell ; "shell")] +#[test_case(ApplyPatchModelOutput::ShellViaHeredoc ; "shell_heredoc")] +#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc ; "shell_command_heredoc")] +async fn apply_patch_cli_preserves_existing_hard_link_outside_workspace( + model_output: ApplyPatchModelOutput, +) -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_remote!( + Ok(()), + "link setup needs local filesystem hard 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 outside_file = outside_dir.join("victim.txt"); + std::fs::write(&outside_file, "original outside content\n")?; + + let link_rel = "hard-link.txt"; + let link_path = harness.path(link_rel); + std::fs::hard_link(&outside_file, &link_path)?; + + let patch = format!( + r#"*** Begin Patch +*** Update File: {link_rel} +@@ +-original outside content ++updated through existing hard link +*** End Patch"# + ); + let call_id = "apply-hard-link"; + mount_apply_patch(&harness, call_id, &patch, "ok", model_output).await; + + harness + .submit_with_permission_profile( + "update existing hard link via apply_patch", + workspace_write_with_read_only_root(outside_dir.clone()), + ) + .await?; + + let out = harness.apply_patch_output(call_id, model_output).await; + if cfg!(windows) { + assert!( + out.contains("patch rejected: writing outside of the project"), + "Windows sandboxing intentionally rejects writes through existing hard links to files outside the workspace; tool output: {out}" + ); + assert_eq!( + std::fs::read_to_string(&outside_file)?, + "original outside content\n", + "Windows rejection must leave the outside hard-link target unchanged" + ); + assert_eq!( + std::fs::read_to_string(&link_path)?, + "original outside content\n", + "Windows rejection must leave the workspace hard-link path unchanged" + ); + + std::fs::write(&outside_file, "post-reject outside write\n")?; + assert_eq!( + std::fs::read_to_string(&link_path)?, + "post-reject outside write\n", + "Windows rejection must not unlink or replace an existing hard link" + ); + + return Ok(()); + } + + assert!( + out.contains("Success. Updated the following files:"), + "apply_patch should intentionally allow updates through existing hard links; tool output: {out}" + ); + assert_eq!( + std::fs::read_to_string(&outside_file)?, + "updated through existing hard link\n", + "apply_patch intentionally preserves existing hard-link semantics; the outside path observes the shared inode update" + ); + assert_eq!( + std::fs::read_to_string(&link_path)?, + "updated through existing hard link\n", + "apply_patch intentionally preserves existing hard-link semantics; the workspace path observes the same update" + ); + + std::fs::write(&outside_file, "post-apply outside write\n")?; + assert_eq!( + std::fs::read_to_string(&link_path)?, + "post-apply outside write\n", + "apply_patch must not unlink or replace an existing hard link; later writes through either path should still be visible" + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Shell)] diff --git a/codex-rs/exec-server/tests/file_system.rs b/codex-rs/exec-server/tests/file_system.rs index 0840b2a909..3da796df24 100644 --- a/codex-rs/exec-server/tests/file_system.rs +++ b/codex-rs/exec-server/tests/file_system.rs @@ -2,6 +2,7 @@ mod common; +use std::os::unix::fs::MetadataExt; #[cfg(target_os = "linux")] use std::os::unix::fs::PermissionsExt; use std::os::unix::fs::symlink; @@ -732,6 +733,53 @@ async fn file_system_sandboxed_write_rejects_symlink_escape(use_remote: bool) -> Ok(()) } +#[test_case(false ; "local")] +#[test_case(true ; "remote")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn file_system_sandboxed_write_preserves_existing_hard_link(use_remote: bool) -> Result<()> { + let context = create_file_system_context(use_remote).await?; + let file_system = context.file_system; + + let tmp = TempDir::new()?; + let allowed_dir = tmp.path().join("allowed"); + let outside_dir = tmp.path().join("outside"); + std::fs::create_dir_all(&allowed_dir)?; + std::fs::create_dir_all(&outside_dir)?; + + let outside_file = outside_dir.join("outside.txt"); + let hard_link = allowed_dir.join("hard-link.txt"); + std::fs::write(&outside_file, "outside\n")?; + std::fs::hard_link(&outside_file, &hard_link)?; + + let sandbox = workspace_write_sandbox(allowed_dir); + file_system + .write_file( + &absolute_path(hard_link.clone()), + b"updated through existing hard link\n".to_vec(), + Some(&sandbox), + ) + .await + .with_context(|| format!("mode={use_remote}"))?; + + assert_eq!( + std::fs::read_to_string(&outside_file)?, + "updated through existing hard link\n" + ); + assert_eq!( + std::fs::read_to_string(&hard_link)?, + "updated through existing hard link\n" + ); + + let outside_metadata = std::fs::metadata(&outside_file)?; + let link_metadata = std::fs::metadata(&hard_link)?; + assert_eq!( + (link_metadata.dev(), link_metadata.ino()), + (outside_metadata.dev(), outside_metadata.ino()) + ); + + Ok(()) +} + #[test_case(false ; "local")] #[test_case(true ; "remote")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)]