diff --git a/CHANGELOG.md b/CHANGELOG.md index ae716dd5de9..6395fbad3ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ - Emit outcomes in both `log_byte` and `log_item` categories when logs are dropped. ([#5766](https://github.com/getsentry/relay/pull/5766)) - Propagate an event's retention policy to its attachments ([#5774](https://github.com/getsentry/relay/pull/5774)) - Prevent panic in span description normalization. ([#5781](https://github.com/getsentry/relay/pull/5781)) +- Limit overall stream size in playstation multiparts. ([#5795](https://github.com/getsentry/relay/pull/5795)) **Features**: diff --git a/relay-server/src/endpoints/attachments.rs b/relay-server/src/endpoints/attachments.rs index 1e923f21b16..f1e3d56b3c9 100644 --- a/relay-server/src/endpoints/attachments.rs +++ b/relay-server/src/endpoints/attachments.rs @@ -1,7 +1,7 @@ -use axum::extract::Path; +use axum::extract::{Path, Request}; use axum::http::StatusCode; use axum::response::IntoResponse; -use multer::Field; +use multer::{Field, Multipart}; use relay_config::Config; use relay_event_schema::protocol::EventId; use serde::Deserialize; @@ -10,7 +10,7 @@ use crate::endpoints::common::{self, BadStoreRequest}; use crate::envelope::{AttachmentType, Envelope, Item}; use crate::extractors::RequestMeta; use crate::service::ServiceState; -use crate::utils::{AttachmentStrategy, ConstrainedMultipart, read_attachment_bytes_into_item}; +use crate::utils::{self, AttachmentStrategy, read_attachment_bytes_into_item}; #[derive(Debug, Deserialize)] pub struct AttachmentPath { @@ -37,12 +37,10 @@ impl AttachmentStrategy for AttachmentsAttachmentStrategy { async fn extract_envelope( meta: RequestMeta, path: AttachmentPath, - multipart: ConstrainedMultipart, + multipart: Multipart<'static>, config: &Config, ) -> Result, BadStoreRequest> { - let items = multipart - .items(config, AttachmentsAttachmentStrategy) - .await?; + let items = utils::multipart_items(multipart, config, AttachmentsAttachmentStrategy).await?; let mut envelope = Envelope::from_request(Some(path.event_id), meta); for item in items { @@ -56,8 +54,9 @@ pub async fn handle( state: ServiceState, meta: RequestMeta, Path(path): Path, - multipart: ConstrainedMultipart, + request: Request, ) -> axum::response::Result { + let multipart = utils::multipart_from_request(request, state.config().max_attachments_size())?; let envelope = extract_envelope(meta, path, multipart, state.config()).await?; common::handle_envelope(&state, envelope) .await? diff --git a/relay-server/src/endpoints/minidump.rs b/relay-server/src/endpoints/minidump.rs index 17d5f6f2cb2..ff6e71be08a 100644 --- a/relay-server/src/endpoints/minidump.rs +++ b/relay-server/src/endpoints/minidump.rs @@ -23,9 +23,7 @@ use crate::extractors::{RawContentType, RequestMeta}; use crate::middlewares; use crate::service::ServiceState; use crate::services::outcome::{DiscardAttachmentType, DiscardItemType}; -use crate::utils::{ - self, AttachmentStrategy, ConstrainedMultipart, read_attachment_bytes_into_item, -}; +use crate::utils::{self, AttachmentStrategy, read_attachment_bytes_into_item}; /// The field name of a minidump in the multipart form-data upload. /// @@ -192,11 +190,11 @@ impl AttachmentStrategy for MinidumpAttachmentStrategy { } async fn extract_multipart( - multipart: ConstrainedMultipart, + multipart: Multipart<'static>, meta: RequestMeta, config: &Config, ) -> Result, BadStoreRequest> { - let mut items = multipart.items(config, MinidumpAttachmentStrategy).await?; + let mut items = utils::multipart_items(multipart, config, MinidumpAttachmentStrategy).await?; let minidump_item = items .iter_mut() @@ -262,7 +260,8 @@ async fn handle( let envelope = if MINIDUMP_RAW_CONTENT_TYPES.contains(&content_type.as_ref()) { extract_raw_minidump(request.extract().await?, meta, config.max_attachment_size())? } else { - let multipart = request.extract_with_state(&state).await?; + let multipart = + utils::multipart_from_request(request, state.config().max_attachments_size())?; extract_multipart(multipart, meta, config).await? }; @@ -453,11 +452,8 @@ mod tests { let config = Config::default(); - let multipart = ConstrainedMultipart( - utils::multipart_from_request(request, multer::Constraints::new()).unwrap(), - ); - let items = multipart - .items(&config, MinidumpAttachmentStrategy) + let multipart = utils::multipart_from_request(request, multipart_body.len()).unwrap(); + let items = utils::multipart_items(multipart, &config, MinidumpAttachmentStrategy) .await .unwrap(); diff --git a/relay-server/src/endpoints/mod.rs b/relay-server/src/endpoints/mod.rs index 0abe45cb3da..f7fc435162c 100644 --- a/relay-server/src/endpoints/mod.rs +++ b/relay-server/src/endpoints/mod.rs @@ -3,11 +3,12 @@ //! This module contains implementations for all supported relay endpoints, as well as a generic //! `forward` endpoint that sends unknown requests to the upstream. +pub(crate) mod common; + mod attachments; mod autoscaling; mod batch_metrics; mod batch_outcomes; -mod common; mod envelope; mod forward; mod health_check; diff --git a/relay-server/src/endpoints/playstation.rs b/relay-server/src/endpoints/playstation.rs index 1149b4a24e1..2faca457586 100644 --- a/relay-server/src/endpoints/playstation.rs +++ b/relay-server/src/endpoints/playstation.rs @@ -30,7 +30,7 @@ use crate::services::outcome::DiscardReason; use crate::services::projects::cache::Project; use crate::services::upload::{Create, Stream, Upload}; use crate::utils::{self, MeteredStream}; -use crate::utils::{AttachmentStrategy, BadMultipart, BoundedStream}; +use crate::utils::{AttachmentStrategy, BoundedStream}; /// The extension of a prosperodump in the multipart form-data upload. const PROSPERODUMP_EXTENSION: &str = ".prosperodmp"; @@ -216,14 +216,15 @@ async fn handle( return Ok(axum::Json(create_data_request_response()).into_response()); } + let config = state.config(); let project = state .project_cache_handle() - .ready(meta.public_key(), state.config().query_timeout()) + .ready(meta.public_key(), config.query_timeout()) .await .ok_or(StatusCode::SERVICE_UNAVAILABLE)?; - let multipart = utils::multipart_from_request(request, multer::Constraints::new()) - .map_err(BadMultipart::Multipart)?; + let stream_size_limit = config.max_upload_size() + config.max_attachments_size(); + let multipart = utils::multipart_from_request(request, stream_size_limit)?; let mut envelope = extract_multipart(multipart, meta, &state, &project).await?; envelope.require_feature(Feature::PlaystationIngestion); diff --git a/relay-server/src/extractors/mod.rs b/relay-server/src/extractors/mod.rs index ee69db65024..15692de0abd 100644 --- a/relay-server/src/extractors/mod.rs +++ b/relay-server/src/extractors/mod.rs @@ -3,7 +3,6 @@ mod forwarded_for; mod integration_builder; mod mime; mod received_at; -mod remote; mod request_meta; mod signature; mod signed_json; @@ -13,6 +12,5 @@ pub use self::forwarded_for::*; pub use self::integration_builder::*; pub use self::mime::*; pub use self::received_at::*; -pub use self::remote::*; pub use self::request_meta::*; pub use self::signed_json::*; diff --git a/relay-server/src/extractors/remote.rs b/relay-server/src/extractors/remote.rs deleted file mode 100644 index 08cda75f4a5..00000000000 --- a/relay-server/src/extractors/remote.rs +++ /dev/null @@ -1,72 +0,0 @@ -//! Extractors for types from other crates via [`Remote`]. - -use axum::extract::{FromRequest, Request}; -use axum::http::StatusCode; -use axum::response::{IntoResponse, Response}; -use multer::Multipart; - -use crate::service::ServiceState; -use crate::utils::{self, ApiErrorResponse}; - -/// A transparent wrapper around a remote type that implements [`FromRequest`] or [`IntoResponse`]. -/// -/// # Example -/// -/// ```ignore -/// use std::convert::Infallible; -/// -/// use axum::extract::{FromRequest, Request}; -/// use axum::response::IntoResponse; -/// -/// use crate::extractors::Remote; -/// -/// // Derive `FromRequest` for `bool` for illustration purposes: -/// impl axum::extract::FromRequest for Remote { -/// type Rejection = Remote; -/// -/// async fn from_request(request: Request) -> Result { -/// Ok(Remote(true)) -/// } -/// } -/// -/// impl IntoResponse for Remote { -/// fn into_response(self) -> axum::response::Response { -/// match self.0 {} -/// } -/// } -/// ``` -#[derive(Debug)] -pub struct Remote(pub T); - -impl From for Remote { - fn from(inner: T) -> Self { - Self(inner) - } -} - -impl FromRequest for Remote> { - type Rejection = Remote; - - async fn from_request( - request: Request, - _state: &ServiceState, - ) -> Result { - utils::multipart_from_request(request, multer::Constraints::new()) - .map(Remote) - .map_err(Remote) - } -} - -impl IntoResponse for Remote { - 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, - _ => StatusCode::BAD_REQUEST, - }; - - (status_code, ApiErrorResponse::from_error(error)).into_response() - } -} diff --git a/relay-server/src/utils/multipart.rs b/relay-server/src/utils/multipart.rs index 825f68158a3..c4d04909445 100644 --- a/relay-server/src/utils/multipart.rs +++ b/relay-server/src/utils/multipart.rs @@ -1,21 +1,16 @@ -use std::convert::Infallible; use std::future::Future; use std::io; use std::task::Poll; -use axum::extract::{FromRequest, Request}; -use axum::http::StatusCode; -use axum::response::{IntoResponse, Response}; +use axum::extract::Request; use bytes::{Bytes, BytesMut}; use futures::{StreamExt, TryStreamExt}; -use multer::{Field, Multipart}; +use multer::{Constraints, Field, Multipart, SizeLimit}; use relay_config::Config; use serde::{Deserialize, Serialize}; +use crate::endpoints::common::BadStoreRequest; use crate::envelope::{AttachmentType, ContentType, Item, ItemType, Items}; -use crate::extractors::{BadEventMeta, Remote}; -use crate::service::ServiceState; -use crate::utils::ApiErrorResponse; /// Type used for encoding string lengths. type Len = u32; @@ -159,34 +154,6 @@ pub fn get_multipart_boundary(data: &[u8]) -> Option<&str> { .and_then(|slice| std::str::from_utf8(&slice[2..]).ok()) } -#[cfg_attr(not(sentry), allow(dead_code))] -#[derive(Debug, thiserror::Error)] -pub enum BadMultipart { - #[error("event metadata error: {0}")] - EventMeta(#[from] BadEventMeta), - #[error("multipart error: {0}")] - Multipart(#[from] multer::Error), -} - -impl From for BadMultipart { - fn from(infallible: Infallible) -> Self { - match infallible {} - } -} - -impl IntoResponse for BadMultipart { - fn into_response(self) -> Response { - let status_code = match self { - BadMultipart::Multipart( - multer::Error::FieldSizeExceeded { .. } | multer::Error::StreamSizeExceeded { .. }, - ) => StatusCode::PAYLOAD_TOO_LARGE, - _ => StatusCode::BAD_REQUEST, - }; - - (status_code, ApiErrorResponse::from_error(&self)).into_response() - } -} - /// Strategy for how to infer attachment type and add a multipart attachment to an envelope item. /// /// This enables different endpoints to have different ways of dealing with multipart attachments, @@ -357,53 +324,25 @@ impl futures::Stream for LimitedField<'_> { } } -/// Wrapper around [`multer::Multipart`] that prevents the total stream size from exceeding -/// [`Config::max_attachments_size`]. -/// -/// Although [`LimitedField`] prevents reading large fields into memory, it still reads -/// the entire field before returning an error. This led to the incident described in PR -/// [#4836](https://github.com/getsentry/relay/pull/4836) which necessitated introducing -/// [`ConstrainedMultipart`]'s stream size limit. -pub struct ConstrainedMultipart(pub Multipart<'static>); - -impl FromRequest for ConstrainedMultipart { - type Rejection = Remote; - - async fn from_request(request: Request, state: &ServiceState) -> Result { - let limits = - multer::SizeLimit::new().whole_stream(state.config().max_attachments_size() as u64); - - multipart_from_request(request, multer::Constraints::new().size_limit(limits)) - .map(Self) - .map_err(Remote) - } -} - -impl ConstrainedMultipart { - pub async fn items( - self, - config: &Config, - attachment_strategy: impl AttachmentStrategy, - ) -> Result { - multipart_items(self.0, config, attachment_strategy).await - } -} - pub fn multipart_from_request( request: Request, - constraints: multer::Constraints, -) -> Result, multer::Error> { + stream_size_limit: usize, +) -> Result, BadStoreRequest> { 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 boundary = + multer::parse_boundary(content_type).map_err(BadStoreRequest::InvalidMultipart)?; + // Limits the overall stream size, preventing overly long processing times which can cause + // incidents like the one described in [#4836](https://github.com/getsentry/relay/pull/4836). + let stream_size_limit = SizeLimit::new().whole_stream(stream_size_limit as u64); Ok(Multipart::with_constraints( request.into_body().into_data_stream(), boundary, - constraints, + Constraints::new().size_limit(stream_size_limit), )) } diff --git a/tests/integration/test_playstation.py b/tests/integration/test_playstation.py index 93d0c642d37..4f3fa456c3e 100644 --- a/tests/integration/test_playstation.py +++ b/tests/integration/test_playstation.py @@ -25,7 +25,9 @@ def playstation_project_config(): "eventRetention": 36500, "features": [ "organizations:relay-playstation-ingestion", + "organizations:relay-new-error-processing", "projects:relay-upload-endpoint", + "projects:relay-playstation-uploads", ], } } @@ -326,6 +328,35 @@ def test_playstation_max_attachment_size_exceeded( assert len(outcomes_consumer.get_outcomes()) == 0 +def test_playstation_max_stream_size_exceeded( + mini_sentry, relay_processing_with_playstation, outcomes_consumer +): + PROJECT_ID = 42 + playstation_dump = load_dump_file("playstation.prosperodmp") + stream_size_limit = len(playstation_dump) - 131 + relay = relay_processing_with_playstation( + { + "limits": { + "max_upload_size": int(stream_size_limit / 2), + "max_attachments_size": int(stream_size_limit / 2), + } + } + ) + mini_sentry.add_full_project_config(PROJECT_ID, extra=playstation_project_config()) + outcomes_consumer = outcomes_consumer() + + with pytest.raises(requests.exceptions.HTTPError) as exc_info: + _ = relay.send_playstation_request(PROJECT_ID, playstation_dump) + + response = exc_info.value.response + assert response.status_code == 400, "Expected a 400 status code" + assert ( + response.content.decode("utf-8") + == f'{{"detail":"invalid multipart data","causes":["stream size exceeded limit: {stream_size_limit} bytes"]}}' + ) + assert len(outcomes_consumer.get_outcomes()) == 0 + + @pytest.mark.parametrize("num_intermediate_relays", [0, 1, 2]) def test_playstation_with_feature_flag( mini_sentry, @@ -400,9 +431,7 @@ def test_playstation_large_attachments( ): PROJECT_ID = 42 playstation_dump = load_dump_file("playstation.prosperodmp") - config = playstation_project_config() - config["config"]["features"].append("projects:relay-playstation-uploads") - mini_sentry.add_full_project_config(PROJECT_ID, extra=config) + mini_sentry.add_full_project_config(PROJECT_ID, extra=playstation_project_config()) outcomes_consumer = outcomes_consumer() attachments_consumer = attachments_consumer() credentials = relay_credentials()