diff --git a/api/server/controllers/mcp.js b/api/server/controllers/mcp.js index 8d20cbc82c..a50680f3d1 100644 --- a/api/server/controllers/mcp.js +++ b/api/server/controllers/mcp.js @@ -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, diff --git a/api/server/routes/__tests__/mcp.spec.js b/api/server/routes/__tests__/mcp.spec.js index 30a86a53b2..3a3e6ac796 100644 --- a/api/server/routes/__tests__/mcp.spec.js +++ b/api/server/routes/__tests__/mcp.spec.js @@ -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', () => { diff --git a/api/server/services/MCP.js b/api/server/services/MCP.js index c691c4330a..85c29ddfdc 100644 --- a/api/server/services/MCP.js +++ b/api/server/services/MCP.js @@ -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} + */ +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, diff --git a/api/server/services/__tests__/MCP.spec.js b/api/server/services/__tests__/MCP.spec.js index 2de88cfcd0..cfc71b7752 100644 --- a/api/server/services/__tests__/MCP.spec.js +++ b/api/server/services/__tests__/MCP.spec.js @@ -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()); diff --git a/packages/api/src/mcp/registry/MCPServersRegistry.ts b/packages/api/src/mcp/registry/MCPServersRegistry.ts index 534cb3fd8b..81c59878ff 100644 --- a/packages/api/src/mcp/registry/MCPServersRegistry.ts +++ b/packages/api/src/mcp/registry/MCPServersRegistry.ts @@ -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> { - 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 { 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, ): Promise { 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 { 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 { 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 { + 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 = [], + ): Promise { + 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, + candidateConfigs: Record, + 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. diff --git a/packages/api/src/mcp/registry/ServerConfigsRepositoryInterface.ts b/packages/api/src/mcp/registry/ServerConfigsRepositoryInterface.ts index e55f66faa7..71eac64908 100644 --- a/packages/api/src/mcp/registry/ServerConfigsRepositoryInterface.ts +++ b/packages/api/src/mcp/registry/ServerConfigsRepositoryInterface.ts @@ -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; diff --git a/packages/api/src/mcp/registry/__tests__/MCPServersRegistry.test.ts b/packages/api/src/mcp/registry/__tests__/MCPServersRegistry.test.ts index a20c09705f..a1298dcd6e 100644 --- a/packages/api/src/mcp/registry/__tests__/MCPServersRegistry.test.ts +++ b/packages/api/src/mcp/registry/__tests__/MCPServersRegistry.test.ts @@ -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', () => { diff --git a/packages/api/src/mcp/registry/__tests__/ServerConfigsDB.test.ts b/packages/api/src/mcp/registry/__tests__/ServerConfigsDB.test.ts index e8c089df34..8d031c33a7 100644 --- a/packages/api/src/mcp/registry/__tests__/ServerConfigsDB.test.ts +++ b/packages/api/src/mcp/registry/__tests__/ServerConfigsDB.test.ts @@ -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); diff --git a/packages/api/src/mcp/registry/__tests__/ensureConfigServers.test.ts b/packages/api/src/mcp/registry/__tests__/ensureConfigServers.test.ts index 3d1433f194..9b601dcee2 100644 --- a/packages/api/src/mcp/registry/__tests__/ensureConfigServers.test.ts +++ b/packages/api/src/mcp/registry/__tests__/ensureConfigServers.test.ts @@ -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 }); diff --git a/packages/api/src/mcp/registry/db/ServerConfigsDB.ts b/packages/api/src/mcp/registry/db/ServerConfigsDB.ts index 30e0185721..5eaf92a2c2 100644 --- a/packages/api/src/mcp/registry/db/ServerConfigsDB.ts +++ b/packages/api/src/mcp/registry/db/ServerConfigsDB.ts @@ -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, ): Promise { 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, diff --git a/packages/data-schemas/src/methods/mcpServer.spec.ts b/packages/data-schemas/src/methods/mcpServer.spec.ts index 66f0982438..9d5f915bfd 100644 --- a/packages/data-schemas/src/methods/mcpServer.spec.ts +++ b/packages/data-schemas/src/methods/mcpServer.spec.ts @@ -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({ diff --git a/packages/data-schemas/src/methods/mcpServer.ts b/packages/data-schemas/src/methods/mcpServer.ts index 4851e2c566..d48fd2a4e4 100644 --- a/packages/data-schemas/src/methods/mcpServer.ts +++ b/packages/data-schemas/src/methods/mcpServer.ts @@ -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 { + async function findNextAvailableServerName( + baseName: string, + reservedServerNames: Set = new Set(), + ): Promise { const MCPServer = mongoose.models.MCPServer as Model; // 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>(); - 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; }): Promise { const MCPServer = mongoose.models.MCPServer as Model; 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)}`; }