Copilot review round 2 on #2100, plus a collision the review did not find.
The default app-origin port was 6276 — which is already
RUNNER_OAUTH_CALLBACK_DEFAULT_PORT, the CLI/TUI loopback OAuth callback.
That port is fixed precisely so an OAuth app can pre-register
http://127.0.0.1:6276/oauth/callback, which makes it the one listener here
that cannot move when something takes it first. A running --web would hold
it and break a later --cli/--tui OAuth flow at its registered redirect URI,
and docs/mcp-app-review.md has those two running side by side. This
listener has a documented dynamic fallback, so this listener yields: the
default is now 6278 (6277 skipped — v1's retired proxy port, which the
migration guide tells people to stop forwarding). A test asserts the two
defaults stay distinct.
Review comments:
- POST /api/app-document now applies hono's bodyLimit BEFORE parsing. The
8MiB check ran after c.req.json() had buffered and parsed the whole
request, and bounded only `html` — so bulk in `csp`, or in keys the route
never reads, was fully materialized before the 413.
- `csp` is validated as an HTTP header value and length-bounded. The
controller hands it to res.writeHead() verbatim, where Node throws
ERR_INVALID_CHAR synchronously — inside the handler for a later,
unrelated request, outside this route's error handling.
- The app-origin port now propagates everywhere the other two do: the CLI
--print-handoff port-forward command (and its test), the Dockerfile's ENV
and EXPOSE, and the remote-review SSH recipe in docs/mcp-app-review.md.
Without it an app declaring _meta.ui.domain is unreachable through every
documented remote workflow.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
The copy said `progressToken` may be "an integer" while the implementation
validates with `ProgressTokenSchema` — zod's `.int()`, which rejects anything
past `Number.MAX_SAFE_INTEGER`. This repo's own tests assert that
`MAX_SAFE_INTEGER + 1` is dropped, so the documentation was promising a value
the client discards.
Corrected in all three places, so the code comment, the docs callout and the UI
hint agree: `dropInvalidReservedMeta`'s doc block (which review did not flag),
the `metadata` reserved-keys callout, and the Request Metadata hint.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
`progressToken` was not the only reserved key. The pinned SDK's
`RequestMetaSchema` also constrains `io.modelcontextprotocol/related-task` to
`{ taskId: string }`, and a conforming server rejects the *whole request* over
one bad reserved member rather than ignoring that key. Before #1910 every value
was a string, so only `progressToken` could be wrong; the widening made all of
them reachable.
Replace both hand-rolled checks with the SDK's schemas — `RequestMetaSchema`
for the payload, `ProgressTokenSchema` for the single value. The schema is
loose, so unknown keys pass through untouched, and zod reports the offending
top-level key as `issue.path[0]`, so only that member is dropped and its
siblings survive. A member the SDK constrains in a later release is then
covered without anyone remembering to add it — which is the drift that produced
the `isInteger`/`isSafeInteger` bug one round ago.
`name` was hard-coded, and react-ace uses it as the editor container's DOM id,
so two instances emitted duplicate ids. Derived from the per-instance id, and
deliberately distinct from the id given to the textarea — that one is what the
wrapper's `<label for>` targets, so reusing it would just move the collision.
The docs table, the docs prose and the form hint all claimed "any JSON" without
qualification, presenting a configuration as valid that the client then drops.
All three now name the reserved keys and say what happens to a bad one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
The field table said this payload is "merged into every outgoing request",
which is true of web and the TUI but not the CLI — it never passes the
persisted value to its client, so the setting is inert for `--cli` runs
(#2093). Scope the claim and point at the per-invocation `--metadata` flag as
the CLI workaround, so a CLI user isn't told a setting works when it doesn't.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Closes#1910
`_meta` was typed `Record<string, string>` end to end in the Inspector —
on disk as a `{ key, value }[]` pair array, in the Server Settings form as
key/value rows, and in every request verb's per-call metadata parameter.
Nothing in the MCP spec restricts a `_meta` value to a string, and the SDK
models the field as a `z.looseObject`, so the restriction was ours alone.
Widen the whole path to `RequestMetadata` (`Record<string, JsonValue>`):
- Per-server default metadata is a JSON **object** on disk and in memory.
The pre-#1910 pair array is still read (`normalizeStoredMetadata`) so an
existing `mcp.json` keeps working; it is never written back, so a save
migrates the entry.
- The Server Settings → Request Metadata section is a JSON editor
(`JsonObjectInput`) instead of key/value rows — rows cannot express an
object value. It keeps the typed text as the display source and emits
only parsed JSON, the same draft split `SchemaJsonField` uses, so the box
stays typeable; invalid text is flagged inline and not applied.
- The CLI stops flattening `--metadata` / `--tool-metadata` through
`metaValueToString`. Those values were already JSON-parsed, so
`trace={"id":"abc"}` used to reach the wire as the *string* `{"id":"abc"}`
and now reaches it as an object.
- `servers/show` redaction replaces a sensitive key's whole value rather
than a string, so a structured value cannot leak through its members.
- `progressTokenOf` narrows the reserved `progressToken` key back to
`string | number` before it reaches the SDK; any other JSON still travels
inside `_meta` but wires up no progress callback.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Two review findings on the fetch wrapper:
The content-type test was a substring match for "json", which also
matches `application/x-ndjson` — how the streamable-HTTP transport can
serve its long-lived server-push channel (`isLongLivedStreamResponse`).
Awaiting `.json()` on a clone of an unbounded stream never resolves, so
the MCP connection would hang for as long as an override was configured.
It now matches the media type exactly (`application/json` or a `+json`
suffix), with a regression test driven by a stream that never closes.
The overrides also leave `issuer` as discovery returned it, so pointing
one at an authorization server that advertises a different issuer is
rejected on callback by the RFC 9207 / SEP-2352 mix-up defense. That
check is deliberately not weakened; the limit is now stated on the
module, in the field description, and in the configuration guide.
Signed-off-by: cliffhall <cliff@futurescale.com>
The overrides are per-server configuration for the *resource* server's
authorization server, but the fetch wrapper that applies them is shared
with the enterprise-managed (EMA) flow, whose IdP OIDC discovery runs
through the same fetch and satisfies the metadata predicate. An
authorization override could therefore redirect the IdP login to the
resource AS, and a token override could send the IdP code or refresh
token there.
OAuthManager.getEndpointOverrides() now returns nothing when the server
is enterprise-managed — the same boundary redirectToExternalAuthorization
draws for the custom authorization parameters (#2018) — with a regression
test, the note surfaced on both form fields, and the rule documented.
Signed-off-by: cliffhall <cliff@futurescale.com>
Adds two optional per-server fields — `oauth.authorizationUrl` and
`oauth.tokenUrl` — that override the endpoints authorization-server
metadata discovery resolved, so a server can be pointed at a development
or staging authorization server without changing what it advertises.
SDK v2 routes both endpoints through the discovered metadata document and
neither through OAuthClientProvider, so the override is applied by
wrapping the client's base fetch and patching the metadata document in
flight (core/auth/endpointOverrides.ts). Wrapping the base fetch — rather
than only the auth fetch — also covers the discovery the SDK runs from
inside the transport on the 401/refresh path.
The fields round-trip through mcp.json, the /api/servers routes, the
runner (CLI/TUI), and the Server Settings → Authorization panel, where a
value that is not an absolute http(s) URL is flagged inline; at runtime
such a value is dropped with a single warning rather than failing the
connection.
Signed-off-by: cliffhall <cliff@futurescale.com>
Real deployments gate IdP selection, prompt behavior, or audience on a query
parameter the OAuth/OIDC core specs never standardized (Keycloak's
`kc_idp_hint`, OIDC's `login_hint` / `prompt` / `acr_values`, Auth0's
`audience`), and none of them could be driven from the Inspector.
The Inspector never builds the authorize URL — the SDK's `auth()` does, and it
exposes no hook for extra parameters — so the merge happens at the one seam
that sees the finished URL: `BaseOAuthClientProvider.redirectToAuthorization`.
It lives in `core/`, so web, CLI, and TUI all inherit it.
- `core/auth/authorizationParams.ts`: the reserved-key list (`client_id`,
`code_challenge`, `code_challenge_method`, `redirect_uri`, `resource`,
`response_type`, `scope`, `state`), the key validator the form renders
inline, and the merge itself. Reserved keys are dropped with a warning
rather than silently overriding PKCE / the state binding / RFC 8707;
blank-key rows are skipped; the caller's URL is never mutated.
- Applied to the authorization request only — nothing wires it into the token
request.
- Persisted as `oauth.authorizationParams` (a string record) on the per-server
config beside `scopes`, edited as key/value rows in Server Settings ->
Authorization ("Additional authorization parameters"), with an inline error
per offending row plus a section-level warning.
- The enterprise-managed (EMA) leg redirects to a *different* authorization
server (the enterprise IdP), so it goes through the explicit
`redirectToExternalAuthorization` entry point, which skips the merge — a
`kc_idp_hint` or `audience` meant for the MCP server's AS must never be
appended to the IdP's authorize URL.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E
Signed-off-by: cliffhall <cliff@futurescale.com>
- The `retriedDynamic` latch test never made the retry fail: a real `listen(0)`
always succeeds, so it produced one EADDRINUSE and passed with the latch
deleted. Mock `node:http` so every listen fails, asserting exactly two listen
attempts and a settled result. Verified by mutation: removing the latch now
hangs the test instead of passing it.
- The migration guide's Docker notes still described a dynamic sandbox port and
told readers to pin a custom one, contradicting the new fixed 6275 two
sections later.
- Narrowed "or any container" in the troubleshooting entry, which overstated the
scope and contradicted the PR's own scope note. It now names the IPv4-forwarder
case and explicitly excludes the official image and a plain loopback -p.
Signed-off-by: cliffhall <cliff@futurescale.com>
Two defaults that each break the Inspector inside a container.
#1951 — the web UI never loads in a VS Code dev container. `HOST` defaulted
to the NAME `localhost`, and `server.listen(port, host)` resolves a name
through `dns.lookup` and binds the SINGLE address it gets back. Since Node 17
the default result order is `verbatim`, so on a glibc Linux host — whose
/etc/hosts maps localhost to both 127.0.0.1 and ::1 — that is `::1`. The
server bound IPv6 loopback only and refused every IPv4 client, including the
dev container port forwarder. It presents as an endless spinner rather than a
connection error because the forwarder accepts on the host side before failing
to connect inward. Default to the ADDRESS 127.0.0.1 instead; an explicit
HOST=localhost is still honored as typed.
#2008 — the Apps tab is unreachable in any container. The MCP Apps sandbox
listened on an OS-assigned port, so it changed every run and could never be
named in a devcontainer `forwardPorts`, a `docker run -p`, or a tunnel. Default
it to a fixed 6275, mirroring CLIENT_PORT's fixed 6274, and retry once on an
OS-assigned port (loudly) if it is taken — the collision a dynamic default
never had.
Verified in the reporter's exact image
(mcr.microsoft.com/devcontainers/typescript-node:4-24-trixie): both listeners
come up on 127.0.0.1 and answer 200 on 6274 and 6275/sandbox.
Also EXPOSE 6275 and set MCP_SANDBOX_PORT in the image, and document both
defaults in the web README, the migration guide, and the root README's Docker
section.
Signed-off-by: cliffhall <cliff@futurescale.com>
Through v1 the Inspector was a follow-along project — the spec moved, we
chased it, and whatever planning capacity was left went to keeping up rather
than to the tool's own design. v2 meeting the 2026-07-28 spec across all three
clients lifts that constraint, but nothing written down said so, and there was
no shared list of what the MCP roadmap is likely to demand of us next.
Add docs/inspector-roadmap-2026-h2.md covering 2026-08-11 → 2027-02-11 (~26
weekly milestones, v2.2.0 → ~v2.27.0), in two explicitly-budgeted tracks.
Track A predicts Inspector features per MCP roadmap theme and WG charter,
each tagged build-now / design-now-build-on-signal / watch, so a speculative
item gets a liaison rather than code: transports and session lifecycle, Server
Cards (SEP-2127), Tasks retry/expiry and the extension→core migration,
enterprise audit + ID-JAG, triggers/events, result types, interceptors
(SEP-1763), file inputs (SEP-2356), skills (SEP-2640), primitive grouping,
and conformance.
Track B is the work we choose, headlined by the zoomable protocol/network
timeline — lanes, spans rather than points, brush-to-filter, MRTR and task
grouping — then session record/replay, a shared diff primitive, a command
palette, saved-call collections feeding CI assertions, the argument-editor
workstream, and Connection Doctor.
The sequencing argument is that several Track B items are force multipliers
for Track A: each protocol feature arrives with a rendering problem, and one
general timeline plus one general diff is cheaper than a bespoke panel per
SEP. So the general surfaces come first and the SEP work renders into them.
Two consolidations the survey surfaced, both worth acting on regardless of
whether this plan is adopted as written:
- #1853, #1856, #1885, #1928, #1919 and #1910 are one defect class, not six
bugs — the argument editor is not schema-aware. Treating them separately
has already produced one regression from v1.x (#1928).
- #962, #1936, #1951, #1944 and #1914 are one missing feature: an ordered
diagnostic that reports which connection step failed and what to do.
Sourcing is the published roadmap plus the nine WG and six IG charters, cited
in full at the end; the doc says so, and flags that Track A should be revised
if internal planning carries themes the public page omits. Status is "Draft
for WG review" — merging this starts the discussion rather than settling it.
Adds one file and touches nothing else. The README guide list and the
AGENTS.md docs/ tree entry are deliberately left for a follow-up rather than
bundled here, to keep this diff to the document itself. Markdown-only, and
outside every format and typecheck glob (SOURCE_EXTENSIONS has no "md"), so
no gate is affected.
Closes#1980
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013CaLPZQzfWDHoPxyL1vbYL
Signed-off-by: cliffhall <cliff@futurescale.com>
- The v1 recipe published on 127.0.0.1 and the v2 one silently dropped
that. Docker's -p binds host interfaces independently of the
container's HOST, and a no-Origin request skips the allow-list
(core/mcp/remote/node/server.ts:241), so the token is the only guard on
a process-spawning backend. Restore the loopback prefix on both
recipes and say why.
- MCP_PROXY_AUTH_TOKEN is not removed: web-server-config.ts:329 still
falls back to it. Call it a deprecated fallback rather than a rename,
and distinguish it from the query param, which has no fallback
(App.tsx getAuthToken reads only the new name).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SFXDQBEjvjCEBhmYkmw79a
- The "nothing combines with an ad-hoc target" rule has a web exception:
run-web.ts:130 exempts `--transport stdio`, so web ignores it rather
than rejecting. Only cli/tui reject every `--transport`.
- mcp-server-configuration.md still said a leading-dash argument cannot
reach a stdio server under --cli, the opposite of what the migration
guide now says. The parser puts every pre-`--` token in targetArgs, so
the guide is right; corrected the older section to match.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SFXDQBEjvjCEBhmYkmw79a
- `--` under --cli: everything before the separator IS forwarded to the
target, flags included (cli.ts:536-541), so "no way to pass a
leading-dash argument" was wrong. Show the correct placement instead
of sending users to a catalog file.
- Stdout: v2's emitResult appends "\n" where v1's awaitableLog wrote the
JSON with no terminator, so the output is not byte-identical.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SFXDQBEjvjCEBhmYkmw79a
- Sandbox port: the Apps tab needs a second, dynamic listener, so
"no second port" was wrong for Docker/remote deployments. Qualify the
architecture section, add a pinned-port Docker recipe, and add a
troubleshooting entry.
- Node floor: npm only warns EBADENGINE unless engine-strict is set, so
npx does not refuse to run on an older Node.
- v1 default list: the web UI persisted its list in localStorage; only
the on-disk default catalog is new.
- The v1→v2 --server example needs --cli (and a --method) to select an
entry; note there is no one-entry web equivalent.
- New behavior change #4: an ambiguous URL path (neither /mcp nor /sse)
now errors instead of falling back to SSE.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SFXDQBEjvjCEBhmYkmw79a
Closes#1822
Adds docs/v1-to-v2-migration.md — the map for users arriving from v1:
- CLI flag mapping (every v1 flag survives; lists what v2 adds)
- `--config` vs `--catalog` semantics with before/after examples
- Node engine bump (>=22.7.5 → >=22.19.0)
- what no longer ships (the three sub-packages; v2 is one tarball)
- the three behavior changes most likely to bite: exit code 5 on a
failing tool call, target-must-come-first under --cli, and the
reversed `--` separator
- env-var mapping (MCP_PROXY_AUTH_TOKEN → MCP_INSPECTOR_API_TOKEN,
MCP_PROXY_FULL_ADDRESS removed, SERVER_PORT repurposed), the v1 UI
config settings, web query params, and Docker
Linked from the root README (banner + docs list), the CLI README, and
docs/mcp-server-configuration.md; AGENTS.md's docs/ tree entry updated.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt
Closes#1811
Four README links pointed at files that don't exist on `v2/main`, and the launcher README described catalog seeding as one behavior when it is two.
- `docs/mcp-server-configuration.md` — written fresh for v2 (the v1.5 file predates the `--catalog`/`--config` split)
- `docs/launcher-config-consolidation-plan.md` — restored from `v1.5/main`, path drift corrected, v2 notes appended
- `docs/images/tui-screenshot.png` — regenerated against the current TUI
- launcher README seeding wording — split into the two real behaviors (web seeds two sample servers; cli/tui seed empty)
Review across three rounds also corrected several claims verified against source: the `--` separator is inverted between web and cli, a cli target placed after a flag is silently discarded, `--server` is a no-op on web and an unknown-option error on tui, the `MCP_CATALOG_PATH` ad-hoc guard is cli-only, and the conflict matrix diverges on both `--header` and `--transport stdio`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD
Closes#1795.
Enforce localhost-only binding in code (the "never 0.0.0.0" rule was only a spec note): refuse an all-interfaces HOST — across every spelling the OS binds as the wildcard (inet_aton decimal/octal/hex, IPv6, IPv4-mapped, zone-scoped, IDNA/Unicode) — unless DANGEROUSLY_BIND_ALL_INTERFACES=true (Docker's sanctioned opt-in). Fixes the reported stdio-connect 403 by expanding the default loopback origin allow-list to all three interchangeable forms.
Hardening surfaced across an extended review: closes an IDNA/fullwidth wildcard bypass, a blank-ALLOWED_ORIGINS fail-open, and the CLI/TUI OAuth-callback wildcard bind (loopback-only now, RFC 8252); canonicalizes the origin allow-list the way a browser does; derives frame-ancestors from the real embedder list (IPv6-literal exclusion verified in headless Chromium); strictPort on all three bind points; and preserves the CLI exit-code map + JSON envelope through the mcp-inspector --cli launcher path. Shared host normalization lives in core/node/hostUrl.ts. Docs + tests + smokes throughout.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
- Root README no longer links to docs/mcp-server-configuration.md (not yet
ported to v2); keep only the mcp-app-review.md pointer (review #3).
- Collapse the Local-fixture parenthetical onto one line so prettier no longer
renders '- resource preset …' as a mid-sentence bullet (review #5).
Findings #1/#2 (documents unmerged #1576/#1577/#1665 behavior) and #4 (the
web-README #deep-link-auto-connect anchor) resolve in the Wave 4 rollup, which
lands this doc together with those code PRs — the reviewer noted the same.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
Closes#1578
Adds docs/mcp-app-review.md — the CLI-first → one-shot-web recipe for automated
MCP App review: `--app-info` probe → deep-link navigate → rendered widget, plus
OAuth-gated servers (stored-auth handoff, now honoring refresh_token per #1665),
SSH port-forward (both ports), and HTTP proxy support.
Adapted from PR #1510's reference doc to only document what actually merged and
to match the current implementation:
- Correct driver contract: `data-testid="connection-status"` + `data-status`
/`data-error-message`/`data-deeplink`, `data-testid="open-app"`, and the
`data-app-status="ready"` app lifecycle (not the reference's stale selectors).
- The zero-click `autoOpen` path (#1577) alongside the click-to-open variant.
- `--use-stored-auth` now refreshes a stored refresh_token (#1665), so the
"token is not refreshed" caveat is replaced.
The per-client READMEs (web deep-link scheme + data-* contract, CLI handoff +
refresh + proxy) were updated in their own Wave 4 PRs; this adds the root README
pointer to docs/. Proxy/undici is already documented in the CLI README.
Part of the #1579 decomposition (Wave 4, docs — lands last).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
