From 530c1aed58b88060cfe851005c9dae3a89f44ad0 Mon Sep 17 00:00:00 2001 From: sayan-oai Date: Wed, 19 Aug 2026 20:47:30 +0000 Subject: [PATCH] Prevent `apply_patch` from widening write permissions (#39614) ## Why Deriving permissions from the parent of an already-writable patch target can unnecessarily grant write access outside the intended workspace. ## What changed Skip targets already covered by the active filesystem sandbox policy before deriving additional parent-directory permissions. Targets outside the writable area continue to request the required parent access. ## Testing Added unit and CLI regression coverage for workspace-directory targets, already-writable parents, and symlink escapes outside the workspace. GitOrigin-RevId: 187109ff0b56a1a399cb8a9981b7e822977025d5 --- .../core/src/tools/handlers/apply_patch.rs | 5 ++ .../src/tools/handlers/apply_patch_tests.rs | 34 ++++++++++ codex-rs/core/tests/suite/apply_patch_cli.rs | 65 +++++++++++++++++++ 3 files changed, 104 insertions(+) diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 36ae51ba7b..c57302c4ce 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -239,6 +239,11 @@ fn write_permissions_for_paths( ) -> Option { let write_paths = file_paths .iter() + // Skip already-writable targets before deriving parent permissions. + // Otherwise, a writable directory could grant access to its parent. + .filter(|path| { + !file_system_sandbox_policy.can_write_path_with_cwd(path.as_path(), cwd.as_path()) + }) .map(|path| { path.parent() .unwrap_or_else(|| path.clone()) diff --git a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs index c964846bc1..b78e62e2da 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs @@ -1,6 +1,8 @@ use super::*; use codex_apply_patch::MaybeApplyPatchVerified; use codex_exec_server::LOCAL_FS; +use codex_protocol::permissions::FileSystemAccessMode; +use codex_protocol::permissions::FileSystemSandboxEntry; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::protocol::FileChange; use core_test_support::PathBufExt; @@ -312,3 +314,35 @@ fn write_permissions_for_paths_keep_dirs_outside_workspace_root() { Some(vec![expected_outside]) ); } + +#[test] +fn write_permissions_for_paths_do_not_widen_workspace_root_target() { + let tmp = TempDir::new().expect("tmp"); + let cwd = tmp.path().join("workspace").abs(); + std::fs::create_dir_all(&cwd).expect("create workspace"); + let sandbox_policy = FileSystemSandboxPolicy::workspace_write( + &[], + /*exclude_tmpdir_env_var*/ true, + /*exclude_slash_tmp*/ true, + ); + + let permissions = + write_permissions_for_paths(std::slice::from_ref(&cwd), &sandbox_policy, &cwd); + + assert_eq!(permissions, None); +} + +#[test] +fn write_permissions_for_paths_do_not_regrant_an_already_writable_parent() { + let tmp = TempDir::new().expect("tmp"); + let cwd = tmp.path().abs(); + let file_path = cwd.join("protected.txt"); + let sandbox_policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry::new(cwd.clone().into(), FileSystemAccessMode::Write), + FileSystemSandboxEntry::new(file_path.clone().into(), FileSystemAccessMode::Read), + ]); + + let permissions = write_permissions_for_paths(&[file_path], &sandbox_policy, &cwd); + + assert_eq!(permissions, None); +} diff --git a/codex-rs/core/tests/suite/apply_patch_cli.rs b/codex-rs/core/tests/suite/apply_patch_cli.rs index 3a88ab6d7a..013d1de2d4 100644 --- a/codex-rs/core/tests/suite/apply_patch_cli.rs +++ b/codex-rs/core/tests/suite/apply_patch_cli.rs @@ -986,6 +986,71 @@ async fn apply_patch_cli_does_not_write_through_symlink_escape_outside_workspace Ok(()) } +#[cfg(any(target_os = "linux", target_os = "macos"))] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn apply_patch_cli_does_not_widen_permissions_for_workspace_directory_target() -> 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"))?; + std::fs::create_dir_all(work_dir.as_path())?; + let outside_file = test_root.path().join("victim.txt"); + let original_contents = "original outside content\n"; + std::fs::write(&outside_file, original_contents)?; + + let harness_work_dir = work_dir.clone(); + let harness = apply_patch_harness_with(move |builder| { + builder.with_config(move |config| { + config.approvals_reviewer = codex_protocol::config_types::ApprovalsReviewer::User; + config.workspace_roots = vec![harness_work_dir.clone()]; + config + .permissions + .set_workspace_roots(config.workspace_roots.clone()); + config.cwd = harness_work_dir; + }) + }) + .await?; + let link_path = harness.path("link.txt"); + create_file_symlink(&outside_file, &link_path)?; + + // The second hunk names the already-writable workspace directory. It must + // not grant access to that directory's parent before the first hunk runs. + let patch = r#"*** Begin Patch +*** Update File: link.txt +@@ +-original outside content ++pwned +*** Add File: ../work ++decoy +*** End Patch"#; + let call_id = "apply-workspace-directory-decoy"; + mount_apply_patch(&harness, call_id, patch, "fail").await; + + harness + .submit_with_permission_profile( + "attempt to widen apply_patch permissions with a directory target", + restrictive_workspace_write_profile(), + ) + .await?; + + let out = harness.apply_patch_output(call_id).await; + assert_eq!( + std::fs::read_to_string(&outside_file)?, + original_contents, + "directory target must not make the outside victim writable; tool output: {out}", + ); + assert!( + std::fs::symlink_metadata(&link_path)? + .file_type() + .is_symlink() + ); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn apply_patch_cli_preserves_existing_hard_link_outside_workspace() -> Result<()> { skip_if_no_network!(Ok(()));