fix(asgi): Gate query string and client IP behind send_default_pii#6501
fix(asgi): Gate query string and client IP behind send_default_pii#6501ericapisani wants to merge 4 commits into
Conversation
Move http.query and client.address attribute collection inside the should_send_default_pii() check so sensitive values are not captured by default. Fixes PY-2514 Fixes #6499
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 666e6fb. Configure here.
Codecov Results 📊✅ 88436 passed | ⏭️ 6022 skipped | Total: 94458 | Pass Rate: 93.62% | Execution Time: 298m 12s 📊 Comparison with Base Branch
All tests are passing successfully. ✅ Patch coverage is 100.00%. Project has 2481 uncovered lines. Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
- Coverage 89.38% 89.35% -0.03%
==========================================
Files 192 192 —
Lines 23285 23286 +1
Branches 8002 8004 +2
==========================================
+ Hits 20811 20805 -6
- Misses 2474 2481 +7
- Partials 1309 1309 —Generated by Codecov Action |
|
If Warden is right about
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit eb14dd0. Configure here.
| asgi_scope, "http" if ty == "http" else "ws", headers.get("host") | ||
| ) | ||
| query_string = _get_query(asgi_scope) | ||
| attributes["url.full"] = f"{url_without_query_string}?{query_string}" |
There was a problem hiding this comment.
Empty query yields bogus url.full
Medium Severity
When send_default_pii is enabled, url.full is always built as f"{base}?{query_string}" even if the request has no query string. _get_query returns None in that case, so url.full becomes the base URL with a literal ?None suffix instead of the path-only URL.
Reviewed by Cursor Bugbot for commit eb14dd0. Configure here.
| query_string = _get_query(asgi_scope) | ||
| attributes["url.full"] = f"{url_without_query_string}?{query_string}" | ||
|
|
||
| client = asgi_scope.get("client") | ||
| if client and should_send_default_pii(): |
There was a problem hiding this comment.
Bug: The code unconditionally appends ?{query_string} to the URL, resulting in a malformed URL like .../path?None when no query string is present.
Severity: MEDIUM
Suggested Fix
Conditionally append the query string to the URL only if it has a value. For example: url = url_without_query_string; if query_string: url = f'{url}?{query_string}'; attributes['url.full'] = url.
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/_asgi_common.py#L132-L136
Potential issue: The `_get_query` function returns `None` when an ASGI request lacks a
query string. The code then constructs the `url.full` attribute using an f-string,
`f"{url_without_query_string}?{query_string}"`. When `query_string` is `None`, this
results in a malformed URL with a literal `?None` appended (e.g.,
`http://localhost/path?None`). This bug is triggered for any request without query
parameters when `send_default_pii` is enabled, which is a common scenario, and corrupts
the `url.full` attribute sent to Sentry.


Move http.query and client.address attribute collection inside the
should_send_default_pii() check so sensitive values are not captured
by default.
Fixes PY-2514
Fixes #6499