From c1ccd7155cca884174f58c16d540f0d570bab930 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Sun, 9 Aug 2026 15:57:58 -0700 Subject: [PATCH] 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. --- .../Chat/Messages/Content/ToolCall.tsx | 6 +- .../Messages/Content/UIResourceCarousel.tsx | 6 +- .../MCPUIResource/MCPUIResource.tsx | 6 +- client/src/utils/__tests__/mcpApps.spec.ts | 62 ++++++++++++++++ client/src/utils/mcpApps.ts | 72 ++++++++++++++++--- .../api/src/mcp/__tests__/parsers.test.ts | 43 ++++++++++- packages/api/src/mcp/parsers.ts | 32 ++++++++- .../src/mcp/registry/MCPServersRegistry.ts | 8 ++- .../__tests__/MCPServersRegistry.test.ts | 6 +- 9 files changed, 221 insertions(+), 20 deletions(-) create mode 100644 client/src/utils/__tests__/mcpApps.spec.ts diff --git a/client/src/components/Chat/Messages/Content/ToolCall.tsx b/client/src/components/Chat/Messages/Content/ToolCall.tsx index 342211a5e0..30df77dac0 100644 --- a/client/src/components/Chat/Messages/Content/ToolCall.tsx +++ b/client/src/components/Chat/Messages/Content/ToolCall.tsx @@ -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); } }, []); diff --git a/client/src/components/Chat/Messages/Content/UIResourceCarousel.tsx b/client/src/components/Chat/Messages/Content/UIResourceCarousel.tsx index 2be01a40df..0f7cf48ee1 100644 --- a/client/src/components/Chat/Messages/Content/UIResourceCarousel.tsx +++ b/client/src/components/Chat/Messages/Content/UIResourceCarousel.tsx @@ -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], diff --git a/client/src/components/MCPUIResource/MCPUIResource.tsx b/client/src/components/MCPUIResource/MCPUIResource.tsx index 757a8ca7bb..6f968f86d2 100644 --- a/client/src/components/MCPUIResource/MCPUIResource.tsx +++ b/client/src/components/MCPUIResource/MCPUIResource.tsx @@ -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); } }, []); diff --git a/client/src/utils/__tests__/mcpApps.spec.ts b/client/src/utils/__tests__/mcpApps.spec.ts new file mode 100644 index 0000000000..ae2abdf171 --- /dev/null +++ b/client/src/utils/__tests__/mcpApps.spec.ts @@ -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); + }); +}); diff --git a/client/src/utils/mcpApps.ts b/client/src/utils/mcpApps.ts index 496d17eab8..9a2ebcb825 100644 --- a/client/src/utils/mcpApps.ts +++ b/client/src/utils/mcpApps.ts @@ -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 = { '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) { diff --git a/packages/api/src/mcp/__tests__/parsers.test.ts b/packages/api/src/mcp/__tests__/parsers.test.ts index 6d8ee26b7f..e31c24a6a9 100644 --- a/packages/api/src/mcp/__tests__/parsers.test.ts +++ b/packages/api/src/mcp/__tests__/parsers.test.ts @@ -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('

hi

'); }); 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: [ diff --git a/packages/api/src/mcp/parsers.ts b/packages/api/src/mcp/parsers.ts index c54f0849b2..0df853636f 100644 --- a/packages/api/src/mcp/parsers.ts +++ b/packages/api/src/mcp/parsers.ts @@ -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; + 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 })?._meta, toolArgs: metadata?.toolArgs, diff --git a/packages/api/src/mcp/registry/MCPServersRegistry.ts b/packages/api/src/mcp/registry/MCPServersRegistry.ts index d16da34a16..511a3fc6c7 100644 --- a/packages/api/src/mcp/registry/MCPServersRegistry.ts +++ b/packages/api/src/mcp/registry/MCPServersRegistry.ts @@ -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 { diff --git a/packages/api/src/mcp/registry/__tests__/MCPServersRegistry.test.ts b/packages/api/src/mcp/registry/__tests__/MCPServersRegistry.test.ts index 83a86afff0..34645ed7eb 100644 --- a/packages/api/src/mcp/registry/__tests__/MCPServersRegistry.test.ts +++ b/packages/api/src/mcp/registry/__tests__/MCPServersRegistry.test.ts @@ -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, }); });