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
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.

103 changes: 101 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,12 @@ impl Client {
}

async fn cleanup_connection_state(&self) {
// Note: node_waiters are intentionally NOT cleared here — they are
// cross-connection (callers may register a waiter before an action that
// completes on a subsequent connection, e.g. after 515 reconnect).
// sent_node_waiters ARE cleared because they match pre-encryption
// outgoing stanzas, which are transport-scoped.
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 +2635,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 +2997,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 +3040,39 @@ 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());
let count = waiters.len();
if count > 0 {
waiters.clear();
self.sent_node_waiter_count
.fetch_sub(count, 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 +3224,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