From 4d35ea422b32f3da95b31dc504c76f4e44fa5c8c Mon Sep 17 00:00:00 2001 From: Ivana Kellyer Date: Fri, 20 Sep 2024 12:56:45 +0200 Subject: [PATCH 1/8] fix(django): Handle RawPostDataException --- sentry_sdk/integrations/django/__init__.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/sentry_sdk/integrations/django/__init__.py b/sentry_sdk/integrations/django/__init__.py index fce93503e9..8eb2239338 100644 --- a/sentry_sdk/integrations/django/__init__.py +++ b/sentry_sdk/integrations/django/__init__.py @@ -32,6 +32,7 @@ from django.conf import settings as django_settings from django.core import signals from django.conf import settings + from django.http.request import RawPostDataException try: from django.urls import resolve @@ -550,8 +551,14 @@ def cookies(self): return clean_cookies def raw_data(self): - # type: () -> bytes - return self.request.body + # type: () -> Optional[bytes] + try: + return self.request.body + except RawPostDataException: + # We can't access the body anymore after the stream has been read. + # TODO: This is just a mitigation so that we don't break. Figure out + # how to prevent this from happening in the first place. + return None def form(self): # type: () -> QueryDict From 04a0226755ef7887815e2ab7bb53d1f297302fb5 Mon Sep 17 00:00:00 2001 From: Ivana Kellyer Date: Fri, 20 Sep 2024 14:02:24 +0200 Subject: [PATCH 2/8] wip tests --- sentry_sdk/integrations/django/__init__.py | 5 ++-- tests/integrations/django/test_basic.py | 31 ++++++++++++++++++++++ 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/sentry_sdk/integrations/django/__init__.py b/sentry_sdk/integrations/django/__init__.py index 8eb2239338..c212a49db0 100644 --- a/sentry_sdk/integrations/django/__init__.py +++ b/sentry_sdk/integrations/django/__init__.py @@ -555,9 +555,8 @@ def raw_data(self): try: return self.request.body except RawPostDataException: - # We can't access the body anymore after the stream has been read. - # TODO: This is just a mitigation so that we don't break. Figure out - # how to prevent this from happening in the first place. + # We can't access the body anymore because the stream has already + # been read return None def form(self): diff --git a/tests/integrations/django/test_basic.py b/tests/integrations/django/test_basic.py index 45c25595f3..aa93372f91 100644 --- a/tests/integrations/django/test_basic.py +++ b/tests/integrations/django/test_basic.py @@ -3,6 +3,7 @@ import re import pytest from functools import partial +from unittest import mock from werkzeug.test import Client @@ -10,6 +11,7 @@ from django.contrib.auth.models import User from django.core.management import execute_from_command_line from django.db.utils import OperationalError, ProgrammingError, DataError +from django.http.request import HttpRequest, RawPostDataException try: from django.urls import reverse @@ -740,6 +742,35 @@ def test_read_request(sentry_init, client, capture_events): assert "data" not in event["request"] +def test_request_body_already_read(sentry_init, client, capture_events): + sentry_init(integrations=[DjangoIntegration()]) + + events = capture_events() + + class MockRequest(HttpRequest): + @property + def body(self): + raise RawPostDataException + + @property + def data(self): + raise AttributeError + + with mock.patch("django.http.request.HttpRequest", MockRequest): + response = client.post( + reverse("post_echo"), data=b'{"hey": 42}', content_type="application/json" + ) + content, status, headers = unpack_werkzeug_response(response) + + assert status.lower() == "200 ok" + assert content == b'{"hey": 42}' + + (event,) = events + + assert event["message"] == "hi" + assert event["request"]["data"] == {"hey": 42} + + def test_template_tracing_meta(sentry_init, client, capture_events): sentry_init(integrations=[DjangoIntegration()]) events = capture_events() From f09e071c428fabf0101bf3c77fd8667ff5403538 Mon Sep 17 00:00:00 2001 From: Ivana Kellyer Date: Fri, 20 Sep 2024 16:17:00 +0200 Subject: [PATCH 3/8] more wip tests --- tests/integrations/django/test_basic.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integrations/django/test_basic.py b/tests/integrations/django/test_basic.py index aa93372f91..8d198d746d 100644 --- a/tests/integrations/django/test_basic.py +++ b/tests/integrations/django/test_basic.py @@ -756,7 +756,7 @@ def body(self): def data(self): raise AttributeError - with mock.patch("django.http.request.HttpRequest", MockRequest): + with mock.patch("werkzeug.test.Request", MockRequest): response = client.post( reverse("post_echo"), data=b'{"hey": 42}', content_type="application/json" ) From 217382a258432bf74579820cd34eaaf7c60ba979 Mon Sep 17 00:00:00 2001 From: Ivana Kellyer Date: Fri, 20 Sep 2024 17:04:58 +0200 Subject: [PATCH 4/8] follow the existing example --- sentry_sdk/integrations/_wsgi_common.py | 8 +++++++- sentry_sdk/integrations/django/__init__.py | 8 +------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/sentry_sdk/integrations/_wsgi_common.py b/sentry_sdk/integrations/_wsgi_common.py index 14a4c4aea4..c4f3f1c77e 100644 --- a/sentry_sdk/integrations/_wsgi_common.py +++ b/sentry_sdk/integrations/_wsgi_common.py @@ -152,7 +152,13 @@ def json(self): if not self.is_json(): return None - raw_data = self.raw_data() + try: + raw_data = self.raw_data() + except (RawPostDataException, ValueError): + # The body might have already been read, in which case this will + # fail + raw_data = None + if raw_data is None: return None diff --git a/sentry_sdk/integrations/django/__init__.py b/sentry_sdk/integrations/django/__init__.py index c212a49db0..c35eb87349 100644 --- a/sentry_sdk/integrations/django/__init__.py +++ b/sentry_sdk/integrations/django/__init__.py @@ -32,7 +32,6 @@ from django.conf import settings as django_settings from django.core import signals from django.conf import settings - from django.http.request import RawPostDataException try: from django.urls import resolve @@ -552,12 +551,7 @@ def cookies(self): def raw_data(self): # type: () -> Optional[bytes] - try: - return self.request.body - except RawPostDataException: - # We can't access the body anymore because the stream has already - # been read - return None + return self.request.body def form(self): # type: () -> QueryDict From 94078faa8a8e86e606362139e4939d8ce9aa3076 Mon Sep 17 00:00:00 2001 From: Ivana Kellyer Date: Fri, 20 Sep 2024 17:06:19 +0200 Subject: [PATCH 5/8] revert type --- sentry_sdk/integrations/django/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry_sdk/integrations/django/__init__.py b/sentry_sdk/integrations/django/__init__.py index c35eb87349..fce93503e9 100644 --- a/sentry_sdk/integrations/django/__init__.py +++ b/sentry_sdk/integrations/django/__init__.py @@ -550,7 +550,7 @@ def cookies(self): return clean_cookies def raw_data(self): - # type: () -> Optional[bytes] + # type: () -> bytes return self.request.body def form(self): From f1d467bcd15d709b1e0b60de014b11ae82f5d4d5 Mon Sep 17 00:00:00 2001 From: Ivana Kellyer Date: Fri, 20 Sep 2024 17:07:26 +0200 Subject: [PATCH 6/8] test --- tests/integrations/django/test_basic.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/integrations/django/test_basic.py b/tests/integrations/django/test_basic.py index 8d198d746d..f3934a467c 100644 --- a/tests/integrations/django/test_basic.py +++ b/tests/integrations/django/test_basic.py @@ -3,15 +3,16 @@ import re import pytest from functools import partial -from unittest import mock +from unittest.mock import patch from werkzeug.test import Client from django import VERSION as DJANGO_VERSION from django.contrib.auth.models import User +from django.core.handlers.wsgi import WSGIRequest from django.core.management import execute_from_command_line from django.db.utils import OperationalError, ProgrammingError, DataError -from django.http.request import HttpRequest, RawPostDataException +from django.http.request import RawPostDataException try: from django.urls import reverse @@ -747,7 +748,7 @@ def test_request_body_already_read(sentry_init, client, capture_events): events = capture_events() - class MockRequest(HttpRequest): + class MockRequest(WSGIRequest): @property def body(self): raise RawPostDataException @@ -756,7 +757,7 @@ def body(self): def data(self): raise AttributeError - with mock.patch("werkzeug.test.Request", MockRequest): + with patch("django.core.handlers.wsgi.WSGIRequest", MockRequest): response = client.post( reverse("post_echo"), data=b'{"hey": 42}', content_type="application/json" ) @@ -768,7 +769,7 @@ def data(self): (event,) = events assert event["message"] == "hi" - assert event["request"]["data"] == {"hey": 42} + assert "data" not in event["request"] def test_template_tracing_meta(sentry_init, client, capture_events): From 2ca480a79e3cb8220f1e151c48ea089d744c9ee4 Mon Sep 17 00:00:00 2001 From: Ivana Kellyer Date: Fri, 20 Sep 2024 17:08:51 +0200 Subject: [PATCH 7/8] test --- tests/integrations/django/test_basic.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/integrations/django/test_basic.py b/tests/integrations/django/test_basic.py index f3934a467c..90db33ddd5 100644 --- a/tests/integrations/django/test_basic.py +++ b/tests/integrations/django/test_basic.py @@ -758,13 +758,9 @@ def data(self): raise AttributeError with patch("django.core.handlers.wsgi.WSGIRequest", MockRequest): - response = client.post( + client.post( reverse("post_echo"), data=b'{"hey": 42}', content_type="application/json" ) - content, status, headers = unpack_werkzeug_response(response) - - assert status.lower() == "200 ok" - assert content == b'{"hey": 42}' (event,) = events From 1c1a4b015184d65261673c00e5a73ad05493c7d2 Mon Sep 17 00:00:00 2001 From: Ivana Kellyer Date: Fri, 20 Sep 2024 17:31:54 +0200 Subject: [PATCH 8/8] test --- tests/integrations/django/test_basic.py | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/tests/integrations/django/test_basic.py b/tests/integrations/django/test_basic.py index 90db33ddd5..f02f8ee217 100644 --- a/tests/integrations/django/test_basic.py +++ b/tests/integrations/django/test_basic.py @@ -9,7 +9,6 @@ from django import VERSION as DJANGO_VERSION from django.contrib.auth.models import User -from django.core.handlers.wsgi import WSGIRequest from django.core.management import execute_from_command_line from django.db.utils import OperationalError, ProgrammingError, DataError from django.http.request import RawPostDataException @@ -23,7 +22,11 @@ from sentry_sdk._compat import PY310 from sentry_sdk import capture_message, capture_exception from sentry_sdk.consts import SPANDATA -from sentry_sdk.integrations.django import DjangoIntegration, _set_db_data +from sentry_sdk.integrations.django import ( + DjangoIntegration, + DjangoRequestExtractor, + _set_db_data, +) from sentry_sdk.integrations.django.signals_handlers import _get_receiver_name from sentry_sdk.integrations.executing import ExecutingIntegration from sentry_sdk.tracing import Span @@ -748,24 +751,19 @@ def test_request_body_already_read(sentry_init, client, capture_events): events = capture_events() - class MockRequest(WSGIRequest): - @property - def body(self): + class MockExtractor(DjangoRequestExtractor): + def raw_data(self): raise RawPostDataException - @property - def data(self): - raise AttributeError - - with patch("django.core.handlers.wsgi.WSGIRequest", MockRequest): + with patch("sentry_sdk.integrations.django.DjangoRequestExtractor", MockExtractor): client.post( reverse("post_echo"), data=b'{"hey": 42}', content_type="application/json" ) - (event,) = events + (event,) = events - assert event["message"] == "hi" - assert "data" not in event["request"] + assert event["message"] == "hi" + assert "data" not in event["request"] def test_template_tracing_meta(sentry_init, client, capture_events):