diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index ab8e87c63c..4d173d2030 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2931,6 +2931,7 @@ name = "codex-utils-absolute-path" version = "0.0.0" dependencies = [ "dirs", + "dunce", "pretty_assertions", "schemars 0.8.22", "serde", diff --git a/codex-rs/core/src/exec_tests.rs b/codex-rs/core/src/exec_tests.rs index 334757e53f..937a7d6f80 100644 --- a/codex-rs/core/src/exec_tests.rs +++ b/codex-rs/core/src/exec_tests.rs @@ -650,14 +650,8 @@ fn windows_restricted_token_supports_full_read_split_write_read_carveouts() { }, ]); - #[cfg(windows)] - let expected_deny_write_paths = vec![ - codex_utils_absolute_path::AbsolutePathBuf::from_absolute_path(cwd.join(".codex")) - .expect("absolute .codex"), - codex_utils_absolute_path::AbsolutePathBuf::from_absolute_path(&docs) - .expect("absolute docs"), - ]; - #[cfg(not(windows))] + // The legacy workspace-write root already protects top-level `.codex`, so + // the restricted-token overlay only needs the extra read-only docs carveout. let expected_deny_write_paths = vec![ codex_utils_absolute_path::AbsolutePathBuf::from_absolute_path(&docs) .expect("absolute docs"), diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index f3b281a822..81a0961fa4 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -84,6 +84,7 @@ use codex_protocol::protocol::SessionConfiguredEvent; use codex_protocol::protocol::SessionSource; use codex_protocol::user_input::UserInput; use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_absolute_path::canonicalize_existing_preserving_symlinks; use codex_utils_oss::ensure_oss_provider_ready; use codex_utils_oss::get_default_model_for_oss_provider; use event_processor_with_human_output::EventProcessorWithHumanOutput; @@ -278,7 +279,9 @@ pub async fn run_main(cli: Cli, arg0_paths: Arg0DispatchPaths) -> anyhow::Result let resolved_cwd = cwd.clone(); let config_cwd = match resolved_cwd.as_deref() { - Some(path) => AbsolutePathBuf::from_absolute_path(path.canonicalize()?)?, + Some(path) => { + AbsolutePathBuf::from_absolute_path(canonicalize_existing_preserving_symlinks(path)?)? + } None => AbsolutePathBuf::current_dir()?, }; diff --git a/codex-rs/linux-sandbox/src/bwrap.rs b/codex-rs/linux-sandbox/src/bwrap.rs index 0709324f64..91d6621029 100644 --- a/codex-rs/linux-sandbox/src/bwrap.rs +++ b/codex-rs/linux-sandbox/src/bwrap.rs @@ -11,6 +11,7 @@ //! - bubblewrap used to construct the filesystem view before exec. use std::collections::BTreeSet; use std::collections::HashSet; +use std::fs; use std::fs::File; use std::os::fd::AsRawFd; use std::path::Path; @@ -273,19 +274,35 @@ fn create_filesystem_args( if !root.exists() { continue; } + // Writable roots are rebound by real target below; mirror that + // for their restricted-read bootstrap mount. Plain read-only + // roots must stay logical because callers may execute those + // paths inside bwrap, such as Bazel runfiles helper binaries. + let mount_root = if writable_roots + .iter() + .any(|writable_root| root.starts_with(writable_root.root.as_path())) + { + canonical_target_if_symlinked_path(&root).unwrap_or(root) + } else { + root + }; args.push("--ro-bind".to_string()); - args.push(path_to_string(&root)); - args.push(path_to_string(&root)); + args.push(path_to_string(&mount_root)); + args.push(path_to_string(&mount_root)); } } args }; let mut preserved_files = Vec::new(); - let allowed_write_paths: Vec = writable_roots - .iter() - .map(|writable_root| writable_root.root.as_path().to_path_buf()) - .collect(); + let mut allowed_write_paths = Vec::with_capacity(writable_roots.len()); + for writable_root in &writable_roots { + let root = writable_root.root.as_path(); + allowed_write_paths.push(root.to_path_buf()); + if let Some(target) = canonical_target_if_symlinked_path(root) { + allowed_write_paths.push(target); + } + } let unreadable_paths: HashSet = unreadable_roots .iter() .map(|path| path.as_path().to_path_buf()) @@ -321,6 +338,7 @@ fn create_filesystem_args( for writable_root in &sorted_writable_roots { let root = writable_root.root.as_path(); + let symlink_target = canonical_target_if_symlinked_path(root); // If a denied ancestor was already masked, recreate any missing mount // target parents before binding the narrower writable descendant. if let Some(masking_root) = unreadable_roots @@ -332,9 +350,10 @@ fn create_filesystem_args( append_mount_target_parent_dir_args(&mut args, root, masking_root); } + let mount_root = symlink_target.as_deref().unwrap_or(root); args.push("--bind".to_string()); - args.push(path_to_string(root)); - args.push(path_to_string(root)); + args.push(path_to_string(mount_root)); + args.push(path_to_string(mount_root)); let mut read_only_subpaths: Vec = writable_root .read_only_subpaths @@ -342,6 +361,9 @@ fn create_filesystem_args( .map(|path| path.as_path().to_path_buf()) .filter(|path| !unreadable_paths.contains(path)) .collect(); + if let Some(target) = &symlink_target { + read_only_subpaths = remap_paths_for_symlink_target(read_only_subpaths, root, target); + } read_only_subpaths.sort_by_key(|path| path_depth(path)); for subpath in read_only_subpaths { append_read_only_subpath_args(&mut args, &subpath, &allowed_write_paths); @@ -351,6 +373,10 @@ fn create_filesystem_args( .filter(|path| path.as_path().starts_with(root)) .map(|path| path.as_path().to_path_buf()) .collect(); + if let Some(target) = &symlink_target { + nested_unreadable_roots = + remap_paths_for_symlink_target(nested_unreadable_roots, root, target); + } nested_unreadable_roots.sort_by_key(|path| path_depth(path)); for unreadable_root in nested_unreadable_roots { append_unreadable_root_args( @@ -396,6 +422,52 @@ fn path_depth(path: &Path) -> usize { path.components().count() } +fn canonical_target_if_symlinked_path(path: &Path) -> Option { + let mut current = PathBuf::new(); + for component in path.components() { + use std::path::Component; + match component { + Component::RootDir => { + current.push(Path::new("/")); + continue; + } + Component::CurDir => continue, + Component::ParentDir => { + current.pop(); + continue; + } + Component::Normal(part) => current.push(part), + Component::Prefix(_) => continue, + } + + let metadata = match fs::symlink_metadata(¤t) { + Ok(metadata) => metadata, + Err(_) => return None, + }; + if metadata.file_type().is_symlink() { + let target = fs::canonicalize(path).ok()?; + if target.as_path() == path { + return None; + } + return Some(target); + } + } + None +} + +fn remap_paths_for_symlink_target(paths: Vec, root: &Path, target: &Path) -> Vec { + paths + .into_iter() + .map(|path| { + if let Ok(relative) = path.strip_prefix(root) { + target.join(relative) + } else { + path + } + }) + .collect() +} + fn normalize_command_cwd_for_bwrap(command_cwd: &Path) -> PathBuf { command_cwd .canonicalize() @@ -427,10 +499,15 @@ fn append_read_only_subpath_args( subpath: &Path, allowed_write_paths: &[PathBuf], ) { - if let Some(symlink_path) = find_symlink_in_path(subpath, allowed_write_paths) { + if let Some(target) = canonical_target_for_symlink_in_path(subpath, allowed_write_paths) { + // bwrap takes `--ro-bind `. Use the resolved target + // for both operands so a protected symlinked directory is remounted + // read-only in place instead of binding onto the symlink path itself. + let mount_source = path_to_string(&target); + let mount_destination = path_to_string(&target); args.push("--ro-bind".to_string()); - args.push("/dev/null".to_string()); - args.push(path_to_string(&symlink_path)); + args.push(mount_source); + args.push(mount_destination); return; } @@ -458,11 +535,16 @@ fn append_unreadable_root_args( unreadable_root: &Path, allowed_write_paths: &[PathBuf], ) -> Result<()> { - if let Some(symlink_path) = find_symlink_in_path(unreadable_root, allowed_write_paths) { - args.push("--ro-bind".to_string()); - args.push("/dev/null".to_string()); - args.push(path_to_string(&symlink_path)); - return Ok(()); + if let Some(target) = canonical_target_for_symlink_in_path(unreadable_root, allowed_write_paths) + { + // Apply unreadable handling to the resolved symlink target, not the + // logical symlink path, to avoid file-vs-directory bind mismatches. + return append_existing_unreadable_path_args( + args, + preserved_files, + &target, + allowed_write_paths, + ); } if !unreadable_root.exists() { @@ -476,6 +558,20 @@ fn append_unreadable_root_args( return Ok(()); } + append_existing_unreadable_path_args( + args, + preserved_files, + unreadable_root, + allowed_write_paths, + ) +} + +fn append_existing_unreadable_path_args( + args: &mut Vec, + preserved_files: &mut Vec, + unreadable_root: &Path, + allowed_write_paths: &[PathBuf], +) -> Result<()> { if unreadable_root.is_dir() { let mut writable_descendants: Vec<&Path> = allowed_write_paths .iter() @@ -525,12 +621,10 @@ fn is_within_allowed_write_paths(path: &Path, allowed_write_paths: &[PathBuf]) - .any(|root| path.starts_with(root)) } -/// Find the first symlink along `target_path` that is also under a writable root. -/// -/// This blocks symlink replacement attacks where a protected path is a symlink -/// inside a writable root (e.g., `.codex -> ./decoy`). In that case we mount -/// `/dev/null` on the symlink itself to prevent rewiring it. -fn find_symlink_in_path(target_path: &Path, allowed_write_paths: &[PathBuf]) -> Option { +fn canonical_target_for_symlink_in_path( + target_path: &Path, + allowed_write_paths: &[PathBuf], +) -> Option { let mut current = PathBuf::new(); for component in target_path.components() { @@ -557,7 +651,7 @@ fn find_symlink_in_path(target_path: &Path, allowed_write_paths: &[PathBuf]) -> if metadata.file_type().is_symlink() && is_within_allowed_write_paths(¤t, allowed_write_paths) { - return Some(current); + return fs::canonicalize(¤t).ok(); } } @@ -701,7 +795,13 @@ mod tests { BwrapOptions::default(), ) .expect("create bwrap args"); + let canonical_sandbox_cwd = path_to_string( + &real_root + .canonicalize() + .expect("canonicalize sandbox policy cwd"), + ); let canonical_command_cwd = path_to_string(&canonical_command_cwd); + let link_sandbox_cwd = path_to_string(&link_root); let link_command_cwd = path_to_string(&command_cwd); assert!( @@ -709,12 +809,199 @@ mod tests { .windows(2) .any(|window| { window == ["--chdir", canonical_command_cwd.as_str()] }) ); + assert!(args.args.windows(3).any(|window| { + window + == [ + "--ro-bind", + canonical_sandbox_cwd.as_str(), + canonical_sandbox_cwd.as_str(), + ] + })); assert!( !args .args .windows(2) .any(|window| { window == ["--chdir", link_command_cwd.as_str()] }) ); + assert!(!args.args.windows(3).any(|window| { + window + == [ + "--ro-bind", + link_sandbox_cwd.as_str(), + link_sandbox_cwd.as_str(), + ] + })); + } + + #[cfg(unix)] + #[test] + fn symlinked_writable_roots_bind_real_target_and_remap_carveouts() { + let temp_dir = TempDir::new().expect("temp dir"); + let real_root = temp_dir.path().join("real"); + let link_root = temp_dir.path().join("link"); + let blocked = real_root.join("blocked"); + std::fs::create_dir_all(&blocked).expect("create blocked dir"); + std::os::unix::fs::symlink(&real_root, &link_root).expect("create symlinked root"); + + let link_root = + AbsolutePathBuf::from_absolute_path(&link_root).expect("absolute symlinked root"); + let link_blocked = link_root.join("blocked"); + let real_root_str = path_to_string(&real_root); + let real_blocked_str = path_to_string(&blocked); + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: link_root }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: link_blocked }, + access: FileSystemAccessMode::None, + }, + ]); + + let args = create_filesystem_args(&policy, temp_dir.path()).expect("filesystem args"); + + assert!(args.args.windows(3).any(|window| { + window == ["--bind", real_root_str.as_str(), real_root_str.as_str()] + })); + assert!(args.args.windows(6).any(|window| { + window + == [ + "--perms", + "000", + "--tmpfs", + real_blocked_str.as_str(), + "--remount-ro", + real_blocked_str.as_str(), + ] + })); + } + + #[cfg(unix)] + #[test] + fn writable_roots_under_symlinked_ancestors_bind_real_target() { + let temp_dir = TempDir::new().expect("temp dir"); + let logical_home = temp_dir.path().join("home"); + let real_codex = temp_dir.path().join("real-codex"); + let logical_codex = logical_home.join(".codex"); + let real_memories = real_codex.join("memories"); + let logical_memories = logical_codex.join("memories"); + std::fs::create_dir_all(&logical_home).expect("create logical home"); + std::fs::create_dir_all(&real_memories).expect("create memories dir"); + std::os::unix::fs::symlink(&real_codex, &logical_codex) + .expect("create symlinked codex home"); + + let logical_memories_root = + AbsolutePathBuf::from_absolute_path(&logical_memories).expect("absolute memories"); + let real_memories_str = path_to_string(&real_memories); + let logical_memories_str = path_to_string(&logical_memories); + let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: logical_memories_root, + }, + access: FileSystemAccessMode::Write, + }]); + + let args = create_filesystem_args(&policy, temp_dir.path()).expect("filesystem args"); + + assert!(args.args.windows(3).any(|window| { + window + == [ + "--bind", + real_memories_str.as_str(), + real_memories_str.as_str(), + ] + })); + assert!(!args.args.windows(3).any(|window| { + window + == [ + "--bind", + logical_memories_str.as_str(), + logical_memories_str.as_str(), + ] + })); + } + + #[cfg(unix)] + #[test] + fn protected_symlinked_directory_subpaths_bind_target_read_only() { + let temp_dir = TempDir::new().expect("temp dir"); + let root = temp_dir.path().join("root"); + let agents_target = root.join("agents-target"); + let agents_link = root.join(".agents"); + std::fs::create_dir_all(&agents_target).expect("create agents target"); + std::os::unix::fs::symlink(&agents_target, &agents_link).expect("create symlinked .agents"); + + let root = AbsolutePathBuf::from_absolute_path(&root).expect("absolute root"); + let agents_link_str = path_to_string(&agents_link); + let agents_target_str = path_to_string(&agents_target); + let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { + path: FileSystemPath::Path { path: 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", + agents_target_str.as_str(), + agents_target_str.as_str(), + ] + })); + assert!(!args.args.iter().any(|arg| arg == agents_link_str.as_str())); + } + + #[cfg(unix)] + #[test] + fn symlinked_writable_roots_mask_nested_symlink_escape_paths_without_binding_targets() { + let temp_dir = TempDir::new().expect("temp dir"); + let real_root = temp_dir.path().join("real"); + let link_root = temp_dir.path().join("link"); + let outside = temp_dir.path().join("outside-private"); + let linked_private = real_root.join("linked-private"); + std::fs::create_dir_all(&real_root).expect("create real root"); + std::fs::create_dir_all(&outside).expect("create outside dir"); + std::os::unix::fs::symlink(&real_root, &link_root).expect("create symlinked root"); + std::os::unix::fs::symlink(&outside, &linked_private) + .expect("create nested escape symlink"); + + let link_root = + AbsolutePathBuf::from_absolute_path(&link_root).expect("absolute symlinked root"); + let link_private = link_root.join("linked-private"); + let real_linked_private_str = path_to_string(&linked_private); + let outside_str = path_to_string(&outside); + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: link_root }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: link_private }, + access: FileSystemAccessMode::None, + }, + ]); + + let args = create_filesystem_args(&policy, temp_dir.path()).expect("filesystem args"); + + assert!(args.args.windows(6).any(|window| { + window + == [ + "--perms", + "000", + "--tmpfs", + outside_str.as_str(), + "--remount-ro", + outside_str.as_str(), + ] + })); + assert!( + !args + .args + .iter() + .any(|arg| arg == real_linked_private_str.as_str()) + ); } #[test] diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index 416d291fd1..f101f8bef0 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -5,6 +5,7 @@ use std::path::Path; use std::path::PathBuf; use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_absolute_path::canonicalize_preserving_symlinks; use schemars::JsonSchema; use serde::Deserialize; use serde::Serialize; @@ -471,6 +472,9 @@ impl FileSystemSandboxPolicy { // so root-wide aliases do not create duplicate top-level masks. // Example: keep `/var/...` normalized under `/` instead of // materializing both `/var/...` and `/private/var/...`. + // Nested symlink paths under a writable root stay logical so + // downstream sandboxes can still bind the real target while + // masking the user-visible symlink inode when needed. let preserve_raw_carveout_paths = root.as_path().parent().is_some(); let raw_writable_roots: Vec<&AbsolutePathBuf> = writable_entries .iter() @@ -1081,14 +1085,17 @@ fn dedup_absolute_paths( fn normalize_effective_absolute_path(path: AbsolutePathBuf) -> AbsolutePathBuf { let raw_path = path.to_path_buf(); for ancestor in raw_path.ancestors() { - let Ok(canonical_ancestor) = ancestor.canonicalize() else { + if std::fs::symlink_metadata(ancestor).is_err() { + continue; + } + let Ok(normalized_ancestor) = canonicalize_preserving_symlinks(ancestor) else { continue; }; let Ok(suffix) = raw_path.strip_prefix(ancestor) else { continue; }; if let Ok(normalized_path) = - AbsolutePathBuf::from_absolute_path(canonical_ancestor.join(suffix)) + AbsolutePathBuf::from_absolute_path(normalized_ancestor.join(suffix)) { return normalized_path; } @@ -1422,7 +1429,7 @@ mod tests { #[cfg(unix)] #[test] - fn effective_runtime_roots_canonicalize_symlinked_paths() { + fn effective_runtime_roots_preserve_symlinked_paths() { let cwd = TempDir::new().expect("tempdir"); let real_root = cwd.path().join("real"); let link_root = cwd.path().join("link"); @@ -1436,18 +1443,9 @@ mod tests { let link_root = AbsolutePathBuf::from_absolute_path(&link_root).expect("absolute symlinked root"); let link_blocked = link_root.join("blocked"); - let expected_root = AbsolutePathBuf::from_absolute_path( - real_root.canonicalize().expect("canonicalize real root"), - ) - .expect("absolute canonical root"); - let expected_blocked = AbsolutePathBuf::from_absolute_path( - blocked.canonicalize().expect("canonicalize blocked"), - ) - .expect("absolute canonical blocked"); - let expected_codex = AbsolutePathBuf::from_absolute_path( - codex_dir.canonicalize().expect("canonicalize .codex"), - ) - .expect("absolute canonical .codex"); + let expected_root = link_root.clone(); + let expected_blocked = link_blocked.clone(); + let expected_codex = link_root.join(".codex"); let policy = FileSystemSandboxPolicy::restricted(vec![ FileSystemSandboxEntry { @@ -1482,7 +1480,7 @@ mod tests { #[cfg(unix)] #[test] - fn current_working_directory_special_path_canonicalizes_symlinked_cwd() { + fn current_working_directory_special_path_preserves_symlinked_cwd() { let cwd = TempDir::new().expect("tempdir"); let real_root = cwd.path().join("real"); let link_root = cwd.path().join("link"); @@ -1497,22 +1495,11 @@ mod tests { let link_blocked = AbsolutePathBuf::from_absolute_path(link_root.join("blocked")).expect("link blocked"); - let expected_root = AbsolutePathBuf::from_absolute_path( - real_root.canonicalize().expect("canonicalize real root"), - ) - .expect("absolute canonical root"); - let expected_blocked = AbsolutePathBuf::from_absolute_path( - blocked.canonicalize().expect("canonicalize blocked"), - ) - .expect("absolute canonical blocked"); - let expected_agents = AbsolutePathBuf::from_absolute_path( - agents_dir.canonicalize().expect("canonicalize .agents"), - ) - .expect("absolute canonical .agents"); - let expected_codex = AbsolutePathBuf::from_absolute_path( - codex_dir.canonicalize().expect("canonicalize .codex"), - ) - .expect("absolute canonical .codex"); + let expected_root = + AbsolutePathBuf::from_absolute_path(&link_root).expect("absolute symlinked root"); + let expected_blocked = link_blocked.clone(); + let expected_agents = expected_root.join(".agents"); + let expected_codex = expected_root.join(".codex"); let policy = FileSystemSandboxPolicy::restricted(vec![ FileSystemSandboxEntry { @@ -1617,11 +1604,8 @@ mod tests { let link_root = AbsolutePathBuf::from_absolute_path(&link_root).expect("absolute symlinked root"); let link_private = link_root.join("linked-private"); - let expected_root = AbsolutePathBuf::from_absolute_path( - real_root.canonicalize().expect("canonicalize real root"), - ) - .expect("absolute canonical root"); - let expected_linked_private = expected_root.join("linked-private"); + let expected_root = link_root.clone(); + let expected_linked_private = link_private.clone(); let unexpected_decoy = AbsolutePathBuf::from_absolute_path(decoy.canonicalize().expect("canonicalize decoy")) .expect("absolute canonical decoy"); @@ -1667,11 +1651,8 @@ mod tests { let link_root = AbsolutePathBuf::from_absolute_path(&link_root).expect("absolute symlinked root"); let link_private = link_root.join("linked-private"); - let expected_root = AbsolutePathBuf::from_absolute_path( - real_root.canonicalize().expect("canonicalize real root"), - ) - .expect("absolute canonical root"); - let expected_linked_private = expected_root.join("linked-private"); + let expected_root = link_root.clone(); + let expected_linked_private = link_private.clone(); let unexpected_decoy = AbsolutePathBuf::from_absolute_path(decoy.canonicalize().expect("canonicalize decoy")) .expect("absolute canonical decoy"); @@ -1737,12 +1718,12 @@ mod tests { #[cfg(unix)] #[test] - fn tmpdir_special_path_canonicalizes_symlinked_tmpdir() { + fn tmpdir_special_path_preserves_symlinked_tmpdir() { if std::env::var_os(SYMLINKED_TMPDIR_TEST_ENV).is_none() { let output = std::process::Command::new(std::env::current_exe().expect("test binary")) .env(SYMLINKED_TMPDIR_TEST_ENV, "1") .arg("--exact") - .arg("permissions::tests::tmpdir_special_path_canonicalizes_symlinked_tmpdir") + .arg("permissions::tests::tmpdir_special_path_preserves_symlinked_tmpdir") .output() .expect("run tmpdir subprocess test"); @@ -1767,20 +1748,10 @@ mod tests { let link_blocked = AbsolutePathBuf::from_absolute_path(link_tmpdir.join("blocked")).expect("link blocked"); - let expected_root = AbsolutePathBuf::from_absolute_path( - real_tmpdir - .canonicalize() - .expect("canonicalize real tmpdir"), - ) - .expect("absolute canonical tmpdir"); - let expected_blocked = AbsolutePathBuf::from_absolute_path( - blocked.canonicalize().expect("canonicalize blocked"), - ) - .expect("absolute canonical blocked"); - let expected_codex = AbsolutePathBuf::from_absolute_path( - codex_dir.canonicalize().expect("canonicalize .codex"), - ) - .expect("absolute canonical .codex"); + let expected_root = + AbsolutePathBuf::from_absolute_path(&link_tmpdir).expect("absolute symlinked tmpdir"); + let expected_blocked = link_blocked.clone(); + let expected_codex = expected_root.join(".codex"); unsafe { std::env::set_var("TMPDIR", &link_tmpdir); @@ -1932,8 +1903,7 @@ mod tests { let cwd = TempDir::new().expect("tempdir"); let docs = AbsolutePathBuf::resolve_path_against_base("docs", cwd.path()); let expected_docs = AbsolutePathBuf::from_absolute_path( - cwd.path() - .canonicalize() + canonicalize_preserving_symlinks(cwd.path()) .expect("canonicalize cwd") .join("docs"), ) diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 62d43893fe..2c2c9cabec 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -4150,7 +4150,8 @@ mod tests { #[test] fn restricted_file_system_policy_treats_root_with_carveouts_as_scoped_access() { let cwd = TempDir::new().expect("tempdir"); - let canonical_cwd = cwd.path().canonicalize().expect("canonicalize cwd"); + let canonical_cwd = codex_utils_absolute_path::canonicalize_preserving_symlinks(cwd.path()) + .expect("canonicalize cwd"); let root = AbsolutePathBuf::from_absolute_path(&canonical_cwd) .expect("absolute canonical tempdir") .as_path() @@ -4160,8 +4161,7 @@ mod tests { .expect("filesystem root"); let blocked = AbsolutePathBuf::resolve_path_against_base("blocked", cwd.path()); let expected_blocked = AbsolutePathBuf::from_absolute_path( - cwd.path() - .canonicalize() + codex_utils_absolute_path::canonicalize_preserving_symlinks(cwd.path()) .expect("canonicalize cwd") .join("blocked"), ) @@ -4206,7 +4206,8 @@ 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"); - let canonical_cwd = cwd.path().canonicalize().expect("canonicalize cwd"); + let canonical_cwd = codex_utils_absolute_path::canonicalize_preserving_symlinks(cwd.path()) + .expect("canonicalize cwd"); let cwd_absolute = AbsolutePathBuf::from_absolute_path(&canonical_cwd).expect("absolute tempdir"); let secret = AbsolutePathBuf::resolve_path_against_base("secret", cwd.path()); @@ -4273,7 +4274,8 @@ mod tests { #[test] fn restricted_file_system_policy_treats_read_entries_as_read_only_subpaths() { let cwd = TempDir::new().expect("tempdir"); - let canonical_cwd = cwd.path().canonicalize().expect("canonicalize cwd"); + let canonical_cwd = codex_utils_absolute_path::canonicalize_preserving_symlinks(cwd.path()) + .expect("canonicalize cwd"); let docs = AbsolutePathBuf::resolve_path_against_base("docs", cwd.path()); let docs_public = AbsolutePathBuf::resolve_path_against_base("docs/public", cwd.path()); let expected_docs = AbsolutePathBuf::from_absolute_path(canonical_cwd.join("docs")) @@ -4320,7 +4322,8 @@ mod tests { fn legacy_workspace_write_nested_readable_root_stays_writable() { let cwd = TempDir::new().expect("tempdir"); let docs = AbsolutePathBuf::resolve_path_against_base("docs", cwd.path()); - let canonical_cwd = cwd.path().canonicalize().expect("canonicalize cwd"); + let canonical_cwd = codex_utils_absolute_path::canonicalize_preserving_symlinks(cwd.path()) + .expect("canonicalize cwd"); let expected_dot_codex = AbsolutePathBuf::from_absolute_path(canonical_cwd.join(".codex")) .expect("canonical .codex"); let policy = SandboxPolicy::WorkspaceWrite { diff --git a/codex-rs/sandboxing/src/policy_transforms.rs b/codex-rs/sandboxing/src/policy_transforms.rs index 25c79fcc9e..73ecf81c9b 100644 --- a/codex-rs/sandboxing/src/policy_transforms.rs +++ b/codex-rs/sandboxing/src/policy_transforms.rs @@ -11,7 +11,7 @@ use codex_protocol::protocol::NetworkAccess; use codex_protocol::protocol::ReadOnlyAccess; use codex_protocol::protocol::SandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; -use dunce::canonicalize; +use codex_utils_absolute_path::canonicalize_preserving_symlinks; use std::collections::HashSet; #[derive(Debug, Clone, PartialEq, Eq)] @@ -171,7 +171,7 @@ fn normalize_permission_paths( let mut seen = HashSet::new(); for path in paths { - let canonicalized = canonicalize(path.as_path()) + let canonicalized = canonicalize_preserving_symlinks(path.as_path()) .ok() .and_then(|path| AbsolutePathBuf::from_absolute_path(path).ok()) .unwrap_or(path); diff --git a/codex-rs/sandboxing/src/policy_transforms_tests.rs b/codex-rs/sandboxing/src/policy_transforms_tests.rs index 10f49d95f3..f33d312f15 100644 --- a/codex-rs/sandboxing/src/policy_transforms_tests.rs +++ b/codex-rs/sandboxing/src/policy_transforms_tests.rs @@ -130,7 +130,7 @@ fn normalize_additional_permissions_preserves_network() { #[cfg(unix)] #[test] -fn normalize_additional_permissions_canonicalizes_symlinked_write_paths() { +fn normalize_additional_permissions_preserves_symlinked_write_paths() { let temp_dir = TempDir::new().expect("create temp dir"); let real_root = temp_dir.path().join("real"); let link_root = temp_dir.path().join("link"); @@ -140,11 +140,6 @@ fn normalize_additional_permissions_canonicalizes_symlinked_write_paths() { let link_write_dir = AbsolutePathBuf::from_absolute_path(link_root.join("write")).expect("link write dir"); - let expected_write_dir = AbsolutePathBuf::from_absolute_path( - write_dir.canonicalize().expect("canonicalize write dir"), - ) - .expect("absolute canonical write dir"); - let permissions = normalize_additional_permissions(PermissionProfile { file_system: Some(FileSystemPermissions { read: Some(vec![]), @@ -158,7 +153,10 @@ fn normalize_additional_permissions_canonicalizes_symlinked_write_paths() { permissions.file_system, Some(FileSystemPermissions { read: Some(vec![]), - write: Some(vec![expected_write_dir]), + write: Some(vec![ + AbsolutePathBuf::from_absolute_path(link_root.join("write")) + .expect("link write dir") + ]), }) ); } diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 700d685b0f..19c233c62f 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -54,6 +54,7 @@ use codex_rollout::state_db::get_state_db; use codex_state::log_db; use codex_terminal_detection::terminal_info; use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_absolute_path::canonicalize_existing_preserving_symlinks; use codex_utils_oss::ensure_oss_provider_ready; use codex_utils_oss::get_default_model_for_oss_provider; use color_eyre::eyre::WrapErr; @@ -623,7 +624,9 @@ fn config_cwd_for_app_server_target( } let cwd = match cwd { - Some(path) => AbsolutePathBuf::from_absolute_path(path.canonicalize()?), + Some(path) => { + AbsolutePathBuf::from_absolute_path(canonicalize_existing_preserving_symlinks(path)?) + } None => AbsolutePathBuf::current_dir(), }?; Ok(Some(cwd)) @@ -1944,13 +1947,28 @@ mod tests { assert_eq!( config_cwd, - Some(AbsolutePathBuf::from_absolute_path( - temp_dir.path().canonicalize()? - )?) + Some(AbsolutePathBuf::from_absolute_path(dunce::canonicalize( + temp_dir.path() + )?)?) ); Ok(()) } + #[test] + fn config_cwd_for_app_server_target_errors_for_missing_embedded_cli_cwd() -> std::io::Result<()> + { + let temp_dir = TempDir::new()?; + let missing = temp_dir.path().join("missing"); + let target = AppServerTarget::Embedded; + let environment_manager = EnvironmentManager::new(/*exec_server_url*/ None); + + let err = config_cwd_for_app_server_target(Some(&missing), &target, &environment_manager) + .expect_err("missing embedded cwd should fail"); + + assert_eq!(err.kind(), std::io::ErrorKind::NotFound); + Ok(()) + } + #[test] fn config_cwd_for_app_server_target_omits_cwd_for_remote_exec_server() -> std::io::Result<()> { let remote_only_cwd = if cfg!(windows) { diff --git a/codex-rs/utils/absolute-path/Cargo.toml b/codex-rs/utils/absolute-path/Cargo.toml index 801bc9c1f0..1d35198ed4 100644 --- a/codex-rs/utils/absolute-path/Cargo.toml +++ b/codex-rs/utils/absolute-path/Cargo.toml @@ -10,6 +10,7 @@ workspace = true [dependencies] dirs = { workspace = true } +dunce = { workspace = true } schemars = { workspace = true } serde = { workspace = true, features = ["derive"] } ts-rs = { workspace = true, features = [ diff --git a/codex-rs/utils/absolute-path/src/lib.rs b/codex-rs/utils/absolute-path/src/lib.rs index a09ce8f0cb..55ba273bdf 100644 --- a/codex-rs/utils/absolute-path/src/lib.rs +++ b/codex-rs/utils/absolute-path/src/lib.rs @@ -106,6 +106,48 @@ impl AbsolutePathBuf { } } +/// Canonicalize a path when possible, but preserve the logical absolute path +/// whenever canonicalization would rewrite it through a nested symlink. +/// +/// Top-level system aliases such as macOS `/var -> /private/var` still remain +/// canonicalized so existing runtime expectations around those paths stay +/// stable. If the full path cannot be canonicalized, this returns the logical +/// absolute path; use [`canonicalize_existing_preserving_symlinks`] for paths +/// that must exist. +pub fn canonicalize_preserving_symlinks(path: &Path) -> std::io::Result { + let logical = AbsolutePathBuf::from_absolute_path(path)?.into_path_buf(); + let preserve_logical_path = should_preserve_logical_path(&logical); + match dunce::canonicalize(path) { + Ok(canonical) if preserve_logical_path && canonical != logical => Ok(logical), + Ok(canonical) => Ok(canonical), + Err(_) => Ok(logical), + } +} + +/// Canonicalize an existing path while preserving the logical absolute path +/// whenever canonicalization would rewrite it through a nested symlink. +/// +/// Unlike [`canonicalize_preserving_symlinks`], canonicalization failures are +/// propagated so callers can reject invalid working directories early. +pub fn canonicalize_existing_preserving_symlinks(path: &Path) -> std::io::Result { + let logical = AbsolutePathBuf::from_absolute_path(path)?.into_path_buf(); + let canonical = dunce::canonicalize(path)?; + if should_preserve_logical_path(&logical) && canonical != logical { + Ok(logical) + } else { + Ok(canonical) + } +} + +fn should_preserve_logical_path(logical: &Path) -> bool { + logical.ancestors().any(|ancestor| { + let Ok(metadata) = std::fs::symlink_metadata(ancestor) else { + return false; + }; + metadata.file_type().is_symlink() && ancestor.parent().and_then(Path::parent).is_some() + }) +} + impl AsRef for AbsolutePathBuf { fn as_ref(&self) -> &Path { &self.0 @@ -335,6 +377,63 @@ mod tests { assert_eq!(abs_path_buf.as_path(), home.join("code").as_path()); } + #[cfg(unix)] + #[test] + fn canonicalize_preserving_symlinks_keeps_logical_symlink_path() { + let temp_dir = tempdir().expect("temp dir"); + let real = temp_dir.path().join("real"); + let link = temp_dir.path().join("link"); + std::fs::create_dir_all(&real).expect("create real dir"); + std::os::unix::fs::symlink(&real, &link).expect("create symlink"); + + let canonicalized = + canonicalize_preserving_symlinks(&link).expect("canonicalize preserving symlinks"); + + assert_eq!(canonicalized, link); + } + + #[cfg(unix)] + #[test] + fn canonicalize_preserving_symlinks_keeps_logical_missing_child_under_symlink() { + let temp_dir = tempdir().expect("temp dir"); + let real = temp_dir.path().join("real"); + let link = temp_dir.path().join("link"); + std::fs::create_dir_all(&real).expect("create real dir"); + std::os::unix::fs::symlink(&real, &link).expect("create symlink"); + let missing = link.join("missing.txt"); + + let canonicalized = + canonicalize_preserving_symlinks(&missing).expect("canonicalize preserving symlinks"); + + assert_eq!(canonicalized, missing); + } + + #[test] + fn canonicalize_existing_preserving_symlinks_errors_for_missing_path() { + let temp_dir = tempdir().expect("temp dir"); + let missing = temp_dir.path().join("missing"); + + let err = canonicalize_existing_preserving_symlinks(&missing) + .expect_err("missing path should fail canonicalization"); + + assert_eq!(err.kind(), std::io::ErrorKind::NotFound); + } + + #[cfg(unix)] + #[test] + fn canonicalize_existing_preserving_symlinks_keeps_logical_symlink_path() { + let temp_dir = tempdir().expect("temp dir"); + let real = temp_dir.path().join("real"); + let link = temp_dir.path().join("link"); + std::fs::create_dir_all(&real).expect("create real dir"); + std::os::unix::fs::symlink(&real, &link).expect("create symlink"); + + let canonicalized = + canonicalize_existing_preserving_symlinks(&link).expect("canonicalize symlink"); + + assert_eq!(canonicalized, link); + } + #[cfg(target_os = "windows")] #[test] fn home_directory_backslash_subpath_is_expanded_in_deserialization() { @@ -350,4 +449,22 @@ mod tests { }; assert_eq!(abs_path_buf.as_path(), home.join("code").as_path()); } + + #[cfg(target_os = "windows")] + #[test] + fn canonicalize_preserving_symlinks_avoids_verbatim_prefixes() { + let temp_dir = tempdir().expect("temp dir"); + + let canonicalized = + canonicalize_preserving_symlinks(temp_dir.path()).expect("canonicalize"); + + assert_eq!( + canonicalized, + dunce::canonicalize(temp_dir.path()).expect("canonicalize temp dir") + ); + assert!( + !canonicalized.to_string_lossy().starts_with(r"\\?\"), + "expected a non-verbatim Windows path, got {canonicalized:?}" + ); + } }