diff --git a/CHANGELOG.md b/CHANGELOG.md index 20d71d288ea..c353de48715 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ - Set sentry.trace.status on segment spans. ([#6140](https://github.com/getsentry/relay/pull/6140)) - Don't modify segment information for V2 web vital spans. ([#6160](https://github.com/getsentry/relay/pull/6160)) - Support compressed minidumps when the `relay-minidump-uploads` feature is enabled. ([#6151](https://github.com/getsentry/relay/pull/6151)) +- Limit indexes of event form-data chunks. ([#6173](https://github.com/getsentry/relay/pull/6173)) **Internal**: diff --git a/relay-server/src/processing/errors/errors/utils/formdata.rs b/relay-server/src/processing/errors/errors/utils/formdata.rs index 0c52670f24a..cac9d834ae7 100644 --- a/relay-server/src/processing/errors/errors/utils/formdata.rs +++ b/relay-server/src/processing/errors/errors/utils/formdata.rs @@ -1,9 +1,10 @@ +use relay_config::Config; use serde_json::Value as SerdeValue; use crate::envelope::Item; use crate::utils::{self, ChunkedFormDataAggregator, FormDataIter}; -pub fn merge_formdata(target: &mut SerdeValue, item: &Item) { +pub fn merge_formdata(target: &mut SerdeValue, item: &Item, config: &Config) { let payload = item.payload(); let mut aggregator = ChunkedFormDataAggregator::new(); @@ -15,7 +16,7 @@ pub fn merge_formdata(target: &mut SerdeValue, item: &Item) { Ok(event) => utils::merge_values(target, event), Err(_) => relay_log::debug!("invalid json event payload in sentry form field"), } - } else if let Some(index) = utils::get_sentry_chunk_index(entry.key(), "sentry__") { + } else if let Some(index) = utils::get_sentry_chunk_index(entry.key(), "sentry__", config) { // Electron SDK splits up long payloads into chunks starting at sentry__1 with an // incrementing counter. Assemble these chunks here and then decode them below. aggregator.insert(index, entry.value()); @@ -39,3 +40,43 @@ pub fn merge_formdata(target: &mut SerdeValue, item: &Item) { } } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::envelope::{ContentType, ItemType}; + use crate::utils::FormDataWriter; + + fn form_data_item(fields: &[(&str, &str)]) -> Item { + let mut writer = FormDataWriter::new(); + for (key, value) in fields { + writer.append(key, value); + } + let mut item = Item::new(ItemType::FormData); + item.set_payload(ContentType::Text, writer.into_inner()); + item + } + + #[test] + fn test_merge_formdata_assembles_chunks() { + let item = form_data_item(&[("sentry__0", r#"{"message":"#), ("sentry__1", r#""hi"}"#)]); + let mut target = SerdeValue::Object(Default::default()); + + merge_formdata(&mut target, &item, &Config::default()); + + assert_eq!(target, serde_json::json!({ "message": "hi" })); + } + + #[test] + fn test_merge_formdata_ignores_too_large_index() { + let item = form_data_item(&[("sentry__2000", "x")]); + let mut target = SerdeValue::Object(Default::default()); + + merge_formdata(&mut target, &item, &Config::default()); + + assert_eq!( + target, + serde_json::json!({ "extra": { "sentry__2000": "x" } }) + ); + } +} diff --git a/relay-server/src/processing/errors/errors/utils/mod.rs b/relay-server/src/processing/errors/errors/utils/mod.rs index 976aaeaf479..c89da3c1763 100644 --- a/relay-server/src/processing/errors/errors/utils/mod.rs +++ b/relay-server/src/processing/errors/errors/utils/mod.rs @@ -158,6 +158,7 @@ pub fn take_event_from_attachments( pub fn take_event_from_formdata( items: &mut Vec, metrics: &mut Metrics, + ctx: Context<'_>, ) -> Result> { let Some(form_data) = take_item_of_type(items, ItemType::FormData) else { return Ok(Annotated::empty()); @@ -165,7 +166,7 @@ pub fn take_event_from_formdata( let event = { let mut value = serde_json::Value::Object(Default::default()); - formdata::merge_formdata(&mut value, &form_data); + formdata::merge_formdata(&mut value, &form_data, ctx.processing.config); Annotated::deserialize_with_meta(value).map_err(ProcessingError::InvalidJson) }?; metrics.bytes_ingested_event = Annotated::new(form_data.len() as u64); @@ -199,7 +200,7 @@ pub fn take_event_from_crash_items( return Ok(event); } - let event = take_event_from_formdata(items, metrics)?; + let event = take_event_from_formdata(items, metrics, ctx)?; if event.0.is_some() { return Ok(event); } diff --git a/relay-server/src/utils/multipart.rs b/relay-server/src/utils/multipart.rs index ae5c0268a70..ba2dc942c4f 100644 --- a/relay-server/src/utils/multipart.rs +++ b/relay-server/src/utils/multipart.rs @@ -94,7 +94,7 @@ impl<'a> FormDataEntry<'a> { /// /// This writer is used to serialize multiple plain fields from a multipart form data request into a /// single envelope item. Use `FormDataIter` to iterate all entries. -struct FormDataWriter { +pub struct FormDataWriter { data: Vec, } diff --git a/relay-server/src/utils/param_parser.rs b/relay-server/src/utils/param_parser.rs index 1f5ff4cf83a..3e586f1d23a 100644 --- a/relay-server/src/utils/param_parser.rs +++ b/relay-server/src/utils/param_parser.rs @@ -1,3 +1,4 @@ +use relay_config::Config; use serde_json::Value; enum IndexingState { @@ -98,8 +99,12 @@ pub fn get_sentry_entry_indexes(param_name: &str) -> Option> { /// /// Electron SDK splits up long payloads into chunks starting at sentry__1 with an /// incrementing counter. Assemble these chunks here and then decode them below. -pub fn get_sentry_chunk_index(key: &str, prefix: &str) -> Option { - key.strip_prefix(prefix).and_then(|rest| rest.parse().ok()) +/// +/// If the index is too large, or unparsable from `key`, `None` is returned. +pub fn get_sentry_chunk_index(key: &str, prefix: &str, config: &Config) -> Option { + key.strip_prefix(prefix) + .and_then(|rest| rest.parse().ok()) + .filter(|&index| index <= config.max_event_size() / 1024) } /// Aggregates slices of strings in random order. @@ -239,13 +244,42 @@ mod tests { #[test] fn test_chunk_index() { - assert_eq!(get_sentry_chunk_index("sentry__0", "sentry__"), Some(0)); - assert_eq!(get_sentry_chunk_index("sentry__1", "sentry__"), Some(1)); + let config = Config::default(); + assert_eq!( + get_sentry_chunk_index("sentry__0", "sentry__", &config), + Some(0) + ); + assert_eq!( + get_sentry_chunk_index("sentry__1", "sentry__", &config), + Some(1) + ); + + assert_eq!(get_sentry_chunk_index("foo__0", "sentry__", &config), None); + assert_eq!( + get_sentry_chunk_index("sentry__", "sentry__", &config), + None + ); + assert_eq!( + get_sentry_chunk_index("sentry__-1", "sentry__", &config), + None + ); + assert_eq!( + get_sentry_chunk_index("sentry__xx", "sentry__", &config), + None + ); + } - assert_eq!(get_sentry_chunk_index("foo__0", "sentry__"), None); - assert_eq!(get_sentry_chunk_index("sentry__", "sentry__"), None); - assert_eq!(get_sentry_chunk_index("sentry__-1", "sentry__"), None); - assert_eq!(get_sentry_chunk_index("sentry__xx", "sentry__"), None); + #[test] + fn test_chunk_index_derived_from_max_event_size() { + let config = Config::default(); + assert_eq!( + get_sentry_chunk_index("sentry__1024", "sentry__", &config), + Some(1024) + ); + assert_eq!( + get_sentry_chunk_index("sentry__1025", "sentry__", &config), + None + ); } #[test]