Skip to content
Merged
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
5 changes: 3 additions & 2 deletions sentry_sdk/integrations/httpx2.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import sentry_sdk
from sentry_sdk.consts import OP, SPANDATA
from sentry_sdk.integrations import DidNotEnable, Integration
from sentry_sdk.scope import should_send_default_pii
from sentry_sdk.tracing import BAGGAGE_HEADER_NAME
from sentry_sdk.tracing_utils import (
add_http_request_source,
Expand Down Expand Up @@ -70,10 +71,10 @@
"sentry.origin": Httpx2Integration.origin,
"http.request.method": request.method,
},
) as streamed_span:

Check failure on line 74 in sentry_sdk/integrations/httpx2.py

View check run for this annotation

@sentry/warden / warden: code-review

PII gate not applied to default non-streaming path — URL, query, and fragment still exposed

The `should_send_default_pii()` guard added in this PR only protects the experimental span-streaming path (`is_span_streaming_enabled`). The non-streaming `else` branch — which is the DEFAULT code path since streaming requires opting in via `_experiments.trace_lifecycle == "stream"` — still calls `span.set_data("url", parsed_url.url)`, `span.set_data(SPANDATA.HTTP_QUERY, parsed_url.query)`, and `span.set_data(SPANDATA.HTTP_FRAGMENT, parsed_url.fragment)` gated only on `parsed_url is not None`. As a result, query parameters and credentials embedded in URLs are still recorded in spans for most users even when `send_default_pii=False`, so the PR does not achieve its stated goal on the default path. The same gap exists in both `_install_httpx2_client` and `_install_httpx2_async_client`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PII gate not applied to default non-streaming path — URL, query, and fragment still exposed

The should_send_default_pii() guard added in this PR only protects the experimental span-streaming path (is_span_streaming_enabled). The non-streaming else branch — which is the DEFAULT code path since streaming requires opting in via _experiments.trace_lifecycle == "stream" — still calls span.set_data("url", parsed_url.url), span.set_data(SPANDATA.HTTP_QUERY, parsed_url.query), and span.set_data(SPANDATA.HTTP_FRAGMENT, parsed_url.fragment) gated only on parsed_url is not None. As a result, query parameters and credentials embedded in URLs are still recorded in spans for most users even when send_default_pii=False, so the PR does not achieve its stated goal on the default path. The same gap exists in both _install_httpx2_client and _install_httpx2_async_client.

Evidence
  • has_span_streaming_enabled in tracing_utils.py returns true only when _experiments.trace_lifecycle == "stream", so the else (non-streaming) branch is the default path for most users.
  • In _install_httpx2_client's else branch (httpx2.py:122-126) span.set_data("url", ...), span.set_data(SPANDATA.HTTP_QUERY, ...), and span.set_data(SPANDATA.HTTP_FRAGMENT, ...) are gated only on parsed_url is not None, with no should_send_default_pii() check.
  • The streaming branch above (httpx2.py:77) does add the should_send_default_pii() guard, confirming the gate was applied to only one of the two paths.
  • _install_httpx2_async_client's else branch (httpx2.py:232-235) repeats the same ungated set_data calls.

Identified by Warden code-review · PJJ-2ZL

attributes: "Attributes" = {}

if parsed_url is not None:
if parsed_url is not None and should_send_default_pii():
attributes["url.full"] = parsed_url.url
if parsed_url.query:
attributes["url.query"] = parsed_url.query
Comment on lines 74 to 80

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The non-streaming path for httpx2 requests unconditionally adds URL data to spans, ignoring the send_default_pii setting.
Severity: MEDIUM

Suggested Fix

Wrap the span.set_data calls for URL and query parameters in the non-streaming paths (both sync and async) with a if should_send_default_pii(): check. This will ensure consistent PII handling across both streaming and non-streaming modes.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: sentry_sdk/integrations/httpx2.py#L74-L80

Potential issue: The change to gate URL data behind `should_send_default_pii()` was only
applied to the streaming span path in the `httpx2` integration. The non-streaming path,
for both synchronous and asynchronous requests, still unconditionally sets
`span.set_data("url", ...)` and `span.set_data(SPANDATA.HTTP_QUERY, ...)` without
checking the PII setting. This means that even if a user configures
`send_default_pii=False` to prevent sending sensitive information like query parameters,
this data will still be sent for non-streaming `httpx` requests, leading to a potential
PII leak.

Also affects:

  • sentry_sdk/integrations/httpx2.py:184~190

Did we get this right? 👍 / 👎 to inform future reviews.

Expand Down Expand Up @@ -183,7 +184,7 @@
) as streamed_span:
attributes: "Attributes" = {}

if parsed_url is not None:
if parsed_url is not None and should_send_default_pii():
attributes["url.full"] = parsed_url.url
if parsed_url.query:
attributes["url.query"] = parsed_url.query
Expand Down
37 changes: 37 additions & 0 deletions tests/integrations/httpx2/test_httpx2.py
Original file line number Diff line number Diff line change
Expand Up @@ -1123,6 +1123,7 @@ def test_http_url_attributes_span_streaming(
sentry_init(
integrations=[Httpx2Integration()],
traces_sample_rate=1.0,
send_default_pii=True,
_experiments={"trace_lifecycle": "stream"},
)

Expand Down Expand Up @@ -1158,6 +1159,7 @@ def test_http_url_attributes_no_query_or_fragment_span_streaming(
sentry_init(
integrations=[Httpx2Integration()],
traces_sample_rate=1.0,
send_default_pii=True,
_experiments={"trace_lifecycle": "stream"},
)

Expand All @@ -1179,3 +1181,38 @@ def test_http_url_attributes_no_query_or_fragment_span_streaming(
assert "url.query" not in http_span["attributes"]
assert "url.fragment" not in http_span["attributes"]
assert http_span["attributes"]["http.response.status_code"] == 200


@pytest.mark.parametrize(
"httpx2_client",
(httpx2.Client(), httpx2.AsyncClient()),
)
def test_http_url_attributes_pii_disabled_span_streaming(
sentry_init, capture_items, httpx2_client, httpx2_mock
):
httpx2_mock.add_response()

sentry_init(
integrations=[Httpx2Integration()],
traces_sample_rate=1.0,
_experiments={"trace_lifecycle": "stream"},
)

items = capture_items("span")

url = "http://example.com/?foo=bar#frag"

if asyncio.iscoroutinefunction(httpx2_client.get):
asyncio.run(httpx2_client.get(url))
else:
httpx2_client.get(url)

sentry_sdk.flush()

http_span = _get_http_client_span(items)

assert http_span["attributes"]["http.request.method"] == "GET"
assert "url.full" not in http_span["attributes"]
assert "url.query" not in http_span["attributes"]
assert "url.fragment" not in http_span["attributes"]
assert http_span["attributes"]["http.response.status_code"] == 200
Loading