diff --git a/src/backend/api/middleware/rate_limit.py b/src/backend/api/middleware/rate_limit.py index 48f8a4c..ccd1576 100644 --- a/src/backend/api/middleware/rate_limit.py +++ b/src/backend/api/middleware/rate_limit.py @@ -20,6 +20,11 @@ class RateLimitMiddleware(BaseHTTPMiddleware): - Login endpoint (/api/auth/login): limited to `rate_limit_login` req/min. - All other endpoints: limited to `rate_limit_general` req/min. Returns HTTP 429 with Retry-After header when limit exceeded. + + Both configured limits are divided across worker processes before being + applied (see `_effective_limit`): each bucket lives in this process's + memory, so with `--workers N` a single configured limit is enforced N + times over across the cluster unless it is divided down first. """ LOGIN_PATH = "/api/auth/login" @@ -34,23 +39,48 @@ class RateLimitMiddleware(BaseHTTPMiddleware): @staticmethod def _client_ip(request: Request) -> str: - """Resolve the originating client IP, honoring proxy headers. + """Resolve the originating client IP behind `trusted_proxy_count` proxies. - Order of precedence: ``X-Forwarded-For`` (first hop), ``X-Real-IP``, - ``request.client.host``. Required because Nginx and the Flask client - sit between the tablet and the API; without parsing these headers - every tablet shares one bucket. + Nginx (and, behind it, the Flask client) each add one hop to + X-Forwarded-For. The *leftmost* entries are whatever the original + caller claimed and are trivially spoofed; trust only extends to the + hops actually appended by our own trusted proxies, counted from the + *right*. With `trusted_proxy_count = N`, the client IP is the Nth + element from the end of XFF (the one added by the outermost trusted + proxy). If XFF has fewer elements than that, or is absent, fall back + to `request.client.host` - except that `X-Real-IP` is tried first when + XFF is simply missing and at least one proxy is trusted. + + With `trusted_proxy_count = 0`, no proxy header is honored at all + (neither XFF nor X-Real-IP) and only `request.client.host` is used. """ - xff = request.headers.get("x-forwarded-for") - if xff: - first = xff.split(",")[0].strip() - if first: - return first - real = request.headers.get("x-real-ip") - if real: - return real.strip() + count = settings.trusted_proxy_count + if count > 0: + xff = request.headers.get("x-forwarded-for") + if xff: + hops = [h.strip() for h in xff.split(",") if h.strip()] + if len(hops) >= count: + return hops[-count] + # Fewer hops than trusted proxies: the chain is short of what + # was configured, so fall back rather than trust an + # under-populated header. + else: + real = request.headers.get("x-real-ip") + if real: + return real.strip() return request.client.host if request.client else "unknown" + @staticmethod + def _effective_limit(configured: int) -> int: + """Divide a configured per-minute limit across worker processes. + + Rounds up and never returns less than 1, so a low configured limit + with high concurrency still allows at least one request per worker + per window rather than locking everyone out. + """ + concurrency = max(settings.web_concurrency, 1) + return max(1, -(-configured // concurrency)) + def _clean_window(self, timestamps: list[float], now: float) -> list[float]: """Remove timestamps outside the current sliding window.""" cutoff = now - self.WINDOW_SECONDS @@ -100,7 +130,8 @@ class RateLimitMiddleware(BaseHTTPMiddleware): # Check login-specific rate limit if path == self.LOGIN_PATH and request.method == "POST": allowed, retry_after = self._check_rate_limit( - self._login_requests, client_ip, settings.rate_limit_login, now + self._login_requests, client_ip, + self._effective_limit(settings.rate_limit_login), now, ) if not allowed: return JSONResponse( @@ -111,7 +142,8 @@ class RateLimitMiddleware(BaseHTTPMiddleware): # Check general rate limit allowed, retry_after = self._check_rate_limit( - self._general_requests, client_ip, settings.rate_limit_general, now + self._general_requests, client_ip, + self._effective_limit(settings.rate_limit_general), now, ) if not allowed: return JSONResponse( diff --git a/src/backend/config.py b/src/backend/config.py index 7eff6d7..479f39f 100644 --- a/src/backend/config.py +++ b/src/backend/config.py @@ -18,6 +18,16 @@ class Settings(BaseSettings): server_port: int = 8000 server_secret_key: str = "change-this-to-a-random-secret-key" server_cors_origins: str = "http://localhost:5000" + # Worker processes (e.g. `uvicorn --workers N`). Each rate-limit bucket lives + # in process memory, so this is how RateLimitMiddleware divides the + # configured limits to keep the cluster-wide effective limit close to what + # was configured instead of multiplying it by N. + web_concurrency: int = 1 + # How many reverse proxies between the client and this API are trusted to + # have appended their own hop to X-Forwarded-For (Nginx, in the default + # deploy). Used by RateLimitMiddleware._client_ip. 0 disables proxy header + # parsing entirely (only request.client.host is trusted). + trusted_proxy_count: int = 1 # File Storage upload_dir: str = "uploads" diff --git a/src/backend/tests/test_rate_limit_proxy.py b/src/backend/tests/test_rate_limit_proxy.py index b8f1bb2..a81a031 100644 --- a/src/backend/tests/test_rate_limit_proxy.py +++ b/src/backend/tests/test_rate_limit_proxy.py @@ -1,5 +1,8 @@ """Tests for RateLimitMiddleware honoring proxy headers (X-Forwarded-For).""" +import pytest + from src.backend.api.middleware.rate_limit import RateLimitMiddleware +from src.backend.config import settings class _FakeRequest: @@ -18,12 +21,33 @@ class _FakeRequest: self.client.host = host -def test_client_ip_uses_x_forwarded_for_first_hop(): +@pytest.fixture(autouse=True) +def default_trusted_proxy_count(monkeypatch): + """Every test in this file assumes the default (1 trusted proxy).""" + monkeypatch.setattr(settings, "trusted_proxy_count", 1) + + +def test_client_ip_uses_last_trusted_hop_of_x_forwarded_for(): + """With one trusted proxy, the client IP is the LAST element of XFF - + + the one appended by that proxy - not the first, which the caller supplies + and can set to anything. + """ req = _FakeRequest( headers={"x-forwarded-for": "203.0.113.5, 10.0.0.2"}, host="10.0.0.1", ) - assert RateLimitMiddleware._client_ip(req) == "203.0.113.5" + assert RateLimitMiddleware._client_ip(req) == "10.0.0.2" + + +def test_client_ip_ignores_spoofed_first_hop(): + """Rotating the client-supplied (leftmost) part of XFF changes nothing: + + the trusted, rightmost hop stays the same, so the bucket key is stable. + """ + req1 = _FakeRequest(headers={"x-forwarded-for": "1.1.1.1, 10.0.0.2"}) + req2 = _FakeRequest(headers={"x-forwarded-for": "9.9.9.9, 10.0.0.2"}) + assert RateLimitMiddleware._client_ip(req1) == RateLimitMiddleware._client_ip(req2) == "10.0.0.2" def test_client_ip_strips_whitespace(): @@ -63,3 +87,55 @@ def test_empty_x_forwarded_for_falls_through(): host="10.0.0.1", ) assert RateLimitMiddleware._client_ip(req) == "203.0.113.42" + + +def test_x_forwarded_for_shorter_than_trusted_proxy_count_falls_back(monkeypatch): + """Two trusted proxies configured, but XFF has only one hop: falls back to + request.client.host rather than trusting an under-populated header.""" + monkeypatch.setattr(settings, "trusted_proxy_count", 2) + req = _FakeRequest( + headers={"x-forwarded-for": "203.0.113.5"}, + host="172.18.0.9", + ) + assert RateLimitMiddleware._client_ip(req) == "172.18.0.9" + + +def test_trusted_proxy_count_two_uses_second_to_last_hop(monkeypatch): + monkeypatch.setattr(settings, "trusted_proxy_count", 2) + req = _FakeRequest( + headers={"x-forwarded-for": "1.1.1.1, 203.0.113.5, 10.0.0.2"}, + host="10.0.0.1", + ) + assert RateLimitMiddleware._client_ip(req) == "203.0.113.5" + + +def test_trusted_proxy_count_zero_ignores_all_proxy_headers(monkeypatch): + monkeypatch.setattr(settings, "trusted_proxy_count", 0) + req = _FakeRequest( + headers={ + "x-forwarded-for": "203.0.113.5, 10.0.0.2", + "x-real-ip": "203.0.113.99", + }, + host="172.18.0.9", + ) + assert RateLimitMiddleware._client_ip(req) == "172.18.0.9" + + +class TestEffectiveLimit: + """_effective_limit: dividing a configured limit across worker processes.""" + + def test_single_worker_keeps_configured_limit(self, monkeypatch): + monkeypatch.setattr(settings, "web_concurrency", 1) + assert RateLimitMiddleware._effective_limit(5) == 5 + + def test_divides_and_rounds_up(self, monkeypatch): + monkeypatch.setattr(settings, "web_concurrency", 4) + assert RateLimitMiddleware._effective_limit(5) == 2 # ceil(5/4) + + def test_never_below_one(self, monkeypatch): + monkeypatch.setattr(settings, "web_concurrency", 100) + assert RateLimitMiddleware._effective_limit(5) == 1 + + def test_non_positive_concurrency_treated_as_one(self, monkeypatch): + monkeypatch.setattr(settings, "web_concurrency", 0) + assert RateLimitMiddleware._effective_limit(5) == 5 diff --git a/src/backend/tests/test_security.py b/src/backend/tests/test_security.py index b669706..ca0414c 100644 --- a/src/backend/tests/test_security.py +++ b/src/backend/tests/test_security.py @@ -101,6 +101,29 @@ class TestRateLimiting: retry_val = int(resp.headers["retry-after"]) assert retry_val >= 1 + async def test_login_rate_limit_survives_rotated_x_forwarded_for( + self, client: AsyncClient + ): + """A client rotating the spoofable (first) hop of XFF cannot dodge the + login rate limit: with one trusted proxy (the default), the real + client IP is the last hop, which stays constant.""" + for i in range(5): + resp = await client.post( + "/api/auth/login", + json={"username": f"rotated{i}", "password": "pass"}, + headers={ + "X-Forwarded-For": f"{i}.{i}.{i}.{i}, 198.51.100.42", + }, + ) + assert resp.status_code == 401 + + resp = await client.post( + "/api/auth/login", + json={"username": "rotated-overflow", "password": "pass"}, + headers={"X-Forwarded-For": "9.9.9.9, 198.51.100.42"}, + ) + assert resp.status_code == 429 + class TestPathTraversal: """Path traversal protection in file endpoints."""