test(seatbelt): e2e guard for un-resolved $TMPDIR read-write grants

[Carlos Alexandro Becker (caarlos0)]

📖 Description

Scope changed substantially. This PR started as a fix for macOS temp-path grants. While it was in review, upstream main landed an independent — and more complete — fix for the same root cause. The builder change here has therefore been dropped in favor of upstream's, and all that remains is the end-to-end test, which upstream does not have.

Background

On macOS, Seatbelt evaluates subpath filters against the resolved path the kernel sees. /etc, /tmp, and /var are symlinks into /private, so a rule emitted as (subpath "/var/folders/…") matches nothing at all — the access is checked as /private/var/folders/….

That silently voided every caller-supplied policy path under those roots: the grant was emitted, the profile loaded fine, and the access was still denied. It bites hardest on $TMPDIR, whose ordinary spelling is the per-user _CS_DARWIN_USER_TEMP_DIR (/var/folders/<a>/<b>/T/).

Verified directly against sandbox-exec while investigating review feedback: with an otherwise identical profile, a grant spelled /var/folders/<container>/C denies the write, while /private/var/folders/<container>/C allows it. The same holds for /tmp vs /private/tmp.

Upstream main now handles this in resolve_policy_path / resolve_macos_root_symlinks, together with lexical normalization, .. rejection, /home, and denied > readonly > readwrite precedence. That implementation is a superset of what this branch had, so this branch now simply takes it.

What is left in this PR

A single macOS-gated e2e test, seatbelt_honors_uncanonicalized_var_readwrite_path, in src/testing/wxc_e2e_tests/tests/e2e_seatbelt_characterization.rs.

It passes the host $TMPDIR (via std::env::temp_dir(), so no machine-specific container id) as the sole readwritePaths entry without canonicalizing it, then runs a sandboxed command that writes there and asserts both a zero exit and that the file exists.

It is worth keeping on top of upstream's unit tests because it closes a gap none of them cover:

• Every other filesystem test in that suite calls fs::canonicalize on its temp dir first, which resolves the symlink itself. So none of them ever exercises the un-resolved /var spelling that $TMPDIR actually has, and none would catch a regression in the resolution.
• The builder's own unit tests assert on the generated profile text, so they can show the right rule was emitted but not that the kernel honors it.
• It also covers profile loadability: if the rewritten rules were malformed, the sandbox would fail to launch rather than fail to write, and the test distinguishes the two.

It early-returns when env::temp_dir() is not the /var spelling, so it is a clean no-op rather than a false pass on a host where the premise does not hold.

🔗 References

No linked issue. Supersedes this branch's earlier builder change; the fix itself now lives in upstream main.

🔍 Validation

On a macOS host (aarch64-apple-darwin), run from src/, against the merged tree:

• cargo test -p seatbelt_common — 63 passed (upstream's suite, unmodified).
• cargo test -p wxc_e2e_tests --test e2e_seatbelt_characterization — 9 passed, including the new test.
• cargo fmt --all -- --check — clean.
• cargo clippy -p wxc_e2e_tests --all-targets -- -D warnings — clean.

✅ Checklist

• Signed the Contributor License Agreement
• Linked to an issue
• Updated documentation (if applicable)
• Updated Copilot instructions (if build, architecture, or conventions changed)
• If this PR changes Cargo.lock, the dependency-feed-check check passes (no dependency change)

📋 Issue Type

• Bug fix
• Feature
• Task (test coverage)

Microsoft Reviewers: Open in CodeFlow

[Copilot]

Pull request overview

This PR adjusts the macOS Seatbelt profile generator to broaden certain readwritePaths entries that target per-user Darwin temp/cache leaves under /var/folders/<a>/<b>/{T,C,0} so the enclosing per-user container is granted instead, preventing cache-related write denials in sandboxed tools. The change is implemented in the profile builder so it benefits all macOS backend consumers (CLI/SDKs/FFI) without duplicating Darwin path knowledge.

Changes:

• Widen readwritePaths grants for /var/folders/<a>/<b>/{T,C,0} (and /private/...) to /var/folders/<a>/<b> in the Seatbelt profile builder.
• Add unit tests validating widening behavior and non-matching cases.
• Document the widening behavior in the Seatbelt backend docs.

Show a summary per file

File	Description
src/backends/seatbelt/common/src/profile_builder.rs	Adds Darwin temp/cache leaf widening logic in write_filesystem_allow plus new helper + tests.
docs/macos-support/seatbelt-backend.md	Documents the readwritePaths widening behavior and its safety constraints.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Low

[Carlos Alexandro Becker (caarlos0)]

Huzaifa Danish (@huzaifa-d) thank you, resolved.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[Gudge (MGudgin)]

Adversarial review — 10 axes

Ran a multi-axis adversarial review of this PR (security, reliability, performance, maintainability, correctness, testability, test coverage, documentation-drift, cross-platform-parity, threat-model), each axis in an isolated context on a non-Claude model, then consolidated. Findings: 6 High, 6 Medium, 3 Low. Correctness and performance both came back clean, with documented traces.

Credit where it's due: the underlying complaint is real — granting $TMPDIR and having the sibling cache stay unwritable is a genuine usability bug — and darwin_temp_sibling_grants is the strongest part of this PR. A dedicated correctness pass traced it character by character (doubled slashes in every position, multiple trailing slashes, /private boundary handling, "/" / "/private" / "", ./.. in both segments) and found no parser defects. The guard genuinely fails closed.

My concerns are about the approach, not the parsing.

1. The serializer silently rewrites the caller's security policy

write_filesystem_allow's job is to render a policy; this makes it also mutate one. Three consequences, all found independently:

• A sibling explicitly listed in readonlyPaths comes out read-write (found by cross-platform-parity and security separately). The RO block is emitted at lines 153-161, the expanded RW block at 163-180, and Seatbelt is last-match-wins — so readonlyPaths: [".../C"] + readwritePaths: [".../T"] grants write to C. deniedPaths can't fix it, because it denies reads too. This is a straightforward privilege bug and it's my main blocker.
• The caller can't see or disable the expansion. The deniedPaths escape hatch assumes the caller knows about a transformation that has no flag, no schema field, no log line, and no API surface.
• Every SDK inherits it silently — Node, C#/FFI, Rust — with no version signal.

2. It may not work where it matters

Every other hardcoded macOS path in this file is /private-anchored (lines 120-122, 329, 445-446, 456) because Seatbelt matches resolved paths and /var is a symlink to /private/var. The new grants instead echo whichever spelling the caller passed. For the common un-prefixed $TMPDIR form, the emitted C/0 rules plausibly match nothing at runtime — i.e. the feature silently no-ops. Three axes converged on this (reliability, maintainability, security-via-deny-forms).

Nothing would catch it: all four tests assert on substrings of a generated string, and nothing anywhere feeds a profile to sandbox_init.

---

Findings that are out of diff range (no inline anchor)

Medium (security) — the deniedPaths carve-out is path-form sensitive. write_filesystem_deny emits denies verbatim with no canonicalization, so a deny written /private/var/folders/ab/cd/C will not override an expanded allow written /var/folders/ab/cd/C, or vice versa. The new deny test only exercises matching forms. If deniedPaths is the documented escape hatch for this expansion, it needs to work across both spellings.

High (documentation-drift) — the widening isn't discoverable where policy authors look. readwritePaths is a cross-backend schema field, but this is documented only in the macOS backend deep-dive. Unchanged and now incomplete: docs/schema.md:113-119, the wire-model source src/core/wxc_common/src/wire.rs:253-259 (and therefore the generated schemas/dev/), sdk/node/src/types.ts JSDoc, sdk/dotnet/README.md, and the Rust SDK docs. Someone writing policy JSON or calling spawnSandbox has no way to learn their $TMPDIR grant became three grants.

Medium (documentation-drift) — no version or release signal. Sandbox-policy semantics changed with no schema-version bump, no entry in sdk/node/CHANGELOG.md, and no release note, while docs/sandbox-policy/v1/policy.md:75-80 tells consumers a version pins behavior. There's no way to tell which release started widening permissions.

Medium (test coverage) — the readonlyPaths non-expansion isn't locked in. The existing readonly test uses unrelated paths, so nothing would catch sibling expansion being applied there later.

---

What I'd want before approving

1. Make it opt-in — a policy flag, or require the caller to list the siblings. Silent widening of a security boundary is the core objection.
2. Never widen a sibling the caller named in readonlyPaths or deniedPaths — resolve intents before emission with denied > readonly > readwrite.
3. Normalize to /private/var/folders/... to match this file's existing convention.
4. One macOS sandbox-exec test granting only the real host .../T and writing under .../C. That single test covers both the spelling risk and profile loadability.

If (1) is unappealing, I'd settle for the expansion staying unconditional but strictly subordinate to explicit caller intent, plus the docs moving up to the shared readwritePaths contract. Happy to discuss — and if I've misread the Seatbelt path-resolution semantics in (3), please push back, that one is the least certain of the set.

[Gudge (MGudgin)]

Correction to my review — two findings re-attributed

I ran an attribution pass over my own review, checking every finding that cited code outside this PR's diff against git show f379b0e2:<path>. Two of them don't hold up as written, and since they're on a review that requested changes, I want to correct them publicly rather than leave you chasing them.

1. Withdrawn — "denied-path carve-outs are path-form sensitive" (was Medium, security)

I claimed the deniedPaths escape hatch is unreliable because denies and allows can use different /var vs /private/var spellings.

That is pre-existing and not introduced by this PR:

• write_filesystem_deny is byte-identical between f379b0e2 and this branch — it has always emitted deniedPaths verbatim after expand_tilde, with no canonicalization.
• The allow side was also always verbatim, so a spelling mismatch between readwritePaths and deniedPaths was equally possible before this change.
• Your expansion propagates the caller's own prefix into the derived siblings, so a deny written in the same spelling the caller used still matches.

So this PR neither introduces nor worsens it. Please disregard that finding. It's a reasonable repo-level issue but it isn't yours, and it shouldn't be a condition on this merge.

2. Rescoped and downgraded — "/var grants may not match Seatbelt's resolved paths" (was High, reliability)

My inline comment on profile_builder.rs:540 framed the un-normalized /var/folders/... spelling as an inconsistency this PR introduces. That framing was wrong in an important way, and I checked it properly this time:

• There is no canonicalization anywhere — not in config_parser.rs, not in profile_builder.rs, in either revision. Caller-supplied readwritePaths have always been emitted exactly as written.
• Which means the pre-existing $TMPDIR (.../T) grant already had precisely the same exposure. If /var/... rules genuinely never matched, that grant would have been broken before this PR too.

That last point cuts against my own finding: since $TMPDIR grants apparently do work in practice, either callers are passing the /private-anchored form, or Seatbelt resolves the symlink when matching subpath. I don't have a macOS host to settle it, and I should have said so instead of asserting the failure mode.

What I think does still stand, at Medium: for the derived C and 0 siblings, the code is choosing a spelling for paths the caller never wrote. That is a new decision point, and it's the natural place to normalize — every other macOS path in this file is /private-anchored (lines 120-122, 329, 445-446, 456), and your own doc comment at line 517 calls /private "the post-canonicalization form" while nothing performs that canonicalization.

So the ask shrinks to: normalize the derived siblings to /private/var/folders/... (cheap, matches the file's convention, removes the question entirely), rather than "this feature may be a no-op". If you already know from testing on a real host which spelling matches, that answer is more valuable than my speculation — I'd take it and drop the point.

Unchanged

The rest of the review stands, and I re-confirmed the load-bearing one directly in the source:

• The read-only sibling upgrade is real and is introduced here. readonly_paths emits at profile_builder.rs:153-161, the expanded readwrite_paths block at 163-180, and Seatbelt is last-match-wins — so readonlyPaths: [".../C"] + readwritePaths: [".../T"] makes C writable. Before this PR the RW block only ever contained the caller's own paths, so C stayed read-only. deniedPaths can't restore it because it denies reads too. That remains my main blocker.
• The silent widening into .../C and .../0 with no opt-out, the docs/schema/SDK disclosure gap, the missing version signal, and the test-coverage points are all about code and behavior added by this PR.

Apologies for the noise on those two — better to hand you a shorter accurate list than a longer one.

[Copilot]

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

Review details

Suppressed comments (2)

src/testing/wxc_e2e_tests/tests/e2e_seatbelt_characterization.rs:272

• The current PR diff adds only this E2E test, while the normalization implementation and documentation are already present on the base branch. The title/body therefore inaccurately claim that this PR implements the rewrite and updates the documentation. Please update the PR metadata to describe this as regression coverage, or include the implementation changes it claims to deliver.

/// End-to-end guard for the macOS root-symlink resolution in
/// `seatbelt_common::profile_builder` (`resolve_macos_root_symlinks`).

src/testing/wxc_e2e_tests/tests/e2e_seatbelt_characterization.rs:303

• This returns success without exercising the regression whenever TMPDIR is overridden to /tmp, /private/..., or another location, so the new E2E guard can silently become a no-op in CI. Use a guaranteed unresolved macOS alias such as writable /var/tmp (with the existing unique child directory) instead of conditionally skipping.

    let temp_root = std::env::temp_dir();
    if !temp_root.starts_with("/var") {
        return;
    }

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Gudge (MGudgin)]

Approving — and correcting my earlier correction

The PR I reviewed no longer exists in any meaningful sense: the sibling-expansion approach is gone, and what remains is a single 66-line e2e guard. That is the right outcome, and the underlying bug turned out to be both deeper and more general than what the original change was reaching for.

For the record, tracing where everything went:

My finding	Disposition
High — expanded read-write grant silently upgrades a sibling the caller listed in readonlyPaths	Moot, and fixed more generally. Sibling expansion is gone; ResolvedPaths::from_policy in main now re-applies deny > readonly > readwrite after path resolution, so two spellings of the same path can no longer let the weaker grant win
High — /var grants may not match Seatbelt's resolved /private/var	Confirmed and fixed in main (resolve_macos_root_symlinks, #749)
High — no test proves the profile loads or that the write actually succeeds	This PR
Silent widening into .../C / .../0 with no opt-out; docs/schema/SDK disclosure; missing version signal; per-user container validation; parser edge cases	Moot — no policy rewriting remains

I got one of my own corrections wrong

In my correction comment I downgraded the /var vs /private/var finding from High to Medium and wrote that the "feature may be a no-op" claim was "overstated without a macOS host to confirm it." That was wrong, and the reasoning that led me there is worth naming.

I applied a counter-evidence test: if un-resolved /var grants really never matched, the pre-existing $TMPDIR grant would already be broken — and since it evidently works, my claim must be too strong. Main's own doc comment now settles it the other way:

a rule written against the unresolved path is dead: (subpath "/tmp/work") never matches, because the kernel only ever sees /private/tmp/work. That silently voided every policy path under these roots — including the automatic $TMPDIR grant, which resolves to /var/folders/... on macOS.

So the $TMPDIR grant was broken. My test assumed a visible symptom would exist if the bug were real — but the visible symptom was sitting in this PR's own description ("tools that stage under the per-user cache were denied"). I read that as evidence for the sibling theory instead of evidence for the resolution bug, and then used its apparent absence to argue myself out of the correct finding. The original High stood; my downgrade did not.

Which also reframes the original diagnosis: the sibling expansion was treating a symptom. The cache directory was not unwritable because siblings went ungranted — it was unwritable because no /var/... grant matched at all. Landing the resolution fix upstream and reducing this PR to a guard is a strictly better outcome than the change I first reviewed.

On the test itself

This is the test I asked for, and slightly better. It uses std::env::temp_dir() so there is no machine-specific container id, it asserts both that the profile loads and that the write succeeds, and the doc comment calls out the reason this class of bug survived so long:

It deliberately does not canonicalize the path. Every other filesystem test in this file calls fs::canonicalize first, which resolves the symlink itself — so none of them would notice this class of bug.

That last point is the valuable part. A test suite that canonicalizes its inputs cannot see a resolution bug, and stating that inline stops someone "tidying" it later. The early return when temp_dir() is not under /var is the right call too — it keeps the test honest on hosts where the spelling would not exercise the regression.

Not verified locally: the file is #![cfg(target_os = "macos")] and I am on Windows, so I read it rather than ran it.

Thanks for taking the redesign rather than patching the original approach.