Skip to content

fix(runtime): resolve macOS sandbox temp roots to canonical paths - #4234

Merged
Astro-Han merged 1 commit into
apache:mainfrom
jsiu93:fix/seatbelt-canonical-temp-roots
Aug 30, 2026
Merged

fix(runtime): resolve macOS sandbox temp roots to canonical paths#4234
Astro-Han merged 1 commit into
apache:mainfrom
jsiu93:fix/seatbelt-canonical-temp-roots

Conversation

@jsiu93

@jsiu93 jsiu93 commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Resolve every macOS Seatbelt root to its canonical filesystem path before emitting policy definitions, failing policy construction when a root cannot be canonicalized. Policy generation is the narrowest correct layer because Seatbelt matches kernel-resolved paths; Linux and Windows backends retain their current behavior. Denied roots and protected writable roots now share the same canonical representation. Roots that do not exist yet are canonicalized through their deepest existing ancestor with the missing tail re-appended (same semantics as realpathAllowMissing), so an allow and its deny never split across path aliases.

Adds policy coverage for a real symlinked temp root and a sandbox-exec smoke test that runs mktemp under $TMPDIR and touch under /tmp from an uncanonicalized workspace path.

Fixes #4233

Verification

mise exec node@24.19.0 -- node --test <macos-seatbelt unit and smoke tests>
Before fix: 18 pass, 3 fail, exit 1
After fix: 21 pass, 0 fail, exit 0
After review fixups (missing-leaf canonicalization + fail-closed on EACCES/ELOOP, rebased on main with #4237): 25 pass, 0 fail, exit 0

The failing run reproduced both lexical /tmp definitions and temp writes returning Operation not permitted.

npm --workspace @maka/runtime run build: exit 0
npm run format:check: exit 0
npm run lint: exit 0
npm run typecheck: exit 0

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: pi (gpt-5.6-sol), human-reviewed

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/S Under 100 readable lines label Aug 30, 2026

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact commit 34db9e6754148823f4cd52181eed65bbb28bb692.

The change canonicalizes explicit, special, protected-workspace, runtime-readable, and executable Seatbelt roots, and adds unit plus macOS smoke coverage for /tmp and $TMPDIR. I found one security-relevant P1 inline.

Validation: npm ci; full npm run build:test; 14/14 affected Seatbelt unit tests passed; 13/13 realpathAllowMissing tests passed; git diff --check passed. The macOS-only smoke suite was skipped on this Linux host, so I could not independently execute sandbox-exec. The hosted test, label, windows_recovery, and windows_sandbox_w0_protocol checks are green; package was still running at the final pre-publication check. The branch is based directly on current main d2346707.

Codex-assisted review performed under the maintainer-approved review workflow.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

try {
return realpathSync(path);
} catch {
return path;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Canonicalize missing descendants through their existing ancestor

When the leaf does not exist, this fallback leaves it in the caller’s lexical path space. After this patch that can split an allow and its deny across aliases: on macOS a custom managed profile that writes :slash_tmp but denies an exact, not-yet-created /tmp/blocked.txt gets a writable root of /private/tmp, while the deny remains the literal /tmp/blocked.txt. Seatbelt evaluates the write at the canonical /private/tmp/blocked.txt, so the require-not no longer excludes it and an explicitly denied file can be created. I reproduced the generated mismatch with a symlinked temp root and missing denied leaf. The existing realpathAllowMissing helper already canonicalizes the deepest existing ancestor; please use equivalent semantics here and cover the missing-leaf deny case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that split is real and I reproduced it the same way (symlinked temp root + a denied leaf that does not exist yet).

Updated in the amended commit: resolveRootPath now walks up to the deepest existing ancestor, canonicalizes it, and re-appends the missing tail (mirroring realpathAllowMissing, including the dangling-symlink hop cap), so the writable root and the require-not deny for a not-yet-created file land in the same path space. The lexical path is kept only when even / cannot be resolved. Added a unit test that builds a managed profile with a symlinked write root and an exact deny on a missing leaf, and asserts the deny literal is emitted under the canonical root and the lexical alias no longer appears. The pre-existing /tmp/repo.(test)+[x] escaping test now expects the canonical /tmp ancestor.

Local run after npm ci on current main: 22/22 seatbelt unit + macOS smoke tests, format:check, lint, typecheck, full build all exit 0.

@jsiu93
jsiu93 force-pushed the fix/seatbelt-canonical-temp-roots branch from 34db9e6 to 121a89b Compare August 30, 2026 02:49

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed exact head 121a89b932137dcb219a1d7c0a4de64baefb757c and found no remaining blocking issue.

The previous missing-leaf deny bypass is closed: Seatbelt roots now canonicalize through the deepest existing ancestor, keeping writable roots and exact denies in the same path space. On macOS, the old head allowed creation of an explicitly denied /tmp/... file, while this head rejects the same sandbox-exec probe with Operation not permitted. The full Runtime suite and all 22 focused macOS Seatbelt tests pass.


Automated review notice: This comment was posted by an automated review agent operated by WAWQAQ. It is not an independent human review and does not replace one.

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact commit 121a89b932137dcb219a1d7c0a4de64baefb757c.

The original missing-leaf alias split is fixed and covered, but one P1 remains inline: non-missing filesystem errors still fall back to the lexical path and can recreate the same allow/deny namespace split.

Validation: clean dependency install; full npm run build:test; 15/15 affected Seatbelt tests passed; 13/13 path-containment tests passed; git diff --check passed. All hosted checks are green and the head is directly based on current main d2346707. The macOS-only sandbox-exec smoke suite was skipped on this Linux host.

Codex-assisted review performed under the maintainer-approved review workflow.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

try {
return resolve(realpathSync(cursor), ...missing.reverse());
} catch (error) {
if (!isMissingPathError(error)) return path;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Fail closed on non-missing canonicalization errors

This branch contradicts the comment above it: any EACCES, ELOOP, or other non-missing error immediately returns the original lexical path, rather than reserving that fallback for an unresolvable filesystem root. That can recreate the deny bypass under review. I reproduced it with a symlinked writable root containing a mode-000 directory: the allow is emitted for the canonical target, while an exact deny below the inaccessible directory remains under the symlink alias. Because the child runs as the same owner, it can first restore the directory mode and then create the canonical denied file, which no longer matches the lexical require-not. The existing realpathAllowMissing contract deliberately propagates non-missing errors for this reason. Please fail policy construction on these errors (or otherwise preserve a single canonical namespace) and add an EACCES regression.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — the early return on non-missing errors contradicted the stated contract. resolveRootPath now propagates any non-ENOENT/ENOTDIR failure (and throws on the dangling-symlink hop cap), so policy construction fails instead of emitting a root in lexical path space; the only fallback left is re-appending genuinely missing tail components under their canonicalized existing ancestor. Added the EACCES regression you described: a symlinked writable root with a mode-000 directory and an exact deny beneath it now makes buildSeatbeltPolicy throw (skipped when running as root).

Also rebased onto current main to pick up #4237, so the branch now carries both seatbelt suites together: 25/25 unit + macOS smoke tests, full build, format:check, lint, typecheck all exit 0 after npm ci.

@github-actions github-actions Bot added effort/M Under 500 readable lines and removed effort/S Under 100 readable lines labels Aug 30, 2026
Auto sandbox mode denied reads and writes beneath $TMPDIR and /tmp because packages/runtime/src/sandbox/macos-seatbelt.ts:380 emitted lexical roots while Seatbelt evaluated canonical /private paths.

Resolve every macOS policy root before classification so readable, writable, denied, and protected-root comparisons stay aligned. Roots that do not exist yet (for example a denied file that has not been created) are canonicalized through their deepest existing ancestor with the missing tail re-appended, mirroring realpathAllowMissing, so an allow and its deny never split across path aliases. Any other resolution failure (EACCES, ELOOP, ...) propagates and fails policy construction instead of emitting a root in lexical path space.

Verified with failing-first policy and sandbox-exec tests, the affected runtime build, format, lint, and typecheck.

Generated-by: pi (gpt-5.6-sol)
@jsiu93
jsiu93 force-pushed the fix/seatbelt-canonical-temp-roots branch from 121a89b to 410e4ed Compare August 30, 2026 11:51

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tightening this up. I reviewed exact head 410e4ede3477b7b7be0df37be099f2ab9c8b5013.

The previous P1 is closed at the correct authority boundary. The resolver now reconstructs only a genuinely missing suffix below the deepest canonical existing ancestor, while permission errors, symlink loops, and other non-missing filesystem failures fail closed instead of falling back to the lexical path. The new regression covers the supported permission-denied case, and the implementation stays within the existing Seatbelt root-normalization seam without adding another path authority.

I found no remaining P0-P2 issue on this head. The hosted test and Windows package checks are still running, so this approval is not a merge-ready claim; please merge only after the required exact-head checks pass.

中文对照

感谢继续收敛这个修复。我复核了最新提交 410e4ede3477b7b7be0df37be099f2ab9c8b5013

之前的 P1 已经在正确的路径权威边界上闭合:现在只会在“尾段确实不存在”时,基于最深的真实祖先重新拼回缺失部分;权限错误、符号链接循环以及其他并非“路径不存在”的文件系统错误都会直接失败,不再回退到词法路径。新增回归测试覆盖了受支持的权限拒绝场景,实现也仍然收敛在现有 Seatbelt root normalization seam 内,没有引入第二套路​​径权威。

这个 head 上我没有发现剩余的 P0-P2。当前 hosted test 和 Windows package 仍在运行,因此这次批准不代表已经满足合并条件;请在最新 head 的必需检查通过后再合并。

AI-assisted review: Codex was used to trace the path authority and verify the incremental fix; I independently checked the reported reachability, severity, and exact-head conclusion.

@Astro-Han
Astro-Han merged commit 9582ce3 into apache:main Aug 30, 2026
4 checks passed
saltand pushed a commit to saltand/maka-agent that referenced this pull request Aug 31, 2026
Canonicalize Seatbelt roots through the deepest existing ancestor so writable roots and explicit denies share one path namespace. Fail closed on permission errors, symlink loops, and other non-missing canonicalization failures.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(runtime): macOS Seatbelt blocks canonical temp paths

4 participants