diff --git a/relay-server/src/endpoints/attachments.rs b/relay-server/src/endpoints/attachments.rs index 25ef10e1e66..a4c715bfb4e 100644 --- a/relay-server/src/endpoints/attachments.rs +++ b/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, config: &Config, - ) -> impl Future>, multer::Error>> + Send { - read_attachment_bytes_into_item(field, item, config, false) + ) -> Result>, multer::Error> { + read_bytes_into_item(field, item, config).await.map(Some) } } diff --git a/relay-server/src/endpoints/minidump.rs b/relay-server/src/endpoints/minidump.rs index 50771a419f5..8fef258b0ee 100644 --- a/relay-server/src/endpoints/minidump.rs +++ b/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, config: &Config, ) -> Result>, multer::Error> { + let read_inline = async |field: Field<'static>, item: Managed| { + 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. diff --git a/relay-server/src/endpoints/playstation.rs b/relay-server/src/endpoints/playstation.rs index 7863360abd7..5d89cbcf453 100644 --- a/relay-server/src/endpoints/playstation.rs +++ b/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), + }, } } } diff --git a/relay-server/src/utils/multipart.rs b/relay-server/src/utils/multipart.rs index 215819052af..54f7fb478f9 100644 --- a/relay-server/src/utils/multipart.rs +++ b/relay-server/src/utils/multipart.rs @@ -186,12 +186,11 @@ pub trait AttachmentStrategy { ) -> impl Future>, multer::Error>> + Send; } -pub async fn read_attachment_bytes_into_item( +pub async fn read_bytes_into_item( field: Field<'static>, mut item: Managed, config: &Config, - ignore_size_exceeded: bool, -) -> Result>, multer::Error> { +) -> Result, 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, config: &Config, - ) -> impl Future>, multer::Error>> + Send - { - read_attachment_bytes_into_item(field, item, config, false) + ) -> Result>, 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, config: &Config, - ) -> impl Future>, multer::Error>> + Send - { - read_attachment_bytes_into_item(field, item, config, true) + ) -> Result>, multer::Error> { + read_bytes_into_item(field, item, config).await.map(Some) } fn infer_type(&self, _: &Field) -> AttachmentType { diff --git a/tests/integration/test_minidump.py b/tests/integration/test_minidump.py index 4f0e20a17fe..1fbe24a4232 100644 --- a/tests/integration/test_minidump.py +++ b/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