Fix blocked welcome sender checks - #848
Conversation
Walkthrough
ChangesWelcome block/mute verification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 3 individual chapters for you:
Chapters generated by Stage for commit d660299 on May 25, 2026 1:14pm UTC. |
✅ Coverage: 94.08% → 94.09% (+0.01%)History
|
|
@coderabbitai full review please |
|
PR Review: Fix blocked welcome sender checksSummary: The core security fix is correct and well-reasoned — replacing the attacker-controllable 🔴 High — Orphaned MDK state when post-MLS check drops the welcome
// process_welcome creates internal MDK state here:
let welcome = mdk
.process_welcome(&event.id, &rumor)
.map_err(WhitenoiseError::MdkCoreError)?;
// ... post-MLS block check ...
if session.mute_list().is_user_blocked(&pubkey).await? {
return Ok(()); // MDK state from process_welcome is left orphaned
}
// Only reached for un-blocked welcomes:
mdk.accept_welcome(&welcome)?;If MDK tracks pending-welcome state keyed by event ID (e.g. to prevent double-processing), a relay replay of the same giftwrap after the initial drop could hit a "already processed" error path rather than being silently dropped again. Depending on Recommendation: Check whether // NOTE: mdk.process_welcome() was called but mdk.accept_welcome() is intentionally
// skipped for blocked welcomers. MDK treats un-accepted welcomes as ephemeral —
// no persistent state is written. Re-delivery of this welcome will be dropped again
// by the same block check.
return Ok(());🟡 Medium — Missing explanatory comment on the early
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/whitenoise/event_processor/event_handlers/handle_giftwrap.rs (1)
149-176:⚠️ Potential issue | 🟠 MajorAdd pending-welcome discard/reject on blocked welcomer/admin early return
Insrc/whitenoise/event_processor/event_handlers/handle_giftwrap.rs(the earlyreturn Ok(())aftermdk.process_welcome(...)for blockedwelcomer/admin identities),mdk-core’sprocess_welcomepersists the incoming welcome as “pending welcomes” (pending_welcomes). That path should explicitly call the mdk-core pending-welcome reject/discard cleanup API (fromcrates/mdk-core/src/welcomes.rs) so dropped welcomes don’t accumulate.
- Extend the regression tests to assert the pending-welcome state is empty/not retained for these dropped cases (not just that
AccountGroup::visible_for_account(...)is empty).🤖 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 `@src/whitenoise/event_processor/event_handlers/handle_giftwrap.rs` around lines 149 - 176, The early return inside the loop in handle_giftwrap after calling mdk.process_welcome(...) can leave the welcome persisted in mdk-core’s pending_welcomes; before returning from the blocked-welcomer/admin branch (the branch that checks session.mute_list().is_user_blocked(...) and currently does return Ok(())) call the mdk-core pending-welcome cleanup API (the reject/discard function exposed in crates::mdk_core::welcomes.rs) to explicitly reject/discard the pending welcome for that event ID (use the event.id or welcome metadata to identify the pending welcome), then return; also update the existing regression tests to assert that mdk-core’s pending_welcomes is empty/not retaining the dropped welcome (in addition to asserting AccountGroup::visible_for_account(...) remains empty).
🧹 Nitpick comments (1)
src/whitenoise/event_processor/event_handlers/handle_giftwrap.rs (1)
859-937: ⚡ Quick winAdd a regression that isolates the blocked-admin branch.
The new post-MLS guard also walks
welcome.group_admin_pubkeys, but this test blocks the creator/welcomer. A case where the welcomer is allowed and only an injected admin is blocked would lock down the admin-path logic separately.🤖 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 `@src/whitenoise/event_processor/event_handlers/handle_giftwrap.rs` around lines 859 - 937, The test test_handle_giftwrap_welcome_blocked_welcomer_with_decoy_rumor_is_dropped currently mutes the creator/welcomer (MuteListEntry::insert(&creator_account.pubkey,...)) which walks welcome.group_admin_pubkeys too; change or add a new regression test that instead leaves the welcomer allowed and mutes an injected admin key from welcome.group_admin_pubkeys so the admin-path is exercised independently. Concretely: create a separate admin Keys (e.g., admin_keys), include admin_pubkey in the group config via create_nostr_group_config_data so it appears in welcome.group_admin_pubkeys, call MuteListEntry::insert(&admin_pubkey, false, &member_session.account_db).await.unwrap() (not the creator), then build the same giftwrap via EventBuilder::gift_wrap and call handle_giftwrap; assert the giftwrap is dropped and AccountGroup::visible_for_account still returns empty. Ensure the new test name reflects "blocked_admin" to isolate the admin branch.
🤖 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 `@src/whitenoise/event_processor/event_handlers/handle_giftwrap.rs`:
- Around line 850-856: The tests currently only assert
AccountGroup::visible_for_account(...) is empty which can miss cases where a
welcome was accepted later; update both test blocks (around the
visible_for_account checks at the shown diff and also at the other location) to
additionally assert the account's pending/group state is empty by calling
create_mdk_for_account(...).get_groups().unwrap().is_empty() (or the equivalent
pending-invite check) for the same member_account.pubkey, ensuring that no MLS
groups exist even in the MDK (i.e., the welcome was never accepted).
---
Outside diff comments:
In `@src/whitenoise/event_processor/event_handlers/handle_giftwrap.rs`:
- Around line 149-176: The early return inside the loop in handle_giftwrap after
calling mdk.process_welcome(...) can leave the welcome persisted in mdk-core’s
pending_welcomes; before returning from the blocked-welcomer/admin branch (the
branch that checks session.mute_list().is_user_blocked(...) and currently does
return Ok(())) call the mdk-core pending-welcome cleanup API (the reject/discard
function exposed in crates::mdk_core::welcomes.rs) to explicitly reject/discard
the pending welcome for that event ID (use the event.id or welcome metadata to
identify the pending welcome), then return; also update the existing regression
tests to assert that mdk-core’s pending_welcomes is empty/not retaining the
dropped welcome (in addition to asserting AccountGroup::visible_for_account(...)
remains empty).
---
Nitpick comments:
In `@src/whitenoise/event_processor/event_handlers/handle_giftwrap.rs`:
- Around line 859-937: The test
test_handle_giftwrap_welcome_blocked_welcomer_with_decoy_rumor_is_dropped
currently mutes the creator/welcomer
(MuteListEntry::insert(&creator_account.pubkey,...)) which walks
welcome.group_admin_pubkeys too; change or add a new regression test that
instead leaves the welcomer allowed and mutes an injected admin key from
welcome.group_admin_pubkeys so the admin-path is exercised independently.
Concretely: create a separate admin Keys (e.g., admin_keys), include
admin_pubkey in the group config via create_nostr_group_config_data so it
appears in welcome.group_admin_pubkeys, call
MuteListEntry::insert(&admin_pubkey, false,
&member_session.account_db).await.unwrap() (not the creator), then build the
same giftwrap via EventBuilder::gift_wrap and call handle_giftwrap; assert the
giftwrap is dropped and AccountGroup::visible_for_account still returns empty.
Ensure the new test name reflects "blocked_admin" to isolate the admin branch.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 99b41010-14c5-46b0-83a7-975007522868
📒 Files selected for processing (1)
src/whitenoise/event_processor/event_handlers/handle_giftwrap.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/whitenoise/event_processor/event_handlers/handle_giftwrap.rs (1)
947-952:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMirror the MDK-state assertion in the blocked-seal-sender regression too.
This closes the gap for the decoy-welcomer path, but
test_handle_giftwrap_welcome_blocked_seal_sender_is_droppedstill only checks hidden app state. A future reorder that leaves pending MDK membership behind would still pass that test.Suggested change
let visible_groups = AccountGroup::visible_for_account(&whitenoise, &member_account.pubkey) .await .unwrap(); assert!( visible_groups.is_empty(), "blocked seal sender must not create a visible account group" ); + + let mdk_groups = member_session.mdk.get_groups().unwrap(); + assert!( + mdk_groups.is_empty(), + "blocked seal sender must not leave pending MDK group state" + );🤖 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 `@src/whitenoise/event_processor/event_handlers/handle_giftwrap.rs` around lines 947 - 952, Add the same MDK-state assertion used for the blocked welcomer path to the blocked-seal-sender regression: after the code path that handles the blocked seal sender (the logic exercised by test_handle_giftwrap_welcome_blocked_seal_sender_is_dropped), call member_session.mdk.get_groups().unwrap() and assert that the returned groups are empty (same message text: "blocked welcomer must not leave pending MDK group state" or adjust to reference blocked-seal-sender), ensuring the test checks that no pending MDK membership is left behind.
🧹 Nitpick comments (1)
src/whitenoise/event_processor/event_handlers/handle_giftwrap.rs (1)
142-145: ⚡ Quick winUse this file's module path for the new tracing targets.
These new logs keep the older pseudo-target (
whitenoise::event_processor::process_welcome) instead of the module path forsrc/whitenoise/event_processor/event_handlers/handle_giftwrap.rs, which makes filtering/routing inconsistent with the repo convention.Suggested change
- tracing::info!( - target: "whitenoise::event_processor::process_welcome", + tracing::info!( + target: "whitenoise::event_processor::event_handlers::handle_giftwrap", "Dropping welcome from blocked gift-wrap sender {}", seal_sender, );- tracing::warn!( - target: "whitenoise::event_processor::process_welcome", + tracing::warn!( + target: "whitenoise::event_processor::event_handlers::handle_giftwrap", error = %e, "Failed to clean up pending MDK group after blocked welcome" );As per coding guidelines
src/whitenoise/**/*.rs: Usetarget: "whitenoise::module_name"in logging calls to match the module path.Also applies to: 180-183
🤖 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 `@src/whitenoise/event_processor/event_handlers/handle_giftwrap.rs` around lines 142 - 145, The tracing calls in handle_giftwrap.rs are using the old pseudo-target "whitenoise::event_processor::process_welcome"; update those tracing::info/debug/error targets to the file's module path (e.g., "whitenoise::event_processor::event_handlers::handle_giftwrap" or whatever the module declared at the top of the file is) so logs follow the repo convention; locate the tracing invocations in the handle_giftwrap function (the call that logs "Dropping welcome from blocked gift-wrap sender {}" and the other calls around lines noted) and replace their target strings accordingly to match the module path used by this file.
🤖 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.
Duplicate comments:
In `@src/whitenoise/event_processor/event_handlers/handle_giftwrap.rs`:
- Around line 947-952: Add the same MDK-state assertion used for the blocked
welcomer path to the blocked-seal-sender regression: after the code path that
handles the blocked seal sender (the logic exercised by
test_handle_giftwrap_welcome_blocked_seal_sender_is_dropped), call
member_session.mdk.get_groups().unwrap() and assert that the returned groups are
empty (same message text: "blocked welcomer must not leave pending MDK group
state" or adjust to reference blocked-seal-sender), ensuring the test checks
that no pending MDK membership is left behind.
---
Nitpick comments:
In `@src/whitenoise/event_processor/event_handlers/handle_giftwrap.rs`:
- Around line 142-145: The tracing calls in handle_giftwrap.rs are using the old
pseudo-target "whitenoise::event_processor::process_welcome"; update those
tracing::info/debug/error targets to the file's module path (e.g.,
"whitenoise::event_processor::event_handlers::handle_giftwrap" or whatever the
module declared at the top of the file is) so logs follow the repo convention;
locate the tracing invocations in the handle_giftwrap function (the call that
logs "Dropping welcome from blocked gift-wrap sender {}" and the other calls
around lines noted) and replace their target strings accordingly to match the
module path used by this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 698ef378-62d1-4e2a-b665-b9d3bd59e864
📒 Files selected for processing (1)
src/whitenoise/event_processor/event_handlers/handle_giftwrap.rs
1 participant
Summary
Closes marmot-protocol/marmot-security#105
Verification
Note: just precommit-quick was also attempted; fmt/docs/clippy/dead_code passed, but the full unit test phase hit existing relay-dependent localhost failures unrelated to this change.
Summary by CodeRabbit
Bug Fixes
Tests