From fa62d2a38172a48b564f6af45d3166516edc7647 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Tue, 8 Sep 2026 14:35:10 -0700 Subject: [PATCH 01/10] Refresh OAuth tokens before browser re-auth and cap prompts Linear MCP servers periodically disconnect and every auth failure fell through to a browser re-auth prompt. The provider now performs a refresh_token grant against discovered authorization server metadata before any browser flow, and browser prompts are capped at three per server with a five minute cooldown, surfacing a clear error through the connect failure path instead of looping forever. --- src/mcp/client-auth-reauth-cap.test.ts | 185 +++++++++++++++++++++++++ src/mcp/client.ts | 73 +++++++++- src/mcp/oauth-provider.ts | 41 +++++- 3 files changed, 296 insertions(+), 3 deletions(-) create mode 100644 src/mcp/client-auth-reauth-cap.test.ts diff --git a/src/mcp/client-auth-reauth-cap.test.ts b/src/mcp/client-auth-reauth-cap.test.ts new file mode 100644 index 000000000..6f7788b76 --- /dev/null +++ b/src/mcp/client-auth-reauth-cap.test.ts @@ -0,0 +1,185 @@ +import { beforeEach, describe, expect, test } from "bun:test"; +import { UnauthorizedError } from "@modelcontextprotocol/sdk/client/auth.js"; +import { withMockedModule } from "../../tests/helpers/mock-module.js"; + +let finishAuthCalls = 0; +let connectFailuresLeft = 0; +let providerCreates = 0; +let refreshCalls = 0; +let refreshSucceeds = false; +let authEvents: string[] = []; + +const fakeProvider = { + resetAuthorization: async () => undefined, + redirectToAuthorization: (url: URL) => { + authEvents.push("authURL"); + void url; + }, + tokens: () => ({ access_token: "stale", refresh_token: "refresh-me" }), + refreshToken: async () => { + refreshCalls += 1; + authEvents.push("refresh"); + if (!refreshSucceeds) throw new UnauthorizedError("refresh rejected"); + return { access_token: "fresh", refresh_token: "refresh-me" }; + }, +}; + +await withMockedModule( + import.meta.resolve("@modelcontextprotocol/sdk/client/index.js"), + (real: typeof import("@modelcontextprotocol/sdk/client/index.js")) => ({ + ...real, + Client: class { + async connect(): Promise { + if (connectFailuresLeft > 0) { + connectFailuresLeft -= 1; + throw new UnauthorizedError("authorization required"); + } + } + async listTools(): Promise<{ tools: [] }> { + return { tools: [] }; + } + async close(): Promise {} + }, + }), +); + +await withMockedModule( + import.meta.resolve("@modelcontextprotocol/sdk/client/streamableHttp.js"), + (real: typeof import("@modelcontextprotocol/sdk/client/streamableHttp.js")) => ({ + ...real, + StreamableHTTPClientTransport: class { + provider?: { redirectToAuthorization?: (url: URL) => void } | undefined; + constructor( + _url: URL, + options?: { authProvider?: { redirectToAuthorization?: (url: URL) => void } | undefined }, + ) { + this.provider = options?.authProvider; + } + async finishAuth(): Promise { + // The real SDK transport emits the browser URL from auth(); emulate the + // prompt here so tests can order it against the refresh attempt. + this.provider?.redirectToAuthorization?.(new URL("https://auth.test/authorize")); + finishAuthCalls += 1; + throw new Error("finishAuth exploded"); + } + get sessionId(): string | undefined { + return undefined; + } + }, + }), +); + +await withMockedModule( + import.meta.resolve("./callback-server.js"), + (real: typeof import("./callback-server.js")) => ({ + ...real, + startCallbackServer: async () => ({ + redirectUrl: "http://127.0.0.1:12345/callback", + expectState: () => undefined, + waitForCode: async () => "code", + close: () => undefined, + }), + }), +); + +await withMockedModule( + import.meta.resolve("./oauth-provider.js"), + (real: typeof import("./oauth-provider.js")) => ({ + ...real, + createOAuthProvider: async () => { + providerCreates += 1; + return fakeProvider; + }, + }), +); + +const { connectMCPServer, resetBrowserAuthState, MAX_BROWSER_AUTH_ATTEMPTS } = + await import("./client.js"); + +const config = { name: "linear", type: "http" as const, url: "https://mcp.linear.app/mcp" }; + +async function connectWithAuthPrompt(): Promise<{ ok: boolean; error?: string }> { + const result = await connectMCPServer(config, { + onAuthURL: () => { + authEvents.push("authURL"); + }, + }); + return result.ok ? { ok: true } : { ok: false, error: result.error }; +} + +describe("HTTP MCP re-auth loop prevention", () => { + beforeEach(() => { + connectFailuresLeft = 0; + finishAuthCalls = 0; + providerCreates = 0; + refreshCalls = 0; + refreshSucceeds = false; + authEvents = []; + resetBrowserAuthState(); + }); + + test("refreshes tokens before any browser re-auth prompt", async () => { + refreshSucceeds = true; + connectFailuresLeft = 1; + + const result = await connectWithAuthPrompt(); + + expect(result.ok).toBe(true); + expect(authEvents).toEqual(["refresh"]); + expect(refreshCalls).toBe(1); + expect(finishAuthCalls).toBe(0); + }); + + test("a failed refresh still precedes the browser prompt", async () => { + connectFailuresLeft = Number.POSITIVE_INFINITY; + // finishAuth always throws, so the episode ends at the failed exchange. + const result = await connectWithAuthPrompt(); + expect(result.ok).toBe(false); + expect(result.error).toContain("finishAuth exploded"); + + expect(authEvents).toEqual(["refresh", "authURL"]); + expect(refreshCalls).toBe(1); + }); + + test("failed browser re-auth is capped and pauses re-prompting across episodes", async () => { + connectFailuresLeft = Number.POSITIVE_INFINITY; + + for (let episode = 0; episode < MAX_BROWSER_AUTH_ATTEMPTS; episode += 1) { + const result = await connectWithAuthPrompt(); + expect(result.ok).toBe(false); + expect(result.error).toContain("finishAuth exploded"); + } + + for (let episode = 0; episode < 2; episode += 1) { + const result = await connectWithAuthPrompt(); + expect(result.ok).toBe(false); + expect(result.error).toContain( + `MCP authorization for linear failed after ${MAX_BROWSER_AUTH_ATTEMPTS} attempts`, + ); + expect(result.error).toContain("retrying paused for 5 minutes"); + } + + expect(finishAuthCalls).toBe(MAX_BROWSER_AUTH_ATTEMPTS); + expect(providerCreates).toBeGreaterThan(MAX_BROWSER_AUTH_ATTEMPTS); + }); + + test("resetBrowserAuthState clears the cap within the process", async () => { + connectFailuresLeft = Number.POSITIVE_INFINITY; + for (let episode = 0; episode < MAX_BROWSER_AUTH_ATTEMPTS; episode += 1) { + const result = await connectWithAuthPrompt(); + expect(result.ok).toBe(false); + expect(result.error).toContain("finishAuth exploded"); + } + expect(await connectWithAuthPrompt()).toEqual({ + ok: false, + error: expect.stringContaining("retrying paused"), + }); + + resetBrowserAuthState(); + const promptsBefore = finishAuthCalls; + const result = await connectWithAuthPrompt(); + expect(result.ok).toBe(false); + expect(result.error).toContain("finishAuth exploded"); + expect(finishAuthCalls).toBe(promptsBefore + 1); + }); +}); diff --git a/src/mcp/client.ts b/src/mcp/client.ts index 9da4b4e16..10e71a120 100644 --- a/src/mcp/client.ts +++ b/src/mcp/client.ts @@ -72,6 +72,61 @@ function isRecoverableAuthError(err: unknown): boolean { return err instanceof UnauthorizedError || err instanceof OAuthError; } +export const MAX_BROWSER_AUTH_ATTEMPTS = 3; +export const BROWSER_AUTH_COOLDOWN_MS = 5 * 60_000; + +interface BrowserAuthAttempts { + count: number; + cooldownUntil?: number | undefined; +} +// Keyed by server identity, not provider instance, so the cap survives the +// provider re-creation that every reconnect performs. +const browserAuthAttempts = new Map(); + +export function resetBrowserAuthState(): void { + browserAuthAttempts.clear(); +} + +function browserAuthCapError(serverName: string): Error { + const minutes = Math.round(BROWSER_AUTH_COOLDOWN_MS / 60_000); + return new Error( + `MCP authorization for ${serverName} failed after ${MAX_BROWSER_AUTH_ATTEMPTS} attempts; ` + + `retrying paused for ${minutes} minutes. Reconnect the server to try again.`, + ); +} + +function beginBrowserAuth(context: HTTPAuthContext): { clear(): void } { + const key = `${context.serverName}|${context.url.toString()}`; + const entry = browserAuthAttempts.get(key) ?? { count: 0 }; + const now = Date.now(); + if (entry.cooldownUntil !== undefined) { + if (now < entry.cooldownUntil) throw browserAuthCapError(context.serverName); + entry.cooldownUntil = undefined; + entry.count = 0; + } + if (entry.count >= MAX_BROWSER_AUTH_ATTEMPTS) { + entry.cooldownUntil = now + BROWSER_AUTH_COOLDOWN_MS; + throw browserAuthCapError(context.serverName); + } + entry.count += 1; + browserAuthAttempts.set(key, entry); + return { + clear: () => browserAuthAttempts.delete(key), + }; +} + +async function tryTokenRefresh(context: HTTPAuthContext): Promise { + const refreshToken = (await context.authProvider.tokens?.())?.refresh_token; + if (refreshToken === undefined) return false; + try { + const tokens = await context.authProvider.refreshToken?.(refreshToken); + return tokens !== undefined; + } catch { + // Refresh failure is auth-invalid; the browser flow remains the fallback. + return false; + } +} + /** * Fetch that always attaches the connect AbortSignal. SDK 403 upscoping calls * `auth()` with raw `_fetch` (no `requestInit.signal`); `_fetchWithInit` still @@ -137,16 +192,32 @@ async function recoverHTTPAuthorization( ): Promise { if (context === undefined || !isRecoverableAuthError(err)) throw err; let lastErr: unknown = err; + let refreshAttempted = false; for (let attempt = 0; attempt < 2; attempt += 1) { if (lastErr instanceof OAuthError) await context.authProvider.resetAuthorization(); if (lastErr instanceof UnauthorizedError) { - return retryAfterInteractiveAuth( + if (!refreshAttempted) { + refreshAttempted = true; + if (await tryTokenRefresh(context)) { + try { + return await operation(); + } catch (nextErr) { + if (!isRecoverableAuthError(nextErr)) throw nextErr; + lastErr = nextErr; + continue; + } + } + } + const guard = beginBrowserAuth(context); + const value = await retryAfterInteractiveAuth( () => completeInteractiveAuth(context), operation, context.onAuthorized === undefined ? undefined : () => context.onAuthorized?.(context.serverName), ); + guard.clear(); + return value; } try { return await operation(); diff --git a/src/mcp/oauth-provider.ts b/src/mcp/oauth-provider.ts index 7c58b2b1a..15db63832 100644 --- a/src/mcp/oauth-provider.ts +++ b/src/mcp/oauth-provider.ts @@ -1,7 +1,13 @@ import { statSync } from "node:fs"; import { homedir } from "node:os"; -import type { OAuthClientProvider } from "@modelcontextprotocol/sdk/client/auth.js"; +import { + discoverAuthorizationServerMetadata, + refreshAuthorization, + UnauthorizedError, + type OAuthClientProvider, +} from "@modelcontextprotocol/sdk/client/auth.js"; import type { + AuthorizationServerMetadata, OAuthClientInformationFull, OAuthClientInformationMixed, OAuthClientMetadata, @@ -24,7 +30,10 @@ export interface OAuthProviderOptions { onAuthorizationState?: (state: string) => void; home?: string; } -export type CorbitsOAuthProvider = OAuthClientProvider & { resetAuthorization(): Promise }; +export type CorbitsOAuthProvider = OAuthClientProvider & { + resetAuthorization(): Promise; + refreshToken(refreshToken: string): Promise; +}; function redirectUrisInclude( info: OAuthClientInformationFull | undefined, @@ -144,6 +153,33 @@ export async function createOAuthProvider( }; let oauthState: string | undefined; + // The SDK consults provider.refreshToken? only in newer versions; 1.30.0 never + // does, so client.ts calls this before any browser re-auth. Discovery runs + // once per provider; failures are auth-invalid, not refresh-retryable. + let authorizationServerMetadata: AuthorizationServerMetadata | undefined; + const refreshToken = async (refreshToken: string): Promise => { + try { + authorizationServerMetadata ??= + (await discoverAuthorizationServerMetadata(opts.serverURL)) ?? undefined; + if (authorizationServerMetadata === undefined) + throw new Error("authorization server metadata unavailable"); + const clientInformation = stored.clientInformation; + if (clientInformation === undefined) throw new Error("no client registration to refresh"); + const tokens = await refreshAuthorization(opts.serverURL, { + metadata: authorizationServerMetadata, + clientInformation, + refreshToken, + }); + await apply((state) => { + state.tokens = tokens; + }); + return tokens; + } catch (err) { + throw new UnauthorizedError( + `Token refresh failed for ${opts.serverName}: ${err instanceof Error ? err.message : String(err)}`, + ); + } + }; return { get redirectUrl(): string { return opts.redirectUrl; @@ -208,5 +244,6 @@ export async function createOAuthProvider( }); delete stored.codeVerifier; }, + refreshToken, }; } From df6c19fb3dc83cf3c087553a9cf0a098ff386925 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Tue, 8 Sep 2026 22:22:00 -0700 Subject: [PATCH 02/10] Gate MCP browser prompts at redirect and pass resource on refresh --- src/mcp/client-auth-policy.test.ts | 5 +- src/mcp/client-auth-reauth-cap.test.ts | 124 ++++++++++++++--- src/mcp/client.ts | 54 +++++-- src/mcp/oauth-provider.test.ts | 186 +++++++++++++++++++++++++ src/mcp/oauth-provider.ts | 86 ++++++++---- 5 files changed, 396 insertions(+), 59 deletions(-) diff --git a/src/mcp/client-auth-policy.test.ts b/src/mcp/client-auth-policy.test.ts index 54bc14f0b..fb828e412 100644 --- a/src/mcp/client-auth-policy.test.ts +++ b/src/mcp/client-auth-policy.test.ts @@ -35,7 +35,10 @@ function hangUntilAbort( }); } -const authProvider = { resetAuthorization: async () => undefined }; +const authProvider = { + resetAuthorization: async () => undefined, + redirectToAuthorization: () => undefined, +}; await withMockedModule( import.meta.resolve("@modelcontextprotocol/sdk/client/index.js"), diff --git a/src/mcp/client-auth-reauth-cap.test.ts b/src/mcp/client-auth-reauth-cap.test.ts index 6f7788b76..d2fd8a070 100644 --- a/src/mcp/client-auth-reauth-cap.test.ts +++ b/src/mcp/client-auth-reauth-cap.test.ts @@ -1,20 +1,18 @@ -import { beforeEach, describe, expect, test } from "bun:test"; +import { afterEach, beforeEach, describe, expect, setSystemTime, test } from "bun:test"; import { UnauthorizedError } from "@modelcontextprotocol/sdk/client/auth.js"; import { withMockedModule } from "../../tests/helpers/mock-module.js"; let finishAuthCalls = 0; +let finishAuthError: Error | undefined = new Error("finishAuth exploded"); let connectFailuresLeft = 0; let providerCreates = 0; let refreshCalls = 0; let refreshSucceeds = false; let authEvents: string[] = []; +let authURLCount = 0; const fakeProvider = { resetAuthorization: async () => undefined, - redirectToAuthorization: (url: URL) => { - authEvents.push("authURL"); - void url; - }, tokens: () => ({ access_token: "stale", refresh_token: "refresh-me" }), refreshToken: async () => { refreshCalls += 1; @@ -29,9 +27,18 @@ await withMockedModule( (real: typeof import("@modelcontextprotocol/sdk/client/index.js")) => ({ ...real, Client: class { - async connect(): Promise { + async connect(transport?: { + provider?: { + redirectToAuthorization?: (url: URL) => void | Promise; + }; + }): Promise { if (connectFailuresLeft > 0) { connectFailuresLeft -= 1; + // Production StreamableHTTPClientTransport calls SDK auth() on HTTP 401, + // which invokes redirectToAuthorization and then throws UnauthorizedError. + await transport?.provider?.redirectToAuthorization?.( + new URL("https://auth.test/authorize"), + ); throw new UnauthorizedError("authorization required"); } } @@ -48,19 +55,22 @@ await withMockedModule( (real: typeof import("@modelcontextprotocol/sdk/client/streamableHttp.js")) => ({ ...real, StreamableHTTPClientTransport: class { - provider?: { redirectToAuthorization?: (url: URL) => void } | undefined; + provider?: { + redirectToAuthorization?: (url: URL) => void | Promise; + }; constructor( _url: URL, - options?: { authProvider?: { redirectToAuthorization?: (url: URL) => void } | undefined }, + options?: { + authProvider?: { + redirectToAuthorization?: (url: URL) => void | Promise; + }; + }, ) { - this.provider = options?.authProvider; + if (options?.authProvider !== undefined) this.provider = options.authProvider; } async finishAuth(): Promise { - // The real SDK transport emits the browser URL from auth(); emulate the - // prompt here so tests can order it against the refresh attempt. - this.provider?.redirectToAuthorization?.(new URL("https://auth.test/authorize")); finishAuthCalls += 1; - throw new Error("finishAuth exploded"); + if (finishAuthError !== undefined) throw finishAuthError; } get sessionId(): string | undefined { return undefined; @@ -86,21 +96,34 @@ await withMockedModule( import.meta.resolve("./oauth-provider.js"), (real: typeof import("./oauth-provider.js")) => ({ ...real, - createOAuthProvider: async () => { + createOAuthProvider: async (options: { + serverName: string; + onAuthURL: (serverName: string, authorizationUrl: string) => void; + }) => { providerCreates += 1; - return fakeProvider; + return { + ...fakeProvider, + redirectToAuthorization: (url: URL) => { + options.onAuthURL(options.serverName, url.toString()); + }, + }; }, }), ); -const { connectMCPServer, resetBrowserAuthState, MAX_BROWSER_AUTH_ATTEMPTS } = - await import("./client.js"); +const { + connectMCPServer, + resetBrowserAuthState, + MAX_BROWSER_AUTH_ATTEMPTS, + BROWSER_AUTH_COOLDOWN_MS, +} = await import("./client.js"); const config = { name: "linear", type: "http" as const, url: "https://mcp.linear.app/mcp" }; async function connectWithAuthPrompt(): Promise<{ ok: boolean; error?: string }> { const result = await connectMCPServer(config, { onAuthURL: () => { + authURLCount += 1; authEvents.push("authURL"); }, }); @@ -111,11 +134,18 @@ describe("HTTP MCP re-auth loop prevention", () => { beforeEach(() => { connectFailuresLeft = 0; finishAuthCalls = 0; + finishAuthError = new Error("finishAuth exploded"); providerCreates = 0; refreshCalls = 0; refreshSucceeds = false; authEvents = []; + authURLCount = 0; resetBrowserAuthState(); + setSystemTime(); + }); + + afterEach(() => { + setSystemTime(); }); test("refreshes tokens before any browser re-auth prompt", async () => { @@ -128,17 +158,18 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(authEvents).toEqual(["refresh"]); expect(refreshCalls).toBe(1); expect(finishAuthCalls).toBe(0); + expect(authURLCount).toBe(0); }); test("a failed refresh still precedes the browser prompt", async () => { connectFailuresLeft = Number.POSITIVE_INFINITY; - // finishAuth always throws, so the episode ends at the failed exchange. const result = await connectWithAuthPrompt(); expect(result.ok).toBe(false); expect(result.error).toContain("finishAuth exploded"); expect(authEvents).toEqual(["refresh", "authURL"]); expect(refreshCalls).toBe(1); + expect(authURLCount).toBe(1); }); test("failed browser re-auth is capped and pauses re-prompting across episodes", async () => { @@ -149,6 +180,8 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(result.ok).toBe(false); expect(result.error).toContain("finishAuth exploded"); } + expect(authURLCount).toBe(MAX_BROWSER_AUTH_ATTEMPTS); + expect(finishAuthCalls).toBe(MAX_BROWSER_AUTH_ATTEMPTS); for (let episode = 0; episode < 2; episode += 1) { const result = await connectWithAuthPrompt(); @@ -157,12 +190,65 @@ describe("HTTP MCP re-auth loop prevention", () => { `MCP authorization for linear failed after ${MAX_BROWSER_AUTH_ATTEMPTS} attempts`, ); expect(result.error).toContain("retrying paused for 5 minutes"); + expect(result.error).toContain("Retry later after the cooldown"); + expect(result.error).not.toContain("Reconnect the server"); } + expect(authURLCount).toBe(MAX_BROWSER_AUTH_ATTEMPTS); expect(finishAuthCalls).toBe(MAX_BROWSER_AUTH_ATTEMPTS); expect(providerCreates).toBeGreaterThan(MAX_BROWSER_AUTH_ATTEMPTS); }); + test("successful interactive auth clears the cap so a later failure can prompt", async () => { + connectFailuresLeft = Number.POSITIVE_INFINITY; + for (let episode = 0; episode < 2; episode += 1) { + const result = await connectWithAuthPrompt(); + expect(result.ok).toBe(false); + expect(result.error).toContain("finishAuth exploded"); + } + expect(authURLCount).toBe(2); + + finishAuthError = undefined; + connectFailuresLeft = 1; + const recovered = await connectWithAuthPrompt(); + expect(recovered.ok).toBe(true); + expect(authURLCount).toBe(3); + + finishAuthError = new Error("finishAuth exploded"); + connectFailuresLeft = Number.POSITIVE_INFINITY; + for (let episode = 0; episode < MAX_BROWSER_AUTH_ATTEMPTS; episode += 1) { + const result = await connectWithAuthPrompt(); + expect(result.ok).toBe(false); + expect(result.error).toContain("finishAuth exploded"); + } + expect(authURLCount).toBe(3 + MAX_BROWSER_AUTH_ATTEMPTS); + + const capped = await connectWithAuthPrompt(); + expect(capped.ok).toBe(false); + expect(capped.error).toContain("retrying paused"); + expect(authURLCount).toBe(3 + MAX_BROWSER_AUTH_ATTEMPTS); + }); + + test("prompts resume after the cooldown", async () => { + connectFailuresLeft = Number.POSITIVE_INFINITY; + for (let episode = 0; episode < MAX_BROWSER_AUTH_ATTEMPTS; episode += 1) { + const result = await connectWithAuthPrompt(); + expect(result.ok).toBe(false); + } + expect(authURLCount).toBe(MAX_BROWSER_AUTH_ATTEMPTS); + + const duringCooldown = await connectWithAuthPrompt(); + expect(duringCooldown.ok).toBe(false); + expect(duringCooldown.error).toContain("retrying paused"); + expect(authURLCount).toBe(MAX_BROWSER_AUTH_ATTEMPTS); + + setSystemTime(Date.now() + BROWSER_AUTH_COOLDOWN_MS + 1); + const afterCooldown = await connectWithAuthPrompt(); + expect(afterCooldown.ok).toBe(false); + expect(afterCooldown.error).toContain("finishAuth exploded"); + expect(authURLCount).toBe(MAX_BROWSER_AUTH_ATTEMPTS + 1); + }); + test("resetBrowserAuthState clears the cap within the process", async () => { connectFailuresLeft = Number.POSITIVE_INFINITY; for (let episode = 0; episode < MAX_BROWSER_AUTH_ATTEMPTS; episode += 1) { @@ -174,6 +260,7 @@ describe("HTTP MCP re-auth loop prevention", () => { ok: false, error: expect.stringContaining("retrying paused"), }); + expect(authURLCount).toBe(MAX_BROWSER_AUTH_ATTEMPTS); resetBrowserAuthState(); const promptsBefore = finishAuthCalls; @@ -181,5 +268,6 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(result.ok).toBe(false); expect(result.error).toContain("finishAuth exploded"); expect(finishAuthCalls).toBe(promptsBefore + 1); + expect(authURLCount).toBe(MAX_BROWSER_AUTH_ATTEMPTS + 1); }); }); diff --git a/src/mcp/client.ts b/src/mcp/client.ts index 10e71a120..5536aded8 100644 --- a/src/mcp/client.ts +++ b/src/mcp/client.ts @@ -66,6 +66,8 @@ interface HTTPAuthContext { interactive: boolean; serverName: string; onAuthorized?: (serverName: string) => void; + promptGuard?: { clear(): void }; + refreshedWithoutPrompt?: boolean; } function isRecoverableAuthError(err: unknown): boolean { @@ -91,7 +93,7 @@ function browserAuthCapError(serverName: string): Error { const minutes = Math.round(BROWSER_AUTH_COOLDOWN_MS / 60_000); return new Error( `MCP authorization for ${serverName} failed after ${MAX_BROWSER_AUTH_ATTEMPTS} attempts; ` + - `retrying paused for ${minutes} minutes. Reconnect the server to try again.`, + `retrying paused for ${minutes} minutes. Retry later after the cooldown.`, ); } @@ -119,7 +121,7 @@ async function tryTokenRefresh(context: HTTPAuthContext): Promise { const refreshToken = (await context.authProvider.tokens?.())?.refresh_token; if (refreshToken === undefined) return false; try { - const tokens = await context.authProvider.refreshToken?.(refreshToken); + const tokens = await context.authProvider.refreshToken(refreshToken); return tokens !== undefined; } catch { // Refresh failure is auth-invalid; the browser flow remains the fallback. @@ -127,6 +129,18 @@ async function tryTokenRefresh(context: HTTPAuthContext): Promise { } } +function gateRedirectToAuthorization(context: HTTPAuthContext): void { + const inner = context.authProvider.redirectToAuthorization.bind(context.authProvider); + context.authProvider.redirectToAuthorization = async (authorizationUrl: URL) => { + if (await tryTokenRefresh(context)) { + context.refreshedWithoutPrompt = true; + return; + } + context.promptGuard ??= beginBrowserAuth(context); + await inner(authorizationUrl); + }; +} + /** * Fetch that always attaches the connect AbortSignal. SDK 403 upscoping calls * `auth()` with raw `_fetch` (no `requestInit.signal`); `_fetchWithInit` still @@ -196,19 +210,32 @@ async function recoverHTTPAuthorization( for (let attempt = 0; attempt < 2; attempt += 1) { if (lastErr instanceof OAuthError) await context.authProvider.resetAuthorization(); if (lastErr instanceof UnauthorizedError) { - if (!refreshAttempted) { - refreshAttempted = true; - if (await tryTokenRefresh(context)) { - try { - return await operation(); - } catch (nextErr) { - if (!isRecoverableAuthError(nextErr)) throw nextErr; - lastErr = nextErr; - continue; + if (context.refreshedWithoutPrompt === true) { + context.refreshedWithoutPrompt = false; + try { + return await operation(); + } catch (nextErr) { + if (!isRecoverableAuthError(nextErr)) throw nextErr; + lastErr = nextErr; + continue; + } + } + if (context.promptGuard === undefined) { + if (!refreshAttempted) { + refreshAttempted = true; + if (await tryTokenRefresh(context)) { + try { + return await operation(); + } catch (nextErr) { + if (!isRecoverableAuthError(nextErr)) throw nextErr; + lastErr = nextErr; + continue; + } } } + context.promptGuard = beginBrowserAuth(context); } - const guard = beginBrowserAuth(context); + const guard = context.promptGuard; const value = await retryAfterInteractiveAuth( () => completeInteractiveAuth(context), operation, @@ -217,6 +244,7 @@ async function recoverHTTPAuthorization( : () => context.onAuthorized?.(context.serverName), ); guard.clear(); + delete context.promptGuard; return value; } try { @@ -334,6 +362,7 @@ async function connectHttp( redirectUrl: callback.redirectUrl, onAuthURL: (name, authUrl) => options.onAuthURL?.(name, authUrl), onAuthorizationState: callback.expectState, + ...(options.signal === undefined ? {} : { fetchFn: fetchWithConnectAbort(options.signal) }), }); makeTransport = () => new StreamableHTTPClientTransport( @@ -349,6 +378,7 @@ async function connectHttp( ...(options.onAuthorized !== undefined ? { onAuthorized: options.onAuthorized } : {}), ...(options.signal !== undefined ? { signal: options.signal } : {}), }; + gateRedirectToAuthorization(authContext); } const connectedClient = new Client({ name: MCP_CLIENT_NAME, version: "1.0.0" }); client = connectedClient; diff --git a/src/mcp/oauth-provider.test.ts b/src/mcp/oauth-provider.test.ts index 0696f8c06..24441e8c8 100644 --- a/src/mcp/oauth-provider.test.ts +++ b/src/mcp/oauth-provider.test.ts @@ -4,7 +4,9 @@ import { readFileSync } from "node:fs"; import { chmod, mkdir, mkdtemp, readFile, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; +import { UnauthorizedError } from "@modelcontextprotocol/sdk/client/auth.js"; import { authFilePath, loadAuthState, saveAuthState, deleteAuthState } from "./auth-store.js"; +import { fetchWithConnectAbort } from "./client.js"; import { createOAuthProvider } from "./oauth-provider.js"; async function tempHome(): Promise { @@ -603,4 +605,188 @@ describe("createOAuthProvider", () => { "scoped-secret", ); }); + + test("refreshToken posts grant_type=refresh_token with a resource and persists tokens", async () => { + const home = await tempHome(); + await saveAuthState( + linear, + { + clientInformation: clientInfo(1), + tokens: { + access_token: "stale", + token_type: "bearer", + refresh_token: "refresh-me", + }, + }, + home, + ); + + const tokenBodies: string[] = []; + const fetchFn = async (url: string | URL, init?: RequestInit): Promise => { + const href = String(url); + if (init?.method === "POST") { + tokenBodies.push(String(init.body)); + return new Response( + JSON.stringify({ + access_token: "fresh-access", + token_type: "bearer", + expires_in: 3600, + refresh_token: "fresh-refresh", + }), + { status: 200, headers: { "content-type": "application/json" } }, + ); + } + if (href.includes("oauth-protected-resource")) { + return new Response( + JSON.stringify({ + resource: "https://mcp.linear.app/mcp", + authorization_servers: ["https://mcp.linear.app"], + }), + { status: 200, headers: { "content-type": "application/json" } }, + ); + } + if (href.includes("oauth-authorization-server") || href.includes("openid-configuration")) { + return new Response( + JSON.stringify({ + issuer: "https://mcp.linear.app", + authorization_endpoint: "https://mcp.linear.app/authorize", + token_endpoint: "https://mcp.linear.app/oauth/token", + response_types_supported: ["code"], + }), + { status: 200, headers: { "content-type": "application/json" } }, + ); + } + return new Response(null, { status: 404 }); + }; + + const provider = await createOAuthProvider({ + serverName: "linear", + serverURL: linear.serverURL, + redirectUrl: "http://127.0.0.1:1/callback", + onAuthURL: () => undefined, + home, + fetchFn, + }); + + const tokens = await provider.refreshToken("refresh-me"); + expect(tokens.access_token).toBe("fresh-access"); + expect(tokenBodies).toHaveLength(1); + const body = tokenBodies[0] ?? ""; + expect(body).toContain("grant_type=refresh_token"); + expect(body).toContain("refresh_token=refresh-me"); + expect(body).toContain(`resource=${encodeURIComponent("https://mcp.linear.app/mcp")}`); + expect((await syncValue(provider.tokens()))?.access_token).toBe("fresh-access"); + expect((await loadAuthState(linear, home)).tokens?.access_token).toBe("fresh-access"); + }); + + test("refreshToken wraps failures as UnauthorizedError", async () => { + const home = await tempHome(); + await saveAuthState(linear, { clientInformation: clientInfo(1) }, home); + + const fetchFn = async (url: string | URL, init?: RequestInit): Promise => { + const href = String(url); + if (init?.method === "POST") { + return new Response(JSON.stringify({ error: "invalid_grant" }), { + status: 400, + headers: { "content-type": "application/json" }, + }); + } + if (href.includes("oauth-protected-resource")) { + return new Response( + JSON.stringify({ + resource: "https://mcp.linear.app/mcp", + authorization_servers: ["https://mcp.linear.app"], + }), + { status: 200, headers: { "content-type": "application/json" } }, + ); + } + if (href.includes("oauth-authorization-server") || href.includes("openid-configuration")) { + return new Response( + JSON.stringify({ + issuer: "https://mcp.linear.app", + authorization_endpoint: "https://mcp.linear.app/authorize", + token_endpoint: "https://mcp.linear.app/oauth/token", + response_types_supported: ["code"], + }), + { status: 200, headers: { "content-type": "application/json" } }, + ); + } + return new Response(null, { status: 404 }); + }; + + const provider = await createOAuthProvider({ + serverName: "linear", + serverURL: linear.serverURL, + redirectUrl: "http://127.0.0.1:1/callback", + onAuthURL: () => undefined, + home, + fetchFn, + }); + + await expect(provider.refreshToken("refresh-me")).rejects.toBeInstanceOf(UnauthorizedError); + await expect(provider.refreshToken("refresh-me")).rejects.toThrow( + "Token refresh failed for linear", + ); + }); + + test("refreshToken fetch honors the connect AbortSignal", async () => { + const home = await tempHome(); + await saveAuthState(linear, { clientInformation: clientInfo(1) }, home); + const abort = new AbortController(); + const seen: (AbortSignal | undefined)[] = []; + const fetchFn = fetchWithConnectAbort(abort.signal, (url, init) => { + seen.push(init?.signal ?? undefined); + const href = String(url); + if (init?.method === "POST") { + return new Promise((_resolve, reject) => { + const fail = (): void => { + reject(init.signal?.reason ?? new Error("aborted")); + }; + if (init.signal?.aborted === true) fail(); + else init.signal?.addEventListener("abort", fail, { once: true }); + }); + } + if (href.includes("oauth-protected-resource")) { + return Promise.resolve( + new Response( + JSON.stringify({ + resource: "https://mcp.linear.app/mcp", + authorization_servers: ["https://mcp.linear.app"], + }), + { status: 200, headers: { "content-type": "application/json" } }, + ), + ); + } + if (href.includes("oauth-authorization-server") || href.includes("openid-configuration")) { + return Promise.resolve( + new Response( + JSON.stringify({ + issuer: "https://mcp.linear.app", + authorization_endpoint: "https://mcp.linear.app/authorize", + token_endpoint: "https://mcp.linear.app/oauth/token", + response_types_supported: ["code"], + }), + { status: 200, headers: { "content-type": "application/json" } }, + ), + ); + } + return Promise.resolve(new Response(null, { status: 404 })); + }); + + const provider = await createOAuthProvider({ + serverName: "linear", + serverURL: linear.serverURL, + redirectUrl: "http://127.0.0.1:1/callback", + onAuthURL: () => undefined, + home, + fetchFn, + }); + + const pending = provider.refreshToken("refresh-me"); + while (seen.every((signal) => signal !== abort.signal)) await Promise.resolve(); + abort.abort(new Error("toolset disposed")); + await expect(pending).rejects.toBeInstanceOf(UnauthorizedError); + await expect(pending).rejects.toThrow("toolset disposed"); + expect(seen.some((signal) => signal === abort.signal)).toBe(true); + }); }); diff --git a/src/mcp/oauth-provider.ts b/src/mcp/oauth-provider.ts index 15db63832..64d625a4b 100644 --- a/src/mcp/oauth-provider.ts +++ b/src/mcp/oauth-provider.ts @@ -2,17 +2,22 @@ import { statSync } from "node:fs"; import { homedir } from "node:os"; import { discoverAuthorizationServerMetadata, + discoverOAuthProtectedResourceMetadata, refreshAuthorization, + selectResourceURL, UnauthorizedError, type OAuthClientProvider, } from "@modelcontextprotocol/sdk/client/auth.js"; +import { resourceUrlFromServerUrl } from "@modelcontextprotocol/sdk/shared/auth-utils.js"; import type { AuthorizationServerMetadata, OAuthClientInformationFull, OAuthClientInformationMixed, OAuthClientMetadata, + OAuthProtectedResourceMetadata, OAuthTokens, } from "@modelcontextprotocol/sdk/shared/auth.js"; +import type { FetchLike } from "@modelcontextprotocol/sdk/shared/transport.js"; import { authFilePath, tryLoadAuthStateSync, @@ -29,6 +34,7 @@ export interface OAuthProviderOptions { onAuthURL: (serverName: string, authorizationUrl: string) => void; onAuthorizationState?: (state: string) => void; home?: string; + fetchFn?: FetchLike; } export type CorbitsOAuthProvider = OAuthClientProvider & { resetAuthorization(): Promise; @@ -153,34 +159,10 @@ export async function createOAuthProvider( }; let oauthState: string | undefined; - // The SDK consults provider.refreshToken? only in newer versions; 1.30.0 never - // does, so client.ts calls this before any browser re-auth. Discovery runs - // once per provider; failures are auth-invalid, not refresh-retryable. let authorizationServerMetadata: AuthorizationServerMetadata | undefined; - const refreshToken = async (refreshToken: string): Promise => { - try { - authorizationServerMetadata ??= - (await discoverAuthorizationServerMetadata(opts.serverURL)) ?? undefined; - if (authorizationServerMetadata === undefined) - throw new Error("authorization server metadata unavailable"); - const clientInformation = stored.clientInformation; - if (clientInformation === undefined) throw new Error("no client registration to refresh"); - const tokens = await refreshAuthorization(opts.serverURL, { - metadata: authorizationServerMetadata, - clientInformation, - refreshToken, - }); - await apply((state) => { - state.tokens = tokens; - }); - return tokens; - } catch (err) { - throw new UnauthorizedError( - `Token refresh failed for ${opts.serverName}: ${err instanceof Error ? err.message : String(err)}`, - ); - } - }; - return { + let authorizationServerUrl: string | undefined; + let resource: URL | undefined; + const provider: CorbitsOAuthProvider = { get redirectUrl(): string { return opts.redirectUrl; }, @@ -244,6 +226,54 @@ export async function createOAuthProvider( }); delete stored.codeVerifier; }, - refreshToken, + refreshToken: async (refreshToken: string): Promise => { + try { + const fetchFn = opts.fetchFn; + if (authorizationServerMetadata === undefined || authorizationServerUrl === undefined) { + let resourceMetadata: OAuthProtectedResourceMetadata | undefined; + try { + resourceMetadata = await discoverOAuthProtectedResourceMetadata( + opts.serverURL, + undefined, + fetchFn, + ); + } catch { + resourceMetadata = undefined; + } + const fromPrm = resourceMetadata?.authorization_servers?.[0]; + authorizationServerUrl = + fromPrm === undefined ? String(new URL("/", opts.serverURL)) : String(fromPrm); + authorizationServerMetadata = + (await discoverAuthorizationServerMetadata( + authorizationServerUrl, + fetchFn === undefined ? {} : { fetchFn }, + )) ?? undefined; + resource = + (await selectResourceURL(opts.serverURL, provider, resourceMetadata)) ?? + resourceUrlFromServerUrl(opts.serverURL); + } + if (authorizationServerMetadata === undefined) + throw new Error("authorization server metadata unavailable"); + const clientInformation = stored.clientInformation; + if (clientInformation === undefined) throw new Error("no client registration to refresh"); + const resourceURL = resource ?? resourceUrlFromServerUrl(opts.serverURL); + const tokens = await refreshAuthorization(authorizationServerUrl, { + metadata: authorizationServerMetadata, + clientInformation, + refreshToken, + resource: resourceURL, + ...(fetchFn === undefined ? {} : { fetchFn }), + }); + await apply((state) => { + state.tokens = tokens; + }); + return tokens; + } catch (err) { + throw new UnauthorizedError( + `Token refresh failed for ${opts.serverName}: ${err instanceof Error ? err.message : String(err)}`, + ); + } + }, }; + return provider; } From fdb7862ea3535e822f0395e29990360b451dea57 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Wed, 9 Sep 2026 09:05:27 -0700 Subject: [PATCH 03/10] Share MCP browser authentication episode state --- src/mcp/client-auth-reauth-cap.test.ts | 108 ++++++++++++++++++++----- src/mcp/client.ts | 49 ++++++----- 2 files changed, 113 insertions(+), 44 deletions(-) diff --git a/src/mcp/client-auth-reauth-cap.test.ts b/src/mcp/client-auth-reauth-cap.test.ts index d2fd8a070..d8f17f2c8 100644 --- a/src/mcp/client-auth-reauth-cap.test.ts +++ b/src/mcp/client-auth-reauth-cap.test.ts @@ -5,11 +5,15 @@ import { withMockedModule } from "../../tests/helpers/mock-module.js"; let finishAuthCalls = 0; let finishAuthError: Error | undefined = new Error("finishAuth exploded"); let connectFailuresLeft = 0; +let callFailuresLeft = 0; +let redirectsPerFailure = 1; +let redirectConcurrently = false; let providerCreates = 0; let refreshCalls = 0; let refreshSucceeds = false; let authEvents: string[] = []; let authURLCount = 0; +let liveProvider: { redirectToAuthorization?: (url: URL) => void | Promise } | undefined; const fakeProvider = { resetAuthorization: async () => undefined, @@ -22,6 +26,18 @@ const fakeProvider = { }, }; +async function emitRedirects( + provider: { redirectToAuthorization?: (url: URL) => void | Promise } | undefined, +): Promise { + const redirect = () => + provider?.redirectToAuthorization?.(new URL("https://auth.test/authorize")); + if (redirectConcurrently) { + await Promise.all(Array.from({ length: redirectsPerFailure }, redirect)); + } else { + for (let call = 0; call < redirectsPerFailure; call += 1) await redirect(); + } +} + await withMockedModule( import.meta.resolve("@modelcontextprotocol/sdk/client/index.js"), (real: typeof import("@modelcontextprotocol/sdk/client/index.js")) => ({ @@ -32,19 +48,24 @@ await withMockedModule( redirectToAuthorization?: (url: URL) => void | Promise; }; }): Promise { + liveProvider = transport?.provider; if (connectFailuresLeft > 0) { connectFailuresLeft -= 1; - // Production StreamableHTTPClientTransport calls SDK auth() on HTTP 401, - // which invokes redirectToAuthorization and then throws UnauthorizedError. - await transport?.provider?.redirectToAuthorization?.( - new URL("https://auth.test/authorize"), - ); + await emitRedirects(transport?.provider); throw new UnauthorizedError("authorization required"); } } async listTools(): Promise<{ tools: [] }> { return { tools: [] }; } + async callTool(): Promise<{ content: [] }> { + if (callFailuresLeft > 0) { + callFailuresLeft -= 1; + await emitRedirects(liveProvider); + throw new UnauthorizedError("authorization required"); + } + return { content: [] }; + } async close(): Promise {} }, }), @@ -133,9 +154,13 @@ async function connectWithAuthPrompt(): Promise<{ ok: boolean; error?: string }> describe("HTTP MCP re-auth loop prevention", () => { beforeEach(() => { connectFailuresLeft = 0; + callFailuresLeft = 0; + redirectsPerFailure = 1; + redirectConcurrently = false; finishAuthCalls = 0; finishAuthError = new Error("finishAuth exploded"); providerCreates = 0; + liveProvider = undefined; refreshCalls = 0; refreshSucceeds = false; authEvents = []; @@ -148,8 +173,9 @@ describe("HTTP MCP re-auth loop prevention", () => { setSystemTime(); }); - test("refreshes tokens before any browser re-auth prompt", async () => { + test("uses one custom refresh before prompting when recovery starts without a redirect", async () => { refreshSucceeds = true; + redirectsPerFailure = 0; connectFailuresLeft = 1; const result = await connectWithAuthPrompt(); @@ -161,17 +187,64 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(authURLCount).toBe(0); }); - test("a failed refresh still precedes the browser prompt", async () => { - connectFailuresLeft = Number.POSITIVE_INFINITY; + test("does not repeat the SDK refresh after redirecting to authorization", async () => { + finishAuthError = undefined; + connectFailuresLeft = 1; + const result = await connectWithAuthPrompt(); - expect(result.ok).toBe(false); - expect(result.error).toContain("finishAuth exploded"); - expect(authEvents).toEqual(["refresh", "authURL"]); - expect(refreshCalls).toBe(1); + expect(result.ok).toBe(true); + expect(authEvents).toEqual(["authURL"]); + expect(refreshCalls).toBe(0); + expect(finishAuthCalls).toBe(1); expect(authURLCount).toBe(1); }); + test("live-call auth episodes share redirect state and stop after three prompts", async () => { + const connected = await connectMCPServer(config, { + onAuthURL: () => { + authURLCount += 1; + }, + }); + expect(connected.ok).toBe(true); + if (!connected.ok) return; + + for (let episode = 0; episode < MAX_BROWSER_AUTH_ATTEMPTS; episode += 1) { + callFailuresLeft = 1; + await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( + "finishAuth exploded", + ); + } + expect(authURLCount).toBe(MAX_BROWSER_AUTH_ATTEMPTS); + + callFailuresLeft = 1; + await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( + "retrying paused", + ); + expect(authURLCount).toBe(MAX_BROWSER_AUTH_ATTEMPTS); + }); + + test("repeated concurrent redirects emit one counted prompt", async () => { + const connected = await connectMCPServer(config, { + onAuthURL: () => { + authURLCount += 1; + }, + }); + expect(connected.ok).toBe(true); + if (!connected.ok) return; + redirectsPerFailure = 3; + redirectConcurrently = true; + callFailuresLeft = 1; + + await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( + "finishAuth exploded", + ); + + expect(authURLCount).toBe(1); + expect(finishAuthCalls).toBe(1); + expect(refreshCalls).toBe(0); + }); + test("failed browser re-auth is capped and pauses re-prompting across episodes", async () => { connectFailuresLeft = Number.POSITIVE_INFINITY; @@ -229,7 +302,9 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(authURLCount).toBe(3 + MAX_BROWSER_AUTH_ATTEMPTS); }); - test("prompts resume after the cooldown", async () => { + test("prompts resume five minutes after the third failed episode without a fourth probe", async () => { + const thirdEpisodeAt = new Date("2026-01-01T00:00:00Z").getTime(); + setSystemTime(thirdEpisodeAt); connectFailuresLeft = Number.POSITIVE_INFINITY; for (let episode = 0; episode < MAX_BROWSER_AUTH_ATTEMPTS; episode += 1) { const result = await connectWithAuthPrompt(); @@ -237,12 +312,7 @@ describe("HTTP MCP re-auth loop prevention", () => { } expect(authURLCount).toBe(MAX_BROWSER_AUTH_ATTEMPTS); - const duringCooldown = await connectWithAuthPrompt(); - expect(duringCooldown.ok).toBe(false); - expect(duringCooldown.error).toContain("retrying paused"); - expect(authURLCount).toBe(MAX_BROWSER_AUTH_ATTEMPTS); - - setSystemTime(Date.now() + BROWSER_AUTH_COOLDOWN_MS + 1); + setSystemTime(thirdEpisodeAt + BROWSER_AUTH_COOLDOWN_MS + 1); const afterCooldown = await connectWithAuthPrompt(); expect(afterCooldown.ok).toBe(false); expect(afterCooldown.error).toContain("finishAuth exploded"); diff --git a/src/mcp/client.ts b/src/mcp/client.ts index 5536aded8..9749476dc 100644 --- a/src/mcp/client.ts +++ b/src/mcp/client.ts @@ -62,12 +62,16 @@ interface HTTPAuthContext { url: URL; authProvider: CorbitsOAuthProvider; callback: CallbackServer; + episode: HTTPAuthEpisodeState; signal?: AbortSignal; interactive: boolean; serverName: string; onAuthorized?: (serverName: string) => void; +} + +interface HTTPAuthEpisodeState { promptGuard?: { clear(): void }; - refreshedWithoutPrompt?: boolean; + customRefreshAttempted: boolean; } function isRecoverableAuthError(err: unknown): boolean { @@ -107,10 +111,12 @@ function beginBrowserAuth(context: HTTPAuthContext): { clear(): void } { entry.count = 0; } if (entry.count >= MAX_BROWSER_AUTH_ATTEMPTS) { - entry.cooldownUntil = now + BROWSER_AUTH_COOLDOWN_MS; throw browserAuthCapError(context.serverName); } entry.count += 1; + if (entry.count === MAX_BROWSER_AUTH_ATTEMPTS) { + entry.cooldownUntil = now + BROWSER_AUTH_COOLDOWN_MS; + } browserAuthAttempts.set(key, entry); return { clear: () => browserAuthAttempts.delete(key), @@ -132,11 +138,8 @@ async function tryTokenRefresh(context: HTTPAuthContext): Promise { function gateRedirectToAuthorization(context: HTTPAuthContext): void { const inner = context.authProvider.redirectToAuthorization.bind(context.authProvider); context.authProvider.redirectToAuthorization = async (authorizationUrl: URL) => { - if (await tryTokenRefresh(context)) { - context.refreshedWithoutPrompt = true; - return; - } - context.promptGuard ??= beginBrowserAuth(context); + if (context.episode.promptGuard !== undefined) return; + context.episode.promptGuard = beginBrowserAuth(context); await inner(authorizationUrl); }; } @@ -206,23 +209,12 @@ async function recoverHTTPAuthorization( ): Promise { if (context === undefined || !isRecoverableAuthError(err)) throw err; let lastErr: unknown = err; - let refreshAttempted = false; for (let attempt = 0; attempt < 2; attempt += 1) { if (lastErr instanceof OAuthError) await context.authProvider.resetAuthorization(); if (lastErr instanceof UnauthorizedError) { - if (context.refreshedWithoutPrompt === true) { - context.refreshedWithoutPrompt = false; - try { - return await operation(); - } catch (nextErr) { - if (!isRecoverableAuthError(nextErr)) throw nextErr; - lastErr = nextErr; - continue; - } - } - if (context.promptGuard === undefined) { - if (!refreshAttempted) { - refreshAttempted = true; + if (context.episode.promptGuard === undefined) { + if (!context.episode.customRefreshAttempted) { + context.episode.customRefreshAttempted = true; if (await tryTokenRefresh(context)) { try { return await operation(); @@ -233,9 +225,9 @@ async function recoverHTTPAuthorization( } } } - context.promptGuard = beginBrowserAuth(context); + context.episode.promptGuard = beginBrowserAuth(context); } - const guard = context.promptGuard; + const guard = context.episode.promptGuard; const value = await retryAfterInteractiveAuth( () => completeInteractiveAuth(context), operation, @@ -244,7 +236,6 @@ async function recoverHTTPAuthorization( : () => context.onAuthorized?.(context.serverName), ); guard.clear(); - delete context.promptGuard; return value; } try { @@ -264,7 +255,14 @@ async function withHTTPAuthorizationRecovery( try { return await operation(); } catch (err) { - return recoverHTTPAuthorization(err, context, operation); + try { + return await recoverHTTPAuthorization(err, context, operation); + } finally { + if (context !== undefined) { + delete context.episode.promptGuard; + context.episode.customRefreshAttempted = false; + } + } } } @@ -373,6 +371,7 @@ async function connectHttp( url, authProvider, callback, + episode: { customRefreshAttempted: false }, interactive: options.onAuthURL !== undefined, serverName: config.name, ...(options.onAuthorized !== undefined ? { onAuthorized: options.onAuthorized } : {}), From 8aee7765621cd508aaae2698154b304bd2099c8b Mon Sep 17 00:00:00 2001 From: Sawyer Date: Wed, 9 Sep 2026 12:57:41 -0700 Subject: [PATCH 04/10] Coordinate MCP authorization recovery per connection --- src/mcp/client-auth-policy.test.ts | 17 +- src/mcp/client-auth-reauth-cap.test.ts | 164 ++++++++++++++++- src/mcp/client.ts | 246 +++++++++++++++++-------- 3 files changed, 344 insertions(+), 83 deletions(-) diff --git a/src/mcp/client-auth-policy.test.ts b/src/mcp/client-auth-policy.test.ts index fb828e412..ba6cdfd98 100644 --- a/src/mcp/client-auth-policy.test.ts +++ b/src/mcp/client-auth-policy.test.ts @@ -17,6 +17,7 @@ let blockTokenExchange = false; let tokenExchangeSignals: (AbortSignal | null | undefined)[] = []; let tokenExchangeAborts = 0; let lastTransportAuth: (() => Promise) | undefined; +let lastTransportRedirect: (() => Promise) | undefined; let tokenRefreshSignals: (AbortSignal | null | undefined)[] = []; let tokenRefreshAborts = 0; @@ -35,9 +36,10 @@ function hangUntilAbort( }); } +const redirectToAuthorization = () => undefined; const authProvider = { resetAuthorization: async () => undefined, - redirectToAuthorization: () => undefined, + redirectToAuthorization, }; await withMockedModule( @@ -46,7 +48,10 @@ await withMockedModule( ...real, Client: class { async connect(): Promise { - if (clientConnectError !== undefined) throw clientConnectError; + if (clientConnectError !== undefined) { + if (clientConnectError instanceof UnauthorizedError) await lastTransportRedirect?.(); + throw clientConnectError; + } } async listTools( _params?: unknown, @@ -86,12 +91,18 @@ await withMockedModule( constructor( _url: URL, private readonly options?: { + authProvider?: { redirectToAuthorization?: (url: URL) => void | Promise }; requestInit?: RequestInit; fetch?: (url: string | URL, init?: RequestInit) => Promise; }, ) { transportOptions.push(options); lastTransportAuth = () => this.auth(); + lastTransportRedirect = async () => { + await this.options?.authProvider?.redirectToAuthorization?.( + new URL("https://auth.test/authorize"), + ); + }; } async finishAuth(): Promise { const signal = this.options?.requestInit?.signal; @@ -177,8 +188,10 @@ describe("HTTP MCP auth policy", () => { tokenExchangeSignals = []; tokenExchangeAborts = 0; lastTransportAuth = undefined; + lastTransportRedirect = undefined; tokenRefreshSignals = []; tokenRefreshAborts = 0; + authProvider.redirectToAuthorization = redirectToAuthorization; }); test("built-in anonymous Exa treats 401 as a normal failure without OAuth machinery", async () => { diff --git a/src/mcp/client-auth-reauth-cap.test.ts b/src/mcp/client-auth-reauth-cap.test.ts index d8f17f2c8..65fc445ef 100644 --- a/src/mcp/client-auth-reauth-cap.test.ts +++ b/src/mcp/client-auth-reauth-cap.test.ts @@ -5,14 +5,23 @@ import { withMockedModule } from "../../tests/helpers/mock-module.js"; let finishAuthCalls = 0; let finishAuthError: Error | undefined = new Error("finishAuth exploded"); let connectFailuresLeft = 0; +let listFailuresLeft = 0; let callFailuresLeft = 0; +let callToolCalls = 0; let redirectsPerFailure = 1; +let redirectOnListFailure = false; let redirectConcurrently = false; let providerCreates = 0; let refreshCalls = 0; let refreshSucceeds = false; let authEvents: string[] = []; let authURLCount = 0; +let authorizedCount = 0; +let waitForCodeCalls = 0; +let refreshGate: Promise | undefined; +let releaseRefresh: (() => void) | undefined; +let callbackGate: Promise | undefined; +let releaseCallback: (() => void) | undefined; let liveProvider: { redirectToAuthorization?: (url: URL) => void | Promise } | undefined; const fakeProvider = { @@ -21,6 +30,7 @@ const fakeProvider = { refreshToken: async () => { refreshCalls += 1; authEvents.push("refresh"); + await refreshGate; if (!refreshSucceeds) throw new UnauthorizedError("refresh rejected"); return { access_token: "fresh", refresh_token: "refresh-me" }; }, @@ -38,6 +48,22 @@ async function emitRedirects( } } +function waitForGate(signal: AbortSignal): Promise { + if (callbackGate === undefined) return Promise.resolve(); + if (signal.aborted) return Promise.reject(signal.reason); + return new Promise((resolve, reject) => { + const onAbort = () => { + signal.removeEventListener("abort", onAbort); + reject(signal.reason); + }; + signal.addEventListener("abort", onAbort, { once: true }); + void callbackGate?.then(() => { + signal.removeEventListener("abort", onAbort); + resolve(); + }); + }); +} + await withMockedModule( import.meta.resolve("@modelcontextprotocol/sdk/client/index.js"), (real: typeof import("@modelcontextprotocol/sdk/client/index.js")) => ({ @@ -56,9 +82,17 @@ await withMockedModule( } } async listTools(): Promise<{ tools: [] }> { + if (listFailuresLeft > 0) { + listFailuresLeft -= 1; + if (redirectOnListFailure) { + await liveProvider?.redirectToAuthorization?.(new URL("https://auth.test/authorize")); + } + throw new UnauthorizedError("authorization required"); + } return { tools: [] }; } async callTool(): Promise<{ content: [] }> { + callToolCalls += 1; if (callFailuresLeft > 0) { callFailuresLeft -= 1; await emitRedirects(liveProvider); @@ -107,7 +141,11 @@ await withMockedModule( startCallbackServer: async () => ({ redirectUrl: "http://127.0.0.1:12345/callback", expectState: () => undefined, - waitForCode: async () => "code", + waitForCode: async (signal: AbortSignal) => { + waitForCodeCalls += 1; + await waitForGate(signal); + return "code"; + }, close: () => undefined, }), }), @@ -154,8 +192,11 @@ async function connectWithAuthPrompt(): Promise<{ ok: boolean; error?: string }> describe("HTTP MCP re-auth loop prevention", () => { beforeEach(() => { connectFailuresLeft = 0; + listFailuresLeft = 0; callFailuresLeft = 0; + callToolCalls = 0; redirectsPerFailure = 1; + redirectOnListFailure = false; redirectConcurrently = false; finishAuthCalls = 0; finishAuthError = new Error("finishAuth exploded"); @@ -165,6 +206,12 @@ describe("HTTP MCP re-auth loop prevention", () => { refreshSucceeds = false; authEvents = []; authURLCount = 0; + authorizedCount = 0; + waitForCodeCalls = 0; + refreshGate = undefined; + releaseRefresh = undefined; + callbackGate = undefined; + releaseCallback = undefined; resetBrowserAuthState(); setSystemTime(); }); @@ -187,6 +234,121 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(authURLCount).toBe(0); }); + test("rejects promptly when refresh and an auth probe fail without emitting a URL", async () => { + const connected = await connectMCPServer(config, { + onAuthURL: () => (authURLCount += 1), + onAuthorized: () => (authorizedCount += 1), + }); + expect(connected.ok).toBe(true); + if (!connected.ok) return; + redirectsPerFailure = 0; + callFailuresLeft = 1; + listFailuresLeft = 1; + + await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( + "authorization required", + ); + + expect(refreshCalls).toBe(1); + expect(authURLCount).toBe(0); + expect(waitForCodeCalls).toBe(0); + expect(authorizedCount).toBe(0); + }); + + test("shares live-call recovery across concurrent unauthorized calls", async () => { + finishAuthError = undefined; + const connected = await connectMCPServer(config, { + onAuthURL: () => (authURLCount += 1), + onAuthorized: () => (authorizedCount += 1), + }); + expect(connected.ok).toBe(true); + if (!connected.ok) return; + redirectsPerFailure = 0; + callFailuresLeft = 2; + listFailuresLeft = 1; + redirectOnListFailure = true; + + const calls = [ + connected.client.call("first", { value: 1 }, new AbortController().signal), + connected.client.call("second", { value: 2 }, new AbortController().signal), + ]; + + await expect(Promise.all(calls)).resolves.toEqual(["", ""]); + expect(refreshCalls).toBe(1); + expect(authURLCount).toBe(1); + expect(waitForCodeCalls).toBe(1); + expect(finishAuthCalls).toBe(1); + expect(callToolCalls).toBe(4); + expect(authorizedCount).toBe(1); + }); + + test("does not start browser fallback while shared refresh is pending", async () => { + const connected = await connectMCPServer(config, { onAuthURL: () => (authURLCount += 1) }); + expect(connected.ok).toBe(true); + if (!connected.ok) return; + refreshGate = new Promise((resolve) => { + releaseRefresh = resolve; + }); + redirectsPerFailure = 0; + callFailuresLeft = 1; + const first = connected.client.call("first", {}, new AbortController().signal); + while (refreshCalls === 0) await Promise.resolve(); + + redirectsPerFailure = 1; + callFailuresLeft = 1; + const second = connected.client.call("second", {}, new AbortController().signal); + await Promise.resolve(); + expect(authURLCount).toBe(0); + + releaseRefresh?.(); + await expect(Promise.all([first, second])).rejects.toThrow("finishAuth exploded"); + expect(refreshCalls).toBe(1); + expect(authURLCount).toBe(1); + expect(waitForCodeCalls).toBe(1); + }); + + test("caller abort does not cancel shared recovery for another call", async () => { + finishAuthError = undefined; + callbackGate = new Promise((resolve) => { + releaseCallback = resolve; + }); + const connected = await connectMCPServer(config, { onAuthURL: () => (authURLCount += 1) }); + expect(connected.ok).toBe(true); + if (!connected.ok) return; + callFailuresLeft = 2; + const firstAbort = new AbortController(); + const first = connected.client.call("first", {}, firstAbort.signal); + const second = connected.client.call("second", {}, new AbortController().signal); + while (waitForCodeCalls === 0) await Promise.resolve(); + + firstAbort.abort(new Error("caller stopped")); + await expect(first).rejects.toThrow("caller stopped"); + releaseCallback?.(); + + await expect(second).resolves.toBe(""); + expect(waitForCodeCalls).toBe(1); + expect(finishAuthCalls).toBe(1); + expect(authURLCount).toBe(1); + }); + + test("client close aborts the shared callback waiter", async () => { + finishAuthError = undefined; + callbackGate = new Promise((resolve) => { + releaseCallback = resolve; + }); + const connected = await connectMCPServer(config, { onAuthURL: () => (authURLCount += 1) }); + expect(connected.ok).toBe(true); + if (!connected.ok) return; + callFailuresLeft = 1; + const call = connected.client.call("ping", {}, new AbortController().signal); + while (waitForCodeCalls === 0) await Promise.resolve(); + + await connected.client.close(); + + await expect(call).rejects.toHaveProperty("name", "AbortError"); + expect(finishAuthCalls).toBe(0); + }); + test("does not repeat the SDK refresh after redirecting to authorization", async () => { finishAuthError = undefined; connectFailuresLeft = 1; diff --git a/src/mcp/client.ts b/src/mcp/client.ts index 9749476dc..a8fab5400 100644 --- a/src/mcp/client.ts +++ b/src/mcp/client.ts @@ -62,19 +62,27 @@ interface HTTPAuthContext { url: URL; authProvider: CorbitsOAuthProvider; callback: CallbackServer; - episode: HTTPAuthEpisodeState; - signal?: AbortSignal; + coordinator: HTTPAuthCoordinator; interactive: boolean; serverName: string; onAuthorized?: (serverName: string) => void; } -interface HTTPAuthEpisodeState { - promptGuard?: { clear(): void }; - customRefreshAttempted: boolean; +interface BrowserAuthFlow { + attempt: { clear(): void }; + promptEmitted: Promise; +} + +interface HTTPAuthCoordinator { + lifecycle: AbortController; + requestSignal: AbortSignal; + inFlight?: Promise; + refreshInFlight?: Promise; + browserFlow?: BrowserAuthFlow; + probe(): Promise; } -function isRecoverableAuthError(err: unknown): boolean { +function isRecoverableAuthError(err: unknown): err is UnauthorizedError | OAuthError { return err instanceof UnauthorizedError || err instanceof OAuthError; } @@ -135,12 +143,36 @@ async function tryTokenRefresh(context: HTTPAuthContext): Promise { } } +function getOrStartRefresh(context: HTTPAuthContext): Promise { + const coordinator = context.coordinator; + if (coordinator.refreshInFlight !== undefined) return coordinator.refreshInFlight; + const shared = Promise.resolve().then(() => tryTokenRefresh(context)); + coordinator.refreshInFlight = shared; + const clear = () => { + if (coordinator.refreshInFlight === shared) delete coordinator.refreshInFlight; + }; + void shared.then(clear, clear); + return shared; +} + function gateRedirectToAuthorization(context: HTTPAuthContext): void { const inner = context.authProvider.redirectToAuthorization.bind(context.authProvider); context.authProvider.redirectToAuthorization = async (authorizationUrl: URL) => { - if (context.episode.promptGuard !== undefined) return; - context.episode.promptGuard = beginBrowserAuth(context); - await inner(authorizationUrl); + const coordinator = context.coordinator; + if (coordinator.browserFlow !== undefined) return coordinator.browserFlow.promptEmitted; + const refresh = coordinator.refreshInFlight; + if (refresh !== undefined && (await refresh)) return; + const concurrentBrowserFlow = coordinator.browserFlow as BrowserAuthFlow | undefined; + if (concurrentBrowserFlow !== undefined) return concurrentBrowserFlow.promptEmitted; + if (!context.interactive) + throw new Error("Authorization required but no interactive handler is available."); + + const attempt = beginBrowserAuth(context); + const startPrompt = Promise.withResolvers(); + const promptEmitted = startPrompt.promise.then(() => inner(authorizationUrl)); + coordinator.browserFlow = { attempt, promptEmitted }; + startPrompt.resolve(); + return promptEmitted; }; } @@ -176,16 +208,6 @@ function streamableHTTPTransportOptions( }; } -async function completeInteractiveAuth(context: HTTPAuthContext): Promise { - if (!context.interactive) - throw new Error("Authorization required but no interactive handler is available."); - const code = await context.callback.waitForCode(context.signal ?? new AbortController().signal); - await new StreamableHTTPClientTransport( - context.url, - streamableHTTPTransportOptions(context.authProvider, context.signal), - ).finishAuth(code); -} - /** * Run interactive OAuth, retry the failed operation, and notify only when the * retry itself succeeded — a failed re-auth must leave standing "needs auth" @@ -202,67 +224,97 @@ export async function retryAfterInteractiveAuth( return value; } -async function recoverHTTPAuthorization( - err: unknown, - context: HTTPAuthContext | undefined, - operation: () => Promise, -): Promise { - if (context === undefined || !isRecoverableAuthError(err)) throw err; - let lastErr: unknown = err; - for (let attempt = 0; attempt < 2; attempt += 1) { - if (lastErr instanceof OAuthError) await context.authProvider.resetAuthorization(); - if (lastErr instanceof UnauthorizedError) { - if (context.episode.promptGuard === undefined) { - if (!context.episode.customRefreshAttempted) { - context.episode.customRefreshAttempted = true; - if (await tryTokenRefresh(context)) { - try { - return await operation(); - } catch (nextErr) { - if (!isRecoverableAuthError(nextErr)) throw nextErr; - lastErr = nextErr; - continue; - } - } - } - context.episode.promptGuard = beginBrowserAuth(context); - } - const guard = context.episode.promptGuard; - const value = await retryAfterInteractiveAuth( - () => completeInteractiveAuth(context), - operation, - context.onAuthorized === undefined - ? undefined - : () => context.onAuthorized?.(context.serverName), - ); - guard.clear(); - return value; - } +async function driveRecovery(err: UnauthorizedError | OAuthError, context: HTTPAuthContext) { + const coordinator = context.coordinator; + if (err instanceof OAuthError) await context.authProvider.resetAuthorization(); + if (err instanceof UnauthorizedError && coordinator.browserFlow === undefined) { + await getOrStartRefresh(context); + // SDK redirects waiting on this refresh must reserve the browser flow before the probe. + await Promise.resolve(); + } + + if (coordinator.browserFlow === undefined) { try { - return await operation(); - } catch (nextErr) { - if (!isRecoverableAuthError(nextErr)) throw nextErr; - lastErr = nextErr; + await coordinator.probe(); + resetAfterVerifiedRecovery(context); + return; + } catch (probeErr) { + if (!isRecoverableAuthError(probeErr)) throw probeErr; + if (coordinator.browserFlow === undefined) throw probeErr; } } - throw lastErr; + + const browserFlow = coordinator.browserFlow; + await browserFlow.promptEmitted; + const code = await context.callback.waitForCode(coordinator.lifecycle.signal); + await new StreamableHTTPClientTransport( + context.url, + streamableHTTPTransportOptions(context.authProvider, coordinator.requestSignal), + ).finishAuth(code); + await coordinator.probe(); + resetAfterVerifiedRecovery(context); +} + +function resetAfterVerifiedRecovery(context: HTTPAuthContext): void { + context.coordinator.browserFlow?.attempt.clear(); + context.onAuthorized?.(context.serverName); +} + +function getOrStartRecovery( + err: UnauthorizedError | OAuthError, + context: HTTPAuthContext, +): Promise { + const coordinator = context.coordinator; + if (coordinator.inFlight !== undefined) return coordinator.inFlight; + const shared = Promise.resolve().then(() => driveRecovery(err, context)); + coordinator.inFlight = shared; + const clear = () => { + if (coordinator.inFlight !== shared) return; + delete coordinator.inFlight; + delete coordinator.refreshInFlight; + delete coordinator.browserFlow; + }; + void shared.then(clear, clear); + return shared; +} + +function abortReason(signal: AbortSignal): unknown { + return signal.reason ?? new DOMException("The operation was aborted", "AbortError"); +} + +function awaitRecovery(recovery: Promise, signal: AbortSignal | undefined): Promise { + if (signal === undefined) return recovery; + if (signal.aborted) return Promise.reject(abortReason(signal)); + return new Promise((resolve, reject) => { + const onAbort = () => { + signal.removeEventListener("abort", onAbort); + reject(abortReason(signal)); + }; + signal.addEventListener("abort", onAbort, { once: true }); + void recovery.then( + () => { + signal.removeEventListener("abort", onAbort); + resolve(); + }, + (err) => { + signal.removeEventListener("abort", onAbort); + reject(err); + }, + ); + }); } async function withHTTPAuthorizationRecovery( context: HTTPAuthContext | undefined, operation: () => Promise, + signal?: AbortSignal, ): Promise { try { return await operation(); } catch (err) { - try { - return await recoverHTTPAuthorization(err, context, operation); - } finally { - if (context !== undefined) { - delete context.episode.promptGuard; - context.episode.customRefreshAttempted = false; - } - } + if (context === undefined || !isRecoverableAuthError(err)) throw err; + await awaitRecovery(getOrStartRecovery(err, context), signal); + return operation(); } } @@ -271,6 +323,7 @@ async function finishClient( serverName: string, authContext?: HTTPAuthContext, signal?: AbortSignal, + closeLifecycle?: () => void, ): Promise { const result = await withHTTPAuthorizationRecovery(authContext, () => signal === undefined ? client.listTools() : client.listTools(undefined, { signal }), @@ -289,13 +342,15 @@ async function finishClient( serverName, tools, async call(toolName, args, signal) { - const context = authContext === undefined ? undefined : { ...authContext, signal }; - const result = await withHTTPAuthorizationRecovery(context, () => - client.callTool({ name: toolName, arguments: args }, undefined, { signal }), + const result = await withHTTPAuthorizationRecovery( + authContext, + () => client.callTool({ name: toolName, arguments: args }, undefined, { signal }), + signal, ); return unwrapToolContent(result.content); }, async close() { + closeLifecycle?.(); authContext?.callback.close(); await client.close().catch(() => undefined); }, @@ -341,6 +396,14 @@ async function connectHttp( return { ok: false, serverName: config.name, error: "http MCP server requires a url" }; let callback: CallbackServer | undefined; let client: Client | undefined; + const lifecycle = new AbortController(); + const abortLifecycle = () => lifecycle.abort(options.signal?.reason); + const closeLifecycle = () => { + lifecycle.abort(); + options.signal?.removeEventListener("abort", abortLifecycle); + }; + if (options.signal?.aborted) abortLifecycle(); + else options.signal?.addEventListener("abort", abortLifecycle, { once: true }); try { const normalizedURL = normalizeMCPServerURL(config.url); const url = new URL(normalizedURL); @@ -360,38 +423,61 @@ async function connectHttp( redirectUrl: callback.redirectUrl, onAuthURL: (name, authUrl) => options.onAuthURL?.(name, authUrl), onAuthorizationState: callback.expectState, - ...(options.signal === undefined ? {} : { fetchFn: fetchWithConnectAbort(options.signal) }), + ...(options.signal === undefined + ? { fetchFn: fetchWithConnectAbort(lifecycle.signal) } + : { fetchFn: fetchWithConnectAbort(options.signal) }), }); makeTransport = () => new StreamableHTTPClientTransport( url, streamableHTTPTransportOptions(authProvider, options.signal), ) as unknown as Transport; + const coordinator: HTTPAuthCoordinator = { + lifecycle, + requestSignal: options.signal ?? lifecycle.signal, + probe: async () => { + const probeClient = new Client({ name: MCP_CLIENT_NAME, version: "1.0.0" }); + try { + await probeClient.connect(makeTransport(), { signal: lifecycle.signal }); + } finally { + await probeClient.close().catch(() => undefined); + } + }, + }; authContext = { url, authProvider, callback, - episode: { customRefreshAttempted: false }, + coordinator, interactive: options.onAuthURL !== undefined, serverName: config.name, ...(options.onAuthorized !== undefined ? { onAuthorized: options.onAuthorized } : {}), - ...(options.signal !== undefined ? { signal: options.signal } : {}), }; gateRedirectToAuthorization(authContext); } const connectedClient = new Client({ name: MCP_CLIENT_NAME, version: "1.0.0" }); client = connectedClient; - await withHTTPAuthorizationRecovery(authContext, () => - connectedClient.connect( - makeTransport(), - options.signal === undefined ? undefined : { signal: options.signal }, - ), + await withHTTPAuthorizationRecovery( + authContext, + () => connectedClient.connect(makeTransport(), { signal: lifecycle.signal }), + lifecycle.signal, ); + if (authContext !== undefined) { + authContext.coordinator.probe = () => + connectedClient.listTools(undefined, { signal: lifecycle.signal }).then(() => undefined); + } return { ok: true, - client: await finishClient(connectedClient, config.name, authContext, options.signal), + client: await finishClient( + connectedClient, + config.name, + authContext, + options.signal, + closeLifecycle, + ), }; } catch (err) { + closeLifecycle(); await client?.close().catch(() => undefined); try { callback?.close(); From 06a3822d48918d70d7b2936147a02441e2180761 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Wed, 9 Sep 2026 16:17:22 -0700 Subject: [PATCH 05/10] Close leftover MCP authorization recovery races --- src/mcp/client-auth-policy.test.ts | 56 +++++- src/mcp/client-auth-reauth-cap.test.ts | 268 +++++++++++++++++++++++-- src/mcp/client.ts | 74 +++++-- 3 files changed, 355 insertions(+), 43 deletions(-) diff --git a/src/mcp/client-auth-policy.test.ts b/src/mcp/client-auth-policy.test.ts index ba6cdfd98..8c2e93ccc 100644 --- a/src/mcp/client-auth-policy.test.ts +++ b/src/mcp/client-auth-policy.test.ts @@ -259,7 +259,8 @@ describe("HTTP MCP auth policy", () => { ); while (tokenExchangeSignals.length === 0) await Promise.resolve(); - expect(tokenExchangeSignals[0]).toBe(abort.signal); + expect(tokenExchangeSignals[0]).toBeDefined(); + expect(tokenExchangeSignals[0]?.aborted).toBe(false); abort.abort(new Error("toolset disposed")); const result = await connection; @@ -280,12 +281,17 @@ describe("HTTP MCP auth policy", () => { expect(result.ok).toBe(true); if (!result.ok) return; expect(transportOptions).toEqual([ - { authProvider, requestInit: { signal: abort.signal }, fetch: expect.any(Function) }, + { + authProvider, + requestInit: { signal: expect.any(AbortSignal) }, + fetch: expect.any(Function), + }, ]); const call = result.client.call("ping", {}, abort.signal); while (tokenRefreshSignals.length === 0) await Promise.resolve(); - expect(tokenRefreshSignals[0]).toBe(abort.signal); + expect(tokenRefreshSignals[0]).toBeDefined(); + expect(tokenRefreshSignals[0]?.aborted).toBe(false); abort.abort(new Error("toolset disposed")); await expect(call).rejects.toThrow("toolset disposed"); expect(tokenRefreshAborts).toBe(1); @@ -310,7 +316,49 @@ describe("HTTP MCP auth policy", () => { expect(callbackStarts).toBe(1); expect(providerCreates).toBe(1); expect(providerServerURL).toBe("https://custom.example/mcp?mode=full"); - expect(transportOptions).toEqual([{ authProvider }]); + expect(transportOptions).toEqual([ + { + authProvider, + requestInit: { signal: expect.any(AbortSignal) }, + fetch: expect.any(Function), + }, + ]); + }); + + test("client close aborts in-flight OAuth without aborting the connect signal", async () => { + const connectAbort = new AbortController(); + const result = await connectMCPServer( + { name: "linear", type: "http", url: "https://mcp.linear.app/mcp" }, + { signal: connectAbort.signal }, + ); + expect(result.ok).toBe(true); + if (!result.ok) return; + + const transport = transportOptions[0] as { + requestInit?: { signal?: AbortSignal }; + fetch?: (url: string | URL, init?: RequestInit) => Promise; + }; + expect(transport.requestInit?.signal).toBeDefined(); + expect(transport.requestInit?.signal).not.toBe(connectAbort.signal); + expect(transport.fetch).toBeTypeOf("function"); + + const call = result.client.call("ping", {}, new AbortController().signal); + while (tokenRefreshSignals.length === 0) await Promise.resolve(); + expect(tokenRefreshSignals[0]).not.toBe(connectAbort.signal); + expect(tokenRefreshSignals[0]?.aborted).toBe(false); + expect(connectAbort.signal.aborted).toBe(false); + + await result.client.close(); + + await expect(call).rejects.toThrow(); + expect(tokenRefreshAborts).toBe(1); + expect(connectAbort.signal.aborted).toBe(false); + expect(transport.requestInit?.signal?.aborted).toBe(true); + const fetchFn = transport.fetch; + expect(fetchFn).toBeTypeOf("function"); + if (fetchFn === undefined) return; + await expect(fetchFn("https://auth.test/token")).rejects.toThrow(); + expect(connectAbort.signal.aborted).toBe(false); }); }); diff --git a/src/mcp/client-auth-reauth-cap.test.ts b/src/mcp/client-auth-reauth-cap.test.ts index 65fc445ef..309a0be1f 100644 --- a/src/mcp/client-auth-reauth-cap.test.ts +++ b/src/mcp/client-auth-reauth-cap.test.ts @@ -8,9 +8,13 @@ let connectFailuresLeft = 0; let listFailuresLeft = 0; let callFailuresLeft = 0; let callToolCalls = 0; +let callRedirectsLeft = Number.POSITIVE_INFINITY; let redirectsPerFailure = 1; let redirectOnListFailure = false; let redirectConcurrently = false; +let saveThenRedirectPair = false; +let overlappingSDKSaves = false; +let redirectVerifier: string | undefined; let providerCreates = 0; let refreshCalls = 0; let refreshSucceeds = false; @@ -18,11 +22,22 @@ let authEvents: string[] = []; let authURLCount = 0; let authorizedCount = 0; let waitForCodeCalls = 0; +let storedCodeVerifier: string | undefined; +let exchangedCodeVerifier: string | undefined; +let emittedAuthURL: string | undefined; +let saveStarted = 0; let refreshGate: Promise | undefined; let releaseRefresh: (() => void) | undefined; let callbackGate: Promise | undefined; let releaseCallback: (() => void) | undefined; -let liveProvider: { redirectToAuthorization?: (url: URL) => void | Promise } | undefined; +let saveGate: Promise | undefined; +let releaseSave: (() => void) | undefined; +interface MockAuthProvider { + redirectToAuthorization?: (url: URL) => void | Promise; + saveCodeVerifier?: (codeVerifier: string) => void | Promise; + codeVerifier?: () => string | undefined; +} +let liveProvider: MockAuthProvider | undefined; const fakeProvider = { resetAuthorization: async () => undefined, @@ -34,11 +49,46 @@ const fakeProvider = { if (!refreshSucceeds) throw new UnauthorizedError("refresh rejected"); return { access_token: "fresh", refresh_token: "refresh-me" }; }, + saveCodeVerifier: async (codeVerifier: string) => { + storedCodeVerifier = codeVerifier; + saveStarted += 1; + await saveGate; + }, + codeVerifier: () => storedCodeVerifier, }; -async function emitRedirects( - provider: { redirectToAuthorization?: (url: URL) => void | Promise } | undefined, +async function saveThenRedirect( + provider: MockAuthProvider | undefined, + verifier: string, ): Promise { + await provider?.saveCodeVerifier?.(verifier); + await provider?.redirectToAuthorization?.( + new URL(`https://auth.test/authorize?v=${encodeURIComponent(verifier)}`), + ); +} + +async function emitRedirects(provider: MockAuthProvider | undefined): Promise { + if (overlappingSDKSaves) { + const first = saveThenRedirect(provider, "v1"); + while (saveStarted === 0) await Promise.resolve(); + const second = saveThenRedirect(provider, "v2"); + await Promise.resolve(); + releaseSave?.(); + await Promise.all([first, second]); + return; + } + if (saveThenRedirectPair) { + const first = saveThenRedirect(provider, "v1"); + await Promise.resolve(); + await Promise.all([first, saveThenRedirect(provider, "v2")]); + return; + } + if (redirectVerifier !== undefined) { + const verifier = redirectVerifier; + redirectVerifier = undefined; + await saveThenRedirect(provider, verifier); + return; + } const redirect = () => provider?.redirectToAuthorization?.(new URL("https://auth.test/authorize")); if (redirectConcurrently) { @@ -69,11 +119,7 @@ await withMockedModule( (real: typeof import("@modelcontextprotocol/sdk/client/index.js")) => ({ ...real, Client: class { - async connect(transport?: { - provider?: { - redirectToAuthorization?: (url: URL) => void | Promise; - }; - }): Promise { + async connect(transport?: { provider?: MockAuthProvider }): Promise { liveProvider = transport?.provider; if (connectFailuresLeft > 0) { connectFailuresLeft -= 1; @@ -95,7 +141,10 @@ await withMockedModule( callToolCalls += 1; if (callFailuresLeft > 0) { callFailuresLeft -= 1; - await emitRedirects(liveProvider); + if (callRedirectsLeft > 0) { + callRedirectsLeft -= 1; + await emitRedirects(liveProvider); + } throw new UnauthorizedError("authorization required"); } return { content: [] }; @@ -110,21 +159,13 @@ await withMockedModule( (real: typeof import("@modelcontextprotocol/sdk/client/streamableHttp.js")) => ({ ...real, StreamableHTTPClientTransport: class { - provider?: { - redirectToAuthorization?: (url: URL) => void | Promise; - }; - constructor( - _url: URL, - options?: { - authProvider?: { - redirectToAuthorization?: (url: URL) => void | Promise; - }; - }, - ) { + provider?: MockAuthProvider; + constructor(_url: URL, options?: { authProvider?: MockAuthProvider }) { if (options?.authProvider !== undefined) this.provider = options.authProvider; } async finishAuth(): Promise { finishAuthCalls += 1; + exchangedCodeVerifier = this.provider?.codeVerifier?.(); if (finishAuthError !== undefined) throw finishAuthError; } get sessionId(): string | undefined { @@ -195,9 +236,13 @@ describe("HTTP MCP re-auth loop prevention", () => { listFailuresLeft = 0; callFailuresLeft = 0; callToolCalls = 0; + callRedirectsLeft = Number.POSITIVE_INFINITY; redirectsPerFailure = 1; redirectOnListFailure = false; redirectConcurrently = false; + saveThenRedirectPair = false; + overlappingSDKSaves = false; + redirectVerifier = undefined; finishAuthCalls = 0; finishAuthError = new Error("finishAuth exploded"); providerCreates = 0; @@ -208,15 +253,22 @@ describe("HTTP MCP re-auth loop prevention", () => { authURLCount = 0; authorizedCount = 0; waitForCodeCalls = 0; + storedCodeVerifier = undefined; + exchangedCodeVerifier = undefined; + emittedAuthURL = undefined; + saveStarted = 0; refreshGate = undefined; releaseRefresh = undefined; callbackGate = undefined; releaseCallback = undefined; + saveGate = undefined; + releaseSave = undefined; resetBrowserAuthState(); setSystemTime(); }); afterEach(() => { + resetBrowserAuthState(); setSystemTime(); }); @@ -502,4 +554,180 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(finishAuthCalls).toBe(promptsBefore + 1); expect(authURLCount).toBe(MAX_BROWSER_AUTH_ATTEMPTS + 1); }); + + test("first in-episode saveCodeVerifier wins until the episode ends", async () => { + finishAuthError = undefined; + saveThenRedirectPair = true; + const connected = await connectMCPServer(config, { + onAuthURL: () => { + authURLCount += 1; + }, + }); + expect(connected.ok).toBe(true); + if (!connected.ok) return; + callFailuresLeft = 1; + + await expect(connected.client.call("ping", {}, new AbortController().signal)).resolves.toBe(""); + + expect(authURLCount).toBe(1); + expect(waitForCodeCalls).toBe(1); + expect(finishAuthCalls).toBe(1); + expect(exchangedCodeVerifier).toBe("v1"); + expect(storedCodeVerifier).toBe("v1"); + }); + + test("overlapping SDK-order saves emit the first verifier's authorize URL", async () => { + finishAuthError = undefined; + overlappingSDKSaves = true; + saveGate = new Promise((resolve) => { + releaseSave = resolve; + }); + const connected = await connectMCPServer(config, { + onAuthURL: (_name, url) => { + authURLCount += 1; + emittedAuthURL = url; + }, + }); + expect(connected.ok).toBe(true); + if (!connected.ok) return; + callFailuresLeft = 1; + + await expect(connected.client.call("ping", {}, new AbortController().signal)).resolves.toBe(""); + + expect(authURLCount).toBe(1); + expect(waitForCodeCalls).toBe(1); + expect(finishAuthCalls).toBe(1); + expect(exchangedCodeVerifier).toBe("v1"); + expect(storedCodeVerifier).toBe("v1"); + expect(emittedAuthURL).toBe("https://auth.test/authorize?v=v1"); + }); + + test("refresh-skip unfreezes so a later browser episode can save a new verifier", async () => { + refreshSucceeds = true; + finishAuthError = undefined; + const connected = await connectMCPServer(config, { + onAuthURL: () => { + authURLCount += 1; + }, + }); + expect(connected.ok).toBe(true); + if (!connected.ok) return; + + refreshGate = new Promise((resolve) => { + releaseRefresh = resolve; + }); + redirectsPerFailure = 0; + callFailuresLeft = 1; + const first = connected.client.call("first", {}, new AbortController().signal); + while (refreshCalls === 0) await Promise.resolve(); + + const skipped = saveThenRedirect(liveProvider, "v-refresh"); + await Promise.resolve(); + expect(authURLCount).toBe(0); + + releaseRefresh?.(); + await skipped; + await expect(first).resolves.toBe(""); + expect(authURLCount).toBe(0); + expect(waitForCodeCalls).toBe(0); + expect(storedCodeVerifier).toBe("v-refresh"); + + refreshSucceeds = false; + redirectVerifier = "v-later"; + redirectsPerFailure = 1; + callFailuresLeft = 1; + await expect(connected.client.call("second", {}, new AbortController().signal)).resolves.toBe( + "", + ); + + expect(authURLCount).toBe(1); + expect(waitForCodeCalls).toBe(1); + expect(exchangedCodeVerifier).toBe("v-later"); + expect(storedCodeVerifier).toBe("v-later"); + }); + + test("does not clear the cap or fire onAuthorized until a retried tool call succeeds", async () => { + finishAuthError = undefined; + const connected = await connectMCPServer(config, { + onAuthURL: () => { + authURLCount += 1; + }, + onAuthorized: () => { + authorizedCount += 1; + }, + }); + expect(connected.ok).toBe(true); + if (!connected.ok) return; + + callFailuresLeft = 2; + callRedirectsLeft = 1; + await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( + "authorization required", + ); + expect(authorizedCount).toBe(0); + expect(authURLCount).toBe(1); + expect(waitForCodeCalls).toBe(1); + expect(finishAuthCalls).toBe(1); + + finishAuthError = new Error("finishAuth exploded"); + callRedirectsLeft = Number.POSITIVE_INFINITY; + for (let episode = 0; episode < 2; episode += 1) { + callFailuresLeft = 1; + await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( + "finishAuth exploded", + ); + } + expect(authURLCount).toBe(3); + expect(authorizedCount).toBe(0); + + callFailuresLeft = 1; + await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( + "retrying paused", + ); + expect(authURLCount).toBe(3); + expect(authorizedCount).toBe(0); + }); + + test("clears the cap and fires onAuthorized after a retried tool call succeeds", async () => { + finishAuthError = undefined; + const connected = await connectMCPServer(config, { + onAuthURL: () => { + authURLCount += 1; + }, + onAuthorized: () => { + authorizedCount += 1; + }, + }); + expect(connected.ok).toBe(true); + if (!connected.ok) return; + + callFailuresLeft = 2; + callRedirectsLeft = 1; + await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( + "authorization required", + ); + expect(authorizedCount).toBe(0); + + callRedirectsLeft = Number.POSITIVE_INFINITY; + callFailuresLeft = 1; + await expect(connected.client.call("ping", {}, new AbortController().signal)).resolves.toBe(""); + expect(authorizedCount).toBe(1); + expect(authURLCount).toBe(2); + + finishAuthError = new Error("finishAuth exploded"); + for (let episode = 0; episode < MAX_BROWSER_AUTH_ATTEMPTS; episode += 1) { + callFailuresLeft = 1; + await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( + "finishAuth exploded", + ); + } + expect(authURLCount).toBe(2 + MAX_BROWSER_AUTH_ATTEMPTS); + + callFailuresLeft = 1; + await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( + "retrying paused", + ); + expect(authURLCount).toBe(2 + MAX_BROWSER_AUTH_ATTEMPTS); + expect(authorizedCount).toBe(1); + }); }); diff --git a/src/mcp/client.ts b/src/mcp/client.ts index a8fab5400..371f1f371 100644 --- a/src/mcp/client.ts +++ b/src/mcp/client.ts @@ -79,6 +79,10 @@ interface HTTPAuthCoordinator { inFlight?: Promise; refreshInFlight?: Promise; browserFlow?: BrowserAuthFlow; + pkceFrozen?: boolean; + pkceSavePromise?: Promise; + pendingAttempt?: { clear(): void }; + notified?: boolean; probe(): Promise; } @@ -157,22 +161,45 @@ function getOrStartRefresh(context: HTTPAuthContext): Promise { function gateRedirectToAuthorization(context: HTTPAuthContext): void { const inner = context.authProvider.redirectToAuthorization.bind(context.authProvider); + const innerSave = context.authProvider.saveCodeVerifier?.bind(context.authProvider); + if (innerSave !== undefined) { + context.authProvider.saveCodeVerifier = (codeVerifier: string) => { + const coordinator = context.coordinator; + if (coordinator.pkceFrozen) return coordinator.pkceSavePromise ?? Promise.resolve(); + coordinator.pkceFrozen = true; + coordinator.pkceSavePromise = Promise.resolve(innerSave(codeVerifier)); + return coordinator.pkceSavePromise; + }; + } context.authProvider.redirectToAuthorization = async (authorizationUrl: URL) => { const coordinator = context.coordinator; if (coordinator.browserFlow !== undefined) return coordinator.browserFlow.promptEmitted; const refresh = coordinator.refreshInFlight; - if (refresh !== undefined && (await refresh)) return; + if (refresh !== undefined && (await refresh)) { + coordinator.pkceFrozen = false; + delete coordinator.pkceSavePromise; + return; + } const concurrentBrowserFlow = coordinator.browserFlow as BrowserAuthFlow | undefined; if (concurrentBrowserFlow !== undefined) return concurrentBrowserFlow.promptEmitted; - if (!context.interactive) + if (!context.interactive) { + coordinator.pkceFrozen = false; + delete coordinator.pkceSavePromise; throw new Error("Authorization required but no interactive handler is available."); + } - const attempt = beginBrowserAuth(context); - const startPrompt = Promise.withResolvers(); - const promptEmitted = startPrompt.promise.then(() => inner(authorizationUrl)); - coordinator.browserFlow = { attempt, promptEmitted }; - startPrompt.resolve(); - return promptEmitted; + try { + const attempt = beginBrowserAuth(context); + const startPrompt = Promise.withResolvers(); + const promptEmitted = startPrompt.promise.then(() => inner(authorizationUrl)); + coordinator.browserFlow = { attempt, promptEmitted }; + startPrompt.resolve(undefined); + return promptEmitted; + } catch (err) { + coordinator.pkceFrozen = false; + delete coordinator.pkceSavePromise; + throw err; + } }; } @@ -236,7 +263,6 @@ async function driveRecovery(err: UnauthorizedError | OAuthError, context: HTTPA if (coordinator.browserFlow === undefined) { try { await coordinator.probe(); - resetAfterVerifiedRecovery(context); return; } catch (probeErr) { if (!isRecoverableAuthError(probeErr)) throw probeErr; @@ -252,11 +278,14 @@ async function driveRecovery(err: UnauthorizedError | OAuthError, context: HTTPA streamableHTTPTransportOptions(context.authProvider, coordinator.requestSignal), ).finishAuth(code); await coordinator.probe(); - resetAfterVerifiedRecovery(context); } -function resetAfterVerifiedRecovery(context: HTTPAuthContext): void { - context.coordinator.browserFlow?.attempt.clear(); +function completeVerifiedRecovery(context: HTTPAuthContext): void { + const coordinator = context.coordinator; + if (coordinator.notified) return; + coordinator.notified = true; + coordinator.pendingAttempt?.clear(); + delete coordinator.pendingAttempt; context.onAuthorized?.(context.serverName); } @@ -266,10 +295,16 @@ function getOrStartRecovery( ): Promise { const coordinator = context.coordinator; if (coordinator.inFlight !== undefined) return coordinator.inFlight; + coordinator.notified = false; + delete coordinator.pendingAttempt; const shared = Promise.resolve().then(() => driveRecovery(err, context)); coordinator.inFlight = shared; const clear = () => { if (coordinator.inFlight !== shared) return; + if (coordinator.browserFlow?.attempt !== undefined) + coordinator.pendingAttempt = coordinator.browserFlow.attempt; + coordinator.pkceFrozen = false; + delete coordinator.pkceSavePromise; delete coordinator.inFlight; delete coordinator.refreshInFlight; delete coordinator.browserFlow; @@ -313,8 +348,11 @@ async function withHTTPAuthorizationRecovery( return await operation(); } catch (err) { if (context === undefined || !isRecoverableAuthError(err)) throw err; - await awaitRecovery(getOrStartRecovery(err, context), signal); - return operation(); + return retryAfterInteractiveAuth( + () => awaitRecovery(getOrStartRecovery(err, context), signal), + operation, + () => completeVerifiedRecovery(context), + ); } } @@ -423,18 +461,16 @@ async function connectHttp( redirectUrl: callback.redirectUrl, onAuthURL: (name, authUrl) => options.onAuthURL?.(name, authUrl), onAuthorizationState: callback.expectState, - ...(options.signal === undefined - ? { fetchFn: fetchWithConnectAbort(lifecycle.signal) } - : { fetchFn: fetchWithConnectAbort(options.signal) }), + fetchFn: fetchWithConnectAbort(lifecycle.signal), }); makeTransport = () => new StreamableHTTPClientTransport( url, - streamableHTTPTransportOptions(authProvider, options.signal), + streamableHTTPTransportOptions(authProvider, lifecycle.signal), ) as unknown as Transport; const coordinator: HTTPAuthCoordinator = { lifecycle, - requestSignal: options.signal ?? lifecycle.signal, + requestSignal: lifecycle.signal, probe: async () => { const probeClient = new Client({ name: MCP_CLIENT_NAME, version: "1.0.0" }); try { From 3d36855af7f9fa8832a1e93cd61c901bab23fcdc Mon Sep 17 00:00:00 2001 From: Sawyer Date: Wed, 9 Sep 2026 17:18:12 -0700 Subject: [PATCH 06/10] Close MCP recovery abort and cap-clear races --- src/mcp/client-auth-reauth-cap.test.ts | 167 ++++++++++++++++++++++++- src/mcp/client.ts | 84 +++++++++---- src/mcp/oauth-provider.test.ts | 2 +- src/mcp/oauth-provider.ts | 12 +- 4 files changed, 230 insertions(+), 35 deletions(-) diff --git a/src/mcp/client-auth-reauth-cap.test.ts b/src/mcp/client-auth-reauth-cap.test.ts index 309a0be1f..138094058 100644 --- a/src/mcp/client-auth-reauth-cap.test.ts +++ b/src/mcp/client-auth-reauth-cap.test.ts @@ -30,6 +30,9 @@ let refreshGate: Promise | undefined; let releaseRefresh: (() => void) | undefined; let callbackGate: Promise | undefined; let releaseCallback: (() => void) | undefined; +let lastRequestSignal: AbortSignal | undefined; +let retryGate: Promise | undefined; +let releaseRetry: (() => void) | undefined; let saveGate: Promise | undefined; let releaseSave: (() => void) | undefined; interface MockAuthProvider { @@ -45,7 +48,7 @@ const fakeProvider = { refreshToken: async () => { refreshCalls += 1; authEvents.push("refresh"); - await refreshGate; + await waitForOptionalGate(refreshGate, lastRequestSignal); if (!refreshSucceeds) throw new UnauthorizedError("refresh rejected"); return { access_token: "fresh", refresh_token: "refresh-me" }; }, @@ -98,8 +101,12 @@ async function emitRedirects(provider: MockAuthProvider | undefined): Promise { - if (callbackGate === undefined) return Promise.resolve(); +function waitForOptionalGate( + gate: Promise | undefined, + signal: AbortSignal | undefined, +): Promise { + if (gate === undefined) return Promise.resolve(); + if (signal === undefined) return gate; if (signal.aborted) return Promise.reject(signal.reason); return new Promise((resolve, reject) => { const onAbort = () => { @@ -107,13 +114,17 @@ function waitForGate(signal: AbortSignal): Promise { reject(signal.reason); }; signal.addEventListener("abort", onAbort, { once: true }); - void callbackGate?.then(() => { + void gate.then(() => { signal.removeEventListener("abort", onAbort); resolve(); }); }); } +function waitForGate(signal: AbortSignal): Promise { + return waitForOptionalGate(callbackGate, signal); +} + await withMockedModule( import.meta.resolve("@modelcontextprotocol/sdk/client/index.js"), (real: typeof import("@modelcontextprotocol/sdk/client/index.js")) => ({ @@ -147,6 +158,7 @@ await withMockedModule( } throw new UnauthorizedError("authorization required"); } + await retryGate; return { content: [] }; } async close(): Promise {} @@ -160,8 +172,13 @@ await withMockedModule( ...real, StreamableHTTPClientTransport: class { provider?: MockAuthProvider; - constructor(_url: URL, options?: { authProvider?: MockAuthProvider }) { + constructor( + _url: URL, + options?: { authProvider?: MockAuthProvider; requestInit?: RequestInit }, + ) { if (options?.authProvider !== undefined) this.provider = options.authProvider; + const signal = options?.requestInit?.signal; + if (signal !== undefined && signal !== null) lastRequestSignal = signal; } async finishAuth(): Promise { finishAuthCalls += 1; @@ -263,6 +280,9 @@ describe("HTTP MCP re-auth loop prevention", () => { releaseCallback = undefined; saveGate = undefined; releaseSave = undefined; + lastRequestSignal = undefined; + retryGate = undefined; + releaseRetry = undefined; resetBrowserAuthState(); setSystemTime(); }); @@ -383,6 +403,90 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(authURLCount).toBe(1); }); + test("aborted waiter still fires onAuthorized when background finishAuth succeeds", async () => { + finishAuthError = undefined; + callbackGate = new Promise((resolve) => { + releaseCallback = resolve; + }); + const connected = await connectMCPServer(config, { + onAuthURL: () => (authURLCount += 1), + onAuthorized: () => (authorizedCount += 1), + }); + expect(connected.ok).toBe(true); + if (!connected.ok) return; + callFailuresLeft = 1; + const abort = new AbortController(); + const call = connected.client.call("ping", {}, abort.signal); + while (authURLCount === 0 || waitForCodeCalls === 0) await Promise.resolve(); + + abort.abort(new Error("caller stopped")); + await expect(call).rejects.toThrow("caller stopped"); + expect(authorizedCount).toBe(0); + + releaseCallback?.(); + while (finishAuthCalls === 0) await Promise.resolve(); + for (let tick = 0; tick < 20 && authorizedCount === 0; tick += 1) await Promise.resolve(); + expect(authorizedCount).toBe(1); + expect(finishAuthCalls).toBe(1); + + finishAuthError = new Error("finishAuth exploded"); + for (let episode = 0; episode < MAX_BROWSER_AUTH_ATTEMPTS; episode += 1) { + callFailuresLeft = 1; + await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( + "finishAuth exploded", + ); + } + expect(authURLCount).toBe(1 + MAX_BROWSER_AUTH_ATTEMPTS); + callFailuresLeft = 1; + await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( + "retrying paused", + ); + expect(authURLCount).toBe(1 + MAX_BROWSER_AUTH_ATTEMPTS); + }); + + test("refresh-only recovery clears prior browser-cap counts", async () => { + const connected = await connectMCPServer(config, { + onAuthURL: () => (authURLCount += 1), + onAuthorized: () => (authorizedCount += 1), + }); + expect(connected.ok).toBe(true); + if (!connected.ok) return; + + for (let episode = 0; episode < 2; episode += 1) { + callFailuresLeft = 1; + await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( + "finishAuth exploded", + ); + } + expect(authURLCount).toBe(2); + expect(authorizedCount).toBe(0); + + refreshSucceeds = true; + finishAuthError = undefined; + redirectsPerFailure = 0; + callFailuresLeft = 1; + await expect(connected.client.call("ping", {}, new AbortController().signal)).resolves.toBe(""); + expect(authorizedCount).toBe(1); + expect(authURLCount).toBe(2); + + refreshSucceeds = false; + finishAuthError = new Error("finishAuth exploded"); + redirectsPerFailure = 1; + for (let episode = 0; episode < MAX_BROWSER_AUTH_ATTEMPTS; episode += 1) { + callFailuresLeft = 1; + await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( + "finishAuth exploded", + ); + } + expect(authURLCount).toBe(2 + MAX_BROWSER_AUTH_ATTEMPTS); + + callFailuresLeft = 1; + await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( + "retrying paused", + ); + expect(authURLCount).toBe(2 + MAX_BROWSER_AUTH_ATTEMPTS); + }); + test("client close aborts the shared callback waiter", async () => { finishAuthError = undefined; callbackGate = new Promise((resolve) => { @@ -401,6 +505,57 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(finishAuthCalls).toBe(0); }); + test("client close during hung refresh does not emit a browser prompt", async () => { + const connected = await connectMCPServer(config, { onAuthURL: () => (authURLCount += 1) }); + expect(connected.ok).toBe(true); + if (!connected.ok) return; + refreshGate = new Promise(() => undefined); + redirectsPerFailure = 0; + callFailuresLeft = 1; + const call = connected.client.call("ping", {}, new AbortController().signal); + while (refreshCalls === 0) await Promise.resolve(); + + await connected.client.close(); + + await expect(call).rejects.toHaveProperty("name", "AbortError"); + expect(authURLCount).toBe(0); + expect(waitForCodeCalls).toBe(0); + }); + + test("late retry success from a prior recovery does not fire onAuthorized after a new recovery starts", async () => { + finishAuthError = undefined; + const connected = await connectMCPServer(config, { + onAuthURL: () => (authURLCount += 1), + onAuthorized: () => (authorizedCount += 1), + }); + expect(connected.ok).toBe(true); + if (!connected.ok) return; + retryGate = new Promise((resolve) => { + releaseRetry = resolve; + }); + callFailuresLeft = 2; + const first = connected.client.call("first", {}, new AbortController().signal); + const second = connected.client.call("second", {}, new AbortController().signal); + while (callToolCalls < 4) await Promise.resolve(); + + callbackGate = new Promise((resolve) => { + releaseCallback = resolve; + }); + callFailuresLeft = 1; + const third = connected.client.call("third", {}, new AbortController().signal); + while (waitForCodeCalls < 2) await Promise.resolve(); + + releaseRetry?.(); + await expect(first).resolves.toBe(""); + await expect(second).resolves.toBe(""); + expect(authorizedCount).toBe(0); + + releaseCallback?.(); + await expect(third).resolves.toBe(""); + expect(authorizedCount).toBe(1); + expect(authURLCount).toBe(2); + }); + test("does not repeat the SDK refresh after redirecting to authorization", async () => { finishAuthError = undefined; connectFailuresLeft = 1; @@ -516,7 +671,7 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(authURLCount).toBe(3 + MAX_BROWSER_AUTH_ATTEMPTS); }); - test("prompts resume five minutes after the third failed episode without a fourth probe", async () => { + test("prompts resume five minutes after the third failed episode", async () => { const thirdEpisodeAt = new Date("2026-01-01T00:00:00Z").getTime(); setSystemTime(thirdEpisodeAt); connectFailuresLeft = Number.POSITIVE_INFINITY; diff --git a/src/mcp/client.ts b/src/mcp/client.ts index 371f1f371..f25b9982a 100644 --- a/src/mcp/client.ts +++ b/src/mcp/client.ts @@ -75,7 +75,6 @@ interface BrowserAuthFlow { interface HTTPAuthCoordinator { lifecycle: AbortController; - requestSignal: AbortSignal; inFlight?: Promise; refreshInFlight?: Promise; browserFlow?: BrowserAuthFlow; @@ -83,6 +82,8 @@ interface HTTPAuthCoordinator { pkceSavePromise?: Promise; pendingAttempt?: { clear(): void }; notified?: boolean; + recoveryGeneration?: number; + waiters?: number; probe(): Promise; } @@ -113,8 +114,12 @@ function browserAuthCapError(serverName: string): Error { ); } +function browserAuthKey(context: HTTPAuthContext): string { + return `${context.serverName}|${context.url.toString()}`; +} + function beginBrowserAuth(context: HTTPAuthContext): { clear(): void } { - const key = `${context.serverName}|${context.url.toString()}`; + const key = browserAuthKey(context); const entry = browserAuthAttempts.get(key) ?? { count: 0 }; const now = Date.now(); if (entry.cooldownUntil !== undefined) { @@ -135,14 +140,19 @@ function beginBrowserAuth(context: HTTPAuthContext): { clear(): void } { }; } +function unfreezePkce(coordinator: HTTPAuthCoordinator): void { + coordinator.pkceFrozen = false; + delete coordinator.pkceSavePromise; +} + async function tryTokenRefresh(context: HTTPAuthContext): Promise { const refreshToken = (await context.authProvider.tokens?.())?.refresh_token; if (refreshToken === undefined) return false; try { - const tokens = await context.authProvider.refreshToken(refreshToken); - return tokens !== undefined; - } catch { - // Refresh failure is auth-invalid; the browser flow remains the fallback. + await context.authProvider.refreshToken(refreshToken); + return true; + } catch (err) { + if (context.coordinator.lifecycle.signal.aborted) throw err; return false; } } @@ -165,7 +175,12 @@ function gateRedirectToAuthorization(context: HTTPAuthContext): void { if (innerSave !== undefined) { context.authProvider.saveCodeVerifier = (codeVerifier: string) => { const coordinator = context.coordinator; - if (coordinator.pkceFrozen) return coordinator.pkceSavePromise ?? Promise.resolve(); + if (coordinator.pkceFrozen) { + const pending = coordinator.pkceSavePromise; + if (pending === undefined) + throw new Error("PKCE verifier save is frozen without a pending write"); + return pending; + } coordinator.pkceFrozen = true; coordinator.pkceSavePromise = Promise.resolve(innerSave(codeVerifier)); return coordinator.pkceSavePromise; @@ -173,18 +188,17 @@ function gateRedirectToAuthorization(context: HTTPAuthContext): void { } context.authProvider.redirectToAuthorization = async (authorizationUrl: URL) => { const coordinator = context.coordinator; - if (coordinator.browserFlow !== undefined) return coordinator.browserFlow.promptEmitted; + const browserFlowBeforeRefresh = coordinator.browserFlow; + if (browserFlowBeforeRefresh !== undefined) return browserFlowBeforeRefresh.promptEmitted; const refresh = coordinator.refreshInFlight; if (refresh !== undefined && (await refresh)) { - coordinator.pkceFrozen = false; - delete coordinator.pkceSavePromise; + unfreezePkce(coordinator); return; } - const concurrentBrowserFlow = coordinator.browserFlow as BrowserAuthFlow | undefined; + const concurrentBrowserFlow = coordinator.browserFlow ?? browserFlowBeforeRefresh; if (concurrentBrowserFlow !== undefined) return concurrentBrowserFlow.promptEmitted; if (!context.interactive) { - coordinator.pkceFrozen = false; - delete coordinator.pkceSavePromise; + unfreezePkce(coordinator); throw new Error("Authorization required but no interactive handler is available."); } @@ -196,8 +210,7 @@ function gateRedirectToAuthorization(context: HTTPAuthContext): void { startPrompt.resolve(undefined); return promptEmitted; } catch (err) { - coordinator.pkceFrozen = false; - delete coordinator.pkceSavePromise; + unfreezePkce(coordinator); throw err; } }; @@ -275,17 +288,19 @@ async function driveRecovery(err: UnauthorizedError | OAuthError, context: HTTPA const code = await context.callback.waitForCode(coordinator.lifecycle.signal); await new StreamableHTTPClientTransport( context.url, - streamableHTTPTransportOptions(context.authProvider, coordinator.requestSignal), + streamableHTTPTransportOptions(context.authProvider, coordinator.lifecycle.signal), ).finishAuth(code); await coordinator.probe(); } -function completeVerifiedRecovery(context: HTTPAuthContext): void { +function completeVerifiedRecovery(context: HTTPAuthContext, generation: number): void { const coordinator = context.coordinator; + if (coordinator.recoveryGeneration !== generation) return; if (coordinator.notified) return; coordinator.notified = true; coordinator.pendingAttempt?.clear(); delete coordinator.pendingAttempt; + browserAuthAttempts.delete(browserAuthKey(context)); context.onAuthorized?.(context.serverName); } @@ -295,6 +310,7 @@ function getOrStartRecovery( ): Promise { const coordinator = context.coordinator; if (coordinator.inFlight !== undefined) return coordinator.inFlight; + coordinator.recoveryGeneration = (coordinator.recoveryGeneration ?? 0) + 1; coordinator.notified = false; delete coordinator.pendingAttempt; const shared = Promise.resolve().then(() => driveRecovery(err, context)); @@ -303,13 +319,20 @@ function getOrStartRecovery( if (coordinator.inFlight !== shared) return; if (coordinator.browserFlow?.attempt !== undefined) coordinator.pendingAttempt = coordinator.browserFlow.attempt; - coordinator.pkceFrozen = false; - delete coordinator.pkceSavePromise; + unfreezePkce(coordinator); delete coordinator.inFlight; delete coordinator.refreshInFlight; delete coordinator.browserFlow; }; - void shared.then(clear, clear); + void shared.then( + () => { + clear(); + if ((coordinator.waiters ?? 0) === 0) { + completeVerifiedRecovery(context, coordinator.recoveryGeneration ?? 0); + } + }, + clear, + ); return shared; } @@ -348,11 +371,21 @@ async function withHTTPAuthorizationRecovery( return await operation(); } catch (err) { if (context === undefined || !isRecoverableAuthError(err)) throw err; - return retryAfterInteractiveAuth( - () => awaitRecovery(getOrStartRecovery(err, context), signal), - operation, - () => completeVerifiedRecovery(context), - ); + const coordinator = context.coordinator; + coordinator.waiters = (coordinator.waiters ?? 0) + 1; + let generation = 0; + try { + return await retryAfterInteractiveAuth( + async () => { + await awaitRecovery(getOrStartRecovery(err, context), signal); + generation = coordinator.recoveryGeneration ?? 0; + }, + operation, + () => completeVerifiedRecovery(context, generation), + ); + } finally { + coordinator.waiters = (coordinator.waiters ?? 1) - 1; + } } } @@ -470,7 +503,6 @@ async function connectHttp( ) as unknown as Transport; const coordinator: HTTPAuthCoordinator = { lifecycle, - requestSignal: lifecycle.signal, probe: async () => { const probeClient = new Client({ name: MCP_CLIENT_NAME, version: "1.0.0" }); try { diff --git a/src/mcp/oauth-provider.test.ts b/src/mcp/oauth-provider.test.ts index 24441e8c8..2e7a52257 100644 --- a/src/mcp/oauth-provider.test.ts +++ b/src/mcp/oauth-provider.test.ts @@ -785,8 +785,8 @@ describe("createOAuthProvider", () => { const pending = provider.refreshToken("refresh-me"); while (seen.every((signal) => signal !== abort.signal)) await Promise.resolve(); abort.abort(new Error("toolset disposed")); - await expect(pending).rejects.toBeInstanceOf(UnauthorizedError); await expect(pending).rejects.toThrow("toolset disposed"); + await expect(pending).rejects.not.toBeInstanceOf(UnauthorizedError); expect(seen.some((signal) => signal === abort.signal)).toBe(true); }); }); diff --git a/src/mcp/oauth-provider.ts b/src/mcp/oauth-provider.ts index 64d625a4b..f27501d17 100644 --- a/src/mcp/oauth-provider.ts +++ b/src/mcp/oauth-provider.ts @@ -50,6 +50,12 @@ function redirectUrisInclude( return uris.includes(redirectUrl); } +function isAbortError(err: unknown): boolean { + if (typeof err !== "object" || err === null) return false; + if ("name" in err && err.name === "AbortError") return true; + return err instanceof Error && err.name === "Error"; +} + // Dynamic client registration bakes in the loopback redirect_uri (ephemeral port). // A later session that binds a new port cannot reuse that client_id for authorize // / token exchange — drop the stale registration when we have no refreshable @@ -253,9 +259,10 @@ export async function createOAuthProvider( resourceUrlFromServerUrl(opts.serverURL); } if (authorizationServerMetadata === undefined) - throw new Error("authorization server metadata unavailable"); + throw new UnauthorizedError("authorization server metadata unavailable"); const clientInformation = stored.clientInformation; - if (clientInformation === undefined) throw new Error("no client registration to refresh"); + if (clientInformation === undefined) + throw new UnauthorizedError("no client registration to refresh"); const resourceURL = resource ?? resourceUrlFromServerUrl(opts.serverURL); const tokens = await refreshAuthorization(authorizationServerUrl, { metadata: authorizationServerMetadata, @@ -269,6 +276,7 @@ export async function createOAuthProvider( }); return tokens; } catch (err) { + if (isAbortError(err) || err instanceof UnauthorizedError) throw err; throw new UnauthorizedError( `Token refresh failed for ${opts.serverName}: ${err instanceof Error ? err.message : String(err)}`, ); From 9373811f4dc4b2ef33a7c2bce54f8b2e073e857d Mon Sep 17 00:00:00 2001 From: Sawyer Date: Wed, 9 Sep 2026 17:19:13 -0700 Subject: [PATCH 07/10] Reformat MCP recovery completion callback --- src/mcp/client.ts | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/mcp/client.ts b/src/mcp/client.ts index f25b9982a..a560c9e74 100644 --- a/src/mcp/client.ts +++ b/src/mcp/client.ts @@ -324,15 +324,12 @@ function getOrStartRecovery( delete coordinator.refreshInFlight; delete coordinator.browserFlow; }; - void shared.then( - () => { - clear(); - if ((coordinator.waiters ?? 0) === 0) { - completeVerifiedRecovery(context, coordinator.recoveryGeneration ?? 0); - } - }, - clear, - ); + void shared.then(() => { + clear(); + if ((coordinator.waiters ?? 0) === 0) { + completeVerifiedRecovery(context, coordinator.recoveryGeneration ?? 0); + } + }, clear); return shared; } From ff2bea7fb2f4b100b2e9e7ed6c4bb8df50051ce7 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Wed, 9 Sep 2026 17:20:06 -0700 Subject: [PATCH 08/10] Treat only AbortError as a cancelled OAuth refresh --- src/mcp/oauth-provider.test.ts | 2 +- src/mcp/oauth-provider.ts | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/mcp/oauth-provider.test.ts b/src/mcp/oauth-provider.test.ts index 2e7a52257..4328a401c 100644 --- a/src/mcp/oauth-provider.test.ts +++ b/src/mcp/oauth-provider.test.ts @@ -784,7 +784,7 @@ describe("createOAuthProvider", () => { const pending = provider.refreshToken("refresh-me"); while (seen.every((signal) => signal !== abort.signal)) await Promise.resolve(); - abort.abort(new Error("toolset disposed")); + abort.abort(new DOMException("toolset disposed", "AbortError")); await expect(pending).rejects.toThrow("toolset disposed"); await expect(pending).rejects.not.toBeInstanceOf(UnauthorizedError); expect(seen.some((signal) => signal === abort.signal)).toBe(true); diff --git a/src/mcp/oauth-provider.ts b/src/mcp/oauth-provider.ts index f27501d17..615a3e488 100644 --- a/src/mcp/oauth-provider.ts +++ b/src/mcp/oauth-provider.ts @@ -51,9 +51,7 @@ function redirectUrisInclude( } function isAbortError(err: unknown): boolean { - if (typeof err !== "object" || err === null) return false; - if ("name" in err && err.name === "AbortError") return true; - return err instanceof Error && err.name === "Error"; + return typeof err === "object" && err !== null && "name" in err && err.name === "AbortError"; } // Dynamic client registration bakes in the loopback redirect_uri (ephemeral port). From a31c01c5430f7b9c5f78cd713462d44219782dba Mon Sep 17 00:00:00 2001 From: Sawyer Date: Wed, 9 Sep 2026 19:15:35 -0700 Subject: [PATCH 09/10] Clear MCP auth chrome when close aborts a verified retry --- src/mcp/client-auth-reauth-cap.test.ts | 23 ++++++++++++++++++++++- src/mcp/client-auth-retry.test.ts | 16 ++++++++++++++++ src/mcp/client.ts | 21 +++++++++++++++------ 3 files changed, 53 insertions(+), 7 deletions(-) diff --git a/src/mcp/client-auth-reauth-cap.test.ts b/src/mcp/client-auth-reauth-cap.test.ts index 138094058..d94804d03 100644 --- a/src/mcp/client-auth-reauth-cap.test.ts +++ b/src/mcp/client-auth-reauth-cap.test.ts @@ -158,7 +158,7 @@ await withMockedModule( } throw new UnauthorizedError("authorization required"); } - await retryGate; + await waitForOptionalGate(retryGate, lastRequestSignal); return { content: [] }; } async close(): Promise {} @@ -522,6 +522,27 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(waitForCodeCalls).toBe(0); }); + test("client close after recovery during retry still fires onAuthorized", async () => { + finishAuthError = undefined; + retryGate = new Promise(() => undefined); + const connected = await connectMCPServer(config, { + onAuthURL: () => (authURLCount += 1), + onAuthorized: () => (authorizedCount += 1), + }); + expect(connected.ok).toBe(true); + if (!connected.ok) return; + callFailuresLeft = 1; + const call = connected.client.call("ping", {}, new AbortController().signal); + while (finishAuthCalls === 0 || callToolCalls < 2) await Promise.resolve(); + expect(authorizedCount).toBe(0); + + await connected.client.close(); + + await expect(call).rejects.toHaveProperty("name", "AbortError"); + expect(authorizedCount).toBe(1); + expect(authURLCount).toBe(1); + }); + test("late retry success from a prior recovery does not fire onAuthorized after a new recovery starts", async () => { finishAuthError = undefined; const connected = await connectMCPServer(config, { diff --git a/src/mcp/client-auth-retry.test.ts b/src/mcp/client-auth-retry.test.ts index 98b63152f..ddfde5b9d 100644 --- a/src/mcp/client-auth-retry.test.ts +++ b/src/mcp/client-auth-retry.test.ts @@ -48,4 +48,20 @@ describe("retryAfterInteractiveAuth", () => { expect(operationRan).toBe(false); expect(notified).toBe(false); }); + + test("notifies when the retried operation is aborted after auth completes", async () => { + let notified = false; + await expect( + retryAfterInteractiveAuth( + async () => undefined, + async () => { + throw new DOMException("toolset disposed", "AbortError"); + }, + () => { + notified = true; + }, + ), + ).rejects.toHaveProperty("name", "AbortError"); + expect(notified).toBe(true); + }); }); diff --git a/src/mcp/client.ts b/src/mcp/client.ts index a560c9e74..94b052d4b 100644 --- a/src/mcp/client.ts +++ b/src/mcp/client.ts @@ -91,6 +91,10 @@ function isRecoverableAuthError(err: unknown): err is UnauthorizedError | OAuthE return err instanceof UnauthorizedError || err instanceof OAuthError; } +function isAbortError(err: unknown): boolean { + return typeof err === "object" && err !== null && "name" in err && err.name === "AbortError"; +} + export const MAX_BROWSER_AUTH_ATTEMPTS = 3; export const BROWSER_AUTH_COOLDOWN_MS = 5 * 60_000; @@ -249,9 +253,9 @@ function streamableHTTPTransportOptions( } /** - * Run interactive OAuth, retry the failed operation, and notify only when the - * retry itself succeeded — a failed re-auth must leave standing "needs auth" - * chrome alone. + * Run interactive OAuth, retry the failed operation, and notify when the retry + * succeeds or is aborted after auth completed — a failed re-auth must leave + * standing "needs auth" chrome alone. */ export async function retryAfterInteractiveAuth( completeAuth: () => Promise, @@ -259,9 +263,14 @@ export async function retryAfterInteractiveAuth( onAuthorized: (() => void) | undefined, ): Promise { await completeAuth(); - const value = await operation(); - onAuthorized?.(); - return value; + try { + const value = await operation(); + onAuthorized?.(); + return value; + } catch (err) { + if (isAbortError(err)) onAuthorized?.(); + throw err; + } } async function driveRecovery(err: UnauthorizedError | OAuthError, context: HTTPAuthContext) { From 7b9ed3d0e5322979ce43e14d35dae2fb2a36783f Mon Sep 17 00:00:00 2001 From: Sawyer Date: Wed, 9 Sep 2026 19:47:03 -0700 Subject: [PATCH 10/10] Cap MCP browser re-auth at one prompt and time out ignored login Reconnect during an in-flight browser wait was starting a new coordinator and prompting again. An ignored tab left connect pending until abort. --- src/mcp/client-auth-reauth-cap.test.ts | 123 ++++++++++++++----------- src/mcp/client.ts | 34 ++++++- 2 files changed, 100 insertions(+), 57 deletions(-) diff --git a/src/mcp/client-auth-reauth-cap.test.ts b/src/mcp/client-auth-reauth-cap.test.ts index d94804d03..611f38234 100644 --- a/src/mcp/client-auth-reauth-cap.test.ts +++ b/src/mcp/client-auth-reauth-cap.test.ts @@ -231,6 +231,7 @@ await withMockedModule( const { connectMCPServer, resetBrowserAuthState, + setBrowserAuthWaitMs, MAX_BROWSER_AUTH_ATTEMPTS, BROWSER_AUTH_COOLDOWN_MS, } = await import("./client.js"); @@ -452,13 +453,11 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(connected.ok).toBe(true); if (!connected.ok) return; - for (let episode = 0; episode < 2; episode += 1) { - callFailuresLeft = 1; - await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( - "finishAuth exploded", - ); - } - expect(authURLCount).toBe(2); + callFailuresLeft = 1; + await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( + "finishAuth exploded", + ); + expect(authURLCount).toBe(1); expect(authorizedCount).toBe(0); refreshSucceeds = true; @@ -467,7 +466,7 @@ describe("HTTP MCP re-auth loop prevention", () => { callFailuresLeft = 1; await expect(connected.client.call("ping", {}, new AbortController().signal)).resolves.toBe(""); expect(authorizedCount).toBe(1); - expect(authURLCount).toBe(2); + expect(authURLCount).toBe(1); refreshSucceeds = false; finishAuthError = new Error("finishAuth exploded"); @@ -478,13 +477,13 @@ describe("HTTP MCP re-auth loop prevention", () => { "finishAuth exploded", ); } - expect(authURLCount).toBe(2 + MAX_BROWSER_AUTH_ATTEMPTS); + expect(authURLCount).toBe(1 + MAX_BROWSER_AUTH_ATTEMPTS); callFailuresLeft = 1; await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( "retrying paused", ); - expect(authURLCount).toBe(2 + MAX_BROWSER_AUTH_ATTEMPTS); + expect(authURLCount).toBe(1 + MAX_BROWSER_AUTH_ATTEMPTS); }); test("client close aborts the shared callback waiter", async () => { @@ -559,22 +558,20 @@ describe("HTTP MCP re-auth loop prevention", () => { const second = connected.client.call("second", {}, new AbortController().signal); while (callToolCalls < 4) await Promise.resolve(); - callbackGate = new Promise((resolve) => { - releaseCallback = resolve; - }); + retryGate = undefined; + refreshSucceeds = true; + redirectsPerFailure = 0; callFailuresLeft = 1; const third = connected.client.call("third", {}, new AbortController().signal); - while (waitForCodeCalls < 2) await Promise.resolve(); + await expect(third).resolves.toBe(""); + expect(authorizedCount).toBe(1); + expect(authURLCount).toBe(1); releaseRetry?.(); await expect(first).resolves.toBe(""); await expect(second).resolves.toBe(""); - expect(authorizedCount).toBe(0); - - releaseCallback?.(); - await expect(third).resolves.toBe(""); expect(authorizedCount).toBe(1); - expect(authURLCount).toBe(2); + expect(waitForCodeCalls).toBe(1); }); test("does not repeat the SDK refresh after redirecting to authorization", async () => { @@ -590,7 +587,29 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(authURLCount).toBe(1); }); - test("live-call auth episodes share redirect state and stop after three prompts", async () => { + test("reconnect during in-flight waitForCode does not emit a second prompt", async () => { + connectFailuresLeft = Number.POSITIVE_INFINITY; + callbackGate = new Promise(() => undefined); + const firstAbort = new AbortController(); + const first = connectMCPServer(config, { + onAuthURL: () => (authURLCount += 1), + signal: firstAbort.signal, + }); + while (authURLCount === 0 || waitForCodeCalls === 0) await Promise.resolve(); + expect(authURLCount).toBe(1); + + const second = await connectWithAuthPrompt(); + + expect(second.ok).toBe(false); + expect(second.error).toContain("retrying paused"); + expect(authURLCount).toBe(1); + expect(waitForCodeCalls).toBe(1); + + firstAbort.abort(); + await expect(first).resolves.toMatchObject({ ok: false }); + }); + + test("live-call auth episodes share redirect state and stop after one prompt", async () => { const connected = await connectMCPServer(config, { onAuthURL: () => { authURLCount += 1; @@ -650,7 +669,7 @@ describe("HTTP MCP re-auth loop prevention", () => { const result = await connectWithAuthPrompt(); expect(result.ok).toBe(false); expect(result.error).toContain( - `MCP authorization for linear failed after ${MAX_BROWSER_AUTH_ATTEMPTS} attempts`, + `MCP authorization for linear failed after ${MAX_BROWSER_AUTH_ATTEMPTS} ${MAX_BROWSER_AUTH_ATTEMPTS === 1 ? "attempt" : "attempts"}`, ); expect(result.error).toContain("retrying paused for 5 minutes"); expect(result.error).toContain("Retry later after the cooldown"); @@ -663,19 +682,11 @@ describe("HTTP MCP re-auth loop prevention", () => { }); test("successful interactive auth clears the cap so a later failure can prompt", async () => { - connectFailuresLeft = Number.POSITIVE_INFINITY; - for (let episode = 0; episode < 2; episode += 1) { - const result = await connectWithAuthPrompt(); - expect(result.ok).toBe(false); - expect(result.error).toContain("finishAuth exploded"); - } - expect(authURLCount).toBe(2); - finishAuthError = undefined; connectFailuresLeft = 1; const recovered = await connectWithAuthPrompt(); expect(recovered.ok).toBe(true); - expect(authURLCount).toBe(3); + expect(authURLCount).toBe(1); finishAuthError = new Error("finishAuth exploded"); connectFailuresLeft = Number.POSITIVE_INFINITY; @@ -684,15 +695,15 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(result.ok).toBe(false); expect(result.error).toContain("finishAuth exploded"); } - expect(authURLCount).toBe(3 + MAX_BROWSER_AUTH_ATTEMPTS); + expect(authURLCount).toBe(1 + MAX_BROWSER_AUTH_ATTEMPTS); const capped = await connectWithAuthPrompt(); expect(capped.ok).toBe(false); expect(capped.error).toContain("retrying paused"); - expect(authURLCount).toBe(3 + MAX_BROWSER_AUTH_ATTEMPTS); + expect(authURLCount).toBe(1 + MAX_BROWSER_AUTH_ATTEMPTS); }); - test("prompts resume five minutes after the third failed episode", async () => { + test("prompts resume five minutes after the capped failed episode", async () => { const thirdEpisodeAt = new Date("2026-01-01T00:00:00Z").getTime(); setSystemTime(thirdEpisodeAt); connectFailuresLeft = Number.POSITIVE_INFINITY; @@ -847,20 +858,11 @@ describe("HTTP MCP re-auth loop prevention", () => { finishAuthError = new Error("finishAuth exploded"); callRedirectsLeft = Number.POSITIVE_INFINITY; - for (let episode = 0; episode < 2; episode += 1) { - callFailuresLeft = 1; - await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( - "finishAuth exploded", - ); - } - expect(authURLCount).toBe(3); - expect(authorizedCount).toBe(0); - callFailuresLeft = 1; await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( "retrying paused", ); - expect(authURLCount).toBe(3); + expect(authURLCount).toBe(1); expect(authorizedCount).toBe(0); }); @@ -877,33 +879,46 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(connected.ok).toBe(true); if (!connected.ok) return; - callFailuresLeft = 2; - callRedirectsLeft = 1; - await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( - "authorization required", - ); - expect(authorizedCount).toBe(0); - - callRedirectsLeft = Number.POSITIVE_INFINITY; callFailuresLeft = 1; + callRedirectsLeft = 1; await expect(connected.client.call("ping", {}, new AbortController().signal)).resolves.toBe(""); expect(authorizedCount).toBe(1); - expect(authURLCount).toBe(2); + expect(authURLCount).toBe(1); finishAuthError = new Error("finishAuth exploded"); + callRedirectsLeft = Number.POSITIVE_INFINITY; for (let episode = 0; episode < MAX_BROWSER_AUTH_ATTEMPTS; episode += 1) { callFailuresLeft = 1; await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( "finishAuth exploded", ); } - expect(authURLCount).toBe(2 + MAX_BROWSER_AUTH_ATTEMPTS); + expect(authURLCount).toBe(1 + MAX_BROWSER_AUTH_ATTEMPTS); callFailuresLeft = 1; await expect(connected.client.call("ping", {}, new AbortController().signal)).rejects.toThrow( "retrying paused", ); - expect(authURLCount).toBe(2 + MAX_BROWSER_AUTH_ATTEMPTS); + expect(authURLCount).toBe(1 + MAX_BROWSER_AUTH_ATTEMPTS); expect(authorizedCount).toBe(1); }); + + test("ignored browser wait times out as disconnected and still counts the prompt", async () => { + setBrowserAuthWaitMs(50); + connectFailuresLeft = Number.POSITIVE_INFINITY; + callbackGate = new Promise(() => undefined); + + const result = await connectWithAuthPrompt(); + + expect(result.ok).toBe(false); + expect(result.error).toContain("timed out waiting for the browser"); + expect(result.error).toContain("disconnected"); + expect(authURLCount).toBe(1); + expect(waitForCodeCalls).toBe(1); + + const capped = await connectWithAuthPrompt(); + expect(capped.ok).toBe(false); + expect(capped.error).toContain("retrying paused"); + expect(authURLCount).toBe(1); + }); }); diff --git a/src/mcp/client.ts b/src/mcp/client.ts index 94b052d4b..1f25a126d 100644 --- a/src/mcp/client.ts +++ b/src/mcp/client.ts @@ -95,8 +95,9 @@ function isAbortError(err: unknown): boolean { return typeof err === "object" && err !== null && "name" in err && err.name === "AbortError"; } -export const MAX_BROWSER_AUTH_ATTEMPTS = 3; +export const MAX_BROWSER_AUTH_ATTEMPTS = 1; export const BROWSER_AUTH_COOLDOWN_MS = 5 * 60_000; +export const BROWSER_AUTH_WAIT_MS = 2 * 60_000; interface BrowserAuthAttempts { count: number; @@ -105,19 +106,46 @@ interface BrowserAuthAttempts { // Keyed by server identity, not provider instance, so the cap survives the // provider re-creation that every reconnect performs. const browserAuthAttempts = new Map(); +let browserAuthWaitMs = BROWSER_AUTH_WAIT_MS; export function resetBrowserAuthState(): void { browserAuthAttempts.clear(); + browserAuthWaitMs = BROWSER_AUTH_WAIT_MS; +} + +export function setBrowserAuthWaitMs(ms: number): void { + browserAuthWaitMs = ms; } function browserAuthCapError(serverName: string): Error { const minutes = Math.round(BROWSER_AUTH_COOLDOWN_MS / 60_000); + const attempts = + MAX_BROWSER_AUTH_ATTEMPTS === 1 ? "1 attempt" : `${String(MAX_BROWSER_AUTH_ATTEMPTS)} attempts`; return new Error( - `MCP authorization for ${serverName} failed after ${MAX_BROWSER_AUTH_ATTEMPTS} attempts; ` + + `MCP authorization for ${serverName} failed after ${attempts}; ` + `retrying paused for ${minutes} minutes. Retry later after the cooldown.`, ); } +function browserAuthWaitError(serverName: string): Error { + return new Error( + `MCP authorization for ${serverName} timed out waiting for the browser; ` + + `the server is disconnected. Retry later after the cooldown.`, + ); +} + +async function waitForBrowserAuthCode(context: HTTPAuthContext): Promise { + const lifecycle = context.coordinator.lifecycle.signal; + const deadline = AbortSignal.any([lifecycle, AbortSignal.timeout(browserAuthWaitMs)]); + try { + return await context.callback.waitForCode(deadline); + } catch (err) { + if (lifecycle.aborted) throw err; + if (deadline.aborted) throw browserAuthWaitError(context.serverName); + throw err; + } +} + function browserAuthKey(context: HTTPAuthContext): string { return `${context.serverName}|${context.url.toString()}`; } @@ -294,7 +322,7 @@ async function driveRecovery(err: UnauthorizedError | OAuthError, context: HTTPA const browserFlow = coordinator.browserFlow; await browserFlow.promptEmitted; - const code = await context.callback.waitForCode(coordinator.lifecycle.signal); + const code = await waitForBrowserAuthCode(context); await new StreamableHTTPClientTransport( context.url, streamableHTTPTransportOptions(context.authProvider, coordinator.lifecycle.signal),