fix(link): address 4 codex [P2] findings on the rate limiter

All four were real bypass or fail-open paths on an endpoint about to be
publicly exposed:

- X-Forwarded-For was trusted from ANY peer. The origin also listens on the
  tailnet, so anyone reaching it directly could pick -- and rotate -- their
  own rate-limit key by sending a header. Now honored only when the socket
  peer is in a configured trusted_proxies list, and the last hop must parse
  as a real IP. trust_forwarded_for without trusted_proxies REFUSES startup.
- Capacity eviction was fail-open and exploitable: an attacker able to mint
  many distinct keys could evict their own live window and start fresh. Now
  reclaims only EXPIRED windows and refuses the new key when all are live.
  Fail closed -- /v1/link is invite-only, so hitting the cap is an attack.
- The clock was read outside the lock, so racing threads could append out of
  order; both retry_after (hits[0]) and reclamation (v[-1]) assume the list
  is chronological. Moved inside.
- Config types were unvalidated: `"enabled": null` or `0` silently disabled
  limiting, and the string "false" enabled XFF trust (non-empty strings are
  truthy). Booleans must now be real JSON booleans; rate_limit must be an
  object.

Codex confirmed no path-variant bypass (dispatch is exact-match) and no
keep-alive/pipelining bypass (rejects set close_connection).

Tests 87 -> 99: capacity fail-closed with the victim's window proven
untouched through a 40-key flood, 200-thread chronological-order check,
untrusted-peer spoof, junk XFF, and every config-type trap.

Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01LTARYHX7GPepi3CH3tp5pg

README.md

@@ -102,25 +102,37 @@ header, decided *before* the body is read so an abusive caller costs nothing.
 "rate_limit": {
   "enabled": true,
   "trust_forwarded_for": false,
+  "trusted_proxies": [],
   "rules": {"/v1/link": [5, 3600], "/v1/repos": [60, 3600]}
 }
 ```
 
-* A malformed rule **refuses startup** rather than silently meaning
+* Anything malformed **refuses startup** rather than silently meaning
-  unlimited; `[0, N]` disables an endpoint outright.
+  unlimited: a bad rule, a non-boolean `enabled`/`trust_forwarded_for` (JSON
+  `null`, `0`, or the *string* `"false"` — every non-empty string is truthy),
+  or a `rate_limit` that is not an object. `[0, N]` disables an endpoint
+  outright.
 * Denied requests are *not* recorded, so a client that keeps hammering cannot
   push its own window forward and lock itself out permanently.
 * State is an in-process dict behind a lock — granthi-link is one
   `ThreadingHTTPServer`, so that is the entire store. **If this ever runs
-  multi-process or multi-host, the limiter must move with it.** The key store
+  multi-process or multi-host, the limiter must move with it.** The clock is
-  is capped (`MAX_RATE_KEYS`); at capacity it drops least-recent windows and
+  read *inside* the lock; taken outside, racing threads append out of order
-  logs loudly, which is fail-open, chosen over an unbounded dict that is a
+  and both `Retry-After` and window reclamation silently go wrong.
-  memory DoS.
+* The key store is capped (`MAX_RATE_KEYS`). At capacity it reclaims expired
-* `trust_forwarded_for` is **off** by default. Turn it on only behind
+  windows, and if every window is still live it **refuses the new key** —
-  cloudflared, where every request otherwise arrives from the tunnel and one
+  fail closed. Evicting a live window would let an attacker who can mint many
-  abuser would starve everyone. A caller can *prepend* anything to
+  distinct keys clear their *own* limit on demand, which is worse than
-  `X-Forwarded-For`; a trusted proxy *appends* the peer it actually saw, so
+  turning a request away on an invite-only endpoint.
-  the service reads the **last** entry, never the first.
+* `trust_forwarded_for` is **off** by default, and turning it on **requires a
+  non-empty `trusted_proxies`** — the header is honored only when the socket
+  peer is in that list. Without it, anyone reaching the origin directly (it
+  also listens on the tailnet) could pick and rotate their own rate-limit key
+  just by sending a header. Turn it on when exposing behind cloudflared,
+  where every request otherwise arrives from the tunnel and one abuser would
+  starve everyone. A caller can *prepend* anything to `X-Forwarded-For`; a
+  trusted proxy *appends* the peer it actually saw, so the service reads the
+  **last** entry, never the first, and requires it to parse as a real IP.
 
 #### Identity binding (`state.json`)
 

server/granthi_link.py

@@ -52,6 +52,7 @@ Stdlib only. Python 3.9+.
 """
 
 import base64
+import ipaddress
 import json
 import logging
 import os
@@ -120,52 +121,93 @@ class RateLimiter:
         limit, window = rule
         if limit <= 0:                      # 0 = endpoint disabled entirely
             return False, window
-        now = self._clock()
         key = (route, client)
         with self._lock:
+            # Clock read INSIDE the lock: taken outside, two racing threads
+            # can append out of order, and both hits[0] (retry_after) and
+            # v[-1] (eviction age) assume the list is chronological.
+            now = self._clock()
             hits = [t for t in self._hits.get(key, ()) if now - t < window]
             if len(hits) >= limit:
                 self._hits[key] = hits
                 return False, max(1, int(window - (now - hits[0])) + 1)
+            if key not in self._hits and len(self._hits) >= self.max_keys:
+                # At capacity, reclaim expired keys first...
+                self._reclaim_expired(now)
+                if len(self._hits) >= self.max_keys:
+                    # ...and if every window is still live, this is an
+                    # identity flood, not organic load. Evicting here would
+                    # let an attacker reset their own limit on demand, so
+                    # refuse the NEW key instead. Fail closed: /v1/link is
+                    # invite-only and low-volume, so hitting this cap is an
+                    # attack, and turning tokens away beats minting them.
+                    LOG.error("rate limiter at capacity (%d keys) with no "
+                              "expired windows: refusing new client %r on %s",
+                              self.max_keys, client, route)
+                    return False, window
             hits.append(now)
             self._hits[key] = hits
-            if len(self._hits) > self.max_keys:
-                self._prune(now)
             return True, 0
 
-    def _prune(self, now):
+    def _reclaim_expired(self, now):
-        """Caller holds the lock. Drop expired keys; if still over the cap,
+        """Caller holds the lock. Drop only keys whose window has fully
-        drop the least-recently-hit. Dropping RESETS those clients' windows,
+        expired -- never a live one, or eviction becomes the bypass."""
-        which is fail-open -- but an unbounded dict is a memory DoS, and a
+        dead = [k for k, v in self._hits.items()
-        bounded one is the lesser failure. Say so loudly either way."""
+                if not v or now - v[-1] >= self.rules[k[0]][1]]
-        for k in [k for k, v in self._hits.items()
+        for k in dead:
-                  if not v or now - v[-1] >= self.rules[k[0]][1]]:
             del self._hits[k]
-        if len(self._hits) > self.max_keys:
+        if dead:
-            over = len(self._hits) - self.max_keys
+            LOG.info("rate limiter reclaimed %d expired windows", len(dead))
-            for k in sorted(self._hits, key=lambda k: self._hits[k][-1])[:over]:
-                del self._hits[k]
-            LOG.warning("rate limiter at capacity (%d keys): dropped %d "
-                        "least-recent windows; those clients start fresh",
-                        self.max_keys, over)
 
 
-def client_ip(handler, trust_forwarded_for):
+def client_ip(handler, trust_forwarded_for, trusted_proxies=()):
     """The address to rate-limit on.
 
     Behind cloudflared every request arrives from the tunnel, so limiting on
     the socket peer would let one abuser starve everyone. X-Forwarded-For is
-    client-controlled, though: a caller can prepend anything. A trusted proxy
+    client-controlled, though: a caller can prepend anything. Two guards:
-    APPENDS the peer it actually saw, so the LAST entry is the only one we did
+
-    not let the client choose -- take that, never the first. Off by default;
+    1. The header is honored ONLY when the socket peer is itself a configured
-    only enable when something trustworthy really is in front.
+       trusted proxy. Without that, anyone who can reach the origin directly
+       -- and the origin also listens on the tailnet -- picks their own
+       rate-limit key and rotates it at will.
+    2. A trusted proxy APPENDS the peer it actually saw, so the LAST entry is
+       the only one the client did not choose. Take that, never the first.
+
+    The value must parse as a real IP; junk falls back to the socket peer
+    rather than becoming a key of its own.
     """
-    if trust_forwarded_for:
+    peer = handler.client_address[0]
+    if not trust_forwarded_for:
+        return peer
+    if not _ip_in_any(peer, trusted_proxies):
+        LOG.warning("X-Forwarded-For ignored: peer %s is not a trusted proxy",
+                    peer)
+        return peer
     xff = handler.headers.get("X-Forwarded-For", "")
     parts = [p.strip() for p in xff.split(",") if p.strip()]
-        if parts:
+    if not parts:
-            return parts[-1]
+        return peer
-    return handler.client_address[0]
+    try:
+        return str(ipaddress.ip_address(parts[-1]))
+    except ValueError:
+        LOG.warning("X-Forwarded-For last hop %r is not an IP; using peer",
+                    parts[-1][:60])
+        return peer
+
+
+def _ip_in_any(addr, networks):
+    try:
+        ip = ipaddress.ip_address(addr)
+    except ValueError:
+        return False
+    for net in networks:
+        try:
+            if ip in ipaddress.ip_network(net, strict=False):
+                return True
+        except ValueError:
+            LOG.error("ignoring malformed trusted_proxies entry %r", net)
+    return False
 
 
 # --------------------------------------------------------------------------
@@ -294,8 +336,26 @@ class LinkService:
         # Rate limiting is ON by default: this endpoint creates accounts and
         # mints tokens, so the safe default is limited, and disabling it has
         # to be a deliberate config act rather than an omission.
-        rl = config.get("rate_limit") or {}
+        rl = config.get("rate_limit", {})
-        self.trust_forwarded_for = bool(rl.get("trust_forwarded_for", False))
+        if rl is None:
+            rl = {}
+        if not isinstance(rl, dict):
+            raise SystemExit("config rate_limit must be an object")
+        # Real JSON booleans only. `"enabled": null` or `0` must not quietly
+        # turn limiting off, and the string "false" must not turn XFF trust
+        # ON (every non-empty string is truthy).
+        for flag, default in (("enabled", True), ("trust_forwarded_for", False)):
+            if flag in rl and not isinstance(rl[flag], bool):
+                raise SystemExit(
+                    f"config rate_limit.{flag} must be true or false, "
+                    f"got {rl[flag]!r}")
+        self.trust_forwarded_for = rl.get("trust_forwarded_for", False)
+        self.trusted_proxies = tuple(rl.get("trusted_proxies") or ())
+        if self.trust_forwarded_for and not self.trusted_proxies:
+            raise SystemExit(
+                "config rate_limit.trust_forwarded_for requires a non-empty "
+                "trusted_proxies list -- trusting the header from any peer "
+                "lets callers choose their own rate-limit key")
         if rl.get("enabled", True):
             rules = dict(DEFAULT_RATE_RULES)
             for route, spec in (rl.get("rules") or {}).items():
@@ -308,9 +368,10 @@ class LinkService:
                         f"[max_requests, window_seconds] with window > 0")
                 rules[route] = (spec[0], spec[1])
             self.limiter = RateLimiter(rules)
-            LOG.info("rate limiting active: %s (trust_forwarded_for=%s)",
+            LOG.info("rate limiting active: %s (trust_forwarded_for=%s, "
+                     "trusted_proxies=%s)",
                      {k: f"{v[0]}/{v[1]}s" for k, v in rules.items()},
-                     self.trust_forwarded_for)
+                     self.trust_forwarded_for, list(self.trusted_proxies))
         else:
             self.limiter = None
             LOG.warning("rate limiting DISABLED by config -- /v1/link will "
@@ -571,7 +632,8 @@ class Handler(BaseHTTPRequestHandler):
         # to drain a body we already decided to reject.
         limiter = getattr(self.service, "limiter", None)
         if limiter is not None:
-            who = client_ip(self, self.service.trust_forwarded_for)
+            who = client_ip(self, self.service.trust_forwarded_for,
+                            self.service.trusted_proxies)
             allowed, retry_after = limiter.check(self.path, who)
             if not allowed:
                 LOG.warning("rate limited %s %s (retry after %ss)",

tests/test_server.py

@@ -585,19 +585,43 @@ class _FakeHandler:
 
 
 class TestClientIp(unittest.TestCase):
+    PROXY = ("10.0.0.0/8",)
+
     def test_socket_peer_by_default(self):
         h = _FakeHandler("5.5.5.5", xff="1.2.3.4")
-        self.assertEqual(granthi_link.client_ip(h, False), "5.5.5.5")
+        self.assertEqual(granthi_link.client_ip(h, False, self.PROXY), "5.5.5.5")
 
     def test_trusted_proxy_uses_the_last_hop_not_the_client_supplied_first(self):
         """A caller can PREPEND anything to X-Forwarded-For; a trusted proxy
         appends the peer it really saw. Only the last entry is trustworthy."""
         h = _FakeHandler("10.0.0.1", xff="1.2.3.4, 203.0.113.9")
-        self.assertEqual(granthi_link.client_ip(h, True), "203.0.113.9")
+        self.assertEqual(granthi_link.client_ip(h, True, self.PROXY),
+                         "203.0.113.9")
+
+    def test_untrusted_peer_cannot_choose_its_own_key(self):
+        """The origin also listens on the tailnet. Anyone reaching it
+        directly must not be able to pick (and rotate) their rate-limit key
+        just by sending a header."""
+        h = _FakeHandler("203.0.113.50", xff="9.9.9.9")
+        self.assertEqual(granthi_link.client_ip(h, True, self.PROXY),
+                         "203.0.113.50")
+
+    def test_non_ip_last_hop_falls_back_to_peer(self):
+        for junk in ("not-an-ip", "', OR 1=1", "x" * 500, "10.0.0.1:8080"):
+            with self.subTest(junk=junk):
+                h = _FakeHandler("10.0.0.1", xff=f"1.2.3.4, {junk}")
+                self.assertEqual(
+                    granthi_link.client_ip(h, True, self.PROXY), "10.0.0.1")
 
     def test_trusted_proxy_falls_back_when_header_absent(self):
         h = _FakeHandler("10.0.0.1")
-        self.assertEqual(granthi_link.client_ip(h, True), "10.0.0.1")
+        self.assertEqual(granthi_link.client_ip(h, True, self.PROXY),
+                         "10.0.0.1")
+
+    def test_malformed_trusted_proxy_entry_does_not_grant_trust(self):
+        h = _FakeHandler("10.0.0.1", xff="9.9.9.9")
+        self.assertEqual(
+            granthi_link.client_ip(h, True, ("not-a-cidr",)), "10.0.0.1")
 
 
 class TestRateLimitConfig(ServiceTestBase):
@@ -655,3 +679,94 @@ class TestRateLimitOverHttp(HandlerTestBase):
             conn.request("GET", "/health")
             self.assertEqual(conn.getresponse().status, 200)
             conn.close()
+
+
+class TestRateLimitHardening(unittest.TestCase):
+    """The four codex [P2] findings, each pinned by a test."""
+
+    def test_capacity_fails_closed_instead_of_resetting_a_live_window(self):
+        """Evicting a live window would let an attacker who can mint many
+        distinct keys clear their OWN limit on demand. New keys are refused
+        instead while every window is still live."""
+        now = [1000.0]
+        rl = granthi_link.RateLimiter(rules={"/v1/link": (1, 3600)},
+                                      max_keys=5, clock=lambda: now[0])
+        for i in range(5):
+            self.assertTrue(rl.check("/v1/link", f"10.0.0.{i}")[0])
+        victim_hits = list(rl._hits[("/v1/link", "10.0.0.0")])
+        for i in range(100, 140):                 # identity flood
+            allowed, retry = rl.check("/v1/link", f"10.0.1.{i}")
+            self.assertFalse(allowed)
+            self.assertGreater(retry, 0)
+        # the earlier client's window survived the flood untouched
+        self.assertEqual(rl._hits[("/v1/link", "10.0.0.0")], victim_hits)
+        self.assertLessEqual(len(rl._hits), 5)
+
+    def test_capacity_recovers_once_windows_expire(self):
+        now = [1000.0]
+        rl = granthi_link.RateLimiter(rules={"/v1/link": (1, 60)},
+                                      max_keys=3, clock=lambda: now[0])
+        for i in range(3):
+            rl.check("/v1/link", f"10.0.0.{i}")
+        self.assertFalse(rl.check("/v1/link", "10.0.9.9")[0])
+        now[0] += 61
+        self.assertTrue(rl.check("/v1/link", "10.0.9.9")[0])
+
+    def test_hits_stay_chronological_under_thread_contention(self):
+        """retry_after uses hits[0] and reclamation uses v[-1]; both assume
+        the list is ordered. Reading the clock outside the lock let racing
+        threads append out of order."""
+        rl = granthi_link.RateLimiter(rules={"/v1/link": (500, 3600)})
+        ts = [threading.Thread(target=lambda: rl.check("/v1/link", "7.7.7.7"))
+              for _ in range(200)]
+        for t in ts:
+            t.start()
+        for t in ts:
+            t.join()
+        hits = rl._hits[("/v1/link", "7.7.7.7")]
+        self.assertEqual(len(hits), 200)
+        self.assertEqual(hits, sorted(hits), "timestamps out of order")
+
+
+class TestRateLimitConfigTypes(ServiceTestBase):
+    def _svc(self, rl):
+        cfg = dict(self.svc.cfg)
+        cfg["rate_limit"] = rl
+        return granthi_link.LinkService(cfg)
+
+    def test_non_boolean_enabled_refuses_startup(self):
+        """`"enabled": null` or `0` must not quietly mean unlimited."""
+        for bad in (None, 0, "", "false", "no", []):
+            with self.subTest(enabled=bad):
+                with self.assertRaises(SystemExit):
+                    self._svc({"enabled": bad})
+
+    def test_string_false_does_not_enable_forwarded_trust(self):
+        """Every non-empty string is truthy -- "false" used to mean True."""
+        with self.assertRaises(SystemExit):
+            self._svc({"trust_forwarded_for": "false",
+                       "trusted_proxies": ["10.0.0.0/8"]})
+
+    def test_rate_limit_must_be_an_object(self):
+        for bad in ("yes", 5, ["/v1/link"]):
+            with self.subTest(rl=bad):
+                with self.assertRaises(SystemExit):
+                    self._svc(bad)
+
+    def test_null_rate_limit_means_defaults_not_disabled(self):
+        svc = self._svc(None)
+        self.assertIsNotNone(svc.limiter)
+
+    def test_forwarded_trust_without_trusted_proxies_refuses_startup(self):
+        """Trusting the header from ANY peer lets callers choose their own
+        rate-limit key -- that must not be reachable by omission."""
+        with self.assertRaises(SystemExit):
+            self._svc({"trust_forwarded_for": True})
+        with self.assertRaises(SystemExit):
+            self._svc({"trust_forwarded_for": True, "trusted_proxies": []})
+
+    def test_forwarded_trust_with_proxies_is_accepted(self):
+        svc = self._svc({"trust_forwarded_for": True,
+                         "trusted_proxies": ["10.0.0.0/8", "127.0.0.1/32"]})
+        self.assertTrue(svc.trust_forwarded_for)
+        self.assertEqual(len(svc.trusted_proxies), 2)