From 100871c3ec88e1eab2908d95fdc2efd10261a623 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 30 May 2026 16:19:49 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=82=20fix:=20Enforce=20MCP=20Permissio?= =?UTF-8?q?ns=20for=20Agent=20Tools=20(#13174)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: Enforce MCP Permissions for Agent Tools * fix: Measure MCP Image Limit by Decoded Size * fix: gate cached MCP tools and tighten remote image URL detection Addresses Codex review findings on the MCP permissions PR: - filterAuthorizedTools previously fast-accepted any tool present in the global tool cache before reaching the MCP-use permission gate. App-level MCP tools (keyed `name_mcp_server` by MCPServerInspector and merged into the cache via mergeAppTools) therefore bypassed the canUseMCP check, letting a user without MCP_SERVERS.USE persist/bind them. Route all MCP-delimited tools through the permission + server-access gate regardless of cache presence. - assertImageDataWithinLimit / image formatter used startsWith("http") to skip the size cap, which also matched base64 payloads that happen to begin with those chars. Require http:// or https:// via a shared isRemoteImageUrl helper so oversized inline base64 can no longer bypass MCP_IMAGE_DATA_MAX_BYTES. Adds regression tests for both paths. * fix: address Codex round-2 findings on MCP permissions PR - parsers.ts: parseAsString dropped the image payload for unrecognized providers, returning only `Image result: `. Pre-PR these items survived via JSON.stringify(item). Keep the size guard but fall through to JSON.stringify so the data/URL is preserved. - MCP.js: the runtime MCP-use check only read `configurable.user`, so paths that propagate `user_id` only (e.g. the OpenAI-compatible API in agents/openai/service.ts) rejected every MCP tool call for an authenticated user. Add resolveMCPPermissionUser: use the safe user directly when it already carries a role (no extra DB call), otherwise fall back to loading the role by user_id. Update fail-closed tests to the resolved behavior. - v1.js: the update path only re-filtered newly added MCP tools, so a user who lost MCP_SERVERS.USE kept existing MCP bindings on edit while create/duplicate/revert stripped them. Strip all MCP tools on update when the permission is revoked; keep the narrower new-tool gating (and disconnect/registry preservation) when it is intact. Updates and adds regression tests for all three paths. * fix: populate safe user at producer instead of resolving in runtime MCP check Corrects the Finding B approach from the previous commit. Rather than loading the user by id inside the runtime MCP permission check, populate `configurable.user` (and createRun's `user`) with the full safe user at the producer, matching the in-repo agent controllers (responses.js / openai.js) which already pass `createSafeUser(req.user)`. - service.ts: derive `safeUser` via createSafeUser(req.user) and pass it to both createRun and processStream's configurable, so the role-bearing identity reaches the runtime `userCanUseMCPServers(configurable.user)` check. Falls back to a bare id when the host app attached no user, which correctly leaves MCP gated (fail closed). - MCP.js: revert the resolveMCPPermissionUser DB-load fallback; the runtime check again reads configurable.user directly and fails closed when absent (defense in depth). - MCP.spec.js: revert to the matching runtime test expectations. * test: cover safe-user propagation in createAgentChatCompletion Adds a focused spec for the OpenAI-compatible chat completion service (the producer fixed for Codex Finding B). Injects mocked deps and asserts that createRun and processStream's configurable.user carry the role from req.user (with sensitive fields stripped by createSafeUser), and that an unauthenticated request falls back to a bare { id: 'api-user' } so the runtime MCP check fails closed. * fix: address Codex round-3 findings + TS6133 - MCP.js (P1): the assistants required-action path invokes tool._call( toolInput) with no LangChain config, so the runtime check saw no configurable.user and rejected authorized users. createToolInstance now captures the creation-time user (req.user via createMCPTool) and _call falls back to it for both the permission check and userId. Still fails closed when neither config nor captured user carries a role. - v1.js (P2): the update-path isMCPTool used a bare mcp_delimiter substring check, misclassifying action tools whose operationId contains "_mcp_" (e.g. sync_mcp_state_action_...) as MCP and dropping them on a permission-revoked edit. Delegate to the canonical isActionTool so only real MCP tools are gated. Regression test added. - service.ts: drop the now-unused IUser import (TS6133); derive reqUser's type from createSafeUser's own parameter instead. * fix: resolve TS7022 self-reference in service.spec mock res The mock response object referenced `res` inside its own `status`/`json` initializers without a type annotation, so tsc inferred `res` as `any` (TS7022). Annotate the object and assign the self-referencing chainable methods after declaration. * fix: correct round-4 findings (isActionTool import, captured user, partial-update) - v1.js: import isActionTool from librechat-data-provider (its real export; @librechat/api does not export it, so the prior import was undefined and threw TypeError). Exclude action tools from MCP classification in both the main filterAuthorizedTools loop and the update path, so action tools whose operationId contains _mcp_ (e.g. sync_mcp_state_action_...) are preserved regardless of MCP permission. - v1.js: evaluate the effective tool set (updateData.tools ?? existingAgent.tools) so a tools-less PATCH by a user who lost MCP_SERVERS.USE still strips stale MCP bindings, matching create/duplicate/revert. - MCP.js: createToolInstance now receives the construction-time user and _call falls back to it (permissionUser) when configurable.user is absent, fixing the assistants required-action path that invokes _call without a config and resolving the capturedUser no-undef/ReferenceError. - Tests: action-tool preservation (authorized + denied), tools-less revocation PATCH, updated revocation test to expect all MCP tools stripped. Affected specs pass locally: MCP 49/49, filterAuthorizedTools 49/49. * fix: guard isActionTool against non-string tools; correct actionDelimiter import Two test regressions from the prior commit: - The main filterAuthorizedTools loop called isActionTool(tool) directly, but isActionTool does toolName.indexOf(...) and throws on null/undefined. Compute isActionToolName = typeof tool === 'string' && isActionTool(tool) once and reuse it, restoring graceful null/undefined handling. - The action-tool test referenced Constants.actionDelimiter (undefined); actionDelimiter is a standalone librechat-data-provider export. Import and use it directly. filterAuthorizedTools 36/36 and MCP 40/40 pass locally. * fix: address MCP permission review follow-ups * fix: preserve shared agent MCP tools --- api/app/clients/tools/util/handleTools.js | 22 +- .../agents/filterAuthorizedTools.spec.js | 230 +++++++++++++++++- api/server/controllers/agents/v1.js | 92 +++++-- api/server/routes/__tests__/mcp.spec.js | 22 +- api/server/routes/mcp.js | 2 +- api/server/services/MCP.js | 44 +++- api/server/services/MCP.spec.js | 53 +++- api/server/services/ToolService.js | 19 +- .../services/__tests__/ToolService.spec.js | 23 ++ .../api/src/agents/openai/service.spec.ts | 107 ++++++++ packages/api/src/agents/openai/service.ts | 16 +- .../api/src/mcp/__tests__/parsers.test.ts | 99 ++++++++ packages/api/src/mcp/parsers.ts | 53 +++- .../data-schemas/src/methods/agent.spec.ts | 38 +++ packages/data-schemas/src/methods/agent.ts | 10 +- 15 files changed, 786 insertions(+), 44 deletions(-) create mode 100644 packages/api/src/agents/openai/service.spec.ts diff --git a/api/app/clients/tools/util/handleTools.js b/api/app/clients/tools/util/handleTools.js index 89a79f3cbd..4577d8a9ea 100644 --- a/api/app/clients/tools/util/handleTools.js +++ b/api/app/clients/tools/util/handleTools.js @@ -35,7 +35,12 @@ const { createGeminiImageTool, createOpenAIImageTools, } = require('../'); -const { createMCPTool, createMCPTools, resolveConfigServers } = require('~/server/services/MCP'); +const { + createMCPTool, + createMCPTools, + resolveConfigServers, + userCanUseMCPServers, +} = require('~/server/services/MCP'); const { createFileSearchTool, primeFiles: primeSearchFiles } = require('./fileSearch'); const { primeFiles: primeCodeFiles } = require('~/server/services/Files/Code/process'); const { getUserPluginAuthValue } = require('~/server/services/PluginService'); @@ -227,6 +232,9 @@ const loadTools = async ({ }; const requestedTools = {}; + const hasMCPTools = tools.some((toolName) => toolName && mcpToolPattern.test(toolName)); + const canUseMCP = hasMCPTools ? await userCanUseMCPServers(options.req?.user) : true; + let loggedMCPDenied = false; if (functions === true) { toolConstructors.dalle = DALLE3; @@ -266,7 +274,7 @@ const loadTools = async ({ /** Resolve config-source servers for the current user/tenant context */ let configServers; - if (tools.some((tool) => tool && mcpToolPattern.test(tool))) { + if (hasMCPTools && canUseMCP) { configServers = await resolveConfigServers(options.req); } @@ -345,6 +353,16 @@ const loadTools = async ({ }; continue; } else if (tool && mcpToolPattern.test(tool)) { + if (!canUseMCP) { + if (!loggedMCPDenied) { + logger.warn( + `[handleTools] User ${options.req?.user?.id} lacks MCP server use permission`, + ); + loggedMCPDenied = true; + } + continue; + } + const [toolName, serverName] = tool.split(Constants.mcp_delimiter); if (toolName === Constants.mcp_server) { /** Placeholder used for UI purposes */ diff --git a/api/server/controllers/agents/filterAuthorizedTools.spec.js b/api/server/controllers/agents/filterAuthorizedTools.spec.js index e6b41aef16..4bdb870524 100644 --- a/api/server/controllers/agents/filterAuthorizedTools.spec.js +++ b/api/server/controllers/agents/filterAuthorizedTools.spec.js @@ -1,12 +1,13 @@ const mongoose = require('mongoose'); const { v4: uuidv4 } = require('uuid'); -const { Constants } = require('librechat-data-provider'); +const { Constants, actionDelimiter } = require('librechat-data-provider'); const { agentSchema } = require('@librechat/data-schemas'); const { MongoMemoryServer } = require('mongodb-memory-server'); const d = Constants.mcp_delimiter; const mockGetAllServerConfigs = jest.fn(); +const mockUserCanUseMCPServers = jest.fn(); jest.mock('~/server/services/Config', () => ({ getCachedTools: jest.fn().mockResolvedValue({ @@ -24,6 +25,7 @@ jest.mock('~/config', () => ({ jest.mock('~/server/services/MCP', () => ({ resolveConfigServers: jest.fn().mockResolvedValue({}), + userCanUseMCPServers: (...args) => mockUserCanUseMCPServers(...args), })); jest.mock('~/server/services/Files/strategies', () => ({ @@ -106,6 +108,7 @@ describe('MCP Tool Authorization', () => { authorizedServer: { type: 'sse', url: 'https://authorized.example.com' }, anotherServer: { type: 'sse', url: 'https://another.example.com' }, }); + mockUserCanUseMCPServers.mockResolvedValue(true); mockReq = { user: { @@ -127,11 +130,13 @@ describe('MCP Tool Authorization', () => { describe('filterAuthorizedTools', () => { const availableTools = { web_search: true, custom_tool: true }; const userId = 'test-user-123'; + const testUser = { id: userId, role: 'USER' }; test('should keep authorized MCP tools and strip unauthorized ones', async () => { const result = await filterAuthorizedTools({ tools: [`toolA${d}authorizedServer`, `toolB${d}forbiddenServer`, 'web_search'], userId, + user: testUser, availableTools, }); @@ -140,6 +145,39 @@ describe('MCP Tool Authorization', () => { expect(result).not.toContain(`toolB${d}forbiddenServer`); }); + test('should strip MCP tools when user lacks MCP server use permission', async () => { + mockUserCanUseMCPServers.mockResolvedValue(false); + + const result = await filterAuthorizedTools({ + tools: [ + `toolA${d}authorizedServer`, + `${Constants.mcp_all}${d}authorizedServer`, + 'web_search', + ], + userId, + user: testUser, + availableTools, + }); + + expect(result).toEqual(['web_search']); + expect(mockUserCanUseMCPServers).toHaveBeenCalledWith({ id: userId, role: 'USER' }); + expect(mockGetAllServerConfigs).not.toHaveBeenCalled(); + }); + + test('should strip MCP tools when user context is missing', async () => { + mockUserCanUseMCPServers.mockResolvedValueOnce(false); + + const result = await filterAuthorizedTools({ + tools: [`toolA${d}authorizedServer`, 'web_search'], + userId, + availableTools, + }); + + expect(result).toEqual(['web_search']); + expect(mockUserCanUseMCPServers).toHaveBeenCalledWith(undefined); + expect(mockGetAllServerConfigs).not.toHaveBeenCalled(); + }); + test('should keep system tools without querying MCP registry', async () => { const result = await filterAuthorizedTools({ tools: ['execute_code', 'file_search', 'web_search'], @@ -170,6 +208,7 @@ describe('MCP Tool Authorization', () => { const result = await filterAuthorizedTools({ tools: [`toolA${d}someServer`, 'web_search'], userId, + user: testUser, availableTools, }); @@ -188,6 +227,7 @@ describe('MCP Tool Authorization', () => { `steal${d}nonexistent`, ], userId, + user: testUser, availableTools, }); @@ -224,6 +264,7 @@ describe('MCP Tool Authorization', () => { await filterAuthorizedTools({ tools: [`tool${d}authorizedServer`], userId: 'specific-user-id', + user: { id: 'specific-user-id', role: 'USER' }, availableTools, }); @@ -241,6 +282,7 @@ describe('MCP Tool Authorization', () => { const result = await filterAuthorizedTools({ tools: [`tool${d}config-override-server`, `tool${d}unauthorizedServer`], userId, + user: testUser, availableTools, configServers, }); @@ -254,6 +296,7 @@ describe('MCP Tool Authorization', () => { await filterAuthorizedTools({ tools: [`tool1${d}authorizedServer`, `tool2${d}anotherServer`, `tool3${d}unknownServer`], userId, + user: testUser, availableTools, }); @@ -270,6 +313,7 @@ describe('MCP Tool Authorization', () => { const result = await filterAuthorizedTools({ tools: [...existingTools, `newTool${d}unknownServer`, 'web_search'], userId, + user: testUser, availableTools, existingTools, }); @@ -288,6 +332,7 @@ describe('MCP Tool Authorization', () => { const result = await filterAuthorizedTools({ tools: [`toolA${d}serverA`, 'web_search'], userId, + user: testUser, availableTools, }); @@ -303,6 +348,7 @@ describe('MCP Tool Authorization', () => { const result = await filterAuthorizedTools({ tools: [malformedTool, `legit${d}serverA`, 'web_search'], userId, + user: testUser, availableTools, existingTools: [malformedTool, `legit${d}serverA`], }); @@ -312,6 +358,43 @@ describe('MCP Tool Authorization', () => { expect(result).not.toContain(malformedTool); }); + test('should gate app-level MCP tools present in the global tool cache', async () => { + const appMcpTool = `appTool${d}authorizedServer`; + const forbiddenAppMcpTool = `appTool${d}forbiddenServer`; + const cacheWithMCPTools = { + ...availableTools, + [appMcpTool]: true, + [forbiddenAppMcpTool]: true, + }; + + const result = await filterAuthorizedTools({ + tools: [appMcpTool, forbiddenAppMcpTool, 'web_search'], + userId, + user: testUser, + availableTools: cacheWithMCPTools, + }); + + expect(result).toContain(appMcpTool); + expect(result).toContain('web_search'); + expect(result).not.toContain(forbiddenAppMcpTool); + }); + + test('should strip app-level MCP tools from the cache when user lacks MCP server use permission', async () => { + mockUserCanUseMCPServers.mockResolvedValue(false); + const appMcpTool = `appTool${d}authorizedServer`; + const cacheWithMCPTools = { ...availableTools, [appMcpTool]: true }; + + const result = await filterAuthorizedTools({ + tools: [appMcpTool, 'web_search'], + userId, + user: testUser, + availableTools: cacheWithMCPTools, + }); + + expect(result).toEqual(['web_search']); + expect(mockGetAllServerConfigs).not.toHaveBeenCalled(); + }); + test('should reject malformed MCP tool keys with multiple delimiters', async () => { const result = await filterAuthorizedTools({ tools: [ @@ -321,6 +404,7 @@ describe('MCP Tool Authorization', () => { 'web_search', ], userId, + user: testUser, availableTools, }); @@ -348,6 +432,27 @@ describe('MCP Tool Authorization', () => { expect(agent.tools).not.toContain(`attack${d}forbiddenServer`); }); + test('should strip all MCP tools on create when user lacks MCP server use permission', async () => { + mockUserCanUseMCPServers.mockResolvedValue(false); + mockReq.body = { + provider: 'openai', + model: 'gpt-4', + name: 'MCP Denied Test Agent', + tools: [ + 'web_search', + `validTool${d}authorizedServer`, + `${Constants.mcp_all}${d}authorizedServer`, + ], + }; + + await createAgentHandler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(201); + const agent = mockRes.json.mock.calls[0][0]; + expect(agent.tools).toEqual(['web_search']); + expect(agent.mcpServerNames).toEqual([]); + }); + test('should not 500 when MCP registry is uninitialized', async () => { getMCPServersRegistry.mockImplementation(() => { throw new Error('MCPServersRegistry has not been initialized.'); @@ -446,6 +551,129 @@ describe('MCP Tool Authorization', () => { expect(updatedAgent.tools).not.toContain(`attack${d}forbiddenServer`); }); + test('should strip all MCP tools, including retained ones, when user lacks MCP server use permission', async () => { + mockUserCanUseMCPServers.mockResolvedValue(false); + mockReq.user.id = existingAgentAuthorId.toString(); + mockReq.params.id = existingAgentId; + mockReq.body = { + tools: ['web_search', `existingTool${d}authorizedServer`, `newTool${d}anotherServer`], + }; + + await updateAgentHandler(mockReq, mockRes); + + expect(mockRes.json).toHaveBeenCalled(); + const updatedAgent = mockRes.json.mock.calls[0][0]; + // Permission revoked: update must not preserve stale MCP bindings, matching + // the create/duplicate/revert paths. + expect(updatedAgent.tools).toEqual(['web_search']); + expect(mockGetAllServerConfigs).not.toHaveBeenCalled(); + }); + + test('should strip retained MCP tools on an unrelated owner edit after permission revocation', async () => { + mockUserCanUseMCPServers.mockResolvedValue(false); + mockReq.user.id = existingAgentAuthorId.toString(); + mockReq.params.id = existingAgentId; + mockReq.body = { + name: 'Renamed After Revocation', + }; + + await updateAgentHandler(mockReq, mockRes); + + expect(mockRes.json).toHaveBeenCalled(); + const updatedAgent = mockRes.json.mock.calls[0][0]; + expect(updatedAgent.tools).toEqual(['web_search']); + expect(updatedAgent.name).toBe('Renamed After Revocation'); + }); + + test('should not strip shared agent MCP tools on unrelated editor changes after revocation', async () => { + mockUserCanUseMCPServers.mockResolvedValue(false); + mockReq.user.id = new mongoose.Types.ObjectId().toString(); + mockReq.params.id = existingAgentId; + mockReq.body = { + name: 'Shared Rename After Revocation', + }; + + await updateAgentHandler(mockReq, mockRes); + + expect(mockRes.json).toHaveBeenCalled(); + const updatedAgent = mockRes.json.mock.calls[0][0]; + const agentInDb = await Agent.findOne({ id: existingAgentId }); + expect(updatedAgent.tools).toContain(`existingTool${d}authorizedServer`); + expect(updatedAgent.name).toBe('Shared Rename After Revocation'); + expect(agentInDb.tools).toContain(`existingTool${d}authorizedServer`); + expect(agentInDb.mcpServerNames).toEqual(['authorizedServer']); + }); + + test('should not strip shared agent MCP tools on frontend-style full tools save after revocation', async () => { + mockUserCanUseMCPServers.mockResolvedValue(false); + mockReq.user.id = new mongoose.Types.ObjectId().toString(); + mockReq.params.id = existingAgentId; + mockReq.body = { + name: 'Shared Full Save After Revocation', + tools: ['web_search', `existingTool${d}authorizedServer`], + }; + + await updateAgentHandler(mockReq, mockRes); + + expect(mockRes.json).toHaveBeenCalled(); + const updatedAgent = mockRes.json.mock.calls[0][0]; + const agentInDb = await Agent.findOne({ id: existingAgentId }); + expect(updatedAgent.tools).toContain(`existingTool${d}authorizedServer`); + expect(updatedAgent.name).toBe('Shared Full Save After Revocation'); + expect(agentInDb.tools).toContain(`existingTool${d}authorizedServer`); + expect(agentInDb.mcpServerNames).toEqual(['authorizedServer']); + expect(mockGetAllServerConfigs).not.toHaveBeenCalled(); + }); + + test('should reject new shared-agent MCP tools after revocation while retaining existing MCP tools', async () => { + mockUserCanUseMCPServers.mockResolvedValue(false); + mockReq.user.id = new mongoose.Types.ObjectId().toString(); + mockReq.params.id = existingAgentId; + mockReq.body = { + tools: ['web_search', `existingTool${d}authorizedServer`, `newTool${d}anotherServer`], + }; + + await updateAgentHandler(mockReq, mockRes); + + expect(mockRes.json).toHaveBeenCalled(); + const updatedAgent = mockRes.json.mock.calls[0][0]; + const agentInDb = await Agent.findOne({ id: existingAgentId }); + expect(updatedAgent.tools).toContain(`existingTool${d}authorizedServer`); + expect(updatedAgent.tools).not.toContain(`newTool${d}anotherServer`); + expect(agentInDb.tools).toContain(`existingTool${d}authorizedServer`); + expect(agentInDb.tools).not.toContain(`newTool${d}anotherServer`); + expect(agentInDb.mcpServerNames).toEqual(['authorizedServer']); + expect(mockGetAllServerConfigs).not.toHaveBeenCalled(); + }); + + test('should not strip action tools whose operationId contains the MCP delimiter on revocation', async () => { + // `sync_mcp_state_action_...` contains the `_mcp_` substring but is a + // genuine OpenAPI action tool (isActionTool === true). Losing + // MCP_SERVERS.USE must not drop it — action use is unrelated to MCP. + const actionTool = `sync_mcp_state${actionDelimiter}api---example---com`; + await Agent.updateOne( + { id: existingAgentId }, + { $set: { tools: ['web_search', actionTool] } }, + ); + + mockUserCanUseMCPServers.mockResolvedValue(false); + mockReq.user.id = existingAgentAuthorId.toString(); + mockReq.params.id = existingAgentId; + mockReq.body = { + name: 'Edited Without MCP Permission', + tools: ['web_search', actionTool], + }; + + await updateAgentHandler(mockReq, mockRes); + + expect(mockRes.json).toHaveBeenCalled(); + const updatedAgent = mockRes.json.mock.calls[0][0]; + const agentInDb = await Agent.findOne({ id: existingAgentId }); + expect(updatedAgent.tools).toContain(actionTool); + expect(updatedAgent.tools).toContain('web_search'); + expect(agentInDb.mcpServerNames).toEqual([]); + }); + test('should allow adding authorized MCP tools', async () => { mockReq.user.id = existingAgentAuthorId.toString(); mockReq.params.id = existingAgentId; diff --git a/api/server/controllers/agents/v1.js b/api/server/controllers/agents/v1.js index 95bf5c6246..79ebbbd2ae 100644 --- a/api/server/controllers/agents/v1.js +++ b/api/server/controllers/agents/v1.js @@ -24,6 +24,7 @@ const { AccessRoleIds, PrincipalType, EToolResources, + isActionTool, PermissionBits, actionDelimiter, AgentCapabilities, @@ -42,7 +43,7 @@ const { resizeAvatar } = require('~/server/services/Files/images/avatar'); const { getFileStrategy } = require('~/server/utils/getFileStrategy'); const { filterFile } = require('~/server/services/Files/process'); const { getCachedTools } = require('~/server/services/Config'); -const { resolveConfigServers } = require('~/server/services/MCP'); +const { resolveConfigServers, userCanUseMCPServers } = require('~/server/services/MCP'); const { getMCPServersRegistry } = require('~/config'); const { getLogStores } = require('~/cache'); const db = require('~/models'); @@ -190,6 +191,7 @@ const isSubagentsCapabilityEnabled = (req) => { * @param {string[]} params.tools - Raw tool strings from the request * @param {string} params.userId - Requesting user ID for MCP server access check * @param {string} [params.role] - Requesting user's role for ACL principal resolution + * @param {object} [params.user] - Requesting user for MCP server use permission checks * @param {Record} params.availableTools - Global non-MCP tool cache * @param {string[]} [params.existingTools] - Tools already persisted on the agent document * @param {Record} [params.configServers] - Config-source MCP servers resolved from appConfig overrides @@ -199,6 +201,7 @@ const filterAuthorizedTools = async ({ tools, userId, role, + user, availableTools, existingTools, configServers, @@ -207,14 +210,26 @@ const filterAuthorizedTools = async ({ let mcpServerConfigs; let registryUnavailable = false; const existingToolSet = existingTools?.length ? new Set(existingTools) : null; + const hasMCPTools = tools.some((tool) => tool?.includes(Constants.mcp_delimiter)); + const canUseMCP = hasMCPTools ? await userCanUseMCPServers(user) : true; + let loggedMCPDenied = false; for (const tool of tools) { - if (availableTools[tool] || systemTools[tool]) { - filteredTools.push(tool); + const isActionToolName = typeof tool === 'string' && isActionTool(tool); + const isMCPTool = tool?.includes(Constants.mcp_delimiter) && !isActionToolName; + + if (!isMCPTool) { + if (availableTools[tool] || systemTools[tool] || isActionToolName) { + filteredTools.push(tool); + } continue; } - if (!tool?.includes(Constants.mcp_delimiter)) { + if (!canUseMCP) { + if (!loggedMCPDenied) { + logger.warn(`[filterAuthorizedTools] User ${userId} lacks MCP server use permission`); + loggedMCPDenied = true; + } continue; } @@ -392,6 +407,7 @@ const createAgentHandler = async (req, res) => { tools, userId, role: req.user.role, + user: req.user, availableTools, configServers, }); @@ -611,27 +627,53 @@ const updateAgentHandler = async (req, res) => { }); } - if (updateData.tools) { - const existingToolSet = new Set(existingAgent.tools ?? []); - const newMCPTools = updateData.tools.filter( - (t) => !existingToolSet.has(t) && t?.includes(Constants.mcp_delimiter), - ); + const isMCPTool = (t) => + typeof t === 'string' && t.includes(Constants.mcp_delimiter) && !isActionTool(t); + const hasToolUpdate = updateData.tools !== undefined; + const editingOwnAgent = existingAgent.author?.toString() === req.user.id; + const existingTools = existingAgent.tools ?? []; + const effectiveTools = (hasToolUpdate ? updateData.tools : existingAgent.tools) ?? []; + const requestedMCPTools = effectiveTools.filter(isMCPTool); + const existingMCPTools = existingTools.filter(isMCPTool); - if (newMCPTools.length > 0) { - const [availableTools, configServers] = await Promise.all([ - getCachedTools().then((t) => t ?? {}), - resolveConfigServers(req), - ]); - const approvedNew = await filterAuthorizedTools({ - tools: newMCPTools, - userId: req.user.id, - role: req.user.role, - availableTools, - configServers, - }); - const rejectedSet = new Set(newMCPTools.filter((t) => !approvedNew.includes(t))); - if (rejectedSet.size > 0) { - updateData.tools = updateData.tools.filter((t) => !rejectedSet.has(t)); + if (requestedMCPTools.length > 0 || (hasToolUpdate && existingMCPTools.length > 0)) { + if (!(await userCanUseMCPServers(req.user))) { + if (editingOwnAgent) { + updateData.tools = effectiveTools.filter((t) => !isMCPTool(t)); + } else if (hasToolUpdate) { + const existingMCPToolSet = new Set(existingMCPTools); + const nextTools = updateData.tools.filter( + (t) => !isMCPTool(t) || existingMCPToolSet.has(t), + ); + const nextToolSet = new Set(nextTools); + for (const existingMCPTool of existingMCPTools) { + if (!nextToolSet.has(existingMCPTool)) { + nextTools.push(existingMCPTool); + } + } + updateData.tools = nextTools; + } + } else if (hasToolUpdate) { + const existingToolSet = new Set(existingTools); + const newMCPTools = requestedMCPTools.filter((t) => !existingToolSet.has(t)); + + if (newMCPTools.length > 0) { + const [availableTools, configServers] = await Promise.all([ + getCachedTools().then((t) => t ?? {}), + resolveConfigServers(req), + ]); + const approvedNew = await filterAuthorizedTools({ + tools: newMCPTools, + userId: req.user.id, + role: req.user.role, + user: req.user, + availableTools, + configServers, + }); + const rejectedSet = new Set(newMCPTools.filter((t) => !approvedNew.includes(t))); + if (rejectedSet.size > 0) { + updateData.tools = updateData.tools.filter((t) => !rejectedSet.has(t)); + } } } } @@ -788,6 +830,7 @@ const duplicateAgentHandler = async (req, res) => { tools: newAgentData.tools, userId, role: req.user.role, + user: req.user, availableTools, existingTools: newAgentData.tools, configServers, @@ -1163,6 +1206,7 @@ const revertAgentVersionHandler = async (req, res) => { tools: updatedAgent.tools, userId: req.user.id, role: req.user.role, + user: req.user, availableTools, existingTools: updatedAgent.tools, configServers, diff --git a/api/server/routes/__tests__/mcp.spec.js b/api/server/routes/__tests__/mcp.spec.js index 3a3e6ac796..b86d339dac 100644 --- a/api/server/routes/__tests__/mcp.spec.js +++ b/api/server/routes/__tests__/mcp.spec.js @@ -25,6 +25,7 @@ const mockRegistryInstance = { getAllowedDomains: jest.fn().mockReturnValue(null), getAllowedAddresses: jest.fn().mockReturnValue(null), }; +let mockMCPUseAllowed = true; jest.mock('@librechat/api', () => { const actual = jest.requireActual('@librechat/api'); @@ -46,7 +47,15 @@ jest.mock('@librechat/api', () => { deleteUserTokens: jest.fn(), }, getUserMCPAuthMap: jest.fn(), - generateCheckAccess: jest.fn(() => (req, res, next) => next()), + generateCheckAccess: jest.fn(({ permissionType, permissions }) => (req, res, next) => { + const { PermissionTypes, Permissions } = require('librechat-data-provider'); + const isMCPUseCheck = + permissionType === PermissionTypes.MCP_SERVERS && permissions.includes(Permissions.USE); + if (isMCPUseCheck && !mockMCPUseAllowed) { + return res.status(403).json({ message: 'Forbidden: Insufficient permissions' }); + } + return next(); + }), MCPServersRegistry: { getInstance: () => mockRegistryInstance, }, @@ -175,6 +184,7 @@ describe('MCP Routes', () => { jest.clearAllMocks(); mockResolveAllMcpConfigs.mockResolvedValue({}); mockResolveMcpConfigNames.mockResolvedValue([]); + mockMCPUseAllowed = true; }); describe('GET /:serverName/oauth/initiate', () => { @@ -1957,6 +1967,16 @@ describe('MCP Routes', () => { }); describe('GET /tools', () => { + it('should deny MCP tools when user lacks MCP server use permission', async () => { + mockMCPUseAllowed = false; + + const response = await request(app).get('/api/mcp/tools'); + + expect(response.status).toBe(403); + expect(response.body).toEqual({ message: 'Forbidden: Insufficient permissions' }); + expect(mockResolveAllMcpConfigs).not.toHaveBeenCalled(); + }); + it('should continue returning MCP tools when one server cache lookup fails', async () => { const { Constants } = require('librechat-data-provider'); const { logger } = require('@librechat/data-schemas'); diff --git a/api/server/routes/mcp.js b/api/server/routes/mcp.js index 219f6455dc..09d6b7d49a 100644 --- a/api/server/routes/mcp.js +++ b/api/server/routes/mcp.js @@ -68,7 +68,7 @@ const checkMCPCreate = generateCheckAccess({ * Get all MCP tools available to the user * Returns only MCP tools, completely decoupled from regular LibreChat tools */ -router.get('/tools', requireJwtAuth, async (req, res) => { +router.get('/tools', requireJwtAuth, checkMCPUsePermissions, async (req, res) => { return getMCPTools(req, res); }); diff --git a/api/server/services/MCP.js b/api/server/services/MCP.js index 85c29ddfdc..2d81d66070 100644 --- a/api/server/services/MCP.js +++ b/api/server/services/MCP.js @@ -7,6 +7,7 @@ const { Constants: AgentConstants, } = require('@librechat/agents'); const { + checkAccess, sendEvent, MCPOAuthHandler, isMCPDomainAllowed, @@ -16,14 +17,22 @@ const { resolveJsonSchemaRefs, buildOAuthToolCallName, } = require('@librechat/api'); -const { Time, CacheKeys, Constants, isAssistantsEndpoint } = require('librechat-data-provider'); +const { + Time, + CacheKeys, + Constants, + Permissions, + PermissionTypes, + isAssistantsEndpoint, +} = require('librechat-data-provider'); const { getOAuthReconnectionManager, getMCPServersRegistry, getFlowStateManager, getMCPManager, } = require('~/config'); -const { findToken, createToken, updateToken, deleteTokens } = require('~/models'); +const db = require('~/models'); +const { findToken, createToken, updateToken, deleteTokens } = db; const { getGraphApiToken } = require('./GraphTokenService'); const { reinitMCPServer } = require('./Tools/mcp'); const { getAppConfig } = require('./Config'); @@ -36,6 +45,24 @@ const RECONNECT_THROTTLE_MS = 10_000; const missingToolCache = new Map(); const MISSING_TOOL_TTL_MS = 10_000; +async function userCanUseMCPServers(user) { + if (!user?.id || !user?.role) { + return false; + } + + try { + return await checkAccess({ + user, + permissionType: PermissionTypes.MCP_SERVERS, + permissions: [Permissions.USE], + getRoleByName: db.getRoleByName, + }); + } catch (error) { + logger.error(`[MCP][User: ${user.id}] Failed MCP permission check`, error); + return false; + } +} + function evictStale(map, ttl) { if (map.size <= MAX_CACHE_SIZE) { return; @@ -586,6 +613,7 @@ async function createMCPTool({ return createToolInstance({ res, + user, provider, toolName, serverName, @@ -597,6 +625,7 @@ async function createMCPTool({ function createToolInstance({ res, + user: capturedUser = null, toolName, serverName, serverConfig: capturedServerConfig, @@ -624,18 +653,24 @@ function createToolInstance({ /** @type {(toolArguments: Object | string, config?: GraphRunnableConfig) => Promise} */ const _call = async (toolArguments, config) => { - const userId = config?.configurable?.user?.id || config?.configurable?.user_id; + const permissionUser = config?.configurable?.user ?? capturedUser; + const userId = + config?.configurable?.user?.id || config?.configurable?.user_id || capturedUser?.id; /** @type {ReturnType} */ let abortHandler = null; /** @type {AbortSignal} */ let derivedSignal = null; try { + const provider = (config?.metadata?.provider || capturedProvider)?.toLowerCase(); + const canUseMCP = await userCanUseMCPServers(permissionUser); + if (!canUseMCP) { + throw new Error('Forbidden: Insufficient MCP server permissions'); + } const flowsCache = getLogStores(CacheKeys.FLOWS); const flowManager = getFlowStateManager(flowsCache); derivedSignal = config?.signal ? AbortSignal.any([config.signal]) : undefined; const mcpManager = getMCPManager(userId); - const provider = (config?.metadata?.provider || capturedProvider)?.toLowerCase(); const { args: _args, stepId, ...toolCall } = config.toolCall ?? {}; const flowId = `${serverName}:oauth_login:${config.metadata.thread_id}:${config.metadata.run_id}`; @@ -888,6 +923,7 @@ async function getServerConnectionStatus( module.exports = { createMCPTool, createMCPTools, + userCanUseMCPServers, getMCPSetupData, resolveConfigServers, resolveMcpConfigNames, diff --git a/api/server/services/MCP.spec.js b/api/server/services/MCP.spec.js index 8b9f055ff8..31d8055239 100644 --- a/api/server/services/MCP.spec.js +++ b/api/server/services/MCP.spec.js @@ -38,7 +38,7 @@ jest.mock('@librechat/api', () => { const { logger } = require('@librechat/data-schemas'); const { MCPOAuthHandler } = require('@librechat/api'); -const { CacheKeys, Constants } = require('librechat-data-provider'); +const { CacheKeys, Constants, Permissions, PermissionTypes } = require('librechat-data-provider'); const D = Constants.mcp_delimiter; const { createMCPTool, @@ -71,6 +71,8 @@ jest.mock('~/models', () => ({ findToken: jest.fn(), createToken: jest.fn(), updateToken: jest.fn(), + deleteTokens: jest.fn(), + getRoleByName: jest.fn(), })); jest.mock('./Tools/mcp', () => ({ @@ -660,12 +662,14 @@ describe('tests for the new helper functions used by the MCP connection status e describe('User parameter passing tests', () => { let mockReinitMCPServer; + let mockGetMCPManager; let mockGetFlowStateManager; let mockGetLogStores; beforeEach(() => { jest.clearAllMocks(); mockReinitMCPServer = require('./Tools/mcp').reinitMCPServer; + mockGetMCPManager = require('~/config').getMCPManager; mockGetFlowStateManager = require('~/config').getFlowStateManager; mockGetLogStores = require('~/cache').getLogStores; @@ -813,6 +817,53 @@ describe('User parameter passing tests', () => { // Verify reinitMCPServer was NOT called since tool was in cache expect(mockReinitMCPServer).not.toHaveBeenCalled(); }); + + it('should reject tool execution when user lacks MCP server use permission', async () => { + const mockUser = { id: 'mcp-denied-user', role: 'USER' }; + const mockRes = { write: jest.fn(), flush: jest.fn() }; + const { getRoleByName } = require('~/models'); + getRoleByName.mockResolvedValue({ + permissions: { + [PermissionTypes.MCP_SERVERS]: { + [Permissions.USE]: false, + }, + }, + }); + + const mcpTool = await createMCPTool({ + res: mockRes, + user: mockUser, + toolKey: `test-tool${D}test-server`, + provider: 'openai', + userMCPAuthMap: {}, + availableTools: { + [`test-tool${D}test-server`]: { + function: { + description: 'Cached tool', + parameters: { type: 'object', properties: {} }, + }, + }, + }, + }); + + await expect( + mcpTool.invoke( + {}, + { + configurable: { + user: mockUser, + }, + metadata: { + provider: 'openai', + }, + toolCall: {}, + }, + ), + ).rejects.toThrow( + '[MCP][test-server][test-tool] tool call failed: Forbidden: Insufficient MCP server permissions', + ); + expect(mockGetMCPManager).not.toHaveBeenCalled(); + }); }); describe('reinitMCPServer (via reconnectServer)', () => { diff --git a/api/server/services/ToolService.js b/api/server/services/ToolService.js index 83e47e445b..261ac42547 100644 --- a/api/server/services/ToolService.js +++ b/api/server/services/ToolService.js @@ -63,7 +63,7 @@ const { primeFiles: primeCodeFiles } = require('~/server/services/Files/Code/pro const { manifestToolMap, toolkits } = require('~/app/clients/tools/manifest'); const { createOnSearchResults } = require('~/server/services/Tools/search'); const { reinitMCPServer } = require('~/server/services/Tools/mcp'); -const { resolveConfigServers } = require('~/server/services/MCP'); +const { resolveConfigServers, userCanUseMCPServers } = require('~/server/services/MCP'); const { recordUsage } = require('~/server/services/Threads'); const { loadTools } = require('~/app/clients/tools/util'); const { redactMessage } = require('~/config/parsers'); @@ -551,6 +551,8 @@ async function loadToolDefinitionsWrapper({ req, res, agent, streamId = null, to const codeExecutionEnabled = agent.tools?.includes(Tools.execute_code) === true && enabledCapabilities.has(AgentCapabilities.execute_code); + const hasMCPTools = agent.tools?.some((tool) => tool?.includes(Constants.mcp_delimiter)); + const canUseMCP = hasMCPTools ? await userCanUseMCPServers(req.user) : true; const filteredTools = agent.tools?.filter((tool) => { if (tool === Tools.file_search) { @@ -565,6 +567,9 @@ async function loadToolDefinitionsWrapper({ req, res, agent, streamId = null, to if (isActionTool(tool)) { return actionsEnabled; } + if (tool?.includes(Constants.mcp_delimiter)) { + return areToolsEnabled && canUseMCP; + } if (!areToolsEnabled) { return false; } @@ -577,9 +582,9 @@ async function loadToolDefinitionsWrapper({ req, res, agent, streamId = null, to /** @type {Record>} */ let userMCPAuthMap; - if (agent.tools?.some((t) => t.includes(Constants.mcp_delimiter))) { + if (filteredTools?.some((t) => t.includes(Constants.mcp_delimiter))) { userMCPAuthMap = await getUserMCPAuthMap({ - tools: agent.tools, + tools: filteredTools, userId: req.user.id, findPluginAuthsByKeys, }); @@ -983,6 +988,8 @@ async function loadAgentTools({ }; const areToolsEnabled = checkCapability(AgentCapabilities.tools); const actionsEnabled = checkCapability(AgentCapabilities.actions); + const hasMCPTools = agent.tools?.some((tool) => tool?.includes(Constants.mcp_delimiter)); + const canUseMCP = hasMCPTools ? await userCanUseMCPServers(req.user) : true; let includesWebSearch = false; const _agentTools = agent.tools?.filter((tool) => { @@ -995,6 +1002,8 @@ async function loadAgentTools({ return includesWebSearch; } else if (isActionTool(tool)) { return actionsEnabled; + } else if (tool?.includes(Constants.mcp_delimiter)) { + return areToolsEnabled && canUseMCP; } else if (!areToolsEnabled) { return false; } @@ -1012,9 +1021,9 @@ async function loadAgentTools({ /** @type {Record>} */ let userMCPAuthMap; - if (agent.tools?.some((t) => t.includes(Constants.mcp_delimiter))) { + if (_agentTools?.some((t) => t.includes(Constants.mcp_delimiter))) { userMCPAuthMap = await getUserMCPAuthMap({ - tools: agent.tools, + tools: _agentTools, userId: req.user.id, findPluginAuthsByKeys, }); diff --git a/api/server/services/__tests__/ToolService.spec.js b/api/server/services/__tests__/ToolService.spec.js index ab95b389ca..451120eaa3 100644 --- a/api/server/services/__tests__/ToolService.spec.js +++ b/api/server/services/__tests__/ToolService.spec.js @@ -77,6 +77,7 @@ jest.mock('~/config', () => ({ })); jest.mock('~/server/services/MCP', () => ({ resolveConfigServers: (...args) => mockResolveConfigServers(...args), + userCanUseMCPServers: jest.fn().mockResolvedValue(true), })); jest.mock('~/cache', () => ({ getLogStores: jest.fn(() => ({})), @@ -255,6 +256,28 @@ describe('ToolService - Action Capability Gating', () => { expect(callArgs.tools).toContain(regularTool); }); + it('should filter MCP tool definitions when user lacks MCP server use permission', async () => { + const { userCanUseMCPServers } = require('~/server/services/MCP'); + userCanUseMCPServers.mockResolvedValueOnce(false); + + const mcpTool = `search${Constants.mcp_delimiter}myserver`; + const capabilities = [AgentCapabilities.tools]; + const req = createMockReq(capabilities); + mockGetEndpointsConfig.mockResolvedValue(createEndpointsConfig(capabilities)); + + await loadAgentTools({ + req, + res: {}, + agent: { id: 'agent_123', tools: [regularTool, mcpTool] }, + definitionsOnly: true, + }); + + expect(mockLoadToolDefinitions).toHaveBeenCalledTimes(1); + const [callArgs] = mockLoadToolDefinitions.mock.calls[0]; + expect(callArgs.tools).toContain(regularTool); + expect(callArgs.tools).not.toContain(mcpTool); + }); + it('should return actionsEnabled in the result', async () => { const capabilities = [AgentCapabilities.tools]; const req = createMockReq(capabilities); diff --git a/packages/api/src/agents/openai/service.spec.ts b/packages/api/src/agents/openai/service.spec.ts new file mode 100644 index 0000000000..542ea54b6a --- /dev/null +++ b/packages/api/src/agents/openai/service.spec.ts @@ -0,0 +1,107 @@ +import { createAgentChatCompletion } from './service'; +import type { ChatCompletionDependencies } from './service'; + +jest.mock('@librechat/data-schemas', () => ({ + logger: { + debug: jest.fn(), + error: jest.fn(), + info: jest.fn(), + warn: jest.fn(), + }, +})); + +type CreateRunArgs = { user?: Record }; +type ProcessStreamConfig = { configurable?: Record }; + +function createMockReq(user?: Record) { + return { + body: { + model: 'agent_test', + messages: [{ role: 'user', content: 'hi' }], + stream: false, + }, + user, + on: jest.fn(), + } as unknown as Parameters[0]; +} + +function createMockRes() { + const res: Record = { + setHeader: jest.fn(), + flushHeaders: jest.fn(), + write: jest.fn(), + end: jest.fn(), + headersSent: false, + }; + res.status = jest.fn(() => res); + res.json = jest.fn(() => res); + return res as unknown as Parameters[1]; +} + +describe('createAgentChatCompletion - MCP permission user propagation', () => { + let createRun: jest.Mock; + let processStream: jest.Mock; + let deps: ChatCompletionDependencies; + + beforeEach(() => { + processStream = jest.fn().mockResolvedValue(undefined); + createRun = jest.fn().mockResolvedValue({ processStream }); + + deps = { + getAgent: jest.fn().mockResolvedValue({ + id: 'agent_test', + provider: 'openai', + model: 'gpt-4o-mini', + tools: [], + }), + initializeAgent: jest.fn().mockResolvedValue({ + id: 'agent_test', + provider: 'openai', + model: 'gpt-4o-mini', + tools: [], + attachments: [], + toolContextMap: {}, + maxContextTokens: 1000, + model_parameters: {}, + }), + createRun: createRun as unknown as ChatCompletionDependencies['createRun'], + }; + }); + + it('forwards the role-bearing safe user to createRun and configurable.user', async () => { + const req = createMockReq({ + id: 'user-123', + role: 'ADMIN', + email: 'admin@example.com', + password: 'secret', + }); + + await createAgentChatCompletion(req, createMockRes(), deps); + + expect(createRun).toHaveBeenCalledTimes(1); + const runArgs = createRun.mock.calls[0][0] as CreateRunArgs; + expect(runArgs.user).toMatchObject({ id: 'user-123', role: 'ADMIN' }); + // createSafeUser must strip sensitive fields. + expect(runArgs.user).not.toHaveProperty('password'); + + expect(processStream).toHaveBeenCalledTimes(1); + const streamConfig = processStream.mock.calls[0][1] as ProcessStreamConfig; + expect(streamConfig.configurable?.user).toMatchObject({ id: 'user-123', role: 'ADMIN' }); + expect(streamConfig.configurable?.user_id).toBe('user-123'); + }); + + it('falls back to a bare id when no authenticated user is attached', async () => { + const req = createMockReq(undefined); + + await createAgentChatCompletion(req, createMockRes(), deps); + + expect(createRun).toHaveBeenCalledTimes(1); + const runArgs = createRun.mock.calls[0][0] as CreateRunArgs; + expect(runArgs.user).toEqual({ id: 'api-user' }); + + const streamConfig = processStream.mock.calls[0][1] as ProcessStreamConfig; + // No role present → the runtime MCP check fails closed. + expect(streamConfig.configurable?.user).toEqual({ id: 'api-user' }); + expect(streamConfig.configurable?.user).not.toHaveProperty('role'); + }); +}); diff --git a/packages/api/src/agents/openai/service.ts b/packages/api/src/agents/openai/service.ts index 0d637f7a50..dd8688d051 100644 --- a/packages/api/src/agents/openai/service.ts +++ b/packages/api/src/agents/openai/service.ts @@ -39,6 +39,7 @@ import { createChunk, writeSSE, } from './handlers'; +import { createSafeUser } from '~/utils'; import type { ToolExecuteOptions } from '../handlers'; /** @@ -500,7 +501,17 @@ export async function createAgentChatCompletion( // Create and run the agent if (deps.createRun) { - const userId = (req as unknown as { user?: { id?: string } }).user?.id ?? 'api-user'; + const reqUser = (req as unknown as { user?: Parameters[0] }).user; + const userId = reqUser?.id ?? 'api-user'; + /** + * Propagate the full safe user (id + role), matching the in-repo agent + * controllers (responses.js / openai.js). The runtime MCP permission + * check reads `configurable.user`; passing only `user_id` would make + * every MCP tool call fail closed for an authenticated caller. When the + * host app didn't attach a user, this falls back to a bare id, which + * correctly leaves MCP gated. + */ + const safeUser: Record = { ...createSafeUser(reqUser), id: userId }; const run = await deps.createRun({ agents: [initializedAgent], @@ -512,7 +523,7 @@ export async function createAgentChatCompletion( messageId: requestId, conversationId, }, - user: { id: userId }, + user: safeUser, }); if (run) { @@ -523,6 +534,7 @@ export async function createAgentChatCompletion( configurable: { thread_id: conversationId, user_id: userId, + user: safeUser, }, signal: abortController.signal, streamMode: 'values', diff --git a/packages/api/src/mcp/__tests__/parsers.test.ts b/packages/api/src/mcp/__tests__/parsers.test.ts index afc3fd9de3..3d1c71edbd 100644 --- a/packages/api/src/mcp/__tests__/parsers.test.ts +++ b/packages/api/src/mcp/__tests__/parsers.test.ts @@ -29,6 +29,18 @@ describe('formatToolContent', () => { expect(content).toBe('(No response)'); expect(artifacts).toBeUndefined(); }); + + it('should preserve the image payload in the string for unrecognized providers', () => { + const result: t.MCPToolCallResponse = { + content: [{ type: 'image', data: 'iVBORw0KGgoAAAA...', mimeType: 'image/png' }], + }; + + const [content, artifacts] = formatToolContent(result, 'unknown' as t.Provider); + + expect(artifacts).toBeUndefined(); + expect(content).toContain('iVBORw0KGgoAAAA...'); + expect(content).toContain('image/png'); + }); }); describe('recognized providers', () => { @@ -91,6 +103,16 @@ describe('formatToolContent', () => { }); describe('image handling', () => { + const originalMaxImageBytes = process.env.MCP_IMAGE_DATA_MAX_BYTES; + + afterEach(() => { + if (originalMaxImageBytes === undefined) { + delete process.env.MCP_IMAGE_DATA_MAX_BYTES; + return; + } + process.env.MCP_IMAGE_DATA_MAX_BYTES = originalMaxImageBytes; + }); + it('should handle images with http URLs', () => { const result: t.MCPToolCallResponse = { content: [{ type: 'image', data: 'https://example.com/image.png', mimeType: 'image/png' }], @@ -147,6 +169,83 @@ describe('formatToolContent', () => { expect(artifacts).toBeDefined(); expect(artifacts?.content).toHaveLength(2); }); + + it('should reject oversized base64 image data before creating artifacts', () => { + process.env.MCP_IMAGE_DATA_MAX_BYTES = '3'; + const result: t.MCPToolCallResponse = { + content: [{ type: 'image', data: 'QUJDRA==', mimeType: 'image/png' }], + }; + + expect(() => formatToolContent(result, 'openai')).toThrow( + 'MCP image result exceeds maximum size of 3 bytes', + ); + }); + + it('should allow base64 image data when decoded size is within the cap', () => { + process.env.MCP_IMAGE_DATA_MAX_BYTES = '4'; + const result: t.MCPToolCallResponse = { + content: [{ type: 'image', data: 'QUJDRA==', mimeType: 'image/png' }], + }; + + const [content, artifacts] = formatToolContent(result, 'openai'); + + expect(content).toBe(''); + expect(artifacts?.content?.[0]).toEqual({ + type: 'image_url', + image_url: { url: 'data:image/png;base64,QUJDRA==' }, + }); + }); + + it('should reject oversized image data for unrecognized providers before stringifying', () => { + process.env.MCP_IMAGE_DATA_MAX_BYTES = '3'; + const result: t.MCPToolCallResponse = { + content: [{ type: 'image', data: 'QUJDRA==', mimeType: 'image/png' }], + }; + + expect(() => formatToolContent(result, 'unknown' as t.Provider)).toThrow( + 'MCP image result exceeds maximum size of 3 bytes', + ); + }); + + it('should not apply the image data cap to remote image URLs', () => { + process.env.MCP_IMAGE_DATA_MAX_BYTES = '3'; + const result: t.MCPToolCallResponse = { + content: [{ type: 'image', data: 'https://example.com/large.png', mimeType: 'image/png' }], + }; + + const [content, artifacts] = formatToolContent(result, 'openai'); + + expect(content).toBe(''); + expect(artifacts?.content?.[0]).toEqual({ + type: 'image_url', + image_url: { url: 'https://example.com/large.png' }, + }); + }); + + it('should enforce the image cap on base64 data that merely starts with "http"', () => { + process.env.MCP_IMAGE_DATA_MAX_BYTES = '3'; + const result: t.MCPToolCallResponse = { + content: [{ type: 'image', data: 'httpAAAAAAAA', mimeType: 'image/png' }], + }; + + expect(() => formatToolContent(result, 'openai')).toThrow( + 'MCP image result exceeds maximum size of 3 bytes', + ); + }); + + it('should treat base64 starting with "http" as inline data, not a remote URL', () => { + const result: t.MCPToolCallResponse = { + content: [{ type: 'image', data: 'httpAAAA', mimeType: 'image/png' }], + }; + + const [content, artifacts] = formatToolContent(result, 'openai'); + + expect(content).toBe(''); + expect(artifacts?.content?.[0]).toEqual({ + type: 'image_url', + image_url: { url: 'data:image/png;base64,httpAAAA' }, + }); + }); }); describe('resource handling', () => { diff --git a/packages/api/src/mcp/parsers.ts b/packages/api/src/mcp/parsers.ts index c9b824e782..04065f62ad 100644 --- a/packages/api/src/mcp/parsers.ts +++ b/packages/api/src/mcp/parsers.ts @@ -3,10 +3,57 @@ import { Tools } from 'librechat-data-provider'; import type { UIResource } from 'librechat-data-provider'; import type * as t from './types'; +export const DEFAULT_MCP_IMAGE_DATA_MAX_BYTES = 10 * 1024 * 1024; + function generateResourceId(text: string): string { return crypto.createHash('sha256').update(text).digest('hex').substring(0, 10); } +function getMCPImageDataMaxBytes(): number { + const raw = process.env.MCP_IMAGE_DATA_MAX_BYTES; + if (!raw) { + return DEFAULT_MCP_IMAGE_DATA_MAX_BYTES; + } + + const parsed = Number(raw); + return Number.isSafeInteger(parsed) && parsed > 0 ? parsed : DEFAULT_MCP_IMAGE_DATA_MAX_BYTES; +} + +function getBase64Padding(data: string): number { + if (data.endsWith('==')) { + return 2; + } + if (data.endsWith('=')) { + return 1; + } + return 0; +} + +function estimateBase64ImageBytes(data: string): number { + const padding = getBase64Padding(data); + return Math.max(0, Math.floor((data.length * 3) / 4) - padding); +} + +function isRemoteImageUrl(data: string): boolean { + return data.startsWith('http://') || data.startsWith('https://'); +} + +function assertImageDataWithinLimit(item: t.ImageContent): void { + if (isRemoteImageUrl(item.data)) { + return; + } + + const maxBytes = getMCPImageDataMaxBytes(); + const estimatedBytes = estimateBase64ImageBytes(item.data); + if (estimatedBytes <= maxBytes) { + return; + } + + throw new Error( + `MCP image result exceeds maximum size of ${maxBytes} bytes: ${estimatedBytes} bytes`, + ); +} + const RECOGNIZED_PROVIDERS = new Set([ 'google', 'anthropic', @@ -38,7 +85,7 @@ const imageFormatters: Record = { default: (item) => ({ type: 'image_url', image_url: { - url: item.data.startsWith('http') ? item.data : `data:${item.mimeType};base64,${item.data}`, + url: isRemoteImageUrl(item.data) ? item.data : `data:${item.mimeType};base64,${item.data}`, }, }), }; @@ -71,6 +118,9 @@ function parseAsString(result: t.MCPToolCallResponse): string { } return resourceText.join('\n'); } + if (isImageContent(item)) { + assertImageDataWithinLimit(item); + } return JSON.stringify(item, null, 2); }) .filter(Boolean) @@ -120,6 +170,7 @@ export function formatToolContent( if (!isImageContent(item)) { return; } + assertImageDataWithinLimit(item); const formatter = imageFormatters.default as t.ImageFormatter; const formattedImage = formatter(item); diff --git a/packages/data-schemas/src/methods/agent.spec.ts b/packages/data-schemas/src/methods/agent.spec.ts index 2ae40cf89b..eb5a15fedc 100644 --- a/packages/data-schemas/src/methods/agent.spec.ts +++ b/packages/data-schemas/src/methods/agent.spec.ts @@ -8,6 +8,8 @@ import { PrincipalModel, PermissionBits, EToolResources, + Constants, + actionDelimiter, } from 'librechat-data-provider'; import type { UpdateWithAggregationPipeline, @@ -520,6 +522,42 @@ describe('Agent Methods', () => { expect(retrievedAgent!.description).toBe('Test description'); }); + test('should derive mcpServerNames only from MCP tools on create', async () => { + const { agentId, authorId } = createTestIds(); + const actionTool = `sync${Constants.mcp_delimiter}state${actionDelimiter}api---example---com`; + const mcpTool = `search${Constants.mcp_delimiter}authorizedServer`; + + const newAgent = await createAgent({ + id: agentId, + name: 'MCP Names Agent', + provider: 'test', + model: 'test-model', + author: authorId, + tools: [actionTool, mcpTool], + }); + + expect(newAgent.mcpServerNames).toEqual(['authorizedServer']); + }); + + test('should derive mcpServerNames only from MCP tools on update', async () => { + const { agentId, authorId } = createTestIds(); + const actionTool = `sync${Constants.mcp_delimiter}state${actionDelimiter}api---example---com`; + const mcpTool = `search${Constants.mcp_delimiter}authorizedServer`; + + await createAgent({ + id: agentId, + name: 'MCP Names Agent', + provider: 'test', + model: 'test-model', + author: authorId, + tools: [], + }); + + const updatedAgent = await updateAgent({ id: agentId }, { tools: [actionTool, mcpTool] }); + + expect(updatedAgent!.mcpServerNames).toEqual(['authorizedServer']); + }); + test('should delete an agent', async () => { const agentId = `agent_${uuidv4()}`; const authorId = new mongoose.Types.ObjectId(); diff --git a/packages/data-schemas/src/methods/agent.ts b/packages/data-schemas/src/methods/agent.ts index 6f11fa720b..8a593b4498 100644 --- a/packages/data-schemas/src/methods/agent.ts +++ b/packages/data-schemas/src/methods/agent.ts @@ -1,5 +1,11 @@ import crypto from 'node:crypto'; -import { Constants, EToolResources, ResourceType, actionDelimiter } from 'librechat-data-provider'; +import { + Constants, + EToolResources, + ResourceType, + actionDelimiter, + isActionTool, +} from 'librechat-data-provider'; import type { AgentToolResources } from 'librechat-data-provider'; import type { FilterQuery, Model, Types } from 'mongoose'; import type { IAgent, IAclEntry } from '~/types'; @@ -46,7 +52,7 @@ function extractMCPServerNames(tools: string[] | undefined | null): string[] { } const serverNames = new Set(); for (const tool of tools) { - if (!tool || !tool.includes(mcp_delimiter)) { + if (!tool || !tool.includes(mcp_delimiter) || isActionTool(tool)) { continue; } const parts = tool.split(mcp_delimiter);