fix(http)!: key on the hop the proxy wrote, not the one the client sent

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 `<what the client sent>, <what the
+  proxy saw>` 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

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",

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 `<whatever the client sent>, <what the proxy actually saw>` — 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";
 }

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 "<client-supplied>, <what Caddy saw>"
+  // 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');
 });