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 exception is Subject because there are known cases when it's only
present in the outer message part, e.g. an encrypted unsigned Thunderbird message.

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

@@ -446,26 +446,9 @@ 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);
-                }
+                // Other headers are removed by `MimeMessage::merge_headers()`.
+                remove_header(&mut headers, "subject", &mut headers_removed);
             }
 
             // let known protected headers from the decrypted
@@ -1565,12 +1548,15 @@ impl MimeMessage {
         chat_disposition_notification_to: &mut Option<SingleInfo>,
         fields: &[mailparse::MailHeader<'_>],
     ) {
+        // Keep Subject so that it's displayed for signed-only messages. They are shown w/o a
+        // padlock anyway.
+        headers.retain(|k, _| !is_protected(k) || k == "subject");
         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 +1997,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();