fix: reject symlinked config and output parents

Co-authored-by: Codex noreply@openai.com
This commit is contained in:
viyatb-oai
2026-06-01 17:05:39 -07:00
co-authored by Codex noreply@openai.com
parent ed8ce593e2
commit 93b7196dd6
19 changed files with 451 additions and 30 deletions
+3
View File
@@ -2742,6 +2742,7 @@ dependencies = [
"codex-utils-cargo-bin",
"codex-utils-cli",
"codex-utils-oss",
"codex-utils-path",
"codex-utils-sandbox-summary",
"core_test_support",
"libc",
@@ -2782,6 +2783,7 @@ dependencies = [
"codex-sandboxing",
"codex-test-binary-support",
"codex-utils-absolute-path",
"codex-utils-path",
"codex-utils-pty",
"codex-utils-rustls-provider",
"ctor 0.6.3",
@@ -4062,6 +4064,7 @@ version = "0.0.0"
dependencies = [
"codex-utils-absolute-path",
"dunce",
"libc",
"pretty_assertions",
"tempfile",
]
+28 -7
View File
@@ -1160,12 +1160,17 @@ async fn load_project_layers(
let mut startup_warnings = Vec::new();
for dir in dirs {
let dot_codex_abs = dir.join(".codex");
if !fs
.get_metadata(&dot_codex_abs, /*sandbox*/ None)
.await
.map(|metadata| metadata.is_directory)
.unwrap_or(false)
{
let Ok(dot_codex_metadata) = fs.get_metadata(&dot_codex_abs, /*sandbox*/ None).await else {
continue;
};
if dot_codex_metadata.is_symlink {
let dot_codex_display = dot_codex_abs.as_path().display();
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
format!("Project config directory {dot_codex_display} must not be a symlink"),
));
}
if !dot_codex_metadata.is_directory {
continue;
}
@@ -1178,6 +1183,18 @@ async fn load_project_layers(
continue;
}
let config_file = dot_codex_abs.join(CONFIG_TOML_FILE);
let config_file_for_read = fs
.canonicalize(&dir, /*sandbox*/ None)
.await
.map_err(|err| {
let dir_display = dir.as_path().display();
io::Error::new(
err.kind(),
format!("Failed to resolve project directory {dir_display}: {err}"),
)
})?
.join(".codex")
.join(CONFIG_TOML_FILE);
match fs.get_metadata(&config_file, /*sandbox*/ None).await {
Ok(metadata) => {
if metadata.is_symlink {
@@ -1194,6 +1211,7 @@ async fn load_project_layers(
&dot_codex_abs,
TomlValue::Table(toml::map::Map::new()),
disabled_reason,
hooks_config_folder_override,
));
continue;
}
@@ -1204,7 +1222,10 @@ async fn load_project_layers(
));
}
}
match fs.read_file_text(&config_file, /*sandbox*/ None).await {
match fs
.read_file_text_no_follow(&config_file_for_read, /*sandbox*/ None)
.await
{
Ok(contents) => {
let config: TomlValue = match toml::from_str(&contents) {
Ok(config) => config,
+8
View File
@@ -43,6 +43,14 @@ impl ExecutorFileSystem for TestFileSystem {
tokio::fs::read(path.as_path()).await
}
async fn read_file_no_follow(
&self,
path: &AbsolutePathBuf,
_sandbox: Option<&FileSystemSandboxContext>,
) -> FileSystemResult<Vec<u8>> {
tokio::fs::read(path.as_path()).await
}
async fn write_file(
&self,
_path: &AbsolutePathBuf,
@@ -2993,6 +2993,47 @@ async fn symlinked_project_config_is_rejected() -> std::io::Result<()> {
Ok(())
}
#[cfg(unix)]
#[tokio::test]
async fn symlinked_project_config_directory_is_rejected() -> std::io::Result<()> {
let tmp = tempdir()?;
let project_root = tmp.path().join("project");
let codex_home = tmp.path().join("home");
let payload_dir = tmp.path().join("payload");
tokio::fs::create_dir_all(&project_root).await?;
tokio::fs::create_dir_all(&codex_home).await?;
tokio::fs::create_dir_all(&payload_dir).await?;
tokio::fs::write(project_root.join(".git"), "gitdir: here").await?;
tokio::fs::write(
payload_dir.join(CONFIG_TOML_FILE),
"sandbox_mode = \"danger-full-access\"\n",
)
.await?;
symlink(&payload_dir, project_root.join(".codex")).expect("create config directory symlink");
make_config_for_test(
&codex_home,
&project_root,
TrustLevel::Trusted,
/*project_root_markers*/ None,
)
.await?;
let err = ConfigBuilder::default()
.codex_home(codex_home)
.fallback_cwd(Some(project_root))
.build()
.await
.expect_err("symlinked project config directory should fail");
assert_eq!(err.kind(), std::io::ErrorKind::InvalidInput);
assert!(
err.to_string().contains("must not be a symlink"),
"unexpected error: {err}"
);
Ok(())
}
#[tokio::test]
async fn project_layer_without_config_toml_is_disabled_when_untrusted_or_unknown()
-> std::io::Result<()> {
+1
View File
@@ -23,6 +23,7 @@ codex-file-system = { workspace = true }
codex-protocol = { workspace = true }
codex-sandboxing = { workspace = true }
codex-utils-absolute-path = { workspace = true }
codex-utils-path = { workspace = true }
codex-utils-pty = { workspace = true }
codex-utils-rustls-provider = { workspace = true }
futures = { workspace = true }
@@ -236,6 +236,14 @@ mod tests {
Ok(b"skill contents".to_vec())
}
async fn read_file_no_follow(
&self,
path: &AbsolutePathBuf,
sandbox: Option<&FileSystemSandboxContext>,
) -> io::Result<Vec<u8>> {
self.read_file(path, sandbox).await
}
async fn write_file(
&self,
_path: &AbsolutePathBuf,
+8 -4
View File
@@ -189,10 +189,14 @@ pub(crate) async fn run_direct_request(
let file_system = DirectFileSystem;
match request {
FsHelperRequest::ReadFile(params) => {
let data = file_system
.read_file(&params.path, /*sandbox*/ None)
.await
.map_err(map_fs_error)?;
let data = if params.no_follow_symlinks {
file_system
.read_file_no_follow(&params.path, /*sandbox*/ None)
.await
} else {
file_system.read_file(&params.path, /*sandbox*/ None).await
}
.map_err(map_fs_error)?;
Ok(FsHelperPayload::ReadFile(FsReadFileResponse {
data_base64: STANDARD.encode(data),
}))
@@ -7,6 +7,8 @@ use std::sync::LazyLock;
use std::time::SystemTime;
use std::time::UNIX_EPOCH;
use tokio::io;
#[cfg(unix)]
use tokio::io::AsyncReadExt;
use crate::CopyOptions;
use crate::CreateDirectoryOptions;
@@ -109,6 +111,15 @@ impl ExecutorFileSystem for LocalFileSystem {
file_system.read_file(path, sandbox).await
}
async fn read_file_no_follow(
&self,
path: &AbsolutePathBuf,
sandbox: Option<&FileSystemSandboxContext>,
) -> FileSystemResult<Vec<u8>> {
let (file_system, sandbox) = self.file_system_for(sandbox)?;
file_system.read_file_no_follow(path, sandbox).await
}
async fn write_file(
&self,
path: &AbsolutePathBuf,
@@ -203,6 +214,17 @@ impl ExecutorFileSystem for UnsandboxedFileSystem {
self.file_system.read_file(path, /*sandbox*/ None).await
}
async fn read_file_no_follow(
&self,
path: &AbsolutePathBuf,
sandbox: Option<&FileSystemSandboxContext>,
) -> FileSystemResult<Vec<u8>> {
reject_platform_sandbox_context(sandbox)?;
self.file_system
.read_file_no_follow(path, /*sandbox*/ None)
.await
}
async fn write_file(
&self,
path: &AbsolutePathBuf,
@@ -317,6 +339,33 @@ impl ExecutorFileSystem for DirectFileSystem {
tokio::fs::read(path.as_path()).await
}
async fn read_file_no_follow(
&self,
path: &AbsolutePathBuf,
sandbox: Option<&FileSystemSandboxContext>,
) -> FileSystemResult<Vec<u8>> {
reject_sandbox_context(sandbox)?;
#[cfg(unix)]
{
let file = codex_utils_path::open_file_for_read_no_follow(path.as_path())?;
let metadata = file.metadata()?;
if metadata.len() > MAX_READ_FILE_BYTES {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
format!("file is too large to read: limit is {MAX_READ_FILE_BYTES} bytes"),
));
}
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(
&self,
path: &AbsolutePathBuf,
+2
View File
@@ -165,6 +165,8 @@ pub struct TerminateResponse {
pub struct FsReadFileParams {
pub path: AbsolutePathBuf,
pub sandbox: Option<FileSystemSandboxContext>,
#[serde(default, skip_serializing_if = "std::ops::Not::not")]
pub no_follow_symlinks: bool,
}
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
@@ -98,6 +98,30 @@ impl ExecutorFileSystem for RemoteFileSystem {
.fs_read_file(FsReadFileParams {
path: path.clone(),
sandbox: remote_sandbox_context(sandbox),
no_follow_symlinks: false,
})
.await
.map_err(map_remote_error)?;
STANDARD.decode(response.data_base64).map_err(|err| {
io::Error::new(
io::ErrorKind::InvalidData,
format!("remote fs/readFile returned invalid base64 dataBase64: {err}"),
)
})
}
async fn read_file_no_follow(
&self,
path: &AbsolutePathBuf,
sandbox: Option<&FileSystemSandboxContext>,
) -> FileSystemResult<Vec<u8>> {
trace!("remote fs read_file_no_follow");
let client = self.client.get().await.map_err(map_remote_error)?;
let response = client
.fs_read_file(FsReadFileParams {
path: path.clone(),
sandbox: remote_sandbox_context(sandbox),
no_follow_symlinks: true,
})
.await
.map_err(map_remote_error)?;
@@ -97,6 +97,33 @@ impl ExecutorFileSystem for SandboxedFileSystem {
FsHelperRequest::ReadFile(FsReadFileParams {
path: path.clone(),
sandbox: None,
no_follow_symlinks: false,
}),
)
.await?
.expect_read_file()
.map_err(map_sandbox_error)?;
STANDARD.decode(response.data_base64).map_err(|err| {
io::Error::new(
io::ErrorKind::InvalidData,
format!("fs/readFile returned invalid base64 dataBase64: {err}"),
)
})
}
async fn read_file_no_follow(
&self,
path: &AbsolutePathBuf,
sandbox: Option<&FileSystemSandboxContext>,
) -> FileSystemResult<Vec<u8>> {
let sandbox = require_platform_sandbox(sandbox)?;
let response = self
.run_sandboxed(
sandbox,
FsHelperRequest::ReadFile(FsReadFileParams {
path: path.clone(),
sandbox: None,
no_follow_symlinks: true,
}),
)
.await?
@@ -52,11 +52,16 @@ impl FileSystemHandler {
&self,
params: FsReadFileParams,
) -> Result<FsReadFileResponse, JSONRPCErrorError> {
let bytes = self
.file_system
.read_file(&params.path, params.sandbox.as_ref())
.await
.map_err(map_fs_error)?;
let bytes = if params.no_follow_symlinks {
self.file_system
.read_file_no_follow(&params.path, params.sandbox.as_ref())
.await
} else {
self.file_system
.read_file(&params.path, params.sandbox.as_ref())
.await
}
.map_err(map_fs_error)?;
Ok(FsReadFileResponse {
data_base64: STANDARD.encode(bytes),
})
@@ -269,6 +274,7 @@ mod tests {
sandbox_policy,
sandbox_cwd.clone(),
)),
no_follow_symlinks: false,
})
.await
.expect("read file");
+36
View File
@@ -735,6 +735,42 @@ async fn file_system_sandboxed_read_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_read_file_no_follow_rejects_symlinked_parent(use_remote: bool) -> Result<()> {
let context = create_file_system_context(use_remote).await?;
let file_system = context.file_system;
let tmp = TempDir::new()?;
let tmp = tmp.path().canonicalize()?;
let target_dir = tmp.join("target");
let symlinked_dir = tmp.join("link");
std::fs::create_dir(&target_dir)?;
let target_path = target_dir.join("payload");
std::fs::write(&target_path, "original")?;
symlink(&target_dir, &symlinked_dir)?;
let contents = file_system
.read_file_no_follow(&absolute_path(target_path), /*sandbox*/ None)
.await
.with_context(|| format!("mode={use_remote}"))?;
assert_eq!(contents, b"original");
if file_system
.read_file_no_follow(
&absolute_path(symlinked_dir.join("payload")),
/*sandbox*/ None,
)
.await
.is_ok()
{
anyhow::bail!("symlinked parent should fail");
}
Ok(())
}
#[test_case(false ; "local")]
#[test_case(true ; "remote")]
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+2 -1
View File
@@ -39,8 +39,9 @@ codex-protocol = { workspace = true }
codex-utils-absolute-path = { workspace = true }
codex-utils-cli = { workspace = true }
codex-utils-oss = { workspace = true }
libc = { workspace = true }
codex-utils-sandbox-summary = { workspace = true }
codex-utils-path = { workspace = true }
libc = { workspace = true }
owo-colors = { workspace = true }
serde = { workspace = true, features = ["derive"] }
serde_json = { workspace = true }
+30 -13
View File
@@ -1,9 +1,5 @@
#[cfg(unix)]
use std::fs::OpenOptions;
#[cfg(unix)]
use std::io::Write;
#[cfg(unix)]
use std::os::unix::fs::OpenOptionsExt;
use std::path::Path;
use codex_app_server_protocol::ServerNotification;
@@ -55,12 +51,7 @@ fn write_last_message_file(contents: &str, last_message_path: Option<&Path>) {
#[cfg(unix)]
fn write_last_message_to_path(path: &Path, contents: &str) -> std::io::Result<()> {
let mut file = OpenOptions::new()
.write(true)
.create(true)
.truncate(true)
.custom_flags(libc::O_NOFOLLOW)
.open(path)?;
let mut file = codex_utils_path::open_file_for_write_no_follow(path)?;
file.write_all(contents.as_bytes())
}
@@ -78,7 +69,11 @@ mod tests {
#[test]
fn writes_last_message_to_regular_file() {
let temp_dir = tempdir().expect("tempdir");
let output_path = temp_dir.path().join("output.md");
let output_path = temp_dir
.path()
.canonicalize()
.expect("canonical tempdir")
.join("output.md");
write_last_message_to_path(&output_path, "hello").expect("write output");
@@ -92,8 +87,9 @@ mod tests {
#[test]
fn rejects_symlinked_last_message_path() {
let temp_dir = tempdir().expect("tempdir");
let target_path = temp_dir.path().join("target.md");
let output_path = temp_dir.path().join("output.md");
let temp_dir = temp_dir.path().canonicalize().expect("canonical tempdir");
let target_path = temp_dir.join("target.md");
let output_path = temp_dir.join("output.md");
std::fs::write(&target_path, "original").expect("write target");
std::os::unix::fs::symlink(&target_path, &output_path).expect("create symlink");
@@ -106,4 +102,25 @@ mod tests {
"original"
);
}
#[cfg(unix)]
#[test]
fn rejects_symlinked_last_message_parent_directory() {
let temp_dir = tempdir().expect("tempdir");
let temp_dir = temp_dir.path().canonicalize().expect("canonical tempdir");
let target_dir = temp_dir.join("target");
let symlinked_dir = temp_dir.join("link");
std::fs::create_dir(&target_dir).expect("create target directory");
let target_path = target_dir.join("output.md");
std::fs::write(&target_path, "original").expect("write target");
std::os::unix::fs::symlink(&target_dir, &symlinked_dir).expect("create symlink");
write_last_message_to_path(&symlinked_dir.join("output.md"), "hello")
.expect_err("symlinked parent should fail");
assert_eq!(
std::fs::read_to_string(&target_path).expect("read target"),
"original"
);
}
}
+17
View File
@@ -156,6 +156,13 @@ pub trait ExecutorFileSystem: Send + Sync {
sandbox: Option<&FileSystemSandboxContext>,
) -> FileSystemResult<Vec<u8>>;
/// Reads a file while rejecting symlinks in any path component.
async fn read_file_no_follow(
&self,
path: &AbsolutePathBuf,
sandbox: Option<&FileSystemSandboxContext>,
) -> FileSystemResult<Vec<u8>>;
/// Reads a file and decodes it as UTF-8 text.
async fn read_file_text(
&self,
@@ -166,6 +173,16 @@ 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.
async fn read_file_text_no_follow(
&self,
path: &AbsolutePathBuf,
sandbox: Option<&FileSystemSandboxContext>,
) -> FileSystemResult<String> {
let bytes = self.read_file_no_follow(path, sandbox).await?;
String::from_utf8(bytes).map_err(|err| io::Error::new(io::ErrorKind::InvalidData, err))
}
async fn write_file(
&self,
path: &AbsolutePathBuf,
+3
View File
@@ -12,6 +12,9 @@ codex-utils-absolute-path = { workspace = true }
dunce = { workspace = true }
tempfile = { workspace = true }
[target.'cfg(unix)'.dependencies]
libc = { workspace = true }
[dev-dependencies]
pretty_assertions = { workspace = true }
tempfile = { workspace = true }
+96
View File
@@ -5,7 +5,19 @@ pub use env::is_wsl;
use codex_utils_absolute_path::AbsolutePathBuf;
use std::collections::HashSet;
#[cfg(unix)]
use std::ffi::CString;
#[cfg(unix)]
use std::fs::File;
use std::io;
#[cfg(unix)]
use std::os::fd::AsRawFd;
#[cfg(unix)]
use std::os::fd::FromRawFd;
#[cfg(unix)]
use std::os::unix::ffi::OsStrExt;
#[cfg(unix)]
use std::path::Component;
use std::path::Path;
use std::path::PathBuf;
use tempfile::NamedTempFile;
@@ -132,6 +144,90 @@ pub fn write_atomically(write_path: &Path, contents: &str) -> io::Result<()> {
Ok(())
}
/// Opens a file for reading without following symlinks in any path component.
#[cfg(unix)]
pub fn open_file_for_read_no_follow(path: &Path) -> io::Result<File> {
open_file_no_follow(path, libc::O_RDONLY, /*mode*/ 0)
}
/// Opens a file for writing without following symlinks in any path component.
#[cfg(unix)]
pub fn open_file_for_write_no_follow(path: &Path) -> io::Result<File> {
open_file_no_follow(
path,
libc::O_WRONLY | libc::O_CREAT | libc::O_TRUNC,
/*mode*/ 0o666,
)
}
#[cfg(unix)]
fn open_file_no_follow(path: &Path, flags: libc::c_int, mode: libc::mode_t) -> io::Result<File> {
let components = path
.components()
.filter_map(|component| match component {
Component::RootDir | Component::CurDir => None,
Component::ParentDir => Some(Ok(std::ffi::OsStr::new(".."))),
Component::Normal(component) => Some(Ok(component)),
Component::Prefix(_) => Some(Err(io::Error::new(
io::ErrorKind::InvalidInput,
format!("unsupported path prefix in {}", path.display()),
))),
})
.collect::<io::Result<Vec<_>>>()?;
let Some((file_name, parent_components)) = components.split_last() else {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
format!("path {} does not name a file", path.display()),
));
};
let mut current_dir = if path.is_absolute() {
File::open("/")?
} else {
File::open(".")?
};
for component in parent_components {
current_dir = openat(
&current_dir,
component,
libc::O_RDONLY | libc::O_CLOEXEC | libc::O_DIRECTORY | libc::O_NOFOLLOW,
/*mode*/ 0,
)?;
}
openat(
&current_dir,
file_name,
flags | libc::O_CLOEXEC | libc::O_NOFOLLOW,
mode,
)
}
#[cfg(unix)]
fn openat(
directory: &File,
path: &std::ffi::OsStr,
flags: libc::c_int,
mode: libc::mode_t,
) -> io::Result<File> {
let path = CString::new(path.as_bytes())
.map_err(|err| io::Error::new(io::ErrorKind::InvalidInput, err))?;
// SAFETY: `path` is NUL-terminated, `directory` owns a valid fd, and the
// returned fd is transferred into a `File` exactly once.
let fd = unsafe {
libc::openat(
directory.as_raw_fd(),
path.as_ptr(),
flags,
libc::c_uint::from(mode),
)
};
if fd == -1 {
return Err(io::Error::last_os_error());
}
// SAFETY: `openat` returned a new owned file descriptor.
Ok(unsafe { File::from_raw_fd(fd) })
}
fn normalize_for_wsl(path: PathBuf) -> PathBuf {
normalize_for_wsl_with_flag(path, env::is_wsl())
}
@@ -1,7 +1,11 @@
#[cfg(unix)]
mod symlinks {
use super::super::open_file_for_read_no_follow;
use super::super::open_file_for_write_no_follow;
use super::super::resolve_symlink_write_paths;
use pretty_assertions::assert_eq;
use std::io::Read;
use std::io::Write;
use std::os::unix::fs::symlink;
#[test]
@@ -19,6 +23,59 @@ mod symlinks {
assert_eq!(resolved.write_path, a);
Ok(())
}
#[test]
fn no_follow_read_rejects_symlinked_parent_directory() -> std::io::Result<()> {
let dir = tempfile::tempdir()?;
let dir = dir.path().canonicalize()?;
let target_dir = dir.join("target");
let symlinked_dir = dir.join("link");
std::fs::create_dir(&target_dir)?;
std::fs::write(target_dir.join("payload"), "secret")?;
symlink(&target_dir, &symlinked_dir)?;
let err = open_file_for_read_no_follow(&symlinked_dir.join("payload"))
.expect_err("symlinked parent should fail");
assert!(err.raw_os_error().is_some());
Ok(())
}
#[test]
fn no_follow_write_rejects_symlinked_parent_directory() -> std::io::Result<()> {
let dir = tempfile::tempdir()?;
let dir = dir.path().canonicalize()?;
let target_dir = dir.join("target");
let symlinked_dir = dir.join("link");
std::fs::create_dir(&target_dir)?;
std::fs::write(target_dir.join("payload"), "original")?;
symlink(&target_dir, &symlinked_dir)?;
let err = open_file_for_write_no_follow(&symlinked_dir.join("payload"))
.expect_err("symlinked parent should fail");
assert!(err.raw_os_error().is_some());
assert_eq!(
std::fs::read_to_string(target_dir.join("payload"))?,
"original"
);
Ok(())
}
#[test]
fn no_follow_helpers_read_and_write_regular_file() -> std::io::Result<()> {
let dir = tempfile::tempdir()?;
let path = dir.path().canonicalize()?.join("payload");
let mut writer = open_file_for_write_no_follow(&path)?;
writer.write_all(b"hello")?;
let mut reader = open_file_for_read_no_follow(&path)?;
let mut contents = String::new();
reader.read_to_string(&mut contents)?;
assert_eq!(contents, "hello");
Ok(())
}
}
#[cfg(target_os = "linux")]