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 |
…red (#1795) Address the round-24 review of #1796 (all on the newest guard's error-reporting): - AJ1: the OAuth callback guard's message contains "OAuth", so the CLI exit-code heuristic classified it as AUTH_REQUIRED (exit 3) — telling an automated caller to re-auth on a config error. And parseRunnerOAuthCallbackUrl runs for EVERY --cli invocation, so an exported MCP_OAUTH_CALLBACK_URL=http://0.0.0.0/… made every call exit 3. Wrapped the call in cli.ts and rethrow as CliExitCodeError(USAGE) → exit 1, code "error". Test asserts exit 1 / not auth_required. - AJ2: the TUI bin (clients/tui/index.ts) and the launcher bin (clients/launcher/src/index.ts) printed a config-error Error as a raw stack trace; now print err.message like run-web.ts's house pattern, so the guard's actionable message isn't buried. - AJ3: AGENTS.md + root README name isLoopbackHost in the core/node/hostUrl entry. - AJ4a: CSP_HOST_SOURCE now excludes `*` too, matching its "can't corrupt/widen" invariant. AJ4b: pin isLoopbackHost's canonical ::ffff:7f00:1 serialization. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Round-24 addressed in AJ1 — OAuth guard error misclassified as AJ2 — TUI/launcher printed the guard message as a stack trace (low) — Fixed, the round-1 item-3 pattern one client over. AJ3 — AJ4a — AJ4b — test pinned AJ4c — Green locally: web + CLI + launcher |
|
@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
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