Repository navigation
feat(desktop): allowlist /auth/passkey-register-external for system-browser handoff - #1008
Merged
Merged
Conversation
Add failing tests for /auth/passkey-register-external covering app-origin (https + localhost), third-party rejection, lookalike-host rejection, and http-on-non-localhost rejection. Existing /auth/passkey-external and OAuth provider regression tests continue to pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Refactor the single PASSKEY_EXTERNAL_PATH constant into an ALLOWED_PASSKEY_PATHS set so both /auth/passkey-external (sign-in handoff) and /auth/passkey-register-external (Settings add-passkey handoff) pass the auth:open-external check. Origin matching, OAuth provider branch, and isAppOriginMatch rules are unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/main/ipc-handlers.ts (1)
110-113: Consider centralizing passkey handoff path constants in a shared module.These path literals now live in desktop and web code, which can drift over time. Moving them to a shared constant source would reduce future allowlist mismatches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/ipc-handlers.ts` around lines 110 - 113, ALLOWED_PASSKEY_PATHS is defined locally in ipc-handlers.ts causing duplicate literals across desktop and web; extract these path strings into a shared constants module (e.g., export a single PASSKEY_HANDOFF_PATHS or ALLOWED_PASSKEY_PATHS constant) and replace the local ReadonlySet initialization in ipc-handlers.ts with an import from that shared module, then update the web code to import the same constant so both consumers reference the single source of truth; preserve the same type/shape (ReadonlySet<string> or equivalent) and update any tests or usages that reference the original symbol name to the new shared export.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/main/ipc-handlers.ts`:
- Around line 110-113: ALLOWED_PASSKEY_PATHS is defined locally in
ipc-handlers.ts causing duplicate literals across desktop and web; extract these
path strings into a shared constants module (e.g., export a single
PASSKEY_HANDOFF_PATHS or ALLOWED_PASSKEY_PATHS constant) and replace the local
ReadonlySet initialization in ipc-handlers.ts with an import from that shared
module, then update the web code to import the same constant so both consumers
reference the single source of truth; preserve the same type/shape
(ReadonlySet<string> or equivalent) and update any tests or usages that
reference the original symbol name to the new shared export.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48c88125-4526-4f12-92a3-876310eeb84e
📒 Files selected for processing (2)
apps/desktop/src/main/__tests__/ipc-handlers.test.tsapps/desktop/src/main/ipc-handlers.ts
Owner
Author
Declining this refactor for this PR, intentionally:
Leaving the structural refactor for a follow-up once the web-side |
2witstudios
added a commit
that referenced
this pull request
Apr 14, 2026
Tests for the integration layer that wires the foundation PRs (#1008, #1009, #1010, #1011) into a working end-to-end flow for adding a passkey from the Settings → Security panel on desktop. Fails until the routes, ceremony helper, external page, and PasskeyManager desktop branch land in the next commit. - handoff mint route: session auth + CSRF + rate limit, mints handoffToken via createPasskeyRegisterHandoff and audits auth.token.created - register/options handoffToken branch: peek (non-destructive), skips session auth + CSRF, 401 HANDOFF_INVALID + audit on miss, rate limit bound to resolved userId, session path regression - register verify handoffToken branch: consume (one-time-use), second call rejected, rate limit bound to resolved userId, session path regression - runPasskeyRegisterExternalCeremony: options → startRegistration → verify → pagespace://passkey-registered, NotAllowedError → cancelled result - PasskeyManager desktop branch: update-prompt when bridge missing, POST handoff → openExternal with /auth/passkey-register-external URL, no in-window startRegistration, onRegistered → mutate('/api/auth/passkey') + toast, unsubscribe on unmount, web regression Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2witstudios
added a commit
that referenced
this pull request
Apr 14, 2026
Wires the merged foundation PRs (#1008, #1009, #1010, #1011) into a working end-to-end flow so Electron users can add a passkey from Settings → Security. Electron's Chromium cannot drive Touch ID / Windows Hello without entitlements we don't ship, so the ceremony is delegated to the system browser via the same `shell.openExternal` + `pagespace://` deep-link pattern PR #1001 used for sign-in. New: - POST /api/auth/passkey/register/handoff — authenticated mint endpoint for one-time handoff tokens (session auth + CSRF + per-user rate limit). - apps/web/src/components/auth/runPasskeyRegisterExternalCeremony.ts — options → startRegistration → verify → pagespace://passkey-registered. - apps/web/src/app/auth/passkey-register-external/page.tsx — system-browser landing page that runs the ceremony and deep-links back to the desktop app. Modified in place (mirroring authenticate/route.ts desktopExchange branch): - register/options/route.ts — handoffToken branch uses peekPasskeyRegisterHandoff (non-destructive; verify step still needs the token), skips session + CSRF, 401 HANDOFF_INVALID on miss, rate limit bound to resolved userId. - register/route.ts — handoffToken branch uses consumePasskeyRegisterHandoff (atomic one-time-use), 401 on miss, same rate limit binding. - PasskeyManager.tsx — desktop branch in handleRegister: update-prompt guard when bridge missing, POST handoff → openExternal with /auth/passkey-register- external URL, no in-window startRegistration. Deep-link subscription via window.electron.passkey.onRegistered refreshes the passkey list and toasts on return. Web path unchanged. Completes the Passkey Add Desktop Fix epic (tasks/passkey-add-desktop-fix.md). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
10 tasks
2witstudios
added a commit
that referenced
this pull request
Apr 14, 2026
* test(auth): RED — passkey add-from-settings desktop integration Tests for the integration layer that wires the foundation PRs (#1008, #1009, #1010, #1011) into a working end-to-end flow for adding a passkey from the Settings → Security panel on desktop. Fails until the routes, ceremony helper, external page, and PasskeyManager desktop branch land in the next commit. - handoff mint route: session auth + CSRF + rate limit, mints handoffToken via createPasskeyRegisterHandoff and audits auth.token.created - register/options handoffToken branch: peek (non-destructive), skips session auth + CSRF, 401 HANDOFF_INVALID + audit on miss, rate limit bound to resolved userId, session path regression - register verify handoffToken branch: consume (one-time-use), second call rejected, rate limit bound to resolved userId, session path regression - runPasskeyRegisterExternalCeremony: options → startRegistration → verify → pagespace://passkey-registered, NotAllowedError → cancelled result - PasskeyManager desktop branch: update-prompt when bridge missing, POST handoff → openExternal with /auth/passkey-register-external URL, no in-window startRegistration, onRegistered → mutate('/api/auth/passkey') + toast, unsubscribe on unmount, web regression Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(auth): GREEN — passkey add-from-settings desktop integration Wires the merged foundation PRs (#1008, #1009, #1010, #1011) into a working end-to-end flow so Electron users can add a passkey from Settings → Security. Electron's Chromium cannot drive Touch ID / Windows Hello without entitlements we don't ship, so the ceremony is delegated to the system browser via the same `shell.openExternal` + `pagespace://` deep-link pattern PR #1001 used for sign-in. New: - POST /api/auth/passkey/register/handoff — authenticated mint endpoint for one-time handoff tokens (session auth + CSRF + per-user rate limit). - apps/web/src/components/auth/runPasskeyRegisterExternalCeremony.ts — options → startRegistration → verify → pagespace://passkey-registered. - apps/web/src/app/auth/passkey-register-external/page.tsx — system-browser landing page that runs the ceremony and deep-links back to the desktop app. Modified in place (mirroring authenticate/route.ts desktopExchange branch): - register/options/route.ts — handoffToken branch uses peekPasskeyRegisterHandoff (non-destructive; verify step still needs the token), skips session + CSRF, 401 HANDOFF_INVALID on miss, rate limit bound to resolved userId. - register/route.ts — handoffToken branch uses consumePasskeyRegisterHandoff (atomic one-time-use), 401 on miss, same rate limit binding. - PasskeyManager.tsx — desktop branch in handleRegister: update-prompt guard when bridge missing, POST handoff → openExternal with /auth/passkey-register- external URL, no in-window startRegistration. Deep-link subscription via window.electron.passkey.onRegistered refreshes the passkey list and toasts on return. Web path unchanged. Completes the Passkey Add Desktop Fix epic (tasks/passkey-add-desktop-fix.md). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(auth): address PR #1012 review feedback — fragment handoff, 400 on parse Addresses Codex and CodeRabbit review feedback on PR #1012: **Security — move handoff token out of the query string** (CodeRabbit Major on passkey-register-external/page.tsx) - buildPasskeyRegisterExternalUrl now places handoffToken in the URL fragment (`#handoffToken=...`), never the query. Fragments never hit the network, server access logs, CDN logs, or the Referer header, so the capability token no longer leaks anywhere a log scraper or a same-origin fetch could see it. - parsePasskeyRegisterExternalParams takes an optional `hash` argument and reads the token from there; deviceId/deviceName remain in the query. - External page clears the fragment via history.replaceState after reading, so the token is not left sitting in the browser address bar. - Round-trip and missing-field tests updated to cover the new transport. **Robustness — request body parsing** (Codex P2 + CodeRabbit Minor) - register/route.ts wraps `await req.json()` in try/catch and returns a controlled 400 on malformed bodies instead of falling to the outer catch and emitting internal-error noise to unauthenticated callers. - register/options/route.ts readOptionalJson normalizes non-plain-object parses (null, arrays, primitives) to `{}` so `body.handoffToken` access never throws. Regression tests for `'null'` and malformed bodies confirm the session-auth path still runs. **Robustness — unhandled promise on external page** (Codex P2 on passkey-register-external/page.tsx) - Effect now attaches a .catch that surfaces the error to the status state instead of leaving the spinner stuck if runPasskeyRegisterExternalCeremony rejects (e.g. browser offline, fetch throw). **Cleanup — ceremony helper typing** (CodeRabbit Nit on runPasskeyRegisterExternalCeremony.ts) - Derived `RegisterOptionsJSON` from `Parameters<typeof startRegistration>` and typed the parsed options directly so startRegistration no longer needs a double `as unknown as` cast. **Test hygiene** (CodeRabbit Nit on PasskeyManager.desktop.test.tsx) - Replaced a raw `setTimeout(10)` with a `waitFor` on an observable assertion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(auth): tighten Bearer check + surface parse errors in options route Addresses a second round of review findings on PR #1012: **CSRF bypass must require the Bearer scheme, not any Authorization header** The three routes touched by this PR (`register/handoff`, `register/options`, `register`) previously skipped CSRF whenever `authorization` was present, which would let a non-Bearer scheme (`Basic`, `Digest`, random garbage) bypass CSRF even though `authenticateSessionRequest` would reject it upstream. Now each route uses `getBearerToken(req)` (already the canonical scheme check in `@/lib/auth`, previously private) and only skips CSRF when the header actually carries a `Bearer ` prefix. `getBearerToken` is now exported as a named symbol. Regression tests added to all three route suites: an `Authorization: Basic …` header with a bad `x-csrf-token` must still return 403. **`options/route.ts` must surface malformed JSON as 400, not fall through** `readOptionalJson` previously caught `JSON.parse` errors and returned `{}`, which meant a syntactically invalid body would silently engage the session path instead of returning a controlled 400 (mismatched with `register/route.ts` behavior). The helper no longer catches `SyntaxError`; the caller wraps the call in a try/catch and returns a 400 `{ error: 'Invalid request body' }`. Empty body still resolves to `{}` (so callers without a body still reach the session path), and non-object JSON (`null`, arrays, primitives) still normalizes to `{}`. The existing "treats malformed JSON as empty body and falls through" test is inverted to "returns 400 (not 500, not session fallthrough) when body is malformed JSON" and now also asserts `authenticateSessionRequest` is never called. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 of 7 tasks
This branch was previously deployed
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
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.
Epic sub-task: Electron Open-External Allowlist (
tasks/passkey-add-desktop-fix.md).Extends the
auth:open-externalIPC allowlist so the Settings → Security add-passkey flow can hand the ceremony off to the system browser the same way sign-in already does (PRs #1001, #1003, #1004). Converts the one-offPASSKEY_EXTERNAL_PATHconstant into anALLOWED_PASSKEY_PATHSset so future passkey flows don't require another one-off edit. Origin matching, OAuth provider branch, andisAppOriginMatchrules are untouched.TDD trail
/auth/passkey-register-externalRequirement checklist
/auth/passkey-register-externalon app origin (https) → allowed/auth/passkey-register-externalon localhost http app origin → allowed/auth/passkey-externalon app origin → still allowed (regression)/dashboardand other app-origin paths → blocked (regression)https://accounts.google.com/...→ still allowed (regression)https://appleid.apple.com/...→ still allowed (regression)/auth/passkey-register-externalon third-party origin → blockedpagespace.ai.evil.com→ blockedhttp://on non-localhost for either passkey path → blockedScope guardrails
apps/desktop/src/main/ipc-handlers.ts(+ its colocated test) modified.ALLOWED_AUTH_HOSTNAMESandisAppOriginMatchuntouched — regression tests cover Google, Apple, lookalike hosts, http-on-non-localhost, and the existing/auth/passkey-externalsign-in path to prove it.Test plan
pnpm --filter desktop typecheckpnpm --filter desktop test— 114/114 passing (21 inipc-handlers.test.ts, including 5 new/auth/passkey-register-externalcases)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests