fix: Ignore SecureJoin messages from blocked contacts (#8295)#8326
fix: Ignore SecureJoin messages from blocked contacts (#8295)#8326iequidoo wants to merge 2 commits into
Conversation
| .await; | ||
| let alice1_bob_id = alice1.add_or_lookup_contact_id(bob).await; | ||
| assert_eq!(get_chat_contacts(alice1, alice1_chat_id).await?.len(), 2); | ||
| assert_eq!(get_chat_contacts(alice2, alice2_chat_id).await?.len(), 1); |
There was a problem hiding this comment.
Not sure it's good that messages from self are ignored and this results in inconsistency. Bob will be added with the next message anyway.
"Ignore SecureJoin messages on an observing device if the joiner is blocked. Even if it's a "vc-contact-confirm" message, it might be issued by another device before the joiner was blocked on the observing device." sounds just like an accidental side effect of blocking messages in observe_securejoin_on_other_device. Makes sense for vc-request-with-auth, but if the other device has already added member to the group, i think it should not be ignored, just like "member added" should not be ignored.
There was a problem hiding this comment.
Agree, providing membership consistency makes sense, have fixed this.
iequidoo
force-pushed
the
iequidoo/ignore-sj-msgs-from-blocked
branch
from
6176503 to
6af7d7a
Compare
| for inviter in inviters { | ||
| if let Some(sent) = inviter.pop_sent_msg_ex(rev_order, Duration::ZERO).await { | ||
| let inviter_addr = inviter.get_primary_self_addr().await.unwrap(); | ||
| if sent.recipients.split(' ').any(|addr| addr == inviter_addr) { |
There was a problem hiding this comment.
Interestingly, without this condition test_blocked_bob_cant_create_11_chat_via_securejoin doesn't work:
========== Chats of alice2: ==========
Single#Chat#1001: Saved messages [KEY alice@example.org] Icon: 969142cb84015bc135767bc2370934a.png
--------------------------------------------------------------------------------
Msg#1003🔒: Me (Contact#Contact#Self): Secure-Join – Secure-Join √
--------------------------------------------------------------------------------
The self-chat with this weird message is created from a vc-pubkey message which isn't self-sent normally. Maybe should be fixed because if the server copies sent messages to INBOX for some reason, this will happen. But not in this PR.
iequidoo
force-pushed
the
iequidoo/ignore-sj-msgs-from-blocked
branch
from
6af7d7a to
bdb9209
Compare
| let inviter_addr = inviter.get_primary_self_addr().await.unwrap(); | ||
| if sent.recipients.split(' ').any(|addr| addr == inviter_addr) { | ||
| for observer in inviters { | ||
| observer.recv_msg_opt(&sent).await; |
There was a problem hiding this comment.
Now inviter receives the message that it just sent? This is why golden tests are changed?
This should be avoidable by comparing get_id() on the context to avoid sending to self.
There was a problem hiding this comment.
Golden tests anyway need to be changed because now inviter devices receive each other messages, but i have added a comparison of the context IDs. Indeed, we normally don't download self-sent messages because of the logic in imap::prefetch_should_download()
|
|
||
| /// Executes SecureJoin initiated by `joiner` | ||
| /// scanning `qr` generated by one of the `inviters` devices. | ||
| /// `inviters` devices must have the same primary address. |
There was a problem hiding this comment.
Why, what will happen if they don't?
If this is really needed, probably should be checked with an assertion.
There was a problem hiding this comment.
Have added an assertion. I just don't want to complicate the logic by comparing recipients against secondary addresses as well, to find out if a message should be received by a particular device
I hope this doesn't mean that if you blocked an email address now the normal contact key associated will also be blocked, some people where blocking an "unencrypted gray-avatar address" of a DC user because some weird bug/manipulation from email provider that caused some messages of that user to arrive as "unencrypted" chat despite being a chatmail address |
… each other messages
Otherwise we're testing an abnormal scenario when `Config::BccSelf` ("multi-device") is disabled.
- Ignore "vc-request-with-auth" if the sender is blocked. - Ignore SecureJoin messages on an observing device if the joiner is blocked. Even if it's a "vc-contact-confirm" message, it might be issued by another device before the joiner was blocked on the observing device. Still, to avoid membership inconsistency on devices, don't ignore "vg-member-added". - Ignore "vc-request-pubkey" if the sender is blocked by address. We don't know sender's key yet, so we should handle the sender as an address contact at this point. The sender can generate a new key and still join, but it's at least an extra work and also, in case of a group, other members will see that there's a new unknown member and be more careful.
iequidoo
force-pushed
the
iequidoo/ignore-sj-msgs-from-blocked
branch
from
bdb9209 to
87b0e3a
Compare
Only |
3 participants
"vc-contact-confirm" message, it might be issued by another device before the joiner was blocked
on the observing device. Still, to avoid membership inconsistency on devices, don't ignore
"vg-member-added".
The sender can generate a new key and still join, but it's at least an extra work and also, in case of a group, other members will see that there's a new unknown member and be more careful.
Fix #8295