From 639903958911eb7df036aab214122269967feabf Mon Sep 17 00:00:00 2001 From: Indiana Date: Wed, 22 Jul 2026 23:59:35 +0000 Subject: [PATCH] fix: resolve real visitor IP via CF-Connecting-IP for per-IP limiting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit websocket.client.host is always the Cloudflare Tunnel machine's LAN IP for every internet-facing connection (the tunnel runs on a separate machine and terminates TLS there), which collapsed per-IP rate limiting into a single shared bucket for all remote visitors — the exact gap flagged in review. Cloudflare's edge sets CF-Connecting-IP itself, overwriting any client-supplied value, so it's safe to trust when present. Falls back to the raw socket peer for direct LAN/local access. --- backend/app/rate_limit.py | 19 ++++++++++++++++++ backend/app/ws.py | 5 +++-- backend/tests/test_rate_limit.py | 19 +++++++++++++++++- backend/tests/test_ws_session.py | 33 ++++++++++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 3 deletions(-) diff --git a/backend/app/rate_limit.py b/backend/app/rate_limit.py index d6dc4c6..918699d 100644 --- a/backend/app/rate_limit.py +++ b/backend/app/rate_limit.py @@ -1,5 +1,24 @@ import time from collections import defaultdict +from typing import Mapping + + +def resolve_client_ip(headers: Mapping[str, str], direct_host: str | None) -> str: + """Resolves the real visitor IP for per-IP rate limiting. + + The App CT sits behind a Cloudflare Tunnel that runs on a separate + machine (see README Architecture) — every internet-facing connection's + raw TCP peer is that tunnel machine, not the visitor, which would + collapse per-IP limiting to a single shared bucket for all remote + traffic. Cloudflare's edge sets `CF-Connecting-IP` itself, stripping any + client-supplied value first, so it's safe to trust here. Direct + LAN/local access (no Cloudflare in front, e.g. local dev) has no such + header and falls back to the raw socket peer. + """ + forwarded = headers.get("cf-connecting-ip") + if forwarded: + return forwarded + return direct_host or "unknown" class RateLimiter: diff --git a/backend/app/ws.py b/backend/app/ws.py index 6f2f17f..1ea4e36 100644 --- a/backend/app/ws.py +++ b/backend/app/ws.py @@ -36,7 +36,7 @@ from app.models.contact_session import ContactSession from app.models.entity import Entity from app.models.entity_sighting import EntitySighting from app.models.event import Event -from app.rate_limit import RateLimiter +from app.rate_limit import RateLimiter, resolve_client_ip from app.telemetry import detect_wire_spike, sample_network from app.tts.piper import synthesize_spirit_voice from app.tts.voices import pick_voice @@ -103,7 +103,8 @@ def serialize_entity(entity: Entity) -> dict: def _client_ip(websocket: WebSocket) -> str: - return websocket.client.host if websocket.client else "unknown" + host = websocket.client.host if websocket.client else None + return resolve_client_ip(websocket.headers, host) async def _authenticate(websocket: WebSocket) -> uuid.UUID | None: diff --git a/backend/tests/test_rate_limit.py b/backend/tests/test_rate_limit.py index dc331e0..83122fa 100644 --- a/backend/tests/test_rate_limit.py +++ b/backend/tests/test_rate_limit.py @@ -1,6 +1,6 @@ from unittest.mock import patch -from app.rate_limit import RateLimiter +from app.rate_limit import RateLimiter, resolve_client_ip def test_allows_up_to_limit_then_blocks(): @@ -29,3 +29,20 @@ def test_hits_expire_after_window_elapses(): with patch("app.rate_limit.time.monotonic", return_value=111.0): # 11 seconds later, both prior hits (at t=100) are older than window_start (111 - 10 = 101) assert limiter.allow("user-1") is True + + +def test_resolve_client_ip_prefers_cf_connecting_ip_over_socket_peer(): + # The App CT sits behind a Cloudflare Tunnel on a separate machine — the + # raw socket peer is always the tunnel, never the visitor, for every + # internet-facing request. + headers = {"cf-connecting-ip": "203.0.113.7"} + assert resolve_client_ip(headers, "10.30.20.1") == "203.0.113.7" + + +def test_resolve_client_ip_falls_back_to_socket_peer_without_header(): + # Direct LAN/local access (no Cloudflare in front) has no such header. + assert resolve_client_ip({}, "10.30.20.1") == "10.30.20.1" + + +def test_resolve_client_ip_falls_back_to_unknown_with_no_peer_or_header(): + assert resolve_client_ip({}, None) == "unknown" diff --git a/backend/tests/test_ws_session.py b/backend/tests/test_ws_session.py index 99bb55f..b06a9cc 100644 --- a/backend/tests/test_ws_session.py +++ b/backend/tests/test_ws_session.py @@ -232,3 +232,36 @@ async def test_summon_rate_limited_per_ip_even_with_fresh_account( assert rejection["message"] == ( "The veil is crowded. The spirits need a moment before another summoning." ) + + +@pytest.mark.asyncio +async def test_summon_per_ip_bucket_follows_cf_connecting_ip_not_socket_peer( + sync_client, monkeypatch +): + # Every connection in this test suite shares the same simulated socket + # peer (TestClient has no real network). Without preferring + # CF-Connecting-IP, distinct visitors behind the Cloudflare Tunnel would + # collapse into one shared per-IP bucket — this proves they don't. + monkeypatch.setattr( + app.ws, "summon_ip_limiter", RateLimiter(max_requests=1, window_seconds=60) + ) + + token_a = _login(sync_client, "cf-ip-a") + with sync_client.websocket_connect( + "/ws/session", + headers={"cookie": f"qm_session={token_a}", "cf-connecting-ip": "203.0.113.1"}, + ) as ws: + _read_until(ws, "session") + ws.send_json({"type": "summon"}) + _read_until(ws, "entity") # spends visitor A's per-IP slot only + + token_b = _login(sync_client, "cf-ip-b") + with sync_client.websocket_connect( + "/ws/session", + headers={"cookie": f"qm_session={token_b}", "cf-connecting-ip": "203.0.113.2"}, + ) as ws: + _read_until(ws, "session") + ws.send_json({"type": "summon"}) + # A different visitor IP behind the same tunnel — must not be + # rejected by visitor A's already-spent bucket. + _read_until(ws, "entity")