mirror of
https://github.com/openai/codex.git
synced 2026-05-17 09:43:19 +00:00
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
This commit is contained in:
@@ -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(
|
||||
|
||||
@@ -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)]
|
||||
|
||||
@@ -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)]
|
||||
|
||||
Reference in New Issue
Block a user