diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index c3531ceb..14f7647f 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -850,6 +850,20 @@ function App() { }, [appElicitationController], ); + // Request ownership must not cross connections. `elicitationBridgeFactory` + // resolves its client at call time, so an entry queued by a previous + // InspectorClient would otherwise be able to rebuild against the NEXT one and + // read (or answer) through a different server. Core aborts its own pending + // app requests during teardown, but that teardown is awaited asynchronously + // while the swap here is synchronous — so drop them on the swap instead of + // racing it. The one client alive at a time owns every live entry. + useEffect(() => { + return () => { + appElicitationController.failAll( + new Error("Connection replaced before the app answered"), + ); + }; + }, [appElicitationController, inspectorClient]); const [managedToolsState, setManagedToolsState] = useState(null); diff --git a/clients/web/src/components/elements/AppElicitation/AppElicitationHost.tsx b/clients/web/src/components/elements/AppElicitation/AppElicitationHost.tsx index c66f932b..1276a724 100644 --- a/clients/web/src/components/elements/AppElicitation/AppElicitationHost.tsx +++ b/clients/web/src/components/elements/AppElicitation/AppElicitationHost.tsx @@ -226,11 +226,12 @@ function AppElicitationFrame({ {/* `aria-label` and the `data-*` attributes go on the CONTENT (the role="dialog" element), not the Root — the Root is only a portal wrapper, so a name placed there never reaches the dialog. Named per - request rather than per prompt: two concurrent elicitations can carry - the same message, and two identically-named dialogs are what - `landmark-unique` rejects. */} + request rather than per prompt or per app: two concurrent + elicitations can be for the SAME app URI with the SAME message, and + only the request id tells those two dialogs apart — for + `landmark-unique` and for anyone navigating by screen reader. */} diff --git a/clients/web/src/components/elements/AppRenderer/appCapabilities.test.ts b/clients/web/src/components/elements/AppRenderer/appCapabilities.test.ts index 6e15829e..6979d170 100644 --- a/clients/web/src/components/elements/AppRenderer/appCapabilities.test.ts +++ b/clients/web/src/components/elements/AppRenderer/appCapabilities.test.ts @@ -70,6 +70,30 @@ describe("appCapabilities (#1854)", () => { expect(appAdvertisesElicitation(bridge)).toBe(false); }); + it("ignores frames that are not shaped like ui/initialize at all", () => { + // The frame comes from sandboxed view code, so nothing about its shape is + // guaranteed — a non-object, or an initialize with no params, must not + // throw on the way through to the bridge's own handler. + const bridge = makeBridge({}); + const { transport, inner } = makeTransport(); + observeAppCapabilities(bridge, transport); + transport.onmessage?.("not-a-frame" as unknown as JSONRPCMessage); + transport.onmessage?.(null as unknown as JSONRPCMessage); + transport.onmessage?.({ + jsonrpc: "2.0", + id: 1, + method: "ui/initialize", + } as unknown as JSONRPCMessage); + transport.onmessage?.({ + jsonrpc: "2.0", + id: 2, + method: "ui/initialize", + params: null, + } as unknown as JSONRPCMessage); + expect(appAdvertisesElicitation(bridge)).toBe(false); + expect(inner).toHaveBeenCalledTimes(4); + }); + it("keeps bridges independent", () => { const a = makeBridge({}); const b = makeBridge({}); diff --git a/clients/web/src/components/elements/AppRenderer/appCapabilities.ts b/clients/web/src/components/elements/AppRenderer/appCapabilities.ts index 4ff832c1..a676fe98 100644 --- a/clients/web/src/components/elements/AppRenderer/appCapabilities.ts +++ b/clients/web/src/components/elements/AppRenderer/appCapabilities.ts @@ -1,5 +1,4 @@ import type { AppBridge } from "@modelcontextprotocol/ext-apps/app-bridge"; -import type { Transport } from "@modelcontextprotocol/client"; /** * The app capabilities exactly as the view sent them, per bridge. @@ -22,10 +21,41 @@ import type { Transport } from "@modelcontextprotocol/client"; */ const rawAppCapabilities = new WeakMap>(); -/** Shape of the `ui/initialize` frame this reads — nothing else is touched. */ -interface UiInitializeFrame { - method?: unknown; - params?: { appCapabilities?: unknown }; +/** + * The only part of a transport this needs: the inbound-message callback. + * + * Structural, and generic over the message type, on purpose. ext-apps' + * `PostMessageTransport` implements the SDK *v1* `Transport` — a different + * nominal type from the v2 client's, though runtime-identical — so naming + * either would force a cast at the call site. Generic rather than + * `(message: unknown)` because a handler typed for a narrower message is not + * assignable to one typed for `unknown` (contravariance), which would put the + * cast back; `rest` is `never[]` for the same reason, accepting any trailing + * parameter list without claiming to know it. The message is only ever read + * through {@link recordAdvertisedCapabilities}, which narrows from `unknown`. + */ +export interface MessageObservable { + onmessage?: (message: TMessage, ...rest: never[]) => void; +} + +/** + * Record `params.appCapabilities` if `message` is a `ui/initialize` frame. + * Narrows from `unknown` rather than asserting a shape: the frame is whatever + * the sandboxed view posted. + */ +function recordAdvertisedCapabilities( + bridge: AppBridge, + message: unknown, +): void { + if (typeof message !== "object" || message === null) return; + const frame = message as { method?: unknown; params?: unknown }; + if (frame.method !== "ui/initialize") return; + if (typeof frame.params !== "object" || frame.params === null) return; + const advertised = (frame.params as { appCapabilities?: unknown }) + .appCapabilities; + if (typeof advertised === "object" && advertised !== null) { + rawAppCapabilities.set(bridge, advertised as Record); + } } /** @@ -36,20 +66,14 @@ interface UiInitializeFrame { * wraps. That ordering is safe: the view cannot send `ui/initialize` until the * host has pushed its HTML into the sandbox, which happens later still. */ -export function observeAppCapabilities( +export function observeAppCapabilities( bridge: AppBridge, - transport: Transport, + transport: MessageObservable, ): void { const inner = transport.onmessage?.bind(transport); - transport.onmessage = (message, extra) => { - const frame = message as UiInitializeFrame; - if (frame.method === "ui/initialize") { - const advertised = frame.params?.appCapabilities; - if (typeof advertised === "object" && advertised !== null) { - rawAppCapabilities.set(bridge, advertised as Record); - } - } - inner?.(message, extra); + transport.onmessage = (message: TMessage, ...rest: never[]) => { + recordAdvertisedCapabilities(bridge, message); + inner?.(message, ...rest); }; } diff --git a/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.test.ts b/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.test.ts index cccdd33d..7f32c26b 100644 --- a/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.test.ts +++ b/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.test.ts @@ -158,6 +158,33 @@ describe("createAppBridgeFactory", () => { } }); + it("does not advertise hostCapabilities.elicitation by default (#1854)", async () => { + // An App-tool frame is never handed an elicitation, so claiming the host + // capability there would tell those apps something untrue about the host. + const factory = createAppBridgeFactory({ + getClient: () => fakeClient, + readResource: vi.fn(), + }); + await factory(makeIframe(), { kind: "tool", tool }); + expect(bridgeInstances[0].ctorArgs[2]).not.toHaveProperty("elicitation"); + }); + + it("advertises hostCapabilities.elicitation when opted in (#1854)", async () => { + // The value reaches the AppBridge CONSTRUCTOR, which is what the bridge + // echoes in its `ui/initialize` response — an app reads it there to decide + // whether this host will forward an elicitation to it at all. + const factory = createAppBridgeFactory({ + advertiseElicitation: true, + getClient: () => fakeClient, + readResource: vi.fn(), + }); + await factory(makeIframe(), { + kind: "resource", + resourceUri: "ui://demo/pick.html", + }); + expect(bridgeInstances[0].ctorArgs[2]).toMatchObject({ elicitation: {} }); + }); + it("on sandboxready, reads the UI resource, wraps the html with the per-app CSP, and echoes the approved sandbox config", async () => { const readResource = vi.fn().mockResolvedValue( uiResource("

weather

", { diff --git a/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.ts b/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.ts index 3aea5c1c..1447d359 100644 --- a/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.ts +++ b/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.ts @@ -8,7 +8,7 @@ import type { McpUiHostCapabilities, McpUiResourceMeta, } from "@modelcontextprotocol/ext-apps/app-bridge"; -import type { Client, Transport } from "@modelcontextprotocol/client"; +import type { Client } from "@modelcontextprotocol/client"; import type { EmbeddedResource, Implementation, @@ -373,7 +373,7 @@ export function createAppBridgeFactory( // Record the view's raw `ui/initialize` capabilities before the bridge's // own schema strips the keys it predates (#1854). Must follow `connect`, // which is what installs the handler this wraps. - observeAppCapabilities(bridge, transport as unknown as Transport); + observeAppCapabilities(bridge, transport); return bridge; }; } diff --git a/clients/web/src/lib/appElicitationController.test.ts b/clients/web/src/lib/appElicitationController.test.ts index ee2ef8fd..d59da9d9 100644 --- a/clients/web/src/lib/appElicitationController.test.ts +++ b/clients/web/src/lib/appElicitationController.test.ts @@ -108,6 +108,33 @@ describe("AppElicitationController (#1854)", () => { expect(controller.getEntries()).toHaveLength(0); }); + it("failAll drops every entry so a renderer cannot outlive its connection", async () => { + // The host calls this when the InspectorClient is replaced: the bridge + // factory resolves its client at call time, so an entry left queued could + // otherwise rebuild against the NEXT connection and answer through a + // different server. + const controller = new AppElicitationController(); + const listener = vi.fn(); + const first = request(controller, "a"); + const second = request(controller, "b"); + controller.subscribe(listener); + + controller.failAll(new Error("connection replaced")); + + await expect(first).rejects.toThrow(/connection replaced/); + await expect(second).rejects.toThrow(/connection replaced/); + expect(controller.getEntries()).toHaveLength(0); + expect(listener).toHaveBeenCalledTimes(1); + }); + + it("failAll on an empty queue notifies nobody", () => { + const controller = new AppElicitationController(); + const listener = vi.fn(); + controller.subscribe(listener); + controller.failAll(new Error("nothing to drop")); + expect(listener).not.toHaveBeenCalled(); + }); + it("stops notifying an unsubscribed listener", () => { const controller = new AppElicitationController(); const listener = vi.fn(); diff --git a/clients/web/src/lib/appElicitationController.ts b/clients/web/src/lib/appElicitationController.ts index d529cee2..49bdd12d 100644 --- a/clients/web/src/lib/appElicitationController.ts +++ b/clients/web/src/lib/appElicitationController.ts @@ -96,4 +96,20 @@ export class AppElicitationController { fail(requestId: string, error: Error): void { this.take(requestId)?.reject(error); } + + /** + * Drop every queued request at once. + * + * For the host's "these entries no longer belong to the live connection" + * moment (a replaced `InspectorClient`): a renderer that outlived its client + * would rebuild against the *next* one and could read or answer through a + * different server. + */ + failAll(error: Error): void { + const dropped = this.entries; + if (dropped.length === 0) return; + this.entries = []; + this.emit(); + for (const entry of dropped) entry.reject(error); + } } diff --git a/clients/web/src/test/core/mcp/appElicitation.test.ts b/clients/web/src/test/core/mcp/appElicitation.test.ts index cf433f16..5f0cd45b 100644 --- a/clients/web/src/test/core/mcp/appElicitation.test.ts +++ b/clients/web/src/test/core/mcp/appElicitation.test.ts @@ -215,7 +215,7 @@ describe("appElicitation negotiation (#1854)", () => { params, null as unknown as ElicitResult, ), - ).toMatch(/non-object/); + ).toMatch(/invalid elicitation result/); }); it("rejects an unknown action", () => { @@ -223,7 +223,19 @@ describe("appElicitation negotiation (#1854)", () => { validateAppElicitResult(provider, params, { action: "maybe", } as unknown as ElicitResult), - ).toMatch(/unknown elicitation action/); + ).toMatch(/invalid elicitation result/); + }); + + it("rejects a decline whose content the standard result forbids", () => { + // Checking `action` alone would wave this through: `ElicitResult` permits + // only primitives and string arrays in `content`, so a nested object + // would reach the server as a result it can legitimately reject. + expect( + validateAppElicitResult(provider, params, { + action: "decline", + content: { x: {} }, + } as unknown as ElicitResult), + ).toMatch(/invalid elicitation result/); }); it("rejects an accept with no usable content", () => { @@ -237,7 +249,7 @@ describe("appElicitation negotiation (#1854)", () => { action: "accept", content: [] as unknown as Record, } as ElicitResult), - ).toMatch(/without a content object/); + ).toMatch(/invalid elicitation result|without a content object/); }); it("rejects content that does not match the requested schema", () => { diff --git a/clients/web/src/test/core/mcp/inspectorClient-app-elicitation.test.ts b/clients/web/src/test/core/mcp/inspectorClient-app-elicitation.test.ts index fd12401e..25216b41 100644 --- a/clients/web/src/test/core/mcp/inspectorClient-app-elicitation.test.ts +++ b/clients/web/src/test/core/mcp/inspectorClient-app-elicitation.test.ts @@ -405,6 +405,38 @@ describe("app-rendered elicitation routing (#1854)", () => { }); }); + describe("request-scoped ids", () => { + it("mints ids that are unique across client instances, not just within one", async () => { + // A host may hold ONE renderer across replacement clients (the web client + // rebuilds its InspectorClient on a settings change). A per-client + // counter alone would give each new client's first request the same id, + // and settling it would resolve the wrong server's request. + const ids: string[] = []; + const renderer: AppElicitationRenderer = async (request) => { + ids.push(request.requestId); + return { action: "cancel" }; + }; + const first = new ElicitTransport(); + const clientA = await connectClient({ + transport: first, + appElicitation: renderer, + }); + await first.elicit(40, appParams()); + await clientA.disconnect(); + + const second = new ElicitTransport(); + const clientB = await connectClient({ + transport: second, + appElicitation: renderer, + }); + await second.elicit(41, appParams()); + await clientB.disconnect(); + + expect(ids).toHaveLength(2); + expect(ids[0]).not.toBe(ids[1]); + }); + }); + describe("teardown", () => { it("aborts a pending app elicitation on disconnect", async () => { const transport = new ElicitTransport(); @@ -427,5 +459,31 @@ describe("app-rendered elicitation routing (#1854)", () => { // abandoned it, and the connection it belonged to is gone. expect(client.getPendingElicitations()).toHaveLength(0); }); + + it("aborts a pending app elicitation on a mid-session transport close", async () => { + // The `onclose` route reaches teardown only through + // `clearAndAnnouncePendingPeerRequests`, whose emptiness check used to + // look at the native queues alone — so a lone app elicitation survived a + // dropped connection and left its modal on screen. + const transport = new ElicitTransport(); + let aborted = false; + const client = await connectClient({ + transport, + appElicitation: (request) => + new Promise((_resolve, reject) => { + request.signal.addEventListener("abort", () => { + aborted = true; + reject(new Error("aborted")); + }); + }), + }); + void transport.elicit(31, appParams()).catch(() => {}); + await vi.waitFor(() => expect(aborted).toBe(false)); + + transport.onclose?.(); + + await vi.waitFor(() => expect(aborted).toBe(true)); + expect(client.getPendingElicitations()).toHaveLength(0); + }); }); }); diff --git a/core/mcp/appElicitation.ts b/core/mcp/appElicitation.ts index adfff56c..1ea84651 100644 --- a/core/mcp/appElicitation.ts +++ b/core/mcp/appElicitation.ts @@ -6,6 +6,7 @@ import type { jsonSchemaValidator, ServerCapabilities, } from "@modelcontextprotocol/client"; +import { ElicitResultSchema } from "@modelcontextprotocol/core"; import { MCP_APP_MIME_TYPE, UI_EXTENSION_KEY } from "./extensions.js"; /** @@ -149,8 +150,13 @@ export function isFormElicitation(params: ElicitRequest["params"]): boolean { return mode === undefined || mode === "form"; } -/** The three actions a completed elicitation may report. */ -const ELICIT_ACTIONS = ["accept", "decline", "cancel"] as const; +/** + * The result contract a completed elicitation must satisfy — the standard MCP + * one, not a hand-rolled subset. Checking `action` by hand would accept, for + * instance, `{ action: "decline", content: { x: {} } }`, whose content the + * schema forbids, and hand the server a result it can reject. + */ +const ELICIT_RESULT_ERROR = "App returned an invalid elicitation result"; /** * Validates what an app returned before it is handed back to the server. @@ -170,16 +176,13 @@ export function validateAppElicitResult( params: ElicitRequest["params"], result: ElicitResult, ): string | undefined { - if (typeof result !== "object" || result === null) { - return "App returned a non-object elicitation result"; - } - const action = (result as { action?: unknown }).action; - if (!ELICIT_ACTIONS.includes(action as (typeof ELICIT_ACTIONS)[number])) { - return `App returned an unknown elicitation action: ${JSON.stringify(action)}`; + const parsed = ElicitResultSchema.safeParse(result); + if (!parsed.success) { + return `${ELICIT_RESULT_ERROR}: ${parsed.error.issues[0]?.message ?? "does not match ElicitResult"}`; } // decline / cancel carry no content — they are complete as they stand. - if (action !== "accept") return undefined; - const content = (result as { content?: unknown }).content; + if (parsed.data.action !== "accept") return undefined; + const content = parsed.data.content; if ( typeof content !== "object" || content === null || diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index b2b6ae88..9ed280b2 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -584,6 +584,17 @@ export class InspectorClient extends InspectorClientEventTarget { * elicitations cannot resolve through each other's bridges. */ private appElicitationSeq = 0; + /** + * Per-instance prefix for {@link appElicitationSeq}. + * + * A counter alone is not enough: a host may hold ONE renderer across + * replacement clients (the web client rebuilds its `InspectorClient` on a + * settings change), and every fresh instance would otherwise mint + * `app-elicitation-1` for its first request. Settling that id would then + * resolve — or discard — whichever connection's request the host happened to + * be keying. + */ + private readonly appElicitationPrefix = `app-elicitation-${globalThis.crypto?.randomUUID?.() ?? Math.random().toString(36).slice(2)}`; /** * Abort controllers for app-rendered elicitations still awaiting an answer. * Aborted alongside the native pending queue on disconnect, so a rendered app @@ -1755,7 +1766,12 @@ export class InspectorClient extends InspectorClientEventTarget { private clearAndAnnouncePendingPeerRequests(): void { if ( this.pendingSamples.length === 0 && - this.pendingElicitations.length === 0 + this.pendingElicitations.length === 0 && + // App-rendered elicitations (#1854) are not in either array — they live in + // the host's renderer — so an emptiness check that ignores them lets a + // mid-session close (the `onclose` route, which reaches teardown only + // through here) leave a modal open for a connection that is gone. + this.activeAppElicitations.size === 0 ) { return; } @@ -2946,7 +2962,7 @@ export class InspectorClient extends InspectorClientEventTarget { this.activeAppElicitations.add(controller); try { const result = await renderer({ - requestId: `app-elicitation-${++this.appElicitationSeq}`, + requestId: `${this.appElicitationPrefix}-${++this.appElicitationSeq}`, resourceUri, params: request.params, signal: controller.signal, diff --git a/scripts/smoke-web-elicitation.mjs b/scripts/smoke-web-elicitation.mjs index 16cf2bd9..d8169f6c 100644 --- a/scripts/smoke-web-elicitation.mjs +++ b/scripts/smoke-web-elicitation.mjs @@ -60,6 +60,9 @@ const PORT = process.env.SMOKE_WEB_ELICIT_PORT ?? "6296"; const TOKEN = "smoke-web-elicit-token"; const TOOL = "app_choose_option"; const SHOT_DIR = process.env.SMOKE_SCREENSHOT_DIR; +// The async half of the uncaught-crash class. Kept identical to +// smoke-web-browser.mjs / smoke-web-app.mjs. +const FATAL_CONSOLE = /^Uncaught\b|Failed to fetch dynamically imported module/; const servers = []; let browser = null; @@ -235,10 +238,20 @@ try { viewport: { width: 1280, height: 900 }, }); + // Uncaught *synchronous* page errors, and their *async* twin (an unhandled + // rejection or a failed dynamic import), which Chromium reports on the + // console channel instead. Both are hard failures; every other console error + // is only a diagnostic, so benign font/React noise cannot flake CI. Same + // split as smoke:web:browser and smoke:web:app, which document it at length. const pageErrors = []; + const consoleErrors = []; page.on("pageerror", (err) => pageErrors.push(err instanceof Error ? err.message : String(err)), ); + page.on("console", (msg) => { + if (msg.type() === "error") consoleErrors.push(msg.text()); + }); + const fatalConsole = () => consoleErrors.filter((m) => FATAL_CONSOLE.test(m)); const drive = async () => { // ── 1. Negotiated: the server's app answers the elicitation ──────────── @@ -304,13 +317,26 @@ try { try { await Promise.race([web.whenChildExits(), drive()]); } catch (err) { + const diagnostics = [ + ...pageErrors, + ...fatalConsole().map((m) => `console: ${m}`), + ]; await fail( `${err instanceof Error ? err.message : String(err)}${ - pageErrors.length ? ` — page errors: ${pageErrors.join("; ")}` : "" + diagnostics.length + ? ` — page diagnostics: ${diagnostics.join("; ")}` + : "" }`, ); } + // A drive that reached all of its assertions still fails if the page threw on + // the way: without this the smoke prints OK over a broken bundle. + const fatal = [...pageErrors, ...fatalConsole()]; + if (fatal.length > 0) { + await fail(`page logged uncaught error(s): ${fatal.join("; ")}`); + } + console.log("smoke:web:elicit OK — app-rendered and native paths both drive"); await shutdown(); } catch (err) {