Fix #1120 Contact requests are not shown when name of sender includes a comma character (#1438)

* First try making get_recipients use MailHeader (nice and functional)

* Get it to compile by using not-so-functional style

* Add "empty-from" test, drop unnecessary check for error; continue using addrparse_header() instead of addrparse()

* Try to use functional style, unfortunately, I can't get the compiler to accept it

* Do it imperative-style: Do not overwrite To with Cc and vice versa

* Use addrparse_header() once more

* Still addrparse_header()

* Clippy

* Fix compile errors in tests

* Fix typo

* Fix tests again ;-)

* Code style

* Code style; try a HashMap<addr: String, display_name: String> as an address list but I am not convinced

* Code style; Use Vec<SingleInfo> as address list

* Clippy

* Add tests

* Add another test

* Remove stale comments
This commit is contained in:
Hocuri
2020-05-07 13:55:09 +02:00
committed by GitHub
parent 4724101e75
commit a586a1d525
5 changed files with 295 additions and 147 deletions

View File

@@ -3,6 +3,8 @@ use sha2::{Digest, Sha256};
use num_traits::FromPrimitive;
use mailparse::SingleInfo;
use crate::chat::{self, Chat, ChatId};
use crate::config::Config;
use crate::constants::*;
@@ -110,29 +112,23 @@ pub fn dc_receive_imf(
// we do not check Return-Path any more as this is unreliable, see
// https://github.com/deltachat/deltachat-core/issues/150)
let (from_id, from_id_blocked, incoming_origin) =
if let Some(field_from) = mime_parser.get(HeaderDef::From_) {
from_field_to_contact_id(context, field_from)?
} else {
(0, false, Origin::Unknown)
};
from_field_to_contact_id(context, &mime_parser.from)?;
let incoming = from_id != DC_CONTACT_ID_SELF;
let mut to_ids = ContactIds::new();
for header_def in &[HeaderDef::To, HeaderDef::Cc] {
if let Some(field) = mime_parser.get(header_def.clone()) {
to_ids.extend(&dc_add_or_lookup_contacts_by_address_list(
context,
&field,
if !incoming {
Origin::OutgoingTo
} else if incoming_origin.is_known() {
Origin::IncomingTo
} else {
Origin::IncomingUnknownTo
},
)?);
}
}
to_ids.extend(&dc_add_or_lookup_contacts_by_address_list(
context,
&mime_parser.recipients,
if !incoming {
Origin::OutgoingTo
} else if incoming_origin.is_known() {
Origin::IncomingTo
} else {
Origin::IncomingUnknownTo
},
)?);
// Add parts
@@ -242,11 +238,11 @@ pub fn dc_receive_imf(
/// Also returns whether it is blocked or not and its origin.
pub fn from_field_to_contact_id(
context: &Context,
field_from: &str,
from_address_list: &[SingleInfo],
) -> Result<(u32, bool, Origin)> {
let from_ids = dc_add_or_lookup_contacts_by_address_list(
context,
&field_from,
from_address_list,
Origin::IncomingUnknownFrom,
)?;
@@ -256,7 +252,7 @@ pub fn from_field_to_contact_id(
if from_ids.len() > 1 {
warn!(
context,
"mail has more than one From address, only using first: {:?}", field_from
"mail has more than one From address, only using first: {:?}", from_address_list
);
}
let from_id = from_ids.get_index(0).cloned().unwrap_or_default();
@@ -269,7 +265,10 @@ pub fn from_field_to_contact_id(
}
Ok((from_id, from_id_blocked, incoming_origin))
} else {
warn!(context, "mail has an empty From header: {:?}", field_from);
warn!(
context,
"mail has an empty From header: {:?}", from_address_list
);
// if there is no from given, from_id stays 0 which is just fine. These messages
// are very rare, however, we have to add them to the database (they go to the
// "deaddrop" chat) to avoid a re-download from the server. See also [**]
@@ -1593,38 +1592,17 @@ fn is_msgrmsg_rfc724_mid(context: &Context, rfc724_mid: &str) -> bool {
fn dc_add_or_lookup_contacts_by_address_list(
context: &Context,
addr_list_raw: &str,
address_list: &[SingleInfo],
origin: Origin,
) -> Result<ContactIds> {
let addrs = match mailparse::addrparse(addr_list_raw) {
Ok(addrs) => addrs,
Err(err) => {
bail!("could not parse {:?}: {:?}", addr_list_raw, err);
}
};
let mut contact_ids = ContactIds::new();
for addr in addrs.iter() {
match addr {
mailparse::MailAddr::Single(info) => {
contact_ids.insert(add_or_lookup_contact_by_addr(
context,
&info.display_name,
&info.addr,
origin,
)?);
}
mailparse::MailAddr::Group(infos) => {
for info in &infos.addrs {
contact_ids.insert(add_or_lookup_contact_by_addr(
context,
&info.display_name,
&info.addr,
origin,
)?);
}
}
}
for info in address_list.iter() {
contact_ids.insert(add_or_lookup_contact_by_addr(
context,
&info.display_name,
&info.addr,
origin,
)?);
}
Ok(contact_ids)
@@ -2017,4 +1995,151 @@ mod tests {
let one2one = Chat::load_from_db(&t.ctx, one2one_id).unwrap();
assert!(one2one.get_visibility() == ChatVisibility::Archived);
}
#[test]
fn test_no_from() {
// if there is no from given, from_id stays 0 which is just fine. These messages
// are very rare, however, we have to add them to the database (they go to the
// "deaddrop" chat) to avoid a re-download from the server. See also [**]
let t = configured_offline_context();
let context = &t.ctx;
let chats = Chatlist::try_load(&t.ctx, 0, None, None).unwrap();
assert!(chats.get_msg_id(0).is_err());
dc_receive_imf(
context,
b"To: bob@example.org\n\
Subject: foo\n\
Message-ID: <3924@example.org>\n\
Chat-Version: 1.0\n\
Date: Sun, 22 Mar 2020 22:37:57 +0000\n\
\n\
hello\n",
"INBOX",
1,
false,
)
.unwrap();
let chats = Chatlist::try_load(&t.ctx, 0, None, None).unwrap();
// Check that the message was added to the database:
assert!(chats.get_msg_id(0).is_ok());
}
#[test]
fn test_escaped_from() {
let t = configured_offline_context();
let contact_id = Contact::create(&t.ctx, "foobar", "foobar@example.com").unwrap();
let chat_id = chat::create_by_contact_id(&t.ctx, contact_id).unwrap();
dc_receive_imf(
&t.ctx,
b"From: =?UTF-8?B?0JjQvNGPLCDQpNCw0LzQuNC70LjRjw==?= <foobar@example.com>\n\
To: alice@example.org\n\
Subject: foo\n\
Message-ID: <asdklfjjaweofi@example.org>\n\
Chat-Version: 1.0\n\
Chat-Disposition-Notification-To: =?UTF-8?B?0JjQvNGPLCDQpNCw0LzQuNC70LjRjw==?= <foobar@example.com>\n\
Date: Sun, 22 Mar 2020 22:37:57 +0000\n\
\n\
hello\n",
"INBOX",
1,
false,
).unwrap();
assert_eq!(
Contact::load_from_db(&t.ctx, contact_id)
.unwrap()
.get_authname(),
"Фамилия Имя", // The name was "Имя, Фамилия" and ("lastname, firstname") and should be swapped to "firstname, lastname"
);
let msgs = chat::get_chat_msgs(&t.ctx, chat_id, 0, None);
assert_eq!(msgs.len(), 1);
let msg_id = msgs.first().unwrap();
let msg = message::Message::load_from_db(&t.ctx, msg_id.clone()).unwrap();
assert_eq!(msg.is_dc_message, MessengerMessage::Yes);
assert_eq!(msg.text.unwrap(), "hello");
assert_eq!(msg.param.get_int(Param::WantsMdn).unwrap(), 1);
}
#[test]
fn test_escaped_recipients() {
let t = configured_offline_context();
Contact::create(&t.ctx, "foobar", "foobar@example.com").unwrap();
let carl_contact_id =
Contact::add_or_lookup(&t.ctx, "Carl", "carl@host.tld", Origin::IncomingUnknownFrom)
.unwrap()
.0;
dc_receive_imf(
&t.ctx,
b"From: Foobar <foobar@example.com>\n\
To: =?UTF-8?B?0JjQvNGPLCDQpNCw0LzQuNC70LjRjw==?= alice@example.org\n\
Cc: =?utf-8?q?=3Ch2=3E?= <carl@host.tld>\n\
Subject: foo\n\
Message-ID: <asdklfjjaweofi@example.org>\n\
Chat-Version: 1.0\n\
Chat-Disposition-Notification-To: <foobar@example.com>\n\
Date: Sun, 22 Mar 2020 22:37:57 +0000\n\
\n\
hello\n",
"INBOX",
1,
false,
)
.unwrap();
assert_eq!(
Contact::load_from_db(&t.ctx, carl_contact_id)
.unwrap()
.get_name(),
"h2"
);
let chats = Chatlist::try_load(&t.ctx, 0, None, None).unwrap();
let msg = Message::load_from_db(&t.ctx, chats.get_msg_id(0).unwrap()).unwrap();
assert_eq!(msg.is_dc_message, MessengerMessage::Yes);
assert_eq!(msg.text.unwrap(), "hello");
assert_eq!(msg.param.get_int(Param::WantsMdn).unwrap(), 1);
}
#[test]
fn test_cc_to_contact() {
let t = configured_offline_context();
Contact::create(&t.ctx, "foobar", "foobar@example.com").unwrap();
let carl_contact_id = Contact::add_or_lookup(
&t.ctx,
"garabage",
"carl@host.tld",
Origin::IncomingUnknownFrom,
)
.unwrap()
.0;
dc_receive_imf(
&t.ctx,
b"From: Foobar <foobar@example.com>\n\
To: alice@example.org\n\
Cc: Carl <carl@host.tld>\n\
Subject: foo\n\
Message-ID: <asdklfjjaweofi@example.org>\n\
Chat-Version: 1.0\n\
Chat-Disposition-Notification-To: <foobar@example.com>\n\
Date: Sun, 22 Mar 2020 22:37:57 +0000\n\
\n\
hello\n",
"INBOX",
1,
false,
)
.unwrap();
assert_eq!(
Contact::load_from_db(&t.ctx, carl_contact_id)
.unwrap()
.get_name(),
"Carl"
);
}
}

View File

@@ -9,7 +9,8 @@ use crate::aheader::*;
use crate::config::Config;
use crate::context::Context;
use crate::error::*;
use crate::headerdef::{HeaderDef, HeaderDefMap};
use crate::headerdef::HeaderDef;
use crate::headerdef::HeaderDefMap;
use crate::key::{DcKey, Key, SignedPublicKey, SignedSecretKey};
use crate::keyring::*;
use crate::peerstate::*;
@@ -122,8 +123,8 @@ pub fn try_decrypt(
) -> Result<(Option<Vec<u8>>, HashSet<String>)> {
let from = mail
.headers
.get_header_value(HeaderDef::From_)
.and_then(|from_addr| mailparse::addrparse(&from_addr).ok())
.get_header(HeaderDef::From_)
.and_then(|from_addr| mailparse::addrparse_header(&from_addr).ok())
.and_then(|from| from.extract_single_info())
.map(|from| from.addr)
.unwrap_or_default();

View File

@@ -53,12 +53,16 @@ impl HeaderDef {
pub trait HeaderDefMap {
fn get_header_value(&self, headerdef: HeaderDef) -> Option<String>;
fn get_header(&self, headerdef: HeaderDef) -> Option<&MailHeader>;
}
impl HeaderDefMap for [MailHeader<'_>] {
fn get_header_value(&self, headerdef: HeaderDef) -> Option<String> {
self.get_first_value(headerdef.get_headername())
}
fn get_header(&self, headerdef: HeaderDef) -> Option<&MailHeader> {
self.get_first_header(headerdef.get_headername())
}
}
#[cfg(test)]

View File

@@ -25,6 +25,7 @@ use crate::headerdef::{HeaderDef, HeaderDefMap};
use crate::job::{job_add, Action};
use crate::login_param::{CertificateChecks, LoginParam};
use crate::message::{self, update_server_uid};
use crate::mimeparser;
use crate::oauth2::dc_get_oauth2_access_token;
use crate::param::Params;
use crate::stock::StockMessage;
@@ -1373,11 +1374,8 @@ fn prefetch_should_download(
.get_header_value(HeaderDef::AutocryptSetupMessage)
.is_some();
let from_field = headers
.get_header_value(HeaderDef::From_)
.unwrap_or_default();
let (_contact_id, blocked_contact, origin) = from_field_to_contact_id(context, &from_field)?;
let (_contact_id, blocked_contact, origin) =
from_field_to_contact_id(context, &mimeparser::get_from(headers))?;
let accepted_contact = origin.is_known();
let show = is_autocrypt_setup_message

View File

@@ -3,7 +3,7 @@ use std::collections::{HashMap, HashSet};
use anyhow::Context as _;
use deltachat_derive::{FromSql, ToSql};
use lettre_email::mime::{self, Mime};
use mailparse::{DispositionType, MailAddr, MailHeaderMap};
use mailparse::{addrparse_header, DispositionType, MailHeader, MailHeaderMap, SingleInfo};
use crate::aheader::Aheader;
use crate::blob::BlobObject;
@@ -37,6 +37,11 @@ use crate::stock::StockMessage;
pub struct MimeMessage {
pub parts: Vec<Part>,
header: HashMap<String, String>,
/// Addresses are normalized and lowercased:
pub recipients: Vec<SingleInfo>,
pub from: Vec<SingleInfo>,
pub chat_disposition_notification_to: Option<SingleInfo>,
pub decrypting_failed: bool,
pub signatures: HashSet<String>,
pub gossipped_addr: HashSet<String>,
@@ -88,9 +93,19 @@ impl MimeMessage {
.unwrap_or_default();
let mut headers = Default::default();
let mut recipients = Default::default();
let mut from = Default::default();
let mut chat_disposition_notification_to = None;
// init known headers with what mailparse provided us
MimeMessage::merge_headers(&mut headers, &mail.headers);
MimeMessage::merge_headers(
context,
&mut headers,
&mut recipients,
&mut from,
&mut chat_disposition_notification_to,
&mail.headers,
);
// remove headers that are allowed _only_ in the encrypted part
headers.remove("secure-join-fingerprint");
@@ -118,7 +133,14 @@ impl MimeMessage {
// let known protected headers from the decrypted
// part override the unencrypted top-level
MimeMessage::merge_headers(&mut headers, &decrypted_mail.headers);
MimeMessage::merge_headers(
context,
&mut headers,
&mut recipients,
&mut from,
&mut chat_disposition_notification_to,
&decrypted_mail.headers,
);
(decrypted_mail, signatures)
} else {
@@ -142,6 +164,9 @@ impl MimeMessage {
let mut parser = MimeMessage {
parts: Vec::new(),
header: headers,
recipients,
from,
chat_disposition_notification_to,
decrypting_failed: false,
// only non-empty if it was a valid autocrypt message
@@ -310,11 +335,9 @@ impl MimeMessage {
// See if an MDN is requested from the other side
if !self.decrypting_failed && !self.parts.is_empty() {
if let Some(ref dn_to_addr) =
self.parse_first_addr(context, HeaderDef::ChatDispositionNotificationTo)
{
if let Some(ref from_addr) = self.parse_first_addr(context, HeaderDef::From_) {
if compare_addrs(from_addr, dn_to_addr) {
if let Some(ref dn_to) = self.chat_disposition_notification_to {
if let Some(ref from) = self.from.get(0) {
if from.addr == dn_to.addr {
if let Some(part) = self.parts.last_mut() {
part.param.set_int(Param::WantsMdn, 1);
}
@@ -388,20 +411,6 @@ impl MimeMessage {
self.header.get(headerdef.get_headername())
}
fn parse_first_addr(&self, context: &Context, headerdef: HeaderDef) -> Option<MailAddr> {
if let Some(value) = self.get(headerdef.clone()) {
match mailparse::addrparse(&value) {
Ok(ref addrs) => {
return addrs.first().cloned();
}
Err(err) => {
warn!(context, "header {} parse error: {:?}", headerdef, err);
}
}
}
None
}
fn parse_mime_recursive(
&mut self,
context: &Context,
@@ -745,17 +754,41 @@ impl MimeMessage {
.and_then(|msgid| parse_message_id(msgid).ok())
}
fn merge_headers(headers: &mut HashMap<String, String>, fields: &[mailparse::MailHeader<'_>]) {
fn merge_headers(
context: &Context,
headers: &mut HashMap<String, String>,
recipients: &mut Vec<SingleInfo>,
from: &mut Vec<SingleInfo>,
chat_disposition_notification_to: &mut Option<SingleInfo>,
fields: &[mailparse::MailHeader<'_>],
) {
for field in fields {
// lowercasing all headers is technically not correct, but makes things work better
let key = field.get_key().to_lowercase();
if !headers.contains_key(&key) || // key already exists, only overwrite known types (protected headers)
is_known(&key) || key.starts_with("chat-")
{
let value = field.get_value();
headers.insert(key.to_string(), value);
if key == HeaderDef::ChatDispositionNotificationTo.get_headername() {
match addrparse_header(field) {
Ok(addrlist) => {
*chat_disposition_notification_to = addrlist.extract_single_info();
}
Err(e) => warn!(context, "Could not read {} address: {}", key, e),
}
} else {
let value = field.get_value();
headers.insert(key.to_string(), value);
}
}
}
let recipients_new = get_recipients(fields);
if !recipients_new.is_empty() {
*recipients = recipients_new;
}
let from_new = get_from(fields);
if !from_new.is_empty() {
*from = from_new;
}
}
fn process_report(
@@ -823,23 +856,15 @@ fn update_gossip_peerstates(
gossip_headers: Vec<String>,
) -> Result<HashSet<String>> {
// XXX split the parsing from the modification part
let mut recipients: Option<HashSet<String>> = None;
let mut gossipped_addr: HashSet<String> = Default::default();
for value in &gossip_headers {
let gossip_header = value.parse::<Aheader>();
if let Ok(ref header) = gossip_header {
if recipients.is_none() {
recipients = Some(get_recipients(
mail.headers.iter().map(|v| (v.get_key(), v.get_value())),
));
}
if recipients
.as_ref()
.unwrap()
.contains(&header.addr.to_lowercase())
if get_recipients(&mail.headers)
.iter()
.any(|info| info.addr == header.addr.to_lowercase())
{
let mut peerstate = Peerstate::from_addr(context, &context.sql, &header.addr);
if let Some(ref mut peerstate) = peerstate {
@@ -1004,50 +1029,50 @@ fn get_attachment_filename(mail: &mailparse::ParsedMail) -> Result<Option<String
}
}
// returned addresses are normalized and lowercased.
fn get_recipients<S: AsRef<str>, T: Iterator<Item = (S, S)>>(headers: T) -> HashSet<String> {
let mut recipients: HashSet<String> = Default::default();
/// Returned addresses are normalized and lowercased.
fn get_recipients(headers: &[MailHeader]) -> Vec<SingleInfo> {
get_all_addresses_from_header(headers, |header_key| {
header_key == "to" || header_key == "cc"
})
}
for (hkey, hvalue) in headers {
let hkey = hkey.as_ref().to_lowercase();
let hvalue = hvalue.as_ref();
if hkey == "to" || hkey == "cc" {
if let Ok(addrs) = mailparse::addrparse(hvalue) {
for addr in addrs.iter() {
match addr {
mailparse::MailAddr::Single(ref info) => {
recipients.insert(addr_normalize(&info.addr).to_lowercase());
}
mailparse::MailAddr::Group(ref infos) => {
for info in &infos.addrs {
recipients.insert(addr_normalize(&info.addr).to_lowercase());
}
/// Returned addresses are normalized and lowercased.
pub(crate) fn get_from(headers: &[MailHeader]) -> Vec<SingleInfo> {
get_all_addresses_from_header(headers, |header_key| header_key == "from")
}
fn get_all_addresses_from_header<F>(headers: &[MailHeader], pred: F) -> Vec<SingleInfo>
where
F: Fn(String) -> bool,
{
let mut result: Vec<SingleInfo> = Default::default();
headers
.iter()
.filter(|header| pred(header.get_key().to_lowercase()))
.filter_map(|header| mailparse::addrparse_header(header).ok())
.for_each(|addrs| {
for addr in addrs.iter() {
match addr {
mailparse::MailAddr::Single(ref info) => {
result.push(SingleInfo {
addr: addr_normalize(&info.addr).to_lowercase(),
display_name: info.display_name.clone(),
});
}
mailparse::MailAddr::Group(ref infos) => {
for info in &infos.addrs {
result.push(SingleInfo {
addr: addr_normalize(&info.addr).to_lowercase(),
display_name: info.display_name.clone(),
});
}
}
}
}
}
}
});
recipients
}
/// Check if the only addrs match, ignoring names.
fn compare_addrs(a: &mailparse::MailAddr, b: &mailparse::MailAddr) -> bool {
match a {
mailparse::MailAddr::Group(group_a) => match b {
mailparse::MailAddr::Group(group_b) => group_a
.addrs
.iter()
.zip(group_b.addrs.iter())
.all(|(a, b)| a.addr == b.addr),
_ => false,
},
mailparse::MailAddr::Single(single_a) => match b {
mailparse::MailAddr::Single(single_b) => single_a.addr == single_b.addr,
_ => false,
},
}
result
}
#[cfg(test)]
@@ -1096,12 +1121,11 @@ mod tests {
#[test]
fn test_get_recipients() {
let context = dummy_context();
let raw = include_bytes!("../test-data/message/mail_with_cc.txt");
let mimeparser = MimeMessage::from_bytes(&context.ctx, &raw[..]).unwrap();
let recipients = get_recipients(mimeparser.header.iter());
assert!(recipients.contains("abc@bcd.com"));
assert!(recipients.contains("def@def.de"));
let mail = mailparse::parse_mail(&raw[..]).unwrap();
let recipients = get_recipients(&mail.headers);
assert!(recipients.iter().any(|info| info.addr == "abc@bcd.com"));
assert!(recipients.iter().any(|info| info.addr == "def@def.de"));
assert_eq!(recipients.len(), 2);
}
@@ -1156,14 +1180,10 @@ mod tests {
let mimeparser = MimeMessage::from_bytes(&context.ctx, &raw[..]).unwrap();
let of = mimeparser
.parse_first_addr(&context.ctx, HeaderDef::From_)
.unwrap();
assert_eq!(of, mailparse::addrparse("hello@one.org").unwrap()[0]);
let of = &mimeparser.from[0];
assert_eq!(of.addr, "hello@one.org");
let of =
mimeparser.parse_first_addr(&context.ctx, HeaderDef::ChatDispositionNotificationTo);
assert!(of.is_none());
assert!(mimeparser.chat_disposition_notification_to.is_none());
}
#[test]