trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRuns13g rEventsFlows

A test pins the origin connect flows: a state claims once, a short code reveals nothing, and only the starter sees the flow #6926

Merged scoutcrew opened this

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

scout/forge:scout/6903-a-test-pins-the-origin-connect-flows-a-s → main 1 commit fork 2 threads · 2 unresolved

  • Merged. scout/6903-a-test-pins-the-origin-connect-flows-a-s was merged into main as 269cec7.

Review

Sign in to review this change.
1 file changed +158 −0
test/origins-oauth-flows.test.ts added +158
@@ -0,0 +1,158 @@
1 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
2 import { mkdtemp, rm } from "node:fs/promises";
3 import { tmpdir } from "node:os";
4 import { join } from "node:path";
5 import { createApp, type App } from "../src/app.ts";
6 import {
7 FLOW_TTL_MS,
8 claimFlowByState,
9 createFlow,
10 finishFlow,
11 flowSecrets,
12 getFlowFor,
13 pruneFlows,
14 targetForShortCode,
15 type AppRegistration,
16 } from "../src/origins/oauth.ts";
17
18 /**
19 * The connect flows' bookkeeping: a state claims once, a short code reveals
20 * nothing about why it failed, only the starter sees a flow, and pruning
21 * takes the secrets with the rows. `now` is always passed in.
22 */
23
24 const REGISTRATION: AppRegistration = {
25 kind: "forgejo",
26 apiUrl: "https://git.example/api/v1",
27 webUrl: "https://git.example",
28 clientId: "client-id",
29 clientSecret: "client-secret-value",
30 };
31 const TARGET = "https://git.example/login/oauth/authorize?state=raw-state-1";
32
33 let dataDir: string;
34 let app: App;
35
36 beforeAll(async () => {
37 dataDir = await mkdtemp(join(tmpdir(), "forge-oauth-flows-"));
38 app = createApp({ dataDir, port: 0 });
39 // The flows are keyed by user id only; they need no real account.
40 app.db.run("PRAGMA foreign_keys = OFF");
41 });
42 afterAll(async () => {
43 app.close();
44 await rm(dataDir, { recursive: true, force: true });
45 });
46
47 const start = (userId: string, state: string) =>
48 createFlow(app.db, dataDir, {
49 userId,
50 flow: "code",
51 app: REGISTRATION,
52 state,
53 verifier: "verifier-value",
54 redirectUri: "https://forge.example/settings/origins/callback",
55 target: TARGET,
56 });
57
58 const rowText = (flowId: string): string =>
59 JSON.stringify(
60 app.db.query("SELECT * FROM oauth_flow WHERE id = ?1").get(flowId),
61 );
62
63 const expireAt = (flowId: string, iso: string): void => {
64 app.db.query("UPDATE oauth_flow SET expires_at = ?1 WHERE id = ?2").run(iso, flowId);
65 };
66
67 describe("claiming a flow by its state", () => {
68 test("claims once; a replay and an unknown state answer nothing", () => {
69 const flow = start("u-claim", "raw-state-1");
70 const claimed = claimFlowByState(app.db, "raw-state-1", "u-claim");
71 expect(claimed?.id).toBe(flow.id);
72 expect(claimFlowByState(app.db, "raw-state-1", "u-claim")).toBeUndefined();
73 expect(claimFlowByState(app.db, "never-issued", "u-claim")).toBeUndefined();
74 expect(claimFlowByState(app.db, "", "u-claim")).toBeUndefined();
75 });
76
77 test("someone else's callback does not claim it, nor spend it", () => {
78 start("u-owner", "raw-state-2");
79 expect(claimFlowByState(app.db, "raw-state-2", "u-other")).toBeUndefined();
80 expect(claimFlowByState(app.db, "raw-state-2", "u-owner")).toBeDefined();
81 });
82
83 test("the raw state and the secrets appear nowhere in the row", () => {
84 const flow = start("u-raw", "raw-state-3-unique");
85 const text = rowText(flow.id);
86 expect(text).not.toContain("raw-state-3-unique");
87 expect(text).not.toContain("verifier-value");
88 expect(text).not.toContain("client-secret-value");
89 expect(text).not.toContain("login/oauth/authorize");
90 });
91 });
92
93 describe("a short code", () => {
94 const now = new Date("2030-01-01T00:00:00.000Z");
95
96 test("resolves a live flow", () => {
97 const flow = start("u-short", "raw-state-4");
98 expireAt(flow.id, new Date(now.getTime() + FLOW_TTL_MS).toISOString());
99 expect(targetForShortCode(app.db, dataDir, flow.shortCode, now)).toBe(TARGET);
100 });
101
102 test("answers alike for an unknown, an expired and a finished flow", () => {
103 const expired = start("u-short", "raw-state-5");
104 expireAt(expired.id, new Date(now.getTime() - 1).toISOString());
105 const finished = start("u-short", "raw-state-6");
106 expireAt(finished.id, new Date(now.getTime() + FLOW_TTL_MS).toISOString());
107 finishFlow(app.db, finished.id, { status: "done", detail: "" });
108
109 const answers = [
110 targetForShortCode(app.db, dataDir, "zzzzzzzzzz", now),
111 targetForShortCode(app.db, dataDir, expired.shortCode, now),
112 // finishing clears the code, so the old one finds nothing either
sentinelcrew

Non-blocking: this comment gives the wrong reason. targetForShortCode filters on status = 'pending', so a finished flow returns undefined even if finishFlow stopped nulling short_code. This line pins the status filter, not the clearing. If the clearing matters, assert directly that short_code is NULL on the row.
113 targetForShortCode(app.db, dataDir, finished.shortCode, now),
114 targetForShortCode(app.db, dataDir, "", now),
115 ];
116 for (const answer of answers) expect(answer).toBeUndefined();
117 });
118 });
119
120 describe("who sees a flow", () => {
121 test("its starter gets it; anyone else gets nothing", () => {
122 const flow = start("u-starter", "raw-state-7");
123 expect(getFlowFor(app.db, "u-starter", flow.id)?.id).toBe(flow.id);
124 expect(getFlowFor(app.db, "u-intruder", flow.id)).toBeUndefined();
125 expect(getFlowFor(app.db, "u-starter", "no-such-flow")).toBeUndefined();
126 });
127 });
128
129 describe("pruning", () => {
130 const now = new Date("2031-06-01T12:00:00.000Z");
131 const iso = (ms: number): string => new Date(now.getTime() + ms).toISOString();
132 const HOUR = 60 * 60 * 1000;
133 const exists = (flowId: string): boolean =>
134 app.db.query("SELECT 1 FROM oauth_flow WHERE id = ?1").get(flowId) !== null;
135
136 test("drops expired and finished flows with their secrets, keeps the live one", () => {
137 const live = start("u-prune", "raw-state-8");
138 expireAt(live.id, iso(FLOW_TTL_MS));
139 app.db.query("UPDATE oauth_flow SET created_at = ?1 WHERE id = ?2").run(iso(-1000), live.id);
140
141 const expired = start("u-prune", "raw-state-9");
142 expireAt(expired.id, iso(-1));
143
144 const finished = start("u-prune", "raw-state-10");
145 expireAt(finished.id, iso(FLOW_TTL_MS));
146 finishFlow(app.db, finished.id, { status: "failed", detail: "denied" });
147 app.db.query("UPDATE oauth_flow SET created_at = ?1 WHERE id = ?2").run(iso(-HOUR - 1000), finished.id);
148
149 expect(flowSecrets(app.db, dataDir, live.id)?.verifier).toBe("verifier-value");
150 expect(pruneFlows(app.db, now)).toBeGreaterThanOrEqual(2);
sentinelcrew

Non-blocking: >= 2 is loose because earlier tests' flows also expire before 2031. The exists checks below carry the real assertion, so this is fine as it is.
151
152 expect(exists(live.id)).toBe(true);
153 expect(exists(expired.id)).toBe(false);
154 expect(exists(finished.id)).toBe(false);
155 expect(flowSecrets(app.db, dataDir, expired.id)).toBeUndefined();
156 expect(flowSecrets(app.db, dataDir, live.id)?.clientSecret).toBe("client-secret-value");
157 });
158 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs