Skip to content

fix(send): isolate a group device's session-setup failure from the cohort #996

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.

Sign up for GitHub

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

Merged
jlucaso1 merged 4 commits into from
Jul 7, 2026
Merged

fix(send): isolate a group device's session-setup failure from the cohort #996

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5fa3f8f
fix(send): isolate a group device's session-setup failure from the co…
jlucaso1 Jul 7, 2026
f08d750
test(send): extract signed-bundle helper; tighten isolation comment
jlucaso1 Jul 7, 2026
f26ce47
test(send): finish the signed-bundle refactor across session helpers
jlucaso1 Jul 7, 2026
fcca55d
style: rustfmt the trimmed import block
jlucaso1 Jul 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion wacore/src/send/encrypt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,13 @@ pub async fn ensure_sessions_for_devices(
// identity; notify the client so it can react off-path.
Ok(Ok(Some(changed_jid))) => resolver.on_local_identity_change(&changed_jid),
Ok(Ok(None)) => {}
Ok(Err(e)) => return Err(e),
// Isolate the failure to this device so one participant can't abort
// the cohort's SKDM (matching WA Web GroupKeyDistributionMsg's
// per-device try/catch). The sessionless device is dropped by the
// fan-out below.
Ok(Err(e)) => {

@cubic-dev-ai cubic-dev-ai Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1: When a device reaches this setup-failure branch, it can still be persisted as has_key=true even though no SKDM was encrypted for it. Consider carrying the setup-failed device list through the plan/result or otherwise excluding these failures from PreparedGroupStanza.skdm_devices, especially for own companions that do not have a retry-receipt repair path.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At wacore/src/send/encrypt.rs, line 616:

<comment>When a device reaches this setup-failure branch, it can still be persisted as `has_key=true` even though no SKDM was encrypted for it. Consider carrying the setup-failed device list through the plan/result or otherwise excluding these failures from `PreparedGroupStanza.skdm_devices`, especially for own companions that do not have a retry-receipt repair path.</comment>

<file context>
@@ -604,7 +604,18 @@ pub async fn ensure_sessions_for_devices(
+                // GroupKeyDistributionMsg wraps each device's encrypt in try/catch
+                // and drops only the failing one; the skipped device just stays
+                // sessionless and is skipped by the fan-out below.
+                Ok(Err(e)) => {
+                    log::warn!("Group session setup failed for a device, skipping it: {e}");
+                }
</file context>

log::warn!("Group session setup failed for a device, skipping it: {e}");
}
Err(SpawnCanceled) => {
log::warn!(
"Session-establishment task did not deliver a result; skipping device."
Expand Down
165 changes: 109 additions & 56 deletions wacore/src/send/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -802,6 +802,30 @@ fn create_mock_bundle() -> PreKeyBundle {
.expect("Failed to create PreKeyBundle")
}

/// A bundle whose signed-prekey signature actually verifies, so
/// `process_prekey_bundle` establishes a session. Contrast `create_mock_bundle`,
/// whose zeroed signature deliberately fails X3DH (used to exercise the reject path).
fn signed_prekey_bundle() -> PreKeyBundle {
let mut rng = rand::make_rng::<rand::rngs::StdRng>();
let receiver = IdentityKeyPair::generate(&mut rng);
let spk = KeyPair::generate(&mut rng);
let opk = KeyPair::generate(&mut rng);
let sig = receiver
.private_key()
.calculate_signature(&spk.public_key.serialize(), &mut rng)
.unwrap();
PreKeyBundle::new(
1,
1u32.into(),
Some((1u32.into(), opk.public_key)),
1u32.into(),
spk.public_key,
sig.to_vec(),
*receiver.identity_key(),
)
.unwrap()
}

Comment thread
coderabbitai[bot] marked this conversation as resolved.
// These tests validate the fix for the LID-PN session mismatch issue.
// When a message is received with sender_lid, the session is stored under the LID address.
// When sending a reply using the phone number, we must reuse the existing LID session
Expand Down Expand Up @@ -1212,8 +1236,8 @@ fn test_lid_prekey_lookup_normalization() {
mod group_retry {
use super::*;
use crate::libsignal::protocol::{
Direction, IdentityChange, IdentityKey, IdentityKeyPair, IdentityKeyStore, KeyPair,
PreKeyBundle, ProtocolAddress, SessionStore, process_prekey_bundle,
Direction, IdentityChange, IdentityKey, IdentityKeyPair, IdentityKeyStore,
ProtocolAddress, SessionStore, process_prekey_bundle,
};
use crate::types::message::AddressingMode;
use std::collections::HashMap;
Expand Down Expand Up @@ -1300,23 +1324,7 @@ mod group_retry {
async fn setup_session() -> (MemSessionStore, MemIdentityStore, Jid) {
let mut rng = rand::make_rng::<rand::rngs::StdRng>();
let sender = IdentityKeyPair::generate(&mut rng);
let receiver = IdentityKeyPair::generate(&mut rng);
let spk = KeyPair::generate(&mut rng);
let opk = KeyPair::generate(&mut rng);
let sig = receiver
.private_key()
.calculate_signature(&spk.public_key.serialize(), &mut rng)
.unwrap();
let bundle = PreKeyBundle::new(
1,
1u32.into(),
Some((1u32.into(), opk.public_key)),
1u32.into(),
spk.public_key,
sig.to_vec(),
*receiver.identity_key(),
)
.unwrap();
let bundle = signed_prekey_bundle();
let jid: Jid = "559911112222@s.whatsapp.net".parse().unwrap();
let addr = jid.to_protocol_address();
let mut ss = MemSessionStore::new();
Expand Down Expand Up @@ -2924,23 +2932,7 @@ mod mark_full_distribution_list {
async fn established_stores(a: &Jid) -> (MemSessionStore, MemIdentityStore) {
let mut rng = rand::make_rng::<rand::rngs::StdRng>();
let sender = IdentityKeyPair::generate(&mut rng);
let receiver = IdentityKeyPair::generate(&mut rng);
let spk = KeyPair::generate(&mut rng);
let opk = KeyPair::generate(&mut rng);
let sig = receiver
.private_key()
.calculate_signature(&spk.public_key.serialize(), &mut rng)
.unwrap();
let bundle = PreKeyBundle::new(
1,
1u32.into(),
Some((1u32.into(), opk.public_key)),
1u32.into(),
spk.public_key,
sig.to_vec(),
*receiver.identity_key(),
)
.unwrap();
let bundle = signed_prekey_bundle();
let mut ss = MemSessionStore::default();
let mut is = MemIdentityStore {
pair: sender,
Expand Down Expand Up @@ -3193,27 +3185,8 @@ mod mark_full_distribution_list {
signed_prekey_store: &spks,
};

// Verifiable bundle (create_mock_bundle's zeroed signature fails X3DH).
let receiver = IdentityKeyPair::generate(&mut rng);
let spk = KeyPair::generate(&mut rng);
let opk = KeyPair::generate(&mut rng);
let sig = receiver
.private_key()
.calculate_signature(&spk.public_key.serialize(), &mut rng)
.unwrap();
let bundle = PreKeyBundle::new(
1,
1u32.into(),
Some((1u32.into(), opk.public_key)),
1u32.into(),
spk.public_key,
sig.to_vec(),
*receiver.identity_key(),
)
.unwrap();

let resolver = MockSendContextResolver::new()
.with_bundle(b.clone(), bundle)
.with_bundle(b.clone(), signed_prekey_bundle())
.with_chain_lock_probe(probe.clone());
let rt = TokioTestRuntime;

Expand Down Expand Up @@ -3271,6 +3244,86 @@ mod mark_full_distribution_list {
"B must receive a pairwise SKDM via the pre-established session"
);
}

/// One participant's session-setup failure must NOT abort the SKDM for the
/// rest of the cohort: the good device still gets its pairwise SKDM, the bad
/// one is dropped. Before the fix, the failing device's process_prekey_bundle
/// error nulled the whole session_plan, so no device got an SKDM (and the
/// cohort was still marked has_key=true, orphaning own companions).
#[tokio::test]
async fn group_skdm_setup_failure_is_isolated_to_the_bad_device() {
let group: Jid = "120363000000000003@g.us".parse().unwrap();
let own_jid: Jid = "559900000001@s.whatsapp.net".parse().unwrap();
let own_lid: Jid = "100000000000001@lid".parse().unwrap();
// good: valid bundle → session establishes. bad: create_mock_bundle's
// zeroed signature fails X3DH inside process_prekey_bundle.
let good: Jid = "559911112222:0@s.whatsapp.net".parse().unwrap();
let bad: Jid = "559933334444:0@s.whatsapp.net".parse().unwrap();

let mut rng = rand::make_rng::<rand::rngs::StdRng>();
let mut ss = MemSessionStore::default();
let mut is = MemIdentityStore {
pair: IdentityKeyPair::generate(&mut rng),
reg_id: 7,
known: Default::default(),
};
let mut sks = MemSenderKeyStore::default();
let mut pks = UnusedPreKeyStore;
let spks = UnusedSignedPreKeyStore;
let mut stores = SignalStores {
sender_key_store: &mut sks,
session_store: &mut ss,
identity_store: &mut is,
prekey_store: &mut pks,
signed_prekey_store: &spks,
};

let resolver = MockSendContextResolver::new()
.with_bundle(good.clone(), signed_prekey_bundle())
.with_bundle(bad.clone(), create_mock_bundle());
let rt = TokioTestRuntime;

let group_info = GroupInfo::new(
vec![own_jid.to_non_ad(), good.to_non_ad(), bad.to_non_ad()],
AddressingMode::Pn,
);
let msg = wa::Message {
conversation: Some("hi".into()),
..Default::default()
};

let prepared = prepare_group_stanza(
&rt,
&mut stores,
&resolver,
&group_info,
&own_jid,
&own_lid,
None,
group,
&msg,
"TESTREQID_ISO".into(),
false,
Some(vec![good.clone(), bad.clone()]),
None,
None,
&[],
None,
)
.await
.expect("prepare_group_stanza must succeed despite one device's setup failure");

let participants = prepared
.node
.get_optional_child("participants")
.expect("the good device's SKDM must still be distributed");
assert_eq!(
participants.children().map(|c| c.len()).unwrap_or(0),
1,
"only the good device receives an SKDM; the failed one is skipped, \
not aborting the whole cohort"
);
}
}

Comment thread
greptile-apps[bot] marked this conversation as resolved.
/// Item 3 — phash device-set construction. The set hashed is the full
Expand Down
Loading