mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-08 04:32:42 +00:00
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.
This commit is contained in:
1 parent
92c5b0a60d
commit
bc69dec8e6
4 files changed
+50
-26
No files matched your search
@@ -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
|
||||
|
||||
|
||||
@@ -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)", () => {
|
||||
|
||||
@@ -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();
|
||||
|
||||
+18
-12
@@ -390,9 +390,9 @@ export async function safeJson<T = unknown>(
|
||||
* 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 {
|
||||
|
||||
Reference in new issue
Block a user