fix(pyreqwest): Gate url.full, url.query, url.fragment on send_default_pii - #6673
fix(pyreqwest): Gate url.full, url.query, url.fragment on send_default_pii#6673ericapisani merged 3 commits into
Conversation
Sorry, something went wrong.
…t_pii Gate URL attributes behind should_send_default_pii() in the streamed span path to avoid leaking sensitive URL data. Update tests to parametrize on send_default_pii and assert attributes are present only when PII sending is enabled. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sorry, something went wrong.
Import from sentry_sdk.scope instead of build.lib.sentry_sdk.scope. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Non-streaming test assertions always expect URL/query/fragment, regardless of send_default_pii
In test_sync_client_spans, the non-streaming else branch unconditionally asserts span['data']['url'], HTTP_QUERY, and HTTP_FRAGMENT are present even when send_default_pii=False, so the test suite will not catch PII leaking through the non-streaming path.
Evidence- test_sync_client_spans is parametrized with send_default_pii in [True, False].
- The streaming if span_streaming: branch gates assertions with if send_default_pii: ... else: assert 'url.full' not in ....
- The non-streaming else: branch has no such conditional: it always asserts span['data']['url'] == ..., span['data'][SPANDATA.HTTP_QUERY] == 'q=test', and span['data'][SPANDATA.HTTP_FRAGMENT] == 'frag' for both PII settings.
- This means the send_default_pii=False parametrized case passes even though PII is being leaked, giving false confidence.
Identified by Warden code-review
Sorry, something went wrong.
| }, | ||
| ) as span: | ||
| if parsed_url is not None: | ||
| if parsed_url is not None and should_send_default_pii(): |
There was a problem hiding this comment.
Bug: The non-streamed request path in the pyreqwest integration does not respect the should_send_default_pii() setting, leading to potential PII leakage in URL data.
Severity: HIGH
Add the if should_send_default_pii(): check around the lines that set url, HTTP_QUERY, and HTTP_FRAGMENT data in the non-streamed path (lines 131-137), mirroring the implementation in the streamed path.
Prompt for AI AgentReview 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/pyreqwest.py#L99 Potential issue: The pyreqwest integration has two code paths for creating spans: a streamed path and a non-streamed path. The pull request adds a check for `should_send_default_pii()` to prevent leaking URL data in the streamed path. However, the same check is missing from the non-streamed path (lines 131-137). As a result, when `send_default_pii` is set to `False`, the non-streamed path will still send `url`, `HTTP_QUERY`, and `HTTP_FRAGMENT` data, which may contain personally identifiable information (PII), contrary to the user's configuration.
Did we get this right? 👍 / 👎 to inform future reviews.
Sorry, something went wrong.
Codecov Results 📊✅ 89953 passed | ⏭️ 6240 skipped | Total: 96193 | Pass Rate: 93.51% | Execution Time: 316m 9s 📊 Comparison with Base Branch
All tests are passing successfully. ✅ Patch coverage is 100.00%. Project has 2395 uncovered lines. @@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 89.93% 89.93% —%
==========================================
Files 192 192 —
Lines 23784 23785 +1
Branches 8210 8210 —
==========================================
+ Hits 21389 21390 +1
- Misses 2395 2395 —
- Partials 1342 1342 —Generated by Codecov Action |
Sorry, something went wrong.
…t_pii (getsentry#6673) ## Summary - Gate `url.full`, `url.query`, and `url.fragment` span attributes behind `should_send_default_pii()` in the streamed span path - Update tests to parametrize on `send_default_pii` and assert attributes are present/absent accordingly Fixes PY-2560 Fixes getsentry#6671 --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Fixes PY-2560
Fixes #6671