Skip to content
Open
60 changes: 59 additions & 1 deletion sentry_sdk/data_collection.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@
"""

import warnings
from typing import TYPE_CHECKING, cast
from typing import TYPE_CHECKING, List, Mapping, Optional, cast

from sentry_sdk._types import SENSITIVE_DATA_SUBSTITUTE

if TYPE_CHECKING:
from typing import Any, Dict, Literal
Expand Down Expand Up @@ -77,6 +79,62 @@
]


def _is_sensitive_key(key: str, extra_terms: "Optional[List[str]]" = None) -> bool:
"""
Return whether ``key`` matches the sensitive denylist using a partial,
case-insensitive substring match.

:param extra_terms: additional deny terms (e.g. user-provided) to consider
alongside the built-in `_SENSITIVE_DENYLIST`.
"""
lowered = key.lower()
for term in _SENSITIVE_DENYLIST:
if term in lowered:
return True
if extra_terms:
for term in extra_terms:
if term and term.lower() in lowered:
return True
return False


def _apply_key_value_collection_filtering(
items: "Mapping[str, Any]",
behaviour: "KeyValueCollectionBehaviour",
substitute: "Any" = SENSITIVE_DATA_SUBSTITUTE,
) -> "Dict[str, Any]":

if behaviour["mode"] == "off":
return {}

Comment thread
sentry-warden[bot] marked this conversation as resolved.
result: "Dict[str, Any]" = {}

if behaviour["mode"] == "allowlist":
for key, value in items.items():
is_allowed = False
if isinstance(key, str):
lowered = key.lower()
is_allowed = any(
term and term.lower() in lowered
for term in behaviour.get("terms", [])
)
if is_allowed and not _is_sensitive_key(key):
result[key] = value
else:
result[key] = substitute
return result

# denylist behaviour
for key, value in items.items():
if isinstance(key, str) and _is_sensitive_key(
key=key, extra_terms=behaviour.get("terms", [])
):
result[key] = substitute
else:
result[key] = value
return result


def _map_from_send_default_pii(
*,
send_default_pii: bool,
Expand Down
14 changes: 10 additions & 4 deletions sentry_sdk/integrations/_asgi_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,13 @@ def _get_request_data(
if ty in ("http", "websocket"):
request_data["method"] = asgi_scope.get("method")

request_data["headers"] = headers = _filter_headers(
_get_headers(asgi_scope),
headers = _get_headers(asgi_scope)

request_data["headers"] = _filter_headers(
headers,
use_annotated_value=False,
Comment on lines +118 to +122

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was done because when if the headers are filtered with "allowlist" and no terms are provided, and the "host" header is present, then the constructed URL in _get_url below would contain [Filtered] within the URL.

Doing this ensures that it gets filtered from the headers to respect data collection, but still correctly appears in the url

)

request_data["query_string"] = _get_query(asgi_scope)

request_data["url"] = _get_url(
Expand Down Expand Up @@ -148,8 +152,10 @@ def _get_request_attributes(
if asgi_scope.get("method"):
attributes["http.request.method"] = asgi_scope["method"].upper()

headers = _filter_headers(_get_headers(asgi_scope), use_annotated_value=False)
for header, value in headers.items():
headers = _get_headers(asgi_scope)

filtered_headers = _filter_headers(headers, use_annotated_value=False)
for header, value in filtered_headers.items():
attributes[f"http.request.header.{header.lower()}"] = value

if should_send_default_pii():
Expand Down
45 changes: 33 additions & 12 deletions sentry_sdk/integrations/_wsgi_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@

import sentry_sdk
from sentry_sdk._types import SENSITIVE_DATA_SUBSTITUTE
from sentry_sdk.data_collection import _apply_key_value_collection_filtering
Comment thread
ericapisani marked this conversation as resolved.
from sentry_sdk.scope import should_send_default_pii
from sentry_sdk.utils import AnnotatedValue, logger
from sentry_sdk.utils import AnnotatedValue, has_data_collection_enabled, logger

Check warning on line 9 in sentry_sdk/integrations/_wsgi_common.py

View check run for this annotation

@sentry/warden / warden: find-bugs

IP headers (X-Forwarded-For / X-Real-Ip) leak when data_collection is enabled with default header denylist

When a user supplies an explicit `data_collection` config, `_filter_headers` routes header scrubbing through `_apply_key_value_collection_filtering`. If `http_headers.request` is omitted, `_http_headers_from_value` resolves it to `{"mode": "denylist"}` with no extra terms, so only the built-in `_SENSITIVE_DENYLIST` applies. That denylist contains none of the terms needed to match `x-forwarded-for` or `x-real-ip`, so those IP-address headers (PII) pass through unfiltered. This is a regression relative to the `send_default_pii`-derived mapping (`_map_from_send_default_pii`), which — when PII is off — explicitly adds the terms `["forwarded", "-ip", "remote-", "via", "-user"]` and the legacy `SENSITIVE_HEADERS` path which scrubs `X_FORWARDED_FOR` and `X_REAL_IP`. A user who enables `data_collection` for an unrelated purpose (e.g. cookies) inadvertently starts sending client IP headers that were previously scrubbed.

try:
from django.http.request import RawPostDataException
Expand Down Expand Up @@ -213,19 +214,39 @@
headers: "Mapping[str, str]",
use_annotated_value: bool = True,
) -> "Mapping[str, Union[AnnotatedValue, str]]":
if should_send_default_pii():
return headers
client_options = sentry_sdk.get_client().options

substitute: "Union[AnnotatedValue, str]" = (
SENSITIVE_DATA_SUBSTITUTE
if not use_annotated_value
else AnnotatedValue.removed_because_over_size_limit()
)
if has_data_collection_enabled(client_options):
data_collection_configuration = client_options["data_collection"]

filtered = _apply_key_value_collection_filtering(
items=headers,
behaviour=data_collection_configuration["http_headers"]["request"],
)

for key in filtered:
if isinstance(key, str) and key.lower() in ("cookie", "set-cookie"):
filtered[key] = SENSITIVE_DATA_SUBSTITUTE

return {
k: (v if k.upper().replace("-", "_") not in SENSITIVE_HEADERS else substitute)
for k, v in headers.items()
}
return filtered
else:
if should_send_default_pii():
return headers

substitute: "Union[AnnotatedValue, str]" = (
SENSITIVE_DATA_SUBSTITUTE
if not use_annotated_value
else AnnotatedValue.removed_because_over_size_limit()
)

return {
k: (
v
if k.upper().replace("-", "_") not in SENSITIVE_HEADERS
else substitute
)
for k, v in headers.items()
}


def _in_http_status_code_range(
Expand Down
3 changes: 2 additions & 1 deletion sentry_sdk/integrations/aiohttp.py
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,8 @@ async def sentry_app_handle(

header_attributes: "dict[str, Any]" = {}
for header, header_value in _filter_headers(
headers, use_annotated_value=False
headers,
use_annotated_value=False,
).items():
header_attributes[
f"http.request.header.{header.lower()}"
Expand Down
177 changes: 177 additions & 0 deletions tests/integrations/aiohttp/test_aiohttp.py
Original file line number Diff line number Diff line change
Expand Up @@ -1258,8 +1258,185 @@ async def hello(request):
)


@pytest.mark.parametrize(
"options,expected",
[
pytest.param(
{
"send_default_pii": True,
"data_collection": None,
},
{
"authorization": "[Filtered]",
"custom": "foobar",
"cookie": "[Filtered]",
},
id="enabled_send_default_pii_redacts_auth_header_due_to_data_collection_default_settings",
),
pytest.param(
{
"send_default_pii": False,
"data_collection": None,
},
{
"authorization": "[Filtered]",
"custom": "foobar",
"cookie": "[Filtered]",
},
id="disabled_send_default_pii_redacts_auth_header_due_to_data_collection_default_settings",
),
pytest.param(
{
"send_default_pii": False,
"data_collection": {"http_headers": {"request": {"mode": "off"}}},
},
None,
id="data_collection_off_does_not_add_headers",
),
pytest.param(
{
"send_default_pii": False,
"data_collection": {"http_headers": {"request": {"mode": "allowlist"}}},
},
{
"authorization": "[Filtered]",
"custom": "[Filtered]",
"cookie": "[Filtered]",
},
id="data_collection_allow_list_redacts_terms_that_do_not_appear",
),
pytest.param(
{
"send_default_pii": False,
"data_collection": {
"http_headers": {
"request": {"mode": "allowlist", "terms": ["Authorization"]}
}
},
},
{
"authorization": "[Filtered]",
"custom": "[Filtered]",
"cookie": "[Filtered]",
},
id="data_collection_allow_list_redacts_sensitive_terms_even_when_provided_by_user",
),
pytest.param(
{
"send_default_pii": False,
"data_collection": {
"http_headers": {
"request": {"mode": "allowlist", "terms": ["custom"]}
}
},
},
{
"authorization": "[Filtered]",
"custom": "foobar",
"cookie": "[Filtered]",
},
id="data_collection_allow_list_does_not_redact_provided_term",
),
pytest.param(
{
"send_default_pii": False,
"data_collection": {
"http_headers": {
"request": {"mode": "denylist", "terms": ["custom"]}
}
},
},
{
"authorization": "[Filtered]",
"custom": "[Filtered]",
"cookie": "[Filtered]",
},
id="data_collection_deny_list_redacts_sensitive_terms_when_provided_by_user",
),
pytest.param(
{
"send_default_pii": False,
"data_collection": {
"http_headers": {
"request": {"mode": "allowlist", "terms": ["cookie"]}
}
},
},
{
"authorization": "[Filtered]",
"custom": "[Filtered]",
"cookie": "[Filtered]",
},
id="data_collection_cookie_is_always_redacted_even_when_allow_listed",
),
],
)
@pytest.mark.asyncio
async def test_sensitive_header_passthrough_with_pii_span_streaming(
sentry_init, aiohttp_client, capture_items, options, expected, request
):
sentry_init(
integrations=[AioHttpIntegration()],
traces_sample_rate=1.0,
send_default_pii=options["send_default_pii"],
_experiments={
"trace_lifecycle": "stream",
"data_collection": options["data_collection"],
},
)

async def hello(request):
return web.Response(text="hello")

app = web.Application()
app.router.add_get("/", hello)

items = capture_items("span")

client = await aiohttp_client(app)
await client.get(
"/",
headers={
"Authorization": "Bearer secret-token",
"x-custom-header": "foobar",
"Cookie": "sessionid=secret",
},
)

sentry_sdk.flush()

server_span, _client_segment = [item.payload for item in items]

if request.node.callspec.id.endswith("data_collection_off_does_not_add_headers"):
assert "http.request.header.authorization" not in server_span["attributes"]
assert "http.request.header.cookie" not in server_span["attributes"]
else:
assert (
server_span["attributes"]["http.request.header.authorization"]
== expected["authorization"]
)
assert (
server_span["attributes"]["http.request.header.x-custom-header"]
== expected["custom"]
)
assert (
server_span["attributes"]["http.request.header.cookie"]
== expected["cookie"]
)

# client.address and user.ip_address is captured under send_default_pii=True.
# TODO: This block will eventually need to be removed from this test into a separate
# test once data collection gating is introduced on these values
if options["send_default_pii"]:
assert server_span["attributes"]["client.address"] == "127.0.0.1"
assert server_span["attributes"]["user.ip_address"] == "127.0.0.1"
else:
assert "user.ip_address" not in server_span["attributes"]
assert "client.address" not in server_span["attributes"]


@pytest.mark.asyncio
async def test_sensitive_header_passthrough_with_pii_span_streaming_without_data_collection(
sentry_init, aiohttp_client, capture_items
):
sentry_init(
Expand Down
Loading
Loading