Fix replay deduplication for wrapped messages#316
Open
mubarakcoded wants to merge 2 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
✅ Files skipped from review due to trivial changes (3)
📝 WalkthroughWalkthroughThis PR implements stable, payload-level replay deduplication for MLS messages wrapped in Nostr kind:445 events. It derives a stable inner EventId from payload content, adds a group-scoped storage lookup, inserts early and post-decrypt replay gates that return ChangesCiphertext replay deduplication via stable content hash
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Ready to review this PR? Stage has broken it down into 8 individual chapters for you: 
Chapters generated by Stage for commit d111728 on May 25, 2026 10:27am UTC. |
Contributor
Author
|
@coderabbitai full review. |
✅ Coverage: 94.51% → 94.52% (+0.01%) |
|
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/mdk-core/tests/shared/message_tests.rs (1)
97-100: ⚡ Quick winAdd a wrong-group negative lookup assertion.
Line 98 currently verifies only a missing
message_event_id. Add a case using the samemessage_event_idwith a differentmls_group_idto confirm strict group scoping.Suggested test addition
let result = storage .find_processed_message_by_message_event_id(&mls_group_id, &non_existent_id) .unwrap(); assert!(result.is_none()); + + // Same message_event_id in a different group should not match + let other_group_id = GroupId::from_slice(&[9, 9, 9, 9]); + let result = storage + .find_processed_message_by_message_event_id(&other_group_id, &message_event_id) + .unwrap(); + assert!(result.is_none());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mdk-core/tests/shared/message_tests.rs` around lines 97 - 100, Add a negative lookup asserting group scoping by calling find_processed_message_by_message_event_id with the same non_existent_id but a different MLS group id: create or derive a distinct mls_group_id (e.g., other_mls_group_id) and call storage.find_processed_message_by_message_event_id(&other_mls_group_id, &non_existent_id).unwrap() and assert that the result is None to ensure the lookup is strict to the group; place this immediately after the existing assertion that uses mls_group_id to keep the test logic clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/mdk-core/src/messages/application.rs`:
- Around line 58-82: The current replay check treats any existing record from
storage().find_message_by_event_id(&group.mls_group_id, &rumor_id) as a replay
and returns MessageProcessingResult::Unprocessable, which also matches valid
“own cached message” paths; fix by binding the found record to a variable
instead of is_some(), inspect its metadata (e.g., originator/sender/wrapper and
processed state) and only consider it a replay if the stored record indicates it
was processed by a different origin/wrapper or has a Processed state that should
block reprocessing; otherwise continue normal ApplicationMessage processing.
Update the logic around find_message_by_event_id, the conditional that currently
creates create_processed_message_record and calls save_processed_message_record,
so you only create/save and return Unprocessable when the stored record actually
represents a true replay (different origin/wrapper or already-final state).
In `@crates/mdk-core/src/messages/process.rs`:
- Around line 444-478: The current replay gate queries
find_processed_message_by_message_event_id(mls_group_id, content_hash_event_id)
and rejects whenever any prior match exists, which also blocks idempotent
reprocessing of the same wrapper event; change the logic to fetch the existing
processed message (if any) and only treat it as a replay if the stored processed
message's message_event_id differs from the current event.id—if they are equal,
allow processing to continue. Update the block around content_hash_event_id,
find_group_by_nostr_group_id, and the subsequent
find_processed_message_by_message_event_id call (and the analogous block at
Lines 513–539) to perform this explicit message_event_id comparison before
creating the processed_message record, saving it via
save_processed_message_record, and returning
MessageProcessingOutcome::without_context(MessageProcessingResult::Unprocessable).
---
Nitpick comments:
In `@crates/mdk-core/tests/shared/message_tests.rs`:
- Around line 97-100: Add a negative lookup asserting group scoping by calling
find_processed_message_by_message_event_id with the same non_existent_id but a
different MLS group id: create or derive a distinct mls_group_id (e.g.,
other_mls_group_id) and call
storage.find_processed_message_by_message_event_id(&other_mls_group_id,
&non_existent_id).unwrap() and assert that the result is None to ensure the
lookup is strict to the group; place this immediately after the existing
assertion that uses mls_group_id to keep the test logic clear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 439b1b94-d4fb-438d-86a1-23b73c9a72f8
📒 Files selected for processing (9)
crates/mdk-core/src/messages/application.rscrates/mdk-core/src/messages/commit.rscrates/mdk-core/src/messages/mod.rscrates/mdk-core/src/messages/process.rscrates/mdk-core/src/messages/proposal.rscrates/mdk-core/tests/shared/message_tests.rscrates/mdk-memory-storage/src/messages.rscrates/mdk-sqlite-storage/src/messages.rscrates/mdk-storage-traits/src/messages/mod.rs
mubarakcoded
force-pushed
the
fix/replay-dedup-marmot-security-2
branch
from
0c104c4 to
070acd3
Compare
mubarakcoded
force-pushed
the
fix/replay-dedup-marmot-security-2
branch
from
070acd3 to
f96403b
Compare
Contributor
Author
|
@claude full review |
mubarakcoded
requested review from
dannym-arx,
erskingardner and
jgmontoya
as code owners
dannym-arx
requested changes
May 25, 2026
dannym-arx
left a comment
dannym-arx
left a comment
Contributor
There was a problem hiding this comment.
I'm confused about the separate hash. Nostr IDs are already unique and stable.
they are sha256 of:
json_encode([
0,
<pubkey, as a lowercase hex string>,
<created_at, as a number>,
<kind, as a number>,
<tags, as an array of arrays of non-null strings>,
<content, as a string>
])
https://github.com/nostr-protocol/nips/blob/master/01.md#events-and-signatures
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes marmot-protocol/marmot-security#2.
mdk-corepreviously deduplicated incomingkind:445messages only by the outer wrapper event ID. An attacker (without the group key) could re-publish the same encrypted MLS payload inside a freshly-signed wrapper — new outer event ID → step-0 dedup misses → MDK re-decrypts and re-emits the same inner message. For application messages this surfaced as duplicateApplicationMessagedeliveries (and overwrittenwrapper_event_id/processed_atmetadata) to the caller. For commits and proposals it forced redundant re-entry into MLS processing and error-handling paths.The fix adds a stable post-decrypt dedup key independent of the outer event ID, and lifts the check ahead of decrypt where possible so replays are dropped before paying any MLS cost:
process_messagenow looks up(mls_group_id, SHA-256(event.content))againstprocessed_messages.message_event_idbefore attempting to decrypt. Replays whose group is already in local storage are rejected here; no MLS state machinery is touched.decrypt_messagesucceeds, for the case where the group wasn't yet in storage during the pre-decrypt pass.process_application_messagerejects an application replay whose inner rumor ID is already in the messages table, returningUnprocessableinstead of re-emittingApplicationMessage.To populate the content-hash key, every commit/proposal/external-join save-site now stores
Some(content_hash_event_id(&event.content))inProcessedMessage::message_event_idinstead ofNone. Theprocessed_messagescolumn is unchanged (32-byte BLOB); no DB migration. SHA-256 collision resistance keeps the synthetic content-hash IDs from clashing with real Nostr rumor IDs that already occupy the column.Breaking Change
mdk-storage-traitsadds requiredMessageStorage::find_processed_message_by_message_event_id.In-tree memory/sqlite backends are updated in this PR, but external storage backend implementors must add this method when upgrading.
Verification
cargo test -p mdk-core --lib replayed_— both Layer A integration tests pass.cargo test -p mdk-core --lib test_application_replay_with_fresh_wrapper_returns_unprocessable— Layer B regression passes.cargo test -p mdk-core --test storage_traits_memory --test storage_traits_sqlite test_processed_message— new lookup verified on both backends.cargo test -p mdk-core --lib test_content_hash_event_id_is_stable_and_content_sensitive— unit test on the hash helper.just precommitpassesWhat changed:
process.rsthat performs a two-stage check: pre-decrypt lookup inprocessed_messagesby (mls_group_id, content_hash_event_id), and a post-decrypt fallback when the group was not present initially, rejecting replays with different wrapper event IDs.application.rsthat looks up existing messages by their decrypted rumor event ID and returnsUnprocessablewhen the same rumor is republished with a different wrapper event ID.content_hash_event_id()helper function inmessages/mod.rsthat converts SHA-256(event.content) into a stable syntheticEventId(independent of the outer wrapper event ID).commit.rsandproposal.rsto store content-hash-based synthetic IDs inProcessedMessage::message_event_idinstead ofNone.Security impact:
API surface:
MessageStorage::find_processed_message_by_message_event_id(mls_group_id, message_event_id)trait method for group-scoped processed-message lookup by stable inner message/content-hash ID; all storage backends must implement this method.process_application_message()signature to returnResult<MessageProcessingResult>instead ofResult<message_types::Message>.content_hash_event_id(content: &str) -> EventIdfor converting SHA-256 digests into synthetic event IDs.Testing:
Unprocessablereturn and still records a processed-message entry inProcessedstate.find_processed_message_by_message_event_idlookups in memory and SQLite storage backends.