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
43 changes: 36 additions & 7 deletions src/sentry/api/endpoints/project_trace_item_details.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import time
import uuid
from datetime import timedelta
from typing import Any, Literal

import sentry_sdk
Expand All @@ -17,8 +18,10 @@
from sentry.api.base import cell_silo_endpoint
from sentry.api.bases.project import ProjectEndpoint
from sentry.api.exceptions import BadRequest
from sentry.api.utils import get_date_range_from_params
from sentry.auth.staff import is_active_staff
from sentry.auth.superuser import is_active_superuser
from sentry.exceptions import InvalidParams
from sentry.models.project import Project
from sentry.search.eap import constants
from sentry.search.eap.types import (
Expand All @@ -36,8 +39,10 @@
translate_search_type_for_internal_column,
translate_to_sentry_conventions,
)
from sentry.search.utils import InvalidQuery, parse_datetime_string
from sentry.snuba.referrer import Referrer
from sentry.utils import json, snuba_rpc
from sentry.utils import json
from sentry.utils.snuba_rpc import trace_item_details_rpc

_NUMERIC_COERCIONS: dict[str, type] = {"valFloat": float, "valDouble": float}
_VAL_TYPE_TO_COLUMN_TYPE: dict[str, ColumnType] = {
Expand Down Expand Up @@ -362,9 +367,31 @@ def get(request: Request, project: Project, item_id: str) -> Response:
if not serializer.is_valid():
return Response(serializer.errors, status=400)

try:
start, end = get_date_range_from_params(request.GET, optional=True)
except InvalidParams:
return Response("date range parameters invalid", status=400)
if "timestamp" in request.GET:
try:
example_timestamp = parse_datetime_string(request.GET["timestamp"])
except InvalidQuery:
return Response("timestamp parameter invalid", status=400)
time_buffer = 1.5
example_start = example_timestamp - timedelta(days=time_buffer)
example_end = example_timestamp + timedelta(days=time_buffer)
if start is not None:
start = max(start, example_start)
else:
start = example_start
if end is not None:
end = min(end, example_end)
else:
end = example_end
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.

Intersection of non-overlapping ranges yields inverted timestamps

Medium Severity

When both timestamp and date range params (e.g. statsPeriod) are provided but their resulting intervals don't overlap, the intersection logic (max for start, min for end) produces start > end. This inverted range is then passed directly to FromDatetime and sent to the RPC without validation, which could cause an error or undefined query behavior downstream.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 228016a. Configure here.


serialized = serializer.validated_data
trace_id = serialized.get("trace_id")
item_type = serialized.get("item_type")
sentry_sdk.set_tag("trace_item_details.item_type", item_type)
referrer = serialized.get("referrer", Referrer.API_ORGANIZATION_TRACE_ITEM_DETAILS.value)

trace_item_type = None
Expand All @@ -377,12 +404,14 @@ def get(request: Request, project: Project, item_id: str) -> Response:
raise BadRequest(detail=f"Unknown trace item type: {item_type}")

start_timestamp_proto = ProtoTimestamp()
start_timestamp_proto.FromSeconds(0)

end_timestamp_proto = ProtoTimestamp()

# due to clock drift, the end time can be in the future - add a week to be safe
end_timestamp_proto.FromSeconds(int(time.time()) + 60 * 60 * 24 * 7)
if start is not None and end is not None:
start_timestamp_proto.FromDatetime(start)
end_timestamp_proto.FromDatetime(end)
else:
start_timestamp_proto.FromSeconds(0)
# due to clock drift, the end time can be in the future - add a week to be safe
end_timestamp_proto.FromSeconds(int(time.time()) + 60 * 60 * 24 * 7)

trace_id = request.GET.get("trace_id")
if not trace_id:
Expand All @@ -403,7 +432,7 @@ def get(request: Request, project: Project, item_id: str) -> Response:
trace_id=trace_id,
)

resp = MessageToDict(snuba_rpc.trace_item_details_rpc(req))
resp = MessageToDict(trace_item_details_rpc(req))

use_sentry_conventions = features.has(
"organizations:performance-sentry-conventions-fields",
Expand Down
135 changes: 130 additions & 5 deletions tests/snuba/api/endpoints/test_project_trace_item_details.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import uuid
from datetime import timedelta
from unittest import mock

import pytest
Expand Down Expand Up @@ -32,7 +33,7 @@ def setUp(self) -> None:
self.one_min_ago = before_now(minutes=1)
self.trace_uuid = str(uuid.uuid4()).replace("-", "")

def do_request(self, event_type: str, item_id: str, features=None):
def do_request(self, event_type: str, item_id: str, extra_data=None, features=None):
item_details_url = reverse(
"sentry-api-0-project-trace-item-details",
kwargs={
Expand All @@ -43,13 +44,16 @@ def do_request(self, event_type: str, item_id: str, features=None):
)
if features is None:
features = self.features
data = {
"item_type": event_type,
"trace_id": self.trace_uuid,
}
if extra_data is not None:
data.update(extra_data)
with self.feature(features):
return self.client.get(
item_details_url,
{
"item_type": event_type,
"trace_id": self.trace_uuid,
},
data,
)

def test_simple(self) -> None:
Expand Down Expand Up @@ -639,3 +643,124 @@ def test_attachment(self) -> None:
"meta": {},
"timestamp": mock.ANY,
}

def test_with_timestamp(self) -> None:
log = self.create_ourlog(
{
"body": "foo",
"trace_id": self.trace_uuid,
},
attributes={
"str_attr": {
"string_value": "1",
},
"int_attr": {"int_value": 2},
"float_attr": {
"double_value": 3.0,
},
"bool_attr": {
"bool_value": True,
},
},
timestamp=self.one_min_ago,
)
self.store_eap_items([log])
item_id = log.item_id.hex()

for extra_data in [
{"timestamp": self.one_min_ago.isoformat()},
{"statsPeriod": "24h"},
]:
trace_details_response = self.do_request("logs", item_id, extra_data=extra_data)

assert trace_details_response.status_code == 200, trace_details_response.content

timestamp_nanos = int(self.one_min_ago.timestamp() * 1_000_000_000)
assert trace_details_response.data["attributes"] == [
{"name": "tags[bool_attr,boolean]", "type": "bool", "value": True},
{"name": "tags[float_attr,number]", "type": "float", "value": 3.0},
{
"name": "observed_timestamp",
"type": "int",
"value": str(timestamp_nanos),
},
{"name": "project_id", "type": "int", "value": str(self.project.id)},
{"name": "severity_number", "type": "int", "value": "0"},
{"name": "tags[int_attr,number]", "type": "int", "value": "2"},
{
"name": "timestamp_precise",
"type": "int",
"value": str(timestamp_nanos),
},
{"name": "message", "type": "str", "value": "foo"},
{"name": "severity", "type": "str", "value": "INFO"},
{"name": "str_attr", "type": "str", "value": "1"},
{"name": "trace", "type": "str", "value": self.trace_uuid},
]
assert trace_details_response.data["itemId"] == item_id
assert (
trace_details_response.data["timestamp"]
== self.one_min_ago.replace(microsecond=0, tzinfo=None).isoformat() + "Z"
)

def test_with_incorrect_timestamp(self) -> None:
log = self.create_ourlog(
{
"body": "foo",
"trace_id": self.trace_uuid,
},
attributes={
"str_attr": {
"string_value": "1",
},
"int_attr": {"int_value": 2},
"float_attr": {
"double_value": 3.0,
},
"bool_attr": {
"bool_value": True,
},
},
timestamp=self.one_min_ago,
)
self.store_eap_items([log])
item_id = log.item_id.hex()

for extra_data in [
{"timestamp": (self.one_min_ago - timedelta(days=30)).isoformat()},
{"statsPeriodEnd": "24h", "statsPeriodStart": "48h"},
]:
trace_details_response = self.do_request("logs", item_id, extra_data=extra_data)

assert trace_details_response.status_code == 404, trace_details_response.content

def test_with_invalid_timestamp(self) -> None:
log = self.create_ourlog(
{
"body": "foo",
"trace_id": self.trace_uuid,
},
attributes={
"str_attr": {
"string_value": "1",
},
"int_attr": {"int_value": 2},
"float_attr": {
"double_value": 3.0,
},
"bool_attr": {
"bool_value": True,
},
},
timestamp=self.one_min_ago,
)
self.store_eap_items([log])
item_id = log.item_id.hex()

for extra_data in [
{"timestamp": "beepboop"},
{"statsPeriod": "hello"},
]:
trace_details_response = self.do_request("logs", item_id, extra_data=extra_data)

assert trace_details_response.status_code == 400, trace_details_response.content
Loading