fix: Preserve event id of minidump upload

server/src/constants.rs

@@ -2,3 +2,22 @@ include!(concat!(env!("OUT_DIR"), "/constants.gen.rs"));
 
 /// Shutdown timeout before killing all tasks and dropping queued events.
 pub const SHUTDOWN_TIMEOUT: u16 = 10;
+
+/// Name of the event attachment.
+///
+/// This is a special attachment that can contain a sentry event payload encoded as message pack.
+pub const ITEM_NAME_EVENT: &str = "__sentry-event";
+
+/// Name of the breadcrumb attachment (1).
+///
+/// This is a special attachment that can contain breadcrumbs encoded as message pack. There can be
+/// two attachments that the SDK may use as swappable buffers. Both attachments will be merged and
+/// truncated to the maxmimum number of allowed attachments.
+pub const ITEM_NAME_BREADCRUMBS1: &str = "__sentry-breadcrumbs1";
+
+/// Name of the breadcrumb attachment (2).
+///
+/// This is a special attachment that can contain breadcrumbs encoded as message pack. There can be
+/// two attachments that the SDK may use as swappable buffers. Both attachments will be merged and
+/// truncated to the maxmimum number of allowed attachments.
+pub const ITEM_NAME_BREADCRUMBS2: &str = "__sentry-breadcrumbs2";

server/src/endpoints/attachments.rs

@@ -4,11 +4,11 @@ use futures::Future;
 use semaphore_common::tryf;
 use semaphore_general::protocol::EventId;
 
-use crate::endpoints::common::{handle_store_like_request, BadStoreRequest};
+use crate::endpoints::common::{self, BadStoreRequest};
 use crate::envelope::Envelope;
 use crate::extractors::{EventMeta, StartTime};
 use crate::service::{ServiceApp, ServiceState};
-use crate::utils::MultipartEnvelope;
+use crate::utils::MultipartItems;
 
 fn extract_envelope(
     request: &HttpRequest<ServiceState>,
@@ -22,11 +22,18 @@ fn extract_envelope(
         .parse::<EventId>()
         .map_err(|_| BadStoreRequest::InvalidEventId));
 
-    let envelope = Envelope::from_request(event_id, meta);
-
-    let future = MultipartEnvelope::new(envelope, max_payload_size)
+    let future = MultipartItems::new(max_payload_size)
         .handle_request(request)
-        .map_err(BadStoreRequest::InvalidMultipart);
+        .map_err(BadStoreRequest::InvalidMultipart)
+        .map(move |items| {
+            let mut envelope = Envelope::from_request(event_id, meta);
+
+            for item in items {
+                envelope.add_item(item);
+            }
+
+            envelope
+        });
 
     Box::new(future)
 }
@@ -42,14 +49,14 @@ fn store_attachment(
 ) -> ResponseFuture<HttpResponse, BadStoreRequest> {
     let attachment_size = request.state().config().max_attachment_payload_size();
 
-    Box::new(handle_store_like_request(
+    common::handle_store_like_request(
         meta,
         false,
         start_time,
         request,
         move |data, meta| extract_envelope(data, meta, attachment_size),
         create_response,
-    ))
+    )
 }
 
 pub fn configure_app(app: ServiceApp) -> ServiceApp {

server/src/endpoints/common.rs

@@ -11,6 +11,7 @@ use futures::prelude::*;
 use parking_lot::Mutex;
 use sentry::Hub;
 use sentry_actix::ActixWebHubExt;
+use serde::Deserialize;
 
 use semaphore_common::{clone, metric, tryf, LogError};
 use semaphore_general::protocol::EventId;
@@ -19,10 +20,11 @@ use crate::actors::events::{QueueEvent, QueueEventError};
 use crate::actors::outcome::{DiscardReason, Outcome, TrackOutcome};
 use crate::actors::project::{EventAction, GetEventAction, GetProject, ProjectError, RateLimit};
 use crate::body::StorePayloadError;
-use crate::envelope::{Envelope, EnvelopeError};
+use crate::constants::ITEM_NAME_EVENT;
+use crate::envelope::{Envelope, EnvelopeError, ItemType, Items};
 use crate::extractors::{EventMeta, StartTime};
 use crate::service::ServiceState;
-use crate::utils::{ApiErrorResponse, MultipartError};
+use crate::utils::{ApiErrorResponse, FormDataIter, MultipartError};
 
 #[derive(Fail, Debug)]
 pub enum BadStoreRequest {
@@ -41,6 +43,9 @@ pub enum BadStoreRequest {
     #[fail(display = "invalid JSON data")]
     InvalidJson(#[cause] serde_json::Error),
 
+    #[fail(display = "invalid messagepack data")]
+    InvalidMsgpack(#[cause] rmp_serde::decode::Error),
+
     #[fail(display = "invalid event envelope")]
     InvalidEnvelope(#[cause] EnvelopeError),
 
@@ -79,6 +84,7 @@ impl BadStoreRequest {
 
             BadStoreRequest::EmptyBody => Outcome::Invalid(DiscardReason::NoData),
             BadStoreRequest::InvalidJson(_) => Outcome::Invalid(DiscardReason::InvalidJson),
+            BadStoreRequest::InvalidMsgpack(_) => Outcome::Invalid(DiscardReason::InvalidMsgpack),
             BadStoreRequest::InvalidMultipart(_) => {
                 Outcome::Invalid(DiscardReason::InvalidMultipart)
             }
@@ -156,6 +162,90 @@ impl ResponseError for BadStoreRequest {
     }
 }
 
+#[derive(Deserialize)]
+struct EventIdHelper {
+    #[serde(default, rename = "event_id")]
+    id: Option<EventId>,
+}
+
+/// Extracts the event id from a JSON payload.
+///
+/// If the payload contains no event id, `Ok(None)` is returned. This function also validates that
+/// the provided is valid and returns an `Err` on parse errors. If the event id itself is malformed,
+/// an `Err` is returned.
+pub fn event_id_from_json(data: &[u8]) -> Result<Option<EventId>, BadStoreRequest> {
+    serde_json::from_slice(data)
+        .map(|helper: EventIdHelper| helper.id)
+        .map_err(BadStoreRequest::InvalidJson)
+}
+
+/// Extracts the event id from a MessagePack payload.
+///
+/// If the payload contains no event id, `Ok(None)` is returned. This function also validates that
+/// the provided is valid and returns an `Err` on parse errors. If the event id itself is malformed,
+/// an `Err` is returned.
+pub fn event_id_from_msgpack(data: &[u8]) -> Result<Option<EventId>, BadStoreRequest> {
+    rmp_serde::from_slice(data)
+        .map(|helper: EventIdHelper| helper.id)
+        .map_err(BadStoreRequest::InvalidMsgpack)
+}
+
+/// Extracts the event id from `sentry` JSON payload or the `sentry[event_id]` formdata key.
+///
+/// If the event id itself is malformed, an `Err` is returned. If there is a `sentry` key containing
+/// malformed JSON, an error is returned.
+pub fn event_id_from_formdata(data: &[u8]) -> Result<Option<EventId>, BadStoreRequest> {
+    for entry in FormDataIter::new(data) {
+        if entry.key() == "sentry" {
+            return event_id_from_json(entry.value().as_bytes());
+        } else if entry.key() == "sentry[event_id]" {
+            return entry
+                .value()
+                .parse()
+                .map(Some)
+                .map_err(|_| BadStoreRequest::InvalidEventId);
+        }
+    }
+
+    Ok(None)
+}
+
+/// Extracts the event id from multiple items.
+///
+/// Submitting multiple event payloads is undefined behavior. This function will check for an event
+/// id in the following precedence:
+///
+///  1. The `Event` item.
+///  2. The `__sentry-event` event attachment.
+///  3. The `sentry` JSON payload.
+///  4. The `sentry[event_id]` formdata key.
+pub fn event_id_from_items(items: &Items) -> Result<Option<EventId>, BadStoreRequest> {
+    if let Some(item) = items.iter().find(|item| item.ty() == ItemType::Event) {
+        if let Some(event_id) = event_id_from_json(&item.payload())? {
+            return Ok(Some(event_id));
+        }
+    }
+
+    if let Some(item) = items
+        .iter()
+        .find(|item| item.name() == Some(ITEM_NAME_EVENT))
+    {
+        if let Some(event_id) = event_id_from_msgpack(&item.payload())? {
+            return Ok(Some(event_id));
+        }
+    }
+
+    if let Some(item) = items.iter().find(|item| item.ty() == ItemType::FormData) {
+        // Swallow all other errors here since it is quite common to receive invalid secondary
+        // payloads. `EventProcessor` also retains events in such cases.
+        if let Ok(Some(event_id)) = event_id_from_formdata(&item.payload()) {
+            return Ok(Some(event_id));
+        }
+    }
+
+    Ok(None)
+}
+
 /// Handles Sentry events.
 ///
 /// Sentry events may come either directly from a http request ( the store endpoint

server/src/endpoints/minidump.rs

@@ -6,36 +6,18 @@ use futures::{future, Future, Stream};
 use semaphore_general::protocol::EventId;
 
 use crate::body::ForwardBody;
-use crate::endpoints::common::{handle_store_like_request, BadStoreRequest};
+use crate::constants::{ITEM_NAME_BREADCRUMBS1, ITEM_NAME_BREADCRUMBS2, ITEM_NAME_EVENT};
+use crate::endpoints::common::{self, BadStoreRequest};
 use crate::envelope::{AttachmentType, ContentType, Envelope, Item, ItemType};
 use crate::extractors::{EventMeta, StartTime};
 use crate::service::{ServiceApp, ServiceState};
-use crate::utils::{consume_field, get_multipart_boundary, MultipartEnvelope, MultipartError};
+use crate::utils::{consume_field, get_multipart_boundary, MultipartError, MultipartItems};
 
 /// The field name of a minidump in the multipart form-data upload.
 ///
 /// Sentry requires
 const MINIDUMP_FIELD_NAME: &str = "upload_file_minidump";
 
-/// Name of the event attachment.
-///
-/// This is a special attachment that can contain a sentry event payload encoded as message pack.
-const ITEM_NAME_EVENT: &str = "__sentry-event";
-
-/// Name of the breadcrumb attachment (1).
-///
-/// This is a special attachment that can contain breadcrumbs encoded as message pack. There can be
-/// two attachments that the SDK may use as swappable buffers. Both attachments will be merged and
-/// truncated to the maxmimum number of allowed attachments.
-const ITEM_NAME_BREADCRUMBS1: &str = "__sentry-breadcrumbs1";
-
-/// Name of the breadcrumb attachment (2).
-///
-/// This is a special attachment that can contain breadcrumbs encoded as message pack. There can be
-/// two attachments that the SDK may use as swappable buffers. Both attachments will be merged and
-/// truncated to the maxmimum number of allowed attachments.
-const ITEM_NAME_BREADCRUMBS2: &str = "__sentry-breadcrumbs2";
-
 /// File name for a standalone minidump upload.
 ///
 /// In contrast to the field name, this is used when a standalone minidump is uploaded not in a
@@ -102,14 +84,7 @@ fn extract_envelope(
     request: &HttpRequest<ServiceState>,
     meta: EventMeta,
     max_payload_size: usize,
-) -> ResponseFuture<Envelope, BadStoreRequest>
-where {
-    // TODO: at the moment we override any pre exsiting (set by an SDK) event id here.
-    //  We shouldn't do that. The problem is that at this stage we cannot afford to parse the
-    //  request and look for the event id so we simply set one.
-    //  We need to somehow preserve SDK set event ids ( the current behaviour needs to change).
-    let mut envelope = Envelope::from_request(EventId::new(), meta);
-
+) -> ResponseFuture<Envelope, BadStoreRequest> {
     // Minidump request payloads do not have the same structure as usual events from other SDKs. The
     // minidump can either be transmitted as request body, or as `upload_file_minidump` in a
     // multipart formdata request.
@@ -124,38 +99,43 @@ where {
                 item.set_filename(MINIDUMP_FILE_NAME);
                 item.set_attachment_type(AttachmentType::Minidump);
 
+                // Create an envelope with a random event id.
+                let mut envelope = Envelope::from_request(EventId::new(), meta);
                 envelope.add_item(item);
                 Ok(envelope)
             });
 
         return Box::new(future);
     }
 
-    let future = MultipartEnvelope::new(envelope, max_payload_size)
+    let future = MultipartItems::new(max_payload_size)
         .handle_request(request)
         .map_err(BadStoreRequest::InvalidMultipart)
-        .and_then(move |mut envelope| {
-            let mut minidump_item =
-                match envelope.take_item_by(|item| item.name() == Some(MINIDUMP_FIELD_NAME)) {
-                    Some(item) => item,
-                    None => {
-                        return Box::new(future::err(BadStoreRequest::MissingMinidump))
-                            as ResponseFuture<Envelope, BadStoreRequest>
-                    }
-                };
+        .and_then(move |mut items| {
+            let minidump_index = items
+                .iter()
+                .position(|item| item.name() == Some(MINIDUMP_FIELD_NAME));
+
+            let mut minidump_item = match minidump_index {
+                Some(index) => items.swap_remove(index),
+                None => {
+                    return Box::new(future::err(BadStoreRequest::MissingMinidump))
+                        as ResponseFuture<_, _>
+                }
+            };
 
             // HACK !!
+            //
             // This field is not a minidump (.i.e. it doesn't start with the minidump magic header).
-            // It could be a multipart field containing a minidump; this happens in some
-            // Linux Electron SDKs.
-            // Unfortunately the embedded multipart field is not recognized by the multipart
-            // parser as a multipart field containing a multipart body.
-            // My (RaduW) guess is that it is not recognized because the Content-Disposition
-            // header at the beginning of the field does *NOT* contain a boundary field.
-            // For this case we will look if field the field starts with a '--' and manually
-            // extract the boundary (which is what follows '--' up to the end of line)
-            // and manually construct a multipart with the detected boundary.
-            // If we can extract a multipart with an embedded minidump than use that field.
+            // It could be a multipart field containing a minidump; this happens in old versions of
+            // the Linux Electron SDK.
+            //
+            // Unfortunately, the embedded multipart field is not recognized by the multipart parser
+            // as a multipart field containing a multipart body. For this case we will look if field
+            // the field starts with a '--' and manually extract the boundary (which is what follows
+            // '--' up to the end of line) and manually construct a multipart with the detected
+            // boundary. If we can extract a multipart with an embedded minidump, then use that
+            // field.
             let future = get_embedded_minidump(minidump_item.payload(), max_payload_size).and_then(
                 move |embedded_opt| {
                     if let Some(embedded) = embedded_opt {
@@ -169,26 +149,31 @@ where {
 
                     minidump_item.set_attachment_type(AttachmentType::Minidump);
                     validate_minidump(&minidump_item.payload())?;
-                    envelope.add_item(minidump_item);
+                    items.push(minidump_item);
 
-                    Ok(envelope)
+                    Ok(items)
                 },
             );
 
             Box::new(future)
         })
-        .and_then(move |mut envelope| {
-            for field_name in &[ITEM_NAME_BREADCRUMBS1, ITEM_NAME_BREADCRUMBS2] {
-                if let Some(item) = envelope.get_item_by_mut(|item| item.name() == Some(field_name))
-                {
-                    item.set_attachment_type(AttachmentType::Breadcrumbs);
+        .and_then(move |items| {
+            let event_id = common::event_id_from_items(&items)?.unwrap_or_else(EventId::new);
+            let mut envelope = Envelope::from_request(event_id, meta);
+
+            for mut item in items {
+                let attachment_type = match item.name() {
+                    Some(self::ITEM_NAME_BREADCRUMBS1) => Some(AttachmentType::Breadcrumbs),
+                    Some(self::ITEM_NAME_BREADCRUMBS2) => Some(AttachmentType::Breadcrumbs),
+                    Some(self::ITEM_NAME_EVENT) => Some(AttachmentType::MsgpackEvent),
+                    _ => None,
+                };
+
+                if let Some(ty) = attachment_type {
+                    item.set_attachment_type(ty);
                 }
-            }
 
-            if let Some(item) =
-                envelope.get_item_by_mut(|item| item.name() == Some(ITEM_NAME_EVENT))
-            {
-                item.set_attachment_type(AttachmentType::MsgpackEvent);
+                envelope.add_item(item);
             }
 
             Ok(envelope)
@@ -212,14 +197,14 @@ fn store_minidump(
 ) -> ResponseFuture<HttpResponse, BadStoreRequest> {
     let event_size = request.state().config().max_attachment_payload_size();
 
-    Box::new(handle_store_like_request(
+    common::handle_store_like_request(
         meta,
         true,
         start_time,
         request,
         move |data, meta| extract_envelope(data, meta, event_size),
         create_response,
-    ))
+    )
 }
 
 pub fn configure_app(app: ServiceApp) -> ServiceApp {