fix(tornado): Gate url.full, url.path, url.query on send_default_pii - #6664
fix(tornado): Gate url.full, url.path, url.query on send_default_pii#6664ericapisani merged 1 commit into
Conversation
Sorry, something went wrong.
Sorry, something went wrong.
| "data": {"heyoo": [""]}, | ||
| "url": "http://{host}/hi".format(host=host), | ||
| } | ||
| if send_pii: | ||
| expected_request["cookies"] = {} | ||
| assert server_tx["request"] == expected_request | ||
|
|
||
| assert ( | ||
| client_tx["contexts"]["trace"]["trace_id"] |
There was a problem hiding this comment.
Bug: The query_string is always included in the legacy event payload, regardless of the send_default_pii setting, potentially leaking PII.
Severity: HIGH
In the _make_event_processor function, wrap the assignment to request_info["query_string"] in a conditional that checks if should_send_default_pii() is true. Additionally, update the corresponding test case to assert that query_string is not present in the expected_request when send_pii is False.
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: tests/integrations/tornado/test_tornado.py#L249-L257 Potential issue: The pull request correctly gates the `url.query` attribute for span streaming behind the `send_default_pii` setting. However, for the legacy (non-streaming) event format, the `query_string` is unconditionally added to the event's request payload in the `_make_event_processor` function. This happens even when `send_default_pii` is `False`, leading to a potential PII leak. The test suite for the legacy path fails to assert that the `query_string` is absent when PII is disabled, which is why this issue was not caught. This creates an inconsistency where span streaming correctly redacts PII, but legacy events do not.
Did we get this right? 👍 / 👎 to inform future reviews.
Sorry, something went wrong.
Codecov Results 📊✅ 89929 passed | ⏭️ 6240 skipped | Total: 96169 | Pass Rate: 93.51% | Execution Time: 322m 35s 📊 Comparison with Base Branch
All tests are passing successfully. ✅ Patch coverage is 100.00%. Project has 2401 uncovered lines. @@ Coverage Diff @@
## main #PR +/-##
==========================================
- Coverage 89.93% 89.91% -0.02%
==========================================
Files 192 192 —
Lines 23784 23786 +2
Branches 8210 8212 +2
==========================================
+ Hits 21389 21385 -4
- Misses 2395 2401 +6
- Partials 1342 1340 -2Generated by Codecov Action |
Sorry, something went wrong.
…etsentry#6664) Gates `url.full`, `url.path`, and `url.query` span attributes in the Tornado integration behind `send_default_pii`, consistent with the same fix applied to the aiohttp (getsentry#6650) and wsgi (getsentry#6654) integrations. Adds a `send_pii` parametrize dimension to the transactions test to cover both the PII-on and PII-off paths explicitly. Fixes PY-2556 Fixes getsentry#6661 Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Gates url.full, url.path, and url.query span attributes in the Tornado integration behind send_default_pii, consistent with the same fix applied to the aiohttp (#6650) and wsgi (#6654) integrations.
Adds a send_pii parametrize dimension to the transactions test to cover both the PII-on and PII-off paths explicitly.
Fixes PY-2556
Fixes #6661