Fix macOS release launch entitlement#252
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
📝 WalkthroughWalkthroughThis PR implements an environment variable gate for Touch ID WebAuthn on macOS. The implementation adds a conditional check that reads ChangesTouch ID WebAuthn Environment Gate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } else if (process.platform === "darwin") { | ||
| logger()?.debug("built_in_browser.webauthn_touchid_disabled", { | ||
| reason: "missing_provisioned_keychain_access_group", | ||
| keychainAccessGroup: BUILT_IN_BROWSER_WEBAUTHN_KEYCHAIN_ACCESS_GROUP, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The reason field says "missing_provisioned_keychain_access_group", but the access group constant is still defined in the code — the feature is disabled intentionally via the absent env var. A reader looking at this log in production would search for a provisioning issue that doesn't exist. A value like "touch_id_not_enabled" or "env_var_not_set" more accurately describes why this path was taken.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/builtInBrowser/builtInBrowserWebAuthn.ts
Line: 46-51
Comment:
**Misleading debug log reason string**
The `reason` field says `"missing_provisioned_keychain_access_group"`, but the access group constant is still defined in the code — the feature is disabled intentionally via the absent env var. A reader looking at this log in production would search for a provisioning issue that doesn't exist. A value like `"touch_id_not_enabled"` or `"env_var_not_set"` more accurately describes why this path was taken.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Summary
Validation
Release note
Desktop release pipeline fix only; no changelog update needed.
Summary by CodeRabbit
Greptile Summary
This PR fixes the macOS Developer ID release pipeline by removing an unprovisioned
keychain-access-groupsentitlement from the plist and gating theapp.configureWebAuthn()Touch ID call behind an explicitADE_ENABLE_TOUCH_ID_WEBAUTHNenv var. The built-in browser session WebAuthn account-chooser (theselect-webauthn-accounthandler) is preserved unchanged.Confidence Score: 4/5
Safe to merge — all findings are P2; the fix is targeted and the core WebAuthn session handler is untouched.
No P0 or P1 issues found. Three P2 observations: a misleading debug log reason string, a silent-failure risk when the env flag is set in a build without the entitlement, and a platform-guarded test assertion that is skipped on Linux CI. None of these block the stated release-pipeline fix.
Minor attention to the env-var/entitlement pairing across entitlements.mac.plist and builtInBrowserWebAuthn.ts.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[configureBuiltInBrowserWebAuthn called] --> B{already configured?} B -- yes --> Z[return] B -- no --> C[set configured = true] C --> D{platform === darwin?} D -- no --> G D -- yes --> E{ADE_ENABLE_TOUCH_ID_WEBAUTHN set?} E -- yes --> F[app.configureWebAuthn with keychainAccessGroup] F --> F1{success?} F1 -- yes --> G F1 -- no --> F2[log warn: webauthn_configure_failed] --> G E -- no --> H[log debug: webauthn_touchid_disabled] --> G G[session.fromPartition 'persist:ade-browser'] G --> I[register select-webauthn-account handler]Comments Outside Diff (2)
apps/desktop/src/main/services/builtInBrowser/builtInBrowserWebAuthn.test.ts, line 102-115 (link)The
expect(fakes.app.configureWebAuthn).toHaveBeenCalledWith(…)assertion is insideif (process.platform === "darwin"), so on any Linux runner only theelsebranch executes — the critical check that the rightkeychainAccessGroupis passed is skipped. The module mock already stubselectron, so the assertion is safe to run unconditionally on all platforms. Removing the platform guard would give full coverage everywhere.Prompt To Fix With AI
apps/desktop/build/entitlements.mac.plist, line 1-10 (link)The
keychain-access-groupsentitlement was removed, but the runtime code still accepts the env flag to callapp.configureWebAuthn(). In a Developer ID-signed build the OS will reject the keychain access group request because the entitlement is absent; the try/catch swallows the error and only logs a warning, so a developer who sets the flag expecting Touch ID to work will see silent failure. Consider adding a comment in the plist (or in the source) noting that the entitlement must be re-added alongside the feature flag, or remove the env-var path entirely until the entitlement is properly provisioned.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix: remove unprovisioned mac keychain e..." | Re-trigger Greptile