From 06b376d2427caff57bc596140e8954cdb3a70aff Mon Sep 17 00:00:00 2001 From: Hocuri Date: Tue, 22 Nov 2022 21:10:26 +0100 Subject: [PATCH] Bugfix: Don't let malformed From: headers block the receiving pipeline (#3769) That's a bug which @Simon-Laux and probably also @hpk42 had, where one malformed incoming (Spam-) mail blocked the receiving of all emails coming after it. The problem was that from_field_to_contact_id() returned ContactId::UNDEFINED, and then lookup_by_contact() returned Err. --- CHANGELOG.md | 1 + python/tests/test_1_online.py | 34 ++++++++++++++++++++++++++++++++-- src/imap.rs | 12 ++++++++++-- src/receive_imf.rs | 12 ++---------- 4 files changed, 45 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2967574e7..355a3522b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ ### Fixes - fix detection of "All mail", "Trash", "Junk" etc folders. #3760 - fetch messages sequentially to fix reactions on partially downloaded messages #3688 +- Fix a bug where one malformed message blocked receiving any further messages #3769 ## 1.101.0 diff --git a/python/tests/test_1_online.py b/python/tests/test_1_online.py index 5d4763394..2a7f1d78b 100644 --- a/python/tests/test_1_online.py +++ b/python/tests/test_1_online.py @@ -901,6 +901,34 @@ def test_dont_show_emails(acfactory, lp): ac1.get_config("configured_addr") ), ) + ac1.direct_imap.append( + "Spam", + """ + From: unknown.address@junk.org, unkwnown.add@junk.org + Subject: subj + To: {} + Message-ID: + Content-Type: text/plain; charset=utf-8 + + Unknown & malformed message in Spam + """.format( + ac1.get_config("configured_addr") + ), + ) + ac1.direct_imap.append( + "Spam", + """ + From: alice@example.org + Subject: subj + To: {} + Message-ID: + Content-Type: text/plain; charset=utf-8 + + Actually interesting message in Spam + """.format( + ac1.get_config("configured_addr") + ), + ) ac1.direct_imap.append( "Junk", """ @@ -926,7 +954,9 @@ def test_dont_show_emails(acfactory, lp): ac1._evtracker.wait_idle_inbox_ready() assert msg.text == "subj – message in Sent" - assert len(msg.chat.get_messages()) == 1 + chat_msgs = msg.chat.get_messages() + assert len(chat_msgs) == 2 + assert any(msg.text == "subj – Actually interesting message in Spam" for msg in chat_msgs) assert not any("unknown.address" in c.get_name() for c in ac1.get_chats()) ac1.direct_imap.select_folder("Spam") @@ -942,7 +972,7 @@ def test_dont_show_emails(acfactory, lp): msg2 = ac1._evtracker.wait_next_messages_changed() assert msg2.text == "subj – message in Drafts that is moved to Sent later" - assert len(msg.chat.get_messages()) == 2 + assert len(msg.chat.get_messages()) == 3 def test_no_old_msg_is_fresh(acfactory, lp): diff --git a/src/imap.rs b/src/imap.rs index bf5c9e312..fc540d888 100644 --- a/src/imap.rs +++ b/src/imap.rs @@ -1754,9 +1754,13 @@ async fn should_move_out_of_spam( return Ok(false); } } else { + let from = match mimeparser::get_from(headers) { + Some(f) => f, + None => return Ok(false), + }; // No chat found. let (from_id, blocked_contact, _origin) = - from_field_to_contact_id(context, mimeparser::get_from(headers).as_ref(), true).await?; + from_field_to_contact_id(context, &from, true).await?; if blocked_contact { // Contact is blocked, leave the message in spam. return Ok(false); @@ -2041,8 +2045,12 @@ pub(crate) async fn prefetch_should_download( .get_header_value(HeaderDef::AutocryptSetupMessage) .is_some(); + let from = match mimeparser::get_from(headers) { + Some(f) => f, + None => return Ok(false), + }; let (_from_id, blocked_contact, origin) = - from_field_to_contact_id(context, mimeparser::get_from(headers).as_ref(), true).await?; + from_field_to_contact_id(context, &from, true).await?; // prevent_rename=true as this might be a mailing list message and in this case it would be bad if we rename the contact. // (prevent_rename is the last argument of from_field_to_contact_id()) diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 2286c040c..a60ebc339 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -172,7 +172,7 @@ pub(crate) async fn receive_imf_inner( // If this is a mailing list email (i.e. list_id_header is some), don't change the displayname because in // a mailing list the sender displayname sometimes does not belong to the sender email address. let (from_id, _from_id_blocked, incoming_origin) = - from_field_to_contact_id(context, Some(&mime_parser.from), prevent_rename).await?; + from_field_to_contact_id(context, &mime_parser.from, prevent_rename).await?; let incoming = from_id != ContactId::SELF; @@ -370,17 +370,9 @@ pub(crate) async fn receive_imf_inner( /// * `prevent_rename`: passed through to `add_or_lookup_contacts_by_address_list()` pub async fn from_field_to_contact_id( context: &Context, - from: Option<&SingleInfo>, + from: &SingleInfo, prevent_rename: bool, ) -> Result<(ContactId, bool, Origin)> { - let from = match from { - Some(f) => f, - None => { - warn!(context, "mail has an empty From header"); - return Ok((ContactId::UNDEFINED, false, Origin::Unknown)); - } - }; - let display_name = if prevent_rename { Some("") } else {