mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-28 12:44:28 +00:00
fix(mcp): match app links by full source, pick the requested resource, clamp and deduplicate
Validate host-opened app links against the complete declared CSP source. The matcher parsed a scheme and port and then discarded both, so a declaration such as https://api.example.com:443 also authorized http://api.example.com:8080; scheme and effective port are now compared, an entry that omits them still matches either scheme and any port, and a ws(s) declaration no longer authorizes a navigable link. Select the resources/read content item by requested URI and MCP App profile instead of taking the first entry, so a server that returns auxiliary content first no longer renders the wrong document under the wrong CSP and permissions. Clamp app-requested iframe heights through a shared helper in all three renderers: the size comes from the sandboxed app, so an unclamped value let it impose multi-million-pixel layout on the host. Strip embedded resource bodies from the shared tool-result snapshot attached to each app resource. Every app kept a copy of every other app's html, so a result with several large views grew quadratically and could exceed the document size limit; each app still carries its own text or blob. Fail closed on apps when the scoped allowlist resolver throws. Allowlists still fall back to the operator baseline, but a scope that disabled apps would otherwise get inline app HTML persisted and rendered, which the gated endpoints cannot retract after the fact.
This commit is contained in:
parent
2c556d4ea5
commit
c1ccd7155c
9 changed files with 221 additions and 20 deletions
|
|
@ -16,6 +16,7 @@ import {
|
|||
buildAppToolResult,
|
||||
isMcpAppResource,
|
||||
getInlineResourceHtml,
|
||||
clampAppViewHeight,
|
||||
} from '~/utils/mcpApps';
|
||||
import { useMCPIconMap, useAppBridge, useMCPServerNames } from '~/hooks/MCP';
|
||||
import { useLocalize, useProgress, useExpandCollapse } from '~/hooks';
|
||||
|
|
@ -64,8 +65,9 @@ const MCPAppView = React.memo(function MCPAppView({
|
|||
const toolResult = useMemo(() => buildAppToolResult(app), [app]);
|
||||
|
||||
const handleSizeChanged = useCallback((params: { height?: number; width?: number }) => {
|
||||
if (params.height && params.height > 0) {
|
||||
setHeight(params.height);
|
||||
const clamped = clampAppViewHeight(params.height);
|
||||
if (clamped != null) {
|
||||
setHeight(clamped);
|
||||
setLoaded(true);
|
||||
}
|
||||
}, []);
|
||||
|
|
|
|||
|
|
@ -7,6 +7,7 @@ import {
|
|||
buildAppToolResult,
|
||||
isMcpAppResource,
|
||||
getInlineResourceHtml,
|
||||
clampAppViewHeight,
|
||||
} from '~/utils/mcpApps';
|
||||
import { useIsMessagesViewReadOnly } from '~/Providers';
|
||||
import { useAppBridge } from '~/hooks/MCP';
|
||||
|
|
@ -46,9 +47,10 @@ function MCPAppCard({
|
|||
|
||||
const handleSizeChanged = React.useCallback(
|
||||
(params: { height?: number; width?: number }) => {
|
||||
if (params.height && params.height > 0) {
|
||||
const clamped = clampAppViewHeight(params.height);
|
||||
if (clamped != null) {
|
||||
setLoaded(true);
|
||||
onHeightChange?.(params.height);
|
||||
onHeightChange?.(clamped);
|
||||
}
|
||||
},
|
||||
[onHeightChange],
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@ import {
|
|||
buildAppToolResult,
|
||||
isMcpAppResource,
|
||||
getInlineResourceHtml,
|
||||
clampAppViewHeight,
|
||||
} from '~/utils/mcpApps';
|
||||
import { useConversationUIResources } from '~/hooks/Messages/useConversationUIResources';
|
||||
import { useOptionalMessagesConversation, useIsMessagesViewReadOnly } from '~/Providers';
|
||||
|
|
@ -55,8 +56,9 @@ export function MCPUIResource(props: MCPUIResourceProps) {
|
|||
);
|
||||
|
||||
const handleSizeChanged = useCallback((params: { height?: number; width?: number }) => {
|
||||
if (params.height && params.height > 0) {
|
||||
setHeight(params.height);
|
||||
const clamped = clampAppViewHeight(params.height);
|
||||
if (clamped != null) {
|
||||
setHeight(clamped);
|
||||
setLoaded(true);
|
||||
}
|
||||
}, []);
|
||||
|
|
|
|||
62
client/src/utils/__tests__/mcpApps.spec.ts
Normal file
62
client/src/utils/__tests__/mcpApps.spec.ts
Normal file
|
|
@ -0,0 +1,62 @@
|
|||
import { isAllowedAppLink, clampAppViewHeight, MAX_APP_VIEW_HEIGHT } from '~/utils/mcpApps';
|
||||
|
||||
describe('isAllowedAppLink', () => {
|
||||
it('refuses everything when the resource declares no egress domains', () => {
|
||||
expect(isAllowedAppLink('https://api.example.com/x', undefined)).toBe(false);
|
||||
expect(isAllowedAppLink('https://api.example.com/x', {})).toBe(false);
|
||||
});
|
||||
|
||||
it('allows a declared host on either scheme when the declaration omits one', () => {
|
||||
const csp = { connectDomains: ['api.example.com'] };
|
||||
expect(isAllowedAppLink('https://api.example.com/x', csp)).toBe(true);
|
||||
expect(isAllowedAppLink('http://api.example.com:8080/x', csp)).toBe(true);
|
||||
expect(isAllowedAppLink('https://evil.com/x', csp)).toBe(false);
|
||||
});
|
||||
|
||||
it('honors a declared scheme', () => {
|
||||
const csp = { connectDomains: ['https://api.example.com'] };
|
||||
expect(isAllowedAppLink('https://api.example.com/x', csp)).toBe(true);
|
||||
expect(isAllowedAppLink('http://api.example.com/x', csp)).toBe(false);
|
||||
});
|
||||
|
||||
it('honors a declared port, including the scheme default', () => {
|
||||
const csp = { connectDomains: ['https://api.example.com:443'] };
|
||||
expect(isAllowedAppLink('https://api.example.com/x', csp)).toBe(true);
|
||||
expect(isAllowedAppLink('http://api.example.com:8080/collect?d=1', csp)).toBe(false);
|
||||
expect(isAllowedAppLink('https://api.example.com:8443/x', csp)).toBe(false);
|
||||
});
|
||||
|
||||
it('supports wildcard subdomains without matching an unrelated suffix', () => {
|
||||
const csp = { resourceDomains: ['*.example.com'] };
|
||||
expect(isAllowedAppLink('https://cdn.example.com/a.png', csp)).toBe(true);
|
||||
expect(isAllowedAppLink('https://example.com/a.png', csp)).toBe(true);
|
||||
expect(isAllowedAppLink('https://notexample.com/a.png', csp)).toBe(false);
|
||||
});
|
||||
|
||||
it('refuses non-http(s) schemes and malformed urls', () => {
|
||||
const csp = { connectDomains: ['api.example.com'] };
|
||||
expect(isAllowedAppLink('javascript:alert(1)', csp)).toBe(false);
|
||||
expect(isAllowedAppLink('not a url', csp)).toBe(false);
|
||||
});
|
||||
|
||||
it('does not treat a ws(s) declaration as a navigable link target', () => {
|
||||
expect(
|
||||
isAllowedAppLink('https://api.example.com/x', { connectDomains: ['wss://api.example.com'] }),
|
||||
).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe('clampAppViewHeight', () => {
|
||||
it('ignores non-positive or non-finite heights', () => {
|
||||
expect(clampAppViewHeight(undefined)).toBeUndefined();
|
||||
expect(clampAppViewHeight(0)).toBeUndefined();
|
||||
expect(clampAppViewHeight(-5)).toBeUndefined();
|
||||
expect(clampAppViewHeight(Number.POSITIVE_INFINITY)).toBeUndefined();
|
||||
expect(clampAppViewHeight(Number.NaN)).toBeUndefined();
|
||||
});
|
||||
|
||||
it('clamps an app-requested height to the host maximum', () => {
|
||||
expect(clampAppViewHeight(500)).toBe(500);
|
||||
expect(clampAppViewHeight(10_000_000)).toBe(MAX_APP_VIEW_HEIGHT);
|
||||
});
|
||||
});
|
||||
|
|
@ -111,16 +111,58 @@ export function decodeBase64Utf8(b64: string): string {
|
|||
return new TextDecoder('utf-8').decode(bytes);
|
||||
}
|
||||
|
||||
const APP_LINK_HOST_PATTERN =
|
||||
/^(?:(?:https?|wss?):\/\/)?(\*\.)?([a-zA-Z0-9][a-zA-Z0-9.-]*)(?::\d{1,5})?$/;
|
||||
/**
|
||||
* Upper bound for an app-requested iframe height. The size comes from the sandboxed app, so an
|
||||
* unclamped value would let it impose multi-million-pixel layout on the host conversation.
|
||||
*/
|
||||
export const MAX_APP_VIEW_HEIGHT = 4000;
|
||||
|
||||
function hostMatchesDeclaredDomain(hostname: string, entry: string): boolean {
|
||||
/** Clamps an app-reported height, returning undefined when it is not a usable positive number. */
|
||||
export function clampAppViewHeight(height?: number): number | undefined {
|
||||
if (typeof height !== 'number' || !Number.isFinite(height) || height <= 0) {
|
||||
return undefined;
|
||||
}
|
||||
return Math.min(Math.round(height), MAX_APP_VIEW_HEIGHT);
|
||||
}
|
||||
|
||||
const APP_LINK_HOST_PATTERN =
|
||||
/^(?:(https?|wss?):\/\/)?(\*\.)?([a-zA-Z0-9][a-zA-Z0-9.-]*)(?::(\d{1,5}))?$/;
|
||||
|
||||
const DEFAULT_PORTS: Record<string, string> = { 'http:': '80', 'https:': '443' };
|
||||
|
||||
function effectivePort(url: URL): string {
|
||||
return url.port || DEFAULT_PORTS[url.protocol] || '';
|
||||
}
|
||||
|
||||
/**
|
||||
* Matches a URL against one declared CSP source. A declared scheme or port narrows the match the
|
||||
* same way it would inside the sandbox CSP, so `https://api.example.com:443` must not authorize
|
||||
* `http://api.example.com:8080`. An entry with no scheme/port matches either scheme and any port,
|
||||
* mirroring CSP host-source semantics.
|
||||
*/
|
||||
function urlMatchesDeclaredSource(url: URL, entry: string): boolean {
|
||||
const match = APP_LINK_HOST_PATTERN.exec(entry.trim());
|
||||
if (!match) {
|
||||
return false;
|
||||
}
|
||||
const [, wildcard, declaredHost] = match;
|
||||
const host = hostname.toLowerCase();
|
||||
const [, declaredScheme, wildcard, declaredHost, declaredPort] = match;
|
||||
|
||||
if (declaredScheme) {
|
||||
const scheme = declaredScheme.toLowerCase();
|
||||
// ws(s) declarations are for sockets, not navigable links.
|
||||
if (scheme !== 'http' && scheme !== 'https') {
|
||||
return false;
|
||||
}
|
||||
if (url.protocol !== `${scheme}:`) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
if (declaredPort && effectivePort(url) !== declaredPort) {
|
||||
return false;
|
||||
}
|
||||
|
||||
const host = url.hostname.toLowerCase();
|
||||
const target = declaredHost.toLowerCase();
|
||||
return wildcard ? host === target || host.endsWith(`.${target}`) : host === target;
|
||||
}
|
||||
|
|
@ -147,7 +189,7 @@ export function isAllowedAppLink(url: string, csp: UIResource['csp']): boolean {
|
|||
...(csp?.frameDomains ?? []),
|
||||
];
|
||||
return declared.some(
|
||||
(entry) => typeof entry === 'string' && hostMatchesDeclaredDomain(parsed.hostname, entry),
|
||||
(entry) => typeof entry === 'string' && urlMatchesDeclaredSource(parsed, entry),
|
||||
);
|
||||
}
|
||||
|
||||
|
|
@ -179,9 +221,23 @@ export async function fetchMCPResourceHtml(
|
|||
permissions?: ResourceUiMeta['permissions'];
|
||||
}> {
|
||||
const result = (await readMCPResource(serverName, uri)) as {
|
||||
contents?: Array<{ text?: string; blob?: string; _meta?: { ui?: ResourceUiMeta } }>;
|
||||
contents?: Array<{
|
||||
uri?: string;
|
||||
mimeType?: string;
|
||||
text?: string;
|
||||
blob?: string;
|
||||
_meta?: { ui?: ResourceUiMeta };
|
||||
}>;
|
||||
};
|
||||
const item = result?.contents?.[0];
|
||||
const contents = result?.contents ?? [];
|
||||
// A server may return auxiliary items alongside the app document, in any order, so pick the entry
|
||||
// for the requested URI (preferring the MCP App profile) rather than trusting response order;
|
||||
// otherwise the wrong document renders under the wrong CSP and permissions.
|
||||
const item =
|
||||
contents.find((c) => c.uri === uri && (c.mimeType ?? '').includes('profile=mcp-app')) ??
|
||||
contents.find((c) => c.uri === uri) ??
|
||||
contents.find((c) => (c.mimeType ?? '').includes('profile=mcp-app')) ??
|
||||
contents[0];
|
||||
const uiMeta = item?._meta?.ui;
|
||||
let html = item?.text ?? '';
|
||||
if (!html && typeof item?.blob === 'string' && item.blob) {
|
||||
|
|
|
|||
|
|
@ -329,7 +329,12 @@ describe('formatToolContent', () => {
|
|||
toolName: 'do_thing',
|
||||
structuredContent: { count: 3 },
|
||||
});
|
||||
expect(uiResourceArtifact?.content).toEqual(result.content);
|
||||
// The shared result snapshot keeps the resource reference but not its body (see the
|
||||
// no-duplication test below); the app's own html stays on the resource itself.
|
||||
expect(uiResourceArtifact?.content).toEqual([
|
||||
{ type: 'resource', resource: { uri: 'ui://app', mimeType: 'text/html;profile=mcp-app' } },
|
||||
]);
|
||||
expect(uiResourceArtifact?.text).toBe('<p>hi</p>');
|
||||
});
|
||||
|
||||
it('renders a plain text/html ui:// resource statically without app-bridge metadata', () => {
|
||||
|
|
@ -402,6 +407,42 @@ describe('formatToolContent', () => {
|
|||
expect(uris).toEqual(['ui://app']);
|
||||
});
|
||||
|
||||
it('does not copy every embedded app body into each app resource result', () => {
|
||||
const bigA = 'A'.repeat(5000);
|
||||
const bigB = 'B'.repeat(5000);
|
||||
const result: t.MCPToolCallResponse = {
|
||||
content: [
|
||||
{
|
||||
type: 'resource',
|
||||
resource: { uri: 'ui://a', mimeType: 'text/html;profile=mcp-app', text: bigA },
|
||||
},
|
||||
{
|
||||
type: 'resource',
|
||||
resource: { uri: 'ui://b', mimeType: 'text/html;profile=mcp-app', text: bigB },
|
||||
},
|
||||
],
|
||||
};
|
||||
|
||||
const [, artifacts] = formatToolContent(result, 'openai', {
|
||||
serverName: 'srv',
|
||||
toolName: 'do_thing',
|
||||
});
|
||||
|
||||
const data = artifacts?.ui_resources?.data ?? [];
|
||||
expect(data).toHaveLength(2);
|
||||
// Each app keeps its OWN html...
|
||||
expect(data[0].text).toBe(bigA);
|
||||
expect(data[1].text).toBe(bigB);
|
||||
// ...but the shared result snapshot carries no resource bodies, so N apps do not persist N
|
||||
// copies of every app's html.
|
||||
for (const resource of data) {
|
||||
const snapshot = JSON.stringify(resource.content ?? []);
|
||||
expect(snapshot).not.toContain(bigA);
|
||||
expect(snapshot).not.toContain(bigB);
|
||||
expect(snapshot).toContain('ui://a');
|
||||
}
|
||||
});
|
||||
|
||||
it('suppresses embedded ui:// resources when apps are disabled for the scope', () => {
|
||||
const result: t.MCPToolCallResponse = {
|
||||
content: [
|
||||
|
|
|
|||
|
|
@ -173,6 +173,34 @@ export function isRenderableUiResource(item: t.ToolContentPart): boolean {
|
|||
return mimeType.includes('html');
|
||||
}
|
||||
|
||||
/**
|
||||
* The shared tool result is attached to each app resource for the App Bridge. Embedded resource
|
||||
* bodies are dropped from that copy: otherwise N app resources each persist every resource's HTML or
|
||||
* blob, so a result with several large views grows quadratically and can exceed the document size
|
||||
* limit. Every app still carries its own `text`/`blob`, and the URI/mime of siblings is preserved.
|
||||
*/
|
||||
function stripEmbeddedResourceBodies(
|
||||
content?: t.ToolContentPart[],
|
||||
): t.ToolContentPart[] | undefined {
|
||||
if (!Array.isArray(content)) {
|
||||
return content;
|
||||
}
|
||||
let stripped = false;
|
||||
const next = content.map((item) => {
|
||||
if (item.type !== 'resource') {
|
||||
return item;
|
||||
}
|
||||
const resource = item.resource as Record<string, unknown>;
|
||||
if (typeof resource.text !== 'string' && typeof resource.blob !== 'string') {
|
||||
return item;
|
||||
}
|
||||
stripped = true;
|
||||
const { text: _text, blob: _blob, ...rest } = resource;
|
||||
return { ...item, resource: rest } as t.ToolContentPart;
|
||||
});
|
||||
return stripped ? next : content;
|
||||
}
|
||||
|
||||
export function resultHasRenderableUiResource(result: t.MCPToolCallResponse): boolean {
|
||||
const content = result?.content;
|
||||
if (!Array.isArray(content)) {
|
||||
|
|
@ -217,6 +245,8 @@ export function formatToolContent(
|
|||
const imageUrls: t.FormattedContent[] = [];
|
||||
const uiResources: UIResource[] = [];
|
||||
let currentTextBlock = '';
|
||||
/** Built once and shared by every app resource rather than per-resource. */
|
||||
const sharedResultContent = stripEmbeddedResourceBodies(result?.content);
|
||||
|
||||
type ContentHandler = undefined | ((item: t.ToolContentPart) => void);
|
||||
|
||||
|
|
@ -279,7 +309,7 @@ export function formatToolContent(
|
|||
serverName: metadata?.serverName,
|
||||
toolName: metadata?.toolName,
|
||||
structuredContent: result?.structuredContent,
|
||||
content: result?.content,
|
||||
content: sharedResultContent,
|
||||
isError: result?.isError,
|
||||
resultMeta: (result as { _meta?: Record<string, unknown> })?._meta,
|
||||
toolArgs: metadata?.toolArgs,
|
||||
|
|
|
|||
|
|
@ -252,7 +252,7 @@ export class MCPServersRegistry {
|
|||
let allowedDomains = this.allowedDomains;
|
||||
let allowedAddresses = this.allowedAddresses;
|
||||
// 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.
|
||||
// falling back to the YAML base when the resolver omits it or is absent.
|
||||
let appsEnabled = this.getAppsEnabled();
|
||||
if (this.allowlistResolver) {
|
||||
try {
|
||||
|
|
@ -264,9 +264,13 @@ export class MCPServersRegistry {
|
|||
}
|
||||
} catch (error) {
|
||||
logger.warn(
|
||||
'[MCPServersRegistry] Allowlist resolver failed; falling back to YAML base allowlists',
|
||||
'[MCPServersRegistry] Allowlist resolver failed; falling back to YAML base allowlists and disabling apps',
|
||||
error,
|
||||
);
|
||||
// Allowlists fall back to the operator baseline, but apps fail CLOSED: a scope that disabled
|
||||
// them would otherwise get inline app HTML persisted and rendered, and the gated endpoints
|
||||
// cannot retract HTML that already reached the transcript.
|
||||
appsEnabled = false;
|
||||
}
|
||||
}
|
||||
return {
|
||||
|
|
|
|||
|
|
@ -292,15 +292,17 @@ describe('MCPServersRegistry', () => {
|
|||
});
|
||||
});
|
||||
|
||||
it('falls back to the YAML base allowlists when the resolver throws', async () => {
|
||||
it('falls back to the YAML base allowlists but disables apps when the resolver throws', async () => {
|
||||
const resolver = jest.fn().mockRejectedValue(new Error('DB down'));
|
||||
const reg = createWith(['yaml.com'], null, resolver);
|
||||
|
||||
// Allowlists fall back to the operator baseline; apps fail closed because inline app HTML
|
||||
// cannot be retracted once it reaches the transcript.
|
||||
await expect(reg.resolveAllowlists()).resolves.toEqual({
|
||||
allowedDomains: ['yaml.com'],
|
||||
allowedAddresses: null,
|
||||
useSSRFProtection: false,
|
||||
appsEnabled: true,
|
||||
appsEnabled: false,
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue