feat(attachments): Redirect Objectstore-backed attachment downloads to Objectstore

src/sentry/api/endpoints/event_attachment_details.py

@@ -1,7 +1,7 @@
 import posixpath
 
 import sentry_sdk
-from django.http import StreamingHttpResponse
+from django.http import HttpResponseRedirect, StreamingHttpResponse
 from drf_spectacular.utils import OpenApiParameter, extend_schema
 from rest_framework.request import Request
 from rest_framework.response import Response
@@ -44,7 +44,9 @@
     description=(
         "If this parameter is present, the response will be a binary file download "
         "instead of JSON metadata. The value does not matter — any value (including "
-        "empty) triggers the download."
+        "empty) triggers the download. Depending on where the attachment is stored, "
+        "the response may be a redirect to the storage service, so clients must follow "
+        "redirects."
     ),
 )
 
@@ -87,7 +89,12 @@ class EventAttachmentDetailsEndpoint(ProjectEndpoint):
     }
     permission_classes = (EventAttachmentDetailsPermission,)
 
-    def download(self, attachment: EventAttachment, request: Request) -> StreamingHttpResponse:
+    def download(
+        self, attachment: EventAttachment, request: Request
+    ) -> HttpResponseRedirect | StreamingHttpResponse:
+        if attachment.uses_objectstore():
+            return HttpResponseRedirect(attachment.get_objectstore_presigned_url(request))
+
         name = posixpath.basename(" ".join(attachment.name.split()))
         accept_encoding = parse_accept_encoding(request.headers.get("Accept-Encoding", ""))
         blob_stream = attachment.get_blob_stream(accept_encoding)
@@ -131,6 +138,7 @@ def get(
         Response[EventAttachmentSerializerResponse]
         | Response[None]
         | Response[DetailResponse]
+        | HttpResponseRedirect
         | StreamingHttpResponse
     ):
         """

src/sentry/models/eventattachment.py

@@ -13,6 +13,7 @@
 from django.db import models
 from django.db.models.expressions import DatabaseDefault
 from django.db.models.functions import Now
+from django.http import HttpRequest
 from django.utils import timezone
 from objectstore_client import TimeToLive
 
@@ -22,7 +23,11 @@
 from sentry.db.models.fields.bounded import BoundedIntegerField
 from sentry.db.models.manager.base_query_set import BaseQuerySet
 from sentry.models.files.utils import get_size_and_checksum, get_storage
-from sentry.objectstore import default_attachment_retention, get_attachments_session
+from sentry.objectstore import (
+    default_attachment_retention,
+    get_attachments_session,
+    get_download_redirect_url,
+)
 from sentry.objectstore.metrics import measure_storage_operation
 from sentry.options.rollout import in_random_rollout
 
@@ -159,6 +164,28 @@ def delete(self, *args: Any, **kwargs: Any) -> tuple[int, dict[str, int]]:
 
         return rv
 
+    def uses_objectstore(self) -> bool:
+        """Whether this attachment's payload is stored in Objectstore."""
+        return self.blob_path is not None and self.blob_path.startswith(V2_PREFIX)
+
+    def get_objectstore_presigned_url(self, request: HttpRequest) -> str:
+        """
+        Returns the URL that `request` should be redirected to in order to download this
+        attachment directly from Objectstore.
+
+        This function should only be called if this attachment is Objectstore-backed, it
+        will raise an exception otherwise.
+        """
+        if not self.uses_objectstore():
+            raise ValueError("attachment is not stored in Objectstore")
+        assert self.blob_path is not None
+
+        organization_id = _get_organization(self.project_id)
+        session = get_attachments_session(organization_id, self.project_id)
+        return get_download_redirect_url(
+            request, session, organization_id, self.blob_path.removeprefix(V2_PREFIX)
+        )
+
     def getfile(self) -> IO[bytes]:
         if not self.blob_path:
             return BytesIO(b"")
@@ -195,7 +222,8 @@ def get_blob_stream(self, accept_encoding: list[str]) -> BlobStream:
         bytes can be transferred directly to the client. For all other blob types
         (inline, V1), delegates to :meth:`getfile`.
         """
-        if self.blob_path and self.blob_path.startswith(V2_PREFIX):
+        if self.uses_objectstore():
+            assert self.blob_path is not None
             key = self.blob_path.removeprefix(V2_PREFIX)
             session = get_attachments_session(_get_organization(self.project_id), self.project_id)
             response = session.get(key, accept_encoding=accept_encoding or None)

tests/sentry/api/endpoints/test_event_attachment_details.py

@@ -1,4 +1,7 @@
+from unittest.mock import patch
+
 import pytest
+import requests
 from django.test import override_settings
 
 from sentry.attachments.base import CachedAttachment
@@ -74,7 +77,11 @@ def test_simple(self) -> None:
     def test_download(self) -> None:
         self.login_as(user=self.user)
 
-        self.create_attachment()
+        # Attachments not backed by Objectstore are streamed through Sentry.
+        with override_options({"objectstore.enable_for.attachments": 0}):
+            self.create_attachment()
+
+        assert not self.attachment.uses_objectstore()
         path1 = f"/api/0/projects/{self.organization.slug}/{self.project.slug}/events/{self.event.event_id}/attachments/{self.attachment.id}/?download"
 
         response = self.client.get(path1)
@@ -87,54 +94,47 @@ def test_download(self) -> None:
 
     @with_feature("organizations:event-attachments")
     @requires_objectstore
-    def test_download_objectstore(self) -> None:
+    def test_download_objectstore_redirects_internal_to_objectstore(self) -> None:
         self.login_as(user=self.user)
 
         with override_options({"objectstore.enable_for.attachments": 1}):
             attachment = self.create_attachment()
-
-            assert attachment.blob_path is not None
-            assert attachment.blob_path.startswith("v2/")
-
-            path1 = f"/api/0/projects/{self.organization.slug}/{self.project.slug}/events/{self.event.event_id}/attachments/{attachment.id}/?download"
-            response = self.client.get(path1)
-
-            assert response.status_code == 200, response.content
-            assert response.get("Content-Disposition") == 'attachment; filename="hello.png"'
-            assert response.get("Content-Length") == str(attachment.size)
-            assert response.get("Content-Type") == "image/png"
-            assert close_streaming_response(response) == ATTACHMENT_CONTENT
+            assert attachment.uses_objectstore()
+
+            path = f"/api/0/projects/{self.organization.slug}/{self.project.slug}/events/{self.event.event_id}/attachments/{attachment.id}/?download"
+            with patch("sentry.auth.system.is_internal_ip", return_value=True):
+                response = self.client.get(path)
+
+            assert response.status_code == 302
+            location = response["Location"]
+            # Internal callers are redirected straight to Objectstore, not through the cell proxy.
+            assert "/organizations/" not in location
+            # In dev/test the host may be rewritten to the Docker-internal `objectstore`
+            # hostname (so Symbolicator can reach it); rewrite it back so we can follow it.
+            downloaded = requests.get(location.replace("://objectstore:", "://127.0.0.1:"))
+            assert downloaded.status_code == 200, downloaded.text
+            assert downloaded.content == ATTACHMENT_CONTENT
 
     @with_feature("organizations:event-attachments")
     @requires_objectstore
-    def test_download_objectstore_accept_encoding(self) -> None:
-        import zstandard
-
+    def test_download_objectstore_redirects_external_to_cell_proxy(self) -> None:
         self.login_as(user=self.user)
 
         with override_options({"objectstore.enable_for.attachments": 1}):
             attachment = self.create_attachment()
+            assert attachment.uses_objectstore()
 
-            assert attachment.blob_path is not None
-            assert attachment.blob_path.startswith("v2/")
-
-            path1 = f"/api/0/projects/{self.organization.slug}/{self.project.slug}/events/{self.event.event_id}/attachments/{attachment.id}/?download"
-            response = self.client.get(path1, HTTP_ACCEPT_ENCODING="zstd")
+            path = f"/api/0/projects/{self.organization.slug}/{self.project.slug}/events/{self.event.event_id}/attachments/{attachment.id}/?download"
+            with patch("sentry.auth.system.is_internal_ip", return_value=False):
+                response = self.client.get(path)
 
-            assert response.status_code == 200, response.content
-            assert response.get("Content-Disposition") == 'attachment; filename="hello.png"'
-            assert response.get("Content-Type") == "image/png"
-
-            body = close_streaming_response(response)
-            if response.get("Content-Encoding") == "zstd":
-                # Object was stored compressed; verify we got valid compressed bytes
-                dctx = zstandard.ZstdDecompressor()
-                with dctx.stream_reader(body) as reader:
-                    assert reader.read() == ATTACHMENT_CONTENT
-            else:
-                # Object was stored uncompressed; content-length should be present
-                assert response.get("Content-Length") == str(attachment.size)
-                assert body == ATTACHMENT_CONTENT
+            assert response.status_code == 302
+            location = response["Location"]
+            assert (
+                f"/organizations/{self.organization.id}/objectstore/v1/objects/attachments/"
+                in location
+            )
+            assert "os_auth=" in location
 
     @with_feature("organizations:event-attachments")
     def test_zero_sized_attachment(self) -> None:
@@ -207,7 +207,10 @@ def test_delete_activity_with_group(self) -> None:
 class EventAttachmentDetailsPermissionTest(PermissionTestCase, CreateAttachmentMixin):
     def setUp(self) -> None:
         super().setUp()
-        self.create_attachment()
+        # `PermissionTestCase` treats any 3xx as "access denied", so keep the attachment on
+        # the streaming path rather than the Objectstore redirect.
+        with override_options({"objectstore.enable_for.attachments": 0}):
+            self.create_attachment()
         self.path = f"/api/0/projects/{self.organization.slug}/{self.project.slug}/events/{self.event.event_id}/attachments/{self.attachment.id}/?download"
 
     @with_feature("organizations:event-attachments")