fix: Ignore protected headers in outer message part (#6357)

Delta Chat always adds protected headers to the inner encrypted or signed message, so if a protected
header is only present in the outer part, it should be ignored because it's probably added by the
server or somebody else. The exceptions are Subject and List-ID because there are known cases when
they are only present in the outer message part.

Also treat any Chat-* headers as protected. This fixes e.g. a case when the server injects a
"Chat-Version" IMF header tricking Delta Chat into thinking that it's a chat message.

Also handle "Auto-Submitted" and "Autocrypt-Setup-Message" as protected headers on the receiver
side, this was apparently forgotten.

Author: iequidoo

src/decrypt.rs

@@ -178,7 +178,8 @@ mod tests {
         let bob = TestContext::new_bob().await;
         receive_imf(&bob, attachment_mime, false).await?;
         let msg = bob.get_last_msg().await;
-        assert_eq!(msg.text, "Hello from Thunderbird!");
+        // Subject should be prepended because the attachment doesn't have "Chat-Version".
+        assert_eq!(msg.text, "Hello, Bob! – Hello from Thunderbird!");
 
         Ok(())
     }

src/mimeparser.rs

@@ -246,6 +246,7 @@ impl MimeMessage {
         MimeMessage::merge_headers(
             context,
             &mut headers,
+            &mut headers_removed,
             &mut recipients,
             &mut past_members,
             &mut from,
@@ -273,6 +274,7 @@ impl MimeMessage {
                     MimeMessage::merge_headers(
                         context,
                         &mut headers,
+                        &mut headers_removed,
                         &mut recipients,
                         &mut past_members,
                         &mut from,
@@ -446,26 +448,11 @@ impl MimeMessage {
         });
         if let (Ok(mail), true) = (mail, is_encrypted) {
             if !signatures.is_empty() {
-                // Remove unsigned opportunistically protected headers from messages considered
-                // Autocrypt-encrypted / displayed with padlock.
-                // For "Subject" see <https://github.com/deltachat/deltachat-core-rust/issues/1790>.
-                for h in [
-                    HeaderDef::Subject,
-                    HeaderDef::ChatGroupId,
-                    HeaderDef::ChatGroupName,
-                    HeaderDef::ChatGroupNameChanged,
-                    HeaderDef::ChatGroupNameTimestamp,
-                    HeaderDef::ChatGroupAvatar,
-                    HeaderDef::ChatGroupMemberRemoved,
-                    HeaderDef::ChatGroupMemberAdded,
-                    HeaderDef::ChatGroupMemberTimestamps,
-                    HeaderDef::ChatGroupPastMembers,
-                    HeaderDef::ChatDelete,
-                    HeaderDef::ChatEdit,
-                    HeaderDef::ChatUserAvatar,
-                ] {
-                    remove_header(&mut headers, h.get_headername(), &mut headers_removed);
-                }
+                // Unsigned "Subject" mustn't be prepended to messages shown as encrypted
+                // (<https://github.com/deltachat/deltachat-core-rust/issues/1790>).
+                // Other headers are removed by `MimeMessage::merge_headers()` except for "List-ID".
+                remove_header(&mut headers, "subject", &mut headers_removed);
+                remove_header(&mut headers, "list-id", &mut headers_removed);
             }
 
             // let known protected headers from the decrypted
@@ -478,6 +465,7 @@ impl MimeMessage {
             MimeMessage::merge_headers(
                 context,
                 &mut headers,
+                &mut headers_removed,
                 &mut recipients,
                 &mut past_members,
                 &mut inner_from,
@@ -1558,19 +1546,28 @@ impl MimeMessage {
     fn merge_headers(
         context: &Context,
         headers: &mut HashMap<String, String>,
+        headers_removed: &mut HashSet<String>,
         recipients: &mut Vec<SingleInfo>,
         past_members: &mut Vec<SingleInfo>,
         from: &mut Option<SingleInfo>,
         list_post: &mut Option<String>,
         chat_disposition_notification_to: &mut Option<SingleInfo>,
         fields: &[mailparse::MailHeader<'_>],
     ) {
+        // There are known cases when Subject and List-ID only appear in the IMF section of
+        // signed-only messages. Such messages are shown as unencrypted anyway.
+        headers.retain(|k, _| {
+            !is_protected(k) || k == "subject" || k == "list-id" || {
+                headers_removed.insert(k.to_string());
+                false
+            }
+        });
         for field in fields {
             // lowercasing all headers is technically not correct, but makes things work better
             let key = field.get_key().to_lowercase();
-            if !headers.contains_key(&key) || // key already exists, only overwrite known types (protected headers)
-                    is_known(&key) || key.starts_with("chat-")
-            {
+            // Don't overwrite unprotected headers, but overwrite protected ones because DKIM
+            // signature applies to the last headers.
+            if !headers.contains_key(&key) || is_protected(&key) {
                 if key == HeaderDef::ChatDispositionNotificationTo.get_headername() {
                     match addrparse_header(field) {
                         Ok(addrlist) => {
@@ -2011,24 +2008,27 @@ pub(crate) fn parse_message_id(ids: &str) -> Result<String> {
 
 /// Returns true if the header overwrites outer header
 /// when it comes from protected headers.
-fn is_known(key: &str) -> bool {
-    matches!(
-        key,
-        "return-path"
-            | "date"
-            | "from"
-            | "sender"
-            | "reply-to"
-            | "to"
-            | "cc"
-            | "bcc"
-            | "message-id"
-            | "in-reply-to"
-            | "references"
-            | "subject"
-            | "secure-join"
-            | "list-id"
-    )
+fn is_protected(key: &str) -> bool {
+    key.starts_with("chat-")
+        || matches!(
+            key,
+            "return-path"
+                | "auto-submitted"
+                | "autocrypt-setup-message"
+                | "date"
+                | "from"
+                | "sender"
+                | "reply-to"
+                | "to"
+                | "cc"
+                | "bcc"
+                | "message-id"
+                | "in-reply-to"
+                | "references"
+                | "subject"
+                | "secure-join"
+                | "list-id"
+        )
 }
 
 /// Returns if the header is hidden and must be ignored in the IMF section.

src/mimeparser/mimeparser_tests.rs

@@ -1402,6 +1402,26 @@ async fn test_x_microsoft_original_message_id_precedence() -> Result<()> {
     Ok(())
 }
 
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_extra_imf_chat_header() -> Result<()> {
+    let mut tcm = TestContextManager::new();
+    let t = &tcm.alice().await;
+    let chat_id = t.get_self_chat().await.id;
+
+    chat::send_text_msg(t, chat_id, "hi!".to_string()).await?;
+    let sent_msg = t.pop_sent_msg().await;
+    // Check removal of some nonexistent "Chat-*" header to protect the code from future breakages.
+    let payload = sent_msg
+        .payload
+        .replace("Message-ID:", "Chat-Forty-Two: 42\r\nMessage-ID:");
+    let msg = MimeMessage::from_bytes(t, payload.as_bytes(), None)
+        .await
+        .unwrap();
+    assert!(msg.headers.contains_key("chat-version"));
+    assert!(!msg.headers.contains_key("chat-forty-two"));
+    Ok(())
+}
+
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn test_long_in_reply_to() -> Result<()> {
     let t = TestContext::new_alice().await;

src/receive_imf/receive_imf_tests.rs

@@ -3682,6 +3682,24 @@ async fn test_unsigned_chat_group_hdr() -> Result<()> {
     Ok(())
 }
 
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_ignore_protected_headers_in_outer_msg() -> Result<()> {
+    let mut tcm = TestContextManager::new();
+    let alice = &tcm.alice().await;
+    let bob = &tcm.bob().await;
+    let bob_chat_id = tcm.send_recv_accept(alice, bob, "hi").await.chat_id;
+    send_text_msg(bob, bob_chat_id, "hi all!".to_string()).await?;
+    let mut sent_msg = bob.pop_sent_msg().await;
+    sent_msg.payload = sent_msg.payload.replace(
+        "Chat-Version:",
+        "Auto-Submitted: auto-generated\r\nChat-Version:",
+    );
+    alice.recv_msg(&sent_msg).await;
+    let ab_contact = alice.add_or_lookup_contact(bob).await;
+    assert!(!ab_contact.is_bot());
+    Ok(())
+}
+
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn test_sync_member_list_on_rejoin() -> Result<()> {
     let mut tcm = TestContextManager::new();