client_ip() read the first element of X-Forwarded-For, which is correct only if
the proxy replaces the header. Caddy appends the peer address to whatever the
client sent, so element 0 was whatever the caller claimed: rotating a fake value
per request minted a fresh identity every time and walked straight through the
login and registration throttles (B-33) and the SSE per-IP subscriber cap
(B-38). Only the per-username login bucket, which doesn't key on the IP, still
bit.
Both halves of the audit's fix, since they hold independently:
- the Caddyfile overwrites the header with `header_up X-Forwarded-For
{remote_host}`, so what reaches the app is the actual peer and nothing else.
This is the one that makes the app's assumption true at the source.
- client_ip() reads the *last* hop rather than the first — the element written
by the hop closest to us, i.e. by our own proxy. Exactly one trusted proxy
sits in front of the app (`app` is only `expose`d on the compose network,
never published to the host), so that element is the real peer.
An empty or comma-only header now falls back to request.client.host instead of
returning "", which was its own shared-bucket evasion.
Regression tests both sides: two requests spoofing different prefixes must key
to the same IP, and the Caddyfile must keep the header_up directive (checked by
`caddy validate`).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GET /rounds/stream capped concurrent subscribers with one global
counter (MAX_SUBSCRIBERS=500): anyone opening 500 connections degraded
every other user to polling. The comment called it a defensive cap;
it was actually the vector, since nothing stopped a single source from
exhausting it alone.
RoundEventBroadcaster now also tracks subscribers per client IP,
capped much lower (MAX_SUBSCRIBERS_PER_IP=5). Past that cap, opening
one more stream evicts that same IP's own oldest connection (woken via
a new EVICTED sentinel so the SSE generator closes it promptly) rather
than refusing the new one or letting one abusive IP crowd out unrelated
clients under the old global-only cap. The global cap stays as a
backstop against overall resource exhaustion regardless of source.
Extracted client_ip() (X-Forwarded-For, since Caddy reverse-proxies
every request) out of auth/routes.py into app/api/client_ip.py so the
login/registration throttles (B-33) and this new per-IP cap share one
definition instead of two that could drift apart.
Not implemented: enforcing the connection cap at the Caddy layer
itself, which the proposed fix also suggested - the standard Caddy
image this project uses has no such directive without a third-party
module, and building a custom image felt like a bigger, separate change
than this fix warranted.
Suite grows from 201 to 211 tests. BUGS.md moves B-38 to Previously
fixed.
davideandClaude Opus 5