xmtp_mls: route Sender::NewMemberCommit through from_external_commit + envelope-timestamp plumbing

[tylerhawkes]

Summary

The ingestion layer for XIP-82 external commits — routing plus the envelope-timestamp plumbing the validator's time bounds require:

• Sender dispatch in validate_and_process_external_message: Sender::Member continues through from_staged_commit; Sender::NewMemberCommit (the atomic external-commit join) routes to ValidatedCommit::from_external_commit (xmtp_mls: ValidatedCommit::from_external_commit — XIP-82 receive-side check set #3667), sourcing metadata capability-aware (migrated groups read from the AppData dict, unmigrated from legacy extensions). Both arms converge at merge_staged_commit_logged. Any other sender is rejected.
• PublicMessage framing accepted at the protocol-message gate (external commits are public messages per RFC 9420 §12.4.3.2); the per-sender policy is enforced by the dispatch above.
• Joiner identity from the update-path leaf in extract_message_sender: the joiner isn't in the pre-commit tree; OpenMLS has already verified the framing signature against the path leaf, so it is the source of truth for inbox id + installation id.
• epoch_entered_at_ns column on groups (nullable BIGINT; migration included): the delivery-service envelope timestamp of the message by which this client entered the current epoch. Written on every successful commit merge — own commits echoed back (process_own_message) and others' commits (process_external_message) — and at welcome-join (covering re-welcomes too, which insert_or_replace_group would otherwise skip). NULL falls back to created_at_ns, the initial epoch.
• Hardening from review: the column setter is monotonic (envelope time only moves forward over a group's life — a backdated write from pathological reordering or corruption would widen the staleness window), and both i64→u64 casts clamp at zero (a negative epoch-start would otherwise wrap and saturate the staleness age to zero, silently disabling check 9).
• ExternalCommitTimestamps sourcing at the dispatch: commit_envelope_ns from the commit's own envelope, epoch_started_at_ns from the column. Checks 8 + 9 thus evaluate against envelope timestamps only — a member that syncs the commit a week later reaches the same verdict as one that processed it immediately. A missing group row degrades toward rejection (fail-closed), never toward acceptance.

Stack

Base: tyler/qr-invite-l7-validator (#3667) ← #3666 ← #3671 ← #3751 ← main.

Test plan

• New xmtp_db round-trip test for the column (NULL fresh → set → overwrite); full xmtp_db suite (209) passes the migration.
• Full xmtp_mls suite: 688 passed (welcome-path mocks extended for the new write).
• cargo check --locked, just lint-rust clean.
• End-to-end (a real QR-invite join driven through ingestion) rides with the integration-test PR (xmtp_mls: integration test for QR-invite end-to-end flow #3677).

🤖 Generated with Claude Code

Note

Route Sender::NewMemberCommit through from_external_commit and persist epoch timestamps

• Adds epoch_entered_at_ns (nullable BIGINT) to the groups table via a new migration, with monotonic-write semantics: updates are only persisted if the new value is greater than the stored value.
• Extends the commit message handler in mls_sync.rs to accept Sender::NewMemberCommit (external commits), routing them through ValidatedCommit::from_external_commit with delivery-service envelope timestamps; unsupported sender types are rejected.
• Records epoch_entered_at_ns after merging any commit (own, external, or welcome), using the envelope timestamp as the value.
• Extends the message dispatcher to accept ProtocolMessage::PublicMessage in addition to PrivateMessage, enabling external commit processing.
• Updates extract_message_sender to extract identity and key from the update-path leaf node for external commits.
• Behavioral Change: the dispatcher now processes PublicMessages; previously these were rejected as unsupported.

^{Macroscope summarized 04fbcca.}

[claude]

Claude finished @tylerhawkes's task —— View job

---

Code Review — June 10, 2026

Macroscope has already identified 4 important issues that need addressing. Beyond those, here are additional findings:

🔴 Critical: Unsupported sender rejection doesn't advance cursor

Location: crates/xmtp_mls/src/groups/mls_sync.rs:1285-1295

The other arm in the sender-dispatch match rejects unsupported sender types with ActorNotMember, but does not call maybe_update_cursor before returning the error. This means:

1. The rejection is correct (good)
2. The cursor is not advanced (bad)
3. The message will be retried indefinitely on subsequent syncs (very bad)

Every other early-return error path in validate_and_process_external_message (lines 1203, 1209, 1220, 1230, 1303, 1329, 1352, 1360, 1381, 1398, 1435) calls maybe_update_cursor first. This path should follow the same pattern.

Impact: A malicious or misconfigured node sending commits with unexpected sender types will cause the group to become stuck, unable to sync past that message.

Suggested fix:

other => {
    tracing::warn!(
        inbox_id = self.context.inbox_id(),
        group_id = %self.group_id,
        ?other,
        "rejecting commit from unsupported sender type"
    );
    self.maybe_update_cursor(&self.context.db(), envelope)?;
    return Err(GroupMessageProcessingError::CommitValidation(
        CommitValidationError::ActorNotMember,
    ));
}

---

🟡 Code Quality: Database write uses monotonic guard but comment suggests unconditional behavior

Location: crates/xmtp_db/src/encrypted_store/group.rs:1231-1251

The implementation of set_group_epoch_entered_at_ns includes a monotonic guard (lines 1245-1248):

.filter(
    dsl::epoch_entered_at_ns
        .is_null()
        .or(dsl::epoch_entered_at_ns.lt(epoch_entered_at_ns)),
)

This is excellent defensive design! However, the call sites' comments don't reflect this:

• mls_sync.rs:979-980: "record when (by envelope time) we entered the new epoch"
• mls_sync.rs:1674-1676: "record when (by envelope time) we entered the new epoch"

These comments suggest unconditional writes, which could mislead future maintainers into expecting different behavior. The Macroscope medium-severity finding about line 1672 is actually already mitigated by the monotonic guard in the database layer, but the comment doesn't make that clear.

Suggested improvement: Update call-site comments to mention the monotonic property:

// XIP-82: record when (by envelope time) we entered the new
// epoch (monotonic: never moves backward). The external-commit
// validator measures the `expire_in_ns` staleness bound from here.
storage
    .db()
    .set_group_epoch_entered_at_ns(&self.group_id, envelope_timestamp_ns)?;

---

✅ Good: The actual timestamp handling is correct

Despite Macroscope's medium-severity concern about line 1672, the implementation correctly handles the welcome-vs-commit timestamp ordering issue:

1. Database layer enforces monotonicity (group.rs:1245-1248)
2. Welcome sets initial value (xmtp_welcome.rs:522)
3. Commits only advance if later (enforced by the filter)

The max(welcome_time, commit_time) behavior Macroscope suggested is effectively what happens, just implemented at the database layer rather than the call site. This is good design.

---

📊 Test Coverage Observation

The 82.35% patch coverage with 12 missing lines is concerning for security-critical path changes. The uncovered lines appear to be in error-handling branches (based on Codecov's report focusing on mls_sync.rs). Consider:

1. Adding negative test cases for the new sender-type dispatch paths
2. Testing the cursor-advancement behavior for rejected external commits
3. Testing timestamp monotonicity edge cases (out-of-order delivery)

---

Summary

Action Items:

1. 🔴 Must fix: Add cursor advancement before rejecting unsupported sender types (mls_sync.rs:1285)
2. 🟠 Address Macroscope findings: 3 high-severity issues require human review (Delete mutation, DM check, cursor handling)
3. 🟡 Nice to have: Update comments to clarify monotonic epoch-time behavior
4. 📝 Consider: Expand test coverage for error paths

The core timestamp-tracking design is sound, but the cursor-handling gap could cause operational issues in production.

---

[macroscopeapp]

🟠 High groups/validated_commit.rs:2692

enforce_app_data_update_scope permits TlsMapMutation::Delete mutations when they target the joiner's own inbox id, but a delete-only delta leaves the joiner in the MLS group with no GROUP_MEMBERSHIP entry — violating the invariant that external joins must register the joiner. Since resync external commits are unsupported (ResyncExternalCommitNotSupported), the joiner has no pre-existing entry to delete or update; only Insert should be allowed.

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

`enforce_app_data_update_scope` permits `TlsMapMutation::Delete` mutations when they target the joiner's own inbox id, but a delete-only delta leaves the joiner in the MLS group with no `GROUP_MEMBERSHIP` entry — violating the invariant that external joins must register the joiner. Since resync external commits are unsupported (`ResyncExternalCommitNotSupported`), the joiner has no pre-existing entry to delete or update; only `Insert` should be allowed.

Evidence trail:
crates/xmtp_mls/src/groups/validated_commit.rs lines 2692-2701 (REVIEWED_COMMIT): Loop permits Delete/Update mutations as long as key matches joiner_id.
crates/xmtp_mls/src/groups/validated_commit.rs lines 948-1032 (REVIEWED_COMMIT): `from_external_commit` has no downstream check that delta contains an Insert.
crates/xmtp_mls/src/groups/validated_commit.rs line 2582 (REVIEWED_COMMIT): ResyncExternalCommitNotSupported confirms joiner has no pre-existing entry.
crates/xmtp_mls/src/groups/validated_commit.rs lines 2641-2642 (REVIEWED_COMMIT): Docstring states intent is only Insert.
crates/xmtp_mls_common/src/tls_map.rs line 386 (REVIEWED_COMMIT): TlsMapMutation enum with Insert/Update/Delete variants.

[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

5 blocking correctness issues found. This PR introduces significant new MLS external commit validation logic with security implications for group membership. Multiple high-severity review comments remain unresolved, including concerns about DM policy bypass and cursor advancement on error paths.

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

[codecov]

Codecov Report

❌ Patch coverage is 84.81013% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.22%. Comparing base (9bfd0b7) to head (04fbcca).

Files with missing lines	Patch %	Lines
crates/xmtp_mls/src/groups/mls_sync.rs	40.00%	12 Missing ⚠️

Additional details and impacted files

@@                       Coverage Diff                        @@
##           tyler/qr-invite-l7-validator    #3668      +/-   ##
================================================================
- Coverage                         84.29%   84.22%   -0.07%     
================================================================
  Files                               410      410              
  Lines                             61577    61655      +78     
================================================================
+ Hits                              51906    51930      +24     
- Misses                             9671     9725      +54     

☔ 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]

🟠 High groups/mls_sync.rs:1203

In the Sender::NewMemberCommit validation arm, maybe_update_cursor is called to advance the stored cursor before the later allow_cursor_increment gate is checked. When process_message is called with allow_cursor_increment=false (streaming path), callers rely on the cursor not moving so the message can be retried during the next sync. Any external commit that hits metadata/permission decode failures (lines 1203, 1209, 1230) will have its cursor advanced anyway, causing the subsequent sync to permanently skip that commit and potentially leave the group stuck behind.

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

In file @crates/xmtp_mls/src/groups/mls_sync.rs around line 1203:

In the `Sender::NewMemberCommit` validation arm, `maybe_update_cursor` is called to advance the stored cursor before the later `allow_cursor_increment` gate is checked. When `process_message` is called with `allow_cursor_increment=false` (streaming path), callers rely on the cursor not moving so the message can be retried during the next sync. Any external commit that hits metadata/permission decode failures (lines 1203, 1209, 1230) will have its cursor advanced anyway, causing the subsequent sync to permanently skip that commit and potentially leave the group stuck behind.

[macroscopeapp]

🟡 Medium groups/mls_sync.rs:1672

set_group_epoch_entered_at_ns at line 1674 unconditionally overwrites the welcome-based epoch start with the commit's envelope timestamp. For newly added members, the welcome timestamp is the true epoch start, but backfilled commits have earlier timestamps. When external-commit validation later computes age_ns = commit_envelope_ns - epoch_started_at_ns, this overwritten value makes valid commits appear older than they are, potentially rejecting them as stale when expire_in_ns is tight. Consider only setting the epoch entered time when the field is unset, or using max(welcome_time, commit_time) to preserve the earliest timestamp.

-                // XIP-82: record when (by envelope time) we entered the
-                // new epoch — the external-commit validator measures the
-                // `expire_in_ns` staleness bound from here.
-                storage
-                    .db()
-                    .set_group_epoch_entered_at_ns(&self.group_id, envelope_timestamp_ns)?;

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

In file @crates/xmtp_mls/src/groups/mls_sync.rs around lines 1672-1674:

`set_group_epoch_entered_at_ns` at line 1674 unconditionally overwrites the welcome-based epoch start with the commit's envelope timestamp. For newly added members, the welcome timestamp is the true epoch start, but backfilled commits have earlier timestamps. When external-commit validation later computes `age_ns = commit_envelope_ns - epoch_started_at_ns`, this overwritten value makes valid commits appear older than they are, potentially rejecting them as stale when `expire_in_ns` is tight. Consider only setting the epoch entered time when the field is unset, or using `max(welcome_time, commit_time)` to preserve the earliest timestamp.

[macroscopeapp]

🟡 Medium encrypted_store/group.rs:109

StoredGroup::epoch_entered_at_ns defaults to NULL for existing rows, but xmtp_welcome.rs creates groups with created_at_ns set to local now_ns() rather than the welcome message's envelope timestamp. This causes pre-migration groups to measure XIP-82 staleness from local insertion time instead of the actual epoch-entry message time, so external commits may be incorrectly accepted or rejected until the group advances epoch.

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

In file @crates/xmtp_db/src/encrypted_store/group.rs around line 109:

`StoredGroup::epoch_entered_at_ns` defaults to `NULL` for existing rows, but `xmtp_welcome.rs` creates groups with `created_at_ns` set to local `now_ns()` rather than the welcome message's envelope timestamp. This causes pre-migration groups to measure XIP-82 staleness from local insertion time instead of the actual epoch-entry message time, so external commits may be incorrectly accepted or rejected until the group advances epoch.