From 848bc84a5760f46c109df7f23fb6918a1a4329ac Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Wed, 12 Aug 2026 17:34:47 +0200 Subject: [PATCH 1/6] Drop an authorization session whose completion cannot be retried MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/core/sdk/src/oauth-service.ts | 16 +- .../sdk/src/oauth-session-cleanup.test.ts | 139 ++++++++++++++++++ 2 files changed, 154 insertions(+), 1 deletion(-) create mode 100644 packages/core/sdk/src/oauth-session-cleanup.test.ts diff --git a/packages/core/sdk/src/oauth-service.ts b/packages/core/sdk/src/oauth-service.ts index 742cf65ad..8a4e5736a 100644 --- a/packages/core/sdk/src/oauth-service.ts +++ b/packages/core/sdk/src/oauth-service.ts @@ -14,7 +14,7 @@ // redeems the session, exchanges the code, and mints the connection. // --------------------------------------------------------------------------- -import { Duration, Effect, Layer, Option, Schema } from "effect"; +import { Duration, Effect, Layer, Option, Predicate, Schema } from "effect"; import { FetchHttpClient, type HttpClient } from "effect/unstable/http"; import { connectionIdentifier } from "./connection-name-identifier"; @@ -1395,6 +1395,20 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { yield* deleteSession(input.state); return connection; }).pipe( + // A completion that cannot be retried has finished with this session, so + // drop it rather than leaving its PKCE verifier sitting in the table. The + // happy path and `cancel` already delete; the failure paths did not, and + // nothing sweeps the table, so a flow that died here kept its verifier + // indefinitely. `restartRequired` is the authorization the code already + // computes for this: false means the caller may redeem the same state + // again, and deleting it then would turn a retryable hiccup into a + // restart. Best-effort — a failed cleanup must not replace the real + // error with a storage one. + Effect.tapError((error) => + Predicate.isTagged(error, "OAuthCompleteError") && error.restartRequired === true + ? deleteSession(input.state).pipe(Effect.ignore) + : Effect.void, + ), Effect.withSpan("executor.oauth.complete", { attributes: { "executor.oauth.grant": "authorization_code", diff --git a/packages/core/sdk/src/oauth-session-cleanup.test.ts b/packages/core/sdk/src/oauth-session-cleanup.test.ts new file mode 100644 index 000000000..6b4b69880 --- /dev/null +++ b/packages/core/sdk/src/oauth-session-cleanup.test.ts @@ -0,0 +1,139 @@ +import { describe, expect, it } from "@effect/vitest"; +import { Effect } from "effect"; + +import { + AuthTemplateSlug, + ConnectionName, + IntegrationSlug, + OAuthClientSlug, + ProviderItemId, + ProviderKey, + ToolName, +} from "./ids"; +import { definePlugin } from "./plugin"; +import { makeTestWorkspaceHarness } from "./test-config"; +import { serveOAuthTestServer } from "./testing/oauth-test-server"; + +// An in-flight authorization flow parks its PKCE verifier in `oauth_session` in +// plaintext, which is fine while the flow can still spend it. What is not fine is +// leaving it there after the flow has died: the happy path and `cancel` delete the +// row, but a completion that FAILED did not, and nothing sweeps the table, so the +// verifier outlived the flow indefinitely. +// +// Paired, like every deletion test: an unredeemable session must go, and a +// perfectly good one sitting beside it must not. + +const INTEG = IntegrationSlug.make("acme"); +const TEMPLATE = AuthTemplateSlug.make("oauth"); +const CLIENT = OAuthClientSlug.make("acme-app"); + +const memoryCredentialsPlugin = definePlugin(() => { + const store = new Map(); + return { + id: "memory-credentials" as const, + storage: () => ({}), + credentialProviders: [ + { + key: ProviderKey.make("memory"), + writable: true as const, + get: (id: ProviderItemId) => Effect.sync(() => store.get(String(id)) ?? null), + set: (id: ProviderItemId, value: string) => + Effect.sync(() => { + store.set(String(id), value); + }), + delete: (id: ProviderItemId) => + Effect.sync(() => { + store.delete(String(id)); + }), + }, + ], + }; +})(); + +const acmePlugin = definePlugin(() => ({ + id: "acme" as const, + storage: () => ({}), + resolveTools: () => + Effect.succeed({ tools: [{ name: ToolName.make("whoami"), description: "whoami" }] }), + describeAuthMethods: () => [ + { + id: "oauth", + label: "OAuth2", + kind: "oauth" as const, + template: String(TEMPLATE), + oauth: { scopes: [] }, + }, + ], + invokeTool: ({ credential }) => Effect.succeed({ token: credential.value }), + extension: (ctx) => ({ + seed: () => ctx.core.integrations.register({ slug: INTEG, description: "Acme", config: {} }), + }), +}))(); + +const startFlow = (executor: any, server: any, name: string) => + Effect.gen(function* () { + const started = yield* executor.oauth.start({ + owner: "org", + client: CLIENT, + clientOwner: "org", + name: ConnectionName.make(name), + integration: INTEG, + template: TEMPLATE, + }); + if (started.status !== "redirect") { + return yield* Effect.die("expected a redirect-status OAuth start"); + } + const callback = yield* server.completeAuthorizationCodeFlow({ + authorizationUrl: started.authorizationUrl, + }); + return { state: started.state, code: callback.code }; + }); + +const sessionRow = (config: any, state: string) => + Effect.promise(() => + config.db.findFirst("oauth_session", { where: (b: any) => b("state", "=", state) }), + ); + +describe("a dead authorization flow does not keep its PKCE verifier", () => { + it.effect("drops the session when the completion cannot be retried", () => + Effect.scoped( + Effect.gen(function* () { + const server = yield* serveOAuthTestServer({}); + const { executor, config } = yield* makeTestWorkspaceHarness({ + plugins: [memoryCredentialsPlugin, acmePlugin] as const, + }); + yield* executor.acme.seed(); + yield* executor.oauth.createClient({ + owner: "org", + slug: CLIENT, + authorizationUrl: server.authorizationEndpoint, + tokenUrl: server.tokenEndpoint, + grant: "authorization_code", + clientId: "test-client", + clientSecret: "test-secret", + }); + + const dying = yield* startFlow(executor, server, "dying"); + const bystander = yield* startFlow(executor, server, "bystander"); + + // The verifier really is sitting there in plaintext. + const before = yield* sessionRow(config, dying.state); + expect(before?.pkce_verifier).toEqual(expect.any(String)); + + // Remove the app the flow was started against. Completion now fails with + // restartRequired, so this state can never be redeemed again. + yield* executor.oauth.removeClient("org", CLIENT); + const failed = yield* Effect.flip( + executor.oauth.complete({ state: dying.state, code: dying.code }), + ); + expect(JSON.stringify(failed)).toContain("restartRequired"); + + expect(yield* sessionRow(config, dying.state)).toBeNull(); + // The other flow is still live and untouched — a cleanup must not sweep + // sessions it was not asked about. + const survivor = yield* sessionRow(config, bystander.state); + expect(survivor?.pkce_verifier).toEqual(expect.any(String)); + }), + ), + ); +}); From e7101f72deafc107ff7f8ddd37a405bb46f5ca6f Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Wed, 12 Aug 2026 17:44:01 +0200 Subject: [PATCH 2/6] 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. --- .../sdk/src/oauth-session-cleanup.test.ts | 74 +++++++++++-------- 1 file changed, 43 insertions(+), 31 deletions(-) diff --git a/packages/core/sdk/src/oauth-session-cleanup.test.ts b/packages/core/sdk/src/oauth-session-cleanup.test.ts index 6b4b69880..0ee76be9f 100644 --- a/packages/core/sdk/src/oauth-session-cleanup.test.ts +++ b/packages/core/sdk/src/oauth-session-cleanup.test.ts @@ -70,29 +70,9 @@ const acmePlugin = definePlugin(() => ({ }), }))(); -const startFlow = (executor: any, server: any, name: string) => - Effect.gen(function* () { - const started = yield* executor.oauth.start({ - owner: "org", - client: CLIENT, - clientOwner: "org", - name: ConnectionName.make(name), - integration: INTEG, - template: TEMPLATE, - }); - if (started.status !== "redirect") { - return yield* Effect.die("expected a redirect-status OAuth start"); - } - const callback = yield* server.completeAuthorizationCodeFlow({ - authorizationUrl: started.authorizationUrl, - }); - return { state: started.state, code: callback.code }; - }); - -const sessionRow = (config: any, state: string) => - Effect.promise(() => - config.db.findFirst("oauth_session", { where: (b: any) => b("state", "=", state) }), - ); +interface SessionRow { + readonly pkce_verifier?: string | null; +} describe("a dead authorization flow does not keep its PKCE verifier", () => { it.effect("drops the session when the completion cannot be retried", () => @@ -113,25 +93,57 @@ describe("a dead authorization flow does not keep its PKCE verifier", () => { clientSecret: "test-secret", }); - const dying = yield* startFlow(executor, server, "dying"); - const bystander = yield* startFlow(executor, server, "bystander"); + const readSession = (state: string) => + Effect.promise( + () => + config.db.findFirst("oauth_session", { + where: (b) => b("state", "=", state), + }) as Promise, + ); + + const dying = yield* executor.oauth.start({ + owner: "org", + client: CLIENT, + clientOwner: "org", + name: ConnectionName.make("dying"), + integration: INTEG, + template: TEMPLATE, + }); + if (dying.status !== "redirect") { + return yield* Effect.die("expected a redirect-status OAuth start"); + } + const dyingCallback = yield* server.completeAuthorizationCodeFlow({ + authorizationUrl: dying.authorizationUrl, + }); + + const bystander = yield* executor.oauth.start({ + owner: "org", + client: CLIENT, + clientOwner: "org", + name: ConnectionName.make("bystander"), + integration: INTEG, + template: TEMPLATE, + }); + if (bystander.status !== "redirect") { + return yield* Effect.die("expected a redirect-status OAuth start"); + } // The verifier really is sitting there in plaintext. - const before = yield* sessionRow(config, dying.state); + const before = yield* readSession(String(dying.state)); expect(before?.pkce_verifier).toEqual(expect.any(String)); - // Remove the app the flow was started against. Completion now fails with - // restartRequired, so this state can never be redeemed again. + // Remove the app this flow was started against, so completion fails with + // restartRequired — this state can never be redeemed again. yield* executor.oauth.removeClient("org", CLIENT); const failed = yield* Effect.flip( - executor.oauth.complete({ state: dying.state, code: dying.code }), + executor.oauth.complete({ state: dying.state, code: dyingCallback.code }), ); expect(JSON.stringify(failed)).toContain("restartRequired"); - expect(yield* sessionRow(config, dying.state)).toBeNull(); + expect(yield* readSession(String(dying.state))).toBeNull(); // The other flow is still live and untouched — a cleanup must not sweep // sessions it was not asked about. - const survivor = yield* sessionRow(config, bystander.state); + const survivor = yield* readSession(String(bystander.state)); expect(survivor?.pkce_verifier).toEqual(expect.any(String)); }), ), From 5c9eb3d8af4437f4c1418b7bb6836220f3749eb4 Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Wed, 12 Aug 2026 19:09:13 +0200 Subject: [PATCH 3/6] Sweep expired authorization sessions when a new one starts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/core/sdk/src/oauth-service.ts | 20 +++++ .../sdk/src/oauth-session-cleanup.test.ts | 84 +++++++++++++++++++ 2 files changed, 104 insertions(+) diff --git a/packages/core/sdk/src/oauth-service.ts b/packages/core/sdk/src/oauth-service.ts index 8a4e5736a..cb0e82b46 100644 --- a/packages/core/sdk/src/oauth-service.ts +++ b/packages/core/sdk/src/oauth-service.ts @@ -1197,6 +1197,26 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { const now = new Date(); const expiresAt = Date.now() + OAUTH2_SESSION_TTL_MS; + + // Drop verifiers that have already expired before parking a new one. + // `complete` discards an expired session lazily, but an ABANDONED flow is + // never completed, so that check never runs for it — and nothing else + // sweeps this table, so its verifier would sit here in plaintext forever. + // Doing it 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. + // + // Owner-scoped by the table's own delete policy, so a caller only ever + // sweeps rows it can already see. Best-effort: failing to tidy up must not + // stop someone connecting an account. + yield* deps.fuma + .use("oauth_session.sweepExpired", (db) => + looseDb(db).deleteMany("oauth_session", { + where: (b: any) => b("expires_at", "<", Date.now()), + }), + ) + .pipe(Effect.ignore); + yield* deps.fuma.use("oauth_session.create", (db) => looseDb(db).create("oauth_session", { tenant: keys.tenant, diff --git a/packages/core/sdk/src/oauth-session-cleanup.test.ts b/packages/core/sdk/src/oauth-session-cleanup.test.ts index 0ee76be9f..e47b25c8c 100644 --- a/packages/core/sdk/src/oauth-session-cleanup.test.ts +++ b/packages/core/sdk/src/oauth-session-cleanup.test.ts @@ -75,6 +75,90 @@ interface SessionRow { } describe("a dead authorization flow does not keep its PKCE verifier", () => { + it.effect("sweeps an expired verifier the next time authorization starts", () => + Effect.scoped( + Effect.gen(function* () { + const server = yield* serveOAuthTestServer({}); + const { executor, config } = yield* makeTestWorkspaceHarness({ + plugins: [memoryCredentialsPlugin, acmePlugin] as const, + }); + yield* executor.acme.seed(); + yield* executor.oauth.createClient({ + owner: "org", + slug: CLIENT, + authorizationUrl: server.authorizationEndpoint, + tokenUrl: server.tokenEndpoint, + grant: "authorization_code", + clientId: "test-client", + clientSecret: "test-secret", + }); + + const readSession = (state: string) => + Effect.promise( + () => + config.db.findFirst("oauth_session", { + where: (b) => b("state", "=", state), + }) as Promise, + ); + + // An abandoned flow: started, never returned to. Nothing completes it, so + // the lazy expiry check in `complete` never runs for it. + const abandoned = yield* executor.oauth.start({ + owner: "org", + client: CLIENT, + clientOwner: "org", + name: ConnectionName.make("abandoned"), + integration: INTEG, + template: TEMPLATE, + }); + if (abandoned.status !== "redirect") { + return yield* Effect.die("expected a redirect-status OAuth start"); + } + // A live flow started beside it, which must survive the sweep. + const live = yield* executor.oauth.start({ + owner: "org", + client: CLIENT, + clientOwner: "org", + name: ConnectionName.make("live"), + integration: INTEG, + template: TEMPLATE, + }); + if (live.status !== "redirect") { + return yield* Effect.die("expected a redirect-status OAuth start"); + } + + // Age only the abandoned one past its expiry. + yield* Effect.promise(() => + config.db.updateMany("oauth_session", { + where: (b) => b("state", "=", String(abandoned.state)), + set: { expires_at: Date.now() - 60_000 }, + }), + ); + expect((yield* readSession(String(abandoned.state)))?.pkce_verifier).toEqual( + expect.any(String), + ); + + // Starting any authorization is what tidies up. + const third = yield* executor.oauth.start({ + owner: "org", + client: CLIENT, + clientOwner: "org", + name: ConnectionName.make("third"), + integration: INTEG, + template: TEMPLATE, + }); + if (third.status !== "redirect") { + return yield* Effect.die("expected a redirect-status OAuth start"); + } + + expect(yield* readSession(String(abandoned.state))).toBeNull(); + // The unexpired flow is untouched — a sweep must not cancel someone + // else's authorization mid-flight. + expect((yield* readSession(String(live.state)))?.pkce_verifier).toEqual(expect.any(String)); + }), + ), + ); + it.effect("drops the session when the completion cannot be retried", () => Effect.scoped( Effect.gen(function* () { From e393a8762ba5d9dd0795f67af2dd6d10d9ade6a7 Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Wed, 12 Aug 2026 22:56:37 +0200 Subject: [PATCH 4/6] Add a changeset for the authorization-session sweep --- .changeset/expired-authorization-session-sweep.md | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 .changeset/expired-authorization-session-sweep.md diff --git a/.changeset/expired-authorization-session-sweep.md b/.changeset/expired-authorization-session-sweep.md new file mode 100644 index 000000000..dbd89bf4b --- /dev/null +++ b/.changeset/expired-authorization-session-sweep.md @@ -0,0 +1,9 @@ +--- +"executor": patch +--- + +**Abandoned authorization sessions no longer keep their PKCE verifier forever** + +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. + +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. From fb0985e0cf02b5cf74680fcb475ea548565834ca Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 27 Aug 2026 21:17:09 -0700 Subject: [PATCH 5/6] Format changeset --- .changeset/expired-authorization-session-sweep.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/expired-authorization-session-sweep.md b/.changeset/expired-authorization-session-sweep.md index dbd89bf4b..bf8c0bae8 100644 --- a/.changeset/expired-authorization-session-sweep.md +++ b/.changeset/expired-authorization-session-sweep.md @@ -4,6 +4,6 @@ **Abandoned authorization sessions no longer keep their PKCE verifier forever** -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. +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. 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. From b0b6ef8f1ad36336d34c757588cb2b427424dcd8 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 27 Aug 2026 23:30:07 -0700 Subject: [PATCH 6/6] 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. --- .../expired-authorization-session-sweep.md | 2 + packages/core/sdk/src/oauth-service.ts | 15 +- .../sdk/src/oauth-session-cleanup.test.ts | 219 ++++++++++++++++-- 3 files changed, 209 insertions(+), 27 deletions(-) diff --git a/.changeset/expired-authorization-session-sweep.md b/.changeset/expired-authorization-session-sweep.md index bf8c0bae8..60cf946da 100644 --- a/.changeset/expired-authorization-session-sweep.md +++ b/.changeset/expired-authorization-session-sweep.md @@ -7,3 +7,5 @@ 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. 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. + +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. diff --git a/packages/core/sdk/src/oauth-service.ts b/packages/core/sdk/src/oauth-service.ts index 189b4af66..85e069aed 100644 --- a/packages/core/sdk/src/oauth-service.ts +++ b/packages/core/sdk/src/oauth-service.ts @@ -1637,15 +1637,24 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { // authorization is STARTED rather than by how often it is abandoned. // // Owner-scoped by the table's own delete policy, so a caller only ever - // sweeps rows it can already see. Best-effort: failing to tidy up must not - // stop someone connecting an account. + // sweeps rows it can already see. + // + // Best-effort, but NOT silent. Failing to tidy up must not stop someone + // connecting an account, so the failure is caught — and logged, because + // this is the only caller that ever runs the sweep, so a sweep that keeps + // failing quietly reinstates the very leak it exists to prevent. Warning + // rather than error: the authorization itself is unharmed. yield* deps.fuma .use("oauth_session.sweepExpired", (db) => looseDb(db).deleteMany("oauth_session", { where: (b: any) => b("expires_at", "<", Date.now()), }), ) - .pipe(Effect.ignore); + .pipe( + Effect.catch((failure) => + Effect.logWarning("executor oauth expired-session sweep failed", { cause: failure }), + ), + ); yield* deps.fuma.use("oauth_session.create", (db) => looseDb(db).create("oauth_session", { diff --git a/packages/core/sdk/src/oauth-session-cleanup.test.ts b/packages/core/sdk/src/oauth-session-cleanup.test.ts index e47b25c8c..7517c274d 100644 --- a/packages/core/sdk/src/oauth-session-cleanup.test.ts +++ b/packages/core/sdk/src/oauth-session-cleanup.test.ts @@ -1,6 +1,13 @@ +import { mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + import { describe, expect, it } from "@effect/vitest"; -import { Effect } from "effect"; +import { Effect, Logger } from "effect"; +import { withQueryContext } from "@executor-js/fumadb/query"; +import { createExecutor } from "./executor"; +import { StorageError, type FumaDb } from "./fuma-runtime"; import { AuthTemplateSlug, ConnectionName, @@ -11,7 +18,7 @@ import { ToolName, } from "./ids"; import { definePlugin } from "./plugin"; -import { makeTestWorkspaceHarness } from "./test-config"; +import { makeTestConfig, makeTestWorkspaceHarness } from "./test-config"; import { serveOAuthTestServer } from "./testing/oauth-test-server"; // An in-flight authorization flow parks its PKCE verifier in `oauth_session` in @@ -72,8 +79,48 @@ const acmePlugin = definePlugin(() => ({ interface SessionRow { readonly pkce_verifier?: string | null; + readonly expires_at?: number | null; } +/** Read a session straight from storage through the given executor's own db + * handle, so the read is scoped exactly like that executor's writes. */ +const readSession = (db: FumaDb, state: string) => + Effect.promise( + () => + db.findFirst("oauth_session", { + where: (b) => b("state", "=", state), + }) as Promise, + ); + +/** A test db whose `deleteMany` on ONE table always rejects — the storage fault + * the sweep has to survive. `withContext` is forwarded through the wrapper + * because the executor re-derives its own owner context from whatever db it is + * handed; without that the fault would be unwrapped away on construction. */ +const withFailingDeletes = (db: FumaDb, table: string): FumaDb => + new Proxy(db, { + get(target, property) { + if (property === "withContext") { + return (context: unknown) => withFailingDeletes(withQueryContext(target, context), table); + } + if (property === "deleteMany") { + return async (name: string, ...rest: readonly unknown[]) => { + if (name === table) { + // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: a FumaDB query reports failure by rejecting, which is the fault being simulated + throw new StorageError({ + message: `simulated storage failure deleting from "${table}"`, + cause: undefined, + }); + } + const forward = Reflect.get(target, property) as ( + ...args: readonly unknown[] + ) => Promise; + return forward.call(target, name, ...rest); + }; + } + return Reflect.get(target, property); + }, + }); + describe("a dead authorization flow does not keep its PKCE verifier", () => { it.effect("sweeps an expired verifier the next time authorization starts", () => Effect.scoped( @@ -93,14 +140,6 @@ describe("a dead authorization flow does not keep its PKCE verifier", () => { clientSecret: "test-secret", }); - const readSession = (state: string) => - Effect.promise( - () => - config.db.findFirst("oauth_session", { - where: (b) => b("state", "=", state), - }) as Promise, - ); - // An abandoned flow: started, never returned to. Nothing completes it, so // the lazy expiry check in `complete` never runs for it. const abandoned = yield* executor.oauth.start({ @@ -134,7 +173,7 @@ describe("a dead authorization flow does not keep its PKCE verifier", () => { set: { expires_at: Date.now() - 60_000 }, }), ); - expect((yield* readSession(String(abandoned.state)))?.pkce_verifier).toEqual( + expect((yield* readSession(config.db, String(abandoned.state)))?.pkce_verifier).toEqual( expect.any(String), ); @@ -151,10 +190,12 @@ describe("a dead authorization flow does not keep its PKCE verifier", () => { return yield* Effect.die("expected a redirect-status OAuth start"); } - expect(yield* readSession(String(abandoned.state))).toBeNull(); + expect(yield* readSession(config.db, String(abandoned.state))).toBeNull(); // The unexpired flow is untouched — a sweep must not cancel someone // else's authorization mid-flight. - expect((yield* readSession(String(live.state)))?.pkce_verifier).toEqual(expect.any(String)); + expect((yield* readSession(config.db, String(live.state)))?.pkce_verifier).toEqual( + expect.any(String), + ); }), ), ); @@ -177,14 +218,6 @@ describe("a dead authorization flow does not keep its PKCE verifier", () => { clientSecret: "test-secret", }); - const readSession = (state: string) => - Effect.promise( - () => - config.db.findFirst("oauth_session", { - where: (b) => b("state", "=", state), - }) as Promise, - ); - const dying = yield* executor.oauth.start({ owner: "org", client: CLIENT, @@ -213,7 +246,7 @@ describe("a dead authorization flow does not keep its PKCE verifier", () => { } // The verifier really is sitting there in plaintext. - const before = yield* readSession(String(dying.state)); + const before = yield* readSession(config.db, String(dying.state)); expect(before?.pkce_verifier).toEqual(expect.any(String)); // Remove the app this flow was started against, so completion fails with @@ -224,12 +257,150 @@ describe("a dead authorization flow does not keep its PKCE verifier", () => { ); expect(JSON.stringify(failed)).toContain("restartRequired"); - expect(yield* readSession(String(dying.state))).toBeNull(); + expect(yield* readSession(config.db, String(dying.state))).toBeNull(); // The other flow is still live and untouched — a cleanup must not sweep // sessions it was not asked about. - const survivor = yield* readSession(String(bystander.state)); + const survivor = yield* readSession(config.db, String(bystander.state)); expect(survivor?.pkce_verifier).toEqual(expect.any(String)); }), ), ); + + // The sweep runs with no owner clause of its own — it trusts the table's + // delete policy to scope it. That trust is the whole safety argument, and it + // is invisible at the call site, so it is pinned here: one member starting an + // authorization must never reach into another member's rows, however stale + // they are. Two subjects on ONE database is the only way to observe it. + it.effect("one member's sweep leaves another member's expired session alone", () => + Effect.scoped( + Effect.gen(function* () { + const server = yield* serveOAuthTestServer({}); + const dataDir = mkdtempSync(join(tmpdir(), "oauth-session-sweep-")); + const tenant = "shared-tenant"; + const plugins = [memoryCredentialsPlugin, acmePlugin] as const; + + const a = yield* makeTestWorkspaceHarness({ + plugins, + tenant, + subject: "subject-a", + dataDir, + }); + yield* a.executor.acme.seed(); + yield* a.executor.oauth.createClient({ + owner: "org", + slug: CLIENT, + authorizationUrl: server.authorizationEndpoint, + tokenUrl: server.tokenEndpoint, + grant: "authorization_code", + clientId: "test-client", + clientSecret: "test-secret", + }); + + // A second member of the same tenant, on the same database. + const b = yield* makeTestWorkspaceHarness({ + plugins, + tenant, + subject: "subject-b", + dataDir, + }); + + // Both members abandon a USER-owned flow. `owner: "user"` is what makes + // the rows private to each subject; an org row is tenant-shared by + // design and its removal by any member is correct. + const startUserFlow = (harness: typeof a, name: string) => + Effect.gen(function* () { + const started = yield* harness.executor.oauth.start({ + owner: "user", + client: CLIENT, + clientOwner: "org", + name: ConnectionName.make(name), + integration: INTEG, + template: TEMPLATE, + }); + if (started.status !== "redirect") { + return yield* Effect.die("expected a redirect-status OAuth start"); + } + return started; + }); + + const expire = (db: FumaDb, state: string) => + Effect.promise(() => + db.updateMany("oauth_session", { + where: (builder) => builder("state", "=", state), + set: { expires_at: Date.now() - 60_000 }, + }), + ); + + const bStale = yield* startUserFlow(b, "b-stale"); + yield* expire(b.config.db, String(bStale.state)); + + const aStale = yield* startUserFlow(a, "a-stale"); + yield* expire(a.config.db, String(aStale.state)); + + // B's row really is a sweep candidate — otherwise surviving would prove + // nothing about scoping. + const bBefore = yield* readSession(b.config.db, String(bStale.state)); + expect(Number(bBefore?.expires_at)).toBeLessThan(Date.now()); + + // A starts again. Its sweep must reach its OWN expired row and no + // further. + yield* startUserFlow(a, "a-fresh"); + + expect(yield* readSession(a.config.db, String(aStale.state))).toBeNull(); + expect((yield* readSession(b.config.db, String(bStale.state)))?.pkce_verifier).toEqual( + expect.any(String), + ); + }), + ), + ); + + // Best-effort must not mean silent. The sweep is the only thing keeping this + // table bounded, and `start` is the only caller that runs it, so a sweep that + // fails every time would otherwise reinstate the original leak with nothing + // anywhere to say so. + it.effect("warns when the sweep fails, and starts the authorization anyway", () => + Effect.scoped( + Effect.gen(function* () { + const server = yield* serveOAuthTestServer({}); + const base = makeTestConfig({ + plugins: [memoryCredentialsPlugin, acmePlugin] as const, + }); + const executor = yield* createExecutor({ + ...base, + db: withFailingDeletes(base.db, "oauth_session"), + }); + yield* Effect.addFinalizer(() => executor.close().pipe(Effect.ignore)); + yield* executor.acme.seed(); + yield* executor.oauth.createClient({ + owner: "org", + slug: CLIENT, + authorizationUrl: server.authorizationEndpoint, + tokenUrl: server.tokenEndpoint, + grant: "authorization_code", + clientId: "test-client", + clientSecret: "test-secret", + }); + + const warnings: string[] = []; + const capture = Logger.make(({ logLevel, message }) => { + if (logLevel === "Warn") warnings.push(JSON.stringify(message)); + }); + + const started = yield* executor.oauth + .start({ + owner: "org", + client: CLIENT, + clientOwner: "org", + name: ConnectionName.make("undeterred"), + integration: INTEG, + template: TEMPLATE, + }) + .pipe(Effect.provide(Logger.layer([capture]))); + + // The authorization is unharmed: the caller still gets somewhere to go. + expect(started.status).toBe("redirect"); + expect(warnings.join("\n")).toContain("expired-session sweep failed"); + }), + ), + ); });