From 79ce0b8ce9d39894c0cd80f3f840b8414a9a5064 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= Date: Tue, 28 Apr 2026 14:11:49 -0300 Subject: [PATCH 1/4] fix(send): close SKDM flow gaps for forward secrecy and cache hygiene MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five WA Web alignment issues found while auditing the post-PR-#603 SKDM flow. Each had an empirical regression test before being fixed. R1 — Forward secrecy on participant remove `handle_group_notification` Remove and `Groups::remove_participants` only patched `group_cache`. WA Web's `removeParticipantInfo` (`GroupParticipantHelpers.js`) sets `rotateKey=true` whenever any removed participant had `has_key=true`, then `GroupSkmsgJob.js:238` calls `deleteGroupSenderKeyInfo` to drop the bot's own sender key before the next send. Fix: new `Client::rotate_sender_key_on_participant_remove` helper wired into both paths. Reads `sender_key_devices`, checks if any removed user had `has_key=true`, and if so deletes the bot's own sender key + clears the group's tracker so the next send takes `force_skdm=true` (`!key_exists`) and redistributes to remaining participants. R2 — `patch_device_add` / `patch_device_remove` `invalidate_all()` The previous code dropped every group's sender-key cache when any user's device set changed. WA Web mutates only the affected groups' `senderKey` Map (`UpdateParticipantApi.js`). Fix: drop the global invalidation. Unknown new devices are picked up automatically because `device_has_key()` returns `None` for entries not yet in the map → falls into `needs_skdm` on the next send. R3 — Phash mismatch left persisted state stale `spawn_phash_validation` only invalidated the in-memory cache; reload from DB surfaced the same stale rows. Group sends also skipped the group_cache invalidation that status sends performed. Fix: clear `sender_key_devices` for the chat on mismatch and invalidate `group_cache` for groups too. R4 — Orphan `sender_key_devices` rows after `patch_device_remove` The removed device's row was never deleted, accumulating dead state. Fix: new `delete_sender_key_device_rows(device_jids)` backend method (Sqlite + InMemory + Persistence wrapper). `patch_device_remove` builds candidate JIDs under both LID and PN aliases via `resolve_lookup_keys` and deletes them. R5 — No periodic sender-key rotation WA Web has `EXPIRY_REASON.PERIODIC_ROTATION` in the WAM enum; captured-js doesn't surface the threshold. We never rotated. Fix: when a group send finds the existing sender key with `chain_key.iteration() ≥ 1000`, delete it and clear the tracker so the next send regenerates and full-distributes. Conservative default; can be adjusted if a WA Web reference surfaces. --- src/appstate_sync.rs | 3 + src/client/device_registry.rs | 308 ++++++++++++++++---- src/client/sender_keys.rs | 63 ++++ src/features/groups.rs | 3 + src/handlers/notification.rs | 9 +- src/send.rs | 61 +++- src/sender_key_device_cache.rs | 6 - src/store/persistence_manager.rs | 9 + storages/sqlite-storage/src/sqlite_store.rs | 24 ++ wacore/src/store/in_memory.rs | 12 + wacore/src/store/persistence.rs | 9 + wacore/src/store/traits.rs | 4 + 12 files changed, 445 insertions(+), 66 deletions(-) diff --git a/src/appstate_sync.rs b/src/appstate_sync.rs index 31da1a9a4..9d7b59f6d 100644 --- a/src/appstate_sync.rs +++ b/src/appstate_sync.rs @@ -164,6 +164,9 @@ mod tests { async fn clear_all_sender_key_devices(&self) -> StoreResult<()> { Ok(()) } + async fn delete_sender_key_device_rows(&self, _: &[&str]) -> StoreResult<()> { + Ok(()) + } async fn get_lid_mapping(&self, _: &str) -> StoreResult> { Ok(None) } diff --git a/src/client/device_registry.rs b/src/client/device_registry.rs index 22f07d00f..bff08746e 100644 --- a/src/client/device_registry.rs +++ b/src/client/device_registry.rs @@ -232,8 +232,6 @@ impl Client { return; }; - let devices_before: Vec = record.devices.iter().map(|d| d.device_id).collect(); - let signed_bytes = key_index_info.and_then(|ki| ki.signed_bytes.as_deref()); if let Some(bytes) = signed_bytes { @@ -277,15 +275,9 @@ impl Client { self.append_device_if_new(&mut record, device_id, device.key_index); } - // Detect new devices: any device_id present now that wasn't before. - // Invalidate sender key device cache so SKDM is sent on next group message. - let has_new_device = record - .devices - .iter() - .any(|d| !devices_before.contains(&d.device_id)); - if has_new_device { - self.sender_key_device_cache.invalidate_all(); - } + // New devices are picked up automatically by `resolve_skdm_targets`: + // unknown device → `device_has_key()` returns `None` → falls into + // `needs_skdm`. No global cache invalidation needed. if let Err(e) = self.update_device_list(record).await { warn!("patch_device_add: failed to persist: {e}"); @@ -372,7 +364,10 @@ impl Client { self.delete_sessions_for_devices(user, &[device_id as u16]) .await; } - self.sender_key_device_cache.invalidate_all(); + // WA Web's `updateGroupParticipantsInTransaction` deletes the + // device JID from each affected group's senderKey Map. + self.delete_sender_key_rows_for_device(user, device_id) + .await; if let Err(e) = self.update_device_list(record).await { warn!("patch_device_remove: failed to persist: {e}"); } @@ -380,6 +375,32 @@ impl Client { } } + /// Delete `sender_key_devices` rows whose `device_jid` matches the given + /// (user, device_id) under either LID or PN addressing. Both alias keys + /// for the user are tried via `resolve_lookup_keys`. Touches DB then + /// invalidates the in-memory cache for groups that may have indexed the + /// removed JID. + async fn delete_sender_key_rows_for_device(&self, user: &str, device_id: u32) { + let lookup = self.resolve_lookup_keys(user).await; + let servers = [wacore_binary::Server::Lid, wacore_binary::Server::Pn]; + let mut candidates: Vec = Vec::with_capacity(4); + for server in servers { + for key in lookup.all_keys() { + let mut jid = Jid::new(key, server); + jid.device = device_id as u16; + candidates.push(jid.to_string()); + } + } + let refs: Vec<&str> = candidates.iter().map(|s| s.as_str()).collect(); + if let Err(e) = self + .persistence_manager + .delete_sender_key_device_rows(&refs) + .await + { + warn!("delete_sender_key_rows_for_device: {e}"); + } + } + /// Update key_index for a device in the registry. pub(crate) async fn patch_device_update( &self, @@ -1101,18 +1122,17 @@ mod tests { assert_eq!(updated.devices[0].device_id, 0); } - // ── Sender key device cache invalidation tests ────────────────────── + // ── Sender key device cache: post-fix behavior ────────────────────── + /// `device_has_key` returns `None` for unknown devices, so an added device + /// naturally falls into `needs_skdm` on the next send without any cache wipe. #[tokio::test] - async fn test_patch_device_add_invalidates_sender_key_cache() { + async fn test_patch_device_add_keeps_cache_warm_new_device_seen_as_unknown() { use crate::sender_key_device_cache::SenderKeyDeviceMap; let client = create_test_client().await; - - // Pre-populate device registry with device 0 only setup_device_record(&client, "15551234567", &[0]).await; - // Warm the sender key device cache for a group let group = "120363000000000001@g.us"; let map = SenderKeyDeviceMap::from_db_rows(&[("15551234567:0@s.whatsapp.net".into(), true)]); @@ -1121,23 +1141,17 @@ mod tests { .get_or_init(group, async { std::sync::Arc::new(map) }) .await; - // Add device 3 — should invalidate sender key cache let elem = make_device_element(3, Some(5)); client.patch_device_add("15551234567", &elem, None).await; - // Sender key cache should be cleared (get_or_init would need to re-fetch) - // We verify by checking that the cached map doesn't contain the old entry - // anymore through the cache's internal state. Since invalidate_all() was - // called, re-init will produce a fresh map. - let fresh_map = SenderKeyDeviceMap::from_db_rows(&[]); - let result = client + let warm = client .sender_key_device_cache - .get_or_init(group, async { std::sync::Arc::new(fresh_map) }) + .get_or_init(group, async { + panic!("cache should still be warm — no global invalidation") + }) .await; - assert!( - result.is_empty(), - "sender key cache should have been invalidated and re-initialized empty" - ); + assert_eq!(warm.device_has_key("15551234567", 0), Some(true)); + assert_eq!(warm.device_has_key("15551234567", 3), None); } #[tokio::test] @@ -1194,36 +1208,43 @@ mod tests { assert!(!cached.is_empty(), "cache should still be warm"); } + /// On remove, the sender_key_devices DB row for the device is dropped + /// (mirrors WA Web's `senderKey.delete(deviceJid)`). The next resolve sees + /// the device gone from the registry and skips it, so no SKDM redistribution + /// is needed for surviving devices. #[tokio::test] - async fn test_patch_device_remove_invalidates_sender_key_cache() { - use crate::sender_key_device_cache::SenderKeyDeviceMap; - + async fn test_patch_device_remove_clears_row_and_keeps_others_warm() { let client = create_test_client().await; - setup_device_record(&client, "15551234567", &[0, 3]).await; - // Warm sender key device cache let group = "120363000000000001@g.us"; - let map = SenderKeyDeviceMap::from_db_rows(&[ - ("15551234567:0@s.whatsapp.net".into(), true), - ("15551234567:3@s.whatsapp.net".into(), true), - ]); client - .sender_key_device_cache - .get_or_init(group, async { std::sync::Arc::new(map) }) - .await; + .persistence_manager + .set_sender_key_status( + group, + &[ + ("15551234567:0@s.whatsapp.net", true), + ("15551234567:3@s.whatsapp.net", true), + ], + ) + .await + .unwrap(); - // Remove device 3 — should invalidate sender key cache client.patch_device_remove("15551234567", 3).await; - let fresh_map = SenderKeyDeviceMap::from_db_rows(&[]); - let result = client - .sender_key_device_cache - .get_or_init(group, async { std::sync::Arc::new(fresh_map) }) - .await; + let rows = client + .persistence_manager + .get_sender_key_devices(group) + .await + .unwrap(); assert!( - result.is_empty(), - "sender key cache should have been invalidated after device removal" + rows.iter() + .any(|(j, _)| j == "15551234567:0@s.whatsapp.net") + ); + assert!( + !rows + .iter() + .any(|(j, _)| j == "15551234567:3@s.whatsapp.net") ); } @@ -1439,4 +1460,193 @@ mod tests { "DB[pn] must be deleted after canonical flip" ); } + + // ── SKDM flow regression tests ───────────────────────────────────── + + #[tokio::test] + async fn patch_device_remove_clears_sender_key_device_rows() { + let client = create_test_client().await; + let user = "15551234567"; + setup_device_record(&client, user, &[0, 5]).await; + + let group = "120363000000000001@g.us"; + let device_jid = format!("{user}:5@s.whatsapp.net"); + client + .persistence_manager + .set_sender_key_status(group, &[(device_jid.as_str(), true)]) + .await + .unwrap(); + + client.patch_device_remove(user, 5).await; + + let rows = client + .persistence_manager + .get_sender_key_devices(group) + .await + .unwrap(); + assert!(rows.iter().all(|(jid, _)| jid != &device_jid)); + } + + #[tokio::test] + async fn patch_device_add_preserves_unrelated_group_caches() { + use crate::sender_key_device_cache::SenderKeyDeviceMap; + + let client = create_test_client().await; + setup_device_record(&client, "15551234567", &[0]).await; + + let group = "120363000000000002@g.us"; + let map = + SenderKeyDeviceMap::from_db_rows(&[("99999999999:0@s.whatsapp.net".into(), true)]); + client + .sender_key_device_cache + .get_or_init(group, async { std::sync::Arc::new(map) }) + .await; + + let elem = make_device_element(3, Some(5)); + client.patch_device_add("15551234567", &elem, None).await; + + let warm = client + .sender_key_device_cache + .get_or_init(group, async { + panic!("cache should still be warm — no global invalidation") + }) + .await; + assert_eq!(warm.device_has_key("99999999999", 0), Some(true)); + } + + #[tokio::test] + async fn patch_device_remove_preserves_unrelated_group_caches() { + use crate::sender_key_device_cache::SenderKeyDeviceMap; + + let client = create_test_client().await; + setup_device_record(&client, "15551234567", &[0, 5]).await; + + let group = "120363000000000002@g.us"; + let map = + SenderKeyDeviceMap::from_db_rows(&[("99999999999:0@s.whatsapp.net".into(), true)]); + client + .sender_key_device_cache + .get_or_init(group, async { std::sync::Arc::new(map) }) + .await; + + client.patch_device_remove("15551234567", 5).await; + + let warm = client + .sender_key_device_cache + .get_or_init(group, async { + panic!("cache should still be warm — no global invalidation") + }) + .await; + assert_eq!(warm.device_has_key("99999999999", 0), Some(true)); + } + + /// Forward secrecy: removing a participant who had `has_key=true` must + /// drop the bot's own sender key and clear the group's tracker so the + /// next send forces full SKDM redistribution. + #[tokio::test] + async fn participant_remove_rotates_sender_key_when_any_had_key() { + use std::str::FromStr; + use wacore::libsignal::protocol::SenderKeyRecord; + use wacore::libsignal::store::sender_key_name::SenderKeyName; + use wacore::types::jid::JidExt; + + let client = create_test_client().await; + let group = "120363000000000001@g.us"; + let own_lid = Jid::from_str("193832511623409:13@lid").unwrap(); + client + .persistence_manager + .process_command(crate::store::commands::DeviceCommand::SetLid(Some( + own_lid.clone(), + ))) + .await; + + let sk_name = SenderKeyName::from_parts(group, own_lid.to_protocol_address().as_str()); + client + .signal_cache + .put_sender_key(&sk_name, SenderKeyRecord::new_empty()) + .await; + + client + .persistence_manager + .set_sender_key_status( + group, + &[ + ("271060335329480:0@lid", true), + ("77610646245392:0@lid", true), + ], + ) + .await + .unwrap(); + + client + .rotate_sender_key_on_participant_remove(group, &["271060335329480"]) + .await; + + let device_arc = client.persistence_manager.get_device_arc().await; + let device = device_arc.read().await; + let key = client + .signal_cache + .get_sender_key(&sk_name, &*device.backend) + .await + .unwrap(); + assert!( + key.is_none(), + "sender key must be deleted on remove rotation" + ); + + let rows = client + .persistence_manager + .get_sender_key_devices(group) + .await + .unwrap(); + assert!(rows.is_empty(), "sender_key_devices must be cleared"); + } + + /// No rotation when removed participants never received an SKDM — there + /// is nothing for them to decrypt forward, so don't pay the redistribute cost. + #[tokio::test] + async fn participant_remove_skips_rotation_when_none_had_key() { + use std::str::FromStr; + use wacore::libsignal::protocol::SenderKeyRecord; + use wacore::libsignal::store::sender_key_name::SenderKeyName; + use wacore::types::jid::JidExt; + + let client = create_test_client().await; + let group = "120363000000000001@g.us"; + let own_lid = Jid::from_str("193832511623409:13@lid").unwrap(); + client + .persistence_manager + .process_command(crate::store::commands::DeviceCommand::SetLid(Some( + own_lid.clone(), + ))) + .await; + + let sk_name = SenderKeyName::from_parts(group, own_lid.to_protocol_address().as_str()); + client + .signal_cache + .put_sender_key(&sk_name, SenderKeyRecord::new_empty()) + .await; + + client + .persistence_manager + .set_sender_key_status(group, &[("271060335329480:0@lid", false)]) + .await + .unwrap(); + + client + .rotate_sender_key_on_participant_remove(group, &["271060335329480"]) + .await; + + let device_arc = client.persistence_manager.get_device_arc().await; + let device = device_arc.read().await; + let key = client + .signal_cache + .get_sender_key(&sk_name, &*device.backend) + .await + .unwrap(); + assert!( + key.is_some(), + "sender key must survive when removed had no key" + ); + } } diff --git a/src/client/sender_keys.rs b/src/client/sender_keys.rs index 92ef97a16..53d0afece 100644 --- a/src/client/sender_keys.rs +++ b/src/client/sender_keys.rs @@ -67,6 +67,69 @@ impl Client { Ok(()) } + /// Forward-secrecy rotation when participants leave a group. Mirrors WA + /// Web's `removeParticipantInfo` (`GroupParticipantHelpers.js`): if any + /// removed user had `has_key=true`, delete the bot's own sender key for + /// the group and wipe `sender_key_devices` so the next send takes the + /// `force_skdm=true` path (`!key_exists`) and redistributes to all + /// remaining participants. + pub(crate) async fn rotate_sender_key_on_participant_remove( + &self, + group_jid: &str, + removed_user_ids: &[&str], + ) { + if removed_user_ids.is_empty() { + return; + } + + let rows = match self + .persistence_manager + .get_sender_key_devices(group_jid) + .await + { + Ok(rows) => rows, + Err(e) => { + log::warn!( + "rotate_sender_key_on_participant_remove: read failed for {group_jid}: {e}" + ); + return; + } + }; + + let any_had_key = rows.iter().any(|(jid_str, has_key)| { + *has_key + && jid_str + .parse::() + .ok() + .is_some_and(|jid| removed_user_ids.iter().any(|u| *u == jid.user.as_str())) + }); + if !any_had_key { + return; + } + + use wacore::libsignal::store::sender_key_name::SenderKeyName; + use wacore::types::jid::JidExt; + let snapshot = self.persistence_manager.get_device_snapshot().await; + for own_jid in snapshot.lid.iter().chain(snapshot.pn.iter()) { + let sk_name = + SenderKeyName::from_parts(group_jid, own_jid.to_protocol_address().as_str()); + self.signal_cache + .delete_sender_key(sk_name.cache_key()) + .await; + } + self.flush_signal_cache_logged("rotate_sender_key_on_participant_remove", None) + .await; + + if let Err(e) = self + .persistence_manager + .clear_sender_key_devices(group_jid) + .await + { + log::warn!("rotate_sender_key_on_participant_remove: clear DB failed: {e}"); + } + self.sender_key_device_cache.invalidate(group_jid).await; + } + /// Take a sent message for retry handling. Checks L1 cache first (if enabled), /// then falls back to DB. On miss, tries an alternate PN/LID key to handle /// mapping changes between send time and retry time (WAWebLidMigrationUtils diff --git a/src/features/groups.rs b/src/features/groups.rs index 67cc6be23..634e1c2c0 100644 --- a/src/features/groups.rs +++ b/src/features/groups.rs @@ -390,6 +390,9 @@ impl<'a> Groups<'a> { info.remove_participants(&accepted); group_cache.insert(jid.clone(), info).await; } + self.client + .rotate_sender_key_on_participant_remove(&jid.to_string(), &accepted) + .await; } Ok(result) } diff --git a/src/handlers/notification.rs b/src/handlers/notification.rs index adf68b2dd..81b2c8b4e 100755 --- a/src/handlers/notification.rs +++ b/src/handlers/notification.rs @@ -1192,10 +1192,9 @@ async fn handle_group_notification(client: &Arc, node: Arc } } GroupNotificationAction::Remove { participants, .. } => { + let users: Vec<&str> = participants.iter().map(|p| p.jid.user.as_str()).collect(); let group_cache = client.get_group_cache().await; if let Some(mut info) = group_cache.get(¬ification.group_jid).await { - let users: Vec<&str> = - participants.iter().map(|p| p.jid.user.as_str()).collect(); info.remove_participants(&users); group_cache .insert(notification.group_jid.clone(), info) @@ -1206,6 +1205,12 @@ async fn handle_group_notification(client: &Arc, node: Arc notification.group_jid, participants.len() ); } + client + .rotate_sender_key_on_participant_remove( + ¬ification.group_jid.to_string(), + &users, + ) + .await; } _ => {} } diff --git a/src/send.rs b/src/send.rs index b68d0512b..54794400f 100644 --- a/src/send.rs +++ b/src/send.rs @@ -696,10 +696,17 @@ impl Client { client.invalidate_device_cache(&own_pn.user).await; } } - client - .sender_key_device_cache - .invalidate(&jid.to_string()) - .await; + let jid_str = jid.to_string(); + // Cache-only invalidation re-reads the same stale rows on + // the next send. Drop the persisted state too so the next + // send takes the full-distribution path. + if (jid.is_group() || jid.is_status_broadcast()) + && let Err(e) = + client.persistence_manager.clear_sender_key_devices(&jid_str).await + { + log::warn!("phash mismatch: clear_sender_key_devices failed: {e}"); + } + client.sender_key_device_cache.invalidate(&jid_str).await; if invalidate_group_cache { client.get_group_cache().await.invalidate(&jid).await; } @@ -966,13 +973,40 @@ impl Client { let sender_key_name = SenderKeyName::from_parts(&to_str, sender_address.as_str()); let device_guard = device_store_arc.read().await; - let key_exists = self + let record = self .signal_cache .get_sender_key(&sender_key_name, &*device_guard.backend) - .await? - .is_some(); + .await?; + let key_exists = record.is_some(); + + // WA Web posts SenderKeyExpired with `PERIODIC_ROTATION` after + // a chain advances past a threshold. Captured-js doesn't show + // the value; 1000 mirrors common Signal hygiene defaults. + const SENDER_KEY_ROTATION_THRESHOLD: u32 = 1000; + let needs_rotation = record + .and_then(|mut r| r.sender_key_state_mut().ok().cloned()) + .and_then(|state| state.sender_chain_key().map(|ck| ck.iteration())) + .is_some_and(|iter| iter >= SENDER_KEY_ROTATION_THRESHOLD); + drop(device_guard); + + if needs_rotation { + log::info!( + "Periodic sender-key rotation for {to} (chain iteration ≥ {SENDER_KEY_ROTATION_THRESHOLD})" + ); + self.signal_cache + .delete_sender_key(sender_key_name.cache_key()) + .await; + if let Err(e) = self + .persistence_manager + .clear_sender_key_devices(&to_str) + .await + { + log::warn!("periodic rotation: clear_sender_key_devices failed: {e}"); + } + self.sender_key_device_cache.invalidate(&to_str).await; + } - force_key_distribution || !key_exists + force_key_distribution || !key_exists || needs_rotation }; let mut store_adapter = self.signal_adapter_from(device_store_arc.clone()); @@ -1231,7 +1265,16 @@ impl Client { } if let Some((rx, phash, msg_id)) = ack { - self.spawn_phash_validation(rx, phash, tc_issue_target.clone(), false, msg_id); + // Group sends also invalidate group cache on mismatch — server's + // participant set diverged, the next send needs a fresh query. + let invalidate_group = tc_issue_target.is_group(); + self.spawn_phash_validation( + rx, + phash, + tc_issue_target.clone(), + invalidate_group, + msg_id, + ); } if let Some(update) = skdm_update { diff --git a/src/sender_key_device_cache.rs b/src/sender_key_device_cache.rs index 07ab2aeec..15c1a2b16 100644 --- a/src/sender_key_device_cache.rs +++ b/src/sender_key_device_cache.rs @@ -84,12 +84,6 @@ impl SenderKeyDeviceCache { self.inner.invalidate(group_jid).await; } - /// Invalidate all entries. Used on raw_id mismatch (identity change) - /// to force SKDM redistribution for all groups. - pub(crate) fn invalidate_all(&self) { - self.inner.invalidate_all(); - } - #[cfg(feature = "debug-diagnostics")] pub(crate) fn entry_count(&self) -> u64 { self.inner.entry_count() diff --git a/src/store/persistence_manager.rs b/src/store/persistence_manager.rs index 9fec958a4..9ec24d926 100644 --- a/src/store/persistence_manager.rs +++ b/src/store/persistence_manager.rs @@ -253,6 +253,15 @@ impl PersistenceManager { pub async fn clear_sender_key_devices(&self, group_jid: &str) -> Result<(), StoreError> { self.backend.clear_sender_key_devices(group_jid).await } + + pub async fn delete_sender_key_device_rows( + &self, + device_jids: &[&str], + ) -> Result<(), StoreError> { + self.backend + .delete_sender_key_device_rows(device_jids) + .await + } } #[cfg(test)] diff --git a/storages/sqlite-storage/src/sqlite_store.rs b/storages/sqlite-storage/src/sqlite_store.rs index 5588a120e..e8407be23 100644 --- a/storages/sqlite-storage/src/sqlite_store.rs +++ b/storages/sqlite-storage/src/sqlite_store.rs @@ -1854,6 +1854,30 @@ impl ProtocolStore for SqliteStore { .await } + async fn delete_sender_key_device_rows(&self, device_jids: &[&str]) -> Result<()> { + if device_jids.is_empty() { + return Ok(()); + } + let device_id = self.device_id; + let owned: Arc> = Arc::new(device_jids.iter().map(|s| s.to_string()).collect()); + self.with_retry("delete_sender_key_device_rows", || { + let owned = Arc::clone(&owned); + Box::new(move |conn: &mut SqliteConnection| { + const CHUNK: usize = 190; + for chunk in owned.chunks(CHUNK) { + diesel::delete( + sender_key_devices::table + .filter(sender_key_devices::device_jid.eq_any(chunk)) + .filter(sender_key_devices::device_id.eq(device_id)), + ) + .execute(conn)?; + } + Ok(()) + }) + }) + .await + } + async fn get_lid_mapping(&self, lid: &str) -> Result> { let pool = self.pool.clone(); let device_id = self.device_id; diff --git a/wacore/src/store/in_memory.rs b/wacore/src/store/in_memory.rs index 9f43121d0..83bb56ed9 100644 --- a/wacore/src/store/in_memory.rs +++ b/wacore/src/store/in_memory.rs @@ -385,6 +385,18 @@ impl ProtocolStore for InMemoryBackend { Ok(()) } + async fn delete_sender_key_device_rows(&self, device_jids: &[&str]) -> Result<()> { + if device_jids.is_empty() { + return Ok(()); + } + let mut state = self.state.lock().await; + let targets: std::collections::HashSet<&str> = device_jids.iter().copied().collect(); + for group_map in state.sender_key_devices.values_mut() { + group_map.retain(|jid, _| !targets.contains(jid.as_str())); + } + Ok(()) + } + // --- LID-PN Mapping --- async fn get_lid_mapping(&self, lid: &str) -> Result> { diff --git a/wacore/src/store/persistence.rs b/wacore/src/store/persistence.rs index 361eda789..d31e0184a 100644 --- a/wacore/src/store/persistence.rs +++ b/wacore/src/store/persistence.rs @@ -210,4 +210,13 @@ impl PersistenceManager { pub async fn clear_sender_key_devices(&self, group_jid: &str) -> Result<(), StoreError> { self.backend.clear_sender_key_devices(group_jid).await } + + pub async fn delete_sender_key_device_rows( + &self, + device_jids: &[&str], + ) -> Result<(), StoreError> { + self.backend + .delete_sender_key_device_rows(device_jids) + .await + } } diff --git a/wacore/src/store/traits.rs b/wacore/src/store/traits.rs index 9cb3a8849..c65ecc9f7 100644 --- a/wacore/src/store/traits.rs +++ b/wacore/src/store/traits.rs @@ -230,6 +230,10 @@ pub trait ProtocolStore: Send + Sync { /// Clear all sender key device tracking for a group (on sender key rotation). async fn clear_sender_key_devices(&self, group_jid: &str) -> Result<()>; + /// Delete specific `sender_key_devices` rows by device JID across all groups. + /// Mirrors WA Web's per-group `senderKey.delete(deviceJid)` cleanup. + async fn delete_sender_key_device_rows(&self, device_jids: &[&str]) -> Result<()>; + /// Clear all sender key device tracking across ALL groups. /// Called on identity change (raw_id mismatch) to force SKDM redistribution. async fn clear_all_sender_key_devices(&self) -> Result<()>; From b4fdad27fecd9f49882859b44ba8f020f4364f20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= Date: Tue, 28 Apr 2026 14:28:12 -0300 Subject: [PATCH 2/4] fix(send): address review of skdm-flow-r1-r5 - patch_device_remove now also evicts cached has_key entries for the removed (user, device_id). A re-add of the same device_id would otherwise hit the stale `has_key=true` entry and skip SKDM redistribution. New helper `SenderKeyDeviceCache::invalidate_entries_for_device`. - rotate_sender_key_on_participant_remove rotates conservatively when the `sender_key_devices` read fails, instead of returning early. Better to pay the redistribute cost than leave the sender key in place after a remove we couldn't audit. - phash mismatch handler now falls back to deleting the bot's own sender key when `clear_sender_key_devices` fails. With the persisted state potentially stale, the next send is forced through `force_skdm=true` via `!key_exists` instead of trusting the in-memory invalidation alone. - New regression `patch_device_remove_evicts_cached_has_key_for_removed_device`. --- src/client/device_registry.rs | 48 +++++++++++++++++++++++++++++++--- src/client/sender_keys.rs | 13 +++++---- src/send.rs | 33 ++++++++++++++++++----- src/sender_key_device_cache.rs | 19 ++++++++++++++ 4 files changed, 99 insertions(+), 14 deletions(-) diff --git a/src/client/device_registry.rs b/src/client/device_registry.rs index bff08746e..0a765820c 100644 --- a/src/client/device_registry.rs +++ b/src/client/device_registry.rs @@ -377,9 +377,10 @@ impl Client { /// Delete `sender_key_devices` rows whose `device_jid` matches the given /// (user, device_id) under either LID or PN addressing. Both alias keys - /// for the user are tried via `resolve_lookup_keys`. Touches DB then - /// invalidates the in-memory cache for groups that may have indexed the - /// removed JID. + /// for the user are tried via `resolve_lookup_keys`. The in-memory cache + /// is also evicted for groups that indexed the removed JID — necessary + /// because a future re-add of the same device_id would otherwise hit + /// a stale `has_key=true` entry and skip SKDM. async fn delete_sender_key_rows_for_device(&self, user: &str, device_id: u32) { let lookup = self.resolve_lookup_keys(user).await; let servers = [wacore_binary::Server::Lid, wacore_binary::Server::Pn]; @@ -399,6 +400,12 @@ impl Client { { warn!("delete_sender_key_rows_for_device: {e}"); } + + for key in lookup.all_keys() { + self.sender_key_device_cache + .invalidate_entries_for_device(key, device_id as u16) + .await; + } } /// Update key_index for a device in the registry. @@ -1463,6 +1470,41 @@ mod tests { // ── SKDM flow regression tests ───────────────────────────────────── + /// After remove, the in-memory cache must not return `has_key=true` for + /// the removed JID. A future re-add of the same device_id would otherwise + /// hit the stale entry and skip SKDM redistribution. + #[tokio::test] + async fn patch_device_remove_evicts_cached_has_key_for_removed_device() { + use crate::sender_key_device_cache::SenderKeyDeviceMap; + + let client = create_test_client().await; + let user = "15551234567"; + setup_device_record(&client, user, &[0, 5]).await; + + let group = "120363000000000001@g.us"; + let map = SenderKeyDeviceMap::from_db_rows(&[(format!("{user}:5@s.whatsapp.net"), true)]); + client + .sender_key_device_cache + .get_or_init(group, async { std::sync::Arc::new(map) }) + .await; + + client.patch_device_remove(user, 5).await; + + let reloaded = client + .sender_key_device_cache + .get_or_init(group, async { + std::sync::Arc::new(SenderKeyDeviceMap::from_db_rows( + &client + .persistence_manager + .get_sender_key_devices(group) + .await + .unwrap(), + )) + }) + .await; + assert_eq!(reloaded.device_has_key(user, 5), None); + } + #[tokio::test] async fn patch_device_remove_clears_sender_key_device_rows() { let client = create_test_client().await; diff --git a/src/client/sender_keys.rs b/src/client/sender_keys.rs index 53d0afece..455112e4c 100644 --- a/src/client/sender_keys.rs +++ b/src/client/sender_keys.rs @@ -82,17 +82,20 @@ impl Client { return; } - let rows = match self + // Read failure → rotate anyway. Better to pay the redistribute cost + // than leave the sender key in place after a removal we couldn't audit. + let (rows, read_failed) = match self .persistence_manager .get_sender_key_devices(group_jid) .await { - Ok(rows) => rows, + Ok(r) => (r, false), Err(e) => { log::warn!( - "rotate_sender_key_on_participant_remove: read failed for {group_jid}: {e}" + "rotate_sender_key_on_participant_remove: read failed for {group_jid}: {e} \ + — rotating conservatively" ); - return; + (Vec::new(), true) } }; @@ -103,7 +106,7 @@ impl Client { .ok() .is_some_and(|jid| removed_user_ids.iter().any(|u| *u == jid.user.as_str())) }); - if !any_had_key { + if !read_failed && !any_had_key { return; } diff --git a/src/send.rs b/src/send.rs index 54794400f..eee01b4b7 100644 --- a/src/send.rs +++ b/src/send.rs @@ -699,12 +699,33 @@ impl Client { let jid_str = jid.to_string(); // Cache-only invalidation re-reads the same stale rows on // the next send. Drop the persisted state too so the next - // send takes the full-distribution path. - if (jid.is_group() || jid.is_status_broadcast()) - && let Err(e) = - client.persistence_manager.clear_sender_key_devices(&jid_str).await - { - log::warn!("phash mismatch: clear_sender_key_devices failed: {e}"); + // send takes the full-distribution path. If the clear + // fails, fall back to deleting the bot's own sender key + // for the chat — the next send will see `!key_exists` + // and force_skdm without depending on the tracker. + if jid.is_group() || jid.is_status_broadcast() { + let cleared = client + .persistence_manager + .clear_sender_key_devices(&jid_str) + .await; + if let Err(e) = cleared { + log::warn!( + "phash mismatch: clear_sender_key_devices failed: {e} — \ + deleting own sender key as fallback to force redistribution" + ); + use wacore::libsignal::store::sender_key_name::SenderKeyName; + use wacore::types::jid::JidExt; + let snapshot = + client.persistence_manager.get_device_snapshot().await; + for own in snapshot.lid.iter().chain(snapshot.pn.iter()) { + let sk = + SenderKeyName::from_parts(&jid_str, own.to_protocol_address().as_str()); + client.signal_cache.delete_sender_key(sk.cache_key()).await; + } + let _ = client + .flush_signal_cache_logged("phash-mismatch-fallback", None) + .await; + } } client.sender_key_device_cache.invalidate(&jid_str).await; if invalidate_group_cache { diff --git a/src/sender_key_device_cache.rs b/src/sender_key_device_cache.rs index 15c1a2b16..c0a673dfa 100644 --- a/src/sender_key_device_cache.rs +++ b/src/sender_key_device_cache.rs @@ -84,6 +84,25 @@ impl SenderKeyDeviceCache { self.inner.invalidate(group_jid).await; } + /// Drop cache entries whose map indexes the given (user, device_id). Needed + /// after a device is removed: a future re-add of the same device_id would + /// otherwise hit a stale `has_key=true` entry and skip SKDM redistribution. + pub(crate) async fn invalidate_entries_for_device(&self, user: &str, device_id: u16) { + let to_drop: Vec = self + .inner + .iter() + .filter_map(|(group_jid, map)| { + map.devices + .get(user) + .and_then(|devmap| devmap.get(&device_id)) + .map(|_| group_jid.as_ref().clone()) + }) + .collect(); + for g in to_drop { + self.inner.invalidate(&g).await; + } + } + #[cfg(feature = "debug-diagnostics")] pub(crate) fn entry_count(&self) -> u64 { self.inner.entry_count() From 01b18597f9d715ca02040a65ee64e45dede325a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= Date: Tue, 28 Apr 2026 15:12:13 -0300 Subject: [PATCH 3/4] fix(send): propagate sender-key delete failures from patch_device_remove MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `delete_sender_key_rows_for_device` now returns `Result<(), StoreError>` instead of swallowing the persistence error with `warn!`. Cache eviction runs only on success. `patch_device_remove` aborts the registry update when the sender-key cleanup fails — a half-applied state where `resolve_devices` reports "gone" but the tracker still vouches `has_key=true` would silently skip SKDM redistribution and leave forward-secrecy gaps the rotation paths were designed to close. --- src/client/device_registry.rs | 36 +++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/src/client/device_registry.rs b/src/client/device_registry.rs index 0a765820c..4de2f6355 100644 --- a/src/client/device_registry.rs +++ b/src/client/device_registry.rs @@ -365,9 +365,20 @@ impl Client { .await; } // WA Web's `updateGroupParticipantsInTransaction` deletes the - // device JID from each affected group's senderKey Map. - self.delete_sender_key_rows_for_device(user, device_id) - .await; + // device JID from each affected group's senderKey Map. Skip + // the registry update on failure: a half-applied state where + // `resolve_devices` says "gone" but the tracker still vouches + // `has_key=true` would silently skip SKDM redistribution. + if let Err(e) = self + .delete_sender_key_rows_for_device(user, device_id) + .await + { + warn!( + "patch_device_remove: sender-key cleanup failed for {user}:{device_id}: {e} \ + — aborting registry update" + ); + return; + } if let Err(e) = self.update_device_list(record).await { warn!("patch_device_remove: failed to persist: {e}"); } @@ -381,7 +392,15 @@ impl Client { /// is also evicted for groups that indexed the removed JID — necessary /// because a future re-add of the same device_id would otherwise hit /// a stale `has_key=true` entry and skip SKDM. - async fn delete_sender_key_rows_for_device(&self, user: &str, device_id: u32) { + /// + /// Cache eviction runs only after the DB delete succeeds; on failure the + /// error is propagated so the caller can leave both DB and cache in their + /// pre-call state rather than half-applying the cleanup. + async fn delete_sender_key_rows_for_device( + &self, + user: &str, + device_id: u32, + ) -> Result<(), wacore::store::error::StoreError> { let lookup = self.resolve_lookup_keys(user).await; let servers = [wacore_binary::Server::Lid, wacore_binary::Server::Pn]; let mut candidates: Vec = Vec::with_capacity(4); @@ -393,19 +412,16 @@ impl Client { } } let refs: Vec<&str> = candidates.iter().map(|s| s.as_str()).collect(); - if let Err(e) = self - .persistence_manager + self.persistence_manager .delete_sender_key_device_rows(&refs) - .await - { - warn!("delete_sender_key_rows_for_device: {e}"); - } + .await?; for key in lookup.all_keys() { self.sender_key_device_cache .invalidate_entries_for_device(key, device_id as u16) .await; } + Ok(()) } /// Update key_index for a device in the registry. From ae0ab110e4c3cb97b1f8272435417d29ad4f9b81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= Date: Tue, 28 Apr 2026 15:21:27 -0300 Subject: [PATCH 4/4] =?UTF-8?q?fix(device=5Fregistry):=20guard=20u32?= =?UTF-8?q?=E2=86=92u16=20downcast=20and=20refresh=20patch=5Fdevice=5Fadd?= =?UTF-8?q?=20doc?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `patch_device_remove` now uses `u16::try_from` before passing the device id to JID-keyed cleanup paths. Out-of-range ids are logged and the registry update is still applied so `resolve_devices` stops returning the device — blind `as u16` would otherwise truncate to a different id and clean up the wrong device's sessions/sender-key rows. The inner helper takes `u16` directly so the bounds check happens once at the entry point. `patch_device_add`'s doc comment now describes the post-PR-#604 flow (automatic resolution via `resolve_skdm_targets` / `device_has_key`) instead of claiming a global cache invalidation the body no longer does. --- src/client/device_registry.rs | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/src/client/device_registry.rs b/src/client/device_registry.rs index 4de2f6355..f764a784f 100644 --- a/src/client/device_registry.rs +++ b/src/client/device_registry.rs @@ -218,8 +218,10 @@ impl Client { /// 4. Replace the full device record /// /// If `signed_bytes` is absent, falls back to simple append (lenient). - /// When a genuinely new device is added, invalidates the sender key device - /// cache so SKDM will be sent on the next group message. + /// + /// New devices need no explicit cache invalidation: `resolve_skdm_targets` + /// queries the registry on each send and `device_has_key()` returns `None` + /// for unseen device IDs, dropping them into `needs_skdm` automatically. pub(crate) async fn patch_device_add( &self, user: &str, @@ -360,8 +362,22 @@ impl Client { let before = record.devices.len(); record.devices.retain(|d| d.device_id != device_id); if record.devices.len() != before { - if device_id != 0 { - self.delete_sessions_for_devices(user, &[device_id as u16]) + // JID-keyed structures (Signal sessions, sender_key_devices) + // store device as u16. A blind cast for ids > u16::MAX would + // truncate to a different value and cleanup the wrong device. + let Ok(device_id_u16) = u16::try_from(device_id) else { + warn!( + "patch_device_remove: device_id {device_id} > u16::MAX — skipping \ + session/SKDM cleanup but still persisting registry removal" + ); + if let Err(e) = self.update_device_list(record).await { + warn!("patch_device_remove: failed to persist: {e}"); + } + return; + }; + + if device_id_u16 != 0 { + self.delete_sessions_for_devices(user, &[device_id_u16]) .await; } // WA Web's `updateGroupParticipantsInTransaction` deletes the @@ -370,7 +386,7 @@ impl Client { // `resolve_devices` says "gone" but the tracker still vouches // `has_key=true` would silently skip SKDM redistribution. if let Err(e) = self - .delete_sender_key_rows_for_device(user, device_id) + .delete_sender_key_rows_for_device(user, device_id_u16) .await { warn!( @@ -399,7 +415,7 @@ impl Client { async fn delete_sender_key_rows_for_device( &self, user: &str, - device_id: u32, + device_id: u16, ) -> Result<(), wacore::store::error::StoreError> { let lookup = self.resolve_lookup_keys(user).await; let servers = [wacore_binary::Server::Lid, wacore_binary::Server::Pn]; @@ -407,7 +423,7 @@ impl Client { for server in servers { for key in lookup.all_keys() { let mut jid = Jid::new(key, server); - jid.device = device_id as u16; + jid.device = device_id; candidates.push(jid.to_string()); } } @@ -418,7 +434,7 @@ impl Client { for key in lookup.all_keys() { self.sender_key_device_cache - .invalidate_entries_for_device(key, device_id as u16) + .invalidate_entries_for_device(key, device_id) .await; } Ok(())