From 978ce2b4ebe458118d6008a7ac568b3a9bdafa67 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 3 Apr 2026 15:47:54 -0400 Subject: [PATCH] fix: validate auth server identity and target cleanup to reused clients - Gate client reuse on authorization server identity: compare stored issuer against freshly discovered metadata before reusing, preventing wrong-client reuse when the MCP server switches auth providers - Add reusedStoredClient flag to MCPOAuthFlowMetadata so cleanup only runs when the failed flow actually reused a stored registration, not on unrelated failures (timeouts, user-denied consent, etc.) - Add cleanup in returnOnOAuth path: when a prior flow that reused a stored client is detected as failed, clear the stale registration before re-initiating - Add tests for issuer mismatch and reusedStoredClient flag assertions --- packages/api/src/mcp/MCPConnectionFactory.ts | 37 +++++++++----- .../MCPOAuthClientRegistrationReuse.test.ts | 48 +++++++++++++++++-- packages/api/src/mcp/oauth/handler.ts | 10 ++++ packages/api/src/mcp/oauth/types.ts | 2 + 4 files changed, 83 insertions(+), 14 deletions(-) diff --git a/packages/api/src/mcp/MCPConnectionFactory.ts b/packages/api/src/mcp/MCPConnectionFactory.ts index a305cb00a6..02deb18c37 100644 --- a/packages/api/src/mcp/MCPConnectionFactory.ts +++ b/packages/api/src/mcp/MCPConnectionFactory.ts @@ -355,7 +355,17 @@ export class MCPConnectionFactory { ); if (existingFlow) { - const oldState = (existingFlow.metadata as MCPOAuthFlowMetadata)?.state; + const oldMeta = existingFlow.metadata as MCPOAuthFlowMetadata | undefined; + if (oldMeta?.reusedStoredClient && this.tokenMethods?.deleteTokens) { + await MCPTokenStorage.deleteClientRegistration({ + userId: this.userId!, + serverName: this.serverName, + deleteTokens: this.tokenMethods.deleteTokens, + }).catch((err) => { + logger.debug(`${this.logPrefix} Failed to clear stale client registration`, err); + }); + } + const oldState = oldMeta?.state; await this.flowManager!.deleteFlow(newFlowId, 'mcp_oauth'); if (oldState) { await MCPOAuthHandler.deleteStateMapping(oldState, this.flowManager!); @@ -413,16 +423,21 @@ export class MCPConnectionFactory { if (result?.tokens) { connection.emit('oauthHandled'); } else { - // OAuth failed — clear stored client registration so the next attempt - // does a fresh DCR instead of reusing a potentially stale client_id - if (this.tokenMethods?.deleteTokens) { - await MCPTokenStorage.deleteClientRegistration({ - userId: this.userId!, - serverName: this.serverName, - deleteTokens: this.tokenMethods.deleteTokens, - }).catch((err) => { - logger.debug(`${this.logPrefix} Failed to clear stale client registration`, err); - }); + // OAuth failed — if we reused a stored client registration, clear it + // so the next attempt falls through to fresh DCR + if (result?.clientInfo && this.tokenMethods?.deleteTokens) { + const flowId = MCPOAuthHandler.generateFlowId(this.userId!, this.serverName); + const failedFlow = await this.flowManager?.getFlowState(flowId, 'mcp_oauth'); + const failedMeta = failedFlow?.metadata as MCPOAuthFlowMetadata | undefined; + if (failedMeta?.reusedStoredClient) { + await MCPTokenStorage.deleteClientRegistration({ + userId: this.userId!, + serverName: this.serverName, + deleteTokens: this.tokenMethods.deleteTokens, + }).catch((err) => { + logger.debug(`${this.logPrefix} Failed to clear stale client registration`, err); + }); + } } logger.warn(`${this.logPrefix} OAuth failed, emitting oauthFailed event`); connection.emit('oauthFailed', new Error('OAuth authentication failed')); diff --git a/packages/api/src/mcp/__tests__/MCPOAuthClientRegistrationReuse.test.ts b/packages/api/src/mcp/__tests__/MCPOAuthClientRegistrationReuse.test.ts index 6a79880fa4..7456b56600 100644 --- a/packages/api/src/mcp/__tests__/MCPOAuthClientRegistrationReuse.test.ts +++ b/packages/api/src/mcp/__tests__/MCPOAuthClientRegistrationReuse.test.ts @@ -145,6 +145,7 @@ describe('MCPOAuthHandler - client registration reuse on reconnection', () => { expect(server.registeredClients.size).toBe(1); expect(secondResult.flowMetadata.clientInfo?.client_id).toBe(firstClientId); + expect(secondResult.flowMetadata.reusedStoredClient).toBe(true); }); it('should reuse the same client when two reconnections fire concurrently with pre-seeded token', async () => { @@ -315,10 +316,11 @@ describe('MCPOAuthHandler - client registration reuse on reconnection', () => { expect(firstResult.flowMetadata.clientInfo?.client_id).toBe( 'stale-client-that-oauth-server-deleted', ); + expect(firstResult.flowMetadata.reusedStoredClient).toBe(true); expect(server.registeredClients.size).toBe(0); - // Simulate flow failure: the OAuth server rejected the stale client_id, - // so the operator clears the stored client registration + // Simulate what MCPConnectionFactory does on failure when reusedStoredClient is set: + // clear the stored client registration so the next attempt does a fresh DCR await MCPTokenStorage.deleteClientRegistration({ userId: 'user-1', serverName: 'test-server', @@ -336,11 +338,51 @@ describe('MCPOAuthHandler - client registration reuse on reconnection', () => { tokenStore.findToken, ); - // Now it registered a new client expect(server.registeredClients.size).toBe(1); expect(secondResult.flowMetadata.clientInfo?.client_id).not.toBe( 'stale-client-that-oauth-server-deleted', ); + expect(secondResult.flowMetadata.reusedStoredClient).toBeUndefined(); + }); + + it('should re-register when stored client was issued by a different authorization server', async () => { + server = await createOAuthMCPServer({ tokenTTLMs: 60000 }); + const tokenStore = new InMemoryTokenStore(); + + // Seed a stored client that was registered with a different issuer + await MCPTokenStorage.storeTokens({ + userId: 'user-1', + serverName: 'test-server', + tokens: { access_token: 'old-token', token_type: 'Bearer' }, + createToken: tokenStore.createToken, + updateToken: tokenStore.updateToken, + findToken: tokenStore.findToken, + clientInfo: { + client_id: 'old-issuer-client', + client_secret: 'secret', + redirect_uris: ['http://localhost:3080/api/mcp/test-server/oauth/callback'], + } as OAuthClientInformation & { redirect_uris: string[] }, + metadata: { + issuer: 'https://old-auth-server.example.com', + authorization_endpoint: 'https://old-auth-server.example.com/authorize', + token_endpoint: 'https://old-auth-server.example.com/token', + }, + }); + + const result = await MCPOAuthHandler.initiateOAuthFlow( + 'test-server', + server.url, + 'user-1', + {}, + undefined, + undefined, + tokenStore.findToken, + ); + + // Should have registered a NEW client because the issuer changed + expect(server.registeredClients.size).toBe(1); + expect(result.flowMetadata.clientInfo?.client_id).not.toBe('old-issuer-client'); + expect(result.flowMetadata.reusedStoredClient).toBeUndefined(); }); it('should not call getClientInfoAndMetadata when findToken is not provided', async () => { diff --git a/packages/api/src/mcp/oauth/handler.ts b/packages/api/src/mcp/oauth/handler.ts index a9ecd3af30..e84ada9755 100644 --- a/packages/api/src/mcp/oauth/handler.ts +++ b/packages/api/src/mcp/oauth/handler.ts @@ -500,6 +500,7 @@ export class MCPOAuthHandler { logger.debug(`[MCPOAuth] Resolving OAuth client with redirect URI: ${redirectUri}`); let clientInfo: OAuthClientInformation | undefined; + let reusedStoredClient = false; if (findToken) { try { @@ -511,15 +512,23 @@ export class MCPOAuthHandler { if (existing?.clientInfo?.client_id) { const storedRedirectUri = (existing.clientInfo as OAuthClientInformation) .redirect_uris?.[0]; + const storedIssuer = (existing.clientMetadata as Record)?.issuer; + const currentIssuer = metadata.issuer ?? authServerUrl.toString(); + if (!storedRedirectUri || storedRedirectUri !== redirectUri) { logger.debug( `[MCPOAuth] Stored redirect_uri "${storedRedirectUri}" differs from current "${redirectUri}", will re-register`, ); + } else if (storedIssuer && storedIssuer !== currentIssuer) { + logger.debug( + `[MCPOAuth] Stored issuer "${storedIssuer}" differs from current "${currentIssuer}", will re-register`, + ); } else { logger.debug( `[MCPOAuth] Reusing existing client registration: ${existing.clientInfo.client_id}`, ); clientInfo = existing.clientInfo; + reusedStoredClient = true; } } } catch (error) { @@ -610,6 +619,7 @@ export class MCPOAuthHandler { metadata, resourceMetadata, ...(Object.keys(oauthHeaders).length > 0 && { oauthHeaders }), + ...(reusedStoredClient && { reusedStoredClient }), }; logger.debug( diff --git a/packages/api/src/mcp/oauth/types.ts b/packages/api/src/mcp/oauth/types.ts index bc5f53f60c..ee8ce2d76d 100644 --- a/packages/api/src/mcp/oauth/types.ts +++ b/packages/api/src/mcp/oauth/types.ts @@ -91,6 +91,8 @@ export interface MCPOAuthFlowMetadata extends FlowMetadata { authorizationUrl?: string; /** Custom headers for OAuth token exchange, persisted at flow initiation for the callback. */ oauthHeaders?: Record; + /** True when the flow reused a stored client registration from a prior successful OAuth flow */ + reusedStoredClient?: boolean; } export interface MCPOAuthTokens extends OAuthTokens {