From 0eb4454bcfddc3950bd72d86173a67cec2960cc8 Mon Sep 17 00:00:00 2001 From: khipu-agent Date: Sun, 26 Jul 2026 22:16:27 -0600 Subject: [PATCH] fix(pwa): claim device codes atomically in /cli/device/token The read-check-claim sequence let two concurrent polls both read "approved" before either UPDATE landed, so one single-use device code minted two API keys. Make the claim conditional (status = 'approved') and reject the loser, mirroring the /cli/token fix (#46). --- apps/pwa/src/routes/cli.mjs | 8 +- apps/pwa/test/cli-device-token.test.mjs | 137 ++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 1 deletion(-) create mode 100644 apps/pwa/test/cli-device-token.test.mjs diff --git a/apps/pwa/src/routes/cli.mjs b/apps/pwa/src/routes/cli.mjs index ad9fb10..546fca2 100644 --- a/apps/pwa/src/routes/cli.mjs +++ b/apps/pwa/src/routes/cli.mjs @@ -162,7 +162,13 @@ cliRouter.post("/cli/device/token", async (req, res) => { if (row.status === "denied") return res.status(400).json({ error: "access_denied" }); if (row.status !== "approved") return res.status(400).json({ error: "authorization_pending" }); - await run(`UPDATE device_codes SET status = 'claimed' WHERE device_code = ?`, [row.device_code]); + // Claim the code atomically, the same way /cli/token claims auth codes: the + // status check above is not enough on its own — with a remote (network) + // database, two concurrent polls can both read "approved" before either + // UPDATE lands, and one single-use code would mint two API keys. Only the + // first claim wins. + const claimed = await run(`UPDATE device_codes SET status = 'claimed' WHERE device_code = ? AND status = 'approved'`, [row.device_code]); + if (!claimed.rowsAffected) return res.status(400).json({ error: "expired_token" }); const user = await get(`SELECT * FROM users WHERE id = ?`, [row.user_id]); const { plaintext } = await createApiKey(user.id, row.name || "moshcode cli"); res.json({ access_token: plaintext, token_type: "bearer", user: { id: user.id, email: user.email || null, name: user.display_name } }); diff --git a/apps/pwa/test/cli-device-token.test.mjs b/apps/pwa/test/cli-device-token.test.mjs new file mode 100644 index 0000000..3dabcab --- /dev/null +++ b/apps/pwa/test/cli-device-token.test.mjs @@ -0,0 +1,137 @@ +// Integration tests for the device-code exchange (POST /cli/device/token). +// +// These boot the real router against a throwaway libsql file database. They +// skip cleanly when the PWA dependencies are not installed (a fresh repo +// clone only has the root CLI deps), so the root `npm test` stays green +// either way. Run `npm install` in apps/pwa to enable them. +import assert from "node:assert/strict"; +import http from "node:http"; +import fs from "node:fs"; +import { mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { createRequire } from "node:module"; +import test from "node:test"; + +const require = createRequire(import.meta.url); +let deps = null; +try { + deps = { express: require("express"), cookieParser: require("cookie-parser") }; +} catch { + deps = null; // pwa dependencies not installed — tests below skip +} + +// Point the app at a throwaway database BEFORE importing its modules (config +// reads the environment once, at import time). +const workdir = mkdtempSync(path.join(tmpdir(), "moshcode-pwa-test-")); +process.env.DATABASE_URL = `file:${path.join(workdir, "test.db")}`; +process.env.SESSION_SECRET = "test-secret"; + +async function boot() { + const { migrate } = await import("../src/migrate.mjs"); + await migrate(); + const { run, all, db } = await import("../src/db.mjs"); + // The local libsql driver resolves statements in microtasks, which fully + // serializes concurrent request handlers and hides read-check-write races. + // Production runs against a network database (Turso), where every statement + // is a round trip. Defer each statement to a macrotask so two in-flight + // handlers genuinely interleave, like they would against the remote DB. + const execute = db.execute.bind(db); + db.execute = (stmt) => new Promise((resolve, reject) => { + setTimeout(() => execute(stmt).then(resolve, reject), 2); + }); + const { sessionMiddleware, csrfGuard } = await import("../src/lib/session.mjs"); + const { cliRouter } = await import("../src/routes/cli.mjs"); + + const app = deps.express(); + app.use(deps.express.json({ verify: (req, _res, buf) => { req.rawBody = buf.toString("utf8"); } })); + app.use(deps.express.urlencoded({ extended: false })); + app.use(deps.cookieParser()); + app.use(sessionMiddleware); + app.use(csrfGuard); + app.use(cliRouter); + const server = await new Promise((resolve) => { + const s = app.listen(0, "127.0.0.1", () => resolve(s)); + }); + const { port } = server.address(); + + const seedDeviceCode = async (deviceCode, { status = "approved", ageMs = 0 } = {}) => { + await run(`INSERT OR REPLACE INTO users (id, email, display_name, created_at) VALUES ('u1','a@b.c','demo',1)`); + const now = Date.now(); + await run( + `INSERT INTO device_codes (device_code,user_code,user_id,status,name,interval_s,created_at,expires_at) VALUES (?,?,?,?,?,?,?,?)`, + [deviceCode, "ABCD-2345", status === "pending" ? null : "u1", status, "test", 5, now - ageMs, now - ageMs + 10 * 60 * 1000] + ); + }; + // Raw http with a fresh connection per request: fetch()/undici would reuse a + // keep-alive socket for same-origin calls and serialize the "concurrent" + // polls, hiding the race this exercises. + const poll = (deviceCode) => new Promise((resolve, reject) => { + const req = http.request({ + host: "127.0.0.1", port, path: "/cli/device/token", method: "POST", agent: false, + headers: { "content-type": "application/json" }, + }, (res) => { + let data = ""; + res.on("data", (chunk) => { data += chunk; }); + res.on("end", () => resolve({ status: res.statusCode, body: JSON.parse(data) })); + }); + req.on("error", reject); + req.end(JSON.stringify({ device_code: deviceCode })); + }); + + return { run, all, db, server, seedDeviceCode, poll }; +} + +// One shared app/db for the whole file (db.mjs is a module-level singleton — +// closing it between tests would break the next boot). +let booted = null; +const app = () => (booted ||= boot()); + +test.after(() => { + if (!booted) return; + booted.then(({ server, db }) => { server.close(); db.close?.(); }) + .finally(() => { try { fs.rmSync(workdir, { recursive: true, force: true }); } catch { /* noop */ } }); +}); + +test("cli/device/token: an approved code exchanges exactly once, then is rejected", { skip: !deps && "apps/pwa deps not installed" }, async () => { + const { all, seedDeviceCode, poll } = await app(); + + await seedDeviceCode("dev-once"); + const before = (await all(`SELECT id FROM api_keys`)).length; + + const first = await poll("dev-once"); + assert.equal(first.status, 200); + assert.ok(first.body.access_token, "first poll must mint an API key"); + assert.equal((await all(`SELECT id FROM api_keys`)).length, before + 1); + + // Replay with the same device code must fail — the code is single-use. + const replay = await poll("dev-once"); + assert.equal(replay.status, 400); + assert.equal((await all(`SELECT id FROM api_keys`)).length, before + 1, "replay must not mint a second key"); +}); + +test("cli/device/token: concurrent polls claim the code once — one key, not two", { skip: !deps && "apps/pwa deps not installed" }, async () => { + const { all, seedDeviceCode, poll } = await app(); + + await seedDeviceCode("dev-race"); + const before = (await all(`SELECT id FROM api_keys`)).length; + + // Two CLI polls in flight at once (retry after a timeout, double-clicked + // CI job, …). Both read status "approved" before either claim lands; only + // one may win. + const [a, b] = await Promise.all([poll("dev-race"), poll("dev-race")]); + const wins = [a, b].filter((r) => r.status === 200); + assert.equal(wins.length, 1, `exactly one poll may mint a key, got statuses ${a.status}/${b.status}`); + assert.equal((await all(`SELECT id FROM api_keys`)).length, before + 1, "one device code must mint exactly one API key"); +}); + +test("cli/device/token: a pending code is not exchangeable", { skip: !deps && "apps/pwa deps not installed" }, async () => { + const { all, seedDeviceCode, poll } = await app(); + + await seedDeviceCode("dev-pending", { status: "pending" }); + const before = (await all(`SELECT id FROM api_keys`)).length; + const res = await poll("dev-pending"); + assert.equal(res.status, 400); + assert.equal(res.body.error, "authorization_pending"); + assert.equal((await all(`SELECT id FROM api_keys`)).length, before); +});