From 87341c67c06f0ec97a0847524fc5b199246515eb Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Mon, 29 Jun 2026 00:52:58 -0700 Subject: [PATCH] fix(mcp): carry apps flag through the request resolver and canonicalize resource-read auth resolveMCPAllowlists now returns appsEnabled from the merged tenant-scoped config, so a tenant/role/user override of mcpSettings.apps reaches the registry's per-request resolution and callTool attaches no UI resource for users whose tenant disabled apps. Authorize app-driven resource reads in the canonical (fully percent-decoded) space the server resolves and reject any relative path segment, so a percent-encoded traversal such as %2e%2e%2f can no longer match an advertised template. Exact resources/list matches are unaffected. Trim narrating comments across the MCP Apps changes so the code is self-documenting. --- api/server/controllers/mcpApps.js | 13 ++-- api/server/services/initializeMCPs.js | 1 + client/src/Providers/MessagesViewContext.tsx | 7 +- .../Chat/Messages/Content/ToolCall.tsx | 4 +- .../Share/ShareMessagesProvider.tsx | 3 +- client/src/hooks/MCP/useAppBridge.ts | 15 ++-- packages/api/src/mcp/MCPManager.ts | 78 ++++++++++++------- .../api/src/mcp/__tests__/MCPManager.test.ts | 26 +++++++ packages/api/src/mcp/apps.ts | 26 +------ packages/api/src/mcp/connection.ts | 10 +-- .../src/mcp/registry/MCPServersRegistry.ts | 5 +- 11 files changed, 103 insertions(+), 85 deletions(-) diff --git a/api/server/controllers/mcpApps.js b/api/server/controllers/mcpApps.js index 38bafd914c..b4e1b40ed2 100644 --- a/api/server/controllers/mcpApps.js +++ b/api/server/controllers/mcpApps.js @@ -24,15 +24,13 @@ const { getLogStores } = require('~/cache'); const MCP_INVALID_REQUEST = -32600; /** - * Resolves the request-scoped config, the user's custom variables, and the OAuth flow/token - * context for a server so app follow-up requests can connect to config-sourced servers and - * re-resolve credentialed or OAuth connections even when the original tool-call connection is gone. + * 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 config resolution: an app request targets one server by name, so a transient - // failure must reject rather than fall back to the base config for that name and proxy to the - // wrong server. Auth map resolution may still degrade, since a missing var fails closed downstream. + // 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.) const [configServers, userMCPAuthMap] = await Promise.all([ resolveConfigServers(req, { throwOnError: true }), Promise.resolve() @@ -63,8 +61,7 @@ const readMCPResource = async (req, res) => { const result = await readAppResource(getMCPManager(), ctx, uri); return res.json(result); } catch (error) { - // A denied read (non-advertised / non-ui:// resource) is an expected client error, not a - // backend failure, so return 400 and skip the error-level log, mirroring appToolCall. + // 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 }); } diff --git a/api/server/services/initializeMCPs.js b/api/server/services/initializeMCPs.js index f1c0f95d43..bc4fd02ed8 100644 --- a/api/server/services/initializeMCPs.js +++ b/api/server/services/initializeMCPs.js @@ -15,6 +15,7 @@ async function resolveMCPAllowlists(ctx) { return { allowedDomains: appConfig?.mcpSettings?.allowedDomains, allowedAddresses: appConfig?.mcpSettings?.allowedAddresses, + appsEnabled: appConfig?.mcpSettings?.apps, }; } diff --git a/client/src/Providers/MessagesViewContext.tsx b/client/src/Providers/MessagesViewContext.tsx index 2c58b4a358..5e43d48528 100644 --- a/client/src/Providers/MessagesViewContext.tsx +++ b/client/src/Providers/MessagesViewContext.tsx @@ -6,7 +6,6 @@ interface MessagesViewContextValue { conversation: ReturnType['conversation']; conversationId: string | null | undefined; - /** True when the view cannot mutate server state (shared/search); MCP App bridges render display-only. */ readOnly: boolean; /** Submission and control states */ @@ -117,11 +116,7 @@ export function useMessagesViewContext() { return context; } -/** - * True when MCP App bridges should be display-only: the shared view, the /search route, or any - * mount outside an interactive MessagesViewProvider. Defaults to read-only when no provider is - * present so a new render context never accidentally enables live, auth-bearing app actions. - */ +/** Defaults to read-only when no provider is present, so live auth-bearing app actions stay off. */ export function useIsMessagesViewReadOnly(): boolean { return useContext(MessagesViewContext)?.readOnly ?? true; } diff --git a/client/src/components/Chat/Messages/Content/ToolCall.tsx b/client/src/components/Chat/Messages/Content/ToolCall.tsx index 810e022df0..4e9fe5f7f5 100644 --- a/client/src/components/Chat/Messages/Content/ToolCall.tsx +++ b/client/src/components/Chat/Messages/Content/ToolCall.tsx @@ -77,9 +77,7 @@ const MCPAppView = React.memo(function MCPAppView({ } const isAppBacked = isMcpAppResource(app); - // Read-only views (shared/search) don't fetch app HTML from the viewer's MCP server, so a - // server-fetched (resourceUri-only) app cannot render; show a placeholder instead of a failing - // iframe. Inline apps (with persisted HTML) still render. + // Read-only views don't fetch app HTML, so a resourceUri-only app shows a placeholder. if (isAppBacked && !app.text && readOnly) { return (
diff --git a/client/src/components/Share/ShareMessagesProvider.tsx b/client/src/components/Share/ShareMessagesProvider.tsx index 81ee66effc..61eee26c3d 100644 --- a/client/src/components/Share/ShareMessagesProvider.tsx +++ b/client/src/components/Share/ShareMessagesProvider.tsx @@ -21,8 +21,7 @@ export function ShareMessagesProvider({ messages, children }: ShareMessagesProvi () => ({ conversation: null, conversationId: undefined, - // Share view is read-only: MCP App bridges must render display-only and never proxy - // auth-bearing tool calls or resource reads against the viewer's MCP servers. + // Read-only so app bridges never proxy auth-bearing calls against the viewer's MCP servers. readOnly: true, // These are required by the context but not used in share view ask: () => {}, diff --git a/client/src/hooks/MCP/useAppBridge.ts b/client/src/hooks/MCP/useAppBridge.ts index f7f90097b7..7a70ebfee2 100644 --- a/client/src/hooks/MCP/useAppBridge.ts +++ b/client/src/hooks/MCP/useAppBridge.ts @@ -35,9 +35,8 @@ export function useAppBridge( ) { const user = useRecoilValue(store.user); const { ask } = useOptionalMessagesOperations(); - // Shared transcripts and /search render read-only: the embedded app must not proxy tool calls or - // resource reads against the viewer's MCP servers with the viewer's auth. Such views render the - // app display-only (initial tool input/result still shown), with no host-bound action handlers. + // Read-only views (shared transcripts, /search) must not let the embedded app proxy tool calls + // or resource reads against the viewer's MCP servers with the viewer's auth. const readOnly = useIsMessagesViewReadOnly(); const queryClient = useQueryClient(); const bridgeRef = useRef(null); @@ -79,8 +78,8 @@ export function useAppBridge( const theme = document.documentElement.classList.contains('dark') ? 'dark' : 'light'; const { locale, timeZone } = Intl.DateTimeFormat().resolvedOptions(); - // Display-only views advertise no host-bound action capabilities, so a well-behaved app - // disables those affordances instead of issuing calls the host will ignore. + // Display-only views advertise no host-bound action capabilities so a well-behaved app + // disables those affordances rather than issuing calls the host ignores. const interactive = !readOnlyRef.current; bridge = new AppBridge( @@ -118,7 +117,7 @@ export function useAppBridge( }; // Host-bound actions (tool calls, resource reads/lists, model messages) run with the viewer's - // auth, so they are only wired in interactive views — never for shared transcripts or /search. + // auth, so they are only wired in interactive views, never in shared transcripts or /search. if (interactive) { bridge.oncalltool = async (params) => callMCPAppTool( @@ -153,8 +152,8 @@ export function useAppBridge( return; } sandboxReadyHandled = true; - // Read-only views (shared/search) must not resolve app HTML from the viewer's MCP server, - // so only inline (persisted) HTML renders here; resourceUri-only apps stay display-only. + // Read-only views must not resolve app HTML from the viewer's MCP server, so only inline + // (persisted) HTML renders here. if (!resource.text && readOnlyRef.current) { logger.debug( '[MCP App] Read-only view: skipping server HTML fetch for resourceUri-only app', diff --git a/packages/api/src/mcp/MCPManager.ts b/packages/api/src/mcp/MCPManager.ts index 8af04c7413..e364f126c3 100644 --- a/packages/api/src/mcp/MCPManager.ts +++ b/packages/api/src/mcp/MCPManager.ts @@ -79,9 +79,8 @@ export class MCPManager extends UserConnectionManager { */ private readonly toolCacheConnStamp = new Map(); /** - * Per-connection snapshot of the resource URIs and URI templates a server advertises, used to - * authorize app-driven `resources/read` so an embedded app can only proxy resources the server - * publicly exposes — not arbitrary `file://`/`secret://` URIs it happens to be reachable for. + * Snapshot of the resources a server advertises, used to authorize app-driven `resources/read` + * so an embedded app can only proxy publicly exposed resources, not arbitrary reachable URIs. */ private readonly advertisedResourceCache = new Map< string, @@ -89,7 +88,6 @@ export class MCPManager extends UserConnectionManager { >(); private readonly advertisedResourceConnStamp = new Map(); - /** Bounds the resources/list + templates/list pagination loops when snapshotting advertised resources. */ private static readonly RESOURCE_LIST_MAX_PAGES = 20; /** Creates and initializes the singleton MCPManager instance */ @@ -404,18 +402,16 @@ Please follow these instructions when using tools from the respective MCP server } /** - * App-level connections can be transparently recreated when a server config changes - * (ConnectionsRepository.get), so cached tool metadata is only valid while it was built - * from the current connection instance. + * App-level connections can be recreated when a server config changes, so cached tool metadata + * is only valid while it was built from the current connection instance. */ private connStamp(connection: MCPConnection): string { return `${connection.createdAt}:${connection.toolListVersion}`; } /** - * Freshness stamp for the advertised-resource cache: keyed on the connection instance (createdAt) - * and the resources/list_changed counter, so removed/added server resources re-authorize without - * waiting for a reconnect. + * Freshness stamp keyed on the connection instance and the resources/list_changed counter, so + * removed or added server resources re-authorize without waiting for a reconnect. */ private resourceConnStamp(connection: MCPConnection): string { return `${connection.createdAt}:${connection.resourceListVersion}`; @@ -453,8 +449,8 @@ Please follow these instructions when using tools from the respective MCP server if (isToolVisibilityModelOnly(tool)) { modelOnly.add(tool.name); } - // A malformed `_meta.ui.resourceUri` on one tool must not abort discovery for the whole - // server, so isolate the parse: a bad declaration only disables that tool's UI metadata. + // A malformed `_meta.ui.resourceUri` on one tool only disables that tool's UI metadata, + // never aborting discovery for the whole server. try { const uri = getToolUiResourceUri(tool); if (uri) { @@ -731,10 +727,9 @@ Please follow these instructions when using tools from the respective MCP server requiresEphemeralUserConnection(rawConfig), ); if (resourceMeta) { - // App-backed tool: honor the per-request `mcpSettings.apps` setting so a tenant that - // disabled apps gets no UI resource attached (it would otherwise render as a broken - // iframe once the gated app endpoints reject the follow-up calls). Resolved lazily here - // so ordinary, non-app tools skip the per-request lookup. + // Honor the per-request apps setting so a tenant that disabled apps gets no UI resource + // (it would otherwise render as a broken iframe once the gated app endpoints reject the + // follow-up calls). Resolved lazily so non-app tools skip the per-request lookup. const { appsEnabled } = await registry.resolveAllowlists({ userId, role: user?.role }); if (!appsEnabled) { resourceMeta = undefined; @@ -745,7 +740,7 @@ Please follow these instructions when using tools from the respective MCP server } } } catch { - // Non-critical -- tools render without the app UI + /* empty */ } } @@ -785,10 +780,6 @@ Please follow these instructions when using tools from the respective MCP server } } - /** - * Reads a UI resource from an MCP server. - * Used by MCP Apps iframes to fetch additional resources via the host. - */ /** * Resolves the same registry-backed config the original tool call used and hands it to * getConnection so config-source servers resolve, then refreshes headers for non-DB-sourced @@ -950,14 +941,15 @@ Please follow these instructions when using tools from the respective MCP server `${logPrefix} Resource "${uri}" is not permitted.`, ); } - // Exact advertised URIs are trusted as-is. A template match must additionally not resolve to a - // path-traversal URI, so a parameterized template can never authorize an unrelated resource. if (advertised.uris.has(uri)) { return; } + // Match templates in canonical (fully percent-decoded) space, never raw bytes, so an encoded + // traversal like `%2e%2e%2f` cannot slip past a template guard. + const canonicalUri = MCPManager.canonicalizeUri(uri); if ( - !uri.split('/').includes('..') && - advertised.templates.some((pattern) => pattern.test(uri)) + canonicalUri != null && + advertised.templates.some((pattern) => pattern.test(canonicalUri)) ) { return; } @@ -1007,7 +999,16 @@ Please follow these instructions when using tools from the respective MCP server { timeout: connection.timeout }, ); for (const template of result.resourceTemplates) { - const pattern = MCPManager.uriTemplateToRegExp(template.uriTemplate); + // Compile templates in the same decoded space the requested URI is canonicalized into, + // so matching is encoding-agnostic; fall back to the raw template if it is not valid + // percent-encoding. + let templateStr = template.uriTemplate; + try { + templateStr = decodeURIComponent(templateStr); + } catch { + /* keep raw template */ + } + const pattern = MCPManager.uriTemplateToRegExp(templateStr); if (pattern) { templates.push(pattern); } @@ -1030,6 +1031,31 @@ Please follow these instructions when using tools from the respective MCP server return entry; } + /** + * Fully percent-decodes a URI to the canonical form a server resolves. Returns null when it + * cannot be decoded or contains a relative (`.`/`..`) segment, so neither encoded traversal nor + * relative segments can satisfy a template guard. + */ + private static canonicalizeUri(uri: string): string | null { + let current = uri; + for (let depth = 0; depth < 5; depth++) { + let decoded: string; + try { + decoded = decodeURIComponent(current); + } catch { + return null; + } + if (decoded === current) { + break; + } + current = decoded; + } + if (current.split(/[/\\]/).some((segment) => segment === '.' || segment === '..')) { + return null; + } + return current; + } + /** * Converts an RFC 6570 resource URI template into an anchored matcher. Simple expansions match a * single path segment; reserved/operator expansions (`{+x}`, `{#x}`, `{/x}`, ...) may span `/`. diff --git a/packages/api/src/mcp/__tests__/MCPManager.test.ts b/packages/api/src/mcp/__tests__/MCPManager.test.ts index e416fa1224..20dc87954f 100644 --- a/packages/api/src/mcp/__tests__/MCPManager.test.ts +++ b/packages/api/src/mcp/__tests__/MCPManager.test.ts @@ -1455,6 +1455,32 @@ describe('MCPManager', () => { const methods = request.mock.calls.map((c) => (c[0] as { method: string }).method); expect(methods).not.toContain('resources/read'); }); + + it('rejects a percent-encoded path traversal even when a broad template would match', 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: [{ uriTemplate: 'file://docs{+path}' }] }); + } + return Promise.resolve({ contents: [] }); + }); + const manager = await MCPManager.createInstance(newMCPServersConfig()); + jest.spyOn(manager, 'getConnection').mockResolvedValue(buildConnection(request)); + + await expect( + manager.readResource({ + userId: 'user-123', + serverName: 'srv', + uri: 'file://docs/%2e%2e%2fsecret', + user: mockUser as IUser, + }), + ).rejects.toThrow(/not advertised/); + + const methods = request.mock.calls.map((c) => (c[0] as { method: string }).method); + expect(methods).not.toContain('resources/read'); + }); }); describe('getConnection', () => { diff --git a/packages/api/src/mcp/apps.ts b/packages/api/src/mcp/apps.ts index 0a61a82154..63cd022e4c 100644 --- a/packages/api/src/mcp/apps.ts +++ b/packages/api/src/mcp/apps.ts @@ -1,8 +1,6 @@ /** - * MCP Apps tool-metadata helpers, mirrored from the spec's reference - * implementation in `@modelcontextprotocol/ext-apps`. They are reimplemented - * here so `@librechat/api` (emitted as CommonJS) never statically imports the - * ESM-only ext-apps package; the client bundle keeps importing ext-apps directly. + * Reimplemented here so `@librechat/api` (emitted as CommonJS) never statically + * imports the ESM-only `@modelcontextprotocol/ext-apps` package. */ import { McpError, ErrorCode } from '@modelcontextprotocol/sdk/types.js'; @@ -22,17 +20,10 @@ interface McpUiToolMeta { visibility?: McpUiToolVisibility[]; } -/** Deprecated flat metadata key for a tool's UI resource URI. */ export const RESOURCE_URI_META_KEY = 'ui/resourceUri'; -/** MIME type identifying HTML content as an MCP App UI resource. */ export const RESOURCE_MIME_TYPE = 'text/html;profile=mcp-app'; -/** - * Extract a tool's UI resource URI. Prefers the nested `_meta.ui.resourceUri` - * format and falls back to the deprecated flat `_meta["ui/resourceUri"]`. - * Throws if a URI is present but does not use the `ui://` scheme. - */ export function getToolUiResourceUri(tool: ToolWithMeta): string | undefined { const uiMeta = tool._meta?.ui as McpUiToolMeta | undefined; let uri: unknown = uiMeta?.resourceUri; @@ -49,23 +40,17 @@ export function getToolUiResourceUri(tool: ToolWithMeta): string | undefined { return undefined; } -/** True when a tool is exposed to the model only (never callable from an app). */ export function isToolVisibilityModelOnly(tool: ToolWithMeta): boolean { const visibility = (tool._meta?.ui as McpUiToolMeta | undefined)?.visibility; return Array.isArray(visibility) && visibility.length === 1 && visibility[0] === 'model'; } -/** True when a tool is exposed to the app only (hidden from the model). */ export function isToolVisibilityAppOnly(tool: ToolWithMeta): boolean { const visibility = (tool._meta?.ui as McpUiToolMeta | undefined)?.visibility; return Array.isArray(visibility) && visibility.length === 1 && visibility[0] === 'app'; } -/** - * Structural manager interface backing the MCP App proxy services. Declared here - * rather than importing MCPManager so this module stays free of a circular import - * (MCPManager imports the helpers above). The argument shapes mirror MCPManager. - */ +/** Declared here rather than importing MCPManager to avoid a circular import. */ export interface MCPAppsProxyManager { readResource(args: { userId: string; @@ -110,7 +95,6 @@ export interface MCPAppsProxyManager { }): Promise; } -/** Request-scoped context shared by every MCP App proxy service. */ export interface MCPAppRequestContext { userId: string; serverName: string; @@ -121,7 +105,6 @@ export interface MCPAppRequestContext { tokenMethods?: TokenMethods; } -/** Reads an MCP App resource after validating the server name and uri. */ export async function readAppResource( manager: MCPAppsProxyManager, ctx: MCPAppRequestContext, @@ -145,7 +128,6 @@ export async function readAppResource( }); } -/** Lists MCP App resources after validating the server name and optional cursor. */ export async function listAppResources( manager: MCPAppsProxyManager, ctx: MCPAppRequestContext, @@ -169,7 +151,6 @@ export async function listAppResources( }); } -/** Lists MCP App resource templates after validating the server name and optional cursor. */ export async function listAppResourceTemplates( manager: MCPAppsProxyManager, ctx: MCPAppRequestContext, @@ -193,7 +174,6 @@ export async function listAppResourceTemplates( }); } -/** Proxies an MCP App tool call after validating the server name, tool name, and arguments. */ export async function callAppTool( manager: MCPAppsProxyManager, ctx: MCPAppRequestContext, diff --git a/packages/api/src/mcp/connection.ts b/packages/api/src/mcp/connection.ts index c5ccb9898b..24a4b7157e 100644 --- a/packages/api/src/mcp/connection.ts +++ b/packages/api/src/mcp/connection.ts @@ -1147,9 +1147,8 @@ export class MCPConnection extends EventEmitter { public toolListVersion = 0; /** - * Bumped on every resources/list_changed notification. Consumers that cache the set of resources - * a server advertises fold this into their freshness check so cached authorization data is - * refreshed when the server adds or removes resources on a live connection. + * Bumped on every resources/list_changed notification so consumers caching a server's advertised + * resources refresh their authorization data when resources are added or removed. */ public resourceListVersion = 0; @@ -1267,9 +1266,8 @@ export class MCPConnection extends EventEmitter { if (params.oauthTokens) { this.oauthTokens = params.oauthTokens; } - // Advertise MCP Apps support so servers using the ext-apps graceful-degradation path - // (getUiCapability) expose app-enhanced tools. The capability rides on the `extensions` - // field keyed by ext-apps EXTENSION_ID. Suppressed when MCP Apps are disabled by config. + // Advertise the UI capability so servers expose app-enhanced tools; suppressed when MCP Apps + // are disabled by config. const appsEnabled = params.enableApps !== false; const capabilities: ClientCapabilities = appsEnabled ? { extensions: { 'io.modelcontextprotocol/ui': { mimeTypes: [RESOURCE_MIME_TYPE] } } } diff --git a/packages/api/src/mcp/registry/MCPServersRegistry.ts b/packages/api/src/mcp/registry/MCPServersRegistry.ts index 4762464872..ada1231358 100644 --- a/packages/api/src/mcp/registry/MCPServersRegistry.ts +++ b/packages/api/src/mcp/registry/MCPServersRegistry.ts @@ -251,9 +251,8 @@ export class MCPServersRegistry { }> { let allowedDomains = this.allowedDomains; let allowedAddresses = this.allowedAddresses; - // MCP Apps, like the allowlists, are tenant/principal-scoped: resolve the per-request value so a - // tenant/role/user override of `mcpSettings.apps` is honored. Inherit the YAML base when the - // resolver omits it; fall back to the base entirely if the resolver is absent or fails. + // Apps are tenant/principal-scoped, so honor a per-request override of `mcpSettings.apps`, + // falling back to the YAML base when the resolver omits it, is absent, or fails. let appsEnabled = this.getAppsEnabled(); if (this.allowlistResolver) { try {