xmtp_mls: ValidatedCommit::from_external_commit — XIP-82 receive-side check set

[tylerhawkes]

Summary

ValidatedCommit::from_external_commit — the full XIP-82 receive-side check set for Sender::NewMemberCommit (external commit) joins, replacing the earlier draft's boolean policy gate:

1. Framing sender is NewMemberCommit (defensive re-check under the dispatch).
2. Exactly one ExternalInit.
3. Every Add binds to the joiner's path-leaf inbox id (anti-smuggling).
4. Already-member rejection — no existing ratchet-tree leaf and no existing GROUP_MEMBERSHIP entry (re-adding yourself would let you re-tag your own entry; resync is deferred, not v1).
5. Exactly one AppDataUpdate: a single Insert of the joiner's own GROUP_MEMBERSHIP entry. Insert-only is not extra restriction — check 4 guarantees no existing entry and TlsMapDelta apply fails Update/Delete on a missing key, so any other shape could never merge.
6. Forbidden proposals: by-reference, Remove/SelfRemove, PSKs (changed from the draft — a non-member has no PSK it can legitimately reference; gratuitous attack surface), GroupContextExtensions, anything else.
7. Typed policy gate: EXTERNAL_COMMIT_POLICY.allow_external_commit == true; absent component / unknown version / decode failure all fail closed.
8. • 9. Envelope-timestamp time bounds — expires_at_ns (absolute) and expire_in_ns (vs epoch-start envelope ts), threaded in as the new ExternalCommitTimestamps param so the verdict never depends on a validator's wall clock. Age saturates at zero to absorb publish-latency skew.
9. GROUP_MEMBERSHIP.external_committer_permissions admits the joiner-self insert (the grant the enable commit establishes atomically in xmtp_mls: EXTERNAL_COMMIT_POLICY component + enable/revoke API with XIP-82 lifecycle invariants #3666).
10. The inserted entry records admitted_via_external_group_id equal to the active external_group_id — on every external commit, whatever max_uses is.
11. max_uses: strictly fewer than the cap of current entries tagged with the active slot, counted from shared pre-commit state so every member — including post-invite joiners — computes the same count.

Plus the two member-path halves the XIP requires to land together with the validator (the deferral documented in #3751):

• Write-once tag enforcement (enforce_admitted_via_write_once, hooked into validate_one_app_data_update_with_old_value): a member-sender commit that sets, clears, or alters admitted_via_external_group_id on any entry — its own included — is rejected, so an invited member can't untag itself and free a max_uses slot. Delete (member removal) remains the one legal way a tag disappears. Fails closed on undecodable/unknown-version entry values: an opaque blob over a tagged V1 entry would hide the tag from every V1 reader's count.
• Tag preservation in build_group_membership_app_data_payload: the Update arm (sequence-id bumps, installation changes) now carries the existing entry's tag through from the AppData dict; fresh Inserts stay untagged. Without this, any member's routine membership rewrite would be convergently rejected by the new write-once check.

Supporting: read_group_membership_entries (decoded per-inbox entries, tag included — the legacy-flattening reader drops it; read_group_membership_from_dict now layers on it), new structured error variants for every check.

Stack

Base: tyler/qr-invite-l6-external-commit-policy (#3666) ← #3671 ← #3751 ← main.

Test plan

• 37 unit tests in external_commit_validator_tests: typed policy gate (absent/disabled/enabled), both time bounds incl. exact-boundary and skew-clamp cases, already-member (tree hit / map hit / clean), max_uses (0 = unlimited, below/at cap, untagged + rotated-slot entries don't count), scope + tag (untagged entry, wrong-slot tag, unknown-version entry, non-Insert mutation, mixed-inbox, empty/malformed delta, Remove op, wrong component), write-once (carry-through, untagged rewrite, clear/set/alter rejected, delete allowed, unknown-version rewrite rejected), PSK-forbidden, plus the pre-existing structure/sender cases.
• Full xmtp_mls suite: 518 passed.
• cargo check --locked, just lint-rust clean.
• End-to-end coverage (a real external commit driven through process_message) rides with L-8 (xmtp_mls: route Sender::NewMemberCommit through from_external_commit + envelope-timestamp plumbing #3668), which owns the dispatch that routes NewMemberCommit here and sources ExternalCommitTimestamps from envelopes.

🤖 Generated with Claude Code

Note

Implement XIP-82 receive-side validation in ValidatedCommit::from_external_commit

• Adds a full XIP-82 validator for NewMemberCommit-sender commits in validated_commit.rs, enforcing policy gate, time bounds (absolute expiry and per-epoch staleness), proposal structure, Add-to-joiner binding, and GROUP_MEMBERSHIP AppData scope.
• Introduces ExternalCommitTimestamps to thread delivery-envelope timestamps into expiry/staleness checks, and adds ~20 new CommitValidationError variants for granular rejection reasons.
• Enforces write-once semantics for admitted_via_external_group_id on member-sent GROUP_MEMBERSHIP updates via enforce_admitted_via_write_once, rejecting any rewrite that sets, clears, or alters the tag.
• Updates build_group_membership_app_data_payload and encode_membership_entry in update_group_membership.rs to preserve existing admitted_via_external_group_id on updates while leaving inserts untagged.
• Extracts read_group_membership_entries into a reusable helper in component_source.rs and refactors read_group_membership_from_dict to use it.

📊 Macroscope summarized 9bfd0b7. 5 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted

🗂️ Filtered Issues

No issues evaluated.

[claude]

Claude finished @tylerhawkes's task —— View job

---

Code Review — 2026-06-10 05:45 UTC

Reviewed the external commit validation implementation. Macroscope flagged two issues; one is a false positive, the other is valid.

---

❌ False Positive: Macroscope's Delete/Update Concern

Macroscope's claim: enforce_app_data_update_scope accepts Delete and Update mutations when only Insert should be allowed.

Reality: The code at validated_commit.rs:3203-3204 uses a slice pattern match that only accepts exactly one Insert mutation:

let [TlsMapMutation::Insert { key, value }] = delta.mutations.as_slice() else {
    return Err(CommitValidationError::ExternalCommitAppDataUpdateOutOfScope);
};

This pattern match rejects any delta containing Delete, Update, multiple mutations, or zero mutations. The implementation is correct.

---

✅ Valid Issue: Missing DM Check

Location: groups/mod.rs:2157 (set_external_commit_policy)

Issue: This method doesn't check if the conversation is a DM before allowing external commit policy changes. Every other metadata update method in the file includes this guard:

if self.metadata().await?.conversation_type == ConversationType::Dm {
    return Err(MetadataPermissionsError::DmGroupMetadataForbidden.into());
}

Examples of protected methods:

• update_group_name (line 1972)
• update_app_data (line 1998)
• update_group_description (line 2128)
• update_permission_policy (line 2317)
• update_group_image_url_square (line 2375)
• update_group_pinned_frame_url (line 2415)
• update_admin_list (line 2713)

Risk: Without this check, external commit policies could be set on DM conversations, potentially allowing third parties to join a two-person DM via external commits.

Recommendation: Add the DM check at the start of set_external_commit_policy (after line 2160) to match the pattern used by all other metadata update methods.

---

Minor Observations

1. Test Coverage: 23 unit tests covering the validation helpers is solid. The tests are well-structured and test each rule independently without materializing full MLS groups. Good engineering.

2. Error Granularity: 11 specific CommitValidationError variants provide excellent debuggability. Each failure mode surfaces a clear reason.

3. Documentation: Inline comments explaining XIP-82 rules and RFC references are helpful for maintenance.

---

Summary: One valid security issue (missing DM check). The Delete/Update concern flagged by Macroscope is incorrect due to misreading the slice pattern match.

[macroscopeapp]

🟠 High groups/mod.rs:2015

set_external_commit_policy allows external commit policies to be set on DM conversations, bypassing the restriction that other metadata update methods enforce. This could enable third parties to join a two-person DM via external commits. Consider adding the DM check that other methods use: if self.metadata().await?.conversation_type == ConversationType::Dm { return Err(MetadataPermissionsError::DmGroupMetadataForbidden.into()); }

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

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

`set_external_commit_policy` allows external commit policies to be set on DM conversations, bypassing the restriction that other metadata update methods enforce. This could enable third parties to join a two-person DM via external commits. Consider adding the DM check that other methods use: `if self.metadata().await?.conversation_type == ConversationType::Dm { return Err(MetadataPermissionsError::DmGroupMetadataForbidden.into()); }`

Evidence trail:
crates/xmtp_mls/src/groups/mod.rs lines 2015-2046 (set_external_commit_policy missing DM check), lines 1987-2004 (update_commit_log_signer with DM check at 1993-1995), lines 2051-2060 (set_allow_external_commit wrapper). git_grep for DmGroupMetadataForbidden shows the check at lines 1901, 1927, 1994, 2086, 2144, 2184, 2482 — all other metadata update paths.

[macroscopeapp]

Approvability

Verdict: Needs human review

4 blocking correctness issues found. This PR introduces substantial new external commit validation logic (XIP-82 receive-side checks) - a significant new capability affecting runtime behavior. Additionally, unresolved review comments identify a HIGH severity security concern (DM conversations can have external commit policies set) and MEDIUM severity validation gaps that warrant human attention.

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

[codecov]

Codecov Report

❌ Patch coverage is 85.18519% with 124 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.29%. Comparing base (ccb136f) to head (9bfd0b7).

Files with missing lines	Patch %	Lines
crates/xmtp_mls/src/groups/validated_commit.rs	84.46%	124 Missing ⚠️

Additional details and impacted files

@@                              Coverage Diff                              @@
##           tyler/qr-invite-l6-external-commit-policy    #3667      +/-   ##
=============================================================================
+ Coverage                                      84.25%   84.29%   +0.03%     
=============================================================================
  Files                                            410      410              
  Lines                                          60747    61577     +830     
=============================================================================
+ Hits                                           51184    51906     +722     
- Misses                                          9563     9671     +108     

☔ View full report in Codecov by Harness.
📢 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/validated_commit.rs:1085

from_external_commit trusts the decoded policy from load_external_commit_policy without running validate_policy_v1, so a malformed enabled policy (e.g., empty external_group_id or missing per-invite fields) is accepted as active. This lets external joins proceed against state that the write-side rejects, and with an empty external_group_id the enforce_max_uses check is skipped entirely, breaking the invite access control.

        let policy = super::external_commit_policy::load_external_commit_policy(openmls_group)?;
-        let policy = enforce_external_commit_policy(policy.as_ref())?;
+        let policy = policy
+            .filter(|p| super::external_commit_policy::validate_policy_v1(p).is_ok())
+            .ok_or(CommitValidationError::ExternalCommitNotAllowed)?;
+        let policy = enforce_external_commit_policy(Some(&policy))?;

🚀 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 1085-1090:

`from_external_commit` trusts the decoded policy from `load_external_commit_policy` without running `validate_policy_v1`, so a malformed enabled policy (e.g., empty `external_group_id` or missing per-invite fields) is accepted as active. This lets external joins proceed against state that the write-side rejects, and with an empty `external_group_id` the `enforce_max_uses` check is skipped entirely, breaking the invite access control.

[macroscopeapp]

🟢 Low groups/validated_commit.rs:1180

from_external_commit documents that the joiner should be surfaced in ValidatedCommit.proposers because "the joiner is the sole authoring party," but line 1180 initializes proposers: Vec::new(). Any consumer that reads or serializes proposers will see an external join as having no proposer at all, breaking attribution and contradicting the struct's contract that it contains the unique proposal authors for the commit.

-            proposers: Vec::new(),
+            proposers: vec![joiner_participant.clone()],

🚀 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 1180:

`from_external_commit` documents that the joiner should be surfaced in `ValidatedCommit.proposers` because "the joiner is the sole authoring party," but line 1180 initializes `proposers: Vec::new()`. Any consumer that reads or serializes `proposers` will see an external join as having no proposer at all, breaking attribution and contradicting the struct's contract that it contains the unique proposal authors for the commit.

[macroscopeapp]

🟡 Medium groups/validated_commit.rs:3258

enforce_admitted_via_write_once returns Ok(()) immediately for AppDataUpdateOperation::Remove, so a member-authored wipe of the GROUP_MEMBERSHIP component bypasses the write-once check entirely. If the registry's delete_policy permits the actor, they can clear all entries and their admitted_via_external_group_id tags in one commit, resetting the max_uses count to zero and freeing invite slots for reuse. Consider rejecting Remove operations on GROUP_MEMBERSHIP when any entry carries a non-empty tag, or documenting if this is an accepted trade-off.

🚀 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 3258:

`enforce_admitted_via_write_once` returns `Ok(())` immediately for `AppDataUpdateOperation::Remove`, so a member-authored wipe of the `GROUP_MEMBERSHIP` component bypasses the write-once check entirely. If the registry's `delete_policy` permits the actor, they can clear all entries and their `admitted_via_external_group_id` tags in one commit, resetting the `max_uses` count to zero and freeing invite slots for reuse. Consider rejecting `Remove` operations on `GROUP_MEMBERSHIP` when any entry carries a non-empty tag, or documenting if this is an accepted trade-off.

[macroscopeapp]

groups/validated_commit.rs:2847

first_unsupported captures PreSharedKey as an unsupported proposal type and downstream validation rejects it with ExternalCommitUnsupportedProposalType(PreSharedKey). However, the PR's stated contract lists PreSharedKey as a legal proposal type for external commits, so valid joins including a PSK will be incorrectly rejected. Consider either removing PreSharedKey from the unsupported classification or updating the PR contract to exclude PSKs.

🚀 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 2847:

`first_unsupported` captures `PreSharedKey` as an unsupported proposal type and downstream validation rejects it with `ExternalCommitUnsupportedProposalType(PreSharedKey)`. However, the PR's stated contract lists `PreSharedKey` as a legal proposal type for external commits, so valid joins including a PSK will be incorrectly rejected. Consider either removing `PreSharedKey` from the unsupported classification or updating the PR contract to exclude PSKs.