feat: handle 1:1 messages with differing from addr as alias #6174
Conversation
src/receive_imf.rs
Outdated
| .cloned(); | ||
| if let Some(name) = name { | ||
| if from_id != name && name != ContactId::SELF { | ||
| mime_parser.set_override_sender_name(Some(name.get_stock_name(context).await)); |
There was a problem hiding this comment.
Not sure what to do here, should we just pretend that the non-alias sender is the sender of this message or should we actually show the alias address? Anyways, we should probably change from_id to the un-aliased contact here. Otherwise, the name is overridden again later anyways.
This comment was marked as outdated.
This comment was marked as outdated.
Septias
changed the title
Revert "add alias handling" This reverts commit 5f7ca4f. add alias handling add test improvements only do it for single chats comment out test clippy improve test remove unused function revert change to stockstr don't take mut refs fixes simplify move outwards
|
I had to move out the check that upgrades protection status. This is executed now on every message processing so maybe we should add a flag or something from the preceding code |
| .send_recv( | ||
| alice, | ||
| bob, | ||
| "[This message is not encrypted. See 'Info' for more details]", |
There was a problem hiding this comment.
The 1:1 chat isn't verified anymore, so the message should just be shown normally
There was a problem hiding this comment.
Probably it's so, but "[This message is not encrypted. See 'Info' for more details]" makes it unobvious
| .send_recv( | ||
| alice, | ||
| bob, | ||
| "[This message is not encrypted. See 'Info' for more details]", |
There was a problem hiding this comment.
Probably it's so, but "[This message is not encrypted. See 'Info' for more details]" makes it unobvious
| .filter(|chat| chat.typ == Chattype::Single), | ||
| false => None, | ||
| if let Some(chat_id) = chat_id { | ||
| let contact = Contact::get_by_id(context, from_id).await?; |
There was a problem hiding this comment.
Performance: get_by_id() loads the contact from the db, we should only do this if really necessary.
I like that this reduces the indentation level of the following block by 1.
What does this change do?
There was a problem hiding this comment.
We now unconditionally check if we want to upgrade the protection status instead of only doing it when we created a chat.
There was a problem hiding this comment.
Do we need this? If so, should probably go into an extra PR since it's unrelated?
| @@ -1805,7 +1808,13 @@ async fn lookup_chat_by_reply( | |||
|
|
|||
| // If this was a private message just to self, it was probably a private reply. | |||
| // It should not go into the group then, but into the private chat. | |||
| if is_probably_private_reply(context, to_ids, from_id, mime_parser, parent_chat.id).await? { | |||
| if parent_chat.get_type() != Chattype::Single | |||
There was a problem hiding this comment.
At least if one of the contacts is verified, the new logic mustn't be triggered. If both use the same verified key, an AEAP should happen, so this case isn't interesting. Otherwise it may be an attack, Message-ID isn't and can't be a protected header.
I even think that if both contacts are unverified, but use different keys (actually use, i.e. have mutual encryption preference and sign their messages), we shouldn't assign foreign messages to 1:1 chats.
There was a problem hiding this comment.
I'm not much into email attacks, but if it is feasible for an attacker to retrieve the msg-id of a 1:1 chat, this is indeed insecure. If both senders use the same key we could still assign them to the same chat but I think aliases are in general also allowed to have a different keys so it would not be a general solution. Maybe we should just assign to the chat if the 1:1 is not verified for now.
We want to remove AEAP I think but we can still extend this logic if that is the case.
This PR adds alias handling to 1:1 chats. If a message is received that only has self as to-address and references a MID that we know and is a 1:1 chat, it should go into that chat, even when the sender has a different email. In this case we could overwrite the sender address (from_id) or only set
OverrideSenderDisplayname. This is still open for discussion.close #2509