Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds frontend-side validation for WebAuthn RP ID values returned by the backend, to fail fast (with a clearer error) when passkey options are misconfigured for the current deployment hostname.
Changes:
- Added RP ID extraction + validation helpers to
src/utils/webauthn.ts. - Added unit tests for RP ID validation in
src/utils/webauthn.test.ts. - Integrated RP ID validation into the passkey registration/login flows in Signup, Signin, and Profile, with new/updated UI tests for Signup and Signin.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/webauthn.ts | Adds RP ID extraction/validation utilities and error formatting for passkey flows. |
| src/utils/webauthn.test.ts | Introduces tests covering RP ID validation and extraction behavior. |
| src/pages/Signup.tsx | Validates register options RP ID before calling navigator.credentials.create. |
| src/pages/Signup.test.tsx | Adds a regression test ensuring invalid RP IDs fail before WebAuthn API calls. |
| src/pages/Signin.tsx | Validates authenticate options RP ID before calling navigator.credentials.get. |
| src/pages/Signin.test.tsx | Adds a regression test ensuring invalid RP IDs fail before WebAuthn API calls; updates existing options fixture. |
| src/pages/Profile.tsx | Validates register options RP ID before starting passkey registration from Profile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| interface WebAuthnRpIdValidationResult { | ||
| isValid: boolean; | ||
| message?: string; | ||
| rpId?: string; | ||
| currentHostname: string; | ||
| } |
There was a problem hiding this comment.
WebAuthnRpIdValidationResult marks message/rpId as optional, but the implementation always provides them for invalid results. This type is easy to misuse (e.g., showing an undefined error message) and doesn’t encode the invariant that message exists when isValid is false. Consider changing this to a discriminated union where the isValid: false branch requires message and rpId.
| interface WebAuthnRpIdValidationResult { | |
| isValid: boolean; | |
| message?: string; | |
| rpId?: string; | |
| currentHostname: string; | |
| } | |
| type WebAuthnRpIdValidationResult = | |
| | { | |
| isValid: true; | |
| currentHostname: string; | |
| rpId?: string; | |
| message?: undefined; | |
| } | |
| | { | |
| isValid: false; | |
| currentHostname: string; | |
| rpId: string; | |
| message: string; | |
| }; |
| if (!rpId) { | ||
| return { isValid: true, currentHostname }; | ||
| } | ||
|
|
||
| const normalizedRpId = rpId.trim().toLowerCase(); | ||
| const normalizedHostname = currentHostname.trim().toLowerCase(); |
There was a problem hiding this comment.
When rpId is undefined, the function returns { currentHostname } without normalizing it, but all other branches return a trimmed/lowercased hostname. This makes the result shape inconsistent for callers that log/display currentHostname. Consider normalizing currentHostname up front and always returning the normalized value.
| if (!rpId) { | |
| return { isValid: true, currentHostname }; | |
| } | |
| const normalizedRpId = rpId.trim().toLowerCase(); | |
| const normalizedHostname = currentHostname.trim().toLowerCase(); | |
| const normalizedHostname = currentHostname.trim().toLowerCase(); | |
| if (!rpId) { | |
| return { isValid: true, currentHostname: normalizedHostname }; | |
| } | |
| const normalizedRpId = rpId.trim().toLowerCase(); |
| const rpIdValidation = validateWebAuthnOptionsRpId(rawOptions); | ||
| if (!rpIdValidation.isValid) { | ||
| console.error("Invalid WebAuthn register RP ID", rpIdValidation); | ||
| throw new Error(rpIdValidation.message); |
There was a problem hiding this comment.
rpIdValidation.message is typed as optional, so new Error(rpIdValidation.message) can produce an Error with an undefined message (which would surface to the user as an unhelpful toast). Either provide a fallback message here or adjust the validation result type so message is required when isValid is false.
| throw new Error(rpIdValidation.message); | |
| throw new Error( | |
| rpIdValidation.message ?? "Invalid WebAuthn register RP ID", | |
| ); |
| const rpIdValidation = validateWebAuthnOptionsRpId(rawOptions); | ||
| if (!rpIdValidation.isValid) { | ||
| console.error("Invalid WebAuthn authenticate RP ID", rpIdValidation); | ||
| throw new Error(rpIdValidation.message); |
There was a problem hiding this comment.
rpIdValidation.message is optional in the return type, so throwing new Error(rpIdValidation.message) can result in an undefined/empty error message being shown to users. Consider adding a fallback message at this call site, or make the validator return type guarantee message when isValid is false.
| throw new Error(rpIdValidation.message); | |
| throw new Error( | |
| rpIdValidation.message || "Invalid WebAuthn authenticate RP ID", | |
| ); |
No description provided.