fix(auth): give a sign-in code a budget of guesses and a short life

A six-digit code has a million values, and nothing was counting how many
of them a caller tried. The code travelled in an encrypted cookie the
caller held, verification compared against that cookie, and a wrong
answer simply re-rendered the form. Nobody has to be the person the code
was mailed to: type somebody else's address into the first screen and the
code goes to their mailbox while the cookie stays with you. At that point
the only thing between a stranger and an account is a million requests,
and the constant-time comparison protecting the code was guarding a door
you could just keep knocking on.

Guesses are now counted on the server, under a name that changes with
every code. That placement is the point: a counter kept beside the code,
in the cookie, is a counter the guesser can wind back by replaying an
older copy. Starting over is still allowed and still costs a fresh code
sent to the mailbox being aimed at, which is where somebody notices. A
correct code spends its record too, so its remaining guesses do not carry
into the next one.

The cookie also lived for twenty-four hours, which made the pin a
password with a million possible values and a day to try them. Ten
minutes now, and the code stops being accepted when the clock says so
rather than when the cookie happens to go away.

Resend had no limit either, so the button was a way to mail a stranger as
fast as requests go out. Codes to one address are spaced, and one attempt
at signing in can only ask for so many.

Both refusals say the same thing on purpose. Which of the two it was is a
fact about somebody else's mailbox.

apps/auth/test/worker.test.ts

@@ -95,8 +95,14 @@ function jar() {
 	};
 }
 
-/** Ask for a code, redeem it, and come back holding tokens. */
+/**
-async function signIn() {
+ * Ask for a code, redeem it, and come back holding tokens.
+ *
+ * The address is a parameter because codes to one mailbox are rate limited, and
+ * two sign-ins in the same second are exactly what that limit is for. Each
+ * caller uses its own.
+ */
+async function signIn(email: string) {
 	const client = createClient({
 		issuer: 'https://auth.internal',
 		clientID: 'api',
@@ -115,7 +121,7 @@ async function signIn() {
 	const requested = await auth.request('https://auth.internal/code/authorize', {
 		method: 'POST',
 		headers: { cookie: cookies.header(), 'content-type': 'application/x-www-form-urlencoded' },
-		body: new URLSearchParams({ action: 'request', email: 'ada@example.com' })
+		body: new URLSearchParams({ action: 'request', email })
 	});
 	cookies.absorb(requested);
 	expect(lastCode).not.toBe('');
@@ -142,7 +148,7 @@ async function signIn() {
 
 describe('signing in with an email address', () => {
 	test('a redeemed code becomes tokens that verify', async () => {
-		const { client, tokens } = await signIn();
+		const { client, tokens } = await signIn('ada@example.com');
 
 		expect(tokens.access).toBeString();
 		expect(tokens.refresh).toBeString();
@@ -161,7 +167,7 @@ describe('signing in with an email address', () => {
 
 describe('User info', () => {
 	test('returns subject properties for valid access token', async () => {
-		const { tokens } = await signIn();
+		const { tokens } = await signIn('grace@example.com');
 
 		const infoRes = await auth.request('https://auth.internal/userinfo', {
 			headers: { Authorization: `Bearer ${tokens.access}` }

packages/auth/src/provider/code.ts

@@ -54,7 +54,8 @@
  */
 import { Context } from 'hono';
 
-import { generateUnbiasedDigits, timingSafeCompare } from '../random.js';
+import { generateUnbiasedDigits, generateUnbiasedString, timingSafeCompare } from '../random.js';
+import { Storage } from '../storage/storage.js';
 import { Provider } from './provider.js';
 
 export interface CodeProviderConfig<
@@ -66,6 +67,46 @@ export interface CodeProviderConfig<
 	 * @default 6
 	 */
 	length?: number;
+	/**
+	 * How long a code stays usable, in seconds.
+	 *
+	 * A pin is six digits, which is a small space, and the only thing keeping
+	 * it small enough to type is that it does not have to last. A code that is
+	 * still good tomorrow is a password with a million possible values.
+	 *
+	 * @default 600
+	 */
+	ttl?: number;
+	/**
+	 * How many wrong guesses a code survives.
+	 *
+	 * Counted where the person asking cannot reach it, which is the whole
+	 * point: the code itself travels in an encrypted cookie the caller holds,
+	 * so a counter kept alongside it would be a counter they could reset by
+	 * replaying an older copy. Starting over is allowed and costs them a fresh
+	 * code — sent to the mailbox they are trying to break into, where somebody
+	 * notices.
+	 *
+	 * @default 5
+	 */
+	maxAttempts?: number;
+	/**
+	 * How many codes one attempt at signing in may ask for.
+	 *
+	 * @default 3
+	 */
+	maxSends?: number;
+	/**
+	 * Seconds between one code and the next for the same claim.
+	 *
+	 * Without this, `resend` is an open relay pointed at anybody's mailbox: the
+	 * address is not the caller's own and nothing asks them to prove otherwise,
+	 * so the send button is a way to mail a stranger as fast as requests go
+	 * out.
+	 *
+	 * @default 30
+	 */
+	resendInterval?: number;
 	/**
 	 * The request handler to generate the UI for the code flow.
 	 *
@@ -116,6 +157,14 @@ export type CodeProviderState =
 			resend?: boolean;
 			code: string;
 			claims: Record<string, string>;
+			/**
+			 * Names the server-side record holding this code's remaining
+			 * guesses. Regenerated with every code, so a caller who rolls back
+			 * to an older cookie rolls back to a code that is no longer live.
+			 */
+			flow: string;
+			/** When the code stops being accepted, in ms. */
+			expires: number;
 	  };
 
 /**
@@ -134,16 +183,50 @@ export type CodeProviderError =
 			type: 'invalid_claim';
 			key: string;
 			value: string;
+	  }
+	/** Too many guesses, or codes asked for too quickly. */
+	| {
+			type: 'rate_limit';
 	  };
 
+/** Nothing a person reads, so the whole alphabet is available. */
+const FLOW_ALPHABET = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789';
+
 export function CodeProvider<Claims extends Record<string, string> = Record<string, string>>(
 	config: CodeProviderConfig<Claims>
 ): Provider<{ claims: Claims }> {
 	const length = config.length || 6;
+	const ttl = config.ttl ?? 60 * 10;
+	const maxAttempts = config.maxAttempts ?? 5;
+	const maxSends = config.maxSends ?? 3;
+	const resendInterval = config.resendInterval ?? 30;
+
 	function generate() {
 		return generateUnbiasedDigits(length);
 	}
 
+	/** Where a flow's remaining guesses live, on the server. */
+	function attemptKey(flow: string) {
+		return ['oauth:code:flow', flow];
+	}
+
+	/**
+	 * Where the last send to one claim is remembered.
+	 *
+	 * Keyed by the claim and not by the caller, because the mailbox is what is
+	 * being protected and the caller is whoever is pointing at it. Two people
+	 * asking for a code for one address in the same minute is the case this is
+	 * for, and it is the same case whether they are the same person or not.
+	 */
+	function claimKey(claims: Record<string, string>) {
+		const flattened = Object.entries(claims)
+			.filter(([key]) => key !== 'action')
+			.map(([key, value]) => `${key}=${String(value).trim().toLowerCase()}`)
+			.sort()
+			.join('&');
+		return ['oauth:code:claim', flattened];
+	}
+
 	return {
 		type: 'code',
 		init(routes, ctx) {
@@ -153,10 +236,14 @@ export function CodeProvider<Claims extends Record<string, string> = Record<stri
 				fd?: FormData,
 				err?: CodeProviderError
 			) {
-				await ctx.set<CodeProviderState>(c, 'provider', 60 * 60 * 24, next);
+				// The cookie lives exactly as long as the code inside it.
+				// Twenty-four hours, which is what this was, made a six-digit
+				// pin usable for a day.
+				await ctx.set<CodeProviderState>(c, 'provider', ttl, next);
 				const resp = ctx.forward(c, await config.request(c.req.raw, next, fd, err));
 				return resp;
 			}
+
 			routes.get('/authorize', async (c) => {
 				const resp = await transition(c, {
 					type: 'start'
@@ -165,7 +252,6 @@ export function CodeProvider<Claims extends Record<string, string> = Record<stri
 			});
 
 			routes.post('/authorize', async (c) => {
-				const code = generate();
 				const fd = await c.req.formData();
 				const state = await ctx.get<CodeProviderState>(c, 'provider');
 				const action = fd.get('action')?.toString();
@@ -173,22 +259,83 @@ export function CodeProvider<Claims extends Record<string, string> = Record<stri
 				if (action === 'request' || action === 'resend') {
 					const claims = Object.fromEntries(fd) as Claims;
 					delete claims.action;
+
+					// Asked for too soon, or too many times for one attempt.
+					// Both answers are the same on purpose: saying which would
+					// tell a caller whether the address they typed has had a
+					// code sent to it lately, which is a fact about somebody
+					// else's mailbox.
+					const sentAt = await Storage.get<{ at: number }>(ctx.storage, claimKey(claims));
+					if (sentAt && Date.now() - sentAt.at < resendInterval * 1000) {
+						return transition(c, state ?? { type: 'start' }, fd, { type: 'rate_limit' });
+					}
+					if (action === 'resend' && state?.type === 'code') {
+						const record = await Storage.get<{ attempts: number; sends: number }>(
+							ctx.storage,
+							attemptKey(state.flow)
+						);
+						if ((record?.sends ?? 1) >= maxSends) {
+							return transition(c, state, fd, { type: 'rate_limit' });
+						}
+					}
+
+					const code = generate();
 					const err = await config.sendCode(claims, code);
 					if (err) return transition(c, { type: 'start' }, fd, err);
+
+					// A new code means a new flow, which means a fresh budget
+					// of guesses — and, more to the point, that the budget
+					// attached to the previous code is now unreachable rather
+					// than reset.
+					const flow = generateUnbiasedString(FLOW_ALPHABET, 32);
+					const sends =
+						action === 'resend' && state?.type === 'code'
+							? ((
+									await Storage.get<{ sends: number }>(ctx.storage, attemptKey(state.flow))
+								)?.sends ?? 1) + 1
+							: 1;
+					await Storage.set(ctx.storage, attemptKey(flow), { attempts: 0, sends }, ttl);
+					await Storage.set(ctx.storage, claimKey(claims), { at: Date.now() }, resendInterval);
+
 					return transition(
 						c,
 						{
 							type: 'code',
 							resend: action === 'resend',
 							claims,
-							code
+							code,
+							flow,
+							expires: Date.now() + ttl * 1000
 						},
 						fd
 					);
 				}
 
-				if (fd.get('action')?.toString() === 'verify' && state.type === 'code') {
+				if (action === 'verify' && state?.type === 'code') {
-					const fd = await c.req.formData();
+					if (state.expires <= Date.now()) {
+						await ctx.unset(c, 'provider');
+						return transition(c, { type: 'start' }, fd, { type: 'invalid_code' });
+					}
+
+					// Counted before the comparison, so a guess costs whether or
+					// not it is right. Counted on the server, so the caller
+					// holding the cookie cannot wind it back.
+					const record = await Storage.get<{ attempts: number; sends: number }>(
+						ctx.storage,
+						attemptKey(state.flow)
+					);
+					if (!record || record.attempts >= maxAttempts) {
+						await ctx.unset(c, 'provider');
+						await Storage.remove(ctx.storage, attemptKey(state.flow));
+						return transition(c, { type: 'start' }, fd, { type: 'rate_limit' });
+					}
+					await Storage.set(
+						ctx.storage,
+						attemptKey(state.flow),
+						{ ...record, attempts: record.attempts + 1 },
+						Math.max(1, Math.ceil((state.expires - Date.now()) / 1000))
+					);
+
 					const compare = fd.get('code')?.toString();
 					if (!state.code || !compare || !timingSafeCompare(state.code, compare)) {
 						return transition(
@@ -201,9 +348,15 @@ export function CodeProvider<Claims extends Record<string, string> = Record<stri
 							{ type: 'invalid_code' }
 						);
 					}
+
+					// Spent. Without this the same code answers again, and the
+					// budget of guesses is per code rather than per sign-in.
+					await Storage.remove(ctx.storage, attemptKey(state.flow));
 					await ctx.unset(c, 'provider');
 					return ctx.forward(c, await ctx.success(c, { claims: state.claims as Claims }));
 				}
+
+				return transition(c, { type: 'start' }, fd);
 			});
 		}
 		};

packages/auth/src/ui/code.tsx

@@ -69,7 +69,13 @@ const DEFAULT_COPY = {
 	/**
 	 * Copy for the resend button.
 	 */
-	code_resend: 'Resend'
+	code_resend: 'Resend',
+	/**
+	 * Error message when too many codes have been asked for, or too many
+	 * guesses made. Deliberately one message for both: which of the two it was
+	 * is a fact about somebody else's mailbox.
+	 */
+	rate_limited: 'Too many attempts. Wait a moment and start again.'
 };
 
 export type CodeUICopy = typeof DEFAULT_COPY;
@@ -124,6 +130,7 @@ export function CodeUI(props: CodeUIOptions): CodeProviderOptions {
 					<Layout>
 						<form data-component="form" method="post">
 							{error?.type === 'invalid_claim' && <FormAlert message={copy.email_invalid} />}
+						{error?.type === 'rate_limit' && <FormAlert message={copy.rate_limited} />}
 							<input type="hidden" name="action" value="request" />
 							<input
 								data-component="input"
@@ -151,6 +158,7 @@ export function CodeUI(props: CodeUIOptions): CodeProviderOptions {
 					<Layout>
 						<form data-component="form" class="form" method="post">
 							{error?.type === 'invalid_code' && <FormAlert message={copy.code_invalid} />}
+						{error?.type === 'rate_limit' && <FormAlert message={copy.rate_limited} />}
 							{state.type === 'code' && (
 								<FormAlert
 									message={(state.resend ? copy.code_resent : copy.code_sent) + state.claims.email}

packages/auth/test/code.test.ts

@@ -0,0 +1,215 @@
+import { beforeEach, describe, expect, test } from 'bun:test';
+
+import { object, string } from 'valibot';
+
+import { CodeProvider } from '../src/provider/code.js';
+import { issuer } from '../src/issuer.js';
+import { MemoryStorage } from '../src/storage/memory.js';
+import { createSubjects } from '../src/subject.js';
+
+const subjects = createSubjects({ user: object({ email: string() }) });
+
+let sent: string[] = [];
+
+const auth = issuer({
+	storage: MemoryStorage(),
+	subjects,
+	allow: async () => true,
+	providers: {
+		code: CodeProvider({
+			maxAttempts: 3,
+			maxSends: 2,
+			resendInterval: 0,
+			request: async (_req, _state, _form, error) =>
+				new Response(JSON.stringify({ error: error?.type ?? null }), {
+					status: 200,
+					headers: { 'content-type': 'application/json' }
+				}),
+			sendCode: async (claims, code) => {
+				if (!claims.email?.includes('@')) {
+					return { type: 'invalid_claim', key: 'email', value: claims.email ?? '' };
+				}
+				sent.push(code);
+			}
+		})
+	},
+	success: async (ctx, value) => ctx.subject('user', { email: (value as any).claims.email })
+});
+
+const ORIGIN = 'https://auth.example.com';
+
+function jar() {
+	const cookies = new Map<string, string>();
+	return {
+		absorb(response: Response) {
+			for (const raw of response.headers.getSetCookie()) {
+				const [pair] = raw.split(';');
+				const index = pair!.indexOf('=');
+				cookies.set(pair!.slice(0, index), pair!.slice(index + 1));
+			}
+		},
+		header() {
+			return [...cookies].map(([name, value]) => `${name}=${value}`).join('; ');
+		}
+	};
+}
+
+async function post(cookies: ReturnType<typeof jar>, body: Record<string, string>) {
+	const response = await auth.request(`${ORIGIN}/code/authorize`, {
+		method: 'POST',
+		headers: { cookie: cookies.header(), 'content-type': 'application/x-www-form-urlencoded' },
+		body: new URLSearchParams(body)
+	});
+	cookies.absorb(response);
+	return response;
+}
+
+/**
+ * Begin an authorization the way a client does, so success has somewhere to go.
+ *
+ * Without this there is no authorization state and a correct code produces
+ * tokens rather than the redirect a browser flow ends in — which would make
+ * "did this sign in?" a different question in the test than in the product.
+ */
+async function begin() {
+	const cookies = jar();
+	const url = new URL(`${ORIGIN}/authorize`);
+	url.searchParams.set('client_id', 'test');
+	url.searchParams.set('redirect_uri', 'https://client.example.com/callback');
+	url.searchParams.set('response_type', 'code');
+	url.searchParams.set('provider', 'code');
+	cookies.absorb(await auth.request(url.toString()));
+	return cookies;
+}
+
+/** Start a sign-in and ask for a code, coming back with the cookies and the code. */
+async function ask(email = 'ada@example.com') {
+	const cookies = await begin();
+	await post(cookies, { action: 'request', email });
+	return { cookies, code: sent.at(-1)! };
+}
+
+/** What the stub UI reported, so a test can name the error rather than a status. */
+async function errorOf(response: Response) {
+	return ((await response.clone().json()) as { error: string | null }).error;
+}
+
+beforeEach(() => {
+	sent = [];
+});
+
+describe('signing in with a code', () => {
+	test('the right code signs you in', async () => {
+		const { cookies, code } = await ask();
+		const response = await post(cookies, { action: 'verify', code });
+		expect(response.status).toBe(302);
+	});
+
+	test('a wrong code is refused and says so', async () => {
+		const { cookies, code } = await ask();
+		const response = await post(cookies, { action: 'verify', code: code === '000000' ? '111111' : '000000' });
+		expect(response.status).toBe(200);
+		expect(await errorOf(response)).toBe('invalid_code');
+	});
+});
+
+/**
+ * The attack a six-digit pin invites, and what stops it.
+ *
+ * The code travels in an encrypted cookie the caller holds, and the caller is
+ * not necessarily the person the code was mailed to — anybody can type somebody
+ * else's address into the first screen. So the only thing between an attacker
+ * and an account is how many times they may guess, and that number has to be
+ * kept somewhere they cannot reach.
+ */
+describe('guessing the code', () => {
+	test('runs out of guesses long before it runs out of codes', async () => {
+		const { cookies, code } = await ask();
+		const wrong = code === '000000' ? '111111' : '000000';
+
+		expect(await errorOf(await post(cookies, { action: 'verify', code: wrong }))).toBe(
+			'invalid_code'
+		);
+		expect(await errorOf(await post(cookies, { action: 'verify', code: wrong }))).toBe(
+			'invalid_code'
+		);
+		expect(await errorOf(await post(cookies, { action: 'verify', code: wrong }))).toBe(
+			'invalid_code'
+		);
+
+		// Out of budget. The next guess is refused whether or not it is right.
+		expect(await errorOf(await post(cookies, { action: 'verify', code: wrong }))).toBe(
+			'rate_limit'
+		);
+	});
+
+	test('the real code stops working once the guesses are spent', async () => {
+		const { cookies, code } = await ask();
+		const wrong = code === '000000' ? '111111' : '000000';
+		for (let i = 0; i < 3; i++) await post(cookies, { action: 'verify', code: wrong });
+
+		const response = await post(cookies, { action: 'verify', code });
+		expect(response.status).toBe(200);
+		expect(await errorOf(response)).toBe('rate_limit');
+	});
+
+	// The counter would be worthless if it lived where the guesser does. This
+	// replays the cookie from before any guess was made, which is the cheapest
+	// way to wind back anything held in one.
+	test('replaying an earlier cookie does not hand back the spent guesses', async () => {
+		const { cookies, code } = await ask();
+		const untouched = cookies.header();
+		const wrong = code === '000000' ? '111111' : '000000';
+		for (let i = 0; i < 3; i++) await post(cookies, { action: 'verify', code: wrong });
+
+		const replayed = await auth.request(`${ORIGIN}/code/authorize`, {
+			method: 'POST',
+			headers: { cookie: untouched, 'content-type': 'application/x-www-form-urlencoded' },
+			body: new URLSearchParams({ action: 'verify', code: wrong })
+		});
+		expect(await errorOf(replayed)).toBe('rate_limit');
+	});
+
+	test('a code is spent when it is used, so its guesses do not carry over', async () => {
+		const { cookies, code } = await ask();
+		expect((await post(cookies, { action: 'verify', code })).status).toBe(302);
+
+		const again = await post(cookies, { action: 'verify', code });
+		expect(again.status).toBe(200);
+	});
+});
+
+describe('asking for codes', () => {
+	test('a fresh code comes with a fresh budget of guesses', async () => {
+		const first = await ask();
+		const wrong = '000000' === first.code ? '111111' : '000000';
+		for (let i = 0; i < 3; i++) await post(first.cookies, { action: 'verify', code: wrong });
+		expect(await errorOf(await post(first.cookies, { action: 'verify', code: wrong }))).toBe(
+			'rate_limit'
+		);
+
+		// Starting over is allowed. It costs a code sent to the mailbox being
+		// aimed at, which is where somebody would notice.
+		const second = await ask();
+		expect(second.code).not.toBe(first.code);
+		expect((await post(second.cookies, { action: 'verify', code: second.code })).status).toBe(302);
+	});
+
+	test('one sign-in cannot ask for codes forever', async () => {
+		const { cookies } = await ask();
+		expect(await errorOf(await post(cookies, { action: 'resend', email: 'ada@example.com' }))).toBe(
+			null
+		);
+		expect(await errorOf(await post(cookies, { action: 'resend', email: 'ada@example.com' }))).toBe(
+			'rate_limit'
+		);
+		expect(sent).toHaveLength(2);
+	});
+
+	test('a bad address still gets told it is a bad address', async () => {
+		const cookies = await begin();
+		const response = await post(cookies, { action: 'request', email: 'not-an-address' });
+		expect(await errorOf(response)).toBe('invalid_claim');
+		expect(sent).toHaveLength(0);
+	});
+});