diff --git a/backend/src/app.ts b/backend/src/app.ts index 9e7e1a5..46957b5 100755 --- a/backend/src/app.ts +++ b/backend/src/app.ts @@ -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); diff --git a/backend/src/customerSession.ts b/backend/src/customerSession.ts new file mode 100644 index 0000000..f1a4ea8 --- /dev/null +++ b/backend/src/customerSession.ts @@ -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 { + 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 { + setSessionCookie(res, await createSession(customerId)); +} diff --git a/backend/src/passkeys/signatureCounter.ts b/backend/src/passkeys/signatureCounter.ts new file mode 100644 index 0000000..6d96bb2 --- /dev/null +++ b/backend/src/passkeys/signatureCounter.ts @@ -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' + }; +} diff --git a/backend/src/routes/customers.ts b/backend/src/routes/customers.ts index 47db4f5..f1f9ee3 100755 --- a/backend/src/routes/customers.ts +++ b/backend/src/routes/customers.ts @@ -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 { - 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 diff --git a/backend/src/routes/passkeyLogin.ts b/backend/src/routes/passkeyLogin.ts new file mode 100644 index 0000000..59884bc --- /dev/null +++ b/backend/src/routes/passkeyLogin.ts @@ -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 { + 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( + `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; diff --git a/backend/tests/unit/signatureCounter.test.ts b/backend/tests/unit/signatureCounter.test.ts new file mode 100644 index 0000000..a2a14e5 --- /dev/null +++ b/backend/tests/unit/signatureCounter.test.ts @@ -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'); + }); +});