fix: prevent X-Forwarded-For rate-limit bypass - #1456
Open
ionfwsrijan wants to merge 1 commit into
Open
Conversation
…figured proxy CIDR Replace the blanket 'trust proxy: 1' with a REVERSE_PROXY_CIDR-based trust setting so a client-supplied X-Forwarded-For header can no longer rotate the rate-limit bucket. All limiters now use an explicit IP key generator that normalizes IPv6 via the express-rate-limit helper, keeping brute-force and AI cost guards effective. Closes Canopus-Labs#1438
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe server now trusts forwarded client addresses only from configured proxy CIDRs. All rate limiters use a shared stable IP key generator, with IPv6 masking. A login integration test verifies that rotating ChangesProxy trust and rate limiting
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
app.set("trust proxy", 1)combined with the defaultkeyGenerator(based onreq.ip) lets any client send a spoofedX-Forwarded-Forheader. Because the server blindly trusts the first entry of that header, an attacker can rotate their perceived IP on every request and defeat every rate limiter (login brute-force protection, AI cost guards, general endpoint limits).Fix
trust proxy: 1inserver.jswith aREVERSE_PROXY_CIDR-based trust setting.X-Forwarded-Foris now honored only when the request comes from a configured trusted proxy CIDR; when unset,req.ipis the direct socket address and the header is ignored entirely.rateLimiter.jsnow use an explicit IPkeyGeneratorthat normalizes IPv6 via the express-rate-limit helper, so IPv6 callers cannot dodge limits by rotating addresses.REVERSE_PROXY_CIDRvariable in.env.example.Files changed
backend/server.js— conditionaltrust proxyfromREVERSE_PROXY_CIDR.backend/middlewares/rateLimiter.js— explicitipKeyGeneratoron all five limiters.backend/.env.example— documentedREVERSE_PROXY_CIDR.backend/tests/rateLimiter.xff.bypass.unit.test.js— new test proving a rotatedX-Forwarded-Forno longer resets the limit bucket.Testing
npx vitest run tests/rateLimiter.xff.bypass.unit.test.js— 1/1 passing (11th login attempt rejected with 429 despite spoofing a fresh IP each request).Closes #1438
Summary
trust proxy: 1with configurableREVERSE_PROXY_CIDRtrust settings.X-Forwarded-Forvalues.Ready to merge.