diff --git a/.env.example b/.env.example index f6acdee9..b5b55bac 100644 --- a/.env.example +++ b/.env.example @@ -45,9 +45,12 @@ CSRF_TRUSTED_ORIGINS= # Email login OTP. AUTH_SMTP_* overrides the feedback SMTP settings below; # when omitted, email login reuses FEEDBACK_SMTP_*. EMAIL_OTP_SECRET= -# Only set this when the reverse proxy overwrites/appends this header and the -# app cannot be reached directly. Example for the production nginx setup: -# TRUSTED_CLIENT_IP_HEADER=x-forwarded-for +# Header carrying the real client IP for rate limiting. Leave empty behind a +# plain nginx reverse proxy: the rightmost X-Forwarded-For entry is used, which +# nginx appends via $proxy_add_x_forwarded_for. Set it only when a CDN publishes +# the client IP in a different header it overwrites (e.g. cf-connecting-ip behind +# Cloudflare). Never point it at a header the client can set — the limit keys off +# its rightmost value. TRUSTED_CLIENT_IP_HEADER= AUTH_SMTP_HOST= AUTH_SMTP_PORT=465 diff --git a/lib/security/rate-limit.ts b/lib/security/rate-limit.ts index 21e999ac..feee6f53 100644 --- a/lib/security/rate-limit.ts +++ b/lib/security/rate-limit.ts @@ -65,11 +65,26 @@ export function createRateLimitHeaders( }; } +/** + * Identify the caller for rate-limiting. + * + * Only a proxy-appended value may be trusted. We read the *rightmost* entry of + * the chosen header because nginx's `$proxy_add_x_forwarded_for` appends the + * real peer to the end — the leftmost entries are client-supplied and forgeable, + * so keying on them would let an attacker mint a fresh bucket per request. + * + * When `TRUSTED_CLIENT_IP_HEADER` is unset we fall back to `x-forwarded-for`, + * which the default nginx deployment always appends. Returning a constant here + * instead would collapse every visitor into ONE global bucket per namespace, + * letting a single actor exhaust it and lock the whole site out of OTP login, + * feedback, etc. Only when no forwarding header exists at all (e.g. a direct, + * un-proxied hit) do we degrade to a shared "unknown" key. + */ export function getRequestClientKey(request: Request): string { const configuredHeader = process.env.TRUSTED_CLIENT_IP_HEADER?.trim(); - if (!configuredHeader) return "ip:unknown"; + const headerName = configuredHeader || "x-forwarded-for"; - const raw = request.headers.get(configuredHeader); + const raw = request.headers.get(headerName); if (!raw) return "ip:unknown"; const entries = raw diff --git a/tests/unit/lib/rate-limit.test.ts b/tests/unit/lib/rate-limit.test.ts index 556f2564..52176240 100644 --- a/tests/unit/lib/rate-limit.test.ts +++ b/tests/unit/lib/rate-limit.test.ts @@ -54,7 +54,7 @@ describe("rate-limit utility", () => { expect(Number(headers["retry-after"])).toBeGreaterThan(0); }); - it("extracts client key from request headers", () => { + it("uses the rightmost entry of the configured proxy header", () => { vi.stubEnv("TRUSTED_CLIENT_IP_HEADER", "x-forwarded-for"); const request = new Request("https://example.com/api/test", { headers: { @@ -63,13 +63,38 @@ describe("rate-limit utility", () => { }, }); + // Rightmost is the proxy-appended peer; leftmost entries are client-forged. const key = getRequestClientKey(request); expect(key).toBe("ip:203.0.113.11"); }); - it("does not trust client IP headers until a proxy header is configured", () => { + it("falls back to the rightmost X-Forwarded-For entry when no header is configured", () => { + // Default deployment (TRUSTED_CLIENT_IP_HEADER unset) must still key per-IP, + // not collapse every visitor into one global bucket. const request = new Request("https://example.com/api/test", { - headers: { "x-forwarded-for": "203.0.113.10" }, + headers: { "x-forwarded-for": "198.51.100.7, 203.0.113.10" }, + }); + + expect(getRequestClientKey(request)).toBe("ip:203.0.113.10"); + }); + + it("does not let a client forge a fresh key by prepending X-Forwarded-For entries", () => { + const attacker = new Request("https://example.com/api/test", { + headers: { "x-forwarded-for": "10.0.0.1, 203.0.113.10" }, + }); + const attackerAgain = new Request("https://example.com/api/test", { + headers: { "x-forwarded-for": "10.0.0.2, 203.0.113.10" }, + }); + + // The forgeable leftmost entry changes, but the proxy-appended rightmost + // one is identical, so both land in the same bucket. + expect(getRequestClientKey(attacker)).toBe("ip:203.0.113.10"); + expect(getRequestClientKey(attackerAgain)).toBe("ip:203.0.113.10"); + }); + + it("degrades to a shared key only when no forwarding header exists at all", () => { + const request = new Request("https://example.com/api/test", { + headers: { "user-agent": "StyleKit-Test/1.0" }, }); expect(getRequestClientKey(request)).toBe("ip:unknown");