mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-03-15 20:26:33 +01:00
🪪 fix: MCP API Responses and OAuth Validation (#12217)
* 🔒 fix: Validate MCP Configs in Server Responses * 🔒 fix: Enhance OAuth URL Validation in MCPOAuthHandler - Introduced validation for OAuth URLs to ensure they do not target private or internal addresses, enhancing security against SSRF attacks. - Updated the OAuth flow to validate both authorization and token URLs before use, ensuring compliance with security standards. - Refactored redirect URI handling to streamline the OAuth client registration process. - Added comprehensive error handling for invalid URLs, improving robustness in OAuth interactions. * 🔒 feat: Implement Permission Checks for MCP Server Management - Added permission checkers for MCP server usage and creation, enhancing access control. - Updated routes for reinitializing MCP servers and retrieving authentication values to include these permission checks, ensuring only authorized users can access these functionalities. - Refactored existing permission logic to improve clarity and maintainability. * 🔒 fix: Enhance MCP Server Response Validation and Redaction - Updated MCP route tests to use `toMatchObject` for better validation of server response structures, ensuring consistency in expected properties. - Refactored the `redactServerSecrets` function to streamline the removal of sensitive information, ensuring that user-sourced API keys are properly redacted while retaining their source. - Improved OAuth security tests to validate rejection of private URLs across multiple endpoints, enhancing protection against SSRF vulnerabilities. - Added comprehensive tests for the `redactServerSecrets` function to ensure proper handling of various server configurations, reinforcing security measures. * chore: eslint * 🔒 fix: Enhance OAuth Server URL Validation in MCPOAuthHandler - Added validation for discovered authorization server URLs to ensure they meet security standards. - Improved logging to provide clearer insights when an authorization server is found from resource metadata. - Refactored the handling of authorization server URLs to enhance robustness against potential security vulnerabilities. * 🔒 test: Bypass SSRF validation for MCP OAuth Flow tests - Mocked SSRF validation functions to allow tests to use real local HTTP servers, facilitating more accurate testing of the MCP OAuth flow. - Updated test setup to ensure compatibility with the new mocking strategy, enhancing the reliability of the tests. * 🔒 fix: Add Validation for OAuth Metadata Endpoints in MCPOAuthHandler - Implemented checks for the presence and validity of registration and token endpoints in the OAuth metadata, enhancing security by ensuring that these URLs are properly validated before use. - Improved error handling and logging to provide better insights during the OAuth metadata processing, reinforcing the robustness of the OAuth flow. * 🔒 refactor: Simplify MCP Auth Values Endpoint Logic - Removed redundant permission checks for accessing the MCP server resource in the auth-values endpoint, streamlining the request handling process. - Consolidated error handling and response structure for improved clarity and maintainability. - Enhanced logging for better insights during the authentication value checks, reinforcing the robustness of the endpoint. * 🔒 test: Refactor LeaderElection Integration Tests for Improved Cleanup - Moved Redis key cleanup to the beforeEach hook to ensure a clean state before each test. - Enhanced afterEach logic to handle instance resignations and Redis key deletion more robustly, improving test reliability and maintainability.
This commit is contained in:
Danny Avila
GitHub
parent
f32907cd36
commit
fa9e1b228a
10 changed files with 845 additions and 102 deletions
|
|
@ -1693,12 +1693,14 @@ describe('MCP Routes', () => {
|
|||
it('should return all server configs for authenticated user', async () => {
|
||||
const mockServerConfigs = {
|
||||
'server-1': {
|
||||
endpoint: 'http://server1.com',
|
||||
name: 'Server 1',
|
||||
type: 'sse',
|
||||
url: 'http://server1.com/sse',
|
||||
title: 'Server 1',
|
||||
},
|
||||
'server-2': {
|
||||
endpoint: 'http://server2.com',
|
||||
name: 'Server 2',
|
||||
type: 'sse',
|
||||
url: 'http://server2.com/sse',
|
||||
title: 'Server 2',
|
||||
},
|
||||
};
|
||||
|
||||
|
|
@ -1707,7 +1709,18 @@ describe('MCP Routes', () => {
|
|||
const response = await request(app).get('/api/mcp/servers');
|
||||
|
||||
expect(response.status).toBe(200);
|
||||
expect(response.body).toEqual(mockServerConfigs);
|
||||
expect(response.body['server-1']).toMatchObject({
|
||||
type: 'sse',
|
||||
url: 'http://server1.com/sse',
|
||||
title: 'Server 1',
|
||||
});
|
||||
expect(response.body['server-2']).toMatchObject({
|
||||
type: 'sse',
|
||||
url: 'http://server2.com/sse',
|
||||
title: 'Server 2',
|
||||
});
|
||||
expect(response.body['server-1'].headers).toBeUndefined();
|
||||
expect(response.body['server-2'].headers).toBeUndefined();
|
||||
expect(mockRegistryInstance.getAllServerConfigs).toHaveBeenCalledWith('test-user-id');
|
||||
});
|
||||
|
||||
|
|
@ -1762,10 +1775,10 @@ describe('MCP Routes', () => {
|
|||
const response = await request(app).post('/api/mcp/servers').send({ config: validConfig });
|
||||
|
||||
expect(response.status).toBe(201);
|
||||
expect(response.body).toEqual({
|
||||
serverName: 'test-sse-server',
|
||||
...validConfig,
|
||||
});
|
||||
expect(response.body.serverName).toBe('test-sse-server');
|
||||
expect(response.body.type).toBe('sse');
|
||||
expect(response.body.url).toBe('https://mcp-server.example.com/sse');
|
||||
expect(response.body.title).toBe('Test SSE Server');
|
||||
expect(mockRegistryInstance.addServer).toHaveBeenCalledWith(
|
||||
'temp_server_name',
|
||||
expect.objectContaining({
|
||||
|
|
@ -1864,6 +1877,33 @@ describe('MCP Routes', () => {
|
|||
expect(mockRegistryInstance.addServer).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should redact secrets from create response', async () => {
|
||||
const validConfig = {
|
||||
type: 'sse',
|
||||
url: 'https://mcp-server.example.com/sse',
|
||||
title: 'Test Server',
|
||||
};
|
||||
|
||||
mockRegistryInstance.addServer.mockResolvedValue({
|
||||
serverName: 'test-server',
|
||||
config: {
|
||||
...validConfig,
|
||||
apiKey: { source: 'admin', authorization_type: 'bearer', key: 'admin-secret-key' },
|
||||
oauth: { client_id: 'cid', client_secret: 'admin-oauth-secret' },
|
||||
headers: { Authorization: 'Bearer leaked-token' },
|
||||
},
|
||||
});
|
||||
|
||||
const response = await request(app).post('/api/mcp/servers').send({ config: validConfig });
|
||||
|
||||
expect(response.status).toBe(201);
|
||||
expect(response.body.apiKey?.key).toBeUndefined();
|
||||
expect(response.body.oauth?.client_secret).toBeUndefined();
|
||||
expect(response.body.headers).toBeUndefined();
|
||||
expect(response.body.apiKey?.source).toBe('admin');
|
||||
expect(response.body.oauth?.client_id).toBe('cid');
|
||||
});
|
||||
|
||||
it('should return 500 when registry throws error', async () => {
|
||||
const validConfig = {
|
||||
type: 'sse',
|
||||
|
|
@ -1893,7 +1933,9 @@ describe('MCP Routes', () => {
|
|||
const response = await request(app).get('/api/mcp/servers/test-server');
|
||||
|
||||
expect(response.status).toBe(200);
|
||||
expect(response.body).toEqual(mockConfig);
|
||||
expect(response.body.type).toBe('sse');
|
||||
expect(response.body.url).toBe('https://mcp-server.example.com/sse');
|
||||
expect(response.body.title).toBe('Test Server');
|
||||
expect(mockRegistryInstance.getServerConfig).toHaveBeenCalledWith(
|
||||
'test-server',
|
||||
'test-user-id',
|
||||
|
|
@ -1909,6 +1951,29 @@ describe('MCP Routes', () => {
|
|||
expect(response.body).toEqual({ message: 'MCP server not found' });
|
||||
});
|
||||
|
||||
it('should redact secrets from get response', async () => {
|
||||
mockRegistryInstance.getServerConfig.mockResolvedValue({
|
||||
type: 'sse',
|
||||
url: 'https://mcp-server.example.com/sse',
|
||||
title: 'Secret Server',
|
||||
apiKey: { source: 'admin', authorization_type: 'bearer', key: 'decrypted-admin-key' },
|
||||
oauth: { client_id: 'cid', client_secret: 'decrypted-oauth-secret' },
|
||||
headers: { Authorization: 'Bearer internal-token' },
|
||||
oauth_headers: { 'X-OAuth': 'secret-value' },
|
||||
});
|
||||
|
||||
const response = await request(app).get('/api/mcp/servers/secret-server');
|
||||
|
||||
expect(response.status).toBe(200);
|
||||
expect(response.body.title).toBe('Secret Server');
|
||||
expect(response.body.apiKey?.key).toBeUndefined();
|
||||
expect(response.body.apiKey?.source).toBe('admin');
|
||||
expect(response.body.oauth?.client_secret).toBeUndefined();
|
||||
expect(response.body.oauth?.client_id).toBe('cid');
|
||||
expect(response.body.headers).toBeUndefined();
|
||||
expect(response.body.oauth_headers).toBeUndefined();
|
||||
});
|
||||
|
||||
it('should return 500 when registry throws error', async () => {
|
||||
mockRegistryInstance.getServerConfig.mockRejectedValue(new Error('Database error'));
|
||||
|
||||
|
|
@ -1935,7 +2000,9 @@ describe('MCP Routes', () => {
|
|||
.send({ config: updatedConfig });
|
||||
|
||||
expect(response.status).toBe(200);
|
||||
expect(response.body).toEqual(updatedConfig);
|
||||
expect(response.body.type).toBe('sse');
|
||||
expect(response.body.url).toBe('https://updated-mcp-server.example.com/sse');
|
||||
expect(response.body.title).toBe('Updated Server');
|
||||
expect(mockRegistryInstance.updateServer).toHaveBeenCalledWith(
|
||||
'test-server',
|
||||
expect.objectContaining({
|
||||
|
|
@ -1947,6 +2014,35 @@ describe('MCP Routes', () => {
|
|||
);
|
||||
});
|
||||
|
||||
it('should redact secrets from update response', async () => {
|
||||
const validConfig = {
|
||||
type: 'sse',
|
||||
url: 'https://mcp-server.example.com/sse',
|
||||
title: 'Updated Server',
|
||||
};
|
||||
|
||||
mockRegistryInstance.updateServer.mockResolvedValue({
|
||||
...validConfig,
|
||||
apiKey: { source: 'admin', authorization_type: 'bearer', key: 'preserved-admin-key' },
|
||||
oauth: { client_id: 'cid', client_secret: 'preserved-oauth-secret' },
|
||||
headers: { Authorization: 'Bearer internal-token' },
|
||||
env: { DATABASE_URL: 'postgres://admin:pass@localhost/db' },
|
||||
});
|
||||
|
||||
const response = await request(app)
|
||||
.patch('/api/mcp/servers/test-server')
|
||||
.send({ config: validConfig });
|
||||
|
||||
expect(response.status).toBe(200);
|
||||
expect(response.body.title).toBe('Updated Server');
|
||||
expect(response.body.apiKey?.key).toBeUndefined();
|
||||
expect(response.body.apiKey?.source).toBe('admin');
|
||||
expect(response.body.oauth?.client_secret).toBeUndefined();
|
||||
expect(response.body.oauth?.client_id).toBe('cid');
|
||||
expect(response.body.headers).toBeUndefined();
|
||||
expect(response.body.env).toBeUndefined();
|
||||
});
|
||||
|
||||
it('should return 400 for invalid configuration', async () => {
|
||||
const invalidConfig = {
|
||||
type: 'sse',
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue