Skip to content

fix(passkey): don't strand an account when a registration is submitted twice - #91

Merged
ralyodio merged 1 commit into
moshcoder:mainfrom
clawedassistant26:fix/passkey-duplicate-registration
Jul 30, 2026
Merged

fix(passkey): don't strand an account when a registration is submitted twice#91
ralyodio merged 1 commit into
moshcoder:mainfrom
clawedassistant26:fix/passkey-duplicate-registration

Conversation

@clawedassistant26

Copy link
Copy Markdown
Contributor

The bug

mc_c_reg is only cleared on the way out of a successful verify, so two requests that are already in flight both pass the ceremony check at the top of /auth/passkey/register/verify. That happens on an ordinary double click, or when a client retries after a slow first attempt — no crafted cookies, the browser legitimately still holds mc_c_reg until the first reply lands.

Both requests then call createUserPasskey() before either writes its credential. webauthn_credentials.id is the primary key, so the second insert always loses, and the loss is unhandled:

  • an account is left behind with no passkey, so nobody can ever sign in to it
  • seedDefaults() already paid the 100-credit SIGNUP_BONUS to it — a second time, for one registration
  • express 4 does not catch async rejections, so the losing request never gets a response, and the rejection takes the process down with it

Repro

Against unmodified main (b03f3b1), real passkeyRouter, real verifyRegistrationResponse, throwaway libsql, no stubs and no module mocking. One registration response, submitted twice concurrently:

submit A: 200 {"ok":true,"redirect":"/"}
submit B: NO RESPONSE (timed out)
!! unhandled rejection: SQLITE_CONSTRAINT_PRIMARYKEY

=> expected: 1 user, 1 credential, 100 credits, 0 orphans
=> actual:   2 users, 1 credential, 200 credits, 1 orphan

The test builds a genuine "none"-attestation response (P-256 COSE key, UP|UV|AT flags) with the library's own CBOR encoder, so your real verifier runs — nothing about the WebAuthn path is faked.

The fix

Catch the failed insert, undo the user that attempt created, and answer the loser with a plain 400. channels and credit_ledger are both ON DELETE CASCADE and PRAGMA foreign_keys is on under libsql (checked both, didn't assume), so the row takes the seeded channels and the signup bonus with it. The two users have distinct ids, so the loser only ever deletes its own. ceremony.existing skips the delete — that path never created a user.

Non-constraint failures get a 500 and a different message, so a transient DB error isn't reported to the user as "already registered".

28 insertions / 11 deletions, 11 of the removals being the re-indent of the existing INSERT into the try.

Tests

New apps/pwa/test/passkey-register-duplicate.test.mjs, 7 tests, same shape as sessions-output-seq.test.mjs (real router, throwaway db, skips cleanly when the PWA deps aren't installed).

3 are the bug:

  • a double-submitted registration creates exactly one account, with no orphan
  • the signup bonus is paid once (100 credits, not 200)
  • the losing submit answers 400 instead of hanging, and nothing rejects unhandled — asserted on a recorded unhandledRejection, since that one is fatal in production

4 are controls that pass both ways:

  • a single registration still creates the account, sets mc_sess, clears mc_c_reg
  • two separate registrations still create two accounts and two bonuses
  • a verify with no ceremony cookie is rejected and writes nothing
  • an unverifiable attestation is rejected and writes nothing

Fail-before by git checkout --: 3 fail / 4 pass unpatched, 7/7 patched. apps/pwa 97 → 104, root 313 → 320, 0 failures.

Deliberately scoped out

  • Making the challenge single-use server-side. That is the root cause, and WebAuthn wants challenges consumed exactly once, but the ceremony lives entirely in a signed cookie — there is no server-side record to mark. Doing it properly needs a table and a migration, and it is a design call about where that state should live. Happy to do it separately if you want it.
  • userById() returning null on the ceremony.existing path (account deleted mid-ceremony) throws on user.id and crashes the same way. One line to guard, but it is a different defect — say the word and I will send it.

…d twice

mc_c_reg is only cleared on the way out of a successful verify, so two
requests that are already in flight - a double click, or a client retry
after a slow first attempt - both pass the ceremony check and both create
a user before either writes its credential.

The credential id is the primary key, so the second insert always loses.
Unhandled, that left behind an account with no passkey to sign in with,
paid the 100-credit signup bonus a second time, and rejected inside the
request: express 4 does not catch async rejections, so the losing request
never got a response and the rejection took the process down with it.

Catch the failed insert, undo the user that attempt created (channels and
the signup bonus cascade with the row), and answer the loser with a plain
400 instead.
@ralyodio
ralyodio merged commit b396d28 into moshcoder:main Jul 30, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants