fix(sandbox): canonicalize deny-rule paths at build; unbreak macOS/Windows read_guard baseline - #5729
Conversation
… root-parent test macOS read_guard baseline (CI red on main): - Rules and exempts were stored lexically (normalize_lexically), while check() compares canonicalized candidates. On macOS the system redirects /var and /tmp to /private/..., so a denied path under a symlinked root never matched the canonical candidate — the deny could be walked around. Canonicalize every rule and exempt path at build time so both sides of path_is_within compare canonical paths (subtree_paths() now hands the OS wrappers the canonical form, which is also correct for Seatbelt). - root_parent_traversal_does_not_escape_above_root asserted Unix path semantics on every platform: a leading '/' is not absolute on Windows, so the test failed there. Split the assertion per platform (drive-rooted Windows expectation).
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if path.as_os_str().is_empty() { | ||
| continue; | ||
| } | ||
| let path = canonicalize_best_effort(&path); |
There was a problem hiding this comment.
📝 Info: Nonexistent rules retain their suffix
canonicalize_best_effort resolves the deepest existing ancestor and reattaches the missing suffix. Configured future paths remain denied without broadening the rule.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Rules are canonicalized at build (symlinked roots such as macOS | ||
| // `/var` → `/private/var` must not split the comparison), so the OS | ||
| // wrapper handoff is the canonical form of the temp dir. | ||
| assert_eq!(paths[0], canonicalize_best_effort(tmp.path())); |
There was a problem hiding this comment.
📝 Info: Canonical wrapper handoff is consistent
The wrapper setter now receives an already canonical path. Seatbelt matches that kernel-resolved spelling, while bubblewrap masks the same resolved filesystem object.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Closing — superseded by the maintainer's own fix, merged in #5724 ("match the read deny-list against a rule's resolved path"). This PR was prepared in parallel and takes the same approach (canonicalize/resolve deny rules at build + per-platform root-parent assertion); the merged fix covers the same ground. Closing to avoid overlap. |
1 participant
Summary
Fixes the
sandbox::read_guardbaseline failures that are currently red onmain's own CI (macOS + Windows) and block every PR targetingmain— including FEAT-021 #5717, whose only red checks are these exact tests.Two independent defects in
crates/tui/src/sandbox/read_guard.rs:1. macOS — deny rules were lexical while candidates were canonical (real deny-bypass gap).
ReadDenylist::buildstored rule and exempt paths withnormalize_lexicallyonly.check()comparescanonicalize_best_effortd candidates against those rules viapath_is_within(component-wise)./var(and/tmp) to/private/.... A temp-dir deny rule stayed/var/folders/...while the canonical candidate was/private/var/folders/...→ first component mismatch → the deny was silently missed. This is why the symlink/..evasion tests (dot_dot_through_a_symlink…,symlink_chains…,relative_read_from_inside_a_denied_tree…,symlinked_parent_directory…,symlink_pointing_into_a_denied_tree…,denial_message_names_the_rule…) fail on macOS but pass on Linux.Fix: canonicalize every rule and exempt path at build time (
canonicalize_best_effort(&normalize_lexically(&path))), so both sides ofpath_is_withincompare canonical paths.subtree_paths()now hands the OS wrappers the canonical form, which is also the correct form for Seatbelt on macOS. Non-existent built-ins (~/.ssh, …) resolve to their canonical parent + suffix as before.canonicalize_best_effortnever fails to produce at least the root-canonicalized path, so no rule can be lost.2. Windows —
root_parent_traversal_does_not_escape_above_rootasserted Unix path semantics everywhere.normalize_lexically(Path::new("/../../etc")) == PathBuf::from("/etc"). On Windows a leading/is not absolute (drive-relative), so the path joinscurrent_dir()and normalizes to the drive root — the assertion failed.subtree_paths_feed_the_os_wrappers_and_omit_the_filename_rulecompared against the lexical form (now canonical).Fix: per-platform assertions — Unix keeps
/../../etc→/etc; Windows uses a drive-rooted pathC:\..\..\etc→C:\etc(the root never pops above itself). The subtree_paths test now asserts the canonical form.Testing
cargo fmt --all -- --checkcargo test -p codewhale-tui --lib --locked sandbox::read_guard— 20 passed / 0 failed (Linux)cargo test -p codewhale-tui --lib --locked sandbox::— 74 passed / 0 failed (Linux)cargo check --workspace --locked— clean/var → /private/varredirect; the per-platform test expectations follow the respective path models); the previous CI matrix onmainreproduced the failures, so a green macOS/Windows run on this PR is the acceptance signal.Note: the only remaining clippy error in this workspace (
too_many_argumentsinruntime_threads.rs:2562) is pre-existing onmainwith this toolchain and unrelated to this change.Paulo Aboim Pinto