From 1a65fd08aba5164b0decc81a2bac7e779ba2a1d9 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Tue, 8 Sep 2026 11:13:41 -0400 Subject: [PATCH 1/3] fix(sentry): cap request bodies at 1KB and scrub Postgres DETAIL rows Two ways learner data reaches Sentry, neither gated by send_default_pii. Request bodies. The SDK sets request.data unconditionally at sentry_sdk/integrations/_wsgi_common.py:123; max_request_body_size, checked at :61, is the only control, and left unset it defaults to "medium" -- 10,000-byte bodies. Nobody chose that. Measured over the last 30 days, the write endpoints that actually raise are the sensitive ones: SCIM user PATCH, /api/v1/enrollments/, /api/checkout/result/, /api/checkout/redeem_discount/, /api/profile/details/, and CMS page edits. Set to "small" explicitly, so the choice is findable at the call site instead of in a dependency's defaults. Postgres DETAIL lines. A constraint violation carries a DETAIL line that echoes the whole offending row, and psycopg puts it in str(exc) -- so it ships inside the exception value, which no SDK privacy option covers. Measured on mitxonline MITXONLINE-6PK: a SCIM PATCH IntegrityError reproducing a learner email address three times per event, 46,764 occurrences since 2026-05-27. before_send now truncates at the DETAIL marker across exception values, logentry, and the legacy top-level message, keeping the primary error that names the failure. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RVJKTGk9KHUTN2xfU59ujX --- .../core/observability/sentry.py | 52 +++++++++++++++- tests/test_sentry.py | 62 +++++++++++++++++++ 2 files changed, 113 insertions(+), 1 deletion(-) create mode 100644 tests/test_sentry.py diff --git a/src/ol_analytics_api/core/observability/sentry.py b/src/ol_analytics_api/core/observability/sentry.py index b3ef005..01dac42 100644 --- a/src/ol_analytics_api/core/observability/sentry.py +++ b/src/ol_analytics_api/core/observability/sentry.py @@ -20,12 +20,56 @@ _SHUTDOWN_ERRORS = (SystemExit,) +# Postgres appends a DETAIL line to constraint violations that echoes the whole +# offending row verbatim. psycopg puts it in str(exc), so it ships inside the +# exception value, where no SDK privacy setting reaches it: send_default_pii +# governs user/cookie/header capture and max_request_body_size governs request +# bodies, and neither touches exception text. +_PG_DETAIL_MARKER = "\nDETAIL:" +_PG_DETAIL_REPLACEMENT = "\nDETAIL: [scrubbed]" + + +def _scrub_pg_detail(text: str) -> str: + """Truncate a Postgres error string at its DETAIL line. + + Keeps the primary message, which is what identifies the failure, and drops + the row echo plus any HINT/CONTEXT Postgres appends after it. + """ + index = text.find(_PG_DETAIL_MARKER) + if index == -1: + return text + return text[:index] + _PG_DETAIL_REPLACEMENT + + +def _scrub_pg_details(event: Event) -> Event: + """Apply _scrub_pg_detail everywhere an error string lands on the event. + + Covers exception values, the logentry message/formatted pair, and the legacy + top-level message, so the scrub holds whether the event arrived as an + uncaught exception or via logger.exception. + """ + for entry in (event.get("exception") or {}).get("values") or []: + value = entry.get("value") + if isinstance(value, str): + entry["value"] = _scrub_pg_detail(value) + logentry = event.get("logentry") + if isinstance(logentry, dict): + for key in ("formatted", "message"): + value = logentry.get(key) + if isinstance(value, str): + logentry[key] = _scrub_pg_detail(value) + top_message = event.get("message") + if isinstance(top_message, str): + event["message"] = _scrub_pg_detail(top_message) + return event + + def _before_send(event: Event, hint: Hint) -> Event | None: if "exc_info" in hint: _, exc_value, _ = hint["exc_info"] if isinstance(exc_value, _SHUTDOWN_ERRORS): return None - return event + return _scrub_pg_details(event) def init_sentry( # noqa: PLR0913 -- matches the org's established init_sentry() shape @@ -50,6 +94,12 @@ def init_sentry( # noqa: PLR0913 -- matches the org's established init_sentry() environment=environment, release=version, before_send=_before_send, + # Request bodies are NOT gated on send_default_pii: the SDK sets + # request.data unconditionally (sentry_sdk/integrations/_wsgi_common.py + # :123) and this is the only control (:61). Left unset it defaults to + # "medium", i.e. 10,000-byte bodies. Set explicitly so the choice is + # findable here rather than in a dependency's defaults. + max_request_body_size="small", # This service serves aggregated-only analytics (no individual # learner PII) — default to not sending request/user PII to Sentry. send_default_pii=False, diff --git a/tests/test_sentry.py b/tests/test_sentry.py new file mode 100644 index 0000000..d45ba14 --- /dev/null +++ b/tests/test_sentry.py @@ -0,0 +1,62 @@ +"""Tests for Sentry event scrubbing.""" + +from ol_analytics_api.core.observability.sentry import ( + _before_send, + _scrub_pg_detail, + _scrub_pg_details, +) + +# A real MITXONLINE-6PK exception value, with the learner identifiers replaced. +PG_INTEGRITY_ERROR = ( + 'null value in column "name" of relation "users_user" violates not-null ' + "constraint\n" + "DETAIL: Failing row contains (1863408, , 2026-08-07 18:38:14.503726+00, f, " + "learner@example.invalid, learner@example.invalid, null, f, t, " + "12d7dfc5-6f84-46db-9383-2d7079434173, 1863408, learner@example.invalid, f)." +) +PG_PRIMARY_MESSAGE = ( + 'null value in column "name" of relation "users_user" violates not-null constraint' +) + + +def test_detail_line_is_truncated(): + """The row echo goes; the primary error that names the failure stays.""" + scrubbed = _scrub_pg_detail(PG_INTEGRITY_ERROR) + assert scrubbed.startswith(PG_PRIMARY_MESSAGE) + assert "learner@example.invalid" not in scrubbed + assert "12d7dfc5-6f84-46db-9383-2d7079434173" not in scrubbed + + +def test_message_without_detail_is_unchanged(): + """A message with no DETAIL line passes through untouched.""" + message = "connection to server failed" + assert _scrub_pg_detail(message) == message + + +def test_hint_and_context_after_detail_are_dropped(): + """HINT and CONTEXT follow DETAIL and can quote row data too.""" + text = "boom\nDETAIL: row data\nHINT: try again\nCONTEXT: SQL statement" + scrubbed = _scrub_pg_detail(text) + assert "row data" not in scrubbed + assert "try again" not in scrubbed + assert "SQL statement" not in scrubbed + + +def test_scrubs_exception_values_logentry_and_message(): + """Every place the SDK can put an error string is covered.""" + event = { + "exception": {"values": [{"value": PG_INTEGRITY_ERROR}]}, + "logentry": { + "message": PG_INTEGRITY_ERROR, + "formatted": PG_INTEGRITY_ERROR, + }, + "message": PG_INTEGRITY_ERROR, + } + _scrub_pg_details(event) + assert "learner@example.invalid" not in repr(event) + + +def test_before_send_scrubs_the_event(): + """The scrub is wired into the before_send hook, not just callable.""" + event = {"exception": {"values": [{"value": PG_INTEGRITY_ERROR}]}} + assert "learner@example.invalid" not in repr(_before_send(event, {})) From 4d2ee383e52f40c65fe08d965557966f86d04c30 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Tue, 8 Sep 2026 11:54:16 -0400 Subject: [PATCH 2/3] fix(sentry): scrub DETAIL rows from the whole event, not three named fields Copilot review, verified against sentry-sdk 2.55.0 source. The first pass enumerated three paths -- exception values, logentry.message/.formatted, and the legacy top-level message -- and missed every other field that can carry the same string: breadcrumbs[].message LoggingIntegration records each log record as a breadcrumb (integrations/logging.py:311). This is exactly MITXONLINE-6PK's shape: mechanism=logging, logger=django_scim.views. logentry.params record.args verbatim (:274), so logger.error("...: %s", exc) carries it. frames[].vars include_local_variables defaults to True (consts.py:1028, utils.py:616), so a catch block holding the exception in a local carries it. Confirmed the old implementation leaked on all three shapes before changing it; the new tests fail against it and pass against the walk. Replaced with a recursive walk of the event instead of a longer path list -- it covers these without enumerating them and does not go stale when the SDK grows another such field. The walk only rewrites str leaves and preserves everything else, with a test pinning that. Copilot also suggested normalizing exception-valued params. Not needed: client._prepare_event serializes the event before calling before_send (client.py:650 vs :658), so every leaf is already a JSON primitive by then and there are no live exception objects left to coerce. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RVJKTGk9KHUTN2xfU59ujX --- .../core/observability/sentry.py | 50 ++++++++------ tests/test_sentry.py | 65 +++++++++++++++++++ 2 files changed, 96 insertions(+), 19 deletions(-) diff --git a/src/ol_analytics_api/core/observability/sentry.py b/src/ol_analytics_api/core/observability/sentry.py index 01dac42..6d49977 100644 --- a/src/ol_analytics_api/core/observability/sentry.py +++ b/src/ol_analytics_api/core/observability/sentry.py @@ -6,6 +6,7 @@ from __future__ import annotations import logging +from typing import Any, cast import sentry_sdk from sentry_sdk.integrations.fastapi import FastApiIntegration @@ -42,26 +43,37 @@ def _scrub_pg_detail(text: str) -> str: def _scrub_pg_details(event: Event) -> Event: - """Apply _scrub_pg_detail everywhere an error string lands on the event. - - Covers exception values, the logentry message/formatted pair, and the legacy - top-level message, so the scrub holds whether the event arrived as an - uncaught exception or via logger.exception. + """Truncate Postgres DETAIL lines everywhere in a Sentry event. + + The row echo reaches Sentry through more fields than the exception value: + LoggingIntegration puts the log message in a breadcrumb + (integrations/logging.py:311), logger.error("...: %s", exc) puts it in + logentry.params (:274), and captured stack-frame locals carry it in + frame vars because include_local_variables defaults to True + (consts.py:1028, utils.py:616). Walking the whole event covers those + without enumerating them, and does not go stale when the SDK adds another. + + Safe to walk naively because client._prepare_event serializes the event + before calling before_send (client.py:650 vs :658), so every leaf here is + already a JSON primitive -- no live exception objects to coerce. """ - for entry in (event.get("exception") or {}).get("values") or []: - value = entry.get("value") - if isinstance(value, str): - entry["value"] = _scrub_pg_detail(value) - logentry = event.get("logentry") - if isinstance(logentry, dict): - for key in ("formatted", "message"): - value = logentry.get(key) - if isinstance(value, str): - logentry[key] = _scrub_pg_detail(value) - top_message = event.get("message") - if isinstance(top_message, str): - event["message"] = _scrub_pg_detail(top_message) - return event + return cast("Event", _scrub_node(event)) + + +def _scrub_node(node: Any) -> Any: # noqa: ANN401 -- walks arbitrary JSON + """Recurse through the serialized event, rewriting strings in place.""" + if isinstance(node, str): + return _scrub_pg_detail(node) + if isinstance(node, dict): + for key, value in node.items(): + node[key] = _scrub_node(value) + return node + if isinstance(node, list): + node[:] = [_scrub_node(item) for item in node] + return node + if isinstance(node, tuple): + return tuple(_scrub_node(item) for item in node) + return node def _before_send(event: Event, hint: Hint) -> Event | None: diff --git a/tests/test_sentry.py b/tests/test_sentry.py index d45ba14..5ab49cb 100644 --- a/tests/test_sentry.py +++ b/tests/test_sentry.py @@ -60,3 +60,68 @@ def test_before_send_scrubs_the_event(): """The scrub is wired into the before_send hook, not just callable.""" event = {"exception": {"values": [{"value": PG_INTEGRITY_ERROR}]}} assert "learner@example.invalid" not in repr(_before_send(event, {})) + + +def test_scrubs_breadcrumb_messages(): + """LoggingIntegration records the log message as a breadcrumb.""" + event = { + "breadcrumbs": { + "values": [ + {"type": "log", "category": "django_scim.views", "message": PG_INTEGRITY_ERROR} + ] + } + } + _scrub_pg_details(event) + assert "learner@example.invalid" not in repr(event) + + +def test_scrubs_logentry_params(): + """logger.error("...: %s", exc) puts the exception in logentry.params.""" + event = { + "logentry": { + "message": "Unable to complete SCIM call: %s", + "formatted": "Unable to complete SCIM call: " + PG_INTEGRITY_ERROR, + "params": [PG_INTEGRITY_ERROR], + } + } + _scrub_pg_details(event) + assert "learner@example.invalid" not in repr(event) + + +def test_scrubs_captured_frame_locals(): + """include_local_variables defaults to True, so frame vars carry it too.""" + event = { + "exception": { + "values": [ + { + "value": "boom", + "stacktrace": { + "frames": [ + { + "function": "save", + "vars": {"exc": PG_INTEGRITY_ERROR, "retries": 3}, + } + ] + }, + } + ] + } + } + _scrub_pg_details(event) + assert "learner@example.invalid" not in repr(event) + frame = event["exception"]["values"][0]["stacktrace"]["frames"][0] + assert frame["vars"]["retries"] == 3 + + +def test_walk_preserves_non_string_leaves(): + """The walk must not coerce timestamps, ints or None into strings.""" + event = { + "timestamp": 1757345533.179, + "level": "error", + "extra": {"count": 42, "missing": None, "flag": True}, + "message": PG_INTEGRITY_ERROR, + } + _scrub_pg_details(event) + assert event["timestamp"] == 1757345533.179 + assert event["extra"] == {"count": 42, "missing": None, "flag": True} + assert "learner@example.invalid" not in event["message"] From 6e774cea75a04b22f0497c1c5b256baa64aa8002 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Thu, 10 Sep 2026 14:18:46 -0400 Subject: [PATCH 3/3] fix(sentry): scrub repr'd DETAIL lines and narrow the body-size comment The SDK repr()s frame locals and logging params before before_send, so the DETAIL newline arrives there as a literal backslash-n and the old find() missed it. Tests now use that shape, plus one that goes through the real SDK. The body-size comment cited the WSGI extractor; this app uses the Starlette one. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01MYc2F3cFvSCfVzqeRMshGy --- .../core/observability/sentry.py | 45 ++++++------ tests/test_sentry.py | 71 +++++++++++++++++-- 2 files changed, 91 insertions(+), 25 deletions(-) diff --git a/src/ol_analytics_api/core/observability/sentry.py b/src/ol_analytics_api/core/observability/sentry.py index 6d49977..76cc70c 100644 --- a/src/ol_analytics_api/core/observability/sentry.py +++ b/src/ol_analytics_api/core/observability/sentry.py @@ -6,6 +6,7 @@ from __future__ import annotations import logging +import re from typing import Any, cast import sentry_sdk @@ -26,8 +27,11 @@ # exception value, where no SDK privacy setting reaches it: send_default_pii # governs user/cookie/header capture and max_request_body_size governs request # bodies, and neither touches exception text. -_PG_DETAIL_MARKER = "\nDETAIL:" -_PG_DETAIL_REPLACEMENT = "\nDETAIL: [scrubbed]" +# +# The newline is matched both raw and as a literal backslash-n: the SDK repr()s +# frame locals and non-string logging params during serialization, so there the +# DETAIL line arrives as "...constraint\\nDETAIL: ..." inside a repr string. +_PG_DETAIL_RE = re.compile(r"(\n|\\n)DETAIL:.*", re.DOTALL) def _scrub_pg_detail(text: str) -> str: @@ -36,10 +40,7 @@ def _scrub_pg_detail(text: str) -> str: Keeps the primary message, which is what identifies the failure, and drops the row echo plus any HINT/CONTEXT Postgres appends after it. """ - index = text.find(_PG_DETAIL_MARKER) - if index == -1: - return text - return text[:index] + _PG_DETAIL_REPLACEMENT + return _PG_DETAIL_RE.sub(lambda match: match.group(1) + "DETAIL: [scrubbed]", text, count=1) def _scrub_pg_details(event: Event) -> Event: @@ -47,15 +48,15 @@ def _scrub_pg_details(event: Event) -> Event: The row echo reaches Sentry through more fields than the exception value: LoggingIntegration puts the log message in a breadcrumb - (integrations/logging.py:311), logger.error("...: %s", exc) puts it in - logentry.params (:274), and captured stack-frame locals carry it in - frame vars because include_local_variables defaults to True - (consts.py:1028, utils.py:616). Walking the whole event covers those - without enumerating them, and does not go stale when the SDK adds another. - - Safe to walk naively because client._prepare_event serializes the event - before calling before_send (client.py:650 vs :658), so every leaf here is - already a JSON primitive -- no live exception objects to coerce. + (BreadcrumbHandler._breadcrumb_from_record), logger.error("...: %s", exc) + puts it in logentry.params (EventHandler._emit), and captured stack-frame + locals carry it in frame vars because include_local_variables defaults to + True (serialize_frame). Walking the whole event covers those without + enumerating them, and does not go stale when the SDK adds another. + + Safe to walk naively because Client._prepare_event serializes the event + before calling before_send, so every leaf here is already a JSON + primitive -- no live exception objects to coerce. """ return cast("Event", _scrub_node(event)) @@ -71,8 +72,6 @@ def _scrub_node(node: Any) -> Any: # noqa: ANN401 -- walks arbitrary JSON if isinstance(node, list): node[:] = [_scrub_node(item) for item in node] return node - if isinstance(node, tuple): - return tuple(_scrub_node(item) for item in node) return node @@ -106,11 +105,15 @@ def init_sentry( # noqa: PLR0913 -- matches the org's established init_sentry() environment=environment, release=version, before_send=_before_send, - # Request bodies are NOT gated on send_default_pii: the SDK sets - # request.data unconditionally (sentry_sdk/integrations/_wsgi_common.py - # :123) and this is the only control (:61). Left unset it defaults to + # Request bodies are NOT gated on send_default_pii: the Starlette and + # FastAPI integrations set request.data unconditionally + # (StarletteRequestExtractor.extract_request_info) and this is the only + # control (request_body_within_bounds). extract_request_info attaches + # no body when the content-length header is absent, and applies the + # bound to the declared value otherwise. Left unset it defaults to # "medium", i.e. 10,000-byte bodies. Set explicitly so the choice is - # findable here rather than in a dependency's defaults. + # findable here rather than in a dependency's defaults. Every route + # this service registers is a GET. max_request_body_size="small", # This service serves aggregated-only analytics (no individual # learner PII) — default to not sending request/user PII to Sentry. diff --git a/tests/test_sentry.py b/tests/test_sentry.py index 5ab49cb..2e16e1c 100644 --- a/tests/test_sentry.py +++ b/tests/test_sentry.py @@ -1,5 +1,13 @@ """Tests for Sentry event scrubbing.""" +import json +import logging + +import pytest +import sentry_sdk +from sentry_sdk.integrations.logging import LoggingIntegration +from sentry_sdk.transport import Transport + from ol_analytics_api.core.observability.sentry import ( _before_send, _scrub_pg_detail, @@ -19,6 +27,32 @@ ) +class FakeTransport(Transport): + """Collect outgoing events instead of sending them.""" + + def __init__(self): + super().__init__() + self.events = [] + + def capture_envelope(self, envelope): + self.events.extend(item.payload.json for item in envelope.items if item.type == "event") + + +@pytest.fixture +def sentry_transport(): + """Initialize the real SDK with _before_send, and detach it afterwards.""" + transport = FakeTransport() + sentry_sdk.init( + dsn="https://k@o0.ingest.sentry.io/0", + transport=transport, + before_send=_before_send, + default_integrations=False, + integrations=[LoggingIntegration(level=logging.INFO, event_level=logging.ERROR)], + ) + yield transport + sentry_sdk.get_global_scope().set_client(None) + + def test_detail_line_is_truncated(): """The row echo goes; the primary error that names the failure stays.""" scrubbed = _scrub_pg_detail(PG_INTEGRITY_ERROR) @@ -27,6 +61,13 @@ def test_detail_line_is_truncated(): assert "12d7dfc5-6f84-46db-9383-2d7079434173" not in scrubbed +def test_escaped_detail_line_is_truncated(): + """repr() turns the newline into a literal backslash-n; that form goes too.""" + scrubbed = _scrub_pg_detail(repr(Exception(PG_INTEGRITY_ERROR))) + assert PG_PRIMARY_MESSAGE in scrubbed + assert "learner@example.invalid" not in scrubbed + + def test_message_without_detail_is_unchanged(): """A message with no DETAIL line passes through untouched.""" message = "connection to server failed" @@ -76,12 +117,12 @@ def test_scrubs_breadcrumb_messages(): def test_scrubs_logentry_params(): - """logger.error("...: %s", exc) puts the exception in logentry.params.""" + """logger.error("...: %s", exc) puts the repr'd exception in logentry.params.""" event = { "logentry": { "message": "Unable to complete SCIM call: %s", "formatted": "Unable to complete SCIM call: " + PG_INTEGRITY_ERROR, - "params": [PG_INTEGRITY_ERROR], + "params": [repr(Exception(PG_INTEGRITY_ERROR))], } } _scrub_pg_details(event) @@ -89,7 +130,7 @@ def test_scrubs_logentry_params(): def test_scrubs_captured_frame_locals(): - """include_local_variables defaults to True, so frame vars carry it too.""" + """include_local_variables defaults to True, so repr'd frame vars carry it.""" event = { "exception": { "values": [ @@ -99,7 +140,10 @@ def test_scrubs_captured_frame_locals(): "frames": [ { "function": "save", - "vars": {"exc": PG_INTEGRITY_ERROR, "retries": 3}, + "vars": { + "exc": repr(Exception(PG_INTEGRITY_ERROR)), + "retries": 3, + }, } ] }, @@ -125,3 +169,22 @@ def test_walk_preserves_non_string_leaves(): assert event["timestamp"] == 1757345533.179 assert event["extra"] == {"count": 42, "missing": None, "flag": True} assert "learner@example.invalid" not in event["message"] + + +def test_real_sdk_scrubs_params_and_local_variables(sentry_transport): + """Go through the real SDK, which repr()s params and locals before before_send.""" + + def save(): + exc = Exception(PG_INTEGRITY_ERROR) + raise exc + + try: + save() + except Exception as e: # noqa: BLE001 + logging.getLogger("x").error("Unable to save: %s", e) # noqa: TRY400 + sentry_sdk.capture_exception(e) + sentry_sdk.flush() + + assert len(sentry_transport.events) == 2 + for event in sentry_transport.events: + assert "learner@example.invalid" not in json.dumps(event)