diff --git a/clients/web/server/app-origin-controller.ts b/clients/web/server/app-origin-controller.ts index 5be55127..a933a320 100644 --- a/clients/web/server/app-origin-controller.ts +++ b/clients/web/server/app-origin-controller.ts @@ -334,8 +334,10 @@ export function createAppOriginController( * (#1862) when it is still safe to use, else the bind-derived one. * * Two reasons to refuse it here rather than only at config time: - * - After the EADDRINUSE fallback the reverse proxy still routes the - * public origin to the PINNED port, which someone else holds. + * - A reverse proxy routes the public origin to one stable port, so it + * is only usable on a FIXED port: not an explicit `0`, not a + * config-time collision resolved to `0`, and not the EADDRINUSE + * fallback, where someone else holds the pinned port. * - It must differ from every trusted ancestor — the sandbox proxy and * the Inspector page, i.e. `embedderOrigins`. The frame is granted * `allow-same-origin` on this path, so sharing either origin hands @@ -345,10 +347,10 @@ export function createAppOriginController( */ const adoptedOrigin = (derived: string): string => { if (!publicOrigin) return derived; - if (retriedDynamic) { + if (port === 0 || retriedDynamic) { console.warn( - `App origin: not using MCP_APP_ORIGIN_FULL_ADDRESS, since it routes to port ${port}, ` + - `which is taken; serving app documents from ${derived} instead.`, + `App origin: not using MCP_APP_ORIGIN_FULL_ADDRESS, since the listener is not on a fixed ` + + `port for it to route to; serving app documents from ${derived} instead.`, ); return derived; } diff --git a/clients/web/server/public-address.ts b/clients/web/server/public-address.ts index 001d0c60..704a073f 100644 --- a/clients/web/server/public-address.ts +++ b/clients/web/server/public-address.ts @@ -94,14 +94,15 @@ function parsePublicUrl(raw: string): Parsed { /** * The value as it may appear in a log line. Never the raw string: the values - * refused for carrying credentials or a query are exactly the ones likely to - * hold a secret, and echoing them would leak what the check refused. Userinfo, - * query and fragment are dropped; an unparseable value is not echoed at all. + * refused for carrying credentials, a query or a path are exactly the ones + * likely to hold a secret, and echoing them would leak what the check refused. + * Only scheme and host are shown — that is enough to identify which address was + * refused — and an unparseable value is not echoed at all. */ export function describeForLog(raw: string): string { try { const u = new URL(raw); - return `"${u.protocol}${u.host ? `//${u.host}` : ""}${u.pathname}"`; + return `"${u.protocol}${u.host ? `//${u.host}` : ""}"`; } catch { return "(unparseable value, not echoed)"; } diff --git a/clients/web/server/sandbox-controller.ts b/clients/web/server/sandbox-controller.ts index d9102574..d55c063b 100644 --- a/clients/web/server/sandbox-controller.ts +++ b/clients/web/server/sandbox-controller.ts @@ -341,17 +341,20 @@ export function createSandboxController( const derivedUrl = `http://${urlHost}:${actualPort}/sandbox`; // An operator-supplied public URL (#1862) replaces the derived one // only now that the listener it routes to is actually up — and only - // on the port it was configured for. After the EADDRINUSE fallback - // the proxy still routes the public URL to the PINNED port, which - // someone else holds, so advertising it would send the browser to - // the wrong listener. The derived URL at least works locally. - if (publicUrl && retriedDynamic) { + // when that listener is on a FIXED port. A reverse proxy routes the + // public URL to one stable port; an OS-assigned port (an explicit + // `0`, a config-time collision resolved to `0`, or the EADDRINUSE + // fallback, where someone else holds the pinned port) is not it, so + // advertising the public URL would send the browser to the wrong + // listener or none. The derived URL at least works locally. + const onFixedPort = port !== 0 && !retriedDynamic; + if (publicUrl && !onFixedPort) { console.warn( - `Sandbox: not advertising MCP_SANDBOX_FULL_ADDRESS, since it routes to port ${port}, ` + - `which is taken; advertising ${derivedUrl} instead.`, + `Sandbox: not advertising MCP_SANDBOX_FULL_ADDRESS, since the sandbox is not on a fixed ` + + `port for it to route to; advertising ${derivedUrl} instead.`, ); } - sandboxUrl = publicUrl && !retriedDynamic ? publicUrl : derivedUrl; + sandboxUrl = publicUrl && onFixedPort ? publicUrl : derivedUrl; boundPort = actualPort; settle({ port: actualPort, url: sandboxUrl }); }); 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 72cd41d5..8be38899 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 @@ -23,6 +23,18 @@ import { } from "../../../../server/app-origin-controller.js"; import { RUNNER_OAUTH_CALLBACK_DEFAULT_PORT } from "@inspector/core/auth/node/runner-oauth-callback.js"; +/** A port that was free a moment ago, for tests that need a FIXED port. */ +async function freePort(): Promise { + const probe: Server = createServer(); + await new Promise((resolve) => + probe.listen(0, "127.0.0.1", () => resolve()), + ); + const addr = probe.address(); + const port = typeof addr === "object" && addr !== null ? addr.port : 0; + await new Promise((resolve) => probe.close(() => resolve())); + return port; +} + describe("resolveAppOriginPort", () => { const saved = process.env.MCP_APP_ORIGIN_PORT; afterEach(() => { @@ -180,12 +192,15 @@ describe("createAppOriginController", () => { it("publishes under a public origin while still serving the bound listener (#1862)", async () => { const publicOrigin = "https://apps.example.com"; + // A fixed port: a public origin is only used where a proxy can route. + const port = await freePort(); controller = createAppOriginController({ - port: 0, + port, host: "127.0.0.1", publicOrigin, }); const first = await controller.start(); + expect(first.port).toBe(port); expect(first.url).toBe(publicOrigin); expect(controller.getOrigin()).toBe(publicOrigin); // The cached start reports the BOUND port; parsing one out of a public @@ -204,6 +219,27 @@ describe("createAppOriginController", () => { await expect(res.text()).resolves.toBe("

proxied

"); }); + it("does not use a public origin on an OS-assigned port (port 0)", async () => { + // Both an explicit MCP_APP_ORIGIN_PORT=0 and a config-time collision with + // CLIENT_PORT / MCP_SANDBOX_PORT (which buildWebServerConfig resolves to 0) + // land here: there is no stable port for a reverse proxy to route to. + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + controller = createAppOriginController({ + port: 0, + host: "127.0.0.1", + publicOrigin: "https://apps.example.com", + }); + const { url, port } = await controller.start(); + expect(url).toBe(`http://127.0.0.1:${port}`); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("not using MCP_APP_ORIGIN_FULL_ADDRESS"), + ); + } finally { + warnSpy.mockRestore(); + } + }); + it("does not adopt a public origin when the listener never bound", async () => { const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); try { @@ -423,7 +459,7 @@ describe("createAppOriginController", () => { const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); try { controller = createAppOriginController({ - port: 0, + port: await freePort(), host: "127.0.0.1", publicOrigin: "http://127.0.0.1:6275", embedderOrigins: appDocumentEmbedders("http://127.0.0.1:6275/sandbox", [ diff --git a/clients/web/src/test/integration/server/public-address.test.ts b/clients/web/src/test/integration/server/public-address.test.ts index cb06a283..efb88e6a 100644 --- a/clients/web/src/test/integration/server/public-address.test.ts +++ b/clients/web/src/test/integration/server/public-address.test.ts @@ -83,6 +83,18 @@ describe("public addresses (#1862)", () => { expect(warned()).not.toContain(secret); }); + it("never echoes a path, which can carry a secret too", () => { + expect( + resolveAppOriginPublicOrigin( + "https://apps.example.com/hunter2", + ["https://inspector.example.com"], + undefined, + ), + ).toBeUndefined(); + expect(warned()).toContain('"https://apps.example.com"'); + expect(warned()).not.toContain("hunter2"); + }); + it("refuses an origin shared with the Inspector UI — the same-origin collapse", () => { // The natural reverse-proxy layout: one hostname, /sandbox routed to 6275. // The spec requires host != sandbox origin, so this must not be advertised. diff --git a/clients/web/src/test/integration/server/sandbox-controller.test.ts b/clients/web/src/test/integration/server/sandbox-controller.test.ts index 0b5f0da1..c839bf3c 100644 --- a/clients/web/src/test/integration/server/sandbox-controller.test.ts +++ b/clients/web/src/test/integration/server/sandbox-controller.test.ts @@ -14,6 +14,18 @@ import { sandboxFrameAncestors, } from "../../../../server/sandbox-controller.js"; +/** A port that was free a moment ago, for tests that need a FIXED port. */ +async function freePort(): Promise { + const probe: Server = createServer(); + await new Promise((resolve) => + probe.listen(0, "127.0.0.1", () => resolve()), + ); + const addr = probe.address(); + const port = typeof addr === "object" && addr !== null ? addr.port : 0; + await new Promise((resolve) => probe.close(() => resolve())); + return port; +} + const PROXY_PAGE = join( dirname(fileURLToPath(import.meta.url)), "../../../../static/sandbox_proxy.html", @@ -276,8 +288,10 @@ describe("createSandboxController", () => { it("advertises a public URL in place of the bind-derived one (#1862)", async () => { const publicUrl = "https://sb.example.com/sandbox"; + // A fixed port: a public URL is only advertised where a proxy can route. + const port = await freePort(); const controller = createSandboxController({ - port: 0, + port, host: "127.0.0.1", publicUrl, }); @@ -286,7 +300,7 @@ describe("createSandboxController", () => { expect(first.url).toBe(publicUrl); expect(controller.getUrl()).toBe(publicUrl); // The port is still the bound one — the public URL has none of its own. - expect(first.port).toBeGreaterThan(0); + expect(first.port).toBe(port); expect( (await fetch(`http://127.0.0.1:${first.port}/sandbox`)).status, ).toBe(200); @@ -298,6 +312,26 @@ describe("createSandboxController", () => { } }); + it("does not advertise a public URL on an OS-assigned port (port 0)", async () => { + // MCP_SANDBOX_PORT=0 leaves no stable port for a reverse proxy to route to. + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const controller = createSandboxController({ + port: 0, + host: "127.0.0.1", + publicUrl: "https://sb.example.com/sandbox", + }); + try { + const { url, port } = await controller.start(); + expect(url).toBe(`http://127.0.0.1:${port}/sandbox`); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("not advertising MCP_SANDBOX_FULL_ADDRESS"), + ); + } finally { + warnSpy.mockRestore(); + await controller.close(); + } + }); + it("does not advertise a public URL when the listener never bound", async () => { const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); const controller = createSandboxController({ diff --git a/docs/environment-variables.md b/docs/environment-variables.md index b0801508..0c7cf6ce 100644 --- a/docs/environment-variables.md +++ b/docs/environment-variables.md @@ -41,7 +41,7 @@ The sandbox and app-origin servers advertise a URL built from their own bind (`h | `MCP_SANDBOX_FULL_ADDRESS` | web | `http://:/sandbox` | Public URL of the MCP Apps sandbox proxy, returned as `sandboxUrl` by `GET /api/config` and printed in the banner (e.g. `https://inspector-sandbox.example.com/sandbox`). A bare origin gets `/sandbox` appended. An empty value counts as unset. | | `MCP_APP_ORIGIN_FULL_ADDRESS` | web | `http://:` | Public origin that `_meta.ui.domain` app documents are published under (e.g. `https://inspector-apps.example.com`). **Origin only** — a path is refused, since documents are served at `/app-document/`. An empty value counts as unset. | -Both are **refused** — ignored with a warning, keeping the bind-derived address — when the value is not an absolute `http(s)` URL, carries credentials, a query string, a fragment or a wildcard, or is a bracketed IPv6 literal. ⚠️ **Each needs its own origin.** The MCP Apps spec requires the sandbox origin to differ from the Inspector's, so a sandbox address sharing an `ALLOWED_ORIGINS` origin is refused (`https://inspector.example.com/sandbox` behind the same hostname as the UI is the common case), as is an app origin equal to the Inspector's or the sandbox's. Neither is used if its pinned port was taken at startup and the server fell back to an OS-assigned one, since the proxy would route it to the wrong listener. A plain-`http` address while `ALLOWED_ORIGINS` lists an `https` origin is used, but warned about: the browser blocks it as mixed content. +Both are **refused** — ignored with a warning, keeping the bind-derived address — when the value is not an absolute `http(s)` URL, carries credentials, a query string, a fragment or a wildcard, or is a bracketed IPv6 literal. ⚠️ **Each needs its own origin.** The MCP Apps spec requires the sandbox origin to differ from the Inspector's, so a sandbox address sharing an `ALLOWED_ORIGINS` origin is refused (`https://inspector.example.com/sandbox` behind the same hostname as the UI is the common case), as is an app origin equal to the Inspector's or the sandbox's. Neither is used unless its listener is on a fixed port — not when the port is `0`, and not when the pinned port was taken at startup (or collided with another Inspector port) and the server fell back to an OS-assigned one — since the proxy has no stable port to route it to. A plain-`http` address while `ALLOWED_ORIGINS` lists an `https` origin is used, but warned about: the browser blocks it as mixed content. ## Behavior