From e2740e86c82367dce3cd377e73bbfc6e05a98569 Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Tue, 3 Sep 2024 09:24:13 +0200 Subject: [PATCH 1/5] ref(server): Use multer directly --- Cargo.lock | 21 +-------- Cargo.toml | 2 +- relay-server/Cargo.toml | 7 +-- relay-server/src/endpoints/attachments.rs | 26 ++++------- relay-server/src/endpoints/common.rs | 5 +- relay-server/src/endpoints/minidump.rs | 41 ++++++----------- relay-server/src/endpoints/mod.rs | 6 +-- relay-server/src/extractors/mod.rs | 2 + relay-server/src/extractors/remote.rs | 56 +++++++++++++++++++++++ relay-server/src/utils/multipart.rs | 53 +++++---------------- 10 files changed, 101 insertions(+), 118 deletions(-) create mode 100644 relay-server/src/extractors/remote.rs diff --git a/Cargo.lock b/Cargo.lock index 10ea3d843f..8556d8e2f9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -269,7 +269,6 @@ dependencies = [ "matchit", "memchr", "mime", - "multer 3.1.0", "percent-encoding", "pin-project-lite", "rustversion", @@ -2345,24 +2344,6 @@ dependencies = [ "windows-sys 0.52.0", ] -[[package]] -name = "multer" -version = "2.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "01acbdc23469fd8fe07ab135923371d5f5a422fbf9c522158677c8eb15bc51c2" -dependencies = [ - "bytes", - "encoding_rs", - "futures-util", - "http 0.2.12", - "httparse", - "log", - "memchr", - "mime", - "spin", - "version_check", -] - [[package]] name = "multer" version = "3.1.0" @@ -3776,7 +3757,7 @@ dependencies = [ "json-forensics", "mime", "minidump", - "multer 2.1.0", + "multer", "once_cell", "pin-project-lite", "priority-queue", diff --git a/Cargo.toml b/Cargo.toml index 14970a2af9..a6175a907f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -109,7 +109,7 @@ memchr = "2.7.4" md5 = "0.7.0" mime = "0.3.16" minidump = "0.22.0" -multer = "2.0.4" +multer = "3.1.0" num-traits = "0.2.18" num_cpus = "1.13.0" once_cell = "1.13.1" diff --git a/relay-server/Cargo.toml b/relay-server/Cargo.toml index de5d50d311..fb0b7c56f9 100644 --- a/relay-server/Cargo.toml +++ b/relay-server/Cargo.toml @@ -32,12 +32,7 @@ workspace = true [dependencies] anyhow = { workspace = true } serde_path_to_error = { workspace = true } -axum = { workspace = true, features = [ - "macros", - "matched-path", - "multipart", - "tracing", -] } +axum = { workspace = true, features = ["macros", "matched-path", "tracing"] } axum-extra = { workspace = true } axum-server = { workspace = true } arc-swap = { workspace = true } diff --git a/relay-server/src/endpoints/attachments.rs b/relay-server/src/endpoints/attachments.rs index b112ab113f..e7d39bd881 100644 --- a/relay-server/src/endpoints/attachments.rs +++ b/relay-server/src/endpoints/attachments.rs @@ -1,49 +1,43 @@ -use axum::extract::{DefaultBodyLimit, Multipart, Path}; +use axum::extract::Path; use axum::http::StatusCode; use axum::response::IntoResponse; -use axum::routing::{post, MethodRouter}; -use relay_config::Config; +use multer::Multipart; use relay_event_schema::protocol::EventId; use serde::Deserialize; use crate::endpoints::common::{self, BadStoreRequest}; use crate::envelope::{AttachmentType, Envelope}; -use crate::extractors::RequestMeta; +use crate::extractors::{RequestMeta, Xt}; use crate::service::ServiceState; use crate::utils; #[derive(Debug, Deserialize)] -struct AttachmentPath { +pub struct AttachmentPath { event_id: EventId, } async fn extract_envelope( - config: &Config, meta: RequestMeta, path: AttachmentPath, - multipart: Multipart, + multipart: Multipart<'static>, ) -> Result, BadStoreRequest> { - let max_size = config.max_attachment_size(); - let items = utils::multipart_items(multipart, max_size, |_| AttachmentType::default()).await?; + let items = utils::multipart_items(multipart, |_| AttachmentType::default()).await?; let mut envelope = Envelope::from_request(Some(path.event_id), meta); for item in items { envelope.add_item(item); } + Ok(envelope) } -async fn handle( +pub async fn handle( state: ServiceState, meta: RequestMeta, Path(path): Path, - multipart: Multipart, + Xt(multipart): Xt>, ) -> Result { - let envelope = extract_envelope(state.config(), meta, path, multipart).await?; + let envelope = extract_envelope(meta, path, multipart).await?; common::handle_envelope(&state, envelope).await?; Ok(StatusCode::CREATED) } - -pub fn route(config: &Config) -> MethodRouter { - post(handle).route_layer(DefaultBodyLimit::max(config.max_attachments_size())) -} diff --git a/relay-server/src/endpoints/common.rs b/relay-server/src/endpoints/common.rs index bec9ebfd5c..d7ea170161 100644 --- a/relay-server/src/endpoints/common.rs +++ b/relay-server/src/endpoints/common.rs @@ -15,7 +15,7 @@ use crate::services::outcome::{DiscardReason, Outcome}; use crate::services::processor::{MetricData, ProcessMetricMeta, ProcessingGroup}; use crate::services::project_cache::{CheckEnvelope, ProcessMetrics, ValidateEnvelope}; use crate::statsd::{RelayCounters, RelayHistograms}; -use crate::utils::{self, ApiErrorResponse, FormDataIter, ManagedEnvelope, MultipartError}; +use crate::utils::{self, ApiErrorResponse, FormDataIter, ManagedEnvelope}; #[derive(Clone, Copy, Debug, thiserror::Error)] #[error("the service is overloaded")] @@ -61,9 +61,6 @@ pub enum BadStoreRequest { #[error("invalid multipart data")] InvalidMultipart(#[from] multer::Error), - #[error("invalid multipart data")] - InvalidMultipartAxum(#[from] MultipartError), - #[error("invalid minidump")] InvalidMinidump, diff --git a/relay-server/src/endpoints/minidump.rs b/relay-server/src/endpoints/minidump.rs index e1b78c68ee..247decc2b2 100644 --- a/relay-server/src/endpoints/minidump.rs +++ b/relay-server/src/endpoints/minidump.rs @@ -1,18 +1,16 @@ use std::convert::Infallible; -use axum::extract::{DefaultBodyLimit, Multipart, Request}; +use axum::extract::Request; use axum::response::IntoResponse; -use axum::routing::{post, MethodRouter}; use axum::RequestExt; use bytes::Bytes; -use futures::{future, FutureExt}; -use relay_config::Config; +use multer::Multipart; use relay_event_schema::protocol::EventId; use crate::constants::{ITEM_NAME_BREADCRUMBS1, ITEM_NAME_BREADCRUMBS2, ITEM_NAME_EVENT}; use crate::endpoints::common::{self, BadStoreRequest, TextResponse}; use crate::envelope::{AttachmentType, ContentType, Envelope, Item, ItemType}; -use crate::extractors::{RawContentType, RequestMeta}; +use crate::extractors::{RawContentType, RequestMeta, Xt}; use crate::service::ServiceState; use crate::utils; @@ -69,8 +67,8 @@ async fn extract_embedded_minidump(payload: Bytes) -> Result, BadS None => return Ok(None), }; - let stream = future::ok::<_, Infallible>(payload.clone()).into_stream(); - let mut multipart = multer::Multipart::new(stream, boundary); + let stream = futures::stream::once(async { Ok::<_, Infallible>(payload.clone()) }); + let mut multipart = Multipart::new(stream, boundary); while let Some(field) = multipart.next_field().await? { if field.name() == Some(MINIDUMP_FIELD_NAME) { @@ -82,12 +80,10 @@ async fn extract_embedded_minidump(payload: Bytes) -> Result, BadS } async fn extract_multipart( - config: &Config, - multipart: Multipart, + multipart: Multipart<'static>, meta: RequestMeta, ) -> Result, BadStoreRequest> { - let max_size = config.max_attachment_size(); - let mut items = utils::multipart_items(multipart, max_size, infer_attachment_type).await?; + let mut items = utils::multipart_items(multipart, infer_attachment_type).await?; let minidump_item = items .iter_mut() @@ -125,7 +121,7 @@ fn extract_raw_minidump(data: Bytes, meta: RequestMeta) -> Result, Ok(envelope) } -async fn handle( +pub async fn handle( state: ServiceState, meta: RequestMeta, content_type: RawContentType, @@ -139,7 +135,8 @@ async fn handle( let envelope = if MINIDUMP_RAW_CONTENT_TYPES.contains(&content_type.as_ref()) { extract_raw_minidump(request.extract().await?, meta)? } else { - extract_multipart(state.config(), request.extract().await?, meta).await? + let Xt(multipart) = request.extract_with_state(&state).await?; + extract_multipart(multipart, meta).await? }; let id = envelope.event_id(); @@ -155,17 +152,11 @@ async fn handle( Ok(TextResponse(id)) } -pub fn route(config: &Config) -> MethodRouter { - post(handle).route_layer(DefaultBodyLimit::max(config.max_attachments_size())) -} - #[cfg(test)] mod tests { use axum::body::Body; use axum::extract::FromRequest; - use relay_config::ByteSize; - use crate::utils::{multipart_items, FormDataIter}; use super::*; @@ -214,14 +205,12 @@ mod tests { .body(Body::from(multipart_body)) .unwrap(); - let multipart = Multipart::from_request(request, &()).await?; + let state: ServiceState = todo!("service state, 100MB max size"); + let Xt(multipart) = Xt::>::from_request(request, &state) + .await + .map_err(Xt::into_inner)?; - let items = multipart_items( - multipart, - ByteSize::mebibytes(100).as_bytes(), - infer_attachment_type, - ) - .await?; + let items = multipart_items(multipart, infer_attachment_type).await?; // we expect the multipart body to contain // * one arbitrary attachment from the user (a `config.json`) diff --git a/relay-server/src/endpoints/mod.rs b/relay-server/src/endpoints/mod.rs index c0e7a6b740..de33e601af 100644 --- a/relay-server/src/endpoints/mod.rs +++ b/relay-server/src/endpoints/mod.rs @@ -70,9 +70,9 @@ pub fn routes(config: &Config) -> Router{ .route("/api/:project_id/csp-report/", security_report::route(config)) .route("/api/:project_id/nel/", nel::route(config)) // No mandatory trailing slash here because people already use it like this. - .route("/api/:project_id/minidump", minidump::route(config)) - .route("/api/:project_id/minidump/", minidump::route(config)) - .route("/api/:project_id/events/:event_id/attachments/", attachments::route(config)) + .route("/api/:project_id/minidump", post(minidump::handle)) + .route("/api/:project_id/minidump/", post(minidump::handle)) + .route("/api/:project_id/events/:event_id/attachments/", post(attachments::handle)) .route("/api/:project_id/unreal/:sentry_key/", unreal::route(config)) // NOTE: If you add a new (non-experimental) route here, please also list it in // https://github.com/getsentry/sentry-docs/blob/master/docs/product/relay/operating-guidelines.mdx diff --git a/relay-server/src/extractors/mod.rs b/relay-server/src/extractors/mod.rs index 7d4b621451..4878817d1f 100644 --- a/relay-server/src/extractors/mod.rs +++ b/relay-server/src/extractors/mod.rs @@ -1,6 +1,7 @@ mod content_type; mod forwarded_for; mod mime; +mod remote; mod request_meta; mod signed_json; mod start_time; @@ -8,6 +9,7 @@ mod start_time; pub use self::content_type::*; pub use self::forwarded_for::*; pub use self::mime::*; +pub use self::remote::*; pub use self::request_meta::*; pub use self::signed_json::*; pub use self::start_time::*; diff --git a/relay-server/src/extractors/remote.rs b/relay-server/src/extractors/remote.rs new file mode 100644 index 0000000000..851d0c0c27 --- /dev/null +++ b/relay-server/src/extractors/remote.rs @@ -0,0 +1,56 @@ +//! Extractors for types from other crates via [`Xt`]. + +use axum::extract::{FromRequest, Request}; +use axum::http::header; +use axum::response::IntoResponse; +use multer::{Constraints, Multipart, SizeLimit}; + +use crate::service::ServiceState; +use crate::utils::ApiErrorResponse; + +/// A transparent wrapper around a type that implements [`FromRequest`] or [`IntoResponse`]. +#[derive(Debug)] +pub struct Xt(pub T); + +impl Xt { + /// Returns the inner value. + pub fn into_inner(self) -> T { + self.0 + } +} + +impl From for Xt { + fn from(inner: T) -> Self { + Self(inner) + } +} + +#[axum::async_trait] +impl FromRequest for Xt> { + type Rejection = Xt; + + async fn from_request(request: Request, state: &ServiceState) -> Result { + let content_type = request + .headers() + .get(header::CONTENT_TYPE) + .and_then(|v| v.to_str().ok()) + .unwrap_or(""); + let boundary = multer::parse_boundary(content_type)?; + + let limits = SizeLimit::new() + .whole_stream(state.config().max_attachments_size() as u64) + .per_field(state.config().max_attachment_size() as u64); + + Ok(Self(Multipart::with_constraints( + request.into_body().into_data_stream(), + boundary, + Constraints::new().size_limit(limits), + ))) + } +} + +impl IntoResponse for Xt { + fn into_response(self) -> axum::response::Response { + ApiErrorResponse::from_error(&self.0).into_response() + } +} diff --git a/relay-server/src/utils/multipart.rs b/relay-server/src/utils/multipart.rs index 8952d0b257..10aae5c53f 100644 --- a/relay-server/src/utils/multipart.rs +++ b/relay-server/src/utils/multipart.rs @@ -1,6 +1,6 @@ use std::io; -use axum::extract::multipart::{Field, Multipart}; +use multer::Multipart; use serde::{Deserialize, Serialize}; use crate::envelope::{AttachmentType, ContentType, Item, ItemType, Items}; @@ -147,58 +147,32 @@ pub fn get_multipart_boundary(data: &[u8]) -> Option<&str> { .and_then(|slice| std::str::from_utf8(&slice[2..]).ok()) } -#[derive(Debug, thiserror::Error)] -pub enum MultipartError { - #[error("field exceeded the size limit")] - FieldSizeExceeded, - #[error(transparent)] - Raw(#[from] axum::extract::multipart::MultipartError), -} - -async fn field_data<'a>(field: &mut Field<'a>, limit: usize) -> Result, MultipartError> { - let mut body = Vec::new(); - - while let Some(chunk) = field.chunk().await? { - if body.len() + chunk.len() > limit { - return Err(MultipartError::FieldSizeExceeded); - } - body.extend_from_slice(&chunk); - } - - Ok(body) -} - pub async fn multipart_items( - mut multipart: Multipart, - item_limit: usize, + mut multipart: Multipart<'_>, mut infer_type: F, -) -> Result +) -> Result where F: FnMut(Option<&str>) -> AttachmentType, { let mut items = Items::new(); let mut form_data = FormDataWriter::new(); - while let Some(mut field) = multipart.next_field().await? { + while let Some(field) = multipart.next_field().await? { if let Some(file_name) = field.file_name() { let mut item = Item::new(ItemType::Attachment); item.set_attachment_type(infer_type(field.name())); item.set_filename(file_name); // Extract the body after the immutable borrow on `file_name` is gone. if let Some(content_type) = field.content_type() { - item.set_payload( - content_type.into(), - field_data(&mut field, item_limit).await?, - ); + item.set_payload(content_type.as_ref().into(), field.bytes().await?); } else { - item.set_payload_without_content_type(field_data(&mut field, item_limit).await?); + item.set_payload_without_content_type(field.bytes().await?); } items.push(item); } else if let Some(field_name) = field.name().map(str::to_owned) { - let data = field_data(&mut field, item_limit).await?; - // Ensure to decode this safely to match Django's POST data behavior. This allows us to + // Ensure to decode this SAFELY to match Django's POST data behavior. This allows us to // process sentry event payloads even if they contain invalid encoding. - let string = String::from_utf8_lossy(&data); + let string = field.text().await?; form_data.append(&field_name, &string); } else { relay_log::trace!("multipart content without name or file_name"); @@ -219,9 +193,7 @@ where #[cfg(test)] mod tests { - use axum::body::Body; - use axum::extract::FromRequest; - use axum::http::Request; + use std::convert::Infallible; use super::*; @@ -283,12 +255,9 @@ mod tests { let data = "--X-BOUNDARY\r\nContent-Disposition: form-data; \ name=\"my_text_field\"\r\n\r\nabcd\r\n--X-BOUNDARY--"; // No trailing newline - let request = Request::builder() - .header("content-type", "multipart/form-data; boundary=X-BOUNDARY") - .body(Body::from(data)) - .unwrap(); + let stream = futures::stream::once(async { Ok::<_, Infallible>(data) }); + let mut multipart = Multipart::new(stream, "X-BOUNDARY"); - let mut multipart = Multipart::from_request(request, &()).await?; assert!(multipart.next_field().await?.is_some()); assert!(multipart.next_field().await?.is_none()); From d531555a669875cce3dfb14c054ed9c7443eb635 Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Tue, 3 Sep 2024 09:43:45 +0200 Subject: [PATCH 2/5] fix: Tests --- relay-server/src/endpoints/minidump.rs | 8 ++-- relay-server/src/extractors/remote.rs | 58 ++++++++++++++------------ relay-server/src/utils/multipart.rs | 24 +++++++++++ 3 files changed, 59 insertions(+), 31 deletions(-) diff --git a/relay-server/src/endpoints/minidump.rs b/relay-server/src/endpoints/minidump.rs index 247decc2b2..c627e6ac37 100644 --- a/relay-server/src/endpoints/minidump.rs +++ b/relay-server/src/endpoints/minidump.rs @@ -155,7 +155,7 @@ pub async fn handle( #[cfg(test)] mod tests { use axum::body::Body; - use axum::extract::FromRequest; + use relay_config::Config; use crate::utils::{multipart_items, FormDataIter}; @@ -205,11 +205,9 @@ mod tests { .body(Body::from(multipart_body)) .unwrap(); - let state: ServiceState = todo!("service state, 100MB max size"); - let Xt(multipart) = Xt::>::from_request(request, &state) - .await - .map_err(Xt::into_inner)?; + let config = Config::default(); + let multipart = utils::multipart_from_request(request, &config)?; let items = multipart_items(multipart, infer_attachment_type).await?; // we expect the multipart body to contain diff --git a/relay-server/src/extractors/remote.rs b/relay-server/src/extractors/remote.rs index 851d0c0c27..f1c8f1561c 100644 --- a/relay-server/src/extractors/remote.rs +++ b/relay-server/src/extractors/remote.rs @@ -1,24 +1,43 @@ //! Extractors for types from other crates via [`Xt`]. use axum::extract::{FromRequest, Request}; -use axum::http::header; use axum::response::IntoResponse; -use multer::{Constraints, Multipart, SizeLimit}; +use multer::Multipart; use crate::service::ServiceState; -use crate::utils::ApiErrorResponse; +use crate::utils::{self, ApiErrorResponse}; /// A transparent wrapper around a type that implements [`FromRequest`] or [`IntoResponse`]. +/// +/// # Example +/// +/// ```ignore +/// use std::convert::Infallible; +/// +/// use axum::extract::{FromRequest, Request}; +/// use axum::response::IntoResponse; +/// +/// use crate::extractors::Xt; +/// +/// // Derive `FromRequest` for `bool` for illustration purposes: +/// #[axum::async_trait] +/// impl axum::extract::FromRequest for Xt { +/// type Rejection = Xt; +/// +/// async fn from_request(request: Request) -> Result { +/// Ok(Xt(true)) +/// } +/// } +/// +/// impl IntoResponse for Xt { +/// fn into_response(self) -> axum::response::Response { +/// match self.0 {} +/// } +/// } +/// ``` #[derive(Debug)] pub struct Xt(pub T); -impl Xt { - /// Returns the inner value. - pub fn into_inner(self) -> T { - self.0 - } -} - impl From for Xt { fn from(inner: T) -> Self { Self(inner) @@ -30,22 +49,9 @@ impl FromRequest for Xt> { type Rejection = Xt; async fn from_request(request: Request, state: &ServiceState) -> Result { - let content_type = request - .headers() - .get(header::CONTENT_TYPE) - .and_then(|v| v.to_str().ok()) - .unwrap_or(""); - let boundary = multer::parse_boundary(content_type)?; - - let limits = SizeLimit::new() - .whole_stream(state.config().max_attachments_size() as u64) - .per_field(state.config().max_attachment_size() as u64); - - Ok(Self(Multipart::with_constraints( - request.into_body().into_data_stream(), - boundary, - Constraints::new().size_limit(limits), - ))) + utils::multipart_from_request(request, state.config()) + .map(Xt) + .map_err(Xt) } } diff --git a/relay-server/src/utils/multipart.rs b/relay-server/src/utils/multipart.rs index 10aae5c53f..990d41f309 100644 --- a/relay-server/src/utils/multipart.rs +++ b/relay-server/src/utils/multipart.rs @@ -1,6 +1,8 @@ use std::io; +use axum::extract::Request; use multer::Multipart; +use relay_config::Config; use serde::{Deserialize, Serialize}; use crate::envelope::{AttachmentType, ContentType, Item, ItemType, Items}; @@ -191,6 +193,28 @@ where Ok(items) } +pub fn multipart_from_request( + request: Request, + config: &Config, +) -> Result, multer::Error> { + let content_type = request + .headers() + .get("content-type") + .and_then(|v| v.to_str().ok()) + .unwrap_or(""); + let boundary = multer::parse_boundary(content_type)?; + + let limits = multer::SizeLimit::new() + .whole_stream(config.max_attachments_size() as u64) + .per_field(config.max_attachment_size() as u64); + + Ok(Multipart::with_constraints( + request.into_body().into_data_stream(), + boundary, + multer::Constraints::new().size_limit(limits), + )) +} + #[cfg(test)] mod tests { use std::convert::Infallible; From bb9accef45c9a1f7a870711bee7c786b7da13ddc Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Tue, 3 Sep 2024 11:16:58 +0200 Subject: [PATCH 3/5] fix: Minidump endpoint size limits and test --- relay-server/src/endpoints/minidump.rs | 12 ++++++++++-- relay-server/src/endpoints/mod.rs | 4 ++-- relay-server/src/extractors/remote.rs | 17 ++++++++++++++--- tests/integration/test_basic.py | 21 ++++++++++++--------- 4 files changed, 38 insertions(+), 16 deletions(-) diff --git a/relay-server/src/endpoints/minidump.rs b/relay-server/src/endpoints/minidump.rs index c627e6ac37..cab227a11e 100644 --- a/relay-server/src/endpoints/minidump.rs +++ b/relay-server/src/endpoints/minidump.rs @@ -1,10 +1,12 @@ use std::convert::Infallible; -use axum::extract::Request; +use axum::extract::{DefaultBodyLimit, Request}; use axum::response::IntoResponse; +use axum::routing::{post, MethodRouter}; use axum::RequestExt; use bytes::Bytes; use multer::Multipart; +use relay_config::Config; use relay_event_schema::protocol::EventId; use crate::constants::{ITEM_NAME_BREADCRUMBS1, ITEM_NAME_BREADCRUMBS2, ITEM_NAME_EVENT}; @@ -121,7 +123,7 @@ fn extract_raw_minidump(data: Bytes, meta: RequestMeta) -> Result, Ok(envelope) } -pub async fn handle( +async fn handle( state: ServiceState, meta: RequestMeta, content_type: RawContentType, @@ -152,6 +154,12 @@ pub async fn handle( Ok(TextResponse(id)) } +pub fn route(config: &Config) -> MethodRouter { + // Set the single-attachment limit that applies only for raw minidumps. Multipart bypasses the + // limited body and applies its own limits. + post(handle).route_layer(DefaultBodyLimit::max(config.max_attachment_size())) +} + #[cfg(test)] mod tests { use axum::body::Body; diff --git a/relay-server/src/endpoints/mod.rs b/relay-server/src/endpoints/mod.rs index de33e601af..d590c529a1 100644 --- a/relay-server/src/endpoints/mod.rs +++ b/relay-server/src/endpoints/mod.rs @@ -70,8 +70,8 @@ pub fn routes(config: &Config) -> Router{ .route("/api/:project_id/csp-report/", security_report::route(config)) .route("/api/:project_id/nel/", nel::route(config)) // No mandatory trailing slash here because people already use it like this. - .route("/api/:project_id/minidump", post(minidump::handle)) - .route("/api/:project_id/minidump/", post(minidump::handle)) + .route("/api/:project_id/minidump", minidump::route(config)) + .route("/api/:project_id/minidump/", minidump::route(config)) .route("/api/:project_id/events/:event_id/attachments/", post(attachments::handle)) .route("/api/:project_id/unreal/:sentry_key/", unreal::route(config)) // NOTE: If you add a new (non-experimental) route here, please also list it in diff --git a/relay-server/src/extractors/remote.rs b/relay-server/src/extractors/remote.rs index f1c8f1561c..6ffb80bff4 100644 --- a/relay-server/src/extractors/remote.rs +++ b/relay-server/src/extractors/remote.rs @@ -1,7 +1,8 @@ //! Extractors for types from other crates via [`Xt`]. use axum::extract::{FromRequest, Request}; -use axum::response::IntoResponse; +use axum::http::StatusCode; +use axum::response::{IntoResponse, Response}; use multer::Multipart; use crate::service::ServiceState; @@ -56,7 +57,17 @@ impl FromRequest for Xt> { } impl IntoResponse for Xt { - fn into_response(self) -> axum::response::Response { - ApiErrorResponse::from_error(&self.0).into_response() + fn into_response(self) -> Response { + let Self(ref error) = self; + + let status_code = match error { + multer::Error::FieldSizeExceeded { .. } => StatusCode::PAYLOAD_TOO_LARGE, + multer::Error::StreamSizeExceeded { .. } => StatusCode::PAYLOAD_TOO_LARGE, + multer::Error::NoMultipart => StatusCode::UNSUPPORTED_MEDIA_TYPE, + multer::Error::NoBoundary => StatusCode::UNSUPPORTED_MEDIA_TYPE, + _ => StatusCode::BAD_REQUEST, + }; + + (status_code, ApiErrorResponse::from_error(error)).into_response() } } diff --git a/tests/integration/test_basic.py b/tests/integration/test_basic.py index 3783bdaf26..6b6a943b20 100644 --- a/tests/integration/test_basic.py +++ b/tests/integration/test_basic.py @@ -153,16 +153,15 @@ def test_store_allowed_origins_passes(mini_sentry, relay, allowed_origins): @pytest.mark.parametrize( - "route,status_code", + "route", [ - ("/api/42/store/", 413), - ("/api/42/envelope/", 413), - ("/api/42/attachment/", 413), - # Minidumps attempt to read the first line (using dedicated limits) and parse it - ("/api/42/minidump/", 400), + "/api/42/store/", + "/api/42/envelope/", + "/api/42/attachment/", + "/api/42/minidump/", ], ) -def test_zipbomb_content_encoding(mini_sentry, relay, route, status_code): +def test_zipbomb_content_encoding(mini_sentry, relay, route): project_id = 42 mini_sentry.add_basic_project_config(project_id) relay = relay( @@ -189,11 +188,15 @@ def test_zipbomb_content_encoding(mini_sentry, relay, route, status_code): route, mini_sentry.get_dsn_public_key(project_id), ), - headers={"content-encoding": "gzip", "content-length": str(size)}, + headers={ + "content-encoding": "gzip", + "content-length": str(size), + "content-type": "application/octet-stream", + }, data=f, ) - assert response.status_code == status_code + assert response.status_code == 413 @pytest.mark.parametrize("content_encoding", ["gzip", "deflate", "identity", ""]) From f0fdc112aa2444386767f10668151cd4c5da5b54 Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Fri, 6 Sep 2024 08:49:10 +0200 Subject: [PATCH 4/5] ref: Return 400 --- relay-server/src/extractors/remote.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/relay-server/src/extractors/remote.rs b/relay-server/src/extractors/remote.rs index 6ffb80bff4..a9e0eff55b 100644 --- a/relay-server/src/extractors/remote.rs +++ b/relay-server/src/extractors/remote.rs @@ -63,8 +63,6 @@ impl IntoResponse for Xt { let status_code = match error { multer::Error::FieldSizeExceeded { .. } => StatusCode::PAYLOAD_TOO_LARGE, multer::Error::StreamSizeExceeded { .. } => StatusCode::PAYLOAD_TOO_LARGE, - multer::Error::NoMultipart => StatusCode::UNSUPPORTED_MEDIA_TYPE, - multer::Error::NoBoundary => StatusCode::UNSUPPORTED_MEDIA_TYPE, _ => StatusCode::BAD_REQUEST, }; From c71db88fac23839aab4a7bdc235534f4009b2f01 Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Fri, 6 Sep 2024 09:21:46 +0200 Subject: [PATCH 5/5] ref: Rename --- relay-server/src/endpoints/attachments.rs | 4 ++-- relay-server/src/endpoints/minidump.rs | 4 ++-- relay-server/src/extractors/remote.rs | 28 +++++++++++------------ 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/relay-server/src/endpoints/attachments.rs b/relay-server/src/endpoints/attachments.rs index e7d39bd881..7e1d33bd1d 100644 --- a/relay-server/src/endpoints/attachments.rs +++ b/relay-server/src/endpoints/attachments.rs @@ -7,7 +7,7 @@ use serde::Deserialize; use crate::endpoints::common::{self, BadStoreRequest}; use crate::envelope::{AttachmentType, Envelope}; -use crate::extractors::{RequestMeta, Xt}; +use crate::extractors::{Remote, RequestMeta}; use crate::service::ServiceState; use crate::utils; @@ -35,7 +35,7 @@ pub async fn handle( state: ServiceState, meta: RequestMeta, Path(path): Path, - Xt(multipart): Xt>, + Remote(multipart): Remote>, ) -> Result { let envelope = extract_envelope(meta, path, multipart).await?; common::handle_envelope(&state, envelope).await?; diff --git a/relay-server/src/endpoints/minidump.rs b/relay-server/src/endpoints/minidump.rs index cab227a11e..80d513d709 100644 --- a/relay-server/src/endpoints/minidump.rs +++ b/relay-server/src/endpoints/minidump.rs @@ -12,7 +12,7 @@ use relay_event_schema::protocol::EventId; use crate::constants::{ITEM_NAME_BREADCRUMBS1, ITEM_NAME_BREADCRUMBS2, ITEM_NAME_EVENT}; use crate::endpoints::common::{self, BadStoreRequest, TextResponse}; use crate::envelope::{AttachmentType, ContentType, Envelope, Item, ItemType}; -use crate::extractors::{RawContentType, RequestMeta, Xt}; +use crate::extractors::{RawContentType, Remote, RequestMeta}; use crate::service::ServiceState; use crate::utils; @@ -137,7 +137,7 @@ async fn handle( let envelope = if MINIDUMP_RAW_CONTENT_TYPES.contains(&content_type.as_ref()) { extract_raw_minidump(request.extract().await?, meta)? } else { - let Xt(multipart) = request.extract_with_state(&state).await?; + let Remote(multipart) = request.extract_with_state(&state).await?; extract_multipart(multipart, meta).await? }; diff --git a/relay-server/src/extractors/remote.rs b/relay-server/src/extractors/remote.rs index a9e0eff55b..f611476abc 100644 --- a/relay-server/src/extractors/remote.rs +++ b/relay-server/src/extractors/remote.rs @@ -1,4 +1,4 @@ -//! Extractors for types from other crates via [`Xt`]. +//! Extractors for types from other crates via [`Remote`]. use axum::extract::{FromRequest, Request}; use axum::http::StatusCode; @@ -8,7 +8,7 @@ use multer::Multipart; use crate::service::ServiceState; use crate::utils::{self, ApiErrorResponse}; -/// A transparent wrapper around a type that implements [`FromRequest`] or [`IntoResponse`]. +/// A transparent wrapper around a remote type that implements [`FromRequest`] or [`IntoResponse`]. /// /// # Example /// @@ -18,45 +18,45 @@ use crate::utils::{self, ApiErrorResponse}; /// use axum::extract::{FromRequest, Request}; /// use axum::response::IntoResponse; /// -/// use crate::extractors::Xt; +/// use crate::extractors::Remote; /// /// // Derive `FromRequest` for `bool` for illustration purposes: /// #[axum::async_trait] -/// impl axum::extract::FromRequest for Xt { -/// type Rejection = Xt; +/// impl axum::extract::FromRequest for Remote { +/// type Rejection = Remote; /// /// async fn from_request(request: Request) -> Result { -/// Ok(Xt(true)) +/// Ok(Remote(true)) /// } /// } /// -/// impl IntoResponse for Xt { +/// impl IntoResponse for Remote { /// fn into_response(self) -> axum::response::Response { /// match self.0 {} /// } /// } /// ``` #[derive(Debug)] -pub struct Xt(pub T); +pub struct Remote(pub T); -impl From for Xt { +impl From for Remote { fn from(inner: T) -> Self { Self(inner) } } #[axum::async_trait] -impl FromRequest for Xt> { - type Rejection = Xt; +impl FromRequest for Remote> { + type Rejection = Remote; async fn from_request(request: Request, state: &ServiceState) -> Result { utils::multipart_from_request(request, state.config()) - .map(Xt) - .map_err(Xt) + .map(Remote) + .map_err(Remote) } } -impl IntoResponse for Xt { +impl IntoResponse for Remote { fn into_response(self) -> Response { let Self(ref error) = self;