modelcontextprotocol / modelcontextprotocol/inspector

REQUEST_DEADLINES WeakMap in requestTimeout.ts has no test/assertion enforcing its one-entry-per-call invariant

Open
#2,418 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

chore v2
Dominant language
TypeScript
Stars
10.9k
Forks
1.5k
Avg merge
6h 17m
Merged PRs (30d)
151

Description

Which version line?

v2 — current (@modelcontextprotocol/inspector@latest)

Which client?

All / shared core

Inspector version

2.7.0 (git tag) — static code-review finding, not run locally

Node version

N/A — static code review, no live run performed

Operating system (and browser, for the web client)

N/A — static code review

Transport

Streamable HTTP

MCP server under inspection

N/A — this is a static code-review finding against the 2.7.0 tag source (core/auth/requestTimeout.ts, core/mcp/remote/createRemoteFetch.ts), not a live reproduction against a running MCP server.

Steps to reproduce

Found via static review of the 2.7.0 tag source, not a live run.

  1. core/auth/requestTimeout.ts keys the timeout budget for each wrapped request in a module-level WeakMap<object, number> called REQUEST_DEADLINES (line 292), read back by deadlineForRequestInit() (line 301) so createRemoteFetch.ts knows whether a given /api/fetch call is bounded.
  2. The design comment above the WeakMap (lines 274-291) states the invariant this depends on explicitly: the wrapper must build a fresh RequestInit object per call, so there is exactly one WeakMap entry per in-flight request.
  3. That invariant holds today only because withOAuthRequestTimeout's returned fetch always constructs a brand-new object literal (const nextInit: RequestInit = { ...init, signal };, line 504) immediately before REQUEST_DEADLINES.set(nextInit, budget) (line 505) and hands that exact object to fetchFn.
  4. Nothing in the module asserts this at runtime, and no test exercises what happens if a future refactor (a retry wrapper, a caching layer, or a change that hoists nextInit construction outside the per-call closure) ends up passing the same RequestInit object into fetchFn twice.

I have not exercised this failure path live; flagging the absence of an enforcement mechanism for an invariant the code currently only holds by convention.

Expected behavior

A runtime assertion (or a unit test) makes this invariant explicit — e.g. throwing if REQUEST_DEADLINES.has(nextInit) is already true before .set() is called — so a future violation fails loudly at the point of the mistake.

Actual behavior

If the same RequestInit object were ever passed into fetchFn twice, the second call's lookup via deadlineForRequestInit would silently read the first call's stamped budget (or vice versa), misclassifying a request as bounded/unbounded. Because the failure mode is a stale WeakMap hit rather than a thrown error, it would surface only as a confusing, hard-to-reproduce OAuth timeout rather than a clear failure at the point of the mistake.

Suggested fix: add a lightweight runtime assertion guarding REQUEST_DEADLINES.set() against overwriting an existing entry, plus a unit test asserting two back-to-back calls through the same wrapped fetch never share a WeakMap entry.

Logs, errors, or screenshots

No response

Already prototyped a fix?

No response

Before you submit
  • I searched existing issues and this is not a duplicate.
  • This is not a security vulnerability report (those go through the private advisory process).

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in core/auth/requestTimeout.ts by reading the REQUEST_DEADLINES comment, deadlineForRequestInit(), and withOAuthRequestTimeout’s per-call RequestInit construction. Check core/mcp/remote/createRemoteFetch.ts to understand the lookup, then add a focused unit test for two back-to-back wrapped calls and verify that overwriting an existing entry is detected. Done means the invariant is tested and future violations fail clearly.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
authentication, testing
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.