diff --git a/sentry_sdk/integrations/sanic.py b/sentry_sdk/integrations/sanic.py index 3f584b37ed..2d839d1c61 100644 --- a/sentry_sdk/integrations/sanic.py +++ b/sentry_sdk/integrations/sanic.py @@ -196,8 +196,12 @@ async def _context_enter(request: "Request") -> None: sentry_sdk.traces.continue_trace(dict(request.headers)) scope.set_custom_sampling_context({"sanic_request": request}) - if should_send_default_pii() and request.remote_addr: - scope.set_attribute(SPANDATA.USER_IP_ADDRESS, request.remote_addr) + if request.remote_addr: + if has_data_collection_enabled(client.options): + if client.options["data_collection"]["user_info"]: + scope.set_attribute(SPANDATA.USER_IP_ADDRESS, request.remote_addr) + elif should_send_default_pii(): + scope.set_attribute(SPANDATA.USER_IP_ADDRESS, request.remote_addr) span = sentry_sdk.traces.start_span( # Unless the request results in a 404 error, the name and source @@ -401,6 +405,10 @@ def _get_request_attributes(request: "Request") -> "Dict[str, Any]": query=filtered_query or "" ).geturl() + if request.remote_addr: + if client_options["data_collection"]["user_info"]: + attributes[SPANDATA.CLIENT_ADDRESS] = request.remote_addr + elif should_send_default_pii(): attributes[SPANDATA.URL_FULL] = request.url attributes["url.path"] = urlparts.path @@ -408,12 +416,12 @@ def _get_request_attributes(request: "Request") -> "Dict[str, Any]": if urlparts.query: attributes[SPANDATA.HTTP_QUERY] = urlparts.query + if request.remote_addr: + attributes[SPANDATA.CLIENT_ADDRESS] = request.remote_addr + if urlparts.scheme: attributes[SPANDATA.NETWORK_PROTOCOL_NAME] = urlparts.scheme - if should_send_default_pii() and request.remote_addr: - attributes[SPANDATA.CLIENT_ADDRESS] = request.remote_addr - return attributes diff --git a/tests/integrations/django/asgi/test_asgi.py b/tests/integrations/django/asgi/test_asgi.py index fb3212aff8..752dba702a 100644 --- a/tests/integrations/django/asgi/test_asgi.py +++ b/tests/integrations/django/asgi/test_asgi.py @@ -16,10 +16,8 @@ from sentry_sdk.integrations.django import DjangoIntegration from sentry_sdk.integrations.django.asgi import _asgi_middleware_mixin_factory from tests.integrations.django.myapp.asgi import channels_application -from tests.integrations.django.utils import ( - USER_INFO_INIT_KWARGS, - pytest_mark_django_db_decorator, -) +from tests.integrations.django.utils import pytest_mark_django_db_decorator +from tests.integrations.utils import DATA_COLLECTION_USER_INFO_CASES try: from django.urls import reverse @@ -1108,7 +1106,7 @@ async def test_async_middleware_process_exception_is_awaited( @pytest.mark.skipif( django.VERSION < (3, 0), reason="Django ASGI support shipped in 3.0" ) -@pytest.mark.parametrize("init_kwargs, expect_user", USER_INFO_INIT_KWARGS) +@pytest.mark.parametrize("init_kwargs, expect_user", DATA_COLLECTION_USER_INFO_CASES) @pytest_mark_django_db_decorator() async def test_user_identity_error_event_data_collection( sentry_init, capture_events, application, init_kwargs, expect_user diff --git a/tests/integrations/django/test_data_scrubbing.py b/tests/integrations/django/test_data_scrubbing.py index 4f20153a89..559b9b0466 100644 --- a/tests/integrations/django/test_data_scrubbing.py +++ b/tests/integrations/django/test_data_scrubbing.py @@ -6,10 +6,8 @@ from sentry_sdk.integrations.django import DjangoIntegration from tests.conftest import unpack_werkzeug_response, werkzeug_set_cookie from tests.integrations.django.myapp.wsgi import application -from tests.integrations.django.utils import ( - USER_INFO_INIT_KWARGS, - pytest_mark_django_db_decorator, -) +from tests.integrations.django.utils import pytest_mark_django_db_decorator +from tests.integrations.utils import DATA_COLLECTION_USER_INFO_CASES try: from django.urls import reverse @@ -397,7 +395,7 @@ def test_empty_query_string_is_dropped_with_data_collection( @pytest.mark.forked @pytest_mark_django_db_decorator() -@pytest.mark.parametrize("init_kwargs, expect_ip", USER_INFO_INIT_KWARGS) +@pytest.mark.parametrize("init_kwargs, expect_ip", DATA_COLLECTION_USER_INFO_CASES) def test_user_info_span_attributes_data_collection( sentry_init, client, capture_items, init_kwargs, expect_ip ): @@ -433,7 +431,7 @@ def test_user_info_span_attributes_data_collection( @pytest.mark.forked @pytest_mark_django_db_decorator() -@pytest.mark.parametrize("init_kwargs, expect_user", USER_INFO_INIT_KWARGS) +@pytest.mark.parametrize("init_kwargs, expect_user", DATA_COLLECTION_USER_INFO_CASES) def test_user_identity_span_attributes_data_collection( sentry_init, client, capture_items, init_kwargs, expect_user ): @@ -469,7 +467,7 @@ def test_user_identity_span_attributes_data_collection( @pytest.mark.forked @pytest_mark_django_db_decorator() -@pytest.mark.parametrize("init_kwargs, expect_ip", USER_INFO_INIT_KWARGS) +@pytest.mark.parametrize("init_kwargs, expect_ip", DATA_COLLECTION_USER_INFO_CASES) def test_user_info_error_event_data_collection( sentry_init, client, capture_events, init_kwargs, expect_ip ): @@ -490,7 +488,7 @@ def test_user_info_error_event_data_collection( @pytest.mark.forked @pytest_mark_django_db_decorator() -@pytest.mark.parametrize("init_kwargs, expect_user", USER_INFO_INIT_KWARGS) +@pytest.mark.parametrize("init_kwargs, expect_user", DATA_COLLECTION_USER_INFO_CASES) def test_user_identity_error_event_data_collection( sentry_init, client, capture_events, init_kwargs, expect_user ): diff --git a/tests/integrations/django/utils.py b/tests/integrations/django/utils.py index 94495def4a..8270b997ea 100644 --- a/tests/integrations/django/utils.py +++ b/tests/integrations/django/utils.py @@ -19,40 +19,3 @@ ) except AttributeError: pass - - -# Shared parametrization test matrix exercising the precedence between the legacy -# ``send_default_pii`` boolean and the ``data_collection.user_info`` setting. -# The second value indicates whether user info is expected to be collected. -USER_INFO_INIT_KWARGS = [ - pytest.param({"send_default_pii": True}, True, id="legacy_send_default_pii_true"), - pytest.param( - {"send_default_pii": False}, False, id="legacy_send_default_pii_false" - ), - pytest.param( - {"_experiments": {"data_collection": {"user_info": True}}}, - True, - id="data_collection_user_info_true", - ), - pytest.param( - {"_experiments": {"data_collection": {"user_info": False}}}, - False, - id="data_collection_user_info_false", - ), - pytest.param( - { - "send_default_pii": True, - "_experiments": {"data_collection": {"user_info": False}}, - }, - False, - id="data_collection_wins_over_send_default_pii_true", - ), - pytest.param( - { - "send_default_pii": False, - "_experiments": {"data_collection": {"user_info": True}}, - }, - True, - id="data_collection_wins_over_send_default_pii_false", - ), -] diff --git a/tests/integrations/flask/test_flask.py b/tests/integrations/flask/test_flask.py index c091e23f61..71c56d3fef 100644 --- a/tests/integrations/flask/test_flask.py +++ b/tests/integrations/flask/test_flask.py @@ -32,6 +32,7 @@ from sentry_sdk.consts import SPANDATA from sentry_sdk.integrations.logging import LoggingIntegration from sentry_sdk.serializer import MAX_DATABAG_BREADTH +from tests.integrations.utils import DATA_COLLECTION_USER_INFO_CASES # 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. @@ -1398,45 +1399,7 @@ def test_empty_query_string_is_dropped_with_data_collection( assert "query_string" not in event["request"] -# Parametrization shared by the user_info tests below. ``expect_ip`` is -# whether the client IP may be collected under the given init kwargs. -USER_INFO_INIT_KWARGS = [ - pytest.param({"send_default_pii": True}, True, id="legacy_send_default_pii_true"), - pytest.param( - {"send_default_pii": False}, False, id="legacy_send_default_pii_false" - ), - pytest.param( - {"_experiments": {"data_collection": {"user_info": True}}}, - True, - id="data_collection_user_info_true", - ), - pytest.param( - {"_experiments": {"data_collection": {"user_info": False}}}, - False, - id="data_collection_user_info_false", - ), - # ``data_collection`` is the single source of truth: it must win over - # ``send_default_pii`` when both are configured. - pytest.param( - { - "send_default_pii": True, - "_experiments": {"data_collection": {"user_info": False}}, - }, - False, - id="data_collection_wins_over_send_default_pii_true", - ), - pytest.param( - { - "send_default_pii": False, - "_experiments": {"data_collection": {"user_info": True}}, - }, - True, - id="data_collection_wins_over_send_default_pii_false", - ), -] - - -@pytest.mark.parametrize("init_kwargs, expect_ip", USER_INFO_INIT_KWARGS) +@pytest.mark.parametrize("init_kwargs, expect_ip", DATA_COLLECTION_USER_INFO_CASES) def test_user_info_span_attributes_data_collection( sentry_init, app, capture_items, monkeypatch, init_kwargs, expect_ip ): @@ -1472,7 +1435,7 @@ def test_user_info_span_attributes_data_collection( assert "client.address" not in segment["attributes"] -@pytest.mark.parametrize("init_kwargs, expect_ip", USER_INFO_INIT_KWARGS) +@pytest.mark.parametrize("init_kwargs, expect_ip", DATA_COLLECTION_USER_INFO_CASES) def test_user_info_error_event_data_collection( sentry_init, app, capture_events, monkeypatch, init_kwargs, expect_ip ): @@ -1523,7 +1486,7 @@ def crash(): assert "ip_address" not in event.get("user", {}) -@pytest.mark.parametrize("init_kwargs, expect_user", USER_INFO_INIT_KWARGS) +@pytest.mark.parametrize("init_kwargs, expect_user", DATA_COLLECTION_USER_INFO_CASES) def test_flask_login_user_identity_error_event_data_collection( sentry_init, app, capture_events, init_kwargs, expect_user ): @@ -1571,7 +1534,7 @@ def crash(): assert "username" not in user -@pytest.mark.parametrize("init_kwargs, expect_user", USER_INFO_INIT_KWARGS) +@pytest.mark.parametrize("init_kwargs, expect_user", DATA_COLLECTION_USER_INFO_CASES) def test_flask_login_user_identity_span_attributes_data_collection( sentry_init, app, capture_items, init_kwargs, expect_user ): diff --git a/tests/integrations/sanic/test_sanic.py b/tests/integrations/sanic/test_sanic.py index 1f030c425f..0bc6133744 100644 --- a/tests/integrations/sanic/test_sanic.py +++ b/tests/integrations/sanic/test_sanic.py @@ -16,6 +16,7 @@ from sentry_sdk.integrations.sanic import SanicIntegration from sentry_sdk.tracing import TransactionSource from tests.conftest import get_free_port +from tests.integrations.utils import DATA_COLLECTION_USER_INFO_CASES try: from sanic_testing import TestManager @@ -559,9 +560,9 @@ def test_span_origin(sentry_init, app, capture_events, capture_items, span_strea @pytest.mark.skipif( not PERFORMANCE_SUPPORTED, reason="Performance not supported on this Sanic version" ) -@pytest.mark.parametrize("send_default_pii", [True, False]) +@pytest.mark.parametrize("init_kwargs, expect_ip", DATA_COLLECTION_USER_INFO_CASES) def test_user_ip_address_on_all_spans( - sentry_init, app, capture_items, send_default_pii + sentry_init, app, capture_items, init_kwargs, expect_ip ): app.config.FORWARDED_SECRET = "test" @@ -575,8 +576,8 @@ def child_span_handler(request): integrations=[SanicIntegration()], default_integrations=False, traces_sample_rate=1.0, - send_default_pii=send_default_pii, trace_lifecycle="stream", + **init_kwargs, ) items = capture_items("span") @@ -592,7 +593,7 @@ def child_span_handler(request): child_span, server_span = [item.payload for item in items] - if send_default_pii: + if expect_ip: assert server_span["attributes"]["user.ip_address"] == "127.0.0.1" assert child_span["attributes"]["user.ip_address"] == "127.0.0.1" else: @@ -600,6 +601,46 @@ def child_span_handler(request): assert "user.ip_address" not in child_span["attributes"] +@pytest.mark.skipif( + not PERFORMANCE_SUPPORTED, reason="Performance not supported on this Sanic version" +) +@pytest.mark.parametrize("init_kwargs, expect_ip", DATA_COLLECTION_USER_INFO_CASES) +def test_client_address_span_attribute_data_collection( + sentry_init, app, capture_items, init_kwargs, expect_ip +): + app.config.FORWARDED_SECRET = "test" + + sentry_init( + integrations=[SanicIntegration()], + default_integrations=False, + traces_sample_rate=1.0, + trace_lifecycle="stream", + **init_kwargs, + ) + + items = capture_items("span") + + c = get_client(app) + with c as client: + client.get( + "/message", + headers={"Forwarded": "for=127.0.0.1;secret=test"}, + ) + + sentry_sdk.flush() + + (server_span,) = [ + item.payload + for item in items + if item.payload["attributes"].get("sentry.origin") == "auto.http.sanic" + ] + + if expect_ip: + assert server_span["attributes"]["client.address"] == "127.0.0.1" + else: + assert "client.address" not in server_span["attributes"] + + _QUERY_PARAM_DATA_COLLECTION_CASES = [ pytest.param( {"send_default_pii": True}, diff --git a/tests/integrations/utils.py b/tests/integrations/utils.py new file mode 100644 index 0000000000..557ed3b62c --- /dev/null +++ b/tests/integrations/utils.py @@ -0,0 +1,38 @@ +import pytest + +# Shared parametrization test matrix exercising the precedence between the legacy +# ``send_default_pii`` boolean and the ``data_collection.user_info`` setting. +# Each case is ``(init_kwargs, expect_user_info)`` where the second element indicates +# whether user info (IP address, user identity, etc.) is expected to be collected. +DATA_COLLECTION_USER_INFO_CASES = [ + pytest.param({"send_default_pii": True}, True, id="legacy_send_default_pii_true"), + pytest.param( + {"send_default_pii": False}, False, id="legacy_send_default_pii_false" + ), + pytest.param( + {"_experiments": {"data_collection": {"user_info": True}}}, + True, + id="data_collection_user_info_true", + ), + pytest.param( + {"_experiments": {"data_collection": {"user_info": False}}}, + False, + id="data_collection_user_info_false", + ), + pytest.param( + { + "send_default_pii": True, + "_experiments": {"data_collection": {"user_info": False}}, + }, + False, + id="data_collection_wins_over_send_default_pii_true", + ), + pytest.param( + { + "send_default_pii": False, + "_experiments": {"data_collection": {"user_info": True}}, + }, + True, + id="data_collection_wins_over_send_default_pii_false", + ), +] diff --git a/tests/integrations/wsgi/test_wsgi.py b/tests/integrations/wsgi/test_wsgi.py index 084abe0745..eb73be1499 100644 --- a/tests/integrations/wsgi/test_wsgi.py +++ b/tests/integrations/wsgi/test_wsgi.py @@ -11,6 +11,7 @@ _ScopedResponse, get_request_url, ) +from tests.integrations.utils import DATA_COLLECTION_USER_INFO_CASES @pytest.fixture @@ -1303,45 +1304,7 @@ def dogpark(environ, start_response): assert "user.ip_address" not in child_span["attributes"] -# Parametrization shared by the user_info tests below. ``expect_ip`` is -# whether the client IP may be collected under the given init kwargs. -USER_INFO_INIT_KWARGS = [ - pytest.param({"send_default_pii": True}, True, id="legacy_send_default_pii_true"), - pytest.param( - {"send_default_pii": False}, False, id="legacy_send_default_pii_false" - ), - pytest.param( - {"_experiments": {"data_collection": {"user_info": True}}}, - True, - id="data_collection_user_info_true", - ), - pytest.param( - {"_experiments": {"data_collection": {"user_info": False}}}, - False, - id="data_collection_user_info_false", - ), - # ``data_collection`` is the single source of truth: it must win over - # ``send_default_pii`` when both are configured. - pytest.param( - { - "send_default_pii": True, - "_experiments": {"data_collection": {"user_info": False}}, - }, - False, - id="data_collection_wins_over_send_default_pii_true", - ), - pytest.param( - { - "send_default_pii": False, - "_experiments": {"data_collection": {"user_info": True}}, - }, - True, - id="data_collection_wins_over_send_default_pii_false", - ), -] - - -@pytest.mark.parametrize("init_kwargs, expect_ip", USER_INFO_INIT_KWARGS) +@pytest.mark.parametrize("init_kwargs, expect_ip", DATA_COLLECTION_USER_INFO_CASES) def test_user_info_span_attributes_data_collection( sentry_init, capture_items, init_kwargs, expect_ip ): @@ -1381,7 +1344,7 @@ def dogpark(environ, start_response): assert "client.address" not in server_span["attributes"] -@pytest.mark.parametrize("init_kwargs, expect_ip", USER_INFO_INIT_KWARGS) +@pytest.mark.parametrize("init_kwargs, expect_ip", DATA_COLLECTION_USER_INFO_CASES) def test_user_info_error_event_data_collection( sentry_init, crashing_app, capture_events, init_kwargs, expect_ip ):