feat(sandbox): unix_socket_connects primitive for AF_UNIX socket grants (SEA-636)#620
Closed
mattwilkinsonn wants to merge 4 commits into
Closed
feat(sandbox): unix_socket_connects primitive for AF_UNIX socket grants (SEA-636)#620mattwilkinsonn wants to merge 4 commits into
mattwilkinsonn wants to merge 4 commits into
Conversation
mattwilkinsonn
changed the base branch from
sea-636-clippy-base
to
graphite-base/620
This was referenced Jun 20, 2026
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesAF_UNIX socket connect surface (SEA-636)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has required the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
Greptile SummaryThis PR adds
Confidence Score: 5/5Safe to merge — the change is well-scoped, all new code paths are covered by unit and integration tests, and the security-sensitive USER validator correctly guards against path traversal. The unix_socket_connects primitive is threaded cleanly through the stack with no regressions to existing hash stability or sandbox profiles. The BASELINE_SBPL addition is a narrow, justified change fixing a latent EPERM. The NOCONNECTTOOL listener-thread deadlock flagged in a prior review has been resolved. No correctness or security issues were identified. No files require special attention. Important Files Changed
Reviews (11): Last reviewed commit: "test(seal-sandbox): fix macOS flake in s..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/seal-policy/src/grant.rs`:
- Around line 768-777: The `unix_socket_connects` field is now part of the grant
policy and requires test coverage for the `from_manifest_entry` behavior. Add a
unit test that verifies two behaviors: first, that non-empty values from the
manifest are correctly copied into the `GrantToolEntry.unix_socket_connects`
field, and second, that when `unix_socket_connects` is an empty list, it is
properly elided from the canonical serialization output. This test should ensure
the field integration with the manifest entry parsing logic is robust and
maintains consistency with the grant policy shape.
In `@crates/seal-sandbox/src/compile.rs`:
- Around line 326-336: The expand_unix_socket_connects() function reads the USER
environment variable directly, which means identical compile() arguments can
produce different socket paths depending on the caller's environment. This
violates the replayable-input contract for this hash-bearing function. To fix
this, add USER as an explicit parameter to the compile() function signature,
thread it through to expand_unix_socket_connects() instead of having that
function read from the process environment, and ensure the USER value is
captured in the audit/replay inputs so the hash-bearing behavior remains
deterministic across different invocations with different USER values.
- Around line 7731-7770: Add a new integration test after the existing
expand_user_placeholder tests that creates a GrantToolEntry with
unix_socket_connects field containing a path with the <USER> placeholder, sets
up the USER environment variable and a /tmp to /private/tmp stub mapping, then
calls the compile() function and asserts that the resulting
KernelParams.unix_socket_connects contains both the resolved /tmp and
/private/tmp canonical forms. This will catch regressions in apply_bundles(),
compile() wiring, or the canonical dual-emission logic for the
unix_socket_connects field that the current unit tests of
expand_user_placeholder() alone would not detect.
- Around line 1364-1373: The issue is that dual_emit() fails to emit both the
literal and canonical forms of socket paths when the socket file does not exist
yet (e.g., during daemon startup or restart on macOS). To fix this, modify the
logic in expand_unix_socket_connects to canonicalize the parent directory of the
socket path instead of the full path, since the parent directory always exists
but the socket file may not. Before calling dual_emit() with the path variable,
extract the parent directory, canonicalize it, then reconstruct the full socket
path with the canonicalized parent and the socket filename. This ensures that on
macOS where /tmp firmlinks to /private/tmp, both forms of the socket path will
be properly emitted even if the socket does not yet exist.
In `@crates/seal-sandbox/src/kernel_params.rs`:
- Line 514: The fixture at line 514 initializes `unix_socket_connects` as an
empty vector, but there is no focused test case (parallel to the existing
`dbus_filter_rules_round_trip_and_hash` test) that verifies non-empty
`unix_socket_connects` values serialize and deserialize correctly and affect the
kernel-params hash. Add a new test function similar in structure to
`dbus_filter_rules_round_trip_and_hash` that validates when
`unix_socket_connects` is populated with actual socket connection paths, the
serialization round-trips correctly and produces a different hash compared to
the default empty configuration.
In `@crates/seal-sandbox/tests/bwrap_integration.rs`:
- Around line 1107-1114: The listener thread is blocked on accept() waiting for
a client connection, but when the skip condition for missing connect tools is
triggered, drop(rx) is called followed by listener_handle.join(). Since no
client will ever connect in this scenario, the join() blocks indefinitely and
hangs the test. To fix this, before calling listener_handle.join() in the skip
branch, you need to unblock the listener thread from its accept() call. This can
be done by closing the listener socket itself before joining, which will cause
the accept() call to return with an error that the listener thread can handle
and exit cleanly. Ensure the listener thread is designed to propagate or handle
this socket closure error gracefully instead of panicking.
In `@crates/seal-tui/src/approval.rs`:
- Line 983: The test fixture for `unix_socket_connects` is initialized with an
empty vector, which means the test does not actually verify that AF_UNIX socket
grants are displayed to reviewers. Replace the empty `Vec::new()` initialization
of `unix_socket_connects` with at least one test value representing a Unix
socket connection, then add an assertion that checks this value appears in the
`view.display` output to ensure the functionality is properly tested.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c8b37a55-3795-4598-9bc3-333a2b0511a6
⛔ Files ignored due to path filters (5)
crates/seal-sandbox/src/snapshots/seal_sandbox__macos__tests__insta_profile_all_mask_on.snapis excluded by!**/*.snapcrates/seal-sandbox/src/snapshots/seal_sandbox__macos__tests__insta_profile_none_mask_on.snapis excluded by!**/*.snapcrates/seal-sandbox/src/snapshots/seal_sandbox__macos__tests__insta_profile_permissive_mask_on.snapis excluded by!**/*.snapcrates/seal-sandbox/src/snapshots/seal_sandbox__macos__tests__insta_profile_system_mask_on.snapis excluded by!**/*.snapcrates/seal-sandbox/src/snapshots/seal_sandbox__macos__tests__insta_profile_with_proxy_network.snapis excluded by!**/*.snap
📒 Files selected for processing (13)
CHANGELOG.mdcrates/seal-policy/src/grant.rscrates/seal-policy/src/manifest/sandbox.rscrates/seal-runtime/src/scope/sandbox_spawn/linux.rscrates/seal-runtime/src/scope/sandbox_spawn/macos.rscrates/seal-runtime/src/scope/sandbox_spawn/mod.rscrates/seal-sandbox/src/compile.rscrates/seal-sandbox/src/kernel_params.rscrates/seal-sandbox/src/linux.rscrates/seal-sandbox/src/macos.rscrates/seal-sandbox/tests/bwrap_integration.rscrates/seal-sandbox/tests/sandbox_exec_integration.rscrates/seal-tui/src/approval.rs
mattwilkinsonn
changed the base branch from
graphite-base/620
to
sea-636-clippy-base
mattwilkinsonn
changed the base branch from
sea-636-clippy-base
to
graphite-base/620
mattwilkinsonn
force-pushed
the
sea-636-unix-socket-connects
branch
from
b4b5d3a to
f85ae92
Compare
mattwilkinsonn
force-pushed
the
graphite-base/620
branch
from
68888c7 to
447ac20
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/seal-sandbox/src/compile.rs`:
- Around line 1349-1352: The rustdoc comment for the helper function in the
compile module contains outdated information about how the user parameter is
sourced. The documentation currently states that the expansion source is read
via seal_utils::env::var, but the implementation now accepts user as a
caller-provided parameter instead. Update the rustdoc comment to accurately
reflect that the user is passed by the caller rather than being read from the
daemon's $USER environment variable through seal_utils::env::var.
In `@crates/seal-sandbox/tests/sandbox_exec_integration.rs`:
- Around line 890-894: The keychain sandbox-exec tests need to be gated with a
nesting probe check to prevent hard-failures when running under an outer
sandbox. Add a guard condition using sandbox_exec_can_nest() at the beginning of
the test functions that start around line 881 and line 924 (the ones checking
binary presence), similar to how other sandbox-exec integration tests are
protected. Apply the same gate to the code at lines 933-937. This will skip
these tests when nesting is not available instead of allowing them to fail with
sandbox_apply EPERM errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 01802dde-85da-457b-83a4-10c59904ff24
⛔ Files ignored due to path filters (5)
crates/seal-sandbox/src/snapshots/seal_sandbox__macos__tests__insta_profile_all_mask_on.snapis excluded by!**/*.snapcrates/seal-sandbox/src/snapshots/seal_sandbox__macos__tests__insta_profile_none_mask_on.snapis excluded by!**/*.snapcrates/seal-sandbox/src/snapshots/seal_sandbox__macos__tests__insta_profile_permissive_mask_on.snapis excluded by!**/*.snapcrates/seal-sandbox/src/snapshots/seal_sandbox__macos__tests__insta_profile_system_mask_on.snapis excluded by!**/*.snapcrates/seal-sandbox/src/snapshots/seal_sandbox__macos__tests__insta_profile_with_proxy_network.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
CHANGELOG.mdcrates/seal-policy/src/grant.rscrates/seal-policy/src/manifest/sandbox.rscrates/seal-runtime/src/scope/sandbox_spawn/linux.rscrates/seal-runtime/src/scope/sandbox_spawn/macos.rscrates/seal-runtime/src/scope/sandbox_spawn/mod.rscrates/seal-runtime/src/scope/tool_scope.rscrates/seal-runtime/tests/bwrap_dispatcher_integration.rscrates/seal-sandbox/src/compile.rscrates/seal-sandbox/src/kernel_params.rscrates/seal-sandbox/src/linux.rscrates/seal-sandbox/src/macos.rscrates/seal-sandbox/tests/bwrap_integration.rscrates/seal-sandbox/tests/sandbox_exec_integration.rscrates/seal-tui/src/approval.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/seal-sandbox/src/compile.rs`:
- Around line 1349-1352: The rustdoc comment for the helper function in the
compile module contains outdated information about how the user parameter is
sourced. The documentation currently states that the expansion source is read
via seal_utils::env::var, but the implementation now accepts user as a
caller-provided parameter instead. Update the rustdoc comment to accurately
reflect that the user is passed by the caller rather than being read from the
daemon's $USER environment variable through seal_utils::env::var.
In `@crates/seal-sandbox/tests/sandbox_exec_integration.rs`:
- Around line 890-894: The keychain sandbox-exec tests need to be gated with a
nesting probe check to prevent hard-failures when running under an outer
sandbox. Add a guard condition using sandbox_exec_can_nest() at the beginning of
the test functions that start around line 881 and line 924 (the ones checking
binary presence), similar to how other sandbox-exec integration tests are
protected. Apply the same gate to the code at lines 933-937. This will skip
these tests when nesting is not available instead of allowing them to fail with
sandbox_apply EPERM errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 01802dde-85da-457b-83a4-10c59904ff24
⛔ Files ignored due to path filters (5)
crates/seal-sandbox/src/snapshots/seal_sandbox__macos__tests__insta_profile_all_mask_on.snapis excluded by!**/*.snapcrates/seal-sandbox/src/snapshots/seal_sandbox__macos__tests__insta_profile_none_mask_on.snapis excluded by!**/*.snapcrates/seal-sandbox/src/snapshots/seal_sandbox__macos__tests__insta_profile_permissive_mask_on.snapis excluded by!**/*.snapcrates/seal-sandbox/src/snapshots/seal_sandbox__macos__tests__insta_profile_system_mask_on.snapis excluded by!**/*.snapcrates/seal-sandbox/src/snapshots/seal_sandbox__macos__tests__insta_profile_with_proxy_network.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
CHANGELOG.mdcrates/seal-policy/src/grant.rscrates/seal-policy/src/manifest/sandbox.rscrates/seal-runtime/src/scope/sandbox_spawn/linux.rscrates/seal-runtime/src/scope/sandbox_spawn/macos.rscrates/seal-runtime/src/scope/sandbox_spawn/mod.rscrates/seal-runtime/src/scope/tool_scope.rscrates/seal-runtime/tests/bwrap_dispatcher_integration.rscrates/seal-sandbox/src/compile.rscrates/seal-sandbox/src/kernel_params.rscrates/seal-sandbox/src/linux.rscrates/seal-sandbox/src/macos.rscrates/seal-sandbox/tests/bwrap_integration.rscrates/seal-sandbox/tests/sandbox_exec_integration.rscrates/seal-tui/src/approval.rs
🛑 Comments failed to post (2)
crates/seal-sandbox/src/compile.rs (1)
1349-1352:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the stale
$USERsource docs.The helper no longer reads
seal_utils::env::var; it uses the caller-provideduser, so the rustdoc currently contradicts the implementation.📝 Proposed doc fix
-/// `$USER`, read via `seal_utils::env::var` so the test harness -/// can override it through the same scoped-env mechanism the -/// rest of the compile path uses. +/// `$USER`, supplied by the caller as `user` so `compile()` +/// remains pure on its explicit inputs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/seal-sandbox/src/compile.rs` around lines 1349 - 1352, The rustdoc comment for the helper function in the compile module contains outdated information about how the user parameter is sourced. The documentation currently states that the expansion source is read via seal_utils::env::var, but the implementation now accepts user as a caller-provided parameter instead. Update the rustdoc comment to accurately reflect that the user is passed by the caller rather than being read from the daemon's $USER environment variable through seal_utils::env::var.crates/seal-sandbox/tests/sandbox_exec_integration.rs (1)
890-894:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the nesting probe gate to the keychain sandbox-exec tests.
These tests still only check binary presence (functions starting at Line 881 and Line 924) and can hard-fail with
sandbox_applyEPERM under an outer sandbox. Gate them withsandbox_exec_can_nest()like the other sandbox-exec integration tests.Suggested patch
fn keychain_granted_spawn_can_read_keychains_dir() { - let Some(_) = find_sandbox_exec() else { - eprintln!("sandbox-exec not present — skipping"); + if !sandbox_exec_can_nest() { + eprintln!( + "sandbox-exec can't nest (either missing or running inside an outer \ + sandbox-exec that EPERMs sandbox_apply) — skipping" + ); return; - }; + } let Some(sh) = resolve_on_path("sh") else { eprintln!("no sh on PATH — skipping"); return; @@ fn keychain_ungranted_spawn_cannot_read_keychains_dir() { - let Some(_) = find_sandbox_exec() else { - eprintln!("sandbox-exec not present — skipping"); + if !sandbox_exec_can_nest() { + eprintln!( + "sandbox-exec can't nest (either missing or running inside an outer \ + sandbox-exec that EPERMs sandbox_apply) — skipping" + ); return; - }; + } let Some(sh) = resolve_on_path("sh") else { eprintln!("no sh on PATH — skipping"); return;Also applies to: 933-937
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/seal-sandbox/tests/sandbox_exec_integration.rs` around lines 890 - 894, The keychain sandbox-exec tests need to be gated with a nesting probe check to prevent hard-failures when running under an outer sandbox. Add a guard condition using sandbox_exec_can_nest() at the beginning of the test functions that start around line 881 and line 924 (the ones checking binary presence), similar to how other sandbox-exec integration tests are protected. Apply the same gate to the code at lines 933-937. This will skip these tests when nesting is not available instead of allowing them to fail with sandbox_apply EPERM errors.
mattwilkinsonn
force-pushed
the
sea-636-unix-socket-connects
branch
from
f85ae92 to
e036454
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/seal-sandbox/src/compile.rs`:
- Around line 1390-1393: The validation check at line 1390 in the `user_env`
function is incomplete and allows dot-segments (`.` and `..`) to pass through,
which could enable path traversal attacks. Extend the existing condition that
checks for `/`, `\\`, and `\0` characters to also reject the strings `.` and
`..` before the substitution occurs in `tail.replace("<USER>", u)`. Add checks
to ensure the user variable `u` is not equal to `.` or `..` as part of the same
validation condition.
- Around line 8093-8100: The stub_canon closure in the test currently
canonicalizes the full socket path directly, which prevents the absent-socket
parent fallback code path from being exercised. Modify the closure to return
None when the path contains a socket file (such as paths ending with .socket),
and only canonicalize the parent directory path `/tmp`. This ensures that when
the closure is invoked with a socket path, it falls back to canonicalizing the
parent directory instead, properly exercising the absent-socket parent fallback
logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eb707db8-c8a3-4a13-9682-ee679e9f5251
⛔ Files ignored due to path filters (5)
crates/seal-sandbox/src/snapshots/seal_sandbox__macos__tests__insta_profile_all_mask_on.snapis excluded by!**/*.snapcrates/seal-sandbox/src/snapshots/seal_sandbox__macos__tests__insta_profile_none_mask_on.snapis excluded by!**/*.snapcrates/seal-sandbox/src/snapshots/seal_sandbox__macos__tests__insta_profile_permissive_mask_on.snapis excluded by!**/*.snapcrates/seal-sandbox/src/snapshots/seal_sandbox__macos__tests__insta_profile_system_mask_on.snapis excluded by!**/*.snapcrates/seal-sandbox/src/snapshots/seal_sandbox__macos__tests__insta_profile_with_proxy_network.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
CHANGELOG.mdcrates/seal-policy/src/grant.rscrates/seal-policy/src/manifest/sandbox.rscrates/seal-runtime/src/scope/sandbox_spawn/linux.rscrates/seal-runtime/src/scope/sandbox_spawn/macos.rscrates/seal-runtime/src/scope/sandbox_spawn/mod.rscrates/seal-runtime/src/scope/tool_scope.rscrates/seal-runtime/tests/bwrap_dispatcher_integration.rscrates/seal-sandbox/src/compile.rscrates/seal-sandbox/src/kernel_params.rscrates/seal-sandbox/src/linux.rscrates/seal-sandbox/src/macos.rscrates/seal-sandbox/tests/bwrap_integration.rscrates/seal-sandbox/tests/sandbox_exec_integration.rscrates/seal-tui/src/approval.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/seal-sandbox/src/compile.rs (1)
8295-8302:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not drop
unix_socket_connectson Linux.Line 8300 makes compile intentionally skip socket grants on Linux, but the Linux backend consumes
KernelParams.unix_socket_connectsto emit--bind-try. With the compile-side macOS gate, Linux AF_UNIX grants never reach bwrap, so the new primitive is ineffective on Linux. Remove thePlatform::Macoscollection gate and update this test to assert the resolved socket path is present.Suggested test expectation update
- fn socket_connects_not_collected_on_linux() { + fn socket_connects_collected_on_linux_for_bind_try() { @@ - // AF_UNIX socket-connect grants are macOS-only in effect: - // the load-bearing emission is the sandbox-exec - // `network-outbound` literal rule, and the only bundle that - // declares sockets (yabai) is macOS-only. `apply_bundles` - // gates the collection on `Platform::Macos`, so the same - // grant compiled for Linux yields no socket connects (no - // `--bind-try` of a host inode into the namespace). + // AF_UNIX socket-connect grants must reach KernelParams on + // Linux so the bwrap backend can emit `--bind-try`. @@ - assert!( - p.unix_socket_connects.is_empty(), - "socket connects must not be collected on Linux; got {:?}", - p.unix_socket_connects - ); + assert_eq!( + p.unix_socket_connects, + vec![PathBuf::from("/tmp/wm_alice.socket")], + "socket connects must be collected on Linux for bwrap --bind-try" + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/seal-sandbox/src/compile.rs` around lines 8295 - 8302, The test `socket_connects_not_collected_on_linux` and the corresponding compile-side code incorrectly gate `unix_socket_connects` collection to macOS only via `Platform::Macos`, but the Linux backend actually needs these socket connects to emit `--bind-try` parameters to bwrap. Remove the `Platform::Macos` conditional gate that prevents socket grant collection on Linux, then update the `socket_connects_not_collected_on_linux` test to assert that the resolved socket path is present in the compiled output for Linux instead of asserting that no socket connects were collected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/seal-sandbox/src/compile.rs`:
- Around line 8295-8302: The test `socket_connects_not_collected_on_linux` and
the corresponding compile-side code incorrectly gate `unix_socket_connects`
collection to macOS only via `Platform::Macos`, but the Linux backend actually
needs these socket connects to emit `--bind-try` parameters to bwrap. Remove the
`Platform::Macos` conditional gate that prevents socket grant collection on
Linux, then update the `socket_connects_not_collected_on_linux` test to assert
that the resolved socket path is present in the compiled output for Linux
instead of asserting that no socket connects were collected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6d858f57-6bfc-4be2-bd5f-c9dbdcf37581
📒 Files selected for processing (2)
crates/seal-policy/src/grant.rscrates/seal-sandbox/src/compile.rs
mattwilkinsonn
force-pushed
the
sea-636-unix-socket-connects
branch
from
ec47141 to
1716de4
Compare
Merge activity
|
mattwilkinsonn
force-pushed
the
sea-636-unix-socket-connects
branch
3 times, most recently
from
4ee0460 to
9eb9ac2
Compare
…ts (SEA-636) Adds the unix_socket_connects BundleSpec field + per-platform emission machinery so a curated bundle can authorize AF_UNIX connect(2) to a host daemon socket from inside the sandbox. - macOS: emits (allow network-outbound (literal <path>)) SBPL rules in compile_profile, dual-emitted through host_fs canonicalize so /tmp <-> /private/tmp firmlink variants both land on a matching rule. - Linux: emits --bind-try <path> <path> in compile_into so the host socket inode is bind-mounted into the bwrap namespace. <USER> placeholder expands against the daemon's $USER at compile() time. Also lands the macOS SBPL (/private) literal read allow + sandbox_exec_can_nest() probe so the integration tests run/skip cleanly in-session. Co-Authored-By: seal <noreply@sealedsecurity.com>
Reject `.` / `..` dot-segments in the `<USER>` placeholder so a template like `/tmp/<USER>/sock` can't be widened to `/tmp/../sock`. Add compile-level coverage for the absent-socket parent fallback (the `None` canonicalize branch) and a grant-level test pinning that `unix_socket_connects` round-trips in the canonical form when populated and elides when empty. Refs SEA-636 Co-Authored-By: seal <noreply@sealedsecurity.com>
`sandboxed_curl_reaches_loopback_proxy_with_bridge_rules` flaked on the macOS CI runner (`curl-exit=56`, `connect_seen` never set). The stub proxy's accept loop read the accepted socket once with `read().unwrap_or(0)` — but on macOS the accepted socket inherits the listener's non-blocking mode (BSD semantics), so the read can return `WouldBlock` before curl's CONNECT bytes arrive, miss the request line, and leave the flag false even though the connect reached the listener. Force the accepted socket to blocking mode with a bounded read timeout and loop until the request line arrives. Refs SEA-636 Co-Authored-By: seal <noreply@sealedsecurity.com>
mattwilkinsonn
force-pushed
the
sea-636-unix-socket-connects
branch
from
9eb9ac2 to
b90ce29
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull request
Summary
Adds
unix_socket_connectsas a first-class primitive onBundleSpecandGrantToolEntry, allowing curated bundles to declare AF_UNIX socket paths their CLI needs toconnect(2)to. On macOS, the compile pipeline emits(allow network-outbound (literal "..."))SBPL rules with literal+canonical dual-emission to handle/tmp↔/private/tmpfirmlink resolution. On Linux, each entry emits a--bind-try <path> <path>so the host-side socket inode is reachable inside the bwrap mount namespace.Related issues
Refs SEA-636
Changes
unix_socket_connects: &'static [&'static str]toBundleSpec(currently&[]for all existing bundles) andunix_socket_connects: Vec<String>toGrantToolEntry, withskip_serializing_if = "Vec::is_empty"so existing grant hashes don't churn.unix_socket_connects: Vec<PathBuf>toKernelParamswith the same elision policy.<USER>template substitution in socket paths is resolved atcompile()time from the daemon's captured$USER(threaded in asuser_env: Option<&str>) so the grant hash stays per-user-agnostic while the compiled kernel params are user-specific.dual_emit_socket_connectcanonicalizes the parent directory when the socket file itself is absent (daemon stopped or lazy-created), recovering the/private/tmp/...firmlink form for the SBPL profile even when the socket inode doesn't exist yet.compile_profileemits one(allow network-outbound (literal ...))rule per resolved socket entry, gated on a non-empty list.compile_bwrap_argsemits one--bind-try <path> <path>per entry; collection inapply_bundlesis gated onPlatform::Macossince the only current consumer (yabai) is macOS-only, keeping the Linux profile clean.(allow file-read* (literal "/private"))to the macOS SBPL baseline sorealpath/canonicalizecalls thatlstat("/private")during firmlink traversal don't hit EPERM.sandbox_exec_can_nest()probe added to macOS integration tests so they skip gracefully when running inside an outersandbox-execwrapper (e.g.cargo nextestfrom a seal session) rather than failing with EPERM.unix_socket_connectsentries to the reviewer.Test plan
expand_user_placeholder_*unit tests cover passthrough, substitution, missing$USER, path-unsafe values (traversal, empty, NUL, separators), and multiple-occurrence replacement.compile_resolves_and_dual_emits_socket_connectsverifies the fullcompile()→expand_unix_socket_connects→KernelParamswiring with a stub that firmlink-maps/tmp/...→/private/tmp/....compile_dual_emits_socket_connect_when_socket_file_absentdrives the parent-directory fallback branch where the socket file doesn't exist at compile time.compile_socket_connects_track_user_env_not_process_envconfirms that two compiles differing only inuser_envproduce different socket paths and different kernel-params hashes, and thatNonedrops the templated entry.socket_connects_not_collected_on_linuxasserts the Linux compile path produces an emptyunix_socket_connects.unix_socket_connects_emits_bind_try,_emits_one_bind_per_entry,_empty_*, and_bind_emitted_before_tmpdircover the Linux bwrap argv shape.unix_socket_connects_emits_network_outbound_literal,_emits_multiple_literals_in_order,_empty_*, and_path_escapes_quotes_and_backslashescover the macOS SBPL emission.unix_socket_connects_round_trip_and_hashcoversKernelParamsserialization and hash sensitivity.sandbox_unix_socket_connects_canonical_form_round_trips_and_elidescoversGrantToolEntrycanonical-form round-trip and elision.unix_socket_connect_succeeds_inside_namespace(bwrap integration): starts a host AF_UNIX listener in a directory outside all fixture binds, setsunix_socket_connects, runs an in-sandboxnc -U/ python3 connect, and asserts the marker round-trips to the host listener.unix_socket_connect_fails_inside_namespace_without_bind(negative): same setup without the grant, asserts the in-sandbox connect fails with ENOENT/ECONNREFUSED.Notes for reviewers
Socket-connect collection in
apply_bundlesis explicitly gated onPlatform::Macos— a deliberate scope cut while yabai is the only declared consumer. A future Linux-daemon bundle (tmux socket, i3-msg, etc.) would lift that guard intentionally; the--bind-tryprimitive itself is already exercised by the integration tests. The<USER>safety validator rejects empty strings, dot-segments (./..), path separators, backslashes, and NUL bytes to prevent a malformed$USERfrom escaping the intended/tmp/<daemon>_<USER>.socketshape and widening the connect grant.Co-Authored-By: seal noreply@sealedsecurity.com
Co-Authored-By: seal noreply@sealedsecurity.com