-
-
Notifications
You must be signed in to change notification settings - Fork 126
fix(status): use LID addressing, skip unresolvable recipients #568
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 3 commits
b679e02
083f903
87f3b38
765a318
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 |
|---|---|---|
|
|
@@ -290,10 +290,15 @@ impl Client { | |
| Ok(result) | ||
| } | ||
|
|
||
| /// Send a status/story update to the given recipients using sender key encryption. | ||
| /// Send a status/story update using sender-key encryption. | ||
| /// | ||
| /// This builds a `GroupInfo` from the provided recipients (always PN addressing mode), | ||
| /// then reuses the group encryption pipeline with `to = status@broadcast`. | ||
| /// Status uses LID addressing (matches `WAWebEncryptAndSendStatusMsg`): | ||
| /// LID recipients pass through, PN recipients are resolved to LID via | ||
| /// `Client::get_lid_pn_entry` (cache-aside), and unresolvable recipients | ||
| /// are skipped silently. The resulting `GroupInfo` carries | ||
| /// `AddressingMode::Lid`; `prepare_group_stanza` signs with `own_lid` | ||
| /// and emits `addressing_mode="lid"` on the stanza. Errors only if no | ||
| /// recipient could be resolved. | ||
| pub(crate) async fn send_status_message( | ||
| &self, | ||
| message: wa::Message, | ||
|
|
@@ -316,58 +321,39 @@ impl Client { | |
| .pn | ||
| .take() | ||
| .ok_or(crate::client::ClientError::NotLoggedIn)?; | ||
| let own_lid = device_snapshot | ||
| .lid | ||
| .take() | ||
| .unwrap_or_else(|| own_jid.clone()); | ||
|
|
||
| // Status always uses PN addressing. Resolve any LID recipients to their | ||
| // phone numbers so we don't end up with duplicate PN+LID entries for the | ||
| // same user (which causes server error 400). | ||
| // Reject non-user JIDs (groups, broadcasts, etc.) to prevent invalid | ||
| // <participants> entries that cause server errors. | ||
| let mut resolved_recipients = Vec::with_capacity(recipients.len()); | ||
| // Status is LID-addressed (matches WA Web post-LID-migration). Without | ||
| // a real device LID we can't sign or fan out correctly; refuse rather | ||
| // than silently emit `addressing_mode="lid"` with a PN sender. | ||
| let own_lid = device_snapshot.lid.take().ok_or_else(|| { | ||
| anyhow!( | ||
| "Cannot send status: device has no LID yet. Finish pairing / LID \ | ||
| migration before posting status." | ||
| ) | ||
| })?; | ||
|
|
||
| // Fail fast for any JID that isn't a user (PN or LID). Mirrors WA | ||
| // Web's `asUserWidOrThrow` inside `toUserLid`: non-user inputs are a | ||
| // programming bug, not something to silently drop during resolution. | ||
| for jid in recipients { | ||
| if jid.is_group() || jid.is_status_broadcast() || jid.is_broadcast_list() { | ||
| if !(jid.is_pn() || jid.is_lid()) { | ||
| return Err(anyhow!( | ||
| "Invalid status recipient {}: must be a user JID, not a group/broadcast", | ||
| "Invalid status recipient {}: must be a user JID (PN or LID), \ | ||
| not a group/broadcast/newsletter/hosted/etc.", | ||
| jid | ||
| )); | ||
| } | ||
| if jid.is_lid() { | ||
| if let Some(pn) = self.lid_pn_cache.get_phone_number(&jid.user).await { | ||
| resolved_recipients.push(Jid::new(&pn, Server::Pn)); | ||
| } else { | ||
| return Err(anyhow!( | ||
| "No PN mapping for LID {}. Ensure the recipient has been \ | ||
| contacted previously.", | ||
| jid | ||
| )); | ||
| } | ||
| } else { | ||
| resolved_recipients.push(jid.clone()); | ||
| } | ||
| } | ||
|
|
||
| if resolved_recipients.is_empty() { | ||
| return Err(anyhow!("No valid PN recipients after LID resolution")); | ||
| // Resolve every user JID to its LID form (LID passes through; PN goes | ||
| // through the cache-aside lookup; `None` means no mapping — dropped | ||
| // silently to match WA Web `compactMap(list, toUserLid)`). | ||
| let mut resolved: Vec<Option<Jid>> = Vec::with_capacity(recipients.len()); | ||
| for jid in recipients { | ||
| resolved.push(self.resolve_recipient_to_lid(jid).await); | ||
| } | ||
|
|
||
| // Deduplicate by user (in case both LID and PN were provided for the same user) | ||
| let mut seen_users = std::collections::HashSet::new(); | ||
| resolved_recipients.retain(|jid| seen_users.insert(jid.user.clone())); | ||
|
|
||
| let mut group_info = GroupInfo::new(resolved_recipients, AddressingMode::Pn); | ||
|
|
||
| // Ensure we're in the participant list | ||
| let own_base = own_jid.to_non_ad(); | ||
| if !group_info | ||
| .participants | ||
| .iter() | ||
| .any(|p| p.is_same_user_as(&own_base)) | ||
| { | ||
| group_info.participants.push(own_base); | ||
| } | ||
| let participants = wacore::send::assemble_status_participants(resolved, &own_lid)?; | ||
| let mut group_info = GroupInfo::new(participants, AddressingMode::Lid); | ||
|
jlucaso1 marked this conversation as resolved.
Outdated
|
||
|
|
||
| self.add_recent_message(&to, &request_id, &message).await; | ||
|
|
||
|
|
@@ -376,7 +362,11 @@ impl Client { | |
|
|
||
| let force_skdm = { | ||
| use wacore::libsignal::store::sender_key_name::SenderKeyName; | ||
| let sender_address = own_jid.to_protocol_address(); | ||
| // Sender key name tracks the addressing mode of the group stanza. | ||
| // Since status now uses LID addressing (see send_status_message | ||
| // header), the key is stored under own_lid, matching the address | ||
| // prepare_group_stanza derives internally. | ||
| let sender_address = own_lid.to_protocol_address(); | ||
| let sender_key_name = SenderKeyName::from_parts(&to_str, sender_address.as_str()); | ||
|
Comment on lines
+374
to
379
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. Normalize the sender-key identity to the bare LID before building the key name. Right now this keys sender-key state off the device-scoped Suggested fix let force_skdm = {
use wacore::libsignal::store::sender_key_name::SenderKeyName;
@@
- let sender_address = own_lid.to_protocol_address();
+ let sender_address = own_lid.to_non_ad().to_protocol_address();
let sender_key_name = SenderKeyName::from_parts(&to_str, sender_address.as_str());🤖 Prompt for AI Agents
Collaborator
Author
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. I checked the statement "receiver-side expects bare LID" in docs/captured-js/WAWeb/Signal/Address.js:15-34 and Signal/CommonUtils.js:30-48: toString = function () { var t = this.wid.device != null && this.wid.device !== 0 ? ":" + this.wid.device : ""; ... return [u.user, t, "@lid"].join(""); } WA Web includes :device in the name when device != 0, and createSignalLikeSenderKeyName uses this toString() + .0. It forms device-scoped, not bare. The current code uses own_lid.to_protocol_address() both in the force_skdm check and within prepare_group_stanza (line 1054). Both match each other and match with WA Web. Changing to .to_non_ad().to_protocol_address() would break: Parity with WA Web receivers (which expect device-scoped). Consistency with the group send path, which already works with device-scoped. Applying this finding would introduce a bug where there wasn't one. The reviewer started from an incorrect premise about what the receiver reads. 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.
✏️ Learnings added
|
||
|
|
||
| let device_guard = device_store_arc.read().await; | ||
|
|
@@ -396,7 +386,7 @@ impl Client { | |
| let skdm_target_devices: Option<Vec<Jid>> = if force_skdm { | ||
| None | ||
| } else { | ||
| self.resolve_skdm_targets(&to_str, &group_info.participants, &own_jid) | ||
| self.resolve_skdm_targets(&to_str, &group_info.participants, &own_lid) | ||
| .await | ||
|
jlucaso1 marked this conversation as resolved.
Outdated
|
||
| }; | ||
|
|
||
|
|
||
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.
When
get_lid_pn_entryfails (e.g., transient DB/backend outage), this path converts the error intoNone, andsend_status_messagetreats that recipient as merely “unresolvable” and silently drops it. That means status sends can partially succeed while omitting valid recipients without surfacing an error to callers, which is especially risky for status revoke flows that must target the original audience. Lookup failures should be surfaced distinctly from a true “no mapping” miss.Useful? React with 👍 / 👎.