Don't directly download messages from the Spam folder (#3015)

fix #3007

My approach is:

We don't download any messages from the spam folder anymore, and only download them if they were moved out. This means that is-it-spam logic only resides in spam_target_folder(). This has some implications, see the comments.

CHANGELOG.md

@@ -10,6 +10,7 @@
 ### Fixes
 - avoid archived, fresh chats #3053
 - treat "NO" IMAP response to MOVE and COPY commands as an error #3058
+- Fix a bug where messages in the Spam folder created contact requests #3015
 
 
 ## 1.75.0

python/tests/test_account.py

@@ -1398,11 +1398,13 @@ class TestOnlineAccount:
         assert not device_chat.can_send()
         assert device_chat.get_draft() is None
 
-    def test_dont_show_emails_in_draft_folder(self, acfactory, lp):
+    def test_dont_show_emails(self, acfactory, lp):
         """Most mailboxes have a "Drafts" folder where constantly new emails appear but we don't actually want to show them.
         So: If it's outgoing AND there is no Received header AND it's not in the sentbox, then ignore the email.
 
-        If the draft email is sent out later (i.e. moved to "Sent"), it must be shown."""
+        If the draft email is sent out later (i.e. moved to "Sent"), it must be shown.
+
+        Also, test that unknown emails in the Spam folder are not shown."""
         ac1 = acfactory.get_online_configuring_account()
         ac1.set_config("show_emails", "2")
         ac1.create_contact("alice@example.org").create_chat()
@@ -1410,6 +1412,7 @@ class TestOnlineAccount:
         acfactory.wait_configure(ac1)
         ac1.direct_imap.create_folder("Drafts")
         ac1.direct_imap.create_folder("Sent")
+        ac1.direct_imap.create_folder("Spam")
 
         acfactory.wait_configure_and_start_io()
         # Wait until each folder was selected once and we are IDLEing again:
@@ -1434,6 +1437,15 @@ class TestOnlineAccount:
 
             message in Sent
         """.format(ac1.get_config("configured_addr")))
+        ac1.direct_imap.append("Spam", """
+            From: unknown.address@junk.org
+            Subject: subj
+            To: {}
+            Message-ID: <spam.message@junk.org>
+            Content-Type: text/plain; charset=utf-8
+
+            Unknown message in Spam
+        """.format(ac1.get_config("configured_addr")))
 
         ac1.set_config("scan_all_folders_debounce_secs", "0")
         lp.sec("All prepared, now let DC find the message")
@@ -1447,6 +1459,10 @@ class TestOnlineAccount:
         assert msg.text == "subj – message in Sent"
         assert len(msg.chat.get_messages()) == 1
 
+        assert not any("unknown.address" in c.get_name() for c in ac1.get_chats())
+        ac1.direct_imap.select_folder("Spam")
+        assert ac1.direct_imap.get_uid_by_message_id("spam.message@junk.org")
+
         ac1.stop_io()
         lp.sec("'Send out' the draft, i.e. move it to the Sent folder, and wait for DC to display it this time")
         ac1.direct_imap.select_folder("Drafts")

src/dc_receive_imf.rs

@@ -193,7 +193,6 @@ pub(crate) async fn dc_receive_imf_inner(
         &mut mime_parser,
         imf_raw,
         incoming,
-        incoming_origin,
         server_folder,
         &to_ids,
         &rfc724_mid,
@@ -415,7 +414,6 @@ async fn add_parts(
     mime_parser: &mut MimeMessage,
     imf_raw: &[u8],
     incoming: bool,
-    incoming_origin: Origin,
     server_folder: &str,
     to_ids: &[u32],
     rfc724_mid: &str,
@@ -432,7 +430,6 @@ async fn add_parts(
 ) -> Result<ReceivedMsg> {
     let mut chat_id = None;
     let mut chat_id_blocked = Blocked::Not;
-    let mut incoming_origin = incoming_origin;
 
     let parent = get_parent_message(context, mime_parser).await?;
 
@@ -676,7 +673,6 @@ async fn add_parts(
                 if chat_id_blocked != Blocked::Not {
                     if chat_id_blocked != create_blocked {
                         chat_id.set_blocked(context, create_blocked).await?;
-                        chat_id_blocked = create_blocked;
                     }
                     if create_blocked == Blocked::Request && parent.is_some() {
                         // we do not want any chat to be created implicitly.  Because of the origin-scale-up,
@@ -687,9 +683,6 @@ async fn add_parts(
                             context,
                             "Message is a reply to a known message, mark sender as known.",
                         );
-                        if !incoming_origin.is_known() {
-                            incoming_origin = Origin::IncomingReplyTo;
-                        }
                     }
                 }
             }
@@ -701,15 +694,6 @@ async fn add_parts(
             } else {
                 MessageState::InFresh
             };
-
-        let is_spam = (chat_id_blocked == Blocked::Request)
-            && !incoming_origin.is_known()
-            && (is_dc_message == MessengerMessage::No)
-            && context.is_spam_folder(server_folder).await?;
-        if is_spam {
-            chat_id = Some(DC_CHAT_ID_TRASH);
-            info!(context, "Message is probably spam (TRASH)");
-        }
     } else {
         // Outgoing
 
@@ -2363,12 +2347,10 @@ fn dc_create_incoming_rfc724_mid(mime: &MimeMessage) -> String {
 
 #[cfg(test)]
 mod tests {
-    use chat::get_chat_contacts;
-
-    use mailparse::MailHeaderMap;
 
     use super::*;
 
+    use crate::chat::get_chat_contacts;
     use crate::chat::{get_chat_msgs, ChatItem, ChatVisibility};
     use crate::chatlist::Chatlist;
     use crate::constants::{DC_CONTACT_ID_INFO, DC_GCL_NO_SPECIALS};
@@ -2977,7 +2959,7 @@ mod tests {
             &headers,
             "some-other-message-id",
             std::iter::empty(),
-            ShowEmails::Off
+            ShowEmails::Off,
         )
         .await
         .unwrap());
@@ -4298,76 +4280,6 @@ YEAAAAAA!.
         assert_eq!(msg.text.unwrap(), "Reply");
     }
 
-    #[async_std::test]
-    async fn test_dont_show_spam() {
-        async fn is_shown(t: &TestContext, raw: &[u8], server_folder: &str) -> bool {
-            let mail = mailparse::parse_mail(raw).unwrap();
-            dc_receive_imf(t, raw, server_folder, false).await.unwrap();
-            t.get_last_msg().await.rfc724_mid
-                == mail.get_headers().get_first_value("Message-Id").unwrap()
-        }
-
-        let t = TestContext::new_alice().await;
-        t.set_config(Config::ConfiguredSpamFolder, Some("Spam"))
-            .await
-            .unwrap();
-        t.set_config(Config::ShowEmails, Some("2")).await.unwrap();
-
-        assert!(
-            is_shown(
-                &t,
-                b"Message-Id: abcd1@exmaple.com\n\
-                From: bob@example.org\n\
-                Chat-Version: 1.0\n",
-                "Inbox",
-            )
-            .await,
-        );
-
-        assert!(
-            is_shown(
-                &t,
-                b"Message-Id: abcd2@exmaple.com\n\
-                From: bob@example.org\n",
-                "Inbox",
-            )
-            .await,
-        );
-
-        assert!(
-            is_shown(
-                &t,
-                b"Message-Id: abcd3@exmaple.com\n\
-                From: bob@example.org\n\
-                Chat-Version: 1.0\n",
-                "Spam",
-            )
-            .await,
-        );
-
-        assert!(
-            // Note the `!`:
-            !is_shown(
-                &t,
-                b"Message-Id: abcd4@exmaple.com\n\
-                From: bob@example.org\n",
-                "Spam",
-            )
-            .await,
-        );
-
-        Contact::create(&t, "", "bob@example.org").await.unwrap();
-        assert!(
-            is_shown(
-                &t,
-                b"Message-Id: abcd5@exmaple.com\n\
-                From: bob@example.org\n",
-                "Spam",
-            )
-            .await,
-        );
-    }
-
     #[async_std::test]
     async fn test_dont_show_all_outgoing_msgs_in_self_chat() {
         // Regression test for <https://github.com/deltachat/deltachat-android/issues/1940>:

src/imap.rs

@@ -748,6 +748,11 @@ impl Imap {
             // message, move it to the movebox and then download the second message before
             // downloading the first one, if downloading from inbox before moving is allowed.
             if folder == target
+                // Never download messages directly from the spam folder.
+                // If the sender is known, the message will be moved to the Inbox or Mvbox
+                // and then we download the message from there.
+                // Also see `spam_target_folder()`.
+                && !context.is_spam_folder(folder).await?
                 && prefetch_should_download(
                     context,
                     &headers,
@@ -1598,16 +1603,28 @@ impl Imap {
     }
 }
 
-/// Returns target folder for a message found in the Spam folder.
+async fn should_move_out_of_spam(
-async fn spam_target_folder(
     context: &Context,
-    folder: &str,
     headers: &[mailparse::MailHeader<'_>],
-) -> Result<Option<Config>> {
+) -> Result<bool> {
-    if let Some(chat) = prefetch_get_chat(context, headers).await? {
+    if headers.get_header_value(HeaderDef::ChatVersion).is_some() {
-        if chat.blocked != Blocked::Not {
+        // If this is a chat message (i.e. has a ChatVersion header), then this might be
+        // a securejoin message. We can't find out at this point as we didn't prefetch
+        // the SecureJoin header. So, we always move chat messages out of Spam.
+        // Two possibilities to change this would be:
+        // 1. Remove the `&& !context.is_spam_folder(folder).await?` check from
+        // `fetch_new_messages()`, and then let `dc_receive_imf()` check
+        // if it's a spam message and should be hidden.
+        // 2. Or add a flag to the ChatVersion header that this is a securejoin
+        // request, and return `true` here only if the message has this flag.
+        // `dc_receive_imf()` can then check if the securejoin request is valid.
+        return Ok(true);
+    }
+
+    if let Some(msg) = get_prefetch_parent_message(context, headers).await? {
+        if msg.chat_blocked != Blocked::Not {
             // Blocked or contact request message in the spam folder, leave it there.
-            return Ok(None);
+            return Ok(false);
         }
     } else {
         // No chat found.
@@ -1615,22 +1632,38 @@ async fn spam_target_folder(
             from_field_to_contact_id(context, &mimeparser::get_from(headers), true).await?;
         if blocked_contact {
             // Contact is blocked, leave the message in spam.
-            return Ok(None);
+            return Ok(false);
         }
 
         if let Some(chat_id_blocked) = ChatIdBlocked::lookup_by_contact(context, from_id).await? {
             if chat_id_blocked.blocked != Blocked::Not {
-                return Ok(None);
+                return Ok(false);
             }
         } else if from_id != DC_CONTACT_ID_SELF {
             // No chat with this contact found.
-            return Ok(None);
+            return Ok(false);
         }
     }
 
+    Ok(true)
+}
+
+/// Returns target folder for a message found in the Spam folder.
+/// If this returns None, the message will not be moved out of the
+/// Spam folder, and as `fetch_new_messages()` doesn't download
+/// messages from the Spam folder, the message will be ignored.
+async fn spam_target_folder(
+    context: &Context,
+    folder: &str,
+    headers: &[mailparse::MailHeader<'_>],
+) -> Result<Option<Config>> {
+    if !should_move_out_of_spam(context, headers).await? {
+        return Ok(None);
+    }
+
     if needs_move_to_mvbox(context, headers).await?
-        // We don't want to move the message to the inbox or sentbox where we wouldn't
+        // If OnlyFetchMvbox is set, we don't want to move the message to
-        // fetch it again:
+        // the inbox or sentbox where we wouldn't fetch it again:
         || context.get_config_bool(Config::OnlyFetchMvbox).await?
     {
         Ok(Some(Config::ConfiguredMvboxFolder))
@@ -2383,7 +2416,7 @@ mod tests {
         ("Spam", true, true, "DeltaChat"),
     ];
 
-    // These are the same as above, but all messages in Spam stay in Spam
+    // These are the same as above, but non-chat messages in Spam stay in Spam
     const COMBINATIONS_REQUEST: &[(&str, bool, bool, &str)] = &[
         ("INBOX", false, false, "INBOX"),
         ("INBOX", false, true, "INBOX"),
@@ -2394,9 +2427,9 @@ mod tests {
         ("Sent", true, false, "Sent"),
         ("Sent", true, true, "DeltaChat"),
         ("Spam", false, false, "Spam"),
-        ("Spam", false, true, "Spam"),
+        ("Spam", false, true, "INBOX"),
         ("Spam", true, false, "Spam"),
-        ("Spam", true, true, "Spam"),
+        ("Spam", true, true, "DeltaChat"),
     ];
 
     #[async_std::test]