mirror of
https://github.com/openai/codex.git
synced 2026-10-30 19:18:49 +00:00
fix: harden symlinked output and project config writes
This commit is contained in:
@@ -1000,6 +1000,32 @@ async fn load_project_layers(
|
||||
continue;
|
||||
}
|
||||
let config_file = dot_codex_abs.join(CONFIG_TOML_FILE);
|
||||
match fs.get_metadata(&config_file, /*sandbox*/ None).await {
|
||||
Ok(metadata) => {
|
||||
if metadata.is_symlink {
|
||||
let config_file_display = config_file.as_path().display();
|
||||
return Err(io::Error::new(
|
||||
io::ErrorKind::InvalidInput,
|
||||
format!("Project config file {config_file_display} must not be a symlink"),
|
||||
));
|
||||
}
|
||||
}
|
||||
Err(err) => {
|
||||
if err.kind() == io::ErrorKind::NotFound {
|
||||
layers.push(project_layer_entry(
|
||||
&dot_codex_abs,
|
||||
TomlValue::Table(toml::map::Map::new()),
|
||||
disabled_reason,
|
||||
));
|
||||
continue;
|
||||
}
|
||||
let config_file_display = config_file.as_path().display();
|
||||
return Err(io::Error::new(
|
||||
err.kind(),
|
||||
format!("Failed to inspect project config file {config_file_display}: {err}"),
|
||||
));
|
||||
}
|
||||
}
|
||||
match fs.read_file_text(&config_file, /*sandbox*/ None).await {
|
||||
Ok(contents) => {
|
||||
let config: TomlValue = match toml::from_str(&contents) {
|
||||
|
||||
@@ -34,6 +34,8 @@ use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::collections::BTreeMap;
|
||||
use std::collections::HashMap;
|
||||
#[cfg(unix)]
|
||||
use std::os::unix::fs::symlink;
|
||||
use std::path::Path;
|
||||
use tempfile::tempdir;
|
||||
use toml::Value as TomlValue;
|
||||
@@ -2068,6 +2070,37 @@ async fn invalid_project_config_ignored_when_untrusted_or_unknown() -> std::io::
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn symlinked_project_config_is_rejected() -> std::io::Result<()> {
|
||||
let tmp = tempdir()?;
|
||||
let project_root = tmp.path().join("project");
|
||||
let dot_codex = project_root.join(".codex");
|
||||
let codex_home = tmp.path().join("home");
|
||||
let payload = project_root.join("payload.toml");
|
||||
tokio::fs::create_dir_all(&dot_codex).await?;
|
||||
tokio::fs::create_dir_all(&codex_home).await?;
|
||||
tokio::fs::write(project_root.join(".git"), "gitdir: here").await?;
|
||||
tokio::fs::write(&payload, "sandbox_mode = \"danger-full-access\"\n").await?;
|
||||
symlink(&payload, dot_codex.join(CONFIG_TOML_FILE)).expect("create config symlink");
|
||||
make_config_for_test(&codex_home, &project_root, TrustLevel::Trusted, None).await?;
|
||||
|
||||
let err = ConfigBuilder::default()
|
||||
.codex_home(codex_home)
|
||||
.fallback_cwd(Some(project_root))
|
||||
.build()
|
||||
.await
|
||||
.expect_err("symlinked project config 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<()> {
|
||||
|
||||
@@ -38,6 +38,7 @@ codex-protocol = { workspace = true }
|
||||
codex-utils-absolute-path = { workspace = true }
|
||||
codex-utils-cli = { workspace = true }
|
||||
codex-utils-oss = { workspace = true }
|
||||
libc = { workspace = true }
|
||||
owo-colors = { workspace = true }
|
||||
serde = { workspace = true, features = ["derive"] }
|
||||
serde_json = { workspace = true }
|
||||
@@ -64,7 +65,6 @@ assert_cmd = { workspace = true }
|
||||
codex-apply-patch = { workspace = true }
|
||||
codex-utils-cargo-bin = { workspace = true }
|
||||
core_test_support = { workspace = true }
|
||||
libc = { workspace = true }
|
||||
opentelemetry = { workspace = true }
|
||||
opentelemetry_sdk = { workspace = true }
|
||||
predicates = { workspace = true }
|
||||
|
||||
@@ -1,3 +1,9 @@
|
||||
#[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;
|
||||
@@ -41,8 +47,63 @@ pub(crate) fn handle_last_message(last_agent_message: Option<&str>, output_file:
|
||||
|
||||
fn write_last_message_file(contents: &str, last_message_path: Option<&Path>) {
|
||||
if let Some(path) = last_message_path
|
||||
&& let Err(e) = std::fs::write(path, contents)
|
||||
&& let Err(e) = write_last_message_to_path(path, contents)
|
||||
{
|
||||
eprintln!("Failed to write last message file {path:?}: {e}");
|
||||
}
|
||||
}
|
||||
|
||||
#[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)?;
|
||||
file.write_all(contents.as_bytes())
|
||||
}
|
||||
|
||||
#[cfg(not(unix))]
|
||||
fn write_last_message_to_path(path: &Path, contents: &str) -> std::io::Result<()> {
|
||||
std::fs::write(path, contents)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::write_last_message_to_path;
|
||||
use pretty_assertions::assert_eq;
|
||||
use tempfile::tempdir;
|
||||
|
||||
#[test]
|
||||
fn writes_last_message_to_regular_file() {
|
||||
let temp_dir = tempdir().expect("tempdir");
|
||||
let output_path = temp_dir.path().join("output.md");
|
||||
|
||||
write_last_message_to_path(&output_path, "hello").expect("write output");
|
||||
|
||||
assert_eq!(
|
||||
std::fs::read_to_string(&output_path).expect("read output"),
|
||||
"hello"
|
||||
);
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[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");
|
||||
std::fs::write(&target_path, "original").expect("write target");
|
||||
std::os::unix::fs::symlink(&target_path, &output_path).expect("create symlink");
|
||||
|
||||
let err =
|
||||
write_last_message_to_path(&output_path, "hello").expect_err("symlink should fail");
|
||||
|
||||
assert_eq!(err.raw_os_error(), Some(libc::ELOOP));
|
||||
assert_eq!(
|
||||
std::fs::read_to_string(&target_path).expect("read target"),
|
||||
"original"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2224,6 +2224,39 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn split_policy_masks_symlinked_project_config_file() {
|
||||
let temp_dir = TempDir::new().expect("temp dir");
|
||||
let writable_root = temp_dir.path().join("workspace");
|
||||
let dot_codex = writable_root.join(".codex");
|
||||
let payload = writable_root.join("payload.toml");
|
||||
let config = dot_codex.join("config.toml");
|
||||
std::fs::create_dir_all(&dot_codex).expect("create .codex");
|
||||
std::fs::write(&payload, "sandbox_mode = \"danger-full-access\"").expect("write payload");
|
||||
std::os::unix::fs::symlink(&payload, &config).expect("create config symlink");
|
||||
let writable_root =
|
||||
AbsolutePathBuf::from_absolute_path(&writable_root).expect("absolute writable root");
|
||||
let config = AbsolutePathBuf::from_absolute_path(&config).expect("absolute config");
|
||||
let config_str = path_to_string(config.as_path());
|
||||
let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Path {
|
||||
path: writable_root,
|
||||
},
|
||||
access: FileSystemAccessMode::Write,
|
||||
}]);
|
||||
|
||||
let args = create_filesystem_args(&policy, temp_dir.path()).expect("filesystem args");
|
||||
|
||||
assert!(
|
||||
args.args
|
||||
.windows(3)
|
||||
.any(|window| window == ["--ro-bind", "/dev/null", config_str.as_str()]),
|
||||
"expected symlinked config.toml to be masked: {:#?}",
|
||||
args.args
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn split_policy_reenables_nested_writable_subpaths_after_read_only_parent() {
|
||||
let temp_dir = TempDir::new().expect("temp dir");
|
||||
|
||||
@@ -1419,7 +1419,7 @@ pub(crate) fn default_read_only_subpaths_for_writable_root(
|
||||
}
|
||||
|
||||
let top_level_agents = writable_root.join(PROTECTED_METADATA_AGENTS_PATH_NAME);
|
||||
if top_level_agents.as_path().is_dir() {
|
||||
if path_exists_or_is_symlink(top_level_agents.as_path()) {
|
||||
subpaths.push(top_level_agents);
|
||||
}
|
||||
|
||||
@@ -1428,9 +1428,13 @@ pub(crate) fn default_read_only_subpaths_for_writable_root(
|
||||
// directory exists so first-time creation still goes through the
|
||||
// protected-path approval flow.
|
||||
let top_level_codex = writable_root.join(PROTECTED_METADATA_CODEX_PATH_NAME);
|
||||
if protect_missing_dot_codex || top_level_codex.as_path().is_dir() {
|
||||
if protect_missing_dot_codex || path_exists_or_is_symlink(top_level_codex.as_path()) {
|
||||
subpaths.push(top_level_codex);
|
||||
}
|
||||
let top_level_codex_config = writable_root.join(".codex/config.toml");
|
||||
if path_exists_or_is_symlink(top_level_codex_config.as_path()) {
|
||||
subpaths.push(top_level_codex_config);
|
||||
}
|
||||
|
||||
dedup_absolute_paths(subpaths, /*normalize_effective_paths*/ false)
|
||||
}
|
||||
@@ -1655,6 +1659,13 @@ 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)
|
||||
}
|
||||
|
||||
fn is_git_pointer_file(path: &AbsolutePathBuf) -> bool {
|
||||
path.as_path().is_file()
|
||||
&& path.as_path().file_name() == Some(OsStr::new(PROTECTED_METADATA_GIT_PATH_NAME))
|
||||
@@ -2185,6 +2196,47 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn writable_roots_preserve_symlinked_project_config_file() {
|
||||
let cwd = TempDir::new().expect("tempdir");
|
||||
let root = cwd.path().join("root");
|
||||
let dot_codex = root.join(".codex");
|
||||
let payload = root.join("payload.toml");
|
||||
let config = dot_codex.join("config.toml");
|
||||
fs::create_dir_all(&dot_codex).expect("create .codex");
|
||||
fs::write(&payload, "sandbox_mode = \"danger-full-access\"").expect("write payload");
|
||||
std::os::unix::fs::symlink(&payload, &config).expect("create config symlink");
|
||||
|
||||
let root = AbsolutePathBuf::from_absolute_path(
|
||||
root.canonicalize().expect("canonicalize root"),
|
||||
)
|
||||
.expect("absolute root");
|
||||
let expected_dot_codex = root.join(".codex").expect("absolute .codex");
|
||||
let expected_config = root
|
||||
.join(".codex/config.toml")
|
||||
.expect("absolute .codex/config.toml");
|
||||
let unexpected_payload =
|
||||
AbsolutePathBuf::from_absolute_path(payload).expect("absolute payload");
|
||||
|
||||
let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Path { path: root },
|
||||
access: FileSystemAccessMode::Write,
|
||||
}]);
|
||||
|
||||
let writable_roots = policy.get_writable_roots_with_cwd(cwd.path());
|
||||
assert_eq!(writable_roots.len(), 1);
|
||||
assert_eq!(
|
||||
writable_roots[0].read_only_subpaths,
|
||||
vec![expected_dot_codex, expected_config]
|
||||
);
|
||||
assert!(
|
||||
!writable_roots[0]
|
||||
.read_only_subpaths
|
||||
.contains(&unexpected_payload)
|
||||
);
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn writable_roots_preserve_explicit_symlinked_carveouts_under_symlinked_roots() {
|
||||
|
||||
@@ -4402,6 +4402,7 @@ mod tests {
|
||||
let cwd = TempDir::new().expect("tempdir");
|
||||
std::fs::create_dir_all(cwd.path().join(".agents")).expect("create .agents");
|
||||
std::fs::create_dir_all(cwd.path().join(".codex")).expect("create .codex");
|
||||
std::fs::write(cwd.path().join(".codex/config.toml"), "").expect("create config.toml");
|
||||
let canonical_cwd = codex_utils_absolute_path::canonicalize_preserving_symlinks(cwd.path())
|
||||
.expect("canonicalize cwd");
|
||||
let cwd_absolute =
|
||||
@@ -4413,6 +4414,9 @@ mod tests {
|
||||
.expect("canonical .agents");
|
||||
let expected_codex = AbsolutePathBuf::from_absolute_path(canonical_cwd.join(".codex"))
|
||||
.expect("canonical .codex");
|
||||
let expected_codex_config =
|
||||
AbsolutePathBuf::from_absolute_path(canonical_cwd.join(".codex/config.toml"))
|
||||
.expect("canonical .codex/config.toml");
|
||||
let policy = FileSystemSandboxPolicy::restricted(vec![
|
||||
FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Special {
|
||||
@@ -4465,6 +4469,12 @@ mod tests {
|
||||
.iter()
|
||||
.any(|path| path.as_path() == expected_codex.as_path())
|
||||
);
|
||||
assert!(
|
||||
writable_roots[0]
|
||||
.read_only_subpaths
|
||||
.iter()
|
||||
.any(|path| path.as_path() == expected_codex_config.as_path())
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
Reference in New Issue
Block a user