modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

feat(test-servers): stall an OAuth endpoint so #2319's timeouts can be driven (#2383)

Browse source 
* feat(test-servers): stall an OAuth endpoint so #2319's timeouts can be driven

#2319 put an `AbortSignal.timeout` on five OAuth-path calls that previously
went out with no signal at all. Nothing in the repo could make any of them
fire: the composable server's `oauth` block had no stall, delay or
never-answer option, so all five were covered only by tests injecting a
`fetch` stub.

A stub settles on the client side. The state #2319 is actually about is an
established, idle socket in the Inspector's own Node process — invisible in
browser devtools, and held behind a connection status that deliberately stays
at `connecting`. Only a server that accepts and withholds reproduces it.

Adds `oauth.stallEndpoints` (any of `protected-resource-metadata`,
`as-metadata`, `authorize`, `token`, `revoke`, `register`) plus an optional
`stallMs` to answer late rather than never. Endpoints are named by the CALL,
not the path: the exchange and the refresh share `/oauth/token`, and two of
the documents sit at configurable paths, so a path-keyed option would be both
ambiguous and wrong when `resourceMetadataPath` or `asMetadataPath` moves one.

Per endpoint rather than global, so a test can tell "discovery timed out" from
"token exchange timed out" — the distinction the five separate timeouts exist
to make. An unrecognized name throws at start rather than being ignored, since
a typo would otherwise yield a fixture that answers normally and a timeout test
that fails pointing at the timeout instead of at the config.

Two showcase configs and a `docs/test-servers.md` section covering both, what
to do with them, and what the broken build did.

Teardown needed no new machinery: `TestServerHttp.stop()` already calls
`httpServer.closeAllConnections?.()`, which destroys an established socket
whether or not a response was written. Measured at 0s with a withheld response
in flight, and pinned by a test, because it is the property most likely to be
broken by a future change to the stop path — and it would break as a hang in
`afterEach`, pointing at the wrong test.

Each guard was mutation-checked against the new suite:

  middleware never registered      -> 7 of 8 failed
  match req.url instead of req.path -> 1 of 8 failed
  unknown-entry validation removed  -> 1 of 8 failed
  stall calls next() anyway         -> 6 of 8 failed
  (unmutated)                       -> 8 passed

The `req.path` mutation initially escaped: every stallable endpoint but
`authorize` is requested at a bare path, so matching on `req.url` passed the
whole file. The query-string case exists to close that, and its comment says
so.

Closes #2382

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>

* fix(test-servers): key the OAuth stall by method, and cover every endpoint (#2383 review)

Five findings from review, all defects in what this PR added.

**Method is part of the endpoint identity.** The middleware collapsed each
endpoint to a path, but two of the paths are caller-supplied: a valid
`asMetadataPath: "/oauth/token"` made stalling `token` also stall the metadata
GET, and vice versa — breaking the per-call contract the option exists to
provide. Keyed on `${METHOD} ${path}` now, with `authorize` carrying both GET
(consent page) and POST (submission).

**Every advertised endpoint is driven, not three of six.** `as-metadata`,
`revoke` and `register` were reachable in config but never requested, so a typo
in any of their hardcoded entries would yield a fixture that quietly answers
normally — the exact failure this option exists to prevent. The suite is now
table-driven over `STALLABLE_OAUTH_ENDPOINTS`, so adding a name without
covering it is impossible.

**`stallMs` is tested.** It was advertised and every fixture used 0, leaving the
delay branch and its timer cleanup able to regress unseen. Three cases now:
answers late rather than never, times out when the caller's budget is shorter
than the delay, and releases the timer when the client aborts first.

**The teardown test waits for a real signal, not a fixed sleep.** A 250ms sleep
that lost the race would have stopped the server before the request arrived; the
fetch would reject because the server closed and the test would pass without
ever exercising `closeAllConnections()` on an established request. It now waits
until the request is actually parked.

⚠️ That signal started as a module-level counter and failed under Vitest, which
can load the fixture module more than once — the middleware incremented one copy
while the test polled another, and the wait timed out with the fixture working
perfectly (verified standalone: parked after 100ms). It is per-server state read
off the instance the test already holds, which makes module identity irrelevant
and scopes the count to one fixture.

**The docs section is top-level.** It had been inserted as `###` directly under
`## Revoking tokens on clear`, so that section's prose became part of the
stalled-fixture subsection.

Re-mutated against the expanded suite; every guard is independently detected:

  middleware never registered        -> 19 of 20 failed
  match req.url instead of req.path  ->  2 of 20 failed
  ignore method (path-only key)      ->  1 of 20 failed
  stallMs answers immediately        ->  3 of 20 failed
  registry never decremented         ->  1 of 20 failed
  as-metadata path typo              ->  2 of 20 failed
  (unmutated)                        -> 20 passed

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>

* fix(test-servers): validate stallMs, cover both configurable paths and the timer (#2383 review round 2)

Six more findings, all defects in what this PR added.

**The timer-cleanup test did not test timer cleanup.** It observed
`registry.parked`, which a separate `close` listener decrements, so it passed
with the 10s timer still armed — a test whose name and comment claimed
something it could not see. Split in two: the end-to-end case now says only
what it proves (the fixture lets go), and a focused middleware test with fake
timers and a `next` spy checks the actual claim. Removing `clearTimeout` now
fails; before, it did not.

**Both configurable metadata paths are now driven at non-default values.**
`stallTargetsFor` reads `resourceMetadataPath` and `asMetadataPath`, and every
test requested the defaults — so replacing either lookup with its default
literal left the suite green while `stallEndpoints` silently stopped working
for any config that moves the document. That is the exact typo class this
option exists to prevent, and it was the one my own doc comment claimed to
guard.

**`authorize` is driven as a POST as well as a GET.** It is the only endpoint
mapped to two methods, and only one was exercised, so dropping `"POST"` passed
the suite while a consent submission stopped stalling.

**`stallMs` is validated at setup**, like its sibling `stallEndpoints`. A
JSON/YAML config is only *cast* to its interface, so anything can arrive: a
negative or non-numeric value made `stallMs > 0` false and silently became a
PERMANENT stall, and a value past the 32-bit timer range overflows to ~1ms and
answers almost immediately. Both read as "the timeout behaved strangely"
rather than "the config is wrong". `MAX_STALL_MS` mirrors
`MAX_TIMER_DELAY_MS` in `core/auth/requestTimeout.ts`, which exists for the
same reason.

**The docs no longer claim a configurable budget.** They said the failure
arrives "after roughly 30s unless a shorter one is configured". There is no
such setting: `InspectorClient` passes `DEFAULT_OAUTH_REQUEST_TIMEOUT_MS`
straight into the transport chain and passes no budget at all on the auth
chain, so both take the default. Verified in
`core/mcp/inspectorClient.ts:866-889` before changing the text — it now states
the fixed 30 seconds.

**`stop()`'s JSDoc is back above `stop()`.** The new accessor had been inserted
between them, orphaning it.

Each fix mutation-checked against the 26-test suite; every one is detected, and
each by exactly one test:

  drop POST from authorize's methods       -> 1 of 26 failed
  resourceMetadataPath -> default literal  -> 1 of 26 failed
  asMetadataPath -> default literal        -> 1 of 26 failed
  clearTimeout on close removed            -> 1 of 26 failed
  stallMs validation removed               -> 1 of 26 failed
  (unmutated)                              -> 26 passed

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>

* fix(test-servers): validate the stallEndpoints container, reject stallMs null, drop `any` (#2383 review round 3)

Four findings — two inline, two suppressed. All valid.

**`stallEndpoints` is validated as a container, not just by length.** A config
file is cast rather than parsed, so the value can arrive as a string, `null`, or
an object with a `length`. The old `.length === 0` check accepted `""` and
`{ length: 0 }` and returned "no stalling configured" — silently disabling the
capability this startup validation exists to protect — while a non-empty string
failed later with an incidental `.filter is not a function`.

**An explicit `stallMs: null` is rejected rather than defaulted.** `?? 0` turned
it into a valid 0, which skipped every check below and silently produced a
permanent stall. Only an omitted value takes the default now; `null` falls
through to the finite-number check.

**Both `as any` suppressions are gone.** This repo forbids `any` outright and
forbids disabling the rule to satisfy the linter, and I did both — twice. The
doubles are now bridged to `Request`/`Response` with a single justified
`as unknown as` and a comment saying why no narrower option exists: the literal
is missing ~50 properties so a single `as Request` is rejected, and the
middleware reads only `method`, `path` and `res.on`. If it ever reads more, the
double is wrong at runtime instead of being silently absorbed — which is what
the `any` was doing.

Mutation-checked, both new guards detected:

  stallEndpoints container check removed -> 1 of 28 failed
  stallMs back to `?? 0` (null coerced)  -> 1 of 28 failed
  (unmutated)                            -> 28 passed

⚠️ The container mutation initially read as ESCAPED at 28 passed. It had not:
`npm run format` had rewrapped the condition across lines, so the perl pattern
matched nothing and the "mutation" was a no-op. Re-applied by line range it
fails as expected. Worth recording — a mutation that silently fails to apply
reads exactly like a test gap, and I nearly reported it as one.

`npm run check` passes in both clients/cli and clients/web, so the `any` rule
and the type-aware lint are clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>

* fix(test-servers): refuse to stall a route the config does not serve (#2383 review round 4)

Three findings, all suppressed comments (no inline threads this round).

**A stall is refused for an endpoint this config does not actually expose.** The
middleware runs *before* Express routing, so it would happily hold a request for
a route that would otherwise 404 — `supportDCR: false` with
`stallEndpoints: ["register"]` produced a hanging registration endpoint instead
of a configuration error, and a timeout test written against it would have
passed for entirely the wrong reason. `servesEndpoint` mirrors the registration
rules in `setupOAuthRoutes`/`setupMetadataEndpoints`: the protected-resource
document is always served, the rest of the local AS exists only in `combined`
mode, `revoke` carries `supportRevocation` and `register` carries `supportDCR`.
It lives next to `stallTargetsFor` so the two stay in step — a new OAuth route
needs an entry in both.

This is the same class as the validation already here, and the reason is the one
this PR keeps restating: a misconfiguration must fail loudly rather than produce
a hang that reads as a timeout.

**The error no longer misreports NaN and Infinity.** `JSON.stringify` returns
the string `"null"` for both — the two values most likely to reach the `stallMs`
check — so the message named a value the author never wrote. `describeValue`
renders non-finite numbers with `String` and keeps quotes on strings, which is
what makes `"600"` distinguishable from `600`.

**The PR description's test count is corrected again** (26 → 30) along with the
mutation table.

Mutation-checked, both new guards detected:

  availability check removed            -> 1 of 30 failed
  describeValue -> JSON.stringify       -> 1 of 30 failed
  (unmutated)                           -> 30 passed

The availability test asserts both directions: five contradictory configs throw,
and the protected-resource document (served in every mode) plus the enabled
forms are accepted, so the check cannot pass by over-rejecting.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>

---------

Signed-off-by: cliffhall <cliff@futurescale.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Cliff Hall 2026-09-16 02:10:29 -04:00 • committed by GitHub
commit de2cbaeeaf
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
1 changed files with 1168 additions and 3 deletions
Download patch file Download diff file Expand all files Collapse all files

16
docs/test-servers.md
View file

@ -56,6 +56,7 @@ as a missing capability rather than an error.
| `oauth-rfc8414-at-oidc-path-http.json` **(legacy era)** | Plain OAuth 2.0 AS metadata served at the OIDC well-known path | [#2172](https://github.com/modelcontextprotocol/inspector/issues/2172) |
| `oauth-insecure-token-endpoint-http.json` **(legacy era)** | A token endpoint the SDK refuses to post credentials to | [#2280](https://github.com/modelcontextprotocol/inspector/issues/2280) |
| `oauth-cimd-http.json` **(legacy era)** | URL-based client IDs (CIMD / SEP-991), DCR deliberately off | [#2242](https://github.com/modelcontextprotocol/inspector/issues/2242) |
| `oauth-stalled-{token,discovery}-http.json` **(legacy era)** | An OAuth endpoint that accepts the request and never answers, so the OAuth-path request timeouts fire against a real idle socket | [#2382](https://github.com/modelcontextprotocol/inspector/issues/2382) |
| `logging-{legacy,modern}-http.json` **(era per file)** | Logging, both eras | [#1629](https://github.com/modelcontextprotocol/inspector/issues/1629) |
| `subscriptions-{legacy,modern}-http.json` **(era per file)** | Resource subscriptions, both eras | [#1630](https://github.com/modelcontextprotocol/inspector/issues/1630) |
| `subscriptions-never-acknowledged-http.json` **(modern era)** | A `subscriptions/listen` answered with a bare result | [#2097](https://github.com/modelcontextprotocol/inspector/issues/2097) |
@ -648,6 +649,21 @@ string included — so a server that walked to another port on `EADDRINUSE` stil
`client_id` equal to the URL you fetched, and the integration test drives it on a harness-chosen port
for exactly that reason. The fixed-port dependency that *does* bite is `redirect_uris`, above.
## An OAuth endpoint that never answers
Two OAuth-protected servers (combined AS + resource, DCR, refresh tokens) that are ordinary in every respect but one: a single endpoint **accepts the request and withholds the response forever**. Plain streamable-HTTP — connect with the **default (legacy)** protocol era.
- `oauth-stalled-token-http.json` stalls `POST /oauth/token`, so the token exchange and the refresh — which share that endpoint — both hang.
- `oauth-stalled-discovery-http.json` stalls `GET /.well-known/oauth-protected-resource`, so the stall happens before a client has any authorization server to talk to.
**What to do.** Connect to either from the Inspector and start an OAuth flow. Each should fail with an `OAuthRequestTimeoutError` naming the endpoint it gave up on and the budget it used, after **30 seconds** — `DEFAULT_OAUTH_REQUEST_TIMEOUT_MS`. That wait is not configurable from the UI or from a config file: `InspectorClient` passes the constant straight into its transport chain and passes no budget at all on the auth chain, so both take the default. Budget it for a full 30s of waiting per attempt. Every *other* endpoint on the same server answers normally, so the failure points at one call rather than at the server.
**What the broken build did.** Before [#2319](https://github.com/modelcontextprotocol/inspector/issues/2319), every OAuth-path call went out with no `AbortSignal` at all and these fixtures produced an unbounded spinner rather than an error — and a silent one, because a connect-time auth error deliberately holds the connection status at `connecting` on the theory that a redirect is about to end the attempt. The stall is server-side, in the Inspector's own Node process, so browser devtools showed no pending request while a socket sat established and idle.
**Why the fixture exists at all.** #2319's own tests inject a `fetch` stub, which settles on the client side and cannot produce an established idle socket — the precise state the issue is about. Until [#2382](https://github.com/modelcontextprotocol/inspector/issues/2382) added `oauth.stallEndpoints`, nothing in the repo could make those five timeouts fire.
**Configuring your own.** Any config's `oauth` block takes `stallEndpoints` — any of `protected-resource-metadata`, `as-metadata`, `authorize`, `token`, `revoke`, `register` — plus an optional `stallMs` to answer *late* instead of never. Endpoints are named by the **call**, not the path, because the token exchange and the refresh share `/oauth/token` and two of the documents sit at configurable paths. An unrecognized name throws when the server starts rather than being ignored, so a typo cannot quietly produce a fixture that answers normally.
## Revoking tokens on clear (RFC 7009)
`oauth-revocation-http.json` and `oauth-no-revocation-http.json` are the same OAuth-protected server (combined AS + resource, DCR, refresh tokens) differing in one thing: the first advertises a `revocation_endpoint`, the second advertises none. Plain streamable-HTTP — connect with the **default (legacy)** protocol era.