Repository navigation
feat(auth): passkey conditional UI and WebAuthn Level 3 hints - #894
Merged
Merged
Conversation
Destructure options fields into useCallback dependency array to prevent infinite re-render loop caused by object identity changing every render. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace QR-first passkey flow with native platform authenticator experience. Adds WebAuthn Level 3 hints: ['client-device'] to prefer Touch ID / Windows Hello over hybrid/QR transport, and activates conditional UI (autofill) on the signin page so passkeys appear in the browser dropdown without a button click — per FIDO Alliance UX guidelines. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The useConditionalPasskeyUI hook recreated startConditionalUI every render because callers passed inline onSuccess/refreshToken arrows. Using refs for option callbacks decouples identity from the useCallback dep array, so startConditionalUI stays stable across re-renders. Also: extract hook to own module for testability, guard conditional UI behind isAvailable check and on-prem flag, fix GenerateAuthOptionsResult type to include hints field, remove redundant mountedRef initialization. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Point index.ts directly at useConditionalPasskeyUI.ts and remove the now-unnecessary re-export from PasskeyLoginButton.tsx. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 20 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughRefactored Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SignInForm as SignIn Page
participant ConditionalUI as useConditionalPasskeyUI
participant WebAuthn as Browser WebAuthn
participant API as Backend API
participant AuthStore as Auth Store
User->>SignInForm: Load sign-in page
SignInForm->>ConditionalUI: Initialize hook with csrfToken
ConditionalUI->>WebAuthn: Check isConditionalMediationAvailable()
WebAuthn-->>ConditionalUI: availability status
ConditionalUI-->>SignInForm: isAvailable flag
alt Conditional UI Available & CSRF Present
SignInForm->>SignInForm: Render email input with autofill anchor
User->>SignInForm: Focus email field (triggers autofill)
SignInForm->>ConditionalUI: startConditionalUI()
ConditionalUI->>API: POST /api/auth/passkey/authenticate/options
API-->>ConditionalUI: auth challenge & options
ConditionalUI->>WebAuthn: startAuthentication(useBrowserAutofill: true)
WebAuthn-->>ConditionalUI: credential response
ConditionalUI->>ConditionalUI: Refresh CSRF token via callback
ConditionalUI->>API: POST /api/auth/passkey/authenticate (verify credential)
API-->>ConditionalUI: verification success
ConditionalUI->>AuthStore: Clear auth failure flag
ConditionalUI-->>User: Success toast & redirect
else Fallback (No Conditional UI)
SignInForm->>SignInForm: Render passkey button
User->>SignInForm: Click passkey button
SignInForm->>User: Redirect or manual auth flow
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
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 |
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/app/auth/signin/page.tsx (1)
4-4:⚠️ Potential issue | 🟠 MajorAuto-start conditional UI only once per mount.
refreshToken()updates thecsrfTokenstate, and bothuseConditionalPasskeyUIandPasskeyLoginButtoncall it during auth. Because this effect keys offcsrfToken, a mid-flight token refresh can re-enterstartConditionalUI()while another passkey ceremony is already running. That turns the passive autofill flow and the explicit passkey button into a race on browsers that support conditional UI.🛠️ Suggested fix
-import { useState, useEffect, Suspense } from "react"; +import { useState, useEffect, useRef, Suspense } from "react"; ... + const conditionalUiStartedRef = useRef(false); + useEffect(() => { - if (csrfToken && !onPrem) startConditionalUI(); - }, [csrfToken, startConditionalUI, onPrem]); + if (!csrfToken || onPrem || !conditionalUIAvailable || conditionalUiStartedRef.current) { + return; + } + + conditionalUiStartedRef.current = true; + void startConditionalUI(); + }, [csrfToken, conditionalUIAvailable, startConditionalUI, onPrem]);Also applies to: 95-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/auth/signin/page.tsx` at line 4, refreshToken updating csrfToken can retrigger startConditionalUI and cause two passkey ceremonies; prevent re-entry by adding a mount-scoped guard (e.g., a ref like conditionalUIStartedRef) inside useConditionalPasskeyUI and PasskeyLoginButton so startConditionalUI is only invoked once per component mount even if csrfToken changes; check the ref before calling startConditionalUI, set it true immediately when starting, and reset it on unmount/cleanup to allow future mounts to start the UI again.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/auth/useConditionalPasskeyUI.ts`:
- Line 4: The pending startAuthentication call in useConditionalPasskeyUI.ts
must be explicitly cancelled on unmount and its expected abort error must be
ignored: import and call WebAuthnAbortService.cancelCeremony() in the cleanup to
abort the SimpleWebAuthn ceremony started by startAuthentication, and update the
error handling for the promise to treat both DOM AbortError and SimpleWebAuthn's
WebAuthnError with code === 'ERROR_CEREMONY_ABORTED' as non-failures (silently
ignore), ensuring you import WebAuthnAbortService and WebAuthnError from
'@simplewebauthn/browser' and reference the startAuthentication invocation and
its catch/cleanup logic when making the changes.
---
Outside diff comments:
In `@apps/web/src/app/auth/signin/page.tsx`:
- Line 4: refreshToken updating csrfToken can retrigger startConditionalUI and
cause two passkey ceremonies; prevent re-entry by adding a mount-scoped guard
(e.g., a ref like conditionalUIStartedRef) inside useConditionalPasskeyUI and
PasskeyLoginButton so startConditionalUI is only invoked once per component
mount even if csrfToken changes; check the ref before calling
startConditionalUI, set it true immediately when starting, and reset it on
unmount/cleanup to allow future mounts to start the UI again.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 73ed8fca-4f9e-44ee-874f-28a2e6046572
📒 Files selected for processing (8)
apps/web/src/app/auth/signin/page.tsxapps/web/src/components/auth/MagicLinkForm.tsxapps/web/src/components/auth/PasskeyLoginButton.tsxapps/web/src/components/auth/__tests__/useConditionalPasskeyUI.test.tsapps/web/src/components/auth/index.tsapps/web/src/components/auth/useConditionalPasskeyUI.tspackages/lib/src/auth/__tests__/passkey-service.test.tspackages/lib/src/auth/passkey-service.ts
…RTED Import WebAuthnAbortService and WebAuthnError from @simplewebauthn/browser. Call cancelCeremony() in the cleanup effect so the conditional UI promise doesn't leak across client-side navigation. Catch ERROR_CEREMONY_ABORTED separately from generic AbortError to avoid spurious debug logging. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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: 7f7c1444ef
ℹ️ 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".
…anup Addresses two review concerns from PR #894 (Codex P1 + P2): P2 — Refresh conditional UI challenge before long-idle verify The hook fetched authentication options once and waited indefinitely in conditional mediation; the server challenge expires after 5 min so long-idle autofill failed with CHALLENGE_EXPIRED. The ceremony is now driven by a pure state machine that proactively re-fetches options at 4 minutes (server TTL - 1 min buffer, derived from a shared constant so client and server can't drift). If verify still returns CHALLENGE_EXPIRED as a safety net the same retry path fires. The ceremony logic is extracted into conditionalPasskeyCeremony.ts as pure predicates + classifiers + asyncPipe-composed steps + a reducer driven loop. The hook becomes a thin wiring layer and its public surface is unchanged (existing callback-ref stability tests still pass unmodified). P1 — Stop clobbering concurrent sessions' in-flight challenges generateAuthenticationOptions unconditionally deleted all unused webauthn_auth rows for the cleanup user. For the user-keyed flow that is fine (single in-flight ceremony per user), but for the conditional-UI flow the cleanup user is the shared system user, so one visitor's /options call was invalidating every other concurrent visitor's challenge. The P2 refresh timer increases the /options call rate which would have made this race more frequent, so the cleanup is now split: user-keyed flows still wipe all unused rows, the system-user flow only evicts EXPIRED rows. New shared client-safe constant PASSKEY_CHALLENGE_EXPIRY_MINUTES is exported from @pagespace/lib so the browser-side refresh derivation has a single source of truth with PASSKEY_CONFIG.challengeExpiryMinutes. Tests: - 33 new pure unit tests for conditionalPasskeyCeremony (predicates, classifiers, deriveRefreshIntervalMs, nextState reducer, driveCeremony loop, handleCeremonyResult, integrated runCeremony pipe with fake- timer refresh-abort path) - 2 new tests asserting the cleanup-scope contract (conditional UI adds lt(expiresAt, now); email-scoped flow does not) - Existing useConditionalPasskeyUI callback-ref stability tests pass unchanged - Full lib test suite: 4028/4028 passing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Minor cleanup — the refreshTimer variable was being set to undefined in the finally block after clearTimeout, but the variable is about to go out of scope, so the assignment is noise. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The new conditionalPasskeyCeremony client module imported PASSKEY_CHALLENGE_EXPIRY_MINUTES from '@pagespace/lib', which Next.js's webpack resolves to the main server index (not the browser-conditional client-safe entry), dragging the whole server tree into the client bundle — google-auth-library then tried to pull node:fs/node:https/ node:buffer/child_process and the web#build step failed with UnhandledSchemeError. Switched the import to '@pagespace/lib/client-safe' explicitly, matching the project convention used in all other client-side files. Constant is re-exported from both entries so semantics are identical; only the bundle path changes. Verified locally with a full `pnpm --filter web build` — build now succeeds. Targeted ceremony tests still pass (33/33). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Only startAssertionWithRefreshTimer had its own catch block — a throw from fetchAuthenticationOptions (e.g. network error, JSON parse error, getDevicePlatformFields throwing) would propagate up to the hook's useEffect as an unhandled rejection. The original hook's catch swallowed non-abort errors with a console.debug, which this refactor had lost. runCeremony now wraps the pipe in a try/catch that classifies the error (distinguishing abort vs. ceremony-error) and preserves the original console.debug behavior for non-abort failures, so anything thrown by a step returns a terminal CeremonyResult instead of propagating. Adds a test that a throwing getDevicePlatformFields is classified as abort/ceremony-error and debug-logged once. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2witstudios
added a commit
that referenced
this pull request
Apr 13, 2026
…d output shape Wave 1 of 3 in the SIEM dual-read effort. After PRs #894–#898 routed ~170 routes through audit()/auditRequest() into security_audit_log, those events became invisible to the SIEM worker which only reads activity_logs. Rather than dual-write at the route hot path, this PR lays the groundwork to dual-read at the worker. - Verify siem_delivery_cursors.id is unconstrained text (multi-source ready) - Add SecurityAuditSiemRow + pure mapSecurityAuditToSiemEntry mapper that folds forensic context (sessionId, ipAddress, riskScore, anomalyFlags) into metadata while preserving the hash chain - Unify AuditLogEntry with a `source` field; bump webhook payload to v1.1 and add `source` SD-PARAM to syslog so receivers can distinguish origins - Stamp existing activity_logs entries with source: 'activity_logs' Worker refactor (Wave 2) is blocked on this merging. Plan at ~/.claude/plans/giggly-hopping-church.md. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
5 of 6 tasks
2witstudios
added a commit
that referenced
this pull request
Apr 13, 2026
…d output shape (#899) * feat(siem): dual-read foundation — security_audit_log mapper + unified output shape Wave 1 of 3 in the SIEM dual-read effort. After PRs #894–#898 routed ~170 routes through audit()/auditRequest() into security_audit_log, those events became invisible to the SIEM worker which only reads activity_logs. Rather than dual-write at the route hot path, this PR lays the groundwork to dual-read at the worker. - Verify siem_delivery_cursors.id is unconstrained text (multi-source ready) - Add SecurityAuditSiemRow + pure mapSecurityAuditToSiemEntry mapper that folds forensic context (sessionId, ipAddress, riskScore, anomalyFlags) into metadata while preserving the hash chain - Unify AuditLogEntry with a `source` field; bump webhook payload to v1.1 and add `source` SD-PARAM to syslog so receivers can distinguish origins - Stamp existing activity_logs entries with source: 'activity_logs' Worker refactor (Wave 2) is blocked on this merging. Plan at ~/.claude/plans/giggly-hopping-church.md. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore(siem): address review feedback on Wave 1 foundation - Document why mapSecurityAuditToSiemEntry hard-codes isAiGenerated: false (security_audit_log has no AI-attribution columns today). - Drop the tautological "accepts arbitrary source identifiers" test — it tested JS property assignment, not the schema. - Drop the columnType === 'PgText' assertion — Drizzle internal, fragile across minor bumps. dataType + enumValues already prove the same thing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
useConditionalPasskeyUIhook, allowing users to authenticate with a single tap from the browser's autofill dropdownhints: ['client-device']to authentication options (WebAuthn Level 3) to prefer platform authenticators over QR/hybrid promptsautoComplete="email webauthn"to email inputs on sign-in and magic-link forms to anchor conditional mediationuseConditionalPasskeyUIinto its own module with ref-based callback stability (prevents infinite re-render loops from inline arrow props)CHALLENGE_EXPIRED. Ceremony logic extracted into a pureconditionalPasskeyCeremonymodule (asyncPipe-composed steps, explicit state machine, result objects) so the hook is a thin wiring layer.generateAuthenticationOptionsno longer deletes concurrent visitors' in-flight challenges when the flow is conditional-UI (shared system user). User-keyed flows still wipe all unusedwebauthn_authrows; the system-user branch now only evicts expired rows.Changes
packages/lib/src/auth/passkey-service.tshints: ['client-device']+ split cleanup (user-keyed wipes all unused; system-user conditional-UI only wipes expired) +challengeExpiryMinutesnow sourced from shared constantpackages/lib/src/auth/passkey-client-constants.tsPASSKEY_CHALLENGE_EXPIRY_MINUTESconstant, single source of truth for client/serverpackages/lib/src/{index,client-safe,auth/index}.tsapps/web/src/components/auth/useConditionalPasskeyUI.tsdriveCeremony+handleCeremonyResultapps/web/src/components/auth/conditionalPasskeyCeremony.tsderiveRefreshIntervalMs,asyncPipe, pipe steps,nextStatereducer,driveCeremonyloop,handleCeremonyResultapps/web/src/components/auth/PasskeyLoginButton.tsxapps/web/src/app/auth/signin/page.tsxapps/web/src/components/auth/MagicLinkForm.tsxwebauthnto autoCompleteapps/web/src/components/auth/index.tsTest plan
packages/lib/src/auth/__tests__/passkey-service.test.ts— 43/43 passing (hints assertions + 2 new cleanup-scope assertions: conditional UI addslt(expiresAt, now), email-scoped flow does not)apps/web/src/components/auth/__tests__/useConditionalPasskeyUI.test.ts— 4/4 passing (callback stability with stable refs, changing token, inline arrows; public surface unchanged)apps/web/src/components/auth/__tests__/conditionalPasskeyCeremony.test.ts— 33/33 passing (predicates, classifiers,deriveRefreshIntervalMs,nextStatereducer,driveCeremonyloop,handleCeremonyResult, integratedrunCeremonypipe with fake-timer refresh-abort path)@pagespace/libtest suite — 4028/4028 passingweband@pagespace/lib)/auth/signin, verify passkey autofill appears in email field on supported browsers (Chrome 108+, Safari 16+)POST /api/auth/passkey/authenticate/optionshitting the network tab at ~4 min without user interaction, then selecting a passkey from autofill succeeds🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements