mls_group: reserve joiner leaf before applying Adds in external commits

Before this change, external commits silently miscomputed leaf placement
when they carried by-value `Add` proposals alongside the implicit
`ExternalInit`. The joiner's `own_leaf_index` was determined by
`leftmost_free_index` at builder time (over the queued proposals only),
but during `apply_proposals` the new addees called `add_leaf`, which
also picks the leftmost-free slot, racing the joiner for the same
index. The placeholder for the joiner's leaf was then planted in
`apply_own_update_path` (send) / `apply_received_update_path` (receive),
but by then the slot had been taken by a co-resident Add, leading to
either an `InconsistentSenderIndex` rejection on the receive side or an
encryption-failure when the path was computed against a leaf the
addees had displaced.

This commit moves the joiner-leaf placeholder reservation up into
`apply_proposals`, between Remove processing and Add processing, when
an `ExternalInit` proposal is present in the queue. After Removes have
been applied to the diff, `diff.free_leaf_index()` matches the
`PublicGroup::leftmost_free_index` computation used by both sides to
derive the joiner's slot, so the placeholder lands in the right place
without needing to thread additional indices through the function.

The duplicate placeholder add in `apply_own_update_path` becomes a
no-op (the leaf is already there); a defensive fallback is kept in
case the function is invoked from a path that did not run
`apply_proposals` (e.g. the treekem KAT tests).

Adds two new integration tests in `openmls/tests/external_commit.rs`:

  * `test_external_commit_with_inline_add` — Alice creates a group,
    Bob1 joins via external commit including a by-value Add for Bob2.
    Asserts that the CommitMessageBundle includes a Welcome, that
    Alice can process the commit, that Bob2 can join via the Welcome,
    and that all three converge on the same epoch and tree.

  * `test_external_commit_with_inline_app_data_update` (gated on
    `extensions-draft-08`) — Bob joins via external commit and ships
    an `AppDataUpdate` proposal that mutates the AppDataDictionary
    atomically with the join. Asserts that the dictionary is updated
    on both sides post-commit.

Existing single-leaf external commits, by-reference rejection
(`test_valsem244`), and AppDataUpdate inside member commits are
unaffected.

Author: tylerhawkes

openmls/src/group/public_group/diff/apply_proposals.rs

@@ -129,6 +129,27 @@ impl PublicGroupDiff<'_> {
             }
         }
 
+        // For external commits, reserve the joiner's own leaf with a placeholder
+        // before processing Add proposals. Otherwise the by-value Adds would
+        // race for the leftmost-free slot and collide with the joiner's slot
+        // (which was chosen by `leftmost_free_index` at external-commit-builder
+        // time, before the joiner was aware of any subsequent by-value Adds).
+        //
+        // After Removes and SelfRemoves are processed above, `free_leaf_index`
+        // on the diff matches the `leftmost_free_index` computation used both
+        // by the joiner (when populating `own_leaf_index` for the new group)
+        // and by receivers (when validating the commit, see
+        // `PublicGroup::leftmost_free_index`). The placeholder we plant here
+        // is later overwritten by the joiner's freshly-signed LeafNode in
+        // `apply_own_update_path` on the send side, or by
+        // `apply_received_update_path` on the receive side.
+        if external_init_proposal_option.is_some() {
+            let placeholder = LeafNode::new_placeholder();
+            self.diff
+                .add_leaf(placeholder)
+                .map_err(|_| LibraryError::custom("Tree full: cannot reserve joiner leaf"))?;
+        }
+
         // Process adds
         let add_proposals = proposal_queue
             .filtered_by_type(ProposalType::Add)

openmls/src/treesync/diff.rs

@@ -323,14 +323,20 @@ impl TreeSyncDiff<'_> {
         leaf_index: LeafNodeIndex,
         leaf_node_params: UpdateLeafNodeParams,
     ) -> Result<UpdatePathResult, TreeSyncAddLeaf> {
-        // For External Commits, we temporarily add a placeholder leaf node to the tree, because it
-        // might be required to make the tree grow to the right size. If we
-        // don't do that, calculating the direct path might fail. It's important
-        // to not do anything with the value of that leaf until it has been
-        // replaced.
+        // For External Commits, the joiner's leaf is reserved with a placeholder
+        // earlier, during `apply_proposals`, so that any by-value Add proposals
+        // committed alongside the ExternalInit do not race for the leftmost-free
+        // slot. We only need to grow the tree here if the placeholder somehow
+        // didn't get inserted (e.g. a regular commit path is being computed).
+        // It's important to not do anything with the value of that leaf until
+        // it has been replaced below.
         if let CommitType::External(_) = commit_type {
-            let leaf_node = LeafNode::new_placeholder();
-            self.add_leaf(leaf_node)?;
+            if self.diff.leaf(leaf_index).node().is_none()
+                || leaf_index.u32() >= self.leaf_count()
+            {
+                let leaf_node = LeafNode::new_placeholder();
+                self.add_leaf(leaf_node)?;
+            }
         }
 
         // We calculate the parent hash so that we can use it for a fresh leaf

openmls/tests/external_commit.rs

@@ -228,3 +228,303 @@ fn test_not_present_group_info() {
 
     assert!(group_info.is_none());
 }
+
+/// External commit carrying a by-value `Add` proposal alongside the implicit
+/// `ExternalInit` and the joiner's path leaf. This is the building block for
+/// atomic multi-leaf joins (e.g. a single device joining via external commit
+/// while introducing a co-resident sibling device).
+///
+/// Verifies:
+///   - The resulting [`CommitMessageBundle`] contains a Welcome (because an
+///     invitation list was produced).
+///   - The existing member (Alice) accepts and processes the external commit
+///     including the by-value Add.
+///   - The invitee from the by-value Add (Bob2) can join via the Welcome.
+///   - All three parties converge to the same epoch and tree.
+#[openmls_test]
+fn test_external_commit_with_inline_add() {
+    let alice_provider = &Provider::default();
+    let bob1_provider = &Provider::default();
+    let bob2_provider = &Provider::default();
+
+    // Alice creates a group with ratchet tree extension so we can export a
+    // self-contained GroupInfo for the external commit.
+    let (mut alice_group, _alice_credential, alice_signer) =
+        create_alice_group(ciphersuite, alice_provider, true);
+
+    // Self-update so that the exported GroupInfo carries an external_pub
+    // extension.
+    let group_info = alice_group
+        .self_update(alice_provider, &alice_signer, LeafNodeParameters::default())
+        .unwrap()
+        .into_group_info()
+        .expect("self-update should produce a GroupInfo");
+    alice_group.merge_pending_commit(alice_provider).unwrap();
+
+    let verifiable_group_info = {
+        let serialized = group_info.tls_serialize_detached().unwrap();
+        VerifiableGroupInfo::tls_deserialize(&mut serialized.as_slice()).unwrap()
+    };
+
+    // Bob1 (primary joiner) and Bob2 (co-resident sibling) generate
+    // credentials. Bob2 also generates a KeyPackage that Bob1 will include as
+    // an inline Add proposal in the external commit.
+    let (bob1_credential, bob1_signer) =
+        new_credential(bob1_provider, b"Bob1", ciphersuite.signature_algorithm());
+    let (bob2_credential, bob2_signer) =
+        new_credential(bob2_provider, b"Bob2", ciphersuite.signature_algorithm());
+
+    let bob2_key_package = KeyPackage::builder()
+        .build(ciphersuite, bob2_provider, &bob2_signer, bob2_credential)
+        .unwrap()
+        .key_package()
+        .clone();
+
+    // Bob1 builds an external commit that adds Bob2 by value.
+    let join_config = MlsGroupJoinConfig::builder()
+        .use_ratchet_tree_extension(true)
+        .build();
+    let (bob1_group, bundle) = MlsGroup::external_commit_builder()
+        .with_config(join_config.clone())
+        .build_group(bob1_provider, verifiable_group_info, bob1_credential)
+        .unwrap()
+        .propose_adds([bob2_key_package])
+        .load_psks(bob1_provider.storage())
+        .unwrap()
+        .build(
+            bob1_provider.rand(),
+            bob1_provider.crypto(),
+            &bob1_signer,
+            |_| true,
+        )
+        .unwrap()
+        .finalize(bob1_provider)
+        .unwrap();
+
+    // The bundle MUST carry a Welcome for Bob2.
+    assert!(
+        bundle.welcome().is_some(),
+        "external commit with by-value Add must produce a Welcome"
+    );
+    let welcome = bundle.welcome().unwrap().clone();
+    let commit_msg: MlsMessageIn = bundle.commit().clone().into();
+
+    // Alice processes Bob1's external commit.
+    let processed = alice_group
+        .process_message(
+            alice_provider,
+            commit_msg.try_into_protocol_message().unwrap(),
+        )
+        .expect("Alice should accept external commit with inline Add");
+    match processed.into_content() {
+        ProcessedMessageContent::StagedCommitMessage(staged) => {
+            alice_group
+                .merge_staged_commit(alice_provider, *staged)
+                .unwrap();
+        }
+        _ => panic!("Expected StagedCommitMessage"),
+    }
+
+    // Bob2 joins via the Welcome.
+    let bob2_group = StagedWelcome::new_from_welcome(
+        bob2_provider,
+        &join_config,
+        welcome,
+        Some(alice_group.export_ratchet_tree().into()),
+    )
+    .expect("Bob2 should accept the Welcome")
+    .into_group(bob2_provider)
+    .expect("Bob2 should be able to build the group");
+
+    // All three converge to the same epoch and group context.
+    assert_eq!(alice_group.epoch(), bob1_group.epoch());
+    assert_eq!(alice_group.epoch(), bob2_group.epoch());
+    assert_eq!(
+        alice_group.export_group_context(),
+        bob1_group.export_group_context(),
+    );
+    assert_eq!(
+        alice_group.export_group_context(),
+        bob2_group.export_group_context(),
+    );
+
+    // The group has three members: Alice, Bob1, Bob2.
+    assert_eq!(alice_group.members().count(), 3);
+    assert_eq!(bob1_group.members().count(), 3);
+    assert_eq!(bob2_group.members().count(), 3);
+
+    // Sanity: silence unused warnings in case `bob2_signer` is not otherwise
+    // referenced (it is captured by the closure during KeyPackage build above).
+    let _ = bob2_signer;
+}
+
+/// External commit carrying a by-value `AppDataUpdate` proposal. This exercises
+/// the `extensions-draft-08` code path: a non-member joining via external
+/// commit applies an update to the AppDataDictionary atomically with the
+/// join.
+#[cfg(feature = "extensions-draft-08")]
+#[openmls_test]
+fn test_external_commit_with_inline_app_data_update() {
+    use openmls::component::*;
+    use openmls::extensions::*;
+    use openmls::messages::proposals::AppDataUpdateProposal;
+
+    const COMPONENT_ID: ComponentId = 16;
+
+    let alice_provider = &Provider::default();
+    let bob_provider = &Provider::default();
+
+    // Alice configures the group to require the AppDataUpdate proposal type
+    // and the AppDataDictionary extension. Her own leaf must advertise the
+    // matching capabilities.
+    let capabilities = Capabilities::new(
+        None,
+        None,
+        Some(&[ExtensionType::AppDataDictionary]),
+        Some(&[ProposalType::AppDataUpdate]),
+        None,
+    );
+    let required_capabilities =
+        Extension::RequiredCapabilities(RequiredCapabilitiesExtension::new(
+            &[ExtensionType::AppDataDictionary],
+            &[ProposalType::AppDataUpdate],
+            &[],
+        ));
+
+    let group_config = MlsGroupCreateConfig::builder()
+        .ciphersuite(ciphersuite)
+        .capabilities(capabilities.clone())
+        .use_ratchet_tree_extension(true)
+        .with_group_context_extensions(Extensions::single(required_capabilities).unwrap())
+        .build();
+
+    let (alice_credential, alice_signer) =
+        new_credential(alice_provider, b"Alice", ciphersuite.signature_algorithm());
+
+    let mut alice_group = MlsGroup::new(
+        alice_provider,
+        &alice_signer,
+        &group_config,
+        alice_credential.clone(),
+    )
+    .unwrap();
+
+    // Self-update Alice so the exported GroupInfo includes an external_pub.
+    let group_info = alice_group
+        .self_update(alice_provider, &alice_signer, LeafNodeParameters::default())
+        .unwrap()
+        .into_group_info()
+        .expect("self-update should produce a GroupInfo");
+    alice_group.merge_pending_commit(alice_provider).unwrap();
+
+    let verifiable_group_info = {
+        let serialized = group_info.tls_serialize_detached().unwrap();
+        VerifiableGroupInfo::tls_deserialize(&mut serialized.as_slice()).unwrap()
+    };
+
+    // Bob joins via external commit and includes an AppDataUpdate proposal.
+    // His new leaf node must advertise the AppDataUpdate capability.
+    let (bob_credential, bob_signer) =
+        new_credential(bob_provider, b"Bob", ciphersuite.signature_algorithm());
+
+    let bob_leaf_node_parameters = LeafNodeParameters::builder()
+        .with_credential_with_key(bob_credential.clone())
+        .with_capabilities(capabilities)
+        .build();
+
+    let app_data_update = Proposal::AppDataUpdate(Box::new(AppDataUpdateProposal::update(
+        COMPONENT_ID,
+        b"hello-from-external-commit",
+    )));
+
+    let mut bob_commit_stage = MlsGroup::external_commit_builder()
+        .with_config(group_config.join_config().clone())
+        .build_group(bob_provider, verifiable_group_info, bob_credential)
+        .unwrap()
+        .leaf_node_parameters(bob_leaf_node_parameters)
+        .add_proposal(app_data_update)
+        .load_psks(bob_provider.storage())
+        .unwrap();
+
+    // Stage the AppDataUpdate so the dictionary diff is included in the
+    // commit's GroupContext.
+    let mut app_data_updater = bob_commit_stage.app_data_dictionary_updater();
+    for proposal in bob_commit_stage.app_data_update_proposals() {
+        let component_id = proposal.component_id();
+        if let AppDataUpdateOperation::Update(data) = proposal.operation() {
+            app_data_updater
+                .set(ComponentData::from_parts(component_id, data.clone()));
+        }
+    }
+    let changes = app_data_updater.changes();
+    assert!(
+        changes.as_ref().is_some_and(|c| !c.is_empty()),
+        "expected AppDataUpdate to produce dictionary changes"
+    );
+    bob_commit_stage.with_app_data_dictionary_updates(changes);
+
+    let (bob_group, bundle) = bob_commit_stage
+        .build(
+            bob_provider.rand(),
+            bob_provider.crypto(),
+            &bob_signer,
+            |_| true,
+        )
+        .unwrap()
+        .finalize(bob_provider)
+        .unwrap();
+
+    // Alice processes Bob's external commit. She must compute the same
+    // dictionary updates to validate the commit successfully.
+    let commit_msg: MlsMessageIn = bundle.commit().clone().into();
+    let unverified = alice_group
+        .unprotect_message(alice_provider, commit_msg.into_protocol_message().unwrap())
+        .unwrap();
+    let mut alice_updater = alice_group.app_data_dictionary_updater();
+    for proposal in unverified.committed_proposals().unwrap().iter() {
+        let validated = proposal
+            .clone()
+            .validate(alice_provider.crypto(), ciphersuite, ProtocolVersion::Mls10)
+            .unwrap();
+        let proposal = match validated {
+            ProposalOrRef::Proposal(p) => *p,
+            ProposalOrRef::Reference(_) => continue,
+        };
+        if let Proposal::AppDataUpdate(p) = proposal {
+            let component_id = p.component_id();
+            if let AppDataUpdateOperation::Update(data) = p.operation() {
+                alice_updater.set(ComponentData::from_parts(component_id, data.clone()));
+            }
+        }
+    }
+    let processed = alice_group
+        .process_unverified_message_with_app_data_updates(
+            alice_provider,
+            unverified,
+            alice_updater.changes(),
+        )
+        .expect("Alice should accept external commit with AppDataUpdate");
+    match processed.into_content() {
+        ProcessedMessageContent::StagedCommitMessage(staged) => {
+            alice_group
+                .merge_staged_commit(alice_provider, *staged)
+                .unwrap();
+        }
+        _ => panic!("Expected StagedCommitMessage"),
+    }
+
+    // Both parties should now agree on the AppDataDictionary contents.
+    assert_eq!(alice_group.epoch(), bob_group.epoch());
+    assert_eq!(
+        alice_group.extensions().app_data_dictionary(),
+        bob_group.extensions().app_data_dictionary(),
+    );
+    let alice_dict = alice_group
+        .extensions()
+        .app_data_dictionary()
+        .expect("AppDataDictionary must be present");
+    assert_eq!(
+        alice_dict.dictionary().get(&COMPONENT_ID),
+        Some(b"hello-from-external-commit".as_ref()),
+    );
+}