fix(runtime): admit macOS Bash toolchain dependencies - #4279
Conversation
Sandboxed Bash only exposed the Electron runtime root, so Homebrew and Apple Git could be selected from PATH but fail while loading dylibs. Provide a fixed read/execute-only toolchain substrate, make inaccessible Git global config behave as absent, and cover both toolchains with real Seatbelt regression tests.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for tackling the reported macOS dependency failures and for keeping the new grants fixed, read/execute-only, and within the existing Seatbelt owner. I found two bounded normal-path gaps below; these are suggestions rather than a Request Changes review, and please push back if I have misunderstood the supported macOS toolchain contract.
AI-assisted review disclosure: Codex ran two independent analysis lanes; Astro-Han independently verified the exact-head code paths, CI, and findings and owns this review.
Resolve inherited DEVELOPER_DIR or the active xcode-select path before compiling the Bash Seatbelt substrate. Tighten the Homebrew root regression assertions and add an alternate Xcode sandbox smoke test. Generated-by: OpenAI Codex
YayoiNanoka
left a comment
There was a problem hiding this comment.
Thanks for following up on the two earlier macOS coverage gaps. I found one managed-sandbox boundary blocker in the new active-toolchain resolution below; this is a Commented review so the issue stays attached to the exact diff line, and please push back if I have missed an upstream environment-validation invariant.
AI-assisted review disclosure: OpenAI Codex performed the exact-head code-path review and adversarial Seatbelt probe; YayoiNanoka reviewed the evidence and owns this review.
中文
感谢跟进之前的两个 macOS 覆盖缺口。我在新增的 active-toolchain 解析中发现一个受管沙箱边界阻塞问题;这是一个 Commented review,让问题精确挂在对应 diff 行上。如果我遗漏了上游对环境变量的验证不变量,请指正。
AI 辅助审查声明:OpenAI Codex 执行了精确 head 的代码路径审查和对抗性 Seatbelt probe;YayoiNanoka 复核了证据并对本次 review 负责。
| ? dirname(canonicalSelected) | ||
| : canonicalSelected.endsWith('.app') | ||
| ? join(canonicalSelected, 'Contents') | ||
| : canonicalSelected; |
There was a problem hiding this comment.
Thanks for resolving the alternate-Xcode case. [P1] (category ① — managed sandbox boundary integrity): an inherited absolute DEVELOPER_DIR is accepted even when it is not a developer-toolchain path, and a failed realpathSync also preserves that lexical path. For example, DEVELOPER_DIR=/ makes resolveMacosDeveloperToolchainRoot() return /; macosBashExecutableRoots() then collapses to ['/'], and buildSeatbeltPolicy() emits -DEXECUTABLE_ROOT_0=/, allowing file-read* and file-map-executable over the whole filesystem inside a managed Bash sandbox. In a real sandbox-exec positive/negative test, the same managed profile denied an outside fixture without this root but read it successfully after the root was added. Pointing the variable at the user's home similarly exposes that tree, contradicting the PR's security claim. Could we fail closed unless the value resolves to a validated Xcode Contents/Developer or CommandLineTools layout, instead of granting an arbitrary absolute root? Please push back if a caller invariant sanitizes DEVELOPER_DIR before this code; I could not find one.
中文
感谢修复替代 Xcode 场景。[P1](类别 ① —— 受管沙箱边界完整性):继承的绝对路径 DEVELOPER_DIR 即使不是开发工具链路径也会被接受,而 realpathSync 失败时还会保留该 lexical path。例如,DEVELOPER_DIR=/ 会使 resolveMacosDeveloperToolchainRoot() 返回 /;macosBashExecutableRoots() 随后收敛为 ['/'],buildSeatbeltPolicy() 则生成 -DEXECUTABLE_ROOT_0=/,使受管 Bash 沙箱能对整个文件系统执行 file-read* 和 file-map-executable。在真实 sandbox-exec 正反测试中,同一受管 profile 在没有该 root 时拒绝读取 workspace 外的 fixture,加入该 root 后则成功读取。将该变量指向用户主目录同样会暴露整棵目录树,与 PR 的安全声明矛盾。能否只在该值解析为经验证的 Xcode Contents/Developer 或 CommandLineTools 布局时才放行,否则 fail closed,而不是授权任意绝对路径?如果调用方在此代码之前已经验证了 DEVELOPER_DIR,请指正;我没有找到这样的不变量。
There was a problem hiding this comment.
Reviewed b7da3005 (+345/-18). Good evidence — both failures reproduced with the blocked artifact named, and the smoke tests drive real sandbox-exec.
P1 — GIT_CONFIG_GLOBAL=/dev/null silently changes what git add stages. It is set for every sandboxed darwin command, which discards the whole global config, not just the unreadable-file case it targets. core.excludesFile is global-only in most setups, so git add -A now stages .DS_Store, *.local, and whatever else the user's global ignore covered — no error, no warning, committed by the next git commit. The new assertion (env.GIT_CONFIG_GLOBAL === (process.env.GIT_CONFIG_GLOBAL ?? '/dev/null')) restates the assignment and passes for any value. Fix: grant (literal "$HOME/.gitconfig") and (subpath "$HOME/.config/git") read-only, drop both env assignments and the test.
P2 — the /usr/local grant is unconditional in practice. The trigger is PATH membership, /usr/local/bin is line 1 of /etc/paths, and shell-env.ts imports the login PATH — so it fires on every Mac, Homebrew or not, putting read+exec on a user-writable prefix. macos-seatbelt.test.ts:142 pins this; the negative case at :170 uses a PATH without /usr/local, so it doesn't contradict it. Same shape for the Apple roots, appended unconditionally with existence unchecked. Narrower fix: resolve argv's binary through PATH + realpath and open the prefix that actually contains it — one function instead of 2×7 entries, and MacPorts/nix/mise work for free.
P2 — xcode-select -p is spawned synchronously per command (blocking spawnSync, 1s ceiling, on the loop that serves every session). Memoize for the process lifetime; keep the DEVELOPER_DIR read live.
P3 — lib duplicates (allow file-read* (subpath "/opt/homebrew/lib")) and /usr/local/lib already in the base policy.
The bigger point
The base policy is an allow-list of locations accreted one incident at a time — /System, /usr, /bin for Apple's tools, the isolated literals below them from past reports, now Homebrew and Xcode. The next report is MacPorts or nix or mise or rustup. It can't be completed, because it enumerates the unbounded side: programs live in unboundedly many places, secrets in boundedly many.
I'd rather invert it — allow read+exec by default, deny ~/Library, the credential dotfiles, the user-data folders, other /Users, /Library/Keychains, /private/var/root, /Volumes. A deny-list can afford to be generous (over-denying breaks a command visibly; over-allowing is silent), and that list holds still while the toolchain list keeps growing. The real cost: other projects under home become readable, and flipping the default is one-way, so it has to be right on the day it lands — which is why it's an issue for a maintainer to rule on, not something to smuggle in here.
Next step
Fix the P1 here — it's a silent change to committed state, larger than the PR's stated scope, and the replacement is smaller than what's written. Memoize xcode-select while you're in the file. I won't block #4267's fix on the policy redesign; I'd just prefer the table land in resolved-executable form, since hardcoded prefixes pinned by smoke tests are harder to remove later. If you'd rather keep the fixed list, say so and I'll take it as it stands.
On the policy shape: that's yours to file if you agree with it — you have the repro and the dyld traces, and you'll write it better than I would. If you don't, drop it; it's a maintainer call either way and I'd rather it be argued by someone who's actually debugged the failures than added to a backlog by me.
Checked and clean: env comes from process.env only, so the model can't widen the policy through tool params (my first concern); resolveMacosDeveloperToolchainRoot canonicalizes before admitting; the linuxExecutableRoots replacement is behavior-preserving; isPathWithin has no prefix bug. I didn't run sandbox-exec myself.
AI-assisted review: drafted with Maka; I verified the env provenance, the PATH trigger via shell-env.ts, and the base-policy duplication against source myself. The architectural position is mine.
|
Closing this implementation after review. The reported failure remains valid: sandboxed Bash can discover Homebrew or Apple developer tools but still fail while loading runtime dependencies. However, this patch broadens the policy surface and introduces unresolved security and Git-behavior concerns, including unvalidated The problem should remain tracked in #4267 and converge with the Runtime-owned sandbox architecture in #3629. A future implementation should preserve Git semantics, validate every admitted root, and grant only a bounded dependency substrate or dependencies derived from the executable actually selected. Thanks to @Astro-Han and @YayoiNanoka for the detailed reviews. |
Summary
Sandboxed Bash could select Homebrew or Apple developer tools from
PATH, but macOS Seatbelt exposed only the Electron runtime root. The selected executable then failed while loading dependencies under Homebrewopt/Cellaror the active XcodeContents/SharedFrameworks.This change:
DEVELOPER_DIRor boundedxcode-select -p, including beta, versioned, relocated, and symlink-selected Xcode installations;PATHdirectories, Homebrewetc, and the user's home directory outside the sandbox;sandbox-execcoverage for the reported Homebrewgit logcommand shape, Apple Git startup, and an alternate selected Xcode root.Refs #4267
Verification
libpcre2-8.0.dylibblocked by Seatbelt.npm --workspace @maka/runtime test: 3096 passed, 13 skipped, 0 failed on the clean rerun. One preceding run had an unrelated timing failure inworkspace-executorabort-output capture; the same full suite passed immediately on rerun.npm run build: passed.npm run format:check: passed.npm run lint: passed.npm run typecheck: passed after the completed workspace build.Security
The added roots remain read/execute-only. Package-manager grants are fixed subdirectories rather than the whole Homebrew prefix. The active developer selection is canonicalized before admitting its toolchain root. The implementation does not grant writes, network access, arbitrary
PATHroots, Homebrew configuration directories, or home-directory reads.AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex assisted with reproduction, implementation, tests, and PR drafting; human-reviewed.
Checklist
Does this PR entail a change in behavior?