diff --git a/README.md b/README.md index db796475..d9fce752 100644 --- a/README.md +++ b/README.md @@ -172,7 +172,7 @@ For the scripted version of the same flow (`--app-info` probe → deep link → That field is how a server asks its host for a stable, dedicated origin. Without one, an App renders into a `srcdoc` frame sandboxed without `allow-same-origin`, so its document has an *opaque* origin and every request it makes carries `Origin: null` — which no CORS policy, OAuth callback, or API-key allowlist can admit ([#2056](https://github.com/modelcontextprotocol/inspector/issues/2056)). -Open the Apps tab and run `mcp_app_demo`. The widget renders identically to `mcp-app-http.json` — the difference is not visual. Inspect the inner iframe in devtools: on this server it is served from `http://localhost:6276/app-document/` and `location.origin` is that real origin, where on `mcp-app-http.json` it is `about:srcdoc` with an origin of `null`. +Open the Apps tab and run `mcp_app_demo`. The widget renders identically to `mcp-app-http.json` — the difference is not visual. Inspect the inner iframe in devtools: on this server it is served from `http://127.0.0.1:6276/app-document/` and `location.origin` is that real origin, where on `mcp-app-http.json` it is `about:srcdoc` with an origin of `null`. (The host is whatever the Inspector bound to — `127.0.0.1` by default, an *address* rather than the name `localhost`, for the reason `resolve-bind-host.ts` documents.) The spec makes `domain`'s format **host-dependent**, and the Inspector owns no domain infrastructure — so it reads any non-empty value as a *request* rather than an address, and answers with a real loopback origin of its own. See [MCP App dedicated origins](./clients/web/README.md#mcp-app-dedicated-origins-metauidomain) for the full contract, including what the one shared origin does and does not isolate, and how every failure falls back to the default render rather than blanking the app. diff --git a/clients/web/README.md b/clients/web/README.md index f6963294..5065ea17 100644 --- a/clients/web/README.md +++ b/clients/web/README.md @@ -160,7 +160,11 @@ Inspector owns no domain infrastructure — it cannot serve - The app document is served from there under an unguessable path, its per-app CSP delivered as a real response **header** (stronger than the `` the `srcdoc` path relies on) plus a `frame-ancestors` that admits - only the sandbox proxy. + only the two origins in its real ancestor chain — the sandbox proxy that + frames it, **and** the Inspector page that frames the proxy. Both are + required, not belt-and-braces: `frame-ancestors` is checked against every + ancestor, so omitting the Inspector's own origin blocks the frame outright + (see `appDocumentEmbedders`). - The inner iframe is granted `allow-same-origin` on this path **only** — that is what makes the origin real rather than opaque. It is not a weakening of #1565: the listener is on its own port, so the app is diff --git a/clients/web/server/app-origin-controller.ts b/clients/web/server/app-origin-controller.ts index f455542a..a13bfe84 100644 --- a/clients/web/server/app-origin-controller.ts +++ b/clients/web/server/app-origin-controller.ts @@ -273,8 +273,15 @@ export function createAppOriginController( console.error("App origin server error:", err); } // Degrade rather than reject: `publish` then returns null and the - // renderer keeps working on the default srcdoc path. + // renderer keeps working on the default srcdoc path. Clearing + // `origin` is what makes that true — `publish` gates on it, not on + // `server`, so an error arriving AFTER a successful listen would + // otherwise keep minting URLs on a dead listener and the browser + // would never take the promised fallback. Dropping the documents + // with it releases their memory, since nothing can fetch them now. server = null; + origin = null; + documents.clear(); settle({ port: 0, url: "" }); }); diff --git a/clients/web/server/sandbox-controller.ts b/clients/web/server/sandbox-controller.ts index 148ceb62..042e8b3f 100644 --- a/clients/web/server/sandbox-controller.ts +++ b/clients/web/server/sandbox-controller.ts @@ -79,9 +79,11 @@ export function sandboxFrameAncestors(allowedOrigins?: string[]): string { * {@link LOOPBACK_FRAME_ANCESTORS} when nothing valid remains. * * Shared with the app-origin server (#2056), which restricts a published app - * document to being framed by the sandbox proxy. Same grammar, same failure - * mode — a directive whose only source is invalid degrades to `'none'` and - * blocks the frame — so the two derive it here rather than each rolling one. + * document to being framed by the sandbox proxy and the Inspector page above + * it. Same grammar, same fallback — an all-invalid list yields the loopback + * family rather than an empty source list, since a `frame-ancestors` with no + * sources means `'none'` and would block the frame outright — so the two + * derive it here rather than each rolling one. */ export function frameAncestorsDirective(origins?: string[]): string { const valid = (origins ?? []).filter((o) => CSP_HOST_SOURCE.test(o)); diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 2b638d79..b7f7a11f 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -391,6 +391,24 @@ vi.mock( }, ); +// --- publishAppDocument spy (#2056) ----------------------------------------- +// The dedicated-origin publisher App hands both bridge factories. Mocked at the +// module so a test can assert the wiring is live — the factory's own tests +// inject the dep and cannot see App failing to supply it. +const publishAppDocumentMock = + vi.fn< + ( + doc: { html: string; csp?: string }, + opts: { baseUrl: string; authToken?: string }, + ) => Promise + >(); +vi.mock("./lib/publishAppDocument", () => ({ + publishAppDocument: ( + doc: { html: string; csp?: string }, + opts: { baseUrl: string; authToken?: string }, + ) => publishAppDocumentMock(doc, opts), +})); + // --- InspectorView double --------------------------------------------------- // Surfaces each piece of session-scoped state under test and exposes buttons // that invoke the App's connect / call-tool / get-prompt / read-resource / @@ -3463,4 +3481,40 @@ describe("App MCP App listed-resource metadata wiring (#2055)", () => { ).toBeUndefined(); } }); + + it("hands BOTH bridge factories a working publishAppDocument (#2056)", async () => { + // Both call sites pass the same `publishDocument`, and the end-to-end smoke + // only covers the Apps-screen one — so without this the elicitation + // factory's `publishAppDocument` could be dropped and every test stays + // green while a domain-declaring elicitation app silently loses its real + // origin. Assert it is not merely present but actually reaches the lib. + publishAppDocumentMock.mockResolvedValue( + "http://127.0.0.1:6276/app-document/deadbeef", + ); + + renderWithMantine(); + + const appsFactory = () => + appBridgeFactoryDeps.find((d) => !d.advertiseElicitation); + const elicitationFactory = () => + appBridgeFactoryDeps.find((d) => d.advertiseElicitation === true); + await waitFor(() => { + expect(appsFactory()).toBeDefined(); + expect(elicitationFactory()).toBeDefined(); + }); + + for (const deps of [appsFactory(), elicitationFactory()]) { + publishAppDocumentMock.mockClear(); + await expect( + deps?.publishAppDocument?.({ + html: "

app

", + csp: "default-src 'none'", + }), + ).resolves.toBe("http://127.0.0.1:6276/app-document/deadbeef"); + expect(publishAppDocumentMock).toHaveBeenCalledWith( + { html: "

app

", csp: "default-src 'none'" }, + expect.objectContaining({ baseUrl: expect.any(String) }), + ); + } + }); }); diff --git a/clients/web/src/lib/publishAppDocument.test.ts b/clients/web/src/lib/publishAppDocument.test.ts index 289cdfdb..1f435683 100644 --- a/clients/web/src/lib/publishAppDocument.test.ts +++ b/clients/web/src/lib/publishAppDocument.test.ts @@ -10,12 +10,16 @@ import { describe, it, expect, vi } from "vitest"; import { publishAppDocument } from "./publishAppDocument"; +/** + * A real `Response`, not a shaped literal. The platform implementation is + * available here, so building one keeps `ok`/`status`/`json()` honest rather + * than asserting a hand-written object is a Response. + */ function jsonResponse(body: unknown, status = 200): Response { - return { - ok: status >= 200 && status < 300, + return new Response(JSON.stringify(body), { status, - json: async () => body, - } as unknown as Response; + headers: { "Content-Type": "application/json" }, + }); } describe("publishAppDocument", () => { @@ -88,13 +92,11 @@ describe("publishAppDocument", () => { }); it("resolves null when the response body is not JSON", async () => { - const fetchFn = vi.fn().mockResolvedValue({ - ok: true, - status: 200, - json: async () => { - throw new Error("not json"); - }, - } as unknown as Response); + // A real 200 whose body is not JSON, so `json()` rejects for the reason + // the production path has to survive rather than because a stub threw. + const fetchFn = vi + .fn() + .mockResolvedValue(new Response("not json", { status: 200 })); await expect( publishAppDocument( { html: "

app

" }, diff --git a/clients/web/src/test/integration/mcp/remote/app-document-route.test.ts b/clients/web/src/test/integration/mcp/remote/app-document-route.test.ts index d9939f38..d60a41c5 100644 --- a/clients/web/src/test/integration/mcp/remote/app-document-route.test.ts +++ b/clients/web/src/test/integration/mcp/remote/app-document-route.test.ts @@ -84,6 +84,20 @@ describe("createRemoteApp POST /api/app-document", () => { expect(res.status).toBe(400); }); + it.each([ + ["a JSON null body", "null"], + ["a JSON array body", "[1,2]"], + ])("rejects %s with 400 rather than a 500", async (_label, raw) => { + // Both parse cleanly, so the try/catch above sees nothing — destructuring + // them is what used to throw, outside it, turning a bad shape into a 500. + const publish = vi + .fn<(doc: { html: string; csp?: string }) => { url: string } | null>() + .mockReturnValue({ url: "http://x/y" }); + const res = await appWith(publish).request(post(raw)); + expect(res.status).toBe(400); + expect(publish).not.toHaveBeenCalled(); + }); + it.each([ ["missing html", {}], ["empty html", { html: "" }], diff --git a/clients/web/src/test/integration/server/app-origin-controller.test.ts b/clients/web/src/test/integration/server/app-origin-controller.test.ts index dced7e5e..1126b630 100644 --- a/clients/web/src/test/integration/server/app-origin-controller.test.ts +++ b/clients/web/src/test/integration/server/app-origin-controller.test.ts @@ -337,6 +337,58 @@ describe("createAppOriginController", () => { } }); + it("stops minting URLs when the listener errors AFTER it bound", async () => { + // `publish` gates on `origin`, not on `server`. An error arriving once the + // listener is live must therefore clear `origin` too — otherwise the + // browser keeps being handed URLs on a dead port and never takes the + // srcdoc fallback the caller is promised. + vi.resetModules(); + let emitter!: EventEmitter; + vi.doMock("node:http", async () => { + const actual = + await vi.importActual("node:http"); + return { + ...actual, + createServer: () => { + emitter = new EventEmitter(); + return Object.assign(emitter, { + listen: (_p: number, _h: string, cb?: () => void) => + setImmediate(() => cb?.()), + address: () => ({ + address: "127.0.0.1", + family: "IPv4", + port: 6276, + }), + close: (cb?: () => void) => cb?.(), + }); + }, + }; + }); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + try { + const mod = await import("../../../../server/app-origin-controller.js"); + const c = mod.createAppOriginController({ + port: 6276, + host: "127.0.0.1", + }); + await c.start(); + expect(c.getOrigin()).toBe("http://127.0.0.1:6276"); + expect(c.publish({ html: "

x

" })).not.toBeNull(); + + emitter.emit( + "error", + Object.assign(new Error("boom"), { code: "EPIPE" }), + ); + + expect(c.getOrigin()).toBeNull(); + expect(c.publish({ html: "

x

" })).toBeNull(); + } finally { + errorSpy.mockRestore(); + vi.doUnmock("node:http"); + vi.resetModules(); + } + }); + it("gives up after one retry when the dynamic fallback also fails", async () => { // Guards the `retriedDynamic` latch. A real `listen(0)` effectively always // succeeds, so a claimed port only ever produces ONE EADDRINUSE — under diff --git a/clients/web/static/sandbox_proxy.html b/clients/web/static/sandbox_proxy.html index ddd676a2..99d27f73 100644 --- a/clients/web/static/sandbox_proxy.html +++ b/clients/web/static/sandbox_proxy.html @@ -254,6 +254,14 @@ // delivered as a real response header there (stronger than the // the srcdoc path relies on), so no wrapping applies here // — this proxy still never parses the untrusted bytes. + // + // Drop any `srcdoc` a previous render left behind first: it takes + // precedence over `src`, so a frame that had rendered an opaque + // document would keep showing it and never load this URL. Not + // reachable today (each render remounts the iframe, which reloads + // this document), but the cost of being wrong about that later is + // an app silently rendering the wrong bytes. + inner.removeAttribute("srcdoc"); innerOrigin = srcUrl.origin; inner.src = srcUrl.href; } else if (typeof html === "string") { @@ -265,6 +273,20 @@ // boundary). Known trade-off: a small set of libraries (e.g. // CesiumJS Map) don't initialise correctly under srcdoc — that // is accepted in exchange for the isolation guarantee. + // + // Restore the opaque-origin posture first. A frame that + // previously took the dedicated-origin path still carries + // `allow-same-origin`, and `innerOrigin` still names that origin; + // assigning `srcdoc` under those would put the untrusted document + // in THIS proxy's realm — the exact escape the double-iframe + // model exists to prevent. The sandbox attribute is read at + // navigation time and assigning `srcdoc` is the navigation, so + // stripping it here takes effect for the document being loaded. + const fallbackTokens = (inner.getAttribute("sandbox") || "") + .split(/\s+/) + .filter((t) => t && t.toLowerCase() !== "allow-same-origin"); + inner.setAttribute("sandbox", fallbackTokens.join(" ")); + innerOrigin = "null"; inner.srcdoc = html; } } else { diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 1541d646..6b2b237f 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -749,13 +749,24 @@ export function createRemoteApp( if (!publish) { return c.json({ error: "App-origin hosting is not available" }, 503); } - let body: { html?: unknown; csp?: unknown }; + let parsed: unknown; try { - body = (await c.req.json()) as { html?: unknown; csp?: unknown }; + parsed = await c.req.json(); } catch { return c.json({ error: "Invalid JSON body" }, 400); } - const { html, csp } = body; + // `null` and `[1,2]` are both valid JSON, so `c.req.json()` resolving is + // not proof there is an object to destructure — and destructuring `null` + // here would throw OUTSIDE the try above, turning a malformed body into a + // 500 instead of the 400 every other bad shape gets. + if ( + parsed === null || + typeof parsed !== "object" || + Array.isArray(parsed) + ) { + return c.json({ error: "Invalid JSON body" }, 400); + } + const { html, csp } = parsed as { html?: unknown; csp?: unknown }; if (typeof html !== "string" || html === "") { return c.json({ error: "Missing html" }, 400); }