Sweep expired authorization sessions so abandoned PKCE verifiers do not persist

.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.

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",

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<string, string>();
+  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<SessionRow | null>,
+          );
+
+        // 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<SessionRow | null>,
+          );
+
+        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));
+      }),
+    ),
+  );
+});