From 39c15e121740b69c0f88cdcb1176d01eab160894 Mon Sep 17 00:00:00 2001 From: Ahmed Etefy Date: Mon, 1 Mar 2021 14:27:25 +0100 Subject: [PATCH 1/5] Passed django setting USE_X_FORWARDED_FOR to sentry wsgi middleware upon creation --- sentry_sdk/integrations/django/__init__.py | 6 +++++- sentry_sdk/integrations/wsgi.py | 24 ++++++++++++++-------- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/sentry_sdk/integrations/django/__init__.py b/sentry_sdk/integrations/django/__init__.py index 2b571f5e11..f8653e4ab9 100644 --- a/sentry_sdk/integrations/django/__init__.py +++ b/sentry_sdk/integrations/django/__init__.py @@ -120,7 +120,11 @@ def sentry_patched_wsgi_handler(self, environ, start_response): bound_old_app = old_app.__get__(self, WSGIHandler) - return SentryWsgiMiddleware(bound_old_app)(environ, start_response) + from django.conf import settings + use_x_forwarded_for = settings.get('USE_X_FORWARDED_HOST', False) + + return SentryWsgiMiddleware( + bound_old_app, use_x_forwarded_for)(environ, start_response) WSGIHandler.__call__ = sentry_patched_wsgi_handler diff --git a/sentry_sdk/integrations/wsgi.py b/sentry_sdk/integrations/wsgi.py index 2f63298ffa..988f9d99ad 100644 --- a/sentry_sdk/integrations/wsgi.py +++ b/sentry_sdk/integrations/wsgi.py @@ -54,10 +54,16 @@ def wsgi_decoding_dance(s, charset="utf-8", errors="replace"): return s.encode("latin1").decode(charset, errors) -def get_host(environ): +def get_host(environ, use_x_forwarded_for=False): # type: (Dict[str, str]) -> str """Return the host for the given WSGI environment. Yanked from Werkzeug.""" - if environ.get("HTTP_HOST"): + if use_x_forwarded_for and 'HTTP_X_FORWARDED_HOST' in environ: + rv = environ['HTTP_X_FORWARDED_HOST'] + if environ["wsgi.url_scheme"] == "http" and rv.endswith(":80"): + rv = rv[:-3] + elif environ["wsgi.url_scheme"] == "https" and rv.endswith(":443"): + rv = rv[:-4] + elif environ.get("HTTP_HOST"): rv = environ["HTTP_HOST"] if environ["wsgi.url_scheme"] == "http" and rv.endswith(":80"): rv = rv[:-3] @@ -77,13 +83,13 @@ def get_host(environ): return rv -def get_request_url(environ): +def get_request_url(environ, use_x_forwarded_for=False): # type: (Dict[str, str]) -> str """Return the absolute URL without query string for the given WSGI environment.""" return "%s://%s/%s" % ( environ.get("wsgi.url_scheme"), - get_host(environ), + get_host(environ, use_x_forwarded_for), wsgi_decoding_dance(environ.get("PATH_INFO") or "").lstrip("/"), ) @@ -91,9 +97,10 @@ def get_request_url(environ): class SentryWsgiMiddleware(object): __slots__ = ("app",) - def __init__(self, app): + def __init__(self, app, use_x_forwarded_for=False): # type: (Callable[[Dict[str, str], Callable[..., Any]], Any]) -> None self.app = app + self.use_x_forwarded_for = use_x_forwarded_for def __call__(self, environ, start_response): # type: (Dict[str, str], Callable[..., Any]) -> _ScopedResponse @@ -110,7 +117,8 @@ def __call__(self, environ, start_response): scope.clear_breadcrumbs() scope._name = "wsgi" scope.add_event_processor( - _make_wsgi_event_processor(environ) + _make_wsgi_event_processor(environ, + self.use_x_forwarded_for) ) transaction = Transaction.continue_from_environ( @@ -269,7 +277,7 @@ def close(self): reraise(*_capture_exception(self._hub)) -def _make_wsgi_event_processor(environ): +def _make_wsgi_event_processor(environ, use_x_forwarded_for): # type: (Dict[str, str]) -> EventProcessor # It's a bit unfortunate that we have to extract and parse the request data # from the environ so eagerly, but there are a few good reasons for this. @@ -284,7 +292,7 @@ def _make_wsgi_event_processor(environ): # https://github.com/unbit/uwsgi/issues/1950 client_ip = get_client_ip(environ) - request_url = get_request_url(environ) + request_url = get_request_url(environ, use_x_forwarded_for) query_string = environ.get("QUERY_STRING") method = environ.get("REQUEST_METHOD") env = dict(_get_environ(environ)) From eaf122e32fad17531106e34c57e1813e4fd52dbf Mon Sep 17 00:00:00 2001 From: Ahmed Etefy Date: Mon, 1 Mar 2021 14:35:20 +0100 Subject: [PATCH 2/5] Linting changes --- sentry_sdk/integrations/django/__init__.py | 8 +++++--- sentry_sdk/integrations/wsgi.py | 19 ++++++++++--------- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/sentry_sdk/integrations/django/__init__.py b/sentry_sdk/integrations/django/__init__.py index f8653e4ab9..d396da85ec 100644 --- a/sentry_sdk/integrations/django/__init__.py +++ b/sentry_sdk/integrations/django/__init__.py @@ -121,10 +121,12 @@ def sentry_patched_wsgi_handler(self, environ, start_response): bound_old_app = old_app.__get__(self, WSGIHandler) from django.conf import settings - use_x_forwarded_for = settings.get('USE_X_FORWARDED_HOST', False) - return SentryWsgiMiddleware( - bound_old_app, use_x_forwarded_for)(environ, start_response) + use_x_forwarded_for = settings.get("USE_X_FORWARDED_HOST", False) + + return SentryWsgiMiddleware(bound_old_app, use_x_forwarded_for)( + environ, start_response + ) WSGIHandler.__call__ = sentry_patched_wsgi_handler diff --git a/sentry_sdk/integrations/wsgi.py b/sentry_sdk/integrations/wsgi.py index 988f9d99ad..4f274fa00c 100644 --- a/sentry_sdk/integrations/wsgi.py +++ b/sentry_sdk/integrations/wsgi.py @@ -55,10 +55,10 @@ def wsgi_decoding_dance(s, charset="utf-8", errors="replace"): def get_host(environ, use_x_forwarded_for=False): - # type: (Dict[str, str]) -> str + # type: (Dict[str, str], bool) -> str """Return the host for the given WSGI environment. Yanked from Werkzeug.""" - if use_x_forwarded_for and 'HTTP_X_FORWARDED_HOST' in environ: - rv = environ['HTTP_X_FORWARDED_HOST'] + if use_x_forwarded_for and "HTTP_X_FORWARDED_HOST" in environ: + rv = environ["HTTP_X_FORWARDED_HOST"] if environ["wsgi.url_scheme"] == "http" and rv.endswith(":80"): rv = rv[:-3] elif environ["wsgi.url_scheme"] == "https" and rv.endswith(":443"): @@ -84,7 +84,7 @@ def get_host(environ, use_x_forwarded_for=False): def get_request_url(environ, use_x_forwarded_for=False): - # type: (Dict[str, str]) -> str + # type: (Dict[str, str], bool) -> str """Return the absolute URL without query string for the given WSGI environment.""" return "%s://%s/%s" % ( @@ -95,10 +95,10 @@ def get_request_url(environ, use_x_forwarded_for=False): class SentryWsgiMiddleware(object): - __slots__ = ("app",) + __slots__ = ("app", "use_x_forwarded_for") def __init__(self, app, use_x_forwarded_for=False): - # type: (Callable[[Dict[str, str], Callable[..., Any]], Any]) -> None + # type: (Callable[[Dict[str, str], Callable[..., Any]], Any], bool) -> None self.app = app self.use_x_forwarded_for = use_x_forwarded_for @@ -117,8 +117,9 @@ def __call__(self, environ, start_response): scope.clear_breadcrumbs() scope._name = "wsgi" scope.add_event_processor( - _make_wsgi_event_processor(environ, - self.use_x_forwarded_for) + _make_wsgi_event_processor( + environ, self.use_x_forwarded_for + ) ) transaction = Transaction.continue_from_environ( @@ -278,7 +279,7 @@ def close(self): def _make_wsgi_event_processor(environ, use_x_forwarded_for): - # type: (Dict[str, str]) -> EventProcessor + # type: (Dict[str, str], bool) -> EventProcessor # It's a bit unfortunate that we have to extract and parse the request data # from the environ so eagerly, but there are a few good reasons for this. # From e77ed8c9ea2c5b695e1e953f059c5a34bd80c3e8 Mon Sep 17 00:00:00 2001 From: Ahmed Etefy Date: Mon, 1 Mar 2021 16:42:56 +0100 Subject: [PATCH 3/5] Accessed settings attr correctly --- 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 d396da85ec..40f6ab3011 100644 --- a/sentry_sdk/integrations/django/__init__.py +++ b/sentry_sdk/integrations/django/__init__.py @@ -122,7 +122,7 @@ def sentry_patched_wsgi_handler(self, environ, start_response): from django.conf import settings - use_x_forwarded_for = settings.get("USE_X_FORWARDED_HOST", False) + use_x_forwarded_for = settings.USE_X_FORWARDED_HOST return SentryWsgiMiddleware(bound_old_app, use_x_forwarded_for)( environ, start_response From f46a2699d5002ee843d1886e95d25344b0c675f4 Mon Sep 17 00:00:00 2001 From: Ahmed Etefy Date: Mon, 1 Mar 2021 16:44:15 +0100 Subject: [PATCH 4/5] Added django tests for django setting of USE_X_FORWARDED_HOST and extracting the correct request url from it --- tests/integrations/django/test_basic.py | 43 +++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/tests/integrations/django/test_basic.py b/tests/integrations/django/test_basic.py index e094d23a72..acabb18461 100644 --- a/tests/integrations/django/test_basic.py +++ b/tests/integrations/django/test_basic.py @@ -40,6 +40,49 @@ def test_view_exceptions(sentry_init, client, capture_exceptions, capture_events assert event["exception"]["values"][0]["mechanism"]["type"] == "django" +def test_ensures_x_forwarded_header_is_honored_in_sdk_when_enabled_in_django( + sentry_init, client, capture_exceptions, capture_events): + """ + Test that ensures if django settings.USE_X_FORWARDED_HOST is set to True + then the SDK sets the request url to the `HTTP_X_FORWARDED_FOR` + """ + from django.conf import settings + settings.USE_X_FORWARDED_HOST = True + + sentry_init(integrations=[DjangoIntegration()], send_default_pii=True) + exceptions = capture_exceptions() + events = capture_events() + client.get(reverse("view_exc"), + headers={"X_FORWARDED_HOST": 'example.com'}) + + (error,) = exceptions + assert isinstance(error, ZeroDivisionError) + + (event,) = events + assert event["request"]["url"] == 'http://example.com/view-exc' + + settings.USE_X_FORWARDED_HOST = False + + +def test_ensures_x_forwarded_header_is_not_honored_when_unenabled_in_django( + sentry_init, client, capture_exceptions, capture_events): + """ + Test that ensures if django settings.USE_X_FORWARDED_HOST is set to False + then the SDK sets the request url to the `HTTP_POST` + """ + sentry_init(integrations=[DjangoIntegration()], send_default_pii=True) + exceptions = capture_exceptions() + events = capture_events() + client.get(reverse("view_exc"), + headers={"X_FORWARDED_HOST": 'example.com'}) + + (error,) = exceptions + assert isinstance(error, ZeroDivisionError) + + (event,) = events + assert event["request"]["url"] == 'http://localhost/view-exc' + + def test_middleware_exceptions(sentry_init, client, capture_exceptions): sentry_init(integrations=[DjangoIntegration()], send_default_pii=True) exceptions = capture_exceptions() From 9a13364aaf0a9c9889fa86d96ad7c5110ba2f2f3 Mon Sep 17 00:00:00 2001 From: sentry-bot Date: Mon, 1 Mar 2021 15:45:07 +0000 Subject: [PATCH 5/5] fix: Formatting --- tests/integrations/django/test_basic.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/tests/integrations/django/test_basic.py b/tests/integrations/django/test_basic.py index acabb18461..5a4d801374 100644 --- a/tests/integrations/django/test_basic.py +++ b/tests/integrations/django/test_basic.py @@ -41,31 +41,33 @@ def test_view_exceptions(sentry_init, client, capture_exceptions, capture_events def test_ensures_x_forwarded_header_is_honored_in_sdk_when_enabled_in_django( - sentry_init, client, capture_exceptions, capture_events): + sentry_init, client, capture_exceptions, capture_events +): """ Test that ensures if django settings.USE_X_FORWARDED_HOST is set to True then the SDK sets the request url to the `HTTP_X_FORWARDED_FOR` """ from django.conf import settings + settings.USE_X_FORWARDED_HOST = True sentry_init(integrations=[DjangoIntegration()], send_default_pii=True) exceptions = capture_exceptions() events = capture_events() - client.get(reverse("view_exc"), - headers={"X_FORWARDED_HOST": 'example.com'}) + client.get(reverse("view_exc"), headers={"X_FORWARDED_HOST": "example.com"}) (error,) = exceptions assert isinstance(error, ZeroDivisionError) (event,) = events - assert event["request"]["url"] == 'http://example.com/view-exc' + assert event["request"]["url"] == "http://example.com/view-exc" settings.USE_X_FORWARDED_HOST = False def test_ensures_x_forwarded_header_is_not_honored_when_unenabled_in_django( - sentry_init, client, capture_exceptions, capture_events): + sentry_init, client, capture_exceptions, capture_events +): """ Test that ensures if django settings.USE_X_FORWARDED_HOST is set to False then the SDK sets the request url to the `HTTP_POST` @@ -73,14 +75,13 @@ def test_ensures_x_forwarded_header_is_not_honored_when_unenabled_in_django( sentry_init(integrations=[DjangoIntegration()], send_default_pii=True) exceptions = capture_exceptions() events = capture_events() - client.get(reverse("view_exc"), - headers={"X_FORWARDED_HOST": 'example.com'}) + client.get(reverse("view_exc"), headers={"X_FORWARDED_HOST": "example.com"}) (error,) = exceptions assert isinstance(error, ZeroDivisionError) (event,) = events - assert event["request"]["url"] == 'http://localhost/view-exc' + assert event["request"]["url"] == "http://localhost/view-exc" def test_middleware_exceptions(sentry_init, client, capture_exceptions):