fix: bind loopback by address and pin the sandbox port so containers work
Two defaults that each break the Inspector inside a container. #1951 — the web UI never loads in a VS Code dev container. `HOST` defaulted to the NAME `localhost`, and `server.listen(port, host)` resolves a name through `dns.lookup` and binds the SINGLE address it gets back. Since Node 17 the default result order is `verbatim`, so on a glibc Linux host — whose /etc/hosts maps localhost to both 127.0.0.1 and ::1 — that is `::1`. The server bound IPv6 loopback only and refused every IPv4 client, including the dev container port forwarder. It presents as an endless spinner rather than a connection error because the forwarder accepts on the host side before failing to connect inward. Default to the ADDRESS 127.0.0.1 instead; an explicit HOST=localhost is still honored as typed. #2008 — the Apps tab is unreachable in any container. The MCP Apps sandbox listened on an OS-assigned port, so it changed every run and could never be named in a devcontainer `forwardPorts`, a `docker run -p`, or a tunnel. Default it to a fixed 6275, mirroring CLIENT_PORT's fixed 6274, and retry once on an OS-assigned port (loudly) if it is taken — the collision a dynamic default never had. Verified in the reporter's exact image (mcr.microsoft.com/devcontainers/typescript-node:4-24-trixie): both listeners come up on 127.0.0.1 and answer 200 on 6274 and 6275/sandbox. Also EXPOSE 6275 and set MCP_SANDBOX_PORT in the image, and document both defaults in the web README, the migration guide, and the root README's Docker section. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
5ec2ecf956
commit
adde9ccf94
3 changed files with 246 additions and 52 deletions
|
|
@ -1,12 +1,24 @@
|
|||
import { describe, it, expect } from "vitest";
|
||||
import {
|
||||
BIND_ALL_INTERFACES_ENV,
|
||||
DEFAULT_BIND_HOST,
|
||||
resolveBindHostname,
|
||||
} from "../../../../server/resolve-bind-host.js";
|
||||
|
||||
describe("resolveBindHostname", () => {
|
||||
it("defaults to localhost when HOST is unset", () => {
|
||||
expect(resolveBindHostname({})).toBe("localhost");
|
||||
// Regression guard for #1951. The default must be an *address*, never the
|
||||
// name `localhost`: `listen()` resolves a name and binds the single address
|
||||
// the resolver returns first, which on glibc Linux is `::1` — leaving every
|
||||
// IPv4 client (a dev container's port forwarder, an `ssh -L` to 127.0.0.1, a
|
||||
// container healthcheck) refused. See DEFAULT_BIND_HOST.
|
||||
it("defaults to the IPv4 loopback address, not the name localhost", () => {
|
||||
expect(resolveBindHostname({})).toBe("127.0.0.1");
|
||||
expect(resolveBindHostname({})).toBe(DEFAULT_BIND_HOST);
|
||||
expect(resolveBindHostname({})).not.toBe("localhost");
|
||||
});
|
||||
|
||||
it("still honors an explicit HOST=localhost as typed", () => {
|
||||
expect(resolveBindHostname({ HOST: "localhost" })).toBe("localhost");
|
||||
});
|
||||
|
||||
it("returns a loopback HOST unchanged", () => {
|
||||
|
|
|
|||
|
|
@ -2,6 +2,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
|
|||
import { createServer, type Server } from "node:http";
|
||||
import {
|
||||
createSandboxController,
|
||||
DEFAULT_SANDBOX_PORT,
|
||||
resolveSandboxPort,
|
||||
sandboxFrameAncestors,
|
||||
} from "../../../../server/sandbox-controller.js";
|
||||
|
|
@ -75,7 +76,17 @@ describe("resolveSandboxPort", () => {
|
|||
else process.env.SERVER_PORT = envSnapshot.server;
|
||||
});
|
||||
|
||||
it("returns 0 when no env vars are set", () => {
|
||||
// Regression guard for #2008: the default must be a FIXED port. An
|
||||
// OS-assigned one changes every run, so it can never be named in a dev
|
||||
// container's `forwardPorts`, a `docker run -p`, or an SSH tunnel — leaving
|
||||
// the Apps tab unreachable in every such environment.
|
||||
it("returns a fixed default port when no env vars are set", () => {
|
||||
expect(resolveSandboxPort()).toBe(DEFAULT_SANDBOX_PORT);
|
||||
expect(resolveSandboxPort()).not.toBe(0);
|
||||
});
|
||||
|
||||
it("still honors an explicit MCP_SANDBOX_PORT=0 as OS-assigned", () => {
|
||||
process.env.MCP_SANDBOX_PORT = "0";
|
||||
expect(resolveSandboxPort()).toBe(0);
|
||||
});
|
||||
|
||||
|
|
@ -102,19 +113,19 @@ describe("resolveSandboxPort", () => {
|
|||
expect(resolveSandboxPort()).toBe(9100);
|
||||
});
|
||||
|
||||
it("returns 0 when SERVER_PORT is non-numeric and no MCP_SANDBOX_PORT", () => {
|
||||
it("falls back to the default when SERVER_PORT is non-numeric and no MCP_SANDBOX_PORT", () => {
|
||||
process.env.SERVER_PORT = "not-a-port";
|
||||
expect(resolveSandboxPort()).toBe(0);
|
||||
expect(resolveSandboxPort()).toBe(DEFAULT_SANDBOX_PORT);
|
||||
});
|
||||
|
||||
it("ignores empty-string SERVER_PORT", () => {
|
||||
process.env.SERVER_PORT = "";
|
||||
expect(resolveSandboxPort()).toBe(0);
|
||||
expect(resolveSandboxPort()).toBe(DEFAULT_SANDBOX_PORT);
|
||||
});
|
||||
|
||||
it("ignores negative values", () => {
|
||||
process.env.MCP_SANDBOX_PORT = "-1";
|
||||
expect(resolveSandboxPort()).toBe(0);
|
||||
expect(resolveSandboxPort()).toBe(DEFAULT_SANDBOX_PORT);
|
||||
});
|
||||
|
||||
it("rejects a partial-parse value (6274abc) rather than binding 6274", () => {
|
||||
|
|
@ -125,7 +136,7 @@ describe("resolveSandboxPort", () => {
|
|||
|
||||
it("rejects an out-of-range value (70000) so it can't crash listen", () => {
|
||||
process.env.MCP_SANDBOX_PORT = "70000";
|
||||
expect(resolveSandboxPort()).toBe(0);
|
||||
expect(resolveSandboxPort()).toBe(DEFAULT_SANDBOX_PORT);
|
||||
});
|
||||
});
|
||||
|
||||
|
|
@ -135,7 +146,8 @@ describe("createSandboxController", () => {
|
|||
try {
|
||||
const { url, port } = await controller.start();
|
||||
expect(port).toBeGreaterThan(0);
|
||||
expect(url).toBe(`http://localhost:${port}/sandbox`);
|
||||
// Default host is now the IPv4 loopback address, not the name (#1951).
|
||||
expect(url).toBe(`http://127.0.0.1:${port}/sandbox`);
|
||||
expect(controller.getUrl()).toBe(url);
|
||||
|
||||
const res = await fetch(url);
|
||||
|
|
@ -312,11 +324,11 @@ describe("createSandboxController", () => {
|
|||
}
|
||||
});
|
||||
|
||||
it("resolves with empty values when listen fails (EADDRINUSE)", async () => {
|
||||
// Bind a placeholder HTTP server to claim a port, then point a sandbox
|
||||
// controller at the same port to force EADDRINUSE. The Vite plugin awaits
|
||||
// start() in configureServer; if start() ever stops resolving, the entire
|
||||
// dev backend hangs. This test pins down the resolve-on-error contract.
|
||||
/** Claim a port so a subsequent listen on it fails with EADDRINUSE. */
|
||||
async function claimPort(): Promise<{
|
||||
port: number;
|
||||
release: () => Promise<void>;
|
||||
}> {
|
||||
const blocker: Server = createServer();
|
||||
await new Promise<void>((resolve) =>
|
||||
blocker.listen(0, "127.0.0.1", () => resolve()),
|
||||
|
|
@ -327,21 +339,96 @@ describe("createSandboxController", () => {
|
|||
? addr.port
|
||||
: 0;
|
||||
expect(port).toBeGreaterThan(0);
|
||||
return {
|
||||
port,
|
||||
release: () =>
|
||||
new Promise<void>((resolve) => blocker.close(() => resolve())),
|
||||
};
|
||||
}
|
||||
|
||||
it("falls back to an OS-assigned port when the fixed port is taken", async () => {
|
||||
// #2008 made the default port fixed, which introduced a collision the
|
||||
// dynamic default never had: a second Inspector, or anything else already
|
||||
// on 6275, would have taken the Apps tab down entirely. Retrying on a
|
||||
// dynamic port keeps it working locally.
|
||||
const { port, release } = await claimPort();
|
||||
const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
const controller = createSandboxController({ port, host: "127.0.0.1" });
|
||||
try {
|
||||
const result = await controller.start();
|
||||
expect(result.port).toBeGreaterThan(0);
|
||||
expect(result.port).not.toBe(port);
|
||||
expect(result.url).toBe(`http://127.0.0.1:${result.port}/sandbox`);
|
||||
expect(controller.getUrl()).toBe(result.url);
|
||||
// The sandbox must actually be serving on the fallback port, not merely
|
||||
// reporting one.
|
||||
expect((await fetch(result.url)).status).toBe(200);
|
||||
// Loud, not silent: a pinned-and-forwarded port that quietly moved is the
|
||||
// very failure the fixed default exists to prevent.
|
||||
expect(warnSpy).toHaveBeenCalledWith(
|
||||
expect.stringContaining(`Sandbox: port ${port} in use`),
|
||||
);
|
||||
expect(warnSpy).toHaveBeenCalledWith(
|
||||
expect.stringContaining("MCP_SANDBOX_PORT"),
|
||||
);
|
||||
} 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
|
||||
// contract for the case the retry can't rescue — an explicit `port: 0` that
|
||||
// fails has nowhere left to fall back to, so it degrades as before.
|
||||
const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {});
|
||||
const controller = createSandboxController({ port: 0, host: "127.0.0.1" });
|
||||
try {
|
||||
// Force the listen to fail the way a dynamic listen can (an unusable bind
|
||||
// address), since port 0 cannot collide.
|
||||
const broken = createSandboxController({
|
||||
port: 0,
|
||||
host: "203.0.113.1", // TEST-NET-3: not assigned to any local interface
|
||||
});
|
||||
const result = await broken.start();
|
||||
expect(result).toEqual({ port: 0, url: "" });
|
||||
expect(broken.getUrl()).toBeNull();
|
||||
// close() must be a no-op since the server never bound.
|
||||
await expect(broken.close()).resolves.toBeUndefined();
|
||||
expect(errorSpy).toHaveBeenCalledWith(
|
||||
"Sandbox server error:",
|
||||
expect.objectContaining({ code: "EADDRNOTAVAIL" }),
|
||||
);
|
||||
} finally {
|
||||
errorSpy.mockRestore();
|
||||
await controller.close();
|
||||
}
|
||||
});
|
||||
|
||||
it("gives up after one retry when the dynamic fallback also fails", async () => {
|
||||
// Guards the `retriedDynamic` latch: without it an EADDRINUSE loop could
|
||||
// re-listen forever instead of settling.
|
||||
const { port, release } = await claimPort();
|
||||
const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {});
|
||||
const controller = createSandboxController({ port, host: "127.0.0.1" });
|
||||
try {
|
||||
const result = await controller.start();
|
||||
expect(result).toEqual({ port: 0, url: "" });
|
||||
expect(controller.getUrl()).toBeNull();
|
||||
// close() must be a no-op since the server never bound.
|
||||
await expect(controller.close()).resolves.toBeUndefined();
|
||||
expect(errorSpy).toHaveBeenCalledWith(
|
||||
expect.stringContaining(`Sandbox: port ${port} in use`),
|
||||
);
|
||||
// The retry succeeds here (a dynamic port is always available), so assert
|
||||
// the latch is what bounds it: exactly one retry warning was emitted.
|
||||
expect(
|
||||
warnSpy.mock.calls.filter((c) =>
|
||||
String(c[0]).includes("falling back to an OS-assigned port"),
|
||||
),
|
||||
).toHaveLength(1);
|
||||
expect(result.port).toBeGreaterThan(0);
|
||||
} finally {
|
||||
warnSpy.mockRestore();
|
||||
errorSpy.mockRestore();
|
||||
await new Promise<void>((resolve) => blocker.close(() => resolve()));
|
||||
await controller.close();
|
||||
await release();
|
||||
}
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -7,6 +7,7 @@ import {
|
|||
webServerConfigToInitialPayload,
|
||||
type WebServerConfig,
|
||||
} from "../../../../server/web-server-config.js";
|
||||
import { DEFAULT_SANDBOX_PORT } from "../../../../server/sandbox-controller.js";
|
||||
import {
|
||||
API_SERVER_ENV_VARS,
|
||||
LEGACY_AUTH_TOKEN_ENV,
|
||||
|
|
@ -32,7 +33,7 @@ const MUTATED_ENV_KEYS = [
|
|||
|
||||
const baseConfig = (): WebServerConfig => ({
|
||||
port: 6274,
|
||||
hostname: "localhost",
|
||||
hostname: "127.0.0.1",
|
||||
authToken: "tok",
|
||||
dangerouslyOmitAuth: false,
|
||||
initialMcpConfig: null,
|
||||
|
|
@ -42,7 +43,7 @@ const baseConfig = (): WebServerConfig => ({
|
|||
storageDir: undefined,
|
||||
allowedOrigins: ["http://localhost:6274"],
|
||||
sandboxPort: 0,
|
||||
sandboxHost: "localhost",
|
||||
sandboxHost: "127.0.0.1",
|
||||
logger: undefined,
|
||||
autoOpen: false,
|
||||
});
|
||||
|
|
@ -72,7 +73,7 @@ describe("buildWebServerConfigFromEnv", () => {
|
|||
it("returns defaults when no env vars are set", () => {
|
||||
const cfg = buildWebServerConfigFromEnv();
|
||||
expect(cfg.port).toBe(6274);
|
||||
expect(cfg.hostname).toBe("localhost");
|
||||
expect(cfg.hostname).toBe("127.0.0.1");
|
||||
expect(cfg.authToken).toBe("");
|
||||
expect(cfg.dangerouslyOmitAuth).toBe(false);
|
||||
expect(cfg.initialMcpConfig).toBeNull();
|
||||
|
|
@ -82,8 +83,8 @@ describe("buildWebServerConfigFromEnv", () => {
|
|||
"http://127.0.0.1:6274",
|
||||
"http://[::1]:6274",
|
||||
]);
|
||||
expect(cfg.sandboxPort).toBe(0);
|
||||
expect(cfg.sandboxHost).toBe("localhost");
|
||||
expect(cfg.sandboxPort).toBe(DEFAULT_SANDBOX_PORT);
|
||||
expect(cfg.sandboxHost).toBe("127.0.0.1");
|
||||
expect(cfg.logger).toBeUndefined();
|
||||
// Vitest sets `process.env.VITEST = 'true'`, so the autoOpen default is
|
||||
// suppressed here. Real `vite dev` runs don't set VITEST and default to
|
||||
|
|
@ -289,7 +290,9 @@ describe("buildWebServerConfigFromEnv", () => {
|
|||
|
||||
it("ignores non-numeric SERVER_PORT", () => {
|
||||
process.env.SERVER_PORT = "nope";
|
||||
expect(buildWebServerConfigFromEnv().sandboxPort).toBe(0);
|
||||
expect(buildWebServerConfigFromEnv().sandboxPort).toBe(
|
||||
DEFAULT_SANDBOX_PORT,
|
||||
);
|
||||
});
|
||||
|
||||
it("treats MCP_SANDBOX_PORT empty string as unset", () => {
|
||||
|
|
@ -596,7 +599,7 @@ describe("printServerBanner", () => {
|
|||
it("includes the auth token in the URL when auth is enabled", () => {
|
||||
const url = printServerBanner(baseConfig(), 6274, "secret", undefined);
|
||||
expect(url).toBe(
|
||||
`http://localhost:6274?${API_SERVER_ENV_VARS.AUTH_TOKEN}=secret`,
|
||||
`http://127.0.0.1:6274?${API_SERVER_ENV_VARS.AUTH_TOKEN}=secret`,
|
||||
);
|
||||
expect(logSpy.lines.some((l) => l.includes("Auth token: secret"))).toBe(
|
||||
true,
|
||||
|
|
@ -607,7 +610,7 @@ describe("printServerBanner", () => {
|
|||
const cfg = baseConfig();
|
||||
cfg.dangerouslyOmitAuth = true;
|
||||
const url = printServerBanner(cfg, 6274, "irrelevant", undefined);
|
||||
expect(url).toBe("http://localhost:6274");
|
||||
expect(url).toBe("http://127.0.0.1:6274");
|
||||
expect(
|
||||
logSpy.lines.some((l) =>
|
||||
l.includes("Auth: disabled (DANGEROUSLY_OMIT_AUTH)"),
|
||||
|
|
@ -617,7 +620,7 @@ describe("printServerBanner", () => {
|
|||
|
||||
it("omits the query string when no token is supplied", () => {
|
||||
const url = printServerBanner(baseConfig(), 6274, "", undefined);
|
||||
expect(url).toBe("http://localhost:6274");
|
||||
expect(url).toBe("http://127.0.0.1:6274");
|
||||
});
|
||||
|
||||
it("brackets an IPv6 bind host in the printed URL", () => {
|
||||
|
|
|
|||
Loading…
Reference in a new issue