diff --git a/e2e/selfhost/oauth-picker-cross-integration.test.ts b/e2e/selfhost/oauth-picker-cross-integration.test.ts new file mode 100644 index 000000000..28cdf5784 --- /dev/null +++ b/e2e/selfhost/oauth-picker-cross-integration.test.ts @@ -0,0 +1,166 @@ +// Selfhost (browser): REPRO for issue #1120 ("Can't configure oauth mcp which +// used to work"). The user added an Atlassian remote-MCP source and the OAuth +// app picker listed a pile of unrelated *Datadog* apps. +// +// Why it happens: a remote MCP OAuth method carries ONLY a `discoveryUrl` +// (endpoints are discovered live, see describeMcpAuthMethods in the mcp plugin), +// never a static token/authorization URL. When transparent DCR falls back to the +// bring-your-own app picker, the picker filters candidate apps by endpoint root +// domain (`selectClientsForEndpoints`). With no declared endpoints the filter +// short-circuits to "every app is usable", so EVERY registered OAuth client the +// owner has, including DCR-minted apps from other integrations, leaks into this +// integration's picker. The `origin.integration` slug each client carries is +// never consulted. +// +// The fix: the picker matches apps to the integration by `origin.integration` +// (recorded on every app, DCR or manual), falling back to endpoint-domain +// matching only when endpoints are declared. So an app registered FOR Atlassian +// shows, and a Datadog app (different integration, no shared endpoint) does not. +// +// This scenario registers two apps for the same owner — one Datadog app with NO +// integration association, and one app registered FOR the Atlassian MCP source — +// then drives that source into the BYO picker (its discovery endpoint advertises +// no OAuth metadata, so DCR falls back) and asserts the Atlassian app appears +// while the Datadog app does not. +import { randomBytes } from "node:crypto"; + +import { expect } from "@effect/vitest"; +import { Effect } from "effect"; +import { composePluginApi } from "@executor-js/api/server"; +import { mcpHttpPlugin } from "@executor-js/plugin-mcp/api"; +import { makeGreetingMcpServer, serveMcpServer } from "@executor-js/plugin-mcp/testing"; +import { IntegrationSlug, OAuthClientSlug } from "@executor-js/sdk/shared"; + +import { scenario } from "../src/scenario"; +import { Api, Browser, Target } from "../src/services"; + +const api = composePluginApi([mcpHttpPlugin()] as const); + +scenario( + "OAuth picker · a different integration's app does not leak into an MCP source's picker", + { timeout: 180_000 }, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const browser = yield* Browser; + const { client: makeApiClient } = yield* Api; + const identity = yield* target.newIdentity(); + const client = yield* makeApiClient(api, identity); + + // Selfhost shares one tenant across identities, so make every resource + // unique per run — including the Datadog host, which is what the picker + // row renders and what we assert against. + const id = randomBytes(4).toString("hex"); + const datadogSlug = OAuthClientSlug.make(`datadog-${id}`); + const datadogHost = `api-${id}.datadoghq.com`; + const atlassianSlug = OAuthClientSlug.make(`atlassian-app-${id}`); + const atlassianHost = `auth-${id}.atlassian.com`; + const mcpSlug = IntegrationSlug.make(`atlassian-mcp-${id}`); + + // An Atlassian-shaped remote MCP source: it speaks MCP but advertises NO + // OAuth metadata, so the connect-time DCR probe finds no registration + // endpoint and the modal falls back to the bring-your-own app picker. + const server = yield* serveMcpServer(() => + makeGreetingMcpServer({ name: `atlassian-mcp-${id}` }), + ); + + yield* Effect.ensuring( + Effect.gen(function* () { + // A Datadog OAuth app the owner registered for a DIFFERENT integration. + // Endpoints are on datadoghq.com — nothing to do with Atlassian, and no + // integration association — so it must never surface in this picker. + yield* client.oauth.createClient({ + payload: { + owner: "user", + slug: datadogSlug, + authorizationUrl: `https://${datadogHost}/oauth2/authorize`, + tokenUrl: `https://${datadogHost}/oauth2/token`, + grant: "authorization_code", + clientId: `datadog-client-${id}`, + clientSecret: "datadog-secret", + }, + }); + + // An app the owner registered FOR the Atlassian MCP source. It carries + // `origin.integration`, so the picker surfaces it even though the source + // declares no static endpoints to match on. + yield* client.oauth.createClient({ + payload: { + owner: "user", + slug: atlassianSlug, + authorizationUrl: `https://${atlassianHost}/authorize`, + tokenUrl: `https://${atlassianHost}/token`, + grant: "authorization_code", + clientId: `atlassian-client-${id}`, + clientSecret: "atlassian-secret", + originIntegration: mcpSlug, + }, + }); + + // Persistence guard: the integration association round-trips through the + // API so the picker can rely on it (isolates server-side persistence + // from the browser read path). + const listed = yield* client.oauth.listClients(); + const atlassianRow = listed.find((c) => String(c.slug) === String(atlassianSlug)); + expect( + atlassianRow?.origin.integration && String(atlassianRow.origin.integration), + "the manually-registered app records its integration association", + ).toBe(String(mcpSlug)); + + // The Atlassian MCP source as the add flow would leave it: OAuth only. + yield* client.mcp.addServer({ + payload: { + transport: "remote", + name: `Atlassian MCP ${id}`, + endpoint: server.endpoint, + slug: mcpSlug, + authenticationTemplate: [{ kind: "oauth2" }], + }, + }); + + yield* browser.session(identity, async ({ page, step }) => { + await step("Open the Atlassian MCP source's connect modal", async () => { + await page.goto(`/integrations/${mcpSlug}`, { waitUntil: "networkidle" }); + await page.getByRole("button", { name: "Add connection" }).first().click(); + // OAuth is the only method; transparent DCR shows the Connect CTA. + await page.getByRole("button", { name: "Connect", exact: true }).waitFor(); + }); + + await step("DCR falls back to the bring-your-own app picker", async () => { + await page.getByRole("button", { name: "Connect", exact: true }).click(); + // The probe finds no OAuth metadata and DCR falls back to the app + // step. Either register affordance ("Register a new app" when an app + // matched, "Register app" in the empty state) means we're past DCR. + await page + .getByRole("button", { name: /^Register (app|a new app)$/ }) + .first() + .waitFor({ timeout: 30_000 }); + }); + + await step("The picker lists the Atlassian app, not the Datadog one", async () => { + const dialog = page.getByRole("dialog"); + expect( + await dialog.getByText(atlassianHost).count(), + `the app registered for this integration (${atlassianHost}) is offered`, + ).toBeGreaterThan(0); + expect( + await dialog.getByText(datadogHost).count(), + `a Datadog OAuth app (${datadogHost}) must not leak into Atlassian's picker`, + ).toBe(0); + }); + }); + }), + // Never leak the source or the apps into the shared selfhost tenant. + Effect.gen(function* () { + yield* client.mcp.removeServer({ params: { slug: mcpSlug } }).pipe(Effect.ignore); + yield* client.oauth + .removeClient({ params: { slug: datadogSlug }, payload: { owner: "user" } }) + .pipe(Effect.ignore); + yield* client.oauth + .removeClient({ params: { slug: atlassianSlug }, payload: { owner: "user" } }) + .pipe(Effect.ignore); + }), + ); + }), + ), +); diff --git a/packages/core/api/src/handlers/oauth.ts b/packages/core/api/src/handlers/oauth.ts index 924aff682..dab67175b 100644 --- a/packages/core/api/src/handlers/oauth.ts +++ b/packages/core/api/src/handlers/oauth.ts @@ -103,6 +103,7 @@ export const OAuthHandlers = HttpApiBuilder.group(ExecutorApi, "oauth", (handler clientId: payload.clientId, clientSecret: payload.clientSecret, resource: payload.resource ?? null, + origin: { kind: "manual", integration: payload.originIntegration ?? null }, }); return { client }; }), diff --git a/packages/core/api/src/oauth/api.ts b/packages/core/api/src/oauth/api.ts index ed9e74356..aa73d58a7 100644 --- a/packages/core/api/src/oauth/api.ts +++ b/packages/core/api/src/oauth/api.ts @@ -65,6 +65,10 @@ const CreateClientPayload = Schema.Struct({ clientId: Schema.String, clientSecret: Schema.String, resource: Schema.optional(Schema.NullOr(Schema.String)), + /** Integration this app is being registered for, recorded on the client so the + * connect picker can surface it for that integration (mirrors the DCR path). + * A manual app can still be reused across integrations. */ + originIntegration: Schema.optional(Schema.NullOr(IntegrationSlug)), }); const CreateClientResponse = Schema.Struct({ @@ -111,7 +115,10 @@ const OAuthClientSummaryResponse = Schema.Struct({ resource: Schema.optional(Schema.NullOr(Schema.String)), clientId: Schema.String, origin: Schema.Union([ - Schema.Struct({ kind: Schema.Literal("manual") }), + Schema.Struct({ + kind: Schema.Literal("manual"), + integration: Schema.optional(Schema.NullOr(IntegrationSlug)), + }), Schema.Struct({ kind: Schema.Literal("dynamic_client_registration"), integration: Schema.optional(Schema.NullOr(IntegrationSlug)), diff --git a/packages/core/sdk/src/core-tools.ts b/packages/core/sdk/src/core-tools.ts index 042906557..bd9118d05 100644 --- a/packages/core/sdk/src/core-tools.ts +++ b/packages/core/sdk/src/core-tools.ts @@ -221,7 +221,10 @@ const OAuthClientOutput = Schema.Struct({ resource: Schema.optional(Schema.NullOr(Schema.String)), clientId: Schema.String, origin: Schema.Union([ - Schema.Struct({ kind: Schema.Literal("manual") }), + Schema.Struct({ + kind: Schema.Literal("manual"), + integration: Schema.optional(Schema.NullOr(Schema.String)), + }), Schema.Struct({ kind: Schema.Literal("dynamic_client_registration"), integration: Schema.optional(Schema.NullOr(Schema.String)), diff --git a/packages/core/sdk/src/index.ts b/packages/core/sdk/src/index.ts index bbf46064c..a8a86f8d4 100644 --- a/packages/core/sdk/src/index.ts +++ b/packages/core/sdk/src/index.ts @@ -228,7 +228,9 @@ export { type OAuthGrant, type OAuthAuthentication, type OAuthClient, + type OAuthClientOrigin, type OAuthClientSummary, + oauthClientOriginIntegration, type CreateOAuthClientInput, type RegisterDynamicClientInput, type ConnectResult, diff --git a/packages/core/sdk/src/oauth-client.ts b/packages/core/sdk/src/oauth-client.ts index 3b56d9f05..d48ed0ee7 100644 --- a/packages/core/sdk/src/oauth-client.ts +++ b/packages/core/sdk/src/oauth-client.ts @@ -56,12 +56,27 @@ export interface OAuthClient { } export type OAuthClientOrigin = - | { readonly kind: "manual" } + | { + readonly kind: "manual"; + /** Integration this app was registered FOR, when known. A user can reuse + * one app across integrations, so this is a hint (the integration whose + * connect flow created it), not an exclusive binding. Used by the connect + * picker to surface this app for its integration even when the integration + * declares no static endpoints to match on (e.g. an MCP source). */ + readonly integration?: IntegrationSlug | null; + } | { readonly kind: "dynamic_client_registration"; readonly integration?: IntegrationSlug | null; }; +/** The integration an OAuth app was registered for, from either origin kind + * (null when unknown). Centralizes the "which integration owns this app" read so + * the picker and the duplicate-DCR guard agree. */ +export const oauthClientOriginIntegration = ( + origin: OAuthClientOrigin, +): IntegrationSlug | null => origin.integration ?? null; + export type CreateOAuthClientInput = OAuthClient & { readonly origin?: OAuthClientOrigin; }; diff --git a/packages/core/sdk/src/oauth-service.ts b/packages/core/sdk/src/oauth-service.ts index 008719b75..382e72a61 100644 --- a/packages/core/sdk/src/oauth-service.ts +++ b/packages/core/sdk/src/oauth-service.ts @@ -229,14 +229,10 @@ const parseOAuthClientOrigin = (row: { readonly origin_kind?: unknown; readonly origin_integration?: unknown; }): OAuthClientOrigin => { + const integration = + row.origin_integration == null ? null : IntegrationSlug.make(String(row.origin_integration)); if (row.origin_kind === "dynamic_client_registration") { - return { - kind: "dynamic_client_registration", - integration: - row.origin_integration == null - ? null - : IntegrationSlug.make(String(row.origin_integration)), - }; + return { kind: "dynamic_client_registration", integration }; } const slug = row.slug == null ? "" : String(row.slug); const resource = row.resource == null ? "" : String(row.resource); @@ -246,9 +242,9 @@ const parseOAuthClientOrigin = (row: { /(^|[-_])mcp($|[-_])/.test(slug) && /(^|\/)mcp($|[/?#])/.test(resource) ) { - return { kind: "dynamic_client_registration", integration: null }; + return { kind: "dynamic_client_registration", integration }; } - return { kind: "manual" }; + return { kind: "manual", integration }; }; interface LoadedOAuthClient { @@ -404,11 +400,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { resource: input.resource ?? null, origin_kind: input.origin?.kind ?? "manual", origin_integration: - input.origin?.kind === "dynamic_client_registration" - ? input.origin.integration == null - ? null - : String(input.origin.integration) - : null, + input.origin?.integration == null ? null : String(input.origin.integration), created_at: now, }), ); diff --git a/packages/core/sdk/src/shared.ts b/packages/core/sdk/src/shared.ts index fe5546a90..a3729c7b8 100644 --- a/packages/core/sdk/src/shared.ts +++ b/packages/core/sdk/src/shared.ts @@ -106,7 +106,9 @@ export { type OAuthGrant, type OAuthAuthentication, type OAuthClient, + type OAuthClientOrigin, type OAuthClientSummary, + oauthClientOriginIntegration, type CreateOAuthClientInput, type RegisterDynamicClientInput, type ConnectResult, diff --git a/packages/react/src/api/atoms.tsx b/packages/react/src/api/atoms.tsx index 9cc364fda..607e338db 100644 --- a/packages/react/src/api/atoms.tsx +++ b/packages/react/src/api/atoms.tsx @@ -460,6 +460,7 @@ export const createOAuthClientOptimistic = oauthClientsOptimisticAtom.pipe( readonly grant: OAuthGrant; readonly clientId: string; readonly resource?: string | null; + readonly originIntegration?: IntegrationSlug | null; }; }, ) => @@ -472,7 +473,7 @@ export const createOAuthClientOptimistic = oauthClientsOptimisticAtom.pipe( tokenUrl: arg.payload.tokenUrl, resource: arg.payload.resource ?? null, clientId: arg.payload.clientId, - origin: { kind: "manual" }, + origin: { kind: "manual", integration: arg.payload.originIntegration ?? null }, }; const index = rows.findIndex( (client) => client.owner === summary.owner && client.slug === summary.slug, diff --git a/packages/react/src/components/add-account-modal.tsx b/packages/react/src/components/add-account-modal.tsx index 918cb06fb..ceb12316d 100644 --- a/packages/react/src/components/add-account-modal.tsx +++ b/packages/react/src/components/add-account-modal.tsx @@ -827,7 +827,9 @@ function AddAccountModalView(props: AddAccountModalProps) { loading: oauthLoading, endpointMatched: oauthEndpointMatched, displayRegisterCTA: oauthDisplayRegisterCTA, + allClients: oauthAllClients, } = useOAuthClientsForIntegration({ + integration, tokenUrl: method?.oauth?.tokenUrl, authorizationUrl: method?.oauth?.authorizationUrl, }); @@ -1201,9 +1203,8 @@ function AddAccountModalView(props: AddAccountModalProps) {