Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
162 changes: 160 additions & 2 deletions src/client/device_registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,23 @@ impl Client {
self.append_device_if_new(&mut record, device_id, device.key_index);
}

// WA Web `AdvDeviceNotificationApi.handleDeviceAddNotification` re-adds the
// primary (device 0) to the rebuilt list unconditionally, so a raw_id
// mismatch (which clears the list) never drops it. `filter_devices_by_key_index`
// only keeps device 0 when it is already in the input, so after a clear the
// primary would otherwise be lost, leaving a record with no device 0 (or an
// empty list) that suppresses the usync re-fetch forever.
//
// The primary's key_index is never read (`filter_devices_by_key_index` keeps
// device 0 regardless and `is_key_index_valid` is not applied to it), so store
// `None` to match how device 0 is recorded everywhere else.
if !record.devices.iter().any(|d| d.device_id == 0) {
record.devices.push(wacore::store::traits::DeviceInfo {
device_id: 0,
key_index: None,
});
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// 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.
Expand Down Expand Up @@ -1028,7 +1045,7 @@ mod tests {
async fn test_patch_device_add_deduplicates() {
let client = create_test_client().await;

setup_device_record(&client, "15551234567", &[3]).await;
setup_device_record(&client, "15551234567", &[0, 3]).await;

// Patch: add device 3 again — should not duplicate
let elem = make_device_element(3, None);
Expand All @@ -1039,7 +1056,12 @@ mod tests {
.get("15551234567")
.await
.unwrap();
assert_eq!(updated.devices.len(), 1);
assert_eq!(
updated.devices.iter().filter(|d| d.device_id == 3).count(),
1
);
assert!(updated.devices.iter().any(|d| d.device_id == 0));
assert_eq!(updated.devices.len(), 2);
}

#[tokio::test]
Expand Down Expand Up @@ -1136,6 +1158,142 @@ mod tests {
assert_eq!(dev3.key_index, Some(2));
}

/// Encode an `ADVSignedKeyIndexList` whose decoded `raw_id`/`valid_indexes`
/// drive `patch_device_add` (the signature is not verified locally; the
/// notification arrives over the authenticated Noise channel).
fn make_signed_key_index_bytes(
raw_id: u32,
current_index: u32,
valid_indexes: Vec<u32>,
) -> Vec<u8> {
use prost::Message;
let details = waproto::whatsapp::AdvKeyIndexList {
raw_id: Some(raw_id),
timestamp: Some(100),
current_index: Some(current_index),
valid_indexes,
account_type: None,
}
.encode_to_vec();
waproto::whatsapp::AdvSignedKeyIndexList {
details: Some(details),
account_signature: None,
account_signature_key: None,
}
.encode_to_vec()
}

fn record_with_raw_id(
user: &str,
device_ids: &[u32],
raw_id: u32,
) -> wacore::store::traits::DeviceListRecord {
wacore::store::traits::DeviceListRecord {
user: user.into(),
devices: device_ids
.iter()
.map(|&id| wacore::store::traits::DeviceInfo {
device_id: id,
key_index: if id == 0 { None } else { Some(7) },
})
.collect(),
timestamp: 1000,
phash: None,
raw_id: Some(raw_id),
}
}

// raw_id mismatch clears the list and rebuilds from the notification; the
// primary (device 0) must survive. Regression guard for a record that would
// otherwise persist without device 0 (and then suppress usync forever).
#[tokio::test]
async fn test_patch_device_add_raw_id_mismatch_preserves_primary() {
let client = create_test_client().await;

client
.device_registry_cache
.insert(
"15551234567".to_string(),
Arc::new(record_with_raw_id("15551234567", &[0, 5], 1)),
)
.await;

// New raw_id (2) != stored (1) → clear + rebuild. Notified device 19 has a
// valid key index, so the rebuilt list is the companion plus the primary.
let signed = make_signed_key_index_bytes(2, 0, vec![7]);
let key_index_info = wacore::stanza::devices::KeyIndexInfo {
timestamp: 100,
signed_bytes: Some(signed),
};
let elem = make_device_element(19, Some(7));
client
.patch_device_add("15551234567", &elem, Some(&key_index_info))
.await;

let updated = client
.device_registry_cache
.get("15551234567")
.await
.unwrap();
let dev0 = updated
.devices
.iter()
.find(|d| d.device_id == 0)
.unwrap_or_else(|| {
panic!(
"primary (device 0) must survive a raw_id mismatch clear, got {:?}",
updated.devices
)
});
// The re-seeded primary carries no key index (it is never read for device 0).
assert_eq!(dev0.key_index, None);
assert!(updated.devices.iter().any(|d| d.device_id == 19));
// Stale companion from the old identity is dropped by the clear.
assert!(!updated.devices.iter().any(|d| d.device_id == 5));
}
Comment on lines +1210 to +1253

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Consider asserting device 0's key_index value in this test.

The test verifies device 0 is present after the raw_id mismatch rebuild, but it doesn't check what key_index value device 0 has. Given the inconsistency I flagged earlier (Some(0) in production code vs None in test helpers), we should explicitly verify the expected key_index here. That way if we need to change it later, the test will document the correct behavior.

At WhatsApp's scale, we can't afford ambiguity about device state. Add an assertion like:

let dev0 = updated.devices.iter().find(|d| d.device_id == 0).unwrap();
assert_eq!(dev0.key_index, Some(0), "device 0 should have key_index 0 after rebuild");
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/device_registry.rs` around lines 1206 - 1242, In
test_patch_device_add_raw_id_mismatch_preserves_primary, after retrieving
updated from device_registry_cache, locate the device with device_id == 0 (e.g.
via updated.devices.iter().find(|d| d.device_id == 0).unwrap()) and add an
assertion that its key_index equals Some(0) (assert_eq!(dev0.key_index, Some(0),
...)) so the test documents the expected key_index for the primary after the
raw_id mismatch rebuild.


// Same mismatch but the notified device's key index is rejected, so the
// rebuilt list would be empty without the primary re-seed. Guards the `[]`
// record that otherwise leaves the user with zero devices.
#[tokio::test]
async fn test_patch_device_add_raw_id_mismatch_rejected_device_keeps_primary() {
let client = create_test_client().await;

client
.device_registry_cache
.insert(
"15551234567".to_string(),
Arc::new(record_with_raw_id("15551234567", &[0, 5], 1)),
)
.await;

// current_index 10, empty valid set → notified key index 3 is invalid
// (not in valid set, not > current_index), so no companion is added.
let signed = make_signed_key_index_bytes(2, 10, vec![]);
let key_index_info = wacore::stanza::devices::KeyIndexInfo {
timestamp: 100,
signed_bytes: Some(signed),
};
let elem = make_device_element(19, Some(3));
client
.patch_device_add("15551234567", &elem, Some(&key_index_info))
.await;

let updated = client
.device_registry_cache
.get("15551234567")
.await
.unwrap();
assert_eq!(
updated.devices.len(),
1,
"expected only the primary, got {:?}",
updated.devices
);
assert_eq!(updated.devices[0].device_id, 0);
assert_eq!(updated.devices[0].key_index, None);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

#[tokio::test]
async fn test_lid_migration_preserves_registry_cache() {
use wacore::store::traits::{DeviceInfo, DeviceListRecord};
Expand Down
Loading