From 3f70c24350281d07a896c02e740d068a32760954 Mon Sep 17 00:00:00 2001 From: theartofsatoshi Date: Sun, 19 Jul 2026 11:28:07 -0500 Subject: [PATCH] fix(rate-limit): read proxy headers from both websockets shapes; stop CI/runtime drift MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found while assessing dependabot PR #28 (websockets >=16.1,<17.0). Two problems, one root cause. 1. A websockets upgrade would silently disable X-Forwarded-For. websockets 14 swapped `websockets.serve` from the legacy asyncio server to websockets.asyncio.server, and the two expose the handshake differently. Verified live against both: 13.1 serve -> websockets.legacy.server request_headers present, request absent 16.1 serve -> websockets.asyncio.server request_headers ABSENT, request present IPRateLimiter._header only looked at `websocket.request_headers`, via getattr(..., None). On the new server that yields None — so it would not raise, it would just never find the header again. With TRUST_PROXY on (as in production, behind a proxy on 172.18.0.0/16) every client would resolve to the proxy's own address and share a single rate-limit bucket: one abusive client throttles everyone, and is_exempt_peer sees a loopback peer for all of them. A silent availability regression that no test would catch. _header now also reads `websocket.request.headers`. Harmless on 13.x, where `request` is absent and the getattr chain skips it. 2. CI resolved websockets 15.0.1 while production runs 13.1. requirements-ci.txt allowed <16.0 while requirements.txt pinned <14.0 — the only constraint that differed between the two files. So CI exercised the new asyncio implementation while the server runs the legacy one, on the exact axis where they diverge. The drift was invisible because the rate-limiter tests fake the session object rather than standing up a real websockets server, so neither implementation was ever actually driven. Aligned requirements-ci.txt to <14.0 so CI tests what the server runs. Left requirements.txt alone: raising the runtime ceiling changes what the next image build installs and deserves its own deliberate testing (that is what PR #28 is for), not a drift fix. 5 new tests cover both websocket shapes plus a connection exposing neither (must degrade to the socket peer, not raise). Verified the three new-shape tests fail without the _header change while the legacy ones still pass. Suite: 1001 passed, 0 failed. Co-Authored-By: Claude Opus 4.8 --- electrumx/server/rate_limiter.py | 19 +++++- requirements-ci.txt | 10 +++- tests/server/test_ip_rate_limiter.py | 90 ++++++++++++++++++++++++++++ 3 files changed, 116 insertions(+), 3 deletions(-) diff --git a/electrumx/server/rate_limiter.py b/electrumx/server/rate_limiter.py index 975d239..8a54e06 100644 --- a/electrumx/server/rate_limiter.py +++ b/electrumx/server/rate_limiter.py @@ -334,8 +334,21 @@ def _header(session, name: str) -> Optional[str]: """Best-effort read of a request header from a (WS) session/transport. Supports several shapes so tests and the live WS transport both work: - ``session.request_headers``, ``session.transport.websocket.request_headers`` - and a plain ``session.headers`` mapping. Returns None if unavailable. + ``session.request_headers``, ``session.transport.websocket.request_headers``, + ``…websocket.request.headers`` and a plain ``session.headers`` mapping. + Returns None if unavailable. + + The two websocket shapes are a library-version split, and getting it + wrong fails *silently* rather than loudly — which is why both are + covered here. ``websockets.serve`` resolves to the legacy asyncio + server below 14 and to ``websockets.asyncio.server`` from 14 on, and + the new server does not carry ``request_headers`` at all; the handshake + request moved to ``websocket.request``. Since this helper falls back to + None on a missing attribute, a version bump alone would leave + X-Forwarded-For permanently unresolved: with TRUST_PROXY on, every + client would collapse onto the proxy's own address and share one + rate-limit bucket. Verified against 13.1 (``request_headers`` present, + ``request`` absent) and 16.1 (the reverse). """ candidates = [] for obj in (session, getattr(session, 'transport', None)): @@ -344,6 +357,8 @@ def _header(session, name: str) -> Optional[str]: ws = getattr(obj, 'websocket', None) if ws is not None: candidates.append(getattr(ws, 'request_headers', None)) + candidates.append( + getattr(getattr(ws, 'request', None), 'headers', None)) candidates.append(getattr(obj, 'request_headers', None)) candidates.append(getattr(obj, 'headers', None)) for headers in candidates: diff --git a/requirements-ci.txt b/requirements-ci.txt index 46e125c..b7717cf 100644 --- a/requirements-ci.txt +++ b/requirements-ci.txt @@ -3,7 +3,15 @@ attrs plyvel pylru aiohttp>=3.3,<4.0 -websockets>=10.0,<16.0 +# Must track requirements.txt exactly. websockets 14 swapped `websockets.serve` +# from the legacy asyncio server to websockets.asyncio.server, and the two +# expose the handshake differently (legacy: connection.request_headers; new: +# connection.request.headers). This file used to allow <16.0 while the runtime +# pinned <14.0, so CI resolved 15.0.1 against a production that runs 13.1 — +# i.e. CI exercised a different websockets implementation than the server +# actually uses, on the exact axis where they diverge. Bump both together, +# deliberately, not one as a side effect. +websockets>=10.0,<14.0 psutil cbor2>=5.4.0 diff --git a/tests/server/test_ip_rate_limiter.py b/tests/server/test_ip_rate_limiter.py index 7839660..02ba39e 100644 --- a/tests/server/test_ip_rate_limiter.py +++ b/tests/server/test_ip_rate_limiter.py @@ -656,3 +656,93 @@ def test_connection_cap_blocked_ip_refused(): allowed, reason = rl.check_can_register(ip, now=1000.0) assert allowed is False assert 'blocked' in reason + + +# --------------------------------------------------------------------------- # +# Header discovery across websockets-library shapes. +# +# `websockets.serve` resolves to the legacy asyncio server below 14 and to +# websockets.asyncio.server from 14 on. The legacy connection exposes +# `request_headers`; the new one does NOT -- the handshake request moved to +# `connection.request`, with headers underneath it. Verified live against 13.1 +# (request_headers present, request absent) and 16.1 (the reverse). +# +# _header falls back to None on a missing attribute, so a websockets upgrade +# alone would not raise -- X-Forwarded-For would just never resolve again, and +# with TRUST_PROXY on every client would collapse onto the proxy's own address +# and share a single rate-limit bucket. These pin both shapes so the upgrade +# cannot regress it silently. +# --------------------------------------------------------------------------- # + +class _FakeRequest: + """websockets >=14: connection.request.headers.""" + + def __init__(self, headers): + self.headers = _FakeHeaders(headers) + + +class _FakeWSLegacy: + """websockets <14: connection.request_headers.""" + + def __init__(self, headers): + self.request_headers = _FakeHeaders(headers) + + +class _FakeWSNew: + def __init__(self, headers): + self.request = _FakeRequest(headers) + + +class _FakeTransport: + def __init__(self, websocket): + self.websocket = websocket + + +class _WSSession: + """A session whose headers are reachable only via transport.websocket, + as with the live WS transport (see HTTPTransport in httpserver.py).""" + + def __init__(self, websocket, peer_host='127.0.0.1', port=50002): + self.transport = _FakeTransport(websocket) + self._peer = NetAddress(peer_host, port) + self.session_id = 0 + self.client_ip = None + + def remote_address(self): + return self._peer + + +def test_xff_via_legacy_websocket_shape(): + rl = IPRateLimiter(_env(trust_proxy=True, trust_proxy_hops=1)) + sess = _WSSession(_FakeWSLegacy({'X-Forwarded-For': '203.0.113.7'})) + assert rl.client_ip(sess) == '203.0.113.7' + + +def test_xff_via_new_websocket_request_shape(): + """websockets >=14: headers live at connection.request.headers.""" + rl = IPRateLimiter(_env(trust_proxy=True, trust_proxy_hops=1)) + sess = _WSSession(_FakeWSNew({'X-Forwarded-For': '203.0.113.7'})) + assert rl.client_ip(sess) == '203.0.113.7' + + +def test_x_real_ip_via_new_websocket_request_shape(): + rl = IPRateLimiter(_env(trust_proxy=True)) + sess = _WSSession(_FakeWSNew({'X-Real-IP': '203.0.113.42'})) + assert rl.client_ip(sess) == '203.0.113.42' + + +def test_new_shape_multi_hop_selection(): + rl = IPRateLimiter(_env(trust_proxy=True, trust_proxy_hops=2)) + sess = _WSSession(_FakeWSNew( + {'X-Forwarded-For': '203.0.113.7, 198.51.100.9, 10.0.0.1'})) + assert rl.client_ip(sess) == '198.51.100.9' + + +def test_websocket_without_either_shape_falls_back_to_peer(): + """A connection object exposing neither attribute must degrade to the + socket peer rather than raising.""" + class _Bare: + pass + rl = IPRateLimiter(_env(trust_proxy=True)) + sess = _WSSession(_Bare(), peer_host='8.8.8.8') + assert rl.client_ip(sess) == '8.8.8.8'