Repository navigation
feat: implement passkey-first authentication with WebAuthn - #681
Conversation
Add passwordless authentication via passkeys (TouchID, FaceID, Windows Hello, security keys): **Infrastructure:** - Add passkeys table schema with migration - Add passkey-service with Result pattern matching magic-link-service - Add rate limiting for passkey registration and authentication **Registration Flow:** - POST /api/auth/passkey/register/options - Generate registration challenge - POST /api/auth/passkey/register - Verify and store passkey - PasskeyManager component for settings page **Authentication Flow:** - POST /api/auth/passkey/authenticate/options - Generate auth challenge - POST /api/auth/passkey/authenticate - Verify passkey and create session - PasskeyLoginButton component for sign-in page **Security:** - Challenge stored as hash only (5-minute expiry, single-use) - Counter replay protection with atomic database update - Session fixation prevention (revoke existing sessions) - CSRF protection on all endpoints Implements #568, #569, #570 (Passkeys Epic #586) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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:
📝 WalkthroughWalkthroughAdds full WebAuthn/passkey support: DB migration and schema, passkey service and tests, server API routes for register/auth/list/rename/delete, frontend components/hooks for signup/login/management, rate-limits, and monitoring event extensions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Browser as Browser (WebAuthn)
participant Server
participant DB
Client->>Server: POST /api/auth/passkey/register/options
Server->>Server: Validate session & CSRF, enforce rate limit, check per-user limit
Server->>DB: Store challenge (verification token)
Server-->>Client: Return registration options + challengeId
Client->>Browser: navigator.credentials.create()
Browser-->>Client: attestationObject + clientDataJSON
Client->>Server: POST /api/auth/passkey/register
Server->>Server: Verify registration via simplewebauthn
Server->>DB: Insert passkey record
Server-->>Client: Return { passkeyId }
sequenceDiagram
participant Client
participant Browser as Browser (WebAuthn)
participant Server
participant DB
Client->>Server: POST /api/auth/passkey/authenticate/options
Server->>Server: Validate CSRF, rate limit by IP, generate options
Server->>DB: Store challenge (may use system user)
Server-->>Client: Return options + challengeId
Client->>Browser: navigator.credentials.get()
Browser-->>Client: authenticatorData + signature
Client->>Server: POST /api/auth/passkey/authenticate
Server->>DB: Retrieve passkey by credentialId
Server->>Server: Verify authentication, check counter/replay
Server->>DB: Revoke old sessions, create new session
Server->>Server: Generate CSRF token for new session
Server-->>Client: Set cookies, return success + redirect
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/db/src/schema/auth.ts (1)
171-181:⚠️ Potential issue | 🟡 MinorAdd missing
passkeysrelation tousersRelationsfor schema consistency.The
passkeysRelationsdefines the inverse relation (passkey → user), butusersRelationslackspasskeys: many(passkeys). While current code queries passkeys directly viadb.query.passkeys, the relation should be included to maintain consistency with all other user-related tables and enable relational queries likedb.query.users.findFirst({ with: { passkeys: true } }).Proposed fix
export const usersRelations = relations(users, ({ many }) => ({ deviceTokens: many(deviceTokens), chatMessages: many(chatMessages), aiSettings: many(userAiSettings), mcpTokens: many(mcpTokens), verificationTokens: many(verificationTokens), socketTokens: many(socketTokens), subscriptions: many(subscriptions), sessions: many(sessions), emailUnsubscribeTokens: many(emailUnsubscribeTokens), + passkeys: many(passkeys), }));
🤖 Fix all issues with AI agents
In `@apps/web/src/app/api/auth/passkey/authenticate/route.ts`:
- Around line 109-116: The current code calls
sessionService.revokeAllUserSessions(userId, 'passkey_login') which
force-terminates every active session on passkey login; change this to a less
disruptive behavior by either revoking only the current session (if one exists)
or rotating the current session token instead of calling revokeAllUserSessions,
and make the behavior configurable (e.g., add a flag or ENV to keep aggressive
revoke as an option). Update the code paths around revokeAllUserSessions,
sessionService, and the passkey login flow to call a new method like
sessionService.revokeCurrentSession or sessionService.rotateSessionToken (or
gate the full revoke behind a config check), and if you keep full-revoke as an
option, add a loggers.auth.info message and ensure the API surface returns a
user-facing notice indicating other sessions will be terminated.
In `@apps/web/src/components/auth/PasskeyLoginButton.tsx`:
- Around line 100-107: The cookie parsing is fragile because split('=')[1]
truncates values containing '='; update both places (in handleLogin and
startConditionalUI where csrfCookie/newCsrfToken are used) to extract the value
by finding the first '=' and taking the substring after it (e.g., use
indexOf('=') + 1 and slice/substring) so the entire cookie value (including any
'=' chars) is preserved before storing into localStorage as 'csrfToken'.
In `@apps/web/src/components/settings/PasskeyManager.tsx`:
- Around line 39-43: The SWR fetcher in PasskeyManager.tsx and the component's
mutation calls currently use bare fetch which bypasses the app's auth layer;
import and use fetchWithAuth from '@/lib/auth/auth-fetch' instead of fetch in
the fetcher (const fetcher) and in all POST/DELETE/PATCH mutation requests in
this file so bearer-token platforms, automatic 401 retry, and CSRF refresh are
respected; update call sites to pass the same url and options to fetchWithAuth
and remove or adapt manual CSRF header logic if fetchWithAuth already manages
CSRF/token headers.
In `@packages/lib/src/auth/passkey-service.test.ts`:
- Around line 90-98: The afterEach cleanup only deletes verificationTokens for
testUserId but generateAuthenticationOptions(...) creates tokens with userId
'system', so update the afterEach in the test to also remove those system
tokens: extend the db.delete call that targets verificationTokens (the
verificationTokens table used alongside db.delete and eq in afterEach) to remove
rows for both testUserId and 'system' (e.g., add an OR/in condition), ensuring
all challenges created by generateAuthenticationOptions are cleaned up between
tests.
In `@packages/lib/src/auth/passkey-service.ts`:
- Around line 483-508: The atomic counter guard is missing: update the
db.update(passkeys).where(...) call used with passkey.id so it includes a
counter comparison (e.g., passkeys.counter <
verification.authenticationInfo.newCounter) using Drizzle's sql raw expression,
then check counterUpdateResult (the returned rows) and treat an empty result as
COUNTER_REPLAY_DETECTED instead of comparing against the stale passkey.counter;
ensure the update still sets counter and lastUsedAt and return ok only when the
update returned at least one row.
- Around line 357-381: The else-branch inserts userId: 'system' into
verificationTokens which violates the verificationTokens.userId FK constraint;
update the schema and code so anonymous challenges don't reference a bogus user:
modify the verificationTokens table to allow userId to be nullable (remove
.notNull() / FK or make FK DEFERRABLE) and change the insert in
passkey-service.ts (the db.insert(verificationTokens).values(...) call in the
branch that checks challengeUserId) to set userId to null or omit the field when
challengeUserId is falsy; alternatively, if you prefer not to change the
existing table, create a new webauthn_challenges table (or use Redis) and move
the anonymous-challenge inserts/reads there and update the verification/lookup
paths accordingly.
🧹 Nitpick comments (11)
apps/web/src/hooks/useLoginCSRF.ts (1)
1-51: Consider extracting a shared CSRF hook to reduce duplication withuseCSRFToken.
useLoginCSRFanduseCSRFTokenare nearly identical — they differ only in the endpoint URL (/api/auth/login-csrfvs/api/auth/csrf). A shareduseCSRFBase(endpoint)helper would eliminate the copy-paste and make both easier to maintain.Also,
refreshToken(Line 41-43) is a trivial passthrough offetchToken— you can returnfetchTokendirectly asrefreshTokenand drop the extra wrapper.packages/db/src/schema/auth.ts (1)
223-239: Naming convention inconsistency: snake_case DB column names vs. camelCase in older tables.The
passkeystable uses snake_case for DB column names ('user_id','credential_id', etc.), which matchesemailUnsubscribeTokensbut diverges from older tables in this file (deviceTokens,verificationTokens, etc.) that use camelCase ('userId','tokenHash'). This isn't a bug — Drizzle maps either way — but the inconsistency within the same schema file may cause confusion.Not blocking, just flagging for awareness since both conventions coexist.
apps/web/src/app/api/auth/passkey/[passkeyId]/route.ts (1)
13-15: Route-levelupdateNameSchemaduplicates service-level validation.The passkey service already validates
namewith the same constraints (min(1).max(255)). The duplication is minor and adds defense-in-depth at the API boundary, but the hardcoded255here could drift fromPASSKEY_CONFIG.maxNameLength. Consider importing the constant.packages/lib/src/auth/passkey-service.ts (1)
40-54:z.any()violates the "never useanytypes" coding guideline.Both
verifyRegSchema.response(Line 42) andverifyAuthSchema.response(Line 52) usez.any(). Even though SimpleWebAuthn validates the response internally, usingz.any()bypasses TypeScript safety entirely. Consider usingz.record(z.unknown())or a more specific schema matchingRegistrationResponseJSON/AuthenticationResponseJSONshapes.As per coding guidelines: "Never use
anytypes - always use proper TypeScript types".apps/web/src/components/auth/PasskeyLoginButton.tsx (1)
40-131: Duplicate authentication flow betweenhandleLoginandstartConditionalUI.Both functions share the same fetch-options → start-authentication → verify → extract-CSRF → redirect pipeline. Consider extracting the shared core (verify + post-login) into a helper to reduce duplication and ensure consistent behavior for cookie handling and error messages.
Also applies to: 187-261
apps/web/src/app/auth/signin/page.tsx (1)
22-22:'passkey'variant inAuthMethodis unused.The
'passkey'value is never assigned viasetAuthMethod. Only'magic-link'and'password'are used. If passkey login doesn't need its own view mode (it's a button overlay on existing modes), this variant is dead code.apps/web/src/app/settings/account/page.tsx (1)
24-24: Duplicatelucide-reactimport source.
SmartphoneandFingerprintare imported fromlucide-reacton line 24, while line 14 already imports many icons from the same package. Consolidating them into a single import statement would be cleaner.Proposed fix
-import { User, Mail, Calendar, AlertTriangle, Loader2, ArrowLeft, Upload, X, CheckCircle2, AlertCircle, Download, Clock } from "lucide-react"; +import { User, Mail, Calendar, AlertTriangle, Loader2, ArrowLeft, Upload, X, CheckCircle2, AlertCircle, Download, Clock, Smartphone, Fingerprint } from "lucide-react"; ... -import { Smartphone, Fingerprint } from "lucide-react";apps/web/src/app/api/auth/passkey/register/route.ts (2)
17-21:z.any()forresponsefield violates the "no any types" guideline.The coding guidelines state: "Never use
anytypes." While the WebAuthn response is complex and validated downstream by@simplewebauthn/server, usingz.any()bypasses all schema-level validation, meaning malformed payloads reach the service layer before being rejected.Consider using
z.record(z.string(), z.unknown())orz.object({}).passthrough()as a lightweight structural guard that still satisfies the guideline. As per coding guidelines, "Never useanytypes - always use proper TypeScript types."Proposed fix
const verifySchema = z.object({ - response: z.any(), // WebAuthn response - validated by simplewebauthn + response: z.record(z.string(), z.unknown()), // Structural guard; full validation by simplewebauthn expectedChallenge: z.string().min(1), name: z.string().max(255).optional(), });
77-86: Zod error format inconsistency with codebase pattern.Line 83 uses
validation.error.issues, but the codebase convention (per learnings) is to usevalidation.error.flatten().fieldErrorsfor consistency across API routes. Note that in Zod v4, the equivalent would bez.flattenError(validation.error)when that migration happens, but for now the existing pattern should be maintained. Based on learnings, "Maintain the existing pattern of using validation.error.flatten().fieldErrors across API routes."Proposed fix
if (!validation.success) { return NextResponse.json( - { error: 'Invalid request body', details: validation.error.issues }, + { error: 'Invalid request body', details: validation.error.flatten().fieldErrors }, { status: 400 } ); }apps/web/src/app/api/auth/passkey/authenticate/route.ts (2)
19-23: Samez.any()issue as the register route — replace with a structural guard.Same guideline violation as in the register route. As per coding guidelines, "Never use
anytypes - always use proper TypeScript types."Proposed fix
const verifySchema = z.object({ - response: z.any(), // WebAuthn response - validated by simplewebauthn + response: z.record(z.string(), z.unknown()), // Structural guard; full validation by simplewebauthn expectedChallenge: z.string().min(1), csrfToken: z.string().min(1), });
57-62: Zod error format — use.flatten().fieldErrorsfor consistency.Same as the register route: align with the codebase convention. Based on learnings, "Maintain the existing pattern of using validation.error.flatten().fieldErrors across API routes."
Proposed fix
if (!validation.success) { return NextResponse.json( - { error: 'Invalid request body', details: validation.error.issues }, + { error: 'Invalid request body', details: validation.error.flatten().fieldErrors }, { status: 400 } ); }
- Fix mock to return excludeCredentials when passkeys exist - Create system user for anonymous auth challenges (conditional UI) - Update migration to create system user on deploy Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add missing passkeys relation to usersRelations for schema consistency - Fix cookie parsing to handle '=' characters in CSRF token values Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Addressing CodeRabbit Review FeedbackThank you for the thorough review! I've addressed the following items in commit 6623adf: ✅ Fixed
Already Addressed (in earlier commits)
Acknowledged (not changing)
Note on Failing TestThe failing test |
- Implement atomic counter replay protection with SQL WHERE clause - Use fetchWithAuth in PasskeyManager for consistent auth handling Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Additional Critical Fixes (commit 6325ec7)✅ Counter Replay Protection (Critical)Implemented truly atomic counter replay protection using SQL WHERE clause: .where(
and(
eq(passkeys.id, passkey.id),
sql`${passkeys.counter} < ${newCounter}`
)
)If no rows are updated (counter wasn't greater), we return ✅ Use fetchWithAuth (Major)Updated PasskeyManager to use
All CI checks passed on previous commit. These changes should also pass. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/web/src/components/settings/PasskeyManager.tsx`:
- Around line 285-301: The icon-only Button elements for renaming and deleting
passkeys are missing accessible labels; update the two Button components (the
one that calls setEditPasskey/setNewName and the one that calls
setDeletePasskeyId) to include descriptive accessibility attributes such as
aria-label or aria-labelledby (e.g., aria-label="Rename passkey" and
aria-label="Delete passkey") and ensure the labels reference the specific
passkey (use passkey.name or passkey.id in the label text) so screen readers can
identify the action and which passkey it targets while keeping the Pencil and
Trash2 icons visually unchanged.
- Around line 63-66: The useSWR call in PasskeyManager currently omits SWR
config; update the useSWR invocation that fetches '/api/auth/passkey' (the const
{ data, error, isLoading } = useSWR<{ passkeys: Passkey[] }>(...)) to pass a
config object with revalidateOnFocus: false so the settings component does not
refetch when the user switches tabs.
In `@packages/lib/src/auth/passkey-service.ts`:
- Around line 348-384: generateAuthenticationOptions currently inserts a new
'webauthn_auth' verificationTokens row (using challengeId, challengeHash,
PASSKEY_CONFIG.systemUserId or challengeUserId) but never cleans up old/stale
auth challenges, which will accumulate (especially for the system user). Before
the db.insert(...) in generateAuthenticationOptions (the block that uses
simpleGenerateAuthenticationOptions and writes to verificationTokens), delete
previous rows of type 'webauthn_auth' for the same user context: remove expired
rows and/or limit to non-expired tokens for either challengeUserId or
PASSKEY_CONFIG.systemUserId (use verificationTokens table, filtering by userId
and type === 'webauthn_auth'), mirroring the cleanup logic used in
generateRegistrationOptions so only current/valid challenges remain. Ensure the
cleanup runs regardless of whether challengeUserId is set and uses the same
expiresAt/expiry semantics as the registration cleanup.
- Around line 486-507: The current atomic update rejects authenticators that
report a zero signCount; change the conditional used in the db.update(where ...)
so authenticators with a stored counter of 0 (or newCounter of 0) are
allowed—e.g., replace the sql`${passkeys.counter} < ${newCounter}` check with a
predicate that permits updates when passkeys.counter = 0 OR passkeys.counter <
newCounter (or alternatively read the existing passkey counter and branch to
skip the strict check when stored counter === 0); update the where clause used
in the update call around passkeys, newCounter, and the sql template to
implement this logic so genuine zero-count passkeys are accepted instead of
returning COUNTER_REPLAY_DETECTED.
🧹 Nitpick comments (5)
packages/db/src/schema/auth.ts (2)
225-240: DB column naming convention is inconsistent with existing tables.The
passkeystable (andemailUnsubscribeTokens) uses snake_case for database column names ('user_id','credential_id','public_key', etc.), while all pre-existing tables in this file use camelCase ('userId','tokenHash','expiresAt', etc.). This creates a mixed convention within the same schema file, which can cause confusion when writing raw SQL or inspecting the database directly.Consider aligning to one convention. If snake_case is the desired direction going forward, a follow-up migration to rename legacy columns would keep things consistent.
228-228: Redundant index oncredentialId.Line 228 declares
.unique()oncredentialId, which in PostgreSQL automatically creates a unique index. The explicitindex('passkeys_credential_id_idx')on line 239 is therefore redundant and adds unnecessary write overhead.Proposed fix
}, (table) => ({ userIdx: index('passkeys_user_id_idx').on(table.userId), - credentialIdx: index('passkeys_credential_id_idx').on(table.credentialId), }));Also applies to: 239-239
apps/web/src/components/auth/PasskeyLoginButton.tsx (1)
189-265: Extract shared authentication-verify-redirect logic to reduce duplication.
startConditionalUIandhandleLoginshare nearly identical flows: fetch options →startAuthentication→ POST verify → parse CSRF cookie → redirect. The cookie-parsing block (Lines 239-247) and the verify+redirect block are verbatim copies. A shared helper (e.g.,verifyAndRedirect(verifyRes, onSuccess)) would eliminate ~25 duplicated lines and ensure future fixes (like addingdecodeURIComponentto the cookie value) are applied in one place.♻️ Sketch of extracted helper
+async function handleVerifyAndRedirect( + verifyRes: Response, + onSuccess?: (redirectUrl: string) => void +): Promise<boolean> { + if (!verifyRes.ok) { + const error = await verifyRes.json(); + toast.error(error.error || 'Authentication failed'); + return false; + } + + const { redirectUrl } = await verifyRes.json(); + + const csrfCookie = document.cookie + .split('; ') + .find(row => row.startsWith('csrf_token=')); + + if (csrfCookie) { + const eqIndex = csrfCookie.indexOf('='); + const newCsrfToken = csrfCookie.substring(eqIndex + 1); + localStorage.setItem('csrfToken', newCsrfToken); + } + + toast.success('Signed in successfully'); + + if (onSuccess) { + onSuccess(redirectUrl); + } else { + window.location.href = redirectUrl; + } + return true; +}Then
handleLogincan add its richer error handling before delegating:// In handleLogin, after the verifyRes fetch: if (!verifyRes.ok) { // specific error code handling... return; } await handleVerifyAndRedirect(verifyRes, onSuccess);apps/web/src/components/settings/PasskeyManager.tsx (1)
39-40: Nit: Move this import to the top with the other imports.The
fetchWithAuthimport is separated from the rest of the imports by thePasskeyinterface. Group all imports together at the top of the file for consistency.packages/lib/src/auth/passkey-service.ts (1)
43-57:z.any()violates the project's "never useanytypes" guideline.Lines 45 and 55 use
z.any()for WebAuthn response fields. I see from the PR discussion that this is intentional since@simplewebauthn/serverperforms its own structural validation, and duplicating that complex schema in Zod adds maintenance burden. That rationale is reasonable, but it's still a deviation from the coding guidelines.A lighter-weight compromise: use
z.record(z.string(), z.unknown())orz.object({}).passthrough()to at least enforce that the input is a non-null object, preventing obviously wrong types (strings, numbers, arrays) from reaching the library.Suggested minimal constraint
const verifyRegSchema = z.object({ userId: z.string().min(1), - response: z.any(), // RegistrationResponseJSON validated by simplewebauthn + response: z.record(z.string(), z.unknown()), // structural validation delegated to simplewebauthn expectedChallenge: z.string().min(1), name: z.string().max(PASSKEY_CONFIG.maxNameLength).optional(), });const verifyAuthSchema = z.object({ - response: z.any(), // AuthenticationResponseJSON validated by simplewebauthn + response: z.record(z.string(), z.unknown()), // structural validation delegated to simplewebauthn expectedChallenge: z.string().min(1), });As per coding guidelines: "
**/*.{ts,tsx}: Never useanytypes - always use proper TypeScript types".
- Add revalidateOnFocus: false to SWR config in PasskeyManager - Add aria-label attributes to icon-only buttons for accessibility - Support authenticators with counter=0 (iCloud Keychain, Google Password Manager) - Add cleanup of stale webauthn_auth challenges before creating new ones Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Response to CodeRabbit ReviewAddressed in commit 106ec41:
Regarding session revocation (authenticate/route.ts:116):The aggressive session revocation on passkey login is intentional per the PR objectives ("Session fixation prevention"). This is a security-first design choice:
If the team prefers a less aggressive policy (e.g., only rotate the current session token), I can make this configurable via an environment variable. However, the current default provides the strongest session fixation protection. |
Implements passkey as the primary signup method per FIDO Alliance 2026 guidelines: - Update WebAuthn config: residentKey='required', userVerification='required' - Add generateRegistrationOptionsForSignup() for new user registration - Add verifySignupRegistration() for atomic user + passkey creation - Create /api/auth/signup-passkey/* routes for signup flow - Add PasskeySignupButton component with WebAuthn support detection - Redesign signup page: passkey primary, OAuth secondary, password collapsed - Add metadata column to verification_tokens for signup context - Add comprehensive tests for new signup functions Users can now create accounts with just name, email, and a passkey. Password signup remains available but is de-emphasized. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2witstudios
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@apps/web/src/app/api/auth/signup-passkey/options/route.ts`:
- Around line 92-113: Add a small error map and use it before falling back to
the 500 path: detect result.error.code (e.g. 'VALIDATION_FAILED') alongside
'EMAIL_EXISTS' in the options route handler so you return an appropriate
status/message instead of always returning 500; update the conditional around
result.ok in apps/web/src/app/api/auth/signup-passkey/options/route.ts to
consult the new errorMap (similar to the sibling route) and call
NextResponse.json with the mapped status/message for recognized service error
codes, otherwise keep the existing logging via loggers.auth.warn and the 500
response.
In `@apps/web/src/app/api/auth/signup-passkey/route.ts`:
- Around line 152-158: The insert of userAiSettings
(db.insert(userAiSettings).values({...})) can throw and currently is
unprotected, causing a 500 after the user and passkey are already created; wrap
this insert in its own try-catch inside the signup-passkey route handler (near
the existing provisionGettingStartedDriveIfNeeded try-catch), catch and log the
error (including the error object) and do not rethrow so the response still
succeeds; optionally record a telemetry event or fallback to a no-op if the
insert fails so account creation remains atomic from the client's perspective.
- Around line 170-177: The trackAuthEvent call in signup-passkey route currently
sends full PII (email, name) into userActivities.metadata; before invoking
trackAuthEvent (in apps/web/src/app/api/auth/signup-passkey/route.ts) mask
sensitive fields (e.g., obfuscate email local-part leaving domain like
a****@example.com and redact or initials-only for name) and pass the masked
values instead of raw email/name, or alternatively remove those fields entirely;
reference the function trackAuthEvent and the userActivities metadata field when
making this change so PII is never persisted without a documented retention
policy.
In `@packages/lib/src/auth/passkey-service.test.ts`:
- Around line 294-299: The assertions use !== null but Drizzle's findFirst
returns undefined when no row is found, so update the tests to check against
undefined (or use strict non-null checks) for the variables produced by
findFirst and optional fields: replace storedPasskey !== null with storedPasskey
!== undefined (or a truthy check), replace usedChallenge?.usedAt !== null with
usedChallenge?.usedAt !== undefined, and replace storedChallenge !== null with
storedChallenge !== undefined — locate these variables (storedPasskey,
usedChallenge, storedChallenge) in passkey-service.test.ts and update the three
assertions accordingly.
In `@packages/lib/src/auth/passkey-service.ts`:
- Around line 832-870: Wrap the two independent inserts (the users insert and
the passkeys insert that use userId, passkeyId, credential.id, publicKeyBase64,
etc.) inside a single Drizzle transaction so they commit or rollback together;
use db.transaction (or the project's transaction helper) to execute both inserts
atomically, preserve the existing unique-constraint detection logic (returning {
ok: false, error: { code: 'EMAIL_EXISTS' } } when the email conflict is
detected) and let other errors propagate after the transaction rolls back to
avoid creating an orphan user with no passkey.
🧹 Nitpick comments (6)
packages/db/src/schema/auth.ts (1)
239-242: Redundant index oncredentialId.The
.unique()constraint oncredentialId(line 230) already creates a unique index in PostgreSQL. The explicitcredentialIdxindex on line 241 is redundant and adds unnecessary overhead on writes.♻️ Suggested fix
}, (table) => ({ userIdx: index('passkeys_user_id_idx').on(table.userId), - credentialIdx: index('passkeys_credential_id_idx').on(table.credentialId), }));packages/lib/src/auth/passkey-service.test.ts (1)
815-816:signupEmailis evaluated once atdescribescope — shared across all tests in the block.
Date.now()is called when thedescribeblock is parsed, not per test. All tests withingenerateRegistrationOptionsForSignupandverifySignupRegistrationshare the same email value. This is fine for isolation within a single run, but if the describe blocks run in parallel or are revisited, collisions are possible. A minor point — moving the assignment intobeforeEachwould improve robustness.Also applies to: 924-925
packages/lib/src/auth/passkey-service.ts (2)
43-48:z.any()used for WebAuthn response fields violates the "never useany" guideline.Three schemas use
z.any()(Lines 45, 55, 82). The PR discussion acknowledges this is intentional to avoid duplicating@simplewebauthn/server's validation. Consider usingz.unknown()instead — it provides the same pass-through behavior but results in anunknowntype rather thanany, which is safer and consistent with the coding guidelines.Proposed fix
const verifyRegSchema = z.object({ userId: z.string().min(1), - response: z.any(), // RegistrationResponseJSON validated by simplewebauthn + response: z.unknown(), // RegistrationResponseJSON validated by simplewebauthn expectedChallenge: z.string().min(1), name: z.string().max(PASSKEY_CONFIG.maxNameLength).optional(), });Apply the same change at Lines 55 and 82.
As per coding guidelines:
**/*.{ts,tsx}: "Never useanytypes - always use proper TypeScript types".Also applies to: 54-57, 79-88
773-777: UnguardedJSON.parsecan throw on corrupted metadata.If
challenge.metadatais a non-null string that isn't valid JSON, this will throw an unhandled exception. While the metadata is written bygenerateRegistrationOptionsForSignupusingJSON.stringify, defensive parsing would be more robust.Proposed fix
- const metadata = challenge.metadata ? JSON.parse(challenge.metadata as string) : {}; + let metadata: { email?: string; name?: string } = {}; + try { + metadata = challenge.metadata ? JSON.parse(challenge.metadata as string) : {}; + } catch { + return { ok: false, error: { code: 'VALIDATION_FAILED', message: 'Invalid challenge metadata' } }; + }apps/web/src/components/auth/PasskeySignupButton.tsx (1)
31-36: Component duplicatesuseWebAuthnSupporthook logic internally.The
PasskeySignupButtoncomponent (Lines 31-36) has its ownisSupportedstate with the sameuseEffect(() => setIsSupported(browserSupportsWebAuthn()), [])pattern that theuseWebAuthnSupporthook (Lines 187-194) implements. The component could use its own exported hook to avoid duplication.Proposed fix
export function PasskeySignupButton({ ... }: PasskeySignupButtonProps) { - const [isSupported, setIsSupported] = useState<boolean | null>(null); const [isRegistering, setIsRegistering] = useState(false); - - useEffect(() => { - setIsSupported(browserSupportsWebAuthn()); - }, []); + const isSupported = useWebAuthnSupport();Also applies to: 187-194
apps/web/src/app/auth/signup/page.tsx (1)
374-394: Consider using shadcn Checkbox component for consistency.The TOS checkbox uses a raw
<input type="checkbox">(Line 376) while the rest of the form uses shadcn/ui components (Input,Button,Label). Using the shadcnCheckboxcomponent would maintain consistent styling and accessibility behavior.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Wrap signup user+passkey creation in DB transaction for atomicity, replace z.any() with z.record() for safer validation, deduplicate CSRF cookie persistence logic, and consolidate CSRF hooks via factory. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add no-store Cache-Control headers to all passkey route success responses. Migrate CSRF and rate-limit logging from loggers.auth.warn to logSecurityEvent with typed passkey-specific event names for consistent security dashboard visibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add VALIDATION_FAILED error handling in signup-passkey/options route - Wrap userAiSettings insert in try-catch to prevent 500 on failure - Mask PII (email, name) before passing to trackAuthEvent - Fix test assertions: use undefined check instead of null for findFirst Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
Implements passwordless authentication via WebAuthn passkeys (TouchID, FaceID, Windows Hello, security keys) with passkey-first signup.
This PR is a clean rebase of #680 on updated master to resolve migration conflicts.
Passkey-First Signup (New!)
POST /api/auth/signup-passkey/options- Generate registration challenge for new usersPOST /api/auth/signup-passkey- Verify passkey and atomically create user + passkeyPasskeySignupButtoncomponent with WebAuthn support detectionWebAuthn Configuration (Updated)
residentKey: 'required'- ensures discoverable credentials for true passwordlessuserVerification: 'required'- ensures biometric/PIN is always usedInfrastructure
passkeystable schema with migrationpasskey-servicewith Result pattern matchingmagic-link-servicemetadatacolumn toverification_tokensfor signup contextRegistration Flow (Adding to existing account)
POST /api/auth/passkey/register/options- Generate registration challengePOST /api/auth/passkey/register- Verify and store passkeyPasskeyManagercomponent for settings pageAuthentication Flow
POST /api/auth/passkey/authenticate/options- Generate auth challengePOST /api/auth/passkey/authenticate- Verify passkey and create sessionPasskeyLoginButtoncomponent for sign-in pageSecurity
Test Plan
Passkey Signup (New)
Passkey Login
Database Migration
Run
pnpm db:migrateto apply:0085_marvelous_norrin_radd.sql- Addsmetadatacolumn toverification_tokensRelated Issues
Implements #568 (Infrastructure), #569 (Registration), #570 (Authentication)
Part of Passkeys Epic #586
Advances "Work OS: Passwordless + Enterprise" GitHub project
Supersedes #680 (rebase to resolve migration conflicts)
"Passwords are insecure and obsolete. All new apps should be using passkey auth." - Eric Elliott
Summary by CodeRabbit
New Features
Tests