Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions sentry_sdk/_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ class DataCollectionUserOptions(TypedDict, total=False):
cookies: "KeyValueCollectionBehaviour"
http_headers: "HttpHeadersCollectionUserOptions"
http_bodies: "List[str]"
query_params: "KeyValueCollectionBehaviour"
url_query_params: "KeyValueCollectionBehaviour"
graphql: "GraphQLCollectionUserOptions"
gen_ai: "GenAICollectionUserOptions"
database_query_data: bool
Expand All @@ -197,7 +197,7 @@ class DataCollection(TypedDict):
cookies: "KeyValueCollectionBehaviour"
http_headers: "HttpHeadersCollectionBehaviour"
http_bodies: "List[str]"
query_params: "KeyValueCollectionBehaviour"
url_query_params: "KeyValueCollectionBehaviour"
graphql: "GraphQLCollectionBehaviour"
gen_ai: "GenAICollectionBehaviour"
database_query_data: bool
Expand Down
36 changes: 28 additions & 8 deletions sentry_sdk/data_collection.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

``data_collection`` supersedes the single ``send_default_pii`` boolean with a
structured configuration that lets users enable or restrict automatically
collected data by category (user identity, cookies, HTTP headers, query params,
collected data by category (user identity, cookies, HTTP headers, URL query params,
HTTP bodies, generative AI inputs/outputs, stack frame variables, source
context).

Expand All @@ -23,12 +23,13 @@
"""

import warnings
from typing import TYPE_CHECKING, List, Mapping, Optional, cast
from typing import TYPE_CHECKING, List, Mapping, Optional, Union, cast
from urllib.parse import parse_qs

from sentry_sdk._types import SENSITIVE_DATA_SUBSTITUTE

if TYPE_CHECKING:
from typing import Any, Dict, Literal
from typing import Any, Dict

from sentry_sdk._types import (
DataCollection,
Expand All @@ -50,7 +51,7 @@
# Default number of source lines captured above and below a stack frame.
_DEFAULT_FRAME_CONTEXT_LINES = 5

# Collection modes for key-value data (cookies, headers, query params).
# Collection modes for key-value data (cookies, headers, URL query params).
# snake_case (Python-only deviation from the spec's camelCase); never
# serialized to Sentry.
_VALID_KEY_VALUE_COLLECTION_BEHAVIOUR_MODES = ("off", "denylist", "allowlist")
Expand Down Expand Up @@ -98,6 +99,26 @@
return False


def _apply_data_collection_filtering_to_query_string(
query_string: str,
behaviour: "KeyValueCollectionBehaviour",
) -> "Union[str, None]":
parsed_qs = parse_qs(query_string, keep_blank_values=True)
filtered_qs = _apply_key_value_collection_filtering(
items=parsed_qs, behaviour=behaviour
)

if filtered_qs:
parts = []
for key, value in filtered_qs.items():
values = value if isinstance(value, list) else [value]
for item in values:
parts.append(f"{key}={item}")

Check warning on line 116 in sentry_sdk/data_collection.py

View check run for this annotation

@sentry/warden / warden: find-bugs

[SYF-5NQ] Filtered query string is reassembled without URL-encoding, corrupting values and re-revealing scrubbed data (additional location)

`_apply_data_collection_filtering_to_query_string` parses the query string with `parse_qs`, which percent-decodes keys and values and splits on `&`/`=`, then rebuilds the string with plain `f"{key}={item}"` joined by `&` without re-encoding. Decoded values containing `&`/`=` split into extra apparent params, so a sensitive value that was scrubbed to `[Filtered]` under one key can reappear intact when it was embedded (URL-encoded) inside another kept key's value. Benign percent-encoded values (e.g. `q=hello%20world`) are also silently corrupted in the reported query string.
return "&".join(parts)

Check warning on line 118 in sentry_sdk/data_collection.py

View check run for this annotation

@sentry/warden / warden: code-review

Query string reconstruction drops percent-encoding, corrupting values

parse_qs decodes percent-encoded query values, but reconstruction with f"{key}={item}" does not re-encode them, so characters like `&`, `=`, spaces, or `%` in the original values produce a malformed/ambiguous query string. Consider using urlencode() to re-encode the filtered pairs.
return None
Comment thread
ericapisani marked this conversation as resolved.


def _apply_key_value_collection_filtering(
items: "Mapping[str, Any]",
behaviour: "KeyValueCollectionBehaviour",
Expand Down Expand Up @@ -146,13 +167,12 @@
``send_default_pii`` collects today. Used when ``data_collection`` is not
provided explicitly.
"""
kv_mode: "Literal['denylist', 'off']" = "denylist" if send_default_pii else "off"
terms = [] if send_default_pii else ["forwarded", "-ip", "remote-", "via", "-user"]

return {
"provided_by_user": False,
"user_info": send_default_pii,
"cookies": {"mode": kv_mode, "terms": terms},
"cookies": {"mode": "denylist", "terms": terms},
# Headers are collected in both PII modes today (sensitive ones filtered
# when PII is off), so this never maps to "off".
"http_headers": {
Expand All @@ -161,7 +181,7 @@
# Bodies are collected regardless of PII today, bounded by
# ``max_request_body_size``.
"http_bodies": list(_ALL_HTTP_BODY_TYPES),
"query_params": {"mode": kv_mode, "terms": terms},
"url_query_params": {"mode": "denylist", "terms": terms},
"graphql": {"document": send_default_pii, "variables": send_default_pii},
"gen_ai": {"inputs": send_default_pii, "outputs": send_default_pii},
"database_query_data": send_default_pii,
Expand Down Expand Up @@ -211,7 +231,7 @@
"cookies": _kvcb_from_value(d.get("cookies") or {}),
"http_headers": _http_headers_from_value(d.get("http_headers") or {}),
"http_bodies": http_bodies,
"query_params": _kvcb_from_value(d.get("query_params") or {}),
"url_query_params": _kvcb_from_value(d.get("url_query_params") or {}),
"graphql": _graphql_from_value(d.get("graphql") or {}),
"gen_ai": _gen_ai_from_value(d.get("gen_ai") or {}),
"database_query_data": d.get("database_query_data", True),
Expand Down
37 changes: 35 additions & 2 deletions sentry_sdk/integrations/wsgi.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from sentry_sdk._werkzeug import _get_headers, get_host
from sentry_sdk.api import continue_trace
from sentry_sdk.consts import OP, SPANDATA
from sentry_sdk.data_collection import _apply_data_collection_filtering_to_query_string
from sentry_sdk.integrations._wsgi_common import (
DEFAULT_HTTP_METHODS_TO_CAPTURE,
_filter_headers,
Expand All @@ -19,6 +20,7 @@
ContextVar,
capture_internal_exceptions,
event_from_exception,
has_data_collection_enabled,
nullcontext,
reraise,
)
Expand Down Expand Up @@ -355,6 +357,7 @@
method = environ.get("REQUEST_METHOD")
env = dict(_get_environ(environ))
headers = _filter_headers(dict(_get_headers(environ)))
client_options = sentry_sdk.get_client().options

def event_processor(event: "Event", hint: "Dict[str, Any]") -> "Event":
with capture_internal_exceptions():
Expand All @@ -367,11 +370,22 @@
user_info.setdefault("ip_address", client_ip)

request_info["url"] = request_url
request_info["query_string"] = query_string
request_info["method"] = method
request_info["env"] = env
request_info["headers"] = headers

if has_data_collection_enabled(client_options):
if query_string:
filtered_qs = _apply_data_collection_filtering_to_query_string(
query_string=query_string,
behaviour=client_options["data_collection"]["url_query_params"],
)

Check warning on line 382 in sentry_sdk/integrations/wsgi.py

View check run for this annotation

@sentry/warden / warden: find-bugs

Filtered query string is reassembled without URL-encoding, corrupting values and re-revealing scrubbed data

`_apply_data_collection_filtering_to_query_string` parses the query string with `parse_qs`, which percent-decodes keys and values and splits on `&`/`=`, then rebuilds the string with plain `f"{key}={item}"` joined by `&` without re-encoding. Decoded values containing `&`/`=` split into extra apparent params, so a sensitive value that was scrubbed to `[Filtered]` under one key can reappear intact when it was embedded (URL-encoded) inside another kept key's value. Benign percent-encoded values (e.g. `q=hello%20world`) are also silently corrupted in the reported query string.
if filtered_qs:
request_info["query_string"] = filtered_qs

Check warning on line 384 in sentry_sdk/integrations/wsgi.py

View check run for this annotation

@sentry/warden / warden: code-review

[376-W8H] Query string reconstruction drops percent-encoding, corrupting values (additional location)

parse_qs decodes percent-encoded query values, but reconstruction with f"{key}={item}" does not re-encode them, so characters like `&`, `=`, spaces, or `%` in the original values produce a malformed/ambiguous query string. Consider using urlencode() to re-encode the filtered pairs.
else:
# This was not originally gated so if data collection is not enabled, leave as-is.
request_info["query_string"] = query_string

return event

return event_processor
Expand Down Expand Up @@ -409,8 +423,27 @@
except ValueError:
pass

if should_send_default_pii():
client_options = sentry_sdk.get_client().options

if has_data_collection_enabled(client_options):
query_string = environ.get("QUERY_STRING")
if query_string:
filtered_qs = _apply_data_collection_filtering_to_query_string(
query_string=query_string,
behaviour=client_options["data_collection"]["url_query_params"],
)

if filtered_qs:
attributes["http.query"] = filtered_qs

path = environ.get("PATH_INFO", "")
if path:
attributes["url.path"] = path

attributes["url.full"] = get_request_url(environ, use_x_forwarded_for)

elif should_send_default_pii():
Comment thread
ericapisani marked this conversation as resolved.
client_ip = get_client_ip(environ)

Check warning on line 446 in sentry_sdk/integrations/wsgi.py

View check run for this annotation

@sentry/warden / warden: find-bugs

client.address span attribute never set when data_collection is enabled

When `data_collection` is configured, the `if has_data_collection_enabled(...)` branch is taken and never adds `client.address`, so client IP is silently dropped on spans even when `user_info` is enabled — a regression from the `should_send_default_pii()` path which set it.
if client_ip:
attributes["client.address"] = client_ip

Expand Down
181 changes: 180 additions & 1 deletion tests/integrations/django/test_data_scrubbing.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import pytest
from werkzeug.test import Client

import sentry_sdk
from sentry_sdk.consts import SPANDATA
from sentry_sdk.integrations.django import DjangoIntegration
from tests.conftest import werkzeug_set_cookie
from tests.conftest import unpack_werkzeug_response, werkzeug_set_cookie
from tests.integrations.django.myapp.wsgi import application
from tests.integrations.django.utils import pytest_mark_django_db_decorator

Expand All @@ -14,6 +16,9 @@

NO_COOKIES = object()

# Sentinel: the query string (event) / ``http.query`` attribute (span) is absent.
NO_QUERY_STRING = object()


@pytest.fixture
def client():
Expand Down Expand Up @@ -217,3 +222,177 @@ def test_data_collection_cookies_precedence_over_send_default_pii(
"csrftoken": "[Filtered]",
"foo": "bar",
}


# 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"


@pytest.mark.forked
@pytest_mark_django_db_decorator()
@pytest.mark.parametrize(
"init_kwargs, expected_query_string",
[
pytest.param(
{"send_default_pii": True},
"toy=tennisball&color=red&auth=secret",
id="legacy_send_default_pii_true",
),
pytest.param(
{"send_default_pii": False},
"toy=tennisball&color=red&auth=secret",
id="legacy_send_default_pii_false",
),
pytest.param(
{"_experiments": {"data_collection": {}}},
"toy=tennisball&color=red&auth=[Filtered]",
id="data_collection_denylist_default",
),
pytest.param(
{
"_experiments": {
"data_collection": {
"url_query_params": {"mode": "allowlist", "terms": ["toy"]}
}
}
},
"toy=tennisball&color=[Filtered]&auth=[Filtered]",
id="data_collection_allowlist",
),
pytest.param(
{
"_experiments": {
"data_collection": {"url_query_params": {"mode": "off"}}
}
},
NO_QUERY_STRING,
id="data_collection_off",
),
],
)
def test_query_string_data_collection(
sentry_init,
client,
capture_events,
init_kwargs,
expected_query_string,
):
sentry_init(integrations=[DjangoIntegration()], **init_kwargs)
events = capture_events()

client.get(reverse("view_exc") + "?" + QUERY_STRING)

(event,) = events

if expected_query_string is NO_QUERY_STRING:
assert "query_string" not in event["request"]
else:
assert event["request"]["query_string"] == expected_query_string


@pytest.mark.forked
@pytest_mark_django_db_decorator()
@pytest.mark.parametrize(
"init_kwargs, expected_query",
[
pytest.param(
{"send_default_pii": True},
"toy=tennisball&color=red&auth=secret",
id="legacy_send_default_pii_true",
),
pytest.param(
{"send_default_pii": False},
NO_QUERY_STRING,
id="legacy_send_default_pii_false",
),
pytest.param(
{"_experiments": {"data_collection": {}}},
"toy=tennisball&color=red&auth=[Filtered]",
id="data_collection_denylist_default",
),
pytest.param(
{
"_experiments": {
"data_collection": {
"url_query_params": {"mode": "allowlist", "terms": ["toy"]}
}
}
},
"toy=tennisball&color=[Filtered]&auth=[Filtered]",
id="data_collection_allowlist",
),
pytest.param(
{
"_experiments": {
"data_collection": {"url_query_params": {"mode": "off"}}
}
},
NO_QUERY_STRING,
id="data_collection_off",
),
],
)
def test_span_http_query_data_collection(
sentry_init,
client,
capture_items,
init_kwargs,
expected_query,
):
sentry_init(
integrations=[DjangoIntegration()],
traces_sample_rate=1.0,
_experiments={
"trace_lifecycle": "stream",
**init_kwargs.pop("_experiments", {}),
},
**init_kwargs,
)

items = capture_items("span")

unpack_werkzeug_response(client.get(reverse("message") + "?" + QUERY_STRING))

sentry_sdk.flush()

spans = [item.payload for item in items]
(root_span,) = (span for span in spans if span["name"] == "/message")

if expected_query is NO_QUERY_STRING:
assert SPANDATA.HTTP_QUERY not in root_span["attributes"]
else:
assert root_span["attributes"][SPANDATA.HTTP_QUERY] == expected_query


@pytest.mark.forked
@pytest_mark_django_db_decorator()
def test_query_string_empty_legacy_emits_empty_string(
sentry_init, client, capture_events
):
sentry_init(integrations=[DjangoIntegration()], send_default_pii=True)
events = capture_events()

client.get(reverse("view_exc"))

(event,) = events
assert event["request"]["query_string"] == ""


@pytest.mark.forked
@pytest_mark_django_db_decorator()
def test_empty_query_string_is_dropped_with_data_collection(
sentry_init, client, capture_events
):
# ``data_collection`` path: an empty query string is dropped entirely to
# reduce envelope size, so the ``query_string`` key is absent.
sentry_init(
integrations=[DjangoIntegration()],
_experiments={"data_collection": {}},
)
events = capture_events()

client.get(reverse("view_exc"))

(event,) = events
assert "query_string" not in event["request"]
Loading
Loading