-
-
Notifications
You must be signed in to change notification settings - Fork 127
fix: centralize timestamp handling via wacore::time and fix signed parsing #532
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
b2013c1
ac2353d
e29e9f5
57d53ce
7933363
eb1f038
8115993
02c4dbf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -219,10 +219,15 @@ impl Client { | |
| self.store_tc_token_from_conversation_bytes(&raw_bytes) | ||
| .await; | ||
|
|
||
| // Wrap Bytes in LazyConversation using from_bytes (true zero-copy) | ||
| // Parsing only happens if the event handler calls .conversation() or .get() | ||
| let lazy_conv = LazyConversation::from_bytes(raw_bytes); | ||
| self.core.event_bus.dispatch(Event::JoinedGroup(lazy_conv)); | ||
| if let Some(conv) = lazy_conv.get() | ||
| && let Ok(group_jid) = conv.id.parse() | ||
| { | ||
| self.core.event_bus.dispatch(Event::JoinedGroup { | ||
| group_jid, | ||
| conversation: lazy_conv, | ||
| }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
💡 Suggested fix- let lazy_conv = LazyConversation::from_bytes(raw_bytes);
- if let Some(conv) = lazy_conv.get()
- && let Ok(group_jid) = conv.id.parse()
- {
+ if let Ok(conv) =
+ <ConversationTcTokenFields as prost::Message>::decode(raw_bytes.clone())
+ && let Ok(group_jid) = conv.id.parse::<wacore_binary::Jid>()
+ && group_jid.is_group()
+ {
+ let lazy_conv = LazyConversation::from_bytes(raw_bytes);
self.core.event_bus.dispatch(Event::JoinedGroup {
group_jid,
conversation: lazy_conv,
});
}🤖 Prompt for AI Agents |
||
| } | ||
| } | ||
|
|
||
| // Drop receiver before awaiting the blocking task. If we broke out | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,7 @@ use wacore_binary::Node; | |
| pub fn notification_timestamp(node: &Node) -> chrono::DateTime<chrono::Utc> { | ||
| node.attrs() | ||
| .optional_u64("t") | ||
| .and_then(|t| chrono::DateTime::from_timestamp(t as i64, 0)) | ||
| .and_then(|t| crate::time::from_secs(t as i64)) | ||
| .unwrap_or_else(crate::time::now_utc) | ||
|
Comment on lines
21
to
24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: When converting a u64 to i64 using the as cast where the u64 value exceeds i64::MAX (9223372036854775807), it performs a reinterpretation of the bit pattern. Since both types are 64 bits and Rust uses two's complement for signed integers, casting u64 to i64 is a no-op on the bits—it reinterprets the unsigned bits as a signed value. For values <= i64::MAX, the numerical value is preserved. For values > i64::MAX, the result is a negative i64. Specifically, u64::MAX (all bits 1) becomes -1i64, as confirmed by the official documentation: u64::cast_signed example shows u64::MAX.cast_signed == -1i64, and this produces the same result as as. This is neither saturation (clamping to i64::MAX), nor does it error or panic. It "wraps" in the sense of bit reinterpretation, producing a negative number equivalent to the two's complement interpretation. Official Rust Reference confirms: "Casting between two integers of the same size (e.g. i32 -> u32) is a no-op (Rust uses 2's complement for negative numbers)." Rust By Example notes: "When casting to a signed type, the (bitwise) result is the same as first casting to the corresponding unsigned type." It does not panic on overflow like arithmetic operations in debug mode; casts are always defined and do not trigger overflow checks. Citations:
🏁 Script executed: cat -n wacore/src/stanza/notification.rs | head -50Repository: jlucaso1/whatsapp-rust Length of output: 2503 🏁 Script executed: rg "parse_disappearing_mode" wacore/src/stanza/notification.rs -A 10Repository: jlucaso1/whatsapp-rust Length of output: 2290 🏁 Script executed: rg "fn from_secs" wacore/src/time.rs -A 5Repository: jlucaso1/whatsapp-rust Length of output: 521 🏁 Script executed: rg "notification_timestamp" wacore/src -B 2 -A 5Repository: jlucaso1/whatsapp-rust Length of output: 1924 🏁 Script executed: rg "from_timestamp" wacore/src -B 2 -A 2Repository: jlucaso1/whatsapp-rust Length of output: 813 🏁 Script executed: # Check if there are any tests for notification_timestamp that show the wrapping issue
rg "test.*notification_timestamp" wacore/src/stanza/notification.rs -A 15Repository: jlucaso1/whatsapp-rust Length of output: 48 Use signed parsing or checked conversion for the timestamp. The current code uses Suggested change pub fn notification_timestamp(node: &Node) -> chrono::DateTime<chrono::Utc> {
node.attrs()
- .optional_u64("t")
- .and_then(|t| crate::time::from_secs(t as i64))
+ .optional_string("t")
+ .and_then(|t| t.parse::<i64>().ok())
+ .and_then(crate::time::from_secs)
.unwrap_or_else(crate::time::now_utc)
}Parsing as 🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -316,9 +316,11 @@ impl From<crate::stanza::business::BusinessNotificationType> for BusinessUpdateT | |
| /// Business status update notification. | ||
| #[derive(Debug, Clone, Serialize)] | ||
| pub struct BusinessStatusUpdate { | ||
| /// The business account whose status changed. | ||
| pub jid: Jid, | ||
| pub update_type: BusinessUpdateType, | ||
| pub timestamp: i64, | ||
| #[serde(with = "chrono::serde::ts_seconds")] | ||
| pub timestamp: DateTime<Utc>, | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub target_jid: Option<Jid>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
|
|
@@ -344,9 +346,10 @@ pub struct DisappearingModeChanged { | |
| pub from: Jid, | ||
| /// New duration in seconds (0 = disabled, 86400 = 24h, etc.). | ||
| pub duration: u32, | ||
| /// Unix timestamp (seconds) when the setting was changed. | ||
| /// Consumers should only apply this if it's newer than their stored timestamp. | ||
| pub setting_timestamp: u64, | ||
| /// When the setting was changed. | ||
| /// Consumers should only apply this if it's newer than their stored value. | ||
| #[serde(with = "chrono::serde::ts_seconds")] | ||
| pub setting_timestamp: DateTime<Utc>, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Serialize)] | ||
|
|
@@ -386,7 +389,10 @@ pub enum Event { | |
| ContactNumberChanged(ContactNumberChanged), | ||
| ContactSyncRequested(ContactSyncRequested), | ||
|
|
||
| JoinedGroup(LazyConversation), | ||
| JoinedGroup { | ||
| group_jid: Jid, | ||
| conversation: LazyConversation, | ||
| }, | ||
| /// Group metadata/settings/participant change from w:gp2 notification. | ||
| GroupUpdate(GroupUpdate), | ||
| ContactUpdate(ContactUpdate), | ||
|
|
@@ -445,12 +451,67 @@ impl Event { | |
| let (msg, _) = self.as_message()?; | ||
| msg.conversation.as_deref() | ||
| } | ||
|
|
||
| /// Returns the primary JID associated with this event, if any. | ||
| /// | ||
| /// Useful for routing or filtering events without exhaustively matching every variant. | ||
| /// Returns `None` for connection-lifecycle and sync events that have no associated JID. | ||
| pub fn primary_jid(&self) -> Option<&Jid> { | ||
| match self { | ||
| Event::Message(_, info) => Some(&info.source.chat), | ||
| Event::Receipt(r) => Some(&r.source.chat), | ||
| Event::UndecryptableMessage(u) => Some(&u.info.source.chat), | ||
| Event::ChatPresence(c) => Some(&c.source.chat), | ||
| Event::Presence(p) => Some(&p.from), | ||
| Event::PictureUpdate(p) => Some(&p.jid), | ||
| Event::UserAboutUpdate(u) => Some(&u.jid), | ||
| Event::ContactUpdated(c) => Some(&c.jid), | ||
| Event::ContactNumberChanged(c) => Some(&c.new_jid), | ||
| Event::GroupUpdate(g) => Some(&g.group_jid), | ||
| Event::JoinedGroup { group_jid, .. } => Some(group_jid), | ||
| Event::ContactUpdate(c) => Some(&c.jid), | ||
| Event::PushNameUpdate(p) => Some(&p.jid), | ||
| Event::PinUpdate(p) => Some(&p.jid), | ||
| Event::MuteUpdate(m) => Some(&m.jid), | ||
| Event::ArchiveUpdate(a) => Some(&a.jid), | ||
| Event::StarUpdate(s) => Some(&s.chat_jid), | ||
| Event::MarkChatAsReadUpdate(m) => Some(&m.jid), | ||
| Event::DeleteChatUpdate(d) => Some(&d.jid), | ||
| Event::DeleteMessageForMeUpdate(d) => Some(&d.chat_jid), | ||
| Event::BusinessStatusUpdate(b) => Some(&b.jid), | ||
| Event::DisappearingModeChanged(d) => Some(&d.from), | ||
| Event::NewsletterLiveUpdate(n) => Some(&n.newsletter_jid), | ||
| Event::DeviceListUpdate(d) => Some(&d.user), | ||
| Event::IdentityChange(i) => Some(&i.user), | ||
| Event::Connected(_) | ||
| | Event::Disconnected(_) | ||
| | Event::PairSuccess(_) | ||
| | Event::PairError(_) | ||
| | Event::LoggedOut(_) | ||
| | Event::PairingQrCode { .. } | ||
| | Event::PairingCode { .. } | ||
| | Event::QrScannedWithoutMultidevice(_) | ||
| | Event::ClientOutdated(_) | ||
| | Event::SelfPushNameUpdated(_) | ||
| | Event::HistorySync(_) | ||
| | Event::OfflineSyncPreview(_) | ||
| | Event::OfflineSyncCompleted(_) | ||
| | Event::StreamReplaced(_) | ||
| | Event::TemporaryBan(_) | ||
| | Event::ConnectFailure(_) | ||
| | Event::StreamError(_) | ||
| | Event::ContactSyncRequested(_) | ||
| | Event::Notification(_) | ||
| | Event::RawNode(_) => None, | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
🤖 Prompt for AI Agents |
||
| } | ||
| } | ||
|
|
||
| /// A newsletter live update notification, typically containing updated | ||
| /// reaction counts for one or more messages. | ||
| #[derive(Debug, Clone, Serialize)] | ||
| pub struct NewsletterLiveUpdate { | ||
| /// The newsletter channel this update belongs to. | ||
| pub newsletter_jid: Jid, | ||
| pub messages: Vec<NewsletterLiveUpdateMessage>, | ||
| } | ||
|
|
@@ -706,6 +767,7 @@ pub struct ChatPresenceUpdate { | |
|
|
||
| #[derive(Debug, Clone, Serialize)] | ||
| pub struct PresenceUpdate { | ||
| /// The contact whose presence changed. | ||
| pub from: Jid, | ||
| pub unavailable: bool, | ||
| pub last_seen: Option<DateTime<Utc>>, | ||
|
|
@@ -729,6 +791,7 @@ pub struct PictureUpdate { | |
|
|
||
| #[derive(Debug, Clone, Serialize)] | ||
| pub struct UserAboutUpdate { | ||
| /// The contact whose about text changed. | ||
| pub jid: Jid, | ||
| pub status: String, | ||
| pub timestamp: DateTime<Utc>, | ||
|
|
@@ -744,6 +807,7 @@ pub struct UserAboutUpdate { | |
| /// sync mutations (different source, different payload). | ||
| #[derive(Debug, Clone, Serialize)] | ||
| pub struct ContactUpdated { | ||
| /// The contact whose profile was updated. | ||
| pub jid: Jid, | ||
| pub timestamp: DateTime<Utc>, | ||
| } | ||
|
|
@@ -804,6 +868,7 @@ pub struct GroupUpdate { | |
|
|
||
| #[derive(Debug, Clone, Serialize)] | ||
| pub struct ContactUpdate { | ||
| /// The chat/contact this sync action applies to. | ||
| pub jid: Jid, | ||
| pub timestamp: DateTime<Utc>, | ||
| pub action: Box<wa::sync_action_value::ContactAction>, | ||
|
|
@@ -812,14 +877,17 @@ pub struct ContactUpdate { | |
|
|
||
| #[derive(Debug, Clone, Serialize)] | ||
| pub struct PushNameUpdate { | ||
| /// The contact who changed their push name. | ||
| pub jid: Jid, | ||
| pub message: Box<MessageInfo>, | ||
| pub message: Arc<MessageInfo>, | ||
| pub old_push_name: String, | ||
| pub new_push_name: String, | ||
| pub from_full_sync: bool, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Serialize)] | ||
| pub struct PinUpdate { | ||
| /// The chat being pinned or unpinned. | ||
| pub jid: Jid, | ||
| pub timestamp: DateTime<Utc>, | ||
| pub action: Box<wa::sync_action_value::PinAction>, | ||
|
|
@@ -828,6 +896,7 @@ pub struct PinUpdate { | |
|
|
||
| #[derive(Debug, Clone, Serialize)] | ||
| pub struct MuteUpdate { | ||
| /// The chat being muted or unmuted. | ||
| pub jid: Jid, | ||
| pub timestamp: DateTime<Utc>, | ||
| pub action: Box<wa::sync_action_value::MuteAction>, | ||
|
|
@@ -836,6 +905,7 @@ pub struct MuteUpdate { | |
|
|
||
| #[derive(Debug, Clone, Serialize)] | ||
| pub struct ArchiveUpdate { | ||
| /// The chat being archived or unarchived. | ||
| pub jid: Jid, | ||
| pub timestamp: DateTime<Utc>, | ||
| pub action: Box<wa::sync_action_value::ArchiveChatAction>, | ||
|
|
@@ -844,6 +914,7 @@ pub struct ArchiveUpdate { | |
|
|
||
| #[derive(Debug, Clone, Serialize)] | ||
| pub struct StarUpdate { | ||
| /// The chat containing the starred or unstarred message. | ||
| pub chat_jid: Jid, | ||
| /// The participant who sent the message. `Some` for group messages from | ||
| /// others, `None` for self-authored or 1-on-1 messages (wire value `"0"`). | ||
|
|
@@ -857,6 +928,7 @@ pub struct StarUpdate { | |
|
|
||
| #[derive(Debug, Clone, Serialize)] | ||
| pub struct MarkChatAsReadUpdate { | ||
| /// The chat being marked as read or unread. | ||
| pub jid: Jid, | ||
| pub timestamp: DateTime<Utc>, | ||
| pub action: Box<wa::sync_action_value::MarkChatAsReadAction>, | ||
|
|
@@ -865,6 +937,7 @@ pub struct MarkChatAsReadUpdate { | |
|
|
||
| #[derive(Debug, Clone, Serialize)] | ||
| pub struct DeleteChatUpdate { | ||
| /// The chat being deleted. | ||
| pub jid: Jid, | ||
| /// From the index, not the proto — DeleteChatAction only has messageRange. | ||
| pub delete_media: bool, | ||
|
|
@@ -875,6 +948,7 @@ pub struct DeleteChatUpdate { | |
|
|
||
| #[derive(Debug, Clone, Serialize)] | ||
| pub struct DeleteMessageForMeUpdate { | ||
| /// The chat containing the deleted message. | ||
| pub chat_jid: Jid, | ||
| pub participant_jid: Option<Jid>, | ||
| pub message_id: String, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: jlucaso1/whatsapp-rust
Length of output: 689
🏁 Script executed:
Repository: jlucaso1/whatsapp-rust
Length of output: 92
🏁 Script executed:
Repository: jlucaso1/whatsapp-rust
Length of output: 257
🏁 Script executed:
Repository: jlucaso1/whatsapp-rust
Length of output: 92
🏁 Script executed:
Repository: jlucaso1/whatsapp-rust
Length of output: 240
🏁 Script executed:
Repository: jlucaso1/whatsapp-rust
Length of output: 681
🏁 Script executed:
Repository: jlucaso1/whatsapp-rust
Length of output: 7301
🏁 Script executed:
Repository: jlucaso1/whatsapp-rust
Length of output: 3823
Avoid
u64 as i64forafterto reject oversized timestamps.Rust wraps on overflow, so a malformed
afterabovei64::MAXbecomes a negative timestamp (before 1970) instead ofNone, likely unintended for a sync request attribute.💡 Suggested fix
let after = child .attrs() .optional_u64("after") - .and_then(|after| wacore::time::from_secs(after as i64)); + .and_then(|after| i64::try_from(after).ok()) + .and_then(wacore::time::from_secs);📝 Committable suggestion
🤖 Prompt for AI Agents