diff --git a/AGENTS.md b/AGENTS.md index 0281b69a..2cad2e5d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -25,7 +25,7 @@ v2/main/ │ │ # HOST-DEPENDENT and the Inspector owns no domain │ │ # infrastructure, so it treats any non-empty value as a │ │ # REQUEST, not an address, and answers with a real loopback -│ │ # origin on its own port (MCP_APP_ORIGIN_PORT, default 6276). +│ │ # origin on its own port (MCP_APP_ORIGIN_PORT, default 6278). │ │ # ONE SHARED origin, path-keyed per document — a real, │ │ # allowlistable origin without a port or DNS name per app, │ │ # and therefore NOT a per-app isolation boundary (two such diff --git a/Dockerfile b/Dockerfile index 24455398..84f40d29 100644 --- a/Dockerfile +++ b/Dockerfile @@ -32,11 +32,14 @@ ENV HOST=0.0.0.0 \ DANGEROUSLY_BIND_ALL_INTERFACES=true \ CLIENT_PORT=6274 \ MCP_SANDBOX_PORT=6275 \ + MCP_APP_ORIGIN_PORT=6278 \ MCP_AUTO_OPEN_ENABLED=false -# 6275 is the MCP Apps sandbox, a second listener the browser reaches directly. -# It is only needed for the Apps tab, so it is EXPOSEd but publishing it is +# 6275 is the MCP Apps sandbox and 6278 the dedicated app origin (#2056) — two +# further listeners the browser reaches DIRECTLY, so neither works through the +# 6274 publish alone. Both are only needed for the Apps tab (6278 only for an +# App declaring `_meta.ui.domain`), so they are EXPOSEd but publishing them is # optional — see the Docker section of the root README. -EXPOSE 6274 6275 +EXPOSE 6274 6275 6278 # Run as the non-root `node` user the base image ships. The inspector resolves # its runtime-state dir (default catalog, OAuth token storage) from `HOME` diff --git a/README.md b/README.md index d9fce752..ce97d1cd 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://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.) +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:6278/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. @@ -477,10 +477,10 @@ docker run --rm -p 127.0.0.1:6274:6274 -p 127.0.0.1:6275:6275 \ ghcr.io/modelcontextprotocol/inspector ``` -**And `6276` if your app declares `_meta.ui.domain`.** That is the spec field a server uses to ask its host for a stable, dedicated origin — without one the app runs at an opaque origin and its requests carry `Origin: null`, which no CORS / OAuth-callback / API-key allowlist can admit. The Inspector answers the request with a real loopback origin on a third listener, `MCP_APP_ORIGIN_PORT` (default `6276`); apps that declare no `domain` never touch it. An app that declares one and can't reach it still renders — at an opaque origin, with a console warning. See [MCP App dedicated origins](./clients/web/README.md#mcp-app-dedicated-origins-metauidomain) for the host-specific contract and its isolation trade-offs. +**And `6278` if your app declares `_meta.ui.domain`.** That is the spec field a server uses to ask its host for a stable, dedicated origin — without one the app runs at an opaque origin and its requests carry `Origin: null`, which no CORS / OAuth-callback / API-key allowlist can admit. The Inspector answers the request with a real loopback origin on a third listener, `MCP_APP_ORIGIN_PORT` (default `6278`); apps that declare no `domain` never touch it. An app that declares one and can't reach it still renders — at an opaque origin, with a console warning. See [MCP App dedicated origins](./clients/web/README.md#mcp-app-dedicated-origins-metauidomain) for the host-specific contract and its isolation trade-offs. ```bash -docker run --rm -p 127.0.0.1:6274:6274 -p 127.0.0.1:6275:6275 -p 127.0.0.1:6276:6276 \ +docker run --rm -p 127.0.0.1:6274:6274 -p 127.0.0.1:6275:6275 -p 127.0.0.1:6278:6278 \ ghcr.io/modelcontextprotocol/inspector ``` diff --git a/clients/cli/__tests__/stored-auth.test.ts b/clients/cli/__tests__/stored-auth.test.ts index 4a45ec33..1b9b5768 100644 --- a/clients/cli/__tests__/stored-auth.test.ts +++ b/clients/cli/__tests__/stored-auth.test.ts @@ -554,6 +554,7 @@ describe("--print-handoff", () => { MCP_INSPECTOR_API_TOKEN: "tok123", CLIENT_PORT: "16274", MCP_SANDBOX_PORT: "16275", + MCP_APP_ORIGIN_PORT: "16278", MCP_STORAGE_DIR: "/tmp/inspector-storage", MCP_INSPECTOR_OAUTH_STATE_PATH: "", }, @@ -574,6 +575,9 @@ describe("--print-handoff", () => { expect(out.deepLink).toContain("transport=http"); expect(out.portForwardCmd).toContain("--tcp 16274:16274"); expect(out.portForwardCmd).toContain("--tcp 16275:16275"); + // The dedicated app origin (#2056) forwards with the other two — an app + // declaring `_meta.ui.domain` is unreachable without it. + expect(out.portForwardCmd).toContain("--tcp 16278:16278"); expect(out.oauthStatePath).toBe( join("/tmp/inspector-storage", "oauth.json"), ); diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index 6f2f894e..60e5a3d8 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -452,6 +452,11 @@ function buildHandoff( : canonicalUrlHost(host); const clientPort = process.env.CLIENT_PORT || "6274"; const sandboxPort = process.env.MCP_SANDBOX_PORT || "6275"; + // The dedicated app origin (#2056). Forwarded alongside the other two: an App + // whose UI resource declares `_meta.ui.domain` is served from this port and + // the browser reaches it DIRECTLY, so a handoff that forwards only 6274/6275 + // renders that app from an unreachable origin. + const appOriginPort = process.env.MCP_APP_ORIGIN_PORT || "6278"; // Treat an empty MCP_INSPECTOR_API_TOKEN the same as unset — an empty token // can't satisfy the deep-link autoConnect gate. const apiToken = process.env.MCP_INSPECTOR_API_TOKEN || undefined; @@ -469,7 +474,7 @@ function buildHandoff( return { serverUrl: normalizedUrl, deepLink: `http://${linkHost}:${clientPort}/?${params.toString()}`, - portForwardCmd: `coder port-forward --tcp ${clientPort}:${clientPort} --tcp ${sandboxPort}:${sandboxPort}`, + portForwardCmd: `coder port-forward --tcp ${clientPort}:${clientPort} --tcp ${sandboxPort}:${sandboxPort} --tcp ${appOriginPort}:${appOriginPort}`, oauthStatePath: statePath, apiToken: apiToken ?? null, note: diff --git a/clients/web/README.md b/clients/web/README.md index 5065ea17..63023953 100644 --- a/clients/web/README.md +++ b/clients/web/README.md @@ -155,8 +155,11 @@ Inspector owns no domain infrastructure — it cannot serve not parsed, matched, or reserved — declare whatever your production host expects. - The Inspector answers with a real HTTP origin on loopback: - `http://:`, default **`6276`** (beside the web - port `6274` and the sandbox port `6275`, so the three forward together). + `http://:`, default **`6278`** — in the same + `627x` family as the web port `6274` and the sandbox port `6275`, so the + three forward together. It is **not** `6276`: that is the fixed loopback + OAuth callback the CLI and TUI listen on, which OAuth apps pre-register and + which therefore cannot move. `6277` is skipped as v1's retired proxy port. - 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 @@ -186,7 +189,7 @@ default (opaque-origin) way and logs a console warning naming `_meta.ui.domain`. Losing the real origin degrades what the app can reach; losing the app itself would be worse. -**Forward `6276` too** if you need this off loopback (a container, a tunnel). +**Forward `6278` too** if you need this off loopback (a container, a tunnel). As with the sandbox port, a taken port falls back to an OS-assigned one with a loud warning — the app still renders, but the origin an app's backend was told to allowlist is then wrong, which is what the warning tells you. diff --git a/clients/web/server/app-origin-controller.ts b/clients/web/server/app-origin-controller.ts index a13bfe84..a9dc61e9 100644 --- a/clients/web/server/app-origin-controller.ts +++ b/clients/web/server/app-origin-controller.ts @@ -57,11 +57,22 @@ import { frameAncestorsDirective } from "./sandbox-controller.js"; /** * The default app-origin listen port — **fixed**, for the same reason * `DEFAULT_SANDBOX_PORT` is: a port that changes per run can never be named in - * a dev container's `forwardPorts`, a `docker run -p`, or an SSH tunnel. `6276` - * sits next to the web (`6274`) and sandbox (`6275`) ports so the three forward - * together. + * a dev container's `forwardPorts`, a `docker run -p`, or an SSH tunnel. + * + * **Not 6276, and not 6277.** `6276` is already taken, by the fixed loopback + * OAuth callback the CLI and TUI listen on + * (`RUNNER_OAUTH_CALLBACK_DEFAULT_PORT`) — fixed precisely so an OAuth app can + * pre-register `http://127.0.0.1:6276/oauth/callback`, which means it is the + * one port here that CANNOT move when something else takes it first. Binding it + * would let a running `--web` (started at login, holding the port) break a + * later `--cli`/`--tui` OAuth flow at its registered redirect URI — and the + * handoff recipe in `docs/mcp-app-review.md` has those two running side by + * side. Of the two listeners this listener is the one with a documented + * dynamic fallback, so it is the one that yields. `6277` is skipped because it + * was v1's MCP proxy port, which the migration guide tells people to stop + * forwarding; reusing it would make that instruction wrong. */ -export const DEFAULT_APP_ORIGIN_PORT = 6276; +export const DEFAULT_APP_ORIGIN_PORT = 6278; /** Documents kept at once. Oldest is evicted past this; see {@link publish}. */ const MAX_DOCUMENTS = 32; @@ -142,7 +153,7 @@ export interface AppOriginController { /** * A usable listen port from a raw env value, or `undefined` if it isn't a plain * integer in `0`–`65535`. Mirrors `sandbox-controller`'s parser: `^\d+$` rejects - * `parseInt`'s partial parses (`6276abc`) and the bound keeps an out-of-range + * `parseInt`'s partial parses (`6278abc`) and the bound keeps an out-of-range * value from reaching `server.listen`, which throws synchronously. */ function parseListenPort(raw: string | undefined): number | undefined { diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index b7f7a11f..0d0f7007 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -3489,7 +3489,7 @@ describe("App MCP App listed-resource metadata wiring (#2055)", () => { // 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", + "http://127.0.0.1:6278/app-document/deadbeef", ); renderWithMantine(); @@ -3510,7 +3510,7 @@ describe("App MCP App listed-resource metadata wiring (#2055)", () => { html: "

app

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

app

", csp: "default-src 'none'" }, expect.objectContaining({ baseUrl: expect.any(String) }), diff --git a/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.test.ts b/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.test.ts index 742fabf1..55d624dc 100644 --- a/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.test.ts +++ b/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.test.ts @@ -498,7 +498,7 @@ describe("createAppBridgeFactory", () => { ); const publishAppDocument = vi .fn<(doc: { html: string; csp?: string }) => Promise>() - .mockResolvedValue("http://localhost:6276/app-document/abc"); + .mockResolvedValue("http://localhost:6278/app-document/abc"); const factory = createAppBridgeFactory({ getClient: () => fakeClient, readResource, @@ -517,7 +517,7 @@ describe("createAppBridgeFactory", () => { const call = bridgeInstances[0].sendSandboxResourceReady.mock .calls[0][0] as { html: string; src?: string }; - expect(call.src).toBe("http://localhost:6276/app-document/abc"); + expect(call.src).toBe("http://localhost:6278/app-document/abc"); // `html` still rides along: a proxy that ignores `src` renders the app // rather than a blank frame. expect(call.html).toBe(published.html); @@ -530,7 +530,7 @@ describe("createAppBridgeFactory", () => { ])("does not take the dedicated path for %s", async (_label, domain) => { const publishAppDocument = vi .fn<(doc: { html: string; csp?: string }) => Promise>() - .mockResolvedValue("http://localhost:6276/app-document/abc"); + .mockResolvedValue("http://localhost:6278/app-document/abc"); const factory = createAppBridgeFactory({ getClient: () => fakeClient, readResource: vi @@ -550,7 +550,7 @@ describe("createAppBridgeFactory", () => { it("never publishes a resource that declares no domain", async () => { const publishAppDocument = vi .fn<(doc: { html: string; csp?: string }) => Promise>() - .mockResolvedValue("http://localhost:6276/app-document/abc"); + .mockResolvedValue("http://localhost:6278/app-document/abc"); const factory = createAppBridgeFactory({ getClient: () => fakeClient, readResource: vi.fn().mockResolvedValue(uiResource("

x

")), diff --git a/clients/web/src/lib/publishAppDocument.test.ts b/clients/web/src/lib/publishAppDocument.test.ts index 1f435683..f9fd0158 100644 --- a/clients/web/src/lib/publishAppDocument.test.ts +++ b/clients/web/src/lib/publishAppDocument.test.ts @@ -27,13 +27,13 @@ describe("publishAppDocument", () => { const fetchFn = vi .fn() .mockResolvedValue( - jsonResponse({ url: "http://localhost:6276/app-document/abc" }), + jsonResponse({ url: "http://localhost:6278/app-document/abc" }), ); const url = await publishAppDocument( { html: "

app

", csp: "default-src 'none'" }, { baseUrl: "http://localhost:6274", authToken: "tok", fetchFn }, ); - expect(url).toBe("http://localhost:6276/app-document/abc"); + expect(url).toBe("http://localhost:6278/app-document/abc"); expect(fetchFn).toHaveBeenCalledWith( "http://localhost:6274/api/app-document", expect.objectContaining({ 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 d60a41c5..ecd556a7 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 @@ -37,13 +37,13 @@ describe("createRemoteApp POST /api/app-document", () => { it("publishes the document and returns the URL", async () => { const publish = vi .fn<(doc: { html: string; csp?: string }) => { url: string } | null>() - .mockReturnValue({ url: "http://localhost:6276/app-document/abc" }); + .mockReturnValue({ url: "http://localhost:6278/app-document/abc" }); const res = await appWith(publish).request( post({ html: "

hi

", csp: "default-src 'none'" }), ); expect(res.status).toBe(200); await expect(res.json()).resolves.toEqual({ - url: "http://localhost:6276/app-document/abc", + url: "http://localhost:6278/app-document/abc", }); expect(publish).toHaveBeenCalledWith({ html: "

hi

", @@ -54,7 +54,7 @@ describe("createRemoteApp POST /api/app-document", () => { it("accepts a document with no csp", async () => { const publish = vi .fn<(doc: { html: string; csp?: string }) => { url: string } | null>() - .mockReturnValue({ url: "http://localhost:6276/app-document/abc" }); + .mockReturnValue({ url: "http://localhost:6278/app-document/abc" }); const res = await appWith(publish).request(post({ html: "

hi

" })); expect(res.status).toBe(200); expect(publish).toHaveBeenCalledWith({ @@ -112,6 +112,46 @@ describe("createRemoteApp POST /api/app-document", () => { expect(publish).not.toHaveBeenCalled(); }); + it.each([ + ["a CR/LF-carrying csp", "default-src 'none'\r\nX-Evil: 1"], + ["a NUL-carrying csp", "default-src 'none'\u0000"], + ])("rejects %s with 400", async (_label, csp) => { + // The controller hands `csp` to `res.writeHead()` verbatim. Node throws + // ERR_INVALID_CHAR there SYNCHRONOUSLY, inside the handler for a later, + // unrelated request — so accepting one here turns a bad publish into a + // thrown listener rather than a 400 anyone can see. + const publish = vi + .fn<(doc: { html: string; csp?: string }) => { url: string } | null>() + .mockReturnValue({ url: "http://x/y" }); + const res = await appWith(publish).request(post({ html: "

x

", csp })); + expect(res.status).toBe(400); + expect(publish).not.toHaveBeenCalled(); + }); + + it("rejects an oversized csp with 413 — it is retained per document", async () => { + const publish = vi + .fn<(doc: { html: string; csp?: string }) => { url: string } | null>() + .mockReturnValue({ url: "http://x/y" }); + const res = await appWith(publish).request( + post({ html: "

x

", csp: "a".repeat(8 * 1024 + 1) }), + ); + expect(res.status).toBe(413); + expect(publish).not.toHaveBeenCalled(); + }); + + it("rejects an oversized BODY before parsing it", async () => { + // Distinct from the document check below, which only runs once the whole + // request has been buffered and parsed — and only bounds `html`. + const publish = vi + .fn<(doc: { html: string; csp?: string }) => { url: string } | null>() + .mockReturnValue({ url: "http://x/y" }); + const res = await appWith(publish).request( + post(`{"junk":"${"x".repeat(24 * 1024 * 1024 + 1024)}"}`), + ); + expect(res.status).toBe(413); + expect(publish).not.toHaveBeenCalled(); + }); + it("rejects an oversized document with 413 rather than pinning it in memory", async () => { const publish = vi .fn<(doc: { html: string; csp?: string }) => { url: string } | null>() 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 1126b630..7e9e66fa 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 @@ -21,6 +21,7 @@ import { resolveAppOriginPort, appDocumentEmbedders, } from "../../../../server/app-origin-controller.js"; +import { RUNNER_OAUTH_CALLBACK_DEFAULT_PORT } from "@inspector/core/auth/node/runner-oauth-callback.js"; describe("resolveAppOriginPort", () => { const saved = process.env.MCP_APP_ORIGIN_PORT; @@ -34,6 +35,19 @@ describe("resolveAppOriginPort", () => { expect(resolveAppOriginPort()).toBe(DEFAULT_APP_ORIGIN_PORT); }); + it("does not default to the runners' fixed OAuth callback port", () => { + // Both listeners are fixed-by-default and both live in the 627x family, so + // the next free number is the obvious pick for each — which is how they + // collided. This one can move (it warns and retries on a dynamic port); + // the OAuth callback cannot, because apps pre-register + // `http://127.0.0.1:/oauth/callback`. A `--web` holding the + // port would therefore break a later `--cli`/`--tui` OAuth flow, and the + // handoff recipe runs those side by side. + expect(DEFAULT_APP_ORIGIN_PORT).not.toBe( + RUNNER_OAUTH_CALLBACK_DEFAULT_PORT, + ); + }); + it("honors a valid MCP_APP_ORIGIN_PORT, including an explicit 0", () => { process.env.MCP_APP_ORIGIN_PORT = "9200"; expect(resolveAppOriginPort()).toBe(9200); @@ -41,7 +55,7 @@ describe("resolveAppOriginPort", () => { expect(resolveAppOriginPort()).toBe(0); }); - it.each(["6276abc", "70000", "-1", "nope", " "])( + it.each(["6278abc", "70000", "-1", "nope", " "])( "warns and falls back for an invalid value %j", (value) => { const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); @@ -357,7 +371,7 @@ describe("createAppOriginController", () => { address: () => ({ address: "127.0.0.1", family: "IPv4", - port: 6276, + port: 6278, }), close: (cb?: () => void) => cb?.(), }); @@ -368,11 +382,11 @@ describe("createAppOriginController", () => { try { const mod = await import("../../../../server/app-origin-controller.js"); const c = mod.createAppOriginController({ - port: 6276, + port: 6278, host: "127.0.0.1", }); await c.start(); - expect(c.getOrigin()).toBe("http://127.0.0.1:6276"); + expect(c.getOrigin()).toBe("http://127.0.0.1:6278"); expect(c.publish({ html: "

x

" })).not.toBeNull(); emitter.emit( diff --git a/clients/web/src/test/integration/server/web-server-config.test.ts b/clients/web/src/test/integration/server/web-server-config.test.ts index dec8406e..ce345d4a 100644 --- a/clients/web/src/test/integration/server/web-server-config.test.ts +++ b/clients/web/src/test/integration/server/web-server-config.test.ts @@ -303,13 +303,13 @@ describe("buildWebServerConfigFromEnv", () => { expect(buildWebServerConfigFromEnv().sandboxPort).toBe(9100); }); - it("resolves appOriginPort from MCP_APP_ORIGIN_PORT, defaulting to 6276", () => { + it("resolves appOriginPort from MCP_APP_ORIGIN_PORT, defaulting to 6278", () => { // The dedicated app origin (#2056). Deliberately NOT sharing the sandbox's // SERVER_PORT fallback: it is a different listener and must be pinnable // (and forwardable) on its own. delete process.env.MCP_APP_ORIGIN_PORT; process.env.SERVER_PORT = "9100"; - expect(buildWebServerConfigFromEnv().appOriginPort).toBe(6276); + expect(buildWebServerConfigFromEnv().appOriginPort).toBe(6278); process.env.MCP_APP_ORIGIN_PORT = "9300"; expect(buildWebServerConfigFromEnv().appOriginPort).toBe(9300); }); diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 6b2b237f..75d8d9c4 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -24,6 +24,7 @@ import type { LogEvent } from "pino"; import { Hono } from "hono"; import type { Context, Env, Next } from "hono"; import { streamSSE } from "hono/streaming"; +import { bodyLimit } from "hono/body-limit"; import { watch as chokidarWatch, type FSWatcher } from "chokidar"; import { createTransportNode } from "../../node/transport.js"; import { createProxyFetch } from "../../node/proxyFetch.js"; @@ -306,6 +307,41 @@ export interface RemoteServerOptions { */ const MAX_APP_DOCUMENT_CHARS = 8 * 1024 * 1024; +/** + * Upper bound on the request BODY, in bytes, enforced by `bodyLimit` *before* + * anything is parsed. {@link MAX_APP_DOCUMENT_CHARS} alone is not enough: it is + * checked after `c.req.json()` has already buffered and parsed the whole + * request, so an unbounded body — or one whose bulk sits in `csp` or in keys + * the route never reads — is fully materialized in memory before being + * rejected. Generous relative to the document bound because JSON escaping + * inflates the payload (`<` stays one byte, but a `"` or a newline becomes + * two, and non-BMP text more), so this must not reject a document the char + * check would accept. + */ +const MAX_APP_DOCUMENT_BODY_BYTES = 24 * 1024 * 1024; + +/** + * Upper bound on the per-app CSP policy string. Unlike the document, this is + * retained for the life of the published entry, and a real policy is a few + * hundred characters — an app pushing more is not describing sources. + */ +const MAX_APP_CSP_CHARS = 8 * 1024; + +/** + * Whether a string is safe to emit as an HTTP header VALUE. + * + * The published `csp` is handed to `res.writeHead()` verbatim by the app-origin + * controller. Node validates header values there and throws + * `ERR_INVALID_CHAR` **synchronously inside the request handler** — outside + * this route's error handling, on a request this route is not even part of — + * so a CR/LF or NUL accepted here surfaces later as a thrown listener rather + * than a 400. Restrict to tab plus printable ASCII, which is stricter than RFC + * 9110 field-value grammar and is all a CSP policy needs. + */ +function isHeaderSafeValue(value: string): boolean { + return !/[^\t\x20-\x7e]/.test(value); +} + export interface CreateRemoteAppResult { /** The Hono app */ app: Hono; @@ -744,44 +780,61 @@ export function createRemoteApp( * listener on its own port, which is what makes the app's origin real * instead of opaque. */ - app.post("/api/app-document", async (c) => { - const publish = options.publishAppDocument; - if (!publish) { - return c.json({ error: "App-origin hosting is not available" }, 503); - } - let parsed: unknown; - try { - parsed = await c.req.json(); - } catch { - return c.json({ error: "Invalid JSON body" }, 400); - } - // `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); - } - if (csp !== undefined && typeof csp !== "string") { - return c.json({ error: "csp must be a string" }, 400); - } - if (html.length > MAX_APP_DOCUMENT_CHARS) { - return c.json({ error: "Document too large" }, 413); - } - const published = publish({ html, csp }); - if (!published) { - return c.json({ error: "App-origin hosting is not available" }, 503); - } - return c.json(published); - }); + app.post( + "/api/app-document", + // Before the parse, not after: see MAX_APP_DOCUMENT_BODY_BYTES. + bodyLimit({ + maxSize: MAX_APP_DOCUMENT_BODY_BYTES, + onError: (c) => c.json({ error: "Document too large" }, 413), + }), + async (c) => { + const publish = options.publishAppDocument; + if (!publish) { + return c.json({ error: "App-origin hosting is not available" }, 503); + } + let parsed: unknown; + try { + parsed = await c.req.json(); + } catch { + return c.json({ error: "Invalid JSON body" }, 400); + } + // `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); + } + if (csp !== undefined && typeof csp !== "string") { + return c.json({ error: "csp must be a string" }, 400); + } + if (csp !== undefined && !isHeaderSafeValue(csp)) { + // Rejected here rather than sanitized: a policy carrying a control + // character is not a policy with a stray byte in it, it is a caller + // doing something this route has no honest interpretation of. + return c.json({ error: "csp is not a valid header value" }, 400); + } + if (html.length > MAX_APP_DOCUMENT_CHARS) { + return c.json({ error: "Document too large" }, 413); + } + if (csp !== undefined && csp.length > MAX_APP_CSP_CHARS) { + return c.json({ error: "csp too large" }, 413); + } + const published = publish({ html, csp }); + if (!published) { + return c.json({ error: "App-origin hosting is not available" }, 503); + } + return c.json(published); + }, + ); app.post("/api/mcp/connect", async (c) => { let body: RemoteConnectRequest; diff --git a/docs/mcp-app-review.md b/docs/mcp-app-review.md index 7a0acbe0..593e7b17 100644 --- a/docs/mcp-app-review.md +++ b/docs/mcp-app-review.md @@ -159,11 +159,11 @@ launching. ## 6. Reaching the web inspector remotely (SSH port-forward) When the backend runs on another host and you want to complete OAuth or -visually inspect a rendered App from your local browser, forward **both** +visually inspect a rendered App from your local browser, forward **all three** ports: ```bash -ssh -L 6274:127.0.0.1:6274 -L 6275:127.0.0.1:6275 +ssh -L 6274:127.0.0.1:6274 -L 6275:127.0.0.1:6275 -L 6278:127.0.0.1:6278 ``` Then open `http://127.0.0.1:6274/?MCP_INSPECTOR_API_TOKEN=$TOKEN` locally. @@ -178,6 +178,11 @@ Then open `http://127.0.0.1:6274/?MCP_INSPECTOR_API_TOKEN=$TOKEN` locally. - Forward `:6275` (`MCP_SANDBOX_PORT`) as well: the Apps tab renders widgets in an iframe served from that second origin; without it the App frame stays blank. +- Forward `:6278` (`MCP_APP_ORIGIN_PORT`) too, for an App whose UI resource + declares `_meta.ui.domain` (#2056). That App is served from a *third* origin + the browser loads directly, so without the forward its frame fails to load — + and the backend cannot detect that, since publishing succeeded on its side. + An App that declares no `domain` is unaffected. - Forward the **same** local port number you bind on the remote — the browser sends `Origin: http://127.0.0.1:` and the backend compares it to its own port.