From e8a5c3ee7fe7b2fc7fb5469a57ac050bb823a806 Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Sun, 20 Sep 2026 16:01:52 +0200 Subject: [PATCH 1/2] fix(sdk): delete the credentials a connection minted when it is removed Removing a connection dropped its rows but left the credential it had minted in the provider, unreferenced and invisible. Removal now deletes the items the connection minted, chosen by the row's credential_write marker, after the removal has committed. Referenced items, items another connection still points at, and non-writable providers are left alone; failures never fail the removal. The integrations.remove half of the original fix follows separately. --- .changeset/connection-remove-credential-gc.md | 5 + .../connection-remove-credential-gc.test.ts | 435 ++++++++++++++++++ packages/core/sdk/src/executor.ts | 106 ++++- 3 files changed, 545 insertions(+), 1 deletion(-) create mode 100644 .changeset/connection-remove-credential-gc.md create mode 100644 packages/core/sdk/src/connection-remove-credential-gc.test.ts diff --git a/.changeset/connection-remove-credential-gc.md b/.changeset/connection-remove-credential-gc.md new file mode 100644 index 0000000000..e29b705265 --- /dev/null +++ b/.changeset/connection-remove-credential-gc.md @@ -0,0 +1,5 @@ +--- +"@executor-js/sdk": patch +--- + +Removing a connection deleted its row and left the credential it had minted sitting in the provider, referenced by nothing and visible nowhere in the product, so a user who disconnected an account still had that account's tokens held on their behalf. Removal now also deletes the items that connection minted, including the long-lived OAuth refresh token. An item the connection merely referenced is left alone, as is a minted item another connection still points at, and the deletion runs only once the removal has committed. diff --git a/packages/core/sdk/src/connection-remove-credential-gc.test.ts b/packages/core/sdk/src/connection-remove-credential-gc.test.ts new file mode 100644 index 0000000000..f9f6de192c --- /dev/null +++ b/packages/core/sdk/src/connection-remove-credential-gc.test.ts @@ -0,0 +1,435 @@ +import { describe, expect, it } from "@effect/vitest"; +import { Effect, Exit } from "effect"; + +import { StorageError } from "./fuma-runtime"; +import { + AuthTemplateSlug, + ConnectionName, + IntegrationSlug, + OAuthClientSlug, + ProviderItemId, + ProviderKey, + ToolName, +} from "./ids"; +import { definePlugin } from "./plugin"; +import type { CredentialProvider } from "./provider"; +import { makeTestExecutor, makeTestWorkspaceHarness } from "./test-config"; +import { serveOAuthTestServer } from "./testing/oauth-test-server"; + +// Removing a connection has to remove the SECRET, not just the row that points +// at it — an item left behind in the store is still decryptable, which is the +// one thing a user deleting a credential is asking us to stop being true. +// +// The hard half is the opposite case. A connection can REFERENCE an item the +// user already had rather than minting one, and destroying that is +// unrecoverable. So these tests are written in pairs: every "it is gone" has a +// matching "it is still there", because a change that deleted everything would +// pass the first alone. + +const INTEG = IntegrationSlug.make("vercel"); +const TEMPLATE = AuthTemplateSlug.make("apiKey"); + +/** A provider whose store the test can inspect directly, so an assertion reads + * the actual item rather than a resolution a removed connection can no longer + * perform. */ +const inspectableProvider = ( + store: Map, + options: { + /** An ACCESSOR, not a literal, because one case needs a provider that stops + * being writable after it has already minted — the only state in which the + * writability gate is reachable. The executor wraps a registered provider + * with `Object.create`, so the accessor survives the wrap and stays live. */ + readonly isWritable?: () => boolean; + readonly deleteFails?: boolean; + } = {}, +): CredentialProvider => ({ + key: ProviderKey.make("memory"), + get writable() { + return options.isWritable?.() ?? true; + }, + get: (id) => Effect.sync(() => store.get(String(id)) ?? null), + set: (id, value) => + Effect.sync(() => { + store.set(String(id), value); + }), + has: (id) => Effect.sync(() => store.has(String(id))), + delete: (id) => + options.deleteFails === true + ? Effect.fail(new StorageError({ message: "credential store is offline", cause: undefined })) + : Effect.sync(() => { + store.delete(String(id)); + }), +}); + +const demoPlugin = (provider: CredentialProvider) => + definePlugin(() => ({ + id: "demo" as const, + credentialProviders: [provider], + storage: () => ({}), + resolveTools: () => + Effect.succeed({ tools: [{ name: ToolName.make("deploy"), description: "deploy" }] }), + invokeTool: ({ toolRow }) => Effect.succeed({ ran: toolRow.name }), + extension: (ctx) => ({ + seed: () => + ctx.core.integrations.register({ slug: INTEG, description: "Vercel", config: {} }), + /** The plugin-owned OUTER transaction the removal can find itself inside. */ + inTransaction: (effect: Effect.Effect) => ctx.transaction(effect), + }), + }))(); + +const setup = (provider: CredentialProvider) => + makeTestExecutor({ plugins: [demoPlugin(provider)] as const }).pipe( + Effect.tap((executor) => executor.demo.seed()), + ); + +/** The one id in the store, asserted to be the only one. Minted ids carry a + * per-attempt uuid, so a test cannot spell one out; it reads back what the + * mint actually wrote. */ +const onlyItemId = (store: ReadonlyMap): string => { + const keys = [...store.keys()]; + expect(keys).toHaveLength(1); + const [only = ""] = keys; + return only; +}; + +const OAUTH_INTEG = IntegrationSlug.make("oauthdemo"); +const OAUTH_TEMPLATE = AuthTemplateSlug.make("oauth"); + +const oauthIntegrationPlugin = definePlugin(() => ({ + id: "oauthdemo" as const, + storage: () => ({}), + resolveTools: () => + Effect.succeed({ tools: [{ name: ToolName.make("whoami"), description: "whoami" }] }), + describeAuthMethods: () => [ + { + id: "oauth", + label: "OAuth2", + kind: "oauth" as const, + template: String(OAUTH_TEMPLATE), + oauth: { scopes: [] }, + }, + ], + invokeTool: ({ credential }) => Effect.succeed({ token: credential.value }), + extension: (ctx) => ({ + seed: () => + ctx.core.integrations.register({ slug: OAUTH_INTEG, description: "OAuth demo", config: {} }), + }), +}))(); + +describe("removing a connection removes the credential it minted", () => { + // The premise the whole design rests on. A minted item id is NOT derivable + // from the row: it carries a uuid unique to the write attempt, so the + // connection's own columns cannot reproduce it. That is why ownership is read + // from the `credential_write` marker instead of by rebuilding a deterministic + // id and comparing — a rebuild would match nothing and delete nothing. + it.effect("mints an item id the row cannot reproduce", () => + Effect.gen(function* () { + const store = new Map(); + const executor = yield* setup(inspectableProvider(store)); + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make("main"), + integration: INTEG, + template: TEMPLATE, + value: "secret-token", + }); + + const mintedId = onlyItemId(store); + expect(mintedId).not.toBe("connection:org:vercel:main:token"); + expect(mintedId.startsWith("connection:org:vercel:main:")).toBe(true); + expect(mintedId.endsWith(":token")).toBe(true); + }), + ); + + it.effect("deletes the item a pasted connection minted", () => + Effect.gen(function* () { + const store = new Map(); + const executor = yield* setup(inspectableProvider(store)); + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make("main"), + integration: INTEG, + template: TEMPLATE, + value: "secret-token", + }); + const mintedId = onlyItemId(store); + expect(store.get(mintedId)).toBe("secret-token"); + + yield* executor.connections.remove({ + owner: "org", + integration: INTEG, + name: ConnectionName.make("main"), + }); + + expect(store.has(mintedId)).toBe(false); + // Nothing else was swept up on the way past. + expect([...store.keys()]).toEqual([]); + }), + ); + + it.effect("LEAVES an item the connection only referenced", () => + Effect.gen(function* () { + const store = new Map(); + const executor = yield* setup(inspectableProvider(store)); + // The user already had this, in their own store, under their own id. We + // never wrote it, and deleting it would destroy a credential that has + // nothing to do with this connection. The row records that by leaving + // `credential_write` null. + store.set("ext-item", "user-owned-secret"); + + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make("byo"), + integration: INTEG, + template: TEMPLATE, + from: { provider: ProviderKey.make("memory"), id: ProviderItemId.make("ext-item") }, + }); + yield* executor.connections.remove({ + owner: "org", + integration: INTEG, + name: ConnectionName.make("byo"), + }); + + expect(store.get("ext-item")).toBe("user-owned-secret"); + }), + ); + + // The writability gate, exercised the only way it is reachable: the item was + // minted while the provider was writable, and the provider stopped being + // writable before the removal. `writable: false` means we never write there, + // and by the same contract we never delete there either. + it.effect("LEAVES a minted item once its provider stops being writable", () => + Effect.gen(function* () { + const store = new Map(); + let writable = true; + const executor = yield* setup(inspectableProvider(store, { isWritable: () => writable })); + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make("main"), + integration: INTEG, + template: TEMPLATE, + value: "secret-token", + }); + const mintedId = onlyItemId(store); + + writable = false; + yield* executor.connections.remove({ + owner: "org", + integration: INTEG, + name: ConnectionName.make("main"), + }); + + expect(store.get(mintedId)).toBe("secret-token"); + }), + ); + + it.effect("LEAVES a minted item that another connection has aliased", () => + Effect.gen(function* () { + const store = new Map(); + const executor = yield* setup(inspectableProvider(store)); + // `first` mints its own item. + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make("first"), + integration: INTEG, + template: TEMPLATE, + value: "shared-token", + }); + const mintedId = onlyItemId(store); + expect(store.get(mintedId)).toBe("shared-token"); + + // `second` points AT that same item instead of minting its own. Nothing + // stops this: the reference path stores whatever id it is handed. + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make("second"), + integration: INTEG, + template: TEMPLATE, + from: { provider: ProviderKey.make("memory"), id: ProviderItemId.make(mintedId) }, + }); + + yield* executor.connections.remove({ + owner: "org", + integration: INTEG, + name: ConnectionName.make("first"), + }); + + // Removing the minting connection must not pull the credential out from + // under the one still using it — that would break a live connection. + expect(store.get(mintedId)).toBe("shared-token"); + }), + ); + + it.effect("removing one connection does not touch another's credential", () => + Effect.gen(function* () { + const store = new Map(); + const executor = yield* setup(inspectableProvider(store)); + for (const name of ["first", "second"]) { + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make(name), + integration: INTEG, + template: TEMPLATE, + value: `${name}-token`, + }); + } + const idOf = (value: string): string => { + const entry = [...store.entries()].find(([, stored]) => stored === value); + expect(entry).toBeDefined(); + return entry?.[0] ?? ""; + }; + const firstId = idOf("first-token"); + const secondId = idOf("second-token"); + + yield* executor.connections.remove({ + owner: "org", + integration: INTEG, + name: ConnectionName.make("first"), + }); + + expect(store.has(firstId)).toBe(false); + expect(store.get(secondId)).toBe("second-token"); + }), + ); + + // Best effort, and deliberately so. A provider that cannot delete must not + // resurrect a connection the user has already removed: a failure here leaves + // the orphan that existed before, which is recoverable, rather than failing a + // removal whose rows are already gone. + it.effect("a provider whose delete fails does not fail the removal", () => + Effect.gen(function* () { + const store = new Map(); + const executor = yield* setup(inspectableProvider(store, { deleteFails: true })); + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make("main"), + integration: INTEG, + template: TEMPLATE, + value: "secret-token", + }); + const mintedId = onlyItemId(store); + + const outcome = yield* Effect.exit( + executor.connections.remove({ + owner: "org", + integration: INTEG, + name: ConnectionName.make("main"), + }), + ); + + expect(Exit.isSuccess(outcome)).toBe(true); + // The row is gone even though the item could not be deleted. + const gone = yield* executor.connections.get({ + owner: "org", + integration: INTEG, + name: ConnectionName.make("main"), + }); + expect(gone).toBeNull(); + expect(store.get(mintedId)).toBe("secret-token"); + }), + ); + + it.effect("deletes BOTH the access and the refresh token of an OAuth connection", () => + Effect.scoped( + Effect.gen(function* () { + // The OAuth mint is the security-relevant half: it parks a long-lived + // REFRESH token, and leaving that behind is far worse than leaving an + // access token. Nothing else in the suite exercises an `oauth:` item id + // or a `refresh_item_id`, so without this the refresh half is unpinned. + const store = new Map(); + const server = yield* serveOAuthTestServer({}); + const { executor } = yield* makeTestWorkspaceHarness({ + plugins: [demoPlugin(inspectableProvider(store)), oauthIntegrationPlugin] as const, + }); + yield* executor.oauthdemo.seed(); + yield* executor.oauth.createClient({ + owner: "org", + slug: OAuthClientSlug.make("demo-app"), + authorizationUrl: server.authorizationEndpoint, + tokenUrl: server.tokenEndpoint, + grant: "authorization_code", + clientId: "test-client", + clientSecret: "test-secret", + }); + + const started = yield* executor.oauth.start({ + owner: "org", + client: OAuthClientSlug.make("demo-app"), + clientOwner: "org", + name: ConnectionName.make("main"), + integration: OAUTH_INTEG, + template: OAUTH_TEMPLATE, + }); + if (started.status !== "redirect") { + return yield* Effect.die("expected a redirect-status OAuth start"); + } + const callback = yield* server.completeAuthorizationCodeFlow({ + authorizationUrl: started.authorizationUrl, + }); + yield* executor.oauth.complete({ state: started.state, code: callback.code }); + + // Both halves are versioned per attempt, so they are read back by shape + // rather than spelled out. + const oauthIds = [...store.keys()].filter((id) => id.startsWith("oauth:")); + const refreshIds = oauthIds.filter((id) => id.endsWith(":refresh")); + const accessIds = oauthIds.filter((id) => !id.endsWith(":refresh")); + expect(refreshIds).toHaveLength(1); + expect(accessIds).toHaveLength(1); + const [accessId = ""] = accessIds; + const [refreshId = ""] = refreshIds; + + yield* executor.connections.remove({ + owner: "org", + integration: OAUTH_INTEG, + name: ConnectionName.make("main"), + }); + + expect(store.has(accessId)).toBe(false); + // The long-lived half. Leaving this behind is the worst outcome here. + expect(store.has(refreshId)).toBe(false); + }), + ), + ); +}); + +// The deletion reaches OUTSIDE the database, so it must not run inside the +// transaction that removes the rows. Nothing in a provider — a sealed store, a +// keychain, someone else's API — enlists in that transaction or rolls back with +// it. If an abort restores the connection row after its secret has already been +// destroyed, the result is a live connection pointing at a credential that no +// longer exists: worse than the orphan this whole feature removes, and unlike +// the orphan, unrepairable. +describe("the credential deletion runs after the transaction commits", () => { + it.effect("a rolled-back removal leaves the credential intact", () => + Effect.gen(function* () { + const store = new Map(); + const executor = yield* setup(inspectableProvider(store)); + const ref = { + owner: "org", + integration: INTEG, + name: ConnectionName.make("main"), + } as const; + yield* executor.connections.create({ ...ref, template: TEMPLATE, value: "secret-token" }); + const mintedId = onlyItemId(store); + + // A caller wraps the removal in its own transaction and then fails, so + // the row deletions roll back. + 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 came back... + const stillThere = yield* executor.connections.get(ref); + expect(String(stillThere?.name)).toBe("main"); + // ...so its credential MUST still be there. A restored row pointing at a + // destroyed secret is the one outcome that cannot be repaired. + expect(store.get(mintedId)).toBe("secret-token"); + }), + ); +}); diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index fdbc9b7671..9899d0b2d2 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -4823,6 +4823,108 @@ export const createExecutor = => + Effect.gen(function* () { + // Removing the rows only dropped the POINTER: the secret stayed in the + // provider and stayed decryptable, which is precisely what a user + // removing a connection is asking us to stop being true. + // + // Only ids this connection MINTED. A connection can instead REFERENCE + // an item the user already had (the `from` origin on the create path), + // and the provider contract is explicit that such a removal "only drops + // our routing, leaving the item intact" — deleting one would destroy a + // credential we never wrote and cannot restore. + // + // `credential_write` is what tells the two apart, and it is the only + // thing that can. Provider item ids are opaque after creation by + // design — ownership is "determined exclusively from persisted + // metadata" (`credential-item-reference.ts`) — and rebuilding a + // deterministic id to compare would not work anyway: a minted id embeds + // a per-attempt uuid (`credentialAttemptItemId`) that no other column + // records. + // + // The marker is per ROW rather than per item, which is sufficient + // because a row cannot hold both kinds: `connectionsCreate` rejects a + // connection mixing pasted and external inputs, its external branch + // leaves `credential_write` null, and the OAuth mint sets it for every + // item it writes. Null therefore means every id on this row is external + // or v1-legacy, opaque to core, and left alone. A v1-migrated + // `secret_` item is executor-owned but unmarked, so its value is + // still left behind; closing that needs a schema change, not a looser + // rule here. + if (parseCredentialWriteAttempt(row.credential_write) === null) return; + // `writable` is checked as well as the marker, never instead of it. + // This is only reachable when a provider stops being writable after the + // item was minted, where honouring the contract's "we never write here" + // is the safer reading. + const provider = credentialProviders.get(String(row.provider)); + if (provider?.writable !== true || !provider.delete) return; + const deleteItem = provider.delete; + const minted = [ + ...new Set([ + ...Object.values(connectionItemIds(row)), + ...(row.refresh_item_id === null ? [] : [String(row.refresh_item_id)]), + ]), + ]; + if (minted.length === 0) return; + // Nothing stops a SECOND connection pointing at this one's minted item + // through the `from` origin — the reference path stores whatever id it + // is handed. Deleting the item would then pull the credential out from + // under a connection that is still live and still using it. This row is + // already gone by the time this runs, so anything still naming the id + // is by definition somebody else, and the item stays. + // + // An item id only means anything inside ONE provider's namespace, so a + // connection on a different provider holding the same string is not an + // alias. Counting it as one would leave this connection's secret + // behind, which is the orphan this delete exists to remove. + // + // This read is owner-scoped by the table's own visibility policy: it + // sees the org partition plus this caller's own rows and NOT another + // subject's. An alias held by a different subject is therefore + // invisible here and its credential is still deleted. That limit is + // accepted deliberately — reading around a tenant-isolation boundary to + // widen a DELETE would be a worse defect than the narrow one it closes. + const others = yield* core.findMany("connection", { + where: (b: AnyCb) => b("provider", "=", String(row.provider)), + }); + const stillReferenced = new Set( + others.flatMap((other) => [ + ...Object.values(connectionItemIds(other)), + ...(other.refresh_item_id === null ? [] : [String(other.refresh_item_id)]), + ]), + ); + yield* Effect.forEach( + minted.filter((id) => !stillReferenced.has(id)), + (id) => deleteItem(ProviderItemId.make(id)).pipe(Effect.ignore), + { discard: true }, + ); + }).pipe(Effect.ignoreCause({ log: false })); + const connectionsRemove = ( ref: ConnectionRef, ): Effect.Effect => @@ -4868,8 +4970,10 @@ export const createExecutor = b("integration", "=", String(slug)); + // Read the doomed connections BEFORE the cascade destroys them. A + // minted item id carries a per-attempt uuid recorded nowhere but the + // row (`credentialAttemptItemId`), so the moment the row is gone the + // id of the secret it minted is unreconstructible and that secret is + // stranded in the provider for good. + // + // Through `cascadeCore`, the handle doing the DELETE below, and this + // adds NO reach. `ownedExecutorTable` feeds one `ownerVisibility` + // condition to both `onRead` and `onDelete` (`core-schema.ts`), so + // the same context and the same predicate return exactly the rows the + // next statement destroys and never one more; reading rows in order + // to destroy them cannot expose anything the destruction does not + // already reach. The bound `core` handle is the unsafe option here, + // not the safe one: it sees only the remover's own rows, so every + // other member's credential would be silently left behind — which is + // the bulk half of the leak this closes. + const doomed = yield* cascadeCore.findMany("connection", { where }); yield* cascadeCore.deleteMany("tool", { where }); yield* cascadeCore.deleteMany("definition", { where }); yield* cascadeCore.deleteMany("connection", { where }); + // Then delete what those connections minted, after the OUTERMOST + // commit. Same reasoning as `connections.remove`, whose + // `deleteMintedCredentials` this reuses unchanged: a provider does + // not enlist in this transaction, so deleting inside it would let a + // rollback restore every row with its secret already destroyed. + // + // One residual limit, inherited rather than introduced: the alias + // hold-back inside `deleteMintedCredentials` reads through the bound + // handle, so an item that a SURVIVING connection of ANOTHER subject + // references is invisible to it and is deleted with the rest. Closing + // that would mean reading other subjects' credential ids OUTSIDE the + // set being destroyed — a read wider than the delete, and a worse + // defect than the narrow one it would fix. + yield* afterCommit(Effect.forEach(doomed, deleteMintedCredentials, { discard: true })); return existing.plugin_id; }), ).pipe( diff --git a/packages/core/sdk/src/integration-remove-credential-gc.test.ts b/packages/core/sdk/src/integration-remove-credential-gc.test.ts new file mode 100644 index 0000000000..e001231604 --- /dev/null +++ b/packages/core/sdk/src/integration-remove-credential-gc.test.ts @@ -0,0 +1,382 @@ +import { describe, expect, it } from "@effect/vitest"; +import { Effect, Exit } from "effect"; +import { withQueryContext } from "@executor-js/fumadb/query"; + +import { createExecutor, type Executor } from "./executor"; +import { StorageError } from "./fuma-runtime"; +import { + AuthTemplateSlug, + ConnectionName, + IntegrationSlug, + OAuthClientSlug, + ProviderItemId, + ProviderKey, + Subject, + ToolName, +} from "./ids"; +import { definePlugin } from "./plugin"; +import type { CredentialProvider } from "./provider"; +import { makeTestConfig, makeTestWorkspaceHarness } from "./test-config"; +import { serveOAuthTestServer } from "./testing/oauth-test-server"; + +// Removing an INTEGRATION strands the same secrets `connections.remove` used to +// strand, only in bulk and across people: the cascade drops every subject's +// connection rows under the slug, and the credentials those rows minted stayed +// behind in the provider, pointed at by nothing and reachable from nowhere in +// the product. +// +// The fixtures build TWO executors over ONE database bound to two subjects, the +// way `integration-removal-cascade.test.ts` does, over a store the test can +// inspect directly, the way `connection-remove-credential-gc.test.ts` does. The +// reads that matter are of the store itself: a removed connection can no longer +// resolve its own credential, so asking the executor would prove nothing. +// +// Every "it is gone" is paired with an "it is still there". A change that +// simply emptied the store would pass the first half of this file alone. + +const INTEG = IntegrationSlug.make("datadog"); +const KEPT = IntegrationSlug.make("linear"); +const TEMPLATE = AuthTemplateSlug.make("apiKey"); +const ALICE = "user_alice"; +const BOB = "user_bob"; + +const inspectableProvider = ( + store: Map, + options: { + readonly deleteFails?: boolean; + /** Every id the removal ASKED to delete, recorded before the outcome, so a + * best-effort test can tell a swallowed failure from a delete that was + * never attempted at all. */ + readonly deleteAttempts?: string[]; + } = {}, +): CredentialProvider => ({ + key: ProviderKey.make("memory"), + writable: true, + get: (id) => Effect.sync(() => store.get(String(id)) ?? null), + set: (id, value) => + Effect.sync(() => { + store.set(String(id), value); + }), + has: (id) => Effect.sync(() => store.has(String(id))), + delete: (id) => + Effect.suspend(() => { + options.deleteAttempts?.push(String(id)); + return options.deleteFails === true + ? Effect.fail( + new StorageError({ message: "credential store is offline", cause: undefined }), + ) + : Effect.sync(() => { + store.delete(String(id)); + }); + }), +}); + +const demoPlugin = (provider: CredentialProvider) => + definePlugin(() => ({ + id: "demo" as const, + credentialProviders: [provider], + storage: () => ({}), + resolveTools: () => + Effect.succeed({ tools: [{ name: ToolName.make("query"), description: "query" }] }), + invokeTool: ({ toolRow }) => Effect.succeed({ ran: toolRow.name }), + extension: (ctx) => ({ + seed: (slug: IntegrationSlug) => + ctx.core.integrations.register({ slug, description: String(slug), config: {} }), + /** The plugin-owned OUTER transaction the removal can find itself inside. */ + inTransaction: (effect: Effect.Effect) => ctx.transaction(effect), + }), + }))(); + +/** Two executors over ONE database: `alice` is the admin who seeds and removes, + * `bob` is a member whose rows the removal reaches but whose rows the remover's + * own bound handle cannot see. */ +const setup = (provider: CredentialProvider) => + Effect.gen(function* () { + const config = makeTestConfig({ plugins: [demoPlugin(provider)] as const, subject: ALICE }); + const alice = yield* createExecutor(config); + const bob = yield* createExecutor({ + ...config, + subject: Subject.make(BOB), + db: withQueryContext(config.testDb.db, { tenant: String(config.tenant), subject: BOB }), + }); + yield* Effect.addFinalizer(() => + alice.close().pipe(Effect.andThen(bob.close()), Effect.ignore), + ); + yield* alice.demo.seed(INTEG); + yield* alice.demo.seed(KEPT); + return { alice, bob }; + }); + +const connectPersonal = ( + executor: Executor, + integration: IntegrationSlug, + name: string, + value: string, +) => + executor.connections.create({ + owner: "user", + name: ConnectionName.make(name), + integration, + template: TEMPLATE, + value, + }); + +const referencePersonal = ( + executor: Executor, + integration: IntegrationSlug, + name: string, + itemId: string, +) => + executor.connections.create({ + owner: "user", + name: ConnectionName.make(name), + integration, + template: TEMPLATE, + from: { provider: ProviderKey.make("memory"), id: ProviderItemId.make(itemId) }, + }); + +/** Minted ids carry a per-attempt uuid, so a test cannot spell one out; it finds + * the id by the value the mint actually wrote under it. */ +const idOf = (store: ReadonlyMap, value: string): string => { + const entry = [...store.entries()].find(([, stored]) => stored === value); + expect(entry).toBeDefined(); + return entry?.[0] ?? ""; +}; + +const OAUTH_INTEG = IntegrationSlug.make("oauthdemo"); +const OAUTH_TEMPLATE = AuthTemplateSlug.make("oauth"); + +const oauthIntegrationPlugin = definePlugin(() => ({ + id: "oauthdemo" as const, + storage: () => ({}), + resolveTools: () => + Effect.succeed({ tools: [{ name: ToolName.make("whoami"), description: "whoami" }] }), + describeAuthMethods: () => [ + { + id: "oauth", + label: "OAuth2", + kind: "oauth" as const, + template: String(OAUTH_TEMPLATE), + oauth: { scopes: [] }, + }, + ], + invokeTool: ({ credential }) => Effect.succeed({ token: credential.value }), + extension: (ctx) => ({ + seed: () => + ctx.core.integrations.register({ slug: OAUTH_INTEG, description: "OAuth demo", config: {} }), + }), +}))(); + +describe("removing an integration removes the credentials its connections minted", () => { + it.effect("deletes the minted item of EVERY subject's connection under the slug", () => + Effect.gen(function* () { + const store = new Map(); + const { alice, bob } = yield* setup(inspectableProvider(store)); + yield* connectPersonal(alice, INTEG, "aliceDd", "alice-token"); + yield* connectPersonal(bob, INTEG, "bobDd", "bob-token"); + yield* connectPersonal(bob, KEPT, "bobLinear", "bob-kept-token"); + const aliceId = idOf(store, "alice-token"); + const bobId = idOf(store, "bob-token"); + const keptId = idOf(store, "bob-kept-token"); + + yield* alice.integrations.remove(INTEG); + + expect(store.has(aliceId)).toBe(false); + // The half that only this change closes. The removal destroys Bob's row + // through the tenant-reach cascade, but the remover's own bound handle + // cannot even see that row — so the doomed set has to be read through the + // same widened handle that deletes it, or Bob's secret is left behind. + expect(store.has(bobId)).toBe(false); + // And the sweep stops at the slug: an integration that was not removed + // keeps its credential. + expect(store.get(keptId)).toBe("bob-kept-token"); + expect([...store.keys()]).toEqual([keptId]); + }).pipe(Effect.scoped), + ); + + it.effect("LEAVES an item a doomed connection only referenced", () => + Effect.gen(function* () { + const store = new Map(); + const { alice, bob } = yield* setup(inspectableProvider(store)); + // Bob already had this, in his own store, under his own id. We never + // wrote it, so deleting it would destroy a credential that has nothing to + // do with the integration being removed. The row records that by leaving + // `credential_write` null. + store.set("bob-owned-item", "bob-owned-secret"); + yield* referencePersonal(bob, INTEG, "byo", "bob-owned-item"); + // A minted item under the SAME slug, so the sweep is known to have run + // and to have skipped the referenced one rather than skipped everything. + yield* connectPersonal(alice, INTEG, "aliceDd", "alice-token"); + const aliceId = idOf(store, "alice-token"); + + yield* alice.integrations.remove(INTEG); + + expect(store.has(aliceId)).toBe(false); + expect(store.get("bob-owned-item")).toBe("bob-owned-secret"); + }).pipe(Effect.scoped), + ); + + it.effect("LEAVES a minted item a SURVIVING connection still points at", () => + Effect.gen(function* () { + const store = new Map(); + const { alice } = yield* setup(inspectableProvider(store)); + yield* connectPersonal(alice, INTEG, "doomed", "shared-token"); + const sharedId = idOf(store, "shared-token"); + // A connection under an integration that is NOT being removed points at + // that same item instead of minting its own. Nothing stops this: the + // reference path stores whatever id it is handed. + yield* referencePersonal(alice, KEPT, "survivor", sharedId); + // A second doomed connection with an item nobody else points at, so the + // survival below is a hold-back and not an inert sweep. + yield* connectPersonal(alice, INTEG, "alsoDoomed", "lonely-token"); + const lonelyId = idOf(store, "lonely-token"); + + yield* alice.integrations.remove(INTEG); + + expect(store.has(lonelyId)).toBe(false); + // Sweeping the aliased item away would pull the credential out from under + // a connection that is still live and still using it. + expect(store.get(sharedId)).toBe("shared-token"); + }).pipe(Effect.scoped), + ); + + // Best effort, and deliberately so. A provider that cannot delete must not + // resurrect an integration the user has already removed: the failure leaves + // the orphan that existed before, which is recoverable, where failing the + // removal would leave rows the catalog no longer has an entry for. + it.effect("a provider whose delete fails does not fail the removal", () => + Effect.gen(function* () { + const store = new Map(); + const deleteAttempts: string[] = []; + const { alice, bob } = yield* setup( + inspectableProvider(store, { deleteFails: true, deleteAttempts }), + ); + yield* connectPersonal(bob, INTEG, "bobDd", "bob-token"); + const bobId = idOf(store, "bob-token"); + + const outcome = yield* Effect.exit(alice.integrations.remove(INTEG)); + + expect(Exit.isSuccess(outcome)).toBe(true); + // The delete was ATTEMPTED and its failure swallowed. Without this the + // test would pass just as happily against a removal that never tried. + expect(deleteAttempts).toEqual([bobId]); + // The rows went even though the item could not be deleted. + const bobConnections = yield* bob.connections.list(); + expect(bobConnections.map((connection) => String(connection.integration))).toEqual([]); + expect(store.get(bobId)).toBe("bob-token"); + }).pipe(Effect.scoped), + ); + + it.effect("deletes BOTH the access and the refresh token of an OAuth connection", () => + Effect.scoped( + Effect.gen(function* () { + // The OAuth mint is the security-relevant half: it parks a long-lived + // REFRESH token, and an integration removal sweeping up every member at + // once is exactly where leaving those behind adds up. + const store = new Map(); + const server = yield* serveOAuthTestServer({}); + const { executor } = yield* makeTestWorkspaceHarness({ + plugins: [demoPlugin(inspectableProvider(store)), oauthIntegrationPlugin] as const, + }); + yield* executor.oauthdemo.seed(); + yield* executor.oauth.createClient({ + owner: "org", + slug: OAuthClientSlug.make("demo-app"), + authorizationUrl: server.authorizationEndpoint, + tokenUrl: server.tokenEndpoint, + grant: "authorization_code", + clientId: "test-client", + clientSecret: "test-secret", + }); + // Everything in the store at this point belongs to the OAuth APP, not + // to any connection. Captured before the flow so the assertion below + // cannot accidentally be about nothing. + const clientItemIds = [...store.keys()]; + expect(clientItemIds.length).toBeGreaterThan(0); + + const started = yield* executor.oauth.start({ + owner: "org", + client: OAuthClientSlug.make("demo-app"), + clientOwner: "org", + name: ConnectionName.make("main"), + integration: OAUTH_INTEG, + template: OAUTH_TEMPLATE, + }); + if (started.status !== "redirect") { + return yield* Effect.die("expected a redirect-status OAuth start"); + } + const callback = yield* server.completeAuthorizationCodeFlow({ + authorizationUrl: started.authorizationUrl, + }); + yield* executor.oauth.complete({ state: started.state, code: callback.code }); + + // Both halves are versioned per attempt, so they are read back by shape + // rather than spelled out. + const oauthIds = [...store.keys()].filter((id) => id.startsWith("oauth:")); + const refreshIds = oauthIds.filter((id) => id.endsWith(":refresh")); + const accessIds = oauthIds.filter((id) => !id.endsWith(":refresh")); + expect(refreshIds).toHaveLength(1); + expect(accessIds).toHaveLength(1); + const [accessId = ""] = accessIds; + const [refreshId = ""] = refreshIds; + + yield* executor.integrations.remove(OAUTH_INTEG); + + expect(store.has(accessId)).toBe(false); + // The long-lived half. Leaving this behind is the worst outcome here. + expect(store.has(refreshId)).toBe(false); + // The app's own client secret is the OAuth client's, not the + // connection's, and it outlives the integration it was registered + // against. Nothing here is entitled to delete it. + expect(clientItemIds.filter((id) => store.has(id))).toEqual(clientItemIds); + }), + ), + ); +}); + +// The deletion reaches OUTSIDE the database, so it must not run inside the +// transaction that removes the rows. Nothing in a provider — a sealed store, a +// keychain, someone else's API — enlists in that transaction or rolls back with +// it. If an abort restores the rows after their secrets have already been +// destroyed, the result is a whole integration's worth of live connections +// pointing at credentials that no longer exist. +describe("the credential deletion runs after the transaction commits", () => { + it.effect("a rolled-back removal leaves every subject's credential intact", () => + Effect.gen(function* () { + const store = new Map(); + const { alice, bob } = yield* setup(inspectableProvider(store)); + yield* connectPersonal(alice, INTEG, "aliceDd", "alice-token"); + yield* connectPersonal(bob, INTEG, "bobDd", "bob-token"); + const aliceId = idOf(store, "alice-token"); + const bobId = idOf(store, "bob-token"); + + // A caller wraps the removal in its own transaction and then fails, so + // the catalog row and the whole cascade roll back. + const outcome = yield* Effect.exit( + alice.demo.inTransaction( + Effect.gen(function* () { + yield* alice.integrations.remove(INTEG); + return yield* Effect.fail("rollback" as const); + }), + ), + ); + expect(Exit.isFailure(outcome)).toBe(true); + + // The connections came back... + const bobConnections = yield* bob.connections.list(); + expect(bobConnections.map((connection) => String(connection.integration))).toEqual([ + String(INTEG), + ]); + // ...so their credentials MUST still be there. A restored row pointing at + // a destroyed secret is the one outcome that cannot be repaired. + expect(store.get(aliceId)).toBe("alice-token"); + expect(store.get(bobId)).toBe("bob-token"); + + // And the rollback is what spared them, not an inert fixture: the same + // removal, committed this time, takes both. + yield* alice.integrations.remove(INTEG); + expect(store.has(aliceId)).toBe(false); + expect(store.has(bobId)).toBe(false); + }).pipe(Effect.scoped), + ); +});