From ae0e98e4c9e16bb9bb35d4bebc0cd2f5c496edbb Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Tue, 23 Jun 2026 21:16:34 -0700 Subject: [PATCH] fix(mcp): address Codex round-3 findings declined in error Re-verification against source showed five previously declined Codex threads had merit, so this implements the fixes rather than the incorrect push-backs. Restores rendering of inline ui:// text resources attached to a tool call. The selector in ToolCall excluded text-bearing resources and ToolCallInfo no longer received attachments, so an inline resource vanished unless the model echoed a marker. MCPAppView now renders every renderable attachment and routes text resources through its srcDoc branch. Wires metadata cache invalidation. clearResourceUriCache had no caller, so the resourceUri, modelOnly and knownToolNames caches were never cleared. Tearing down a user connection now evicts that server/user cache entry. Threads the original registry config through the iframe follow-up RPCs. readResource and appToolCall share a getAppConnection helper that resolves the server config, hands it to getConnection so config-source servers resolve, refreshes headers for non-DB-sourced servers, and rejects configs that still need request body placeholders the app context cannot supply. Completes the host side of the App Bridge so migrated mcp-ui apps keep working: advertises serverResources and sets onreadresource for lazy resource loads, and wires onmessage so ui/message (the standardized replacement for mcp-ui prompt and intent actions) reaches the conversation. Honors a dedicated sandbox origin. getMCPSandboxUrl passes the parent origin to the sandbox, which the proxy reads from the parentOrigin query param so the handshake targets LibreChat instead of the sandbox's own origin. --- client/public/mcp-sandbox.html | 15 ++- .../Chat/Messages/Content/ToolCall.tsx | 10 +- .../Content/__tests__/ToolCall.test.tsx | 30 +++++ .../__tests__/UIResourceCarousel.test.tsx | 1 - client/src/hooks/MCP/useAppBridge.ts | 26 ++++- client/src/utils/mcpApps.ts | 10 +- packages/api/src/mcp/MCPManager.ts | 108 ++++++++++++------ .../api/src/mcp/__tests__/MCPManager.test.ts | 24 ++++ 8 files changed, 180 insertions(+), 44 deletions(-) diff --git a/client/public/mcp-sandbox.html b/client/public/mcp-sandbox.html index 254a0df60f..a6e87b0122 100644 --- a/client/public/mcp-sandbox.html +++ b/client/public/mcp-sandbox.html @@ -18,9 +18,18 @@ let readyInterval = null; const SANDBOX_PREFIX = 'ui/notifications/sandbox-'; - // The sandbox is always served same-origin with LibreChat, so window.location.origin - // is the exact expected parent origin. No referrer fallback or lazy-set needed. - const trustedOrigin = window.location.origin; + // Default to same-origin. When the sandbox is served from a dedicated origin, the parent + // passes its origin via the parentOrigin query param so the handshake targets LibreChat + // rather than the sandbox's own origin. + const trustedOrigin = (function () { + try { + const param = new URLSearchParams(window.location.search).get('parentOrigin'); + if (param) { + return new URL(param).origin; + } + } catch (e) {} + return window.location.origin; + })(); function notifyReady() { window.parent.postMessage( diff --git a/client/src/components/Chat/Messages/Content/ToolCall.tsx b/client/src/components/Chat/Messages/Content/ToolCall.tsx index 02cc767a0a..ba134f96c7 100644 --- a/client/src/components/Chat/Messages/Content/ToolCall.tsx +++ b/client/src/components/Chat/Messages/Content/ToolCall.tsx @@ -267,12 +267,15 @@ export default function ToolCall({ [args, output], ); - const mcpApp = useMemo(() => { + const mcpApps = useMemo(() => { const uiResources: UIResource[] = attachments ?.filter((a) => a.type === Tools.ui_resources) .flatMap((a) => (a[Tools.ui_resources] ?? []) as UIResource[]) ?? []; - return uiResources.find((r) => r.toolName && r.serverName && !r.text) ?? null; + return uiResources.filter( + (r) => + (r.toolName && r.serverName) || (r.text && (r.mimeType ?? 'text/html').includes('html')), + ); }, [attachments]); const authDomain = useMemo(() => { @@ -388,7 +391,8 @@ export default function ToolCall({ {!hideAttachments && attachments && attachments.length > 0 && ( )} - {mcpApp && hasOutput && } + {hasOutput && + mcpApps.map((app) => )} ); } diff --git a/client/src/components/Chat/Messages/Content/__tests__/ToolCall.test.tsx b/client/src/components/Chat/Messages/Content/__tests__/ToolCall.test.tsx index 8b7184babc..c519198365 100644 --- a/client/src/components/Chat/Messages/Content/__tests__/ToolCall.test.tsx +++ b/client/src/components/Chat/Messages/Content/__tests__/ToolCall.test.tsx @@ -35,6 +35,7 @@ jest.mock('~/hooks', () => ({ jest.mock('~/hooks/MCP', () => ({ useMCPIconMap: () => new Map(), + useAppBridge: jest.fn(), })); jest.mock('~/components/Chat/Messages/Content/MessageContent', () => ({ @@ -175,6 +176,35 @@ describe('ToolCall', () => { const attachmentGroup = screen.getByTestId('attachment-group'); expect(JSON.parse(attachmentGroup.textContent!)).toEqual(attachments); }); + + it('renders an iframe for an inline ui:// text resource attached to the tool call', () => { + const attachments = [ + { + type: Tools.ui_resources, + messageId: 'msg1', + toolCallId: 'tool1', + conversationId: 'conv1', + [Tools.ui_resources]: [ + { + uri: 'ui://test-server/inline.html', + mimeType: 'text/html', + text: '

inline resource

', + resourceId: 'inline-1', + toolName: 'test-tool', + serverName: 'test-server', + }, + ], + }, + ]; + + const { container } = renderWithRecoil( + , + ); + + const iframe = container.querySelector('iframe[srcdoc]'); + expect(iframe).toBeInTheDocument(); + expect(iframe).toHaveAttribute('srcdoc', '

inline resource

'); + }); }); describe('attachment group rendering', () => { diff --git a/client/src/components/Chat/Messages/Content/__tests__/UIResourceCarousel.test.tsx b/client/src/components/Chat/Messages/Content/__tests__/UIResourceCarousel.test.tsx index d26b7f85ff..082ee66d3d 100644 --- a/client/src/components/Chat/Messages/Content/__tests__/UIResourceCarousel.test.tsx +++ b/client/src/components/Chat/Messages/Content/__tests__/UIResourceCarousel.test.tsx @@ -23,7 +23,6 @@ Object.defineProperty(HTMLElement.prototype, 'scrollTo', { const makeResource = (n: number): UIResource => ({ uri: `resource${n}`, mimeType: 'text/html', - text: `Resource ${n}`, resourceId: `r${n}`, toolName: 'test-tool', serverName: 'test-server', diff --git a/client/src/hooks/MCP/useAppBridge.ts b/client/src/hooks/MCP/useAppBridge.ts index e2aac894ff..ba98be4c80 100644 --- a/client/src/hooks/MCP/useAppBridge.ts +++ b/client/src/hooks/MCP/useAppBridge.ts @@ -6,10 +6,13 @@ import { buildAllowAttribute, } from '@modelcontextprotocol/ext-apps/app-bridge'; import type { UIResource } from 'librechat-data-provider'; -import { callMCPAppTool, fetchMCPResourceHtml } from '~/utils/mcpApps'; +import { callMCPAppTool, fetchMCPResourceHtml, readMCPResource } from '~/utils/mcpApps'; +import { useOptionalMessagesOperations } from '~/Providers'; import { logger } from '~/utils'; import store from '~/store'; +type MessageContentBlock = { type?: string; text?: string }; + type SizeParams = { width?: number; height?: number }; export function useAppBridge( @@ -20,7 +23,12 @@ export function useAppBridge( onSizeChanged: (params: SizeParams) => void, ) { const user = useRecoilValue(store.user); + const { ask } = useOptionalMessagesOperations(); const bridgeRef = useRef(null); + const askRef = useRef(ask); + useEffect(() => { + askRef.current = ask; + }); // Refs keep latest values accessible inside the stable effect closure without triggering remount. // The bridge mounts once per resourceId; toolArgs/toolResult/onSizeChanged are captured at @@ -56,7 +64,7 @@ export function useAppBridge( bridge = new AppBridge( null, { name: 'LibreChat', version: '1.0.0' }, - { openLinks: {}, serverTools: {}, logging: {} }, + { openLinks: {}, serverTools: {}, serverResources: {}, logging: {} }, { hostContext: { theme, @@ -81,6 +89,20 @@ export function useAppBridge( return {}; }; + bridge.onreadresource = async (params) => + readMCPResource(resource.serverName as string, params.uri, user?.id) as never; + + bridge.onmessage = async ({ content }) => { + const text = (content as MessageContentBlock[]) + .filter((block) => block.type === 'text' && typeof block.text === 'string') + .map((block) => block.text) + .join('\n'); + if (text) { + askRef.current({ text }); + } + return {}; + }; + bridge.addEventListener('sandboxready', async () => { try { const { html, csp, permissions } = await fetchMCPResourceHtml( diff --git a/client/src/utils/mcpApps.ts b/client/src/utils/mcpApps.ts index 6c354f2472..78baeafefa 100644 --- a/client/src/utils/mcpApps.ts +++ b/client/src/utils/mcpApps.ts @@ -2,8 +2,14 @@ import { request, apiBaseUrl } from 'librechat-data-provider'; export function getMCPSandboxUrl(): string { const configured = (import.meta.env as Record).VITE_MCP_SANDBOX_URL; - if (configured) return configured; - return `${window.location.origin}${apiBaseUrl()}/api/mcp/sandbox`; + const base = configured ?? `${window.location.origin}${apiBaseUrl()}/api/mcp/sandbox`; + try { + const url = new URL(base, window.location.origin); + url.searchParams.set('parentOrigin', window.location.origin); + return url.toString(); + } catch { + return base; + } } export async function callMCPAppTool( diff --git a/packages/api/src/mcp/MCPManager.ts b/packages/api/src/mcp/MCPManager.ts index da8341b3a8..1a143b8684 100644 --- a/packages/api/src/mcp/MCPManager.ts +++ b/packages/api/src/mcp/MCPManager.ts @@ -347,7 +347,14 @@ ${formattedInstructions} Please follow these instructions when using tools from the respective MCP servers.`; } - public clearResourceUriCache(serverName?: string): void { + public clearResourceUriCache(serverName?: string, userId?: string): void { + if (serverName && userId != null) { + const cacheKey = `${serverName}:${userId}`; + this.resourceUriCache.delete(cacheKey); + this.modelOnlyToolCache.delete(cacheKey); + this.knownToolNamesCache.delete(cacheKey); + return; + } if (serverName) { for (const key of this.resourceUriCache.keys()) { if (key === serverName || key.startsWith(`${serverName}:`)) { @@ -363,6 +370,11 @@ Please follow these instructions when using tools from the respective MCP server } } + protected removeUserConnection(userId: string, serverName: string): void { + this.clearResourceUriCache(serverName, userId); + super.removeUserConnection(userId, serverName); + } + private async populateToolCaches(connection: MCPConnection, cacheKey: string): Promise { const tools = await connection.fetchTools(); const serverMap = new Map< @@ -652,6 +664,66 @@ 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 + * servers. Iframe follow-up requests arrive without the original requestBody, so configs that + * still need runtime body placeholders are rejected rather than connected with unresolved values. + */ + private async getAppConnection({ + serverName, + userId, + user, + }: { + serverName: string; + userId: string; + user?: IUser; + }): Promise { + const logPrefix = `[MCP][User: ${userId}][${serverName}]`; + const rawConfig = await MCPServersRegistry.getInstance().getServerConfig(serverName, userId); + const isDbSourced = rawConfig ? isUserSourced(rawConfig) : false; + if (rawConfig) { + if (rawConfig.obo) { + throw new McpError( + ErrorCode.InvalidRequest, + `${logPrefix} Server "${serverName}" requires per-call OBO token resolution which is not supported for app requests.`, + ); + } + if (!isDbSourced && mcpOptionsContainGraphTokenPlaceholder(rawConfig as t.MCPOptions)) { + throw new McpError( + ErrorCode.InvalidRequest, + `${logPrefix} Server "${serverName}" requires Graph API token resolution which is not supported for app requests.`, + ); + } + const missingBodyFields = getMissingRuntimeBodyPlaceholderFields(rawConfig); + if (missingBodyFields.length > 0) { + throw new McpError( + ErrorCode.InvalidRequest, + `${logPrefix} Server "${serverName}" requires request body field(s) (${missingBodyFields.join(', ')}) that are not available for app requests.`, + ); + } + } + + const connection = await this.getConnection({ + serverName, + user, + serverConfig: rawConfig ?? undefined, + }); + + if (rawConfig && !isDbSourced) { + const currentOptions = processMCPEnv({ + user, + dbSourced: false, + options: rawConfig as t.MCPOptions, + }); + const resolvedHeaders: Record = + 'headers' in currentOptions ? { ...(currentOptions.headers || {}) } : {}; + connection.setRequestHeaders(resolvedHeaders); + } + + return connection; + } + async readResource({ userId, serverName, @@ -665,7 +737,7 @@ Please follow these instructions when using tools from the respective MCP server }): Promise { const logPrefix = `[MCP][User: ${userId}][${serverName}]`; if (userId && user) this.updateUserLastActivity(userId); - const connection = await this.getConnection({ serverName, user }); + const connection = await this.getAppConnection({ serverName, userId, user }); if (!(await connection.isConnected())) { throw new McpError( @@ -705,7 +777,7 @@ Please follow these instructions when using tools from the respective MCP server }): Promise { const logPrefix = `[MCP][User: ${userId}][${serverName}]`; if (userId && user) this.updateUserLastActivity(userId); - const connection = await this.getConnection({ serverName, user }); + const connection = await this.getAppConnection({ serverName, userId, user }); if (!(await connection.isConnected())) { throw new McpError( @@ -732,36 +804,6 @@ Please follow these instructions when using tools from the respective MCP server ); } - const rawConfig = await MCPServersRegistry.getInstance().getServerConfig(serverName, userId); - if (rawConfig) { - if (rawConfig.obo) { - throw new McpError( - ErrorCode.InvalidRequest, - `${logPrefix} Server "${serverName}" requires per-call OBO token resolution which is not supported for app tool calls.`, - ); - } - const isDbSourced = isUserSourced(rawConfig); - if (!isDbSourced && mcpOptionsContainGraphTokenPlaceholder(rawConfig as t.MCPOptions)) { - throw new McpError( - ErrorCode.InvalidRequest, - `${logPrefix} Server "${serverName}" requires Graph API token resolution which is not supported for app tool calls.`, - ); - } - // DB-sourced servers have their customUserVars (e.g. {{MCP_API_KEY}}) resolved during - // the original callTool setup. Re-processing without customUserVars would overwrite - // those resolved headers with unresolved placeholders, so skip for DB-sourced servers. - if (!isDbSourced) { - const currentOptions = processMCPEnv({ - user, - dbSourced: false, - options: rawConfig as t.MCPOptions, - }); - const resolvedHeaders: Record = - 'headers' in currentOptions ? { ...(currentOptions.headers || {}) } : {}; - connection.setRequestHeaders(resolvedHeaders); - } - } - const result = await connection.client.request( { method: 'tools/call', diff --git a/packages/api/src/mcp/__tests__/MCPManager.test.ts b/packages/api/src/mcp/__tests__/MCPManager.test.ts index 865de69eb4..665d0681c5 100644 --- a/packages/api/src/mcp/__tests__/MCPManager.test.ts +++ b/packages/api/src/mcp/__tests__/MCPManager.test.ts @@ -1182,6 +1182,30 @@ describe('MCPManager', () => { }); }); + describe('appToolCall - app request context', () => { + const mockUser: Partial = { id: 'user-123' }; + + it('rejects when the server config needs request body placeholders unavailable to app calls', async () => { + (mockRegistryInstance.getServerConfig as jest.Mock).mockResolvedValue({ + source: 'yaml', + type: 'sse', + url: 'https://example.com/{{LIBRECHAT_BODY_CONVERSATIONID}}/mcp', + }); + + const manager = await MCPManager.createInstance(newMCPServersConfig()); + + await expect( + manager.appToolCall({ + userId: 'user-123', + serverName: 'body-server', + toolName: 'do_thing', + toolArguments: {}, + user: mockUser as IUser, + }), + ).rejects.toThrow(/request body field/); + }); + }); + describe('getConnection', () => { const mockUser: Partial = { id: 'user-123',