Skip to content
Draft
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
63 changes: 62 additions & 1 deletion src/sentry/api/endpoints/project_profiling_profile.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
from typing import Any

import orjson
from django.http import HttpResponse
import vroomrs

Check warning on line 4 in src/sentry/api/endpoints/project_profiling_profile.py

View check run for this annotation

@sentry/warden / warden: sentry-backend-bugs

vroomrs.decompress_profile_chunk exceptions not caught — corrupt chunk causes unhandled 500

At line 178, `vroomrs.decompress_profile_chunk(f.read())` is wrapped only in `except OSError`, but vroomrs (a PyO3 Rust extension) raises non-OSError exceptions on corrupt or invalid compressed data. A bad chunk in the object store will propagate uncaught and return a 500. Broaden the catch to `except (OSError, Exception)` or catch `Exception` and raise `Http404`.

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.

vroomrs.decompress_profile_chunk corrupt-data exception escapes narrow OSError catch (500)

In ProjectProfilingChunkAttachmentEndpoint.get, the stored chunk blob is decompressed via vroomrs.decompress_profile_chunk(f.read()) inside a try/except OSError (lines 176-180). OSError only covers I/O failures from storage.open/f.read(). If the stored chunk blob is corrupt, truncated, or otherwise malformed, the vroomrs Rust binding raises a non-OSError exception (e.g. ValueError or a binding-specific error), which is not caught here and propagates as an unhandled HTTP 500. The intended fallback (Http404) is bypassed. The codebase already treats vroomrs decode failures as broad exceptions: src/sentry/profiles/task.py wraps every vroomrs.profile_chunk_from_json_str(...)/profile_from_json_str(...) call in except Exception (lines 1410, 1480), confirming these bindings raise non-OSError errors on invalid input. Fix by catching the broader exception around the decode and raising Http404.

Evidence
  • Endpoint get (lines 176-180) wraps vroomrs.decompress_profile_chunk(f.read()) in try/except OSError: raise Http404, so only I/O errors map to 404.
  • src/sentry/profiles/task.py lines 1433/1480 and 1340/1410 wrap vroomrs decode calls in except Exception, showing the bindings raise non-OSError exceptions on malformed data.
  • A corrupt or truncated chunk blob in object storage (partial write, storage corruption) is a realistic input that reaches decompress_profile_chunk directly, so a non-OSError exception escapes to an unhandled 500.
Also found at 1 additional location
  • src/sentry/api/endpoints/project_profiling_profile.py:175-179

Identified by Warden sentry-backend-bugs · TUA-UAA

from django.http import Http404, HttpResponse, StreamingHttpResponse
from drf_spectacular.utils import OpenApiParameter, extend_schema
from rest_framework.request import Request
from rest_framework.response import Response
Expand All @@ -16,6 +17,7 @@
from sentry.apidocs.examples.profiling_examples import ProfilingExamples
from sentry.apidocs.parameters import GlobalParams
from sentry.apidocs.utils import inline_sentry_response_serializer
from sentry.models.files.utils import get_profiles_storage
from sentry.models.project import Project
from sentry.models.release import Release
from sentry.profiles.utils import get_from_profiling_service, proxy_profiling_service
Expand Down Expand Up @@ -141,3 +143,62 @@
"path": f"/organizations/{project.organization_id}/projects/{project.id}/raw_chunks/{profiler_id}/{chunk_id}",
}
return proxy_profiling_service(**kwargs)


@cell_silo_endpoint
class ProjectProfilingChunkAttachmentEndpoint(ProjectProfilingBaseEndpoint):
def get(
self,
request: Request,
project: Project,
profiler_id: str,
chunk_id: str,
attachment_name: str,
) -> Response | StreamingHttpResponse:
"""Download an attachment (e.g. a perfetto trace) of a profile chunk.

The chunk is loaded from the profiles object store and carries the list
of its attachments, each with a name, content type and the object store
id of the attached file. The client only supplies the profiler/chunk IDs
and the attachment name; everything else is resolved server-side.
"""
if not features.has(
"organizations:continuous-profiling", project.organization, actor=request.user
):
return Response(status=404)

storage = get_profiles_storage()

chunk_path = f"{project.organization_id}/{project.id}/{profiler_id}/{chunk_id}"
if not storage.exists(chunk_path):
raise Http404

try:
with storage.open(chunk_path) as f:
chunk = vroomrs.decompress_profile_chunk(f.read())
except OSError:

Check warning on line 179 in src/sentry/api/endpoints/project_profiling_profile.py

View check run for this annotation

@sentry/warden / warden: sentry-backend-bugs

[JYT-YHQ] vroomrs.decompress_profile_chunk exceptions not caught — corrupt chunk causes unhandled 500 (additional location)

At line 178, `vroomrs.decompress_profile_chunk(f.read())` is wrapped only in `except OSError`, but vroomrs (a PyO3 Rust extension) raises non-OSError exceptions on corrupt or invalid compressed data. A bad chunk in the object store will propagate uncaught and return a 500. Broaden the catch to `except (OSError, Exception)` or catch `Exception` and raise `Http404`.
raise Http404

attachment = next(
(a for a in chunk.get_attachments() if a.name == attachment_name),
None,
)
if attachment is None or not attachment.stored_id:
raise Http404

try:
fp = storage.open(attachment.stored_id)
except OSError:
raise Http404

def stream_attachment():
with fp:
while chunk := fp.read(4096):
yield chunk

response = StreamingHttpResponse(
stream_attachment(),
content_type=attachment.content_type or "application/octet-stream",
)
response["Content-Disposition"] = f'attachment; filename="{attachment.name}"'
return response
6 changes: 6 additions & 0 deletions src/sentry/api/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -813,6 +813,7 @@
from .endpoints.project_plugin_details import ProjectPluginDetailsEndpoint
from .endpoints.project_plugins import ProjectPluginsEndpoint
from .endpoints.project_profiling_profile import (
ProjectProfilingChunkAttachmentEndpoint,
ProjectProfilingProfileEndpoint,
ProjectProfilingRawChunkEndpoint,
ProjectProfilingRawProfileEndpoint,
Expand Down Expand Up @@ -3224,6 +3225,11 @@ def create_group_urls(name_prefix: str) -> list[URLPattern | URLResolver]:
ProjectProfilingRawChunkEndpoint.as_view(),
name="sentry-api-0-project-profiling-raw-chunk",
),
re_path(
r"^(?P<organization_id_or_slug>[^/]+)/(?P<project_id_or_slug>[^/]+)/profiling/chunks/(?P<profiler_id>(?:\d+|[A-Fa-f0-9-]{32,36}))/(?P<chunk_id>(?:\d+|[A-Fa-f0-9-]{32,36}))/attachments/(?P<attachment_name>[^/]+)/$",
ProjectProfilingChunkAttachmentEndpoint.as_view(),
name="sentry-api-0-project-profiling-chunk-attachment",
),
re_path(
r"^(?P<organization_id_or_slug>[^/]+)/(?P<project_id_or_slug>[^/]+)/statistical-detector/$",
ProjectStatisticalDetectors.as_view(),
Expand Down
5 changes: 3 additions & 2 deletions static/app/utils/api/knownSentryApiUrls.generated.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ export type KnownSentryApiUrls =
| '/organizations/$organizationIdOrSlug/events/'
| '/organizations/$organizationIdOrSlug/events/$projectIdOrSlug:$eventId/'
| '/organizations/$organizationIdOrSlug/events/anomalies/'
| '/organizations/$organizationIdOrSlug/events/validate/'
| '/organizations/$organizationIdOrSlug/explore/saved/'
| '/organizations/$organizationIdOrSlug/explore/saved/$id/'
| '/organizations/$organizationIdOrSlug/explore/saved/$id/starred/'
Expand Down Expand Up @@ -195,6 +196,7 @@ export type KnownSentryApiUrls =
| '/organizations/$organizationIdOrSlug/issue-view-title/generate/'
| '/organizations/$organizationIdOrSlug/issues-count/'
| '/organizations/$organizationIdOrSlug/issues-metrics/'
| '/organizations/$organizationIdOrSlug/issues-progress/'
| '/organizations/$organizationIdOrSlug/issues-stats/'
| '/organizations/$organizationIdOrSlug/issues-timeseries/'
| '/organizations/$organizationIdOrSlug/issues-with-supergroups/'
Expand Down Expand Up @@ -309,7 +311,6 @@ export type KnownSentryApiUrls =
| '/organizations/$organizationIdOrSlug/preprodartifacts/size-analysis/compare/$headArtifactId/$baseArtifactId/'
| '/organizations/$organizationIdOrSlug/preprodartifacts/snapshots/$snapshotId/'
| '/organizations/$organizationIdOrSlug/preprodartifacts/snapshots/$snapshotId/archive/'
| '/organizations/$organizationIdOrSlug/preprodartifacts/snapshots/$snapshotId/download/'
| '/organizations/$organizationIdOrSlug/preprodartifacts/snapshots/$snapshotId/images/$imageIdentifier/'
| '/organizations/$organizationIdOrSlug/preprodartifacts/snapshots/$snapshotId/recompare/'
| '/organizations/$organizationIdOrSlug/preprodartifacts/snapshots/latest-base/'
Expand Down Expand Up @@ -392,7 +393,6 @@ export type KnownSentryApiUrls =
| '/organizations/$organizationIdOrSlug/shortids/$issueId/'
| '/organizations/$organizationIdOrSlug/spans-samples/'
| '/organizations/$organizationIdOrSlug/spans/fields/'
| '/organizations/$organizationIdOrSlug/spans/fields/$key/values/'
| '/organizations/$organizationIdOrSlug/stats-summary/'
| '/organizations/$organizationIdOrSlug/stats/'
| '/organizations/$organizationIdOrSlug/stats_v2/'
Expand Down Expand Up @@ -500,6 +500,7 @@ export type KnownSentryApiUrls =
| '/projects/$organizationIdOrSlug/$projectIdOrSlug/preprodartifacts/snapshots/upload-options/'
| '/projects/$organizationIdOrSlug/$projectIdOrSlug/processing-errors/'
| '/projects/$organizationIdOrSlug/$projectIdOrSlug/processing-errors/$uuid/'
| '/projects/$organizationIdOrSlug/$projectIdOrSlug/profiling/chunks/$profilerId/$chunkId/attachments/$attachmentName/'
| '/projects/$organizationIdOrSlug/$projectIdOrSlug/profiling/profiles/$profileId/'
| '/projects/$organizationIdOrSlug/$projectIdOrSlug/profiling/raw_chunks/$profilerId/$chunkId/'
| '/projects/$organizationIdOrSlug/$projectIdOrSlug/profiling/raw_profiles/$profileId/'
Expand Down
159 changes: 159 additions & 0 deletions tests/sentry/api/endpoints/test_project_profiling_profile.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
from io import BytesIO
from typing import Any
from unittest.mock import Mock, patch
from uuid import uuid4

import vroomrs

from sentry.testutils.cases import APITestCase

PROFILING_FEATURES = {"organizations:profiling": True}
Expand All @@ -14,3 +19,157 @@ def setUp(self) -> None:
def test_feature_flag_disabled(self) -> None:
response = self.get_response(self.project.organization.slug, self.project.id, str(uuid4()))
assert response.status_code == 404


class ProjectProfilingChunkAttachmentTest(APITestCase):
endpoint = "sentry-api-0-project-profiling-chunk-attachment"

def setUp(self) -> None:
self.login_as(user=self.user)
self.profiler_id = uuid4().hex
self.chunk_id = uuid4().hex
self.chunk_path = (
f"{self.organization.id}/{self.project.id}/{self.profiler_id}/{self.chunk_id}"
)

def get_attachment_response(self, attachment_name: str = "raw_profile") -> Any:
return self.get_response(
self.organization.slug,
self.project.slug,
self.profiler_id,
self.chunk_id,
attachment_name,
)

def make_chunk(self, attachments: list[vroomrs.Attachment]) -> Mock:
chunk = Mock()
chunk.get_attachments.return_value = attachments
return chunk

def make_storage(self, chunk_exists: bool, stored_files: dict[str, bytes]) -> Mock:
storage = Mock()
storage.exists.return_value = chunk_exists

def open_(path: str) -> BytesIO:
if path == self.chunk_path and chunk_exists:
return BytesIO(b"compressed-chunk")
if path in stored_files:
return BytesIO(stored_files[path])
raise OSError(f"no such object: {path}")

storage.open.side_effect = open_
return storage

def test_feature_flag_disabled(self) -> None:
response = self.get_attachment_response()
assert response.status_code == 404

@patch("sentry.api.endpoints.project_profiling_profile.vroomrs")
@patch("sentry.api.endpoints.project_profiling_profile.get_profiles_storage")
def test_download(self, mock_get_storage: Mock, mock_vroomrs: Mock) -> None:
storage = self.make_storage(True, {"aef123345": b"raw-profile-bytes"})
mock_get_storage.return_value = storage
mock_vroomrs.decompress_profile_chunk.return_value = self.make_chunk(
[
vroomrs.Attachment(
name="raw_profile",
content_type="application/x-perfetto-trace",
stored_id="aef123345",
)
]
)

with self.feature("organizations:continuous-profiling"):
response = self.get_attachment_response()

assert response.status_code == 200
assert b"".join(response.streaming_content) == b"raw-profile-bytes"
assert response["Content-Type"] == "application/x-perfetto-trace"
assert response["Content-Disposition"] == 'attachment; filename="raw_profile"'
storage.exists.assert_called_once_with(self.chunk_path)

@patch("sentry.api.endpoints.project_profiling_profile.vroomrs")
@patch("sentry.api.endpoints.project_profiling_profile.get_profiles_storage")
def test_download_without_content_type_falls_back_to_octet_stream(
self, mock_get_storage: Mock, mock_vroomrs: Mock
) -> None:
mock_get_storage.return_value = self.make_storage(True, {"aef123345": b"trace"})
mock_vroomrs.decompress_profile_chunk.return_value = self.make_chunk(
[vroomrs.Attachment(name="raw_profile", content_type=None, stored_id="aef123345")]
)

with self.feature("organizations:continuous-profiling"):
response = self.get_attachment_response()

assert response.status_code == 200
assert response["Content-Type"] == "application/octet-stream"

@patch("sentry.api.endpoints.project_profiling_profile.vroomrs")
@patch("sentry.api.endpoints.project_profiling_profile.get_profiles_storage")
def test_unknown_attachment_name_returns_404(
self, mock_get_storage: Mock, mock_vroomrs: Mock
) -> None:
mock_get_storage.return_value = self.make_storage(True, {"aef123345": b"trace"})
mock_vroomrs.decompress_profile_chunk.return_value = self.make_chunk(
[
vroomrs.Attachment(
name="raw_profile",
content_type="application/x-perfetto-trace",
stored_id="aef123345",
)
]
)

with self.feature("organizations:continuous-profiling"):
response = self.get_attachment_response("some_other_attachment")

assert response.status_code == 404

@patch("sentry.api.endpoints.project_profiling_profile.vroomrs")
@patch("sentry.api.endpoints.project_profiling_profile.get_profiles_storage")
def test_chunk_without_attachments_returns_404(
self, mock_get_storage: Mock, mock_vroomrs: Mock
) -> None:
mock_get_storage.return_value = self.make_storage(True, {})
mock_vroomrs.decompress_profile_chunk.return_value = self.make_chunk([])

with self.feature("organizations:continuous-profiling"):
response = self.get_attachment_response()

assert response.status_code == 404

@patch("sentry.api.endpoints.project_profiling_profile.vroomrs")
@patch("sentry.api.endpoints.project_profiling_profile.get_profiles_storage")
def test_missing_chunk_returns_404(self, mock_get_storage: Mock, mock_vroomrs: Mock) -> None:
storage = self.make_storage(False, {})
mock_get_storage.return_value = storage

with self.feature("organizations:continuous-profiling"):
response = self.get_attachment_response()

assert response.status_code == 404
storage.open.assert_not_called()
mock_vroomrs.decompress_profile_chunk.assert_not_called()

@patch("sentry.api.endpoints.project_profiling_profile.vroomrs")
@patch("sentry.api.endpoints.project_profiling_profile.get_profiles_storage")
def test_missing_attachment_file_returns_404(
self, mock_get_storage: Mock, mock_vroomrs: Mock
) -> None:
# The attachment is referenced by the chunk but the file is gone from
# the object store.
mock_get_storage.return_value = self.make_storage(True, {})
mock_vroomrs.decompress_profile_chunk.return_value = self.make_chunk(
[
vroomrs.Attachment(
name="raw_profile",
content_type="application/x-perfetto-trace",
stored_id="aef123345",
)
]
)

with self.feature("organizations:continuous-profiling"):
response = self.get_attachment_response()

assert response.status_code == 404
Loading