Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 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
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

96 changes: 94 additions & 2 deletions src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,11 @@ struct NodeWaiter {
tx: futures::channel::oneshot::Sender<Arc<Node>>,
}

struct SentNodeWaiter {
filter: NodeFilter,
tx: futures::channel::oneshot::Sender<Arc<Node>>,
}

use async_lock::Mutex;
use async_lock::RwLock;
use std::time::Duration;
Expand Down Expand Up @@ -282,6 +287,9 @@ pub struct Client {
/// Guarded by `node_waiter_count` for zero-cost when no waiters are active.
node_waiters: std::sync::Mutex<Vec<NodeWaiter>>,
node_waiter_count: AtomicUsize,
/// Waiters for raw outgoing nodes before encryption.
sent_node_waiters: std::sync::Mutex<Vec<SentNodeWaiter>>,
sent_node_waiter_count: AtomicUsize,
Comment on lines +290 to +292

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 | 🟡 Minor

Cancel pending sent-node waiters on disconnect.

These waiters are transport-scoped, but cleanup_connection_state() never drops them. If the socket dies before a match arrives, the receiver can stay pending across reconnects and then resolve against a later stanza from the wrong connection.

Suggested cleanup
 async fn cleanup_connection_state(&self) {
+    {
+        let mut waiters = self
+            .sent_node_waiters
+            .lock()
+            .unwrap_or_else(|poisoned| poisoned.into_inner());
+        if !waiters.is_empty() {
+            waiters.clear(); // dropping senders cancels receivers
+            self.sent_node_waiter_count.store(0, Ordering::Release);
+        }
+    }
+
     self.is_logged_in.store(false, Ordering::Relaxed);
     self.is_ready.store(false, Ordering::Relaxed);

Also applies to: 593-594, 2994-3011, 3037-3055, 3208-3210

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

In `@src/client.rs` around lines 290 - 292, cleanup_connection_state() currently
leaves transport-scoped SentNodeWaiter entries live; grab the sent_node_waiters
mutex inside cleanup_connection_state() and cancel/resolve all pending
SentNodeWaiter instances (and clear the Vec) so they cannot match against a
later connection, and decrement/reset sent_node_waiter_count accordingly; apply
the same pattern wherever connection teardown logic runs (areas referencing
sent_node_waiters / sent_node_waiter_count / SentNodeWaiter) to ensure each
waiter is notified/faulted and removed on disconnect.


pub(crate) unique_id: String,
pub(crate) id_counter: Arc<AtomicU64>,
Expand Down Expand Up @@ -582,6 +590,8 @@ impl Client {
response_waiters: Arc::new(Mutex::new(HashMap::new())),
node_waiters: std::sync::Mutex::new(Vec::new()),
node_waiter_count: AtomicUsize::new(0),
sent_node_waiters: std::sync::Mutex::new(Vec::new()),
sent_node_waiter_count: AtomicUsize::new(0),
unique_id: format!("{}.{}", unique_id_bytes[0], unique_id_bytes[1]),
id_counter: Arc::new(AtomicU64::new(0)),
unified_session: crate::unified_session::UnifiedSessionManager::new(),
Expand Down Expand Up @@ -1051,6 +1061,7 @@ impl Client {
}

async fn cleanup_connection_state(&self) {
self.clear_sent_node_waiters();
self.is_logged_in.store(false, Ordering::Relaxed);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
self.is_ready.store(false, Ordering::Relaxed);
// Signal the keepalive loop (and any other tasks) to exit promptly.
Expand Down Expand Up @@ -2619,10 +2630,38 @@ impl Client {
) {
use wacore::types::events::Event;

if m.operation != wa::syncd_mutation::SyncdOperation::Set {
if m.index.is_empty() {
return;
}
if m.index.is_empty() {

// NCT salt sync — handles both "set" (store salt) and "remove" (clear salt).
// Source: WAWebNctSaltSync, syncd collection RegularHigh, action "nct_salt_sync".
if m.index[0] == "nct_salt_sync" {
if m.operation == wa::syncd_mutation::SyncdOperation::Remove {
debug!(target: "Client/AppState", "Removing NCT salt via app state sync");
self.persistence_manager
.process_command(DeviceCommand::SetNctSalt(None))
.await;
} else if let Some(val) = &m.action_value
&& let Some(act) = &val.nct_salt_sync_action
&& let Some(salt) = &act.salt
{
if salt.is_empty() {
warn!(target: "Client/AppState", "nct_salt_sync mutation has empty salt, ignoring");
} else {
debug!(target: "Client/AppState", "Stored NCT salt via app state sync ({} bytes)", salt.len());
self.persistence_manager
.process_command(DeviceCommand::SetNctSalt(Some(salt.clone())))
.await;
}
} else {
warn!(target: "Client/AppState", "nct_salt_sync mutation missing salt in action value");
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
return;
}

// All remaining mutations only care about Set operations
if m.operation != wa::syncd_mutation::SyncdOperation::Set {
return;
}

Expand Down Expand Up @@ -2953,6 +2992,25 @@ impl Client {
rx
}

/// Register a waiter for an outgoing node before it is encrypted and sent.
///
/// This is intended for tests and diagnostics that need to inspect the raw
/// stanza built by the client, such as asserting whether `<tctoken>` or
/// `<cstoken>` was attached.
pub fn wait_for_sent_node(
&self,
filter: NodeFilter,
) -> futures::channel::oneshot::Receiver<Arc<Node>> {
let (tx, rx) = futures::channel::oneshot::channel();
self.sent_node_waiter_count.fetch_add(1, Ordering::Release);
let mut waiters = self
.sent_node_waiters
.lock()
.unwrap_or_else(|poisoned| poisoned.into_inner());
waiters.push(SentNodeWaiter { filter, tx });
Comment thread
coderabbitai[bot] marked this conversation as resolved.
rx
}

/// Check pending node waiters against an incoming node.
/// Only called when `node_waiter_count > 0`.
fn resolve_node_waiters(&self, node: &Arc<Node>) {
Expand All @@ -2977,6 +3035,37 @@ impl Client {
}
}

fn resolve_sent_node_waiters(&self, node: &Arc<Node>) {
let mut waiters = self
.sent_node_waiters
.lock()
.unwrap_or_else(|poisoned| poisoned.into_inner());
let mut i = 0;
while i < waiters.len() {
if waiters[i].tx.is_canceled() {
waiters.swap_remove(i);
self.sent_node_waiter_count.fetch_sub(1, Ordering::Release);
} else if waiters[i].filter.matches(node) {
let w = waiters.swap_remove(i);
self.sent_node_waiter_count.fetch_sub(1, Ordering::Release);
let _ = w.tx.send(Arc::clone(node));
} else {
i += 1;
}
}
}

fn clear_sent_node_waiters(&self) {
let mut waiters = self
.sent_node_waiters
.lock()
.unwrap_or_else(|poisoned| poisoned.into_inner());
if !waiters.is_empty() {
waiters.clear();
self.sent_node_waiter_count.store(0, Ordering::Release);
}
}

pub(crate) fn update_server_time_offset(&self, node: &wacore_binary::node::Node) {
self.unified_session.update_server_time_offset(node);
}
Expand Down Expand Up @@ -3128,6 +3217,9 @@ impl Client {
};

debug!(target: "Client/Send", "{}", DisplayableNode(&node));
if self.sent_node_waiter_count.load(Ordering::Acquire) > 0 {
self.resolve_sent_node_waiters(&Arc::new(node.clone()));
}

let mut plaintext_buf = Vec::with_capacity(1024);

Expand Down
39 changes: 37 additions & 2 deletions src/features/contacts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,35 @@ impl<'a> Contacts<'a> {
Self { client }
}

async fn persist_lid_mappings<'b, I>(&self, entries: I)
where
I: IntoIterator<Item = (&'b Jid, Option<&'b Jid>)>,
{
for (jid, lid) in entries {
let Some(lid) = lid else {
continue;
};
if !jid.is_pn() || !lid.is_lid() {
continue;
}
if let Err(err) = self
.client
.add_lid_pn_mapping(
&lid.user,
&jid.user,
crate::lid_pn_cache::LearningSource::Usync,
)
Comment on lines +36 to +45

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Enforce PN/LID validation in the persistence API, not only at this callsite.

This helper filters invalid pairs, but other write paths (src/usync.rs:38-46, src/pair.rs:250-256) persist without the same guard. Move/duplicate this invariant into Client::add_lid_pn_mapping (or a single shared validator) so cache integrity is consistent regardless of caller.

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

In `@src/features/contacts.rs` around lines 36 - 45, The code currently filters
invalid jid/lid pairs at the callsite, but the invariant should live inside the
persistence API so all writers are protected; add validation logic at the start
of Client::add_lid_pn_mapping (or implement a shared validator function used by
that method) to assert/judge that the provided jid.is_pn() and lid.is_lid() are
true and return a descriptive error if not, and update callers (e.g., sites that
call add_lid_pn_mapping from usync and pair paths) to handle the returned error
instead of relying on local guards.

.await
{
log::warn!(
"Failed to persist usync LID mapping {} -> {}: {err}",
jid,
lid
);
Comment on lines +48 to +52

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

Redact JIDs in warning logs to avoid identifier leakage.

Line 49 logs full jid/lid values on failure. These identifiers can contain user-linked data and should not be emitted in plain logs.

Suggested fix
-                log::warn!(
-                    "Failed to persist usync LID mapping {} -> {}: {err}",
-                    jid,
-                    lid
-                );
+                log::warn!("Failed to persist usync LID mapping (jid/lid redacted): {err}");
📝 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
log::warn!(
"Failed to persist usync LID mapping {} -> {}: {err}",
jid,
lid
);
log::warn!("Failed to persist usync LID mapping (jid/lid redacted): {err}");
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/features/contacts.rs` around lines 48 - 52, The log::warn! call emitting
"jid" and "lid" in src/features/contacts.rs should not print raw identifiers;
replace the direct jid and lid interpolation with redacted or hashed
representations (e.g., call a helper like redact_jid(jid) and redact_id(lid) or
compute a short hash) and use those redacted values in the log::warn! invocation
so the warning still conveys context without leaking user-identifying data.

}
}
}

pub async fn is_on_whatsapp(&self, phones: &[&str]) -> Result<Vec<IsOnWhatsAppResult>> {
if phones.is_empty() {
return Ok(Vec::new());
Expand All @@ -50,7 +79,10 @@ impl<'a> Contacts<'a> {
let phone_strings: Vec<String> = phones.iter().map(|s| s.to_string()).collect();
let spec = ContactInfoSpec::new(phone_strings, request_id);

Ok(self.client.execute(spec).await?)
let info = self.client.execute(spec).await?;
self.persist_lid_mappings(info.iter().map(|entry| (&entry.jid, entry.lid.as_ref())))
.await;
Ok(info)
}

pub async fn get_profile_picture(
Expand Down Expand Up @@ -98,7 +130,10 @@ impl<'a> Contacts<'a> {
let request_id = self.client.generate_request_id();
let spec = UserInfoSpec::new(jids.to_vec(), request_id);

Ok(self.client.execute(spec).await?)
let info = self.client.execute(spec).await?;
self.persist_lid_mappings(info.values().map(|entry| (&entry.jid, entry.lid.as_ref())))
.await;
Ok(info)
}
}

Expand Down
14 changes: 14 additions & 0 deletions src/history_sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,20 @@ impl Client {
log::info!("Updating own push name from history sync to '{new_name}'");
self.update_push_name_and_notify(new_name).await;
}

// Store NCT salt if found.
// WA Web: storeNctSaltFromHistorySync in MsgHandlerAction.js
if let Some(salt) = sync_result.nct_salt {
log::info!(
"History sync provided NCT salt ({} bytes); applying as backfill only",
salt.len()
);
self.persistence_manager
.process_command(
wacore::store::commands::DeviceCommand::SetNctSaltFromHistorySync(salt),
)
.await;
}
}
Some(Err(e)) => {
log::error!("Failed to process HistorySync data: {:?}", e);
Expand Down
Loading
Loading