feat(cli/build init): import existing iOS signing credentials from this Mac#2211
feat(cli/build init): import existing iOS signing credentials from this Mac#2211WcaleNieWolny wants to merge 9 commits into
Conversation
…is Mac
`build init` now offers an "Import existing from this Mac" branch as a peer
of the existing "Create new via App Store Connect API" path. Users with a
distribution cert already in their login Keychain and a matching Xcode
provisioning profile can finish onboarding without revoking certs or
touching the Apple Developer Portal.
The new branch:
- silently inventories `security find-identity` + scans both legacy and
Xcode 16+ provisioning-profile dirs (no Keychain prompt)
- matches identities to profiles by SHA1 of embedded developer certs
- asks the user to pick an identity + a matching profile
- asks for distribution mode (app_store / ad_hoc)
- exports a single-identity PKCS#12 via `security export` + node-forge
filtering — this is the *only* macOS Keychain dialog
- routes ad_hoc straight to saving-credentials; app_store reuses the
existing .p8 / Key ID / Issuer ID screens for TestFlight upload
macOS-only: the fork is hidden on non-Darwin hosts (Linux CI still gets
the create-new path unchanged).
New module: cli/src/build/onboarding/macos-signing.ts with injectable
subprocess runner for testability. mobileprovision-parser gains a
parseMobileprovisionDetailed function exposing team ID, expiration,
profile type, and embedded cert SHA1s.
Tests: 11 new tests for macos-signing (parsing + matching + filesystem
scan against a fake home dir) + 5 new tests for the detailed parser.
All existing onboarding/credentials tests still pass.
v1 scope:
- single-bundle apps (extensions/widgets fall back to the existing
`build credentials update --ios-provisioning-profile bundleId=path`
flow with a warning banner)
- login.keychain-db only (custom keychains out of scope for v1)
- both app_store and ad_hoc distribution modes
📝 WalkthroughWalkthroughAdds a macOS "import existing credentials from this Mac" onboarding flow plus supporting tooling: a Swift keychain-export helper, macOS signing utilities, detailed mobileprovision parsing, UI wiring/resume logic, tests, and build/package updates to ship and run the new helper and tests. ChangesmacOS onboarding / signing feature
Packaging, tests, and build wiring
Sequence Diagram(s)sequenceDiagram
participant CLI as Onboarding CLI (Node/TS)
participant Swift as keychain-export (Swift helper)
participant Keychain as macOS Keychain
participant FS as Filesystem
participant AppleAPI as App Store Connect API
CLI->>CLI: scan identities (security) & scan profiles (mobileprovision)
CLI->>CLI: match identities ↔ profiles by cert SHA1
CLI->>Swift: compile helper if missing (swiftc)
CLI->>Swift: run helper with --sha1,--output,--passphrase
Swift->>Keychain: SecItemCopyMatching & SecItemExport (PKCS#12)
Keychain-->>Swift: p12 Data / user-denied error
Swift->>FS: write temp .p12 (atomic), chmod 0600
Swift-->>CLI: emit single-line JSON (success/failure)
CLI->>AppleAPI: optionally list/create profiles / attach certs
CLI->>FS: cleanup temp files
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| // Generate a fake DER cert and compute its SHA1 | ||
| const fakeDer = Buffer.from('hello-world-fake-cert') | ||
| const expectedSha1 = createHash('sha1').update(fakeDer).digest('hex').toLowerCase() |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Fleehopper
left a comment
Fleehopper
left a comment
There was a problem hiding this comment.
Left two things I would fix before marking this ready:
-
cli/src/build/onboarding/macos-signing.ts:376usesrequire('node-forge')inside the new export path. The CLI package istype: moduleand the TS config emits ESNext modules; this function is not covered by the new tests because they injectforgeFilter/ avoid the real P12 filter path. On the actual import flow,filterP12ToSingleIdentity()can be the first time this branch runs, so I would switch this to an ESM-safe lazy import path or a top-levelimport * as forge from 'node-forge'and add a small test that exercises the default filter path enough to catch module loading. -
cli/src/build/onboarding/ui/app.tsx:1050-1083lets the user select any profile that matches the certificate SHA1, regardless of bundle ID or profile type. That can include an old profile for a different app, wildcard/profile edge cases, or an enterprise profile even though enterprise is explicitly out of v1 scope.doSaveCredentials()then persists the map underchosenProfile.bundleId, so onboarding can finish successfully while the next build has no profile for the actual Capacitor app target. I would filter/warn at selection time toappId-compatible profiles and rejectenterprise/developmentprofiles unless that support is intentionally added.
I couldn't run the Bun test suite locally because this environment does not have bun on PATH, so this is a source review rather than a verified run.
… iOS setup-method fork Previously "Import existing" was the first option. Putting "Create new" first matches the conservative default — users hitting the screen for the first time aren't pre-selecting key export — and keeps the existing-flow muscle memory intact for users running `build init` after upgrade.
…recovery
v1.1 of the import-existing flow. Re-orders so the user picks distribution
mode FIRST (right after the silent scan), which makes the .p8 question
asked exactly once and exactly where it belongs:
- app_store → .p8 required (TestFlight upload + free profile recovery)
- ad_hoc → no .p8 by default (preserves the contractor path)
Adds an `import-no-match-recovery` step that turns the previous hard-error
("no profile linked to this cert") into a 4-option menu:
🌐 Open Apple Developer Portal (re-scan after 5s)
🔍 Fetch matching profile from Apple via ASC API
✨ Create a new profile for this cert via ASC API (D2)
↩️ Back to identity selection
The "Fetch" path uses the existing ASC API to list profiles for the
chosen cert (matched by SHA1) and surfaces them through the normal
pick-profile UI. The "Create new profile" path (D2) reuses the existing
ensureBundleId + createProfile from apple-api.ts but skips cert creation
since the user already has one.
ad_hoc users who hit no-match can opt in to providing a .p8 inline; it's
used one-shot and is NOT persisted to credentials.json (the existing
import-mode save logic already excludes APPLE_KEY_* fields for ad_hoc).
Implementation details:
apple-api.ts:
- `listDistributionCerts` gains an `includeContent` option that surfaces
the base64 DER (existing callers unaffected — non-breaking addition)
- new `computeCertSha1(base64) -> hex` helper
- new `findCertIdBySha1(token, sha1) -> AppleCertId | null` helper
- new `listProfilesForCert(token, certId) -> AscProfileSummary[]` helper
- `CertificateLimitError.certificates` typed via new `AscDistributionCert`
types.ts:
- 3 new step values: import-no-match-recovery, import-fetching-profile,
import-create-profile-only
- STEP_PROGRESS values re-tiered to show 4-of-4 progression
- getPhaseLabel renamed (now Step 1-of-4 starts with distribution mode)
ui/app.tsx:
- import-distribution-mode moved to be the FIRST visible import step
- app_store routes through existing api-key-instructions chain → after
`verifying-key`, the new import-mode branch routes back to
import-pick-identity (or resumes a pending recovery action)
- import-pick-identity: no-match no longer dead-ends — routes to
import-no-match-recovery instead
- import-pick-profile: removed the dist-mode pre-selection logic
(dist mode is already chosen upstream)
- import-export-warning back-button now goes to import-pick-profile
- import-exporting: removed the late api-key-instructions detour for
app_store; goes straight to saving-credentials
- 3 new render branches + 2 new useEffect handlers for fetching/creating
- `chosenProfile` can now be synthesized from Apple-API response
(no on-disk path); export logic reads profileBase64 from the
synthesized object when path is empty
Tests:
- 5 new pure-function tests for computeCertSha1
- All v1 tests pass: macos-signing 11/11, mobileprovision 9/9,
credentials 17/17, credentials-validation 13/13
Out of scope for v1.1:
- "I'm at the cert limit but want to create a profile anyway" — not handled
here because findCertIdBySha1 will return null when the cert isn't on
Apple's side, with a clear "use Create new" fallback message
- Saving the one-shot ad_hoc ASC key to progress.json — intentionally
transient so the saved credentials don't carry an unused .p8 path
|
|
||
| t('computeCertSha1 hashes base64-encoded DER bytes', () => { | ||
| const fakeDer = Buffer.from('hello-cert-der') | ||
| const expected = createHash('sha1').update(fakeDer).digest('hex').toLowerCase() |
| options={[ | ||
| ...matchedProfiles.map(p => ({ | ||
| label: `📜 ${p.name} · bundle ${p.bundleId} · ${p.profileType} · expires ${p.expirationDate.split('T')[0]}`, | ||
| value: p.path, |
There was a problem hiding this comment.
This breaks the Apple-side profile recovery path when the certificate has more than one profile. import-fetching-profile synthesizes those profiles with path: '', but this picker uses p.path as the option value and then resolves the selection with find(p => p.path === value). Every fetched profile therefore has the same empty value, so selecting the second/third profile either resolves to the first one or does not change selection at all.
That can save the wrong provisioning profile for a different bundle/distribution mode. Use a stable unique value here, for example p.path || p.uuid, and do the lookup against the same key.
| type: 'profiles', | ||
| attributes: { | ||
| name: profileName, | ||
| profileType: 'IOS_APP_STORE', |
There was a problem hiding this comment.
This ad-hoc recovery branch still creates an App Store profile.
The import flow lets the user choose ad_hoc, and the no-match recovery menu then offers to create a new profile for the existing certificate. That path calls createProfile(), but createProfile() hardcodes profileType: 'IOS_APP_STORE'. So an ad-hoc user can end up saving an App Store provisioning profile while the saved credentials say CAPGO_IOS_DISTRIBUTION: 'ad_hoc', which will not work for QR/direct device installs.
The profile type should be derived from importDistribution here, e.g. pass IOS_APP_ADHOC for ad-hoc recovery and keep IOS_APP_STORE for TestFlight.
digzrow-coder
left a comment
digzrow-coder
left a comment
There was a problem hiding this comment.
The import flow can save a provisioning profile that does not match the distribution mode selected earlier.
import-distribution-mode now asks the user to choose app_store or ad_hoc before identity/profile selection, and doSaveCredentials() later persists that original importDistribution. But import-pick-profile builds its options from all matchedProfiles for the identity without filtering or validating p.profileType against importDistribution; the Apple recovery path also appends every profile returned by listProfilesForCert().
So a user can choose App Store / TestFlight, then pick a locally matching ad-hoc profile. The saved credentials will have distribution: "app_store" and an ad-hoc provisioning profile, so onboarding appears successful but the first TestFlight/build signing path fails later with a hard-to-diagnose profile mismatch. The inverse is also possible for ad-hoc mode with an app-store profile.
Please filter the profile picker to the selected distribution mode, or validate before import-export-warning/saving-credentials and route the user back to the distribution/profile choice with a clear error. A regression test can cover a match set containing one app_store and one ad_hoc profile and assert only the selected mode is accepted.
|
I think the Apple-side recovery path still has a selection bug when more than one profile is returned for the imported certificate.
This matters for the fallback flow because |
Replaces the `security export -t identities` + node-forge filter pipeline
with a small Swift helper (keychain-export.swift) that uses Security
framework's SecItemExport(.formatPKCS12) on the chosen SecIdentity.
Why: `security export -t identities` exports EVERY identity in the user's
Keychain, triggering a separate macOS dialog per private key (4-8 prompts
per import for a typical iOS dev). The Swift helper touches only the
chosen identity, so macOS only prompts about that one.
Prompt count on first run drops from N×2 to exactly 2 (one for the
"access" ACL, one for the "export" ACL — both intrinsic to PKCS#12 export
on Xcode-imported non-extractable keys). Both decisions are cached when
the user clicks "Always Allow", so subsequent runs are silent.
Distribution model: source-only.
- The .swift file ships in dist/keychain-export.swift via a one-line
addition to build.mjs (no precompiled binary, no signing CI yet)
- On first import-flow invocation, macos-signing.ts compiles via
`swiftc` to $TMPDIR/capgo-keychain-export-v$VERSION
- Atomic compile via `<path>.<rand>.tmp` + rename so partial files
never land at the cache key
- Versioned cache key → CLI upgrade triggers fresh compile
- Tmp folder location → reboots / `periodic` daily naturally invalidate
Helper contract:
- Takes --sha1 / --output / --passphrase
- ALWAYS emits one line of JSON on stdout (success or failure) so the
Node caller never has to parse stderr or guess from exit codes
- Distinct exit code 4 for USER_DENIED so callers can offer retry vs.
fall back without string-matching error messages
Removed: ~80 LOC of `filterP12ToSingleIdentity` node-forge surgery (the
v1.1 workaround for the multi-identity P12 problem). The Swift helper
produces a single-identity P12 directly, so node-forge re-encoding is no
longer needed for the import path.
Tests: 7 new tests for `parseHelperJson` (success, USER_DENIED, empty
stdout, unparseable JSON, non-object JSON, whitespace tolerance,
multi-line stdout).
User-facing constraint: requires Xcode Command Line Tools (`swiftc`).
Every iOS dev already has these — Capacitor itself depends on them for
`cap sync`. If `swiftc` is missing, the helper compile fails with a
clear "run xcode-select --install" message.
Deferred to follow-up:
- Code signing + notarization + Developer ID cert provisioning
- Precompiled universal binary distribution via optionalDependencies
- Both unblock removing the `swiftc` prerequisite, but require Apple
Developer Program infra changes — kept separate from this PR
…p, fix JSX whitespace
Three independent fixes bundled together — all in the build-init flow.
1. Resume from interrupted import flow no longer falls into the
create-new path's certificate-creation step (which was triggering the
3-cert-limit error for users at Apple's max). The fork choice now
persists to ~/.capgo-credentials/onboarding/<appId>.json as
`setupMethod`, and getResumeStep branches on it. Legacy progress
files lacking the field default to create-new (no behavior change for
existing users).
The error screen's "Restart onboarding" button now also deletes the
progress file and resets all in-memory import-flow state, so users
stuck in the broken state can recover without manually rm'ing the
progress JSON.
2. New `import-compiling-helper` step shown only on first run (or after
tmpdir cleanup) — explicitly tells the user we're compiling the
keychain-export Swift helper. Without this, the user would stare at
a "look for the macOS dialog" spinner for 2-3 seconds while we
silently invoked swiftc. Cache hit (the common case) skips this step
entirely via a sync existsSync check on the version-keyed tmp path.
Adds isHelperCached() and precompileSwiftHelper() to macos-signing.ts
for the UI to use.
3. Five JSX whitespace bugs where text content adjacent to inline <Text>
elements rendered without a separating space:
- "Track it athttps://..." → "Track it at https://..."
- "Usecapgo build credentials save" → "Use capgo build credentials save"
- "Key ID(detected from filename):" → "Key ID (detected from filename):"
- "Key ID(shown next to the key...":" → "Key ID (shown next to the key...):"
- "Issuer ID(UUID at the very top...)" → "Issuer ID (UUID at the very top...)"
Standard React/Ink behavior: newline + indentation between sibling
elements doesn't render as a space. Fixed via the existing {' '}
convention used elsewhere in the file.
Three independent CodeQL alerts:
1. ui/app.tsx — `importProfiles` declared but never read. The setter is
still called from the import-scanning useEffect to record the full
profile list, but the UI uses `importMatches` for display. Switched
to leading-comma destructuring (`const [, setImportProfiles] = …`)
so the state hook + setter remain valid for any future references
without leaving an unused binding.
2. test/test-mobileprovision-parser.mjs — `Math.random()` feeding into
a SHA1 createHash. Even in test code this trips the "weak RNG into a
hash" rule. Switched to `randomBytes(8)` from node:crypto for the
uniqueness suffix. The fake DER bytes carry no security weight either
way — they're scaffolding that the parser hashes — but using
randomBytes silences the alert and is also strictly better practice.
3. apple-api.ts + mobileprovision-parser.ts — `createHash('sha1')` calls
flagged as "weak cryptographic algorithm." These are NOT a security
primitive: macOS itself reports code-signing identities as cert-DER
SHA1 (via `security find-identity`), and we have to use the same
hash to look up an Apple-side cert by its on-Mac counterpart.
Replacing with SHA256 would break the matching and isn't an option.
Added clear doc comments + `// lgtm[js/weak-cryptographic-algorithm]`
suppression with explanation at each site.
A fourth CodeQL alert about SHA1 in `filterP12ToSingleIdentity` will
auto-resolve once GitHub re-scans the latest tip — that function was
deleted in commit 5b16793 when the Swift helper replaced the
node-forge P12 surgery.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f510606df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| options={[ | ||
| ...matchedProfiles.map(p => ({ | ||
| label: `📜 ${p.name} · bundle ${p.bundleId} · ${p.profileType} · expires ${p.expirationDate.split('T')[0]}`, | ||
| value: p.path, |
There was a problem hiding this comment.
The profile selector keys options by p.path, but profiles synthesized from Apple responses use an empty path, so multiple fetched profiles end up with the same option value. In that case onChange always resolves the first matching profile and the user cannot actually choose among multiple Apple profiles linked to the certificate.
Useful? React with 👍 / 👎.
| const { bundleIdResourceId } = await ensureBundleId(token, appId) | ||
| if (cancelled) | ||
| return | ||
| const profile = await createProfile(token, bundleIdResourceId, certId, appId) |
There was a problem hiding this comment.
The import recovery path always calls createProfile(...), which creates an IOS_APP_STORE profile. If the user chose ad_hoc distribution and hits "create profile" recovery, onboarding still saves CAPGO_IOS_DISTRIBUTION=ad_hoc but pairs it with an App Store provisioning profile, creating a mismatched signing configuration that can break ad-hoc installation flows.
Useful? React with 👍 / 👎.
…mport-ios-creds # Conflicts: # cli/package.json # cli/src/build/onboarding/ui/app.tsx
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (2)
cli/src/build/onboarding/apple-api.ts (1)
384-437:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
createProfilehardcodes App Store profile type, breaking ad-hoc imports.The
createProfilefunction hardcodesprofileType: 'IOS_APP_STORE'at line 405. When the import flow's no-match recovery path creates a profile for an ad-hoc distribution, this results in saving an App Store profile while the credentials recordCAPGO_IOS_DISTRIBUTION: 'ad_hoc', which will fail for QR/direct device installs.🔧 Proposed fix
Add a
profileTypeparameter tocreateProfileand pass it through to the API call:export async function createProfile( token: string, bundleIdResourceId: string, certificateId: string, appId: string, + profileType: 'IOS_APP_STORE' | 'IOS_APP_ADHOC' = 'IOS_APP_STORE', ): Promise<{ profileId: string profileName: string profileContent: string expirationDate: string }> { const profileName = getCapgoProfileName(appId) try { const body = await ascFetch('/profiles', token, { method: 'POST', body: JSON.stringify({ data: { type: 'profiles', attributes: { name: profileName, - profileType: 'IOS_APP_STORE', + profileType, },Callers should pass
'IOS_APP_ADHOC'whenimportDistribution === 'ad_hoc'and'IOS_APP_STORE'otherwise.🤖 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 `@cli/src/build/onboarding/apple-api.ts` around lines 384 - 437, The createProfile function currently hardcodes profileType to 'IOS_APP_STORE'; change its signature (createProfile) to accept a profileType parameter and use that value in the ascFetch POST payload instead of the hardcoded string, and update all callers (e.g., where import flow/no-match recovery calls createProfile) to pass 'IOS_APP_ADHOC' when importDistribution === 'ad_hoc' and 'IOS_APP_STORE' otherwise so ad-hoc imports create the correct IOS_APP_ADHOC provisioning profile.cli/src/build/onboarding/ui/app.tsx (1)
1253-1288:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSelection bug: synthesized profiles all share empty
pathvalue.Profiles fetched from Apple (via
import-fetching-profile) are created withpath: ''(line 558). Usingp.pathas the Select option value means all synthesized profiles have the same empty string value, somatchedProfiles.find(p => p.path === value)always returns the first match regardless of which row the user selected.Use a stable unique key like
p.path || p.uuidfor both the option value and the lookup.🐛 Proposed fix
<Select options={[ ...matchedProfiles.map(p => ({ label: `📜 ${p.name} · bundle ${p.bundleId} · ${p.profileType} · expires ${p.expirationDate.split('T')[0]}`, - value: p.path, + value: p.path || p.uuid, })), { label: '↩️ Back to identity selection', value: '__back__' }, ]} onChange={(value) => { if (value === '__back__') { setStep('import-pick-identity') return } - const profile = matchedProfiles.find(p => p.path === value) + const profile = matchedProfiles.find(p => (p.path || p.uuid) === value) if (!profile) return setChosenProfile(profile) addLog(`✔ Profile · ${profile.name}`) setStep('import-export-warning') }} />🤖 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 `@cli/src/build/onboarding/ui/app.tsx` around lines 1253 - 1288, The Select option values and lookup use p.path which is empty for synthesized profiles, causing every option to share the same value; change the option value to a stable unique key (e.g. use value: p.path || p.uuid) and update the onChange lookup to find by that same key (e.g. find by (p.path || p.uuid) === value) so matchedProfiles selection correctly resolves the chosen profile before calling setChosenProfile, addLog and setStep in the import-pick-profile block.
🤖 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.
Duplicate comments:
In `@cli/src/build/onboarding/apple-api.ts`:
- Around line 384-437: The createProfile function currently hardcodes
profileType to 'IOS_APP_STORE'; change its signature (createProfile) to accept a
profileType parameter and use that value in the ascFetch POST payload instead of
the hardcoded string, and update all callers (e.g., where import flow/no-match
recovery calls createProfile) to pass 'IOS_APP_ADHOC' when importDistribution
=== 'ad_hoc' and 'IOS_APP_STORE' otherwise so ad-hoc imports create the correct
IOS_APP_ADHOC provisioning profile.
In `@cli/src/build/onboarding/ui/app.tsx`:
- Around line 1253-1288: The Select option values and lookup use p.path which is
empty for synthesized profiles, causing every option to share the same value;
change the option value to a stable unique key (e.g. use value: p.path ||
p.uuid) and update the onChange lookup to find by that same key (e.g. find by
(p.path || p.uuid) === value) so matchedProfiles selection correctly resolves
the chosen profile before calling setChosenProfile, addLog and setStep in the
import-pick-profile block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75993644-c6a6-479a-a041-9defcd9db3f0
📒 Files selected for processing (12)
cli/build.mjscli/package.jsoncli/src/build/mobileprovision-parser.tscli/src/build/onboarding/apple-api.tscli/src/build/onboarding/keychain-export.swiftcli/src/build/onboarding/macos-signing.tscli/src/build/onboarding/progress.tscli/src/build/onboarding/types.tscli/src/build/onboarding/ui/app.tsxcli/test/test-apple-api-import-helpers.mjscli/test/test-macos-signing.mjscli/test/test-mobileprovision-parser.mjs
Codex caught a follow-up bug to the earlier setupMethod fix: after a CLI
restart in an import-existing flow, getResumeStep() correctly chooses
the right step (e.g. verifying-key or import-scanning), but the React
state that branches behavior INSIDE those steps wasn't being hydrated.
`importMode` defaulted to `false` and `importDistribution` to `null`,
both reset on every mount. Concrete failure:
1. User picks Import → app_store, enters .p8/keyId/issuerId
2. CLI closes mid `verifying-key`
3. On restart: getResumeStep returns 'verifying-key' ← correct
4. UI mounts with importMode=false ← STALE
5. verifying-key useEffect: `if (importMode) ... else setStep('creating-certificate')`
→ branches into the create-new path
6. Calls Apple to create a NEW distribution cert the user never asked for
7. If user is at the 3-cert limit, blows up with CertificateLimitError
(the exact symptom we already fixed once)
8. Otherwise, silently creates and revokes certs
doSaveCredentials has the same shape of bug:
`needsAscKey = !importMode || importDistribution === 'app_store'`
→ on a clean resume that completed the import, evaluates to
`!false || null === 'app_store'` = `true`, so credentials get
persisted as if create-new and the import path's no-ASC-for-ad_hoc
invariant breaks.
Fix:
1. Hydrate `importMode` from `initialProgress?.setupMethod === 'import-existing'`.
2. Persist `importDistribution` to progress (new `OnboardingProgress.importDistribution`
field) when the user picks it, hydrate it on mount. Could not infer
distribution from .p8 presence because ad_hoc users CAN legitimately
enter a one-shot .p8 during no-match recovery, which would otherwise
make `.p8-presence-implies-app_store` an incorrect heuristic.
3. Clear setupMethod + importDistribution on cancel-out-of-import (the
"↩️ Cancel and use Create new instead" option) so the next resume
doesn't think the user is still mid-import.
Backward compatible: legacy progress files lacking either field default
to false / null, which is the same as today's behavior.
Codex caught a third resume-adjacent bug, this one in the no-match
recovery path:
The import-pick-profile Select keyed options by `p.path`:
options=[
...profiles.map(p => ({ label: ..., value: p.path })),
{ label: 'Back', value: '__back__' },
]
onChange=(value) => { matchedProfiles.find(p => p.path === value) }
Disk-discovered profiles each have a unique path, so this works fine for
the local-match case. But the D path (no-match-recovery → "Fetch from
Apple") synthesizes DiscoveredProfile entries with `path: ''` because
the profile lives only in memory at that point — it was returned by
the ASC API, not read from `~/Library/.../Provisioning Profiles`.
So when Apple returns N profiles linked to the chosen cert, the Select
has N options all with `value: ''`. The user picks one,
`matchedProfiles.find(p => p.path === '')` returns the FIRST match
regardless of which option the user actually highlighted. The user
cannot pick any non-first Apple-fetched profile.
Fix: key by `p.uuid` instead. UUIDs are unique for both kinds:
- Disk-discovered profiles: the mobileprovision plist's UUID field
- Apple-fetched (synthesized) profiles: the Apple resource ID (p.id)
Both flows already populate `uuid`, so no other changes needed.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/src/build/onboarding/ui/app.tsx (1)
872-892:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDuplicate profile handling does not account for import mode — can corrupt flow.
When
import-create-profile-onlythrowsDuplicateProfileError(line 672), it routes toduplicate-profile-prompt. After the user deletes old profiles, line 885 unconditionally resumes tocreating-profile, which is the create-new flow. In import mode,certData.certificateIdis an empty string (line 544), socreateProfileat line 839 will receive an invalid cert ID.The fix: track which step triggered the duplicate-profile flow and resume to that step after deletion.
🐛 Sketch of proposed fix
Track the originating step in state when routing to
duplicate-profile-prompt:+ const [duplicateProfileRetryStep, setDuplicateProfileRetryStep] = useState<OnboardingStep>('creating-profile') // In import-create-profile-only error handler (around line 677): if (err instanceof DuplicateProfileError) { setDuplicateProfiles(err.profiles) + setDuplicateProfileRetryStep('import-create-profile-only') setStep('duplicate-profile-prompt') } // In creating-profile error handler (around line 862): if (err instanceof DuplicateProfileError) { setDuplicateProfiles(err.profiles) + setDuplicateProfileRetryStep('creating-profile') setStep('duplicate-profile-prompt') } // In deleting-duplicate-profiles (line 885): - setStep('creating-profile') + setStep(duplicateProfileRetryStep)🤖 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 `@cli/src/build/onboarding/ui/app.tsx` around lines 872 - 892, When you navigate to the 'duplicate-profile-prompt' flow from the catch in import-create-profile-only (the branch that throws DuplicateProfileError), record the originating step in component state (e.g., originStep or previousStep) so it can be resumed; then in the 'deleting-duplicate-profiles' handler, after successful deletion do not unconditionally setStep('creating-profile') but setStep(originStep || 'creating-profile') so import mode resumes the import path (preserving certData.certificateId semantics) — update the code that routes to 'duplicate-profile-prompt' to set this originStep and clear it after resuming.
🤖 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.
Outside diff comments:
In `@cli/src/build/onboarding/ui/app.tsx`:
- Around line 872-892: When you navigate to the 'duplicate-profile-prompt' flow
from the catch in import-create-profile-only (the branch that throws
DuplicateProfileError), record the originating step in component state (e.g.,
originStep or previousStep) so it can be resumed; then in the
'deleting-duplicate-profiles' handler, after successful deletion do not
unconditionally setStep('creating-profile') but setStep(originStep ||
'creating-profile') so import mode resumes the import path (preserving
certData.certificateId semantics) — update the code that routes to
'duplicate-profile-prompt' to set this originStep and clear it after resuming.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a7946c5-43fe-4118-8ffe-bf4017eb9787
📒 Files selected for processing (2)
cli/src/build/onboarding/types.tscli/src/build/onboarding/ui/app.tsx
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@cli/src/build/onboarding/ui/app.tsx`:
- Around line 672-678: The duplicate-profile recovery currently always sets
setStep('duplicate-profile-prompt') and loses context for import flows; modify
the DuplicateProfileError handling (the block that calls setDuplicateProfiles
and setStep) to preserve an import-specific retry target (e.g., store a
retryOrigin flag or enum alongside setDuplicateProfiles) when the error occurred
during an import path (when certData or import flow variables are present), and
on resume from the duplicate-profile prompt use that stored retry origin to
route back to the import-specific step ('import-create-profile-only' and
re-invoke findCertIdBySha1(...)) instead of blindly retrying 'creating-profile';
ensure the same stored token is set/cleared around deletion so "delete
duplicates and retry" returns to the correct import flow.
- Around line 649-664: The code always synthesizes an App Store profile after
createProfile by hardcoding profileType: 'app_store' and should honor the
selected distribution; update the call site around createProfile and the
synthesized object (symbols: createProfile, synthesized, DiscoveredProfile,
importDistribution, ad_hoc) so that you pass the selected distribution into
createProfile (or into a new parameter like profileType) and set
synthesized.profileType and related fields (e.g., certificateSha1s, bundleId,
applicationIdentifier) based on importDistribution instead of hardcoding
'app_store'; alternatively, if ad_hoc is not supported yet, hide/disable the
"create profile" recovery option when importDistribution === 'ad_hoc' to prevent
creating the wrong profile.
- Around line 1341-1368: The profile picker currently shows every profile in
matchedProfiles which allows selecting a profile whose bundleId or profileType
doesn't match the current appId/importDistribution; filter the options to only
show valid profiles by changing the source to matchedProfiles.filter(p =>
p.bundleId === appId && p.profileType === importDistribution) before mapping to
options, and also validate in the onChange handler (the find for profile by
uuid) that the chosen profile satisfies profile.bundleId === appId &&
profile.profileType === importDistribution before calling
setChosenProfile/addLog/setStep (or else show/return an error); update
references in this block (matchedProfiles, appId, importDistribution,
setChosenProfile, onChange) so both the UI and the continuation logic enforce
the same constraint.
🪄 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
Run ID: 2d4381d7-77cc-449f-a2c9-f59dd8923e0e
📒 Files selected for processing (1)
cli/src/build/onboarding/ui/app.tsx
| const profile = await createProfile(token, bundleIdResourceId, certId, appId) | ||
| if (cancelled) | ||
| return | ||
| // Use the freshly-created profile directly as the chosen profile. | ||
| const synthesized = { | ||
| path: '', | ||
| uuid: profile.profileId, | ||
| name: profile.profileName, | ||
| applicationIdentifier: '', | ||
| bundleId: appId, | ||
| teamId: chosenIdentity.teamId, | ||
| expirationDate: profile.expirationDate, | ||
| profileType: 'app_store' as const, | ||
| certificateSha1s: [chosenIdentity.sha1], | ||
| profileBase64: profile.profileContent, | ||
| } as DiscoveredProfile & { profileBase64: string } |
There was a problem hiding this comment.
Don't hardcode App Store profile creation in the ad_hoc import flow.
This branch is reachable after choosing ad_hoc, but it always creates/synthesizes an App Store profile. That means the saved credentials can end up with CAPGO_IOS_DISTRIBUTION='ad_hoc' paired with an App Store mobileprovision.
Either pass the selected distribution into profile creation here, or hide/disable the "create profile" recovery option when importDistribution === 'ad_hoc' until ad-hoc creation is implemented.
🤖 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 `@cli/src/build/onboarding/ui/app.tsx` around lines 649 - 664, The code always
synthesizes an App Store profile after createProfile by hardcoding profileType:
'app_store' and should honor the selected distribution; update the call site
around createProfile and the synthesized object (symbols: createProfile,
synthesized, DiscoveredProfile, importDistribution, ad_hoc) so that you pass the
selected distribution into createProfile (or into a new parameter like
profileType) and set synthesized.profileType and related fields (e.g.,
certificateSha1s, bundleId, applicationIdentifier) based on importDistribution
instead of hardcoding 'app_store'; alternatively, if ad_hoc is not supported
yet, hide/disable the "create profile" recovery option when importDistribution
=== 'ad_hoc' to prevent creating the wrong profile.
| if (err instanceof DuplicateProfileError) { | ||
| // Existing flow already handles this case for the create-new path — | ||
| // route to the existing duplicate-profile-prompt step, which on | ||
| // resume will retry creation. Set the duplicateProfiles state so | ||
| // the prompt renders correctly. | ||
| setDuplicateProfiles(err.profiles) | ||
| setStep('duplicate-profile-prompt') |
There was a problem hiding this comment.
Preserve an import-specific retry target for duplicate-profile recovery.
Reusing the shared duplicate-profile prompt here breaks the import path: after deletion, the flow retries creating-profile, but this branch hasn't populated certData and should go back through import-create-profile-only/findCertIdBySha1(...) instead. As written, "delete duplicates and retry" can't succeed for imported certificates.
Please carry the retry origin through the duplicate-profile flow and route back to the import-specific step after deletion.
🤖 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 `@cli/src/build/onboarding/ui/app.tsx` around lines 672 - 678, The
duplicate-profile recovery currently always sets
setStep('duplicate-profile-prompt') and loses context for import flows; modify
the DuplicateProfileError handling (the block that calls setDuplicateProfiles
and setStep) to preserve an import-specific retry target (e.g., store a
retryOrigin flag or enum alongside setDuplicateProfiles) when the error occurred
during an import path (when certData or import flow variables are present), and
on resume from the duplicate-profile prompt use that stored retry origin to
route back to the import-specific step ('import-create-profile-only' and
re-invoke findCertIdBySha1(...)) instead of blindly retrying 'creating-profile';
ensure the same stored token is set/cleared around deletion so "delete
duplicates and retry" returns to the correct import flow.
| options={[ | ||
| ...matchedProfiles.map(p => ({ | ||
| label: `📜 ${p.name} · bundle ${p.bundleId} · ${p.profileType} · expires ${p.expirationDate.split('T')[0]}`, | ||
| // Key by UUID, NOT path. Disk-discovered profiles have a | ||
| // unique path, but Apple-fetched profiles (from the D | ||
| // no-match-recovery path) are synthesized with path=''. | ||
| // Keying by path collapses every Apple-fetched profile to | ||
| // value="" and onChange's `find(p => p.path === '')` only | ||
| // ever resolves the first synthesized entry — user can't | ||
| // pick any other. | ||
| // UUID is unique for both kinds: disk profiles use the | ||
| // mobileprovision UUID, synthesized ones use Apple's | ||
| // profile resource ID. | ||
| value: p.uuid, | ||
| })), | ||
| { label: '↩️ Back to identity selection', value: '__back__' }, | ||
| ]} | ||
| onChange={(value) => { | ||
| if (value === '__back__') { | ||
| setStep('import-pick-identity') | ||
| return | ||
| } | ||
| const profile = matchedProfiles.find(p => p.uuid === value) | ||
| if (!profile) | ||
| return | ||
| setChosenProfile(profile) | ||
| addLog(`✔ Profile · ${profile.name}`) | ||
| setStep('import-export-warning') |
There was a problem hiding this comment.
Filter the profile picker to the current app and distribution mode.
This still offers every profile attached to the certificate. If the cert is reused across apps or both App Store/ad-hoc, the user can select a profile with bundleId !== appId or profileType !== importDistribution, and doSaveCredentials() will save a mismatched provisioning map/distribution pair. That will produce unusable signing credentials for this app.
Please either filter matchedProfiles to p.bundleId === appId && p.profileType === importDistribution, or block continuation when the selected profile doesn't match both.
🤖 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 `@cli/src/build/onboarding/ui/app.tsx` around lines 1341 - 1368, The profile
picker currently shows every profile in matchedProfiles which allows selecting a
profile whose bundleId or profileType doesn't match the current
appId/importDistribution; filter the options to only show valid profiles by
changing the source to matchedProfiles.filter(p => p.bundleId === appId &&
p.profileType === importDistribution) before mapping to options, and also
validate in the onChange handler (the find for profile by uuid) that the chosen
profile satisfies profile.bundleId === appId && profile.profileType ===
importDistribution before calling setChosenProfile/addLog/setStep (or else
show/return an error); update references in this block (matchedProfiles, appId,
importDistribution, setChosenProfile, onChange) so both the UI and the
continuation logic enforce the same constraint.
|
I reproduced the current CI blockers locally and pushed a small patch branch against this PR head:
What it fixes:
Validation I ran on the patch:
I did not open a competing upstream PR since this is just a helper patch for #2211. |
|
Update: I extended the helper branch to cover the CodeQL failures too and force-updated it to a single current commit:
Additional fix included:
Validation rerun on
|
|
I put together a helper branch for the current CI/review blockers: https://github.com/digzrow-coder/capgo/tree/codex/capgo-pr2211-helper-forkbase What it addresses:
Validated locally:
I also tried full |
|
Follow-up: the fork CI for the helper branch is green now: https://github.com/digzrow-coder/capgo/actions/runs/25905147169 |
|
I think the helper cache path is too trusting for a flow that exports signing private keys.
I would avoid a shared predictable executable here. Safer options are: compile into a per-user 0700 cache directory such as |
|
I think there is still a resume regression in the import path after the API key is verified.
That means an app-store import session can follow the intended path initially, then if the CLI is interrupted after The fix should be to merge the existing progress record when saving |
7 participants
Summary
build initnow offers an "Import existing from this Mac" branch as a peer of the existing "Create new via App Store Connect API" path. iOS developers who already have a distribution cert in their login Keychain and a matching Xcode provisioning profile can now finish onboarding without revoking certs or touching the Apple Developer Portal.Closes the user-reported gap: "the onboarding will never allow the user to export the key from their machine."
What changes for the user
When a macOS user runs
npx @capgo/cli build initand selects iOS, they now see:The import branch:
security find-identity+ scans both legacy (~/Library/MobileDevice/Provisioning Profiles) and Xcode 16+ (~/Library/Developer/Xcode/UserData/Provisioning Profiles) directories — no Keychain prompt during inventory/usr/bin/security export+ node-forge filtering — exactly one Keychain GUI dialog, regardless of how many identities you havead_hocstraight to saving credentials;app_storereuses the existing.p8/ Key ID / Issuer ID screens for TestFlight uploadWhy this is needed
The current iOS flow is single-track and Apple-API-driven: it always creates a brand-new distribution cert + provisioning profile via the App Store Connect API. Two failure modes:
.p8. Forad_hocdistribution, importing makes the.p8requirement go away entirely.Implementation
cli/src/build/onboarding/macos-signing.ts— wraps/usr/bin/securitysubprocess calls, scans provisioning-profile directories, matches identities to profiles, and exports a filtered single-identity P12. Pure functions where possible; subprocess runner is dependency-injected for testing.cli/src/build/mobileprovision-parser.ts— addedparseMobileprovisionDetailed()that exposes team ID, expiration date, profile type (app_store/ad_hoc/development/enterprise), and SHA1 of every embedded developer cert. ExistingparseMobileprovisionandparseMobileprovisionFromBase64are unchanged.cli/src/build/onboarding/types.ts— 7 new step values + correspondingSTEP_PROGRESSentries +getPhaseLabelcases. All additive; existing step values unchanged.cli/src/build/onboarding/ui/app.tsx— adds the fork atplatform-select(gated byisMacOS()), the import sub-flow renders, and correspondinguseEffecthandlers.doSaveCredentialsis updated to handle the import-mode case.v1 scope
build credentials update --ios-provisioning-profile bundleId=path)login.keychain-db(default Keychain)app_storeandad_hocdistributionSecurity considerations
~/.capgo-credentials/credentials.jsonat0o600alongside existing static'capgo'passphrase from create-new path → no regressionsecurity exportcall writes a temp.p12into amkdtempdirectory andrm -rfs it infinallywhether export succeeds or failsspawn(SECURITY_BIN, [...args])with absolute path and array argv — no shell interpolationTest plan
bun run typecheckcleanbun run lintcleanbun run test:macos-signing— 11/11 passbun test/test-mobileprovision-parser.mjs— 9/9 pass (4 existing + 5 new)test:credentials(15/15),test:credentials-validation(13/13),test:provisioning-map-validation(4/4),test:platform-paths(4/4)security importon Linux) — confirm on first end-to-end buildPre-existing failure (not introduced by this PR)
test:onboarding-recoveryhas 1 pre-existing failure inupdater install state requires node_modules installonorigin/main. None of the files touched here relate toreadUpdaterStateorupdater.ts.Rollback
Remove the
'import'option from theSelectwidget in thesetup-method-selectrender branch. Themacos-signing.tsmodule + new step values become dead code (kept until next cleanup) but harmless.Summary by CodeRabbit
New Features
Enhancements
Tests
Chores