From 6ebee069c756348a7de3d1c6232c308917ed6165 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Tue, 3 Mar 2026 22:44:13 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=A4=9D=20fix:=20Respect=20Server=20Token?= =?UTF-8?q?=20Endpoint=20Auth=20Method=20Preference=20in=20MCP=20OAuth=20(?= =?UTF-8?q?#12052)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(mcp): respect server's token endpoint auth method preference order * fix(mcp): update token endpoint auth method to client_secret_basic * fix(mcp): correct auth method to client_secret_basic in OAuth handler * test(mcp): add tests for OAuth client registration method selection based on server preferences * refactor(mcp): extract and implement token endpoint auth methods into separate utility functions - Moved token endpoint authentication method logic from the MCPOAuthHandler to new utility functions in methods.ts for better organization and reusability. - Added tests for the new methods to ensure correct behavior in selecting and resolving authentication methods based on server preferences and token exchange methods. - Updated MCPOAuthHandler to utilize the new utility functions, improving code clarity and maintainability. * chore(mcp): remove redundant comments in OAuth handler - Cleaned up the MCPOAuthHandler by removing unnecessary comments related to authentication methods, improving code readability and maintainability. * refactor(mcp): update supported auth methods to use ReadonlySet for better performance - Changed the SUPPORTED_AUTH_METHODS from an array to a ReadonlySet for improved lookup efficiency. - Enhanced the logic in selectRegistrationAuthMethod to prioritize credential-based methods and handle cases where the server advertises 'none' correctly, ensuring compliance with RFC 7591. * test(mcp): add tests for selectRegistrationAuthMethod to handle 'none' and empty array cases - Introduced new test cases to ensure selectRegistrationAuthMethod correctly prioritizes credential-based methods over 'none' when listed first or before other methods. - Added a test to verify that an empty token_endpoint_auth_methods_supported returns undefined, adhering to RFC 8414. * refactor(mcp): streamline authentication method handling in OAuth handler - Simplified the logic for determining the authentication method by consolidating checks into a single function call. - Removed redundant checks for supported auth methods, enhancing code clarity and maintainability. - Updated the request header and body handling based on the resolved authentication method. * fix(mcp): ensure compliance with RFC 6749 by removing credentials from body when using client_secret_basic - Updated the MCPOAuthHandler to delete client_id and client_secret from body parameters when using the client_secret_basic authentication method, ensuring adherence to RFC 6749 §2.3.1. * test(mcp): add tests for OAuth flow handling of client_secret_basic and client_secret_post methods - Introduced new test cases to verify that the MCPOAuthHandler correctly removes client_id and client_secret from the request body when using client_secret_basic. - Added tests to ensure proper handling of client_secret_post and none authentication methods, confirming that the correct parameters are included or excluded based on the specified method. - Enhanced the test suite for completeOAuthFlow to cover various scenarios, ensuring compliance with OAuth 2.0 specifications. * test(mcp): enhance tests for selectRegistrationAuthMethod and resolveTokenEndpointAuthMethod - Added new test cases to verify the selection of the first supported credential method from a mixed list in selectRegistrationAuthMethod. - Included tests to ensure resolveTokenEndpointAuthMethod correctly ignores unsupported preferred methods and handles empty tokenAuthMethods, returning undefined as expected. - Improved test coverage for various scenarios in the OAuth flow, ensuring compliance with relevant specifications. --------- Co-authored-by: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> --- .../api/src/mcp/__tests__/handler.test.ts | 307 +++++++++++++++++- .../api/src/mcp/__tests__/methods.test.ts | 195 +++++++++++ packages/api/src/mcp/oauth/handler.ts | 116 ++----- packages/api/src/mcp/oauth/index.ts | 1 + packages/api/src/mcp/oauth/methods.ts | 111 +++++++ 5 files changed, 644 insertions(+), 86 deletions(-) create mode 100644 packages/api/src/mcp/__tests__/methods.test.ts create mode 100644 packages/api/src/mcp/oauth/methods.ts diff --git a/packages/api/src/mcp/__tests__/handler.test.ts b/packages/api/src/mcp/__tests__/handler.test.ts index f7347b8bbe..db88afe581 100644 --- a/packages/api/src/mcp/__tests__/handler.test.ts +++ b/packages/api/src/mcp/__tests__/handler.test.ts @@ -1,3 +1,4 @@ +import { TokenExchangeMethodEnum } from 'librechat-data-provider'; import type { MCPOptions } from 'librechat-data-provider'; import type { AuthorizationServerMetadata } from '@modelcontextprotocol/sdk/shared/auth.js'; import { MCPOAuthFlowMetadata, MCPOAuthHandler, MCPOAuthTokens } from '~/mcp/oauth'; @@ -898,7 +899,7 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => { it('passes headers to client registration', async () => { mockRegisterClient.mockImplementation(async (_, options) => { await options.fetchFn?.('http://example.com/register', {}); - return { client_id: 'test', redirect_uris: [] }; + return { client_id: 'test', redirect_uris: [], logo_uri: undefined, tos_uri: undefined }; }); await MCPOAuthHandler.initiateOAuthFlow( @@ -993,6 +994,308 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => { }); }); + describe('Fetch wrapper client_secret_basic body cleanup', () => { + 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; + }); + + it('should remove client_id and client_secret from body when using client_secret_basic via completeOAuthFlow', async () => { + const mockFlowManager = { + getFlowState: jest.fn().mockResolvedValue({ + status: 'PENDING', + metadata: { + serverName: 'test-server', + serverUrl: 'https://example.com/mcp', + codeVerifier: 'test-verifier', + clientInfo: { + client_id: 'test-client-id', + client_secret: 'test-client-secret', + redirect_uris: ['http://localhost:3080/api/mcp/test-server/oauth/callback'], + token_endpoint_auth_method: 'client_secret_basic', + }, + metadata: { + issuer: 'https://example.com', + authorization_endpoint: 'https://example.com/authorize', + token_endpoint: 'https://example.com/token', + response_types_supported: ['code'], + token_endpoint_auth_methods_supported: ['client_secret_basic'], + }, + } as MCPOAuthFlowMetadata, + }), + completeFlow: jest.fn(), + } as unknown as FlowStateManager; + + mockExchangeAuthorization.mockImplementation(async (_, options) => { + const body = new URLSearchParams({ + grant_type: 'authorization_code', + code: 'test-auth-code', + client_id: 'test-client-id', + client_secret: 'test-client-secret', + }); + await options.fetchFn?.('https://example.com/token', { + method: 'POST', + body, + }); + return { access_token: 'test-token', token_type: 'Bearer', expires_in: 3600 }; + }); + + await MCPOAuthHandler.completeOAuthFlow('test-flow', 'test-auth-code', mockFlowManager, {}); + + const callArgs = mockFetch.mock.calls[0]; + const sentBody = callArgs[1]?.body as string; + expect(sentBody).not.toContain('client_id='); + expect(sentBody).not.toContain('client_secret='); + + const sentHeaders = callArgs[1]?.headers as Headers; + expect(sentHeaders.get('Authorization')).toMatch(/^Basic /); + }); + }); + + describe('completeOAuthFlow auth method propagation', () => { + 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; + }); + + it('should use client_secret_post when clientInfo specifies that method', async () => { + const mockFlowManager = { + getFlowState: jest.fn().mockResolvedValue({ + status: 'PENDING', + metadata: { + serverName: 'test-server', + serverUrl: 'https://example.com/mcp', + codeVerifier: 'test-verifier', + clientInfo: { + client_id: 'test-client-id', + client_secret: 'test-client-secret', + redirect_uris: ['http://localhost:3080/api/mcp/test-server/oauth/callback'], + token_endpoint_auth_method: 'client_secret_post', + }, + metadata: { + issuer: 'https://example.com', + authorization_endpoint: 'https://example.com/authorize', + token_endpoint: 'https://example.com/token', + response_types_supported: ['code'], + token_endpoint_auth_methods_supported: ['client_secret_post'], + }, + } as MCPOAuthFlowMetadata, + }), + completeFlow: jest.fn(), + } as unknown as FlowStateManager; + + mockExchangeAuthorization.mockImplementation(async (_, options) => { + const body = new URLSearchParams({ + grant_type: 'authorization_code', + code: 'test-auth-code', + }); + await options.fetchFn?.('https://example.com/token', { + method: 'POST', + body, + }); + return { access_token: 'test-token', token_type: 'Bearer', expires_in: 3600 }; + }); + + await MCPOAuthHandler.completeOAuthFlow('test-flow', 'test-auth-code', mockFlowManager, {}); + + const callArgs = mockFetch.mock.calls[0]; + const sentBody = callArgs[1]?.body as string; + expect(sentBody).toContain('client_id=test-client-id'); + expect(sentBody).toContain('client_secret=test-client-secret'); + + const sentHeaders = callArgs[1]?.headers as Headers; + expect(sentHeaders.has('Authorization')).toBe(false); + }); + + it('should use none auth when clientInfo has no secret', async () => { + const mockFlowManager = { + getFlowState: jest.fn().mockResolvedValue({ + status: 'PENDING', + metadata: { + serverName: 'test-server', + serverUrl: 'https://example.com/mcp', + codeVerifier: 'test-verifier', + clientInfo: { + client_id: 'test-client-id', + redirect_uris: ['http://localhost:3080/api/mcp/test-server/oauth/callback'], + token_endpoint_auth_method: 'none', + }, + metadata: { + issuer: 'https://example.com', + authorization_endpoint: 'https://example.com/authorize', + token_endpoint: 'https://example.com/token', + response_types_supported: ['code'], + token_endpoint_auth_methods_supported: ['none'], + }, + } as MCPOAuthFlowMetadata, + }), + completeFlow: jest.fn(), + } as unknown as FlowStateManager; + + mockExchangeAuthorization.mockImplementation(async (_, options) => { + const body = new URLSearchParams({ + grant_type: 'authorization_code', + code: 'test-auth-code', + }); + await options.fetchFn?.('https://example.com/token', { + method: 'POST', + body, + }); + return { access_token: 'test-token', token_type: 'Bearer', expires_in: 3600 }; + }); + + await MCPOAuthHandler.completeOAuthFlow('test-flow', 'test-auth-code', mockFlowManager, {}); + + const callArgs = mockFetch.mock.calls[0]; + const sentBody = callArgs[1]?.body as string; + expect(sentBody).toContain('client_id=test-client-id'); + expect(sentBody).not.toContain('client_secret='); + + const sentHeaders = callArgs[1]?.headers as Headers; + expect(sentHeaders.has('Authorization')).toBe(false); + }); + }); + + describe('refreshOAuthTokens with forced token_exchange_method', () => { + 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; + }); + + it('should force client_secret_post even when server advertises client_secret_basic', async () => { + const metadata = { + serverName: 'test-server', + serverUrl: 'https://auth.example.com', + clientInfo: { + client_id: 'test-client-id', + client_secret: 'test-client-secret', + token_endpoint_auth_method: 'client_secret_basic', + }, + }; + + 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('refresh-token', metadata, {}, { + token_exchange_method: TokenExchangeMethodEnum.DefaultPost, + } as MCPOptions['oauth']); + + expect(mockFetch).toHaveBeenCalledWith( + 'https://auth.example.com/oauth/token', + expect.objectContaining({ + method: 'POST', + headers: expect.not.objectContaining({ + Authorization: expect.any(String), + }), + }), + ); + + 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'); + }); + }); + + describe('revokeOAuthToken with empty auth methods', () => { + 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; + }); + + it('should send no client credentials when revocationEndpointAuthMethodsSupported is empty', async () => { + const metadata = { + serverUrl: 'https://auth.example.com', + clientId: 'test-client-id', + clientSecret: 'test-client-secret', + revocationEndpoint: 'https://auth.example.com/oauth/revoke', + revocationEndpointAuthMethodsSupported: [] as string[], + }; + + mockFetch.mockResolvedValueOnce({ + ok: true, + status: 200, + } as Response); + + await MCPOAuthHandler.revokeOAuthToken('test-server', 'test-token', 'access', metadata); + + expect(mockFetch).toHaveBeenCalledWith( + expect.any(URL), + expect.objectContaining({ + headers: expect.not.objectContaining({ + Authorization: expect.any(String), + }), + }), + ); + + const callArgs = mockFetch.mock.calls[0]; + const body = callArgs[1]?.body as string; + expect(body).not.toContain('client_id='); + expect(body).not.toContain('client_secret='); + }); + }); + describe('Fallback OAuth Metadata (Legacy Server Support)', () => { const originalFetch = global.fetch; const mockFetch = jest.fn(); @@ -1020,6 +1323,8 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => { client_id: 'dynamic-client-id', client_secret: 'dynamic-client-secret', redirect_uris: ['http://localhost:3080/api/mcp/test-server/oauth/callback'], + logo_uri: undefined, + tos_uri: undefined, }); // Mock startAuthorization to return a successful response diff --git a/packages/api/src/mcp/__tests__/methods.test.ts b/packages/api/src/mcp/__tests__/methods.test.ts new file mode 100644 index 0000000000..acd96186a3 --- /dev/null +++ b/packages/api/src/mcp/__tests__/methods.test.ts @@ -0,0 +1,195 @@ +import { TokenExchangeMethodEnum } from 'librechat-data-provider'; +import { + getForcedTokenEndpointAuthMethod, + resolveTokenEndpointAuthMethod, + selectRegistrationAuthMethod, + inferClientAuthMethod, +} from '~/mcp/oauth/methods'; + +describe('getForcedTokenEndpointAuthMethod', () => { + it('returns client_secret_post for DefaultPost', () => { + expect(getForcedTokenEndpointAuthMethod(TokenExchangeMethodEnum.DefaultPost)).toBe( + 'client_secret_post', + ); + }); + + it('returns client_secret_basic for BasicAuthHeader', () => { + expect(getForcedTokenEndpointAuthMethod(TokenExchangeMethodEnum.BasicAuthHeader)).toBe( + 'client_secret_basic', + ); + }); + + it('returns undefined when not set', () => { + expect(getForcedTokenEndpointAuthMethod(undefined)).toBeUndefined(); + }); +}); + +describe('selectRegistrationAuthMethod', () => { + it('respects server preference order: client_secret_post first', () => { + expect(selectRegistrationAuthMethod(['client_secret_post', 'client_secret_basic'])).toBe( + 'client_secret_post', + ); + }); + + it('respects server preference order: client_secret_basic first', () => { + expect(selectRegistrationAuthMethod(['client_secret_basic', 'client_secret_post'])).toBe( + 'client_secret_basic', + ); + }); + + it('selects none when server only advertises none', () => { + expect(selectRegistrationAuthMethod(['none'])).toBe('none'); + }); + + it('prefers credential-based method over none when server lists none first', () => { + expect(selectRegistrationAuthMethod(['none', 'client_secret_basic'])).toBe( + 'client_secret_basic', + ); + }); + + it('prefers credential-based method over none when server lists none before post', () => { + expect(selectRegistrationAuthMethod(['none', 'client_secret_post'])).toBe('client_secret_post'); + }); + + it('falls back to server first method when none of our methods match', () => { + expect(selectRegistrationAuthMethod(['private_key_jwt', 'tls_client_auth'])).toBe( + 'private_key_jwt', + ); + }); + + it('returns undefined when server omits token_endpoint_auth_methods_supported (RFC 8414 default preserved)', () => { + expect(selectRegistrationAuthMethod(undefined)).toBeUndefined(); + }); + + it('returns undefined for empty token_endpoint_auth_methods_supported (RFC 8414 forbids zero-element arrays)', () => { + expect(selectRegistrationAuthMethod([])).toBeUndefined(); + }); + + it('forced token_exchange_method overrides server preference', () => { + expect( + selectRegistrationAuthMethod(['client_secret_basic'], TokenExchangeMethodEnum.DefaultPost), + ).toBe('client_secret_post'); + }); + + it('forced BasicAuthHeader overrides server preference', () => { + expect( + selectRegistrationAuthMethod( + ['client_secret_post', 'none'], + TokenExchangeMethodEnum.BasicAuthHeader, + ), + ).toBe('client_secret_basic'); + }); + + it('picks first supported credential method from mixed supported/unsupported list', () => { + expect(selectRegistrationAuthMethod(['private_key_jwt', 'client_secret_post'])).toBe( + 'client_secret_post', + ); + }); + + it('skips unsupported and none to find credential method deeper in the list', () => { + expect(selectRegistrationAuthMethod(['tls_client_auth', 'none', 'client_secret_basic'])).toBe( + 'client_secret_basic', + ); + }); +}); + +describe('resolveTokenEndpointAuthMethod', () => { + it('prefers forced tokenExchangeMethod over everything', () => { + expect( + resolveTokenEndpointAuthMethod({ + tokenExchangeMethod: TokenExchangeMethodEnum.DefaultPost, + tokenAuthMethods: ['client_secret_basic'], + preferredMethod: 'client_secret_basic', + }), + ).toBe('client_secret_post'); + }); + + it('prefers DCR registration response method when no forced override', () => { + expect( + resolveTokenEndpointAuthMethod({ + tokenAuthMethods: ['client_secret_basic', 'client_secret_post'], + preferredMethod: 'client_secret_post', + }), + ).toBe('client_secret_post'); + }); + + it('falls back to server methods when no preferred method', () => { + expect( + resolveTokenEndpointAuthMethod({ + tokenAuthMethods: ['client_secret_post', 'client_secret_basic'], + }), + ).toBe('client_secret_basic'); + }); + + it('picks client_secret_post when basic is not in server methods', () => { + expect( + resolveTokenEndpointAuthMethod({ + tokenAuthMethods: ['client_secret_post', 'none'], + }), + ).toBe('client_secret_post'); + }); + + it('returns undefined when no recognized methods', () => { + expect( + resolveTokenEndpointAuthMethod({ + tokenAuthMethods: ['private_key_jwt'], + }), + ).toBeUndefined(); + }); + + it('defaults to client_secret_basic when no methods advertised (RFC 8414)', () => { + expect( + resolveTokenEndpointAuthMethod({ + tokenAuthMethods: ['client_secret_basic'], + }), + ).toBe('client_secret_basic'); + }); + + it('ignores exotic preferredMethod and falls back to server methods', () => { + expect( + resolveTokenEndpointAuthMethod({ + preferredMethod: 'private_key_jwt', + tokenAuthMethods: ['client_secret_post'], + }), + ).toBe('client_secret_post'); + }); + + it('ignores none preferredMethod and falls back to server methods', () => { + expect( + resolveTokenEndpointAuthMethod({ + preferredMethod: 'none', + tokenAuthMethods: ['client_secret_basic'], + }), + ).toBe('client_secret_basic'); + }); + + it('returns undefined when tokenAuthMethods is empty', () => { + expect( + resolveTokenEndpointAuthMethod({ + tokenAuthMethods: [], + }), + ).toBeUndefined(); + }); +}); + +describe('inferClientAuthMethod', () => { + it('returns client_secret_basic when Authorization header is present', () => { + expect(inferClientAuthMethod(true, false, false, true)).toBe('client_secret_basic'); + }); + + it('returns client_secret_post when body has client_id', () => { + expect(inferClientAuthMethod(false, true, false, true)).toBe('client_secret_post'); + }); + + it('returns client_secret_post when body has client_secret', () => { + expect(inferClientAuthMethod(false, false, true, true)).toBe('client_secret_post'); + }); + + it('defaults to client_secret_basic for confidential client with no prior auth (RFC 8414)', () => { + expect(inferClientAuthMethod(false, false, false, true)).toBe('client_secret_basic'); + }); + + it('returns none for public client', () => { + expect(inferClientAuthMethod(false, false, false, false)).toBe('none'); + }); +}); diff --git a/packages/api/src/mcp/oauth/handler.ts b/packages/api/src/mcp/oauth/handler.ts index 976db8e68b..92b9f1211c 100644 --- a/packages/api/src/mcp/oauth/handler.ts +++ b/packages/api/src/mcp/oauth/handler.ts @@ -18,6 +18,12 @@ import type { MCPOAuthTokens, OAuthMetadata, } from './types'; +import { + resolveTokenEndpointAuthMethod, + getForcedTokenEndpointAuthMethod, + selectRegistrationAuthMethod, + inferClientAuthMethod, +} from './methods'; import { sanitizeUrlForLogging } from '~/mcp/utils'; /** Type for the OAuth metadata from the SDK */ @@ -27,39 +33,6 @@ export class MCPOAuthHandler { private static readonly FLOW_TYPE = 'mcp_oauth'; private static readonly FLOW_TTL = 10 * 60 * 1000; // 10 minutes - private static getForcedTokenEndpointAuthMethod( - tokenExchangeMethod?: TokenExchangeMethodEnum, - ): 'client_secret_basic' | 'client_secret_post' | undefined { - if (tokenExchangeMethod === TokenExchangeMethodEnum.DefaultPost) { - return 'client_secret_post'; - } - if (tokenExchangeMethod === TokenExchangeMethodEnum.BasicAuthHeader) { - return 'client_secret_basic'; - } - return undefined; - } - - private static resolveTokenEndpointAuthMethod(options: { - tokenExchangeMethod?: TokenExchangeMethodEnum; - tokenAuthMethods: string[]; - preferredMethod?: string; - }): 'client_secret_basic' | 'client_secret_post' | undefined { - const forcedMethod = this.getForcedTokenEndpointAuthMethod(options.tokenExchangeMethod); - const preferredMethod = forcedMethod ?? options.preferredMethod; - - if (preferredMethod === 'client_secret_basic' || preferredMethod === 'client_secret_post') { - return preferredMethod; - } - - if (options.tokenAuthMethods.includes('client_secret_basic')) { - return 'client_secret_basic'; - } - if (options.tokenAuthMethods.includes('client_secret_post')) { - return 'client_secret_post'; - } - return undefined; - } - /** * Creates a fetch function with custom headers injected */ @@ -95,19 +68,14 @@ export class MCPOAuthHandler { newHeaders.set('Content-Type', 'application/x-www-form-urlencoded'); if (clientInfo?.client_id) { - let authMethod = clientInfo.token_endpoint_auth_method; - - if (!authMethod) { - if (newHeaders.has('Authorization')) { - authMethod = 'client_secret_basic'; - } else if (params.has('client_id') || params.has('client_secret')) { - authMethod = 'client_secret_post'; - } else if (clientInfo.client_secret) { - authMethod = 'client_secret_post'; - } else { - authMethod = 'none'; - } - } + const authMethod = + clientInfo.token_endpoint_auth_method ?? + inferClientAuthMethod( + newHeaders.has('Authorization'), + params.has('client_id'), + params.has('client_secret'), + !!clientInfo.client_secret, + ); if (!clientInfo.client_secret || authMethod === 'none') { newHeaders.delete('Authorization'); @@ -123,6 +91,9 @@ export class MCPOAuthHandler { params.set('client_secret', clientInfo.client_secret); } } else if (authMethod === 'client_secret_basic') { + /** RFC 6749 §2.3.1: credentials MUST NOT appear in both the header and the body. The SDK defaults to body params, so remove them before setting the Basic header. */ + params.delete('client_id'); + params.delete('client_secret'); if (!newHeaders.has('Authorization')) { const clientAuth = Buffer.from( `${clientInfo.client_id}:${clientInfo.client_secret}`, @@ -300,22 +271,12 @@ export class MCPOAuthHandler { clientMetadata.response_types = metadata.response_types_supported || ['code']; - const forcedAuthMethod = this.getForcedTokenEndpointAuthMethod(tokenExchangeMethod); - - if (forcedAuthMethod) { - clientMetadata.token_endpoint_auth_method = forcedAuthMethod; - } else if (metadata.token_endpoint_auth_methods_supported) { - // Prefer client_secret_basic if supported, otherwise use the first supported method - if (metadata.token_endpoint_auth_methods_supported.includes('client_secret_basic')) { - clientMetadata.token_endpoint_auth_method = 'client_secret_basic'; - } else if (metadata.token_endpoint_auth_methods_supported.includes('client_secret_post')) { - clientMetadata.token_endpoint_auth_method = 'client_secret_post'; - } else if (metadata.token_endpoint_auth_methods_supported.includes('none')) { - clientMetadata.token_endpoint_auth_method = 'none'; - } else { - clientMetadata.token_endpoint_auth_method = - metadata.token_endpoint_auth_methods_supported[0]; - } + const selectedAuthMethod = selectRegistrationAuthMethod( + metadata.token_endpoint_auth_methods_supported, + tokenExchangeMethod, + ); + if (selectedAuthMethod) { + clientMetadata.token_endpoint_auth_method = selectedAuthMethod; } const availableScopes = resourceMetadata?.scopes_supported || metadata.scopes_supported; @@ -334,6 +295,7 @@ export class MCPOAuthHandler { fetchFn: this.createOAuthFetch(oauthHeaders), }); + const forcedAuthMethod = getForcedTokenEndpointAuthMethod(tokenExchangeMethod); if (forcedAuthMethod) { clientInfo.token_endpoint_auth_method = forcedAuthMethod; } else if (!clientInfo.token_endpoint_auth_method) { @@ -401,8 +363,7 @@ export class MCPOAuthHandler { // When token_exchange_method is undefined or not DefaultPost, default to using // client_secret_basic (Basic Auth header) for token endpoint authentication. tokenEndpointAuthMethod = - this.getForcedTokenEndpointAuthMethod(config.token_exchange_method) ?? - 'client_secret_basic'; + getForcedTokenEndpointAuthMethod(config.token_exchange_method) ?? 'client_secret_basic'; } let defaultTokenAuthMethods: string[]; @@ -815,26 +776,23 @@ export class MCPOAuthHandler { if (metadata.clientInfo.client_secret) { /** Default to client_secret_basic if no methods specified (per RFC 8414) */ const tokenAuthMethods = authMethods ?? ['client_secret_basic']; - const authMethod = this.resolveTokenEndpointAuthMethod({ + const authMethod = resolveTokenEndpointAuthMethod({ tokenExchangeMethod: config?.token_exchange_method, tokenAuthMethods, preferredMethod: metadata.clientInfo.token_endpoint_auth_method, }); if (authMethod === 'client_secret_basic') { - /** 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 (authMethod === 'client_secret_post') { - /** 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}`, @@ -896,13 +854,12 @@ export class MCPOAuthHandler { const tokenAuthMethods = config.token_endpoint_auth_methods_supported ?? [ 'client_secret_basic', ]; - const authMethod = this.resolveTokenEndpointAuthMethod({ + const authMethod = resolveTokenEndpointAuthMethod({ tokenExchangeMethod: config.token_exchange_method, tokenAuthMethods, }); if (authMethod === 'client_secret_basic') { - /** Use Basic auth */ logger.debug( '[MCPOAuth] Using client_secret_basic authentication method (pre-configured)', ); @@ -911,14 +868,12 @@ export class MCPOAuthHandler { ); headers['Authorization'] = `Basic ${clientAuth}`; } else if (authMethod === 'client_secret_post') { - /** 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)', ); @@ -1028,32 +983,23 @@ export class MCPOAuthHandler { ? new URL(metadata.revocationEndpoint) : new URL('/revoke', metadata.serverUrl); - // detect auth method to use - const authMethods = metadata.revocationEndpointAuthMethodsSupported ?? [ - 'client_secret_basic', // RFC 8414 (https://datatracker.ietf.org/doc/html/rfc8414) - ]; - const usesBasicAuth = authMethods.includes('client_secret_basic'); - const usesClientSecretPost = authMethods.includes('client_secret_post'); + const authMethods = metadata.revocationEndpointAuthMethodsSupported ?? ['client_secret_basic']; + const authMethod = resolveTokenEndpointAuthMethod({ tokenAuthMethods: authMethods }); - // init the request headers const headers: Record = { 'Content-Type': 'application/x-www-form-urlencoded', ...oauthHeaders, }; - // init the request body const body = new URLSearchParams({ token }); body.set('token_type_hint', tokenType === 'refresh' ? 'refresh_token' : 'access_token'); - // process auth method - if (usesBasicAuth) { - // encode the client id and secret and add to the headers + if (authMethod === 'client_secret_basic') { const credentials = Buffer.from(`${metadata.clientId}:${metadata.clientSecret}`).toString( 'base64', ); headers['Authorization'] = `Basic ${credentials}`; - } else if (usesClientSecretPost) { - // add the client id and secret to the body + } else if (authMethod === 'client_secret_post') { body.set('client_secret', metadata.clientSecret); body.set('client_id', metadata.clientId); } diff --git a/packages/api/src/mcp/oauth/index.ts b/packages/api/src/mcp/oauth/index.ts index d9c75071e0..1c4d98213e 100644 --- a/packages/api/src/mcp/oauth/index.ts +++ b/packages/api/src/mcp/oauth/index.ts @@ -2,3 +2,4 @@ export * from './types'; export * from './handler'; export * from './tokens'; export * from './detectOAuth'; +export * from './methods'; diff --git a/packages/api/src/mcp/oauth/methods.ts b/packages/api/src/mcp/oauth/methods.ts new file mode 100644 index 0000000000..a2b7cfd1e2 --- /dev/null +++ b/packages/api/src/mcp/oauth/methods.ts @@ -0,0 +1,111 @@ +import { TokenExchangeMethodEnum } from 'librechat-data-provider'; + +type ClientAuthMethod = 'client_secret_basic' | 'client_secret_post' | 'none'; + +/** Unordered roster of auth methods we can handle — order is irrelevant; server's list controls priority */ +const SUPPORTED_AUTH_METHODS: ReadonlySet = new Set([ + 'client_secret_basic', + 'client_secret_post', + 'none', +]); + +/** Maps a user-facing `TokenExchangeMethodEnum` to an OAuth auth method string. */ +export function getForcedTokenEndpointAuthMethod( + tokenExchangeMethod?: TokenExchangeMethodEnum, +): 'client_secret_basic' | 'client_secret_post' | undefined { + if (tokenExchangeMethod === TokenExchangeMethodEnum.DefaultPost) { + return 'client_secret_post'; + } + if (tokenExchangeMethod === TokenExchangeMethodEnum.BasicAuthHeader) { + return 'client_secret_basic'; + } + return undefined; +} + +/** + * Selects the auth method to request during dynamic client registration. + * + * Priority: + * 1. Forced override from `tokenExchangeMethod` config + * 2. First credential-based method from server's advertised list (skips `none` per RFC 7591 — + * `none` declares a public client, which is incorrect for DCR with a generated secret) + * 3. `none` if the server only advertises `none` + * 4. Server's first listed method (unsupported exotic method — best-effort) + * 5. Falls through to `undefined` (caller keeps its default) + */ +export function selectRegistrationAuthMethod( + serverAdvertised: string[] | undefined, + tokenExchangeMethod?: TokenExchangeMethodEnum, +): string | undefined { + const forced = getForcedTokenEndpointAuthMethod(tokenExchangeMethod); + if (forced) { + return forced; + } + + if (!serverAdvertised?.length) { + return undefined; + } + + const credentialPreferred = serverAdvertised.find( + (m) => SUPPORTED_AUTH_METHODS.has(m) && m !== 'none', + ); + if (credentialPreferred) { + return credentialPreferred; + } + + const serverPreferred = serverAdvertised.find((m) => SUPPORTED_AUTH_METHODS.has(m)); + return serverPreferred ?? serverAdvertised[0]; +} + +/** + * Resolves the auth method for token endpoint requests (refresh, pre-configured flows). + * + * Priority: + * 1. Forced override from `tokenExchangeMethod` config + * 2. Preferred method from client registration response (`clientInfo.token_endpoint_auth_method`) + * 3. First match from server's advertised methods + */ +export function resolveTokenEndpointAuthMethod(options: { + tokenExchangeMethod?: TokenExchangeMethodEnum; + tokenAuthMethods: string[]; + preferredMethod?: string; +}): 'client_secret_basic' | 'client_secret_post' | undefined { + const forced = getForcedTokenEndpointAuthMethod(options.tokenExchangeMethod); + const preferredMethod = forced ?? options.preferredMethod; + + if (preferredMethod === 'client_secret_basic' || preferredMethod === 'client_secret_post') { + return preferredMethod; + } + + if (options.tokenAuthMethods.includes('client_secret_basic')) { + return 'client_secret_basic'; + } + if (options.tokenAuthMethods.includes('client_secret_post')) { + return 'client_secret_post'; + } + return undefined; +} + +/** + * Infers the client auth method from request state when `clientInfo.token_endpoint_auth_method` + * is not set. Used inside the fetch wrapper to determine how credentials were applied by the SDK. + * + * Per RFC 8414 Section 2, defaults to `client_secret_basic` for confidential clients. + */ +export function inferClientAuthMethod( + hasAuthorizationHeader: boolean, + hasBodyClientId: boolean, + hasBodyClientSecret: boolean, + hasClientSecret: boolean, +): ClientAuthMethod { + if (hasAuthorizationHeader) { + return 'client_secret_basic'; + } + if (hasBodyClientId || hasBodyClientSecret) { + return 'client_secret_post'; + } + if (hasClientSecret) { + return 'client_secret_basic'; + } + return 'none'; +}