mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-24 19:32:10 +00:00
xarmian
3319ad5ea1
* feat(mcp): per-token rate limit on /mcp (TASK-959)
Add a per-token rate limit to /mcp's auth middleware. Closes the
"runaway agent burns through user quota" gap that PLAN-943 left as
a follow-up to TASK-950.
## Policy
- 60 requests / minute / token, burst 20.
- Per-token (not per-IP): office-NAT-shared users don't share a
bucket, and a runaway agent on one token can't burn another
token's quota for the same user.
- Limiter key: SHA-256(bearer) — the raw token never lives in the
limiter map even though buckets persist for the 5-minute
retention window.
- Discovery docs (`/.well-known/oauth-*`) are NOT rate-limited.
They're polled by MCP clients before any token exists; rate-
limiting them per-IP would penalize office NATs and per-bearer
doesn't apply (no bearer to hash).
- No-bearer requests are 401'd before the limiter sees them, so a
bare-bones DoS via empty Authorization headers gets the cheap
rejection path without sharing a (necessarily-empty) bucket key.
## 429 response
Per RFC 6585: `Retry-After: <seconds>` header (computed from the
limiter's refill rate), plus `X-RateLimit-Limit`,
`X-RateLimit-Remaining: 0`. Body is the MCP-shaped JSON envelope
`{"error": {"code": "rate_limited", "message": "..."}}` so MCP
clients (Claude Desktop, Cursor) can surface the error consistently.
## Implementation
- `RateLimiters.MCPPerToken` — new `*ipRateLimiter` instance,
drained in `Stop()` so cleanup goroutines don't leak (BUG-851
pattern).
- `Server.checkMCPRateLimit` — called from `MCPBearerAuth` BEFORE
auth validation. Returns false + writes 429 when bucket is
exhausted; auth still 401s if the token is also invalid (the
rate limit and validity checks are independent).
- `hashTokenForLimiter` — SHA-256 hex digest helper. Uniform with
the limiter's other (IP-string) keys.
- `writeMCPRateLimit` — emits the 429 envelope.
## Tests
- TestMCPRateLimit_PerToken_BucketEnforced — single token → 429
within 30 attempts (60/min, burst 20).
- TestMCPRateLimit_PerToken_TwoTokensIndependent — drain token1
to 429, verify token2 still passes a full burst.
- TestMCPRateLimit_DiscoveryDocsExempt — 50 hits to
/.well-known/oauth-protected-resource, zero 429s.
- TestMCPRateLimit_NoBearer_NotCounted — no-bearer requests 401
before the limiter, no 429s.
- TestMCPRateLimit_429EnvelopeShape — Retry-After,
X-RateLimit-* headers, MCP error envelope shape.
- TestHashTokenForLimiter — hash determinism, length, no collision
by prefix, empty input safety.
* fix(mcp): move per-token rate limit AFTER auth validation (Codex round 1)
Codex review #378 round 1 caught a memory-DoS risk: the pre-auth
limiter created a new bucket entry for every distinct bearer
string. An attacker rotating random bearer values would grow the
limiter map unbounded until the 5-minute cleanup tick — millions
of phantom entries before the goroutine catches up.
Fix: relocate the checkMCPRateLimit call to AFTER auth validation
in both PAT and OAuth paths. The limiter map now only fills with
hashes of *valid* tokens, bounding map size by the active-token
count rather than by the bearer-string space.
Trade-off: invalid-bearer spam still hits the auth path's DB
lookup (CPU cost, but a single indexed read per request) without
any rate limiting. The CPU exposure is small enough to accept for
v1; a follow-up could add a pre-auth per-IP cap for invalid-token
flooding if real abuse appears.
Tests:
- TestMCPRateLimit_InvalidBearerNotRateLimited — 50 invalid
bearers in a row, none get 429 (always 401).
- TestMCPRateLimit_LimiterMapBoundedByValidTokensOnly — direct
regression: 100 distinct invalid bearers, limiter map size
must NOT grow.
- Existing happy-path tests updated to use real PATs (via the new
mustCreatePATForTest helper) so the post-auth-validation guard
doesn't short-circuit them.
* fix(mcp): move OAuth rate limit AFTER all validation gates (Codex round 2)
Codex review #378 round 2 caught a P3 gap in round 1's fix. The
OAuth path's rate-limit check ran AFTER IntrospectToken but BEFORE:
- access-token-vs-refresh-token check
- RFC 8707 audience match
- session.GetSubject() presence
- GetUser lookup
So an active-but-not-authorized OAuth bearer (refresh token used as
a bearer, wrong-audience token, deleted user) would create a
limiter entry. After 30 such requests the response would flip from
the intended 401 invalid_token to 429 — leaking limiter state to
attackers and slightly defeating the bounded-map property.
Fix: move the OAuth-path checkMCPRateLimit call to the very end of
handleMCPOAuthAuth, just before context attachment + next.ServeHTTP.
Now the limiter map only contains tokens that would have reached
the dispatcher otherwise.
Test: TestMCPRateLimit_OAuthRefreshTokenNotCounted — mints a real
refresh token via the full OAuth flow, hammers /mcp with it 50
times, asserts every response is 401 AND the limiter map size is
unchanged.
* fix(mcp): move PAT rate limit AFTER all validation gates (Codex round 3)
Codex review #378 round 3 caught the symmetric issue in the PAT
path that round 2 fixed for OAuth. checkMCPRateLimit ran AFTER
ValidateToken but BEFORE:
- apiToken.UserID == "" check (legacy workspace-scoped tokens)
- GetUser lookup (deleted-user case)
Active-but-not-authorized PAT bearers (legacy tokens with no
user_id, tokens whose user was deleted) would have created limiter
entries and eventually 429'd instead of returning the intended
401 invalid_token.
Fix: move the PAT-path checkMCPRateLimit call to the very end of
handleMCPPATAuth, just before context attachment + next.ServeHTTP.
Now mirrors the OAuth path's positioning — both run the rate limit
exactly once, at the END of their happy path, so the limiter map
only contains tokens that would otherwise reach the dispatcher.