Repository navigation
fix(oauth): rate-limit consent form submissions - #749
aliasunder wants to merge 8 commits into
Conversation
POST /oauth/decide checked the submitted token with no rate limit, so a client could submit token guesses against its own consent request as fast as it liked. It now allows 5 submissions a minute per client IP (an IPv6 client by its /56), the budget the other OAuth endpoints and /setup use, with its own counter and the same rate-limit headers as the SDK's endpoints. The limiter runs before the form parser, so an over-limit submission is refused unparsed, and the route's no-store header covers the 429. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
umm-actually re-reviewed at No new findings (3 tracked finding(s) across all runs). Context notes
umm-actually · z-ai/glm-5.3-flash |
Ship-Check: code-quality · gpt-6.1-sol
Cover wrong-token counting, minute-boundary recovery, endpoint counter isolation, IPv6 grouping and untrusted forwarding headers. Exercise consent setup over HTTP and tighten test assertions and case labels. Ship-Check: test-audit · gpt-6.1-sol
Treat an absent parsed body as an empty form so bodyless requests and unsupported content types receive the existing 400 response. Cover both paths over HTTP, including no-store and submission counting. Ship-Check: bug-check · gpt-6.1-sol
![umm-actually[bot]](https://avatars.githubusercontent.com/in/4233716?s=60&v=4){kind=small}
|
Answer oversized consent forms with 413, not the middleware's 500
Pre-existing: createErrorMiddleware answers every thrown error — including body-parser's 413 for an oversized /oauth/decide form — with a generic 500 and an error-level log. The PR's new limiter shields this path only once the per-IP budget is spent, so a first oversized submission still misclassifies a client-caused condition as a server fault. Failure scenario: POST /oauth/decide with a 101 KB form body (first request in the minute, so the limiter passes it) makes express-urlencoded throw its 413; the error middleware logs 'unhandled_error' at error level and answers 500 {error: internal server error}, polluting error alerts with a client-caused oversize. Suggested fixForward the error's status when present: const status = "status" in err && typeof err.status === "number" ? err.status : 500, then res.status(status).json({ error: "internal server error" }) — body-parser's 413 reaches the client as 413 without leaking any detail.umm-actually · z-ai/glm-5.3-flash |
Add a test that five successful approvals exhaust the consent submission budget. Remove the claim that consent rate limiting precedes all body parsing. Correct the comment explaining why the SDK rate-limit config uses max. Implemented-By: codex · gpt-6.1-sol
|
Reply to the umm-actually finding "Answer oversized consent forms with 413, not the middleware's 500" (comment 6714973173): already handled in #750, which is stacked on this PR. Its error middleware answers a body-parser error with its own status (413 for an oversized form) and a short warn line, and logs a stack only for real server errors. 🔍 ship-check · pr-monitor · claude-opus-5-5 |
The consent limiter set `standardHeaders: "draft-6"`, which equals what the SDK's `true` resolves to in express-rate-limit 8 but would drift if a later version changed that meaning. It now passes `true`, the same value the SDK passes to its OAuth limiters. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t-submit-rate-limit
The /setup limiter left express-rate-limit's header defaults in place, so its responses carried legacy `X-RateLimit-*` headers and no standard `RateLimit-*` ones, unlike every OAuth endpoint. It now sets `standardHeaders: true` and `legacyHeaders: false`, as the SDK's OAuth limiters and the consent limiter do, with a test that pins the 429's headers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
POST /oauth/decidechecked the submittedMCP_AUTH_TOKENwith no rate limit, so a client could open its own consent request through/authorizeand submit token guesses against it as fast as it liked for the request's 10-minute life. The consent form now allows 5 submissions a minute per client IP, the same budget/setupand the other OAuth endpoints use.Changes
src/vault-mcp/oauth/oauth-routes.ts: a limiter onPOST /oauth/decide, built once from the shared OAuth limiter options (5 a minute, keyed by client IP, an IPv6 client by its /56, logged asoauth_rate_limited). It keeps its own counter and passes the SDK's own header setting (standardHeaders: true,legacyHeaders: false), so it sends the sameRateLimit-*headers as the other OAuth endpoints and no legacyX-RateLimit-*. It runs after the route'sCache-Control: no-storemiddleware and before the form parser, so the 429 is never cached and an over-limit submission is refused before its form is parsed. Every submission counts, approvals included; a sixth in one minute gets the library's default 429 page, and the consent request stays valid for a retry after the minute.src/vault-mcp/oauth/__tests__/oauth-routes.test.ts: the sixth submission from one client IP gets 429 withno-store,Retry-After, theRateLimitheader and the default body; the 429 logsoauth_rate_limitedwith path/oauth/decide; another client IP is unaffected; an oversized form from a limited IP gets 429, not the parser's 413.src/vault-mcp/setup/setup-routes.ts: the/setuplimiter takes the same header setting, so it sendsRateLimit-*headers instead of the library's default legacyX-RateLimit-*ones; a test pins its 429 headers.SECURITY.md,ARCHITECTURE.md: the consent section and the rate-limiting paragraph name the new limit.Verification
npx vitest run src/vault-mcp/oauth src/__tests__/integration/server-oauth-integration.test.ts: 179 passed.express.urlencodedfails the oversized-form test.npm run build,npm run lint(no errors),npx knip, Prettier and markdownlint pass.🤖 Generated with Claude Code
Live Test Results
Head: baba1ed
One throwaway client opened one consent request; after a quiet minute, six wrong-token submissions for that request went out in one batch.
Cache-Control: no-storeno-store,RateLimit-Limit: 5,RateLimit-Remainingcounting downToo many requests, please try again later.,no-store,Retry-After: 60,RateLimit-Remaining: 0X-RateLimit-*headersoauth_rate_limitedwarning with path/oauth/decideand the probe's client IP, during the batchOn the instance: test-deploy run 42432916009 built and deployed the head; the image's revision label is
baba1ed1787a6caf6e314d48c760ad25342a346band the running container's image issha256:291939da7ae22df2f00bdee1ad44499fbf9c4006daf5e5b51ebb6d10e99e8d12, healthy since 01:12:23 UTC. The image, container and PR head were unchanged after the probes, and no other deploy or release ran meanwhile.🤖 Generated with Claude Code