From 3bae578af33e43268e7bc2d6d5d0266f704bb554 Mon Sep 17 00:00:00 2001 From: jif Date: Thu, 15 Oct 2026 11:51:44 +0000 Subject: [PATCH] Protect metadata writes through macOS case aliases (#54273) ## Why On case-insensitive macOS filesystems, paths such as `.GIT/config` refer to protected `.git/config` metadata but can pass case-sensitive write permission checks. ## What changed Check native filesystem case sensitivity when matching protected metadata paths, including case aliases in workspace roots. Recheck both the target and policy entries with normalized aliases so explicit metadata grants and narrower restrictions retain their precedence. Return errors when filesystem case sensitivity cannot be established. ## Testing Add macOS coverage for metadata and workspace-root case aliases, explicit grants, narrower restrictions, and missing directories. Strengthen the patch regression test to assert the approval rejection message for both direct `apply_patch` calls and shell interception. GitOrigin-RevId: 8ae2eb97ce6ec71c51bead130515b9ce6c1cdfab --- codex-rs/Cargo.lock | 1 + .../tests/suite/apply_patch_system_aliases.rs | 10 ++ codex-rs/protocol/Cargo.toml | 3 + .../protocol/src/permissions/local_aliases.rs | 135 +++++++++++++++++- .../src/permissions/local_aliases_tests.rs | 98 +++++++++++++ 5 files changed, 244 insertions(+), 3 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 094cd52d20..0dcd53a4e6 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -4336,6 +4336,7 @@ dependencies = [ "icu_locale_core", "icu_provider", "landlock", + "libc", "pretty_assertions", "quick-xml 0.41.0", "schemars 0.8.22", diff --git a/codex-rs/core/tests/suite/apply_patch_system_aliases.rs b/codex-rs/core/tests/suite/apply_patch_system_aliases.rs index 2ba778a4a8..03482611a4 100644 --- a/codex-rs/core/tests/suite/apply_patch_system_aliases.rs +++ b/codex-rs/core/tests/suite/apply_patch_system_aliases.rs @@ -94,6 +94,16 @@ async fn case_variant_metadata_patch_cannot_write_without_approval(shell: bool) ("# changed\n".to_string(), "# original\n".to_string()), ); assert_eq!(responses.requests().len(), 3); + let request = responses.last_request().expect("protected patch response"); + let (output, _) = if shell { + request.function_call_output_content_and_success("protected") + } else { + request.custom_tool_call_output_content_and_success("protected") + } + .expect("protected patch output"); + assert!(output.expect("patch rejection text").contains( + "patch rejected: writing outside of the project; rejected by user approval settings" + )); Ok(()) } diff --git a/codex-rs/protocol/Cargo.toml b/codex-rs/protocol/Cargo.toml index 914ed5260c..8314592a7d 100644 --- a/codex-rs/protocol/Cargo.toml +++ b/codex-rs/protocol/Cargo.toml @@ -56,6 +56,9 @@ wildmatch = { workspace = true } landlock = { workspace = true } seccompiler = { workspace = true } +[target.'cfg(target_os = "macos")'.dependencies] +libc = { workspace = true } + [dev-dependencies] anyhow = { workspace = true } pretty_assertions = { workspace = true } diff --git a/codex-rs/protocol/src/permissions/local_aliases.rs b/codex-rs/protocol/src/permissions/local_aliases.rs index 83c8ba0dd5..c3badbb37b 100644 --- a/codex-rs/protocol/src/permissions/local_aliases.rs +++ b/codex-rs/protocol/src/permissions/local_aliases.rs @@ -1,5 +1,6 @@ //! Prepare local policy paths in the sandbox's trusted system-alias namespace. //! Normalize grants and restrictions once per operation without resolving mutable symlinks. +//! Check native macOS ASCII aliases before approving protected metadata writes. //! Borrow unchanged policies and return normalization failures to fallible callers. use super::FileSystemAccessMode; @@ -8,12 +9,22 @@ use super::FileSystemSandboxEntry; use super::FileSystemSandboxPolicy; use super::FileSystemSandboxPolicyContext; use super::FileSystemSpecialPath; +#[cfg(target_os = "macos")] +use super::PROTECTED_METADATA_PATH_NAMES; use super::local_temporary_directories; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; use std::borrow::Cow; +#[cfg(target_os = "macos")] +use std::ffi::CString; use std::io; +#[cfg(target_os = "macos")] +use std::os::unix::ffi::OsStrExt; +#[cfg(target_os = "macos")] +use std::path::Component; use std::path::Path; +#[cfg(target_os = "macos")] +use std::path::PathBuf; /// Owns native environment bindings so one matcher can serve a whole root calculation. pub(super) struct LocalPolicyContext { @@ -43,7 +54,8 @@ impl LocalPolicyContext { /// An operation-scoped view of a configured policy with local roots resolved. /// Reuse it for each target in the operation; prepare again when policy or -/// environment bindings change. Only target paths are normalized during matching. +/// environment bindings change. Metadata checks also account for native ASCII +/// case aliases without changing executor-independent path comparisons. pub struct LocalFileSystemPolicyMatcher<'a> { policy: Cow<'a, FileSystemSandboxPolicy>, context: FileSystemSandboxPolicyContext<'a>, @@ -100,9 +112,45 @@ impl FileSystemSandboxPolicy { } impl LocalFileSystemPolicyMatcher<'_> { - /// Check write access, preserving any failure to normalize the target path. + /// Check write access, preserving failures to establish native path equivalence. pub fn can_write_path(&self, path: &PathUri) -> io::Result { - self.with_path(path, |path| self.policy.can_write_path(path, &self.context)) + self.with_path(path, |path| -> io::Result { + let writable = self.policy.can_write_path(path, &self.context); + #[cfg(target_os = "macos")] + if writable + && !self + .policy + .has_full_disk_write_access_with_context(&self.context) + { + let native_path = path.to_abs_path()?; + for (root, access) in self.policy.resolved_entries(&self.context) { + if !access.can_write() || root.is_opaque() { + continue; + } + let root = root.to_abs_path()?; + let Some(candidate) = normalize_metadata_case_alias(&native_path, &root)? + else { + continue; + }; + // This second policy view may only deny an already allowed write. + // Normalize both candidate and entries so explicit metadata grants + // and narrower restrictions keep their precedence. + let mut policy = self.policy.clone().into_owned(); + for entry in &mut policy.entries { + if let FileSystemPath::Path { path } = &mut entry.path + && let Some(normalized) = + normalize_metadata_case_alias(&path.to_abs_path()?, &root)? + { + *path = PathUri::from(normalized); + } + } + if !policy.can_write_path(&PathUri::from(candidate), &self.context) { + return Ok(false); + } + } + } + Ok(writable) + })? } pub(super) fn resolve_access(&self, path: &PathUri) -> FileSystemAccessMode { @@ -126,6 +174,87 @@ impl LocalFileSystemPolicyMatcher<'_> { } } +// Match only ASCII case aliases leading to protected metadata. Each differing +// component is checked against its containing filesystem, including ancestors +// above mount points. Mutable symlink targets are never substituted into paths. +#[cfg(target_os = "macos")] +fn normalize_metadata_case_alias( + path: &AbsolutePathBuf, + root: &AbsolutePathBuf, +) -> io::Result> { + let mut relative = path.components(); + let Some(Component::Normal(name)) = relative.nth(root.components().count()) else { + return Ok(None); + }; + let Some(metadata_name) = PROTECTED_METADATA_PATH_NAMES + .iter() + .find(|metadata| name.as_bytes().eq_ignore_ascii_case(metadata.as_bytes())) + else { + return Ok(None); + }; + let mut parent = PathBuf::new(); + for (expected, actual) in root.components().zip(path.components()) { + if expected != actual + && (!expected + .as_os_str() + .as_bytes() + .eq_ignore_ascii_case(actual.as_os_str().as_bytes()) + || !is_case_insensitive_filesystem(&parent)?) + { + return Ok(None); + } + parent.push(expected.as_os_str()); + } + // Exact metadata names still need root alias normalization even when the + // metadata's own filesystem is case-sensitive. + if name.as_bytes() != metadata_name.as_bytes() + && !is_case_insensitive_filesystem(root.as_path())? + { + return Ok(None); + } + Ok(Some(root.join(*metadata_name).join(relative.as_path()))) +} + +#[cfg(target_os = "macos")] +fn is_case_insensitive_filesystem(path: &Path) -> io::Result { + // Missing paths inherit their nearest existing ancestor's semantics. + // An unknown result must never authorize a protected metadata write. + for ancestor in path.ancestors() { + let path = CString::new(ancestor.as_os_str().as_bytes())?; + // SAFETY: path is NUL-terminated and __error points to this thread's + // errno. Reset it to distinguish an unavailable value from a failure. + let sensitivity = unsafe { + *libc::__error() = 0; + libc::pathconf(path.as_ptr(), libc::_PC_CASE_SENSITIVE) + }; + match sensitivity { + 0 => return Ok(true), + 1 => return Ok(false), + -1 => { + let error = io::Error::last_os_error(); + if error.raw_os_error() == Some(0) { + return Err(io::Error::new( + io::ErrorKind::Unsupported, + "filesystem case sensitivity is unavailable", + )); + } + if error.kind() != io::ErrorKind::NotFound { + return Err(error); + } + } + value => { + return Err(io::Error::other(format!( + "unexpected filesystem case sensitivity: {value}", + ))); + } + } + } + Err(io::Error::new( + io::ErrorKind::NotFound, + "no existing metadata parent", + )) +} + #[cfg(all(test, target_os = "macos"))] #[path = "local_aliases_tests.rs"] mod tests; diff --git a/codex-rs/protocol/src/permissions/local_aliases_tests.rs b/codex-rs/protocol/src/permissions/local_aliases_tests.rs index 2e06e016c3..6f7d47e101 100644 --- a/codex-rs/protocol/src/permissions/local_aliases_tests.rs +++ b/codex-rs/protocol/src/permissions/local_aliases_tests.rs @@ -63,3 +63,101 @@ fn equivalent_alias_entries_keep_restrictive_precedence() -> anyhow::Result<()> ); Ok(()) } + +#[test] +fn local_metadata_case_aliases_respect_filesystem_and_explicit_grants() -> anyhow::Result<()> { + let directory = tempfile::tempdir()?; + let parent = PathUri::from_host_native_path(directory.path().canonicalize()?)?; + let workspace = parent.join("workspace")?.to_abs_path()?; + std::fs::create_dir(&workspace)?; + std::fs::create_dir(workspace.join(".git"))?; + let case_insensitive = workspace.join(".GIT").exists(); + let root = PathUri::from_host_native_path(&workspace)?; + let context = FileSystemSandboxPolicyContext { + cwd: &root, + workspace_roots: std::slice::from_ref(&root), + user_home_dir: None, + temporary_directories: None, + }; + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry::new(parent.clone().into(), FileSystemAccessMode::Write), + FileSystemSandboxEntry::new(root.clone().into(), FileSystemAccessMode::Write), + ]); + let matching = policy.prepare_local_matching(&context)?; + for relative in [ + ".GIT/config", + ".CoDeX/config.toml", + ".AGENTS/new", + ".AWS/config", + ] { + let path = root.join(relative)?; + assert_eq!( + ( + matching.can_write_path(&path)?, + policy.can_write_path(&path, &context), + ), + (!case_insensitive, true), + "local aliases must not change executor-independent matching: {relative}", + ); + } + let root_alias = parent.join("WORKSPACE")?; + for relative in [".GIT/config", ".git/config", ".CoDeX/config.toml"] { + let path = root_alias.join(relative)?; + assert_eq!( + ( + matching.can_write_path(&path)?, + policy.can_write_path(&path, &context), + ), + (!case_insensitive, true), + "a broader grant must not bypass metadata protection via a root alias: {relative}", + ); + } + for name in [".git", ".GIT"] { + let mut explicitly_writable = policy.clone(); + explicitly_writable + .entries + .push(FileSystemSandboxEntry::new( + root.join(name)?.into(), + FileSystemAccessMode::Write, + )); + let matching = explicitly_writable.prepare_local_matching(&context)?; + for spelling in [&root, &root_alias] { + assert!( + matching.can_write_path(&spelling.join(".GIT/config")?)?, + "explicit metadata grant must remain effective: {name}", + ); + } + } + for (grant, restriction, candidate) in [ + (".git", ".GIT/private", ".git/private"), + (".GIT", ".git/private", ".GIT/private"), + ] { + for access in [FileSystemAccessMode::Read, FileSystemAccessMode::Deny] { + let mut restricted = policy.clone(); + restricted.entries.extend([ + FileSystemSandboxEntry::new(root.join(grant)?.into(), FileSystemAccessMode::Write), + FileSystemSandboxEntry::new(root.join(restriction)?.into(), access), + ]); + assert_eq!( + restricted + .prepare_local_matching(&context)? + .can_write_path(&root.join(candidate)?)?, + !case_insensitive, + "narrower metadata restrictions must retain precedence: {restriction}", + ); + } + } + let missing_root = root.join("missing")?; + let missing_policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry::new( + missing_root.clone().into(), + FileSystemAccessMode::Write, + )]); + assert_eq!( + missing_policy + .prepare_local_matching(&context)? + .can_write_path(&missing_root.join(".GIT/config")?)?, + !case_insensitive, + ); + assert!(matching.can_write_path(&root.join("nested/.GIT/config")?)?); + Ok(()) +}