diff --git a/README.md b/README.md index 73cae53..1a1497b 100644 --- a/README.md +++ b/README.md @@ -71,12 +71,14 @@ because the store holds and the algorithm decides. `Retry-After` on refusals only. A plain object you spread into your own response; constructing a `Response` would mean choosing your framework for you. -- **`clientIp(headers)`** — first `x-forwarded-for` hop, then `x-real-ip`, then - `"unknown"`. Only as honest as the proxy in front of you; the `"unknown"` - fallback throttles anonymous traffic collectively rather than throwing. - -## Deliberately not included - +- **`clientIp(headers, { trustedProxies = 1 })`** — the forwarded hop your own + proxy wrote, then `x-real-ip`, then `"unknown"`. A proxy **appends** to + `X-Forwarded-For`, so the header reads `, ` and only the **last** entry is unforgeable. Reading the first + one — which this did before v0.2.0 — lets a caller vary the header per + request, mint a fresh bucket each time and never trip the limit at all. + Set `trustedProxies: 2` when a CDN sits in front of your proxy, or `0` when + the server is exposed directly and no forwarded header can be believed. - **No HTTP client, no middleware, no framework types** — the package supplies the decision; your app keeps its conventions. - **No limit values** — how many attempts a login route allows is app diff --git a/package.json b/package.json index 9fcbda6..c0ef6e3 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "limitkit", - "version": "0.1.0", + "version": "0.2.0", "description": "Rate limiting as a decision, not a middleware: fixed and sliding windows over an injectable store, a bounded in-memory store, standard RateLimit headers, and client-IP extraction. No HTTP client, no framework.", "license": "MIT", "author": "Mao Nakamoto", diff --git a/src/http.ts b/src/http.ts index 55f08ac..980eb7b 100644 --- a/src/http.ts +++ b/src/http.ts @@ -38,17 +38,49 @@ export type HeadersLike = { get(name: string): string | null }; /** * Best-effort client identity for keying a limiter. * - * `x-forwarded-for` is a client-controlled header on a directly-exposed - * server, so this is only as honest as the proxy in front of it — behind - * Caddy/nginx (this fleet's shape) the first entry is what the proxy saw. + * WHICH HOP, AND WHY IT IS NOT THE FIRST ONE + * + * `X-Forwarded-For` is a LIST, and a reverse proxy APPENDS to it. Caddy and + * nginx both do. So for a request that arrived through one proxy the header + * reads `, ` — and the + * only entry the client could not forge is the LAST one. + * + * This function used to return the first, with a comment asserting that was + * "what the proxy saw", and a test pinning it. Both were wrong in the same + * direction. The effect was that every limiter keyed on this — across every + * adopter — could be bypassed completely by sending a random + * `X-Forwarded-For` with each request: a new header value is a new bucket, so + * no bucket ever fills. A limiter that cannot be tripped is not a limiter. + * Found in orangecat's payments audit (bitbaum/orangecat#563, finding 2). + * + * `trustedProxies` is how many proxies of your own sit in front. Default 1 — + * one reverse proxy is the overwhelmingly common shape and the only one where + * a default can be safe. Two proxies (a CDN in front of your own) means 2, and + * the answer moves one further left. + * + * `trustedProxies: 0` means the server is exposed directly, so EVERY forwarded + * header is written by the client and none can be believed: the result is + * "unknown" rather than a number that looks like evidence. + * * Falls back to "unknown" rather than throwing: a limiter keyed on "unknown" - * throttles the anonymous bucket collectively, which is the right failure - * mode for the abuse this exists to blunt. + * throttles the anonymous bucket collectively, which is the right failure mode + * for the abuse this exists to blunt. */ -export function clientIp(headers: HeadersLike): string { - return ( - headers.get("x-forwarded-for")?.split(",")[0]?.trim() || - headers.get("x-real-ip") || - "unknown" - ); +export function clientIp( + headers: HeadersLike, + opts: { trustedProxies?: number } = {}, +): string { + const trusted = opts.trustedProxies ?? 1; + if (trusted > 0) { + const hops = (headers.get("x-forwarded-for") ?? "") + .split(",") + .map((hop) => hop.trim()) + .filter(Boolean); + // Count from the RIGHT: the rightmost hop was written by the proxy nearest + // us. Clamped, because a shorter chain than configured means fewer proxies + // ran than expected — the leftmost is then the only candidate we have, and + // it is still the one our own proxy wrote. + if (hops.length > 0) return hops[Math.max(0, hops.length - trusted)]!; + } + return headers.get("x-real-ip")?.trim() || "unknown"; } diff --git a/test/store-http.test.js b/test/store-http.test.js index 07c6275..65ba383 100644 --- a/test/store-http.test.js +++ b/test/store-http.test.js @@ -44,12 +44,45 @@ test('toHeaders emits the standard trio, Retry-After only on refusal', () => { assert.equal(no['X-RateLimit-Reset'], String(Math.ceil((1_000_000 + 60_000) / 1000))); }); -test('clientIp: first forwarded hop wins; absence degrades to a shared bucket', () => { - const h = (map) => ({ get: (k) => map[k.toLowerCase()] ?? null }); - assert.equal(clientIp(h({ 'x-forwarded-for': '9.9.9.9, 10.0.0.1' })), '9.9.9.9'); +const h = (map) => ({ get: (k) => map[k.toLowerCase()] ?? null }); + +test('clientIp: the hop the PROXY wrote, not the one the client sent', () => { + // A proxy APPENDS. So this header reads ", " + // and only the last entry is unforgeable. The previous version returned + // 9.9.9.9 here — the attacker's own value — and a test pinned it. + assert.equal(clientIp(h({ 'x-forwarded-for': '9.9.9.9, 10.0.0.1' })), '10.0.0.1'); + + // The bypass this closes: vary the header per request and every request is a + // new bucket, so no bucket ever fills. Same real client, one key. + const keys = new Set( + ['1.1.1.1', '2.2.2.2', '3.3.3.3'].map((spoof) => + clientIp(h({ 'x-forwarded-for': `${spoof}, 203.0.113.7` })), + ), + ); + assert.deepEqual([...keys], ['203.0.113.7'], 'a spoofed prefix must not mint new buckets'); + + // A single hop is the normal case: nothing was forged, the proxy wrote it. + assert.equal(clientIp(h({ 'x-forwarded-for': '203.0.113.7' })), '203.0.113.7'); +}); + +test('clientIp: trustedProxies moves the hop, and 0 believes nothing', () => { + // Two of our own proxies (a CDN in front of Caddy) — the answer is one further left. + assert.equal( + clientIp(h({ 'x-forwarded-for': 'evil, 203.0.113.7, 10.0.0.1' }), { trustedProxies: 2 }), + '203.0.113.7', + ); + // Directly exposed: every forwarded header is written by the client, so none + // is evidence. "unknown" is honest; a number here would look like proof. + assert.equal(clientIp(h({ 'x-forwarded-for': 'evil' }), { trustedProxies: 0 }), 'unknown'); + // Fewer hops than configured — still the entry our own proxy wrote. + assert.equal(clientIp(h({ 'x-forwarded-for': '203.0.113.7' }), { trustedProxies: 3 }), '203.0.113.7'); +}); + +test('clientIp: absence degrades to a shared bucket, never a throw', () => { assert.equal(clientIp(h({ 'x-real-ip': '8.8.8.8' })), '8.8.8.8'); // "unknown" throttles anonymous traffic COLLECTIVELY — the right failure // mode for abuse-blunting, and it must never throw. assert.equal(clientIp(h({})), 'unknown'); assert.equal(clientIp(h({ 'x-forwarded-for': '' })), 'unknown', 'an empty header is not an identity'); + assert.equal(clientIp(h({ 'x-forwarded-for': ' , , ' })), 'unknown', 'separators alone are not an identity'); });