fix(runtime): resolve macOS sandbox temp roots to canonical paths - #4234
Conversation
hqhq1025
left a comment
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
34db9e6 to
121a89b
Compare
jackwener
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
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)
121a89b to
410e4ed
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
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.
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.
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-execsmoke test that runsmktempunder$TMPDIRandtouchunder/tmpfrom an uncanonicalized workspace path.Fixes #4233
Verification
The failing run reproduced both lexical
/tmpdefinitions and temp writes returningOperation not permitted.AI use
Select exactly one:
Tool(s) and scope: pi (gpt-5.6-sol), human-reviewed
Checklist
Does this PR entail a change in behavior?