feat(web): enforce localhost-only binding; expand loopback origin allow-list - #1796
Conversation
…ow-list (#1795) Refuse binding an all-interfaces host (0.0.0.0 / :: / empty) by default — it exposes the process-spawning backend to the local network, the surface DNS-rebinding attacks target — unless DANGEROUSLY_BIND_ALL_INTERFACES=true is set. A new shared guard (server/resolve-bind-host.ts) is used by both bind points (web-server-config.ts and vite.config.ts). The Docker image, which must bind 0.0.0.0 to be reachable via -p, sets the opt-in flag. Also fixes the original bug: connecting to a stdio server 403'd with "Invalid origin ... DNS rebinding" because the dev server binds IPv6 loopback [::1] while the default allowedOrigins was the single literal "http://localhost:PORT". The default loopback origin list now expands to all three interchangeable forms (localhost / 127.0.0.1 / [::1]). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
…ED_ORIGINS guidance (#1795) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
@claude review |
|
Claude finished @cliffhall's task in 3m 51s —— View job Code review — PR #1796
Solid, well-scoped change: one shared helper for both bind points, an explicit 1. The
|
HOST |
Node listen() result |
|---|---|
0 |
bound 0.0.0.0 (wildcard) — not caught |
0x0.0.0.0 |
bound 0.0.0.0 — not caught |
::ffff:0.0.0.0 |
bound ::ffff:0.0.0.0 (v4-mapped wildcard) — not caught |
[::] |
ENOTFOUND (fails loudly — fine) |
0.0.0.0. |
ENOTFOUND (fine) |
HOST=0 is the one that matters in practice — it's a common shorthand in Node/PaaS tooling, and it silently defeats the guard the PR exists to add. Since the helper's job is fail-fast, consider normalizing instead of literal-matching: e.g. keep the set, then additionally treat the value as all-interfaces when net.isIP()/BigInt-parses to the unspecified address (or, minimally, add "0" and "::ffff:0.0.0.0"). Cheap hardening and easy to unit-test alongside the existing it.each.
3. The actionable message is printed as a raw stack trace (low)
buildWebServerConfig() is called at clients/web/server/run-web.ts:235, outside the try/catch at 253–268. So the throw propagates to the launcher's top-level handler (clients/launcher/src/index.ts:60):
Error running MCP Inspector: Error: Refusing to bind HOST="0.0.0.0": …
at resolveBindHostname (…)
…
The carefully-worded guidance ends up buried under a stack dump. Twenty lines up, run-web.ts:200-206 already has the house pattern (console.error("Error:", message); process.exit(1)) — wrapping the buildWebServerConfig call the same way makes the error read the way the PR body describes it. Same consideration for the dev path, where the throw surfaces as a Vite config-load error.
4. resolveBindHostname returns the untrimmed value (low)
isAllInterfacesHost trims for detection, but line 56 returns the raw host. So HOST=" 127.0.0.1 " passes the guard and then fails at listen() with ENOTFOUND — and HOST=" 0.0.0.0 " + opt-in returns a padded string that can't bind either. Returning the trimmed value makes the detection and the return value agree.
5. defaultAllowedOrigins non-loopback branch: unbracketed IPv6, uncased host (low)
return [`http://${hostname}:${port}`];HOST=fe80::1→http://fe80::1:6274, not a valid origin — never matches, silent 403.HOST=Example.com→http://Example.com:PORT; browsers send the host lowercased, so this never matches either (the loopback branch lowercases, this one doesn't).
Same bracket omission exists in printServerBanner (web-server-config.ts:167) and sandboxUrl (sandbox-controller.ts:135) — pre-existing, but ::1 is now an explicitly documented input via LOOPBACK_HOSTNAMES, so HOST=::1 yields a malformed banner URL and a sandboxUrl that new URL() rejects on the restart path. A small formatHostForUrl(host) helper used by all three would settle it.
6. The guard runs for vite build and for the vitest projects too (low)
vite.config.ts:161 calls resolveBindHostname() inside the config factory's server block, which is evaluated for every command — vite build, vitest, not just serve. With HOST=0.0.0.0 exported in a shell or CI env, npm run build / npm run test now fail at config load with a message about binding, which is confusing when nothing is being bound. The factory already receives command; gating on command === "serve" (or resolving lazily) keeps the guard where it applies. Not a problem for the Dockerfile as written (ENV lands in the runtime stage and sets the opt-in, so the on-demand ensureWebBuild build is fine).
7. ALLOWED_ORIGINS entries aren't trimmed (nit)
process.env.ALLOWED_ORIGINS?.split(",").filter(Boolean) — ALLOWED_ORIGINS="http://a:1, http://b:2" yields " http://b:2", which can never match an Origin header, producing a 403 with no clue why. The new README section actively instructs users to write comma-separated lists, so this gets more reachable. .map((s) => s.trim()).filter(Boolean).
8. Test nit
resolve-bind-host.test.ts:9 — it.each(["0.0.0.0", "::", "", " 0.0.0.0 ", "::"]) repeats "::"; presumably " :: " was intended (the whitespace variant is only covered for the IPv4 form).
9. Docs: "a resolvable hostname works directly" may not hold in --dev (verify)
The new Hosting on a network section says HOST=<hostname> works with no extra config. That's true for the prod backend, but the dev server also has Vite's own server.allowedHosts Host-header check, whose default accepts loopback + IP literals and blocks other hostnames unless listed — and vite.config.ts doesn't set it. Worth confirming against the pinned Vite version; if it applies, either add the caveat ("in --dev, set server.allowedHosts") or scope that bullet to IP addresses.
Not blocking, and correct where it counts: the origin expansion doesn't weaken the rebinding guard (an attacker-controlled Origin, rebound or not, still never matches a loopback origin), and the true/1-only parsing is the right call. Items 1 and 2 are the ones I'd fix before merge — 1 because it leaves the reported symptom half-fixed, 2 because a bypass makes the guard advisory.
Note: I reviewed by reading the code; clients/web/node_modules isn't installed in this checkout, so I did not run the test suite locally (the HOST=0 / [::] / ::ffff:0.0.0.0 bind behavior in item 2 was verified by running Node directly). CI on this PR was still in progress at review time.
· branch v2/enforce-loopback-bind
…SP/URLs (#1795) Address the @claude review of #1796: - sandbox CSP: add http://[::1]:* to frame-ancestors so the MCP Apps iframe isn't blocked when the browser is on the IPv6 loopback (the same case the origin fix targets). - resolve-bind-host: catch the wildcard bypasses HOST=0 / 0x0.0.0.0 / ::ffff:0.0.0.0 / dotted-all-zero (inet_aton spellings Node still binds as 0.0.0.0); trim the returned host so detection and the bind value agree. - add shared formatHostForUrl() (brackets IPv6) and use it in the origin allow-list, the startup banner, and the sandbox URL; lowercase a non-loopback origin host so it matches the browser's Origin. - run-web: wrap buildWebServerConfig so the guard's message prints as a clean "Error: …" line instead of a raw stack trace. - vite.config: only enforce the guard for `command === "serve"`, so an ambient HOST=0.0.0.0 doesn't fail `vite build` / vitest at config load. - ALLOWED_ORIGINS: trim entries so a spaced comma list still matches. - docs: note that `--dev` additionally enforces Vite's server.allowedHosts, so a named host needs configuring there (or use the prod server / a specific IP). - tests: cover all of the above. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Thanks — thorough review. All nine addressed in 1. Sandbox CSP excluded 2. 3. Raw stack trace instead of the actionable message (low) — Fixed. Wrapped the 4. Returned untrimmed value (low) — Fixed. 5. Unbracketed IPv6 / uncased host in the non-loopback origin, banner, sandboxUrl (low) — Fixed with the shared 6. Guard ran for 7. 8. Duplicate 9.
|
|
@claude review |
|
Claude finished @cliffhall's task in 3m 38s —— View job Code review — PR #1796 (re-review after
|
| # | Prior finding | Status |
|---|---|---|
| 1 | Sandbox CSP excluded [::1] |
✅ fixed — but untested (see A2) |
| 2 | HOST=0 bypass |
✅ fixed, thoroughly |
| 3 | Raw stack trace | ✅ fixed (prod path) |
| 4 | Untrimmed return | ✅ fixed (see A3 for the bracket twin) |
| 5 | Unbracketed IPv6 / uncased host | ✅ fixed via formatHostForUrl (two call sites remain — A6) |
| 6 | Guard runs for vite build / vitest |
|
| 7 | ALLOWED_ORIGINS trimming |
✅ fixed |
| 8 | Duplicate "::" in it.each |
✅ fixed |
| 9 | --dev + allowedHosts |
✅ verified & documented correctly |
New findings below, most significant first.
A1. The item-6 fix is cosmetic — vite build / vitest still throw at config load (medium-low)
clients/web/vite.config.ts:164-168 now gates server.host on command === "serve", but twenty lines up the plugins array is built unconditionally:
// vite.config.ts:117
honoMiddlewarePlugin(buildWebServerConfigFromEnv()),buildWebServerConfigFromEnv() → buildWebServerConfig() → resolveBindHostname() (web-server-config.ts:250). That argument is evaluated eagerly for every command, so with an ambient HOST=0.0.0.0 exported in the shell/CI, vite build and every vitest project still fail at config load with the bind message — exactly the scenario the new comment says "must not fail them." The server.host gate only removes the second throw site, not the first (and the first is hit earlier, at array construction).
The plugin already declares apply: "serve" (vite-hono-plugin.ts:48), so skipping it outright for non-serve commands is behavior-neutral and avoids the wasted config build (vite preview also runs with command === "serve", so dev and preview are unaffected):
plugins: [
react(),
...(command === "serve" ? [honoMiddlewarePlugin(buildWebServerConfigFromEnv())] : []),
browserExternalizedBuiltinGate(),
],A2. The CSP fix (item 1) has no test — the existing assertion passes either way (low-medium)
clients/web/src/test/integration/server/sandbox-controller.test.ts:88:
expect(csp).toContain("frame-ancestors http://127.0.0.1:*");That's a prefix match on the old value, so it's green with or without http://[::1]:*. The bug this PR set out to fix (IPv6 embedder → CSP-blocked sandbox iframe) is therefore unguarded: delete the new source and CI stays green. Assert the full directive, or add expect(csp).toContain("http://[::1]:*").
A3. resolveBindHostname returns a bracketed host that can't be bound (low)
Same class as the trim fix, one spelling over. isAllInterfacesHost strips brackets for detection (resolve-bind-host.ts:55-58), but line 108 returns the host as-given:
HOST=[::1]→ passes the guard →serve({ hostname: "[::1]" })/server.listen(…, "[::1]")→ENOTFOUND(Node wants the bare::1).HOST=[::]+ opt-in → returns"[::]"→ same failure.
Notably LOOPBACK_HOSTNAMES (web-server-config.ts:206) explicitly lists "[::1]", so the code treats a bracketed HOST as a legitimate input for origin purposes while the bind path can't consume it. Stripping the brackets in resolveBindHostname (before returning) makes detection, listen(), and the origin list all agree — formatHostForUrl re-adds them wherever a URL is built.
A4. Dead branch in isAllZeroIpv4 (nit)
resolve-bind-host.ts:47 — if (value === "") return false; is unreachable: the only caller short-circuits on "" via ALL_INTERFACES_LITERALS.has("") before the ||, and any input that normalizes to "" (" ", "[]") hits the same literal. Even if reached it's redundant — "".split(".") → [""] → NaN → false. It's an uncoverable branch under the ≥90 gate; drop it (or annotate it) rather than leave it looking load-bearing.
A5. let webConfig; is an implicit-any evolving let (nit)
run-web.ts:235. It typechecks (the catch ends in process.exit(1), which is never, so flow analysis narrows the reads), but AGENTS.md's no-any rule reads better with the declared type spelled out: let webConfig: WebServerConfig;. Also, the house pattern two blocks up is console.error("Error:", message) (run-web.ts:203) whereas the new code uses console.error(\Error: ${message}`)` — identical output modulo the separator, but worth matching.
A6. formatHostForUrl sweep is one-and-a-half sites short (nit / follow-up)
Two remaining places build http://${host}:${port} unbracketed:
clients/web/server/server.ts:134— theEADDRINUSEmessage. Cosmetic (message only), trivial to include.clients/cli/src/cli.ts:727—buildHandoff()'s deep-link URL, from the sameprocess.env.HOST.HOST=::1yields a malformed handoff link that the browser can't open. The CLI can't importclients/web/server/, so fixing it means the helper belongs incore/node/. Reasonable as a follow-up issue rather than scope creep here.
Verdict: A1 is the one I'd fix before merge — not because the failure is severe, but because the code now carries a comment asserting a property it doesn't have, which is the kind of thing that survives for years. A2 is cheap and makes the headline fix regression-proof. A3–A6 are polish. The security substance of the PR is sound: the guard's normalization closes the bypass class properly, and the loopback origin expansion still can't be satisfied by an attacker-controlled Origin.
Note: node_modules isn't installed in this checkout (root and clients/web both absent), so I did not run the test suite, validate, or a real vite build — A1 is established by reading the evaluation order (eager argument at vite.config.ts:117 → buildWebServerConfigFromEnv → resolveBindHostname), not by executing it.
· branch v2/enforce-loopback-bind
…, test CSP (#1795) Address the re-review of #1796: - A1: gate honoMiddlewarePlugin on isDevServer, not just its `apply`/the server.host ternary. Its config arg buildWebServerConfigFromEnv() (→ resolveBindHostname) was evaluated eagerly, so an ambient HOST=0.0.0.0 still threw at config load for `vite build` and vitest. Comment corrected. - A2: assert the full sandbox frame-ancestors directive (incl. http://[::1]:*) so the IPv6-loopback CSP fix is regression-guarded, not prefix-matched. - A3: resolveBindHostname de-brackets its return (HOST=[::1] → ::1) so detection, listen(), and the origin list agree; formatHostForUrl re-adds. - A4: drop the dead `value === ""` branch in isAllZeroIpv4. - A5: `let webConfig: WebServerConfig` + match console.error("Error:", msg). - A6: move formatHostForUrl to core/node/hostUrl.ts (shared) and apply it at the two remaining unbracketed sites — server.ts EADDRINUSE message and the CLI deep-link (cli.ts buildHandoff, which reads HOST). resolve-bind-host re-exports it; new core test covers it (100%). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Round-2 addressed in A1 — item-6 fix was cosmetic (medium-low) — Correct, good catch. The A2 — CSP fix untested (low-medium) — Fixed. The assertion now matches the full A3 — bracketed return can't bind (low) — Fixed. Added A4 — dead branch (nit) — Removed the unreachable A5 — evolving A6 — formatter sweep (nit/follow-up) — Handled here rather than deferred (this PR is meant to close the class). Moved Green locally: web |
|
@claude review |
|
Claude finished @cliffhall's task in 3m 58s —— View job Code review — PR #1796 (round 3, after
|
| # | Round-2 finding | Status |
|---|---|---|
| A1 | Plugin config arg evaluated eagerly | ✅ fixed via isDevServer, comment rewritten |
| A2 | CSP fix untested | ✅ fixed — full-directive assertion, now regression-proof |
| A3 | Bracketed return can't bind | ✅ fixed (stripIpv6Brackets; [::] also now hits the guard message) |
| A4 | Dead value === "" branch |
✅ removed |
| A5 | Evolving let + separator |
✅ let webConfig: WebServerConfig + console.error("Error:", message) |
| A6 | Formatter sweep | ✅ moved to core/node/hostUrl.ts, applied at server.ts + cli.ts (see C3 for two more) |
New findings, most significant first. Nothing here is a blocker; C1 is the one I'd still fix.
C1. The IPv6 half of the wildcard-bypass class is still open — HOST=::0 binds all interfaces (low-medium)
Round 2 normalized the IPv4 spellings properly (isAllZeroIpv4 + inet_aton parsing), but the IPv6 side is still a literal set (resolve-bind-host.ts:24-31), so the same near-miss class survives one family over. Verified by actually binding:
::0 -> {"address":"::","family":"IPv6"} // NOT flagged
0::0 -> {"address":"::","family":"IPv6"} // NOT flagged
::0.0.0.0 (WHATWG-normalizes to ::) -> wildcard // NOT flagged
0:0::0 -> wildcard // NOT flagged
0000:0000:0000:0000:0000:0000:0000:0000 -> wildcard // NOT flagged
isAllZeroIpv4("::0") → parseAddressPart("::0") → NaN → false, and "::0" isn't in the literal set — so HOST=::0 sails past a guard whose entire purpose is to refuse exactly that bind. It's the mirror image of the HOST=0 hole, and ::0 is a more plausible hand-typed value than 0:0:0:0:0:0:0:0 (which is in the set).
Cleanest fix is to canonicalize IPv6 through the WHATWG serializer before the set lookup — it compresses zero runs, so every spelling above collapses to ::, and ::ffff:0.0.0.0 → ::ffff:0:0, which the existing set already contains (both verified):
function canonicalizeIpv6(value: string): string {
if (!isIPv6(value)) return value; // node:net
return new URL(`http://[${value}]`).hostname.slice(1, -1);
}
// isAllInterfacesHost: ALL_INTERFACES_LITERALS.has(canonicalizeIpv6(normalized)) || isAllZeroIpv4(normalized)That also lets the literal set shrink to "" | "0.0.0.0" | "::" | "::ffff:0:0". Non-wildcard v6 hosts are unaffected (::1 → ::1, ::ffff:0 → ::ffff:0, both verified as not flagged).
C2. defaultAllowedOrigins can never match on port 80/443 — and a new test enshrines the broken form (low)
web-server-config.ts:233 always appends :${port}, but browsers omit the default port from Origin. The check is an exact string match (core/mcp/remote/node/server.ts:202,225 — allowedOrigins.includes(origin)), so:
CLIENT_PORT=80 HOST=192.168.1.50→ allow-listhttp://192.168.1.50:80, browser sendsOrigin: http://192.168.1.50→ 403, "Invalid origin … DNS rebinding" — the same confusing failure mode this PR set out to eliminate, in the deployment shape the new README section now actively documents.- The new test at
web-server-config.test.tsasserts exactly this:defaultAllowedOrigins("Example.COM", 80)→["http://example.com:80"], freezing the value that can't match.
Emitting both forms when the port is the scheme default (or just http://host for 80) fixes it and stays exact-match. Worth folding in since the loopback branch is already a multi-form list.
C3. Two pre-existing copies of formatHostForUrl in core/auth/node/ weren't swapped (low)
A6's premise was "this PR is meant to close the class," and the helper now lives in core/node/hostUrl.ts where core/auth can import it — but the two inline duplicates remain, byte-for-byte the same logic:
// core/auth/node/oauth-callback-server.ts:223-225
const needsBrackets = host.includes(":") && !host.startsWith("[");
const formattedHost = needsBrackets ? `[${host}]` : host;
// core/auth/node/runner-oauth-callback.ts:91-94 (same, on config.hostname)Swapping both for formatHostForUrl is a two-line change that makes the new module the single definition rather than a third copy.
C4. Sandbox frame-ancestors is loopback-only, so MCP Apps is CSP-blocked in the case this PR sanctions (low, pre-existing)
sandbox-controller.ts:65 is now frame-ancestors http://127.0.0.1:* http://localhost:* http://[::1]:*. Correct for the [::1] bug, but under the newly-documented network-hosting path (DANGEROUSLY_BIND_ALL_INTERFACES=true, or the Docker image, browsed from another machine at http://192.168.1.50:6274) the embedder origin matches none of the three and the browser blocks the sandbox iframe — MCP Apps silently doesn't render. createSandboxController only receives { port, host }, so the principled fix (derive the directive from config.allowedOrigins) is a small signature change; the cheap alternative is a caveat in the new Hosting on a network section noting the MCP Apps tab is loopback-only today. Either way, better acknowledged than discovered.
C5. Nits
isAllZeroIpv4'sparts.length > 4early return is never exercised (resolve-bind-host.ts:52). Ironic follow-on to removing the A4 dead branch — no test input has 5 parts."0.0.0.0.0"in the negativeit.eachcovers the branch and documents thatinet_atonrejects it.- Import path for
formatHostForUrlis inconsistent, and the re-export comment is inaccurate.server.ts:18imports fromcore/node/hostUrl.ts;web-server-config.ts:19andsandbox-controller.ts:10go through the re-export atresolve-bind-host.ts:12— which drags the bind-guard module (and its error copy) into the sandbox controller purely for a pass-through. The comment there says the re-export exists "so the two web bind points can keep importing the URL formatter from this module," but neither importer is a bind point (vite.config.tsimports onlyresolveBindHostname). Pick one direction — dropping the re-export and importingcore/node/hostUrl.tsat all three sites is the simpler shape. - Zone-index IPv6.
formatHostForUrl("fe80::1%eth0")→[fe80::1%eth0], whichnew URL()rejects (a zone id must be%25-encoded). Genuinely exotic for a bind host; noting it only so it isn't a surprise if it ever shows up.
Verdict: the security substance holds up — the guard's IPv4 normalization is thorough, the trim/de-bracket round trip makes detection and listen() agree, the origin expansion still can't be satisfied by an attacker-controlled Origin, and the CSP assertion is now a real regression guard. C1 is worth closing because a guard with a known one-character bypass reads as complete when it isn't; C2 is the kind of 403 that costs someone an afternoon.
Note: node_modules isn't installed in this checkout, so I did not run the suite, validate, smoke, or test:storybook. The bind behavior in C1 and the WHATWG normalization it suggests were verified by running Node directly. One thing worth confirming locally: the reported local run covered validate + test:coverage + build, but not npm run ci — and the A1 fix changes plugin resolution for every vitest project including the Storybook browser one (VITEST set ⇒ the Hono middleware no longer attaches, where it previously did via apply: "serve"). Stories are presentational so I'd expect no impact, but npm run ci (which includes test:storybook) is the mandated pre-push gate for exactly this reason.
· branch v2/enforce-loopback-bind
…in, dedupe formatter (#1795) Address the round-3 review of #1796: - C1: canonicalize IPv6 via the WHATWG serializer before the wildcard-set lookup, so HOST=::0 / 0::0 / ::0.0.0.0 / 0:0::0 / fully-expanded zeros are all refused (the IPv6 mirror of the HOST=0 hole). Literal set shrinks to "" | 0.0.0.0 | :: | ::ffff:0:0. - C2: defaultAllowedOrigins omits the port when it's the http default (80) — browsers drop :80 from Origin and the guard is an exact match, so :80 could never match. - C3: swap the two pre-existing formatHostForUrl copies in core/auth/node (oauth-callback-server, runner-oauth-callback) for the shared core/node one. - C4: derive the sandbox frame-ancestors from allowedOrigins (new sandboxFrameAncestors) so the MCP Apps iframe isn't CSP-blocked under network hosting; loopback-wildcard fallback when the list is empty. - C5: add the parts.length>4 negative case (0.0.0.0.0); drop the resolve-bind-host re-export and import formatHostForUrl from core/node directly at all sites. - tests for all of the above. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
…k-bind # Conflicts: # clients/cli/src/cli.ts
|
Round-3 addressed in C1 — IPv6 wildcard bypass (low-medium) — Fixed, and thanks for catching the mirror-image hole. Added C2 — port-80 origin can never match (low) — Fixed. C3 — two pre-existing copies in C4 — MCP Apps CSP-blocked under network hosting (low) — Did the principled fix rather than the caveat. New C5 nits — All done: added the On your closing note about |
|
@claude review |
|
Claude finished @cliffhall's task in 4m 51s —— View job Code review — PR #1796 (round 4, after
|
| # | Round-3 finding | Status |
|---|---|---|
| C1 | IPv6 wildcard bypass | ✅ fixed via canonicalizeIpv6; set shrank to 4 entries (see E1 for the zone-index remainder) |
| C2 | Port-80 origin can never match | ✅ fixed via httpOrigin, both branches; frozen test corrected |
| C3 | Two duplicate formatters in core/auth/node/ |
✅ both swapped; core/node/hostUrl.ts is the single definition |
| C4 | MCP Apps CSP-blocked off loopback | ✅ principled fix — sandboxFrameAncestors(allowedOrigins), threaded from both callers, with a loopback fallback + tests (see E3 for what remains) |
| C5 | Nits (parts.length > 4 case, re-export) |
✅ all done; the resolve-bind-host re-export is gone, all three sites import core/node/hostUrl.ts |
Findings below, most significant first.
E1. A zone index makes the guard throw TypeError: Invalid URL — and HOST=::%eth0 is the last wildcard spelling (medium-low)
canonicalizeIpv6 (resolve-bind-host.ts:33-36) guards on net.isIPv6 and then hands the value to new URL(). Those two disagree about zone indices, which I verified directly:
net.isIPv6("fe80::1%eth0") -> true // Node accepts the zone id
net.isIPv6("::%eth0") -> true
new URL("http://[fe80::1%eth0]") -> TypeError: Invalid URL // WHATWG wants %25
new URL("http://[::%eth0]") -> TypeError: Invalid URL
So the if (!isIPv6(value)) return value early-out does not protect the new URL() call, and any zone-scoped HOST throws a bare TypeError: Invalid URL out of resolveBindHostname. Two consequences:
- A legitimate link-local bind is now broken.
HOST=fe80::1%eth0is the canonical use of a zone id. Before this PR it passed straight through tolisten(); now it dies at config load withError: Invalid URL(via the newrun-web.tshandler) — no mention of hosts or binding, and the value is perfectly valid (listen(0, "fe80::1%eth0")givesEADDRNOTAVAILhere only because this box has noeth0). HOST=::%eth0really is a wildcard bind — verified:server.listen(0, "::%eth0")→{"address":"::","family":"IPv6"}. It currently fails closed, but with the wrong error, so the guard's whole purpose (an actionable message) is lost on the one input class that's both a real wildcard and crashes.
Stripping the zone before canonicalizing fixes both at once and needs no try/catch — it's strictly better for detection, because the zone is irrelevant to which address it is:
function canonicalizeIpv6(value: string): string {
if (!isIPv6(value)) return value;
const [address] = value.split("%"); // net.isIPv6 accepts a zone id; new URL() rejects it
return new URL(`http://[${address}]`).hostname.slice(1, -1);
}::%eth0 → :: → flagged (correct); fe80::1%eth0 → fe80::1 → not flagged, and resolveBindHostname still returns the value with its zone intact so listen() can consume it. That also retires the round-3 C5 zone-index note as a real bug rather than an exotic formatting quirk (formatHostForUrl("fe80::1%eth0") → [fe80::1%eth0], which new URL() rejects — reachable via sandbox-controller.ts:100 on the restart path).
E2. User-controlled ALLOWED_ORIGINS now flows unvalidated into a response header (low)
C4's fix is right, but it turns ALLOWED_ORIGINS into CSP header content (sandbox-controller.ts:84 → res.writeHead). Two failure modes that didn't exist when the directive was a constant:
- A newline kills the sandbox page. I confirmed
res.writeHead(200, {"Content-Security-Policy": "frame-ancestors http://a:1\nX-Evil: 1"})throwsERR_INVALID_CHAR, the handler rejects, and the socket is destroyed (UND_ERR_SOCKETclient-side) — so the MCP Apps iframe fails to load with no diagnostic pointing at the env var. Reachable from a multi-line-e ALLOWED_ORIGINS=…or a shell heredoc. - A
;injects extra CSP directives into the proxy page (ALLOWED_ORIGINS="http://a:1; sandbox"), quietly changing the defense-in-depth header the long comment above it is careful about.
Both are self-inflicted, so this is hardening rather than a vulnerability — but the fix is one line in sandboxFrameAncestors: drop entries that aren't a bare scheme+host(+port) source (e.g. /^[a-z][a-z0-9+.-]*:\/\/[^\s;,'"]+$/i), so a malformed allow-list degrades to the loopback fallback instead of corrupting the header.
E3. C4 unblocks the CSP, but MCP Apps still can't work off loopback — the sandbox URL is built from the bind host (low)
The CSP was one of two blockers. The other: sandboxUrl is http://${formatHostForUrl(host)}:${actualPort}/sandbox where host is config.sandboxHost = the bind host (web-server-config.ts:296), and that string is handed to the browser via GET /api/config (server.ts:61, consumed at App.tsx:735). Under the two paths this PR newly documents:
- Docker /
HOST=0.0.0.0→sandboxUrl=http://0.0.0.0:<dynamic>/sandbox, which is not a reachable address from the client machine — and the dynamic sandbox port isn'tEXPOSEd (Dockerfileexposes only6274, andMCP_SANDBOX_PORTis unset ⇒ port0). HOST=192.168.1.50→ the URL is right, but the sandbox port is still dynamic and separate, so it must be reachable independently.
Nothing here regresses (it was equally broken before), but the new Hosting on a network section now reads as though network hosting is fully supported. Either add one caveat sentence there (MCP Apps needs the sandbox port reachable and a browser-reachable sandbox host — set MCP_SANDBOX_PORT and expose it), or file it as a follow-up. Given C4 was fixed principled-not-caveated, a follow-up issue is the consistent choice.
E4. Nits
- Banner vs. origin list disagree on port 80.
httpOrigindrops:80butprintServerBanner(web-server-config.ts:168) always prints it, soCLIENT_PORT=80advertiseshttp://localhost:80while the allow-list holdshttp://localhost. Harmless (browsers normalize the request), just an inconsistency now that one side is port-aware. resolve-bind-host.ts:57—parts.length > 4is guarded, butparts.length < 4isn't. Deliberate and correct (inet_atonbinds0.0as the wildcard, so flagging it is right), but the doc comment lists only 4-part and bare-integer spellings; worth naming0.0/0.0.0so a future reader doesn't "tighten" it to=== 4.- Web README wording. "refuses an all-interfaces host (
0.0.0.0,::, or empty)" now undersells the guard — it also refuses every legacy/canonical spelling. A trailing "…or any spelling of them (0,0x0,::0,::ffff:0.0.0.0, …)" makes the coverage discoverable to someone deciding whether to trust it.
Verdict: the security substance is in good shape. The wildcard cover is now closed on both families, hostUrl.ts is a single definition consumed by all five call sites including the CLI deep-link, the origin expansion still can't be satisfied by an attacker-controlled Origin, and the CSP assertion is a real regression guard. E1 is the only one I'd fix before merge — not for the wildcard (that fails closed) but because the guard now crashes with an unrelated error on a valid host spelling it previously accepted, which is a regression the tests can't see. E2–E4 are polish, and E3 is a fair follow-up issue.
Note: node_modules isn't installed in this checkout, so I did not run the suite, validate, coverage, or a build. Everything asserted about Node behavior in E1 and E2 (net.isIPv6 vs. new URL on zone ids, the real listen("::%eth0") → :: bind, writeHead ERR_INVALID_CHAR) was verified by running Node directly. CI run 30220104780 was still in progress at review time.
· branch v2/enforce-loopback-bind
… docs (#1795) Address the round-4 review of #1796: - E1: strip the IPv6 zone index (%eth0) before canonicalizing. net.isIPv6 accepts a zone id but new URL() rejects it (even %25-encoded), so a zone-scoped HOST previously threw an opaque TypeError out of the guard — breaking a legitimate link-local bind (fe80::1%eth0) and losing the actionable message on ::%eth0 (a real wildcard). Now ::%eth0 → :: (flagged), fe80::1%eth0 → fe80::1 (not flagged) and the zone is kept on the returned value for listen(). - E2: filter allowedOrigins to well-formed scheme://host[:port] sources before interpolating them into the sandbox CSP header, so a newline (ERR_INVALID_CHAR → dead sandbox page) or ';' (injected CSP directives) in ALLOWED_ORIGINS can't corrupt the header; falls back to loopback when nothing valid remains. - E4a: printServerBanner uses httpOrigin too, so banner and origin list agree on the default-port (:80) form. - E4b/E4c/E3: doc clarifications — isAllZeroIpv4's >4 guard rationale + short inet_aton forms; README notes the wildcard covers every spelling, and an MCP Apps caveat (sandbox runs on a separate port that must be reachable off loopback). - tests for the zone-index and CSP-filtering paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Round-4 addressed in E1 — zone index crashes the guard (medium-low) — Fixed, and thanks — this was a real regression (a valid host spelling that previously passed now threw). I verified your Node findings: E2 — E3 — sandbox URL is built from the bind host (low) — Since C4 was fixed principled-not-caveated, I kept it consistent by... adding the caveat here rather than a follow-up (the review process for this PR isn't creating follow-ups). The new Hosting on a network section now notes the MCP Apps sandbox runs on a separate E4 nits — Done: Green locally: web |
|
@claude review |
|
Claude finished @cliffhall's task in 5m 18s —— View job Code review — PR #1796 (round 5, after
|
| # | Round-4 finding | Status |
|---|---|---|
| E1 | Zone index crashed the guard | ✅ fixed — zone stripped for detection, kept on the returned value; +/− it.each cases and a resolveBindHostname zone test |
| E2 | ALLOWED_ORIGINS → CSP header unvalidated |
✅ fixed — CSP_HOST_SOURCE filter + fallback, both paths tested |
| E3 | Sandbox URL built from the bind host | ✅ documented (MCP Apps caveat in the web README) |
| E4 | Nits (banner/origin port-80 agreement, > 4 doc, README spellings) |
✅ all three |
The bind guard itself I now consider closed: ALL_INTERFACES_LITERALS + canonicalizeIpv6 + isAllZeroIpv4 is a complete cover of the wildcard family across both address families and the legacy inet_aton spellings, detection/listen()/origin-list all consume the same trimmed, de-bracketed, zone-preserving value, and it's well tested. The remaining findings are all on the origin allow-list / CSP side.
F1. The 0.0.0.0 default allow-list can never match — so the Docker image (this PR's one sanctioned exception) 403s every connect (medium)
defaultAllowedOrigins returns a single exact origin for a non-loopback host, so under the opt-in path it yields http://0.0.0.0:6274. The origin check is an exact string match (core/mcp/remote/node/server.ts:202,225), and a browser never sends 0.0.0.0 as its Origin — it sends whatever it navigated to (http://localhost:6274 for docker run -p 6274:6274). Result, in the shipped image with default env:
GET /and the SPA load fine (same-origin GETs carry noOrigin).- The first POST to
/api/*(every connect, andcreateRemoteFetch/remoteClientTransportgenerally) carriesOrigin: http://localhost:6274→ 403 "Invalid origin … DNS rebinding" — the exact failure mode this PR set out to eliminate, one config over.
The PR knows this: clients/web/README.md:174 says that under the wildcard you must also set ALLOWED_ORIGINS. But the Dockerfile — the place this PR grants the opt-in — sets HOST, DANGEROUSLY_BIND_ALL_INTERFACES, CLIENT_PORT, MCP_AUTO_OPEN_ENABLED and not ALLOWED_ORIGINS, and the root README's Docker section doesn't mention it either. So the documented requirement and the shipped artifact disagree.
The pre-existing part is the value (main also returned [baseUrl]); what's new is (a) the PR now documents the requirement without satisfying it at the one place it applies, (b) web-server-config.test.ts now asserts the unusable value (expect(cfg.allowedOrigins).toEqual(["http://0.0.0.0:8123"])), freezing it, and (c) since round 3, the sandbox CSP is derived from this same list — so frame-ancestors becomes http://0.0.0.0:6274, where it used to be the loopback wildcard family that did admit a http://localhost:6274 embedder. That last one is a small regression from the C4 fix, in the same configuration.
The principled fix is in defaultAllowedOrigins: an all-interfaces bind does serve loopback, so the loopback origins are legitimately reachable and adding them weakens nothing (still exact-match, still loopback-or-the-bound-host only) — e.g. return the loopback trio plus http://<host>:port when isAllInterfacesHost(hostname). That fixes the connect 403, the derived CSP, and the Docker default in one place. (Belt-and-braces: ENV ALLOWED_ORIGINS=… in the Dockerfile for the -p case.)
F2. Worth verifying in a browser: a bracketed IPv6 host-source isn't in the CSP grammar (low-medium, verify)
The http://[::1]:* entry added in round 1 (and the http://[::1]:6274 the derived list now emits) rests on browsers accepting a bracketed IPv6 literal as a CSP host-source. The CSP3 grammar doesn't cover it:
host-source = [ scheme-part "://" ] host-part [ port-part ] [ path-part ]
host-part = "*" / [ "*." ] 1*host-char *( "." 1*host-char )
host-char = ALPHA / DIGIT / "-"
[, :, ] aren't host-char, and browsers are required to ignore an invalid source expression (with a console warning). Chromium's CSP parser does appear to special-case brackets for IPv6, but I can't confirm that here (no browser in this checkout), and I don't know Firefox/WebKit's behavior. If a browser drops the source, the headline symptom — IPv6-loopback embedder ⇒ sandbox iframe CSP-blocked — is still present despite the fix, and nothing in the suite would show it (the tests assert the header string, not the browser's parse). Cheapest confirmation: load the app at http://[::1]:6274 with an MCP App open and check for a CSP console warning / blocked frame. If it doesn't hold, the note belongs in the code comment next to LOOPBACK_FRAME_ANCESTORS so the entry isn't mistaken for coverage it doesn't provide.
F3. E1 fixed the guard, but the URL builders downstream are still zone-unaware (low)
Now that a zone-scoped HOST legitimately passes the guard and is returned with its zone, that value flows into formatHostForUrl, which brackets anything containing ::
sandbox-controller.ts:175→sandboxUrl = "http://[fe80::1%eth0]:PORT/sandbox", andstart()'s already-started branch (:116) doesnew URL(sandboxUrl)→TypeError(verified: throws even as%25eth0). Also handed to the browser via/api/config→App.tsx:735.printServerBanner→ the same string, thenopen(url).defaultAllowedOrigins→ an origin that can never match.
Practically unreachable (a zone-scoped bind host is exotic, and start() is called once per controller), and strictly better than the pre-E1 crash — but "the guard accepts it" now implies "the rest of the pipeline handles it," and it doesn't. Either normalize in formatHostForUrl (drop the zone — a URL host can't carry one at all) or say so in its doc comment, so the boundary is explicit rather than latent.
F4. Nits
- The CSP integration test only exercises the fallback.
sandbox-controller.test.ts:127creates the controller withoutallowedOrigins, so it assertsframe-ancestors http://127.0.0.1:* http://localhost:* http://[::1]:*— but both real callers (server.ts:43,vite-hono-plugin.ts:65) always pass the list, so the shipped default directive is the exact-port trio (http://localhost:6274 …), which no end-to-end test covers.sandboxFrameAncestorsis unit-tested, so this is coverage shape rather than a gap; worth one controller case constructed with a list so the header the product actually serves is guarded. formatHostForUrltreats any:as IPv6. A mistypedHOST=localhost:6274becomes[localhost:6274]in the banner/origin/sandbox URL rather than failing legibly. Anet.isIP-based check would be exact now that the helper is shared by five call sites including the CLI deep-link.resolveBindHostnamepreserves case (HOST=LOCALHOST→"LOCALHOST"), which is fine only because every consumer lowercases (defaultAllowedOrigins) or is case-insensitive (listen). Since the function already normalizes trim + brackets, folding case in would make "the returned value is the normalized one" true without exception.clients/cli/src/cli.ts:727reads rawprocess.env.HOSTfor the handoff deep-link, so a wildcard/zonedHOSTstill yields an unusable link. The formatter swap fixed the bracketing half; the value half is out of scope here.
Verdict: the bind guard is done — closed cover, good normalization, real tests, and the actionable message now actually reaches the user. F1 is the one I'd fix before merge, because it's the same defect class as the bug in the PR title (a legitimate browser origin that the allow-list can't match), it now has a test freezing the broken value, and it lands on the exact path the PR sanctions and documents. F2 is a five-minute browser check that decides whether the headline [::1] fix works at all. F3/F4 are polish.
Note: node_modules isn't installed in this checkout (root and clients/web both absent), so I did not run the suite, validate, coverage, or a build. The Node behavior asserted above (net.isIPv6 vs. new URL on zone ids, including the %25-encoded form) was verified by running Node directly; F1's 403 is established by reading the exact-match middleware (core/mcp/remote/node/server.ts:202,225) against the derived list, plus the Origin-on-POST behavior the PR body itself documents.
· branch v2/enforce-loopback-bind
… builder (#1795) Address the round-5 review of #1796: - F1: defaultAllowedOrigins returns the loopback trio for an all-interfaces bind (0.0.0.0 / ::) — a wildcard bind serves loopback, so `docker run -p` browsed at http://localhost:PORT now connects out of the box instead of 403ing against the unmatchable http://0.0.0.0:PORT. README/Docker bullet reconciled (non-loopback access still needs ALLOWED_ORIGINS). - F2: VERIFIED in headless Chromium that a bracketed IPv6 literal is NOT a valid CSP host-source — `frame-ancestors http://[::1]:*` degrades to 'none' and blocks the frame. So the round-1 [::1] CSP entry never worked: dropped it from the loopback fallback and from the derived directive (CSP_HOST_SOURCE now rejects brackets), and documented that MCP Apps requires browsing at a name/IPv4, not a bare [::1]. - F3/F4b: formatHostForUrl brackets only real IPv6 (net.isIPv6, not "has a colon") and drops the zone id (a URL host can't carry one), so a mistyped host:port and a zone-scoped bind host no longer produce invalid URLs. - F4a: integration test asserts the CSP the product actually serves (controller constructed WITH allowedOrigins), not just the fallback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Round-5 addressed in F1 — Docker/wildcard allow-list can never match (medium) — Fixed. F2 — bracketed IPv6 as a CSP host-source (verify) — Verified in headless Chromium, and your grammar reading is right: it does not work. I served
F3 + F4b — zone-unaware / colon-based URL building (low) — Fixed together. F4a — CSP integration test only hit the fallback (nit) — Added a controller case constructed with F4c / F4d — deferred with reason. F4c (folding case in Green locally: web |
|
@claude review |
|
Claude finished @cliffhall's task in 4m 13s —— View job Code review — PR #1796 (round 6, after
|
| # | Round-5 finding | Status |
|---|---|---|
| F1 | Wildcard allow-list unmatchable | ✅ loopback trio for a wildcard bind (but see H2 — the omitted entry isn't dead weight) |
| F2 | Bracketed IPv6 as a CSP host-source | ✅ verified empirically, fix inverted accordingly, documented in code + README |
| F3/F4b | Zone-unaware / colon-based URL building | ✅ formatHostForUrl now keys off net.isIPv6 and strips the zone |
| F4a | CSP test only hit the fallback | ✅ derived-directive controller case added |
| F4c/F4d | Case folding, CLI raw HOST |
✅ deferred with stated reasons — both reasonable |
The bind guard remains closed and well-tested. The two findings below are both on the origin allow-list.
H1. ALLOWED_ORIGINS that parses to an empty list silently disables the origin guard (medium)
web-server-config.ts:307-310:
allowedOrigins:
process.env.ALLOWED_ORIGINS?.split(",").map((o) => o.trim()).filter(Boolean) ??
defaultAllowedOrigins(hostname, port),?? only fires on undefined, and [] is not undefined — so ALLOWED_ORIGINS="", " ", or "," yields []. The middleware treats that as allow everything (core/mcp/remote/node/server.ts:192-196):
// If no allowedOrigins configured, skip validation (allow all)
if (!allowedOrigins || allowedOrigins.length === 0) { await next(); return; }So -e ALLOWED_ORIGINS= (a bare -e with no value, or an empty line in a compose .env) turns off the DNS-rebinding protection this PR exists to strengthen, with no log line saying so. The "" spelling is pre-existing, but the new .map(trim) widens it: ALLOWED_ORIGINS=" " previously produced [" "] — a non-empty list that matched nothing, i.e. fail closed — and now produces [], i.e. fail open. Small surface, wrong direction.
Worth noting the two consumers of the same field now disagree about what empty means: sandboxFrameAncestors treats it as "no information, be strict" and falls back to loopback; the origin middleware treats it as "allow all". The fix keeps them consistent — fall back to the default when nothing survives parsing:
const configured = process.env.ALLOWED_ORIGINS?.split(",").map((o) => o.trim()).filter(Boolean);
allowedOrigins: configured?.length ? configured : defaultAllowedOrigins(hostname, port),H2. My round-5 premise was wrong: http://0.0.0.0:PORT is a real browser origin — and the banner advertises it (medium-low)
web-server-config.ts:228-232 now states, on my say-so, that "the bind host itself (0.0.0.0) is never sent as an Origin, so a http://0.0.0.0:PORT entry would only ever be dead weight." That's not right, and it matters here. 0.0.0.0 is locally connectable — I verified a client connect to 0.0.0.0 on a listening port returns 200 — and browsers on Linux/macOS navigate to http://0.0.0.0:6274 fine, sending that exact string as Origin on the connect POST.
Which is a problem, because the Docker banner prints that URL: printServerBanner builds it from config.hostname, so the container logs read
MCP Inspector Web is up and running at:
http://0.0.0.0:6274?MCP_INSPECTOR_API_TOKEN=…
and that log line is the user's only source for the token — pasting it is the natural move. Under main that origin was the one entry in the allow-list; after F1 it's the one loopback-equivalent form not in it, so following the banner now 403s on connect. Net effect of F1 in the Docker default: localhost fixed, 0.0.0.0 broken. Both should work.
Adding it back weakens nothing — a rebinding attacker's page carries Origin: http://attacker.com, and no attacker-controlled document can claim http://0.0.0.0:6274 without the user having navigated there. Two independent fixes, ideally both:
- Include
httpOrigin(formatHostForUrl(h), port)alongside the trio in theisAllInterfacesHostbranch (and correct that comment). - Have
printServerBanneradvertiselocalhostwhen the bind host is the wildcard, so the log points at a URL that's reachable and allow-listed.
H3. Docker with a remapped port still 403s, and no doc covers it (low)
docker run -p 8080:6274 (any remap — common when 6274 is taken) puts the browser at http://localhost:8080, while CLIENT_PORT=6274 makes the allow-list the trio at 6274. Exact-match ⇒ 403 on connect, and the derived frame-ancestors blocks the sandbox iframe too. Not a regression (equally broken before), and unavoidable with port-scoped origins — but the new Hosting on a network section only tells users they need ALLOWED_ORIGINS for non-loopback access, so a loopback-with-a-different-port user has no signal. One sentence in the root README's Docker paragraph ("if you remap the published port, set -e ALLOWED_ORIGINS=http://localhost:<hostPort>") closes it. Alternatively -p 8080:6274 users can just set -e CLIENT_PORT=8080 -p 8080:8080.
H4. Nits
- The new
run-web.tsfailure path has no test. The round-1 item-3 fix (actionable message instead of a stack trace) is unguarded — delete thetry/catchand CI stays green, same class as A2. It's a ~6-line addition mirroringrun-web.test.ts:153:process.exitis already spied to throw, soprocess.env.HOST = "0.0.0.0"+await expect(runWeb([...])).rejects.toThrow("process.exit:1")+ asserterrorLinesmentionsDANGEROUSLY_BIND_ALL_INTERFACES(remember to snapshot/restoreHOST, which isn't in that file's env-cleanup list). Also worth checkingrun-web.tsstill clears the ≥90 branch gate with the new branches — you reporttest:coveragegreen, so presumably yes. - An explicitly narrow
ALLOWED_ORIGINScan silently widen the CSP.ALLOWED_ORIGINS=http://[::1]:6274(only IPv6 entries) → every entry filtered → fallbackhttp://127.0.0.1:* http://localhost:*, i.e. any-port loopback, broader than what the user asked for. Defense-in-depth only, and the alternative ('none') is worse; just noting the direction of the degradation isn't obviously the safe one. formatHostForUrlstill passes a bracketed-with-zone host straight through.formatHostForUrl("[fe80::1%eth0]")→ unchanged (thestartsWith("[")early return precedes the zone strip), sonew URL()still rejects it. Unreachable fromresolveBindHostname(which de-brackets), reachable fromcli.ts's rawprocess.env.HOST. Moving the strip ahead of the early return would make the helper total.- The vitest bypass leaves browser-mode vitest binding raw
HOST.isDevServerexcludesVITEST, which is correct forserver.hostin the unit projects — but the Storybook browser project does bind a real listener, so an ambientHOST=0.0.0.0binds it wide. Test-only, serves stories not the backend, ephemeral — mentioning for completeness, not asking for a change.
Verdict: the bind guard is done and I have nothing left on it. H1 is the one I'd fix before merge: it's a fail-open in the same subsystem the PR hardens, one character of env value away, and the fix is three lines. H2 is worth taking because the PR's own banner points users at the origin it just removed — and because the comment now asserts something untrue on my recommendation.
Note: node_modules isn't installed in this checkout, so I did not run the suite, validate, coverage, or a build. H2's connectability claim was verified by running Node directly (listen('0.0.0.0') + client connect to 0.0.0.0 → 200); H1 is established by reading the ?? semantics against createOriginMiddleware's length === 0 short-circuit.
· branch v2/enforce-loopback-bind
… real origin (#1795) Address the round-6 review of #1796: - H1: ALLOWED_ORIGINS that parses to an empty list ("", " ", ",") now falls back to the default allow-list instead of [] — the origin middleware treats [] as allow-all, so a blank value silently disabled the DNS-rebinding guard (fail open). Now fails closed. Tests added. - H2: 0.0.0.0 IS a real, locally-connectable browser origin (my round-5 premise was wrong), and the banner prints http://0.0.0.0:PORT — the user's only source for the token in Docker. So (a) defaultAllowedOrigins includes the bound-host origin alongside the loopback trio for a wildcard bind, and (b) printServerBanner advertises localhost for a wildcard bind, so the printed URL is both reachable and allow-listed. Comment corrected. - H3: root README notes a remapped Docker port needs ALLOWED_ORIGINS. - H4a: test the run-web bind-guard failure path (actionable message, not a stack trace). H4c: formatHostForUrl is now total — a bracketed-with-zone host yields a valid URL authority. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Round-6 addressed in H1 — empty H2 —
Together: in the Docker default, both H3 — remapped Docker port (low) — Added a sentence to the root README Docker paragraph: H4a — run-web failure path untested (nit) — Added a H4c — H4b / H4d — noted, no change — H4b (a lone-IPv6 Green locally: web |
|
@claude review |
|
Claude finished @cliffhall's task in 5m 5s —— View job Code review — PR #1796 (round 25, after
|
| # | Round-24 finding | Status |
|---|---|---|
| AJ1 | OAuth guard error classified auth_required |
✅ pinned to USAGE; exit 1 / code error asserted — but see AK1 |
| AJ2 | TUI/launcher printed a raw stack | ✅ both print err.message — see AK2 for the cost |
| AJ3 | isLoopbackHost doc drift |
✅ AGENTS.md + root README |
| AJ4a | CSP_HOST_SOURCE admitted * |
✅ excluded + tested — see AK3 |
| AJ4b | ::ffff:7f00:1 unpinned |
✅ added to the positives |
I re-derived isLoopbackHost independently rather than inheriting round 23's sweep: 36 spellings through the current hostUrl.ts logic compared against a real http.listen(). No false accepts (nothing accepted binds a non-loopback address) and no false rejects among bindable loopback forms except the two already declined with reason (lvh.me, a hosts/DNS alias that does resolve to 127.0.0.1; localhost.., which fails ENOTFOUND anyway). I also confirmed AH2's trailing-dot fix through the real call path rather than the predicate alone — new URL("http://127.0.0.1.:6276/…").hostname is 127.0.0.1 (WHATWG strips it for IP literals) while localhost. is preserved and binds ::1, so the .replace(/\.$/,"") is load-bearing for exactly the name form.
One substantive finding, then nits. No defect in the host-normalization, allow-list, or CSP logic.
AK1. The CLI's exit-code map and JSON envelope are unreachable through mcp-inspector --cli — the documented invocation (medium-low, pre-existing)
AJ1's premise is that "an automated caller (CI, an agent) can branch on the failure class". That holds for node clients/cli/build/index.js …, whose bin does runCli(process.argv).then(…).catch(handleError) (clients/cli/src/index.ts:16-18). It does not hold for the way every doc tells users to run it.
mcp-inspector --cli goes through the launcher, which imports runCli as a module and owns the rejection itself:
// clients/launcher/src/index.ts:52-53, 60-68
const { runCli } = await import(clientEntry("cli"));
await runCli(forwardedArgv);
…
run().catch((err: unknown) => {
console.error("Error running MCP Inspector:", err instanceof Error ? err.message : err);
process.exit(1); // ← every failure class → 1
});The CLI bin's isMain guard is false under the launcher (process.argv[1] is the launcher's path), so handleError — the only caller of formatErrorOutput — never runs. Consequences for npx @modelcontextprotocol/inspector --cli:
- All five non-zero codes collapse to 1.
NO_APP(2),AUTH_REQUIRED(3),UNREACHABLE(4),TOOL_ERROR(5) are indistinguishable. - The JSON envelope is never emitted — stderr is a human string, not
{"error":{…}}.
Both are documented contracts, in the launcher's own voice: clients/cli/README.md:3 says the CLI is "Invoked as mcp-inspector --cli", :212 promises "it exits 3 (auth_required, envelope refresh_failed)", :248 shows the envelope; and docs/mcp-app-review.md — an agent-facing recipe whose every command is npx @modelcontextprotocol/inspector --cli (lines 16, 47, 140, 195) — tells the reader at :150 that a revoked token "exits 3 (auth_required)". Under those exact commands it exits 1 with no envelope, so an agent following the doc silently loses the branch.
This is entirely pre-existing — the launcher's catch-all predates the PR, and this commit only changed err → err.message inside it. I'm raising it here because round 24 hardened the classification on a premise the shipped entry point doesn't honor, and AJ2 just edited the precise handler that flattens it. The fix is small and local to the launcher: for the CLI mode, route the rejection through the CLI's own sink (the client build already exports it alongside runCli) instead of the generic handler.
AK2. AJ2 dropped the stack at a catch-all, not at a config boundary (nit)
run-web.ts's house pattern is a narrow try/catch around one call known to throw actionable config errors; everything else still surfaces normally. AJ2 applied the same shape to clients/tui/index.ts:15 and clients/launcher/src/index.ts:60, which are the top-level sinks for all work in their modes — an ESM resolution failure, an Ink render crash, a TypeError deep inside InspectorClient. Those now print one line with no frames (verified: console.error(err) prints err.stack, console.error(err.message) does not), so a user's bug report carries nothing to locate the fault. The actionable-message goal and the debuggability goal aren't in conflict — print the message always and append err.stack when process.env.DEBUG/MCP_DEBUG is set, or keep the stack for errors that aren't a known config class.
AK3. AJ4a made LOOPBACK_FRAME_ANCESTORS fail its own CSP_HOST_SOURCE (nit)
sandbox-controller.ts:40 is ["http://127.0.0.1:*", "http://localhost:*"], and the newly-tightened regex (:57) excludes * — so the module's own fallback no longer matches the predicate that guards the same header, and the doc's closing sentence ("Only values matching this shape survive into sandboxFrameAncestors") is now literally false for the fallback path. Benign today because the filter is applied to allowedOrigins only, but it's the kind of thing a later tidy-up ("filter sources, not valid — belt and braces") turns into frame-ancestors with no sources. One clause noting the fallback is a trusted constant that deliberately uses the :* port-wildcard would keep the two from reading as contradictory.
AK4. Nits
isLoopbackHostaccepts a host-with-port.isLoopbackHost("localhost:6276")→true(verified):formatHostForUrlpasses it through, thennew URL("http://localhost:6276")parses and.hostnamedrops the port. Unreachable fromparseRunnerOAuthCallbackUrl, which feeds iturl.hostname— but exactly the AH1 situation one input over, and its siblingisAllInterfacesHostis called with a raw env value. Same resolution as AH1: bound it, or say in the doc comment that it expects a bare host.- Two error prefixes in one launcher file.
parse-launcher-argv's catch (index.ts:31) printsError: <message>while the new top-level catch printsError running MCP Inspector: <message>. Cosmetic, but the two are now three lines of the same shape. docs/mcp-app-review.md:150's exit-3 claim is the concrete casualty of AK1 — whichever way AK1 lands (fix the launcher, or narrow the doc), that line and theclients/cli/README.md:212/:248envelope example should end up agreeing with the shipped behavior.
Verdict: converged; nothing in this PR's own surface blocks. Twenty-five rounds on, the host-address work is closed and I have no change to ask for in it. Re-derived this round rather than inherited: the wildcard refusal covers both address families, the legacy inet_aton family (decimal/octal/hex, 1–4 parts), bracketed, trailing-dot, zone-scoped, IPv4-mapped, and IDNA/fullwidth spellings, with every escape failing loudly at listen(); the predicate reasons about the address the socket binds rather than the string typed; detection, listen(), the origin allow-list, the banner, the sandbox URL, the CLI deep-link, and both OAuth redirect builders consume one normalized value from one shared module in core/node/, with banner ⊆ allowedOrigins asserted as a property; the allow-list canonicalizes derived and typed inputs, fails closed on empty with no env knob to disable it, and warns-and-drops opaque, wildcard, and unparseable entries; frame-ancestors derives from the real embedder list with an IPv6 exclusion settled in a real browser; all three web bind points enforce the same host/port invariants with strictPort on each; and the OAuth callback is loopback-only with message, help text, docs, and tests in agreement. AK1 is a pre-existing launcher gap that round 24's fix walked into — a defensible follow-up issue, though the fix is small enough to fold in.
Note: node_modules isn't installed in this checkout (root and all four clients absent), so I did not run the suite, validate, coverage, smoke, or a build — taking your reported green at face value. Everything asserted above was verified by running Node directly against logic copied verbatim from the current core/node/hostUrl.ts (the 36-spelling isLoopbackHost-vs-real-listen() sweep, the new URL().hostname results for each callback-URL form, and the localhost:6276 port-swallow), or by reading the actual call chain (launcher/src/index.ts → runCli, cli/src/index.ts's isMain guard, and handleError's single call site).
· branch v2/enforce-loopback-bind
…her (#1795) Address the round-25 review of #1796: - AK1 (medium-low, pre-existing but AJ1 walked into it): `mcp-inspector --cli` goes through the launcher, which imports runCli as a module — so the CLI bin's own .catch(handleError) never fires (isMain is false), and the launcher's top-level catch flattened EVERY failure to exit 1 with a plain-text message, losing the whole EXIT_CODES map and the JSON {"error":…} envelope the CLI README documents. Exported handleError from the CLI entry and routed the launcher's --cli branch through it, so exit codes (incl. AJ1's usage/exit-1 and auth_required/exit-3) and the envelope survive the documented invocation. New smoke:cli assertion drives the built launcher with a bad --callback-url and checks the JSON envelope + exit 1 (verified green). - AK2: the TUI + launcher top-level sinks are catch-alls, not config boundaries — print err.message for readability but append err.stack under DEBUG / MCP_DEBUG so a real fault is still debuggable. - AK3: note LOOPBACK_FRAME_ANCESTORS is a trusted constant that deliberately uses :* and is emitted as-is (doesn't pass CSP_HOST_SOURCE, by design). - AK4a: doc isLoopbackHost / isAllInterfacesHost expect a bare host (no port). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Round-25 addressed in AK1 — AK2 — stack dropped at a catch-all, not a config boundary (nit) — Fixed. The TUI ( AK3 — AK4a — AK4b — two launcher error prefixes (nit) — Left as-is: Green locally: web + CLI + launcher |
|
@claude review |
|
Claude finished @cliffhall's task in 5m 27s —— View job Code review — PR #1796 (round 26, after
|
| regression | what fails |
|---|---|
| launcher routing removed (back to the generic catch-all) | JSON.parse(stderr) throws → fail (status would still be 1, so the envelope check is the load-bearing one) |
AJ1's CliExitCodeError(USAGE) removed |
"OAuth" re-matches the auth heuristic → exit 3 → status !== 1 → fail |
docs/mcp-app-review.md:150's exit-3 claim and clients/cli/README.md:212/:244 are now true of npx @modelcontextprotocol/inspector --cli, which resolves AK4c by fixing the code rather than narrowing the docs. I also re-checked that the earlier smoke steps (3–6) still pass under the new JSON stderr: they're all regex.test(stderr) against messages with no characters JSON-escaping would alter.
| # | Round-25 finding | Status |
|---|---|---|
| AK1 | mcp-inspector --cli flattened exit codes / dropped the envelope |
✅ routed through the CLI's own sink; smoke pins the envelope + AJ1's code |
| AK2 | Stack dropped at a catch-all | ✅ err.message by default, err.stack under DEBUG/MCP_DEBUG |
| AK3 | LOOPBACK_FRAME_ANCESTORS fails its own CSP_HOST_SOURCE |
✅ doc states it's a trusted constant emitted as-is |
| AK4a | isLoopbackHost / isAllInterfacesHost accept a host-with-port |
✅ documented as expecting a bare host |
| AK4b | Two launcher error prefixes | ✅ declined — different phases, agreed |
No defects in the security surface. I re-read core/node/hostUrl.ts, resolve-bind-host.ts, web-server-config.ts's defaultAllowedOrigins, sandbox-controller.ts, and runner-oauth-callback.ts end to end; nothing changed there this round beyond comments, and the invariants hold as of round 25. Four nits below, all on the newest (error-reporting) surface.
AL1. A stale clients/cli/build makes the launcher replace the real CLI error with handleError is not a function (nit)
clients/launcher/src/index.ts:57 destructures handleError out of await import(clientEntry("cli")), which resolves to the sibling build directory (join(launcherDir, "..", "..", "cli", "build", "index.js")) — untyped, so nothing checks it exists. If that build predates 4ff578c9 (a dev running npm run build:launcher alone, or an interrupted npm run build), handleError is undefined, and calling it throws a TypeError inside the catch. That propagates to the outer run().catch, which prints:
Error running MCP Inspector: handleError is not a function
— the original CLI error is gone entirely. It's dev-only (the tarball ships both builds from one commit), but the failure mode is precisely the one AK1 set out to remove: an internal error standing in for the user's actual one. One line makes it degrade instead of mask:
if (typeof handleError !== "function") throw err; // stale cli build → generic sink
handleError(err);AL2. The success half of the CLI contract still diverges from the direct bin (nit)
AK1 restored the failure contract; the success one is still asymmetric. clients/cli/src/index.ts:20-22 is runCli(...).then(() => process.exit(0)).catch(handleError) — an explicit exit. The launcher's --cli branch just awaits and lets run() resolve, so termination depends on the event loop draining. Any lingering handle (a keep-alive socket from an HTTP transport, an unclosed stdio child) hangs mcp-inspector --cli where node clients/cli/build/index.js would have exited.
Nothing lingers on the covered paths today — smoke step 1 (stdio) and step 8 (HTTP) both terminate — so this is an observation, not a bug. Worth being deliberate about rather than incidental, because the fix isn't free either: adding process.exit(0) inherits the direct bin's truncation risk (Node's process.stdout is asynchronous on pipes on macOS, so an immediate process.exit can cut a large tools/list payload). Either choice is defensible; a one-line comment saying which was chosen and why would stop the next reader from "fixing" it in the wrong direction.
AL3. Nothing in the gate exercises a non-1 exit code through the launcher (nit)
All four round-25 edits land in files outside the coverage gate — clients/launcher/src/index.ts and clients/cli/src/index.ts are documented exclude entries, and clients/tui/index.ts is outside the src/** include — so smoke:cli step 9 is their only guard. It's a good one (it pins the envelope and AJ1's classification, per the table above), but it exercises exit 1, which is also what the old catch-all produced. The claim AK1 actually makes — that EXIT_CODES survives the launcher — has no case behind it.
A second smoke case would close it cheaply, using fixtures already in the file: --use-stored-auth --server-url http://example.invalid/mcp with HOME pointed at the existing fakeHome throws CliExitCodeError(AUTH_REQUIRED, …, { code: "no_stored_token" }) (cli.ts:914) before any connect, so it's fast and offline — assert status === 3 and envelope.error.code === "no_stored_token".
AL4. Nits
DEBUGis truthy forDEBUG=0(verified — any non-empty string), and it's thedebugpackage's namespace filter, so a developer withDEBUG=someotherlib:*exported gets Inspector stacks on every launcher/TUI failure. Harmless and arguably intended for a debug flag;MCP_DEBUGalone would be unambiguous, and anisEnabled-style parse (the oneresolve-bind-host.ts:24-27already defines forDANGEROUSLY_*) would makeDEBUG=0read as off.- Two launcher error shapes now coexist, which is a step past AK4b's "different phases":
parse-launcher-argv's catch printsError: <msg>, the top-level printsError running MCP Inspector: <msg>, and the--clipath prints a JSON envelope. All three are correct for their audience; the CLI one is worth a sentence inclients/launcher/README.md, whose flag table doesn't mention that--clifailures are machine-readable while--web/--tuifailures are prose. handleErrorin the launcher is implicitlyany(dynamic-import destructure). Pre-existing forrunWeb/runCli/runTuiand not caught byno-explicit-any, so no action — noting only because AL1's guard is what a type would otherwise have given you.
Verdict: converged. Ship it. Twenty-six rounds on, I have no change to ask for in the PR's own subject, and the newest surface holds up too. The final state: the wildcard refusal covers both address families, the legacy inet_aton family (decimal/octal/hex, 1–4 parts), bracketed, trailing-dot, zone-scoped, IPv4-mapped, and IDNA/fullwidth spellings, with every escape failing loudly at listen(); the predicate reasons about the address the socket binds rather than the string typed; detection, listen(), the origin allow-list, the banner, the sandbox URL, the CLI deep-link, and both OAuth redirect builders consume one normalized value from one shared module in core/node/, with banner ⊆ allowedOrigins asserted as a property; the allow-list canonicalizes derived and typed inputs, fails closed on empty with no env knob to disable it, and warns-and-drops opaque, wildcard, and unparseable entries; frame-ancestors derives from the real embedder list with an IPv6 exclusion settled in a real browser; all three web bind points enforce the same host/port invariants with strictPort on each; the OAuth callback is loopback-only with message, help text, docs and tests in agreement; and its failure now reaches an automated caller as a usage error with the documented envelope through the documented invocation. AL1–AL4 are all a line or two each and none of them blocks.
Note: node_modules isn't installed in this checkout (root and all four clients absent), so I did not run the suite, validate, coverage, smoke, or a build — taking your reported green at face value. This round's conclusions come from tracing the actual call chain (launcher/src/index.ts → cli/build/index.js → handleError → classifyError → process.exit, the shortCircuit return ordering in cli.ts, and the two smoke regression directions) plus a direct Node check of the DEBUG=0 truthiness and console.error(err.message) behavior in AL4/AK2.
· branch v2/enforce-loopback-bind
… docs (#1795) Address the round-26 review of #1796 (converged; all nits): - AL1: guard the launcher --cli catch against a stale cli/build where handleError is undefined — rethrow to the generic sink instead of masking the real error with "handleError is not a function". - AL3: smoke:cli step 10 exercises a NON-1 exit code through the launcher (--use-stored-auth with no token → exit 3 / envelope code no_stored_token, offline), pinning that the whole EXIT_CODES map — not just exit 1 — survives the launcher. Verified green. - AL2: comment on the launcher --cli success path (let the loop drain rather than process.exit(0), which risks truncating a large piped stdout on macOS). - AL4a: DEBUG=0 / "false" / empty now read as off for the stack-on-debug gate (launcher + tui), via a small parse — DEBUG stays useful for the debug package. - AL4b: launcher README documents that --cli failures are machine-readable JSON (exit-code map + envelope) while --web/--tui are prose. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Round-26 addressed in AL1 — stale AL3 — smoke only pinned exit 1 (nit) — Fixed, and you're right it was the load-bearing gap: exit 1 is also what the old catch-all produced, so nothing proved the map survives. Added AL2 — success-path asymmetry (nit) — Documented the choice inline: the launcher AL4a — AL4b — launcher README (nit) — Added an "Error reporting" note: AL4c — Green locally: |
|
@claude review |
|
Claude finished @cliffhall's task in 4m 38s —— View job Code review — PR #1796 (round 27, after
|
| # | Round-26 finding | Status |
|---|---|---|
| AL1 | Stale cli/build masked the real error |
✅ typeof guard + comment |
| AL2 | Success-path asymmetry | ✅ documented as a deliberate choice (see AM3) |
| AL3 | Smoke only pinned exit 1 | ✅ step 10 pins exit 3 + envelope code no_stored_token |
| AL4a | DEBUG=0 truthy |
✅ launcher + tui (see AM2) |
| AL4b | Launcher README | ✅ names the --cli vs --web/--tui split |
| AL4c | handleError implicitly any |
✅ declined — AL1's runtime guard covers it |
No defects. I re-derived one thing rather than inheriting it: the OAuth callback listener has exactly one hostname source. OAuthCallbackServer.start({ hostname }) (oauth-callback-server.ts:98) is reached only from runner-interactive-oauth.ts:64-67 via options.callbackListen, whose only producer is parseRunnerOAuthCallbackUrl (cli.ts:1049, tui.tsx:104) — so the round-22 loopback guard covers the credential listener with no second door. I also confirmed the TUI's call sits before Ink renders (line 104, above the process.stdout.write patch and the render()), so a refusal exits cleanly instead of leaving the terminal in raw mode.
Four nits, all outside the security surface. Nothing blocks.
AM1. smoke-cli.mjs's docblock enumerates checks 1–8; the two that guard the newest contract aren't listed (nit)
The file header (scripts/smoke-cli.mjs:11-25) is a numbered list of what the smoke asserts, and it stops at 8 — steps 9 (usage error → envelope + exit 1) and 10 (auth error → envelope + exit 3) aren't there. That matters more than usual here: clients/launcher/src/index.ts and clients/cli/src/index.ts are both coverage-excluded, so those two steps are the only automated guard on the AK1/AL3 contract, and this docblock is where a reader learns they exist. Two lines, matching the existing style.
AM2. Three env-boolean parsers now, two truth tables (nit)
AL4a produced a fourth-and-fifth copy of "is this env var on":
| location | accepts | rejects |
|---|---|---|
resolve-bind-host.ts:24 isEnabled |
true, 1 only |
everything else (incl. yes, on) |
launcher/src/index.ts:22 wantsDebugStack |
any non-empty except 0 / false |
— |
tui/index.ts:23 debugOn (verbatim copy of the above) |
— | — |
The two semantics are each right for their job (a DANGEROUSLY_* opt-in should be strict; a DEBUG flag should be permissive because the debug package treats any non-empty value as on), so this isn't a bug. What's worth a line is that debugOn is a byte-for-byte duplicate of wantsDebugStack across two clients that can't import each other — and the launcher's version already carries the explanatory comment the TUI's lacks. If a third client ever grows a top-level sink, this becomes the shape that drifts.
AM3. AL2's new comment and handleError reason oppositely about the same hazard (nit, non-actionable)
The comment added at launcher/src/index.ts:73-75 says the success path deliberately avoids process.exit(0) because "an eager exit risks truncating a large stdout payload on a pipe (async on macOS)". Eight lines down, the failure path calls handleError, which is exactly process.stderr.write(stderr); process.exit(exitCode) (error-handler.ts:213-215) — the same write-then-exit shape on the same class of stream. It's fine in practice (the envelope is a few hundred bytes, well inside the 64 KB pipe buffer) and it's the direct bin's long-standing behavior, so I'd change nothing — but the two comments now sit in the same function asserting different things about the same risk, which is the kind of thing that reads as an oversight later. Half a clause on the handleError call ("small payload, fits the pipe buffer") would settle it.
AM4. smoke-cli.mjs inherits ambient env, and the round-24 guard made one of those vars fatal for every step (nit)
runCli in the smoke spreads ...process.env with no scrubbing. Since parseRunnerOAuthCallbackUrl runs unconditionally in runCli (cli.ts:1049, the "not flag-only" point from AJ1), a developer with MCP_OAUTH_CALLBACK_URL=http://0.0.0.0:6276/oauth/callback exported now fails steps 1–8 — none of which are about OAuth — with a loopback-guard message. The sibling harness already solves this: scripts/lib/prod-web-server.mjs:37-42 pins HOST / CLIENT_PORT explicitly over the inherited env, which is why smoke:web is immune to an ambient HOST=0.0.0.0. Adding MCP_OAUTH_CALLBACK_URL: "" to runCli/runCliAsync's env (empty reads as unset in the parser) gives smoke:cli the same immunity in one line.
AM5. Nit
clients/cli/__tests__/stored-auth.test.ts— the new AJ1 case assertsexpect(envelope.error.code).toBe("error")and thenexpect(envelope.error.code).not.toBe("auth_required"). The second is implied by the first; it reads as documentation of intent, which is fine, but a comment would say it more cheaply than a redundant assertion.
Verdict: converged. Ship it. Twenty-seven rounds on, I have no code change to ask for, and the two things I chose to re-derive from scratch this round both held. The final state:
- The wildcard refusal covers both address families, the full legacy
inet_atonfamily (decimal/octal/hex, 1–4 parts), bracketed, trailing-dot, zone-scoped, IPv4-mapped, and IDNA/fullwidth spellings — with every escape failing loudly atlisten()— and the predicate reasons about the address the socket binds, not the string typed. - Detection,
listen(), the origin allow-list, the banner, the sandbox URL, the CLI deep-link, and both OAuth redirect builders consume one normalized value from one shared module incore/node/, withbanner ⊆ allowedOriginsasserted as a property rather than a string. - The allow-list canonicalizes derived and typed inputs the way a browser does, fails closed on empty with no env knob to disable it, and warns-and-drops opaque, wildcard, and unparseable entries.
frame-ancestorsderives from the real embedder list, with an IPv6 exclusion settled in a real browser rather than read off the CSP grammar.- All three web bind points enforce the same host and port invariants from one validated config object, with
strictPorton each. - The OAuth callback — the credential listener — is loopback-only through its single hostname source, with message,
--helptext, READMEs, and tests in agreement; and its failure reaches an automated caller as a usage error with the documented exit code and JSON envelope through the documentedmcp-inspector --cliinvocation.
AM1–AM5 are all a line or two each and none of them touches behavior that matters.
Note: node_modules isn't installed in this checkout (root and all four clients absent), so I did not run the suite, validate, coverage, smoke, or a build — taking your reported green at face value. This round's conclusions come from reading the actual call chains: OAuthCallbackServer.start ← runner-interactive-oauth ← RunnerOAuthCallbackConfig ← parseRunnerOAuthCallbackUrl (the only producer, both clients); parseArgs's --use-stored-auth throw at cli.ts:914 preceding loadServerEntries at :935; getStateFilePath() being call-time not load-time; the tui.tsx:104 parse preceding the Ink render; and prod-web-server.mjs's explicit HOST/CLIENT_PORT pinning versus smoke-cli.mjs's bare ...process.env.
· branch v2/enforce-loopback-bind
Address the round-27 review of #1796 (converged; all nits): - AM4: pin MCP_OAUTH_CALLBACK_URL="" in smoke-cli's runCli/runCliAsync base env, so an ambient non-loopback value can't fail steps 1–8 (parseRunnerOAuthCallbackUrl runs on every --cli invocation). Mirrors prod-web-server.mjs's HOST pin. Verified smoke:cli still passes with a hostile MCP_OAUTH_CALLBACK_URL=http://0.0.0.0 exported. - AM1: smoke-cli docblock now lists steps 9 + 10 (the only guard on the AK1/AL3 launcher exit-code/envelope contract, since the bins are coverage-excluded). - AM3: note on the handleError call that its small envelope fits the pipe buffer, so AL2's large-stdout truncation caveat doesn't apply to it. - AM2: note on the tui debugOn copy that it mirrors the launcher's wantsDebugStack (the two bins can't import each other). - AM5: replaced the redundant `.not.toBe("auth_required")` assertion with a comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Round-27 addressed in AM4 — smoke inherited a now-fatal ambient var (nit) — Fixed, good catch. AM1 — docblock stopped at step 8 (nit) — Fixed. Added steps 9 and 10 to the header list, and noted explicitly that they're the only guard on the AK1/AL3 contract since AM3 — AL2's comment vs AM2 — AM5 — redundant assertion (nit) — Replaced Green locally: |
|
@claude review |
|
Claude finished @cliffhall's task in 7m 3s —— View job Code review — PR #1796 (round 28, after
|
| # | Round-27 finding | Status |
|---|---|---|
| AM4 | Smoke inherited a now-fatal ambient var | ✅ MCP_OAUTH_CALLBACK_URL: "" pinned in both helpers |
| AM1 | Docblock stopped at step 8 | ✅ steps 9–10 + the coverage-exclusion note |
| AM3 | AL2 comment vs. handleError reasoned oppositely |
✅ clause added on the handleError call |
| AM2 | debugOn duplicates wantsDebugStack |
✅ cross-referenced with a keep-in-sync note |
| AM5 | Redundant assertion | ✅ replaced with a comment |
I re-ran the security audit from scratch rather than inheriting it. 44 host spellings through the current core/node/hostUrl.ts logic, each compared against a real http.listen(): zero mismatches in either direction — everything that binds 0.0.0.0/:: is flagged wildcard, everything that binds loopback is flagged loopback, and every unflagged spelling either binds a genuinely distinct address or fails loudly (ENOTFOUND / EADDRNOTAVAIL). Two spellings I hadn't seen tried in earlier rounds also came out right: HOST=𝟎 (U+1D7CE, mathematical bold digit zero) and HOST=0。0。0。0 (U+3002 ideographic full stop) both bind 0.0.0.0 and are flagged — so AD1's canonicalize-first fix generalizes past the fullwidth forms it was written for, rather than enumerating them. I also confirmed new URL().hostname percent-decodes (http://%31%32%37.0.0.1:6276/… → 127.0.0.1), so the OAuth guard can't be fed an encoded host it accepts but can't bind. Also traced parsed.shortCircuit (cli.ts:1028) preceding the OAuth parse (:1049), and --use-stored-auth's throw (:914, inside parseArgs) preceding any connect — so smoke step 10 really is offline.
Two findings, both non-blocking; neither is a security defect.
AP1. Three pieces of the wildcard cover became dead when AD1 canonicalized inside the predicate — and their comments still claim they're load-bearing (low)
Round 19 (AD1) changed isAllInterfacesHost to run canonicalUrlHost(host) first. That was the right fix, but canonicalUrlHost does everything the downstream helpers were written to do, so three of them no longer fire. I measured this rather than reasoned it — instrumented the real pipeline and counted which sub-check produced the verdict, over 27 wildcard spellings plus 125 generated IPv6 forms:
| element | claimed job | times it changed the verdict |
|---|---|---|
canonicalizeIpv6 (hostUrl.ts:96) |
compress ::0 / 0::0 / 0000:… → :: |
0 — always identity (new URL() in canonicalUrlHost already compressed) |
"::ffff:0:0" in ALL_INTERFACES_LITERALS (:82) |
catch the IPv4-mapped wildcard | 0 — canonicalUrlHost unmaps it to 0.0.0.0 first, so ::ffff:0:0 can never be the normalized value |
isAllZeroIpv4 (:125) |
"guards the near-miss bypasses of the literal set" | 0 for any bindable input — 0, 0x0, 0.0, 0.0.0, 000.000.000.000, 00000000000, 0x00000000, ０ all canonicalize to exactly 0.0.0.0 |
isAllZeroIpv4 isn't fully dead — it's the only thing that flags HOST="[0.0.0]" (bracketed non-IPv6 → canonicalUrlHost throws → catch → 0.0.0), but that value fails ENOTFOUND at listen() anyway. So all three are belt-and-braces now, not the cover.
Nothing is broken and I'd keep the defense-in-depth. What's worth fixing is the comments, which read as a specification of how the guard works: ALL_INTERFACES_LITERALS's doc says the two IPv6 entries "cover the whole IPv6 family (see canonicalizeIpv6)", and isAllZeroIpv4's says it "guards the near-miss bypasses" — a reader (or a future refactor) will conclude that deleting canonicalUrlHost from line 145 is safe because those still catch the legacy spellings. They don't; that call is now the entire cover. This is the same class as U3a (round 12's dead "::1" in LOOPBACK_HOSTNAMES), which was resolved by deleting the entry — here I'd keep the code and correct the prose, since one of the three still has a live (if unbindable) input.
AP2. ALLOWED_ORIGINS replaces the default list, so the network-hosting recipe the new docs give silently breaks local access (low)
web-server-config.ts:398 is configuredOrigins?.length ? configuredOrigins : defaultAllowedOrigins(...) — an explicit list wins outright, it isn't merged. That's a defensible design, but it interacts badly with the two places this PR newly tells users to set the var:
clients/web/README.md, wildcard bullet: "local access works out of the box … Reaching it at a non-loopback address still needsALLOWED_ORIGINS:ALLOWED_ORIGINS=http://192.168.1.50:PORT,https://inspector.example.com." Following that exact line removes the loopback trio, sohttp://localhost:6274— the access the same paragraph just promised works out of the box, and the URL the Docker banner prints (H2's fix) — starts 403-ing. And since round 3's C4, the derivedframe-ancestorsfollows the same list, so MCP Apps breaks on loopback too.- Root
README.md, Docker port-remap:-e ALLOWED_ORIGINS=http://localhost:8080is fine as written (the browser is atlocalhost:8080), but it inherits the same trap the moment anyone adds a second origin.
The failure is silent and self-inflicted-looking: the user added an origin and lost a different one. Either merge the defaults into a configured list (adding loopback origins weakens nothing — no attacker document can claim them), or add one clause to the README: ALLOWED_ORIGINS replaces the default list rather than adding to it, so include every origin you'll browse from, including the loopback forms.
AP3. Nits
isLoopbackHost's negativeit.eachhas no empty-string case.isLoopbackHost("")isfalse(correct), and the OAuth path guards!hostnameseparately, but""is the one value the sibling predicate treats as the wildcard — pinning it here would make the two suites say the same thing about the same input.MCP_DEBUG=1prints no stack on the--clipath.handleErrorexits before the launcher's top-level catch can consultwantsDebugStack(), so the debug affordance covers--web/--tuionly. That matches the direct bin and the launcher README scopes the claim correctly (--web/--tui… appendMCP_DEBUG=1), so nothing is wrong — noting it because "the stack is still available under DEBUG / MCP_DEBUG for real faults" (launcher/src/index.ts:96) sits ten lines below a branch where it isn't.formatHostForUrlpreserves case whilecanonicalUrlHostlowercases — a known, doc-noted boundary (round 15 F4), unchanged. The only remainingformatHostForUrl-not-canonicalUrlHostcall sites areserver.ts:136'sEADDRINUSEmessage (deliberate, AA3) and the two OAuth redirect builders (correct — a redirect URI must echo what was registered, not a re-canonicalized form).
Verdict: converged. Ship it. Twenty-eight rounds on, I have no code change to ask for, and the two properties I chose to re-derive independently this round both held — the wildcard cover survived a fresh 44-spelling flag-vs-real-listen() audit including two Unicode forms nobody had tried, and the guard's single hostname source into the OAuth listener is unchanged. The final state:
- The wildcard refusal covers both address families, the full legacy
inet_atonfamily (decimal/octal/hex, 1–4 parts), bracketed, trailing-dot, zone-scoped, IPv4-mapped, and IDNA/Unicode spellings — with every escape failing loudly atlisten()— because the predicate reasons about the address the socket binds, not the string typed. - Detection,
listen(), the origin allow-list, the banner, the sandbox URL, the CLI deep-link, and both OAuth redirect builders consume one normalized value from one shared module incore/node/, withbanner ⊆ allowedOriginsasserted as a property rather than a string. - The allow-list canonicalizes derived and typed inputs the way a browser does, fails closed on empty with no env knob to disable it, and warns-and-drops opaque, wildcard, and unparseable entries.
frame-ancestorsderives from the real embedder list, with an IPv6 exclusion settled in a real browser rather than read off the CSP grammar.- All three web bind points enforce the same host and port invariants from one validated config object, with
strictPorton each. - The OAuth callback — the credential listener — is loopback-only through its single hostname source, with message,
--helptext, READMEs, and tests in agreement; and its failure reaches an automated caller as a usage error with the documented exit code and JSON envelope through the documentedmcp-inspector --cliinvocation, now pinned by two smoke steps covering both an exit-1 and a non-1 code.
AP1 is comment-only; AP2 is a doc clause (or a small merge). Neither blocks.
Note: node_modules isn't installed in this checkout (root and all four clients absent), so I did not run the suite, validate, coverage, smoke, or a build — taking your reported green at face value. Everything asserted above was verified by running Node directly against logic copied verbatim from the current core/node/hostUrl.ts: the 44-spelling flag-vs-listen() audit, the instrumented sub-check counts behind AP1 (27 wildcard spellings + 125 generated IPv6 forms, 0 hits for all three elements), and the new URL().hostname percent-decode / trailing-dot results. AP2 is established by reading web-server-config.ts:398 against createOriginMiddleware's exact-match check and sandboxFrameAncestors's input.
· branch v2/enforce-loopback-bind
…ces doc (#1795) Address the round-28 review of #1796 (converged; both non-blocking): - AP1 (comment-only): since AD1 (round 19) made isAllInterfacesHost run canonicalUrlHost FIRST, that call is the actual wildcard cover — canonicalizeIpv6, the ::ffff:0:0 literal, and isAllZeroIpv4 became a redundant second layer that (measured) changes no verdict for any bindable input. Corrected the comments so they no longer read as the cover, with an explicit "do NOT drop the canonicalUrlHost call" note so a future refactor doesn't regress. Kept the code as defense-in-depth (it does cover canonicalUrlHost's non-URL catch path). - AP2: ALLOWED_ORIGINS REPLACES the default list (exact-match, no merge), so the network-hosting recipe that set only a LAN origin silently dropped the loopback trio — breaking http://localhost (which the same paragraph/Docker banner promises) and the derived sandbox CSP. Documented "replaces, not merges — list every origin incl. loopback" and fixed the wildcard-bullet example to keep the loopback forms. - AP3a: isLoopbackHost negative test now pins "" (the wildcard's job, not loopback). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Round-28 addressed in AP1 — dead-cover comments (low, comment-only) — Fixed. You're right that AD1 (round 19) made AP2 — AP3a — empty-string AP3b/AP3c — Green locally: web |
|
@claude review |
|
Claude finished @cliffhall's task in 6m 1s —— View job Code review — PR #1796 (round 29, after
|
| # | Round-28 finding | Status |
|---|---|---|
| AP1 | Dead-cover comments claimed load-bearing | ✅ layering note + all three doc comments corrected |
| AP2 | ALLOWED_ORIGINS replaces the default |
✅ documented; wildcard-bullet example now keeps the loopback forms (see AQ2 for the third recipe) |
| AP3a | isLoopbackHost empty-string negative |
✅ pinned |
The audits I ran this round (not inherited)
I copied the current core/node/hostUrl.ts logic verbatim into a standalone module and compared its verdicts against a real http.listen(), deliberately choosing spellings none of the prior 28 rounds tried:
Wildcard cover — 31 exotic spellings, zero mismatches. The interesting positives, all of which genuinely bind 0.0.0.0 and are all flagged:
HOST |
codepoint | binds | flagged |
|---|---|---|---|
𝟎 / 𝟢 |
U+1D7CE / U+1D7E2 (mathematical digit zero) | 0.0.0.0 |
✅ |
0 |
0 + U+200B zero-width space |
0.0.0.0 |
✅ |
0 |
0 + U+FEFF BOM |
0.0.0.0 |
✅ |
0。0。0。0 |
U+3002 ideographic full stop | 0.0.0.0 |
✅ |
0x0000000000000000000 / 0000000000000000000000 |
overlong hex/decimal | 0.0.0.0 |
✅ |
0:0:0:0:0:0:0.0.0.0 |
IPv4-embedded IPv6 wildcard | :: |
✅ |
And the correct negatives: Arabic-Indic ٠ / Extended-Arabic ۰ are not IDNA-mapped to ASCII digits (they punycode to xn--8hb / xn--dmb and don't bind), 4294967295 binds 255.255.255.255, ::ffff:0:0:0 is EADDRNOTAVAIL — none flagged. This is the payoff of AD1's canonicalize-first shape: it generalizes to Unicode forms nobody enumerated, rather than covering the three fullwidth spellings that motivated it.
OAuth loopback guard — 19 callback URLs through the real path (new URL(...).hostname → isLoopbackHost → stripBrackets → listen()): no false accepts. 127.1, 0177.0.0.1, 2130706433, 017777777777, [0:0:0:0:0:0:0:1], [::ffff:127.0.0.1], localhost., 127.0.0.1., and percent-encoded %31%32%37.0.0.1 all accept and bind loopback; xn--localhost-. correctly rejects.
I also re-verified: handleError → formatErrorOutput → classifyError → process.exit(exitCode) terminates before the launcher's catch-all; smoke step 10's no_stored_token throw (cli.ts:920) precedes loadServerEntries (:935), so it really is offline; fakeHome is in scope at smoke-cli.mjs:469; and the launcher README's exit-code enumeration (1/2/3/4/5) matches EXIT_CODES exactly.
No code defects. Two nits, both one-liners, both outside the security logic.
AQ1. smoke:tui didn't get AM4's env pin — the same ambient var breaks it (nit)
Round 27 fixed smoke-cli.mjs by pinning MCP_OAUTH_CALLBACK_URL: "" in SMOKE_BASE_ENV, because parseRunnerOAuthCallbackUrl runs unconditionally on every --cli invocation. The TUI has the same shape and didn't get the same treatment:
clients/tui/tui.tsx:104—parseRunnerOAuthCallbackUrl(options.callbackUrl)runs afterloadTuiServersand before the Inkrender().scripts/smoke-tui.mjs:93—env: { ...process.env, HOME: work, USERPROFILE: work }, no scrub.
So a developer with MCP_OAUTH_CALLBACK_URL=http://0.0.0.0:6276/oauth/callback exported fails smoke:tui with a crash-before-render, from a guard that has nothing to do with what the smoke asserts. Strictly a developer paper cut — smoke:tui self-skips under CI (needs a TTY) — but it's the one remaining spawn site of the class AM4 named, and the fix is the same line.
AQ2. AP2's "replaces, not merges" clause didn't reach the third ALLOWED_ORIGINS recipe (nit)
Round 28 fixed the two clients/web/README.md bullets that hand out an ALLOWED_ORIGINS value. The root README.md Docker paragraph is the third, and still reads:
If you remap the published port (
-p 8080:6274) … set-e ALLOWED_ORIGINS=http://localhost:8080… or connects will 403.
Under the replace semantics that value is the whole allow-list, so http://127.0.0.1:8080 — which the web README two files over calls interchangeable with localhost, and which is the form docs/mcp-app-review.md tells reviewers to use — now 403s, and the derived frame-ancestors becomes http://localhost:8080 alone, blocking the Apps iframe for a 127.0.0.1 embedder. Same trap AP2 just closed, one recipe over. Listing both forms (-e ALLOWED_ORIGINS=http://localhost:8080,http://127.0.0.1:8080) fixes it in place.
AQ3. Observations, no action
- A specific-IPv6 bind makes the CSP and the connect guard disagree in direction.
HOST=2001:db8::1→allowedOriginsis the singlehttp://[2001:db8::1]:6274, whichCSP_HOST_SOURCEdrops (bracketed IPv6), soframe-ancestorsfalls back to the any-port loopback family — broader than the allow-list, and it doesn't admit the origin the app is actually served on, so Apps is blocked there. Both halves are correct-by-design (the fallback direction is H4b's deliberate choice; the IPv6-literal limit is F2's browser-verified finding and is documented in the MCP Apps caveat). Naming the combination only so it isn't rediscovered as a bug. resolveBindHostnamereturns the typed spelling, not the canonical address (HOST=０+ opt-in →"０"). Correct —listen()resolves it, and every downstream consumer canonicalizes independently — but the docstring's "detection,listen(), and the origin list all consume the same value" means the same string, with canonicalization happening per-consumer, not that the returned value is canonical. Only reachable behind the opt-in.smoke:clistep 9/10'sfail()-inside-catcharoundJSON.parseis the good shape (failexits), and step 9 pins both AK1's routing and AJ1's classification — deleting either regresses it. Nice.
Verdict: converged. Ship it. Twenty-nine rounds on, I have no code change to ask for, and the two properties I chose to re-derive from scratch — the wildcard cover and the OAuth loopback guard — both survived spellings that hadn't been tried before, which is the strongest signal available that the cover is structural rather than enumerated. The final state:
- The wildcard refusal covers both address families, the full legacy
inet_atonfamily (decimal/octal/hex, 1–4 parts), bracketed, trailing-dot, zone-scoped, IPv4-mapped, and arbitrary IDNA/Unicode spellings — with every escape failing loudly atlisten()— because the predicate reasons about the address the socket binds, not the string typed. - Detection,
listen(), the origin allow-list, the banner, the sandbox URL, the CLI deep-link, and both OAuth redirect builders consume one normalized value from one shared module incore/node/, withbanner ⊆ allowedOriginsasserted as a property rather than a string. - The allow-list canonicalizes derived and typed inputs the way a browser does, fails closed on empty with no env knob to disable it, and warns-and-drops opaque, wildcard, and unparseable entries.
frame-ancestorsderives from the real embedder list, with an IPv6 exclusion settled in a real browser rather than read off the CSP grammar.- All three web bind points enforce the same host and port invariants from one validated config object, with
strictPorton each. - The OAuth callback — the credential listener — is loopback-only through its single hostname source, with message,
--helptext, READMEs, and tests in agreement; and its failure reaches an automated caller as a usage error with the documented exit code and JSON envelope through the documentedmcp-inspector --cliinvocation, pinned by two smoke steps covering both an exit-1 and a non-1 code.
AQ1 and AQ2 are each a single line and neither blocks.
Note: node_modules isn't installed in this checkout (root and all four clients absent), so I did not run the suite, validate, coverage, smoke, or a build — taking your reported green at face value. Everything asserted above was verified by running Node directly against logic copied verbatim from the current core/node/hostUrl.ts (the 31-spelling flag-vs-listen() audit and the 19-URL OAuth accept-vs-bind audit), or by reading the actual call chains (tui.tsx:104 preceding render(); smoke-tui.mjs:93's spawn env; cli.ts:920 preceding :935; EXIT_CODES vs. the launcher README).
· branch v2/enforce-loopback-bind
…cipe (#1795) Address the round-29 review of #1796 (converged; two one-liners): - AQ1: smoke-tui.mjs pins MCP_OAUTH_CALLBACK_URL="" like smoke-cli's SMOKE_BASE_ENV (round 27 AM4) — parseRunnerOAuthCallbackUrl runs before the TUI's Ink render, so an ambient non-loopback value would crash the smoke before render. - AQ2: the root README Docker port-remap recipe now lists both loopback forms (http://localhost:8080,http://127.0.0.1:8080) and notes ALLOWED_ORIGINS replaces the default — the third recipe AP2's clause hadn't reached. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Round-29 addressed in AQ1 — AQ2 — root README Docker recipe missed AP2's clause (nit) — Fixed. The port-remap recipe now sets AQ3 — observations, no action — All three are correct-by-design as you noted: the specific-IPv6-bind CSP/connect direction split is H4b's fallback + F2's documented IPv6-literal limit; Green locally: |
|
@claude review |
|
Claude finished @cliffhall's task in 5m 30s —— View job Code review — PR #1796 (round 30, after
|
| # | Round-29 finding | Status |
|---|---|---|
| AQ1 | smoke:tui missed the env pin |
✅ pinned, comment cross-references SMOKE_BASE_ENV |
| AQ2 | Root README Docker recipe missed the "replaces" clause | ✅ both forms + the clause + link |
| AQ3 | Three observations | ✅ correct-by-design, no change — agreed |
The audits I ran this round (fresh, not inherited)
1. The residual risk area for the wildcard cover — hosts where new URL() throws, so canonicalUrlHost falls back to the raw string and only the redundant second layer (isAllZeroIpv4 / canonicalizeIpv6) can catch them. I ran ~28 such candidates through the current core/node/hostUrl.ts logic and a real http.listen(). Zero misses — everything that binds the wildcard is flagged, and every unflagged spelling fails loudly:
HOST |
new URL throws |
flagged | listen() |
|---|---|---|---|
[0.0.0] / [0] / [] / [0.0.0.0] |
✅ | ✅ | ENOTFOUND |
0..0 / 0 0 / 0% / 0%0 / % |
✅ | ❌ | ENOTFOUND |
0X0 / 00 / 𝟎 (U+1D7CE) / ０．０ |
— | ✅ | 0.0.0.0 |
0. / 0x / 0:0 / 0.0.0.0. / [::ffff:0.0.0.0] |
— | ✅ | EAI_AGAIN / ENOTFOUND |
٠ (Arabic-Indic zero) → xn--8hb |
— | ❌ | EAI_AGAIN (correctly not IDNA-folded) |
So the isAllZeroIpv4 fallback layer AP1 documented as "only live input is a bracketed non-IPv6 like [0.0.0]" is exactly right, and that input isn't bindable.
2. The OAuth loopback guard through the real path (new URL(url).hostname → isLoopbackHost → stripBrackets → listen()), 19 callback URLs. No false accepts. Accepted-and-binds-loopback includes the ones a literal-set guard would miss: fullwidth ｌｏｃａｌｈｏｓｔ, percent-encoded %6c%6f%63…, 0177.0.0.1, 127.0.0.001, 2130706433, [0:0:0:0:0:ffff:127.0.0.1], 127.0.0.0, 127.255.255.255, localhost.. Rejected: 0.0.0.0, [::], 192.168.1.50, [::ffff:192.168.1.50], xn--8hb.
3. URL-builder sweep — still complete. Every http://${…} host interpolation across core/, clients/web/server/, clients/cli/, clients/tui/, clients/launcher/ goes through formatHostForUrl (the two OAuth redirect builders + server.ts's EADDRINUSE message — all deliberate) or canonicalUrlHost (web-server-config.ts:181, sandbox-controller.ts:213, cli.ts:429), except vite-hono-plugin.ts:144, which uses the request's own Host header.
4. Ambient-env sweep — now closed. This PR introduces exactly three env vars that can fatally abort a run that previously started: an all-interfaces HOST, an invalid CLIENT_PORT, and a non-loopback MCP_OAUTH_CALLBACK_URL. All three are pinned in the smoke harnesses that could hit them — prod-web-server.mjs pins HOST/CLIENT_PORT (shared by smoke:web + smoke:web:browser), smoke-cli.mjs's SMOKE_BASE_ENV and now smoke-tui.mjs pin the callback URL. AQ1 was the last hole.
I also re-checked the Docker path end to end: HEALTHCHECK fetches http://127.0.0.1:6274/ from inside the container — a same-origin-less GET against a wildcard bind that serves loopback, so it's unaffected by the origin guard; and printServerBanner advertises localhost while defaultAllowedOrigins emits the trio + the canonical wildcard pair, so both the printed URL and a hand-typed 0.0.0.0 connect.
Observations (no change requested)
localhost.localdomainis rejected by the OAuth guard, and it's distro-shipped, not user-configured. Verified it binds::1here. This is the AH3a class (hosts-file loopback alias) you declined in round 23 on the grounds that the usual reason to reach for a name is served bylocalhost— still the right call for a--callback-urlvalue; noting only that RHEL/Fedora ship127.0.0.1 localhost localhost.localdomain localhost4by default, so this alias is slightly more "real" than themyapp.localexample the declination was written against. If a report ever surfaces, that's the shape.hostUrl.test.ts:117mislabels one case."[::1]"is in theit.eachtitled "passes the non-IPv6 / already-bracketed host … through", but it actually takes the IPv6 branch (stripBrackets→isIPv6("::1")→ re-bracket). The assertion (toBe(host.trim())) is green either way and the value is identical, so nothing is wrong — the title just describes a path this input doesn't take. A one-word retitle, or move it to the bracketing block above where["[fe80::1%eth0]", "[fe80::1]"]already lives.- Three env-boolean parsers, two truth tables (
isEnabledinresolve-bind-host.ts,wantsDebugStackin the launcher, its verbatim twin inclients/tui/index.ts) — unchanged since AM2, correctly declined; the TUI copy carries the keep-in-sync note. - The
.gitignore*.tsbuildinforule is unanchored, so it also covers a stray buildinfo in any subdirectory, which is what you want given cli/tui write theirs next to their tsconfig.git ls-files | grep tsbuildinfois empty.
Verdict: converged. Ship it. Thirty rounds on, I have no code change to ask for, and the two hardest properties — the wildcard cover and the OAuth loopback guard — held under probe classes designed specifically to defeat a canonicalize-first predicate (URL-parse failures, Unicode digit variants outside the fullwidth block, percent-encoding, IPv4-embedded IPv6, out-of-range octets). The final state:
- The wildcard refusal covers both address families, the full legacy
inet_atonfamily (decimal/octal/hex, 1–4 parts), bracketed, trailing-dot, zone-scoped, IPv4-mapped, and arbitrary IDNA/Unicode spellings — with every escape failing loudly atlisten()— because the predicate reasons about the address the socket binds, not the string typed. The redundant second layer is now documented as redundant, with an explicit warning against removing the call that actually does the work. - Detection,
listen(), the origin allow-list, the banner, the sandbox URL, the CLI deep-link, and both OAuth redirect builders consume one normalized value from one shared module incore/node/, withbanner ⊆ allowedOriginsasserted as a property rather than a string. - The allow-list canonicalizes derived and typed inputs the way a browser does, fails closed on empty with no env knob to disable it, and warns-and-drops opaque, wildcard, and unparseable entries.
frame-ancestorsderives from the real embedder list, with an IPv6 exclusion settled in a real browser rather than read off the CSP grammar.- All three web bind points enforce the same host and port invariants from one validated config object, with
strictPorton each. - The OAuth callback — the credential listener — is loopback-only through its single hostname source, with message,
--helptext, READMEs, and tests in agreement; and its failure reaches an automated caller as a usage error with the documented exit code and JSON envelope through the documentedmcp-inspector --cliinvocation, pinned by two smoke steps covering both an exit-1 and a non-1 code.
Note: node_modules isn't installed in this checkout (root and all four clients absent), so I did not run the suite, validate, coverage, smoke, or a build — taking your reported green at face value. Audits 1 and 2 above were run for real via Node, against logic copied verbatim out of the current core/node/hostUrl.ts plus real http.listen() binds; audits 3 and 4 are exhaustive greps/reads of the current tree.
· branch v2/enforce-loopback-bind
|
Round-30: converged — thanks for the thirty-round adversarial pass. No actionable findings this round, so I'm leaving HEAD (
Appreciate the empirical audits throughout — the headless-Chromium CSP check (F2), the pinned-Vite |
- cleanRoots' opening sentence still framed it as the settings-form normalizer, contradicting the paragraph below it that says every client feeds it from disk. It has four callers now; says so. - Marked the two runtime guards `Keep:` with the reason. The `Root[]` parameter narrows both to `never`, so "the compiler says this can't happen" is the argument someone will use to delete the checks rounds 6-7 installed. The reason now lives where the deletion would happen. - Noted the constructor-normalization case in the timing test's header, so it is discoverable from a file otherwise about the initialized race. Also merged origin/v2/main (#1796, loopback binding), which had advanced by one commit and touched the smoke scripts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): register peer-request handlers before connect (#1797)
The roots/sampling/elicitation capabilities are advertised on the SDK
`Client` at construction, so from the moment `connect()` sends
`notifications/initialized` the server may issue any of those requests.
The handlers, however, were registered after the handshake resolved and
after `fetchServerInfo()` — leaving a window in which the SDK answered
`-32601 Method not found`.
`server-filesystem` asks for `roots/list` the instant it is initialized
(that is how it learns its allowed directories) and lost the race, so it
silently fell back to its CLI-argument directories instead of the roots
configured in the Inspector. `server-everything` asks later and did not.
Extract the four peer-request handler blocks into
`registerPeerRequestHandlers()` and call it before `client.connect()`.
None of them depend on server capabilities — only on constructor-set
state. The notification handlers that do gate on `this.capabilities` stay
in `connect()`, after `initialize`.
Regression test drives the race deterministically with a fake transport
that delivers `roots/list` synchronously from inside the `send()` of
`notifications/initialized`; it fails on the pre-fix ordering.
Closes #1797
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-1 fixes — pre-connect roots/list_changed, wider timing test (#1797)
Finding 1: `notifications/roots/list_changed` was the one handler in the
post-handshake block that gates on nothing but constructor state, so it
sat in the same race — a server emitting it right after `initialized` had
it silently dropped (no wire error; the symptom is a missed UI refresh).
Moved to a new `registerPeerNotificationHandlers()` called next to
`registerPeerRequestHandlers()` before `connect()`.
Finding 2: the timing test asserted one of the four moved handler blocks.
It now injects `tasks/list` and the `roots/list_changed` notification in
the same burst as `roots/list`, so moving any of the block back after
`connect()` fails the test. Sampling/elicitation stay out — they park a
pending request awaiting user input and have no reply to assert on.
Dropped the unused `sessionId`/`setProtocolVersion` transport fields.
Verified the added assertions are load-bearing: the notification
assertion fails against the previous commit, the `roots/list` one against
`origin/v2/main`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-2 fixes — correct the roots/list_changed premise (#1797)
The round-1 doc comment claimed a server may emit
`notifications/roots/list_changed` at initialize. It can't, normally:
`RootsListChangedNotificationSchema` is a member of
`ClientNotificationSchema` and absent from `ServerNotificationSchema` —
the client sends it (setRoots does, at :4251). The inbound handler is
defensive coverage for a non-conformant server, and dispatches
`rootsChange` with our own already-known roots. Reworded the method doc
and the test's header so neither states the false premise; the move
itself stands (it gates on no server capability, so it belongs with the
request handlers).
Also from review:
- Dropped the vacuous `notificationRejected` flag and its inverted
comment. The SDK does not throw for an unregistered notification, so
the try/catch could never observe one — confirmed by bisect, where the
pre-fix failure was always `rootsChanges`, never the flag. Removing the
catch also stops it swallowing a genuine throw from the injected
requests.
- Narrowed `replies` to responses (`"result" in message || "error" in
message`) so it can't also collect client-originated requests.
- Annotated the unreachable `if (!this.client)` guard with a justified
`v8 ignore` per the AGENTS.md dead-guard policy.
Re-verified both regression directions: the `roots/list` assertion fails
against `origin/v2/main`, the `rootsChange` one against `55e25b7c`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-3 fix — drop stale comments inside the roots handler (#1797)
The four inline comments moved verbatim with the handler contradicted the
method doc reworded one round earlier: they said "server's roots" (they
are ours — it is a ClientNotification) and "we'll just dispatch an empty
array", which is not what `this.roots || []` does when roots are
configured. The new timing test asserts exactly that non-empty dispatch.
Replaced with one accurate line. Comment-only.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(cli): advertise roots so roots/set is actually serviceable (#1797)
Round-4 review found the same -32601 symptom on a second path: the CLI
constructed its InspectorClient with no `roots` option, so
`capabilities.roots` was never advertised and no `roots/list` handler was
registered — yet `--method roots/set` calls `setRoots()`, which announces
`notifications/roots/list_changed` to the server. A server taking up that
invitation got "Method not found", after the CLI reported success.
The review's suggested fix — register `roots/list` unconditionally — is
not implementable: the SDK asserts the client capability inside
`setRequestHandler` ("Client does not support roots capability"), so it
throws during connect for any client built without the option, and
`registerCapabilities` refuses to run after connect. A client that omits
`roots` therefore cannot legally serve `roots/list` at all.
Fixed at the layer that can fix it: `cli.ts` seeds `roots: []`, matching
web (`App.tsx` always passes it). The core registration stays gated on
the constructor value, now with the SDK constraint documented so the next
reader doesn't retry the unconditional version.
`setRoots()`'s doc claimed it "will enable roots capability if it wasn't
already enabled" — it cannot, for the reasons above. Corrected, and
dropped the dead `if (this.roots === undefined) this.roots = []` the very
next line overwrote.
Tests: a CLI test drives `list_roots` over an HTTP test server (verified
load-bearing — without the seed it fails on the -32601 error text), and
the core timing test gains a case pinning that roots set after connect
are served with the current values, not the constructor's.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(cli,tui): advertise the roots configured in mcp.json (#1797)
Round-5 review: seeding `roots: []` fixed the -32601 but not the outcome.
`server-filesystem` discards an empty roots answer (`validatedRootDirs
.length > 0`) and keeps its CLI-argument directories — which is the
symptom this PR set out to fix. The user's roots were already in hand two
lines away, on `serverSettings.roots` (lifted from mcp.json by
`mcpConfigToServerEntries`). Now passed through `cleanRoots`, the same
shared helper web uses, so both clients answer `roots/list` byte-identically
for the same config.
This matters most on the CLI because it is one-shot: `--method roots/set`
sets roots on a connection torn down a few statements later, so the config
file is the only durable way to give a run its roots.
Same gap in the TUI (`App.tsx` built its options with no `roots` key at
all), fixed the same way. The review suggested a follow-up issue for it;
folding it in here instead, since it is the identical line and splitting
it would leave a known-wrong client in tree.
Also from review: hoisted `waiters` up with the other fields, and
`injectRequest` now rejects after 1s naming the method and id rather than
hanging to the vitest timeout.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-6 fixes — document roots in the CLI README, harden cleanRoots (#1797)
- clients/cli/README.md enumerated the settings lifted from a catalog/config
file without roots. Roots are the least discoverable member of that list —
there is no flag, and `--method roots/set` dies with the connection — so
the file is the only durable way to supply them. Said so.
- cleanRoots() trusted its input's shape. `Root[]` is a compile-time type over
hand-editable mcp.json, and as of this PR all three clients feed it straight
from disk, so `"roots": [{"name":"Work"}]` or `"roots": "file:///work"` threw
at connect. Non-array bails to []; an entry without a string uri is dropped
with a warning. Hardened in the one shared place rather than three call
sites. Review suggested a follow-up issue; folded in for the same reason as
the TUI fix last round.
- Test comment said the CLI passes a `roots: []` seed — true one commit ago.
- injectRequest clears its timeout on reply (an armed timer outliving a test is
the #1760 teardown class), and setRoots' doc drops the "despite what this
comment used to claim" archaeology.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-7 fix — cleanRoots also throws on a non-string name (#1797)
The round-6 hardening guarded `uri` and left `name` throwing one line
later: `r.name?.trim()` guards null/undefined, not "is a string", so
`"roots": [{"uri":"file:///a","name":42}]` still died at connect — the
same crash class, one field over, reachable from all three clients and
the web settings-save path. Reproduced against the shipped function body
before fixing.
A bad `name` now costs the name, not the root (it is optional, and the
uri is still usable), with a warning. The doc no longer overstates what
is validated.
Also from review: the CLI test comment said "`cli.ts` now seeds
`roots: []`" — true only of that ad-hoc test's path; and the
registration comment's `(:588)` line reference is now a durable name.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): normalize roots in setRoots() too (#1797)
Round-8 review: this PR made cleanRoots the single normalizer for roots at
connect and at save, but setRoots — the third way roots enter the client —
did a bare `this.roots = [...roots]`. Web cleans at its call site; the CLI
does not, so `--method roots/set --roots-json '[{"name":"Work"}]'` stored a
root with no uri, advertised it in the roots/list reply, and echoed it back
as success. cleanRoots is idempotent, so calling it inside setRoots covers
the CLI without disturbing web's pre-clean, and all three entry points now
agree.
Review filed this as an optional follow-up; folding it in for the same
reason as the TUI and cleanRoots items — it is smaller than the issue
describing it.
Test asserts the malformed entry is dropped and the valid one survives;
verified load-bearing against the bare-spread version.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-9 fixes — normalize roots in the constructor too (#1797)
The round-8 commit claimed "all three entry points now agree"; review
pointed out the constructor is a fourth, and the only one still raw. All
three clients clean at their call site, so nothing was broken — but the
invariant was enforced by convention rather than by the class, at the very
entry point this whole thread started from. `this.roots` now runs through
cleanRoots, with a ternary so the `undefined`-vs-`[]` distinction that
gates capabilities.roots survives.
Also from review:
- The CLI roots/set test provoked a cleanRoots warning without suppressing
it (AGENTS.md). Suppressed, and the spy now asserts the drop was
reported rather than silent.
- `enables roots when previously undefined …` in coverage-backfill was
named after behaviour round 5 established does not exist. Renamed, with
a comment on what that client actually can and cannot do.
- setRoots dispatches a copy to rootsChange, as getRoots() already did, so
a listener can't push into the list we advertise.
- run-method.test.ts's connectStdio now passes `roots: []` like cli.ts, so
the helper matches the client shape production builds.
Verified load-bearing: the constructor test fails against the raw
assignment. validate / coverage / build-gate / smoke / storybook all green
(run as separate stages — full `npm run ci` kept being killed mid-coverage
by the environment).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-10 fixes — copy at both rootsChange dispatches (#1797)
Round-9 claimed setRoots' copy closed the "a listener can't push into the
list we advertise" hole. It closed one of two sites: my reply conflated
the notification handler's *dispatch* with the roots/list *request*
handler — the latter is serialized to the wire immediately and is fine,
the former hands listeners the internal array. Both copy now.
Also from review: dropped the `({}) as never` in the new constructor test
for the real factory the sibling tests use (this test never connects, so
it is never invoked), and the needless `async` on it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-11 doc fixes; merge v2/main (#1797)
- cleanRoots' opening sentence still framed it as the settings-form
normalizer, contradicting the paragraph below it that says every client
feeds it from disk. It has four callers now; says so.
- Marked the two runtime guards `Keep:` with the reason. The `Root[]`
parameter narrows both to `never`, so "the compiler says this can't
happen" is the argument someone will use to delete the checks rounds 6-7
installed. The reason now lives where the deletion would happen.
- Noted the constructor-normalization case in the timing test's header, so
it is discoverable from a file otherwise about the initialized race.
Also merged origin/v2/main (#1796, loopback binding), which had advanced
by one commit and touched the smoke scripts.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): clear queued peer requests when connect() fails (#1797)
Round-12 review found a second-order consequence of the handler move.
Before it, registration happened after every await in connect(), so the
moment a peer request could be queued the connect could no longer fail —
queue and failure path never overlapped. Registering before the handshake
puts both the handshake and the post-connect `setLoggingLevel` inside that
window.
The catch sets status to error and drops the transport but never cleared
pendingSamples/pendingElicitations; only disconnect() does, and connect()
reaches it solely on the connect-timeout path (not the default). So a
server that elicits at `initialized` and then fails the connect left a
live pending-request modal on a dead connection — web derives it from the
queue lengths with no status gate — and the elicitation's cancel() never
fired, hanging any awaitUrlElicitation waiter.
Extracted disconnect()'s clear-and-cancel into clearPendingPeerRequests()
and called it from the catch, dispatching the change events so the UI
drops the modal. CLI/TUI are unaffected either way (sample/elicit off).
Test note: the first version of this test passed with and without the fix
— its `requestedSchema` omitted `properties`, so the SDK rejected the
inbound params and nothing was ever queued. Fixed the fixture; it now
fails without the change with a queued ElicitationCreateMessage.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): clear the peer-request queue on a mid-session crash too (#1797)
Round-13 review, two items.
The test asserted only `getPendingElicitations()`/`getPendingSamples()`,
which read the arrays directly — but the arrays are not what removes the
modal. `usePendingClientRequests` tracks its own state off the
pendingSamplesChange/pendingElicitationsChange events, so a regression that
cleared without dispatching would strand a live modal on a dead connection
and stay green. Both tests now assert the events fired with an empty
payload; verified by deleting just the dispatches, which fails them.
The connect-failure fix left the other way a connection ends uncovered:
a mid-session crash goes through the transport's `onclose`, which sets
status and fires `disconnect` but never touched the queues. Same two
hazards, and arguably more reachable. `clearAndAnnouncePendingPeerRequests`
wraps the clear plus its events, guarded on a non-empty queue so the paths
that overlap (a connect failure whose dropCachedTransport also fires
onclose) announce once; called from both. New test queues an elicitation,
fires onclose, and asserts the queue and the event — fails without the
change.
Review flagged the crash path as a follow-up; folding it in, as with the
earlier out-of-scope items — it is the same seam and three lines.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-14 fixes — make both teardown paths agree (#1797)
- The crash path's comment claimed "cleared before the `disconnect` event
so a consumer sees an empty queue" while disconnect() cleared *after*
its own dispatch — an invariant stated as a property and held at one of
two sites, the same shape as the round-10 finding. Moved disconnect()'s
clear above its status/disconnect block so both paths agree; its change
events stay batched with the other teardown dispatches. Full gate green,
so nothing depended on the old ordering.
- clearPendingPeerRequests' doc still named the `connect()` failure path as
a caller; that path now goes through clearAndAnnouncePendingPeerRequests.
Names the two real callers.
- The mid-session test waited on `await Promise.resolve()` — the
microtask-counting pattern this file replaced with injectRequest two
rounds ago, working today only because the enqueue happens exactly one
tick deep. Waits on the client's own `newPendingElicitation` instead.
- The file header still called the constructor case "the last case"; it is
third of five, and the two teardown cases went undescribed. Rewritten.
- Restored the missing blank line between two tests, and noted why the
sampling-event assertion holds for a queue that never held anything.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-15 fixes — scope the parity claim to the arrays (#1797)
Round-14 made the two teardown paths agree on when the queue is cleared,
and the comment then claimed parity flatly. It holds for the arrays but
not the events: the crash path announces before `disconnect`, while
disconnect() batches its change events with the rest of its teardown, so
they land just after. Since the events are what drives the modal, the
distinction matters to the next reader. Scoped the comment rather than
un-batching — un-batching would trade a documented ordering for a real
behaviour change with no gain.
Same rule as round 14, one level down: check the other site, then check
the other axis.
Also gave the new `newPendingElicitation` wait the timeout that
`injectRequest` documents, so a regression that stops the enqueue fails at
the wait instead of hanging to the vitest timeout.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* test: pin disconnect()'s clear-before-announce ordering (#1797)
Round-14 moved clearPendingPeerRequests() above disconnect()'s status
block so a `disconnect` consumer sees an empty queue on both teardown
paths, and round 15 documented it — but nothing enforced it. The existing
`disconnect` listeners only count events, and the two teardown tests cover
the crash and connect-failure paths, neither of which goes through
disconnect(). So the one behavioural change in the stack was untested in
both directions, and the tidy that undoes it (regrouping the clear with
the batched change events) would have stayed green.
Test records the queue length from inside a `disconnect` listener; the
fixture's close() never fires onclose, so it exercises disconnect()'s own
ordering rather than the crash path. Verified against the pre-round-14
position: `expected 1 to be +0`.
Folded the newPendingElicitation wait into a shared helper now that two
tests use it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* test: pin disconnect()'s announce as well as its clear (#1797)
Round-16 pinned the array half of disconnect()'s teardown and left the
events unpinned — and nothing else in the suite covers them, so deleting
disconnect()'s two change-event dispatches would have left a live modal
after a user-initiated disconnect with the suite green. That is the path
most likely to have a modal open when it runs. Asserted after the await
rather than inside the disconnect listener, since disconnect() batches the
events deliberately; verified by deleting the dispatches (`expected
undefined to be +0`).
Header rewritten by category rather than position: it has now gone stale
twice in three rounds because it counted tests ("the last two") and
enumerated paths that a new test falsified. It names the three teardown
paths and what distinguishes them instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-18 doc fixes — scope the three-path claim, re-attribute cancel() (#1797)
The header I rewrote last round claimed all three teardown paths clear
before announcing. True only if "announcing" means the `disconnect` event:
the connect-failure path emits none at all, and both it and the crash path
clear after their `statusChange`. Scoped to what's true. Same shape as
rounds 10/14/15 — a parity asserted in prose that holds on one of the axes
it could mean — which is now the fourth time, in a sentence I wrote.
ElicitationCreateMessage.cancel()'s doc still credited `disconnect()`
teardown; this PR made clearPendingPeerRequests() its only caller, serving
three paths. Re-attributed, and promoted the no-`onRemove` note to its own
paragraph with the consequence spelled out — it quietly became a
three-caller invariant during this PR, and "don't splice mid-iteration"
reads like a style note until you know it would skip every other entry and
leave those promises unsettled.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): gate roots/list on what was advertised, not on this.roots (#1797)
Round-19 review found a latent wedge, reproduced before fixing: a client
constructed without `roots` that later calls setRoots() has this.roots
defined, so registerPeerRequestHandlers tried to register the handler on
every subsequent connect() — and the SDK throws "Client does not support
roots capability" from setRequestHandler when the capability was never
advertised. That throw lands before the handshake, so the client can never
reconnect. Worse than the -32601 this PR started from: the connection is
gone rather than answering one method badly.
Not a regression (the same gate lived after the handshake before) and
unreachable from the three shipped clients, which all pass roots. But
core/ is a library surface, and the failure is unrecoverable.
Gate now reads a construction-time `rootsCapabilityAdvertised` — the thing
the SDK actually asserts on — so this.roots stays free to change. setRoots'
doc says what does happen on such a client: the roots are stored and
readable, but no server can ask for them.
Also scoped the header's claim that the connect-failure path emits no
`disconnect`: true of connect()'s own catch, but on a real transport
dropCachedTransport's close() fires onclose first, which does dispatch one
— making the catch's own call the auth-recovery backstop.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-20 — derive the roots gate from the advertisement itself (#1797)
Round-19's fix restored the invariant but not the derivation:
rootsCapabilityAdvertised was computed from `options.roots` alongside the
capability block rather than from it, so the two encodings of "did we
advertise roots" could drift. Adding any term to the advertisement
condition would leave the flag true with `capabilities.roots` absent —
the same unrecoverable wedge, reintroduced silently and in the throwing
direction.
Now read off the built capability object after it is assembled, so
divergence is structurally impossible rather than merely absent. Verified
the reconnect test still fails against the old `this.roots` gate.
Also: named the capability-lifetime category in the test header (the new
test was none of the three it listed), moved that test below the teardown
group so the group stays contiguous, and rewrapped the over-long comment
line.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): gate elicitation/create on what was advertised too (#1797)
Round-21 review asked round-20's question of the capability next door, and
there the two derivations already disagreed — no future edit needed.
`elicit` is typed `boolean | { form?, url? }`, so `{}` or
`{ form: false, url: false }` is valid, enables no mode, and advertises no
`capabilities.elicitation` — while the registration gated on `this.elicit`
being truthy. Reproduced before fixing:
CONNECT ERROR: Client does not support elicitation capability
(required for elicitation/create)
Since #1797 moved registration before the handshake, that throw means the
client cannot connect at all. Same wedge as the roots one, one capability
over, and reachable through a documented option value rather than a future
edit.
Gated on a new elicitationCapabilityAdvertised, derived from the built
capability object exactly as the roots flag now is. Test constructs with
`{ form: false, url: false }` and asserts connect() resolves; verified
load-bearing.
Also made the four construction-time capability inputs (sample, elicit,
receiverTasks, advertisedExtensions) `readonly`. The round-19 bug was a
mutator making a construction-time fact stale, so a future setter for any
of these should be a compile error rather than a review catch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): derive tasks.requests and the remaining gates from the advertisement (#1797)
capabilities.tasks.requests declares which server→client requests we accept
as tasks, but was built from `receiverTasks` alone — so
`{ receiverTasks: true, elicit: false }` told the server it could send a
task-augmented elicitation/create while advertising no elicitation
capability and (since the last commit) registering no handler. A server
taking the invitation gets -32601: the advertise-then-refuse shape this PR
opened with, and the last place it survived. Now built from
capabilities.sampling / capabilities.elicitation, both decided just above,
with the key omitted entirely if neither applies.
Also made the remaining three registration gates read the built capability
object like the two from the last two rounds — sampling, tasks/*, and the
URL-elicitation completion handler. None could diverge today, but a mixed
convention means the next reader has to re-derive which gates are safe, and
adding one condition to an advertisement block is exactly how elicitation
broke. Five gates, one rule now.
Test asserts a receiverTasks+elicit:false client advertises tasks.requests
with sampling and without elicitation; verified load-bearing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-23 — pin the omitted-requests arm, drop the count again (#1797)
The `requests` key is omitted when neither sampling nor elicitation is
advertised, but every `receiverTasks: true` construction in the tree left
one of them enabled, so that arm was unexercised — and it would not have
shown as a coverage failure, since one uncovered branch in a ~4900-line
file sits well inside the per-file gate. Second client in the same test
asserts `tasks` survives (list/cancel are still serviceable) while
`requests` is absent rather than empty; verified against `requests:
taskRequests`.
The header regained a numeral four commits after round 17 rewrote it by
category to stop exactly that, and it was already stale. Dropped, and named
the third category — advertise-only-what-you-serve, which is about the
capability object rather than a registration gate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): settle queued samples on teardown instead of discarding them (#1797)
Round-24 review. Round 13 recorded the samples-vs-elicitations asymmetry
as deliberate on two grounds: nothing internal awaits a sample, and the
transport is gone by then. The first still holds; the second is false on
one path, and it is a path this PR created. `connect()`'s catch drops the
transport only when `!transportHasAuthProvider`, so on the HTTP+OAuth path
— web's ordinary authenticated case — it is retained and the caller
reconnects over it. A sampling request queued during the handshake was
then discarded with the connection still live: no response frame ever
written, and the server waits forever for a reply to a request we
accepted. Accept-then-silence, which is worse for the peer than the
-32601 this PR opened with.
SamplingCreateMessage.cancel() mirrors the elicitation one — settles
(rejects, what the UI's decline path already sends and the SDK turns into
an error response) without calling onRemove, since the caller iterates the
queue and clears it. clearPendingPeerRequests loops it like the
elicitations.
Also corrected two comments that assumed the catch always tore the
transport down, and noted that the retention is gated on
transportHasAuthProvider alone — independently of
isConnectAuthRecoveryError, which gates only the status hold.
Test asserts settlement rather than a wire frame, since on the disconnect()
path the transport is closed before the settle; a later respond() throwing
"already resolved or rejected" is the proof. Verified load-bearing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): mark receiver-task payload promises handled (#1797)
Round-25 review. clearPendingPeerRequests iterates every entry in
pendingSamples, and two shapes live there: the plain server-request one,
whose reject reaches the SDK handler, and the task-augmented one, whose
reject reaches the receiver task's payloadPromise. That promise only gets
a consumer when the server polls tasks/result, so a task sitting in
input_required has no handler attached — and the settle added last commit
surfaced as an unhandled rejection.
Not hypothetical: the repo already worked around it test-side, with a
helper that reaches into private state to attach no-op catches, whose
comment describes this exact race. Fixed at the source instead — one
`void payloadPromise.catch(() => {})` at creation, which also covers the
pre-existing cancelReceiverTask path. Removed the now-redundant helper and
its two call sites. Verified: dropping the source line makes that suite
report two unhandled rejections.
Corrected SamplingCreateMessage.cancel()'s doc — its "no response frame is
ever written" rationale is true of the plain shape only; a task-augmented
sample already answered the wire with a CreateTaskResult, so settling it is
about not leaving the task in input_required limbo.
Nits: generalized the enqueue wait helper to both events so the sampling
wait gets the timeout the elicitation one has; moved the sampling teardown
test back into the contiguous teardown group; named the settle property in
the header; fixed the fixture doc that still claimed to record replies.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-26 — drop the comment that outlived its call (#1797)
The suppressReceiverPayloadRejections call went last commit; its two-line
comment stayed, describing a pre-attach that no longer happens and
crediting the test for handling a rejection the source now handles.
Removed. Also reworded the surviving .catch() in inspectorClient.test.ts:
it is still needed, but as a value capture for the assertion, not for
unhandled-rejection suppression.
Rewrapped the teardown paragraph in the test header — my round-25 edit
left a 104-char line, and two hand-fixes left it ragged, so it is reflowed
in one pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-27 — skip the roots/list_changed the SDK would refuse (#1797)
Review reported setRoots() emitting roots/list_changed on a client that
never advertised roots, inviting a re-fetch we'd answer -32601. Probed it:
the premise is wrong. The SDK asserts the notification capability too —
`notification()` rejects with "Client does not support roots list changed
notifications" and nothing reaches the wire. No spec violation and no
invitation; what actually happened is that setRoots provoked that
rejection and logged it as "Failed to send", implying a transient failure
where the truth is structural.
Kept the early return on that corrected basis: it stops us provoking a
rejection we know will come, and warns accurately instead. Doc and comment
say the SDK is the enforcer rather than claiming this guard is what keeps
the wire clean.
The test pins the end state (no invitation for roots we can't serve) and
passes either way — recorded as such rather than presented as a regression
guard, since it isn't one.
Also moved the advertise-category test below the two gate cases, matching
the order the header introduces them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-28 — gate the notification on the predicate the SDK asserts (#1797)
The roots/list_changed guard read `capabilities.roots !== undefined`, but
the SDK asserts `capabilities.roots.listChanged` for the notification —
wider than the flag. Identical today (roots are only ever advertised as
`{ listChanged: true }`), but a conditional advertisement would un-gate it
silently, which is the round-21 shape again. Split into
rootsListChangedCapabilityAdvertised, mirroring
urlElicitationCapabilityAdvertised, which already reads its assertion's
sub-field; rootsCapabilityAdvertised keeps the roots/list registration,
where presence is what setRequestHandler checks.
Also moved the new test out of the gate pair it had split — it is a
runtime-emission case, not a registration-gate one — and extended the
header's converse-category paragraph to cover notifications, which indexes
it without inventing a category. Reflowed the setRoots doc paragraph in one
pass (111-char line from the last commit, plus a ragged orphan from an
earlier edit).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-29 — chase the corrected premise into clients/cli (#1797)
Round 27 corrected "setRoots announces roots/list_changed to a server that
then gets -32601" — the SDK refuses the notification from a client that
never declared it, so nothing reached the wire. That reword happened in
core/, but the same premise had been written into the two comments
justifying the CLI's roots seed, and they still asserted it. Both now state
the real pre-fix failure: a server asking for roots on its own got -32601,
and roots/set could not announce at all.
Also fixed the warn message under the round-28 gate: it named the roots
capability while the gate is on roots.listChanged — precisely the pair that
split was made to distinguish, so a client advertising `{}` would have been
told it advertised nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): reject in-flight raw-wire requests on a mid-session crash (#1797)
rejectPendingRawWireRequests had one caller — disconnect() — so the crash
path settled the inbound peer-request queue this PR added and left the
outbound raw-wire map alone. That map holds the modern tasks/* frames the
SDK's era gate refuses to route, so the SDK's chained _onclose doesn't know
about them: a Tasks-tab poll in flight when the server died waited out its
own 30s timeout and reported a timeout for what was a crash, on a
connection that had already dispatched `disconnect`.
Pre-existing — both the map and the crash path predate this PR. Fixing it
here because the invariant it violates is one this PR wrote down: "every
path that ends a connection has to clear that queue, announce it, and
settle each entry", in clearAndAnnouncePendingPeerRequests' doc and the
timing test's header. One channel was true of one path out of three.
Test starts a raw-wire request and fires onclose; without the call it
hangs to the vitest timeout, which is the symptom.
Checked the two siblings and left both: taskInputAbortControllers is
already unblocked via the elicitation cancel the crash path performs, and
activeToolCallAbortController rides an ordinary client.request the SDK
settles itself.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-31 test fixes — justify the cast, drop the no-op tick (#1797)
- The new test's `as unknown as` had no justification, which AGENTS.md
explicitly prohibits. It is unavoidable — rawWireRequest is private and
every public route is gated on isTasksExtensionNegotiated(), which needs
a modern handshake the fixture doesn't perform — so that reasoning is now
written down instead of left to be re-derived.
- `await Promise.resolve()` was a no-op: rawWireRequest registers its entry
in the promise executor, synchronously, before any suspension. Dropped,
which also makes the test pin that ordering rather than paper over it —
and stops the third recurrence of the microtask-counting pattern this
file's own helper doc argues against.
- Moved the test into the teardown group and extended the header's teardown
paragraph to cover the outbound direction, which it framed entirely as
inbound.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-32 — separate the outbound teardown claim from the inbound one (#1797)
My round-31 header edit spliced the outbound raw-wire passage into the
middle of the inbound claim, so "There are three: a failed connect(), a
mid-session transport close, and an explicit disconnect()" ended up reading
as scoping the outbound settle — which is two by design, since nothing
populates the map before the handshake. Round 31 established that scoping
so the code wouldn't imply an impossible state; the prose then un-scoped
it. Split into its own paragraph with its own scope clause, and reflowed
both in one pass rather than nudging breaks.
Also raced the raw-wire assertion, for the reason the file's own wait
helper documents: without the teardown rejection the test waited out the
request timeout and reported a bare vitest timeout. It now fails with
"raw-wire request not settled by teardown" — verified.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-33 — extract the timeout race, mark the failure-path promise handled (#1797)
Three copies of "reject after 1s so a regression fails where it happened"
had accumulated, each re-explaining itself. Extracted `withTimeout`, used
by the enqueue wait and the raw-wire assertion; injectRequest keeps its own
since its timeout also deletes the waiter entry.
On the failing path the raw-wire test left the production 30s timer armed,
so a regressed run got the intended failure plus a stray unhandled
rejection up to 30s later — the armed-timer class the file's own comment
names, on the failure path. `settled` is marked handled before the race.
Verified: with the fix removed the run fails with "raw-wire request not
settled by teardown" and no unhandled rejection.
Recorded why disconnect() drains the SDK's in-flight requests but rejects
the raw-wire map outright: the user asked to disconnect, and a Tasks poll
is not worth delaying that for. Pre-existing and deliberate, now written
down where the asymmetry is visible.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-34 — drop a no-op guard and the false mechanism it asserted (#1797)
Round 33 claimed the raw-wire test could leave `settled` rejecting
unhandled on a regression run, and round 33's fix encoded that as
`void settled.catch(() => {})` with a comment explaining it. Both were
wrong: `Promise.race` attaches a rejection handler to every input, so
`withTimeout(settled, …)` on the next line already marks it handled.
Verified with a standalone script — no unhandledRejection fires, and the
pre-fix version was equally safe.
Removed the line and the comment. The comment mattered more than the line:
it told the next reader that `Promise.race` does not mark its losers
handled, which is false and would cost them elsewhere.
Kept the true half by setting `timeout: 50` on that test's client, so a
regression leaves no 30s timer armed and fails with `Raw request
"tasks/get" timed out after 50 ms` — more self-describing than the raced
generic message. Verified.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-35 — the drain comment described a trade-off nobody made (#1797)
Last round's comment said raw-wire requests "get no share of the drain
above: the user asked to disconnect, and a Tasks poll is not worth delaying
that for" — framing a decision. Verified it isn't one: `safeDisconnectTimeout`
defaults to 0 and the only caller anywhere that passes a value is a single
test, so nothing is drained in production; and the drain polls the SDK's
`_responseHandlers`, which never holds raw-wire ids, so those were never
eligible regardless.
Reworded to what is true. The framing came from my own round-33 reply,
which described the drain as running on every disconnect — a comment
faithfully encoding a half-read mechanism, which is the same failure as the
`Promise.race` one it followed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-36 — the race comment outlived the mechanism its own commit replaced (#1797)
`timeout: 50` made the request settle in ~50ms, so the 1s race can never
win and a regression fails on the assertion (`timed out after 50 ms` vs the
expected /Connection closed/) — which the commit message said, while the
comment beside it still credited the race. Reworded: the short timeout is
what fails fast and names itself, the race is the backstop for a promise
that never settles at all. Race kept; it covers the case where the
request's own timer is broken too.
Third comment in three rounds encoding a mechanism the same commit changed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-37 — scope the timeout comment, align the bypass guards (#1797)
`timeout: 50` is the client-wide request timeout, not a bound on the
raw-wire request; it is inert in that test only because the test issues no
SDK request after connect. Noted, so someone adding one later knows it
would inherit a 50ms budget rather than reading the comment and looking
elsewhere.
The two installReceiverTaskResponseBypass guards read `this.receiverTasks`
while the five gates around them read the advertisement. Installing the
bypass mutates the Protocol's _requestHandlers map, so it is registration —
same as the setRequestHandler it follows. Equivalent today; aligned so the
convention is uniform rather than something the next reader has to
re-derive per site. The genuinely-runtime `receiverTasks` reads are left.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-38 — one predicate for the whole bypass mechanism (#1797)
Round 37 moved the bypass *install* guard to the advertisement and left the
three branches it controls on the raw option, splitting one immutable fact
across two sources. The dead direction is the harmful one: if the tasks
advertisement ever acquires a condition — the exact edit that produced the
elicitation wedge — then advertised goes false while receiverTasks stays
true, the bypass is not installed, and the handler still returns
`{ task }` into the SDK's validating wrapper, which rejects it. That is the
breakage the bypass exists to prevent, and before round 37 it could not
happen because install and branch read the same field.
All three now read tasksCapabilityAdvertised. `receiverTasks` is left where
it belongs: assigned once, and driving the advertisement.
Also noted that the wrapper's own check is redundant with its install guard
and kept deliberately, since it must agree with the handler branch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): don't carry receiver tasks into the next session (#1797)
Round-39 review found the last member of the enumeration rounds 12-14 and
31 built: receiverTaskRecords was cleared only by disconnect(). The crash
path doesn't clear it, the connect() failure catch doesn't, and connect()
never reset it — so a record created in one session survived into the next
connect() on the same instance, and `tasks/list` is answered from that map.
A new server would be told about a task it never created.
Reachable in web (the only client with receiverTasks): a task-augmented
sampling/createMessage arriving in the handshake window creates a record;
if that connect then fails with a 401, App.tsx retries connect() on the
same instance.
Cleared at the top of connect() rather than only on the crash path — the
auth-recovery retry means ending a session isn't the only way a new one
begins, and starting clean covers every route in. disconnect() keeps its
clear via the same extracted helper.
Pre-existing — the crash path never cleared these — but the handshake
window this PR opened is what lets a receiver task exist before connect
resolves, and this was the one member left outside the invariant the PR
documented. Test verified load-bearing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): reset all session-scoped state on connect, not just receiver tasks (#1797)
Round-40 review: receiverTaskRecords was one of five collections cleared
only by disconnect(). Two of the other four have real symptoms on the same
crash-then-reconnect path:
- subscribedResources — the modern subscribeToResource early-returns on a
uri already in the set, so after a reconnect the user's Subscribe click
silently sends nothing to the new server while the UI keeps listing it.
- cancelledTaskIds — a leftover id mislabels a *new* task sharing it as
cancelled rather than failed, and server task ids are often short.
taskInputAbortControllers leaked un-aborted controllers.
modernLogLevel had the opposite asymmetry: disconnect() cleared it so it
couldn't leak (#1629), but nothing restored it, so a reconnect after an
explicit disconnect silently lost the user's configured level while a
reconnect after a crash kept a mid-session override. It is now recomputed
from serverSettings the way the constructor does, so both routes agree.
clearReceiverTasks became resetSessionState, called from the same place.
Tests cover the two symptomatic siblings and now assert receiver tasks
through the `tasks/list` handler — what a server actually sees, and the
symptom itself — which also removed the unjustified `as unknown as` the
previous commit added. The remaining cast is justified in place, since
cancelledTaskIds has no public reader.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(web): re-seed the Logs control when the client re-seeds its own level (#1797)
Round-41 review caught a consequence of the last commit. `resetSessionState`
restores the client's modern log level from the server setting at connect;
`resetSessionScopedUiState` still blanked the web control to null on every
disconnect. On the one path that reconnects the *same* instance —
auth recovery — the control then read Off while every modern request carried
`_meta` logLevel "debug", the default. A tool whose job is showing what you
send should not disagree with itself about what it sent.
The control now re-seeds from the active server's settings exactly as
setupClientForServer does, instead of blanking.
Also from review:
- Added the tests the restore never had, covering both halves: a mid-session
override does not survive a reconnect, and a disconnect no longer drops the
configured level (the #1629 half, and the actual behaviour change).
- resetSessionState's doc claimed "every piece of state scoped to one
connection", which invites assuming membership rather than checking it.
Now states the rule: state a new session would misread; what only needs
settling on the way out stays with the teardown paths.
- Header: session-scoping paragraph moved below the outbound one to match
test order, and it now names all five members rather than three.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* refactor(core): one derivation for the modern log level (#1797)
Round-42 review: the configured level was computed independently in four
places — twice in InspectorClient (constructor, resetSessionState) and twice
in App.tsx (the seed and the re-seed). The client and the Logs control
agreed only because four sites happened to compute the same expression, and
any drift reproduces exactly the symptom the previous commit fixed: the
control reading one level while every modern request stamps another.
Extracted resolveModernLogLevel into core/mcp/types.ts beside
DEFAULT_MODERN_LOG_LEVEL; all four call sites use it, with the web mapping
undefined→null at its own boundary (display, not a second derivation).
Also from review:
- disconnect()'s comment still credited itself with preventing the #1629
leak, which resetSessionState now prevents. It says what the line does
now — read "not opted in" while disconnected — and notes the web control
deliberately shows the configured level in that window.
- resetSessionState's doc claimed an un-aborted taskInputAbortControllers
entry "leaks a paused poll loop". Both registration sites release in a
finally, so nothing leaks permanently; the abort closes the window
between a crash and the unwind. Softened, and it now has the test it
lacked — the only member of the five without one.
- Noted in App.tsx that the re-seed depends on activeServerIdRef still
holding the outgoing id (it syncs in a passive effect), so an eager clear
would silently drop it to the default.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): reset the subscription stream state with the set it derives from (#1797)
Round-43 review: resetSessionState cleared subscribedResources but left
modernStreamState, so a crash-then-reconnect on the same instance held an
empty set alongside `{ active: true }` — a combination the rest of the file
treats as impossible, since onModernSubscriptionClosed and
onModernReconnectFailed both compute `active` from `subscribedResources.size
> 0`, and the comment at :4712 asserts a terminal drop clears both. No
symptom today only because the UI mirror resets itself independently, in
another file — which is the arrangement this PR has spent its rounds
removing. Clearing it here also announces the reset; that set's every other
mutation dispatches.
Also from review:
- The web re-seed treated "no server found" as the default level rather
than Off, because resolveModernLogLevel returns the default for absent
*settings*. Reachable via a second disconnect after a crash, and via
removing the active server. Now distinguishes the two.
- Converged the two injectRequest copies: withTimeout takes an optional
onTimeout, so both drop their waiter on the timeout path. The newer copy
had lost exactly the cleanup that was round 33's reason for leaving the
original alone.
Test seeds the stream state a live subscription would have left; verified
load-bearing (the earlier version passed either way, since the fixture
never activates a stream).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(web,core): correct the re-seed branch; share the subscription-stream reset (#1797)
Round-44 review. My round-43 fix traded one wrong branch for another: it
read `?.settings`, which is undefined both when no server matched *and*
when the server has no settings node — and the second is the common case
(a hand-written mcp.json entry never opened in Server Settings). Those
servers showed Debug while connected and Off after disconnect, which is
exactly the client/UI disagreement rounds 41-42 removed, with no
modernLogLevelChange event to reconcile it. Now branches on the server.
Worth noting no test would have caught it: App.tsx is a documented
exclusion from the coverage gate.
Also from review:
- resetSessionState cleared subscribedResources without announcing it,
two lines below a comment arguing that every other mutation of that set
dispatches. And it took two statements of a six-statement cluster
disconnect() clears together — leaving modernReconnectAttempts (a new
session's first drop would back off as if it were the old one's nth),
modernListenGeneration (the bump that makes an in-flight re-listen bail),
and a stale modernSubscription. One resetSubscriptionStream helper now
serves both, announcing both axes; disconnect() keeps only the live
stream's best-effort close. That also removed the file's one assignment
to modernStreamState that bypassed its setter.
- The stream-state test pinned the value, not the dispatch — the half the
UI actually tracks. Now asserts both; verified against a bare field
assignment.
- onModernSubscriptionClosed's comment claimed the disconnect reset always
clears the set; on a crash it doesn't until the next connect. Says so.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-45 — reattach the orphaned doc, order the announcements (#1797)
Extracting resetSubscriptionStream last round inserted it *and its doc*
between resetSessionState's doc and resetSessionState — so two doc blocks
stacked, the new helper took the nearest, and the 23-line block explaining
each member's symptom attached to nothing. Dead text where it sat, and the
most load-bearing comment in the batch. Moved, not reworded.
That is round 32's finding in production code, from the same cause:
inserting anything between a doc block and its method is a move that needs
checking, prose or code.
Also from review:
- resetSubscriptionStream announced the cleared set before updating the
stream state derived from it, so a listener in between saw an empty set
with an `active` stream — the combination the helper exists to prevent,
exposed by its own dispatches. Both fields move first, then both are
announced. The onModernSubscriptionClosed comment now says that window
is never observable, which is newly true.
- The test header called the log level "cleared"; it is re-seeded, which is
the #1629 half of the test twenty lines below. Says "reset", with the
distinction spelled out, and the paragraph reflowed in one pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): close the listen stream the connect-path reset drops (#1797)
Round-46 review: resetSubscriptionStream's doc claimed disconnect() is the
only caller with a stream worth closing, and the helper did a bare
`modernSubscription = null`. Not guaranteed on the connect path — an
`onerror` without an `onclose` leaves the transport up, connect() reuses
it, and the reset then drops the last reference to a stream still open on
the server. Nothing can close it afterwards: the `closed` handler bails on
the bumped generation, and `closed` never resolves for a live stream.
Before the extraction the surviving reference was eventually closed by
refreshModernSubscription as `previous`, so this was newly unclosable.
The helper now closes it itself, best-effort, after the dispatches so the
round-45 ordering is unaffected; disconnect() drops its two hand-rolled
lines.
Also from review:
- The session test pinned the subscription set's value but not its
dispatch — the half the UI tracks, and the gap round 43 closed for the
stream axis one line away. Both now asserted; verified load-bearing.
- resetSessionState's doc said disconnect() "clears all of this", which is
true by duplication for two of the five members; it now says a sixth has
to be added in both places. And "the teardown paths" handle the in-flight
tool call only on the disconnect route — on a crash it is callTool's own
finally. Named.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-47 — fix the doc's count, catch the closed-promise chain (#1797)
- resetSessionState's doc said "two members through the same helpers, the
other two hand-rolled" for a body with five. It is two and three — and
the member the enumeration dropped is the one that is *paired* rather
than duplicated: modernLogLevel is re-derived here and blanked in
disconnect(), deliberately. Both corrected.
- `subscription.closed.then(...)` had no rejection handler, unlike every
other promise around that stream. Newly reachable: before the last
commit a connect-path reset dropped the reference with `closed`
permanently pending, so the chain never settled at all; closing the
stream is what makes it settle. Added, with the Node-terminates reasoning
the file already applies to payloadPromise.
- The listen-stream test exercised transport reuse without asserting it —
the half that makes "still open on the server" true. Counts factory calls
now, so the premise fails loudly if a future change recreates the
transport rather than silently ceasing to apply.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-48 — scope the catch, guard the close, index the test (#1797)
- The .catch comment claimed the connect-path close made a *rejecting*
`closed` reachable. It doesn't: closing resolves it. What that close newly
reaches is the handler running at all, where the reference used to be
dropped with `closed` pending forever. And chaining after `.then` also
swallowed handler throws — silently abandoning a re-listen on the path
whose job is recovering a dropped stream. Now the second argument to
`.then`, scoped to the rejection, with the comment saying which half is
which.
- `closing?.close()` reaches into third-party code from `disconnect()`'s
straight-line teardown, none of which is inside a `try` — a synchronous
throw would skip the raw-wire rejects, the task aborts, the receiver-task
clear and nine dispatches, silently, since most callers catch a
disconnect(). Best-effort now against both failure modes.
- Header indexes the listen-stream test: same category, but a release
obligation rather than a misread one.
- Its comment described the generation guard, which the stub can't exercise;
marked as production behaviour.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-49 — pin the best-effort close, unorphan the doc (#1797)
Round 48 made `resetSubscriptionStream`'s close best-effort against both a
rejected promise and a synchronous throw, but nothing exercised either arm:
both `modernSubscription` stubs in the tree resolve. On the `disconnect()`
path an escaping failure would silently skip the raw-wire rejects, the paused
task-input aborts, the tool-call abort and `clearReceiverTasks()` — the
findings rounds 13 and 30 closed, re-opened at once with a green suite. Two
table-driven cases now seed a `close()` that throws and one that rejects,
and assert teardown continued past it.
Also: the `.then` comment gave opposite verdicts on one mechanism (an
unhandled rejection is named unacceptable, then chosen for a handler throw) —
reworded to say the handler cannot throw today and why the shape is
defensive; and `refreshModernSubscription`'s doc block, orphaned two methods
up since before this branch, moved onto the method it describes.
Closes #1797
* review: round-50 — sweep the queues start-clean, unify the close (#1797)
`resetSessionState`'s doc excluded the peer-request queues and the raw-wire
map because "the teardown paths handle them" — true of the paths that run.
An `onerror` without an `onclose` runs none of them: it flips status to
"error" and leaves `baseTransport` cached, so a `connect()` on the same
instance reuses a live transport. That is the route the round-46 stream close
was built on, and it strands these two the same way — the web pending-request
modal is derived from the queue length with no status gate, so it outlives
the session, and answering it writes a response for the old request id onto
the new connection. Both are now swept start-clean beside the reset (both
helpers are idempotent, so the routes that already ran them are unaffected),
the end-clean clears stay put for the `disconnect`-event consumer, and both
docs say which is which.
Also folds the three subscription `close()` sites onto one
`closeSubscriptionBestEffort` helper, so all three absorb a synchronous throw
as well as a rejection — at `refreshModernSubscription`'s first site the
reference is already dropped, so a throw there both leaks a live stream and
aborts the refresh before its replacement is opened.
Closes #1797
* review: round-51 — pin the sweep's ordering, name the category (#1797)
The connect-start sweep must sit after `resetSessionState()`, and the two
calls read as independent. Cancelling a task-augmented peer request settles
it synchronously into the record callback, which ends in `upsertReceiverTask`
— a no-op only because `clearReceiverTasks()` just emptied the map. Hoisted,
it would emit a `notifications/tasks/status` for the outgoing session's task
onto the transport the connect is about to reuse. Stated at the call and
pinned by a test (which fails on the hoist).
Adding that third caller also made five docs stale, all describing the old
set by count or as "teardown" — the class round 23 diagnosed. All five now
name the category: every route out, plus the top of …
1 participant
Closes #1795
What & why
The web server bound whatever
HOSTenv you gave it, unvalidated.HOST=0.0.0.0(or::, or empty) binds every network interface, exposing a backend that can spawn local processes and reach MCP servers to the whole network — the surface DNS-rebinding attacks target. The "never 0.0.0.0" rule existed only as a note inspecification/v2_scope.md; nothing enforced it.This PR enshrines it in code.
Host-binding guard (the escape hatch)
clients/web/server/resolve-bind-host.ts) + detection (core/node/hostUrl.tsisAllInterfacesHost), used by both bind points (web-server-config.tsprod backend andvite.config.tsdev server) and, viastrictPort, the launcher's in-process dev server too.DANGEROUSLY_BIND_ALL_INTERFACES=true(explicittrue/1, so=falsereads as off) — sameDANGEROUSLY_*pattern asDANGEROUSLY_OMIT_AUTH.Dockerfilesets the flag (a container must bind0.0.0.0to be reachable via-p); that's the one sanctioned exception.Loopback origin fix (the bug that started this)
Connecting to a plain stdio server (
everything-server) failed withRemote connect failed (403): Invalid origin … DNS rebinding. Root cause: the dev server binds IPv6 loopback[::1], the browser lands onhttp://[::1]:6274, but the defaultallowedOriginswas the single literalhttp://localhost:6274, so the origin guard 403'd the connect. (The server list still rendered because same-origin GETs carry noOriginheader — only the connect POST does.)The default loopback origin list now expands to all three interchangeable forms —
http://localhost:PORT,http://127.0.0.1:PORT,http://[::1]:PORT— keeping the DNS-rebinding guard effective (still loopback-only, that exact port) while not rejecting the IPv6 alias of the same machine.Security holes closed (during review)
This PR went through an extended adversarial review. The load-bearing principle that emerged: the guard reasons about the address the socket binds, not the raw string — one shared, canonicalized value drives detection,
listen(), the origin allow-list, the banner, the sandbox URL, and the CLI deep-link. Notable holes closed:0.0.0.0/::. The naïve guard only caught the literal0.0.0.0. Now refused (each verified against a realhttp.listen()):inet_atonforms:0,0x0,0.0,0.0.0, and octal/hex (00000000000,0x00000000,0x0.0.0.0,000.000.000.000).::0,0::0,::0.0.0.0, fully-expanded zeros; the IPv4-mapped wildcard::ffff:0.0.0.0/::ffff:0:0; the zone-scoped::%eth0; bracketed[::]; empty/whitespace.HOST="０"(U+FF10),"０0.0.0","0．0．0．0"(fullwidth dots). Node's resolver maps these to0.0.0.0before binding, so a raw-string check missed them and the server bound every interface silently. The guard now canonicalizes throughnew URL()(which applies the same IDNA mapping) first. Verified withdns.lookup+http.listen()on both Linux and macOS.ALLOWED_ORIGINSfail-open. A blank / whitespace /,-only value parsed to[], which the origin middleware treats as allow-all — silently disabling the DNS-rebinding guard. Now falls back to the default (fail closed); there is no env knob to turn origin validation off.ALLOWED_ORIGINSentries. A scheme-less entry (localhost:6274) parses to the literal origin"null"— which is a realOrigin: nullheader browsers send from opaque origins (sandboxed iframes,data:docs), so allow-listing it eroded the guard; now dropped with a warning. A*.example.comentry worked for the sandbox CSP but silently 403'd every connect (the most confusing split); now dropped.HOST=fe80::1%eth0(a valid link-local) threw an opaqueTypeErrorout of the guard;net.isIPv6accepts a zone id butnew URL()rejects it. The zone is now stripped for detection and kept forlisten().frame-ancestorsgrounded in a real browser, not the grammar. A bracketed IPv6 literal is not a valid CSP host-source — verified in headless Chromium that a lonehttp://[::1]:*collapses the directive to'none'and blocks the sandbox iframe. It's excluded from the derivedframe-ancestors, and the MCP Apps IPv6-literal limitation is documented.--callback-url http://0.0.0.0:6276/oauth/callback(orMCP_OAUTH_CALLBACK_URL) put the listener that receives the OAuth authorization code on every interface — a credential-interception risk. It now goes through the sameisAllInterfacesHostguard and refuses a non-loopback callback host.Alongside these, the review tightened correctness:
banner ⊆ allowedOriginsholds by construction (verified as a property),CLIENT_PORT/MCP_SANDBOX_PORTreject non-integer/out-of-range values,strictPortholds on all three bind points, and origin canonicalization matches what browsers actually send (trailing slash, uppercase host, default:80, IPv4-mapped, IDNA).Tests
hostUrl.test.ts(host normalization + theisAllInterfacesHostwildcard sweep — decimal/octal/hex/IPv6/mapped/zone/IDNA spellings, positive and negative),resolve-bind-host.test.ts(the guard end-to-end: refuse-without-opt-in incl. fullwidth, allow-with-opt-in),web-server-config.test.ts(origin allow-list, banner,ALLOWED_ORIGINScanonicalization + fail-closed,CLIENT_PORTvalidation),sandbox-controller.test.ts(derived CSP + IPv6-drop, port validation), and CLI handoff tests (deep-link canonicalization + wildcard→localhost).npm run coverage(web and CLI) passes — every touched file clears the ≥90 per-file gate;core/node/hostUrl.tsis at 100%.Docs
Root README (
core/node/entry, Docker section),clients/web/README.md("Host binding & the origin allow-list" + "Hosting on a network" + MCP Apps caveats),AGENTS.md(structure entries forresolve-bind-host.tsandcore/node/hostUrl.ts),docs/mcp-app-review.md(loopback/Apps guidance), and theDockerfilecomment.🤖 Generated with Claude Code
https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5