From d355be7dd0d1421323b8b2bf5106282e006bc4ae Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 3 Apr 2026 15:26:43 -0400 Subject: [PATCH] fix: clear stale client registration on OAuth flow failure When a stored client_id is no longer recognized by the OAuth server, the flow fails but the stale client stays in MongoDB, causing every retry to reuse the same invalid registration in an infinite loop. On OAuth failure, clear the stored client registration so the next attempt falls through to fresh Dynamic Client Registration. - Add MCPTokenStorage.deleteClientRegistration() for targeted cleanup - Call it from MCPConnectionFactory's OAuth failure path - Add integration test proving recovery from stale client reuse --- packages/api/src/mcp/MCPConnectionFactory.ts | 12 +++- .../MCPOAuthClientRegistrationReuse.test.ts | 60 +++++++++++++++++++ packages/api/src/mcp/oauth/tokens.ts | 21 +++++++ 3 files changed, 92 insertions(+), 1 deletion(-) diff --git a/packages/api/src/mcp/MCPConnectionFactory.ts b/packages/api/src/mcp/MCPConnectionFactory.ts index 00d381a437..a305cb00a6 100644 --- a/packages/api/src/mcp/MCPConnectionFactory.ts +++ b/packages/api/src/mcp/MCPConnectionFactory.ts @@ -413,7 +413,17 @@ export class MCPConnectionFactory { if (result?.tokens) { connection.emit('oauthHandled'); } else { - // OAuth failed, emit oauthFailed to properly reject the promise + // 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); + }); + } 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 92ca017430..6a79880fa4 100644 --- a/packages/api/src/mcp/__tests__/MCPOAuthClientRegistrationReuse.test.ts +++ b/packages/api/src/mcp/__tests__/MCPOAuthClientRegistrationReuse.test.ts @@ -283,6 +283,66 @@ describe('MCPOAuthHandler - client registration reuse on reconnection', () => { expect(result.flowMetadata.clientInfo?.client_id).toBeTruthy(); }); + it('should not reuse a stale client on retry after a failed flow', async () => { + server = await createOAuthMCPServer({ tokenTTLMs: 60000 }); + const tokenStore = new InMemoryTokenStore(); + + // Seed a stored client with a client_id that the OAuth server doesn't recognize + 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: 'stale-client-that-oauth-server-deleted', + client_secret: 'stale-secret', + redirect_uris: ['http://localhost:3080/api/mcp/test-server/oauth/callback'], + } as OAuthClientInformation & { redirect_uris: string[] }, + }); + + // First attempt: reuses the stale client (this is expected — we don't know it's stale yet) + const firstResult = await MCPOAuthHandler.initiateOAuthFlow( + 'test-server', + server.url, + 'user-1', + {}, + undefined, + undefined, + tokenStore.findToken, + ); + expect(firstResult.flowMetadata.clientInfo?.client_id).toBe( + 'stale-client-that-oauth-server-deleted', + ); + 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 + await MCPTokenStorage.deleteClientRegistration({ + userId: 'user-1', + serverName: 'test-server', + deleteTokens: tokenStore.deleteToken, + }); + + // Second attempt (retry after failure): should do a fresh DCR + const secondResult = await MCPOAuthHandler.initiateOAuthFlow( + 'test-server', + server.url, + 'user-1', + {}, + undefined, + undefined, + 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', + ); + }); + it('should not call getClientInfoAndMetadata when findToken is not provided', async () => { server = await createOAuthMCPServer({ tokenTTLMs: 60000 }); const spy = jest.spyOn(MCPTokenStorage, 'getClientInfoAndMetadata'); diff --git a/packages/api/src/mcp/oauth/tokens.ts b/packages/api/src/mcp/oauth/tokens.ts index 1e31a64511..d470ace103 100644 --- a/packages/api/src/mcp/oauth/tokens.ts +++ b/packages/api/src/mcp/oauth/tokens.ts @@ -476,6 +476,27 @@ export class MCPTokenStorage { }; } + /** Deletes only the stored client registration for a specific user and server */ + static async deleteClientRegistration({ + userId, + serverName, + deleteTokens, + }: { + userId: string; + serverName: string; + deleteTokens: (query: { userId: string; type: string; identifier: string }) => Promise; + }): Promise { + const identifier = `mcp:${serverName}`; + await deleteTokens({ + userId, + type: 'mcp_oauth_client', + identifier: `${identifier}:client`, + }); + logger.debug( + `[MCPTokenStorage] Cleared stored client registration for ${serverName} (userId: ${userId})`, + ); + } + /** * Deletes all OAuth-related tokens for a specific user and server */