History

Sweep expired authorization sessions so abandoned PKCE verifiers do not persist (#1569)

* Drop an authorization session whose completion cannot be retried An in-flight flow parks its PKCE verifier in oauth_session in plaintext, which is fine while the flow can still spend it. The happy path and cancel delete the row, and an expired redemption drops it lazily — but a completion that FAILED did not, and nothing sweeps the table. A flow that died there kept its verifier indefinitely, and for an abandoned flow the lazy path never runs. restartRequired is the authorization the code already computes for this: false means the caller may redeem the same state again, so deleting then would turn a retryable hiccup into a forced restart. Only the unredeemable case is cleaned up, best-effort, so a failed cleanup cannot replace the real error. * Type the session-cleanup test without any-casts The helper's any-typed parameters widened the Effect error and context channels to unknown, so the suite passed while typecheck failed. Inlining the flow lets the real types flow through. * Sweep expired authorization sessions when a new one starts An abandoned flow is never completed, so the lazy expiry check in complete never runs for it, and nothing else sweeps the table — its PKCE verifier sat there in plaintext indefinitely. Closing that was the larger half of the earlier session cleanup and had been left open for needing host lifecycle work. It does not: sweeping on start costs one delete on a path that is already writing, needs no scheduler in any host, and bounds the table by how often authorization is STARTED rather than by how often it is abandoned. The delete is owner-scoped by the table's own policy, so a caller only ever sweeps rows it can already see, and it is best-effort so tidying up cannot stop someone connecting an account. * Add a changeset for the authorization-session sweep * Format changeset * Log a failed expired-session sweep instead of ignoring it The sweep stays best-effort, but a silent failure would quietly reinstate the leak it exists to prevent. Adds a cross-subject isolation test and a test that a failed sweep warns and still starts the authorization. ---------

Sergio Fernández committed 2 months ago

Commit d8744551 parent 597ce903 files · +461 −1Browse files

Unified · Split

3 files
  1. .changeset/expired-authorization-session-sweep.md +11 −0
  2. packages/core/sdk/src/oauth-service.ts +44 −1
  3. packages/core/sdk/src/oauth-session-cleanup.test.ts +406 −0
.changeset/expired-authorization-session-sweep.md+11−0
@@ -0,0 +1,11 @@
1+---
2+"executor": patch
3+---
4+
5+**Abandoned authorization sessions no longer keep their PKCE verifier forever**
6+
7+An OAuth authorization session stores its PKCE verifier so the callback can redeem the code. `complete` discarded an expired session lazily, but an _abandoned_ flow is never completed, so that check never ran for it and nothing else swept the table — the verifier sat there in plaintext indefinitely.
8+
9+Starting a new authorization now sweeps sessions that have already expired. Doing it on `start` bounds the table by how often authorization is begun rather than by how often it is abandoned, and needs no scheduler in any host. A session whose completion cannot be retried is dropped rather than left behind.
10+
11+The sweep only ever reaches rows the caller can already see, so one member's authorization never touches another member's sessions. It is best-effort: a sweep that fails logs a warning and lets the authorization continue.
packages/core/sdk/src/oauth-service.ts+44−1
@@ -14,7 +14,7 @@ · 13 unmodified lines
1414 // redeems the session, exchanges the code, and mints the connection.
1515 // ---------------------------------------------------------------------------
1616
17−import { Duration, Effect, Layer, Match, Option, Schema } from "effect";
17+import { Duration, Effect, Layer, Match, Option, Predicate, Schema } from "effect";
1818 import { FetchHttpClient, type HttpClient } from "effect/unstable/http";
1919
2020 import { connectionIdentifier } from "./connection-name-identifier";
@@ -1627,6 +1627,35 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { · 1606 unmodified lines
16271627
16281628 const now = new Date();
16291629 const expiresAt = Date.now() + OAUTH2_SESSION_TTL_MS;
1630+
1631+ // Drop verifiers that have already expired before parking a new one.
1632+ // `complete` discards an expired session lazily, but an ABANDONED flow is
1633+ // never completed, so that check never runs for it — and nothing else
1634+ // sweeps this table, so its verifier would sit here in plaintext forever.
1635+ // Doing it on `start` costs one delete on a path that is already writing,
1636+ // needs no scheduler in any host, and bounds the table by how often
1637+ // authorization is STARTED rather than by how often it is abandoned.
1638+ //
1639+ // Owner-scoped by the table's own delete policy, so a caller only ever
1640+ // sweeps rows it can already see.
1641+ //
1642+ // Best-effort, but NOT silent. Failing to tidy up must not stop someone
1643+ // connecting an account, so the failure is caught — and logged, because
1644+ // this is the only caller that ever runs the sweep, so a sweep that keeps
1645+ // failing quietly reinstates the very leak it exists to prevent. Warning
1646+ // rather than error: the authorization itself is unharmed.
1647+ yield* deps.fuma
1648+ .use("oauth_session.sweepExpired", (db) =>
1649+ looseDb(db).deleteMany("oauth_session", {
1650+ where: (b: any) => b("expires_at", "<", Date.now()),
1651+ }),
1652+ )
1653+ .pipe(
1654+ Effect.catch((failure) =>
1655+ Effect.logWarning("executor oauth expired-session sweep failed", { cause: failure }),
1656+ ),
1657+ );
1658+
16301659 yield* deps.fuma.use("oauth_session.create", (db) =>
16311660 looseDb(db).create("oauth_session", {
16321661 tenant: keys.tenant,
@@ -1842,6 +1871,20 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { · 209 unmodified lines
18421871 yield* deleteSession(input.state);
18431872 return connection;
18441873 }).pipe(
1874+ // A completion that cannot be retried has finished with this session, so
1875+ // drop it rather than leaving its PKCE verifier sitting in the table. The
1876+ // happy path and `cancel` already delete; the failure paths did not, and
1877+ // nothing sweeps the table, so a flow that died here kept its verifier
1878+ // indefinitely. `restartRequired` is the authorization the code already
1879+ // computes for this: false means the caller may redeem the same state
1880+ // again, and deleting it then would turn a retryable hiccup into a
1881+ // restart. Best-effort — a failed cleanup must not replace the real
1882+ // error with a storage one.
1883+ Effect.tapError((error) =>
1884+ Predicate.isTagged(error, "OAuthCompleteError") && error.restartRequired === true
1885+ ? deleteSession(input.state).pipe(Effect.ignore)
1886+ : Effect.void,
1887+ ),
18451888 Effect.withSpan("executor.oauth.complete", {
18461889 attributes: {
18471890 "executor.oauth.grant": "authorization_code",
packages/core/sdk/src/oauth-session-cleanup.test.ts+406−0
@@ -0,0 +1,406 @@
1+import { mkdtempSync } from "node:fs";
2+import { tmpdir } from "node:os";
3+import { join } from "node:path";
4+
5+import { describe, expect, it } from "@effect/vitest";
6+import { Effect, Logger } from "effect";
7+import { withQueryContext } from "@executor-js/fumadb/query";
8+
9+import { createExecutor } from "./executor";
10+import { StorageError, type FumaDb } from "./fuma-runtime";
11+import {
12+ AuthTemplateSlug,
13+ ConnectionName,
14+ IntegrationSlug,
15+ OAuthClientSlug,
16+ ProviderItemId,
17+ ProviderKey,
18+ ToolName,
19+} from "./ids";
20+import { definePlugin } from "./plugin";
21+import { makeTestConfig, makeTestWorkspaceHarness } from "./test-config";
22+import { serveOAuthTestServer } from "./testing/oauth-test-server";
23+
24+// An in-flight authorization flow parks its PKCE verifier in `oauth_session` in
25+// plaintext, which is fine while the flow can still spend it. What is not fine is
26+// leaving it there after the flow has died: the happy path and `cancel` delete the
27+// row, but a completion that FAILED did not, and nothing sweeps the table, so the
28+// verifier outlived the flow indefinitely.
29+//
30+// Paired, like every deletion test: an unredeemable session must go, and a
31+// perfectly good one sitting beside it must not.
32+
33+const INTEG = IntegrationSlug.make("acme");
34+const TEMPLATE = AuthTemplateSlug.make("oauth");
35+const CLIENT = OAuthClientSlug.make("acme-app");
36+
37+const memoryCredentialsPlugin = definePlugin(() => {
38+ const store = new Map<string, string>();
39+ return {
40+ id: "memory-credentials" as const,
41+ storage: () => ({}),
42+ credentialProviders: [
43+ {
44+ key: ProviderKey.make("memory"),
45+ writable: true as const,
46+ get: (id: ProviderItemId) => Effect.sync(() => store.get(String(id)) ?? null),
47+ set: (id: ProviderItemId, value: string) =>
48+ Effect.sync(() => {
49+ store.set(String(id), value);
50+ }),
51+ delete: (id: ProviderItemId) =>
52+ Effect.sync(() => {
53+ store.delete(String(id));
54+ }),
55+ },
56+ ],
57+ };
58+})();
59+
60+const acmePlugin = definePlugin(() => ({
61+ id: "acme" as const,
62+ storage: () => ({}),
63+ resolveTools: () =>
64+ Effect.succeed({ tools: [{ name: ToolName.make("whoami"), description: "whoami" }] }),
65+ describeAuthMethods: () => [
66+ {
67+ id: "oauth",
68+ label: "OAuth2",
69+ kind: "oauth" as const,
70+ template: String(TEMPLATE),
71+ oauth: { scopes: [] },
72+ },
73+ ],
74+ invokeTool: ({ credential }) => Effect.succeed({ token: credential.value }),
75+ extension: (ctx) => ({
76+ seed: () => ctx.core.integrations.register({ slug: INTEG, description: "Acme", config: {} }),
77+ }),
78+}))();
79+
80+interface SessionRow {
81+ readonly pkce_verifier?: string | null;
82+ readonly expires_at?: number | null;
83+}
84+
85+/** Read a session straight from storage through the given executor's own db
86+ * handle, so the read is scoped exactly like that executor's writes. */
87+const readSession = (db: FumaDb, state: string) =>
88+ Effect.promise(
89+ () =>
90+ db.findFirst("oauth_session", {
91+ where: (b) => b("state", "=", state),
92+ }) as Promise<SessionRow | null>,
93+ );
94+
95+/** A test db whose `deleteMany` on ONE table always rejects — the storage fault
96+ * the sweep has to survive. `withContext` is forwarded through the wrapper
97+ * because the executor re-derives its own owner context from whatever db it is
98+ * handed; without that the fault would be unwrapped away on construction. */
99+const withFailingDeletes = (db: FumaDb, table: string): FumaDb =>
100+ new Proxy(db, {
101+ get(target, property) {
102+ if (property === "withContext") {
103+ return (context: unknown) => withFailingDeletes(withQueryContext(target, context), table);
104+ }
105+ if (property === "deleteMany") {
106+ return async (name: string, ...rest: readonly unknown[]) => {
107+ if (name === table) {
108+ // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: a FumaDB query reports failure by rejecting, which is the fault being simulated
109+ throw new StorageError({
110+ message: `simulated storage failure deleting from "${table}"`,
111+ cause: undefined,
112+ });
113+ }
114+ const forward = Reflect.get(target, property) as (
115+ ...args: readonly unknown[]
116+ ) => Promise<unknown>;
117+ return forward.call(target, name, ...rest);
118+ };
119+ }
120+ return Reflect.get(target, property);
121+ },
122+ });
123+
124+describe("a dead authorization flow does not keep its PKCE verifier", () => {
125+ it.effect("sweeps an expired verifier the next time authorization starts", () =>
126+ Effect.scoped(
127+ Effect.gen(function* () {
128+ const server = yield* serveOAuthTestServer({});
129+ const { executor, config } = yield* makeTestWorkspaceHarness({
130+ plugins: [memoryCredentialsPlugin, acmePlugin] as const,
131+ });
132+ yield* executor.acme.seed();
133+ yield* executor.oauth.createClient({
134+ owner: "org",
135+ slug: CLIENT,
136+ authorizationUrl: server.authorizationEndpoint,
137+ tokenUrl: server.tokenEndpoint,
138+ grant: "authorization_code",
139+ clientId: "test-client",
140+ clientSecret: "test-secret",
141+ });
142+
143+ // An abandoned flow: started, never returned to. Nothing completes it, so
144+ // the lazy expiry check in `complete` never runs for it.
145+ const abandoned = yield* executor.oauth.start({
146+ owner: "org",
147+ client: CLIENT,
148+ clientOwner: "org",
149+ name: ConnectionName.make("abandoned"),
150+ integration: INTEG,
151+ template: TEMPLATE,
152+ });
153+ if (abandoned.status !== "redirect") {
154+ return yield* Effect.die("expected a redirect-status OAuth start");
155+ }
156+ // A live flow started beside it, which must survive the sweep.
157+ const live = yield* executor.oauth.start({
158+ owner: "org",
159+ client: CLIENT,
160+ clientOwner: "org",
161+ name: ConnectionName.make("live"),
162+ integration: INTEG,
163+ template: TEMPLATE,
164+ });
165+ if (live.status !== "redirect") {
166+ return yield* Effect.die("expected a redirect-status OAuth start");
167+ }
168+
169+ // Age only the abandoned one past its expiry.
170+ yield* Effect.promise(() =>
171+ config.db.updateMany("oauth_session", {
172+ where: (b) => b("state", "=", String(abandoned.state)),
173+ set: { expires_at: Date.now() - 60_000 },
174+ }),
175+ );
176+ expect((yield* readSession(config.db, String(abandoned.state)))?.pkce_verifier).toEqual(
177+ expect.any(String),
178+ );
179+
180+ // Starting any authorization is what tidies up.
181+ const third = yield* executor.oauth.start({
182+ owner: "org",
183+ client: CLIENT,
184+ clientOwner: "org",
185+ name: ConnectionName.make("third"),
186+ integration: INTEG,
187+ template: TEMPLATE,
188+ });
189+ if (third.status !== "redirect") {
190+ return yield* Effect.die("expected a redirect-status OAuth start");
191+ }
192+
193+ expect(yield* readSession(config.db, String(abandoned.state))).toBeNull();
194+ // The unexpired flow is untouched — a sweep must not cancel someone
195+ // else's authorization mid-flight.
196+ expect((yield* readSession(config.db, String(live.state)))?.pkce_verifier).toEqual(
197+ expect.any(String),
198+ );
199+ }),
200+ ),
201+ );
202+
203+ it.effect("drops the session when the completion cannot be retried", () =>
204+ Effect.scoped(
205+ Effect.gen(function* () {
206+ const server = yield* serveOAuthTestServer({});
207+ const { executor, config } = yield* makeTestWorkspaceHarness({
208+ plugins: [memoryCredentialsPlugin, acmePlugin] as const,
209+ });
210+ yield* executor.acme.seed();
211+ yield* executor.oauth.createClient({
212+ owner: "org",
213+ slug: CLIENT,
214+ authorizationUrl: server.authorizationEndpoint,
215+ tokenUrl: server.tokenEndpoint,
216+ grant: "authorization_code",
217+ clientId: "test-client",
218+ clientSecret: "test-secret",
219+ });
220+
221+ const dying = yield* executor.oauth.start({
222+ owner: "org",
223+ client: CLIENT,
224+ clientOwner: "org",
225+ name: ConnectionName.make("dying"),
226+ integration: INTEG,
227+ template: TEMPLATE,
228+ });
229+ if (dying.status !== "redirect") {
230+ return yield* Effect.die("expected a redirect-status OAuth start");
231+ }
232+ const dyingCallback = yield* server.completeAuthorizationCodeFlow({
233+ authorizationUrl: dying.authorizationUrl,
234+ });
235+
236+ const bystander = yield* executor.oauth.start({
237+ owner: "org",
238+ client: CLIENT,
239+ clientOwner: "org",
240+ name: ConnectionName.make("bystander"),
241+ integration: INTEG,
242+ template: TEMPLATE,
243+ });
244+ if (bystander.status !== "redirect") {
245+ return yield* Effect.die("expected a redirect-status OAuth start");
246+ }
247+
248+ // The verifier really is sitting there in plaintext.
249+ const before = yield* readSession(config.db, String(dying.state));
250+ expect(before?.pkce_verifier).toEqual(expect.any(String));
251+
252+ // Remove the app this flow was started against, so completion fails with
253+ // restartRequired — this state can never be redeemed again.
254+ yield* executor.oauth.removeClient("org", CLIENT);
255+ const failed = yield* Effect.flip(
256+ executor.oauth.complete({ state: dying.state, code: dyingCallback.code }),
257+ );
258+ expect(JSON.stringify(failed)).toContain("restartRequired");
259+
260+ expect(yield* readSession(config.db, String(dying.state))).toBeNull();
261+ // The other flow is still live and untouched — a cleanup must not sweep
262+ // sessions it was not asked about.
263+ const survivor = yield* readSession(config.db, String(bystander.state));
264+ expect(survivor?.pkce_verifier).toEqual(expect.any(String));
265+ }),
266+ ),
267+ );
268+
269+ // The sweep runs with no owner clause of its own — it trusts the table's
270+ // delete policy to scope it. That trust is the whole safety argument, and it
271+ // is invisible at the call site, so it is pinned here: one member starting an
272+ // authorization must never reach into another member's rows, however stale
273+ // they are. Two subjects on ONE database is the only way to observe it.
274+ it.effect("one member's sweep leaves another member's expired session alone", () =>
275+ Effect.scoped(
276+ Effect.gen(function* () {
277+ const server = yield* serveOAuthTestServer({});
278+ const dataDir = mkdtempSync(join(tmpdir(), "oauth-session-sweep-"));
279+ const tenant = "shared-tenant";
280+ const plugins = [memoryCredentialsPlugin, acmePlugin] as const;
281+
282+ const a = yield* makeTestWorkspaceHarness({
283+ plugins,
284+ tenant,
285+ subject: "subject-a",
286+ dataDir,
287+ });
288+ yield* a.executor.acme.seed();
289+ yield* a.executor.oauth.createClient({
290+ owner: "org",
291+ slug: CLIENT,
292+ authorizationUrl: server.authorizationEndpoint,
293+ tokenUrl: server.tokenEndpoint,
294+ grant: "authorization_code",
295+ clientId: "test-client",
296+ clientSecret: "test-secret",
297+ });
298+
299+ // A second member of the same tenant, on the same database.
300+ const b = yield* makeTestWorkspaceHarness({
301+ plugins,
302+ tenant,
303+ subject: "subject-b",
304+ dataDir,
305+ });
306+
307+ // Both members abandon a USER-owned flow. `owner: "user"` is what makes
308+ // the rows private to each subject; an org row is tenant-shared by
309+ // design and its removal by any member is correct.
310+ const startUserFlow = (harness: typeof a, name: string) =>
311+ Effect.gen(function* () {
312+ const started = yield* harness.executor.oauth.start({
313+ owner: "user",
314+ client: CLIENT,
315+ clientOwner: "org",
316+ name: ConnectionName.make(name),
317+ integration: INTEG,
318+ template: TEMPLATE,
319+ });
320+ if (started.status !== "redirect") {
321+ return yield* Effect.die("expected a redirect-status OAuth start");
322+ }
323+ return started;
324+ });
325+
326+ const expire = (db: FumaDb, state: string) =>
327+ Effect.promise(() =>
328+ db.updateMany("oauth_session", {
329+ where: (builder) => builder("state", "=", state),
330+ set: { expires_at: Date.now() - 60_000 },
331+ }),
332+ );
333+
334+ const bStale = yield* startUserFlow(b, "b-stale");
335+ yield* expire(b.config.db, String(bStale.state));
336+
337+ const aStale = yield* startUserFlow(a, "a-stale");
338+ yield* expire(a.config.db, String(aStale.state));
339+
340+ // B's row really is a sweep candidate — otherwise surviving would prove
341+ // nothing about scoping.
342+ const bBefore = yield* readSession(b.config.db, String(bStale.state));
343+ expect(Number(bBefore?.expires_at)).toBeLessThan(Date.now());
344+
345+ // A starts again. Its sweep must reach its OWN expired row and no
346+ // further.
347+ yield* startUserFlow(a, "a-fresh");
348+
349+ expect(yield* readSession(a.config.db, String(aStale.state))).toBeNull();
350+ expect((yield* readSession(b.config.db, String(bStale.state)))?.pkce_verifier).toEqual(
351+ expect.any(String),
352+ );
353+ }),
354+ ),
355+ );
356+
357+ // Best-effort must not mean silent. The sweep is the only thing keeping this
358+ // table bounded, and `start` is the only caller that runs it, so a sweep that
359+ // fails every time would otherwise reinstate the original leak with nothing
360+ // anywhere to say so.
361+ it.effect("warns when the sweep fails, and starts the authorization anyway", () =>
362+ Effect.scoped(
363+ Effect.gen(function* () {
364+ const server = yield* serveOAuthTestServer({});
365+ const base = makeTestConfig({
366+ plugins: [memoryCredentialsPlugin, acmePlugin] as const,
367+ });
368+ const executor = yield* createExecutor({
369+ ...base,
370+ db: withFailingDeletes(base.db, "oauth_session"),
371+ });
372+ yield* Effect.addFinalizer(() => executor.close().pipe(Effect.ignore));
373+ yield* executor.acme.seed();
374+ yield* executor.oauth.createClient({
375+ owner: "org",
376+ slug: CLIENT,
377+ authorizationUrl: server.authorizationEndpoint,
378+ tokenUrl: server.tokenEndpoint,
379+ grant: "authorization_code",
380+ clientId: "test-client",
381+ clientSecret: "test-secret",
382+ });
383+
384+ const warnings: string[] = [];
385+ const capture = Logger.make<unknown, void>(({ logLevel, message }) => {
386+ if (logLevel === "Warn") warnings.push(JSON.stringify(message));
387+ });
388+
389+ const started = yield* executor.oauth
390+ .start({
391+ owner: "org",
392+ client: CLIENT,
393+ clientOwner: "org",
394+ name: ConnectionName.make("undeterred"),
395+ integration: INTEG,
396+ template: TEMPLATE,
397+ })
398+ .pipe(Effect.provide(Logger.layer([capture])));
399+
400+ // The authorization is unharmed: the caller still gets somewhere to go.
401+ expect(started.status).toBe("redirect");
402+ expect(warnings.join("\n")).toContain("expired-session sweep failed");
403+ }),
404+ ),
405+ );
406+});