From c3dc55758f24b8b627e0dc8a8d3f5f0d49d2d2d0 Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Wed, 12 Aug 2026 17:09:47 +0200 Subject: [PATCH 01/10] Delete a connection's minted credential when the connection is removed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removing a connection deleted the tool, definition and connection rows and then returned. The secret itself stayed in the provider store, still decryptable — so a user who deleted a credential had not actually deleted it. Only ids the connection MINTED are removed. A connection can instead reference an item the user already had, and the provider contract is explicit that such a removal drops our routing and leaves the item intact; deleting one would destroy a credential we never created and cannot restore. The row carries no flag saying which is which, but it does not need one: minted ids are deterministic, so rebuilding them from the row and keeping only exact matches recovers the distinction with no schema change. It also leaves the OAuth app's shared client secret alone, which every connection minted through that app still needs. --- .../connection-remove-credential-gc.test.ts | 168 ++++++++++++++++++ packages/core/sdk/src/executor.ts | 53 ++++++ 2 files changed, 221 insertions(+) create mode 100644 packages/core/sdk/src/connection-remove-credential-gc.test.ts 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 000000000..d8df406d1 --- /dev/null +++ b/packages/core/sdk/src/connection-remove-credential-gc.test.ts @@ -0,0 +1,168 @@ +import { describe, expect, it } from "@effect/vitest"; +import { Effect } from "effect"; + +import { + AuthTemplateSlug, + ConnectionName, + IntegrationSlug, + ProviderItemId, + ProviderKey, + ToolName, +} from "./ids"; +import { definePlugin } from "./plugin"; +import type { CredentialProvider } from "./provider"; +import { makeTestExecutor } from "./test-config"; + +// 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 that a deleted connection can no + * longer perform. */ +const inspectableProvider = (store: Map, writable: boolean): CredentialProvider => ({ + key: ProviderKey.make("memory"), + writable, + get: (id) => Effect.sync(() => store.get(String(id)) ?? null), + set: (id, value) => + Effect.sync(() => { + store.set(String(id), value); + }), + delete: (id) => + Effect.sync(() => { + store.delete(String(id)); + }), + has: (id) => Effect.sync(() => store.has(String(id))), +}); + +const demoPlugin = (store: Map, writable = true) => + definePlugin(() => ({ + id: "demo" as const, + credentialProviders: [inspectableProvider(store, writable)], + 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: {} }), + }), + }))(); + +const setup = (store: Map, writable = true) => + makeTestExecutor({ plugins: [demoPlugin(store, writable)] as const }).pipe( + Effect.tap((executor) => executor.demo.seed()), + ); + +describe("removing a connection removes the credential it minted", () => { + it.effect("deletes a pasted value from the provider", () => + Effect.gen(function* () { + const store = new Map(); + const executor = yield* setup(store); + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make("main"), + integration: INTEG, + template: TEMPLATE, + value: "secret-token", + }); + // The id this connection mints is deterministic, and the value is really there. + const mintedId = "connection:org:vercel:main:token"; + 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(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. + 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"); + }), + ); + + it.effect("leaves everything alone when the provider is not writable", () => + Effect.gen(function* () { + const store = new Map(); + const executor = yield* setup(store, false); + 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"), + }); + + // `writable: false` means we never write there, and by the same contract + // we never delete there either. + expect(store.get("ext-item")).toBe("user-owned-secret"); + }), + ); + + it.effect("removing one connection does not touch another's credential", () => + Effect.gen(function* () { + const store = new Map(); + const executor = yield* setup(store); + for (const name of ["first", "second"]) { + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make(name), + integration: INTEG, + template: TEMPLATE, + value: `${name}-token`, + }); + } + + yield* executor.connections.remove({ + owner: "org", + integration: INTEG, + name: ConnectionName.make("first"), + }); + + expect(store.has("connection:org:vercel:first:token")).toBe(false); + expect(store.get("connection:org:vercel:second:token")).toBe("second-token"); + }), + ); +}); diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index 56cb2e299..3ffd57b64 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -864,6 +864,33 @@ const normalizeConnectionInputs = ( /** Decode a connection row's `item_ids` JSON map (`variable → provider item id`). * Tolerates the historically-single shape by returning `{}` for anything that * isn't an object. */ +/** The provider items a connection MINTED, as opposed to ones it merely points + * at. + * + * Both kinds sit in the same `item_ids` map, and the row records no flag saying + * which is which — but it does not need to, because the minted ids are + * deterministic. `connectionsCreate` writes + * `connection::::` for a pasted value and + * the OAuth mint writes `oauth:::` plus its + * `:refresh` sibling, while a referenced item keeps whatever id the user's own + * store gave it. So rebuilding those ids from the row and keeping only exact + * matches recovers the distinction with no schema change and no guesswork: an + * id we did not write cannot equal one we would have. */ +const mintedItemIds = (row: ConnectionRow): readonly string[] => { + const owner = String(row.owner); + const integration = String(row.integration); + const name = String(row.name); + const oauthItemId = `oauth:${owner}:${integration}:${name}`; + const itemIds = connectionItemIds(row); + const mintable = new Set([oauthItemId, `${oauthItemId}:refresh`]); + for (const variable of Object.keys(itemIds)) { + mintable.add(`connection:${owner}:${integration}:${name}:${variable}`); + } + const stored = Object.values(itemIds); + if (row.refresh_item_id) stored.push(String(row.refresh_item_id)); + return [...new Set(stored.filter((id) => mintable.has(id)))]; +}; + const connectionItemIds = (row: ConnectionRow): Record => { const decoded = decodeJsonColumn(row.item_ids); if (decoded == null || typeof decoded !== "object") return {}; @@ -3151,6 +3178,32 @@ export const createExecutor = Date: Wed, 12 Aug 2026 17:24:15 +0200 Subject: [PATCH 02/10] Record what the credential deletion deliberately does not cover --- .../src/connection-remove-credential-gc.test.ts | 5 ++++- packages/core/sdk/src/executor.ts | 15 +++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/packages/core/sdk/src/connection-remove-credential-gc.test.ts b/packages/core/sdk/src/connection-remove-credential-gc.test.ts index d8df406d1..824a9f2e1 100644 --- a/packages/core/sdk/src/connection-remove-credential-gc.test.ts +++ b/packages/core/sdk/src/connection-remove-credential-gc.test.ts @@ -28,7 +28,10 @@ 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 that a deleted connection can no * longer perform. */ -const inspectableProvider = (store: Map, writable: boolean): CredentialProvider => ({ +const inspectableProvider = ( + store: Map, + writable: boolean, +): CredentialProvider => ({ key: ProviderKey.make("memory"), writable, get: (id) => Effect.sync(() => store.get(String(id)) ?? null), diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index 3ffd57b64..6c33af90f 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -3198,6 +3198,21 @@ export const createExecutor = ` id, which is + // executor-owned but not derivable from v2 state, so it cannot be + // matched and its item is still left behind. Closing that needs a + // schema change, not a better rule here. + // - A second connection can point at this one's minted id through the + // `from` origin, in which case deleting it breaks that connection. + // Detecting it needs a reference scan across the partition. + // + // `writable` is checked as well as the id, never instead of it: a + // picked item can live in a writable store, so writability alone would + // destroy user data. It 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) { for (const id of mintedItemIds(row)) { From 904d30fe56560fdb870ff3546290239d213a7dc0 Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Wed, 12 Aug 2026 18:50:39 +0200 Subject: [PATCH 03/10] Keep a minted credential that another connection still points at MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing stops a second connection referencing this one's minted item through the `from` origin — the reference path stores whatever id it is handed. Deleting the item on removal then pulled the credential out from under a connection that was still live and still using it, which is the same unrecoverable loss the minted-id check exists to prevent, arriving by a different route. The connection row is already deleted at that point, so anything still holding the id is by definition somebody else, and the item stays. This was recorded as a known gap when the deletion landed; the test that now covers it failed before this change. --- .../connection-remove-credential-gc.test.ts | 37 +++++++++++++++++++ packages/core/sdk/src/executor.ts | 25 ++++++++++++- 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/packages/core/sdk/src/connection-remove-credential-gc.test.ts b/packages/core/sdk/src/connection-remove-credential-gc.test.ts index 824a9f2e1..6a9298971 100644 --- a/packages/core/sdk/src/connection-remove-credential-gc.test.ts +++ b/packages/core/sdk/src/connection-remove-credential-gc.test.ts @@ -144,6 +144,43 @@ describe("removing a connection removes the credential it minted", () => { }), ); + it.effect("LEAVES a minted item that another connection has aliased", () => + Effect.gen(function* () { + const store = new Map(); + const executor = yield* setup(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 = "connection:org:vercel:first:token"; + 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"), + }); + + // Deleting 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(); diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index 6c33af90f..6d4947345 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -3215,7 +3215,30 @@ export const createExecutor = () + : yield* core.findMany("connection", { where: () => true }).pipe( + Effect.map( + (rows) => + new Set( + rows.flatMap((other) => [ + ...Object.values(connectionItemIds(other)), + ...(other.refresh_item_id ? [String(other.refresh_item_id)] : []), + ]), + ), + ), + ); + for (const id of minted) { + if (stillReferenced.has(id)) continue; yield* provider.delete(ProviderItemId.make(id)).pipe(Effect.ignore); } } From 8679d1a0d6f845586fced05026f83dad9f347cdf Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Wed, 12 Aug 2026 19:02:41 +0200 Subject: [PATCH 04/10] Record the visibility limit of the alias scan The scan reads through the table's owner-visibility policy, so it sees the org partition and this caller's own rows but not another subject's. An alias held by a different subject is invisible to it. Left as-is on purpose: reading around a tenant-isolation boundary to widen a DELETE would be a worse defect than the narrow one it closes. --- packages/core/sdk/src/executor.ts | 32 ++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index 6d4947345..9266f28bb 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -3223,20 +3223,30 @@ export const createExecutor = () - : yield* core.findMany("connection", { where: () => true }).pipe( - Effect.map( - (rows) => - new Set( - rows.flatMap((other) => [ - ...Object.values(connectionItemIds(other)), - ...(other.refresh_item_id ? [String(other.refresh_item_id)] : []), - ]), - ), - ), - ); + : yield* core + .findMany("connection", { where: () => true }) + .pipe( + Effect.map( + (rows) => + new Set( + rows.flatMap((other) => [ + ...Object.values(connectionItemIds(other)), + ...(other.refresh_item_id ? [String(other.refresh_item_id)] : []), + ]), + ), + ), + ); for (const id of minted) { if (stillReferenced.has(id)) continue; yield* provider.delete(ProviderItemId.make(id)).pipe(Effect.ignore); From 10efdc23eff23b7d81751966df805c2642bc52c5 Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Wed, 12 Aug 2026 19:29:38 +0200 Subject: [PATCH 05/10] Cover the OAuth half of the deletion, and scope the alias scan by provider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A fresh review found a mutation that survived the entire suite: dropping the `:refresh` sibling from the rebuilt id set left a live, decryptable refresh token in the store after removal — the same orphan this change exists to close, in its most damaging form. Nothing in the SDK exercised an `oauth:` item id, so the security-relevant half was the untested one. An end-to-end authorization flow now covers it, and it fails under that mutation. The alias scan also compared id strings without regard to which provider held them. An id means nothing outside its own provider's namespace, so an unrelated connection holding the same string wrongly protected the item and left it behind. The scan is now scoped to the same provider, which narrows it too. Also records in the provider contract that an item id is unique only within an owner partition: the SDK's ids embed the owner literal but not the subject, so a provider keeping one flat namespace across subjects lets one member's write overwrite another's. The shipped stores file per (tenant, owner, subject); the contract now says so rather than leaving it to be discovered. Restores the JSDoc that the earlier insertion had detached from `connectionItemIds`. --- .../connection-remove-credential-gc.test.ts | 84 ++++++++++++++++++- packages/core/sdk/src/executor.ts | 15 +++- packages/core/sdk/src/provider.ts | 11 ++- 3 files changed, 104 insertions(+), 6 deletions(-) diff --git a/packages/core/sdk/src/connection-remove-credential-gc.test.ts b/packages/core/sdk/src/connection-remove-credential-gc.test.ts index 6a9298971..f5a5fde3e 100644 --- a/packages/core/sdk/src/connection-remove-credential-gc.test.ts +++ b/packages/core/sdk/src/connection-remove-credential-gc.test.ts @@ -5,13 +5,15 @@ import { AuthTemplateSlug, ConnectionName, IntegrationSlug, + OAuthClientSlug, ProviderItemId, ProviderKey, ToolName, } from "./ids"; import { definePlugin } from "./plugin"; import type { CredentialProvider } from "./provider"; -import { makeTestExecutor } from "./test-config"; +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 @@ -60,6 +62,30 @@ const demoPlugin = (store: Map, writable = true) => }), }))(); +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: {} }), + }), +}))(); + const setup = (store: Map, writable = true) => makeTestExecutor({ plugins: [demoPlugin(store, writable)] as const }).pipe( Effect.tap((executor) => executor.demo.seed()), @@ -181,6 +207,62 @@ describe("removing a connection removes the credential it minted", () => { }), ); + 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, + // so without this the `:refresh` half of the rebuild is unpinned. + const store = new Map(); + const server = yield* serveOAuthTestServer({}); + const { executor } = yield* makeTestWorkspaceHarness({ + plugins: [demoPlugin(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 }); + + const accessId = "oauth:org:oauthdemo:main"; + expect(store.get(accessId)).toEqual(expect.any(String)); + expect(store.get(`${accessId}:refresh`)).toEqual(expect.any(String)); + + 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(`${accessId}:refresh`)).toBe(false); + }), + ), + ); + it.effect("removing one connection does not touch another's credential", () => Effect.gen(function* () { const store = new Map(); diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index 9266f28bb..5627d9e49 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -861,9 +861,6 @@ const normalizeConnectionInputs = ( return [{ variable: PRIMARY_INPUT_VARIABLE, origin: { value: input.value } }]; }; -/** Decode a connection row's `item_ids` JSON map (`variable → provider item id`). - * Tolerates the historically-single shape by returning `{}` for anything that - * isn't an object. */ /** The provider items a connection MINTED, as opposed to ones it merely points * at. * @@ -891,6 +888,9 @@ const mintedItemIds = (row: ConnectionRow): readonly string[] => { return [...new Set(stored.filter((id) => mintable.has(id)))]; }; +/** Decode a connection row's `item_ids` JSON map (`variable → provider item id`). + * Tolerates the historically-single shape by returning `{}` for anything that + * isn't an object. */ const connectionItemIds = (row: ConnectionRow): Record => { const decoded = decodeJsonColumn(row.item_ids); if (decoded == null || typeof decoded !== "object") return {}; @@ -3235,7 +3235,14 @@ export const createExecutor = () : yield* core - .findMany("connection", { where: () => true }) + .findMany("connection", { + // 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. + where: (b: AnyCb) => b("provider", "=", String(row.provider)), + }) .pipe( Effect.map( (rows) => diff --git a/packages/core/sdk/src/provider.ts b/packages/core/sdk/src/provider.ts index 42a3defa4..98835883d 100644 --- a/packages/core/sdk/src/provider.ts +++ b/packages/core/sdk/src/provider.ts @@ -24,7 +24,16 @@ export interface CredentialProvider { * connection's `remove` only drops our routing, leaving the item intact. */ readonly writable: boolean; /** Resolve a value by opaque id. The single hop a credential goes through - * before its template is applied. The provider interprets the id. */ + * before its template is applied. The provider interprets the id. + * + * AN ITEM ID IS UNIQUE ONLY WITHIN AN OWNER PARTITION, never globally. The + * ids the SDK constructs embed the owner LITERAL (`org` / `user`) but not the + * subject, so two members of the same org computing an id for their own + * `user`-owned connection arrive at the SAME string. A provider that keeps + * one flat namespace across subjects will therefore let one member's write + * overwrite another's, and one member's delete remove another's — the shipped + * stores avoid this by filing rows per `(tenant, owner, subject)`. Partition + * by the same key, or two people quietly share one credential slot. */ readonly get: (id: ProviderItemId) => Effect.Effect; readonly has?: (id: ProviderItemId) => Effect.Effect; readonly set?: (id: ProviderItemId, value: string) => Effect.Effect; From c63bb0d6c8e6a94150b9e9b2f81c82a6279343b9 Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Wed, 12 Aug 2026 22:56:35 +0200 Subject: [PATCH 06/10] Add a changeset for the orphaned-credential fix --- .changeset/orphaned-credential-on-connection-remove.md | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 .changeset/orphaned-credential-on-connection-remove.md diff --git a/.changeset/orphaned-credential-on-connection-remove.md b/.changeset/orphaned-credential-on-connection-remove.md new file mode 100644 index 000000000..c7232906e --- /dev/null +++ b/.changeset/orphaned-credential-on-connection-remove.md @@ -0,0 +1,9 @@ +--- +"executor": patch +--- + +**Removing a connection now deletes the credential it minted** + +`connections.remove` deleted the connection row and left the credential it had minted in the store. The secret outlived the only thing that referenced it, with no surface left in the product to see or remove it — so a user who disconnected an account still had that account's tokens held on their behalf. + +Removal now also deletes the items the connection minted, identified by rebuilding their deterministic ids from the connection row rather than by scanning for anything that looks related. A minted credential that another connection still points at is kept. The alias scan is scoped to the provider that owns the connection, and both the OAuth and the static halves of the deletion are covered. From 64b9376f393890c1af6ad1d598e9cecb53117e20 Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Wed, 12 Aug 2026 23:24:59 +0200 Subject: [PATCH 07/10] Delete the minted credential after the outermost transaction commits --- .../connection-remove-credential-gc.test.ts | 63 +++++- packages/core/sdk/src/executor.ts | 192 ++++++++++-------- 2 files changed, 173 insertions(+), 82 deletions(-) diff --git a/packages/core/sdk/src/connection-remove-credential-gc.test.ts b/packages/core/sdk/src/connection-remove-credential-gc.test.ts index f5a5fde3e..f228a65a5 100644 --- a/packages/core/sdk/src/connection-remove-credential-gc.test.ts +++ b/packages/core/sdk/src/connection-remove-credential-gc.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "@effect/vitest"; -import { Effect } from "effect"; +import { Effect, Exit } from "effect"; import { AuthTemplateSlug, @@ -288,3 +288,64 @@ describe("removing a connection removes the credential it minted", () => { }), ); }); + +// 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. +const txPlugin = (store: Map) => + definePlugin(() => ({ + id: "demo" as const, + credentialProviders: [inspectableProvider(store, true)], + 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), + }), + }))(); + +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* makeTestExecutor({ plugins: [txPlugin(store)] as const }).pipe( + Effect.tap((e) => e.demo.seed()), + ); + const ref = { + owner: "org", + integration: INTEG, + name: ConnectionName.make("main"), + } as const; + yield* executor.connections.create({ ...ref, template: TEMPLATE, value: "secret-token" }); + const mintedId = "connection:org:vercel:main:token"; + expect(store.get(mintedId)).toBe("secret-token"); + + // 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 5627d9e49..ccd7b887a 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -3134,6 +3134,115 @@ export const createExecutor = => + Effect.gen(function* () { + // Now the credential itself, not just the routing to it. Deleting the + // rows above only dropped the pointer: the secret stayed in the + // provider and stayed decryptable, which is precisely what a user + // deleting 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 at 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 created and cannot restore. The two + // are told apart by rebuilding the deterministic id we would have + // written and requiring exact equality, because that is the only test + // that cannot mistake somebody else's item for one of ours. It also + // leaves the OAuth app's `oauth-client:…:secret` alone, which is + // shared by every connection minted through that app. + // + // Best-effort: a provider that cannot delete must not resurrect a + // connection the user has already removed, so a failure here leaves + // an orphan exactly as before rather than failing the removal. + // + // TWO CASES THIS DOES NOT COVER, both deliberate: + // - A v1-migrated connection stores a `secret_` id, which is + // executor-owned but not derivable from v2 state, so it cannot be + // matched and its item is still left behind. Closing that needs a + // schema change, not a better rule here. + // - A second connection can point at this one's minted id through the + // `from` origin, in which case deleting it breaks that connection. + // Detecting it needs a reference scan across the partition. + // + // `writable` is checked as well as the id, never instead of it: a + // picked item can live in a writable store, so writability alone would + // destroy user data. It 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) { + const minted = mintedItemIds(row); + // 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. The connection row above is already gone, so + // anything still referencing the id here is by definition somebody + // else, and the item stays. + // + // This read is owner-scoped by the table's own visibility policy, so + // 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 can still be deleted. + // That is left as-is deliberately: reading around a tenant-isolation + // boundary to widen a DELETE would be a worse defect than the narrow + // one it closes. + const stillReferenced = + minted.length === 0 + ? new Set() + : yield* core + .findMany("connection", { + // 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. + where: (b: AnyCb) => b("provider", "=", String(row.provider)), + }) + .pipe( + Effect.map( + (rows) => + new Set( + rows.flatMap((other) => [ + ...Object.values(connectionItemIds(other)), + ...(other.refresh_item_id ? [String(other.refresh_item_id)] : []), + ]), + ), + ), + ); + for (const id of minted) { + if (stillReferenced.has(id)) continue; + yield* provider.delete(ProviderItemId.make(id)).pipe(Effect.ignore); + } + } + }).pipe(Effect.ignoreCause({ log: false })); + const connectionsRemove = ( ref: ConnectionRef, ): Effect.Effect => @@ -3179,88 +3288,9 @@ export const createExecutor = ` id, which is - // executor-owned but not derivable from v2 state, so it cannot be - // matched and its item is still left behind. Closing that needs a - // schema change, not a better rule here. - // - A second connection can point at this one's minted id through the - // `from` origin, in which case deleting it breaks that connection. - // Detecting it needs a reference scan across the partition. - // - // `writable` is checked as well as the id, never instead of it: a - // picked item can live in a writable store, so writability alone would - // destroy user data. It 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) { - const minted = mintedItemIds(row); - // 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. The connection row above is already gone, so - // anything still referencing the id here is by definition somebody - // else, and the item stays. - // - // This read is owner-scoped by the table's own visibility policy, so - // 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 can still be deleted. - // That is left as-is deliberately: reading around a tenant-isolation - // boundary to widen a DELETE would be a worse defect than the narrow - // one it closes. - const stillReferenced = - minted.length === 0 - ? new Set() - : yield* core - .findMany("connection", { - // 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. - where: (b: AnyCb) => b("provider", "=", String(row.provider)), - }) - .pipe( - Effect.map( - (rows) => - new Set( - rows.flatMap((other) => [ - ...Object.values(connectionItemIds(other)), - ...(other.refresh_item_id ? [String(other.refresh_item_id)] : []), - ]), - ), - ), - ); - for (const id of minted) { - if (stillReferenced.has(id)) continue; - yield* provider.delete(ProviderItemId.make(id)).pipe(Effect.ignore); - } - } + return row; }), - ); + ).pipe(Effect.flatMap((row) => afterCommit(deleteMintedCredentials(row)))); const connectionsRefresh = ( ref: ConnectionRef, From 2f1387d42336e5d9a66c65b2793db8e88cefe6f8 Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Thu, 13 Aug 2026 01:11:25 +0200 Subject: [PATCH 08/10] Remove the credentials a dropped integration's connections minted --- .../integration-removal-credential-gc.md | 11 +++ .../connection-remove-credential-gc.test.ts | 83 +++++++++++++++++++ packages/core/sdk/src/executor.ts | 29 ++++++- 3 files changed, 119 insertions(+), 4 deletions(-) create mode 100644 .changeset/integration-removal-credential-gc.md diff --git a/.changeset/integration-removal-credential-gc.md b/.changeset/integration-removal-credential-gc.md new file mode 100644 index 000000000..e411898e3 --- /dev/null +++ b/.changeset/integration-removal-credential-gc.md @@ -0,0 +1,11 @@ +--- +"executor": patch +--- + +**Removing an integration now removes the credentials its connections minted** + +`integrations.remove` deletes every connection row belonging to the integration. It left the credentials those connections had minted in the store — the same orphan `connections.remove` was fixed to prevent, reachable through a different path and stranding many secrets at once rather than one. + +An orphaned refresh token is the worst case: long-lived by design, no longer referenced by anything, and invisible in the product, so nobody can see it to revoke it. + +The rows are read before they are deleted, because once they are gone nothing names the items they minted. Only minted ids are removed — an item the connection merely referenced is left alone, exactly as on the single-connection path. The deletion is deferred until the removal commits, so a rolled-back removal leaves the credentials intact rather than restoring connections that point at secrets which no longer exist. diff --git a/packages/core/sdk/src/connection-remove-credential-gc.test.ts b/packages/core/sdk/src/connection-remove-credential-gc.test.ts index f228a65a5..b585fcfcc 100644 --- a/packages/core/sdk/src/connection-remove-credential-gc.test.ts +++ b/packages/core/sdk/src/connection-remove-credential-gc.test.ts @@ -349,3 +349,86 @@ describe("the credential deletion runs after the transaction commits", () => { }), ); }); + +// Removing the INTEGRATION takes the same connection rows out, in bulk. It left +// every secret those connections had minted behind — the identical orphan the +// per-connection removal above exists to prevent, reachable through a different +// door and stranding many at once instead of one. Paired the same way: what we +// minted goes, what the user already had stays. +describe("removing an integration removes the credentials its connections minted", () => { + it.effect("deletes the minted items of every connection it drops", () => + Effect.gen(function* () { + const store = new Map(); + const executor = yield* setup(store); + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make("one"), + integration: INTEG, + template: TEMPLATE, + value: "secret-one", + }); + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make("two"), + integration: INTEG, + template: TEMPLATE, + value: "secret-two", + }); + expect(store.get("connection:org:vercel:one:token")).toBe("secret-one"); + expect(store.get("connection:org:vercel:two:token")).toBe("secret-two"); + + yield* executor.integrations.remove(INTEG); + + // Both, not just the first — the bulk delete is the whole point. + expect(store.has("connection:org:vercel:one:token")).toBe(false); + expect(store.has("connection:org:vercel:two:token")).toBe(false); + }), + ); + + it.effect("keeps an item the connection only referenced", () => + Effect.gen(function* () { + const store = new Map(); + const executor = yield* setup(store); + 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.integrations.remove(INTEG); + + // Widening a delete to a whole integration must not widen WHAT it deletes. + expect(store.get("ext-item")).toBe("user-owned-secret"); + }), + ); + + it.effect("a rolled-back integration removal leaves the credentials intact", () => + Effect.gen(function* () { + const store = new Map(); + const executor = yield* makeTestExecutor({ plugins: [txPlugin(store)] as const }).pipe( + Effect.tap((e) => e.demo.seed()), + ); + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make("one"), + integration: INTEG, + template: TEMPLATE, + value: "secret-one", + }); + + const outcome = yield* Effect.exit( + executor.demo.inTransaction( + Effect.gen(function* () { + yield* executor.integrations.remove(INTEG); + return yield* Effect.fail("rollback" as const); + }), + ), + ); + expect(Exit.isFailure(outcome)).toBe(true); + expect(store.get("connection:org:vercel:one:token")).toBe("secret-one"); + }), + ); +}); diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index ccd7b887a..36a0e83c7 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -2441,18 +2441,39 @@ export const createExecutor = b("integration", "=", String(slug)); + // Read the connections BEFORE dropping them. Once those rows are gone + // nothing names the credentials they minted, and those are the + // long-lived secrets: a refresh token that outlives the integration it + // belonged to is invisible in the product and revocable by nobody. + // `connections.remove` already deletes them one at a time; removing + // the integration took the same rows out in bulk and left every + // secret behind. + const doomed = yield* core.findMany("connection", { where }); yield* core.deleteMany("tool", { where }); yield* core.deleteMany("definition", { where }); yield* core.deleteMany("connection", { where }); yield* core.deleteMany("integration", { where: (b: AnyCb) => b("slug", "=", String(slug)), }); - return existing.plugin_id; + return { pluginId: existing.plugin_id, doomed }; }), ).pipe( - Effect.tap((removedPluginId) => - removedPluginId !== null - ? notifyIntegrationChange({ kind: "removed", pluginKey: removedPluginId, slug }) + Effect.tap((removed) => + removed !== null + ? notifyIntegrationChange({ kind: "removed", pluginKey: removed.pluginId, slug }) + : Effect.void, + ), + Effect.tap((removed) => + removed !== null + ? // After the OUTERMOST commit, for the same reason the single-row + // removal defers: deleting a secret is not undone by a rollback, + // so a restored connection would point at a credential that no + // longer exists. + afterCommit( + Effect.gen(function* () { + for (const row of removed.doomed) yield* deleteMintedCredentials(row); + }), + ) : Effect.void, ), Effect.asVoid, From 1ee7aa9dab81427f52c43f2ab9d349a99627a04c Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Thu, 13 Aug 2026 01:19:50 +0200 Subject: [PATCH 09/10] Pin that a bulk removal spares an item another integration still uses --- .../connection-remove-credential-gc.test.ts | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/packages/core/sdk/src/connection-remove-credential-gc.test.ts b/packages/core/sdk/src/connection-remove-credential-gc.test.ts index b585fcfcc..569533505 100644 --- a/packages/core/sdk/src/connection-remove-credential-gc.test.ts +++ b/packages/core/sdk/src/connection-remove-credential-gc.test.ts @@ -432,3 +432,68 @@ describe("removing an integration removes the credentials its connections minted }), ); }); + +// The dangerous direction of the bulk delete: a connection on a DIFFERENT +// integration can reference an item this integration's connection minted. That +// connection survives the removal and is still using the credential, so +// deleting it would destroy a secret belonging to something still alive — the +// same "it is still there" pairing the single-connection path already carries, +// asked of the wider blast radius. +const OTHER = IntegrationSlug.make("netlify"); + +const twoIntegrationPlugin = (store: Map) => + definePlugin(() => ({ + id: "demo" as const, + credentialProviders: [inspectableProvider(store, true)], + storage: () => ({}), + resolveTools: () => + Effect.succeed({ tools: [{ name: ToolName.make("deploy"), description: "deploy" }] }), + invokeTool: ({ toolRow }) => Effect.succeed({ ran: toolRow.name }), + extension: (ctx) => ({ + seed: () => + Effect.gen(function* () { + yield* ctx.core.integrations.register({ slug: INTEG, description: "Vercel", config: {} }); + yield* ctx.core.integrations.register({ + slug: OTHER, + description: "Netlify", + config: {}, + }); + }), + }), + }))(); + +describe("removing an integration respects connections that outlive it", () => { + it.effect("keeps a minted item another integration's connection still points at", () => + Effect.gen(function* () { + const store = new Map(); + const executor = yield* makeTestExecutor({ + plugins: [twoIntegrationPlugin(store)] as const, + }).pipe(Effect.tap((e) => e.demo.seed())); + + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make("a"), + integration: INTEG, + template: TEMPLATE, + value: "shared-secret", + }); + const mintedId = "connection:org:vercel:a:token"; + expect(store.get(mintedId)).toBe("shared-secret"); + + // A live connection on a different integration, pointing at that item. + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make("b"), + integration: OTHER, + template: TEMPLATE, + from: { provider: ProviderKey.make("memory"), id: ProviderItemId.make(mintedId) }, + }); + + yield* executor.integrations.remove(INTEG); + + // "b" is untouched by this removal and is still using the credential. + // Deleting it would break a connection that nobody asked to remove. + expect(store.get(mintedId)).toBe("shared-secret"); + }), + ); +}); From 01dbbb03b885eaff5f4d88ee05ed8509f61fc4f3 Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Fri, 14 Aug 2026 18:31:35 +0200 Subject: [PATCH 10/10] style(sdk): indent the minted-credential block to the repo's format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Effect.gen body sat at the same column as the Effect.gen( line rather than one level deeper, so `oxfmt --check` fails on this branch. Whitespace only — `git diff -w` is empty. Caught by running the repo's own gates against the branch. Fork CI has never run on it, so nothing had checked. --- packages/core/sdk/src/executor.ts | 156 +++++++++++++++--------------- 1 file changed, 78 insertions(+), 78 deletions(-) diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index 36a0e83c7..2f89b130c 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -3182,86 +3182,86 @@ export const createExecutor = => Effect.gen(function* () { - // Now the credential itself, not just the routing to it. Deleting the - // rows above only dropped the pointer: the secret stayed in the - // provider and stayed decryptable, which is precisely what a user - // deleting 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 at 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 created and cannot restore. The two - // are told apart by rebuilding the deterministic id we would have - // written and requiring exact equality, because that is the only test - // that cannot mistake somebody else's item for one of ours. It also - // leaves the OAuth app's `oauth-client:…:secret` alone, which is - // shared by every connection minted through that app. - // - // Best-effort: a provider that cannot delete must not resurrect a - // connection the user has already removed, so a failure here leaves - // an orphan exactly as before rather than failing the removal. - // - // TWO CASES THIS DOES NOT COVER, both deliberate: - // - A v1-migrated connection stores a `secret_` id, which is - // executor-owned but not derivable from v2 state, so it cannot be - // matched and its item is still left behind. Closing that needs a - // schema change, not a better rule here. - // - A second connection can point at this one's minted id through the - // `from` origin, in which case deleting it breaks that connection. - // Detecting it needs a reference scan across the partition. - // - // `writable` is checked as well as the id, never instead of it: a - // picked item can live in a writable store, so writability alone would - // destroy user data. It 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) { - const minted = mintedItemIds(row); - // 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. The connection row above is already gone, so - // anything still referencing the id here is by definition somebody - // else, and the item stays. + // Now the credential itself, not just the routing to it. Deleting the + // rows above only dropped the pointer: the secret stayed in the + // provider and stayed decryptable, which is precisely what a user + // deleting a connection is asking us to stop being true. // - // This read is owner-scoped by the table's own visibility policy, so - // 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 can still be deleted. - // That is left as-is deliberately: reading around a tenant-isolation - // boundary to widen a DELETE would be a worse defect than the narrow - // one it closes. - const stillReferenced = - minted.length === 0 - ? new Set() - : yield* core - .findMany("connection", { - // 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. - where: (b: AnyCb) => b("provider", "=", String(row.provider)), - }) - .pipe( - Effect.map( - (rows) => - new Set( - rows.flatMap((other) => [ - ...Object.values(connectionItemIds(other)), - ...(other.refresh_item_id ? [String(other.refresh_item_id)] : []), - ]), - ), - ), - ); - for (const id of minted) { - if (stillReferenced.has(id)) continue; - yield* provider.delete(ProviderItemId.make(id)).pipe(Effect.ignore); + // Only ids THIS connection minted. A connection can instead REFERENCE + // an item the user already had (the `from` origin at 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 created and cannot restore. The two + // are told apart by rebuilding the deterministic id we would have + // written and requiring exact equality, because that is the only test + // that cannot mistake somebody else's item for one of ours. It also + // leaves the OAuth app's `oauth-client:…:secret` alone, which is + // shared by every connection minted through that app. + // + // Best-effort: a provider that cannot delete must not resurrect a + // connection the user has already removed, so a failure here leaves + // an orphan exactly as before rather than failing the removal. + // + // TWO CASES THIS DOES NOT COVER, both deliberate: + // - A v1-migrated connection stores a `secret_` id, which is + // executor-owned but not derivable from v2 state, so it cannot be + // matched and its item is still left behind. Closing that needs a + // schema change, not a better rule here. + // - A second connection can point at this one's minted id through the + // `from` origin, in which case deleting it breaks that connection. + // Detecting it needs a reference scan across the partition. + // + // `writable` is checked as well as the id, never instead of it: a + // picked item can live in a writable store, so writability alone would + // destroy user data. It 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) { + const minted = mintedItemIds(row); + // 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. The connection row above is already gone, so + // anything still referencing the id here is by definition somebody + // else, and the item stays. + // + // This read is owner-scoped by the table's own visibility policy, so + // 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 can still be deleted. + // That is left as-is deliberately: reading around a tenant-isolation + // boundary to widen a DELETE would be a worse defect than the narrow + // one it closes. + const stillReferenced = + minted.length === 0 + ? new Set() + : yield* core + .findMany("connection", { + // 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. + where: (b: AnyCb) => b("provider", "=", String(row.provider)), + }) + .pipe( + Effect.map( + (rows) => + new Set( + rows.flatMap((other) => [ + ...Object.values(connectionItemIds(other)), + ...(other.refresh_item_id ? [String(other.refresh_item_id)] : []), + ]), + ), + ), + ); + for (const id of minted) { + if (stillReferenced.has(id)) continue; + yield* provider.delete(ProviderItemId.make(id)).pipe(Effect.ignore); + } } - } }).pipe(Effect.ignoreCause({ log: false })); const connectionsRemove = (