From 33cb06bbae88bcafca99eaa69d6ae7b45c38780c Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Sat, 11 Jul 2026 11:48:23 -0700 Subject: [PATCH 1/8] feat(mcp): rate-limit client_credentials issuance and CIMD fetch attempts (#163) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the last leg of #159 (req 5, defense-in-depth): - New per-node in-memory token-bucket module (rateLimit.ts): continuous refill, injected clock, LRU-bounded key space (keys are attacker-chosen client_ids/URLs). Per-node by design — Harper replication makes a shared counter table a hot-write anti-pattern, and the assertion replay guard + 60s exp window already bound cross-node abuse. - Grant limiter: mcp.clientCredentials.rateLimit requests/min per client_id (default 30; false/0 disables), checked BEFORE resolveClient so an over-limit client triggers no CIMD fetch or crypto work. Over-limit: 429 + error "slow_down" + Retry-After. - CIMD fetch limiter: fixed 10 attempts/min per client_id URL at the resolution layer (post-cache, post-dedup — only actual fetch attempts consume), closing the bad-document fetch-amplification deferral from #167. Both #163-deferral comments updated. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01SerGP6Am3xz2CKyPgbRc73 --- docs/configuration.md | 41 ++++++++--------- docs/mcp-oauth.md | 11 +++++ src/lib/mcp/cimd.ts | 37 +++++++++++++--- src/lib/mcp/rateLimit.ts | 79 +++++++++++++++++++++++++++++++++ src/lib/mcp/token.ts | 53 ++++++++++++++++++++++ src/types.ts | 7 +++ test/lib/mcp/cimd.test.js | 78 ++++++++++++++++++++++++++++++++ test/lib/mcp/rateLimit.test.js | 81 ++++++++++++++++++++++++++++++++++ test/lib/mcp/token.test.js | 54 ++++++++++++++++++++++- 9 files changed, 415 insertions(+), 26 deletions(-) create mode 100644 src/lib/mcp/rateLimit.ts create mode 100644 test/lib/mcp/rateLimit.test.js diff --git a/docs/configuration.md b/docs/configuration.md index e9228e5..74113aa 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -84,26 +84,27 @@ Opt-in support for the Model Context Protocol authorization flow ([issue #86](ht - app.example.com ``` -| Option | Type | Default | Description | -| ------------------------------------------------------- | -------- | --------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| `mcp.enabled` | boolean | `false` | Master switch for the MCP OAuth endpoints | -| `mcp.issuer` | string | (none) | Authorization-server URI advertised in AS metadata. **Required when `mcp.enabled`** — startup fails otherwise, to prevent a Host-header-driven `iss`/`aud` | -| `mcp.resource` | string | `/mcp` | Canonical resource URI advertised in PRM (RFC 9728) and validated as `aud` on issued tokens (RFC 8707). Optional override; defaults safely from the pinned `issuer` | -| `mcp.providers` | string[] | (all providers) | Subset of upstream providers eligible for the MCP auth flow. **v1 requires exactly one** resolved provider — set this when more than one provider is configured globally, otherwise `/oauth/mcp/authorize` returns `server_error` | -| `mcp.dynamicClientRegistration.enabled` | boolean | `true` | Enable the `/register` endpoint when `mcp.enabled` is true | -| `mcp.dynamicClientRegistration.initialAccessToken` | string | (none) | If set, registration requires `Authorization: Bearer `. Otherwise open per RFC 7591 | -| `mcp.dynamicClientRegistration.allowedRedirectUriHosts` | string[] | (none) | Allowlist for redirect_uri hosts. Localhost always allowed per RFC 8252 | -| `mcp.clientIdMetadataDocuments.enabled` | boolean | `true` | Enable CIMD resolution for URL-shaped `client_id` values. Disable with `false` to reject all CIMD clients; see [Client ID Metadata Documents](./mcp-oauth.md#client-id-metadata-documents-cimd) | -| `mcp.clientIdMetadataDocuments.allowedHosts` | string[] | (none) | If set, only CIMD `client_id` URLs whose hostname is in this list are resolved. Others are silently rejected (`invalid_client`) without revealing the allowlist | -| `mcp.clientIdMetadataDocuments.fetchTimeoutMs` | number | `5000` | Deadline for CIMD document retrieval covering DNS, connect, and body read (milliseconds). Non-finite or non-positive values fall back to the default | -| `mcp.clientIdMetadataDocuments.maxDocumentBytes` | number | `65536` | Maximum CIMD document size in bytes (64 KB default). Responses exceeding this limit are rejected. Non-finite or non-positive values fall back to the default | -| `mcp.clientCredentials.enabled` | boolean | `false` | Enable the RFC 7523 `client_credentials` grant (`private_key_jwt`, EdDSA) for headless agents. Explicit opt-in; requires a non-empty `mcp.clientIdMetadataDocuments.allowedHosts` allowlist, CIMD enabled, and an `https:` `mcp.issuer` (RFC 6749 §3.2 — the token endpoint must be TLS; `http:` is permitted only for loopback development issuers) — all enforced at startup. See [Headless agents](./mcp-oauth.md#headless-agents-client_credentials) | -| `mcp.clientCredentials.accessTokenTtl` | number | `300` | Access-token lifetime in seconds for the `client_credentials` grant. No refresh token is ever issued — agents re-mint on 401. Non-finite or non-positive values fall back to the default | -| `mcp.signingKeyPem` | string | (generated) | PEM-encoded RS256 private key (PKCS#8) used to sign access tokens. When set, this key **always** wins as the signer — it is found in the key set by material match, or persisted on first use (under a deterministic kid so concurrent cluster nodes are idempotent). When unset, a UUID-kid keypair is generated on first boot. Because all persisted keys are published in the JWKS, tokens signed by any node verify everywhere — pinning is **recommended** for clusters but not strictly required | -| `mcp.keyRotationInterval` | number | `0` (disabled) | Signing-key rotation period in seconds. When `> 0`, a fresh UUID-kid keypair is generated at token-mint time once the current signer is older than this interval. Old keys are kept in the JWKS and deleted lazily once `2 × accessTokenTtl` has passed since their immediate successor was created (covering replication lag). Rotation is skipped while `signingKeyPem` is set — setting both emits a startup warning | -| `mcp.signingAlgorithm` | string | `RS256` | JWT signing algorithm. Only `RS256` is supported in v1 (reserved for a future EdDSA option) | -| `mcp.accessTokenTtl` | number | `3600` | Access-token lifetime in seconds (default 1 hour) | -| `mcp.refreshTokenTtl` | number | `2592000` | Refresh-token (family) lifetime in seconds (default 30 days) | +| Option | Type | Default | Description | +| ------------------------------------------------------- | --------------- | --------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `mcp.enabled` | boolean | `false` | Master switch for the MCP OAuth endpoints | +| `mcp.issuer` | string | (none) | Authorization-server URI advertised in AS metadata. **Required when `mcp.enabled`** — startup fails otherwise, to prevent a Host-header-driven `iss`/`aud` | +| `mcp.resource` | string | `/mcp` | Canonical resource URI advertised in PRM (RFC 9728) and validated as `aud` on issued tokens (RFC 8707). Optional override; defaults safely from the pinned `issuer` | +| `mcp.providers` | string[] | (all providers) | Subset of upstream providers eligible for the MCP auth flow. **v1 requires exactly one** resolved provider — set this when more than one provider is configured globally, otherwise `/oauth/mcp/authorize` returns `server_error` | +| `mcp.dynamicClientRegistration.enabled` | boolean | `true` | Enable the `/register` endpoint when `mcp.enabled` is true | +| `mcp.dynamicClientRegistration.initialAccessToken` | string | (none) | If set, registration requires `Authorization: Bearer `. Otherwise open per RFC 7591 | +| `mcp.dynamicClientRegistration.allowedRedirectUriHosts` | string[] | (none) | Allowlist for redirect_uri hosts. Localhost always allowed per RFC 8252 | +| `mcp.clientIdMetadataDocuments.enabled` | boolean | `true` | Enable CIMD resolution for URL-shaped `client_id` values. Disable with `false` to reject all CIMD clients; see [Client ID Metadata Documents](./mcp-oauth.md#client-id-metadata-documents-cimd) | +| `mcp.clientIdMetadataDocuments.allowedHosts` | string[] | (none) | If set, only CIMD `client_id` URLs whose hostname is in this list are resolved. Others are silently rejected (`invalid_client`) without revealing the allowlist | +| `mcp.clientIdMetadataDocuments.fetchTimeoutMs` | number | `5000` | Deadline for CIMD document retrieval covering DNS, connect, and body read (milliseconds). Non-finite or non-positive values fall back to the default | +| `mcp.clientIdMetadataDocuments.maxDocumentBytes` | number | `65536` | Maximum CIMD document size in bytes (64 KB default). Responses exceeding this limit are rejected. Non-finite or non-positive values fall back to the default | +| `mcp.clientCredentials.enabled` | boolean | `false` | Enable the RFC 7523 `client_credentials` grant (`private_key_jwt`, EdDSA) for headless agents. Explicit opt-in; requires a non-empty `mcp.clientIdMetadataDocuments.allowedHosts` allowlist, CIMD enabled, and an `https:` `mcp.issuer` (RFC 6749 §3.2 — the token endpoint must be TLS; `http:` is permitted only for loopback development issuers) — all enforced at startup. See [Headless agents](./mcp-oauth.md#headless-agents-client_credentials) | +| `mcp.clientCredentials.accessTokenTtl` | number | `300` | Access-token lifetime in seconds for the `client_credentials` grant. No refresh token is ever issued — agents re-mint on 401. Non-finite or non-positive values fall back to the default | +| `mcp.clientCredentials.rateLimit` | number \| false | `30` | Issuance rate limit in requests/minute per `client_id` (token bucket, per node — a shared counter table would be a replication hot-write anti-pattern; the assertion replay guard + 60s window bound cross-node abuse). Over-limit requests get `429` with `error: "slow_down"` and a `Retry-After` header. `false` or `0` disables; non-finite/non-positive values fall back to the default. CIMD metadata fetches are separately limited at a fixed 10 attempts/min per client_id URL (not configurable) | +| `mcp.signingKeyPem` | string | (generated) | PEM-encoded RS256 private key (PKCS#8) used to sign access tokens. When set, this key **always** wins as the signer — it is found in the key set by material match, or persisted on first use (under a deterministic kid so concurrent cluster nodes are idempotent). When unset, a UUID-kid keypair is generated on first boot. Because all persisted keys are published in the JWKS, tokens signed by any node verify everywhere — pinning is **recommended** for clusters but not strictly required | +| `mcp.keyRotationInterval` | number | `0` (disabled) | Signing-key rotation period in seconds. When `> 0`, a fresh UUID-kid keypair is generated at token-mint time once the current signer is older than this interval. Old keys are kept in the JWKS and deleted lazily once `2 × accessTokenTtl` has passed since their immediate successor was created (covering replication lag). Rotation is skipped while `signingKeyPem` is set — setting both emits a startup warning | +| `mcp.signingAlgorithm` | string | `RS256` | JWT signing algorithm. Only `RS256` is supported in v1 (reserved for a future EdDSA option) | +| `mcp.accessTokenTtl` | number | `3600` | Access-token lifetime in seconds (default 1 hour) | +| `mcp.refreshTokenTtl` | number | `2592000` | Refresh-token (family) lifetime in seconds (default 30 days) | Sensitive leaves inside `mcp` support `${ENV_VAR}` expansion (e.g., `initialAccessToken: ${OAUTH_MCP_REGISTRATION_TOKEN}`), the same way provider credentials do. diff --git a/docs/mcp-oauth.md b/docs/mcp-oauth.md index 25fae99..827e8ba 100644 --- a/docs/mcp-oauth.md +++ b/docs/mcp-oauth.md @@ -636,6 +636,7 @@ mcp: clientCredentials: enabled: true accessTokenTtl: 300 # default; agents re-mint on 401 + rateLimit: 30 # default; issuance requests/min per client_id, false disables ``` Agents don't register. Each agent's `client_id` is an HTTPS URL to a CIMD @@ -707,6 +708,16 @@ is the document-declared `scope`; a `scope` parameter on the token request is not honored (a client can never escalate past its registered scope, and RFC 6749 §3.3 downscoping-on-request is future work). +Issuance is **rate-limited per `client_id`** (`mcp.clientCredentials.rateLimit`, +default 30 requests/min, `false` disables): over-limit requests receive `429` +with `error: "slow_down"` and a `Retry-After` header (seconds until a retry can +succeed), and are rejected **before** any client resolution or crypto work. +CIMD metadata fetches are separately limited at a fixed 10 attempts/min per +`client_id` URL — cache hits don't consume, so only failing documents repeat. +Both limits are per-node token buckets (a replicated counter would be a +hot-write anti-pattern; the assertion replay guard and ≤60s window bound +cross-node abuse). + Key rotation / revocation semantics: the fleet rotates a key by updating the agent's metadata document. The change takes effect within the CIMD cache TTL (up to 24 h, typically 1 h — bound it with `Cache-Control: max-age` on the diff --git a/src/lib/mcp/cimd.ts b/src/lib/mcp/cimd.ts index ea4a639..88ad503 100644 --- a/src/lib/mcp/cimd.ts +++ b/src/lib/mcp/cimd.ts @@ -54,8 +54,10 @@ * protection (an attacker's document must not be able to force a fetch * per authorize request). * - Failures are NOT cached (the CIMD draft forbids caching error responses - * and invalid documents); repeated fetches of bad documents are left to - * rate limiting (#163). + * and invalid documents); repeated fetch attempts are instead rate-limited + * per client_id URL (fixed 10/min token bucket, #163) — legit clients hit + * the cache after their first success, so only failures repeat. Issuance + * itself is separately rate-limited at the client_credentials grant. * - Cached records are revalidated against the live redirect-host policy on * every hit, so tightening `allowedRedirectUriHosts` takes effect * immediately instead of after cache expiry (up to 24 h). @@ -76,6 +78,7 @@ import { request as httpsRequest } from 'node:https'; import { Readable } from 'node:stream'; import type { Logger, MCPClientIdMetadataDocumentsConfig, MCPClientRecord, MCPConfig } from '../../types.ts'; import { MCPClientStore } from './clientStore.ts'; +import { createRateLimiter } from './rateLimit.ts'; import { validateGrantTypes, validateRedirectUri, @@ -117,6 +120,16 @@ const cimdCache = new Map(); // requests for one uncached URL trigger a single fetch (thundering-herd guard). const inFlightResolutions = new Map>(); +// Per-URL fetch-attempt rate limit (#163) — fixed policy, no config knob: +// legit clients hit the cache after their first success (only failures +// repeat, and failures are never cached per the CIMD draft). Per-node +// semantics: see rateLimit.ts module header. +const CIMD_FETCH_ATTEMPTS_PER_MINUTE = 10; +const cimdFetchLimiter = createRateLimiter({ + capacity: CIMD_FETCH_ATTEMPTS_PER_MINUTE, + refillPerMinute: CIMD_FETCH_ATTEMPTS_PER_MINUTE, +}); + /** * Thrown when CIMD resolution fails due to a client-side issue (bad URL, * invalid document, unsupported auth method, allowedHosts policy). @@ -210,9 +223,12 @@ export function _setFetch(fn: CimdFetch | null): void { _fetch = fn ?? pinnedHttpsFetch; } -/** Clear the CIMD cache (for testing). @internal */ +/** Clear the CIMD cache AND the per-URL fetch limiter (for testing) — tests + * that loop many resolution attempts against one URL rely on the limiter + * resetting alongside the cache. @internal */ export function _clearCimdCache(): void { cimdCache.clear(); + cimdFetchLimiter._reset(); } // --- SSRF guard helpers --- @@ -844,6 +860,16 @@ export async function resolveCimdClient( // Dedup concurrent resolutions of the same uncached client_id → single fetch. const existing = inFlightResolutions.get(clientId); if (existing) return existing; + // Per-URL fetch rate limit (#163): only actual fetch attempts consume — + // cache hits returned above and dedup'd joiners never reach this. Fixed + // policy (no knob): legit clients are served from the cache after their + // first success; repeated attempts are the bad-document amplification + // vector this closes. + const rateLimit = cimdFetchLimiter.tryTake(clientId); + if (!rateLimit.allowed) { + logger?.warn?.(`CIMD: fetch rate limit reached for ${clientId}`); + throw new CimdClientError('temporarily_unavailable', 'CIMD resolution rate limit reached; retry shortly'); + } // Total-concurrency bound: the in-flight map IS the counter (no await sits // between this check and the set below, so the cap cannot be raced past). if (inFlightResolutions.size >= MAX_CONCURRENT_RESOLUTIONS) { @@ -977,8 +1003,9 @@ async function fetchAndValidateCimd( return record; } catch (err) { // Failures are never cached — the CIMD draft forbids caching error - // responses and invalid documents. Fetch amplification from repeated - // bad requests is rate limiting's job (#163). + // responses and invalid documents. Amplification from repeated bad + // requests is bounded by the per-URL fetch rate limit in + // `resolveCimdClient` (#163) instead. if (err instanceof CimdClientError) { logger?.warn?.(`CIMD: rejected client ${clientId}: ${err.message}`); throw err; diff --git a/src/lib/mcp/rateLimit.ts b/src/lib/mcp/rateLimit.ts new file mode 100644 index 0000000..93f854c --- /dev/null +++ b/src/lib/mcp/rateLimit.ts @@ -0,0 +1,79 @@ +/** + * Per-node in-memory token-bucket rate limiter (#163). + * + * Deliberately PER-NODE, not replicated: Harper replication makes a shared + * counter table a hot-write anti-pattern, and the surfaces this limits are + * already bounded cross-node — the client_credentials grant by the assertion + * replay guard and its ≤60s `exp` window, CIMD fetches by the concurrency + * caps. A node-local bucket is the right defense-in-depth without turning + * every token request into a replicated write. + * + * Buckets refill continuously (elapsed-time based — no interval timers), so a + * limit of N/min admits a burst of up to N and then one request per 60/N + * seconds. Keys are ATTACKER-CHOSEN strings (client_ids, URLs), so the key + * space is LRU-bounded: at `maxKeys` the least-recently-used bucket is + * evicted. An evicted bucket forgets that key's spend history — acceptable: + * eviction requires `maxKeys` distinct keys inside one window, and the + * global concurrency caps still bound aggregate work. + */ + +type BucketState = { tokens: number; lastRefill: number }; + +export type RateLimitResult = { allowed: true } | { allowed: false; retryAfterSeconds: number }; + +export type RateLimiter = { + tryTake(key: string): RateLimitResult; + /** Drop all bucket state (for testing). @internal */ + _reset(): void; +}; + +const DEFAULT_MAX_KEYS = 10_000; + +export function createRateLimiter(options: { + /** Bucket capacity (maximum burst). */ + capacity: number; + /** Continuous refill rate, tokens per minute. */ + refillPerMinute: number; + /** LRU bound on distinct keys tracked. Default 10 000. */ + maxKeys?: number; + /** Clock override (for testing). Default Date.now. */ + now?: () => number; +}): RateLimiter { + const { capacity, refillPerMinute } = options; + const maxKeys = options.maxKeys ?? DEFAULT_MAX_KEYS; + const now = options.now ?? Date.now; + const buckets = new Map(); + + return { + tryTake(key: string): RateLimitResult { + const at = now(); + let bucket = buckets.get(key); + if (bucket) { + const elapsedMs = at - bucket.lastRefill; + if (elapsedMs > 0) { + bucket.tokens = Math.min(capacity, bucket.tokens + (elapsedMs / 60_000) * refillPerMinute); + bucket.lastRefill = at; + } + // Delete + re-insert so Map iteration order tracks recency (LRU). + buckets.delete(key); + } else { + bucket = { tokens: capacity, lastRefill: at }; + if (buckets.size >= maxKeys) { + const oldest = buckets.keys().next().value; + if (oldest !== undefined) buckets.delete(oldest); + } + } + if (bucket.tokens >= 1) { + bucket.tokens -= 1; + buckets.set(key, bucket); + return { allowed: true }; + } + buckets.set(key, bucket); + const deficit = 1 - bucket.tokens; + return { allowed: false, retryAfterSeconds: Math.ceil((deficit * 60) / refillPerMinute) }; + }, + _reset(): void { + buckets.clear(); + }, + }; +} diff --git a/src/lib/mcp/token.ts b/src/lib/mcp/token.ts index 7b00eca..f148785 100644 --- a/src/lib/mcp/token.ts +++ b/src/lib/mcp/token.ts @@ -19,6 +19,7 @@ import { MCPAuthCodeStore } from './authCodeStore.ts'; import { CimdClientError, resolveClient } from './cimd.ts'; import { CLIENT_ASSERTION_TYPE_JWT_BEARER, verifyClientAssertion } from './clientAssertion.ts'; import { MCPKeyStore } from './keyStore.ts'; +import { createRateLimiter, type RateLimiter } from './rateLimit.ts'; import { hashRefreshToken, makeRefreshToken, @@ -72,6 +73,44 @@ function coerceTtl(value: unknown, fallback: number): number { return Number.isFinite(n) && n > 0 ? n : fallback; } +// --- client_credentials issuance rate limiting (#163) --- + +const RATE_LIMIT_DEFAULT_PER_MINUTE = 30; + +/** + * Resolve `mcp.clientCredentials.rateLimit` to requests/minute or `false` + * (disabled). `false`/`0` (and their env-expanded string forms) disable the + * limiter explicitly; anything non-finite/non-positive falls back to the + * default rather than failing open — mirrors `coerceTtl`. + */ +function resolveRateLimit(value: unknown): number | false { + if (value === false || value === 0 || value === 'false' || value === '0') return false; + if (value === undefined || value === null) return RATE_LIMIT_DEFAULT_PER_MINUTE; + const n = typeof value === 'number' ? value : Number(value); + return Number.isFinite(n) && n > 0 ? n : RATE_LIMIT_DEFAULT_PER_MINUTE; +} + +// Per-node bucket keyed by client_id; memoized on the configured rate so a +// live config change rebuilds it (dropping state — acceptable, the limiter is +// defense-in-depth, not an accounting ledger). Per-node rationale: see +// rateLimit.ts module header. +let grantLimiter: RateLimiter | undefined; +let grantLimiterRate: number | undefined; + +function getGrantLimiter(ratePerMinute: number): RateLimiter { + if (!grantLimiter || grantLimiterRate !== ratePerMinute) { + grantLimiter = createRateLimiter({ capacity: ratePerMinute, refillPerMinute: ratePerMinute }); + grantLimiterRate = ratePerMinute; + } + return grantLimiter; +} + +/** Drop grant-limiter state (for testing). @internal */ +export function _resetGrantRateLimiter(): void { + grantLimiter = undefined; + grantLimiterRate = undefined; +} + /** Does the client's registered grant_types permit refresh tokens? Defaults to true when unspecified (DCR default includes refresh_token). */ function allowsRefresh(client: MCPClientRecord): boolean { return !client.grant_types || client.grant_types.includes('refresh_token'); @@ -475,6 +514,20 @@ async function handleClientCredentialsGrant( ); } + // Issuance rate limit (#163, #159 req 5): checked BEFORE resolveClient so + // an over-limit client cannot trigger CIMD fetches or crypto work. Keyed + // by the requested client_id — attacker-chosen, but the bucket map is + // LRU-bounded and the point is bounding per-identity issuance. + const ratePerMinute = resolveRateLimit(mcpConfig.clientCredentials?.rateLimit); + if (ratePerMinute !== false) { + const limit = getGrantLimiter(ratePerMinute).tryTake(clientId); + if (!limit.allowed) { + const response = errorResponse(429, 'slow_down', 'Token issuance rate limit reached for this client'); + response.headers = { ...response.headers, 'Retry-After': String(limit.retryAfterSeconds) }; + return response; + } + } + let client: MCPClientRecord | null; try { client = await resolveClient(clientId, mcpConfig, logger); diff --git a/src/types.ts b/src/types.ts index 27bed09..a7d22b6 100644 --- a/src/types.ts +++ b/src/types.ts @@ -125,6 +125,13 @@ export interface MCPClientCredentialsConfig { * agents re-mint on 401; no refresh token is ever issued. */ accessTokenTtl?: number; + /** + * Issuance rate limit in requests/minute per client_id (per node — see + * rateLimit.ts). Default: 30. `false` or `0` disables the limiter; + * non-finite/non-positive values fall back to the default. Over-limit + * requests get 429 + `error: "slow_down"` + `Retry-After`. + */ + rateLimit?: number | false; } /** diff --git a/test/lib/mcp/cimd.test.js b/test/lib/mcp/cimd.test.js index bbb0d75..71c6dc7 100644 --- a/test/lib/mcp/cimd.test.js +++ b/test/lib/mcp/cimd.test.js @@ -872,6 +872,84 @@ describe('resolveCimdClient — cache', () => { }); }); +describe('resolveCimdClient — per-URL fetch rate limit (#163)', () => { + beforeEach(() => _clearCimdCache()); + afterEach(() => { + _setDnsLookup(null); + _setFetch(null); + }); + + // Fails validation (client_id mismatch) → never cached → every attempt fetches. + const BAD_DOC = { client_id: 'https://elsewhere.example.com/x.json', client_name: 'x', redirect_uris: [] }; + + it('rejects the 11th fetch attempt for one URL within the window', async () => { + _setDnsLookup(makeDnsOk()); + _setFetch(makeOkFetch(BAD_DOC)); + for (let i = 0; i < 10; i++) { + await assert.rejects( + () => resolveCimdClient(VALID_URL, undefined), + (err) => err instanceof CimdClientError && err.oauthError === 'invalid_client' + ); + } + await assert.rejects( + () => resolveCimdClient(VALID_URL, undefined), + (err) => { + assert.equal(err.oauthError, 'temporarily_unavailable'); + assert.match(err.message, /rate limit reached/); + return true; + } + ); + }); + + it('is per-URL — one throttled URL does not affect another', async () => { + _setDnsLookup(makeDnsOk()); + _setFetch(makeOkFetch(BAD_DOC)); + for (let i = 0; i < 10; i++) { + await resolveCimdClient(VALID_URL, undefined).catch(() => {}); + } + await assert.rejects(() => resolveCimdClient(VALID_URL, undefined), /rate limit reached/); + const OTHER_URL = 'https://other.example.com/client.json'; + _setFetch( + makeOkFetch({ client_id: OTHER_URL, client_name: 'Other', redirect_uris: ['https://other.example.com/cb'] }) + ); + const record = await resolveCimdClient(OTHER_URL, undefined); + assert.ok(record, 'a different client_id URL has its own bucket'); + }); + + it('cache hits do not consume fetch-attempt tokens', async () => { + _setDnsLookup(makeDnsOk()); + _setFetch(makeOkFetch(VALID_DOC)); + await resolveCimdClient(VALID_URL, undefined); + for (let i = 0; i < 30; i++) { + const record = await resolveCimdClient(VALID_URL, undefined); + assert.ok(record, 'cached resolutions are never rate-limited'); + } + }); + + it('deduped concurrent resolutions consume a single attempt', async () => { + _setDnsLookup(makeDnsOk()); + let release; + const gate = new Promise((resolve) => (release = resolve)); + _setFetch(async (url, init) => { + await gate; + return makeOkFetch(BAD_DOC)(url, init); + }); + const batch = Array.from({ length: 5 }, () => resolveCimdClient(VALID_URL, undefined).catch((err) => err)); + release(); + await Promise.all(batch); + // The 5-caller batch consumed exactly one attempt → 9 more pass the + // limiter (failing validation), and the 11th is rate-limited. + _setFetch(makeOkFetch(BAD_DOC)); + for (let i = 0; i < 9; i++) { + await assert.rejects( + () => resolveCimdClient(VALID_URL, undefined), + (err) => err instanceof CimdClientError && err.oauthError === 'invalid_client' + ); + } + await assert.rejects(() => resolveCimdClient(VALID_URL, undefined), /rate limit reached/); + }); +}); + describe('resolveClient — routing', () => { let originalDatabases; let storedClients; diff --git a/test/lib/mcp/rateLimit.test.js b/test/lib/mcp/rateLimit.test.js new file mode 100644 index 0000000..97d549e --- /dev/null +++ b/test/lib/mcp/rateLimit.test.js @@ -0,0 +1,81 @@ +/** + * Tests for the per-node token-bucket rate limiter (#163). + * The clock is injected — no test sleeps. + */ + +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; +import { createRateLimiter } from '../../../dist/lib/mcp/rateLimit.js'; + +function makeClock(startMs = 0) { + let t = startMs; + return { now: () => t, advance: (ms) => (t += ms) }; +} + +describe('createRateLimiter', () => { + it('admits up to capacity, then blocks with seconds until one token refills', () => { + const clock = makeClock(); + const limiter = createRateLimiter({ capacity: 2, refillPerMinute: 60, now: clock.now }); + assert.equal(limiter.tryTake('k').allowed, true); + assert.equal(limiter.tryTake('k').allowed, true); + const blocked = limiter.tryTake('k'); + assert.equal(blocked.allowed, false); + assert.equal(blocked.retryAfterSeconds, 1); // 60/min = one token per second + }); + + it('refills continuously with elapsed time', () => { + const clock = makeClock(); + const limiter = createRateLimiter({ capacity: 1, refillPerMinute: 60, now: clock.now }); + assert.equal(limiter.tryTake('k').allowed, true); + assert.equal(limiter.tryTake('k').allowed, false); + clock.advance(500); // half a token — still blocked + assert.equal(limiter.tryTake('k').allowed, false); + clock.advance(500); // a full token has refilled + assert.equal(limiter.tryTake('k').allowed, true); + }); + + it('caps refill at capacity — idle time never accrues an unbounded burst', () => { + const clock = makeClock(); + const limiter = createRateLimiter({ capacity: 2, refillPerMinute: 60, now: clock.now }); + limiter.tryTake('k'); + clock.advance(3_600_000); // an hour idle + assert.equal(limiter.tryTake('k').allowed, true); + assert.equal(limiter.tryTake('k').allowed, true); + assert.equal(limiter.tryTake('k').allowed, false, 'burst is bounded by capacity, not elapsed time'); + }); + + it('isolates keys — a drained key does not starve another', () => { + const clock = makeClock(); + const limiter = createRateLimiter({ capacity: 1, refillPerMinute: 1, now: clock.now }); + assert.equal(limiter.tryTake('noisy').allowed, true); + assert.equal(limiter.tryTake('noisy').allowed, false); + assert.equal(limiter.tryTake('quiet').allowed, true); + }); + + it('LRU-bounds the key space — a unique-key flood evicts the oldest bucket', () => { + const clock = makeClock(); + const limiter = createRateLimiter({ capacity: 1, refillPerMinute: 1, maxKeys: 2, now: clock.now }); + assert.equal(limiter.tryTake('a').allowed, true); + assert.equal(limiter.tryTake('a').allowed, false); // 'a' drained + limiter.tryTake('b'); + limiter.tryTake('c'); // map at maxKeys → evicts 'a' (least recently used) + assert.equal(limiter.tryTake('a').allowed, true, 'an evicted key returns with a fresh bucket'); + }); + + it('retryAfterSeconds reflects the deficit at slow refill rates', () => { + const clock = makeClock(); + const limiter = createRateLimiter({ capacity: 1, refillPerMinute: 2, now: clock.now }); + limiter.tryTake('k'); + const blocked = limiter.tryTake('k'); + assert.equal(blocked.allowed, false); + assert.equal(blocked.retryAfterSeconds, 30); // 2/min = one token per 30 s + }); + + it('_reset drops all bucket state', () => { + const limiter = createRateLimiter({ capacity: 1, refillPerMinute: 1 }); + limiter.tryTake('k'); + assert.equal(limiter.tryTake('k').allowed, false); + limiter._reset(); + assert.equal(limiter.tryTake('k').allowed, true); + }); +}); diff --git a/test/lib/mcp/token.test.js b/test/lib/mcp/token.test.js index 92f19b4..cde0628 100644 --- a/test/lib/mcp/token.test.js +++ b/test/lib/mcp/token.test.js @@ -7,7 +7,7 @@ import { describe, it, before, after, beforeEach } from 'node:test'; import assert from 'node:assert/strict'; import { createHash, generateKeyPairSync, randomBytes, sign } from 'node:crypto'; -import { handleToken } from '../../../dist/lib/mcp/token.js'; +import { handleToken, _resetGrantRateLimiter } from '../../../dist/lib/mcp/token.js'; import { resetMCPAssertionJtisTableCache } from '../../../dist/lib/mcp/assertionJtiStore.js'; import { resetMCPAuthCodesTableCache } from '../../../dist/lib/mcp/authCodeStore.js'; import { _clearCimdCache, _setDnsLookup, _setFetch } from '../../../dist/lib/mcp/cimd.js'; @@ -711,6 +711,7 @@ describe('handleToken — client_credentials grant (#162)', () => { resetMCPKeysTableCache(); resetMCPAssertionJtisTableCache(); _clearCimdCache(); + _resetGrantRateLimiter(); hookEvents = []; clients = new Map(); @@ -916,4 +917,55 @@ describe('handleToken — client_credentials grant (#162)', () => { assert.equal(res.status, 400); assert.equal(res.body.error, 'unauthorized_client'); }); + + it('rate-limits issuance per client — 429 + slow_down + Retry-After (#163)', async () => { + const limited = { ...ccConfig, clientCredentials: { ...ccConfig.clientCredentials, rateLimit: 2 } }; + assert.equal((await handleToken({ headers: {} }, grantBody(), limited)).status, 200); + assert.equal((await handleToken({ headers: {} }, grantBody(), limited)).status, 200); + const blocked = await handleToken({ headers: {} }, grantBody(), limited); + assert.equal(blocked.status, 429); + assert.equal(blocked.body.error, 'slow_down'); + assert.ok(Number(blocked.headers['Retry-After']) >= 1, 'Retry-After carries seconds until a token refills'); + }); + + it('rateLimit: false disables the issuance limiter', async () => { + const unlimited = { ...ccConfig, clientCredentials: { ...ccConfig.clientCredentials, rateLimit: false } }; + for (let i = 0; i < 5; i++) { + assert.equal((await handleToken({ headers: {} }, grantBody(), unlimited)).status, 200); + } + }); + + it('the limiter runs before client resolution — an over-limit request triggers no fetch', async () => { + const limited = { ...ccConfig, clientCredentials: { ...ccConfig.clientCredentials, rateLimit: 1 } }; + let fetchCalls = 0; + _setFetch(async () => { + fetchCalls++; + const bytes = Buffer.from(JSON.stringify(AGENT_DOC)); + return { + status: 200, + headers: new Map([ + ['content-type', 'application/json'], + ['content-length', String(bytes.length)], + ]), + body: { + getReader: () => { + let sent = false; + return { + read: async () => + sent ? { done: true, value: undefined } : ((sent = true), { done: false, value: bytes }), + cancel: () => {}, + }; + }, + }, + }; + }); + assert.equal((await handleToken({ headers: {} }, grantBody(), limited)).status, 200); + assert.equal(fetchCalls, 1); + // Clear the CIMD cache so a second request WOULD have to fetch if it got + // past the limiter — proving the 429 short-circuits before resolution. + _clearCimdCache(); + const blocked = await handleToken({ headers: {} }, grantBody(), limited); + assert.equal(blocked.status, 429); + assert.equal(fetchCalls, 1, 'an over-limit request must not reach CIMD resolution'); + }); }); From cdaa77db40fc67623a0fac81ddf489a9e610ffed Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Sat, 11 Jul 2026 12:12:16 -0700 Subject: [PATCH 2/8] refactor(cimd): check concurrency cap before spending a fetch-limiter token; doc the per-thread limit ceiling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cross-model review (domain pass) follow-ups: - Move the MAX_CONCURRENT_RESOLUTIONS check ahead of the per-URL fetch-limiter take so a capacity reject no longer consumes a token without a fetch — makes 'only actual fetch attempts consume' literally true. - Document that the per-node token buckets are in fact per worker thread, so N threads means an N× effective ceiling (inherited from the existing per-thread CIMD cache/concurrency design). Intentional for a defense-in-depth control; docs now say so rather than implying a hard node-wide cap. Both Gemini findings (LRU-reset, per-key 'bypass') were adjudicated by-design/noise and need no change — see PR description. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01SerGP6Am3xz2CKyPgbRc73 --- docs/configuration.md | 42 +++++++++++++++++++++--------------------- docs/mcp-oauth.md | 7 ++++++- src/lib/mcp/cimd.ts | 20 +++++++++++--------- 3 files changed, 38 insertions(+), 31 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 74113aa..8eec471 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -84,27 +84,27 @@ Opt-in support for the Model Context Protocol authorization flow ([issue #86](ht - app.example.com ``` -| Option | Type | Default | Description | -| ------------------------------------------------------- | --------------- | --------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `mcp.enabled` | boolean | `false` | Master switch for the MCP OAuth endpoints | -| `mcp.issuer` | string | (none) | Authorization-server URI advertised in AS metadata. **Required when `mcp.enabled`** — startup fails otherwise, to prevent a Host-header-driven `iss`/`aud` | -| `mcp.resource` | string | `/mcp` | Canonical resource URI advertised in PRM (RFC 9728) and validated as `aud` on issued tokens (RFC 8707). Optional override; defaults safely from the pinned `issuer` | -| `mcp.providers` | string[] | (all providers) | Subset of upstream providers eligible for the MCP auth flow. **v1 requires exactly one** resolved provider — set this when more than one provider is configured globally, otherwise `/oauth/mcp/authorize` returns `server_error` | -| `mcp.dynamicClientRegistration.enabled` | boolean | `true` | Enable the `/register` endpoint when `mcp.enabled` is true | -| `mcp.dynamicClientRegistration.initialAccessToken` | string | (none) | If set, registration requires `Authorization: Bearer `. Otherwise open per RFC 7591 | -| `mcp.dynamicClientRegistration.allowedRedirectUriHosts` | string[] | (none) | Allowlist for redirect_uri hosts. Localhost always allowed per RFC 8252 | -| `mcp.clientIdMetadataDocuments.enabled` | boolean | `true` | Enable CIMD resolution for URL-shaped `client_id` values. Disable with `false` to reject all CIMD clients; see [Client ID Metadata Documents](./mcp-oauth.md#client-id-metadata-documents-cimd) | -| `mcp.clientIdMetadataDocuments.allowedHosts` | string[] | (none) | If set, only CIMD `client_id` URLs whose hostname is in this list are resolved. Others are silently rejected (`invalid_client`) without revealing the allowlist | -| `mcp.clientIdMetadataDocuments.fetchTimeoutMs` | number | `5000` | Deadline for CIMD document retrieval covering DNS, connect, and body read (milliseconds). Non-finite or non-positive values fall back to the default | -| `mcp.clientIdMetadataDocuments.maxDocumentBytes` | number | `65536` | Maximum CIMD document size in bytes (64 KB default). Responses exceeding this limit are rejected. Non-finite or non-positive values fall back to the default | -| `mcp.clientCredentials.enabled` | boolean | `false` | Enable the RFC 7523 `client_credentials` grant (`private_key_jwt`, EdDSA) for headless agents. Explicit opt-in; requires a non-empty `mcp.clientIdMetadataDocuments.allowedHosts` allowlist, CIMD enabled, and an `https:` `mcp.issuer` (RFC 6749 §3.2 — the token endpoint must be TLS; `http:` is permitted only for loopback development issuers) — all enforced at startup. See [Headless agents](./mcp-oauth.md#headless-agents-client_credentials) | -| `mcp.clientCredentials.accessTokenTtl` | number | `300` | Access-token lifetime in seconds for the `client_credentials` grant. No refresh token is ever issued — agents re-mint on 401. Non-finite or non-positive values fall back to the default | -| `mcp.clientCredentials.rateLimit` | number \| false | `30` | Issuance rate limit in requests/minute per `client_id` (token bucket, per node — a shared counter table would be a replication hot-write anti-pattern; the assertion replay guard + 60s window bound cross-node abuse). Over-limit requests get `429` with `error: "slow_down"` and a `Retry-After` header. `false` or `0` disables; non-finite/non-positive values fall back to the default. CIMD metadata fetches are separately limited at a fixed 10 attempts/min per client_id URL (not configurable) | -| `mcp.signingKeyPem` | string | (generated) | PEM-encoded RS256 private key (PKCS#8) used to sign access tokens. When set, this key **always** wins as the signer — it is found in the key set by material match, or persisted on first use (under a deterministic kid so concurrent cluster nodes are idempotent). When unset, a UUID-kid keypair is generated on first boot. Because all persisted keys are published in the JWKS, tokens signed by any node verify everywhere — pinning is **recommended** for clusters but not strictly required | -| `mcp.keyRotationInterval` | number | `0` (disabled) | Signing-key rotation period in seconds. When `> 0`, a fresh UUID-kid keypair is generated at token-mint time once the current signer is older than this interval. Old keys are kept in the JWKS and deleted lazily once `2 × accessTokenTtl` has passed since their immediate successor was created (covering replication lag). Rotation is skipped while `signingKeyPem` is set — setting both emits a startup warning | -| `mcp.signingAlgorithm` | string | `RS256` | JWT signing algorithm. Only `RS256` is supported in v1 (reserved for a future EdDSA option) | -| `mcp.accessTokenTtl` | number | `3600` | Access-token lifetime in seconds (default 1 hour) | -| `mcp.refreshTokenTtl` | number | `2592000` | Refresh-token (family) lifetime in seconds (default 30 days) | +| Option | Type | Default | Description | +| ------------------------------------------------------- | --------------- | --------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `mcp.enabled` | boolean | `false` | Master switch for the MCP OAuth endpoints | +| `mcp.issuer` | string | (none) | Authorization-server URI advertised in AS metadata. **Required when `mcp.enabled`** — startup fails otherwise, to prevent a Host-header-driven `iss`/`aud` | +| `mcp.resource` | string | `/mcp` | Canonical resource URI advertised in PRM (RFC 9728) and validated as `aud` on issued tokens (RFC 8707). Optional override; defaults safely from the pinned `issuer` | +| `mcp.providers` | string[] | (all providers) | Subset of upstream providers eligible for the MCP auth flow. **v1 requires exactly one** resolved provider — set this when more than one provider is configured globally, otherwise `/oauth/mcp/authorize` returns `server_error` | +| `mcp.dynamicClientRegistration.enabled` | boolean | `true` | Enable the `/register` endpoint when `mcp.enabled` is true | +| `mcp.dynamicClientRegistration.initialAccessToken` | string | (none) | If set, registration requires `Authorization: Bearer `. Otherwise open per RFC 7591 | +| `mcp.dynamicClientRegistration.allowedRedirectUriHosts` | string[] | (none) | Allowlist for redirect_uri hosts. Localhost always allowed per RFC 8252 | +| `mcp.clientIdMetadataDocuments.enabled` | boolean | `true` | Enable CIMD resolution for URL-shaped `client_id` values. Disable with `false` to reject all CIMD clients; see [Client ID Metadata Documents](./mcp-oauth.md#client-id-metadata-documents-cimd) | +| `mcp.clientIdMetadataDocuments.allowedHosts` | string[] | (none) | If set, only CIMD `client_id` URLs whose hostname is in this list are resolved. Others are silently rejected (`invalid_client`) without revealing the allowlist | +| `mcp.clientIdMetadataDocuments.fetchTimeoutMs` | number | `5000` | Deadline for CIMD document retrieval covering DNS, connect, and body read (milliseconds). Non-finite or non-positive values fall back to the default | +| `mcp.clientIdMetadataDocuments.maxDocumentBytes` | number | `65536` | Maximum CIMD document size in bytes (64 KB default). Responses exceeding this limit are rejected. Non-finite or non-positive values fall back to the default | +| `mcp.clientCredentials.enabled` | boolean | `false` | Enable the RFC 7523 `client_credentials` grant (`private_key_jwt`, EdDSA) for headless agents. Explicit opt-in; requires a non-empty `mcp.clientIdMetadataDocuments.allowedHosts` allowlist, CIMD enabled, and an `https:` `mcp.issuer` (RFC 6749 §3.2 — the token endpoint must be TLS; `http:` is permitted only for loopback development issuers) — all enforced at startup. See [Headless agents](./mcp-oauth.md#headless-agents-client_credentials) | +| `mcp.clientCredentials.accessTokenTtl` | number | `300` | Access-token lifetime in seconds for the `client_credentials` grant. No refresh token is ever issued — agents re-mint on 401. Non-finite or non-positive values fall back to the default | +| `mcp.clientCredentials.rateLimit` | number \| false | `30` | Issuance rate limit in requests/minute per `client_id` (token bucket, per node, and in fact per worker thread — N threads means an N× effective ceiling; a shared counter table would be a replication hot-write anti-pattern, and the assertion replay guard + 60s window bound cross-node abuse). Over-limit requests get `429` with `error: "slow_down"` and a `Retry-After` header. `false` or `0` disables; non-finite/non-positive values fall back to the default. CIMD metadata fetches are separately limited at a fixed 10 attempts/min per client_id URL (not configurable) | +| `mcp.signingKeyPem` | string | (generated) | PEM-encoded RS256 private key (PKCS#8) used to sign access tokens. When set, this key **always** wins as the signer — it is found in the key set by material match, or persisted on first use (under a deterministic kid so concurrent cluster nodes are idempotent). When unset, a UUID-kid keypair is generated on first boot. Because all persisted keys are published in the JWKS, tokens signed by any node verify everywhere — pinning is **recommended** for clusters but not strictly required | +| `mcp.keyRotationInterval` | number | `0` (disabled) | Signing-key rotation period in seconds. When `> 0`, a fresh UUID-kid keypair is generated at token-mint time once the current signer is older than this interval. Old keys are kept in the JWKS and deleted lazily once `2 × accessTokenTtl` has passed since their immediate successor was created (covering replication lag). Rotation is skipped while `signingKeyPem` is set — setting both emits a startup warning | +| `mcp.signingAlgorithm` | string | `RS256` | JWT signing algorithm. Only `RS256` is supported in v1 (reserved for a future EdDSA option) | +| `mcp.accessTokenTtl` | number | `3600` | Access-token lifetime in seconds (default 1 hour) | +| `mcp.refreshTokenTtl` | number | `2592000` | Refresh-token (family) lifetime in seconds (default 30 days) | Sensitive leaves inside `mcp` support `${ENV_VAR}` expansion (e.g., `initialAccessToken: ${OAUTH_MCP_REGISTRATION_TOKEN}`), the same way provider credentials do. diff --git a/docs/mcp-oauth.md b/docs/mcp-oauth.md index 827e8ba..5348c71 100644 --- a/docs/mcp-oauth.md +++ b/docs/mcp-oauth.md @@ -716,7 +716,12 @@ CIMD metadata fetches are separately limited at a fixed 10 attempts/min per `client_id` URL — cache hits don't consume, so only failing documents repeat. Both limits are per-node token buckets (a replicated counter would be a hot-write anti-pattern; the assertion replay guard and ≤60s window bound -cross-node abuse). +cross-node abuse). The bucket state is per worker thread: if the plugin runs +across N HTTP worker threads on a node, the effective ceiling is N × the +configured limit per `client_id` (as with the CIMD fetch cache and concurrency +caps, which are likewise per-thread). This is intentional for a defense-in-depth +control — treat the configured value as a per-thread floor, not a hard node-wide +cap. Key rotation / revocation semantics: the fleet rotates a key by updating the agent's metadata document. The change takes effect within the CIMD cache TTL diff --git a/src/lib/mcp/cimd.ts b/src/lib/mcp/cimd.ts index 88ad503..c8e3f9d 100644 --- a/src/lib/mcp/cimd.ts +++ b/src/lib/mcp/cimd.ts @@ -860,21 +860,23 @@ export async function resolveCimdClient( // Dedup concurrent resolutions of the same uncached client_id → single fetch. const existing = inFlightResolutions.get(clientId); if (existing) return existing; + // Total-concurrency bound: the in-flight map IS the counter (no await sits + // between this check and the set below, so the cap cannot be raced past). + // Checked BEFORE the rate-limit take so a capacity reject doesn't spend a + // token without a fetch (keeps "only actual fetch attempts consume" true). + if (inFlightResolutions.size >= MAX_CONCURRENT_RESOLUTIONS) { + throw new CimdClientError('temporarily_unavailable', 'CIMD resolution capacity reached; retry shortly'); + } // Per-URL fetch rate limit (#163): only actual fetch attempts consume — - // cache hits returned above and dedup'd joiners never reach this. Fixed - // policy (no knob): legit clients are served from the cache after their - // first success; repeated attempts are the bad-document amplification - // vector this closes. + // cache hits returned above, dedup'd joiners, and capacity rejects never + // reach this. Fixed policy (no knob): legit clients are served from the + // cache after their first success; repeated attempts are the bad-document + // amplification vector this closes. const rateLimit = cimdFetchLimiter.tryTake(clientId); if (!rateLimit.allowed) { logger?.warn?.(`CIMD: fetch rate limit reached for ${clientId}`); throw new CimdClientError('temporarily_unavailable', 'CIMD resolution rate limit reached; retry shortly'); } - // Total-concurrency bound: the in-flight map IS the counter (no await sits - // between this check and the set below, so the cap cannot be raced past). - if (inFlightResolutions.size >= MAX_CONCURRENT_RESOLUTIONS) { - throw new CimdClientError('temporarily_unavailable', 'CIMD resolution capacity reached; retry shortly'); - } const pending = fetchAndValidateCimd(clientId, cimdConfig, allowedRedirectUriHosts, logger).finally(() => inFlightResolutions.delete(clientId) ); From ad30fb2caa2df14b2706534c5884ea3b554fb10b Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Sat, 11 Jul 2026 12:43:02 -0700 Subject: [PATCH 3/8] fix(mcp): clamp sub-1 bucket capacity and cap client_id length MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gemini review follow-ups on #171: - createRateLimiter clamps capacity to >= 1. A configured rate below 1/min gave a burst ceiling under the 1 token a take needs, so the bucket could never admit anyone (worse than the reported 'first request blocked' — it was every request). Refill rate is untouched, so a sub-1/min limit still means one request then one per 60/rate seconds. - Cap client_id at 2048 chars before it becomes a rate-limiter map key (attacker-chosen, retained up to maxKeys). Same defense-in-depth family as the repo's request-path and assertion-length caps. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01SerGP6Am3xz2CKyPgbRc73 --- src/lib/mcp/rateLimit.ts | 6 +++++- src/lib/mcp/token.ts | 12 ++++++++++++ test/lib/mcp/rateLimit.test.js | 13 +++++++++++++ test/lib/mcp/token.test.js | 8 ++++++++ 4 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/lib/mcp/rateLimit.ts b/src/lib/mcp/rateLimit.ts index 93f854c..d61cb8e 100644 --- a/src/lib/mcp/rateLimit.ts +++ b/src/lib/mcp/rateLimit.ts @@ -39,7 +39,11 @@ export function createRateLimiter(options: { /** Clock override (for testing). Default Date.now. */ now?: () => number; }): RateLimiter { - const { capacity, refillPerMinute } = options; + // A token bucket takes 1 token per request, so a burst ceiling below 1 could + // never admit anyone — clamp it up. The refill rate is left untouched, so a + // sub-1/min limit still means "one request, then one per 60/rate seconds". + const capacity = Math.max(1, options.capacity); + const { refillPerMinute } = options; const maxKeys = options.maxKeys ?? DEFAULT_MAX_KEYS; const now = options.now ?? Date.now; const buckets = new Map(); diff --git a/src/lib/mcp/token.ts b/src/lib/mcp/token.ts index f148785..05d8f66 100644 --- a/src/lib/mcp/token.ts +++ b/src/lib/mcp/token.ts @@ -77,6 +77,11 @@ function coerceTtl(value: unknown, fallback: number): number { const RATE_LIMIT_DEFAULT_PER_MINUTE = 30; +// Upper bound on an accepted client_id, applied before it becomes a +// rate-limiter map key. Same defense-in-depth family as the repo's 2048-char +// request-path cap and the assertion/jti length caps in clientAssertion.ts. +const MAX_CLIENT_ID_LENGTH = 2048; + /** * Resolve `mcp.clientCredentials.rateLimit` to requests/minute or `false` * (disabled). `false`/`0` (and their env-expanded string forms) disable the @@ -496,6 +501,13 @@ async function handleClientCredentialsGrant( if (!clientId) { return errorResponse(400, 'invalid_request', 'client_id is required'); } + // Cap the client_id length before it becomes a rate-limiter map key + // (attacker-chosen, retained up to maxKeys entries) — same defense-in-depth + // family as the repo's request-path and assertion-length caps. 2048 covers + // any legitimate CIMD URL or DCR id. + if (clientId.length > MAX_CLIENT_ID_LENGTH) { + return errorResponse(400, 'invalid_request', 'client_id exceeds the maximum length'); + } if (assertionType !== CLIENT_ASSERTION_TYPE_JWT_BEARER) { return errorResponse(400, 'invalid_request', `client_assertion_type must be ${CLIENT_ASSERTION_TYPE_JWT_BEARER}`); } diff --git a/test/lib/mcp/rateLimit.test.js b/test/lib/mcp/rateLimit.test.js index 97d549e..550ad94 100644 --- a/test/lib/mcp/rateLimit.test.js +++ b/test/lib/mcp/rateLimit.test.js @@ -71,6 +71,19 @@ describe('createRateLimiter', () => { assert.equal(blocked.retryAfterSeconds, 30); // 2/min = one token per 30 s }); + it('clamps a sub-1 capacity up to 1 so a slow rate still admits the first request', () => { + const clock = makeClock(); + // rate 0.5/min → capacity would be 0.5, below the 1 token a take needs; + // without the clamp this bucket could never admit anyone. + const limiter = createRateLimiter({ capacity: 0.5, refillPerMinute: 0.5, now: clock.now }); + assert.equal(limiter.tryTake('k').allowed, true, 'first request admitted despite sub-1 rate'); + const blocked = limiter.tryTake('k'); + assert.equal(blocked.allowed, false); + assert.equal(blocked.retryAfterSeconds, 120); // 0.5/min = one token per 120 s + clock.advance(120_000); + assert.equal(limiter.tryTake('k').allowed, true, 'refills at the configured slow rate'); + }); + it('_reset drops all bucket state', () => { const limiter = createRateLimiter({ capacity: 1, refillPerMinute: 1 }); limiter.tryTake('k'); diff --git a/test/lib/mcp/token.test.js b/test/lib/mcp/token.test.js index cde0628..a2c9a07 100644 --- a/test/lib/mcp/token.test.js +++ b/test/lib/mcp/token.test.js @@ -928,6 +928,14 @@ describe('handleToken — client_credentials grant (#162)', () => { assert.ok(Number(blocked.headers['Retry-After']) >= 1, 'Retry-After carries seconds until a token refills'); }); + it('rejects an over-length client_id before it becomes a rate-limiter key', async () => { + const longId = 'https://agents.example.com/' + 'a'.repeat(2100) + '.json'; + const res = await handleToken({ headers: {} }, grantBody({ client_id: longId }), ccConfig); + assert.equal(res.status, 400); + assert.equal(res.body.error, 'invalid_request'); + assert.match(res.body.error_description, /client_id exceeds the maximum length/); + }); + it('rateLimit: false disables the issuance limiter', async () => { const unlimited = { ...ccConfig, clientCredentials: { ...ccConfig.clientCredentials, rateLimit: false } }; for (let i = 0; i < 5; i++) { From 523ce128a96c98f1f7e51d833f1be213ff65e81a Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Sat, 11 Jul 2026 12:53:50 -0700 Subject: [PATCH 4/8] fix(mcp): move issuance limiter post-auth; fingerprint limiter keys Codex review (#171 request-changes): - #1 (medium): the issuance limiter debited a bucket keyed by the submitted client_id BEFORE verification. Since CIMD client_ids are public URLs, any caller could flood a known agent's URL with a bogus assertion and 429 the real agent before its valid assertion was checked. Move the limiter to AFTER proof-of-possession, keyed by the verified client_id. Pre-auth work stays bounded by the per-URL CIMD fetch limiter + resolution/DNS concurrency caps (signature verify is CPU-only, no jti burn). Regression test: forged assertions for a victim's client_id can't drain its quota. - #2 (medium): the LRU map retained raw keys, so maxKeys bounded entry COUNT not bytes. Store a SHA-256 fingerprint as the bucket key; memory is now maxKeys x constant regardless of key length. Long-unique-key flood test added. - #3 (low): fractional rates already clamped in ad30fb2 (capacity = max(1, rate), fractional refill preserved). Also folds in two gemini nits: consolidated the redundant buckets.set in tryTake, and the client_id length cap (ad30fb2) bounds pre-hash input. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01SerGP6Am3xz2CKyPgbRc73 --- src/lib/mcp/rateLimit.ts | 35 ++++++++++++++++--------- src/lib/mcp/token.ts | 31 ++++++++++++---------- test/lib/mcp/rateLimit.test.js | 13 +++++++++ test/lib/mcp/token.test.js | 48 ++++++++++++---------------------- 4 files changed, 69 insertions(+), 58 deletions(-) diff --git a/src/lib/mcp/rateLimit.ts b/src/lib/mcp/rateLimit.ts index d61cb8e..4261930 100644 --- a/src/lib/mcp/rateLimit.ts +++ b/src/lib/mcp/rateLimit.ts @@ -10,13 +10,17 @@ * * Buckets refill continuously (elapsed-time based — no interval timers), so a * limit of N/min admits a burst of up to N and then one request per 60/N - * seconds. Keys are ATTACKER-CHOSEN strings (client_ids, URLs), so the key - * space is LRU-bounded: at `maxKeys` the least-recently-used bucket is - * evicted. An evicted bucket forgets that key's spend history — acceptable: - * eviction requires `maxKeys` distinct keys inside one window, and the - * global concurrency caps still bound aggregate work. + * seconds. Keys may be ATTACKER-CHOSEN strings (client_ids, URLs), so: (1) the + * key space is LRU-bounded — at `maxKeys` the least-recently-used bucket is + * evicted (an evicted bucket forgets that key's spend history, acceptable + * because eviction requires `maxKeys` distinct keys inside one window and the + * global concurrency caps still bound aggregate work); and (2) the key is + * stored as a fixed-size SHA-256 fingerprint, so a flood of long distinct keys + * bounds map memory to `maxKeys` × a constant, not × the raw key length. */ +import { createHash } from 'node:crypto'; + type BucketState = { tokens: number; lastRefill: number }; export type RateLimitResult = { allowed: true } | { allowed: false; retryAfterSeconds: number }; @@ -50,16 +54,19 @@ export function createRateLimiter(options: { return { tryTake(key: string): RateLimitResult { + // Fixed-size fingerprint: the raw key may be an attacker-chosen URL of + // arbitrary length, and it is retained in the map until evicted. + const mapKey = createHash('sha256').update(key).digest('base64url'); const at = now(); - let bucket = buckets.get(key); + let bucket = buckets.get(mapKey); if (bucket) { const elapsedMs = at - bucket.lastRefill; if (elapsedMs > 0) { bucket.tokens = Math.min(capacity, bucket.tokens + (elapsedMs / 60_000) * refillPerMinute); bucket.lastRefill = at; } - // Delete + re-insert so Map iteration order tracks recency (LRU). - buckets.delete(key); + // Delete now; the set below re-inserts so Map order tracks recency (LRU). + buckets.delete(mapKey); } else { bucket = { tokens: capacity, lastRefill: at }; if (buckets.size >= maxKeys) { @@ -67,14 +74,16 @@ export function createRateLimiter(options: { if (oldest !== undefined) buckets.delete(oldest); } } + let result: RateLimitResult; if (bucket.tokens >= 1) { bucket.tokens -= 1; - buckets.set(key, bucket); - return { allowed: true }; + result = { allowed: true }; + } else { + const deficit = 1 - bucket.tokens; + result = { allowed: false, retryAfterSeconds: Math.ceil((deficit * 60) / refillPerMinute) }; } - buckets.set(key, bucket); - const deficit = 1 - bucket.tokens; - return { allowed: false, retryAfterSeconds: Math.ceil((deficit * 60) / refillPerMinute) }; + buckets.set(mapKey, bucket); + return result; }, _reset(): void { buckets.clear(); diff --git a/src/lib/mcp/token.ts b/src/lib/mcp/token.ts index 05d8f66..1d0484c 100644 --- a/src/lib/mcp/token.ts +++ b/src/lib/mcp/token.ts @@ -526,20 +526,6 @@ async function handleClientCredentialsGrant( ); } - // Issuance rate limit (#163, #159 req 5): checked BEFORE resolveClient so - // an over-limit client cannot trigger CIMD fetches or crypto work. Keyed - // by the requested client_id — attacker-chosen, but the bucket map is - // LRU-bounded and the point is bounding per-identity issuance. - const ratePerMinute = resolveRateLimit(mcpConfig.clientCredentials?.rateLimit); - if (ratePerMinute !== false) { - const limit = getGrantLimiter(ratePerMinute).tryTake(clientId); - if (!limit.allowed) { - const response = errorResponse(429, 'slow_down', 'Token issuance rate limit reached for this client'); - response.headers = { ...response.headers, 'Retry-After': String(limit.retryAfterSeconds) }; - return response; - } - } - let client: MCPClientRecord | null; try { client = await resolveClient(clientId, mcpConfig, logger); @@ -581,6 +567,23 @@ async function handleClientCredentialsGrant( return errorResponse(401, 'invalid_client', `client_assertion verification failed: ${result.reason}`); } + // Issuance rate limit (#163, #159 req 5): applied AFTER proof-of-possession, + // keyed by the now-verified client_id. Running it pre-auth would let any + // caller drain a real agent's quota by replaying the agent's PUBLIC CIMD + // client_id URL with a bogus assertion (429 the victim before its valid + // assertion is even checked). Pre-auth work is bounded elsewhere: CIMD + // fetches by the per-URL fetch limiter + resolution/DNS concurrency caps, + // signature verification by Node's request concurrency (no I/O, no jti burn). + const ratePerMinute = resolveRateLimit(mcpConfig.clientCredentials?.rateLimit); + if (ratePerMinute !== false) { + const limit = getGrantLimiter(ratePerMinute).tryTake(clientId); + if (!limit.allowed) { + const response = errorResponse(429, 'slow_down', 'Token issuance rate limit reached for this client'); + response.headers = { ...response.headers, 'Retry-After': String(limit.retryAfterSeconds) }; + return response; + } + } + // RFC 8707 resource binding: exact match against the canonical MCP // resource, fail closed — no prefix or wildcard comparisons (#159 req 3). // Checked BEFORE the jti is consumed: a recoverable request-param mistake diff --git a/test/lib/mcp/rateLimit.test.js b/test/lib/mcp/rateLimit.test.js index 550ad94..32437a3 100644 --- a/test/lib/mcp/rateLimit.test.js +++ b/test/lib/mcp/rateLimit.test.js @@ -84,6 +84,19 @@ describe('createRateLimiter', () => { assert.equal(limiter.tryTake('k').allowed, true, 'refills at the configured slow rate'); }); + it('bounds memory under a long, unique-key flood — keys are fingerprinted, LRU still evicts', () => { + const clock = makeClock(); + const limiter = createRateLimiter({ capacity: 1, refillPerMinute: 1, maxKeys: 3, now: clock.now }); + // Keys of arbitrary length must not grow the map beyond maxKeys, and each + // distinct key still gets its own bucket (fingerprints don't collide). + const long = (n) => `https://attacker.example.com/${'x'.repeat(4000)}/${n}`; + for (let i = 0; i < 1000; i++) assert.equal(limiter.tryTake(long(i)).allowed, true); + // The three most-recent keys retain state; older ones were evicted, so a + // re-request of a recent drained key is still blocked (state survived). + assert.equal(limiter.tryTake(long(999)).allowed, false, 'recent key keeps its drained bucket'); + assert.equal(limiter.tryTake(long(0)).allowed, true, 'long-evicted key returns fresh'); + }); + it('_reset drops all bucket state', () => { const limiter = createRateLimiter({ capacity: 1, refillPerMinute: 1 }); limiter.tryTake('k'); diff --git a/test/lib/mcp/token.test.js b/test/lib/mcp/token.test.js index a2c9a07..49d6961 100644 --- a/test/lib/mcp/token.test.js +++ b/test/lib/mcp/token.test.js @@ -943,37 +943,23 @@ describe('handleToken — client_credentials grant (#162)', () => { } }); - it('the limiter runs before client resolution — an over-limit request triggers no fetch', async () => { - const limited = { ...ccConfig, clientCredentials: { ...ccConfig.clientCredentials, rateLimit: 1 } }; - let fetchCalls = 0; - _setFetch(async () => { - fetchCalls++; - const bytes = Buffer.from(JSON.stringify(AGENT_DOC)); - return { - status: 200, - headers: new Map([ - ['content-type', 'application/json'], - ['content-length', String(bytes.length)], - ]), - body: { - getReader: () => { - let sent = false; - return { - read: async () => - sent ? { done: true, value: undefined } : ((sent = true), { done: false, value: bytes }), - cancel: () => {}, - }; - }, - }, - }; - }); + it('bogus assertions cannot drain a real client’s issuance quota (limiter is post-auth)', async () => { + // The limiter is keyed by the client_id but debited only AFTER + // proof-of-possession. An attacker who knows the victim’s public CIMD + // client_id but not its key can flood the endpoint; every request fails + // verification (401) and none touches the victim’s bucket. + const limited = { ...ccConfig, clientCredentials: { ...ccConfig.clientCredentials, rateLimit: 2 } }; + const rogue = generateKeyPairSync('ed25519'); + for (let i = 0; i < 5; i++) { + const forged = await handleToken( + { headers: {} }, + grantBody({ client_assertion: signAssertion({ key: rogue.privateKey }) }), + limited + ); + assert.equal(forged.status, 401, 'forged assertion rejected'); + } + // The genuine agent’s quota is untouched: two valid mints still succeed. + assert.equal((await handleToken({ headers: {} }, grantBody(), limited)).status, 200); assert.equal((await handleToken({ headers: {} }, grantBody(), limited)).status, 200); - assert.equal(fetchCalls, 1); - // Clear the CIMD cache so a second request WOULD have to fetch if it got - // past the limiter — proving the 429 short-circuits before resolution. - _clearCimdCache(); - const blocked = await handleToken({ headers: {} }, grantBody(), limited); - assert.equal(blocked.status, 429); - assert.equal(fetchCalls, 1, 'an over-limit request must not reach CIMD resolution'); }); }); From e267f9193df6fe786815bb1460feafeff48a9d45 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Sat, 11 Jul 2026 13:00:56 -0700 Subject: [PATCH 5/8] docs(mcp): correct the rate-limiter ordering claim after the post-auth move MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The doc still said issuance was limited 'before any client resolution or crypto work' — stale after 523ce12 moved the limiter to after assertion verification. Rewrite to describe the actual (and safer) ordering: debited post-verification so bogus assertions can't drain a real agent's quota, with pre-auth work bounded by the CIMD fetch limiter + concurrency caps. Claude review nit. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01SerGP6Am3xz2CKyPgbRc73 --- docs/mcp-oauth.md | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/docs/mcp-oauth.md b/docs/mcp-oauth.md index 5348c71..bf52cf9 100644 --- a/docs/mcp-oauth.md +++ b/docs/mcp-oauth.md @@ -711,9 +711,13 @@ RFC 6749 §3.3 downscoping-on-request is future work). Issuance is **rate-limited per `client_id`** (`mcp.clientCredentials.rateLimit`, default 30 requests/min, `false` disables): over-limit requests receive `429` with `error: "slow_down"` and a `Retry-After` header (seconds until a retry can -succeed), and are rejected **before** any client resolution or crypto work. -CIMD metadata fetches are separately limited at a fixed 10 attempts/min per -`client_id` URL — cache hits don't consume, so only failing documents repeat. +succeed). The limit is debited **after** the client assertion is verified, so it +counts only authenticated issuance — a caller cannot drain a real agent's quota +by replaying the agent's public `client_id` URL with a bogus assertion (those +fail verification with `401` and never touch the bucket). Pre-auth work is +bounded separately: CIMD metadata fetches are limited at a fixed 10 attempts/min +per `client_id` URL (cache hits don't consume, so only failing documents +repeat), and resolution/DNS concurrency is capped globally. Both limits are per-node token buckets (a replicated counter would be a hot-write anti-pattern; the assertion replay guard and ≤60s window bound cross-node abuse). The bucket state is per worker thread: if the plugin runs From b221570825810c13d6a9274911bb6bd539553432 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Sat, 11 Jul 2026 13:13:33 -0700 Subject: [PATCH 6/8] feat(mcp): surface 429 + Retry-After on CIMD throttles; centralize client_id cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gemini nits on #171: - The CIMD fetch rate limit now throws slow_down + statusCode 429 + retryAfterSeconds, and both the token endpoint and the authorize handler emit that status with a Retry-After header (via a shared cimdErrorResponse helper on the token side, widened ErrorJSON on the authorize side) — mirroring the issuance limiter instead of the old 401 temporarily_unavailable that misleadingly read as an auth failure. The concurrency cap now also returns 429 (kept its temporarily_unavailable code — server-busy, not client-too-fast). - MAX_CLIENT_ID_LENGTH moved to cimd.ts and exported; resolveClient now rejects an over-length client_id (unknown-client null) before it becomes a fetch-limiter key, and token.ts imports the shared constant. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01SerGP6Am3xz2CKyPgbRc73 --- src/lib/mcp/authorize.ts | 9 ++++++--- src/lib/mcp/cimd.ts | 35 ++++++++++++++++++++++++++++++----- src/lib/mcp/token.ts | 25 +++++++++++++++++-------- test/lib/mcp/cimd.test.js | 6 +++++- 4 files changed, 58 insertions(+), 17 deletions(-) diff --git a/src/lib/mcp/authorize.ts b/src/lib/mcp/authorize.ts index fc88b5a..0038db8 100644 --- a/src/lib/mcp/authorize.ts +++ b/src/lib/mcp/authorize.ts @@ -48,8 +48,9 @@ import { import { resolveIssuer, resolveResource } from './wellKnown.ts'; type ErrorJSON = { - status: 400 | 500; + status: 400 | 429 | 500; body: { error: string; error_description?: string }; + headers?: Record; }; type Redirect = { @@ -384,10 +385,12 @@ export async function handleAuthorize( client = await resolveClient(query.client_id, mcpConfig, logger); } catch (error) { if (error instanceof CimdClientError) { - return { - status: 400, + const json: ErrorJSON = { + status: error.statusCode === 429 ? 429 : 400, body: { error: error.oauthError, error_description: error.message }, }; + if (error.retryAfterSeconds !== undefined) json.headers = { 'Retry-After': String(error.retryAfterSeconds) }; + return json; } logger?.error?.('MCP authorize: client lookup failed:', error instanceof Error ? error.message : String(error)); return { diff --git a/src/lib/mcp/cimd.ts b/src/lib/mcp/cimd.ts index c8e3f9d..1aadf11 100644 --- a/src/lib/mcp/cimd.ts +++ b/src/lib/mcp/cimd.ts @@ -130,19 +130,36 @@ const cimdFetchLimiter = createRateLimiter({ refillPerMinute: CIMD_FETCH_ATTEMPTS_PER_MINUTE, }); +/** + * Upper bound on a client_id length, shared by every entry point that uses a + * caller-supplied client_id as a lookup or rate-limiter map key. Same + * defense-in-depth family as the repo's 2048-char request-path cap. + */ +export const MAX_CLIENT_ID_LENGTH = 2048; + /** * Thrown when CIMD resolution fails due to a client-side issue (bad URL, * invalid document, unsupported auth method, allowedHosts policy). * Callers should surface this as the given `oauthError` with the `message` - * as the `error_description`. + * as the `error_description`. Throttle rejections additionally carry a + * `statusCode` (429) and `retryAfterSeconds` so callers can emit a proper + * `Retry-After` instead of a misleading auth error. */ export class CimdClientError extends Error { readonly oauthError: string; - - constructor(oauthError: string, message: string, options?: { cause?: unknown }) { + readonly statusCode?: number; + readonly retryAfterSeconds?: number; + + constructor( + oauthError: string, + message: string, + options?: { cause?: unknown; statusCode?: number; retryAfterSeconds?: number } + ) { super(message, options); this.name = 'CimdClientError'; this.oauthError = oauthError; + this.statusCode = options?.statusCode; + this.retryAfterSeconds = options?.retryAfterSeconds; } } @@ -865,7 +882,9 @@ export async function resolveCimdClient( // Checked BEFORE the rate-limit take so a capacity reject doesn't spend a // token without a fetch (keeps "only actual fetch attempts consume" true). if (inFlightResolutions.size >= MAX_CONCURRENT_RESOLUTIONS) { - throw new CimdClientError('temporarily_unavailable', 'CIMD resolution capacity reached; retry shortly'); + throw new CimdClientError('temporarily_unavailable', 'CIMD resolution capacity reached; retry shortly', { + statusCode: 429, + }); } // Per-URL fetch rate limit (#163): only actual fetch attempts consume — // cache hits returned above, dedup'd joiners, and capacity rejects never @@ -875,7 +894,10 @@ export async function resolveCimdClient( const rateLimit = cimdFetchLimiter.tryTake(clientId); if (!rateLimit.allowed) { logger?.warn?.(`CIMD: fetch rate limit reached for ${clientId}`); - throw new CimdClientError('temporarily_unavailable', 'CIMD resolution rate limit reached; retry shortly'); + throw new CimdClientError('slow_down', 'CIMD resolution rate limit reached; retry shortly', { + statusCode: 429, + retryAfterSeconds: rateLimit.retryAfterSeconds, + }); } const pending = fetchAndValidateCimd(clientId, cimdConfig, allowedRedirectUriHosts, logger).finally(() => inFlightResolutions.delete(clientId) @@ -1031,6 +1053,9 @@ export async function resolveClient( mcpConfig: MCPConfig | undefined, logger?: Logger ): Promise { + // An over-length client_id is never a real client — reject before it becomes + // a CIMD fetch-limiter key or a store lookup (unknown-client null, no leak). + if (clientId.length > MAX_CLIENT_ID_LENGTH) return null; const cimdConfig = mcpConfig?.clientIdMetadataDocuments; // CIMD is enabled by default when mcp.enabled; disabled only by explicit `enabled: false`. const cimdEnabled = cimdConfig?.enabled !== false; diff --git a/src/lib/mcp/token.ts b/src/lib/mcp/token.ts index 1d0484c..c60125e 100644 --- a/src/lib/mcp/token.ts +++ b/src/lib/mcp/token.ts @@ -16,7 +16,7 @@ import type { Logger, MCPClientRecord, MCPConfig, Request } from '../../types.ts import { emitMCPAuditEvent } from './audit.ts'; import { MCPAssertionJtiStore } from './assertionJtiStore.ts'; import { MCPAuthCodeStore } from './authCodeStore.ts'; -import { CimdClientError, resolveClient } from './cimd.ts'; +import { CimdClientError, MAX_CLIENT_ID_LENGTH, resolveClient } from './cimd.ts'; import { CLIENT_ASSERTION_TYPE_JWT_BEARER, verifyClientAssertion } from './clientAssertion.ts'; import { MCPKeyStore } from './keyStore.ts'; import { createRateLimiter, type RateLimiter } from './rateLimit.ts'; @@ -58,6 +58,20 @@ function errorResponse(status: number, error: string, description?: string): Tok }; } +/** + * Map a CimdClientError to a token-endpoint response. A throttle rejection + * carries `statusCode` (429) and, for the fetch rate limit, `retryAfterSeconds` + * — surface those (with a `Retry-After` header) instead of the default 401, so + * a rate-limited client backs off rather than treating it as an auth failure. + */ +function cimdErrorResponse(err: CimdClientError): TokenResponse { + const response = errorResponse(err.statusCode ?? 401, err.oauthError, err.message); + if (err.retryAfterSeconds !== undefined) { + response.headers = { ...response.headers, 'Retry-After': String(err.retryAfterSeconds) }; + } + return response; +} + function nowSeconds(): number { return Math.floor(Date.now() / 1000); } @@ -77,11 +91,6 @@ function coerceTtl(value: unknown, fallback: number): number { const RATE_LIMIT_DEFAULT_PER_MINUTE = 30; -// Upper bound on an accepted client_id, applied before it becomes a -// rate-limiter map key. Same defense-in-depth family as the repo's 2048-char -// request-path cap and the assertion/jti length caps in clientAssertion.ts. -const MAX_CLIENT_ID_LENGTH = 2048; - /** * Resolve `mcp.clientCredentials.rateLimit` to requests/minute or `false` * (disabled). `false`/`0` (and their env-expanded string forms) disable the @@ -176,7 +185,7 @@ async function authenticateClient( client = await resolveClient(clientId, mcpConfig, logger); } catch (err) { if (err instanceof CimdClientError) { - return { error: errorResponse(401, err.oauthError, err.message) }; + return { error: cimdErrorResponse(err) }; } logger?.error?.('MCP token: client lookup failed:', err instanceof Error ? err.message : String(err)); return { error: errorResponse(500, 'server_error', 'Client lookup failed') }; @@ -531,7 +540,7 @@ async function handleClientCredentialsGrant( client = await resolveClient(clientId, mcpConfig, logger); } catch (err) { if (err instanceof CimdClientError) { - return errorResponse(401, err.oauthError, err.message); + return cimdErrorResponse(err); } logger?.error?.('MCP token: client lookup failed:', err instanceof Error ? err.message : String(err)); return errorResponse(500, 'server_error', 'Client lookup failed'); diff --git a/test/lib/mcp/cimd.test.js b/test/lib/mcp/cimd.test.js index 71c6dc7..78e8184 100644 --- a/test/lib/mcp/cimd.test.js +++ b/test/lib/mcp/cimd.test.js @@ -894,7 +894,11 @@ describe('resolveCimdClient — per-URL fetch rate limit (#163)', () => { await assert.rejects( () => resolveCimdClient(VALID_URL, undefined), (err) => { - assert.equal(err.oauthError, 'temporarily_unavailable'); + // Throttle surfaces as slow_down + 429 + Retry-After, mirroring the + // issuance limiter — not the old auth-flavoured temporarily_unavailable. + assert.equal(err.oauthError, 'slow_down'); + assert.equal(err.statusCode, 429); + assert.ok(err.retryAfterSeconds >= 1, 'carries a Retry-After hint'); assert.match(err.message, /rate limit reached/); return true; } From f0564754ad6643cf5ecb195bfbf97dfda5c39c9f Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Sat, 11 Jul 2026 13:22:43 -0700 Subject: [PATCH 7/8] fix(mcp): cap Retry-After; cover the authorize-endpoint 429 branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more gemini nits on #171: - Clamp retryAfterSeconds at 2,147,483 (int32-second max) so a tiny configured rate can't advertise a multi-year backoff. - Add an authorize.test.js case that trips the CIMD fetch limiter and asserts handleAuthorize returns 429 slow_down + Retry-After — the 429 status-selection and header branch added in b221570 were only exercised at the token endpoint. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01SerGP6Am3xz2CKyPgbRc73 --- src/lib/mcp/rateLimit.ts | 8 +++++++- test/lib/mcp/authorize.test.js | 18 ++++++++++++++++++ test/lib/mcp/rateLimit.test.js | 10 ++++++++++ 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/lib/mcp/rateLimit.ts b/src/lib/mcp/rateLimit.ts index 4261930..e60af87 100644 --- a/src/lib/mcp/rateLimit.ts +++ b/src/lib/mcp/rateLimit.ts @@ -33,6 +33,11 @@ export type RateLimiter = { const DEFAULT_MAX_KEYS = 10_000; +// Cap the advertised Retry-After. A tiny configured rate (e.g. 0.001/min) +// would otherwise yield an absurd multi-year value; ~2.1e9 s (≈24.8 days) is +// the largest that stays within an int32 of seconds for any client parsing it. +const MAX_RETRY_AFTER_SECONDS = 2_147_483; + export function createRateLimiter(options: { /** Bucket capacity (maximum burst). */ capacity: number; @@ -80,7 +85,8 @@ export function createRateLimiter(options: { result = { allowed: true }; } else { const deficit = 1 - bucket.tokens; - result = { allowed: false, retryAfterSeconds: Math.ceil((deficit * 60) / refillPerMinute) }; + const retryAfterSeconds = Math.min(MAX_RETRY_AFTER_SECONDS, Math.ceil((deficit * 60) / refillPerMinute)); + result = { allowed: false, retryAfterSeconds }; } buckets.set(mapKey, bucket); return result; diff --git a/test/lib/mcp/authorize.test.js b/test/lib/mcp/authorize.test.js index c577b23..797f50c 100644 --- a/test/lib/mcp/authorize.test.js +++ b/test/lib/mcp/authorize.test.js @@ -616,6 +616,24 @@ describe('handleAuthorize — CIMD interstitial', () => { resource: 'https://app.example.com/mcp', }; + it('surfaces 429 + Retry-After when the CIMD fetch rate limit trips', async () => { + // A doc whose client_id doesn't match the URL fails validation and is not + // cached, so each attempt re-fetches and spends a fetch-limiter token + // (fixed 10/min per URL). The 11th is throttled and must reach the client + // as a 429 slow_down with a Retry-After header, not a 400/401. + _setFetch(makeCimdFetch(makeCimdDoc('https://mcp-client.example.com/mismatch.json'))); + const { entries } = makeProviderRegistry('github'); + const target = makeTarget(CIMD_QUERY); + for (let i = 0; i < 10; i++) { + const r = await handleAuthorize(makeRequest(), target, { enabled: true }, entries); + assert.equal(r.status, 400, `attempt ${i + 1} fails validation (token consumed)`); + } + const limited = await handleAuthorize(makeRequest(), target, { enabled: true }, entries); + assert.equal(limited.status, 429); + assert.equal(limited.body.error, 'slow_down'); + assert.ok(limited.headers?.['Retry-After'], 'carries a Retry-After header'); + }); + it('returns 200 HTML interstitial page for a CIMD client (not 302)', async () => { _setFetch(makeCimdFetch(makeCimdDoc(CIMD_CLIENT_ID))); const { entries } = makeProviderRegistry('github'); diff --git a/test/lib/mcp/rateLimit.test.js b/test/lib/mcp/rateLimit.test.js index 32437a3..985783a 100644 --- a/test/lib/mcp/rateLimit.test.js +++ b/test/lib/mcp/rateLimit.test.js @@ -97,6 +97,16 @@ describe('createRateLimiter', () => { assert.equal(limiter.tryTake(long(0)).allowed, true, 'long-evicted key returns fresh'); }); + it('caps retryAfterSeconds at a sane upper bound for tiny rates', () => { + const clock = makeClock(); + // 1e-5/min would compute a ~6,000,000 s wait; capped to the int32-second max. + const limiter = createRateLimiter({ capacity: 1, refillPerMinute: 0.00001, now: clock.now }); + limiter.tryTake('k'); + const blocked = limiter.tryTake('k'); + assert.equal(blocked.allowed, false); + assert.equal(blocked.retryAfterSeconds, 2_147_483); + }); + it('_reset drops all bucket state', () => { const limiter = createRateLimiter({ capacity: 1, refillPerMinute: 1 }); limiter.tryTake('k'); From 1fa1e28955430f6533d0740b206f56e3b17d3093 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Sat, 11 Jul 2026 13:42:02 -0700 Subject: [PATCH 8/8] fix(mcp): correct Retry-After comment; guard refill rate in the limiter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gemini nits on #171: - The MAX_RETRY_AFTER_SECONDS comment said ~2.1e9 s; the value is 2,147,483 s (~2.1e6, ≈24.8 days — int32-max ms as whole seconds). - Guard the utility against a non-finite/non-positive refillPerMinute (would make the bucket never refill and divide the retry-after math by zero/negative): fall back to the ≥1 capacity so the limiter stays well-defined even if a future caller misconfigures it. Test added. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01SerGP6Am3xz2CKyPgbRc73 --- src/lib/mcp/rateLimit.ts | 15 ++++++++++----- test/lib/mcp/rateLimit.test.js | 13 +++++++++++++ 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/lib/mcp/rateLimit.ts b/src/lib/mcp/rateLimit.ts index e60af87..b6a357d 100644 --- a/src/lib/mcp/rateLimit.ts +++ b/src/lib/mcp/rateLimit.ts @@ -34,8 +34,9 @@ export type RateLimiter = { const DEFAULT_MAX_KEYS = 10_000; // Cap the advertised Retry-After. A tiny configured rate (e.g. 0.001/min) -// would otherwise yield an absurd multi-year value; ~2.1e9 s (≈24.8 days) is -// the largest that stays within an int32 of seconds for any client parsing it. +// would otherwise yield an absurd multi-year value; 2,147,483 s (≈24.8 days, +// int32-max milliseconds expressed as whole seconds) is a sane ceiling for any +// client parsing the header. const MAX_RETRY_AFTER_SECONDS = 2_147_483; export function createRateLimiter(options: { @@ -49,10 +50,14 @@ export function createRateLimiter(options: { now?: () => number; }): RateLimiter { // A token bucket takes 1 token per request, so a burst ceiling below 1 could - // never admit anyone — clamp it up. The refill rate is left untouched, so a - // sub-1/min limit still means "one request, then one per 60/rate seconds". + // never admit anyone — clamp it up. The refill rate is left untouched (a + // sub-1/min limit still means "one request, then one per 60/rate seconds"), + // except that a non-finite or non-positive rate — which would make the + // bucket never refill and the retry-after math divide by zero/negative — + // falls back to the (already ≥1) capacity so the limiter stays well-defined. const capacity = Math.max(1, options.capacity); - const { refillPerMinute } = options; + const refillPerMinute = + Number.isFinite(options.refillPerMinute) && options.refillPerMinute > 0 ? options.refillPerMinute : capacity; const maxKeys = options.maxKeys ?? DEFAULT_MAX_KEYS; const now = options.now ?? Date.now; const buckets = new Map(); diff --git a/test/lib/mcp/rateLimit.test.js b/test/lib/mcp/rateLimit.test.js index 985783a..5d22a0f 100644 --- a/test/lib/mcp/rateLimit.test.js +++ b/test/lib/mcp/rateLimit.test.js @@ -107,6 +107,19 @@ describe('createRateLimiter', () => { assert.equal(blocked.retryAfterSeconds, 2_147_483); }); + it('stays well-defined when constructed with a non-positive refill rate', () => { + const clock = makeClock(); + // A 0 rate would divide-by-zero the retry-after math; the guard falls back + // to capacity so the limiter still admits the burst and yields a finite wait. + const limiter = createRateLimiter({ capacity: 3, refillPerMinute: 0, now: clock.now }); + assert.equal(limiter.tryTake('k').allowed, true); + assert.equal(limiter.tryTake('k').allowed, true); + assert.equal(limiter.tryTake('k').allowed, true); + const blocked = limiter.tryTake('k'); + assert.equal(blocked.allowed, false); + assert.ok(Number.isFinite(blocked.retryAfterSeconds) && blocked.retryAfterSeconds >= 1); + }); + it('_reset drops all bucket state', () => { const limiter = createRateLimiter({ capacity: 1, refillPerMinute: 1 }); limiter.tryTake('k');