diff --git a/api/server/controllers/UserController.js b/api/server/controllers/UserController.js index 9766760b8a..5fd43b66d1 100644 --- a/api/server/controllers/UserController.js +++ b/api/server/controllers/UserController.js @@ -530,9 +530,10 @@ const maybeUninstallOAuthMCP = async (userId, pluginKey, appConfig) => { serverConfig.oauth?.revocation_endpoint_auth_methods_supported ?? clientMetadata.revocation_endpoint_auth_methods_supported; const oauthHeaders = serverConfig.oauth_headers ?? {}; - const registry = getMCPServersRegistry(); - const allowedDomains = registry.getAllowedDomains(); - const allowedAddresses = registry.getAllowedAddresses(); + // Use the request's merged (tenant/principal-scoped) allowlists so admin-panel mcpSettings + // overrides are honored for OAuth revocation, consistent with inspection/connection. + const allowedDomains = appConfig?.mcpSettings?.allowedDomains; + const allowedAddresses = appConfig?.mcpSettings?.allowedAddresses; if (tokens?.access_token) { try { diff --git a/api/server/controllers/__tests__/UserController.mcpOAuth.spec.js b/api/server/controllers/__tests__/UserController.mcpOAuth.spec.js index a831184333..c5457d468c 100644 --- a/api/server/controllers/__tests__/UserController.mcpOAuth.spec.js +++ b/api/server/controllers/__tests__/UserController.mcpOAuth.spec.js @@ -132,7 +132,10 @@ function setupMCPMocks() { getAllowedAddresses: jest.fn().mockReturnValue(null), }; - mockGetAppConfig.mockResolvedValue({}); + // Revocation reads the merged config's mcpSettings allowlists (not the registry getters). + mockGetAppConfig.mockResolvedValue({ + mcpSettings: { allowedDomains: [], allowedAddresses: null }, + }); mockUpdateUserPlugins.mockResolvedValue(); mockDeleteUserPluginAuth.mockResolvedValue(); mockInvalidateCachedTools.mockResolvedValue(); diff --git a/api/server/routes/__tests__/mcp.spec.js b/api/server/routes/__tests__/mcp.spec.js index 823bf783fc..5323fa0d0e 100644 --- a/api/server/routes/__tests__/mcp.spec.js +++ b/api/server/routes/__tests__/mcp.spec.js @@ -24,6 +24,11 @@ const mockRegistryInstance = { removeServer: jest.fn(), getAllowedDomains: jest.fn().mockReturnValue(null), getAllowedAddresses: jest.fn().mockReturnValue(null), + resolveAllowlists: jest.fn().mockResolvedValue({ + allowedDomains: null, + allowedAddresses: null, + useSSRFProtection: true, + }), }; let mockMCPUseAllowed = true; diff --git a/api/server/routes/mcp.js b/api/server/routes/mcp.js index 5e9fad5422..1637b8f7e1 100644 --- a/api/server/routes/mcp.js +++ b/api/server/routes/mcp.js @@ -182,8 +182,10 @@ router.get('/:serverName/oauth/initiate', requireJwtAuth, setOAuthSession, async const configServers = await resolveConfigServers(req); const oauthHeaders = await getOAuthHeaders(serverName, userId, configServers); const registry = getMCPServersRegistry(); - const allowedDomains = registry.getAllowedDomains(); - const allowedAddresses = registry.getAllowedAddresses(); + const { allowedDomains, allowedAddresses } = await registry.resolveAllowlists({ + userId, + role: req.user?.role, + }); const { authorizationUrl, flowId: oauthFlowId, diff --git a/api/server/services/initializeMCPs.js b/api/server/services/initializeMCPs.js index be52b6e6ed..e3b35a6e86 100644 --- a/api/server/services/initializeMCPs.js +++ b/api/server/services/initializeMCPs.js @@ -3,6 +3,21 @@ const { logger } = require('@librechat/data-schemas'); const { mergeAppTools, getAppConfig } = require('./Config'); const { createMCPServersRegistry, createMCPManager } = require('~/config'); +/** + * Resolves the current request's effective MCP allowlists from the merged (tenant-scoped) + * config. The registry calls this per inspection/connection so admin-panel `mcpSettings` + * overrides are honored without a restart. Tenant comes from the ALS context inside + * `getAppConfig`; `userId`/`role` pick up user/role-scoped overrides when an actor exists. + * @param {{ userId?: string, role?: string }} [ctx] + */ +async function resolveMCPAllowlists(ctx) { + const appConfig = await getAppConfig({ role: ctx?.role, userId: ctx?.userId }); + return { + allowedDomains: appConfig?.mcpSettings?.allowedDomains, + allowedAddresses: appConfig?.mcpSettings?.allowedAddresses, + }; +} + /** * Initialize MCP servers */ @@ -15,6 +30,7 @@ async function initializeMCPs() { mongoose, appConfig?.mcpSettings?.allowedDomains, appConfig?.mcpSettings?.allowedAddresses, + resolveMCPAllowlists, ); } catch (error) { logger.error('[MCP] Failed to initialize MCPServersRegistry:', error); diff --git a/api/server/services/initializeMCPs.spec.js b/api/server/services/initializeMCPs.spec.js index c62b85ae1b..fe0766343c 100644 --- a/api/server/services/initializeMCPs.spec.js +++ b/api/server/services/initializeMCPs.spec.js @@ -82,6 +82,7 @@ describe('initializeMCPs', () => { expect.anything(), // mongoose ['localhost'], undefined, + expect.any(Function), // per-request allowlist resolver ); }); @@ -98,6 +99,7 @@ describe('initializeMCPs', () => { expect.anything(), allowedDomains, undefined, + expect.any(Function), ); }); @@ -113,9 +115,34 @@ describe('initializeMCPs', () => { expect.anything(), undefined, undefined, + expect.any(Function), ); }); + it('wires a per-request resolver that reads the merged (non-baseOnly) config', async () => { + mockGetAppConfig.mockResolvedValue({ + mcpConfig: null, + mcpSettings: { allowedDomains: ['yaml.com'] }, + }); + + await initializeMCPs(); + + const resolver = mockCreateMCPServersRegistry.mock.calls[0][3]; + expect(typeof resolver).toBe('function'); + + // The resolver resolves the request's merged allowlists — not the boot YAML base. + mockGetAppConfig.mockResolvedValue({ + mcpSettings: { allowedDomains: ['merged.com'], allowedAddresses: ['10.0.0.0/8'] }, + }); + const resolved = await resolver({ userId: 'u1', role: 'ADMIN' }); + + expect(mockGetAppConfig).toHaveBeenLastCalledWith({ role: 'ADMIN', userId: 'u1' }); + expect(resolved).toEqual({ + allowedDomains: ['merged.com'], + allowedAddresses: ['10.0.0.0/8'], + }); + }); + it('should throw and log error if MCPServersRegistry initialization fails', async () => { const registryError = new Error('Registry initialization failed'); mockCreateMCPServersRegistry.mockImplementation(() => { diff --git a/e2e/config/librechat.e2e.yaml b/e2e/config/librechat.e2e.yaml index bc96e37b4f..daa5154bb4 100644 --- a/e2e/config/librechat.e2e.yaml +++ b/e2e/config/librechat.e2e.yaml @@ -9,6 +9,14 @@ interface: # Mock models price at the default rate, so synthetic usage yields a value. contextCost: true +mcpSettings: + # Deliberately excludes 127.0.0.1, so the URL-based `e2e-http` server below is blocked + # at boot (stored as inspectionFailed). mcp-allowlist-override.spec.ts adds that origin + # via an admin-panel config override and asserts the server reinitializes — proving the + # admin override is honored by inspection/connection. stdio servers skip this check. + allowedDomains: + - https://allowed.example.com + mcpServers: e2e-memory: type: stdio @@ -18,6 +26,12 @@ mcpServers: title: E2E Memory description: Local MCP fixture used by mock end-to-end tests. timeout: 30000 + e2e-http: + type: streamable-http + url: http://127.0.0.1:8765/mcp + title: E2E HTTP + description: Local HTTP MCP fixture for allowlist-override e2e tests. + timeout: 30000 endpoints: custom: diff --git a/e2e/playwright.config.mock.ts b/e2e/playwright.config.mock.ts index d1ef19097d..74aac8dbd0 100644 --- a/e2e/playwright.config.mock.ts +++ b/e2e/playwright.config.mock.ts @@ -5,6 +5,9 @@ import { getLocalE2EEnv, getE2EBaseURL } from './setup/env'; const rootPath = path.resolve(__dirname, '..'); const serverPath = path.resolve(rootPath, 'e2e/setup/start-server.js'); +const mcpHttpServerPath = path.resolve(rootPath, 'e2e/setup/fake-mcp-http-server.js'); +/** Must match the `e2e-http` server URL in e2e/config/librechat.e2e.yaml. */ +const MCP_HTTP_PORT = process.env.E2E_MCP_HTTP_PORT || '8765'; const fakeModelHookPath = path.resolve(rootPath, 'e2e/setup/fake-model.js'); const configTemplatePath = path.resolve(rootPath, 'e2e/config/librechat.e2e.yaml'); const configPath = path.resolve(rootPath, 'e2e/.generated/librechat.e2e.yaml'); @@ -133,5 +136,15 @@ export default defineConfig({ timeout: 120_000, reuseExistingServer: false, }, + { + // URL-based MCP fixture for the allowlist-override spec (its health route is GET /). + command: `node ${mcpHttpServerPath}`, + cwd: rootPath, + env: { ...process.env, E2E_MCP_HTTP_PORT: MCP_HTTP_PORT }, + url: `http://127.0.0.1:${MCP_HTTP_PORT}/`, + stdout: 'pipe', + timeout: 60_000, + reuseExistingServer: false, + }, ], }); diff --git a/e2e/setup/fake-mcp-http-server.js b/e2e/setup/fake-mcp-http-server.js new file mode 100644 index 0000000000..503e82d9e2 --- /dev/null +++ b/e2e/setup/fake-mcp-http-server.js @@ -0,0 +1,78 @@ +#!/usr/bin/env node + +/** + * Streamable-HTTP MCP fixture for the allowlist-override e2e spec. + * + * Unlike the stdio `fake-mcp-server.js`, this one is reachable over a URL so it + * exercises the `mcpSettings.allowedDomains` check (stdio transports skip it). The + * e2e config deliberately omits this server's origin from `allowedDomains`, so it + * boots as `inspectionFailed`; `mcp-allowlist-override.spec.ts` then adds the origin + * via an admin-panel config override and asserts the server reinitializes. + * + * Mirrors the stateful streamable-HTTP pattern in + * packages/api/src/mcp/__tests__/helpers/oauthTestServer.ts (without OAuth). + */ + +const http = require('http'); +const { randomUUID } = require('crypto'); +const { McpServer } = require('@modelcontextprotocol/sdk/server/mcp.js'); +const { + StreamableHTTPServerTransport, +} = require('@modelcontextprotocol/sdk/server/streamableHttp.js'); + +const PORT = parseInt(process.env.E2E_MCP_HTTP_PORT || '8765', 10); +const HOST = '127.0.0.1'; + +function createMcpServer() { + const server = new McpServer({ name: 'e2e-http', version: '1.0.0' }); + server.registerTool( + 'http_ping', + { + description: 'Returns a deterministic value for LibreChat allowlist-override e2e tests.', + inputSchema: {}, + }, + async () => ({ + content: [{ type: 'text', text: 'E2E HTTP MCP pong' }], + }), + ); + return server; +} + +/** @type {Map>} */ +const sessions = new Map(); + +const httpServer = http.createServer(async (req, res) => { + const url = new URL(req.url, `http://${req.headers.host}`); + + // Health endpoint so Playwright's `webServer` can detect readiness. + if (url.pathname === '/' && req.method === 'GET') { + res.writeHead(200, { 'Content-Type': 'text/plain' }); + res.end('ok'); + return; + } + + if (url.pathname !== '/mcp') { + res.writeHead(404); + res.end(); + return; + } + + const sid = req.headers['mcp-session-id']; + let transport = typeof sid === 'string' ? sessions.get(sid) : undefined; + if (!transport) { + transport = new StreamableHTTPServerTransport({ sessionIdGenerator: () => randomUUID() }); + const mcp = createMcpServer(); + await mcp.connect(transport); + } + + await transport.handleRequest(req, res); + + if (transport.sessionId && !sessions.has(transport.sessionId)) { + sessions.set(transport.sessionId, transport); + transport.onclose = () => sessions.delete(transport.sessionId); + } +}); + +httpServer.listen(PORT, HOST, () => { + console.log(`[e2e] fake HTTP MCP server listening on http://${HOST}:${PORT}/mcp`); +}); diff --git a/e2e/specs/mock/mcp-allowlist-override.spec.ts b/e2e/specs/mock/mcp-allowlist-override.spec.ts new file mode 100644 index 0000000000..281c1aebd0 --- /dev/null +++ b/e2e/specs/mock/mcp-allowlist-override.spec.ts @@ -0,0 +1,77 @@ +import { expect, test } from '@playwright/test'; +import type { APIRequestContext } from '@playwright/test'; +import { getPrimaryE2EUser } from '../../setup/users.mock'; + +/** + * Proves the #13809 fix end to end: an admin-panel `mcpSettings.allowedDomains` + * override is honored by MCP inspection/connection without a restart. + * + * `e2e-http` (a URL-based MCP fixture) boots `inspectionFailed` because its origin + * is absent from the YAML allowlist. Adding that origin via an admin config override + * must let the server reinitialize. Before the fix, reinspection used the frozen + * YAML allowlist and the server stayed unreachable. + * + * Pure-API e2e against the real backend + DB: the JWT comes from the Authorization + * header (`ExtractJwt.fromAuthHeaderAsBearerToken`), so we log in for a token rather + * than relying on the browser storage state. + */ + +const SERVER_NAME = 'e2e-http'; +/** Must match the `e2e-http` URL origin in e2e/config/librechat.e2e.yaml. */ +const FIXTURE_ORIGIN = `http://127.0.0.1:${process.env.E2E_MCP_HTTP_PORT || '8765'}`; + +async function reinitialize( + request: APIRequestContext, + headers: Record, +): Promise<{ status: number; success: boolean }> { + const res = await request.post(`/api/mcp/${SERVER_NAME}/reinitialize`, { headers }); + if (!res.ok()) { + return { status: res.status(), success: false }; + } + const body = (await res.json()) as { success?: boolean }; + return { status: res.status(), success: body.success === true }; +} + +test.describe('MCP admin-panel allowlist override', () => { + test('honors an admin mcpSettings.allowedDomains override so a blocked server reinitializes', async ({ + request, + }) => { + test.setTimeout(120000); + + // The seeded primary user is the first-registered user → ADMIN, so it can write + // config overrides. Log in for a Bearer token + the user id. + const { email, password } = getPrimaryE2EUser(); + const loginRes = await request.post('/api/auth/login', { data: { email, password } }); + expect(loginRes.ok()).toBeTruthy(); + const { token, user } = (await loginRes.json()) as { + token: string; + user: { id?: string; _id?: string }; + }; + const userId = user.id ?? user._id; + expect(token).toBeTruthy(); + expect(userId).toBeTruthy(); + + const headers = { Authorization: `Bearer ${token}` }; + + // Baseline: the fixture's origin is not in the YAML allowlist, so reinit fails. + const before = await reinitialize(request, headers); + expect(before.status).toBe(200); + expect(before.success).toBe(false); + + // Admin-panel override: allow the fixture's origin for this user. + const put = await request.put(`/api/admin/config/user/${userId}`, { + headers, + data: { overrides: { mcpSettings: { allowedDomains: [FIXTURE_ORIGIN] } } }, + }); + expect(put.ok()).toBeTruthy(); + + // The override is honored on reinit: the server now connects. invalidateConfigCaches + // runs asynchronously after the PUT, so poll until the merged allowlist lands. + await expect + .poll(async () => (await reinitialize(request, headers)).success, { + timeout: 30000, + intervals: [1000, 2000, 3000], + }) + .toBe(true); + }); +}); diff --git a/packages/api/src/mcp/ConnectionsRepository.ts b/packages/api/src/mcp/ConnectionsRepository.ts index a6e3e07a67..7dd2695467 100644 --- a/packages/api/src/mcp/ConnectionsRepository.ts +++ b/packages/api/src/mcp/ConnectionsRepository.ts @@ -1,8 +1,8 @@ import { logger } from '@librechat/data-schemas'; import type * as t from './types'; import { MCPServersRegistry } from '~/mcp/registry/MCPServersRegistry'; -import { MCPConnectionFactory } from '~/mcp/MCPConnectionFactory'; import { isUserSourced, requiresUserScopedConnection } from './utils'; +import { MCPConnectionFactory } from '~/mcp/MCPConnectionFactory'; import { MCPConnection } from './connection'; const CONNECT_CONCURRENCY = 3; @@ -78,14 +78,16 @@ export class ConnectionsRepository { } } const registry = MCPServersRegistry.getInstance(); + const { allowedDomains, allowedAddresses, useSSRFProtection } = + await registry.resolveAllowlists({ userId: this.ownerId }); const connection = await MCPConnectionFactory.create( { serverName, serverConfig, dbSourced: isUserSourced(serverConfig as t.ParsedServerConfig), - useSSRFProtection: registry.shouldEnableSSRFProtection(), - allowedDomains: registry.getAllowedDomains(), - allowedAddresses: registry.getAllowedAddresses(), + useSSRFProtection, + allowedDomains, + allowedAddresses, }, this.oauthOpts, ); diff --git a/packages/api/src/mcp/MCPManager.ts b/packages/api/src/mcp/MCPManager.ts index 6b26bc709a..0269937e5b 100644 --- a/packages/api/src/mcp/MCPManager.ts +++ b/packages/api/src/mcp/MCPManager.ts @@ -157,9 +157,8 @@ export class MCPManager extends UserConnectionManager { } const registry = MCPServersRegistry.getInstance(); - const useSSRFProtection = registry.shouldEnableSSRFProtection(); - const allowedDomains = registry.getAllowedDomains(); - const allowedAddresses = registry.getAllowedAddresses(); + const { allowedDomains, allowedAddresses, useSSRFProtection } = + await registry.resolveAllowlists({ userId: user?.id, role: user?.role }); await this.assertResolvedRuntimeConfigAllowed({ config: serverConfig, user, @@ -485,15 +484,17 @@ Please follow these instructions when using tools from the respective MCP server resolvedHeaders['Authorization'] = `Bearer ${oboTokens.access_token}`; } if (userId && user && oauthStart && flowManager && isOAuthServer(currentOptions)) { + const { allowedDomains, allowedAddresses, useSSRFProtection } = + await registry.resolveAllowlists({ userId, role: user?.role }); cleanupRequestOAuthHandler = MCPConnectionFactory.attachRequestOAuthHandler( { serverName, serverConfig: currentOptions, dbSourced: isDbSourced, skipEnvProcessing: true, - useSSRFProtection: registry.shouldEnableSSRFProtection(), - allowedDomains: registry.getAllowedDomains(), - allowedAddresses: registry.getAllowedAddresses(), + useSSRFProtection, + allowedDomains, + allowedAddresses, }, { useOAuth: true, diff --git a/packages/api/src/mcp/UserConnectionManager.ts b/packages/api/src/mcp/UserConnectionManager.ts index e12720dc01..c083d79979 100644 --- a/packages/api/src/mcp/UserConnectionManager.ts +++ b/packages/api/src/mcp/UserConnectionManager.ts @@ -453,8 +453,8 @@ export abstract class UserConnectionManager { graphTokenResolver, }); const registry = MCPServersRegistry.getInstance(); - const allowedDomains = registry.getAllowedDomains(); - const allowedAddresses = registry.getAllowedAddresses(); + const { allowedDomains, allowedAddresses, useSSRFProtection } = + await registry.resolveAllowlists({ userId: user?.id, role: user?.role }); await this.assertResolvedRuntimeConfigAllowed({ config: runtimeConfig, user, @@ -469,7 +469,7 @@ export abstract class UserConnectionManager { serverConfig: runtimeConfig, serverName: serverName, dbSourced: isUserSourced(runtimeConfig), - useSSRFProtection: registry.shouldEnableSSRFProtection(), + useSSRFProtection, allowedDomains, allowedAddresses, ephemeralConnection, @@ -665,8 +665,10 @@ export abstract class UserConnectionManager { } const registry = MCPServersRegistry.getInstance(); - const allowedDomains = registry.getAllowedDomains(); - const allowedAddresses = registry.getAllowedAddresses(); + const { allowedDomains, allowedAddresses } = await registry.resolveAllowlists({ + userId: user?.id, + role: user?.role, + }); const allowed = await isMCPDomainAllowed(resolvedConfig, allowedDomains, allowedAddresses); if (!allowed) { throw new McpError( diff --git a/packages/api/src/mcp/__tests__/ConnectionsRepository.test.ts b/packages/api/src/mcp/__tests__/ConnectionsRepository.test.ts index bd52537778..73b88587de 100644 --- a/packages/api/src/mcp/__tests__/ConnectionsRepository.test.ts +++ b/packages/api/src/mcp/__tests__/ConnectionsRepository.test.ts @@ -21,12 +21,20 @@ jest.mock('../MCPConnectionFactory', () => ({ jest.mock('../connection'); // Mock the registry +const mockShouldEnableSSRFProtection = jest.fn().mockReturnValue(false); +const mockGetAllowedDomains = jest.fn().mockReturnValue(null); +const mockGetAllowedAddresses = jest.fn().mockReturnValue(null); const mockRegistryInstance = { getServerConfig: jest.fn(), getAllServerConfigs: jest.fn(), - shouldEnableSSRFProtection: jest.fn().mockReturnValue(false), - getAllowedDomains: jest.fn().mockReturnValue(null), - getAllowedAddresses: jest.fn().mockReturnValue(null), + shouldEnableSSRFProtection: mockShouldEnableSSRFProtection, + getAllowedDomains: mockGetAllowedDomains, + getAllowedAddresses: mockGetAllowedAddresses, + resolveAllowlists: jest.fn(async () => ({ + allowedDomains: mockGetAllowedDomains(), + allowedAddresses: mockGetAllowedAddresses(), + useSSRFProtection: mockShouldEnableSSRFProtection(), + })), }; jest.mock('../registry/MCPServersRegistry', () => ({ diff --git a/packages/api/src/mcp/__tests__/MCPManager.test.ts b/packages/api/src/mcp/__tests__/MCPManager.test.ts index 0a0b519f24..865de69eb4 100644 --- a/packages/api/src/mcp/__tests__/MCPManager.test.ts +++ b/packages/api/src/mcp/__tests__/MCPManager.test.ts @@ -42,13 +42,23 @@ jest.mock('~/auth/domain', () => ({ isMCPDomainAllowed: jest.fn().mockResolvedValue(true), })); +const mockShouldEnableSSRFProtection = jest.fn().mockReturnValue(false); +const mockGetAllowedDomains = jest.fn().mockReturnValue(null); +const mockGetAllowedAddresses = jest.fn().mockReturnValue(null); const mockRegistryInstance = { getServerConfig: jest.fn(), getAllServerConfigs: jest.fn(), getOAuthServers: jest.fn(), - shouldEnableSSRFProtection: jest.fn().mockReturnValue(false), - getAllowedDomains: jest.fn().mockReturnValue(null), - getAllowedAddresses: jest.fn().mockReturnValue(null), + shouldEnableSSRFProtection: mockShouldEnableSSRFProtection, + getAllowedDomains: mockGetAllowedDomains, + getAllowedAddresses: mockGetAllowedAddresses, + // Mirrors the real per-request resolver by reading the base-allowlist mocks above, so + // existing tests that override getAllowedDomains/shouldEnableSSRFProtection still apply. + resolveAllowlists: jest.fn(async () => ({ + allowedDomains: mockGetAllowedDomains(), + allowedAddresses: mockGetAllowedAddresses(), + useSSRFProtection: mockShouldEnableSSRFProtection(), + })), }; jest.mock('~/mcp/registry/MCPServersRegistry', () => ({ diff --git a/packages/api/src/mcp/__tests__/MCPOAuthRaceCondition.test.ts b/packages/api/src/mcp/__tests__/MCPOAuthRaceCondition.test.ts index adf97fe980..fbb682c3cf 100644 --- a/packages/api/src/mcp/__tests__/MCPOAuthRaceCondition.test.ts +++ b/packages/api/src/mcp/__tests__/MCPOAuthRaceCondition.test.ts @@ -93,6 +93,11 @@ describe('MCP OAuth Race Condition Fixes', () => { shouldEnableSSRFProtection: jest.fn().mockReturnValue(false), getAllowedDomains: jest.fn().mockReturnValue(null), getAllowedAddresses: jest.fn().mockReturnValue(null), + resolveAllowlists: jest.fn().mockResolvedValue({ + allowedDomains: null, + allowedAddresses: null, + useSSRFProtection: false, + }), }); const { MCPConnectionFactory } = await import('~/mcp/MCPConnectionFactory'); @@ -165,6 +170,11 @@ describe('MCP OAuth Race Condition Fixes', () => { shouldEnableSSRFProtection: jest.fn().mockReturnValue(false), getAllowedDomains: jest.fn().mockReturnValue(null), getAllowedAddresses: jest.fn().mockReturnValue(null), + resolveAllowlists: jest.fn().mockResolvedValue({ + allowedDomains: null, + allowedAddresses: null, + useSSRFProtection: false, + }), }); const { MCPConnectionFactory } = await import('~/mcp/MCPConnectionFactory'); @@ -246,6 +256,11 @@ describe('MCP OAuth Race Condition Fixes', () => { shouldEnableSSRFProtection: jest.fn().mockReturnValue(false), getAllowedDomains: jest.fn().mockReturnValue(null), getAllowedAddresses: jest.fn().mockReturnValue(null), + resolveAllowlists: jest.fn().mockResolvedValue({ + allowedDomains: null, + allowedAddresses: null, + useSSRFProtection: false, + }), }); let releaseConnection: () => void = () => undefined; @@ -346,6 +361,11 @@ describe('MCP OAuth Race Condition Fixes', () => { shouldEnableSSRFProtection: jest.fn().mockReturnValue(false), getAllowedDomains: jest.fn().mockReturnValue(null), getAllowedAddresses: jest.fn().mockReturnValue(null), + resolveAllowlists: jest.fn().mockResolvedValue({ + allowedDomains: null, + allowedAddresses: null, + useSSRFProtection: false, + }), }); const { MCPConnectionFactory } = await import('~/mcp/MCPConnectionFactory'); diff --git a/packages/api/src/mcp/registry/MCPServersRegistry.ts b/packages/api/src/mcp/registry/MCPServersRegistry.ts index 06e583608a..fa0bdb4e13 100644 --- a/packages/api/src/mcp/registry/MCPServersRegistry.ts +++ b/packages/api/src/mcp/registry/MCPServersRegistry.ts @@ -82,6 +82,28 @@ const CONFIG_SERVER_INIT_TIMEOUT_MS = (() => { return Number.isFinite(parsed) && parsed > 0 ? parsed : 30_000; })(); +/** Request context for resolving the effective MCP allowlists. */ +export interface MCPAllowlistContext { + userId?: string; + role?: string; +} + +/** + * Resolves the effective `mcpSettings` allowlists for a request. Injected from the app + * layer (where the merged, tenant-scoped config lives) so the registry keeps no app-config + * dependency. Reads the ALS tenant context internally; pass the acting user to also pick up + * user/role-scoped overrides. + */ +export type MCPAllowlistResolver = ( + ctx?: MCPAllowlistContext, +) => Promise<{ allowedDomains?: string[] | null; allowedAddresses?: string[] | null }>; + +/** Effective allowlists resolved for a request. */ +interface ResolvedMCPAllowlists { + allowedDomains?: string[] | null; + allowedAddresses?: string[] | null; +} + /** * Central registry for managing MCP server configurations. * Authoritative source of truth for all MCP servers provided by LibreChat. @@ -99,8 +121,11 @@ export class MCPServersRegistry { private readonly dbConfigsRepo: ServerConfigsDB; private readonly cacheConfigsRepo: IServerConfigsRepositoryInterface; private readonly configCacheRepo: IServerConfigsRepositoryInterface; + /** YAML-derived base allowlists; used at boot and as the fallback when no resolver is set. */ private readonly allowedDomains?: string[] | null; private readonly allowedAddresses?: string[] | null; + /** Resolves the per-request (tenant-scoped) merged allowlists; falls back to the base above. */ + private readonly allowlistResolver?: MCPAllowlistResolver; private readonly readThroughCache: Keyv; private readonly readThroughCacheAll: Keyv>; private readonly pendingGetAllPromises = new Map< @@ -122,12 +147,14 @@ export class MCPServersRegistry { mongoose: typeof import('mongoose'), allowedDomains?: string[] | null, allowedAddresses?: string[] | null, + allowlistResolver?: MCPAllowlistResolver, ) { this.dbConfigsRepo = new ServerConfigsDB(mongoose); this.cacheConfigsRepo = ServerConfigsCacheFactory.create(APP_CACHE_NAMESPACE, false); this.configCacheRepo = ServerConfigsCacheFactory.create(CONFIG_CACHE_NAMESPACE, false); this.allowedDomains = allowedDomains; this.allowedAddresses = allowedAddresses; + this.allowlistResolver = allowlistResolver; const ttl = cacheConfig.MCP_REGISTRY_CACHE_TTL; @@ -147,6 +174,7 @@ export class MCPServersRegistry { mongoose: typeof import('mongoose'), allowedDomains?: string[] | null, allowedAddresses?: string[] | null, + allowlistResolver?: MCPAllowlistResolver, ): MCPServersRegistry { if (!mongoose) { throw new Error( @@ -163,6 +191,7 @@ export class MCPServersRegistry { mongoose, allowedDomains, allowedAddresses, + allowlistResolver, ); return MCPServersRegistry.instance; } @@ -175,10 +204,12 @@ export class MCPServersRegistry { return MCPServersRegistry.instance; } + /** YAML base allowlist (boot/fallback). For request-time decisions use {@link resolveAllowlists}. */ public getAllowedDomains(): string[] | null | undefined { return this.allowedDomains; } + /** YAML base allowlist (boot/fallback). For request-time decisions use {@link resolveAllowlists}. */ public getAllowedAddresses(): string[] | null | undefined { return this.allowedAddresses; } @@ -188,6 +219,43 @@ export class MCPServersRegistry { return !Array.isArray(this.allowedDomains) || this.allowedDomains.length === 0; } + /** + * Resolves the effective domain/address allowlists for the current request. + * + * MCP allowlists live in `mcpSettings`, which is tenant/principal-scoped admin config, so + * they must be read per-request from the merged config — not from a process-global value + * that would leak across tenants. The injected resolver reads the ALS tenant context; pass + * the acting user so user/role-scoped overrides resolve too (config-source inspection has no + * user and resolves at tenant scope). Falls back to the YAML base allowlists when no resolver + * is injected or the resolver fails, so a transient lookup error fails to the operator's + * baseline rather than disabling the allowlist. + */ + public async resolveAllowlists(ctx?: MCPAllowlistContext): Promise<{ + allowedDomains?: string[] | null; + allowedAddresses?: string[] | null; + useSSRFProtection: boolean; + }> { + let allowedDomains = this.allowedDomains; + let allowedAddresses = this.allowedAddresses; + if (this.allowlistResolver) { + try { + const resolved = await this.allowlistResolver(ctx); + allowedDomains = resolved.allowedDomains; + allowedAddresses = resolved.allowedAddresses; + } catch (error) { + logger.warn( + '[MCPServersRegistry] Allowlist resolver failed; falling back to YAML base allowlists', + error, + ); + } + } + return { + allowedDomains, + allowedAddresses, + useSSRFProtection: !Array.isArray(allowedDomains) || allowedDomains.length === 0, + }; + } + /** * Returns the config for a single server, mirroring the precedence used by * getAllServerConfigs so list views and single-server lookups agree on @@ -341,14 +409,15 @@ export class MCPServersRegistry { const configRepo = this.getConfigRepository(storageLocation); const source = (storageLocation === 'CACHE' ? 'yaml' : 'user') as t.MCPServerSource; const configForInspection = { ...config, source } as t.ParsedServerConfig; + const { allowedDomains, allowedAddresses } = await this.resolveAllowlists({ userId }); let parsedConfig: t.ParsedServerConfig; try { parsedConfig = await MCPServerInspector.inspect( serverName, configForInspection, undefined, - this.allowedDomains, - this.allowedAddresses, + allowedDomains, + allowedAddresses, ); } catch (error) { logger.error(`[MCPServersRegistry] Failed to inspect server "${serverName}":`, error); @@ -398,14 +467,15 @@ export class MCPServersRegistry { } const { inspectionFailed: _, ...configForInspection } = existing; + const { allowedDomains, allowedAddresses } = await this.resolveAllowlists({ userId }); let parsedConfig: t.ParsedServerConfig; try { parsedConfig = await MCPServerInspector.inspect( serverName, configForInspection, undefined, - this.allowedDomains, - this.allowedAddresses, + allowedDomains, + allowedAddresses, ); } catch (error) { logger.error(`[MCPServersRegistry] Reinspection failed for server "${serverName}":`, error); @@ -445,14 +515,15 @@ export class MCPServersRegistry { } } + const { allowedDomains, allowedAddresses } = await this.resolveAllowlists({ userId }); let parsedConfig: t.ParsedServerConfig; try { parsedConfig = await MCPServerInspector.inspect( serverName, { ...configForInspection, source } as t.ParsedServerConfig, undefined, - this.allowedDomains, - this.allowedAddresses, + allowedDomains, + allowedAddresses, ); } catch (error) { logger.error(`[MCPServersRegistry] Failed to inspect server "${serverName}":`, error); @@ -483,6 +554,12 @@ export class MCPServersRegistry { const result: Record = {}; + // Config-source servers are admin-defined with no acting user; resolve the effective + // allowlists once at tenant scope and fold them into each config-cache key so a tenant + // whose allowlist rejects a URL cannot poison the shared key for a tenant that allows it. + const { allowedDomains, allowedAddresses } = await this.resolveAllowlists(); + const allowlists: ResolvedMCPAllowlists = { allowedDomains, allowedAddresses }; + /** Single snapshot of the YAML cache for the whole pass: in the Redis aggregate-key backend, every per-name get() reads and deserializes the full map, so N concurrent per-server lookups would issue N full-map reads. The snapshot also keeps the unchanged-YAML comparison consistent against one view of YAML across all entries. */ const yamlSnapshot = await this.cacheConfigsRepo.getAll(); @@ -491,7 +568,7 @@ export class MCPServersRegistry { if (this.isUnmodifiedYamlServer(yamlSnapshot, serverName, rawConfig)) { return; } - const parsed = await this.ensureSingleConfigServer(serverName, rawConfig); + const parsed = await this.ensureSingleConfigServer(serverName, rawConfig, allowlists); if (parsed) { result[serverName] = parsed; } @@ -540,8 +617,9 @@ export class MCPServersRegistry { private async ensureSingleConfigServer( serverName: string, rawConfig: t.MCPOptions, + allowlists: ResolvedMCPAllowlists, ): Promise { - const cacheKey = this.configCacheKey(serverName, rawConfig); + const cacheKey = this.configCacheKey(serverName, rawConfig, allowlists); const cached = await this.configCacheRepo.get(cacheKey); if (cached) { @@ -558,7 +636,7 @@ export class MCPServersRegistry { return pending; } - const initPromise = this.lazyInitConfigServer(cacheKey, serverName, rawConfig); + const initPromise = this.lazyInitConfigServer(cacheKey, serverName, rawConfig, allowlists); this.pendingConfigInits.set(cacheKey, initPromise); try { @@ -576,6 +654,7 @@ export class MCPServersRegistry { cacheKey: string, serverName: string, rawConfig: t.MCPOptions, + allowlists: ResolvedMCPAllowlists, ): Promise { const prefix = `[MCP][config][${serverName}]`; logger.info(`${prefix} Lazy-initializing config-source server`); @@ -585,13 +664,14 @@ export class MCPServersRegistry { ...rawConfig, source: 'config' as const, } as t.ParsedServerConfig; + const { allowedDomains, allowedAddresses } = allowlists; const inspected = await withTimeout( MCPServerInspector.inspect( serverName, configForInspection, undefined, - this.allowedDomains, - this.allowedAddresses, + allowedDomains, + allowedAddresses, ), CONFIG_SERVER_INIT_TIMEOUT_MS, `${prefix} Server initialization timed out`, @@ -787,12 +867,24 @@ export class MCPServersRegistry { } /** - * Produces a config-cache key scoped by server name AND a hash of the raw config. - * Prevents cross-tenant cache poisoning when two tenants define the same server name - * with different configurations. + * Produces a config-cache key scoped by server name AND a hash of the raw config plus the + * effective allowlists. Hashing the raw config prevents cross-tenant poisoning when two + * tenants define the same server name with different configurations; hashing the allowlists + * prevents poisoning when the same config resolves differently because tenants have different + * `mcpSettings.allowedDomains` / `allowedAddresses` (so one tenant's inspection result — e.g. + * an `inspectionFailed` stub from a rejected domain — never satisfies another tenant's lookup). */ - private configCacheKey(serverName: string, rawConfig: t.MCPOptions): string { - const sorted = JSON.stringify(rawConfig, (_key, value: unknown) => { + private configCacheKey( + serverName: string, + rawConfig: t.MCPOptions, + allowlists?: ResolvedMCPAllowlists, + ): string { + const payload = { + rawConfig, + allowedDomains: allowlists?.allowedDomains ?? null, + allowedAddresses: allowlists?.allowedAddresses ?? null, + }; + const sorted = JSON.stringify(payload, (_key, value: unknown) => { if (value !== null && typeof value === 'object' && !Array.isArray(value)) { return Object.fromEntries( Object.entries(value as Record).sort(([a], [b]) => a.localeCompare(b)), diff --git a/packages/api/src/mcp/registry/__tests__/MCPServersRegistry.test.ts b/packages/api/src/mcp/registry/__tests__/MCPServersRegistry.test.ts index 706d34ed2a..cf0b18a739 100644 --- a/packages/api/src/mcp/registry/__tests__/MCPServersRegistry.test.ts +++ b/packages/api/src/mcp/registry/__tests__/MCPServersRegistry.test.ts @@ -220,6 +220,114 @@ describe('MCPServersRegistry', () => { }); }); + describe('resolveAllowlists (per-request, tenant-scoped)', () => { + const createWith = ( + allowedDomains?: string[] | null, + allowedAddresses?: string[] | null, + resolver?: (ctx?: { userId?: string; role?: string }) => Promise<{ + allowedDomains?: string[] | null; + allowedAddresses?: string[] | null; + }>, + ): MCPServersRegistry => { + (MCPServersRegistry as unknown as { instance: undefined }).instance = undefined; + MCPServersRegistry.createInstance(mockMongoose, allowedDomains, allowedAddresses, resolver); + return MCPServersRegistry.getInstance(); + }; + + it('returns the YAML base allowlists when no resolver is injected', async () => { + const reg = createWith(['yaml.com'], ['10.0.0.0/8']); + await expect(reg.resolveAllowlists()).resolves.toEqual({ + allowedDomains: ['yaml.com'], + allowedAddresses: ['10.0.0.0/8'], + useSSRFProtection: false, + }); + }); + + it('enables SSRF protection when the effective allowlist is empty', async () => { + const reg = createWith(undefined, undefined); + await expect(reg.resolveAllowlists()).resolves.toEqual({ + allowedDomains: undefined, + allowedAddresses: undefined, + useSSRFProtection: true, + }); + }); + + it('returns the resolver-provided merged allowlists and forwards the context', async () => { + const resolver = jest.fn().mockResolvedValue({ + allowedDomains: ['admin-added.com'], + allowedAddresses: ['172.16.0.0/12'], + }); + const reg = createWith(['yaml.com'], null, resolver); + + const result = await reg.resolveAllowlists({ userId: 'u1', role: 'ADMIN' }); + + expect(resolver).toHaveBeenCalledWith({ userId: 'u1', role: 'ADMIN' }); + expect(result).toEqual({ + allowedDomains: ['admin-added.com'], + allowedAddresses: ['172.16.0.0/12'], + useSSRFProtection: false, + }); + }); + + it('falls back to the YAML base allowlists when the resolver throws', async () => { + const resolver = jest.fn().mockRejectedValue(new Error('DB down')); + const reg = createWith(['yaml.com'], null, resolver); + + await expect(reg.resolveAllowlists()).resolves.toEqual({ + allowedDomains: ['yaml.com'], + allowedAddresses: null, + useSSRFProtection: false, + }); + }); + + it('inspects against the resolved (admin-panel) allowlist, not the YAML base', async () => { + const resolver = jest.fn().mockResolvedValue({ + allowedDomains: ['admin-added.com'], + allowedAddresses: ['10.0.0.0/8'], + }); + const reg = createWith(['yaml-only.com'], null, resolver); + const inspectSpy = jest.spyOn(MCPServerInspector, 'inspect'); + await reg.reset(); + + await reg.addServer( + 'admin_panel_server', + { type: 'streamable-http', url: 'https://admin-added.com/mcp' }, + 'DB', + 'user-1', + ); + + expect(resolver).toHaveBeenCalledWith({ userId: 'user-1' }); + expect(inspectSpy).toHaveBeenCalledWith( + 'admin_panel_server', + expect.objectContaining({ url: 'https://admin-added.com/mcp' }), + undefined, + ['admin-added.com'], + ['10.0.0.0/8'], + ); + }); + + it('scopes the config-source cache key by the resolved allowlist (no cross-tenant poison)', async () => { + const resolver = jest + .fn() + .mockResolvedValueOnce({ allowedDomains: ['a.com'], allowedAddresses: null }) + .mockResolvedValueOnce({ allowedDomains: ['b.com'], allowedAddresses: null }); + const reg = createWith(null, null, resolver); + const inspectSpy = jest.spyOn(MCPServerInspector, 'inspect'); + await reg.reset(); + inspectSpy.mockClear(); + + const cfg = { + srv: { type: 'streamable-http' as const, url: 'https://srv.example.com/mcp' }, + }; + await reg.ensureConfigServers(cfg); // resolver call 1 → allowlist A + await reg.ensureConfigServers(cfg); // resolver call 2 → allowlist B (distinct key) + + // Different resolved allowlists ⇒ different cache keys ⇒ the second pass re-inspects + // instead of reusing the first allowlist's cached entry. + expect(inspectSpy).toHaveBeenCalledTimes(2); + }); + }); + describe('reset', () => { it('should clear all servers from cache repository', async () => { // Add servers to cache using the new API