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/4] 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/4] 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/4] 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/4] 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.