xmtp_mls: MlsGroup::create_external_invite (produce QR-invite payload + encrypted GroupInfo)#3673
Conversation
|
Claude finished @tylerhawkes's task —— View job PR Review - June 10, 2026SummaryThis PR introduces the producer-side API for QR-invite flows ( Critical IssueMissing DM conversation type check in The function only checks if self.metadata().await?.conversation_type == ConversationType::Dm {
return Err(MetadataPermissionsError::DmGroupMetadataForbidden.into());
}If a DM somehow has Fix: Add the standard DM guard after pub async fn create_external_invite(
&self,
opts: CreateExternalInviteOpts,
) -> Result<CreateExternalInviteOutput, GroupError> {
self.ensure_not_paused().await?;
// Add this check
if self.metadata().await?.conversation_type == ConversationType::Dm {
return Err(MetadataPermissionsError::DmGroupMetadataForbidden.into());
}
let signer = self.context.identity().installation_keys.clone();
// ... rest of implementation
}Similarly, Macroscope Findings - Analysis1. AEAD metadata tampering - ✅ Already fixed The Macroscope finding about 2. Race condition: epoch_entered_at_ns - ✅ Not an issue The read happens inside the 3. The code at line 3203 uses pattern matching 4. Generating fresh keys every time is intentional per the docstring (line 2210-2212): "Uniform randomness is what guarantees a re-enable never revives a revoked key". This is the documented behavior. If key stability across repeated calls is needed, that's a feature request, not a bug. 5. Identity verification bypass - ✅ False positive
Minor Observations
RecommendationRequired: Add DM conversation type check to |
ApprovabilityVerdict: Needs human review 7 blocking correctness issues found. This PR introduces new external invite functionality with unresolved Critical and High severity review comments, including a potential security bypass in external commit validation where joiner installation keys aren't verified against association state. Human review is needed to address these security concerns. You can customize Macroscope's approvability policy. Learn more. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## tyler/qr-invite-l9-openmls-bump #3673 +/- ##
===================================================================
- Coverage 84.29% 84.27% -0.02%
===================================================================
Files 410 411 +1
Lines 61655 61751 +96
===================================================================
+ Hits 51970 52041 +71
- Misses 9685 9710 +25 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
tylerhawkes
force-pushed
the
tyler/qr-invite-l9-openmls-bump
branch
from
8f3e6d6 to
9ba7051
Compare
tylerhawkes
force-pushed
the
tyler/qr-invite-l10-create-invite
branch
from
ffaaf90 to
c12079b
Compare
tylerhawkes
force-pushed
the
tyler/qr-invite-l9-openmls-bump
branch
from
9ba7051 to
e861d43
Compare
tylerhawkes
force-pushed
the
tyler/qr-invite-l10-create-invite
branch
from
c12079b to
096e94d
Compare
tylerhawkes
force-pushed
the
tyler/qr-invite-l9-openmls-bump
branch
from
e861d43 to
6b218e5
Compare
tylerhawkes
force-pushed
the
tyler/qr-invite-l10-create-invite
branch
2 times, most recently
from
5ebf785 to
e8978a2
Compare
tylerhawkes
force-pushed
the
tyler/qr-invite-l9-openmls-bump
branch
from
6b218e5 to
b4b3789
Compare
tylerhawkes
force-pushed
the
tyler/qr-invite-l10-create-invite
branch
from
e8978a2 to
e8ceed3
Compare
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| &self, | ||
| policy: xmtp_proto::xmtp::mls::message_contents::ExternalCommitPolicyV1, | ||
| ) -> Result<(), GroupError> { | ||
| self.ensure_not_paused().await?; |
There was a problem hiding this comment.
🟡 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_invitenever checks the group'sconversation_typebefore 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 anEXTERNAL_COMMIT_POLICYentry (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.
| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 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.
| /// Sugar wrapper over [`MlsGroup::set_external_commit_policy`] that | ||
| /// only flips the master switch and leaves the time-window controls | ||
| /// at their defaults (no automatic expiry / no staleness bound). | ||
| pub async fn set_allow_external_commit(&self, allowed: bool) -> Result<(), GroupError> { |
There was a problem hiding this comment.
🟡 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.
| let joiner_leaf = staged_commit | ||
| .update_path_leaf_node() | ||
| .ok_or(CommitValidationError::ExternalCommitMissingPathLeaf)?; | ||
| let joiner_inbox_id = inbox_id_from_credential(joiner_leaf.credential())?; |
There was a problem hiding this comment.
🔴 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.
e8ceed3 to
57e7e30
Compare
tylerhawkes
force-pushed
the
tyler/qr-invite-l9-openmls-bump
branch
from
b4b3789 to
6849a56
Compare
tylerhawkes
force-pushed
the
tyler/qr-invite-l10-create-invite
branch
from
57e7e30 to
88884e2
Compare
| self.ensure_not_paused().await?; | ||
|
|
||
| let signer = self.context.identity().installation_keys.clone(); | ||
|
|
||
| // Epoch start (envelope time) for the effective-expiry fold — | ||
| // the same value the validator's check 9 measures from. NULL | ||
| // (no epoch advance since the column landed) falls back to | ||
| // group creation, the initial epoch. | ||
| let epoch_started_at_ns = self | ||
| .context | ||
| .db() | ||
| .find_group(&self.group_id)? | ||
| .map(|group| group.epoch_entered_at_ns.unwrap_or(group.created_at_ns)) | ||
| .unwrap_or(0) | ||
| .max(0) as u64; |
There was a problem hiding this comment.
🟠 High groups/external_invite.rs:116
create_external_invite allows generating a QR invite for ConversationType::Dm groups because it only checks ensure_not_paused(). If a DM somehow has an EXTERNAL_COMMIT_POLICY in its AppData, this produces a shareable invite that turns a 2-party direct message into an externally joinable group. Consider adding a ConversationType check to reject DMs, matching the validation in other metadata/policy mutators.
pub async fn create_external_invite(
&self,
opts: CreateExternalInviteOpts,
) -> Result<CreateExternalInviteOutput, GroupError> {
self.ensure_not_paused().await?;
+ self.require_group_type(GroupMetadataOptions::DEFAULT_GROUP_TYPE)?;
let signer = self.context.identity().installation_keys.clone();🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/xmtp_mls/src/groups/external_invite.rs around lines 116-130:
`create_external_invite` allows generating a QR invite for `ConversationType::Dm` groups because it only checks `ensure_not_paused()`. If a DM somehow has an `EXTERNAL_COMMIT_POLICY` in its AppData, this produces a shareable invite that turns a 2-party direct message into an externally joinable group. Consider adding a `ConversationType` check to reject DMs, matching the validation in other metadata/policy mutators.
… + encrypted GroupInfo) Producer side of the XIP-82 invite flow: reads the typed EXTERNAL_COMMIT_POLICY (validate_policy_v1 — same invariants validators enforced at write time), exports the current-epoch GroupInfo with the ratchet tree, wraps it under the in-group key with a fresh CSPRNG nonce (epoch/group_state_hash/expires_at_ns authenticated as AAD), and builds the typed ExternalInvitePayload (optional ServicePointer; absent = application-resolved). The blob expiry is the spec-pinned effective fold: tightest of the policy absolute expires_at_ns and epoch_start + expire_in_ns (epoch start from the groups.epoch_entered_at_ns column the validator also measures from), optionally tightened per call.
tylerhawkes
force-pushed
the
tyler/qr-invite-l10-create-invite
branch
from
88884e2 to
47d1f52
Compare
| // Enabled ⇒ 32-byte key + ≥4-byte slot id + well-formed | ||
| // refresh pointers (the same invariants every validator | ||
| // enforced when this policy was written). | ||
| validate_policy_v1(&policy)?; |
There was a problem hiding this comment.
🟡 Medium groups/external_invite.rs:146
create_external_invite only validates the policy field shape via validate_policy_v1, but doesn't verify the caller holds GROUP_MEMBERSHIP.external_committer_permissions. When the policy enables external commits but the membership grant is missing, the method emits an invite that will deterministically fail at join time with ExternalCommitInsertNotAdmitted. Consider adding the membership permission check before creating the invite.
🚀 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 146:
`create_external_invite` only validates the policy field shape via `validate_policy_v1`, but doesn't verify the caller holds `GROUP_MEMBERSHIP.external_committer_permissions`. When the policy enables external commits but the membership grant is missing, the method emits an invite that will deterministically fail at join time with `ExternalCommitInsertNotAdmitted`. Consider adding the membership permission check before creating the invite.
…ves) + adapt invite::payload Regenerates xmtp_proto from xmtp/proto#336 (typed SymmetricKey / GroupStateHash / ServicePointer newtypes, ExternalCommitPolicyV1 max_uses + refresh_pointers, EncryptedGroupInfoBlobV1 AAD + effective-expiry semantics, GroupMembershipEntry.V1 admitted_via_external_group_id). Consuming changes kept to what main already contains: - invite::payload rewritten for the typed shape: symmetric_key is a SymmetricKey submessage (MissingSymmetricKey + material-length checks), service_pointer is an optional ServicePointer (absent = application-resolved; present-but-empty oneof fails closed; https_url parsed and scheme-checked via url). New https_service_pointer / opaque_service_pointer / validate_service_pointer helpers. - GroupMembershipEntry.V1 construction sites gain the new admitted_via_external_group_id field, empty everywhere: the migrator's synthesized entries are all Welcome/legacy members (absent is permanently correct), and the membership-update rewrite path documents that tag preservation MUST land together with the validator's write-once enforcement in the external-commit stack — nothing on main can set the tag yet, so empty is correct today. The rest of the QR-invite stack (#3671 -> #3666 -> #3667 -> #3668 -> #3673 -> #3674) rebases onto this and picks up the new fields layer by layer.
…ves) + adapt invite::payload Regenerates xmtp_proto from xmtp/proto#336 (typed SymmetricKey / GroupStateHash / ServicePointer newtypes, ExternalCommitPolicyV1 max_uses + refresh_pointers, EncryptedGroupInfoBlobV1 AAD + effective-expiry semantics, GroupMembershipEntry.V1 admitted_via_external_group_id). Consuming changes kept to what main already contains: - invite::payload rewritten for the typed shape: symmetric_key is a SymmetricKey submessage (MissingSymmetricKey + material-length checks), service_pointer is an optional ServicePointer (absent = application-resolved; present-but-empty oneof fails closed; https_url parsed and scheme-checked via url). New https_service_pointer / opaque_service_pointer / validate_service_pointer helpers. - GroupMembershipEntry.V1 construction sites gain the new admitted_via_external_group_id field, empty everywhere: the migrator's synthesized entries are all Welcome/legacy members (absent is permanently correct), and the membership-update rewrite path documents that tag preservation MUST land together with the validator's write-once enforcement in the external-commit stack — nothing on main can set the tag yet, so empty is correct today. The rest of the QR-invite stack (#3671 -> #3666 -> #3667 -> #3668 -> #3673 -> #3674) rebases onto this and picks up the new fields layer by layer.
1 participant
Summary
MlsGroup::create_external_invite— the producer side of the XIP-82 QR-invite flow, updated to the revised spec:EXTERNAL_COMMIT_POLICYand re-checks it withvalidate_policy_v1— the same field-coupling invariants every validator enforced when the policy was written (xmtp_mls: EXTERNAL_COMMIT_POLICY component + enable/revoke API with XIP-82 lifecycle invariants #3666). Absent/disabled →ExternalCommitNotAllowed; malformed → the structuredExternalCommitPolicyerror (the ad-hocExternalCommitPolicyMalformed(String)variant is gone).GroupInfowith the ratchet tree embedded and wraps it under the in-group key with a fresh CSPRNG nonce per wrap (re-wraps under one key must never share a nonce);epoch,group_state_hash, andexpires_at_nsride as AAD (xmtp_mls_common: add invite::encrypted_group_info + XMTP_EXTERNAL_INVITE_LABEL #3671), so the joiner validates-and-selects — the service never arbitrates.expires_at_nsis the spec-pinned effective expiry: tightest of the policy's absoluteexpires_at_nsandepoch_start + expire_in_ns— epoch start read from thegroups.epoch_entered_at_nscolumn (xmtp_mls: route Sender::NewMemberCommit through from_external_commit + envelope-timestamp plumbing #3668), the same value validators measure check 9 from, not upload time. Folding the staleness bound in at wrap time means the joiner's single expiry check also discards candidates validators would reject (avoiding zombie joins). A per-call hint can only tighten.ExternalInvitePayload: optionalServicePointer(https_url|opaque);None= application-resolved, saving QR space.Stack
Base:
tyler/qr-invite-l9-openmls-bump(#3672) ← #3668 ← #3667 ← #3666 ← #3671 ← #3751 ← main.Test plan
tighten_expiry(hint tighter/looser/zero/bounds-unbounded), the effective-expiry fold measuring from epoch start (mirrors validator check 9).xmtp_mlssuite: 691 passed;just lint-rustclean.enable_external_commits → create_external_invite → join) needs the full intent/commit pipeline and rides in the integration test (xmtp_mls: integration test for QR-invite end-to-end flow #3677).🤖 Generated with Claude Code
Note
Add
MlsGroup::create_external_inviteto produce QR-invite payload and encrypted GroupInfoexternal_invite.rswithcreate_external_invite, an async method that exports a GroupInfo for the current MLS epoch, wraps it in anEncryptedGroupInfoblob, and returns serializedExternalInvitePayloadand encrypted blob bytes.EXTERNAL_COMMIT_POLICYbefore producing any artifacts — rejects withGroupError::ExternalCommitNotAllowedif policy is absent or forbids external commits.CreateExternalInviteOpts::blob_expires_at_ns.GroupErrorwith four new non-retryable variants:ExportGroupInfo,EncryptedGroupInfo,ExternalInvitePayloadBuild, andExternalCommitNotAllowed.Macroscope summarized 47d1f52.