mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
🪪 fix: Prevent MCP Server Name Collisions (#13256)
* fix: prevent MCP server name collisions * chore: address MCP registry review nits * fix: reserve MCP config names from request context * chore: format MCP registry changes * chore: address MCP collision review findings
This commit is contained in:
parent
d462bf4113
commit
bd64251eb9
12 changed files with 354 additions and 49 deletions
|
|
@ -14,7 +14,11 @@ const {
|
|||
isMCPInspectionFailedError,
|
||||
} = require('@librechat/api');
|
||||
const { Constants, MCPServerUserInputSchema } = require('librechat-data-provider');
|
||||
const { resolveConfigServers, resolveAllMcpConfigs } = require('~/server/services/MCP');
|
||||
const {
|
||||
resolveConfigServers,
|
||||
resolveMcpConfigNames,
|
||||
resolveAllMcpConfigs,
|
||||
} = require('~/server/services/MCP');
|
||||
const { cacheMCPServerTools, getMCPServerTools } = require('~/server/services/Config');
|
||||
const { getMCPManager, getMCPServersRegistry } = require('~/config');
|
||||
|
||||
|
|
@ -213,11 +217,13 @@ const createMCPServerController = async (req, res) => {
|
|||
errors: validation.error.errors,
|
||||
});
|
||||
}
|
||||
const reservedServerNames = await resolveMcpConfigNames(req);
|
||||
const result = await getMCPServersRegistry().addServer(
|
||||
'temp_server_name',
|
||||
validation.data,
|
||||
'DB',
|
||||
userId,
|
||||
reservedServerNames,
|
||||
);
|
||||
res.status(201).json({
|
||||
serverName: result.serverName,
|
||||
|
|
|
|||
|
|
@ -108,9 +108,11 @@ jest.mock('~/server/services/Config/mcp', () => ({
|
|||
}));
|
||||
|
||||
const mockResolveAllMcpConfigs = jest.fn().mockResolvedValue({});
|
||||
const mockResolveMcpConfigNames = jest.fn().mockResolvedValue([]);
|
||||
jest.mock('~/server/services/MCP', () => ({
|
||||
getMCPSetupData: jest.fn(),
|
||||
resolveConfigServers: jest.fn().mockResolvedValue({}),
|
||||
resolveMcpConfigNames: (...args) => mockResolveMcpConfigNames(...args),
|
||||
resolveAllMcpConfigs: (...args) => mockResolveAllMcpConfigs(...args),
|
||||
getServerConnectionStatus: jest.fn(),
|
||||
}));
|
||||
|
|
@ -171,6 +173,8 @@ describe('MCP Routes', () => {
|
|||
|
||||
beforeEach(() => {
|
||||
jest.clearAllMocks();
|
||||
mockResolveAllMcpConfigs.mockResolvedValue({});
|
||||
mockResolveMcpConfigNames.mockResolvedValue([]);
|
||||
});
|
||||
|
||||
describe('GET /:serverName/oauth/initiate', () => {
|
||||
|
|
@ -2155,6 +2159,35 @@ describe('MCP Routes', () => {
|
|||
}),
|
||||
'DB',
|
||||
'test-user-id',
|
||||
[],
|
||||
);
|
||||
});
|
||||
|
||||
it('should reserve config-managed server names when creating MCP server', async () => {
|
||||
const validConfig = {
|
||||
type: 'sse',
|
||||
url: 'https://mcp-server.example.com/sse',
|
||||
title: 'Test SSE Server',
|
||||
};
|
||||
|
||||
mockResolveMcpConfigNames.mockResolvedValueOnce(['config_slack']);
|
||||
mockRegistryInstance.addServer.mockResolvedValue({
|
||||
serverName: 'test-sse-server',
|
||||
config: validConfig,
|
||||
});
|
||||
|
||||
const response = await request(app).post('/api/mcp/servers').send({ config: validConfig });
|
||||
|
||||
expect(response.status).toBe(201);
|
||||
expect(mockRegistryInstance.addServer).toHaveBeenCalledWith(
|
||||
'temp_server_name',
|
||||
expect.objectContaining({
|
||||
type: 'sse',
|
||||
url: 'https://mcp-server.example.com/sse',
|
||||
}),
|
||||
'DB',
|
||||
'test-user-id',
|
||||
['config_slack'],
|
||||
);
|
||||
});
|
||||
|
||||
|
|
@ -2286,6 +2319,22 @@ describe('MCP Routes', () => {
|
|||
expect(response.status).toBe(500);
|
||||
expect(response.body).toEqual({ message: 'Database connection failed' });
|
||||
});
|
||||
|
||||
it('should fail closed when config-managed names cannot be resolved', async () => {
|
||||
const validConfig = {
|
||||
type: 'sse',
|
||||
url: 'https://mcp-server.example.com/sse',
|
||||
title: 'Test Server',
|
||||
};
|
||||
|
||||
mockResolveMcpConfigNames.mockRejectedValueOnce(new Error('Config lookup failed'));
|
||||
|
||||
const response = await request(app).post('/api/mcp/servers').send({ config: validConfig });
|
||||
|
||||
expect(response.status).toBe(500);
|
||||
expect(response.body).toEqual({ message: 'Config lookup failed' });
|
||||
expect(mockRegistryInstance.addServer).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('GET /servers/:serverName', () => {
|
||||
|
|
|
|||
|
|
@ -54,6 +54,15 @@ function evictStale(map, ttl) {
|
|||
const unavailableMsg =
|
||||
"This tool's MCP server is temporarily unavailable. Please try again shortly.";
|
||||
|
||||
async function getAppConfigForRequest(req) {
|
||||
const user = req?.user;
|
||||
return await getAppConfigForUser(user?.id, user);
|
||||
}
|
||||
|
||||
async function getAppConfigForUser(userId, user) {
|
||||
return await getAppConfig({ role: user?.role, tenantId: getTenantId(), userId });
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolves config-source MCP servers from admin Config overrides for the current
|
||||
* request context. Returns the parsed configs keyed by server name.
|
||||
|
|
@ -63,12 +72,7 @@ const unavailableMsg =
|
|||
async function resolveConfigServers(req) {
|
||||
try {
|
||||
const registry = getMCPServersRegistry();
|
||||
const user = req?.user;
|
||||
const appConfig = await getAppConfig({
|
||||
role: user?.role,
|
||||
tenantId: getTenantId(),
|
||||
userId: user?.id,
|
||||
});
|
||||
const appConfig = await getAppConfigForRequest(req);
|
||||
return await registry.ensureConfigServers(appConfig?.mcpConfig || {});
|
||||
} catch (error) {
|
||||
logger.warn(
|
||||
|
|
@ -79,6 +83,18 @@ async function resolveConfigServers(req) {
|
|||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolves operator-managed MCP server names from admin Config overrides for the current request.
|
||||
* Returns a request-time snapshot for DB server creation, not a cross-process lock.
|
||||
* @throws Propagates app config lookup errors to keep DB server creation fail-closed.
|
||||
* @param {import('express').Request} req - Express request with user context
|
||||
* @returns {Promise<string[]>}
|
||||
*/
|
||||
async function resolveMcpConfigNames(req) {
|
||||
const appConfig = await getAppConfigForRequest(req);
|
||||
return Object.keys(appConfig?.mcpConfig || {});
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolves config-source servers and merges all server configs (YAML + config + user DB)
|
||||
* for the given user context. Shared helper for controllers needing the full merged config.
|
||||
|
|
@ -88,7 +104,7 @@ async function resolveConfigServers(req) {
|
|||
*/
|
||||
async function resolveAllMcpConfigs(userId, user) {
|
||||
const registry = getMCPServersRegistry();
|
||||
const appConfig = await getAppConfig({ role: user?.role, tenantId: getTenantId(), userId });
|
||||
const appConfig = await getAppConfigForUser(userId, user);
|
||||
let configServers = {};
|
||||
try {
|
||||
configServers = await registry.ensureConfigServers(appConfig?.mcpConfig || {});
|
||||
|
|
@ -874,6 +890,7 @@ module.exports = {
|
|||
createMCPTools,
|
||||
getMCPSetupData,
|
||||
resolveConfigServers,
|
||||
resolveMcpConfigNames,
|
||||
resolveAllMcpConfigs,
|
||||
checkOAuthFlowStatus,
|
||||
getServerConnectionStatus,
|
||||
|
|
|
|||
|
|
@ -48,7 +48,7 @@ jest.mock('~/server/services/Tools/mcp', () => ({
|
|||
}));
|
||||
|
||||
const { getAppConfig } = require('~/server/services/Config');
|
||||
const { resolveConfigServers, resolveAllMcpConfigs } = require('../MCP');
|
||||
const { resolveConfigServers, resolveMcpConfigNames, resolveAllMcpConfigs } = require('../MCP');
|
||||
|
||||
describe('resolveConfigServers', () => {
|
||||
beforeEach(() => jest.clearAllMocks());
|
||||
|
|
@ -93,6 +93,35 @@ describe('resolveConfigServers', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('resolveMcpConfigNames', () => {
|
||||
beforeEach(() => jest.clearAllMocks());
|
||||
|
||||
it('resolves current request config server names', async () => {
|
||||
getAppConfig.mockResolvedValue({ mcpConfig: { cfg_srv: {}, yaml_srv: {} } });
|
||||
|
||||
const result = await resolveMcpConfigNames({ user: { id: 'u1', role: 'admin' } });
|
||||
|
||||
expect(result).toEqual(['cfg_srv', 'yaml_srv']);
|
||||
expect(getAppConfig).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ role: 'admin', userId: 'u1' }),
|
||||
);
|
||||
});
|
||||
|
||||
it('returns [] when mcpConfig is absent', async () => {
|
||||
getAppConfig.mockResolvedValue({});
|
||||
|
||||
const result = await resolveMcpConfigNames({ user: { id: 'u1' } });
|
||||
|
||||
expect(result).toEqual([]);
|
||||
});
|
||||
|
||||
it('propagates getAppConfig failures for write-path callers', async () => {
|
||||
getAppConfig.mockRejectedValue(new Error('db timeout'));
|
||||
|
||||
await expect(resolveMcpConfigNames({ user: { id: 'u1' } })).rejects.toThrow('db timeout');
|
||||
});
|
||||
});
|
||||
|
||||
describe('resolveAllMcpConfigs', () => {
|
||||
beforeEach(() => jest.clearAllMocks());
|
||||
|
||||
|
|
|
|||
|
|
@ -35,12 +35,12 @@ const CONFIG_SERVER_INIT_TIMEOUT_MS = (() => {
|
|||
* - Config Cache (configCacheRepo): Admin-defined configs from Config overrides, lazily initialized
|
||||
* - DB Repository (dbConfigsRepo): User-provided configs created at runtime (MongoDB + ACL)
|
||||
*
|
||||
* Query priority: YAML cache → Config cache → DB.
|
||||
* Query priority: Config cache → YAML cache → DB.
|
||||
*/
|
||||
export class MCPServersRegistry {
|
||||
private static instance: MCPServersRegistry;
|
||||
|
||||
private readonly dbConfigsRepo: IServerConfigsRepositoryInterface;
|
||||
private readonly dbConfigsRepo: ServerConfigsDB;
|
||||
private readonly cacheConfigsRepo: IServerConfigsRepositoryInterface;
|
||||
private readonly configCacheRepo: IServerConfigsRepositoryInterface;
|
||||
private readonly allowedDomains?: string[] | null;
|
||||
|
|
@ -164,8 +164,7 @@ export class MCPServersRegistry {
|
|||
|
||||
/**
|
||||
* Returns all server configs visible to the given user.
|
||||
* YAML and Config tiers are mutually exclusive by design (`ensureConfigServers` filters
|
||||
* YAML names), so the spread order only matters for User DB (highest priority) overriding both.
|
||||
* Operator-managed servers (YAML + Config) override User DB servers on name collisions.
|
||||
*/
|
||||
public async getAllServerConfigs(
|
||||
userId?: string,
|
||||
|
|
@ -176,11 +175,13 @@ export class MCPServersRegistry {
|
|||
return this.getBaseServerConfigs(userId, role);
|
||||
}
|
||||
const base = await this.getBaseServerConfigs(userId, role);
|
||||
return { ...configServers, ...base };
|
||||
this.warnOnOperatorManagedNameCollisions(configServers, base, 'Config');
|
||||
return { ...base, ...configServers };
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns YAML + user-DB server configs, cached via `readThroughCacheAll`.
|
||||
* YAML wins on name collisions so a user-created server cannot hide global config.
|
||||
* Always called by `getAllServerConfigs` so the DB query is amortized across
|
||||
* requests within the TTL window regardless of whether `configServers` is present.
|
||||
*/
|
||||
|
|
@ -214,10 +215,14 @@ export class MCPServersRegistry {
|
|||
userId?: string,
|
||||
role?: string,
|
||||
): Promise<Record<string, t.ParsedServerConfig>> {
|
||||
const result = {
|
||||
...(await this.cacheConfigsRepo.getAll()),
|
||||
...(await this.dbConfigsRepo.getAll(userId, role)),
|
||||
};
|
||||
const [dbConfigs, yamlConfigs] = await Promise.all([
|
||||
this.dbConfigsRepo.getAll(userId, role),
|
||||
this.cacheConfigsRepo.getAll(),
|
||||
]);
|
||||
|
||||
this.warnOnOperatorManagedNameCollisions(yamlConfigs, dbConfigs, 'YAML');
|
||||
|
||||
const result = { ...dbConfigs, ...yamlConfigs };
|
||||
|
||||
await this.readThroughCacheAll.set(cacheKey, result);
|
||||
return result;
|
||||
|
|
@ -230,15 +235,14 @@ export class MCPServersRegistry {
|
|||
public async addServerStub(
|
||||
serverName: string,
|
||||
config: t.MCPOptions,
|
||||
storageLocation: 'CACHE' | 'DB',
|
||||
storageLocation: 'CACHE',
|
||||
userId?: string,
|
||||
): Promise<t.AddServerResult> {
|
||||
const configRepo = this.getConfigRepository(storageLocation);
|
||||
const source: t.MCPServerSource = storageLocation === 'CACHE' ? 'yaml' : 'user';
|
||||
const stubConfig: t.ParsedServerConfig = { ...config, inspectionFailed: true, source };
|
||||
const stubConfig: t.ParsedServerConfig = { ...config, inspectionFailed: true, source: 'yaml' };
|
||||
const result = await configRepo.add(serverName, stubConfig, userId);
|
||||
await this.readThroughCache.delete(this.getReadThroughCacheKey(serverName, userId));
|
||||
await this.readThroughCache.delete(this.getReadThroughCacheKey(serverName));
|
||||
await this.invalidateServerReadCaches(result.serverName, userId);
|
||||
this.resetYamlServerNamesMemo();
|
||||
return result;
|
||||
}
|
||||
|
||||
|
|
@ -247,6 +251,7 @@ export class MCPServersRegistry {
|
|||
config: t.MCPOptions,
|
||||
storageLocation: 'CACHE' | 'DB',
|
||||
userId?: string,
|
||||
reservedServerNames?: Iterable<string>,
|
||||
): Promise<t.AddServerResult> {
|
||||
const configRepo = this.getConfigRepository(storageLocation);
|
||||
let parsedConfig: t.ParsedServerConfig;
|
||||
|
|
@ -269,7 +274,20 @@ export class MCPServersRegistry {
|
|||
...parsedConfig,
|
||||
source: (storageLocation === 'CACHE' ? 'yaml' : 'user') as t.MCPServerSource,
|
||||
};
|
||||
return await configRepo.add(serverName, tagged, userId);
|
||||
const result =
|
||||
storageLocation === 'DB'
|
||||
? await this.dbConfigsRepo.add(
|
||||
serverName,
|
||||
tagged,
|
||||
userId,
|
||||
await this.getOperatorManagedServerNames(reservedServerNames),
|
||||
)
|
||||
: await configRepo.add(serverName, tagged, userId);
|
||||
await this.invalidateServerReadCaches(result.serverName, userId);
|
||||
if (storageLocation === 'CACHE') {
|
||||
this.resetYamlServerNamesMemo();
|
||||
}
|
||||
return result;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -312,10 +330,7 @@ export class MCPServersRegistry {
|
|||
|
||||
const updatedConfig = { ...parsedConfig, updatedAt: Date.now() };
|
||||
await configRepo.update(serverName, updatedConfig, userId);
|
||||
await this.readThroughCache.delete(this.getReadThroughCacheKey(serverName, userId));
|
||||
await this.readThroughCache.delete(this.getReadThroughCacheKey(serverName));
|
||||
// Full clear required: getAllServerConfigs is keyed by userId with no reverse index to enumerate cached keys
|
||||
await this.readThroughCacheAll.clear();
|
||||
await this.invalidateServerReadCaches(serverName, userId);
|
||||
return { serverName, config: updatedConfig };
|
||||
}
|
||||
|
||||
|
|
@ -359,6 +374,7 @@ export class MCPServersRegistry {
|
|||
throw new MCPInspectionFailedError(serverName, error as Error);
|
||||
}
|
||||
await configRepo.update(serverName, parsedConfig, userId);
|
||||
await this.invalidateServerReadCaches(serverName, userId);
|
||||
return parsedConfig;
|
||||
}
|
||||
|
||||
|
|
@ -515,12 +531,7 @@ export class MCPServersRegistry {
|
|||
public async invalidateConfigCache(): Promise<string[]> {
|
||||
const allCached = await this.configCacheRepo.getAll();
|
||||
const evictedNames = [
|
||||
...new Set(
|
||||
Object.keys(allCached).map((key) => {
|
||||
const lastColon = key.lastIndexOf(':');
|
||||
return lastColon > 0 ? key.slice(0, lastColon) : key;
|
||||
}),
|
||||
),
|
||||
...new Set(Object.keys(allCached).map((key) => this.parseServerNameFromConfigCacheKey(key))),
|
||||
];
|
||||
|
||||
await Promise.all([
|
||||
|
|
@ -553,8 +564,7 @@ export class MCPServersRegistry {
|
|||
await this.configCacheRepo.reset();
|
||||
await this.readThroughCache.clear();
|
||||
await this.readThroughCacheAll.clear();
|
||||
this.yamlServerNames = null;
|
||||
this.yamlServerNamesPromise = null;
|
||||
this.resetYamlServerNamesMemo();
|
||||
}
|
||||
|
||||
public async removeServer(
|
||||
|
|
@ -564,6 +574,10 @@ export class MCPServersRegistry {
|
|||
): Promise<void> {
|
||||
const configRepo = this.getConfigRepository(storageLocation);
|
||||
await configRepo.remove(serverName, userId);
|
||||
await this.invalidateServerReadCaches(serverName, userId);
|
||||
if (storageLocation === 'CACHE') {
|
||||
this.resetYamlServerNamesMemo();
|
||||
}
|
||||
}
|
||||
|
||||
private getConfigRepository(storageLocation: 'CACHE' | 'DB'): IServerConfigsRepositoryInterface {
|
||||
|
|
@ -583,6 +597,55 @@ export class MCPServersRegistry {
|
|||
return userId ? `${serverName}::${userId}` : serverName;
|
||||
}
|
||||
|
||||
private async invalidateServerReadCaches(serverName: string, userId?: string): Promise<void> {
|
||||
const deletes = [
|
||||
this.readThroughCache.delete(this.getReadThroughCacheKey(serverName)),
|
||||
this.readThroughCacheAll.clear(),
|
||||
];
|
||||
|
||||
if (userId) {
|
||||
deletes.push(this.readThroughCache.delete(this.getReadThroughCacheKey(serverName, userId)));
|
||||
}
|
||||
|
||||
await Promise.all(deletes);
|
||||
}
|
||||
|
||||
private async getOperatorManagedServerNames(
|
||||
reservedServerNames: Iterable<string> = [],
|
||||
): Promise<string[]> {
|
||||
const yamlNames = await this.getYamlServerNames();
|
||||
|
||||
return [...new Set([...yamlNames, ...reservedServerNames])];
|
||||
}
|
||||
|
||||
private parseServerNameFromConfigCacheKey(cacheKey: string): string {
|
||||
const lastColon = cacheKey.lastIndexOf(':');
|
||||
return lastColon > 0 ? cacheKey.slice(0, lastColon) : cacheKey;
|
||||
}
|
||||
|
||||
private warnOnOperatorManagedNameCollisions(
|
||||
operatorConfigs: Record<string, t.ParsedServerConfig>,
|
||||
candidateConfigs: Record<string, t.ParsedServerConfig>,
|
||||
operatorSource: 'Config' | 'YAML',
|
||||
): void {
|
||||
const shadowedNames = Object.keys(operatorConfigs).filter(
|
||||
(serverName) => candidateConfigs[serverName]?.source === 'user',
|
||||
);
|
||||
if (!shadowedNames.length) {
|
||||
return;
|
||||
}
|
||||
|
||||
logger.warn(
|
||||
`[MCPServersRegistry] ${operatorSource} MCP server(s) shadow DB-backed server(s) with colliding name(s): ` +
|
||||
`${shadowedNames.join(', ')}. DB records remain stored but are hidden while operator-managed servers use these names.`,
|
||||
);
|
||||
}
|
||||
|
||||
private resetYamlServerNamesMemo(): void {
|
||||
this.yamlServerNames = null;
|
||||
this.yamlServerNamesPromise = null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns memoized YAML server names. Populated lazily on first call after boot/reset.
|
||||
* YAML servers don't change after boot, so this avoids repeated `getAll()` calls.
|
||||
|
|
|
|||
|
|
@ -1,7 +1,7 @@
|
|||
import { ParsedServerConfig, AddServerResult } from '~/mcp/types';
|
||||
|
||||
/**
|
||||
* Interface for future DB implementation
|
||||
* Contract for MCP server configuration storage, whether cache-backed or DB-backed.
|
||||
*/
|
||||
export interface IServerConfigsRepositoryInterface {
|
||||
add(serverName: string, config: ParsedServerConfig, userId?: string): Promise<AddServerResult>;
|
||||
|
|
|
|||
|
|
@ -1,4 +1,5 @@
|
|||
import type * as t from '~/mcp/types';
|
||||
import { logger } from '@librechat/data-schemas';
|
||||
import { MCPServersRegistry } from '~/mcp/registry/MCPServersRegistry';
|
||||
import { MCPServerInspector } from '~/mcp/registry/MCPServerInspector';
|
||||
|
||||
|
|
@ -10,7 +11,10 @@ jest.mock('~/mcp/registry/db/ServerConfigsDB', () => ({
|
|||
ServerConfigsDB: jest.fn().mockImplementation(() => ({
|
||||
get: jest.fn().mockResolvedValue(undefined),
|
||||
getAll: jest.fn().mockResolvedValue({}),
|
||||
add: jest.fn().mockResolvedValue(undefined),
|
||||
add: jest.fn().mockImplementation(async (serverName: string, config: t.ParsedServerConfig) => ({
|
||||
serverName,
|
||||
config,
|
||||
})),
|
||||
update: jest.fn().mockResolvedValue(undefined),
|
||||
remove: jest.fn().mockResolvedValue(undefined),
|
||||
reset: jest.fn().mockResolvedValue(undefined),
|
||||
|
|
@ -99,6 +103,90 @@ describe('MCPServersRegistry', () => {
|
|||
expect(configs).toHaveProperty('app_server');
|
||||
expect(configs).toHaveProperty('user_server');
|
||||
});
|
||||
|
||||
it('should keep YAML servers authoritative when a DB server has the same name', async () => {
|
||||
const warnSpy = jest.spyOn(logger, 'warn').mockImplementation();
|
||||
const yamlConfig = { ...testParsedConfig, source: 'yaml' as const, title: 'YAML Slack' };
|
||||
const dbConfig = { ...testParsedConfig, source: 'user' as const, title: 'User Slack' };
|
||||
await registry['cacheConfigsRepo'].add('slack', yamlConfig);
|
||||
jest.spyOn(registry['dbConfigsRepo'], 'getAll').mockResolvedValue({
|
||||
slack: dbConfig,
|
||||
user_server: dbConfig,
|
||||
});
|
||||
|
||||
try {
|
||||
const configs = await registry.getAllServerConfigs('user-1');
|
||||
|
||||
expect(configs.slack).toMatchObject({ source: 'yaml', title: 'YAML Slack' });
|
||||
expect(configs.user_server).toMatchObject({ source: 'user', title: 'User Slack' });
|
||||
} finally {
|
||||
warnSpy.mockRestore();
|
||||
}
|
||||
});
|
||||
|
||||
it('should warn when operator-managed servers shadow DB servers', async () => {
|
||||
const warnSpy = jest.spyOn(logger, 'warn').mockImplementation();
|
||||
const yamlConfig = { ...testParsedConfig, source: 'yaml' as const, title: 'YAML Slack' };
|
||||
const dbConfig = { ...testParsedConfig, source: 'user' as const, title: 'User Slack' };
|
||||
await registry['cacheConfigsRepo'].add('slack', yamlConfig);
|
||||
jest.spyOn(registry['dbConfigsRepo'], 'getAll').mockResolvedValue({ slack: dbConfig });
|
||||
|
||||
try {
|
||||
await registry.getAllServerConfigs('user-1');
|
||||
|
||||
expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('slack'));
|
||||
expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('shadow DB-backed server'));
|
||||
} finally {
|
||||
warnSpy.mockRestore();
|
||||
}
|
||||
});
|
||||
|
||||
it('should warn when config servers shadow DB servers', async () => {
|
||||
const warnSpy = jest.spyOn(logger, 'warn').mockImplementation();
|
||||
const configServer = {
|
||||
...testParsedConfig,
|
||||
source: 'config' as const,
|
||||
title: 'Config Slack',
|
||||
};
|
||||
const dbConfig = { ...testParsedConfig, source: 'user' as const, title: 'User Slack' };
|
||||
jest.spyOn(registry['dbConfigsRepo'], 'getAll').mockResolvedValue({ slack: dbConfig });
|
||||
|
||||
try {
|
||||
await registry.getAllServerConfigs('user-1', { slack: configServer });
|
||||
|
||||
expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('Config MCP server'));
|
||||
expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('slack'));
|
||||
} finally {
|
||||
warnSpy.mockRestore();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('addServer', () => {
|
||||
it('should reserve YAML and current config server names when creating DB servers', async () => {
|
||||
await registry.addServer('slack', { ...testParsedConfig, title: 'Slack' }, 'CACHE');
|
||||
await registry['configCacheRepo'].upsert('other_tenant:hash', {
|
||||
...testParsedConfig,
|
||||
source: 'config',
|
||||
title: 'Other Tenant Server',
|
||||
});
|
||||
const dbAddSpy = jest.spyOn(registry['dbConfigsRepo'], 'add').mockResolvedValue({
|
||||
serverName: 'slack-2',
|
||||
config: { ...testParsedConfig, source: 'user', title: 'Slack' },
|
||||
});
|
||||
|
||||
await registry.addServer(
|
||||
'temp_server_name',
|
||||
{ ...testParsedConfig, title: 'Slack' },
|
||||
'DB',
|
||||
'user-1',
|
||||
['config_slack'],
|
||||
);
|
||||
|
||||
const reservedServerNames = Array.from(dbAddSpy.mock.calls[0]?.[3] ?? []);
|
||||
expect(reservedServerNames).toEqual(expect.arrayContaining(['slack', 'config_slack']));
|
||||
expect(reservedServerNames).not.toContain('other_tenant');
|
||||
});
|
||||
});
|
||||
|
||||
describe('reset', () => {
|
||||
|
|
|
|||
|
|
@ -124,6 +124,13 @@ describe('ServerConfigsDB', () => {
|
|||
expect(result.config.dbId).toBeDefined();
|
||||
});
|
||||
|
||||
it('should reserve operator-managed server names when creating a DB server', async () => {
|
||||
const config = createSSEConfig('My Test Server', 'A test server');
|
||||
const result = await serverConfigsDB.add('temp-name', config, userId, ['my-test-server']);
|
||||
|
||||
expect(result.serverName).toBe('my-test-server-2');
|
||||
});
|
||||
|
||||
it('should grant owner ACL to the user', async () => {
|
||||
const config = createSSEConfig('ACL Test Server');
|
||||
const result = await serverConfigsDB.add('temp-name', config, userId);
|
||||
|
|
|
|||
|
|
@ -221,7 +221,7 @@ describe('MCPServersRegistry — ensureConfigServers', () => {
|
|||
});
|
||||
|
||||
describe('merge order', () => {
|
||||
it('should merge YAML → config → user with correct precedence in getAllServerConfigs', async () => {
|
||||
it('should keep operator-managed servers authoritative in getAllServerConfigs', async () => {
|
||||
await registry.addServer('yaml_srv', yamlConfig, 'CACHE');
|
||||
|
||||
const configServers = await registry.ensureConfigServers({ config_srv: sseConfig });
|
||||
|
|
|
|||
|
|
@ -105,7 +105,7 @@ export class ServerConfigsDB implements IServerConfigsRepositoryInterface {
|
|||
|
||||
/**
|
||||
* Creates a new MCP server and grants owner permissions to the user.
|
||||
* @param serverName - Temporary server name (not persisted) will be replaced by the nano id generated by the db method
|
||||
* @param serverName - Placeholder name kept for repository compatibility; final serverName comes from config.title
|
||||
* @param config - Server configuration to store
|
||||
* @param userId - ID of the user creating the server (required)
|
||||
* @returns The created server result with serverName and config (including dbId)
|
||||
|
|
@ -115,9 +115,10 @@ export class ServerConfigsDB implements IServerConfigsRepositoryInterface {
|
|||
serverName: string,
|
||||
config: ParsedServerConfig,
|
||||
userId?: string,
|
||||
reservedServerNames?: Iterable<string>,
|
||||
): Promise<AddServerResult> {
|
||||
logger.debug(
|
||||
`[ServerConfigsDB.add] Starting Creating server with temp servername: ${serverName} for the user with the ID ${userId}`,
|
||||
`[ServerConfigsDB.add] Creating DB-backed server from config title. Placeholder: ${serverName}; userId: ${userId}`,
|
||||
);
|
||||
if (!userId) {
|
||||
throw new Error(
|
||||
|
|
@ -139,6 +140,7 @@ export class ServerConfigsDB implements IServerConfigsRepositoryInterface {
|
|||
const createdServer = await this._dbMethods.createMCPServer({
|
||||
config: encryptedConfig,
|
||||
author: userId,
|
||||
reservedServerNames,
|
||||
});
|
||||
await this._aclService.grantPermission({
|
||||
principalType: PrincipalType.USER,
|
||||
|
|
|
|||
|
|
@ -141,6 +141,37 @@ describe('MCPServer Model Tests', () => {
|
|||
expect(server.serverName).toBe('test-server-2');
|
||||
});
|
||||
|
||||
test('should append suffix when base name is reserved', async () => {
|
||||
const server = await methods.createMCPServer({
|
||||
config: createSSEConfig('Test Server'),
|
||||
author: authorId,
|
||||
reservedServerNames: ['test-server'],
|
||||
});
|
||||
|
||||
expect(server.serverName).toBe('test-server-2');
|
||||
});
|
||||
|
||||
test('should skip both DB and reserved names when finding next suffix', async () => {
|
||||
await MCPServer.create({
|
||||
serverName: 'test-server',
|
||||
config: createSSEConfig('Test Server'),
|
||||
author: authorId,
|
||||
});
|
||||
await MCPServer.create({
|
||||
serverName: 'test-server-2',
|
||||
config: createSSEConfig('Test Server'),
|
||||
author: authorId,
|
||||
});
|
||||
|
||||
const server = await methods.createMCPServer({
|
||||
config: createSSEConfig('Test Server'),
|
||||
author: authorId,
|
||||
reservedServerNames: ['test-server-3'],
|
||||
});
|
||||
|
||||
expect(server.serverName).toBe('test-server-4');
|
||||
});
|
||||
|
||||
test('should find next available number in sequence', async () => {
|
||||
// Create servers with sequential names
|
||||
await MCPServer.create({
|
||||
|
|
|
|||
|
|
@ -46,27 +46,38 @@ function generateServerNameFromTitle(title: string): string {
|
|||
|
||||
export function createMCPServerMethods(mongoose: typeof import('mongoose')) {
|
||||
/**
|
||||
* Finds the next available server name by checking for duplicates.
|
||||
* If baseName exists, returns baseName-2, baseName-3, etc.
|
||||
* Finds the next available server name by checking DB and reserved-name collisions.
|
||||
* If baseName is taken or reserved, returns baseName-2, baseName-3, etc.
|
||||
*/
|
||||
async function findNextAvailableServerName(baseName: string): Promise<string> {
|
||||
async function findNextAvailableServerName(
|
||||
baseName: string,
|
||||
reservedServerNames: Set<string> = new Set(),
|
||||
): Promise<string> {
|
||||
const MCPServer = mongoose.models.MCPServer as Model<MCPServerDocument>;
|
||||
|
||||
// Find all servers with matching base name pattern (baseName or baseName-N)
|
||||
const escapedBaseName = escapeRegex(baseName);
|
||||
const matchingNamePattern = new RegExp(`^${escapedBaseName}(-\\d+)?$`);
|
||||
const existing = await MCPServer.find({
|
||||
serverName: { $regex: `^${escapedBaseName}(-\\d+)?$` },
|
||||
serverName: { $regex: matchingNamePattern },
|
||||
})
|
||||
.select('serverName')
|
||||
.lean<Array<{ serverName: string }>>();
|
||||
|
||||
if (existing.length === 0) {
|
||||
const existingNames = new Set([
|
||||
...existing.map((server) => server.serverName),
|
||||
...Array.from(reservedServerNames).filter((serverName) =>
|
||||
matchingNamePattern.test(serverName),
|
||||
),
|
||||
]);
|
||||
|
||||
if (existingNames.size === 0) {
|
||||
return baseName;
|
||||
}
|
||||
|
||||
// Extract numbers from existing names
|
||||
const numbers = existing.map((s) => {
|
||||
const match = s.serverName.match(/-(\d+)$/);
|
||||
const numbers = Array.from(existingNames).map((serverName) => {
|
||||
const match = serverName.match(/-(\d+)$/);
|
||||
return match ? parseInt(match[1], 10) : 1;
|
||||
});
|
||||
|
||||
|
|
@ -86,9 +97,11 @@ export function createMCPServerMethods(mongoose: typeof import('mongoose')) {
|
|||
async function createMCPServer(data: {
|
||||
config: MCPOptions;
|
||||
author: string | Types.ObjectId;
|
||||
reservedServerNames?: Iterable<string>;
|
||||
}): Promise<MCPServerDocument> {
|
||||
const MCPServer = mongoose.models.MCPServer as Model<MCPServerDocument>;
|
||||
let lastError: unknown;
|
||||
const reservedServerNames = new Set(data.reservedServerNames ?? []);
|
||||
|
||||
for (let attempt = 0; attempt < MAX_CREATE_RETRIES; attempt++) {
|
||||
try {
|
||||
|
|
@ -97,7 +110,7 @@ export function createMCPServerMethods(mongoose: typeof import('mongoose')) {
|
|||
let serverName: string;
|
||||
if (data.config.title) {
|
||||
const baseSlug = generateServerNameFromTitle(data.config.title);
|
||||
serverName = await findNextAvailableServerName(baseSlug);
|
||||
serverName = await findNextAvailableServerName(baseSlug, reservedServerNames);
|
||||
} else {
|
||||
serverName = `mcp-${nanoid(16)}`;
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue