From c91608e9f180e94fe28cfa38ba451fbb33a8dc9b Mon Sep 17 00:00:00 2001 From: iequidoo Date: Sat, 6 Jun 2026 13:34:49 -0300 Subject: [PATCH] fix: Don't send removal message to contact that hasn't been a chat member (#8298) I.e. don't fail `remove_contact_from_chat()` for such a contact because there may be a race condition with a remote removal of the contact done without trace, but don't send a removal message and sync message in this case and don't emit a `ChatModified` event. If a contact is already a past member, we still send a removal message to the chat, this is safe and protects from lost removal messages, so there's no need to complicate the code in this case. --- src/chat.rs | 34 +++++++++++++++++++++------------- src/chat/chat_tests.rs | 24 ++++++++++++++++++++++++ src/receive_imf.rs | 31 +++++++++++++++++++------------ 3 files changed, 64 insertions(+), 25 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index 376b87e1e..4b1a31301 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -3800,13 +3800,15 @@ pub(crate) async fn add_to_chat_contacts_table( /// Removes a contact from the chat /// by updating the `remove_timestamp`. +/// Returns whether the contact has been a chat member recently. If so, a removal message should be +/// sent. pub(crate) async fn remove_from_chat_contacts_table( context: &Context, chat_id: ChatId, contact_id: ContactId, -) -> Result<()> { +) -> Result { let now = time(); - context + let is_past_member = context .sql .execute( "UPDATE chats_contacts @@ -3814,12 +3816,15 @@ pub(crate) async fn remove_from_chat_contacts_table( WHERE chat_id=? AND contact_id=?", (now, chat_id, contact_id), ) - .await?; - Ok(()) + .await? + > 0; + Ok(is_past_member) } /// Removes a contact from the chat -/// without leaving a trace. +/// without leaving a trace in the db. +/// Returns whether the contact was removed, even if it was a past contact. If so, a removal message +/// should be sent if the removal is issued by this device. /// /// Note that if we call this function, /// and then receive a message from another device @@ -3829,17 +3834,17 @@ pub(crate) async fn remove_from_chat_contacts_table_without_trace( context: &Context, chat_id: ChatId, contact_id: ContactId, -) -> Result<()> { - context +) -> Result { + let removed = context .sql .execute( "DELETE FROM chats_contacts WHERE chat_id=? AND contact_id=?", (chat_id, contact_id), ) - .await?; - - Ok(()) + .await? + > 0; + Ok(removed) } /// Adds a contact to the chat. @@ -4159,10 +4164,13 @@ pub async fn remove_contact_from_chat( let mut sync = Nosync; - if chat.is_promoted() && chat.typ != Chattype::OutBroadcast { - remove_from_chat_contacts_table(context, chat_id, contact_id).await?; + let removed = if chat.is_promoted() && chat.typ != Chattype::OutBroadcast { + remove_from_chat_contacts_table(context, chat_id, contact_id).await? } else { - remove_from_chat_contacts_table_without_trace(context, chat_id, contact_id).await?; + remove_from_chat_contacts_table_without_trace(context, chat_id, contact_id).await? + }; + if !removed { + return Ok(()); } // We do not return an error if the contact does not exist in the database. diff --git a/src/chat/chat_tests.rs b/src/chat/chat_tests.rs index 381a6e9d0..04f639ccf 100644 --- a/src/chat/chat_tests.rs +++ b/src/chat/chat_tests.rs @@ -2800,6 +2800,30 @@ async fn test_can_send_group() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_cant_remove_nonmember() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + let bob = &tcm.bob().await; + let charlie = &tcm.charlie().await; + + let alice_broadcast_id = create_broadcast(alice, "Channel".to_string()).await?; + let qr = get_securejoin_qr(alice, Some(alice_broadcast_id)) + .await + .unwrap(); + tcm.exec_securejoin_qr(bob, alice, &qr).await; + + let alice_charlie_id = alice.add_or_lookup_contact_id(charlie).await; + remove_contact_from_chat(alice, alice_broadcast_id, alice_charlie_id).await?; + assert!(alice.pop_sent_msg_opt(Duration::ZERO).await.is_none()); + assert!(!remove_from_chat_contacts_table(alice, alice_broadcast_id, alice_charlie_id).await?); + assert!( + !remove_from_chat_contacts_table_without_trace(alice, alice_broadcast_id, alice_charlie_id) + .await? + ); + Ok(()) +} + /// Tests that in a broadcast channel, /// the recipients can't see the identity of their fellow recipients. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 9b2e89993..5713d9eed 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -3790,13 +3790,17 @@ async fn apply_out_broadcast_changes( } else if from_id == ContactId::SELF && let Some(removed_id) = removed_id { - chat::remove_from_chat_contacts_table_without_trace(context, chat.id, removed_id) - .await?; - - better_msg.get_or_insert( - stock_str::msg_del_member_local(context, removed_id, ContactId::SELF).await, - ); - added_removed_id = Some(removed_id); + if chat::remove_from_chat_contacts_table_without_trace(context, chat.id, removed_id) + .await? + { + better_msg.get_or_insert( + stock_str::msg_del_member_local(context, removed_id, ContactId::SELF).await, + ); + added_removed_id = Some(removed_id); + } else { + info!(context, "No-op broadcast member removal message (TRASH)."); + better_msg = Some("".to_string()); + } } } @@ -3870,17 +3874,20 @@ async fn apply_in_broadcast_changes( } chat::delete_broadcast_secret(context, chat.id).await?; - if from_id == ContactId::SELF { + let removed = + chat::remove_from_chat_contacts_table_without_trace(context, chat.id, ContactId::SELF) + .await?; + if !removed { + info!(context, "No-op broadcast SELF-removal message (TRASH)."); + better_msg = Some("".to_string()); + } else if from_id == ContactId::SELF { better_msg.get_or_insert(stock_str::msg_you_left_broadcast(context)); } else { better_msg.get_or_insert( stock_str::msg_del_member_local(context, ContactId::SELF, from_id).await, ); } - - chat::remove_from_chat_contacts_table_without_trace(context, chat.id, ContactId::SELF) - .await?; - send_event_chat_modified = true; + send_event_chat_modified |= removed; } else if !chat.is_self_in_chat(context).await? { chat::add_to_chat_contacts_table( context,