diff --git a/main/sentry.py b/main/sentry.py index 9a2d5fd754..4304b3b98e 100644 --- a/main/sentry.py +++ b/main/sentry.py @@ -1,6 +1,7 @@ """Sentry setup and configuration""" import logging +import re import sentry_sdk from celery.exceptions import WorkerLostError @@ -18,6 +19,62 @@ log = logging.getLogger() +# Postgres appends a DETAIL line to constraint violations that echoes the whole +# offending row -- on a users table that is the learner's name, email and +# external UUID. 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. +# +# 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): + """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. + """ + return PG_DETAIL_RE.sub( + lambda match: match.group(1) + "DETAIL: [scrubbed]", text, count=1 + ) + + +def scrub_pg_details(event): + """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 + (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 _scrub_node(event) + + +def _scrub_node(node): + """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 + return node + + def before_send(event, hint): """ Filter or transform events before they're sent to Sentry @@ -34,7 +91,7 @@ def before_send(event, hint): if isinstance(exc_value, SHUTDOWN_ERRORS): # so we don't want to report expected shutdown errors to sentry return None - return event + return scrub_pg_details(event) def init_sentry( # noqa: PLR0913 @@ -74,6 +131,23 @@ def init_sentry( # noqa: PLR0913 environment=environment, release=version, before_send=before_send, + # Request bodies are NOT gated on send_default_pii: the SDK sets + # request.data unconditionally (RequestExtractor.extract_into_event) + # and this is the only control (request_body_within_bounds). 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. + # + # This applies to every Django view, the webhooks included. The + # content_files webhook posts a few short fields (content_path, source, + # course ids), so its bodies still fit under the bound. + # + # It is not a hard 1KB cap: the bound is checked against the declared + # Content-Length, and a missing one counts as 0. Under ASGI (granian + # serving main.asgi), Django takes CONTENT_LENGTH from the request + # header, so a chunked request without one has its parsed body + # captured in full. + max_request_body_size="small", traces_sample_rate=traces_sample_rate, profiles_sample_rate=profiles_sample_rate, # Sentry's auto-enabling integrations (langchain, openai, etc.) import diff --git a/main/sentry_test.py b/main/sentry_test.py new file mode 100644 index 0000000000..96cbe47e5c --- /dev/null +++ b/main/sentry_test.py @@ -0,0 +1,198 @@ +"""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 main.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' +) + + +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) + assert scrubbed.startswith(PG_PRIMARY_MESSAGE) + assert "learner@example.invalid" not in scrubbed + 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" + 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, {})) + + +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 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": [repr(Exception(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 repr'd frame vars carry it.""" + event = { + "exception": { + "values": [ + { + "value": "boom", + "stacktrace": { + "frames": [ + { + "function": "save", + "vars": { + "exc": repr(Exception(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"] + + +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)