fix(web): guard the app-origin port and the trusted-origin grant
Copilot review round 3 on #2100. - MCP_APP_ORIGIN_PORT could equal CLIENT_PORT or MCP_SANDBOX_PORT. The app-origin listener starts BEFORE the web server binds, so that is not a race it loses — it wins the port and the Inspector dies with EADDRINUSE. `CLIENT_PORT=6278` was a valid config before this feature existed. It now resolves to a dynamic port with a warning: of the three listeners this is the only one that can move, and refusing to boot would cost the user the whole Inspector over a feature they may not be using. - The proxy's same-origin refusal covered only its own origin. An app served from the INSPECTOR's origin is no safer: same-origin with the top document means direct access to window.top, its DOM and its storage, and the cross-origin frame in between buys nothing. Both trusted origins are now refused before allow-same-origin is granted. - Replaced the `as unknown as number` on `server.address()` with a real narrowing of the string/null members of its return type. - MCP_APP_ORIGIN_PORT added to the v1→v2 migration guide's env table, its Docker recipe, and a troubleshooting entry for the symptom this actually produces (the app renders, but its own backend rejects `Origin: null`). - clients/web/README.md now states which failure is outside the fallback guarantee — a published document the browser cannot reach — and why there is no honest signal to fall back on. Docs and code no longer disagree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
GitHub
parent
80f76e365e
commit
bb3ba91b8a
6 changed files with 118 additions and 14 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
|
|
@ -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");
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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:<container port>/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).
|
||||
|
|
|
|||
Loading…
Reference in a new issue