feat(minidump): Shed large attachments (#5970)

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

Author: tobias-wilfert

relay-server/src/endpoints/attachments.rs

@@ -14,7 +14,7 @@ use crate::envelope::{AttachmentType, Envelope, Item};
 use crate::extractors::RequestMeta;
 use crate::managed::Managed;
 use crate::service::ServiceState;
-use crate::utils::{self, AttachmentStrategy, read_attachment_bytes_into_item};
+use crate::utils::{self, AttachmentStrategy, read_bytes_into_item};
 
 #[derive(Debug, Deserialize)]
 pub struct AttachmentPath {
@@ -28,13 +28,13 @@ impl AttachmentStrategy for AttachmentsAttachmentStrategy {
         AttachmentType::default()
     }
 
-    fn add_to_item(
+    async fn add_to_item(
         &self,
         field: Field<'static>,
         item: Managed<Item>,
         config: &Config,
-    ) -> impl Future<Output = Result<Option<Managed<Item>>, multer::Error>> + Send {
-        read_attachment_bytes_into_item(field, item, config, false)
+    ) -> Result<Option<Managed<Item>>, multer::Error> {
+        read_bytes_into_item(field, item, config).await.map(Some)
     }
 }
 

relay-server/src/endpoints/minidump.rs

@@ -30,7 +30,7 @@ use crate::service::ServiceState;
 use crate::services::outcome::{DiscardAttachmentType, DiscardItemType, DiscardReason, Outcome};
 use crate::services::upload::Upload;
 use crate::statsd::RelayCounters;
-use crate::utils::{self, AttachmentStrategy, read_attachment_bytes_into_item};
+use crate::utils::{self, AttachmentStrategy, read_bytes_into_item};
 
 /// The field name of a minidump in the multipart form-data upload.
 ///
@@ -220,9 +220,18 @@ impl<'a> AttachmentStrategy for MinidumpAttachmentStrategy<'a> {
         item: Managed<Item>,
         config: &Config,
     ) -> Result<Option<Managed<Item>>, multer::Error> {
+        let read_inline = async |field: Field<'static>, item: Managed<Item>| {
+            let is_minidump = matches!(item.attachment_type(), Some(AttachmentType::Minidump));
+            match read_bytes_into_item(field, item, config).await {
+                // Don't bubble up errors caused by large items unless it is the minidump itself.
+                Err(multer::Error::FieldSizeExceeded { .. }) if !is_minidump => Ok(None),
+                r => r.map(Some),
+            }
+        };
+
         // If we have no upload context just fall back to the old behavior.
         let Some(ref upload_context) = self.upload_context else {
-            return read_attachment_bytes_into_item(field, item, config, false).await;
+            return read_inline(field, item).await;
         };
 
         match upload_context.upload_decision(item.attachment_type()) {
@@ -239,9 +248,7 @@ impl<'a> AttachmentStrategy for MinidumpAttachmentStrategy<'a> {
                 )
                 .await)
             }
-            UploadDecision::Inline => {
-                read_attachment_bytes_into_item(field, item, config, false).await
-            }
+            UploadDecision::Inline => read_inline(field, item).await,
             UploadDecision::Drop(limits) => {
                 // This is best effort, the item here does not yet have its content set hence size
                 // is not correct.

relay-server/src/endpoints/playstation.rs

@@ -152,7 +152,12 @@ impl<'a> AttachmentStrategy for PlaystationAttachmentStrategy<'a> {
                 )
                 .await)
             }
-            _ => utils::read_attachment_bytes_into_item(field, item, config, true).await,
+            _ => match utils::read_bytes_into_item(field, item, config).await {
+                // Don't bubble up errors caused by large attachments, skip over them and continue
+                // with the next item.
+                Err(multer::Error::FieldSizeExceeded { .. }) => Ok(None),
+                r => r.map(Some),
+            },
         }
     }
 }

relay-server/src/utils/multipart.rs

@@ -186,12 +186,11 @@ pub trait AttachmentStrategy {
     ) -> impl Future<Output = Result<Option<Managed<Item>>, multer::Error>> + Send;
 }
 
-pub async fn read_attachment_bytes_into_item(
+pub async fn read_bytes_into_item(
     field: Field<'static>,
     mut item: Managed<Item>,
     config: &Config,
-    ignore_size_exceeded: bool,
-) -> Result<Option<Managed<Item>>, multer::Error> {
+) -> Result<Managed<Item>, multer::Error> {
     let content_type = field
         .content_type()
         .map(|ct| ct.as_ref().parse().unwrap_or(ContentType::OctetStream));
@@ -213,19 +212,19 @@ pub async fn read_attachment_bytes_into_item(
         };
         records.lenient(DataCategory::Attachment);
     });
+
     if n_bytes > limit {
         let attachment_type = item.attachment_type().unwrap_or(AttachmentType::Attachment);
         let item_type = DiscardItemType::Attachment(DiscardAttachmentType::from(attachment_type));
         let _ = item.reject_err(Outcome::Invalid(DiscardReason::TooLarge(item_type)));
-        return match ignore_size_exceeded {
-            true => Ok(None),
-            false => Err(multer::Error::FieldSizeExceeded {
-                limit: limit as u64,
-                field_name,
-            }),
-        };
+
+        Err(multer::Error::FieldSizeExceeded {
+            limit: limit as u64,
+            field_name,
+        })
+    } else {
+        Ok(item)
     }
-    Ok(Some(item))
 }
 
 pub async fn multipart_items(
@@ -412,14 +411,13 @@ mod tests {
 
         struct MockAttachmentStrategy;
         impl AttachmentStrategy for MockAttachmentStrategy {
-            fn add_to_item(
+            async fn add_to_item(
                 &self,
                 field: Field<'static>,
                 item: Managed<Item>,
                 config: &Config,
-            ) -> impl Future<Output = Result<Option<Managed<Item>>, multer::Error>> + Send
-            {
-                read_attachment_bytes_into_item(field, item, config, false)
+            ) -> Result<Option<Managed<Item>>, multer::Error> {
+                read_bytes_into_item(field, item, config).await.map(Some)
             }
 
             fn infer_type(&self, _: &Field) -> AttachmentType {
@@ -465,14 +463,13 @@ mod tests {
 
         struct MockAttachmentStrategy;
         impl AttachmentStrategy for MockAttachmentStrategy {
-            fn add_to_item(
+            async fn add_to_item(
                 &self,
                 field: Field<'static>,
                 item: Managed<Item>,
                 config: &Config,
-            ) -> impl Future<Output = Result<Option<Managed<Item>>, multer::Error>> + Send
-            {
-                read_attachment_bytes_into_item(field, item, config, true)
+            ) -> Result<Option<Managed<Item>>, multer::Error> {
+                read_bytes_into_item(field, item, config).await.map(Some)
             }
 
             fn infer_type(&self, _: &Field) -> AttachmentType {

tests/integration/test_minidump.py

@@ -1215,3 +1215,65 @@ def test_minidump_max_attachment_size_exceeded(
             "quantity": 1,
         },
     ]
+
+
+def test_minidump_large_attachment_skipped_when_no_project_fetching(mini_sentry, relay):
+    """
+    When the project fetching in the endpoints is disabled (and as a consequence
+    large attachments can not be uploaded to the objectstore), oversized regular
+    attachments should be silently skipped rather than rejecting the entire request.
+    The minidump must still be present in the forwarded envelope.
+    """
+    project_id = 42
+    minidump_content = b"MDMP content"
+    attachment_content = b"some attachment" * 100
+
+    mini_sentry.add_full_project_config(project_id)
+    mini_sentry.global_config["options"]["relay.endpoint-fetch-config.enabled"] = False
+
+    relay = relay(
+        mini_sentry,
+        options={
+            "limits": {
+                "max_attachment_size": len(attachment_content) - 1,
+                "max_attachments_size": 1000 * 1024 * 1024,
+            },
+            "outcomes": {
+                "emit_outcomes": True,
+                "batch_size": 1,
+                "batch_interval": 1,
+            },
+        },
+    )
+
+    attachments = [
+        ("attachment1", "attach1.txt", attachment_content),
+        (MINIDUMP_ATTACHMENT_NAME, "minidump.dmp", minidump_content),
+    ]
+
+    response = relay.send_minidump(project_id=project_id, files=attachments)
+    assert response.ok
+
+    outcomes = mini_sentry.get_aggregated_outcomes()
+
+    assert outcomes == [
+        {
+            "category": 4,
+            "outcome": 3,
+            "project_id": 42,
+            "quantity": 1500,
+            "reason": "too_large:attachment:attachment",
+        },
+        {
+            "category": 22,
+            "outcome": 3,
+            "project_id": 42,
+            "quantity": 1,
+            "reason": "too_large:attachment:attachment",
+        },
+    ]
+
+    envelope = mini_sentry.get_captured_envelope()
+
+    assert len(envelope.items) == 1
+    assert envelope.items[0].payload.bytes == minidump_content