From bc69dec8e63441ced07eab1f543aace8758a64f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sat, 26 Sep 2026 20:10:12 +0200 Subject: [PATCH] fix(http): trust X-Real-IP only behind a declared proxy The client-IP resolver read X-Real-IP on every path: with TRUST_PROXY_HOPS=0, which means no proxy in front and no forwarding header to believe, and on the path where the X-Forwarded-For chain was shorter than the declared hops, which means the proxies were bypassed. A caller sending a new X-Real-IP with each request got a new per-IP rate-limit bucket each time. X-Real-IP now gets the trust X-Forwarded-For gets: it is read only when at least one proxy hop is declared and the request carries no X-Forwarded-For. A short chain resolves to no address. The reverse-proxy guide no longer suggests TRUST_PROXY_HOPS=0 for a proxy that sets X-Real-IP; such a proxy takes 1. --- docs/self-hosting/reverse-proxy.md | 22 +++++++++----- src/lib/__tests__/get-client-ip.test.ts | 19 +++++++++--- .../__tests__/rate-limit-auth-surface.test.ts | 5 ++-- src/lib/api-response.ts | 30 +++++++++++-------- 4 files changed, 50 insertions(+), 26 deletions(-) diff --git a/docs/self-hosting/reverse-proxy.md b/docs/self-hosting/reverse-proxy.md index 7f18095ec..6ef02203b 100644 --- a/docs/self-hosting/reverse-proxy.md +++ b/docs/self-hosting/reverse-proxy.md @@ -40,17 +40,23 @@ chain from the **right**, counting back `TRUST_PROXY_HOPS` entries. This guards against a client rotating `X-Forwarded-For` per request to defeat IP-based rate limits. -| Topology | `TRUST_PROXY_HOPS` | -| --------------------------------------------------------- | -------------------------- | -| App is internet-facing with no proxy | `0` (XFF ignored entirely) | -| Single proxy in front (Caddy / Traefik / Nginx / Coolify) | `1` (default) | -| Cloudflare → your proxy → app | `2` | -| Cloudflare → Coolify-Tunnel → Coolify → app | `3` | +| Topology | `TRUST_PROXY_HOPS` | +| --------------------------------------------------------- | ------------------------------------- | +| App is internet-facing with no proxy | `0` (no forwarding header is trusted) | +| Single proxy in front (Caddy / Traefik / Nginx / Coolify) | `1` (default) | +| Cloudflare → your proxy → app | `2` | +| Cloudflare → Coolify-Tunnel → Coolify → app | `3` | A misconfigured count logs a one-shot warning to stderr and collapses every anonymous caller into one shared rate-limit bucket. Match the -value to the actual hop count or set it to `0` and let `x-real-ip` -drive the IP resolution. +value to the actual hop count. + +`X-Real-IP` gets the same trust as `X-Forwarded-For`. It is read only +when `TRUST_PROXY_HOPS` is `1` or more and the request carries no +`X-Forwarded-For`, which covers a proxy that sets only `X-Real-IP`. With +`0` neither header is believed, because without a proxy in front both +come from the caller. If your proxy sets only `X-Real-IP` and you had +set `0` for it, set `1` instead. ## Caddy diff --git a/src/lib/__tests__/get-client-ip.test.ts b/src/lib/__tests__/get-client-ip.test.ts index 9e5a92d0e..56b082589 100644 --- a/src/lib/__tests__/get-client-ip.test.ts +++ b/src/lib/__tests__/get-client-ip.test.ts @@ -72,7 +72,7 @@ describe("getClientIp trusted-proxy semantics (V3 audit)", () => { }, ); - it("with TRUST_PROXY_HOPS=0 ignores XFF entirely", () => { + it("with TRUST_PROXY_HOPS=0 trusts no forwarding header, x-real-ip included", () => { process.env.TRUST_PROXY_HOPS = "0"; const ip = getClientIp( makeRequest({ @@ -80,7 +80,17 @@ describe("getClientIp trusted-proxy semantics (V3 audit)", () => { "x-real-ip": "8.8.8.8", }), ); - expect(ip).toBe("8.8.8.8"); + expect(ip).toBeNull(); + }); + + it("x-real-ip rotation — with no declared proxy a caller cannot mint a fresh bucket per request", () => { + process.env.TRUST_PROXY_HOPS = "0"; + const seen = new Set( + ["1.1.1.1", "2.2.2.2", "3.3.3.3"].map((ip) => + getClientIp(makeRequest({ "x-real-ip": ip })), + ), + ); + expect(seen).toEqual(new Set([null])); }); it("with TRUST_PROXY_HOPS=0 and no x-real-ip returns null", () => { @@ -277,7 +287,8 @@ describe("getClientIpOrTrustWarning shape (F-6, 2026-05-16)", () => { expect(result.ip).toBeNull(); }); - it("returns trustViolation=true and x-real-ip when chain is short but x-real-ip is set", () => { + it("returns trustViolation=true and ignores x-real-ip when the chain is short", () => { + // The declared proxies were bypassed, so x-real-ip came from the caller. process.env.TRUST_PROXY_HOPS = "2"; const result = getClientIpOrTrustWarning( makeRequest({ @@ -285,7 +296,7 @@ describe("getClientIpOrTrustWarning shape (F-6, 2026-05-16)", () => { "x-real-ip": "9.9.9.9", }), ); - expect(result).toEqual({ ip: "9.9.9.9", trustViolation: true }); + expect(result).toEqual({ ip: null, trustViolation: true }); }); it("returns trustViolation=false when XFF is absent (no chain to validate)", () => { diff --git a/src/lib/__tests__/rate-limit-auth-surface.test.ts b/src/lib/__tests__/rate-limit-auth-surface.test.ts index 8331e5479..f6b7369c3 100644 --- a/src/lib/__tests__/rate-limit-auth-surface.test.ts +++ b/src/lib/__tests__/rate-limit-auth-surface.test.ts @@ -176,8 +176,9 @@ describe("checkAuthSurfaceRateLimit — trust-violation tightening", () => { 5, 15 * 60 * 1000, ); - // Falls through to x-real-ip when the chain is too short. - expect(result.ip).toBe("9.9.9.9"); + // A short chain means the declared proxies were bypassed, so the + // caller-set x-real-ip is not believed either. + expect(result.ip).toBeNull(); expect(lastKey()).toBe("auth:anon:trust-violation"); } finally { warn.mockRestore(); diff --git a/src/lib/api-response.ts b/src/lib/api-response.ts index b54276608..adcbf113c 100644 --- a/src/lib/api-response.ts +++ b/src/lib/api-response.ts @@ -390,9 +390,9 @@ export async function safeJson( * IP-based rate-limits. * * Trust model (`TRUST_PROXY_HOPS` env): - * - "0" → ignore XFF entirely, fall back to x-real-ip / null + * - "0" → trust no forwarding header at all, XFF or x-real-ip * (use this if HealthLog is internet-facing without a - * reverse proxy you control) + * reverse proxy you control); the IP resolves to null * - "1" (default)→ trust exactly one hop (typical Coolify / Caddy / * Cloudflare-Tunnel single-proxy deployment); read the * rightmost XFF entry which is the IP your proxy @@ -540,18 +540,24 @@ export function getClientIpOrTrustWarning(request: Request): { // configured trust. Without this warning the silent degrade was // invisible until rate-limits visibly misfired in production. warnTrustViolationOnce(hops, chain.length); - const realIp = request.headers.get("x-real-ip"); - return { - ip: realIp && looksLikeIp(realIp) ? realIp : null, - trustViolation: true, - }; + // Not x-real-ip either. A chain shorter than the configured hops means + // the request did not come through the proxies the operator declared, + // so whatever x-real-ip it carries was set by the caller. + return { ip: null, trustViolation: true }; } + // x-real-ip is a forwarding header like XFF and gets the same trust: only + // when the operator declares a proxy in front (hops > 0), and only as the + // single-proxy stand-in for a missing XFF. It used to be read on every + // path, including `TRUST_PROXY_HOPS=0` ("no proxy, trust no header") and + // the broken-chain path above, so a caller sending a fresh x-real-ip per + // request got a fresh per-IP rate-limit bucket each time. + const realIp = request.headers.get("x-real-ip")?.trim(); + return { + ip: realIp && looksLikeIp(realIp) ? realIp : null, + trustViolation: false, + }; } - const realIp = request.headers.get("x-real-ip"); - return { - ip: realIp && looksLikeIp(realIp) ? realIp : null, - trustViolation: false, - }; + return { ip: null, trustViolation: false }; } export function getClientIp(request: Request): string | null {