From caf7dda26d6e3e976db5d5eff349d297fca7c2d0 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Wed, 6 May 2026 00:01:14 -0700 Subject: [PATCH] fix: keep allow aliases lexical in deny matcher Co-authored-by: Codex noreply@openai.com --- codex-rs/protocol/src/permissions.rs | 68 +++++++++++++++++++---- codex-rs/sandboxing/src/seatbelt_tests.rs | 7 ++- 2 files changed, 60 insertions(+), 15 deletions(-) diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index d8e1a35e3c..ddec7c7bc2 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -221,6 +221,7 @@ struct FileSystemSemanticSignature { /// Runtime matcher for read-deny entries in a filesystem sandbox policy. pub struct ReadDenyMatcher { exact_candidates: Vec, + canonical_deny_candidates: Vec, deny_read_matchers: Vec, invalid_pattern: bool, } @@ -236,13 +237,15 @@ impl ReadDenyMatcher { return None; } - // Exact entries are stored with all meaningful path spellings we can - // derive cheaply. That lets direct tool checks preserve the same - // most-specific precedence as the main resolver while still catching - // both a symlink path and its canonical target. - let exact_candidates = file_system_sandbox_policy - .resolved_entries_with_cwd(cwd) - .into_iter() + // Exact entries keep their lexical spelling so the precedence check + // mirrors the main resolver. Deny entries also keep canonical spellings + // so direct tool checks still catch a denied target reached through a + // symlink alias without letting an alias-specific allow reopen the + // canonical target. + let exact_candidates = file_system_sandbox_policy.resolved_entries_with_cwd(cwd); + let canonical_deny_candidates = exact_candidates + .iter() + .filter(|entry| !entry.access.can_read()) .flat_map(|entry| { normalized_and_canonical_candidates(entry.path.as_path()) .into_iter() @@ -272,6 +275,7 @@ impl ReadDenyMatcher { .collect(); Some(Self { exact_candidates, + canonical_deny_candidates, deny_read_matchers, invalid_pattern, }) @@ -286,18 +290,24 @@ impl ReadDenyMatcher { } // Exact entries preserve the same most-specific precedence as the - // policy resolver, across every candidate spelling we know about. + // policy resolver for the lexical spelling the caller asked about. + // Canonical spellings are only considered for deny entries so an alias- + // specific allow does not reopen the canonical target. let path_candidates = normalized_and_canonical_candidates(path); - if self + let lexical_entry = self .exact_candidates .iter() - .filter(|entry| { + .filter(|entry| path.starts_with(entry.path.as_path())) + .max_by_key(|entry| resolved_entry_precedence(entry)); + if lexical_entry.is_some_and(|entry| !entry.access.can_read()) { + return true; + } + if lexical_entry.is_none() + && self.canonical_deny_candidates.iter().any(|entry| { path_candidates .iter() .any(|candidate| candidate.starts_with(entry.path.as_path())) }) - .max_by_key(|entry| resolved_entry_precedence(entry)) - .is_some_and(|entry| !entry.access.can_read()) { return true; } @@ -2903,6 +2913,40 @@ mod tests { )); } + #[cfg(unix)] + #[test] + fn symlinked_allow_alias_does_not_reopen_canonical_denied_path() { + let temp = TempDir::new().expect("tempdir"); + let real_dir = temp.path().join("real"); + let alias_dir = temp.path().join("alias"); + let real_child = real_dir.join("child"); + let alias_child = alias_dir.join("child"); + let real_file = real_child.join("visible.txt"); + let alias_file = alias_child.join("visible.txt"); + std::fs::create_dir_all(&real_child).expect("create real child"); + std::fs::write(&real_file, "visible").expect("write visible"); + symlink_dir(&real_dir, &alias_dir).expect("symlink alias"); + + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: AbsolutePathBuf::from_absolute_path(&real_dir).expect("absolute real"), + }, + access: FileSystemAccessMode::None, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: AbsolutePathBuf::from_absolute_path(&alias_child) + .expect("absolute alias child"), + }, + access: FileSystemAccessMode::Read, + }, + ]); + + assert!(is_read_denied(real_file.as_path(), &policy, temp.path())); + assert!(!is_read_denied(alias_file.as_path(), &policy, temp.path())); + } + #[cfg(unix)] #[test] fn canonical_target_matches_denied_symlink_alias() { diff --git a/codex-rs/sandboxing/src/seatbelt_tests.rs b/codex-rs/sandboxing/src/seatbelt_tests.rs index 4f2e8cf99b..1436bd0c82 100644 --- a/codex-rs/sandboxing/src/seatbelt_tests.rs +++ b/codex-rs/sandboxing/src/seatbelt_tests.rs @@ -355,7 +355,8 @@ fn reopened_readable_children_under_unreadable_roots_get_metadata_traversal() { #[test] fn reopened_children_under_unreadable_roots_work_under_seatbelt() { let tmp = TempDir::new().expect("tempdir"); - let parent = tmp.path().join("home"); + let tmp_path = tmp.path().canonicalize().expect("canonical tempdir"); + let parent = tmp_path.join("home"); let gitconfig = parent.join(".gitconfig"); let blocked = parent.join(".config").join("my-app").join(".env"); let writable_dir = parent.join(".cache").join("uv"); @@ -404,14 +405,14 @@ fn reopened_children_under_unreadable_roots_work_under_seatbelt() { ], file_system_sandbox_policy: &file_system_policy, network_sandbox_policy: NetworkSandboxPolicy::Restricted, - sandbox_policy_cwd: tmp.path(), + sandbox_policy_cwd: &tmp_path, enforce_managed_network: false, network: None, extra_allow_unix_sockets: &[], }); Command::new(MACOS_PATH_TO_SEATBELT_EXECUTABLE) .args(&args) - .current_dir(tmp.path()) + .current_dir(&tmp_path) .output() .expect("execute seatbelt command") };