mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-04-07 00:15:23 +02:00
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
This commit is contained in:
Danny Avila
parent
e22c4675e8
commit
ca60c83aa3
2 changed files with 129 additions and 82 deletions
|
|
@ -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();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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 },
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue