From c055532a6edc46ec20c75030488b95d3fdf0c907 Mon Sep 17 00:00:00 2001 From: elramen Date: Wed, 6 May 2026 17:06:41 +0200 Subject: [PATCH 1/5] fix(multipart): Use correct discard reasons --- relay-server/src/endpoints/attachments.rs | 9 +- relay-server/src/endpoints/common.rs | 28 +--- relay-server/src/endpoints/minidump.rs | 95 ++++++++----- relay-server/src/endpoints/playstation.rs | 31 ++-- relay-server/src/envelope/item.rs | 2 - relay-server/src/envelope/mod.rs | 8 ++ relay-server/src/services/outcome.rs | 8 ++ relay-server/src/utils/multipart.rs | 35 +++-- tests/integration/test_minidump.py | 163 ++++++++++++++++++++-- tests/integration/test_playstation.py | 16 +-- 10 files changed, 290 insertions(+), 105 deletions(-) diff --git a/relay-server/src/endpoints/attachments.rs b/relay-server/src/endpoints/attachments.rs index 3c160f892d5..25ef10e1e66 100644 --- a/relay-server/src/endpoints/attachments.rs +++ b/relay-server/src/endpoints/attachments.rs @@ -5,6 +5,7 @@ use axum::routing::{MethodRouter, post}; use multer::{Field, Multipart}; use relay_config::Config; use relay_event_schema::protocol::EventId; +use relay_quotas::DataCategory; use serde::Deserialize; use tower_http::limit::RequestBodyLimitLayer; @@ -52,8 +53,12 @@ async fn multipart_to_envelope( ) .await?; - let envelope = - common::managed_items_to_envelope(items, meta, state.outcome_aggregator(), path.event_id); + let envelope = items.map(|items, records| { + if items.iter().any(|i| i.creates_event()) { + records.modify_by(DataCategory::Error, 1); + } + Box::new(Envelope::from_request(Some(path.event_id), meta).with_items(items)) + }); Ok(envelope) } diff --git a/relay-server/src/endpoints/common.rs b/relay-server/src/endpoints/common.rs index d608a9da2a1..ddbdeedfae2 100644 --- a/relay-server/src/endpoints/common.rs +++ b/relay-server/src/endpoints/common.rs @@ -17,13 +17,12 @@ use serde::Deserialize; use crate::envelope::{ AttachmentPlaceholder, AttachmentType, ContentType, Envelope, EnvelopeError, Item, ItemType, - ManagedItems, + Items, }; -use crate::extractors::RequestMeta; use crate::managed::{Managed, Rejected}; use crate::service::ServiceState; use crate::services::buffer::{ProjectKeyPair, PushError}; -use crate::services::outcome::{DiscardItemType, DiscardReason, Outcome, TrackOutcome}; +use crate::services::outcome::{DiscardItemType, DiscardReason, Outcome}; use crate::services::processor::{BucketSource, MetricData, ProcessMetrics}; use crate::services::upload::{Create, Stream, Upload}; use crate::statsd::{RelayCounters, RelayDistributions}; @@ -292,7 +291,7 @@ pub fn event_id_from_formdata(data: &[u8]) -> Result, BadStoreRe /// /// Extracting the event id from chunked formdata fields on the Minidump endpoint (`sentry__1`, /// `sentry__2`, ...) is not supported. In this case, `None` is returned. -pub fn event_id_from_items(items: &ManagedItems) -> Result, BadStoreRequest> { +pub fn event_id_from_items(items: &Items) -> Result, BadStoreRequest> { if let Some(item) = items.iter().find(|item| item.ty() == &ItemType::Event) && let Some(event_id) = event_id_from_json(&item.payload())? { @@ -575,27 +574,6 @@ where Some(item) } -pub fn managed_items_to_envelope( - items: ManagedItems, - meta: RequestMeta, - outcome_aggregator: &Addr, - event_id: EventId, -) -> Managed> { - let envelope = Envelope::from_request(Some(event_id), meta); - let mut envelope = Managed::from_envelope(envelope, outcome_aggregator.clone()); - let mut has_event = false; - for item in items { - envelope.merge_with(item, |envelope, item, records| { - if !has_event && item.creates_event() { - records.modify_by(DataCategory::Error, 1); - has_event = true; - } - envelope.add_item(item); - }); - } - envelope -} - #[derive(Debug)] pub struct TextResponse(pub Option); diff --git a/relay-server/src/endpoints/minidump.rs b/relay-server/src/endpoints/minidump.rs index 0df643a5c64..467a9a545d7 100644 --- a/relay-server/src/endpoints/minidump.rs +++ b/relay-server/src/endpoints/minidump.rs @@ -24,7 +24,7 @@ use crate::endpoints::common::{self, BadStoreRequest, TextResponse, upload_to_ob use crate::envelope::ContentType::Minidump; use crate::envelope::{AttachmentType, Envelope, Item, ItemType}; use crate::extractors::{RawContentType, RequestMeta}; -use crate::managed::Managed; +use crate::managed::{Counted, Managed}; use crate::middlewares; use crate::service::ServiceState; use crate::services::outcome::{DiscardAttachmentType, DiscardItemType, DiscardReason, Outcome}; @@ -63,9 +63,10 @@ const ZSTD_MAGIC_HEADER: &[u8] = b"\x28\xB5\x2F\xFD"; /// Content types by which standalone uploads can be recognized. const MINIDUMP_RAW_CONTENT_TYPES: &[&str] = &["application/octet-stream", "application/x-dmp"]; -fn validate_minidump(data: &[u8]) -> Result<(), BadStoreRequest> { +fn validate_minidump(data: &[u8], items: &Managed) -> Result<(), BadStoreRequest> { if !data.starts_with(MINIDUMP_MAGIC_HEADER_LE) && !data.starts_with(MINIDUMP_MAGIC_HEADER_BE) { relay_log::trace!("invalid minidump file"); + let _ = items.reject_err(Outcome::Invalid(DiscardReason::InvalidMinidump)); return Err(BadStoreRequest::InvalidMinidump); } @@ -107,7 +108,11 @@ fn decoder_from(minidump_data: Bytes) -> Option> { /// or returns the provided minidump payload untouched if no format where detected. /// /// Returns an `Overflow` error if the decompressed size exceeds `max_size`. -fn decode_minidump(minidump_data: Bytes, max_size: usize) -> Result { +fn decode_minidump( + minidump_data: Bytes, + max_size: usize, + items: &Managed, +) -> Result { let Some(decoder) = decoder_from(minidump_data.clone()) else { // this means we haven't detected any compression container // proceed to process the payload untouched (as a plain minidump). @@ -119,15 +124,16 @@ fn decode_minidump(minidump_data: Bytes, max_size: usize) -> Result { if decoded.len() > max_size { - return Err(BadStoreRequest::Overflow(DiscardItemType::Attachment( - DiscardAttachmentType::Minidump, - ))); + let item_type = DiscardItemType::Attachment(DiscardAttachmentType::Minidump); + let _ = items.reject_err(Outcome::Invalid(DiscardReason::TooLarge(item_type))); + return Err(BadStoreRequest::Overflow(item_type)); } Ok(Bytes::from(decoded)) } Err(err) => { // we detected a compression container but failed to decode it relay_log::trace!("invalid compression container"); + let _ = items.reject_err(Outcome::Invalid(DiscardReason::InvalidCompression)); Err(BadStoreRequest::InvalidCompressionContainer(err)) } } @@ -280,36 +286,39 @@ async fn multipart_to_envelope( ) .await?; - let minidump_item = items - .iter_mut() - .find(|item| item.attachment_type() == Some(AttachmentType::Minidump)) - .ok_or(BadStoreRequest::MissingMinidump)?; + let minidump_idx = items + .iter() + .position(|item| item.attachment_type() == Some(AttachmentType::Minidump)) + .ok_or(BadStoreRequest::MissingMinidump) + .inspect_err(|_| { + let _ = items.reject_err(Outcome::Invalid(DiscardReason::MissingMinidumpUpload)); + })?; // Doing these operations does not make sense if we already streamed the minidump to objectstore. - if !minidump_item.is_attachment_ref() { - let payload = minidump_item.payload(); + if !items[minidump_idx].is_attachment_ref() { + let payload = items[minidump_idx].payload(); let payload = extract_embedded_minidump(payload.clone()) .await? .unwrap_or(payload); - let payload = decode_minidump(payload, config.max_attachment_size())?; + let payload = decode_minidump(payload, config.max_attachment_size(), &items)?; - minidump_item.modify(|inner, records| { - inner.set_payload(Minidump, payload); - records.lenient(DataCategory::Attachment); - }); - - validate_minidump(&minidump_item.payload())?; + validate_minidump(&payload, &items)?; - minidump_item.modify(|inner, _| { - if let Some(minidump_filename) = inner.filename() { - inner.set_filename(remove_container_extension(minidump_filename).to_owned()) + items.modify(|items, records| { + let minidump_item = &mut items[minidump_idx]; + minidump_item.set_payload(Minidump, payload); + records.lenient(DataCategory::Attachment); // decoding the minidump changes its size + if let Some(minidump_filename) = minidump_item.filename() { + minidump_item.set_filename(remove_container_extension(minidump_filename).to_owned()) } }); } let event_id = common::event_id_from_items(&items)?.unwrap_or_else(EventId::new); - let envelope = - common::managed_items_to_envelope(items, meta, state.outcome_aggregator(), event_id); + let envelope = items.map(|items, records| { + records.modify_by(DataCategory::Error, 1); + Box::new(Envelope::from_request(Some(event_id), meta).with_items(items)) + }); Ok(envelope) } @@ -396,7 +405,6 @@ async fn raw_minidump_to_envelope( item.set_filename(MINIDUMP_FILE_NAME); item.set_attachment_type(AttachmentType::Minidump); let mut item = Managed::with_meta_from_request_meta(&meta, state.outcome_aggregator(), item); - if let Some(upload_context) = upload_context && matches!(upload_context.upload_minidumps, UploadDecision::Upload) { @@ -415,20 +423,19 @@ async fn raw_minidump_to_envelope( let decoded_payload = decode_minidump( request.extract().await?, state.config().max_attachment_size(), + &item, )?; + validate_minidump(&decoded_payload, &item)?; item.modify(|inner, records| { inner.set_payload(Minidump, decoded_payload); records.lenient(DataCategory::Attachment); // decoding the minidump changes its size }); - validate_minidump(&item.payload())?; }; // Create an envelope with a random event id. - let envelope = Envelope::from_request(Some(EventId::new()), meta); - let mut envelope = Managed::from_envelope(envelope, state.outcome_aggregator().clone()); - envelope.merge_with(item, |envelope, item, records| { + let envelope = item.map(|item, records| { records.modify_by(DataCategory::Error, 1); - envelope.add_item(item); + Box::new(Envelope::from_request(Some(EventId::new()), meta).with_items(vec![item])) }); Ok(envelope) } @@ -510,14 +517,28 @@ mod tests { #[test] fn test_validate_minidump() { + let (items, mut handle) = Managed::for_test(Item::new(ItemType::Attachment)).build(); + let be_minidump = b"PMDMxxxxxx"; - assert!(validate_minidump(be_minidump).is_ok()); + assert!(validate_minidump(be_minidump, &items).is_ok()); let le_minidump = b"MDMPxxxxxx"; - assert!(validate_minidump(le_minidump).is_ok()); + assert!(validate_minidump(le_minidump, &items).is_ok()); let garbage = b"xxxxxx"; - assert!(validate_minidump(garbage).is_err()); + assert!(validate_minidump(garbage, &items).is_err()); + + drop(items); + handle.assert_outcome( + &Outcome::Invalid(DiscardReason::InvalidMinidump), + DataCategory::Attachment, + 1, + ); + handle.assert_outcome( + &Outcome::Invalid(DiscardReason::InvalidMinidump), + DataCategory::AttachmentItem, + 1, + ); } type EncodeFunction = fn(&[u8]) -> Result>; @@ -566,7 +587,9 @@ mod tests { let decoder = decoder_from(compressed).unwrap(); let decoded = run_decoder(decoder); assert!(decoded.is_ok()); - assert!(validate_minidump(&decoded.unwrap()).is_err()); + assert!( + validate_minidump(&decoded.unwrap(), &Managed::for_test(()).build().0).is_err() + ); } Ok(()) @@ -579,12 +602,12 @@ mod tests { let compressed = encode_gzip(&minidump_data)?; // With a limit larger than the decompressed size, decoding should succeed - let result = decode_minidump(compressed.clone(), 200); + let result = decode_minidump(compressed.clone(), 200, &Managed::for_test(()).build().0); assert!(result.is_ok()); assert_eq!(result.unwrap().len(), 100); // With a limit smaller than the decompressed size, decoding should fail with Overflow - let result = decode_minidump(compressed, 50); + let result = decode_minidump(compressed, 50, &Managed::for_test(()).build().0); assert!(matches!(result, Err(BadStoreRequest::Overflow(_)))); Ok(()) diff --git a/relay-server/src/endpoints/playstation.rs b/relay-server/src/endpoints/playstation.rs index 567c7366b9f..9c9153625c7 100644 --- a/relay-server/src/endpoints/playstation.rs +++ b/relay-server/src/endpoints/playstation.rs @@ -15,12 +15,12 @@ use tower_http::limit::RequestBodyLimitLayer; use crate::endpoints::common::{self, BadStoreRequest, TextResponse}; use crate::envelope::ContentType::OctetStream; -use crate::envelope::{AttachmentType, Envelope, Item}; +use crate::envelope::{AttachmentType, Envelope, Item, Items}; use crate::extractors::{RawContentType, RequestMeta}; use crate::managed::Managed; use crate::middlewares; use crate::service::ServiceState; -use crate::services::outcome::DiscardReason; +use crate::services::outcome::{DiscardReason, Outcome}; use crate::services::upload::Upload; use crate::utils::{self, AttachmentStrategy}; @@ -165,9 +165,10 @@ fn create_data_request_response() -> DataRequestResponse { } } -fn validate_prosperodump(data: &[u8]) -> Result<(), BadStoreRequest> { +fn validate_prosperodump(data: &[u8], items: &Managed) -> Result<(), BadStoreRequest> { if !data.starts_with(LZ4_MAGIC_HEADER) && !data.starts_with(PROSPERODUMP_MAGIC_HEADER) { relay_log::trace!("invalid prosperodump file"); + let _ = items.reject_err(Outcome::Invalid(DiscardReason::InvalidProsperodump)); return Err(BadStoreRequest::InvalidProsperodump); } @@ -189,16 +190,24 @@ async fn multipart_to_envelope( ) .await?; - let prosperodump_item = items - .iter_mut() - .find(|item| item.attachment_type() == Some(AttachmentType::Prosperodump)) - .ok_or(BadStoreRequest::MissingProsperodump)?; - prosperodump_item.modify(|inner, _| inner.set_payload(OctetStream, inner.payload())); - validate_prosperodump(&prosperodump_item.payload())?; + let prosperodump_idx = items + .iter() + .position(|item| item.attachment_type() == Some(AttachmentType::Prosperodump)) + .ok_or(BadStoreRequest::MissingProsperodump) + .inspect_err(|_| { + let _ = items.reject_err(Outcome::Invalid(DiscardReason::MissingProsperodumpUpload)); + })?; + let payload = items[prosperodump_idx].payload(); + validate_prosperodump(&payload, &items)?; + items.modify(|items, _| { + items[prosperodump_idx].set_payload(OctetStream, payload); + }); let event_id = common::event_id_from_items(&items)?.unwrap_or_else(EventId::new); - let envelope = - common::managed_items_to_envelope(items, meta, state.outcome_aggregator(), event_id); + let envelope = items.map(|items, records| { + records.modify_by(DataCategory::Error, 1); + Box::new(Envelope::from_request(Some(event_id), meta).with_items(items)) + }); Ok(envelope) } diff --git a/relay-server/src/envelope/item.rs b/relay-server/src/envelope/item.rs index d6467b8d57b..a7512e33a83 100644 --- a/relay-server/src/envelope/item.rs +++ b/relay-server/src/envelope/item.rs @@ -12,7 +12,6 @@ use smallvec::{SmallVec, smallvec}; use crate::envelope::{AttachmentType, ContentType, EnvelopeError}; use crate::integrations::{Integration, LogsIntegration, SpansIntegration}; -use crate::managed::Managed; use crate::statsd::RelayTimers; #[derive(Clone, Debug)] @@ -770,7 +769,6 @@ impl Item { } } -pub type ManagedItems = SmallVec<[Managed; 3]>; pub type Items = SmallVec<[Item; 3]>; pub type ItemIter<'a> = std::slice::Iter<'a, Item>; pub type ItemIterMut<'a> = std::slice::IterMut<'a, Item>; diff --git a/relay-server/src/envelope/mod.rs b/relay-server/src/envelope/mod.rs index 5b67f481b8c..2fc78868e55 100644 --- a/relay-server/src/envelope/mod.rs +++ b/relay-server/src/envelope/mod.rs @@ -551,6 +551,14 @@ impl Envelope { self.items.push(item) } + /// Add new items and return `Self`. + pub fn with_items(mut self, items: impl IntoIterator) -> Self { + for item in items { + self.items.push(item) + } + self + } + /// Splits off the items from the envelope using provided predicates. /// /// First predicate is the additional condition on the count of found items by second diff --git a/relay-server/src/services/outcome.rs b/relay-server/src/services/outcome.rs index 2611b50e156..ee3a7a45040 100644 --- a/relay-server/src/services/outcome.rs +++ b/relay-server/src/services/outcome.rs @@ -377,6 +377,12 @@ pub enum DiscardReason { /// since it resolves the project first, and then checks for the valid project key. MultiProjectId, + /// (Relay) A request without a prosperodump was made to the playstation endpoint. + MissingProsperodumpUpload, + + /// (Relay) The prosperodump submitted to the playstation endpoint was invalid. + InvalidProsperodump, + /// (Relay) A minidump file was missing for the minidump endpoint. MissingMinidumpUpload, @@ -509,6 +515,8 @@ impl DiscardReason { DiscardReason::DisallowedMethod => "disallowed_method", DiscardReason::ContentType => "content_type", DiscardReason::MultiProjectId => "multi_project_id", + DiscardReason::MissingProsperodumpUpload => "missing_prosperodump_upload", + DiscardReason::InvalidProsperodump => "invalid_prosperodump", DiscardReason::MissingMinidumpUpload => "missing_minidump_upload", DiscardReason::InvalidMinidump => "invalid_minidump", DiscardReason::SecurityReportType => "security_report_type", diff --git a/relay-server/src/utils/multipart.rs b/relay-server/src/utils/multipart.rs index 9a81cee7ff7..215819052af 100644 --- a/relay-server/src/utils/multipart.rs +++ b/relay-server/src/utils/multipart.rs @@ -13,10 +13,12 @@ use tokio::io::AsyncReadExt; use tokio_util::io::StreamReader; use crate::endpoints::common::BadStoreRequest; -use crate::envelope::{AttachmentType, ContentType, Item, ItemType, ManagedItems}; +use crate::envelope::{AttachmentType, ContentType, Item, ItemType, Items}; use crate::extractors::RequestMeta; use crate::managed::Managed; -use crate::services::outcome::TrackOutcome; +use crate::services::outcome::{ + DiscardAttachmentType, DiscardItemType, DiscardReason, Outcome, TrackOutcome, +}; /// Type used for encoding string lengths. type Len = u32; @@ -212,6 +214,9 @@ 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 { @@ -229,11 +234,11 @@ pub async fn multipart_items( attachment_strategy: impl AttachmentStrategy, request_meta: &RequestMeta, outcome_aggregator: &Addr, -) -> Result { - let mut items = ManagedItems::new(); +) -> Result, multer::Error> { + let mut items = + Managed::with_meta_from_request_meta(request_meta, outcome_aggregator, Items::new()); let mut form_data = FormDataWriter::new(); let mut attachments_size = 0; - let meta_provider = Managed::with_meta_from_request_meta(request_meta, outcome_aggregator, ()); while let Some(field) = multipart.next_field().await? { if let Some(file_name) = field.file_name() { @@ -241,19 +246,31 @@ pub async fn multipart_items( let attachment_type = attachment_strategy.infer_type(&field); item.set_attachment_type(attachment_type); item.set_filename(file_name); - let item = meta_provider.wrap(item); - let item = attachment_strategy.add_to_item(field, item, config).await?; + let item = items.wrap(item); + let item = attachment_strategy + .add_to_item(field, item, config) + .await + .inspect_err(|e| { + if let multer::Error::FieldSizeExceeded { .. } = e { + let attachment_type = DiscardAttachmentType::from(attachment_type); + let item_type = DiscardItemType::Attachment(attachment_type); + let discard_reason = DiscardReason::TooLarge(item_type); + let _ = items.reject_err(Outcome::Invalid(discard_reason)); + } + })?; if let Some(item) = item { // This increases the attachments byte count even if the item is an attachment ref. // This is by design as the total number of bytes read into memory should be // constrained. attachments_size += item.len(); + items.merge_with(item, |items, item, _| items.push(item)); if attachments_size > config.max_attachments_size() { + let item_type = DiscardItemType::Attachment(DiscardAttachmentType::Attachment); + let _ = items.reject_err(Outcome::Invalid(DiscardReason::TooLarge(item_type))); return Err(multer::Error::StreamSizeExceeded { limit: config.max_attachments_size() as u64, }); } - items.push(item); } } else if let Some(field_name) = field.name().map(str::to_owned) { // Ensure to decode this SAFELY to match Django's POST data behavior. This allows us to @@ -271,7 +288,7 @@ pub async fn multipart_items( // Content type is `Text` (since it is not a json object but multiple // json arrays serialized one after the other). item.set_payload(ContentType::Text, form_data); - items.push(meta_provider.wrap(item)); + items.merge_with(items.wrap(item), |items, item, _| items.push(item)); } Ok(items) diff --git a/tests/integration/test_minidump.py b/tests/integration/test_minidump.py index ce63c9d1bbb..4f0e20a17fe 100644 --- a/tests/integration/test_minidump.py +++ b/tests/integration/test_minidump.py @@ -13,6 +13,7 @@ from urllib3.filepost import encode_multipart_formdata from sentry_relay.consts import DataCategory +from .asserts import time_within_delta from .test_attachment_ref import upload_and_make_ref from .consts import DUMMY_UPLOAD_LOCATION @@ -280,31 +281,104 @@ def test_minidump_invalid_json(mini_sentry, relay): assert_only_minidump(envelope) -def test_minidump_invalid_magic(mini_sentry, relay): +def test_minidump_invalid_magic(mini_sentry, relay_with_processing, outcomes_consumer): project_id = 42 - relay = relay(mini_sentry) mini_sentry.add_full_project_config(project_id) + outcomes_consumer = outcomes_consumer() + relay = relay_with_processing() - attachments = [ - (MINIDUMP_ATTACHMENT_NAME, "minidump.dmp", "content without MDMP magic"), - ] - - with pytest.raises(HTTPError): + content = b"content without MDMP magic" + attachments = [(MINIDUMP_ATTACHMENT_NAME, "minidump.dmp", content)] + with pytest.raises(HTTPError) as exc_info: relay.send_minidump(project_id=project_id, files=attachments) + assert exc_info.value.response.status_code == 400 + assert outcomes_consumer.get_outcomes() == [ + { + "timestamp": time_within_delta(), + "project_id": 42, + "outcome": 3, + "reason": "invalid_minidump", + "category": DataCategory.ATTACHMENT.value, + "quantity": len(content), + }, + { + "timestamp": time_within_delta(), + "project_id": 42, + "outcome": 3, + "reason": "invalid_minidump", + "category": DataCategory.ATTACHMENT_ITEM.value, + "quantity": 1, + }, + ] -def test_minidump_invalid_field(mini_sentry, relay): + +def test_minidump_invalid_field(mini_sentry, relay_with_processing, outcomes_consumer): project_id = 42 - relay = relay(mini_sentry) mini_sentry.add_full_project_config(project_id) + outcomes_consumer = outcomes_consumer() + relay = relay_with_processing() - attachments = [ - ("unknown_field_name", "minidump.dmp", "MDMP content"), + content = b"MDMP content" + attachments = [("unknown_field_name", "minidump.dmp", content)] + with pytest.raises(HTTPError) as exc_info: + relay.send_minidump(project_id=project_id, files=attachments) + + assert exc_info.value.response.status_code == 400 + assert outcomes_consumer.get_outcomes() == [ + { + "timestamp": time_within_delta(), + "project_id": 42, + "outcome": 3, + "reason": "missing_minidump_upload", + "category": DataCategory.ATTACHMENT.value, + "quantity": len(content), + }, + { + "timestamp": time_within_delta(), + "project_id": 42, + "outcome": 3, + "reason": "missing_minidump_upload", + "category": DataCategory.ATTACHMENT_ITEM.value, + "quantity": 1, + }, ] - with pytest.raises(HTTPError): + +def test_minidump_invalid_compression_outcome( + mini_sentry, relay_with_processing, outcomes_consumer +): + project_id = 42 + mini_sentry.add_full_project_config(project_id) + outcomes_consumer = outcomes_consumer() + relay = relay_with_processing() + content = b"\x1f\x8b" + b"not a valid gzip stream" + attachments = [(MINIDUMP_ATTACHMENT_NAME, "minidump.dmp.gz", content)] + + with pytest.raises(HTTPError) as exc_info: relay.send_minidump(project_id=project_id, files=attachments) + assert exc_info.value.response.status_code == 400 + outcomes = outcomes_consumer.get_outcomes() + assert outcomes == [ + { + "timestamp": time_within_delta(), + "project_id": 42, + "outcome": 3, + "reason": "invalid_compression", + "category": DataCategory.ATTACHMENT.value, + "quantity": len(content), + }, + { + "timestamp": time_within_delta(), + "project_id": 42, + "outcome": 3, + "reason": "invalid_compression", + "category": DataCategory.ATTACHMENT_ITEM.value, + "quantity": 1, + }, + ] + @pytest.mark.parametrize( "content_type", ("application/octet-stream", "application/x-dmp") @@ -1076,3 +1150,68 @@ def test_minidump_objectstore_uploads_rate_limits( assert mini_sentry.get_aggregated_outcomes() == sorted( expected_outcomes, key=lambda o: sorted(o.items()) ) + + +def test_minidump_max_attachment_size_exceeded( + mini_sentry, relay_with_processing, outcomes_consumer +): + project_id = 42 + dmp_path = os.path.join(os.path.dirname(__file__), "fixtures/native/minidump.dmp") + with open(dmp_path, "rb") as f: + minidump_content = f.read() + attachment_content = b"yo" + + mini_sentry.add_full_project_config(project_id) + outcomes_consumer = outcomes_consumer() + relay = relay_with_processing( + { + "limits": { + "max_attachment_size": len(minidump_content) - 1, + "max_attachments_size": 1000 * 1024 * 1024, + } + } + ) + + attachments = [ + ("attachment1", "attach1.txt", attachment_content), + (MINIDUMP_ATTACHMENT_NAME, "minidump.dmp", minidump_content), + ] + with pytest.raises(HTTPError) as exc_info: + relay.send_minidump(project_id=project_id, files=attachments) + + assert exc_info.value.response.status_code == 400 + outcomes = outcomes_consumer.get_outcomes() + assert outcomes == [ + { + "timestamp": time_within_delta(), + "project_id": 42, + "outcome": 3, + "reason": "too_large:attachment:minidump", + "category": DataCategory.ATTACHMENT.value, + "quantity": len(minidump_content), + }, + { + "timestamp": time_within_delta(), + "project_id": 42, + "outcome": 3, + "reason": "too_large:attachment:minidump", + "category": DataCategory.ATTACHMENT_ITEM.value, + "quantity": 1, + }, + { + "timestamp": time_within_delta(), + "project_id": 42, + "outcome": 3, + "reason": "too_large:attachment:minidump", + "category": DataCategory.ATTACHMENT.value, + "quantity": len(attachment_content), + }, + { + "timestamp": time_within_delta(), + "project_id": 42, + "outcome": 3, + "reason": "too_large:attachment:minidump", + "category": DataCategory.ATTACHMENT_ITEM.value, + "quantity": 1, + }, + ] diff --git a/tests/integration/test_playstation.py b/tests/integration/test_playstation.py index 288547bf538..fb1668e43b5 100644 --- a/tests/integration/test_playstation.py +++ b/tests/integration/test_playstation.py @@ -288,7 +288,7 @@ def test_playstation_invalid_prosperodump( "timestamp": time_within_delta(), "project_id": 42, "outcome": 3, - "reason": "internal", + "reason": "invalid_prosperodump", "category": 4, "quantity": len(playstation_dump), }, @@ -296,7 +296,7 @@ def test_playstation_invalid_prosperodump( "timestamp": time_within_delta(), "project_id": 42, "outcome": 3, - "reason": "internal", + "reason": "invalid_prosperodump", "category": 22, "quantity": 1, }, @@ -325,7 +325,7 @@ def test_playstation_missing_prosperodump( "timestamp": time_within_delta(), "project_id": 42, "outcome": 3, - "reason": "internal", + "reason": "missing_prosperodump_upload", "category": 4, "quantity": len(video_content), }, @@ -333,7 +333,7 @@ def test_playstation_missing_prosperodump( "timestamp": time_within_delta(), "project_id": 42, "outcome": 3, - "reason": "internal", + "reason": "missing_prosperodump_upload", "category": 22, "quantity": 1, }, @@ -367,7 +367,7 @@ def test_playstation_max_attachments_size_exceeded( "timestamp": time_within_delta(), "project_id": 42, "outcome": 3, - "reason": "internal", + "reason": "too_large:attachment:attachment", "category": 4, "quantity": len(playstation_dump), }, @@ -375,7 +375,7 @@ def test_playstation_max_attachments_size_exceeded( "timestamp": time_within_delta(), "project_id": 42, "outcome": 3, - "reason": "internal", + "reason": "too_large:attachment:attachment", "category": 22, "quantity": 1, }, @@ -409,7 +409,7 @@ def test_playstation_max_attachment_size_exceeded( "timestamp": time_within_delta(), "project_id": 42, "outcome": 3, - "reason": "internal", + "reason": "too_large:attachment:prosperodump", "category": 4, "quantity": len(playstation_dump), }, @@ -417,7 +417,7 @@ def test_playstation_max_attachment_size_exceeded( "timestamp": time_within_delta(), "project_id": 42, "outcome": 3, - "reason": "internal", + "reason": "too_large:attachment:prosperodump", "category": 22, "quantity": 1, }, From d4025f14b5a49230c90d20e6d85c19556ace7cb3 Mon Sep 17 00:00:00 2001 From: elramen Date: Wed, 6 May 2026 17:40:10 +0200 Subject: [PATCH 2/5] add changelog entry --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4ca247dee2..1eb09b64e30 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +**Bug Fixes**: + +- Emit more precise outcome discard reasons for the Playstation, Minidump, and Attachments endpoints. ([#5950](https://github.com/getsentry/relay/pull/5950)) + ## 26.4.2 **Features**: From 1a20b80b50c07bbfcced12d5cb6574864561f5ba Mon Sep 17 00:00:00 2001 From: elramen Date: Thu, 7 May 2026 12:59:16 +0200 Subject: [PATCH 3/5] use reject --- relay-server/src/endpoints/common.rs | 20 +++++++++ relay-server/src/endpoints/minidump.rs | 55 +++++++---------------- relay-server/src/endpoints/playstation.rs | 13 +++--- relay-server/src/managed/managed.rs | 9 ++++ 4 files changed, 49 insertions(+), 48 deletions(-) diff --git a/relay-server/src/endpoints/common.rs b/relay-server/src/endpoints/common.rs index ddbdeedfae2..d0d45f30efe 100644 --- a/relay-server/src/endpoints/common.rs +++ b/relay-server/src/endpoints/common.rs @@ -124,6 +124,26 @@ pub enum BadStoreRequest { ObjectstoreUploadFailed, } +impl BadStoreRequest { + pub fn to_outcome(&self) -> Option { + let discard_reason = match self { + Self::InvalidCompressionContainer(_) => DiscardReason::InvalidCompression, + Self::InvalidMinidump => DiscardReason::InvalidMinidump, + Self::InvalidProsperodump => DiscardReason::InvalidProsperodump, + Self::MissingMinidump => DiscardReason::MissingMinidumpUpload, + Self::Overflow(item_type) => DiscardReason::TooLarge(*item_type), + _ => DiscardReason::Internal, + }; + Some(Outcome::Invalid(discard_reason)) + } +} + +impl From> for BadStoreRequest { + fn from(rejected: Rejected) -> Self { + rejected.into_inner() + } +} + impl From for BadStoreRequest { fn from(value: BytesRejection) -> Self { match value { diff --git a/relay-server/src/endpoints/minidump.rs b/relay-server/src/endpoints/minidump.rs index 467a9a545d7..fa6a7bfd7ea 100644 --- a/relay-server/src/endpoints/minidump.rs +++ b/relay-server/src/endpoints/minidump.rs @@ -24,7 +24,7 @@ use crate::endpoints::common::{self, BadStoreRequest, TextResponse, upload_to_ob use crate::envelope::ContentType::Minidump; use crate::envelope::{AttachmentType, Envelope, Item, ItemType}; use crate::extractors::{RawContentType, RequestMeta}; -use crate::managed::{Counted, Managed}; +use crate::managed::{Managed, ManagedResult}; use crate::middlewares; use crate::service::ServiceState; use crate::services::outcome::{DiscardAttachmentType, DiscardItemType, DiscardReason, Outcome}; @@ -63,10 +63,9 @@ const ZSTD_MAGIC_HEADER: &[u8] = b"\x28\xB5\x2F\xFD"; /// Content types by which standalone uploads can be recognized. const MINIDUMP_RAW_CONTENT_TYPES: &[&str] = &["application/octet-stream", "application/x-dmp"]; -fn validate_minidump(data: &[u8], items: &Managed) -> Result<(), BadStoreRequest> { +fn validate_minidump(data: &[u8]) -> Result<(), BadStoreRequest> { if !data.starts_with(MINIDUMP_MAGIC_HEADER_LE) && !data.starts_with(MINIDUMP_MAGIC_HEADER_BE) { relay_log::trace!("invalid minidump file"); - let _ = items.reject_err(Outcome::Invalid(DiscardReason::InvalidMinidump)); return Err(BadStoreRequest::InvalidMinidump); } @@ -108,11 +107,7 @@ fn decoder_from(minidump_data: Bytes) -> Option> { /// or returns the provided minidump payload untouched if no format where detected. /// /// Returns an `Overflow` error if the decompressed size exceeds `max_size`. -fn decode_minidump( - minidump_data: Bytes, - max_size: usize, - items: &Managed, -) -> Result { +fn decode_minidump(minidump_data: Bytes, max_size: usize) -> Result { let Some(decoder) = decoder_from(minidump_data.clone()) else { // this means we haven't detected any compression container // proceed to process the payload untouched (as a plain minidump). @@ -125,7 +120,6 @@ fn decode_minidump( Ok(decoded) => { if decoded.len() > max_size { let item_type = DiscardItemType::Attachment(DiscardAttachmentType::Minidump); - let _ = items.reject_err(Outcome::Invalid(DiscardReason::TooLarge(item_type))); return Err(BadStoreRequest::Overflow(item_type)); } Ok(Bytes::from(decoded)) @@ -133,7 +127,6 @@ fn decode_minidump( Err(err) => { // we detected a compression container but failed to decode it relay_log::trace!("invalid compression container"); - let _ = items.reject_err(Outcome::Invalid(DiscardReason::InvalidCompression)); Err(BadStoreRequest::InvalidCompressionContainer(err)) } } @@ -290,9 +283,7 @@ async fn multipart_to_envelope( .iter() .position(|item| item.attachment_type() == Some(AttachmentType::Minidump)) .ok_or(BadStoreRequest::MissingMinidump) - .inspect_err(|_| { - let _ = items.reject_err(Outcome::Invalid(DiscardReason::MissingMinidumpUpload)); - })?; + .reject(&items)?; // Doing these operations does not make sense if we already streamed the minidump to objectstore. if !items[minidump_idx].is_attachment_ref() { @@ -300,9 +291,9 @@ async fn multipart_to_envelope( let payload = extract_embedded_minidump(payload.clone()) .await? .unwrap_or(payload); - let payload = decode_minidump(payload, config.max_attachment_size(), &items)?; + let payload = decode_minidump(payload, config.max_attachment_size()).reject(&items)?; - validate_minidump(&payload, &items)?; + validate_minidump(&payload).reject(&items)?; items.modify(|items, records| { let minidump_item = &mut items[minidump_idx]; @@ -423,9 +414,9 @@ async fn raw_minidump_to_envelope( let decoded_payload = decode_minidump( request.extract().await?, state.config().max_attachment_size(), - &item, - )?; - validate_minidump(&decoded_payload, &item)?; + ) + .reject(&item)?; + validate_minidump(&decoded_payload).reject(&item)?; item.modify(|inner, records| { inner.set_payload(Minidump, decoded_payload); records.lenient(DataCategory::Attachment); // decoding the minidump changes its size @@ -517,28 +508,14 @@ mod tests { #[test] fn test_validate_minidump() { - let (items, mut handle) = Managed::for_test(Item::new(ItemType::Attachment)).build(); - let be_minidump = b"PMDMxxxxxx"; - assert!(validate_minidump(be_minidump, &items).is_ok()); + assert!(validate_minidump(be_minidump).is_ok()); let le_minidump = b"MDMPxxxxxx"; - assert!(validate_minidump(le_minidump, &items).is_ok()); + assert!(validate_minidump(le_minidump).is_ok()); let garbage = b"xxxxxx"; - assert!(validate_minidump(garbage, &items).is_err()); - - drop(items); - handle.assert_outcome( - &Outcome::Invalid(DiscardReason::InvalidMinidump), - DataCategory::Attachment, - 1, - ); - handle.assert_outcome( - &Outcome::Invalid(DiscardReason::InvalidMinidump), - DataCategory::AttachmentItem, - 1, - ); + assert!(validate_minidump(garbage).is_err()); } type EncodeFunction = fn(&[u8]) -> Result>; @@ -587,9 +564,7 @@ mod tests { let decoder = decoder_from(compressed).unwrap(); let decoded = run_decoder(decoder); assert!(decoded.is_ok()); - assert!( - validate_minidump(&decoded.unwrap(), &Managed::for_test(()).build().0).is_err() - ); + assert!(validate_minidump(&decoded.unwrap()).is_err()); } Ok(()) @@ -602,12 +577,12 @@ mod tests { let compressed = encode_gzip(&minidump_data)?; // With a limit larger than the decompressed size, decoding should succeed - let result = decode_minidump(compressed.clone(), 200, &Managed::for_test(()).build().0); + let result = decode_minidump(compressed.clone(), 200); assert!(result.is_ok()); assert_eq!(result.unwrap().len(), 100); // With a limit smaller than the decompressed size, decoding should fail with Overflow - let result = decode_minidump(compressed, 50, &Managed::for_test(()).build().0); + let result = decode_minidump(compressed, 50); assert!(matches!(result, Err(BadStoreRequest::Overflow(_)))); Ok(()) diff --git a/relay-server/src/endpoints/playstation.rs b/relay-server/src/endpoints/playstation.rs index 9c9153625c7..06b911e9da1 100644 --- a/relay-server/src/endpoints/playstation.rs +++ b/relay-server/src/endpoints/playstation.rs @@ -15,9 +15,9 @@ use tower_http::limit::RequestBodyLimitLayer; use crate::endpoints::common::{self, BadStoreRequest, TextResponse}; use crate::envelope::ContentType::OctetStream; -use crate::envelope::{AttachmentType, Envelope, Item, Items}; +use crate::envelope::{AttachmentType, Envelope, Item}; use crate::extractors::{RawContentType, RequestMeta}; -use crate::managed::Managed; +use crate::managed::{Managed, ManagedResult}; use crate::middlewares; use crate::service::ServiceState; use crate::services::outcome::{DiscardReason, Outcome}; @@ -165,10 +165,9 @@ fn create_data_request_response() -> DataRequestResponse { } } -fn validate_prosperodump(data: &[u8], items: &Managed) -> Result<(), BadStoreRequest> { +fn validate_prosperodump(data: &[u8]) -> Result<(), BadStoreRequest> { if !data.starts_with(LZ4_MAGIC_HEADER) && !data.starts_with(PROSPERODUMP_MAGIC_HEADER) { relay_log::trace!("invalid prosperodump file"); - let _ = items.reject_err(Outcome::Invalid(DiscardReason::InvalidProsperodump)); return Err(BadStoreRequest::InvalidProsperodump); } @@ -194,11 +193,9 @@ async fn multipart_to_envelope( .iter() .position(|item| item.attachment_type() == Some(AttachmentType::Prosperodump)) .ok_or(BadStoreRequest::MissingProsperodump) - .inspect_err(|_| { - let _ = items.reject_err(Outcome::Invalid(DiscardReason::MissingProsperodumpUpload)); - })?; + .reject(&items)?; let payload = items[prosperodump_idx].payload(); - validate_prosperodump(&payload, &items)?; + validate_prosperodump(&payload).reject(&items)?; items.modify(|items, _| { items[prosperodump_idx].set_payload(OctetStream, payload); }); diff --git a/relay-server/src/managed/managed.rs b/relay-server/src/managed/managed.rs index ad3c691411d..ccfc3539dcf 100644 --- a/relay-server/src/managed/managed.rs +++ b/relay-server/src/managed/managed.rs @@ -16,6 +16,7 @@ use relay_system::Addr; use smallvec::SmallVec; use crate::Envelope; +use crate::endpoints::common::BadStoreRequest; use crate::extractors::RequestMeta; use crate::managed::{Counted, ManagedEnvelope, Quantities}; use crate::services::outcome::{DiscardReason, Outcome, TrackOutcome}; @@ -88,6 +89,14 @@ impl OutcomeError for Infallible { } } +impl OutcomeError for BadStoreRequest { + type Error = Self; + + fn consume(self) -> (Option, Self) { + (self.to_outcome(), self) + } +} + /// A wrapper type which ensures outcomes have been emitted for an error. /// /// [`Managed`] wraps an error in [`Rejected`] once outcomes for have been emitted for the managed From 6f358f8fa94b5e1cf831ded1eb9c59ef197b1b15 Mon Sep 17 00:00:00 2001 From: elramen Date: Thu, 7 May 2026 14:37:59 +0200 Subject: [PATCH 4/5] ref playstation --- relay-server/src/endpoints/common.rs | 3 +++ relay-server/src/endpoints/playstation.rs | 24 +++++++++++------------ 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/relay-server/src/endpoints/common.rs b/relay-server/src/endpoints/common.rs index d0d45f30efe..9b93b9a1e49 100644 --- a/relay-server/src/endpoints/common.rs +++ b/relay-server/src/endpoints/common.rs @@ -129,8 +129,11 @@ impl BadStoreRequest { let discard_reason = match self { Self::InvalidCompressionContainer(_) => DiscardReason::InvalidCompression, Self::InvalidMinidump => DiscardReason::InvalidMinidump, + #[cfg(sentry)] Self::InvalidProsperodump => DiscardReason::InvalidProsperodump, Self::MissingMinidump => DiscardReason::MissingMinidumpUpload, + #[cfg(sentry)] + Self::MissingProsperodump => DiscardReason::MissingProsperodumpUpload, Self::Overflow(item_type) => DiscardReason::TooLarge(*item_type), _ => DiscardReason::Internal, }; diff --git a/relay-server/src/endpoints/playstation.rs b/relay-server/src/endpoints/playstation.rs index 06b911e9da1..7863360abd7 100644 --- a/relay-server/src/endpoints/playstation.rs +++ b/relay-server/src/endpoints/playstation.rs @@ -17,10 +17,10 @@ use crate::endpoints::common::{self, BadStoreRequest, TextResponse}; use crate::envelope::ContentType::OctetStream; use crate::envelope::{AttachmentType, Envelope, Item}; use crate::extractors::{RawContentType, RequestMeta}; -use crate::managed::{Managed, ManagedResult}; +use crate::managed::Managed; use crate::middlewares; use crate::service::ServiceState; -use crate::services::outcome::{DiscardReason, Outcome}; +use crate::services::outcome::DiscardReason; use crate::services::upload::Upload; use crate::utils::{self, AttachmentStrategy}; @@ -189,16 +189,16 @@ async fn multipart_to_envelope( ) .await?; - let prosperodump_idx = items - .iter() - .position(|item| item.attachment_type() == Some(AttachmentType::Prosperodump)) - .ok_or(BadStoreRequest::MissingProsperodump) - .reject(&items)?; - let payload = items[prosperodump_idx].payload(); - validate_prosperodump(&payload).reject(&items)?; - items.modify(|items, _| { - items[prosperodump_idx].set_payload(OctetStream, payload); - }); + items.try_modify(|inner, _| -> Result<(), BadStoreRequest> { + let prosperodump = inner + .iter_mut() + .find(|item| item.attachment_type() == Some(AttachmentType::Prosperodump)) + .ok_or(BadStoreRequest::MissingProsperodump)?; + let payload = prosperodump.payload(); + validate_prosperodump(&payload)?; + prosperodump.set_payload(OctetStream, payload); + Ok(()) + })?; let event_id = common::event_id_from_items(&items)?.unwrap_or_else(EventId::new); let envelope = items.map(|items, records| { From 67519505b714932f068c326e9f7012612d4ef80b Mon Sep 17 00:00:00 2001 From: elramen Date: Thu, 7 May 2026 15:55:36 +0200 Subject: [PATCH 5/5] move down validate_minidump --- relay-server/src/endpoints/minidump.rs | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/relay-server/src/endpoints/minidump.rs b/relay-server/src/endpoints/minidump.rs index fa6a7bfd7ea..413ff026fb3 100644 --- a/relay-server/src/endpoints/minidump.rs +++ b/relay-server/src/endpoints/minidump.rs @@ -293,16 +293,16 @@ async fn multipart_to_envelope( .unwrap_or(payload); let payload = decode_minidump(payload, config.max_attachment_size()).reject(&items)?; - validate_minidump(&payload).reject(&items)?; - - items.modify(|items, records| { + items.try_modify(|items, records| -> Result<(), BadStoreRequest> { let minidump_item = &mut items[minidump_idx]; minidump_item.set_payload(Minidump, payload); records.lenient(DataCategory::Attachment); // decoding the minidump changes its size if let Some(minidump_filename) = minidump_item.filename() { minidump_item.set_filename(remove_container_extension(minidump_filename).to_owned()) } - }); + validate_minidump(&minidump_item.payload())?; + Ok(()) + })?; } let event_id = common::event_id_from_items(&items)?.unwrap_or_else(EventId::new); @@ -411,16 +411,14 @@ async fn raw_minidump_to_envelope( .await .ok_or(BadStoreRequest::ObjectstoreUploadFailed)?; } else { - let decoded_payload = decode_minidump( - request.extract().await?, - state.config().max_attachment_size(), - ) - .reject(&item)?; - validate_minidump(&decoded_payload).reject(&item)?; - item.modify(|inner, records| { - inner.set_payload(Minidump, decoded_payload); + let minidump_data = request.extract().await?; + item.try_modify(|inner, records| -> Result<(), BadStoreRequest> { + let payload = decode_minidump(minidump_data, state.config().max_attachment_size())?; + inner.set_payload(Minidump, payload); records.lenient(DataCategory::Attachment); // decoding the minidump changes its size - }); + validate_minidump(&inner.payload())?; + Ok(()) + })?; }; // Create an envelope with a random event id.