Conversation
|
Cursor Agent can help with this pull request. Just |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDetects browser WebAuthn autofill on the login page, orchestrates conditional passkey authentication (start/cancel/cleanup), requires resident credentials server-side, clears legacy login session data during verify, updates UI/autocomplete and success messaging, and adds an E2E Playwright test for passkey autofill sign-in. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant LoginPage as Login Page
participant WebAuthnAPI as WebAuthn API
participant Server
participant LoginSession as Login Session
Browser->>LoginPage: Load login page
LoginPage->>WebAuthnAPI: Check autofill support
WebAuthnAPI-->>LoginPage: Autofill supported
LoginPage->>Server: Request authentication options (residentKey required)
Server-->>LoginPage: Return options
LoginPage->>WebAuthnAPI: Start conditional authentication (autofill)
WebAuthnAPI-->>Browser: Show conditional UI / autofill prompt
Browser->>WebAuthnAPI: User selects passkey → assertion
WebAuthnAPI-->>LoginPage: Assertion returned
LoginPage->>Server: Submit assertion for verification
Server->>LoginSession: Clear legacy login info (clean)
LoginSession-->>Server: Confirm cleaned
Server-->>LoginPage: Verification success + merged headers
LoginPage->>Browser: Navigate to user account
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/routes/login.tsx`:
- Around line 246-259: In the catch block for the autofill/passkey flow, when an
unexpected error occurs (i.e., not NotAllowedError or AbortError) clear the
lingering passkey message and restore the form state so the user can recover:
inside the existing catch (after the isMounted check and excluding the
NotAllowedError/AbortError branch) call the state setters to clear
passkeyMessage (e.g., setPasskeyMessage('') or null), set a visible error (e.g.,
setLoginError(e.message || 'Passkey sign-in failed')), and re-enable the
form/submission state (e.g., setIsSubmitting(false) or setFormDisabled(false))
so the UI is no longer inert; keep these updates guarded by isMounted.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/routes/login.tsxapp/routes/resources/webauthn/verify-authentication.tsapp/utils/webauthn.server.tse2e/passkey-form-autofill.spec.ts
| } catch (e) { | ||
| if (!isMounted) return | ||
|
|
||
| // Autofill flow should fail silently when the user cancels or chooses a | ||
| // password instead. | ||
| if ( | ||
| e instanceof Error && | ||
| (e.name === 'NotAllowedError' || e.name === 'AbortError') | ||
| ) { | ||
| return | ||
| } | ||
|
|
||
| console.error(e) | ||
| } |
There was a problem hiding this comment.
Clear the overlay on unexpected autofill failures.
For non-NotAllowedError/AbortError, passkeyMessage stays set and the form remains inert. Clear the message (and surface an error) so users can recover.
🐛 Proposed fix
if (
e instanceof Error &&
(e.name === 'NotAllowedError' || e.name === 'AbortError')
) {
return
}
console.error(e)
+ setPasskeyMessage(null)
+ setError(
+ e instanceof Error ? e.message : 'Failed to authenticate with passkey',
+ )
}
}📝 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.
| } catch (e) { | |
| if (!isMounted) return | |
| // Autofill flow should fail silently when the user cancels or chooses a | |
| // password instead. | |
| if ( | |
| e instanceof Error && | |
| (e.name === 'NotAllowedError' || e.name === 'AbortError') | |
| ) { | |
| return | |
| } | |
| console.error(e) | |
| } | |
| } catch (e) { | |
| if (!isMounted) return | |
| // Autofill flow should fail silently when the user cancels or chooses a | |
| // password instead. | |
| if ( | |
| e instanceof Error && | |
| (e.name === 'NotAllowedError' || e.name === 'AbortError') | |
| ) { | |
| return | |
| } | |
| console.error(e) | |
| setPasskeyMessage(null) | |
| setError( | |
| e instanceof Error ? e.message : 'Failed to authenticate with passkey', | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/routes/login.tsx` around lines 246 - 259, In the catch block for the
autofill/passkey flow, when an unexpected error occurs (i.e., not
NotAllowedError or AbortError) clear the lingering passkey message and restore
the form state so the user can recover: inside the existing catch (after the
isMounted check and excluding the NotAllowedError/AbortError branch) call the
state setters to clear passkeyMessage (e.g., setPasskeyMessage('') or null), set
a visible error (e.g., setLoginError(e.message || 'Passkey sign-in failed')),
and re-enable the form/submission state (e.g., setIsSubmitting(false) or
setFormDisabled(false)) so the UI is no longer inert; keep these updates guarded
by isMounted.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/routes/login.tsx (1)
277-321:⚠️ Potential issue | 🟠 MajorAdd explicit
WebAuthnAbortService.cancelCeremony()call at the start ofhandlePasskeyLogin().The conditional UI autofill ceremony (line 223) may still be active when the user clicks the manual passkey login button. Starting a new
startAuthentication()ceremony while the autofill ceremony is pending can triggerNotAllowedError/InvalidStateErroron some browsers. While the library auto-cancels prior ceremonies when you callstartAuthentication()again, explicitly cancelling before the manual flow ensures the collision is prevented.🛠️ Proposed fix
async function handlePasskeyLogin() { try { + WebAuthnAbortService.cancelCeremony() setError(undefined) setPasskeyMessage('Generating Authentication Options')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/login.tsx` around lines 277 - 321, The manual passkey flow can collide with a running autofill ceremony; call WebAuthnAbortService.cancelCeremony() at the start of handlePasskeyLogin() before any startAuthentication() invocation (i.e., as the first action inside handlePasskeyLogin), so any pending autofill ceremony is explicitly canceled; wrap the cancel call appropriately (await or handle its promise/rejections) before proceeding to setPasskeyMessage, fetch authentication options, or call startAuthentication().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/routes/login.tsx`:
- Around line 277-321: The manual passkey flow can collide with a running
autofill ceremony; call WebAuthnAbortService.cancelCeremony() at the start of
handlePasskeyLogin() before any startAuthentication() invocation (i.e., as the
first action inside handlePasskeyLogin), so any pending autofill ceremony is
explicitly canceled; wrap the cancel call appropriately (await or handle its
promise/rejections) before proceeding to setPasskeyMessage, fetch authentication
options, or call startAuthentication().
cursor
bot
force-pushed
the
cursor/passkey-welcome-back-1d0f
branch
from
8ebdc4b to
4e1e015
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/routes/login.tsx (1)
195-327:⚠️ Potential issue | 🟠 MajorPrevent autofill ceremonies from interfering with manual passkey login.
Only one
navigator.credentials.get()request can be outstanding at a time. If the user clicks "Login with Passkey" while the autofill ceremony is pending, the autofill flow can still callstartAuthentication()after the manual flow has begun, causing a hard browser error (Chromium) or race condition failure (iOS). The currentWebAuthnAbortService.cancelCeremony()call inhandlePasskeyLogin()may not be sufficient on iOS, where aborting and immediately starting a new ceremony can fail even after abort completes.Add a shared guard (e.g.,
autofillCancelledRef) that preventssetupPasskeyAutofill()from callingstartAuthentication()once manual login starts:🔧 Suggested guard to prevent overlapping ceremonies
const [passkeyAutofillSupported, setPasskeyAutofillSupported] = React.useState(false) +const autofillCancelledRef = React.useRef(false) React.useEffect(() => { let isMounted = true + autofillCancelledRef.current = false async function setupPasskeyAutofill() { try { const supports = await browserSupportsWebAuthnAutofill() if (!supports) return - if (!isMounted) return + if (!isMounted || autofillCancelledRef.current) return setPasskeyAutofillSupported(true) const optionsResponse = await fetch( '/resources/webauthn/generate-authentication-options', { method: 'POST' }, ) + if (!isMounted || autofillCancelledRef.current) return const json = await optionsResponse.json() const { options } = AuthenticationOptionsSchema.parse(json) + if (!isMounted || autofillCancelledRef.current) return const authResponse = await startAuthentication({ optionsJSON: options, useBrowserAutofill: true, }) @@ return () => { isMounted = false + autofillCancelledRef.current = true WebAuthnAbortService.cancelCeremony() } }, [navigate, revalidate]) async function handlePasskeyLogin() { try { + autofillCancelledRef.current = true // Avoid collisions with a pending conditional UI ceremony. WebAuthnAbortService.cancelCeremony()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/login.tsx` around lines 195 - 327, The autofill and manual passkey flows can race because both call startAuthentication(); add a shared guard to prevent the autofill flow from proceeding once manual login begins: create a React ref (e.g., autofillCancelledRef = React.useRef(false)) and check it inside setupPasskeyAutofill before calling startAuthentication (and again after any await that could allow the manual flow to start), and in handlePasskeyLogin set autofillCancelledRef.current = true immediately before calling WebAuthnAbortService.cancelCeremony() so the autofill path returns early and never invokes startAuthentication; keep existing cancelCeremony calls and unmount cleanup but use this guard to avoid overlapping ceremonies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/routes/login.tsx`:
- Around line 195-327: The autofill and manual passkey flows can race because
both call startAuthentication(); add a shared guard to prevent the autofill flow
from proceeding once manual login begins: create a React ref (e.g.,
autofillCancelledRef = React.useRef(false)) and check it inside
setupPasskeyAutofill before calling startAuthentication (and again after any
await that could allow the manual flow to start), and in handlePasskeyLogin set
autofillCancelledRef.current = true immediately before calling
WebAuthnAbortService.cancelCeremony() so the autofill path returns early and
never invokes startAuthentication; keep existing cancelCeremony calls and
unmount cleanup but use this guard to avoid overlapping ceremonies.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/routes/login.tsxapp/routes/resources/webauthn/verify-authentication.tsapp/utils/webauthn.server.tse2e/passkey-form-autofill.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/routes/resources/webauthn/verify-authentication.ts
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/routes/login.tsx (1)
311-314:⚠️ Potential issue | 🟡 MinorIncomplete type union — success branch is missing from the cast.
The type assertion only covers the error case. TypeScript treats
verificationJson.statusas the literal'error', makingverificationJson.status === 'error'trivially true, which can suppress type-checker warnings and mislead future editors. The equivalent cast in the autofill flow (Lines 243–245) correctly includes both union members.🐛 Proposed fix
- const verificationJson = (await verificationResponse.json()) as { - status: 'error' - error: string - } + const verificationJson = (await verificationResponse.json()) as + | { status: 'success' } + | { status: 'error'; error: string }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/login.tsx` around lines 311 - 314, The type assertion for verificationJson (the result of verificationResponse.json()) only covers the error shape and forces verificationJson.status to be the literal 'error'; update the cast to the correct union that includes both the error and success shapes (like the autofill flow's union) so TypeScript sees verificationJson.status as 'error' | 'ok' (and include any success fields expected), then adjust downstream code to use the proper discriminated-union checks on verificationJson.status.
🧹 Nitpick comments (1)
app/routes/login.tsx (1)
218-222: Addresponse.okguards before parsing JSON in both flows.Both the autofill effect (
optionsResponseat Line 218 andverificationResponseat Line 234) andhandlePasskeyLogin(Line 290, Line 302) parse response JSON unconditionally. When the server returns 4xx/5xx, the body won't match the expected shape —AuthenticationOptionsSchema.parsewill throw a Zod validation error (e.g., "Expected string, received undefined") that surfaces as a confusing user-visible message.🔧 Proposed fix (applies identically to all four fetch call sites)
const optionsResponse = await fetch( '/resources/webauthn/generate-authentication-options', { method: 'POST' }, ) + if (!optionsResponse.ok) { + throw new Error('Failed to generate authentication options') + } const json = await optionsResponse.json()const verificationResponse = await fetch( '/resources/webauthn/verify-authentication', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(authResponse), }, ) + if (!verificationResponse.ok) { + throw new Error('Failed to verify passkey') + }Apply the same two patches to the corresponding fetch calls in
handlePasskeyLogin(Lines 290 and 302).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/login.tsx` around lines 218 - 222, The fetch calls currently parse JSON unconditionally which can feed invalid shapes into AuthenticationOptionsSchema.parse and surface confusing Zod errors; update the four call sites (the autofill effect using optionsResponse and verificationResponse, and inside handlePasskeyLogin) to check response.ok before calling response.json(), and on non-ok either throw or return a handled error (e.g., read text or json for error details and surface a clear message) so AuthenticationOptionsSchema.parse only receives valid bodies; ensure you adjust the branches that call AuthenticationOptionsSchema.parse (or subsequent logic) to run only when the response was ok.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/routes/login.tsx`:
- Around line 311-314: The type assertion for verificationJson (the result of
verificationResponse.json()) only covers the error shape and forces
verificationJson.status to be the literal 'error'; update the cast to the
correct union that includes both the error and success shapes (like the autofill
flow's union) so TypeScript sees verificationJson.status as 'error' | 'ok' (and
include any success fields expected), then adjust downstream code to use the
proper discriminated-union checks on verificationJson.status.
---
Nitpick comments:
In `@app/routes/login.tsx`:
- Around line 218-222: The fetch calls currently parse JSON unconditionally
which can feed invalid shapes into AuthenticationOptionsSchema.parse and surface
confusing Zod errors; update the four call sites (the autofill effect using
optionsResponse and verificationResponse, and inside handlePasskeyLogin) to
check response.ok before calling response.json(), and on non-ok either throw or
return a handled error (e.g., read text or json for error details and surface a
clear message) so AuthenticationOptionsSchema.parse only receives valid bodies;
ensure you adjust the branches that call AuthenticationOptionsSchema.parse (or
subsequent logic) to run only when the response was ok.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/routes/login.tsx (1)
328-338:⚠️ Potential issue | 🟡 Minor
handlePasskeyLoginsurfaces a confusing error when the user intentionally cancels the passkey dialog.When the WebAuthn dialog is dismissed,
startAuthenticationthrowsNotAllowedError. The autofill flow (lines 263–268) silently swallows it; the explicit button path does not — the catch block shows the raw browser error string (e.g. "The operation either timed out or was not allowed."). Thefinallyblock is correct (autofill re-arms unconditionally), but the user-visible error is misleading for a deliberate cancellation.🐛 Suggested fix
} catch (e) { setPasskeyMessage(null) + if (e instanceof Error && e.name === 'NotAllowedError') { + // User dismissed the dialog — fail silently, autofill is re-armed below. + return + } console.error(e) setError( e instanceof Error ? e.message : 'Failed to authenticate with passkey', ) } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/login.tsx` around lines 328 - 338, The catch in handlePasskeyLogin currently surfaces the raw browser error (from startAuthentication) when the user cancels the WebAuthn prompt; detect NotAllowedError (or e.name === "NotAllowedError") in the catch and treat it as a user-cancel (clear or set a friendly message via setPasskeyMessage and avoid calling setError with the raw message), otherwise preserve the existing behavior (setError for other errors); keep the finally behavior that calls setPasskeyAutofillResetKey to re-arm autofill.
♻️ Duplicate comments (1)
app/routes/login.tsx (1)
258-275: Past review comment is resolved — unexpected autofill errors now correctly clearpasskeyMessageand surface an error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/login.tsx` around lines 258 - 275, The catch block in the login handler must silently ignore user-cancelled WebAuthn flows and otherwise clear the passkey UI state and surface an error; update the catch clause in app/routes/login.tsx (the try/catch around the passkey/auth flow) so that it returns early for e instanceof Error with e.name === 'NotAllowedError' or 'AbortError', and for all other errors it calls setPasskeyMessage(null), logs the error, and calls setError(e instanceof Error ? e.message : 'Failed to authenticate with passkey').
🧹 Nitpick comments (1)
app/routes/login.tsx (1)
182-184:z.object()silently strips all server-sent WebAuthn options beyondchallenge.
z.object({ challenge: z.string() })uses Zod's default strip mode, so every field the server sends beyondchallenge(userVerification,rpId,timeout,extensions, etc.) is discarded beforestartAuthenticationseesoptionsJSON. For the autofill/conditional-UI flow, emptyallowCredentialsis intentional, but silently dropping server-specifieduserVerificationorrpIdcould produce a ceremony that diverges from server expectations.♻️ Suggested fix — use `.passthrough()` to preserve server-sent options
const AuthenticationOptionsSchema = z.object({ - options: z.object({ challenge: z.string() }), + options: z.object({ challenge: z.string() }).passthrough(), }) satisfies z.ZodType<{ options: PublicKeyCredentialRequestOptionsJSON }>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/login.tsx` around lines 182 - 184, AuthenticationOptionsSchema currently uses z.object({ challenge: z.string() }) which strips any server-sent WebAuthn fields beyond challenge; update the schema to preserve extra properties by calling .passthrough() on the inner object (keep the outer ZodType satisfy line intact) so startAuthentication receives the full optionsJSON (including userVerification, rpId, timeout, extensions, allowCredentials, etc.) instead of silently dropping them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/routes/login.tsx`:
- Around line 328-338: The catch in handlePasskeyLogin currently surfaces the
raw browser error (from startAuthentication) when the user cancels the WebAuthn
prompt; detect NotAllowedError (or e.name === "NotAllowedError") in the catch
and treat it as a user-cancel (clear or set a friendly message via
setPasskeyMessage and avoid calling setError with the raw message), otherwise
preserve the existing behavior (setError for other errors); keep the finally
behavior that calls setPasskeyAutofillResetKey to re-arm autofill.
---
Duplicate comments:
In `@app/routes/login.tsx`:
- Around line 258-275: The catch block in the login handler must silently ignore
user-cancelled WebAuthn flows and otherwise clear the passkey UI state and
surface an error; update the catch clause in app/routes/login.tsx (the try/catch
around the passkey/auth flow) so that it returns early for e instanceof Error
with e.name === 'NotAllowedError' or 'AbortError', and for all other errors it
calls setPasskeyMessage(null), logs the error, and calls setError(e instanceof
Error ? e.message : 'Failed to authenticate with passkey').
---
Nitpick comments:
In `@app/routes/login.tsx`:
- Around line 182-184: AuthenticationOptionsSchema currently uses z.object({
challenge: z.string() }) which strips any server-sent WebAuthn fields beyond
challenge; update the schema to preserve extra properties by calling
.passthrough() on the inner object (keep the outer ZodType satisfy line intact)
so startAuthentication receives the full optionsJSON (including
userVerification, rpId, timeout, extensions, allowCredentials, etc.) instead of
silently dropping them.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
cursor
bot
force-pushed
the
cursor/passkey-welcome-back-1d0f
branch
from
b4041c1 to
b30f9ad
Compare
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
This reverts commit 74594ca.
2 participants
Implement passkey form autofill ('welcome back') on the login page for a privacy-friendly and seamless sign-in experience.
Note
Medium Risk
Changes WebAuthn authentication flow and server-side session/cookie behavior, which can affect login reliability across browsers. The scope is contained and backed by an end-to-end test, but regressions could block sign-in.
Overview
Adds passkey form-autofill (WebAuthn conditional UI) to the
/loginpage by prefetching authentication options, startingstartAuthenticationwithuseBrowserAutofill, and cancelling pending ceremonies on unmount or when switching to explicit passkey login.Refactors client verification into
verifyPasskeyWithServer, preserves server-sent WebAuthn request option fields, updates the email fieldautocompletetousername webauthn, and shows a tip when autofill is supported.On successful passkey verification, the server now also clears the traditional login-info session,
getWebAuthnConfignow requires discoverable credentials (residentKey: 'required'), and a new Playwright E2E test covers passkey registration plus autofill-based sign-in.Written by Cursor Bugbot for commit b3d9e92. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Improvements
Tests