Skip to content
Merged
Show file tree
Hide file tree
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
18 changes: 14 additions & 4 deletions src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3581,8 +3581,13 @@ impl Client {
}

pub async fn get_push_name(&self) -> String {
let device_snapshot = self.persistence_manager.get_device_snapshot().await;
device_snapshot.push_name.clone()
self.persistence_manager
.get_device_arc()
.await
.read()
.await
.push_name
.clone()
Comment on lines +3584 to +3590

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

We need to keep device reads on the snapshot path

Line 3584 and Line 3604 now read through get_device_arc().read(). That breaks the repo’s state-access contract for reads. Move both getters back to get_device_snapshot().await (or an explicitly approved accessor that preserves the same contract).

Suggested fix
 pub async fn get_push_name(&self) -> String {
     self.persistence_manager
-        .get_device_arc()
-        .await
-        .read()
+        .get_device_snapshot()
         .await
         .push_name
         .clone()
 }
@@
 pub async fn get_lid(&self) -> Option<Jid> {
     self.persistence_manager
-        .get_device_arc()
-        .await
-        .read()
+        .get_device_snapshot()
         .await
         .lid
         .clone()
 }

As per coding guidelines: Never modify Device state directly; always use DeviceCommand + PersistenceManager::process_command() for state mutations and get_device_snapshot() for reading state.

Also applies to: 3604-3610

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/client.rs` around lines 3584 - 3590, The code is reading Device state via
self.persistence_manager.get_device_arc().read().await which violates the read
contract; change both occurrences (the getter that accesses push_name and the
other at lines ~3604-3610) to use
self.persistence_manager.get_device_snapshot().await so reads use the snapshot
accessor, and keep any mutations routed through DeviceCommand +
PersistenceManager::process_command(); locate uses of get_device_arc(), replace
with get_device_snapshot().await (or an approved accessor) and ensure no direct
Device state mutation occurs outside DeviceCommand/ProcessCommand.

}

pub async fn get_pn(&self) -> Option<Jid> {
Expand All @@ -3596,8 +3601,13 @@ impl Client {
}

pub async fn get_lid(&self) -> Option<Jid> {
let snapshot = self.persistence_manager.get_device_snapshot().await;
snapshot.lid.clone()
self.persistence_manager
.get_device_arc()
.await
.read()
.await
.lid
.clone()
}

pub(crate) async fn require_pn(&self) -> Result<Jid> {
Expand Down
22 changes: 13 additions & 9 deletions src/client/sender_keys.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,22 +15,26 @@ impl Client {
has_key: bool,
exclude_own_devices: bool,
) -> Result<()> {
let (own_lid_user, own_pn_user) = if exclude_own_devices {
let snapshot = self.persistence_manager.get_device_snapshot().await;
(
snapshot.lid.as_ref().map(|j| j.user.clone()),
snapshot.pn.as_ref().map(|j| j.user.clone()),
)
let snapshot = if exclude_own_devices {
Some(self.persistence_manager.get_device_snapshot().await)
} else {
(None, None)
None
};
let own_lid_user = snapshot
.as_ref()
.and_then(|s| s.lid.as_ref())
.map(|j| j.user.as_str());
let own_pn_user = snapshot
.as_ref()
.and_then(|s| s.pn.as_ref())
.map(|j| j.user.as_str());

let device_ids: Vec<String> = device_jids
.iter()
.filter(|jid| {
!exclude_own_devices
|| !(own_lid_user.as_deref().is_some_and(|u| u == jid.user)
|| own_pn_user.as_deref().is_some_and(|u| u == jid.user))
|| !(own_lid_user.is_some_and(|u| u == jid.user)
|| own_pn_user.is_some_and(|u| u == jid.user))
})
.map(ToString::to_string)
.collect();
Expand Down
31 changes: 15 additions & 16 deletions src/features/groups.rs
Original file line number Diff line number Diff line change
Expand Up @@ -193,22 +193,21 @@ impl<'a> Groups<'a> {

let group = self.client.execute(GroupQueryIq::new(jid)).await?;

let participants: Vec<Jid> = group.participants.iter().map(|p| p.jid.clone()).collect();

let lid_to_pn_map: HashMap<wacore_binary::CompactString, Jid> =
if group.addressing_mode == AddressingMode::Lid {
group
.participants
.iter()
.filter_map(|p| {
p.phone_number
.as_ref()
.map(|pn| (p.jid.user.clone(), pn.clone()))
})
.collect()
} else {
HashMap::new()
};
// Single pass: move participants out and build lid_to_pn_map alongside.
let n = group.participants.len();
let is_lid = group.addressing_mode == AddressingMode::Lid;
let mut participants: Vec<Jid> = Vec::with_capacity(n);
let mut lid_to_pn_map: HashMap<wacore_binary::CompactString, Jid> = if is_lid {
HashMap::with_capacity(n)
} else {
HashMap::new()
};
for p in group.participants {
if is_lid && let Some(pn) = p.phone_number {
lid_to_pn_map.insert(p.jid.user.clone(), pn);
}
participants.push(p.jid);
}

let mut info = GroupInfo::new(participants, group.addressing_mode);
if !lid_to_pn_map.is_empty() {
Expand Down
32 changes: 21 additions & 11 deletions src/features/polls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,8 @@ impl<'a> Polls<'a> {
.get_pn()
.await
.ok_or_else(|| anyhow!("Not logged in — cannot determine own JID"))?;
let voter_jid_str = my_jid.to_non_ad().to_string();
let my_base = my_jid.to_non_ad();
let voter_jid_str = my_base.to_string();
let creator_jid_str = poll_creator_jid.to_non_ad().to_string();

let selected_hashes: Vec<Vec<u8>> = option_names
Expand All @@ -136,7 +137,7 @@ impl<'a> Polls<'a> {
let (enc_payload, iv) =
poll::encrypt_poll_vote(&selected_hashes, &key, poll_msg_id, &voter_jid_str)?;

let from_me = my_jid.to_non_ad() == poll_creator_jid.to_non_ad();
let from_me = my_base.is_same_user_as(poll_creator_jid);

let poll_update = wa::message::PollUpdateMessage {
poll_creation_message_key: Some(wa::MessageKey {
Expand Down Expand Up @@ -196,24 +197,33 @@ impl<'a> Polls<'a> {
.map(|name| (poll::compute_option_hash(name), name.as_str()))
.collect();

// `creator_str` is invariant across voters; `decrypt_vote` used to
// recompute it per voter via `poll_creator_jid.to_non_ad().to_string()`.
let creator_str = poll_creator_jid.to_non_ad().to_string();

// Last-vote-wins: each new vote from the same voter replaces the previous
let mut latest_votes: HashMap<String, Vec<Vec<u8>>> = HashMap::new();
let mut latest_votes: HashMap<String, Vec<Vec<u8>>> = HashMap::with_capacity(votes.len());
for (voter_jid, enc_payload, enc_iv) in votes {
let voter_key = voter_jid.to_non_ad().to_string();
match Self::decrypt_vote(
enc_payload,
enc_iv,
let voter_str = voter_jid.to_non_ad().to_string();
let key = match poll::derive_vote_encryption_key(
message_secret,
poll_msg_id,
poll_creator_jid,
voter_jid,
&creator_str,
&voter_str,
) {
Ok(k) => k,
Err(e) => {
log::warn!("Failed to derive vote key for {voter_jid}: {e}");
continue;
}
};
match poll::decrypt_poll_vote(enc_payload, enc_iv, &key, poll_msg_id, &voter_str) {
Ok(selected_hashes) => {
if selected_hashes.is_empty() {
// Empty selection = voter cleared their vote
latest_votes.remove(&voter_key);
latest_votes.remove(&voter_str);
} else {
latest_votes.insert(voter_key, selected_hashes);
latest_votes.insert(voter_str, selected_hashes);
}
}
Err(e) => {
Expand Down
25 changes: 15 additions & 10 deletions src/message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1098,17 +1098,18 @@ impl Client {
}
let mut adapter = self.signal_adapter().await;

// Always use bare sender for sender key operations. Real WA delivers
// skmsg with bare participant but pkmsg (SKDM) with device-qualified
// participant — normalizing to bare ensures consistent lookup.
// Hoisted out of the payload loop: all three are loop-invariant.
let sender_for_sk = info.source.sender.to_non_ad();
let sender_address = sender_for_sk.to_protocol_address();
let sender_key_name = make_sender_key_name(&info.source.chat, &sender_address);
Comment on lines +1101 to +1107

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Stop stripping device scope from sender-key addresses here.

This path normalizes info.source.sender with to_non_ad(), which can collapse device-qualified LID sender identities and miss sender-key state keyed by the device-scoped address.

Suggested fix
-        let sender_for_sk = info.source.sender.to_non_ad();
-        let sender_address = sender_for_sk.to_protocol_address();
+        let sender_address = info.source.sender.to_protocol_address();
         let sender_key_name = make_sender_key_name(&info.source.chat, &sender_address);

Based on learnings: sender-key naming in this codebase should use device-scoped LID protocol addresses (to_protocol_address()), not to_non_ad().to_protocol_address().

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Always use bare sender for sender key operations. Real WA delivers
// skmsg with bare participant but pkmsg (SKDM) with device-qualified
// participant — normalizing to bare ensures consistent lookup.
// Hoisted out of the payload loop: all three are loop-invariant.
let sender_for_sk = info.source.sender.to_non_ad();
let sender_address = sender_for_sk.to_protocol_address();
let sender_key_name = make_sender_key_name(&info.source.chat, &sender_address);
// Always use bare sender for sender key operations. Real WA delivers
// skmsg with bare participant but pkmsg (SKDM) with device-qualified
// participant — normalizing to bare ensures consistent lookup.
// Hoisted out of the payload loop: all three are loop-invariant.
let sender_address = info.source.sender.to_protocol_address();
let sender_key_name = make_sender_key_name(&info.source.chat, &sender_address);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/message.rs` around lines 1101 - 1107, The code is incorrectly stripping
device scope by calling to_non_ad() on info.source.sender before generating the
sender key name, which can cause missing device-scoped sender-key state; remove
the to_non_ad() call and build the protocol address from the original
device-scoped sender (use info.source.sender.to_protocol_address()) so that
sender_address and the call to make_sender_key_name(&info.source.chat,
&sender_address) use the device-scoped LID protocol address instead of a non-AD
normalized one.


for payload in payloads {
let ciphertext = &payload.ciphertext[..];
let padding_version = payload.padding_version;

// Always use bare sender for sender key operations. Real WA delivers
// skmsg with bare participant but pkmsg (SKDM) with device-qualified
// participant — normalizing to bare ensures consistent lookup.
let sender_for_sk = info.source.sender.to_non_ad();
let sender_address = sender_for_sk.to_protocol_address();
let sender_key_name = make_sender_key_name(&info.source.chat, &sender_address);

log::debug!(
"Looking up sender key for group {} with sender address {} (from sender JID: {})",
info.source.chat,
Expand Down Expand Up @@ -1434,10 +1435,14 @@ impl Client {
&self,
node: &wacore_binary::NodeRef<'_>,
) -> Result<MessageInfo, anyhow::Error> {
let device_snapshot = self.persistence_manager.get_device_snapshot().await;
let (own_pn, own_lid) = {
let arc = self.persistence_manager.get_device_arc().await;
let guard = arc.read().await;
(guard.pn.clone(), guard.lid.clone())
};
let default_jid = Jid::default();
let own_jid = device_snapshot.pn.as_ref().unwrap_or(&default_jid);
wacore::messages::parse_message_info(node, own_jid, device_snapshot.lid.as_ref())
let own_jid = own_pn.as_ref().unwrap_or(&default_jid);
wacore::messages::parse_message_info(node, own_jid, own_lid.as_ref())
}

pub(crate) async fn handle_app_state_sync_key_share(
Expand Down
19 changes: 11 additions & 8 deletions src/retry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,10 @@ fn resolve_retry_chat_info(
let is_peer = own_pn.is_some_and(|pn| from.is_same_user_as(pn))
|| own_lid.is_some_and(|lid| from.is_same_user_as(lid));

let chat = if is_bot && recipient.is_some() {
recipient.clone().unwrap().to_non_ad()
let chat = if is_bot && let Some(r) = recipient.as_ref() {
r.to_non_ad()
} else if is_peer {
match &recipient {
match recipient.as_ref() {
Some(r) => r.to_non_ad(),
// No recipient on peer retry — chat will be our own JID,
// message lookup will likely fail. WA Web returns null here.
Expand Down Expand Up @@ -233,13 +233,14 @@ impl Client {
log::debug!("Ignoring retry for {processing_key}: a retry is already in progress.");
return Ok(());
}
// processing_key isn't needed by name after this point — move it into
// the scopeguard instead of cloning again.
let pending = Arc::clone(&self.pending_retries);
let guard_key = processing_key.clone();
let _guard = scopeguard::guard((), move |()| {
pending
.lock()
.unwrap_or_else(|p| p.into_inner())
.remove(&guard_key);
.remove(&processing_key);
});

let (original_msg, alt_chat) = match self.take_recent_message(&info.chat, &message_id).await
Expand Down Expand Up @@ -282,11 +283,13 @@ impl Client {
};

let sender_device_id = info.requester.device() as u32;
let sender_user = info.requester.user.clone();
if !self.has_device(&sender_user, sender_device_id).await {
if !self
.has_device(&info.requester.user, sender_device_id)
.await
{
warn!(
"handle_retry_receipt: device not found for device={}, user={}",
sender_device_id, sender_user
sender_device_id, info.requester.user
);
return Ok(());
}
Expand Down
10 changes: 4 additions & 6 deletions src/send.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1154,12 +1154,10 @@ impl Client {
!is_sender
});

// Dedup for self-DMs: recipient and own device lists overlap
// when sending to own account (WA Web uses Map keyed by toString)
{
let mut seen = std::collections::HashSet::with_capacity(all_dm_jids.len());
all_dm_jids.retain(|j| seen.insert(j.clone()));
}
// Dedup for self-DMs: recipient and own device lists overlap when
// sending to own account. `participant_list_hash` sorts internally,
// so reordering here is safe.
wacore::types::jid::sort_dedup_by_device(&mut all_dm_jids);

self.ensure_e2e_sessions(&all_dm_jids).await?;

Expand Down
4 changes: 2 additions & 2 deletions src/sender_key_device_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ pub(crate) struct SenderKeyDeviceMap {

impl SenderKeyDeviceMap {
pub fn from_db_rows(rows: &[(String, bool)]) -> Self {
let mut devices: HashMap<Arc<str>, HashMap<u16, bool>> = HashMap::new();
let mut forgotten_users = HashSet::new();
let mut devices: HashMap<Arc<str>, HashMap<u16, bool>> = HashMap::with_capacity(rows.len());
let mut forgotten_users = HashSet::with_capacity(rows.len() / 4);

for (jid_str, has_key) in rows {
match jid_str.parse::<Jid>() {
Expand Down
5 changes: 3 additions & 2 deletions wacore/src/prekeys.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,9 @@ impl PreKeyUtils {
.get_optional_child("list")
.ok_or_else(|| anyhow::anyhow!("<list> not found in pre-key response"))?;

let mut bundles = HashMap::new();
for user_node_ref in list_node.children().unwrap_or_default() {
let children = list_node.children().unwrap_or_default();
let mut bundles = HashMap::with_capacity(children.len());
for user_node_ref in children {
if user_node_ref.tag != "user" {
continue;
}
Expand Down
28 changes: 15 additions & 13 deletions wacore/src/send.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1090,21 +1090,23 @@ pub async fn prepare_group_stanza<
})
.collect();

// Determine what JID to check for - use phone number if we're in LID mode and have a mapping
let own_jid_to_check = if own_base_jid.is_lid() {
group_info
.phone_jid_for_lid_user(&own_base_jid.user)
.map(|pn| pn.to_non_ad())
.unwrap_or_else(|| own_base_jid.clone())
// Determine what user to check for — use the PN user when own is LID
// and we have a mapping. Keeping this as a borrow avoids allocating a
// throwaway Jid when own is already in the list.
let own_pn_mapping = if own_base_jid.is_lid() {
group_info.phone_jid_for_lid_user(&own_base_jid.user)
} else {
own_base_jid.clone()
None
};

if !jids_to_resolve
.iter()
.any(|participant| participant.is_same_user_as(&own_jid_to_check))
{
jids_to_resolve.push(own_jid_to_check);
let own_check_user = own_pn_mapping
.map(|pn| pn.user.as_str())
.unwrap_or(own_base_jid.user.as_str());

if !jids_to_resolve.iter().any(|p| p.user == own_check_user) {
jids_to_resolve.push(match own_pn_mapping {
Some(pn) => pn.to_non_ad(),
None => own_base_jid.clone(),
});
}

crate::types::jid::sort_dedup_by_user(&mut jids_to_resolve);
Expand Down
Loading