Merge pull request 'feat(passkeys): registration ceremony (#38)' (#331) from feature/38-passkey-registration into main

Reviewed-on: #331

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;`);
+};

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);

backend/src/db-kysely/schema.ts

@@ -79,6 +79,7 @@ export interface CustomerCredentials {
   customer_id: number;
   id: Generated<number>;
   last_used_at: Timestamp | null;
+  name: Generated<string>;
   public_key: string;
   signature_counter: Generated<Int8>;
   transports: string | null;

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);
+}

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<string | null> {
+  const { rows } = await pool.query<ChallengeRow>(
+    `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<CredentialIdRow>(
+      `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;

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);
+  });
+});