From 7bf9dae72f1a1c8dd3645fbccb820c15764b2b36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= <55464917+jlucaso1@users.noreply.github.com> Date: Tue, 21 Jul 2026 20:38:52 -0300 Subject: [PATCH 1/8] feat(core): expose typed stanza responses and retries --- src/client.rs | 68 ++-- src/client/node_io.rs | 45 ++- src/client/tests.rs | 120 +++++- src/features/mod.rs | 5 + src/features/stanza.rs | 203 +++++++++++ src/lib.rs | 11 +- src/message.rs | 4 +- src/message/retry.rs | 152 +++++--- src/message/tests.rs | 609 ++++++++++++++++++++++++++++++- src/portable_cache.rs | 81 ++++ src/prekeys.rs | 9 +- src/receipt.rs | 337 +++++++++++++++-- src/retry.rs | 258 +++---------- tests/e2e/tests/app_state.rs | 2 + tests/e2e/tests/chat_actions.rs | 2 + tests/e2e/tests/memory_soak.rs | 9 + tests/e2e/tests/messaging.rs | 2 + tests/e2e/tests/profile.rs | 2 + wacore/binary/src/jid.rs | 62 ++++ wacore/binary/src/node.rs | 79 ++-- wacore/src/message_processing.rs | 3 +- wacore/src/protocol/nack.rs | 13 +- wacore/src/protocol/retry.rs | 60 ++- 23 files changed, 1720 insertions(+), 416 deletions(-) create mode 100644 src/features/stanza.rs diff --git a/src/client.rs b/src/client.rs index 3562d9ab5..d8d136367 100644 --- a/src/client.rs +++ b/src/client.rs @@ -1044,6 +1044,22 @@ fn build_pong(to: String, id: Option<&str>) -> wacore_binary::Node { builder.build() } +/// Compare decoded attribute values by their wire display without allocating. +#[inline] +fn value_refs_display_equal( + left: &wacore_binary::node::ValueRef<'_>, + right: &wacore_binary::node::ValueRef<'_>, +) -> bool { + use wacore_binary::node::ValueRef; + + match (left, right) { + (ValueRef::String(left), ValueRef::String(right)) => left == right, + (ValueRef::Jid(left), ValueRef::Jid(right)) => left == right, + (ValueRef::String(left), ValueRef::Jid(right)) => right.display_eq(left), + (ValueRef::Jid(left), ValueRef::String(right)) => left.display_eq(right), + } +} + /// Build an `` for the given stanza, matching WA Web / whatsmeow behavior: /// /// - `class` = original stanza tag @@ -1056,32 +1072,37 @@ fn build_pong(to: String, id: Option<&str>) -> wacore_binary::Node { /// `ackString = maybeAttrString("type")` — so `type` is only included when /// explicitly present on the incoming receipt (delivery receipts normally /// have no type attribute, meaning the ack also has no type). +/// /// Encode an ack stanza directly to bytes, bypassing Node + marshal_auto. /// Acks are the most frequent outbound stanza (~1 per inbound message). fn encode_ack_bytes( node: &wacore_binary::NodeRef<'_>, own_device_pn: Option<&Jid>, -) -> Result>, wacore_binary::error::BinaryError> { +) -> Result, crate::features::StanzaResponseError> { use wacore_binary::encoder::{ByteWriter, EncodeNode, Encoder}; - let Some(id_val) = node.get_attr("id") else { - return Ok(None); - }; - let Some(from_val) = node.get_attr("from") else { - return Ok(None); - }; + let id_val = node + .get_attr("id") + .ok_or(crate::features::StanzaResponseError::MissingAttribute("id"))?; + let from_val = + node.get_attr("from") + .ok_or(crate::features::StanzaResponseError::MissingAttribute( + "from", + ))?; + let tag = node.tag.as_ref(); // WAWebReceiptAck: `participant: r && r !== e ? DEVICE_JID(r) : DROP_ATTR`. - // Drop the attribute when it would duplicate `to` (which is the flipped `from`). - let participant_val = node.get_attr("participant").filter(|p| { - let p_str = p.as_str(); - let from_str = from_val.as_str(); - p_str.as_ref() != from_str.as_ref() + // This is specific to the specialized receipt ACK. Generic ACKs and NACKs + // preserve participant even when it duplicates the destination. + let participant_val = node.get_attr("participant").filter(|participant| { + if tag != "receipt" { + return true; + } + !value_refs_display_equal(participant, from_val) }); // Server expects `recipient` echoed back so it can route the ack to the // origin companion/device (hosted-companion, peer, LID-routed stanzas). // Dropping it makes the server close the stream with ``. let recipient_val = node.get_attr("recipient"); - let tag = node.tag.as_ref(); let typ_val = if tag != "message" && !is_encrypt_identity_notification(node) { node.get_attr("type") @@ -1089,11 +1110,15 @@ fn encode_ack_bytes( None }; - let include_from = tag == "message" && own_device_pn.is_some(); + let own_device_pn = if tag == "message" { + Some(own_device_pn.ok_or(crate::features::StanzaResponseError::MissingLocalIdentity)?) + } else { + None + }; // Count attrs: class + id + to + optional(from, participant, recipient, type) let attr_count = 3 - + usize::from(include_from) + + usize::from(own_device_pn.is_some()) + usize::from(participant_val.is_some()) + usize::from(recipient_val.is_some()) + usize::from(typ_val.is_some()); @@ -1161,7 +1186,7 @@ fn encode_ack_bytes( participant: participant_val, recipient: recipient_val, typ: typ_val, - own_pn: if include_from { own_device_pn } else { None }, + own_pn: own_device_pn, tag_str: tag, attr_count, }; @@ -1169,7 +1194,7 @@ fn encode_ack_bytes( let mut buf = Vec::with_capacity(64); let mut encoder = Encoder::new_vec(&mut buf)?; encoder.write_node(&ack)?; - Ok(Some(buf)) + Ok(buf) } /// Minimal `` stanza carrying the attrs `encode_ack_bytes` needs, @@ -1201,13 +1226,14 @@ fn build_ack_node(node: &wacore_binary::NodeRef<'_>, own_device_pn: Option<&Jid> let id = node.get_attr("id")?.to_node_value(); let from_ref = node.get_attr("from")?; let from = from_ref.to_node_value(); - // Drop participant when it duplicates `to` (the flipped `from`). + let tag = node.tag.as_ref(); + // Only the specialized receipt ACK drops a participant that duplicates + // `to` (the flipped `from`). let participant = node .get_attr("participant") - .filter(|p| p.as_str().as_ref() != from_ref.as_str().as_ref()) + .filter(|participant| tag != "receipt" || !value_refs_display_equal(participant, from_ref)) .map(|v| v.to_node_value()); let recipient = node.get_attr("recipient").map(|v| v.to_node_value()); - let tag = node.tag.as_ref(); let typ = if tag != "message" && !is_encrypt_identity_notification(node) { node.get_attr("type").map(|v| v.to_node_value()) } else { @@ -1243,7 +1269,7 @@ fn is_encrypt_identity_notification(node: &wacore_binary::NodeRef<'_>) -> bool { node.tag == "notification" && node .get_attr("type") - .is_some_and(|v| v.as_str() == "encrypt") + .is_some_and(|value| value == "encrypt") && node.get_optional_child("identity").is_some() } diff --git a/src/client/node_io.rs b/src/client/node_io.rs index db6090a2e..62f658d7f 100644 --- a/src/client/node_io.rs +++ b/src/client/node_io.rs @@ -604,33 +604,54 @@ impl Client { if !self.is_connected() { return Err(ClientError::NotConnected); } - let own_pn = self.get_pn(); - let buf = match encode_ack_bytes(node, own_pn.as_ref()) { - Ok(Some(buf)) => buf, - Ok(None) => return Ok(()), + let device = self.persistence_manager.get_device_snapshot(); + let buf = match encode_ack_bytes(node, device.pn.as_ref()) { + Ok(buf) => buf, Err(e) => { log::warn!("Failed to encode ack: {e}"); return Ok(()); } }; + drop(device); self.send_raw_bytes(buf).await } + /// Confirm a received stanza using its original borrowed node. + /// + /// Unlike the tolerant automatic receive path, malformed input is returned + /// to the caller and no successful outcome is reported unless the response + /// reaches the transport. + #[cfg_attr( + feature = "tracing", + tracing::instrument(name = "wa.conn.ack_explicit", level = "debug", skip_all, err(Debug)) + )] + pub async fn acknowledge_stanza( + &self, + stanza: &wacore_binary::NodeRef<'_>, + ) -> Result<(), crate::features::StanzaResponseError> { + let device = self.persistence_manager.get_device_snapshot(); + let bytes = encode_ack_bytes(stanza, device.pn.as_ref())?; + drop(device); + self.send_raw_bytes(bytes).await?; + Ok(()) + } + /// Send a transport ack so the server stops replaying a stanza from the /// offline queue. Awaitable so callers can order it after a retry receipt /// in a single flushed task. pub(crate) async fn send_transport_ack(&self, info: &crate::types::message::MessageInfo) { let source = message_ack_source_node(info); - let own_pn = self.get_pn(); - match encode_ack_bytes(&source.as_node_ref(), own_pn.as_ref()) { - Ok(Some(buf)) => { + let device = self.persistence_manager.get_device_snapshot(); + let encoded = encode_ack_bytes(&source.as_node_ref(), device.pn.as_ref()); + drop(device); + match encoded { + Ok(buf) => { if let Err(e) = self.send_raw_bytes(buf).await && !e.is_transport_unavailable() { log::warn!("Failed to send transport ack for undecryptable message: {e:?}"); } } - Ok(None) => {} Err(e) => log::warn!("Failed to encode transport ack: {e}"), } } @@ -655,15 +676,15 @@ impl Client { self: &Arc, node: &wacore_binary::NodeRef<'_>, ) { - let own_pn = self.get_pn(); - let buf = match encode_ack_bytes(node, own_pn.as_ref()) { - Ok(Some(b)) => b, - Ok(None) => return, + let device = self.persistence_manager.get_device_snapshot(); + let buf = match encode_ack_bytes(node, device.pn.as_ref()) { + Ok(buf) => buf, Err(e) => { log::warn!("Failed to encode node transport ack: {e}"); return; } }; + drop(device); let client = Arc::clone(self); self.outbound_flush.spawn(&*self.runtime, async move { if let Err(e) = client.send_raw_bytes(buf).await diff --git a/src/client/tests.rs b/src/client/tests.rs index 2098a64cc..501be7d78 100644 --- a/src/client/tests.rs +++ b/src/client/tests.rs @@ -2333,7 +2333,6 @@ fn test_encode_ack_bytes_roundtrip_recipient() { .attr("recipient", "146991363395800@lid") .build(); let buf = encode_ack_bytes(&with_recipient.as_node_ref(), Some(&own_device_pn)) - .expect("encode_ack_bytes should not error") .expect("encode_ack_bytes should produce bytes"); // The Encoder prepends a leading format byte (see `marshal`); the // decoder wants raw protocol bytes — same handling as `node_to_owned_ref`. @@ -2360,7 +2359,6 @@ fn test_encode_ack_bytes_roundtrip_recipient() { .attr("participant", "181531758878822@lid") .build(); let buf = encode_ack_bytes(&without_recipient.as_node_ref(), Some(&own_device_pn)) - .expect("encode_ack_bytes should not error") .expect("encode_ack_bytes should produce bytes"); let decoded = wacore_binary::marshal::unmarshal_ref(&buf[1..]).expect("encoded ack should decode"); @@ -2370,6 +2368,124 @@ fn test_encode_ack_bytes_roundtrip_recipient() { ); } +#[test] +fn test_encode_ack_bytes_requires_public_response_inputs() { + let without_id = NodeBuilder::new("receipt") + .attr("from", "15551234567@s.whatsapp.net") + .build(); + assert!(matches!( + encode_ack_bytes(&without_id.as_node_ref(), None), + Err(crate::features::StanzaResponseError::MissingAttribute("id")) + )); + + let without_from = NodeBuilder::new("receipt") + .attr("id", "MISSING-FROM") + .build(); + assert!(matches!( + encode_ack_bytes(&without_from.as_node_ref(), None), + Err(crate::features::StanzaResponseError::MissingAttribute( + "from" + )) + )); + + let message = NodeBuilder::new("message") + .attr("id", "MISSING-IDENTITY") + .attr("from", "15551234567@s.whatsapp.net") + .build(); + assert!(matches!( + encode_ack_bytes(&message.as_node_ref(), None), + Err(crate::features::StanzaResponseError::MissingLocalIdentity) + )); +} + +#[test] +fn test_encode_ack_bytes_preserves_specialized_receipt_rules() { + let from: Jid = "15551234567@s.whatsapp.net".parse().unwrap(); + let receipt = NodeBuilder::new("receipt") + .attr("id", "RECEIPT-ACK") + .attr("from", &from) + .attr("participant", "15551234567@s.whatsapp.net") + .attr("type", "retry") + .build(); + let bytes = encode_ack_bytes(&receipt.as_node_ref(), None) + .expect("complete receipt should produce an ack"); + let ack = wacore_binary::marshal::unmarshal_ref(&bytes[1..]) + .expect("encoded receipt ack should decode"); + + assert!( + ack.get_attr("class") + .is_some_and(|value| value.as_str() == "receipt") + ); + assert!( + ack.get_attr("type") + .is_some_and(|value| value.as_str() == "retry") + ); + assert!( + ack.get_attr("participant").is_none(), + "receipt ack must omit a participant that duplicates its destination" + ); + assert!(ack.get_attr("from").is_none()); + + let generic = NodeBuilder::new("message") + .attr("id", "MESSAGE-ACK") + .attr("from", "15551234567@s.whatsapp.net") + .attr("participant", &from) + .build(); + let bytes = encode_ack_bytes(&generic.as_node_ref(), Some(&from)) + .expect("complete message should produce an ack"); + let ack = wacore_binary::marshal::unmarshal_ref(&bytes[1..]) + .expect("encoded message ack should decode"); + assert!( + ack.get_attr("participant") + .is_some_and(|value| value.as_str() == "15551234567@s.whatsapp.net"), + "generic ack must not inherit the receipt-only participant rule" + ); +} + +#[test] +fn test_encode_ack_bytes_drops_encrypt_identity_notification_type() { + let notification = NodeBuilder::new("notification") + .attr("id", "IDENTITY-NOTIFICATION") + .attr("from", "15551234567@s.whatsapp.net") + .attr("type", "encrypt") + .children([NodeBuilder::new("identity").build()]) + .build(); + let bytes = encode_ack_bytes(¬ification.as_node_ref(), None) + .expect("complete notification should produce an ack"); + let ack = wacore_binary::marshal::unmarshal_ref(&bytes[1..]) + .expect("encoded notification ack should decode"); + + assert!( + ack.get_attr("class") + .is_some_and(|value| value.as_str() == "notification") + ); + assert!(ack.get_attr("type").is_none()); + assert!(ack.get_attr("from").is_none()); +} + +#[test] +fn test_encode_ack_bytes_preserves_call_class_and_type() { + let call = NodeBuilder::new("call") + .attr("id", "CALL-ACK") + .attr("from", "15551234567@s.whatsapp.net") + .attr("type", "offer_notice") + .build(); + let bytes = + encode_ack_bytes(&call.as_node_ref(), None).expect("complete call should produce an ack"); + let ack = + wacore_binary::marshal::unmarshal_ref(&bytes[1..]).expect("encoded call ack should decode"); + + assert!( + ack.get_attr("class") + .is_some_and(|value| value.as_str() == "call") + ); + assert!( + ack.get_attr("type") + .is_some_and(|value| value.as_str() == "offer_notice") + ); + assert!(ack.get_attr("from").is_none()); +} + /// Own-account fan-out ack must address back to the original `from` (own /// LID) echoing `recipient`, not to the chat. Guards against regressing to /// the chat-addressed `build_nack_node` style. diff --git a/src/features/mod.rs b/src/features/mod.rs index 42b79ff06..b49385f18 100644 --- a/src/features/mod.rs +++ b/src/features/mod.rs @@ -17,6 +17,7 @@ mod profile; mod reaction; mod rotate_key; mod signal; +mod stanza; pub(crate) mod status; mod tctoken; @@ -77,6 +78,10 @@ pub use profile::{Profile, ProfileError, SetProfilePictureResponse}; pub use status::{Status, StatusPrivacySetting, StatusSendOptions}; pub use signal::{Signal, SignalError, SignalSessionInfo, SignalSessionMigration}; +pub use stanza::{ + NackReason, RetryReason, RetryRequestError, RetryRequestOptions, RetryRequestOutcome, + StanzaRejection, StanzaResponseError, +}; pub use wacore::message_processing::EncType; pub use tctoken::{TcToken, TcTokenError}; diff --git a/src/features/stanza.rs b/src/features/stanza.rs new file mode 100644 index 000000000..fcbee608e --- /dev/null +++ b/src/features/stanza.rs @@ -0,0 +1,203 @@ +//! Typed operations for responding to inbound protocol stanzas. + +use thiserror::Error; + +use crate::client::ClientError; + +pub use wacore::protocol::nack::NackReason; +pub use wacore::protocol::retry::RetryReason; + +/// A protocol rejection sent as an `` stanza. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct StanzaRejection { + reason: NackReason, + failure_reason: Option, +} + +impl StanzaRejection { + /// Reject with a protocol reason and no protobuf failure detail. + pub const fn new(reason: NackReason) -> Self { + Self { + reason, + failure_reason: None, + } + } + + /// Reject malformed protobuf content, optionally attaching its typed failure detail. + pub const fn invalid_protobuf(failure_reason: Option) -> Self { + Self { + reason: NackReason::InvalidProtobuf, + failure_reason, + } + } + + /// Protocol reason encoded in the rejection. + pub const fn reason(self) -> NackReason { + self.reason + } + + /// Optional protobuf failure detail, present only for `InvalidProtobuf`. + pub const fn failure_reason(self) -> Option { + self.failure_reason + } +} + +impl From for StanzaRejection { + fn from(reason: NackReason) -> Self { + Self::new(reason) + } +} + +/// Failure while confirming or rejecting an inbound stanza. +#[derive(Debug, Error)] +#[non_exhaustive] +pub enum StanzaResponseError { + #[error("stanza is missing required '{0}' attribute")] + MissingAttribute(&'static str), + #[error("the local device identity is unavailable")] + MissingLocalIdentity, + #[error("the stanza class does not support this response")] + UnsupportedStanzaClass, + #[error("failed to encode stanza response")] + Encoding(#[from] wacore_binary::error::BinaryError), + #[error(transparent)] + Client(#[from] ClientError), +} + +/// Options for requesting retransmission of one inbound message stanza. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[non_exhaustive] +pub struct RetryRequestOptions { + reason: RetryReason, + force_include_keys: bool, +} + +impl RetryRequestOptions { + /// Use the default retry reason without forcing a key bundle. + pub const fn new() -> Self { + Self { + reason: RetryReason::UnknownError, + force_include_keys: false, + } + } + + /// Attach the diagnostic reason sent to the original sender. + pub const fn with_reason(mut self, reason: RetryReason) -> Self { + self.reason = reason; + self + } + + /// Require key material even before the normal retry threshold. + pub const fn with_force_include_keys(mut self, force_include_keys: bool) -> Self { + self.force_include_keys = force_include_keys; + self + } + + /// The diagnostic reason sent with this request. + pub const fn reason(self) -> RetryReason { + self.reason + } + + /// Whether key material is required before the normal retry threshold. + pub const fn force_include_keys(self) -> bool { + self.force_include_keys + } +} + +impl Default for RetryRequestOptions { + fn default() -> Self { + Self::new() + } +} + +/// Result of a retransmission request. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[non_exhaustive] +pub enum RetryRequestOutcome { + /// A retry receipt reached the transport. + Sent { + /// Shared attempt count encoded in the retry receipt. + retry_count: u8, + /// Whether this attempt carried the local key bundle. + included_keys: bool, + }, + /// The protocol excludes this sender/chat combination from retry receipts. + Suppressed { + /// Shared attempt count consumed by the suppressed request. + retry_count: u8, + }, + /// The shared retry counter had already reached its configured limit. + LimitReached, +} + +/// Failure while parsing or sending a retransmission request. +#[derive(Debug, Error)] +#[non_exhaustive] +pub enum RetryRequestError { + #[error("retry requests require a message stanza")] + UnsupportedStanzaClass, + #[error("stanza is missing required '{0}' attribute")] + MissingAttribute(&'static str), + #[error("the local device identity is unavailable")] + MissingLocalIdentity, + #[error("invalid message stanza")] + InvalidStanza(#[source] anyhow::Error), + #[error(transparent)] + Client(#[from] ClientError), + #[error("failed to prepare retry request")] + Internal(#[from] anyhow::Error), +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn invalid_protobuf_is_the_only_rejection_with_failure_detail() { + let rejection = StanzaRejection::invalid_protobuf(Some(17)); + assert_eq!(rejection.reason(), NackReason::InvalidProtobuf); + assert_eq!(rejection.failure_reason(), Some(17)); + + let rejection = StanzaRejection::new(NackReason::ParsingError); + assert_eq!(rejection.reason(), NackReason::ParsingError); + assert_eq!(rejection.failure_reason(), None); + } + + #[test] + fn retry_request_options_have_protocol_safe_defaults() { + let defaults = RetryRequestOptions::default(); + assert_eq!(defaults.reason(), RetryReason::UnknownError); + assert!(!defaults.force_include_keys()); + + let configured = defaults + .with_reason(RetryReason::BadMac) + .with_force_include_keys(true); + assert_eq!(configured.reason(), RetryReason::BadMac); + assert!(configured.force_include_keys()); + } + + #[test] + fn internal_retry_error_preserves_its_source() { + use std::error::Error as _; + + let error = RetryRequestError::from(anyhow::anyhow!("storage sentinel")); + assert_eq!( + error.source().map(ToString::to_string).as_deref(), + Some("storage sentinel") + ); + } + + #[test] + fn response_encoding_error_preserves_its_source() { + use std::error::Error as _; + + let error = StanzaResponseError::from(wacore_binary::error::BinaryError::MissingAttr( + "sentinel".to_owned(), + )); + assert!( + error + .source() + .is_some_and(|source| source.to_string().contains("sentinel")) + ); + } +} diff --git a/src/lib.rs b/src/lib.rs index 95de556bc..3e733ccb6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -151,15 +151,16 @@ pub use features::{ InviteInfoError, IsOnWhatsAppResult, JoinGroupResult, Labels, LinkSubgroupsResult, MediaRetryResult, MediaReupload, MediaReuploadError, MediaReuploadRequest, MemberAddMode, MemberLinkMode, MemberShareHistoryMode, MembershipApprovalMode, MembershipRequest, Mex, - MexError, MexErrorExtensions, MexRequest, MexResponse, Newsletter, NewsletterError, + MexError, MexErrorExtensions, MexRequest, MexResponse, NackReason, Newsletter, NewsletterError, NewsletterMessage, NewsletterMessageType, NewsletterMetadata, NewsletterReactionCount, NewsletterRole, NewsletterState, NewsletterVerification, ParticipantChangeResponse, ParticipantType, PictureType, PollError, Presence, PresenceError, PresenceStatus, Profile, - ProfileError, ProfilePicture, ReachoutTimelock, SecretEncKind, SecretEncrypted, + ProfileError, ProfilePicture, ReachoutTimelock, RetryReason, RetryRequestError, + RetryRequestOptions, RetryRequestOutcome, SecretEncKind, SecretEncrypted, SetProfilePictureResponse, Signal, SignalError, SignalSessionInfo, SignalSessionMigration, - Status, StatusPrivacySetting, StatusSendOptions, SyncActionMessageRange, TcToken, TcTokenError, - UnlinkSubgroupsResult, UserInfo, UsyncSubprotocolError, VerifiedName, group_type, message_key, - message_range, + StanzaRejection, StanzaResponseError, Status, StatusPrivacySetting, StatusSendOptions, + SyncActionMessageRange, TcToken, TcTokenError, UnlinkSubgroupsResult, UserInfo, + UsyncSubprotocolError, VerifiedName, group_type, message_key, message_range, }; pub mod bot; diff --git a/src/message.rs b/src/message.rs index 396396cc3..58d63dbef 100644 --- a/src/message.rs +++ b/src/message.rs @@ -20,9 +20,7 @@ use wacore_binary::JidExt as _; use wacore_binary::{NodeRef, OwnedNodeRef}; use waproto::whatsapp::{self as wa}; -/// Maximum retry attempts per message (matches WhatsApp Web's MAX_RETRY = 5). -/// After this many retries, we stop sending retry receipts and rely solely on PDO. -const MAX_DECRYPT_RETRIES: u8 = 5; +use wacore::protocol::retry::MAX_RETRY_COUNT as MAX_DECRYPT_RETRIES; /// Pre-extracted enc node payload. Holds owned copies of the fields needed for /// decryption so the async decrypt phase doesn't borrow the original NodeRef tree. diff --git a/src/message/retry.rs b/src/message/retry.rs index 687238b64..43b55b455 100644 --- a/src/message/retry.rs +++ b/src/message/retry.rs @@ -3,6 +3,43 @@ use super::*; impl Client { + /// Request retransmission of an inbound message stanza. + /// + /// The stanza is parsed once into the canonical message metadata model. + /// This operation sends only the retry receipt; transport acknowledgement + /// remains the caller's responsibility. + #[cfg_attr( + feature = "tracing", + tracing::instrument(name = "wa.recv.request_retry", level = "debug", skip_all, err(Debug)) + )] + pub async fn request_message_retry( + self: &Arc, + stanza: &NodeRef<'_>, + options: crate::features::RetryRequestOptions, + ) -> Result { + if stanza.tag.as_ref() != "message" { + return Err(crate::features::RetryRequestError::UnsupportedStanzaClass); + } + if stanza.get_attr("id").is_none() { + return Err(crate::features::RetryRequestError::MissingAttribute("id")); + } + if stanza.get_attr("from").is_none() { + return Err(crate::features::RetryRequestError::MissingAttribute("from")); + } + + let device = self.persistence_manager.get_device_snapshot(); + let own_pn = device + .pn + .as_ref() + .ok_or(crate::features::RetryRequestError::MissingLocalIdentity)?; + let info = wacore::messages::parse_message_info(stanza, own_pn, device.lid.as_ref()) + .map_err(crate::features::RetryRequestError::InvalidStanza)?; + let info = Arc::new(info); + drop(device); + + self.request_retry_for_info(&info, options).await + } + /// Dispatch an `UndecryptableMessage` event at most once per `(chat, id)` /// via the single-flight `get_with` semantic on `undecryptable_dispatched`. /// The atomic arm avoids the get-then-insert race where two concurrent @@ -125,26 +162,21 @@ impl Client { /// Increments the retry count for a message and returns the new count. /// Returns `None` if max retries have been reached. /// - /// Note: get-then-insert has a theoretical TOCTOU window since - /// `spawn_retry_receipt` detaches. In practice, retries for the same - /// message are rare and a double-send is benign (recipients deduplicate - /// by message ID). pub(crate) async fn increment_retry_count( &self, cache_key: &str, reason: RetryReason, ) -> Option { - let cache_key = cache_key.to_owned(); - let current = self.message_retry_counts.get(&cache_key).await; - let new_count = match current { - Some((count, _)) if count >= MAX_DECRYPT_RETRIES => return None, - Some((count, _)) => count + 1, - None => 1, - }; self.message_retry_counts - .insert(cache_key, (new_count, Some(reason))) - .await; - Some(new_count) + .upsert_with_by_ref(cache_key, |current| { + let count = match current { + Some((count, _)) if *count >= MAX_DECRYPT_RETRIES => return (None, None), + Some((count, _)) => *count + 1, + None => 1, + }; + (Some((count, Some(reason))), Some(count)) + }) + .await } /// Generate consistent cache key for retry logic. @@ -208,26 +240,19 @@ impl Client { } /// Increment the retry count and send the retry receipt (or, at the cap, a - /// last-resort PDO). Awaitable so it can be ordered before the transport ack. - /// - /// Returns whether the caller should send the ack: `false` when we intended - /// to retry but the send failed (so the stanza stays queued for another try), - /// `true` when the resend went out or we deliberately gave up at the cap. - #[cfg_attr(feature = "tracing", tracing::instrument(name = "wa.recv.retry_receipt", level = "debug", skip_all, fields(chat = %info.source.chat.observe(), sender = %info.source.sender.observe(), msg_id = %info.id, reason = ?reason)))] - async fn run_retry_receipt( + /// last-resort PDO). This is the shared operation used by explicit requests + /// and the automatic decrypt-failure pipeline. + async fn request_retry_for_info( self: &Arc, info: &Arc, - reason: RetryReason, - ) -> bool { + options: crate::features::RetryRequestOptions, + ) -> Result { + let reason = options.reason(); let cache_key = self .make_retry_cache_key(&info.source.chat, &info.id, &info.source.sender) .await; let Some(retry_count) = self.increment_retry_count(&cache_key, reason).await else { - // Every further redelivery of a capped message lands here, so - // keep it at debug; the high-retry warn already fired on the way - // to the cap, and the PDO is a once-per-message no-op after the - // first request. log::debug!( "Max retries ({}) reached for message {} from {} [{:?}]. Requesting PDO fallback.", MAX_DECRYPT_RETRIES, @@ -235,9 +260,8 @@ impl Client { info.source.sender.observe(), reason ); - // Capped: give up and clear the backlog regardless of PDO outcome. self.run_pdo_request(info).await; - return true; + return Ok(crate::features::RetryRequestOutcome::LimitReached); }; if retry_count > HIGH_RETRY_COUNT_THRESHOLD { @@ -250,14 +274,25 @@ impl Client { reason ); } - let retry_sent = match self.send_retry_receipt(info, retry_count, reason).await { - Ok(()) => { + + let send_result = self + .send_retry_receipt(info, retry_count, reason, options.force_include_keys()) + .await; + + // PDO is an independent first-attempt recovery path. Preserve it even + // when building or sending the retry receipt fails; the caller still + // receives that failure and the automatic pipeline still withholds its + // transport acknowledgement. + if retry_count == 1 { + self.run_pdo_request(info).await; + } + + let send_outcome = send_result?; + + let outcome = match send_outcome { + crate::retry::RetryReceiptSendOutcome::Sent { included_keys } => { wacore::telemetry::retry_receipt(reason.as_str()); if retry_count >= MAX_DECRYPT_RETRIES { - // Parity with WA Web's MessageHighRetryCount WAM event (id - // 3132): committed after the retry receipt is sent, not - // before — WAWebHandleMsgSendReceipt awaits sendRetryReceipt - // and only then calls maybePostMessageHighRetryCountMetric. wacore::telemetry::high_retry(reason.as_str()); } debug!( @@ -268,25 +303,46 @@ impl Client { info.source.sender.observe(), reason ); - true + crate::features::RetryRequestOutcome::Sent { + retry_count, + included_keys, + } } - Err(e) => { + crate::retry::RetryReceiptSendOutcome::Suppressed => { + crate::features::RetryRequestOutcome::Suppressed { retry_count } + } + }; + + Ok(outcome) + } + + /// Awaitable automatic wrapper so retry can be ordered before transport ack. + /// + /// Returns whether the caller should send the ack: `false` when we intended + /// to retry but the send failed (so the stanza stays queued for another try), + /// `true` when the resend went out or we deliberately gave up at the cap. + #[cfg_attr(feature = "tracing", tracing::instrument(name = "wa.recv.retry_receipt", level = "debug", skip_all, fields(chat = %info.source.chat.observe(), sender = %info.source.sender.observe(), msg_id = %info.id, reason = ?reason)))] + async fn run_retry_receipt( + self: &Arc, + info: &Arc, + reason: RetryReason, + ) -> bool { + match self + .request_retry_for_info( + info, + crate::features::RetryRequestOptions::new().with_reason(reason), + ) + .await + { + Ok(_) => true, + Err(error) => { log::error!( - "Failed to send retry receipt #{} for message {} [{:?}]: {:?}", - retry_count, + "Failed to send retry receipt for message {} [{:?}]: {error:?}", info.id, - reason, - e + reason ); false } - }; - - // First retry only, to avoid duplicate PDO requests. Awaited so it runs - // before the caller's ack; the retry receipt already landed first. - if retry_count == 1 { - self.run_pdo_request(info).await; } - retry_sent } } diff --git a/src/message/tests.rs b/src/message/tests.rs index efc8f71b0..4b941f21d 100644 --- a/src/message/tests.rs +++ b/src/message/tests.rs @@ -3602,10 +3602,6 @@ async fn test_retry_cache_key_format() { } /// Test concurrent retry increments are properly serialized. -/// -/// The increment operation uses get+insert which is not fully atomic, -/// but is sufficient since message retry processing is serialized per key -/// by the per-chat lock. At most 5 increments should succeed. #[tokio::test] async fn test_concurrent_retry_increments() { use tokio::task::JoinSet; @@ -5564,8 +5560,593 @@ async fn capturing_client( (client, transport) } +#[tokio::test] +async fn explicit_stanza_responses_use_the_canonical_wire_paths() { + let (client, transport) = capturing_client("explicit_stanza_responses").await; + + let message = NodeBuilder::new("message") + .attr("id", "EXPLICIT-ACK") + .attr("from", "15551234567:7@s.whatsapp.net") + .attr("recipient", "5511000000001@s.whatsapp.net") + .attr("type", "text") + .build(); + client + .acknowledge_stanza(&message.as_node_ref()) + .await + .expect("complete message should be acknowledged"); + + let receipt = NodeBuilder::new("receipt") + .attr("id", "EXPLICIT-NACK") + .attr("from", "120363021033254949@g.us") + .attr("participant", "15551234567:7@s.whatsapp.net") + .attr("type", "retry") + .build(); + client + .reject_stanza( + &receipt.as_node_ref(), + crate::features::StanzaRejection::invalid_protobuf(Some(42)), + ) + .await + .expect("complete receipt should be rejected"); + + let frames = transport.sent(); + assert_eq!(frames.len(), 2); + + let ack_bytes = decode_frame(0, &frames[0]).expect("ack frame should decrypt"); + let ack = + wacore_binary::marshal::unmarshal_ref(&ack_bytes[1..]).expect("ack frame should decode"); + assert_eq!(ack.tag.as_ref(), "ack"); + assert!( + ack.get_attr("id") + .is_some_and(|value| value.as_str() == "EXPLICIT-ACK") + ); + assert!( + ack.get_attr("class") + .is_some_and(|value| value.as_str() == "message") + ); + assert!( + ack.get_attr("to") + .is_some_and(|value| value.as_str() == "15551234567:7@s.whatsapp.net") + ); + assert!( + ack.get_attr("from") + .is_some_and(|value| value.as_str() == "5511000000001@s.whatsapp.net") + ); + assert!(ack.get_attr("type").is_none()); + + let nack_bytes = decode_frame(1, &frames[1]).expect("nack frame should decrypt"); + let nack = + wacore_binary::marshal::unmarshal_ref(&nack_bytes[1..]).expect("nack frame should decode"); + assert_eq!(nack.tag.as_ref(), "ack"); + assert!( + nack.get_attr("id") + .is_some_and(|value| value.as_str() == "EXPLICIT-NACK") + ); + assert!( + nack.get_attr("class") + .is_some_and(|value| value.as_str() == "receipt") + ); + assert!( + nack.get_attr("error") + .is_some_and(|value| value.as_str() == "491") + ); + assert!( + nack.get_attr("participant") + .is_some_and(|value| value.as_str() == "15551234567:7@s.whatsapp.net") + ); + let meta = nack + .get_optional_child("meta") + .expect("InvalidProtobuf should carry its failure detail"); + assert!( + meta.get_attr("failure_reason") + .is_some_and(|value| value.as_str() == "42") + ); +} + +#[tokio::test] +async fn explicit_stanza_responses_reject_incomplete_input_without_sending() { + use std::error::Error as _; + + let (client, transport) = capturing_client("explicit_stanza_invalid").await; + + let ack_without_id = NodeBuilder::new("receipt") + .attr("from", "15551234567@s.whatsapp.net") + .build(); + assert!(matches!( + client + .acknowledge_stanza(&ack_without_id.as_node_ref()) + .await, + Err(crate::features::StanzaResponseError::MissingAttribute("id")) + )); + + let nack_without_from = NodeBuilder::new("message").attr("id", "NO-FROM").build(); + assert!(matches!( + client + .reject_stanza( + &nack_without_from.as_node_ref(), + crate::features::StanzaRejection::new(NackReason::ParsingError), + ) + .await, + Err(crate::features::StanzaResponseError::MissingAttribute( + "from" + )) + )); + + assert_eq!(transport.sent_count(), 0); + + transport.fail_next_sends(1); + let valid_receipt = NodeBuilder::new("receipt") + .attr("id", "TRANSPORT-FAILURE") + .attr("from", "15551234567@s.whatsapp.net") + .build(); + let error = client + .acknowledge_stanza(&valid_receipt.as_node_ref()) + .await + .expect_err("transport failure must reach the caller"); + let mut source = error.source(); + let mut found_injected_failure = false; + while let Some(current) = source { + found_injected_failure |= current.to_string().contains("injected transport failure"); + source = current.source(); + } + assert!( + found_injected_failure, + "response error lost its cause: {error:?}" + ); + assert_eq!(transport.failed_sends(), 1); +} + +fn retry_request_stanza(id: &'static str) -> wacore_binary::Node { + NodeBuilder::new("message") + .attr("id", id) + .attr("from", "15551234567:7@s.whatsapp.net") + .attr("t", "1") + .attr("type", "text") + .build() +} + +#[tokio::test] +async fn explicit_retry_sends_only_the_retry_receipt() { + let (client, transport) = capturing_client("explicit_retry").await; + let stanza = retry_request_stanza("EXPLICIT-RETRY"); + + let outcome = client + .request_message_retry( + &stanza.as_node_ref(), + crate::features::RetryRequestOptions::new().with_reason(RetryReason::BadMac), + ) + .await + .expect("retry receipt should send"); + assert_eq!( + outcome, + crate::features::RetryRequestOutcome::Sent { + retry_count: 1, + included_keys: false, + } + ); + + let frames = transport.sent(); + assert_eq!(message_acks_for(&frames, "EXPLICIT-RETRY"), 0); + let mut found_retry = false; + for (index, frame) in frames.iter().enumerate() { + let Some(bytes) = decode_frame(index, frame) else { + continue; + }; + let Ok(receipt) = wacore_binary::marshal::unmarshal_ref(&bytes[1..]) else { + continue; + }; + if receipt.tag.as_ref() != "receipt" + || !receipt + .get_attr("id") + .is_some_and(|value| value.as_str() == "EXPLICIT-RETRY") + { + continue; + } + assert!( + receipt + .get_attr("type") + .is_some_and(|value| value.as_str() == "retry") + ); + assert!(receipt.get_optional_child("keys").is_none()); + let retry = receipt + .get_optional_child("retry") + .expect("retry receipt should have retry metadata"); + assert!( + retry + .get_attr("count") + .is_some_and(|value| value.as_str() == "1") + ); + assert!( + retry + .get_attr("error") + .is_some_and(|value| value.as_str() == "7") + ); + found_retry = true; + } + assert!(found_retry, "manual operation must emit a retry receipt"); +} + +#[tokio::test] +async fn explicit_retry_force_includes_keys_on_first_attempt() { + let (client, transport) = capturing_client("explicit_retry_force").await; + client + .persistence_manager + .process_command(crate::store::commands::DeviceCommand::SetAccount(Some( + wa::ADVSignedDeviceIdentity::default(), + ))) + .await; + let before = client.persistence_manager.get_device_snapshot(); + let stanza = retry_request_stanza("EXPLICIT-RETRY-FORCE"); + + let outcome = client + .request_message_retry( + &stanza.as_node_ref(), + crate::features::RetryRequestOptions::new().with_force_include_keys(true), + ) + .await + .expect("forced first retry should send its key bundle"); + assert_eq!( + outcome, + crate::features::RetryRequestOutcome::Sent { + retry_count: 1, + included_keys: true, + } + ); + + let frames = transport.sent(); + let mut found_keys = false; + for (index, frame) in frames.iter().enumerate() { + let Some(bytes) = decode_frame(index, frame) else { + continue; + }; + let Ok(receipt) = wacore_binary::marshal::unmarshal_ref(&bytes[1..]) else { + continue; + }; + if receipt.tag.as_ref() == "receipt" + && receipt + .get_attr("id") + .is_some_and(|value| value.as_str() == "EXPLICIT-RETRY-FORCE") + { + found_keys = receipt.get_optional_child("keys").is_some(); + } + } + assert!( + found_keys, + "force_include_keys must affect the first wire request" + ); + assert_eq!(message_acks_for(&frames, "EXPLICIT-RETRY-FORCE"), 0); + + let after = client.persistence_manager.get_device_snapshot(); + assert_ne!(after.next_pre_key_id, before.next_pre_key_id); + assert_eq!( + after.first_unupload_pre_key_id, after.next_pre_key_id, + "the directly distributed prekey must leave the upload window" + ); +} + +#[tokio::test] +async fn explicit_retry_includes_keys_at_the_normal_threshold() { + let (client, _transport) = capturing_client("explicit_retry_threshold").await; + client + .persistence_manager + .process_command(crate::store::commands::DeviceCommand::SetAccount(Some( + wa::ADVSignedDeviceIdentity::default(), + ))) + .await; + let stanza = retry_request_stanza("EXPLICIT-RETRY-THRESHOLD"); + + assert_eq!( + client + .request_message_retry( + &stanza.as_node_ref(), + crate::features::RetryRequestOptions::default(), + ) + .await + .expect("first retry should send"), + crate::features::RetryRequestOutcome::Sent { + retry_count: 1, + included_keys: false, + } + ); + assert_eq!( + client + .request_message_retry( + &stanza.as_node_ref(), + crate::features::RetryRequestOptions::default(), + ) + .await + .expect("second retry should send"), + crate::features::RetryRequestOutcome::Sent { + retry_count: 2, + included_keys: true, + } + ); +} + +#[tokio::test] +async fn explicit_retry_preserves_canonical_routing_shapes() { + let (client, transport) = capturing_client("explicit_retry_routing").await; + client + .persistence_manager + .process_command(crate::store::commands::DeviceCommand::SetAccount(Some( + wa::ADVSignedDeviceIdentity::default(), + ))) + .await; + + let group = NodeBuilder::new("message") + .attr("id", "RETRY-GROUP") + .attr("from", "120363021033254949@g.us") + .attr("participant", "15551234567:7@s.whatsapp.net") + .attr("t", "1") + .build(); + assert_eq!( + client + .request_message_retry( + &group.as_node_ref(), + crate::features::RetryRequestOptions::default(), + ) + .await + .expect("group retry should send"), + crate::features::RetryRequestOutcome::Sent { + retry_count: 1, + included_keys: false, + } + ); + let group_receipt = + find_receipt_details(&transport.sent(), "RETRY-GROUP").expect("group retry receipt"); + assert_eq!(group_receipt.to, "120363021033254949@g.us"); + assert_eq!( + group_receipt.participant.as_deref(), + Some("15551234567:7@s.whatsapp.net") + ); + assert_eq!(group_receipt.recipient, None); + assert_eq!(group_receipt.category, None); + + let status = NodeBuilder::new("message") + .attr("id", "RETRY-STATUS") + .attr("from", "status@broadcast") + .attr("participant", "15551234567:7@s.whatsapp.net") + .attr("t", "1") + .build(); + client + .request_message_retry( + &status.as_node_ref(), + crate::features::RetryRequestOptions::default(), + ) + .await + .expect("status retry should send"); + let status_receipt = + find_receipt_details(&transport.sent(), "RETRY-STATUS").expect("status retry receipt"); + assert_eq!(status_receipt.to, "status@broadcast"); + assert_eq!( + status_receipt.participant.as_deref(), + Some("15551234567:7@s.whatsapp.net") + ); + + let peer = NodeBuilder::new("message") + .attr("id", "RETRY-PEER") + .attr("from", "5511000000001:9@s.whatsapp.net") + .attr("recipient", "15551234567@s.whatsapp.net") + .attr("category", "peer") + .attr("t", "1") + .build(); + client + .request_message_retry( + &peer.as_node_ref(), + crate::features::RetryRequestOptions::default(), + ) + .await + .expect("peer retry should send"); + let peer_receipt = + find_receipt_details(&transport.sent(), "RETRY-PEER").expect("peer retry receipt"); + assert_eq!(peer_receipt.to, "5511000000001:9@s.whatsapp.net"); + assert_eq!(peer_receipt.category.as_deref(), Some("peer")); + assert_eq!(peer_receipt.recipient, None); + + let self_fanout = NodeBuilder::new("message") + .attr("id", "RETRY-SELF") + .attr("from", "5511000000001:9@s.whatsapp.net") + .attr("recipient", "15551234567@s.whatsapp.net") + .attr("t", "1") + .build(); + client + .request_message_retry( + &self_fanout.as_node_ref(), + crate::features::RetryRequestOptions::default(), + ) + .await + .expect("self-fanout retry should send"); + let self_receipt = + find_receipt_details(&transport.sent(), "RETRY-SELF").expect("self retry receipt"); + assert_eq!(self_receipt.to, "5511000000001:9@s.whatsapp.net"); + assert_eq!( + self_receipt.recipient.as_deref(), + Some("15551234567@s.whatsapp.net") + ); + assert_eq!(self_receipt.category, None); + + let hosted = NodeBuilder::new("message") + .attr("id", "RETRY-HOSTED") + .attr("from", "15551234567:7@hosted") + .attr("t", "1") + .build(); + assert_eq!( + client + .request_message_retry( + &hosted.as_node_ref(), + crate::features::RetryRequestOptions::default(), + ) + .await + .expect("stateless retry should send"), + crate::features::RetryRequestOutcome::Sent { + retry_count: 1, + included_keys: true, + } + ); + let hosted_receipt = + find_receipt_details(&transport.sent(), "RETRY-HOSTED").expect("hosted retry receipt"); + assert_eq!(hosted_receipt.to, "15551234567:7@hosted"); + assert!(hosted_receipt.has_keys); + + let bot_dm = NodeBuilder::new("message") + .attr("id", "RETRY-BOT-DM") + .attr("from", "200000000000002@bot") + .attr("t", "1") + .build(); + client + .request_message_retry( + &bot_dm.as_node_ref(), + crate::features::RetryRequestOptions::default(), + ) + .await + .expect("bot DM retry should send"); + let bot_receipt = + find_receipt_details(&transport.sent(), "RETRY-BOT-DM").expect("bot retry receipt"); + assert_eq!(bot_receipt.to, "200000000000002@bot"); + assert_eq!(bot_receipt.participant, None); + + let bot_group = NodeBuilder::new("message") + .attr("id", "RETRY-BOT-GROUP") + .attr("from", "120363021033254949@g.us") + .attr("participant", "200000000000002@bot") + .attr("t", "1") + .build(); + assert_eq!( + client + .request_message_retry( + &bot_group.as_node_ref(), + crate::features::RetryRequestOptions::default(), + ) + .await + .expect("suppression is an explicit outcome"), + crate::features::RetryRequestOutcome::Suppressed { retry_count: 1 } + ); + assert!(find_receipt_details(&transport.sent(), "RETRY-BOT-GROUP").is_none()); + + for id in [ + "RETRY-GROUP", + "RETRY-STATUS", + "RETRY-PEER", + "RETRY-SELF", + "RETRY-HOSTED", + "RETRY-BOT-DM", + "RETRY-BOT-GROUP", + ] { + assert_eq!(message_acks_for(&transport.sent(), id), 0); + } +} + +#[tokio::test] +async fn explicit_retry_validates_input_and_reports_the_shared_limit() { + let (client, transport) = capturing_client("explicit_retry_validation").await; + + let receipt = NodeBuilder::new("receipt") + .attr("id", "WRONG-CLASS") + .attr("from", "15551234567@s.whatsapp.net") + .build(); + assert!(matches!( + client + .request_message_retry( + &receipt.as_node_ref(), + crate::features::RetryRequestOptions::default(), + ) + .await, + Err(crate::features::RetryRequestError::UnsupportedStanzaClass) + )); + + let missing_id = NodeBuilder::new("message") + .attr("from", "15551234567@s.whatsapp.net") + .build(); + assert!(matches!( + client + .request_message_retry( + &missing_id.as_node_ref(), + crate::features::RetryRequestOptions::default(), + ) + .await, + Err(crate::features::RetryRequestError::MissingAttribute("id")) + )); + + let missing_from = NodeBuilder::new("message") + .attr("id", "MISSING-FROM") + .build(); + assert!(matches!( + client + .request_message_retry( + &missing_from.as_node_ref(), + crate::features::RetryRequestOptions::default(), + ) + .await, + Err(crate::features::RetryRequestError::MissingAttribute("from")) + )); + + let stanza = retry_request_stanza("RETRY-LIMIT"); + let sender: Jid = "15551234567:7@s.whatsapp.net".parse().expect("sender"); + let chat = sender.to_non_ad(); + let cache_key = client + .make_retry_cache_key(&chat, "RETRY-LIMIT", &sender) + .await; + client + .message_retry_counts + .insert(cache_key, (MAX_DECRYPT_RETRIES, None)) + .await; + assert_eq!( + client + .request_message_retry( + &stanza.as_node_ref(), + crate::features::RetryRequestOptions::default(), + ) + .await + .expect("reaching the retry limit is an explicit outcome"), + crate::features::RetryRequestOutcome::LimitReached + ); + assert_eq!(transport.sent_count(), 0); +} + +#[tokio::test] +async fn explicit_retry_preserves_transport_error_chain() { + use std::error::Error as _; + + let (client, transport) = capturing_client("explicit_retry_error").await; + client + .persistence_manager + .process_command(crate::store::commands::DeviceCommand::SetAccount(Some( + wa::ADVSignedDeviceIdentity::default(), + ))) + .await; + let before = client.persistence_manager.get_device_snapshot(); + transport.fail_next_sends(1); + let stanza = retry_request_stanza("EXPLICIT-RETRY-ERROR"); + let error = client + .request_message_retry( + &stanza.as_node_ref(), + crate::features::RetryRequestOptions::new().with_force_include_keys(true), + ) + .await + .expect_err("transport failure must reach the caller"); + + let mut source = error.source(); + let mut found_injected_failure = false; + while let Some(current) = source { + found_injected_failure |= current.to_string().contains("injected transport failure"); + source = current.source(); + } + assert!( + found_injected_failure, + "typed retry error must preserve the original transport cause: {error:?}" + ); + assert_eq!(transport.sent_count(), 0); + + let after = client.persistence_manager.get_device_snapshot(); + assert_ne!(after.next_pre_key_id, before.next_pre_key_id); + assert_eq!( + after.first_unupload_pre_key_id, after.next_pre_key_id, + "a possibly delivered direct-distribution key must not be offered again" + ); +} + /// Regression: a malformed pkmsg used to fall through silently. Now -/// it dispatches the consumer event AND emits a nack on the wire so +/// it dispatches the undecryptable event AND emits a nack on the wire so /// the server stops retransmitting. #[tokio::test] async fn pkmsg_parse_error_dispatches_parsing_error_nack() { @@ -5795,6 +6376,8 @@ struct SentReceipt { recipient: Option, participant: Option, context: Option, + category: Option, + has_keys: bool, } fn find_receipt_details(frames: &[bytes::Bytes], id: &str) -> Option { @@ -5815,6 +6398,8 @@ fn find_receipt_details(frames: &[bytes::Bytes], id: &str) -> Option( + &self, + key: &Q, + update: impl FnOnce(Option<&V>) -> (Option, R), + ) -> R + where + K: Borrow, + Q: ToOwned + Hash + Eq + ?Sized, + { + let now = self.entry_time(); + let mut guard = self.inner.write().await; + + if guard + .map + .get(key) + .is_some_and(|entry| self.is_expired(entry, now)) + && let Some(owned_key) = Self::find_key(&guard, key) + { + guard.remove_key(&owned_key); + } + + let (next, result) = update(guard.map.get(key).map(|entry| &entry.value)); + let Some(next) = next else { + return result; + }; + + if let Some(entry) = guard.map.get_mut(key) { + entry.value = next; + entry.inserted_at = now; + entry.last_accessed_at = now; + } else if self.max_capacity != Some(0) { + guard.insert_new( + key.to_owned(), + next, + now, + self.max_capacity, + self.evict_guard, + ); + } + + result + } + /// Insert and return a clone of the value in one write lock. async fn insert_and_return(&self, key: K, value: V) -> V { let now = self.entry_time(); @@ -634,6 +683,38 @@ mod tests { assert_eq!(cache.entry_count(), 1); } + #[tokio::test] + async fn upsert_with_by_ref_serializes_read_modify_write() { + let cache = Arc::new(build_cache::()); + let mut tasks = Vec::new(); + for _ in 0..32 { + let cache = Arc::clone(&cache); + tasks.push(tokio::spawn(async move { + cache + .upsert_with_by_ref("counter", |current| { + let next = current.copied().unwrap_or_default() + 1; + (Some(next), next) + }) + .await + })); + } + + let mut results = Vec::with_capacity(tasks.len()); + for task in tasks { + results.push(task.await.unwrap()); + } + results.sort_unstable(); + + assert_eq!(results, (1..=32).collect::>()); + assert_eq!(cache.get("counter").await, Some(32)); + + let unchanged = cache + .upsert_with_by_ref("counter", |current| (None, current.copied())) + .await; + assert_eq!(unchanged, Some(32)); + assert_eq!(cache.get("counter").await, Some(32)); + } + #[tokio::test] async fn test_capacity_eviction() { let cache: PortableCache = PortableCache::builder().max_capacity(3).build(); diff --git a/src/prekeys.rs b/src/prekeys.rs index 56802fc67..c36e5f5b2 100644 --- a/src/prekeys.rs +++ b/src/prekeys.rs @@ -6,6 +6,7 @@ use crate::client::Client; use anyhow; +use anyhow::Context as _; use log; use std::sync::atomic::Ordering; @@ -420,7 +421,7 @@ impl Client { self.persistence_manager .flush() .await - .map_err(|e| anyhow::anyhow!("failed to flush prekey watermarks: {e:?}"))?; + .context("failed to flush prekey watermarks")?; Ok((id, key_pair.public_key)) } @@ -460,7 +461,7 @@ impl Client { self.persistence_manager .flush() .await - .map_err(|e| anyhow::anyhow!("failed to flush prekey watermark after mark: {e:?}"))?; + .context("failed to flush prekey watermark after mark")?; Ok(()) } @@ -594,7 +595,7 @@ impl Client { self.persistence_manager .flush() .await - .map_err(|e| anyhow::anyhow!("failed to flush prekey watermarks: {e:?}"))?; + .context("failed to flush prekey watermarks")?; // Only the leftover (already-stored) window keys are read back and decoded; // the fresh ones are already in `fresh_pre_keys`. On the common connect path @@ -693,7 +694,7 @@ impl Client { self.persistence_manager .flush() .await - .map_err(|e| anyhow::anyhow!("failed to flush abandon watermark: {e:?}"))?; + .context("failed to flush abandon watermark")?; self.execute(spec).await?; diff --git a/src/receipt.rs b/src/receipt.rs index 711a885e2..4b80f0ebe 100644 --- a/src/receipt.rs +++ b/src/receipt.rs @@ -7,7 +7,7 @@ use std::sync::Arc; use wacore::protocol::nack::NackReason; use wacore::types::message::MessageCategory; use wacore_binary::builder::NodeBuilder; -use wacore_binary::{Jid, JidExt as _}; +use wacore_binary::{Jid, JidExt as _, NodeRef, NodeValue}; use wacore_binary::OwnedNodeRef; @@ -287,41 +287,109 @@ fn build_aggregate_delivery_receipt_nodes( .collect() } -/// `` builder, mirrors WA Web's -/// `Handle/MsgSendAck.js::sendNack`. `failure_reason` is only emitted -/// for `InvalidProtobuf` (as `` child). -fn build_nack_node( - info: &MessageInfo, +trait NackSource { + fn class(&self, reason: NackReason) -> Result<&str, crate::features::StanzaResponseError>; + fn id(&self) -> Result; + fn to(&self) -> Result; + fn participant(&self) -> Option; + fn stanza_type(&self) -> Option; +} + +impl NackSource for NodeRef<'_> { + fn class(&self, reason: NackReason) -> Result<&str, crate::features::StanzaResponseError> { + if reason == NackReason::UnrecognizedStanza + || matches!(self.tag.as_ref(), "message" | "notification" | "receipt") + { + Ok(self.tag.as_ref()) + } else { + Err(crate::features::StanzaResponseError::UnsupportedStanzaClass) + } + } + + fn id(&self) -> Result { + self.get_attr("id") + .map(|value| value.to_node_value()) + .ok_or(crate::features::StanzaResponseError::MissingAttribute("id")) + } + + fn to(&self) -> Result { + self.get_attr("from") + .map(|value| value.to_node_value()) + .ok_or(crate::features::StanzaResponseError::MissingAttribute( + "from", + )) + } + + fn participant(&self) -> Option { + self.get_attr("participant") + .map(|value| value.to_node_value()) + } + + fn stanza_type(&self) -> Option { + self.get_attr("type").map(|value| value.to_node_value()) + } +} + +impl NackSource for MessageInfo { + fn class(&self, _reason: NackReason) -> Result<&str, crate::features::StanzaResponseError> { + Ok("message") + } + + fn id(&self) -> Result { + if self.id.is_empty() { + Err(crate::features::StanzaResponseError::MissingAttribute("id")) + } else { + Ok(NodeValue::from(&self.id)) + } + } + + fn to(&self) -> Result { + Ok(NodeValue::from(&self.source.chat)) + } + + fn participant(&self) -> Option { + (self.source.is_group || self.source.chat.is_status_broadcast()) + .then(|| NodeValue::from(&self.source.sender)) + } + + fn stanza_type(&self) -> Option { + (!self.r#type.is_empty()).then(|| NodeValue::from(&self.r#type)) + } +} + +/// Build the canonical rejection for either an original stanza or parsed +/// message metadata. `failure_reason` is valid only for `InvalidProtobuf`. +fn build_nack_node( + source: &S, own_pn: &Jid, reason: NackReason, failure_reason: Option, -) -> wacore_binary::Node { +) -> Result { let mut builder = NodeBuilder::new("ack") - .attr("class", "message") - .attr("id", &info.id) + .attr("class", source.class(reason)?) + .attr("id", source.id()?) .attr("from", own_pn) - .attr("to", &info.source.chat) - .attr("error", reason.code().to_string()); + .attr("to", source.to()?) + .attr("error", reason.code()); - let is_status = info.source.chat.is_status_broadcast(); - if info.source.is_group || is_status { - builder = builder.attr("participant", &info.source.sender); + if let Some(participant) = source.participant() { + builder = builder.attr("participant", participant); } - if !info.r#type.is_empty() { - builder = builder.attr("type", &info.r#type); + if let Some(stanza_type) = source.stanza_type() { + builder = builder.attr("type", stanza_type); } if reason == NackReason::InvalidProtobuf && let Some(code) = failure_reason { let meta = NodeBuilder::new("meta") - .attr("failure_reason", code.to_string()) + .attr("failure_reason", code) .build(); builder = builder.children(vec![meta]); } - builder.build() + Ok(builder.build()) } impl Client { @@ -688,7 +756,8 @@ impl Client { if info.id.is_empty() { return; } - let Some(own_pn) = self.get_pn() else { + let device = self.persistence_manager.get_device_snapshot(); + let Some(own_pn) = device.pn.as_ref() else { log::debug!( "[msg:{}] Skipping nack ({:?}): own PN not yet set", info.id, @@ -697,7 +766,15 @@ impl Client { return; }; - let nack = build_nack_node(info, &own_pn, reason, failure_reason); + let nack = match build_nack_node(info, own_pn, reason, failure_reason) { + Ok(nack) => nack, + Err(error) => { + log::warn!(target: "Client/Receipt", + "Failed to build nack for message {}: {error}", info.id); + return; + } + }; + drop(device); debug!(target: "Client/Receipt", "Sending nack (reason={:?}, code={}) for message {} from {}", reason, reason.code(), info.id, info.source.sender.observe()); @@ -710,6 +787,37 @@ impl Client { } } + /// Reject a received stanza using its original borrowed representation. + #[cfg_attr( + feature = "tracing", + tracing::instrument( + name = "wa.receipt.reject_stanza", + level = "debug", + skip_all, + err(Debug) + ) + )] + pub async fn reject_stanza( + &self, + stanza: &NodeRef<'_>, + rejection: crate::features::StanzaRejection, + ) -> Result<(), crate::features::StanzaResponseError> { + let device = self.persistence_manager.get_device_snapshot(); + let own_pn = device + .pn + .as_ref() + .ok_or(crate::features::StanzaResponseError::MissingLocalIdentity)?; + let nack = build_nack_node( + stanza, + own_pn, + rejection.reason(), + rejection.failure_reason(), + )?; + drop(device); + self.send_node(nack).await?; + Ok(()) + } + /// Sends read receipts for one or more messages. /// /// For group messages, pass the message sender as `sender`. @@ -1263,10 +1371,176 @@ mod tests { .expect("own PN should parse") } + #[test] + fn nack_from_original_stanza_preserves_each_supported_class() { + for tag in ["message", "receipt", "notification"] { + let stanza = NodeBuilder::new(tag) + .attr("id", "STANZA-ID") + .attr("from", "120363021033254949@g.us") + .attr("participant", "15551234567:4@s.whatsapp.net") + .attr("type", "test-type") + .build(); + let nack = build_nack_node( + &stanza.as_node_ref(), + &own_pn(), + NackReason::ParsingError, + None, + ) + .expect("supported stanza should produce a nack"); + + assert_eq!( + nack.attrs + .get("class") + .map(|value| value.as_str()) + .as_deref(), + Some(tag) + ); + assert_eq!( + nack.attrs.get("id").map(|value| value.as_str()).as_deref(), + Some("STANZA-ID") + ); + assert_eq!( + nack.attrs.get("to").map(|value| value.as_str()).as_deref(), + Some("120363021033254949@g.us") + ); + assert_eq!( + nack.attrs + .get("participant") + .map(|value| value.as_str()) + .as_deref(), + Some("15551234567:4@s.whatsapp.net") + ); + assert_eq!( + nack.attrs + .get("type") + .map(|value| value.as_str()) + .as_deref(), + Some("test-type") + ); + assert_eq!( + nack.attrs + .get("from") + .map(|value| value.as_str()) + .as_deref(), + Some("5511000000001@s.whatsapp.net") + ); + } + } + + #[test] + fn unrecognized_stanza_rejection_preserves_custom_class() { + let stanza = NodeBuilder::new("future-stanza") + .attr("id", "FUTURE-ID") + .attr("from", "15551234567@s.whatsapp.net") + .build(); + let nack = build_nack_node( + &stanza.as_node_ref(), + &own_pn(), + NackReason::UnrecognizedStanza, + None, + ) + .expect("unrecognized stanza reason supports arbitrary classes"); + + assert_eq!( + nack.attrs + .get("class") + .map(|value| value.as_str()) + .as_deref(), + Some("future-stanza") + ); + assert!(matches!( + build_nack_node( + &stanza.as_node_ref(), + &own_pn(), + NackReason::ParsingError, + None + ), + Err(crate::features::StanzaResponseError::UnsupportedStanzaClass) + )); + } + + #[test] + fn nack_does_not_apply_the_receipt_ack_participant_rule() { + let stanza = NodeBuilder::new("receipt") + .attr("id", "NACK-DUPLICATE-PARTICIPANT") + .attr("from", "15551234567@s.whatsapp.net") + .attr("participant", "15551234567@s.whatsapp.net") + .build(); + let nack = build_nack_node( + &stanza.as_node_ref(), + &own_pn(), + NackReason::ParsingError, + None, + ) + .expect("supported stanza should produce a nack"); + + assert!( + nack.attrs + .get("participant") + .is_some_and(|value| value == "15551234567@s.whatsapp.net"), + "nack must preserve participant even when a receipt ack would omit it" + ); + } + + #[test] + fn nack_from_original_stanza_requires_id_and_from() { + let without_id = NodeBuilder::new("message") + .attr("from", "15551234567@s.whatsapp.net") + .build(); + assert!(matches!( + build_nack_node( + &without_id.as_node_ref(), + &own_pn(), + NackReason::ParsingError, + None + ), + Err(crate::features::StanzaResponseError::MissingAttribute("id")) + )); + + let without_from = NodeBuilder::new("message") + .attr("id", "MISSING-FROM") + .build(); + assert!(matches!( + build_nack_node( + &without_from.as_node_ref(), + &own_pn(), + NackReason::ParsingError, + None + ), + Err(crate::features::StanzaResponseError::MissingAttribute( + "from" + )) + )); + } + + #[test] + fn nack_preserves_unknown_numeric_reason() { + let stanza = NodeBuilder::new("message") + .attr("id", "UNKNOWN-REASON") + .attr("from", "15551234567@s.whatsapp.net") + .build(); + let nack = build_nack_node( + &stanza.as_node_ref(), + &own_pn(), + NackReason::Unknown(599), + None, + ) + .expect("known stanza supports unknown future error codes"); + + assert_eq!( + nack.attrs + .get("error") + .map(|value| value.as_str()) + .as_deref(), + Some("599") + ); + } + #[test] fn nack_for_dm_carries_class_message_and_error_code() { let info = info_with("12345@s.whatsapp.net", "12345@s.whatsapp.net", false); - let node = build_nack_node(&info, &own_pn(), NackReason::ParsingError, None); + let node = build_nack_node(&info, &own_pn(), NackReason::ParsingError, None) + .expect("valid DM should produce a nack"); assert_eq!(node.tag, "ack"); assert_eq!( @@ -1293,7 +1567,8 @@ mod tests { "15551234567@s.whatsapp.net", true, ); - let node = build_nack_node(&info, &own_pn(), NackReason::UnhandledError, None); + let node = build_nack_node(&info, &own_pn(), NackReason::UnhandledError, None) + .expect("valid group message should produce a nack"); assert_eq!( node.attrs.get("participant").map(|v| v.as_str()).as_deref(), @@ -1308,7 +1583,8 @@ mod tests { #[test] fn nack_for_status_broadcast_carries_participant() { let info = info_with("status@broadcast", "12345@s.whatsapp.net", false); - let node = build_nack_node(&info, &own_pn(), NackReason::ParsingError, None); + let node = build_nack_node(&info, &own_pn(), NackReason::ParsingError, None) + .expect("valid status message should produce a nack"); assert_eq!( node.attrs.get("participant").map(|v| v.as_str()).as_deref(), @@ -1319,7 +1595,8 @@ mod tests { #[test] fn nack_invalid_protobuf_includes_meta_failure_reason() { let info = info_with("12345@s.whatsapp.net", "12345@s.whatsapp.net", false); - let node = build_nack_node(&info, &own_pn(), NackReason::InvalidProtobuf, Some(42)); + let node = build_nack_node(&info, &own_pn(), NackReason::InvalidProtobuf, Some(42)) + .expect("valid message should produce a nack"); assert_eq!( node.attrs.get("error").map(|v| v.as_str()).as_deref(), @@ -1340,7 +1617,8 @@ mod tests { #[test] fn nack_invalid_protobuf_without_failure_reason_omits_meta() { let info = info_with("12345@s.whatsapp.net", "12345@s.whatsapp.net", false); - let node = build_nack_node(&info, &own_pn(), NackReason::InvalidProtobuf, None); + let node = build_nack_node(&info, &own_pn(), NackReason::InvalidProtobuf, None) + .expect("valid message should produce a nack"); assert!(node.get_optional_child("meta").is_none()); } @@ -1348,7 +1626,8 @@ mod tests { #[test] fn nack_omits_meta_for_non_invalid_protobuf_even_with_failure_reason() { let info = info_with("12345@s.whatsapp.net", "12345@s.whatsapp.net", false); - let node = build_nack_node(&info, &own_pn(), NackReason::ParsingError, Some(99)); + let node = build_nack_node(&info, &own_pn(), NackReason::ParsingError, Some(99)) + .expect("valid message should produce a nack"); assert!(node.get_optional_child("meta").is_none()); } @@ -1356,7 +1635,8 @@ mod tests { fn nack_includes_type_when_present() { let mut info = info_with("12345@s.whatsapp.net", "12345@s.whatsapp.net", false); info.r#type = "text".to_string(); - let node = build_nack_node(&info, &own_pn(), NackReason::ParsingError, None); + let node = build_nack_node(&info, &own_pn(), NackReason::ParsingError, None) + .expect("valid message should produce a nack"); assert_eq!( node.attrs.get("type").map(|v| v.as_str()).as_deref(), Some("text") @@ -1367,7 +1647,8 @@ mod tests { fn nack_omits_type_when_empty() { let mut info = info_with("12345@s.whatsapp.net", "12345@s.whatsapp.net", false); info.r#type = String::new(); - let node = build_nack_node(&info, &own_pn(), NackReason::ParsingError, None); + let node = build_nack_node(&info, &own_pn(), NackReason::ParsingError, None) + .expect("valid message should produce a nack"); assert!(node.attrs.get("type").is_none()); } diff --git a/src/retry.rs b/src/retry.rs index 89a021260..8060c5481 100644 --- a/src/retry.rs +++ b/src/retry.rs @@ -1,4 +1,5 @@ use crate::client::Client; +use crate::features::RetryRequestError; use crate::message::RetryReason; use crate::types::events::Receipt; use log::{debug, info, warn}; @@ -9,6 +10,7 @@ use std::sync::Arc; use wacore::iq::prekeys::{OneTimePreKeyNode, SignedPreKeyNode}; use wacore::libsignal::protocol::{PreKeyBundle, PublicKey}; use wacore::protocol::ProtocolNode; +use wacore::protocol::retry::{MAX_RETRY_COUNT, MIN_RETRY_FOR_BASE_KEY_CHECK}; use wacore::types::jid::JidExt; use wacore_binary::JidExt as _; #[cfg(test)] @@ -34,18 +36,15 @@ fn get_bytes_content_ref<'a>(node: &'a NodeRef<'_>) -> Option<&'a [u8]> { } } -/// Maximum retry attempts we'll honor (matches WhatsApp Web's MAX_RETRY = 5). -/// We refuse to resend if the requester has already retried this many times. -const MAX_RETRY_COUNT: u8 = 5; - -/// Minimum retry count before we start tracking base keys. -/// WhatsApp Web saves base key on retry 2, checks on retry > 2. -const MIN_RETRY_FOR_BASE_KEY_CHECK: u8 = 2; - /// Throttle for the "no-keys + retry≥2" forced-recreate fallback. Mirrors /// whatsmeow's `recreateSessionTimeout` (`retry.go:156`). const RECREATE_SESSION_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(3600); +pub(crate) enum RetryReceiptSendOutcome { + Sent { included_keys: bool }, + Suppressed, +} + /// Separated chat and requester JIDs for retry receipt handling. /// Mirrors WAWebHandleRetryRequest `getActualChatInfo` + `getTargetChat`. struct RetryChatInfo { @@ -1078,7 +1077,8 @@ impl Client { info: &crate::types::message::MessageInfo, retry_count: u8, reason: RetryReason, - ) -> Result<(), anyhow::Error> { + force_include_keys: bool, + ) -> Result { let device_snapshot = self.persistence_manager.get_device_snapshot(); // WA Web's sendRetryReceipt aborts only when `!to.isBot() && participant.isBot()`, @@ -1092,7 +1092,7 @@ impl Client { info.source.sender.observe(), info.source.chat.observe() ); - return Ok(()); + return Ok(RetryReceiptSendOutcome::Suppressed); } debug!( @@ -1124,7 +1124,18 @@ impl Client { .bytes(registration_id_bytes) .build(); - let keys_node = if wacore::protocol::retry::should_include_keys(retry_count, reason) { + let receipt_to = if info.source.is_group { + &info.source.chat + } else { + &info.source.sender + }; + let include_keys = wacore::protocol::retry::should_include_keys_with_policy( + retry_count, + force_include_keys, + receipt_to.is_hosted(), + ); + + let keys_node = if include_keys { // Validate the account BEFORE reserving/marking the prekey: a missing // account bails here, and marking after would abandon a one-time // prekey from the upload window without any receipt going out. @@ -1158,12 +1169,6 @@ impl Client { None }; - let receipt_to = if info.source.is_group { - &info.source.chat - } else { - &info.source.sender - }; - // Build the receipt node. For group messages, include the participant attribute // to identify which group member should resend. For DMs, omit it since the // "to" address already identifies the sender. @@ -1202,7 +1207,8 @@ impl Client { } } - // Build children list - keys are only included when retryCount >= 2 + // Build the final child list after the policy has decided whether this + // request carries key material. let receipt_node = if let Some(keys) = keys_node { builder .children([retry_node, registration_node, keys]) @@ -1211,8 +1217,11 @@ impl Client { builder.children([retry_node, registration_node]).build() }; + drop(device_snapshot); self.send_node(receipt_node).await?; - Ok(()) + Ok(RetryReceiptSendOutcome::Sent { + included_keys: include_keys, + }) } /// Sends an `enc_rekey_retry` receipt for VoIP call encryption re-keying. @@ -2900,205 +2909,22 @@ mod tests { assert!(!(dm.is_group() || dm.is_status_broadcast())); } - /// Test that verifies the key inclusion optimization: - /// - Keys should be included on retry#1 for NoSession errors (the optimization) - /// - Keys should NOT be included on retry#1 for other error types - /// - Keys should be included on retry#2+ for ALL error types + /// The key-bundle policy is driven only by explicit force, stateless routing, + /// and the retry threshold. The diagnostic reason must not change the wire + /// shape of a first retry. #[test] - fn keys_inclusion_optimization_for_no_session_errors() { - use crate::message::RetryReason; - - // Test cases: (retry_count, reason, should_include_keys) - let test_cases = [ - // NoSession errors - optimization kicks in at retry#1 - ( - 1, - RetryReason::NoSession, - true, - "NoSession at retry#1 should include keys (optimization)", - ), - ( - 2, - RetryReason::NoSession, - true, - "NoSession at retry#2 should include keys", - ), - ( - 3, - RetryReason::NoSession, - true, - "NoSession at retry#3 should include keys", - ), - // InvalidMessage errors - no keys at retry#1, keys at retry#2+ - ( - 1, - RetryReason::InvalidMessage, - false, - "InvalidMessage at retry#1 should NOT include keys", - ), - ( - 2, - RetryReason::InvalidMessage, - true, - "InvalidMessage at retry#2 should include keys", - ), - ( - 3, - RetryReason::InvalidMessage, - true, - "InvalidMessage at retry#3 should include keys", - ), - // BadMac errors - same as InvalidMessage - ( - 1, - RetryReason::BadMac, - false, - "BadMac at retry#1 should NOT include keys", - ), - ( - 2, - RetryReason::BadMac, - true, - "BadMac at retry#2 should include keys", - ), - // UnknownError - no keys at retry#1 - ( - 1, - RetryReason::UnknownError, - false, - "UnknownError at retry#1 should NOT include keys", - ), - ( - 2, - RetryReason::UnknownError, - true, - "UnknownError at retry#2 should include keys", - ), - ]; - - for (retry_count, reason, should_include_keys, description) in test_cases { - // Replicate the logic from send_retry_receipt - let would_include_keys = - wacore::protocol::retry::should_include_keys(retry_count, reason); - - assert_eq!( - would_include_keys, should_include_keys, - "Failed: {description}. retry_count={retry_count}, reason={reason:?}" - ); - } - } - - /// Integration test simulating high concurrent offline message scenarios. - /// This tests the scenario where many skmsg-only messages arrive before SKDM, - /// causing NoSession errors that need retry with keys. - #[tokio::test] - async fn concurrent_offline_messages_retry_key_optimization() { - use crate::message::RetryReason; - use std::sync::atomic::{AtomicUsize, Ordering}; - use tokio::sync::Barrier; + fn retry_key_inclusion_matches_canonical_policy() { + use wacore::protocol::retry::{should_include_keys, should_include_keys_with_policy}; - let _ = env_logger::builder().is_test(true).try_init(); - - // Simulate processing multiple concurrent skmsg failures - // Each represents a skmsg-only message from the same sender that failed with NoSession - let num_messages = 50; - let barrier = Arc::new(Barrier::new(num_messages)); - - // Track how many would include keys on retry#1 - let keys_included_count = Arc::new(AtomicUsize::new(0)); - let no_keys_count = Arc::new(AtomicUsize::new(0)); - - let mut handles = Vec::new(); - - for i in 0..num_messages { - let barrier = barrier.clone(); - let keys_included = keys_included_count.clone(); - let no_keys = no_keys_count.clone(); - - handles.push(tokio::spawn(async move { - // Simulate concurrent message processing - barrier.wait().await; - - // Each message is a skmsg-only message that fails with NoSession - // (simulating burst of group messages before SKDM arrives) - let retry_count = 1; // First retry - let reason = if i % 5 == 0 { - // Some messages have MAC failure (pkmsg failed) - RetryReason::InvalidMessage - } else { - // Most are skmsg-only NoSession failures - RetryReason::NoSession - }; - - let would_include_keys = - wacore::protocol::retry::should_include_keys(retry_count, reason); - - if would_include_keys { - keys_included.fetch_add(1, Ordering::SeqCst); - } else { - no_keys.fetch_add(1, Ordering::SeqCst); - } - })); - } - - // Wait for all tasks to complete - for handle in handles { - handle.await.expect("task should complete"); - } - - let total_keys_included = keys_included_count.load(Ordering::SeqCst); - let total_no_keys = no_keys_count.load(Ordering::SeqCst); - - // With our optimization: - // - 80% (40/50) are NoSession → keys included on retry#1 - // - 20% (10/50) are InvalidMessage → no keys on retry#1 - assert_eq!( - total_keys_included, 40, - "Expected 40 messages to include keys (NoSession), got {total_keys_included}" - ); - assert_eq!( - total_no_keys, 10, - "Expected 10 messages to NOT include keys (InvalidMessage), got {total_no_keys}" - ); - - // Verify the optimization reduces round-trips - // Without optimization: ALL 50 would need retry#2 for keys - // With optimization: Only 10 need retry#2 for keys (80% improvement for NoSession) - let optimization_benefit = (total_keys_included as f64 / num_messages as f64) * 100.0; - assert!( - optimization_benefit >= 80.0, - "Optimization should benefit at least 80% of NoSession messages, got {optimization_benefit:.1}%" - ); - } - - /// Test that the retry optimization correctly handles the edge case where - /// a sender device is removed mid-retry (cannot respond to retry receipts). - /// This tests our ability to handle the root cause of permanent failures. - #[test] - fn retry_optimization_with_removed_device_scenario() { - use crate::message::RetryReason; - - // Simulate the scenario from the log: - // 1. skmsg arrives → NoSession error → retry#1 with keys (optimization) - // 2. Device is removed → no response to retry - // 3. Message is permanently lost (expected behavior) - - let retry_count = 1; - let reason = RetryReason::NoSession; - - // With optimization, we include keys on retry#1 - let would_include_keys = wacore::protocol::retry::should_include_keys(retry_count, reason); - - assert!( - would_include_keys, - "NoSession should include keys on retry#1 to give sender best chance to respond" - ); - - // Even if sender device is removed, we tried our best by including keys early - // This reduces the window for message loss from: - // - Before: retry#1 (no keys) → sender can't establish session → retry#2 (keys) → device removed - // - After: retry#1 (keys) → sender can establish session immediately → device removed before response - // The optimization gives the sender one fewer round-trip to respond. + assert!(!should_include_keys(1, RetryReason::NoSession)); + assert!(!should_include_keys( + 1, + RetryReason::UnknownCompanionNoPrekey + )); + assert!(should_include_keys_with_policy(1, true, false)); + assert!(should_include_keys_with_policy(1, false, true)); + assert!(should_include_keys(2, RetryReason::InvalidMessage)); + assert!(should_include_keys(3, RetryReason::BadMac)); } /// Helper to build a DM Receipt for testing resolve_retry_chat_info. diff --git a/tests/e2e/tests/app_state.rs b/tests/e2e/tests/app_state.rs index e69456c51..7f731a591 100644 --- a/tests/e2e/tests/app_state.rs +++ b/tests/e2e/tests/app_state.rs @@ -1,3 +1,5 @@ +#![recursion_limit = "512"] + use e2e_tests::TestClient; use log::info; use wacore::store::traits::AppSyncStore; diff --git a/tests/e2e/tests/chat_actions.rs b/tests/e2e/tests/chat_actions.rs index ec9b40502..143c65b5e 100644 --- a/tests/e2e/tests/chat_actions.rs +++ b/tests/e2e/tests/chat_actions.rs @@ -1,3 +1,5 @@ +#![recursion_limit = "512"] + use e2e_tests::TestClient; use log::info; use whatsapp_rust::waproto::whatsapp as wa; diff --git a/tests/e2e/tests/memory_soak.rs b/tests/e2e/tests/memory_soak.rs index ed8de823d..b467293bf 100644 --- a/tests/e2e/tests/memory_soak.rs +++ b/tests/e2e/tests/memory_soak.rs @@ -205,6 +205,15 @@ fn analyze_growth(label: &str, snapshots: &[Snapshot]) { info!(" {name}: {fv} -> {lv}"); } + #[cfg(feature = "dhat-heap")] + { + let heap_growth = last.heap_bytes.saturating_sub(first.heap_bytes); + info!( + " Tracked heap: {}B -> {}B (delta: +{}B)", + first.heap_bytes, last.heap_bytes, heap_growth + ); + } + // RSS growth: warn if RSS grew more than 50 MiB (not a hard fail, just FYI) let rss_growth_kib = last.rss_kib.saturating_sub(first.rss_kib); info!( diff --git a/tests/e2e/tests/messaging.rs b/tests/e2e/tests/messaging.rs index 567c1bfcc..6c019701f 100644 --- a/tests/e2e/tests/messaging.rs +++ b/tests/e2e/tests/messaging.rs @@ -1,3 +1,5 @@ +#![recursion_limit = "512"] + use e2e_tests::{TestClient, text_msg}; use log::info; use whatsapp_rust::waproto::whatsapp as wa; diff --git a/tests/e2e/tests/profile.rs b/tests/e2e/tests/profile.rs index 1372ec85c..dadbffe32 100644 --- a/tests/e2e/tests/profile.rs +++ b/tests/e2e/tests/profile.rs @@ -1,3 +1,5 @@ +#![recursion_limit = "512"] + use e2e_tests::TestClient; use log::info; use wacore::types::events::Event; diff --git a/wacore/binary/src/jid.rs b/wacore/binary/src/jid.rs index 18f6f141e..fb63268af 100644 --- a/wacore/binary/src/jid.rs +++ b/wacore/binary/src/jid.rs @@ -709,6 +709,12 @@ impl Jid { write_jid_fallible(writer, &self.user, self.server, self.agent, self.device) } + /// Compare the display representation with `other` without allocating. + #[inline] + pub fn display_eq(&self, other: &str) -> bool { + jid_display_eq(&self.user, self.server, self.agent, self.device, other) + } + /// Compare device identity (user, server, device) without allocation. #[inline] pub fn device_eq(&self, other: &Jid) -> bool { @@ -759,6 +765,12 @@ impl<'a> JidRef<'a> { integrator: self.integrator, } } + + /// Compare the display representation with `other` without allocating. + #[inline] + pub fn display_eq(&self, other: &str) -> bool { + jid_display_eq(&self.user, self.server, self.agent, self.device, other) + } } #[cfg(feature = "serde")] @@ -963,6 +975,38 @@ fn write_jid_fallible( write_jid!(fallible w, user, server, agent, device) } +struct StrEqWriter<'a> { + target: &'a [u8], + position: usize, + matches: bool, +} + +impl fmt::Write for StrEqWriter<'_> { + #[inline] + fn write_str(&mut self, value: &str) -> fmt::Result { + if self.matches { + let bytes = value.as_bytes(); + let end = self.position + bytes.len(); + if end > self.target.len() || self.target[self.position..end] != *bytes { + self.matches = false; + } + self.position = end; + } + Ok(()) + } +} + +#[inline] +fn jid_display_eq(user: &str, server: Server, agent: u8, device: u16, other: &str) -> bool { + let mut writer = StrEqWriter { + target: other.as_bytes(), + position: 0, + matches: true, + }; + let written = write_jid_fallible(&mut writer, user, server, agent, device).is_ok(); + written && writer.matches && writer.position == other.len() +} + impl fmt::Display for Jid { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let mut w = JidStackWriter::new(); @@ -1114,6 +1158,24 @@ mod tests { use super::*; use std::str::FromStr; + #[test] + fn display_eq_matches_owned_and_borrowed_jids_without_normalizing() { + let canonical = "123456789.4:17@interop"; + let owned = Jid::from_str(canonical).unwrap(); + let borrowed = parse_jid_ref(canonical).unwrap(); + + for value in [canonical, "123456789.4:16@interop", "123456789@interop", ""] { + assert_eq!(owned.display_eq(value), value == canonical); + assert_eq!(borrowed.display_eq(value), value == canonical); + } + + let long_user = "a".repeat(128); + let long_value = format!("{long_user}@lid"); + let long_jid = Jid::lid(long_user); + assert!(long_jid.display_eq(&long_value)); + assert!(!long_jid.display_eq(&format!("{long_value}x"))); + } + #[cfg(feature = "serde")] #[test] fn server_deserializes_borrowed_and_owned_strings() { diff --git a/wacore/binary/src/node.rs b/wacore/binary/src/node.rs index 371b950e2..7c50dcee7 100644 --- a/wacore/binary/src/node.rs +++ b/wacore/binary/src/node.rs @@ -184,37 +184,7 @@ impl PartialEq for NodeValue { fn eq(&self, other: &str) -> bool { match self { NodeValue::String(s) => s == other, - // Compare JID to string without heap allocation by streaming the - // Display output through a writer that checks byte-by-byte. - NodeValue::Jid(j) => { - use std::fmt::Write; - struct EqCheck<'a> { - target: &'a [u8], - pos: usize, - matches: bool, - } - impl fmt::Write for EqCheck<'_> { - fn write_str(&mut self, s: &str) -> fmt::Result { - if !self.matches { - return Ok(()); - } - let bytes = s.as_bytes(); - let end = self.pos + bytes.len(); - if end > self.target.len() || self.target[self.pos..end] != *bytes { - self.matches = false; - } - self.pos = end; - Ok(()) - } - } - let mut check = EqCheck { - target: other.as_bytes(), - pos: 0, - matches: true, - }; - let _ = write!(check, "{}", j); - check.matches && check.pos == other.len() - } + NodeValue::Jid(j) => j.display_eq(other), } } } @@ -582,6 +552,30 @@ impl<'a> ValueRef<'a> { } } +impl PartialEq for ValueRef<'_> { + #[inline] + fn eq(&self, other: &str) -> bool { + match self { + ValueRef::String(value) => value == other, + ValueRef::Jid(value) => value.display_eq(other), + } + } +} + +impl PartialEq<&str> for ValueRef<'_> { + #[inline] + fn eq(&self, other: &&str) -> bool { + self == *other + } +} + +impl PartialEq for ValueRef<'_> { + #[inline] + fn eq(&self, other: &String) -> bool { + self == other.as_str() + } +} + use std::str::FromStr; impl<'a> fmt::Display for ValueRef<'a> { @@ -1027,6 +1021,29 @@ impl std::fmt::Debug for OwnedNodeRef { } } +#[cfg(test)] +mod value_ref_tests { + use super::*; + use crate::jid::Server; + + #[test] + fn value_ref_compares_string_and_jid_display_without_conversion() { + let string = ValueRef::String(NodeStr::Borrowed("encrypt")); + assert!(string == "encrypt"); + assert!(string != "other"); + + let jid = ValueRef::Jid(JidRef { + user: NodeStr::Borrowed("15551234567"), + server: Server::Pn, + agent: 0, + device: 7, + integrator: 0, + }); + assert!(jid == "15551234567:7@s.whatsapp.net"); + assert!(jid != "15551234567@s.whatsapp.net"); + } +} + #[cfg(test)] #[cfg(feature = "serde")] mod serde_tests { diff --git a/wacore/src/message_processing.rs b/wacore/src/message_processing.rs index d0b13f615..46acf8df7 100644 --- a/wacore/src/message_processing.rs +++ b/wacore/src/message_processing.rs @@ -96,8 +96,7 @@ pub struct CategorizedEncNodes<'a> { pub has_ordering_violation: bool, } -/// Maximum retry attempts per message (matches WhatsApp Web's MAX_RETRY = 5). -const MAX_DECRYPT_RETRIES: u8 = 5; +use crate::protocol::retry::MAX_RETRY_COUNT as MAX_DECRYPT_RETRIES; /// Categorize the `` child nodes of a message stanza into session (1:1) /// and group (sender-key) buckets. diff --git a/wacore/src/protocol/nack.rs b/wacore/src/protocol/nack.rs index 33524abd3..4da97889c 100644 --- a/wacore/src/protocol/nack.rs +++ b/wacore/src/protocol/nack.rs @@ -1,7 +1,6 @@ -//! Codes sent as the `error` attr on `` nacks -//! (`Handle/MsgSendAck.js` + `Create/NackFromStanza.js`). Server stops -//! retransmitting on receipt; use vs `` for -//! recoverable errors. +//! Codes sent as the `error` attribute on protocol rejection acknowledgements. +//! The server stops retransmitting on receipt; use `` +//! instead for recoverable errors. #[derive(Debug, Clone, Copy, PartialEq, Eq, crate::WireEnum)] #[wire(kind = "int")] @@ -66,4 +65,10 @@ mod tests { assert_eq!(NackReason::UnsupportedLIDGroup.code(), 551); assert_eq!(NackReason::DBOperationFailed.code(), 552); } + + #[test] + fn nack_reason_preserves_unknown_numeric_codes() { + assert_eq!(NackReason::from(599), NackReason::Unknown(599)); + assert_eq!(NackReason::Unknown(599).code(), 599); + } } diff --git a/wacore/src/protocol/retry.rs b/wacore/src/protocol/retry.rs index 7e6cf1eae..524867105 100644 --- a/wacore/src/protocol/retry.rs +++ b/wacore/src/protocol/retry.rs @@ -125,16 +125,26 @@ pub fn extract_registration_id_from_node_ref(node: &NodeRef<'_>) -> Option parse_registration_id(registration_node.content_bytes()?) } -/// Returns whether keys should be included in a retry receipt for the given -/// retry count and reason. +/// Whether a normal stateful retry receipt carries the local key bundle. /// -/// WhatsApp Web only includes keys when `retryCount >= 2`. As an optimization, -/// keys are included on retry #1 for `NoSession` errors to reduce round-trips -/// for skmsg-only message failures. -pub fn should_include_keys(retry_count: u8, reason: RetryReason) -> bool { - let include_keys_early = - reason == RetryReason::NoSession || reason == RetryReason::UnknownCompanionNoPrekey; - retry_count >= MIN_RETRY_COUNT_FOR_KEYS || include_keys_early +/// The diagnostic reason does not affect key distribution. Use +/// [`should_include_keys_with_policy`] when the caller has an explicit force +/// request or stateless destination. +pub fn should_include_keys(retry_count: u8, _reason: RetryReason) -> bool { + should_include_keys_with_policy(retry_count, false, false) +} + +/// Whether a retry receipt carries the local key bundle under the full policy. +/// +/// The observed sender has only two early-inclusion inputs: an explicit force +/// request and stateless addressing. The retry reason remains an independent +/// diagnostic attribute and does not affect key distribution. +pub fn should_include_keys_with_policy( + retry_count: u8, + force_include_keys: bool, + is_stateless: bool, +) -> bool { + force_include_keys || is_stateless || retry_count >= MIN_RETRY_COUNT_FOR_KEYS } /// Whether a retry from a device that is not in our device registry must be @@ -333,41 +343,29 @@ mod tests { } #[test] - fn should_include_keys_no_session_retry_1() { - assert!( - should_include_keys(1, RetryReason::NoSession), - "NoSession at retry#1 should include keys (optimization)" - ); + fn should_not_include_keys_on_first_normal_retry() { + assert!(!should_include_keys(1, RetryReason::NoSession)); + assert!(!should_include_keys(1, RetryReason::InvalidMessage)); } #[test] - fn should_include_keys_unknown_companion_retry_1() { - assert!( - should_include_keys(1, RetryReason::UnknownCompanionNoPrekey), - "UnknownCompanionNoPrekey at retry#1 should include keys" - ); + fn should_include_keys_when_forced() { + assert!(should_include_keys_with_policy(1, true, false)); } #[test] - fn should_include_keys_invalid_message_retry_1() { - assert!( - !should_include_keys(1, RetryReason::InvalidMessage), - "InvalidMessage at retry#1 should NOT include keys" - ); + fn should_include_keys_for_stateless_recipient() { + assert!(should_include_keys_with_policy(1, false, true)); } #[test] - fn should_include_keys_retry_2_any_reason() { - assert!(should_include_keys(2, RetryReason::InvalidMessage)); + fn should_include_keys_at_retry_threshold() { assert!(should_include_keys(2, RetryReason::UnknownError)); - assert!(should_include_keys(2, RetryReason::BadMac)); - assert!(should_include_keys(2, RetryReason::NoSession)); } #[test] - fn should_include_keys_retry_3_any_reason() { - assert!(should_include_keys(3, RetryReason::InvalidMessage)); - assert!(should_include_keys(3, RetryReason::UnknownError)); + fn should_include_keys_after_retry_threshold() { + assert!(should_include_keys(3, RetryReason::BadMac)); } #[test] From 43a80b9140b911601e02c98b39159e432f777064 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= <55464917+jlucaso1@users.noreply.github.com> Date: Tue, 21 Jul 2026 21:16:08 -0300 Subject: [PATCH 2/8] fix(core): tighten stanza response semantics --- src/client.rs | 10 ++-- src/client/node_io.rs | 27 +++++----- src/client/tests.rs | 47 ++++++++++++++-- src/message/tests.rs | 14 ++++- tests/e2e/tests/memory_soak.rs | 6 +-- wacore/binary/src/attrs.rs | 25 +++++++++ wacore/binary/src/jid.rs | 98 ++++++++++++++++++++++++++++++++++ wacore/src/messages.rs | 33 ++++++++++-- 8 files changed, 231 insertions(+), 29 deletions(-) diff --git a/src/client.rs b/src/client.rs index d8d136367..20aa50ed6 100644 --- a/src/client.rs +++ b/src/client.rs @@ -1054,7 +1054,7 @@ fn value_refs_display_equal( match (left, right) { (ValueRef::String(left), ValueRef::String(right)) => left == right, - (ValueRef::Jid(left), ValueRef::Jid(right)) => left == right, + (ValueRef::Jid(left), ValueRef::Jid(right)) => left.display_eq_jid(right), (ValueRef::String(left), ValueRef::Jid(right)) => right.display_eq(left), (ValueRef::Jid(left), ValueRef::String(right)) => left.display_eq(right), } @@ -1065,8 +1065,8 @@ fn value_refs_display_equal( /// - `class` = original stanza tag /// - `id`, `to` (flipped from `from`), `participant` copied from original /// - `from` = own device PN, only for message acks -/// - `type` echoed for non-message stanzas (whatsmeow: `node.Tag != "message"`), -/// except `notification type="encrypt"` with `` child (WA Web drops type there). +/// - `type` echoed when present, except `notification type="encrypt"` with +/// an `` child /// /// For receipt acks, WA Web uses `MAYBE_CUSTOM_STRING(ackString)` where /// `ackString = maybeAttrString("type")` — so `type` is only included when @@ -1104,7 +1104,7 @@ fn encode_ack_bytes( // Dropping it makes the server close the stream with ``. let recipient_val = node.get_attr("recipient"); - let typ_val = if tag != "message" && !is_encrypt_identity_notification(node) { + let typ_val = if !is_encrypt_identity_notification(node) { node.get_attr("type") } else { None @@ -1234,7 +1234,7 @@ fn build_ack_node(node: &wacore_binary::NodeRef<'_>, own_device_pn: Option<&Jid> .filter(|participant| tag != "receipt" || !value_refs_display_equal(participant, from_ref)) .map(|v| v.to_node_value()); let recipient = node.get_attr("recipient").map(|v| v.to_node_value()); - let typ = if tag != "message" && !is_encrypt_identity_notification(node) { + let typ = if !is_encrypt_identity_notification(node) { node.get_attr("type").map(|v| v.to_node_value()) } else { None diff --git a/src/client/node_io.rs b/src/client/node_io.rs index 62f658d7f..fcf4dcee6 100644 --- a/src/client/node_io.rs +++ b/src/client/node_io.rs @@ -589,6 +589,17 @@ impl Client { } } + #[inline] + fn encode_ack_from_snapshot( + &self, + node: &wacore_binary::NodeRef<'_>, + ) -> Result, crate::features::StanzaResponseError> { + let device = self.persistence_manager.get_device_snapshot(); + let encoded = encode_ack_bytes(node, device.pn.as_ref()); + drop(device); + encoded + } + /// Build and send an node corresponding to the given stanza. #[cfg_attr( feature = "tracing", @@ -604,15 +615,13 @@ impl Client { if !self.is_connected() { return Err(ClientError::NotConnected); } - let device = self.persistence_manager.get_device_snapshot(); - let buf = match encode_ack_bytes(node, device.pn.as_ref()) { + let buf = match self.encode_ack_from_snapshot(node) { Ok(buf) => buf, Err(e) => { log::warn!("Failed to encode ack: {e}"); return Ok(()); } }; - drop(device); self.send_raw_bytes(buf).await } @@ -629,9 +638,7 @@ impl Client { &self, stanza: &wacore_binary::NodeRef<'_>, ) -> Result<(), crate::features::StanzaResponseError> { - let device = self.persistence_manager.get_device_snapshot(); - let bytes = encode_ack_bytes(stanza, device.pn.as_ref())?; - drop(device); + let bytes = self.encode_ack_from_snapshot(stanza)?; self.send_raw_bytes(bytes).await?; Ok(()) } @@ -641,9 +648,7 @@ impl Client { /// in a single flushed task. pub(crate) async fn send_transport_ack(&self, info: &crate::types::message::MessageInfo) { let source = message_ack_source_node(info); - let device = self.persistence_manager.get_device_snapshot(); - let encoded = encode_ack_bytes(&source.as_node_ref(), device.pn.as_ref()); - drop(device); + let encoded = self.encode_ack_from_snapshot(&source.as_node_ref()); match encoded { Ok(buf) => { if let Err(e) = self.send_raw_bytes(buf).await @@ -676,15 +681,13 @@ impl Client { self: &Arc, node: &wacore_binary::NodeRef<'_>, ) { - let device = self.persistence_manager.get_device_snapshot(); - let buf = match encode_ack_bytes(node, device.pn.as_ref()) { + let buf = match self.encode_ack_from_snapshot(node) { Ok(buf) => buf, Err(e) => { log::warn!("Failed to encode node transport ack: {e}"); return; } }; - drop(device); let client = Arc::clone(self); self.outbound_flush.spawn(&*self.runtime, async move { if let Err(e) = client.send_raw_bytes(buf).await diff --git a/src/client/tests.rs b/src/client/tests.rs index 501be7d78..cdda2cfd1 100644 --- a/src/client/tests.rs +++ b/src/client/tests.rs @@ -2081,9 +2081,9 @@ fn test_device_notification_is_not_encrypt_identity() { } #[test] -fn test_build_ack_node_for_message_omits_type_includes_from() { - // Whatsmeow: message acks do NOT echo type (node.Tag != "message" guard). - // They DO include `from` with own device PN. +fn test_build_ack_node_for_message_preserves_type_and_includes_from() { + // Generic message acknowledgements echo the stanza type and identify the + // local device in `from`. let incoming = NodeBuilder::new("message") .attr("from", "120363161500776365@g.us") .attr("id", "A5791A5392EF60E3FB0670098DE010D4") @@ -2117,8 +2117,8 @@ fn test_build_ack_node_for_message_omits_type_includes_from() { .is_some_and(|v| v == "181531758878822@lid") ); assert!( - !ack.attrs.contains_key("type"), - "message ACK must NOT echo type (matches whatsmeow behavior)" + ack.attrs.get("type").is_some_and(|v| v == "text"), + "message ACK must echo its explicit type" ); } @@ -2351,6 +2351,12 @@ fn test_encode_ack_bytes_roundtrip_recipient() { .is_some_and(|v| v.as_str() == "146991363395800@lid"), "encode_ack_bytes must echo `recipient` onto the wire" ); + assert!( + decoded + .get_attr("type") + .is_some_and(|value| value.as_str() == "text"), + "generic message ACK must echo its explicit type" + ); let without_recipient = NodeBuilder::new("message") .attr("from", "120363161500776365@g.us") @@ -2442,6 +2448,37 @@ fn test_encode_ack_bytes_preserves_specialized_receipt_rules() { ); } +#[test] +fn test_encode_ack_bytes_compares_jid_participants_by_display() { + let from = Jid { + user: "15551234567".into(), + server: wacore_binary::Server::Hosted, + agent: 1, + device: 7, + integrator: 0, + }; + let participant = Jid { + agent: 2, + ..from.clone() + }; + assert_eq!(from.to_string(), participant.to_string()); + + let receipt = NodeBuilder::new("receipt") + .attr("id", "DISPLAY-EQUIVALENT-PARTICIPANT") + .attr("from", &from) + .attr("participant", &participant) + .build(); + let bytes = encode_ack_bytes(&receipt.as_node_ref(), None) + .expect("complete receipt should produce an ack"); + let ack = wacore_binary::marshal::unmarshal_ref(&bytes[1..]) + .expect("encoded receipt ack should decode"); + + assert!( + ack.get_attr("participant").is_none(), + "receipt ack must omit display-equivalent participant JIDs" + ); +} + #[test] fn test_encode_ack_bytes_drops_encrypt_identity_notification_type() { let notification = NodeBuilder::new("notification") diff --git a/src/message/tests.rs b/src/message/tests.rs index 4b941f21d..0de8ba932 100644 --- a/src/message/tests.rs +++ b/src/message/tests.rs @@ -5567,6 +5567,7 @@ async fn explicit_stanza_responses_use_the_canonical_wire_paths() { let message = NodeBuilder::new("message") .attr("id", "EXPLICIT-ACK") .attr("from", "15551234567:7@s.whatsapp.net") + .attr("participant", "15557654321:4@s.whatsapp.net") .attr("recipient", "5511000000001@s.whatsapp.net") .attr("type", "text") .build(); @@ -5612,7 +5613,18 @@ async fn explicit_stanza_responses_use_the_canonical_wire_paths() { ack.get_attr("from") .is_some_and(|value| value.as_str() == "5511000000001@s.whatsapp.net") ); - assert!(ack.get_attr("type").is_none()); + assert!( + ack.get_attr("participant") + .is_some_and(|value| value.as_str() == "15557654321:4@s.whatsapp.net") + ); + assert!( + ack.get_attr("recipient") + .is_some_and(|value| value.as_str() == "5511000000001@s.whatsapp.net") + ); + assert!( + ack.get_attr("type") + .is_some_and(|value| value.as_str() == "text") + ); let nack_bytes = decode_frame(1, &frames[1]).expect("nack frame should decrypt"); let nack = diff --git a/tests/e2e/tests/memory_soak.rs b/tests/e2e/tests/memory_soak.rs index b467293bf..e2e87b2dd 100644 --- a/tests/e2e/tests/memory_soak.rs +++ b/tests/e2e/tests/memory_soak.rs @@ -207,10 +207,10 @@ fn analyze_growth(label: &str, snapshots: &[Snapshot]) { #[cfg(feature = "dhat-heap")] { - let heap_growth = last.heap_bytes.saturating_sub(first.heap_bytes); + let heap_delta = last.heap_bytes as i128 - first.heap_bytes as i128; info!( - " Tracked heap: {}B -> {}B (delta: +{}B)", - first.heap_bytes, last.heap_bytes, heap_growth + " Tracked heap: {}B -> {}B (delta: {heap_delta:+}B)", + first.heap_bytes, last.heap_bytes ); } diff --git a/wacore/binary/src/attrs.rs b/wacore/binary/src/attrs.rs index 9f346ece6..272279a9c 100644 --- a/wacore/binary/src/attrs.rs +++ b/wacore/binary/src/attrs.rs @@ -92,6 +92,20 @@ impl<'a> AttrParserRef<'a> { }) } + /// Get a required JID attribute, failing immediately when it is missing or invalid. + /// + /// Structured JIDs are converted directly from their decoded representation; + /// string attributes are parsed exactly once. + pub fn required_jid(&mut self, key: &str) -> Result { + match self + .get_raw(key, false) + .ok_or_else(|| BinaryError::MissingAttr(key.to_string()))? + { + ValueRef::Jid(jid) => Ok(jid.to_owned()), + ValueRef::String(value) => Jid::from_str(value).map_err(BinaryError::from), + } + } + pub fn jid(&mut self, key: &str) -> Jid { self.get_raw(key, true); self.optional_jid(key).unwrap_or_default() @@ -248,6 +262,17 @@ impl<'a> AttrParser<'a> { }) } + /// Get a required JID attribute, failing immediately when it is missing or invalid. + pub fn required_jid(&mut self, key: &str) -> Result { + match self + .get_raw(key, false) + .ok_or_else(|| BinaryError::MissingAttr(key.to_string()))? + { + NodeValue::Jid(jid) => Ok(jid.clone()), + NodeValue::String(value) => Jid::from_str(value).map_err(BinaryError::from), + } + } + pub fn jid(&mut self, key: &str) -> Jid { self.get_raw(key, true); // Push "not found" error if needed. self.optional_jid(key).unwrap_or_default() diff --git a/wacore/binary/src/jid.rs b/wacore/binary/src/jid.rs index fb63268af..76c41426b 100644 --- a/wacore/binary/src/jid.rs +++ b/wacore/binary/src/jid.rs @@ -715,6 +715,15 @@ impl Jid { jid_display_eq(&self.user, self.server, self.agent, self.device, other) } + /// Compare two JIDs by the representation emitted by [`fmt::Display`]. + #[inline] + pub fn display_eq_jid(&self, other: &Self) -> bool { + jid_displays_equal( + (&self.user, self.server, self.agent, self.device), + (&other.user, other.server, other.agent, other.device), + ) + } + /// Compare device identity (user, server, device) without allocation. #[inline] pub fn device_eq(&self, other: &Jid) -> bool { @@ -771,6 +780,15 @@ impl<'a> JidRef<'a> { pub fn display_eq(&self, other: &str) -> bool { jid_display_eq(&self.user, self.server, self.agent, self.device, other) } + + /// Compare two borrowed JIDs by the representation emitted by [`fmt::Display`]. + #[inline] + pub fn display_eq_jid(&self, other: &Self) -> bool { + jid_displays_equal( + (&self.user, self.server, self.agent, self.device), + (&other.user, other.server, other.agent, other.device), + ) + } } #[cfg(feature = "serde")] @@ -1007,6 +1025,20 @@ fn jid_display_eq(user: &str, server: Server, agent: u8, device: u16, other: &st written && writer.matches && writer.position == other.len() } +#[inline] +fn jid_displays_equal(left: (&str, Server, u8, u16), right: (&str, Server, u8, u16)) -> bool { + let (left_user, left_server, left_agent, left_device) = left; + let (right_user, right_server, right_agent, right_device) = right; + if left_user.is_empty() || right_user.is_empty() { + return left_user.is_empty() && right_user.is_empty() && left_server == right_server; + } + + left_user == right_user + && left_server == right_server + && left_device == right_device + && (!left_server.renders_agent() || left_agent == right_agent) +} + impl fmt::Display for Jid { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let mut w = JidStackWriter::new(); @@ -1176,6 +1208,72 @@ mod tests { assert!(!long_jid.display_eq(&format!("{long_value}x"))); } + #[test] + fn display_eq_jid_uses_only_rendered_components() { + let pn = Jid { + user: "15551234567".into(), + server: Server::Pn, + agent: 1, + device: 7, + integrator: 3, + }; + let pn_same_display = Jid { + agent: 2, + integrator: 9, + ..pn.clone() + }; + assert_eq!(pn.to_string(), pn_same_display.to_string()); + assert!(pn.display_eq_jid(&pn_same_display)); + + let pn_other_device = Jid { + device: 8, + ..pn.clone() + }; + assert!(!pn.display_eq_jid(&pn_other_device)); + + let bot = Jid { + user: "13136555001".into(), + server: Server::Bot, + agent: 1, + device: 0, + integrator: 0, + }; + let other_bot_agent = Jid { + agent: 2, + ..bot.clone() + }; + assert!(!bot.display_eq_jid(&other_bot_agent)); + + let server_only = Jid { + user: "".into(), + server: Server::Pn, + agent: 1, + device: 7, + integrator: 3, + }; + let same_server_only = Jid { + agent: 2, + device: 9, + integrator: 4, + ..server_only.clone() + }; + assert_eq!(server_only.to_string(), same_server_only.to_string()); + assert!(server_only.display_eq_jid(&same_server_only)); + + let borrowed = JidRef { + user: NodeStr::Borrowed("15551234567"), + server: Server::Pn, + agent: 1, + device: 7, + integrator: 0, + }; + let borrowed_same_display = JidRef { + agent: 2, + ..borrowed.clone() + }; + assert!(borrowed.display_eq_jid(&borrowed_same_display)); + } + #[cfg(feature = "serde")] #[test] fn server_deserializes_borrowed_and_owned_strings() { diff --git a/wacore/src/messages.rs b/wacore/src/messages.rs index 2623f526d..3edca1ce2 100644 --- a/wacore/src/messages.rs +++ b/wacore/src/messages.rs @@ -919,13 +919,13 @@ pub fn parse_message_info( use wacore_binary::{JidExt as _, STATUS_BROADCAST_USER, Server}; let mut attrs = node.attrs(); - let from = attrs.jid("from"); + let from = attrs.required_jid("from")?; let addressing_mode = attrs .optional_string("addressing_mode") .and_then(|s| AddressingMode::try_from(s.as_ref()).ok()); let mut source = if from.server == Server::Broadcast { - let participant = attrs.jid("participant"); + let participant = attrs.required_jid("participant")?; let is_from_me = participant.matches_user_or_lid(own_jid, own_lid); // Match WAWebMsgParser: read participant_lid/_pn unconditionally so @@ -952,7 +952,7 @@ pub fn parse_message_info( ..Default::default() } } else if from.is_group() { - let sender = attrs.jid("participant"); + let sender = attrs.required_jid("participant")?; let sender_alt = match addressing_mode { Some(AddressingMode::Lid) => attrs.optional_jid("participant_pn"), Some(AddressingMode::Pn) => attrs.optional_jid("participant_lid"), @@ -1314,6 +1314,33 @@ mod parse_message_info_tests { use wacore_binary::Jid; use wacore_binary::builder::NodeBuilder; + #[test] + fn invalid_routing_jids_are_rejected() { + let own_pn = Jid::from_str("559900000000@s.whatsapp.net").unwrap(); + let cases = [ + NodeBuilder::new("message") + .attr("from", "not-a-jid") + .attr("id", "INVALID-FROM") + .build(), + NodeBuilder::new("message") + .attr("from", "120363021033254949@g.us") + .attr("id", "MISSING-PARTICIPANT") + .build(), + NodeBuilder::new("message") + .attr("from", "120363021033254949@g.us") + .attr("participant", "not-a-jid") + .attr("id", "INVALID-PARTICIPANT") + .build(), + ]; + + for node in &cases { + assert!( + parse_message_info(&node.as_node_ref(), &own_pn, None).is_err(), + "invalid routing attributes must not produce default JIDs: {node:?}" + ); + } + } + #[test] fn status_broadcast_with_participant_lid_populates_sender_alt() { let own_pn = Jid::from_str("559900000000@s.whatsapp.net").unwrap(); From c4f2e7ddbb32323b3fabd28c9cfedbd80efa8b21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= <55464917+jlucaso1@users.noreply.github.com> Date: Tue, 21 Jul 2026 21:28:34 -0300 Subject: [PATCH 3/8] test(core): standardize fictional jid fixtures --- src/client/tests.rs | 2 +- wacore/binary/src/jid.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/client/tests.rs b/src/client/tests.rs index cdda2cfd1..359c3f96f 100644 --- a/src/client/tests.rs +++ b/src/client/tests.rs @@ -2451,7 +2451,7 @@ fn test_encode_ack_bytes_preserves_specialized_receipt_rules() { #[test] fn test_encode_ack_bytes_compares_jid_participants_by_display() { let from = Jid { - user: "15551234567".into(), + user: "12025550111".into(), server: wacore_binary::Server::Hosted, agent: 1, device: 7, diff --git a/wacore/binary/src/jid.rs b/wacore/binary/src/jid.rs index 76c41426b..34243178a 100644 --- a/wacore/binary/src/jid.rs +++ b/wacore/binary/src/jid.rs @@ -1211,7 +1211,7 @@ mod tests { #[test] fn display_eq_jid_uses_only_rendered_components() { let pn = Jid { - user: "15551234567".into(), + user: "12025550111".into(), server: Server::Pn, agent: 1, device: 7, @@ -1261,7 +1261,7 @@ mod tests { assert!(server_only.display_eq_jid(&same_server_only)); let borrowed = JidRef { - user: NodeStr::Borrowed("15551234567"), + user: NodeStr::Borrowed("12025550111"), server: Server::Pn, agent: 1, device: 7, From d05996cd683ab87ede7c99657d54c7975f6a2318 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= <55464917+jlucaso1@users.noreply.github.com> Date: Tue, 21 Jul 2026 23:05:09 -0300 Subject: [PATCH 4/8] fix(core): harden inbound stanza recovery --- src/client/lifecycle.rs | 5 +- src/message/receive.rs | 11 +- src/message/retry.rs | 15 +++ src/message/tests.rs | 199 +++++++++++++++++++++++++------------ src/receipt.rs | 77 +++++++++----- wacore/binary/src/attrs.rs | 20 ++-- wacore/src/messages.rs | 7 +- 7 files changed, 228 insertions(+), 106 deletions(-) diff --git a/src/client/lifecycle.rs b/src/client/lifecycle.rs index 4d7de6912..23a2339d7 100644 --- a/src/client/lifecycle.rs +++ b/src/client/lifecycle.rs @@ -990,9 +990,8 @@ impl Client { self.is_connected.load(Ordering::Acquire) } - /// Force the connected flag (tests only): the facade's connect path now gates on `is_connected`, - /// so a unit test driving `spawn_call`/`place_call` must mark the client connected first. - #[cfg(all(test, feature = "voip-runtime"))] + /// Force the connected flag for tests that exercise connected-only operations. + #[cfg(test)] pub(crate) fn set_connected_for_test(&self, connected: bool) { self.is_connected.store(connected, Ordering::Release); } diff --git a/src/message/receive.rs b/src/message/receive.rs index e7d341ebb..89275dc9c 100644 --- a/src/message/receive.rs +++ b/src/message/receive.rs @@ -125,6 +125,7 @@ impl Client { let id = nr.get_attr("id").map(|v| v.as_str()); let from = nr.get_attr("from").map(|v| v.as_str()); log::warn!("Failed to parse message info (id={id:?}, from={from:?}): {e:?}"); + self.spawn_stanza_nack(nr, NackReason::ParsingError, None); return None; } }; @@ -414,14 +415,8 @@ impl Client { let cache_key = self .make_retry_cache_key(&info.source.chat, &info.id, &info.source.sender) .await; - let existing = self.message_retry_counts.get(&cache_key).await; - if max_sender_retry_count > existing.map_or(0, |(count, _)| count) { - // Keep any locally recorded reason; the echoed count carries none. - let reason = existing.and_then(|(_, reason)| reason); - self.message_retry_counts - .insert(cache_key, (max_sender_retry_count, reason)) - .await; - } + self.preseed_retry_count(&cache_key, max_sender_retry_count) + .await; log::debug!( "[msg:{}] Sender retry count {} pre-seeded into cache", info.id, diff --git a/src/message/retry.rs b/src/message/retry.rs index 43b55b455..dc7dbb674 100644 --- a/src/message/retry.rs +++ b/src/message/retry.rs @@ -26,6 +26,9 @@ impl Client { if stanza.get_attr("from").is_none() { return Err(crate::features::RetryRequestError::MissingAttribute("from")); } + if !self.is_connected() { + return Err(crate::client::ClientError::NotConnected.into()); + } let device = self.persistence_manager.get_device_snapshot(); let own_pn = device @@ -179,6 +182,18 @@ impl Client { .await } + /// Raise the local retry count to a sender-echoed count without allowing a + /// concurrent local increment to be overwritten. + pub(crate) async fn preseed_retry_count(&self, cache_key: &str, sender_count: u8) { + self.message_retry_counts + .upsert_with_by_ref(cache_key, |current| match current { + Some((count, _)) if *count >= sender_count => (None, ()), + Some((_, reason)) => (Some((sender_count, *reason)), ()), + None => (Some((sender_count, None)), ()), + }) + .await; + } + /// Generate consistent cache key for retry logic. pub(crate) async fn make_retry_cache_key( &self, diff --git a/src/message/tests.rs b/src/message/tests.rs index 0de8ba932..059a59e07 100644 --- a/src/message/tests.rs +++ b/src/message/tests.rs @@ -4568,24 +4568,13 @@ async fn test_enc_count_preseeds_retry_cache() { let chat_jid: Jid = "5551234567@s.whatsapp.net".parse().unwrap(); let msg_id = "ENC_COUNT_MSG1"; - // Pre-seed via the same logic used in handle_incoming_message let max_sender_retry_count: u8 = 3; let cache_key = client .make_retry_cache_key(&chat_jid, msg_id, &chat_jid) .await; - // Insert only if absent (get-then-insert; the cache has no atomic upsert) - if client - .message_retry_counts - .get(&cache_key) - .await - .map(|(c, _)| c) - .is_none() - { - client - .message_retry_counts - .insert(cache_key.clone(), (max_sender_retry_count, None)) - .await; - } + client + .preseed_retry_count(&cache_key, max_sender_retry_count) + .await; assert_eq!( client @@ -4611,18 +4600,9 @@ async fn test_enc_no_count_cache_empty() { let cache_key = client .make_retry_cache_key(&chat_jid, msg_id, &chat_jid) .await; - if client - .message_retry_counts - .get(&cache_key) - .await - .map(|(c, _)| c) - .is_none() - { - client - .message_retry_counts - .insert(cache_key, (max_sender_retry_count, None)) - .await; - } + client + .preseed_retry_count(&cache_key, max_sender_retry_count) + .await; } let cache_key = client @@ -4656,20 +4636,10 @@ async fn test_enc_count_does_not_overwrite_higher() { .insert(cache_key.clone(), (4, None)) .await; - // max(existing, incoming) should NOT overwrite with a lower value let max_sender_retry_count: u8 = 2; - let existing = client - .message_retry_counts - .get(&cache_key) - .await - .map(|(c, _)| c) - .unwrap_or(0); - if max_sender_retry_count > existing { - client - .message_retry_counts - .insert(cache_key.clone(), (max_sender_retry_count, None)) - .await; - } + client + .preseed_retry_count(&cache_key, max_sender_retry_count) + .await; assert_eq!( client @@ -4693,38 +4663,49 @@ async fn test_enc_count_updates_when_sender_higher() { .make_retry_cache_key(&chat_jid, msg_id, &chat_jid) .await; - // Pre-insert a lower value + // Pre-insert a lower value and a local reason that the echoed count must preserve. client .message_retry_counts - .insert(cache_key.clone(), (1, None)) + .insert(cache_key.clone(), (1, Some(RetryReason::BadMac))) .await; - // max(existing, incoming) SHOULD update with a higher value let max_sender_retry_count: u8 = 3; - let existing = client - .message_retry_counts - .get(&cache_key) - .await - .map(|(c, _)| c) - .unwrap_or(0); - if max_sender_retry_count > existing { - client - .message_retry_counts - .insert(cache_key.clone(), (max_sender_retry_count, None)) - .await; - } + client + .preseed_retry_count(&cache_key, max_sender_retry_count) + .await; assert_eq!( - client - .message_retry_counts - .get(&cache_key) - .await - .map(|(c, _)| c), - Some(3), - "should update to higher sender count" + client.message_retry_counts.get(&cache_key).await, + Some((3, Some(RetryReason::BadMac))), + "should update to the higher sender count without losing the local reason" ); } +#[tokio::test] +async fn test_enc_count_preseed_cannot_overwrite_concurrent_increments() { + let client = create_test_client_for_retry_with_id("enc_atomic_preseed").await; + let cache_key = "enc_atomic_preseed:msg:sender"; + client + .message_retry_counts + .insert(cache_key.to_owned(), (3, Some(RetryReason::NoSession))) + .await; + + let (_, first, second) = futures::join!( + client.preseed_retry_count(cache_key, 4), + client.increment_retry_count(cache_key, RetryReason::BadMac), + client.increment_retry_count(cache_key, RetryReason::InvalidMessage), + ); + + assert!(first.is_some() || second.is_some()); + let final_state = client + .message_retry_counts + .get(cache_key) + .await + .expect("retry state"); + assert_eq!(final_state.0, MAX_DECRYPT_RETRIES); + assert!(final_state.1.is_some(), "a local reason must be retained"); +} + /// Shared helper: the OLD semaphore acquire logic that silently dropped tasks /// on generation mismatch. Used by the bug-demonstration test. async fn acquire_permit_old_behavior( @@ -5550,9 +5531,8 @@ async fn capturing_client( write_key, read_key, ); - // send_node only needs noise_socket Some; is_connected is read by - // other layers but not on this path. *client.noise_socket.lock().await = Some(Arc::new(noise_socket)); + client.set_connected_for_test(true); seed_test_pn(&client).await; // Live-path semantics by default; drain tests re-enter drain state // themselves. @@ -6092,6 +6072,21 @@ async fn explicit_retry_validates_input_and_reports_the_shared_limit() { Err(crate::features::RetryRequestError::MissingAttribute("from")) )); + let invalid_self_recipient = NodeBuilder::new("message") + .attr("id", "INVALID-SELF-RECIPIENT") + .attr("from", "5511000000001:9@s.whatsapp.net") + .attr("recipient", "not-a-jid") + .build(); + assert!(matches!( + client + .request_message_retry( + &invalid_self_recipient.as_node_ref(), + crate::features::RetryRequestOptions::default(), + ) + .await, + Err(crate::features::RetryRequestError::InvalidStanza(_)) + )); + let stanza = retry_request_stanza("RETRY-LIMIT"); let sender: Jid = "15551234567:7@s.whatsapp.net".parse().expect("sender"); let chat = sender.to_non_ad(); @@ -6115,6 +6110,50 @@ async fn explicit_retry_validates_input_and_reports_the_shared_limit() { assert_eq!(transport.sent_count(), 0); } +#[tokio::test] +async fn explicit_retry_while_disconnected_preserves_budget_and_prekeys() { + let (client, transport) = capturing_client("explicit_retry_disconnected").await; + client + .persistence_manager + .process_command(crate::store::commands::DeviceCommand::SetAccount(Some( + wa::ADVSignedDeviceIdentity::default(), + ))) + .await; + client.set_connected_for_test(false); + + let stanza = retry_request_stanza("EXPLICIT-RETRY-DISCONNECTED"); + let before = client.persistence_manager.get_device_snapshot(); + let before_next_pre_key_id = before.next_pre_key_id; + let before_first_unupload_pre_key_id = before.first_unupload_pre_key_id; + drop(before); + + assert!(matches!( + client + .request_message_retry( + &stanza.as_node_ref(), + crate::features::RetryRequestOptions::new().with_force_include_keys(true), + ) + .await, + Err(crate::features::RetryRequestError::Client( + crate::client::ClientError::NotConnected + )) + )); + + let sender: Jid = "15551234567:7@s.whatsapp.net".parse().expect("sender"); + let cache_key = client + .make_retry_cache_key(&sender.to_non_ad(), "EXPLICIT-RETRY-DISCONNECTED", &sender) + .await; + assert!(client.message_retry_counts.get(&cache_key).await.is_none()); + assert_eq!(transport.sent_count(), 0); + + let after = client.persistence_manager.get_device_snapshot(); + assert_eq!(after.next_pre_key_id, before_next_pre_key_id); + assert_eq!( + after.first_unupload_pre_key_id, + before_first_unupload_pre_key_id + ); +} + #[tokio::test] async fn explicit_retry_preserves_transport_error_chain() { use std::error::Error as _; @@ -8081,6 +8120,40 @@ async fn own_bot_author_dm_acks_not_sender_receipt() { } } +#[tokio::test] +async fn malformed_group_envelopes_are_rejected_from_the_original_stanza() { + let (client, transport) = capturing_client("malformed_group_envelope").await; + let expected_error = + u32::try_from(NackReason::ParsingError.code()).expect("NACK error codes are positive"); + + for (id, participant) in [ + ("GROUP-MISSING-PARTICIPANT", None), + ("GROUP-INVALID-PARTICIPANT", Some("not-a-jid")), + ] { + let mut builder = NodeBuilder::new("message") + .attr("from", "120363021033254949@g.us") + .attr("id", id) + .attr("type", "text"); + if let Some(participant) = participant { + builder = builder.attr("participant", participant); + } + let owned = node_to_arc(builder.build()); + + assert!(client.classify_incoming_message(&owned).await.is_none()); + + let mut nack_error = None; + for _ in 0..80 { + nack_error = find_message_nack_error(&transport.sent(), id); + if nack_error.is_some() { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + assert_eq!(nack_error, Some(expected_error)); + assert_eq!(message_acks_for(&transport.sent(), id), 0); + } +} + /// An `` message (no ``) must be transport-acked so the /// server stops replaying it (DM/group aren't covered by the should_ack gate). #[tokio::test] diff --git a/src/receipt.rs b/src/receipt.rs index 4b80f0ebe..70a9802ee 100644 --- a/src/receipt.rs +++ b/src/receipt.rs @@ -743,6 +743,48 @@ impl Client { .detach(); } + fn build_nack_from_snapshot( + &self, + source: &S, + reason: NackReason, + failure_reason: Option, + ) -> Result { + let device = self.persistence_manager.get_device_snapshot(); + let own_pn = device + .pn + .as_ref() + .ok_or(crate::features::StanzaResponseError::MissingLocalIdentity)?; + let nack = build_nack_node(source, own_pn, reason, failure_reason); + drop(device); + nack + } + + /// Reject a malformed stanza without retaining or cloning its decoded tree. + pub(crate) fn spawn_stanza_nack( + self: &Arc, + stanza: &NodeRef<'_>, + reason: NackReason, + failure_reason: Option, + ) { + let nack = match self.build_nack_from_snapshot(stanza, reason, failure_reason) { + Ok(nack) => nack, + Err(error) => { + log::warn!(target: "Client/Receipt", "Failed to build stanza nack: {error}"); + return; + } + }; + let client = Arc::clone(self); + self.runtime + .spawn(Box::pin(async move { + if let Err(error) = client.send_node(nack).await + && !matches!(error, crate::client::ClientError::NotConnected) + { + log::warn!(target: "Client/Receipt", "Failed to send stanza nack: {error:?}"); + } + })) + .detach(); + } + /// Emits a nack so the server stops retransmitting an unrecoverable /// failure. Prefer [`Client::send_retry_receipt`] for recoverable /// errors (BadMac, NoSession, etc). @@ -756,25 +798,22 @@ impl Client { if info.id.is_empty() { return; } - let device = self.persistence_manager.get_device_snapshot(); - let Some(own_pn) = device.pn.as_ref() else { - log::debug!( - "[msg:{}] Skipping nack ({:?}): own PN not yet set", - info.id, - reason - ); - return; - }; - - let nack = match build_nack_node(info, own_pn, reason, failure_reason) { + let nack = match self.build_nack_from_snapshot(info, reason, failure_reason) { Ok(nack) => nack, + Err(crate::features::StanzaResponseError::MissingLocalIdentity) => { + log::debug!( + "[msg:{}] Skipping nack ({:?}): own PN not yet set", + info.id, + reason + ); + return; + } Err(error) => { log::warn!(target: "Client/Receipt", "Failed to build nack for message {}: {error}", info.id); return; } }; - drop(device); debug!(target: "Client/Receipt", "Sending nack (reason={:?}, code={}) for message {} from {}", reason, reason.code(), info.id, info.source.sender.observe()); @@ -802,18 +841,8 @@ impl Client { stanza: &NodeRef<'_>, rejection: crate::features::StanzaRejection, ) -> Result<(), crate::features::StanzaResponseError> { - let device = self.persistence_manager.get_device_snapshot(); - let own_pn = device - .pn - .as_ref() - .ok_or(crate::features::StanzaResponseError::MissingLocalIdentity)?; - let nack = build_nack_node( - stanza, - own_pn, - rejection.reason(), - rejection.failure_reason(), - )?; - drop(device); + let nack = + self.build_nack_from_snapshot(stanza, rejection.reason(), rejection.failure_reason())?; self.send_node(nack).await?; Ok(()) } diff --git a/wacore/binary/src/attrs.rs b/wacore/binary/src/attrs.rs index 272279a9c..e8b417fe5 100644 --- a/wacore/binary/src/attrs.rs +++ b/wacore/binary/src/attrs.rs @@ -92,18 +92,24 @@ impl<'a> AttrParserRef<'a> { }) } + /// Get an optional JID attribute, failing when a present value is invalid. + pub fn optional_jid_result(&mut self, key: &str) -> Result> { + match self.get_raw(key, false) { + None => Ok(None), + Some(ValueRef::Jid(jid)) => Ok(Some(jid.to_owned())), + Some(ValueRef::String(value)) => { + Jid::from_str(value).map(Some).map_err(BinaryError::from) + } + } + } + /// Get a required JID attribute, failing immediately when it is missing or invalid. /// /// Structured JIDs are converted directly from their decoded representation; /// string attributes are parsed exactly once. pub fn required_jid(&mut self, key: &str) -> Result { - match self - .get_raw(key, false) - .ok_or_else(|| BinaryError::MissingAttr(key.to_string()))? - { - ValueRef::Jid(jid) => Ok(jid.to_owned()), - ValueRef::String(value) => Jid::from_str(value).map_err(BinaryError::from), - } + self.optional_jid_result(key)? + .ok_or_else(|| BinaryError::MissingAttr(key.to_string())) } pub fn jid(&mut self, key: &str) -> Jid { diff --git a/wacore/src/messages.rs b/wacore/src/messages.rs index 3edca1ce2..1a071fbb8 100644 --- a/wacore/src/messages.rs +++ b/wacore/src/messages.rs @@ -970,7 +970,7 @@ pub fn parse_message_info( ..Default::default() } } else if from.matches_user_or_lid(own_jid, own_lid) { - let recipient = attrs.optional_jid("recipient"); + let recipient = attrs.optional_jid_result("recipient")?; let chat = recipient .as_ref() .map(|r| r.to_non_ad()) @@ -1331,6 +1331,11 @@ mod parse_message_info_tests { .attr("participant", "not-a-jid") .attr("id", "INVALID-PARTICIPANT") .build(), + NodeBuilder::new("message") + .attr("from", "559900000000:4@s.whatsapp.net") + .attr("recipient", "not-a-jid") + .attr("id", "INVALID-SELF-RECIPIENT") + .build(), ]; for node in &cases { From e8dbf6e7fc48fea27031c06aa82e13c27be9df08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= <55464917+jlucaso1@users.noreply.github.com> Date: Tue, 21 Jul 2026 23:18:05 -0300 Subject: [PATCH 5/8] test(core): use reserved retry sender fixture --- src/message/tests.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/message/tests.rs b/src/message/tests.rs index 059a59e07..a5a8961d6 100644 --- a/src/message/tests.rs +++ b/src/message/tests.rs @@ -5688,10 +5688,12 @@ async fn explicit_stanza_responses_reject_incomplete_input_without_sending() { assert_eq!(transport.failed_sends(), 1); } +const EXPLICIT_RETRY_SENDER: &str = "12025550111:7@s.whatsapp.net"; + fn retry_request_stanza(id: &'static str) -> wacore_binary::Node { NodeBuilder::new("message") .attr("id", id) - .attr("from", "15551234567:7@s.whatsapp.net") + .attr("from", EXPLICIT_RETRY_SENDER) .attr("t", "1") .attr("type", "text") .build() @@ -6088,7 +6090,7 @@ async fn explicit_retry_validates_input_and_reports_the_shared_limit() { )); let stanza = retry_request_stanza("RETRY-LIMIT"); - let sender: Jid = "15551234567:7@s.whatsapp.net".parse().expect("sender"); + let sender: Jid = EXPLICIT_RETRY_SENDER.parse().expect("sender"); let chat = sender.to_non_ad(); let cache_key = client .make_retry_cache_key(&chat, "RETRY-LIMIT", &sender) @@ -6139,7 +6141,7 @@ async fn explicit_retry_while_disconnected_preserves_budget_and_prekeys() { )) )); - let sender: Jid = "15551234567:7@s.whatsapp.net".parse().expect("sender"); + let sender: Jid = EXPLICIT_RETRY_SENDER.parse().expect("sender"); let cache_key = client .make_retry_cache_key(&sender.to_non_ad(), "EXPLICIT-RETRY-DISCONNECTED", &sender) .await; From 451443e349b49e2ab7cecb94a804bfa44a52d3b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= <55464917+jlucaso1@users.noreply.github.com> Date: Wed, 22 Jul 2026 09:32:36 -0300 Subject: [PATCH 6/8] fix(core): preserve explicit receipt ack participants --- src/client.rs | 49 ++++++++++++++++--------- src/client/node_io.rs | 14 ++++--- src/client/tests.rs | 85 ++++++++++++++++++++++++++++++++++--------- src/message/tests.rs | 53 +++++++++++++++++++++++++++ 4 files changed, 162 insertions(+), 39 deletions(-) diff --git a/src/client.rs b/src/client.rs index 20aa50ed6..8111d494a 100644 --- a/src/client.rs +++ b/src/client.rs @@ -1060,10 +1060,32 @@ fn value_refs_display_equal( } } +#[derive(Clone, Copy)] +enum AckParticipantPolicy { + Preserve, + OmitReceiptDestinationDuplicate, +} + +#[inline] +fn ack_participant<'node, 'data>( + node: &'node wacore_binary::NodeRef<'data>, + from: &wacore_binary::node::ValueRef<'data>, + policy: AckParticipantPolicy, +) -> Option<&'node wacore_binary::node::ValueRef<'data>> { + node.get_attr("participant") + .filter(|participant| match policy { + AckParticipantPolicy::Preserve => true, + AckParticipantPolicy::OmitReceiptDestinationDuplicate => { + node.tag != "receipt" || !value_refs_display_equal(participant, from) + } + }) +} + /// Build an `` for the given stanza, matching WA Web / whatsmeow behavior: /// /// - `class` = original stanza tag -/// - `id`, `to` (flipped from `from`), `participant` copied from original +/// - `id`, `to` (flipped from `from`) copied from original +/// - `participant` follows the generic or receipt-specialized policy /// - `from` = own device PN, only for message acks /// - `type` echoed when present, except `notification type="encrypt"` with /// an `` child @@ -1078,6 +1100,7 @@ fn value_refs_display_equal( fn encode_ack_bytes( node: &wacore_binary::NodeRef<'_>, own_device_pn: Option<&Jid>, + participant_policy: AckParticipantPolicy, ) -> Result, crate::features::StanzaResponseError> { use wacore_binary::encoder::{ByteWriter, EncodeNode, Encoder}; @@ -1090,15 +1113,7 @@ fn encode_ack_bytes( "from", ))?; let tag = node.tag.as_ref(); - // WAWebReceiptAck: `participant: r && r !== e ? DEVICE_JID(r) : DROP_ATTR`. - // This is specific to the specialized receipt ACK. Generic ACKs and NACKs - // preserve participant even when it duplicates the destination. - let participant_val = node.get_attr("participant").filter(|participant| { - if tag != "receipt" { - return true; - } - !value_refs_display_equal(participant, from_val) - }); + let participant_val = ack_participant(node, from_val, participant_policy); // Server expects `recipient` echoed back so it can route the ack to the // origin companion/device (hosted-companion, peer, LID-routed stanzas). // Dropping it makes the server close the stream with ``. @@ -1220,19 +1235,19 @@ fn message_ack_source_node(info: &crate::types::message::MessageInfo) -> Node { builder.build() } -/// Build an ack Node (used in tests for structure verification). +/// Build an automatic ack Node (used in tests for structure verification). #[cfg(test)] fn build_ack_node(node: &wacore_binary::NodeRef<'_>, own_device_pn: Option<&Jid>) -> Option { let id = node.get_attr("id")?.to_node_value(); let from_ref = node.get_attr("from")?; let from = from_ref.to_node_value(); let tag = node.tag.as_ref(); - // Only the specialized receipt ACK drops a participant that duplicates - // `to` (the flipped `from`). - let participant = node - .get_attr("participant") - .filter(|participant| tag != "receipt" || !value_refs_display_equal(participant, from_ref)) - .map(|v| v.to_node_value()); + let participant = ack_participant( + node, + from_ref, + AckParticipantPolicy::OmitReceiptDestinationDuplicate, + ) + .map(|value| value.to_node_value()); let recipient = node.get_attr("recipient").map(|v| v.to_node_value()); let typ = if !is_encrypt_identity_notification(node) { node.get_attr("type").map(|v| v.to_node_value()) diff --git a/src/client/node_io.rs b/src/client/node_io.rs index fcf4dcee6..4db844432 100644 --- a/src/client/node_io.rs +++ b/src/client/node_io.rs @@ -593,9 +593,10 @@ impl Client { fn encode_ack_from_snapshot( &self, node: &wacore_binary::NodeRef<'_>, + participant_policy: AckParticipantPolicy, ) -> Result, crate::features::StanzaResponseError> { let device = self.persistence_manager.get_device_snapshot(); - let encoded = encode_ack_bytes(node, device.pn.as_ref()); + let encoded = encode_ack_bytes(node, device.pn.as_ref(), participant_policy); drop(device); encoded } @@ -615,7 +616,9 @@ impl Client { if !self.is_connected() { return Err(ClientError::NotConnected); } - let buf = match self.encode_ack_from_snapshot(node) { + let buf = match self + .encode_ack_from_snapshot(node, AckParticipantPolicy::OmitReceiptDestinationDuplicate) + { Ok(buf) => buf, Err(e) => { log::warn!("Failed to encode ack: {e}"); @@ -638,7 +641,7 @@ impl Client { &self, stanza: &wacore_binary::NodeRef<'_>, ) -> Result<(), crate::features::StanzaResponseError> { - let bytes = self.encode_ack_from_snapshot(stanza)?; + let bytes = self.encode_ack_from_snapshot(stanza, AckParticipantPolicy::Preserve)?; self.send_raw_bytes(bytes).await?; Ok(()) } @@ -648,7 +651,8 @@ impl Client { /// in a single flushed task. pub(crate) async fn send_transport_ack(&self, info: &crate::types::message::MessageInfo) { let source = message_ack_source_node(info); - let encoded = self.encode_ack_from_snapshot(&source.as_node_ref()); + let encoded = + self.encode_ack_from_snapshot(&source.as_node_ref(), AckParticipantPolicy::Preserve); match encoded { Ok(buf) => { if let Err(e) = self.send_raw_bytes(buf).await @@ -681,7 +685,7 @@ impl Client { self: &Arc, node: &wacore_binary::NodeRef<'_>, ) { - let buf = match self.encode_ack_from_snapshot(node) { + let buf = match self.encode_ack_from_snapshot(node, AckParticipantPolicy::Preserve) { Ok(buf) => buf, Err(e) => { log::warn!("Failed to encode node transport ack: {e}"); diff --git a/src/client/tests.rs b/src/client/tests.rs index 359c3f96f..caf125813 100644 --- a/src/client/tests.rs +++ b/src/client/tests.rs @@ -2332,8 +2332,12 @@ fn test_encode_ack_bytes_roundtrip_recipient() { .attr("type", "text") .attr("recipient", "146991363395800@lid") .build(); - let buf = encode_ack_bytes(&with_recipient.as_node_ref(), Some(&own_device_pn)) - .expect("encode_ack_bytes should produce bytes"); + let buf = encode_ack_bytes( + &with_recipient.as_node_ref(), + Some(&own_device_pn), + AckParticipantPolicy::Preserve, + ) + .expect("encode_ack_bytes should produce bytes"); // The Encoder prepends a leading format byte (see `marshal`); the // decoder wants raw protocol bytes — same handling as `node_to_owned_ref`. let decoded = @@ -2364,8 +2368,12 @@ fn test_encode_ack_bytes_roundtrip_recipient() { .attr("type", "text") .attr("participant", "181531758878822@lid") .build(); - let buf = encode_ack_bytes(&without_recipient.as_node_ref(), Some(&own_device_pn)) - .expect("encode_ack_bytes should produce bytes"); + let buf = encode_ack_bytes( + &without_recipient.as_node_ref(), + Some(&own_device_pn), + AckParticipantPolicy::Preserve, + ) + .expect("encode_ack_bytes should produce bytes"); let decoded = wacore_binary::marshal::unmarshal_ref(&buf[1..]).expect("encoded ack should decode"); assert!( @@ -2380,7 +2388,11 @@ fn test_encode_ack_bytes_requires_public_response_inputs() { .attr("from", "15551234567@s.whatsapp.net") .build(); assert!(matches!( - encode_ack_bytes(&without_id.as_node_ref(), None), + encode_ack_bytes( + &without_id.as_node_ref(), + None, + AckParticipantPolicy::Preserve, + ), Err(crate::features::StanzaResponseError::MissingAttribute("id")) )); @@ -2388,7 +2400,11 @@ fn test_encode_ack_bytes_requires_public_response_inputs() { .attr("id", "MISSING-FROM") .build(); assert!(matches!( - encode_ack_bytes(&without_from.as_node_ref(), None), + encode_ack_bytes( + &without_from.as_node_ref(), + None, + AckParticipantPolicy::Preserve, + ), Err(crate::features::StanzaResponseError::MissingAttribute( "from" )) @@ -2399,7 +2415,7 @@ fn test_encode_ack_bytes_requires_public_response_inputs() { .attr("from", "15551234567@s.whatsapp.net") .build(); assert!(matches!( - encode_ack_bytes(&message.as_node_ref(), None), + encode_ack_bytes(&message.as_node_ref(), None, AckParticipantPolicy::Preserve,), Err(crate::features::StanzaResponseError::MissingLocalIdentity) )); } @@ -2413,8 +2429,12 @@ fn test_encode_ack_bytes_preserves_specialized_receipt_rules() { .attr("participant", "15551234567@s.whatsapp.net") .attr("type", "retry") .build(); - let bytes = encode_ack_bytes(&receipt.as_node_ref(), None) - .expect("complete receipt should produce an ack"); + let bytes = encode_ack_bytes( + &receipt.as_node_ref(), + None, + AckParticipantPolicy::OmitReceiptDestinationDuplicate, + ) + .expect("complete receipt should produce an ack"); let ack = wacore_binary::marshal::unmarshal_ref(&bytes[1..]) .expect("encoded receipt ack should decode"); @@ -2432,13 +2452,36 @@ fn test_encode_ack_bytes_preserves_specialized_receipt_rules() { ); assert!(ack.get_attr("from").is_none()); + let group_receipt = NodeBuilder::new("receipt") + .attr("id", "GROUP-RECEIPT-ACK") + .attr("from", "120363098765432100@g.us") + .attr("participant", "12025550111:7@s.whatsapp.net") + .build(); + let bytes = encode_ack_bytes( + &group_receipt.as_node_ref(), + None, + AckParticipantPolicy::OmitReceiptDestinationDuplicate, + ) + .expect("group receipt should produce an ack"); + let ack = wacore_binary::marshal::unmarshal_ref(&bytes[1..]) + .expect("encoded group receipt ack should decode"); + assert!( + ack.get_attr("participant") + .is_some_and(|value| value.as_str() == "12025550111:7@s.whatsapp.net"), + "receipt ack must preserve a participant distinct from its destination" + ); + let generic = NodeBuilder::new("message") .attr("id", "MESSAGE-ACK") .attr("from", "15551234567@s.whatsapp.net") .attr("participant", &from) .build(); - let bytes = encode_ack_bytes(&generic.as_node_ref(), Some(&from)) - .expect("complete message should produce an ack"); + let bytes = encode_ack_bytes( + &generic.as_node_ref(), + Some(&from), + AckParticipantPolicy::Preserve, + ) + .expect("complete message should produce an ack"); let ack = wacore_binary::marshal::unmarshal_ref(&bytes[1..]) .expect("encoded message ack should decode"); assert!( @@ -2468,8 +2511,12 @@ fn test_encode_ack_bytes_compares_jid_participants_by_display() { .attr("from", &from) .attr("participant", &participant) .build(); - let bytes = encode_ack_bytes(&receipt.as_node_ref(), None) - .expect("complete receipt should produce an ack"); + let bytes = encode_ack_bytes( + &receipt.as_node_ref(), + None, + AckParticipantPolicy::OmitReceiptDestinationDuplicate, + ) + .expect("complete receipt should produce an ack"); let ack = wacore_binary::marshal::unmarshal_ref(&bytes[1..]) .expect("encoded receipt ack should decode"); @@ -2487,8 +2534,12 @@ fn test_encode_ack_bytes_drops_encrypt_identity_notification_type() { .attr("type", "encrypt") .children([NodeBuilder::new("identity").build()]) .build(); - let bytes = encode_ack_bytes(¬ification.as_node_ref(), None) - .expect("complete notification should produce an ack"); + let bytes = encode_ack_bytes( + ¬ification.as_node_ref(), + None, + AckParticipantPolicy::Preserve, + ) + .expect("complete notification should produce an ack"); let ack = wacore_binary::marshal::unmarshal_ref(&bytes[1..]) .expect("encoded notification ack should decode"); @@ -2507,8 +2558,8 @@ fn test_encode_ack_bytes_preserves_call_class_and_type() { .attr("from", "15551234567@s.whatsapp.net") .attr("type", "offer_notice") .build(); - let bytes = - encode_ack_bytes(&call.as_node_ref(), None).expect("complete call should produce an ack"); + let bytes = encode_ack_bytes(&call.as_node_ref(), None, AckParticipantPolicy::Preserve) + .expect("complete call should produce an ack"); let ack = wacore_binary::marshal::unmarshal_ref(&bytes[1..]).expect("encoded call ack should decode"); diff --git a/src/message/tests.rs b/src/message/tests.rs index a5a8961d6..a6d44129b 100644 --- a/src/message/tests.rs +++ b/src/message/tests.rs @@ -5626,6 +5626,10 @@ async fn explicit_stanza_responses_use_the_canonical_wire_paths() { nack.get_attr("participant") .is_some_and(|value| value.as_str() == "15551234567:7@s.whatsapp.net") ); + assert!( + nack.get_attr("type") + .is_some_and(|value| value.as_str() == "retry") + ); let meta = nack .get_optional_child("meta") .expect("InvalidProtobuf should carry its failure detail"); @@ -5635,6 +5639,55 @@ async fn explicit_stanza_responses_use_the_canonical_wire_paths() { ); } +#[tokio::test] +async fn explicit_and_automatic_receipt_acks_use_distinct_participant_policies() { + let (client, transport) = capturing_client("receipt_ack_participant_policy").await; + const FROM: &str = "12025550111:7@s.whatsapp.net"; + + let explicit = NodeBuilder::new("receipt") + .attr("id", "EXPLICIT-RECEIPT-ACK") + .attr("from", FROM) + .attr("participant", FROM) + .attr("type", "read") + .build(); + client + .acknowledge_stanza(&explicit.as_node_ref()) + .await + .expect("generic receipt ack should send"); + + let automatic = NodeBuilder::new("receipt") + .attr("id", "AUTOMATIC-RECEIPT-ACK") + .attr("from", FROM) + .attr("participant", FROM) + .attr("type", "read") + .build(); + client + .send_ack_for(&automatic.as_node_ref()) + .await + .expect("specialized receipt ack should send"); + + let frames = transport.sent(); + assert_eq!(frames.len(), 2); + + let explicit_bytes = decode_frame(0, &frames[0]).expect("explicit ack frame should decrypt"); + let explicit_ack = wacore_binary::marshal::unmarshal_ref(&explicit_bytes[1..]) + .expect("explicit ack frame should decode"); + assert!( + explicit_ack + .get_attr("participant") + .is_some_and(|value| value.as_str() == FROM), + "generic explicit ack must preserve participant" + ); + + let automatic_bytes = decode_frame(1, &frames[1]).expect("automatic ack frame should decrypt"); + let automatic_ack = wacore_binary::marshal::unmarshal_ref(&automatic_bytes[1..]) + .expect("automatic ack frame should decode"); + assert!( + automatic_ack.get_attr("participant").is_none(), + "specialized receipt ack must omit a participant equal to its destination" + ); +} + #[tokio::test] async fn explicit_stanza_responses_reject_incomplete_input_without_sending() { use std::error::Error as _; From 435e3f72d4d99afb1d2204c78e6547eff079b1f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= <55464917+jlucaso1@users.noreply.github.com> Date: Wed, 22 Jul 2026 09:57:09 -0300 Subject: [PATCH 7/8] fix(core): reject empty message stanza ids --- src/client/tests.rs | 16 +++++----- src/message/tests.rs | 61 +++++++++++++++++++++++++++------------ src/receipt.rs | 16 +++++----- wacore/binary/src/node.rs | 6 ++-- wacore/src/messages.rs | 15 ++++++++-- 5 files changed, 73 insertions(+), 41 deletions(-) diff --git a/src/client/tests.rs b/src/client/tests.rs index caf125813..922c5139e 100644 --- a/src/client/tests.rs +++ b/src/client/tests.rs @@ -2385,7 +2385,7 @@ fn test_encode_ack_bytes_roundtrip_recipient() { #[test] fn test_encode_ack_bytes_requires_public_response_inputs() { let without_id = NodeBuilder::new("receipt") - .attr("from", "15551234567@s.whatsapp.net") + .attr("from", "12025550111@s.whatsapp.net") .build(); assert!(matches!( encode_ack_bytes( @@ -2412,7 +2412,7 @@ fn test_encode_ack_bytes_requires_public_response_inputs() { let message = NodeBuilder::new("message") .attr("id", "MISSING-IDENTITY") - .attr("from", "15551234567@s.whatsapp.net") + .attr("from", "12025550111@s.whatsapp.net") .build(); assert!(matches!( encode_ack_bytes(&message.as_node_ref(), None, AckParticipantPolicy::Preserve,), @@ -2422,11 +2422,11 @@ fn test_encode_ack_bytes_requires_public_response_inputs() { #[test] fn test_encode_ack_bytes_preserves_specialized_receipt_rules() { - let from: Jid = "15551234567@s.whatsapp.net".parse().unwrap(); + let from: Jid = "12025550111@s.whatsapp.net".parse().unwrap(); let receipt = NodeBuilder::new("receipt") .attr("id", "RECEIPT-ACK") .attr("from", &from) - .attr("participant", "15551234567@s.whatsapp.net") + .attr("participant", "12025550111@s.whatsapp.net") .attr("type", "retry") .build(); let bytes = encode_ack_bytes( @@ -2473,7 +2473,7 @@ fn test_encode_ack_bytes_preserves_specialized_receipt_rules() { let generic = NodeBuilder::new("message") .attr("id", "MESSAGE-ACK") - .attr("from", "15551234567@s.whatsapp.net") + .attr("from", "12025550111@s.whatsapp.net") .attr("participant", &from) .build(); let bytes = encode_ack_bytes( @@ -2486,7 +2486,7 @@ fn test_encode_ack_bytes_preserves_specialized_receipt_rules() { .expect("encoded message ack should decode"); assert!( ack.get_attr("participant") - .is_some_and(|value| value.as_str() == "15551234567@s.whatsapp.net"), + .is_some_and(|value| value.as_str() == "12025550111@s.whatsapp.net"), "generic ack must not inherit the receipt-only participant rule" ); } @@ -2530,7 +2530,7 @@ fn test_encode_ack_bytes_compares_jid_participants_by_display() { fn test_encode_ack_bytes_drops_encrypt_identity_notification_type() { let notification = NodeBuilder::new("notification") .attr("id", "IDENTITY-NOTIFICATION") - .attr("from", "15551234567@s.whatsapp.net") + .attr("from", "12025550111@s.whatsapp.net") .attr("type", "encrypt") .children([NodeBuilder::new("identity").build()]) .build(); @@ -2555,7 +2555,7 @@ fn test_encode_ack_bytes_drops_encrypt_identity_notification_type() { fn test_encode_ack_bytes_preserves_call_class_and_type() { let call = NodeBuilder::new("call") .attr("id", "CALL-ACK") - .attr("from", "15551234567@s.whatsapp.net") + .attr("from", "12025550111@s.whatsapp.net") .attr("type", "offer_notice") .build(); let bytes = encode_ack_bytes(&call.as_node_ref(), None, AckParticipantPolicy::Preserve) diff --git a/src/message/tests.rs b/src/message/tests.rs index a6d44129b..d00185f05 100644 --- a/src/message/tests.rs +++ b/src/message/tests.rs @@ -5546,8 +5546,8 @@ async fn explicit_stanza_responses_use_the_canonical_wire_paths() { let message = NodeBuilder::new("message") .attr("id", "EXPLICIT-ACK") - .attr("from", "15551234567:7@s.whatsapp.net") - .attr("participant", "15557654321:4@s.whatsapp.net") + .attr("from", "12025550111:7@s.whatsapp.net") + .attr("participant", "13125550112:4@s.whatsapp.net") .attr("recipient", "5511000000001@s.whatsapp.net") .attr("type", "text") .build(); @@ -5559,7 +5559,7 @@ async fn explicit_stanza_responses_use_the_canonical_wire_paths() { let receipt = NodeBuilder::new("receipt") .attr("id", "EXPLICIT-NACK") .attr("from", "120363021033254949@g.us") - .attr("participant", "15551234567:7@s.whatsapp.net") + .attr("participant", "12025550111:7@s.whatsapp.net") .attr("type", "retry") .build(); client @@ -5587,7 +5587,7 @@ async fn explicit_stanza_responses_use_the_canonical_wire_paths() { ); assert!( ack.get_attr("to") - .is_some_and(|value| value.as_str() == "15551234567:7@s.whatsapp.net") + .is_some_and(|value| value.as_str() == "12025550111:7@s.whatsapp.net") ); assert!( ack.get_attr("from") @@ -5595,7 +5595,7 @@ async fn explicit_stanza_responses_use_the_canonical_wire_paths() { ); assert!( ack.get_attr("participant") - .is_some_and(|value| value.as_str() == "15557654321:4@s.whatsapp.net") + .is_some_and(|value| value.as_str() == "13125550112:4@s.whatsapp.net") ); assert!( ack.get_attr("recipient") @@ -5624,7 +5624,7 @@ async fn explicit_stanza_responses_use_the_canonical_wire_paths() { ); assert!( nack.get_attr("participant") - .is_some_and(|value| value.as_str() == "15551234567:7@s.whatsapp.net") + .is_some_and(|value| value.as_str() == "12025550111:7@s.whatsapp.net") ); assert!( nack.get_attr("type") @@ -5695,7 +5695,7 @@ async fn explicit_stanza_responses_reject_incomplete_input_without_sending() { let (client, transport) = capturing_client("explicit_stanza_invalid").await; let ack_without_id = NodeBuilder::new("receipt") - .attr("from", "15551234567@s.whatsapp.net") + .attr("from", "12025550111@s.whatsapp.net") .build(); assert!(matches!( client @@ -5722,7 +5722,7 @@ async fn explicit_stanza_responses_reject_incomplete_input_without_sending() { transport.fail_next_sends(1); let valid_receipt = NodeBuilder::new("receipt") .attr("id", "TRANSPORT-FAILURE") - .attr("from", "15551234567@s.whatsapp.net") + .attr("from", "12025550111@s.whatsapp.net") .build(); let error = client .acknowledge_stanza(&valid_receipt.as_node_ref()) @@ -5923,7 +5923,7 @@ async fn explicit_retry_preserves_canonical_routing_shapes() { let group = NodeBuilder::new("message") .attr("id", "RETRY-GROUP") .attr("from", "120363021033254949@g.us") - .attr("participant", "15551234567:7@s.whatsapp.net") + .attr("participant", "12025550111:7@s.whatsapp.net") .attr("t", "1") .build(); assert_eq!( @@ -5944,7 +5944,7 @@ async fn explicit_retry_preserves_canonical_routing_shapes() { assert_eq!(group_receipt.to, "120363021033254949@g.us"); assert_eq!( group_receipt.participant.as_deref(), - Some("15551234567:7@s.whatsapp.net") + Some("12025550111:7@s.whatsapp.net") ); assert_eq!(group_receipt.recipient, None); assert_eq!(group_receipt.category, None); @@ -5952,7 +5952,7 @@ async fn explicit_retry_preserves_canonical_routing_shapes() { let status = NodeBuilder::new("message") .attr("id", "RETRY-STATUS") .attr("from", "status@broadcast") - .attr("participant", "15551234567:7@s.whatsapp.net") + .attr("participant", "12025550111:7@s.whatsapp.net") .attr("t", "1") .build(); client @@ -5967,13 +5967,13 @@ async fn explicit_retry_preserves_canonical_routing_shapes() { assert_eq!(status_receipt.to, "status@broadcast"); assert_eq!( status_receipt.participant.as_deref(), - Some("15551234567:7@s.whatsapp.net") + Some("12025550111:7@s.whatsapp.net") ); let peer = NodeBuilder::new("message") .attr("id", "RETRY-PEER") .attr("from", "5511000000001:9@s.whatsapp.net") - .attr("recipient", "15551234567@s.whatsapp.net") + .attr("recipient", "12025550111@s.whatsapp.net") .attr("category", "peer") .attr("t", "1") .build(); @@ -5993,7 +5993,7 @@ async fn explicit_retry_preserves_canonical_routing_shapes() { let self_fanout = NodeBuilder::new("message") .attr("id", "RETRY-SELF") .attr("from", "5511000000001:9@s.whatsapp.net") - .attr("recipient", "15551234567@s.whatsapp.net") + .attr("recipient", "12025550111@s.whatsapp.net") .attr("t", "1") .build(); client @@ -6008,13 +6008,13 @@ async fn explicit_retry_preserves_canonical_routing_shapes() { assert_eq!(self_receipt.to, "5511000000001:9@s.whatsapp.net"); assert_eq!( self_receipt.recipient.as_deref(), - Some("15551234567@s.whatsapp.net") + Some("12025550111@s.whatsapp.net") ); assert_eq!(self_receipt.category, None); let hosted = NodeBuilder::new("message") .attr("id", "RETRY-HOSTED") - .attr("from", "15551234567:7@hosted") + .attr("from", "12025550111:7@hosted") .attr("t", "1") .build(); assert_eq!( @@ -6032,7 +6032,7 @@ async fn explicit_retry_preserves_canonical_routing_shapes() { ); let hosted_receipt = find_receipt_details(&transport.sent(), "RETRY-HOSTED").expect("hosted retry receipt"); - assert_eq!(hosted_receipt.to, "15551234567:7@hosted"); + assert_eq!(hosted_receipt.to, "12025550111:7@hosted"); assert!(hosted_receipt.has_keys); let bot_dm = NodeBuilder::new("message") @@ -6089,7 +6089,7 @@ async fn explicit_retry_validates_input_and_reports_the_shared_limit() { let receipt = NodeBuilder::new("receipt") .attr("id", "WRONG-CLASS") - .attr("from", "15551234567@s.whatsapp.net") + .attr("from", "12025550111@s.whatsapp.net") .build(); assert!(matches!( client @@ -6102,7 +6102,7 @@ async fn explicit_retry_validates_input_and_reports_the_shared_limit() { )); let missing_id = NodeBuilder::new("message") - .attr("from", "15551234567@s.whatsapp.net") + .attr("from", "12025550111@s.whatsapp.net") .build(); assert!(matches!( client @@ -6114,6 +6114,29 @@ async fn explicit_retry_validates_input_and_reports_the_shared_limit() { Err(crate::features::RetryRequestError::MissingAttribute("id")) )); + let empty_id = retry_request_stanza(""); + assert!(matches!( + client + .request_message_retry( + &empty_id.as_node_ref(), + crate::features::RetryRequestOptions::default(), + ) + .await, + Err(crate::features::RetryRequestError::InvalidStanza(_)) + )); + let retry_sender: Jid = EXPLICIT_RETRY_SENDER.parse().expect("sender"); + let empty_id_cache_key = client + .make_retry_cache_key(&retry_sender.to_non_ad(), "", &retry_sender) + .await; + assert!( + client + .message_retry_counts + .get(&empty_id_cache_key) + .await + .is_none(), + "an empty stanza id must be rejected before retry accounting" + ); + let missing_from = NodeBuilder::new("message") .attr("id", "MISSING-FROM") .build(); diff --git a/src/receipt.rs b/src/receipt.rs index 70a9802ee..2de644c80 100644 --- a/src/receipt.rs +++ b/src/receipt.rs @@ -1406,7 +1406,7 @@ mod tests { let stanza = NodeBuilder::new(tag) .attr("id", "STANZA-ID") .attr("from", "120363021033254949@g.us") - .attr("participant", "15551234567:4@s.whatsapp.net") + .attr("participant", "12025550111:4@s.whatsapp.net") .attr("type", "test-type") .build(); let nack = build_nack_node( @@ -1437,7 +1437,7 @@ mod tests { .get("participant") .map(|value| value.as_str()) .as_deref(), - Some("15551234567:4@s.whatsapp.net") + Some("12025550111:4@s.whatsapp.net") ); assert_eq!( nack.attrs @@ -1460,7 +1460,7 @@ mod tests { fn unrecognized_stanza_rejection_preserves_custom_class() { let stanza = NodeBuilder::new("future-stanza") .attr("id", "FUTURE-ID") - .attr("from", "15551234567@s.whatsapp.net") + .attr("from", "12025550111@s.whatsapp.net") .build(); let nack = build_nack_node( &stanza.as_node_ref(), @@ -1492,8 +1492,8 @@ mod tests { fn nack_does_not_apply_the_receipt_ack_participant_rule() { let stanza = NodeBuilder::new("receipt") .attr("id", "NACK-DUPLICATE-PARTICIPANT") - .attr("from", "15551234567@s.whatsapp.net") - .attr("participant", "15551234567@s.whatsapp.net") + .attr("from", "12025550111@s.whatsapp.net") + .attr("participant", "12025550111@s.whatsapp.net") .build(); let nack = build_nack_node( &stanza.as_node_ref(), @@ -1506,7 +1506,7 @@ mod tests { assert!( nack.attrs .get("participant") - .is_some_and(|value| value == "15551234567@s.whatsapp.net"), + .is_some_and(|value| value == "12025550111@s.whatsapp.net"), "nack must preserve participant even when a receipt ack would omit it" ); } @@ -1514,7 +1514,7 @@ mod tests { #[test] fn nack_from_original_stanza_requires_id_and_from() { let without_id = NodeBuilder::new("message") - .attr("from", "15551234567@s.whatsapp.net") + .attr("from", "12025550111@s.whatsapp.net") .build(); assert!(matches!( build_nack_node( @@ -1546,7 +1546,7 @@ mod tests { fn nack_preserves_unknown_numeric_reason() { let stanza = NodeBuilder::new("message") .attr("id", "UNKNOWN-REASON") - .attr("from", "15551234567@s.whatsapp.net") + .attr("from", "12025550111@s.whatsapp.net") .build(); let nack = build_nack_node( &stanza.as_node_ref(), diff --git a/wacore/binary/src/node.rs b/wacore/binary/src/node.rs index 7c50dcee7..8e2ca5166 100644 --- a/wacore/binary/src/node.rs +++ b/wacore/binary/src/node.rs @@ -1033,14 +1033,14 @@ mod value_ref_tests { assert!(string != "other"); let jid = ValueRef::Jid(JidRef { - user: NodeStr::Borrowed("15551234567"), + user: NodeStr::Borrowed("12025550111"), server: Server::Pn, agent: 0, device: 7, integrator: 0, }); - assert!(jid == "15551234567:7@s.whatsapp.net"); - assert!(jid != "15551234567@s.whatsapp.net"); + assert!(jid == "12025550111:7@s.whatsapp.net"); + assert!(jid != "12025550111@s.whatsapp.net"); } } diff --git a/wacore/src/messages.rs b/wacore/src/messages.rs index 1a071fbb8..348addbab 100644 --- a/wacore/src/messages.rs +++ b/wacore/src/messages.rs @@ -919,6 +919,12 @@ pub fn parse_message_info( use wacore_binary::{JidExt as _, STATUS_BROADCAST_USER, Server}; let mut attrs = node.attrs(); + let id = attrs.required_string("id")?; + anyhow::ensure!( + !id.is_empty(), + "message stanza has an empty required 'id' attribute" + ); + let id = id.into_owned(); let from = attrs.required_jid("from")?; let addressing_mode = attrs .optional_string("addressing_mode") @@ -1029,7 +1035,6 @@ pub fn parse_message_info( .map(|s| MessageCategory::from(s.as_ref())) .unwrap_or_default(); - let id = attrs.required_string("id")?.to_string(); let server_id = attrs .optional_u64("server_id") .filter(|&v| (99..=2_147_476_647).contains(&v)) @@ -1315,9 +1320,13 @@ mod parse_message_info_tests { use wacore_binary::builder::NodeBuilder; #[test] - fn invalid_routing_jids_are_rejected() { + fn invalid_routing_and_identity_attributes_are_rejected() { let own_pn = Jid::from_str("559900000000@s.whatsapp.net").unwrap(); let cases = [ + NodeBuilder::new("message") + .attr("from", "559980000001@s.whatsapp.net") + .attr("id", "") + .build(), NodeBuilder::new("message") .attr("from", "not-a-jid") .attr("id", "INVALID-FROM") @@ -1341,7 +1350,7 @@ mod parse_message_info_tests { for node in &cases { assert!( parse_message_info(&node.as_node_ref(), &own_pn, None).is_err(), - "invalid routing attributes must not produce default JIDs: {node:?}" + "invalid identity or routing attributes must be rejected: {node:?}" ); } } From 4d2234418d8369fecc8bf6fec87014caac58a961 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= <55464917+jlucaso1@users.noreply.github.com> Date: Wed, 22 Jul 2026 10:23:41 -0300 Subject: [PATCH 8/8] fix(core): harden explicit stanza operations --- src/client.rs | 10 +---- src/client/tests.rs | 28 +++++++++++++ src/features/mod.rs | 1 + src/features/stanza.rs | 10 +++++ src/message.rs | 43 +++++++++++++++++++ src/message/receive.rs | 34 +++------------ src/message/retry.rs | 18 +++++++- src/message/tests.rs | 90 ++++++++++++++++++++++++++++++++++++++++ src/receipt.rs | 10 +---- wacore/binary/src/jid.rs | 45 ++++++++++++++++++++ 10 files changed, 244 insertions(+), 45 deletions(-) diff --git a/src/client.rs b/src/client.rs index 8111d494a..93778357b 100644 --- a/src/client.rs +++ b/src/client.rs @@ -1104,14 +1104,8 @@ fn encode_ack_bytes( ) -> Result, crate::features::StanzaResponseError> { use wacore_binary::encoder::{ByteWriter, EncodeNode, Encoder}; - let id_val = node - .get_attr("id") - .ok_or(crate::features::StanzaResponseError::MissingAttribute("id"))?; - let from_val = - node.get_attr("from") - .ok_or(crate::features::StanzaResponseError::MissingAttribute( - "from", - ))?; + let id_val = crate::features::required_stanza_attr(node, "id")?; + let from_val = crate::features::required_stanza_attr(node, "from")?; let tag = node.tag.as_ref(); let participant_val = ack_participant(node, from_val, participant_policy); // Server expects `recipient` echoed back so it can route the ack to the diff --git a/src/client/tests.rs b/src/client/tests.rs index 922c5139e..b27dfece0 100644 --- a/src/client/tests.rs +++ b/src/client/tests.rs @@ -2396,6 +2396,19 @@ fn test_encode_ack_bytes_requires_public_response_inputs() { Err(crate::features::StanzaResponseError::MissingAttribute("id")) )); + let empty_id = NodeBuilder::new("receipt") + .attr("id", "") + .attr("from", "12025550111@s.whatsapp.net") + .build(); + assert!(matches!( + encode_ack_bytes( + &empty_id.as_node_ref(), + None, + AckParticipantPolicy::Preserve, + ), + Err(crate::features::StanzaResponseError::MissingAttribute("id")) + )); + let without_from = NodeBuilder::new("receipt") .attr("id", "MISSING-FROM") .build(); @@ -2410,6 +2423,21 @@ fn test_encode_ack_bytes_requires_public_response_inputs() { )) )); + let empty_from = NodeBuilder::new("receipt") + .attr("id", "EMPTY-FROM") + .attr("from", "") + .build(); + assert!(matches!( + encode_ack_bytes( + &empty_from.as_node_ref(), + None, + AckParticipantPolicy::Preserve, + ), + Err(crate::features::StanzaResponseError::MissingAttribute( + "from" + )) + )); + let message = NodeBuilder::new("message") .attr("id", "MISSING-IDENTITY") .attr("from", "12025550111@s.whatsapp.net") diff --git a/src/features/mod.rs b/src/features/mod.rs index b49385f18..ec34d93c3 100644 --- a/src/features/mod.rs +++ b/src/features/mod.rs @@ -78,6 +78,7 @@ pub use profile::{Profile, ProfileError, SetProfilePictureResponse}; pub use status::{Status, StatusPrivacySetting, StatusSendOptions}; pub use signal::{Signal, SignalError, SignalSessionInfo, SignalSessionMigration}; +pub(crate) use stanza::required_stanza_attr; pub use stanza::{ NackReason, RetryReason, RetryRequestError, RetryRequestOptions, RetryRequestOutcome, StanzaRejection, StanzaResponseError, diff --git a/src/features/stanza.rs b/src/features/stanza.rs index fcbee608e..f75a5d1f2 100644 --- a/src/features/stanza.rs +++ b/src/features/stanza.rs @@ -4,6 +4,16 @@ use thiserror::Error; use crate::client::ClientError; +pub(crate) fn required_stanza_attr<'node, 'data>( + node: &'node wacore_binary::NodeRef<'data>, + name: &'static str, +) -> Result<&'node wacore_binary::node::ValueRef<'data>, StanzaResponseError> { + match node.get_attr(name) { + Some(value) if value != "" => Ok(value), + _ => Err(StanzaResponseError::MissingAttribute(name)), + } +} + pub use wacore::protocol::nack::NackReason; pub use wacore::protocol::retry::RetryReason; diff --git a/src/message.rs b/src/message.rs index 58d63dbef..d3c93f10b 100644 --- a/src/message.rs +++ b/src/message.rs @@ -17,11 +17,54 @@ use wacore::protocol::nack::NackReason; use wacore::types::jid::{JidExt, make_sender_key_name}; use wacore_binary::Jid; use wacore_binary::JidExt as _; +use wacore_binary::node::ValueRef; use wacore_binary::{NodeRef, OwnedNodeRef}; use waproto::whatsapp::{self as wa}; use wacore::protocol::retry::MAX_RETRY_COUNT as MAX_DECRYPT_RETRIES; +#[inline] +fn sender_retry_count(enc_node: &NodeRef<'_>) -> u8 { + enc_node + .get_attr("count") + .and_then(|value| match value { + ValueRef::String(value) => value.parse::().ok(), + ValueRef::Jid(_) => None, + }) + .map(|count| count.min(MAX_DECRYPT_RETRIES as u64) as u8) + .unwrap_or(0) +} + +#[inline] +fn attr_matches_jid(value: &ValueRef<'_>, jid: &Jid) -> bool { + match value { + ValueRef::String(value) => wacore_binary::jid::parse_jid_ref(value) + .map(|parsed| jid == &parsed) + .unwrap_or_else(|| value.parse::().is_ok_and(|parsed| jid == &parsed)), + ValueRef::Jid(value) => jid == value, + } +} + +fn message_enc_nodes_for_device<'node, 'data: 'node>( + node: &'node NodeRef<'data>, + own_jid: Option<&'node Jid>, +) -> impl Iterator> + 'node { + let per_device = node + .get_optional_child("participants") + .into_iter() + .flat_map(|participants| participants.get_children_by_tag("to")) + .filter(move |to_node| { + own_jid.is_some_and(|ours| { + to_node + .get_attr("jid") + .is_some_and(|value| attr_matches_jid(value, ours)) + }) + }) + .flat_map(|to_node| to_node.get_children_by_tag("enc")); + + node.get_children_by_tag("enc").chain(per_device) +} + /// Pre-extracted enc node payload. Holds owned copies of the fields needed for /// decryption so the async decrypt phase doesn't borrow the original NodeRef tree. pub(crate) struct EncPayload { diff --git a/src/message/receive.rs b/src/message/receive.rs index 89275dc9c..fd4c4b6cd 100644 --- a/src/message/receive.rs +++ b/src/message/receive.rs @@ -146,26 +146,11 @@ impl Client { let unavailable_node = nr.get_optional_child("unavailable"); + let own_jid = nr + .get_optional_child("participants") + .and_then(|_| self.get_pn()); let mut all_enc_nodes: Vec<&NodeRef<'_>> = Vec::with_capacity(4); - - let direct_enc_nodes = nr.get_children_by_tag("enc"); - all_enc_nodes.extend(direct_enc_nodes); - - let participants = nr.get_optional_child_by_tag(&["participants"]); - if let Some(participants_node) = participants { - let own_jid = self.get_pn(); - let to_nodes = participants_node.get_children_by_tag("to"); - for to_node in to_nodes { - let to_jid = match to_node.attrs().optional_jid("jid") { - Some(jid) => jid, - None => continue, - }; - if own_jid.as_ref().is_some_and(|ours| *ours == to_jid) { - let enc_children = to_node.get_children_by_tag("enc"); - all_enc_nodes.extend(enc_children); - } - } - } + all_enc_nodes.extend(message_enc_nodes_for_device(nr, own_jid.as_ref())); if all_enc_nodes.is_empty() && unavailable_node.is_none() { log::warn!( @@ -246,7 +231,7 @@ impl Client { let mut session_payloads = Vec::with_capacity(all_enc_nodes.len()); let mut group_payloads = Vec::with_capacity(all_enc_nodes.len()); let mut bot_payloads = Vec::with_capacity(all_enc_nodes.len()); - let mut max_sender_retry_count: u8 = 0; + let mut max_sender_retry_count = 0; let mut has_hide_fail = false; let mut had_unknown_enc = false; let mut had_custom_handler = false; @@ -257,14 +242,7 @@ impl Client { let custom_enc_handlers = self.custom_enc_handlers.get(); for enc_node in &all_enc_nodes { - // Parse sender retry count (WA Web: e.maybeAttrInt("count") ?? 0) - // Clamp to MAX_DECRYPT_RETRIES to prevent u64→u8 truncation on unexpected values. - let sender_count = enc_node - .attrs() - .optional_u64("count") - .map(|c| c.min(MAX_DECRYPT_RETRIES as u64) as u8) - .unwrap_or(0); - max_sender_retry_count = max_sender_retry_count.max(sender_count); + max_sender_retry_count = max_sender_retry_count.max(sender_retry_count(enc_node)); // Parse decrypt-fail attribute (WA Web: e.maybeAttrString("decrypt-fail") === "hide") if enc_node diff --git a/src/message/retry.rs b/src/message/retry.rs index dc7dbb674..662504de0 100644 --- a/src/message/retry.rs +++ b/src/message/retry.rs @@ -37,10 +37,19 @@ impl Client { .ok_or(crate::features::RetryRequestError::MissingLocalIdentity)?; let info = wacore::messages::parse_message_info(stanza, own_pn, device.lid.as_ref()) .map_err(crate::features::RetryRequestError::InvalidStanza)?; + let max_sender_retry_count = message_enc_nodes_for_device(stanza, Some(own_pn)) + .map(sender_retry_count) + .max() + .unwrap_or(0); let info = Arc::new(info); drop(device); - self.request_retry_for_info(&info, options).await + self.request_retry_for_info( + &info, + options, + (max_sender_retry_count > 0).then_some(max_sender_retry_count), + ) + .await } /// Dispatch an `UndecryptableMessage` event at most once per `(chat, id)` @@ -261,12 +270,18 @@ impl Client { self: &Arc, info: &Arc, options: crate::features::RetryRequestOptions, + sender_retry_count: Option, ) -> Result { let reason = options.reason(); let cache_key = self .make_retry_cache_key(&info.source.chat, &info.id, &info.source.sender) .await; + if let Some(sender_retry_count) = sender_retry_count { + self.preseed_retry_count(&cache_key, sender_retry_count) + .await; + } + let Some(retry_count) = self.increment_retry_count(&cache_key, reason).await else { log::debug!( "Max retries ({}) reached for message {} from {} [{:?}]. Requesting PDO fallback.", @@ -346,6 +361,7 @@ impl Client { .request_retry_for_info( info, crate::features::RetryRequestOptions::new().with_reason(reason), + None, ) .await { diff --git a/src/message/tests.rs b/src/message/tests.rs index d00185f05..f1000bb3e 100644 --- a/src/message/tests.rs +++ b/src/message/tests.rs @@ -4587,6 +4587,35 @@ async fn test_enc_count_preseeds_retry_cache() { ); } +#[test] +fn message_enc_selection_includes_only_the_local_device_envelopes() { + const OWN_JID: &str = "5511000000001@s.whatsapp.net"; + const OWN_WIRE_JID: &str = "5511000000001:0@s.whatsapp.net"; + let message = NodeBuilder::new("message") + .children([ + NodeBuilder::new("enc").attr("count", "1").build(), + NodeBuilder::new("participants") + .children([ + NodeBuilder::new("to") + .attr("jid", OWN_WIRE_JID) + .children([NodeBuilder::new("enc").attr("count", "2").build()]) + .build(), + NodeBuilder::new("to") + .attr("jid", "13125550112@s.whatsapp.net") + .children([NodeBuilder::new("enc").attr("count", "5").build()]) + .build(), + ]) + .build(), + ]) + .build(); + let own_jid: Jid = OWN_JID.parse().expect("local JID"); + let counts = message_enc_nodes_for_device(&message.as_node_ref(), Some(&own_jid)) + .map(sender_retry_count) + .collect::>(); + + assert_eq!(counts, [1, 2]); +} + #[tokio::test] async fn test_enc_no_count_cache_empty() { let client = create_test_client_for_retry_with_id("enc_no_count").await; @@ -5704,6 +5733,17 @@ async fn explicit_stanza_responses_reject_incomplete_input_without_sending() { Err(crate::features::StanzaResponseError::MissingAttribute("id")) )); + let ack_with_empty_id = NodeBuilder::new("receipt") + .attr("id", "") + .attr("from", "12025550111@s.whatsapp.net") + .build(); + assert!(matches!( + client + .acknowledge_stanza(&ack_with_empty_id.as_node_ref()) + .await, + Err(crate::features::StanzaResponseError::MissingAttribute("id")) + )); + let nack_without_from = NodeBuilder::new("message").attr("id", "NO-FROM").build(); assert!(matches!( client @@ -5717,6 +5757,20 @@ async fn explicit_stanza_responses_reject_incomplete_input_without_sending() { )) )); + let nack_with_empty_id = NodeBuilder::new("message") + .attr("id", "") + .attr("from", "12025550111@s.whatsapp.net") + .build(); + assert!(matches!( + client + .reject_stanza( + &nack_with_empty_id.as_node_ref(), + crate::features::StanzaRejection::new(NackReason::ParsingError), + ) + .await, + Err(crate::features::StanzaResponseError::MissingAttribute("id")) + )); + assert_eq!(transport.sent_count(), 0); transport.fail_next_sends(1); @@ -5910,6 +5964,42 @@ async fn explicit_retry_includes_keys_at_the_normal_threshold() { ); } +#[tokio::test] +async fn explicit_retry_continues_the_sender_echoed_count() { + let (client, _transport) = capturing_client("explicit_retry_sender_count").await; + client + .persistence_manager + .process_command(crate::store::commands::DeviceCommand::SetAccount(Some( + wa::ADVSignedDeviceIdentity::default(), + ))) + .await; + let stanza = NodeBuilder::new("message") + .attr("id", "EXPLICIT-RETRY-SENDER-COUNT") + .attr("from", EXPLICIT_RETRY_SENDER) + .attr("t", "1") + .attr("type", "text") + .children([NodeBuilder::new("enc") + .attr("type", "msg") + .attr("count", "1") + .bytes([0_u8]) + .build()]) + .build(); + + assert_eq!( + client + .request_message_retry( + &stanza.as_node_ref(), + crate::features::RetryRequestOptions::default(), + ) + .await + .expect("sender count should seed the shared retry state"), + crate::features::RetryRequestOutcome::Sent { + retry_count: 2, + included_keys: true, + } + ); +} + #[tokio::test] async fn explicit_retry_preserves_canonical_routing_shapes() { let (client, transport) = capturing_client("explicit_retry_routing").await; diff --git a/src/receipt.rs b/src/receipt.rs index 2de644c80..8683f2ce6 100644 --- a/src/receipt.rs +++ b/src/receipt.rs @@ -307,17 +307,11 @@ impl NackSource for NodeRef<'_> { } fn id(&self) -> Result { - self.get_attr("id") - .map(|value| value.to_node_value()) - .ok_or(crate::features::StanzaResponseError::MissingAttribute("id")) + crate::features::required_stanza_attr(self, "id").map(|value| value.to_node_value()) } fn to(&self) -> Result { - self.get_attr("from") - .map(|value| value.to_node_value()) - .ok_or(crate::features::StanzaResponseError::MissingAttribute( - "from", - )) + crate::features::required_stanza_attr(self, "from").map(|value| value.to_node_value()) } fn participant(&self) -> Option { diff --git a/wacore/binary/src/jid.rs b/wacore/binary/src/jid.rs index 34243178a..92ccbcffb 100644 --- a/wacore/binary/src/jid.rs +++ b/wacore/binary/src/jid.rs @@ -791,6 +791,24 @@ impl<'a> JidRef<'a> { } } +impl PartialEq> for Jid { + #[inline] + fn eq(&self, other: &JidRef<'_>) -> bool { + self.user.as_str() == other.user.as_ref() + && self.server == other.server + && self.agent == other.agent + && self.device == other.device + && self.integrator == other.integrator + } +} + +impl PartialEq for JidRef<'_> { + #[inline] + fn eq(&self, other: &Jid) -> bool { + other == self + } +} + #[cfg(feature = "serde")] impl serde::Serialize for JidRef<'_> { fn serialize(&self, serializer: S) -> Result { @@ -1274,6 +1292,33 @@ mod tests { assert!(borrowed.display_eq_jid(&borrowed_same_display)); } + #[test] + fn owned_and_borrowed_jids_compare_without_conversion() { + let owned = Jid { + user: "12025550111".into(), + server: Server::Pn, + agent: 0, + device: 7, + integrator: 0, + }; + let borrowed = JidRef { + user: NodeStr::Borrowed("12025550111"), + server: Server::Pn, + agent: 0, + device: 7, + integrator: 0, + }; + + assert_eq!(owned, borrowed); + assert_eq!(borrowed, owned); + + let other_device = JidRef { + device: 8, + ..borrowed.clone() + }; + assert_ne!(owned, other_device); + } + #[cfg(feature = "serde")] #[test] fn server_deserializes_borrowed_and_owned_strings() {