fix(link): 3 more codex [P2] findings — per-route caps, bool rules, proxy validation
Round 2 of review on the same branch: - The fail-closed capacity guard was itself a DoS lever. MAX_RATE_KEYS was global, and the limiter runs before auth, so a flood of cheap /v1/repos keys could exhaust the table and 429 never-seen /v1/link clients until live windows expired. Budgets are now per route. - Rule values accepted booleans: bool subclasses int, so isinstance let [5, true] through as a 1-SECOND window (5/hour -> ~5/sec) and false in the limit slot disabled the endpoint. Now `type(x) is int`. - trusted_proxies was unvalidated: a bare string would be iterated character by character, malformed entries only surfaced as a per-request log line, and 0.0.0.0/0 or ::/0 restored "trust XFF from any peer" — the exact hole the setting closes. Now parsed and validated once at startup, wildcards refused, and _ip_in_any takes pre-parsed networks so nothing can degrade to a silent per-request skip. Tests 99 -> 108: cross-route flood isolation, per-route reclamation windows, every bool-in-rule position, bare-string and wildcard proxies, and a v4/v6 mismatch case. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01LTARYHX7GPepi3CH3tp5pg
This commit is contained in:
Nirav Patel
co-authored by
Claude Opus 5
Claude Opus 5
parent
c442721aff
commit
c742ca4798
+73
-26
@@ -104,9 +104,13 @@ class RateLimiter:
|
||||
|
||||
def __init__(self, rules=None, max_keys=MAX_RATE_KEYS, clock=None):
|
||||
self.rules = dict(rules or DEFAULT_RATE_RULES)
|
||||
# Per-ROUTE budget, not one global table. A shared cap lets a flood of
|
||||
# cheap /v1/repos keys exhaust the table and lock brand-new /v1/link
|
||||
# clients out -- turning the fail-closed capacity guard into a
|
||||
# cross-route denial of service. Each route gets its own space.
|
||||
self.max_keys = max_keys
|
||||
self._clock = clock or time.monotonic
|
||||
self._hits = {}
|
||||
self._hits = {route: {} for route in self.rules}
|
||||
self._lock = threading.Lock()
|
||||
|
||||
def check(self, route, client):
|
||||
@@ -121,43 +125,45 @@ class RateLimiter:
|
||||
limit, window = rule
|
||||
if limit <= 0: # 0 = endpoint disabled entirely
|
||||
return False, window
|
||||
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.
|
||||
# v[-1] (reclamation age) assume the list is chronological.
|
||||
now = self._clock()
|
||||
hits = [t for t in self._hits.get(key, ()) if now - t < window]
|
||||
table = self._hits.setdefault(route, {})
|
||||
hits = [t for t in table.get(client, ()) if now - t < window]
|
||||
if len(hits) >= limit:
|
||||
self._hits[key] = hits
|
||||
table[client] = hits
|
||||
return False, max(1, int(window - (now - hits[0])) + 1)
|
||||
if key not in self._hits and len(self._hits) >= self.max_keys:
|
||||
if client not in table and len(table) >= self.max_keys:
|
||||
# At capacity, reclaim expired keys first...
|
||||
self._reclaim_expired(now)
|
||||
if len(self._hits) >= self.max_keys:
|
||||
self._reclaim_expired(route, now)
|
||||
if len(table) >= 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)
|
||||
LOG.error("rate limiter at capacity for %s (%d keys) with "
|
||||
"no expired windows: refusing new client %r",
|
||||
route, self.max_keys, client)
|
||||
return False, window
|
||||
hits.append(now)
|
||||
self._hits[key] = hits
|
||||
table[client] = hits
|
||||
return True, 0
|
||||
|
||||
def _reclaim_expired(self, now):
|
||||
def _reclaim_expired(self, route, now):
|
||||
"""Caller holds the lock. Drop only keys whose window has fully
|
||||
expired -- never a live one, or eviction becomes the bypass."""
|
||||
dead = [k for k, v in self._hits.items()
|
||||
if not v or now - v[-1] >= self.rules[k[0]][1]]
|
||||
for k in dead:
|
||||
del self._hits[k]
|
||||
expired -- never a live one, or reclamation becomes the bypass."""
|
||||
window = self.rules[route][1]
|
||||
table = self._hits.get(route, {})
|
||||
dead = [c for c, v in table.items() if not v or now - v[-1] >= window]
|
||||
for c in dead:
|
||||
del table[c]
|
||||
if dead:
|
||||
LOG.info("rate limiter reclaimed %d expired windows", len(dead))
|
||||
LOG.info("rate limiter reclaimed %d expired windows on %s",
|
||||
len(dead), route)
|
||||
|
||||
|
||||
def client_ip(handler, trust_forwarded_for, trusted_proxies=()):
|
||||
@@ -202,11 +208,10 @@ def _ip_in_any(addr, networks):
|
||||
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)
|
||||
# Pre-parsed at startup by LinkService._parse_trusted_proxies, so a
|
||||
# malformed entry can never reach here as a silent per-request skip.
|
||||
if ip.version == net.version and ip in net:
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
@@ -350,7 +355,8 @@ class LinkService:
|
||||
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 ())
|
||||
self.trusted_proxies = self._parse_trusted_proxies(
|
||||
rl.get("trusted_proxies"))
|
||||
if self.trust_forwarded_for and not self.trusted_proxies:
|
||||
raise SystemExit(
|
||||
"config rate_limit.trust_forwarded_for requires a non-empty "
|
||||
@@ -359,8 +365,12 @@ class LinkService:
|
||||
if rl.get("enabled", True):
|
||||
rules = dict(DEFAULT_RATE_RULES)
|
||||
for route, spec in (rl.get("rules") or {}).items():
|
||||
# `type(x) is int`, NOT isinstance: bool subclasses int, so
|
||||
# isinstance lets [5, true] through as a 1-SECOND window --
|
||||
# an hourly limit silently becomes ~5/sec -- and false in the
|
||||
# limit slot disables the endpoint.
|
||||
if (not isinstance(spec, (list, tuple)) or len(spec) != 2
|
||||
or not all(isinstance(x, int) for x in spec)
|
||||
or not all(type(x) is int for x in spec)
|
||||
or spec[1] <= 0):
|
||||
# A malformed rule must not silently mean "unlimited".
|
||||
raise SystemExit(
|
||||
@@ -377,6 +387,43 @@ class LinkService:
|
||||
LOG.warning("rate limiting DISABLED by config -- /v1/link will "
|
||||
"mint tokens without any throttle")
|
||||
|
||||
@staticmethod
|
||||
def _parse_trusted_proxies(raw):
|
||||
"""Validate at STARTUP, not per request.
|
||||
|
||||
This setting gates a spoofable identity, so every failure mode has to
|
||||
be loud and early: a bare string would be iterated character by
|
||||
character (each char a "network"), malformed entries would only
|
||||
surface as a per-request log line, and a wildcard like 0.0.0.0/0 or
|
||||
::/0 quietly restores "trust X-Forwarded-For from anyone" -- the exact
|
||||
hole trusted_proxies exists to close.
|
||||
"""
|
||||
if raw is None:
|
||||
return ()
|
||||
if isinstance(raw, str) or not isinstance(raw, (list, tuple)):
|
||||
raise SystemExit(
|
||||
"config rate_limit.trusted_proxies must be a list of CIDRs, "
|
||||
f"got {type(raw).__name__}")
|
||||
nets = []
|
||||
for entry in raw:
|
||||
if not isinstance(entry, str):
|
||||
raise SystemExit(
|
||||
f"config rate_limit.trusted_proxies entry {entry!r} "
|
||||
"must be a string")
|
||||
try:
|
||||
net = ipaddress.ip_network(entry, strict=False)
|
||||
except ValueError as e:
|
||||
raise SystemExit(
|
||||
f"config rate_limit.trusted_proxies entry {entry!r} "
|
||||
f"is not a valid network: {e}")
|
||||
if net.prefixlen == 0:
|
||||
raise SystemExit(
|
||||
f"config rate_limit.trusted_proxies entry {entry!r} "
|
||||
"matches every address, which is the same as trusting "
|
||||
"X-Forwarded-For from any peer -- refusing")
|
||||
nets.append(net)
|
||||
return tuple(nets)
|
||||
|
||||
def test_mode_enabled(self):
|
||||
"""Config test_mode is honored ONLY with the env gate also set."""
|
||||
if not self.cfg.get("test_mode"):
|
||||
|
||||
Reference in New Issue
Block a user