fix: accept only data: MCP icons, and keep the shipped icon as fallback

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgzuxyafKNF2uaEeL35XQw
2026-09-21 00:02:30 +00:00 · 2026-09-10 16:21:58 +02:00
parent 56a827c6cc
commit 3e93457898
5 changed files with 87 additions and 42 deletions
@@ -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}
-			<McpServerIcon src={message.mcpIconSrc} size={14} />
-		{:else if mcpServerPath && aiChatManager.operatingWorkspace}
+		{#if mcpServerPath && aiChatManager.operatingWorkspace}
+			<!-- Both sources are resolved, not just the first: the component falls back to
+			     the shipped icon when a published one fails to render, and gating on the
+			     src would leave a plug where Windmill has an icon for the integration. -->
 			{#await resolveMcpServerMark(aiChatManager.operatingWorkspace, mcpServerPath) then mark}
-				{#if mark.icon}
-					<McpServerIcon icon={mark.icon} size={14} />
+				{#if message.mcpIconSrc || mark.icon}
+					<McpServerIcon src={message.mcpIconSrc} icon={mark.icon} size={14} />
 				{/if}
 			{/await}
+		{:else if message.mcpIconSrc}
+			<McpServerIcon src={message.mcpIconSrc} size={14} />
 		{/if}
 	{/snippet}

@@ -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<string, any> = {
+			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<string, any> = {
+			type: 'object',
+			format: '',
+			properties: { a: { type: 'string', format: null } }
+		}
+
+		normalizeToolParameterSchema(schema)
+
+		expect(schema.format).toBeUndefined()
+		expect(schema.properties.a.format).toBeUndefined()
+	})
+})
@@ -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<any>; 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<string | undefined>(undefined)

 	const px = $derived(`${size}px`)
@@ -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', () => {
@@ -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}`))
 }