Rate limit tool calls on the HTTP transport - #521
Merged
Conversation
The MCP specification has required servers to rate limit tool invocations since its first revision; this server delegated that to the reverse proxy, which can only limit by IP address. Only this server knows which signed-in user a request acts as, so per-caller fairness has to live here: tools/call is now metered by a token bucket per authenticated caller, with one shared bucket for anonymous traffic — a flood backstop for the wiki, since without a verified identity or a trusted X-Forwarded-For there is nothing honest to key on. A request over the limit is refused 429 with Retry-After and a JSON-RPC body on the new -31004 code, before anything reaches the wiki. The limiter runs after bearer resolution, which is load-bearing: keyed on the raw header, a caller rotating garbage bearers would mint a fresh bucket per request, evading the limit and growing the map without bound. Verified subjects key on the proxy JWT's jti (stable across token refreshes; resolveUpstreamBearer now returns it alongside the access token), forwarded bearers under the deprecated opt-in on a digest of the token, and invalid bearers die at the existing 401 without creating state. Only tools/call is metered — discovery, initialize and the held-open subscriptions/listen stream pass untouched, so a stream cannot consume a token forever. Eviction exploits the bucket's one memoryless state: a full bucket is indistinguishable from no bucket, so under cap pressure only refilled-to-capacity buckets are dropped, never drained ones — recreating a drained bucket would hand its caller the fresh burst the refusal exists to withhold. Overflow beyond the cap shares the anonymous bucket rather than passing free. On by default at 30/s per caller (burst 60) and 100/s shared anonymous; MCP_RATE_LIMIT, MCP_RATE_LIMIT_BURST and MCP_RATE_LIMIT_ANONYMOUS tune it and MCP_RATE_LIMIT=0 disables. mcp_rate_limited_total on /metrics counts refusals. The limiter is per-process, matching the documented single-replica posture. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Review found both live: on a drained bucket, a JSON-RPC batch of 25 tool calls returned 25 results, and a request sent as `application/json;` ran its tool every time. Either one made the limiter decorative. The legacy stateless leg executes a batch entry by entry, so a batch now costs one token per tools/call entry and is refused whole when the cost does not fit — wrapping calls in brackets buys nothing. The content-type hole was a mismatch between two predicates: the SDK accepts a header body-parser's default matcher rejects, so express left req.body undefined, the limiter saw no tool call, and the handler read the raw stream anyway. express.json now types on the SDK's own isJsonContentType, which cannot drift from what the handler accepts. That also restores MCP_MAX_REQUEST_BODY, which the same gap had been bypassing. Two further holes the review surfaced. A forwarded bearer is never verified by this server, so a caller minting a random token per request minted a fresh allowance per request; forwarded bearers now charge a shared allowance as well as their own, capping the whole deprecated path. And an overflow key arriving when every tracked bucket was mid-drain fell back to the anonymous bucket, which passes everything when anonymous limiting is switched off; it is now refused outright. The refusal also reports which allowance rejected it, so mcp_rate_limited_total stops labelling a shared-bucket refusal as the caller's own. The regression tests drive the real buildApp: both bypasses were invisible to tests that stubbed the handler or built their own express app, and a first attempt at a content-type test reproduced that mistake — it inlined the fix in its own harness and passed against the unfixed server. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the last confirmed gap from the 2026-07-28 spec-compliance audit. The spec has required servers to rate limit tool invocations since its first revision (2024-11-05); this server delegated it to the reverse proxy, which can only limit by IP. The division of labour this PR encodes: the proxy limits by address, the server limits by identity — neither can do the other's job.
What changes
tools/callon the HTTP transport is metered by a token bucket per authenticated caller, plus one shared bucket for all anonymous traffic. Over the limit:429,Retry-After, and a JSON-RPC body on the new-31004code echoing the request id — before anything reaches the wiki. Onlytools/callis metered; discovery,initialize, cancellations and the held-opensubscriptions/listenstream pass untouched, so a stream can never consume a token forever.On by default: 30/s per caller (burst 60), 100/s shared anonymous (burst 200).
MCP_RATE_LIMIT/MCP_RATE_LIMIT_BURST/MCP_RATE_LIMIT_ANONYMOUStune it;MCP_RATE_LIMIT=0disables. Stdio is untouched — one local user.mcp_rate_limited_total{caller}counts refusals on/metrics.What a reviewer should weigh
The limiter runs after bearer resolution, not in front of the route. Keyed on the raw header, a caller rotating garbage bearers mints a fresh bucket per request — evading the limit and growing the map without bound. Placed after resolution, invalid bearers die at the existing 401 and never create state. The cost — JWT verification itself is unmetered — is an HMAC check, and the upstream refresh behind it is already coalesced.
Keys: proxy callers by the JWT's
jti(the store lookup key, stable across both proxy-JWT and upstream-token refreshes;resolveUpstreamBearernow returns it alongside the access token — the one signature change). Forwarded bearers under the deprecated opt-in key on a token digest, so rotating tokens rotates buckets; that shape cannot be fairly limited, which is one more reason it is deprecated. Anonymous traffic shares one bucket by design: without a verified identity or a trustedX-Forwarded-For, a per-IP key behind the documented reverse proxy would collapse every caller into one bucket while looking per-caller. Per-IP anonymous fairness stays at the proxy; enabling Expresstrust proxyinteracts with the existingresolveRequestProtoheader handling and is deliberately not bundled here.Two request shapes had to be handled explicitly, because either one made the limiter decorative. A JSON-RPC batch is executed entry by entry on the legacy stateless leg, so it costs one token per
tools/callentry and is refused whole when the cost does not fit. Andexpress.jsonis typed on the SDK's ownisJsonContentType: the SDK accepts aContent-Typethat body-parser's default matcher rejects, which leftreq.bodyundefined while the handler read the raw stream — unmetered, and also skippingMCP_MAX_REQUEST_BODY. Both were found live by review, and both regression tests drive the realbuildApp.Forwarded bearers charge a shared allowance as well as their own. This server never verifies them, so a per-token bucket alone handed a fresh allowance to every random token; the shared bucket caps the whole deprecated path while distinct legitimate callers stay separated.
Eviction never frees a drained bucket. A full bucket is indistinguishable from no bucket, so under cap pressure (10k keys) only refilled-to-capacity buckets are swept — recreating a drained one would hand its caller the fresh burst the refusal exists to withhold. Overflow beyond a full cap is refused outright rather than delegated to the anonymous bucket, which passes everything when anonymous limiting is switched off.
Filed as Breaking because a working setup can hit it: bulk automation driving concurrent writes is a legitimate high-rate caller. Sequential automation at real wiki round-trip times sits well under the defaults.
Verification
Suite 1,732 green; typecheck, lint, format clean.
Every guard is mutation-bound — reverting each fix in turn fails tests: the route's limiter check (5), the bucket itself (15), batch cost accounting (2), the
express.jsontype predicate (2), the shared passthrough allowance (1), and the overflow refusal (2).Live against a real wiki with
MCP_RATE_LIMIT_ANONYMOUS=2: burst passes, then429+Retry-After+-31004with the id echoed, whiletools/listpasses the drained bucket. On the same drained bucket, a batch of 25 returns429with zero tool results,application/json;is metered, and a 3 MB body returns413. This exercises the env →resolveHttpConfig→createRateLimiter→buildAppwiring that route-level tests bypass.Also in this PR
One stale paragraph pair in
deployment.md's bearer section read as contradictory after #518 (list-wikisreports the wikis' authorization server vs. "a client cannot discover where to mint a token"); reconciled by scoping the first to the opt-in and the second to OAuth discovery.🤖 Generated with Claude Code