diff --git a/.changeset/defer-irreversible-cleanup.md b/.changeset/defer-irreversible-cleanup.md new file mode 100644 index 000000000..67ff23983 --- /dev/null +++ b/.changeset/defer-irreversible-cleanup.md @@ -0,0 +1,15 @@ +--- +"executor": minor +--- + +**Irreversible cleanup now waits for the transaction to commit, and plugins can do the same** + +`oauth.removeClient` deleted the client row and then deleted the client secret from the credential provider. The provider does not enlist in the caller's transaction and does not roll back with it, so an abort restored the client row while its secret stayed destroyed — a client that looks configured and can never authenticate again. The deletion now waits until the removal is durable and is discarded if the removal rolls back. With no transaction active it runs immediately, exactly as before. + +Deferring the deletion is not enough on its own. The secret is stored under a key derived from the app's `(owner, slug)` identity alone, so the key outlives the row it belonged to: whoever holds that identity when the deletion finally runs owns the key. A slug registered again before the removal committed would lose the new app's secret to the old app's queued deletion — the same unauthenticatable client, reached the other way round. The deferred deletion now re-checks that the app is still gone and stands down when it is not. A removal that matched no row also no longer queues a deletion at all: it removed nothing, so it has no claim on the key, which may well hold another subject's live secret. + +The same trap was reachable by plugins and they had no way out of it. `removeConnection` and `removeIntegration` run inside core's removal transaction — deliberately, so a plugin's own rows die atomically with the connection — which makes them exactly the wrong place to revoke a token at the provider's API, delete a remote object, or notify a third party. Nothing in the hooks' documentation said so, and `PluginCtx` exposed `transaction` but nothing to defer past it. + +`PluginCtx` gains `afterCommit`. It runs the effect once the outermost transaction commits, discards it if that transaction rolls back, and runs it immediately when no transaction is active. The lifecycle hooks now document that they run inside core's transaction and that outside-world work belongs in `afterCommit`. + +Sequencing work after your own `transaction(...)` call is not equivalent, and the documentation says so explicitly: `transaction` nests by pass-through, so inside an active transaction the inner call simply runs its effect and "afterwards" is still before any commit. diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index f5ad432e7..88595727b 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -5141,6 +5141,7 @@ export const createExecutor = execute(address, args, options), transaction: (effect: Effect.Effect) => transaction(effect), + afterCommit: (effect: Effect.Effect) => afterCommit(effect), }; if (plugin.toolPolicyProvider) { diff --git a/packages/core/sdk/src/oauth-remove-client.test.ts b/packages/core/sdk/src/oauth-remove-client.test.ts index 7ce3ffccc..1239895af 100644 --- a/packages/core/sdk/src/oauth-remove-client.test.ts +++ b/packages/core/sdk/src/oauth-remove-client.test.ts @@ -3,10 +3,21 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { describe, expect, it } from "@effect/vitest"; -import { Effect } from "effect"; - -import { OAuthClientSlug, ToolAddress } from "./ids"; +import { Effect, Exit } from "effect"; + +import { + AuthTemplateSlug, + ConnectionName, + IntegrationSlug, + OAuthClientSlug, + ProviderItemId, + ProviderKey, + ToolAddress, + ToolName, +} from "./ids"; +import { definePlugin } from "./plugin"; import { makeTestWorkspaceHarness, memoryCredentialsPlugin } from "./test-config"; +import { serveOAuthTestServer } from "./testing/oauth-test-server"; // removeClient permanently deletes an owner-scoped oauth_client row, keyed by // (owner, slug). The owner policy on `oauth_client` prevents removing another @@ -241,3 +252,262 @@ describe("oauth.removeClient", () => { ), ); }); + +// Removing a client deletes its secret from the provider, and that reaches a +// store which does not roll back with a transaction. `removeClient` opens none +// itself, but a caller can wrap it — and an abort would then restore the client +// row while its secret stayed destroyed, leaving a client that looks configured +// and can never authenticate again. +const txPlugin = (store: Map) => + definePlugin(() => ({ + id: "demo" 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)); + }), + }, + ], + extension: (ctx) => ({ + inTransaction: (effect: Effect.Effect) => ctx.transaction(effect), + }), + }))(); + +// The client secret is keyed by (owner, slug) ALONE — the key outlives the row +// it belonged to, which is what makes the deferred delete a claim that has to be +// re-checked rather than replayed. +const SECRET_ITEM = "oauth-client:user:acme-user:secret"; + +/** The registration used by the secret-lifecycle tests below, parameterised only + * by the secret so each one can prove which incarnation's secret survived. */ +const userClient = (clientSecret: string) => + ({ + owner: "user", + slug: USER_CLIENT, + authorizationUrl: "https://acme.test/authorize", + tokenUrl: "https://acme.test/token", + grant: "authorization_code", + clientId: "user-client-id", + clientSecret, + }) as const; + +describe("removing a client defers the secret deletion to the outermost commit", () => { + it.effect("a rolled-back removal leaves the client secret intact", () => + Effect.scoped( + Effect.gen(function* () { + const store = new Map(); + const { executor } = yield* makeTestWorkspaceHarness({ + plugins: [txPlugin(store)] as const, + }); + yield* executor.oauth.createClient(userClient("user-secret")); + expect(store.get(SECRET_ITEM)).toBe("user-secret"); + + // A caller wraps the removal in its own transaction, then fails. + const outcome = yield* Effect.exit( + executor.demo.inTransaction( + Effect.gen(function* () { + yield* executor.oauth.removeClient("user", USER_CLIENT); + return yield* Effect.fail("rollback" as const); + }), + ), + ); + expect(Exit.isFailure(outcome)).toBe(true); + + // The client came back... + const after = yield* executor.oauth.listClients(); + expect(after.map((client) => String(client.slug))).toContain(String(USER_CLIENT)); + // ...so its secret must still be there, or it can never authenticate again. + expect(store.get(SECRET_ITEM)).toBe("user-secret"); + }), + ), + ); + + // Deferring must not mean dropping. Without this, removing the deferred + // cleanup entirely still passes the rollback test above — the secret would + // simply leak on every successful removal, unobserved. + it.effect("a committed removal deletes the client secret", () => + Effect.scoped( + Effect.gen(function* () { + const store = new Map(); + const { executor } = yield* makeTestWorkspaceHarness({ + plugins: [txPlugin(store)] as const, + }); + yield* executor.oauth.createClient(userClient("user-secret")); + expect(store.get(SECRET_ITEM)).toBe("user-secret"); + + yield* executor.demo.inTransaction(executor.oauth.removeClient("user", USER_CLIENT)); + + expect(yield* executor.oauth.listClients()).toEqual([]); + expect(store.has(SECRET_ITEM)).toBe(false); + }), + ), + ); + + // With no transaction active `afterCommit` runs its effect inline, so the + // ordinary removal path stays synchronous rather than waiting for a commit + // that will never come. + it.effect("a removal outside any transaction deletes the secret immediately", () => + Effect.scoped( + Effect.gen(function* () { + const store = new Map(); + const { executor } = yield* makeTestWorkspaceHarness({ + plugins: [txPlugin(store)] as const, + }); + yield* executor.oauth.createClient(userClient("user-secret")); + expect(store.get(SECRET_ITEM)).toBe("user-secret"); + + yield* executor.oauth.removeClient("user", USER_CLIENT); + + expect(store.has(SECRET_ITEM)).toBe(false); + }), + ), + ); + + // A removal that matched no row removed nothing, so it has no claim on the + // key — and the key is not private to the caller. Here B's removal is scoped + // away by the owner policy and touches no row at all, while the secret it + // would have deleted is A's live one. + it.effect("a removal that matched no row leaves the key alone", () => + Effect.scoped( + Effect.gen(function* () { + const store = new Map(); + const plugins = [txPlugin(store)] as const; + const dataDir = mkdtempSync(join(tmpdir(), "oauth-remove-client-secret-")); + const tenant = "shared-tenant"; + + const a = yield* makeTestWorkspaceHarness({ + plugins, + tenant, + subject: "subject-a", + dataDir, + }); + yield* a.executor.oauth.createClient(userClient("a-secret")); + expect(store.get(SECRET_ITEM)).toBe("a-secret"); + + const b = yield* makeTestWorkspaceHarness({ + plugins, + tenant, + subject: "subject-b", + dataDir, + }); + yield* b.executor.oauth.removeClient("user", USER_CLIENT); + + // A's client survived the owner-scoped delete... + const clientsA = yield* a.executor.oauth.listClients(); + expect(clientsA.map((client) => String(client.slug))).toContain(String(USER_CLIENT)); + // ...and so must its secret, which B never owned. + expect(store.get(SECRET_ITEM)).toBe("a-secret"); + }), + ), + ); +}); + +// An integration to hang an OAuth connection off, so the recreated client's +// secret can be proven to still WORK — the test authorization server rejects a +// token request that presents the wrong secret (or none). +const INTEG = IntegrationSlug.make("acme"); +const TEMPLATE = AuthTemplateSlug.make("oauth"); + +const integrationPlugin = 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: {} }), + }), +}))(); + +// Deferring the delete to the commit is only half the answer. The provider key +// is derived from (owner, slug) and nothing else, so it is not owned by the row +// that was removed — it is owned by whichever row holds that identity when the +// deletion finally runs. A slug recreated inside the removal's own transaction +// commits together with the removal, and the queued delete then fires against +// the NEW app's secret: a client that looks configured and can never +// authenticate, the exact state the rollback path above prevents. +describe("removing a client does not delete a recreated client's secret", () => { + it.effect("a slug recreated before the commit keeps its own working secret", () => + Effect.scoped( + Effect.gen(function* () { + const server = yield* serveOAuthTestServer({ + scopes: ["read"], + clients: { "recreated-client": "second-secret" }, + }); + const store = new Map(); + const { executor } = yield* makeTestWorkspaceHarness({ + plugins: [txPlugin(store), integrationPlugin] as const, + redirectUri: null, + }); + yield* executor.acme.seed(); + + // The first incarnation, registered against a client id the server does + // not know: only the second one can ever mint a token. + yield* executor.oauth.createClient({ + owner: "user", + slug: USER_CLIENT, + authorizationUrl: server.authorizationEndpoint, + tokenUrl: server.tokenEndpoint, + grant: "client_credentials", + clientId: "first-client", + clientSecret: "first-secret", + }); + expect(store.get(SECRET_ITEM)).toBe("first-secret"); + + // The race, made deterministic: the removal and the re-registration of + // the same slug commit together, so the deferred delete runs against a + // live client. + yield* executor.demo.inTransaction( + Effect.gen(function* () { + yield* executor.oauth.removeClient("user", USER_CLIENT); + yield* executor.oauth.createClient({ + owner: "user", + slug: USER_CLIENT, + authorizationUrl: server.authorizationEndpoint, + tokenUrl: server.tokenEndpoint, + grant: "client_credentials", + clientId: "recreated-client", + clientSecret: "second-secret", + }); + }), + ); + + // The new incarnation is listed, and its secret survived... + const after = yield* executor.oauth.listClients(); + expect(after.map((client) => String(client.slug))).toContain(String(USER_CLIENT)); + expect(store.get(SECRET_ITEM)).toBe("second-secret"); + + // ...and still authenticates: the server refuses a token request that + // presents the wrong secret or none, so a connection can only be minted + // with the intact one. + const started = yield* executor.oauth.start({ + owner: "user", + client: USER_CLIENT, + clientOwner: "user", + name: ConnectionName.make("cc"), + integration: INTEG, + template: TEMPLATE, + }); + expect(started.status).toBe("connected"); + }), + ), + ); +}); diff --git a/packages/core/sdk/src/oauth-service.ts b/packages/core/sdk/src/oauth-service.ts index ad35a07aa..c9a638714 100644 --- a/packages/core/sdk/src/oauth-service.ts +++ b/packages/core/sdk/src/oauth-service.ts @@ -20,7 +20,7 @@ import { FetchHttpClient, type HttpClient } from "effect/unstable/http"; import { connectionIdentifier } from "./connection-name-identifier"; import type { Connection } from "./connection"; import type { IFumaClient, StorageFailure } from "./fuma-runtime"; -import { StorageError } from "./fuma-runtime"; +import { afterCommit, StorageError } from "./fuma-runtime"; import { AuthTemplateSlug, ConnectionName, @@ -905,6 +905,17 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { cause: undefined, }); } + // "Is there an app at (owner, slug) right now?" — asked twice, for two + // different reasons. Before the delete it says whether this call removes + // anything at all; after the commit it says whether the secret key still + // belongs to the app this call removed. + const findClientRow = deps.fuma.use("oauth_client.findFirst", (db) => + looseDb(db).findFirst("oauth_client", { + where: (b: any) => b.and(b("owner", "=", owner), b("slug", "=", String(slug))), + }), + ); + + const removedRow = yield* findClientRow; yield* deps.fuma .use("oauth_client.delete", (db) => looseDb(db).deleteMany("oauth_client", { @@ -912,12 +923,41 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { }), ) .pipe(Effect.asVoid); + // Nothing matched, so this call removed nothing and owns no secret. The + // idempotent no-op and the cross-subject miss both land here, and both + // used to queue a delete of a key they never had a claim on. + if (!removedRow) return; + // Best-effort: drop the secret from the provider so it isn't orphaned. + // + // Deferred to the outermost commit. This function opens no transaction of + // its own, but a caller can wrap it in one — and `provider.delete` reaches + // a store that does not roll back with it. An abort would then restore the + // client row while its secret stayed destroyed, leaving a client that + // looks configured and can never authenticate again. Orphaning a secret is + // recoverable; deleting one that is still referenced is not, so the + // deletion waits until the row's removal is durable. With no transaction + // active `afterCommit` runs it immediately, which is the behaviour this + // path already had. const provider = deps.defaultWritableProvider(); - if (provider?.delete) { - yield* provider - .delete(ProviderItemId.make(clientSecretItemId(owner, slug))) - .pipe(Effect.catch(() => Effect.void)); + const dropSecret = provider?.delete; + if (provider && dropSecret) { + yield* afterCommit( + Effect.gen(function* () { + // Deferral alone is not enough: the secret is keyed by (owner, slug) + // ALONE, so the key outlives the row it belonged to. If the same + // slug is registered again before this hook runs, the key now holds + // the NEW app's secret, and deleting it recreates exactly the state + // the deferral exists to prevent — a client that looks configured + // and can never authenticate. Re-check that the app is still gone + // and stand down when it is not. A re-check that FAILS is caught + // below and also stands down, which is the deliberate direction: + // an orphaned secret is recoverable, a destroyed live one is not. + const recreated = yield* findClientRow; + if (recreated) return; + yield* dropSecret.call(provider, ProviderItemId.make(clientSecretItemId(owner, slug))); + }).pipe(Effect.catch(() => Effect.void)), + ); } }); diff --git a/packages/core/sdk/src/plugin-after-commit.test.ts b/packages/core/sdk/src/plugin-after-commit.test.ts new file mode 100644 index 000000000..9449ae605 --- /dev/null +++ b/packages/core/sdk/src/plugin-after-commit.test.ts @@ -0,0 +1,116 @@ +import { describe, expect, it } from "@effect/vitest"; +import { Effect, Exit } from "effect"; + +import { + AuthTemplateSlug, + ConnectionName, + IntegrationSlug, + ProviderItemId, + ProviderKey, + ToolName, +} from "./ids"; +import { definePlugin } from "./plugin"; +import { makeTestExecutor } from "./test-config"; + +// A plugin's `removeConnection` runs INSIDE core's removal transaction, which is +// what makes its database work atomic with the row deletions. The same property +// makes anything reaching outside the database unsafe there: revoking a token at +// the provider's API cannot be rolled back with the transaction, so an abort +// leaves the connection restored and the token already dead. +// +// `ctx.afterCommit` is the way out, and these pin both directions of its +// contract — it runs when the removal is durable, and it is discarded when the +// removal is not. + +const INTEG = IntegrationSlug.make("vercel"); +const TEMPLATE = AuthTemplateSlug.make("apiKey"); + +const revokingPlugin = (revoked: string[]) => + definePlugin(() => { + const store = new Map(); + return { + id: "demo" as const, + 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)); + }), + }, + ], + storage: () => ({}), + resolveTools: () => + Effect.succeed({ tools: [{ name: ToolName.make("deploy"), description: "deploy" }] }), + invokeTool: ({ toolRow }) => Effect.succeed({ ran: toolRow.name }), + /** Stands in for "revoke the token at the provider's API" — the archetypal + * irreversible, outside-the-database cleanup. */ + removeConnection: ({ ctx, connection }) => + ctx.afterCommit( + Effect.sync(() => { + revoked.push(String(connection.name)); + }), + ), + extension: (ctx) => ({ + seed: () => + ctx.core.integrations.register({ slug: INTEG, description: "Vercel", config: {} }), + inTransaction: (effect: Effect.Effect) => ctx.transaction(effect), + }), + }; + })(); + +const setup = (revoked: string[]) => + makeTestExecutor({ plugins: [revokingPlugin(revoked)] as const }).pipe( + Effect.tap((executor) => executor.demo.seed()), + ); + +const REF = { + owner: "org", + integration: INTEG, + name: ConnectionName.make("main"), +} as const; + +describe("ctx.afterCommit inside a lifecycle hook", () => { + it.effect("runs the deferred cleanup once the removal is durable", () => + Effect.gen(function* () { + const revoked: string[] = []; + const executor = yield* setup(revoked); + yield* executor.connections.create({ ...REF, template: TEMPLATE, value: "secret-token" }); + + yield* executor.connections.remove(REF); + + // Deferring must not mean dropping: an ordinary removal still revokes. + expect(revoked).toEqual(["main"]); + }), + ); + + it.effect("discards the deferred cleanup when the removal rolls back", () => + Effect.gen(function* () { + const revoked: string[] = []; + const executor = yield* setup(revoked); + yield* executor.connections.create({ ...REF, template: TEMPLATE, value: "secret-token" }); + + const outcome = yield* Effect.exit( + executor.demo.inTransaction( + Effect.gen(function* () { + yield* executor.connections.remove(REF); + return yield* Effect.fail("rollback" as const); + }), + ), + ); + expect(Exit.isFailure(outcome)).toBe(true); + + // The connection survived, so revoking its token would have destroyed a + // live credential with nothing left to undo it. + const stillThere = yield* executor.connections.get(REF); + expect(String(stillThere?.name)).toBe("main"); + expect(revoked).toEqual([]); + }), + ); +}); diff --git a/packages/core/sdk/src/plugin.ts b/packages/core/sdk/src/plugin.ts index c30a3f43e..41c1112ed 100644 --- a/packages/core/sdk/src/plugin.ts +++ b/packages/core/sdk/src/plugin.ts @@ -286,6 +286,25 @@ export interface PluginCtx { /** Run `effect` inside a FumaDB transaction (atomic across plugin storage + * core integration/tool writes). */ readonly transaction: (effect: Effect.Effect) => Effect.Effect; + + /** Defer `effect` until the OUTERMOST transaction commits; discard it if that + * transaction rolls back. With none active it runs immediately. + * + * Use this for anything that reaches OUTSIDE the database — revoking a token + * at the provider's API, deleting a remote object, sending a webhook. Such + * work does not enlist in the transaction and cannot be rolled back with it, + * so performing it inline means a later abort leaves the database restored + * and the outside world already changed. That gap is not theoretical: the + * lifecycle hooks below run inside core's own transaction. + * + * Sequencing it after your `transaction(...)` call is NOT the same thing. + * `transaction` nests by pass-through, so inside an active transaction the + * inner call just runs its effect and "afterwards" is still before any + * commit. This is the only construct that waits for the real one. + * + * Best-effort by contract: failures and defects are swallowed, so a hook that + * cannot tidy up never fails the operation that triggered it. */ + readonly afterCommit: (effect: Effect.Effect) => Effect.Effect; } // --------------------------------------------------------------------------- @@ -707,13 +726,27 @@ export interface PluginSpec< readonly toolRows: readonly ToolInvocationRow[]; }) => Effect.Effect, unknown>; - /** Plugin-side cleanup when a connection is removed. */ + /** Plugin-side cleanup when a connection is removed. + * + * RUNS INSIDE core's removal transaction, so database work here is atomic + * with the row deletions — which is the point. The consequence is that + * anything reaching outside the database is NOT: revoking the token at the + * provider's API, deleting a remote object, notifying a third party. If the + * transaction later aborts, the connection is restored and that external + * action has already happened, with nothing left to undo it. + * + * Wrap such work in `ctx.afterCommit(...)`. It runs once the removal is + * durable and is discarded if the removal rolls back. */ readonly removeConnection?: ( input: ConnectionLifecycleInput, ) => Effect.Effect; /** Plugin-side cleanup when a removable integration is removed. Core still - * owns deleting the integration, connection, tool, and definition rows. */ + * owns deleting the integration, connection, tool, and definition rows. + * + * Runs inside core's removal transaction, with the same consequence as + * `removeConnection` above: defer any work that reaches outside the database + * through `ctx.afterCommit(...)`. */ readonly removeIntegration?: ( input: IntegrationLifecycleInput, ) => Effect.Effect;