feat(attachment): Add verification logic for attachment placeholders by tobias-wilfert · Pull Request #5747 · getsentry/relay

tobias-wilfert · 2026-03-19T15:17:00Z

Adds logic to verify attachment placeholders (to ensure that the self reported size is indeed correct) and to correctly store them (send the correct kafka message for them and never upload to the object store).

ref: https://linear.app/getsentry/issue/INGEST-726/accept-envelope-placeholders-in-relay

Follow up: Clean up the validation functions once the retain_mut is relaxed

linear-code · 2026-03-19T15:17:08Z

INGEST-726 Accept envelope placeholders in Relay

tobias-wilfert · 2026-03-19T15:17:57Z

relay-server/src/services/store.rs

+    fn produce_attachment_ref(
+        &self,
+        event_id: EventId,
+        project_id: ProjectId,
+        item: &Item,
+        send_individual_attachments: bool,


Logic inspired by that of produce_attachment.

tests/integration/test_attachment_ref.py

relay-server/src/services/store.rs

tobias-wilfert · 2026-03-23T12:20:22Z

relay-server/src/processing/utils/attachments.rs

+    // NOTE: Using the received timestamp here breaks tests without a pop-relay.
+    let location = signed_location
+        .verify(chrono::Utc::now(), config)
+        .map_err(|_| ProcessingError::InvalidAttachmentRef)?;


Potentially using chrono::Utc::now() might cause some issues if the palceholder gets spooled? Not sure if we worry about this.

relay-server/src/processing/utils/attachments.rs

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Autofix Details

Bugbot Autofix prepared a fix for the issue found in the latest run.

✅ Fixed: Placeholder deserialization rejects non-enum content types
- Changed AttachmentPlaceholder content_type field from Option to Option to accept arbitrary MIME types like 'image/png' and 'application/pdf', consistent with regular attachments.

Or push these changes by commenting:

@cursor push 98ef05f070

Preview (98ef05f070)

diff --git a/relay-server/src/endpoints/playstation.rs b/relay-server/src/endpoints/playstation.rs
--- a/relay-server/src/endpoints/playstation.rs
+++ b/relay-server/src/endpoints/playstation.rs
@@ -2,7 +2,6 @@
 //!
 //! Crashes are received as multipart uploads in this [format](https://game.develop.playstation.net/resources/documents/SDK/12.000/Core_Dump_System-Overview/ps5-core-dump-file-set-sending-format.html).
 use std::io;
-use std::str::FromStr;
 
 use axum::extract::{DefaultBodyLimit, Request};
 use axum::response::IntoResponse;
@@ -127,7 +126,7 @@
             && let Ok(location) = location.to_str()
             && let Ok(payload) = serde_json::to_vec(&AttachmentPlaceholder {
                 location,
-                content_type: content_type.and_then(|ct| ContentType::from_str(ct.as_ref()).ok()),
+                content_type: content_type.map(|ct| ct.to_string()),
             })
         {
             item.set_payload(ContentType::AttachmentRef, payload);

diff --git a/relay-server/src/envelope/attachment.rs b/relay-server/src/envelope/attachment.rs
--- a/relay-server/src/envelope/attachment.rs
+++ b/relay-server/src/envelope/attachment.rs
@@ -2,8 +2,6 @@
 
 use serde::{Deserialize, Serialize};
 
-use crate::envelope::ContentType;
-
 /// The type of an event attachment.
 ///
 /// These item types must align with the Sentry processing pipeline.
@@ -83,7 +81,7 @@
     #[serde(borrow)]
     pub location: &'a str,
     #[serde(skip_serializing_if = "Option::is_none")]
-    pub content_type: Option<ContentType>,
+    pub content_type: Option<String>,
 }
 
 #[derive(Debug)]
@@ -113,3 +111,53 @@
     AttachmentType,
     "an attachment type (see sentry develop docs)"
 );
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_placeholder_with_arbitrary_content_type() {
+        // Test that placeholders can accept any content type string, including
+        // unrecognized MIME types like "image/png"
+        let json = r#"{"location":"https://example.com/file","content_type":"image/png"}"#;
+        let placeholder: AttachmentPlaceholder = serde_json::from_str(json).unwrap();
+        assert_eq!(placeholder.location, "https://example.com/file");
+        assert_eq!(placeholder.content_type, Some("image/png".to_string()));
+    }
+
+    #[test]
+    fn test_placeholder_with_various_content_types() {
+        // Test various content types including PDF, video, etc.
+        let test_cases = vec![
+            "application/pdf",
+            "video/mp4",
+            "audio/mpeg",
+            "image/jpeg",
+            "text/html",
+        ];
+
+        for content_type in test_cases {
+            let json = format!(
+                r#"{{"location":"https://example.com/file","content_type":"{}"}}"#,
+                content_type
+            );
+            let placeholder: AttachmentPlaceholder = serde_json::from_str(&json).unwrap();
+            assert_eq!(
+                placeholder.content_type,
+                Some(content_type.to_string()),
+                "Failed to deserialize content_type: {}",
+                content_type
+            );
+        }
+    }
+
+    #[test]
+    fn test_placeholder_without_content_type() {
+        // Test that content_type is optional
+        let json = r#"{"location":"https://example.com/file"}"#;
+        let placeholder: AttachmentPlaceholder = serde_json::from_str(json).unwrap();
+        assert_eq!(placeholder.location, "https://example.com/file");
+        assert_eq!(placeholder.content_type, None);
+    }
+}

diff --git a/relay-server/src/services/store.rs b/relay-server/src/services/store.rs
--- a/relay-server/src/services/store.rs
+++ b/relay-server/src/services/store.rs
@@ -987,7 +987,7 @@
             id: Uuid::new_v4().to_string(),
             name: item.filename().unwrap_or(UNNAMED_ATTACHMENT).to_owned(),
             rate_limited: item.rate_limited(),
-            content_type: placeholder.content_type.map(|c| c.as_str().to_owned()),
+            content_type: placeholder.content_type.map(|s| s.to_ascii_lowercase()),
             attachment_type: item.attachment_type().unwrap_or_default(),
             size: item.attachment_body_size(),
             payload: AttachmentPayload::Stored(store_key),

_{This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.}

cursor · 2026-03-23T13:17:00Z

relay-server/src/envelope/attachment.rs

-#[derive(Serialize)]
+#[derive(Serialize, Deserialize)]
 pub struct AttachmentPlaceholder<'a> {
+    #[serde(borrow)]


Placeholder deserialization rejects non-enum content types

Medium Severity

The content_type field in AttachmentPlaceholder is Option<ContentType>, but ContentType is an enum that only supports a small set of known MIME types (e.g., text/plain, application/json, application/octet-stream). When serde_json::from_slice encounters an unrecognized content type string like "image/png" or "application/pdf", deserialization of the entire placeholder fails, causing the attachment to be rejected as invalid. Regular attachments accept any content type via raw_content_type() as a free-form string. Using Option<String> here would be consistent with how regular attachments handle content types.

Additional Locations (2)

relay-server/src/processing/utils/attachments.rs#L22-L25

relay-server/src/services/store.rs#L975-L978

This is true, but it's an issue that may have been introduced by #5671. So I wouldn't change it in this PR.

relay-server/src/services/objectstore.rs

relay-server/src/services/outcome.rs

loewenheim · 2026-03-23T13:34:33Z

relay-server/src/services/store.rs

produce_attachment and produce_attachment_ref split in the beginning depending on whether the item is an "actual" attachment or an attachment placeholder, but they end the same starting at if send_individual_attachments …. A different way to factor this might be like this:

fn produce_attachment(…) -> Result<Option<ChunkedAttachment>, StoreError> { let chunked_attachment = if item.is_attachment_ref() { chunked_attachment_from_placeholder(item) } else { chunked_attachment_from_attachment(item) }; if send_individual_attachments { let message = KafkaMessage::Attachment(AttachmentKafkaMessage { event_id, project_id, chunked_attachment, }); self.produce(KafkaTopic::Attachments, message)?; Ok(None) } else { Ok(Some(chunked_attachment)) } }

where chunked_attachment_from_placeholder and chunked_attachment_from_attachment are two new functions that express the logical differences between the current produce_attachment and produce_attachment_ref. WDYT @tobias-wilfert and @jjbayer ?

Sure we can do that.

jjbayer

Looks good, there's no rate limiting yet or did I miss that? Can be a separate PR.

jjbayer · 2026-03-23T15:19:25Z

relay-server/src/envelope/attachment.rs

-#[derive(Serialize)]
+#[derive(Serialize, Deserialize)]
 pub struct AttachmentPlaceholder<'a> {
+    #[serde(borrow)]


This is true, but it's an issue that may have been introduced by #5671. So I wouldn't change it in this PR.

relay-server/src/processing/attachments/process.rs

jjbayer · 2026-03-23T15:21:35Z

relay-server/src/processing/attachments/mod.rs

    async fn process(
        &self,
-        attachments: Managed<Self::Input>,
+        #[allow(unused_mut)] mut attachments: Managed<Self::Input>,


I would rather follow's @Dav1dde 's advice here and only use #[cfg(feature = "processing")] if absolutely necessary. I.e. remove it from validate_attachments and its calls. It is already guarded internally by ctx.is_processing().

Ok so rather move the #[cfg(feature = "processing")] inside the internals that call the other function that is #[cfg(feature = "processing")] ?

Yes, basically prevent it from spreading further out than necessary.

jjbayer · 2026-03-23T15:30:59Z

relay-server/src/processing/transactions/process.rs

+pub fn validate_attachments(transaction: &mut Managed<Box<ExpandedTransaction>>, ctx: Context<'_>) {
+    if !ctx.is_processing() {
+        return;
+    }
+
+    transaction.modify(|transaction, records| {
+        transaction.attachments.retain_mut(|attachment| {
+            match utils::attachments::validate(attachment, ctx.config) {
+                Ok(()) => true,
+                Err(err) => {
+                    records.reject_err(err, &*attachment);
+                    false
+                }
+            }
+        });
+    });
+}


I feel like it should be possible to generalize this function, something like

fn validate_attachments<T, F>(parent: T, getter: F, ctx: Context<'_>) where T: Managed, F: FnOnce(&mut T) -> &mut impl RetainMut<Item>

relay-server/src/processing/utils/attachments.rs

jjbayer · 2026-03-23T15:35:21Z

relay-server/src/services/store.rs

Co-authored-by: Sebastian Zivota <loewenheim@users.noreply.github.com>

Co-authored-by: Joris Bayer <joris.bayer@sentry.io>

tobias-wilfert · 2026-03-24T09:23:11Z

Looks good, there's no rate limiting yet or did I miss that? Can be a separate PR.

As far as I could tell the enforce quotas should work "out of the box" for attachment items (hence I left them as Items) but not sure we need more?

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

^{Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

cursor · 2026-03-24T09:23:30Z

relay-server/src/services/store.rs

    #[error("failed to store event because event id was missing")]
    NoEventId,
+    #[error("invalid attachment reference")]
+    InvalidAttachmentRef,


Store error maps to wrong discard reason

Low Severity

StoreError::InvalidAttachmentRef is mapped to DiscardReason::Internal by the blanket OutcomeError impl for StoreError, rather than to the dedicated DiscardReason::InvalidAttachmentRef that was specifically created for this purpose. While the validation step typically catches errors first, if produce_attachment_ref fails independently (e.g., signature timeout), the outcome will be reported with the wrong discard reason.

Additional Locations (1)

relay-server/src/services/store.rs#L62-L68

jjbayer · 2026-03-24T12:44:09Z

Looks good, there's no rate limiting yet or did I miss that? Can be a separate PR.

As far as I could tell the enforce quotas should work "out of the box" for attachment items (hence I left them as Items) but not sure we need more?

You are right, I forgot that I already added an integration test for that part: #5653

jjbayer · 2026-03-24T12:44:50Z

Looks good, there's no rate limiting yet or did I miss that? Can be a separate PR.

As far as I could tell the enforce quotas should work "out of the box" for attachment items (hence I left them as Items) but not sure we need more?

You are right, I forgot that I already added an integration test for that part: #5653

correctly handle attachment references in the stores

84eb00e

tobias-wilfert self-assigned this Mar 19, 2026

tobias-wilfert commented Mar 19, 2026

View reviewed changes

tobias-wilfert added 2 commits March 20, 2026 10:48

assertstored_key is none before it is set

0d39672

add happy path test

15c8294

jjbayer reviewed Mar 20, 2026

View reviewed changes

tests/integration/test_attachment_ref.py Outdated Show resolved Hide resolved

relay-server/src/services/store.rs Outdated Show resolved Hide resolved

tobias-wilfert mentioned this pull request Mar 20, 2026

feat(managed): Add logic so retain can be used both on vec and smallvec #5753

Draft

tobias-wilfert added 9 commits March 20, 2026 15:27

fix test (broke due to me adding relay-new-error-processing)

12da644

add validation logic

e246ebe

add test for validation logic and update ratelimit test

a3db734

appease clippy

51f3560

Merge branch 'master' into tobias-wilfert/feat/envelope-placeholder

119e4df

use application/vnd.sentry.attachment-ref+json

7b90e97

use verify in store to get key

efaa253

use post + patch in test helper

6fcb34d

add changelog entry

fab22b8

tobias-wilfert changed the title ~~WIP: Accept envelope placeholders in Relay~~ feat(attachment): Add verification logic for attachment placeholders Mar 23, 2026

tobias-wilfert commented Mar 23, 2026

View reviewed changes

tobias-wilfert marked this pull request as ready for review March 23, 2026 13:08

tobias-wilfert requested a review from a team as a code owner March 23, 2026 13:08

sentry bot reviewed Mar 23, 2026

View reviewed changes

relay-server/src/processing/utils/attachments.rs Show resolved Hide resolved

cursor bot reviewed Mar 23, 2026

View reviewed changes

loewenheim reviewed Mar 23, 2026

View reviewed changes

jjbayer reviewed Mar 23, 2026

View reviewed changes

tobias-wilfert and others added 4 commits March 24, 2026 10:12

Update relay-server/src/services/objectstore.rs

dec60e7

Co-authored-by: Sebastian Zivota <loewenheim@users.noreply.github.com>

Update relay-server/src/services/outcome.rs

6612cb3

Co-authored-by: Sebastian Zivota <loewenheim@users.noreply.github.com>

Update relay-server/src/processing/attachments/process.rs

0dd5dd5

Co-authored-by: Joris Bayer <joris.bayer@sentry.io>

Update relay-server/src/processing/utils/attachments.rs

56171c9

Co-authored-by: Joris Bayer <joris.bayer@sentry.io>

cursor bot reviewed Mar 24, 2026

View reviewed changes

Conversation

tobias-wilfert commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linear-code bot commented Mar 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cursor bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cursor bot Mar 23, 2026

Choose a reason for hiding this comment

Placeholder deserialization rejects non-enum content types

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjbayer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobias-wilfert commented Mar 24, 2026

Uh oh!

cursor bot left a comment

Choose a reason for hiding this comment

Uh oh!

cursor bot Mar 24, 2026

Choose a reason for hiding this comment

Store error maps to wrong discard reason

Uh oh!

jjbayer commented Mar 24, 2026

Uh oh!

jjbayer commented Mar 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tobias-wilfert commented Mar 19, 2026 •

edited

Loading

cursor bot left a comment •

edited

Loading