diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index 5e9f1a5c63..be5ded9dd3 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -795,24 +795,32 @@ export class McpHub { throw new Error("SecretStorageService not initialized — call setSecretStorage() before connecting") } - // Create an OAuth provider for this server. + // Decide whether to perform OAuth discovery (RFC 9728 + RFC 8414) + // upfront or skip it to avoid a wasted network round-trip for + // non-OAuth servers. // - // McpOAuthClientProvider.create() performs OAuth discovery (RFC 9728 + - // RFC 8414) once and starts the local callback server so the redirect - // URI port is stable before any connect attempt. - // - // If the server already has a stored token the SDK will use it - // transparently; the browser is only opened when a 401 forces a new - // authorization flow. - const authProvider = await McpOAuthClientProvider.create(configInjected.url, this.secretStorage, name) + // Three cases: + // 1. SecretStorage has OAuth data → known OAuth server → discover + // 2. In-memory negative cache says non-OAuth → skip discovery + // 3. Unknown server (first connection) → discover once, cache result + const hasOAuthData = await this.secretStorage.hasOAuthData(configInjected.url) + const knownNonOAuth = McpOAuthClientProvider.isKnownNonOAuth(configInjected.url) + const skipDiscovery = !hasOAuthData && knownNonOAuth + + const authProvider = await McpOAuthClientProvider.create(configInjected.url, this.secretStorage, name, { + skipDiscovery, + }) // Pre-register the OAuth client so the SDK can skip its own // registration step (broken for path-prefixed issuers — see // utils/oauth.ts for upstream issue links). - try { - await authProvider.registerClientIfNeeded() - } catch { - // Registration may not be supported — the SDK will attempt its own. + // Skip when discovery was skipped — there's no metadata to register with. + if (!skipDiscovery) { + try { + await authProvider.registerClientIfNeeded() + } catch { + // Registration may not be supported — the SDK will attempt its own. + } } transport = new StreamableHTTPClientTransport(new URL(configInjected.url), { @@ -959,10 +967,23 @@ export class McpHub { await client.connect(transport) } catch (connectError) { if (connectError instanceof UnauthorizedError && streamableHttpAuthProvider && configInjected.url) { - // The server requires OAuth. Mark this connection as "connecting" and - // detach the toast + browser flow from the initialization path so that + // The server requires OAuth. + McpOAuthClientProvider.clearNonOAuthCache(configInjected.url) + + // If discovery was skipped (provider has no metadata), we need to + // tear down and reconnect with full discovery now that we know + // this is an OAuth server. The reconnect will do discovery since + // the negative cache was cleared and no SecretStorage data exists yet. + if (!streamableHttpAuthProvider.hasMetadata) { + await streamableHttpAuthProvider.close() + await this.deleteConnection(name, source) + void this.connectToServer(name, config, source) + return + } + + // Mark this connection as "connecting" and detach the toast + + // browser flow from the initialization path so that // waitUntilReady() resolves immediately and the MCP panel can load. - const serverUrl = configInjected.url connection.server.status = "connecting" void this._initiateOAuthFlow( diff --git a/src/services/mcp/McpOAuthClientProvider.ts b/src/services/mcp/McpOAuthClientProvider.ts index 0433786400..1017746c31 100644 --- a/src/services/mcp/McpOAuthClientProvider.ts +++ b/src/services/mcp/McpOAuthClientProvider.ts @@ -31,6 +31,35 @@ import { fetchOAuthAuthServerMetadata } from "./utils/oauth" * 6. `await authProvider.close()` when done (success or permanent failure) */ export class McpOAuthClientProvider implements OAuthClientProvider { + // ── Static negative cache ──────────────────────────────────────────────── + // Remembers servers that returned no OAuth metadata so we can skip the + // discovery probe on subsequent connection attempts (reconnect, restart). + private static _nonOAuthCache = new Map() // serverUrl → timestamp + private static NON_OAUTH_TTL_MS = 30 * 60 * 1000 // 30 minutes + + static isKnownNonOAuth(serverUrl: string): boolean { + const ts = McpOAuthClientProvider._nonOAuthCache.get(serverUrl) + if (ts === undefined) return false + if (Date.now() - ts > McpOAuthClientProvider.NON_OAUTH_TTL_MS) { + McpOAuthClientProvider._nonOAuthCache.delete(serverUrl) + return false + } + return true + } + + static markNonOAuth(serverUrl: string): void { + McpOAuthClientProvider._nonOAuthCache.set(serverUrl, Date.now()) + } + + static clearNonOAuthCache(serverUrl?: string): void { + if (serverUrl) { + McpOAuthClientProvider._nonOAuthCache.delete(serverUrl) + } else { + McpOAuthClientProvider._nonOAuthCache.clear() + } + } + + // ── Instance fields ────────────────────────────────────────────────────── private _codeVerifier?: string // Client info is kept in-memory only (not persisted) to avoid stale registrations // when the redirect URI port changes between sessions. @@ -71,14 +100,25 @@ export class McpOAuthClientProvider implements OAuthClientProvider { serverUrl: string, secretStorage: SecretStorageService, serverName?: string, + options?: { skipDiscovery?: boolean }, ): Promise { - // Fetch auth server metadata once. Reused for: - // - selecting token_endpoint_auth_method / grant_types / scopes - // - pre-registering the client (registration_endpoint) - // - RFC 8707 resource indicator (injected into authorization URL) - const discovery = await fetchOAuthAuthServerMetadata(serverUrl) - const authServerMeta = discovery?.authServerMeta ?? null - const resourceIndicator = discovery?.resourceIndicator ?? null + let authServerMeta: Record | null = null + let resourceIndicator: string | null = null + + if (!options?.skipDiscovery) { + // Fetch auth server metadata once. Reused for: + // - selecting token_endpoint_auth_method / grant_types / scopes + // - pre-registering the client (registration_endpoint) + // - RFC 8707 resource indicator (injected into authorization URL) + const discovery = await fetchOAuthAuthServerMetadata(serverUrl) + authServerMeta = discovery?.authServerMeta ?? null + resourceIndicator = discovery?.resourceIndicator ?? null + + // Cache the result so subsequent connections can skip the probe. + if (!authServerMeta) { + McpOAuthClientProvider.markNonOAuth(serverUrl) + } + } // Extract auth-method preferences. // Prefer "none" → first supported → "client_secret_post" @@ -113,6 +153,11 @@ export class McpOAuthClientProvider implements OAuthClientProvider { // ── OAuthClientProvider interface ──────────────────────────────────────── + /** Whether this provider was created with OAuth metadata (discovery succeeded). */ + get hasMetadata(): boolean { + return this._authServerMeta !== null + } + get redirectUrl(): string { return `http://localhost:${this._port}/callback` } diff --git a/src/services/mcp/SecretStorageService.ts b/src/services/mcp/SecretStorageService.ts index 330ccd9aad..abcf201915 100644 --- a/src/services/mcp/SecretStorageService.ts +++ b/src/services/mcp/SecretStorageService.ts @@ -45,6 +45,11 @@ export class SecretStorageService { await this._storage.store(this._key(serverUrl), JSON.stringify(data)) } + async hasOAuthData(serverUrl: string): Promise { + const raw = await this._storage.get(this._key(serverUrl)) + return raw !== undefined + } + async deleteOAuthData(serverUrl: string): Promise { await this._storage.delete(this._key(serverUrl)) } diff --git a/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts b/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts index 669dfbdb2f..575329cb64 100644 --- a/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts +++ b/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts @@ -82,6 +82,43 @@ function setupCallbackServerMock(code = "test-auth-code", state?: string) { describe("McpOAuthClientProvider", () => { beforeEach(() => { vi.clearAllMocks() + McpOAuthClientProvider.clearNonOAuthCache() + }) + + describe("static negative cache", () => { + it("isKnownNonOAuth returns false for unknown servers", () => { + expect(McpOAuthClientProvider.isKnownNonOAuth("https://unknown.com/mcp")).toBe(false) + }) + + it("markNonOAuth makes isKnownNonOAuth return true", () => { + McpOAuthClientProvider.markNonOAuth("https://example.com/mcp") + expect(McpOAuthClientProvider.isKnownNonOAuth("https://example.com/mcp")).toBe(true) + }) + + it("clearNonOAuthCache(url) clears a specific entry", () => { + McpOAuthClientProvider.markNonOAuth("https://a.com/mcp") + McpOAuthClientProvider.markNonOAuth("https://b.com/mcp") + McpOAuthClientProvider.clearNonOAuthCache("https://a.com/mcp") + expect(McpOAuthClientProvider.isKnownNonOAuth("https://a.com/mcp")).toBe(false) + expect(McpOAuthClientProvider.isKnownNonOAuth("https://b.com/mcp")).toBe(true) + }) + + it("clearNonOAuthCache() with no arg clears all entries", () => { + McpOAuthClientProvider.markNonOAuth("https://a.com/mcp") + McpOAuthClientProvider.markNonOAuth("https://b.com/mcp") + McpOAuthClientProvider.clearNonOAuthCache() + expect(McpOAuthClientProvider.isKnownNonOAuth("https://a.com/mcp")).toBe(false) + expect(McpOAuthClientProvider.isKnownNonOAuth("https://b.com/mcp")).toBe(false) + }) + + it("entries expire after the TTL", () => { + const realNow = Date.now() + McpOAuthClientProvider.markNonOAuth("https://example.com/mcp") + // Advance time past the 30-minute TTL + const spy = vi.spyOn(Date, "now").mockReturnValue(realNow + 31 * 60 * 1000) + expect(McpOAuthClientProvider.isKnownNonOAuth("https://example.com/mcp")).toBe(false) + spy.mockRestore() + }) }) describe("create", () => { @@ -95,6 +132,36 @@ describe("McpOAuthClientProvider", () => { expect(provider.redirectUrl).toBe("http://localhost:0/callback") await provider.close() }) + + it("should skip discovery when skipDiscovery option is true", async () => { + const secretStorage = createMockSecretStorage() + const provider = await McpOAuthClientProvider.create("https://example.com/mcp", secretStorage, undefined, { + skipDiscovery: true, + }) + + expect(discoverOAuthProtectedResourceMetadata).not.toHaveBeenCalled() + expect(provider.hasMetadata).toBe(false) + }) + + it("should have hasMetadata true when discovery succeeds", async () => { + const secretStorage = createMockSecretStorage() + const provider = await McpOAuthClientProvider.create("https://example.com/mcp", secretStorage) + + expect(discoverOAuthProtectedResourceMetadata).toHaveBeenCalled() + expect(provider.hasMetadata).toBe(true) + }) + + it("should cache server as non-OAuth when discovery fails", async () => { + // Use mockImplementationOnce to override just this call + ;(discoverOAuthProtectedResourceMetadata as any).mockImplementationOnce(() => { + throw new Error("not found") + }) + const secretStorage = createMockSecretStorage() + const provider = await McpOAuthClientProvider.create("https://no-oauth.example.com/mcp", secretStorage) + + expect(provider.hasMetadata).toBe(false) + expect(McpOAuthClientProvider.isKnownNonOAuth("https://no-oauth.example.com/mcp")).toBe(true) + }) }) describe("redirectUrl (pre-server-start)", () => { diff --git a/src/services/mcp/__tests__/SecretStorageService.spec.ts b/src/services/mcp/__tests__/SecretStorageService.spec.ts index 59bf87ade5..9893ed5fe0 100644 --- a/src/services/mcp/__tests__/SecretStorageService.spec.ts +++ b/src/services/mcp/__tests__/SecretStorageService.spec.ts @@ -89,6 +89,31 @@ describe("SecretStorageService", () => { }) }) + describe("hasOAuthData", () => { + it("should return false when no data stored", async () => { + expect(await service.hasOAuthData("https://example.com/mcp")).toBe(false) + }) + + it("should return true when data is stored", async () => { + const data: StoredMcpOAuthData = { + tokens: { access_token: "tok", token_type: "Bearer" }, + expires_at: Date.now() + 3600_000, + } + await service.saveOAuthData("https://example.com/mcp", data) + expect(await service.hasOAuthData("https://example.com/mcp")).toBe(true) + }) + + it("should return false after data is deleted", async () => { + const data: StoredMcpOAuthData = { + tokens: { access_token: "tok", token_type: "Bearer" }, + expires_at: Date.now() + 3600_000, + } + await service.saveOAuthData("https://example.com/mcp", data) + await service.deleteOAuthData("https://example.com/mcp") + expect(await service.hasOAuthData("https://example.com/mcp")).toBe(false) + }) + }) + describe("deleteOAuthData", () => { it("should delete stored data", async () => { const data: StoredMcpOAuthData = {