Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion wacore/src/send/encrypt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,18 @@ 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: one participant's session
// setup must NOT abort the whole cohort. Aborting here left every
// other target — including our own companion devices — without an
// SKDM even though their sessions established fine, and the cohort
// was then marked has_key=true, permanently orphaning own companions
// (their retry path is an own-device no-op). WA Web's
// 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)) => {

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}");
Comment thread
greptile-apps[bot] marked this conversation as resolved.
Outdated
}
Err(SpawnCanceled) => {
log::warn!(
"Session-establishment task did not deliver a result; skipping device."
Expand Down
99 changes: 99 additions & 0 deletions wacore/src/send/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3271,6 +3271,105 @@ 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,
};

// Properly-signed bundle for the good device.
let receiver = IdentityKeyPair::generate(&mut rng);
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
Outdated
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 good_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(good.clone(), good_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