From c576b9633fdde4794a5e2673c9f282706a04fdea Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 6 Sep 2026 22:36:02 -0400 Subject: [PATCH 01/19] feat: accept *.localhost origins, and stop reporting a terminal OAuth refusal as retryable Closes #1944 Closes #2280 #1944 turned out to span two gates with two different owners, so this does the half we own and reports the half we do not. Ours: when the origin allow-list is the derived default and the bind host serves loopback, /api/* now also accepts any http(s) origin whose host ends in .localhost, at any port. The MCP Apps sandbox and app-origin frame-ancestors admit the same set, so the two layers cannot disagree and blank the Apps frame. That is the default Vite, Django and Rails all ship. Setting ALLOWED_ORIGINS turns it off (that list replaces the default and is documented as honoured exactly), as does a specific non-loopback bind. allowedOrigins stays a list of literal origins compared exactly; the widening is a separately named flag rather than a wildcard entry, because the list is also read by the CSP builder and a value the two layers read differently is the split-behaviour hazard ALLOWED_ORIGINS already rejects wildcards to avoid. Theirs (#2280): InsecureTokenEndpointError appeared nowhere in our source, so it fell through to the generic auth path and rendered a "Re-authentication required" banner whose button could never work - the error does not extend OAuthError and auth() rethrows it rather than redirecting. It is now recognized and surfaced as the configuration error it is, naming the endpoint and both ways out, with no action affordance. Making such an endpoint actually work has to land in the SDK (typescript-sdk#2591) - the assertion takes no options and there is no hook we could reach, the same conclusion #1911 reached. Adds oauth-insecure-token-endpoint-http.json to reproduce it: its issuer is http://localhost.:8091, the root-anchored spelling every resolver sends to loopback but which is none of the SDK's three exempt literals. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- .claude/skills/test-servers/SKILL.md | 1 + clients/web/README.md | 6 +- clients/web/server/app-origin-controller.ts | 19 ++- clients/web/server/sandbox-controller.ts | 63 +++++++++- clients/web/server/server.ts | 3 + clients/web/server/vite-hono-plugin.ts | 3 + clients/web/server/web-server-config.ts | 47 +++++++ .../web/src/hooks/useConnectionLifecycle.ts | 12 ++ clients/web/src/hooks/useOAuthRecovery.ts | 15 +++ .../lib/insecureTokenEndpointNotice.test.ts | 57 +++++++++ .../src/lib/insecureTokenEndpointNotice.ts | 52 ++++++++ .../core/auth/insecureTokenEndpoint.test.ts | 78 ++++++++++++ .../web/src/test/core/auth/oauthUx.test.ts | 45 +++++++ .../web/src/test/core/node/hostUrl.test.ts | 73 +++++++++++ .../integration/mcp/remote/transport.test.ts | 115 ++++++++++++++++++ .../server/app-origin-controller.test.ts | 18 +++ .../server/sandbox-controller.test.ts | 62 ++++++++++ .../server/server-auto-open.test.ts | 1 + .../server/server-token-injection.test.ts | 1 + .../server/web-server-config.test.ts | 63 ++++++++++ clients/web/src/utils/oauthUx.ts | 2 + core/auth/insecureTokenEndpoint.ts | 58 +++++++++ core/auth/oauthUx.ts | 51 +++++++- core/mcp/remote/node/server.ts | 49 +++++++- core/node/hostUrl.ts | 59 +++++++++ docs/test-servers.md | 15 +++ .../oauth-insecure-token-endpoint-http.json | 20 +++ 27 files changed, 969 insertions(+), 19 deletions(-) create mode 100644 clients/web/src/lib/insecureTokenEndpointNotice.test.ts create mode 100644 clients/web/src/lib/insecureTokenEndpointNotice.ts create mode 100644 clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts create mode 100644 core/auth/insecureTokenEndpoint.ts create mode 100644 test-servers/configs/oauth-insecure-token-endpoint-http.json diff --git a/.claude/skills/test-servers/SKILL.md b/.claude/skills/test-servers/SKILL.md index 16556b18b9..90051f4a3b 100644 --- a/.claude/skills/test-servers/SKILL.md +++ b/.claude/skills/test-servers/SKILL.md @@ -83,6 +83,7 @@ usually looks like a missing capability rather than an error. | A tool result's `structuredContent` section | `structured-output-http.json` (legacy) | | RFC 6570 resource-template expansion | `rfc6570-templates-http.json` | | OAuth token revocation on clear | `oauth-revocation-http.json` (legacy) | +| A token endpoint the SDK refuses (SEP-2207) | `oauth-insecure-token-endpoint-http.json` (legacy) | | Cancelling a call mid-flight | `cancellation-modern-http.json` (modern) | ## Adding a config or preset diff --git a/clients/web/README.md b/clients/web/README.md index 619865abfb..caa1182cff 100644 --- a/clients/web/README.md +++ b/clients/web/README.md @@ -379,6 +379,8 @@ Both the prod backend (`server/web-server-config.ts`) and the dev Vite server (` 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. +**`*.localhost` origins are accepted by default** (#1944). When the allow-list is the derived default *and* the bind host serves loopback (a loopback address, or an all-interfaces wildcard), the backend additionally accepts any `http://` or `https://` origin whose host ends in `.localhost`, **at any port** — `http://mcp.localhost`, `https://tenant.app.localhost:8443`. That covers the common local-dev shape where a reverse proxy on a `*.localhost` name fronts the Inspector, and it is the same default Vite, Django and Rails ship in their own host allow-lists. The suffix is [reserved to the loopback interface by RFC 6761 §6.3](https://www.rfc-editor.org/info/rfc6761/) and is not publicly registrable, so it cannot be obtained by an attacker the way a rebound domain can; `/api/*` still requires the bearer token regardless. Two things follow from the gating: setting `ALLOWED_ORIGINS` **turns this off** (that list replaces the default and is honoured exactly — add your `*.localhost` origins to it explicitly), and binding a *specific* non-loopback address turns it off too (a browser at `foo.localhost` resolves to `127.0.0.1` and never reaches such a process). Note that only the **browser** resolves these names for free: Chrome and Firefox map them to loopback internally, but the OS resolver on macOS does not, so an MCP **server** URL on a `*.localhost` host still needs a `/etc/hosts` entry or dnsmasq — the Inspector's backend is what dials it. Safari does not resolve them at all. + ### 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: @@ -387,9 +389,9 @@ The guard blocks only the **wildcard** all-interfaces addresses. Binding a **spe - **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 127.0.0.1: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`. +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`. A `*.localhost` name needs nothing there: Vite's default `allowedHosts` already accepts `localhost` and everything under `.localhost`, which is why only the origin allow-list above had to change. -**MCP Apps caveats.** The MCP Apps sandbox runs on a **separate** port — `MCP_SANDBOX_PORT`, defaulting to a fixed **`6275`** (#2008; it was OS-assigned before, which meant it changed every run and so could never be named in a `forwardPorts` / `-p` / tunnel config written ahead of time). For the Apps tab to work off loopback, that sandbox port must be independently reachable from the browser — expose/forward `6275` alongside `6274`, or set `MCP_SANDBOX_PORT` to pick another. If the port is already taken the sandbox falls back to an OS-assigned one and warns, so a second Inspector still gets a working Apps tab locally — but the forwarded port is then wrong, which is what the warning tells you. (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. +**MCP Apps caveats.** The MCP Apps sandbox runs on a **separate** port — `MCP_SANDBOX_PORT`, defaulting to a fixed **`6275`** (#2008; it was OS-assigned before, which meant it changed every run and so could never be named in a `forwardPorts` / `-p` / tunnel config written ahead of time). For the Apps tab to work off loopback, that sandbox port must be independently reachable from the browser — expose/forward `6275` alongside `6274`, or set `MCP_SANDBOX_PORT` to pick another. If the port is already taken the sandbox falls back to an OS-assigned one and warns, so a second Inspector still gets a working Apps tab locally — but the forwarded port is then wrong, which is what the warning tells you. (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. (A `*.localhost` embedder is fine — `*.localhost` *is* a legal CSP host-source, and the sandbox's `frame-ancestors` admits it whenever the origin allow-list does.) 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. diff --git a/clients/web/server/app-origin-controller.ts b/clients/web/server/app-origin-controller.ts index 12e028c6d5..068c7fe125 100644 --- a/clients/web/server/app-origin-controller.ts +++ b/clients/web/server/app-origin-controller.ts @@ -162,6 +162,14 @@ export interface AppOriginControllerOptions { * loopback, exactly as the sandbox proxy's own `frame-ancestors` does. */ embedderOrigins?: string[]; + /** + * Also admit `*.localhost` embedders (#1944), matching the backend's origin + * guard and the sandbox proxy's own `frame-ancestors`. The app document is + * framed by the sandbox proxy, not by the inspector page directly — but the + * proxy is reached through the same browsing context, so the three have to + * agree or the innermost frame blanks. + */ + allowLocalhostSubdomains?: boolean; } /** A document handed to {@link AppOriginController.publish}. */ @@ -237,7 +245,12 @@ export function createAppOriginController( // Same defaulting rationale as the sandbox controller: never the *name* // `localhost`, which resolves to a single address family and would put this // listener on a different family than the web server (#1951). - const { port, host = DEFAULT_BIND_HOST, embedderOrigins } = options; + const { + port, + host = DEFAULT_BIND_HOST, + embedderOrigins, + allowLocalhostSubdomains, + } = options; let server: Server | null = null; let origin: string | null = null; @@ -256,7 +269,9 @@ export function createAppOriginController( documents.delete(id); } - const FRAME_ANCESTORS = frameAncestorsDirective(embedderOrigins); + const FRAME_ANCESTORS = frameAncestorsDirective(embedderOrigins, { + allowLocalhostSubdomains, + }); /** Drop everything past its TTL. Cheap: the map is bounded by MAX_DOCUMENTS. */ function evictExpired(now: number): void { diff --git a/clients/web/server/sandbox-controller.ts b/clients/web/server/sandbox-controller.ts index 042e8b3f34..cfefcc43aa 100644 --- a/clients/web/server/sandbox-controller.ts +++ b/clients/web/server/sandbox-controller.ts @@ -29,6 +29,11 @@ export interface SandboxControllerOptions { * list (only a hand-constructed caller or a test) falls back to loopback-only. */ allowedOrigins?: string[]; + /** + * Also admit `*.localhost` embedders in the proxy's `frame-ancestors` + * (#1944), matching the backend's origin guard. + */ + allowLocalhostSubdomains?: boolean; } // Loopback fallback. NB: no `http://[::1]:*` — a **bracketed IPv6 literal is not @@ -40,6 +45,24 @@ export interface SandboxControllerOptions { // 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:*"]; +/** + * `frame-ancestors` sources for the RFC 6761 `*.localhost` space, appended when + * the backend is accepting those origins (#1944). Both schemes and any port, so + * this matches the origin guard's predicate exactly — a CSP that admitted fewer + * embedders than the guard would let a connect succeed and then blank the MCP + * Apps frame, which is the confusing split this pair exists to prevent. + * + * `*.localhost` is a legal CSP `host-source`: the CSP3 grammar allows a leading + * `"*."` before the host, unlike the bracketed IPv6 literal noted above. These + * are trusted constants emitted as-is and deliberately do NOT pass + * {@link CSP_HOST_SOURCE}, for the same reason {@link LOOPBACK_FRAME_ANCESTORS} + * does not. + */ +const LOCALHOST_SUBDOMAIN_FRAME_ANCESTORS = [ + "http://*.localhost:*", + "https://*.localhost:*", +]; + /** * A well-formed CSP host-source: `scheme://host[:port]` with no whitespace, CSP * metacharacters, or brackets. `allowedOrigins` comes from @@ -69,8 +92,19 @@ const CSP_HOST_SOURCE = /^[a-z][a-z0-9+.-]*:\/\/[^\s;,'"[\]*]+$/i; * 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 { - return frameAncestorsDirective(allowedOrigins); +export function sandboxFrameAncestors( + allowedOrigins?: string[], + options?: FrameAncestorsOptions, +): string { + return frameAncestorsDirective(allowedOrigins, options); +} + +export interface FrameAncestorsOptions { + /** + * Also admit `*.localhost` embedders (#1944). Mirrors the backend's + * `allowLocalhostSubdomainOrigins`; both are set from the same value. + */ + allowLocalhostSubdomains?: boolean; } /** @@ -85,10 +119,20 @@ export function sandboxFrameAncestors(allowedOrigins?: string[]): string { * sources means `'none'` and would block the frame outright — so the two * derive it here rather than each rolling one. */ -export function frameAncestorsDirective(origins?: string[]): string { +export function frameAncestorsDirective( + origins?: string[], + options?: FrameAncestorsOptions, +): string { const valid = (origins ?? []).filter((o) => CSP_HOST_SOURCE.test(o)); const sources = valid.length > 0 ? valid : LOOPBACK_FRAME_ANCESTORS; - return `frame-ancestors ${sources.join(" ")}`; + // Appended rather than folded into the fallback: the wildcard is additive to + // whatever exact embedders were derived, and it has to survive the + // `valid.length > 0` branch — which is the branch the real backend always + // takes. + const withLocalhostSubdomains = options?.allowLocalhostSubdomains + ? [...sources, ...LOCALHOST_SUBDOMAIN_FRAME_ANCESTORS] + : sources; + return `frame-ancestors ${withLocalhostSubdomains.join(" ")}`; } export interface SandboxController { @@ -156,7 +200,12 @@ export function createSandboxController( // `localhost` — a name resolves to one address family and would reintroduce // the #1951 split (web on IPv4, sandbox on IPv6) for any future call site // that omits `host`. Both call sites pass `config.sandboxHost` today. - const { port, host = DEFAULT_BIND_HOST, allowedOrigins } = options; + const { + port, + host = DEFAULT_BIND_HOST, + allowedOrigins, + allowLocalhostSubdomains, + } = options; let server: Server | null = null; let sandboxUrl: string | null = null; @@ -170,7 +219,9 @@ export function createSandboxController( // 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); + const SANDBOX_PROXY_CSP = sandboxFrameAncestors(allowedOrigins, { + allowLocalhostSubdomains, + }); let sandboxHtml: string; try { diff --git a/clients/web/server/server.ts b/clients/web/server/server.ts index f128685cd8..b95dfb01cf 100644 --- a/clients/web/server/server.ts +++ b/clients/web/server/server.ts @@ -46,6 +46,7 @@ export async function startHonoServer( port: config.sandboxPort, host: config.sandboxHost, allowedOrigins: config.allowedOrigins, + allowLocalhostSubdomains: config.allowLocalhostSubdomainOrigins, }); await sandboxController.start(); // The dedicated origin apps declaring `_meta.ui.domain` are served from @@ -58,6 +59,7 @@ export async function startHonoServer( sandboxController.getUrl(), config.allowedOrigins, ), + allowLocalhostSubdomains: config.allowLocalhostSubdomainOrigins, }); await appOriginController.start(); @@ -80,6 +82,7 @@ export async function startHonoServer( writable: config.writable, initialServers: config.initialServers ?? undefined, allowedOrigins: config.allowedOrigins, + allowLocalhostSubdomainOrigins: config.allowLocalhostSubdomainOrigins, sandboxUrl: sandboxController.getUrl() ?? undefined, publishAppDocument: (doc) => appOriginController.publish(doc), logger: config.logger, diff --git a/clients/web/server/vite-hono-plugin.ts b/clients/web/server/vite-hono-plugin.ts index 59cee7304f..d6d669dd33 100644 --- a/clients/web/server/vite-hono-plugin.ts +++ b/clients/web/server/vite-hono-plugin.ts @@ -69,6 +69,7 @@ export function honoMiddlewarePlugin(config: WebServerConfig): Plugin { port: config.sandboxPort, host: config.sandboxHost, allowedOrigins: config.allowedOrigins, + allowLocalhostSubdomains: config.allowLocalhostSubdomainOrigins, }); await sandboxController.start(); // The dedicated origin apps declaring `_meta.ui.domain` are served from @@ -81,6 +82,7 @@ export function honoMiddlewarePlugin(config: WebServerConfig): Plugin { sandboxController.getUrl(), config.allowedOrigins, ), + allowLocalhostSubdomains: config.allowLocalhostSubdomainOrigins, }); await appOriginController.start(); // Resolved before the API is built so `/api/config` and the banner @@ -99,6 +101,7 @@ export function honoMiddlewarePlugin(config: WebServerConfig): Plugin { writable: config.writable, initialServers: config.initialServers ?? undefined, allowedOrigins: config.allowedOrigins, + allowLocalhostSubdomainOrigins: config.allowLocalhostSubdomainOrigins, sandboxUrl: sandboxController.getUrl() ?? undefined, publishAppDocument: (doc) => appOriginController.publish(doc), logger: config.logger, diff --git a/clients/web/server/web-server-config.ts b/clients/web/server/web-server-config.ts index 5c0b23e1d8..09a9c978ae 100644 --- a/clients/web/server/web-server-config.ts +++ b/clients/web/server/web-server-config.ts @@ -64,6 +64,19 @@ export interface WebServerConfig { initialServers: MCPConfig | null; storageDir: string | undefined; allowedOrigins: string[]; + /** + * Accept any http(s) origin on a `*.localhost` host in addition to + * {@link allowedOrigins} (#1944), and admit the same set as MCP Apps frame + * ancestors. + * + * True only when the server is falling back to {@link defaultAllowedOrigins} + * *and* the bind host actually serves loopback — see + * {@link allowLocalhostSubdomainOriginsFor}. An operator-supplied + * `ALLOWED_ORIGINS` replaces the default list and is honoured exactly, so it + * turns this off: a list that says which origins are allowed should not + * silently gain entries. + */ + allowLocalhostSubdomainOrigins: boolean; /** Sandbox port (0 = dynamic). */ sandboxPort: number; sandboxHost: string; @@ -288,6 +301,38 @@ function loopbackOrigins(port: number): string[] { ]; } +/** + * Whether the default origin allow-list for `hostname` should additionally + * admit `*.localhost` origins (#1944). + * + * `*.localhost` is reserved to the loopback interface by + * [RFC 6761 §6.3](https://www.rfc-editor.org/info/rfc6761/), and Chrome and + * Firefox resolve it there internally without any hosts-file entry — so a + * developer fronting the Inspector with a local reverse proxy (`mcp.localhost`, + * `tenant.app.localhost`) reaches a server that is bound to loopback. Vite, + * Django and Rails all default-allow the same suffix in their own host + * allow-lists, which is the company this keeps. + * + * It does not weaken the DNS-rebinding guard the allow-list exists to be. An + * attacker's document cannot *obtain* a `.localhost` origin: rebinding works by + * pointing a name the attacker controls at `127.0.0.1`, and the `.localhost` + * suffix is reserved and not publicly registrable, so there is no such name to + * control. The residual case — a hostile local resolver, or another local dev + * server the user has browsed on some `*.localhost` name — is the position bare + * `localhost` is already in, and `/api/*` still requires the bearer token + * regardless. + * + * Gated on the bind host serving loopback at all, which keeps the widening to + * the case that motivates it. Bound to a specific non-loopback address, a + * browser at `foo.localhost` resolves to `127.0.0.1` and never reaches this + * process, so admitting the origin there would be a no-op that only made the + * allow-list harder to reason about. + */ +export function allowLocalhostSubdomainOriginsFor(hostname: string): boolean { + const h = canonicalUrlHost(hostname); + return LOOPBACK_HOSTNAMES.has(h) || isAllInterfacesHost(h); +} + /** * The default allowed-origins list for a given bind host/port. * @@ -474,6 +519,8 @@ export function buildWebServerConfig( allowedOrigins: configuredOrigins?.length ? configuredOrigins : defaultAllowedOrigins(hostname, port), + allowLocalhostSubdomainOrigins: + !configuredOrigins?.length && allowLocalhostSubdomainOriginsFor(hostname), sandboxPort, sandboxHost: hostname, appOriginPort, diff --git a/clients/web/src/hooks/useConnectionLifecycle.ts b/clients/web/src/hooks/useConnectionLifecycle.ts index f7064c0fcc..a24aaec595 100644 --- a/clients/web/src/hooks/useConnectionLifecycle.ts +++ b/clients/web/src/hooks/useConnectionLifecycle.ts @@ -30,6 +30,7 @@ import { getActiveEnterpriseManagedAuthIdp, } from "@inspector/core/client/types.js"; import { isEmaClientNotConfiguredError } from "@inspector/core/auth/ema/clientConfigError.js"; +import { showInsecureTokenEndpointNotice } from "../lib/insecureTokenEndpointNotice"; import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; import type { RemoteInspectorClientStorage } from "@inspector/core/mcp/remote/index.js"; import type { SessionRef } from "./useSessionRef"; @@ -637,6 +638,13 @@ export function useConnectionLifecycle({ }); return; } + // SEP-2207 (#2280): a token endpoint the SDK will not post credentials + // to. Terminal, so it gets the same treatment as the EMA arm above + // rather than the generic "OAuth authorization failed" toast, whose + // detail line would be the raw SDK text. + if (showInsecureTokenEndpointNotice(err, target.name)) { + return; + } // A 401 from an OAuth-protected server means we have no (valid) token // yet. Kick off the authorization-code flow: `authenticate()` runs @@ -722,6 +730,10 @@ export function useConnectionLifecycle({ }); return; } + // See the SEP-2207 note on the handshake arm above (#2280). + if (showInsecureTokenEndpointNotice(authErr, target.name)) { + return; + } // The connect attempt failed, same as any other handshake error — // flag the card (#1621) and, with it, open the monitoring sidebar // onto the OAuth requests that explain the failure (#2108). This diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index a47b94c20b..d0f121839f 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -30,6 +30,7 @@ import { emaStepUpSuccessMessage, } from "@inspector/core/auth/oauthUx.js"; import { isEmaClientNotConfiguredError } from "@inspector/core/auth/ema/clientConfigError.js"; +import { showInsecureTokenEndpointNotice } from "../lib/insecureTokenEndpointNotice"; import type { OAuthDetails } from "../components/groups/ConnectionInfoContent/ConnectionInfoContent"; import { oauthDetailsFromConnectionState } from "../components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState"; import { getWebRemoteOAuthStorage } from "../lib/remoteOAuthStorage"; @@ -390,6 +391,14 @@ export function useOAuthRecovery({ options?: { reason?: AuthChallengeReason }, ) => { const server = sessionRef.current.servers.find((s) => s.id === serverId); + // SEP-2207 (#2280). The SDK rethrows `InsecureTokenEndpointError` instead + // of retrying, so the banner's "Re-authenticate" could only fail the same + // way. Claimed here, at the single funnel every re-auth banner goes + // through, rather than at each of its call sites — a new caller then gets + // the right behavior by default instead of by remembering. + if (showInsecureTokenEndpointNotice(detail, server?.name)) { + return; + } const message = reAuthBannerMessage({ serverName: server?.name, detail: @@ -1320,6 +1329,12 @@ export function useOAuthRecovery({ }); return; } + // Above `setFailedServerId` for the same reason the EMA arm is: this is + // a configuration error, not a failed attempt, so it should not flag + // the card red or pull the monitoring sidebar open. + if (showInsecureTokenEndpointNotice(err, server.name)) { + return; + } // The token exchange (or the re-handshake behind it) failed. Flag the // server (#1621) so the monitoring sidebar opens onto the OAuth // requests that explain it (#2108) — the rebuilt client restored the diff --git a/clients/web/src/lib/insecureTokenEndpointNotice.test.ts b/clients/web/src/lib/insecureTokenEndpointNotice.test.ts new file mode 100644 index 0000000000..c4265fe054 --- /dev/null +++ b/clients/web/src/lib/insecureTokenEndpointNotice.test.ts @@ -0,0 +1,57 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { InsecureTokenEndpointError } from "@modelcontextprotocol/client"; +import { + insecureTokenEndpointMessage, + insecureTokenEndpointTitle, +} from "../utils/oauthUx"; + +const show = vi.fn(); +vi.mock("@mantine/notifications", () => ({ + notifications: { show: (...args: unknown[]) => show(...args) }, +})); + +const { showInsecureTokenEndpointNotice } = + await import("./insecureTokenEndpointNotice"); + +const ENDPOINT = "http://tenant.example.localhost:3300/api/oauth/token"; + +beforeEach(() => { + show.mockClear(); +}); + +describe("showInsecureTokenEndpointNotice", () => { + it("claims the SDK error and shows the terminal notice", () => { + const handled = showInsecureTokenEndpointNotice( + new InsecureTokenEndpointError(ENDPOINT), + "Acme", + ); + + expect(handled).toBe(true); + expect(show).toHaveBeenCalledTimes(1); + expect(show).toHaveBeenCalledWith({ + title: insecureTokenEndpointTitle(), + message: insecureTokenEndpointMessage({ + tokenEndpoint: ENDPOINT, + serverName: "Acme", + }), + color: "red", + // Non-recoverable, so the explanation must not vanish on a timer — there + // is no second chance to read it. + autoClose: false, + }); + }); + + it("works without a server name", () => { + expect( + showInsecureTokenEndpointNotice(new InsecureTokenEndpointError(ENDPOINT)), + ).toBe(true); + expect(show.mock.calls[0][0].message).toContain("this server"); + }); + + it("declines any other error, leaving the caller's handling in place", () => { + expect(showInsecureTokenEndpointNotice(new Error("boom"), "Acme")).toBe( + false, + ); + expect(show).not.toHaveBeenCalled(); + }); +}); diff --git a/clients/web/src/lib/insecureTokenEndpointNotice.ts b/clients/web/src/lib/insecureTokenEndpointNotice.ts new file mode 100644 index 0000000000..44f691ab08 --- /dev/null +++ b/clients/web/src/lib/insecureTokenEndpointNotice.ts @@ -0,0 +1,52 @@ +/** + * Surfaces the SDK's SEP-2207 refusal to post credentials to a non-TLS token + * endpoint as the terminal configuration error it is (#2280). + * + * Lives in `lib/` rather than `utils/` because showing a notification is a side + * effect; the copy it renders is pure and lives in `@inspector/core/auth`. + * + * Shaped as a claim-or-decline predicate rather than a plain `show(...)` so the + * three OAuth failure paths that need it — the connect handshake, the post- + * redirect callback, and the re-auth banner funnel — can each spend one line on + * it and keep their existing fall-through intact: + * + * ```ts + * if (showInsecureTokenEndpointNotice(err, server.name)) return; + * ``` + * + * `autoClose: false` matches the other non-recoverable OAuth notices (issuer + * mismatch, unconfigured enterprise IdP): nothing the user does next will make + * this reappear, so a toast that vanishes takes the only explanation with it. + */ + +import { notifications } from "@mantine/notifications"; +import { isInsecureTokenEndpointError } from "@inspector/core/auth/insecureTokenEndpoint.js"; +import { + insecureTokenEndpointMessage, + insecureTokenEndpointTitle, +} from "../utils/oauthUx"; + +/** + * Show the terminal notice when `err` is the SDK's `InsecureTokenEndpointError`. + * + * @returns `true` when it was handled (the caller should stop), `false` when + * `err` is some other failure and the caller's normal handling applies. + */ +export function showInsecureTokenEndpointNotice( + err: unknown, + serverName?: string, +): boolean { + if (!isInsecureTokenEndpointError(err)) { + return false; + } + notifications.show({ + title: insecureTokenEndpointTitle(), + message: insecureTokenEndpointMessage({ + tokenEndpoint: err.tokenEndpoint, + serverName, + }), + color: "red", + autoClose: false, + }); + return true; +} diff --git a/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts b/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts new file mode 100644 index 0000000000..da513f7175 --- /dev/null +++ b/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts @@ -0,0 +1,78 @@ +import { describe, it, expect } from "vitest"; +import { InsecureTokenEndpointError } from "@modelcontextprotocol/client"; +import { isInsecureTokenEndpointError } from "@inspector/core/auth/insecureTokenEndpoint.js"; + +const ENDPOINT = "http://tenant.example.localhost:3300/api/oauth/token"; + +/** + * Documents the assumption the classifier is built on, the same way + * `issuerBinding.test.ts` does for its sibling: the SDK declares `mcpBrand` in + * a `static {}` block, so it lives on the constructor and instances never carry + * it. A classifier that read `err.mcpBrand` would match no real thrown error. + */ +describe("SDK brand placement", () => { + it("keeps `mcpBrand` on the class, not the instance", () => { + expect("mcpBrand" in new InsecureTokenEndpointError(ENDPOINT)).toBe(false); + }); + + it("is not an OAuthError, which is why the retry path must not claim it", () => { + const err = new InsecureTokenEndpointError(ENDPOINT); + // The SDK deliberately keeps this off the `OAuthError` hierarchy so hosts + // do not treat it as a transient authorization failure. If a future SDK + // changes that, the #2280 handling should be revisited rather than silently + // keeping a now-wrong justification. + expect(err.name).toBe("InsecureTokenEndpointError"); + expect(typeof err.tokenEndpoint).toBe("string"); + }); +}); + +describe("isInsecureTokenEndpointError", () => { + it("recognizes a real SDK error and narrows to its endpoint", () => { + const err: unknown = new InsecureTokenEndpointError(ENDPOINT); + expect(isInsecureTokenEndpointError(err)).toBe(true); + if (isInsecureTokenEndpointError(err)) { + expect(err.tokenEndpoint).toBe(ENDPOINT); + } + }); + + it("recognizes a serialized copy by `name`, where the prototype is gone", () => { + // The fallback arm: a structured clone or JSON hop drops the prototype and + // the brand set but keeps `name`. + expect( + isInsecureTokenEndpointError({ + name: "InsecureTokenEndpointError", + message: "Refusing to send credentials…", + tokenEndpoint: ENDPOINT, + }), + ).toBe(true); + }); + + it("rejects a look-alike carrying the endpoint but not the identity", () => { + // Neither the brand nor the name: some other error that happens to have a + // `tokenEndpoint` field must not be swallowed by the terminal arm. + expect( + isInsecureTokenEndpointError({ tokenEndpoint: ENDPOINT, name: "Error" }), + ).toBe(false); + }); + + it("rejects the right identity with no endpoint to report", () => { + // The copy names the endpoint, so a value that cannot supply one is not + // usable by this path and falls through to the generic handling. + expect( + isInsecureTokenEndpointError({ name: "InsecureTokenEndpointError" }), + ).toBe(false); + expect( + isInsecureTokenEndpointError({ + name: "InsecureTokenEndpointError", + tokenEndpoint: 42, + }), + ).toBe(false); + }); + + it.each([null, undefined, "InsecureTokenEndpointError", 0, new Error("x")])( + "rejects %j", + (value) => { + expect(isInsecureTokenEndpointError(value)).toBe(false); + }, + ); +}); diff --git a/clients/web/src/test/core/auth/oauthUx.test.ts b/clients/web/src/test/core/auth/oauthUx.test.ts index dabe207277..aa3a188350 100644 --- a/clients/web/src/test/core/auth/oauthUx.test.ts +++ b/clients/web/src/test/core/auth/oauthUx.test.ts @@ -6,6 +6,8 @@ import { emaStepUpFailureMessage, emaStepUpInProgressMessage, emaStepUpSuccessMessage, + insecureTokenEndpointMessage, + insecureTokenEndpointTitle, isActionTriggeredOAuthRecovery, isEmaStepUp, isReAuthBannerReason, @@ -437,3 +439,46 @@ describe("oauthUx issuer-binding copy", () => { }); }); }); + +describe("insecureTokenEndpoint copy", () => { + const ENDPOINT = "http://tenant.example.localhost:3300/api/oauth/token"; + + it("names the failure as a configuration problem, not an auth failure", () => { + expect(insecureTokenEndpointTitle()).toBe("Token endpoint is not secure"); + }); + + it("names the endpoint, the server, and both ways out", () => { + const message = insecureTokenEndpointMessage({ + tokenEndpoint: ENDPOINT, + serverName: "Acme", + }); + expect(message).toContain('"Acme"'); + expect(message).toContain(ENDPOINT); + expect(message).toContain("HTTPS"); + expect(message).toContain("127.0.0.1"); + expect(message).toContain("Token URL"); + }); + + it("says a retry cannot help, which is the whole point of the message", () => { + // The bug this copy fixes (#2280) was a Re-authenticate button that could + // never succeed. If this sentence goes, the copy stops doing its job. + expect(insecureTokenEndpointMessage({ tokenEndpoint: ENDPOINT })).toContain( + "Re-authenticating cannot change this", + ); + }); + + it("falls back to a generic subject with no server name", () => { + const message = insecureTokenEndpointMessage({ tokenEndpoint: ENDPOINT }); + expect(message).toContain("this server"); + expect(message).not.toContain('""'); + }); + + it("bounds a hostile-length endpoint for display", () => { + // The endpoint is remote-supplied (it comes from the server's AS metadata), + // so an overlong value must not be echoed back whole into the layout. + const long = `https://example.com/${"a".repeat(500)}`; + const message = insecureTokenEndpointMessage({ tokenEndpoint: long }); + expect(message).not.toContain(long); + expect(message).toContain("…"); + }); +}); diff --git a/clients/web/src/test/core/node/hostUrl.test.ts b/clients/web/src/test/core/node/hostUrl.test.ts index baf12d2269..86319a46a7 100644 --- a/clients/web/src/test/core/node/hostUrl.test.ts +++ b/clients/web/src/test/core/node/hostUrl.test.ts @@ -3,6 +3,8 @@ import { canonicalUrlHost, formatHostForUrl, isAllInterfacesHost, + isLocalhostSubdomainHost, + isLocalhostSubdomainOrigin, isLoopbackHost, stripBrackets, } from "@inspector/core/node/hostUrl.js"; @@ -162,3 +164,74 @@ describe("canonicalUrlHost", () => { expect(canonicalUrlHost("")).toBe(""); }); }); + +describe("isLocalhostSubdomainHost", () => { + it.each([ + "app.localhost", + "tenant.example.localhost", + // Canonicalized first, so casing and a root FQDN dot are normalized rather + // than rejected — the browser sends the canonical form in `Origin`. + "APP.LOCALHOST", + "app.localhost.", + // IDNA-mapped to punycode by `new URL`, exactly as a browser would. + "münchen.localhost", + ])("accepts %j", (host) => { + expect(isLocalhostSubdomainHost(host)).toBe(true); + }); + + it.each([ + // The bare TLD is deliberately NOT matched: callers that want it carry it + // as a literal origin already. + "localhost", + // Degenerate labels. + ".localhost", + "a..localhost", + // The suffix trap: `.localhost` must be the END of the name, not a label + // in the middle of an attacker-registrable one. + "app.localhost.evil.com", + "localhost.evil.com", + // Near-misses that must not be read as the reserved suffix. + "notlocalhost", + "mylocalhost", + "localhosts", + "evil.com", + "127.0.0.1", + "[::1]", + "", + ])("rejects %j", (host) => { + expect(isLocalhostSubdomainHost(host)).toBe(false); + }); +}); + +describe("isLocalhostSubdomainOrigin", () => { + it.each([ + "http://app.localhost", + "http://app.localhost:3300", + // Any port, because the point is a reverse proxy on a port we don't know. + "http://tenant.example.localhost:8080", + // Both schemes: a locally-trusted certificate (mkcert) makes the same host + // arrive over https. + "https://app.localhost", + "https://app.localhost:8443", + ])("accepts %j", (origin) => { + expect(isLocalhostSubdomainOrigin(origin)).toBe(true); + }); + + it.each([ + // The opaque-origin header value a sandboxed iframe or `data:` document + // sends. Must never match — allow-listing it would erode the guard. + "null", + "http://localhost:6274", + "http://evil.com", + "http://app.localhost.evil.com", + // Non-http(s) schemes, including ones that parse. + "ws://app.localhost", + "file://app.localhost", + "chrome-extension://app.localhost", + // Not a parseable URL at all. + "app.localhost:3300", + "", + ])("rejects %j", (origin) => { + expect(isLocalhostSubdomainOrigin(origin)).toBe(false); + }); +}); diff --git a/clients/web/src/test/integration/mcp/remote/transport.test.ts b/clients/web/src/test/integration/mcp/remote/transport.test.ts index 98feef7ef8..5c8aa870b4 100644 --- a/clients/web/src/test/integration/mcp/remote/transport.test.ts +++ b/clients/web/src/test/integration/mcp/remote/transport.test.ts @@ -42,6 +42,8 @@ interface StartRemoteServerOptions { logger?: pino.Logger; storageDir?: string; allowedOrigins?: string[]; + /** #1944: additionally accept any http(s) origin on a `*.localhost` host. */ + allowLocalhostSubdomainOrigins?: boolean; /** When true, API routes do not require x-mcp-remote-auth (token is still returned as empty string) */ dangerouslyOmitAuth?: boolean; } @@ -58,6 +60,7 @@ async function startRemoteServer( logger: options.logger, storageDir: options.storageDir, allowedOrigins: options.allowedOrigins, + allowLocalhostSubdomainOrigins: options.allowLocalhostSubdomainOrigins, dangerouslyOmitAuth: options.dangerouslyOmitAuth, initialConfig: { defaultEnvironment: {} }, }); @@ -1294,6 +1297,100 @@ describe("Remote transport e2e", () => { // Split by server config: 5 tests use { allowedOrigins } and share one // server; the "not configured" case needs its own server and is split into // its own describe so each block has symmetric beforeAll/afterAll cleanup. + describe("with allowLocalhostSubdomainOrigins (#1944)", () => { + let sharedServer: ServerType; + let baseUrl: string; + let authToken: string; + + beforeAll(async () => { + const started = await startRemoteServer(0, { + allowedOrigins: ["http://localhost:3000"], + allowLocalhostSubdomainOrigins: true, + }); + sharedServer = started.server; + baseUrl = started.baseUrl; + authToken = started.authToken; + }); + + afterAll(async () => { + await new Promise((resolve, reject) => { + sharedServer.close((err) => (err ? reject(err) : resolve())); + }); + }); + + const connect = (origin: string) => + fetch(`${baseUrl}/api/mcp/connect`, { + method: "POST", + headers: { + "Content-Type": "application/json", + "x-mcp-remote-auth": `Bearer ${authToken}`, + Origin: origin, + }, + body: JSON.stringify({ + config: { type: "sse" as const, url: "http://localhost:3000" }, + }), + }); + + it.each([ + "http://mcp.localhost", + // Any port: the whole point is a reverse proxy on a port the Inspector + // does not know. + "http://tenant.example.localhost:3300", + // Both schemes: a locally-trusted cert makes the same host arrive over + // https. + "https://mcp.localhost:8443", + ])("allows the *.localhost origin %j", async (origin) => { + const res = await connect(origin); + expect(res.status).not.toBe(403); + }); + + it("still honours the exact allow-list it was given", async () => { + const res = await connect("http://localhost:3000"); + expect(res.status).not.toBe(403); + }); + + it.each([ + "http://evil.com", + // The suffix trap: an attacker-registrable name that merely *contains* + // the reserved label must not pass. + "http://mcp.localhost.evil.com", + "http://notlocalhost", + ])("still blocks %j", async (origin) => { + const res = await connect(origin); + expect(res.status).toBe(403); + const json = (await res.json()) as { error?: string }; + expect(json.error).toBe("Forbidden"); + }); + + it("answers a preflight from a *.localhost origin and echoes it back", async () => { + // The preflight and the real request read one predicate, so they cannot + // disagree — a CORS pass with a 403 on the POST would be the worst of + // both. + const res = await fetch(`${baseUrl}/api/mcp/connect`, { + method: "OPTIONS", + headers: { + Origin: "http://mcp.localhost", + "Access-Control-Request-Method": "POST", + }, + }); + expect(res.status).toBe(204); + expect(res.headers.get("Access-Control-Allow-Origin")).toBe( + "http://mcp.localhost", + ); + }); + + it("blocks a preflight from a non-localhost origin", async () => { + const res = await fetch(`${baseUrl}/api/mcp/connect`, { + method: "OPTIONS", + headers: { + Origin: "http://evil.com", + "Access-Control-Request-Method": "POST", + }, + }); + expect(res.status).toBe(403); + }); + }); + describe("with allowedOrigins configured", () => { let sharedServer: ServerType; let baseUrl: string; @@ -1389,6 +1486,24 @@ describe("Remote transport e2e", () => { ); }); + it("blocks a *.localhost origin while the widening is off (#1944)", async () => { + // The default is closed. This is the control for the block below: it is + // what makes the passes there attributable to the flag rather than to + // `.localhost` having been allowed all along. + const res = await fetch(`${baseUrl}/api/mcp/connect`, { + method: "POST", + headers: { + "Content-Type": "application/json", + "x-mcp-remote-auth": `Bearer ${authToken}`, + Origin: "http://mcp.localhost", + }, + body: JSON.stringify({ + config: { type: "sse" as const, url: "http://localhost:3000" }, + }), + }); + expect(res.status).toBe(403); + }); + it("blocks CORS preflight requests with invalid origin", async () => { const res = await fetch(`${baseUrl}/api/mcp/connect`, { method: "OPTIONS", diff --git a/clients/web/src/test/integration/server/app-origin-controller.test.ts b/clients/web/src/test/integration/server/app-origin-controller.test.ts index 92554db38e..605a43ab3c 100644 --- a/clients/web/src/test/integration/server/app-origin-controller.test.ts +++ b/clients/web/src/test/integration/server/app-origin-controller.test.ts @@ -190,6 +190,24 @@ describe("createAppOriginController", () => { ); }); + it("admits *.localhost embedders when the backend does (#1944)", async () => { + // The app document is framed by the sandbox proxy, which is itself reached + // from the inspector page — so all three `frame-ancestors` have to agree or + // the innermost frame blanks. + controller = createAppOriginController({ + port: 0, + host: "127.0.0.1", + embedderOrigins: ["http://127.0.0.1:6275"], + allowLocalhostSubdomains: true, + }); + await controller.start(); + const published = controller.publish({ html: "

x

" })!; + const res = await fetch(published.url); + expect(res.headers.get("content-security-policy")).toBe( + "frame-ancestors http://127.0.0.1:6275 http://*.localhost:* https://*.localhost:*", + ); + }); + it("mints a distinct, unguessable id per document", async () => { controller = createAppOriginController({ port: 0, host: "127.0.0.1" }); const { url } = await controller.start(); 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 6614c06e5f..f653ce5c57 100644 --- a/clients/web/src/test/integration/server/sandbox-controller.test.ts +++ b/clients/web/src/test/integration/server/sandbox-controller.test.ts @@ -52,6 +52,68 @@ describe("sandboxFrameAncestors", () => { sandboxFrameAncestors(["http://a:1; sandbox", "http://[::1]:6274"]), ).toBe("frame-ancestors http://127.0.0.1:* http://localhost:*"); }); + + describe("allowLocalhostSubdomains (#1944)", () => { + it("appends both schemes at any port when enabled", () => { + // Must match the origin guard's predicate exactly — a CSP admitting fewer + // embedders than the guard would let a connect succeed and then blank the + // MCP Apps frame. + expect( + sandboxFrameAncestors(["http://localhost:6274"], { + allowLocalhostSubdomains: true, + }), + ).toBe( + "frame-ancestors http://localhost:6274 http://*.localhost:* https://*.localhost:*", + ); + }); + + it("appends to the derived sources rather than replacing them", () => { + // The real backend always takes the `valid.length > 0` branch, so an + // implementation that only widened the fallback would be dead in + // production while still passing a naive test. + const directive = sandboxFrameAncestors( + ["http://192.168.1.50:6274", "https://inspector.example.com"], + { allowLocalhostSubdomains: true }, + ); + expect(directive).toContain("http://192.168.1.50:6274"); + expect(directive).toContain("https://inspector.example.com"); + expect(directive).toContain("http://*.localhost:*"); + }); + + it("widens the loopback fallback too", () => { + expect( + sandboxFrameAncestors(undefined, { allowLocalhostSubdomains: true }), + ).toBe( + "frame-ancestors http://127.0.0.1:* http://localhost:* http://*.localhost:* https://*.localhost:*", + ); + }); + + it.each([[undefined], [{}], [{ allowLocalhostSubdomains: false }]])( + "adds nothing for options %j", + (options) => { + expect( + sandboxFrameAncestors( + ["http://localhost:6274"], + options as { allowLocalhostSubdomains?: boolean } | undefined, + ), + ).toBe("frame-ancestors http://localhost:6274"); + }, + ); + + it("still drops a caller-supplied wildcard from the allow-list", () => { + // The flag admits `*.localhost` and nothing else. An arbitrary wildcard + // reaching the directive through `allowedOrigins` is the widening + // CSP_HOST_SOURCE exists to prevent, and enabling this must not undo it. + const directive = sandboxFrameAncestors( + ["http://good.example:6274", "http://*.evil.com"], + { allowLocalhostSubdomains: true }, + ); + expect(directive).not.toContain("evil.com"); + expect(directive).toBe( + "frame-ancestors http://good.example:6274 http://*.localhost:* https://*.localhost:*", + ); + }); + }); }); describe("resolveSandboxPort", () => { diff --git a/clients/web/src/test/integration/server/server-auto-open.test.ts b/clients/web/src/test/integration/server/server-auto-open.test.ts index 003ece9f6d..1a7042ac78 100644 --- a/clients/web/src/test/integration/server/server-auto-open.test.ts +++ b/clients/web/src/test/integration/server/server-auto-open.test.ts @@ -122,6 +122,7 @@ describe("startHonoServer autoOpen", () => { initialServers: null, storageDir: undefined, allowedOrigins: [baseUrl], + allowLocalhostSubdomainOrigins: false, sandboxPort: 0, appOriginPort: 0, sandboxHost: "127.0.0.1", diff --git a/clients/web/src/test/integration/server/server-token-injection.test.ts b/clients/web/src/test/integration/server/server-token-injection.test.ts index a39ddc17f5..3c71589c44 100644 --- a/clients/web/src/test/integration/server/server-token-injection.test.ts +++ b/clients/web/src/test/integration/server/server-token-injection.test.ts @@ -72,6 +72,7 @@ describe("startHonoServer index.html token injection (/ -> /api/*)", () => { storageDir: undefined, // Allow the same-origin requests the test issues below. allowedOrigins: [baseUrl], + allowLocalhostSubdomainOrigins: false, sandboxPort: 0, appOriginPort: 0, sandboxHost: "127.0.0.1", 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 51a56993fa..91f522fba0 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 @@ -2,6 +2,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { buildWebServerConfig, buildWebServerConfigFromEnv, + allowLocalhostSubdomainOriginsFor, defaultAllowedOrigins, printServerBanner, webServerConfigToInitialPayload, @@ -43,6 +44,7 @@ const baseConfig = (): WebServerConfig => ({ initialServers: null, storageDir: undefined, allowedOrigins: ["http://localhost:6274"], + allowLocalhostSubdomainOrigins: true, sandboxPort: 0, appOriginPort: 0, sandboxHost: "127.0.0.1", @@ -85,6 +87,7 @@ describe("buildWebServerConfigFromEnv", () => { "http://127.0.0.1:6274", "http://[::1]:6274", ]); + expect(cfg.allowLocalhostSubdomainOrigins).toBe(true); expect(cfg.sandboxPort).toBe(DEFAULT_SANDBOX_PORT); expect(cfg.sandboxHost).toBe("127.0.0.1"); expect(cfg.logger).toBeUndefined(); @@ -177,6 +180,34 @@ describe("buildWebServerConfigFromEnv", () => { expect(cfg.allowedOrigins).toEqual(["http://a:1", "http://b:2"]); }); + it("turns the *.localhost widening off when ALLOWED_ORIGINS is set (#1944)", () => { + // ALLOWED_ORIGINS *replaces* the default list, as documented. A list that + // states which origins are allowed must not silently gain entries. + process.env.ALLOWED_ORIGINS = "http://mcp.localhost"; + const cfg = buildWebServerConfigFromEnv(); + expect(cfg.allowedOrigins).toEqual(["http://mcp.localhost"]); + expect(cfg.allowLocalhostSubdomainOrigins).toBe(false); + }); + + it.each(["", " ", ","])( + "keeps the *.localhost widening on when ALLOWED_ORIGINS is the empty value %j", + (value) => { + // Nothing survives parsing, so the default list is in use and the flag + // must follow it rather than the raw presence of the env var. + process.env.ALLOWED_ORIGINS = value; + expect(buildWebServerConfigFromEnv().allowLocalhostSubdomainOrigins).toBe( + true, + ); + }, + ); + + it("turns the *.localhost widening off for a specific non-loopback HOST", () => { + process.env.HOST = "192.168.1.50"; + expect(buildWebServerConfigFromEnv().allowLocalhostSubdomainOrigins).toBe( + false, + ); + }); + it.each(["", " ", ","])( "falls back to the default (not an empty allow-all) when ALLOWED_ORIGINS is %j", (value) => { @@ -392,6 +423,38 @@ describe("buildWebServerConfigFromEnv", () => { }); }); +describe("allowLocalhostSubdomainOriginsFor (#1944)", () => { + it.each([ + // Loopback binds: the browser resolves `*.localhost` to 127.0.0.1, so the + // proxy in front of this process really does reach it. + "127.0.0.1", + "localhost", + "::1", + "[::1]", + // Non-canonical spellings of the same addresses. + "127.1", + "2130706433", + "0:0:0:0:0:0:0:1", + // All-interfaces binds serve loopback too (the Docker opt-in path). + "0.0.0.0", + "::", + "", + "0", + ])("enables it for the loopback-serving bind host %j", (host) => { + expect(allowLocalhostSubdomainOriginsFor(host)).toBe(true); + }); + + it.each(["192.168.1.50", "127.0.0.2", "inspector.example.com", "10.0.0.1"])( + "leaves it off for the specific non-loopback bind host %j", + (host) => { + // A browser at `foo.localhost` resolves to 127.0.0.1 and never reaches a + // process bound only to one of these, so admitting the origin would be a + // no-op that only made the allow-list harder to reason about. + expect(allowLocalhostSubdomainOriginsFor(host)).toBe(false); + }, + ); +}); + describe("defaultAllowedOrigins", () => { it.each(["localhost", "127.0.0.1", "::1", "[::1]", "LOCALHOST"])( "expands the loopback host %s into all three loopback origins", diff --git a/clients/web/src/utils/oauthUx.ts b/clients/web/src/utils/oauthUx.ts index eeaa94361d..2c8438c135 100644 --- a/clients/web/src/utils/oauthUx.ts +++ b/clients/web/src/utils/oauthUx.ts @@ -14,6 +14,8 @@ export { lostAuthorizationStateTitle, issuerMismatchMessage, issuerMismatchTitle, + insecureTokenEndpointMessage, + insecureTokenEndpointTitle, type OAuthInteractiveAuthKind, type OAuthPreRedirectContext, type OAuthRecoverySource, diff --git a/core/auth/insecureTokenEndpoint.ts b/core/auth/insecureTokenEndpoint.ts new file mode 100644 index 0000000000..49f2e8a39a --- /dev/null +++ b/core/auth/insecureTokenEndpoint.ts @@ -0,0 +1,58 @@ +/** + * SEP-2207: the SDK refuses to send credentials to a non-TLS token endpoint + * whose host is outside its loopback exemption (`localhost` / `127.0.0.1` / + * `::1`), throwing `InsecureTokenEndpointError` from inside + * `executeTokenRequest`. + * + * That error is **terminal by design**. It does not extend `OAuthError`, and + * `auth()` special-cases it to rethrow rather than fall through to a fresh + * `/authorize` redirect — so nothing the Inspector does can make a retry + * succeed. Recognizing it is what lets the UI say so, instead of offering a + * "Re-authenticate" affordance that can only fail the same way (#2280). + * + * The check is not a fix for `*.localhost` (#1944): the exemption list lives in + * the SDK and takes no options, so widening it has to happen upstream + * (typescript-sdk#2591). This is about how the refusal is *reported* — which + * matters for every insecure endpoint, `host.docker.internal` and LAN hostnames + * included, not only the `.localhost` case. + */ + +import { InsecureTokenEndpointError } from "@modelcontextprotocol/client"; + +/** The fields this module needs off the SDK error, once recognized. */ +export interface InsecureTokenEndpointShape { + /** The token endpoint URL the SDK refused to post credentials to. */ + tokenEndpoint: string; +} + +/** + * Recognize the SDK's `InsecureTokenEndpointError`. + * + * Uses the SDK's own `isInstance` predicate, which is cross-copy safe by + * construction: the SDK stamps each instance with a brand set keyed by + * `Symbol.for("mcp.sdk.errorBrands")` and overrides `Symbol.hasInstance` to + * consult it, so an error thrown by a different bundled copy still matches. + * (The brand constant is `static`, so it is never reachable as `err.mcpBrand` + * on an instance — don't check that property.) + * + * The `name` comparison is the same deliberate serialization fallback + * `isAuthorizationServerMismatchShape` carries in `issuerBinding.ts`: today the + * web client runs `auth()` in the browser, so no boundary is crossed, but the + * prototype and brand set are the first things a structured clone or a JSON hop + * would drop, and `name` survives both. + */ +export function isInsecureTokenEndpointError( + err: unknown, +): err is InsecureTokenEndpointShape { + if (err === null || typeof err !== "object") { + return false; + } + const candidate = err as { tokenEndpoint?: unknown; name?: unknown }; + if (typeof candidate.tokenEndpoint !== "string") { + return false; + } + return ( + InsecureTokenEndpointError.isInstance(err) || + candidate.name === "InsecureTokenEndpointError" + ); +} diff --git a/core/auth/oauthUx.ts b/core/auth/oauthUx.ts index 7c07f23319..f12ca1c12e 100644 --- a/core/auth/oauthUx.ts +++ b/core/auth/oauthUx.ts @@ -299,11 +299,16 @@ export function issuerMismatchTitle(): string { */ const MAX_DISPLAYED_ISSUER_LENGTH = 120; +/** Bound a remote-supplied URL for display, marking any truncation. */ +export function truncateUrlForDisplay(url: string): string { + return url.length > MAX_DISPLAYED_ISSUER_LENGTH + ? `${url.slice(0, MAX_DISPLAYED_ISSUER_LENGTH)}…` + : url; +} + /** Bound an issuer for display, marking any truncation. */ export function truncateIssuerForDisplay(issuer: string): string { - return issuer.length > MAX_DISPLAYED_ISSUER_LENGTH - ? `${issuer.slice(0, MAX_DISPLAYED_ISSUER_LENGTH)}…` - : issuer; + return truncateUrlForDisplay(issuer); } /** @@ -357,3 +362,43 @@ export function reAuthBannerMessage(options: { : "Authentication needs attention."; return options.detail ? `${prefix} ${options.detail}` : prefix; } + +/** + * Heading for the SDK's SEP-2207 refusal to post credentials to a non-TLS token + * endpoint (#2280). + * + * Deliberately not phrased as an authentication failure. Like + * {@link issuerMismatchTitle}, this is offered **no** one-click recovery: the + * SDK rethrows `InsecureTokenEndpointError` rather than retrying, so a + * "Re-authenticate" affordance here could only fail the same way, and a button + * that cannot work is worse than no button. + */ +export function insecureTokenEndpointTitle(): string { + return "Token endpoint is not secure"; +} + +/** + * Plain-language explanation and the two things that actually resolve it. + * + * Does not echo the SDK's own message, which names only `localhost`, + * `127.0.0.1` and `::1` as exempt and reads as a flat refusal — accurate, but it + * tells the user nothing about which lever to reach for. The endpoint is + * remote-supplied (it comes from the server's authorization-server metadata), + * so it is bounded for display by {@link truncateUrlForDisplay}; rendering is + * escaped, so that is a layout bound rather than an injection defence. + */ +export function insecureTokenEndpointMessage(options: { + tokenEndpoint: string; + serverName?: string; +}): string { + const target = options.serverName ? `"${options.serverName}"` : "this server"; + return ( + `Authorization for ${target} was stopped before any credentials were sent: ` + + `its token endpoint ${truncateUrlForDisplay(options.tokenEndpoint)} is ` + + "plain HTTP on a host that is not loopback, and OAuth token requests must " + + "use TLS. Re-authenticating cannot change this. " + + "Serve the token endpoint over HTTPS, or point it at a genuinely loopback " + + "host (localhost, 127.0.0.1 or ::1) — Server Settings → Authorization has a " + + "Token URL override if the authorization server advertises a different one." + ); +} diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 69439f6db7..ad1bfb48f8 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -28,6 +28,7 @@ import { bodyLimit } from "hono/body-limit"; import { watch as chokidarWatch, type FSWatcher } from "chokidar"; import { createTransportNode } from "../../node/transport.js"; import { createProxyFetch } from "../../node/proxyFetch.js"; +import { isLocalhostSubdomainOrigin } from "../../../node/hostUrl.js"; import type { RemoteConnectRequest, RemoteSendRequest, @@ -225,6 +226,19 @@ export interface RemoteServerOptions { /** Optional: validate Origin header against allowed origins (for CORS) */ allowedOrigins?: string[]; + /** + * Additionally accept any http(s) origin on a `*.localhost` host, at any port + * (#1944). Off by default. `allowedOrigins` stays a list of **literal** + * origins compared exactly — this is a separate, explicitly-named capability + * rather than a wildcard entry in that list, because the list is also read by + * the sandbox CSP builder and a value the two layers interpret differently is + * the split-behaviour hazard `ALLOWED_ORIGINS` already rejects wildcards to + * avoid. The web backend turns it on only when it is using its own default + * list; an operator-supplied `ALLOWED_ORIGINS` replaces the default and is + * honoured exactly, as documented. + */ + allowLocalhostSubdomainOrigins?: boolean; + /** Optional pino file logger. When set, /api/log forwards received events to it. */ logger?: pino.Logger; @@ -366,9 +380,23 @@ export interface CreateRemoteAppResult { /** * Hono middleware for origin validation (CORS and DNS rebinding protection). - * Validates Origin header against allowedOrigins if provided. + * Validates Origin header against allowedOrigins if provided, plus — when + * `allowLocalhostSubdomainOrigins` is set — any http(s) origin on a + * `*.localhost` host (#1944). */ -function createOriginMiddleware(allowedOrigins?: string[]) { +function createOriginMiddleware( + allowedOrigins?: string[], + options?: { allowLocalhostSubdomainOrigins?: boolean }, +) { + // One predicate for the preflight and the real request, so the two can never + // disagree about what is allowed. Exact-match first: that is the guard, and + // the `*.localhost` arm is an opt-in widening on top of it, not a + // replacement. + const isAllowed = (origin: string): boolean => + (allowedOrigins ?? []).includes(origin) || + (options?.allowLocalhostSubdomainOrigins === true && + isLocalhostSubdomainOrigin(origin)); + return async (c: Context, next: Next) => { // If no allowedOrigins configured, skip validation (allow all) if (!allowedOrigins || allowedOrigins.length === 0) { @@ -380,7 +408,7 @@ function createOriginMiddleware(allowedOrigins?: string[]) { // Handle CORS preflight requests if (c.req.method === "OPTIONS") { - if (origin && allowedOrigins.includes(origin)) { + if (origin && isAllowed(origin)) { c.header("Access-Control-Allow-Origin", origin); c.header("Access-Control-Allow-Methods", "GET, POST, DELETE, OPTIONS"); c.header( @@ -403,7 +431,7 @@ function createOriginMiddleware(allowedOrigins?: string[]) { // For actual requests, validate origin if present if (origin) { - if (!allowedOrigins.includes(origin)) { + if (!isAllowed(origin)) { return c.json( { error: "Forbidden", @@ -579,7 +607,11 @@ export function createRemoteApp( const app = new Hono(); const sessions = new Map(); - const { logger: fileLogger, allowedOrigins } = options; + const { + logger: fileLogger, + allowedOrigins, + allowLocalhostSubdomainOrigins, + } = options; const storageDir = options.storageDir ?? getDefaultStorageDir(); const mcpConfigPath = options.mcpConfigPath ?? getDefaultMcpConfigPath(); const secretStore: SecretStore = options.secretStore ?? defaultSecretStore(); @@ -735,7 +767,12 @@ export function createRemoteApp( // Apply origin validation middleware first (before auth) // This prevents DNS rebinding attacks by validating Origin header - app.use("*", createOriginMiddleware(allowedOrigins)); + app.use( + "*", + createOriginMiddleware(allowedOrigins, { + allowLocalhostSubdomainOrigins, + }), + ); // Apply auth middleware unless dangerously omitted if (!dangerouslyOmitAuth) { diff --git a/core/node/hostUrl.ts b/core/node/hostUrl.ts index 93a9a55c5f..1491f2ccd7 100644 --- a/core/node/hostUrl.ts +++ b/core/node/hostUrl.ts @@ -180,3 +180,62 @@ export function isLoopbackHost(host: string): boolean { /^127(\.(25[0-5]|2[0-4]\d|1?\d?\d)){3}$/.test(h) ); } + +/** + * True when `host` is a name reserved to the loopback interface by + * [RFC 6761 §6.3](https://www.rfc-editor.org/info/rfc6761/) *below* the + * `localhost` TLD — `app.localhost`, `tenant.example.localhost`. The bare + * `localhost` is deliberately **not** matched here: callers that want both ask + * for both, so a caller that only wants the subdomain case (the DNS-rebinding + * guard, which already carries `localhost` as a literal origin) doesn't silently + * widen to it. + * + * Canonicalized through {@link canonicalUrlHost} first, so the comparison sees + * the same lowercased, IDNA-mapped form the browser puts in `Origin` — a + * `Münchén.LOCALHOST` embedder arrives as `xn--mnchn-3ya1b.localhost` and still + * matches. A root FQDN dot is dropped for the same reason it is in + * {@link isLoopbackHost}. Every label left of `.localhost` must be non-empty, so + * the degenerate `.localhost` and `a..localhost` are rejected. + * + * Deliberately separate from {@link isLoopbackHost} rather than folded into it. + * That predicate gates the **OAuth callback listener's bind host**, and this is + * about a host a *browser* resolves: Chrome and Firefox map `*.localhost` to + * loopback internally, but the OS resolver on macOS does not (`dns.lookup` + * returns `ENOTFOUND`), so binding one would fail where browsing one works. + * Widening the bind guard would trade a clear rejection for an obscure listen + * error, which is not an improvement. + */ +export function isLocalhostSubdomainHost(host: string): boolean { + const h = canonicalUrlHost(host).replace(/\.$/, ""); + const suffix = ".localhost"; + if (!h.endsWith(suffix)) return false; + const labels = h.slice(0, -suffix.length).split("."); + return labels.every((label) => label !== ""); +} + +/** + * True when `origin` is an http(s) origin on a `*.localhost` host, at any port. + * + * Both schemes are accepted because the trustworthiness argument is about the + * *host*, not the transport: the suffix is reserved by RFC 6761 and is not + * publicly registrable, so no attacker can obtain such an origin by acquiring a + * domain, and a local proxy fronted with a self-signed certificate (mkcert and + * friends) sends `https://…` for the same machine. Any port, because the whole + * point is a reverse proxy on a port the Inspector does not know. + * + * Callers pass the raw `Origin` header. This parses rather than string-matches + * so that the values a browser really can put there without meaning an http(s) + * origin — `"null"` from an opaque origin (a sandboxed iframe, a `data:` + * document), a non-http scheme — can never match. It reads the parsed host, so + * casing and IDNA spelling are normalized rather than rejected. + */ +export function isLocalhostSubdomainOrigin(origin: string): boolean { + let url: URL; + try { + url = new URL(origin); + } catch { + return false; + } + if (url.protocol !== "http:" && url.protocol !== "https:") return false; + return isLocalhostSubdomainHost(url.hostname); +} diff --git a/docs/test-servers.md b/docs/test-servers.md index c922b58809..a0cd68f677 100644 --- a/docs/test-servers.md +++ b/docs/test-servers.md @@ -52,6 +52,7 @@ as a missing capability rather than an error. | `oauth-custom-resource-metadata-http.json` **(legacy era)** | OAuth discovery driven by the challenge's `resource_metadata` | [#2071](https://github.com/modelcontextprotocol/inspector/issues/2071) | | `oauth-revocation-http.json` / `oauth-no-revocation-http.json` **(legacy era)** | RFC 7009 token revocation on clear, with and without a `revocation_endpoint` | [#2144](https://github.com/modelcontextprotocol/inspector/issues/2144) | | `oauth-rfc8414-at-oidc-path-http.json` **(legacy era)** | Plain OAuth 2.0 AS metadata served at the OIDC well-known path | [#2172](https://github.com/modelcontextprotocol/inspector/issues/2172) | +| `oauth-insecure-token-endpoint-http.json` **(legacy era)** | A token endpoint the SDK refuses to post credentials to (SEP-2207) | [#2280](https://github.com/modelcontextprotocol/inspector/issues/2280) | | `logging-{legacy,modern}-http.json` **(era per file)** | Logging, both eras | [#1629](https://github.com/modelcontextprotocol/inspector/issues/1629) | | `subscriptions-{legacy,modern}-http.json` **(era per file)** | Resource subscriptions, both eras | [#1630](https://github.com/modelcontextprotocol/inspector/issues/1630) | | `subscriptions-never-acknowledged-http.json` **(modern era)** | A `subscriptions/listen` answered with a bare result | [#2097](https://github.com/modelcontextprotocol/inspector/issues/2097) | @@ -428,6 +429,20 @@ The same server is worth running against `--cli` / `--tui`, which reach it by a The value now rides the normalized `AuthChallenge` as a string — it has to be serializable, because the web client's challenge crosses the remote-backend boundary as JSON — and is converted to a `URL` at the OAuth boundary, where it is handed to `auth()` as `resourceMetadataUrl` and to the CIMD pre-registration probe, which runs *before* `auth()` and would otherwise do its own default-location discovery. A malformed value is ignored rather than surfaced, matching the SDK's own `WWW-Authenticate` parser: discovery falls back to the default locations instead of failing the whole authorization on a bad header. The callback leg needs nothing extra — SDK `auth()` persists the URL in its discovery state, so it survives both the web full-page redirect and the CLI/TUI loopback callback. +## A token endpoint the SDK will not use (SEP-2207) + +`oauth-insecure-token-endpoint-http.json` is an ordinary combined AS + resource server with one thing changed: `oauth.issuerUrl` is `http://localhost.:8091`, so its advertised `token_endpoint` is `http://localhost./oauth/token`. Plain streamable-HTTP — connect with the **default (legacy)** protocol era. + +The trailing dot is the whole trick, and it is doing real work rather than being a curiosity. `localhost.` is the *root-anchored* spelling of `localhost`: every resolver on the machine sends it to the loopback interface, so the fixture is reachable and the flow runs for real — but the SDK's `assertSecureTokenEndpoint` exempts only the three literals `localhost`, `127.0.0.1` and `::1`, and `localhost.` is none of them. So the credential-carrying request is refused with `InsecureTokenEndpointError` while everything else about the server works. It is the same over-narrow exemption that makes `http://tenant.app.localhost:3300` fail ([#1944](https://github.com/modelcontextprotocol/inspector/issues/1944), [typescript-sdk#2591](https://github.com/modelcontextprotocol/typescript-sdk/issues/2591)), reproducible without a `/etc/hosts` entry or dnsmasq. + +Add the server, click **Connect**, and complete the authorization. The redirect comes back with a code, the Inspector goes to exchange it, and the SDK refuses. + +What you should see is a red, non-dismissing **"Token endpoint is not secure"** notification naming the endpoint and the two things that resolve it — serve it over HTTPS, or point it at a genuinely loopback host. There is deliberately **no** action button. + +On the broken build you got a **"Re-authentication required"** banner with a **Re-authenticate** button ([#2280](https://github.com/modelcontextprotocol/inspector/issues/2280)). That button could never work: `InsecureTokenEndpointError` does not extend `OAuthError`, and `auth()` special-cases it to rethrow rather than start a fresh `/authorize` redirect, so clicking it re-ran the same flow to the same refusal. The only text on screen was the raw SDK message, which names the three exempt literals and says nothing about which lever to reach for. + +Note that the fix here is presentational only. Making a `*.localhost` token endpoint actually **work** has to land in the SDK — the assertion runs inside `executeTokenRequest`, takes no options, and there is no hook the Inspector could reach. + ## Revoking tokens on clear (RFC 7009) `oauth-revocation-http.json` and `oauth-no-revocation-http.json` are the same OAuth-protected server (combined AS + resource, DCR, refresh tokens) differing in one thing: the first advertises a `revocation_endpoint`, the second advertises none. Plain streamable-HTTP — connect with the **default (legacy)** protocol era. diff --git a/test-servers/configs/oauth-insecure-token-endpoint-http.json b/test-servers/configs/oauth-insecure-token-endpoint-http.json new file mode 100644 index 0000000000..1721f12a9c --- /dev/null +++ b/test-servers/configs/oauth-insecure-token-endpoint-http.json @@ -0,0 +1,20 @@ +{ + "serverInfo": { + "name": "oauth-insecure-token-endpoint", + "version": "1.0.0" + }, + "tools": [{ "preset": "echo" }], + "oauth": { + "enabled": true, + "mode": "combined", + "requireAuth": true, + "scopesSupported": ["mcp"], + "supportDCR": true, + "supportRefreshTokens": true, + "issuerUrl": "http://localhost.:8091" + }, + "transport": { + "type": "streamable-http", + "port": 8091 + } +} From 090059af5af61af0ac325c5de4629af2b9fdf216 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 6 Sep 2026 22:57:17 -0400 Subject: [PATCH 02/19] fix: address Copilot review round 1 All eight findings were legitimate; two rested on claims about existing behavior that I verified before acting (issuerBinding really does walk cause/data.cause; findAvailablePort really does walk ports on EADDRINUSE). - isLocalhostSubdomainHost no longer strips a root FQDN dot, so `app.localhost.` is rejected. Accepting it split the two layers: CSP's `*.localhost` host-source cannot match the root-dotted form, so /api/* would answer an embedder whose MCP Apps frame the sandbox CSP then blanks. - allowLocalhostSubdomainOriginsFor now strips that dot, so `HOST=localhost.` is read as the loopback bind it is. The two run opposite ways deliberately and both say why: one canonicalizes an operator-typed bind host against the OS resolver, the other matches a browser Origin that must also be expressible as a CSP host-source. - The classifier walks cause / data.cause chains and returns the matched inner shape, mirroring findIssuerBindingFailure including its seen set. A top-level-only check missed the connect and refresh paths, where era negotiation and the transport wrappers bury the rejection. - The fixture sets transport.strictPort, a new opt-out from the port walk, so it fails loudly instead of announcing one port while its OAuth issuer names another. - Copy and docs no longer call these hosts "not loopback". They are loopback by RFC 6761 and by every resolver on the machine; what they are outside is the SDK's three-literal exemption, and the old wording sent readers to debug their networking instead of their configuration. - Hook-level tests for both new branches in useConnectionLifecycle and the callback branch in useOAuthRecovery, asserting position (no banner, no failed-server flag, no generic toast) rather than just the helper, plus wrapped-cause cases and live strictPort coverage. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- clients/web/server/web-server-config.ts | 12 ++- .../src/hooks/useConnectionLifecycle.test.tsx | 65 ++++++++++++ .../web/src/hooks/useOAuthRecovery.test.tsx | 40 ++++++++ .../src/lib/insecureTokenEndpointNotice.ts | 10 +- .../core/auth/insecureTokenEndpoint.test.ts | 80 +++++++++++---- .../web/src/test/core/node/hostUrl.test.ts | 10 +- .../test/integration/mcp/strict-port.test.ts | 99 +++++++++++++++++++ .../server/web-server-config.test.ts | 5 + core/auth/insecureTokenEndpoint.ts | 57 ++++++++++- core/auth/oauthUx.ts | 21 ++-- core/node/hostUrl.ts | 18 +++- docs/test-servers.md | 8 +- .../oauth-insecure-token-endpoint-http.json | 13 ++- test-servers/src/composable-test-server.ts | 11 +++ test-servers/src/load-config.ts | 2 + test-servers/src/resolve-config.ts | 1 + test-servers/src/test-server-http.ts | 11 ++- 17 files changed, 416 insertions(+), 47 deletions(-) create mode 100644 clients/web/src/test/integration/mcp/strict-port.test.ts diff --git a/clients/web/server/web-server-config.ts b/clients/web/server/web-server-config.ts index 09a9c978ae..7012bae0f3 100644 --- a/clients/web/server/web-server-config.ts +++ b/clients/web/server/web-server-config.ts @@ -327,9 +327,19 @@ function loopbackOrigins(port: number): string[] { * browser at `foo.localhost` resolves to `127.0.0.1` and never reaches this * process, so admitting the origin there would be a no-op that only made the * allow-list harder to reason about. + * + * A root FQDN dot is stripped before the lookup, so `HOST=localhost.` is read as + * the loopback bind it actually is. Note this runs the *opposite* way from + * {@link isLocalhostSubdomainHost}, which deliberately keeps the dot — and the + * two are not in tension, because they answer different questions. This one + * canonicalizes an operator-typed **bind host** and only has to agree with the + * OS resolver, which treats `localhost.` and `localhost` alike (as + * {@link isLoopbackHost} already does). That one matches a browser-sent + * **`Origin`** that must also be expressible as a CSP host-source, which the + * root-dotted form is not. */ export function allowLocalhostSubdomainOriginsFor(hostname: string): boolean { - const h = canonicalUrlHost(hostname); + const h = canonicalUrlHost(hostname).replace(/\.$/, ""); return LOOPBACK_HOSTNAMES.has(h) || isAllInterfacesHost(h); } diff --git a/clients/web/src/hooks/useConnectionLifecycle.test.tsx b/clients/web/src/hooks/useConnectionLifecycle.test.tsx index f09c020d21..5561c8213c 100644 --- a/clients/web/src/hooks/useConnectionLifecycle.test.tsx +++ b/clients/web/src/hooks/useConnectionLifecycle.test.tsx @@ -11,6 +11,7 @@ import type { ClientConfig } from "@inspector/core/client/types.js"; import type { RemoteInspectorClientStorage } from "@inspector/core/mcp/remote/index.js"; import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; import { EmaClientNotConfiguredError } from "@inspector/core/auth/ema/clientConfigError.js"; +import { InsecureTokenEndpointError } from "@modelcontextprotocol/client"; import { renderWithMantine, act, waitFor } from "../test/renderWithMantine"; import { EMPTY_SETTINGS } from "../utils/serverSettingsDefaults"; import { DEEP_LINK_SERVER_ID } from "../utils/deepLink"; @@ -601,6 +602,70 @@ describe("useConnectionLifecycle", () => { expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("a"); }); + it("reports an insecure token endpoint as terminal, without flagging the card", async () => { + // SEP-2207 (#2280). Asserted on the hook, not just the notice helper, + // because what makes this arm correct is its *position*: above + // `setFailedServerId` and above the generic toast. A helper-only test + // cannot see either of those go wrong. + connectSpy.mockRejectedValueOnce( + new InsecureTokenEndpointError( + "http://tenant.app.localhost:3300/token", + ), + ); + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(toastTitles()).toContain("Token endpoint is not secure"); + // The generic arm must not also fire — two notifications for one failure + // is how the raw SDK text would creep back in beside the good copy. + expect(toastTitles()).not.toContain('Failed to connect to "Server a"'); + expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("a"); + }); + + it("finds an insecure token endpoint wrapped under `cause` on the connect path", async () => { + // Era negotiation and the transport wrappers bury the rejection, so the + // shallow check this replaced would have missed exactly this shape. + connectSpy.mockRejectedValueOnce( + new Error("connect failed", { + cause: new InsecureTokenEndpointError("http://localhost.:8091/token"), + }), + ); + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(toastTitles()).toContain("Token endpoint is not secure"); + expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("a"); + }); + + it("reports an insecure token endpoint raised by the 401 authorization attempt", async () => { + // The second of the two arms: `authenticate()` rejects rather than the + // opening handshake, which is the path a refresh takes. + connectSpy.mockRejectedValueOnce(unauthorized()); + authenticateSpy.mockRejectedValueOnce( + new InsecureTokenEndpointError("http://localhost.:8091/token"), + ); + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(toastTitles()).toContain("Token endpoint is not secure"); + expect(toastTitles()).not.toContain( + 'OAuth authorization failed for "Server a"', + ); + expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("a"); + // The generic arm also records this as the connect error banner text; + // the terminal arm returns before that, so it must stay unset. + expect(h.api().connectErrorMessage).toBeUndefined(); + }); + it("retries the connect when the auth challenge is already satisfied", async () => { connectSpy.mockRejectedValueOnce( new AuthRecoveryRequiredError(new URL("https://as.example/authorize"), { diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index dabc41f72b..bce1be1236 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/clients/web/src/hooks/useOAuthRecovery.test.tsx @@ -9,6 +9,7 @@ import type { import type { AuthChallenge } from "@inspector/core/auth/challenge.js"; import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; import { EmaClientNotConfiguredError } from "@inspector/core/auth/ema/clientConfigError.js"; +import { InsecureTokenEndpointError } from "@modelcontextprotocol/client"; import { useEffect, useLayoutEffect, useRef } from "react"; import { renderWithMantine, act, waitFor } from "../test/renderWithMantine"; import { @@ -1686,6 +1687,45 @@ describe("useOAuthRecovery", () => { expect(h.spies.setFailedServerId).not.toHaveBeenCalled(); }); + it("reports an insecure token endpoint terminally, with no banner and no red card", async () => { + // SEP-2207 (#2280). The three assertions are the whole point of the arm's + // position: the banner would carry a Re-authenticate button that cannot + // work, and flagging the card would present a configuration error as a + // failed connect attempt. + snapshot(); + const client = fakeClient({ + resumeAfterOAuth: vi + .fn() + .mockRejectedValue( + new InsecureTokenEndpointError("http://localhost.:8091/token"), + ), + }); + const h = callbackHarness(`?code=abc&state=${AUTH_ID}`, {}, client); + await waitFor(() => + expect(toastTitles()).toContain("Token endpoint is not secure"), + ); + expect(h.api().reAuthBanner).toBeNull(); + expect(h.spies.setFailedServerId).not.toHaveBeenCalled(); + }); + + it("finds an insecure token endpoint wrapped under `cause` on the callback leg", async () => { + snapshot(); + const client = fakeClient({ + resumeAfterOAuth: vi.fn().mockRejectedValue( + new Error("resume failed", { + cause: new InsecureTokenEndpointError( + "http://localhost.:8091/token", + ), + }), + ), + }); + const h = callbackHarness(`?code=abc&state=${AUTH_ID}`, {}, client); + await waitFor(() => + expect(toastTitles()).toContain("Token endpoint is not secure"), + ); + expect(h.api().reAuthBanner).toBeNull(); + }); + it("offers one-click recovery when the authorization state was lost", async () => { snapshot(); const client = fakeClient({ diff --git a/clients/web/src/lib/insecureTokenEndpointNotice.ts b/clients/web/src/lib/insecureTokenEndpointNotice.ts index 44f691ab08..9f5df42d4d 100644 --- a/clients/web/src/lib/insecureTokenEndpointNotice.ts +++ b/clients/web/src/lib/insecureTokenEndpointNotice.ts @@ -20,7 +20,7 @@ */ import { notifications } from "@mantine/notifications"; -import { isInsecureTokenEndpointError } from "@inspector/core/auth/insecureTokenEndpoint.js"; +import { findInsecureTokenEndpoint } from "@inspector/core/auth/insecureTokenEndpoint.js"; import { insecureTokenEndpointMessage, insecureTokenEndpointTitle, @@ -36,13 +36,17 @@ export function showInsecureTokenEndpointNotice( err: unknown, serverName?: string, ): boolean { - if (!isInsecureTokenEndpointError(err)) { + // Searched rather than type-tested: era negotiation and the transport + // wrappers bury the rejection under `cause` / `data.cause`, so the connect and + // refresh paths hand us a wrapper rather than the error itself. + const found = findInsecureTokenEndpoint(err); + if (!found) { return false; } notifications.show({ title: insecureTokenEndpointTitle(), message: insecureTokenEndpointMessage({ - tokenEndpoint: err.tokenEndpoint, + tokenEndpoint: found.tokenEndpoint, serverName, }), color: "red", diff --git a/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts b/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts index da513f7175..78ff9ccf33 100644 --- a/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts +++ b/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect } from "vitest"; import { InsecureTokenEndpointError } from "@modelcontextprotocol/client"; -import { isInsecureTokenEndpointError } from "@inspector/core/auth/insecureTokenEndpoint.js"; +import { findInsecureTokenEndpoint } from "@inspector/core/auth/insecureTokenEndpoint.js"; const ENDPOINT = "http://tenant.example.localhost:3300/api/oauth/token"; @@ -26,53 +26,99 @@ describe("SDK brand placement", () => { }); }); -describe("isInsecureTokenEndpointError", () => { - it("recognizes a real SDK error and narrows to its endpoint", () => { - const err: unknown = new InsecureTokenEndpointError(ENDPOINT); - expect(isInsecureTokenEndpointError(err)).toBe(true); - if (isInsecureTokenEndpointError(err)) { - expect(err.tokenEndpoint).toBe(ENDPOINT); - } +describe("findInsecureTokenEndpoint", () => { + it("recognizes a real SDK error and returns its endpoint", () => { + expect( + findInsecureTokenEndpoint(new InsecureTokenEndpointError(ENDPOINT)), + ).toMatchObject({ tokenEndpoint: ENDPOINT }); }); it("recognizes a serialized copy by `name`, where the prototype is gone", () => { // The fallback arm: a structured clone or JSON hop drops the prototype and // the brand set but keeps `name`. expect( - isInsecureTokenEndpointError({ + findInsecureTokenEndpoint({ name: "InsecureTokenEndpointError", message: "Refusing to send credentials…", tokenEndpoint: ENDPOINT, }), - ).toBe(true); + ).toMatchObject({ tokenEndpoint: ENDPOINT }); }); it("rejects a look-alike carrying the endpoint but not the identity", () => { // Neither the brand nor the name: some other error that happens to have a // `tokenEndpoint` field must not be swallowed by the terminal arm. expect( - isInsecureTokenEndpointError({ tokenEndpoint: ENDPOINT, name: "Error" }), - ).toBe(false); + findInsecureTokenEndpoint({ tokenEndpoint: ENDPOINT, name: "Error" }), + ).toBeUndefined(); }); it("rejects the right identity with no endpoint to report", () => { // The copy names the endpoint, so a value that cannot supply one is not // usable by this path and falls through to the generic handling. expect( - isInsecureTokenEndpointError({ name: "InsecureTokenEndpointError" }), - ).toBe(false); + findInsecureTokenEndpoint({ name: "InsecureTokenEndpointError" }), + ).toBeUndefined(); expect( - isInsecureTokenEndpointError({ + findInsecureTokenEndpoint({ name: "InsecureTokenEndpointError", tokenEndpoint: 42, }), - ).toBe(false); + ).toBeUndefined(); }); it.each([null, undefined, "InsecureTokenEndpointError", 0, new Error("x")])( "rejects %j", (value) => { - expect(isInsecureTokenEndpointError(value)).toBe(false); + expect(findInsecureTokenEndpoint(value)).toBeUndefined(); }, ); + + describe("cause chains", () => { + // Era negotiation and the transport wrappers bury the rejection, so a + // top-level-only check would miss the connect and refresh paths outright + // and let the retryable UI render anyway. + it("finds it under `cause`", () => { + const wrapped = new Error("connect failed", { + cause: new InsecureTokenEndpointError(ENDPOINT), + }); + expect(findInsecureTokenEndpoint(wrapped)).toMatchObject({ + tokenEndpoint: ENDPOINT, + }); + }); + + it("finds it under `data.cause`", () => { + const wrapped = Object.assign(new Error("negotiation failed"), { + data: { cause: new InsecureTokenEndpointError(ENDPOINT) }, + }); + expect(findInsecureTokenEndpoint(wrapped)).toMatchObject({ + tokenEndpoint: ENDPOINT, + }); + }); + + it("finds it several links down", () => { + const wrapped = new Error("outer", { + cause: new Error("middle", { + cause: new InsecureTokenEndpointError(ENDPOINT), + }), + }); + expect(findInsecureTokenEndpoint(wrapped)).toMatchObject({ + tokenEndpoint: ENDPOINT, + }); + }); + + it("terminates on a self-referential cause instead of looping", () => { + const loop: { cause?: unknown; name: string } = { name: "Loop" }; + loop.cause = loop; + expect(findInsecureTokenEndpoint(loop)).toBeUndefined(); + }); + + it("returns undefined for a chain that never contains one", () => { + expect( + findInsecureTokenEndpoint( + new Error("outer", { cause: new Error("inner") }), + ), + ).toBeUndefined(); + }); + }); }); diff --git a/clients/web/src/test/core/node/hostUrl.test.ts b/clients/web/src/test/core/node/hostUrl.test.ts index 86319a46a7..19efb58f5b 100644 --- a/clients/web/src/test/core/node/hostUrl.test.ts +++ b/clients/web/src/test/core/node/hostUrl.test.ts @@ -169,10 +169,8 @@ describe("isLocalhostSubdomainHost", () => { it.each([ "app.localhost", "tenant.example.localhost", - // Canonicalized first, so casing and a root FQDN dot are normalized rather - // than rejected — the browser sends the canonical form in `Origin`. + // Canonicalized first, so casing is normalized rather than rejected. "APP.LOCALHOST", - "app.localhost.", // IDNA-mapped to punycode by `new URL`, exactly as a browser would. "münchen.localhost", ])("accepts %j", (host) => { @@ -183,6 +181,12 @@ describe("isLocalhostSubdomainHost", () => { // The bare TLD is deliberately NOT matched: callers that want it carry it // as a literal origin already. "localhost", + // The root-dotted form is rejected ON PURPOSE, and this is the case that + // keeps it that way: CSP's `*.localhost` host-source cannot match it, so + // accepting it here would let /api/* answer an embedder whose MCP Apps + // frame the sandbox CSP then blanks. + "app.localhost.", + "localhost.", // Degenerate labels. ".localhost", "a..localhost", diff --git a/clients/web/src/test/integration/mcp/strict-port.test.ts b/clients/web/src/test/integration/mcp/strict-port.test.ts new file mode 100644 index 0000000000..9a315500dd --- /dev/null +++ b/clients/web/src/test/integration/mcp/strict-port.test.ts @@ -0,0 +1,99 @@ +import { describe, it, expect, afterEach } from "vitest"; +import { createServer, type Server } from "node:http"; +import { + createTestServerHttp, + type TestServerHttp, + createTestServerInfo, + loadConfig, + resolveConfig, +} from "@modelcontextprotocol/inspector-test-server"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +/** + * Live coverage of `ServerConfig.strictPort` (#2280). + * + * Every other fixture walks to the next free port on `EADDRINUSE`, which is + * right for them and wrong for one: `oauth-insecure-token-endpoint-http.json` + * hard-codes its port inside an OAuth issuer string, so a relocated server would + * announce 8092 while all its metadata still pointed at whatever unrelated + * process holds 8091 — and would silently stop reproducing the refusal it + * exists for. This asserts the walk still happens by default and does not + * happen for that fixture, because "fails loudly" is only a safety property if + * it actually fails. + */ +const configsDir = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "../../../../../../test-servers/configs", +); + +describe("strictPort (#2280)", () => { + let squatter: Server | null = null; + let server: TestServerHttp | null = null; + + afterEach(async () => { + if (server) { + try { + await server.stop(); + } catch { + // ignore + } + server = null; + } + if (squatter) { + await new Promise((resolve) => squatter!.close(() => resolve())); + squatter = null; + } + }); + + /** Hold a port so the next bind has to decide whether to walk. */ + const squat = async (): Promise => { + squatter = createServer((_req, res) => res.end()); + await new Promise((resolve) => + squatter!.listen(0, "127.0.0.1", () => resolve()), + ); + const address = squatter.address(); + if (typeof address !== "object" || address === null) { + throw new Error("no port"); + } + return address.port; + }; + + it("walks to another port by default", async () => { + const taken = await squat(); + server = createTestServerHttp({ + serverInfo: createTestServerInfo("walks", "1.0.0"), + serverType: "streamable-http", + port: taken, + }); + const bound = await server.start(); + expect(bound).not.toBe(taken); + }); + + it("refuses to relocate when strictPort is set", async () => { + const taken = await squat(); + server = createTestServerHttp({ + serverInfo: createTestServerInfo("strict", "1.0.0"), + serverType: "streamable-http", + port: taken, + strictPort: true, + }); + await expect(server.start()).rejects.toMatchObject({ + code: "EADDRINUSE", + }); + server = null; + }); + + it("is carried from the fixture's config file to the resolved server config", async () => { + // The plumbing half: a flag the loader drops would leave the fixture + // relocating again with nothing to show for it. + const resolved = resolveConfig( + loadConfig( + path.join(configsDir, "oauth-insecure-token-endpoint-http.json"), + ), + ); + expect(resolved.strictPort).toBe(true); + expect(resolved.port).toBe(8091); + expect(resolved.oauth?.issuerUrl?.href).toContain("localhost.:8091"); + }); +}); 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 91f522fba0..a06f620640 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 @@ -433,6 +433,11 @@ describe("allowLocalhostSubdomainOriginsFor (#1944)", () => { "[::1]", // Non-canonical spellings of the same addresses. "127.1", + // The root-anchored spelling binds loopback, and the OS resolver treats it + // as `localhost` — so the widening must not switch off for it. (This runs + // the opposite way from `isLocalhostSubdomainHost`, which keeps the dot + // because CSP cannot express it; see that function's note.) + "localhost.", "2130706433", "0:0:0:0:0:0:0:1", // All-interfaces binds serve loopback too (the Docker opt-in path). diff --git a/core/auth/insecureTokenEndpoint.ts b/core/auth/insecureTokenEndpoint.ts index 49f2e8a39a..9dfa6717e5 100644 --- a/core/auth/insecureTokenEndpoint.ts +++ b/core/auth/insecureTokenEndpoint.ts @@ -13,8 +13,8 @@ * The check is not a fix for `*.localhost` (#1944): the exemption list lives in * the SDK and takes no options, so widening it has to happen upstream * (typescript-sdk#2591). This is about how the refusal is *reported* — which - * matters for every insecure endpoint, `host.docker.internal` and LAN hostnames - * included, not only the `.localhost` case. + * matters for every endpoint outside that exemption, `host.docker.internal` and + * LAN hostnames included, not only the `.localhost` case. */ import { InsecureTokenEndpointError } from "@modelcontextprotocol/client"; @@ -26,7 +26,7 @@ export interface InsecureTokenEndpointShape { } /** - * Recognize the SDK's `InsecureTokenEndpointError`. + * Recognize the SDK's `InsecureTokenEndpointError` itself (not a wrapper). * * Uses the SDK's own `isInstance` predicate, which is cross-copy safe by * construction: the SDK stamps each instance with a brand set keyed by @@ -41,7 +41,7 @@ export interface InsecureTokenEndpointShape { * prototype and brand set are the first things a structured clone or a JSON hop * would drop, and `name` survives both. */ -export function isInsecureTokenEndpointError( +function isInsecureTokenEndpointShape( err: unknown, ): err is InsecureTokenEndpointShape { if (err === null || typeof err !== "object") { @@ -56,3 +56,52 @@ export function isInsecureTokenEndpointError( candidate.name === "InsecureTokenEndpointError" ); } + +/** + * Find an insecure-token-endpoint refusal anywhere in an error's `cause` / + * `data.cause` chain, and return the shape that carries the endpoint. + * + * Walking the chain is not defensive padding: era negotiation and the transport + * wrappers bury the original rejection, so a top-level-only check would miss the + * connect and refresh paths and let exactly the retryable UI this exists to + * remove render anyway. `findIssuerBindingFailure` in `issuerBinding.ts` walks + * the same two links for the same reason, and this deliberately mirrors it — + * including the `seen` set, which keeps a self-referential `cause` from looping. + */ +export function findInsecureTokenEndpoint( + err: unknown, +): InsecureTokenEndpointShape | undefined { + return findInsecureTokenEndpointDeep(err, new Set()); +} + +function findInsecureTokenEndpointDeep( + err: unknown, + seen: Set, +): InsecureTokenEndpointShape | undefined { + if (err === null || typeof err !== "object" || seen.has(err)) { + return undefined; + } + seen.add(err); + + if (isInsecureTokenEndpointShape(err)) { + return err; + } + + const nested = findInsecureTokenEndpointDeep( + (err as { cause?: unknown }).cause, + seen, + ); + if (nested) { + return nested; + } + + const data = (err as { data?: unknown }).data; + if (data !== null && typeof data === "object") { + return findInsecureTokenEndpointDeep( + (data as { cause?: unknown }).cause, + seen, + ); + } + + return undefined; +} diff --git a/core/auth/oauthUx.ts b/core/auth/oauthUx.ts index f12ca1c12e..eba849777e 100644 --- a/core/auth/oauthUx.ts +++ b/core/auth/oauthUx.ts @@ -380,9 +380,14 @@ export function insecureTokenEndpointTitle(): string { /** * Plain-language explanation and the two things that actually resolve it. * - * Does not echo the SDK's own message, which names only `localhost`, - * `127.0.0.1` and `::1` as exempt and reads as a flat refusal — accurate, but it - * tells the user nothing about which lever to reach for. The endpoint is + * Does not echo the SDK's own message, which reads as a flat refusal and tells + * the user nothing about which lever to reach for. + * + * The wording is "outside the SDK's loopback exemption", never "not loopback". + * The motivating hosts — `tenant.app.localhost` (#1944), the `localhost.` + * fixture — *are* loopback by RFC 6761 and by every resolver on the machine; + * what they are outside is a three-literal allow-list. Calling them non-loopback + * would send a reader to debug their networking instead of their configuration. The endpoint is * remote-supplied (it comes from the server's authorization-server metadata), * so it is bounded for display by {@link truncateUrlForDisplay}; rendering is * escaped, so that is a layout bound rather than an injection defence. @@ -395,10 +400,10 @@ export function insecureTokenEndpointMessage(options: { return ( `Authorization for ${target} was stopped before any credentials were sent: ` + `its token endpoint ${truncateUrlForDisplay(options.tokenEndpoint)} is ` + - "plain HTTP on a host that is not loopback, and OAuth token requests must " + - "use TLS. Re-authenticating cannot change this. " + - "Serve the token endpoint over HTTPS, or point it at a genuinely loopback " + - "host (localhost, 127.0.0.1 or ::1) — Server Settings → Authorization has a " + - "Token URL override if the authorization server advertises a different one." + "plain HTTP on a host outside the MCP SDK's loopback exemption, which " + + "covers only localhost, 127.0.0.1 and ::1. Re-authenticating cannot change " + + "this. Serve the token endpoint over HTTPS, or move it to one of those " + + "three spellings — Server Settings → Authorization has a Token URL override " + + "if the authorization server advertises a different one." ); } diff --git a/core/node/hostUrl.ts b/core/node/hostUrl.ts index 1491f2ccd7..e20900b770 100644 --- a/core/node/hostUrl.ts +++ b/core/node/hostUrl.ts @@ -193,9 +193,18 @@ export function isLoopbackHost(host: string): boolean { * Canonicalized through {@link canonicalUrlHost} first, so the comparison sees * the same lowercased, IDNA-mapped form the browser puts in `Origin` — a * `Münchén.LOCALHOST` embedder arrives as `xn--mnchn-3ya1b.localhost` and still - * matches. A root FQDN dot is dropped for the same reason it is in - * {@link isLoopbackHost}. Every label left of `.localhost` must be non-empty, so - * the degenerate `.localhost` and `a..localhost` are rejected. + * matches. Every label left of `.localhost` must be non-empty, so the degenerate + * `.localhost` and `a..localhost` are rejected. + * + * ⚠️ A **root FQDN dot is NOT stripped here**, so `app.localhost.` is rejected — + * deliberately, and unlike {@link isLoopbackHost}, which does strip it. This + * predicate has a second consumer that cannot follow it: the MCP Apps + * `frame-ancestors` CSP, whose `*.localhost` host-source matches by suffix and + * has no way to express the root-dotted form (CSP's `host-char` grammar admits + * no empty final label). Accepting `app.localhost.` here would let `/api/*` + * answer an embedder whose Apps frame the CSP then blanks — the split behaviour + * this pair exists to prevent, and a far worse outcome than declining a + * spelling almost nobody browses. The two layers agree by construction instead. * * Deliberately separate from {@link isLoopbackHost} rather than folded into it. * That predicate gates the **OAuth callback listener's bind host**, and this is @@ -206,7 +215,8 @@ export function isLoopbackHost(host: string): boolean { * error, which is not an improvement. */ export function isLocalhostSubdomainHost(host: string): boolean { - const h = canonicalUrlHost(host).replace(/\.$/, ""); + // No trailing-dot strip — see the note above; the CSP layer cannot match it. + const h = canonicalUrlHost(host); const suffix = ".localhost"; if (!h.endsWith(suffix)) return false; const labels = h.slice(0, -suffix.length).split("."); diff --git a/docs/test-servers.md b/docs/test-servers.md index 51d95c7551..4b477a3682 100644 --- a/docs/test-servers.md +++ b/docs/test-servers.md @@ -438,13 +438,17 @@ The value now rides the normalized `AuthChallenge` as a string — it has to be ## A token endpoint the SDK will not use (SEP-2207) -`oauth-insecure-token-endpoint-http.json` is an ordinary combined AS + resource server with one thing changed: `oauth.issuerUrl` is `http://localhost.:8091`, so its advertised `token_endpoint` is `http://localhost./oauth/token`. Plain streamable-HTTP — connect with the **default (legacy)** protocol era. +`oauth-insecure-token-endpoint-http.json` is an ordinary combined AS + resource server with one thing changed: `oauth.issuerUrl` is `http://localhost.:8091`, so its advertised `token_endpoint` is `http://localhost.:8091/oauth/token`. Plain streamable-HTTP — connect with the **default (legacy)** protocol era. + +⚠️ It sets `transport.strictPort`, so it **fails to start** if 8091 is taken rather than relocating. Every other fixture walks to the next free port on `EADDRINUSE`, which is right for them and wrong for this one: the issuer is a *fixed string* in the config, so a relocated server would announce 8092 while all its OAuth metadata still pointed at whatever unrelated process holds 8091 — and the fixture would quietly stop reproducing the refusal it exists for. A loud failure is the only honest option here. The trailing dot is the whole trick, and it is doing real work rather than being a curiosity. `localhost.` is the *root-anchored* spelling of `localhost`: every resolver on the machine sends it to the loopback interface, so the fixture is reachable and the flow runs for real — but the SDK's `assertSecureTokenEndpoint` exempts only the three literals `localhost`, `127.0.0.1` and `::1`, and `localhost.` is none of them. So the credential-carrying request is refused with `InsecureTokenEndpointError` while everything else about the server works. It is the same over-narrow exemption that makes `http://tenant.app.localhost:3300` fail ([#1944](https://github.com/modelcontextprotocol/inspector/issues/1944), [typescript-sdk#2591](https://github.com/modelcontextprotocol/typescript-sdk/issues/2591)), reproducible without a `/etc/hosts` entry or dnsmasq. Add the server, click **Connect**, and complete the authorization. The redirect comes back with a code, the Inspector goes to exchange it, and the SDK refuses. -What you should see is a red, non-dismissing **"Token endpoint is not secure"** notification naming the endpoint and the two things that resolve it — serve it over HTTPS, or point it at a genuinely loopback host. There is deliberately **no** action button. +What you should see is a red, non-dismissing **"Token endpoint is not secure"** notification naming the endpoint and the two things that resolve it — serve it over HTTPS, or move it to one of the three host spellings the SDK exempts (`localhost`, `127.0.0.1`, `::1`). There is deliberately **no** action button. + +Note the second option is phrased as a *spelling* change, not a networking one. `localhost.` already **is** loopback, and so is `tenant.app.localhost`; what they are outside is a three-literal allow-list. Telling a reader to "use a loopback host" when they demonstrably already are is what sends them off to debug their resolver instead of their configuration. On the broken build you got a **"Re-authentication required"** banner with a **Re-authenticate** button ([#2280](https://github.com/modelcontextprotocol/inspector/issues/2280)). That button could never work: `InsecureTokenEndpointError` does not extend `OAuthError`, and `auth()` special-cases it to rethrow rather than start a fresh `/authorize` redirect, so clicking it re-ran the same flow to the same refusal. The only text on screen was the raw SDK message, which names the three exempt literals and says nothing about which lever to reach for. diff --git a/test-servers/configs/oauth-insecure-token-endpoint-http.json b/test-servers/configs/oauth-insecure-token-endpoint-http.json index 1721f12a9c..c2b26d8927 100644 --- a/test-servers/configs/oauth-insecure-token-endpoint-http.json +++ b/test-servers/configs/oauth-insecure-token-endpoint-http.json @@ -3,18 +3,25 @@ "name": "oauth-insecure-token-endpoint", "version": "1.0.0" }, - "tools": [{ "preset": "echo" }], + "tools": [ + { + "preset": "echo" + } + ], "oauth": { "enabled": true, "mode": "combined", "requireAuth": true, - "scopesSupported": ["mcp"], + "scopesSupported": [ + "mcp" + ], "supportDCR": true, "supportRefreshTokens": true, "issuerUrl": "http://localhost.:8091" }, "transport": { "type": "streamable-http", - "port": 8091 + "port": 8091, + "strictPort": true } } diff --git a/test-servers/src/composable-test-server.ts b/test-servers/src/composable-test-server.ts index 7123873fbf..229df487cd 100644 --- a/test-servers/src/composable-test-server.ts +++ b/test-servers/src/composable-test-server.ts @@ -456,6 +456,17 @@ export interface ServerConfig { | undefined; // Optional callback to customize resource handler during registration serverType?: "sse" | "streamable-http"; // Transport type (default: "streamable-http") port?: number; // Port to use (optional, will find available port if not specified) + /** + * Refuse to relocate: bind {@link port} exactly, or fail with EADDRINUSE. + * + * Off by default, because walking to the next free port is what lets several + * fixtures run side by side. It exists for a fixture whose *advertised* + * configuration hard-codes the port — `oauth-insecure-token-endpoint-http.json` + * puts it in an OAuth issuer — where relocating leaves the server announcing + * one port while its metadata still points at another process entirely, and + * the fixture silently stops reproducing what it exists to reproduce. + */ + strictPort?: boolean; /** * Whether to advertise listChanged capability for each list type * If enabled, modification tools will send list_changed notifications diff --git a/test-servers/src/load-config.ts b/test-servers/src/load-config.ts index 4f08e232ad..30fe40df05 100644 --- a/test-servers/src/load-config.ts +++ b/test-servers/src/load-config.ts @@ -119,6 +119,8 @@ export interface ConfigFile { transport: { type: "stdio" | "streamable-http" | "sse"; port?: number; + /** Bind `port` exactly, or fail — see `ServerConfig.strictPort`. */ + strictPort?: boolean; /** * Serve the modern (2026-07-28) protocol era via the SDK's * `createMcpHandler` (only valid with `type: "streamable-http"`). `true` diff --git a/test-servers/src/resolve-config.ts b/test-servers/src/resolve-config.ts index 52eaca8ea1..8cd876de02 100644 --- a/test-servers/src/resolve-config.ts +++ b/test-servers/src/resolve-config.ts @@ -102,6 +102,7 @@ export function resolveConfig(config: ConfigFile): ServerConfig { ? (transport.type as "sse" | "streamable-http") : undefined, port: isHttp ? transport.port : undefined, + strictPort: isHttp ? transport.strictPort : undefined, }; // Normalize the modern flag: `true` is shorthand for the default (dual-era diff --git a/test-servers/src/test-server-http.ts b/test-servers/src/test-server-http.ts index f8a965c3b2..66bb004633 100644 --- a/test-servers/src/test-server-http.ts +++ b/test-servers/src/test-server-http.ts @@ -401,8 +401,15 @@ export class TestServerHttp { const requestedPort = this.config.port; // If a port is explicitly requested, find an available port starting from that value - // Otherwise, use 0 to let the OS assign an available port - const port = requestedPort ? await findAvailablePort(requestedPort) : 0; + // Otherwise, use 0 to let the OS assign an available port. + // `strictPort` opts out of the walk: bind the requested port or fail loudly + // (see the field's doc comment — a relocated server whose advertised config + // hard-codes the port is worse than one that does not start). + const port = requestedPort + ? this.config.strictPort + ? requestedPort + : await findAvailablePort(requestedPort) + : 0; if (serverType === "streamable-http") { return this.startHttp(port); From 90d85de358d7438ab3c8a81054d4b12d13e76881 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 6 Sep 2026 23:16:46 -0400 Subject: [PATCH 03/19] fix: address Copilot review round 2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both findings were real, and both premises checked against the code first. - HOST=localhost. advertised http://localhost.:PORT. That passed the exact-match API guard and then blanked the MCP Apps frame, because a root-dotted host is not a valid CSP host-source and *.localhost does not cover bare localhost. — the same split round 1 fixed one layer up, still live one line away. The banner and defaultAllowedOrigins now share a canonicalOriginHost helper that drops the root dot, so banner ⊆ allowedOrigins holds through the normalization rather than in spite of it. The bind host is untouched: HOST still binds exactly what was typed, and only the browser-facing spelling changes. Deliberately not folded into canonicalUrlHost, which isLocalhostSubdomainHost needs to keep the dot for the round-1 reason. - runWithCommandAuthRecovery was not the funnel I claimed. It rethrows every non-AuthRecoveryRequiredError, so a mid-session silent refresh against an unusable token endpoint reached runCommandInBackground and either showed the raw SDK text under a generic title or, where the panel owns reporting, was swallowed and left the command looking like it did nothing. It is now claimed there too, taking the same undefined exit the unsatisfied-recovery branch already uses. Tests: the advertised-origin trio and the frame-ancestors directive for HOST=localhost., the banner-subset invariant, and three command-path cases covering the awaited form and both background forms. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- clients/web/server/web-server-config.ts | 37 +++++++++-- .../web/src/hooks/useOAuthRecovery.test.tsx | 66 +++++++++++++++++++ clients/web/src/hooks/useOAuthRecovery.ts | 24 ++++++- .../server/web-server-config.test.ts | 43 +++++++++++- 4 files changed, 164 insertions(+), 6 deletions(-) diff --git a/clients/web/server/web-server-config.ts b/clients/web/server/web-server-config.ts index 7012bae0f3..4fad9bb6fe 100644 --- a/clients/web/server/web-server-config.ts +++ b/clients/web/server/web-server-config.ts @@ -212,7 +212,7 @@ export function printServerBanner( // :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 h = canonicalOriginHost(config.hostname); const bannerHost = isAllInterfacesHost(h) ? "localhost" : h; const baseUrl = httpOrigin(bannerHost, actualPort); const url = @@ -301,6 +301,32 @@ function loopbackOrigins(port: number): string[] { ]; } +/** + * The canonical host to put in a **browser-facing** origin or advertised URL. + * + * {@link canonicalUrlHost} plus one thing it deliberately does not do: drop a + * root FQDN dot. `HOST=localhost.` binds loopback and every resolver treats it + * as `localhost`, but the WHATWG serializer keeps the dot — and a root-dotted + * host is **not a valid CSP `host-source`** (the grammar admits no empty final + * label). Advertising `http://localhost.:PORT` therefore produced a URL that + * passed the exact-match API guard and then blanked the MCP Apps frame, because + * the browser dropped that source from `frame-ancestors` and nothing else + * covered the embedder. + * + * Used by **both** the banner and {@link defaultAllowedOrigins}, which is what + * keeps `banner ⊆ allowedOrigins` true through this normalization rather than + * in spite of it. The **bind host is untouched** — `config.hostname` still + * binds exactly what was typed; this only changes the spelling we hand a + * browser. + * + * Deliberately NOT folded into `canonicalUrlHost`: `isLocalhostSubdomainHost` + * depends on that function preserving the dot, precisely so it can reject + * `app.localhost.` for the same CSP reason. The two live at different layers. + */ +function canonicalOriginHost(hostname: string): string { + return canonicalUrlHost(hostname).replace(/\.$/, ""); +} + /** * Whether the default origin allow-list for `hostname` should additionally * admit `*.localhost` origins (#1944). @@ -339,7 +365,7 @@ function loopbackOrigins(port: number): string[] { * root-dotted form is not. */ export function allowLocalhostSubdomainOriginsFor(hostname: string): boolean { - const h = canonicalUrlHost(hostname).replace(/\.$/, ""); + const h = canonicalOriginHost(hostname); return LOOPBACK_HOSTNAMES.has(h) || isAllInterfacesHost(h); } @@ -380,8 +406,11 @@ export function defaultAllowedOrigins( ): 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); + // every branch reasons about the same address. `canonicalOriginHost` rather + // than `canonicalUrlHost`: this list is compared against a browser `Origin` + // and is also the source of the MCP Apps `frame-ancestors`, so it must not + // emit a root-dotted host that CSP cannot express. + const h = canonicalOriginHost(hostname); if (LOOPBACK_HOSTNAMES.has(h)) { return loopbackOrigins(port); } diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index bce1be1236..d82b45bf4b 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/clients/web/src/hooks/useOAuthRecovery.test.tsx @@ -980,6 +980,72 @@ describe("useOAuthRecovery", () => { }); }); + it("claims an insecure token endpoint on the command path instead of rethrowing", async () => { + // SEP-2207 (#2280). A mid-session silent refresh rejects here rather than + // as an AuthRecoveryRequiredError, so before this it was rethrown into + // the generic reporting below. + const client = fakeClient(); + const h = harness({ servers: [entry("a")], activeServerId: "a", client }); + await act(async () => { + await expect( + h + .api() + .runWithCommandAuthRecovery( + () => + Promise.reject( + new InsecureTokenEndpointError( + "http://localhost.:8091/token", + ), + ), + "tool", + ), + ).resolves.toBeUndefined(); + }); + expect(toastTitles()).toContain("Token endpoint is not secure"); + }); + + it("shows the terminal notice instead of the generic title in the background form", async () => { + // The `errorTitle` call sites would otherwise render the raw SDK text + // under a generic heading. + const client = fakeClient(); + const h = harness({ servers: [entry("a")], activeServerId: "a", client }); + await act(async () => { + h.api().runCommandInBackground( + () => + Promise.reject( + new InsecureTokenEndpointError("http://localhost.:8091/token"), + ), + "ambient", + "Refresh failed", + ); + await Promise.resolve(); + }); + await waitFor(() => + expect(toastTitles()).toContain("Token endpoint is not secure"), + ); + expect(toastTitles()).not.toContain("Refresh failed"); + }); + + it("still reports it at a call site whose panel owns reporting", async () => { + // The worse half of the old behavior: with no `errorTitle` the rejection + // was swallowed outright and the command just appeared to do nothing. + const client = fakeClient(); + const h = harness({ servers: [entry("a")], activeServerId: "a", client }); + await act(async () => { + h.api().runCommandInBackground( + () => + Promise.reject( + new InsecureTokenEndpointError("http://localhost.:8091/token"), + ), + "ambient", + ); + await Promise.resolve(); + }); + await waitFor(() => + expect(toastTitles()).toContain("Token endpoint is not secure"), + ); + }); + it("toasts a background failure only when given a title", async () => { const client = fakeClient(); const h = harness({ servers: [entry("a")], activeServerId: "a", client }); diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index d0f121839f..c19dff7722 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -820,10 +820,32 @@ export function useOAuthRecovery({ } return undefined; } + // SEP-2207 (#2280), on the command path. A mid-session silent refresh + // against an unusable token endpoint rejects here rather than as an + // `AuthRecoveryRequiredError`, so without this it is rethrown and lands + // in `runCommandInBackground` — which either shows the raw SDK text + // under a generic title or, at a call site whose panel owns reporting, + // swallows it and leaves the command looking like it did nothing. + // + // Claimed rather than rethrown, taking the same `undefined` exit the + // unsatisfied-recovery branch above already uses: the failure is + // terminal and now fully reported, so an awaited caller should stop + // rather than render it a second time. + const server = sessionRef.current.servers.find( + (s) => s.id === activeServerId, + ); + if (showInsecureTokenEndpointNotice(err, server?.name)) { + return undefined; + } throw err; } }, - [inspectorClient, activeServerId, handleCommandScopedAuthRecovery], + [ + inspectorClient, + activeServerId, + handleCommandScopedAuthRecovery, + sessionRef, + ], ); /** 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 a06f620640..25af22c6fe 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 @@ -8,7 +8,10 @@ import { webServerConfigToInitialPayload, type WebServerConfig, } from "../../../../server/web-server-config.js"; -import { DEFAULT_SANDBOX_PORT } from "../../../../server/sandbox-controller.js"; +import { + DEFAULT_SANDBOX_PORT, + sandboxFrameAncestors, +} from "../../../../server/sandbox-controller.js"; import { API_SERVER_ENV_VARS, LEGACY_AUTH_TOKEN_ENV, @@ -460,6 +463,44 @@ describe("allowLocalhostSubdomainOriginsFor (#1944)", () => { ); }); +describe("root-dotted loopback host (#2280 review round 2)", () => { + it("advertises the CSP-expressible spelling for HOST=localhost.", () => { + // `localhost.` binds loopback and the resolver treats it as `localhost`, + // but a root-dotted host is not a valid CSP host-source — so advertising + // `http://localhost.:PORT` produced a URL that passed the exact-match API + // guard and then blanked the MCP Apps frame. + expect(defaultAllowedOrigins("localhost.", 6274)).toEqual([ + "http://localhost:6274", + "http://127.0.0.1:6274", + "http://[::1]:6274", + ]); + }); + + it("keeps banner ⊆ allowedOrigins through the normalization", () => { + // The invariant the shared `canonicalOriginHost` exists to hold: whatever + // the banner prints has to be a member of the list, or the advertised URL + // 403s. Both read the same helper, so this asserts they agree rather than + // re-deriving the banner here. + process.env.HOST = "localhost."; + const cfg = buildWebServerConfigFromEnv(); + expect(cfg.allowedOrigins).toContain("http://localhost:6274"); + expect(cfg.allowedOrigins).not.toContain("http://localhost.:6274"); + // The bind host itself is untouched — only the browser-facing spelling + // normalizes. + expect(cfg.hostname).toBe("localhost."); + expect(cfg.allowLocalhostSubdomainOrigins).toBe(true); + }); + + it("emits no root-dotted source into the sandbox frame-ancestors", () => { + const directive = sandboxFrameAncestors( + defaultAllowedOrigins("localhost.", 6274), + { allowLocalhostSubdomains: true }, + ); + expect(directive).not.toContain("localhost.:"); + expect(directive).toContain("http://localhost:6274"); + }); +}); + describe("defaultAllowedOrigins", () => { it.each(["localhost", "127.0.0.1", "::1", "[::1]", "LOCALHOST"])( "expands the loopback host %s into all three loopback origins", From e39e34248d62eefb4d5d31a3be81b214ca1e595a Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 6 Sep 2026 23:42:45 -0400 Subject: [PATCH 04/19] fix: address Copilot review round 4 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings, all real. The second is the one that mattered. - The terminal arm in showReAuthBanner did not clear a banner already on screen, so a later terminal failure showed the notice beside a stale Re-authenticate button — the affordance this change exists to remove, sourced from an earlier failure rather than this one. - resumePendingReauth re-armed the pending slot on any rejection and toasted "will try again". handleAuthChallenge runs the same SDK auth flow, so it can raise this terminal error — and re-arming meant every subsequent tab focus and reconnect replayed it, forever, under a promise of a retry that cannot succeed. Classified before the restore, so it is reported once and released. The regression test asserts the second becomeVisible() does not call handleAuthChallenge again, which is the actual loop. - The copy said "plain HTTP". The SDK's check is protocol !== "https:", so a mistyped ftp: or ws: endpoint reaches the same notice; it now says "not HTTPS" so the diagnosis matches the error's contract. - strict-port.test.ts nulled the failed server to skip teardown, but start() installs the process-global test-server control before it binds and only stop() clears it, so the failed instance left that global pointing at itself for the rest of the worker. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- .../web/src/hooks/useOAuthRecovery.test.tsx | 57 +++++++++++++++++++ clients/web/src/hooks/useOAuthRecovery.ts | 18 ++++++ .../web/src/test/core/auth/oauthUx.test.ts | 11 ++++ .../test/integration/mcp/strict-port.test.ts | 5 +- core/auth/oauthUx.ts | 17 ++++-- 5 files changed, 101 insertions(+), 7 deletions(-) diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index d82b45bf4b..532aaaf833 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/clients/web/src/hooks/useOAuthRecovery.test.tsx @@ -1248,6 +1248,28 @@ describe("useOAuthRecovery", () => { await waitFor(() => expect(h.api().reAuthBanner?.serverId).toBe("a")); }); + it("clears a banner already on screen when a later oauthError is terminal", async () => { + // Otherwise the stale Re-authenticate button sits beside the terminal + // notice — the affordance this change removes, sourced from an earlier + // failure rather than this one. + const client = fakeClient(); + const h = harness({ servers: [entry("a")], activeServerId: "a", client }); + await act(async () => { + client.emit("oauthError", { error: new Error("token endpoint 500") }); + }); + await waitFor(() => expect(h.api().reAuthBanner?.serverId).toBe("a")); + + await act(async () => { + client.emit("oauthError", { + error: new InsecureTokenEndpointError("http://localhost.:8091/token"), + }); + }); + await waitFor(() => + expect(toastTitles()).toContain("Token endpoint is not secure"), + ); + expect(h.api().reAuthBanner).toBeNull(); + }); + it("ignores an oauthError with no active server", async () => { const client = fakeClient(); const h = harness({ servers: [], activeServerId: undefined, client }); @@ -1411,6 +1433,41 @@ describe("useOAuthRecovery", () => { ); }); + it("does not re-arm a deferred recovery whose failure is terminal", async () => { + // The restore's premise is that the recovery is still owed and a later + // trigger should retry it. For a refusal that can only fail the same way, + // re-arming means every future tab focus replays it under a toast + // promising a retry that cannot succeed — an unbounded loop on a terminal + // error (#2280). + const client = fakeClient({ + handleAuthChallenge: vi + .fn() + .mockRejectedValue( + new InsecureTokenEndpointError("http://localhost.:8091/token"), + ), + }); + const h = harness({ servers: [entry("a")], activeServerId: "a", client }); + await defer(client, h); + await act(async () => { + becomeVisible(); + }); + + await waitFor(() => + expect(toastTitles()).toContain("Token endpoint is not secure"), + ); + // Neither the retry promise nor the slot that would make good on it. + // `pendingReauth` is not on the hook's public surface, so it is read back + // through the commit probe, as the step-up tests above do. + expect(toastTitles()).not.toContain("Could not continue authorization"); + expect(h.commits[h.commits.length - 1]?.reauthServerId).toBeUndefined(); + + // The load-bearing assertion: coming back to the tab does not replay it. + await act(async () => { + becomeVisible(); + }); + expect(client.handleAuthChallenge).toHaveBeenCalledTimes(1); + }); + it("does not put a stale challenge back over a newer deferral", async () => { // The tab can go hidden mid-resume and a newer challenge defer itself // into the slot; that one describes the session as it is now, so the diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index c19dff7722..76cd96dffb 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -397,6 +397,11 @@ export function useOAuthRecovery({ // through, rather than at each of its call sites — a new caller then gets // the right behavior by default instead of by remembering. if (showInsecureTokenEndpointNotice(detail, server?.name)) { + // Clear one already on screen. Returning without this leaves a stale + // Re-authenticate button beside the terminal notice — the exact + // affordance this arm exists to remove, just sourced from an earlier + // failure rather than this one. + setReAuthBanner(null); return; } const message = reAuthBannerMessage({ @@ -955,6 +960,19 @@ export function useOAuthRecovery({ }); } } catch (err) { + // SEP-2207 (#2280) first, and specifically BEFORE the restore below. + // `handleAuthChallenge` runs the same SDK auth flow, so it can raise + // this terminal error — and the restore's whole premise is that the + // recovery is still owed and a later trigger should retry it. For a + // refusal that can only fail the same way, re-arming the slot means + // every future tab focus and reconnect replays it, under a toast + // promising a retry that cannot succeed. Report it and let it go. + const failedServer = sessionRef.current.servers.find( + (s) => s.id === pending.serverId, + ); + if (showInsecureTokenEndpointNotice(err, failedServer?.name)) { + return; + } // The slot was cleared above only to keep a tab-visible event and a // reconnect from starting the same authorization twice — not because // the recovery was delivered. It still is owed, so restore it and let diff --git a/clients/web/src/test/core/auth/oauthUx.test.ts b/clients/web/src/test/core/auth/oauthUx.test.ts index aa3a188350..1ef53e11ad 100644 --- a/clients/web/src/test/core/auth/oauthUx.test.ts +++ b/clients/web/src/test/core/auth/oauthUx.test.ts @@ -447,6 +447,17 @@ describe("insecureTokenEndpoint copy", () => { expect(insecureTokenEndpointTitle()).toBe("Token endpoint is not secure"); }); + it("describes the scheme as not-HTTPS rather than as plain HTTP", () => { + // The SDK's check is `protocol !== "https:"`, so a mistyped `ftp:` or `ws:` + // endpoint lands here too; naming the wrong scheme would send the reader + // hunting for a problem they do not have. + const message = insecureTokenEndpointMessage({ + tokenEndpoint: "ftp://as.example.com/token", + }); + expect(message).toContain("not HTTPS"); + expect(message).not.toContain("plain HTTP"); + }); + it("names the endpoint, the server, and both ways out", () => { const message = insecureTokenEndpointMessage({ tokenEndpoint: ENDPOINT, diff --git a/clients/web/src/test/integration/mcp/strict-port.test.ts b/clients/web/src/test/integration/mcp/strict-port.test.ts index 9a315500dd..90e41b0fbe 100644 --- a/clients/web/src/test/integration/mcp/strict-port.test.ts +++ b/clients/web/src/test/integration/mcp/strict-port.test.ts @@ -81,7 +81,10 @@ describe("strictPort (#2280)", () => { await expect(server.start()).rejects.toMatchObject({ code: "EADDRINUSE", }); - server = null; + // Deliberately NOT nulled: `start()` installs the process-global test-server + // control before it binds, and only `stop()` clears it. Dropping the + // reference here would skip teardown and leave that global pointing at a + // dead server for the rest of the worker. }); it("is carried from the fixture's config file to the resolved server config", async () => { diff --git a/core/auth/oauthUx.ts b/core/auth/oauthUx.ts index eba849777e..1a1dfc4dd4 100644 --- a/core/auth/oauthUx.ts +++ b/core/auth/oauthUx.ts @@ -383,7 +383,12 @@ export function insecureTokenEndpointTitle(): string { * Does not echo the SDK's own message, which reads as a flat refusal and tells * the user nothing about which lever to reach for. * - * The wording is "outside the SDK's loopback exemption", never "not loopback". + * The scheme half says "not HTTPS" rather than "plain HTTP": the SDK's check is + * `protocol !== "https:"`, so anything else an authorization server advertises + * — including a mistyped `ftp:` or `ws:` endpoint — lands here too, and naming + * the wrong scheme would send the reader looking for a problem they do not have. + * + * The host half is "outside the SDK's loopback exemption", never "not loopback". * The motivating hosts — `tenant.app.localhost` (#1944), the `localhost.` * fixture — *are* loopback by RFC 6761 and by every resolver on the machine; * what they are outside is a three-literal allow-list. Calling them non-loopback @@ -400,10 +405,10 @@ export function insecureTokenEndpointMessage(options: { return ( `Authorization for ${target} was stopped before any credentials were sent: ` + `its token endpoint ${truncateUrlForDisplay(options.tokenEndpoint)} is ` + - "plain HTTP on a host outside the MCP SDK's loopback exemption, which " + - "covers only localhost, 127.0.0.1 and ::1. Re-authenticating cannot change " + - "this. Serve the token endpoint over HTTPS, or move it to one of those " + - "three spellings — Server Settings → Authorization has a Token URL override " + - "if the authorization server advertises a different one." + "not HTTPS, and its host is outside the MCP SDK's loopback exemption, " + + "which covers only localhost, 127.0.0.1 and ::1. Re-authenticating cannot " + + "change this. Serve the token endpoint over HTTPS, or move it to one of " + + "those three spellings — Server Settings → Authorization has a Token URL " + + "override if the authorization server advertises a different one." ); } From 5af2eeeeb6573dcc7af7c0d4d366d5a559e2def8 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 6 Sep 2026 23:58:33 -0400 Subject: [PATCH 05/19] fix: tear down the client on the terminal connect arm (Copilot review round 5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit InspectorClient.connect() sets its status to "error" and dispatches statusChange before rethrowing — the only exemption is a connect-auth- recovery error, which this is not — and InspectorView pins the monitoring sidebar open on that transition and paints the card red. So returning without a teardown presented this as the failed connect attempt the notice explicitly says it is not: careful configuration-error copy on top of the full failure presentation. The authenticate() arm below already disconnects for the same reason. The tests mock connect(), so they never reach that status transition and could not have caught it; they now assert the teardown, which is the available proxy. The EMA arm directly above has the identical gap but predates this branch, so it is filed as #2284 rather than changed here. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- .../src/hooks/useConnectionLifecycle.test.tsx | 8 ++++++++ .../web/src/hooks/useConnectionLifecycle.ts | 19 +++++++++++++++---- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/clients/web/src/hooks/useConnectionLifecycle.test.tsx b/clients/web/src/hooks/useConnectionLifecycle.test.tsx index 5561c8213c..ab38522621 100644 --- a/clients/web/src/hooks/useConnectionLifecycle.test.tsx +++ b/clients/web/src/hooks/useConnectionLifecycle.test.tsx @@ -623,6 +623,13 @@ describe("useConnectionLifecycle", () => { // is how the raw SDK text would creep back in beside the good copy. expect(toastTitles()).not.toContain('Failed to connect to "Server a"'); expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("a"); + // The real `connect()` sets status `"error"` and dispatches + // `statusChange` before rethrowing, which paints the card red and pins + // the monitoring sidebar open — presenting this as the failed connect + // attempt the notice says it is not. `connect` is mocked here, so the + // teardown is what this asserts; without it the client is left in that + // state. + expect(disconnectSpy).toHaveBeenCalled(); }); it("finds an insecure token endpoint wrapped under `cause` on the connect path", async () => { @@ -641,6 +648,7 @@ describe("useConnectionLifecycle", () => { expect(toastTitles()).toContain("Token endpoint is not secure"); expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("a"); + expect(disconnectSpy).toHaveBeenCalled(); }); it("reports an insecure token endpoint raised by the 401 authorization attempt", async () => { diff --git a/clients/web/src/hooks/useConnectionLifecycle.ts b/clients/web/src/hooks/useConnectionLifecycle.ts index a24aaec595..d7dd0fd65f 100644 --- a/clients/web/src/hooks/useConnectionLifecycle.ts +++ b/clients/web/src/hooks/useConnectionLifecycle.ts @@ -31,6 +31,7 @@ import { } from "@inspector/core/client/types.js"; import { isEmaClientNotConfiguredError } from "@inspector/core/auth/ema/clientConfigError.js"; import { showInsecureTokenEndpointNotice } from "../lib/insecureTokenEndpointNotice"; +import { findInsecureTokenEndpoint } from "@inspector/core/auth/insecureTokenEndpoint.js"; import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; import type { RemoteInspectorClientStorage } from "@inspector/core/mcp/remote/index.js"; import type { SessionRef } from "./useSessionRef"; @@ -639,10 +640,20 @@ export function useConnectionLifecycle({ return; } // SEP-2207 (#2280): a token endpoint the SDK will not post credentials - // to. Terminal, so it gets the same treatment as the EMA arm above - // rather than the generic "OAuth authorization failed" toast, whose - // detail line would be the raw SDK text. - if (showInsecureTokenEndpointNotice(err, target.name)) { + // to. Terminal, so it gets a notice of its own rather than the generic + // "Failed to connect" toast, whose detail line would be the raw SDK + // text. + // + // The teardown is load-bearing, not tidiness. `connect()` sets its + // status to `"error"` and dispatches `statusChange` *before* rethrowing + // (it is not a connect-auth-recovery error), and `InspectorView` pins + // the monitoring sidebar open on that transition and paints the card + // red. Returning without it would present this as the failed connect + // attempt the notice explicitly says it is not. The `authenticate()` + // arm below already disconnects for the same reason. + if (findInsecureTokenEndpoint(err)) { + await client.disconnect().catch(() => {}); + showInsecureTokenEndpointNotice(err, target.name); return; } From 54b2c5b7044b626052a1302c027bbf0f31faff01 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 00:23:12 -0400 Subject: [PATCH 06/19] fix: address Copilot review round 6 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 6 posted zero inline comments and named two findings in the review body. Both were real, so the comment count was not the signal. - The root-dot bug was still live one layer deeper. Round 2 fixed the origin allow-list and the banner, but the sandbox and app-origin controllers still built their advertised URLs with canonicalUrlHost. With HOST=localhost. the sandbox origin http://localhost.:PORT is named in the app document's frame-ancestors, where the browser drops it as an invalid host-source and blanks the INNER MCP Apps frame. canonicalOriginHost now lives in core/node/hostUrl.ts and every advertised URL goes through it — banner, allow-list, sandbox URL, app origin. Bind hosts are still untouched. - ALLOWED_ORIGINS=garbage enabled the widening. The flag keyed on whether any entry survived validation, so a stated-but-unparseable list fell back to the default AND silently gained every *.localhost. A widening should fail closed, so it now keys on whether a list was stated at all; blank, whitespace and comma-only still read as unset, and the list's own fallback is unchanged. Tests: the advertised URL for both controllers under HOST=localhost., and the wholly-invalid ALLOWED_ORIGINS case. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- clients/web/server/app-origin-controller.ts | 9 +++- clients/web/server/sandbox-controller.ts | 9 +++- clients/web/server/web-server-config.ts | 51 ++++++++----------- .../server/app-origin-controller.test.ts | 7 +++ .../server/sandbox-controller.test.ts | 18 +++++++ .../server/web-server-config.test.ts | 20 ++++++++ core/node/hostUrl.ts | 25 +++++++++ 7 files changed, 105 insertions(+), 34 deletions(-) diff --git a/clients/web/server/app-origin-controller.ts b/clients/web/server/app-origin-controller.ts index 068c7fe125..529aa66d6b 100644 --- a/clients/web/server/app-origin-controller.ts +++ b/clients/web/server/app-origin-controller.ts @@ -68,7 +68,7 @@ import { createServer, type Server } from "node:http"; import { randomBytes } from "node:crypto"; import { - canonicalUrlHost, + canonicalOriginHost, isAllInterfacesHost, } from "../../../core/node/hostUrl.ts"; import { DEFAULT_BIND_HOST } from "./resolve-bind-host.js"; @@ -375,7 +375,12 @@ export function createAppOriginController( // A wildcard bind isn't reachable as `http://0.0.0.0:PORT`, but it // does serve loopback — advertise `localhost` there, and otherwise // the same canonical host the origin allow-list emits. - const canonicalHost = canonicalUrlHost(host); + // `canonicalOriginHost`, not `canonicalUrlHost`: this URL's origin is + // named in a `frame-ancestors` directive, and a root-dotted host is + // not a valid CSP host-source — so `HOST=localhost.` would advertise + // a reachable URL whose origin the browser silently drops, blanking + // the frame. The bind host above is untouched. + const canonicalHost = canonicalOriginHost(host); const urlHost = isAllInterfacesHost(canonicalHost) ? "localhost" : canonicalHost; diff --git a/clients/web/server/sandbox-controller.ts b/clients/web/server/sandbox-controller.ts index cfefcc43aa..189b99caba 100644 --- a/clients/web/server/sandbox-controller.ts +++ b/clients/web/server/sandbox-controller.ts @@ -8,7 +8,7 @@ import { readFileSync } from "node:fs"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; import { - canonicalUrlHost, + canonicalOriginHost, isAllInterfacesHost, } from "../../../core/node/hostUrl.ts"; import { DEFAULT_BIND_HOST } from "./resolve-bind-host.js"; @@ -324,7 +324,12 @@ export function createSandboxController( // 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); + // `canonicalOriginHost`, not `canonicalUrlHost`: this URL's origin is + // named in a `frame-ancestors` directive, and a root-dotted host is + // not a valid CSP host-source — so `HOST=localhost.` would advertise + // a reachable URL whose origin the browser silently drops, blanking + // the frame. The bind host above is untouched. + const canonicalHost = canonicalOriginHost(host); const urlHost = isAllInterfacesHost(canonicalHost) ? "localhost" : canonicalHost; diff --git a/clients/web/server/web-server-config.ts b/clients/web/server/web-server-config.ts index 4fad9bb6fe..afd584132d 100644 --- a/clients/web/server/web-server-config.ts +++ b/clients/web/server/web-server-config.ts @@ -22,7 +22,7 @@ import { resolveSandboxPort } from "./sandbox-controller.js"; import { resolveAppOriginPort } from "./app-origin-controller.js"; import { resolveBindHostname } from "./resolve-bind-host.js"; import { - canonicalUrlHost, + canonicalOriginHost, isAllInterfacesHost, } from "../../../core/node/hostUrl.ts"; @@ -279,7 +279,7 @@ export interface BuildWebServerConfigOptions { * 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. + * `canonicalOriginHost(hostname)`, which always brackets an IPv6 literal. */ const LOOPBACK_HOSTNAMES = new Set(["localhost", "127.0.0.1", "[::1]"]); @@ -301,32 +301,6 @@ function loopbackOrigins(port: number): string[] { ]; } -/** - * The canonical host to put in a **browser-facing** origin or advertised URL. - * - * {@link canonicalUrlHost} plus one thing it deliberately does not do: drop a - * root FQDN dot. `HOST=localhost.` binds loopback and every resolver treats it - * as `localhost`, but the WHATWG serializer keeps the dot — and a root-dotted - * host is **not a valid CSP `host-source`** (the grammar admits no empty final - * label). Advertising `http://localhost.:PORT` therefore produced a URL that - * passed the exact-match API guard and then blanked the MCP Apps frame, because - * the browser dropped that source from `frame-ancestors` and nothing else - * covered the embedder. - * - * Used by **both** the banner and {@link defaultAllowedOrigins}, which is what - * keeps `banner ⊆ allowedOrigins` true through this normalization rather than - * in spite of it. The **bind host is untouched** — `config.hostname` still - * binds exactly what was typed; this only changes the spelling we hand a - * browser. - * - * Deliberately NOT folded into `canonicalUrlHost`: `isLocalhostSubdomainHost` - * depends on that function preserving the dot, precisely so it can reject - * `app.localhost.` for the same CSP reason. The two live at different layers. - */ -function canonicalOriginHost(hostname: string): string { - return canonicalUrlHost(hostname).replace(/\.$/, ""); -} - /** * Whether the default origin allow-list for `hostname` should additionally * admit `*.localhost` origins (#1944). @@ -379,7 +353,7 @@ export function allowLocalhostSubdomainOriginsFor(hostname: string): boolean { * 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 + * ({@link canonicalOriginHost}), 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 @@ -514,6 +488,17 @@ export function buildWebServerConfig( // 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. + // Whether the operator *stated* an allow-list, as distinct from whether any + // of it survived validation. `""` / `" "` / `","` carry no entry and are + // treated as unset; `ALLOWED_ORIGINS=garbage` states one and happens to be + // unusable. The distinction only drives the `*.localhost` widening below — + // the list itself still falls back to the default in both cases, which is the + // documented fail-closed behavior and is unchanged. + const explicitOriginEntries = + process.env.ALLOWED_ORIGINS?.split(",").filter((o) => o.trim() !== "") ?? + []; + const hasExplicitOriginList = explicitOriginEntries.length > 0; + const configuredOrigins = process.env.ALLOWED_ORIGINS?.split(",") .map((o) => o.trim()) .filter(Boolean) @@ -558,8 +543,14 @@ export function buildWebServerConfig( allowedOrigins: configuredOrigins?.length ? configuredOrigins : defaultAllowedOrigins(hostname, port), + // Keyed on whether an allow-list was *stated*, not on whether one survived. + // A widening should fail closed: an operator who wrote `ALLOWED_ORIGINS` + // and got it wrong has still said "these are the origins I want", and + // silently adding every `*.localhost` on top of a value we could not parse + // is the opposite of what they asked for. The warnings already name each + // dropped entry. allowLocalhostSubdomainOrigins: - !configuredOrigins?.length && allowLocalhostSubdomainOriginsFor(hostname), + !hasExplicitOriginList && allowLocalhostSubdomainOriginsFor(hostname), sandboxPort, sandboxHost: hostname, appOriginPort, diff --git a/clients/web/src/test/integration/server/app-origin-controller.test.ts b/clients/web/src/test/integration/server/app-origin-controller.test.ts index 605a43ab3c..46c4124ce0 100644 --- a/clients/web/src/test/integration/server/app-origin-controller.test.ts +++ b/clients/web/src/test/integration/server/app-origin-controller.test.ts @@ -208,6 +208,13 @@ describe("createAppOriginController", () => { ); }); + it("drops a root FQDN dot from the app origin (#2280 review round 6)", async () => { + controller = createAppOriginController({ port: 0, host: "localhost." }); + const { url } = await controller.start(); + expect(url).not.toContain("localhost.:"); + expect(controller.getOrigin()).toMatch(/^http:\/\/localhost:\d+$/); + }); + it("mints a distinct, unguessable id per document", async () => { controller = createAppOriginController({ port: 0, host: "127.0.0.1" }); const { url } = await controller.start(); 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 f653ce5c57..a7702cf662 100644 --- a/clients/web/src/test/integration/server/sandbox-controller.test.ts +++ b/clients/web/src/test/integration/server/sandbox-controller.test.ts @@ -116,6 +116,24 @@ describe("sandboxFrameAncestors", () => { }); }); +describe("advertised URL host (#2280 review round 6)", () => { + it("drops a root FQDN dot from the sandbox URL", async () => { + // The sandbox URL's origin is named in the app-origin `frame-ancestors`, + // and a root-dotted host is not a valid CSP host-source — so + // `HOST=localhost.` advertised a reachable URL whose origin the browser + // silently drops, blanking the INNER app frame. Fixing the outer origin + // allow-list alone left this one live. + const controller = createSandboxController({ port: 0, host: "localhost." }); + try { + const { url } = await controller.start(); + expect(url).not.toContain("localhost.:"); + expect(url).toMatch(/^http:\/\/localhost:\d+\/sandbox$/); + } finally { + await controller.close(); + } + }); +}); + describe("resolveSandboxPort", () => { let envSnapshot: { mcp?: string; server?: string }; let warnSpy: ReturnType; 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 25af22c6fe..365b2952f2 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 @@ -183,6 +183,26 @@ describe("buildWebServerConfigFromEnv", () => { expect(cfg.allowedOrigins).toEqual(["http://a:1", "http://b:2"]); }); + it("turns the *.localhost widening off when ALLOWED_ORIGINS is set but wholly invalid", () => { + // A widening fails closed. The operator stated an allow-list and got it + // wrong; the list itself still falls back to the default (documented, + // unchanged), but adding every *.localhost on top of a value we could not + // parse is the opposite of what they asked for. + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + process.env.ALLOWED_ORIGINS = "not-a-url, also-not-a-url"; + const cfg = buildWebServerConfigFromEnv(); + expect(cfg.allowedOrigins).toEqual([ + "http://localhost:6274", + "http://127.0.0.1:6274", + "http://[::1]:6274", + ]); + expect(cfg.allowLocalhostSubdomainOrigins).toBe(false); + } finally { + warnSpy.mockRestore(); + } + }); + it("turns the *.localhost widening off when ALLOWED_ORIGINS is set (#1944)", () => { // ALLOWED_ORIGINS *replaces* the default list, as documented. A list that // states which origins are allowed must not silently gain entries. diff --git a/core/node/hostUrl.ts b/core/node/hostUrl.ts index e20900b770..7820270f75 100644 --- a/core/node/hostUrl.ts +++ b/core/node/hostUrl.ts @@ -181,6 +181,31 @@ export function isLoopbackHost(host: string): boolean { ); } +/** + * The canonical host to put in a **browser-facing** origin or advertised URL. + * + * {@link canonicalUrlHost} plus one thing it deliberately does not do: drop a + * root FQDN dot. `HOST=localhost.` binds loopback and every resolver treats it + * as `localhost`, but the WHATWG serializer keeps the dot — and a root-dotted + * host is **not a valid CSP `host-source`** (the grammar admits no empty final + * label). Anything we advertise is eventually named in a `frame-ancestors` + * directive, so emitting one produces a URL that works for `/api/*` and then + * blanks an MCP Apps frame, with nothing in between to explain it. + * + * Every advertised URL goes through here — the banner, the origin allow-list, + * the sandbox proxy URL and the app-origin URL — which is what keeps them + * agreeing. The **bind host is never touched**: callers still `listen()` on + * exactly what was configured, and this only changes the spelling handed to a + * browser. + * + * Deliberately NOT folded into {@link canonicalUrlHost}, which + * {@link isLocalhostSubdomainHost} needs to preserve the dot so it can reject + * `app.localhost.` for this same CSP reason. The two sit at different layers. + */ +export function canonicalOriginHost(host: string): string { + return canonicalUrlHost(host).replace(/\.$/, ""); +} + /** * True when `host` is a name reserved to the loopback interface by * [RFC 6761 §6.3](https://www.rfc-editor.org/info/rfc6761/) *below* the From 2e41d49784d63f81ad1cc2658dbb16c09d9c5449 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 01:02:57 -0400 Subject: [PATCH 07/19] fix: address Copilot review round 7 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings; the first is a regression this branch introduced in round 6. - Normalizing the banner host broke `banner ⊆ allowedOrigins` whenever an explicit ALLOWED_ORIGINS was in use: HOST=localhost. with ALLOWED_ORIGINS=http://localhost.:6274 advertised http://localhost:6274 while allow-listing the dotted form, and WHATWG treats those as distinct origins, so opening the URL we printed would 403. Explicit entries are now normalized through the same helper. A root-dotted entry could never have admitted an MCP Apps embedder either, since it is not a valid CSP host-source. The opaque-origin and wildcard guards still read URL.origin rather than a reconstructed string — that serialization is what collapses every opaque case to the literal "null", and rebuilding the origin first hands them a plausible-looking `localhost://` instead. The existing test caught that on the first attempt; normalization now happens only after both guards pass. - The stale-banner clear added in round 4 existed on exactly one arm. The callback, command-path and deferred-resume arms all omitted it, so a banner from an earlier failure kept its dead Re-authenticate button beside the terminal notice. All four now go through one reportTerminalInsecureTokenEndpoint wrapper, and both connect-lifecycle arms clear it too — wrapping it is what stops the next arm omitting it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- clients/web/server/web-server-config.ts | 29 +++++++++++-- .../web/src/hooks/useConnectionLifecycle.ts | 9 +++- .../web/src/hooks/useOAuthRecovery.test.tsx | 27 ++++++++++++ clients/web/src/hooks/useOAuthRecovery.ts | 41 ++++++++++++++----- .../server/web-server-config.test.ts | 33 +++++++++++++++ 5 files changed, 124 insertions(+), 15 deletions(-) diff --git a/clients/web/server/web-server-config.ts b/clients/web/server/web-server-config.ts index afd584132d..fded53f2b2 100644 --- a/clients/web/server/web-server-config.ts +++ b/clients/web/server/web-server-config.ts @@ -504,7 +504,13 @@ export function buildWebServerConfig( .filter(Boolean) .map((o) => { try { - const { origin } = new URL(o); + const parsed = new URL(o); + // Both guards below read `URL.origin`, NOT a reconstructed string — + // that serialization is what collapses every opaque case to the literal + // "null", and rebuilding the origin by hand first would hand them a + // plausible-looking `localhost://` instead and wave them through. + // Normalization happens after they have both passed. + const { origin: parsedOrigin } = parsed; // 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:`, @@ -515,14 +521,29 @@ export function buildWebServerConfig( // 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"); + if (parsedOrigin === "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; + if (parsedOrigin.includes("*")) throw new Error("wildcard origin"); + // Root-dotted hosts are normalized here too, not just in the derived + // default. Two reasons, and the first is a hard bug: the banner reads + // `canonicalOriginHost`, so an explicit `http://localhost.:6274` would + // leave the advertised URL (`http://localhost:6274`) outside its own + // allow-list — WHATWG treats those as distinct origins, so opening the + // URL we printed would 403. The second is the standing one: a + // root-dotted host is not a valid CSP host-source, so such an entry + // could never admit an MCP Apps embedder anyway. + // + // Rebuilt from `parsed` rather than string-edited: `parsed.port` is + // empty for a scheme-default port, which is what keeps the `:80` drop + // that `URL.origin` already performed. + const host = canonicalOriginHost(parsed.hostname); + return parsed.port + ? `${parsed.protocol}//${host}:${parsed.port}` + : `${parsed.protocol}//${host}`; } catch { console.warn(`Ignoring invalid ALLOWED_ORIGINS entry: ${o}`); return null; diff --git a/clients/web/src/hooks/useConnectionLifecycle.ts b/clients/web/src/hooks/useConnectionLifecycle.ts index d7dd0fd65f..73b8c349b2 100644 --- a/clients/web/src/hooks/useConnectionLifecycle.ts +++ b/clients/web/src/hooks/useConnectionLifecycle.ts @@ -654,6 +654,10 @@ export function useConnectionLifecycle({ if (findInsecureTokenEndpoint(err)) { await client.disconnect().catch(() => {}); showInsecureTokenEndpointNotice(err, target.name); + // Clear a banner left by an earlier failure: its Re-authenticate + // button is just as dead as the one this arm declines to offer, and + // the user cannot tell which failure it belongs to. + setReAuthBanner(null); return; } @@ -741,8 +745,10 @@ export function useConnectionLifecycle({ }); return; } - // See the SEP-2207 note on the handshake arm above (#2280). + // See the SEP-2207 note on the handshake arm above (#2280). The + // disconnect already happened at the top of this catch. if (showInsecureTokenEndpointNotice(authErr, target.name)) { + setReAuthBanner(null); return; } // The connect attempt failed, same as any other handshake error — @@ -789,6 +795,7 @@ export function useConnectionLifecycle({ setFailedServerId, prepareOAuthRedirect, finalizeExplicitDisconnect, + setReAuthBanner, ], ); diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index 532aaaf833..c50843f0dd 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/clients/web/src/hooks/useOAuthRecovery.test.tsx @@ -1004,6 +1004,33 @@ describe("useOAuthRecovery", () => { expect(toastTitles()).toContain("Token endpoint is not secure"); }); + it("clears a stale banner when a command-path failure is terminal", async () => { + // Every terminal arm goes through one wrapper for this reason: a banner + // left by an earlier failure carries a Re-authenticate button just as + // dead as the one this arm declines to offer, and the user cannot tell + // which failure it belongs to. + const client = fakeClient(); + const h = harness({ servers: [entry("a")], activeServerId: "a", client }); + await act(async () => { + client.emit("oauthError", { error: new Error("session expired") }); + }); + await waitFor(() => expect(h.api().reAuthBanner?.serverId).toBe("a")); + + await act(async () => { + await h + .api() + .runWithCommandAuthRecovery( + () => + Promise.reject( + new InsecureTokenEndpointError("http://localhost.:8091/token"), + ), + "tool", + ); + }); + expect(toastTitles()).toContain("Token endpoint is not secure"); + expect(h.api().reAuthBanner).toBeNull(); + }); + it("shows the terminal notice instead of the generic title in the background form", async () => { // The `errorTitle` call sites would otherwise render the raw SDK text // under a generic heading. diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index 76cd96dffb..4ee96a5f9c 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -384,6 +384,29 @@ export function useOAuthRecovery({ [sessionRef], ); + /** + * Report a terminal SEP-2207 refusal (#2280) and clear any re-auth banner. + * + * Every arm goes through this rather than calling the notice helper directly. + * The banner clear is not incidental: a banner left from an *earlier* failure + * carries a Re-authenticate button that is just as dead as the one this + * change removes, and the user cannot tell which failure it belongs to. Round + * 4 fixed that for one arm by hand; wrapping it is what stops the next arm + * from omitting it. + * + * Returns whether the error was claimed, so callers keep their fall-through. + */ + const reportTerminalInsecureTokenEndpoint = useCallback( + (err: unknown, serverName?: string): boolean => { + if (!showInsecureTokenEndpointNotice(err, serverName)) { + return false; + } + setReAuthBanner(null); + return true; + }, + [setReAuthBanner], + ); + const showReAuthBanner = useCallback( ( serverId: string, @@ -396,12 +419,7 @@ export function useOAuthRecovery({ // way. Claimed here, at the single funnel every re-auth banner goes // through, rather than at each of its call sites — a new caller then gets // the right behavior by default instead of by remembering. - if (showInsecureTokenEndpointNotice(detail, server?.name)) { - // Clear one already on screen. Returning without this leaves a stale - // Re-authenticate button beside the terminal notice — the exact - // affordance this arm exists to remove, just sourced from an earlier - // failure rather than this one. - setReAuthBanner(null); + if (reportTerminalInsecureTokenEndpoint(detail, server?.name)) { return; } const message = reAuthBannerMessage({ @@ -424,7 +442,7 @@ export function useOAuthRecovery({ message, }); }, - [sessionRef], + [sessionRef, reportTerminalInsecureTokenEndpoint], ); /** Clears pending OAuth resume state — explicit user disconnect only. */ @@ -839,7 +857,7 @@ export function useOAuthRecovery({ const server = sessionRef.current.servers.find( (s) => s.id === activeServerId, ); - if (showInsecureTokenEndpointNotice(err, server?.name)) { + if (reportTerminalInsecureTokenEndpoint(err, server?.name)) { return undefined; } throw err; @@ -850,6 +868,7 @@ export function useOAuthRecovery({ activeServerId, handleCommandScopedAuthRecovery, sessionRef, + reportTerminalInsecureTokenEndpoint, ], ); @@ -970,7 +989,7 @@ export function useOAuthRecovery({ const failedServer = sessionRef.current.servers.find( (s) => s.id === pending.serverId, ); - if (showInsecureTokenEndpointNotice(err, failedServer?.name)) { + if (reportTerminalInsecureTokenEndpoint(err, failedServer?.name)) { return; } // The slot was cleared above only to keep a tab-visible event and a @@ -1018,6 +1037,7 @@ export function useOAuthRecovery({ } }, [ + reportTerminalInsecureTokenEndpoint, sessionRef, inspectorClient, connectionStatus, @@ -1372,7 +1392,7 @@ export function useOAuthRecovery({ // Above `setFailedServerId` for the same reason the EMA arm is: this is // a configuration error, not a failed attempt, so it should not flag // the card red or pull the monitoring sidebar open. - if (showInsecureTokenEndpointNotice(err, server.name)) { + if (reportTerminalInsecureTokenEndpoint(err, server.name)) { return; } // The token exchange (or the re-handshake behind it) failed. Flag the @@ -1479,6 +1499,7 @@ export function useOAuthRecovery({ initialConfigSettledRef, clearResultPanels, showReAuthBanner, + reportTerminalInsecureTokenEndpoint, webOAuthStorage, setUi, setActiveTab, 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 365b2952f2..1c0574bb81 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 @@ -183,6 +183,39 @@ describe("buildWebServerConfigFromEnv", () => { expect(cfg.allowedOrigins).toEqual(["http://a:1", "http://b:2"]); }); + it("normalizes a root-dotted explicit entry so the banner stays inside its own list", () => { + // The banner reads `canonicalOriginHost`, so leaving the dot on an explicit + // entry would advertise `http://localhost:6274` while allow-listing + // `http://localhost.:6274` — WHATWG treats those as distinct origins, so + // opening the URL we printed would 403. + process.env.HOST = "localhost."; + process.env.ALLOWED_ORIGINS = "http://localhost.:6274"; + const cfg = buildWebServerConfigFromEnv(); + expect(cfg.allowedOrigins).toEqual(["http://localhost:6274"]); + + const log = vitestSpyOnConsoleLog(); + try { + printServerBanner(cfg, 6274, "tok", undefined); + } finally { + log.restore(); + } + const advertised = log.lines.find((l) => l.includes("http://localhost")); + expect(advertised).toBeDefined(); + // The invariant, asserted rather than assumed: whatever we printed is in + // the list the guard compares against. + expect(cfg.allowedOrigins.some((o) => advertised!.includes(o))).toBe(true); + }); + + it("keeps a non-default port and an IPv6 literal intact while normalizing", () => { + process.env.ALLOWED_ORIGINS = + "http://localhost.:8080, http://[::1]:6274, https://a.example.com"; + expect(buildWebServerConfigFromEnv().allowedOrigins).toEqual([ + "http://localhost:8080", + "http://[::1]:6274", + "https://a.example.com", + ]); + }); + it("turns the *.localhost widening off when ALLOWED_ORIGINS is set but wholly invalid", () => { // A widening fails closed. The operator stated an allow-list and got it // wrong; the list itself still falls back to the default (documented, From 8553b62cd1727b37fea013a368c283f5c27fbf85 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 01:14:44 -0400 Subject: [PATCH 08/19] fix: report a terminal token-endpoint refusal from the banner action (round 8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fifth and last path into the SDK's SEP-2207 refusal. `onReauthenticateFromBanner` calls `authenticate()` directly and its catch always showed the generic "OAuth authorization failed" toast with the raw SDK message. A connected session can raise an ordinary re-auth banner and only hit the unusable token endpoint when the user *clicks* it — the worst place to lose the guidance, since they have just been told retrying is the fix. No banner clear is needed here: the callback already clears it before starting. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- .../src/hooks/useConnectionLifecycle.test.tsx | 33 +++++++++++++++++++ .../web/src/hooks/useConnectionLifecycle.ts | 10 ++++++ 2 files changed, 43 insertions(+) diff --git a/clients/web/src/hooks/useConnectionLifecycle.test.tsx b/clients/web/src/hooks/useConnectionLifecycle.test.tsx index ab38522621..88a3d355b7 100644 --- a/clients/web/src/hooks/useConnectionLifecycle.test.tsx +++ b/clients/web/src/hooks/useConnectionLifecycle.test.tsx @@ -1284,6 +1284,39 @@ describe("useConnectionLifecycle", () => { ); }); + it("reports a terminal token-endpoint refusal from the banner action", async () => { + // The path a user reaches by *acting*: an ordinary re-auth banner, they + // click Re-authenticate, and the exchange is refused. The worst place to + // fall back to the raw SDK text, since they have just been told that + // retrying is the fix (#2280). + const h = harness({ servers: [entry("a")] }); + await act(async () => { + await h.api().onToggleConnection("a"); + }); + const client = lastClient(h); + authenticateSpy.mockRejectedValue( + new InsecureTokenEndpointError("http://localhost.:8091/token"), + ); + h.rerender({ + servers: [entry("a")], + activeServerId: "a", + connectionStatus: "connected", + client, + reAuthBanner: { serverId: "a", message: "lapsed" }, + }); + + await act(async () => { + h.api().onReauthenticateFromBanner(); + }); + + await waitFor(() => + expect(toastTitles()).toContain("Token endpoint is not secure"), + ); + expect(toastTitles()).not.toContain( + 'OAuth authorization failed for "Server a"', + ); + }); + it("falls back to an unnamed failure toast for an unknown server", async () => { const h = harness({ servers: [entry("a")] }); await act(async () => { diff --git a/clients/web/src/hooks/useConnectionLifecycle.ts b/clients/web/src/hooks/useConnectionLifecycle.ts index 73b8c349b2..104e0aa939 100644 --- a/clients/web/src/hooks/useConnectionLifecycle.ts +++ b/clients/web/src/hooks/useConnectionLifecycle.ts @@ -992,6 +992,16 @@ export function useConnectionLifecycle({ authorizationUrl: authUrl, }); } catch (err) { + // SEP-2207 (#2280), and this is the path a user reaches by *acting*: + // a connected session raises an ordinary re-auth banner, they click + // Re-authenticate, and the token exchange is refused. Without this + // the generic toast below reports it with the raw SDK text — the + // worst place to lose the guidance, since they have just been told + // retrying is the fix. No banner clear is needed: this callback + // already cleared it before starting. + if (showInsecureTokenEndpointNotice(err, server?.name)) { + return; + } const message = err instanceof Error ? err.message : String(err); notifications.show({ title: server From e1f78174c366ed62a528ba4dd8a3b09d4f25ebdd Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 01:45:31 -0400 Subject: [PATCH 09/19] fix: admit *.localhost in the sandbox proxy's referrer check (round 9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The third gate an embedder passes, and the one this branch had left closed. static/sandbox_proxy.html carries its own hardcoded allow-list, so browsing at http://mcp.localhost:6274 cleared the backend origin guard and the CSP and then had the proxy page itself throw — the app loads, connects, and only the Apps tab dies, which is the worst shape this could have taken. It is enforced in static HTML that ships as bytes and is never bundled, so it cannot read ALLOWED_ORIGINS; a fixed rule is the only option. Hardcoding the suffix is safe in exactly the way hardcoding `localhost` already is: RFC 6761 reserves it and it is not publicly registrable, so no attacker can obtain such a referrer. Parsed rather than pattern-matched — not because the old regex was wrong (it correctly rejected http://localhost@evil.com/, where the real host is evil.com) but because a suffix rule makes a regex re-earn that correctness per label. The root-dotted form is rejected here too, matching the backend, since its frame-ancestors source cannot be expressed in CSP anyway. The file had no test coverage at all — nothing imports it. The new test extracts the function from the shipped bytes and exercises it, including the userinfo direction that makes hardcoding a suffix safe. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- clients/web/README.md | 2 +- .../server/sandbox-proxy-referrer.test.ts | 94 +++++++++++++++++++ clients/web/static/sandbox_proxy.html | 55 ++++++++++- 3 files changed, 147 insertions(+), 4 deletions(-) create mode 100644 clients/web/src/test/integration/server/sandbox-proxy-referrer.test.ts diff --git a/clients/web/README.md b/clients/web/README.md index 1046c6673d..31aa7d032d 100644 --- a/clients/web/README.md +++ b/clients/web/README.md @@ -406,7 +406,7 @@ The guard blocks only the **wildcard** all-interfaces addresses. Binding a **spe 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`. A `*.localhost` name needs nothing there: Vite's default `allowedHosts` already accepts `localhost` and everything under `.localhost`, which is why only the origin allow-list above had to change. -**MCP Apps caveats.** The MCP Apps sandbox runs on a **separate** port — `MCP_SANDBOX_PORT`, defaulting to a fixed **`6275`** (#2008; it was OS-assigned before, which meant it changed every run and so could never be named in a `forwardPorts` / `-p` / tunnel config written ahead of time). For the Apps tab to work off loopback, that sandbox port must be independently reachable from the browser — expose/forward `6275` alongside `6274`, or set `MCP_SANDBOX_PORT` to pick another. If the port is already taken the sandbox falls back to an OS-assigned one and warns, so a second Inspector still gets a working Apps tab locally — but the forwarded port is then wrong, which is what the warning tells you. (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. (A `*.localhost` embedder is fine — `*.localhost` *is* a legal CSP host-source, and the sandbox's `frame-ancestors` admits it whenever the origin allow-list does.) 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. +**MCP Apps caveats.** The MCP Apps sandbox runs on a **separate** port — `MCP_SANDBOX_PORT`, defaulting to a fixed **`6275`** (#2008; it was OS-assigned before, which meant it changed every run and so could never be named in a `forwardPorts` / `-p` / tunnel config written ahead of time). For the Apps tab to work off loopback, that sandbox port must be independently reachable from the browser — expose/forward `6275` alongside `6274`, or set `MCP_SANDBOX_PORT` to pick another. If the port is already taken the sandbox falls back to an OS-assigned one and warns, so a second Inspector still gets a working Apps tab locally — but the forwarded port is then wrong, which is what the warning tells you. (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. (A `*.localhost` embedder is fine — `*.localhost` *is* a legal CSP host-source, the sandbox's `frame-ancestors` admits it whenever the origin allow-list does, and the proxy page's own referrer check admits it too. That check is a **third** gate, enforced in `static/sandbox_proxy.html`, which ships as static bytes and so cannot read `ALLOWED_ORIGINS` — it admits the two loopback literals and the reserved `*.localhost` suffix and nothing else, which is why hosting the Apps tab on any *other* custom origin still needs the edit its error message suggests.) 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. diff --git a/clients/web/src/test/integration/server/sandbox-proxy-referrer.test.ts b/clients/web/src/test/integration/server/sandbox-proxy-referrer.test.ts new file mode 100644 index 0000000000..abe386c211 --- /dev/null +++ b/clients/web/src/test/integration/server/sandbox-proxy-referrer.test.ts @@ -0,0 +1,94 @@ +import { describe, it, expect } from "vitest"; +import { readFileSync } from "node:fs"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; + +/** + * Coverage for the sandbox proxy's embedder check (#1944). + * + * `clients/web/static/sandbox_proxy.html` is the **third** gate an embedder + * passes, after the backend's origin allow-list and the proxy response's + * `frame-ancestors`. It is plain inline script in a static file — it ships as + * bytes, is never bundled, and cannot read server configuration — so nothing + * else in the suite touches it, and it was left rejecting `*.localhost` while + * the other two gates were widened. That combination is the worst one: the app + * loads, connects, and only the Apps tab fails. + * + * The function is extracted from the shipped file and evaluated rather than + * imported, because there is no module to import. That is deliberate and it is + * the point: the assertions run against the exact text that gets served, so a + * change to the file is a change to this test's subject. + */ +const proxyHtml = readFileSync( + join( + dirname(fileURLToPath(import.meta.url)), + "../../../../static/sandbox_proxy.html", + ), + "utf-8", +); + +function loadIsAllowedEmbedder(): (referrer: string) => boolean { + const source = /function isAllowedEmbedder[\s\S]*?\n {6}}\n/.exec(proxyHtml); + if (!source) { + throw new Error( + "isAllowedEmbedder was not found in sandbox_proxy.html — if it was " + + "renamed or restructured, update this extraction rather than deleting " + + "the coverage.", + ); + } + return new Function(`${source[0]}; return isAllowedEmbedder;`)() as ( + referrer: string, + ) => boolean; +} + +describe("sandbox proxy embedder check", () => { + const isAllowedEmbedder = loadIsAllowedEmbedder(); + + it.each([ + // The two literals it has always admitted, unchanged. + "http://localhost:6274/", + "http://127.0.0.1:6274/", + // The newly supported space (#1944), at any depth and any port. + "http://mcp.localhost/", + "http://tenant.app.localhost:3300/", + // Both schemes, matching the backend's own predicate. + "https://mcp.localhost:8443/", + ])("admits %j", (referrer) => { + expect(isAllowedEmbedder(referrer)).toBe(true); + }); + + it.each([ + // Not the reserved suffix — an attacker-registrable name that merely + // contains or resembles it. + "http://mcp.localhost.evil.com/", + "http://evil.com/", + "http://notlocalhost/", + "http://localhost.evil.com/", + // A path segment that looks like a host. The check parses, so the host is + // `evil.com` and the rest is a path. + "http://evil.com/localhost", + // Degenerate labels. + "http://.localhost/", + "http://a..localhost/", + // Root-dotted, rejected here exactly as the backend rejects it: its CSP + // `frame-ancestors` source cannot be expressed, so admitting it would only + // move the failure one frame inward. + "http://app.localhost./", + // Unchanged: the bare literals stay http-only. + "https://localhost:6274/", + // Not a URL at all, and the empty referrer the caller already guards. + "not a url", + "", + ])("rejects %j", (referrer) => { + expect(isAllowedEmbedder(referrer)).toBe(false); + }); + + it("reads the host, not the userinfo", () => { + // The dangerous direction: the real host is `evil.com` and the reserved + // name is only credentials. Rejecting this is the property that makes + // hardcoding a suffix safe at all. + expect(isAllowedEmbedder("http://localhost@evil.com/")).toBe(false); + expect(isAllowedEmbedder("http://app.localhost@evil.com/")).toBe(false); + expect(isAllowedEmbedder("http://127.0.0.1@evil.com/")).toBe(false); + }); +}); diff --git a/clients/web/static/sandbox_proxy.html b/clients/web/static/sandbox_proxy.html index 5777c474c5..8de3a6f9d2 100644 --- a/clients/web/static/sandbox_proxy.html +++ b/clients/web/static/sandbox_proxy.html @@ -76,8 +76,57 @@ return allowList.join("; "); } - const ALLOWED_REFERRER_PATTERN = - /^http:\/\/(localhost|127\.0\.0\.1)(:|\/|$)/; + /** + * Whether `referrer` names a page allowed to embed this proxy. + * + * This is the THIRD gate an embedder passes, after the backend's origin + * allow-list and the proxy response's `frame-ancestors`. It is enforced + * here, in static HTML, so it cannot read the server's configuration — + * which is why it is a fixed rule rather than a mirror of + * `allowedOrigins`. + * + * `*.localhost` is admitted alongside the two loopback literals (#1944). + * Hardcoding that suffix is safe in exactly the way hardcoding + * `localhost` is: RFC 6761 §6.3 reserves it to the loopback interface and + * it is not publicly registrable, so no attacker can obtain such a + * referrer. Both schemes, matching the backend's own predicate; the two + * bare literals stay `http:`-only, as they were. + * + * Parsed rather than pattern-matched. Not because the old regex was + * wrong — it correctly rejected `http://localhost@evil.com/`, where the + * real host is `evil.com` — but because a suffix rule makes a regex earn + * that correctness a second time, over a longer expression, for every + * label. `URL` already knows where the host ends, so the check reads as + * the question it is asking. A root FQDN dot is deliberately NOT + * stripped, so + * `app.localhost.` is rejected here exactly as the backend rejects it — + * its `frame-ancestors` source cannot be expressed in CSP either, so + * admitting it here would only move the failure. + */ + function isAllowedEmbedder(referrer) { + let url; + try { + url = new URL(referrer); + } catch { + return false; + } + const host = url.hostname; + if ( + url.protocol === "http:" && + (host === "localhost" || host === "127.0.0.1") + ) { + return true; + } + if (url.protocol !== "http:" && url.protocol !== "https:") { + return false; + } + const suffix = ".localhost"; + if (!host.endsWith(suffix)) return false; + return host + .slice(0, -suffix.length) + .split(".") + .every((label) => label !== ""); + } if (window.self === window.top) { throw new Error("This file is only to be used in an iframe sandbox."); @@ -87,7 +136,7 @@ throw new Error("No referrer, cannot validate embedding site."); } - if (!document.referrer.match(ALLOWED_REFERRER_PATTERN)) { + if (!isAllowedEmbedder(document.referrer)) { throw new Error( `Embedding domain not allowed in referrer ${document.referrer}. (Consider updating the validation logic to allow your domain.)`, ); From 6810c738027e984753424b97cd741922a63a014c Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 02:04:09 -0400 Subject: [PATCH 10/19] fix: classify the satisfied-challenge connect retry (round 10) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sixth path. Inside the AuthRecoveryRequiredError branch, a satisfied challenge leads to a second connect() — and that retry still ends in a token exchange, so it can raise the terminal refusal on its own. Its catch reported it as a generic failed connect, flagged the card and set the connect-error banner, which is doubly wrong on this path: the Inspector has just told the user the authorization worked. Placed after the teardown that catch already performs, and before the flag it must not set. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- .../src/hooks/useConnectionLifecycle.test.tsx | 31 +++++++++++++++++++ .../web/src/hooks/useConnectionLifecycle.ts | 12 +++++++ 2 files changed, 43 insertions(+) diff --git a/clients/web/src/hooks/useConnectionLifecycle.test.tsx b/clients/web/src/hooks/useConnectionLifecycle.test.tsx index 88a3d355b7..59175d6a24 100644 --- a/clients/web/src/hooks/useConnectionLifecycle.test.tsx +++ b/clients/web/src/hooks/useConnectionLifecycle.test.tsx @@ -674,6 +674,37 @@ describe("useConnectionLifecycle", () => { expect(h.api().connectErrorMessage).toBeUndefined(); }); + it("reports a terminal refusal raised by the retried connect, without flagging the card", async () => { + // The satisfied-challenge retry still ends in a token exchange, so it can + // raise this on its own. Reporting it as a failed connect is doubly wrong + // here: the Inspector has just told the user the authorization worked + // (#2280). + connectSpy + .mockRejectedValueOnce( + new AuthRecoveryRequiredError( + new URL("https://as.example/authorize"), + { reason: "unauthorized" }, + ), + ) + .mockRejectedValueOnce( + new InsecureTokenEndpointError("http://localhost.:8091/token"), + ); + checkSpy.mockResolvedValueOnce(true); + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(connectSpy).toHaveBeenCalledTimes(2); + expect(toastTitles()).toContain("Token endpoint is not secure"); + expect(toastTitles()).not.toContain('Failed to connect to "Server a"'); + expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("a"); + expect(h.api().connectErrorMessage).toBeUndefined(); + // The teardown the generic arm does is still required on this one. + expect(disconnectSpy).toHaveBeenCalled(); + }); + it("retries the connect when the auth challenge is already satisfied", async () => { connectSpy.mockRejectedValueOnce( new AuthRecoveryRequiredError(new URL("https://as.example/authorize"), { diff --git a/clients/web/src/hooks/useConnectionLifecycle.ts b/clients/web/src/hooks/useConnectionLifecycle.ts index 104e0aa939..9d58cc8d5e 100644 --- a/clients/web/src/hooks/useConnectionLifecycle.ts +++ b/clients/web/src/hooks/useConnectionLifecycle.ts @@ -694,6 +694,18 @@ export function useConnectionLifecycle({ // held. The fetch log survives a disconnect, so the Network // diagnostics this issue is about are unaffected. await client.disconnect().catch(() => {}); + // SEP-2207 (#2280). The retried `connect()` above can raise the + // terminal refusal on its own — a satisfied challenge still ends in + // a token exchange — and reporting that as a failed connect attempt + // is doubly wrong here: the card goes red and the message is the + // raw SDK text, on the one path where the Inspector had just told + // the user the authorization *worked*. Placed after the teardown + // above, which this arm needs for the same reason the generic one + // does, and before the flag it must not set. + if (showInsecureTokenEndpointNotice(recoveryErr, target.name)) { + setReAuthBanner(null); + return; + } setFailedServerId(id); const message = recoveryErr instanceof Error From 5f9327ffa9d310c519bafcaa381d6f0f93a5cf98 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 02:25:53 -0400 Subject: [PATCH 11/19] docs: the notice is non-expiring, not non-dismissing (round 11) `autoClose: false` stops the notification expiring on a timer; Mantine's own close control still dismisses it, which is correct for a message someone has finished reading. Calling it "non-dismissing" made the reproduction steps in docs/test-servers.md describe behavior the build does not have. The code comment now says the same thing, so the wording does not drift back. Found in the review's Suppressed comments block, under a headline that otherwise reported no defects. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- clients/web/src/lib/insecureTokenEndpointNotice.ts | 3 +++ docs/test-servers.md | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/clients/web/src/lib/insecureTokenEndpointNotice.ts b/clients/web/src/lib/insecureTokenEndpointNotice.ts index 9f5df42d4d..932b094591 100644 --- a/clients/web/src/lib/insecureTokenEndpointNotice.ts +++ b/clients/web/src/lib/insecureTokenEndpointNotice.ts @@ -17,6 +17,9 @@ * `autoClose: false` matches the other non-recoverable OAuth notices (issuer * mismatch, unconfigured enterprise IdP): nothing the user does next will make * this reappear, so a toast that vanishes takes the only explanation with it. + * It stops the notice **expiring**, not the user dismissing it — Mantine's close + * control still works, which is correct for a message someone has finished + * reading. Don't describe this as non-dismissible. */ import { notifications } from "@mantine/notifications"; diff --git a/docs/test-servers.md b/docs/test-servers.md index aba13f4683..82a2bccd57 100644 --- a/docs/test-servers.md +++ b/docs/test-servers.md @@ -465,7 +465,7 @@ The trailing dot is the whole trick, and it is doing real work rather than being Add the server, click **Connect**, and complete the authorization. The redirect comes back with a code, the Inspector goes to exchange it, and the SDK refuses. -What you should see is a red, non-dismissing **"Token endpoint is not secure"** notification naming the endpoint and the two things that resolve it — serve it over HTTPS, or move it to one of the three host spellings the SDK exempts (`localhost`, `127.0.0.1`, `::1`). There is deliberately **no** action button. +What you should see is a red, non-expiring **"Token endpoint is not secure"** notification naming the endpoint and the two things that resolve it — serve it over HTTPS, or move it to one of the three host spellings the SDK exempts (`localhost`, `127.0.0.1`, `::1`). It stays until you close it (`autoClose: false` stops it expiring on a timer; Mantine's own close control still dismisses it, which is what you want for a message you have finished reading). There is deliberately **no** action button. Note the second option is phrased as a *spelling* change, not a networking one. `localhost.` already **is** loopback, and so is `tenant.app.localhost`; what they are outside is a three-literal allow-list. Telling a reader to "use a loopback host" when they demonstrably already are is what sends them off to debug their resolver instead of their configuration. From 3874ae49ce34224850f448d8e46edae3f2b2e4c4 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 02:53:53 -0400 Subject: [PATCH 12/19] fix: do not unmap IPv4-mapped IPv6 in explicit ALLOWED_ORIGINS (round 13) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit canonicalOriginHost does two things — drops a root FQDN dot, and unmaps an IPv4-mapped IPv6 host ([::ffff:7f00:1] -> 127.0.0.1). The second is right for a bind host, which is the address the socket answers on, and wrong for an operator's explicit origin, which is already the exact string the browser sends. So ALLOWED_ORIGINS=http://[::ffff:127.0.0.1]:6274 was rewritten to http://127.0.0.1:6274: the origin the operator asked for was blocked and a different one authorized in its place. Introduced by the round-7 fix that started normalizing explicit entries. parsed.hostname is already WHATWG-normalized (lowercased, punycoded, IPv6 bracketed), so the trailing dot is the only thing left to remove, and it is now removed by hand rather than by calling the helper. The comment says why, since the two look interchangeable. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- clients/web/server/web-server-config.ts | 14 +++++++++++++- .../integration/server/web-server-config.test.ts | 16 ++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/clients/web/server/web-server-config.ts b/clients/web/server/web-server-config.ts index fded53f2b2..d6acbb93cb 100644 --- a/clients/web/server/web-server-config.ts +++ b/clients/web/server/web-server-config.ts @@ -537,10 +537,22 @@ export function buildWebServerConfig( // root-dotted host is not a valid CSP host-source, so such an entry // could never admit an MCP Apps embedder anyway. // + // ⚠️ The root dot is dropped by hand rather than by calling + // `canonicalOriginHost`, and the difference is load-bearing. That + // helper also unmaps an IPv4-mapped IPv6 host (`[::ffff:7f00:1]` → + // `127.0.0.1`), which is right for a *bind host* — that is the address + // the socket answers on — and wrong for an operator's explicit origin, + // which is already exactly the string the browser will send. Running it + // here would allow-list `http://127.0.0.1:6274` while the browser asks + // as `http://[::ffff:7f00:1]:6274`: the requested origin blocked, and a + // different one authorized in its place. `parsed.hostname` is already + // WHATWG-normalized (lowercased, punycoded, IPv6 bracketed), so the + // trailing dot is the only thing left to remove. + // // Rebuilt from `parsed` rather than string-edited: `parsed.port` is // empty for a scheme-default port, which is what keeps the `:80` drop // that `URL.origin` already performed. - const host = canonicalOriginHost(parsed.hostname); + const host = parsed.hostname.replace(/\.$/, ""); return parsed.port ? `${parsed.protocol}//${host}:${parsed.port}` : `${parsed.protocol}//${host}`; 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 1c0574bb81..09a9af15ab 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 @@ -216,6 +216,22 @@ describe("buildWebServerConfigFromEnv", () => { ]); }); + it("does NOT unmap an IPv4-mapped IPv6 entry, which would authorize a different origin", () => { + // `canonicalOriginHost` unmaps `[::ffff:7f00:1]` to `127.0.0.1`, which is + // right for a bind host — that is the address the socket answers on — and + // wrong here. An explicit entry is already exactly the string the browser + // sends, so unmapping it would block the requested origin and authorize a + // different one in its place. Only the root dot may be removed. + process.env.ALLOWED_ORIGINS = + "http://[::ffff:127.0.0.1]:6274, http://Example.COM:80"; + expect(buildWebServerConfigFromEnv().allowedOrigins).toEqual([ + // The WHATWG serialization, which is what a browser puts in `Origin`. + "http://[::ffff:7f00:1]:6274", + // Still canonicalized in every other respect: lowercased, `:80` dropped. + "http://example.com", + ]); + }); + it("turns the *.localhost widening off when ALLOWED_ORIGINS is set but wholly invalid", () => { // A widening fails closed. The operator stated an allow-list and got it // wrong; the list itself still falls back to the default (documented, From d5bfb4cb8e5876849e90e9814eab56de9e3e2262 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 03:15:33 -0400 Subject: [PATCH 13/19] fix: use canonicalOriginHost for the CLI handoff link (round 15) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CLI's --print-handoff deep link still built its host with canonicalUrlHost, so HOST=localhost. printed http://localhost.:PORT while the web server's default allow-list — derived through canonicalOriginHost since round 6 — contains only http://localhost:PORT. The page loads and its autoConnect POST is then 403'd on the mismatched Origin: the link appears to work and the connection silently does not. The comment directly above that code already claimed the invariant ("use the canonical host so it matches the allow-list"); this restores it. Swept the remaining canonicalUrlHost call sites rather than waiting to be told of a third. Only resolve-bind-host.ts is left, and it is correct: it reports a resolved BIND address in an error message, where keeping the root dot is the point. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- clients/cli/__tests__/stored-auth.test.ts | 22 ++++++++++++++++++++++ clients/cli/src/cli.ts | 9 +++++++-- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/clients/cli/__tests__/stored-auth.test.ts b/clients/cli/__tests__/stored-auth.test.ts index bea1ad4994..416598754e 100644 --- a/clients/cli/__tests__/stored-auth.test.ts +++ b/clients/cli/__tests__/stored-auth.test.ts @@ -694,6 +694,28 @@ describe("--print-handoff", () => { expect(out.deepLink.startsWith("http://127.0.0.1:16274/?")).toBe(true); }); + it("drops a root FQDN dot from the deep-link host", async () => { + // `HOST=localhost.` binds loopback, but the web server's default allow-list + // is derived through `canonicalOriginHost` and so contains + // `http://localhost:PORT`. A dotted link would load the page and then have + // its autoConnect POST 403'd on the mismatched `Origin` — the page works, + // the connection silently does not (#2280 review round 15). + const result = await runCli( + ["--print-handoff", "--server-url", "https://x.example/mcp"], + { + env: { + MCP_INSPECTOR_API_TOKEN: "tok123", + HOST: "localhost.", + CLIENT_PORT: "16274", + }, + }, + ); + expectCliSuccess(result); + const out = JSON.parse(result.stdout) as { deepLink: string }; + expect(out.deepLink.startsWith("http://localhost:16274/?")).toBe(true); + expect(out.deepLink).not.toContain("localhost.:"); + }); + 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. diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index 17aa5ba088..07aa83a4b6 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -28,7 +28,7 @@ import type { JsonValue } from "@inspector/core/mcp/index.js"; import type { StrictJsonValue } from "@inspector/core/json/jsonUtils.js"; import { isSerializableJson } from "@inspector/core/json/jsonUtils.js"; import { - canonicalUrlHost, + canonicalOriginHost, isAllInterfacesHost, } from "@inspector/core/node/hostUrl.js"; import { getStateFilePath } from "@inspector/core/auth/node/storage-node.js"; @@ -512,9 +512,14 @@ function buildHandoff( // 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. + // `canonicalOriginHost`, not `canonicalUrlHost`: the web server's default + // allow-list is derived through the former, so a root-dotted `HOST=localhost.` + // would otherwise produce a link whose page loads while its auto-connect API + // request carries the dotted `Origin` and is 403'd — the invariant this + // comment claims, quietly broken. const linkHost = isAllInterfacesHost(host) ? "localhost" - : canonicalUrlHost(host); + : canonicalOriginHost(host); const clientPort = process.env.CLIENT_PORT || "6274"; const sandboxPort = process.env.MCP_SANDBOX_PORT || "6275"; // The dedicated app origin (#2056). Forwarded alongside the other two: an App From 66994478743659f031170f714949b5dc304bd29d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 03:30:33 -0400 Subject: [PATCH 14/19] fix: scope the root-dot normalization, and reject strictPort without a port (round 16) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - canonicalOriginHost dropped a root FQDN dot from EVERY hostname. A root dot is the absolute form of a name rather than noise: `service.example.` and `service.example` are different hosts, and the second may be completed through a resolver search suffix or fail outright — so the helper could silently advertise a different server than the one bound. It is now scoped to `localhost` / `*.localhost`, where the suffix is reserved to loopback either way and no such ambiguity exists, which is also the only case the normalization was added for. Every other host keeps its dot and behaves as it did before the helper existed. - strictPort was silently ignored when the port was omitted or 0: it fell through to an OS-assigned port, so a misconfigured fixture would look strict while relocating on every run — precisely the failure the flag exists to prevent, made invisible. That combination now throws. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- .../web/src/test/core/node/hostUrl.test.ts | 44 +++++++++++++++++++ .../test/integration/mcp/strict-port.test.ts | 18 ++++++++ core/node/hostUrl.ts | 29 +++++++++--- test-servers/src/test-server-http.ts | 12 +++++ 4 files changed, 96 insertions(+), 7 deletions(-) diff --git a/clients/web/src/test/core/node/hostUrl.test.ts b/clients/web/src/test/core/node/hostUrl.test.ts index 19efb58f5b..fe4fe1ce40 100644 --- a/clients/web/src/test/core/node/hostUrl.test.ts +++ b/clients/web/src/test/core/node/hostUrl.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect } from "vitest"; import { + canonicalOriginHost, canonicalUrlHost, formatHostForUrl, isAllInterfacesHost, @@ -239,3 +240,46 @@ describe("isLocalhostSubdomainOrigin", () => { expect(isLocalhostSubdomainOrigin(origin)).toBe(false); }); }); + +describe("canonicalOriginHost", () => { + it.each([ + ["localhost.", "localhost"], + ["LOCALHOST.", "localhost"], + ["app.localhost.", "app.localhost"], + ["tenant.example.localhost.", "tenant.example.localhost"], + ])( + "drops the root dot inside the loopback family: %j -> %j", + (host, want) => { + expect(canonicalOriginHost(host)).toBe(want); + }, + ); + + it.each([ + // A root dot is the ABSOLUTE form of a name, not noise. Dropping it changes + // which host is meant: `service.example` is a distinct origin a resolver + // may complete through a search suffix, or fail on. Advertising it for + // `service.example.` would silently hand the user a different server. + "service.example.", + "example.com.", + "inspector.internal.", + ])("keeps the root dot on an absolute non-loopback name: %j", (host) => { + expect(canonicalOriginHost(host)).toBe(host); + }); + + it.each([ + // Everything else behaves exactly as `canonicalUrlHost` does. + "127.0.0.1", + "localhost", + "0.0.0.0", + "example.com", + "[::1]", + ])("is canonicalUrlHost for %j", (host) => { + expect(canonicalOriginHost(host)).toBe(canonicalUrlHost(host)); + }); + + it("still unmaps an IPv4-mapped IPv6 bind host", () => { + // That divergence belongs to bind hosts and is inherited deliberately; the + // explicit-allow-list path deliberately does NOT use this helper for it. + expect(canonicalOriginHost("[::ffff:127.0.0.1]")).toBe("127.0.0.1"); + }); +}); diff --git a/clients/web/src/test/integration/mcp/strict-port.test.ts b/clients/web/src/test/integration/mcp/strict-port.test.ts index 90e41b0fbe..03b8041e01 100644 --- a/clients/web/src/test/integration/mcp/strict-port.test.ts +++ b/clients/web/src/test/integration/mcp/strict-port.test.ts @@ -87,6 +87,24 @@ describe("strictPort (#2280)", () => { // dead server for the rest of the worker. }); + it.each([undefined, 0])( + "refuses to start with strictPort and port %j", + async (port) => { + // Nothing to be strict about. Falling through to an OS-assigned port + // would let a misconfigured fixture look strict while relocating every + // run — the failure the flag exists to prevent, now silent. + server = createTestServerHttp({ + serverInfo: createTestServerInfo("misconfigured", "1.0.0"), + serverType: "streamable-http", + port, + strictPort: true, + }); + await expect(server.start()).rejects.toThrow( + /strictPort requires an explicit non-zero port/, + ); + }, + ); + it("is carried from the fixture's config file to the resolved server config", async () => { // The plumbing half: a flag the loader drops would leave the fixture // relocating again with nothing to show for it. diff --git a/core/node/hostUrl.ts b/core/node/hostUrl.ts index 7820270f75..f8b8af6df2 100644 --- a/core/node/hostUrl.ts +++ b/core/node/hostUrl.ts @@ -185,12 +185,13 @@ export function isLoopbackHost(host: string): boolean { * The canonical host to put in a **browser-facing** origin or advertised URL. * * {@link canonicalUrlHost} plus one thing it deliberately does not do: drop a - * root FQDN dot. `HOST=localhost.` binds loopback and every resolver treats it - * as `localhost`, but the WHATWG serializer keeps the dot — and a root-dotted - * host is **not a valid CSP `host-source`** (the grammar admits no empty final - * label). Anything we advertise is eventually named in a `frame-ancestors` - * directive, so emitting one produces a URL that works for `/api/*` and then - * blanks an MCP Apps frame, with nothing in between to explain it. + * root FQDN dot **within the loopback family**. `HOST=localhost.` binds + * loopback and every resolver treats it as `localhost`, but the WHATWG + * serializer keeps the dot — and a root-dotted host is **not a valid CSP + * `host-source`** (the grammar admits no empty final label). Anything we + * advertise is eventually named in a `frame-ancestors` directive, so emitting + * one produces a URL that works for `/api/*` and then blanks an MCP Apps frame, + * with nothing in between to explain it. * * Every advertised URL goes through here — the banner, the origin allow-list, * the sandbox proxy URL and the app-origin URL — which is what keeps them @@ -203,7 +204,21 @@ export function isLoopbackHost(host: string): boolean { * `app.localhost.` for this same CSP reason. The two sit at different layers. */ export function canonicalOriginHost(host: string): string { - return canonicalUrlHost(host).replace(/\.$/, ""); + const canonical = canonicalUrlHost(host); + // ⚠️ Scoped to the loopback family ON PURPOSE. A root dot is not noise in + // general — it is the absolute form of a name, and dropping it changes which + // host is meant: `service.example.` is the fully-qualified name, while + // `service.example` is a distinct browser origin that a resolver may complete + // through a search suffix or fail outright. Advertising the second for the + // first would silently hand the user a different server. + // + // Inside `localhost` / `*.localhost` there is no such ambiguity — the suffix + // is reserved to loopback either way — and this is the only case the + // normalization exists for: keeping the advertised URL expressible as a CSP + // `host-source`, which a root-dotted host is not. Everything else keeps its + // dot and behaves exactly as it did before this helper existed. + const bare = canonical.replace(/\.$/, ""); + return bare === "localhost" || bare.endsWith(".localhost") ? bare : canonical; } /** diff --git a/test-servers/src/test-server-http.ts b/test-servers/src/test-server-http.ts index 66bb004633..5e394bf8c0 100644 --- a/test-servers/src/test-server-http.ts +++ b/test-servers/src/test-server-http.ts @@ -400,6 +400,18 @@ export class TestServerHttp { const serverType = this.config.serverType ?? "streamable-http"; const requestedPort = this.config.port; + // `strictPort` means "bind exactly this port, or fail". With no fixed port + // to bind there is nothing to be strict about, and falling through to an + // OS-assigned one would let a misconfigured fixture look strict while + // relocating on every run — the exact failure the flag exists to prevent, + // now silent. Reject the combination instead. + if (this.config.strictPort && !requestedPort) { + throw new Error( + "strictPort requires an explicit non-zero port: there is nothing to " + + "bind strictly when the port is omitted or 0 (OS-assigned).", + ); + } + // If a port is explicitly requested, find an available port starting from that value // Otherwise, use 0 to let the OS assign an available port. // `strictPort` opts out of the walk: bind the requested port or fail loudly From 010cda1a145faf9854190ff29eb31cdc4844d52b Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 03:46:15 -0400 Subject: [PATCH 15/19] fix: one rule for the root dot, and validate strictPort's type (round 17) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both findings came from the review's Suppressed comments block. - The explicit ALLOWED_ORIGINS parser still stripped a root dot from every entry — the same mistake round 16 fixed in canonicalOriginHost, in the second place I had hand-rolled it (round 13, which could not call that helper because it must not inherit the IPv4-mapped unmapping). So ALLOWED_ORIGINS=https://service.example. allow-listed https://service.example, a different origin than the operator named. Rather than patch the same rule a second time, "when may a root dot be dropped" is now one exported function, stripLoopbackRootDot, used by both callers. Fixing it twice by hand is what produced this. - strictPort was accepted from a config file without a type check. It is consumed as a plain truthiness check at bind time, so `strictPort: "false"` read as ENABLED and silently disabled the port walk — the opposite of what the author wrote. Validated at load, where the file is being trusted. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- clients/web/server/web-server-config.ts | 11 ++++-- .../test/integration/mcp/strict-port.test.ts | 29 ++++++++++++++ .../server/web-server-config.test.ts | 13 +++++++ core/node/hostUrl.ts | 39 ++++++++++++------- test-servers/src/load-config.ts | 13 +++++++ 5 files changed, 87 insertions(+), 18 deletions(-) diff --git a/clients/web/server/web-server-config.ts b/clients/web/server/web-server-config.ts index d6acbb93cb..5d4ff917a0 100644 --- a/clients/web/server/web-server-config.ts +++ b/clients/web/server/web-server-config.ts @@ -23,6 +23,7 @@ import { resolveAppOriginPort } from "./app-origin-controller.js"; import { resolveBindHostname } from "./resolve-bind-host.js"; import { canonicalOriginHost, + stripLoopbackRootDot, isAllInterfacesHost, } from "../../../core/node/hostUrl.ts"; @@ -537,8 +538,12 @@ export function buildWebServerConfig( // root-dotted host is not a valid CSP host-source, so such an entry // could never admit an MCP Apps embedder anyway. // - // ⚠️ The root dot is dropped by hand rather than by calling - // `canonicalOriginHost`, and the difference is load-bearing. That + // ⚠️ Only the *loopback* root dot is dropped, and it is dropped via + // `stripLoopbackRootDot` rather than `canonicalOriginHost`. Both halves + // of that matter. A root dot elsewhere is the absolute form of a name — + // `https://service.example.` and `https://service.example` are + // different origins — so removing it would authorize a host the + // operator did not name. And That // helper also unmaps an IPv4-mapped IPv6 host (`[::ffff:7f00:1]` → // `127.0.0.1`), which is right for a *bind host* — that is the address // the socket answers on — and wrong for an operator's explicit origin, @@ -552,7 +557,7 @@ export function buildWebServerConfig( // Rebuilt from `parsed` rather than string-edited: `parsed.port` is // empty for a scheme-default port, which is what keeps the `:80` drop // that `URL.origin` already performed. - const host = parsed.hostname.replace(/\.$/, ""); + const host = stripLoopbackRootDot(parsed.hostname); return parsed.port ? `${parsed.protocol}//${host}:${parsed.port}` : `${parsed.protocol}//${host}`; diff --git a/clients/web/src/test/integration/mcp/strict-port.test.ts b/clients/web/src/test/integration/mcp/strict-port.test.ts index 03b8041e01..25753a786b 100644 --- a/clients/web/src/test/integration/mcp/strict-port.test.ts +++ b/clients/web/src/test/integration/mcp/strict-port.test.ts @@ -1,5 +1,7 @@ import { describe, it, expect, afterEach } from "vitest"; import { createServer, type Server } from "node:http"; +import { writeFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; import { createTestServerHttp, type TestServerHttp, @@ -105,6 +107,33 @@ describe("strictPort (#2280)", () => { }, ); + it.each(["false", "true", 1, null])( + "rejects a non-boolean strictPort in a config file: %j", + (value) => { + // Consumed as a plain truthiness check at bind time, so the string + // "false" would read as *enabled* and silently disable the port walk — + // the opposite of what the author wrote. + const file = path.join( + tmpdir(), + `strict-port-${Date.now()}-${Math.random()}.json`, + ); + writeFileSync( + file, + JSON.stringify({ + serverInfo: { name: "x", version: "1.0.0" }, + transport: { type: "streamable-http", port: 8099, strictPort: value }, + }), + ); + try { + expect(() => loadConfig(file)).toThrow( + /transport.strictPort must be a boolean/, + ); + } finally { + rmSync(file, { force: true }); + } + }, + ); + it("is carried from the fixture's config file to the resolved server config", async () => { // The plumbing half: a flag the loader drops would leave the fixture // relocating again with nothing to show for it. 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 09a9af15ab..922497f20f 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 @@ -216,6 +216,19 @@ describe("buildWebServerConfigFromEnv", () => { ]); }); + it("keeps the root dot on an absolute non-loopback explicit entry", () => { + // Same rule as `canonicalOriginHost`, second location: a root dot outside + // the loopback family is the ABSOLUTE form of a name, so dropping it here + // would allow-list `https://service.example` — a different origin — for an + // operator who wrote `https://service.example.`. + process.env.ALLOWED_ORIGINS = + "https://service.example., http://localhost.:6274"; + expect(buildWebServerConfigFromEnv().allowedOrigins).toEqual([ + "https://service.example.", + "http://localhost:6274", + ]); + }); + it("does NOT unmap an IPv4-mapped IPv6 entry, which would authorize a different origin", () => { // `canonicalOriginHost` unmaps `[::ffff:7f00:1]` to `127.0.0.1`, which is // right for a bind host — that is the address the socket answers on — and diff --git a/core/node/hostUrl.ts b/core/node/hostUrl.ts index f8b8af6df2..6acfa7a69b 100644 --- a/core/node/hostUrl.ts +++ b/core/node/hostUrl.ts @@ -203,22 +203,31 @@ export function isLoopbackHost(host: string): boolean { * {@link isLocalhostSubdomainHost} needs to preserve the dot so it can reject * `app.localhost.` for this same CSP reason. The two sit at different layers. */ +/** + * Drop a root FQDN dot, but **only** inside the loopback family. + * + * A root dot is the *absolute* form of a name, not noise: `service.example.` + * and `service.example` are different hosts, and the second may be completed + * through a resolver search suffix or fail outright — so removing it in general + * silently changes which server is meant. Inside `localhost` / `*.localhost` + * there is no such ambiguity, the suffix being reserved to loopback either way, + * and that is the only place the removal is needed: a root-dotted host is not a + * valid CSP `host-source`, so it cannot appear in an MCP Apps + * `frame-ancestors`. + * + * Expects an already-canonical host (lowercased, punycoded) — `URL.hostname` or + * {@link canonicalUrlHost} output. Shared by {@link canonicalOriginHost} and by + * the explicit `ALLOWED_ORIGINS` parser, which cannot use that helper because it + * must not inherit its IPv4-mapped unmapping; one definition of the rule keeps + * the two from drifting. + */ +export function stripLoopbackRootDot(host: string): string { + const bare = host.replace(/\.$/, ""); + return bare === "localhost" || bare.endsWith(".localhost") ? bare : host; +} + export function canonicalOriginHost(host: string): string { - const canonical = canonicalUrlHost(host); - // ⚠️ Scoped to the loopback family ON PURPOSE. A root dot is not noise in - // general — it is the absolute form of a name, and dropping it changes which - // host is meant: `service.example.` is the fully-qualified name, while - // `service.example` is a distinct browser origin that a resolver may complete - // through a search suffix or fail outright. Advertising the second for the - // first would silently hand the user a different server. - // - // Inside `localhost` / `*.localhost` there is no such ambiguity — the suffix - // is reserved to loopback either way — and this is the only case the - // normalization exists for: keeping the advertised URL expressible as a CSP - // `host-source`, which a root-dotted host is not. Everything else keeps its - // dot and behaves exactly as it did before this helper existed. - const bare = canonical.replace(/\.$/, ""); - return bare === "localhost" || bare.endsWith(".localhost") ? bare : canonical; + return stripLoopbackRootDot(canonicalUrlHost(host)); } /** diff --git a/test-servers/src/load-config.ts b/test-servers/src/load-config.ts index 30fe40df05..3fa9d56bfe 100644 --- a/test-servers/src/load-config.ts +++ b/test-servers/src/load-config.ts @@ -198,6 +198,19 @@ function validateConfig( `Invalid config in ${filePath}: transport.type must be stdio, streamable-http, or sse`, ); } + // `strictPort` is consumed as a plain truthiness check at bind time, so a + // string `"false"` would read as *enabled* and silently disable the port walk + // — the opposite of what the author wrote. Validate the type here, where the + // file is being trusted, rather than letting it through as a `ConfigFile`. + if ( + transport.strictPort !== undefined && + typeof transport.strictPort !== "boolean" + ) { + throw new Error( + `Invalid config in ${filePath}: transport.strictPort must be a boolean`, + ); + } + // Only reject *enabling* modern on a non-HTTP transport; a falsy `modern` // (e.g. `false`) is a no-op that `resolveConfig` normalizes away. if (transport.modern && transportType !== "streamable-http") { From 88726840268726ce4a0b4c3d695dd65a2dd6d983 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 04:01:34 -0400 Subject: [PATCH 16/19] docs: reattach the canonicalOriginHost JSDoc, and tidy a stale comment (round 18) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 18 was the first non-blocking round (approval recommended); both comments were documentation. - My round-17 edit inserted stripLoopbackRootDot between canonicalOriginHost's JSDoc and its declaration, so the contract was detached from the export and editors would show the new helper without it. Moved back. - A stray capitalized "That" after a conjunction. Fixing it surfaced that the surrounding comment had gone stale from repeated patching — it still said the trailing dot was "the only thing left to remove" after round 16 scoped that to the loopback family. Rewritten as the two distinct constraints it is really describing. Also added the CLI handoff to the JSDoc's list of advertised-URL consumers, which round 15 introduced. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- clients/web/server/web-server-config.ts | 34 ++++++++++--------- core/node/hostUrl.ts | 44 ++++++++++++------------- 2 files changed, 41 insertions(+), 37 deletions(-) diff --git a/clients/web/server/web-server-config.ts b/clients/web/server/web-server-config.ts index 5d4ff917a0..0c898dec45 100644 --- a/clients/web/server/web-server-config.ts +++ b/clients/web/server/web-server-config.ts @@ -538,21 +538,25 @@ export function buildWebServerConfig( // root-dotted host is not a valid CSP host-source, so such an entry // could never admit an MCP Apps embedder anyway. // - // ⚠️ Only the *loopback* root dot is dropped, and it is dropped via - // `stripLoopbackRootDot` rather than `canonicalOriginHost`. Both halves - // of that matter. A root dot elsewhere is the absolute form of a name — - // `https://service.example.` and `https://service.example` are - // different origins — so removing it would authorize a host the - // operator did not name. And That - // helper also unmaps an IPv4-mapped IPv6 host (`[::ffff:7f00:1]` → - // `127.0.0.1`), which is right for a *bind host* — that is the address - // the socket answers on — and wrong for an operator's explicit origin, - // which is already exactly the string the browser will send. Running it - // here would allow-list `http://127.0.0.1:6274` while the browser asks - // as `http://[::ffff:7f00:1]:6274`: the requested origin blocked, and a - // different one authorized in its place. `parsed.hostname` is already - // WHATWG-normalized (lowercased, punycoded, IPv6 bracketed), so the - // trailing dot is the only thing left to remove. + // ⚠️ Two constraints, and both are load-bearing. + // + // Only the *loopback* root dot may go. Elsewhere a root dot is the + // absolute form of a name — `https://service.example.` and + // `https://service.example` are different origins — so removing it + // would authorize a host the operator never named. + // + // And it must be `stripLoopbackRootDot`, not `canonicalOriginHost`. + // That helper also unmaps an IPv4-mapped IPv6 host + // (`[::ffff:7f00:1]` → `127.0.0.1`), which is right for a *bind host* — + // that is the address the socket answers on — and wrong for an explicit + // origin, which is already the exact string the browser will send. + // Running it here would allow-list `http://127.0.0.1:6274` while the + // browser asks as `http://[::ffff:7f00:1]:6274`: the requested origin + // blocked, a different one authorized in its place. + // + // `parsed.hostname` is already WHATWG-normalized (lowercased, + // punycoded, IPv6 bracketed), so the root dot is the only thing this + // path ever changes. // // Rebuilt from `parsed` rather than string-edited: `parsed.port` is // empty for a scheme-default port, which is what keeps the `:80` drop diff --git a/core/node/hostUrl.ts b/core/node/hostUrl.ts index 6acfa7a69b..3db09a7da1 100644 --- a/core/node/hostUrl.ts +++ b/core/node/hostUrl.ts @@ -181,28 +181,6 @@ export function isLoopbackHost(host: string): boolean { ); } -/** - * The canonical host to put in a **browser-facing** origin or advertised URL. - * - * {@link canonicalUrlHost} plus one thing it deliberately does not do: drop a - * root FQDN dot **within the loopback family**. `HOST=localhost.` binds - * loopback and every resolver treats it as `localhost`, but the WHATWG - * serializer keeps the dot — and a root-dotted host is **not a valid CSP - * `host-source`** (the grammar admits no empty final label). Anything we - * advertise is eventually named in a `frame-ancestors` directive, so emitting - * one produces a URL that works for `/api/*` and then blanks an MCP Apps frame, - * with nothing in between to explain it. - * - * Every advertised URL goes through here — the banner, the origin allow-list, - * the sandbox proxy URL and the app-origin URL — which is what keeps them - * agreeing. The **bind host is never touched**: callers still `listen()` on - * exactly what was configured, and this only changes the spelling handed to a - * browser. - * - * Deliberately NOT folded into {@link canonicalUrlHost}, which - * {@link isLocalhostSubdomainHost} needs to preserve the dot so it can reject - * `app.localhost.` for this same CSP reason. The two sit at different layers. - */ /** * Drop a root FQDN dot, but **only** inside the loopback family. * @@ -226,6 +204,28 @@ export function stripLoopbackRootDot(host: string): string { return bare === "localhost" || bare.endsWith(".localhost") ? bare : host; } +/** + * The canonical host to put in a **browser-facing** origin or advertised URL. + * + * {@link canonicalUrlHost} plus one thing it deliberately does not do: drop a + * root FQDN dot **within the loopback family**. `HOST=localhost.` binds + * loopback and every resolver treats it as `localhost`, but the WHATWG + * serializer keeps the dot — and a root-dotted host is **not a valid CSP + * `host-source`** (the grammar admits no empty final label). Anything we + * advertise is eventually named in a `frame-ancestors` directive, so emitting + * one produces a URL that works for `/api/*` and then blanks an MCP Apps frame, + * with nothing in between to explain it. + * + * Every advertised URL goes through here — the banner, the origin allow-list, + * the sandbox proxy URL, the app-origin URL and the CLI `--print-handoff` deep + * link — which is what keeps them agreeing. The **bind host is never touched**: callers still `listen()` on + * exactly what was configured, and this only changes the spelling handed to a + * browser. + * + * Deliberately NOT folded into {@link canonicalUrlHost}, which + * {@link isLocalhostSubdomainHost} needs to preserve the dot so it can reject + * `app.localhost.` for this same CSP reason. The two sit at different layers. + */ export function canonicalOriginHost(host: string): string { return stripLoopbackRootDot(canonicalUrlHost(host)); } From a4329ba84f89e44dc04a69c587bccabc1a6f7f4a Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 04:16:13 -0400 Subject: [PATCH 17/19] fix: reject every strictPort config that cannot honor the flag (round 19) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit My round-17 guard checked `!requestedPort`, which a string "0" sails past as truthy — Node then coerces it to the dynamic port 0, so a fixture that declares strictPort relocates anyway. `strictPort` on stdio was accepted too, where there is no listener at all and resolveConfig drops the flag. Every one of those combinations fails SILENTLY, which is exactly the outcome this flag exists to prevent, so the loader now rejects them: strictPort requires an HTTP transport and an integer port in 1-65535. The bind-time guard is widened to match as defense in depth for a programmatic caller that bypasses loadConfig. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- .../test/integration/mcp/strict-port.test.ts | 82 ++++++++++++++++++- docs/test-servers.md | 2 +- test-servers/src/load-config.ts | 29 +++++++ test-servers/src/test-server-http.ts | 17 +++- 4 files changed, 123 insertions(+), 7 deletions(-) diff --git a/clients/web/src/test/integration/mcp/strict-port.test.ts b/clients/web/src/test/integration/mcp/strict-port.test.ts index 25753a786b..161603f493 100644 --- a/clients/web/src/test/integration/mcp/strict-port.test.ts +++ b/clients/web/src/test/integration/mcp/strict-port.test.ts @@ -101,9 +101,7 @@ describe("strictPort (#2280)", () => { port, strictPort: true, }); - await expect(server.start()).rejects.toThrow( - /strictPort requires an explicit non-zero port/, - ); + await expect(server.start()).rejects.toThrow(/integer in 1-65535/); }, ); @@ -134,6 +132,84 @@ describe("strictPort (#2280)", () => { }, ); + it.each([ + // Each of these is truthy or type-valid enough to pass a naive check, and + // each fails SILENTLY: the fixture looks strict and relocates anyway. + [ + { type: "streamable-http", port: "0", strictPort: true }, + /integer in 1-65535/, + ], + [ + { type: "streamable-http", port: "8091", strictPort: true }, + /integer in 1-65535/, + ], + [ + { type: "streamable-http", port: 0, strictPort: true }, + /integer in 1-65535/, + ], + [ + { type: "streamable-http", port: 8091.5, strictPort: true }, + /integer in 1-65535/, + ], + [ + { type: "streamable-http", port: 70000, strictPort: true }, + /integer in 1-65535/, + ], + [{ type: "streamable-http", strictPort: true }, /integer in 1-65535/], + // No listener at all, and `resolveConfig` drops the flag. + [{ type: "stdio", strictPort: true }, /requires an HTTP transport/], + ])("rejects the unhonorable strictPort config %j", (transport, message) => { + const file = path.join( + tmpdir(), + `strict-port-combo-${Date.now()}-${Math.random()}.json`, + ); + writeFileSync( + file, + JSON.stringify({ + serverInfo: { name: "x", version: "1.0.0" }, + transport, + }), + ); + try { + expect(() => loadConfig(file)).toThrow(message); + } finally { + rmSync(file, { force: true }); + } + }); + + it("still accepts the honorable combination", () => { + const file = path.join( + tmpdir(), + `strict-port-ok-${Date.now()}-${Math.random()}.json`, + ); + writeFileSync( + file, + JSON.stringify({ + serverInfo: { name: "x", version: "1.0.0" }, + transport: { type: "streamable-http", port: 8091, strictPort: true }, + }), + ); + try { + expect(resolveConfig(loadConfig(file)).strictPort).toBe(true); + } finally { + rmSync(file, { force: true }); + } + }); + + it("rejects a truthy-but-unbindable port at bind time too", async () => { + // Defense in depth for a programmatic caller that bypasses `loadConfig`. + // A string "0" is truthy, so a bare falsiness guard would pass it through + // and Node would coerce it to the dynamic port 0. + server = createTestServerHttp({ + serverInfo: createTestServerInfo("stringy", "1.0.0"), + serverType: "streamable-http", + port: "0" as unknown as number, + strictPort: true, + }); + await expect(server.start()).rejects.toThrow(/integer in 1-65535/); + server = null; + }); + it("is carried from the fixture's config file to the resolved server config", async () => { // The plumbing half: a flag the loader drops would leave the fixture // relocating again with nothing to show for it. diff --git a/docs/test-servers.md b/docs/test-servers.md index 82a2bccd57..f4c1dc04e1 100644 --- a/docs/test-servers.md +++ b/docs/test-servers.md @@ -459,7 +459,7 @@ The value now rides the normalized `AuthChallenge` as a string — it has to be `oauth-insecure-token-endpoint-http.json` is an ordinary combined AS + resource server with one thing changed: `oauth.issuerUrl` is `http://localhost.:8091`, so its advertised `token_endpoint` is `http://localhost.:8091/oauth/token`. Plain streamable-HTTP — connect with the **default (legacy)** protocol era. -⚠️ It sets `transport.strictPort`, so it **fails to start** if 8091 is taken rather than relocating. Every other fixture walks to the next free port on `EADDRINUSE`, which is right for them and wrong for this one: the issuer is a *fixed string* in the config, so a relocated server would announce 8092 while all its OAuth metadata still pointed at whatever unrelated process holds 8091 — and the fixture would quietly stop reproducing the refusal it exists for. A loud failure is the only honest option here. +⚠️ It sets `transport.strictPort`, so it **fails to start** if 8091 is taken rather than relocating. (`strictPort` is only honorable alongside an HTTP transport and an integer `port` in 1–65535, so `loadConfig` rejects every other combination outright — each of them would otherwise leave a fixture looking strict while relocating anyway.) Every other fixture walks to the next free port on `EADDRINUSE`, which is right for them and wrong for this one: the issuer is a *fixed string* in the config, so a relocated server would announce 8092 while all its OAuth metadata still pointed at whatever unrelated process holds 8091 — and the fixture would quietly stop reproducing the refusal it exists for. A loud failure is the only honest option here. The trailing dot is the whole trick, and it is doing real work rather than being a curiosity. `localhost.` is the *root-anchored* spelling of `localhost`: every resolver on the machine sends it to the loopback interface, so the fixture is reachable and the flow runs for real — but the SDK's `assertSecureTokenEndpoint` exempts only the three literals `localhost`, `127.0.0.1` and `::1`, and `localhost.` is none of them. So the credential-carrying request is refused with `InsecureTokenEndpointError` while everything else about the server works. It is the same over-narrow exemption that makes `http://tenant.app.localhost:3300` fail ([#1944](https://github.com/modelcontextprotocol/inspector/issues/1944), [typescript-sdk#2591](https://github.com/modelcontextprotocol/typescript-sdk/issues/2591)), reproducible without a `/etc/hosts` entry or dnsmasq. diff --git a/test-servers/src/load-config.ts b/test-servers/src/load-config.ts index 3fa9d56bfe..74e9fa5ad8 100644 --- a/test-servers/src/load-config.ts +++ b/test-servers/src/load-config.ts @@ -211,6 +211,35 @@ function validateConfig( ); } + // Beyond the type: reject every combination that cannot honor the flag's + // contract, because each of them fails *silently* — the fixture looks strict + // and relocates anyway, which is the failure the flag exists to prevent. + // + // - a string port (`"0"`, `"8091"`) is truthy, so it slips past the runtime + // "no port to be strict about" guard, and Node then coerces `"0"` to the + // dynamic port 0; + // - a non-integer or out-of-range port cannot be bound as written; + // - on `stdio` there is no listener at all, and `resolveConfig` drops the + // flag, so it silently does nothing. + if (transport.strictPort === true) { + if (transportType === "stdio") { + throw new Error( + `Invalid config in ${filePath}: transport.strictPort requires an HTTP transport (streamable-http or sse)`, + ); + } + const port = transport.port; + if ( + typeof port !== "number" || + !Number.isInteger(port) || + port < 1 || + port > 65535 + ) { + throw new Error( + `Invalid config in ${filePath}: transport.strictPort requires transport.port to be an integer in 1-65535 (got ${JSON.stringify(port)})`, + ); + } + } + // Only reject *enabling* modern on a non-HTTP transport; a falsy `modern` // (e.g. `false`) is a no-op that `resolveConfig` normalizes away. if (transport.modern && transportType !== "streamable-http") { diff --git a/test-servers/src/test-server-http.ts b/test-servers/src/test-server-http.ts index 5e394bf8c0..73ca7689dc 100644 --- a/test-servers/src/test-server-http.ts +++ b/test-servers/src/test-server-http.ts @@ -405,10 +405,21 @@ export class TestServerHttp { // OS-assigned one would let a misconfigured fixture look strict while // relocating on every run — the exact failure the flag exists to prevent, // now silent. Reject the combination instead. - if (this.config.strictPort && !requestedPort) { + if ( + this.config.strictPort && + (typeof requestedPort !== "number" || + !Number.isInteger(requestedPort) || + requestedPort < 1 || + requestedPort > 65535) + ) { + // `loadConfig` rejects these for a config file; this covers a + // programmatic caller, and specifically a value that is *truthy* but not + // bindable as written — a string `"0"` slips past a bare falsiness check + // and Node then coerces it to the dynamic port 0, so the fixture looks + // strict and relocates anyway. throw new Error( - "strictPort requires an explicit non-zero port: there is nothing to " + - "bind strictly when the port is omitted or 0 (OS-assigned).", + `strictPort requires an explicit port as an integer in 1-65535 (got ${JSON.stringify(requestedPort)}): ` + + "there is nothing to bind strictly otherwise.", ); } From 8086904f62627e111aa018c24e67b9d85221ea19 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 04:37:23 -0400 Subject: [PATCH 18/19] fix: scope every banner clear to its own server, and stop leaking the test control (round 20) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The terminal SEP-2207 arms cleared the re-auth banner unconditionally. These paths are asynchronous, so server A can reject long after the user switched away and server B raised a banner of its own; the stale A continuation then erased B's, which was still valid and still actionable. Every clear is now a functional update guarded on serverId, which meant widening the exported setter's type so consumers can pass an updater at all. - The strict-port test added in round 19 nulled its failed instance, skipping teardown and leaving the process-global test-server control pointing at a dead server. That is the same leak round 4 fixed on the EADDRINUSE case; the comment there now exists on both so it does not happen a third time. The coverage gate then caught something better than a threshold miss: the three new updaters were never *invoked*, because the harness's setReAuthBanner is a spy that ignores updater functions — so the guard was reaching the line without being tested. The tests now apply the captured updater and assert both directions on each arm: clears its own server's banner, spares another's. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- .../src/hooks/useConnectionLifecycle.test.tsx | 40 ++++++++++++++++++ .../web/src/hooks/useConnectionLifecycle.ts | 20 ++++++--- .../web/src/hooks/useOAuthRecovery.test.tsx | 38 +++++++++++++++++ clients/web/src/hooks/useOAuthRecovery.ts | 41 +++++++++++++++---- .../test/integration/mcp/strict-port.test.ts | 5 ++- 5 files changed, 131 insertions(+), 13 deletions(-) diff --git a/clients/web/src/hooks/useConnectionLifecycle.test.tsx b/clients/web/src/hooks/useConnectionLifecycle.test.tsx index 59175d6a24..a016d3a8a6 100644 --- a/clients/web/src/hooks/useConnectionLifecycle.test.tsx +++ b/clients/web/src/hooks/useConnectionLifecycle.test.tsx @@ -259,6 +259,27 @@ const lastClient = (h: Harness): InspectorClient => { return client; }; +/** + * The last updater handed to `setReAuthBanner`, applied to a banner. + * + * Every terminal SEP-2207 arm clears the banner with a **functional** update + * guarded on `serverId`, because these paths are asynchronous and a late + * continuation for one server must not erase a banner another raised in the + * meantime. The harness's setter is a spy, so the updater is never invoked for + * us — asserting it directly is what actually exercises the guard rather than + * merely reaching the line. + */ +const applyBannerUpdate = ( + spy: ReturnType, + banner: { serverId: string; message: string } | null, +) => { + const updater = spy.mock.calls.at(-1)?.[0] as unknown; + if (typeof updater !== "function") { + throw new Error("expected a functional setReAuthBanner update"); + } + return (updater as (prev: unknown) => unknown)(banner); +}; + const toastTitles = (): string[] => notificationsMock.show.mock.calls.map((c) => String(c[0]?.title)); @@ -623,6 +644,19 @@ describe("useConnectionLifecycle", () => { // is how the raw SDK text would creep back in beside the good copy. expect(toastTitles()).not.toContain('Failed to connect to "Server a"'); expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("a"); + // The clear is scoped: it drops this server's banner and spares another's. + expect( + applyBannerUpdate(h.spies.setReAuthBanner, { + serverId: "a", + message: "x", + }), + ).toBeNull(); + expect( + applyBannerUpdate(h.spies.setReAuthBanner, { + serverId: "other", + message: "x", + }), + ).toMatchObject({ serverId: "other" }); // The real `connect()` sets status `"error"` and dispatches // `statusChange` before rethrowing, which paints the card red and pins // the monitoring sidebar open — presenting this as the failed connect @@ -700,6 +734,12 @@ describe("useConnectionLifecycle", () => { expect(toastTitles()).toContain("Token endpoint is not secure"); expect(toastTitles()).not.toContain('Failed to connect to "Server a"'); expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("a"); + expect( + applyBannerUpdate(h.spies.setReAuthBanner, { + serverId: "other", + message: "x", + }), + ).toMatchObject({ serverId: "other" }); expect(h.api().connectErrorMessage).toBeUndefined(); // The teardown the generic arm does is still required on this one. expect(disconnectSpy).toHaveBeenCalled(); diff --git a/clients/web/src/hooks/useConnectionLifecycle.ts b/clients/web/src/hooks/useConnectionLifecycle.ts index 9d58cc8d5e..2fb94f19e1 100644 --- a/clients/web/src/hooks/useConnectionLifecycle.ts +++ b/clients/web/src/hooks/useConnectionLifecycle.ts @@ -34,6 +34,7 @@ import { showInsecureTokenEndpointNotice } from "../lib/insecureTokenEndpointNot import { findInsecureTokenEndpoint } from "@inspector/core/auth/insecureTokenEndpoint.js"; import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; import type { RemoteInspectorClientStorage } from "@inspector/core/mcp/remote/index.js"; +import type { Dispatch, SetStateAction } from "react"; import type { SessionRef } from "./useSessionRef"; import type { FetchLogOptions } from "./useInspectorStores"; import type { LastPersistedSettings } from "./useLastPersistedSettings"; @@ -174,7 +175,7 @@ export interface UseConnectionLifecycleOptions { prepareOAuthRedirect: (args: PrepareOAuthRedirectArgs) => void; finalizeExplicitDisconnect: () => void; reAuthBanner: ReAuthBannerState | null; - setReAuthBanner: (next: ReAuthBannerState | null) => void; + setReAuthBanner: Dispatch>; /** See `SessionResetSurface`. */ sessionReset: SessionResetSurface; @@ -656,8 +657,11 @@ export function useConnectionLifecycle({ showInsecureTokenEndpointNotice(err, target.name); // Clear a banner left by an earlier failure: its Re-authenticate // button is just as dead as the one this arm declines to offer, and - // the user cannot tell which failure it belongs to. - setReAuthBanner(null); + // the user cannot tell which failure it belongs to. Scoped to this + // server, so an async continuation cannot erase another's. + setReAuthBanner((prev) => + prev && prev.serverId === id ? null : prev, + ); return; } @@ -703,7 +707,11 @@ export function useConnectionLifecycle({ // above, which this arm needs for the same reason the generic one // does, and before the flag it must not set. if (showInsecureTokenEndpointNotice(recoveryErr, target.name)) { - setReAuthBanner(null); + // Only this server's banner — an async continuation must not + // erase one raised for a server the user has since switched to. + setReAuthBanner((prev) => + prev && prev.serverId === id ? null : prev, + ); return; } setFailedServerId(id); @@ -760,7 +768,9 @@ export function useConnectionLifecycle({ // See the SEP-2207 note on the handshake arm above (#2280). The // disconnect already happened at the top of this catch. if (showInsecureTokenEndpointNotice(authErr, target.name)) { - setReAuthBanner(null); + setReAuthBanner((prev) => + prev && prev.serverId === id ? null : prev, + ); return; } // The connect attempt failed, same as any other handshake error — diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index c50843f0dd..dde7cd00fd 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/clients/web/src/hooks/useOAuthRecovery.test.tsx @@ -1004,6 +1004,44 @@ describe("useOAuthRecovery", () => { expect(toastTitles()).toContain("Token endpoint is not secure"); }); + it("does not clear a banner belonging to a different server", async () => { + // The paths are asynchronous: server A can reject long after the user + // switched away and server B raised its own banner. An unconditional + // clear would erase B's, which is still valid and still actionable. + const client = fakeClient(); + const h = harness({ + servers: [entry("a"), entry("b")], + activeServerId: "b", + client, + }); + await act(async () => { + client.emit("oauthError", { error: new Error("session expired") }); + }); + await waitFor(() => expect(h.api().reAuthBanner?.serverId).toBe("b")); + + // The user switches to "a"; a stale continuation for it now rejects. + h.rerender({ + servers: [entry("a"), entry("b")], + activeServerId: "a", + client, + }); + await act(async () => { + await h + .api() + .runWithCommandAuthRecovery( + () => + Promise.reject( + new InsecureTokenEndpointError("http://localhost.:8091/token"), + ), + "tool", + ); + }); + + expect(toastTitles()).toContain("Token endpoint is not secure"); + // B's banner survives — it is still valid and still actionable. + expect(h.api().reAuthBanner?.serverId).toBe("b"); + }); + it("clears a stale banner when a command-path failure is terminal", async () => { // Every terminal arm goes through one wrapper for this reason: a banner // left by an earlier failure carries a Re-authenticate button just as diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index 4ee96a5f9c..1207c076f3 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -1,3 +1,4 @@ +import type { Dispatch, SetStateAction } from "react"; import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import type { RefObject } from "react"; import { notifications } from "@mantine/notifications"; @@ -211,7 +212,13 @@ export interface OAuthRecovery { onBeforeOAuthRedirect: (authorizationUrl: URL) => void; prepareOAuthRedirect: (args: PrepareOAuthRedirectArgs) => void; reAuthBanner: ReAuthBannerState | null; - setReAuthBanner: (next: ReAuthBannerState | null) => void; + /** + * The raw state setter, functional form included. Consumers need the updater + * to clear a banner **only when it belongs to the server they are reporting + * on** — these paths are asynchronous, so a late continuation for server A + * must not erase a banner server B raised in the meantime. + */ + setReAuthBanner: Dispatch>; /** * Drops the banner and both pending-OAuth slots. Called from the session * reset, which runs on every disconnect: an unanswered step-up prompt or a @@ -397,11 +404,23 @@ export function useOAuthRecovery({ * Returns whether the error was claimed, so callers keep their fall-through. */ const reportTerminalInsecureTokenEndpoint = useCallback( - (err: unknown, serverName?: string): boolean => { + ( + err: unknown, + serverId: string | undefined, + serverName?: string, + ): boolean => { if (!showInsecureTokenEndpointNotice(err, serverName)) { return false; } - setReAuthBanner(null); + // Clear only *this* server's banner. The command and deferred-resume + // paths are asynchronous, so server A can reject long after the user + // switched away and server B raised a banner of its own; an unconditional + // clear would then erase B's, which is still valid and still actionable. + // Functional so it sees the queued state rather than the render-time + // value, matching how `setPendingReauth` guards its own late restore. + setReAuthBanner((prev) => + prev && prev.serverId === serverId ? null : prev, + ); return true; }, [setReAuthBanner], @@ -419,7 +438,7 @@ export function useOAuthRecovery({ // way. Claimed here, at the single funnel every re-auth banner goes // through, rather than at each of its call sites — a new caller then gets // the right behavior by default instead of by remembering. - if (reportTerminalInsecureTokenEndpoint(detail, server?.name)) { + if (reportTerminalInsecureTokenEndpoint(detail, serverId, server?.name)) { return; } const message = reAuthBannerMessage({ @@ -857,7 +876,9 @@ export function useOAuthRecovery({ const server = sessionRef.current.servers.find( (s) => s.id === activeServerId, ); - if (reportTerminalInsecureTokenEndpoint(err, server?.name)) { + if ( + reportTerminalInsecureTokenEndpoint(err, activeServerId, server?.name) + ) { return undefined; } throw err; @@ -989,7 +1010,13 @@ export function useOAuthRecovery({ const failedServer = sessionRef.current.servers.find( (s) => s.id === pending.serverId, ); - if (reportTerminalInsecureTokenEndpoint(err, failedServer?.name)) { + if ( + reportTerminalInsecureTokenEndpoint( + err, + pending.serverId, + failedServer?.name, + ) + ) { return; } // The slot was cleared above only to keep a tab-visible event and a @@ -1392,7 +1419,7 @@ export function useOAuthRecovery({ // Above `setFailedServerId` for the same reason the EMA arm is: this is // a configuration error, not a failed attempt, so it should not flag // the card red or pull the monitoring sidebar open. - if (reportTerminalInsecureTokenEndpoint(err, server.name)) { + if (reportTerminalInsecureTokenEndpoint(err, server.id, server.name)) { return; } // The token exchange (or the re-handshake behind it) failed. Flag the diff --git a/clients/web/src/test/integration/mcp/strict-port.test.ts b/clients/web/src/test/integration/mcp/strict-port.test.ts index 161603f493..31c5f5368d 100644 --- a/clients/web/src/test/integration/mcp/strict-port.test.ts +++ b/clients/web/src/test/integration/mcp/strict-port.test.ts @@ -207,7 +207,10 @@ describe("strictPort (#2280)", () => { strictPort: true, }); await expect(server.start()).rejects.toThrow(/integer in 1-65535/); - server = null; + // Deliberately NOT nulled, for the same reason as the EADDRINUSE case + // above: `start()` installs the process-global test-server control before + // it validates, so dropping the reference would skip teardown and leave + // that global pointing at a dead server. }); it("is carried from the fixture's config file to the resolved server config", async () => { From 924085df66f915f7715b0e48a02b1fc1b08d78aa Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 05:01:33 -0400 Subject: [PATCH 19/19] fix: preserve nested origins, and admit a *.localhost bind host (round 22) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - URL.origin is not always protocol + hostname. A nested-origin URL resolves through its inner one — `blob:https://example.com/id` has origin `https://example.com` and an EMPTY hostname — so the round-13 rebuild emitted `blob://`. Being non-empty that entry survived, suppressed the derived default list, and would have 403'd every real browser origin. The rebuild now happens only when a loopback root dot actually has to be removed; otherwise the parsed origin is returned untouched, preserving the previous `new URL(o).origin` contract exactly. - allowLocalhostSubdomainOriginsFor excluded a bind host that is itself under *.localhost, so `HOST=inspector.localhost` allow-listed only its own exact origin and still 403'd a sibling alias like `tenant.inspector.localhost` — which resolves to the same loopback interface and reaches this very process. Excluding it was arbitrary: the whole feature rests on RFC 6761 treating that suffix as loopback, and a *.localhost bind that starts is loopback-serving on the same premise. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- clients/web/server/web-server-config.ts | 29 ++++++++++++++++--- .../server/web-server-config.test.ts | 24 +++++++++++++++ 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/clients/web/server/web-server-config.ts b/clients/web/server/web-server-config.ts index 0c898dec45..320f3788aa 100644 --- a/clients/web/server/web-server-config.ts +++ b/clients/web/server/web-server-config.ts @@ -23,6 +23,7 @@ import { resolveAppOriginPort } from "./app-origin-controller.js"; import { resolveBindHostname } from "./resolve-bind-host.js"; import { canonicalOriginHost, + isLocalhostSubdomainHost, stripLoopbackRootDot, isAllInterfacesHost, } from "../../../core/node/hostUrl.ts"; @@ -341,7 +342,16 @@ function loopbackOrigins(port: number): string[] { */ export function allowLocalhostSubdomainOriginsFor(hostname: string): boolean { const h = canonicalOriginHost(hostname); - return LOOPBACK_HOSTNAMES.has(h) || isAllInterfacesHost(h); + return ( + LOOPBACK_HOSTNAMES.has(h) || + // A `*.localhost` bind that starts is loopback-serving on the same RFC 6761 + // premise as the origins being admitted, so excluding it was arbitrary: + // `HOST=inspector.localhost` would allow-list only its own exact origin and + // still 403 a sibling alias like `tenant.inspector.localhost`, which + // resolves to the same loopback interface and reaches this very process. + isLocalhostSubdomainHost(h) || + isAllInterfacesHost(h) + ); } /** @@ -558,10 +568,21 @@ export function buildWebServerConfig( // punycoded, IPv6 bracketed), so the root dot is the only thing this // path ever changes. // - // Rebuilt from `parsed` rather than string-edited: `parsed.port` is - // empty for a scheme-default port, which is what keeps the `:80` drop - // that `URL.origin` already performed. + // ⚠️ Deviate from `parsedOrigin` ONLY when a dot actually has to go. + // `URL.origin` is not always `protocol + hostname`: a nested-origin URL + // resolves through its inner one (`blob:https://example.com/id` has + // origin `https://example.com` but an EMPTY hostname), so rebuilding + // unconditionally emitted `blob://` — a non-empty entry, which then + // suppressed the derived default and 403'd every real browser origin. + // Returning `parsedOrigin` untouched in the common case keeps the + // previous `new URL(o).origin` contract exactly. + // + // In the rebuild branch `parsed.port` is empty for a scheme-default + // port, which preserves the `:80` drop `URL.origin` already performed. const host = stripLoopbackRootDot(parsed.hostname); + if (host === parsed.hostname) { + return parsedOrigin; + } return parsed.port ? `${parsed.protocol}//${host}:${parsed.port}` : `${parsed.protocol}//${host}`; 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 922497f20f..ab9434198d 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 @@ -216,6 +216,19 @@ describe("buildWebServerConfigFromEnv", () => { ]); }); + it("preserves a nested-origin entry rather than rebuilding it", () => { + // `URL.origin` is not always protocol + hostname: `blob:` resolves through + // its inner origin and has an EMPTY hostname, so rebuilding unconditionally + // emitted `blob://` — non-empty, which suppressed the derived default and + // 403'd every real browser origin. + process.env.ALLOWED_ORIGINS = + "blob:https://example.com/id, https://a.example.com"; + expect(buildWebServerConfigFromEnv().allowedOrigins).toEqual([ + "https://example.com", + "https://a.example.com", + ]); + }); + it("keeps the root dot on an absolute non-loopback explicit entry", () => { // Same rule as `canonicalOriginHost`, second location: a root dot outside // the loopback family is the ABSOLUTE form of a name, so dropping it here @@ -534,6 +547,17 @@ describe("allowLocalhostSubdomainOriginsFor (#1944)", () => { expect(allowLocalhostSubdomainOriginsFor(host)).toBe(true); }); + it.each([ + // A `*.localhost` bind that starts is loopback-serving on the same RFC 6761 + // premise as the origins being admitted, so a sibling alias reaching the + // same process must not 403. + "inspector.localhost", + "tenant.inspector.localhost", + "app.localhost.", + ])("enables it for the *.localhost bind host %j", (host) => { + expect(allowLocalhostSubdomainOriginsFor(host)).toBe(true); + }); + it.each(["192.168.1.50", "127.0.0.2", "inspector.example.com", "10.0.0.1"])( "leaves it off for the specific non-loopback bind host %j", (host) => {