Skip to content

Commit 6ebee06

Browse files
danny-aviladustinhealy
danny-avila
and
dustinhealy
authored
🤝 fix: Respect Server Token Endpoint Auth Method Preference in MCP OAuth (danny-avila#12052)
* 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>
1 parent 4af2347 commit 6ebee06

5 files changed

Lines changed: 644 additions & 86 deletions

File tree

‎packages/api/src/mcp/__tests__/handler.test.ts‎

Lines changed: 306 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { TokenExchangeMethodEnum } from 'librechat-data-provider';
12
import type { MCPOptions } from 'librechat-data-provider';
23
import type { AuthorizationServerMetadata } from '@modelcontextprotocol/sdk/shared/auth.js';
34
import { MCPOAuthFlowMetadata, MCPOAuthHandler, MCPOAuthTokens } from '~/mcp/oauth';
@@ -898,7 +899,7 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => {
898899
it('passes headers to client registration', async () => {
899900
mockRegisterClient.mockImplementation(async (_, options) => {
900901
await options.fetchFn?.('http://example.com/register', {});
901-
return { client_id: 'test', redirect_uris: [] };
902+
return { client_id: 'test', redirect_uris: [], logo_uri: undefined, tos_uri: undefined };
902903
});
903904

904905
await MCPOAuthHandler.initiateOAuthFlow(
@@ -993,6 +994,308 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => {
993994
});
994995
});
995996

997+
describe('Fetch wrapper client_secret_basic body cleanup', () => {
998+
const originalFetch = global.fetch;
999+
const mockFetch = jest.fn() as unknown as jest.MockedFunction<typeof fetch>;
1000+
1001+
beforeEach(() => {
1002+
jest.clearAllMocks();
1003+
global.fetch = mockFetch;
1004+
});
1005+
1006+
afterEach(() => {
1007+
mockFetch.mockClear();
1008+
});
1009+
1010+
afterAll(() => {
1011+
global.fetch = originalFetch;
1012+
});
1013+
1014+
it('should remove client_id and client_secret from body when using client_secret_basic via completeOAuthFlow', async () => {
1015+
const mockFlowManager = {
1016+
getFlowState: jest.fn().mockResolvedValue({
1017+
status: 'PENDING',
1018+
metadata: {
1019+
serverName: 'test-server',
1020+
serverUrl: 'https://example.com/mcp',
1021+
codeVerifier: 'test-verifier',
1022+
clientInfo: {
1023+
client_id: 'test-client-id',
1024+
client_secret: 'test-client-secret',
1025+
redirect_uris: ['http://localhost:3080/api/mcp/test-server/oauth/callback'],
1026+
token_endpoint_auth_method: 'client_secret_basic',
1027+
},
1028+
metadata: {
1029+
issuer: 'https://example.com',
1030+
authorization_endpoint: 'https://example.com/authorize',
1031+
token_endpoint: 'https://example.com/token',
1032+
response_types_supported: ['code'],
1033+
token_endpoint_auth_methods_supported: ['client_secret_basic'],
1034+
},
1035+
} as MCPOAuthFlowMetadata,
1036+
}),
1037+
completeFlow: jest.fn(),
1038+
} as unknown as FlowStateManager<MCPOAuthTokens>;
1039+
1040+
mockExchangeAuthorization.mockImplementation(async (_, options) => {
1041+
const body = new URLSearchParams({
1042+
grant_type: 'authorization_code',
1043+
code: 'test-auth-code',
1044+
client_id: 'test-client-id',
1045+
client_secret: 'test-client-secret',
1046+
});
1047+
await options.fetchFn?.('https://example.com/token', {
1048+
method: 'POST',
1049+
body,
1050+
});
1051+
return { access_token: 'test-token', token_type: 'Bearer', expires_in: 3600 };
1052+
});
1053+
1054+
await MCPOAuthHandler.completeOAuthFlow('test-flow', 'test-auth-code', mockFlowManager, {});
1055+
1056+
const callArgs = mockFetch.mock.calls[0];
1057+
const sentBody = callArgs[1]?.body as string;
1058+
expect(sentBody).not.toContain('client_id=');
1059+
expect(sentBody).not.toContain('client_secret=');
1060+
1061+
const sentHeaders = callArgs[1]?.headers as Headers;
1062+
expect(sentHeaders.get('Authorization')).toMatch(/^Basic /);
1063+
});
1064+
});
1065+
1066+
describe('completeOAuthFlow auth method propagation', () => {
1067+
const originalFetch = global.fetch;
1068+
const mockFetch = jest.fn() as unknown as jest.MockedFunction<typeof fetch>;
1069+
1070+
beforeEach(() => {
1071+
jest.clearAllMocks();
1072+
global.fetch = mockFetch;
1073+
});
1074+
1075+
afterEach(() => {
1076+
mockFetch.mockClear();
1077+
});
1078+
1079+
afterAll(() => {
1080+
global.fetch = originalFetch;
1081+
});
1082+
1083+
it('should use client_secret_post when clientInfo specifies that method', async () => {
1084+
const mockFlowManager = {
1085+
getFlowState: jest.fn().mockResolvedValue({
1086+
status: 'PENDING',
1087+
metadata: {
1088+
serverName: 'test-server',
1089+
serverUrl: 'https://example.com/mcp',
1090+
codeVerifier: 'test-verifier',
1091+
clientInfo: {
1092+
client_id: 'test-client-id',
1093+
client_secret: 'test-client-secret',
1094+
redirect_uris: ['http://localhost:3080/api/mcp/test-server/oauth/callback'],
1095+
token_endpoint_auth_method: 'client_secret_post',
1096+
},
1097+
metadata: {
1098+
issuer: 'https://example.com',
1099+
authorization_endpoint: 'https://example.com/authorize',
1100+
token_endpoint: 'https://example.com/token',
1101+
response_types_supported: ['code'],
1102+
token_endpoint_auth_methods_supported: ['client_secret_post'],
1103+
},
1104+
} as MCPOAuthFlowMetadata,
1105+
}),
1106+
completeFlow: jest.fn(),
1107+
} as unknown as FlowStateManager<MCPOAuthTokens>;
1108+
1109+
mockExchangeAuthorization.mockImplementation(async (_, options) => {
1110+
const body = new URLSearchParams({
1111+
grant_type: 'authorization_code',
1112+
code: 'test-auth-code',
1113+
});
1114+
await options.fetchFn?.('https://example.com/token', {
1115+
method: 'POST',
1116+
body,
1117+
});
1118+
return { access_token: 'test-token', token_type: 'Bearer', expires_in: 3600 };
1119+
});
1120+
1121+
await MCPOAuthHandler.completeOAuthFlow('test-flow', 'test-auth-code', mockFlowManager, {});
1122+
1123+
const callArgs = mockFetch.mock.calls[0];
1124+
const sentBody = callArgs[1]?.body as string;
1125+
expect(sentBody).toContain('client_id=test-client-id');
1126+
expect(sentBody).toContain('client_secret=test-client-secret');
1127+
1128+
const sentHeaders = callArgs[1]?.headers as Headers;
1129+
expect(sentHeaders.has('Authorization')).toBe(false);
1130+
});
1131+
1132+
it('should use none auth when clientInfo has no secret', async () => {
1133+
const mockFlowManager = {
1134+
getFlowState: jest.fn().mockResolvedValue({
1135+
status: 'PENDING',
1136+
metadata: {
1137+
serverName: 'test-server',
1138+
serverUrl: 'https://example.com/mcp',
1139+
codeVerifier: 'test-verifier',
1140+
clientInfo: {
1141+
client_id: 'test-client-id',
1142+
redirect_uris: ['http://localhost:3080/api/mcp/test-server/oauth/callback'],
1143+
token_endpoint_auth_method: 'none',
1144+
},
1145+
metadata: {
1146+
issuer: 'https://example.com',
1147+
authorization_endpoint: 'https://example.com/authorize',
1148+
token_endpoint: 'https://example.com/token',
1149+
response_types_supported: ['code'],
1150+
token_endpoint_auth_methods_supported: ['none'],
1151+
},
1152+
} as MCPOAuthFlowMetadata,
1153+
}),
1154+
completeFlow: jest.fn(),
1155+
} as unknown as FlowStateManager<MCPOAuthTokens>;
1156+
1157+
mockExchangeAuthorization.mockImplementation(async (_, options) => {
1158+
const body = new URLSearchParams({
1159+
grant_type: 'authorization_code',
1160+
code: 'test-auth-code',
1161+
});
1162+
await options.fetchFn?.('https://example.com/token', {
1163+
method: 'POST',
1164+
body,
1165+
});
1166+
return { access_token: 'test-token', token_type: 'Bearer', expires_in: 3600 };
1167+
});
1168+
1169+
await MCPOAuthHandler.completeOAuthFlow('test-flow', 'test-auth-code', mockFlowManager, {});
1170+
1171+
const callArgs = mockFetch.mock.calls[0];
1172+
const sentBody = callArgs[1]?.body as string;
1173+
expect(sentBody).toContain('client_id=test-client-id');
1174+
expect(sentBody).not.toContain('client_secret=');
1175+
1176+
const sentHeaders = callArgs[1]?.headers as Headers;
1177+
expect(sentHeaders.has('Authorization')).toBe(false);
1178+
});
1179+
});
1180+
1181+
describe('refreshOAuthTokens with forced token_exchange_method', () => {
1182+
const originalFetch = global.fetch;
1183+
const mockFetch = jest.fn() as unknown as jest.MockedFunction<typeof fetch>;
1184+
1185+
beforeEach(() => {
1186+
jest.clearAllMocks();
1187+
global.fetch = mockFetch;
1188+
});
1189+
1190+
afterEach(() => {
1191+
mockFetch.mockClear();
1192+
});
1193+
1194+
afterAll(() => {
1195+
global.fetch = originalFetch;
1196+
});
1197+
1198+
it('should force client_secret_post even when server advertises client_secret_basic', async () => {
1199+
const metadata = {
1200+
serverName: 'test-server',
1201+
serverUrl: 'https://auth.example.com',
1202+
clientInfo: {
1203+
client_id: 'test-client-id',
1204+
client_secret: 'test-client-secret',
1205+
token_endpoint_auth_method: 'client_secret_basic',
1206+
},
1207+
};
1208+
1209+
mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce({
1210+
issuer: 'https://auth.example.com',
1211+
authorization_endpoint: 'https://auth.example.com/oauth/authorize',
1212+
token_endpoint: 'https://auth.example.com/oauth/token',
1213+
token_endpoint_auth_methods_supported: ['client_secret_basic'],
1214+
response_types_supported: ['code'],
1215+
jwks_uri: 'https://auth.example.com/.well-known/jwks.json',
1216+
subject_types_supported: ['public'],
1217+
id_token_signing_alg_values_supported: ['RS256'],
1218+
} as AuthorizationServerMetadata);
1219+
1220+
mockFetch.mockResolvedValueOnce({
1221+
ok: true,
1222+
json: async () => ({
1223+
access_token: 'new-access-token',
1224+
refresh_token: 'new-refresh-token',
1225+
expires_in: 3600,
1226+
}),
1227+
} as Response);
1228+
1229+
await MCPOAuthHandler.refreshOAuthTokens('refresh-token', metadata, {}, {
1230+
token_exchange_method: TokenExchangeMethodEnum.DefaultPost,
1231+
} as MCPOptions['oauth']);
1232+
1233+
expect(mockFetch).toHaveBeenCalledWith(
1234+
'https://auth.example.com/oauth/token',
1235+
expect.objectContaining({
1236+
method: 'POST',
1237+
headers: expect.not.objectContaining({
1238+
Authorization: expect.any(String),
1239+
}),
1240+
}),
1241+
);
1242+
1243+
const callArgs = mockFetch.mock.calls[0];
1244+
const body = callArgs[1]?.body as URLSearchParams;
1245+
expect(body.toString()).toContain('client_id=test-client-id');
1246+
expect(body.toString()).toContain('client_secret=test-client-secret');
1247+
});
1248+
});
1249+
1250+
describe('revokeOAuthToken with empty auth methods', () => {
1251+
const originalFetch = global.fetch;
1252+
const mockFetch = jest.fn() as unknown as jest.MockedFunction<typeof fetch>;
1253+
1254+
beforeEach(() => {
1255+
jest.clearAllMocks();
1256+
global.fetch = mockFetch;
1257+
});
1258+
1259+
afterEach(() => {
1260+
mockFetch.mockClear();
1261+
});
1262+
1263+
afterAll(() => {
1264+
global.fetch = originalFetch;
1265+
});
1266+
1267+
it('should send no client credentials when revocationEndpointAuthMethodsSupported is empty', async () => {
1268+
const metadata = {
1269+
serverUrl: 'https://auth.example.com',
1270+
clientId: 'test-client-id',
1271+
clientSecret: 'test-client-secret',
1272+
revocationEndpoint: 'https://auth.example.com/oauth/revoke',
1273+
revocationEndpointAuthMethodsSupported: [] as string[],
1274+
};
1275+
1276+
mockFetch.mockResolvedValueOnce({
1277+
ok: true,
1278+
status: 200,
1279+
} as Response);
1280+
1281+
await MCPOAuthHandler.revokeOAuthToken('test-server', 'test-token', 'access', metadata);
1282+
1283+
expect(mockFetch).toHaveBeenCalledWith(
1284+
expect.any(URL),
1285+
expect.objectContaining({
1286+
headers: expect.not.objectContaining({
1287+
Authorization: expect.any(String),
1288+
}),
1289+
}),
1290+
);
1291+
1292+
const callArgs = mockFetch.mock.calls[0];
1293+
const body = callArgs[1]?.body as string;
1294+
expect(body).not.toContain('client_id=');
1295+
expect(body).not.toContain('client_secret=');
1296+
});
1297+
});
1298+
9961299
describe('Fallback OAuth Metadata (Legacy Server Support)', () => {
9971300
const originalFetch = global.fetch;
9981301
const mockFetch = jest.fn();
@@ -1020,6 +1323,8 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => {
10201323
client_id: 'dynamic-client-id',
10211324
client_secret: 'dynamic-client-secret',
10221325
redirect_uris: ['http://localhost:3080/api/mcp/test-server/oauth/callback'],
1326+
logo_uri: undefined,
1327+
tos_uri: undefined,
10231328
});
10241329

10251330
// Mock startAuthorization to return a successful response

0 commit comments

Comments
 (0)