From 344e7c44b5ccaae876bfc2ac741198a3e05f9857 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 19 Sep 2025 06:50:02 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=90=20fix:=20Respect=20Server's=20Toke?= =?UTF-8?q?n=20Endpoint=20Auth=20Methods=20for=20MCP=20OAuth=20Refresh=20(?= =?UTF-8?q?#9717)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: respect server's token endpoint auth methods for MCP OAuth refresh Previously, LibreChat always used Basic Auth when refreshing OAuth tokens if a client_secret was present. This caused issues with servers (like FastMCP) that only support client_secret_post. Now properly checks and respects the server's advertised token_endpoint_auth_methods_supported. Fixes token refresh failures with error: "refresh_token.client_id: Field required" * chore: remove MCP OAuth URL Logging --- api/server/services/Tools/mcp.js | 2 +- .../api/src/mcp/__tests__/handler.test.ts | 437 +++++++++++++++++- packages/api/src/mcp/oauth/handler.ts | 77 ++- 3 files changed, 503 insertions(+), 13 deletions(-) diff --git a/api/server/services/Tools/mcp.js b/api/server/services/Tools/mcp.js index f6efbb78a..069447e3b 100644 --- a/api/server/services/Tools/mcp.js +++ b/api/server/services/Tools/mcp.js @@ -44,7 +44,7 @@ async function reinitMCPServer({ const oauthStart = _oauthStart ?? (async (authURL) => { - logger.info(`[MCP Reinitialize] OAuth URL received: ${authURL}`); + logger.info(`[MCP Reinitialize] OAuth URL received for ${serverName}`); oauthUrl = authURL; oauthRequired = true; }); diff --git a/packages/api/src/mcp/__tests__/handler.test.ts b/packages/api/src/mcp/__tests__/handler.test.ts index ce49d7eaf..01794fe4d 100644 --- a/packages/api/src/mcp/__tests__/handler.test.ts +++ b/packages/api/src/mcp/__tests__/handler.test.ts @@ -1,4 +1,5 @@ import type { MCPOptions } from 'librechat-data-provider'; +import type { AuthorizationServerMetadata } from '@modelcontextprotocol/sdk/shared/auth.js'; import { MCPOAuthHandler } from '~/mcp/oauth'; jest.mock('@librechat/data-schemas', () => ({ @@ -12,11 +13,19 @@ jest.mock('@librechat/data-schemas', () => ({ jest.mock('@modelcontextprotocol/sdk/client/auth.js', () => ({ startAuthorization: jest.fn(), + discoverAuthorizationServerMetadata: jest.fn(), })); -import { startAuthorization } from '@modelcontextprotocol/sdk/client/auth.js'; +import { + startAuthorization, + discoverAuthorizationServerMetadata, +} from '@modelcontextprotocol/sdk/client/auth.js'; const mockStartAuthorization = startAuthorization as jest.MockedFunction; +const mockDiscoverAuthorizationServerMetadata = + discoverAuthorizationServerMetadata as jest.MockedFunction< + typeof discoverAuthorizationServerMetadata + >; describe('MCPOAuthHandler - Configurable OAuth Metadata', () => { const mockServerName = 'test-server'; @@ -188,6 +197,432 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => { }); }); + describe('refreshOAuthTokens', () => { + const mockRefreshToken = 'refresh-token-12345'; + const originalFetch = global.fetch; + const mockFetch = jest.fn() as unknown as jest.MockedFunction; + + beforeEach(() => { + jest.clearAllMocks(); + global.fetch = mockFetch; + }); + + afterEach(() => { + mockFetch.mockClear(); + }); + + afterAll(() => { + global.fetch = originalFetch; + }); + + describe('with stored metadata', () => { + it('should use client_secret_post when server only supports that method', async () => { + const metadata = { + serverName: 'test-server', + userId: 'user-123', + serverUrl: 'https://auth.example.com', + state: 'state-123', + clientInfo: { + client_id: 'test-client-id', + client_secret: 'test-client-secret', + grant_types: ['authorization_code', 'refresh_token'], + scope: 'read write', + }, + }; + + // Mock OAuth metadata discovery + mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce({ + issuer: 'https://auth.example.com', + authorization_endpoint: 'https://auth.example.com/oauth/authorize', + token_endpoint: 'https://auth.example.com/oauth/token', + token_endpoint_auth_methods_supported: ['client_secret_post'], + response_types_supported: ['code'], + jwks_uri: 'https://auth.example.com/.well-known/jwks.json', + subject_types_supported: ['public'], + id_token_signing_alg_values_supported: ['RS256'], + } as AuthorizationServerMetadata); + + mockFetch.mockResolvedValueOnce({ + ok: true, + json: async () => ({ + access_token: 'new-access-token', + refresh_token: 'new-refresh-token', + expires_in: 3600, + }), + } as Response); + + const result = await MCPOAuthHandler.refreshOAuthTokens(mockRefreshToken, metadata); + + // Verify the call was made without Authorization header + expect(mockFetch).toHaveBeenCalledWith( + 'https://auth.example.com/oauth/token', + expect.objectContaining({ + method: 'POST', + headers: expect.not.objectContaining({ + Authorization: expect.any(String), + }), + }), + ); + + // Verify the body contains client_id and client_secret + const callArgs = mockFetch.mock.calls[0]; + const body = callArgs[1]?.body as URLSearchParams; + expect(body.toString()).toContain('client_id=test-client-id'); + expect(body.toString()).toContain('client_secret=test-client-secret'); + + expect(result).toEqual({ + access_token: 'new-access-token', + refresh_token: 'new-refresh-token', + expires_in: 3600, + obtained_at: expect.any(Number), + expires_at: expect.any(Number), + }); + }); + + it('should use client_secret_basic when server only supports that method', async () => { + const metadata = { + serverName: 'test-server', + userId: 'user-123', + serverUrl: 'https://auth.example.com', + state: 'state-123', + clientInfo: { + client_id: 'test-client-id', + client_secret: 'test-client-secret', + grant_types: ['authorization_code', 'refresh_token'], + scope: 'read write', + }, + }; + + // Mock OAuth metadata discovery + mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce({ + issuer: 'https://auth.example.com', + authorization_endpoint: 'https://auth.example.com/oauth/authorize', + token_endpoint: 'https://auth.example.com/oauth/token', + token_endpoint_auth_methods_supported: ['client_secret_basic'], + response_types_supported: ['code'], + jwks_uri: 'https://auth.example.com/.well-known/jwks.json', + subject_types_supported: ['public'], + id_token_signing_alg_values_supported: ['RS256'], + } as AuthorizationServerMetadata); + + mockFetch.mockResolvedValueOnce({ + ok: true, + json: async () => ({ + access_token: 'new-access-token', + refresh_token: 'new-refresh-token', + expires_in: 3600, + }), + } as Response); + + await MCPOAuthHandler.refreshOAuthTokens(mockRefreshToken, metadata); + + const expectedAuth = `Basic ${Buffer.from('test-client-id:test-client-secret').toString('base64')}`; + expect(mockFetch).toHaveBeenCalledWith( + 'https://auth.example.com/oauth/token', + expect.objectContaining({ + method: 'POST', + headers: expect.objectContaining({ + Authorization: expectedAuth, + }), + body: expect.not.stringContaining('client_id='), + }), + ); + }); + + it('should prefer client_secret_basic when both methods are supported', async () => { + const metadata = { + serverName: 'test-server', + userId: 'user-123', + serverUrl: 'https://auth.example.com', + state: 'state-123', + clientInfo: { + client_id: 'test-client-id', + client_secret: 'test-client-secret', + grant_types: ['authorization_code', 'refresh_token'], + }, + }; + + // Mock OAuth metadata discovery + mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce({ + issuer: 'https://auth.example.com', + authorization_endpoint: 'https://auth.example.com/oauth/authorize', + token_endpoint: 'https://auth.example.com/oauth/token', + token_endpoint_auth_methods_supported: ['client_secret_post', 'client_secret_basic'], + response_types_supported: ['code'], + jwks_uri: 'https://auth.example.com/.well-known/jwks.json', + subject_types_supported: ['public'], + id_token_signing_alg_values_supported: ['RS256'], + } as AuthorizationServerMetadata); + + mockFetch.mockResolvedValueOnce({ + ok: true, + json: async () => ({ + access_token: 'new-access-token', + refresh_token: 'new-refresh-token', + expires_in: 3600, + }), + } as Response); + + await MCPOAuthHandler.refreshOAuthTokens(mockRefreshToken, metadata); + + const expectedAuth = `Basic ${Buffer.from('test-client-id:test-client-secret').toString('base64')}`; + expect(mockFetch).toHaveBeenCalledWith( + 'https://auth.example.com/oauth/token', + expect.objectContaining({ + headers: expect.objectContaining({ + Authorization: expectedAuth, + }), + }), + ); + }); + + it('should default to client_secret_basic when no methods are advertised', async () => { + const metadata = { + serverName: 'test-server', + userId: 'user-123', + serverUrl: 'https://auth.example.com', + state: 'state-123', + clientInfo: { + client_id: 'test-client-id', + client_secret: 'test-client-secret', + grant_types: ['authorization_code', 'refresh_token'], + }, + }; + + // Mock OAuth metadata discovery with no auth methods specified + mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce({ + issuer: 'https://auth.example.com', + authorization_endpoint: 'https://auth.example.com/oauth/authorize', + token_endpoint: 'https://auth.example.com/oauth/token', + // No token_endpoint_auth_methods_supported field + response_types_supported: ['code'], + jwks_uri: 'https://auth.example.com/.well-known/jwks.json', + subject_types_supported: ['public'], + id_token_signing_alg_values_supported: ['RS256'], + } as AuthorizationServerMetadata); + + mockFetch.mockResolvedValueOnce({ + ok: true, + json: async () => ({ + access_token: 'new-access-token', + refresh_token: 'new-refresh-token', + expires_in: 3600, + }), + } as Response); + + await MCPOAuthHandler.refreshOAuthTokens(mockRefreshToken, metadata); + + const expectedAuth = `Basic ${Buffer.from('test-client-id:test-client-secret').toString('base64')}`; + expect(mockFetch).toHaveBeenCalledWith( + 'https://auth.example.com/oauth/token', + expect.objectContaining({ + headers: expect.objectContaining({ + Authorization: expectedAuth, + }), + }), + ); + }); + + it('should include client_id in body for public clients (no secret)', async () => { + const metadata = { + serverName: 'test-server', + userId: 'user-123', + serverUrl: 'https://auth.example.com', + state: 'state-123', + clientInfo: { + client_id: 'test-client-id', + // No client_secret - public client + grant_types: ['authorization_code', 'refresh_token'], + }, + }; + + // Mock OAuth metadata discovery + mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce({ + issuer: 'https://auth.example.com', + authorization_endpoint: 'https://auth.example.com/oauth/authorize', + token_endpoint: 'https://auth.example.com/oauth/token', + token_endpoint_auth_methods_supported: ['none'], + response_types_supported: ['code'], + jwks_uri: 'https://auth.example.com/.well-known/jwks.json', + subject_types_supported: ['public'], + id_token_signing_alg_values_supported: ['RS256'], + } as AuthorizationServerMetadata); + + mockFetch.mockResolvedValueOnce({ + ok: true, + json: async () => ({ + access_token: 'new-access-token', + refresh_token: 'new-refresh-token', + expires_in: 3600, + }), + } as Response); + + await MCPOAuthHandler.refreshOAuthTokens(mockRefreshToken, metadata); + + // Verify the call was made without Authorization header + expect(mockFetch).toHaveBeenCalledWith( + 'https://auth.example.com/oauth/token', + expect.objectContaining({ + method: 'POST', + headers: expect.not.objectContaining({ + Authorization: expect.any(String), + }), + }), + ); + + // Verify the body contains client_id (public client) + const callArgs = mockFetch.mock.calls[0]; + const body = callArgs[1]?.body as URLSearchParams; + expect(body.toString()).toContain('client_id=test-client-id'); + }); + }); + + describe('with pre-configured OAuth settings', () => { + it('should use client_secret_post when configured to only support that method', async () => { + const config = { + token_url: 'https://auth.example.com/oauth/token', + client_id: 'test-client-id', + client_secret: 'test-client-secret', + token_endpoint_auth_methods_supported: ['client_secret_post'], + }; + + mockFetch.mockResolvedValueOnce({ + ok: true, + json: async () => ({ + access_token: 'new-access-token', + refresh_token: 'new-refresh-token', + expires_in: 3600, + }), + } as Response); + + await MCPOAuthHandler.refreshOAuthTokens( + mockRefreshToken, + { serverName: 'test-server' }, + config, + ); + + // Verify the call was made without Authorization header + expect(mockFetch).toHaveBeenCalledWith( + new URL('https://auth.example.com/oauth/token'), + expect.objectContaining({ + method: 'POST', + headers: expect.not.objectContaining({ + Authorization: expect.any(String), + }), + }), + ); + + // Verify the body contains client_id and client_secret + const callArgs = mockFetch.mock.calls[0]; + const body = callArgs[1]?.body as URLSearchParams; + expect(body.toString()).toContain('client_id=test-client-id'); + expect(body.toString()).toContain('client_secret=test-client-secret'); + }); + + it('should use client_secret_basic when configured to support that method', async () => { + const config = { + token_url: 'https://auth.example.com/oauth/token', + client_id: 'test-client-id', + client_secret: 'test-client-secret', + token_endpoint_auth_methods_supported: ['client_secret_basic'], + }; + + mockFetch.mockResolvedValueOnce({ + ok: true, + json: async () => ({ + access_token: 'new-access-token', + refresh_token: 'new-refresh-token', + expires_in: 3600, + }), + } as Response); + + await MCPOAuthHandler.refreshOAuthTokens( + mockRefreshToken, + { serverName: 'test-server' }, + config, + ); + + const expectedAuth = `Basic ${Buffer.from('test-client-id:test-client-secret').toString('base64')}`; + expect(mockFetch).toHaveBeenCalledWith( + new URL('https://auth.example.com/oauth/token'), + expect.objectContaining({ + method: 'POST', + headers: expect.objectContaining({ + Authorization: expectedAuth, + }), + body: expect.not.stringContaining('client_id='), + }), + ); + }); + + it('should default to client_secret_basic when no auth methods configured', async () => { + const config = { + token_url: 'https://auth.example.com/oauth/token', + client_id: 'test-client-id', + client_secret: 'test-client-secret', + // No token_endpoint_auth_methods_supported field + }; + + mockFetch.mockResolvedValueOnce({ + ok: true, + json: async () => ({ + access_token: 'new-access-token', + refresh_token: 'new-refresh-token', + expires_in: 3600, + }), + } as Response); + + await MCPOAuthHandler.refreshOAuthTokens( + mockRefreshToken, + { serverName: 'test-server' }, + config, + ); + + const expectedAuth = `Basic ${Buffer.from('test-client-id:test-client-secret').toString('base64')}`; + expect(mockFetch).toHaveBeenCalledWith( + new URL('https://auth.example.com/oauth/token'), + expect.objectContaining({ + headers: expect.objectContaining({ + Authorization: expectedAuth, + }), + }), + ); + }); + }); + + it('should throw error when refresh fails', async () => { + const metadata = { + serverName: 'test-server', + userId: 'user-123', + serverUrl: 'https://auth.example.com', + state: 'state-123', + clientInfo: { + client_id: 'test-client-id', + client_secret: 'test-client-secret', + grant_types: ['authorization_code', 'refresh_token'], + }, + }; + + // Mock OAuth metadata discovery + mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce({ + token_endpoint: 'https://auth.example.com/oauth/token', + token_endpoint_auth_methods_supported: ['client_secret_post'], + } as AuthorizationServerMetadata); + + mockFetch.mockResolvedValueOnce({ + ok: false, + status: 400, + statusText: 'Bad Request', + text: async () => + '{"error":"invalid_request","error_description":"refresh_token.client_id: Field required"}', + } as Response); + + await expect(MCPOAuthHandler.refreshOAuthTokens(mockRefreshToken, metadata)).rejects.toThrow( + 'Token refresh failed: 400 Bad Request - {"error":"invalid_request","error_description":"refresh_token.client_id: Field required"}', + ); + }); + }); + describe('revokeOAuthToken', () => { const mockServerName = 'test-server'; const mockToken = 'test-token-12345'; diff --git a/packages/api/src/mcp/oauth/handler.ts b/packages/api/src/mcp/oauth/handler.ts index 3c1718cf8..a3fb144e6 100644 --- a/packages/api/src/mcp/oauth/handler.ts +++ b/packages/api/src/mcp/oauth/handler.ts @@ -501,6 +501,7 @@ export class MCPOAuthHandler { /** Use the stored client information and metadata to determine the token URL */ let tokenUrl: string; + let authMethods: string[] | undefined; if (config?.token_url) { tokenUrl = config.token_url; } else if (!metadata.serverUrl) { @@ -515,6 +516,7 @@ export class MCPOAuthHandler { throw new Error('No token endpoint found in OAuth metadata'); } tokenUrl = oauthMetadata.token_endpoint; + authMethods = oauthMetadata.token_endpoint_auth_methods_supported; } const body = new URLSearchParams({ @@ -532,14 +534,36 @@ export class MCPOAuthHandler { Accept: 'application/json', }; - /** Use client_secret for authentication if available */ + /** Handle authentication based on server's advertised methods */ if (metadata.clientInfo.client_secret) { - const clientAuth = Buffer.from( - `${metadata.clientInfo.client_id}:${metadata.clientInfo.client_secret}`, - ).toString('base64'); - headers['Authorization'] = `Basic ${clientAuth}`; + /** Default to client_secret_basic if no methods specified (per RFC 8414) */ + const tokenAuthMethods = authMethods ?? ['client_secret_basic']; + const usesBasicAuth = tokenAuthMethods.includes('client_secret_basic'); + const usesClientSecretPost = tokenAuthMethods.includes('client_secret_post'); + + if (usesBasicAuth) { + /** Use Basic auth */ + logger.debug('[MCPOAuth] Using client_secret_basic authentication method'); + const clientAuth = Buffer.from( + `${metadata.clientInfo.client_id}:${metadata.clientInfo.client_secret}`, + ).toString('base64'); + headers['Authorization'] = `Basic ${clientAuth}`; + } else if (usesClientSecretPost) { + /** Use client_secret_post */ + logger.debug('[MCPOAuth] Using client_secret_post authentication method'); + body.append('client_id', metadata.clientInfo.client_id); + body.append('client_secret', metadata.clientInfo.client_secret); + } else { + /** No recognized method, default to Basic auth per RFC */ + logger.debug('[MCPOAuth] No recognized auth method, defaulting to client_secret_basic'); + const clientAuth = Buffer.from( + `${metadata.clientInfo.client_id}:${metadata.clientInfo.client_secret}`, + ).toString('base64'); + headers['Authorization'] = `Basic ${clientAuth}`; + } } else { /** For public clients, client_id must be in the body */ + logger.debug('[MCPOAuth] Using public client authentication (no secret)'); body.append('client_id', metadata.clientInfo.client_id); } @@ -575,9 +599,6 @@ export class MCPOAuthHandler { logger.debug(`[MCPOAuth] Using pre-configured OAuth settings for token refresh`); const tokenUrl = new URL(config.token_url); - const clientAuth = config.client_secret - ? Buffer.from(`${config.client_id}:${config.client_secret}`).toString('base64') - : null; const body = new URLSearchParams({ grant_type: 'refresh_token', @@ -593,10 +614,44 @@ export class MCPOAuthHandler { Accept: 'application/json', }; - if (clientAuth) { - headers['Authorization'] = `Basic ${clientAuth}`; + /** Handle authentication based on configured methods */ + if (config.client_secret) { + /** Default to client_secret_basic if no methods specified (per RFC 8414) */ + const tokenAuthMethods = config.token_endpoint_auth_methods_supported ?? [ + 'client_secret_basic', + ]; + const usesBasicAuth = tokenAuthMethods.includes('client_secret_basic'); + const usesClientSecretPost = tokenAuthMethods.includes('client_secret_post'); + + if (usesBasicAuth) { + /** Use Basic auth */ + logger.debug( + '[MCPOAuth] Using client_secret_basic authentication method (pre-configured)', + ); + const clientAuth = Buffer.from(`${config.client_id}:${config.client_secret}`).toString( + 'base64', + ); + headers['Authorization'] = `Basic ${clientAuth}`; + } else if (usesClientSecretPost) { + /** Use client_secret_post */ + logger.debug( + '[MCPOAuth] Using client_secret_post authentication method (pre-configured)', + ); + body.append('client_id', config.client_id); + body.append('client_secret', config.client_secret); + } else { + /** No recognized method, default to Basic auth per RFC */ + logger.debug( + '[MCPOAuth] No recognized auth method, defaulting to client_secret_basic (pre-configured)', + ); + const clientAuth = Buffer.from(`${config.client_id}:${config.client_secret}`).toString( + 'base64', + ); + headers['Authorization'] = `Basic ${clientAuth}`; + } } else { - // Use client_id in body for public clients + /** For public clients, client_id must be in the body */ + logger.debug('[MCPOAuth] Using public client authentication (no secret, pre-configured)'); body.append('client_id', config.client_id); }