mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-06-29 10:51:34 +00:00
* 🔐 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.
317 lines
10 KiB
JavaScript
317 lines
10 KiB
JavaScript
/**
|
|
* Tests for initializeMCPs.js
|
|
*
|
|
* These tests verify that MCPServersRegistry and MCPManager are ALWAYS initialized,
|
|
* even when no explicitly configured MCP servers exist. This is critical for the
|
|
* "Dynamic MCP Server Management" feature (introduced in `0.8.2-rc1` release) which
|
|
* allows users to add MCP servers via the UI without requiring explicit configuration.
|
|
*
|
|
* Bug fixed: Previously, MCPManager was only initialized when mcpServers existed
|
|
* in librechat.yaml, causing "MCPManager has not been initialized" errors when
|
|
* users tried to create MCP servers via the UI.
|
|
*/
|
|
|
|
// Mock dependencies before imports
|
|
jest.mock('mongoose', () => ({
|
|
connection: { readyState: 1 },
|
|
}));
|
|
|
|
jest.mock('@librechat/data-schemas', () => ({
|
|
logger: {
|
|
debug: jest.fn(),
|
|
error: jest.fn(),
|
|
info: jest.fn(),
|
|
warn: jest.fn(),
|
|
},
|
|
}));
|
|
|
|
// Mock config functions
|
|
const mockGetAppConfig = jest.fn();
|
|
const mockMergeAppTools = jest.fn();
|
|
|
|
jest.mock('./Config', () => ({
|
|
get getAppConfig() {
|
|
return mockGetAppConfig;
|
|
},
|
|
get mergeAppTools() {
|
|
return mockMergeAppTools;
|
|
},
|
|
}));
|
|
|
|
// Mock MCP singletons
|
|
const mockCreateMCPServersRegistry = jest.fn();
|
|
const mockCreateMCPManager = jest.fn();
|
|
const mockMCPManagerInstance = {
|
|
getAppToolFunctions: jest.fn(),
|
|
};
|
|
|
|
jest.mock('~/config', () => ({
|
|
get createMCPServersRegistry() {
|
|
return mockCreateMCPServersRegistry;
|
|
},
|
|
get createMCPManager() {
|
|
return mockCreateMCPManager;
|
|
},
|
|
}));
|
|
|
|
const { logger } = require('@librechat/data-schemas');
|
|
const initializeMCPs = require('./initializeMCPs');
|
|
|
|
describe('initializeMCPs', () => {
|
|
beforeEach(() => {
|
|
jest.clearAllMocks();
|
|
|
|
// Default: successful initialization
|
|
mockCreateMCPServersRegistry.mockReturnValue(undefined);
|
|
mockCreateMCPManager.mockResolvedValue(mockMCPManagerInstance);
|
|
mockMCPManagerInstance.getAppToolFunctions.mockResolvedValue({});
|
|
mockMergeAppTools.mockResolvedValue(undefined);
|
|
});
|
|
|
|
describe('MCPServersRegistry initialization', () => {
|
|
it('should ALWAYS initialize MCPServersRegistry even without configured servers', async () => {
|
|
mockGetAppConfig.mockResolvedValue({
|
|
mcpConfig: null, // No configured servers
|
|
mcpSettings: { allowedDomains: ['localhost'] },
|
|
});
|
|
|
|
await initializeMCPs();
|
|
|
|
expect(mockCreateMCPServersRegistry).toHaveBeenCalledTimes(1);
|
|
expect(mockCreateMCPServersRegistry).toHaveBeenCalledWith(
|
|
expect.anything(), // mongoose
|
|
['localhost'],
|
|
undefined,
|
|
expect.any(Function), // per-request allowlist resolver
|
|
);
|
|
});
|
|
|
|
it('should pass allowedDomains from mcpSettings to registry', async () => {
|
|
const allowedDomains = ['localhost', '*.example.com', 'trusted-mcp.com'];
|
|
mockGetAppConfig.mockResolvedValue({
|
|
mcpConfig: null,
|
|
mcpSettings: { allowedDomains },
|
|
});
|
|
|
|
await initializeMCPs();
|
|
|
|
expect(mockCreateMCPServersRegistry).toHaveBeenCalledWith(
|
|
expect.anything(),
|
|
allowedDomains,
|
|
undefined,
|
|
expect.any(Function),
|
|
);
|
|
});
|
|
|
|
it('should handle undefined mcpSettings gracefully', async () => {
|
|
mockGetAppConfig.mockResolvedValue({
|
|
mcpConfig: null,
|
|
// mcpSettings is undefined
|
|
});
|
|
|
|
await initializeMCPs();
|
|
|
|
expect(mockCreateMCPServersRegistry).toHaveBeenCalledWith(
|
|
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(() => {
|
|
throw registryError;
|
|
});
|
|
mockGetAppConfig.mockResolvedValue({ mcpConfig: null });
|
|
|
|
await expect(initializeMCPs()).rejects.toThrow('Registry initialization failed');
|
|
expect(logger.error).toHaveBeenCalledWith(
|
|
'[MCP] Failed to initialize MCPServersRegistry:',
|
|
registryError,
|
|
);
|
|
});
|
|
});
|
|
|
|
describe('MCPManager initialization', () => {
|
|
it('should ALWAYS initialize MCPManager even without configured servers', async () => {
|
|
mockGetAppConfig.mockResolvedValue({
|
|
mcpConfig: null, // No configured servers
|
|
});
|
|
|
|
await initializeMCPs();
|
|
|
|
// MCPManager should be created with empty object when no configured servers
|
|
expect(mockCreateMCPManager).toHaveBeenCalledTimes(1);
|
|
expect(mockCreateMCPManager).toHaveBeenCalledWith({});
|
|
});
|
|
|
|
it('should initialize MCPManager with configured servers when provided', async () => {
|
|
const mcpServers = {
|
|
'test-server': { type: 'sse', url: 'http://localhost:3001/sse' },
|
|
'local-server': { type: 'stdio', command: 'node', args: ['server.js'] },
|
|
};
|
|
mockGetAppConfig.mockResolvedValue({ mcpConfig: mcpServers });
|
|
|
|
await initializeMCPs();
|
|
|
|
expect(mockCreateMCPManager).toHaveBeenCalledWith(mcpServers);
|
|
});
|
|
|
|
it('should throw and log error if MCPManager initialization fails', async () => {
|
|
const managerError = new Error('Manager initialization failed');
|
|
mockCreateMCPManager.mockRejectedValue(managerError);
|
|
mockGetAppConfig.mockResolvedValue({ mcpConfig: null });
|
|
|
|
await expect(initializeMCPs()).rejects.toThrow('Manager initialization failed');
|
|
expect(logger.error).toHaveBeenCalledWith(
|
|
'[MCP] Failed to initialize MCPManager:',
|
|
managerError,
|
|
);
|
|
});
|
|
});
|
|
|
|
describe('Tool merging behavior', () => {
|
|
it('should NOT merge tools when no configured servers exist', async () => {
|
|
mockGetAppConfig.mockResolvedValue({
|
|
mcpConfig: null, // No configured servers
|
|
});
|
|
|
|
await initializeMCPs();
|
|
|
|
expect(mockMCPManagerInstance.getAppToolFunctions).not.toHaveBeenCalled();
|
|
expect(mockMergeAppTools).not.toHaveBeenCalled();
|
|
expect(logger.debug).toHaveBeenCalledWith(
|
|
'[MCP] No servers configured. MCPManager ready for UI-based servers.',
|
|
);
|
|
});
|
|
|
|
it('should NOT merge tools when mcpConfig is empty object', async () => {
|
|
mockGetAppConfig.mockResolvedValue({
|
|
mcpConfig: {}, // Empty object
|
|
});
|
|
|
|
await initializeMCPs();
|
|
|
|
expect(mockMCPManagerInstance.getAppToolFunctions).not.toHaveBeenCalled();
|
|
expect(mockMergeAppTools).not.toHaveBeenCalled();
|
|
expect(logger.debug).toHaveBeenCalledWith(
|
|
'[MCP] No servers configured. MCPManager ready for UI-based servers.',
|
|
);
|
|
});
|
|
|
|
it('should merge tools when configured servers exist', async () => {
|
|
const mcpServers = {
|
|
'test-server': { type: 'sse', url: 'http://localhost:3001/sse' },
|
|
};
|
|
const mcpTools = {
|
|
tool1: jest.fn(),
|
|
tool2: jest.fn(),
|
|
};
|
|
mockGetAppConfig.mockResolvedValue({ mcpConfig: mcpServers });
|
|
mockMCPManagerInstance.getAppToolFunctions.mockResolvedValue(mcpTools);
|
|
|
|
await initializeMCPs();
|
|
|
|
expect(mockMCPManagerInstance.getAppToolFunctions).toHaveBeenCalledTimes(1);
|
|
expect(mockMergeAppTools).toHaveBeenCalledWith(mcpTools);
|
|
expect(logger.info).toHaveBeenCalledWith(
|
|
'[MCP] Initialized with 1 configured server and 2 tools.',
|
|
);
|
|
});
|
|
|
|
it('should handle null return from getAppToolFunctions', async () => {
|
|
const mcpServers = { 'test-server': { type: 'sse', url: 'http://localhost:3001' } };
|
|
mockGetAppConfig.mockResolvedValue({ mcpConfig: mcpServers });
|
|
mockMCPManagerInstance.getAppToolFunctions.mockResolvedValue(null);
|
|
|
|
await initializeMCPs();
|
|
|
|
// Should use empty object fallback
|
|
expect(mockMergeAppTools).toHaveBeenCalledWith({});
|
|
expect(logger.info).toHaveBeenCalledWith(
|
|
'[MCP] Initialized with 1 configured server and 0 tools.',
|
|
);
|
|
});
|
|
});
|
|
|
|
describe('Initialization order', () => {
|
|
it('should initialize Registry before Manager', async () => {
|
|
const callOrder = [];
|
|
|
|
mockCreateMCPServersRegistry.mockImplementation(() => {
|
|
callOrder.push('registry');
|
|
});
|
|
mockCreateMCPManager.mockImplementation(async () => {
|
|
callOrder.push('manager');
|
|
return mockMCPManagerInstance;
|
|
});
|
|
mockGetAppConfig.mockResolvedValue({ mcpConfig: null });
|
|
|
|
await initializeMCPs();
|
|
|
|
expect(callOrder).toEqual(['registry', 'manager']);
|
|
});
|
|
|
|
it('should not attempt MCPManager initialization if Registry fails', async () => {
|
|
mockCreateMCPServersRegistry.mockImplementation(() => {
|
|
throw new Error('Registry failed');
|
|
});
|
|
mockGetAppConfig.mockResolvedValue({ mcpConfig: null });
|
|
|
|
await expect(initializeMCPs()).rejects.toThrow('Registry failed');
|
|
expect(mockCreateMCPManager).not.toHaveBeenCalled();
|
|
});
|
|
});
|
|
|
|
describe('UI-based MCP server management support', () => {
|
|
/**
|
|
* This test documents the critical fix:
|
|
* MCPManager must be initialized even without configured servers to support
|
|
* the "Dynamic MCP Server Management" feature where users create
|
|
* MCP servers via the UI.
|
|
*/
|
|
it('should support UI-based server creation without explicit configuration', async () => {
|
|
// Scenario: User has no MCP servers in librechat.yaml but wants to
|
|
// add servers via the UI
|
|
mockGetAppConfig.mockResolvedValue({
|
|
mcpConfig: null,
|
|
mcpSettings: undefined,
|
|
});
|
|
|
|
await initializeMCPs();
|
|
|
|
// Both singletons must be initialized for UI-based management to work
|
|
expect(mockCreateMCPServersRegistry).toHaveBeenCalledTimes(1);
|
|
expect(mockCreateMCPManager).toHaveBeenCalledTimes(1);
|
|
|
|
// Verify manager was created with empty config (not null/undefined)
|
|
expect(mockCreateMCPManager).toHaveBeenCalledWith({});
|
|
});
|
|
});
|
|
});
|