From 4dc5eaa6bb4fa6f58703eec7aff63d5fa67c4188 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 14 Sep 2026 00:37:50 -0400 Subject: [PATCH] fix(web): address Copilot review round 1 on public addresses (#1862) - Never echo a refused value into a warning: credentials, query and fragment are dropped, and an unparseable value is not shown at all. - Do not adopt MCP_SANDBOX_FULL_ADDRESS / MCP_APP_ORIGIN_FULL_ADDRESS after the EADDRINUSE fallback: the reverse proxy still routes them to the pinned port, which another process holds. - Refuse a public app origin equal to any trusted ancestor in the controller, where the real (possibly bind-derived, possibly fallback-port) sandbox origin is known; config time could not see it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HzuzRPJv7M8wGe3wyw5sSg Signed-off-by: cliffhall --- clients/web/server/app-origin-controller.ts | 39 +++++++++++++-- clients/web/server/public-address.ts | 24 ++++++++-- clients/web/server/sandbox-controller.ts | 15 +++++- .../server/app-origin-controller.test.ts | 48 +++++++++++++++++++ .../integration/server/public-address.test.ts | 11 +++++ .../server/sandbox-controller.test.ts | 25 ++++++++++ 6 files changed, 154 insertions(+), 8 deletions(-) diff --git a/clients/web/server/app-origin-controller.ts b/clients/web/server/app-origin-controller.ts index 8329b5aa..5be55127 100644 --- a/clients/web/server/app-origin-controller.ts +++ b/clients/web/server/app-origin-controller.ts @@ -328,6 +328,41 @@ export function createAppOriginController( // dedicated-origin rendering down. Retry once on an OS-assigned port, // loudly — whoever pinned the port to forward it needs to know. let retriedDynamic = false; + + /** + * The origin to publish under once bound: the operator's public origin + * (#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. + * - 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 + * the app that realm. Config time cannot see a bind-derived sandbox + * origin (it is known only once that listener binds, possibly on a + * fallback port); the callers pass the real one here. + */ + const adoptedOrigin = (derived: string): string => { + if (!publicOrigin) return derived; + if (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.`, + ); + return derived; + } + if (embedderOrigins?.includes(publicOrigin)) { + console.warn( + `App origin: not using MCP_APP_ORIGIN_FULL_ADDRESS=${publicOrigin}: it is the sandbox's or ` + + `the Inspector UI's origin, and a dedicated app origin must differ from both; ` + + `serving app documents from ${derived} instead.`, + ); + return derived; + } + return publicOrigin; + }; + server.on("error", (err: NodeJS.ErrnoException) => { if (err.code === "EADDRINUSE") { if (!retriedDynamic && port !== 0) { @@ -380,9 +415,7 @@ export function createAppOriginController( const urlHost = isAllInterfacesHost(canonicalHost) ? "localhost" : canonicalHost; - // An operator-supplied public origin (#1862) replaces the derived one - // only now that the listener it routes to is actually up. - origin = publicOrigin ?? `http://${urlHost}:${actualPort}`; + origin = adoptedOrigin(`http://${urlHost}:${actualPort}`); boundPort = actualPort; settle({ port: actualPort, url: origin }); }); diff --git a/clients/web/server/public-address.ts b/clients/web/server/public-address.ts index c428961d..001d0c60 100644 --- a/clients/web/server/public-address.ts +++ b/clients/web/server/public-address.ts @@ -92,9 +92,24 @@ function parsePublicUrl(raw: string): Parsed { return { url }; } +/** + * 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. + */ +export function describeForLog(raw: string): string { + try { + const u = new URL(raw); + return `"${u.protocol}${u.host ? `//${u.host}` : ""}${u.pathname}"`; + } catch { + return "(unparseable value, not echoed)"; + } +} + function warnIgnored(env: string, raw: string, reason: string): void { console.warn( - `Ignoring ${env}="${raw}": ${reason}. Advertising the bind-derived address instead.`, + `Ignoring ${env}=${describeForLog(raw)}: ${reason}. Advertising the bind-derived address instead.`, ); } @@ -157,8 +172,11 @@ export function resolveSandboxPublicUrl( * `undefined` when it is unset, blank, or refused (warned). * * @param allowedOrigins The resolved Inspector UI origins. - * @param sandboxUrl The advertised sandbox URL when it is known at config time - * (i.e. overridden). A bind-derived one is on its own port and cannot match. + * @param sandboxUrl The overridden sandbox URL, when there is one. This is the + * early, config-time check; a bind-derived sandbox origin is only known once + * that listener binds (possibly on a fallback port), so the authoritative + * comparison against it is made by the app-origin controller at start, which + * receives the real sandbox and Inspector origins as `embedderOrigins`. */ export function resolveAppOriginPublicOrigin( raw: string | undefined, diff --git a/clients/web/server/sandbox-controller.ts b/clients/web/server/sandbox-controller.ts index f47eb52e..d9102574 100644 --- a/clients/web/server/sandbox-controller.ts +++ b/clients/web/server/sandbox-controller.ts @@ -338,9 +338,20 @@ export function createSandboxController( const urlHost = isAllInterfacesHost(canonicalHost) ? "localhost" : canonicalHost; + 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. - sandboxUrl = publicUrl ?? `http://${urlHost}:${actualPort}/sandbox`; + // 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) { + console.warn( + `Sandbox: not advertising MCP_SANDBOX_FULL_ADDRESS, since it routes to port ${port}, ` + + `which is taken; advertising ${derivedUrl} instead.`, + ); + } + sandboxUrl = publicUrl && !retriedDynamic ? 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 175645d6..72cd41d5 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 @@ -393,6 +393,54 @@ describe("createAppOriginController", () => { } }); + it("does not use a public origin after falling back off its pinned port (#1862)", async () => { + const { port, release } = await claimPort(); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + controller = createAppOriginController({ + port, + host: "127.0.0.1", + publicOrigin: "https://apps.example.com", + }); + const result = await controller.start(); + expect(result.port).not.toBe(port); + expect(result.url).toBe(`http://127.0.0.1:${result.port}`); + expect( + controller.publish({ html: "

x

" })!.url.startsWith(result.url), + ).toBe(true); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("not using MCP_APP_ORIGIN_FULL_ADDRESS"), + ); + } finally { + warnSpy.mockRestore(); + await release(); + } + }); + + it("refuses a public origin equal to a trusted ancestor, including a bind-derived sandbox (#1862)", async () => { + // Config time cannot see the bind-derived sandbox origin; the callers pass + // it in embedderOrigins, so the controller is where the collision is caught. + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + controller = createAppOriginController({ + port: 0, + host: "127.0.0.1", + publicOrigin: "http://127.0.0.1:6275", + embedderOrigins: appDocumentEmbedders("http://127.0.0.1:6275/sandbox", [ + "http://localhost:6274", + ]), + }); + const result = await controller.start(); + expect(result.url).toBe(`http://127.0.0.1:${result.port}`); + expect(controller.getOrigin()).not.toBe("http://127.0.0.1:6275"); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("must differ from both"), + ); + } finally { + warnSpy.mockRestore(); + } + }); + it("degrades to publish()===null when listen fails for a non-EADDRINUSE reason", async () => { // The backends await start() during boot; if it ever stopped resolving the // whole web server would hang. Instead it resolves empty and every publish 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 89eff7fd..cb06a283 100644 --- a/clients/web/src/test/integration/server/public-address.test.ts +++ b/clients/web/src/test/integration/server/public-address.test.ts @@ -72,6 +72,17 @@ describe("public addresses (#1862)", () => { expect(warned()).toContain(reason); }); + it.each([ + ["https://user:hunter2@sb.example.com/sandbox", "hunter2"], + ["https://sb.example.com/sandbox?token=hunter2", "hunter2"], + ["https://sb.example.com/sandbox#hunter2", "hunter2"], + ["not a url hunter2", "hunter2"], + ])("never echoes a secret from a refused value (%s)", (raw, secret) => { + expect(resolveSandboxPublicUrl(raw, UI)).toBeUndefined(); + expect(warned()).toContain("MCP_SANDBOX_FULL_ADDRESS"); + expect(warned()).not.toContain(secret); + }); + 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 666a4633..0b5f0da1 100644 --- a/clients/web/src/test/integration/server/sandbox-controller.test.ts +++ b/clients/web/src/test/integration/server/sandbox-controller.test.ts @@ -495,6 +495,31 @@ describe("createSandboxController", () => { } }); + it("does not advertise a public URL after falling back off its pinned port (#1862)", async () => { + // The reverse proxy routes the public URL to the pinned port, which another + // process holds — so the public URL would reach the wrong listener. + const { port, release } = await claimPort(); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const controller = createSandboxController({ + port, + host: "127.0.0.1", + publicUrl: "https://sb.example.com/sandbox", + }); + try { + const result = await controller.start(); + expect(result.port).not.toBe(port); + expect(result.url).toBe(`http://127.0.0.1:${result.port}/sandbox`); + expect(controller.getUrl()).toBe(result.url); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("not advertising MCP_SANDBOX_FULL_ADDRESS"), + ); + } finally { + warnSpy.mockRestore(); + await controller.close(); + await release(); + } + }); + it("resolves with empty values when even the dynamic retry fails", async () => { // The Vite plugin awaits start() in configureServer; if start() ever stops // resolving, the entire dev backend hangs. This pins the resolve-on-error