fix(web): only use a public address on a fixed listener port (#1862)
Addresses Copilot review round 2. A reverse proxy routes a public address to one stable port, so neither MCP_SANDBOX_FULL_ADDRESS nor MCP_APP_ORIGIN_FULL_ADDRESS is used when the listener's port is OS-assigned: an explicit 0, a config-time collision resolved to 0, or the EADDRINUSE fallback. Validation warnings also stop echoing the URL path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HzuzRPJv7M8wGe3wyw5sSg Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
1dcb112987
commit
925461a043
7 changed files with 110 additions and 22 deletions
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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)";
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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 });
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<number> {
|
||||
const probe: Server = createServer();
|
||||
await new Promise<void>((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<void>((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("<p>proxied</p>");
|
||||
});
|
||||
|
||||
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", [
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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<number> {
|
||||
const probe: Server = createServer();
|
||||
await new Promise<void>((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<void>((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({
|
||||
|
|
|
|||
|
|
@ -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://<bind host>:<sandbox port>/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://<bind host>:<app-origin port>` | 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 `<origin>/app-document/<id>`. 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
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue