feat(cli): in-repo macOS ASC key helper + iOS onboarding p8 fork + stdout stats protocol#2493
Open
WcaleNieWolny wants to merge 11 commits into
Open
feat(cli): in-repo macOS ASC key helper + iOS onboarding p8 fork + stdout stats protocol#2493WcaleNieWolny wants to merge 11 commits into
WcaleNieWolny wants to merge 11 commits into
Conversation
Add `build credentials apple-key` (alias `asc-key`): on macOS it launches a precompiled Swift helper that walks the user through creating an App Store Connect team API key in an embedded browser, auto-captures issuer id + key id, intercepts the one-time .p8, validates it with Apple, and (with --appId) saves it into iOS build credentials. The helper streams an NDJSON 'stats protocol' on stdout (tagged capgoAscKey:1): - protocol.ts: typed contract + tolerant streaming parser + PostHog mapping, with a hard secret guard so a private key can never reach analytics. - helper.ts: locate the precompiled binary (CAPGO_ASC_KEY_HELPER_PATH or the ~/.capgo cache), spawn it, forward each event line to trackEvent on the 'app-store-connect-key' channel, and return the credentials from the terminal result line. - command.ts: the command action (macOS gate, progress, optional credential save, --json output, analytics flush). - scripts/build-asc-key-helper.sh: build a universal release binary. - PROTOCOL.md: the wire-format + event contract. - test/test-asc-key-protocol.mjs: 9 parser/mapping/secret-guard tests. Telemetry honours CAPGO_DISABLE_TELEMETRY. The binary is not shipped in the npm tarball (macOS-only); it is located/downloaded at runtime.
Contributor
|
Warning Review limit reached
More reviews will be available in 3 hours, 2 minutes, and 23 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a macOS SwiftUI helper app plus CLI integration to guide creating/capturing/validating App Store Connect API keys; the helper and CLI communicate via an NDJSON stdout protocol, and captured credentials are saved to conventional locations. ChangesApp Store Connect API Key Guided Setup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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. Comment |
Contributor
Merging this PR will not alter performance
Comparing 
Footnotes
|
![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: e28991bd80
ℹ️ 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".
Comment on lines
+36
to
+39
| const cached = join(homedir(), '.capgo', 'asc-key-helper', HELPER_BINARY_NAME) | ||
| if (existsSync(cached)) | ||
| return cached | ||
| return null |
There was a problem hiding this comment.
On a normal npm install, cli/package.json only publishes dist/skills and this commit adds no downloader or installer for the macOS helper; this lookup only accepts CAPGO_ASC_KEY_HELPER_PATH or a file that already exists under ~/.capgo. As a result, the documented npx @capgo/cli build credentials apple-key flow fails with HELPER_NOT_FOUND on a fresh macOS machine unless the user manually builds and caches the companion binary, so the new guided command is unusable for its intended public path.
Useful? React with 👍 / 👎.
Comment on lines
+99
to
+101
| if (options.json) { | ||
| // Deliberately excludes the private key — it's on disk at p8Path. | ||
| stdout.write(`${JSON.stringify({ keyId: credentials.keyId, issuerId: credentials.issuerId, p8Path, savedToAppId, eventCount })}\n`) |
There was a problem hiding this comment.
When --json is used after a successful helper run, this JSON line is emitted only after the command has already written the intro/progress/success/info messages above and it is followed by the outro, so consumers parsing stdout as JSON receive a mixed human log stream rather than the documented JSON result. Please suppress or redirect the interactive output for --json, or emit the machine-readable payload on a clean stream.
Useful? React with 👍 / 👎.
Comment on lines
+155
to
+158
| // Flatten small structured values so they're still queryable. | ||
| try { | ||
| return JSON.stringify(value) | ||
| } |
There was a problem hiding this comment.
The secret guard only checks the top-level prop key before JSON.stringify flattens structured values, so an otherwise non-secret key such as details containing { privateKey: '...' } would be forwarded to PostHog. Because the parser accepts arbitrary helper props, recursively strip secret-looking nested keys or drop structured values to preserve the stated guarantee that accidental private key material in event props never reaches analytics.
Useful? React with 👍 / 👎.
Contributor
🧪 Builder onboarding TUI preview — ❌ failed▶ Open the interactive HTML report (zoomable journey tree + cast playback) Commit: 9593a08 · Job summary with the result table |
Move the native macOS Swift helper into the repo at cli/native/asc-key-helper (SwiftPM package, product P8Extract) so it ships and builds with the CLI instead of living in a separate repo. Wire it into the `build init` iOS credentials wizard as the .p8 entry fork: - New step p8-source-select: 'Do you already have a .p8 file?' - Yes -> existing import flows (unchanged). - No -> (macOS + helper present) p8-create-method-select. - p8-create-method-select: Automated (guided helper) vs Manual (create at Apple). - asc-key-generating: spawns the helper, forwards its stdout stats protocol to PostHog, captures keyId/issuerId/.p8, then converges on the existing verifying-key -> save flow. Locator now also finds a local `swift build` output of the vendored package (dev), and build-asc-key-helper.sh defaults to the in-repo package. tsc 0 errors, oxlint clean, asc-key protocol tests green, Swift package builds.
WcaleNieWolny
changed the title
Drive the whole CLI side of the journey against a fake helper binary that speaks the stdout stats protocol: spawn → stream/parse → forward events to PostHog → return/route credentials. Covers success (events forwarded, credentials returned, private key never forwarded), cancel (USER_CANCELLED), crash (no result line + stderr → NO_RESULT), spawn-failed, the secret guard end-to-end, and analytics-off. On non-macOS it asserts the NotMacOSError gate. Wire both asc-key tests into the aggregate `test` script.
Contributor
Author
Added e2e/journey tests for the helper integrationWe can't drive the Swift GUI, but the whole CLI side of the journey is now tested against a fake helper binary that speaks the stdout stats protocol —
Plus The ink wizard routing (the |
E2E/journey tests (spawn the helper, stream the protocol, forward to PostHog, handle cancel/crash) belong in the private cli-mcp-tests suite, not the public repo — same split as keychain signing (hermetic unit test test-macos-signing.mjs stays here; the integration journey lives private). Removes test-asc-key-helper.mjs + its scripts. Keeps test-asc-key-protocol.mjs, the hermetic pure-parser unit test (analogous to test-macos-signing.mjs).
The 3 new onboarding steps (p8-source-select, p8-create-method-select, asc-key-generating) needed progress-bar values and a phase-label case; the project's pinned tsc flags getPhaseLabel's exhaustive switch otherwise. Groups them under the App Store Connect API-key phase.
Contributor
Author
Tests live in the private suitePer our split (hermetic unit tests in capgo, integration journeys in the private suite — same as keychain signing), the e2e/journey tests for this helper are in Cap-go/cli-mcp-tests#5:
What stays here: |
jinhongliang991013
left a comment
jinhongliang991013
left a comment
There was a problem hiding this comment.
Found one data-loss path in the new key helper.
| guard !FileManager.default.fileExists(atPath: file.path) else { return } | ||
| try Data(credentials.privateKey.utf8).write(to: file) | ||
| try FileManager.default.setAttributes([.posixPermissions: 0o600], ofItemAtPath: file.path) | ||
| } catch { |
There was a problem hiding this comment.
The helper still reports success when the only durable key copy was not saved. savePrivateKeyCopy catches directory/write/chmod failures and returns normally, so emit sends ok: true; the standalone command then prints Saved .p8: ... (and its JSON deliberately omits the private key) even when that path was never created. Because Apple does not allow this key to be downloaded again, running without --appId can lose the only usable copy as soon as the process exits. Please make the save operation throw/return a verified status and only emit success after the exact key has been persisted, or propagate a failure that the CLI can handle without claiming the path is saved.
wsyyj-050320
left a comment
wsyyj-050320
left a comment
There was a problem hiding this comment.
I found one security/privacy issue worth fixing before merge.
The new ASC helper delivers the captured .p8 contents on the stdout NDJSON result line:
cli/native/asc-key-helper/Sources/Models/StatsProtocol.swiftdocuments and emitsprivateKey: credentials.privateKeyinStatsProtocol.result().cli/src/build/onboarding/asc-key/protocol.tsacceptsprivateKeyfrom any successresultline.cli/test/test-asc-key-protocol.mjsmakes the raw private key in stdout part of the expected protocol behavior.
Even if the current CLI does not forward that result line to PostHog, stdout is still a broad capture surface: terminal transcripts, parent process logs, CI/e2e harnesses, process supervisors, debug artifacts, and future wrappers commonly record it. The PR also names this stream a stats/telemetry protocol, so future maintainers may reasonably treat protocol lines as safe to persist. A one-time App Store Connect .p8 key is long-lived signing material; putting it on stdout greatly increases the chance of accidental disclosure compared with the existing local key-file path.
Suggested fix: keep the helper writing ~/.appstoreconnect/private_keys/AuthKey_<keyId>.p8 with restrictive permissions, but have stdout carry only non-secret fields such as keyId, issuerId, and p8Path/an opaque handle. The CLI can then read that file path when it needs to update saved credentials, without making the raw key part of the protocol stream. Please also update PROTOCOL.md and test-asc-key-protocol.mjs so tests fail if a success result contains raw privateKey material.
…ernal support log Add a third stdout protocol kind, `log`, alongside `event`/`result`. Where `event` lines feed PostHog, `log` lines are verbose diagnostics routed into the CLI's internal support log — the bundle a user emails to support when a run goes wrong. Like events, they never carry the private key. Protocol (protocol.ts): AscLogLine + level (debug/info/warn/error, defaulting to info), tolerant parser branch, and formatInternalLogLine() rendering `[asc-helper +<ts>ms] <LEVEL> <message> <props>` with a secret-key prop guard. Consumer (helper.ts): route log lines to appendInternalLog (default on, gated by forwardToInternalLog), an onLog observer, a logCount on the outcome, and a per-run summary breadcrumb so a bundle always shows the helper ran + how it ended. Standalone command.ts now starts an internal log so its bundle captures the diagnostics too (the onboarding path already starts one). Swift (StatsProtocol.swift): log()/debug()/info()/warn()/error() emitters. GuidedFlowModel instruments the failure points support actually needs: DOM finder misses (issuer-id scrape), wrong-role selection, team-level access denial (with team + roles), team-switch attempts/timeouts, off-flow navigation steering, validation-failure detail, and wrong .p8 file selection. Tests: protocol unit test covers log parsing, level defaulting, the renderer, and the secret guard. PROTOCOL.md documents the new kind + emit points. Journey coverage (internal-log forwarding, secret guard, run summary, forwarding-off) lives in the private suite.
… centrally
The CLI was rendering each helper `log` line into a bespoke
`[asc-helper +<ts>ms] <LEVEL> <msg> {props}` string. That added little: the
internal log's appendInternalLog already prefixes an ISO timestamp (making the
run-relative +Nms redundant) and runs redactSecrets, and the formatter
re-implemented a secret guard as a second redaction authority.
Drop formatInternalLogLine. helper.ts now appends the helper's own line with
minimal shaping (`asc-helper <level>: <message> {props}`); appendInternalLog
owns the timestamp + redaction. The one thing the formatter's guard did beyond
cosmetics — catching a p8/pem-keyed secret prop that redactSecrets misses — moves
into redactSecrets (which now protects every internal-log line, not just helper
ones), with the closing-quote anchor keeping useful keys like p8Path intact.
Tests: protocol unit test keeps log parsing + level defaulting (drops the
renderer cases); redact test gains p8/pem coverage. PROTOCOL.md updated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (1)
cli/native/asc-key-helper/Sources/Models/KeyCredentials.swift (1)
15-27:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFail the run when local
.p8persistence fails before emitting success.Line 16 calls
savePrivateKeyCopywithout checking outcome, and Lines 50-52 swallow file-write failures. The helper can still emit a successresultand exit 0 even when no durable key copy exists.💡 Suggested fix
enum CredentialsEmitter { @@ static func emit(_ credentials: KeyCredentials) { - savePrivateKeyCopy(credentials) + do { + try savePrivateKeyCopy(credentials) + } catch { + StatsProtocol.resultFailure( + code: "P8_SAVE_FAILED", + message: "Could not save AuthKey_\(credentials.keyId).p8: \(error.localizedDescription)" + ) + StatsProtocol.event("helper_finished", [ + "ok": false, + "outcome": "save_failed", + "total_ms": StatsProtocol.elapsedMs(), + ]) + exit(1) + } didEmit = true @@ - private static func savePrivateKeyCopy(_ credentials: KeyCredentials) { + private static func savePrivateKeyCopy(_ credentials: KeyCredentials) throws { @@ - do { - try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) - guard !FileManager.default.fileExists(atPath: file.path) else { return } - try Data(credentials.privateKey.utf8).write(to: file) - try FileManager.default.setAttributes([.posixPermissions: 0o600], ofItemAtPath: file.path) - } catch { - FileHandle.standardError.write(Data("warning: could not save key copy: \(error)\n".utf8)) - } + try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) + guard !FileManager.default.fileExists(atPath: file.path) else { return } + try Data(credentials.privateKey.utf8).write(to: file, options: .atomic) + try FileManager.default.setAttributes([.posixPermissions: 0o600], ofItemAtPath: file.path) } }Also applies to: 41-52
🤖 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/native/asc-key-helper/Sources/Models/KeyCredentials.swift` around lines 15 - 27, The emit(_ credentials: KeyCredentials) path currently calls savePrivateKeyCopy and then unconditionally sets didEmit, emits a success StatsProtocol.result/event and exit(0) even if the .p8 write failed; change savePrivateKeyCopy to either throw on failure or return a Bool indicating success (and stop swallowing IO errors where the file write happens), then update emit(_:) to check that result (or catch the error), and only proceed to set didEmit, call StatsProtocol.result and emit a success event/exit(0) when persistence succeeded; on failure, emit a failure StatsProtocol.event (ok: false, include error message), do not call StatsProtocol.result, and exit with a non‑zero code. Ensure both the savePrivateKeyCopy implementation (fix file-write error handling) and the emit(_:) caller are updated accordingly.
🤖 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/native/asc-key-helper/Sources/Models/FlowScripts.swift`:
- Around line 121-125: The function switchTeamViaMenuScript currently builds
`safe` by stripping backslashes and manually escaping single quotes which can
change the team name and miss-escape other characters; replace that manual logic
and instead generate a proper JavaScript string literal from `teamName` (i.e.,
produce a full quoted/escaped JS string using a JSON/string-encoding routine)
and interpolate that JS-safe literal into the returned script; update the use of
the `safe` variable in switchTeamViaMenuScript to use the new `jsStringLiteral`
(no manual replace calls), ensuring all characters (quotes, backslashes,
newlines, unicode, etc.) are correctly escaped for JS.
In `@cli/native/asc-key-helper/Sources/Models/GuidedFlowModel.swift`:
- Line 42: The class GuidedFlowModel spawns a long-lived background Task for
polling but has no teardown; fix by storing that Task in a property (e.g.,
private var pollingTask: Task<Void, Never>?) when you create it (the Task
started in the polling loop) and implement deinit { pollingTask?.cancel() } to
cancel the task on deallocation; update the code that creates the Task to assign
it to pollingTask and ensure any loops check Task.isCancelled so they exit
promptly.
In `@cli/native/asc-key-helper/Sources/Web/WebViewContainer.swift`:
- Around line 150-159: The downloadDidFinish(_:) handler currently returns early
on extension/content mismatch and never removes the temporary file; fix by
capturing the destination first and immediately scheduling its removal (e.g.,
let destination = downloadDestination; defer { try?
FileManager.default.removeItem(at: destination) }) so the temp file is deleted
regardless of validation outcome, then read the file contents and perform the
extension/content checks and call model.privateKeyCaptured(pem) only if valid;
apply the same defer-based removal pattern to the other download completion
block that has the same early-return behavior so all temp downloads are always
removed.
- Around line 96-101: The data-URL navigation is being unconditionally cancelled
in WebViewContainer when url.scheme == "data"; change the logic so you only call
decisionHandler(.cancel) and return after a successful PEM parse (i.e., when
Self.decodePEM(fromDataURL: url) yields a pem and you call
model.privateKeyCaptured(pem)); if decodePEM fails, call decisionHandler(.allow)
(or simply fall through to the normal handling) so the navigation isn’t dropped
and fallback parsing can proceed; update the if-let handling around
navigationAction.request.url, Self.decodePEM(fromDataURL:),
model.privateKeyCaptured(_:) and decisionHandler accordingly.
In `@cli/scripts/build-asc-key-helper.sh`:
- Around line 50-58: The fallback find logic in build-asc-key-helper.sh can pick
up arbitrary/stale executables; restrict the search for BUILT to
release-artifacts only by changing the fallback that sets BUILT to use find with
a path filter for release builds (e.g. -path "*/Release/*/$PRODUCT_NAME" or
-path "*/Products/Release/*/$PRODUCT_NAME"), keep the -type f and -perm -111
checks, and then pick the first matching file; ensure the variable names BUILT,
PRODUCT_NAME and SRC_DIR are used unchanged and that the existing existence
check for BUILT remains after the new find.
In `@cli/src/build/onboarding/asc-key/command.ts`:
- Around line 1-11: Replace the manual error-stringing in the unexpected-error
catch branch with the CLI’s shared formatter: import the shared formatError
function from the CLI’s error-formatting module and call formatError(err) (or
formatError({ error: err })) when logging or showing the error to the user
instead of dumping raw exception text; update the catch branch that currently
builds the raw message (the unexpected-error path in this file that runs
alongside runAscKeyHelper / ASC_KEY_CHANNEL) to use formatError and ensure the
new import is added at the top.
- Around line 44-46: Update the helper-missing remediation text so it reflects
the actual discovery model instead of implying the binary is bundled in CLI
releases: change the log.info call that currently tells users to "upgrade to a
CLI release that bundles it" to instead instruct users to set
CAPGO_ASC_KEY_HELPER_PATH to a compiled helper or ensure the helper is available
via their environment/cache/dev build process; keep the log.error message and
the trackEvent call (ASC_KEY_CHANNEL, trackEvent(..., apikey: options.apikey))
unchanged.
In `@cli/src/build/onboarding/asc-key/helper.ts`:
- Around line 147-149: The stderr buffer is unbounded (variable stderr) and must
be size-capped to prevent memory growth: introduce a MAX_STDERR_BYTES constant
and update all append sites that modify stderr to only append up to the
remaining capacity (or drop/rotate oldest content) so stderr never exceeds
MAX_STDERR_BYTES; keep eventCount and logCount logic unchanged but ensure the
same capping behavior is applied at the other stderr accumulation locations
mentioned (the other stderr append sites in this file) so all places that write
to stderr share the cap and behavior.
- Around line 132-134: The error message in HELPER_NOT_FOUND should be updated
to reflect actual distribution behavior: replace the sentence suggesting “use a
CLI release that bundles it” with guidance to set CAPGO_ASC_KEY_HELPER_PATH to a
compiled helper, or build/compile the helper locally (or install via the
project’s documented local-build/cache workflow). Edit the message string (the
one currently starting "Could not locate the App Store Connect key helper
binary...") to mention CAPGO_ASC_KEY_HELPER_PATH and local build/cache options
instead of an npm/CLI-bundled binary.
In `@cli/src/build/onboarding/asc-key/PROTOCOL.md`:
- Around line 3-5: Update the example CLI invocation in PROTOCOL.md to use the
canonical repository form: replace the current `build credentials apple-key`
example with `npx `@capgo/cli`@latest build credentials apple-key` so all
customer-facing command examples follow the “npx `@capgo/cli`@latest ...”
convention.
In `@cli/src/build/onboarding/asc-key/protocol.ts`:
- Around line 197-205: The coerceTagValue function currently serializes
non-scalar structured values without redacting nested secret-like data, so
update coerceTagValue to detect Objects and Arrays and recursively redact any
keys or values that match secret patterns (e.g., keys like "secret", "token",
"password", "apiKey" or values starting with secret prefixes such as "sk-" or
PEM markers) before calling JSON.stringify; implement the recursion as a helper
(e.g., redactSecretsInStructure) that returns a sanitized structure (replacing
secrets with a fixed redaction token) and call that helper from coerceTagValue
for non-scalar inputs, and apply the same helper in the other serializer sites
mentioned so nested secrets are never emitted.
---
Duplicate comments:
In `@cli/native/asc-key-helper/Sources/Models/KeyCredentials.swift`:
- Around line 15-27: The emit(_ credentials: KeyCredentials) path currently
calls savePrivateKeyCopy and then unconditionally sets didEmit, emits a success
StatsProtocol.result/event and exit(0) even if the .p8 write failed; change
savePrivateKeyCopy to either throw on failure or return a Bool indicating
success (and stop swallowing IO errors where the file write happens), then
update emit(_:) to check that result (or catch the error), and only proceed to
set didEmit, call StatsProtocol.result and emit a success event/exit(0) when
persistence succeeded; on failure, emit a failure StatsProtocol.event (ok:
false, include error message), do not call StatsProtocol.result, and exit with a
non‑zero code. Ensure both the savePrivateKeyCopy implementation (fix file-write
error handling) and the emit(_:) caller are updated accordingly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cf00913f-a4a5-41d2-aa12-8e1eb131c953
📒 Files selected for processing (29)
cli/native/asc-key-helper/.gitignorecli/native/asc-key-helper/Package.swiftcli/native/asc-key-helper/README.mdcli/native/asc-key-helper/Sources/Models/ASCSession.swiftcli/native/asc-key-helper/Sources/Models/FlowScripts.swiftcli/native/asc-key-helper/Sources/Models/FlowStep.swiftcli/native/asc-key-helper/Sources/Models/GuidedFlowModel.swiftcli/native/asc-key-helper/Sources/Models/KeyCredentials.swiftcli/native/asc-key-helper/Sources/Models/StatsProtocol.swiftcli/native/asc-key-helper/Sources/P8ExtractApp.swiftcli/native/asc-key-helper/Sources/UI/ContentView.swiftcli/native/asc-key-helper/Sources/UI/FlowDialogs.swiftcli/native/asc-key-helper/Sources/UI/StepsPanel.swiftcli/native/asc-key-helper/Sources/UI/TeamMonogram.swiftcli/native/asc-key-helper/Sources/Util/P8FileLocator.swiftcli/native/asc-key-helper/Sources/Validation/ASCKeyValidator.swiftcli/native/asc-key-helper/Sources/Web/WebViewContainer.swiftcli/native/asc-key-helper/THIRD-PARTY-LICENSES.mdcli/package.jsoncli/scripts/build-asc-key-helper.shcli/src/build/onboarding/asc-key/PROTOCOL.mdcli/src/build/onboarding/asc-key/command.tscli/src/build/onboarding/asc-key/helper.tscli/src/build/onboarding/asc-key/protocol.tscli/src/build/onboarding/types.tscli/src/build/onboarding/ui/app.tsxcli/src/build/onboarding/ui/steps/ios-credentials.tsxcli/src/index.tscli/test/test-asc-key-protocol.mjs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Cap-go/capacitor-updater(manual)
Comment on lines
+121
to
+125
| static func switchTeamViaMenuScript(teamName: String) -> String { | ||
| let safe = teamName | ||
| .replacingOccurrences(of: "\\", with: "") | ||
| .replacingOccurrences(of: "'", with: "\\'") | ||
| return """ |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="cli/native/asc-key-helper/Sources/Models/FlowScripts.swift"
# Show the relevant section with line numbers (around the cited lines)
nl -ba "$FILE" | sed -n '90,190p'Repository: Cap-go/capgo
Length of output: 99
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="cli/native/asc-key-helper/Sources/Models/FlowScripts.swift"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
# Print with line numbers, focusing around the suspected area.
awk '{printf "%6d %s\n", NR, $0}' "$FILE" | sed -n '90,190p'Repository: Cap-go/capgo
Length of output: 5638
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "switchTeamViaMenuScript" -S cli/native/asc-key-helper/Sources/Models/FlowScripts.swift
rg -n "\\(safe\\)" -S cli/native/asc-key-helper/Sources/Models/FlowScripts.swift
rg -n "replacingOccurrences\\(of:" -S cli/native/asc-key-helper/Sources/Models/FlowScripts.swift | sed -n '1,120p'Repository: Cap-go/capgo
Length of output: 489
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="cli/native/asc-key-helper/Sources/Models/FlowScripts.swift"
awk '{printf "%6d %s\n", NR, $0}' "$FILE" | sed -n '105,160p'Repository: Cap-go/capgo
Length of output: 3298
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="cli/native/asc-key-helper/Sources/Models/FlowScripts.swift"
rg -n "JSONSerialization" "$FILE" || true
rg -n "^import " "$FILE" || trueRepository: Cap-go/capgo
Length of output: 76
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="cli/native/asc-key-helper/Sources/Models/FlowScripts.swift"
awk '{printf "%6d %s\n", NR, $0}' "$FILE" | sed -n '200,270p'Repository: Cap-go/capgo
Length of output: 4286
Generate teamName as a JS string literal instead of stripping/escaping characters manually.
In switchTeamViaMenuScript, teamName is sanitized by removing backslashes and only escaping ', then interpolated into a JS single-quoted literal (=== '\(safe)'). This can change the intended team name (e.g., \ removal) and still leaves other characters that aren’t literal-safe, leading to missed menu-row matches (or potential script breakage).
💡 Suggested fix
static func switchTeamViaMenuScript(teamName: String) -> String {
- let safe = teamName
- .replacingOccurrences(of: "\\", with: "")
- .replacingOccurrences(of: "'", with: "\\'")
+ let safe: String
+ if let data = try? JSONSerialization.data(withJSONObject: [teamName]),
+ let arrayLiteral = String(data: data, encoding: .utf8) {
+ // ["Team"] -> "Team" (already JSON-escaped)
+ safe = String(arrayLiteral.dropFirst().dropLast())
+ } else {
+ safe = "\"\""
+ }
return """
@@
- target = lis.find(li => li.textContent.trim() === '\(safe)');
+ target = lis.find(li => li.textContent.trim() === \(safe));🤖 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/native/asc-key-helper/Sources/Models/FlowScripts.swift` around lines 121
- 125, The function switchTeamViaMenuScript currently builds `safe` by stripping
backslashes and manually escaping single quotes which can change the team name
and miss-escape other characters; replace that manual logic and instead generate
a proper JavaScript string literal from `teamName` (i.e., produce a full
quoted/escaped JS string using a JSON/string-encoding routine) and interpolate
that JS-safe literal into the returned script; update the use of the `safe`
variable in switchTeamViaMenuScript to use the new `jsStringLiteral` (no manual
replace calls), ensuring all characters (quotes, backslashes, newlines, unicode,
etc.) are correctly escaped for JS.
| /// (https://github.com/MortenGregersen/AppStoreConnectKit), | ||
| /// MIT License, © Morten Bjerg Gregersen. See THIRD-PARTY-LICENSES.md. | ||
| @MainActor @Observable | ||
| final class GuidedFlowModel { |
Contributor
There was a problem hiding this comment.
Cancel the polling loop in deinit to prevent an orphaned background task.
Lines 416-424 start a long-lived task, but the class has no teardown. After deallocation, the weak-self loop can continue sleeping forever unless explicitly cancelled.
💡 Suggested fix
`@MainActor` `@Observable`
final class GuidedFlowModel {
@@
private var didEmitAccessDenied = false
+
+ deinit {
+ pollTask?.cancel()
+ }Also applies to: 416-424
🧰 Tools
🪛 SwiftLint (0.63.3)
[Warning] 42-42: Classes should have an explicit deinit method
(required_deinit)
🤖 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/native/asc-key-helper/Sources/Models/GuidedFlowModel.swift` at line 42,
The class GuidedFlowModel spawns a long-lived background Task for polling but
has no teardown; fix by storing that Task in a property (e.g., private var
pollingTask: Task<Void, Never>?) when you create it (the Task started in the
polling loop) and implement deinit { pollingTask?.cancel() } to cancel the task
on deallocation; update the code that creates the Task to assign it to
pollingTask and ensure any loops check Task.isCancelled so they exit promptly.
Source: Linters/SAST tools
Comment on lines
+96
to
+101
| if let url = navigationAction.request.url, url.scheme == "data" { | ||
| if let pem = Self.decodePEM(fromDataURL: url) { | ||
| model.privateKeyCaptured(pem) | ||
| } | ||
| decisionHandler(.cancel) | ||
| return |
Contributor
There was a problem hiding this comment.
Only cancel data: navigation after successful PEM capture.
Line 100 currently cancels every data: navigation even when Line 97 fails to parse PEM. That can drop the key flow if Apple slightly changes payload formatting, with no fallback path.
Suggested fix
) {
if let url = navigationAction.request.url, url.scheme == "data" {
if let pem = Self.decodePEM(fromDataURL: url) {
model.privateKeyCaptured(pem)
+ decisionHandler(.cancel)
+ return
}
- decisionHandler(.cancel)
- return
+ // Unknown data URL shape: don't hard-cancel; allow normal fallback paths.
+ if navigationAction.shouldPerformDownload {
+ decisionHandler(.download)
+ } else {
+ decisionHandler(.allow)
+ }
+ return
}
if navigationAction.shouldPerformDownload {
decisionHandler(.download)
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if let url = navigationAction.request.url, url.scheme == "data" { | |
| if let pem = Self.decodePEM(fromDataURL: url) { | |
| model.privateKeyCaptured(pem) | |
| } | |
| decisionHandler(.cancel) | |
| return | |
| if let url = navigationAction.request.url, url.scheme == "data" { | |
| if let pem = Self.decodePEM(fromDataURL: url) { | |
| model.privateKeyCaptured(pem) | |
| decisionHandler(.cancel) | |
| return | |
| } | |
| // Unknown data URL shape: don't hard-cancel; allow normal fallback paths. | |
| if navigationAction.shouldPerformDownload { | |
| decisionHandler(.download) | |
| } else { | |
| decisionHandler(.allow) | |
| } | |
| return | |
| } |
🤖 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/native/asc-key-helper/Sources/Web/WebViewContainer.swift` around lines 96
- 101, The data-URL navigation is being unconditionally cancelled in
WebViewContainer when url.scheme == "data"; change the logic so you only call
decisionHandler(.cancel) and return after a successful PEM parse (i.e., when
Self.decodePEM(fromDataURL: url) yields a pem and you call
model.privateKeyCaptured(pem)); if decodePEM fails, call decisionHandler(.allow)
(or simply fall through to the normal handling) so the navigation isn’t dropped
and fallback parsing can proceed; update the if-let handling around
navigationAction.request.url, Self.decodePEM(fromDataURL:),
model.privateKeyCaptured(_:) and decisionHandler accordingly.
Comment on lines
+150
to
+159
| func downloadDidFinish(_ download: WKDownload) { | ||
| guard let destination = downloadDestination, | ||
| destination.pathExtension == "p8", | ||
| let pem = try? String(contentsOf: destination, encoding: .utf8), | ||
| pem.contains("PRIVATE KEY") else { | ||
| return | ||
| } | ||
| model.privateKeyCaptured(pem) | ||
| try? FileManager.default.removeItem(at: destination) | ||
| } |
Contributor
There was a problem hiding this comment.
Always remove temporary download files, even on parse/extension mismatch.
Lines 151-156 return early without deleting the temp file. If the file contains sensitive key material but fails extension/content checks, it remains on disk.
Suggested fix
func downloadDidFinish(_ download: WKDownload) {
- guard let destination = downloadDestination,
- destination.pathExtension == "p8",
- let pem = try? String(contentsOf: destination, encoding: .utf8),
- pem.contains("PRIVATE KEY") else {
- return
- }
- model.privateKeyCaptured(pem)
- try? FileManager.default.removeItem(at: destination)
+ guard let destination = downloadDestination else { return }
+ defer {
+ try? FileManager.default.removeItem(at: destination)
+ downloadDestination = nil
+ }
+ guard destination.pathExtension.lowercased() == "p8",
+ let pem = try? String(contentsOf: destination, encoding: .utf8),
+ pem.contains("PRIVATE KEY") else {
+ return
+ }
+ model.privateKeyCaptured(pem)
}
func download(_ download: WKDownload, didFailWithError error: Error, resumeData: Data?) {
+ if let destination = downloadDestination {
+ try? FileManager.default.removeItem(at: destination)
+ }
downloadDestination = nil
}Also applies to: 161-163
🤖 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/native/asc-key-helper/Sources/Web/WebViewContainer.swift` around lines
150 - 159, The downloadDidFinish(_:) handler currently returns early on
extension/content mismatch and never removes the temporary file; fix by
capturing the destination first and immediately scheduling its removal (e.g.,
let destination = downloadDestination; defer { try?
FileManager.default.removeItem(at: destination) }) so the temp file is deleted
regardless of validation outcome, then read the file contents and perform the
extension/content checks and call model.privateKeyCaptured(pem) only if valid;
apply the same defer-based removal pattern to the other download completion
block that has the same early-return behavior so all temp downloads are always
removed.
Comment on lines
+50
to
+58
| BUILT="$SRC_DIR/.build/apple/Products/Release/$PRODUCT_NAME" | ||
| if [[ ! -f "$BUILT" ]]; then | ||
| # Fall back to the single-arch path if a universal build wasn't produced. | ||
| BUILT="$(find "$SRC_DIR/.build" -maxdepth 3 -name "$PRODUCT_NAME" -type f -perm -111 | head -1)" | ||
| fi | ||
| if [[ -z "${BUILT:-}" || ! -f "$BUILT" ]]; then | ||
| echo "error: could not find built product '$PRODUCT_NAME' under $SRC_DIR/.build" >&2 | ||
| exit 1 | ||
| fi |
Contributor
There was a problem hiding this comment.
Don't copy an arbitrary .build executable.
If the expected release binary is missing, find ... | head -1 can pick up a stale debug or single-arch artifact from a previous build. That makes the published helper nondeterministic.
♻️ Proposed fix
BUILT="$SRC_DIR/.build/apple/Products/Release/$PRODUCT_NAME"
if [[ ! -f "$BUILT" ]]; then
- # Fall back to the single-arch path if a universal build wasn't produced.
- BUILT="$(find "$SRC_DIR/.build" -maxdepth 3 -name "$PRODUCT_NAME" -type f -perm -111 | head -1)"
-fi
-if [[ -z "${BUILT:-}" || ! -f "$BUILT" ]]; then
- echo "error: could not find built product '$PRODUCT_NAME' under $SRC_DIR/.build" >&2
+ echo "error: could not find release product '$PRODUCT_NAME' at '$BUILT'" >&2
exit 1
fi
+ARCHS="$(lipo -archs "$BUILT")"
+if [[ "$ARCHS" != *arm64* || "$ARCHS" != *x86_64* ]]; then
+ echo "error: expected a universal binary, got: $ARCHS" >&2
+ exit 1
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| BUILT="$SRC_DIR/.build/apple/Products/Release/$PRODUCT_NAME" | |
| if [[ ! -f "$BUILT" ]]; then | |
| # Fall back to the single-arch path if a universal build wasn't produced. | |
| BUILT="$(find "$SRC_DIR/.build" -maxdepth 3 -name "$PRODUCT_NAME" -type f -perm -111 | head -1)" | |
| fi | |
| if [[ -z "${BUILT:-}" || ! -f "$BUILT" ]]; then | |
| echo "error: could not find built product '$PRODUCT_NAME' under $SRC_DIR/.build" >&2 | |
| exit 1 | |
| fi | |
| BUILT="$SRC_DIR/.build/apple/Products/Release/$PRODUCT_NAME" | |
| if [[ ! -f "$BUILT" ]]; then | |
| echo "error: could not find release product '$PRODUCT_NAME' at '$BUILT'" >&2 | |
| exit 1 | |
| fi | |
| ARCHS="$(lipo -archs "$BUILT")" | |
| if [[ "$ARCHS" != *arm64* || "$ARCHS" != *x86_64* ]]; then | |
| echo "error: expected a universal binary, got: $ARCHS" >&2 | |
| exit 1 | |
| fi |
🤖 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/scripts/build-asc-key-helper.sh` around lines 50 - 58, The fallback find
logic in build-asc-key-helper.sh can pick up arbitrary/stale executables;
restrict the search for BUILT to release-artifacts only by changing the fallback
that sets BUILT to use find with a path filter for release builds (e.g. -path
"*/Release/*/$PRODUCT_NAME" or -path "*/Products/Release/*/$PRODUCT_NAME"), keep
the -type f and -perm -111 checks, and then pick the first matching file; ensure
the variable names BUILT, PRODUCT_NAME and SRC_DIR are used unchanged and that
the existing existence check for BUILT remains after the new find.
Comment on lines
+44
to
+46
| log.error('Could not find the App Store Connect key helper binary.') | ||
| log.info('Set CAPGO_ASC_KEY_HELPER_PATH to a compiled helper, or upgrade to a CLI release that bundles it.') | ||
| void trackEvent({ channel: ASC_KEY_CHANNEL, event: 'ASC Key: Helper Missing', icon: '🔑', apikey: options.apikey }) |
Contributor
There was a problem hiding this comment.
Align helper-missing remediation text with the actual discovery model.
Line 45 tells users to “upgrade to a CLI release that bundles it,” but this feature’s rollout states the helper is discovered via env/cache/dev build and not bundled in npm tarballs. That guidance can send users to a dead end.
💡 Suggested wording update
- log.info('Set CAPGO_ASC_KEY_HELPER_PATH to a compiled helper, or upgrade to a CLI release that bundles it.')
+ log.info('Set CAPGO_ASC_KEY_HELPER_PATH to a compiled helper, ensure the ~/.capgo/asc-key-helper cache is populated, or run from a local dev checkout with the helper built.')🤖 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/asc-key/command.ts` around lines 44 - 46, Update the
helper-missing remediation text so it reflects the actual discovery model
instead of implying the binary is bundled in CLI releases: change the log.info
call that currently tells users to "upgrade to a CLI release that bundles it" to
instead instruct users to set CAPGO_ASC_KEY_HELPER_PATH to a compiled helper or
ensure the helper is available via their environment/cache/dev build process;
keep the log.error message and the trackEvent call (ASC_KEY_CHANNEL,
trackEvent(..., apikey: options.apikey)) unchanged.
Comment on lines
+132
to
+134
| message: 'Could not locate the App Store Connect key helper binary. ' | ||
| + 'Set CAPGO_ASC_KEY_HELPER_PATH to a compiled helper, or use a CLI ' | ||
| + 'release that bundles it.', |
Contributor
There was a problem hiding this comment.
Update HELPER_NOT_FOUND guidance to match actual distribution behavior.
The message currently suggests using “a CLI release that bundles it”, but this flow explicitly relies on env override/cache/local build instead of npm-bundled binaries.
✏️ Proposed fix
message: 'Could not locate the App Store Connect key helper binary. '
- + 'Set CAPGO_ASC_KEY_HELPER_PATH to a compiled helper, or use a CLI '
- + 'release that bundles it.',
+ + 'Set CAPGO_ASC_KEY_HELPER_PATH to a compiled helper, or install/download '
+ + 'the helper into ~/.capgo/asc-key-helper.',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| message: 'Could not locate the App Store Connect key helper binary. ' | |
| + 'Set CAPGO_ASC_KEY_HELPER_PATH to a compiled helper, or use a CLI ' | |
| + 'release that bundles it.', | |
| message: 'Could not locate the App Store Connect key helper binary. ' | |
| 'Set CAPGO_ASC_KEY_HELPER_PATH to a compiled helper, or install/download ' | |
| 'the helper into ~/.capgo/asc-key-helper.', |
🤖 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/asc-key/helper.ts` around lines 132 - 134, The error
message in HELPER_NOT_FOUND should be updated to reflect actual distribution
behavior: replace the sentence suggesting “use a CLI release that bundles it”
with guidance to set CAPGO_ASC_KEY_HELPER_PATH to a compiled helper, or
build/compile the helper locally (or install via the project’s documented
local-build/cache workflow). Edit the message string (the one currently starting
"Could not locate the App Store Connect key helper binary...") to mention
CAPGO_ASC_KEY_HELPER_PATH and local build/cache options instead of an
npm/CLI-bundled binary.
Comment on lines
+147
to
+149
| let stderr = '' | ||
| let eventCount = 0 | ||
| let logCount = 0 |
Contributor
There was a problem hiding this comment.
Bound stderr capture to prevent unbounded memory growth.
stderr is accumulated without a size cap. A noisy helper (or unexpected failure loop) can grow this buffer indefinitely before process close.
🛠️ Proposed fix
+const MAX_STDERR_CHARS = 16_384
@@
- let stderr = ''
+ let stderr = ''
@@
child.stderr?.on('data', (chunk: Buffer) => {
- stderr += chunk.toString('utf-8')
+ if (stderr.length >= MAX_STDERR_CHARS)
+ return
+ const next = stderr + chunk.toString('utf-8')
+ stderr = next.length > MAX_STDERR_CHARS ? next.slice(0, MAX_STDERR_CHARS) : next
})
@@
- : `Helper exited (code ${code}) without a result line.${stderr.trim() ? ` Stderr: ${stderr.trim()}` : ''}`)
+ : `Helper exited (code ${code}) without a result line.${stderr.trim() ? ` Stderr (truncated): ${stderr.trim()}` : ''}`)Also applies to: 184-186, 219-220
🤖 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/asc-key/helper.ts` around lines 147 - 149, The
stderr buffer is unbounded (variable stderr) and must be size-capped to prevent
memory growth: introduce a MAX_STDERR_BYTES constant and update all append sites
that modify stderr to only append up to the remaining capacity (or drop/rotate
oldest content) so stderr never exceeds MAX_STDERR_BYTES; keep eventCount and
logCount logic unchanged but ensure the same capping behavior is applied at the
other stderr accumulation locations mentioned (the other stderr append sites in
this file) so all places that write to stderr share the cap and behavior.
Comment on lines
+3
to
+5
| The `build credentials apple-key` command launches a native macOS helper (a | ||
| precompiled Swift app) that walks the user through creating an App Store Connect | ||
| **team** API key in an embedded browser, then captures the resulting credentials. |
Contributor
There was a problem hiding this comment.
Use the canonical npx @capgo/cli@latest ... form for CLI command examples.
Please render this command reference as npx @capgo/cli@latest build credentials apple-key to match repository docs conventions for customer-facing examples.
As per coding guidelines: “For Capgo CLI references in customer-facing command examples, always use npx @capgo/cli@latest ....”
🤖 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/asc-key/PROTOCOL.md` around lines 3 - 5, Update the
example CLI invocation in PROTOCOL.md to use the canonical repository form:
replace the current `build credentials apple-key` example with `npx
`@capgo/cli`@latest build credentials apple-key` so all customer-facing command
examples follow the “npx `@capgo/cli`@latest ...” convention.
Source: Coding guidelines
Comment on lines
+197
to
+205
| function coerceTagValue(value: unknown): string | number | boolean | undefined { | ||
| if (typeof value === 'string' || typeof value === 'number' || typeof value === 'boolean') | ||
| return value | ||
| if (value === null || value === undefined) | ||
| return undefined | ||
| // Flatten small structured values so they're still queryable. | ||
| try { | ||
| return JSON.stringify(value) | ||
| } |
Contributor
There was a problem hiding this comment.
Secret guard currently misses nested secret data in structured props.
Top-level key filtering is bypassed when a non-scalar prop contains nested secret-like keys/values (it gets serialized as-is), so sensitive content can still reach analytics tags and rendered log props.
🔐 Proposed fix
const SECRET_KEY_PATTERN = /private[_-]?key|secret|p8|pem|password|token/i
+const SECRET_VALUE_PATTERN = /BEGIN (?:EC |RSA )?PRIVATE KEY|PRIVATE KEY|AUTHKEY_|\.p8\b|token|password|secret/i
+
+function sanitizeUnknown(value: unknown): unknown {
+ if (typeof value === 'string')
+ return SECRET_VALUE_PATTERN.test(value) ? '[REDACTED]' : value
+ if (Array.isArray(value))
+ return value.map(item => sanitizeUnknown(item))
+ if (value && typeof value === 'object') {
+ const out: Record<string, unknown> = {}
+ for (const [key, raw] of Object.entries(value as Record<string, unknown>)) {
+ if (SECRET_KEY_PATTERN.test(key))
+ continue
+ out[key] = sanitizeUnknown(raw)
+ }
+ return out
+ }
+ return value
+}
@@
function coerceTagValue(value: unknown): string | number | boolean | undefined {
if (typeof value === 'string' || typeof value === 'number' || typeof value === 'boolean')
return value
if (value === null || value === undefined)
return undefined
// Flatten small structured values so they're still queryable.
try {
- return JSON.stringify(value)
+ return JSON.stringify(sanitizeUnknown(value))
}
catch {
return undefined
}
}
@@
for (const [key, raw] of Object.entries(line.props)) {
if (SECRET_KEY_PATTERN.test(key))
continue
- safeProps[key] = raw
+ safeProps[key] = sanitizeUnknown(raw)
}Also applies to: 222-227, 275-279
🤖 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/asc-key/protocol.ts` around lines 197 - 205, The
coerceTagValue function currently serializes non-scalar structured values
without redacting nested secret-like data, so update coerceTagValue to detect
Objects and Arrays and recursively redact any keys or values that match secret
patterns (e.g., keys like "secret", "token", "password", "apiKey" or values
starting with secret prefixes such as "sk-" or PEM markers) before calling
JSON.stringify; implement the recursion as a helper (e.g.,
redactSecretsInStructure) that returns a sanitized structure (replacing secrets
with a fixed redaction token) and call that helper from coerceTagValue for
non-scalar inputs, and apply the same helper in the other serializer sites
mentioned so nested secrets are never emitted.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
cli/src/build/onboarding/asc-key/PROTOCOL.md (1)
3-4:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the canonical customer-facing CLI invocation format.
Please render the command example as
npx@capgo/cli@latest build credentials apple-keyto match repository conventions for customer-facing docs.✏️ Proposed fix
-The `build credentials apple-key` command launches a native macOS helper (a +The `npx `@capgo/cli`@latest build credentials apple-key` command launches a native macOS helper (aAs per coding guidelines: “For Capgo CLI references in customer-facing command examples, always use
npx@capgo/cli@latest ....”🤖 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/asc-key/PROTOCOL.md` around lines 3 - 4, Update the customer-facing command example so it uses the canonical invocation format: replace occurrences of "build credentials apple-key" (the CLI example text) with the full form "npx `@capgo/cli`@latest build credentials apple-key" in PROTOCOL.md so the docs follow the repository convention for Capgo CLI examples.Source: Coding guidelines
cli/src/build/onboarding/asc-key/helper.ts (2)
132-134:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
HELPER_NOT_FOUNDguidance conflicts with actual distribution behavior.The message still suggests using a CLI release that bundles the helper, but this flow uses env override/cache/local build discovery. The remediation text should match the real install path.
✏️ Proposed fix
message: 'Could not locate the App Store Connect key helper binary. ' - + 'Set CAPGO_ASC_KEY_HELPER_PATH to a compiled helper, or use a CLI ' - + 'release that bundles it.', + + 'Set CAPGO_ASC_KEY_HELPER_PATH to a compiled helper, or install/build ' + + 'the helper into ~/.capgo/asc-key-helper.',🤖 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/asc-key/helper.ts` around lines 132 - 134, The HELPERNOTFOUND error message (the message string assigned for HELPER_NOT_FOUND) is inaccurate about distribution: update that message to reflect the actual discovery methods (environment override, cache, or local build) instead of saying "use a CLI release that bundles it"; specifically change the text to something like: "Could not locate the App Store Connect key helper binary. Set CAPGO_ASC_KEY_HELPER_PATH to a compiled helper, or place/build the helper in your project's local build/cache so the CLI can discover it." Locate and replace the existing message assigned to HELPERNOT_FOUND (or the variable containing that string) so messaging matches the env override/cache/local build discovery flow.
147-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winstderr capture is unbounded and can grow memory under noisy failures.
stderris appended without any cap. A chatty helper process can cause avoidable memory growth before close.🛠️ Proposed fix
+const MAX_STDERR_CHARS = 16_384 @@ - let stderr = '' + let stderr = '' @@ child.stderr?.on('data', (chunk: Buffer) => { - stderr += chunk.toString('utf-8') + if (stderr.length >= MAX_STDERR_CHARS) + return + const next = stderr + chunk.toString('utf-8') + stderr = next.length > MAX_STDERR_CHARS ? next.slice(0, MAX_STDERR_CHARS) : next }) @@ - : `Helper exited (code ${code}) without a result line.${stderr.trim() ? ` Stderr: ${stderr.trim()}` : ''}`) + : `Helper exited (code ${code}) without a result line.${stderr.trim() ? ` Stderr (truncated): ${stderr.trim()}` : ''}`)Also applies to: 188-190, 223-223
🤖 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/asc-key/helper.ts` around lines 147 - 149, The stderr buffer is unbounded (variable stderr) and can grow under noisy failures; define a MAX_STDERR_BYTES constant (e.g. 32KB), and when appending to stderr only keep the most recent bytes (trim the existing buffer or incoming chunk so stderr = (stderr + chunk).slice(-MAX_STDERR_BYTES)) or stop appending once the cap is reached; apply the same capped-append logic to every site that appends to stderr (the other occurrences flagged in the review) so no stderr capture grows without bound.cli/src/build/onboarding/asc-key/protocol.ts (1)
197-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNested secret data can still leak through analytics tags.
buildEventTagsstrips only top-level secret-like keys, butcoerceTagValuestringifies nested objects/arrays as-is. If a helper event prop contains nested secret-like keys/values under a safe top-level key, it can still reach PostHog.🔐 Proposed fix
const SECRET_KEY_PATTERN = /private[_-]?key|secret|p8|pem|password|token/i +const SECRET_VALUE_PATTERN = /BEGIN (?:EC |RSA )?PRIVATE KEY|AUTHKEY_|\.p8\b|token|password|secret/i + +function sanitizeUnknown(value: unknown): unknown { + if (typeof value === 'string') + return SECRET_VALUE_PATTERN.test(value) ? '[REDACTED]' : value + if (Array.isArray(value)) + return value.map(item => sanitizeUnknown(item)) + if (value && typeof value === 'object') { + const out: Record<string, unknown> = {} + for (const [key, raw] of Object.entries(value as Record<string, unknown>)) { + if (SECRET_KEY_PATTERN.test(key)) + continue + out[key] = sanitizeUnknown(raw) + } + return out + } + return value +} @@ function coerceTagValue(value: unknown): string | number | boolean | undefined { @@ try { - return JSON.stringify(value) + return JSON.stringify(sanitizeUnknown(value)) }Also applies to: 222-227
🤖 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/asc-key/protocol.ts` around lines 197 - 205, The coerceTagValue function (and the related logic around buildEventTags) currently JSON.stringify's nested objects/arrays, which allows nested secret-like keys/values to leak; modify coerceTagValue to detect objects and arrays and recursively sanitize them before stringifying: implement a helper (e.g., sanitizeForTags) that traverses objects/arrays, removes or masks keys that match secret-like patterns (password, secret, token, key, api, etc.) and replaces secret-like primitive values with a placeholder, then return JSON.stringify of the sanitized structure; ensure the function still returns primitives unchanged and undefined for null/undefined, and apply the same sanitization call sites referenced in buildEventTags so nested secrets are stripped wherever coerceTagValue was previously stringifying structures.
🤖 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/asc-key/helper.ts`:
- Around line 132-134: The HELPERNOTFOUND error message (the message string
assigned for HELPER_NOT_FOUND) is inaccurate about distribution: update that
message to reflect the actual discovery methods (environment override, cache, or
local build) instead of saying "use a CLI release that bundles it"; specifically
change the text to something like: "Could not locate the App Store Connect key
helper binary. Set CAPGO_ASC_KEY_HELPER_PATH to a compiled helper, or
place/build the helper in your project's local build/cache so the CLI can
discover it." Locate and replace the existing message assigned to
HELPERNOT_FOUND (or the variable containing that string) so messaging matches
the env override/cache/local build discovery flow.
- Around line 147-149: The stderr buffer is unbounded (variable stderr) and can
grow under noisy failures; define a MAX_STDERR_BYTES constant (e.g. 32KB), and
when appending to stderr only keep the most recent bytes (trim the existing
buffer or incoming chunk so stderr = (stderr + chunk).slice(-MAX_STDERR_BYTES))
or stop appending once the cap is reached; apply the same capped-append logic to
every site that appends to stderr (the other occurrences flagged in the review)
so no stderr capture grows without bound.
In `@cli/src/build/onboarding/asc-key/PROTOCOL.md`:
- Around line 3-4: Update the customer-facing command example so it uses the
canonical invocation format: replace occurrences of "build credentials
apple-key" (the CLI example text) with the full form "npx `@capgo/cli`@latest
build credentials apple-key" in PROTOCOL.md so the docs follow the repository
convention for Capgo CLI examples.
In `@cli/src/build/onboarding/asc-key/protocol.ts`:
- Around line 197-205: The coerceTagValue function (and the related logic around
buildEventTags) currently JSON.stringify's nested objects/arrays, which allows
nested secret-like keys/values to leak; modify coerceTagValue to detect objects
and arrays and recursively sanitize them before stringifying: implement a helper
(e.g., sanitizeForTags) that traverses objects/arrays, removes or masks keys
that match secret-like patterns (password, secret, token, key, api, etc.) and
replaces secret-like primitive values with a placeholder, then return
JSON.stringify of the sanitized structure; ensure the function still returns
primitives unchanged and undefined for null/undefined, and apply the same
sanitization call sites referenced in buildEventTags so nested secrets are
stripped wherever coerceTagValue was previously stringifying structures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1d328253-9845-4dc4-add9-fe53800d103b
📒 Files selected for processing (6)
cli/src/build/onboarding/asc-key/PROTOCOL.mdcli/src/build/onboarding/asc-key/helper.tscli/src/build/onboarding/asc-key/protocol.tscli/src/support/redact.tscli/test/test-asc-key-protocol.mjscli/test/test-support-redact.mjs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Cap-go/capacitor-updater(manual)
💤 Files with no reviewable changes (1)
- cli/test/test-asc-key-protocol.mjs
…ailable The 'Do you already have a .p8 file?' question only changes the path when the guided macOS helper can be offered. On any non-automatable host (non-macOS, or macOS without the helper binary) both answers funnel to the same manual api-key-instructions step, so the prompt is a dead click. Gate the entry on the same canAutomate condition that drives the fork: from setup-method-select 'create', go to p8-source-select only when isMacOS() && resolveHelperBinary() !== null, else straight to api-key-instructions (the exact pre-fork behaviour). No new dead-ends; the question now appears only where answering it matters.
…n match Two issues on the 'Open the Generate dialog' step: the red highlight lands off the '+' button and drifts when the page scrolls. - Harden the overlay rAF loop: wrap each per-target tick in try/catch. A finder that throws used to kill the whole loop, freezing every overlay at its last viewport position — which then visibly drifts away from its target as the page scrolls. The loop now keeps ticking regardless. - Add a createKey highlight probe (logged to the support bundle via the new log protocol): records what the 'Active heading → following button' finder matches (tag/text/aria/rect/visibility), nearby candidate +/Generate buttons with positions, and whether <body>/<html> carry a CSS transform (which silently re-bases a position:fixed overlay and is a prime suspect for the scroll drift). Lets us fix the finder/positioning precisely from a bundle, no live DOM.
…picker When a user chose the guided macOS helper to create their .p8, quit onboarding, and restarted, resume dropped them on the manual 'do you have a .p8?' / api-key instructions — losing the path they deliberately picked. Persist the fork choice (new OnboardingProgress.p8CreateMethod: automated|manual) at p8-source-select / p8-create-method-select, and route on it in getResumeStep: create-new + automated + no .p8 inputs → asc-key-generating (re-launches the guided window); manual / legacy → api-key-instructions (unchanged). Re-entering the create fork clears a stale choice so a later switch can't mis-route. Also persist p8Path on the helper's success (not just keyId/issuerId), so a resume AFTER the key was captured lands on verifying-key instead of needlessly re-running the helper. Tests: getResumeStep coverage for automated/manual/legacy + the post-capture verifying-key precedence.
|
…overlay For the static 'Open the Generate dialog' '+' button, stop floating a fixed overlay div (which mis-placed the ring and drifted on scroll) and instead draw the ring ON the real button via inline box-shadow + outline. Safe for React: we only set inline style on an EXISTING node and never insert or remove child nodes — the 'removeChild must be an instance of Node' crash came from node insertion, not from styling an existing element. box-shadow/outline paint outside the box so there's no reflow, and because the style lives on the element it tracks scroll/layout natively (no rAF, no drift). A 200ms interval re-applies if a re-render clears it and restores the element's style on teardown. Scope: createKey only. The re-rendering dropdowns (selectRole etc.) keep the overlay, which deliberately never touches their nodes.
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
3 participants
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.
What & why
Two things:
cli/native/asc-key-helper/(a SwiftPM package, productP8Extract) so it builds and ships with the CLI instead of living in a separate repo.build initiOS onboarding as the.p8entry fork, and forwards the helper's usage statistics to PostHog over a stdout protocol.The helper is a guided macOS app: it walks the user through creating an App Store Connect team API key in an embedded browser, auto-captures the Issuer ID + Key ID, intercepts the one-time
.p8, validates it against Apple, and saves it to~/.appstoreconnect/private_keys/— no copy-pasting credentials.The onboarding UX (the new path)
At the App Store Connect API-key step of
build init(iOS), the wizard now asks:The automated path (
asc-key-generatingstep) spawns the helper, streams its stats to PostHog, captureskeyId/issuerId/.p8, then converges on the existingverifying-key→ save flow — so the rest of onboarding (cert/profile/CI secrets/build) is untouched. On non-macOS or when the binary isn't present, the "No" option falls back to the manual instructions. New steps:p8-source-select,p8-create-method-select,asc-key-generating.Also adds a standalone entry:
build credentials apple-key(aliasasc-key).The stdout stats protocol (helper ⇄ CLI ⇄ PostHog)
Newline-delimited JSON on stdout, every line tagged
capgoAscKey:1so chatter is ignored.eventlines are forwarded to PostHog (channelapp-store-connect-key); the terminalresultline delivers the credentials. Diagnostics stay on stderr. Full spec:cli/native/asc-key-helper/↔cli/src/build/onboarding/asc-key/PROTOCOL.md.{"capgoAscKey":1,"kind":"event","ts":12,"runId":"…","name":"step_changed","props":{"from":"login","to":"verifyAccess","elapsed_ms_on_prev":340}} {"capgoAscKey":1,"kind":"result","ts":900,"runId":"…","ok":true,"keyId":"…","issuerId":"…","privateKey":"…"}Events:
helper_started,signed_in,team_confirmed,api_access_checked/api_access_denied,step_changed(per-step timing),validation_started/succeeded/failed,helper_finished.Security
resultline and is never forwarded to analytics.private_key|secret|p8|pem|password|tokenbefore building PostHog tags (tested).CAPGO_DISABLE_TELEMETRY/CAPGO_DISABLE_POSTHOG.Layout
cli/native/asc-key-helper/README+ license)cli/src/build/onboarding/asc-key/protocol.tscli/src/build/onboarding/asc-key/helper.tstrackEvent, return credentialscli/src/build/onboarding/asc-key/command.tsbuild credentials apple-keyactioncli/src/build/onboarding/asc-key/PROTOCOL.mdcli/src/build/onboarding/ui/steps/ios-credentials.tsxP8SourceSelectStep/P8CreateMethodSelectStep/AscKeyGeneratingStepcli/src/build/onboarding/ui/app.tsx,types.tscli/scripts/build-asc-key-helper.shcli/test/test-asc-key-protocol.mjsDistribution
The macOS-only binary is not bundled in the npm tarball (avoids bloating Linux/Windows installs). The CLI locates it at runtime via:
CAPGO_ASC_KEY_HELPER_PATH→~/.capgo/asc-key-helper/cache → a localswift buildof the in-repo package (dev). Build withscripts/build-asc-key-helper.sh.Test plan
swift buildoncli/native/asc-key-helper— compiles.tsc --noEmit— 0 errors (resolves the full onboarding UI graph).oxlint(repo linter) — clean across all changed files.bun run test:asc-key-protocol— 9/9 (parse, streaming reassembly, flush, event→track mapping, secret stripping).build initrun: pick "No → Automated", complete the guided window, confirm the key flows into the existing save/build path and PostHog events land. (Needs a real Apple login + GUI.)Summary by CodeRabbit
New Features
Documentation
Tests