From 054fa4bfa74292ac63f637e9b3447e0d6bfeb012 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Tue, 16 Jun 2026 08:20:52 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=A5=BD=20fix:=20Restrict=20MCP=20Server?= =?UTF-8?q?=20URL=20Disclosure=20to=20Admins,=20Owners,=20and=20Editors=20?= =?UTF-8?q?(#13784)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🥽 fix: Redact Non-User-Sourced MCP Server URLs by ACL Edit Permission GET /api/mcp/servers and GET /api/mcp/servers/:serverName return MCP server configs to any caller with MCP-use permission. For user-sourced configs (DB-stored, UI-submitted), the URL is the caller's own and is intentionally disclosed. For non-user-sourced configs (YAML or config-tier, operator-defined), the URL and OAuth flow endpoints (authorization_url, token_url) are operator-sensitive: they can encode internal infrastructure hostnames and are not editable through the API. This change redacts those fields on non-user-sourced configs unless the caller has edit authority on the resource, using the same ACL check (PermissionBits.EDIT) that the PATCH and DELETE routes already enforce via canAccessMCPServerResource. Callers with broad MANAGE_MCP_SERVERS capability bypass the per-resource check, matching the existing capability bypass in canAccessResource. customUserVars is intentionally not redacted: its values are UI hint metadata (title, description, sensitive), not user-supplied secrets; blanking it would give non-editor callers a Configure form with no field labels. * 🥽 fix: Correct getResourcePermissionsMap import path + tighten redact comments The MCP server redaction commit imported getResourcePermissionsMap from ~/server/controllers/PermissionsController, but that controller is a consumer of the helper, not its exporter. The canonical export lives in ~/server/services/PermissionService (which controllers/agents/v1.js already imports from). Fixes the runtime getResourcePermissionsMap is not a function failure on GET /api/mcp/servers and the four downstream route-spec failures whose config mocks lacked a source field and were therefore wrongly treated as non-user-sourced; mocks now reflect the real registry behavior (addServer/updateServer tag DB-stored configs with source: 'user'). Trims narrating JSDoc on the redact helpers and resorts the librechat-data-provider destructure by length. * chore: import order * 🥽 fix: Redact OAuth Revocation Endpoint Alongside Authorization And Token URLs The OAuth-URL strip path only dropped authorization_url and token_url. The UserOAuthOptionsSchema in packages/data-provider/src/mcp.ts (line 146) accepts revocation_endpoint as another operator-configurable URL, and the OAuth handler uses it to revoke tokens; it can hold the same internal IdP hostnames the existing strip is trying to hide. Adds revocation_endpoint to the destructure so a non-user-sourced YAML/config MCP server config no longer leaks the revocation URL to non-editor callers. The existing strip url and oauth flow URLs spec is extended with a revocation_endpoint value to lock in the new field. * 🥽 fix: Gate Shared DB Server URL Disclosure On ACL Edit Permission source-driven URL disclosure was incorrect for shared DB-backed MCP servers. ServerConfigsDB.mapDBServerToParsedConfig (packages/api/src/mcp/registry/db/ServerConfigsDB.ts:465) sets source: 'user' on every DB-stored config it returns, regardless of who is accessing it. A user with only VIEW share on a DB server, or with agent-mediated access, was therefore treated by the redaction layer as if they owned the URL, and GET /api/mcp/servers disclosed the owner's URL and OAuth flow URLs to viewers who could not edit the resource. The redaction is now driven purely by ACL edit authority: computeCanEditByServer routes every dbId-bearing config through PermissionBits.EDIT regardless of source; redactServerSecrets strips on !canEdit regardless of source. POST and PATCH controllers explicitly pass canEdit: true since both endpoints establish edit authority (POST creates the resource, PATCH is gated on the EDIT middleware). Legacy/ephemeral configs without a dbId still fall back to the source heuristic. * 📝 docs: correct redactServerSecrets URL-disclosure comment --------- Co-authored-by: Danny Avila --- api/server/controllers/mcp.js | 69 +++++++++- api/server/routes/__tests__/mcp.spec.js | 7 +- packages/api/src/mcp/__tests__/utils.test.ts | 135 ++++++++++++++++++- packages/api/src/mcp/utils.ts | 33 ++++- 4 files changed, 228 insertions(+), 16 deletions(-) diff --git a/api/server/controllers/mcp.js b/api/server/controllers/mcp.js index f5850638b2..85b840891d 100644 --- a/api/server/controllers/mcp.js +++ b/api/server/controllers/mcp.js @@ -5,9 +5,10 @@ * @import { MCPServerRegistry } from '@librechat/api' * @import { MCPServerDocument } from 'librechat-data-provider' */ -const { logger } = require('@librechat/data-schemas'); +const { logger, SystemCapabilities } = require('@librechat/data-schemas'); const { checkAccess, + isUserSourced, MCPErrorCodes, redactServerSecrets, redactAllServerSecrets, @@ -17,9 +18,11 @@ const { const { Constants, Permissions, + ResourceType, + PermissionBits, PermissionTypes, - MCPServerUserInputSchema, MCP_USER_INPUT_FIELDS, + MCPServerUserInputSchema, } = require('librechat-data-provider'); const { resolveConfigServers, @@ -27,6 +30,8 @@ const { resolveAllMcpConfigs, } = require('~/server/services/MCP'); const { cacheMCPServerTools, getMCPServerTools } = require('~/server/services/Config'); +const { getResourcePermissionsMap } = require('~/server/services/PermissionService'); +const { hasCapability } = require('~/server/middleware/roles/capabilities'); const { getMCPManager, getMCPServersRegistry } = require('~/config'); const db = require('~/models'); @@ -196,6 +201,55 @@ const getMCPTools = async (req, res) => { res.status(500).json({ message: error.message }); } }; +/** Mirrors canAccessResource's capability bypass plus per-resource ACL EDIT check. */ +async function computeCanEditByServer(req, serverConfigs) { + const canEditByServer = new Map(); + let bypass = false; + try { + bypass = await hasCapability(req.user, SystemCapabilities.MANAGE_MCP_SERVERS); + } catch (err) { + logger.warn(`[computeCanEditByServer] Capability bypass check failed: ${err.message}`); + } + if (bypass) { + for (const name of Object.keys(serverConfigs)) { + canEditByServer.set(name, true); + } + return canEditByServer; + } + const dbIdsToCheck = []; + const dbIdToServerName = new Map(); + for (const [name, config] of Object.entries(serverConfigs)) { + if (config.dbId) { + dbIdsToCheck.push(config.dbId); + dbIdToServerName.set(String(config.dbId), name); + continue; + } + canEditByServer.set(name, isUserSourced(config)); + } + if (dbIdsToCheck.length > 0) { + try { + const permsMap = await getResourcePermissionsMap({ + userId: req.user.id, + role: req.user.role, + resourceType: ResourceType.MCPSERVER, + resourceIds: dbIdsToCheck, + }); + for (const [dbIdStr, name] of dbIdToServerName) { + const bits = permsMap.get(dbIdStr) ?? 0; + canEditByServer.set(name, (bits & PermissionBits.EDIT) !== 0); + } + } catch (err) { + logger.warn( + `[computeCanEditByServer] ACL lookup failed, defaulting to no edit: ${err.message}`, + ); + for (const name of dbIdToServerName.values()) { + canEditByServer.set(name, false); + } + } + } + return canEditByServer; +} + /** * Get all MCP servers with permissions * @route GET /api/mcp/servers @@ -208,7 +262,8 @@ const getMCPServersList = async (req, res) => { } const serverConfigs = await resolveAllMcpConfigs(userId, req.user); - return res.json(redactAllServerSecrets(serverConfigs)); + const canEditByServer = await computeCanEditByServer(req, serverConfigs); + return res.json(redactAllServerSecrets(serverConfigs, { canEditByServer })); } catch (error) { logger.error('[getMCPServersList]', error); res.status(500).json({ error: error.message }); @@ -306,7 +361,7 @@ const createMCPServerController = async (req, res) => { ); res.status(201).json({ serverName: result.serverName, - ...redactServerSecrets(result.config), + ...redactServerSecrets(result.config, { canEdit: true }), }); } catch (error) { logger.error('[createMCPServer]', error); @@ -339,7 +394,9 @@ const getMCPServerById = async (req, res) => { return res.status(404).json({ message: 'MCP server not found' }); } - res.status(200).json(redactServerSecrets(parsedConfig)); + const canEditMap = await computeCanEditByServer(req, { [serverName]: parsedConfig }); + const canEdit = canEditMap.get(serverName) ?? false; + res.status(200).json(redactServerSecrets(parsedConfig, { canEdit })); } catch (error) { logger.error('[getMCPServerById]', error); res.status(500).json({ message: error.message }); @@ -400,7 +457,7 @@ const updateMCPServerController = async (req, res) => { userId, ); - res.status(200).json(redactServerSecrets(parsedConfig)); + res.status(200).json(redactServerSecrets(parsedConfig, { canEdit: true })); } catch (error) { logger.error('[updateMCPServer]', error); const mcpErrorResponse = handleMCPError(error, res); diff --git a/api/server/routes/__tests__/mcp.spec.js b/api/server/routes/__tests__/mcp.spec.js index a1f981843c..823bf783fc 100644 --- a/api/server/routes/__tests__/mcp.spec.js +++ b/api/server/routes/__tests__/mcp.spec.js @@ -2600,11 +2600,13 @@ describe('MCP Routes', () => { type: 'sse', url: 'http://server1.com/sse', title: 'Server 1', + source: 'user', }, 'server-2': { type: 'sse', url: 'http://server2.com/sse', title: 'Server 2', + source: 'user', }, }; @@ -2676,7 +2678,7 @@ describe('MCP Routes', () => { mockRegistryInstance.addServer.mockResolvedValue({ serverName: 'test-sse-server', - config: validConfig, + config: { ...validConfig, source: 'user' }, }); const response = await request(app).post('/api/mcp/servers').send({ config: validConfig }); @@ -3096,6 +3098,7 @@ describe('MCP Routes', () => { type: 'sse', url: 'https://mcp-server.example.com/sse', title: 'Test Server', + source: 'user', }; mockRegistryInstance.getServerConfig.mockResolvedValue(mockConfig); @@ -3164,7 +3167,7 @@ describe('MCP Routes', () => { description: 'Updated description', }; - mockRegistryInstance.updateServer.mockResolvedValue(updatedConfig); + mockRegistryInstance.updateServer.mockResolvedValue({ ...updatedConfig, source: 'user' }); const response = await request(app) .patch('/api/mcp/servers/test-server') diff --git a/packages/api/src/mcp/__tests__/utils.test.ts b/packages/api/src/mcp/__tests__/utils.test.ts index bd6baa2fa0..cbed141d0f 100644 --- a/packages/api/src/mcp/__tests__/utils.test.ts +++ b/packages/api/src/mcp/__tests__/utils.test.ts @@ -236,13 +236,93 @@ describe('redactServerSecrets', () => { expect(redacted.customUserVars).toEqual(config.customUserVars); }); - it('should pass URLs through unchanged', () => { + it('should pass URLs through unchanged when caller has edit authority', () => { const config: ParsedServerConfig = { type: 'sse', - url: 'https://mcp.example.com/sse?param=value', + url: 'https://infra.internal/mcp', + source: 'config', + }; + const redacted = redactServerSecrets(config, { canEdit: true }); + expect(redacted.url).toBe('https://infra.internal/mcp'); + }); + + it('should strip url and oauth flow URLs for non-user-sourced configs without edit authority', () => { + const config: ParsedServerConfig = { + type: 'sse', + url: 'https://infra.internal/mcp', + source: 'config', + oauth: { + client_id: 'cid', + authorization_url: 'https://infra.internal/oauth/authorize', + token_url: 'https://infra.internal/oauth/token', + revocation_endpoint: 'https://infra.internal/oauth/revoke', + scope: 'read', + }, }; const redacted = redactServerSecrets(config); - expect(redacted.url).toBe('https://mcp.example.com/sse?param=value'); + expect(redacted.url).toBeUndefined(); + expect(redacted.oauth?.authorization_url).toBeUndefined(); + expect(redacted.oauth?.token_url).toBeUndefined(); + expect(redacted.oauth?.revocation_endpoint).toBeUndefined(); + expect(redacted.oauth?.client_id).toBe('cid'); + expect(redacted.oauth?.scope).toBe('read'); + }); + + it('should strip url for a VIEW-shared user-sourced config when caller lacks edit', () => { + const config: ParsedServerConfig = { + type: 'sse', + url: 'https://owner-private.example.com/mcp', + source: 'user', + dbId: 'abc123', + oauth: { + client_id: 'cid', + authorization_url: 'https://owner-private.example.com/oauth/authorize', + token_url: 'https://owner-private.example.com/oauth/token', + }, + }; + const redacted = redactServerSecrets(config); + expect(redacted.url).toBeUndefined(); + expect(redacted.oauth?.authorization_url).toBeUndefined(); + expect(redacted.oauth?.token_url).toBeUndefined(); + expect(redacted.oauth?.client_id).toBe('cid'); + }); + + it('should retain non-sensitive oauth fields when stripping oauth flow URLs', () => { + const config: ParsedServerConfig = { + type: 'sse', + url: 'https://infra.internal/mcp', + source: 'config', + oauth: { + client_id: 'cid', + client_secret: 'csecret', + authorization_url: 'https://infra.internal/oauth/authorize', + token_url: 'https://infra.internal/oauth/token', + scope: 'read', + redirect_uri: 'https://app.example.com/callback', + }, + }; + const redacted = redactServerSecrets(config); + expect(redacted.oauth?.client_secret).toBeUndefined(); + expect(redacted.oauth?.authorization_url).toBeUndefined(); + expect(redacted.oauth?.token_url).toBeUndefined(); + expect(redacted.oauth?.client_id).toBe('cid'); + expect(redacted.oauth?.scope).toBe('read'); + expect(redacted.oauth?.redirect_uri).toBe('https://app.example.com/callback'); + }); + + it('should preserve customUserVars metadata for non-user-sourced configs without edit authority', () => { + const config: ParsedServerConfig = { + type: 'sse', + url: 'https://infra.internal/mcp', + source: 'config', + customUserVars: { + API_KEY: { title: 'API Key', description: 'Your key', sensitive: true }, + }, + }; + const redacted = redactServerSecrets(config); + expect(redacted.customUserVars).toEqual({ + API_KEY: { title: 'API Key', description: 'Your key', sensitive: true }, + }); }); it('should only include explicitly allowlisted fields', () => { @@ -295,6 +375,55 @@ describe('redactAllServerSecrets', () => { expect(redacted['server-b'].oauth?.client_id).toBe('cid-b'); expect((redacted['server-c'] as Record).command).toBeUndefined(); }); + + it('should pass canEdit through per-server via canEditByServer map', () => { + const configs: Record = { + 'config-server': { + type: 'sse', + url: 'https://infra.internal/mcp', + source: 'config', + oauth: { + client_id: 'cid', + authorization_url: 'https://infra.internal/oauth/authorize', + }, + }, + 'user-server-owner': { + type: 'sse', + url: 'https://owner.example.com/mcp', + source: 'user', + dbId: 'owner-id', + }, + 'user-server-shared': { + type: 'sse', + url: 'https://shared.example.com/mcp', + source: 'user', + dbId: 'shared-id', + }, + }; + const canEditByServer = new Map([ + ['config-server', false], + ['user-server-owner', true], + ['user-server-shared', false], + ]); + const redacted = redactAllServerSecrets(configs, { canEditByServer }); + expect(redacted['config-server'].url).toBeUndefined(); + expect(redacted['config-server'].oauth?.authorization_url).toBeUndefined(); + expect(redacted['user-server-owner'].url).toBe('https://owner.example.com/mcp'); + expect(redacted['user-server-shared'].url).toBeUndefined(); + }); + + it('should expose URL for non-user-sourced configs when canEditByServer is true', () => { + const configs: Record = { + 'config-server': { + type: 'sse', + url: 'https://infra.internal/mcp', + source: 'config', + }, + }; + const canEditByServer = new Map([['config-server', true]]); + const redacted = redactAllServerSecrets(configs, { canEditByServer }); + expect(redacted['config-server'].url).toBe('https://infra.internal/mcp'); + }); }); describe('isInvalidClientMessage', () => { diff --git a/packages/api/src/mcp/utils.ts b/packages/api/src/mcp/utils.ts index 9b7d25429d..ab79f691d3 100644 --- a/packages/api/src/mcp/utils.ts +++ b/packages/api/src/mcp/utils.ts @@ -247,11 +247,19 @@ export function isUserSourced(config: Pick { +export function redactServerSecrets( + config: ParsedServerConfig, + options?: { canEdit?: boolean }, +): Partial { const safe: Partial = { type: config.type, url: config.url, @@ -291,6 +299,19 @@ export function redactServerSecrets(config: ParsedServerConfig): Partial v !== undefined), ) as Partial; @@ -299,10 +320,12 @@ export function redactServerSecrets(config: ParsedServerConfig): Partial, + options?: { canEditByServer?: ReadonlyMap }, ): Record> { const result: Record> = {}; for (const [key, config] of Object.entries(configs)) { - result[key] = redactServerSecrets(config); + const canEdit = options?.canEditByServer?.get(key) ?? false; + result[key] = redactServerSecrets(config, { canEdit }); } return result; }