From 23416cb1f9ccf2c88caf01295e04ad18f227e782 Mon Sep 17 00:00:00 2001 From: migatu Date: Wed, 22 Jul 2026 23:26:57 +0200 Subject: [PATCH] fix(prezentacja): limit zadan po adresie klienta, nie proxy (PRE-16) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Po wlaczeniu TLS aplikacja stanie za Ingressem, a wtedy `request.client.host` to adres POD-a Traefika — jednakowy dla wszystkich. Limiter wrzucalby caly ruch do jednego wiadra 120/min i pierwsza osoba, ktora go wyklika, odcielaby pozostalych. Cicha regresja, ktora ujawnilaby sie dopiero na produkcji. Nowe `client_ip()` czyta adres z naglowka, ale WYLACZNIE przy TRUST_PROXY — bo inaczej wystarczyloby dopisywac wlasny X-Forwarded-For, zeby przy kazdym zadaniu wygladac na kogos innego i ominac limit calkowicie. Z tego samego powodu bierzemy OSTATNI wpis listy: to jedyny, ktory dopisal nasz proxy; wczesniejsze mogl podstawic klient, wiec nie znacza nic. Szesc testow, w tym dwa istotne: - podszycie sie pod X-Forwarded-For NIE resetuje wiadra przy wylaczonym TRUST_PROXY (inaczej baze dalo by sie pompowac bez ograniczen), - za proxy dwa rozne adresy dostaja osobne wiadra i nie odcinaja sie nawzajem. Oba sprawdzone celowym zepsuciem implementacji (zawsze ufaj naglowkowi + bierz pierwszy wpis) — testy wtedy czerwienieja. 23 passed. Co-Authored-By: Claude Opus 4.8 --- services/presentation/app/security.py | 37 ++++++++++- services/presentation/tests/test_security.py | 68 ++++++++++++++++++++ 2 files changed, 102 insertions(+), 3 deletions(-) diff --git a/services/presentation/app/security.py b/services/presentation/app/security.py index de8577a..0eddb25 100644 --- a/services/presentation/app/security.py +++ b/services/presentation/app/security.py @@ -7,7 +7,9 @@ jakiegokolwiek modelu językowego. Ten moduł zamyka tę drogę. Dwa mechanizmy: * **HTTP Basic** — wejście do aplikacji; włącza się, gdy ustawiono APP_PASSWORD. - * **limit żądań** — hamuje masowe odpytywanie (eksfiltrację przez pętlę zapytań). + * **limit żądań** — hamuje masowe odpytywanie (eksfiltrację przez pętlę zapytań); + rozliczany per adres klienta, a za odwrotnym proxy — po TRUST_PROXY=true — + per adres z nagłówka, nie per adres proxy (patrz `client_ip`). Świadomie NIE logujemy treści żądań ani promptów — logi to kolejny nośnik wycieku. @@ -46,6 +48,11 @@ def app_password() -> str: def rate_limit_per_min() -> int: return int(os.getenv("RATE_LIMIT_PER_MIN", "120")) + +def trust_proxy() -> bool: + return os.getenv("TRUST_PROXY", "").strip().lower() in {"1", "true", "yes", "on"} + + PUBLIC_PATHS = frozenset({"/health"}) PUBLIC_PREFIXES = ("/static/",) @@ -74,6 +81,31 @@ def _authorized(header: str | None) -> bool: return ok_user and ok_pass +def client_ip(request: Request) -> str: + """Adres, po którym rozliczamy limit żądań. + + Za odwrotnym proxy (u nas: Ingress/Traefik po włączeniu TLS — PRE-16) + `request.client.host` to adres POD-a proxy, jednakowy dla wszystkich. Bez + poprawki cały ruch trafiałby do jednego wiadra i pierwsza osoba, która + wyklika limit, odcięłaby pozostałe. + + Nagłówkom wierzymy WYŁĄCZNIE przy TRUST_PROXY — bo inaczej wystarczyłoby + dopisać własny `X-Forwarded-For`, żeby przy każdym żądaniu wyglądać na kogoś + innego i ominąć limit całkowicie. Z tego samego powodu bierzemy OSTATNI wpis + listy: to jedyny, który dopisał nasz proxy. Wcześniejsze mógł podstawić + klient, więc nie znaczą nic. + """ + peer = request.client.host if request.client else "?" + if not trust_proxy(): + return peer + forwarded = request.headers.get("x-forwarded-for", "") + if forwarded: + last = forwarded.rsplit(",", 1)[-1].strip() + if last: + return last + return request.headers.get("x-real-ip", "").strip() or peer + + def _rate_limited(client: str) -> bool: cap = rate_limit_per_min() if cap <= 0: @@ -105,8 +137,7 @@ def install(app) -> None: if _is_public(request.url.path): return await call_next(request) - client = request.client.host if request.client else "?" - if _rate_limited(client): + if _rate_limited(client_ip(request)): return JSONResponse( {"detail": "Zbyt wiele żądań — spróbuj za chwilę."}, status_code=429, headers={"Retry-After": "60"}, diff --git a/services/presentation/tests/test_security.py b/services/presentation/tests/test_security.py index 9d2a54e..92ec096 100644 --- a/services/presentation/tests/test_security.py +++ b/services/presentation/tests/test_security.py @@ -131,3 +131,71 @@ def test_rate_limit_disabled_when_zero(monkeypatch): monkeypatch.delenv("APP_PASSWORD", raising=False) client = TestClient(_app()) assert all(client.get("/significators").status_code == 200 for _ in range(30)) + + +# ------------------------------------------ adres klienta za odwrotnym proxy +# +# Po włączeniu TLS (PRE-16) aplikacja stoi za Ingressem, więc bezpośredni peer +# to zawsze POD proxy. Te testy pilnują obu stron kompromisu: żeby limit dalej +# rozróżniał ludzi, a jednocześnie żeby nagłówek nie stał się furtką do jego +# ominięcia. + +class _Req: + """Minimalny zamiennik Request — `client_ip` czyta tylko te dwa pola.""" + + def __init__(self, peer: str | None, **headers: str): + self.client = type("C", (), {"host": peer})() if peer else None + self.headers = {k.replace("_", "-"): v for k, v in headers.items()} + + +def test_client_ip_ignores_headers_without_trust_proxy(monkeypatch): + """Bez TRUST_PROXY nagłówek jest bezwartościowy — każdy może go dopisać.""" + monkeypatch.delenv("TRUST_PROXY", raising=False) + req = _Req("10.42.0.7", x_forwarded_for="1.2.3.4", x_real_ip="5.6.7.8") + assert security.client_ip(req) == "10.42.0.7" + + +def test_client_ip_takes_last_forwarded_entry(monkeypatch): + """Ostatni wpis dopisał NASZ proxy; wcześniejsze mógł podstawić klient.""" + monkeypatch.setenv("TRUST_PROXY", "true") + req = _Req("10.42.0.7", x_forwarded_for="1.1.1.1, 2.2.2.2, 192.168.1.50") + assert security.client_ip(req) == "192.168.1.50" + + +def test_client_ip_falls_back_to_real_ip(monkeypatch): + monkeypatch.setenv("TRUST_PROXY", "true") + req = _Req("10.42.0.7", x_real_ip="192.168.1.50") + assert security.client_ip(req) == "192.168.1.50" + + +def test_client_ip_falls_back_to_peer_when_headers_missing(monkeypatch): + monkeypatch.setenv("TRUST_PROXY", "true") + assert security.client_ip(_Req("10.42.0.7")) == "10.42.0.7" + + +def test_spoofed_forwarded_header_cannot_dodge_the_limit(monkeypatch): + """Sedno sprawy: bez zaufania do proxy podszywanie się NIE resetuje wiadra. + + Gdyby limiter brał pierwszy lepszy `X-Forwarded-For`, wystarczyłoby zmieniać + go co żądanie, żeby pompować bazę bez ograniczeń. + """ + monkeypatch.delenv("TRUST_PROXY", raising=False) + monkeypatch.setenv("RATE_LIMIT_PER_MIN", "3") + monkeypatch.delenv("APP_PASSWORD", raising=False) + client = TestClient(_app()) + codes = [client.get("/significators", headers={"X-Forwarded-For": f"9.9.9.{i}"}).status_code + for i in range(6)] + assert 429 in codes + + +def test_proxied_clients_get_separate_buckets(monkeypatch): + """Za proxy dwie różne osoby nie mogą się nawzajem odcinać.""" + monkeypatch.setenv("TRUST_PROXY", "true") + monkeypatch.setenv("RATE_LIMIT_PER_MIN", "3") + monkeypatch.delenv("APP_PASSWORD", raising=False) + client = TestClient(_app()) + first = [client.get("/significators", headers={"X-Forwarded-For": "192.168.1.50"}).status_code + for _ in range(5)] + second = client.get("/significators", headers={"X-Forwarded-For": "192.168.1.51"}) + assert 429 in first, "limit musi zadziałać dla pierwszego adresu" + assert second.status_code == 200, "drugi adres ma własne wiadro"