From 7ac9fd31eb067cb01c224abe59c763b09e249e70 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Sun, 9 Aug 2026 22:27:09 -0700 Subject: [PATCH] fix(mcp): move app backend logic into packages/api, bound template prefixes, stop caching partial snapshots Move the substantive sandbox and app-proxy logic out of the Express controller into packages/api/src/mcp/sandbox.ts and apps.ts, per the workspace rule that new backend code is TypeScript in packages/api with /api limited to thin adapters. The controller drops to request adapters and the extracted CSP construction is unit tested directly. The resource policy is byte-identical across scheme, port, wildcard, path and strict-mode inputs, and the sandbox marker substitution stays byte-exact against the proxy document. Compile RFC 6570 prefix modifiers into the matcher. A varspec prefix was stripped without constraining the pattern, so db://items/{id:3} authorized db://items/admin even though no conforming expansion produces it. Prefixes now bound each variable per operator, and non-conforming prefix syntax, a zero or out-of-range length, prefix combined with explode, and an unreasonable number of prefixed variables all fail closed rather than compiling a permissive matcher. Stop caching advertisement snapshots that a transient listing failure truncated: only the request that saw the failure is denied, and a recovered server is re-listed instead of being denied for the connection lifetime. A snapshot truncated by hitting the page or entry cap is still cached, since it is reproducible and re-walking it per read is the cost the cache exists to avoid. Publish the tool authorization caches only from a complete tools/list snapshot. A paginated listing that failed on a later page returned a non-empty partial list that was stored as authoritative, so tools and tool-declared UI resources from omitted pages were denied until a list change or reconnect. Match HTML media types case-insensitively through the shared predicate, so a differently cased Text/HTML;profile=mcp-app is classified as renderable instead of having its markup fall through into model-visible resource text. --- api/server/controllers/mcpApps.js | 327 ++++-------------- api/server/controllers/mcpApps.test.js | 89 +---- client/src/utils/mcpApps.ts | 4 +- packages/api/src/index.ts | 1 + packages/api/src/mcp/MCPManager.ts | 218 ++++++++++-- .../api/src/mcp/__tests__/MCPManager.test.ts | 223 ++++++++++-- packages/api/src/mcp/__tests__/apps.test.ts | 104 +++++- .../api/src/mcp/__tests__/parsers.test.ts | 76 +++- .../api/src/mcp/__tests__/sandbox.test.ts | 197 +++++++++++ packages/api/src/mcp/apps.ts | 75 +++- packages/api/src/mcp/parsers.ts | 12 +- packages/api/src/mcp/sandbox.ts | 171 +++++++++ packages/data-provider/src/mcp/appMime.ts | 21 +- 13 files changed, 1125 insertions(+), 393 deletions(-) create mode 100644 packages/api/src/mcp/__tests__/sandbox.test.ts create mode 100644 packages/api/src/mcp/sandbox.ts diff --git a/api/server/controllers/mcpApps.js b/api/server/controllers/mcpApps.js index 88d5d73b82..3b9548a895 100644 --- a/api/server/controllers/mcpApps.js +++ b/api/server/controllers/mcpApps.js @@ -1,13 +1,15 @@ -const fs = require('fs'); const path = require('path'); const { logger } = require('@librechat/data-schemas'); -const { CacheKeys, Constants } = require('librechat-data-provider'); +const { CacheKeys } = require('librechat-data-provider'); const { - getUserMCPAuthMap, readAppResource, listAppResources, listAppResourceTemplates, callAppTool, + buildSandboxResponse, + isDeniedAppRequest, + buildAppProxyErrorResponse, + resolveAppRequestContext, } = require('@librechat/api'); const { getMCPManager, getFlowStateManager } = require('~/config'); const { getAppConfig } = require('~/server/services/Config'); @@ -21,150 +23,6 @@ const { } = require('~/models'); const { getLogStores } = require('~/cache'); -// MCP SDK ErrorCode.InvalidRequest = -32600 -const MCP_INVALID_REQUEST = -32600; - -/** - * Resolves the request-scoped config and auth context so app follow-up requests can reconnect to - * config-sourced servers even when the original tool-call connection is gone. - */ -const resolveAppContext = async (req, serverName) => { - const userId = req.user?.id; - // Fail closed on both config and auth resolution: a transient lookup failure must reject rather - // than fall back to the base config (wrong server) or to unresolved/stale credentials. A user who - // genuinely has no vars still resolves to an empty map without throwing, so that path proceeds. - const [configServers, userMCPAuthMap] = await Promise.all([ - resolveConfigServers(req, { throwOnError: true }), - getUserMCPAuthMap({ - userId, - servers: [serverName], - findPluginAuthsByKeys, - throwOnError: true, - }).catch((err) => { - logger.error( - `[resolveAppContext] Failed to resolve MCP auth values for user ${userId}, server ${serverName}; failing closed`, - err, - ); - throw err; - }), - ]); - const customUserVars = userMCPAuthMap?.[`${Constants.mcp_prefix}${serverName}`]; - const flowManager = getFlowStateManager(getLogStores(CacheKeys.FLOWS)); - const tokenMethods = { findToken, createToken, updateToken, deleteTokens }; - return { configServers, customUserVars, flowManager, tokenMethods }; -}; - -/** @route POST /api/mcp/resources/read */ -const readMCPResource = async (req, res) => { - try { - const userId = req.user?.id; - if (!userId) { - return res.status(401).json({ error: 'Unauthorized' }); - } - - const { serverName, uri } = req.body; - const ctx = { - userId, - serverName, - user: req.user, - ...(await resolveAppContext(req, serverName)), - }; - const result = await readAppResource(getMCPManager(), ctx, uri); - return res.json(result); - } catch (error) { - // A denied read is an expected client error, so return 400 and skip the error-level log. - if (error && typeof error === 'object' && error.code === MCP_INVALID_REQUEST) { - return res.status(400).json({ error: error.message }); - } - logger.error('[readMCPResource] Error:', error); - return res.status(500).json({ error: 'Failed to read resource' }); - } -}; - -/** @route POST /api/mcp/resources/list */ -const listMCPResources = async (req, res) => { - try { - const userId = req.user?.id; - if (!userId) { - return res.status(401).json({ error: 'Unauthorized' }); - } - - const { serverName, cursor } = req.body; - const ctx = { - userId, - serverName, - user: req.user, - ...(await resolveAppContext(req, serverName)), - }; - const result = await listAppResources(getMCPManager(), ctx, cursor); - return res.json(result); - } catch (error) { - if (error && typeof error === 'object' && error.code === MCP_INVALID_REQUEST) { - return res.status(400).json({ error: error.message }); - } - logger.error('[listMCPResources] Error:', error); - return res.status(500).json({ error: 'Failed to list resources' }); - } -}; - -/** @route POST /api/mcp/resources/templates/list */ -const listMCPResourceTemplates = async (req, res) => { - try { - const userId = req.user?.id; - if (!userId) { - return res.status(401).json({ error: 'Unauthorized' }); - } - - const { serverName, cursor } = req.body; - const ctx = { - userId, - serverName, - user: req.user, - ...(await resolveAppContext(req, serverName)), - }; - const result = await listAppResourceTemplates(getMCPManager(), ctx, cursor); - return res.json(result); - } catch (error) { - if (error && typeof error === 'object' && error.code === MCP_INVALID_REQUEST) { - return res.status(400).json({ error: error.message }); - } - logger.error('[listMCPResourceTemplates] Error:', error); - return res.status(500).json({ error: 'Failed to list resource templates' }); - } -}; - -/** @route POST /api/mcp/app-tool-call */ -const appToolCall = async (req, res) => { - try { - const userId = req.user?.id; - if (!userId) { - return res.status(401).json({ error: 'Unauthorized' }); - } - - const { serverName, toolName, arguments: toolArgs } = req.body; - const ctx = { - userId, - serverName, - user: req.user, - ...(await resolveAppContext(req, serverName)), - }; - const result = await callAppTool(getMCPManager(), ctx, toolName, toolArgs); - return res.json(result); - } catch (error) { - logger.error('[appToolCall] Error:', error); - if (error && typeof error === 'object' && error.code === MCP_INVALID_REQUEST) { - return res.status(400).json({ error: error.message }); - } - return res.status(500).json({ error: 'Failed to execute tool' }); - } -}; - -const MAX_CSP_DOMAINS = 32; -const MAX_CSP_PARAM_LENGTH = 4096; -/** Replaced on the way out so the proxy can refuse to build a frame it has no response policy for. */ -const CSP_APPLIED_PLACEHOLDER = '/*__CSP_APPLIED__*/'; -const CSP_APPLIED_MARKER = 'window.__MCP_SANDBOX_CSP_APPLIED = true;'; - const SANDBOX_PATH = path.resolve( __dirname, '..', @@ -175,123 +33,86 @@ const SANDBOX_PATH = path.resolve( 'mcp-sandbox.html', ); -/** - * CSP3 host-source shape: optional http(s)/ws(s) scheme, optional wildcard subdomain prefix, - * hostname characters, optional port (numeric or `*`), optional path. Rejects CSP keywords, schemes - * with no host, and injection attempts. - * - * Keep in sync with `APP_LINK_HOST_PATTERN` in `client/src/utils/mcpApps.ts`: the host authorizes an - * `openLink` only for declared sources this filter also emits into the enforced policy, so anything - * the matcher accepts must be accepted here too. - */ -const SAFE_HOST_RE = - /^(?:(?:https?|wss?):\/\/)?(?:\*\.)?[a-zA-Z0-9][a-zA-Z0-9\-.]*(?::(?:\d{1,5}|\*))?(?:\/[^\s;,'"?#]*)?$/i; +const resolveAppContext = (req, serverName) => + resolveAppRequestContext({ + userId: req.user?.id, + serverName, + user: req.user, + resolveConfigServers: () => resolveConfigServers(req, { throwOnError: true }), + findPluginAuthsByKeys, + flowManager: getFlowStateManager(getLogStores(CacheKeys.FLOWS)), + tokenMethods: { findToken, createToken, updateToken, deleteTokens }, + }); -const toDomainList = (value) => { - if (!Array.isArray(value)) { - return ''; +const sendAppProxyError = (res, error, { label, fallback, logExpectedErrors = false }) => { + if (logExpectedErrors || !isDeniedAppRequest(error)) { + logger.error(`[${label}] Error:`, error); } - // Trim before testing and emit the trimmed form: joining the raw entry would put its surrounding - // whitespace (a newline, for instance) into the header. - return value - .map((domain) => (typeof domain === 'string' ? domain.trim() : '')) - .filter((domain) => domain && SAFE_HOST_RE.test(domain)) - .slice(0, MAX_CSP_DOMAINS) - .join(' '); + const { status, body } = buildAppProxyErrorResponse(error, fallback); + return res.status(status).json(body); }; -const buildCspPolicy = (csp, strictCsp) => { - const resourceDomains = toDomainList(csp.resourceDomains); - const connectDomains = toDomainList(csp.connectDomains) || "'none'"; - const frameDomains = toDomainList(csp.frameDomains); - - const scriptSrc = strictCsp - ? "script-src 'unsafe-inline' " + resourceDomains - : "script-src 'unsafe-inline' 'unsafe-eval' 'wasm-unsafe-eval' blob: data: " + resourceDomains; - - return [ - "default-src 'none'", - scriptSrc.trim(), - ("style-src 'unsafe-inline' " + resourceDomains).trim(), - 'connect-src ' + connectDomains, - // form-action does not fall back to default-src, so with allow-forms a form could post to - // any origin; bound it to the declared egress allowlist ('none' when none is declared). - 'form-action ' + connectDomains, - ('img-src data: blob: ' + resourceDomains).trim(), - ('media-src ' + (resourceDomains || "'none'")).trim(), - ('font-src ' + (resourceDomains || "'none'")).trim(), - // The app document is installed by navigating the inner frame to a blob URL, so blob: is - // unconditional: the spec's sample emits frame-src 'none' only because it installs the document - // with document.write into about:blank. frameDomains widens it to declared nested iframes. - ('frame-src blob: ' + frameDomains).trim(), - // Workers are created from blob URLs and inherit this policy, which default-src 'none' blocks. - ('worker-src blob: ' + resourceDomains).trim(), - "object-src 'none'", - 'base-uri ' + (toDomainList(csp.baseUriDomains) || "'self'"), - ].join('; '); -}; - -/** An unparseable, oversized, or repeated `csp` param yields the restrictive default policy. */ -const parseCspParam = (raw) => { - if (typeof raw !== 'string' || raw.length === 0 || raw.length > MAX_CSP_PARAM_LENGTH) { - return {}; - } - try { - const parsed = JSON.parse(raw); - if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { - return {}; +const createAppProxyHandler = + ({ label, fallback, logExpectedErrors, proxy }) => + async (req, res) => { + try { + if (!req.user?.id) { + return res.status(401).json({ error: 'Unauthorized' }); + } + const { serverName } = req.body; + const result = await proxy( + getMCPManager(), + await resolveAppContext(req, serverName), + req.body, + ); + return res.json(result); + } catch (error) { + return sendAppProxyError(res, error, { label, fallback, logExpectedErrors }); } - return parsed; - } catch (error) { - logger.debug('[serveMCPSandbox] Ignoring unparseable csp parameter', error); - return {}; - } -}; + }; -let cachedSandboxHtml = null; -const readSandboxHtml = () => { - if (cachedSandboxHtml == null) { - cachedSandboxHtml = fs.readFileSync(SANDBOX_PATH, 'utf8'); - } - return cachedSandboxHtml; -}; +/** @route POST /api/mcp/resources/read */ +const readMCPResource = createAppProxyHandler({ + label: 'readMCPResource', + fallback: 'Failed to read resource', + proxy: (manager, ctx, body) => readAppResource(manager, ctx, body.uri), +}); + +/** @route POST /api/mcp/resources/list */ +const listMCPResources = createAppProxyHandler({ + label: 'listMCPResources', + fallback: 'Failed to list resources', + proxy: (manager, ctx, body) => listAppResources(manager, ctx, body.cursor), +}); + +/** @route POST /api/mcp/resources/templates/list */ +const listMCPResourceTemplates = createAppProxyHandler({ + label: 'listMCPResourceTemplates', + fallback: 'Failed to list resource templates', + proxy: (manager, ctx, body) => listAppResourceTemplates(manager, ctx, body.cursor), +}); + +/** @route POST /api/mcp/app-tool-call */ +const appToolCall = createAppProxyHandler({ + label: 'appToolCall', + fallback: 'Failed to execute tool', + logExpectedErrors: true, + proxy: (manager, ctx, body) => callAppTool(manager, ctx, body.toolName, body.arguments), +}); /** @route GET /api/mcp/sandbox */ const serveMCPSandbox = async (req, res) => { try { - res.setHeader('Content-Type', 'text/html; charset=utf-8'); - // Required, not merely hygienic: the per-resource policy below varies per request. - res.setHeader('Cache-Control', 'no-store, no-cache, must-revalidate'); - res.setHeader('X-Content-Type-Options', 'nosniff'); - res.setHeader('Referrer-Policy', 'same-origin'); - - // The MCP Apps spec requires the Host and Sandbox to have different origins for web hosts. - // Default to same-origin framing; when a dedicated sandbox origin is deployed, the operator - // lists the allowed host origin(s) so the host page can frame this sandbox cross-origin. - const allowedParents = (process.env.MCP_SANDBOX_FRAME_ANCESTORS || '').trim(); - // Only accept scheme://host[:port] tokens. A raw value is interpolated into the CSP header, so - // an unvalidated token containing ";" would inject an unrelated directive. - const ancestors = allowedParents - .split(/[\s,]+/) - .filter((token) => /^https?:\/\/[a-zA-Z0-9][a-zA-Z0-9.-]*(?::\d{1,5})?$/.test(token)) - .join(' '); - const ancestorsPolicy = ancestors - ? `frame-ancestors 'self' ${ancestors}` - : "frame-ancestors 'self'"; - if (ancestors) { - res.setHeader('Cross-Origin-Resource-Policy', 'cross-origin'); - } else { - res.setHeader('Cross-Origin-Resource-Policy', 'same-origin'); - res.setHeader('X-Frame-Options', 'SAMEORIGIN'); - } - const query = req?.query ?? {}; - const resourcePolicy = buildCspPolicy(parseCspParam(query.csp), query.strictCsp === '1'); - // frame-ancestors stays its own policy: CSP3 excludes it from the meta-element path, and - // multiple policies intersect, so the resource policy cannot loosen it. - res.setHeader('Content-Security-Policy', [ancestorsPolicy, resourcePolicy]); - - return res.send(readSandboxHtml().replace(CSP_APPLIED_PLACEHOLDER, CSP_APPLIED_MARKER)); + const { headers, body } = buildSandboxResponse({ + sandboxPath: SANDBOX_PATH, + csp: query.csp, + strictCsp: query.strictCsp, + }); + for (const [name, value] of Object.entries(headers)) { + res.setHeader(name, value); + } + return res.send(body); } catch (error) { logger.error('[serveMCPSandbox] Error:', error); if (res.headersSent) { diff --git a/api/server/controllers/mcpApps.test.js b/api/server/controllers/mcpApps.test.js index 485c7d941d..f95e644f36 100644 --- a/api/server/controllers/mcpApps.test.js +++ b/api/server/controllers/mcpApps.test.js @@ -1,8 +1,11 @@ jest.mock('@librechat/data-schemas', () => ({ logger: { error: jest.fn(), warn: jest.fn(), debug: jest.fn(), info: jest.fn() }, })); +// The sandbox response builder and the error mapping are exercised for real: they are the +// server half of the CSP contract this controller only adapts to Express. jest.mock('@librechat/api', () => ({ - getUserMCPAuthMap: jest.fn(), + ...jest.requireActual('@librechat/api'), + resolveAppRequestContext: jest.fn(), readAppResource: jest.fn(), listAppResources: jest.fn(), listAppResourceTemplates: jest.fn(), @@ -26,8 +29,7 @@ jest.mock('~/cache', () => ({ getLogStores: jest.fn() })); const fs = require('fs'); const path = require('path'); const { logger } = require('@librechat/data-schemas'); -const { getUserMCPAuthMap, readAppResource } = require('@librechat/api'); -const { resolveConfigServers } = require('~/server/services/MCP'); +const { resolveAppRequestContext, readAppResource } = require('@librechat/api'); const { serveMCPSandbox, readMCPResource } = require('./mcpApps'); const SANDBOX_PATH = path.resolve(__dirname, '../../../client/public/mcp-sandbox.html'); @@ -147,68 +149,6 @@ describe('serveMCPSandbox resource policy', () => { expect(strict).not.toContain("'unsafe-eval'"); }); - it.each([ - 'javascript:alert(1)', - 'data:', - 'blob:', - '*', - 'http://*', - 'https://*', - 'evil.com; script-src *', - "'self'", - "'unsafe-eval'", - "'nonce-abc123'", - 'a\nb.com', - 'a\rb.com', - 'under_score.com', - '[::1]', - 'https://a.com?x=1', - 'https://a.com#f', - ])('drops the illegal declared domain %j', async (domain) => { - const res = await serve({ csp: JSON.stringify({ connectDomains: [domain] }) }); - expect(resourcePolicy(res)).toContain("connect-src 'none'"); - }); - - it.each([ - 'https://api.example.com', - 'https://*.example.com', - 'https://a.example.com:8443', - 'https://a.example.com:*', - 'HTTPS://API.EXAMPLE.COM', - 'wss://socket.example.com', - 'api.example.com', - 'https://api.example.com/path', - ])('emits the legal declared domain %j', async (domain) => { - const res = await serve({ csp: JSON.stringify({ connectDomains: [domain] }) }); - expect(resourcePolicy(res)).toContain(`connect-src ${domain}`); - }); - - it('emits declared domains trimmed', async () => { - const res = await serve({ csp: JSON.stringify({ connectDomains: ['\n https://a.com '] }) }); - expect(resourcePolicy(res)).toContain('connect-src https://a.com;'); - }); - - it('caps the number of declared domains', async () => { - const domains = Array.from({ length: 40 }, (_, i) => `https://d${i}.example.com`); - const res = await serve({ csp: JSON.stringify({ connectDomains: domains }) }); - const emitted = resourcePolicy(res) - .split('; ') - .find((directive) => directive.startsWith('connect-src ')); - expect(emitted.split(' ')).toHaveLength(33); - expect(emitted).not.toContain('d32.example.com'); - }); - - it.each([ - ['oversized', `{"connectDomains":["https://a.com"],"pad":"${'x'.repeat(4200)}"}`], - ['unparseable', '{not json'], - ['an array', '["https://a.com"]'], - ['repeated', ['{"connectDomains":["https://a.com"]}', '{"connectDomains":["https://b.com"]}']], - ])('falls back to the restrictive default for %s csp', async (_name, csp) => { - const res = await serve({ csp }); - expect(resourcePolicy(res)).toContain("connect-src 'none'"); - expect(resourcePolicy(res)).toContain('frame-src blob:'); - }); - it('substitutes the fail-closed csp marker into the served document', async () => { const raw = fs.readFileSync(SANDBOX_PATH, 'utf8'); const res = await serve({}); @@ -221,12 +161,11 @@ describe('serveMCPSandbox resource policy', () => { }); }); -describe('resolveAppContext fail-closed', () => { +describe('app proxy adapters', () => { beforeEach(() => jest.clearAllMocks()); - it('rejects the request and does not proxy when auth-value resolution fails', async () => { - resolveConfigServers.mockResolvedValue({}); - getUserMCPAuthMap.mockRejectedValue(new Error('db down')); + it('rejects the request and does not proxy when context resolution fails', async () => { + resolveAppRequestContext.mockRejectedValue(new Error('db down')); const req = { user: { id: 'user-1' }, body: { serverName: 'srv', uri: 'ui://x' } }; const res = makeRes(); @@ -236,9 +175,17 @@ describe('resolveAppContext fail-closed', () => { expect(res.status).toHaveBeenCalledWith(500); }); + it('rejects an unauthenticated request before resolving any context', async () => { + const res = makeRes(); + + await readMCPResource({ body: {} }, res); + + expect(resolveAppRequestContext).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(401); + }); + it('returns 400 without an error-level log when the read is denied', async () => { - resolveConfigServers.mockResolvedValue({}); - getUserMCPAuthMap.mockResolvedValue({}); + resolveAppRequestContext.mockResolvedValue({ userId: 'user-1', serverName: 'srv' }); readAppResource.mockRejectedValue( Object.assign(new Error('Resource "file:///etc/passwd" is not permitted.'), { code: -32600 }), ); diff --git a/client/src/utils/mcpApps.ts b/client/src/utils/mcpApps.ts index 93a1555ae2..232e147c15 100644 --- a/client/src/utils/mcpApps.ts +++ b/client/src/utils/mcpApps.ts @@ -65,7 +65,7 @@ export function getMCPSandboxUrl(): string { } /** - * Must match `MAX_CSP_PARAM_LENGTH` in `serveMCPSandbox` (`api/server/controllers/mcpApps.js`), which + * Must match `MAX_CSP_PARAM_LENGTH` in `packages/api/src/mcp/sandbox.ts`, which * falls back to the restrictive default policy for anything longer. */ export const MAX_SANDBOX_CSP_PARAM_LENGTH = 4096; @@ -199,7 +199,7 @@ export function clampAppViewHeight( * has no stable answer; `path-part` is rejected because the sandbox filter drops path-bearing * entries, so accepting one here would authorize a link the sandbox policy never granted. * - * Keep in sync with `SAFE_HOST_RE` in `serveMCPSandbox` (`api/server/controllers/mcpApps.js`): + * Keep in sync with `SAFE_HOST_RE` in `packages/api/src/mcp/sandbox.ts`: * `isAllowedAppLink` must authorize a strict subset of what the browser grants the same declared * source list inside the sandbox, and every deviation from CSP3 here narrows. */ diff --git a/packages/api/src/index.ts b/packages/api/src/index.ts index 2e83e1bb2c..99ab3029ff 100644 --- a/packages/api/src/index.ts +++ b/packages/api/src/index.ts @@ -23,6 +23,7 @@ export * from './mcp/errors'; export * from './mcp/cache'; export * from './mcp/tools'; export * from './mcp/apps'; +export * from './mcp/sandbox'; export * from './mcp/catalog/store'; export * from './mcp/assistants'; export * from './mcp/request'; diff --git a/packages/api/src/mcp/MCPManager.ts b/packages/api/src/mcp/MCPManager.ts index e70d86f273..fa88cedbe5 100644 --- a/packages/api/src/mcp/MCPManager.ts +++ b/packages/api/src/mcp/MCPManager.ts @@ -40,6 +40,13 @@ import { MCPConnectionFactory } from './MCPConnectionFactory'; import { processMCPEnv, isPluginSourced } from '~/utils/env'; import { MCPConnection } from './connection'; +/** One RFC 6570 varspec: its name plus the single modifier it may carry. */ +interface UriTemplateVarSpec { + name: string; + prefix?: number; + explode: boolean; +} + function createOboToolCallErrorMessage( logPrefix: string, toolName: string, @@ -88,6 +95,9 @@ export class MCPManager extends UserConnectionManager { private static readonly RESOURCE_LIST_MAX_ENTRIES = 5000; /** RFC 6570 §2.2 reserves these expression operators; a template using one is not matchable here. */ private static readonly RESERVED_TEMPLATE_OPERATOR = /^[=,!@|]/; + /** RE2 rejects a repeat count above this, so a larger RFC 6570 prefix cannot be compiled as one. */ + private static readonly MAX_REPEAT_COUNT = 1000; + private static readonly MAX_PREFIXED_TEMPLATE_VARS = 8; /** Creates and initializes the singleton MCPManager instance */ public static async createInstance(configs: t.MCPServers): Promise { @@ -524,8 +534,9 @@ Please follow these instructions when using tools from the respective MCP server >; appHidden: Set; knownNames: Set; + complete: boolean; }> { - const tools = await connection.fetchTools(); + const { tools, complete } = await connection.fetchToolsSnapshot(); const serverMap = new Map< string, { uri: string; csp?: UIResource['csp']; permissions?: UIResource['permissions'] } @@ -551,15 +562,19 @@ Please follow these instructions when using tools from the respective MCP server logger.warn(`[MCP] Ignoring invalid UI resource metadata on tool "${tool.name}":`, error); } } - return { serverMap, appHidden, knownNames }; + return { serverMap, appHidden, knownNames, complete }; } private async populateToolCaches(connection: MCPConnection, cacheKey: string): Promise { - const { serverMap, appHidden, knownNames } = await this.buildToolCaches(connection); - // fetchTools returns [] both for genuinely tool-less servers and for a transient tools/list - // failure. Caching an empty list as authoritative would disable MCP Apps until reconnect, so - // leave the cache unpopulated when empty and re-fetch on the next call. - if (knownNames.size === 0) { + const { serverMap, appHidden, knownNames, complete } = await this.buildToolCaches(connection); + // These caches authorize app tool calls and tool-declared UI resource reads, so a page missing + // from a partial `tools/list` is a false denial rather than a missing feature. An incomplete + // snapshot (and an empty one, which a transient failure and a genuinely tool-less server both + // produce) is left unpublished so the next call re-fetches instead of denying until reconnect. + // A snapshot truncated by a tools/list budget cap reports complete and is cached, for the same + // reason the advertisement snapshot caches its cap-truncated form: it is reproducible, so + // re-fetching it on every call pays the full listing cost without widening the result. + if (!complete || knownNames.size === 0) { return; } this.resourceUriCache.set(cacheKey, serverMap); @@ -1014,7 +1029,7 @@ Please follow these instructions when using tools from the respective MCP server * than per-tool because apps.mdx scopes an app's privileges to the same server connection, and * needed in addition to the advertised set because servers MAY omit UI-only resources from * `resources/list`. A `tools/list` failure denies (and stays retryable: `populateToolCaches` never - * caches an empty tool set, so the next read re-fetches). + * caches an incomplete or empty tool set, so the next read re-fetches). */ private async isToolDeclaredUiResource( connection: MCPConnection, @@ -1101,34 +1116,49 @@ Please follow these instructions when using tools from the respective MCP server } /** - * Walks one cursor-paginated advertisement list. Returns false when the snapshot it produced is - * partial (page cap or entry cap reached), so a denial can report that rather than imply the - * server does not advertise the resource. An empty-string `nextCursor` ends pagination: treating - * it as a next page re-requests page one until the cap and truncates the snapshot instead. + * Walks one cursor-paginated advertisement list. Reports `truncated` when the snapshot it produced + * stopped at the page or entry cap, so a denial can report that rather than imply the server does + * not advertise the resource, and a request failure propagates so it can be told apart from a cap. + * An empty-string `nextCursor` ends pagination: treating it as a next page re-requests page one + * until the cap and truncates the snapshot instead. */ private static async collectAdvertisedPages( fetchPage: (cursor?: string) => Promise<{ items: T[]; nextCursor?: string }>, collect: (item: T) => void, count: () => number, - ): Promise { + ): Promise<'complete' | 'truncated'> { let cursor: string | undefined; for (let page = 0; page < MCPManager.RESOURCE_LIST_MAX_PAGES; page++) { const { items, nextCursor } = await fetchPage(cursor); for (const item of items) { if (count() >= MCPManager.RESOURCE_LIST_MAX_ENTRIES) { - return false; + return 'truncated'; } collect(item); } if (!nextCursor) { - return true; + return 'complete'; } cursor = nextCursor; } - return false; + return 'truncated'; } - /** Snapshots (and caches per connection) the resource URIs and URI templates a server advertises. */ + /** + * A server that does not implement one of the advertisement methods answers the same way every + * time, so its empty list is its actual advertisement rather than a failure to enumerate. + */ + private static isUnimplementedMethod(error: unknown): boolean { + return error instanceof McpError && error.code === ErrorCode.MethodNotFound; + } + + /** + * Snapshots the resource URIs and URI templates a server advertises. Caching is deliberate per + * outcome: a fully walked or cap-truncated snapshot is cached (both are reproducible, and + * re-walking up to `RESOURCE_LIST_MAX_ENTRIES` entries on every app read is the cost this cache + * exists to avoid), while a request failure is not cached at all, so a transient `resources/list` + * error denies only the read that saw it instead of every read for the connection's lifetime. + */ private async getAdvertisedResources( connection: MCPConnection, cacheKey: string, @@ -1143,16 +1173,17 @@ Please follow these instructions when using tools from the respective MCP server const uris = new Set(); const templates: RE2JS[] = []; - let complete = true; + let truncated = false; + let failed = false; // The handshake capabilities say whether the server has resources at all, so a server declaring // none advertises an empty (and complete) set instead of being probed. Capabilities are unknown // only before initialize resolves, where asking is harmless: a failed call still denies. const capabilities = connection.client.getServerCapabilities?.(); if (capabilities == null || capabilities.resources != null) { - // A template-only server may not implement resources/list; treat its failure as an empty - // concrete list so advertised templates below are still collected and can authorize reads. + // A template-only server may not implement resources/list; treat that as an empty concrete + // list so advertised templates below are still collected and can authorize reads. try { - complete = await MCPManager.collectAdvertisedPages( + const outcome = await MCPManager.collectAdvertisedPages( async (cursor) => { const result: ListResourcesResult = await connection.client.listResources( cursor != null ? { cursor } : {}, @@ -1163,13 +1194,14 @@ Please follow these instructions when using tools from the respective MCP server (resource) => uris.add(resource.uri), () => uris.size, ); + truncated = outcome === 'truncated'; } catch (error) { - complete = false; + failed = !MCPManager.isUnimplementedMethod(error); logger.debug(`[MCP][${cacheKey}] resources/list unavailable; using templates only.`, error); } try { - const templatesComplete = await MCPManager.collectAdvertisedPages( + const outcome = await MCPManager.collectAdvertisedPages( async (cursor) => { const result: ListResourceTemplatesResult = await connection.client.listResourceTemplates(cursor != null ? { cursor } : {}, { @@ -1185,9 +1217,9 @@ Please follow these instructions when using tools from the respective MCP server }, () => templates.length, ); - complete = complete && templatesComplete; + truncated = truncated || outcome === 'truncated'; } catch (error) { - complete = false; + failed = failed || !MCPManager.isUnimplementedMethod(error); logger.debug( `[MCP][${cacheKey}] resources/templates/list unavailable; skipping templates.`, error, @@ -1195,13 +1227,18 @@ Please follow these instructions when using tools from the respective MCP server } } - if (!complete) { + const entry = { uris, templates, complete: !truncated && !failed }; + if (failed) { + logger.warn( + `[MCP][${cacheKey}] Advertised resources could not be enumerated; denying this read and re-listing on the next one.`, + ); + return entry; + } + if (truncated) { logger.warn( `[MCP][${cacheKey}] Advertised resource snapshot is incomplete; resources outside the snapshot will be denied for this connection.`, ); } - - const entry = { uris, templates, complete }; this.advertisedResourceCache.set(cacheKey, entry); this.advertisedResourceConnStamp.set(cacheKey, this.resourceConnStamp(connection)); return entry; @@ -1295,6 +1332,15 @@ Please follow these instructions when using tools from the respective MCP server if (!keys) { return null; } + if (varSpecs.some((spec) => spec.includes(':'))) { + const prefixed = MCPManager.compilePrefixedExpansion(op, varSpecs); + if (prefixed == null) { + return null; + } + pattern += prefixed; + i = end + 1; + continue; + } // RFC 6570 3.2.5/3.2.6: each defined variable contributes exactly one prefixed component, // so a non-exploded expression can never expand past its declared variable count. const exploded = varSpecs.some((spec) => spec.endsWith('*')); @@ -1344,6 +1390,122 @@ Please follow these instructions when using tools from the respective MCP server } } + /** + * RFC 6570 §2.4: a varspec carries at most one modifier, `:max-length` (1 to 9999) or `*`. Anything + * else (`{id:3*}`, `{id:0}`, `{id:abc}`) is not a valid varspec, so no expansion of it is knowable. + */ + private static parseVarSpec(spec: string): UriTemplateVarSpec | null { + const explode = spec.endsWith('*'); + const body = explode ? spec.slice(0, -1) : spec; + const colon = body.indexOf(':'); + if (colon === -1) { + const name = body.trim(); + return name ? { name, explode } : null; + } + if (explode) { + return null; + } + const name = body.slice(0, colon).trim(); + const maxLength = body.slice(colon + 1).trim(); + if (!name || !/^[1-9][0-9]{0,3}$/.test(maxLength)) { + return null; + } + return { name, prefix: Number(maxLength), explode }; + } + + /** + * Compiles an expansion in which at least one variable carries a `:max-length` prefix. RFC 6570 + * §2.4.1 truncates a prefixed string value to that many characters, and templates are matched + * against the fully percent-decoded URI, so the limit is a plain character bound on the matched + * text. Without it, `db://items/{id:3}` authorizes `db://items/admin`. + * + * Variables expand in declared order and an undefined one contributes nothing, so what a + * multi-variable expression can produce is any ordered subsequence of its components. Those are + * compiled as a chain of optional per-variable units, each with its own bound, rather than one + * shared quantifier: a shared quantifier would either apply the tightest bound to every position + * or, as before, none to any. The chain still requires at least one component, keeping the + * existing denial for a URI that omits the whole expansion. + * + * A prefix RE2 cannot express as a repeat count leaves that variable unbounded (its own class + * still applies), which is the pre-existing behavior and cannot deny a legitimate expansion. + */ + private static compilePrefixedExpansion(op: string, varSpecs: string[]): string | null { + // The ordered chain is quadratic in the declared variable count, so a pathological varspec list + // authorizes nothing instead of being handed to RE2 as a compile-time cost on every read. + if (varSpecs.length > MCPManager.MAX_PREFIXED_TEMPLATE_VARS) { + return null; + } + const specs: UriTemplateVarSpec[] = []; + for (const varSpec of varSpecs) { + const parsed = MCPManager.parseVarSpec(varSpec); + if (parsed == null) { + return null; + } + specs.push(parsed); + } + + const bound = (spec: UriTemplateVarSpec, cls: string, min: number): string => { + if (spec.prefix == null || spec.prefix > MCPManager.MAX_REPEAT_COUNT) { + return `${cls}${min === 0 ? '*' : '+'}`; + } + return `${cls}{${min},${spec.prefix}}`; + }; + const component = (spec: UriTemplateVarSpec, delimiter: string, cls: string): string => { + const unit = `${delimiter}${bound(spec, cls, 1)}`; + return spec.explode ? `(?:${unit})+` : unit; + }; + const chain = (units: string[]): string => { + const branches = units.map((unit, index) => + units.slice(index + 1).reduce((branch, rest) => `${branch}(?:${rest})?`, `(?:${unit})`), + ); + return branches.length === 1 ? branches[0] : `(?:${branches.join('|')})`; + }; + /** Comma-joined operators expand to one run, so their bound is the sum plus the separators. */ + const joined = (cls: string, min: number, literal = ''): string => { + let total = specs.length - 1; + for (const spec of specs) { + if (spec.prefix == null || spec.explode) { + return `${literal}${cls}${min === 0 ? '*' : '+'}`; + } + total += spec.prefix; + } + if (total > MCPManager.MAX_REPEAT_COUNT) { + return `${literal}${cls}${min === 0 ? '*' : '+'}`; + } + return `${literal}${cls}{${min},${total}}`; + }; + const escaped = (spec: UriTemplateVarSpec): string => + spec.name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const query = (): string => + specs.map((spec) => `${escaped(spec)}=${bound(spec, '[^#&]', 0)}`).join('|'); + + switch (op) { + case '+': + return joined('[^?#]', 1); + case '#': + return joined('[^\\s]', 0, '#'); + case '/': + return chain(specs.map((spec) => component(spec, '/', '[^/?#]'))); + case '.': + return specs.length === 1 && !specs[0].explode + ? component(specs[0], '\\.', '[^/?#]') + : chain(specs.map((spec) => component(spec, '\\.', '[^/?#.]'))); + case ';': + return chain( + specs.map((spec) => { + const unit = `;${escaped(spec)}(?:=${bound(spec, '[^/?#;&]', 0)})?`; + return spec.explode ? `(?:${unit})+` : unit; + }), + ); + case '?': + return `\\?(?:${query()})(?:&(?:${query()}))*`; + case '&': + return `(?:&(?:${query()}))+`; + default: + return joined('[^/?#&=]', 1); + } + } + /** * Proxies an MCP App resources/list request to the server. Paired with readResource so the * advertised serverResources capability is fully backed (resource-browser apps need listing). diff --git a/packages/api/src/mcp/__tests__/MCPManager.test.ts b/packages/api/src/mcp/__tests__/MCPManager.test.ts index 369a67393d..b414ce6e4e 100644 --- a/packages/api/src/mcp/__tests__/MCPManager.test.ts +++ b/packages/api/src/mcp/__tests__/MCPManager.test.ts @@ -6,6 +6,7 @@ import { ListResourcesResultSchema, ListResourceTemplatesResultSchema, ErrorCode, + McpError, } from '@modelcontextprotocol/sdk/types.js'; import type { IUser } from '@librechat/data-schemas'; import type { GraphTokenResolver } from '~/utils/graph'; @@ -1487,7 +1488,9 @@ describe('MCPManager', () => { const mockConnection = { isConnected: jest.fn().mockResolvedValue(true), setRequestHeaders: jest.fn(), - fetchTools: jest.fn().mockResolvedValue([{ name: 'do_thing', _meta: {} }]), + fetchToolsSnapshot: jest + .fn() + .mockResolvedValue({ tools: [{ name: 'do_thing', _meta: {} }], complete: true }), timeout: 30000, client: fakeClient(jest.fn().mockResolvedValue({ content: [] })), } as unknown as MCPConnection; @@ -1529,7 +1532,9 @@ describe('MCPManager', () => { const mockConnection = { isConnected: jest.fn().mockResolvedValue(true), setRequestHeaders: jest.fn(), - fetchTools: jest.fn().mockResolvedValue([{ name: 'do_thing', _meta: {} }]), + fetchToolsSnapshot: jest + .fn() + .mockResolvedValue({ tools: [{ name: 'do_thing', _meta: {} }], complete: true }), timeout: 30000, client: fakeClient(jest.fn().mockResolvedValue({ content: [] })), } as unknown as MCPConnection; @@ -1565,7 +1570,9 @@ describe('MCPManager', () => { const mockConnection = { isConnected: jest.fn().mockResolvedValue(true), setRequestHeaders: jest.fn(), - fetchTools: jest.fn().mockResolvedValue([{ name: 'do_thing', _meta: {} }]), + fetchToolsSnapshot: jest + .fn() + .mockResolvedValue({ tools: [{ name: 'do_thing', _meta: {} }], complete: true }), timeout: 30000, client: fakeClient(jest.fn().mockResolvedValue({ content: [] })), } as unknown as MCPConnection; @@ -1612,7 +1619,7 @@ describe('MCPManager', () => { const mockConnection = { isConnected: jest.fn().mockResolvedValue(true), setRequestHeaders: jest.fn(), - fetchTools: jest.fn().mockResolvedValue([]), + fetchToolsSnapshot: jest.fn().mockResolvedValue({ tools: [], complete: true }), timeout: 30000, client: fakeClient(request), } as unknown as MCPConnection; @@ -1639,11 +1646,12 @@ describe('MCPManager', () => { describe('readResource - app resource authorization', () => { const mockUser: Partial = { id: 'user-123' }; - const buildConnection = (request: jest.Mock, fetchTools?: jest.Mock) => + const buildConnection = (request: jest.Mock, fetchToolsSnapshot?: jest.Mock) => ({ isConnected: jest.fn().mockResolvedValue(true), setRequestHeaders: jest.fn(), - fetchTools: fetchTools ?? jest.fn().mockResolvedValue([]), + fetchToolsSnapshot: + fetchToolsSnapshot ?? jest.fn().mockResolvedValue({ tools: [], complete: true }), timeout: 30000, client: fakeClient(request), }) as unknown as MCPConnection; @@ -1688,7 +1696,12 @@ describe('MCPManager', () => { jest .spyOn(manager, 'getConnection') .mockResolvedValue( - buildConnection(request, jest.fn().mockResolvedValue([uiTool('show', 'ui://app/main')])), + buildConnection( + request, + jest + .fn() + .mockResolvedValue({ tools: [uiTool('show', 'ui://app/main')], complete: true }), + ), ); await manager.readResource({ @@ -1706,16 +1719,15 @@ describe('MCPManager', () => { // apps.mdx scopes app privileges to the server connection, not to the originating tool. const request = jest.fn().mockResolvedValue({ contents: [] }); const manager = await MCPManager.createInstance(newMCPServersConfig()); - jest - .spyOn(manager, 'getConnection') - .mockResolvedValue( - buildConnection( - request, - jest - .fn() - .mockResolvedValue([uiTool('a', 'ui://app/other'), uiTool('b', 'ui://app/main')]), - ), - ); + jest.spyOn(manager, 'getConnection').mockResolvedValue( + buildConnection( + request, + jest.fn().mockResolvedValue({ + tools: [uiTool('a', 'ui://app/other'), uiTool('b', 'ui://app/main')], + complete: true, + }), + ), + ); await manager.readResource({ userId: 'user-123', @@ -1751,7 +1763,12 @@ describe('MCPManager', () => { jest .spyOn(manager, 'getConnection') .mockResolvedValue( - buildConnection(request, jest.fn().mockResolvedValue([uiTool('show', 'ui://app/main')])), + buildConnection( + request, + jest + .fn() + .mockResolvedValue({ tools: [uiTool('show', 'ui://app/main')], complete: true }), + ), ); await expect( @@ -1825,12 +1842,14 @@ describe('MCPManager', () => { } return Promise.resolve({ contents: [] }); }); - const fetchTools = jest + const fetchToolsSnapshot = jest .fn() .mockRejectedValueOnce(new Error('tools/list unavailable')) - .mockResolvedValue([uiTool('show', 'ui://app/main')]); + .mockResolvedValue({ tools: [uiTool('show', 'ui://app/main')], complete: true }); const manager = await MCPManager.createInstance(newMCPServersConfig()); - jest.spyOn(manager, 'getConnection').mockResolvedValue(buildConnection(request, fetchTools)); + jest + .spyOn(manager, 'getConnection') + .mockResolvedValue(buildConnection(request, fetchToolsSnapshot)); const args = { userId: 'user-123', @@ -1848,6 +1867,44 @@ describe('MCPManager', () => { expect(methods).toContain('resources/read'); }); + it('does not cache a partial tool snapshot as authoritative', async () => { + const request = jest.fn().mockImplementation((req: { method: string }) => { + if (req.method === 'resources/list') { + return Promise.resolve({ resources: [] }); + } + if (req.method === 'resources/templates/list') { + return Promise.resolve({ resourceTemplates: [] }); + } + return Promise.resolve({ contents: [] }); + }); + const fetchToolsSnapshot = jest + .fn() + .mockResolvedValueOnce({ tools: [uiTool('first', 'ui://app/first')], complete: false }) + .mockResolvedValue({ + tools: [uiTool('first', 'ui://app/first'), uiTool('show', 'ui://app/main')], + complete: true, + }); + const manager = await MCPManager.createInstance(newMCPServersConfig()); + jest + .spyOn(manager, 'getConnection') + .mockResolvedValue(buildConnection(request, fetchToolsSnapshot)); + + const args = { + userId: 'user-123', + serverName: 'srv', + uri: 'ui://app/main', + user: mockUser as IUser, + }; + await expect(manager.readResource(args)).rejects.toMatchObject({ + code: ErrorCode.InvalidRequest, + }); + await manager.readResource(args); + + expect(fetchToolsSnapshot).toHaveBeenCalledTimes(2); + const methods = request.mock.calls.map((c) => (c[0] as { method: string }).method); + expect(methods).toContain('resources/read'); + }); + it('allows an advertised uri containing a bare percent sign', async () => { // The exact-match check must stay above canonicalization: `db://100%` cannot be decoded. const request = jest.fn().mockImplementation((req: { method: string }) => { @@ -2110,6 +2167,54 @@ describe('MCPManager', () => { { uriTemplate: 'db://x{|v}', uri: 'db://xv', allowed: false }, // An exploded label repeats a dot-excluded unit, which still spans multiple labels. { uriTemplate: 'x://a{.fmt*}', uri: 'x://a.json.bak', allowed: true }, + // RFC 6570 2.4.1: a `:max-length` prefix caps the expanded value's character count, so a + // longer value is not something the advertised template can produce. + { uriTemplate: 'db://items/{id:3}', uri: 'db://items/42', allowed: true }, + { uriTemplate: 'db://items/{id:3}', uri: 'db://items/abc', allowed: true }, + { uriTemplate: 'db://items/{id:3}', uri: 'db://items/admin', allowed: false }, + { uriTemplate: 'db://items/{id:1}', uri: 'db://items/4', allowed: true }, + { uriTemplate: 'db://items/{id:1}', uri: 'db://items/42', allowed: false }, + { uriTemplate: 'file://docs{+path:5}', uri: 'file://docs/a/b', allowed: true }, + { uriTemplate: 'file://docs{+path:5}', uri: 'file://docs/deep/nested', allowed: false }, + { uriTemplate: 'files://root{/seg:2}', uri: 'files://root/ab', allowed: true }, + { uriTemplate: 'files://root{/seg:2}', uri: 'files://root/private', allowed: false }, + { uriTemplate: 'x://a{.x:4}', uri: 'x://a.json', allowed: true }, + { uriTemplate: 'x://a{.x:4}', uri: 'x://a.jsonnet', allowed: false }, + { uriTemplate: 'db://items{;k:3}', uri: 'db://items;k=1', allowed: true }, + { uriTemplate: 'db://items{;k:3}', uri: 'db://items;k', allowed: true }, + { uriTemplate: 'db://items{;k:3}', uri: 'db://items;k=admin', allowed: false }, + { uriTemplate: 'search://items?q={q:3}', uri: 'search://items?q=foo', allowed: true }, + { uriTemplate: 'search://items{?q:3}', uri: 'search://items?q=foo', allowed: true }, + { uriTemplate: 'search://items{?q:3}', uri: 'search://items?q=foobar', allowed: false }, + { uriTemplate: 'search://items?a=1{&q:3}', uri: 'search://items?a=1&q=foo', allowed: true }, + { + uriTemplate: 'search://items?a=1{&q:3}', + uri: 'search://items?a=1&q=foobar', + allowed: false, + }, + // A prefixed variable and an unprefixed one keep their own bounds, and an undefined leading + // variable lets a later one supply the only component. + { uriTemplate: 'files://root{/a:2,b}', uri: 'files://root/admin', allowed: true }, + { uriTemplate: 'files://root{/a:2,b}', uri: 'files://root/ab/admin', allowed: true }, + { uriTemplate: 'files://root{/a:2,b}', uri: 'files://root/toolong/admin', allowed: false }, + { uriTemplate: 'search://x{?a:2,b}', uri: 'search://x?a=ab&b=anything', allowed: true }, + { uriTemplate: 'search://x{?a:2,b}', uri: 'search://x?a=toolong', allowed: false }, + // A prefix larger than any plausible value cannot deny a legitimate expansion. + { uriTemplate: 'db://items/{id:9999}', uri: `db://items/${'x'.repeat(2000)}`, allowed: true }, + // RFC 6570 2.4 allows one modifier per varspec, so a prefixed explode is not a valid varspec, + // and neither is a zero, oversized, or non-numeric max-length. + { uriTemplate: 'db://items/{id:3*}', uri: 'db://items/42', allowed: false }, + { uriTemplate: 'db://items/{id:0}', uri: 'db://items/42', allowed: false }, + { uriTemplate: 'db://items/{id:10000}', uri: 'db://items/42', allowed: false }, + { uriTemplate: 'db://items/{id:abc}', uri: 'db://items/42', allowed: false }, + { uriTemplate: 'db://items/{:3}', uri: 'db://items/42', allowed: false }, + // The ordered chain is quadratic in the declared variables, so an oversized prefixed list + // authorizes nothing instead of being compiled on every read. + { + uriTemplate: `files://root{/${Array.from({ length: 9 }, (_, i) => `v${i}:2`).join(',')}}`, + uri: 'files://root/ab', + allowed: false, + }, ]; it.each(templateCases)( @@ -2309,6 +2414,82 @@ describe('MCPManager', () => { await read(); expect(listCallCount()).toBe(2); }); + + it('re-lists after a transient resources/list failure instead of caching the partial snapshot', async () => { + let listCalls = 0; + const request = jest.fn().mockImplementation((req: { method: string }) => { + if (req.method === 'resources/list') { + listCalls += 1; + if (listCalls === 1) { + return Promise.reject(new Error('transport reset')); + } + return Promise.resolve({ resources: [{ uri: 'file://allowed.txt' }] }); + } + if (req.method === 'resources/templates/list') { + return Promise.resolve({ resourceTemplates: [] }); + } + return Promise.resolve({ contents: [] }); + }); + const manager = await MCPManager.createInstance(newMCPServersConfig()); + jest.spyOn(manager, 'getConnection').mockResolvedValue(buildConnection(request)); + + const args = { + userId: 'user-123', + serverName: 'srv', + uri: 'file://allowed.txt', + user: mockUser as IUser, + }; + await expect(manager.readResource(args)).rejects.toMatchObject({ + code: ErrorCode.InvalidRequest, + }); + await manager.readResource(args); + + expect(listCalls).toBe(2); + const methods = request.mock.calls.map((c) => (c[0] as { method: string }).method); + expect(methods).toContain('resources/read'); + }); + + it('caches a template-only server that does not implement resources/list', async () => { + const request = jest.fn().mockImplementation((req: { method: string }) => { + if (req.method === 'resources/list') { + return Promise.reject(new McpError(ErrorCode.MethodNotFound, 'Method not found')); + } + if (req.method === 'resources/templates/list') { + return Promise.resolve({ resourceTemplates: [{ uriTemplate: 'db://items/{id}' }] }); + } + return Promise.resolve({ contents: [] }); + }); + const manager = await MCPManager.createInstance(newMCPServersConfig()); + jest.spyOn(manager, 'getConnection').mockResolvedValue(buildConnection(request)); + + await manager.readResource({ + userId: 'user-123', + serverName: 'srv', + uri: 'db://items/42', + user: mockUser as IUser, + }); + await manager.readResource({ + userId: 'user-123', + serverName: 'srv', + uri: 'db://items/43', + user: mockUser as IUser, + }); + // An unimplemented method is not a failure to enumerate, so the denial carries no truncation + // caveat and the snapshot is reused. + await expect( + manager.readResource({ + userId: 'user-123', + serverName: 'srv', + uri: 'file://secret', + user: mockUser as IUser, + }), + ).rejects.toThrow(/is not advertised by the server and cannot be read by an app\.$/); + + const listCalls = request.mock.calls.filter( + (c) => (c[0] as { method: string }).method === 'resources/list', + ); + expect(listCalls).toHaveLength(1); + }); }); describe('getConnection', () => { diff --git a/packages/api/src/mcp/__tests__/apps.test.ts b/packages/api/src/mcp/__tests__/apps.test.ts index 94db7631c4..3ed0a2221a 100644 --- a/packages/api/src/mcp/__tests__/apps.test.ts +++ b/packages/api/src/mcp/__tests__/apps.test.ts @@ -1,5 +1,22 @@ +import { logger } from '@librechat/data-schemas'; +import { ErrorCode, McpError } from '@modelcontextprotocol/sdk/types.js'; +import type { PluginAuthMethods } from '@librechat/data-schemas'; import type { ToolWithMeta } from '../apps'; -import { isToolHiddenFromApp, isToolHiddenFromModel } from '../apps'; +import { + buildAppProxyErrorResponse, + isDeniedAppRequest, + isToolHiddenFromApp, + isToolHiddenFromModel, + resolveAppRequestContext, +} from '../apps'; +import { getPluginAuthMap } from '~/agents/auth'; + +jest.mock('@librechat/data-schemas', () => ({ + ...jest.requireActual('@librechat/data-schemas'), + logger: { error: jest.fn(), warn: jest.fn(), debug: jest.fn(), info: jest.fn() }, +})); + +jest.mock('~/agents/auth', () => ({ getPluginAuthMap: jest.fn() })); const tool = (visibility?: unknown): ToolWithMeta => ({ @@ -44,3 +61,88 @@ describe('tool visibility', () => { }); }); }); + +describe('resolveAppRequestContext', () => { + const findPluginAuthsByKeys = jest.fn() as unknown as PluginAuthMethods['findPluginAuthsByKeys']; + const mockGetPluginAuthMap = getPluginAuthMap as jest.MockedFunction; + + beforeEach(() => jest.clearAllMocks()); + + it('resolves the request config and the server customUserVars together', async () => { + mockGetPluginAuthMap.mockResolvedValue({ mcp_srv: { API_KEY: 'secret' } }); + + const ctx = await resolveAppRequestContext({ + userId: 'user-1', + serverName: 'srv', + resolveConfigServers: () => + Promise.resolve({ srv: { type: 'sse', url: 'https://a.example.com' } }), + findPluginAuthsByKeys, + }); + + expect(ctx.configServers).toEqual({ srv: { type: 'sse', url: 'https://a.example.com' } }); + expect(ctx.customUserVars).toEqual({ API_KEY: 'secret' }); + expect(ctx.userId).toBe('user-1'); + expect(ctx.serverName).toBe('srv'); + }); + + it('fails closed when config resolution fails', async () => { + await expect( + resolveAppRequestContext({ + userId: 'user-1', + serverName: 'srv', + resolveConfigServers: () => Promise.reject(new Error('config unavailable')), + findPluginAuthsByKeys, + }), + ).rejects.toThrow('config unavailable'); + }); + + it('fails closed when auth-value resolution fails rather than proceeding unresolved', async () => { + mockGetPluginAuthMap.mockRejectedValue(new Error('db down')); + + await expect( + resolveAppRequestContext({ + userId: 'user-1', + serverName: 'srv', + resolveConfigServers: () => Promise.resolve({}), + findPluginAuthsByKeys, + }), + ).rejects.toThrow('db down'); + expect(logger.error).toHaveBeenCalled(); + }); + + it('resolves without customUserVars for a user with no stored vars', async () => { + mockGetPluginAuthMap.mockResolvedValue({}); + + const ctx = await resolveAppRequestContext({ + userId: 'user-1', + serverName: 'srv', + resolveConfigServers: () => Promise.resolve({}), + findPluginAuthsByKeys, + }); + + expect(ctx.customUserVars).toBeUndefined(); + expect(ctx.configServers).toEqual({}); + }); +}); + +describe('app proxy error mapping', () => { + it('treats an InvalidRequest denial as a client error and surfaces its message', () => { + const denial = new McpError(ErrorCode.InvalidRequest, 'Resource "x" is not permitted.'); + expect(isDeniedAppRequest(denial)).toBe(true); + expect(buildAppProxyErrorResponse(denial, 'Failed to read resource')).toEqual({ + status: 400, + body: { error: denial.message }, + }); + }); + + it.each([new McpError(ErrorCode.InternalError, 'boom'), new Error('boom'), null, 'boom'])( + 'hides an unexpected failure behind the fallback message: %s', + (error) => { + expect(isDeniedAppRequest(error)).toBe(false); + expect(buildAppProxyErrorResponse(error, 'Failed to read resource')).toEqual({ + status: 500, + body: { error: 'Failed to read resource' }, + }); + }, + ); +}); diff --git a/packages/api/src/mcp/__tests__/parsers.test.ts b/packages/api/src/mcp/__tests__/parsers.test.ts index 130074056f..c3a78baf40 100644 --- a/packages/api/src/mcp/__tests__/parsers.test.ts +++ b/packages/api/src/mcp/__tests__/parsers.test.ts @@ -1,7 +1,7 @@ import { CallToolResultSchema } from '@modelcontextprotocol/sdk/types.js'; import { isMcpAppMimeType, MCP_APP_MIME_TYPE } from 'librechat-data-provider'; import type * as t from '../types'; -import { formatToolContent } from '../parsers'; +import { formatToolContent, isRenderableUiResource } from '../parsers'; describe('formatToolContent', () => { describe('unrecognized providers', () => { @@ -1212,22 +1212,74 @@ describe('isMcpAppMimeType', () => { }); // The bridge payload the server attaches and the App Bridge the client starts must be decided by - // the same predicate, or one side persists fields the other never reads. Scoped to lower-case - // spellings: the tier-1 `includes('html')` gate this path runs first is case-sensitive, so an - // upper-case media type is dropped before classification on both sides. + // the same predicate, or one side persists fields the other never reads. Every accepted spelling + // is covered: the tier-1 renderable gate this path runs first parses the media type through the + // same case-insensitive helper, so a differently-cased app profile reaches classification. + it.each([...accepted, 'text/html', 'text/html;xprofile=mcp-app'])( + 'attaches bridge fields exactly when the profile matches: %s', + (mimeType) => { + const [, artifacts] = formatToolContent( + { + content: [ + { type: 'resource', resource: { uri: 'ui://app', mimeType, text: '

a

' } }, + ], + }, + 'openai', + { serverName: 'srv', toolName: 'do_thing' }, + ); + const uiResource = artifacts?.ui_resources?.data?.[0]; + expect(!!uiResource?.serverName).toBe(isMcpAppMimeType(mimeType)); + }, + ); +}); + +describe('isRenderableUiResource media types', () => { + const uiResource = (mimeType?: string): t.ToolContentPart => + ({ + type: 'resource', + resource: { uri: 'ui://app', mimeType, text: '

hi

' }, + }) as t.ToolContentPart; + it.each([ - ...accepted.filter((mimeType) => mimeType === mimeType.toLowerCase()), - 'text/html', - 'text/html;xprofile=mcp-app', - ])('attaches bridge fields exactly when the profile matches: %s', (mimeType) => { - const [, artifacts] = formatToolContent( + ['text/html', true], + ['Text/HTML', true], + ['TEXT/HTML;profile=mcp-app', true], + ['Text/HTML;profile=mcp-app', true], + ['text/html; charset=UTF-8', true], + [' Text/HTML ', true], + ['application/xhtml+xml', true], + ['Application/XHTML+XML', true], + [undefined, true], + ['application/json', false], + ['text/plain', false], + ['text/plain;x=html', false], + ['image/png', false], + ])('classifies %j renderable=%s', (mimeType, expected) => { + expect(isRenderableUiResource(uiResource(mimeType as string | undefined))).toBe(expected); + }); + + it('keeps a differently-cased app body out of the model-visible text', () => { + const [content, artifacts] = formatToolContent( { - content: [{ type: 'resource', resource: { uri: 'ui://app', mimeType, text: '

a

' } }], + content: [ + { + type: 'resource', + resource: { + uri: 'ui://app', + mimeType: 'Text/HTML;profile=mcp-app', + text: '

secret markup

', + }, + }, + ], }, 'openai', { serverName: 'srv', toolName: 'do_thing' }, ); - const uiResource = artifacts?.ui_resources?.data?.[0]; - expect(!!uiResource?.serverName).toBe(isMcpAppMimeType(mimeType)); + + expect(content).not.toContain('secret markup'); + expect(artifacts?.ui_resources?.data?.[0]).toMatchObject({ + uri: 'ui://app', + serverName: 'srv', + }); }); }); diff --git a/packages/api/src/mcp/__tests__/sandbox.test.ts b/packages/api/src/mcp/__tests__/sandbox.test.ts new file mode 100644 index 0000000000..2e79a40a50 --- /dev/null +++ b/packages/api/src/mcp/__tests__/sandbox.test.ts @@ -0,0 +1,197 @@ +import fs from 'fs'; +import path from 'path'; +import { MAX_CSP_PARAM_LENGTH, buildSandboxResponse } from '../sandbox'; + +jest.mock('@librechat/data-schemas', () => ({ + logger: { error: jest.fn(), warn: jest.fn(), debug: jest.fn(), info: jest.fn() }, +})); + +const SANDBOX_PATH = path.resolve(__dirname, '../../../../../client/public/mcp-sandbox.html'); + +const serve = (query: { csp?: string | string[]; strictCsp?: string | string[] } = {}) => + buildSandboxResponse({ sandboxPath: SANDBOX_PATH, ...query }); + +const policies = (query?: Parameters[0]): string[] => + serve(query).headers['Content-Security-Policy'] as string[]; +const resourcePolicy = (query?: Parameters[0]): string => policies(query)[1]; + +describe('buildSandboxResponse frame-ancestors', () => { + const original = process.env.MCP_SANDBOX_FRAME_ANCESTORS; + afterEach(() => { + if (original === undefined) { + delete process.env.MCP_SANDBOX_FRAME_ANCESTORS; + } else { + process.env.MCP_SANDBOX_FRAME_ANCESTORS = original; + } + }); + + it('keeps frame-ancestors as its own policy, first, so the resource policy cannot loosen it', () => { + delete process.env.MCP_SANDBOX_FRAME_ANCESTORS; + const emitted = policies({ csp: JSON.stringify({ frameDomains: ['https://a.example.com'] }) }); + expect(emitted).toHaveLength(2); + expect(emitted[0]).toBe("frame-ancestors 'self'"); + expect(emitted[1]).not.toContain('frame-ancestors'); + }); + + it('reads the configured ancestors per call rather than at module load', () => { + process.env.MCP_SANDBOX_FRAME_ANCESTORS = 'https://host.example.com'; + expect(policies()[0]).toBe("frame-ancestors 'self' https://host.example.com"); + delete process.env.MCP_SANDBOX_FRAME_ANCESTORS; + expect(policies()[0]).toBe("frame-ancestors 'self'"); + }); + + it('omits X-Frame-Options only when a cross-origin ancestor is configured', () => { + process.env.MCP_SANDBOX_FRAME_ANCESTORS = 'https://host.example.com'; + const crossOrigin = serve().headers; + expect(crossOrigin['Cross-Origin-Resource-Policy']).toBe('cross-origin'); + expect('X-Frame-Options' in crossOrigin).toBe(false); + + delete process.env.MCP_SANDBOX_FRAME_ANCESTORS; + const sameOrigin = serve().headers; + expect(sameOrigin['Cross-Origin-Resource-Policy']).toBe('same-origin'); + expect(sameOrigin['X-Frame-Options']).toBe('SAMEORIGIN'); + }); + + it('drops a token that tries to inject an extra directive', () => { + process.env.MCP_SANDBOX_FRAME_ANCESTORS = 'https://ok.com; script-src *'; + expect(policies()[0]).toBe("frame-ancestors 'self'"); + }); +}); + +describe('buildSandboxResponse resource policy', () => { + it('allows the blob install with no csp declared and never emits a bare frame-src none', () => { + const policy = resourcePolicy(); + expect(policy).toContain('frame-src blob:'); + expect(policy).not.toContain("frame-src 'none'"); + expect(policy).toContain("default-src 'none'"); + expect(policy).toContain("connect-src 'none'"); + expect(policy).toContain("form-action 'none'"); + expect(policy).toContain('worker-src blob:'); + expect(policy).toContain("base-uri 'self'"); + }); + + it('widens frame-src to declared frameDomains only', () => { + expect( + resourcePolicy({ csp: JSON.stringify({ frameDomains: ['https://embed.example.com'] }) }), + ).toContain('frame-src blob: https://embed.example.com'); + }); + + it('bounds form-action and connect-src to the declared egress allowlist', () => { + const policy = resourcePolicy({ + csp: JSON.stringify({ connectDomains: ['https://api.example.com'] }), + }); + expect(policy).toContain('connect-src https://api.example.com'); + expect(policy).toContain('form-action https://api.example.com'); + }); + + it('keeps the proxy script and styles running in both modes', () => { + for (const query of [{}, { strictCsp: '1' }]) { + expect(resourcePolicy(query)).toContain("script-src 'unsafe-inline'"); + expect(resourcePolicy(query)).toContain("style-src 'unsafe-inline'"); + } + }); + + it('drops unsafe-eval, wasm, blob and data script sources under strictCsp', () => { + expect(resourcePolicy()).toContain( + "script-src 'unsafe-inline' 'unsafe-eval' 'wasm-unsafe-eval' blob: data:", + ); + expect(resourcePolicy({ strictCsp: '1' })).not.toContain("'unsafe-eval'"); + }); + + it('only treats the literal "1" as strict mode', () => { + expect(resourcePolicy({ strictCsp: 'true' })).toContain("'unsafe-eval'"); + expect(resourcePolicy({ strictCsp: ['1', '1'] })).toContain("'unsafe-eval'"); + }); + + it.each([ + 'javascript:alert(1)', + 'data:', + 'blob:', + '*', + 'http://*', + 'https://*', + 'evil.com; script-src *', + "'self'", + "'unsafe-eval'", + "'nonce-abc123'", + 'a\nb.com', + 'a\rb.com', + 'under_score.com', + '[::1]', + 'https://a.com?x=1', + 'https://a.com#f', + ])('drops the illegal declared domain %j', (domain) => { + expect(resourcePolicy({ csp: JSON.stringify({ connectDomains: [domain] }) })).toContain( + "connect-src 'none'", + ); + }); + + it.each([ + 'https://api.example.com', + 'https://*.example.com', + 'https://a.example.com:8443', + 'https://a.example.com:*', + 'HTTPS://API.EXAMPLE.COM', + 'wss://socket.example.com', + 'api.example.com', + 'https://api.example.com/path', + ])('emits the legal declared domain %j', (domain) => { + expect(resourcePolicy({ csp: JSON.stringify({ connectDomains: [domain] }) })).toContain( + `connect-src ${domain}`, + ); + }); + + it('emits declared domains trimmed', () => { + expect( + resourcePolicy({ csp: JSON.stringify({ connectDomains: ['\n https://a.com '] }) }), + ).toContain('connect-src https://a.com;'); + }); + + it('caps the number of declared domains', () => { + const domains = Array.from({ length: 40 }, (_, i) => `https://d${i}.example.com`); + const emitted = resourcePolicy({ csp: JSON.stringify({ connectDomains: domains }) }) + .split('; ') + .find((directive) => directive.startsWith('connect-src ')); + expect(emitted?.split(' ')).toHaveLength(33); + expect(emitted).not.toContain('d32.example.com'); + }); + + it.each([ + ['oversized', `{"connectDomains":["https://a.com"],"pad":"${'x'.repeat(4200)}"}`], + ['unparseable', '{not json'], + ['an array', '["https://a.com"]'], + ['null', 'null'], + ['repeated', ['{"connectDomains":["https://a.com"]}', '{"connectDomains":["https://b.com"]}']], + ])('falls back to the restrictive default for %s csp', (_name, csp) => { + const policy = resourcePolicy({ csp }); + expect(policy).toContain("connect-src 'none'"); + expect(policy).toContain('frame-src blob:'); + }); + + it('accepts a declaration exactly at the length the client mirrors', () => { + const pad = 'a'.repeat( + MAX_CSP_PARAM_LENGTH - '{"connectDomains":["https://a.com"],"pad":""}'.length, + ); + const csp = `{"connectDomains":["https://a.com"],"pad":"${pad}"}`; + expect(csp).toHaveLength(MAX_CSP_PARAM_LENGTH); + expect(resourcePolicy({ csp })).toContain('connect-src https://a.com'); + }); +}); + +describe('buildSandboxResponse document', () => { + it('substitutes the fail-closed csp marker on every response and never caches', () => { + const raw = fs.readFileSync(SANDBOX_PATH, 'utf8'); + const expected = raw.replace('/*__CSP_APPLIED__*/', 'window.__MCP_SANDBOX_CSP_APPLIED = true;'); + expect(raw).toContain('/*__CSP_APPLIED__*/'); + + for (const query of [{}, { strictCsp: '1' }]) { + const { headers, body } = serve(query); + expect(body).toBe(expected); + expect(body).not.toContain('/*__CSP_APPLIED__*/'); + expect(headers['Cache-Control']).toContain('no-store'); + expect(headers['Content-Type']).toBe('text/html; charset=utf-8'); + expect(headers['X-Content-Type-Options']).toBe('nosniff'); + expect(headers['Referrer-Policy']).toBe('same-origin'); + } + }); +}); diff --git a/packages/api/src/mcp/apps.ts b/packages/api/src/mcp/apps.ts index a81aa58dad..19cd67ac2b 100644 --- a/packages/api/src/mcp/apps.ts +++ b/packages/api/src/mcp/apps.ts @@ -3,12 +3,14 @@ * imports the ESM-only `@modelcontextprotocol/ext-apps` package. */ +import { logger } from '@librechat/data-schemas'; import { MCP_APP_MIME_TYPE } from 'librechat-data-provider'; import { McpError, ErrorCode } from '@modelcontextprotocol/sdk/types.js'; -import type { TokenMethods, IUser } from '@librechat/data-schemas'; +import type { PluginAuthMethods, TokenMethods, IUser } from '@librechat/data-schemas'; import type { FlowStateManager } from '~/flow/manager'; import type { MCPOAuthTokens } from './oauth'; import type * as t from './types'; +import { getServerCustomUserVars, getUserMCPAuthMap } from './auth'; export interface ToolWithMeta { _meta?: Record | null; @@ -111,6 +113,77 @@ export interface MCPAppRequestContext { tokenMethods?: TokenMethods; } +/** + * Resolves the request-scoped config and auth context so app follow-up requests can reconnect to + * config-sourced servers even when the original tool-call connection is gone. + * + * Fails closed on both config and auth resolution: a transient lookup failure must reject rather + * than fall back to the base config (wrong server) or to unresolved/stale credentials. A user who + * genuinely has no vars still resolves to an empty map without throwing, so that path proceeds. + * `resolveConfigServers` is supplied by the caller because it is bound to the HTTP request. + */ +export async function resolveAppRequestContext({ + userId, + serverName, + user, + resolveConfigServers, + findPluginAuthsByKeys, + flowManager, + tokenMethods, +}: { + userId: string; + serverName: string; + user?: IUser; + resolveConfigServers: () => Promise>; + findPluginAuthsByKeys: PluginAuthMethods['findPluginAuthsByKeys']; + flowManager?: FlowStateManager; + tokenMethods?: TokenMethods; +}): Promise { + const [configServers, userMCPAuthMap] = await Promise.all([ + resolveConfigServers(), + getUserMCPAuthMap({ + userId, + servers: [serverName], + findPluginAuthsByKeys, + throwOnError: true, + }).catch((error) => { + logger.error( + `[resolveAppRequestContext] Failed to resolve MCP auth values for user ${userId}, server ${serverName}; failing closed`, + error, + ); + throw error; + }), + ]); + return { + userId, + serverName, + user, + configServers, + customUserVars: getServerCustomUserVars(userMCPAuthMap, serverName), + flowManager, + tokenMethods, + }; +} + +/** A denied app request is an expected client error, not a host fault. */ +export function isDeniedAppRequest(error: unknown): boolean { + return ( + error != null && + typeof error === 'object' && + (error as { code?: unknown }).code === ErrorCode.InvalidRequest + ); +} + +export function buildAppProxyErrorResponse( + error: unknown, + fallbackMessage: string, +): { status: number; body: { error: string } } { + if (isDeniedAppRequest(error)) { + return { status: 400, body: { error: (error as Error).message } }; + } + return { status: 500, body: { error: fallbackMessage } }; +} + export async function readAppResource( manager: MCPAppsProxyManager, ctx: MCPAppRequestContext, diff --git a/packages/api/src/mcp/parsers.ts b/packages/api/src/mcp/parsers.ts index 02f188a728..cdbfad61a0 100644 --- a/packages/api/src/mcp/parsers.ts +++ b/packages/api/src/mcp/parsers.ts @@ -1,5 +1,10 @@ import crypto from 'node:crypto'; -import { Tools, MCP_APP_MIME_TYPE, isMcpAppMimeType } from 'librechat-data-provider'; +import { + Tools, + MCP_APP_MIME_TYPE, + isHtmlMediaType, + isMcpAppMimeType, +} from 'librechat-data-provider'; import type { UIResource } from 'librechat-data-provider'; import type * as t from './types'; @@ -167,7 +172,8 @@ function parseAsString(result: t.MCPToolCallResponse): string { * and when deciding whether a result carries an app the apps toggle must gate. * * Deliberately wider than `isMcpAppMimeType`: a plain `text/html` `ui://` resource still renders as - * an inert static view. + * an inert static view. Both tiers parse the media type through the same shared helpers, so a + * differently-cased `Text/HTML;profile=mcp-app` cannot be renderable to one and not the other. */ export function isRenderableUiResource(item: t.ToolContentPart): boolean { if (item.type !== 'resource') { @@ -179,7 +185,7 @@ export function isRenderableUiResource(item: t.ToolContentPart): boolean { } const mimeType = typeof item.resource.mimeType === 'string' ? item.resource.mimeType : 'text/html'; - return mimeType.includes('html'); + return isHtmlMediaType(mimeType); } /** diff --git a/packages/api/src/mcp/sandbox.ts b/packages/api/src/mcp/sandbox.ts new file mode 100644 index 0000000000..44aa533dcf --- /dev/null +++ b/packages/api/src/mcp/sandbox.ts @@ -0,0 +1,171 @@ +import fs from 'fs'; +import { logger } from '@librechat/data-schemas'; + +const MAX_CSP_DOMAINS = 32; +/** + * Mirrored by `MAX_SANDBOX_CSP_PARAM_LENGTH` in `client/src/utils/mcpApps.ts`: anything longer + * yields the restrictive default policy, so the host must not authorize a link against a + * declaration the route dropped. + */ +export const MAX_CSP_PARAM_LENGTH = 4096; +/** Replaced on the way out so the proxy can refuse to build a frame it has no response policy for. */ +const CSP_APPLIED_PLACEHOLDER = '/*__CSP_APPLIED__*/'; +const CSP_APPLIED_MARKER = 'window.__MCP_SANDBOX_CSP_APPLIED = true;'; + +/** + * CSP3 host-source shape: optional http(s)/ws(s) scheme, optional wildcard subdomain prefix, + * hostname characters, optional port (numeric or `*`), optional path. Rejects CSP keywords, schemes + * with no host, and injection attempts. + * + * Keep in sync with `APP_LINK_HOST_PATTERN` in `client/src/utils/mcpApps.ts`: the host authorizes an + * `openLink` only for declared sources this filter also emits into the enforced policy, so anything + * the matcher accepts must be accepted here too. + */ +const SAFE_HOST_RE = + /^(?:(?:https?|wss?):\/\/)?(?:\*\.)?[a-zA-Z0-9][a-zA-Z0-9\-.]*(?::(?:\d{1,5}|\*))?(?:\/[^\s;,'"?#]*)?$/i; + +const FRAME_ANCESTOR_RE = /^https?:\/\/[a-zA-Z0-9][a-zA-Z0-9.-]*(?::\d{1,5})?$/; + +/** Per-resource egress declared by the app's `_meta.ui.csp`, as it arrives on the sandbox URL. */ +export interface SandboxCspDeclaration { + resourceDomains?: string[]; + connectDomains?: string[]; + frameDomains?: string[]; + baseUriDomains?: string[]; +} + +export interface SandboxResponse { + headers: Record; + body: string; +} + +const toDomainList = (value?: string[]): string => { + if (!Array.isArray(value)) { + return ''; + } + // Trim before testing and emit the trimmed form: joining the raw entry would put its surrounding + // whitespace (a newline, for instance) into the header. + return value + .map((domain) => (typeof domain === 'string' ? domain.trim() : '')) + .filter((domain) => domain && SAFE_HOST_RE.test(domain)) + .slice(0, MAX_CSP_DOMAINS) + .join(' '); +}; + +const buildCspPolicy = (csp: SandboxCspDeclaration, strictCsp: boolean): string => { + const resourceDomains = toDomainList(csp.resourceDomains); + const connectDomains = toDomainList(csp.connectDomains) || "'none'"; + const frameDomains = toDomainList(csp.frameDomains); + + const scriptSrc = strictCsp + ? "script-src 'unsafe-inline' " + resourceDomains + : "script-src 'unsafe-inline' 'unsafe-eval' 'wasm-unsafe-eval' blob: data: " + resourceDomains; + + return [ + "default-src 'none'", + scriptSrc.trim(), + ("style-src 'unsafe-inline' " + resourceDomains).trim(), + 'connect-src ' + connectDomains, + // form-action does not fall back to default-src, so with allow-forms a form could post to + // any origin; bound it to the declared egress allowlist ('none' when none is declared). + 'form-action ' + connectDomains, + ('img-src data: blob: ' + resourceDomains).trim(), + ('media-src ' + (resourceDomains || "'none'")).trim(), + ('font-src ' + (resourceDomains || "'none'")).trim(), + // The app document is installed by navigating the inner frame to a blob URL, so blob: is + // unconditional: the spec's sample emits frame-src 'none' only because it installs the document + // with document.write into about:blank. frameDomains widens it to declared nested iframes. + ('frame-src blob: ' + frameDomains).trim(), + // Workers are created from blob URLs and inherit this policy, which default-src 'none' blocks. + ('worker-src blob: ' + resourceDomains).trim(), + "object-src 'none'", + 'base-uri ' + (toDomainList(csp.baseUriDomains) || "'self'"), + ].join('; '); +}; + +/** An unparseable, oversized, or repeated `csp` param yields the restrictive default policy. */ +const parseCspParam = (raw?: string | string[]): SandboxCspDeclaration => { + if (typeof raw !== 'string' || raw.length === 0 || raw.length > MAX_CSP_PARAM_LENGTH) { + return {}; + } + try { + const parsed: unknown = JSON.parse(raw); + if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { + return {}; + } + return parsed as SandboxCspDeclaration; + } catch (error) { + logger.debug('[serveMCPSandbox] Ignoring unparseable csp parameter', error); + return {}; + } +}; + +const sandboxHtmlCache = new Map(); + +const readSandboxHtml = (sandboxPath: string): string => { + const cached = sandboxHtmlCache.get(sandboxPath); + if (cached != null) { + return cached; + } + const html = fs.readFileSync(sandboxPath, 'utf8'); + sandboxHtmlCache.set(sandboxPath, html); + return html; +}; + +/** + * The MCP Apps spec requires the Host and Sandbox to have different origins for web hosts. Default + * to same-origin framing; when a dedicated sandbox origin is deployed, the operator lists the + * allowed host origin(s) so the host page can frame this sandbox cross-origin. + */ +const buildFrameAncestors = (): string => { + // Only accept scheme://host[:port] tokens. A raw value is interpolated into the CSP header, so + // an unvalidated token containing ";" would inject an unrelated directive. + return (process.env.MCP_SANDBOX_FRAME_ANCESTORS || '') + .trim() + .split(/[\s,]+/) + .filter((token) => FRAME_ANCESTOR_RE.test(token)) + .join(' '); +}; + +/** + * Builds the sandbox document and the response headers that carry its per-resource policy. The + * policy varies per request, so it is delivered as a header rather than baked into the document, + * and the document is only served with the `__CSP_APPLIED__` marker substituted: the proxy inside + * refuses to build an app frame without it, so a response that skipped this path cannot run an app. + */ +export function buildSandboxResponse({ + sandboxPath, + csp, + strictCsp, +}: { + sandboxPath: string; + csp?: string | string[]; + strictCsp?: string | string[]; +}): SandboxResponse { + const ancestors = buildFrameAncestors(); + const headers: Record = { + 'Content-Type': 'text/html; charset=utf-8', + // Required, not merely hygienic: the per-resource policy below varies per request. + 'Cache-Control': 'no-store, no-cache, must-revalidate', + 'X-Content-Type-Options': 'nosniff', + 'Referrer-Policy': 'same-origin', + 'Cross-Origin-Resource-Policy': ancestors ? 'cross-origin' : 'same-origin', + }; + if (!ancestors) { + headers['X-Frame-Options'] = 'SAMEORIGIN'; + } + const ancestorsPolicy = ancestors + ? `frame-ancestors 'self' ${ancestors}` + : "frame-ancestors 'self'"; + // frame-ancestors stays its own policy: CSP3 excludes it from the meta-element path, and + // multiple policies intersect, so the resource policy cannot loosen it. + headers['Content-Security-Policy'] = [ + ancestorsPolicy, + buildCspPolicy(parseCspParam(csp), strictCsp === '1'), + ]; + + return { + headers, + body: readSandboxHtml(sandboxPath).replace(CSP_APPLIED_PLACEHOLDER, CSP_APPLIED_MARKER), + }; +} diff --git a/packages/data-provider/src/mcp/appMime.ts b/packages/data-provider/src/mcp/appMime.ts index bde9889b71..f250b68060 100644 --- a/packages/data-provider/src/mcp/appMime.ts +++ b/packages/data-provider/src/mcp/appMime.ts @@ -11,6 +11,8 @@ export const MCP_UI_EXTENSION_ID = 'io.modelcontextprotocol/ui'; const MCP_APP_PROFILE = 'mcp-app'; +const HTML_MEDIA_TYPES = new Set(['text/html', 'application/xhtml+xml']); + function unquote(value: string): string { if (value.length > 1 && value.startsWith('"') && value.endsWith('"')) { return value.slice(1, -1); @@ -18,6 +20,23 @@ function unquote(value: string): string { return value; } +/** RFC 9110 media types are case-insensitive, and parameters are not part of the type. */ +function mediaTypeOf(mimeType: string): string { + return mimeType.split(';')[0].trim().toLowerCase(); +} + +/** + * True for any HTML media type an MCP App view can render, whether or not it carries the app + * profile. Shares its parsing with `isMcpAppMimeType` so the renderable tier can never disagree with + * the app-profile tier on the same media type. + */ +export function isHtmlMediaType(mimeType?: string | null): boolean { + if (typeof mimeType !== 'string') { + return false; + } + return HTML_MEDIA_TYPES.has(mediaTypeOf(mimeType)); +} + /** * True only for the MCP Apps profile: media type exactly `text/html` plus a `profile` parameter * whose value is `mcp-app`. Other parameters (`charset`) may precede or follow it, and the value may @@ -29,7 +48,7 @@ export function isMcpAppMimeType(mimeType?: string | null): boolean { return false; } const parts = mimeType.split(';'); - if (parts[0].trim().toLowerCase() !== 'text/html') { + if (mediaTypeOf(parts[0]) !== 'text/html') { return false; } for (let i = 1; i < parts.length; i++) {