-
Notifications
You must be signed in to change notification settings - Fork 635
fix(httpx2): Gate url.full, url.query on send_default_pii #6670
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, | ||
|
|
@@ -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
|
||
| 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: The non-streaming path for Suggested FixWrap the Prompt for AI AgentAlso affects:
Did we get this right? 👍 / 👎 to inform future reviews. |
||
|
|
@@ -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 | ||
|
|
||
There was a problem hiding this comment.
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-streamingelsebranch — which is the DEFAULT code path since streaming requires opting in via_experiments.trace_lifecycle == "stream"— still callsspan.set_data("url", parsed_url.url),span.set_data(SPANDATA.HTTP_QUERY, parsed_url.query), andspan.set_data(SPANDATA.HTTP_FRAGMENT, parsed_url.fragment)gated only onparsed_url is not None. As a result, query parameters and credentials embedded in URLs are still recorded in spans for most users even whensend_default_pii=False, so the PR does not achieve its stated goal on the default path. The same gap exists in both_install_httpx2_clientand_install_httpx2_async_client.Evidence
has_span_streaming_enabledintracing_utils.pyreturns true only when_experiments.trace_lifecycle == "stream", so theelse(non-streaming) branch is the default path for most users._install_httpx2_client'selsebranch (httpx2.py:122-126)span.set_data("url", ...),span.set_data(SPANDATA.HTTP_QUERY, ...), andspan.set_data(SPANDATA.HTTP_FRAGMENT, ...)are gated only onparsed_url is not None, with noshould_send_default_pii()check.should_send_default_pii()guard, confirming the gate was applied to only one of the two paths._install_httpx2_async_client'selsebranch (httpx2.py:232-235) repeats the same ungatedset_datacalls.Identified by Warden code-review · PJJ-2ZL