xmtp_mls: MlsGroup::create_external_invite (produce QR-invite payload + encrypted GroupInfo)

[tylerhawkes]

Summary

Adds the invite-creation half of the QR-invite flow.

• MlsGroup::create_external_invite(opts) -> CreateExternalInviteOutput
• Computes group_id_hash = sha256(group_id()), generates fresh symmetric key + nonce, exports a current-epoch GroupInfo (with ratchet tree bundled), wraps it via wrap_group_info_with_nonce, and returns serialized ExternalInvitePayload + EncryptedGroupInfoBlob protos.
• The envelope's epoch and group_state_hash fields are populated here (the caller has the MlsGroup; the pure-codec helpers in L-5 leave them zero).
• Refuses to operate on pre-AppData groups — external-commit semantics depend on the AppData GROUP_MEMBERSHIP component existing.
• Refuses to operate on DM groups (same constraint as set_external_commit_policy).

Stack

• Base: tyler/qr-invite-l9-openmls-bump (deps: bump openmls fork to feature/external-commit-by-value @ 9cdd5e7 #3672).
• Bindings (NAPI) in PR-B1.

Test plan

• Integration coverage via PR-T1.
• cargo check -p xmtp_mls passes.

Note

Add MlsGroup::create_external_invite to produce QR-invite payload and encrypted GroupInfo

• Adds MlsGroup::create_external_invite in external_invite.rs that exports the current epoch GroupInfo, encrypts it via wrap_group_info, and builds an ExternalInvitePayload governed by the group's EXTERNAL_COMMIT_POLICY.
• Adds MlsGroup::set_external_commit_policy and set_allow_external_commit in mod.rs to write policy into group AppData, generating a symmetric key and external group ID when enabling.
• Adds ValidatedCommit::from_external_commit in validated_commit.rs and a corresponding Sender::NewMemberCommit path in mls_sync.rs so external commits authored by non-members are validated against policy and processed.
• Replaces wrap_welcome/unwrap_welcome with builder-style wrap_payload_hpke/wrap_payload_symmetric APIs in payload_encryption.rs, adding WELCOME_HPKE_LABEL and XMTP_EXTERNAL_INVITE_LABEL for domain separation.
• Removes the separate sync_api_client from ClientBuilder, XmtpMlsLocalContext, and all bindings; replaces dual api_clients() with single api_client() and introduces WorkerConfig for per-worker interval, jitter, and enablement control exposed to WASM, Node, iOS, and Android bindings.
• Adds the xmtp_logging crate with a platform-aware XmtpLoggingBuilder, runtime LoggingHandle, OTLP telemetry support, and rolling-file logging for native targets; removes logging from xmtp_common.
• Risk: removing sync_api_client is a breaking API change for all bindings and any callers using api_clients(); WorkerKind::Event is removed from the enum.

^{Macroscope summarized 57e7e30.}

[claude]

Claude finished @tylerhawkes's task —— View job

---

Code Review — 2026-05-27 18:35 UTC

Reviewed the implementation of MlsGroup::create_external_invite for producing QR-invite payloads with encrypted GroupInfo.

Summary

Overall solid implementation with good cryptographic practices and defensive validation. The code follows expected patterns for this codebase and properly delegates to well-tested helper functions. Main concerns are around test coverage gaps and a few edge cases in expiry computation.

---

Code Quality & Best Practices

Positive:

• Clean separation of concerns: orchestration logic in external_invite.rs, crypto primitives delegated to xmtp_mls_common
• Excellent documentation throughout — the module-level comments clearly explain the flow and lifecycle
• Proper use of tracing instrumentation (#[tracing::instrument])
• Good error handling with descriptive error messages
• Constants are well-defined and documented (SYMMETRIC_KEY_LEN, MIN_EXTERNAL_GROUP_ID_LEN)

Minor issues:

• compute_blob_expiry at line 182 uses u64::try_from(now_ns).unwrap_or(0) — this silently converts negative timestamps to 0, which could mask bugs. Consider adding validation or using u64::saturating_from with a comment explaining the edge case.

---

Potential Bugs / Edge Cases

1. Integer conversion safety (external_invite.rs:192):

   let now_u64 = u64::try_from(now_ns).unwrap_or(0);
   candidates.push(now_u64.saturating_add(policy.expire_in_ns));

   If now_ns is negative (shouldn't be, but defensive programming), this becomes 0 and could produce unexpectedly short expiry times. Consider asserting now_ns >= 0 or documenting why negative values are impossible.

2. Expiry edge case (external_invite.rs:200):
   When all expiry sources are 0, this returns 0 (no expiry). This is documented and correct, but worth noting that expired blobs (with expires_at_ns = 0) will be accepted indefinitely. The unwrap logic correctly handles this (see encrypted_group_info.rs:146).

3. DM group check missing:
   The PR description states "Refuses to operate on DM groups (same constraint as set_external_commit_policy)," but I don't see a DM check in create_external_invite. Should there be a check like:

   if self.is_dm()? {
       return Err(GroupError::DmGroupMetadataForbidden);
   }

   This might be enforced implicitly (DM groups shouldn't have the policy set), but an explicit check would be clearer and safer.

4. Pre-AppData group check (external_invite.rs:115-116):
   The code correctly refuses to operate when EXTERNAL_COMMIT_POLICY is absent, which implicitly enforces the "pre-AppData groups" constraint mentioned in the PR description. This is correct but subtle — consider adding a comment explaining this connection.

---

Security Concerns

Strong points:

• Proper use of ChaCha20Poly1305 AEAD via OpenMLS provider
• Fresh nonce generation for each encryption (critical for AEAD security) — correctly delegated to wrap_group_info which calls generate_nonce()
• Symmetric key length validation before use
• External group ID minimum length enforcement
• No key or sensitive data logged (tracing uses skip_all)

Observations:

• The symmetric key is read from EXTERNAL_COMMIT_POLICY (set by admin via earlier PR in the stack). This means all members with the policy visible can create invites, which appears to be intentional per the documentation. Worth confirming this is the desired authorization model.
• group_state_hash uses epoch_authenticator() which is correct per OpenMLS semantics.

---

Performance Considerations

• export_group_info with with_ratchet_tree = true can be expensive for large groups, but this is documented and expected (joiner needs the tree for external commit). No issues here.
• The lock pattern load_mls_group_with_lock_async is appropriate for this operation.
• encode_to_vec() allocations are fine for invite-creation path (infrequent operation).

---

Test Coverage

Codecov reports 59.13% patch coverage with 47 uncovered lines. This is the main concern.

Looking at the tests:

• create_external_invite_fails_when_policy_not_set (lines 216-228) covers the basic error case
• compute_blob_expiry_picks_tightest_active_value (lines 238-273) has good unit coverage for the expiry logic

Missing coverage likely includes:

• Happy path: successful invite creation when policy IS set ⚠️
• Malformed policy scenarios (wrong key length, short external_group_id) ⚠️
• Edge cases around ensure_not_paused() failure
• export_group_info error path
• Interaction between blob_expires_at_ns hint and policy expiry

Recommendation: The PR description notes "Integration coverage via PR-T1" (unchecked). Consider adding at least one happy-path test to this PR that:

1. Calls set_allow_external_commit(true) to enable the policy
2. Calls create_external_invite successfully
3. Validates the returned payload and encrypted blob can be decoded

This would significantly improve coverage and document expected usage.

---

Error Handling Review

The new error variants in error.rs are well-designed:

ExportGroupInfo(#[from] openmls::group::ExportGroupInfoError),          // Line 452
EncryptedGroupInfo(#[from] EncryptedGroupInfoError),                    // Line 460
ExternalCommitNotAllowed,                                               // Line 466
ExternalCommitPolicyMalformed(String),                                  // Line 473

All four are correctly marked as non-retryable in the RetryableError impl (lines 668-671). Good.

---

Recommendations

1. High priority: Add happy-path test coverage showing successful invite creation with a valid policy
2. Medium priority: Add explicit DM group check if intended (or add comment explaining why it's not needed)
3. Low priority: Consider adding validation for negative now_ns in compute_blob_expiry, or document why it's safe
4. Low priority: Add test case for malformed policy scenarios (wrong key/ID lengths)

---

Overall assessment: Well-implemented feature with good cryptographic hygiene. The main gap is test coverage of the happy path, which should be addressed before or in PR-T1.

---

[macroscopeapp]

Approvability

Verdict: Needs human review

Unable to check for correctness in 57e7e30. Diff is too large for automated approval analysis. A human reviewer should evaluate this PR.

^{You can customize Macroscope's approvability policy. Learn more.}

[codecov]

Codecov Report

❌ Patch coverage is 59.13043% with 47 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.82%. Comparing base (b4b3789) to head (e8978a2).

Files with missing lines	Patch %	Lines
crates/xmtp_mls/src/groups/external_invite.rs	59.13%	47 Missing ⚠️

Additional details and impacted files

@@                         Coverage Diff                         @@
##           tyler/qr-invite-l9-openmls-bump    #3673      +/-   ##
===================================================================
- Coverage                            83.96%   83.82%   -0.14%     
===================================================================
  Files                                  401      402       +1     
  Lines                                59586    59701     +115     
===================================================================
+ Hits                                 50031    50046      +15     
- Misses                                9555     9655     +100     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[macroscopeapp]

🟡 Medium groups/mod.rs:2139

set_external_commit_policy and set_allow_external_commit do not check ConversationType::Dm before queuing the intent, unlike every other metadata-mutating API in this file (update_group_name, update_app_data, update_group_description, update_group_image_url_square, update_permission_policy, update_admin_list). A migrated DM can therefore have EXTERNAL_COMMIT_POLICY persisted, enabling QR/external-commit state on a conversation type the codebase explicitly forbids elsewhere (e.g., create_external_invite documents DM prohibition). Consider adding the standard DM guard to reject the operation early.

Also found in 1 other location(s)

crates/xmtp_mls/src/groups/external_invite.rs:107

create_external_invite never checks the group's conversation_type before exporting a QR invite. The PR explicitly requires DM groups to be rejected, but this implementation only checks pause state and policy presence. If a DM group carries an EXTERNAL_COMMIT_POLICY entry (for example from malformed or legacy state), this method will still mint an external invite for that DM, violating the intended restriction.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file crates/xmtp_mls/src/groups/mod.rs around line 2139:

`set_external_commit_policy` and `set_allow_external_commit` do not check `ConversationType::Dm` before queuing the intent, unlike every other metadata-mutating API in this file (`update_group_name`, `update_app_data`, `update_group_description`, `update_group_image_url_square`, `update_permission_policy`, `update_admin_list`). A migrated DM can therefore have `EXTERNAL_COMMIT_POLICY` persisted, enabling QR/external-commit state on a conversation type the codebase explicitly forbids elsewhere (e.g., `create_external_invite` documents DM prohibition). Consider adding the standard DM guard to reject the operation early.

Also found in 1 other location(s):
- crates/xmtp_mls/src/groups/external_invite.rs:107 -- `create_external_invite` never checks the group's `conversation_type` before exporting a QR invite. The PR explicitly requires DM groups to be rejected, but this implementation only checks pause state and policy presence. If a DM group carries an `EXTERNAL_COMMIT_POLICY` entry (for example from malformed or legacy state), this method will still mint an external invite for that DM, violating the intended restriction.

[macroscopeapp]

🟡 Medium groups/validated_commit.rs:2775

enforce_app_data_update_scope accepts TlsMapMutation::Update and TlsMapMutation::Delete mutations for the joiner's inbox id, so an external commit can pass validation while deleting or mutating the joiner's GROUP_MEMBERSHIP entry instead of inserting it. This violates the function's documented contract that "the joiner is authorized to insert their own entry — but ONLY their own" and leaves the tree membership and AppData membership out of sync. Consider rejecting Update and Delete mutations to ensure the joiner can only insert their own entry.

-    for mutation in &delta.mutations {
-        let key = match mutation {
-            TlsMapMutation::Insert { key, .. }
-            | TlsMapMutation::Update { key, .. }
-            | TlsMapMutation::Delete { key } => key,
-        };
-        if key != &joiner_id {
-            return Err(CommitValidationError::ExternalCommitAppDataUpdateOutOfScope);
-        }
+    for mutation in &delta.mutations {
+        let key = match mutation {
+            TlsMapMutation::Insert { key, .. } => key,
+            _ => {
+                return Err(CommitValidationError::ExternalCommitAppDataUpdateOutOfScope);
+            }
+        };
+        if key != &joiner_id {
+            return Err(CommitValidationError::ExternalCommitAppDataUpdateOutOfScope);
+        }

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file crates/xmtp_mls/src/groups/validated_commit.rs around lines 2775-2784:

`enforce_app_data_update_scope` accepts `TlsMapMutation::Update` and `TlsMapMutation::Delete` mutations for the joiner's inbox id, so an external commit can pass validation while deleting or mutating the joiner's `GROUP_MEMBERSHIP` entry instead of inserting it. This violates the function's documented contract that "the joiner is authorized to insert their own entry — but ONLY their own" and leaves the tree membership and AppData membership out of sync. Consider rejecting `Update` and `Delete` mutations to ensure the joiner can only insert their own entry.

[macroscopeapp]

🟡 Medium groups/mod.rs:2171

set_allow_external_commit(true) unconditionally generates new symmetric_key and external_group_id values via generate_symmetric_key() and generate_external_group_id(). When external commits are already enabled, this call rotates the invite material and invalidates previously issued QR codes, deep links, and uploaded blobs indexed by the old external_group_id. Consider reading the existing policy first and re-using its key/id when allow_external_commit is already true to preserve stability across calls.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file crates/xmtp_mls/src/groups/mod.rs around line 2171:

`set_allow_external_commit(true)` unconditionally generates new `symmetric_key` and `external_group_id` values via `generate_symmetric_key()` and `generate_external_group_id()`. When external commits are already enabled, this call rotates the invite material and invalidates previously issued QR codes, deep links, and uploaded blobs indexed by the old `external_group_id`. Consider reading the existing policy first and re-using its key/id when `allow_external_commit` is already `true` to preserve stability across calls.

[macroscopeapp]

🟠 High invite/encrypted_group_info.rs:92

wrap_group_info_with_nonce encrypts plaintext with AEAD but writes epoch, group_state_hash, and expires_at_ns into EncryptedGroupInfoBlobV1 in cleartext without including them as AEAD associated data. An attacker can tamper with expires_at_ns (e.g., changing an expired timestamp to 0 or a far-future value) and unwrap_group_info will accept the modified blob because the AEAD tag only authenticates ciphertext. This allows expired QR invites to remain valid and breaks integrity of the envelope metadata. Consider including these fields as AEAD associated data so tampering is detected during decryption.

Also found in 1 other location(s)

crates/xmtp_mls_common/src/mls_ext/payload_encryption.rs:139

unwrap_payload_hpke silently accepts a missing secondary ciphertext at line 139 by returning vec![] whenever wrapped_secondary_payload is empty. Because the presence of the second ciphertext is not authenticated anywhere in wrapped_payload, an attacker can strip wrapped_secondary_payload in transit and the receiver will still accept the first plaintext as valid while seeing an empty secondary payload instead of a tamper error. Any protocol field carried in the secondary blob loses integrity protection for its presence.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file crates/xmtp_mls_common/src/invite/encrypted_group_info.rs around line 92:

`wrap_group_info_with_nonce` encrypts `plaintext` with AEAD but writes `epoch`, `group_state_hash`, and `expires_at_ns` into `EncryptedGroupInfoBlobV1` in cleartext without including them as AEAD associated data. An attacker can tamper with `expires_at_ns` (e.g., changing an expired timestamp to `0` or a far-future value) and `unwrap_group_info` will accept the modified blob because the AEAD tag only authenticates `ciphertext`. This allows expired QR invites to remain valid and breaks integrity of the envelope metadata. Consider including these fields as AEAD associated data so tampering is detected during decryption.

Also found in 1 other location(s):
- crates/xmtp_mls_common/src/mls_ext/payload_encryption.rs:139 -- `unwrap_payload_hpke` silently accepts a missing secondary ciphertext at line `139` by returning `vec![]` whenever `wrapped_secondary_payload` is empty. Because the presence of the second ciphertext is not authenticated anywhere in `wrapped_payload`, an attacker can strip `wrapped_secondary_payload` in transit and the receiver will still accept the first plaintext as valid while seeing an empty secondary payload instead of a tamper error. Any protocol field carried in the secondary blob loses integrity protection for its presence.

[macroscopeapp]

🔴 Critical groups/validated_commit.rs:986

from_external_commit extracts the joiner's inbox id from the credential (line 986) and checks that every Add proposal's credential string matches it (line 1000), but it never verifies that the joiner's installation key (or any added installation key) is actually registered in the association state for that inbox id. In contrast, from_staged_commit performs this check (lines 760-778) by looking up each participant's installation_id in the AssociationState at the relevant sequence_id. Without this lookup, a crafted external commit can claim any inbox id and join (or add installations) with signature keys that have no legitimate association to that inbox, bypassing XMTP's installation↔inbox binding. Consider adding the same association-state verification that from_staged_commit performs — this will require threading in a context (or equivalent identity-lookup capability) and making the function async.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file crates/xmtp_mls/src/groups/validated_commit.rs around line 986:

`from_external_commit` extracts the joiner's inbox id from the credential (line 986) and checks that every `Add` proposal's credential string matches it (line 1000), but it never verifies that the joiner's installation key (or any added installation key) is actually registered in the association state for that inbox id. In contrast, `from_staged_commit` performs this check (lines 760-778) by looking up each participant's `installation_id` in the `AssociationState` at the relevant `sequence_id`. Without this lookup, a crafted external commit can claim any inbox id and join (or add installations) with signature keys that have no legitimate association to that inbox, bypassing XMTP's installation↔inbox binding. Consider adding the same association-state verification that `from_staged_commit` performs — this will require threading in a `context` (or equivalent identity-lookup capability) and making the function async.

[macroscopeapp]

🟢 Low groups/external_invite.rs:149

create_external_invite computes blob_expires_at_ns using xmtp_common::time::now_ns() before returning to the caller, but the caller uploads encrypted_group_info only after this method returns. When policy.expire_in_ns is used, the relative expiry is anchored to invite creation time instead of upload time, so any delay between creation and upload can make a freshly uploaded blob already stale or expired.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file crates/xmtp_mls/src/groups/external_invite.rs around line 149:

`create_external_invite` computes `blob_expires_at_ns` using `xmtp_common::time::now_ns()` before returning to the caller, but the caller uploads `encrypted_group_info` only after this method returns. When `policy.expire_in_ns` is used, the relative expiry is anchored to invite creation time instead of upload time, so any delay between creation and upload can make a freshly uploaded blob already stale or expired.