modelcontextprotocol / modelcontextprotocol/inspector
REQUEST_DEADLINES WeakMap in requestTimeout.ts has no test/assertion enforcing its one-entry-per-call invariant
Nobody has claimed this yet.
- 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.
core/auth/requestTimeout.tskeys the timeout budget for each wrapped request in a module-levelWeakMap<object, number>calledREQUEST_DEADLINES(line 292), read back bydeadlineForRequestInit()(line 301) socreateRemoteFetch.tsknows whether a given/api/fetchcall is bounded.- The design comment above the WeakMap (lines 274-291) states the invariant this depends on explicitly: the wrapper must build a fresh
RequestInitobject per call, so there is exactly one WeakMap entry per in-flight request. - 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 beforeREQUEST_DEADLINES.set(nextInit, budget)(line 505) and hands that exact object tofetchFn. - 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
nextInitconstruction outside the per-call closure) ends up passing the sameRequestInitobject intofetchFntwice.
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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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