fix: resolve real visitor IP via CF-Connecting-IP for per-IP limiting
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.
This commit is contained in:
@@ -1,5 +1,24 @@
|
|||||||
import time
|
import time
|
||||||
from collections import defaultdict
|
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:
|
class RateLimiter:
|
||||||
|
|||||||
@@ -36,7 +36,7 @@ from app.models.contact_session import ContactSession
|
|||||||
from app.models.entity import Entity
|
from app.models.entity import Entity
|
||||||
from app.models.entity_sighting import EntitySighting
|
from app.models.entity_sighting import EntitySighting
|
||||||
from app.models.event import Event
|
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.telemetry import detect_wire_spike, sample_network
|
||||||
from app.tts.piper import synthesize_spirit_voice
|
from app.tts.piper import synthesize_spirit_voice
|
||||||
from app.tts.voices import pick_voice
|
from app.tts.voices import pick_voice
|
||||||
@@ -103,7 +103,8 @@ def serialize_entity(entity: Entity) -> dict:
|
|||||||
|
|
||||||
|
|
||||||
def _client_ip(websocket: WebSocket) -> str:
|
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:
|
async def _authenticate(websocket: WebSocket) -> uuid.UUID | None:
|
||||||
|
|||||||
@@ -1,6 +1,6 @@
|
|||||||
from unittest.mock import patch
|
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():
|
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):
|
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)
|
# 11 seconds later, both prior hits (at t=100) are older than window_start (111 - 10 = 101)
|
||||||
assert limiter.allow("user-1") is True
|
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"
|
||||||
|
|||||||
@@ -232,3 +232,36 @@ async def test_summon_rate_limited_per_ip_even_with_fresh_account(
|
|||||||
assert rejection["message"] == (
|
assert rejection["message"] == (
|
||||||
"The veil is crowded. The spirits need a moment before another summoning."
|
"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")
|
||||||
|
|||||||
Reference in New Issue
Block a user