diff --git a/src/client/sessions.rs b/src/client/sessions.rs index cf14b75f6..820d8a4ed 100644 --- a/src/client/sessions.rs +++ b/src/client/sessions.rs @@ -493,10 +493,7 @@ impl Client { let mut failed_count = 0; for jid in jids { - if let Some(bundle) = prekey_bundles - .bundles - .get(&jid.normalize_for_prekey_bundle()) - { + if let Some(bundle) = prekey_bundles.bundles.get(jid) { match self .install_prekey_bundle_cached(jid, bundle, &mut adapter, &mut rng) .await @@ -986,21 +983,13 @@ mod tests { let mut requested_jid = Jid::lid("123456789"); requested_jid.agent = 1; - // 1. Verify direct lookup fails (This is the bug) + // The agent is inert on a LID, so it does not hide the bundle: the raw + // lookup finds it. Normalising the key first was the workaround this + // replaced, and the helper that did it is gone. assert!( - !prekey_bundles.contains_key(&requested_jid), - "Direct lookup of non-normalized JID should fail" + prekey_bundles.contains_key(&requested_jid), + "an inert agent must not hide the bundle" ); - - // 2. Verify normalized lookup succeeds (This is the fix) - // This mirrors the logic change in fetch_and_establish_sessions - let normalized_lookup = requested_jid.normalize_for_prekey_bundle(); - assert!( - prekey_bundles.contains_key(&normalized_lookup), - "Normalized lookup should succeed" - ); - - // Ensure the normalization actually produced the key we stored - assert_eq!(normalized_lookup, normalized_jid); + assert_eq!(requested_jid, normalized_jid); } } diff --git a/src/prekeys.rs b/src/prekeys.rs index fda3156fe..e16ae175c 100644 --- a/src/prekeys.rs +++ b/src/prekeys.rs @@ -185,11 +185,7 @@ impl Client { const COMPANION_IDENTITY_LOAD_CONCURRENCY: usize = 16; let companions: Vec = jids.iter().filter(|j| j.device != 0).cloned().collect(); futures::stream::iter(companions) - .map(|jid| async move { - self.load_account_identity(&jid) - .await - .map(|id| (jid.normalize_for_prekey_bundle(), id)) - }) + .map(|jid| async move { self.load_account_identity(&jid).await.map(|id| (jid, id)) }) .buffer_unordered(COMPANION_IDENTITY_LOAD_CONCURRENCY) .filter_map(|entry| async move { entry }) .collect() diff --git a/wacore/binary/benches/jid_benchmark.rs b/wacore/binary/benches/jid_benchmark.rs index 9210bc8e0..37e8b96e5 100644 --- a/wacore/binary/benches/jid_benchmark.rs +++ b/wacore/binary/benches/jid_benchmark.rs @@ -43,7 +43,7 @@ fn bench_jid_to_non_ad_string(bencher: divan::Bencher) { /// The per-recipient fan-out formatter: writes the AD form into a reused /// buffer instead of allocating a String per device. #[divan::bench] -fn bench_jid_push_ad_to(bencher: divan::Bencher) { +fn bench_jid_push_phash_form(bencher: divan::Bencher) { bencher .with_inputs(|| { let mut jid = Jid::lid("123456789012345"); @@ -51,7 +51,7 @@ fn bench_jid_push_ad_to(bencher: divan::Bencher) { (jid, String::with_capacity(64)) }) .bench_refs(|(jid, buf)| { - jid.push_ad_to(buf); + jid.push_phash_form_to(buf); // black-box the contents, not just the length: observing only // `len` lets LLVM elide the actual formatting writes. black_box(buf.as_bytes()); diff --git a/wacore/binary/fuzz/fuzz_targets/parse_jid.rs b/wacore/binary/fuzz/fuzz_targets/parse_jid.rs index 7355c6b8c..7a319996a 100644 --- a/wacore/binary/fuzz/fuzz_targets/parse_jid.rs +++ b/wacore/binary/fuzz/fuzz_targets/parse_jid.rs @@ -85,14 +85,14 @@ fuzz_target!(|data: &[u8]| { } if let Ok(jid) = text.parse::() { - let _ = jid.to_ad_string(); + let _ = jid.to_phash_form_string(); let _ = jid.device_key(); assert!(jid.display_eq(&jid.to_string())); // The non-AD form drops agent and device by definition, so it must // re-parse to the same identity with both cleared. Users holding a `.` // or `:` are excluded: rendering them back invites the parser to read - // those separators as an agent/device. `to_ad_string` is checked only + // those separators as an agent/device. `to_phash_form_string` is checked only // for panics, since it is not round-trippable at all. if !jid.user.is_empty() && !jid.user.contains(['.', ':']) { let bare = jid.to_non_ad_string(); diff --git a/wacore/binary/src/jid.rs b/wacore/binary/src/jid.rs index 6896551fa..7d7ac2ff4 100644 --- a/wacore/binary/src/jid.rs +++ b/wacore/binary/src/jid.rs @@ -497,8 +497,30 @@ pub trait JidExt { } } +/// The part of `agent` that is actually part of a JID's identity. +/// +/// `agent` is only meaningful where the server renders it. On Pn/Lid/Hosted/ +/// HostedLid the wire spells the server as a domain byte instead, so nothing +/// encodes an agent set there (`server_to_domain_type`), nothing prints it +/// (`renders_agent`), and nothing hashes it (`push_phash_form_to` writes a literal `0`, +/// matching WA Web). Two JIDs differing only there address the same device. +/// +/// Letting it into equality anyway is what made a JID decoded from the wire +/// unequal to the same JID read back from the store, which holds JIDs as text +/// (see `read_ad_jid`). Equality and `Hash` both go through here so they cannot +/// disagree, and `sort_dedup_by_device` keys on the same rule so the fan-out +/// cannot treat one device as two. +/// +/// `integrator` is deliberately NOT normalised here. It is only ever non-zero on +/// interop, but `is_same_chat_as` and `jids_share_user_identity` compare it +/// unconditionally — folding it in here would make `==` disagree with them. +#[inline] +fn identity_agent(server: Server, agent: u8) -> u8 { + if server.renders_agent() { agent } else { 0 } +} + #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] -#[derive(Debug, Clone, PartialEq, Eq, Hash, Default)] +#[derive(Debug, Clone, Default)] pub struct Jid { pub user: CompactString, pub server: Server, @@ -507,7 +529,7 @@ pub struct Jid { pub integrator: u16, } -#[derive(Debug, Clone, PartialEq, Eq, Hash, yoke::Yokeable)] +#[derive(Debug, Clone, yoke::Yokeable)] pub struct JidRef<'a> { pub user: NodeStr<'a>, pub server: Server, @@ -722,38 +744,36 @@ impl Jid { self.is_same_user_as(user) || lid.is_some_and(|l| self.is_same_user_as(l)) } - /// Normalize the JID for use in pre-key bundle storage and lookup. - /// - /// WhatsApp servers may return JIDs with varied agent fields, or we might derive them - /// with agent fields in some contexts. However, pre-key bundles are stored and looked up - /// using a normalized key where the agent is 0 for standard servers (s.whatsapp.net, lid). - pub fn normalize_for_prekey_bundle(&self) -> Self { - let mut jid = self.clone(); - if matches!(jid.server, Server::Pn | Server::Lid) { - jid.agent = 0; - } - jid - } - - pub fn to_ad_string(&self) -> String { + /// See [`Jid::push_phash_form_to`]. Not a general JID rendering — use + /// `Display`/`to_string` for that. + pub fn to_phash_form_string(&self) -> String { let mut s = String::with_capacity(self.user.len() + 20); - self.push_ad_to(&mut s); + self.push_phash_form_to(&mut s); s } - /// Append the AD-string form (`user.agent:device@server`) to `buf`, for - /// callers that batch many JIDs into one shared buffer instead of paying - /// a heap `String` per JID (see `participant_list_hash`). + /// Append the form the participant hash is computed over + /// (`user.0:device@server`) to `buf`, for callers that batch many JIDs into + /// one shared buffer instead of paying a heap `String` per JID (see + /// `participant_list_hash`). + /// + /// **This is not a general-purpose JID rendering.** The agent position is + /// the literal `0`, never `self.agent`, and that is deliberate: WA Web's + /// `formatFull` spelling hardcodes `".0"` unconditionally — there is no + /// per-server carve-out, and `WAWebWid` has no agent field to read one from. + /// The server recomputes this exact string to validate the phash, so writing + /// our agent would mean a rejected hash for any JID that carried one. + /// + /// If you want the JID as it is addressed, including the agent on the servers + /// that render it, use `Display` / [`Jid::push_to`] instead. #[inline] - pub fn push_ad_to(&self, buf: &mut String) { + pub fn push_phash_form_to(&self, buf: &mut String) { if self.user.is_empty() { buf.push_str(self.server.as_str()); return; } buf.push_str(&self.user); - buf.push('.'); - buf.push_str(itoa::Buffer::new().format(self.agent)); - buf.push(':'); + buf.push_str(".0:"); buf.push_str(itoa::Buffer::new().format(self.device)); buf.push('@'); buf.push_str(self.server.as_str()); @@ -794,6 +814,18 @@ impl Jid { self.user == other.user && self.server == other.server && self.device == other.device } + /// The `agent` as far as identity is concerned: the field itself where the + /// server renders it (`@bot`, `@interop`), `0` where it does not. + /// + /// Exposed so callers that build their own key over a JID — sorting, + /// deduplicating, indexing — can key on the same rule `==` and `Hash` use + /// instead of on the raw field, which would split one device in two or, in + /// reverse, merge two real ones. + #[inline] + pub fn identity_agent(&self) -> u8 { + identity_agent(self.server, self.agent) + } + /// Get a borrowing key for O(1) HashSet lookups by device identity. #[inline] pub fn device_key(&self) -> DeviceKey<'_> { @@ -855,14 +887,62 @@ impl<'a> JidRef<'a> { } } +impl PartialEq for Jid { + #[inline] + fn eq(&self, other: &Self) -> bool { + self.user == other.user + && self.server == other.server + && self.device == other.device + && self.integrator == other.integrator + && identity_agent(self.server, self.agent) == identity_agent(other.server, other.agent) + } +} + +impl Eq for Jid {} + +impl std::hash::Hash for Jid { + #[inline] + fn hash(&self, state: &mut H) { + self.user.hash(state); + self.server.hash(state); + self.device.hash(state); + self.integrator.hash(state); + identity_agent(self.server, self.agent).hash(state); + } +} + +impl PartialEq for JidRef<'_> { + #[inline] + fn eq(&self, other: &Self) -> bool { + self.user.as_ref() == other.user.as_ref() + && self.server == other.server + && self.device == other.device + && self.integrator == other.integrator + && identity_agent(self.server, self.agent) == identity_agent(other.server, other.agent) + } +} + +impl Eq for JidRef<'_> {} + +impl std::hash::Hash for JidRef<'_> { + #[inline] + fn hash(&self, state: &mut H) { + self.user.as_ref().hash(state); + self.server.hash(state); + self.device.hash(state); + self.integrator.hash(state); + identity_agent(self.server, self.agent).hash(state); + } +} + impl PartialEq> for Jid { #[inline] fn eq(&self, other: &JidRef<'_>) -> bool { self.user.as_str() == other.user.as_ref() && self.server == other.server - && self.agent == other.agent && self.device == other.device && self.integrator == other.integrator + && identity_agent(self.server, self.agent) == identity_agent(other.server, other.agent) } } @@ -1324,6 +1404,163 @@ mod tests { ); } + /// The phash form is recomputed and validated by the server, so + /// it has to match WA Web byte for byte. WA Web writes a literal `.0` in the + /// agent position (`formatFull`) and its Wid carries no agent at all, so ours + /// must not leak one in either — and two JIDs that compare equal must produce + /// the same string, or the phash memo (keyed by JID) can serve a hash computed + /// for a different one. + #[test] + fn phash_form_writes_the_agent_position_as_zero_like_wa_web() { + let plain = Jid { + user: "5511999998888".into(), + server: Server::Lid, + agent: 0, + device: 3, + integrator: 0, + }; + assert_eq!(plain.to_phash_form_string(), "5511999998888.0:3@lid"); + + let with_agent = Jid { + agent: 7, + ..plain.clone() + }; + assert_eq!( + with_agent.to_phash_form_string(), + "5511999998888.0:3@lid", + "the agent must not reach the hashed string" + ); + + // The pairing the phash memo relies on: equal JIDs, equal AD strings. + assert_eq!(plain, with_agent); + assert_eq!( + plain.to_phash_form_string(), + with_agent.to_phash_form_string() + ); + + // A server-only JID still degenerates to the server, and device still counts. + assert_eq!( + Jid::new("", Server::Pn).to_phash_form_string(), + "s.whatsapp.net" + ); + assert_ne!( + plain.to_phash_form_string(), + Jid { + device: 4, + ..plain.clone() + } + .to_phash_form_string() + ); + } + + /// An `agent` off an agent-rendering server is inert: nothing encodes it, + /// prints it, or hashes it. Two JIDs differing only there address the same + /// device, so equality and `Hash` must both say so — and must agree with each + /// other, or a `HashMap` gets an entry it can never look up again. + /// + /// `integrator` stays in identity: it is only non-zero on interop, but + /// `is_same_chat_as` compares it unconditionally, and `==` must not disagree. + #[test] + fn inert_agent_stays_out_of_identity() { + use std::collections::hash_map::DefaultHasher; + use std::hash::{Hash, Hasher}; + + fn hash_of(jid: &Jid) -> u64 { + let mut h = DefaultHasher::new(); + jid.hash(&mut h); + h.finish() + } + + // The four AD servers spell the server as a domain byte, so an agent set + // on one is a wire artefact, not identity. + for server in [Server::Pn, Server::Lid, Server::Hosted, Server::HostedLid] { + let clean = Jid { + user: "123456789012345".into(), + server, + agent: 0, + device: 7, + integrator: 0, + }; + let with_agent = Jid { + agent: 1, + ..clean.clone() + }; + assert_eq!( + clean, with_agent, + "{server:?}: agent must not split identity" + ); + assert_eq!( + hash_of(&clean), + hash_of(&with_agent), + "{server:?}: Hash must agree with Eq" + ); + + // integrator is NOT normalised: `is_same_chat_as` compares it always, + // and `==` must not disagree with it. + let with_integrator = Jid { + integrator: 0xBEEF, + ..clean.clone() + }; + assert_ne!( + clean, with_integrator, + "{server:?}: integrator stays in identity, matching is_same_chat_as" + ); + assert_eq!( + clean.is_same_chat_as(&with_integrator), + clean == with_integrator, + "{server:?}: == must agree with is_same_chat_as" + ); + + // The borrowed form and the cross-type comparison follow the same rule. + let borrowed = JidRef { + user: NodeStr::Borrowed("123456789012345"), + server, + agent: 1, + device: 7, + integrator: 0, + }; + assert_eq!(clean, borrowed, "{server:?}: owned == borrowed"); + assert_eq!(borrowed, clean, "{server:?}: borrowed == owned"); + } + + // Where the server DOES render the agent it is identity, and must still split. + let bot = Jid { + user: "123456789".into(), + server: Server::Interop, + agent: 4, + device: 0, + integrator: 0, + }; + let other_agent = Jid { + agent: 5, + ..bot.clone() + }; + assert_ne!( + bot, other_agent, + "interop renders the agent, so it is identity" + ); + assert_ne!( + bot, + Jid { + integrator: 9, + ..bot.clone() + }, + "interop is where integrator is real" + ); + + // Fields that are always identity keep splitting. + let pn = Jid::new("123456789012345", Server::Pn); + assert_ne!( + pn, + Jid { + device: 1, + ..pn.clone() + } + ); + assert_ne!(pn, Jid::new("123456789012346", Server::Pn)); + assert_ne!(pn, Jid::new("123456789012345", Server::Lid)); + } + #[test] fn display_eq_matches_owned_and_borrowed_jids_without_normalizing() { let canonical = "123456789.4:17@interop"; diff --git a/wacore/src/messages.rs b/wacore/src/messages.rs index 2c41483fa..4a2676dc6 100644 --- a/wacore/src/messages.rs +++ b/wacore/src/messages.rs @@ -471,7 +471,7 @@ impl MessageUtils { let mut arena = String::with_capacity(ranges.capacity() * 36); for jid in devices { let start = arena.len(); - jid.push_ad_to(&mut arena); + jid.push_phash_form_to(&mut arena); ranges.push((start, arena.len())); } ranges.sort_unstable_by(|a, b| arena[a.0..a.1].cmp(&arena[b.0..b.1])); @@ -1835,7 +1835,10 @@ mod parse_message_info_tests { } let single = vec![dev("5511999999999", 3, wacore_binary::Server::Pn)]; - assert_eq!(single[0].to_ad_string(), "5511999999999.0:3@s.whatsapp.net"); + assert_eq!( + single[0].to_phash_form_string(), + "5511999999999.0:3@s.whatsapp.net" + ); let h_single = MessageUtils::participant_list_hash(&single).unwrap(); let control = vec![dev("5511999999999", 0, wacore_binary::Server::Pn)]; @@ -1889,7 +1892,7 @@ mod parse_message_info_tests { dev("999", 0, 65535, wacore_binary::Server::Bot), ]; - let mut reference: Vec = devices.iter().map(|j| j.to_ad_string()).collect(); + let mut reference: Vec = devices.iter().map(|j| j.to_phash_form_string()).collect(); reference.sort_unstable(); let mut hasher = Sha256::new(); for jid in &reference { diff --git a/wacore/src/prekeys.rs b/wacore/src/prekeys.rs index bf9aaa2aa..7322cad12 100644 --- a/wacore/src/prekeys.rs +++ b/wacore/src/prekeys.rs @@ -217,7 +217,7 @@ impl PreKeyUtils { log::warn!("prekey response carried a with no usable jid; skipping"); continue; }; - let mut jid = named.normalize_for_prekey_bundle(); + let mut jid = named; if jid.device == 0 && matches!( jid.server, @@ -573,7 +573,11 @@ mod tests { let parsed_jid = bundles.keys().next().expect("parsed jid"); assert_eq!(parsed_jid.user, base_jid.user); assert_eq!(parsed_jid.device, base_jid.device); - assert_eq!(parsed_jid.agent, 0); + // The server's stray agent byte rides along on the parsed key rather than + // being scrubbed off it. It is inert on a LID, so it never reaches + // identity — which is what the two `contains_key` assertions above prove, + // and what the scrubbing used to be needed for. + assert_eq!(parsed_jid.agent, 1); } /// The server names a rejected device inside its own ``, which is the diff --git a/wacore/src/send/encrypt.rs b/wacore/src/send/encrypt.rs index 7e54718cb..c5feb5a09 100644 --- a/wacore/src/send/encrypt.rs +++ b/wacore/src/send/encrypt.rs @@ -646,18 +646,11 @@ pub async fn ensure_sessions_for_devices( let make_session_task = |spawn_idx: usize| { let idx = indices_needing_prekeys[spawn_idx]; - let device_jid = devices[idx].clone(); - let mut encryption_jid = encryption_override_at(&encryption_overrides, idx) + let lookup_jid = devices[idx].clone(); + let encryption_jid = encryption_override_at(&encryption_overrides, idx) .cloned() - .unwrap_or_else(|| device_jid.clone()); + .unwrap_or_else(|| lookup_jid.clone()); - // Normalize agent to 0 for LID JIDs to match how pre-key bundles are stored. - // prekeys.rs forces agent=0 for LID; we must match that here. - if encryption_jid.is_lid() { - encryption_jid.agent = 0; - } - - let lookup_jid = device_jid.normalize_for_prekey_bundle(); let bundles = prekey_bundles.clone(); let mut session_store = stores.session_store.clone_box(); let mut identity_store = stores.identity_store.clone_box(); diff --git a/wacore/src/send/tests.rs b/wacore/src/send/tests.rs index a233d061c..56b0c893a 100644 --- a/wacore/src/send/tests.rs +++ b/wacore/src/send/tests.rs @@ -1209,106 +1209,27 @@ fn test_dm_encryption_treats_own_lid_devices_as_self() { ); } -/// Test case: LID Prekey Lookup Normalization -/// -/// Verifies that when looking up pre-key bundles for LID JIDs, the lookup key -/// is normalized (agent=0) to match how the bundles are stored in the map. -/// -/// This validates the fix for "No pre-key bundle returned" when the requested JID -/// has non-standard agent/server fields but the bundle is stored under the normalized key. +/// A pre-key bundle is stored under the JID parsed out of the server's response, +/// and looked up with the JID we already hold for that device. Those two can +/// disagree on `agent` — a LID arriving as an AD-JID used to carry the domain +/// byte there — which once hid the bundle and surfaced as "No pre-key bundle +/// returned". `agent` is not part of a LID's identity, so the raw lookup finds +/// it, and the normalising helper that used to be required is gone. #[test] -fn test_lid_prekey_lookup_normalization() { - // 1. Define JIDs - // The JID we request (simulating what comes from resolve_devices or elsewhere) - // Let's pretend it has agent=1 to simulate a mismatch +fn lid_prekey_bundle_is_found_without_normalising_the_lookup_key() { let mut requested_jid = Jid::lid_device("123456789".to_string(), 0); requested_jid.agent = 1; - // The normalized JID (how it's stored in the bundle map) - let normalized_jid = Jid::lid_device("123456789".to_string(), 0); // agent=0 by default - - // 2. Setup Resolver - // Store the bundle under the NORMALIZED key (agent=0) - let resolver = MockSendContextResolver::new() - .with_bundle(normalized_jid.clone(), create_mock_bundle()) - .with_devices(vec![requested_jid.clone()]); - - // 3. Verify Mock Setup - // Ensure bundle is accessible via normalized key but NOT via requested (raw) key - // This confirms our test condition is valid (that implicit lookup would fail) - assert!( - resolver.prekey_bundles.contains_key(&normalized_jid), - "Setup: bundle should exist for normalized key" - ); - assert!( - !resolver.prekey_bundles.contains_key(&requested_jid), - "Setup: bundle should NOT exist for requested raw key" - ); + let stored_jid = Jid::lid_device("123456789".to_string(), 0); + assert_eq!(requested_jid.agent, 1, "the inert field is really set"); - // 4. Test logic mirroring `encrypt_for_devices` - let mut jid_to_encryption_jid = HashMap::new(); - // Assume direct mapping for simplicity - jid_to_encryption_jid.insert(requested_jid.clone(), requested_jid.clone()); - - // Get the bundles map (mocks `fetch_prekeys_for_identity_check`) - // The mock implementation returns the map as-is filtered by keys. - // HOWEVER, `fetch_prekeys` usually takes a list. - // In `encrypt_for_devices`, we call: - // let prekey_bundles = resolver.fetch_prekeys_for_identity_check(&[requested_jid]).await?; - - // Let's simulate what `fetch_prekeys_for_identity_check` would return. - // Our mock implementation `fetch_prekeys` logic: - // if let Some(bundle_opt) = self.prekey_bundles.get(jid) - - // Wait, if the mock follows exact HashMap lookup, `fetch_prekeys(&[requested_jid])` - // will return EMPTY because `requested_jid` is not in `prekey_bundles`. - // The REAL `fetch_prekeys` (in `client.rs` -> `prekeys.rs`) sends an IQ to the server, - // and the server response is parsed. The parsing logic (in `prekeys.rs`) normalizes the key. - // So the HashMap returned by `fetch_prekeys` will contain NORMALIZED keys. - - // So for this test to be accurate, we must simulate that `fetch_prekeys` returned a map - // where the key is NORMALIZED, even if we asked for `requested_jid`? - // Actually, `PreKeyFetchSpec` asks for JIDs. The response contains JIDs. - // If we ask for `agent=1`, does the server return `agent=1`? - // The logs showed: - // parsed: `...:82@lid` (agent=0 probably, or just not printed?) - // lookup: `...` (failed) - - // The critical part is that the `HashMap` returned by `resolver.fetch_prekeys` - // definitely contains the bundle under some key. - // If `prekeys.rs` normalizes it, it's under the normalized key. - // The `encrypt_for_devices` logic has: - // `match prekey_bundles.get(device_jid)` - // where `device_jid` is the one from the loop (requested_jid). - - // If `fetch_prekeys` returns a map with `normalized_jid`, and we lookup `requested_jid`, it fails. - // My fix was to normalize `requested_jid` before lookup. - - // So I need to construct the `prekey_bundles` map manually here to simulate the return from fetch. let mut prekey_bundles = HashMap::new(); - prekey_bundles.insert(normalized_jid.clone(), create_mock_bundle()); - - // Now test the logic: - let device_jid = &requested_jid; - - // -- Logic from fix -- - // Use centralized normalization logic - let lookup_jid = device_jid.normalize_for_prekey_bundle(); + prekey_bundles.insert(stored_jid, create_mock_bundle()); - // Fix: Use the normalized device_jid to lookup the bundle - let bundle = prekey_bundles.get(&lookup_jid); - // -------------------- - - assert!(bundle.is_some(), "Should find bundle after normalization"); - - // Verify it would have failed without normalization - let raw_lookup = prekey_bundles.get(device_jid); assert!( - raw_lookup.is_none(), - "Should NOT find bundle without normalization" + prekey_bundles.contains_key(&requested_jid), + "an inert agent must not hide the bundle" ); - - println!("✅ LID Prekey Lookup Normalization passed"); } mod group_retry { @@ -5071,7 +4992,7 @@ mod local_identity_change_on_send { }; let resolver = MockSendContextResolver::new() .with_phone_to_lid(pn.user.as_str(), lid.user.as_str()) - .with_bundle(pn.normalize_for_prekey_bundle(), signed_prekey_bundle()); + .with_bundle(pn.clone(), signed_prekey_bundle()); let plan = ensure_sessions_for_devices( &TokioTestRuntime, diff --git a/wacore/src/types/jid.rs b/wacore/src/types/jid.rs index eb847d2e5..7708e62d6 100644 --- a/wacore/src/types/jid.rs +++ b/wacore/src/types/jid.rs @@ -1,6 +1,6 @@ use crate::libsignal::protocol::{AddressBuf, DeviceId, ProtocolAddress}; use crate::libsignal::store::sender_key_name::SenderKeyName; -use wacore_binary::{DEFAULT_USER_SERVER, Jid, LEGACY_USER_SERVER}; +use wacore_binary::{DEFAULT_USER_SERVER, Jid, LEGACY_USER_SERVER, Server}; /// Real WhatsApp logs show max signal address length of 53 chars. /// 64 bytes covers all known addresses without reallocation. @@ -108,18 +108,28 @@ pub fn sort_dedup_by_user(jids: &mut Vec) { jids.dedup_by(|a, b| a.user == b.user && a.server == b.server); } -/// Sort and deduplicate by device identity (user + server + agent + device). +/// Sort and deduplicate by device identity. +/// +/// Keyed on exactly what `Jid`'s equality compares — user, server, device, +/// integrator, and `identity_agent` — so the fan-out cannot disagree with `==` +/// in either direction. Both directions are real: keying on the raw `agent` +/// would let two JIDs that are one device (an inert agent on Pn/Lid/Hosted/ +/// HostedLid, same AD-JID, same Signal address) both survive and give one +/// session two concurrent encryption jobs; dropping the agent entirely would +/// collapse two genuinely different `@bot`/`@interop` devices, which do render +/// it, and silently lose a destination. pub fn sort_dedup_by_device(jids: &mut Vec) { - jids.sort_unstable_by(|a, b| { - a.user - .cmp(&b.user) - .then_with(|| a.server.cmp(&b.server)) - .then_with(|| a.agent.cmp(&b.agent)) - .then_with(|| a.device.cmp(&b.device)) - }); - jids.dedup_by(|a, b| { - a.user == b.user && a.server == b.server && a.agent == b.agent && a.device == b.device - }); + fn key(j: &Jid) -> (&str, Server, u16, u16, u8) { + ( + &j.user, + j.server, + j.device, + j.integrator, + j.identity_agent(), + ) + } + jids.sort_unstable_by(|a, b| key(a).cmp(&key(b))); + jids.dedup_by(|a, b| key(a) == key(b)); } /// Build a `SenderKeyName` from a `&Jid` + `&ProtocolAddress` in a single @@ -339,4 +349,89 @@ mod tests { write_protocol_address_to(&second, &mut buf); assert_eq!(buf.as_str(), "123456789:33@lid.0"); } + + /// The fan-out uses this to collapse duplicate wire destinations, so it has + /// to agree with `Jid`'s equality. Two LID JIDs differing only in the agent + /// are one device — same AD-JID on the wire, same Signal address — and must + /// not both survive, or the group send builds two encryption jobs against + /// one session. + #[test] + fn device_dedup_collapses_jids_that_differ_only_in_an_inert_agent() { + let plain = Jid { + user: "123456789012345".into(), + server: Server::Lid, + agent: 0, + device: 33, + integrator: 0, + }; + let with_agent = Jid { + agent: 1, + ..plain.clone() + }; + assert_eq!(plain, with_agent, "precondition: one identity"); + assert_eq!( + plain.to_signal_address_string(), + with_agent.to_signal_address_string(), + "precondition: one Signal address" + ); + + let mut jids = vec![plain.clone(), with_agent]; + sort_dedup_by_device(&mut jids); + assert_eq!(jids, vec![plain.clone()], "one device, one entry"); + + // A different device still survives as its own entry. + let other_device = Jid { + device: 34, + ..plain.clone() + }; + let mut jids = vec![plain.clone(), other_device.clone()]; + sort_dedup_by_device(&mut jids); + assert_eq!(jids.len(), 2); + } + + /// The mirror of the case above: on the servers that DO render the agent it + /// is identity, `==` treats those JIDs as different devices, and collapsing + /// them here would silently drop a destination from the fan-out. + #[test] + fn device_dedup_keeps_agents_apart_where_the_server_renders_them() { + for server in [Server::Bot, Server::Interop] { + let a = Jid { + user: "123456789".into(), + server, + agent: 1, + device: 0, + integrator: 0, + }; + let b = Jid { + agent: 2, + ..a.clone() + }; + assert_ne!(a, b, "{server:?}: renders the agent, so these differ"); + + let mut jids = vec![a, b]; + sort_dedup_by_device(&mut jids); + assert_eq!( + jids.len(), + 2, + "{server:?}: dedup must not merge two rendered agents" + ); + } + + // `integrator` is identity too, and the key has to carry it. + let base = Jid { + user: "123456789".into(), + server: Server::Interop, + agent: 0, + device: 0, + integrator: 1, + }; + let other = Jid { + integrator: 2, + ..base.clone() + }; + assert_ne!(base, other); + let mut jids = vec![base, other]; + sort_dedup_by_device(&mut jids); + assert_eq!(jids.len(), 2, "integrator must not be dropped from the key"); + } }