diff --git a/migration-graph.md b/migration-graph.md index 9b76090eb..962fba77a 100644 --- a/migration-graph.md +++ b/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) diff --git a/server/src/nest/auth/auth.dto.ts b/server/src/nest/auth/auth.dto.ts index d66a63849..2f806c855 100644 --- a/server/src/nest/auth/auth.dto.ts +++ b/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) {} diff --git a/server/src/nest/auth/passkey.controller.ts b/server/src/nest/auth/passkey.controller.ts index bd4540c32..a85c87e6b 100644 --- a/server/src/nest/auth/passkey.controller.ts +++ b/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!); diff --git a/server/src/nest/auth/passkey.service.ts b/server/src/nest/auth/passkey.service.ts index c8e86a2f0..60ce165e2 100644 --- a/server/src/nest/auth/passkey.service.ts +++ b/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 }; } diff --git a/server/tests/unit/nest/passkey.service.test.ts b/server/tests/unit/nest/passkey.service.test.ts index 4788475d1..20000ccea 100644 --- a/server/tests/unit/nest/passkey.service.test.ts +++ b/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; + expect(row).toEqual({ name: 'Original', user_id: user.id }); + expect(testDb.prepare('SELECT COUNT(*) AS n FROM webauthn_credentials').get()).toEqual({ n: 1 }); + }); }); // ── passkeyLoginOptions ─────────────────────────────────────────────────────── diff --git a/shared/src/auth/auth.schema.spec.ts b/shared/src/auth/auth.schema.spec.ts index b5281dbb6..bd479ef67 100644 --- a/shared/src/auth/auth.schema.spec.ts +++ b/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); + }); }); diff --git a/shared/src/auth/auth.schema.ts b/shared/src/auth/auth.schema.ts index d3c53d511..50f69ecf2 100644 --- a/shared/src/auth/auth.schema.ts +++ b/shared/src/auth/auth.schema.ts @@ -139,3 +139,11 @@ export const passkeyRenameRequestSchema = z.object({ name: z.unknown().optional(), }); export type PasskeyRenameRequest = z.infer; + +// 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;