fix(rate-limit): non fidarsi del primo hop di X-Forwarded-For (#7)

_client_ip usava il primo elemento di XFF, impostabile dal client. Ora
usa l'elemento a -trusted_proxy_count (nuova impostazione, default 1,
env TRUSTED_PROXY_COUNT), quello aggiunto dal proxy fidato più esterno.
Con trusted_proxy_count=0 XFF e X-Real-IP sono ignorati.

Aggiunta anche web_concurrency (env WEB_CONCURRENCY, default 1): i
bucket del rate limit sono per processo, con --workers N il limite
configurato viene diviso per N (arrotondato per eccesso, minimo 1) in
_effective_limit.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

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(

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"

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

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."""