fix(rate-limit): read proxy headers from both websockets shapes; stop CI/runtime drift - #32
Merged
Merged
Conversation
… CI/runtime drift 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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found while assessing dependabot #28 (
websockets >=16.1,<17.0). Two problems, one root cause. This is a prerequisite for #28 — merge this first.1. A websockets upgrade would silently disable X-Forwarded-For
websockets 14 swapped
websockets.servefrom the legacy asyncio server towebsockets.asyncio.server, and the two expose the handshake differently. Verified live against both:serveresolves torequest_headersrequestwebsockets.legacy.serverwebsockets.asyncio.serverIPRateLimiter._headeronly looked atwebsocket.request_headers, viagetattr(..., None). On the new server that yieldsNone— so it would not raise, it would just never find the header again.With
TRUST_PROXYon (as in production, behind a proxy on172.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, andis_exempt_peersees a loopback peer for all of them. A silent availability regression no test would catch._headernow also readswebsocket.request.headers. Harmless on 13.x, whererequestis absent and the getattr chain skips it.2. CI resolved websockets 15.0.1 while production runs 13.1
requirements-ci.txtallowed<16.0whilerequirements.txtpinned<14.0— the only constraint differing between the two files. CI has been exercising the new asyncio implementation while the server runs the legacy one, on the exact axis where they diverge.It stayed 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.txtto<14.0so CI tests what the server runs. Leftrequirements.txtalone — raising the runtime ceiling changes what the next image build installs and deserves its own deliberate testing (that is what #28 is for), not a drift fix.Testing
5 new tests: both websocket shapes, X-Real-IP on the new shape, multi-hop selection on the new shape, and a connection exposing neither attribute (must degrade to the socket peer, not raise).
Verified the three new-shape tests fail without the
_headerchange while the legacy ones still pass.Suite: 1001 passed, 0 failed.
🤖 Generated with Claude Code