diff --git a/api/server/controllers/agents/client.js b/api/server/controllers/agents/client.js index b862dd1dc3..9b9abe2f57 100644 --- a/api/server/controllers/agents/client.js +++ b/api/server/controllers/agents/client.js @@ -170,11 +170,17 @@ const { encodeAndFormat } = require('~/server/services/Files/images/encode'); const { createContextHandlers } = require('~/app/clients/prompts'); const { resolveConfigServers, getAccessibleMcpServerNames } = require('~/server/services/MCP'); const { getMCPServerTools } = require('~/server/services/Config'); +const { getAccessibleMCPServers } = require('~/server/services/MCP'); const BaseClient = require('~/app/clients/BaseClient'); const { getMCPManager } = require('~/config'); const db = require('~/models'); -const loadAgent = (params) => loadAgentFn(params, { getAgent: db.getAgent, getMCPServerTools }); +const loadAgent = (params) => + loadAgentFn(params, { + getAgent: db.getAgent, + getMCPServerTools, + getAccessibleMCPServers, + }); const MEMORY_INPUT_CHARS_PER_TOKEN = 8; diff --git a/api/server/services/Endpoints/agents/addedConvo.js b/api/server/services/Endpoints/agents/addedConvo.js index 9483b1535f..71c2702587 100644 --- a/api/server/services/Endpoints/agents/addedConvo.js +++ b/api/server/services/Endpoints/agents/addedConvo.js @@ -10,13 +10,17 @@ const { const { isEphemeralAgentId } = require('librechat-data-provider'); const { filterFilesByAgentAccess } = require('~/server/services/Files/permissions'); const { getMCPServerTools } = require('~/server/services/Config'); -const { getAccessibleMcpServerNames } = require('~/server/services/MCP'); +const { getAccessibleMcpServerNames, getAccessibleMCPServers } = require('~/server/services/MCP'); const { isFatalAgentInitializationError } = require('~/server/services/ToolService'); const { getSkillDbMethods, canAuthorSkillFiles } = require('./skillDeps'); const db = require('~/models'); const loadAddedAgent = (params) => - loadAddedAgentFn(params, { getAgent: db.getAgent, getMCPServerTools }); + loadAddedAgentFn(params, { + getAgent: db.getAgent, + getMCPServerTools, + getAccessibleMCPServers, + }); /** * Process addedConvo for parallel agent execution. diff --git a/api/server/services/Endpoints/agents/build.js b/api/server/services/Endpoints/agents/build.js index efd7130091..4c9a40a2ab 100644 --- a/api/server/services/Endpoints/agents/build.js +++ b/api/server/services/Endpoints/agents/build.js @@ -2,9 +2,15 @@ const { logger } = require('@librechat/data-schemas'); const { loadAgent: loadAgentFn } = require('@librechat/api'); const { isAgentsEndpoint, removeNullishValues, Constants } = require('librechat-data-provider'); const { getMCPServerTools } = require('~/server/services/Config'); +const { getAccessibleMCPServers } = require('~/server/services/MCP'); const db = require('~/models'); -const loadAgent = (params) => loadAgentFn(params, { getAgent: db.getAgent, getMCPServerTools }); +const loadAgent = (params) => + loadAgentFn(params, { + getAgent: db.getAgent, + getMCPServerTools, + getAccessibleMCPServers, + }); const buildOptions = (req, endpoint, parsedBody, endpointType) => { const { spec, iconURL, agent_id, chatProjectId, ...model_parameters } = parsedBody; diff --git a/api/server/services/MCP.js b/api/server/services/MCP.js index 2e24ad162b..1e8820b487 100644 --- a/api/server/services/MCP.js +++ b/api/server/services/MCP.js @@ -467,6 +467,18 @@ async function resolveCollisionAuditNames({ rawServerNames, accessibleServerName } } +/** + * The MCP servers a user can reach, keyed by name, with the registry's tier + * precedence already applied. This is the resolution behind `GET /api/mcp/servers`, + * so anything derived from it agrees with the catalog the client was given. + * @param {string} userId + * @param {string} [role] + * @returns {Promise>} + */ +async function getAccessibleMCPServers(userId, role) { + return await resolveAllMcpConfigs(userId, role != null ? { role } : undefined); +} + async function resolveAllMcpConfigs(userId, user) { const registry = getMCPServersRegistry(); const appConfig = await getAppConfigForUser(userId, user); @@ -1648,6 +1660,7 @@ module.exports = { resolveCollisionAuditNames, resolveMcpConfigNames, resolveAllMcpConfigs, + getAccessibleMCPServers, createOAuthStart, checkOAuthFlowStatus, getServerConnectionStatus, diff --git a/packages/api/src/agents/__tests__/load.spec.ts b/packages/api/src/agents/__tests__/load.spec.ts index 83aa9cda3c..bb5559b87a 100644 --- a/packages/api/src/agents/__tests__/load.spec.ts +++ b/packages/api/src/agents/__tests__/load.spec.ts @@ -19,10 +19,12 @@ let createAgent: ReturnType['createAgent']; let getAgent: ReturnType['getAgent']; const mockGetMCPServerTools = jest.fn(); +const mockGetAccessibleMCPServers = jest.fn(); const deps: LoadAgentDeps = { getAgent: (searchParameter) => getAgent(searchParameter) as Promise, getMCPServerTools: mockGetMCPServerTools, + getAccessibleMCPServers: mockGetAccessibleMCPServers, }; describe('loadAgent', () => { @@ -167,6 +169,149 @@ describe('loadAgent', () => { expect(result?.tools).toContain('tool1_mcp_server1'); }); + describe('chat-selectable MCP narrowing', () => { + const { EPHEMERAL_AGENT_ID } = Constants; + + const loadEphemeral = ( + config: Record, + body: { ephemeralAgent: { mcp: string[] } }, + spec?: string, + ) => + loadAgent( + { + req: { + user: { id: 'user123', role: 'USER' }, + config: config as unknown as AppConfig, + body, + }, + spec, + agent_id: EPHEMERAL_AGENT_ID as string, + endpoint: 'openai', + model_parameters: { model: 'gpt-4' } as unknown as AgentModelParameters, + }, + deps, + ); + + const selectedServerNames = () => mockGetMCPServerTools.mock.calls.map((call) => call[1]); + + beforeEach(() => { + mockGetAccessibleMCPServers.mockResolvedValue({ + visible: { chatMenu: true }, + hidden: { chatMenu: false }, + 'agent-only': { consumeOnly: true }, + }); + mockGetMCPServerTools.mockImplementation(async (_userId: string, server: string) => ({ + [`tool_mcp_${server}`]: {}, + })); + }); + + test('drops a server the chat menu hides', async () => { + const result = await loadEphemeral( + { mcpConfig: {} }, + { + ephemeralAgent: { mcp: ['visible', 'hidden'] }, + }, + ); + + expect(selectedServerNames()).toEqual(['visible']); + expect(result?.tools).toContain('tool_mcp_visible'); + expect(result?.tools).not.toContain('tool_mcp_hidden'); + }); + + test('drops a server the user only reaches through an agent', async () => { + const result = await loadEphemeral( + { mcpConfig: {} }, + { + ephemeralAgent: { mcp: ['visible', 'agent-only'] }, + }, + ); + + expect(selectedServerNames()).toEqual(['visible']); + expect(result?.tools).not.toContain('tool_mcp_agent-only'); + }); + + test('resolves the accessible catalog with the requesting user role', async () => { + await loadEphemeral({ mcpConfig: {} }, { ephemeralAgent: { mcp: ['visible'] } }); + expect(mockGetAccessibleMCPServers).toHaveBeenCalledWith('user123', 'USER'); + }); + + test('keeps a spec-pinned server even when the chat menu hides it', async () => { + const result = await loadEphemeral( + { + mcpConfig: {}, + modelSpecs: { + list: [ + { + name: 'pins-hidden', + label: 'Pins Hidden', + preset: { endpoint: 'openai', model: 'gpt-4' }, + mcpServers: ['hidden'], + }, + ], + }, + }, + { ephemeralAgent: { mcp: [] } }, + 'pins-hidden', + ); + + expect(selectedServerNames()).toEqual(['hidden']); + expect(result?.tools).toContain('tool_mcp_hidden'); + }); + + test('keeps a request-tier server the registry cannot resolve', async () => { + const result = await loadEphemeral( + { mcpConfig: {} }, + { + ephemeralAgent: { mcp: ['body-scoped'] }, + }, + ); + + expect(selectedServerNames()).toEqual(['body-scoped']); + expect(result?.tools).toContain('tool_mcp_body-scoped'); + }); + + test('keeps the selection when the catalog lookup fails', async () => { + mockGetAccessibleMCPServers.mockRejectedValue(new Error('registry unavailable')); + + const result = await loadEphemeral( + { mcpConfig: {} }, + { + ephemeralAgent: { mcp: ['flaky'] }, + }, + ); + + expect(selectedServerNames()).toEqual(['flaky']); + expect(result?.tools).toContain('tool_mcp_flaky'); + }); + + test('publishes the servers actually in play back onto the request body', async () => { + const body = { ephemeralAgent: { mcp: ['visible', 'hidden'] } }; + + await loadEphemeral( + { + mcpConfig: {}, + modelSpecs: { + list: [ + { + name: 'pins-hidden', + label: 'Pins Hidden', + preset: { endpoint: 'openai', model: 'gpt-4' }, + mcpServers: ['hidden'], + }, + ], + }, + }, + body, + 'pins-hidden', + ); + + /** `applyContextToAgent` reads this straight off the body and prefers it + * over the agent's tools when loading `serverInstructions`: the hidden + * pick is gone, the spec's pin stays. */ + expect(body.ephemeralAgent.mcp).toEqual(['visible', 'hidden']); + }); + }); + test('addresses cached tools with a non-ephemeral request overlay', async () => { const { EPHEMERAL_AGENT_ID } = Constants; const overlayConfig = { diff --git a/packages/api/src/agents/added.ts b/packages/api/src/agents/added.ts index 192b48d18c..b08fe163d3 100644 --- a/packages/api/src/agents/added.ts +++ b/packages/api/src/agents/added.ts @@ -11,7 +11,11 @@ import { import type { Agent, AgentToolOptions, TConversation, TModelSpec } from 'librechat-data-provider'; import type { AppConfig } from '@librechat/data-schemas'; import type { ParsedServerConfig } from '~/mcp/types'; -import { requiresEphemeralUserConnection, validateMCPServerConfig } from '~/mcp/utils'; +import { + requiresEphemeralUserConnection, + filterChatSelectableMCPServers, + validateMCPServerConfig, +} from '~/mcp/utils'; import { ASK_USER_QUESTION_TOOL_NAME } from '~/agents/hitl/askUserQuestionTool'; import { synthesizeBackgroundToolOptions } from '~/agents/background'; import { mergeSynthesizedToolOptions } from '~/agents/selection'; @@ -57,10 +61,17 @@ export interface LoadAddedAgentDeps { serverName: string, serverConfig?: ParsedServerConfig, ) => Promise | null>; + /** The MCP servers this user can reach, with the registry's tier precedence + * already applied — the resolution behind the client's catalog. Omitted, the + * chat selection is used as sent. */ + getAccessibleMCPServers?: ( + userId: string, + role?: string, + ) => Promise>; } interface LoadAddedAgentParams { - req: { user?: { id?: string }; config?: Record }; + req: { user?: { id?: string; role?: string }; config?: Record }; conversation: TConversation | null; primaryAgent?: Agent | null; } @@ -182,8 +193,16 @@ export async function loadAddedAgent( return result as unknown as Agent; } - const mcpServers = new Set(ephemeralAgent?.mcp); const userId = req.user?.id ?? ''; + /** Narrowed like the primary ephemeral loader: picker selection only, spec + * servers added below. */ + const mcpServers = new Set( + await filterChatSelectableMCPServers(ephemeralAgent?.mcp, { + userId, + role: req.user?.role, + getAccessibleMCPServers: deps.getAccessibleMCPServers, + }), + ); const modelSpecs = (appConfig?.modelSpecs as { list?: TModelSpec[] })?.list; let modelSpec: (typeof modelSpecs extends Array | undefined ? T : never) | null = null; diff --git a/packages/api/src/agents/load.ts b/packages/api/src/agents/load.ts index af981b8187..569a9caba8 100644 --- a/packages/api/src/agents/load.ts +++ b/packages/api/src/agents/load.ts @@ -16,7 +16,11 @@ import type { } from 'librechat-data-provider'; import type { AppConfig } from '@librechat/data-schemas'; import type { ParsedServerConfig } from '~/mcp/types'; -import { requiresEphemeralUserConnection, validateMCPServerConfig } from '~/mcp/utils'; +import { + requiresEphemeralUserConnection, + filterChatSelectableMCPServers, + validateMCPServerConfig, +} from '~/mcp/utils'; import { ASK_USER_QUESTION_TOOL_NAME } from '~/agents/hitl/askUserQuestionTool'; import { synthesizeBackgroundToolOptions } from '~/agents/background'; import { mergeSynthesizedToolOptions } from '~/agents/selection'; @@ -33,11 +37,18 @@ export interface LoadAgentDeps { serverName: string, serverConfig?: ParsedServerConfig, ) => Promise | null>; + /** The MCP servers this user can reach, with the registry's tier precedence + * already applied — the resolution behind the client's catalog. Omitted, the + * chat selection is used as sent. */ + getAccessibleMCPServers?: ( + userId: string, + role?: string, + ) => Promise>; } export interface LoadAgentParams { req: { - user?: { id?: string }; + user?: { id?: string; role?: string }; config?: AppConfig; body?: { promptPrefix?: string; @@ -64,13 +75,29 @@ export async function loadEphemeralAgent( modelSpec = modelSpecs?.list?.find((s) => s.name === spec) ?? null; } const ephemeralAgent: TEphemeralAgent | undefined = req.body?.ephemeralAgent; - const mcpServers = new Set(ephemeralAgent?.mcp); const userId = req.user?.id ?? ''; + /** The picker's own selection is narrowed to what the picker may offer; a + * spec's servers are the operator's choice and are added after, so pinning a + * chat-hidden server to a spec keeps working. */ + const mcpServers = new Set( + await filterChatSelectableMCPServers(ephemeralAgent?.mcp, { + userId, + role: req.user?.role, + getAccessibleMCPServers: deps.getAccessibleMCPServers, + }), + ); if (modelSpec?.mcpServers) { for (const mcpServer of modelSpec.mcpServers) { mcpServers.add(mcpServer); } } + /** Publish the servers this request will actually use back onto the body. The + * instruction path reads `req.body.ephemeralAgent.mcp` directly and prefers + * it over the agent's tools, so it would otherwise both inject a hidden + * server's `serverInstructions` and omit a spec-pinned server's. */ + if (ephemeralAgent != null && Array.isArray(ephemeralAgent.mcp)) { + ephemeralAgent.mcp = [...mcpServers]; + } const tools: string[] = []; if (ephemeralAgent?.execute_code === true || modelSpec?.executeCode === true) { tools.push(Tools.execute_code); diff --git a/packages/api/src/mcp/__tests__/utils.test.ts b/packages/api/src/mcp/__tests__/utils.test.ts index 5c212fdff9..bd917ef224 100644 --- a/packages/api/src/mcp/__tests__/utils.test.ts +++ b/packages/api/src/mcp/__tests__/utils.test.ts @@ -20,6 +20,8 @@ import { isUserSourced, validateMCPServerConfig, requiresEphemeralUserConnection, + isChatSelectableMCPServer, + filterChatSelectableMCPServers, } from '~/mcp/utils'; describe('normalizeServerName', () => { @@ -1092,3 +1094,138 @@ describe('getMissingCustomUserVars', () => { ).toEqual([]); }); }); + +describe('isChatSelectableMCPServer', () => { + it('rejects a server hidden from the chat menu', () => { + expect(isChatSelectableMCPServer({ chatMenu: false })).toBe(false); + }); + + it('rejects a server reachable only through an agent', () => { + expect(isChatSelectableMCPServer({ consumeOnly: true })).toBe(false); + }); + + it('accepts a server with the flags unset or explicitly on', () => { + expect(isChatSelectableMCPServer({})).toBe(true); + expect(isChatSelectableMCPServer({ chatMenu: true, consumeOnly: false })).toBe(true); + }); + + it('accepts an unresolved config so request-tier servers are not dropped', () => { + expect(isChatSelectableMCPServer(undefined)).toBe(true); + expect(isChatSelectableMCPServer(null)).toBe(true); + }); +}); + +describe('filterChatSelectableMCPServers', () => { + const catalog = { + visible: { chatMenu: true }, + hidden: { chatMenu: false }, + unset: {}, + 'agent-only': { consumeOnly: true }, + 'private server': { chatMenu: false }, + }; + const accessible = jest.fn(async () => catalog); + + beforeEach(() => accessible.mockClear()); + + it('drops servers hidden from the chat menu', async () => { + await expect( + filterChatSelectableMCPServers(['visible', 'hidden'], { + userId: 'user123', + getAccessibleMCPServers: accessible, + }), + ).resolves.toEqual(['visible']); + }); + + it('drops servers the user only reaches through an agent', async () => { + await expect( + filterChatSelectableMCPServers(['visible', 'agent-only'], { + userId: 'user123', + getAccessibleMCPServers: accessible, + }), + ).resolves.toEqual(['visible']); + }); + + it('keeps servers with the flags unset', async () => { + await expect( + filterChatSelectableMCPServers(['unset'], { + userId: 'user123', + getAccessibleMCPServers: accessible, + }), + ).resolves.toEqual(['unset']); + }); + + it('resolves the catalog once, however long the selection', async () => { + await expect( + filterChatSelectableMCPServers(['visible', 'visible', 'unset', 'visible'], { + userId: 'user123', + getAccessibleMCPServers: accessible, + }), + ).resolves.toEqual(['visible', 'unset']); + expect(accessible).toHaveBeenCalledTimes(1); + }); + + it('passes the role through, since it scopes what the user can reach', async () => { + await filterChatSelectableMCPServers(['visible'], { + userId: 'user123', + role: 'ADMIN', + getAccessibleMCPServers: accessible, + }); + expect(accessible).toHaveBeenCalledWith('user123', 'ADMIN'); + }); + + it('drops a hidden server named by its normalized spelling', async () => { + await expect( + filterChatSelectableMCPServers(['private_server'], { + userId: 'user123', + getAccessibleMCPServers: accessible, + }), + ).resolves.toEqual([]); + }); + + it('lets an exact accessible name win over another server normalizing onto it', async () => { + const crossTier = jest.fn(async () => ({ + 'private server': { chatMenu: false }, + private_server: { chatMenu: true }, + })); + await expect( + filterChatSelectableMCPServers(['private_server'], { + userId: 'user123', + getAccessibleMCPServers: crossTier, + }), + ).resolves.toEqual(['private_server']); + }); + + it('keeps a request-tier server the registry does not know', async () => { + await expect( + filterChatSelectableMCPServers(['body-scoped'], { + userId: 'user123', + getAccessibleMCPServers: accessible, + }), + ).resolves.toEqual(['body-scoped']); + }); + + it('keeps the selection when the catalog lookup fails', async () => { + const failing = jest.fn(async () => { + throw new Error('registry unavailable'); + }); + await expect( + filterChatSelectableMCPServers(['a', 'b'], { + userId: 'user123', + getAccessibleMCPServers: failing, + }), + ).resolves.toEqual(['a', 'b']); + }); + + it('uses the selection as sent when no resolver is supplied', async () => { + await expect( + filterChatSelectableMCPServers(['a', 'a', 'b'], { userId: 'user123' }), + ).resolves.toEqual(['a', 'b']); + }); + + it('returns an empty array for an empty or missing selection', async () => { + const opts = { userId: 'u', getAccessibleMCPServers: accessible }; + await expect(filterChatSelectableMCPServers([], opts)).resolves.toEqual([]); + await expect(filterChatSelectableMCPServers(undefined, opts)).resolves.toEqual([]); + await expect(filterChatSelectableMCPServers(null, opts)).resolves.toEqual([]); + }); +}); diff --git a/packages/api/src/mcp/utils.ts b/packages/api/src/mcp/utils.ts index 9910def2cc..aeb3faed47 100644 --- a/packages/api/src/mcp/utils.ts +++ b/packages/api/src/mcp/utils.ts @@ -1,3 +1,4 @@ +import { logger } from '@librechat/data-schemas'; import { Constants, MCPOptionsSchema, @@ -403,6 +404,98 @@ export function requiresEphemeralUserConnection(config: UserScopedConnectionConf return getMCPRequestScope(config).requestScoped; } +/** + * Whether a resolved server config may be reached from the chat MCP picker. + * + * Mirrors `selectableServers` in the client's `useMCPServerManager`, which is + * the list the dropdown offers: `chatMenu: false` is the operator hiding a + * server from chat, and `consumeOnly` marks a server the user reaches only + * through an agent that references it, never on its own. + * + * An unresolved config is selectable. A name the registry cannot resolve is + * either request-tier (declared on the request body and never registered) or + * genuinely unknown, and both already have their own handling downstream — + * failing closed here would silently drop request-scoped servers instead. + */ +export function isChatSelectableMCPServer( + config?: Pick | null, +): boolean { + if (config == null) { + return true; + } + return config.chatMenu !== false && config.consumeOnly !== true; +} + +/** + * Narrows a chat picker selection to the servers that picker is allowed to + * offer, so a stale client, a replayed body, or a hand-written request cannot + * reach a server the menu hides. + * + * Pass only the picker's own selection. Servers a model spec pins + * (`modelSpec.mcpServers`) are the operator's choice and stay attached even + * when hidden, so they must be added after this call, not through it. + * + * The accessible set comes from the registry — the same resolution that feeds + * the client's catalog, with its own tier precedence already applied — rather + * than being re-derived from the request's config overlay. A user-tier or + * process-backed server outranks a config entry of the same name, and this must + * agree with whatever the picker was actually offered. + * + * Names are deduplicated before the lookup, since the selection is + * request-supplied, and are matched through the same normalized-name aliasing + * that tool loading uses, built over the whole accessible set so an exact name + * always wins over another server's normalized form. A name the registry does + * not know is request-tier — declared on the body, never registered — and is + * kept, as is every name if the lookup fails: this narrows an + * already-authenticated selection and is not the authorization boundary. + * + * Returns the names as they were sent, so callers keep addressing servers the + * way the rest of the request does. + */ +export async function filterChatSelectableMCPServers( + selectedServers: string[] | null | undefined, + { + userId, + role, + getAccessibleMCPServers, + }: { + userId: string; + role?: string; + getAccessibleMCPServers?: ( + userId: string, + role?: string, + ) => Promise>>; + }, +): Promise { + if (!Array.isArray(selectedServers) || selectedServers.length === 0) { + return []; + } + const uniqueServers = [...new Set(selectedServers)]; + if (getAccessibleMCPServers == null) { + return uniqueServers; + } + + let accessible: Record>; + try { + accessible = await getAccessibleMCPServers(userId, role); + } catch (error) { + logger.warn('[MCP] Could not resolve accessible servers; keeping the chat selection', error); + return uniqueServers; + } + + const accessibleNames = Object.keys(accessible ?? {}); + if (accessibleNames.length === 0) { + return uniqueServers; + } + const aliases = buildServerNameAliases(accessibleNames); + + return uniqueServers.filter((serverName) => { + const resolved = + accessible[serverName] ?? accessible[aliases.get(normalizeServerName(serverName)) ?? '']; + return isChatSelectableMCPServer(resolved); + }); +} + /** * Returns true when a server requires a per-user connection instead of an * app-shared connection. diff --git a/packages/data-provider/src/mcp.ts b/packages/data-provider/src/mcp.ts index 8d3309667c..50e9dae2b1 100644 --- a/packages/data-provider/src/mcp.ts +++ b/packages/data-provider/src/mcp.ts @@ -178,7 +178,14 @@ const BaseOptionsSchema = z.object({ /** Timeout (ms) for the long-lived SSE GET stream body before undici aborts it. Default: 300_000 (5 min). */ sseReadTimeout: z.number().int().positive().optional(), initTimeout: z.number().int().nonnegative().optional(), - /** Controls visibility in chat dropdown menu (MCPSelect) */ + /** + * Whether the server is offered in chat. + * + * `false` hides it from the chat dropdown (MCPSelect) AND bars it from the + * chat selection a request carries, so a stale or hand-written request cannot + * reach it either. It does not restrict agents, nor a server a model spec + * pins through `mcpServers` — both are the operator's own choice. + */ chatMenu: z.boolean().optional(), /** * Controls server instruction behavior: