mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-04-07 08:25:23 +02:00
fix: reuse existing OAuth client registrations to prevent client_id mismatch
When using auto-discovered OAuth (DCR), LibreChat calls /register on every flow initiation, getting a new client_id each time. When concurrent connections or reconnections happen, the client_id used during /authorize differs from the one used during /token, causing the server to reject the exchange. Before registering a new client, check if a valid client registration already exists in the database and reuse it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
33ee7dea1e
commit
2fcf8c5419
4 changed files with 261 additions and 9 deletions
|
|
@ -351,6 +351,7 @@ export class MCPConnectionFactory {
|
||||||
config?.oauth_headers ?? {},
|
config?.oauth_headers ?? {},
|
||||||
config?.oauth,
|
config?.oauth,
|
||||||
this.allowedDomains,
|
this.allowedDomains,
|
||||||
|
this.tokenMethods?.findToken,
|
||||||
);
|
);
|
||||||
|
|
||||||
if (existingFlow) {
|
if (existingFlow) {
|
||||||
|
|
@ -615,6 +616,7 @@ export class MCPConnectionFactory {
|
||||||
this.serverConfig.oauth_headers ?? {},
|
this.serverConfig.oauth_headers ?? {},
|
||||||
this.serverConfig.oauth,
|
this.serverConfig.oauth,
|
||||||
this.allowedDomains,
|
this.allowedDomains,
|
||||||
|
this.tokenMethods?.findToken,
|
||||||
);
|
);
|
||||||
|
|
||||||
// Store flow state BEFORE redirecting so the callback can find it
|
// Store flow state BEFORE redirecting so the callback can find it
|
||||||
|
|
|
||||||
|
|
@ -270,6 +270,7 @@ describe('MCPConnectionFactory', () => {
|
||||||
{},
|
{},
|
||||||
undefined,
|
undefined,
|
||||||
undefined,
|
undefined,
|
||||||
|
oauthOptions.tokenMethods.findToken,
|
||||||
);
|
);
|
||||||
|
|
||||||
// initFlow must be awaited BEFORE the redirect to guarantee state is stored
|
// initFlow must be awaited BEFORE the redirect to guarantee state is stored
|
||||||
|
|
|
||||||
|
|
@ -20,6 +20,12 @@ jest.mock('@modelcontextprotocol/sdk/client/auth.js', () => ({
|
||||||
exchangeAuthorization: jest.fn(),
|
exchangeAuthorization: jest.fn(),
|
||||||
}));
|
}));
|
||||||
|
|
||||||
|
jest.mock('../../mcp/oauth/tokens', () => ({
|
||||||
|
MCPTokenStorage: {
|
||||||
|
getClientInfoAndMetadata: jest.fn(),
|
||||||
|
},
|
||||||
|
}));
|
||||||
|
|
||||||
import {
|
import {
|
||||||
startAuthorization,
|
startAuthorization,
|
||||||
discoverAuthorizationServerMetadata,
|
discoverAuthorizationServerMetadata,
|
||||||
|
|
@ -27,6 +33,7 @@ import {
|
||||||
registerClient,
|
registerClient,
|
||||||
exchangeAuthorization,
|
exchangeAuthorization,
|
||||||
} from '@modelcontextprotocol/sdk/client/auth.js';
|
} from '@modelcontextprotocol/sdk/client/auth.js';
|
||||||
|
import { MCPTokenStorage } from '../../mcp/oauth/tokens';
|
||||||
import { FlowStateManager } from '../../flow/manager';
|
import { FlowStateManager } from '../../flow/manager';
|
||||||
|
|
||||||
const mockStartAuthorization = startAuthorization as jest.MockedFunction<typeof startAuthorization>;
|
const mockStartAuthorization = startAuthorization as jest.MockedFunction<typeof startAuthorization>;
|
||||||
|
|
@ -42,6 +49,10 @@ const mockRegisterClient = registerClient as jest.MockedFunction<typeof register
|
||||||
const mockExchangeAuthorization = exchangeAuthorization as jest.MockedFunction<
|
const mockExchangeAuthorization = exchangeAuthorization as jest.MockedFunction<
|
||||||
typeof exchangeAuthorization
|
typeof exchangeAuthorization
|
||||||
>;
|
>;
|
||||||
|
const mockGetClientInfoAndMetadata =
|
||||||
|
MCPTokenStorage.getClientInfoAndMetadata as jest.MockedFunction<
|
||||||
|
typeof MCPTokenStorage.getClientInfoAndMetadata
|
||||||
|
>;
|
||||||
|
|
||||||
describe('MCPOAuthHandler - Configurable OAuth Metadata', () => {
|
describe('MCPOAuthHandler - Configurable OAuth Metadata', () => {
|
||||||
const mockServerName = 'test-server';
|
const mockServerName = 'test-server';
|
||||||
|
|
@ -1391,6 +1402,216 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => {
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('Client Registration Reuse', () => {
|
||||||
|
const originalFetch = global.fetch;
|
||||||
|
const mockFetch = jest.fn();
|
||||||
|
|
||||||
|
beforeEach(() => {
|
||||||
|
jest.clearAllMocks();
|
||||||
|
global.fetch = mockFetch as unknown as typeof fetch;
|
||||||
|
mockFetch.mockResolvedValue({ ok: true, json: async () => ({}) } as Response);
|
||||||
|
process.env.DOMAIN_SERVER = 'http://localhost:3080';
|
||||||
|
});
|
||||||
|
|
||||||
|
afterAll(() => {
|
||||||
|
global.fetch = originalFetch;
|
||||||
|
});
|
||||||
|
|
||||||
|
const mockFindToken = jest.fn();
|
||||||
|
|
||||||
|
it('should reuse existing client registration when findToken is provided and client exists', async () => {
|
||||||
|
const existingClientInfo = {
|
||||||
|
client_id: 'existing-client-id',
|
||||||
|
client_secret: 'existing-client-secret',
|
||||||
|
redirect_uris: ['http://localhost:3080/api/mcp/test-server/oauth/callback'],
|
||||||
|
token_endpoint_auth_method: 'client_secret_basic',
|
||||||
|
};
|
||||||
|
|
||||||
|
mockGetClientInfoAndMetadata.mockResolvedValueOnce({
|
||||||
|
clientInfo: existingClientInfo,
|
||||||
|
clientMetadata: {},
|
||||||
|
});
|
||||||
|
|
||||||
|
// Mock resource metadata discovery to fail
|
||||||
|
mockDiscoverOAuthProtectedResourceMetadata.mockRejectedValueOnce(
|
||||||
|
new Error('No resource metadata'),
|
||||||
|
);
|
||||||
|
|
||||||
|
// Mock authorization server metadata discovery
|
||||||
|
mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce({
|
||||||
|
issuer: 'https://example.com',
|
||||||
|
authorization_endpoint: 'https://example.com/authorize',
|
||||||
|
token_endpoint: 'https://example.com/token',
|
||||||
|
registration_endpoint: 'https://example.com/register',
|
||||||
|
response_types_supported: ['code'],
|
||||||
|
jwks_uri: 'https://example.com/.well-known/jwks.json',
|
||||||
|
subject_types_supported: ['public'],
|
||||||
|
id_token_signing_alg_values_supported: ['RS256'],
|
||||||
|
} as AuthorizationServerMetadata);
|
||||||
|
|
||||||
|
mockStartAuthorization.mockResolvedValueOnce({
|
||||||
|
authorizationUrl: new URL('https://example.com/authorize?client_id=existing-client-id'),
|
||||||
|
codeVerifier: 'test-code-verifier',
|
||||||
|
});
|
||||||
|
|
||||||
|
const result = await MCPOAuthHandler.initiateOAuthFlow(
|
||||||
|
'test-server',
|
||||||
|
'https://example.com/mcp',
|
||||||
|
'user-123',
|
||||||
|
{},
|
||||||
|
undefined,
|
||||||
|
mockFindToken,
|
||||||
|
);
|
||||||
|
|
||||||
|
// Should NOT have called registerClient since we reused the existing one
|
||||||
|
expect(mockRegisterClient).not.toHaveBeenCalled();
|
||||||
|
|
||||||
|
// Should have used the existing client info for startAuthorization
|
||||||
|
expect(mockStartAuthorization).toHaveBeenCalledWith(
|
||||||
|
'https://example.com/mcp',
|
||||||
|
expect.objectContaining({
|
||||||
|
clientInformation: existingClientInfo,
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(result.authorizationUrl).toBeDefined();
|
||||||
|
expect(result.flowId).toBeDefined();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should register a new client when findToken is provided but no existing registration found', async () => {
|
||||||
|
mockGetClientInfoAndMetadata.mockResolvedValueOnce(null);
|
||||||
|
|
||||||
|
// Mock resource metadata discovery to fail
|
||||||
|
mockDiscoverOAuthProtectedResourceMetadata.mockRejectedValueOnce(
|
||||||
|
new Error('No resource metadata'),
|
||||||
|
);
|
||||||
|
|
||||||
|
// Mock authorization server metadata discovery
|
||||||
|
mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce({
|
||||||
|
issuer: 'https://example.com',
|
||||||
|
authorization_endpoint: 'https://example.com/authorize',
|
||||||
|
token_endpoint: 'https://example.com/token',
|
||||||
|
registration_endpoint: 'https://example.com/register',
|
||||||
|
response_types_supported: ['code'],
|
||||||
|
jwks_uri: 'https://example.com/.well-known/jwks.json',
|
||||||
|
subject_types_supported: ['public'],
|
||||||
|
id_token_signing_alg_values_supported: ['RS256'],
|
||||||
|
} as AuthorizationServerMetadata);
|
||||||
|
|
||||||
|
mockRegisterClient.mockResolvedValueOnce({
|
||||||
|
client_id: 'new-client-id',
|
||||||
|
client_secret: 'new-client-secret',
|
||||||
|
redirect_uris: ['http://localhost:3080/api/mcp/test-server/oauth/callback'],
|
||||||
|
});
|
||||||
|
|
||||||
|
mockStartAuthorization.mockResolvedValueOnce({
|
||||||
|
authorizationUrl: new URL('https://example.com/authorize?client_id=new-client-id'),
|
||||||
|
codeVerifier: 'test-code-verifier',
|
||||||
|
});
|
||||||
|
|
||||||
|
await MCPOAuthHandler.initiateOAuthFlow(
|
||||||
|
'test-server',
|
||||||
|
'https://example.com/mcp',
|
||||||
|
'user-123',
|
||||||
|
{},
|
||||||
|
undefined,
|
||||||
|
mockFindToken,
|
||||||
|
);
|
||||||
|
|
||||||
|
// Should have called registerClient since no existing registration was found
|
||||||
|
expect(mockRegisterClient).toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should register a new client when findToken is not provided', async () => {
|
||||||
|
// Mock resource metadata discovery to fail
|
||||||
|
mockDiscoverOAuthProtectedResourceMetadata.mockRejectedValueOnce(
|
||||||
|
new Error('No resource metadata'),
|
||||||
|
);
|
||||||
|
|
||||||
|
// Mock authorization server metadata discovery
|
||||||
|
mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce({
|
||||||
|
issuer: 'https://example.com',
|
||||||
|
authorization_endpoint: 'https://example.com/authorize',
|
||||||
|
token_endpoint: 'https://example.com/token',
|
||||||
|
registration_endpoint: 'https://example.com/register',
|
||||||
|
response_types_supported: ['code'],
|
||||||
|
jwks_uri: 'https://example.com/.well-known/jwks.json',
|
||||||
|
subject_types_supported: ['public'],
|
||||||
|
id_token_signing_alg_values_supported: ['RS256'],
|
||||||
|
} as AuthorizationServerMetadata);
|
||||||
|
|
||||||
|
mockRegisterClient.mockResolvedValueOnce({
|
||||||
|
client_id: 'new-client-id',
|
||||||
|
client_secret: 'new-client-secret',
|
||||||
|
redirect_uris: ['http://localhost:3080/api/mcp/test-server/oauth/callback'],
|
||||||
|
});
|
||||||
|
|
||||||
|
mockStartAuthorization.mockResolvedValueOnce({
|
||||||
|
authorizationUrl: new URL('https://example.com/authorize?client_id=new-client-id'),
|
||||||
|
codeVerifier: 'test-code-verifier',
|
||||||
|
});
|
||||||
|
|
||||||
|
// No findToken passed
|
||||||
|
await MCPOAuthHandler.initiateOAuthFlow(
|
||||||
|
'test-server',
|
||||||
|
'https://example.com/mcp',
|
||||||
|
'user-123',
|
||||||
|
{},
|
||||||
|
undefined,
|
||||||
|
);
|
||||||
|
|
||||||
|
// Should NOT have tried to look up existing registration
|
||||||
|
expect(mockGetClientInfoAndMetadata).not.toHaveBeenCalled();
|
||||||
|
|
||||||
|
// Should have called registerClient
|
||||||
|
expect(mockRegisterClient).toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should fall back to registration when getClientInfoAndMetadata throws', async () => {
|
||||||
|
mockGetClientInfoAndMetadata.mockRejectedValueOnce(new Error('DB error'));
|
||||||
|
|
||||||
|
// Mock resource metadata discovery to fail
|
||||||
|
mockDiscoverOAuthProtectedResourceMetadata.mockRejectedValueOnce(
|
||||||
|
new Error('No resource metadata'),
|
||||||
|
);
|
||||||
|
|
||||||
|
// Mock authorization server metadata discovery
|
||||||
|
mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce({
|
||||||
|
issuer: 'https://example.com',
|
||||||
|
authorization_endpoint: 'https://example.com/authorize',
|
||||||
|
token_endpoint: 'https://example.com/token',
|
||||||
|
registration_endpoint: 'https://example.com/register',
|
||||||
|
response_types_supported: ['code'],
|
||||||
|
jwks_uri: 'https://example.com/.well-known/jwks.json',
|
||||||
|
subject_types_supported: ['public'],
|
||||||
|
id_token_signing_alg_values_supported: ['RS256'],
|
||||||
|
} as AuthorizationServerMetadata);
|
||||||
|
|
||||||
|
mockRegisterClient.mockResolvedValueOnce({
|
||||||
|
client_id: 'new-client-id',
|
||||||
|
client_secret: 'new-client-secret',
|
||||||
|
redirect_uris: ['http://localhost:3080/api/mcp/test-server/oauth/callback'],
|
||||||
|
});
|
||||||
|
|
||||||
|
mockStartAuthorization.mockResolvedValueOnce({
|
||||||
|
authorizationUrl: new URL('https://example.com/authorize?client_id=new-client-id'),
|
||||||
|
codeVerifier: 'test-code-verifier',
|
||||||
|
});
|
||||||
|
|
||||||
|
await MCPOAuthHandler.initiateOAuthFlow(
|
||||||
|
'test-server',
|
||||||
|
'https://example.com/mcp',
|
||||||
|
'user-123',
|
||||||
|
{},
|
||||||
|
undefined,
|
||||||
|
mockFindToken,
|
||||||
|
);
|
||||||
|
|
||||||
|
// Should have fallen back to registerClient
|
||||||
|
expect(mockRegisterClient).toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
describe('Fallback OAuth Metadata (Legacy Server Support)', () => {
|
describe('Fallback OAuth Metadata (Legacy Server Support)', () => {
|
||||||
const originalFetch = global.fetch;
|
const originalFetch = global.fetch;
|
||||||
const mockFetch = jest.fn();
|
const mockFetch = jest.fn();
|
||||||
|
|
|
||||||
|
|
@ -25,6 +25,8 @@ import {
|
||||||
inferClientAuthMethod,
|
inferClientAuthMethod,
|
||||||
} from './methods';
|
} from './methods';
|
||||||
import { isSSRFTarget, resolveHostnameSSRF, isOAuthUrlAllowed } from '~/auth';
|
import { isSSRFTarget, resolveHostnameSSRF, isOAuthUrlAllowed } from '~/auth';
|
||||||
|
import type { TokenMethods } from '@librechat/data-schemas';
|
||||||
|
import { MCPTokenStorage } from './tokens';
|
||||||
import { sanitizeUrlForLogging } from '~/mcp/utils';
|
import { sanitizeUrlForLogging } from '~/mcp/utils';
|
||||||
|
|
||||||
/** Type for the OAuth metadata from the SDK */
|
/** Type for the OAuth metadata from the SDK */
|
||||||
|
|
@ -368,6 +370,7 @@ export class MCPOAuthHandler {
|
||||||
oauthHeaders: Record<string, string>,
|
oauthHeaders: Record<string, string>,
|
||||||
config?: MCPOptions['oauth'],
|
config?: MCPOptions['oauth'],
|
||||||
allowedDomains?: string[] | null,
|
allowedDomains?: string[] | null,
|
||||||
|
findToken?: TokenMethods['findToken'],
|
||||||
): Promise<{ authorizationUrl: string; flowId: string; flowMetadata: MCPOAuthFlowMetadata }> {
|
): Promise<{ authorizationUrl: string; flowId: string; flowMetadata: MCPOAuthFlowMetadata }> {
|
||||||
logger.debug(
|
logger.debug(
|
||||||
`[MCPOAuth] initiateOAuthFlow called for ${serverName} with URL: ${sanitizeUrlForLogging(serverUrl)}`,
|
`[MCPOAuth] initiateOAuthFlow called for ${serverName} with URL: ${sanitizeUrlForLogging(serverUrl)}`,
|
||||||
|
|
@ -496,7 +499,32 @@ export class MCPOAuthHandler {
|
||||||
const redirectUri = this.getDefaultRedirectUri(serverName);
|
const redirectUri = this.getDefaultRedirectUri(serverName);
|
||||||
logger.debug(`[MCPOAuth] Registering OAuth client with redirect URI: ${redirectUri}`);
|
logger.debug(`[MCPOAuth] Registering OAuth client with redirect URI: ${redirectUri}`);
|
||||||
|
|
||||||
const clientInfo = await this.registerOAuthClient(
|
// Before registering, check if we already have a valid client registration
|
||||||
|
let clientInfo: OAuthClientInformation | undefined;
|
||||||
|
|
||||||
|
if (findToken) {
|
||||||
|
try {
|
||||||
|
const existing = await MCPTokenStorage.getClientInfoAndMetadata({
|
||||||
|
userId,
|
||||||
|
serverName,
|
||||||
|
findToken,
|
||||||
|
});
|
||||||
|
if (existing?.clientInfo?.client_id) {
|
||||||
|
logger.debug(
|
||||||
|
`[MCPOAuth] Reusing existing client registration: ${existing.clientInfo.client_id}`,
|
||||||
|
);
|
||||||
|
clientInfo = existing.clientInfo;
|
||||||
|
}
|
||||||
|
} catch (error) {
|
||||||
|
logger.debug(
|
||||||
|
`[MCPOAuth] Failed to look up existing client registration, will register new`,
|
||||||
|
{ error },
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
if (!clientInfo) {
|
||||||
|
clientInfo = await this.registerOAuthClient(
|
||||||
authServerUrl.toString(),
|
authServerUrl.toString(),
|
||||||
metadata,
|
metadata,
|
||||||
oauthHeaders,
|
oauthHeaders,
|
||||||
|
|
@ -504,8 +532,8 @@ export class MCPOAuthHandler {
|
||||||
redirectUri,
|
redirectUri,
|
||||||
config?.token_exchange_method,
|
config?.token_exchange_method,
|
||||||
);
|
);
|
||||||
|
|
||||||
logger.debug(`[MCPOAuth] Client registered with ID: ${clientInfo.client_id}`);
|
logger.debug(`[MCPOAuth] Client registered with ID: ${clientInfo.client_id}`);
|
||||||
|
}
|
||||||
|
|
||||||
/** Authorization Scope */
|
/** Authorization Scope */
|
||||||
const scope =
|
const scope =
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue