feat: prefer the icon an MCP server publishes, and drop the favicon lookup

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:13:14 +02:00
parent f4b1d8bb53
commit 56a827c6cc
15 changed files with 182 additions and 155 deletions
@@ -16,7 +16,6 @@ switch that decides whether this chat carries its tools.
 	import { isMcpEnabled, setMcpEnabled } from '$lib/components/mcp/enabledServers'
 	import { loadProviderIcon } from '$lib/components/mcp/providerIcon'
 	import {
-		cachedProviderHost,
 		cachedProviderKey,
 		forgetProviderKey,
 		rememberProviderKey
@@ -85,7 +84,6 @@ switch that decides whether this chat carries its tools.
 			editedAt?: string
 			enabled: boolean
 			icon?: Component<any>
-			iconHost?: string
 		}[]
 	>([])
 	let loading = $state(false)
@@ -386,7 +384,6 @@ switch that decides whether this chat carries its tools.
 				const icon = await loadProviderIcon(key)
 				if (seq !== loadSeq) return
 				server.icon = icon
-				server.iconHost = cachedProviderHost(target, server.path, server.editedAt)
 			})
 		)
 	}
@@ -473,7 +470,7 @@ switch that decides whether this chat carries its tools.
 				<div class="flex flex-col gap-0.5">
 					{#each servers as server (server.path)}
 						{#snippet icon()}
-							<McpServerIcon icon={server.icon} host={server.iconHost} size={16} />
+							<McpServerIcon icon={server.icon} size={16} />
 						{/snippet}
 						{#snippet title()}
 							<span class="truncate leading-5">{server.path}</span>
@@ -269,13 +269,17 @@
 	{/snippet}

 	<!-- Which system a call reaches is the first thing to know about it, so an MCP
-	     call is marked with its provider before the label. Nothing is drawn until the
-	     mark resolves: a placeholder plug on every row would be noise. -->
+	     call is marked before the label — with the icon the server published for that
+	     tool where there is one, and Windmill's icon for the integration otherwise.
+	     Nothing is drawn until a mark resolves: a placeholder plug on every row would
+	     be noise. -->
 	{#snippet serverMark()}
-		{#if mcpServerPath && aiChatManager.operatingWorkspace}
+		{#if message.mcpIconSrc}
+			<McpServerIcon src={message.mcpIconSrc} size={14} />
+		{:else if mcpServerPath && aiChatManager.operatingWorkspace}
 			{#await resolveMcpServerMark(aiChatManager.operatingWorkspace, mcpServerPath) then mark}
-				{#if mark.icon || mark.host}
-					<McpServerIcon icon={mark.icon} host={mark.host} size={14} />
+				{#if mark.icon}
+					<McpServerIcon icon={mark.icon} size={14} />
 				{/if}
 			{/await}
 		{/if}
@@ -297,7 +301,7 @@
 		labelClass={showPreviewChip ? 'truncate' : ''}
 		contentClass="space-y-3"
 		headerRight={showPreviewChip ? previewChip : undefined}
-		headerLeft={mcpServerPath ? serverMark : undefined}
+		headerLeft={message.mcpIconSrc || mcpServerPath ? serverMark : undefined}
 	>
 		<!-- Image a tool produced (e.g. take_screenshot) — shown inline, not gated on expand. -->
 		{#snippet belowHeader()}
@@ -1,7 +1,6 @@
 <script lang="ts">
 	import { Globe } from 'lucide-svelte'
 	import { SvelteSet } from 'svelte/reactivity'
-	import { faviconUrl } from '$lib/utils/faviconUrl'
 	import type { WebSearchSource } from './shared'

 	interface Props {
@@ -37,6 +36,18 @@
 		}
 	}

+	// Favicons come from Google's public favicon service, which discloses each
+	// consulted hostname to a third party from the user's browser — an accepted
+	// tradeoff for a page the user just searched for (blocked/air-gapped
+	// environments degrade to the Globe icon via onerror). Hit gstatic directly
+	// rather than www.google.com/s2/favicons: the app is served with COEP
+	// require-corp, and the s2 redirect hop carries no Cross-Origin-Resource-Policy
+	// header, so the browser blocks the image. The gstatic endpoint itself responds
+	// with CORP: cross-origin.
+	function faviconUrl(hostname: string): string {
+		return `https://t3.gstatic.com/faviconV2?client=SOCIAL&type=FAVICON&fallback_opts=TYPE,SIZE,URL&url=https://${encodeURIComponent(hostname)}&size=64`
+	}
+
 	const failedFavicons = new SvelteSet<string>()
 </script>

@@ -3,6 +3,7 @@ import { ResourceService, type GetMcpToolsResponse } from '$lib/gen'
 import { createToolDef, type Tool } from '../shared'
 import { normalizeToolParameterSchema } from '../toolSchema'
 import { enabledMcpPaths } from '$lib/components/mcp/enabledServers'
+import { pickMcpIconSrc } from '$lib/components/mcp/mcpIcon'

 /**
 * Access to the MCP servers the user has connected (resources of type `mcp`),
@@ -545,7 +546,10 @@ function createCallTool(owner: string, servers: McpServer[], mode: 'read' | 'wri
 				toolCallbacks.setToolStatus(toolId, { content: error, error })
 				return bounded({ success: false, error })
 			}
-			toolCallbacks.setToolStatus(toolId, { content: `Calling ${parsed.tool}...` })
+			toolCallbacks.setToolStatus(toolId, {
+				content: `Calling ${parsed.tool}...`,
+				mcpIconSrc: pickMcpIconSrc((resolved.tool as { icons?: unknown }).icons)
+			})
 			const result = await executeTool(
 				owner,
 				workspace,
@@ -738,7 +742,8 @@ export function registerMcpTools(
 				touchLoadedTool(owner, key)
 				toolCallbacks.setToolStatus(toolId, {
 					content: `Calling ${tool.name}...`,
-					mcpServer: server.path
+					mcpServer: server.path,
+					mcpIconSrc: pickMcpIconSrc((tool as { icons?: unknown }).icons)
 				})
 				const result = await executeTool(owner, workspace, server, tool, args ?? {}, readOnly)
 				const ok = JSON.parse(result).success === true
@@ -580,6 +580,10 @@ export type ToolDisplayMessage = {
 	 * Recorded here rather than looked up from the tool name: the registry of loaded
 	 * remote tools lives only in memory, so a reloaded transcript could not resolve it. */
 	mcpServer?: string
+	/** Icon the MCP server published for the tool this row called (`icons`, per the
+	 * spec), already validated. Recorded here for the same reason as `mcpServer`: the
+	 * listing it came from is not available to a reloaded transcript. */
+	mcpIconSrc?: string
 	showFade?: boolean
 	actions?: ToolDisplayAction[]
 	userQuestion?: UserQuestionDisplay
@@ -1,35 +1,38 @@
 <script lang="ts">
 	import type { Component } from 'svelte'
 	import { Plug } from 'lucide-svelte'
-	import { faviconUrl } from '$lib/utils/faviconUrl'

 	/**
-	 * A connected server's mark, best available first: the icon Windmill ships for
-	 * that integration, then the host's favicon, then a generic plug. The favicon
-	 * step is what covers the servers Windmill has no icon for, which is most of
-	 * them — MCP's own `icons` field is too new for many servers to populate.
+	 * A connected server's mark, best available first: the icon the MCP server itself
+	 * 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.
 	 */
-	let { icon, host, size = 16 }: { icon?: Component<any>; host?: string; size?: number } = $props()
+	let { src, icon, size = 16 }: { src?: string; icon?: Component<any>; size?: number } = $props()

-	// Keyed by host rather than a boolean so reconnecting a path to a different
-	// server retries instead of inheriting the previous one's failure.
+	// 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.
 	let failedFor = $state<string | undefined>(undefined)

 	const px = $derived(`${size}px`)
 </script>

-{#if icon}
-	{@const Icon = icon}
-	<Icon width={px} height={px} />
-{:else if host && failedFor !== host}
+{#if src && failedFor !== src}
 	<img
-		src={faviconUrl(host)}
+		{src}
 		alt=""
 		width={size}
 		height={size}
 		class="rounded-sm"
-		onerror={() => (failedFor = host)}
+		onerror={() => (failedFor = src)}
 	/>
+{:else if icon}
+	{@const Icon = icon}
+	<Icon width={px} height={px} />
 {:else}
 	<Plug {size} class="text-tertiary" />
 {/if}
@@ -1,34 +0,0 @@
-import { describe, expect, it } from 'vitest'
-
-import { providerHost } from './iconCache'
-
-/**
- * `providerHost` decides which hostnames are sent to a third-party favicon service.
- * A regression here does not fail visibly — it silently starts disclosing an
- * endpoint — so the cases it withholds are pinned.
- */
-describe('providerHost', () => {
-	it('returns the host of a public server url', () => {
-		expect(providerHost('https://mcp.linear.app/mcp')).toBe('mcp.linear.app')
-	})
-
-	it('withholds loopback and bare addresses', () => {
-		expect(providerHost('http://localhost:3000/mcp')).toBeUndefined()
-		expect(providerHost('http://127.0.0.1:8000/mcp')).toBeUndefined()
-		expect(providerHost('http://10.1.2.3/mcp')).toBeUndefined()
-		// `new URL()` brackets an IPv6 literal, which has no dot to split on.
-		expect(providerHost('http://[::1]:8000/mcp')).toBeUndefined()
-	})
-
-	it('withholds intranet names', () => {
-		expect(providerHost('https://mcp-box/mcp')).toBeUndefined()
-		expect(providerHost('https://mcp.acme.internal/mcp')).toBeUndefined()
-		expect(providerHost('https://Tools.Acme.LOCAL/mcp')).toBeUndefined()
-	})
-
-	it('returns nothing for a value that is not a url', () => {
-		expect(providerHost(undefined)).toBeUndefined()
-		expect(providerHost('not a url')).toBeUndefined()
-		expect(providerHost({ url: 'https://example.com' })).toBeUndefined()
-	})
-})
@@ -9,15 +9,10 @@ import { providerKey } from './providerIcon'
 * open. `edited_at` comes back with the list, so a row that has not been edited
 * since it was cached needs no read at all.
 */
-// `host` backs the favicon fallback for a server Windmill ships no icon for. It is
-// cached alongside the key for the same reason: the url is only in the resource
-// value, which the list endpoint strips.
-type Entry = { key: string | null; editedAt?: string; host?: string }
+type Entry = { key: string | null; editedAt?: string }

-// Versioned: entries written before `host` existed hold a key (often `null`, the
-// no-shipped-icon case the favicon fallback is for) and read as a cache hit, so they
-// would never learn a host and never draw a favicon. Bumping starts them over, at one
-// resource read each.
+// Versioned so a shape change starts entries over rather than reading a stale one,
+// at one resource read each.
 const STORE_KEY = 'mcp_provider_icons_v2'

 function read(): Record<string, Record<string, Entry>> {
@@ -48,54 +43,9 @@ export function cachedProviderKey(
 export function cachedProviderMark(
 	workspace: string,
 	path: string
-): { key: string | null; host?: string } | undefined {
+): { key: string | null } | undefined {
 	const entry = read()[workspace]?.[path]
-	return entry ? { key: entry.key, host: entry.host } : undefined
-}
-
-/** Suffixes that only ever name something inside a network, so a favicon lookup for
- * one would disclose an internal endpoint and could not succeed anyway. */
-const PRIVATE_HOST_SUFFIXES = [
-	'.local',
-	'.localhost',
-	'.internal',
-	'.intranet',
-	'.lan',
-	'.corp',
-	'.home',
-	'.localdomain'
-]
-
-/**
- * The url's host, when it has one worth drawing a favicon for.
- *
- * Fetching a favicon tells the favicon service which host was asked about, so this
- * withholds what it can recognise as private. It cannot recognise all of it: a public
- * domain used internally (`mcp.internal.example.com`) is indistinguishable from any
- * other, so a self-hosted instance still discloses that hostname when its server has
- * no shipped icon.
- */
-export function providerHost(url: unknown): string | undefined {
-	if (typeof url !== 'string') return undefined
-	try {
-		const hostname = new URL(url).hostname.toLowerCase()
-		// A loopback, a bare address, or an intranet single-label name has no favicon
-		// to fetch, and asking would disclose it to the favicon service for nothing.
-		if (!hostname.includes('.') || /^[\d.]+$/.test(hostname)) return undefined
-		if (PRIVATE_HOST_SUFFIXES.some((suffix) => hostname.endsWith(suffix))) return undefined
-		return hostname
-	} catch {
-		return undefined
-	}
-}
-
-export function cachedProviderHost(
-	workspace: string,
-	path: string,
-	editedAt?: string
-): string | undefined {
-	const entry = read()[workspace]?.[path]
-	return entry?.editedAt === editedAt ? entry?.host : undefined
+	return entry ? { key: entry.key } : undefined
 }

 export function rememberProviderKey(
@@ -108,7 +58,7 @@ export function rememberProviderKey(
 	const store = read()
 	store[workspace] = {
 		...(store[workspace] ?? {}),
-		[path]: { key, editedAt, host: providerHost(url) }
+		[path]: { key, editedAt }
 	}
 	try {
 		localStorage.setItem(STORE_KEY, JSON.stringify(store))
@@ -0,0 +1,56 @@
+import { describe, expect, it } from 'vitest'
+
+import { pickMcpIconSrc } from './mcpIcon'
+
+const png = 'data:image/png;base64,iVBORw0KGgo='
+
+/**
+ * `src` is chosen by the MCP server and rendered in the user's browser, so what this
+ * refuses is a security boundary, not a preference — a regression here fails silently.
+ */
+describe('pickMcpIconSrc', () => {
+	it('takes a data URI of a safe image type', () => {
+		expect(pickMcpIconSrc([{ src: png }])).toBe(png)
+		expect(pickMcpIconSrc([{ src: 'data:image/webp;base64,UklGRg==' }])).toBe(
+			'data:image/webp;base64,UklGRg=='
+		)
+	})
+
+	it('refuses schemes the spec calls unsafe', () => {
+		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', () => {
+		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.
+	it('judges a data URI by what it declares, not by mimeType', () => {
+		expect(
+			pickMcpIconSrc([{ src: 'data:image/svg+xml;base64,PHN2Zz4=', mimeType: 'image/png' }])
+		).toBeUndefined()
+	})
+
+	it('refuses an oversized data URI', () => {
+		expect(pickMcpIconSrc([{ src: `data:image/png;base64,${'A'.repeat(70_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('returns nothing for a missing or malformed icons field', () => {
+		expect(pickMcpIconSrc(undefined)).toBeUndefined()
+		expect(pickMcpIconSrc([])).toBeUndefined()
+		expect(pickMcpIconSrc([{ src: 42 }, null, 'nope'])).toBeUndefined()
+	})
+})
@@ -0,0 +1,43 @@
+/** One entry of an MCP `icons` array, as the spec defines it. */
+export type McpIcon = {
+	src?: unknown
+	mimeType?: unknown
+	sizes?: unknown
+	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 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.
+ *
+ * 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.
+ */
+export function pickMcpIconSrc(icons: unknown, dark = false): 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
+}
+
+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}`))
+}
@@ -7,7 +7,7 @@ import { sendUserToast } from '$lib/toast'
 import type { Item } from '$lib/utils'
 import type { AIChatManager } from '../copilot/chat/AIChatManager.svelte'
 import { isMcpEnabled, setMcpEnabled } from './enabledServers'
-import { cachedProviderHost, cachedProviderKey, rememberProviderKey } from './iconCache'
+import { cachedProviderKey, rememberProviderKey } from './iconCache'
 import { loadProviderIcon } from './providerIcon'
 import McpServerIcon from './McpServerIcon.svelte'

@@ -16,7 +16,6 @@ type Row = {
 	editedAt?: string
 	enabled: boolean
 	icon?: Component<any>
-	iconHost?: string
 }

 // A menu is a shortcut, not a directory: past this many the list stops being
@@ -105,7 +104,6 @@ export class McpMenu {
 				const icon = await loadProviderIcon(key)
 				if (seq !== this.#seq) return
 				server.icon = icon
-				server.iconHost = cachedProviderHost(ws, server.path, server.editedAt)
 			})
 		)
 	}
@@ -171,11 +169,11 @@ export class McpMenu {
 				// to read through the live list rather than the row captured here, since
 				// a reload replaces every row object and a getter bound to the old one
 				// would go on reporting the state it was built with.
-				// One component for every row, so a shipped icon, a favicon and the plug
-				// all land at the same size and nothing pulls its label out of line.
+				// One component for every row, so the server's own icon, Windmill's and the
+				// plug all land at the same size and nothing pulls its label out of line.
 				icon: McpServerIcon,
 				get iconProps() {
-					return { icon: row(path)?.icon, host: row(path)?.iconHost, size: 14 }
+					return { icon: row(path)?.icon, size: 14 }
 				},
 				get toggle() {
 					return row(path)?.enabled ?? false
@@ -68,8 +68,7 @@ export const MCP_REGISTRY: McpRegistryEntry[] = [
 		icon: LinearIcon,
 		url: 'https://mcp.linear.app/mcp',
 		auth: 'dcr',
-		tokenHint:
-			'Use a Linear API key. The Read permission is enough for the read tools.',
+		tokenHint: 'Use a Linear API key. The Read permission is enough for the read tools.',
 		docsUrl: 'https://linear.app/docs/mcp'
 	},
 	{
@@ -1,14 +1,20 @@
 import { beforeEach, describe, expect, it, vi } from 'vitest'

-const { existsVariable, getVariable, createVariable, updateVariable, deleteVariable, existsResource } =
-	vi.hoisted(() => ({
-		existsVariable: vi.fn(),
-		getVariable: vi.fn(),
-		createVariable: vi.fn(),
-		updateVariable: vi.fn(),
-		deleteVariable: vi.fn(),
-		existsResource: vi.fn()
-	}))
+const {
+	existsVariable,
+	getVariable,
+	createVariable,
+	updateVariable,
+	deleteVariable,
+	existsResource
+} = vi.hoisted(() => ({
+	existsVariable: vi.fn(),
+	getVariable: vi.fn(),
+	createVariable: vi.fn(),
+	updateVariable: vi.fn(),
+	deleteVariable: vi.fn(),
+	existsResource: vi.fn()
+}))

 vi.mock('$lib/gen', () => ({
 	VariableService: { existsVariable, getVariable, createVariable, updateVariable, deleteVariable },
@@ -1,11 +1,11 @@
 import type { Component } from 'svelte'
 import { ResourceService } from '$lib/gen'
-import { cachedProviderMark, providerHost } from './iconCache'
+import { cachedProviderMark } from './iconCache'
 import { loadProviderIcon, providerKey } from './providerIcon'

-/** What identifies a connected server visually: Windmill's icon for that integration
- * if it ships one, otherwise the host its favicon can be fetched from. */
-export type McpServerMark = { icon?: Component<any>; host?: string }
+/** Windmill's own icon for a connected server's integration, when it ships one. The
+ * server's published icon is preferred over this and is resolved separately. */
+export type McpServerMark = { icon?: Component<any> }

 // One resolution per server per session, shared by every transcript row naming it —
 // a chat can hold dozens of calls against the same server.
@@ -23,14 +23,14 @@ export function resolveMcpServerMark(workspace: string, path: string): Promise<M

 async function load(workspace: string, path: string): Promise<McpServerMark> {
 	const cached = cachedProviderMark(workspace, path)
-	if (cached) return { icon: await loadProviderIcon(cached.key), host: cached.host }
+	if (cached) return { icon: await loadProviderIcon(cached.key) }
 	try {
 		// Deliberately not written back to the shared cache: that entry is keyed by
 		// `editedAt` for the server list's sake, and storing one from here — where the
 		// row is a past call and `editedAt` is unknown — would make every list re-read.
 		const resource = await ResourceService.getResource({ workspace, path })
 		const url = (resource.value as { url?: unknown } | undefined)?.url
-		return { icon: await loadProviderIcon(providerKey(url)), host: providerHost(url) }
+		return { icon: await loadProviderIcon(providerKey(url)) }
 	} catch {
 		return {}
 	}
@@ -1,15 +0,0 @@
-/**
- * A hostname's favicon, from Google's public favicon service.
- *
- * This discloses each consulted hostname to a third party from the user's
- * browser — an accepted tradeoff, and blocked or air-gapped environments must
- * degrade to a local icon through the caller's `onerror`.
- *
- * Hit gstatic directly rather than `www.google.com/s2/favicons`: the app is
- * served with COEP require-corp, and the s2 redirect hop carries no
- * Cross-Origin-Resource-Policy header, so the browser blocks the image. The
- * gstatic endpoint itself responds with CORP: cross-origin.
- */
-export function faviconUrl(hostname: string, size = 64): string {
-	return `https://t3.gstatic.com/faviconV2?client=SOCIAL&type=FAVICON&fallback_opts=TYPE,SIZE,URL&url=https://${encodeURIComponent(hostname)}&size=${size}`
-}