fix(passkeys): correct registration error messages and navigation#20674
Closed
vpomerleau wants to merge 3 commits into
Closed
fix(passkeys): correct registration error messages and navigation#20674vpomerleau wants to merge 3 commits into
vpomerleau wants to merge 3 commits into
Conversation
vpomerleau
force-pushed
the
fix/passkey-timeout-cleanup
branch
7 times, most recently
from
c8ff300 to
3bd9994
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the passkey registration UX in fxa-settings by refining WebAuthn error categorization, displaying more actionable/versioned error copy (with help links), ensuring the user is navigated back to Settings on ceremony-time failures, and aligning telemetry reasons with the new categorization (including a synthetic “already registered” case for Firefox).
Changes:
- Remove the
setTimeout/AbortControllerwrapper timeout conversion inwebauthn.ts, relying on browser-drivenpublicKey.timeoutinstead (while still allowing external cancellation viaAbortSignalon registration). - Add synthetic
WebAuthnErrorType.AlreadyRegistered+ new/updated FTL strings to route Firefox’s duplicate-credential collapse to “already registered” copy and a dedicated telemetry bucket. - Update
PagePasskeyAddto dispatch NotSupported/NotAllowed/Timeout to new link-bearing messages and navigate away so users aren’t stranded on the loading state.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/fxa-settings/src/lib/passkeys/webauthn.ts | Removes internal timeout abort wrapper; forwards external abort signal for registration and relies on options.timeout. |
| packages/fxa-settings/src/lib/passkeys/webauthn.test.ts | Updates tests to match browser-driven timeout behavior and new createCredential signature. |
| packages/fxa-settings/src/lib/passkeys/webauthn-errors.ts | Adds synthetic AlreadyRegistered error type + new mapping/glean reason and updated registration FTL ids/fallbacks. |
| packages/fxa-settings/src/lib/passkeys/webauthn-errors.test.ts | Adds coverage for the synthetic AlreadyRegistered categorization and glean reason mapping. |
| packages/fxa-settings/src/lib/passkeys/unsupported-message.tsx | Deletes the old unsupported message helper (replaced by the new consolidated module). |
| packages/fxa-settings/src/lib/passkeys/passkey-custom-error-messages.tsx | Introduces reusable ReactNode helpers for unsupported/not-allowed/timeout registration errors with “Learn more” links + telemetry attributes. |
| packages/fxa-settings/src/lib/passkeys/index.ts | Re-exports the new custom error message helpers from the passkeys barrel. |
| packages/fxa-settings/src/lib/passkeys/en.ftl | Adds new versioned registration strings + link labels for NotAllowed/Timeout. |
| packages/fxa-settings/src/components/Settings/UnitRowPasskey/index.tsx | Updates import to use the new unsupported message helper module. |
| packages/fxa-settings/src/components/Settings/PagePasskeyAdd/index.tsx | Updates registration flow error dispatcher, telemetry reason mapping, and ensures navigation back to settings on failures. |
| packages/fxa-settings/src/components/Settings/PagePasskeyAdd/index.test.tsx | Updates/extends tests for the new dispatcher behavior and message helpers. |
| libs/accounts/passkey/src/lib/webauthn-adapter.ts | Clarifies timeout/challenge-expiry relationship in comments (server-generated options). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…rors
Because:
- The client wrapper's 60s setTimeout converted AbortError into a
synthetic TimeoutError, racing with user-cancel AbortError and
conflating three distinct events into the same code path.
- WebAuthn L3 §5.1.3 / §6.3.2 make NotAllowedError the spec's
anti-fingerprinting catch-all for user-action and authenticator
failures — cancel, dismiss, UV failure, no suitable authenticator,
etc. A single broad message must cover that whole bucket honestly.
- TimeoutError, when surfaced distinctly by a browser, is
unambiguous and deserves its own message.
This commit:
- Removes the client wrapper's setTimeout/AbortController conversion
in createCredential and getCredential. AbortController is used only
for external user-cancel signalling; the browser's native ceremony
timeout flows through the publicKey options.
- Surfaces real DOMException names instead of synthesizing them.
- Routes NotAllowedError and external AbortError on registration to a
new versioned FTL ("Passkey setup couldn't be completed.") plus a
Learn-more SUMO link.
- Routes TimeoutError to its own versioned FTL ("Passkey setup timed
out.") plus a Learn-more SUMO link, distinct from the catch-all.
- Keeps the in-page Cancel path on its own message — the wasCanceled
guard intercepts AbortError before the categorizer.
- Adds per-error Glean click events on the SUMO links:
account_pref_passkey_create_help_{not_supported,not_allowed,timeout}.
- Renames passkey-unsupported-message.tsx -> passkey-custom-error-messages.tsx.
Closes #FXA-13805
Closes #FXA-13806
Because: - Firefox collapses the spec's InvalidStateError into NotAllowedError on duplicate-credential registration. The categorizer recovers the real meaning via the synthetic AuthenticatorAlreadyRegistered name. - That map entry's errorType was WebAuthnErrorType.NotSupported, which PagePasskeyAdd short-circuits on to render the "browser doesn't support passkeys" alert — wrong copy, and no redirect to settings. - The existing test mocked a wrong field name (userMessageKey instead of ftlId) and asserted nothing about the user-facing alert, so the bug never tripped CI. This commit: - Adds WebAuthnErrorType.AlreadyRegistered and re-points the map entry to it. The dispatcher's NotSupported short-circuit no longer fires; the result falls through to the plain-string ftl path and the page auto-navigates to settings. - Maps the new errorType to the existing 'not_supported' Glean bucket for metric continuity (no data review required). - Fixes the broken hadExcludeCredentials test: correct field name, correct errorType, asserts the alert string (aligned to the production curly-apostrophe form) and Glean reason. - Adds two webauthn-errors tests pinning the categorizer's behaviour for the with-context and without-context cases. - Drops the orphan v1 passkey-registration-error-not-allowed FTL key (no remaining references after the prior commit's -v2 migration). Closes #FXA-13876
Because: - The MfaGuard pre-check only validates the browser's WebAuthn Level 3 API surface via isWebAuthnLevel3Supported(). It cannot predict that a specific authenticator or password-manager combination will refuse the ceremony at runtime with NotSupportedError. - Bitwarden on macOS is a real-world repro: pre-check passes, the ceremony throws NotSupportedError, and the catch block's defensive early return strands the user on the loading spinner with no way out except the in-page Cancel link. - The dispatcher's ReactNode-alert tests only asserted React.isValidElement, which can't distinguish which helper was invoked — a routing regression would pass silently. This commit: - Adds navigateToSettings() before the early return in PagePasskeyAdd's NotSupportedError branch. The unsupported alert still shows on the settings landing page, matching every other error path's behaviour. - Updates the corresponding test to assert the redirect (was pinned to no-navigation, which was the bug). - Mocks the three alert helpers at the test module level so the four dispatcher tests assert which helper was invoked by its sentinel string, instead of a generic ReactNode check. Closes #FXA-13777
vpomerleau
force-pushed
the
fix/passkey-timeout-cleanup
branch
from
3bd9994 to
5fd63de
Compare
Contributor
Author
|
Working on splitting this into two distinct PRs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Because
gave no help affordance for the WebAuthn anti-fingerprinting bucket —
FXA-13805, FXA-13806.
Firefox's duplicate-credential cancel because the synthetic
AuthenticatorAlreadyRegistered entry used NotSupported as its errorType —
FXA-13876.
ceremony threw NotSupportedError from a runtime refusal (e.g. Bitwarden +
macOS) — FXA-13777.
This pull request
catch-all alert ("Passkey setup couldn't be completed.") with a SUMO
Learn-more link; Timeout, NotSupported, and the duplicate-credential case
each get distinct copy + Glean buckets (`abort`, `not_readable`,
`already_registered`). Per-error Learn-more click events added.
`gleanReason` so both Firefox-collapsed and spec-compliant
duplicate-credential cases route consistently. NotSupportedError no longer
strands users on the loading spinner — it navigates back to settings.
`webauthn.ts`. NotSupported and Security now log to Sentry (likely config
bugs); AuthenticatorAlreadyRegistered no longer does (user-action).
Issue that this pull request solves
Closes: FXA-13805
Closes: FXA-13806
Closes: FXA-13876
Closes: FXA-13777
Checklist
Put an `x` in the boxes that apply
How to review (Optional)
Screenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Any other information that is important to this pull request.