diff --git a/frontend/src/lib/components/copilot/chat/ToolExecutionDisplay.svelte b/frontend/src/lib/components/copilot/chat/ToolExecutionDisplay.svelte index b714e1a60f..f514b36347 100644 --- a/frontend/src/lib/components/copilot/chat/ToolExecutionDisplay.svelte +++ b/frontend/src/lib/components/copilot/chat/ToolExecutionDisplay.svelte @@ -67,8 +67,8 @@ })() if (!claimed) return undefined // Both sources are ultimately a path the model wrote, and resolving one reads - // that resource and asks a third party for its host's favicon — so it has to be - // a server the user connected, not any path the model can name. + // that resource — so it has to name a server the user connected, not any + // workspace resource the model can point at. if (aiChatManager.mcpServers.some((s) => s.path === claimed)) return claimed // A session runtime does not populate `mcpServers` until its first send, so on a // reloaded transcript the live list is empty and the rows this path was persisted @@ -274,14 +274,17 @@ Nothing is drawn until a mark resolves: a placeholder plug on every row would be noise. --> {#snippet serverMark()} - {#if message.mcpIconSrc} - - {:else if mcpServerPath && aiChatManager.operatingWorkspace} + {#if mcpServerPath && aiChatManager.operatingWorkspace} + {#await resolveMcpServerMark(aiChatManager.operatingWorkspace, mcpServerPath) then mark} - {#if mark.icon} - + {#if message.mcpIconSrc || mark.icon} + {/if} {/await} + {:else if message.mcpIconSrc} + {/if} {/snippet} diff --git a/frontend/src/lib/components/copilot/chat/toolSchema.test.ts b/frontend/src/lib/components/copilot/chat/toolSchema.test.ts new file mode 100644 index 0000000000..f4b2e79574 --- /dev/null +++ b/frontend/src/lib/components/copilot/chat/toolSchema.test.ts @@ -0,0 +1,45 @@ +import { describe, expect, it } from 'vitest' + +import { normalizeToolParameterSchema } from './toolSchema' + +describe('normalizeToolParameterSchema', () => { + // `required` is `uniqueItems` at every subschema, and a provider rejects the whole + // request over a repeat — which for a tool built from a third party's schema means + // every send fails until that tool goes away. `safeInputSchema` only de-dupes the + // root, so the nested case is this function's alone. + it('de-dupes required at every depth', () => { + const schema: Record = { + type: 'object', + properties: { + user: { + type: 'object', + properties: { id: { type: 'string' } }, + required: ['id', 'id'] + }, + tags: { type: 'array', items: { type: 'object', required: ['name', 'name'] } } + }, + anyOf: [{ type: 'object', required: ['a', 'a'] }], + required: ['user', 'user'] + } + + normalizeToolParameterSchema(schema) + + expect(schema.required).toEqual(['user']) + expect(schema.properties.user.required).toEqual(['id']) + expect(schema.properties.tags.items.required).toEqual(['name']) + expect(schema.anyOf[0].required).toEqual(['a']) + }) + + it('strips an empty or null format at every depth', () => { + const schema: Record = { + type: 'object', + format: '', + properties: { a: { type: 'string', format: null } } + } + + normalizeToolParameterSchema(schema) + + expect(schema.format).toBeUndefined() + expect(schema.properties.a.format).toBeUndefined() + }) +}) diff --git a/frontend/src/lib/components/mcp/McpServerIcon.svelte b/frontend/src/lib/components/mcp/McpServerIcon.svelte index c04906cec6..a510f49e45 100644 --- a/frontend/src/lib/components/mcp/McpServerIcon.svelte +++ b/frontend/src/lib/components/mcp/McpServerIcon.svelte @@ -7,15 +7,15 @@ * published (`icons`, per the spec), then the icon Windmill ships for that * integration, then a generic plug. * - * The server's own icon comes first because it is the only source that is both - * authoritative and free of a third party — the bytes arrive over the MCP - * connection the user already made. + * The server's own icon comes first because it is the authoritative one and costs + * nothing to show: `pickMcpIconSrc` admits only `data:` sources, so the bytes are + * already here and no request leaves the browser to render them. */ let { src, icon, size = 16 }: { src?: string; icon?: Component; size?: number } = $props() // Keyed by src rather than a boolean so a server publishing a different icon - // retries instead of inheriting the previous one's failure. An `https:` src is - // usually blocked by the app's COEP require-corp anyway, which lands here. + // retries instead of inheriting the previous one's failure. A malformed data URI + // lands here and falls through to the icon below it. let failedFor = $state(undefined) const px = $derived(`${size}px`) diff --git a/frontend/src/lib/components/mcp/mcpIcon.test.ts b/frontend/src/lib/components/mcp/mcpIcon.test.ts index 5f1f8e7680..0f017a0bf7 100644 --- a/frontend/src/lib/components/mcp/mcpIcon.test.ts +++ b/frontend/src/lib/components/mcp/mcpIcon.test.ts @@ -16,17 +16,20 @@ describe('pickMcpIconSrc', () => { ) }) - it('refuses schemes the spec calls unsafe', () => { + // An https src would make the browser fetch from a host the server names, which is + // the disclosure the favicon lookup was removed for. COEP blocks the response, not + // the request, so it does not save us. + it('refuses a fetchable src, whatever the scheme', () => { + expect(pickMcpIconSrc([{ src: 'https://example.com/logo.png' }])).toBeUndefined() + expect(pickMcpIconSrc([{ src: 'http://example.com/i.png' }])).toBeUndefined() expect(pickMcpIconSrc([{ src: 'javascript:alert(1)' }])).toBeUndefined() expect(pickMcpIconSrc([{ src: 'file:///etc/passwd' }])).toBeUndefined() expect(pickMcpIconSrc([{ src: 'ftp://example.com/i.png' }])).toBeUndefined() - expect(pickMcpIconSrc([{ src: 'http://example.com/i.png' }])).toBeUndefined() }) // SVG can carry script, and sanitising it is not worth an icon. - it('refuses SVG in either form', () => { + it('refuses SVG', () => { expect(pickMcpIconSrc([{ src: 'data:image/svg+xml;base64,PHN2Zz4=' }])).toBeUndefined() - expect(pickMcpIconSrc([{ src: 'https://example.com/logo.svg' }])).toBeUndefined() }) // `mimeType` is advisory per the spec, so it must not launder the payload. @@ -36,16 +39,13 @@ describe('pickMcpIconSrc', () => { ).toBeUndefined() }) + // The chosen src is persisted on a transcript row and re-cloned on every save. it('refuses an oversized data URI', () => { - expect(pickMcpIconSrc([{ src: `data:image/png;base64,${'A'.repeat(70_000)}` }])).toBeUndefined() + expect(pickMcpIconSrc([{ src: `data:image/png;base64,${'A'.repeat(9_000)}` }])).toBeUndefined() }) - it('prefers the icon matching the theme, then an untagged one', () => { - const dark = 'data:image/png;base64,ZGFyaw==' - const icons = [{ src: png, theme: 'light' }, { src: dark, theme: 'dark' }, { src: png }] - - expect(pickMcpIconSrc(icons, true)).toBe(dark) - expect(pickMcpIconSrc(icons, false)).toBe(png) + it('takes the first usable entry, skipping ones it refuses', () => { + expect(pickMcpIconSrc([{ src: 'https://example.com/a.png' }, { src: png }])).toBe(png) }) it('returns nothing for a missing or malformed icons field', () => { diff --git a/frontend/src/lib/components/mcp/mcpIcon.ts b/frontend/src/lib/components/mcp/mcpIcon.ts index 8ff74f574e..33c70a5e79 100644 --- a/frontend/src/lib/components/mcp/mcpIcon.ts +++ b/frontend/src/lib/components/mcp/mcpIcon.ts @@ -6,38 +6,35 @@ export type McpIcon = { theme?: unknown } -// A data URI rides in the request that renders it and is persisted on a transcript -// row, so an oversized one is refused rather than carried. -const MAX_ICON_SRC_CHARS = 64_000 +// The chosen src is persisted on a transcript row, and the whole chat record is +// re-cloned into IndexedDB on every save — so an icon drawn at 14px is held to a size +// that suits one. (`imageUrl` earns its out-of-band store by being a screenshot.) +const MAX_ICON_SRC_CHARS = 8_000 /** * The icon source to render, chosen from what an MCP server published. * - * The `src` is chosen by the server, so the spec's rules are enforced here rather - * than trusted: only `https:` and `data:` (it names `javascript:`, `file:`, `ftp:` - * and `ws:` as the schemes to reject), and no SVG — it can carry script, and - * sanitising it is not worth the icon. `mimeType` is advisory, so a data URI is - * judged on the type it actually declares. + * **`data:` only.** The spec also permits an HTTPS src, but rendering one makes the + * user's browser fetch from a host the server names, disclosing their IP, user agent + * and the moment they looked — and a per-user URL turns it into a read receipt. COEP + * `require-corp` does not prevent that: it blocks the *response*, so the request has + * already left. That is the same disclosure the favicon lookup was removed for, and + * against an arbitrary host rather than one known party. A `data:` src carries its + * bytes over the MCP connection the user already made and fetches nothing. * - * An `https:` src is accepted but will usually fail to load: the app is served with - * COEP require-corp and a third-party image without a Cross-Origin-Resource-Policy - * header is blocked. `McpServerIcon` falls through to the next source when it does. + * The rest is the spec's own list, enforced rather than trusted, since `src` is chosen + * by a third party: no SVG (it can carry script), and a data URI judged by the type it + * declares rather than the sibling `mimeType`, which the spec calls advisory. */ -export function pickMcpIconSrc(icons: unknown, dark = false): string | undefined { +export function pickMcpIconSrc(icons: unknown): string | undefined { if (!Array.isArray(icons)) return undefined - const usable = icons.filter((icon): icon is McpIcon => isRenderable(icon)) - // `theme` names the background the icon was drawn for; an untagged icon suits either. - const themed = usable.find((icon) => icon.theme === (dark ? 'dark' : 'light')) - const untagged = usable.find((icon) => icon.theme === undefined) - return (themed ?? untagged ?? usable[0])?.src as string | undefined + return icons.find((icon): icon is McpIcon => isRenderable(icon))?.src as string | undefined } function isRenderable(icon: unknown): boolean { const src = (icon as McpIcon | null)?.src if (typeof src !== 'string' || src.length > MAX_ICON_SRC_CHARS) return false const lower = src.toLowerCase() - if (lower.startsWith('https://')) return !lower.endsWith('.svg') if (!lower.startsWith('data:image/')) return false - // Judge the data URI by the type it declares, not by the sibling `mimeType`. return ['png', 'jpeg', 'jpg', 'webp'].some((type) => lower.startsWith(`data:image/${type}`)) }