fix(security): fall back to x-real-ip when TRUST_PROXY_COUNT is unset - #308
Merged
Conversation
When TRUST_PROXY_COUNT is not configured, getClientIP() returned 'unknown' for every request, collapsing all clients into a single rate-limit bucket. One abuser could exhaust the shared quota and DoS the endpoint for all legitimate users. The comment claimed it fell back to 'socket peer' but Next.js route handlers do not expose a synchronous socket peer. New three-tier IP resolution: 1. TRUST_PROXY_COUNT set: Nth-from-right XFF entry (most spoof-resistant). 2. TRUST_PROXY_COUNT unset: fall back to x-real-ip. The reverse proxy (OpenResty/ELB) sets this by OVERWRITING client values, so it is far harder to spoof than the append-only X-Forwarded-For. 3. Neither present: 'unknown' + a one-time production warning log. This prevents bucket collapse when an operator forgets TRUST_PROXY_COUNT, as long as the reverse proxy sets x-real-ip (standard practice). Tests: add x-real-ip fallback test (different IPs get separate buckets); split the old 'unknown' test into with/without x-real-ip cases.
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.
Summary
Fixes audit V2 finding #3 (High): when
TRUST_PROXY_COUNTis not configured,getClientIP()returned'unknown'for every request, collapsing all clients into a single rate-limit bucket. One abuser could exhaust the shared quota and DoS the endpoint for all legitimate users. The comment claimed it fell back to "socket peer" but Next.js route handlers do not expose a synchronous socket peer.Changes
src/lib/middleware/rate-limit.ts— new three-tier IP resolution ingetClientIP():TRUST_PROXY_COUNTset: Nth-from-right XFF entry (most spoof-resistant) — unchanged.TRUST_PROXY_COUNTunset: fall back tox-real-ip. The reverse proxy (OpenResty/ELB) sets this by overwriting client-supplied values, so it is far harder to spoof than the append-onlyX-Forwarded-For. This prevents bucket collapse when an operator forgetsTRUST_PROXY_COUNT, as long as the reverse proxy setsx-real-ip(standard practice).'unknown'+ a one-time production warning log (so operators notice).tests/unit/rate-limit-proxy.test.ts:x-real-ipfallback gives different IPs separate buckets.x-real-ipcases.Test plan
pnpm type-check— cleanpnpm test— 471 passed