fix(server): tidy passkey quirks after the DI fold

migration-graph.md

@@ -35,9 +35,11 @@ constructor stub, and the test helper's `RESET_TABLES` gained the two
 webauthn tables. A sibling DTO ratchet cleared the last four
 `PasskeyController` allow-list entries with deliberately permissive schemas
 (password optional, ceremony payloads `z.unknown()`) so the bespoke 401/400
-strings stay service rules; the DELETE route's `{ password }` body remains
-outside the POST/PUT/PATCH boot gate. No trailing quirk commit yet — the
-faithfully-carried quirks await their own `fix(server)` pass. The day
+strings stay service rules. The trailing `fix(server)` commit then cleared
+the two verified defects the relocation had faithfully carried — the DELETE
+route's untyped `{ password }` body (a non-string 500'd in bcrypt) and the
+un-transactioned dup-check→INSERT in registerVerify — see "Quirks fixed
+after the passkey fold" below. The day
 before, the oidcService fold — the first frontier cash-in of the auth fold,
 landed the same day as auth, while the notifications fan-in stayed the
 order's official next step: the 508-line SSO module folded whole into the existing thin wrapper
@@ -803,11 +805,45 @@ Wave-2 `permissions` + `auditLog` pair were the first frontier picks — all don
   DB with `@simplewebauthn/server` mocked at the ceremony-verdict boundary, the repo's
   first mock of that library — and `tests/helpers/test-db.ts` gained the two webauthn
   tables in `RESET_TABLES` (they were never reset before; the suites that touched them
-  did so via user-cascade only). No trailing quirk commit yet — the faithfully-carried
-  quirks (the redundant duplicate-credential pre-check beside the UNIQUE constraint,
-  the un-transactioned check→INSERT→re-select in registerVerify, the DELETE route's
-  `{ password }` body sitting outside the POST/PUT/PATCH body-contract gate) await
-  their own `fix(server)` pass.
+  did so via user-cascade only). The trailing `fix(server)` commit then cleared the two
+  faithfully-carried quirks that were verified defects — the DELETE route's untyped
+  `{ password }` body and the un-transactioned check→INSERT in registerVerify — see
+  "Quirks fixed after the passkey fold" below; the rest (cross-user challenge burn,
+  counter-0 clone exemption, unthrottled register/verify) are deliberate and stay.
+
+## Quirks fixed after the passkey fold (the trailing `fix(server)` commit)
+
+The relocation itself was byte-identical; these two were then fixed on top, each with a
+regression pin, so the parity diff and the behaviour change stayed in separate commits:
+
+- **The DELETE route's `{ password }` body could 500.** `remove` was the one passkey
+  handler left untyped — DELETE sits outside the POST/PUT/PATCH body-contract boot
+  gate, so the ratchet never forced it — and a non-string password
+  (`{"password": 123}`) sailed past the `body?.password` optional-chain into
+  `bcrypt.compareSync`, which throws `Illegal arguments` → a 500 envelope. The body is
+  now typed with `PasskeyDeleteDto` over the new shared `passkeyDeleteRequestSchema`
+  (`password: z.string().optional()`, same permissive doctrine as the other four
+  passkey schemas), so malformed input gets the pipe's 400 while a missing or wrong
+  password still reaches the service's bespoke `'Incorrect password'` 401.
+- **registerVerify's dup-check → INSERT pair is now one transaction.** The legacy code
+  pre-checked `credential_id` uniqueness and then inserted as two independent
+  statements, with a catch-all mapping every insert failure to the generic 400 — so
+  the bespoke 409 depended entirely on the pre-check statement winning, and the pair
+  violated the repo's multi-statement-write transaction rule. Both now run inside
+  `db.transaction()` with a reference-compared duplicate sentinel (the oidc
+  `invite_exhausted` precedent): duplicate → the exact legacy
+  `'This passkey is already registered.'` 409, any other insert failure → the exact
+  legacy `'Could not register this passkey.'` 400, and a failed insert rolls back with
+  no partial state (PASSKEY-SVC-031/032 pin the rollback and the untouched original
+  row; the spent challenge stays spent — single-use is not undone by a failed insert).
+
+Deliberately NOT changed (documented, spec-correct or contract): the cross-user
+registration-challenge claim burning the challenge (single-use semantics), the
+counter-0 clone-detection exemption (synced passkeys legitimately never increment),
+`sanitizeName`'s truncate-before-empty-check order, the management routes staying
+outside `PasskeyEnabledGuard`, the shared `login` rate-limit bucket, `register/verify`
+staying unthrottled (JWT + a single-use claimed challenge already gate it), and the
+`credential.counter ?? 0` nullish default (legacy-original).
 
 ## Quirks fixed after the oidc fold (the trailing `fix(server)` commit)
 

server/src/nest/auth/auth.dto.ts

@@ -18,6 +18,7 @@ import {
   passkeyRegisterVerifyRequestSchema,
   passkeyLoginVerifyRequestSchema,
   passkeyRenameRequestSchema,
+  passkeyDeleteRequestSchema,
 } from '@trek/shared';
 
 /**
@@ -46,3 +47,4 @@ export class PasskeyRegisterOptionsDto extends createZodDto(passkeyRegisterOptio
 export class PasskeyRegisterVerifyDto extends createZodDto(passkeyRegisterVerifyRequestSchema) {}
 export class PasskeyLoginVerifyDto extends createZodDto(passkeyLoginVerifyRequestSchema) {}
 export class PasskeyRenameDto extends createZodDto(passkeyRenameRequestSchema) {}
+export class PasskeyDeleteDto extends createZodDto(passkeyDeleteRequestSchema) {}

server/src/nest/auth/passkey.controller.ts

@@ -8,7 +8,7 @@ import { setAuthCookie } from '../../services/cookie';
 import { getClientIp } from '../audit/client-ip';
 import { AuditService } from '../audit/audit.service';
 import { PasskeyService } from './passkey.service';
-import { PasskeyRegisterOptionsDto, PasskeyRegisterVerifyDto, PasskeyLoginVerifyDto, PasskeyRenameDto } from './auth.dto';
+import { PasskeyRegisterOptionsDto, PasskeyRegisterVerifyDto, PasskeyLoginVerifyDto, PasskeyRenameDto, PasskeyDeleteDto } from './auth.dto';
 import type { User } from '../../types';
 
 const WINDOW = 15 * 60 * 1000;
@@ -110,7 +110,7 @@ export class PasskeyController {
 
   @Delete('credentials/:id')
   @UseGuards(JwtAuthGuard)
-  remove(@CurrentUser() user: User, @Param('id') id: string, @Body() body: { password?: string }, @Req() req: Request) {
+  remove(@CurrentUser() user: User, @Param('id') id: string, @Body() body: PasskeyDeleteDto, @Req() req: Request) {
     this.limit('login', req, 5);
     const result = this.passkeys.deletePasskey(user.id, id, body?.password);
     if (result.error) throw new HttpException({ error: result.error }, result.status!);

server/src/nest/auth/passkey.service.ts

@@ -32,6 +32,11 @@ const NOT_CONFIGURED = { error: 'Passkey login is not configured for this server
 // used to tell "no such credential" apart from "bad signature" (CWE-203).
 const AUTH_FAILED = { error: 'Authentication failed', status: 401 } as const;
 
+// Reference-compared sentinel (oidc invite_exhausted precedent): thrown inside
+// the register transaction to keep the duplicate 409 distinct from the generic
+// insert-failure 400 without string-matching SQLite errors.
+const DUPLICATE_CREDENTIAL = new Error('duplicate credential');
+
 interface CredentialRow {
   id: number;
   user_id: number;
@@ -205,27 +210,34 @@ export class PasskeyService {
     // from the raw client payload.
     const { credential, credentialDeviceType, credentialBackedUp, aaguid } = verification.registrationInfo;
 
-    if (this.db.get('SELECT id FROM webauthn_credentials WHERE credential_id = ?', credential.id)) {
-      return { error: 'This passkey is already registered.', status: 409 };
-    }
-
     const name = sanitizeName(body?.name) || defaultCredentialName(credentialDeviceType);
+    // Duplicate check + INSERT in one transaction so the UNIQUE race can't slip
+    // between them; the sentinel keeps the legacy 409-vs-400 split intact.
     try {
-      this.db.run(
-        `INSERT INTO webauthn_credentials
-           (user_id, credential_id, public_key, counter, transports, device_type, backed_up, name, aaguid, last_used_at)
-         VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, NULL)`,
-        userId,
-        credential.id,
-        Buffer.from(credential.publicKey),
-        credential.counter ?? 0,
-        credential.transports ? JSON.stringify(credential.transports) : null,
-        credentialDeviceType ?? null,
-        credentialBackedUp ? 1 : 0,
-        name,
-        aaguid ?? null,
-      );
-    } catch {
+      this.db.transaction((conn) => {
+        if (conn.prepare('SELECT id FROM webauthn_credentials WHERE credential_id = ?').get(credential.id)) {
+          throw DUPLICATE_CREDENTIAL;
+        }
+        conn.prepare(
+          `INSERT INTO webauthn_credentials
+             (user_id, credential_id, public_key, counter, transports, device_type, backed_up, name, aaguid, last_used_at)
+           VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, NULL)`,
+        ).run(
+          userId,
+          credential.id,
+          Buffer.from(credential.publicKey),
+          credential.counter ?? 0,
+          credential.transports ? JSON.stringify(credential.transports) : null,
+          credentialDeviceType ?? null,
+          credentialBackedUp ? 1 : 0,
+          name,
+          aaguid ?? null,
+        );
+      });
+    } catch (err) {
+      if (err === DUPLICATE_CREDENTIAL) {
+        return { error: 'This passkey is already registered.', status: 409 };
+      }
       return { error: 'Could not register this passkey.', status: 400 };
     }
 

server/tests/unit/nest/passkey.service.test.ts

@@ -1,9 +1,11 @@
 /**
  * Unit tests for the DI-native PasskeyService — PASSKEY-SVC-001 through
- * PASSKEY-SVC-030. Written fresh with the DI fold: the legacy
+ * PASSKEY-SVC-032. Written fresh with the DI fold: the legacy
  * services/passkeyService had no service-level suite, so these characterize
  * the relocated behavior (challenge store semantics, WebAuthn ceremony error
- * paths, clone detection, management CRUD) over a real in-memory SQLite DB.
+ * paths, clone detection, management CRUD) over a real in-memory SQLite DB;
+ * 031–032 pin the post-fold `fix(server)` quirk fix (the transactional
+ * dup-check → INSERT with its 409-vs-400 sentinel split).
  * The @simplewebauthn/server ceremony functions are mocked — the library's
  * crypto is not under test, the service's handling of its verdicts is.
  * Constructed directly (no TestingModule, repo convention).
@@ -320,6 +322,30 @@ describe('passkeyRegisterVerify', () => {
     expect(await svc.passkeyRegisterVerify(999_999, { attestationResponse: ceremonyResponse('c7') }))
       .toEqual({ error: 'Could not register this passkey.', status: 400 });
   });
+
+  // 031/032 pin the post-fold quirk fix: the dup check + INSERT run in one
+  // transaction, keeping the 409-vs-400 split and rolling back cleanly.
+  it('PASSKEY-SVC-031: a failed INSERT rolls back without partial credential state', async () => {
+    swMock.verifyRegistrationResponse.mockResolvedValue(registrationVerdict());
+    testDb.exec('PRAGMA foreign_keys = OFF');
+    seedChallenge('c8', 999_999, 'registration');
+    testDb.exec('PRAGMA foreign_keys = ON');
+    await svc.passkeyRegisterVerify(999_999, { attestationResponse: ceremonyResponse('c8') });
+    expect(testDb.prepare('SELECT COUNT(*) AS n FROM webauthn_credentials').get()).toEqual({ n: 0 });
+    expect(challengeCount()).toBe(0); // the claim stays spent — the challenge is not resurrected
+  });
+
+  it('PASSKEY-SVC-032: the in-transaction duplicate 409 leaves the original row untouched', async () => {
+    const { user } = createUser(testDb);
+    insertCredential(user.id, { credential_id: 'dup-cred-2', name: 'Original' });
+    seedChallenge('c9', user.id, 'registration');
+    swMock.verifyRegistrationResponse.mockResolvedValue(registrationVerdict({}, { id: 'dup-cred-2' }));
+    expect(await svc.passkeyRegisterVerify(user.id, { attestationResponse: ceremonyResponse('c9'), name: 'Impostor' }))
+      .toEqual({ error: 'This passkey is already registered.', status: 409 });
+    const row = testDb.prepare('SELECT name, user_id FROM webauthn_credentials WHERE credential_id = ?').get('dup-cred-2') as Record<string, unknown>;
+    expect(row).toEqual({ name: 'Original', user_id: user.id });
+    expect(testDb.prepare('SELECT COUNT(*) AS n FROM webauthn_credentials').get()).toEqual({ n: 1 });
+  });
 });
 
 // ── passkeyLoginOptions ───────────────────────────────────────────────────────

shared/src/auth/auth.schema.spec.ts

@@ -17,6 +17,7 @@ import {
   passkeyRegisterVerifyRequestSchema,
   passkeyLoginVerifyRequestSchema,
   passkeyRenameRequestSchema,
+  passkeyDeleteRequestSchema,
 } from './auth.schema';
 
 import { describe, it, expect } from 'vitest';
@@ -130,4 +131,10 @@ describe('passkey schemas', () => {
     expect(passkeyRenameRequestSchema.safeParse({}).success).toBe(true);
     expect(passkeyRenameRequestSchema.safeParse({ name: 42 }).success).toBe(true); // service-side sanitize owns the 400
   });
+
+  it('delete rejects a non-string password (used to 500 in bcrypt) but keeps omission a service rule', () => {
+    expect(passkeyDeleteRequestSchema.safeParse({ password: 'pw' }).success).toBe(true);
+    expect(passkeyDeleteRequestSchema.safeParse({}).success).toBe(true);
+    expect(passkeyDeleteRequestSchema.safeParse({ password: 123 }).success).toBe(false);
+  });
 });

shared/src/auth/auth.schema.ts

@@ -139,3 +139,11 @@ export const passkeyRenameRequestSchema = z.object({
   name: z.unknown().optional(),
 });
 export type PasskeyRenameRequest = z.infer<typeof passkeyRenameRequestSchema>;
+
+// DELETE /credentials/:id — outside the POST/PUT/PATCH boot gate, but typed
+// anyway: a non-string password used to reach bcrypt.compareSync and 500.
+// Optional keeps the missing-password 401 a service rule.
+export const passkeyDeleteRequestSchema = z.object({
+  password: z.string().optional(),
+});
+export type PasskeyDeleteRequest = z.infer<typeof passkeyDeleteRequestSchema>;