diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index c1f3d0724b..d8e1a35e3c 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -220,7 +220,7 @@ struct FileSystemSemanticSignature { /// Runtime matcher for read-deny entries in a filesystem sandbox policy. pub struct ReadDenyMatcher { - denied_candidates: Vec>, + exact_candidates: Vec, deny_read_matchers: Vec, invalid_pattern: bool, } @@ -236,13 +236,25 @@ impl ReadDenyMatcher { return None; } - // Exact roots are stored as all meaningful path spellings we can derive - // cheaply. This lets direct tool checks catch both a symlink path and - // its canonical target without changing the policy entries themselves. - let denied_candidates = file_system_sandbox_policy - .get_unreadable_roots_with_cwd(cwd) + // 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() - .map(|path| normalized_and_canonical_candidates(path.as_path())) + .flat_map(|entry| { + normalized_and_canonical_candidates(entry.path.as_path()) + .into_iter() + .filter_map(move |path| { + AbsolutePathBuf::from_absolute_path(path).ok().map(|path| { + ResolvedFileSystemEntry { + path, + access: entry.access, + } + }) + }) + }) .collect(); // Pattern entries stay as policy-level globs. They are matched at read // time here instead of being snapshotted to startup filesystem state. @@ -259,7 +271,7 @@ impl ReadDenyMatcher { }) .collect(); Some(Self { - denied_candidates, + exact_candidates, deny_read_matchers, invalid_pattern, }) @@ -273,17 +285,20 @@ impl ReadDenyMatcher { return true; } - // Check exact roots against each candidate spelling before evaluating - // glob matchers. Exact entries are subtree denies; glob entries match - // according to the pattern compiler's path-separator rules. + // Exact entries preserve the same most-specific precedence as the + // policy resolver, across every candidate spelling we know about. let path_candidates = normalized_and_canonical_candidates(path); - if self.denied_candidates.iter().any(|denied_candidates| { - path_candidates.iter().any(|candidate| { - denied_candidates.iter().any(|denied_candidate| { - candidate == denied_candidate || candidate.starts_with(denied_candidate) - }) + if self + .exact_candidates + .iter() + .filter(|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; } @@ -2858,6 +2873,36 @@ mod tests { )); } + #[test] + fn exact_path_denies_honor_more_specific_read_allows() { + let temp = TempDir::new().expect("tempdir"); + let denied_dir = AbsolutePathBuf::resolve_path_against_base("denied", temp.path()); + let allowed_dir = denied_dir.join("allowed"); + let allowed_file = allowed_dir.join("visible.txt"); + let blocked_file = denied_dir.join("blocked.txt"); + std::fs::create_dir_all(allowed_dir.as_path()).expect("create allowed dir"); + std::fs::write(allowed_file.as_path(), "visible").expect("write allowed"); + std::fs::write(blocked_file.as_path(), "blocked").expect("write blocked"); + + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: denied_dir }, + access: FileSystemAccessMode::None, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: allowed_dir }, + access: FileSystemAccessMode::Read, + }, + ]); + + assert!(is_read_denied(blocked_file.as_path(), &policy, temp.path())); + assert!(!is_read_denied( + allowed_file.as_path(), + &policy, + temp.path() + )); + } + #[cfg(unix)] #[test] fn canonical_target_matches_denied_symlink_alias() { diff --git a/codex-rs/sandboxing/src/seatbelt.rs b/codex-rs/sandboxing/src/seatbelt.rs index 9383c77b3a..81325056df 100644 --- a/codex-rs/sandboxing/src/seatbelt.rs +++ b/codex-rs/sandboxing/src/seatbelt.rs @@ -404,6 +404,49 @@ fn seatbelt_protected_metadata_name_regex(root: &AbsolutePathBuf, name: &str) -> } } +fn build_seatbelt_reopened_read_traversal_policy( + readable_roots: &[AbsolutePathBuf], + unreadable_roots: &[AbsolutePathBuf], +) -> (String, Vec<(String, PathBuf)>) { + let mut traversal_roots = BTreeMap::new(); + + for readable_root in readable_roots { + for unreadable_root in unreadable_roots.iter().filter(|unreadable_root| { + readable_root + .as_path() + .starts_with(unreadable_root.as_path()) + }) { + let mut ancestor = readable_root.as_path().parent(); + while let Some(path) = ancestor { + if !path.starts_with(unreadable_root.as_path()) { + break; + } + if let Ok(path) = AbsolutePathBuf::from_absolute_path(path) { + traversal_roots + .entry(path.to_string_lossy().to_string()) + .or_insert(path); + } + if path == unreadable_root.as_path() { + break; + } + ancestor = path.parent(); + } + } + } + + let mut policy_components = Vec::new(); + let mut params = Vec::new(); + for (index, root) in traversal_roots.into_values().enumerate() { + let param = format!("READABLE_TRAVERSAL_ROOT_{index}"); + params.push((param.clone(), root.into_path_buf())); + policy_components.push(format!( + "(allow file-read-metadata (literal (param \"{param}\")) (vnode-type DIRECTORY))" + )); + } + + (policy_components.join("\n"), params) +} + fn protected_metadata_names_for_writable_root( file_system_sandbox_policy: &FileSystemSandboxPolicy, writable_root: &WritableRoot, @@ -652,6 +695,7 @@ pub fn create_seatbelt_command_args(args: CreateSeatbeltCommandArgsParams<'_>) - ) }; + let readable_roots = file_system_sandbox_policy.get_readable_roots_with_cwd(sandbox_policy_cwd); let (file_read_policy, file_read_dir_params) = if file_system_sandbox_policy.has_full_disk_read_access() { if unreadable_roots.is_empty() { @@ -665,7 +709,7 @@ pub fn create_seatbelt_command_args(args: CreateSeatbeltCommandArgsParams<'_>) - "READABLE_ROOT", vec![SeatbeltAccessRoot { root: root_absolute_path(), - excluded_subpaths: unreadable_roots, + excluded_subpaths: unreadable_roots.clone(), protected_metadata_names: Vec::new(), }], ); @@ -678,9 +722,9 @@ pub fn create_seatbelt_command_args(args: CreateSeatbeltCommandArgsParams<'_>) - let (policy, params) = build_seatbelt_access_policy( "file-read*", "READABLE_ROOT", - file_system_sandbox_policy - .get_readable_roots_with_cwd(sandbox_policy_cwd) - .into_iter() + readable_roots + .iter() + .cloned() .map(|root| SeatbeltAccessRoot { excluded_subpaths: unreadable_roots .iter() @@ -701,6 +745,8 @@ pub fn create_seatbelt_command_args(args: CreateSeatbeltCommandArgsParams<'_>) - ) } }; + let (reopened_read_traversal_policy, reopened_read_traversal_dir_params) = + build_seatbelt_reopened_read_traversal_policy(&readable_roots, &unreadable_roots); let proxy = proxy_policy_inputs(network, extra_allow_unix_sockets); let network_policy = @@ -712,6 +758,7 @@ pub fn create_seatbelt_command_args(args: CreateSeatbeltCommandArgsParams<'_>) - let mut policy_sections = vec![ MACOS_SEATBELT_BASE_POLICY.to_string(), file_read_policy, + reopened_read_traversal_policy, file_write_policy, deny_read_policy, network_policy, @@ -724,6 +771,7 @@ pub fn create_seatbelt_command_args(args: CreateSeatbeltCommandArgsParams<'_>) - let dir_params = [ file_read_dir_params, + reopened_read_traversal_dir_params, file_write_dir_params, macos_dir_params(), unix_socket_dir_params(&proxy), diff --git a/codex-rs/sandboxing/src/seatbelt_tests.rs b/codex-rs/sandboxing/src/seatbelt_tests.rs index a8f14fc26a..4f2e8cf99b 100644 --- a/codex-rs/sandboxing/src/seatbelt_tests.rs +++ b/codex-rs/sandboxing/src/seatbelt_tests.rs @@ -285,6 +285,166 @@ fn explicit_unreadable_paths_are_excluded_from_readable_roots() { ); } +#[test] +fn reopened_readable_children_under_unreadable_roots_get_metadata_traversal() { + let unreadable = absolute_path("/tmp/codex-home"); + let readable_file = absolute_path("/tmp/codex-home/.gitconfig"); + let writable_dir = absolute_path("/tmp/codex-home/.cache/uv"); + let file_system_policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: unreadable }, + access: FileSystemAccessMode::None, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: readable_file, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: writable_dir }, + access: FileSystemAccessMode::Write, + }, + ]); + + let args = create_seatbelt_command_args(CreateSeatbeltCommandArgsParams { + command: vec!["/bin/true".to_string()], + file_system_sandbox_policy: &file_system_policy, + network_sandbox_policy: NetworkSandboxPolicy::Restricted, + sandbox_policy_cwd: Path::new("/"), + enforce_managed_network: false, + network: None, + extra_allow_unix_sockets: &[], + }); + let unreadable_roots = file_system_policy.get_unreadable_roots_with_cwd(Path::new("/")); + let expected_unreadable_root = unreadable_roots.first().expect("expected unreadable root"); + + let policy = seatbelt_policy_arg(&args); + assert!( + policy.contains( + "(allow file-read-metadata (literal (param \"READABLE_TRAVERSAL_ROOT_0\")) (vnode-type DIRECTORY))" + ), + "expected metadata traversal rule for reopened readable descendants:\n{policy}" + ); + assert!( + policy.contains( + "(allow file-read-metadata (literal (param \"READABLE_TRAVERSAL_ROOT_1\")) (vnode-type DIRECTORY))" + ), + "expected metadata traversal rule for reopened writable descendants:\n{policy}" + ); + let traversal_definitions: Vec = args + .iter() + .filter(|arg| arg.starts_with("-DREADABLE_TRAVERSAL_ROOT_")) + .cloned() + .collect(); + assert_eq!( + traversal_definitions, + vec![ + format!( + "-DREADABLE_TRAVERSAL_ROOT_0={}", + expected_unreadable_root.display() + ), + format!( + "-DREADABLE_TRAVERSAL_ROOT_1={}", + expected_unreadable_root.join(".cache").display() + ), + ] + ); +} + +#[test] +fn reopened_children_under_unreadable_roots_work_under_seatbelt() { + let tmp = TempDir::new().expect("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"); + let writable_file = writable_dir.join("state.txt"); + fs::create_dir_all(blocked.parent().expect("blocked parent")).expect("create blocked parent"); + fs::create_dir_all(&writable_dir).expect("create writable dir"); + fs::write(&gitconfig, "visible").expect("write readable child"); + fs::write(&blocked, "secret").expect("write blocked child"); + + let file_system_policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: AbsolutePathBuf::from_absolute_path(&parent).expect("absolute parent"), + }, + access: FileSystemAccessMode::None, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: AbsolutePathBuf::from_absolute_path(&gitconfig).expect("absolute gitconfig"), + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: AbsolutePathBuf::from_absolute_path(&writable_dir) + .expect("absolute writable dir"), + }, + access: FileSystemAccessMode::Write, + }, + ]); + + let run = |script: &str, path: &Path| { + let args = create_seatbelt_command_args(CreateSeatbeltCommandArgsParams { + command: vec![ + "/bin/bash".to_string(), + "-c".to_string(), + script.to_string(), + "bash".to_string(), + path.to_string_lossy().to_string(), + ], + file_system_sandbox_policy: &file_system_policy, + network_sandbox_policy: NetworkSandboxPolicy::Restricted, + 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()) + .output() + .expect("execute seatbelt command") + }; + + let ls_parent = run(r#"ls "$1" >/dev/null"#, &parent); + assert!( + !ls_parent.status.success(), + "parent listing should stay denied" + ); + + let read_allowed = run(r#"cat "$1" >/dev/null"#, &gitconfig); + assert!( + read_allowed.status.success(), + "allowed child read should work" + ); + + let read_blocked = run(r#"cat "$1" >/dev/null"#, &blocked); + assert!( + !read_blocked.status.success(), + "unlisted child should stay denied" + ); + + let write_allowed = run(r#"printf ok > "$1""#, &writable_file); + assert!( + write_allowed.status.success(), + "allowed writable child should accept writes" + ); + assert_eq!( + fs::read_to_string(&writable_file).expect("read writable child"), + "ok" + ); +} + #[test] fn unreadable_globstar_slash_matches_zero_or_more_directories() { let regex = seatbelt_regex_for_unreadable_glob("/tmp/repo/**/*.env");