Stop keying the rate limiter on spoofable forwarding headers - #6
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 3 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe rate limiter now enforces global and per-key request limits, manages bucket capacity, and cleans expired buckets. Client-key extraction validates IP addresses and uses forwarding headers only when trusted proxy configuration is enabled. ChangesRate-limit security controls
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🟡 Moderate · up to The change prevents header rotation from bypassing rate limits and caps in-memory growth, but one unauthenticated client can still consume the shared process capacity needed by others, and trusted proxy behavior is not explicitly defined. Merge should wait for explicit owner acceptance of these bounded availability and deployment-contract risks or for a follow-up fix. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Summary
Fixes the security finding that the token-bucket limiter (the only abuse control on the unauthenticated cert-viewer / cors-tester / misconfig-mapper endpoints) was keyed on client-supplied headers. An attacker could rotate
X-Vercel-Forwarded-Forper request to get a fresh bucket every time — unlimited outbound SSRF-scanner use plus unbounded growth of the in-memorybucketsmap.Changes in
lib/security/rate-limit.ts:clientKeyFromHeadersnow only honors forwarding headers when the deployment declares a trusted proxy (VERCEL=1, where the edge overwrites these headers, or explicitTRUST_PROXY_HEADERS=1), and only when the value parses as an IP (node:netisIP). Otherwise it returns a shared"direct"key, so an untrusted deployment can't be tricked into per-header buckets at all.rateLimitnow also enforces a global secondary cap independent of the client key:Tests updated/added: header trust gating, non-IP header rejection, per-key trip at 12, and global trip at 120 with rotating keys. The old test asserting unconditional header trust codified the vulnerable behavior and was replaced.
No endpoint code changes needed — all three routes already call
rateLimit(clientKeyFromHeaders(...)).Link to Devin session: https://app.devin.ai/sessions/895ec46b1c4244678bfb15206edf6990
Requested by: @tinkthemaker
Summary by CodeRabbit
New Features
Bug Fixes