diff --git a/CHANGELOG.md b/CHANGELOG.md index 35c0cc682..706a21708 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/python/tests/test_account.py b/python/tests/test_account.py index b22f485d1..323d7d0e8 100644 --- a/python/tests/test_account.py +++ b/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: + 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") diff --git a/src/dc_receive_imf.rs b/src/dc_receive_imf.rs index 10b581636..79bb4c407 100644 --- a/src/dc_receive_imf.rs +++ b/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 { 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 : diff --git a/src/imap.rs b/src/imap.rs index 93afe498e..1cadeac9b 100644 --- a/src/imap.rs +++ b/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 spam_target_folder( +async fn should_move_out_of_spam( context: &Context, - folder: &str, headers: &[mailparse::MailHeader<'_>], -) -> Result> { - if let Some(chat) = prefetch_get_chat(context, headers).await? { - if chat.blocked != Blocked::Not { +) -> Result { + if headers.get_header_value(HeaderDef::ChatVersion).is_some() { + // 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> { + 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 - // fetch it again: + // If OnlyFetchMvbox is set, we don't want to move the message to + // 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]