diff --git a/CHANGELOG.md b/CHANGELOG.md index 6764bdcd1..3fa164d69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ ### API-Changes ### Fixes +- Make sure malformed messsages will never block receiving further messages anymore #3771 ## 1.102.0 diff --git a/src/authres.rs b/src/authres.rs index 5620b1cb4..f9f9d11fb 100644 --- a/src/authres.rs +++ b/src/authres.rs @@ -13,8 +13,6 @@ use once_cell::sync::Lazy; use crate::config::Config; use crate::context::Context; use crate::headerdef::HeaderDef; -use crate::mimeparser; -use crate::mimeparser::ParserErrorExt; use crate::tools::time; use crate::tools::EmailAddress; @@ -32,23 +30,19 @@ pub(crate) async fn handle_authres( mail: &ParsedMail<'_>, from: &str, message_time: i64, -) -> mimeparser::ParserResult { +) -> Result { let from_domain = match EmailAddress::new(from) { Ok(email) => email.domain, Err(e) => { // This email is invalid, but don't return an error, we still want to // add a stub to the database so that it's not downloaded again - return Err(anyhow::format_err!("invalid email {}: {:#}", from, e)).map_err_malformed(); + return Err(anyhow::format_err!("invalid email {}: {:#}", from, e)); } }; let authres = parse_authres_headers(&mail.get_headers(), &from_domain); - update_authservid_candidates(context, &authres) - .await - .map_err_sql()?; - compute_dkim_results(context, authres, &from_domain, message_time) - .await - .map_err_sql() + update_authservid_candidates(context, &authres).await?; + compute_dkim_results(context, authres, &from_domain, message_time).await } #[derive(Default, Debug)] diff --git a/src/decrypt.rs b/src/decrypt.rs index cf3e66b98..e9937390a 100644 --- a/src/decrypt.rs +++ b/src/decrypt.rs @@ -13,7 +13,6 @@ use crate::context::Context; use crate::key::{DcKey, Fingerprint, SignedPublicKey, SignedSecretKey}; use crate::keyring::Keyring; use crate::log::LogExt; -use crate::mimeparser::{self, ParserErrorExt}; use crate::peerstate::Peerstate; use crate::pgp; @@ -61,7 +60,7 @@ pub(crate) async fn prepare_decryption( mail: &ParsedMail<'_>, from: &str, message_time: i64, -) -> mimeparser::ParserResult { +) -> Result { let autocrypt_header = Aheader::from_headers(from, &mail.headers) .ok_or_log_msg(context, "Failed to parse Autocrypt header") .flatten(); @@ -76,8 +75,7 @@ pub(crate) async fn prepare_decryption( // Disallowing keychanges is disabled for now: true, // dkim_results.allow_keychange, ) - .await - .map_err_sql()?; + .await?; Ok(DecryptionInfo { from: from.to_string(), diff --git a/src/imap.rs b/src/imap.rs index fc540d888..91948e0db 100644 --- a/src/imap.rs +++ b/src/imap.rs @@ -1364,7 +1364,9 @@ impl Imap { /// Fetches a list of messages by server UID. /// - /// Returns the last uid fetch successfully and the info about each downloaded message. + /// Returns the last UID fetched successfully and the info about each downloaded message. + /// If the message is incorrect or there is a failure to write a message to the database, + /// it is skipped and the error is logged. pub(crate) async fn fetch_many_msgs( &mut self, context: &Context, @@ -1474,12 +1476,12 @@ impl Imap { if let Some(m) = received_msg { received_msgs.push(m); } - last_uid = Some(server_uid) } Err(err) => { warn!(context, "receive_imf error: {:#}", err); } }; + last_uid = Some(server_uid) } } diff --git a/src/mimeparser.rs b/src/mimeparser.rs index 7dec08f53..24a6941e7 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -157,38 +157,9 @@ impl Default for SystemMessage { const MIME_AC_SETUP_FILE: &str = "application/autocrypt-setup"; -#[derive(Debug, thiserror::Error)] -pub(crate) enum ParserError { - #[error("{}", _0)] - Malformed(anyhow::Error), - - #[error("{:#}", _0)] - Sql(anyhow::Error), -} - -pub(crate) type ParserResult = std::result::Result; - -pub(crate) trait ParserErrorExt -where - Self: std::marker::Sized, -{ - fn map_err_malformed(self) -> ParserResult; - fn map_err_sql(self) -> ParserResult; -} - -impl> ParserErrorExt for Result { - fn map_err_malformed(self) -> ParserResult { - self.map_err(|e| ParserError::Malformed(e.into())) - } - - fn map_err_sql(self) -> ParserResult { - self.map_err(|e| ParserError::Sql(e.into())) - } -} - impl MimeMessage { pub async fn from_bytes(context: &Context, body: &[u8]) -> Result { - Ok(MimeMessage::from_bytes_with_partial(context, body, None).await?) + MimeMessage::from_bytes_with_partial(context, body, None).await } /// Parse a mime message. @@ -199,8 +170,8 @@ impl MimeMessage { context: &Context, body: &[u8], partial: Option, - ) -> ParserResult { - let mail = mailparse::parse_mail(body).map_err_malformed()?; + ) -> Result { + let mail = mailparse::parse_mail(body)?; let message_time = mail .headers @@ -227,7 +198,7 @@ impl MimeMessage { ); // Parse hidden headers. - let mimetype = mail.ctype.mimetype.parse::().map_err_malformed()?; + let mimetype = mail.ctype.mimetype.parse::()?; if mimetype.type_() == mime::MULTIPART && mimetype.subtype().as_str() == "mixed" { if let Some(part) = mail.subparts.first() { for field in &part.headers { @@ -245,7 +216,7 @@ impl MimeMessage { headers.remove("secure-join-fingerprint"); headers.remove("chat-verified"); - let from = from.context("No from in message").map_err_malformed()?; + let from = from.context("No from in message")?; let mut decryption_info = prepare_decryption(context, &mail, &from.addr, message_time).await?; @@ -265,7 +236,7 @@ impl MimeMessage { // autocrypt message. mail_raw = raw; - let decrypted_mail = mailparse::parse_mail(&mail_raw).map_err_malformed()?; + let decrypted_mail = mailparse::parse_mail(&mail_raw)?; if std::env::var(crate::DCC_MIME_DEBUG).is_ok() { info!(context, "decrypted message mime-body:"); println!("{}", String::from_utf8_lossy(&mail_raw)); @@ -279,8 +250,7 @@ impl MimeMessage { decrypted_mail.headers.get_all_values("Autocrypt-Gossip"); gossiped_addr = update_gossip_peerstates(context, message_time, &mail, gossip_headers) - .await - .map_err_sql()?; + .await?; } // let known protected headers from the decrypted @@ -333,10 +303,7 @@ impl MimeMessage { // && decryption_info.dkim_results.allow_keychange { peerstate.degrade_encryption(message_time); - peerstate - .save_to_db(&context.sql, false) - .await - .map_err_sql()?; + peerstate.save_to_db(&context.sql, false).await?; } } (Ok(mail), HashSet::new(), false) @@ -380,15 +347,11 @@ impl MimeMessage { Some(org_bytes) => { parser .create_stub_from_partial_download(context, org_bytes) - .await - .map_err_sql()?; + .await?; } None => match mail { Ok(mail) => { - parser - .parse_mime_recursive(context, &mail, false) - .await - .map_err_malformed()?; + parser.parse_mime_recursive(context, &mail, false).await?; } Err(err) => { let msg_body = stock_str::cant_decrypt_msg_body(context).await; @@ -409,7 +372,7 @@ impl MimeMessage { parser.maybe_remove_bad_parts(); parser.maybe_remove_inline_mailinglist_footer(); parser.heuristically_parse_ndn(context).await; - parser.parse_headers(context).await.map_err_malformed()?; + parser.parse_headers(context).await?; // Disallowing keychanges is disabled for now // if !decryption_info.dkim_results.allow_keychange { @@ -427,14 +390,11 @@ impl MimeMessage { parser.decoded_data = mail_raw; } - crate::peerstate::maybe_do_aeap_transition(context, &mut decryption_info, &parser) - .await - .map_err_sql()?; + crate::peerstate::maybe_do_aeap_transition(context, &mut decryption_info, &parser).await?; if let Some(peerstate) = decryption_info.peerstate { peerstate .handle_fingerprint_change(context, message_time) - .await - .map_err_sql()?; + .await?; } Ok(parser) @@ -2196,7 +2156,7 @@ mod tests { let mimeparser = MimeMessage::from_bytes_with_partial(&context.ctx, &raw[..], None).await; - assert!(matches!(mimeparser, Err(ParserError::Malformed(_)))); + assert!(mimeparser.is_err()); } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] diff --git a/src/receive_imf.rs b/src/receive_imf.rs index a60ebc339..76f7f1e4a 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -28,7 +28,7 @@ use crate::message::{ self, rfc724_mid_exists, Message, MessageState, MessengerMessage, MsgId, Viewtype, }; use crate::mimeparser::{ - parse_message_ids, AvatarAction, MailinglistType, MimeMessage, ParserError, SystemMessage, + parse_message_ids, AvatarAction, MailinglistType, MimeMessage, SystemMessage, }; use crate::param::{Param, Params}; use crate::peerstate::{Peerstate, PeerstateKeyType, PeerstateVerifiedStatus}; @@ -72,15 +72,13 @@ pub async fn receive_imf( /// Receive a message and add it to the database. /// -/// Returns an error on recoverable errors, e.g. database errors. In this case, -/// message parsing should be retried later. +/// Returns an error on database failure or if the message is broken, +/// e.g. has nonstandard MIME structure. /// -/// If message itself is wrong, logs -/// the error and returns success: -/// - If possible, creates a database entry to prevent the message from being -/// downloaded again, sets `chat_id=DC_CHAT_ID_TRASH` and returns `Ok(Some(…))` -/// - If the message is so wrong that we didn't even create a database entry, -/// returns `Ok(None)` +/// If possible, creates a database entry to prevent the message from being +/// downloaded again, sets `chat_id=DC_CHAT_ID_TRASH` and returns `Ok(Some(…))`. +/// If the message is so wrong that we didn't even create a database entry, +/// returns `Ok(None)`. /// /// If `is_partial_download` is set, it contains the full message size in bytes. /// Do not confuse that with `replace_partial_download` that will be set when the full message is loaded later. @@ -101,9 +99,8 @@ pub(crate) async fn receive_imf_inner( let mut mime_parser = match MimeMessage::from_bytes_with_partial(context, imf_raw, is_partial_download).await { - Err(ParserError::Malformed(err)) => { + Err(err) => { warn!(context, "receive_imf: can't parse MIME: {}", err); - let msg_ids; if !rfc724_mid.starts_with(GENERATED_PREFIX) { let row_id = context @@ -127,7 +124,6 @@ pub(crate) async fn receive_imf_inner( needs_delete_job: false, })); } - Err(ParserError::Sql(err)) => return Err(err), Ok(mime_parser) => mime_parser, }; @@ -2309,7 +2305,7 @@ mod tests { \n\ hello\x00"; let mimeparser = MimeMessage::from_bytes_with_partial(&context.ctx, &raw[..], None).await; - assert!(matches!(mimeparser, Err(ParserError::Malformed(_)))); + assert!(mimeparser.is_err()); } #[tokio::test(flavor = "multi_thread", worker_threads = 2)]