From 258008ad321c7145a91be4a134be505741d9bede Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Mon, 11 May 2026 17:34:01 +0200 Subject: [PATCH 01/12] test(minidump): Upload from external --- tests/integration/test_minidump.py | 37 ++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tests/integration/test_minidump.py b/tests/integration/test_minidump.py index 1fbe24a4232..880c8e49880 100644 --- a/tests/integration/test_minidump.py +++ b/tests/integration/test_minidump.py @@ -984,6 +984,43 @@ def test_minidump_objectstore_uploads( assert minidump.payload.bytes == minidump_content +def test_minidump_objectstore_uploads_external_chain( + mini_sentry, + relay, + relay_with_processing, + attachments_consumer, +): + project_id = 42 + minidump_content = b"MDMP content" + log_content = b"Some log file content" + + project_config = mini_sentry.add_full_project_config(project_id) + project_config["config"].setdefault("features", []).extend( + [ + "projects:relay-minidump-attachment-uploads", + "projects:relay-minidump-uploads", + ] + ) + mini_sentry.global_config["options"]["relay.endpoint-fetch-config.enabled"] = True + + relay = relay(relay_with_processing(), external=True) + project_config["config"]["trustedRelays"] = list(relay.iter_public_keys()) + + attachments_consumer = attachments_consumer() + + response = relay.send_minidump( + project_id=project_id, + files=[ + (MINIDUMP_ATTACHMENT_NAME, "minidump.dmp", minidump_content), + ("logs", "log.txt", log_content), + ], + ) + assert response.ok + + event, _ = attachments_consumer.get_event() + assert UUID(event["event_id"]) == UUID(response.text) + + def test_size_limits_multipart_chunked(mini_sentry, relay): project_id = 42 From 446608b0c2366f72ab2c3ab82eb9a9ac4affabd2 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Tue, 12 May 2026 15:19:33 +0200 Subject: [PATCH 02/12] types --- relay-server/src/endpoints/upload.rs | 18 +-- .../src/processing/utils/attachments.rs | 11 +- relay-server/src/services/store.rs | 4 +- relay-server/src/services/upload.rs | 127 +++++++++++++----- 4 files changed, 107 insertions(+), 53 deletions(-) diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index 9ddf3afb881..57fa5f46d16 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -30,7 +30,9 @@ use crate::service::ServiceState; #[cfg(feature = "processing")] use crate::services::objectstore; use crate::services::projects::cache::Project; -use crate::services::upload::{self, ByteStream, SignedLocation}; +use crate::services::upload::{ + self, ByteStream, Final, LocationQueryParams, Provisional, SignedLocation, UploadLength, +}; use crate::services::upstream::UpstreamRequestError; use crate::utils::{ApiErrorResponse, MeteredStream}; use crate::utils::{BoundedStream, find_error_source, tus}; @@ -126,7 +128,7 @@ impl IntoResponse for Error { } } -impl IntoResponse for SignedLocation { +impl IntoResponse for SignedLocation { fn into_response(self) -> Response { let mut headers = tus::response_headers(); match self.into_header_value() { @@ -196,7 +198,7 @@ async fn handle_patch( meta: RequestMeta, headers: HeaderMap, Path(upload::LocationPath { project_id, key }): Path, - Query(upload::LocationQueryParams { length, signature }): Query, + Query(LocationQueryParams { length, signature }): Query>, body: Body, ) -> axum::response::Result { relay_log::trace!("Checking project fetching killswitch"); @@ -226,7 +228,7 @@ async fn handle_patch( .ok_or(StatusCode::SERVICE_UNAVAILABLE)?; relay_log::trace!("Checking request"); - let scoping = check_request(&state, meta, length, project).await?; + let scoping = check_request(&state, meta, length.value(), project).await?; let stream = body .into_data_stream() @@ -234,7 +236,7 @@ async fn handle_patch( .boxed(); let stream = MeteredStream::new(stream, "upload"); - let (lower_bound, upper_bound) = match length { + let (lower_bound, upper_bound) = match length.value() { None => (1, config.max_upload_size()), Some(u) => (u, u), }; @@ -272,7 +274,7 @@ async fn create( state: &ServiceState, scoping: Scoping, upload_length: Option, -) -> Result { +) -> Result, Error> { let location = state .upload() .send(upload::Create { @@ -287,9 +289,9 @@ async fn create( async fn upload( state: &ServiceState, scoping: Scoping, - location: SignedLocation, + location: SignedLocation, stream: BoundedStream>, -) -> Result { +) -> Result, Error> { let location = state .upload() .send(upload::Stream { diff --git a/relay-server/src/processing/utils/attachments.rs b/relay-server/src/processing/utils/attachments.rs index f5a08e10883..89d3a5be63a 100644 --- a/relay-server/src/processing/utils/attachments.rs +++ b/relay-server/src/processing/utils/attachments.rs @@ -40,6 +40,8 @@ fn validate(item: &Item, config: &Config) -> Result<(), ProcessingError> { #[cfg(feature = "processing")] { + use crate::services::upload::{Final, SignedLocation}; + if !item.is_attachment_ref() { return Ok(()); } @@ -47,16 +49,13 @@ fn validate(item: &Item, config: &Config) -> Result<(), ProcessingError> { let payload = item.payload(); let payload: AttachmentPlaceholder = serde_json::from_slice(&payload).map_err(|_| ProcessingError::InvalidAttachmentRef)?; - let signed_location = - crate::services::upload::SignedLocation::try_from_str(payload.location) - .ok_or(ProcessingError::InvalidAttachmentRef)?; + let signed_location: SignedLocation = SignedLocation::try_from_str(payload.location) + .ok_or(ProcessingError::InvalidAttachmentRef)?; // 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)?; - let signed_length = location - .length - .ok_or(ProcessingError::InvalidAttachmentRef)?; + let signed_length = location.length.into_inner(); match item.attachment_body_size() == signed_length { true => Ok(()), diff --git a/relay-server/src/services/store.rs b/relay-server/src/services/store.rs index 19b3aa401fe..7c789c3d0fb 100644 --- a/relay-server/src/services/store.rs +++ b/relay-server/src/services/store.rs @@ -39,7 +39,7 @@ use crate::metrics::{ArrayEncoding, BucketEncoder, MetricOutcomes}; use crate::service::ServiceError; use crate::services::global_config::GlobalConfigHandle; use crate::services::outcome::{DiscardReason, Outcome, TrackOutcome}; -use crate::services::upload::SignedLocation; +use crate::services::upload::{Final, SignedLocation}; use crate::statsd::{RelayCounters, RelayGauges, RelayTimers}; use crate::utils::{self, FormDataIter}; @@ -1031,7 +1031,7 @@ impl StoreService { let payload = item.payload(); let placeholder: AttachmentPlaceholder<'_> = serde_json::from_slice(&payload).map_err(|_| StoreError::InvalidAttachmentRef)?; - let location = SignedLocation::try_from_str(placeholder.location) + let location = SignedLocation::::try_from_str(placeholder.location) .ok_or(StoreError::InvalidAttachmentRef)? .verify(Utc::now(), &self.config) .map_err(|_| StoreError::InvalidAttachmentRef)?; diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index f678ff51fbd..8e20f3d4bd1 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -76,12 +76,12 @@ pub enum Upload { /// Creates an upload resource. /// /// Returns the trusted identifier of the upload. - Create(Create, Sender>), + Create(Create, Sender, Error>>), /// Upload a stream of bytes for a given location. /// /// The service also returns the signed location. This is redundant, but creates a simpler /// flow for the caller side. - Upload(Stream, Sender>), + Upload(Stream, Sender, Error>>), } impl Interface for Upload {} @@ -106,23 +106,26 @@ pub struct Stream { /// The organization & project that the stream belongs to. pub scoping: Scoping, /// The location to upload to. - pub location: SignedLocation, + pub location: SignedLocation, /// The body to be uploaded to objectstore, with length validation. pub stream: BoundedStream>, } impl FromMessage for Upload { - type Response = AsyncResponse>; + type Response = AsyncResponse, Error>>; - fn from_message(message: Create, sender: Sender>) -> Self { + fn from_message( + message: Create, + sender: Sender, Error>>, + ) -> Self { Self::Create(message, sender) } } impl FromMessage for Upload { - type Response = AsyncResponse>; + type Response = AsyncResponse, Error>>; - fn from_message(message: Stream, sender: Sender>) -> Self { + fn from_message(message: Stream, sender: Sender, Error>>) -> Self { Self::Upload(message, sender) } } @@ -187,7 +190,10 @@ enum Backend { } impl Service { - async fn create(&self, Create { scoping, length }: Create) -> Result { + async fn create( + &self, + Create { scoping, length }: Create, + ) -> Result, Error> { match &self.backend { Backend::Upstream { addr } => { let (request, rx) = UploadRequest::create(scoping, length); @@ -202,14 +208,14 @@ impl Service { Location { project_id: scoping.project_id, key, - length, + length: Provisional(length), } .try_sign(config) } } } - async fn upload(&self, stream: Stream) -> Result { + async fn upload(&self, stream: Stream) -> Result, Error> { match &self.backend { Backend::Upstream { addr } => { let (request, rx) = UploadRequest::upload(stream); @@ -232,7 +238,7 @@ impl Service { } = location.verify(received, config)?; debug_assert_eq!(scoping.project_id, project_id); - debug_assert!(stream.length().is_none_or(|l| Some(l) == length)); + debug_assert!(stream.length().is_none_or(|l| Some(l) == length.value())); let byte_counter = stream.byte_counter(); let key = addr @@ -245,7 +251,7 @@ impl Service { .await .map_err(Error::ServiceUnavailable)?? .into_inner(); - let length = Some(byte_counter.get()); + let length = Final(byte_counter.get()); Location { project_id, @@ -257,10 +263,11 @@ impl Service { } } - async fn timeout>>( - &self, - future: F, - ) -> Result { + async fn timeout(&self, future: F) -> Result, Error> + where + L: UploadLength, + F: IntoFuture, Error>>, + { tokio::time::timeout(self.timeout, future).await? } } @@ -283,11 +290,57 @@ impl SimpleService for Service { impl LoadShed for Service { fn handle_loadshed(&self, message: Upload) { match message { - Upload::Create(_, tx) | Upload::Upload(_, tx) => tx.send(Err(Error::LoadShed)), + Upload::Create(_, tx) => tx.send(Err(Error::LoadShed)), + Upload::Upload(_, tx) => tx.send(Err(Error::LoadShed)), } } } +/// An interface for known or unknown upload lengths. +/// +/// This allows code sharing between [`Provisional`] and [`Final`] upload locations. +pub trait UploadLength: for<'de> Deserialize<'de> { + fn value(&self) -> Option; +} + +/// A provisional upload length which may or may not yet be known. +/// +/// /// See also [`Final`]. +#[derive(Clone, Copy, Deserialize)] +pub struct Provisional(Option); + +impl Provisional { + /// Defines a provisional upload length. + pub fn new(value: Option) -> Self { + Self(value) + } +} + +impl UploadLength for Provisional { + fn value(&self) -> Option { + self.0.clone() + } +} + +/// A final upload length that represents the actual amount of bytes uploaded to objectstore. +/// +/// See also [`Provisional`]. +#[derive(Clone, Copy, Deserialize)] +pub struct Final(usize); + +impl Final { + /// Get the value. + pub fn into_inner(self) -> usize { + self.0 + } +} + +impl UploadLength for Final { + fn value(&self) -> Option { + Some(self.0) + } +} + /// An identifier for the upload. /// /// The location can be converted into a URI to be put in the `Location` HTTP header @@ -296,30 +349,30 @@ impl LoadShed for Service { /// Calling [`Self::try_sign`] appends a `&signature=` query parameter that can later be used /// to validate whether the URI (especially the length) has been tempered with. #[derive(Debug)] -pub struct Location { +pub struct Location { /// Sentry project ID. pub project_id: ProjectId, /// Objectstore identifier. pub key: String, /// Value of the `Upload-Length` header. `None` if `Upload-Defer-Length: 1`. - pub length: Option, + pub length: L, } -impl Location { +impl Location { fn as_uri(&self) -> String { let Location { project_id, key, - length, + length: _, } = self; - match length { + match self.length.value() { Some(length) => format!("/api/{project_id}/upload/{key}/?length={length}"), None => format!("/api/{project_id}/upload/{key}/"), } } #[cfg(feature = "processing")] - fn try_sign(self, config: &Config) -> Result { + fn try_sign(self, config: &Config) -> Result, Error> { let uri = self.as_uri(); let signature = config .credentials() @@ -349,28 +402,24 @@ pub struct LocationPath { /// Query parameters for the upload endpoint. #[derive(Debug, Deserialize)] -pub struct LocationQueryParams { - pub length: Option, +#[serde(bound(deserialize = "L: UploadLength"))] +pub struct LocationQueryParams { + pub length: L, pub signature: String, } /// A verifiable [`Location`] signed by this Relay or an upstream Relay. #[derive(Debug)] -pub struct SignedLocation { - location: Location, +pub struct SignedLocation { + location: Location, signature: Signature, } -impl SignedLocation { +impl SignedLocation { /// Creates an unverified location from path and query params. /// /// Call `verify` to make sure the signature is correct. - pub fn from_parts( - project_id: ProjectId, - key: String, - length: Option, - signature: String, - ) -> Self { + pub fn from_parts(project_id: ProjectId, key: String, length: L, signature: String) -> Self { // TODO: forward compat: allow other query params? Self { location: Location { @@ -393,7 +442,11 @@ impl SignedLocation { signature, } = self; let mut uri = location.as_uri(); - uri.push(if location.length.is_some() { '&' } else { '?' }); // TODO: brittle. + uri.push(if location.length.value().is_some() { + '&' + } else { + '?' + }); // TODO: brittle. uri.push_str("signature="); uri.push_str(&signature.to_string()); uri @@ -403,7 +456,7 @@ impl SignedLocation { /// /// Fails if the signature is outdated or incorrect. #[cfg(feature = "processing")] - pub fn verify(self, received: DateTime, config: &Config) -> Result { + pub fn verify(self, received: DateTime, config: &Config) -> Result, Error> { let public_key = config.public_key().ok_or(Error::SigningFailed)?; let is_valid = self.signature.verify( self.location.as_uri().as_bytes(), @@ -461,7 +514,7 @@ enum RequestKind { length: Option, }, Upload { - location: SignedLocation, + location: SignedLocation, stream: Option>>, encoding: HttpEncoding, }, From ead27e705b695a4f96c2fb12188d097d4736af68 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Tue, 12 May 2026 16:10:33 +0200 Subject: [PATCH 03/12] untrust --- relay-server/src/endpoints/upload.rs | 4 +-- relay-server/src/utils/tus.rs | 42 +++++++++------------------- tests/integration/test_minidump.py | 7 +++-- 3 files changed, 19 insertions(+), 34 deletions(-) diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index 57fa5f46d16..c607f11f50d 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -77,7 +77,6 @@ impl IntoResponse for Error { } let status = match self { - Error::Tus(tus::Error::DeferLengthNotAllowed) => StatusCode::FORBIDDEN, Error::Tus(_) => StatusCode::BAD_REQUEST, Error::Request(error) => return error.into_response(), Error::SendError(_) => StatusCode::INTERNAL_SERVER_ERROR, @@ -159,8 +158,7 @@ async fn handle_post( } relay_log::trace!("Validating headers"); - let upload_length = tus::validate_post_headers(&headers, meta.request_trust().is_trusted()) - .map_err(Error::from)?; + let upload_length = tus::validate_post_headers(&headers).map_err(Error::from)?; let config = state.config(); if upload_length.is_some_and(|len| len > config.max_upload_size()) { diff --git a/relay-server/src/utils/tus.rs b/relay-server/src/utils/tus.rs index aa3582c8519..333592ac1f4 100644 --- a/relay-server/src/utils/tus.rs +++ b/relay-server/src/utils/tus.rs @@ -29,9 +29,6 @@ pub enum Error { /// The `Upload-Offset` header is missing or invalid #[error("expected Upload-Offset: 0, got: {0:?}")] UploadOffset(Option), - /// The `Upload-Defer-Length` header is not allowed for external/untrusted requests. - #[error("Upload-Defer-Length not allowed")] - DeferLengthNotAllowed, /// The `Content-Type` header is not what TUS expects. #[error("expected Content-Type: {expected}, got: {received}")] ContentType { @@ -78,10 +75,7 @@ const EXPECTED_CONTENT_TYPE_STR: &str = "application/offset+octet-stream"; /// Validates TUS protocol headers and returns a subset of parsed values. /// /// Returns the declared `Upload-Length`. -pub fn validate_post_headers( - headers: &HeaderMap, - allow_defer_length: bool, -) -> Result, Error> { +pub fn validate_post_headers(headers: &HeaderMap) -> Result, Error> { let tus_version = headers.get(TUS_RESUMABLE); if tus_version != Some(&TUS_VERSION) { return Err(Error::Version( @@ -102,10 +96,9 @@ pub fn validate_post_headers( // Exactly one of Upload-Length and Upload-Defer-Length must be present. // Upload-Defer-Length is only accepted if its value is 1 (as demanded by the TUS protocol) // and `allow_defer_length` is true (i.e. the sender is trusted/internal). - let upload_length = match (upload_length, upload_defer_length, allow_defer_length) { - (Some(u), None, _) => Ok(Some(u)), - (None, Some(1), true) => Ok(None), - (None, Some(1), false) => Err(Error::DeferLengthNotAllowed), + let upload_length = match (upload_length, upload_defer_length) { + (Some(u), None) => Ok(Some(u)), + (None, Some(1)) => Ok(None), _ => Err(Error::UploadLength { upload_length, upload_defer_length, @@ -183,7 +176,7 @@ mod tests { #[test] fn test_validate_tus_headers_missing_version() { let headers = HeaderMap::new(); - let result = validate_post_headers(&headers, false); + let result = validate_post_headers(&headers); assert!(matches!(result, Err(Error::Version(_)))); } @@ -191,7 +184,7 @@ mod tests { fn test_validate_tus_headers_missing_length() { let mut headers = HeaderMap::new(); headers.insert(TUS_RESUMABLE, HeaderValue::from_static("1.0.0")); - let result = validate_post_headers(&headers, false); + let result = validate_post_headers(&headers); assert!(matches!(result, Err(Error::UploadLength { .. }))); } @@ -202,7 +195,7 @@ mod tests { headers.insert(hyper::header::CONTENT_LENGTH, 1024.into()); headers.insert(hyper::header::CONTENT_TYPE, EXPECTED_CONTENT_TYPE); headers.insert(UPLOAD_LENGTH, HeaderValue::from_static("1024")); - let result = validate_post_headers(&headers, false); + let result = validate_post_headers(&headers); assert!(matches!(result, Err(Error::ContentType { .. }))); } @@ -211,7 +204,7 @@ mod tests { let mut headers = HeaderMap::new(); headers.insert(TUS_RESUMABLE, HeaderValue::from_static("1.0.0")); headers.insert(UPLOAD_LENGTH, HeaderValue::from_static("1024")); - let result = validate_post_headers(&headers, false); + let result = validate_post_headers(&headers); assert_eq!(result.unwrap(), Some(1024)); } @@ -224,7 +217,7 @@ mod tests { HeaderValue::from_static("chunked"), ); headers.insert(UPLOAD_LENGTH, HeaderValue::from_static("1024")); - let result = validate_post_headers(&headers, false); + let result = validate_post_headers(&headers); assert_eq!(result.unwrap(), Some(1024)); } @@ -233,12 +226,12 @@ mod tests { let mut headers = HeaderMap::new(); headers.insert(TUS_RESUMABLE, HeaderValue::from_static("0.2.0")); headers.insert(UPLOAD_LENGTH, HeaderValue::from_static("1024")); - let result = validate_post_headers(&headers, false); + let result = validate_post_headers(&headers); assert!(matches!(result, Err(Error::Version(_)))); } #[test] - fn test_validate_tus_headers_valid_deferred_length_from_trusted_source() { + fn test_validate_tus_headers_valid_deferred_length() { let mut headers = HeaderMap::new(); headers.insert(TUS_RESUMABLE, HeaderValue::from_static("1.0.0")); headers.insert( @@ -246,25 +239,16 @@ mod tests { HeaderValue::from_static("chunked"), ); headers.insert(UPLOAD_DEFER_LENGTH, HeaderValue::from_static("1")); - let result = validate_post_headers(&headers, true); + let result = validate_post_headers(&headers); assert!(matches!(result, Ok(None))); } - #[test] - fn test_validate_tus_headers_valid_deferred_length_from_untrusted_source() { - let mut headers = HeaderMap::new(); - headers.insert(TUS_RESUMABLE, HeaderValue::from_static("1.0.0")); - headers.insert(UPLOAD_DEFER_LENGTH, HeaderValue::from_static("1")); - let result = validate_post_headers(&headers, false); - assert!(matches!(result, Err(Error::DeferLengthNotAllowed))); - } - #[test] fn test_validate_tus_headers_invalid_deferred_length() { let mut headers = HeaderMap::new(); headers.insert(TUS_RESUMABLE, HeaderValue::from_static("1.0.0")); headers.insert(UPLOAD_DEFER_LENGTH, HeaderValue::from_static("2")); - let result = validate_post_headers(&headers, true); + let result = validate_post_headers(&headers); assert!(matches!(result, Err(Error::UploadLength { .. }))); } diff --git a/tests/integration/test_minidump.py b/tests/integration/test_minidump.py index 880c8e49880..4242783da15 100644 --- a/tests/integration/test_minidump.py +++ b/tests/integration/test_minidump.py @@ -997,8 +997,9 @@ def test_minidump_objectstore_uploads_external_chain( project_config = mini_sentry.add_full_project_config(project_id) project_config["config"].setdefault("features", []).extend( [ - "projects:relay-minidump-attachment-uploads", - "projects:relay-minidump-uploads", + # "projects:relay-minidump-uploads", # TODO: why 400 here (though not related to PR) + "projects:relay-minidump-attachment-uploads", # TODO: why 400 here (though not related to PR) + "projects:relay-upload-endpoint", ] ) mini_sentry.global_config["options"]["relay.endpoint-fetch-config.enabled"] = True @@ -1017,6 +1018,8 @@ def test_minidump_objectstore_uploads_external_chain( ) assert response.ok + attachment, _ = attachments_consumer.get_individual_attachment() + attachment, _ = attachments_consumer.get_individual_attachment() event, _ = attachments_consumer.get_event() assert UUID(event["event_id"]) == UUID(response.text) From d121d95c0322d0a375493a3602b0ae26aef7f92a Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Tue, 12 May 2026 21:10:47 +0200 Subject: [PATCH 04/12] fix --- relay-server/src/services/upload.rs | 37 +++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index 83bbd654452..ffe1ebcc211 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -450,13 +450,41 @@ pub struct LocationPath { } /// Query parameters for the upload endpoint. -#[derive(Debug, Deserialize)] -#[serde(bound(deserialize = "L: UploadLength"))] +#[derive(Debug)] pub struct LocationQueryParams { pub length: L, pub signature: String, } +#[derive(Deserialize)] +struct Helper(LocationQueryParams); + +impl<'de> Deserialize<'de> for LocationQueryParams { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + Helper::deserialize(deserializer).map(|helper| helper.0) + } +} + +impl<'de> Deserialize<'de> for LocationQueryParams { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let LocationQueryParams:: { length, signature } = + Helper::deserialize(deserializer)?.0; + let Some(length) = length.value() else { + return Err(serde::de::Error::custom("missing length")); + }; + Ok(Self { + length: Final(length), + signature, + }) + } +} + /// A verifiable [`Location`] signed by this Relay or an upstream Relay. #[derive(Debug)] pub struct SignedLocation { @@ -518,7 +546,12 @@ impl SignedLocation { false => Err(Error::InvalidSignature), } } +} +impl SignedLocation +where + LocationQueryParams: for<'de> Deserialize<'de>, +{ fn try_from_response(response: Response) -> Result { match response.0.error_for_status() { Ok(response) => { From 3081790d24b519097f507de3f153d90d37b2dd19 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Thu, 21 May 2026 13:18:18 +0200 Subject: [PATCH 05/12] fix: deserialize --- relay-server/src/endpoints/upload.rs | 1 + relay-server/src/services/upload.rs | 23 ++++++++++++++--------- tests/integration/test_minidump.py | 19 ++++++++++++------- 3 files changed, 27 insertions(+), 16 deletions(-) diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index 0ac2eb06fbd..a1b07245b13 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -176,6 +176,7 @@ async fn handle_post( let scoping = check_request(&state, meta, upload_length, project).await?; // Unconditionally create the upload location: + relay_log::trace!("Creating upload location"); let result = create(&state, scoping, upload_length).await; let location = result.inspect_err(|e| { relay_log::warn!(error = e as &dyn std::error::Error, "create failed"); diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index ffe1ebcc211..8eb1ca904a6 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -355,7 +355,7 @@ pub trait UploadLength: for<'de> Deserialize<'de> { /// A provisional upload length which may or may not yet be known. /// /// /// See also [`Final`]. -#[derive(Clone, Copy, Deserialize)] +#[derive(Debug, Clone, Copy, Deserialize)] pub struct Provisional(Option); impl Provisional { @@ -367,14 +367,14 @@ impl Provisional { impl UploadLength for Provisional { fn value(&self) -> Option { - self.0.clone() + self.0 } } /// A final upload length that represents the actual amount of bytes uploaded to objectstore. /// /// See also [`Provisional`]. -#[derive(Clone, Copy, Deserialize)] +#[derive(Debug, Clone, Copy, Deserialize)] pub struct Final(usize); impl Final { @@ -457,14 +457,20 @@ pub struct LocationQueryParams { } #[derive(Deserialize)] -struct Helper(LocationQueryParams); +struct Helper { + length: Option, + signature: String, +} impl<'de> Deserialize<'de> for LocationQueryParams { fn deserialize(deserializer: D) -> Result where D: serde::Deserializer<'de>, { - Helper::deserialize(deserializer).map(|helper| helper.0) + Helper::deserialize(deserializer).map(|Helper { length, signature }| Self { + length: Provisional(length), + signature, + }) } } @@ -473,9 +479,8 @@ impl<'de> Deserialize<'de> for LocationQueryParams { where D: serde::Deserializer<'de>, { - let LocationQueryParams:: { length, signature } = - Helper::deserialize(deserializer)?.0; - let Some(length) = length.value() else { + let Helper { length, signature } = Helper::deserialize(deserializer)?; + let Some(length) = length else { return Err(serde::de::Error::custom("missing length")); }; Ok(Self { @@ -548,7 +553,7 @@ impl SignedLocation { } } -impl SignedLocation +impl SignedLocation where LocationQueryParams: for<'de> Deserialize<'de>, { diff --git a/tests/integration/test_minidump.py b/tests/integration/test_minidump.py index 9365148b5f9..b96ed0cfb31 100644 --- a/tests/integration/test_minidump.py +++ b/tests/integration/test_minidump.py @@ -991,6 +991,12 @@ def test_minidump_objectstore_uploads_external_chain( relay_with_processing, attachments_consumer, ): + """Uploads with `Defer-Length: 1` are accepted from untrusted relays""" + mini_sentry.global_config["options"][ + "relay.objectstore-attachments.sample-rate" + ] = 1.0 + mini_sentry.global_config["options"]["relay.endpoint-fetch-config.enabled"] = True + project_id = 42 minidump_content = b"MDMP content" log_content = b"Some log file content" @@ -998,12 +1004,10 @@ def test_minidump_objectstore_uploads_external_chain( project_config = mini_sentry.add_full_project_config(project_id) project_config["config"].setdefault("features", []).extend( [ - # "projects:relay-minidump-uploads", # TODO: why 400 here (though not related to PR) - "projects:relay-minidump-attachment-uploads", # TODO: why 400 here (though not related to PR) + "projects:relay-minidump-attachment-uploads", "projects:relay-upload-endpoint", ] ) - mini_sentry.global_config["options"]["relay.endpoint-fetch-config.enabled"] = True relay = relay(relay_with_processing(), external=True) project_config["config"]["trustedRelays"] = list(relay.iter_public_keys()) @@ -1019,10 +1023,11 @@ def test_minidump_objectstore_uploads_external_chain( ) assert response.ok - attachment, _ = attachments_consumer.get_individual_attachment() - attachment, _ = attachments_consumer.get_individual_attachment() - event, _ = attachments_consumer.get_event() - assert UUID(event["event_id"]) == UUID(response.text) + _, unpacked = attachments_consumer.get_message() + assert {att["name"] for att in unpacked["attachments"]} == { + "log.txt", + "minidump.dmp", + } def test_minidump_objectstore_errors( From 086aa744fc5b61fc31223fd3541d72ab3982f627 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Thu, 21 May 2026 13:30:57 +0200 Subject: [PATCH 06/12] ref --- relay-server/src/services/upload.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index 8eb1ca904a6..cd2a706d83c 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -412,9 +412,9 @@ impl Location { let Location { project_id, key, - length: _, + length, } = self; - match self.length.value() { + match length.value() { Some(length) => format!("/api/{project_id}/upload/{key}/?length={length}"), None => format!("/api/{project_id}/upload/{key}/"), } From af63465d2b8045729611e54c06dbba7245cd7ff8 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Thu, 21 May 2026 13:33:49 +0200 Subject: [PATCH 07/12] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b527711d558..169ce0dad3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,7 @@ - Enable compression for forwarded uploads. ([#5965](https://github.com/getsentry/relay/pull/5965)) - Change the default partitioning for the envelope buffer from semantic to round-robin. ([#5967](https://github.com/getsentry/relay/pull/5967)) - Enable retries for upload requests to upstream. ([#5975](https://github.com/getsentry/relay/pull/5975)) +- Always allow `Upload-Defer-Length: 1` on the `/upload` endpoint. ([#5977](https://github.com/getsentry/relay/pull/5977)]) ## 26.4.2 From 0fbc612eff76424bb2aa9e625f96cb0727232ae8 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Thu, 21 May 2026 13:35:38 +0200 Subject: [PATCH 08/12] fix changelog --- CHANGELOG.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 169ce0dad3f..e1957268f0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ - Obtain PII values for `SpanData` fields from `sentry-conventions`. ([#5997](https://github.com/getsentry/relay/pull/5997)) - Add `sentry.dsc.transaction` and `sentry.dsc.trace_id` to all spans. ([#6001](https://github.com/getsentry/relay/pull/6001), [#6004](https://github.com/getsentry/relay/pull/6004), [#6008](https://github.com/getsentry/relay/pull/6008)) +**Internal**: + +- Always allow `Upload-Defer-Length: 1` on the `/upload` endpoint. ([#5977](https://github.com/getsentry/relay/pull/5977)) + ## 26.5.0 **Features**: @@ -38,7 +42,7 @@ - Enable compression for forwarded uploads. ([#5965](https://github.com/getsentry/relay/pull/5965)) - Change the default partitioning for the envelope buffer from semantic to round-robin. ([#5967](https://github.com/getsentry/relay/pull/5967)) - Enable retries for upload requests to upstream. ([#5975](https://github.com/getsentry/relay/pull/5975)) -- Always allow `Upload-Defer-Length: 1` on the `/upload` endpoint. ([#5977](https://github.com/getsentry/relay/pull/5977)]) + ## 26.4.2 From 2d181f10c1e451c23e0c0942d1ca7655f958d6b0 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Thu, 21 May 2026 16:49:51 +0200 Subject: [PATCH 09/12] simplify --- CHANGELOG.md | 1 - relay-server/src/services/upload.rs | 69 +++++++++++++---------------- 2 files changed, 32 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e1957268f0a..2475a7565b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,7 +43,6 @@ - Change the default partitioning for the envelope buffer from semantic to round-robin. ([#5967](https://github.com/getsentry/relay/pull/5967)) - Enable retries for upload requests to upstream. ([#5975](https://github.com/getsentry/relay/pull/5975)) - ## 26.4.2 **Features**: diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index cd2a706d83c..3a4dec6a441 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -356,6 +356,7 @@ pub trait UploadLength: for<'de> Deserialize<'de> { /// /// /// See also [`Final`]. #[derive(Debug, Clone, Copy, Deserialize)] +#[serde(transparent)] pub struct Provisional(Option); impl Provisional { @@ -398,7 +399,7 @@ impl UploadLength for Final { /// Calling [`Self::try_sign`] appends a `&signature=` query parameter that can later be used /// to validate whether the URI (especially the length) has been tempered with. #[derive(Debug)] -pub struct Location { +pub struct Location { /// Sentry project ID. pub project_id: ProjectId, /// Objectstore identifier. @@ -450,46 +451,13 @@ pub struct LocationPath { } /// Query parameters for the upload endpoint. -#[derive(Debug)] +#[derive(Debug, Deserialize)] +#[serde(bound = "L: UploadLength")] pub struct LocationQueryParams { pub length: L, pub signature: String, } -#[derive(Deserialize)] -struct Helper { - length: Option, - signature: String, -} - -impl<'de> Deserialize<'de> for LocationQueryParams { - fn deserialize(deserializer: D) -> Result - where - D: serde::Deserializer<'de>, - { - Helper::deserialize(deserializer).map(|Helper { length, signature }| Self { - length: Provisional(length), - signature, - }) - } -} - -impl<'de> Deserialize<'de> for LocationQueryParams { - fn deserialize(deserializer: D) -> Result - where - D: serde::Deserializer<'de>, - { - let Helper { length, signature } = Helper::deserialize(deserializer)?; - let Some(length) = length else { - return Err(serde::de::Error::custom("missing length")); - }; - Ok(Self { - length: Final(length), - signature, - }) - } -} - /// A verifiable [`Location`] signed by this Relay or an upstream Relay. #[derive(Debug)] pub struct SignedLocation { @@ -553,8 +521,9 @@ impl SignedLocation { } } -impl SignedLocation +impl SignedLocation where + L: UploadLength, LocationQueryParams: for<'de> Deserialize<'de>, { fn try_from_response(response: Response) -> Result { @@ -774,3 +743,29 @@ where HttpEncoding::Zstd => ReaderStream::new(ZstdEncoder::new(reader)).boxed(), } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parse_location_incomplete() { + let json = r#"{"signature": "foo"}"#; + + // Can only parse provisional: + let provisional: LocationQueryParams = serde_json::from_str(json).unwrap(); + assert!(provisional.length.0.is_none()); + assert!(serde_json::from_str::>(json).is_err()); + } + + #[test] + fn parse_location_complete() { + let json = r#"{"signature": "foo", "length": 123}"#; + + // Can only parse provisional: + let provisional: LocationQueryParams = serde_json::from_str(json).unwrap(); + assert_eq!(provisional.length.0, Some(123)); + let full: LocationQueryParams = serde_json::from_str(json).unwrap(); + assert_eq!(full.length.0, 123); + } +} From d60db31cbdfcef3e23915484864bb90aa5fa537a Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 22 May 2026 09:43:43 +0200 Subject: [PATCH 10/12] update integration test --- tests/integration/test_upload.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/integration/test_upload.py b/tests/integration/test_upload.py index 5bc405de0ad..c466ddf3e5c 100644 --- a/tests/integration/test_upload.py +++ b/tests/integration/test_upload.py @@ -476,14 +476,10 @@ def test_upload_with_deferred_length( }, ) - expected_status_code = 403 if defer_length_value == "1" else 400 - assert response.status_code == expected_status_code if defer_length_value == "1": - assert response.json() == { - "detail": "TUS protocol error: Upload-Defer-Length not allowed", - "causes": ["Upload-Defer-Length not allowed"], - } + assert response.status_code == 201 else: + assert response.status_code == 400 assert response.json() == { "detail": ( "TUS protocol error: expected Upload-Length or Upload-Defer-Length=1, " From 66d261698be0be1201978adafd3158223e635fd0 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 22 May 2026 09:48:14 +0200 Subject: [PATCH 11/12] use serde_urlencoded in tests --- relay-server/src/services/upload.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index 3a4dec6a441..e7eb5b2ea6e 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -750,22 +750,24 @@ mod tests { #[test] fn parse_location_incomplete() { - let json = r#"{"signature": "foo"}"#; + let url = "signature=foo"; // Can only parse provisional: - let provisional: LocationQueryParams = serde_json::from_str(json).unwrap(); + let provisional: LocationQueryParams = + serde_urlencoded::from_str(url).unwrap(); assert!(provisional.length.0.is_none()); - assert!(serde_json::from_str::>(json).is_err()); + assert!(serde_urlencoded::from_str::>(url).is_err()); } #[test] fn parse_location_complete() { - let json = r#"{"signature": "foo", "length": 123}"#; + let json = r#"signature=foo&length=123"#; // Can only parse provisional: - let provisional: LocationQueryParams = serde_json::from_str(json).unwrap(); + let provisional: LocationQueryParams = + serde_urlencoded::from_str(json).unwrap(); assert_eq!(provisional.length.0, Some(123)); - let full: LocationQueryParams = serde_json::from_str(json).unwrap(); + let full: LocationQueryParams = serde_urlencoded::from_str(json).unwrap(); assert_eq!(full.length.0, 123); } } From 23f6990db431b98d498f08ff5ea198748b13bb19 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 22 May 2026 09:55:45 +0200 Subject: [PATCH 12/12] rm dead code --- relay-server/src/services/upload.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index e7eb5b2ea6e..6cba1fbc74e 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -359,13 +359,6 @@ pub trait UploadLength: for<'de> Deserialize<'de> { #[serde(transparent)] pub struct Provisional(Option); -impl Provisional { - /// Defines a provisional upload length. - pub fn new(value: Option) -> Self { - Self(value) - } -} - impl UploadLength for Provisional { fn value(&self) -> Option { self.0