From c62c39edd644444d0169fe2047bba6f83f7adc85 Mon Sep 17 00:00:00 2001 From: Saatvik Arya Date: Sun, 30 Aug 2026 16:22:09 +0530 Subject: [PATCH] fix(oauth): complete browser callbacks before tool sync Persist the refreshed OAuth grant and connection before returning the popup callback, then keep remote catalog synchronization alive through the host lifecycle. Preserve synchronous completion for programmatic callers and cover slow MCP discovery with unit and browser E2E tests. --- ...mcp-oauth-callback-background-sync.test.ts | 112 ++++++++++++++++++ packages/core/api/src/handlers/oauth.ts | 17 +-- packages/core/sdk/src/executor.ts | 43 ++++++- packages/core/sdk/src/oauth-client.ts | 8 ++ packages/core/sdk/src/oauth-flow.test.ts | 89 +++++++++++++- packages/core/sdk/src/oauth-service.ts | 14 +++ packages/core/sdk/src/test-config.ts | 2 + packages/plugins/mcp/src/testing/server.ts | 12 ++ 8 files changed, 286 insertions(+), 11 deletions(-) create mode 100644 e2e/selfhost/mcp-oauth-callback-background-sync.test.ts 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 0000000000..eef038e3c9 --- /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 058ff4e8ab..eb4b1f939b 100644 --- a/packages/core/api/src/handlers/oauth.ts +++ b/packages/core/api/src/handlers/oauth.ts @@ -212,13 +212,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 6916233cb5..d70ae691cb 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -4453,6 +4453,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; + } }), ); diff --git a/packages/core/sdk/src/oauth-client.ts b/packages/core/sdk/src/oauth-client.ts index 7330ad753b..8778205014 100644 --- a/packages/core/sdk/src/oauth-client.ts +++ b/packages/core/sdk/src/oauth-client.ts @@ -376,6 +376,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 { @@ -535,6 +542,7 @@ export interface OAuthService { ) => Effect.Effect; readonly complete: ( input: OAuthCompleteInput, + options?: OAuthCompleteOptions, ) => Effect.Effect< Connection, OAuthCompleteError | OAuthSessionNotFoundError | OrgWriteDeniedError | StorageFailure diff --git a/packages/core/sdk/src/oauth-flow.test.ts b/packages/core/sdk/src/oauth-flow.test.ts index b9860574bb..d818d24644 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("persists HTTP Basic client auth for code exchange and refresh", () => Effect.scoped( Effect.gen(function* () { diff --git a/packages/core/sdk/src/oauth-service.ts b/packages/core/sdk/src/oauth-service.ts index dd443144bd..99ab41c26c 100644 --- a/packages/core/sdk/src/oauth-service.ts +++ b/packages/core/sdk/src/oauth-service.ts @@ -60,6 +60,7 @@ import { type OAuthClientOrigin, type OAuthClientSummary, type OAuthCompleteInput, + type OAuthCompleteOptions, type OAuthGrant, type OAuthProbeInput, type OAuthProbeResult, @@ -149,6 +150,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, @@ -2078,6 +2085,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { // ----------------------------------------------------------------------- const complete = ( input: OAuthCompleteInput, + options?: OAuthCompleteOptions, ): Effect.Effect< Connection, OAuthCompleteError | OAuthSessionNotFoundError | OrgWriteDeniedError | StorageFailure @@ -2215,6 +2223,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) => Predicate.isTagged(cause, "OrgWriteDeniedError") @@ -2290,6 +2302,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* () { // The token exchange may outlive the role that admitted `start`. Re-read @@ -2357,6 +2370,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 17b8fbc321..df32bcda0f 100644 --- a/packages/core/sdk/src/test-config.ts +++ b/packages/core/sdk/src/test-config.ts @@ -138,6 +138,7 @@ export type TestConfigOptions["orgWrites"]; + readonly waitUntil?: ExecutorConfig["waitUntil"]; }; export const makeTestConfig = ( @@ -181,6 +182,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],