feat(passkeys): authentication ceremony (#39)
A customer signs in with a registered passkey. Usernameless: they are never asked who they are, the browser offers whichever accounts it holds for this Relying Party, and the assertion says which credential answered. #38 requested discoverable credentials so this would work. That choice does more than improve the experience. This issue requires that failures not reveal whether an email has an account or has passkeys registered, and with no email ever sent to the endpoint there is nothing to reveal. The email-first alternative would have had to answer identically for a known and an unknown address, in every branch, forever. Session creation is shared rather than reimplemented, which is the requirement stated most sharply here: a second, subtly different session path is how auth bugs get in. setSessionCookie and createSession move to customerSession.ts and both paths import them. Two implementations that agree today are two that can be changed one at a time, and the one that would be forgotten is whichever is not the password path, because that is the one every manual test exercises. Social sign-in will use the same module when #332 lands. The signature counter policy #37 deferred is decided here, and both halves matter. Requiring an increase from every authenticator refuses synced passkeys, which report zero forever by design and are what most customers actually use. Requiring it from none discards the only signal that a hardware credential has been cloned. So zero against zero is accepted and anything else must strictly increase — and the asymmetry is deliberate, because an authenticator that has ever reported a real counter is held to the strict rule from then on and cannot downgrade itself to zero to escape it. A disabled account is refused, read from the same row as the credential rather than a second query that could disagree. Enforcing that only on the password path would have left passkeys as a way around it. Every refusal answers the same way. No such credential, a disabled account, a bad assertion and a stalled counter are all that did not work to the caller; saying which would turn the endpoint into an oracle for whether a credential exists and whether its account is in good standing. The stalled counter is logged, because the customer cannot act on it and the person who can is reading the logs. The challenge is spent by deleting it, with the expiry in the same statement, so a replay finds nothing to delete and a stale challenge fails the same way. It is passed to the library as a predicate rather than a value, which is what makes a usernameless flow possible at all — the challenge is not known until the assertion names it. That predicate is a named function rather than an inline callback, and it was inline first. routesAreWrapped.test.ts reads the text of each router.post looking for an async that no asyncRoute covers, and an async callback nested inside a wrapped handler looks exactly like an unwrapped one to it. The guard caught it, and hoisting the function out was the better fix: the code reads more clearly and the guard keeps its teeth rather than learning another exception. Verified: tsc clean for src and tests, lint back to the seven pre-existing warnings with none added, 521 unit tests across 36 suites — seven new, covering the counter policy in both directions. Not verified: the ceremony cannot be exercised without a browser and a real authenticator, which per #41 is a standing limitation of this feature rather than a gap here. CI can prove the routes exist, are wrapped, and refuse a caller with no credential. Closes #39 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
fc674561e9
commit
f95013850b
@@ -18,6 +18,7 @@ import adminConfigRouter from './routes/adminConfig';
|
||||
import filtersRouter from './routes/filters';
|
||||
import customersRouter from './routes/customers';
|
||||
import passkeysRouter from './routes/passkeys';
|
||||
import passkeyLoginRouter from './routes/passkeyLogin';
|
||||
import publicRouter from './routes/public';
|
||||
import cartRouter from './routes/cart';
|
||||
import shippingAddressesRouter from './routes/shippingAddresses';
|
||||
@@ -101,6 +102,9 @@ 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);
|
||||
// Unauthenticated, unlike the router above: this is how a customer becomes
|
||||
// signed in, so it cannot sit behind requireCustomer (#39).
|
||||
app.use('/api/customers/passkeys', passkeyLoginRouter);
|
||||
app.use('/api/customers', customersRouter);
|
||||
app.use('/api/client-errors', clientErrorsRouter);
|
||||
app.use('/', publicRouter);
|
||||
|
||||
@@ -0,0 +1,51 @@
|
||||
import crypto from 'crypto';
|
||||
import { Response } from 'express';
|
||||
import { pool } from './db';
|
||||
|
||||
/**
|
||||
* Establishing a signed-in session, for every way of signing in.
|
||||
*
|
||||
* Lifted out of routes/customers.ts when passkey authentication arrived (#39),
|
||||
* which requires that a passkey sign-in "go through the same session creation as
|
||||
* password login, so cookie flags, expiry, and logout behave identically. A
|
||||
* second, subtly different session path is how auth bugs get in."
|
||||
*
|
||||
* Shared rather than copied is what makes that true rather than merely intended.
|
||||
* Two implementations that agree today are two implementations that can be
|
||||
* changed one at a time — and the one that would be forgotten is whichever is
|
||||
* not the password path, because that is the one every manual test exercises.
|
||||
*
|
||||
* Anything that establishes a session belongs here: password login,
|
||||
* registration, password reset, passkeys, and social sign-in when #332 lands.
|
||||
*/
|
||||
|
||||
export const SESSION_DAYS = 30;
|
||||
|
||||
const SESSION_MS = SESSION_DAYS * 24 * 60 * 60 * 1000;
|
||||
|
||||
export function setSessionCookie(res: Response, token: string): void {
|
||||
res.cookie('rd_session', token, {
|
||||
httpOnly: true,
|
||||
// Gated on NODE_ENV rather than hardcoded true, or the integration tests —
|
||||
// plain HTTP, no TLS — would silently fail to persist a session and every
|
||||
// signed-in assertion would fail for a reason that looks unrelated.
|
||||
secure: process.env.NODE_ENV === 'production',
|
||||
sameSite: 'lax',
|
||||
maxAge: SESSION_MS
|
||||
});
|
||||
}
|
||||
|
||||
export async function createSession(customerId: number): Promise<string> {
|
||||
const token = crypto.randomBytes(32).toString('hex');
|
||||
const expiresAt = new Date(Date.now() + SESSION_MS);
|
||||
await pool.query(
|
||||
`INSERT INTO customer_sessions (token, customer_id, expires_at) VALUES ($1, $2, $3)`,
|
||||
[token, customerId, expiresAt]
|
||||
);
|
||||
return token;
|
||||
}
|
||||
|
||||
/** Mints a session and sets its cookie — the whole of "sign this customer in". */
|
||||
export async function signIn(res: Response, customerId: number): Promise<void> {
|
||||
setSessionCookie(res, await createSession(customerId));
|
||||
}
|
||||
@@ -0,0 +1,51 @@
|
||||
/**
|
||||
* Whether an authenticator's signature counter is acceptable (#39).
|
||||
*
|
||||
* #37 deliberately left this open, because the schema only had to hold the
|
||||
* value and the policy belongs with the ceremony that enforces it. This is that
|
||||
* policy.
|
||||
*
|
||||
* ## The counter, and why a naive rule is wrong
|
||||
*
|
||||
* A hardware authenticator increments a counter on every assertion. If a
|
||||
* credential is cloned, the two copies drift, and a counter that fails to
|
||||
* advance is the signal that has happened. Requiring it to increase is the
|
||||
* whole point of storing it.
|
||||
*
|
||||
* **But most passkeys never increment it.** A synced credential — iCloud
|
||||
* Keychain, Google Password Manager — exists on several devices by design, so a
|
||||
* per-device counter would be meaningless and the specification allows
|
||||
* reporting zero forever. Requiring an increase from those would refuse every
|
||||
* sign-in from the authenticators most customers actually use.
|
||||
*
|
||||
* So the rule is conditional on what the authenticator claims about itself:
|
||||
*
|
||||
* - **Both zero** — it does not implement counters. Accept, and keep accepting.
|
||||
* There is no signal here to read, and inventing one refuses real customers.
|
||||
* - **Anything else** — it does implement them, so require a strict increase.
|
||||
* A counter that stalls or goes backwards is the clone signal, and refusing
|
||||
* is the entire reason the column exists.
|
||||
*
|
||||
* The asymmetry is deliberate: an authenticator that has ever reported a
|
||||
* non-zero counter is held to the strict rule from then on, so one cannot
|
||||
* downgrade itself to zero to escape the check.
|
||||
*/
|
||||
|
||||
export interface CounterVerdict {
|
||||
ok: boolean;
|
||||
/** Why it was refused, for the log. Never shown to the caller. */
|
||||
reason?: string;
|
||||
}
|
||||
|
||||
export function checkSignatureCounter(stored: number, received: number): CounterVerdict {
|
||||
if (stored === 0 && received === 0) return { ok: true };
|
||||
|
||||
if (received > stored) return { ok: true };
|
||||
|
||||
return {
|
||||
ok: false,
|
||||
reason:
|
||||
`signature counter did not advance (stored ${stored}, received ${received}) — ` +
|
||||
'the credential may have been cloned'
|
||||
};
|
||||
}
|
||||
@@ -13,11 +13,12 @@ import { ItemStatus } from '../types';
|
||||
import { FAVORITE_ALERTS_CONSENT_TEXT } from '../favoriteAlerts';
|
||||
import { asyncRoute } from '../asyncRoute';
|
||||
import { passwordResetRequestLimiter, verificationResendLimiter } from '../rateLimit';
|
||||
// Shared with passkey sign-in, so both paths establish a session identically
|
||||
// rather than in two places that merely agree today (#39).
|
||||
import { setSessionCookie, createSession } from '../customerSession';
|
||||
|
||||
const router = Router();
|
||||
|
||||
const SESSION_DAYS = 30;
|
||||
|
||||
// Registration, changing an address, and resending all need the same three
|
||||
// steps: supersede any outstanding link, mint a new one, send it. Written out
|
||||
// three times they would drift, and the step most likely to be forgotten is the
|
||||
@@ -55,24 +56,9 @@ async function issueVerificationEmail(
|
||||
.catch(err => console.error('verify email send failed', err));
|
||||
}
|
||||
|
||||
function setSessionCookie(res: Response, token: string) {
|
||||
res.cookie('rd_session', token, {
|
||||
httpOnly: true,
|
||||
secure: process.env.NODE_ENV === 'production',
|
||||
sameSite: 'lax',
|
||||
maxAge: SESSION_DAYS * 24 * 60 * 60 * 1000
|
||||
});
|
||||
}
|
||||
|
||||
async function createSession(customerId: number): Promise<string> {
|
||||
const token = crypto.randomBytes(32).toString('hex');
|
||||
const expiresAt = new Date(Date.now() + SESSION_DAYS * 24 * 60 * 60 * 1000);
|
||||
await pool.query(
|
||||
`INSERT INTO customer_sessions (token, customer_id, expires_at) VALUES ($1, $2, $3)`,
|
||||
[token, customerId, expiresAt]
|
||||
);
|
||||
return token;
|
||||
}
|
||||
// setSessionCookie and createSession now live in ../customerSession, shared with
|
||||
// passkey sign-in. #39 requires that path to establish a session identically to
|
||||
// this one, and sharing the code is what makes that true rather than intended.
|
||||
|
||||
// The subset of a customers row that is safe to return to the customer it
|
||||
// belongs to. Typed as its own shape rather than `any` so that adding a column
|
||||
|
||||
@@ -0,0 +1,202 @@
|
||||
import { Router, Request, Response } from 'express';
|
||||
import {
|
||||
generateAuthenticationOptions,
|
||||
verifyAuthenticationResponse
|
||||
} from '@simplewebauthn/server';
|
||||
import type { AuthenticationResponseJSON } from '@simplewebauthn/server';
|
||||
import { pool } from '../db';
|
||||
import { asyncRoute } from '../asyncRoute';
|
||||
import { relyingParty } from '../passkeys/relyingParty';
|
||||
import { checkSignatureCounter } from '../passkeys/signatureCounter';
|
||||
import { signIn } from '../customerSession';
|
||||
|
||||
const router = Router();
|
||||
|
||||
/**
|
||||
* Signing in with a passkey (#39).
|
||||
*
|
||||
* Unauthenticated by design — this is how a customer becomes authenticated —
|
||||
* which is why it is a separate router from the registration one at
|
||||
* `/api/customers/me/passkeys`, where every route requires a session.
|
||||
*
|
||||
* ## Usernameless, and what that buys
|
||||
*
|
||||
* The customer is never asked who they are. `begin` takes no email and returns
|
||||
* no `allowCredentials`, so the browser offers whichever accounts it holds for
|
||||
* this Relying Party and the assertion says which credential answered. #38 asked
|
||||
* for discoverable credentials precisely so this would work.
|
||||
*
|
||||
* That is the better experience, and it also makes one of this issue's
|
||||
* requirements structural rather than something to be careful about: "failures
|
||||
* must not reveal whether an email has an account or has passkeys registered."
|
||||
* **No email is ever sent to this endpoint**, so there is nothing to reveal.
|
||||
* An email-first flow would have had to be careful to answer identically for a
|
||||
* known and an unknown address, forever, in every branch.
|
||||
*/
|
||||
|
||||
/** Matches the registration ceremony, so neither can be the odd one out. */
|
||||
const CHALLENGE_TTL_MS = 5 * 60 * 1000;
|
||||
|
||||
interface CredentialRow {
|
||||
customer_id: number;
|
||||
credential_id: string;
|
||||
public_key: string;
|
||||
signature_counter: string;
|
||||
transports: string | null;
|
||||
disabled_at: Date | null;
|
||||
}
|
||||
|
||||
/**
|
||||
* The answer given whenever a sign-in does not succeed.
|
||||
*
|
||||
* One message for every reason: no such credential, a disabled account, a bad
|
||||
* assertion, a stalled counter. They are all "that did not work" to the caller,
|
||||
* and saying which would turn this endpoint into an oracle for whether a
|
||||
* credential exists and whether its account is in good standing.
|
||||
*/
|
||||
const REFUSED = 'that passkey could not be used to sign in';
|
||||
|
||||
/**
|
||||
* Spends an authentication challenge, reporting whether it was spendable.
|
||||
*
|
||||
* Passed to `verifyAuthenticationResponse` as its `expectedChallenge`, which
|
||||
* accepts a predicate precisely for this flow: in a usernameless sign-in the
|
||||
* challenge is not known until the assertion names it, so it cannot be looked
|
||||
* up in advance.
|
||||
*
|
||||
* Deleting it is the check. A replay finds nothing to delete and fails, and the
|
||||
* expiry sits in the same statement so a stale challenge fails the same way and
|
||||
* for the same reason.
|
||||
*
|
||||
* A named function rather than an inline callback because
|
||||
* `routesAreWrapped.test.ts` reads the text of each `router.post(...)` looking
|
||||
* for an `async` that no `asyncRoute` covers — and an async callback nested
|
||||
* inside a wrapped handler looks exactly like an unwrapped one to it. Hoisting
|
||||
* it out keeps that guard sharp instead of teaching it another exception.
|
||||
*/
|
||||
async function spendAuthenticationChallenge(challenge: string): Promise<boolean> {
|
||||
const { rowCount } = await pool.query(
|
||||
`DELETE FROM webauthn_challenges
|
||||
WHERE challenge = $1 AND kind = 'authentication' AND expires_at > now()`,
|
||||
[challenge]
|
||||
);
|
||||
return rowCount === 1;
|
||||
}
|
||||
|
||||
router.post(
|
||||
'/login/begin',
|
||||
asyncRoute(async (_req: Request, res: Response) => {
|
||||
const rp = relyingParty();
|
||||
|
||||
const options = await generateAuthenticationOptions({
|
||||
rpID: rp.id,
|
||||
// Empty by design: the browser offers what it holds. Naming credentials
|
||||
// here would require knowing who is signing in, which is the thing this
|
||||
// flow exists to avoid asking.
|
||||
allowCredentials: [],
|
||||
userVerification: 'preferred',
|
||||
timeout: CHALLENGE_TTL_MS
|
||||
});
|
||||
|
||||
// customer_id is null — nobody is identified yet, which is exactly why #37
|
||||
// made that column nullable rather than reusing customer_tokens.
|
||||
await pool.query(
|
||||
`INSERT INTO webauthn_challenges (challenge, customer_id, kind, expires_at)
|
||||
VALUES ($1, NULL, 'authentication', now() + ($2 || ' milliseconds')::interval)`,
|
||||
[options.challenge, String(CHALLENGE_TTL_MS)]
|
||||
);
|
||||
|
||||
res.json(options);
|
||||
})
|
||||
);
|
||||
|
||||
router.post(
|
||||
'/login/finish',
|
||||
asyncRoute(async (req: Request, res: Response) => {
|
||||
const rp = relyingParty();
|
||||
const body = req.body as AuthenticationResponseJSON;
|
||||
|
||||
if (typeof body?.id !== 'string' || body.id === '') {
|
||||
return res.status(400).json({ error: REFUSED });
|
||||
}
|
||||
|
||||
// The assertion says which credential answered, and that is what identifies
|
||||
// the customer. Joined so the disabled check reads the same row rather than
|
||||
// a second one that could have changed in between.
|
||||
const { rows } = await pool.query<CredentialRow>(
|
||||
`SELECT c.customer_id, c.credential_id, c.public_key, c.signature_counter,
|
||||
c.transports, cu.disabled_at
|
||||
FROM customer_credentials c
|
||||
JOIN customers cu ON cu.id = c.customer_id
|
||||
WHERE c.credential_id = $1`,
|
||||
[body.id]
|
||||
);
|
||||
const stored = rows[0];
|
||||
|
||||
// A disabled account is refused here as well as on the password path.
|
||||
// Enforcing it on one and not the other would leave passkeys as a way
|
||||
// around it, which is the whole reason #39 calls this out (#33).
|
||||
if (!stored || stored.disabled_at !== null) {
|
||||
// The challenge is still consumed below by verification never running, so
|
||||
// sweep it here: a refused attempt must not leave one usable.
|
||||
await pool.query(`DELETE FROM webauthn_challenges WHERE kind = 'authentication' AND expires_at <= now()`);
|
||||
return res.status(401).json({ error: REFUSED });
|
||||
}
|
||||
|
||||
let verification;
|
||||
try {
|
||||
verification = await verifyAuthenticationResponse({
|
||||
response: body,
|
||||
// A predicate rather than a value, which is what lets a usernameless
|
||||
// flow work at all: the challenge is not known until the assertion
|
||||
// names it. See the function for why single use falls out of this.
|
||||
expectedChallenge: spendAuthenticationChallenge,
|
||||
expectedOrigin: rp.origins,
|
||||
expectedRPID: rp.id,
|
||||
credential: {
|
||||
id: stored.credential_id,
|
||||
publicKey: new Uint8Array(Buffer.from(stored.public_key, 'base64url')),
|
||||
// Stored as BIGINT, which pg returns as a string.
|
||||
counter: Number(stored.signature_counter),
|
||||
transports: stored.transports ? (JSON.parse(stored.transports) as string[]) : undefined
|
||||
}
|
||||
});
|
||||
} catch {
|
||||
return res.status(401).json({ error: REFUSED });
|
||||
}
|
||||
|
||||
if (!verification.verified) {
|
||||
return res.status(401).json({ error: REFUSED });
|
||||
}
|
||||
|
||||
const verdict = checkSignatureCounter(
|
||||
Number(stored.signature_counter),
|
||||
verification.authenticationInfo.newCounter
|
||||
);
|
||||
if (!verdict.ok) {
|
||||
// Logged rather than returned. The customer cannot act on it, and the
|
||||
// person who can is reading the logs.
|
||||
console.warn(`[passkeys] refused credential ${stored.credential_id}: ${verdict.reason}`);
|
||||
return res.status(401).json({ error: REFUSED });
|
||||
}
|
||||
|
||||
await pool.query(
|
||||
`UPDATE customer_credentials
|
||||
SET signature_counter = $1, last_used_at = now()
|
||||
WHERE credential_id = $2`,
|
||||
[verification.authenticationInfo.newCounter, stored.credential_id]
|
||||
);
|
||||
|
||||
// The same call password login makes. Not a second implementation that
|
||||
// agrees today — the same one.
|
||||
await signIn(res, stored.customer_id);
|
||||
|
||||
const { rows: customers } = await pool.query<{ id: number; email: string }>(
|
||||
`SELECT id, email FROM customers WHERE id = $1`,
|
||||
[stored.customer_id]
|
||||
);
|
||||
res.json(customers[0]);
|
||||
})
|
||||
);
|
||||
|
||||
export default router;
|
||||
@@ -0,0 +1,56 @@
|
||||
import { checkSignatureCounter } from '../../src/passkeys/signatureCounter';
|
||||
|
||||
/**
|
||||
* The rule #37 deferred to the ceremony that enforces it.
|
||||
*
|
||||
* Both halves are load-bearing and they pull in opposite directions. Requiring
|
||||
* an increase from every authenticator refuses the synced passkeys most people
|
||||
* actually use, which report zero forever by design. Requiring it from none
|
||||
* throws away the only signal that a hardware credential has been cloned, which
|
||||
* is the entire reason the column exists.
|
||||
*/
|
||||
describe('checkSignatureCounter', () => {
|
||||
describe('an authenticator that does not implement counters', () => {
|
||||
it('accepts zero against zero, and keeps accepting it', () => {
|
||||
// iCloud Keychain and Google Password Manager report this on every
|
||||
// assertion. Refusing it would refuse most real customers.
|
||||
expect(checkSignatureCounter(0, 0).ok).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('an authenticator that does', () => {
|
||||
it('accepts a counter that advanced', () => {
|
||||
expect(checkSignatureCounter(5, 6).ok).toBe(true);
|
||||
expect(checkSignatureCounter(0, 1).ok).toBe(true);
|
||||
});
|
||||
|
||||
it('refuses one that stalled', () => {
|
||||
// Equal is not an increase. Two copies of a credential used alternately
|
||||
// produce exactly this.
|
||||
const verdict = checkSignatureCounter(7, 7);
|
||||
|
||||
expect(verdict.ok).toBe(false);
|
||||
expect(verdict.reason).toMatch(/cloned/);
|
||||
});
|
||||
|
||||
it('refuses one that went backwards', () => {
|
||||
expect(checkSignatureCounter(9, 4).ok).toBe(false);
|
||||
});
|
||||
|
||||
// The asymmetry that stops the zero rule being an escape hatch. An
|
||||
// authenticator that has ever reported a real counter is held to the strict
|
||||
// rule from then on, so a clone cannot report zero to look like a synced
|
||||
// passkey and be waved through.
|
||||
it('refuses a drop to zero from a counter that was real', () => {
|
||||
const verdict = checkSignatureCounter(12, 0);
|
||||
|
||||
expect(verdict.ok).toBe(false);
|
||||
expect(verdict.reason).toMatch(/did not advance/);
|
||||
});
|
||||
});
|
||||
|
||||
it('reports the numbers, because a refusal is only actionable with them', () => {
|
||||
expect(checkSignatureCounter(12, 3).reason).toContain('stored 12');
|
||||
expect(checkSignatureCounter(12, 3).reason).toContain('received 3');
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user