Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughA passkey management screen was added for account pages (GET/POST) to list, rename, add, and remove passkeys. Shared WebAuthn utilities were extracted, account/security UIs were updated to separate passkeys from MFA, and new u2 routes and registry entries expose the screen. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Page as Account Passkeys Screen
participant Server as Server (GET/POST handlers)
participant WebAuthn as Browser WebAuthn API
participant Session as Session Store
participant DB as AuthenticationMethods DB
User->>Page: GET /u2/account/passkeys
Page->>Server: load user & enrollments
Server->>DB: fetch confirmed passkey enrollments
DB-->>Server: enrollments
Server-->>Page: render list
alt Remove Passkey
User->>Page: POST action=remove_passkey
Page->>Server: validate & delete enrollment
Server->>DB: delete enrollment
DB-->>Server: deleted
Server-->>Page: render success
end
alt Rename Passkey
User->>Page: POST action=rename_passkey
Page->>Server: validate & update friendly_name
Server->>DB: update enrollment
DB-->>Server: updated
Server-->>Page: render success
end
alt Add Passkey (Start)
User->>Page: POST action=start_add_passkey
Page->>Server: generate registration options
Server->>Session: store challenge
Server-->>Page: inject registration script
Page->>WebAuthn: navigator.credentials.create()
WebAuthn-->>Page: credential
Page->>Server: POST action=complete_add_passkey (credential)
Server->>Session: retrieve challenge
Server->>WebAuthn: verify registration response
WebAuthn-->>Server: verification result
Server->>DB: create confirmed passkey enrollment
DB-->>Server: created
Server-->>Page: render success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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)
packages/authhero/src/routes/universal-login/screens/passkey-enrollment.ts (1)
246-248: 🛠️ Refactor suggestion | 🟠 MajorInconsistent: local
passkeyTypesarray should use sharedPASSKEY_TYPES.The GET handler still defines a local
passkeyTypesarray at line 246, whilegenerateFreshOptionsJSONat lines 158-160 correctly uses the importedPASSKEY_TYPES. This creates an inconsistency—ifPASSKEY_TYPESis updated, this local array would be out of sync.Use shared PASSKEY_TYPES constant
// Get existing passkey credentials to exclude const enrollments = await ctx.env.data.authenticationMethods.list( client.tenant.id, user.user_id, ); - const passkeyTypes = ["passkey", "webauthn-roaming", "webauthn-platform"]; const excludeCredentials = enrollments - .filter((e) => passkeyTypes.includes(e.type) && e.credential_id) + .filter( + (e) => + PASSKEY_TYPES.includes(e.type as (typeof PASSKEY_TYPES)[number]) && + e.credential_id, + ) .map((e) => ({ id: e.credential_id!, transports: (e.transports || []) as AuthenticatorTransport[], }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/routes/universal-login/screens/passkey-enrollment.ts` around lines 246 - 248, Replace the locally defined passkeyTypes array with the shared imported PASSKEY_TYPES constant to avoid drift: remove the const passkeyTypes = ["passkey", "webauthn-roaming", "webauthn-platform"] and use PASSKEY_TYPES in the excludeCredentials computation (the same constant already used by generateFreshOptionsJSON) so both places reference the single source of truth.
🧹 Nitpick comments (3)
packages/authhero/src/routes/universal-login/screens/account-passkeys.ts (2)
308-308: Clarify: empty action triggers passkey registration.The condition
action === "start_add_passkey" || action === ""means clicking the "Add Passkey" button (which doesn't set an action value) will start registration. This is likely intentional since the button at line 155-164 is aNEXT_BUTTONwithout explicit action, but a brief comment would improve readability.Add clarifying comment
+ // --- Start add passkey (generate WebAuthn options) --- + // Empty action comes from the "Add Passkey" NEXT_BUTTON which doesn't set a specific action if (action === "start_add_passkey" || action === "") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/routes/universal-login/screens/account-passkeys.ts` at line 308, The check `if (action === "start_add_passkey" || action === "")` in the passkey registration flow is relying on the NEXT_BUTTON (which doesn't set an explicit action) to start registration when `action` is empty; add a concise comment immediately above this condition explaining that an empty `action` is intentional and originates from the NEXT_BUTTON click so readers understand why `""` is included (reference the `action` variable and the NEXT_BUTTON control used to start registration in this screen).
346-359: Potential race condition when storing WebAuthn challenge.The read-modify-write pattern for
state_data(read existing → merge → write) can lose concurrent updates if the user has multiple tabs open or rapid interactions. This is a known pattern across the codebase (per context snippets), but worth noting for future hardening.Since this follows the existing codebase pattern and the risk is low for typical single-tab usage, this is informational rather than a blocker.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/routes/universal-login/screens/account-passkeys.ts` around lines 346 - 359, The current read-modify-write using loginSessions.get(...) then loginSessions.update(...) can drop concurrent changes to state_data (race when multiple tabs); change to an atomic/transactional update or use a server-side merge API so you only set webauthn_challenge without clobbering other fields—e.g., replace the get+update flow around loginSessions.get / loginSessions.update and state_data with a single atomic update/merge that adds webauthn_challenge to the existing state_data JSON (or implement optimistic concurrency via a version field) so concurrent updates to state_data are preserved.packages/authhero/src/routes/universal-login/screens/passkey-utils.ts (1)
26-30: Minor:localhostsubstring match could affect edge-case hostnames.The
host.includes("localhost")check at line 28 would also match hostnames likemyapp.localhost.example.comornotlocalhost.dev, forcing HTTP protocol unexpectedly. Consider using a more precise check.Proposed stricter localhost check
export function getExpectedOrigin(ctx: any): string { const host = ctx.var.host || "localhost"; - const protocol = host.includes("localhost") ? "http" : "https"; + const hostWithoutPort = host.split(":")[0]; + const protocol = hostWithoutPort === "localhost" ? "http" : "https"; return `${protocol}://${host}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/routes/universal-login/screens/passkey-utils.ts` around lines 26 - 30, The getExpectedOrigin function currently uses host.includes("localhost") which can mis-detect hosts like "myapp.localhost.example.com"; change the check in getExpectedOrigin to perform a precise localhost match against ctx.var.host (e.g., exact hostname or hostname with port such as "localhost" or "localhost:3000") or use a regex that matches ^localhost(:\d+)?$ and also consider explicit loopback addresses like 127.0.0.1 and ::1 so the protocol decision (http vs https) is only triggered for true loopback hosts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/authhero/src/routes/universal-login/screens/account-passkeys.ts`:
- Around line 98-99: The inline onclick handler for the Rename button constructs
a prompt default using manual single-quote escaping (name.replace(/'/g, "\\'")),
which is unsafe; update the handler in the Rename button's onclick to use
JSON.stringify(name) for proper JS string escaping and remove the surrounding
single quotes around the prompt default so the injected value is a valid quoted
JS string; keep the existing escapeHtml(passkey.id) usage for the passkey_id
assignment. Reference the Rename button's onclick handler and the injected
variables passkey.id and name.
---
Outside diff comments:
In `@packages/authhero/src/routes/universal-login/screens/passkey-enrollment.ts`:
- Around line 246-248: Replace the locally defined passkeyTypes array with the
shared imported PASSKEY_TYPES constant to avoid drift: remove the const
passkeyTypes = ["passkey", "webauthn-roaming", "webauthn-platform"] and use
PASSKEY_TYPES in the excludeCredentials computation (the same constant already
used by generateFreshOptionsJSON) so both places reference the single source of
truth.
---
Nitpick comments:
In `@packages/authhero/src/routes/universal-login/screens/account-passkeys.ts`:
- Line 308: The check `if (action === "start_add_passkey" || action === "")` in
the passkey registration flow is relying on the NEXT_BUTTON (which doesn't set
an explicit action) to start registration when `action` is empty; add a concise
comment immediately above this condition explaining that an empty `action` is
intentional and originates from the NEXT_BUTTON click so readers understand why
`""` is included (reference the `action` variable and the NEXT_BUTTON control
used to start registration in this screen).
- Around line 346-359: The current read-modify-write using
loginSessions.get(...) then loginSessions.update(...) can drop concurrent
changes to state_data (race when multiple tabs); change to an
atomic/transactional update or use a server-side merge API so you only set
webauthn_challenge without clobbering other fields—e.g., replace the get+update
flow around loginSessions.get / loginSessions.update and state_data with a
single atomic update/merge that adds webauthn_challenge to the existing
state_data JSON (or implement optimistic concurrency via a version field) so
concurrent updates to state_data are preserved.
In `@packages/authhero/src/routes/universal-login/screens/passkey-utils.ts`:
- Around line 26-30: The getExpectedOrigin function currently uses
host.includes("localhost") which can mis-detect hosts like
"myapp.localhost.example.com"; change the check in getExpectedOrigin to perform
a precise localhost match against ctx.var.host (e.g., exact hostname or hostname
with port such as "localhost" or "localhost:3000") or use a regex that matches
^localhost(:\d+)?$ and also consider explicit loopback addresses like 127.0.0.1
and ::1 so the protocol decision (http vs https) is only triggered for true
loopback hosts.
🪄 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: a518de45-0040-4cb9-951b-8a0f5b888c4d
📒 Files selected for processing (8)
.changeset/tricky-dodos-double.mdpackages/authhero/src/routes/universal-login/screens/account-passkeys.tspackages/authhero/src/routes/universal-login/screens/account-security.tspackages/authhero/src/routes/universal-login/screens/account.tspackages/authhero/src/routes/universal-login/screens/passkey-enrollment.tspackages/authhero/src/routes/universal-login/screens/passkey-utils.tspackages/authhero/src/routes/universal-login/screens/registry.tspackages/authhero/src/routes/universal-login/u2-routes.tsx
packages/authhero/src/routes/universal-login/screens/account-passkeys.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/authhero/src/routes/universal-login/screens/account-passkeys.ts (3)
62-64: Silent catch may mask legitimate errors.Swallowing all exceptions means network failures, authorization errors, or data corruption would silently show "no passkeys" rather than an error. Consider logging the error or distinguishing between "adapter not available" and other failures.
🔧 Suggested improvement
- } catch { - // adapter may not exist + } catch (err) { + // Log but don't fail the page - graceful degradation + console.warn("Failed to list passkeys:", err); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/routes/universal-login/screens/account-passkeys.ts` around lines 62 - 64, The current bare catch in the passkeys loading block swallows all exceptions; change it to catch the error (e.g., catch (err)) and either log it via the app logger/console or set an error state, and only suppress the exception when it's the specific "adapter not available" case; update the catch in the account-passkeys loading logic (where "adapter" is referenced) to detect adapter absence vs other failures, log err (or rethrow/propagate a visible error) for non-adapter issues so network/auth/data errors are not silently ignored.
346-359: Read-modify-write onstate_datacould lose concurrent challenges.If a user has multiple passkey registration flows open in different tabs (e.g., account-passkeys and passkey-enrollment), the last write to
webauthn_challengewins and invalidates the other. This could cause intermittent "Challenge expired" errors.Consider namespacing the challenge (e.g.,
webauthn_challenge_accountvswebauthn_challenge_enrollment) to isolate concurrent flows.🔧 Suggested approach
await ctx.env.data.loginSessions.update(tenant.id, state, { state_data: JSON.stringify({ ...stateData, - webauthn_challenge: options.challenge, + webauthn_challenge_account: options.challenge, }), });And correspondingly in the verification step:
- const expectedChallenge = stateData.webauthn_challenge as string; + const expectedChallenge = stateData.webauthn_challenge_account as string;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/routes/universal-login/screens/account-passkeys.ts` around lines 346 - 359, The current read-modify-write of login session state_data (using ctx.env.data.loginSessions.get/update and the state_data JSON) stores a single webauthn_challenge key which can be clobbered by concurrent flows; update the logic to namespace the challenge per flow (e.g., use keys like webauthn_challenge_account and webauthn_challenge_enrollment) when writing via loginSessions.update and when reading/validating in the corresponding verification code paths so each flow reads/writes its own namespaced challenge rather than the shared webauthn_challenge field.
154-164: Consider explicit action value for "Add Passkey" button.The
NEXT_BUTTONsubmits with an empty action, handled byaction === "". For clarity and to avoid unintended triggers, consider using an explicit action value.🔧 Suggested change
Add onclick to set action field, or use a submit button with explicit name/value:
components.push({ id: "add-passkey", type: "NEXT_BUTTON", category: "BLOCK", visible: true, config: { text: "Add Passkey", + onclick: "(function(btn){var f=btn.closest('form');if(!f){var w=document.querySelector('authhero-widget');if(w&&w.shadowRoot)f=w.shadowRoot.querySelector('form')}if(f){var a=f.querySelector('[name=\"action-field\"]');if(a)a.value='start_add_passkey'}})(this)", }, order: 5, });And simplify the handler:
- if (action === "start_add_passkey" || action === "") { + if (action === "start_add_passkey") {Also applies to: 308-308
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/routes/universal-login/screens/account-passkeys.ts` around lines 154 - 164, The Add Passkey NEXT_BUTTON currently submits with an empty action which relies on checking action === ""; update the component with an explicit action/value so form handlers don't rely on empty strings: modify the component object with id "add-passkey" (and the other similar component at the other location) to include an explicit action name (e.g., action: "add-passkey" or add an onclick that sets the form action field) and then simplify the submit handler to check for that explicit action value instead of "" in the function handling the NEXT_BUTTON submission.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/authhero/src/routes/universal-login/screens/account-passkeys.ts`:
- Around line 62-64: The current bare catch in the passkeys loading block
swallows all exceptions; change it to catch the error (e.g., catch (err)) and
either log it via the app logger/console or set an error state, and only
suppress the exception when it's the specific "adapter not available" case;
update the catch in the account-passkeys loading logic (where "adapter" is
referenced) to detect adapter absence vs other failures, log err (or
rethrow/propagate a visible error) for non-adapter issues so network/auth/data
errors are not silently ignored.
- Around line 346-359: The current read-modify-write of login session state_data
(using ctx.env.data.loginSessions.get/update and the state_data JSON) stores a
single webauthn_challenge key which can be clobbered by concurrent flows; update
the logic to namespace the challenge per flow (e.g., use keys like
webauthn_challenge_account and webauthn_challenge_enrollment) when writing via
loginSessions.update and when reading/validating in the corresponding
verification code paths so each flow reads/writes its own namespaced challenge
rather than the shared webauthn_challenge field.
- Around line 154-164: The Add Passkey NEXT_BUTTON currently submits with an
empty action which relies on checking action === ""; update the component with
an explicit action/value so form handlers don't rely on empty strings: modify
the component object with id "add-passkey" (and the other similar component at
the other location) to include an explicit action name (e.g., action:
"add-passkey" or add an onclick that sets the form action field) and then
simplify the submit handler to check for that explicit action value instead of
"" in the function handling the NEXT_BUTTON submission.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85835870-feda-4d0f-988c-2cb8bc35c088
📒 Files selected for processing (2)
packages/authhero/src/routes/universal-login/screens/account-passkeys.tspackages/authhero/src/routes/universal-login/screens/passkey-enrollment.ts
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 by CodeRabbit