Close two ways a request could dodge the rate limiter

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>

Author: alistair3149

CHANGELOG.md

@@ -13,7 +13,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/)
 
 ### Breaking changes
 
-- The HTTP transport now rate limits `tools/call`: each signed-in user gets their own allowance (default 30 per second, burst 60) and anonymous callers share one (default 100 per second). A request over the limit is refused with `429` and a `Retry-After` header. Raise `MCP_RATE_LIMIT` / `MCP_RATE_LIMIT_BURST` / `MCP_RATE_LIMIT_ANONYMOUS` if you run high-throughput automation, or set `MCP_RATE_LIMIT=0` to disable.
+- The HTTP transport now rate limits `tools/call`: each authenticated caller gets its own allowance (default 30 per second, burst 60) and anonymous callers share one (default 100 per second). A request over the limit is refused with `429` and a `Retry-After` header. Raise `MCP_RATE_LIMIT` / `MCP_RATE_LIMIT_BURST` / `MCP_RATE_LIMIT_ANONYMOUS` if you run high-throughput automation, or set `MCP_RATE_LIMIT=0` to disable.
 - The HTTP transport no longer forwards a caller's `Authorization: Bearer` header to MediaWiki. Such a request is refused with `401`, because a token minted by the wiki was not issued for this server. Use [hosted OAuth sign-in](docs/deployment.md#hosted-oauth-sign-in), or set `MCP_ALLOW_BEARER_PASSTHROUGH=true` to keep the old behaviour while you migrate; it is deprecated and will be removed. The server now warns at startup when a wiki requires a signed-in user but neither hosted sign-in nor forwarding is available, since no request could then succeed.
 - The server no longer advertises the wikis' own authorization servers, so a client can no longer discover where to mint a token to send here. Without hosted OAuth sign-in enabled, `/.well-known/oauth-protected-resource` now answers `404`, and `list-wikis` stops reporting each wiki's `authorizationServer`. Deployments running the hosted sign-in are unaffected.
 - The `Origin` header is now validated on every bind, and a request carrying an unlisted origin is refused with `403`. If you serve a browser-based client from a public bind, set `MCP_ALLOWED_ORIGINS` before upgrading. Clients that send no `Origin` header, which is most of them, are unaffected.

docs/deployment.md

@@ -49,7 +49,7 @@ Serve a single wiki for anonymous reads: no sign-in, no writes. Good for public
 
 `readOnly: true` together with `allowWikiManagement: false` hides the wiki-management tools (`add-wiki`, `remove-wiki`) and the six write tools (`create-page`, `update-page`, `delete-page`, `undelete-page`, `upload-file`, `upload-file-from-url`) from `tools/list`. What remains is an anonymous, read-only interface.
 
-Then run it with `MCP_TRANSPORT=http` behind a reverse proxy that terminates TLS (Cloudflare, nginx, and Caddy all work), then set the [Host and Origin allowlists](#security-checklist). The server [rate limits tool calls itself](#rate-limiting); IP-level limiting against anonymous floods still belongs at the proxy, which knows the caller's address when this server does not.
+Then run it with `MCP_TRANSPORT=http` behind a reverse proxy that terminates TLS (Cloudflare, nginx, and Caddy all work), then set the [Host and Origin allowlists](#security-checklist). The server itself [rate limits tool calls](#rate-limiting); IP-level limiting against anonymous floods still belongs at the proxy, which knows the caller's address when this server does not.
 
 ## Hosted OAuth sign-in
 
@@ -242,9 +242,9 @@ Set `MCP_TRANSPORT=http` to select this transport (the Docker image defaults to
 | `MCP_TRUSTED_HOSTS` | unset | Comma-separated **outbound** SSRF-guard exemptions for internal destinations (e.g. `mediawiki.svc`). See [Outbound SSRF guard](#outbound-ssrf-guard). |
 | `MCP_ALLOW_STATIC_FALLBACK` | unset | Allow HTTP startup when a wiki has static credentials, making them a shared fallback identity. See [Security checklist](#security-checklist). |
 | `MCP_ALLOW_BEARER_PASSTHROUGH` | unset | Deprecated. Forward a caller's `Authorization` header to MediaWiki as that caller. Without it such a request is refused with `401`. See [Per-request bearer token](#per-request-bearer-token-http-transport-deprecated). |
-| `MCP_RATE_LIMIT` | `30` | Sustained `tools/call` per second per signed-in caller. `0` disables rate limiting. See [Rate limiting](#rate-limiting). |
+| `MCP_RATE_LIMIT` | `30` | Sustained `tools/call` per second per authenticated caller. `0` disables rate limiting. See [Rate limiting](#rate-limiting). |
 | `MCP_RATE_LIMIT_BURST` | 2 × rate | How far a caller's burst can run ahead of the sustained rate. |
-| `MCP_RATE_LIMIT_ANONYMOUS` | `100` | Sustained `tools/call` per second across **all** anonymous callers combined (burst 2 ×). `0` leaves anonymous traffic unlimited. |
+| `MCP_RATE_LIMIT_ANONYMOUS` | `100` | Sustained `tools/call` per second across **all** anonymous callers combined (burst 2 × the rate, not separately tunable). `0` leaves anonymous traffic unlimited. |
 
 `MCP_MAX_REQUEST_BODY` matches nginx's `client_max_body_size 1m`. Raise it if `update-page` calls return 413 on legitimately large edits or your wiki has raised `$wgMaxArticleSize` (MediaWiki default 2 MB). Lower it for a tighter DoS guard.
 
@@ -313,9 +313,9 @@ A request carrying an `Origin` the server cannot parse at all is rejected with a
 
 ### Rate limiting
 
-`tools/call` is rate limited per caller: each signed-in user gets their own allowance (`MCP_RATE_LIMIT`, burst `MCP_RATE_LIMIT_BURST`), and all anonymous callers share one (`MCP_RATE_LIMIT_ANONYMOUS`). A request over the limit is refused with `429` and a `Retry-After` header, and never reaches the wiki. Discovery calls and subscription streams are not limited.
+`tools/call` is rate limited per caller: each authenticated caller gets its own allowance (`MCP_RATE_LIMIT`, burst `MCP_RATE_LIMIT_BURST`), and all anonymous callers share one (`MCP_RATE_LIMIT_ANONYMOUS`). A request over the limit is refused with `429` and a `Retry-After` header, and never reaches the wiki. Only `tools/call` is limited; every other request, including subscription streams, passes untouched.
 
-The split follows who can see what: only this server knows which signed-in user a request acts as, so per-user fairness lives here; only the reverse proxy can tell anonymous callers apart by IP address, so the anonymous allowance is a flood backstop for the wiki, not fairness between anonymous callers. The limiter is per-process — replicas each enforce their own allowance — and `mcp_rate_limited_total` on [`/metrics`](operations.md#metrics) counts refusals for tuning.
+The limiter is per-process — replicas each enforce their own allowance — and `mcp_rate_limited_total` on [`/metrics`](operations.md#metrics) counts refusals for tuning.
 
 ### v1 limitations
 
@@ -346,7 +346,7 @@ Authorization: Bearer <oauth2-access-token>
 
 Use a MediaWiki OAuth2 access token obtained from `Special:OAuthConsumerRegistration/propose/oauth2` on the target wiki, with [Extension:OAuth](https://www.mediawiki.org/wiki/Extension:OAuth) installed. The server forwards it to MediaWiki as that caller's token, so writes are attributable and MediaWiki's per-user rate limits apply. A bearer is scoped to a single MediaWiki OAuth2 realm, and the server pins nothing across requests: one client can address wikis on different authorization servers by sending the right token per request. While forwarding is enabled, `list-wikis` reports each OAuth wiki's `authorizationServer` so a caller can see which realm a wiki belongs to.
 
-No OAuth discovery leads here: only [Hosted OAuth sign-in](#hosted-oauth-sign-in) publishes a protected-resource document, and it names this server, so an OAuth-aware client is never steered into minting a wiki token to present here. Obtain the token yourself and configure it on the caller. While `MCP_ALLOW_BEARER_PASSTHROUGH=true` is set, a bearer-less request is challenged with `401` when no configured wiki is usable without a token; a deployment mixing OAuth and non-OAuth wikis still serves tokenless clients on the wikis that allow anonymous access.
+No OAuth discovery points at the wikis' authorization servers: the only protected-resource document is the one [Hosted OAuth sign-in](#hosted-oauth-sign-in) publishes, and it names this server. A client cannot discover where to mint a wiki token — obtain it yourself and configure it on the caller. While `MCP_ALLOW_BEARER_PASSTHROUGH=true` is set, a bearer-less request is challenged with `401` when no configured wiki is usable without a token; a deployment mixing OAuth and non-OAuth wikis still serves tokenless clients on the wikis that allow anonymous access.
 
 **Precedence:** request header (only while `MCP_ALLOW_BEARER_PASSTHROUGH=true`; otherwise refused with `401`) → `config.json` `token` → `config.json` `username`/`password` → anonymous. The HTTP transport refuses to start with static credentials in `config.json` unless `MCP_ALLOW_STATIC_FALLBACK=true` is set; see [the Security checklist](#security-checklist) for why.
 

docs/operations.md

@@ -90,7 +90,7 @@ Exposed series:
 - `mcp_inflight_requests` — gauge of `/mcp` requests currently being served. Subscription streams are excluded: they are held open by design.
 - `mcp_subscription_streams` — gauge of open change-notification streams (`subscriptions/listen`), the closest measure of connected clients.
 - `mcp_ready_failures_total` — counter of `/ready` probes that returned non-200.
-- `mcp_rate_limited_total{caller}` — counter of `tools/call` requests refused with `429`, labelled `caller` or `anonymous`. A rising `caller` series means signed-in users hit `MCP_RATE_LIMIT`; a rising `anonymous` series means the shared backstop is engaging.
+- `mcp_rate_limited_total{caller}` — counter of `tools/call` requests refused with `429`; the `caller` label is `caller` for authenticated callers and `anonymous` for the shared bucket. A rising `caller` series means authenticated callers hit `MCP_RATE_LIMIT`; a rising `anonymous` series means the shared backstop is engaging.
 - `mcp_proxy_store_upstream_tokens` — gauge of upstream MediaWiki tokens held in the hosted OAuth proxy store. This set grows with cumulative sign-ins over the process lifetime; watch it to size memory and the flush cost below.
 - `mcp_proxy_store_clients` — gauge of registered clients held in the hosted OAuth proxy store. FIFO-capped at 10,000, so this plateaus rather than growing without bound.
 - `mcp_proxy_store_flush_duration_seconds` — histogram of hosted-proxy store durable-flush durations (serialize + encrypt + write). Every upstream-token write flushes the whole store synchronously, so this scales with the token count above. Records successful flushes only.

src/transport/mcpRoute.ts

@@ -13,7 +13,7 @@ import {
 	RATE_LIMITED_ERROR_CODE,
 	UPSTREAM_UNAVAILABLE_ERROR_CODE,
 } from './errorCodes.ts';
-import type { RateLimiter } from './rateLimit.ts';
+import type { RateLimitDecision, RateLimiter } from './rateLimit.ts';
 import type { ProxyConfig } from '../auth/authorizationServer/proxyConfig.ts';
 import type { ProxyStore } from '../auth/authorizationServer/proxyStore.ts';
 import {
@@ -130,6 +130,23 @@ function emit503Unavailable(req: Request, res: Response): void {
 	});
 }
 
+// One shared allowance for every caller-supplied bearer, charged alongside the
+// per-token bucket. This server cannot verify those tokens, so without it a
+// caller minting a random bearer per request would never meet a limit.
+const PASSTHROUGH_SHARED_KEY = 'bearer:shared';
+
+// The refusal that should be reported when two allowances are charged: a refusal
+// beats an admission, and the longer wait beats the shorter.
+function worstOf(a: RateLimitDecision, b: RateLimitDecision): RateLimitDecision {
+	if (a.allowed) {
+		return b;
+	}
+	if (b.allowed) {
+		return a;
+	}
+	return a.retryAfterSeconds >= b.retryAfterSeconds ? a : b;
+}
+
 function emit429RateLimited(req: Request, res: Response, retryAfterSeconds: number): void {
 	res.set('Retry-After', String(retryAfterSeconds));
 	res.status(429).json({
@@ -146,15 +163,25 @@ function emit429RateLimited(req: Request, res: Response, retryAfterSeconds: numb
 // requirement names, and it is the one that reaches the wiki. Discovery,
 // initialize, cancellations and the held-open subscriptions/listen stream all
 // pass untouched — a stream that consumed a token would hold it forever.
-function isToolsCallRequest(body: unknown): boolean {
+function isToolsCallMessage(message: unknown): boolean {
 	return (
-		typeof body === 'object' &&
-		body !== null &&
-		!Array.isArray(body) &&
-		(body as { method?: unknown }).method === 'tools/call'
+		typeof message === 'object' &&
+		message !== null &&
+		(message as { method?: unknown }).method === 'tools/call'
 	);
 }
 
+// How many tokens a request costs. The legacy stateless leg executes a JSON-RPC
+// batch array entry by entry, so a batch must be charged per tools/call entry:
+// counting the array as one request would let any caller wrap N calls in
+// brackets and pay for one.
+function toolsCallCost(body: unknown): number {
+	if (Array.isArray(body)) {
+		return body.filter(isToolsCallMessage).length;
+	}
+	return isToolsCallMessage(body) ? 1 : 0;
+}
+
 export interface McpRouteOptions {
 	wikiRegistry?: WikiRegistry;
 	// When the hosted OAuth proxy is enabled, the /mcp bearer is a proxy-minted
@@ -277,21 +304,35 @@ export function createMcpRouteHandler(
 		}
 
 		// A bearer that survived to here outside the proxy path is being forwarded
-		// under the deprecated opt-in. Its bucket is keyed on a digest of the token
-		// itself — the only identity available; a caller that rotates tokens
-		// rotates buckets, which is one more way the passthrough shape falls short
-		// of the hosted sign-in.
+		// under the deprecated opt-in. This server never verifies it, so a digest
+		// of the token is the only identity available.
+		let forwardedBearer = false;
 		if (rateLimitKey === undefined && resolvedBearer !== undefined && !(pc && proxyStore)) {
 			const digest = createHash('sha256').update(resolvedBearer).digest('hex').slice(0, 32);
 			rateLimitKey = `bearer:${digest}`;
+			forwardedBearer = true;
 		}
 
-		if (rateLimiter && req.method === 'POST' && isToolsCallRequest(req.body)) {
-			const decision = rateLimiter.take(rateLimitKey);
-			if (!decision.allowed) {
-				recordRateLimited(rateLimitKey === undefined ? 'anonymous' : 'caller');
-				emit429RateLimited(req, res, decision.retryAfterSeconds);
-				return;
+		if (rateLimiter && req.method === 'POST') {
+			const cost = toolsCallCost(req.body);
+			if (cost > 0) {
+				// An unverified bearer would otherwise mint a fresh allowance per
+				// request: rotating random tokens rotates digests. Charging a shared
+				// bucket as well caps the whole forwarding path, while the per-digest
+				// bucket still keeps distinct legitimate callers apart. Refusing on
+				// either leaves the other's token spent — an accounting slip that
+				// only ever makes the limiter stricter under abuse.
+				const decision = forwardedBearer
+					? worstOf(
+							rateLimiter.take(PASSTHROUGH_SHARED_KEY, cost),
+							rateLimiter.take(rateLimitKey, cost),
+						)
+					: rateLimiter.take(rateLimitKey, cost);
+				if (!decision.allowed) {
+					recordRateLimited(decision.bucket);
+					emit429RateLimited(req, res, decision.retryAfterSeconds);
+					return;
+				}
 			}
 		}
 

src/transport/rateLimit.ts

@@ -18,11 +18,17 @@ export interface RateLimitSettings {
 	anonymousBurst: number;
 }
 
-export type RateLimitDecision = { allowed: true } | { allowed: false; retryAfterSeconds: number };
+export type RateLimitDecision =
+	| { allowed: true }
+	// `bucket` names which allowance refused, so the metric can tell a caller
+	// hitting its own limit from one refused by the shared bucket.
+	| { allowed: false; retryAfterSeconds: number; bucket: 'caller' | 'anonymous' };
 
 export interface RateLimiter {
-	// key identifies the authenticated caller; undefined means anonymous.
-	take(key: string | undefined): RateLimitDecision;
+	// key identifies the authenticated caller; undefined means anonymous. cost is
+	// how many tool calls the request carries: a JSON-RPC batch charges one token
+	// per tools/call entry, so wrapping calls in an array buys nothing.
+	take(key: string | undefined, cost?: number): RateLimitDecision;
 }
 
 interface Bucket {
@@ -34,25 +40,44 @@ interface Bucket {
 // tracks real signed-in users — the cap is a backstop, not an expected ceiling.
 const MAX_TRACKED_KEYS = 10_000;
 
+// Retry-After must be a finite number of seconds. A directly-constructed
+// settings object could carry a zero or negative rate, making the deficit
+// division non-finite; clamp rather than emit an unparseable header value.
+const MAX_RETRY_AFTER_SECONDS = 3600;
+
 function refill(bucket: Bucket, ratePerSecond: number, burst: number, now: number): void {
 	bucket.tokens = Math.min(burst, bucket.tokens + ((now - bucket.last) / 1000) * ratePerSecond);
 	bucket.last = now;
 }
 
+function retryAfterFor(deficit: number, ratePerSecond: number): number {
+	const seconds = Math.ceil(deficit / ratePerSecond);
+	if (!Number.isFinite(seconds)) {
+		return MAX_RETRY_AFTER_SECONDS;
+	}
+	return Math.min(MAX_RETRY_AFTER_SECONDS, Math.max(1, seconds));
+}
+
 function takeFrom(
 	bucket: Bucket,
 	ratePerSecond: number,
 	burst: number,
 	now: number,
+	cost: number,
+	which: 'caller' | 'anonymous',
 ): RateLimitDecision {
 	refill(bucket, ratePerSecond, burst, now);
-	if (bucket.tokens >= 1) {
-		bucket.tokens -= 1;
+	if (bucket.tokens >= cost) {
+		bucket.tokens -= cost;
 		return { allowed: true };
 	}
+	// A cost above `burst` can never be satisfied however long the caller waits,
+	// so such a batch has to be split; the deficit still gives a truthful lower
+	// bound. Nothing is consumed on a refusal.
 	return {
 		allowed: false,
-		retryAfterSeconds: Math.max(1, Math.ceil((1 - bucket.tokens) / ratePerSecond)),
+		retryAfterSeconds: retryAfterFor(cost - bucket.tokens, ratePerSecond),
+		bucket: which,
 	};
 }
 
@@ -63,12 +88,13 @@ export function createRateLimiter(
 	const { ratePerSecond, burst, anonymousRatePerSecond, anonymousBurst } = settings;
 	const buckets = new Map<string, Bucket>();
 	const anonymous: Bucket = { tokens: anonymousBurst, last: now() };
+	const anonymousLimited = anonymousRatePerSecond > 0;
 
-	function takeAnonymous(at: number): RateLimitDecision {
-		if (anonymousRatePerSecond <= 0) {
+	function takeAnonymous(at: number, cost: number): RateLimitDecision {
+		if (!anonymousLimited) {
 			return { allowed: true };
 		}
-		return takeFrom(anonymous, anonymousRatePerSecond, anonymousBurst, at);
+		return takeFrom(anonymous, anonymousRatePerSecond, anonymousBurst, at, cost, 'anonymous');
 	}
 
 	// Evicts only buckets that have refilled to capacity: a full bucket is
@@ -85,26 +111,34 @@ export function createRateLimiter(
 	}
 
 	return {
-		take(key: string | undefined): RateLimitDecision {
+		take(key: string | undefined, cost = 1): RateLimitDecision {
+			if (cost <= 0) {
+				return { allowed: true };
+			}
 			const at = now();
 			if (key === undefined) {
-				return takeAnonymous(at);
+				return takeAnonymous(at, cost);
 			}
 			let bucket = buckets.get(key);
 			if (!bucket) {
 				if (buckets.size >= MAX_TRACKED_KEYS) {
 					sweep(at);
 					if (buckets.size >= MAX_TRACKED_KEYS) {
-						// Every tracked bucket is mid-drain. Overflow keys share the
-						// anonymous bucket rather than growing the map or passing free:
-						// memory stays bounded and the overflow caller is still limited.
-						return takeAnonymous(at);
+						// Every tracked bucket is mid-drain, so admitting this key would
+						// mean growing the map without bound. Refuse rather than fall back
+						// to the anonymous bucket, which passes everything when anonymous
+						// limiting is switched off.
+						return {
+							allowed: false,
+							retryAfterSeconds: retryAfterFor(burst, ratePerSecond),
+							bucket: 'caller',
+						};
 					}
 				}
 				bucket = { tokens: burst, last: at };
 				buckets.set(key, bucket);
 			}
-			return takeFrom(bucket, ratePerSecond, burst, at);
+			return takeFrom(bucket, ratePerSecond, burst, at, cost, 'caller');
 		},
 	};
 }