diff --git a/clients/web/README.md b/clients/web/README.md index 63023953..bbbe0835 100644 --- a/clients/web/README.md +++ b/clients/web/README.md @@ -189,6 +189,21 @@ 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. +**One failure is outside that guarantee, deliberately: a published document the +browser cannot reach.** Every case above is one the *host* observes — publishing +returned nothing, so it falls back before choosing a render path. If publishing +succeeds and the browser then cannot load the URL (the port is not forwarded, or +a collision moved the listener to a dynamic port that is not), the frame stays +blank instead. There is no honest signal to fall back on: the navigation is +cross-origin, so `onerror` never fires for an HTTP error, `onload` fires for the +error page too, and nothing about the document is readable. The alternatives are +a timeout — which races an app that is merely slow, and "recovers" it by +re-running it at an opaque origin, executing its side effects twice — or a +reachability probe on every render, which proves the port answers rather than +that this fetch will. So the cause is removed instead of guessed at: every +documented remote workflow forwards the port (see below, the Docker section of +the root README, and the SSH recipe in `docs/mcp-app-review.md`). + **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 diff --git a/clients/web/server/app-origin-controller.ts b/clients/web/server/app-origin-controller.ts index a9dc61e9..6fe43689 100644 --- a/clients/web/server/app-origin-controller.ts +++ b/clients/web/server/app-origin-controller.ts @@ -298,12 +298,15 @@ export function createAppOriginController( server.listen(port, host, () => { const addr = server!.address(); - const actualPort = - typeof addr === "object" && addr !== null && "port" in addr - ? addr.port - : /* v8 ignore next -- unreachable for a TCP listen; `address()` - returns AddressInfo. */ - (addr as unknown as number); + /* v8 ignore next 4 -- unreachable for a TCP listen: inside the + `listening` callback `address()` returns an AddressInfo. The guard + is a narrowing for the string (pipe/socket) and null members of the + return type, not a case this listener can produce. */ + if (typeof addr !== "object" || addr === null) { + settle({ port: 0, url: "" }); + return; + } + const actualPort = addr.port; // A wildcard bind isn't reachable as `http://0.0.0.0:PORT`, but it // does serve loopback — advertise `localhost` there, and otherwise // the same canonical host the origin allow-list emits. diff --git a/clients/web/server/web-server-config.ts b/clients/web/server/web-server-config.ts index cd179df7..5c0b23e1 100644 --- a/clients/web/server/web-server-config.ts +++ b/clients/web/server/web-server-config.ts @@ -377,7 +377,35 @@ export function buildWebServerConfig( ""); const sandboxPort = resolveSandboxPort(); - const appOriginPort = resolveAppOriginPort(); + // The app-origin listener starts BEFORE the web server binds (both in prod + // `startHonoServer` and in the Vite plugin), so a configured collision is not + // a harmless race — the app origin wins the port and the Inspector itself + // then dies with EADDRINUSE. `CLIENT_PORT=6278` is now such a config, and it + // was perfectly valid before this feature existed. + // + // Resolved to a dynamic port rather than by refusing to boot: of the three + // listeners this is the only one that can move (the sandbox URL and the web + // port are both advertised to the browser as fixed), and its own EADDRINUSE + // path already degrades this way. Losing a predictable app origin costs an + // app declaring `_meta.ui.domain` its forwarded port; refusing to boot costs + // the user the whole Inspector over a feature they may not be using. + const requestedAppOriginPort = resolveAppOriginPort(); + const appOriginCollidesWith = + requestedAppOriginPort !== 0 && + (requestedAppOriginPort === port + ? "the web server (CLIENT_PORT)" + : requestedAppOriginPort === sandboxPort + ? "the MCP Apps sandbox (MCP_SANDBOX_PORT)" + : null); + if (appOriginCollidesWith) { + console.warn( + `App origin: port ${requestedAppOriginPort} is already ${appOriginCollidesWith}; ` + + `using an OS-assigned port instead. Apps declaring _meta.ui.domain will still ` + + `render, but the origin they are served from is no longer predictable — set ` + + `MCP_APP_ORIGIN_PORT to a free one if your app's backend allowlists it.`, + ); + } + const appOriginPort = appOriginCollidesWith ? 0 : requestedAppOriginPort; let logger: Logger | undefined; if (process.env.MCP_LOG_FILE) { 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 ce345d4a..51a56993 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 @@ -314,6 +314,42 @@ describe("buildWebServerConfigFromEnv", () => { expect(buildWebServerConfigFromEnv().appOriginPort).toBe(9300); }); + it.each([ + ["the web server", "CLIENT_PORT", "the web server (CLIENT_PORT)"], + [ + "the sandbox", + "MCP_SANDBOX_PORT", + "the MCP Apps sandbox (MCP_SANDBOX_PORT)", + ], + ])( + "yields a dynamic app-origin port when it collides with %s", + (_label, envVar, expectedReason) => { + // The app-origin listener starts BEFORE the web server binds, so a + // configured collision is not a race it loses — it WINS the port and the + // Inspector then dies with EADDRINUSE. `CLIENT_PORT=6278` was a valid + // config before this feature existed. + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + process.env[envVar] = "6500"; + process.env.MCP_APP_ORIGIN_PORT = "6500"; + expect(buildWebServerConfigFromEnv().appOriginPort).toBe(0); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining(expectedReason), + ); + } finally { + warnSpy.mockRestore(); + } + }, + ); + + it("leaves an explicit dynamic app-origin port alone", () => { + // 0 means "OS-assigned" for both, so two zeros are not a collision. + process.env.CLIENT_PORT = "6274"; + process.env.MCP_SANDBOX_PORT = "0"; + process.env.MCP_APP_ORIGIN_PORT = "0"; + expect(buildWebServerConfigFromEnv().appOriginPort).toBe(0); + }); + it("sets MCP_STORAGE_DIR when present", () => { process.env.MCP_STORAGE_DIR = "/tmp/storage"; expect(buildWebServerConfigFromEnv().storageDir).toBe("/tmp/storage"); diff --git a/clients/web/static/sandbox_proxy.html b/clients/web/static/sandbox_proxy.html index 99d27f73..5777c474 100644 --- a/clients/web/static/sandbox_proxy.html +++ b/clients/web/static/sandbox_proxy.html @@ -197,14 +197,24 @@ console.warn("[Sandbox] ignoring unparseable src:", src); } } - // The app origin MUST differ from this proxy's own — otherwise - // granting allow-same-origin below would put the untrusted app in - // this realm, exactly the escape the double-iframe model exists to - // prevent. Refuse rather than degrade quietly: this is a host - // misconfiguration, not app input. - if (srcUrl && srcUrl.origin === window.location.origin) { + // The app origin MUST differ from BOTH trusted origins — this + // proxy's own and the Inspector page above it. Granting + // allow-same-origin below is what makes the app's origin real, and + // an app sharing the proxy's origin would land in this realm + // outright; one sharing the HOST's origin is no better, since + // same-origin with the top document means direct access to + // `window.top`, its DOM and its storage — the cross-origin frame in + // between buys nothing once the inner document is same-origin with + // the thing it is trying to reach. Either is a host + // misconfiguration, not app input, so refuse rather than degrade + // quietly. + if ( + srcUrl && + (srcUrl.origin === window.location.origin || + srcUrl.origin === EXPECTED_HOST_ORIGIN) + ) { console.error( - "[Sandbox] refusing a src on the proxy's own origin:", + "[Sandbox] refusing a src on a trusted origin:", srcUrl.origin, ); srcUrl = null; diff --git a/docs/v1-to-v2-migration.md b/docs/v1-to-v2-migration.md index c241e9df..19727a67 100644 --- a/docs/v1-to-v2-migration.md +++ b/docs/v1-to-v2-migration.md @@ -281,6 +281,7 @@ Stdout is otherwise compatible: the default `text` format still pretty-prints th | — | `DANGEROUSLY_BIND_ALL_INTERFACES` | New opt-in for a wildcard bind (the Docker image sets it) | | — | `MCP_CATALOG_PATH` | Default catalog path | | — | `MCP_SANDBOX_PORT` | MCP Apps sandbox server port (fixed `6275` by default) | +| — | `MCP_APP_ORIGIN_PORT` | Dedicated app-origin server port (fixed `6278` by default), used only by an MCP App whose UI resource declares `_meta.ui.domain` | | — | `MCP_STORAGE_DIR`, `MCP_INSPECTOR_OAUTH_STATE_PATH`, `MCP_CLIENT_CONFIG_PATH` | Storage and OAuth state locations | | — | `MCP_OAUTH_CALLBACK_URL` | CLI/TUI OAuth loopback callback | @@ -339,6 +340,15 @@ Notes: Publish it on the **same port number** inside and out: the sandbox URL is handed to the browser via `/api/config` as `http://localhost:/sandbox`, so remapping it advertises a port the browser can't reach. To use a different number, move both ends — `-e MCP_SANDBOX_PORT=6280 -p 127.0.0.1:6280:6280`. +- **And a third port for an App that declares `_meta.ui.domain`.** That spec field asks the host for a stable, dedicated origin, which v2 answers with a further listener on `MCP_APP_ORIGIN_PORT` (fixed `6278` by default) that the browser also reaches directly. Apps declaring no `domain` never touch it, so publish it only if you need one: + + ```bash + 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:latest + ``` + + Same-number rule and same remap escape hatch (`-e MCP_APP_ORIGIN_PORT=6281 -p 127.0.0.1:6281:6281`). Unpublished, such an app still renders — at an opaque origin, with a console warning — so this one degrades rather than blanking. Nothing in v1 corresponds to it. + - The v2 image sets `DANGEROUSLY_BIND_ALL_INTERFACES=true` internally (a container must bind `0.0.0.0` to be reachable through `-p`). Setting a bare `HOST=0.0.0.0` **outside** a container now exits with an error. - **If you remap the published port** (`-p 8080:6274`), the browser's origin no longer matches the in-container port, so set `ALLOWED_ORIGINS=http://localhost:8080,http://127.0.0.1:8080` (or run `-e CLIENT_PORT=8080 -p 8080:8080`) or connects will 403. - The image runs as non-root and has a `HEALTHCHECK` that assumes `--web`; add `--no-healthcheck` when running `--cli` / `--tui`. @@ -358,6 +368,8 @@ Notes: **"The Apps tab is blank in Docker / over SSH."** The MCP Apps sandbox is a second port (`6275` by default). Publish/forward it too — see [MCP Apps caveats](../clients/web/README.md#hosting-on-a-network). +**"One particular App renders, but its requests are rejected by its own backend."** That App declares `_meta.ui.domain` and is being served from the opaque-origin fallback, so its requests carry `Origin: null`. It needs the **third** port (`MCP_APP_ORIGIN_PORT`, `6278` by default) published or forwarded as well; the console carries a warning naming the field. See [MCP App dedicated origins](../clients/web/README.md#mcp-app-dedicated-origins-metauidomain). + **"The page never loads inside a VS Code dev container."** Fixed in the release carrying #1951 — upgrade; on an older version, work around it with `HOST=127.0.0.1`. Before that fix the default `HOST=localhost` bound IPv6 loopback **only** on Linux, so anything connecting over IPv4 `127.0.0.1` was refused — which is what a dev container's port forwarder does. The terminal printed a normal banner while the browser spun forever; the failure is a **hang** rather than a connection error because the forwarder accepts on the host side before failing to connect inward. The same fix covers an `ssh -L 6274:127.0.0.1:6274` tunnel and a container healthcheck. It does **not** describe the official Docker image (it binds `0.0.0.0`, so it was never affected) or a plain `docker run -p` against a loopback bind, which is unreachable across the network namespace for an unrelated reason. **"`HOST=0.0.0.0` exits with an error."** That's the wildcard-bind guard. Bind a specific address instead, or set `DANGEROUSLY_BIND_ALL_INTERFACES=true` if you genuinely need all interfaces. See [Host binding & the origin allow-list](../clients/web/README.md#host-binding--the-origin-allow-list).