diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index 9c67c10edf..2e1dc33b05 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -823,7 +823,7 @@ export class McpHub { transport.onerror = async (error) => { console.error(`Transport error for "${name}" (streamable-http):`, error) const connection = this.findConnection(name, source) - if (connection) { + if (connection && connection.type === "connected") { if (error instanceof UnauthorizedError && authProvider) { // Mid-session re-auth triggered by a tool call (401) connection.server.status = "connecting" @@ -849,6 +849,9 @@ export class McpHub { return } + connection.server.status = "disconnected" + this.appendErrorMessage(connection, error instanceof Error ? error.message : `${error}`) + } else if (connection) { connection.server.status = "disconnected" this.appendErrorMessage(connection, error instanceof Error ? error.message : `${error}`) } @@ -1950,19 +1953,29 @@ export class McpHub { throw new Error(`Failed to reconnect to server ${serverName} after OAuth`) } - return await newConnection.client.request( - { - method: "tools/call", - params: { - name: toolName, - arguments: toolArguments, + try { + return await newConnection.client.request( + { + method: "tools/call", + params: { + name: toolName, + arguments: toolArguments, + }, }, - }, - CallToolResultSchema, - { - timeout, - }, - ) + CallToolResultSchema, + { + timeout, + }, + ) + } catch (retryError) { + if (retryError instanceof UnauthorizedError) { + throw new Error( + `Authentication succeeded but server "${serverName}" still rejected the request. ` + + `This may indicate a token audience mismatch or server-side configuration issue.`, + ) + } + throw retryError + } } throw error } diff --git a/src/services/mcp/McpOAuthClientProvider.ts b/src/services/mcp/McpOAuthClientProvider.ts index 48ba529ce5..102c826a78 100644 --- a/src/services/mcp/McpOAuthClientProvider.ts +++ b/src/services/mcp/McpOAuthClientProvider.ts @@ -174,7 +174,7 @@ export class McpOAuthClientProvider implements OAuthClientProvider { // Check if we have a cached client_id from previous registration const cachedData = await this._secretStorage.getOAuthData(this._serverUrl) - if (cachedData?.client_id && cachedData.redirect_uri === this.redirectUrl) { + if (cachedData?.client_id) { this._clientInfo = { client_id: cachedData.client_id, redirect_uris: [this.redirectUrl], @@ -219,9 +219,17 @@ export class McpOAuthClientProvider implements OAuthClientProvider { return this._refreshPromise } - this._refreshPromise = this.refreshAccessToken(data.tokens.refresh_token).finally(() => { - this._refreshPromise = null - }) + // Use the client_id stored alongside the tokens — it is the one the + // auth server bound the refresh token to. `this._clientInfo.client_id` + // may differ if a fresh DCR was performed (e.g. after stale token + // cleanup removed the cached data). + const clientIdForRefresh = data.client_id ?? this._clientInfo?.client_id + + this._refreshPromise = this.refreshAccessToken(data.tokens.refresh_token, clientIdForRefresh).finally( + () => { + this._refreshPromise = null + }, + ) try { return await this._refreshPromise @@ -236,13 +244,12 @@ export class McpOAuthClientProvider implements OAuthClientProvider { return undefined } - async saveTokens(tokens: OAuthTokens): Promise { + async saveTokens(tokens: OAuthTokens, clientIdOverride?: string): Promise { const expires_at = tokens.expires_in ? Date.now() + tokens.expires_in * 1000 : Date.now() + 3600 * 1000 // default 1 hour when server omits expires_in await this._secretStorage.saveOAuthData(this._serverUrl, { tokens, expires_at, - client_id: this._clientInfo?.client_id, - redirect_uri: this.redirectUrl, + client_id: clientIdOverride ?? this._clientInfo?.client_id, }) } @@ -374,24 +381,30 @@ export class McpOAuthClientProvider implements OAuthClientProvider { /** * Refreshes the access token using a refresh token. + * * @param refreshToken The refresh token to use. + * @param clientIdOverride Optional client_id to use instead of `this._clientInfo.client_id`. + * This is used when the stored tokens were issued to a different client_id than the + * current in-memory registration (e.g. after a port change caused a new DCR). * @returns The new tokens. */ - async refreshAccessToken(refreshToken: string): Promise { + async refreshAccessToken(refreshToken: string, clientIdOverride?: string): Promise { if (!this._authServerMeta?.token_endpoint) { throw new Error("No token_endpoint in auth server metadata — cannot refresh token") } - if (!this._clientInfo) { + + const clientId = clientIdOverride ?? this._clientInfo?.client_id + if (!clientId) { throw new Error("No client information — registerClientIfNeeded() must be called first") } const params: Record = { grant_type: "refresh_token", refresh_token: refreshToken, - client_id: this._clientInfo.client_id, + client_id: clientId, } - if (this._tokenEndpointAuthMethod === "client_secret_post" && this._clientInfo.client_secret) { + if (this._tokenEndpointAuthMethod === "client_secret_post" && this._clientInfo?.client_secret) { params.client_secret = this._clientInfo.client_secret } @@ -405,11 +418,12 @@ export class McpOAuthClientProvider implements OAuthClientProvider { }) if (!response.ok) { - throw new Error(`Token refresh failed: HTTP ${response.status}`) + const errorBody = await response.text().catch(() => "") + throw new Error(`Token refresh failed: HTTP ${response.status} ${errorBody}`) } const tokens = (await response.json()) as OAuthTokens - await this.saveTokens(tokens) + await this.saveTokens(tokens, clientId) return tokens } diff --git a/src/services/mcp/SecretStorageService.ts b/src/services/mcp/SecretStorageService.ts index 6d49d4f7a1..d3bf4b0132 100644 --- a/src/services/mcp/SecretStorageService.ts +++ b/src/services/mcp/SecretStorageService.ts @@ -7,8 +7,6 @@ export interface StoredMcpOAuthData { expires_at: number /** The client_id used to obtain these tokens (for token reuse without re-registration). */ client_id?: string - /** The redirect_uri used during client registration (to detect port changes). */ - redirect_uri?: string } /** diff --git a/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts b/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts index 9486b75d14..6e6715e924 100644 --- a/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts +++ b/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts @@ -585,7 +585,7 @@ describe("McpOAuthClientProvider", () => { }) describe("registerClientIfNeeded", () => { - it("should reuse cached client_id when redirect_uri matches", async () => { + it("should reuse cached client_id from previous registration", async () => { setupCallbackServerMock() const secretStorage = createMockSecretStorage() @@ -594,7 +594,6 @@ describe("McpOAuthClientProvider", () => { tokens: { access_token: "cached-token", token_type: "Bearer" }, expires_at: Date.now() + 3600000, client_id: "cached-client-id", - redirect_uri: "http://localhost:12345/callback", }) const provider = await McpOAuthClientProvider.create("https://example.com/mcp", secretStorage) @@ -604,7 +603,28 @@ describe("McpOAuthClientProvider", () => { await provider.close() }) - it("should not reuse cached client_id when redirect_uri does not match", async () => { + it("should reuse cached client_id even when callback server port has changed", async () => { + setupCallbackServerMock() + const secretStorage = createMockSecretStorage() + + // Pre-populate storage with cached data — the callback server port (12345) + // may differ from the port used in the original registration, but we still + // reuse the client_id to avoid "refresh token not issued to this client" errors. + await secretStorage.saveOAuthData("https://example.com/mcp", { + tokens: { access_token: "cached-token", token_type: "Bearer" }, + expires_at: Date.now() + 3600000, + client_id: "cached-client-id", + }) + + const provider = await McpOAuthClientProvider.create("https://example.com/mcp", secretStorage) + await provider.registerClientIfNeeded() + + // Should still reuse the cached client_id, NOT perform a new DCR + expect((await provider.clientInformation())?.client_id).toBe("cached-client-id") + await provider.close() + }) + + it("should perform DCR when no cached client_id exists", async () => { setupCallbackServerMock() const secretStorage = createMockSecretStorage() @@ -636,12 +656,10 @@ describe("McpOAuthClientProvider", () => { }), }) - // Pre-populate storage with cached data with different redirect_uri + // No cached client_id in storage await secretStorage.saveOAuthData("https://example.com/mcp", { tokens: { access_token: "cached-token", token_type: "Bearer" }, expires_at: Date.now() + 3600000, - client_id: "cached-client-id", - redirect_uri: "http://localhost:99999/callback", // different port }) const provider = await McpOAuthClientProvider.create("https://example.com/mcp", secretStorage)