diff --git a/src/client/app_state.rs b/src/client/app_state.rs index d792a1849..8d9ee3b4e 100644 --- a/src/client/app_state.rs +++ b/src/client/app_state.rs @@ -3055,8 +3055,15 @@ impl Client { return; } - if crate::features::call_log::dispatch_call_log_mutation(&self.core.event_bus, m, full_sync) - { + // A call's direction is its creator compared against this account; the + // predicate is only consulted once the mutation is known to be a call + // log, so the other mutation kinds do not pay for the snapshot. + if crate::features::call_log::dispatch_call_log_mutation( + &self.core.event_bus, + m, + full_sync, + |jid| self.is_own_jid(jid), + ) { return; } diff --git a/src/features/call_log.rs b/src/features/call_log.rs index fbc3d6f92..21f76048b 100644 --- a/src/features/call_log.rs +++ b/src/features/call_log.rs @@ -6,23 +6,39 @@ //! //! # The index carries what the record can leave out //! -//! The index is `["call_log", callCreatorJid, callId, fromMe]`, not the bare +//! The index is `["call_log", callCreatorJid, callId, direction]`, not the bare //! literal `schemas::CALL_LOG.index_parts` declares. WA Web builds it as //! `JSON.stringify([action, ...indexArgs])` (`WAWebSyncdActionUtils.buildIndex`) -//! and `getCallLogMutation` passes `indexArgs: [d, p, m]` — the call creator, the -//! call id, and whether this account placed the call. +//! and `getCallLogMutation` passes `indexArgs: [d, p, m]` — the call creator, +//! the call id, and a direction flag whose meaning is not agreed on (below). //! //! That matters because the *record*'s `callCreatorJid` is optional and WA Web //! leaves it unset for calls it did not receive one for, while the index's is //! filled in either way — `d == null && (d = fromMe ? me : peerJid)`. A consumer //! given only the record cannot always say who the call was with. //! -//! # `is_incoming` does not mean what it says +//! # The direction fields disagree with each other, so neither is read //! -//! WA Web writes the record with `isIncoming: n.fromMe`, so the field carries -//! "this account placed the call" — the opposite of its name. The index's -//! `fromMe` is the same value under an honest name, which is why -//! [`CallLogSync::from_me`] comes from there and is the field to trust. +//! Two fields claim to carry the call's direction, and which one is honest +//! depends on who wrote the mutation: +//! +//! - WA Web's own writer sets both from its local `fromMe`: `indexArgs: [d, p, +//! m]` with `m = n.fromMe ? "1" : "0"`, and `isIncoming: n.fromMe`. Its +//! `isIncoming` is therefore inverted against its own name. +//! - Mutations authored by the phone carry them the other way round, literally +//! as `isIncoming` — a call the account placed arrives as `0`/`false`. +//! +//! Since app state fans a companion's mutations out to every other device, both +//! flavors reach this client, and no fixed reading of either field is right for +//! both. +//! +//! WA Web's reader sidesteps the whole thing, and so does this module: +//! `generateCallLogFromCallSyncRecord` destructures the record without ever +//! touching `isIncoming`, and takes direction from +//! `getCallLogTargetDetails`, which is `fromMe: isMeAccount(callCreatorWid)` — +//! the call creator compared against this account. [`CallLogSync::from_me`] is +//! derived the same way. That is also why the creator is worth having in the +//! index: it is what the direction is computed from. use crate::appstate_sync::Mutation; use wacore::appstate::schemas; @@ -32,10 +48,15 @@ use waproto::whatsapp as wa; /// Dispatch inbound call-log mutations synced from the primary device. /// Returns `true` if handled, `false` if the mutation is not a call log. +/// +/// `is_own_jid` decides the call's direction and is consulted only once the +/// mutation is known to be a call log, so the app-state path pays nothing for +/// it on the mutations it is not. pub(crate) fn dispatch_call_log_mutation( event_bus: &wacore::types::events::CoreEventBus, m: &Mutation, full_sync: bool, + is_own_jid: impl FnOnce(&Jid) -> bool, ) -> bool { if m.operation != wa::syncd_mutation::SyncdOperation::Set || m.index.first().map(String::as_str) != Some(schemas::CALL_LOG.name) @@ -53,16 +74,9 @@ pub(crate) fn dispatch_call_log_mutation( log::warn!("Skipping call_log mutation: missing call id in index"); return true; }; - // WA Web writes "1"/"0" (`m = n.fromMe ? "1" : "0"`). Anything else is a - // shape we do not know how to read, and guessing would mislabel the call. - let from_me = match m.index.get(3).map(String::as_str) { - Some("1") => true, - Some("0") => false, - other => { - log::warn!("Skipping call_log mutation: unreadable fromMe {other:?} in index"); - return true; - } - }; + // Direction comes from the creator; see the module docs for why not from + // either field that claims to carry it. + let from_me = is_own_jid(&call_creator_jid); // The mutation's own time, which is metadata rather than the call's: WA Web // measures it against the pairing timestamp to drop records that predate the @@ -141,11 +155,31 @@ mod tests { } } + /// This account, as either identity: a creator matching one of these is a + /// call we placed. Stands in for `Client::is_own_jid`, which compares a JID + /// against the device's own PN and LID. + const OWN_PN: &str = "5511888880000@s.whatsapp.net"; + const OWN_LID: &str = "111122223333444@lid"; + /// Whoever we were talking to. Not us, under either identity. + const PEER: &str = "5511999990000@s.whatsapp.net"; + + fn is_own(jid: &Jid) -> bool { + matches!(jid.user.as_str(), "5511888880000" | "111122223333444") + } + fn dispatch(mutation: &Mutation, full_sync: bool) -> (bool, Vec>) { + dispatch_as(mutation, full_sync, is_own) + } + + fn dispatch_as( + mutation: &Mutation, + full_sync: bool, + is_own_jid: impl FnOnce(&Jid) -> bool, + ) -> (bool, Vec>) { let bus = CoreEventBus::new(); let recorder = Arc::new(Recorder::default()); bus.subscribe_handler(recorder.clone()).detach(); - let handled = dispatch_call_log_mutation(&bus, mutation, full_sync); + let handled = dispatch_call_log_mutation(&bus, mutation, full_sync, is_own_jid); let events = recorder.events.lock().unwrap().clone(); (handled, events) } @@ -165,8 +199,12 @@ mod tests { } } + /// A call this account placed: the creator is us, which is what the + /// direction is read from. The fourth part is the writers' disputed field + /// and is deliberately set to the value that would give the wrong answer if + /// anything still read it as `fromMe`. fn full_index() -> [&'static str; 4] { - ["call_log", "5511999990000@s.whatsapp.net", "call-42", "1"] + ["call_log", OWN_PN, "call-42", "0"] } #[test] @@ -206,46 +244,82 @@ mod tests { let Event::CallLogSync(update) = events[0].as_ref() else { panic!("expected CallLogSync event"); }; - assert_eq!( - update.call_creator_jid.to_string(), - "5511999990000@s.whatsapp.net" - ); + assert_eq!(update.call_creator_jid.to_string(), OWN_PN); assert_eq!(update.call_id, "call-42"); - assert!( - update.from_me, - "the index says this account placed the call" - ); assert_eq!(update.timestamp.timestamp_millis(), 1_700_000_000_000); } - /// WA Web writes the record's `isIncoming` from its local `fromMe`, so the - /// two disagree by name and agree by value. A consumer trusting the field - /// name would file every call backwards; `from_me` is the honest one. Both - /// directions, so neither a constant nor the record can stand in for it. + /// The regression: a call this account placed reads as `from_me`. + /// + /// Both of the fields that claim to carry direction are set the way the + /// phone sends them for an outbound call — index `"0"`, record + /// `is_incoming: false` — because that combination is what made this read + /// backwards. Direction comes from the creator, so it survives them. + #[test] + fn a_call_this_account_placed_is_from_me() { + for creator in [OWN_PN, OWN_LID] { + let record = wa::CallLogRecord { + is_incoming: Some(false), + ..Default::default() + }; + let index = ["call_log", creator, "call-7", "0"]; + let (_, events) = dispatch(&call_log_mutation(&index, Some(record)), false); + + let Event::CallLogSync(update) = events[0].as_ref() else { + panic!("expected CallLogSync event"); + }; + assert!( + update.from_me, + "{creator} is this account, so it placed the call" + ); + } + } + + /// The other direction, and the failure case for the fix: a call somebody + /// else placed must not become ours just because the direction fields say + /// so. WA Web's writer sets both to `fromMe`, so an inbound call it logged + /// carries index `"0"` and `is_incoming: false` — identical to the outbound + /// fixture above, and told apart only by the creator. #[test] - fn from_me_comes_from_the_index_not_the_record() { - for (index_from_me, record_is_incoming, expected) in - [("0", Some(true), false), ("1", Some(false), true)] - { + fn a_call_the_peer_placed_is_not_from_me() { + for (index_part, record_is_incoming) in [("0", Some(false)), ("1", Some(true))] { let record = wa::CallLogRecord { is_incoming: record_is_incoming, ..Default::default() }; - let index = [ - "call_log", - "5511999990000@s.whatsapp.net", - "call-7", - index_from_me, - ]; + let index = ["call_log", PEER, "call-7", index_part]; let (_, events) = dispatch(&call_log_mutation(&index, Some(record)), false); let Event::CallLogSync(update) = events[0].as_ref() else { panic!("expected CallLogSync event"); }; - assert_eq!( - update.from_me, expected, - "the index is authoritative, and it says fromMe={index_from_me}" + assert!( + !update.from_me, + "the creator is the peer, whatever index[3]={index_part} claims" + ); + } + } + + /// The fourth index part is no longer read, so a value we cannot parse is + /// no longer a reason to drop the call: everything the event carries comes + /// from parts we did read. + #[test] + fn an_unreadable_direction_part_no_longer_drops_the_call() { + for index in [ + ["call_log", OWN_PN, "call-42", "yes"], + ["call_log", OWN_PN, "call-42", ""], + ] { + let (handled, events) = dispatch( + &call_log_mutation(&index, Some(wa::CallLogRecord::default())), + false, ); + + assert!(handled); + assert_eq!(events.len(), 1, "{index:?} is still a usable call log"); + let Event::CallLogSync(update) = events[0].as_ref() else { + panic!("expected CallLogSync event"); + }; + assert!(update.from_me); } } @@ -259,14 +333,13 @@ mod tests { /// An index we cannot read is still ours — handing it on would only offer it /// to dispatchers keyed on other indexes — but it cannot be turned into an - /// event a consumer could trust. + /// event a consumer could trust. Only the parts the event is built from: + /// without a creator there is no direction either. #[test] fn an_unreadable_index_is_claimed_without_event() { for index in [ &["call_log"][..], - &["call_log", "5511999990000@s.whatsapp.net"][..], - &["call_log", "5511999990000@s.whatsapp.net", "call-42"][..], - &["call_log", "5511999990000@s.whatsapp.net", "call-42", "yes"][..], + &["call_log", PEER][..], &["call_log", "not a jid", "call-42", "1"][..], ] { let (handled, events) = dispatch( diff --git a/src/features/contacts.rs b/src/features/contacts.rs index 617e84712..31e03b3e8 100644 --- a/src/features/contacts.rs +++ b/src/features/contacts.rs @@ -231,14 +231,7 @@ impl<'a> Contacts<'a> { } // Skip own JID: server never responds when tctoken is sent for self - let is_own_jid = { - let snap = self.client.persistence_manager.get_device_snapshot(); - snap.pn.as_ref().is_some_and(|pn| pn.is_same_user_as(jid)) - || snap - .lid - .as_ref() - .is_some_and(|lid| lid.is_same_user_as(jid)) - }; + let is_own_jid = self.client.is_own_jid(jid); if !jid.is_group() && !jid.is_newsletter() && !jid.is_bot() diff --git a/src/send/tctoken_lifecycle.rs b/src/send/tctoken_lifecycle.rs index cffc2e107..a9ba68f1d 100644 --- a/src/send/tctoken_lifecycle.rs +++ b/src/send/tctoken_lifecycle.rs @@ -1,19 +1,27 @@ use super::*; +/// Whether `jid` addresses this account, given its two identities. +/// +/// Both, because a JID reaches us as a phone number or a LID depending on the +/// thread's migration state, and only one of the two ever matches. +/// +/// Addressing mode is part of the answer, not noise: WA Web's `isMeAccount` +/// keys on `isSameAccountAndAddressingMode`, so a peer LID whose digits happen +/// to spell our phone number is not us. `is_same_user_as` compares only the +/// user and would say it is — the same looseness `is_same_chat_as` exists to +/// guard, and the same rule `is_self_dm_recipient` already follows. +pub(crate) fn is_own_identity(own_pn: Option<&Jid>, own_lid: Option<&Jid>, jid: &Jid) -> bool { + own_pn.is_some_and(|pn| pn.is_same_chat_as(jid)) + || own_lid.is_some_and(|lid| lid.is_same_chat_as(jid)) +} + impl Client { /// Whether `jid` is our own account (PN or LID). The privacy-token paths /// never attach to or issue for ourselves; a single source of truth keeps /// the message and call paths from drifting apart. - fn is_own_jid(&self, jid: &Jid) -> bool { + pub(crate) fn is_own_jid(&self, jid: &Jid) -> bool { let snapshot = self.persistence_manager.get_device_snapshot(); - snapshot - .pn - .as_ref() - .is_some_and(|pn| pn.is_same_user_as(jid)) - || snapshot - .lid - .as_ref() - .is_some_and(|lid| lid.is_same_user_as(jid)) + is_own_identity(snapshot.pn.as_ref(), snapshot.lid.as_ref(), jid) } /// Look up and include a privacy token in outgoing 1:1 message stanza nodes. @@ -412,10 +420,42 @@ impl Client { #[cfg(test)] mod tests { + use super::is_own_identity; use crate::test_utils::create_test_client; use wacore::store::traits::TcTokenEntry; use wacore_binary::{Jid, Server}; + /// Self-detection keys on the addressing mode, not just the digits. A LID + /// is an assigned number in its own namespace, so one can spell a phone + /// number that belongs to somebody else — and a user-only comparison would + /// hand that peer our own identity: no privacy token where one is due, and + /// a call they placed filed as one we placed. + #[test] + fn a_peer_addressed_in_the_other_namespace_is_not_us() { + let own_pn = Jid::new("5511888880000", Server::Pn); + let own_lid = Jid::new("111122223333444", Server::Lid); + + // Each identity matches itself, device suffix and all. + assert!(is_own_identity(Some(&own_pn), Some(&own_lid), &own_pn)); + assert!(is_own_identity(Some(&own_pn), Some(&own_lid), &own_lid)); + assert!(is_own_identity( + Some(&own_pn), + Some(&own_lid), + &own_pn.with_device(12) + )); + + // Same digits, other namespace: a different account, both ways round. + let peer_lid = Jid::new("5511888880000", Server::Lid); + let peer_pn = Jid::new("111122223333444", Server::Pn); + assert!(!is_own_identity(Some(&own_pn), Some(&own_lid), &peer_lid)); + assert!(!is_own_identity(Some(&own_pn), Some(&own_lid), &peer_pn)); + + // And with no LID known, our PN's digits in the LID namespace are still + // not us — the case `lid_recipient_without_own_lid_is_not_self_dm` + // pins for self-DM detection, which follows the same rule. + assert!(!is_own_identity(Some(&own_pn), None, &peer_lid)); + } + #[tokio::test] async fn record_sender_timestamp_creates_byteless_placeholder() { let client = create_test_client().await; diff --git a/wacore/src/types/events.rs b/wacore/src/types/events.rs index d993adfb2..f5f2c55a9 100755 --- a/wacore/src/types/events.rs +++ b/wacore/src/types/events.rs @@ -2415,12 +2415,14 @@ pub struct CallLogSync { /// The call's identifier, from the mutation's index (the same value /// `record.call_id` carries when the record carries one). pub call_id: String, - /// Whether *this account* placed the call. + /// Whether *this account* placed the call, from + /// [`call_creator_jid`](Self::call_creator_jid) compared against this + /// account. /// - /// Read this rather than `record.is_incoming`, which despite its name holds - /// the same thing rather than its opposite: WA Web writes the record with - /// `isIncoming: fromMe`, so a consumer taking the field at its word files - /// every call backwards. + /// Read this rather than `record.is_incoming`, which is not reliable in + /// either direction: it means the opposite of its name in mutations WA Web + /// wrote and exactly its name in ones the phone wrote, so a consumer taking + /// it at its word files some calls backwards. pub from_me: bool, /// When the mutation was written, not when the call happened — the call's /// own time is `record.start_time`.