Conversation
🦋 Changeset detectedLatest commit: 1fd50db The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/blocks
@emdash-cms/cloudflare
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
masonjames
changed the title
There was a problem hiding this comment.
Pull request overview
Adjusts passkey verification error handling so expected WebAuthn/authentication failures return a normal 401 response instead of being converted into a 500 (PASSKEY_VERIFY_ERROR), and adds a regression test for the “unregistered credential” case.
Changes:
- Map a set of known passkey authentication failure errors to
401 UNAUTHORIZED. - Add a unit test asserting unregistered credentials return 401 with the standard error shape.
- Add a changeset documenting the patch release.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/core/src/astro/routes/api/auth/passkey/verify.ts | Adds detection of expected passkey auth failures and returns apiError(..., 401) for those cases. |
| packages/core/tests/unit/auth/passkey-verify-route.test.ts | Adds regression coverage for unregistered credential assertions returning 401 instead of 500. |
| .changeset/smart-swans-camp.md | Records a patch-level release note for the passkey auth failure handling change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const EXPECTED_AUTH_FAILURE_MESSAGES = [ | ||
| "Credential not found", | ||
| "Challenge not found or expired", | ||
| "Invalid challenge type", | ||
| "Challenge expired", | ||
| "Invalid client data type", | ||
| "Invalid origin:", | ||
| "Invalid RP ID hash", | ||
| "User presence not verified", | ||
| "Invalid signature counter", | ||
| "Invalid signature", | ||
| ]; | ||
|
|
||
| function isExpectedPasskeyAuthFailure(error: unknown): boolean { | ||
| return ( | ||
| error instanceof Error && | ||
| EXPECTED_AUTH_FAILURE_MESSAGES.some((message) => error.message.startsWith(message)) |
There was a problem hiding this comment.
The 401 mapping relies on matching error.message prefixes. This duplicates the literal throw new Error("...") strings in @emdash-cms/auth (see packages/auth/src/passkey/authenticate.ts) and will silently regress back to 500s if those messages change. Consider switching to a typed error (e.g., a dedicated PasskeyAuthError class or code field) from the auth package, or at least centralizing these prefixes in a shared export so core and auth stay in sync.
| const EXPECTED_AUTH_FAILURE_MESSAGES = [ | |
| "Credential not found", | |
| "Challenge not found or expired", | |
| "Invalid challenge type", | |
| "Challenge expired", | |
| "Invalid client data type", | |
| "Invalid origin:", | |
| "Invalid RP ID hash", | |
| "User presence not verified", | |
| "Invalid signature counter", | |
| "Invalid signature", | |
| ]; | |
| function isExpectedPasskeyAuthFailure(error: unknown): boolean { | |
| return ( | |
| error instanceof Error && | |
| EXPECTED_AUTH_FAILURE_MESSAGES.some((message) => error.message.startsWith(message)) | |
| const EXPECTED_AUTH_FAILURE_MESSAGES = new Set([ | |
| "Credential not found", | |
| "Challenge not found or expired", | |
| "Invalid challenge type", | |
| "Challenge expired", | |
| "Invalid client data type", | |
| "Invalid RP ID hash", | |
| "User presence not verified", | |
| "Invalid signature counter", | |
| "Invalid signature", | |
| ]); | |
| const INVALID_ORIGIN_AUTH_FAILURE_PATTERN = /^Invalid origin:\s.+$/; | |
| function isExpectedPasskeyAuthFailure(error: unknown): boolean { | |
| if (!(error instanceof Error)) { | |
| return false; | |
| } | |
| return ( | |
| EXPECTED_AUTH_FAILURE_MESSAGES.has(error.message) || | |
| INVALID_ORIGIN_AUTH_FAILURE_PATTERN.test(error.message) |
| let user; | ||
| try { | ||
| user = await authenticateWithPasskey(passkeyConfig, adapter, body.credential, challengeStore); | ||
| } catch (error) { | ||
| if (isExpectedPasskeyAuthFailure(error)) { | ||
| return apiError("UNAUTHORIZED", "Authentication failed", 401); | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
let user; leaves user untyped here and requires mutation across the try/catch. Restructuring to keep const user = … within a single try block (and returning the success response from there) would be clearer and avoids accidentally widening user to any/unknown as this code evolves.
|
Updated to use typed passkey auth errors and added regression coverage. |
| throw new Error(`Invalid origin: expected ${config.origin}, got ${clientData.origin}`); | ||
| throw new PasskeyAuthenticationError( | ||
| "invalid_origin", | ||
| `Invalid origin: expected ${config.origin}, got ${clientData.origin}`, |
There was a problem hiding this comment.
| `Invalid origin: expected ${config.origin}, got ${clientData.origin}`, | |
| "Origin mismatch", |
I realise this was pre-existing, but it's probably best to keep this generic and not risk leaking details
What does this PR do?
Passkey verification currently reports expected authentication failures as 500s. For example, if a browser submits a passkey assertion for a credential that is not registered in the local EmDash database, the route throws
Credential not foundand the generic catch block turns it intoPASSKEY_VERIFY_ERROR.This maps expected WebAuthn assertion failures to a normal 401 response while preserving the existing 500 path for unexpected server/configuration errors. It also adds a regression test for the unregistered credential case.
Closes: N/A
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runpnpm locale:extracthas been run (if applicable)AI-generated code disclosure
Screenshots / test output
No screenshots; API behavior only.
Validated with:
pnpm typecheckpnpm vitest run tests/unit/auth/passkey-verify-route.test.tspnpm --silent lint:quickpnpm formatFull lint note:
pnpm --silent lint:json | jq '.diagnostics | length'currently reports 12 pre-existing warnings in unrelated files; this PR does not add diagnostics in the changed files.