fix(desktop): forward macOS XPC bootstrap env vars to v2 terminal shells#3799
fix(desktop): forward macOS XPC bootstrap env vars to v2 terminal shells#3799WadeSeidule wants to merge 2 commits into
Conversation
Forward __CFBundleIdentifier, XPC_SERVICE_NAME, XPC_FLAGS, and MallocNanoZone through the strict-shell snapshot on Darwin. Without these, the helper shell that buildMinimalEnv() seeds for spawnCleanShellEnv() loses its Mach handles to securityd and opendirectoryd. That propagates into every PTY downstream and breaks getpwuid_r (whoami returns the uid; OpenSSH fails with "No user exists for uid N") and SecTrustEvaluateWithError (Go binaries like gh report "x509: OSStatus -26276"). curl was unaffected because of the SSL_CERT_FILE shim, which is now belt-and-suspenders rather than the primary fix. The rest of the runtime-strip allowlist stays as-is; Electron, npm, Vite, and host runtime variables remain stripped.
📝 WalkthroughWalkthroughExports the shell bootstrap allowlist and extends it with macOS XPC/bootstrap keys; updates a comment about macOS SSL_CERT_FILE fallback rationale; adds tests asserting the new keys are present and preserved in terminal base env. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes a macOS-specific regression where the helper shell spawned by Confidence Score: 5/5Safe to merge — minimal, targeted change that restores parity with Terminal.app behavior; no secrets or sensitive data is forwarded; existing strip logic is unaffected. Both changed files have no P0/P1 findings. The new keys are proven low-risk (no credentials, no workspace state), they are correctly exempt from the runtime denylist in env-strip.ts, and the if (value) guard in buildMinimalEnv means they are silently skipped on non-macOS platforms. The only finding is a P2 suggestion for additional unit test coverage. No files require special attention.
|
| Filename | Overview |
|---|---|
| packages/host-service/src/terminal/clean-shell-env.ts | Adds four macOS XPC bootstrap keys to SHELL_BOOTSTRAP_KEYS so the helper shell is seeded with them before the env snapshot is taken; no other logic changes. |
| packages/host-service/src/terminal/env.ts | Comment-only update to the SSL_CERT_FILE shim block, reflecting it is now belt-and-suspenders rather than the primary TLS fix; no functional logic changes. |
Sequence Diagram
sequenceDiagram
participant ES as Electron/host-service
participant BME as buildMinimalEnv()
participant CS as spawnCleanShellEnv()
participant SH as Login Shell (-i -l)
participant PTY as PTY (v2 terminal)
participant SVC as macOS securityd / opendirectoryd
ES->>BME: process.env lookup
Note over BME: Copies SHELL_BOOTSTRAP_KEYS<br/>(incl. __CFBundleIdentifier,<br/>XPC_SERVICE_NAME, XPC_FLAGS,<br/>MallocNanoZone — NEW)
BME-->>CS: minimalEnv
CS->>SH: spawn(shell, ["-i","-l","-c","command env"], {env: minimalEnv})
Note over SH: Shell rc files run,<br/>XPC vars present → Mach handles intact
SH-->>CS: stdout (env snapshot)
CS-->>ES: parsedEnv
ES->>PTY: buildV2TerminalEnv(strippedSnapshot)
PTY->>SVC: getpwuid_r / SecTrustEvaluateWithError
Note over SVC: XPC_SERVICE_NAME + __CFBundleIdentifier<br/>present → bootstrap port resolved ✓
SVC-->>PTY: username / cert chain OK
Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/host-service/src/terminal/clean-shell-env.ts
Line: 24-27
Comment:
**Consider adding unit tests for the new XPC bootstrap keys**
The `env.test.ts` file has good coverage for `stripTerminalRuntimeEnv` and `buildV2TerminalEnv`, but there are no tests specifically asserting that `__CFBundleIdentifier`, `XPC_SERVICE_NAME`, `XPC_FLAGS`, and `MallocNanoZone` survive both `buildMinimalEnv()` seeding (i.e. are copied from `process.env` when present) and the `stripTerminalRuntimeEnv` pass (i.e. are not removed by any denylist prefix). A test seeding those four keys into `process.env`, calling `initTerminalBaseEnv`, and asserting they appear in `getTerminalBaseEnv()` would protect against accidental future regression in the strip rules.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(desktop): forward macOS XPC bootstra..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Export SHELL_BOOTSTRAP_KEYS so the test can assert on the seed list, then add one test that pins both halves of the flow: - the four XPC keys are in SHELL_BOOTSTRAP_KEYS, so buildMinimalEnv() copies them from process.env into the helper shell; - the same keys survive stripTerminalRuntimeEnv into the PTY base env (no denylist prefix or exact-key match). Guards against future regression in either the seed list or the strip rules.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/host-service/src/terminal/clean-shell-env.ts (1)
7-7: Optional: freeze the exported allowlist to prevent test mutation.Now that
SHELL_BOOTSTRAP_KEYSis exported, a careless consumer (or test) could.push()into it and pollute every subsequentbuildMinimalEnv()call (the cache ingetStrictShellEnvironmentwould also serve poisoned results until cleared). A trivial guard:♻️ Proposed refactor
-export const SHELL_BOOTSTRAP_KEYS = [ +export const SHELL_BOOTSTRAP_KEYS = Object.freeze([ "HOME", ... "SYSTEMROOT", -]; +] as const);Skip if you'd rather keep the diff minimal — current consumers only read it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/terminal/clean-shell-env.ts` at line 7, The exported array SHELL_BOOTSTRAP_KEYS should be made immutable to prevent external mutation; update the export so consumers cannot push/pop into it (e.g., export a frozen copy or expose it as a readonly array) and ensure buildMinimalEnv and getStrictShellEnvironment continue to read from the immutable version to avoid poisoned cached results; locate SHELL_BOOTSTRAP_KEYS and change its definition to return or hold an Object.freeze(...) (or a readonly tuple/type) so tests/consumers cannot mutate the exported allowlist.packages/host-service/src/terminal/env.test.ts (1)
221-244: Nit: test placement and a more realisticXPC_SERVICE_NAMEvalue.Two purely optional polish points:
- The test asserts both seed-list membership and
getTerminalBaseEnvpreservation, but it lives underdescribe("stripTerminalRuntimeEnv"). The membership assertions are independent of strip behavior; you could either move them to a siblingdescribe("SHELL_BOOTSTRAP_KEYS")block or rename the enclosing describe to make the dual scope explicit.XPC_SERVICE_NAME: "0"is not a realistic macOS value (actual values look likeapplication.com.example.app.123.456). Using a representative string makes the test self-documenting and would catch any future quoting/escaping logic that mishandles characters real values contain (., digits, etc.).♻️ Suggested tweak (value only)
- XPC_SERVICE_NAME: "0", + XPC_SERVICE_NAME: "application.com.example.app.123456.654321",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/terminal/env.test.ts` around lines 221 - 244, The test mixes two concerns: membership of SHELL_BOOTSTRAP_KEYS and behavior of stripTerminalRuntimeEnv/getTerminalBaseEnv; either move the expect(...) membership assertions into a new or sibling describe block for SHELL_BOOTSTRAP_KEYS (or rename the current describe to reflect both concerns) so the test scope is clear, and update the initTerminalBaseEnv call to use a more realistic XPC_SERVICE_NAME string (e.g. "application.com.example.app.123.456") to better exercise quoting/escaping logic; locate these changes around the symbols SHELL_BOOTSTRAP_KEYS, resetTerminalBaseEnvForTests, initTerminalBaseEnv, and getTerminalBaseEnv in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/host-service/src/terminal/clean-shell-env.ts`:
- Line 7: The exported array SHELL_BOOTSTRAP_KEYS should be made immutable to
prevent external mutation; update the export so consumers cannot push/pop into
it (e.g., export a frozen copy or expose it as a readonly array) and ensure
buildMinimalEnv and getStrictShellEnvironment continue to read from the
immutable version to avoid poisoned cached results; locate SHELL_BOOTSTRAP_KEYS
and change its definition to return or hold an Object.freeze(...) (or a readonly
tuple/type) so tests/consumers cannot mutate the exported allowlist.
In `@packages/host-service/src/terminal/env.test.ts`:
- Around line 221-244: The test mixes two concerns: membership of
SHELL_BOOTSTRAP_KEYS and behavior of stripTerminalRuntimeEnv/getTerminalBaseEnv;
either move the expect(...) membership assertions into a new or sibling describe
block for SHELL_BOOTSTRAP_KEYS (or rename the current describe to reflect both
concerns) so the test scope is clear, and update the initTerminalBaseEnv call to
use a more realistic XPC_SERVICE_NAME string (e.g.
"application.com.example.app.123.456") to better exercise quoting/escaping
logic; locate these changes around the symbols SHELL_BOOTSTRAP_KEYS,
resetTerminalBaseEnvForTests, initTerminalBaseEnv, and getTerminalBaseEnv in the
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4cf3755-119a-4491-a2f2-8b6f3bf30725
📒 Files selected for processing (2)
packages/host-service/src/terminal/clean-shell-env.tspackages/host-service/src/terminal/env.test.ts
|
@Kitenite sorry if you are the wrong person to tag here, but can I get a review on this? This issue has made Superset basically unusable for me. |
1 participant
Title
fix(desktop): forward macOS XPC bootstrap env vars to v2 terminal shellsBody
Summary
__CFBundleIdentifier,XPC_SERVICE_NAME,XPC_FLAGS, andMallocNanoZonethrough the strict-shell snapshot inclean-shell-env.tsSSL_CERT_FILEshim inenv.tsto belt-and-suspenders (it stays in for tools that prefer file-based trust roots)Terminal.appandiTermshells inherit, so anything that calls into Directory Services, Keychain, or the Security framework just worksRoot Cause
spawnCleanShellEnv()seeds the helper shell withbuildMinimalEnv()— a small allowlist of keys to forward (HOME,USER,PATH,TERM, …,Apple_PubSub_Socket_Render, etc.) — and then snapshots whatevercommand envproduces. The four XPC bootstrap variables aren't on that list, so the helper shell starts without them, the snapshot doesn't capture them, and every PTY downstream inherits a base env that's missing them.When those identifiers are stripped, the shell is technically running as the right user (the kernel still knows the uid) but anything that does a
getpwuid_r(3)or aSecTrustEvaluateWithErrorcall gets routed to akCFErrorDomainOSStatus-backed error because the system service can't be reached. From the shell's perspective the failure surfaces as:The
OSStatus -26276in that last line is what Go'scrypto/x509/internal/macos.SecTrustEvaluateWithErrorreports whenCFErrorCopyDescriptionfalls back to printing the rawkCFErrorDomainOSStatuscode —CFCopyLocalizedStringis also affected by the missing services, so even the human-readable string is unavailable. The exact code isn't in any publicSecBase.h; what matters is thatSecTrustEvaluateitself is failing at the framework level, not at the cert level.curlwas unaffected because the existingSSL_CERT_FILEshim feeds it a file-based trust root. Anything that goes through the macOS Security framework — Go binaries (gh,terraform,helm), Apple's own command-line tools (security,dscl), OpenSSH's user-resolution path — failed until those env vars came back.The four variables this PR forwards are the minimal set that fixes the symptoms above without weakening the existing runtime-strip allowlist:
__CFBundleIdentifierlibsecinituses it to bind tosecuritydXPC_SERVICE_NAMElaunchd; required bysecuritydandopendirectorydclientsXPC_FLAGSlaunchdMallocNanoZoneI considered a more permissive "forward everything
launchdwould seed" approach, but it's hard to enumerate that set portably across macOS versions, and the four above are sufficient on 14.x and 15.x.The runtime-strip allowlist (
stripTerminalRuntimeEnvinenv-strip.ts) is innocent — none of these four keys match its prefixes (npm_,ELECTRON_,VITE_,NEXT_PUBLIC_,TURBO_,HOST_) or its exact-key set. So adding them toSHELL_BOOTSTRAP_KEYSis sufficient; the strip step leaves them alone naturally.Validation
Manual repro on macOS 14.6:
Diff
Notes
__CFBundleIdentifieris just the parent's bundle ID,XPC_SERVICE_NAMEandXPC_FLAGSare launchd-assigned identifiers, andMallocNanoZoneis a runtime allocator handshake. None of them carry user data, credentials, or workspace state.MallocNanoZonestill fixeswhoami,gh, andgit pushover SSH on the macOS versions I tested. I'd keep it in to avoid an unrelated nano-allocator warning that some libsystem paths emit.SSL_CERT_FILEshim in place. It's redundant once the XPC vars flow, but it's also harmless and protects tools that explicitly prefer file-based trust roots (curl, anything withOPENSSL_CONF-driven trust resolution).Commit message
Summary by CodeRabbit
New Features
Tests
Chores
Summary by cubic
Forward critical macOS XPC bootstrap env vars to v2 terminal shells so Keychain, Directory Services, and TLS trust evaluation work as expected. Restores parity with Terminal/iTerm and keeps the
SSL_CERT_FILEshim as a fallback. Adds a test to prevent regressions.__CFBundleIdentifier,XPC_SERVICE_NAME,XPC_FLAGS, andMallocNanoZoneinclean-shell-env.ts.SSL_CERT_FILEinenv.tsto a fallback for tools that prefer file-based trust.SHELL_BOOTSTRAP_KEYSand add a unit test to ensure the XPC keys flow from seed to PTY env unchanged.whoamishows username (not UID), OpenSSH/Git “No user exists for uid N”, Go tools TLSx509: OSStatus -26276, andsecurity/dsclerrors.Written for commit 35c7019. Summary will update on new commits. Review in cubic