diff --git a/.gitignore b/.gitignore index 93d61ce45..7e0989944 100644 --- a/.gitignore +++ b/.gitignore @@ -8,6 +8,10 @@ clients/web/dist clients/**/build clients/**/coverage test-servers/build +# TypeScript incremental build cache (written by `tsc -b`); web routes its own +# into node_modules/.tmp via tsBuildInfoFile, but a stray `tsc -b` elsewhere +# drops one next to the tsconfig — never source. +*.tsbuildinfo .env /.playwright-mcp/ diff --git a/AGENTS.md b/AGENTS.md index fe51737a8..9b9ac1b77 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -17,6 +17,10 @@ inspector/ │ │ │ # sandbox-controller.ts (MCP Apps sandbox HTTP server), │ │ │ # inject-auth-token.ts (embeds the API token into served index.html), │ │ │ # vite-base-config.ts (shared optimizeDeps exclusions), +│ │ │ # resolve-bind-host.ts (bind-host POLICY: refuses an +│ │ │ # all-interfaces HOST unless DANGEROUSLY_BIND_ALL_INTERFACES; +│ │ │ # the all-interfaces DETECTION is core/node/hostUrl.isAllInterfacesHost. +│ │ │ # Used by both bind points — web-server-config.ts + vite.config.ts — #1795), │ │ │ # browser-externalized-builtin-gate.ts (build-gate logic that fails │ │ │ # `vite build` on a browser-externalized Node built-in — #1769) │ │ └── static/ # sandbox_proxy.html (served by sandbox-controller for MCP Apps tab) @@ -50,6 +54,14 @@ inspector/ │ │ ├── remote/ # Browser HTTP/SSE transport + remote logger/fetch │ │ │ └── node/ # Hono-based remote server backend (used by remote/ above) │ │ └── state/ # InspectorClient state stores consumed by core/react/ +│ ├── node/ # Node-only shared helpers: version.ts (readInspectorVersion, +│ │ # walks to the root package.json), hostUrl.ts (shared host +│ │ # normalization + detection — formatHostForUrl brackets IPv6, +│ │ # canonicalUrlHost canonicalizes a bind host the way a browser +│ │ # builds `Origin`, isAllInterfacesHost is the wildcard-bind +│ │ # predicate the guard is built on, isLoopbackHost gates the OAuth +│ │ # callback listener; also stripBrackets. Used across +│ │ # clients/web/server, clients/cli, and core/auth/node — #1795) │ ├── react/ # React hooks over the state stores │ └── storage/ # File I/O helpers (store-io.ts) used by OAuth persist backends ├── test-servers/ # Composable MCP test servers + fixtures used by integration tests. diff --git a/Dockerfile b/Dockerfile index 0651b7d28..986231a09 100644 --- a/Dockerfile +++ b/Dockerfile @@ -25,8 +25,11 @@ COPY --from=builder /build/modelcontextprotocol-inspector-*.tgz /tmp/inspector.t RUN npm install -g /tmp/inspector.tgz && rm /tmp/inspector.tgz # Serve on all interfaces so the UI is reachable from outside the container, and -# never try to open a browser from inside it. +# never try to open a browser from inside it. Binding 0.0.0.0 is refused by +# default (it exposes the process-spawning backend to the network); a container +# is the sanctioned exception, so opt in explicitly via DANGEROUSLY_BIND_ALL_INTERFACES. ENV HOST=0.0.0.0 \ + DANGEROUSLY_BIND_ALL_INTERFACES=true \ CLIENT_PORT=6274 \ MCP_AUTO_OPEN_ENABLED=false EXPOSE 6274 diff --git a/README.md b/README.md index a8b358ce7..652a9b41a 100644 --- a/README.md +++ b/README.md @@ -32,6 +32,7 @@ inspector/ │ ├── json/ # JSON + parameter/argument conversion utilities │ ├── logging/ # Silent pino logger singleton │ ├── mcp/ # InspectorClient runtime, state stores, transports, config import +│ ├── node/ # Node-only shared helpers: version reader, hostUrl (host normalize/canonicalize + all-interfaces/loopback detection) │ ├── react/ # React hooks over the state stores │ └── storage/ # File I/O helpers for the OAuth persist backends ├── test-servers/ # Composable MCP test servers + fixtures used by integration tests @@ -198,7 +199,7 @@ docker build -t mcp-inspector . docker run --rm -p 6274:6274 mcp-inspector ``` -The image defaults to `--web` bound to `0.0.0.0:6274` with browser auto-open disabled; override the args to run another mode (`docker run --rm ghcr.io/modelcontextprotocol/inspector --cli …`). Pass `-e MCP_INSPECTOR_API_TOKEN=…` to set a known token (otherwise one is generated and printed in the logs), or `-e DANGEROUSLY_OMIT_AUTH=true` to disable auth. The image runs as the non-root `node` user and has a `HEALTHCHECK` that probes the web UI — it assumes the default `--web` mode, so add `--no-healthcheck` when running `--cli`/`--tui` (which have no web server). +The image defaults to `--web` bound to `0.0.0.0:6274` with browser auto-open disabled; override the args to run another mode (`docker run --rm ghcr.io/modelcontextprotocol/inspector --cli …`). Pass `-e MCP_INSPECTOR_API_TOKEN=…` to set a known token (otherwise one is generated and printed in the logs), or `-e DANGEROUSLY_OMIT_AUTH=true` to disable auth. Binding `0.0.0.0` (all network interfaces) is refused by default outside a container — it exposes the process-spawning backend to the local network — so the image opts in explicitly with `DANGEROUSLY_BIND_ALL_INTERFACES=true` (already set in the `Dockerfile`); a bare `HOST=0.0.0.0` without that flag exits with an error. If you **remap the published port** (`-p 8080:6274`), the browser's origin (`http://localhost:8080`) no longer matches the in-container port, so set `-e ALLOWED_ORIGINS=http://localhost:8080,http://127.0.0.1:8080` (or run `-e CLIENT_PORT=8080 -p 8080:8080`) or connects will 403. `ALLOWED_ORIGINS` **replaces** the default list rather than merging, so list every loopback form you'll browse from (see the [web README](./clients/web/README.md#host-binding--the-origin-allow-list)). The image runs as the non-root `node` user and has a `HEALTHCHECK` that probes the web UI — it assumes the default `--web` mode, so add `--no-healthcheck` when running `--cli`/`--tui` (which have no web server). ## Contributing — `AGENTS.md` and `CLAUDE.md` diff --git a/clients/cli/README.md b/clients/cli/README.md index 6abb958fe..e1b3c4ab5 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -183,7 +183,7 @@ Register `http://127.0.0.1:6276/oauth/callback` on static or enterprise IdPs tha | `--client-id ` | — | OAuth client ID (static client); overrides `client.json`. | | `--client-secret ` | — | OAuth client secret; overrides `client.json`. | | `--client-metadata-url ` | — | CIMD metadata URL; overrides `client.json`. | -| `--callback-url ` | `MCP_OAUTH_CALLBACK_URL` | Redirect URI sent to the authorization server (default: `http://127.0.0.1:6276/oauth/callback`). | +| `--callback-url ` | `MCP_OAUTH_CALLBACK_URL` | Redirect URI sent to the authorization server (default: `http://127.0.0.1:6276/oauth/callback`). Must bind a **loopback** host (`localhost` / `127.0.0.0/8` / `[::1]`) — the listener receives the authorization code over plaintext `http`, so a non-loopback host hard-errors (no opt-in; use a port-forward if the browser is elsewhere). | | — | `MCP_AUTO_OPEN_ENABLED` | Browser auto-open **and** non-TTY interactive-OAuth admit: `true` (allow interactive OAuth without a TTY **and** force-open the browser — same as the web launcher), `false` (never open), unset (open on a TTY unless `VITEST` is set). For CI that must not hang, prefer `--stored-auth-only`. | **Example** — list tools on an OAuth-protected server using stored tokens and CIMD from the command line: diff --git a/clients/cli/__tests__/stored-auth.test.ts b/clients/cli/__tests__/stored-auth.test.ts index 24c87c49e..4a45ec33d 100644 --- a/clients/cli/__tests__/stored-auth.test.ts +++ b/clients/cli/__tests__/stored-auth.test.ts @@ -580,6 +580,65 @@ describe("--print-handoff", () => { expect(out.apiToken).toBe("tok123"); }); + it("canonicalizes the deep-link host so it matches the web allow-list (mapped IPv4)", async () => { + // HOST=::ffff:127.0.0.1 binds/serves 127.0.0.1; the deep link must advertise + // that canonical host, not [::ffff:127.0.0.1], or the web autoConnect POST + // 403s (the web allow-list emits the loopback trio for this HOST). + const result = await runCli( + ["--print-handoff", "--server-url", "https://x.example/mcp"], + { + env: { + MCP_INSPECTOR_API_TOKEN: "tok123", + HOST: "::ffff:127.0.0.1", + CLIENT_PORT: "16274", + }, + }, + ); + expectCliSuccess(result); + const out = JSON.parse(result.stdout) as { deepLink: string }; + expect(out.deepLink.startsWith("http://127.0.0.1:16274/?")).toBe(true); + }); + + it("advertises localhost in the deep link for a wildcard HOST", async () => { + // 0.0.0.0 is allow-listed so it connects, but the deep link is handed to a + // human — advertise localhost like the web banner does. + const result = await runCli( + ["--print-handoff", "--server-url", "https://x.example/mcp"], + { + env: { + MCP_INSPECTOR_API_TOKEN: "tok123", + HOST: "0.0.0.0", + DANGEROUSLY_BIND_ALL_INTERFACES: "true", + CLIENT_PORT: "16274", + }, + }, + ); + expectCliSuccess(result); + const out = JSON.parse(result.stdout) as { deepLink: string }; + expect(out.deepLink.startsWith("http://localhost:16274/?")).toBe(true); + }); + + it("classifies a bad --callback-url as a usage error, not auth_required", async () => { + // The guard's message contains "OAuth"; without the explicit exit-code pin + // the heuristic would map it to auth_required (exit 3) and tell an automated + // caller to re-auth on a config error. Fires before connect, so any server. + const result = await runCli([ + "--server-url", + "https://x.example/mcp", + "--method", + "tools/list", + "--callback-url", + "http://0.0.0.0:6276/oauth/callback", + ]); + expect(result.exitCode).toBe(1); + const envelope = JSON.parse(result.stderr.trim()) as { + error: { code: string; message: string }; + }; + // "error" (USAGE), not "auth_required" — the point of pinning the exit code. + expect(envelope.error.code).toBe("error"); + expect(envelope.error.message).toContain("must bind a loopback host"); + }); + it("derives transport=sse for an SSE server (auto-detected from the /sse path)", async () => { const result = await runCli( ["--print-handoff", "--server-url", "https://x.example/sse"], diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index eddae3ab4..a672be42f 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -23,6 +23,10 @@ import { parseHeaderPair, } from "@inspector/core/mcp/node/index.js"; import type { JsonValue } from "@inspector/core/mcp/index.js"; +import { + canonicalUrlHost, + isAllInterfacesHost, +} from "@inspector/core/node/hostUrl.js"; import { getStateFilePath } from "@inspector/core/auth/node/storage-node.js"; import { consumeMethodOutcome } from "./handlers/consume-outcome.js"; import { runMethod } from "./handlers/run-method.js"; @@ -422,6 +426,13 @@ function buildHandoff( transport: "sse" | "http" | "stdio" | undefined, ): McpResponse { const host = process.env.HOST || "127.0.0.1"; + // The deep link is a URL handed to a human, so advertise localhost for a + // wildcard bind (like the web banner/sandbox URL) rather than the awkward + // http://0.0.0.0 / http://[::] — both are allow-listed, but neither is a nice + // URL to click; otherwise use the canonical host so it matches the allow-list. + const linkHost = isAllInterfacesHost(host) + ? "localhost" + : canonicalUrlHost(host); const clientPort = process.env.CLIENT_PORT || "6274"; const sandboxPort = process.env.MCP_SANDBOX_PORT || "6275"; // Treat an empty MCP_INSPECTOR_API_TOKEN the same as unset — an empty token @@ -440,7 +451,7 @@ function buildHandoff( if (apiToken) params.set("autoConnect", apiToken); return { serverUrl: normalizedUrl, - deepLink: `http://${host}:${clientPort}/?${params.toString()}`, + deepLink: `http://${linkHost}:${clientPort}/?${params.toString()}`, portForwardCmd: `coder port-forward --tcp ${clientPort}:${clientPort} --tcp ${sandboxPort}:${sandboxPort}`, oauthStatePath: statePath, apiToken: apiToken ?? null, @@ -665,7 +676,7 @@ async function parseArgs(argv?: string[]): Promise { ) .option( "--callback-url ", - `OAuth redirect/callback listener URL (default: ${DEFAULT_RUNNER_OAUTH_CALLBACK_URL}, or MCP_OAUTH_CALLBACK_URL)`, + `OAuth redirect/callback listener URL; must be loopback (default: ${DEFAULT_RUNNER_OAUTH_CALLBACK_URL}, or MCP_OAUTH_CALLBACK_URL)`, ) .option( "--use-stored-auth", @@ -1028,7 +1039,20 @@ export async function runCli(argv?: string[]): Promise { relogin, } = parsed; const clientConfig = await loadRunnerClientConfig({ clientConfigPath }); - const callbackUrlConfig = parseRunnerOAuthCallbackUrl(callbackUrl); + // A bad --callback-url / MCP_OAUTH_CALLBACK_URL is a *usage* error, but its + // messages contain "OAuth", which the exit-code heuristic (error-handler.ts) + // would otherwise classify as AUTH_REQUIRED (exit 3) — telling an automated + // caller to kick the auth flow instead of fixing the flag. `core/` can't + // import CliExitCodeError, so pin the class here. + let callbackUrlConfig: RunnerOAuthCallbackConfig; + try { + callbackUrlConfig = parseRunnerOAuthCallbackUrl(callbackUrl); + } catch (err) { + throw new CliExitCodeError( + EXIT_CODES.USAGE, + err instanceof Error ? err.message : String(err), + ); + } await callMethod( serverConfig, serverSettings, diff --git a/clients/cli/src/index.ts b/clients/cli/src/index.ts index 93c1fa228..a03023939 100644 --- a/clients/cli/src/index.ts +++ b/clients/cli/src/index.ts @@ -5,7 +5,11 @@ import { fileURLToPath } from "url"; import { runCli, validLogLevels } from "./cli.js"; import { handleError } from "./error-handler.js"; -export { runCli, validLogLevels }; +// `handleError` is exported so the launcher (which imports `runCli` as a module +// and owns the rejection) can route a `mcp-inspector --cli` failure through the +// CLI's own sink — preserving the EXIT_CODES map and the JSON `{"error":…}` +// envelope that this bin's own `.catch` provides only when run directly. +export { runCli, validLogLevels, handleError }; const __filename = fileURLToPath(import.meta.url); const isMain = diff --git a/clients/launcher/README.md b/clients/launcher/README.md index 73e268c61..c03f52aab 100644 --- a/clients/launcher/README.md +++ b/clients/launcher/README.md @@ -10,6 +10,8 @@ The launcher is the package that provides the global `mcp-inspector` binary (e.g All configuration parsing, config-file loading, and server setup are handled by the app runners and by **core**; the launcher does not interpret config or env vars. +**Error reporting.** A `--cli` failure is routed through the CLI's own error sink, so `mcp-inspector --cli` preserves the CLI exit-code map (`1` usage, `2` no-app, `3` auth-required, `4` unreachable, `5` tool-error) and its machine-readable `{"error":{…}}` stderr envelope — the same as invoking the CLI bin directly. `--web` / `--tui` failures print a human-readable `Error: ` and exit `1` (append `MCP_DEBUG=1` for the stack). + ## Web server-list flags (`--web`) `mcp-inspector --web` chooses which server list the UI shows and whether it is diff --git a/clients/launcher/src/index.ts b/clients/launcher/src/index.ts index a8e4f2dc9..faf5e39f9 100644 --- a/clients/launcher/src/index.ts +++ b/clients/launcher/src/index.ts @@ -13,6 +13,20 @@ function clientEntry(client: "web" | "cli" | "tui"): string { ).href; } +/** + * Whether to append the error stack on a top-level failure. Gated on `MCP_DEBUG` + * or `DEBUG` being *meaningfully* set — `"0"` / `"false"` / empty read as off, so + * a stray `DEBUG=0` doesn't turn stacks on (and `DEBUG` stays useful for the + * `debug` package's namespace filter, which is any non-empty value). + */ +function wantsDebugStack(): boolean { + const on = (v: string | undefined): boolean => { + const s = v?.trim().toLowerCase(); + return !!s && s !== "0" && s !== "false"; + }; + return on(process.env.MCP_DEBUG) || on(process.env.DEBUG); +} + const program = new Command(); program @@ -49,8 +63,27 @@ async function run(): Promise { const { runWeb } = await import(clientEntry("web")); await runWeb(forwardedArgv); } else if (mode === "cli") { - const { runCli } = await import(clientEntry("cli")); - await runCli(forwardedArgv); + // Route a CLI failure through the CLI's own error sink so `mcp-inspector + // --cli` (a module import here, so the CLI bin's own `.catch(handleError)` + // never fires) still honors the EXIT_CODES map and emits the JSON + // `{"error":…}` envelope its README documents — instead of the generic + // exit-1 catch-all below. + const { runCli, handleError } = await import(clientEntry("cli")); + try { + // On success, let `run()` resolve and the event loop drain (rather than a + // `process.exit(0)`) — the CLI closes its own transports, and an eager exit + // risks truncating a large stdout payload on a pipe (async on macOS). + await runCli(forwardedArgv); + } catch (err) { + // A stale `cli/build` (built before handleError was exported) would make + // this `undefined`; fall through to the generic sink with the real message + // rather than throwing "handleError is not a function" over it. + if (typeof handleError !== "function") throw err; + // write-then-exit like the direct bin; the envelope is a few hundred bytes, + // well inside the pipe buffer, so the truncation risk noted above (which is + // about a large stdout payload) doesn't apply to this stderr line. + handleError(err); // writes the envelope + process.exit(code); never returns + } } else { const { runTui } = await import(clientEntry("tui")); await runTui(forwardedArgv); @@ -58,6 +91,15 @@ async function run(): Promise { } run().catch((err: unknown) => { - console.error("Error running MCP Inspector:", err); + // Print the message, not the Error stack — a startup config error (a bad flag, + // the OAuth callback loopback guard) should read as actionable, not internal. + // The stack is still available under DEBUG / MCP_DEBUG for real faults. + console.error( + "Error running MCP Inspector:", + err instanceof Error ? err.message : err, + ); + if (wantsDebugStack() && err instanceof Error) { + console.error(err.stack); + } process.exit(1); }); diff --git a/clients/tui/README.md b/clients/tui/README.md index f80c7a686..ea5582156 100644 --- a/clients/tui/README.md +++ b/clients/tui/README.md @@ -58,7 +58,7 @@ OAuth redirect URIs must match **exactly** what you register on the authorizatio | `--client-id ` | — | OAuth client ID (static client); overrides `client.json`. | | `--client-secret ` | — | OAuth client secret (confidential clients); overrides `client.json`. | | `--client-metadata-url ` | — | Client ID Metadata Document URL (CIMD); overrides `client.json`. | -| `--callback-url ` | `MCP_OAUTH_CALLBACK_URL` | OAuth redirect/callback listener (default: `http://127.0.0.1:6276/oauth/callback`). | +| `--callback-url ` | `MCP_OAUTH_CALLBACK_URL` | OAuth redirect/callback listener (default: `http://127.0.0.1:6276/oauth/callback`). Must bind a **loopback** host (`localhost` / `127.0.0.0/8` / `[::1]`); a non-loopback host hard-errors, since the listener receives the authorization code over plaintext `http`. | #### Authenticating in the TUI diff --git a/clients/tui/index.ts b/clients/tui/index.ts index 46f6d8fed..33c91ece3 100644 --- a/clients/tui/index.ts +++ b/clients/tui/index.ts @@ -13,7 +13,25 @@ const isMain = if (isMain) { runTui(process.argv).catch((err: unknown) => { - console.error(err); + // Print the message, not the stack — a startup config error (e.g. the + // OAuth callback loopback guard) should read as an actionable message, not + // an internal fault. Matches run-web.ts's house pattern. The stack is still + // available under DEBUG / MCP_DEBUG for a real fault. + console.error("Error:", err instanceof Error ? err.message : err); + // Append the stack only when DEBUG / MCP_DEBUG is *meaningfully* set — "0" / + // "false" / empty read as off, so a stray DEBUG=0 doesn't force a stack. + // (Duplicated from the launcher's `wantsDebugStack` — the two bins can't + // import each other; keep them in sync if this logic changes.) + const debugOn = (v: string | undefined): boolean => { + const s = v?.trim().toLowerCase(); + return !!s && s !== "0" && s !== "false"; + }; + if ( + (debugOn(process.env.MCP_DEBUG) || debugOn(process.env.DEBUG)) && + err instanceof Error + ) { + console.error(err.stack); + } process.exit(1); }); } diff --git a/clients/tui/tui.tsx b/clients/tui/tui.tsx index aac8dd25c..3169a7901 100644 --- a/clients/tui/tui.tsx +++ b/clients/tui/tui.tsx @@ -59,7 +59,7 @@ export async function runTui(args?: string[]): Promise { ) .option( "--callback-url ", - `OAuth redirect/callback listener URL (default: ${DEFAULT_RUNNER_OAUTH_CALLBACK_URL}, or MCP_OAUTH_CALLBACK_URL)`, + `OAuth redirect/callback listener URL; must be loopback (default: ${DEFAULT_RUNNER_OAUTH_CALLBACK_URL}, or MCP_OAUTH_CALLBACK_URL)`, ) .argument( "[target...]", diff --git a/clients/web/README.md b/clients/web/README.md index 6fb04b682..c835aaf27 100644 --- a/clients/web/README.md +++ b/clients/web/README.md @@ -16,7 +16,8 @@ The `server/` directory holds the Node-only backend: - **`vite-hono-plugin.ts`** — mounts the Hono `/api/*` middleware onto the Vite dev server (so `npm run dev` has a live backend). - **`server.ts`** — the standalone Hono production server (serves `dist/` + `/api/*`). - **`run-web.ts`** / **`start-vite-dev-server.ts`** — entry points the launcher calls for prod `--web` and `--web --dev`. -- **`web-server-config.ts`** — env parsing, the `GET /api/config` payload, the startup banner. +- **`web-server-config.ts`** — env parsing, the `GET /api/config` payload, the startup banner, the default origin allow-list. +- **`resolve-bind-host.ts`** — the shared bind-host guard (refuses an all-interfaces `HOST` unless `DANGEROUSLY_BIND_ALL_INTERFACES`), used by both bind points (`web-server-config.ts` + `vite.config.ts`); see [Host binding & the origin allow-list](#host-binding--the-origin-allow-list). - **`inject-auth-token.ts`** — embeds the API token into the served `index.html` (see [Auth token](#auth-token)). - **`sandbox-controller.ts`** — the MCP Apps sandbox HTTP server; **`ensure-web-build.ts`** — builds `dist/` on demand for prod `--web`; **`vite-base-config.ts`** — shared `optimizeDeps` exclusions. - **`browser-externalized-builtin-gate.ts`** — Vite-agnostic build-gate logic that fails `vite build` when a Node built-in reaches the browser bundle (#1769); the thin Vite plugin wiring lives in `vite.config.ts`. It sits under `server/` (rather than `src/`) as the home for Node-only, build-time tooling — it's imported by the Vite config, never by the browser — alongside the other `vite-*` config helpers here. @@ -159,6 +160,26 @@ Storybook is first-class here because the components are presentational — each The dev/prod backend guards every `/api/*` route with `x-mcp-remote-auth: Bearer `. The browser recovers the token, in priority order (see `App.tsx` `getAuthToken()`): the `window.__INSPECTOR_API_TOKEN__` global injected into `index.html` on every page load (`server/inject-auth-token.ts`), then a `?MCP_INSPECTOR_API_TOKEN=…` query param, then `sessionStorage`. Injection is a no-op when auth is disabled (`DANGEROUSLY_OMIT_AUTH`). See the root [AGENTS.md](../../AGENTS.md) for the full rationale. +## Host binding & the origin allow-list + +Both the prod backend (`server/web-server-config.ts`) and the dev Vite server (`vite.config.ts`) resolve their bind host through one shared guard, `server/resolve-bind-host.ts`. It binds `localhost` by default and **refuses an all-interfaces host** (`0.0.0.0`, `::`, empty, or any equivalent spelling — `0`, `0x0`, `0.0`, `::0`, `::ffff:0.0.0.0`, … are all folded to the wildcard and refused) — which would expose the process-spawning backend to the whole network, the exposure DNS-rebinding attacks target — unless `DANGEROUSLY_BIND_ALL_INTERFACES=true` is set. The Docker image sets that flag (a container must bind `0.0.0.0` to be reachable through `-p`); a bare `HOST=0.0.0.0` anywhere else exits with an actionable error. + +The backend's `/api/*` routes also enforce an **origin allow-list** (`allowedOrigins`) as DNS-rebinding protection. When left to default on a loopback host, it expands to all three interchangeable loopback origin forms for the port — `http://localhost:PORT`, `http://127.0.0.1:PORT`, and `http://[::1]:PORT` — because `localhost` resolves to either IPv4 or IPv6 loopback and Node/Vite may bind the IPv6 form, so the browser can legitimately arrive at `http://[::1]:PORT`. Set `ALLOWED_ORIGINS` (comma-separated) to override; entries are canonicalized (`new URL(o).origin`), so a trailing slash / uppercase host / explicit `:80` still match. **Each entry must include the scheme** — `http://localhost:6274`, not `localhost:6274` (a scheme-less value is dropped with a warning). `ALLOWED_ORIGINS` **replaces** the default list (it does not merge), so **list every origin you'll browse from, including the loopback forms** you still want (`http://localhost:PORT`, `http://127.0.0.1:PORT`, `http://[::1]:PORT`) — otherwise local access stops working. A blank `ALLOWED_ORIGINS` does **not** disable the check — it falls back to the default (fail closed); there is no env knob to turn origin validation off. + +### Hosting on a network + +The guard blocks only the **wildcard** all-interfaces addresses. Binding a **specific** IP or hostname is allowed with no opt-in — that's a single, deliberate exposure, unlike the wildcard which binds every interface at once (the pattern DNS-rebinding exploits). To serve the Inspector on a LAN or the internet: + +- **Bind a specific address.** `HOST=192.168.1.50` (a LAN IP) or a public IP works directly: the default origin allow-list follows the bind host, so `allowedOrigins` becomes `http://:PORT` and a browser hitting that address is accepted with no extra config. (The host is canonicalized the way a browser is — so `HOST=127.1` advertises `http://127.0.0.1:PORT`, an IPv6 bind host is bracketed as `http://[2001:db8::1]:PORT`, and the port is dropped when it's the http default `:80` — matching what the browser sends.) +- **Behind TLS or a reverse proxy**, the browser's `Origin` becomes the public origin (e.g. `https://inspector.example.com`, often without a port), which won't match the auto-derived `http://:PORT`. Set `ALLOWED_ORIGINS` to the real public origin(s): `ALLOWED_ORIGINS=https://inspector.example.com`. +- **Using the `0.0.0.0` wildcard** (opt-in via `DANGEROUSLY_BIND_ALL_INTERFACES=true`, as the Docker image does): a wildcard bind also serves loopback, so the default allow-list is the loopback trio plus the canonical wildcard origins (`http://0.0.0.0:PORT`, `http://[::]:PORT`), and **local access works out of the box** — `docker run -p 6274:6274` browsed at `http://localhost:6274` connects with no extra config. Reaching it at a **non-loopback** address (a LAN IP, a public hostname) still needs `ALLOWED_ORIGINS` — but since that **replaces** the default, keep the loopback forms in the list if you also browse locally: `ALLOWED_ORIGINS=http://localhost:PORT,http://127.0.0.1:PORT,http://192.168.1.50:PORT,https://inspector.example.com`. + +The bind-host guard and the `ALLOWED_ORIGINS` allow-list apply to both the prod server and `--dev`. Note that in **`--dev`** the Vite dev server _additionally_ enforces its own `server.allowedHosts` Host-header check, whose default accepts loopback and IP-literal hosts. The host you **bind** is auto-allowed (Vite adds the resolved `server.host` — which this config sets from `HOST` — to the allow-list), so `HOST=` works out of the box under `--dev` too. What needs an explicit `server.allowedHosts` entry is reaching the dev server at a **different** name than the one bound — e.g. a wildcard bind reached by hostname, or a reverse-proxy domain. For those, prefer the prod server (`mcp-inspector --web`) or add the host to `server.allowedHosts`. + +**MCP Apps caveats.** The MCP Apps sandbox runs on a **separate** port (`MCP_SANDBOX_PORT`, dynamic by default). For the Apps tab to work off loopback, that sandbox port must be independently reachable from the browser — pin it with `MCP_SANDBOX_PORT` and expose/forward it too (the Docker image `EXPOSE`s only `6274`). (Under a `0.0.0.0` wildcard bind the sandbox URL is advertised as `localhost`, which is reachable — a wildcard bind serves loopback — so only the port needs handling.) Also note the sandbox iframe is gated by a `frame-ancestors` CSP, and **a bracketed IPv6 literal is not a valid CSP host-source** — so MCP Apps requires browsing the app at a name or IPv4 (`localhost`, `127.0.0.1`, a hostname, a LAN IPv4), **not** a bare `http://[::1]:…` address. Finally, the sandbox URL is always `http://` — so **behind TLS** (an `https://` app page) the browser blocks the `http://…/sandbox` iframe as mixed content and MCP Apps can't render; the Apps tab needs a plain-`http` app origin today. + +In every case, exposing the Inspector beyond loopback also means anyone who can reach it can drive its backend — keep authentication on (do **not** set `DANGEROUSLY_OMIT_AUTH`) and prefer a specific bind address over the wildcard. + ## HTTP proxy support The web backend connects to remote MCP servers through the shared Node transport (`core/mcp/node/transport.ts`), which honors the conventional proxy environment variables: `HTTPS_PROXY` / `HTTP_PROXY` (and their lowercase forms) select the proxy, and `NO_PROXY` exempts hosts. Routing is powered by [`undici`](https://www.npmjs.com/package/undici)'s `EnvHttpProxyAgent`, imported lazily only when a proxy variable is set, so runs without a proxy configured pay no cost. See the CLI README for more detail. diff --git a/clients/web/server/resolve-bind-host.ts b/clients/web/server/resolve-bind-host.ts new file mode 100644 index 000000000..a6d6fda1e --- /dev/null +++ b/clients/web/server/resolve-bind-host.ts @@ -0,0 +1,67 @@ +/** + * Resolves and validates the hostname the web server (prod backend + Vite dev + * server) binds to. Enforces the localhost-only default so the Inspector can't + * accidentally expose its process-spawning proxy to the whole network. + * + * Shared by `web-server-config.ts` (the Node backend) and `vite.config.ts` (the + * dev server) so both bind points enforce the same policy. + */ + +import { + canonicalUrlHost, + isAllInterfacesHost, + stripBrackets, +} from "../../../core/node/hostUrl.ts"; + +/** Env var that opts into binding all network interfaces (see {@link resolveBindHostname}). */ +export const BIND_ALL_INTERFACES_ENV = "DANGEROUSLY_BIND_ALL_INTERFACES"; + +/** + * An explicit, unambiguous opt-in. Unlike a bare `!!value` (which treats the + * string `"false"` as truthy), only `"true"`/`"1"` (case-insensitive) enable + * the override, so `DANGEROUSLY_BIND_ALL_INTERFACES=false` reads as "off". + */ +function isEnabled(value: string | undefined): boolean { + const v = value?.trim().toLowerCase(); + return v === "true" || v === "1"; +} + +/** + * Resolve the bind hostname from `env` (default `process.env`), defaulting to + * `localhost`. Refuses an all-interfaces host (`0.0.0.0` / `::` / empty / their + * legacy spellings) unless {@link BIND_ALL_INTERFACES_ENV} is explicitly + * enabled — the published Docker image sets it, since a container must bind + * `0.0.0.0` to be reachable through `-p`. Throws (fail fast, loudly) rather than + * silently binding wide open. The returned value is trimmed and de-bracketed + * (an IPv6 literal is returned bare, e.g. `HOST=[::1]` → `::1`) so detection, + * `listen()`, and the origin list all consume the same value; `formatHostForUrl` + * re-adds the brackets wherever a URL is built. + */ +export function resolveBindHostname( + env: NodeJS.ProcessEnv = process.env, +): string { + const host = (env.HOST ?? "localhost").trim(); + if (isAllInterfacesHost(host) && !isEnabled(env[BIND_ALL_INTERFACES_ENV])) { + // Show the resolved address when it differs from the typed spelling — the + // guard now catches forms the resolver folds to the wildcard (a fullwidth + // `HOST="０"` renders like `0`, `HOST=0` / `0x0` / `::0` bind `0.0.0.0`), and + // "bind a loopback host" reads as a non-sequitur without that hint. + const resolved = canonicalUrlHost(host); + // Compare de-bracketed on BOTH sides so neither a plain `HOST="::"` nor a + // bracketed `HOST="[::]"` is reported as "(resolves to [::])" — the + // bracketing is a URL-authority detail, not a resolution. + const shown = + stripBrackets(resolved) === stripBrackets(host.toLowerCase()) + ? `HOST="${host}"` + : `HOST="${host}" (resolves to ${resolved})`; + throw new Error( + `Refusing to bind ${shown}: this exposes the MCP Inspector to your ` + + `entire network, and its backend can spawn local processes and connect ` + + `to MCP servers on your behalf — the exposure DNS-rebinding attacks ` + + `target. Bind a loopback host (localhost / 127.0.0.1) instead. To ` + + `override — only inside an isolated container or trusted network — set ` + + `${BIND_ALL_INTERFACES_ENV}=true.`, + ); + } + return stripBrackets(host); +} diff --git a/clients/web/server/run-web.ts b/clients/web/server/run-web.ts index e98ed9b8f..f1ed5a11f 100644 --- a/clients/web/server/run-web.ts +++ b/clients/web/server/run-web.ts @@ -18,7 +18,10 @@ import { parseHeaderPair, type ServerConfigOptions, } from "../../../core/mcp/node/config.ts"; -import { buildWebServerConfig } from "./web-server-config.js"; +import { + buildWebServerConfig, + type WebServerConfig, +} from "./web-server-config.js"; import { startViteDevServer } from "./start-vite-dev-server.js"; import { startHonoServer } from "./server.js"; import { ensureWebBuild } from "./ensure-web-build.js"; @@ -232,12 +235,25 @@ export async function runWeb(argv: string[]): Promise { process.exit(1); } - const webConfig = buildWebServerConfig({ - initialMcpConfig, - mcpConfigPath, - writable, - initialServers, - }); + let webConfig: WebServerConfig; + try { + webConfig = buildWebServerConfig({ + initialMcpConfig, + mcpConfigPath, + writable, + initialServers, + }); + } catch (err) { + // e.g. the bind-host guard refusing HOST=0.0.0.0 — surface the actionable + // message rather than a raw stack trace from the launcher's top-level handler. + const message = + err instanceof Error + ? err.message + : /* v8 ignore next -- buildWebServerConfig only ever throws Error instances; this fallback is a defensive guard for the impossible non-Error throw. */ + "Invalid web server configuration."; + console.error("Error:", message); + process.exit(1); + } const webRoot = join(__dirname, ".."); const distRoot = join(webRoot, "dist"); if (!isDev) { diff --git a/clients/web/server/sandbox-controller.ts b/clients/web/server/sandbox-controller.ts index 107e87f7b..e4b0d53e1 100644 --- a/clients/web/server/sandbox-controller.ts +++ b/clients/web/server/sandbox-controller.ts @@ -7,6 +7,10 @@ import { createServer, type Server } from "node:http"; import { readFileSync } from "node:fs"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; +import { + canonicalUrlHost, + isAllInterfacesHost, +} from "../../../core/node/hostUrl.ts"; const __dirname = dirname(fileURLToPath(import.meta.url)); @@ -15,6 +19,59 @@ export interface SandboxControllerOptions { port: number; /** Host to bind (default localhost). */ host?: string; + /** + * The backend's origin allow-list (the embedder origins). Used to build the + * proxy's `frame-ancestors` so the sandbox iframe isn't CSP-blocked when the + * inspector is served on a non-loopback host (network hosting / Docker). The + * real callers always pass a non-empty list (`buildWebServerConfig` never + * produces an empty `allowedOrigins` since #1795's H1 fix); an empty/omitted + * list (only a hand-constructed caller or a test) falls back to loopback-only. + */ + allowedOrigins?: string[]; +} + +// Loopback fallback. NB: no `http://[::1]:*` — a **bracketed IPv6 literal is not +// a valid CSP `host-source`** (the CSP3 `host-char` grammar is ALPHA/DIGIT/"-" +// only). Chromium rejects `http://[::1]:*` and, if it's the sole source, the +// directive degrades to `frame-ancestors 'none'` and blocks the frame — verified +// empirically. So MCP Apps requires browsing the app at a name or IPv4 +// (`localhost` / `127.0.0.1`), which these two cover; a bare `[::1]` embedder +// can't be admitted by any frame-ancestors source and is unsupported for Apps. +const LOOPBACK_FRAME_ANCESTORS = ["http://127.0.0.1:*", "http://localhost:*"]; + +/** + * A well-formed CSP host-source: `scheme://host[:port]` with no whitespace, CSP + * metacharacters, or brackets. `allowedOrigins` comes from + * `web-server-config.ts`, where every entry is already a `new URL().origin` + * (canonical, no path/newline/`;`), so its **load-bearing job today is dropping + * bracketed IPv6** — a `http://[::1]:PORT` literal isn't a valid CSP host-source + * and, as the only source, would collapse the directive to `'none'`. The + * whitespace/metacharacter exclusions are belt-and-braces: `sandboxFrameAncestors` + * takes the list from a caller, not from env directly, so a future caller that + * doesn't pre-canonicalize can't corrupt or widen the header — `*` is excluded + * too, so a `http://*.example.com` entry (which the origin allow-list already + * rejects, but a future caller might not) can't broaden the embedder set. This + * filters the caller-supplied `allowedOrigins` only; {@link LOOPBACK_FRAME_ANCESTORS} + * is a trusted constant that deliberately uses the `:*` port-wildcard and is + * emitted as-is (it does NOT pass this regex, by design). + */ +const CSP_HOST_SOURCE = /^[a-z][a-z0-9+.-]*:\/\/[^\s;,'"[\]*]+$/i; + +/** + * The proxy's `frame-ancestors` sources. The embedder is the inspector app + * page, whose origin is (by construction) in the backend's `allowedOrigins`, so + * derive the directive from that list — this keeps the MCP Apps iframe working + * on whatever host the app is actually served from, not just loopback. + * Malformed entries are dropped (see {@link CSP_HOST_SOURCE}); if nothing valid + * remains (the real backend always passes a non-empty list, so this is a + * hand-constructed caller, a test, or an all-malformed `ALLOWED_ORIGINS`), it + * falls back to the loopback family so the sandbox still loads locally and the + * header can't be corrupted. + */ +export function sandboxFrameAncestors(allowedOrigins?: string[]): string { + const valid = (allowedOrigins ?? []).filter((o) => CSP_HOST_SOURCE.test(o)); + const sources = valid.length > 0 ? valid : LOOPBACK_FRAME_ANCESTORS; + return `frame-ancestors ${sources.join(" ")}`; } export interface SandboxController { @@ -23,27 +80,40 @@ export interface SandboxController { getUrl(): string | null; } +/** + * A usable listen port from a raw env value, or `undefined` if it isn't a plain + * integer in `0`–`65535` (0 = OS-assigned). The `^\d+$` test rejects `parseInt` + * partial parses (`6274abc`) and the upper bound keeps an out-of-range value + * (`70000`) from reaching `server.listen`, which throws `ERR_SOCKET_BAD_PORT` + * synchronously — matching the strict `CLIENT_PORT` validation. + */ +function parseListenPort(raw: string | undefined): number | undefined { + const v = raw?.trim(); + if (!v || !/^\d+$/.test(v)) return undefined; + const n = parseInt(v, 10); + return n <= 65535 ? n : undefined; +} + /** * Resolve sandbox port from env: MCP_SANDBOX_PORT → SERVER_PORT → 0 (dynamic). + * An invalid value falls through rather than crashing the boot; a set-but-invalid + * MCP_SANDBOX_PORT (the dedicated knob) is warned so the fall-through isn't + * silent — matching the warn-and-drop precedent for ALLOWED_ORIGINS. */ export function resolveSandboxPort(): number { - const fromSandbox = process.env.MCP_SANDBOX_PORT; - if (fromSandbox !== undefined && fromSandbox !== "") { - const n = parseInt(fromSandbox, 10); - if (!Number.isNaN(n) && n >= 0) return n; - } - const fromServer = process.env.SERVER_PORT; - if (fromServer !== undefined && fromServer !== "") { - const n = parseInt(fromServer, 10); - if (!Number.isNaN(n) && n >= 0) return n; + const fromSandbox = parseListenPort(process.env.MCP_SANDBOX_PORT); + if (fromSandbox === undefined && process.env.MCP_SANDBOX_PORT?.trim()) { + console.warn( + `Ignoring invalid MCP_SANDBOX_PORT="${process.env.MCP_SANDBOX_PORT}" (need an integer 0–65535); falling back.`, + ); } - return 0; + return fromSandbox ?? parseListenPort(process.env.SERVER_PORT) ?? 0; } export function createSandboxController( options: SandboxControllerOptions, ): SandboxController { - const { port, host = "localhost" } = options; + const { port, host = "localhost", allowedOrigins } = options; let server: Server | null = null; let sandboxUrl: string | null = null; @@ -54,10 +124,10 @@ export function createSandboxController( // the inner app document and, since multiple CSPs intersect, would override // the per-app `connect-src`/`img-src` allowlists the host bakes into the // wrapped HTML (see src/utils/sandbox-csp.ts). The opaque-origin sandbox on - // the inner frame is the structural boundary; `frame-ancestors` ensures the - // proxy can only be embedded by the local inspector itself. - const SANDBOX_PROXY_CSP = - "frame-ancestors http://127.0.0.1:* http://localhost:*"; + // the inner frame is the structural boundary; `frame-ancestors` restricts the + // proxy to being embedded by the inspector app itself — see + // `sandboxFrameAncestors` for how the embedder origins are derived. + const SANDBOX_PROXY_CSP = sandboxFrameAncestors(allowedOrigins); let sandboxHtml: string; try { @@ -132,7 +202,19 @@ export function createSandboxController( string form only occurs for unix-socket/pipe listens, which this controller never performs. */ (addr as unknown as number); - sandboxUrl = `http://${host}:${actualPort}/sandbox`; + // Advertise `localhost` for a wildcard bind: the sandbox URL is + // served to the browser (via /api/config) and printed in the banner, + // and `http://0.0.0.0:PORT` isn't reachable from the client — but a + // wildcard bind serves loopback, so `localhost` is. Otherwise use the + // same canonical host the origin allow-list emits, so the served URL + // is reachable and its origin is allow-listed (matches the app banner). + // (`isAllInterfacesHost` canonicalizes internally too, so passing the + // pre-canonicalized host is belt-and-braces.) + const canonicalHost = canonicalUrlHost(host); + const urlHost = isAllInterfacesHost(canonicalHost) + ? "localhost" + : canonicalHost; + sandboxUrl = `http://${urlHost}:${actualPort}/sandbox`; settle({ port: actualPort, url: sandboxUrl }); }); }); diff --git a/clients/web/server/server.ts b/clients/web/server/server.ts index 3bfde7bf1..9f6ff1347 100644 --- a/clients/web/server/server.ts +++ b/clients/web/server/server.ts @@ -15,6 +15,7 @@ import { serveStatic } from "@hono/node-server/serve-static"; import { Hono } from "hono"; import type { Context } from "hono"; import { createRemoteApp } from "../../../core/mcp/remote/node/server.ts"; +import { formatHostForUrl } from "../../../core/node/hostUrl.ts"; import { createSandboxController } from "./sandbox-controller.js"; import { injectAuthToken } from "./inject-auth-token.js"; import type { WebServerConfig } from "./web-server-config.js"; @@ -39,6 +40,7 @@ export async function startHonoServer( const sandboxController = createSandboxController({ port: config.sandboxPort, host: config.sandboxHost, + allowedOrigins: config.allowedOrigins, }); await sandboxController.start(); @@ -131,7 +133,7 @@ export async function startHonoServer( httpServer.on("error", (err: Error) => { if (err.message.includes("EADDRINUSE")) { console.error( - `MCP Inspector PORT IS IN USE at http://${config.hostname}:${config.port}`, + `MCP Inspector PORT IS IN USE at http://${formatHostForUrl(config.hostname)}:${config.port}`, ); process.exit(1); } else { diff --git a/clients/web/server/start-vite-dev-server.ts b/clients/web/server/start-vite-dev-server.ts index 7037a85cb..91a7fab70 100644 --- a/clients/web/server/start-vite-dev-server.ts +++ b/clients/web/server/start-vite-dev-server.ts @@ -57,6 +57,12 @@ export async function startViteDevServer( server: { port: config.port, host: config.hostname, + // `strictPort: true` (matching `vite.config.ts`) so a busy port fails + // loudly instead of silently binding a different one — the origin + // allow-list and the sandbox `frame-ancestors` are derived from + // `config.port`, so a drifted bind would 403 every connect and CSP-block + // the MCP Apps iframe while the banner advertises the unusable port. + strictPort: true, // Allow Vite to serve source files from the repo root (core/ lives // outside clients/web), matching `vite.config.ts`'s `server.fs.allow`. fs: { diff --git a/clients/web/server/vite-hono-plugin.ts b/clients/web/server/vite-hono-plugin.ts index 67346b428..1331930ce 100644 --- a/clients/web/server/vite-hono-plugin.ts +++ b/clients/web/server/vite-hono-plugin.ts @@ -63,6 +63,7 @@ export function honoMiddlewarePlugin(config: WebServerConfig): Plugin { const sandboxController = createSandboxController({ port: config.sandboxPort, host: config.sandboxHost, + allowedOrigins: config.allowedOrigins, }); await sandboxController.start(); diff --git a/clients/web/server/web-server-config.ts b/clients/web/server/web-server-config.ts index d40a18ac9..6e77bd28e 100644 --- a/clients/web/server/web-server-config.ts +++ b/clients/web/server/web-server-config.ts @@ -16,6 +16,11 @@ import { import type { InitialConfigPayload } from "../../../core/mcp/remote/node/server.ts"; import { readInspectorVersionSafe } from "../../../core/node/version.ts"; import { resolveSandboxPort } from "./sandbox-controller.js"; +import { resolveBindHostname } from "./resolve-bind-host.js"; +import { + canonicalUrlHost, + isAllInterfacesHost, +} from "../../../core/node/hostUrl.ts"; // The single-source Inspector version (root package.json), read once at load. // The browser can't read the filesystem the way the CLI/TUI do, so the backend @@ -163,7 +168,19 @@ export function printServerBanner( resolvedToken: string, sandboxUrl: string | undefined, ): string { - const baseUrl = `http://${config.hostname}:${actualPort}`; + // Advertise `localhost` for a wildcard bind (`http://0.0.0.0:PORT` is an + // awkward URL to click and points the user at a reachable, allow-listed + // address); otherwise use the SAME canonical host the allow-list emits, so the + // advertised URL's origin is always a member of `allowedOrigins` + // (`banner ⊆ allowedOrigins`) — including the IPv4-mapped case where + // `canonicalUrlHost` unmaps but `formatHostForUrl` wouldn't. `httpOrigin` + // keeps the banner and the list agreeing on the default-port form (both drop + // :80). Computing `h` once keeps the predicate and the value reading the same + // form; `isAllInterfacesHost` also canonicalizes internally, so this is + // belt-and-braces, not what makes them agree. + const h = canonicalUrlHost(config.hostname); + const bannerHost = isAllInterfacesHost(h) ? "localhost" : h; + const baseUrl = httpOrigin(bannerHost, actualPort); const url = config.dangerouslyOmitAuth || !resolvedToken ? baseUrl @@ -196,6 +213,87 @@ export interface BuildWebServerConfigOptions { initialServers?: MCPConfig | null; } +/** + * Loopback hostnames that all address the local machine. `localhost` resolves + * to *either* `127.0.0.1` (IPv4) or `::1` (IPv6) depending on the OS resolver, + * and Node/Vite may bind the IPv6 form — so the browser can legitimately end up + * at `http://[::1]:PORT` even though the banner printed `http://localhost:PORT`. + * IPv6 is the **bracketed** form only (`[::1]`): the sole caller looks up + * `canonicalUrlHost(hostname)`, which always brackets an IPv6 literal. + */ +const LOOPBACK_HOSTNAMES = new Set(["localhost", "127.0.0.1", "[::1]"]); + +/** + * Build an http origin string. The port is omitted when it's the http scheme + * default (80): browsers drop the default port from `Origin`, and the guard is + * an exact string match, so `http://host:80` could never match a real request. + */ +function httpOrigin(host: string, port: number): string { + return port === 80 ? `http://${host}` : `http://${host}:${port}`; +} + +/** The three interchangeable loopback origin forms for a port. */ +function loopbackOrigins(port: number): string[] { + return [ + httpOrigin("localhost", port), + httpOrigin("127.0.0.1", port), + httpOrigin("[::1]", port), + ]; +} + +/** + * The default allowed-origins list for a given bind host/port. + * + * When the bind host is loopback, `localhost`, `127.0.0.1`, and `[::1]` are + * interchangeable ways to reach the same server, so the origin the browser + * actually sends is nondeterministic (it depends on how `localhost` resolved + * and which family got bound). Returning all three loopback origin forms keeps + * the DNS-rebinding guard effective — still scoped to loopback at this exact + * port — while not 403-ing a browser that landed on `[::1]` instead of the + * `localhost` the banner advertised. The host is canonicalized first + * ({@link canonicalUrlHost}), so a non-canonical spelling of a loopback address + * still lands in the loopback branch. + * + * An **all-interfaces** bind (`0.0.0.0` / `::`, the opt-in path the Docker image + * uses) also serves loopback, so it gets the loopback trio *plus the canonical + * wildcard origins* `http://0.0.0.0:PORT` and `http://[::]:PORT`. Both are + * locally connectable, the image/docs describe binding to `0.0.0.0:6274`, and a + * dual-stack `::` bind serves IPv4 too — so a browser may send either. We emit + * the **canonical** pair rather than the typed `HOST` spelling because browsers + * canonicalize the address before building the `Origin` (`HOST=0` / `0x0` / + * `0.0.0` all send `http://0.0.0.0:PORT`, `::0` sends `http://[::]:PORT`), so + * echoing the raw spelling would emit an entry the browser can never match. + * Including these weakens nothing (no attacker document can claim a loopback / + * `0.0.0.0` origin without the user having navigated there). Reaching the server + * at a non-loopback address — a LAN IP, a public hostname — still needs + * `ALLOWED_ORIGINS`, since those origins can't be enumerated from the wildcard. + * + * For a specific non-loopback host (a real IP/hostname) we return the single + * exact origin — canonicalized and, for an IPv6 literal, bracketed, so it + * matches the `Origin` header the browser actually sends. + */ +export function defaultAllowedOrigins( + hostname: string, + port: number, +): string[] { + // The loopback lookup and the emitted origin both read the canonicalized `h` + // (the wildcard check via `isAllInterfacesHost` canonicalizes internally), so + // every branch reasons about the same address. + const h = canonicalUrlHost(hostname); + if (LOOPBACK_HOSTNAMES.has(h)) { + return loopbackOrigins(port); + } + if (isAllInterfacesHost(h)) { + return [ + ...loopbackOrigins(port), + httpOrigin("0.0.0.0", port), + httpOrigin("[::]", port), + ]; + } + // `h` is already the canonical, URL-ready host (bracketed for IPv6). + return [httpOrigin(h, port)]; +} + /** * Build WebServerConfig from process.env and optional initial MCP server config. * Used by the launcher runner, Vite dev (`buildWebServerConfigFromEnv`), and prod standalone. @@ -209,9 +307,21 @@ export function buildWebServerConfig( writable = true, initialServers = null, } = options; - const port = parseInt(process.env.CLIENT_PORT ?? "6274", 10); - const hostname = process.env.HOST ?? "localhost"; - const baseUrl = `http://${hostname}:${port}`; + // Treat an empty CLIENT_PORT as unset (matches resolveSandboxPort's handling + // of MCP_SANDBOX_PORT / SERVER_PORT), then require a fixed port: the origin + // allow-list and the sandbox CSP are both built from it, so `0` (OS-assigned), + // a non-numeric value, or trailing garbage would make them reference a port + // the server isn't on — every connect 403s and the MCP Apps iframe is blocked. + // The `^\d+$` test rejects `parseInt`'s partial parses (`6274abc`, `80.9`). + // Fail fast with an actionable message (run-web surfaces it cleanly). + const rawPort = process.env.CLIENT_PORT?.trim() || "6274"; + const port = parseInt(rawPort, 10); + if (!/^\d+$/.test(rawPort) || port < 1 || port > 65535) { + throw new Error( + `Invalid CLIENT_PORT="${process.env.CLIENT_PORT}": the web server needs a fixed port in 1–65535 (0 / dynamic is unsupported — the origin allow-list and sandbox CSP are derived from it).`, + ); + } + const hostname = resolveBindHostname(); const dangerouslyOmitAuth = !!process.env.DANGEROUSLY_OMIT_AUTH; const authToken = dangerouslyOmitAuth ? "" @@ -233,6 +343,48 @@ export function buildWebServerConfig( ); } + // Parse ALLOWED_ORIGINS into canonical origins. Each entry is normalized via + // `new URL(o).origin` (drops a trailing slash/path and any userinfo, lowercases + // and punycodes the host, drops the default :80) so the natural copy-paste + // forms — `http://localhost:6274/` from the address bar, an uppercase or IDN + // host, an explicit `:80` — match the canonical `Origin` the browser sends + // rather than 403ing on an exact-string compare. Unparseable, opaque, and + // wildcard entries are warned and dropped (see inline). + // + // When nothing survives (unset, `""`, `" "`, `","`, or all-invalid) we fall + // back to `defaultAllowedOrigins` below — critically NOT to `[]`, which the + // origin middleware treats as *allow-all*, silently disabling the guard. + const configuredOrigins = process.env.ALLOWED_ORIGINS?.split(",") + .map((o) => o.trim()) + .filter(Boolean) + .map((o) => { + try { + const { origin } = new URL(o); + // A scheme-less entry (`localhost:6274`) doesn't throw — `new URL` reads + // the host as the scheme and `.origin` is the literal string "null" + // (also non-special schemes: `file:`, `about:`, `javascript:`, `data:`, + // and browser-extension schemes like `chrome-extension:`). Reject it: + // it's not the origin the user meant, AND "null" is a real header value + // browsers send from opaque origins (a sandboxed iframe, a `data:` doc), + // so allow-listing it would erode the guard. Entries need an http(s) + // scheme — the app is served over http(s), and a browser's `Origin` on + // any request (including a WebSocket handshake) is its page's http(s) + // origin, never a `ws:` one, so a `ws://` entry could never match anyway. + if (origin === "null") throw new Error("opaque origin"); + // A wildcard (`http://*.example.com`) survives `new URL` but can never + // match the exact-compare origin guard — yet `*.example.com` IS a legal + // CSP host-source, so it would silently work for the sandbox iframe and + // silently 403 every connect (the most confusing split). Reject it so it + // fails loudly and consistently; list exact origins instead. + if (origin.includes("*")) throw new Error("wildcard origin"); + return origin; + } catch { + console.warn(`Ignoring invalid ALLOWED_ORIGINS entry: ${o}`); + return null; + } + }) + .filter((o): o is string => o !== null); + return { port, hostname, @@ -243,9 +395,9 @@ export function buildWebServerConfig( writable, initialServers, storageDir: process.env.MCP_STORAGE_DIR, - allowedOrigins: process.env.ALLOWED_ORIGINS?.split(",").filter(Boolean) ?? [ - baseUrl, - ], + allowedOrigins: configuredOrigins?.length + ? configuredOrigins + : defaultAllowedOrigins(hostname, port), sandboxPort, sandboxHost: hostname, logger, diff --git a/clients/web/src/test/core/auth/runner-oauth-callback.test.ts b/clients/web/src/test/core/auth/runner-oauth-callback.test.ts index 4836cbe3a..76f5d8a8d 100644 --- a/clients/web/src/test/core/auth/runner-oauth-callback.test.ts +++ b/clients/web/src/test/core/auth/runner-oauth-callback.test.ts @@ -76,6 +76,29 @@ describe("runner OAuth callback URL", () => { ).toThrow(/must use http scheme/); }); + it.each([ + "http://0.0.0.0:6276/oauth/callback", // wildcard + "http://[::]:6276/oauth/callback", // IPv6 wildcard + "http://192.168.1.50:6276/oauth/callback", // LAN — plaintext code over the network + "http://example.com:6276/oauth/callback", // routable hostname + ])( + "rejects a non-loopback callback host %j (the code listener must be loopback)", + (url) => { + expect(() => parseRunnerOAuthCallbackUrl(url)).toThrow( + /must bind a loopback host/, + ); + }, + ); + + it.each([ + ["http://127.0.0.1:6276/oauth/callback", "127.0.0.1"], + ["http://localhost:6276/oauth/callback", "localhost"], + ["http://127.5:6276/oauth/callback", "127.0.0.5"], // 127.0.0.0/8, shorthand normalized by new URL + ["http://[::1]:6276/oauth/callback", "::1"], // de-bracketed so listen() can bind it + ])("accepts the loopback callback host %j", (url, expectedHostname) => { + expect(parseRunnerOAuthCallbackUrl(url).hostname).toBe(expectedHostname); + }); + it("formatRunnerOAuthRedirectUrl round-trips default config", () => { const config = parseRunnerOAuthCallbackUrl(); expect(formatRunnerOAuthRedirectUrl(config)).toBe( diff --git a/clients/web/src/test/core/node/hostUrl.test.ts b/clients/web/src/test/core/node/hostUrl.test.ts new file mode 100644 index 000000000..baf12d226 --- /dev/null +++ b/clients/web/src/test/core/node/hostUrl.test.ts @@ -0,0 +1,164 @@ +import { describe, it, expect } from "vitest"; +import { + canonicalUrlHost, + formatHostForUrl, + isAllInterfacesHost, + isLoopbackHost, + stripBrackets, +} from "@inspector/core/node/hostUrl.js"; + +describe("isLoopbackHost", () => { + it.each([ + "localhost", + "LOCALHOST", + "127.0.0.1", + "127.5", // 127.0.0.0/8 shorthand + "0x7f.0.0.1", + "2130706433", + "::1", + "[::1]", + "0:0:0:0:0:0:0:1", + "::ffff:127.0.0.1", // IPv4-mapped loopback → unmapped to 127.0.0.1 + "::ffff:7f00:1", // its canonical serialization (what new URL().hostname yields) + "127.255.255.255", // top of 127.0.0.0/8 + "localhost.", // root-anchored FQDN (WHATWG keeps the dot; binds loopback) + "127.0.0.1.", // trailing dot on an IP literal (WHATWG strips it) + ])("flags the loopback host %j", (host) => { + expect(isLoopbackHost(host)).toBe(true); + }); + + it.each([ + "0.0.0.0", + "::", + "192.168.1.50", + "example.com", + "126.0.0.1", // adjacent to but outside 127/8 + "128.0.0.1", + "0.0.0.127", // bare "127" resolves here, not loopback + "", // empty is the wildcard (isAllInterfacesHost's job), not loopback + // Out-of-range octets survive canonicalUrlHost's non-URL fallback — the + // bounded regex must still reject them. + "127.999.0.1", + "127.0.0.256", + ])("does not flag the non-loopback host %j", (host) => { + expect(isLoopbackHost(host)).toBe(false); + }); +}); + +describe("isAllInterfacesHost", () => { + it.each([ + "0.0.0.0", + "::", + "", + " 0.0.0.0 ", + " :: ", + "[::]", + "0:0:0:0:0:0:0:0", + "::ffff:0.0.0.0", + "::ffff:0:0", + // IPv6 wildcard spellings that canonicalize to `::`. + "::0", + "0::0", + "::0.0.0.0", + "0:0::0", + "0000:0000:0000:0000:0000:0000:0000:0000", + // Zone-scoped wildcard: net.isIPv6 accepts the %zone, new URL() rejects it, + // so the zone must be stripped before canonicalizing (else it throws). + "::%eth0", + // Legacy inet_aton spellings the OS still binds as 0.0.0.0. + "0", + "0x0", + "0x0.0.0.0", + "000.000.000.000", + "0.0", // short inet_aton form (1–3 parts) still binds the wildcard + "0.0.0", + "00000000000", // bare octal zero — parseAddressPart's [0-9]+ handles octal + "0x00000000", // bare hex zero + // Fullwidth (IDNA-mapped) spellings the resolver folds to 0.0.0.0 before + // binding — the guard must canonicalize, not read the raw string. + "０", // fullwidth "0" + "０0.0.0", // fullwidth "0" + ".0.0" + "0．0．0．0", // fullwidth dots + ])("flags the all-interfaces host %j", (host) => { + expect(isAllInterfacesHost(host)).toBe(true); + }); + + it.each([ + "localhost", + "127.0.0.1", + "::1", + "[::1]", + "example.com", + "192.168.1.50", + "1.0.0.0", + "0.0.0.1", + "::ffff:0", // canonicalizes to ::ffff:0, a distinct address — not the wildcard + "0.0.0.0.0", // 5 octets — not a valid IPv4, must not be flagged (parts.length > 4) + "fe80::1%eth0", // a zone-scoped link-local — a real bind host, not the wildcard + "::1%lo0", // zone-scoped loopback — must not be flagged and must not throw + "１２７.0.0.1", // fullwidth 127 → 127.0.0.1, a real loopback address, not the wildcard + ])("does not flag the loopback/specific host %j", (host) => { + expect(isAllInterfacesHost(host)).toBe(false); + }); +}); + +describe("formatHostForUrl", () => { + it.each([ + ["::1", "[::1]"], + ["fe80::1", "[fe80::1]"], + [" ::1 ", "[::1]"], + ["fe80::1%eth0", "[fe80::1]"], // zone id dropped — a URL host can't carry one + ["::1%lo0", "[::1]"], + ["[fe80::1%eth0]", "[fe80::1]"], // bracketed-with-zone → still a valid URL host + ])("brackets the IPv6 literal %j", (host, expected) => { + expect(formatHostForUrl(host)).toBe(expected); + }); + + it.each(["localhost", "127.0.0.1", "192.168.1.50", "example.com", "[::1]"])( + "passes the non-IPv6 / already-bracketed host %j through", + (host) => { + expect(formatHostForUrl(host)).toBe(host.trim()); + }, + ); + + it("does not bracket a non-IPv6 value that merely contains a colon", () => { + // A mistyped host:port must not be wrapped as [host:port]. + expect(formatHostForUrl("localhost:6274")).toBe("localhost:6274"); + }); +}); + +describe("stripBrackets", () => { + it.each([ + ["[::1]", "::1"], + ["[fe80::1%eth0]", "fe80::1%eth0"], + ["[]", ""], // zero-or-more: an empty bracket pair reduces to "" + ["127.0.0.1", "127.0.0.1"], + ])("strips a surrounding bracket pair from %j", (host, expected) => { + expect(stripBrackets(host)).toBe(expected); + }); +}); + +describe("canonicalUrlHost", () => { + it.each([ + ["127.1", "127.0.0.1"], + ["0x7f.0.0.1", "127.0.0.1"], + ["2130706433", "127.0.0.1"], + ["0:0:0:0:0:0:0:1", "[::1]"], + ["::0001", "[::1]"], + ["LOCALHOST", "localhost"], + ["Example.COM", "example.com"], + ["fe80::1", "[fe80::1]"], + // IPv4-mapped IPv6 → the dotted IPv4 the socket answers on (intentional + // divergence from browser canonicalization). + ["::ffff:127.0.0.1", "127.0.0.1"], + ["::ffff:192.168.1.50", "192.168.1.50"], + // A distinct address is unchanged. + ["127.0.0.2", "127.0.0.2"], + ])("canonicalizes %j to %j", (host, expected) => { + expect(canonicalUrlHost(host)).toBe(expected); + }); + + it("falls back to the formatted value when the host isn't a parseable URL", () => { + expect(canonicalUrlHost("")).toBe(""); + }); +}); diff --git a/clients/web/src/test/integration/server/resolve-bind-host.test.ts b/clients/web/src/test/integration/server/resolve-bind-host.test.ts new file mode 100644 index 000000000..c6de0adab --- /dev/null +++ b/clients/web/src/test/integration/server/resolve-bind-host.test.ts @@ -0,0 +1,91 @@ +import { describe, it, expect } from "vitest"; +import { + BIND_ALL_INTERFACES_ENV, + resolveBindHostname, +} from "../../../../server/resolve-bind-host.js"; + +describe("resolveBindHostname", () => { + it("defaults to localhost when HOST is unset", () => { + expect(resolveBindHostname({})).toBe("localhost"); + }); + + it("returns a loopback HOST unchanged", () => { + expect(resolveBindHostname({ HOST: "127.0.0.1" })).toBe("127.0.0.1"); + }); + + it("trims the returned host so detection and the bind value agree", () => { + expect(resolveBindHostname({ HOST: " 127.0.0.1 " })).toBe("127.0.0.1"); + }); + + it("returns a bracketed IPv6 HOST bare so listen() can bind it", () => { + expect(resolveBindHostname({ HOST: "[::1]" })).toBe("::1"); + }); + + it("keeps the zone index on a link-local HOST for listen()", () => { + // The guard must not throw on a zone-scoped host, and must return it with + // the zone intact (listen() needs the zone to pick the interface). + expect(resolveBindHostname({ HOST: "fe80::1%eth0" })).toBe("fe80::1%eth0"); + }); + + it.each([ + "0.0.0.0", + "::", + "", + "0", + "0x0.0.0.0", + "::ffff:0.0.0.0", + " 0 ", + "０", // fullwidth "0" — the resolver binds it as 0.0.0.0, so the guard must refuse it + ])("refuses the all-interfaces host %j without the opt-in", (host) => { + expect(() => resolveBindHostname({ HOST: host })).toThrow( + new RegExp(BIND_ALL_INTERFACES_ENV), + ); + }); + + it("names the resolved wildcard address when the spelling differs (fullwidth)", () => { + // `HOST="０"` renders like `0`, so the message shows it resolves to 0.0.0.0. + expect(() => resolveBindHostname({ HOST: "０" })).toThrow( + /resolves to 0\.0\.0\.0/, + ); + }); + + it("does not add a resolved-address hint for an already-canonical bracketed HOST", () => { + // `HOST="[::]"` is accepted-then-refused as the wildcard; the message must + // not read "(resolves to [::])" — same value, just bracketed. Capture the + // message outside try/catch so a stopped-throwing regression reports clearly. + let message = ""; + expect(() => { + try { + resolveBindHostname({ HOST: "[::]" }); + } catch (err) { + message = (err as Error).message; + throw err; + } + }).toThrow(BIND_ALL_INTERFACES_ENV); + expect(message).not.toContain("resolves to"); + }); + + it.each(["true", "TRUE", "1", " true "])( + "allows 0.0.0.0 when the opt-in is %j", + (flag) => { + expect( + resolveBindHostname({ + HOST: "0.0.0.0", + [BIND_ALL_INTERFACES_ENV]: flag, + }), + ).toBe("0.0.0.0"); + }, + ); + + it.each(["false", "0", "", "yes", "no"])( + "still refuses 0.0.0.0 when the opt-in reads %j", + (flag) => { + expect(() => + resolveBindHostname({ + HOST: "0.0.0.0", + [BIND_ALL_INTERFACES_ENV]: flag, + }), + ).toThrow(); + }, + ); +}); diff --git a/clients/web/src/test/integration/server/run-web.test.ts b/clients/web/src/test/integration/server/run-web.test.ts index 28b291040..65bd23403 100644 --- a/clients/web/src/test/integration/server/run-web.test.ts +++ b/clients/web/src/test/integration/server/run-web.test.ts @@ -42,6 +42,7 @@ describe("runWeb", () => { let exitSpy: ReturnType; let exitCode: number | undefined; let catalogEnvSnapshot: string | undefined; + let hostEnvSnapshot: string | undefined; const signalHandlers = new Map void>(); beforeEach(() => { @@ -54,6 +55,8 @@ describe("runWeb", () => { exitCode = undefined; catalogEnvSnapshot = process.env.MCP_CATALOG_PATH; delete process.env.MCP_CATALOG_PATH; + hostEnvSnapshot = process.env.HOST; + delete process.env.HOST; logLines = []; warnLines = []; @@ -98,6 +101,11 @@ describe("runWeb", () => { } else { process.env.MCP_CATALOG_PATH = catalogEnvSnapshot; } + if (hostEnvSnapshot === undefined) { + delete process.env.HOST; + } else { + process.env.HOST = hostEnvSnapshot; + } }); async function expectServerStarted( @@ -162,6 +170,19 @@ describe("runWeb", () => { expect(startHonoServer).not.toHaveBeenCalled(); }); + it("surfaces the bind-guard error as an actionable message, not a stack trace", async () => { + // HOST=0.0.0.0 without the opt-in makes buildWebServerConfig throw; run-web + // must catch it and print "Error: …" rather than let it bubble as a raw + // stack trace from the launcher's top-level handler. + process.env.HOST = "0.0.0.0"; + await expect(runWeb(["node", "run-web"])).rejects.toThrow("process.exit:1"); + expect( + errorLines.some((l) => l.includes("DANGEROUSLY_BIND_ALL_INTERFACES")), + ).toBe(true); + expect(startHonoServer).not.toHaveBeenCalled(); + expect(startViteDevServer).not.toHaveBeenCalled(); + }); + it("starts Vite when --dev is set", async () => { void runWeb(["node", "run-web", "--dev"]); await expectServerStarted(startViteDevServer); 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 c4f73c47f..5db8d1b3f 100644 --- a/clients/web/src/test/integration/server/sandbox-controller.test.ts +++ b/clients/web/src/test/integration/server/sandbox-controller.test.ts @@ -3,10 +3,58 @@ import { createServer, type Server } from "node:http"; import { createSandboxController, resolveSandboxPort, + sandboxFrameAncestors, } from "../../../../server/sandbox-controller.js"; +describe("sandboxFrameAncestors", () => { + it("derives the directive from the provided allow-list", () => { + expect( + sandboxFrameAncestors([ + "http://192.168.1.50:6274", + "https://inspector.example.com", + ]), + ).toBe( + "frame-ancestors http://192.168.1.50:6274 https://inspector.example.com", + ); + }); + + it.each([[undefined], [[]]])( + "falls back to the loopback family when the list is %j", + (origins) => { + // No `[::1]` source — a bracketed IPv6 literal is not a valid CSP + // host-source (see sandbox-controller.ts). + expect(sandboxFrameAncestors(origins as string[] | undefined)).toBe( + "frame-ancestors http://127.0.0.1:* http://localhost:*", + ); + }, + ); + + it("drops malformed and IPv6-literal entries that can't be valid CSP sources", () => { + // A newline would make writeHead throw ERR_INVALID_CHAR; a ';' would inject + // extra directives; a bracketed IPv6 literal isn't a valid CSP host-source. + // Only the well-formed origin survives. + expect( + sandboxFrameAncestors([ + "http://good.example:6274", + "http://a:1; sandbox", + "http://b:2\nX-Evil: 1", + "http://[::1]:6274", + "http://*.example.com", // a wildcard would widen the embedder set + "not a url", + ]), + ).toBe("frame-ancestors http://good.example:6274"); + }); + + it("falls back to loopback when every entry is malformed or IPv6-literal", () => { + expect( + sandboxFrameAncestors(["http://a:1; sandbox", "http://[::1]:6274"]), + ).toBe("frame-ancestors http://127.0.0.1:* http://localhost:*"); + }); +}); + describe("resolveSandboxPort", () => { let envSnapshot: { mcp?: string; server?: string }; + let warnSpy: ReturnType; beforeEach(() => { envSnapshot = { @@ -15,9 +63,12 @@ describe("resolveSandboxPort", () => { }; delete process.env.MCP_SANDBOX_PORT; delete process.env.SERVER_PORT; + // A set-but-invalid MCP_SANDBOX_PORT warns; keep it off the test console. + warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); }); afterEach(() => { + warnSpy.mockRestore(); if (envSnapshot.mcp === undefined) delete process.env.MCP_SANDBOX_PORT; else process.env.MCP_SANDBOX_PORT = envSnapshot.mcp; if (envSnapshot.server === undefined) delete process.env.SERVER_PORT; @@ -65,6 +116,17 @@ describe("resolveSandboxPort", () => { process.env.MCP_SANDBOX_PORT = "-1"; expect(resolveSandboxPort()).toBe(0); }); + + it("rejects a partial-parse value (6274abc) rather than binding 6274", () => { + process.env.MCP_SANDBOX_PORT = "6274abc"; + process.env.SERVER_PORT = "9100"; + expect(resolveSandboxPort()).toBe(9100); + }); + + it("rejects an out-of-range value (70000) so it can't crash listen", () => { + process.env.MCP_SANDBOX_PORT = "70000"; + expect(resolveSandboxPort()).toBe(0); + }); }); describe("createSandboxController", () => { @@ -85,7 +147,10 @@ describe("createSandboxController", () => { // container, so any default-src/connect-src here would intersect with and // override the per-app CSP baked into the inner document. const csp = res.headers.get("content-security-policy") ?? ""; - expect(csp).toContain("frame-ancestors http://127.0.0.1:*"); + // Assert the FULL directive (toBe, not toContain) so re-adding a source — + // e.g. the `http://[::1]:*` that F2 proved harmful — would fail the test. + // No `[::1]` — a bracketed IPv6 literal is not a valid CSP host-source. + expect(csp).toBe("frame-ancestors http://127.0.0.1:* http://localhost:*"); expect(csp).not.toContain("default-src"); expect(csp).not.toContain("connect-src"); const body = await res.text(); @@ -97,6 +162,57 @@ describe("createSandboxController", () => { } }); + it("serves a CSP derived from allowedOrigins (the shipped default path)", async () => { + // Both real callers always pass allowedOrigins, so the header the product + // actually serves is the derived exact-origin directive, not the fallback. + const controller = createSandboxController({ + port: 0, + allowedOrigins: [ + "http://localhost:6274", + "http://127.0.0.1:6274", + "http://[::1]:6274", // dropped — not a valid CSP host-source + ], + }); + try { + const { url } = await controller.start(); + const res = await fetch(url); + const csp = res.headers.get("content-security-policy") ?? ""; + expect(csp).toBe( + "frame-ancestors http://localhost:6274 http://127.0.0.1:6274", + ); + } finally { + await controller.close(); + } + }); + + it("advertises localhost in the sandbox URL for a wildcard bind", async () => { + // 0.0.0.0 isn't reachable from the browser, but a wildcard bind serves + // loopback — so the URL handed to the client (and printed in the banner) + // uses localhost. + const controller = createSandboxController({ port: 0, host: "0.0.0.0" }); + try { + const { url, port } = await controller.start(); + expect(url).toBe(`http://localhost:${port}/sandbox`); + } finally { + await controller.close(); + } + }); + + it("uses the canonical (unmapped) host in the sandbox URL", async () => { + // Same canonicalization as the origin allow-list, so the served sandbox URL + // is reachable — ::ffff:127.0.0.1 answers at 127.0.0.1, not [::ffff:...]. + const controller = createSandboxController({ + port: 0, + host: "::ffff:127.0.0.1", + }); + try { + const { url, port } = await controller.start(); + expect(url).toBe(`http://127.0.0.1:${port}/sandbox`); + } finally { + await controller.close(); + } + }); + it("returns 404 for paths other than /sandbox", async () => { const controller = createSandboxController({ port: 0 }); try { diff --git a/clients/web/src/test/integration/server/web-server-config.test.ts b/clients/web/src/test/integration/server/web-server-config.test.ts index 2b407118e..5436b8fea 100644 --- a/clients/web/src/test/integration/server/web-server-config.test.ts +++ b/clients/web/src/test/integration/server/web-server-config.test.ts @@ -1,7 +1,8 @@ -import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { buildWebServerConfig, buildWebServerConfigFromEnv, + defaultAllowedOrigins, printServerBanner, webServerConfigToInitialPayload, type WebServerConfig, @@ -20,6 +21,7 @@ const MUTATED_ENV_KEYS = [ "CLIENT_PORT", "HOST", "DANGEROUSLY_OMIT_AUTH", + "DANGEROUSLY_BIND_ALL_INTERFACES", "MCP_STORAGE_DIR", "ALLOWED_ORIGINS", "MCP_SANDBOX_PORT", @@ -75,7 +77,11 @@ describe("buildWebServerConfigFromEnv", () => { expect(cfg.dangerouslyOmitAuth).toBe(false); expect(cfg.initialMcpConfig).toBeNull(); expect(cfg.storageDir).toBeUndefined(); - expect(cfg.allowedOrigins).toEqual(["http://localhost:6274"]); + expect(cfg.allowedOrigins).toEqual([ + "http://localhost:6274", + "http://127.0.0.1:6274", + "http://[::1]:6274", + ]); expect(cfg.sandboxPort).toBe(0); expect(cfg.sandboxHost).toBe("localhost"); expect(cfg.logger).toBeUndefined(); @@ -85,13 +91,54 @@ describe("buildWebServerConfigFromEnv", () => { expect(cfg.autoOpen).toBe(false); }); - it("honors CLIENT_PORT and HOST", () => { + it("honors CLIENT_PORT and a loopback HOST", () => { process.env.CLIENT_PORT = "8123"; - process.env.HOST = "0.0.0.0"; + process.env.HOST = "127.0.0.1"; const cfg = buildWebServerConfigFromEnv(); expect(cfg.port).toBe(8123); + expect(cfg.hostname).toBe("127.0.0.1"); + }); + + it("refuses HOST=0.0.0.0 without the bind-all opt-in", () => { + process.env.HOST = "0.0.0.0"; + expect(() => buildWebServerConfigFromEnv()).toThrow( + /DANGEROUSLY_BIND_ALL_INTERFACES/, + ); + }); + + it("allows HOST=0.0.0.0 when the bind-all opt-in is set", () => { + process.env.CLIENT_PORT = "8123"; + process.env.HOST = "0.0.0.0"; + process.env.DANGEROUSLY_BIND_ALL_INTERFACES = "true"; + const cfg = buildWebServerConfigFromEnv(); expect(cfg.hostname).toBe("0.0.0.0"); - expect(cfg.allowedOrigins).toEqual(["http://0.0.0.0:8123"]); + // A wildcard bind serves loopback, so the default allow-list is the + // loopback trio (what a `docker run -p` browser actually sends) plus the + // canonical wildcard pair (0.0.0.0 / [::]). + expect(cfg.allowedOrigins).toEqual([ + "http://localhost:8123", + "http://127.0.0.1:8123", + "http://[::1]:8123", + "http://0.0.0.0:8123", + "http://[::]:8123", + ]); + }); + + it("expands a loopback HOST into all equivalent loopback origins", () => { + // `localhost` resolves to either 127.0.0.1 or ::1 depending on the OS, and + // Node/Vite may bind the IPv6 form — so a browser can send `Origin: + // http://[::1]:PORT` even though the banner advertised `localhost`. The + // default must accept all three so the DNS-rebinding guard doesn't 403 a + // legitimate loopback connect (the exact bug behind an stdio server that + // "should always work" failing with a 403 Invalid origin). + process.env.HOST = "127.0.0.1"; + process.env.CLIENT_PORT = "6274"; + const cfg = buildWebServerConfigFromEnv(); + expect(cfg.allowedOrigins).toEqual([ + "http://localhost:6274", + "http://127.0.0.1:6274", + "http://[::1]:6274", + ]); }); it("clears authToken when DANGEROUSLY_OMIT_AUTH is set even if AUTH_TOKEN is present", () => { @@ -121,10 +168,107 @@ describe("buildWebServerConfigFromEnv", () => { expect(cfg.authToken).toBe("primary"); }); - it("parses ALLOWED_ORIGINS and filters empty entries", () => { - process.env.ALLOWED_ORIGINS = "http://a,,http://b"; + it("parses ALLOWED_ORIGINS, trimming entries and filtering empties", () => { + process.env.ALLOWED_ORIGINS = "http://a:1, , http://b:2 "; + const cfg = buildWebServerConfigFromEnv(); + expect(cfg.allowedOrigins).toEqual(["http://a:1", "http://b:2"]); + }); + + it.each(["", " ", ","])( + "falls back to the default (not an empty allow-all) when ALLOWED_ORIGINS is %j", + (value) => { + // An empty parsed list must NOT reach the middleware — it treats an empty + // allow-list as allow-all, which would silently disable the origin guard. + process.env.ALLOWED_ORIGINS = value; + const cfg = buildWebServerConfigFromEnv(); + expect(cfg.allowedOrigins).toEqual([ + "http://localhost:6274", + "http://127.0.0.1:6274", + "http://[::1]:6274", + ]); + }, + ); + + it("canonicalizes ALLOWED_ORIGINS entries so copy-paste forms still match", () => { + // Trailing slash, uppercase host, explicit default :80 — all normalized to + // the canonical Origin the browser actually sends. + process.env.ALLOWED_ORIGINS = + "http://localhost:6274/, http://Example.COM:6274, http://myhost:80"; const cfg = buildWebServerConfigFromEnv(); - expect(cfg.allowedOrigins).toEqual(["http://a", "http://b"]); + expect(cfg.allowedOrigins).toEqual([ + "http://localhost:6274", + "http://example.com:6274", + "http://myhost", + ]); + }); + + it("drops unparseable ALLOWED_ORIGINS entries (and falls back if none survive)", () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + process.env.ALLOWED_ORIGINS = "not a url, *, http://ok:1"; + const cfg = buildWebServerConfigFromEnv(); + // "not a url" / "*" throw; "http://ok:1" parses. + expect(cfg.allowedOrigins).toEqual(["http://ok:1"]); + + process.env.ALLOWED_ORIGINS = "not a url, also bad"; + const cfg2 = buildWebServerConfigFromEnv(); + expect(cfg2.allowedOrigins).toEqual([ + "http://localhost:6274", + "http://127.0.0.1:6274", + "http://[::1]:6274", + ]); + } finally { + warnSpy.mockRestore(); + } + }); + + it('drops scheme-less / opaque ALLOWED_ORIGINS entries rather than allow-listing "null"', () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + // `new URL("localhost:6274").origin` is the literal "null" (not a throw) — + // must be dropped, not allow-listed (it'd match a real `Origin: null`). + process.env.ALLOWED_ORIGINS = + "localhost:6274, file:///srv, http://real:1"; + const cfg = buildWebServerConfigFromEnv(); + expect(cfg.allowedOrigins).toEqual(["http://real:1"]); + + // All scheme-less → nothing survives → fail-closed fallback to default. + process.env.ALLOWED_ORIGINS = "localhost:6274, myhost:8080"; + const cfg2 = buildWebServerConfigFromEnv(); + expect(cfg2.allowedOrigins).toEqual([ + "http://localhost:6274", + "http://127.0.0.1:6274", + "http://[::1]:6274", + ]); + } finally { + warnSpy.mockRestore(); + } + }); + + it("drops a wildcard ALLOWED_ORIGINS entry (works for CSP but never for the origin check)", () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + process.env.ALLOWED_ORIGINS = + "http://*.example.com:6274, http://real.example.com:6274"; + const cfg = buildWebServerConfigFromEnv(); + expect(cfg.allowedOrigins).toEqual(["http://real.example.com:6274"]); + } finally { + warnSpy.mockRestore(); + } + }); + + it.each(["0", "abc", "70000", "-1", "6274abc", "80.9"])( + "rejects an unusable CLIENT_PORT %j with an actionable error", + (value) => { + process.env.CLIENT_PORT = value; + expect(() => buildWebServerConfigFromEnv()).toThrow(/CLIENT_PORT/); + }, + ); + + it("treats an empty CLIENT_PORT as unset (defaults to 6274)", () => { + process.env.CLIENT_PORT = ""; + const cfg = buildWebServerConfigFromEnv(); + expect(cfg.port).toBe(6274); }); it("resolves sandboxPort from MCP_SANDBOX_PORT", () => { @@ -196,6 +340,110 @@ describe("buildWebServerConfigFromEnv", () => { }); }); +describe("defaultAllowedOrigins", () => { + it.each(["localhost", "127.0.0.1", "::1", "[::1]", "LOCALHOST"])( + "expands the loopback host %s into all three loopback origins", + (host) => { + expect(defaultAllowedOrigins(host, 6274)).toEqual([ + "http://localhost:6274", + "http://127.0.0.1:6274", + "http://[::1]:6274", + ]); + }, + ); + + it("returns a single exact origin for a specific non-loopback host", () => { + expect(defaultAllowedOrigins("192.168.1.50", 6274)).toEqual([ + "http://192.168.1.50:6274", + ]); + }); + + // A non-canonical spelling of a loopback address is canonicalized (the way the + // browser canonicalizes it into `Origin`), so it's recognized as loopback and + // gets the trio — not a single unmatchable entry. + it.each([ + "127.1", + "0x7f.0.0.1", + "2130706433", + "0:0:0:0:0:0:0:1", + "::0001", + "::ffff:127.0.0.1", // IPv4-mapped loopback — the socket answers on 127.0.0.1 + ])("canonicalizes the loopback spelling %j and returns the trio", (host) => { + expect(defaultAllowedOrigins(host, 6274)).toEqual([ + "http://localhost:6274", + "http://127.0.0.1:6274", + "http://[::1]:6274", + ]); + }); + + it("unmaps an IPv4-mapped non-loopback host to its dotted form", () => { + expect(defaultAllowedOrigins("::ffff:192.168.1.50", 6274)).toEqual([ + "http://192.168.1.50:6274", + ]); + }); + + it("keeps a distinct non-loopback address (127.0.0.2) as a single origin", () => { + // 127.0.0.2 is canonical and a bind there doesn't serve 127.0.0.1, so the + // single-origin branch is correct — only non-canonical *spellings* expand. + expect(defaultAllowedOrigins("127.0.0.2", 6274)).toEqual([ + "http://127.0.0.2:6274", + ]); + }); + + it("handles an empty host (canonicalUrlHost's non-URL fallback) as the wildcard", () => { + // `new URL("http://")` throws, so canonicalUrlHost falls back to "" — which + // isAllInterfacesHost treats as the wildcard. No `http://:PORT` garbage. + expect(defaultAllowedOrigins("", 8123)).toEqual([ + "http://localhost:8123", + "http://127.0.0.1:8123", + "http://[::1]:8123", + "http://0.0.0.0:8123", + "http://[::]:8123", + ]); + }); + + // Any wildcard spelling yields the loopback trio + the CANONICAL wildcard pair + // (0.0.0.0 / [::]) — not the typed spelling, which the browser canonicalizes + // away (HOST=0 / 0x0 / 0.0.0 all send http://0.0.0.0:PORT; ::0 sends [::]). + it.each(["0.0.0.0", "::", "::0", "0", "0x0", "0.0.0"])( + "returns the loopback trio + canonical wildcard pair for the all-interfaces host %j", + (host) => { + expect(defaultAllowedOrigins(host, 8123)).toEqual([ + "http://localhost:8123", + "http://127.0.0.1:8123", + "http://[::1]:8123", + "http://0.0.0.0:8123", + "http://[::]:8123", + ]); + }, + ); + + it("lowercases a non-loopback hostname to match the browser's Origin", () => { + expect(defaultAllowedOrigins("Example.COM", 6274)).toEqual([ + "http://example.com:6274", + ]); + }); + + it("omits the port from the origin when it's the http default (80)", () => { + // Browsers drop :80 from the Origin header, and the guard is an exact + // match, so an origin with :80 could never match a real request. + expect(defaultAllowedOrigins("192.168.1.50", 80)).toEqual([ + "http://192.168.1.50", + ]); + expect(defaultAllowedOrigins("localhost", 80)).toEqual([ + "http://localhost", + "http://127.0.0.1", + "http://[::1]", + ]); + }); + + it("brackets a non-loopback IPv6 literal so it's a valid origin", () => { + expect(defaultAllowedOrigins("fe80::1", 6274)).toEqual([ + "http://[fe80::1]:6274", + ]); + }); +}); + describe("buildWebServerConfig", () => { it("matches buildWebServerConfigFromEnv when initialMcpConfig is omitted", () => { process.env[API_SERVER_ENV_VARS.AUTH_TOKEN] = "shared"; @@ -239,7 +487,11 @@ describe("buildWebServerConfig", () => { const cfg = buildWebServerConfig({ initialMcpConfig }); expect(cfg.port).toBe(7000); expect(cfg.initialMcpConfig).toEqual(initialMcpConfig); - expect(cfg.allowedOrigins).toEqual(["http://localhost:7000"]); + expect(cfg.allowedOrigins).toEqual([ + "http://localhost:7000", + "http://127.0.0.1:7000", + "http://[::1]:7000", + ]); }); it("preserves remote transport initialMcpConfig", () => { @@ -368,6 +620,31 @@ describe("printServerBanner", () => { expect(url).toBe("http://localhost:6274"); }); + it("brackets an IPv6 bind host in the printed URL", () => { + const cfg = baseConfig(); + cfg.hostname = "::1"; + const url = printServerBanner(cfg, 6274, "", undefined); + expect(url).toBe("http://[::1]:6274"); + }); + + it("advertises localhost for a wildcard bind rather than the wildcard host", () => { + const cfg = baseConfig(); + cfg.hostname = "0.0.0.0"; + const url = printServerBanner(cfg, 6274, "", undefined); + expect(url).toBe("http://localhost:6274"); + }); + + it("advertises the canonical (unmapped) host so banner ⊆ allowedOrigins", () => { + // An IPv4-mapped bind host: the allow-list emits the loopback trio (which + // includes http://127.0.0.1), so the banner must advertise a member of it, + // not the [::ffff:127.0.0.1] form (whose Origin is [::ffff:7f00:1]). + const cfg = baseConfig(); + cfg.hostname = "::ffff:127.0.0.1"; + const url = printServerBanner(cfg, 6274, "", undefined); + expect(url).toBe("http://127.0.0.1:6274"); + expect(defaultAllowedOrigins(cfg.hostname, 6274)).toContain(url); + }); + it("prints the sandbox URL when provided", () => { printServerBanner(baseConfig(), 6274, "tok", "http://sandbox:9999/sandbox"); expect( diff --git a/clients/web/vite.config.ts b/clients/web/vite.config.ts index 9d5c0d596..3b99d672b 100644 --- a/clients/web/vite.config.ts +++ b/clients/web/vite.config.ts @@ -93,10 +93,22 @@ function browserExternalizedBuiltinGate(): Plugin { // More info at: https://storybook.js.org/docs/next/writing-tests/integrations/vitest-addon export default defineConfig(({ command }) => { const isDevServer = command === "serve" && !process.env.VITEST; + // Build the validated dev backend config ONCE (when serving) and reuse it for + // both the Hono plugin and the `server` block below, so the dev server's + // `port`/`host` come from the same guard-checked source (`resolveBindHostname` + // + the CLIENT_PORT validation) rather than a second raw parse. This also + // removes the implicit "plugins must be evaluated before server" ordering the + // guard previously relied on to throw first. + const devConfig = isDevServer ? buildWebServerConfigFromEnv() : undefined; return { - // `honoMiddlewarePlugin` is gated by `apply: 'serve'` so it only attaches - // during `vite dev` / `vite preview` — vitest projects share this config - // but never invoke `configureServer`, so the plugin stays inert there. + // `honoMiddlewarePlugin` only attaches during `vite dev` / `vite preview`. + // It's included conditionally on `isDevServer` (not merely `apply: 'serve'`) + // because `devConfig` (from `buildWebServerConfigFromEnv()`, which calls + // `resolveBindHostname()`) is built eagerly above. Left unconditional, an + // ambient `HOST=0.0.0.0` would make the guard throw at config load for + // `vite build` and every vitest project too, not just when serving. Gating + // the whole plugin also skips that wasted config build for non-serve + // commands. // // The plugin statically imports the node-only dev backend // (`core/mcp/remote/node/server.ts`), so Vite's config bundler (Rolldown) @@ -113,7 +125,7 @@ export default defineConfig(({ command }) => { // reaches the browser bundle (#1769) — see its definition above. plugins: [ react(), - honoMiddlewarePlugin(buildWebServerConfigFromEnv()), + ...(devConfig ? [honoMiddlewarePlugin(devConfig)] : []), browserExternalizedBuiltinGate(), ], // Shared optimizeDeps exclusions so node-only packages @@ -146,18 +158,26 @@ export default defineConfig(({ command }) => { // location (which has no node_modules of its own yet). dedupe: sharedDedupe, }, - // Pin the Vite dev server to the same port (and host) the Hono plugin - // configures from env, so `allowedOrigins` actually matches the browser - // origin. Without this, `vite dev` falls back to Vite's default 5173 - // while the dev backend's `buildWebServerConfigFromEnv()` defaults to - // CLIENT_PORT=6274 — origin check rejects every `/api/*` request from - // the browser. CLIENT_PORT / HOST overrides flow through here too. - // `strictPort: true` so a port collision fails loudly instead of - // silently picking a different port (which would leave `allowedOrigins` - // pointing at the wrong host and break browser fetches). + // Pin the Vite dev server to the same port and host the Hono plugin + // configures, so `allowedOrigins` actually matches the browser origin. + // Without this, `vite dev` falls back to Vite's default 5173 while the dev + // backend defaults to CLIENT_PORT=6274 — origin check rejects every `/api/*` + // request. When serving, both come from the already-validated `devConfig` + // (guard-checked host + CLIENT_PORT); `vite build` and the vitest projects + // evaluate this config but never bind, so they fall back to the raw env + // (an ambient HOST=0.0.0.0 must not fail them at config load). + // `strictPort: true` so a port collision fails loudly instead of silently + // picking a different port (which would leave `allowedOrigins` wrong). server: { - port: parseInt(process.env.CLIENT_PORT ?? "6274", 10), - host: process.env.HOST ?? "localhost", + // The `|| "6274"` (empty ⇒ unset) mirrors buildWebServerConfig, so the + // non-serve fallback agrees with the validated dev path on a blank + // CLIENT_PORT rather than parsing it to NaN. + port: + devConfig?.port ?? + parseInt(process.env.CLIENT_PORT?.trim() || "6274", 10), + // `|| "localhost"` (empty ⇒ unset) like the port above — an ambient + // HOST="" is Node's all-interfaces address, the one value this guards. + host: devConfig?.hostname ?? (process.env.HOST?.trim() || "localhost"), strictPort: true, fs: { allow: [path.resolve(dirname, "../..")], diff --git a/core/auth/node/oauth-callback-server.ts b/core/auth/node/oauth-callback-server.ts index 37a0cce6a..fa17b59b3 100644 --- a/core/auth/node/oauth-callback-server.ts +++ b/core/auth/node/oauth-callback-server.ts @@ -1,4 +1,5 @@ import { createServer, type Server } from "node:http"; +import { formatHostForUrl } from "../../node/hostUrl.js"; import { parseOAuthCallbackParams } from "../utils.js"; import { generateOAuthErrorDescription } from "../utils.js"; @@ -220,7 +221,5 @@ export function createOAuthCallbackServer(): OAuthCallbackServer { } function buildRedirectUrl(host: string, port: number, path: string): string { - const needsBrackets = host.includes(":") && !host.startsWith("["); - const formattedHost = needsBrackets ? `[${host}]` : host; - return `http://${formattedHost}:${port}${path}`; + return `http://${formatHostForUrl(host)}:${port}${path}`; } diff --git a/core/auth/node/runner-oauth-callback.ts b/core/auth/node/runner-oauth-callback.ts index e8a374a3f..61affb700 100644 --- a/core/auth/node/runner-oauth-callback.ts +++ b/core/auth/node/runner-oauth-callback.ts @@ -10,6 +10,12 @@ * unsupported; override via `--callback-url` / `MCP_OAUTH_CALLBACK_URL`. */ +import { + formatHostForUrl, + isLoopbackHost, + stripBrackets, +} from "../../node/hostUrl.js"; + export const RUNNER_OAUTH_CALLBACK_DEFAULT_HOSTNAME = "127.0.0.1"; /** Default loopback port for TUI/CLI OAuth callback (6276 ≈ T9 "MCPO", MCP OAuth). */ export const RUNNER_OAUTH_CALLBACK_DEFAULT_PORT = 6276; @@ -58,6 +64,19 @@ export function parseRunnerOAuthCallbackUrl( throw new Error("OAuth callback URL must include a hostname"); } /* v8 ignore stop */ + // The callback listener receives the OAuth *authorization code* over plaintext + // http, so it must be loopback (RFC 8252 §7.3) — a routable host would deliver + // the credential in cleartext over the network. Reject anything non-loopback, + // not just the all-interfaces wildcard. + if (!isLoopbackHost(hostname)) { + throw new Error( + `OAuth callback URL must bind a loopback host (localhost / 127.0.0.1 / ` + + `[::1]), not "${hostname}": the callback listener receives the OAuth ` + + `authorization code over plaintext http, so a network-reachable host ` + + `risks credential interception. Use 127.0.0.1 (and a port-forward if the ` + + `browser is elsewhere).`, + ); + } /* v8 ignore next -- an http: URL always has a non-empty pathname (min "/"), so the fallback is unreachable */ const pathname = url.pathname || "/"; let port: number; @@ -76,7 +95,10 @@ export function parseRunnerOAuthCallbackUrl( } /* v8 ignore stop */ } - return { hostname, port, pathname }; + // De-bracket an IPv6 host (`url.hostname` keeps the brackets) so `listen()` can + // consume it — `[::1]` fails ENOTFOUND, `::1` binds. formatRunnerOAuthRedirectUrl + // re-brackets via formatHostForUrl for the redirect URI. + return { hostname: stripBrackets(hostname), port, pathname }; } /** @@ -87,10 +109,5 @@ export function parseRunnerOAuthCallbackUrl( export function formatRunnerOAuthRedirectUrl( config: RunnerOAuthCallbackConfig, ): string { - const needsBrackets = - config.hostname.includes(":") && !config.hostname.startsWith("["); - const formattedHost = needsBrackets - ? `[${config.hostname}]` - : config.hostname; - return `http://${formattedHost}:${config.port}${config.pathname}`; + return `http://${formatHostForUrl(config.hostname)}:${config.port}${config.pathname}`; } diff --git a/core/node/hostUrl.ts b/core/node/hostUrl.ts new file mode 100644 index 000000000..93a9a55c5 --- /dev/null +++ b/core/node/hostUrl.ts @@ -0,0 +1,182 @@ +import { isIPv6 } from "node:net"; + +/** Strip a single surrounding `[...]` pair from a bracketed IPv6 literal; other hosts pass through. */ +export function stripBrackets(host: string): string { + return host.replace(/^\[(.*)\]$/, "$1"); +} + +/** + * Format a bind host for use inside a URL authority. An IPv6 literal must be + * bracketed (`http://[::1]:6274`); every other host — loopback names, IPv4, + * hostnames, already-bracketed IPv6 — passes through unchanged. Shared by the + * web origin allow-list, the startup banner, the sandbox URL, and the CLI + * deep-link so a bound IPv6 host is formatted the same way everywhere. + * + * Bracketing keys off `net.isIPv6`, not the presence of a `:`, so a mistyped + * `host:port` isn't wrapped as `[host:port]`. A zone index (`%eth0`) is dropped: + * a URL authority cannot carry one (`new URL()` rejects it even `%25`-encoded), + * and the zone is host-local and meaningless to a remote client anyway. + */ +export function formatHostForUrl(host: string): string { + const h = host.trim(); + // Strip surrounding brackets and any zone id before the IPv6 check, so a + // bracketed-with-zone input (`[fe80::1%eth0]`) still yields a valid URL host. + // (A non-IPv6 value is returned as-given — this normalizes IPv6 literals, it + // doesn't validate arbitrary hosts.) + const bare = stripBrackets(h).split("%")[0]; + return isIPv6(bare) ? `[${bare}]` : h; +} + +/** + * Unmap an IPv4-mapped IPv6 host (`[::ffff:7f00:1]`) to its dotted IPv4 form + * (`127.0.0.1`) — the address the socket actually answers on (a + * `::ffff:127.0.0.1` bind is reachable at `127.0.0.1`, not `::1`). Other hosts + * pass through. Mirrors the bind guard, which folds the mapped *wildcard* + * (`::ffff:0:0`) into its all-interfaces set. + */ +function unmapIpv4MappedHost(host: string): string { + const m = /^::ffff:([0-9a-f]{1,4}):([0-9a-f]{1,4})$/i.exec( + stripBrackets(host), + ); + if (!m) return host; + const hi = parseInt(m[1], 16); + const lo = parseInt(m[2], 16); + return `${hi >> 8}.${hi & 0xff}.${lo >> 8}.${lo & 0xff}`; +} + +/** + * Canonicalize a bind host the way a browser does before building `Origin`, so + * that the origin allow-list, the startup banner, and the sandbox URL all use + * the *same* form and can't disagree. Non-canonical spellings of the same + * address — `127.1` / `0x7f.0.0.1` / `2130706433` all mean `127.0.0.1`, + * `0:0:0:0:0:0:0:1` / `::0001` mean `::1` — otherwise produce hosts that miss + * the loopback lookup or don't match the header the browser sends. + * + * One step **intentionally diverges** from browser canonicalization: an + * IPv4-mapped IPv6 host is unmapped to its dotted IPv4 form (the address the + * socket answers on), where a browser would keep the mapped literal. Because the + * banner/sandbox URL are canonicalized through here too, `banner ⊆ allowedOrigins` + * holds by construction — the advertised URL is always an allow-listed origin. + * + * `new URL().hostname` returns the URL-ready form (bracketed for IPv6); falls + * back to the formatted input if it isn't a parseable URL host. + */ +export function canonicalUrlHost(host: string): string { + const formatted = formatHostForUrl(host.trim().toLowerCase()); + try { + return unmapIpv4MappedHost(new URL(`http://${formatted}`).hostname); + } catch { + return formatted; + } +} + +// NB on layering: since round-19 (#1795 AD1), {@link isAllInterfacesHost} runs +// `canonicalUrlHost` FIRST, and that (via `new URL()`) already compresses IPv6 +// zero-runs, unmaps IPv4-mapped addresses, and folds every legacy `inet_aton` +// and IDNA spelling to `0.0.0.0`. So `canonicalUrlHost` is the actual wildcard +// cover; `ALL_INTERFACES_LITERALS`'s `::ffff:0:0` entry, `canonicalizeIpv6`, and +// `isAllZeroIpv4` below are a **redundant second layer** — measured to change no +// verdict for any bindable input. They're kept as defense-in-depth (and they do +// cover `canonicalUrlHost`'s non-URL `catch` fallback), but do NOT remove the +// `canonicalUrlHost` call from `isAllInterfacesHost` on the assumption these +// still catch the legacy spellings — they don't anymore. + +/** + * Canonical spellings of the all-interfaces (unspecified) address: `0.0.0.0` + * (IPv4 wildcard), `::` (IPv6 wildcard), `::ffff:0:0` (IPv4-mapped wildcard — + * redundant now that `canonicalUrlHost` unmaps it to `0.0.0.0`), and `""` + * (Node's `listen()` unspecified address). See the layering note above. + */ +const ALL_INTERFACES_LITERALS = new Set(["", "0.0.0.0", "::", "::ffff:0:0"]); + +/** + * Canonicalize an IPv6 literal via the WHATWG URL serializer, which compresses + * zero-runs — `::0` / `0::0` / `::0.0.0.0` / `0000:…:0000` → `::`, `::ffff:0.0.0.0` + * → `::ffff:0:0`. Redundant second layer (see the note above): `canonicalUrlHost` + * already ran the value through `new URL()`, so for any host it can produce this + * is the identity; kept for defense-in-depth. Non-IPv6 input passes through. + * + * The zone index (`%eth0`) is stripped first: `net.isIPv6` accepts it but + * `new URL()` rejects a zone id outright (even `%25`-encoded), so passing it + * through would throw. The zone is irrelevant to *which* address this is, so + * dropping it is correct for detection — `::%eth0` still canonicalizes to `::`. + */ +function canonicalizeIpv6(value: string): string { + if (!isIPv6(value)) return value; + const [address] = value.split("%"); + return new URL(`http://[${address}]`).hostname.slice(1, -1); +} + +/** + * Parse one dotted-address part (or a bare address) the way the C `inet_aton` + * resolver does — decimal, `0`-prefixed octal, and `0x`-prefixed hex are all + * accepted. Returns `NaN` for anything non-numeric (e.g. a hostname label). + */ +function parseAddressPart(part: string): number { + if (/^0x[0-9a-f]+$/.test(part)) return parseInt(part, 16); + if (/^[0-9]+$/.test(part)) return parseInt(part, 10); + return NaN; +} + +/** + * True when `value` is an all-zero IPv4 address in any legacy spelling the OS + * still binds as the `0.0.0.0` wildcard: the bare integer `0`, `0x0`, dotted + * `0.0.0.0`, `000.000.000.000`, `0x0.0.0.0`, and the short forms `0.0` / `0.0.0` + * (Node/`inet_aton` accept 1–4 parts; `parseAddressPart`'s `[0-9]+` branch does + * double duty for decimal and `0`-prefixed octal). The `> 4` reject is + * intentional — 1–3 parts are valid `inet_aton` spellings, so this is + * deliberately NOT `parts.length === 4`. Called from {@link isAllInterfacesHost} + * with any normalized host, so it may receive an IPv6 literal (`::1`) — the + * dot-split yields a single non-numeric part → `NaN` → `false`, which is correct. + * + * Redundant second layer (see the note above `ALL_INTERFACES_LITERALS`): every + * bindable legacy spelling (`0`, `0x0`, `0.0`, `000.000.000.000`, `０`, …) is + * already folded to `0.0.0.0` by `canonicalUrlHost`, so this changes no verdict + * for a real host; its only live input is `canonicalUrlHost`'s non-URL `catch` + * fallback (e.g. bracketed non-IPv6 `[0.0.0]`, which fails `ENOTFOUND` anyway). + */ +function isAllZeroIpv4(value: string): boolean { + const parts = value.split("."); + if (parts.length > 4) return false; + return parts.every((part) => parseAddressPart(part) === 0); +} + +/** + * True when `host` binds all interfaces (`0.0.0.0` / `::` / empty / their legacy + * spellings) rather than loopback only. Shared by the web bind guard, the + * banner/sandbox wildcard→`localhost` substitution, and the CLI deep-link. + * Expects a bare host (no port) — callers pass `url.hostname` or an env `HOST`. + * + * The value is run through {@link canonicalUrlHost} first — Node's resolver + * applies the IDNA Unicode→ASCII mapping before parsing the literal, so a + * fullwidth `HOST="０"` (U+FF10) binds `0.0.0.0`; canonicalizing (via `new URL`, + * which applies the same mapping) before the address check catches those + * spellings and keeps this predicate reasoning about the *address* the socket + * binds, not the raw string. Idempotent for ASCII hosts. + */ +export function isAllInterfacesHost(host: string): boolean { + const normalized = canonicalizeIpv6(stripBrackets(canonicalUrlHost(host))); + return ALL_INTERFACES_LITERALS.has(normalized) || isAllZeroIpv4(normalized); +} + +/** + * True when `host` is a loopback address — `localhost`, anything in `127.0.0.0/8` + * (incl. IPv4-mapped `::ffff:127.x`, which {@link canonicalUrlHost} unmaps), or + * `::1`. Canonicalized first, so non-canonical spellings (`127.1`, `0x7f.1`, + * `2130706433`, `0:0:…:1`) resolve correctly. Used to constrain the OAuth + * callback listener, which must be loopback (it receives the authorization code + * over plaintext `http`; RFC 8252 §7.3 only sanctions that for loopback). + * Expects a bare host (no port) — the caller passes `url.hostname`. + */ +export function isLoopbackHost(host: string): boolean { + // Drop a root FQDN dot (`localhost.` binds loopback but WHATWG keeps the dot); + // IP literals never carry one (WHATWG strips it for those). + const h = canonicalUrlHost(host).replace(/\.$/, ""); + // Octets are range-bounded (`canonicalUrlHost` returns the raw input when + // `new URL` throws, so `\d{1,3}` alone would pass `127.999.0.1`). + return ( + h === "localhost" || + h === "[::1]" || + /^127(\.(25[0-5]|2[0-4]\d|1?\d?\d)){3}$/.test(h) + ); +} diff --git a/docs/mcp-app-review.md b/docs/mcp-app-review.md index c6ca251bd..7a0acbe00 100644 --- a/docs/mcp-app-review.md +++ b/docs/mcp-app-review.md @@ -168,8 +168,13 @@ ssh -L 6274:127.0.0.1:6274 -L 6275:127.0.0.1:6275 Then open `http://127.0.0.1:6274/?MCP_INSPECTOR_API_TOKEN=$TOKEN` locally. -- Use `127.0.0.1`, **not** `localhost` — the backend's origin guard checks the - literal `Origin` header against the configured `HOST`. +- Use `127.0.0.1` or `localhost` — the default origin allow-list emits both (and + `[::1]`) for a loopback bind (#1795), so the guard accepts either, whichever + the browser sends. **Do not** use a bare `http://[::1]:…` URL for App review: + it connects, but a bracketed IPv6 literal isn't a valid CSP `frame-ancestors` + source, so the App sandbox iframe is CSP-blocked and the widget never renders + (see the MCP Apps caveat in [`clients/web/README.md`](../clients/web/README.md#host-binding--the-origin-allow-list)). + A **non-loopback** forward target needs `ALLOWED_ORIGINS` set to that origin. - Forward `:6275` (`MCP_SANDBOX_PORT`) as well: the Apps tab renders widgets in an iframe served from that second origin; without it the App frame stays blank. diff --git a/scripts/smoke-cli.mjs b/scripts/smoke-cli.mjs index 6649740d3..ef93bdfdf 100644 --- a/scripts/smoke-cli.mjs +++ b/scripts/smoke-cli.mjs @@ -23,6 +23,14 @@ * 8. Over an HTTP transport (in-process test server), a config-file `headers` * object is lifted onto the wire, and a CLI `--header` overrides it — the * headline lift→transport path of #1482, verified end to end. + * 9. A usage error through the launcher `--cli` path emits the JSON `{"error":…}` + * envelope and exits 1 (a bad `--callback-url`, whose message contains + * "OAuth", must NOT misclassify as auth_required — #1795 AK1/AJ1). + * 10. A non-1 exit code survives the launcher too: `--use-stored-auth` with no + * stored token exits 3 with envelope code `no_stored_token`, pinning that the + * whole EXIT_CODES map — not just exit 1 — reaches an automated caller + * through `mcp-inspector --cli`. (Steps 9–10 are the ONLY guard on that + * contract: `clients/{launcher,cli}/src/index.ts` are coverage-excluded.) * * Exits non-zero (failing CI / `npm run validate`) on any mismatch. * @@ -77,11 +85,18 @@ function ensureTestServer() { } } +// Neutralize env that would make an unrelated step fail: parseRunnerOAuthCallbackUrl +// runs on every --cli invocation, so an ambient MCP_OAUTH_CALLBACK_URL pointing at +// a non-loopback host would fail steps 1–8. Empty reads as unset in the parser +// (default 127.0.0.1:6276 applies); per-call extraEnv is spread after, so step 9 +// can still pass --callback-url explicitly. Mirrors prod-web-server.mjs's HOST pin. +const SMOKE_BASE_ENV = { MCP_OAUTH_CALLBACK_URL: "" }; + /** Run the launcher in --cli mode. Returns { status, stdout, stderr }. */ function runCli(args, extraEnv = {}) { const r = spawnSync(process.execPath, [launcher, "--cli", ...args], { cwd: repoRoot, - env: { ...process.env, ...extraEnv }, + env: { ...process.env, ...SMOKE_BASE_ENV, ...extraEnv }, encoding: "utf-8", }); return { status: r.status, stdout: r.stdout ?? "", stderr: r.stderr ?? "" }; @@ -97,7 +112,7 @@ function runCliAsync(args, extraEnv = {}) { return new Promise((resolveRun) => { const child = spawn(process.execPath, [launcher, "--cli", ...args], { cwd: repoRoot, - env: { ...process.env, ...extraEnv }, + env: { ...process.env, ...SMOKE_BASE_ENV, ...extraEnv }, }); let stdout = ""; let stderr = ""; @@ -404,11 +419,81 @@ try { await httpServer.stop(); } + // 9) A usage error through the launcher --cli path still emits the JSON + // envelope and the right exit code (#1795 AK1): the launcher imports runCli + // as a module, so the CLI's own error sink must be routed through, or the + // documented EXIT_CODES/envelope contract is lost. A bad --callback-url is + // a usage error whose message contains "OAuth" (would otherwise misclassify + // as auth_required/exit 3). + const badCallback = runCli([ + "--catalog", + catalogPath, + "--server", + "test", + "--method", + "tools/list", + "--callback-url", + "http://0.0.0.0:6276/oauth/callback", + ]); + if (badCallback.status !== 1) { + fail( + `bad --callback-url should exit 1 (usage) through the launcher, got ${badCallback.status}\n${badCallback.stderr}`, + ); + } + let envelope; + try { + envelope = JSON.parse(badCallback.stderr.trim()); + } catch { + fail( + `bad --callback-url should emit a JSON {"error":…} envelope through the launcher; got:\n${badCallback.stderr}`, + ); + } + if (envelope?.error?.code !== "error") { + fail( + `bad --callback-url envelope should be code "error", got ${JSON.stringify(envelope)}`, + ); + } + + // 10) A NON-1 exit code survives the launcher too (the actual AK1 claim — the + // whole EXIT_CODES map, not just the exit-1 the old catch-all produced). + // --use-stored-auth with no stored token throws AUTH_REQUIRED (3) with + // envelope code "no_stored_token" *before* any connect, so it's offline. + const noStoredAuth = runCli( + [ + "--server-url", + "http://example.invalid/mcp", + "--method", + "tools/list", + "--use-stored-auth", + ], + { HOME: fakeHome, USERPROFILE: fakeHome }, + ); + if (noStoredAuth.status !== 3) { + fail( + `--use-stored-auth with no token should exit 3 (auth_required) through the launcher, got ${noStoredAuth.status}\n${noStoredAuth.stderr}`, + ); + } + let authEnvelope; + try { + authEnvelope = JSON.parse(noStoredAuth.stderr.trim()); + } catch { + fail( + `--use-stored-auth should emit a JSON envelope through the launcher; got:\n${noStoredAuth.stderr}`, + ); + } + if (authEnvelope?.error?.code !== "no_stored_token") { + fail( + `--use-stored-auth envelope should be code "no_stored_token", got ${JSON.stringify(authEnvelope)}`, + ); + } + console.log( "smoke:cli OK — tools/list over stdio via --catalog; default-catalog seed; " + "read-only --config error (no seed); --catalog/--config conflict; " + "unknown --server error; multi-server --server selection; --header merge; " + - "HTTP config-header lift + --header override on the wire", + "HTTP config-header lift + --header override on the wire; " + + "launcher --cli usage error → JSON envelope + exit 1; " + + "launcher --cli auth error → JSON envelope + exit 3", ); } finally { rmSync(work, { recursive: true, force: true }); diff --git a/scripts/smoke-tui.mjs b/scripts/smoke-tui.mjs index dfb5d08ad..0f241d243 100644 --- a/scripts/smoke-tui.mjs +++ b/scripts/smoke-tui.mjs @@ -90,7 +90,15 @@ const child = spawn( { cwd: repoRoot, // Redirect HOME so the TUI's storage never touches the real ~/.mcp-inspector. - env: { ...process.env, HOME: work, USERPROFILE: work }, + // Pin MCP_OAUTH_CALLBACK_URL="" (empty reads as unset) so an ambient + // non-loopback value can't crash the TUI before render via the loopback + // callback guard — same class smoke-cli.mjs's SMOKE_BASE_ENV neutralizes. + env: { + ...process.env, + MCP_OAUTH_CALLBACK_URL: "", + HOME: work, + USERPROFILE: work, + }, stdio: ["ignore", "pipe", "pipe"], }, );