diff --git a/src/bpp/newsfragments/+rollbar-scrub-nie-zjada-kodu.bugfix.rst b/src/bpp/newsfragments/+rollbar-scrub-nie-zjada-kodu.bugfix.rst new file mode 100644 index 000000000..a428d2263 --- /dev/null +++ b/src/bpp/newsfragments/+rollbar-scrub-nie-zjada-kodu.bugfix.rst @@ -0,0 +1,6 @@ +Zgłoszenia błędów wysyłane do monitoringu znów zawierają linie kodu w miejscu +awarii. Reguła maskowania danych wrażliwych obejmowała pole o nazwie ``code``, +a pod tą samą nazwą biblioteka monitoringu przechowuje linię kodu źródłowego +każdej ramki śladu wywołań — od 11 lipca 2026 skutkowało to zamazywaniem +całych śladów wywołań i utrudniało diagnostykę. Kody autoryzacyjne OAuth są +nadal maskowane, również w adresach URL i zmiennych lokalnych. diff --git a/src/bpp/rollbar_config.py b/src/bpp/rollbar_config.py index d4d3de941..aa0b4d573 100644 --- a/src/bpp/rollbar_config.py +++ b/src/bpp/rollbar_config.py @@ -1,5 +1,7 @@ import rollbar from django.conf import settings +from rollbar.lib.transforms.scrub import ScrubTransform +from rollbar.lib.transforms.scruburl import ScrubUrlTransform # Wyjątki, które Rollbar domyślnie rozbija na wiele itemów, bo zmienna treść # w tracebacku (np. wyrenderowany raport z nazwiskiem autora w zmiennej @@ -9,6 +11,61 @@ "DocxConversionError", } +#: Domyślne `url_fields` pyrollbara — klucze, pod którymi spodziewa się URL-i. +#: `settings.ROLLBAR` ich nie nadpisuje, a `ScrubUrlTransform.in_scrub_fields` +#: i tak zwraca True dla każdego stringa; podajemy je dla zgodności. +URL_FIELDS = ("url", "link", "href") + + +class ScrubKoduAutoryzacyjnego(ScrubTransform): + """Zamazuje pole ``code`` WSZĘDZIE POZA dwiema ścieżkami z linią kodu. + + Problem: ``code`` to jednocześnie nazwa parametru OAuth (kod autoryzacyjny + — w POST do ``/o/token/`` oraz w GET do ``/orcid/callback/``, do + zamazania) i nazwa pola, w którym pyrollbar trzyma LINIĘ KODU ŹRÓDŁOWEGO + każdej ramki tracebacku (do zachowania). + + ``ScrubRedactTransform`` dopasowuje ścieżkę klucza po SUFIKSIE, więc + ``"code"`` na liście ``scrub_fields`` trafiał w oba naraz i zamazywał całe + tracebacki (patrz komentarz przy ``ROLLBAR_SCRUB_FIELDS``). + + Wyjątek jest zdefiniowany jako DOKŁADNA lista dwóch ścieżek, a nie jako + „ścieżka zawiera ``frames``". Luźniejszy warunek dawał się obejść — + ``request.POST.frames.code`` czy ``custom.frames[0].code`` przechodziłyby + nietknięte — a co gorsza pomijał ``frames[N].locals.code`` + i ``frames[N].kwargs.code``, czyli DOKŁADNIE ten sekret, dla którego + ``"code"`` w ogóle trafiło na listę: w django-oauth-toolkit ``code`` jest + parametrem kilkunastu metod walidatora (``validate_code``, + ``invalidate_authorization_code``, ``save_authorization_code``…), więc + wyjątek w którejkolwiek z nich wystawiłby aktywny kod w zmiennych + lokalnych ramki. + """ + + @staticmethod + def _czy_linia_kodu_ramki(key): + """Czy to JEDNA z dwóch ścieżek, pod którymi pyrollbar trzyma kod. + + Kształty zrzucone z działającego łańcucha transformów: + ``("body", "trace", "frames", , "code")`` oraz + ``("body", "trace_chain", , "frames", , "code")``. + """ + if len(key) == 5 and key[:3] == ("body", "trace", "frames"): + return isinstance(key[3], int) + if len(key) == 6 and key[:2] == ("body", "trace_chain"): + return ( + isinstance(key[2], int) + and key[3] == "frames" + and isinstance(key[4], int) + ) + return False + + def in_scrub_fields(self, key): + # Case-insensitive jak `build_key_matcher` pyrollbara — bez tego + # `POST.Code` / `POST.CODE` przestałyby być zamazywane. + if not key or str(key[-1]).lower() != "code": + return False + return not self._czy_linia_kodu_ramki(tuple(key)) + def add_hostname_to_payload(payload, **kw): """ @@ -54,6 +111,38 @@ def collapse_noisy_fingerprints(payload, **kw): _initialized = False +def ustawienia_rollbara(): + """``settings.ROLLBAR`` wzbogacone o nasze własne transformy payloadu. + + Transform dokładamy TUTAJ, a nie w ``settings.ROLLBAR``, żeby nie + importować ``bpp.*`` na etapie ładowania ustawień — ``configure_rollbar`` + i tak biegnie z ``AppConfig.ready()`` (patrz ``bpp/apps.py``), czyli PRZED + inicjalizacją middleware'u django-rollbar. To istotne: ``rollbar.init`` + buduje łańcuch transformów tylko przy PIERWSZYM wywołaniu, więc gdyby + ubiegł nas middleware, nasz transform nigdy by nie wszedł. + """ + ustawienia = dict(settings.ROLLBAR) + pola = list(ustawienia.get("scrub_fields") or []) + + wlasne = list(ustawienia.get("custom_transforms") or []) + wlasne.append(ScrubKoduAutoryzacyjnego(redact_char="*")) + # Wbudowany ScrubUrlTransform pyrollbara czyści parametry w URL-ach na + # podstawie `scrub_fields` (`params_to_scrub=SETTINGS['scrub_fields']`), + # więc zdjęcie stamtąd "code" odebrałoby mu wiedzę o TYM parametrze — + # a `?code=` w URL-u to realny wektor: /orcid/callback/ dostaje kod + # autoryzacyjny w query stringu, a pyrollbar zapisuje pełny + # `request.build_absolute_uri()` (także w nagłówku Referer i w zmiennych + # lokalnych). Dokładamy więc własny ScrubUrlTransform, który zna "code". + wlasne.append( + ScrubUrlTransform( + suffixes=[(pole,) for pole in URL_FIELDS], + params_to_scrub=pola + ["code"], + ) + ) + ustawienia["custom_transforms"] = wlasne + return ustawienia + + def configure_rollbar(): """ Initialize Rollbar and register the hostname payload handler. @@ -63,7 +152,7 @@ def configure_rollbar(): if _initialized: return - rollbar.init(**settings.ROLLBAR) + rollbar.init(**ustawienia_rollbara()) rollbar.events.add_payload_handler(add_hostname_to_payload) # PO hostname: collapse_noisy_fingerprints czyta hosta z custom. rollbar.events.add_payload_handler(collapse_noisy_fingerprints) diff --git a/src/bpp/tests/test_rollbar_config.py b/src/bpp/tests/test_rollbar_config.py index ad8c3c5a6..6127f8008 100644 --- a/src/bpp/tests/test_rollbar_config.py +++ b/src/bpp/tests/test_rollbar_config.py @@ -1,5 +1,8 @@ """Testy payload-handlerów Rollbara (src/bpp/rollbar_config.py).""" +import pytest +from rollbar.lib.transforms.scruburl import ScrubUrlTransform + from bpp.rollbar_config import collapse_noisy_fingerprints @@ -91,3 +94,195 @@ def test_payload_bez_body_nie_wybucha(): result = collapse_noisy_fingerprints(payload) assert "fingerprint" not in result["data"] + + +# --- Scrub pola `code`: sekret OAuth TAK, linia kodu w tracebacku NIE ------- +# +# UWAGA METODOLOGICZNA: te testy jadą PRAWDZIWYM łańcuchem transformów +# pyrollbara (`rollbar.init` + `rollbar._build_payload`), a nie jego +# rekonstrukcją. Wcześniejsza wersja składała listę transformów ręcznie +# i przez to POMIJAŁA `ScrubUrlTransform` — a właśnie tam siedział najgroźniejszy +# wyciek (`?code=` w URL-u). Testy świeciły na zielono przy dziurawym kodzie. + + +@pytest.fixture +def zbuduj_payload(monkeypatch): + """Zwraca funkcję ``data -> payload`` przepuszczony przez pełny pyrollbar.""" + import rollbar + + from bpp.rollbar_config import ustawienia_rollbara + + monkeypatch.setattr(rollbar, "_initialized", False) + monkeypatch.setattr(rollbar, "send_payload", lambda p, t: None) + + ustawienia = ustawienia_rollbara() + # `access_token` NIE jest tu ustawiany: `settings.ROLLBAR` wnosi go + # z konfiguracji (w testach = None), a wysyłka i tak jest zaślepiona + # przez podmieniony `send_payload`. Wpisanie tu atrapy tokena zapalało + # skaner sekretów w CI — słusznie, bo wzorzec jest nieodróżnialny od + # prawdziwego przecieku. + ustawienia["environment"] = "test" + ustawienia["handler"] = "blocking" + ustawienia["suppress_reinit_warning"] = True + rollbar.init(**ustawienia) + + return lambda data: rollbar._build_payload(data)["data"] + + +SEKRET = "wartosc-ktora-ma-zniknac-z-payloadu" +LINIA_KODU = "autor_str = str(self.autor) if self.autor_id else '???'" + + +def test_linia_kodu_w_tracebacku_nie_jest_zamazywana(zbuduj_payload): + """Regresja: całe tracebacki w Rollbarze miały `code: "****"`. + + ``ROLLBAR_SCRUB_FIELDS`` zawierało ``"code"`` (dla parametru OAuth), a + ``ScrubRedactTransform`` dopasowuje ścieżkę klucza po SUFIKSIE — więc + trafiało też w ``body.trace.frames[*].code``, czyli linie kodu źródłowego. + """ + out = zbuduj_payload( + {"body": {"trace": {"frames": [{"filename": "a.py", "code": LINIA_KODU}]}}} + ) + + assert out["body"]["trace"]["frames"][0]["code"] == LINIA_KODU + + +def test_linia_kodu_w_trace_chain_tez_nie_jest_zamazywana(zbuduj_payload): + """Wyjątki łańcuchowe mają inną ścieżkę klucza — też musi być pokryta.""" + out = zbuduj_payload( + { + "body": { + "trace_chain": [{"frames": [{"filename": "a.py", "code": LINIA_KODU}]}] + } + } + ) + + assert out["body"]["trace_chain"][0]["frames"][0]["code"] == LINIA_KODU + + +@pytest.mark.parametrize( + "opis,data,sciezka", + [ + ( + "POST /o/token/", + {"request": {"POST": {"code": SEKRET}}}, + ("request", "POST", "code"), + ), + ( + "GET /orcid/callback/", + {"request": {"GET": {"code": SEKRET}}}, + ("request", "GET", "code"), + ), + ( + "inna wielkosc liter", + {"request": {"POST": {"Code": SEKRET}}}, + ("request", "POST", "Code"), + ), + ( + "kolizja klucza `frames` poza tracebackiem", + {"request": {"POST": {"frames": {"code": SEKRET}}}}, + ("request", "POST", "frames", "code"), + ), + ( + "zmienna lokalna ramki (django-oauth-toolkit: validate_code)", + {"body": {"trace": {"frames": [{"locals": {"code": SEKRET}}]}}}, + ("body", "trace", "frames", 0, "locals", "code"), + ), + ( + "argument nazwany ramki", + {"body": {"trace": {"frames": [{"kwargs": {"code": SEKRET}}]}}}, + ("body", "trace", "frames", 0, "kwargs", "code"), + ), + ], +) +def test_kod_autoryzacyjny_jest_zamazywany(zbuduj_payload, opis, data, sciezka): + """Druga strona kontraktu — bez niej poprawka byłaby regresją bezpieczeństwa.""" + out = zbuduj_payload(data) + + biezacy = out + for element in sciezka: + biezacy = biezacy[element] + + assert SEKRET not in str(biezacy), f"WYCIEK sekretu: {opis}" + + +@pytest.mark.parametrize( + "opis,data,sciezka", + [ + ( + "request.url", + { + "request": { + "url": f"https://bpp.example.pl/orcid/callback/?code={SEKRET}" + } + }, + ("request", "url"), + ), + ( + "naglowek Referer", + { + "request": { + "headers": {"Referer": f"https://bpp.example.pl/cb?code={SEKRET}"} + } + }, + ("request", "headers", "Referer"), + ), + ( + "URL w zmiennej lokalnej ramki", + { + "body": { + "trace": { + "frames": [{"locals": {"url": f"https://x/cb?code={SEKRET}"}}] + } + } + }, + ("body", "trace", "frames", 0, "locals", "url"), + ), + ], +) +def test_kod_autoryzacyjny_w_URL_tez_jest_zamazywany( + zbuduj_payload, opis, data, sciezka +): + """Najgroźniejszy wyciek, jaki wyszedł w self-review. + + Wbudowany ``ScrubUrlTransform`` czyści parametry URL na podstawie + ``scrub_fields`` — zdjęcie stamtąd ``"code"`` rozbroiłoby go dla tego + parametru. ``/orcid/callback/`` dostaje kod autoryzacyjny w query stringu, + a pyrollbar zapisuje pełny ``request.build_absolute_uri()``. + """ + out = zbuduj_payload(data) + + biezacy = out + for element in sciezka: + biezacy = biezacy[element] + + assert SEKRET not in str(biezacy), f"WYCIEK sekretu w URL: {opis}" + + +def test_pozostale_pola_wrazliwe_nadal_zamazywane_takze_w_ramkach(zbuduj_payload): + """`password` w zmiennych lokalnych ramki MUSI zniknąć — inaczej niż `code`.""" + out = zbuduj_payload( + {"body": {"trace": {"frames": [{"locals": {"password": "tajne123"}}]}}} + ) + + assert "tajne123" not in str(out["body"]["trace"]["frames"][0]["locals"]) + + +def test_configure_rollbar_przekazuje_nasze_transformy_do_inicjalizacji(mocker): + """Sam transform nic nie da, jeśli nie trafi do ``rollbar.init``. + + Kolejność ma znaczenie: ``rollbar.init`` buduje łańcuch transformów tylko + przy PIERWSZYM wywołaniu. ``configure_rollbar`` biegnie z + ``AppConfig.ready()``, czyli przed middlewarem django-rollbar. + """ + import bpp.rollbar_config as rc + + init = mocker.patch("bpp.rollbar_config.rollbar.init") + mocker.patch("bpp.rollbar_config.rollbar.events.add_payload_handler") + mocker.patch.object(rc, "_initialized", False) + + rc.configure_rollbar() + + transformy = init.call_args.kwargs["custom_transforms"] + assert any(isinstance(t, rc.ScrubKoduAutoryzacyjnego) for t in transformy) + assert any(isinstance(t, ScrubUrlTransform) for t in transformy) diff --git a/src/django_bpp/settings/base.py b/src/django_bpp/settings/base.py index 67d4589ca..055653611 100644 --- a/src/django_bpp/settings/base.py +++ b/src/django_bpp/settings/base.py @@ -1713,12 +1713,25 @@ def iter_namespace(ns_pkg): # ROLLBAR settings # -# pyrollbar dopasowuje pola do scrubu po DOKŁADNEJ nazwie klucza (case-insensitive, -# NIE po sufiksie) i PODMIENIA swoją domyślną listę, gdy podamy własną. Dlatego -# odtwarzamy tu domyślny zestaw pyrollbara i DOKŁADAMY sekrety OAuth/MCP i PBN -# (uwaga reviewera #3/#5): DOT oznacza jako wrażliwe tylko password/client_secret, -# a Rollbar bez tej listy wysłałby aktywny refresh_token / code / code_verifier / -# token przy nieoczekiwanym 500 na /o/token/ czy /o/revoke_token/. +# pyrollbar dopasowuje pola po nazwie klucza (case-insensitive) NA DOWOLNYM +# POZIOMIE ZAGNIEŻDŻENIA: matcher jest budowany jako `type="suffix"` nad +# ŚCIEŻKĄ klucza, więc wpis trafia w każdy klucz o tej nazwie, gdziekolwiek +# w payloadzie. (Poprzedni komentarz twierdził tu „NIE po sufiksie" — to była +# nieprawda i to ona doprowadziła do zamazywania CAŁYCH tracebacków przez +# niewinnie wyglądający wpis "code"; patrz bpp.rollbar_config.) +# +# Podana lista PODMIENIA domyślną listę pyrollbara, więc odtwarzamy tu jego +# domyślny zestaw i DOKŁADAMY sekrety OAuth/MCP i PBN: DOT oznacza jako +# wrażliwe tylko password/client_secret, a Rollbar bez tej listy wysłałby +# aktywny refresh_token / code / code_verifier / token przy nieoczekiwanym 500 +# na /o/token/ czy /o/revoke_token/. +# +# UWAGA: ta lista zasila TAKŻE `ScrubUrlTransform` +# (`params_to_scrub=SETTINGS["scrub_fields"]`), czyszczący parametry w query +# stringach. Zdjęcie czegoś stąd rozbraja — dla tego pola — również czyszczenie +# URL-i, i to WSZĘDZIE (ten transform skanuje każdy string, nie tylko klucz +# "url"). Dlatego usunięcie "code" wymagało dołożenia własnego +# `ScrubUrlTransform` w bpp.rollbar_config. ROLLBAR_SCRUB_FIELDS = [ # domyślne pyrollbara (zachowujemy — nasza lista je nadpisuje): "pw", @@ -1738,7 +1751,11 @@ def iter_namespace(ns_pkg): "token", "refresh_token", "refreshToken", - "code", + # UWAGA: "code" celowo NIE jest tutaj. pyrollbar trzyma pod tą nazwą także + # LINIĘ KODU ŹRÓDŁOWEGO każdej ramki tracebacku, a dopasowanie idzie po + # sufiksie ścieżki klucza — więc wpis na tej liście zamazywał wszystkie + # tracebacki na "****". Kod autoryzacyjny OAuth zamazuje zamiast tego + # bpp.rollbar_config.ScrubKoduAutoryzacyjnego, który pomija ramki stosu. "code_verifier", "codeVerifier", "client_secret", diff --git a/src/oauth_mcp/tests/test_rollbar_scrub.py b/src/oauth_mcp/tests/test_rollbar_scrub.py index 77b6bcfd5..d7f2c6b02 100644 --- a/src/oauth_mcp/tests/test_rollbar_scrub.py +++ b/src/oauth_mcp/tests/test_rollbar_scrub.py @@ -4,24 +4,77 @@ ``password`` i ``client_secret``; pyrollbar domyślnie NIE scrubuje ``refresh_token``, ``code``, ``code_verifier`` ani ``token``. Nieoczekiwany wyjątek 500 podczas wymiany/odświeżenia/rewokacji wysłałby aktywny sekret do -Rollbara. pyrollbar dopasowuje po DOKŁADNEJ nazwie klucza (nie po sufiksie) i -PODMIENIA domyślną listę, gdy podamy własną — więc lista musi zawierać zarówno -domyślne pola, jak i te specyficzne dla OAuth. +Rollbara. + +Test sprawdza GWARANCJĘ (sekret nie wychodzi w payloadzie), a nie sposób jej +realizacji. Wcześniejsza wersja asertowała obecność nazwy pola na liście +``scrub_fields`` — czyli implementację. Gdy ``"code"`` musiało z tej listy +zniknąć (bo zamazywało też linie kodu w tracebackach — patrz +``bpp.rollbar_config.ScrubKoduAutoryzacyjnego``), test padał, choć sekret nadal +był maskowany. Asercja na gwarancję przeżyje kolejną zmianę mechanizmu. """ -from django.conf import settings - - -def test_rollbar_scrubuje_sekrety_oauth(): - scrub = {f.lower() for f in settings.ROLLBAR.get("scrub_fields", [])} - for field in ( - "refresh_token", - "code", - "code_verifier", - "token", - "access_token", - "client_secret", - "authorization", - "password", - ): - assert field in scrub, f"Rollbar nie scrubuje pola {field!r}" +import pytest + +ATRAPA = "wartosc-do-zamaskowania-w-tescie" + +POLA_WRAZLIWE = ( + "refresh_token", + "code", + "code_verifier", + "token", + "access_token", + "client_secret", + "authorization", + "password", +) + + +@pytest.fixture +def zbuduj_payload(monkeypatch): + """Zwraca funkcję ``data -> payload`` przepuszczony przez pełny pyrollbar.""" + import rollbar + + from bpp.rollbar_config import ustawienia_rollbara + + monkeypatch.setattr(rollbar, "_initialized", False) + monkeypatch.setattr(rollbar, "send_payload", lambda p, t: None) + + ustawienia = ustawienia_rollbara() + # `access_token` NIE jest tu ustawiany: `settings.ROLLBAR` wnosi go + # z konfiguracji (w testach = None), a wysyłka i tak jest zaślepiona + # przez podmieniony `send_payload`. Wpisanie tu atrapy tokena zapalało + # skaner sekretów w CI — słusznie, bo wzorzec jest nieodróżnialny od + # prawdziwego przecieku. + ustawienia["environment"] = "test" + ustawienia["handler"] = "blocking" + ustawienia["suppress_reinit_warning"] = True + rollbar.init(**ustawienia) + + return lambda data: rollbar._build_payload(data)["data"] + + +@pytest.mark.parametrize("pole", POLA_WRAZLIWE) +def test_rollbar_maskuje_sekrety_oauth_w_parametrach_zadania(zbuduj_payload, pole): + """Sekret w POST (wymiana kodu na token) nie może opuścić serwera.""" + out = zbuduj_payload({"request": {"POST": {pole: ATRAPA}}}) + + assert ATRAPA not in str(out["request"]["POST"][pole]), ( + f"Rollbar nie maskuje pola {pole!r} w request.POST" + ) + + +@pytest.mark.parametrize("pole", POLA_WRAZLIWE) +def test_rollbar_maskuje_sekrety_oauth_w_zmiennych_lokalnych(zbuduj_payload, pole): + """django-oauth-toolkit przekazuje te sekrety jako argumenty walidatora. + + Wyjątek w ``validate_code`` / ``save_authorization_code`` wystawiłby je + w ``frames[N].locals`` — to inna ścieżka klucza niż parametry żądania, + a właśnie ją przeoczyła pierwsza wersja ``ScrubKoduAutoryzacyjnego``. + """ + out = zbuduj_payload({"body": {"trace": {"frames": [{"locals": {pole: ATRAPA}}]}}}) + + lokalne = out["body"]["trace"]["frames"][0]["locals"] + assert ATRAPA not in str(lokalne[pole]), ( + f"Rollbar nie maskuje pola {pole!r} w zmiennych lokalnych ramki" + )