fix(mcp): address second round of Codex review findings

Fixes 13 correctness issues flagged in the second Codex review pass on the
feat/mcp-apps-support branch.

Core server-side changes: resource URI and model-only-tool caches are now
scoped per user/server key so OAuth and user-sourced servers with differing
tool lists cannot cross-contaminate each other. The model-only visibility check
in appToolCall now blocks iframe-initiated calls to tools declared as
visibility: ['model']. appToolCall also runs processMCPEnv to resolve runtime
env/user vars and set request headers before forwarding to tools/call, and
throws for servers that require per-call OBO token minting (unsupported in this
path). parsers.ts now includes structuredContent in the synthetic resourceId
hash to guarantee uniqueness across repeated same-app calls with different
results, skips the early-return guard when a synthetic app resource is present,
appends the ui{} marker to the synthetic text block, and forwards the raw
content array alongside structuredContent so text/image-only app results are
not silently dropped.

Client-side changes: fetchMCPResourceHtml now returns the full _meta.ui from
the resources/read content item so CSP and permissions come from the canonical
location in the spec rather than the tool descriptor. useAppBridge falls back
to the resource-level values when the read result carries no overrides.
The sandbox retry interval clears when sandbox-resource-ready arrives, fixing
the race where the ready notification arrived before the transport was
connected. The size-change handler in MCPUIResource and UIResourceCarousel now
applies the reported height to the wrapper element, and MCPUIResource's iframe
style uses height: 100% so inline apps are not clipped. The carousel loading
placeholder now uses the localized key. Dockerfile.multi copies the sandbox
from client/dist (the Vite output) rather than the source tree, which is the
only path present in the multi-stage runtime image. baseUriDomains from the
CSP config are now honoured in buildCspPolicy instead of always emitting
base-uri 'self'. serverResources was removed from the AppBridge capabilities
advertisement because no resource handlers are registered on the bridge.
This commit is contained in:
Dustin Healy 2026-06-23 18:18:51 -07:00
parent 4da55e8178
commit d65c228cea
11 changed files with 167 additions and 43 deletions

View file

@ -114,6 +114,7 @@ COPY --from=data-provider-build /app/packages/data-provider/dist ./packages/data
COPY --from=data-schemas-build /app/packages/data-schemas/dist ./packages/data-schemas/dist
COPY --from=api-package-build /app/packages/api/dist ./packages/api/dist
COPY --from=client-build /app/client/dist ./client/dist
COPY --from=client-build /app/client/dist/mcp-sandbox.html ./client/public/mcp-sandbox.html
# Propagate build metadata into runtime env so /api/config can expose it.
# Declared here (after the heavy install/copy steps) so that commit/date
# changing on every CI run does not bust the cache for those layers.

View file

@ -16,6 +16,7 @@
let innerFrame = null;
let innerFrameBlobUrl = null;
let trustedOrigin = null;
let readyInterval = null;
const SANDBOX_PREFIX = 'ui/notifications/sandbox-';
function notifyReady() {
@ -23,6 +24,16 @@
{ jsonrpc: '2.0', method: 'ui/notifications/sandbox-proxy-ready', params: {} },
'*'
);
if (!readyInterval) {
readyInterval = setInterval(() => {
if (!innerFrame) {
window.parent.postMessage(
{ jsonrpc: '2.0', method: 'ui/notifications/sandbox-proxy-ready', params: {} },
'*'
);
}
}, 500);
}
}
window.addEventListener('message', (event) => {
@ -42,6 +53,8 @@
}
if (msg.method === 'ui/notifications/sandbox-resource-ready') {
clearInterval(readyInterval);
readyInterval = null;
createInnerFrame(msg.params);
return;
}
@ -140,7 +153,7 @@
("font-src " + (resourceDomains || "'none'")).trim(),
"frame-src " + frameDomains,
"object-src 'none'",
"base-uri 'self'"
"base-uri " + (toDomainList(csp.baseUriDomains) || "'self'")
].join('; ');
}

View file

@ -51,9 +51,14 @@ const MCPAppView = React.memo(function MCPAppView({
const toolResult = useMemo(() => {
const sc = app.structuredContent as Record<string, unknown> | undefined | null;
if (!sc || typeof sc !== 'object' || Array.isArray(sc)) return undefined;
return { content: [] as [], structuredContent: sc };
}, [app.structuredContent]);
const content = (app.content as [] | undefined) ?? [];
if ((!sc || typeof sc !== 'object' || Array.isArray(sc)) && content.length === 0)
return undefined;
return {
content,
...(sc && typeof sc === 'object' && !Array.isArray(sc) ? { structuredContent: sc } : {}),
};
}, [app.structuredContent, app.content]);
const handleSizeChanged = useCallback((params: { height?: number; width?: number }) => {
if (params.height && params.height > 0) {

View file

@ -2,6 +2,7 @@ import React, { useState } from 'react';
import type { UIResource } from 'librechat-data-provider';
import { getMCPSandboxUrl } from '~/utils/mcpApps';
import { useAppBridge } from '~/hooks/MCP';
import { useLocalize } from '~/hooks';
interface UIResourceCarouselProps {
uiResources: UIResource[];
@ -9,14 +10,20 @@ interface UIResourceCarouselProps {
function MCPAppCard({ resource }: { resource: UIResource }) {
const iframeRef = React.useRef<HTMLIFrameElement>(null);
const localize = useLocalize();
const [loaded, setLoaded] = useState(false);
const sandboxUrl = React.useMemo(() => getMCPSandboxUrl(), []);
const toolResult = React.useMemo(() => {
const sc = resource.structuredContent as Record<string, unknown> | undefined | null;
if (!sc || typeof sc !== 'object' || Array.isArray(sc)) return undefined;
return { content: [] as [], structuredContent: sc };
}, [resource.structuredContent]);
const content = (resource.content as [] | undefined) ?? [];
if ((!sc || typeof sc !== 'object' || Array.isArray(sc)) && content.length === 0)
return undefined;
return {
content,
...(sc && typeof sc === 'object' && !Array.isArray(sc) ? { structuredContent: sc } : {}),
};
}, [resource.structuredContent, resource.content]);
const handleSizeChanged = React.useCallback((params: { height?: number; width?: number }) => {
if (params.height && params.height > 0) {
@ -31,7 +38,7 @@ function MCPAppCard({ resource }: { resource: UIResource }) {
<>
{!loaded && (
<div className="flex h-full items-center justify-center rounded-lg border border-border-light bg-surface-secondary text-sm text-text-secondary">
Loading interactive view...
{localize('com_ui_loading_interactive_view')}
</div>
)}
<iframe

View file

@ -25,16 +25,23 @@ export function MCPUIResource(props: MCPUIResourceProps) {
const iframeRef = useRef<HTMLIFrameElement>(null);
const [loaded, setLoaded] = useState(false);
const [height, setHeight] = useState<number | undefined>(undefined);
const sandboxUrl = useMemo(() => getMCPSandboxUrl(), []);
const toolResult = useMemo(() => {
const sc = uiResource?.structuredContent as Record<string, unknown> | undefined | null;
if (!sc || typeof sc !== 'object' || Array.isArray(sc)) return undefined;
return { content: [] as [], structuredContent: sc };
}, [uiResource?.structuredContent]);
const content = (uiResource?.content as [] | undefined) ?? [];
if ((!sc || typeof sc !== 'object' || Array.isArray(sc)) && content.length === 0)
return undefined;
return {
content,
...(sc && typeof sc === 'object' && !Array.isArray(sc) ? { structuredContent: sc } : {}),
};
}, [uiResource?.structuredContent, uiResource?.content]);
const handleSizeChanged = useCallback((params: { height?: number; width?: number }) => {
if (params.height && params.height > 0) {
setHeight(params.height);
setLoaded(true);
}
}, []);
@ -54,7 +61,10 @@ export function MCPUIResource(props: MCPUIResourceProps) {
try {
if (uiResource.toolName && uiResource.serverName && !uiResource.text) {
return (
<span className="mx-1 inline-block w-full align-middle">
<span
className="mx-1 inline-block w-full align-middle"
style={height ? { height } : { minHeight: '200px' }}
>
{!loaded && (
<div className="flex items-center gap-2 rounded-lg border border-border-light bg-surface-secondary px-4 py-3 text-sm text-text-secondary">
{localize('com_ui_loading_interactive_view')}
@ -66,7 +76,7 @@ export function MCPUIResource(props: MCPUIResourceProps) {
sandbox="allow-scripts allow-forms"
style={{
width: '100%',
minHeight: '200px',
height: '100%',
border: 'none',
display: loaded ? 'block' : 'none',
}}

View file

@ -39,7 +39,7 @@ export function useAppBridge(
bridge = new AppBridge(
null,
{ name: 'LibreChat', version: '1.0.0' },
{ openLinks: {}, serverTools: {}, serverResources: {}, logging: {} },
{ openLinks: {}, serverTools: {}, logging: {} },
{
hostContext: {
theme,
@ -66,15 +66,15 @@ export function useAppBridge(
bridge.addEventListener('sandboxready', async () => {
try {
const html = await fetchMCPResourceHtml(
const { html, csp, permissions } = await fetchMCPResourceHtml(
resource.serverName as string,
resource.uri,
user?.id,
);
await bridge!.sendSandboxResourceReady({
html,
csp: resource.csp as never,
permissions: resource.permissions as never,
csp: (csp ?? resource.csp) as never,
permissions: (permissions ?? resource.permissions) as never,
sandbox: 'allow-scripts allow-forms',
});
} catch (err) {

View file

@ -46,13 +46,38 @@ export async function readMCPResource(serverName: string, uri: string, userId?:
return promise;
}
type ResourceUiMeta = {
csp?: {
connectDomains?: string[];
resourceDomains?: string[];
frameDomains?: string[];
baseUriDomains?: string[];
};
permissions?: {
camera?: Record<string, never>;
microphone?: Record<string, never>;
geolocation?: Record<string, never>;
clipboardWrite?: Record<string, never>;
};
};
export async function fetchMCPResourceHtml(
serverName: string,
uri: string,
userId?: string,
): Promise<string> {
): Promise<{
html: string;
csp?: ResourceUiMeta['csp'];
permissions?: ResourceUiMeta['permissions'];
}> {
const result = (await readMCPResource(serverName, uri, userId)) as {
contents?: Array<{ text?: string }>;
contents?: Array<{ text?: string; _meta?: { ui?: ResourceUiMeta } }>;
};
const item = result?.contents?.[0];
const uiMeta = item?._meta?.ui;
return {
html: item?.text ?? '',
csp: uiMeta?.csp,
permissions: uiMeta?.permissions,
};
return result?.contents?.[0]?.text ?? '';
}

View file

@ -115,6 +115,7 @@
"@keyv/redis": "^4.3.3",
"@librechat/agents": "^3.2.44",
"@librechat/data-schemas": "*",
"@modelcontextprotocol/ext-apps": "^1.7.4",
"@modelcontextprotocol/sdk": "^1.29.0",
"@opentelemetry/api": "^1.9.0",
"@opentelemetry/instrumentation-express": "^0.56.0",

View file

@ -1,7 +1,10 @@
import pick from 'lodash/pick';
import { logger } from '@librechat/data-schemas';
import { Permissions, PermissionTypes } from 'librechat-data-provider';
import { getToolUiResourceUri } from '@modelcontextprotocol/ext-apps/app-bridge';
import {
getToolUiResourceUri,
isToolVisibilityModelOnly,
} from '@modelcontextprotocol/ext-apps/app-bridge';
import {
CallToolResultSchema,
ReadResourceResultSchema,
@ -64,6 +67,8 @@ export class MCPManager extends UserConnectionManager {
Map<string, { uri: string; csp?: UIResource['csp']; permissions?: UIResource['permissions'] }>
>();
private readonly modelOnlyToolCache = new Map<string, Set<string>>();
/** Creates and initializes the singleton MCPManager instance */
public static async createInstance(configs: t.MCPServers): Promise<MCPManager> {
if (MCPManager.instance) throw new Error('MCPManager has already been initialized.');
@ -343,39 +348,67 @@ Please follow these instructions when using tools from the respective MCP server
public clearResourceUriCache(serverName?: string): void {
if (serverName) {
this.resourceUriCache.delete(serverName);
for (const key of this.resourceUriCache.keys()) {
if (key === serverName || key.startsWith(`${serverName}:`)) {
this.resourceUriCache.delete(key);
this.modelOnlyToolCache.delete(key);
}
}
} else {
this.resourceUriCache.clear();
this.modelOnlyToolCache.clear();
}
}
private async populateToolCaches(connection: MCPConnection, cacheKey: string): Promise<void> {
const tools = await connection.fetchTools();
const serverMap = new Map<
string,
{ uri: string; csp?: UIResource['csp']; permissions?: UIResource['permissions'] }
>();
const modelOnly = new Set<string>();
for (const tool of tools) {
if (isToolVisibilityModelOnly(tool)) {
modelOnly.add(tool.name);
}
const uri = getToolUiResourceUri(tool);
if (uri) {
const meta = tool._meta as
| { ui?: { csp?: UIResource['csp']; permissions?: UIResource['permissions'] } }
| undefined;
serverMap.set(tool.name, { uri, csp: meta?.ui?.csp, permissions: meta?.ui?.permissions });
}
}
this.resourceUriCache.set(cacheKey, serverMap);
this.modelOnlyToolCache.set(cacheKey, modelOnly);
}
private async getResourceMeta(
connection: MCPConnection,
serverName: string,
toolName: string,
userId?: string,
): Promise<
{ uri: string; csp?: UIResource['csp']; permissions?: UIResource['permissions'] } | undefined
> {
let serverMap = this.resourceUriCache.get(serverName);
if (!serverMap) {
const tools = await connection.fetchTools();
serverMap = new Map();
for (const tool of tools) {
const uri = getToolUiResourceUri(tool);
if (uri) {
const meta = tool._meta as
| { ui?: { csp?: UIResource['csp']; permissions?: UIResource['permissions'] } }
| undefined;
serverMap.set(tool.name, {
uri,
csp: meta?.ui?.csp,
permissions: meta?.ui?.permissions,
});
}
}
this.resourceUriCache.set(serverName, serverMap);
const cacheKey = `${serverName}:${userId ?? ''}`;
if (!this.resourceUriCache.has(cacheKey)) {
await this.populateToolCaches(connection, cacheKey);
}
return serverMap.get(toolName);
return this.resourceUriCache.get(cacheKey)?.get(toolName);
}
private async isModelOnlyTool(
connection: MCPConnection,
serverName: string,
toolName: string,
userId?: string,
): Promise<boolean> {
const cacheKey = `${serverName}:${userId ?? ''}`;
if (!this.modelOnlyToolCache.has(cacheKey)) {
await this.populateToolCaches(connection, cacheKey);
}
return this.modelOnlyToolCache.get(cacheKey)?.has(toolName) ?? false;
}
/**
@ -584,7 +617,7 @@ Please follow these instructions when using tools from the respective MCP server
| { uri: string; csp?: UIResource['csp']; permissions?: UIResource['permissions'] }
| undefined;
try {
resourceMeta = await this.getResourceMeta(connection, serverName, toolName);
resourceMeta = await this.getResourceMeta(connection, serverName, toolName, userId);
if (resourceMeta) {
logger.debug(`[MCP][${serverName}][${toolName}] Found resourceUri: ${resourceMeta.uri}`);
}
@ -686,6 +719,32 @@ Please follow these instructions when using tools from the respective MCP server
);
}
if (await this.isModelOnlyTool(connection, serverName, toolName, userId)) {
throw new McpError(
ErrorCode.InvalidRequest,
`${logPrefix} Tool "${toolName}" is restricted to model use only.`,
);
}
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);
const currentOptions = processMCPEnv({
user,
dbSourced: isDbSourced,
options: rawConfig as t.MCPOptions,
});
const resolvedHeaders: Record<string, string> =
'headers' in currentOptions ? { ...(currentOptions.headers || {}) } : {};
connection.setRequestHeaders(resolvedHeaders);
}
const result = await connection.client.request(
{
method: 'tools/call',

View file

@ -244,7 +244,8 @@ export function formatToolContent(
metadata.serverName &&
metadata.toolName
) {
const resourceId = generateResourceId(metadata.resourceUri);
const scKey = result?.structuredContent != null ? JSON.stringify(result.structuredContent) : '';
const resourceId = generateResourceId(metadata.resourceUri + '\x00' + scKey);
uiResources.push({
resourceId,
uri: metadata.resourceUri,
@ -252,6 +253,7 @@ export function formatToolContent(
serverName: metadata.serverName,
toolName: metadata.toolName,
structuredContent: result?.structuredContent,
content: result?.content,
csp: metadata.csp,
permissions: metadata.permissions,
});

View file

@ -839,6 +839,7 @@ export type UIResource = {
serverName?: string;
toolName?: string;
structuredContent?: Record<string, unknown>;
content?: unknown[];
csp?: {
connectDomains?: string[];
resourceDomains?: string[];