diff --git a/sentry_sdk/integrations/_asgi_common.py b/sentry_sdk/integrations/_asgi_common.py index 7ff4657013..57ce4d54ca 100644 --- a/sentry_sdk/integrations/_asgi_common.py +++ b/sentry_sdk/integrations/_asgi_common.py @@ -2,8 +2,11 @@ from enum import Enum from typing import TYPE_CHECKING +import sentry_sdk +from sentry_sdk.data_collection import _apply_data_collection_filtering_to_query_string from sentry_sdk.integrations._wsgi_common import _filter_headers from sentry_sdk.scope import should_send_default_pii +from sentry_sdk.utils import has_data_collection_enabled if TYPE_CHECKING: from typing import Any, Dict, Optional, Union @@ -75,7 +78,7 @@ def _get_url( return path -def _get_query(asgi_scope: "Any") -> "Any": +def _get_query(asgi_scope: "Any") -> "Optional[str]": """ Extract querystring from the ASGI scope, in the format that the Sentry protocol expects. """ @@ -122,7 +125,20 @@ def _get_request_data( use_annotated_value=False, ) - request_data["query_string"] = _get_query(asgi_scope) + client_options = sentry_sdk.get_client().options + if has_data_collection_enabled(client_options): + qs = _get_query(asgi_scope) + if qs: + filtered_query_string = ( + _apply_data_collection_filtering_to_query_string( + query_string=qs, + behaviour=client_options["data_collection"]["url_query_params"], + ) + ) + if filtered_query_string: + request_data["query_string"] = filtered_query_string + else: + request_data["query_string"] = _get_query(asgi_scope) request_data["url"] = _get_url( asgi_scope, @@ -158,7 +174,38 @@ def _get_request_attributes( for header, value in filtered_headers.items(): attributes[f"http.request.header.{header.lower()}"] = value - if should_send_default_pii(): + client_options = sentry_sdk.get_client().options + if has_data_collection_enabled(client_options): + filtered_query_string = None + query = _get_query(asgi_scope) + + if query: + filtered_query_string = ( + _apply_data_collection_filtering_to_query_string( + query_string=query, + behaviour=client_options["data_collection"]["url_query_params"], + ) + ) + if filtered_query_string: + attributes["http.query"] = filtered_query_string + + path = _get_path(asgi_scope=asgi_scope, root_path_in_path=root_path_in_path) + attributes["url.path"] = path + + url_without_query_string = _get_url( + asgi_scope, + "http" if ty == "http" else "ws", + headers.get("host"), + path=path, + ) + + attributes["url.full"] = ( + f"{url_without_query_string}?{filtered_query_string}" + if filtered_query_string is not None + else url_without_query_string + ) + + elif should_send_default_pii(): query = _get_query(asgi_scope) if query: attributes["http.query"] = query diff --git a/tests/integrations/asgi/test_asgi.py b/tests/integrations/asgi/test_asgi.py index 04d2a192a7..5957a3e763 100644 --- a/tests/integrations/asgi/test_asgi.py +++ b/tests/integrations/asgi/test_asgi.py @@ -918,6 +918,215 @@ def test_get_request_attributes_url_with_headers_off(sentry_init): assert attributes["url.full"] == "http://example.com/foo?somevalue=123" +QUERY_STRING = "token=abc&theme=dark&lang=en&session=xyz" + + +def _http_scope(query_string=QUERY_STRING): + return { + "type": "http", + "method": "GET", + "scheme": "http", + "server": ("example.com", 80), + "path": "/foo", + "query_string": query_string.encode("latin-1"), + "headers": [(b"host", b"example.com")], + } + + +@pytest.mark.parametrize( + "init_kwargs, expected_query_string", + [ + pytest.param( + {"send_default_pii": True}, + QUERY_STRING, + id="send_default_pii_true", + ), + pytest.param( + {"send_default_pii": False}, + QUERY_STRING, + id="send_default_pii_false", + ), + pytest.param( + {}, + QUERY_STRING, + id="defaults", + ), + pytest.param( + {"_experiments": {"data_collection": {}}}, + "token=%5BFiltered%5D&theme=dark&lang=en&session=%5BFiltered%5D", + id="data_collection_denylist_default", + ), + pytest.param( + { + "_experiments": { + "data_collection": { + "url_query_params": {"mode": "denylist", "terms": ["theme"]} + } + } + }, + "token=%5BFiltered%5D&theme=%5BFiltered%5D&lang=en&session=%5BFiltered%5D", + id="data_collection_denylist_custom_terms", + ), + pytest.param( + { + "_experiments": { + "data_collection": { + "url_query_params": {"mode": "allowlist", "terms": ["theme"]} + } + } + }, + "token=%5BFiltered%5D&theme=dark&lang=%5BFiltered%5D&session=%5BFiltered%5D", + id="data_collection_allowlist", + ), + pytest.param( + { + "_experiments": { + "data_collection": { + "url_query_params": {"mode": "allowlist", "terms": ["token"]} + } + } + }, + "token=%5BFiltered%5D&theme=%5BFiltered%5D&lang=%5BFiltered%5D&session=%5BFiltered%5D", + id="data_collection_allowlist_sensitive_term", + ), + pytest.param( + { + "_experiments": { + "data_collection": {"url_query_params": {"mode": "off"}} + } + }, + None, + id="data_collection_off", + ), + # data_collection wins over send_default_pii: filtering still applies. + pytest.param( + { + "send_default_pii": True, + "_experiments": { + "data_collection": {"url_query_params": {"mode": "off"}} + }, + }, + None, + id="data_collection_wins_over_send_default_pii", + ), + ], +) +def test_get_request_data_query_string_data_collection( + sentry_init, init_kwargs, expected_query_string +): + sentry_init(**init_kwargs) + + request_data = _get_request_data(_http_scope(), _RootPathInPath.EXCLUDED) + + if expected_query_string is None: + assert "query_string" not in request_data + else: + assert request_data["query_string"] == expected_query_string + + +def test_get_request_data_query_string_empty_legacy_is_none(sentry_init): + # Legacy path: the query string is always set even when empty (``None``). + sentry_init(send_default_pii=True) + + request_data = _get_request_data( + _http_scope(query_string=""), _RootPathInPath.EXCLUDED + ) + + assert request_data["query_string"] is None + + +def test_get_request_data_empty_query_string_dropped_with_data_collection(sentry_init): + sentry_init(_experiments={"data_collection": {}}) + + request_data = _get_request_data( + _http_scope(query_string=""), _RootPathInPath.EXCLUDED + ) + + assert "query_string" not in request_data + + +@pytest.mark.parametrize( + "init_kwargs, expected_query, expected_url_full", + [ + pytest.param( + {"send_default_pii": True}, + QUERY_STRING, + "http://example.com/foo?" + QUERY_STRING, + id="send_default_pii_true", + ), + pytest.param( + {"send_default_pii": False}, + None, + None, + id="send_default_pii_false", + ), + pytest.param( + {}, + None, + None, + id="defaults", + ), + pytest.param( + {"_experiments": {"data_collection": {}}}, + "token=%5BFiltered%5D&theme=dark&lang=en&session=%5BFiltered%5D", + "http://example.com/foo?token=%5BFiltered%5D&theme=dark&lang=en&session=%5BFiltered%5D", + id="data_collection_denylist_default", + ), + pytest.param( + { + "_experiments": { + "data_collection": { + "url_query_params": {"mode": "allowlist", "terms": ["theme"]} + } + } + }, + "token=%5BFiltered%5D&theme=dark&lang=%5BFiltered%5D&session=%5BFiltered%5D", + "http://example.com/foo?token=%5BFiltered%5D&theme=dark&lang=%5BFiltered%5D&session=%5BFiltered%5D", + id="data_collection_allowlist", + ), + pytest.param( + { + "_experiments": { + "data_collection": {"url_query_params": {"mode": "off"}} + } + }, + None, + "http://example.com/foo", + id="data_collection_off", + ), + pytest.param( + { + "send_default_pii": True, + "_experiments": { + "data_collection": {"url_query_params": {"mode": "off"}} + }, + }, + None, + "http://example.com/foo", + id="data_collection_wins_over_send_default_pii", + ), + ], +) +def test_get_request_attributes_query_data_collection( + sentry_init, init_kwargs, expected_query, expected_url_full +): + sentry_init(**init_kwargs) + + attributes = _get_request_attributes(_http_scope(), _RootPathInPath.EXCLUDED) + + if expected_query is None: + assert "http.query" not in attributes + else: + assert attributes["http.query"] == expected_query + + if expected_url_full is None: + assert "url.full" not in attributes + assert "url.path" not in attributes + else: + assert attributes["url.full"] == expected_url_full + assert attributes["url.path"] == "/foo" + + @pytest.mark.asyncio @pytest.mark.parametrize( "request_url,transaction_style,expected_transaction_name,expected_transaction_source", diff --git a/tests/integrations/django/test_data_scrubbing.py b/tests/integrations/django/test_data_scrubbing.py index 334c068cf3..7ea1d19d44 100644 --- a/tests/integrations/django/test_data_scrubbing.py +++ b/tests/integrations/django/test_data_scrubbing.py @@ -14,9 +14,6 @@ from django.core.urlresolvers import reverse -NO_COOKIES = object() - - @pytest.fixture def client(): return Client(application) diff --git a/tests/integrations/litestar/test_litestar.py b/tests/integrations/litestar/test_litestar.py index 28d70b6b8c..3567e5c740 100644 --- a/tests/integrations/litestar/test_litestar.py +++ b/tests/integrations/litestar/test_litestar.py @@ -686,9 +686,6 @@ async def __call__(self, scope, receive, send): COOKIE_HEADER = "jwt=tokenval; theme=dark; lang=en; identity=alice" -# Sentinel meaning "the request payload should have no ``cookies`` key at all", -# as opposed to an empty ``{}`` dict. - @pytest.mark.parametrize( "init_kwargs, expected_cookies", diff --git a/tests/integrations/starlette/test_starlette.py b/tests/integrations/starlette/test_starlette.py index eebdd6940f..6ead9f12b5 100644 --- a/tests/integrations/starlette/test_starlette.py +++ b/tests/integrations/starlette/test_starlette.py @@ -42,8 +42,10 @@ COOKIE_HEADER = "jwt=tokenval; theme=dark; lang=en; identity=alice" -# Sentinel meaning "the request payload should have no ``cookies`` key at all", -# as opposed to an empty ``{}`` dict. +# Query string used across the query-param filtering tests below. ``auth`` is a +# built-in sensitive term, so it is redacted by the default denylist. +QUERY_STRING = "toy=tennisball&color=red&auth=secret" + BODY_FORM = """--fd721ef49ea403a6\r\nContent-Disposition: form-data; name="username"\r\n\r\nJane\r\n--fd721ef49ea403a6\r\nContent-Disposition: form-data; name="password"\r\n\r\nhello123\r\n--fd721ef49ea403a6\r\nContent-Disposition: form-data; name="photo"; filename="photo.jpg"\r\nContent-Type: image/jpg\r\nContent-Transfer-Encoding: base64\r\n\r\n{{image_data}}\r\n--fd721ef49ea403a6--\r\n""".replace( "{{image_data}}", str(base64.b64encode(open(PICTURE, "rb").read())) ) @@ -669,6 +671,133 @@ async def test_cookie_data_collection( assert transaction_event["request"]["cookies"] == expected_cookies +@pytest.mark.parametrize( + "init_kwargs, expected_query_string", + [ + pytest.param( + {"send_default_pii": True}, + QUERY_STRING, + id="legacy_send_default_pii_true", + ), + pytest.param( + {"_experiments": {"data_collection": {}}}, + "toy=tennisball&color=red&auth=%5BFiltered%5D", + id="data_collection_denylist_default", + ), + pytest.param( + { + "_experiments": { + "data_collection": { + "url_query_params": {"mode": "allowlist", "terms": ["toy"]} + } + } + }, + "toy=tennisball&color=%5BFiltered%5D&auth=%5BFiltered%5D", + id="data_collection_allowlist", + ), + pytest.param( + { + "_experiments": { + "data_collection": {"url_query_params": {"mode": "off"}} + } + }, + None, + id="data_collection_off", + ), + ], +) +def test_query_string_data_collection( + sentry_init, capture_events, init_kwargs, expected_query_string +): + sentry_init( + traces_sample_rate=1.0, + integrations=[StarletteIntegration()], + **init_kwargs, + ) + + starlette_app = starlette_app_factory() + events = capture_events() + + client = TestClient(starlette_app) + client.get("/message?" + QUERY_STRING) + + (event, transaction_event) = events + + if expected_query_string is None: + assert "query_string" not in event["request"] + assert "query_string" not in transaction_event["request"] + else: + assert event["request"]["query_string"] == expected_query_string + assert transaction_event["request"]["query_string"] == expected_query_string + + +@pytest.mark.parametrize( + "init_kwargs, expected_query, expected_url_full", + [ + pytest.param( + {"send_default_pii": True}, + QUERY_STRING, + "http://testserver/message?" + QUERY_STRING, + id="legacy_send_default_pii_true", + ), + pytest.param( + {"_experiments": {"data_collection": {}}}, + "toy=tennisball&color=red&auth=%5BFiltered%5D", + "http://testserver/message?toy=tennisball&color=red&auth=%5BFiltered%5D", + id="data_collection_denylist_default", + ), + pytest.param( + { + "_experiments": { + "data_collection": {"url_query_params": {"mode": "off"}} + } + }, + None, + "http://testserver/message", + id="data_collection_off", + ), + ], +) +def test_span_http_query_data_collection( + sentry_init, capture_items, init_kwargs, expected_query, expected_url_full +): + sentry_init( + auto_enabling_integrations=False, + integrations=[StarletteIntegration()], + traces_sample_rate=1.0, + _experiments={ + "trace_lifecycle": "stream", + **init_kwargs.pop("_experiments", {}), + }, + **init_kwargs, + ) + + starlette_app = starlette_app_factory() + items = capture_items("span") + + client = TestClient(starlette_app) + client.get("/message?" + QUERY_STRING) + + sentry_sdk.flush() + + segments = [ + item.payload + for item in items + if item.payload.get("is_segment") + and item.payload["attributes"].get("sentry.op") == "http.server" + ] + (segment,) = segments + attributes = segment["attributes"] + + if expected_query is None: + assert SPANDATA.HTTP_QUERY not in attributes + else: + assert attributes[SPANDATA.HTTP_QUERY] == expected_query + + assert attributes["url.full"] == expected_url_full + assert attributes["url.path"] == "/message" + + @pytest.mark.parametrize( "url,transaction_style,expected_transaction,expected_source", [ diff --git a/tests/integrations/starlite/test_starlite.py b/tests/integrations/starlite/test_starlite.py index bc0ba09d0c..e41754b819 100644 --- a/tests/integrations/starlite/test_starlite.py +++ b/tests/integrations/starlite/test_starlite.py @@ -579,9 +579,6 @@ async def __call__(self, scope, receive, send): COOKIE_HEADER = "jwt=tokenval; theme=dark; lang=en; identity=alice" -# Sentinel meaning "the request payload should have no ``cookies`` key at all", -# as opposed to an empty ``{}`` dict. - @pytest.mark.parametrize( "init_kwargs, expected_cookies", diff --git a/tests/integrations/tornado/test_tornado.py b/tests/integrations/tornado/test_tornado.py index cbe71ddc94..0e47303af1 100644 --- a/tests/integrations/tornado/test_tornado.py +++ b/tests/integrations/tornado/test_tornado.py @@ -119,9 +119,6 @@ def test_basic(tornado_testcase, sentry_init, capture_events): # logic, not the always-on scrubber. COOKIE_HEADER = "jwt=tokenval; theme=dark; lang=en; identity=alice" -# Sentinel meaning "the request payload should have no ``cookies`` key at all", -# as opposed to an empty ``{}`` dict. - @pytest.mark.parametrize( "init_kwargs, expected_cookies", diff --git a/tests/integrations/wsgi/test_wsgi.py b/tests/integrations/wsgi/test_wsgi.py index d1f8cc4e1c..658269a158 100644 --- a/tests/integrations/wsgi/test_wsgi.py +++ b/tests/integrations/wsgi/test_wsgi.py @@ -1094,10 +1094,6 @@ def test_request_headers_legacy_pii_passes_headers_through( assert headers["X-Custom-Header"] == "passthrough" -# Sentinel: the query string (event) / ``http.query`` attribute (span) is absent. -NO_QUERY_STRING = object() - - @pytest.mark.parametrize( "init_kwargs, expected_query_string", [ @@ -1155,7 +1151,7 @@ def test_request_headers_legacy_pii_passes_headers_through( "data_collection": {"url_query_params": {"mode": "off"}} } }, - NO_QUERY_STRING, + None, id="data_collection_off", ), ], @@ -1173,7 +1169,7 @@ def test_query_string_data_collection( (event,) = events - if expected_query_string is NO_QUERY_STRING: + if expected_query_string is None: assert "query_string" not in event["request"] else: assert event["request"]["query_string"] == expected_query_string @@ -1191,12 +1187,12 @@ def test_query_string_data_collection( ), pytest.param( {"send_default_pii": False}, - NO_QUERY_STRING, + None, id="send_default_pii_false", ), pytest.param( {}, - NO_QUERY_STRING, + None, id="defaults", ), # data_collection configured: attribute is routed through filtering. @@ -1235,7 +1231,7 @@ def test_query_string_data_collection( "data_collection": {"url_query_params": {"mode": "off"}} } }, - NO_QUERY_STRING, + None, id="data_collection_off", ), ], @@ -1266,7 +1262,7 @@ def dogpark(environ, start_response): (span,) = [item.payload for item in items] - if expected_query is NO_QUERY_STRING: + if expected_query is None: assert "http.query" not in span["attributes"] else: assert span["attributes"]["http.query"] == expected_query