From b5533a6e4fa693fee5826090bccaf82649929e4d Mon Sep 17 00:00:00 2001 From: Rahul Ranjan Date: Tue, 27 Oct 2026 17:37:06 +0530 Subject: [PATCH] fix(store): scope the offboarding sweep and settings read to the active provider retireConnectionsFor and brokeredConnectionsFor read brokered connections keyed (holder, userId) only, so a row filed under another provider would be revoked through the wrong broker while the audit row claimed the vendor was asked, or dropped silently from the settings page. Both reads now filter on the active brokerProvider, the predicate the other eleven selects in the file already carry. Unreachable while assertActivatable refuses mixed deployments; this keeps the two reads correct the moment the provider sunset flow relaxes that refusal. Fixes #783 Co-authored-by: rahul05ranjan <11793455+rahul05ranjan@users.noreply.github.com> --- .../src/plugins/plugin-store/connections.ts | 29 +++++---- server/tests/plugin-store.integration.test.ts | 65 +++++++++++++++++++ 2 files changed, 81 insertions(+), 13 deletions(-) diff --git a/server/src/plugins/plugin-store/connections.ts b/server/src/plugins/plugin-store/connections.ts index 68d09778a2..330aa1ea32 100644 --- a/server/src/plugins/plugin-store/connections.ts +++ b/server/src/plugins/plugin-store/connections.ts @@ -405,12 +405,13 @@ export function connectionMethods(context: StoreContext) { * and the filter keeps that a fact this query states rather than one it happens to rely on. */ /* - * NO PROVIDER PREDICATE HERE, DELIBERATELY, AND THAT IS NOT AN OVERSIGHT. Each row is resolved - * below at the ACTIVE provider's canonical url, so another provider's row would be silently - * dropped from the page. What keeps that unreachable is the startup activation refusal - * (`assertActivatable`, called from `server/src/index.ts`): it will not boot when the - * configured provider differs from the one on the existing rows. No flow relaxes that refusal - * today; whoever builds one must add the `brokerProvider` predicate here. + * SCOPED TO THE ACTIVE PROVIDER. The settings page draws rows the ACTIVE broker's + * canonical scheme resolves, so a row filed under another provider would be silently + * dropped from the page anyway; filtering at the read keeps that true out loud. It is + * unreachable today because the startup activation refusal (`assertActivatable`, called + * from `server/src/index.ts`) will not boot a deployment whose configured provider differs + * from the one on the existing rows — so this predicate costs nothing now, and is already + * right when any flow relaxes that refusal (#783). */ const connections = await database .select({ @@ -423,6 +424,7 @@ export function connectionMethods(context: StoreContext) { .from(brokeredConnections) .where( and( + eq(brokeredConnections.provider, brokerProvider), eq(brokeredConnections.holder, "person"), eq(brokeredConnections.userId, userId), ), @@ -3261,19 +3263,20 @@ export function connectionMethods(context: StoreContext) { * order. */ /* - * NO PROVIDER PREDICATE HERE, DELIBERATELY, AND THAT IS NOT AN OVERSIGHT. The sweep below - * revokes through the ACTIVE broker, so a row filed under another provider would be a - * revocation requested at the wrong vendor, which would not recognise the connection, and an - * audit row claiming `vendorRevocationRequested` about access that was never removed. What - * keeps that unreachable is the startup activation refusal (`assertActivatable`, called from - * `server/src/index.ts`): a deployment holding both providers' rows cannot boot. No flow relaxes - * that refusal today; whoever builds one must add the `brokerProvider` predicate here. + * THE ACTIVE BROKER'S ROWS ONLY. The sweep below revokes through the ACTIVE broker, so a + * row filed under another provider would be a revocation requested at a vendor that would + * not recognise the connection, and an audit row claiming `vendorRevocationRequested` about + * access that was never removed — the one reading of that field that must never be written. + * Unreachable today, because the startup activation refusal (`assertActivatable`, called + * from `server/src/index.ts`) will not boot a deployment holding both providers' rows; the + * predicate keeps the sweep correct the moment any flow relaxes that refusal (#783). */ const brokered = await database .select({ toolkit: brokeredConnections.app }) .from(brokeredConnections) .where( and( + eq(brokeredConnections.provider, brokerProvider), eq(brokeredConnections.holder, "person"), eq(brokeredConnections.userId, userId), ), diff --git a/server/tests/plugin-store.integration.test.ts b/server/tests/plugin-store.integration.test.ts index 342559f8bf..efdc77c197 100644 --- a/server/tests/plugin-store.integration.test.ts +++ b/server/tests/plugin-store.integration.test.ts @@ -8008,6 +8008,71 @@ test("offboarding a brokered account nobody held any more withdraws nothing, and }); }); +/** + * The offboarding sweep must not touch another provider's rows. + * + * CRITERION. `retireConnectionsFor` on somebody holding a foreign-provider row revokes only the + * active broker's rows: the foreign row is not revoked, not deleted, and never audited as + * disconnected. + * + * REASON. The sweep revokes through the ACTIVE broker, so a foreign row would be a revocation + * requested at a vendor that would not recognise the connection, while the audit row claimed + * `vendorRevocationRequested` about access that was never removed — the one reading of that field + * that must never be written. Unreachable while `assertActivatable` refuses to boot a mixed + * deployment; this is what keeps the sweep correct when any flow relaxes that refusal (#783). + * + * THE FOREIGN ROW IS READ BACK, not inferred from the revoke count. An implementation that revoked + * it and deleted it also answers a smaller count once the sweep is otherwise correct, so only the + * standing row distinguishes "left alone" from "silently taken" — and the audit read beside it + * says the trail never claimed a withdrawal this deployment could not have performed. + */ +test("offboarding does not revoke or audit another provider's rows", async () => { + const { broker, order } = brokerSpy({ revoke: async () => true }); + const { store, auditStore } = await freshStore({ broker }); + + // The active provider's own row, so the sweep still retires something. + await database.insert(brokeredConnections).values({ + provider: "composio", + app: "gmail", + holder: "person", + userId: "user_leaver", + vendorUserId: "user_leaver", + }); + // A row filed under the provider this deployment does not use. + await database.insert(brokeredConnections).values({ + provider: "arcade", + app: "slack", + holder: "person", + userId: "user_leaver", + vendorUserId: "user_leaver", + }); + + expect(await store.retireConnectionsFor("user_leaver", "admin")).toEqual({ + // One, because the arcade row is not this broker's to revoke. + retired: 1, + }); + expect(order).toEqual(["revoke:user_leaver"]); + + const standing = await database + .select({ + provider: brokeredConnections.provider, + app: brokeredConnections.app, + }) + .from(brokeredConnections) + .where(eq(brokeredConnections.userId, "user_leaver")); + expect(standing).toEqual([{ provider: "arcade", app: "slack" }]); + + const disconnected = auditStore + .recorded() + .filter((event) => event.eventType === "mcp.account_disconnected"); + expect(disconnected.map((event) => event.targetId)).toEqual(["gmail"]); + expect( + disconnected.every( + (event) => event.payload.vendorRevocationRequested === true, + ), + ).toBe(true); +}); + /** * The same false, on the other act that ends a brokered connection. *