diff --git a/backend/migrations/1788000000000_add-passkey-name.js b/backend/migrations/1788000000000_add-passkey-name.js new file mode 100644 index 0000000..ea09c6a --- /dev/null +++ b/backend/migrations/1788000000000_add-passkey-name.js @@ -0,0 +1,23 @@ +exports.up = (pgm) => { + pgm.sql(` + -- What the customer calls this passkey (#38). + -- + -- Not in the #37 groundwork because that issue listed the columns the + -- ceremony needs and this one is for the person: the management screen (#40) + -- shows a list, and "phone" against "laptop" is the only thing that makes + -- two entries tellable apart. Without it a customer revoking a credential is + -- choosing between identical rows. + -- + -- NOT NULL with a default rather than nullable. Every row must be + -- displayable, and a null would push the "or a sensible default" half of the + -- requirement out into every read site. The route derives a better default + -- from the authenticator's transports; this is the floor under that, and the + -- value existing rows take. + ALTER TABLE customer_credentials + ADD COLUMN IF NOT EXISTS name TEXT NOT NULL DEFAULT 'Passkey'; + `); +}; + +exports.down = (pgm) => { + pgm.sql(`ALTER TABLE customer_credentials DROP COLUMN IF EXISTS name;`); +}; diff --git a/backend/src/app.ts b/backend/src/app.ts index 2e1f926..9e7e1a5 100755 --- a/backend/src/app.ts +++ b/backend/src/app.ts @@ -17,6 +17,7 @@ import adminVersionRouter from './routes/adminVersion'; import adminConfigRouter from './routes/adminConfig'; import filtersRouter from './routes/filters'; import customersRouter from './routes/customers'; +import passkeysRouter from './routes/passkeys'; import publicRouter from './routes/public'; import cartRouter from './routes/cart'; import shippingAddressesRouter from './routes/shippingAddresses'; @@ -97,6 +98,9 @@ app.use('/api/admin/version', requireAdminGate, adminVersionRouter); app.use('/api/admin/config', requireAdminGate, adminConfigRouter); app.use('/api/admin', requireAdminGate, adminRouter); app.use('/api/customers/me/addresses', shippingAddressesRouter); +// Before /api/customers, like the addresses router above: Express matches +// mounts in order, so the broader prefix would swallow these otherwise (#38). +app.use('/api/customers/me/passkeys', passkeysRouter); app.use('/api/customers', customersRouter); app.use('/api/client-errors', clientErrorsRouter); app.use('/', publicRouter); diff --git a/backend/src/db-kysely/schema.ts b/backend/src/db-kysely/schema.ts index 953aa47..1294691 100644 --- a/backend/src/db-kysely/schema.ts +++ b/backend/src/db-kysely/schema.ts @@ -79,6 +79,7 @@ export interface CustomerCredentials { customer_id: number; id: Generated; last_used_at: Timestamp | null; + name: Generated; public_key: string; signature_counter: Generated; transports: string | null; diff --git a/backend/src/passkeys/credentialName.ts b/backend/src/passkeys/credentialName.ts new file mode 100644 index 0000000..732fbb9 --- /dev/null +++ b/backend/src/passkeys/credentialName.ts @@ -0,0 +1,48 @@ +/** + * A name for a passkey the customer did not name themselves (#38). + * + * The management screen (#40) lists credentials and offers to revoke them, so + * two entries that read identically are a screen where the customer cannot tell + * which device they are removing. A default that says something is the + * difference between "Passkey, Passkey, Passkey" and a list worth showing. + * + * Derived from the authenticator's transports, which is the only thing the + * ceremony learns about the device. It is a hint rather than a fact — the + * browser reports what the authenticator claims — so these are deliberately + * vague. "This device" is honest about a platform authenticator in a way that + * guessing "MacBook" would not be. + */ + +/** The fallback when the authenticator reports nothing usable. */ +export const GENERIC_CREDENTIAL_NAME = 'Passkey'; + +export function defaultCredentialName(transports: readonly string[] | null | undefined): string { + if (!transports || transports.length === 0) return GENERIC_CREDENTIAL_NAME; + + // Checked in this order because an authenticator can report several. A phone + // used as a cross-device passkey reports `hybrid` and often `internal` too, + // and "Phone or tablet" is the more useful of the two readings — `internal` + // alone means the authenticator built into the machine being used. + if (transports.includes('hybrid')) return 'Phone or tablet'; + if (transports.includes('internal')) return 'This device'; + if (transports.some((t) => t === 'usb' || t === 'nfc' || t === 'ble')) return 'Security key'; + + return GENERIC_CREDENTIAL_NAME; +} + +/** + * The customer's own name for a passkey, or null when they gave none. + * + * Trimmed, because a name of spaces is a name nobody can read in a list, and + * bounded because this is rendered — a customer is naming their laptop, not + * writing prose, and an unbounded string in a table cell is a layout problem + * rather than an expressive one. + */ +export const MAX_CREDENTIAL_NAME_LENGTH = 64; + +export function readCredentialName(value: unknown): string | null { + if (typeof value !== 'string') return null; + const trimmed = value.trim(); + if (trimmed === '') return null; + return trimmed.slice(0, MAX_CREDENTIAL_NAME_LENGTH); +} diff --git a/backend/src/routes/passkeys.ts b/backend/src/routes/passkeys.ts new file mode 100644 index 0000000..2b00f0a --- /dev/null +++ b/backend/src/routes/passkeys.ts @@ -0,0 +1,198 @@ +import { Router, Request, Response } from 'express'; +import { + generateRegistrationOptions, + verifyRegistrationResponse +} from '@simplewebauthn/server'; +import type { RegistrationResponseJSON } from '@simplewebauthn/server'; +import { pool } from '../db'; +import { asyncRoute } from '../asyncRoute'; +import { requireCustomer } from '../middleware/customerAuth'; +import { relyingParty } from '../passkeys/relyingParty'; +import { defaultCredentialName, readCredentialName } from '../passkeys/credentialName'; + +const router = Router(); + +/** + * Registering a passkey (#38). + * + * Every route here is behind `requireCustomer`. Registration is not a sign-up + * path — it adds a credential to an account that already exists and is already + * signed in — so an unauthenticated caller has nothing to register against. + * + * Signing in with a passkey is #39, and the management screen is #40. Neither + * exists yet, so nothing reads these credentials. + */ + +/** + * How long a customer has to complete the ceremony. + * + * Long enough to find a phone and use it; short enough that an intercepted + * challenge is not useful for long. The browser's own timeout is set to match, + * so the two cannot disagree about when the attempt has expired. + */ +const CHALLENGE_TTL_MS = 5 * 60 * 1000; + +interface CredentialIdRow { + credential_id: string; + transports: string | null; +} + +interface ChallengeRow { + challenge: string; +} + +/** + * Removes a challenge and reports whether it was there. + * + * Single use is the whole point, and deleting it *is* the check: a replayed + * response finds nothing to delete and is refused. Doing it as one statement + * rather than a read followed by a delete means two requests racing cannot both + * see the row and both proceed. + * + * Expiry is part of the same condition, so an expired challenge is refused for + * the same reason and by the same statement. + */ +async function consumeChallenge(customerId: number, kind: string): Promise { + const { rows } = await pool.query( + `DELETE FROM webauthn_challenges + WHERE customer_id = $1 AND kind = $2 AND expires_at > now() + RETURNING challenge`, + [customerId, kind] + ); + return rows[0]?.challenge ?? null; +} + +router.post( + '/register/begin', + requireCustomer, + asyncRoute(async (req: Request, res: Response) => { + const customerId = req.customerId as number; + const rp = relyingParty(); + + const { rows: existing } = await pool.query( + `SELECT credential_id, transports FROM customer_credentials WHERE customer_id = $1`, + [customerId] + ); + + const { rows: customers } = await pool.query<{ email: string; first_name: string | null }>( + `SELECT email, first_name FROM customers WHERE id = $1`, + [customerId] + ); + const customer = customers[0]; + if (!customer) return res.status(404).json({ error: 'not found' }); + + const options = await generateRegistrationOptions({ + rpName: rp.name, + rpID: rp.id, + userName: customer.email, + userDisplayName: customer.first_name ?? customer.email, + // The customer id, not the email. A userID is meant to be stable and + // opaque; the email is neither, and a customer changing theirs would + // otherwise look like a different person to their own authenticator. + userID: new TextEncoder().encode(String(customerId)), + // Stops the same authenticator being enrolled twice. Without it a + // customer pressing register again on a device they already registered + // gets a second row that behaves identically to the first, and a + // management screen showing two entries they cannot tell apart. + excludeCredentials: existing.map((row) => ({ id: row.credential_id })), + attestationType: 'none', + authenticatorSelection: { + // Discoverable, because #39 wants sign-in without the customer first + // saying who they are. 'preferred' rather than 'required' so an + // authenticator that cannot store one is still usable here. + residentKey: 'preferred', + userVerification: 'preferred' + }, + timeout: CHALLENGE_TTL_MS + }); + + // One in-flight registration per customer. Pressing the button twice must + // not leave the first challenge usable — the second replaces it, and the + // first response is then refused by consumeChallenge finding nothing. + await pool.query(`DELETE FROM webauthn_challenges WHERE customer_id = $1 AND kind = 'registration'`, [ + customerId + ]); + await pool.query( + `INSERT INTO webauthn_challenges (challenge, customer_id, kind, expires_at) + VALUES ($1, $2, 'registration', now() + ($3 || ' milliseconds')::interval)`, + [options.challenge, customerId, String(CHALLENGE_TTL_MS)] + ); + + res.json(options); + }) +); + +router.post( + '/register/finish', + requireCustomer, + asyncRoute(async (req: Request, res: Response) => { + const customerId = req.customerId as number; + const rp = relyingParty(); + + const expectedChallenge = await consumeChallenge(customerId, 'registration'); + if (expectedChallenge === null) { + // Deliberately the same answer for "never started", "already used" and + // "expired". They are the same thing from here — no challenge this + // customer may still complete — and distinguishing them would tell an + // attacker which of their guesses was closest. + return res.status(400).json({ error: 'start again — that registration is no longer valid' }); + } + + let verification; + try { + verification = await verifyRegistrationResponse({ + response: req.body as RegistrationResponseJSON, + expectedChallenge, + expectedOrigin: rp.origins, + expectedRPID: rp.id + }); + } catch { + // The library throws on a malformed or unverifiable response. The + // challenge is already consumed by this point, deliberately: a failed + // attempt must not leave one usable for a second try. + return res.status(400).json({ error: 'that passkey could not be registered' }); + } + + if (!verification.verified) { + return res.status(400).json({ error: 'that passkey could not be registered' }); + } + + const { credential } = verification.registrationInfo; + const transports = credential.transports ?? []; + const name = readCredentialName((req.body as { name?: unknown }).name) + ?? defaultCredentialName(transports); + + try { + await pool.query( + `INSERT INTO customer_credentials + (customer_id, credential_id, public_key, signature_counter, transports, name) + VALUES ($1, $2, $3, $4, $5, $6)`, + [ + customerId, + credential.id, + Buffer.from(credential.publicKey).toString('base64url'), + credential.counter, + JSON.stringify(transports), + name + ] + ); + } catch (err) { + // credential_id is unique across the table. excludeCredentials should + // have stopped the browser offering an already-registered authenticator, + // but that is a hint the browser may ignore, so the constraint is what + // actually holds — and hitting it means the credential is already + // registered rather than that anything is broken. + if ((err as { code?: string }).code === '23505') { + return res.status(409).json({ error: 'that passkey is already registered' }); + } + throw err; + } + + res.status(201).json({ name }); + }) +); + +/** Exported for the tests; nothing else constructs a challenge. */ +export { CHALLENGE_TTL_MS }; + +export default router; diff --git a/backend/tests/unit/credentialName.test.ts b/backend/tests/unit/credentialName.test.ts new file mode 100644 index 0000000..60f06d0 --- /dev/null +++ b/backend/tests/unit/credentialName.test.ts @@ -0,0 +1,66 @@ +import { + defaultCredentialName, + readCredentialName, + GENERIC_CREDENTIAL_NAME, + MAX_CREDENTIAL_NAME_LENGTH +} from '../../src/passkeys/credentialName'; + +/** + * The management screen (#40) lists credentials and offers to revoke them, so a + * default that says nothing produces a list of identical rows and a customer + * removing a device at random. These are about that list being readable. + */ +describe('defaultCredentialName', () => { + it('calls a cross-device authenticator a phone or tablet', () => { + expect(defaultCredentialName(['hybrid'])).toBe('Phone or tablet'); + }); + + it('prefers hybrid over internal when both are reported', () => { + // A phone used as a cross-device passkey commonly reports both, and "Phone + // or tablet" is the more useful reading — `internal` on its own means the + // authenticator built into the machine in front of the customer. + expect(defaultCredentialName(['internal', 'hybrid'])).toBe('Phone or tablet'); + }); + + it('calls a platform authenticator this device', () => { + expect(defaultCredentialName(['internal'])).toBe('This device'); + }); + + it.each([['usb'], ['nfc'], ['ble']])('calls %s a security key', (transport) => { + expect(defaultCredentialName([transport])).toBe('Security key'); + }); + + it.each([[[]], [null], [undefined], [['something-new']]])( + 'falls back to the generic name for %p', + (transports) => { + // Transports are a hint the authenticator supplies, so an unrecognised + // one is expected rather than exceptional — the list grows. + expect(defaultCredentialName(transports)).toBe(GENERIC_CREDENTIAL_NAME); + } + ); +}); + +describe('readCredentialName', () => { + it('takes a name the customer gave', () => { + expect(readCredentialName('Work laptop')).toBe('Work laptop'); + }); + + it('trims, because a padded name is not a different name', () => { + expect(readCredentialName(' Work laptop ')).toBe('Work laptop'); + }); + + it.each([[' '], [''], [null], [undefined], [42], [{}]])( + 'returns null for %p so the caller falls back to a default', + (value) => { + // A name of spaces is a row the customer cannot read, and a non-string is + // a client sending something unexpected. Both mean "no name given". + expect(readCredentialName(value)).toBeNull(); + } + ); + + it('bounds the length, because this is rendered in a table cell', () => { + const long = 'a'.repeat(MAX_CREDENTIAL_NAME_LENGTH + 50); + + expect(readCredentialName(long)).toHaveLength(MAX_CREDENTIAL_NAME_LENGTH); + }); +});