feat(policy): grant nix daemon socket connect via unix_socket_connects (SEA-827)#651
feat(policy): grant nix daemon socket connect via unix_socket_connects (SEA-827)#651mattwilkinsonn wants to merge 3 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 15 minutes and 13 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe ChangesNix daemon socket access in macOS sandbox
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Greptile SummaryFixes daemon-backed nix commands (
Confidence Score: 5/5The change is a targeted, minimal allowlist addition — one fixed socket path, macOS-only at the collection layer, with no template expansion. The grant is a single fixed system path with no user-controlled input, collected only on macOS, and harmless on single-user installs that don't run a daemon. Both new tests are well-scoped: the bundle-layer test uses exact slice equality so any future silent addition to the allowlist will fail CI, and the compile-layer test verifies the path reaches the kernel params end-to-end. No files require special attention. Important Files Changed
Reviews (8): Last reviewed commit: "fix(policy): tighten unix_socket_connect..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
mattwilkinsonn
force-pushed
the
sea-636-unix-socket-connects
branch
from
ec47141 to
1716de4
Compare
mattwilkinsonn
force-pushed
the
sea-827-nix-daemon-socket--hermes
branch
from
09f3200 to
c216d9c
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-policy/src/manifest/sandbox.rs`:
- Around line 1187-1188: The documentation comment for the field starting at
line 1187 is outdated and states that the field is empty for every current
bundle. Update this comment to reflect the current state where the nix
BundleSpec now populates the unix_socket_connects field with the daemon socket.
Revise the wording to accurately describe what bundles currently do with this
field rather than claiming it is always empty.
- Around line 3331-3334: The assertion for the Nix bundle's unix_socket_connects
uses .contains() which is too loose for a security-sensitive permission list, as
it would pass even if additional socket paths are accidentally added later.
Replace the .contains() check with a full slice equality assertion that exactly
matches the expected socket paths. This ensures the allowlist remains tightly
controlled and matches the pattern used in existing curated-domain tests.
🪄 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: a1e9dd31-e7a6-4a49-8e77-a8b556d73656
📒 Files selected for processing (5)
CHANGELOG.mdcrates/seal-policy/src/manifest/sandbox.rscrates/seal-sandbox/src/compile.rsdocs/site/src/content/docs/reference/manifest/sandbox/command-tools.mdxschemas/seal.toml.json
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-policy/src/manifest/sandbox.rs`:
- Around line 1187-1188: The documentation comment for the field starting at
line 1187 is outdated and states that the field is empty for every current
bundle. Update this comment to reflect the current state where the nix
BundleSpec now populates the unix_socket_connects field with the daemon socket.
Revise the wording to accurately describe what bundles currently do with this
field rather than claiming it is always empty.
- Around line 3331-3334: The assertion for the Nix bundle's unix_socket_connects
uses .contains() which is too loose for a security-sensitive permission list, as
it would pass even if additional socket paths are accidentally added later.
Replace the .contains() check with a full slice equality assertion that exactly
matches the expected socket paths. This ensures the allowlist remains tightly
controlled and matches the pattern used in existing curated-domain tests.
🪄 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: a1e9dd31-e7a6-4a49-8e77-a8b556d73656
📒 Files selected for processing (5)
CHANGELOG.mdcrates/seal-policy/src/manifest/sandbox.rscrates/seal-sandbox/src/compile.rsdocs/site/src/content/docs/reference/manifest/sandbox/command-tools.mdxschemas/seal.toml.json
🛑 Comments failed to post (2)
crates/seal-policy/src/manifest/sandbox.rs (2)
1187-1188:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the field docs now that nix populates this list.
Empty for every current bundleis stale because the nixBundleSpecnow setsunix_socket_connectsto the daemon socket.Suggested wording
- /// Empty for every current bundle; populated by bundles that - /// wrap a local-daemon CLI talking over an AF_UNIX socket. + /// Empty for bundles that do not wrap a local-daemon CLI; + /// currently populated by the nix bundle for the daemon + /// control socket.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./// Empty for bundles that do not wrap a local-daemon CLI; /// currently populated by the nix bundle for the daemon /// control socket.🤖 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-policy/src/manifest/sandbox.rs` around lines 1187 - 1188, The documentation comment for the field starting at line 1187 is outdated and states that the field is empty for every current bundle. Update this comment to reflect the current state where the nix BundleSpec now populates the unix_socket_connects field with the daemon socket. Revise the wording to accurately describe what bundles currently do with this field rather than claiming it is always empty.
3331-3334: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Pin the exact nix socket allowlist.
This is a security-sensitive permission list;
.contains(...)would still pass if another socket path were accidentally added later. Match the existing curated-domain tests and assert the full slice.Suggested test tightening
- assert!( - bundle(BundleName::Nix) - .unix_socket_connects - .contains(&"/nix/var/nix/daemon-socket/socket"), - "nix bundle must grant connect to the daemon socket (SEA-827); got {:?}", - bundle(BundleName::Nix).unix_socket_connects + assert_eq!( + bundle(BundleName::Nix).unix_socket_connects, + &["/nix/var/nix/daemon-socket/socket"], + "nix bundle must grant only the daemon socket connect permission (SEA-827)" );🤖 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-policy/src/manifest/sandbox.rs` around lines 3331 - 3334, The assertion for the Nix bundle's unix_socket_connects uses .contains() which is too loose for a security-sensitive permission list, as it would pass even if additional socket paths are accidentally added later. Replace the .contains() check with a full slice equality assertion that exactly matches the expected socket paths. This ensures the allowlist remains tightly controlled and matches the pattern used in existing curated-domain tests.
|
Docs preview: https://sea-827-nix-daemon-socket--h.seal-docs.pages.dev |
Merge activity
|
mattwilkinsonn
force-pushed
the
sea-636-unix-socket-connects
branch
from
1716de4 to
8dd401b
Compare
mattwilkinsonn
force-pushed
the
sea-827-nix-daemon-socket--hermes
branch
2 times, most recently
from
0abaa60 to
9f69558
Compare
mattwilkinsonn
force-pushed
the
sea-636-unix-socket-connects
branch
from
ccd7f43 to
4ee0460
Compare
mattwilkinsonn
force-pushed
the
sea-827-nix-daemon-socket--hermes
branch
from
9f69558 to
009d166
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/manifest/sandbox.rs`:
- Around line 1190-1191: The documentation comment for the unix_socket_connects
field at lines 1190-1191 is outdated and states the field is empty for every
current bundle, but this is no longer accurate since nix bundles now populate it
at line 1974. Update the field documentation to correctly reflect that the field
is populated by nix bundles (in addition to bundles that wrap a local-daemon CLI
talking over an AF_UNIX socket), removing the claim that it remains empty for
every current bundle.
🪄 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: ba9ce1eb-a81d-4c2b-bb05-b86439bb418a
📒 Files selected for processing (5)
CHANGELOG.mdcrates/seal-policy/src/manifest/sandbox.rscrates/seal-sandbox/src/compile.rsdocs/site/src/content/docs/reference/manifest/sandbox/command-tools.mdxschemas/seal.toml.json
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: 1
🤖 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/manifest/sandbox.rs`:
- Around line 1190-1191: The documentation comment for the unix_socket_connects
field at lines 1190-1191 is outdated and states the field is empty for every
current bundle, but this is no longer accurate since nix bundles now populate it
at line 1974. Update the field documentation to correctly reflect that the field
is populated by nix bundles (in addition to bundles that wrap a local-daemon CLI
talking over an AF_UNIX socket), removing the claim that it remains empty for
every current bundle.
🪄 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: ba9ce1eb-a81d-4c2b-bb05-b86439bb418a
📒 Files selected for processing (5)
CHANGELOG.mdcrates/seal-policy/src/manifest/sandbox.rscrates/seal-sandbox/src/compile.rsdocs/site/src/content/docs/reference/manifest/sandbox/command-tools.mdxschemas/seal.toml.json
🛑 Comments failed to post (1)
crates/seal-policy/src/manifest/sandbox.rs (1)
1190-1191: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix stale field documentation to match the new nix population.
Line 1190 says
unix_socket_connectsis empty for every current bundle, but Line 1974 now populates it fornix. Please update the wording to avoid future misreads.Suggested doc fix
- /// Empty for every current bundle; populated by bundles that - /// wrap a local-daemon CLI talking over an AF_UNIX socket. + /// Empty for most bundles; populated by bundles that wrap a + /// local-daemon CLI talking over an AF_UNIX socket (currently `nix`).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./// Empty for most bundles; populated by bundles that wrap a /// local-daemon CLI talking over an AF_UNIX socket (currently `nix`).🤖 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-policy/src/manifest/sandbox.rs` around lines 1190 - 1191, The documentation comment for the unix_socket_connects field at lines 1190-1191 is outdated and states the field is empty for every current bundle, but this is no longer accurate since nix bundles now populate it at line 1974. Update the field documentation to correctly reflect that the field is populated by nix bundles (in addition to bundles that wrap a local-daemon CLI talking over an AF_UNIX socket), removing the claim that it remains empty for every current bundle.
mattwilkinsonn
force-pushed
the
sea-636-unix-socket-connects
branch
from
4ee0460 to
9eb9ac2
Compare
mattwilkinsonn
force-pushed
the
sea-827-nix-daemon-socket--hermes
branch
from
009d166 to
7f7383e
Compare
…rtion (SEA-827 review) Address CodeRabbit review on PR #651: - The `unix_socket_connects` field doc said "Empty for every current bundle" — stale now that the nix bundle populates it. Reword to name the nix bundle as the current populator. - `nix_bundle_grants_daemon_socket_connect` used `.contains()`, which would pass even if an extra socket path were added later. Switch to exact slice equality so the security-sensitive allowlist fails the test on any unreviewed addition. Refs SEA-827 Co-Authored-By: seal <noreply@sealedsecurity.com>
…s (SEA-827) Daemon-backed nix commands (`nix flake update`, `nix build`) connect the control socket at `/nix/var/nix/daemon-socket/socket`. The `/nix` RO bind makes the inode visible, but the macOS sandbox default-denies AF_UNIX `connect()`. The nix bundle now declares the socket in `unix_socket_connects` (the SEA-636 primitive), so the macOS profile emits the `network-outbound` allow. Fixed system path, no `<USER>` template; harmless no-op on single-user installs without a daemon. Closes SEA-827 Co-Authored-By: seal <noreply@sealedsecurity.com>
…rtion (SEA-827 review) Address CodeRabbit review on PR #651: - The `unix_socket_connects` field doc said "Empty for every current bundle" — stale now that the nix bundle populates it. Reword to name the nix bundle as the current populator. - `nix_bundle_grants_daemon_socket_connect` used `.contains()`, which would pass even if an extra socket path were added later. Switch to exact slice equality so the security-sensitive allowlist fails the test on any unreviewed addition. Refs SEA-827 Co-Authored-By: seal <noreply@sealedsecurity.com>
mattwilkinsonn
force-pushed
the
sea-636-unix-socket-connects
branch
from
9eb9ac2 to
b90ce29
Compare
mattwilkinsonn
force-pushed
the
sea-827-nix-daemon-socket--hermes
branch
from
65e1f16 to
a48667f
Compare
1 participant
Pull request
Summary
Daemon-backed nix commands (
nix flake update,nix build, etc.) were failing inside the macOS sandbox withcannot connect to socket … Operation not permitted. Thenixbundle now grants aconnect(2)to the nix daemon control socket at/nix/var/nix/daemon-socket/socketon macOS, where the sandbox default-denies AF_UNIX connects even though the/nixRO bind makes the inode visible.Related issues
Closes SEA-827
Changes
/nix/var/nix/daemon-socket/sockettounix_socket_connectsin thenixbundle spec.nixbundle doc comment, the reference docs, and the JSON schema description to document the new socket grant.Test plan
nix_bundle_grants_daemon_socket_connectinseal-policyasserting the socket path is present in the bundle table.nix_bundle_daemon_socket_reaches_macos_profileinseal-sandboxasserting the socket path propagates end-to-end throughcompile()intoKernelParams.unix_socket_connectsfor a macOSnix:*spawn."nix" => BundleName::Nixto the test fixture bundle-name resolver so the compile-layer test can reference the bundle by name.Notes for reviewers
The socket path is a fixed system path (no
<USER>template) shared by multi-user Linux and Determinate Nix installs on both platforms. On single-user installs that never open the daemon socket, the grant is a harmless no-op.Co-Authored-By: seal noreply@sealedsecurity.com