diff --git a/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte b/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte index 5ada7e16f8..5e856f4754 100644 --- a/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte +++ b/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte @@ -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 - 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.
{#each servers as server (server.path)} {#snippet icon()} - + {/snippet} {#snippet title()} {server.path} diff --git a/frontend/src/lib/components/copilot/chat/ToolExecutionDisplay.svelte b/frontend/src/lib/components/copilot/chat/ToolExecutionDisplay.svelte index c450ca1244..b714e1a60f 100644 --- a/frontend/src/lib/components/copilot/chat/ToolExecutionDisplay.svelte +++ b/frontend/src/lib/components/copilot/chat/ToolExecutionDisplay.svelte @@ -269,13 +269,17 @@ {/snippet} + 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} + + {:else if mcpServerPath && aiChatManager.operatingWorkspace} {#await resolveMcpServerMark(aiChatManager.operatingWorkspace, mcpServerPath) then mark} - {#if mark.icon || mark.host} - + {#if mark.icon} + {/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} > {#snippet belowHeader()} diff --git a/frontend/src/lib/components/copilot/chat/WebSearchSourcesDisplay.svelte b/frontend/src/lib/components/copilot/chat/WebSearchSourcesDisplay.svelte index 81a620b91a..8b954362b1 100644 --- a/frontend/src/lib/components/copilot/chat/WebSearchSourcesDisplay.svelte +++ b/frontend/src/lib/components/copilot/chat/WebSearchSourcesDisplay.svelte @@ -1,7 +1,6 @@ diff --git a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts b/frontend/src/lib/components/copilot/chat/global/mcpTools.ts index 844c9017dc..e96c668094 100644 --- a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts +++ b/frontend/src/lib/components/copilot/chat/global/mcpTools.ts @@ -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 diff --git a/frontend/src/lib/components/copilot/chat/shared.ts b/frontend/src/lib/components/copilot/chat/shared.ts index 5fd22d9ad1..57d116c765 100644 --- a/frontend/src/lib/components/copilot/chat/shared.ts +++ b/frontend/src/lib/components/copilot/chat/shared.ts @@ -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 diff --git a/frontend/src/lib/components/mcp/McpServerIcon.svelte b/frontend/src/lib/components/mcp/McpServerIcon.svelte index 88ea3c762a..c04906cec6 100644 --- a/frontend/src/lib/components/mcp/McpServerIcon.svelte +++ b/frontend/src/lib/components/mcp/McpServerIcon.svelte @@ -1,35 +1,38 @@ -{#if icon} - {@const Icon = icon} - -{:else if host && failedFor !== host} +{#if src && failedFor !== src} (failedFor = host)} + onerror={() => (failedFor = src)} /> +{:else if icon} + {@const Icon = icon} + {:else} {/if} diff --git a/frontend/src/lib/components/mcp/iconCache.test.ts b/frontend/src/lib/components/mcp/iconCache.test.ts deleted file mode 100644 index 6fcde2e44f..0000000000 --- a/frontend/src/lib/components/mcp/iconCache.test.ts +++ /dev/null @@ -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() - }) -}) diff --git a/frontend/src/lib/components/mcp/iconCache.ts b/frontend/src/lib/components/mcp/iconCache.ts index 120106c37b..7aeadfbfbb 100644 --- a/frontend/src/lib/components/mcp/iconCache.ts +++ b/frontend/src/lib/components/mcp/iconCache.ts @@ -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> { @@ -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)) diff --git a/frontend/src/lib/components/mcp/mcpIcon.test.ts b/frontend/src/lib/components/mcp/mcpIcon.test.ts new file mode 100644 index 0000000000..5f1f8e7680 --- /dev/null +++ b/frontend/src/lib/components/mcp/mcpIcon.test.ts @@ -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() + }) +}) diff --git a/frontend/src/lib/components/mcp/mcpIcon.ts b/frontend/src/lib/components/mcp/mcpIcon.ts new file mode 100644 index 0000000000..8ff74f574e --- /dev/null +++ b/frontend/src/lib/components/mcp/mcpIcon.ts @@ -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}`)) +} diff --git a/frontend/src/lib/components/mcp/mcpMenu.svelte.ts b/frontend/src/lib/components/mcp/mcpMenu.svelte.ts index 50d7e1dad6..e2ffc93129 100644 --- a/frontend/src/lib/components/mcp/mcpMenu.svelte.ts +++ b/frontend/src/lib/components/mcp/mcpMenu.svelte.ts @@ -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 - 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 diff --git a/frontend/src/lib/components/mcp/registry.ts b/frontend/src/lib/components/mcp/registry.ts index a9bacce3e0..2b305c2a37 100644 --- a/frontend/src/lib/components/mcp/registry.ts +++ b/frontend/src/lib/components/mcp/registry.ts @@ -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' }, { diff --git a/frontend/src/lib/components/mcp/secretVariable.test.ts b/frontend/src/lib/components/mcp/secretVariable.test.ts index ced0f8e611..86fa5d8675 100644 --- a/frontend/src/lib/components/mcp/secretVariable.test.ts +++ b/frontend/src/lib/components/mcp/secretVariable.test.ts @@ -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 }, diff --git a/frontend/src/lib/components/mcp/serverMark.ts b/frontend/src/lib/components/mcp/serverMark.ts index 05d3033251..6ea0d6a390 100644 --- a/frontend/src/lib/components/mcp/serverMark.ts +++ b/frontend/src/lib/components/mcp/serverMark.ts @@ -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; 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 } // 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 { 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 {} } diff --git a/frontend/src/lib/utils/faviconUrl.ts b/frontend/src/lib/utils/faviconUrl.ts deleted file mode 100644 index 3124450e66..0000000000 --- a/frontend/src/lib/utils/faviconUrl.ts +++ /dev/null @@ -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}` -}