From 9334ac5c94cf68dc369653b75dd49edeadb73529 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Thu, 4 Jun 2026 00:07:44 -0700 Subject: [PATCH] test(sandbox): tighten symlink hardening coverage Co-authored-by: Codex noreply@openai.com --- codex-rs/config/src/loader/tests.rs | 15 ++++++++++++++- codex-rs/exec-server/src/protocol.rs | 1 + codex-rs/exec-server/tests/file_system.rs | 5 ++--- codex-rs/file-system/src/lib.rs | 6 ++++-- codex-rs/protocol/src/permissions.rs | 5 +---- codex-rs/utils/path-utils/src/lib.rs | 2 ++ .../utils/path-utils/src/path_utils_tests.rs | 16 ++++++++++++++++ 7 files changed, 40 insertions(+), 10 deletions(-) diff --git a/codex-rs/config/src/loader/tests.rs b/codex-rs/config/src/loader/tests.rs index e8971c4960..b8e4b443e0 100644 --- a/codex-rs/config/src/loader/tests.rs +++ b/codex-rs/config/src/loader/tests.rs @@ -10,6 +10,8 @@ use codex_file_system::RemoveOptions; use pretty_assertions::assert_eq; use std::path::Path; use tempfile::tempdir; +#[cfg(unix)] +use tokio::io::AsyncReadExt; struct TestFileSystem; @@ -48,7 +50,18 @@ impl ExecutorFileSystem for TestFileSystem { path: &AbsolutePathBuf, _sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult> { - tokio::fs::read(path.as_path()).await + #[cfg(unix)] + { + let file = codex_utils_path::open_file_for_read_no_follow(path.as_path())?; + let mut file = tokio::fs::File::from_std(file); + let mut contents = Vec::new(); + file.read_to_end(&mut contents).await?; + Ok(contents) + } + #[cfg(not(unix))] + { + self.read_file(path, /*sandbox*/ None).await + } } async fn write_file( diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index 3e76f8858b..f7ceaa1c0f 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/codex-rs/exec-server/src/protocol.rs @@ -165,6 +165,7 @@ pub struct TerminateResponse { pub struct FsReadFileParams { pub path: AbsolutePathBuf, pub sandbox: Option, + /// Reject symlinks in every path component on Unix executors. #[serde(default, skip_serializing_if = "std::ops::Not::not")] pub no_follow_symlinks: bool, } diff --git a/codex-rs/exec-server/tests/file_system.rs b/codex-rs/exec-server/tests/file_system.rs index 27067fd643..9f8e095ccc 100644 --- a/codex-rs/exec-server/tests/file_system.rs +++ b/codex-rs/exec-server/tests/file_system.rs @@ -757,15 +757,14 @@ async fn file_system_read_file_no_follow_rejects_symlinked_parent(use_remote: bo .with_context(|| format!("mode={use_remote}"))?; assert_eq!(contents, b"original"); - if file_system + if let Ok(contents) = file_system .read_file_no_follow( &absolute_path(symlinked_dir.join("payload")), /*sandbox*/ None, ) .await - .is_ok() { - anyhow::bail!("symlinked parent should fail"); + anyhow::bail!("symlinked parent should fail, but read {contents:?}"); } Ok(()) diff --git a/codex-rs/file-system/src/lib.rs b/codex-rs/file-system/src/lib.rs index a76975bb6c..ecf98aa31d 100644 --- a/codex-rs/file-system/src/lib.rs +++ b/codex-rs/file-system/src/lib.rs @@ -156,7 +156,9 @@ pub trait ExecutorFileSystem: Send + Sync { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult>; - /// Reads a file while rejecting symlinks in any path component. + /// Reads a file while rejecting symlinks in any path component on Unix. + /// + /// Non-Unix executors currently fall back to [`Self::read_file`]. async fn read_file_no_follow( &self, path: &AbsolutePathBuf, @@ -173,7 +175,7 @@ pub trait ExecutorFileSystem: Send + Sync { String::from_utf8(bytes).map_err(|err| io::Error::new(io::ErrorKind::InvalidData, err)) } - /// Reads a file without following symlinks and decodes it as UTF-8 text. + /// Reads a file with [`Self::read_file_no_follow`] and decodes it as UTF-8 text. async fn read_file_text_no_follow( &self, path: &AbsolutePathBuf, diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index 6c4cbaac2f..4fcef4af0b 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -1809,10 +1809,7 @@ fn has_explicit_write_entry_for_metadata_path( } fn path_exists_or_is_symlink(path: &Path) -> bool { - path.exists() - || std::fs::symlink_metadata(path) - .map(|metadata| metadata.file_type().is_symlink()) - .unwrap_or(false) + std::fs::symlink_metadata(path).is_ok() } fn is_git_pointer_file(path: &AbsolutePathBuf) -> bool { diff --git a/codex-rs/utils/path-utils/src/lib.rs b/codex-rs/utils/path-utils/src/lib.rs index 1c56293302..35fee11c23 100644 --- a/codex-rs/utils/path-utils/src/lib.rs +++ b/codex-rs/utils/path-utils/src/lib.rs @@ -145,6 +145,8 @@ pub fn write_atomically(write_path: &Path, contents: &str) -> io::Result<()> { } /// Opens a file for reading without following symlinks in any path component. +/// +/// Relative paths are resolved from the process's current directory. #[cfg(unix)] pub fn open_file_for_read_no_follow(path: &Path) -> io::Result { open_file_no_follow(path, libc::O_RDONLY, /*mode*/ 0) diff --git a/codex-rs/utils/path-utils/src/path_utils_tests.rs b/codex-rs/utils/path-utils/src/path_utils_tests.rs index 360d898cf3..78dd7cf575 100644 --- a/codex-rs/utils/path-utils/src/path_utils_tests.rs +++ b/codex-rs/utils/path-utils/src/path_utils_tests.rs @@ -39,6 +39,22 @@ mod symlinks { Ok(()) } + #[test] + fn no_follow_read_rejects_symlinked_file() -> std::io::Result<()> { + let dir = tempfile::tempdir()?; + let dir = dir.path().canonicalize()?; + let target = dir.join("target"); + let symlink_path = dir.join("link"); + std::fs::write(&target, "secret")?; + symlink(&target, &symlink_path)?; + + let err = + open_file_for_read_no_follow(&symlink_path).expect_err("symlinked file should fail"); + + assert_eq!(err.raw_os_error(), Some(libc::ELOOP)); + Ok(()) + } + #[test] fn no_follow_read_reads_regular_file() -> std::io::Result<()> { let dir = tempfile::tempdir()?;