diff --git a/client/Dockerfile b/client/Dockerfile index c1ffeb2..7d93e3f 100644 --- a/client/Dockerfile +++ b/client/Dockerfile @@ -22,4 +22,12 @@ RUN pybabel compile -d translations EXPOSE 5000 -CMD ["gunicorn", "--workers", "2", "--bind", "0.0.0.0:5000", "app:create_app()"] +CMD ["gunicorn", \ + "--workers", "5", \ + "--threads", "4", \ + "--worker-class", "gthread", \ + "--timeout", "60", \ + "--bind", "0.0.0.0:5000", \ + "--access-logfile", "-", \ + "--forwarded-allow-ips", "*", \ + "app:create_app()"] diff --git a/client/app.py b/client/app.py index 6226e1a..f213f62 100644 --- a/client/app.py +++ b/client/app.py @@ -6,6 +6,7 @@ from flask import Flask, redirect, url_for, session, request from flask_babel import Babel from flask_wtf.csrf import CSRFProtect from markupsafe import Markup +from werkzeug.middleware.proxy_fix import ProxyFix from config import Config @@ -26,6 +27,11 @@ def create_app() -> Flask: app = Flask(__name__) app.config.from_object(Config) + # Trust one reverse-proxy hop (Nginx in dev, Traefik in prod) so that + # request.remote_addr returns the real tablet IP rather than the proxy IP. + # The APIClient forwards that IP to FastAPI for accurate rate limiting. + app.wsgi_app = ProxyFix(app.wsgi_app, x_for=1, x_proto=1, x_host=1) + # Initialize CSRF protection csrf = CSRFProtect(app) diff --git a/client/services/api_client.py b/client/services/api_client.py index 4d3604a..6f89220 100644 --- a/client/services/api_client.py +++ b/client/services/api_client.py @@ -2,7 +2,7 @@ from typing import Any import requests -from flask import session +from flask import has_request_context, request, session from config import Config @@ -14,13 +14,29 @@ class APIClient: self.base_url = Config.API_SERVER_URL.rstrip("/") self.timeout = 30 + @staticmethod + def _real_client_ip() -> str | None: + """Best-effort tablet IP for downstream rate limiting. + + Behind Nginx + ProxyFix, ``request.remote_addr`` already resolves to + the tablet IP. Outside a request context (background tasks, tests), + return None so we don't forge a bogus header. + """ + if not has_request_context(): + return None + return request.remote_addr + @property def _headers(self) -> dict[str, str]: - """Build request headers with API key from session.""" + """Build request headers with API key + real client IP from session.""" headers = {"Content-Type": "application/json"} api_key = session.get("api_key") if api_key: headers["X-API-Key"] = api_key + client_ip = self._real_client_ip() + if client_ip: + headers["X-Forwarded-For"] = client_ip + headers["X-Real-IP"] = client_ip return headers def _handle_response(self, response: requests.Response) -> dict[str, Any]: @@ -76,6 +92,10 @@ class APIClient: try: if files: headers = {"X-API-Key": session.get("api_key", "")} + client_ip = self._real_client_ip() + if client_ip: + headers["X-Forwarded-For"] = client_ip + headers["X-Real-IP"] = client_ip response = requests.post( f"{self.base_url}{endpoint}", headers=headers, diff --git a/client/tests/test_api_client_proxy.py b/client/tests/test_api_client_proxy.py new file mode 100644 index 0000000..1393b7f --- /dev/null +++ b/client/tests/test_api_client_proxy.py @@ -0,0 +1,70 @@ +"""Tests that APIClient forwards the real tablet IP via X-Forwarded-For.""" +from unittest.mock import patch + +from services.api_client import api_client + + +def _last_call_headers(mock): + """Return the headers kwarg from the most recent requests.* call.""" + return mock.call_args.kwargs["headers"] + + +def test_get_forwards_real_client_ip(flask_app): + with flask_app.test_request_context( + "/", environ_base={"REMOTE_ADDR": "203.0.113.10"} + ): + with patch("services.api_client.requests.get") as mock_get: + mock_get.return_value.ok = True + mock_get.return_value.status_code = 200 + mock_get.return_value.json.return_value = {} + api_client.get("/api/health") + + headers = _last_call_headers(mock_get) + assert headers["X-Forwarded-For"] == "203.0.113.10" + assert headers["X-Real-IP"] == "203.0.113.10" + + +def test_post_json_forwards_real_client_ip(flask_app): + with flask_app.test_request_context( + "/", environ_base={"REMOTE_ADDR": "198.51.100.42"} + ): + with patch("services.api_client.requests.post") as mock_post: + mock_post.return_value.ok = True + mock_post.return_value.status_code = 200 + mock_post.return_value.json.return_value = {} + api_client.post("/api/foo", data={"x": 1}) + + headers = _last_call_headers(mock_post) + assert headers["X-Forwarded-For"] == "198.51.100.42" + + +def test_post_with_files_forwards_real_client_ip(flask_app): + with flask_app.test_request_context( + "/", environ_base={"REMOTE_ADDR": "198.51.100.77"} + ): + with patch("services.api_client.requests.post") as mock_post: + mock_post.return_value.ok = True + mock_post.return_value.status_code = 200 + mock_post.return_value.json.return_value = {} + api_client.post( + "/api/files/upload", data={}, files={"file": ("x.txt", b"data")} + ) + + headers = _last_call_headers(mock_post) + assert headers["X-Forwarded-For"] == "198.51.100.77" + assert headers["X-Real-IP"] == "198.51.100.77" + + +def test_real_client_ip_returns_none_outside_request_context(flask_app): + """Background workers without a Flask request must NOT inject a fake IP.""" + from services.api_client import APIClient + + with flask_app.app_context(): + assert APIClient._real_client_ip() is None + + +def test_proxy_fix_is_installed(flask_app): + """ProxyFix must wrap wsgi_app so REMOTE_ADDR is rewritten in production.""" + from werkzeug.middleware.proxy_fix import ProxyFix + + assert isinstance(flask_app.wsgi_app, ProxyFix) diff --git a/server/Dockerfile b/server/Dockerfile index ba1368f..ba699d0 100644 --- a/server/Dockerfile +++ b/server/Dockerfile @@ -23,4 +23,4 @@ RUN mkdir -p uploads/images uploads/pdfs uploads/logos uploads/reports EXPOSE 8000 # Entry point: Alembic upgrade + Uvicorn -CMD ["sh", "-c", "alembic -c migrations/alembic.ini upgrade head && uvicorn main:app --host 0.0.0.0 --port 8000 --workers 2"] +CMD ["sh", "-c", "alembic -c migrations/alembic.ini upgrade head && uvicorn main:app --host 0.0.0.0 --port 8000 --workers 4 --proxy-headers --forwarded-allow-ips='*'"] diff --git a/server/config.py b/server/config.py index 697e5c3..2e05322 100644 --- a/server/config.py +++ b/server/config.py @@ -23,9 +23,9 @@ class Settings(BaseSettings): upload_dir: str = "uploads" max_upload_size_mb: int = 50 - # Rate Limiting (requests per minute) + # Rate Limiting (requests per minute, per real client IP) rate_limit_login: int = 5 - rate_limit_general: int = 100 + rate_limit_general: int = 300 # SSL (Production) ssl_certfile: str | None = None diff --git a/server/middleware/rate_limit.py b/server/middleware/rate_limit.py index 7f76f24..0890d35 100644 --- a/server/middleware/rate_limit.py +++ b/server/middleware/rate_limit.py @@ -32,6 +32,25 @@ class RateLimitMiddleware(BaseHTTPMiddleware): self._general_requests: dict[str, list[float]] = defaultdict(list) self._request_count = 0 # Counter for triggering eviction + @staticmethod + def _client_ip(request: Request) -> str: + """Resolve the originating client IP, honoring proxy headers. + + 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. + """ + 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() + return request.client.host if request.client else "unknown" + def _clean_window(self, timestamps: list[float], now: float) -> list[float]: """Remove timestamps outside the current sliding window.""" cutoff = now - self.WINDOW_SECONDS @@ -68,7 +87,7 @@ class RateLimitMiddleware(BaseHTTPMiddleware): return True, 0 async def dispatch(self, request: Request, call_next: Callable) -> Response: - client_ip = request.client.host if request.client else "unknown" + client_ip = self._client_ip(request) now = time.time() path = request.url.path diff --git a/server/tests/test_rate_limit_proxy.py b/server/tests/test_rate_limit_proxy.py new file mode 100644 index 0000000..18ad5e7 --- /dev/null +++ b/server/tests/test_rate_limit_proxy.py @@ -0,0 +1,65 @@ +"""Tests for RateLimitMiddleware honoring proxy headers (X-Forwarded-For).""" +from middleware.rate_limit import RateLimitMiddleware + + +class _FakeRequest: + """Minimal stand-in for starlette.requests.Request.""" + + def __init__(self, headers: dict[str, str], host: str | None = "10.0.0.1"): + self.headers = headers + + class _Client: + pass + + if host is None: + self.client = None + else: + self.client = _Client() + self.client.host = host + + +def test_client_ip_uses_x_forwarded_for_first_hop(): + 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" + + +def test_client_ip_strips_whitespace(): + req = _FakeRequest(headers={"x-forwarded-for": " 198.51.100.7 "}) + assert RateLimitMiddleware._client_ip(req) == "198.51.100.7" + + +def test_client_ip_falls_back_to_x_real_ip(): + req = _FakeRequest(headers={"x-real-ip": "203.0.113.99"}, host="10.0.0.1") + assert RateLimitMiddleware._client_ip(req) == "203.0.113.99" + + +def test_client_ip_falls_back_to_request_client_host(): + req = _FakeRequest(headers={}, host="172.18.0.5") + assert RateLimitMiddleware._client_ip(req) == "172.18.0.5" + + +def test_client_ip_returns_unknown_without_client(): + req = _FakeRequest(headers={}, host=None) + assert RateLimitMiddleware._client_ip(req) == "unknown" + + +def test_x_forwarded_for_overrides_x_real_ip(): + req = _FakeRequest( + headers={ + "x-forwarded-for": "203.0.113.5", + "x-real-ip": "10.0.0.2", + }, + host="10.0.0.1", + ) + assert RateLimitMiddleware._client_ip(req) == "203.0.113.5" + + +def test_empty_x_forwarded_for_falls_through(): + req = _FakeRequest( + headers={"x-forwarded-for": "", "x-real-ip": "203.0.113.42"}, + host="10.0.0.1", + ) + assert RateLimitMiddleware._client_ip(req) == "203.0.113.42"