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. diff --git a/packages/core/sdk/src/oauth-service.ts b/packages/core/sdk/src/oauth-service.ts index 742cf65ad..cb0e82b46 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"; @@ -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, @@ -1395,6 +1415,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..e47b25c8c --- /dev/null +++ b/packages/core/sdk/src/oauth-session-cleanup.test.ts @@ -0,0 +1,235 @@ +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: {} }), + }), +}))(); + +interface SessionRow { + readonly pkce_verifier?: string | null; +} + +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* () { + 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, + ); + + 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* readSession(String(dying.state)); + expect(before?.pkce_verifier).toEqual(expect.any(String)); + + // 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: dyingCallback.code }), + ); + expect(JSON.stringify(failed)).toContain("restartRequired"); + + 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* readSession(String(bystander.state)); + expect(survivor?.pkce_verifier).toEqual(expect.any(String)); + }), + ), + ); +});