diff --git a/api/server/controllers/mcpApps.js b/api/server/controllers/mcpApps.js index b4e1b40ed2..525e6471a9 100644 --- a/api/server/controllers/mcpApps.js +++ b/api/server/controllers/mcpApps.js @@ -29,13 +29,18 @@ const MCP_INVALID_REQUEST = -32600; */ const resolveAppContext = async (req, serverName) => { const userId = req.user?.id; - // Fail closed on config resolution: a transient failure must reject rather than fall back to the - // base config and proxy to the wrong server. (Auth map resolution fails closed downstream.) + // 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 resolves to undefined without throwing, so that path still proceeds. const [configServers, userMCPAuthMap] = await Promise.all([ resolveConfigServers(req, { throwOnError: true }), - Promise.resolve() - .then(() => getUserMCPAuthMap({ userId, servers: [serverName], findPluginAuthsByKeys })) - .catch(() => undefined), + getUserMCPAuthMap({ userId, servers: [serverName], findPluginAuthsByKeys }).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)); @@ -160,11 +165,13 @@ const serveMCPSandbox = async (_req, res) => { // 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(); - if (allowedParents) { - const ancestors = allowedParents - .split(/[\s,]+/) - .filter(Boolean) - .join(' '); + // 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(' '); + if (ancestors) { res.setHeader('Content-Security-Policy', `frame-ancestors 'self' ${ancestors}`); res.setHeader('Cross-Origin-Resource-Policy', 'cross-origin'); } else { diff --git a/api/server/controllers/mcpApps.test.js b/api/server/controllers/mcpApps.test.js new file mode 100644 index 0000000000..80dc1c034a --- /dev/null +++ b/api/server/controllers/mcpApps.test.js @@ -0,0 +1,98 @@ +jest.mock('@librechat/data-schemas', () => ({ + logger: { error: jest.fn(), warn: jest.fn(), debug: jest.fn(), info: jest.fn() }, +})); +jest.mock('@librechat/api', () => ({ + getUserMCPAuthMap: jest.fn(), + readAppResource: jest.fn(), + listAppResources: jest.fn(), + listAppResourceTemplates: jest.fn(), + callAppTool: jest.fn(), +})); +jest.mock('~/config', () => ({ + getMCPManager: jest.fn(), + getFlowStateManager: jest.fn(), +})); +jest.mock('~/server/services/Config', () => ({ getAppConfig: jest.fn() })); +jest.mock('~/server/services/MCP', () => ({ resolveConfigServers: jest.fn() })); +jest.mock('~/models', () => ({ + findPluginAuthsByKeys: jest.fn(), + findToken: jest.fn(), + createToken: jest.fn(), + updateToken: jest.fn(), + deleteTokens: jest.fn(), +})); +jest.mock('~/cache', () => ({ getLogStores: jest.fn() })); + +const { getUserMCPAuthMap, readAppResource } = require('@librechat/api'); +const { resolveConfigServers } = require('~/server/services/MCP'); +const { serveMCPSandbox, readMCPResource } = require('./mcpApps'); + +const makeRes = () => { + const headers = {}; + return { + headers, + headersSent: false, + setHeader: jest.fn((k, v) => { + headers[k] = v; + }), + sendFile: jest.fn(), + status: jest.fn().mockReturnThis(), + json: jest.fn().mockReturnThis(), + }; +}; + +describe('serveMCPSandbox 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('allows a valid host origin and marks the resource cross-origin', async () => { + process.env.MCP_SANDBOX_FRAME_ANCESTORS = 'https://host.example.com'; + const res = makeRes(); + await serveMCPSandbox({}, res); + expect(res.headers['Content-Security-Policy']).toBe( + "frame-ancestors 'self' https://host.example.com", + ); + expect(res.headers['Cross-Origin-Resource-Policy']).toBe('cross-origin'); + }); + + it('drops a token that tries to inject an extra directive', async () => { + process.env.MCP_SANDBOX_FRAME_ANCESTORS = 'https://ok.com; script-src *'; + const res = makeRes(); + await serveMCPSandbox({}, res); + const csp = res.headers['Content-Security-Policy']; + expect(csp).not.toContain('script-src'); + // The ";"-bearing token is rejected wholesale, leaving no valid ancestors -> same-origin default. + expect(csp).toBe("frame-ancestors 'self'"); + expect(res.headers['X-Frame-Options']).toBe('SAMEORIGIN'); + }); + + it('defaults to same-origin when no ancestors are configured', async () => { + delete process.env.MCP_SANDBOX_FRAME_ANCESTORS; + const res = makeRes(); + await serveMCPSandbox({}, res); + expect(res.headers['Content-Security-Policy']).toBe("frame-ancestors 'self'"); + expect(res.headers['Cross-Origin-Resource-Policy']).toBe('same-origin'); + }); +}); + +describe('resolveAppContext fail-closed', () => { + 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')); + const req = { user: { id: 'user-1' }, body: { serverName: 'srv', uri: 'ui://x' } }; + const res = makeRes(); + + await readMCPResource(req, res); + + expect(readAppResource).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(500); + }); +}); diff --git a/api/server/middleware/limiters/mcpAppResourceLimiter.js b/api/server/middleware/limiters/mcpAppResourceLimiter.js new file mode 100644 index 0000000000..445517aad2 --- /dev/null +++ b/api/server/middleware/limiters/mcpAppResourceLimiter.js @@ -0,0 +1,33 @@ +const rateLimit = require('express-rate-limit'); +const { limiterCache } = require('@librechat/api'); +const { ViolationTypes } = require('librechat-data-provider'); +const logViolation = require('~/cache/logViolation'); + +const { TOOL_CALL_VIOLATION_SCORE: score } = process.env; + +const handler = async (req, res) => { + const type = ViolationTypes.TOOL_CALL_LIMIT; + const errorMessage = { + type, + max: 120, + limiter: 'user', + windowInMinutes: 1, + }; + + await logViolation(req, res, type, errorMessage, score); + res.status(429).json({ message: 'Too many app resource requests. Try again later' }); +}; + +const limiterOptions = { + windowMs: 60 * 1000, + max: 120, + handler, + keyGenerator: function (req) { + return req.user?.id; + }, + store: limiterCache('mcp_app_resource_limiter'), +}; + +const mcpAppResourceLimiter = rateLimit(limiterOptions); + +module.exports = mcpAppResourceLimiter; diff --git a/api/server/routes/mcp.js b/api/server/routes/mcp.js index ffb2228ba4..b2f69bd2e0 100644 --- a/api/server/routes/mcp.js +++ b/api/server/routes/mcp.js @@ -40,6 +40,7 @@ const { requireMCPAppsEnabled, } = require('~/server/controllers/mcpApps'); const mcpAppToolCallLimiter = require('~/server/middleware/limiters/mcpAppToolCallLimiter'); +const mcpAppResourceLimiter = require('~/server/middleware/limiters/mcpAppResourceLimiter'); const { getOAuthReconnectionManager, getMCPServersRegistry, @@ -998,6 +999,7 @@ router.post( requireJwtAuth, checkMCPUsePermissions, requireMCPAppsEnabled, + mcpAppResourceLimiter, readMCPResource, ); @@ -1010,6 +1012,7 @@ router.post( requireJwtAuth, checkMCPUsePermissions, requireMCPAppsEnabled, + mcpAppResourceLimiter, listMCPResources, ); @@ -1022,6 +1025,7 @@ router.post( requireJwtAuth, checkMCPUsePermissions, requireMCPAppsEnabled, + mcpAppResourceLimiter, listMCPResourceTemplates, ); diff --git a/client/public/mcp-sandbox.html b/client/public/mcp-sandbox.html index 27e7bb24bc..dedc5577c1 100644 --- a/client/public/mcp-sandbox.html +++ b/client/public/mcp-sandbox.html @@ -170,11 +170,19 @@ }); function injectIntoHead(html, injection) { - if (/]*>/i.test(html)) { - return html.replace(/]*)>/i, '' + injection); - } - if (/]*>/i.test(html)) { - return html.replace(/]*)>/i, '' + injection + ''); + // A meta CSP only governs what is parsed after it, so it must come before any untrusted + // markup. If nothing but a doctype/ tag precedes , insert the policy as the + // first head child and keep the document's structure. Otherwise (markup before , or + // no ) wrap the document so no untrusted bytes are parsed ahead of the policy. + const headMatch = html.match(/]*>/i); + if (headMatch) { + const beforeHead = html + .slice(0, headMatch.index) + .replace(/]*>/i, '') + .replace(/]*>/i, ''); + if (!/\S/.test(beforeHead)) { + return html.replace(/]*)>/i, '' + injection); + } } return ( '' + injection + '' + html + '' diff --git a/client/src/components/Chat/Messages/Content/ToolCall.tsx b/client/src/components/Chat/Messages/Content/ToolCall.tsx index 4e9fe5f7f5..a7f41350da 100644 --- a/client/src/components/Chat/Messages/Content/ToolCall.tsx +++ b/client/src/components/Chat/Messages/Content/ToolCall.tsx @@ -90,7 +90,7 @@ const MCPAppView = React.memo(function MCPAppView({