fix(auth): make a device sign-in an answer somebody gave

Anybody could ask for a device code and be handed a link with the user
code already in it. Following that link started a sign-in, and finishing
the sign-in approved the grant. So sending somebody the link was enough:
they saw an ordinary sign-in prompt, completed it, and whoever kept the
device code polled and collected their access and refresh tokens. The
victim never saw a question, because there was not one.

There is now. Signing in says who the browser belongs to; it does not say
the person meant to hand an account to a program somewhere else. Those
are two questions and only the second authorizes anything, so the flow
ends at a page that names the program, shows the code back so it can be
compared with what the device is displaying, and offers Approve and Deny.
Approving is a POST carrying a value from the cookie, so another site
cannot submit it on somebody's behalf. Denial moved onto the same page:
it used to be a GET anyone could fire, which meant a link scanner could
cancel a real sign-in and a stranger with a user code could grief one.

Three more things that were wrong underneath.

The grant was read, modified and written back as a whole record. A poll
that read a pending grant and then wrote its bookkeeping erased an
approval that landed in between, and the client polled a dead grant until
it expired. Grants moved to a table, where approving is one conditional
update and redeeming is one delete that returns what it deleted, so
neither party can undo the other and two polls cannot both be served.

Tokens were minted when the person clicked and left sitting in storage
until collected. They are minted at redemption now, so the lifetime the
client is told about starts when it receives them, and a grant nobody
collects leaves no usable refresh token behind.

The client identifier was never checked, at either end. It is validated
when the grant is created and has to match when the code is redeemed —
without that, a leaked code is redeemable by anyone, and the identifier
the token carries is whatever the last caller claimed. The device code
is also stored as a hash now, since it is the credential the tokens are
handed to.

The store is an interface because the issuer cannot reach the database,
and because the guarantees are the point: every method is one operation,
and no caller reads a grant, decides, and writes it back.

packages/core/migrations/0010_device_authorization_grant.sql

@@ -0,0 +1,39 @@
+-- A device authorization grant, while it is still in flight.
+--
+-- Short-lived state that would sit happily in a cache, in a table anyway. The
+-- reason is not durability. Each transition here has to happen exactly once
+-- while two parties are touching the same row — a browser somebody is clicking
+-- through, and a program on another machine polling every few seconds — and a
+-- store that can only read and write whole records cannot promise that: the
+-- poll reads, the browser approves, the poll writes back what it read, and the
+-- approval is gone. Here, approving is one conditional update and redeeming is
+-- one delete that returns what it deleted, so neither can undo the other.
+--
+-- `device_code_hash` and not the code. The device code is the credential the
+-- tokens are handed to, so what is kept is enough to recognise it and not
+-- enough to present it. `user_code` is stored as written, because it is read
+-- off one screen and typed into another by the person looking at both, and it
+-- lives for minutes.
+--
+-- Rows are swept when a new grant is created rather than on a schedule. A grant
+-- lives ten minutes and that is the only statement that adds one, so the table
+-- stays bounded by how many sign-ins are in flight.
+
+CREATE TYPE "public"."device_grant_status" AS ENUM('pending', 'approved', 'denied');--> statement-breakpoint
+CREATE TABLE "device_grant" (
+	"id" char(30) PRIMARY KEY NOT NULL,
+	"time_created" timestamp with time zone DEFAULT now() NOT NULL,
+	"time_updated" timestamp with time zone DEFAULT now() NOT NULL,
+	"time_deleted" timestamp with time zone,
+	"device_code_hash" text NOT NULL,
+	"user_code" text NOT NULL,
+	"client_id" text NOT NULL,
+	"status" "device_grant_status" DEFAULT 'pending' NOT NULL,
+	"poll_interval" integer NOT NULL,
+	"last_polled_at" timestamp with time zone,
+	"expires_at" timestamp with time zone NOT NULL,
+	"subject" jsonb
+);
+--> statement-breakpoint
+CREATE UNIQUE INDEX "device_grant_device_code_unique" ON "device_grant" USING btree ("device_code_hash");--> statement-breakpoint
+CREATE UNIQUE INDEX "device_grant_user_code_unique" ON "device_grant" USING btree ("user_code");

packages/core/migrations/meta/_journal.json

@@ -71,6 +71,13 @@
       "when": 1788555252186,
       "tag": "0009_email_is_the_root_identity",
       "breakpoints": true
+    },
+    {
+      "idx": 10,
+      "version": "7",
+      "when": 1788590292860,
+      "tag": "0010_device_authorization_grant",
+      "breakpoints": true
     }
   ]
 }

packages/core/src/auth/device-grant.sql.ts

@@ -0,0 +1,62 @@
+import { integer, jsonb, pgEnum, pgTable, text, uniqueIndex } from 'drizzle-orm/pg-core';
+
+import { id, timestamps, utc } from '../db/types.js';
+
+export const DeviceGrantStatusEnum = pgEnum('device_grant_status', [
+	'pending',
+	'approved',
+	'denied'
+]);
+
+/**
+ * A device authorization grant, while it is still in flight.
+ *
+ * This is short-lived state that would sit happily in a cache, and it is in a
+ * table anyway. The reason is that every transition here has to happen exactly
+ * once while two parties are touching the row — a browser somebody is clicking
+ * through, and a program polling every few seconds — and a store that can only
+ * read and write whole records cannot promise that. Here, approving is one
+ * conditional update and redeeming is one delete that returns what it deleted,
+ * so the two cannot interleave into each other.
+ *
+ * `device_code_hash` and not the code: the code is the credential the tokens
+ * are handed to, so what is kept is enough to recognise it and not enough to
+ * present it. `user_code` is stored as written, because it is read off a screen
+ * by the person who is looking at it and lives for minutes.
+ */
+export const DeviceGrantTable = pgTable(
+	'device_grant',
+	{
+		...id,
+		...timestamps,
+
+		deviceCodeHash: text('device_code_hash').notNull(),
+		userCode: text('user_code').notNull(),
+		clientId: text('client_id').notNull(),
+		status: DeviceGrantStatusEnum('status').notNull().default('pending'),
+
+		/** Seconds the client is currently being told to wait between polls. */
+		pollInterval: integer('poll_interval').notNull(),
+		/** Null until a poll has been given a real answer. */
+		lastPolledAt: utc('last_polled_at'),
+		expiresAt: utc('expires_at').notNull(),
+
+		/**
+		 * Who the grant turned out to be for, written when it is approved.
+		 *
+		 * Not the tokens. Those are minted when the waiting program redeems the
+		 * code, so their lifetime starts when they are handed over and a grant
+		 * nobody collects leaves no usable credential behind.
+		 */
+		subject: jsonb('subject').$type<{
+			subject: string;
+			type: string;
+			properties: unknown;
+			ttl: { access: number; refresh: number };
+		}>()
+	},
+	(t) => [
+		uniqueIndex('device_grant_device_code_unique').on(t.deviceCodeHash),
+		uniqueIndex('device_grant_user_code_unique').on(t.userCode)
+	]
+);

packages/core/src/auth/device-grant.test.ts

@@ -0,0 +1,201 @@
+import { afterAll, beforeEach, describe, expect, test } from 'bun:test';
+
+import type { DeviceGrant, DeviceGrantSubject } from '@nestri/auth/device';
+
+import { testDb } from '../db/test.js';
+import { PostgresDeviceStore } from './device-grant.js';
+
+const sql = testDb();
+const store = PostgresDeviceStore();
+
+const SUBJECT: DeviceGrantSubject = {
+	subject: 'user:usr_fixture',
+	type: 'user',
+	properties: { userID: 'usr_fixture' },
+	ttl: { access: 60, refresh: 600 }
+};
+
+let counter = 0;
+function hash(): string {
+	counter += 1;
+	return `device-grant-fixture-${counter}`.padEnd(64, '0');
+}
+
+function pending(overrides: Partial<DeviceGrant> = {}): DeviceGrant {
+	const deviceCodeHash = overrides.deviceCodeHash ?? hash();
+	return {
+		deviceCodeHash,
+		userCode: `UC${deviceCodeHash.slice(-6)}`,
+		clientID: 'desktop',
+		status: 'pending',
+		interval: 5,
+		lastPolled: 0,
+		expires: Date.now() + 600_000,
+		...overrides
+	};
+}
+
+async function cleanup() {
+	await sql`delete from device_grant where device_code_hash like 'device-grant-fixture-%'`;
+}
+
+beforeEach(cleanup);
+afterAll(async () => {
+	await cleanup();
+	await sql.end();
+});
+
+describe('what the store remembers', () => {
+	test('a grant is findable by either code, and comes back as it went in', async () => {
+		const grant = pending();
+		await store.create(grant);
+
+		const byDevice = await store.byDeviceCode(grant.deviceCodeHash);
+		expect(byDevice).toMatchObject({
+			deviceCodeHash: grant.deviceCodeHash,
+			userCode: grant.userCode,
+			clientID: 'desktop',
+			status: 'pending',
+			interval: 5,
+			lastPolled: 0
+		});
+		expect((await store.byUserCode(grant.userCode))?.deviceCodeHash).toBe(grant.deviceCodeHash);
+	});
+
+	test('creating a grant clears out the ones that aged out', async () => {
+		const stale = pending({ expires: Date.now() - 1000 });
+		await store.create(stale);
+		await store.create(pending());
+
+		const rows = await sql`
+			select count(*)::int as n from device_grant where device_code_hash = ${stale.deviceCodeHash}
+		`;
+		expect(rows[0]!.n).toBe(0);
+	});
+});
+
+/**
+ * The properties the flow is built on, asserted against a real database.
+ *
+ * Each of these is a claim that a transition happens once even though two
+ * parties are racing for it, and each is enforced by a `where` clause rather
+ * than by application code. That is exactly the sort of claim that reads as
+ * obviously true and is obviously false the moment the condition is dropped, so
+ * it is worth a test that would notice.
+ */
+describe('transitions that must happen once', () => {
+	test('a grant is approved once', async () => {
+		const grant = pending();
+		await store.create(grant);
+
+		expect(await store.approve(grant.deviceCodeHash, SUBJECT)).toBe(true);
+		expect(await store.approve(grant.deviceCodeHash, SUBJECT)).toBe(false);
+	});
+
+	test('an approval cannot overwrite a refusal', async () => {
+		const grant = pending();
+		await store.create(grant);
+
+		expect(await store.deny(grant.deviceCodeHash)).toBe(true);
+		expect(await store.approve(grant.deviceCodeHash, SUBJECT)).toBe(false);
+		expect((await store.byDeviceCode(grant.deviceCodeHash))?.status).toBe('denied');
+	});
+
+	test('a refusal cannot overwrite an approval', async () => {
+		const grant = pending();
+		await store.create(grant);
+
+		expect(await store.approve(grant.deviceCodeHash, SUBJECT)).toBe(true);
+		expect(await store.deny(grant.deviceCodeHash)).toBe(false);
+		expect((await store.byDeviceCode(grant.deviceCodeHash))?.status).toBe('approved');
+	});
+
+	test('several approvals arriving together settle on one', async () => {
+		const grant = pending();
+		await store.create(grant);
+
+		const results = await Promise.all(
+			Array.from({ length: 5 }, () => store.approve(grant.deviceCodeHash, SUBJECT))
+		);
+		expect(results.filter(Boolean)).toHaveLength(1);
+	});
+
+	test('a grant that has aged out can no longer be answered', async () => {
+		const grant = pending({ expires: Date.now() - 1000 });
+		// Inserted directly, because creating one sweeps it.
+		await sql`
+			insert into device_grant (id, device_code_hash, user_code, client_id, status, poll_interval, expires_at)
+			values ('dvg_expired_fixture0000000000', ${grant.deviceCodeHash}, ${grant.userCode},
+				'desktop', 'pending', 5, now() - interval '1 second')
+		`;
+
+		expect(await store.approve(grant.deviceCodeHash, SUBJECT)).toBe(false);
+		expect(await store.deny(grant.deviceCodeHash)).toBe(false);
+	});
+});
+
+describe('redeeming', () => {
+	test('an approved grant is redeemed once, and carries who it was for', async () => {
+		const grant = pending();
+		await store.create(grant);
+		await store.approve(grant.deviceCodeHash, SUBJECT);
+
+		const claimed = await store.consume(grant.deviceCodeHash, 'desktop');
+		expect(claimed?.subject).toEqual(SUBJECT);
+		expect(await store.consume(grant.deviceCodeHash, 'desktop')).toBeNull();
+	});
+
+	test('several polls arriving together are served once', async () => {
+		const grant = pending();
+		await store.create(grant);
+		await store.approve(grant.deviceCodeHash, SUBJECT);
+
+		const results = await Promise.all(
+			Array.from({ length: 5 }, () => store.consume(grant.deviceCodeHash, 'desktop'))
+		);
+		expect(results.filter(Boolean)).toHaveLength(1);
+	});
+
+	test('another client cannot redeem the code', async () => {
+		const grant = pending();
+		await store.create(grant);
+		await store.approve(grant.deviceCodeHash, SUBJECT);
+
+		expect(await store.consume(grant.deviceCodeHash, 'somebody-else')).toBeNull();
+		// And the real client is not robbed of it in the attempt.
+		expect(await store.consume(grant.deviceCodeHash, 'desktop')).not.toBeNull();
+	});
+
+	test('a grant nobody approved is not redeemable', async () => {
+		const grant = pending();
+		await store.create(grant);
+		expect(await store.consume(grant.deviceCodeHash, 'desktop')).toBeNull();
+	});
+});
+
+/**
+ * The bug this store exists to make impossible.
+ *
+ * A poll reads a pending grant, the browser approves while the poll is in
+ * flight, and then the poll writes down that it happened. If writing that down
+ * means writing the whole record back, the approval is gone and the client
+ * polls a dead grant until it expires.
+ */
+describe('recording a poll', () => {
+	test('touches the bookkeeping and nothing else', async () => {
+		const grant = pending();
+		await store.create(grant);
+
+		const stale = await store.byDeviceCode(grant.deviceCodeHash);
+		expect(stale!.status).toBe('pending');
+
+		await store.approve(grant.deviceCodeHash, SUBJECT);
+		await store.recordPoll(grant.deviceCodeHash, Date.now(), stale!.interval + 5);
+
+		const after = await store.byDeviceCode(grant.deviceCodeHash);
+		expect(after!.status).toBe('approved');
+		expect(after!.subject).toEqual(SUBJECT);
+		expect(after!.interval).toBe(10);
+		expect(after!.lastPolled).toBeGreaterThan(0);
+	});
+});

packages/core/src/auth/device-grant.ts

@@ -0,0 +1,153 @@
+import type { DeviceGrant, DeviceGrantSubject, DeviceStore } from '@nestri/auth/device';
+import { and, eq, lt, sql } from 'drizzle-orm';
+
+import { Database } from '../db/index.js';
+import { Identifier } from '../id.js';
+import { DeviceGrantTable } from './device-grant.sql.js';
+
+type Row = typeof DeviceGrantTable.$inferSelect;
+
+function toGrant(row: Row): DeviceGrant {
+	return {
+		deviceCodeHash: row.deviceCodeHash,
+		userCode: row.userCode,
+		clientID: row.clientId,
+		status: row.status,
+		interval: row.pollInterval,
+		lastPolled: row.lastPolledAt?.getTime() ?? 0,
+		expires: row.expiresAt.getTime(),
+		subject: row.subject ?? undefined
+	};
+}
+
+/**
+ * Device authorization grants, kept where a conditional write is possible.
+ *
+ * Each method below is one statement on purpose. The interface asks for
+ * transitions that happen exactly once while a browser and a polling client are
+ * both touching the same grant, and the only way to promise that is to let the
+ * database decide: `update ... where status = 'pending'` either changes a row
+ * or does not, and `delete ... returning` hands the row to exactly one caller.
+ * Read it, decide in application code, and write it back, and the two callers
+ * undo each other — which is the bug this shape exists to make impossible.
+ */
+export function PostgresDeviceStore(): DeviceStore {
+	return {
+		async create(grant) {
+			await Database.use(async (tx) => {
+				// Swept here rather than on a schedule. A grant lives ten
+				// minutes and this is the only statement that adds one, so the
+				// table is bounded by how many sign-ins are in flight without
+				// anything else having to run.
+				await tx.delete(DeviceGrantTable).where(lt(DeviceGrantTable.expiresAt, new Date()));
+
+				await tx.insert(DeviceGrantTable).values({
+					id: Identifier.ascending('deviceGrant'),
+					deviceCodeHash: grant.deviceCodeHash,
+					userCode: grant.userCode,
+					clientId: grant.clientID,
+					status: grant.status,
+					pollInterval: grant.interval,
+					lastPolledAt: grant.lastPolled ? new Date(grant.lastPolled) : null,
+					expiresAt: new Date(grant.expires),
+					subject: grant.subject ?? null
+				});
+			});
+		},
+
+		async byDeviceCode(deviceCodeHash) {
+			return Database.use(async (tx) =>
+				tx
+					.select()
+					.from(DeviceGrantTable)
+					.where(eq(DeviceGrantTable.deviceCodeHash, deviceCodeHash))
+					.then((rows) => (rows[0] ? toGrant(rows[0]) : null))
+			);
+		},
+
+		async byUserCode(userCode) {
+			return Database.use(async (tx) =>
+				tx
+					.select()
+					.from(DeviceGrantTable)
+					.where(eq(DeviceGrantTable.userCode, userCode))
+					.then((rows) => (rows[0] ? toGrant(rows[0]) : null))
+			);
+		},
+
+		async approve(deviceCodeHash, subject: DeviceGrantSubject) {
+			return Database.use(async (tx) =>
+				tx
+					.update(DeviceGrantTable)
+					.set({ status: 'approved', subject })
+					.where(
+						and(
+							eq(DeviceGrantTable.deviceCodeHash, deviceCodeHash),
+							eq(DeviceGrantTable.status, 'pending'),
+							sql`${DeviceGrantTable.expiresAt} > now()`
+						)
+					)
+					.returning({ id: DeviceGrantTable.id })
+					.then((rows) => rows.length > 0)
+			);
+		},
+
+		async deny(deviceCodeHash) {
+			return Database.use(async (tx) =>
+				tx
+					.update(DeviceGrantTable)
+					.set({ status: 'denied' })
+					.where(
+						and(
+							eq(DeviceGrantTable.deviceCodeHash, deviceCodeHash),
+							eq(DeviceGrantTable.status, 'pending'),
+							sql`${DeviceGrantTable.expiresAt} > now()`
+						)
+					)
+					.returning({ id: DeviceGrantTable.id })
+					.then((rows) => rows.length > 0)
+			);
+		},
+
+		async consume(deviceCodeHash, clientID) {
+			// Deleting and reading are the same statement, so two polls
+			// arriving together cannot both be served: one deletes the row and
+			// gets it, the other deletes nothing and gets nothing.
+			return Database.use(async (tx) =>
+				tx
+					.delete(DeviceGrantTable)
+					.where(
+						and(
+							eq(DeviceGrantTable.deviceCodeHash, deviceCodeHash),
+							eq(DeviceGrantTable.clientId, clientID),
+							eq(DeviceGrantTable.status, 'approved'),
+							sql`${DeviceGrantTable.expiresAt} > now()`
+						)
+					)
+					.returning()
+					.then((rows) => (rows[0] ? toGrant(rows[0]) : null))
+			);
+		},
+
+		async recordPoll(deviceCodeHash, at, interval) {
+			// Two columns, and deliberately not the rest of the row. Writing
+			// the whole grant back here is what would let a poll that read a
+			// pending record undo an approval that landed while it was in
+			// flight.
+			await Database.use(async (tx) => {
+				await tx
+					.update(DeviceGrantTable)
+					.set({ lastPolledAt: new Date(at), pollInterval: interval })
+					.where(eq(DeviceGrantTable.deviceCodeHash, deviceCodeHash));
+			});
+		},
+
+		async remove(deviceCodeHash) {
+			await Database.use(async (tx) => {
+				await tx
+					.delete(DeviceGrantTable)
+					.where(eq(DeviceGrantTable.deviceCodeHash, deviceCodeHash));
+			});
+		}
+	};
+}

packages/core/src/id.ts

@@ -19,7 +19,8 @@ export namespace Identifier {
 		userLibrary: 'ulb',
 		gameDepot: 'gdp',
 		gameDownload: 'gdl',
-		waitlistEntry: 'wle'
+		waitlistEntry: 'wle',
+		deviceGrant: 'dvg'
 	} as const;
 
 	export function schema(prefix: keyof typeof prefixes) {