From 615c80bef43c2c53e9476af4c79e1c69c50690be Mon Sep 17 00:00:00 2001 From: iequidoo Date: Tue, 5 Aug 2025 13:20:00 -0300 Subject: [PATCH] feat: Contact::lookup_id_by_addr_ex: Prefer returning accepted contacts We don't want to prefer returning verified contacts because e.g. if a bot was reinstalled and its key changed, it may not be verified, and we don't want to bring the user to the old chat if they click on the bot email address. But trying to return accepted contacts increases security and doesn't break the described scenario. --- src/contact.rs | 20 +++++++++++++++++--- src/contact/contact_tests.rs | 21 +++++++++++++++++---- src/receive_imf.rs | 25 ++++++++++++++++++++----- src/sql/migrations/migrations_tests.rs | 12 ++++++++---- 4 files changed, 62 insertions(+), 16 deletions(-) diff --git a/src/contact.rs b/src/contact.rs index 46fc7541e..7c902555e 100644 --- a/src/contact.rs +++ b/src/contact.rs @@ -807,14 +807,28 @@ impl Contact { .query_get_value( "SELECT id FROM contacts WHERE addr=?1 COLLATE NOCASE - AND id>?2 AND origin>=?3 AND (? OR blocked=?) - ORDER BY last_seen DESC, fingerprint DESC LIMIT 1", + AND id>?2 AND origin>=?3 AND (? OR blocked=?) + ORDER BY + ( + SELECT COUNT(*) FROM chats c + INNER JOIN chats_contacts cc + ON c.id=cc.chat_id + WHERE c.type=? + AND c.id>? + AND c.blocked=? + AND cc.contact_id=contacts.id + ) DESC, + last_seen DESC, fingerprint DESC + LIMIT 1", ( &addr_normalized, ContactId::LAST_SPECIAL, min_origin as u32, blocked.is_none(), - blocked.unwrap_or_default(), + blocked.unwrap_or(Blocked::Not), + Chattype::Single, + constants::DC_CHAT_ID_LAST_SPECIAL, + blocked.unwrap_or(Blocked::Not), ), ) .await?; diff --git a/src/contact/contact_tests.rs b/src/contact/contact_tests.rs index a4cc2fbfa..14ccc4d93 100644 --- a/src/contact/contact_tests.rs +++ b/src/contact/contact_tests.rs @@ -1035,8 +1035,7 @@ async fn test_was_seen_recently_event() -> Result<()> { Ok(()) } -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn test_lookup_id_by_addr_recent() -> Result<()> { +async fn test_lookup_id_by_addr_recent_ex(accept_unencrypted_chat: bool) -> Result<()> { let mut tcm = TestContextManager::new(); let bob = &tcm.bob().await; @@ -1053,10 +1052,12 @@ Date: Thu, 24 Nov 2022 $TIME +0100 Hi"# .to_string(); - for (time, is_key_contact) in [("20:05:57", true), ("20:05:58", false)] { + for (time, is_key_contact) in [("20:05:57", true), ("20:05:58", !accept_unencrypted_chat)] { let raw = raw.replace("$TIME", time); let received_msg = receive_imf(bob, raw.as_bytes(), false).await?.unwrap(); - received_msg.chat_id.accept(bob).await?; + if accept_unencrypted_chat { + received_msg.chat_id.accept(bob).await?; + } let contact_id = Contact::lookup_id_by_addr(bob, "alice@example.org", Origin::Unknown) .await? .unwrap(); @@ -1066,6 +1067,18 @@ Hi"# Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_lookup_id_by_addr_recent() -> Result<()> { + let accept_unencrypted_chat = true; + test_lookup_id_by_addr_recent_ex(accept_unencrypted_chat).await +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_lookup_id_by_addr_recent_accepted() -> Result<()> { + let accept_unencrypted_chat = false; + test_lookup_id_by_addr_recent_ex(accept_unencrypted_chat).await +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_verified_by_none() -> Result<()> { let mut tcm = TestContextManager::new(); diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 0c31cac54..21dcf1c05 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -18,7 +18,7 @@ use crate::chat::{ self, Chat, ChatId, ChatIdBlocked, ProtectionStatus, remove_from_chat_contacts_table, }; use crate::config::Config; -use crate::constants::{Blocked, Chattype, DC_CHAT_ID_TRASH, EDITED_PREFIX, ShowEmails}; +use crate::constants::{self, Blocked, Chattype, DC_CHAT_ID_TRASH, EDITED_PREFIX, ShowEmails}; use crate::contact::{Contact, ContactId, Origin, mark_contact_id_as_verified}; use crate::context::Context; use crate::debug_logging::maybe_set_logging_xdc_inner; @@ -3793,7 +3793,7 @@ async fn add_or_lookup_key_contacts( /// Looks up a key-contact by email address. /// /// If provided, `chat_id` must be an encrypted chat ID that has key-contacts inside. -/// Otherwise the function searches in all contacts, returning the recently seen one. +/// Otherwise the function searches in all contacts, preferring accepted and most recently seen ones. async fn lookup_key_contact_by_address( context: &Context, addr: &str, @@ -3836,11 +3836,26 @@ async fn lookup_key_contact_by_address( .sql .query_row_optional( "SELECT id FROM contacts - WHERE contacts.addr=?1 + WHERE addr=? AND fingerprint<>'' - ORDER BY last_seen DESC, id DESC + ORDER BY + ( + SELECT COUNT(*) FROM chats c + INNER JOIN chats_contacts cc + ON c.id=cc.chat_id + WHERE c.type=? + AND c.id>? + AND c.blocked=? + AND cc.contact_id=contacts.id + ) DESC, + last_seen DESC, id DESC ", - (addr,), + ( + addr, + Chattype::Single, + constants::DC_CHAT_ID_LAST_SPECIAL, + Blocked::Not, + ), |row| { let contact_id: ContactId = row.get(0)?; Ok(contact_id) diff --git a/src/sql/migrations/migrations_tests.rs b/src/sql/migrations/migrations_tests.rs index 7f0d0ae59..d3ed58898 100644 --- a/src/sql/migrations/migrations_tests.rs +++ b/src/sql/migrations/migrations_tests.rs @@ -116,11 +116,15 @@ async fn test_key_contacts_migration_email2() -> Result<()> { )?)).await?; t.sql.run_migrations(&t).await?; - let pgp_bob_id = Contact::lookup_id_by_addr(&t, "bob@example.net", Origin::Hidden) - .await? - .unwrap(); - let pgp_bob = Contact::get_by_id(&t, pgp_bob_id).await?; + // Hidden key-contact can't be looked up. + assert!( + Contact::get_all(&t, 0, Some("bob@example.net")) + .await? + .is_empty() + ); + let pgp_bob = Contact::get_by_id(&t, ContactId::new(11)).await?; assert_eq!(pgp_bob.is_key_contact(), true); + assert_eq!(pgp_bob.origin, Origin::Hidden); let email_bob_id = *Contact::get_all(&t, constants::DC_GCL_ADDRESS, Some("bob@example.net")) .await?