diff --git a/e2e/selfhost/mcp-oauth-callback-background-sync.test.ts b/e2e/selfhost/mcp-oauth-callback-background-sync.test.ts new file mode 100644 index 000000000..eef038e3c --- /dev/null +++ b/e2e/selfhost/mcp-oauth-callback-background-sync.test.ts @@ -0,0 +1,112 @@ +// An OAuth callback commits the fresh grant before it synchronizes a remote +// MCP catalog. A slow tools/list response must not keep the popup request open; +// the host keeps catalog work alive and the tools converge afterward. +import { randomBytes } from "node:crypto"; + +import { expect } from "@effect/vitest"; +import { Effect, Schedule } from "effect"; +import { composePluginApi } from "@executor-js/api/server"; +import { deriveMcpNamespace } from "@executor-js/plugin-mcp"; +import { mcpHttpPlugin } from "@executor-js/plugin-mcp/api"; +import { makeGreetingMcpServer, serveMcpServerWithOAuth } from "@executor-js/plugin-mcp/testing"; +import { IntegrationSlug } from "@executor-js/sdk/shared"; +import { OAuthTestServer } from "@executor-js/sdk/testing"; + +import { scenario } from "../src/scenario"; +import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; + +const api = composePluginApi([mcpHttpPlugin()] as const); +const CATALOG_REQUEST_DELAY_MS = 2_000; + +const submitProviderLogin = async (loginUrl: string): Promise => { + const response = await fetch(loginUrl, { + method: "POST", + redirect: "manual", + headers: { authorization: `Basic ${Buffer.from("alice:password").toString("base64")}` }, + }); + const location = response.headers.get("location"); + if (response.status !== 302 || !location) { + throw new Error(`provider login did not redirect (${response.status})`); + } + return new URL(location, loginUrl).toString(); +}; + +scenario( + "MCP OAuth ยท callback closes before a slow remote catalog finishes syncing", + { timeout: 240_000 }, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const browser = yield* Browser; + const { client: makeApiClient } = yield* Api; + const server = yield* serveMcpServerWithOAuth( + () => makeGreetingMcpServer({ name: "slow-callback-mcp" }), + { path: "/mcp", authenticatedRequestDelayMs: CATALOG_REQUEST_DELAY_MS }, + ); + const identity = yield* target.newIdentity(); + const client = yield* makeApiClient(api, identity); + const displayName = `Slow callback MCP ${randomBytes(3).toString("hex")}`; + const slug = IntegrationSlug.make(deriveMcpNamespace({ name: displayName })); + const clientsBefore = new Set((yield* client.oauth.listClients()).map((item) => item.slug)); + + yield* Effect.gen(function* () { + yield* browser.session(identity, async ({ page, step }) => { + await step("Add an OAuth-protected MCP integration", async () => { + const addUrl = new URL("/integrations/add/mcp", target.baseUrl); + addUrl.searchParams.set("url", server.endpoint); + await visit(page, addUrl.toString()); + await page.getByText("How does this server authenticate?").waitFor({ timeout: 30_000 }); + await page.getByPlaceholder("e.g. Linear").fill(displayName); + await page.getByRole("button", { name: "Add integration" }).click(); + await page.waitForURL(/\/integrations\/(?!add\b)[^/?]+$/, { timeout: 30_000 }); + }); + + await step("Authorize while the MCP catalog is deliberately slow", async () => { + await page.getByRole("button", { name: "Add connection" }).first().click(); + await page.getByRole("heading", { name: /Add connection/ }).waitFor(); + + const popupPromise = page.waitForEvent("popup", { timeout: 30_000 }); + await page.getByRole("button", { name: "Connect", exact: true }).click(); + const popup = await popupPromise; + await popup.waitForURL(/\/login\?/, { timeout: 30_000 }); + const callbackUrl = await submitProviderLogin(popup.url()); + + // Each authenticated MCP transport request is held for two seconds. + // The callback has 1.5 seconds to render, so this can pass only if + // catalog discovery is no longer part of the callback response. + await popup.goto(callbackUrl, { waitUntil: "domcontentloaded", timeout: 1_500 }); + await page.getByText("Connection added", { exact: true }).waitFor({ timeout: 30_000 }); + }); + }); + + const tools = yield* client.tools.list({ query: { integration: slug } }).pipe( + Effect.filterOrFail( + (items) => items.some((tool) => String(tool.name) === "simple_echo"), + () => "slow_mcp_catalog_pending" as const, + ), + Effect.retry(Schedule.both(Schedule.spaced("1 second"), Schedule.recurs(20))), + ); + expect( + tools.map((tool) => String(tool.name)), + "the host-kept background sync eventually publishes the remote tool", + ).toContain("simple_echo"); + }).pipe( + Effect.ensuring( + Effect.gen(function* () { + const clientsAfter = yield* client.oauth.listClients(); + for (const oauthClient of clientsAfter) { + if (!clientsBefore.has(oauthClient.slug)) { + yield* client.oauth.removeClient({ + params: { slug: oauthClient.slug }, + payload: { owner: oauthClient.owner }, + }); + } + } + yield* client.mcp.removeServer({ params: { slug } }); + }).pipe(Effect.ignore), + ), + ); + }), + ).pipe(Effect.provide(OAuthTestServer.layer())), +); diff --git a/packages/core/api/src/handlers/oauth.ts b/packages/core/api/src/handlers/oauth.ts index 92c3e5ed7..8406af59b 100644 --- a/packages/core/api/src/handlers/oauth.ts +++ b/packages/core/api/src/handlers/oauth.ts @@ -211,13 +211,16 @@ export const OAuthHandlers = HttpApiBuilder.group(ExecutorApi, "oauth", (handler const html = yield* runOAuthCallback({ complete: ({ state, code, callbackDomain }) => executor.oauth - .complete({ - // `runOAuthCallback`'s `state` is a raw string from the URL; - // the SDK speaks the branded `OAuthState` (nominal brand). - state: OAuthState.make(state), - code: code ?? "", - callbackDomain, - }) + .complete( + { + // `runOAuthCallback`'s `state` is a raw string from the URL; + // the SDK speaks the branded `OAuthState` (nominal brand). + state: OAuthState.make(state), + code: code ?? "", + callbackDomain, + }, + { toolSync: "background" }, + ) .pipe( Effect.tapError((cause: unknown) => Effect.logError("OAuth callback completion failed", cause), diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index aba6b674c..9cac3506d 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -4118,6 +4118,12 @@ export const createExecutor = Effect.succeed([] as readonly Tool[])), ); + if (input.toolSync === "background") { + const fiber = yield* Effect.forkDetach( + syncTools.pipe( + Effect.catch((error) => + Effect.logWarning("executor OAuth tool sync failed", { + integration: String(ref.integration), + connection: String(ref.name), + error: describeSyncFailure(error), + }), + ), + Effect.withSpan("executor.oauth.tools.sync", { + attributes: { + "executor.integration": String(ref.integration), + "executor.connection": String(ref.name), + }, + }), + ), + ); + config.waitUntil?.( + new Promise((resolve) => fiber.addObserver(() => resolve(undefined))), + ); + } else { + yield* syncTools; + } const row = yield* findConnectionRow(ref); return row diff --git a/packages/core/sdk/src/oauth-client.ts b/packages/core/sdk/src/oauth-client.ts index 1d2e27c28..defab1668 100644 --- a/packages/core/sdk/src/oauth-client.ts +++ b/packages/core/sdk/src/oauth-client.ts @@ -340,6 +340,13 @@ export interface OAuthCompleteInput { readonly callbackDomain?: string | null; } +/** Host-lifecycle behavior for OAuth completion. The HTTP popup uses + * background tool synchronization so it can close after the durable grant; + * programmatic callers keep the default explicit catalog guarantee. */ +export interface OAuthCompleteOptions { + readonly toolSync?: "explicit" | "background"; +} + /** Probe a base/issuer URL for OAuth 2.1 authorization-server metadata so the * onboarding UI can pre-fill a client's endpoints. */ export interface OAuthProbeInput { @@ -496,6 +503,7 @@ export interface OAuthService { ) => Effect.Effect; readonly complete: ( input: OAuthCompleteInput, + options?: OAuthCompleteOptions, ) => Effect.Effect; readonly cancel: (state: OAuthState) => Effect.Effect; readonly probe: ( diff --git a/packages/core/sdk/src/oauth-flow.test.ts b/packages/core/sdk/src/oauth-flow.test.ts index a4f7aff41..fbe90664c 100644 --- a/packages/core/sdk/src/oauth-flow.test.ts +++ b/packages/core/sdk/src/oauth-flow.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "@effect/vitest"; -import { Deferred, Effect, Fiber, Predicate } from "effect"; +import { Deferred, Effect, Fiber, Option, Predicate } from "effect"; import { withQueryContext } from "@executor-js/fumadb/query"; import { @@ -264,6 +264,93 @@ describe("oauth.start / oauth.complete", () => { ), ); + it.effect("complete returns after the durable grant while remote tool discovery continues", () => + Effect.scoped( + Effect.gen(function* () { + const discoveryStarted = yield* Deferred.make(); + const releaseDiscovery = yield* Deferred.make(); + const keptAlive: Promise[] = []; + const slowOAuthPlugin = definePlugin(() => ({ + id: "acme" as const, + storage: () => ({}), + resolveTools: () => + Effect.gen(function* () { + yield* Deferred.succeed(discoveryStarted, undefined); + yield* Deferred.await(releaseDiscovery); + return { + tools: [{ name: ToolName.make("whoami"), description: "whoami" }], + }; + }), + describeAuthMethods: () => [ + { + id: "oauth", + label: "OAuth2", + kind: "oauth" as const, + template: String(TEMPLATE), + oauth: { scopes: ["read"] }, + }, + ], + invokeTool: ({ credential }) => Effect.succeed({ token: credential.value }), + extension: (ctx) => ({ + seed: () => + ctx.core.integrations.register({ + slug: INTEG, + description: "Slow Acme", + config: {}, + }), + }), + }))(); + const server = yield* serveOAuthTestServer({ scopes: ["read"] }); + const { executor } = yield* makeTestWorkspaceHarness({ + plugins: [memoryCredentialsPlugin(), slowOAuthPlugin] as const, + waitUntil: (promise) => keptAlive.push(promise), + }); + yield* executor.acme.seed(); + + yield* executor.oauth.createClient({ + owner: "org", + slug: CLIENT, + authorizationUrl: server.authorizationEndpoint, + tokenUrl: server.tokenEndpoint, + grant: "authorization_code", + clientId: "test-client", + clientSecret: "test-secret", + }); + const started = yield* executor.oauth.start({ + owner: "org", + client: CLIENT, + clientOwner: "org", + name: ConnectionName.make("main-account"), + integration: INTEG, + template: TEMPLATE, + }); + expect(started.status).toBe("redirect"); + if (started.status !== "redirect") return; + const callback = yield* server.completeAuthorizationCodeFlow({ + authorizationUrl: started.authorizationUrl, + }); + + const completed = yield* executor.oauth + .complete({ state: started.state, code: callback.code }, { toolSync: "background" }) + .pipe(Effect.timeoutOption("1 second")); + expect( + Option.isSome(completed), + "the callback returns while listTools remains deliberately blocked", + ).toBe(true); + expect(keptAlive).toHaveLength(1); + yield* Deferred.await(discoveryStarted); + + const connections = yield* executor.connections.list({ integration: INTEG }); + expect(connections.map((connection) => String(connection.name))).toEqual(["mainAccount"]); + + yield* Deferred.succeed(releaseDiscovery, undefined); + yield* Effect.promise(() => Promise.all(keptAlive)); + const tools = yield* executor.tools.list({ integration: INTEG }); + expect(tools.map((tool) => String(tool.name))).toEqual(["whoami"]); + }), + ), + ); + it.effect("carries the URL org selector in provider state without changing redirect_uri", () => Effect.scoped( Effect.gen(function* () { diff --git a/packages/core/sdk/src/oauth-service.ts b/packages/core/sdk/src/oauth-service.ts index 5d7b0ef8a..b0f028432 100644 --- a/packages/core/sdk/src/oauth-service.ts +++ b/packages/core/sdk/src/oauth-service.ts @@ -47,6 +47,7 @@ import { type OAuthClientOrigin, type OAuthClientSummary, type OAuthCompleteInput, + type OAuthCompleteOptions, type OAuthGrant, type OAuthProbeInput, type OAuthProbeResult, @@ -124,6 +125,12 @@ export interface MintOAuthConnectionInput { * code was redeemed at a region other than the client's configured token * host (Datadog multi-site). Null means refresh uses the client's token URL. */ readonly oauthTokenUrl?: string | null; + /** Whether connection tool discovery must finish before the mint returns. + * Interactive authorization-code callbacks persist the fresh grant first, + * then synchronize the remote catalog in host-kept background work so a + * slow MCP server cannot strand the browser popup. Non-interactive grants + * keep the explicit behavior because their caller has no callback window. */ + readonly toolSync?: "explicit" | "background"; } /** Project an enterprise-managed mint failure onto the connect boundary, @@ -1827,6 +1834,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { // ----------------------------------------------------------------------- const complete = ( input: OAuthCompleteInput, + options?: OAuthCompleteOptions, ): Effect.Effect => Effect.gen(function* () { const sessionRow = yield* deps.fuma.use("oauth_session.findFirst", (db) => @@ -1961,6 +1969,10 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { // Persist the regional token endpoint ONLY when it differs from the // client's configured one, so refresh redeems against the same region. tokenUrl === client.tokenUrl ? null : tokenUrl, + // The grant and connection row are the callback's durable contract. + // Remote catalog discovery can be arbitrarily slow and must not keep + // the popup waiting after that contract has committed. + options?.toolSync ?? "explicit", ).pipe( Effect.mapError( (cause) => @@ -2035,6 +2047,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { /** Regional token endpoint override to persist when the code was redeemed * off the client's configured host; null to use the client's token URL. */ oauthTokenUrl: string | null, + toolSync: "explicit" | "background" = "explicit", ): Effect.Effect => Effect.gen(function* () { const provider = deps.defaultWritableProvider(); @@ -2095,6 +2108,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { oauthScope, missingOAuthScopes: missingScopes, oauthTokenUrl, + toolSync, }); }); diff --git a/packages/core/sdk/src/test-config.ts b/packages/core/sdk/src/test-config.ts index cbb64e1da..d890b245c 100644 --- a/packages/core/sdk/src/test-config.ts +++ b/packages/core/sdk/src/test-config.ts @@ -134,6 +134,7 @@ export type TestConfigOptions["onIntegrationChange"]; readonly firstPartyOAuthClients?: ExecutorConfig["firstPartyOAuthClients"]; readonly enterpriseManagedRollout?: ExecutorConfig["enterpriseManagedRollout"]; + readonly waitUntil?: ExecutorConfig["waitUntil"]; }; export const makeTestConfig = ( @@ -176,6 +177,7 @@ export const makeTestConfig = Effect.Effect; readonly authorizationServerUrls?: readonly string[]; @@ -173,6 +176,14 @@ export const serveMcpServer = (factory: () => McpServer, options: McpTestServerO writeUnauthorized(response, origin); return; } + if (options.authenticatedRequestDelayMs !== undefined) { + yield* Effect.promise( + () => + new Promise((resolve) => + setTimeout(resolve, options.authenticatedRequestDelayMs), + ), + ); + } } if (sessionId && request.method === "POST" && nextSessionRequestStatus !== undefined) { @@ -344,6 +355,7 @@ export const serveMcpServerWithOAuth = ( const oauth = yield* OAuthTestServer; return yield* serveMcpServer(factory, { path: options.path, + authenticatedRequestDelayMs: options.authenticatedRequestDelayMs, auth: { validateAuthorization: oauth.acceptsAuthorizationHeader, authorizationServerUrls: [oauth.issuerUrl],