fix: Put Message-ID into hidden headers and take it from there on receiver (#4798)

Put a copy of Message-ID into hidden headers and prefer it over the one in the IMF header section
that servers mess up with.

This also reverts "Set X-Microsoft-Original-Message-ID on outgoing emails for amazonaws (#3077)".

Author: iequidoo

src/chat.rs

@@ -5904,7 +5904,7 @@ mod tests {
         // Alice has an SMTP-server replacing the `Message-ID:`-header (as done eg. by outlook.com).
         let sent_msg = alice.pop_sent_msg().await;
         let msg = sent_msg.payload();
-        assert_eq!(msg.match_indices("Message-ID: <Gr.").count(), 1);
+        assert_eq!(msg.match_indices("Message-ID: <Gr.").count(), 2);
         assert_eq!(msg.match_indices("References: <Gr.").count(), 1);
         let msg = msg.replace("Message-ID: <Gr.", "Message-ID: <XXX");
         assert_eq!(msg.match_indices("Message-ID: <Gr.").count(), 0);

src/mimefactory.rs

@@ -124,7 +124,8 @@ struct MessageHeaders {
     /// Headers that MUST NOT go into IMF header section.
     ///
     /// These are large headers which may hit the header section size limit on the server, such as
-    /// Chat-User-Avatar with a base64-encoded image inside.
+    /// Chat-User-Avatar with a base64-encoded image inside. Also there are headers duplicated here
+    /// that servers mess up with in the IMF header section, like Message-ID.
     pub hidden: Vec<Header>,
 }
 
@@ -560,24 +561,9 @@ impl<'a> MimeFactory<'a> {
             Loaded::Mdn { .. } => create_outgoing_rfc724_mid(None, &self.from_addr),
         };
         let rfc724_mid_headervalue = render_rfc724_mid(&rfc724_mid);
-
-        // Amazon's SMTP servers change the `Message-ID`, just as Outlook's SMTP servers do.
-        // Outlook's servers add an `X-Microsoft-Original-Message-ID` header with the original `Message-ID`,
-        // and when downloading messages we look for this header in order to correctly identify
-        // messages.
-        // Amazon's servers do not add such a header, so we just add it ourselves.
-        if let Some(server) = context.get_config(Config::ConfiguredSendServer).await? {
-            if server.ends_with(".amazonaws.com") {
-                headers.unprotected.push(Header::new(
-                    "X-Microsoft-Original-Message-ID".into(),
-                    rfc724_mid_headervalue.clone(),
-                ))
-            }
-        }
-
-        headers
-            .unprotected
-            .push(Header::new("Message-ID".into(), rfc724_mid_headervalue));
+        let rfc724_mid_header = Header::new("Message-ID".into(), rfc724_mid_headervalue);
+        headers.unprotected.push(rfc724_mid_header.clone());
+        headers.hidden.push(rfc724_mid_header);
 
         // Reply headers as in <https://datatracker.ietf.org/doc/html/rfc5322#appendix-A.2>.
         if !self.in_reply_to.is_empty() {
@@ -783,29 +769,22 @@ impl<'a> MimeFactory<'a> {
                 )
                 .header(("Subject".to_string(), "...".to_string()))
         } else {
-            let message = if headers.hidden.is_empty() {
-                message
-            } else {
-                // Store hidden headers in the inner unencrypted message.
-                let message = headers
-                    .hidden
-                    .into_iter()
-                    .fold(message, |message, header| message.header(header));
-
-                PartBuilder::new()
-                    .message_type(MimeMultipartType::Mixed)
-                    .child(message.build())
-            };
+            // Store hidden headers in the inner unencrypted message.
+            let message = headers
+                .hidden
+                .into_iter()
+                .fold(message, |message, header| message.header(header));
+            let message = PartBuilder::new()
+                .message_type(MimeMultipartType::Mixed)
+                .child(message.build());
 
             // Store protected headers in the outer message.
             let message = headers
                 .protected
                 .iter()
                 .fold(message, |message, header| message.header(header.clone()));
 
-            if self.should_skip_autocrypt()
-                || !context.get_config_bool(Config::SignUnencrypted).await?
-            {
+            if skip_autocrypt || !context.get_config_bool(Config::SignUnencrypted).await? {
                 let protected: HashSet<Header> = HashSet::from_iter(headers.protected.into_iter());
                 for h in headers.unprotected.split_off(0) {
                     if !protected.contains(&h) {
@@ -2165,33 +2144,37 @@ mod tests {
         let body = payload.next().unwrap();
 
         assert_eq!(outer.match_indices("multipart/mixed").count(), 1);
+        assert_eq!(outer.match_indices("Message-ID:").count(), 1);
         assert_eq!(outer.match_indices("Subject:").count(), 1);
         assert_eq!(outer.match_indices("Autocrypt:").count(), 1);
         assert_eq!(outer.match_indices("Chat-User-Avatar:").count(), 0);
 
         assert_eq!(inner.match_indices("text/plain").count(), 1);
+        assert_eq!(inner.match_indices("Message-ID:").count(), 1);
         assert_eq!(inner.match_indices("Chat-User-Avatar:").count(), 1);
         assert_eq!(inner.match_indices("Subject:").count(), 0);
 
         assert_eq!(body.match_indices("this is the text!").count(), 1);
 
         // if another message is sent, that one must not contain the avatar
-        // and no artificial multipart/mixed nesting
         let sent_msg = t.send_msg(chat.id, &mut msg).await;
-        let mut payload = sent_msg.payload().splitn(2, "\r\n\r\n");
+        let mut payload = sent_msg.payload().splitn(3, "\r\n\r\n");
         let outer = payload.next().unwrap();
+        let inner = payload.next().unwrap();
         let body = payload.next().unwrap();
 
-        assert_eq!(outer.match_indices("text/plain").count(), 1);
+        assert_eq!(outer.match_indices("multipart/mixed").count(), 1);
+        assert_eq!(outer.match_indices("Message-ID:").count(), 1);
         assert_eq!(outer.match_indices("Subject:").count(), 1);
         assert_eq!(outer.match_indices("Autocrypt:").count(), 1);
-        assert_eq!(outer.match_indices("multipart/mixed").count(), 0);
         assert_eq!(outer.match_indices("Chat-User-Avatar:").count(), 0);
 
+        assert_eq!(inner.match_indices("text/plain").count(), 1);
+        assert_eq!(inner.match_indices("Message-ID:").count(), 1);
+        assert_eq!(inner.match_indices("Chat-User-Avatar:").count(), 0);
+        assert_eq!(inner.match_indices("Subject:").count(), 0);
+
         assert_eq!(body.match_indices("this is the text!").count(), 1);
-        assert_eq!(body.match_indices("text/plain").count(), 0);
-        assert_eq!(body.match_indices("Chat-User-Avatar:").count(), 0);
-        assert_eq!(body.match_indices("Subject:").count(), 0);
 
         Ok(())
     }
@@ -2223,6 +2206,7 @@ mod tests {
         let part = payload.next().unwrap();
         assert_eq!(part.match_indices("multipart/signed").count(), 1);
         assert_eq!(part.match_indices("From:").count(), 1);
+        assert_eq!(part.match_indices("Message-ID:").count(), 1);
         assert_eq!(part.match_indices("Subject:").count(), 0);
         assert_eq!(part.match_indices("Autocrypt:").count(), 1);
         assert_eq!(part.match_indices("Chat-User-Avatar:").count(), 0);
@@ -2234,13 +2218,15 @@ mod tests {
             1
         );
         assert_eq!(part.match_indices("From:").count(), 1);
+        assert_eq!(part.match_indices("Message-ID:").count(), 0);
         assert_eq!(part.match_indices("Subject:").count(), 1);
         assert_eq!(part.match_indices("Autocrypt:").count(), 0);
         assert_eq!(part.match_indices("Chat-User-Avatar:").count(), 0);
 
         let part = payload.next().unwrap();
         assert_eq!(part.match_indices("text/plain").count(), 1);
         assert_eq!(part.match_indices("From:").count(), 0);
+        assert_eq!(part.match_indices("Message-ID:").count(), 1);
         assert_eq!(part.match_indices("Chat-User-Avatar:").count(), 1);
         assert_eq!(part.match_indices("Subject:").count(), 0);
 
@@ -2261,31 +2247,38 @@ mod tests {
             .is_some());
 
         // if another message is sent, that one must not contain the avatar
-        // and no artificial multipart/mixed nesting
         let sent_msg = t.send_msg(chat.id, &mut msg).await;
-        let mut payload = sent_msg.payload().splitn(3, "\r\n\r\n");
+        let mut payload = sent_msg.payload().splitn(4, "\r\n\r\n");
 
         let part = payload.next().unwrap();
         assert_eq!(part.match_indices("multipart/signed").count(), 1);
         assert_eq!(part.match_indices("From:").count(), 1);
+        assert_eq!(part.match_indices("Message-ID:").count(), 1);
         assert_eq!(part.match_indices("Subject:").count(), 0);
         assert_eq!(part.match_indices("Autocrypt:").count(), 1);
         assert_eq!(part.match_indices("Chat-User-Avatar:").count(), 0);
 
         let part = payload.next().unwrap();
-        assert_eq!(part.match_indices("text/plain").count(), 1);
+        assert_eq!(
+            part.match_indices("multipart/mixed; protected-headers=\"v1\"")
+                .count(),
+            1
+        );
         assert_eq!(part.match_indices("From:").count(), 1);
+        assert_eq!(part.match_indices("Message-ID:").count(), 0);
         assert_eq!(part.match_indices("Subject:").count(), 1);
         assert_eq!(part.match_indices("Autocrypt:").count(), 0);
-        assert_eq!(part.match_indices("multipart/mixed").count(), 0);
         assert_eq!(part.match_indices("Chat-User-Avatar:").count(), 0);
 
+        let part = payload.next().unwrap();
+        assert_eq!(part.match_indices("text/plain").count(), 1);
+        assert_eq!(body.match_indices("From:").count(), 0);
+        assert_eq!(part.match_indices("Message-ID:").count(), 1);
+        assert_eq!(part.match_indices("Chat-User-Avatar:").count(), 0);
+        assert_eq!(part.match_indices("Subject:").count(), 0);
+
         let body = payload.next().unwrap();
         assert_eq!(body.match_indices("this is the text!").count(), 1);
-        assert_eq!(body.match_indices("text/plain").count(), 0);
-        assert_eq!(body.match_indices("From:").count(), 0);
-        assert_eq!(body.match_indices("Chat-User-Avatar:").count(), 0);
-        assert_eq!(body.match_indices("Subject:").count(), 0);
 
         bob.recv_msg(&sent_msg).await;
         let alice_contact = Contact::get_by_id(&bob.ctx, alice_id).await.unwrap();

src/receive_imf.rs

@@ -184,28 +184,49 @@ pub(crate) async fn receive_imf_inner(
         }
     }
 
-    info!(context, "Receiving message {rfc724_mid:?}, seen={seen}...");
+    let rfc724_mid_orig = &mime_parser
+        .get_rfc724_mid()
+        .unwrap_or(rfc724_mid.to_string());
+    info!(
+        context,
+        "Receiving message {rfc724_mid_orig:?}, seen={seen}...",
+    );
 
     // check, if the mail is already in our database.
     // make sure, this check is done eg. before securejoin-processing.
-    let (replace_msg_id, replace_chat_id) =
-        if let Some(old_msg_id) = message::rfc724_mid_exists(context, rfc724_mid).await? {
-            let msg = Message::load_from_db(context, old_msg_id).await?;
-            if msg.download_state() != DownloadState::Done && is_partial_download.is_none() {
-                // the message was partially downloaded before and is fully downloaded now.
-                info!(
-                    context,
-                    "Message already partly in DB, replacing by full message."
-                );
-                (Some(old_msg_id), Some(msg.chat_id))
-            } else {
-                // the message was probably moved around.
-                info!(context, "Message already in DB, doing nothing.");
-                return Ok(None);
-            }
+    let (replace_msg_id, replace_chat_id);
+    if let Some(old_msg_id) = message::rfc724_mid_exists(context, rfc724_mid).await? {
+        if is_partial_download.is_some() {
+            info!(
+                context,
+                "Got a partial download and message is already in DB."
+            );
+            return Ok(None);
+        }
+        let msg = Message::load_from_db(context, old_msg_id).await?;
+        replace_msg_id = Some(old_msg_id);
+        replace_chat_id = if msg.download_state() != DownloadState::Done {
+            // the message was partially downloaded before and is fully downloaded now.
+            info!(
+                context,
+                "Message already partly in DB, replacing by full message."
+            );
+            Some(msg.chat_id)
+        } else {
+            None
+        };
+    } else {
+        replace_msg_id = if rfc724_mid_orig != rfc724_mid {
+            message::rfc724_mid_exists(context, rfc724_mid_orig).await?
         } else {
-            (None, None)
+            None
         };
+        replace_chat_id = None;
+    }
+    if replace_msg_id.is_some() && replace_chat_id.is_none() {
+        info!(context, "Message is already downloaded.");
+        return Ok(None);
+    };
 
     let prevent_rename =
         mime_parser.is_mailinglist_message() || mime_parser.get_header(HeaderDef::Sender).is_some();
@@ -301,7 +322,7 @@ pub(crate) async fn receive_imf_inner(
             imf_raw,
             incoming,
             &to_ids,
-            rfc724_mid,
+            rfc724_mid_orig,
             from_id,
             seen || replace_msg_id.is_some(),
             is_partial_download,
@@ -421,20 +442,30 @@ pub(crate) async fn receive_imf_inner(
     let delete_server_after = context.get_config_delete_server_after().await?;
 
     if !received_msg.msg_ids.is_empty() {
-        if received_msg.needs_delete_job
+        let target = if received_msg.needs_delete_job
             || (delete_server_after == Some(0) && is_partial_download.is_none())
         {
-            let target = context.get_delete_msgs_target().await?;
+            Some(context.get_delete_msgs_target().await?)
+        } else {
+            None
+        };
+        if target.is_some() || rfc724_mid_orig != rfc724_mid {
+            let target_subst = match &target {
+                Some(target) => format!("target='{target}',"),
+                None => "".to_string(),
+            };
             context
                 .sql
                 .execute(
-                    "UPDATE imap SET target=? WHERE rfc724_mid=?",
-                    (target, rfc724_mid),
+                    &format!("UPDATE imap SET {target_subst} rfc724_mid=?1 WHERE rfc724_mid=?2"),
+                    (rfc724_mid_orig, rfc724_mid),
                 )
                 .await?;
-        } else if !mime_parser.mdn_reports.is_empty() && mime_parser.has_chat_version() {
+        }
+        if target.is_none() && !mime_parser.mdn_reports.is_empty() && mime_parser.has_chat_version()
+        {
             // This is a Delta Chat MDN. Mark as read.
-            markseen_on_imap_table(context, rfc724_mid).await?;
+            markseen_on_imap_table(context, rfc724_mid_orig).await?;
         }
     }
 
@@ -528,6 +559,10 @@ async fn add_parts(
     prevent_rename: bool,
     verified_encryption: VerifiedEncryption,
 ) -> Result<ReceivedMsg> {
+    let rfc724_mid_orig = &mime_parser
+        .get_rfc724_mid()
+        .unwrap_or(rfc724_mid.to_string());
+
     let mut chat_id = None;
     let mut chat_id_blocked = Blocked::Not;
 
@@ -1308,7 +1343,7 @@ RETURNING id
 "#)?;
                 let row_id: MsgId = stmt.query_row(params![
                     replace_msg_id,
-                    rfc724_mid,
+                    rfc724_mid_orig,
                     if trash { DC_CHAT_ID_TRASH } else { chat_id },
                     if trash { ContactId::UNDEFINED } else { from_id },
                     if trash { ContactId::UNDEFINED } else { to_id },

src/receive_imf/tests.rs

@@ -3223,6 +3223,22 @@ async fn test_thunderbird_unsigned_with_unencrypted_subject() -> Result<()> {
     Ok(())
 }
 
+/// Tests that DC takes the correct Message-ID from the encrypted message part, not the unencrypted
+/// one messed up by the server.
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_messed_up_message_id() -> Result<()> {
+    let t = TestContext::new_bob().await;
+
+    let raw = include_bytes!("../../test-data/message/messed_up_message_id.eml");
+    receive_imf(&t, raw, false).await?;
+    assert_eq!(
+        t.get_last_msg().await.rfc724_mid,
+        "0bb9ffe1-2596-d997-95b4-1fef8cc4808e@example.org"
+    );
+
+    Ok(())
+}
+
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn test_mua_user_adds_member() -> Result<()> {
     let t = TestContext::new_alice().await;