Skip to content

Commit

Permalink
fix: Ignore protected headers in outer message part (#6357)
Browse files Browse the repository at this point in the history
Delta Chat always adds protected headers to the inner encrypted or signed message, so if a protected
header is only present in the outer part, it should be ignored because it's probably added by the
server or somebody else. The exception is Subject because there are known cases when it's only
present in the outer message part, e.g. an encrypted unsigned Thunderbird message.

Also handle "Auto-Submitted" and "Autocrypt-Setup-Message" as protected headers on the receiver
side, this was apparently forgotten. This may fix #6357 where Saved Messages
(i.e. `ContactId::SELF`) and some contacts are unexpectedly marked as bots which can happen if
e.g. the server adds "Auto-Submitted: auto-generated" to messages for some reason. Maybe sounds
unlikely, but let's try.
  • Loading branch information
iequidoo committed Dec 29, 2024
1 parent 779635d commit 92a4af4
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 34 deletions.
3 changes: 2 additions & 1 deletion src/decrypt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,8 @@ mod tests {
let bob = TestContext::new_bob().await;
receive_imf(&bob, attachment_mime, false).await?;
let msg = bob.get_last_msg().await;
assert_eq!(msg.text, "Hello from Thunderbird!");
// Subject should be prepended because the attachment doesn't have "Chat-Version".
assert_eq!(msg.text, "Hello, Bob! – Hello from Thunderbird!");

Ok(())
}
Expand Down
61 changes: 28 additions & 33 deletions src/mimeparser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -419,20 +419,9 @@ impl MimeMessage {
gossip_headers,
)
.await?;
// Remove unsigned opportunistically protected headers from messages considered
// Autocrypt-encrypted / displayed with padlock.
// For "Subject" see <https://github.com/deltachat/deltachat-core-rust/issues/1790>.
for h in [
HeaderDef::Subject,
HeaderDef::ChatGroupId,
HeaderDef::ChatGroupName,
HeaderDef::ChatGroupNameChanged,
HeaderDef::ChatGroupAvatar,
HeaderDef::ChatGroupMemberRemoved,
HeaderDef::ChatGroupMemberAdded,
] {
headers.remove(h.get_headername());
}
// Other headers are removed by `MimeMessage::merge_headers()`.
headers.remove("subject");
}

// let known protected headers from the decrypted
Expand Down Expand Up @@ -1528,12 +1517,15 @@ impl MimeMessage {
chat_disposition_notification_to: &mut Option<SingleInfo>,
fields: &[mailparse::MailHeader<'_>],
) {
// Keep Subject so that it's displayed for signed-only messages. They are shown w/o a
// padlock anyway.
headers.retain(|k, _| !is_protected(k) || k == "subject");
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-")
{
// Don't overwrite unprotected headers, but overwrite protected ones because DKIM
// signature applies to the last headers.
if !headers.contains_key(&key) || is_protected(&key) {
if key == HeaderDef::ChatDispositionNotificationTo.get_headername() {
match addrparse_header(field) {
Ok(addrlist) => {
Expand Down Expand Up @@ -1929,23 +1921,26 @@ pub(crate) fn parse_message_id(ids: &str) -> Result<String> {

/// Returns true if the header overwrites outer header
/// when it comes from protected headers.
fn is_known(key: &str) -> bool {
matches!(
key,
"return-path"
| "date"
| "from"
| "sender"
| "reply-to"
| "to"
| "cc"
| "bcc"
| "message-id"
| "in-reply-to"
| "references"
| "subject"
| "secure-join"
)
fn is_protected(key: &str) -> bool {
key.starts_with("chat-")
|| matches!(
key,
"return-path"
| "auto-submitted"
| "autocrypt-setup-message"
| "date"
| "from"
| "sender"
| "reply-to"
| "to"
| "cc"
| "bcc"
| "message-id"
| "in-reply-to"
| "references"
| "subject"
| "secure-join"
)
}

/// Parsed MIME part.
Expand Down
18 changes: 18 additions & 0 deletions src/receive_imf/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4052,6 +4052,24 @@ async fn test_unsigned_chat_group_hdr() -> Result<()> {
Ok(())
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_ignore_protected_headers_in_outer_msg() -> Result<()> {
let mut tcm = TestContextManager::new();
let alice = &tcm.alice().await;
let bob = &tcm.bob().await;
let bob_chat_id = tcm.send_recv_accept(alice, bob, "hi").await.chat_id;
send_text_msg(bob, bob_chat_id, "hi all!".to_string()).await?;
let mut sent_msg = bob.pop_sent_msg().await;
sent_msg.payload = sent_msg.payload.replace(
"Chat-Version:",
"Auto-Submitted: auto-generated\r\nChat-Version:",
);
alice.recv_msg(&sent_msg).await;
let ab_contact = alice.add_or_lookup_contact(bob).await;
assert!(!ab_contact.is_bot());
Ok(())
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_sync_member_list_on_rejoin() -> Result<()> {
let mut tcm = TestContextManager::new();
Expand Down

0 comments on commit 92a4af4

Please sign in to comment.