diff --git a/server/src/plugins/plugin-store/connections.ts b/server/src/plugins/plugin-store/connections.ts index 68d09778a..330aa1ea3 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 342559f8b..efdc77c19 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. *