From ca60c83aa3fcb9863e3a9c9389d3fd3ae5296143 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 3 Apr 2026 14:55:51 -0400 Subject: [PATCH] fix: address review findings for client registration reuse - Fix empty redirect_uris bug: invert condition so missing/empty redirect_uris triggers re-registration instead of silent reuse - Revert undocumented config?.redirect_uri in auto-discovery path - Change DB error logging from debug to warn for operator visibility - Fix import order: move package type import to correct section - Remove redundant type cast and misleading JSDoc comment - Test file: remove dead imports, restore process.env.DOMAIN_SERVER, rename describe blocks, add empty redirect_uris edge case test, add concurrent reconnection test with pre-seeded token, scope documentation to reconnection stabilization --- .../MCPOAuthClientRegistrationReuse.test.ts | 191 +++++++++++------- packages/api/src/mcp/oauth/handler.ts | 20 +- 2 files changed, 129 insertions(+), 82 deletions(-) diff --git a/packages/api/src/mcp/__tests__/MCPOAuthClientRegistrationReuse.test.ts b/packages/api/src/mcp/__tests__/MCPOAuthClientRegistrationReuse.test.ts index e448add67e..507be31f3a 100644 --- a/packages/api/src/mcp/__tests__/MCPOAuthClientRegistrationReuse.test.ts +++ b/packages/api/src/mcp/__tests__/MCPOAuthClientRegistrationReuse.test.ts @@ -1,7 +1,7 @@ /** - * Tests for MCP OAuth client registration reuse (PR #11925). + * Tests for MCP OAuth client registration reuse on reconnection. * - * Reproduces the client_id mismatch bug in horizontally scaled deployments: + * Documents the client_id mismatch bug in horizontally scaled deployments: * * When LibreChat runs with multiple replicas (e.g., 3 behind a load balancer), * each replica independently calls registerClient() on the OAuth server's /register @@ -16,18 +16,16 @@ * User completes OAuth in browser with client_A in the URL * Callback reads Redis → finds client_B → token exchange fails: "client_id mismatch" * - * The fix: before calling registerClient(), check MongoDB (shared across replicas) - * for an existing client registration and reuse it. This is done by passing - * findToken to initiateOAuthFlow, which looks up MCPTokenStorage.getClientInfoAndMetadata(). - * - * These tests are designed to FAIL on the current dev branch and PASS with PR #11925. + * The fix stabilizes reconnection flows: before calling registerClient(), check + * MongoDB (shared across replicas) for an existing client registration from a prior + * successful OAuth flow and reuse it. This eliminates redundant /register calls on + * reconnection. Note: the first-time concurrent auth race is NOT addressed by this + * fix and would require a distributed lock (e.g., Redis SETNX) around registration. */ -import { Keyv } from 'keyv'; import type { OAuthTestServer } from './helpers/oauthTestServer'; -import { MockKeyv, InMemoryTokenStore, createOAuthMCPServer } from './helpers/oauthTestServer'; +import { InMemoryTokenStore, createOAuthMCPServer } from './helpers/oauthTestServer'; import { MCPOAuthHandler, MCPTokenStorage } from '~/mcp/oauth'; -import { FlowStateManager } from '~/flow/manager'; jest.mock('@librechat/data-schemas', () => ({ logger: { @@ -53,10 +51,17 @@ jest.mock('~/mcp/mcpConfig', () => ({ mcpConfig: { CONNECTION_CHECK_TTL: 0, USER_CONNECTION_IDLE_TIMEOUT: 30 * 60 * 1000 }, })); -describe('MCP OAuth Client Registration Reuse (PR #11925)', () => { +describe('MCPOAuthHandler - client registration reuse on reconnection', () => { let server: OAuthTestServer; + let originalDomainServer: string | undefined; + + beforeEach(() => { + originalDomainServer = process.env.DOMAIN_SERVER; + process.env.DOMAIN_SERVER = 'http://localhost:3080'; + }); afterEach(async () => { + process.env.DOMAIN_SERVER = originalDomainServer; if (server) { await server.close(); } @@ -66,25 +71,16 @@ describe('MCP OAuth Client Registration Reuse (PR #11925)', () => { describe('Race condition reproduction: concurrent replicas re-register', () => { it('should produce duplicate client registrations when two replicas initiate flows concurrently', async () => { server = await createOAuthMCPServer({ tokenTTLMs: 60000 }); - process.env.DOMAIN_SERVER = 'http://localhost:3080'; - const serverName = 'test-server'; - const userId = 'user-1'; - - // Simulate two replicas calling initiateOAuthFlow concurrently - // Both slip past the PENDING flow check (check-then-act race) const [resultA, resultB] = await Promise.all([ - MCPOAuthHandler.initiateOAuthFlow(serverName, server.url, userId, {}), - MCPOAuthHandler.initiateOAuthFlow(serverName, server.url, userId, {}), + MCPOAuthHandler.initiateOAuthFlow('test-server', server.url, 'user-1', {}), + MCPOAuthHandler.initiateOAuthFlow('test-server', server.url, 'user-1', {}), ]); expect(resultA.authorizationUrl).toBeTruthy(); expect(resultB.authorizationUrl).toBeTruthy(); - - // Two separate client registrations hit the OAuth server expect(server.registeredClients.size).toBe(2); - // Each got a different client_id — this is the root cause of the mismatch const clientA = resultA.flowMetadata.clientInfo?.client_id; const clientB = resultB.flowMetadata.clientInfo?.client_id; expect(clientA).not.toBe(clientB); @@ -92,36 +88,27 @@ describe('MCP OAuth Client Registration Reuse (PR #11925)', () => { it('should re-register on every sequential initiateOAuthFlow call (reconnections)', async () => { server = await createOAuthMCPServer({ tokenTTLMs: 60000 }); - process.env.DOMAIN_SERVER = 'http://localhost:3080'; - // First flow await MCPOAuthHandler.initiateOAuthFlow('test-server', server.url, 'user-1', {}); expect(server.registeredClients.size).toBe(1); - // Second flow (page reload / reconnection) — registers again await MCPOAuthHandler.initiateOAuthFlow('test-server', server.url, 'user-1', {}); expect(server.registeredClients.size).toBe(2); - // Third flow — yet another registration await MCPOAuthHandler.initiateOAuthFlow('test-server', server.url, 'user-1', {}); expect(server.registeredClients.size).toBe(3); }); }); - describe('Fix: reuse stored client registration via findToken', () => { + describe('Client reuse via findToken on reconnection', () => { it('should reuse an existing client registration when findToken returns stored client info', async () => { server = await createOAuthMCPServer({ tokenTTLMs: 60000 }); - process.env.DOMAIN_SERVER = 'http://localhost:3080'; - - const serverName = 'test-server'; - const userId = 'user-1'; const tokenStore = new InMemoryTokenStore(); - // First flow: no stored client → registers a new one const firstResult = await MCPOAuthHandler.initiateOAuthFlow( - serverName, + 'test-server', server.url, - userId, + 'user-1', {}, undefined, undefined, @@ -129,13 +116,10 @@ describe('MCP OAuth Client Registration Reuse (PR #11925)', () => { ); expect(server.registeredClients.size).toBe(1); const firstClientId = firstResult.flowMetadata.clientInfo?.client_id; - expect(firstClientId).toBeTruthy(); - // Simulate what happens after a successful OAuth flow: - // storeTokens() saves the client info to MongoDB (here: InMemoryTokenStore) await MCPTokenStorage.storeTokens({ - userId, - serverName, + userId: 'user-1', + serverName: 'test-server', tokens: { access_token: 'test-token', token_type: 'Bearer' }, createToken: tokenStore.createToken, updateToken: tokenStore.updateToken, @@ -144,70 +128,140 @@ describe('MCP OAuth Client Registration Reuse (PR #11925)', () => { metadata: firstResult.flowMetadata.metadata, }); - // Second flow (reconnection): findToken should find the stored client → reuse it const secondResult = await MCPOAuthHandler.initiateOAuthFlow( - serverName, + 'test-server', server.url, - userId, + 'user-1', {}, undefined, undefined, tokenStore.findToken, ); - // Should still be only 1 registration on the OAuth server (reused the first) expect(server.registeredClients.size).toBe(1); + expect(secondResult.flowMetadata.clientInfo?.client_id).toBe(firstClientId); + }); - // Same client_id used in both flows - const secondClientId = secondResult.flowMetadata.clientInfo?.client_id; - expect(secondClientId).toBe(firstClientId); + it('should reuse the same client when two reconnections fire concurrently with pre-seeded token', async () => { + server = await createOAuthMCPServer({ tokenTTLMs: 60000 }); + const tokenStore = new InMemoryTokenStore(); + + const initialResult = await MCPOAuthHandler.initiateOAuthFlow( + 'test-server', + server.url, + 'user-1', + {}, + undefined, + undefined, + tokenStore.findToken, + ); + const storedClientId = initialResult.flowMetadata.clientInfo?.client_id; + + await MCPTokenStorage.storeTokens({ + userId: 'user-1', + serverName: 'test-server', + tokens: { access_token: 'test-token', token_type: 'Bearer' }, + createToken: tokenStore.createToken, + updateToken: tokenStore.updateToken, + findToken: tokenStore.findToken, + clientInfo: initialResult.flowMetadata.clientInfo, + metadata: initialResult.flowMetadata.metadata, + }); + + const [resultA, resultB] = await Promise.all([ + MCPOAuthHandler.initiateOAuthFlow( + 'test-server', + server.url, + 'user-1', + {}, + undefined, + undefined, + tokenStore.findToken, + ), + MCPOAuthHandler.initiateOAuthFlow( + 'test-server', + server.url, + 'user-1', + {}, + undefined, + undefined, + tokenStore.findToken, + ), + ]); + + // Both should reuse the stored client — only the initial registration should exist + expect(server.registeredClients.size).toBe(1); + expect(resultA.flowMetadata.clientInfo?.client_id).toBe(storedClientId); + expect(resultB.flowMetadata.clientInfo?.client_id).toBe(storedClientId); }); it('should re-register when stored redirect_uri differs from current', async () => { server = await createOAuthMCPServer({ tokenTTLMs: 60000 }); - process.env.DOMAIN_SERVER = 'http://localhost:3080'; - - const serverName = 'test-server'; - const userId = 'user-1'; const tokenStore = new InMemoryTokenStore(); - // Seed a stored client with a different redirect_uri (simulating domain change) - const oldClientInfo = { - client_id: 'old-client-id', - client_secret: 'old-secret', - redirect_uris: ['http://old-domain.com/api/mcp/test-server/oauth/callback'], - }; - await MCPTokenStorage.storeTokens({ - userId, - serverName, + userId: 'user-1', + serverName: 'test-server', tokens: { access_token: 'old-token', token_type: 'Bearer' }, createToken: tokenStore.createToken, updateToken: tokenStore.updateToken, findToken: tokenStore.findToken, - clientInfo: oldClientInfo, + clientInfo: { + client_id: 'old-client-id', + client_secret: 'old-secret', + redirect_uris: ['http://old-domain.com/api/mcp/test-server/oauth/callback'], + }, }); - // New flow with different DOMAIN_SERVER → redirect_uri changed const result = await MCPOAuthHandler.initiateOAuthFlow( - serverName, + 'test-server', server.url, - userId, + 'user-1', {}, undefined, undefined, tokenStore.findToken, ); - // Should have registered a NEW client because redirect_uri changed expect(server.registeredClients.size).toBe(1); expect(result.flowMetadata.clientInfo?.client_id).not.toBe('old-client-id'); }); + it('should re-register when stored client has empty redirect_uris', async () => { + server = await createOAuthMCPServer({ tokenTTLMs: 60000 }); + const tokenStore = new InMemoryTokenStore(); + + 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: 'empty-redirect-client', + client_secret: 'secret', + redirect_uris: [], + }, + }); + + const result = await MCPOAuthHandler.initiateOAuthFlow( + 'test-server', + server.url, + 'user-1', + {}, + undefined, + undefined, + tokenStore.findToken, + ); + + // Should NOT reuse the client with empty redirect_uris — must re-register + expect(server.registeredClients.size).toBe(1); + expect(result.flowMetadata.clientInfo?.client_id).not.toBe('empty-redirect-client'); + }); + it('should fall back to registration when findToken lookup throws', async () => { server = await createOAuthMCPServer({ tokenTTLMs: 60000 }); - process.env.DOMAIN_SERVER = 'http://localhost:3080'; - const failingFindToken = jest.fn().mockRejectedValue(new Error('DB connection lost')); const result = await MCPOAuthHandler.initiateOAuthFlow( @@ -220,22 +274,17 @@ describe('MCP OAuth Client Registration Reuse (PR #11925)', () => { failingFindToken, ); - // Should have fallen back to registering a new client expect(server.registeredClients.size).toBe(1); expect(result.flowMetadata.clientInfo?.client_id).toBeTruthy(); }); it('should not call getClientInfoAndMetadata when findToken is not provided', async () => { server = await createOAuthMCPServer({ tokenTTLMs: 60000 }); - process.env.DOMAIN_SERVER = 'http://localhost:3080'; - const spy = jest.spyOn(MCPTokenStorage, 'getClientInfoAndMetadata'); await MCPOAuthHandler.initiateOAuthFlow('test-server', server.url, 'user-1', {}); - // Without findToken, should not attempt client reuse lookup expect(spy).not.toHaveBeenCalled(); - spy.mockRestore(); }); }); diff --git a/packages/api/src/mcp/oauth/handler.ts b/packages/api/src/mcp/oauth/handler.ts index 3b3c15d2ff..bc351d9660 100644 --- a/packages/api/src/mcp/oauth/handler.ts +++ b/packages/api/src/mcp/oauth/handler.ts @@ -24,8 +24,8 @@ import { selectRegistrationAuthMethod, inferClientAuthMethod, } from './methods'; -import { isSSRFTarget, resolveHostnameSSRF, isOAuthUrlAllowed } from '~/auth'; import type { TokenMethods } from '@librechat/data-schemas'; +import { isSSRFTarget, resolveHostnameSSRF, isOAuthUrlAllowed } from '~/auth'; import { MCPTokenStorage } from './tokens'; import { sanitizeUrlForLogging } from '~/mcp/utils'; @@ -496,8 +496,7 @@ export class MCPOAuthHandler { `[MCPOAuth] OAuth metadata discovered, auth server URL: ${sanitizeUrlForLogging(authServerUrl)}`, ); - /** Dynamic client registration based on the discovered metadata */ - const redirectUri = config?.redirect_uri || this.getDefaultRedirectUri(serverName); + const redirectUri = this.getDefaultRedirectUri(serverName); logger.debug(`[MCPOAuth] Resolving OAuth client with redirect URI: ${redirectUri}`); let clientInfo: OAuthClientInformation | undefined; @@ -510,23 +509,22 @@ export class MCPOAuthHandler { findToken, }); if (existing?.clientInfo?.client_id) { - const existingClient = existing.clientInfo as OAuthClientInformation; - const storedRedirectUri = existingClient.redirect_uris?.[0]; - if (storedRedirectUri && storedRedirectUri !== redirectUri) { + const storedRedirectUri = existing.clientInfo.redirect_uris?.[0]; + if (!storedRedirectUri || storedRedirectUri !== redirectUri) { logger.debug( `[MCPOAuth] Stored redirect_uri "${storedRedirectUri}" differs from current "${redirectUri}", will re-register`, ); } else { logger.debug( - `[MCPOAuth] Reusing existing client registration: ${existingClient.client_id}`, + `[MCPOAuth] Reusing existing client registration: ${existing.clientInfo.client_id}`, ); - clientInfo = existingClient; + clientInfo = existing.clientInfo; } } } catch (error) { - logger.debug( - `[MCPOAuth] Failed to look up existing client registration, will register new`, - { error }, + logger.warn( + `[MCPOAuth] Failed to look up existing client registration, falling back to new registration`, + { error, serverName, userId }, ); } }