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
24 changes: 15 additions & 9 deletions sentry_sdk/integrations/aiohttp.py
Original file line number Diff line number Diff line change
Expand Up @@ -159,11 +159,17 @@ async def sentry_app_handle(
header_value
)

url_query_attribute = (
{"url.query": request.query_string}
if request.query_string
else {}
)
url_attributes = {}
if should_send_default_pii():
url_attributes["url.full"] = "%s://%s%s" % (
request.scheme,
request.host,
request.path,
)
url_attributes["url.path"] = request.path

if request.query_string:
url_attributes["url.query"] = request.query_string

client_address_attributes = {}
if should_send_default_pii() and request.remote:
Expand All @@ -180,10 +186,8 @@ async def sentry_app_handle(
"sentry.op": OP.HTTP_SERVER,
"sentry.origin": AioHttpIntegration.origin,
"sentry.span.source": SegmentSource.ROUTE.value,
"url.full": "%s://%s%s"
% (request.scheme, request.host, request.path),
"http.request.method": request.method,
**url_query_attribute,
**url_attributes,
**client_address_attributes,
**header_attributes,
},
Expand Down Expand Up @@ -354,8 +358,10 @@ async def on_request_start(
"sentry.origin": AioHttpIntegration.origin,
"http.request.method": method,
}
if parsed_url is not None:
if parsed_url is not None and should_send_default_pii():
Comment thread
sentry-warden[bot] marked this conversation as resolved.
attributes["url.full"] = parsed_url.url
attributes["url.path"] = params.url.path

if parsed_url.query:
attributes["url.query"] = parsed_url.query
if parsed_url.fragment:
Expand Down
62 changes: 45 additions & 17 deletions tests/integrations/aiohttp/test_aiohttp.py
Original file line number Diff line number Diff line change
Expand Up @@ -1136,11 +1136,15 @@
assert event["exception"]["values"][0]["type"] == "ZeroDivisionError"


@pytest.mark.asyncio

Check warning on line 1139 in tests/integrations/aiohttp/test_aiohttp.py

View check run for this annotation

@sentry/warden / warden: code-review

[7HK-BDM] Parametrized `send_pii=False` path asserts nothing, leaving PII-leakage regression undetected (additional location)

The `send_pii=False` branch of this parametrized test only skips the positive assertions — it never asserts that `url.full`, `url.path`, and `url.query` are **absent** from `inner_client_span["attributes"]`, so a regression that leaks these attributes when PII is off would silently pass. Add an `else` clause mirroring the pattern used in `test_url_query_attribute_span_streaming` (e.g. `assert "url.full" not in inner_client_span["attributes"]`).
async def test_tracing_span_streaming(sentry_init, aiohttp_client, capture_items):
@pytest.mark.parametrize("send_pii", [True, False])
async def test_tracing_span_streaming(
sentry_init, aiohttp_client, capture_items, send_pii
):
sentry_init(
integrations=[AioHttpIntegration()],
traces_sample_rate=1.0,
send_default_pii=send_pii,
_experiments={"trace_lifecycle": "stream"},
)

Expand Down Expand Up @@ -1184,13 +1188,25 @@

# Request attributes derived directly from the aiohttp request.
assert server_span["attributes"]["http.request.method"] == "GET"
# client.address and user.ip_address is gated on send_default_pii (default False), so it must
# not be captured here.
assert "client.address" not in server_span["attributes"]
assert "user.ip_address" not in server_span["attributes"]
url_full = server_span["attributes"]["url.full"]
assert url_full.startswith("http://127.0.0.1:")
assert url_full.endswith("/")

if send_pii:
assert "client.address" in server_span["attributes"]
assert "user.ip_address" in server_span["attributes"]

url_full = server_span["attributes"]["url.full"]
assert url_full.startswith("http://127.0.0.1:")
assert url_full.endswith("/")

url_path = server_span["attributes"]["url.path"]
assert url_path == "/"
else:
assert "url.full" not in server_span["attributes"]
assert "url.path" not in server_span["attributes"]
assert "url.query" not in server_span["attributes"]
Comment thread
sentry-warden[bot] marked this conversation as resolved.

assert "client.address" not in server_span["attributes"]
assert "user.ip_address" not in server_span["attributes"]

# aiohttp's test client always sends a Host header; we assert it propagates
# into the span attributes via _filter_headers.
assert "http.request.header.host" in server_span["attributes"]
Expand Down Expand Up @@ -1280,12 +1296,14 @@


@pytest.mark.asyncio
@pytest.mark.parametrize("send_pii", [True, False])
async def test_url_query_attribute_span_streaming(
sentry_init, aiohttp_client, capture_items
sentry_init, aiohttp_client, capture_items, send_pii
):
sentry_init(
integrations=[AioHttpIntegration()],
traces_sample_rate=1.0,
send_default_pii=send_pii,
_experiments={"trace_lifecycle": "stream"},
)

Expand All @@ -1306,7 +1324,10 @@
assert len(items) == 2
server_segment, client_segment = [item.payload for item in items]

assert server_segment["attributes"]["url.query"] == "foo=bar&baz=qux"
if send_pii:
assert server_segment["attributes"]["url.query"] == "foo=bar&baz=qux"
else:
assert "url.query" not in server_segment["attributes"]


@pytest.mark.asyncio
Expand Down Expand Up @@ -1486,15 +1507,17 @@


@pytest.mark.asyncio
@pytest.mark.parametrize("send_pii", [True, False])

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.

Parametrized send_pii=False path has no assertions — the PII-off behavior is untested

The send_pii=False parametrized case runs with no assertions after line 1557, so the test won't catch a regression where URL attributes are still included when PII is disabled. Consider adding an else branch asserting url.full, url.path, and url.query are absent from inner_client_span["attributes"].

Evidence
  • The hunk at line 1505 adds @pytest.mark.parametrize("send_pii", [True, False]) to gate assertions.
  • The assertions from lines 1559–1569 are under if send_pii: with no else branch.
  • When send_pii=False, the test passes unconditionally after validating only http.request.method, http.response.status_code, and status.
  • A regression in aiohttp.py that leaves URL attributes on the span regardless of PII would not be caught by this test case.
Also found at 1 additional location
  • tests/integrations/aiohttp/test_aiohttp.py:1559-1571

Identified by Warden code-review · Q2F-VZM

async def test_outgoing_client_span_span_streaming(
sentry_init, aiohttp_raw_server, aiohttp_client, capture_items
sentry_init, aiohttp_raw_server, aiohttp_client, capture_items, send_pii
):
sentry_init(
integrations=[AioHttpIntegration()],
traces_sample_rate=1.0,
send_default_pii=send_pii,
_experiments={"trace_lifecycle": "stream"},
)

Check warning on line 1520 in tests/integrations/aiohttp/test_aiohttp.py

View check run for this annotation

@sentry/warden / warden: code-review

Parametrized `send_pii=False` path asserts nothing, leaving PII-leakage regression undetected

The `send_pii=False` branch of this parametrized test only skips the positive assertions — it never asserts that `url.full`, `url.path`, and `url.query` are **absent** from `inner_client_span["attributes"]`, so a regression that leaks these attributes when PII is off would silently pass. Add an `else` clause mirroring the pattern used in `test_url_query_attribute_span_streaming` (e.g. `assert "url.full" not in inner_client_span["attributes"]`).
async def handler(request):
return web.Response(text="OK")

Expand Down Expand Up @@ -1536,16 +1559,21 @@
assert inner_client_span["attributes"]["sentry.origin"] == "auto.http.aiohttp"
assert inner_client_span["attributes"]["http.request.method"] == "GET"
assert inner_client_span["attributes"]["http.response.status_code"] == 200
assert inner_client_span["attributes"]["url.query"] == "foo=bar"
assert inner_client_span["status"] == "ok"

url_full = inner_client_span["attributes"]["url.full"]
# parse_url() splits the URL — url.full is the base URL only, with the
# query string captured separately on url.query.
assert url_full.startswith("http://127.0.0.1:")
assert url_full.endswith("/")
if send_pii:
assert inner_client_span["attributes"]["url.query"] == "foo=bar"

url_full = inner_client_span["attributes"]["url.full"]

# parse_url() splits the URL — url.full is the base URL only, with the
# query string captured separately on url.query.
assert url_full.startswith("http://127.0.0.1:")
assert url_full.endswith("/")

assert inner_client_span["attributes"]["url.path"] == "/"


Check warning on line 1576 in tests/integrations/aiohttp/test_aiohttp.py

View check run for this annotation

@sentry/warden / warden: code-review

[7HK-BDM] Parametrized `send_pii=False` path asserts nothing, leaving PII-leakage regression undetected (additional location)

The `send_pii=False` branch of this parametrized test only skips the positive assertions — it never asserts that `url.full`, `url.path`, and `url.query` are **absent** from `inner_client_span["attributes"]`, so a regression that leaks these attributes when PII is off would silently pass. Add an `else` clause mirroring the pattern used in `test_url_query_attribute_span_streaming` (e.g. `assert "url.full" not in inner_client_span["attributes"]`).
@pytest.mark.asyncio
async def test_outgoing_trace_headers_span_streaming(
sentry_init, aiohttp_raw_server, aiohttp_client, capture_items
Expand Down
Loading