fix: preserve reopened descendants under read denies#21311
fix: preserve reopened descendants under read denies#21311viyatb-oai wants to merge 2 commits intomainfrom
Conversation
Co-authored-by: Codex [email protected]
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e01fd53004
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .into_iter() | ||
| .map(|path| normalized_and_canonical_candidates(path.as_path())) | ||
| .flat_map(|entry| { | ||
| normalized_and_canonical_candidates(entry.path.as_path()) |
There was a problem hiding this comment.
Do not canonicalize allow entries for deny matching
ReadDenyMatcher now canonicalizes every exact entry, including read/write allows. If /real is denied and /alias/child is allowed where /alias symlinks to /real, is_read_denied('/real/child/file') becomes false because the allow is stored as /real/child. resolve_access_with_cwd would still deny that canonical path, so direct read-deny checks can bypass an exact deny via a symlinked allow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in caf7dda. ReadDenyMatcher now resolves lexical exact entries first and only falls back to canonicalized deny entries when no lexical entry matched, so an alias-specific allow cannot reopen the canonical denied target. I also added symlinked_allow_alias_does_not_reopen_canonical_denied_path to cover this case.
Co-authored-by: Codex [email protected]
Why
FileSystemSandboxPolicyalready resolves exact-path conflicts by choosing the most-specific matching entry (resolve_access_with_cwd). For example, this profile is meant to deny a broader subtree while reopening a couple of specific descendants:Before this change, the projected macOS policy still could not reach the reopened descendants because traversing
/Users/meitself was denied. In that example, reading/Users/me/.gitconfigand writing under/Users/me/.cache/uvcould fail even though the narrower entries were supposed to win.After this change, the reopened descendants work while the broader deny still holds:
/Users/me/.gitconfigworks/Users/me/.cache/uvworks/Users/mestill fails/Users/me/.ssh/configstay denied unless they are explicitly reopenedFixes #21081.
What changed
ReadDenyMatcherpreserve exact-path precedence instead of treating every denied exact root as an unconditional subtree mask.file-read-metadatagrants only for unreadable ancestor directories that are necessary to reach explicitly reopened readable or writable descendants.Verification
cargo test -p codex-protocolcargo test -p codex-sandboxing