fix(mac): harden CUA helper distribution integrity - #2030
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
🚧 Files skipped from review as they are similar to previous changes (15)
📝 WalkthroughWalkthroughThe PR adds purpose-aware CUA macOS signing, Darwin load-path sanitation, nested helper verification, updater ZIP validation, distribution-policy checks, and schema-versioned release evidence for macOS packaging. ChangesCUA macOS distribution integrity
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MacOSWorkflow
participant plugin.mjs
participant build-cua-plugin-runtime.mjs
participant sign-cua-helper.mjs
participant package-manifest
participant release-index
MacOSWorkflow->>plugin.mjs: bundle CUA with PACKAGE_PURPOSE and TARGET_ARCH
plugin.mjs->>build-cua-plugin-runtime.mjs: forward --purpose
build-cua-plugin-runtime.mjs->>sign-cua-helper.mjs: sanitize and sign helper
package-manifest->>package-manifest: verify helper, app, DMG, and updater ZIP
package-manifest->>release-index: record macOS distribution checks
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/apple-notarization.js (1)
16-21: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winError message assumes a single caller.
validateAppleTeamIdis now also invoked fromscripts/ci/verify-cua-macos-helper.mjsandscripts/ci/package-manifest.mjswith ateamIdoption that may not originate fromDEEPCHAT_APPLE_NOTARY_TEAM_ID. Including the received value (or an optional label) makes CI failures diagnosable.♻️ Proposed change
-export function validateAppleTeamId(teamId) { - if (!APPLE_TEAM_ID_PATTERN.test(teamId)) { - throw new Error('DEEPCHAT_APPLE_NOTARY_TEAM_ID must be a 10-character Apple team ID') - } +export function validateAppleTeamId(teamId, label = 'DEEPCHAT_APPLE_NOTARY_TEAM_ID') { + if (typeof teamId !== 'string' || !APPLE_TEAM_ID_PATTERN.test(teamId)) { + throw new Error(`${label} must be a 10-character Apple team ID`) + } return teamId }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/apple-notarization.js` around lines 16 - 21, Update validateAppleTeamId to accept an optional label or otherwise include the received teamId in its validation error, rather than always referencing DEEPCHAT_APPLE_NOTARY_TEAM_ID. Preserve the existing validation behavior and return value while making failures from verify-cua-macos-helper.mjs and package-manifest.mjs identify the relevant input.
🧹 Nitpick comments (9)
scripts/ci/package-manifest.mjs (1)
195-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInjected
runCommandis not propagated to the nested verifiers.
verifyMacZipDistributionaccepts arunCommandoverride and uses it forunzip/ditto, but callsverifyCuaMacHelper/verifyMacAppwith only{ teamId }, so those fall back to the realexecFileAsync. Tests hide this by injecting the verifiers themselves; propagating keeps the seam consistent.♻️ Proposed change
- await verifyCuaMacHelper(extractedAppPath, { teamId }) - await verifyMacApp(extractedAppPath, { teamId }) + await verifyCuaMacHelper(extractedAppPath, { teamId, runCommand }) + await verifyMacApp(extractedAppPath, { teamId, runCommand })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci/package-manifest.mjs` around lines 195 - 196, Update verifyMacZipDistribution so its calls to verifyCuaMacHelper and verifyMacApp pass through the injected runCommand alongside teamId, ensuring nested verification uses the same command runner override.test/main/scripts/buildCuaPluginRuntime.test.ts (2)
26-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType the injected callbacks with their real arguments.
The implementation always calls
inspectExecutable(executable)andinspectArchitectures(executable), but the declared type is zero-arg — which is why the test at Line 178 has to widen it to(targetPath?: string). Declaring the parameter keeps the test double honest about the contract.♻️ Proposed change
- inspectExecutable: () => { + inspectExecutable: (targetPath: string) => { rpaths: string[] linkedLibraries: string[] } - inspectArchitectures: () => string[] + inspectArchitectures: (targetPath: string) => string[]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/scripts/buildCuaPluginRuntime.test.ts` around lines 26 - 30, Update the injected callback types in the test setup so inspectExecutable and inspectArchitectures each accept the executable path argument used by the implementation, matching the production contract and eliminating the need for the later type widening near the affected test.
157-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for the architecture-parity guard.
The universal path throws
CUA helper architecture set changed during sanitationwhenlipo -createloses a slice, and the spec lists per-slice rebuild integrity as a fail-closed boundary. A case whereinspectArchitecturesreturns['x86_64','arm64']for the original and['arm64']for the rebuilt binary would lock that in.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/scripts/buildCuaPluginRuntime.test.ts` around lines 157 - 225, Add a test alongside the asymmetric RPATH case for the universal rebuild path where inspectArchitectures returns both x86_64 and arm64 for the original executable but only arm64 for rebuiltExecutable. Assert enforceDarwinLoadPathContract throws “CUA helper architecture set changed during sanitation” and preserves the fail-closed architecture-parity guard.scripts/build-cua-plugin-runtime.mjs (1)
486-506: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo
ensureToolguard forinstall_name_tool/lipo.Other Darwin steps (
validateDarwinArchitecture,signDarwinHelper) callensureToolbefore use, so a missing/unusable command tool yields a clear error. The sanitation path invokes/usr/bin/install_name_tooland/usr/bin/lipodirectly; on a runner without Command Line Tools this fails with a raw spawn error mid-mutation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/build-cua-plugin-runtime.mjs` around lines 486 - 506, Update the Darwin sanitation flow around enforceThinDarwinLoadPathContract and the corresponding lipo usage to call ensureTool for install_name_tool and lipo before any mutation or invocation. Reuse the existing tool-validation pattern from validateDarwinArchitecture and signDarwinHelper, ensuring missing or unusable tools fail clearly before processing begins.scripts/ci/verify-release-assets.mjs (1)
115-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the darwin required-check list into the shared contract.
This list is now duplicated verbatim in
scripts/ci/assemble-release.mjs(lines 58-66) and inscripts/ci/package-manifest.mjs's emitted checks. A single exported constant inscripts/ci/package-contract.mjswould keep producer and verifier from drifting the next time a macOS check is added.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci/verify-release-assets.mjs` around lines 115 - 123, Move the shared macOS required-check names from the local logic around requiredChecks into an exported constant in package-contract.mjs. Update verify-release-assets.mjs, assemble-release.mjs, and package-manifest.mjs to import and reuse that constant, preserving the existing check order and platform-specific behavior.test/main/scripts/releaseAssembly.test.ts (1)
207-225: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore
release-index.jsonin afinally.If the
rejects.toThrowassertion fails, line 224 never runs and the tampered index leaks into any later assertion in this test body.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/scripts/releaseAssembly.test.ts` around lines 207 - 225, Wrap the tampered release-index verification in a try/finally block, keeping the rejects.toThrow assertion in the try and moving the writeFile call that restores originalReleaseIndex into finally. Ensure release-index.json is restored even when verifyReleaseAssets or the assertion fails.test/main/scripts/packageContract.test.ts (1)
396-466: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard
extractionRootbefore asserting cleanup.
await expect(lstat(extractionRoot)).rejects.toThrow()passes trivially ifextractionRootis still''(i.e.dittowas never invoked), so the cleanup assertion can silently become a no-op. Addexpect(extractionRoot).not.toBe('')before each cleanup check.♻️ Proposed tweak
const extractedAppPath = path.join(extractionRoot, 'DeepChat.app') + expect(extractionRoot).not.toBe('') expect(verifyCuaMacHelper).toHaveBeenCalledWith(extractedAppPath, {expect(verifyMacApp).not.toHaveBeenCalled() + expect(extractionRoot).not.toBe('') await expect(lstat(extractionRoot)).rejects.toThrow()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/scripts/packageContract.test.ts` around lines 396 - 466, Guard both cleanup assertions in the tests around verifyMacZipDistribution by asserting extractionRoot is non-empty before calling lstat. Add expect(extractionRoot).not.toBe('') in both test cases after the distribution verification and before the rejects-toThrow cleanup check, ensuring ditto actually ran before cleanup is validated.scripts/sign-cua-helper.mjs (2)
31-79: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider redacting
error.messagetoo, not justerror.stderr.Node's
execFilerejection message embeds the full command line (Command failed: /usr/bin/security create-keychain -p <password> …). You correctly drop it by constructing a freshError, but nothing prevents a future change (or{ cause: error }) from reattaching it. Running the raw message throughredactSensitiveSecurityDiagnosticbefore discarding/including it would make the redaction robust by construction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/sign-cua-helper.mjs` around lines 31 - 79, The error sanitization in formatSensitiveSecurityError only processes error.stderr; update it to also pass error.message through redactSensitiveSecurityDiagnostic before discarding or including it. Ensure any message-derived details cannot expose sensitive command arguments, while preserving the existing formatted error output behavior.
291-305: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify that
developmentis only valid as the implicit default
resolveCuaSigningPurposereturnsDEVELOPMENT_SIGNING_PURPOSEforundefined/empty values, butvalidateArtifactPurposestill rejects explicit--purpose development. Add a short comment or test callout explaining this shortcut if it is intentional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/sign-cua-helper.mjs` around lines 291 - 305, Clarify in resolveCuaSigningPurpose that DEVELOPMENT_SIGNING_PURPOSE is permitted only through the implicit undefined/empty-purpose default path, while explicit non-empty purposes continue through validateArtifactPurpose and may reject development. Add a brief comment or focused test callout at this boundary without changing validation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/ci/verify-cua-macos-helper.mjs`:
- Around line 291-297: Update the codesign verification invocation in the helper
verification flow to include the --deep flag alongside --strict when applying
the Team-ID test requirement. Keep the existing requirement expression and
helperAppPath unchanged, matching the behavior used by verifyMacAppDistribution.
In `@scripts/cua-macos-contract.mjs`:
- Around line 62-78: Update isAllowedDarwinLoadPath so any path containing "."
or ".." traversal segments is rejected regardless of whether it is absolute,
relative, or prefixed with `@rpath`. Remove the value.startsWith('/') restriction
from the segment validation while preserving the existing null, empty, and
allowed-prefix checks.
In `@test/main/scripts/cuaMacosContract.test.ts`:
- Around line 57-80: Update isAllowedDarwinLoadPath so matched `@executable_path`,
`@loader_path`, and `@rpath` prefixes validate their remaining path components with
the same separator-level ./.. traversal check used for absolute paths. Preserve
valid `@-prefixed` relative paths, and extend the allows-only test with a negative
assertion for a traversal path such as `@loader_path/`../../../../etc/evil.dylib.
---
Outside diff comments:
In `@scripts/apple-notarization.js`:
- Around line 16-21: Update validateAppleTeamId to accept an optional label or
otherwise include the received teamId in its validation error, rather than
always referencing DEEPCHAT_APPLE_NOTARY_TEAM_ID. Preserve the existing
validation behavior and return value while making failures from
verify-cua-macos-helper.mjs and package-manifest.mjs identify the relevant
input.
---
Nitpick comments:
In `@scripts/build-cua-plugin-runtime.mjs`:
- Around line 486-506: Update the Darwin sanitation flow around
enforceThinDarwinLoadPathContract and the corresponding lipo usage to call
ensureTool for install_name_tool and lipo before any mutation or invocation.
Reuse the existing tool-validation pattern from validateDarwinArchitecture and
signDarwinHelper, ensuring missing or unusable tools fail clearly before
processing begins.
In `@scripts/ci/package-manifest.mjs`:
- Around line 195-196: Update verifyMacZipDistribution so its calls to
verifyCuaMacHelper and verifyMacApp pass through the injected runCommand
alongside teamId, ensuring nested verification uses the same command runner
override.
In `@scripts/ci/verify-release-assets.mjs`:
- Around line 115-123: Move the shared macOS required-check names from the local
logic around requiredChecks into an exported constant in package-contract.mjs.
Update verify-release-assets.mjs, assemble-release.mjs, and package-manifest.mjs
to import and reuse that constant, preserving the existing check order and
platform-specific behavior.
In `@scripts/sign-cua-helper.mjs`:
- Around line 31-79: The error sanitization in formatSensitiveSecurityError only
processes error.stderr; update it to also pass error.message through
redactSensitiveSecurityDiagnostic before discarding or including it. Ensure any
message-derived details cannot expose sensitive command arguments, while
preserving the existing formatted error output behavior.
- Around line 291-305: Clarify in resolveCuaSigningPurpose that
DEVELOPMENT_SIGNING_PURPOSE is permitted only through the implicit
undefined/empty-purpose default path, while explicit non-empty purposes continue
through validateArtifactPurpose and may reject development. Add a brief comment
or focused test callout at this boundary without changing validation behavior.
In `@test/main/scripts/buildCuaPluginRuntime.test.ts`:
- Around line 26-30: Update the injected callback types in the test setup so
inspectExecutable and inspectArchitectures each accept the executable path
argument used by the implementation, matching the production contract and
eliminating the need for the later type widening near the affected test.
- Around line 157-225: Add a test alongside the asymmetric RPATH case for the
universal rebuild path where inspectArchitectures returns both x86_64 and arm64
for the original executable but only arm64 for rebuiltExecutable. Assert
enforceDarwinLoadPathContract throws “CUA helper architecture set changed during
sanitation” and preserves the fail-closed architecture-parity guard.
In `@test/main/scripts/packageContract.test.ts`:
- Around line 396-466: Guard both cleanup assertions in the tests around
verifyMacZipDistribution by asserting extractionRoot is non-empty before calling
lstat. Add expect(extractionRoot).not.toBe('') in both test cases after the
distribution verification and before the rejects-toThrow cleanup check, ensuring
ditto actually ran before cleanup is validated.
In `@test/main/scripts/releaseAssembly.test.ts`:
- Around line 207-225: Wrap the tampered release-index verification in a
try/finally block, keeping the rejects.toThrow assertion in the try and moving
the writeFile call that restores originalReleaseIndex into finally. Ensure
release-index.json is restored even when verifyReleaseAssets or the assertion
fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b080721d-f802-4009-96f2-729f8a500b79
📒 Files selected for processing (24)
.github/workflows/_package-macos.ymldocs/architecture/ci-release-packaging/spec.mddocs/issues/cua-macos-distribution-integrity/spec.mdelectron-builder.ymlscripts/apple-notarization.jsscripts/build-cua-plugin-runtime.mjsscripts/ci/assemble-release.mjsscripts/ci/classify-package-impact.mjsscripts/ci/package-contract.mjsscripts/ci/package-manifest.mjsscripts/ci/verify-cua-macos-helper.mjsscripts/ci/verify-release-assets.mjsscripts/cua-macos-contract.mjsscripts/macos-release-contract.mjsscripts/plugin.mjsscripts/sign-cua-helper.mjstest/main/plugin/pluginService.test.tstest/main/scripts/buildCuaPluginRuntime.test.tstest/main/scripts/cuaMacosContract.test.tstest/main/scripts/packageContract.test.tstest/main/scripts/packageWorkflow.test.tstest/main/scripts/releaseAssembly.test.tstest/main/scripts/signCuaHelper.test.tstest/main/scripts/verifyCuaMacosHelper.test.ts
This was referenced Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix the macOS clean-install Gatekeeper failure caused by the bundled CUA helper.
The failure required two conditions:
LC_RPATHentries.disable-library-validation.Together, these caused
syspolicy_check distributionto report a fatal error even though ordinary codesign, stapler, and spctl checks passed.Changes
codesign --deepfrom signing while retaining strict deep verification.syspolicy_check distributionagainst the final application.DeepChat.app.Summary by CodeRabbit
Bug Fixes
Release Process