mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-06-28 18:31:24 +00:00
🔐 fix: Honor Admin-Panel MCP Allowlist Overrides Without Restart (#13814)
* 🔐 fix: Honor Admin-Panel MCP Allowlist Overrides Without Restart MCPServersRegistry was built once at boot from getAppConfig({ baseOnly: true }), freezing allowedDomains/allowedAddresses to YAML. Admin-panel mcpSettings overrides were ignored by both inspection (addServer/ reinspectServer/updateServer/lazyInitConfigServer) and runtime connection enforcement (assertResolvedRuntimeConfigAllowed), so a domain allowed only via the panel failed inspection and never connected. Make the registry's effective allowlists mutable and refresh them from the merged admin-panel config: seed at boot, and re-apply on every config mutation via invalidateConfigCaches -> clearMcpConfigCache. Both inspection and connection paths read the same getters, so both honor overrides without a restart. Fail-safe: current allowlists are preserved when the merged read fails. * 🛡️ fix: Scope MCP allowlist refresh to global config, fail-safe on DB error Address Codex P1 review findings on the allowlist-refresh path: - Tenant-scoped config mutations no longer push one tenant's merged mcpSettings into the process-wide registry singleton (read by all MCP connection paths), which would leak allowlists across tenants. Only global (non-tenant) mutations refresh the registry; tenant mutations still evict the config-server cache. - The refresh read now uses strictOverrides:true so a transient DB error throws instead of silently returning YAML base config — preserving the last-known allowlists rather than overwriting them with fallback values. Adds the strictOverrides option to getAppConfig (default off, no behavior change for existing callers). * ♻️ refactor: Resolve MCP allowlists per-request (tenant-scoped) instead of a global singleton Supersedes the prior global-mutation approach. MCP allowlists live in mcpSettings, which is tenant/principal-scoped admin config, so a process-wide singleton value is the wrong model — it caused cross-tenant bleed and stale reads. Instead, inject a resolver (from the app layer, where the merged config lives) that the registry calls per inspection and per connection. It reads the ALS tenant context via getAppConfig and accepts the acting user so user/role-scoped overrides resolve; config-source inspection (no user) resolves at tenant scope. Falls back to the YAML base allowlists when no resolver is set or the lookup fails, so a transient error fails to the operator baseline rather than disabling the allowlist. Removes the now-unnecessary setAllowlists / boot-seed / invalidateConfigCaches refresh / getAppConfig.strictOverrides machinery. * 🔒 fix: Scope config-source cache by allowlist; resolve OAuth allowlists per-request Address Codex review of the per-request resolver: - Config-source cache key now folds in the resolved allowlists, not just the raw-config hash. Inspection results became allowlist-dependent, so without this a tenant whose allowlist rejects a URL could poison the shared key with an inspectionFailed stub for a tenant that allows it (and vice versa). The tenant-scoped allowlist is resolved once per ensureConfigServers pass and threaded through the cache key + inspection. - The two remaining request-time OAuth allowlist reads now use the merged config instead of the YAML base getters: the fallback OAuth-initiate path (routes/mcp.js) via resolveAllowlists, and OAuth revocation (UserController.maybeUninstallOAuthMCP) via the request's already-merged appConfig.mcpSettings. Without this, an OAuth endpoint allowed only by an admin-panel override was rejected while inspection/connection allowed it. * ✅ test: Update MCP OAuth registry/config mocks for per-request allowlists CI fix for the Finding-12 change. The OAuth-initiate route now calls registry.resolveAllowlists() and the revocation path reads the merged appConfig.mcpSettings, so the affected specs' mocks were asserting the old base-getter values: - routes/__tests__/mcp.spec.js: add resolveAllowlists to the registry mock. - UserController.mcpOAuth.spec.js: provide mcpSettings on the getAppConfig mock so revokeOAuthToken still receives the expected allowlists. * 🧪 test: e2e proof that admin-panel MCP allowlist override takes effect Adds a Playwright mock-harness spec for #13809. A URL-based MCP fixture (e2e-http, streamable-http SDK server) boots inspectionFailed because its origin is omitted from the YAML mcpSettings.allowedDomains; the spec adds that origin via an admin config override (PUT /api/admin/config/user/:id) and asserts the server reinitializes — exercising the real resolver path through the backend + DB. Before the fix, reinspection used the frozen YAML allowlist and the server stayed unreachable. - e2e/setup/fake-mcp-http-server.js: streamable-HTTP MCP fixture (health GET /). - e2e/playwright.config.mock.ts: boot the fixture as a second webServer. - e2e/config/librechat.e2e.yaml: mcpSettings.allowedDomains (excludes 127.0.0.1) + the e2e-http server. - e2e/specs/mock/mcp-allowlist-override.spec.ts: login → baseline reinit fails → apply override → reinit succeeds.
This commit is contained in:
Danny Avila
GitHub
parent
c04bddd304
commit
49f4b659f6
18 changed files with 522 additions and 43 deletions
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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(() => {
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
},
|
||||
],
|
||||
});
|
||||
|
|
|
|||
78
e2e/setup/fake-mcp-http-server.js
Normal file
78
e2e/setup/fake-mcp-http-server.js
Normal file
|
|
@ -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<string, InstanceType<typeof StreamableHTTPServerTransport>>} */
|
||||
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`);
|
||||
});
|
||||
77
e2e/specs/mock/mcp-allowlist-override.spec.ts
Normal file
77
e2e/specs/mock/mcp-allowlist-override.spec.ts
Normal file
|
|
@ -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<string, string>,
|
||||
): 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);
|
||||
});
|
||||
});
|
||||
|
|
@ -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,
|
||||
);
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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', () => ({
|
||||
|
|
|
|||
|
|
@ -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', () => ({
|
||||
|
|
|
|||
|
|
@ -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');
|
||||
|
|
|
|||
|
|
@ -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<t.ParsedServerConfig>;
|
||||
private readonly readThroughCacheAll: Keyv<Record<string, t.ParsedServerConfig>>;
|
||||
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<string, t.ParsedServerConfig> = {};
|
||||
|
||||
// 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<t.ParsedServerConfig | undefined> {
|
||||
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<t.ParsedServerConfig | undefined> {
|
||||
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<string, unknown>).sort(([a], [b]) => a.localeCompare(b)),
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue