add message deletion

[mchenani]

Add message deletion across core, DB, and bindings to represent, validate, persist, and surface DeletedMessage content

Introduce ContentType::DeleteMessage, a DeletedBy model, and deletion records; validate deletions in enrichment, persist via message_deletions, and expose deleted state in Node, WASM, and FFI bindings. Add group APIs to send and process delete messages and filter deletion entries from lists.

📍Where to Start

Start with deletion handling in MlsGroup: review process_delete_message in xmtp_mls/src/groups/mls_sync.rs, then delete_message in xmtp_mls/src/groups/mod.rs, followed by enrichment logic in xmtp_mls/src/messages/enrichment.rs.

Changes since #2772 opened

• Refactored From<ContentType> implementation for ContentTypeSave to explicitly map multiple variants to Unknown [534d631]
• Modified process_delete_message handler in groups.mls_sync to support out-of-order deletions where the target message has not yet arrived [f36cbc3]
• Updated deletion validation and enrichment logic in messages.enrichment to determine DeletedBy based on sender identity rather than stored flags [f36cbc3]
• Added three test cases in groups.tests.test_delete_message to validate out-of-order deletion behavior [f36cbc3]
• Simplified Group.delete_message method in groups.Group by streamlining comments and encoding flow [f36cbc3]

---

📊 Macroscope summarized a4ce98f. 16 files reviewed, 10 issues evaluated, 10 issues filtered, 0 comments posted

🗂️ Filtered Issues

db_tools/src/tasks/migrations.rs — 0 comments posted, 2 evaluated, 2 filtered

• line 73: The migration_status function assumes that values in the applied slice are already purely numeric versions, but this assumption may be incorrect. The old code filtered both name and each applied version a to numeric characters before comparing. The new code only filters name via extract_version() and compares directly with a == &name_version. If db.applied_migrations() returns versions containing non-numeric characters (e.g., dashes like 2025-11-15-232503), the comparison will always fail and migrations will incorrectly be reported as [pending] when they are actually applied. [ Low confidence ]
• line 184: The migration_numeric_version function silently returns 0 via unwrap_or(0) when parsing fails. If a migration name doesn't parse to a valid u64 (e.g., version string is too long or malformed), multiple migrations could get the same sort key of 0, resulting in incorrect sorting order and potentially causing test failures or unexpected rollback behavior in test_migration_status_applied_and_pending. [ Low confidence ]

xmtp_db/src/encrypted_store/group_message.rs — 0 comments posted, 1 evaluated, 1 filtered

• line 270: The Deletable implementation marks ContentType::Unknown as deletable (true), which could allow deletion of messages with unrecognized content types. If a newer protocol version introduces a critical system message type that an older client doesn't recognize, that client would classify it as Unknown and permit its deletion, potentially causing data integrity issues. [ Low confidence ]

xmtp_db/src/encrypted_store/group_message/convert.rs — 0 comments posted, 1 evaluated, 1 filtered

• line 154: In the From<ContentType> for ContentTypeSave implementation, ContentType::DeleteMessage is mapped to ContentTypeSave::Unknown. When a message with ContentType::DeleteMessage is backed up and later restored via TryFrom<ContentTypeSave> for ContentType, it will be restored as ContentType::Unknown instead of ContentType::DeleteMessage, causing permanent data loss of the delete message semantics during backup/restore cycles. [ Already posted ]

xmtp_mls/src/groups/mls_sync.rs — 0 comments posted, 2 evaluated, 2 filtered

• line 1370: In the out-of-order deletion case (lines 1365-1371), when the original message doesn't exist yet, is_authorized is unconditionally set to true. This allows ANY user to delete ANY message by timing their deletion request to arrive before the target message. The code checks if the deleter is a super admin but doesn't use that check to gate authorization - it only uses it to set is_super_admin_deletion for metadata. This is an authorization bypass vulnerability. [ Already posted ]
• line 1373: The authorization check at line 1373 (if !is_authorized) is dead code that can never execute. In the Some branch, unauthorized requests return early at line 1361. In the None branch, is_authorized is always set to true. This suggests the out-of-order case authorization logic is incomplete - it should likely restrict deletions to only super admins when the original message is unavailable. [ Already posted ]

xmtp_mls/src/groups/mod.rs — 0 comments posted, 3 evaluated, 3 filtered

• line 704: Potential TOCTOU (time-of-check-time-of-use) race condition: the is_message_deleted check at line 704 and the is_super_admin check at line 710 could become stale if another thread/process deletes the same message or revokes super admin status before deletion.store(&conn) completes at line 741, potentially allowing duplicate deletions or unauthorized deletions in a narrow time window. [ Previously rejected ]
• line 741: State inconsistency if deletion.store(&conn) fails after send_message_optimistic succeeds. At line 728, the deletion message is sent to the network, but if line 741 fails (e.g., database error), the local message_deletions table won't have a record. Subsequent calls to delete_message for the same message will pass the is_message_deleted check at line 704 and could send duplicate deletion messages to the network. [ Previously rejected ]
• line 864: The new find_enriched_messages method does not call filter_out_hidden_message_types_from_query on the args before calling get_group_messages, unlike the existing find_messages_v2_with_conn method which filters the query. This inconsistency could cause hidden/internal message types to be returned to callers when they should be filtered out. [ Already posted ]

xmtp_mls/src/messages/enrichment.rs — 0 comments posted, 1 evaluated, 1 filtered

• line 248: In get_deletions, if the database returns multiple StoredMessageDeletion records for the same deleted_message_id (e.g., if a message was deleted by multiple parties or if there are duplicate records), only the last one survives the HashMap collection. This could lead to non-deterministic behavior depending on database iteration order, potentially losing information about who deleted the message or their admin status. [ Previously rejected ]

[graphite-app]

How to use the Graphite Merge Queue

Add the label mergequeue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

[claude]

Claude finished @mchenani's task —— View job

---

Code Review — January 6, 2026, 17:02 UTC

Reviewed deletion handling across core, DB, and bindings. Here are unique findings not covered in existing comments:

Critical Issues

1. Inconsistent deletion validation logic between send and receive paths (xmtp_mls/src/groups/mod.rs:716, xmtp_mls/src/groups/mls_sync.rs:1336)

When sending a deletion, both kind and content_type must be deletable (line 716). However, when processing received deletions in out-of-order scenarios (line 1336-1344), these checks are skipped entirely because original_msg_opt is None. This creates an attack surface:

• A malicious actor could send a deletion for a non-deletable message ID (e.g., GroupMembershipChange) before that message exists
• The deletion would be stored with is_authorized = true (line 1370)
• When the actual non-deletable message arrives later, the existing deletion record might incorrectly apply

Recommendation: In the out-of-order case, either (a) reject all deletions when the target message is missing, or (b) defer validation and re-check deletability when the target message eventually arrives.

---

2. Missing transaction boundary in delete_message (xmtp_mls/src/groups/mod.rs:704-741)

The sequence is:

1. Check if already deleted (line 704)
2. Send deletion message to network (line 728)
3. Store deletion record locally (line 741)

If step 3 fails (DB error), the local state becomes inconsistent:

• Network has the deletion message
• Local DB has no deletion record
• Future calls to delete_message will pass the "already deleted" check and attempt to send duplicate deletions

Recommendation: Either use a database transaction encompassing both the store and a send callback, or pre-generate the deletion message ID deterministically so failures can be detected and retried.

---

Design Concerns

3. ContentType::Unknown is deletable (xmtp_db/src/encrypted_store/group_message.rs:270)

The code marks ContentType::Unknown as deletable. If a client receives a message with a content type it doesn't recognize (e.g., a critical system message from a newer protocol version), it will classify it as Unknown and allow deletion. This could enable version-downgrade attacks where older clients delete messages they shouldn't.

Recommendation: Consider making Unknown non-deletable as a safe default, or add explicit allowlists for deletable unknown types.

---

4. Deletion ID collision in get_deletions (xmtp_mls/src/messages/enrichment.rs:245-250)

The function uses HashMap collection with deleted_message_id as key:

deletions.into_iter()
    .map(|deletion| (deletion.deleted_message_id.clone(), deletion))
    .collect()

If multiple deletions exist for the same message (e.g., sender deletes, then admin also deletes), only the last one survives. The iteration order from the DB is not guaranteed, so this is non-deterministic. While the current schema doesn't explicitly prevent multiple deletion records, the behavior is inconsistent.

Recommendation: Either enforce uniqueness in the schema (unique constraint on deleted_message_id), or handle multiple deletions explicitly (e.g., prioritize super admin deletions).

---

Test Coverage Gaps

5. Missing validation tests (87.25% coverage, 97 lines missing)

Based on the codecov report, several critical paths lack coverage:

• bindings_node/src/content_types/deleted_message.rs: 0% coverage (17 lines)
• bindings_ffi/src/message.rs deletion code: 0% coverage (13 lines)
• Error path in is_deletion_valid when group_id mismatches (xmtp_mls/src/messages/enrichment.rs:49)

Recommendation: Add tests for:

• Bindings correctly serialize/deserialize DeletedBy::Admin with inbox_id
• Cross-group deletion attempts are rejected during enrichment
• Out-of-order deletions for non-existent messages (already added in f36cbc3 but ensure coverage)

---

Minor Issues

6. Hex encoding inconsistency (xmtp_mls/src/groups/mod.rs:721)

The wire format uses hex-encoded string (DeleteMessage.message_id), while the DB uses raw bytes (StoredMessageDeletion.deleted_message_id). This split is error-prone and makes debugging harder. While the current code handles this correctly, the dual representation adds cognitive overhead.

Recommendation: Document this clearly in both the proto definition and the DB schema comments, or consider standardizing on one representation.

---

7. DeleteMessage fallback removed but backup mapping to Unknown (xmtp_content_types/src/delete_message.rs:36, xmtp_db/src/encrypted_store/group_message/convert.rs:154)

The codec has fallback: None (correct per review comments), but ContentType::DeleteMessage maps to ContentTypeSave::Unknown during backup. This means delete messages lose their semantic meaning during backup/restore cycles and become unrecoverable Unknown messages.

Recommendation: Either add ContentTypeSave::DeleteMessage variant for backup preservation, or document that delete messages are intentionally not backed up (which may be acceptable since they're control messages).

---

Summary

The implementation is generally solid with good test coverage for happy paths. The main concerns are around out-of-order deletion authorization, transaction boundaries, and the deletability of unknown content types. These issues could lead to security vulnerabilities or data inconsistencies in edge cases.

---

[codecov]

Codecov Report

❌ Patch coverage is 87.25361% with 97 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.29%. Comparing base (f948731) to head (a4ce98f).

Files with missing lines	Patch %	Lines
xmtp_db/src/encrypted_store/message_deletion.rs	91.05%	23 Missing ⚠️
xmtp_mls/src/groups/mls_sync.rs	90.36%	21 Missing ⚠️
bindings_node/src/content_types/deleted_message.rs	0.00%	17 Missing ⚠️
bindings_ffi/src/message.rs	0.00%	13 Missing ⚠️
..._node/src/content_types/decoded_message_content.rs	0.00%	8 Missing ⚠️
bindings_ffi/src/mls.rs	0.00%	4 Missing ⚠️
xmtp_mls/src/groups/error.rs	0.00%	4 Missing ⚠️
xmtp_content_types/src/delete_message.rs	91.89%	3 Missing ⚠️
xmtp_db/src/encrypted_store/group_message.rs	85.71%	2 Missing ⚠️
xmtp_content_types/src/lib.rs	0.00%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2772      +/-   ##
==========================================
+ Coverage   74.24%   74.29%   +0.04%     
==========================================
  Files         413      416       +3     
  Lines       53201    53933     +732     
==========================================
+ Hits        39501    40071     +570     
- Misses      13700    13862     +162     

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

[cameronvoell]

This PR addresses #2912. See XIP 76 for more detail: https://github.com/xmtp/XIPs/blob/main/XIPs/xip-76-delete-messages.md

[rygine]

please rename this file to deleted_message.rs

[rygine]

this is not necessary, right?

[mchenani]

@codabrink what's you opinion here?