feat(seal-policy): add DbusFilterRule typed D-Bus filter rules + dbus_filter field on BundleSpec/GrantToolEntry/KernelParams (SEA-780 slice 1)#511
Closed
mattwilkinsonn wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds typed D-Bus session-bus filter rules to the manifest, validates them, renders rules to xdg-dbus-proxy flag strings into grants, aggregates sorted/deduped rules into KernelParams during compile, and updates tests/fixtures. ChangesD-Bus Session-Bus Filtering for Sandbox Bundles
sequenceDiagram
participant Manifest as manifest::BundleSpec
participant Validator as DbusFilterRule::validate
participant GrantSynth as GrantToolEntry::from_manifest_entry
participant Compile as seal_sandbox::compile
participant Kernel as KernelParams
Manifest->>Validator: validate each rule (rule.validate())
Manifest->>GrantSynth: provide DbusFilterRule items
GrantSynth->>GrantSynth: rule.to_flag(), sort, dedupe
GrantSynth-->>Compile: include dbus_filter strings
Compile->>Compile: resolve_dbus_filter_rules()
Compile-->>Kernel: set dbus_filter_rules
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. |
This was referenced Jun 11, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/grant.rs`:
- Around line 713-725: The dbus_filter Vec built from bundle.dbus_filter
currently preserves catalog order and duplicates, causing different grant hashes
for semantically identical policies; after mapping with rule.to_flag() but
before assigning into the grant payload (the local dbus_filter variable used by
GrantToolEntry), sort_unstable the Vec and then dedup it so the rendered flags
are canonicalized (stable order, no duplicates) to match the downstream
KernelParams normalization.
In `@crates/seal-policy/src/manifest/dbus_filter.rs`:
- Around line 53-80: Update the validation logic to match the xdg-dbus-proxy
NAME/RULE grammar: in validate_bus_name (affecting BusName) allow
segment-leading '-' per the file's documented regex (i.e., accept [A-Za-z_-] as
first char), in validate_interface_match (affecting InterfaceMatch) permit '.'
in names and support the interface wildcard form '.*' while still rejecting
member names that start with a digit (so enforce member-leading char ≠ digit),
and in validate_path_match (affecting PathMatch) prohibit '*' in arbitrary path
segments and only accept the documented subtree wildcard as a trailing '/*'
suffix; keep error returns using DbusFilterRuleError variants already used so
callers (e.g., try_from) continue to get DbusFilterRuleError::Invalid... on
malformed input.
🪄 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: c0b77c7f-dbbd-4d7a-b582-c5dd072826e4
📒 Files selected for processing (14)
crates/seal-policy/src/grant.rscrates/seal-policy/src/manifest.rscrates/seal-policy/src/manifest/dbus_filter.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/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.rs
Greptile SummaryThis PR introduces typed D-Bus session-bus filter rules (
Confidence Score: 5/5All existing bundles default to empty dbus_filter, so no behavioral change reaches any current spawn; the new audit-chain fields elide from canonical JSON when empty, so no existing grant re-signs. The typed newtype validation logic is correct, the applicability predicate in resolve_dbus_filter_rules exactly mirrors apply_bundles (direnv-wrap and gh credential-helper widenings included), the HashSet dedup is O(n) and correct, and the serde elision guards are in place on all three new fields. The catalog test every_bundle_dbus_filter_rule_validates closes the new_unchecked safety gap. No logic errors, data-loss paths, or audit-hash stability risks were found. No files require special attention; the core logic lives in dbus_filter.rs and compile.rs, both of which are well-tested and correct. Important Files Changed
Reviews (7): Last reviewed commit: "test(seal-sandbox): address PR #511 roun..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
mattwilkinsonn
added a commit
that referenced
this pull request
Jun 11, 2026
…ilter validators with xdg-dbus-proxy grammar, canonicalize grant dbus_filter, HashSet dedup (SEA-780)
mattwilkinsonn
added a commit
that referenced
this pull request
Jun 11, 2026
…ilter validators with xdg-dbus-proxy grammar, canonicalize grant dbus_filter, HashSet dedup (SEA-780)
mattwilkinsonn
force-pushed
the
sea-780-dbus-proxy-slice-1--athena
branch
from
288fa51 to
9c3e96c
Compare
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-policy/src/manifest/sandbox.rs (1)
3333-3352:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove duplicated doc comment block.
Lines 3333-3352 contain the same doc comment twice. The second copy (starting at line 3343) duplicates the first. Only one copy is needed.
🧹 Suggested fix
- /// Every entry in a bundle's `psl_exemptions` list must - /// match at least one entry in the same bundle's `domains` - /// list. An exemption for a pattern the bundle doesn't - /// actually use is dead config — and worse, it could mask - /// a typo: a future engineer adding `*.foo.bar.com` to - /// `domains` (which is a PSL-rejected pattern if - /// `bar.com` is a public suffix) might leave the unrelated - /// `*.baz.qux.com` in `psl_exemptions` and assume that - /// covers the new entry. The 1:1 check forces exemptions - /// to be explicitly tied to the domains they unblock. /// Every entry in a bundle's `psl_exemptions` list must /// match at least one entry in the same bundle's `domains` /// list. An exemption for a pattern the bundle doesn't🤖 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 3333 - 3352, Remove the duplicated doc comment block describing the psl_exemptions/domains 1:1 check so only a single copy remains immediately above the item it documents; locate the repeated paragraph that starts "Every entry in a bundle's `psl_exemptions` list must match at least one entry in the same bundle's `domains` list" and delete the second occurrence, preserving the first intact.
🤖 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-policy/src/manifest/sandbox.rs`:
- Around line 3333-3352: Remove the duplicated doc comment block describing the
psl_exemptions/domains 1:1 check so only a single copy remains immediately above
the item it documents; locate the repeated paragraph that starts "Every entry in
a bundle's `psl_exemptions` list must match at least one entry in the same
bundle's `domains` list" and delete the second occurrence, preserving the first
intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 10f7052a-e0ad-4d2d-a959-2a88b6637600
📒 Files selected for processing (14)
crates/seal-policy/src/grant.rscrates/seal-policy/src/manifest.rscrates/seal-policy/src/manifest/dbus_filter.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/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.rs
mattwilkinsonn
added a commit
that referenced
this pull request
Jun 11, 2026
…ption doc block (SEA-780) Co-Authored-By: seal <noreply@sealedsecurity.com>
Merge activity
|
mattwilkinsonn
added a commit
that referenced
this pull request
Jun 12, 2026
…ilter validators with xdg-dbus-proxy grammar, canonicalize grant dbus_filter, HashSet dedup (SEA-780)
mattwilkinsonn
added a commit
that referenced
this pull request
Jun 12, 2026
…ption doc block (SEA-780) Co-Authored-By: seal <noreply@sealedsecurity.com>
mattwilkinsonn
force-pushed
the
sea-780-dbus-proxy-slice-1--athena
branch
from
b5957be to
7aa0145
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 706-726: Extract the sort+dedup normalization of dbus_filter into
a small helper (e.g., fn normalize_dbus_filter(rules: impl
IntoIterator<Item=&Rule>) -> Vec<String>) and update the existing code that
builds dbus_filter to call that helper (use bundle.dbus_filter.iter().map(|r|
r.to_flag()) as input); add a focused unit test for normalize_dbus_filter that
verifies canonicalization (sorted and deduplicated) for a variety of input
orders/duplicates and exercises Rule::to_flag behavior so the canonicalization
contract is pinned before non-empty bundles are used.
In `@crates/seal-sandbox/src/compile.rs`:
- Around line 526-543: resolve_dbus_filter_rules duplicates the "widened
bundle-activation" logic that's implemented in apply_bundles, causing the D-Bus
rule set to drift when that predicate changes; extract the predicate into a
shared helper (e.g., bundle_applies_widened or bundle_matches_with_wrappers) and
update resolve_dbus_filter_rules and apply_bundles to call it instead of
re-implementing logic. The helper should accept the same inputs used here (a
&GrantToolEntry or entry.name, section_wrappers, matched_pattern) plus the
direnv_wrap_active and gh_credential_helper flags and return the boolean;
replace the inline checks (is_direnv_wrap_target /
is_gh_credential_helper_target and bundle_applies call) with a single call to
the new helper so both code paths stay consistent and add the two specific cases
(direnv_wrap_active and gh_credential_helper) inside that helper.
- Around line 1029-1043: The Linux branch currently hardcodes tail =
"~/.cache/sccache" which can miss sccache's actual cache location when
SCCACHE_DIR or XDG_CACHE_HOME is used; update the resolution before calling
bundle_path_to_bind so that for Platform::Linux you first check
std::env::var("SCCACHE_DIR") and use that if present, otherwise use
XDG_CACHE_HOME (or fall back to ~/.cache) and append "sccache" (matching
sccache's ProjectDirs logic), then pass the resulting path to
bundle_path_to_bind and push_unique into fs_write.paths (same call-site where
tail was used), ensuring the resolved path mirrors sccache cache-dir resolution
instead of the hardcoded "~/.cache/sccache".
🪄 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: efa49fab-a2ba-4b4c-9ca4-a9c7c4801473
📒 Files selected for processing (14)
crates/seal-policy/src/grant.rscrates/seal-policy/src/manifest.rscrates/seal-policy/src/manifest/dbus_filter.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/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.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: 3
🤖 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 706-726: Extract the sort+dedup normalization of dbus_filter into
a small helper (e.g., fn normalize_dbus_filter(rules: impl
IntoIterator<Item=&Rule>) -> Vec<String>) and update the existing code that
builds dbus_filter to call that helper (use bundle.dbus_filter.iter().map(|r|
r.to_flag()) as input); add a focused unit test for normalize_dbus_filter that
verifies canonicalization (sorted and deduplicated) for a variety of input
orders/duplicates and exercises Rule::to_flag behavior so the canonicalization
contract is pinned before non-empty bundles are used.
In `@crates/seal-sandbox/src/compile.rs`:
- Around line 526-543: resolve_dbus_filter_rules duplicates the "widened
bundle-activation" logic that's implemented in apply_bundles, causing the D-Bus
rule set to drift when that predicate changes; extract the predicate into a
shared helper (e.g., bundle_applies_widened or bundle_matches_with_wrappers) and
update resolve_dbus_filter_rules and apply_bundles to call it instead of
re-implementing logic. The helper should accept the same inputs used here (a
&GrantToolEntry or entry.name, section_wrappers, matched_pattern) plus the
direnv_wrap_active and gh_credential_helper flags and return the boolean;
replace the inline checks (is_direnv_wrap_target /
is_gh_credential_helper_target and bundle_applies call) with a single call to
the new helper so both code paths stay consistent and add the two specific cases
(direnv_wrap_active and gh_credential_helper) inside that helper.
- Around line 1029-1043: The Linux branch currently hardcodes tail =
"~/.cache/sccache" which can miss sccache's actual cache location when
SCCACHE_DIR or XDG_CACHE_HOME is used; update the resolution before calling
bundle_path_to_bind so that for Platform::Linux you first check
std::env::var("SCCACHE_DIR") and use that if present, otherwise use
XDG_CACHE_HOME (or fall back to ~/.cache) and append "sccache" (matching
sccache's ProjectDirs logic), then pass the resulting path to
bundle_path_to_bind and push_unique into fs_write.paths (same call-site where
tail was used), ensuring the resolved path mirrors sccache cache-dir resolution
instead of the hardcoded "~/.cache/sccache".
🪄 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: efa49fab-a2ba-4b4c-9ca4-a9c7c4801473
📒 Files selected for processing (14)
crates/seal-policy/src/grant.rscrates/seal-policy/src/manifest.rscrates/seal-policy/src/manifest/dbus_filter.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/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.rs
🛑 Comments failed to post (3)
crates/seal-policy/src/grant.rs (1)
706-726: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Extract and pin
dbus_filternormalization with a focused unit test.This path now feeds the signed grant payload, but in this slice it is only exercised through the all-empty catalog. Pulling the sort+dedup into a tiny helper and adding a direct test would lock in the canonicalization contract before slice 3 starts shipping non-empty rules.
🤖 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/grant.rs` around lines 706 - 726, Extract the sort+dedup normalization of dbus_filter into a small helper (e.g., fn normalize_dbus_filter(rules: impl IntoIterator<Item=&Rule>) -> Vec<String>) and update the existing code that builds dbus_filter to call that helper (use bundle.dbus_filter.iter().map(|r| r.to_flag()) as input); add a focused unit test for normalize_dbus_filter that verifies canonicalization (sorted and deduplicated) for a variety of input orders/duplicates and exercises Rule::to_flag behavior so the canonicalization contract is pinned before non-empty bundles are used.crates/seal-sandbox/src/compile.rs (2)
526-543: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Extract the widened bundle-activation predicate.
resolve_dbus_filter_rules()now re-implements the same direnv-wrap / gh-helper widening thatapply_bundles()owns, but the new tests only pin direct-prefix applicability. Pulling this into one shared helper and adding onedirenv_wrap_activecase plus onegh_credential_helpercase would keep the D-Bus surface from drifting away from bind/env/network behavior on the next widening tweak.Suggested shape
+fn bundle_applies_with_widenings( + entry: &GrantToolEntry, + matched_pattern: &str, + section_wrappers: &[String], + direnv_wrap_active: bool, + gh_credential_helper: bool, +) -> bool { + let is_direnv_wrap_target = direnv_wrap_active && entry.name == "direnv"; + let is_gh_credential_helper_target = gh_credential_helper && entry.name == "gh"; + is_direnv_wrap_target + || is_gh_credential_helper_target + || bundle_applies(entry, section_wrappers, matched_pattern) +} + fn resolve_dbus_filter_rules( entries: &[GrantToolEntry], matched_pattern: &str, section_wrappers: &[String], direnv_wrap_active: bool, gh_credential_helper: bool, ) -> Vec<String> { @@ let mut rules: Vec<String> = Vec::new(); for entry in entries { - let is_direnv_wrap_target = direnv_wrap_active && entry.name == "direnv"; - let is_gh_credential_helper_target = gh_credential_helper && entry.name == "gh"; - if !is_direnv_wrap_target - && !is_gh_credential_helper_target - && !bundle_applies(entry, section_wrappers, matched_pattern) - { + if !bundle_applies_with_widenings( + entry, + matched_pattern, + section_wrappers, + direnv_wrap_active, + gh_credential_helper, + ) { continue; }Also applies to: 6470-6668
🤖 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 526 - 543, resolve_dbus_filter_rules duplicates the "widened bundle-activation" logic that's implemented in apply_bundles, causing the D-Bus rule set to drift when that predicate changes; extract the predicate into a shared helper (e.g., bundle_applies_widened or bundle_matches_with_wrappers) and update resolve_dbus_filter_rules and apply_bundles to call it instead of re-implementing logic. The helper should accept the same inputs used here (a &GrantToolEntry or entry.name, section_wrappers, matched_pattern) plus the direnv_wrap_active and gh_credential_helper flags and return the boolean; replace the inline checks (is_direnv_wrap_target / is_gh_credential_helper_target and bundle_applies call) with a single call to the new helper so both code paths stay consistent and add the two specific cases (direnv_wrap_active and gh_credential_helper) inside that helper.
1029-1043:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🌐 Web query:
For current sccache releases, what cache directory does sccache use on Linux by default, and does it honor XDG_CACHE_HOME?💡 Result:
On Linux, sccache uses ~/.cache/sccache as its default directory for local disk storage [1][2][3]. Regarding XDG_CACHE_HOME, sccache's default implementation for determining the cache directory utilizes the directories crate (ProjectDirs) [4]. While sccache explicitly defaults to ~/.cache/sccache on Linux [1][2], it generally relies on the standard behaviors provided by the underlying Rust libraries to resolve platform-specific cache directories [4]. You can override this location entirely by setting the SCCACHE_DIR environment variable to a path of your choosing [1][2][5].
Citations:
- 1: https://github.com/mozilla/sccache/blob/733688fe/docs/Local.md
- 2: https://android.googlesource.com/toolchain/sccache/+/39d4118c3a8fc2f430b747c0262ce4fe8c1b01f9/docs/Local.md
- 3: https://docs.rs/crate/sccache/0.2.5
- 4: https://github.com/mozilla/sccache/blob/d11e2e08/src/config.rs
- 5: https://android.googlesource.com/toolchain/sccache/+/refs/heads/main/docs/Configuration.md
Fix Linux sccache bind to follow sccache’s actual cache-dir resolution (XDG_CACHE_HOME / SCCACHE_DIR)
In
crates/seal-sandbox/src/compile.rs:1029-1043, the Linux branch hardcodes~/.cache/sccache. However, sccache determines its Linux disk cache directory via standard platform cache-dir logic (ProjectDirs) and may vary withXDG_CACHE_HOME; it can also be overridden entirely viaSCCACHE_DIR. If the sandbox sets either, binding~/.cache/sccachemay miss the real sccache cache dir and make the write bind ineffective.
Also applies to: 4091-4158🤖 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 1029 - 1043, The Linux branch currently hardcodes tail = "~/.cache/sccache" which can miss sccache's actual cache location when SCCACHE_DIR or XDG_CACHE_HOME is used; update the resolution before calling bundle_path_to_bind so that for Platform::Linux you first check std::env::var("SCCACHE_DIR") and use that if present, otherwise use XDG_CACHE_HOME (or fall back to ~/.cache) and append "sccache" (matching sccache's ProjectDirs logic), then pass the resulting path to bundle_path_to_bind and push_unique into fs_write.paths (same call-site where tail was used), ensuring the resolved path mirrors sccache cache-dir resolution instead of the hardcoded "~/.cache/sccache".
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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-runtime/src/scope/sandbox_spawn/macos.rs`:
- Around line 58-71: The current loop emitting "(allow file-read* (literal
...))" for each runtime.extra_ro_binds source only whitelists the leaf inode;
modify the loop over runtime.extra_ro_binds to first synthesize and emit "(allow
file-read-metadata (literal ...))" entries for each strict ancestor of the
source path (from the immediate parent up to the filesystem root or sandbox
baseline) using seal_sandbox::sbpl_string_literal(&ancestor.to_string_lossy()),
then emit the existing "(allow file-read* ...)" for the leaf; ensure ancestor
rules are pushed into profile before the leaf rule (you can mirror the
ancestor-synthesis logic from the compile layer if available) so sandbox-exec
can traverse to the whitelisted file.
In `@crates/seal-runtime/src/scope/sandbox_spawn/mod.rs`:
- Around line 454-459: The cleanup currently sets removed_socket = true whenever
self.sock_path was Some, even if std::fs::remove_file failed; change the logic
so you call std::fs::remove_file(p) and set removed_socket based on its Result
(true on Ok, false on Err), and log the actual error (including the error
message) when remove_file returns Err; reference the self.sock_path.take()
usage, the removed_socket binding, and the std::fs::remove_file call so you
locate and replace the current unconditional-true behavior with a Result match
that logs failures and only marks removal as true on success.
In `@crates/seal-sandbox/src/compile.rs`:
- Around line 526-554: The dbus filter selection in resolve_dbus_filter_rules
duplicates the direnv/gh/pattern checks already used elsewhere (notably in
apply_bundles); extract that predicate into a shared helper function (e.g.,
bundle_applies_with_wrappers or is_bundle_applicable) that accepts a
&GrantToolEntry, &str matched_pattern, &[String] section_wrappers,
direnv_wrap_active: bool, gh_credential_helper: bool and returns bool, then
replace the inline condition in resolve_dbus_filter_rules with a call to this
new helper and update apply_bundles() to call the same helper so all
bundle-applicability logic is centralized and stays synchronized.
In `@crates/seal-sandbox/src/macos.rs`:
- Around line 423-438: When expanding FIRMLINKS into sources, handle the case
where strip_prefix yields an empty remainder so the firmlink twin itself is
emitted: inside the loop over FIRMLINKS where you do if let Ok(rest) =
p.strip_prefix(short) { ... }, detect if rest is empty and push
Path::new(long).to_path_buf() (or Path::new(long).join(rest) only when rest is
non-empty) so the `/private/...` twin itself is included in sources; this
ensures the later ancestor-walk in ancestors includes metadata for exact roots
like `/tmp`, `/etc`, `/var`. Also add a unit test exercising an exact `/tmp`
grant (FsBaseline::None) to assert the `/private/tmp` metadata path is present.
🪄 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: 8628a4ac-5c7b-479f-b2ab-7096197603ba
📒 Files selected for processing (14)
crates/seal-policy/src/grant.rscrates/seal-policy/src/manifest.rscrates/seal-policy/src/manifest/dbus_filter.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/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.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: 4
🤖 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-runtime/src/scope/sandbox_spawn/macos.rs`:
- Around line 58-71: The current loop emitting "(allow file-read* (literal
...))" for each runtime.extra_ro_binds source only whitelists the leaf inode;
modify the loop over runtime.extra_ro_binds to first synthesize and emit "(allow
file-read-metadata (literal ...))" entries for each strict ancestor of the
source path (from the immediate parent up to the filesystem root or sandbox
baseline) using seal_sandbox::sbpl_string_literal(&ancestor.to_string_lossy()),
then emit the existing "(allow file-read* ...)" for the leaf; ensure ancestor
rules are pushed into profile before the leaf rule (you can mirror the
ancestor-synthesis logic from the compile layer if available) so sandbox-exec
can traverse to the whitelisted file.
In `@crates/seal-runtime/src/scope/sandbox_spawn/mod.rs`:
- Around line 454-459: The cleanup currently sets removed_socket = true whenever
self.sock_path was Some, even if std::fs::remove_file failed; change the logic
so you call std::fs::remove_file(p) and set removed_socket based on its Result
(true on Ok, false on Err), and log the actual error (including the error
message) when remove_file returns Err; reference the self.sock_path.take()
usage, the removed_socket binding, and the std::fs::remove_file call so you
locate and replace the current unconditional-true behavior with a Result match
that logs failures and only marks removal as true on success.
In `@crates/seal-sandbox/src/compile.rs`:
- Around line 526-554: The dbus filter selection in resolve_dbus_filter_rules
duplicates the direnv/gh/pattern checks already used elsewhere (notably in
apply_bundles); extract that predicate into a shared helper function (e.g.,
bundle_applies_with_wrappers or is_bundle_applicable) that accepts a
&GrantToolEntry, &str matched_pattern, &[String] section_wrappers,
direnv_wrap_active: bool, gh_credential_helper: bool and returns bool, then
replace the inline condition in resolve_dbus_filter_rules with a call to this
new helper and update apply_bundles() to call the same helper so all
bundle-applicability logic is centralized and stays synchronized.
In `@crates/seal-sandbox/src/macos.rs`:
- Around line 423-438: When expanding FIRMLINKS into sources, handle the case
where strip_prefix yields an empty remainder so the firmlink twin itself is
emitted: inside the loop over FIRMLINKS where you do if let Ok(rest) =
p.strip_prefix(short) { ... }, detect if rest is empty and push
Path::new(long).to_path_buf() (or Path::new(long).join(rest) only when rest is
non-empty) so the `/private/...` twin itself is included in sources; this
ensures the later ancestor-walk in ancestors includes metadata for exact roots
like `/tmp`, `/etc`, `/var`. Also add a unit test exercising an exact `/tmp`
grant (FsBaseline::None) to assert the `/private/tmp` metadata path is present.
🪄 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: 8628a4ac-5c7b-479f-b2ab-7096197603ba
📒 Files selected for processing (14)
crates/seal-policy/src/grant.rscrates/seal-policy/src/manifest.rscrates/seal-policy/src/manifest/dbus_filter.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/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.rs
🛑 Comments failed to post (4)
crates/seal-runtime/src/scope/sandbox_spawn/macos.rs (1)
58-71:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winGrant ancestor metadata for dispatcher artifact paths, not just the leaf file.
The new
(allow file-read* (literal ...))rule only opens the final artifact inode. For artifacts outside the baseline tree (the current CA cert case under the per-session security dir),sandbox-execstill needsfile-read-metadataon each strict ancestor to traverse to that leaf. Without those ancestor rules, TLS clients can still fail to open the injected cert even though the exact file is whitelisted.Please emit
file-read-metadata (literal ...)for the strict ancestors of eachruntime.extra_ro_bindssource path before the leaffile-read*rule, or reuse the same ancestor-synthesis logic the compile layer already uses for granted paths.🤖 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-runtime/src/scope/sandbox_spawn/macos.rs` around lines 58 - 71, The current loop emitting "(allow file-read* (literal ...))" for each runtime.extra_ro_binds source only whitelists the leaf inode; modify the loop over runtime.extra_ro_binds to first synthesize and emit "(allow file-read-metadata (literal ...))" entries for each strict ancestor of the source path (from the immediate parent up to the filesystem root or sandbox baseline) using seal_sandbox::sbpl_string_literal(&ancestor.to_string_lossy()), then emit the existing "(allow file-read* ...)" for the leaf; ensure ancestor rules are pushed into profile before the leaf rule (you can mirror the ancestor-synthesis logic from the compile layer if available) so sandbox-exec can traverse to the whitelisted file.crates/seal-runtime/src/scope/sandbox_spawn/mod.rs (1)
454-459:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLog the actual unlink result.
removed_socketis set totruewheneversock_pathis present, even ifremove_filefails or the file is already gone. That makes the cleanup log claim success when the socket artifact may still be on disk.Suggested fix
- let removed_socket = if let Some(p) = self.sock_path.take() { - let _ = std::fs::remove_file(p); - true + let removed_socket = if let Some(p) = self.sock_path.take() { + std::fs::remove_file(&p).is_ok() } else { false };📝 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.let removed_socket = if let Some(p) = self.sock_path.take() { std::fs::remove_file(&p).is_ok() } else { false };🤖 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-runtime/src/scope/sandbox_spawn/mod.rs` around lines 454 - 459, The cleanup currently sets removed_socket = true whenever self.sock_path was Some, even if std::fs::remove_file failed; change the logic so you call std::fs::remove_file(p) and set removed_socket based on its Result (true on Ok, false on Err), and log the actual error (including the error message) when remove_file returns Err; reference the self.sock_path.take() usage, the removed_socket binding, and the std::fs::remove_file call so you locate and replace the current unconditional-true behavior with a Result match that logs failures and only marks removal as true on success.crates/seal-sandbox/src/compile.rs (1)
526-554: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Extract the shared bundle-applicability predicate.
Lines 536-540 duplicate the same direnv/gh/pattern logic already used on Lines 1001-1005. That makes
dbus_filter_ruleseasy to desynchronize from the fs/env/network bundle surfaces the next time the widening rules change.♻️ Extraction sketch
+fn bundle_entry_applies( + entry: &GrantToolEntry, + matched_pattern: &str, + section_wrappers: &[String], + direnv_wrap_active: bool, + gh_credential_helper: bool, +) -> bool { + let is_direnv_wrap_target = direnv_wrap_active && entry.name == "direnv"; + let is_gh_credential_helper_target = gh_credential_helper && entry.name == "gh"; + is_direnv_wrap_target + || is_gh_credential_helper_target + || bundle_applies(entry, section_wrappers, matched_pattern) +} + fn resolve_dbus_filter_rules( entries: &[GrantToolEntry], matched_pattern: &str, section_wrappers: &[String], direnv_wrap_active: bool, gh_credential_helper: bool, ) -> Vec<String> { let mut seen: std::collections::HashSet<&str> = std::collections::HashSet::new(); let mut rules: Vec<String> = Vec::new(); for entry in entries { - let is_direnv_wrap_target = direnv_wrap_active && entry.name == "direnv"; - let is_gh_credential_helper_target = gh_credential_helper && entry.name == "gh"; - if !is_direnv_wrap_target - && !is_gh_credential_helper_target - && !bundle_applies(entry, section_wrappers, matched_pattern) - { + if !bundle_entry_applies( + entry, + matched_pattern, + section_wrappers, + direnv_wrap_active, + gh_credential_helper, + ) { continue; } for rule in &entry.dbus_filter { if seen.insert(rule.as_str()) { rules.push(rule.clone());Apply the same helper in
apply_bundles()so all bundle surfaces stay locked together.🤖 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 526 - 554, The dbus filter selection in resolve_dbus_filter_rules duplicates the direnv/gh/pattern checks already used elsewhere (notably in apply_bundles); extract that predicate into a shared helper function (e.g., bundle_applies_with_wrappers or is_bundle_applicable) that accepts a &GrantToolEntry, &str matched_pattern, &[String] section_wrappers, direnv_wrap_active: bool, gh_credential_helper: bool and returns bool, then replace the inline condition in resolve_dbus_filter_rules with a call to this new helper and update apply_bundles() to call the same helper so all bundle-applicability logic is centralized and stays synchronized.crates/seal-sandbox/src/macos.rs (1)
423-438:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winExact
/tmp//etc//vargrants still miss the/private/...metadata hop.The firmlink expansion only adds the
/private/...twin as asource, and the later walk emits ancestors of that twin, not the twin itself. That means a grant rooted exactly at/tmp,/etc, or/varstill won't get a metadata rule for/private/tmp,/private/etc, or/private/var, so canonicalization of the exact firmlink root can still fail underFsBaseline::None.Please special-case the empty
restcase so the/private/...twin itself is emitted as a metadata path, and add a test for an exact/tmpgrant.🤖 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/macos.rs` around lines 423 - 438, When expanding FIRMLINKS into sources, handle the case where strip_prefix yields an empty remainder so the firmlink twin itself is emitted: inside the loop over FIRMLINKS where you do if let Ok(rest) = p.strip_prefix(short) { ... }, detect if rest is empty and push Path::new(long).to_path_buf() (or Path::new(long).join(rest) only when rest is non-empty) so the `/private/...` twin itself is included in sources; this ensures the later ancestor-walk in ancestors includes metadata for exact roots like `/tmp`, `/etc`, `/var`. Also add a unit test exercising an exact `/tmp` grant (FsBaseline::None) to assert the `/private/tmp` metadata path is present.
…KernelParams::dbus_filter_rules — slice 1 of SEA-780 D-Bus / Secret Service proxy support
…ilter validators with xdg-dbus-proxy grammar, canonicalize grant dbus_filter, HashSet dedup (SEA-780)
…ption doc block (SEA-780) Co-Authored-By: seal <noreply@sealedsecurity.com>
mattwilkinsonn
force-pushed
the
sea-780-dbus-proxy-slice-1--athena
branch
from
7aa0145 to
0ec85d9
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 6530-6729: Add two unit tests that assert dbus_filter_rules follow
the resolver's indirect widenings: create synthetic bundle entries carrying
non-empty dbus_filter and use the same helper builders (e.g.,
cargo_entry_with_install / bun_entry_with_install and
sandbox_grant_with_resolved_tools_for_test) but craft the matched_pattern and/or
bundle setup so the resolver's gh_credential_helper and direnv_wrap_active
widenings are exercised; for each widening, compile(...) with the appropriate
matched_pattern that only becomes applicable via that widening and assert the
dbus_filter_rules either aggregate (when the widening makes the bundle apply) or
remain empty (when it doesn't), mirroring the existing tests
dbus_filter_rules_aggregated_when_bundle_applies and
dbus_filter_rules_skipped_when_bundle_does_not_apply. Ensure tests reference
gh_credential_helper and direnv_wrap_active in their names/comments so future
readers know which widening each test covers.
- Around line 535-541: The applicability predicate used in the entries loop (the
is_direnv_wrap_target / is_gh_credential_helper_target checks combined with
bundle_applies(entry, section_wrappers, matched_pattern)) should be extracted
into a single helper function (e.g., fn bundle_applicable(entry: &Entry,
section_wrappers: &..., matched_pattern: &..., direnv_wrap_active: bool,
gh_credential_helper: bool) -> bool) and both callsites (the current loop using
is_direnv_wrap_target/is_gh_credential_helper_target and the other duplicate at
the later block) should be updated to use this helper to avoid drift; update the
loop to call the new helper and remove the duplicated inline logic, preserving
existing parameters (entry, section_wrappers, matched_pattern,
direnv_wrap_active, gh_credential_helper).
🪄 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: 33c36ec3-69c2-4e13-80a9-295db6530aa1
📒 Files selected for processing (14)
crates/seal-policy/src/grant.rscrates/seal-policy/src/manifest.rscrates/seal-policy/src/manifest/dbus_filter.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/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.rs
…ggregation through gh-credential-helper and direnv-wrap widenings (SEA-780) Co-Authored-By: seal <noreply@sealedsecurity.com>
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
Introduces typed D-Bus session-bus filter rules (
DbusFilterRule,BusName,InterfaceMatch,PathMatch) for the curated bundle catalog, wires them throughBundleSpec→GrantToolEntry→KernelParamsas renderedxdg-dbus-proxyCLI flag strings, and adds the compile-time resolver that aggregates, dedupes, and sorts the rules across all bundles applicable to a spawn. This is SEA-780 slice 1 — the typed shape and audit-chain plumbing that later slices build the dispatcher integration and per-bundle defaults on top of.Related issues
Refs SEA-780, SEA-571, SEA-769
Changes
crates/seal-policy/src/manifest/dbus_filter.rswithDbusFilterRule(Talk,See,Callvariants),BusName,InterfaceMatch, andPathMatchnewtypes. Each hasnew_uncheckedforconst-constructible catalog entries andTryFrom<String>/TryFrom<&str>for future user-bundle deserialization.to_flag()renders each rule into its exactxdg-dbus-proxyCLI flag form.dbus_filter: &'static [DbusFilterRule]toBundleSpec(all existing bundles default to&[]).GrantToolEntry::dbus_filter(elided from canonical JSON when empty, so existing grants don't re-sign).dbus_filter_rules: Vec<String>toKernelParams(elided from canonical JSON when empty). Changing the rule set flips the audit hash, identical to thedomains/psl_exemptionsclosure.resolve_dbus_filter_rulesinseal-sandbox/src/compile.rsthat mirrorsapply_bundles's applicability predicate — including the SEA-571/SEA-769 direnv-wrap and gh credential-helper widenings — then dedupes and sorts the collected flag strings.every_bundle_dbus_filter_rule_validatescatalog test that walks everyBundleSpec::dbus_filterentry and callsrule.validate(), catching typos thatnew_uncheckedskips at construction time.Test plan
dbus_filter.rscoverBusName,InterfaceMatch, andPathMatchvalidation (well-formed, empty, invalid chars, length cap, leading-hyphen, trailing-slash, empty-segment, round-tripTryFrom), andDbusFilterRule::to_flagrendering for all three variants including wildcard members.dbus_filter_ruleschanges.every_bundle_dbus_filter_rule_validatescatalog test guards against malformed rule strings in the staticBUNDLEStable.dbus_filter_rules: Vec::new()to keep the struct exhaustive.Notes for reviewers
All bundles carry
dbus_filter: &[]in this slice — no behavioral change to any existing spawn. Thegh_dbus_disabledbypass (SEA-571/SEA-769) is untouched here; slice 4 retires it once the proxy is the only path. The rendered-string-rather-than-typed-enum choice inKernelParamsandGrantToolEntryis intentional: it decouples audit-chain stability from futureDbusFilterRulevariant additions and lets the dispatcher (slice 2) push strings directly onto the proxy argv without re-parsing.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.