Skip to content

Commit 3a842bb

Browse files
Esteban Espinkhipu-agent
authored andcommitted
fix(pwa): claim CLI auth codes atomically in /cli/token
The exchange did SELECT used -> UPDATE used=1 as separate statements. With a remote (network) libsql database, two concurrent exchanges can both pass the `used` check before either UPDATE lands, so one single-use code minted two API keys. Claim with a single conditional UPDATE ... WHERE used = 0 and reject when rowsAffected is 0. Adds integration tests for the single-use, PKCE, and expiry contracts (skip cleanly when apps/pwa deps aren't installed).
1 parent 05cfd7f commit 3a842bb

2 files changed

Lines changed: 128 additions & 1 deletion

File tree

apps/pwa/src/routes/cli.mjs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,12 @@ cliRouter.post("/cli/token", async (req, res) => {
8383
const challenge = crypto.createHash("sha256").update(String(code_verifier)).digest("base64url");
8484
if (challenge !== row.code_challenge) return res.status(400).json({ error: "PKCE verification failed" });
8585

86-
await run(`UPDATE cli_auth_codes SET used = 1 WHERE code = ?`, [code]);
86+
// Claim the code atomically. The read-check-update sequence above is not
87+
// enough on its own: with a remote (network) database, two concurrent
88+
// exchanges can both pass the `used` check before either UPDATE lands, and
89+
// one single-use code would mint two API keys. Only the first claim wins.
90+
const claimed = await run(`UPDATE cli_auth_codes SET used = 1 WHERE code = ? AND used = 0`, [code]);
91+
if (!claimed.rowsAffected) return res.status(400).json({ error: "invalid or expired code" });
8792
const user = await get(`SELECT * FROM users WHERE id = ?`, [row.user_id]);
8893
const { plaintext } = await createApiKey(user.id, row.name || "moshcode cli");
8994
res.json({ access_token: plaintext, token_type: "bearer", user: { id: user.id, email: user.email || null, name: user.display_name } });

apps/pwa/test/cli-token.test.mjs

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
// Integration tests for the CLI auth-code exchange (POST /cli/token).
2+
//
3+
// These boot the real router against a throwaway libsql file database. They
4+
// skip cleanly when the PWA dependencies are not installed (a fresh repo
5+
// clone only has the root CLI deps), so the root `npm test` stays green
6+
// either way. Run `npm install` in apps/pwa to enable them.
7+
import assert from "node:assert/strict";
8+
import crypto from "node:crypto";
9+
import fs from "node:fs";
10+
import { mkdtempSync } from "node:fs";
11+
import { tmpdir } from "node:os";
12+
import path from "node:path";
13+
import { createRequire } from "node:module";
14+
import test from "node:test";
15+
16+
const require = createRequire(import.meta.url);
17+
let deps = null;
18+
try {
19+
deps = { express: require("express"), cookieParser: require("cookie-parser") };
20+
} catch {
21+
deps = null; // pwa dependencies not installed — tests below skip
22+
}
23+
24+
// Point the app at a throwaway database BEFORE importing its modules (config
25+
// reads the environment once, at import time).
26+
const workdir = mkdtempSync(path.join(tmpdir(), "moshcode-pwa-test-"));
27+
process.env.DATABASE_URL = `file:${path.join(workdir, "test.db")}`;
28+
process.env.SESSION_SECRET = "test-secret";
29+
30+
async function boot() {
31+
const { migrate } = await import("../src/migrate.mjs");
32+
await migrate();
33+
const { run, all, get, db } = await import("../src/db.mjs");
34+
const { sessionMiddleware, csrfGuard } = await import("../src/lib/session.mjs");
35+
const { cliRouter } = await import("../src/routes/cli.mjs");
36+
37+
const app = deps.express();
38+
app.use(deps.express.json({ verify: (req, _res, buf) => { req.rawBody = buf.toString("utf8"); } }));
39+
app.use(deps.express.urlencoded({ extended: false }));
40+
app.use(deps.cookieParser());
41+
app.use(sessionMiddleware);
42+
app.use(csrfGuard);
43+
app.use(cliRouter);
44+
const server = await new Promise((resolve) => {
45+
const s = app.listen(0, "127.0.0.1", () => resolve(s));
46+
});
47+
const base = `http://127.0.0.1:${server.address().port}`;
48+
49+
const seedCode = async (code, verifier, { used = 0, ageMs = 0 } = {}) => {
50+
await run(`INSERT OR REPLACE INTO users (id, email, display_name, created_at) VALUES ('u1','a@b.c','demo',1)`);
51+
const challenge = crypto.createHash("sha256").update(verifier).digest("base64url");
52+
const now = Date.now();
53+
await run(
54+
`INSERT INTO cli_auth_codes (code,user_id,code_challenge,redirect_uri,name,created_at,expires_at) VALUES (?,?,?,?,?,?,?)`,
55+
[code, "u1", challenge, "http://127.0.0.1:9/callback", "test", now - ageMs, now - ageMs + 5 * 60 * 1000]
56+
);
57+
};
58+
const exchange = (code, verifier) => fetch(`${base}/cli/token`, {
59+
method: "POST",
60+
headers: { "content-type": "application/json" },
61+
body: JSON.stringify({ code, code_verifier: verifier }),
62+
}).then(async (res) => ({ status: res.status, body: await res.json() }));
63+
64+
return { run, all, get, db, server, seedCode, exchange };
65+
}
66+
67+
// One shared app/db for the whole file (db.mjs is a module-level singleton —
68+
// closing it between tests would break the next boot).
69+
let booted = null;
70+
const app = () => (booted ||= boot());
71+
72+
test.after(() => {
73+
if (!booted) return;
74+
booted.then(({ server, db }) => { server.close(); db.close?.(); })
75+
.finally(() => { try { fs.rmSync(workdir, { recursive: true, force: true }); } catch { /* noop */ } });
76+
});
77+
78+
test("cli/token: a code exchanges exactly once, then is rejected", { skip: !deps && "apps/pwa deps not installed" }, async () => {
79+
const { all, seedCode, exchange } = await app();
80+
81+
const verifier = "verifier-" + crypto.randomBytes(16).toString("hex");
82+
await seedCode("code-once", verifier);
83+
84+
const before = (await all(`SELECT id FROM api_keys`)).length;
85+
const first = await exchange("code-once", verifier);
86+
assert.equal(first.status, 200);
87+
assert.ok(first.body.access_token, "first exchange must mint an API key");
88+
assert.equal((await all(`SELECT id FROM api_keys`)).length, before + 1);
89+
90+
// Replay with the same code + verifier must fail — the code is single-use.
91+
const replay = await exchange("code-once", verifier);
92+
assert.equal(replay.status, 400);
93+
assert.equal((await all(`SELECT id FROM api_keys`)).length, before + 1, "replay must not mint a second key");
94+
});
95+
96+
test("cli/token: wrong PKCE verifier is rejected and does not consume the code", { skip: !deps && "apps/pwa deps not installed" }, async () => {
97+
const { all, seedCode, exchange } = await app();
98+
99+
const verifier = "verifier-" + crypto.randomBytes(16).toString("hex");
100+
await seedCode("code-pkce", verifier);
101+
102+
const before = (await all(`SELECT id FROM api_keys`)).length;
103+
const bad = await exchange("code-pkce", "wrong-verifier");
104+
assert.equal(bad.status, 400);
105+
assert.equal(bad.body.error, "PKCE verification failed");
106+
assert.equal((await all(`SELECT id FROM api_keys`)).length, before, "failed PKCE must not mint a key");
107+
108+
// The code was NOT consumed by the failed attempt — the real verifier works.
109+
const good = await exchange("code-pkce", verifier);
110+
assert.equal(good.status, 200);
111+
assert.equal((await all(`SELECT id FROM api_keys`)).length, before + 1);
112+
});
113+
114+
test("cli/token: expired codes are rejected", { skip: !deps && "apps/pwa deps not installed" }, async () => {
115+
const { seedCode, exchange } = await app();
116+
117+
const verifier = "verifier-" + crypto.randomBytes(16).toString("hex");
118+
await seedCode("code-old", verifier, { ageMs: 10 * 60 * 1000 }); // 10min old, 5min TTL
119+
120+
const res = await exchange("code-old", verifier);
121+
assert.equal(res.status, 400);
122+
});

0 commit comments

Comments
 (0)