fix: Only include one From: header in securejoin messages (#5917)

This fixes the bug that sometimes made QR scans fail.

The problem was:

When sorting headers into unprotected/hidden/protected, the From: header
was added twice for all messages: Once into unprotected_headers and once
into protected_headers. For messages that are `is_encrypted && verified
|| is_securejoin_message`, the display name is removed before pushing it
into unprotected_headers.

Later, duplicate headers are removed from unprotected_headers right
before prepending unprotected_headers to the message. But since the
unencrypted From: header got modified a bit when removing the display
name, it's not exactly the same anymore, so it's not removed from
unprotected_headers and consequently added again.

src/mimefactory.rs

@@ -726,18 +726,18 @@ impl MimeFactory {
             } else if header_name == "autocrypt" {
                 unprotected_headers.push(header.clone());
             } else if header_name == "from" {
-                protected_headers.push(header.clone());
-                if is_encrypted && verified || is_securejoin_message {
-                    unprotected_headers.push(
-                        Header::new_with_value(
-                            header.name,
-                            vec![Address::new_mailbox(self.from_addr.clone())],
-                        )
-                        .unwrap(),
-                    );
-                } else {
-                    unprotected_headers.push(header);
+                // Unencrypted securejoin messages should _not_ include the display name:
+                if is_encrypted || !is_securejoin_message {
+                    protected_headers.push(header.clone());
                 }
+
+                unprotected_headers.push(
+                    Header::new_with_value(
+                        header.name,
+                        vec![Address::new_mailbox(self.from_addr.clone())],
+                    )
+                    .unwrap(),
+                );
             } else if header_name == "to" {
                 protected_headers.push(header.clone());
                 if is_encrypted {
@@ -902,12 +902,11 @@ impl MimeFactory {
                 .fold(message, |message, header| message.header(header.clone()));
 
             if skip_autocrypt || !context.get_config_bool(Config::SignUnencrypted).await? {
-                let protected: HashSet<Header> = HashSet::from_iter(protected_headers.into_iter());
-                for h in unprotected_headers.split_off(0) {
-                    if !protected.contains(&h) {
-                        unprotected_headers.push(h);
-                    }
-                }
+                // Deduplicate unprotected headers that also are in the protected headers:
+                let protected: HashSet<&str> =
+                    HashSet::from_iter(protected_headers.iter().map(|h| h.name.as_str()));
+                unprotected_headers.retain(|h| !protected.contains(&h.name.as_str()));
+
                 message
             } else {
                 let message = message.header(get_content_type_directives_header());

src/securejoin.rs

@@ -843,6 +843,7 @@ mod tests {
         );
 
         let sent = bob.pop_sent_msg().await;
+        assert!(!sent.payload.contains("Bob Examplenet"));
         assert_eq!(sent.recipient(), EmailAddress::new(alice_addr).unwrap());
         let msg = alice.parse_msg(&sent).await;
         assert!(!msg.was_encrypted());
@@ -860,6 +861,7 @@ mod tests {
         );
 
         let sent = alice.pop_sent_msg().await;
+        assert!(!sent.payload.contains("Alice Exampleorg"));
         let msg = bob.parse_msg(&sent).await;
         assert!(msg.was_encrypted());
         assert_eq!(

src/test_utils.rs

@@ -2,7 +2,7 @@
 //!
 //! This private module is only compiled for test runs.
 #![allow(clippy::indexing_slicing)]
-use std::collections::BTreeMap;
+use std::collections::{BTreeMap, HashSet};
 use std::fmt::Write;
 use std::ops::{Deref, DerefMut};
 use std::panic;
@@ -477,6 +477,36 @@ impl TestContext {
         update_msg_state(&self.ctx, msg_id, MessageState::OutDelivered)
             .await
             .expect("failed to update message state");
+
+        let payload_headers = payload.split("\r\n\r\n").next().unwrap().lines();
+        let payload_header_names: Vec<_> = payload_headers
+            .map(|h| h.split(':').next().unwrap())
+            .collect();
+
+        // Check that we are sending exactly one From, Subject, Date, To, Message-ID, and MIME-Version header:
+        for header in &[
+            "From",
+            "Subject",
+            "Date",
+            "To",
+            "Message-ID",
+            "MIME-Version",
+        ] {
+            assert_eq!(
+                payload_header_names.iter().filter(|h| *h == header).count(),
+                1,
+                "This sent email should contain the header {header} exactly 1 time:\n{payload}"
+            );
+        }
+        // Check that we aren't sending any header twice:
+        let mut hash_set = HashSet::new();
+        for header_name in payload_header_names {
+            assert!(
+                hash_set.insert(header_name),
+                "This sent email shouldn't contain the header {header_name} multiple times:\n{payload}"
+            );
+        }
+
         Some(SentMessage {
             payload,
             sender_msg_id: msg_id,

src/tests/verified_chats.rs

@@ -881,7 +881,7 @@ async fn test_verified_member_added_reordering() -> Result<()> {
 }
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
-async fn test_no_unencrypted_name_if_verified() -> Result<()> {
+async fn test_no_unencrypted_name_if_encrypted() -> Result<()> {
     let mut tcm = TestContextManager::new();
     for verified in [false, true] {
         let alice = tcm.alice().await;
@@ -898,7 +898,7 @@ async fn test_no_unencrypted_name_if_verified() -> Result<()> {
         let chat_id = bob.create_chat(&alice).await.id;
         let msg = &bob.send_text(chat_id, "hi").await;
 
-        assert_eq!(msg.payload.contains("Bob Smith"), !verified);
+        assert_eq!(msg.payload.contains("Bob Smith"), false);
         assert!(msg.payload.contains("BEGIN PGP MESSAGE"));
 
         let msg = alice.recv_msg(msg).await;