fix(security): block non-admin Add/Remove/GCE proposals from riding into admin commits#317
Open
dannym-arx wants to merge 3 commits into
Open
fix(security): block non-admin Add/Remove/GCE proposals from riding into admin commits#317dannym-arx wants to merge 3 commits into
dannym-arx wants to merge 3 commits into
Conversation
…nto admin commits Closes marmot-security#106. Before: any non-admin member could call propose_add_member / propose_remove_member directly against OpenMLS, publish the resulting kind:445 proposal, and have it sit in every peer's pending-proposal store until the next admin commit. That next commit (a rename, a self-update, an unrelated add/remove) consumed the whole store with |_| true, smuggling the non-admin's membership change in under the admin's signature. Receive-side validation only checked the committer, not the proposers, so peers accepted it. Fix is defense-in-depth on both sides: - Build side: prune_unauthorized_pending_proposals drains every non-admin Add / Remove(other) / GroupContextExtensions proposal from the pending store before add_members, remove_members, update_group_data_extension, and self_update sweep it into a commit. - Receive side: validate_committed_proposal_authorship walks every staged commit's queued proposals and rejects any Add / Remove(other) / GCE whose proposer is not in admin_pubkeys. Self- scoped proposals (Update, SelfRemove, Remove(self)) stay allowed so legacy non-admin leaves still work. Adds Error::UnauthorizedProposalInCommit (mapped to the existing authorization_failed failure category) and two tests: the issue's CTF test for the build-side prune, and a malicious-admin simulation that confirms a peer rejects the smuggled commit even when the committing admin skipped the prune.
dannym-arx
requested review from
erskingardner,
jgmontoya and
mubarakcoded
as code owners
|
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 (5)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds per-proposal admin-authorship validation and pending-proposal pruning to prevent unauthorized non-admin proposals from being bundled into admin-signed commits; introduces ChangesProposal Authorization and Smuggling Defense
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
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 6 individual chapters for you: 
Chapters generated by Stage for commit 20c0bdf on May 25, 2026 10:23am UTC. |
✅ Coverage: 94.51% → 94.59% (+0.08%) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/mdk-core/src/messages/error_handling.rs (1)
1042-1117:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd test coverage for
Error::UnauthorizedProposalInCommit.The test covers
Error::CommitFromNonAdminbut does not test the newly addedError::UnauthorizedProposalInCommit(_)variant. Add a test assertion to verify it maps to"authorization_failed".🧪 Proposed test addition
assert_eq!( MDK::<MdkMemoryStorage>::sanitize_error_reason(&Error::CommitFromNonAdmin), "authorization_failed" ); + + assert_eq!( + MDK::<MdkMemoryStorage>::sanitize_error_reason(&Error::UnauthorizedProposalInCommit( + "test_proposal".to_string() + )), + "authorization_failed" + ); // Test catch-all for unmapped variants (should return "processing_failed")🤖 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/src/messages/error_handling.rs` around lines 1042 - 1117, The test function test_sanitize_error_reason_all_variants misses coverage for the new Error::UnauthorizedProposalInCommit variant; add an assertion calling MDK::<MdkMemoryStorage>::sanitize_error_reason(&Error::UnauthorizedProposalInCommit(some_value)) and assert it equals "authorization_failed" (choose any representative inner value, e.g., 1 or a tuple expected by the variant) alongside the existing CommitFromNonAdmin check so the sanitize_error_reason mapping for UnauthorizedProposalInCommit is validated.
🧹 Nitpick comments (1)
crates/mdk-core/src/messages/proposal.rs (1)
971-973: ⚡ Quick winHoist these imports to the test module scope.
These local
usestatements should live in the#[cfg(test)] mod testsimport block instead of inside individual test functions.As per coding guidelines
All use statements must be placed at the TOP of their containing scope, never inside functions, methods, blocks, or individual test functions.Also applies to: 1091-1093, 1201-1201
🤖 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/src/messages/proposal.rs` around lines 971 - 973, Several tests (e.g., ctf_nonadmin_add_proposal_smuggled_into_admin_commit and the tests around lines you noted) have local `use` statements inside the test functions; move those imports (for example `use crate::groups::NostrGroupDataUpdate` and the other per-test `use` lines at 1091-1093 and 1201) up into the `#[cfg(test)] mod tests { ... }` module-level import block so all `use` statements are at the top of their containing scope rather than inside the individual test functions.
🤖 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/groups.rs`:
- Around line 1333-1335: The upgrade_group_capabilities path is missing the
pending-proposal scrub used elsewhere; call
prune_unauthorized_pending_proposals(mls_group, group_id)? at the start of
upgrade_group_capabilities (the same way update_group_context_extensions does)
before reaching the commit API that applies admin capability upgrades, so any
non-admin Add/Remove(other)/GCE proposals are removed from the pending store
prior to creating/applying the commit.
In `@crates/mdk-core/src/messages/proposal.rs`:
- Around line 1047-1079: Add assertions that Alice's and Carol's replicated
group data actually reflect the rename: call alice_mdk.get_group_data(&group_id)
and carol_mdk.get_group_data(&group_id) (or the equivalent accessor in this
module) after processing the rename and assert the group's name equals "Renamed"
on both replicas; reference the existing rename variable
(rename.evolution_event) and group_id so the checks run immediately after
carol_mdk.process_message(&rename.evolution_event).
In `@crates/mdk-core/src/messages/validation.rs`:
- Around line 486-494: In prune_unauthorized_pending_proposals, do not just warn
and continue when mls_group.remove_pending_proposal(self.provider.storage(),
&proposal_ref) returns Err; instead propagate that error back to the caller
(e.g. return Err(...) or map it into the function's error type) so the prune
failure aborts the operation and prevents the unauthorized proposal from
remaining in the pending store; update the match/if-let handling around
remove_pending_proposal to return the error immediately rather than logging and
continuing.
- Around line 474-484: The current prune_unauthorized_pending_proposals only
checks is_proposal_admin_authorized which applies admin semantics and thus
leaves admin-only proposals when a non-admin performs self_update; change the
pruning policy to consider the caller/operation: update
prune_unauthorized_pending_proposals (or add a new parameter such as
is_admin_or_self_update_flag) so it rejects proposals that require admin rights
(Add, Remove(other), GroupContextExtensions) when the caller is not an admin or
when called from a non-admin self_update, and only allows self-scoped proposals
(e.g., UpdateSelf or proposals authored/targeting the caller) to survive;
alternatively, when invoked from self_update by a non-admin, bypass consuming
the proposal store and only include proposals with self-scope. Ensure you
reference pending_proposals(), is_proposal_admin_authorized(), and the
self_update call site so the filter logic distinguishes admin vs non-admin flows
and drops CommitFromNonAdmin-prone proposals.
---
Outside diff comments:
In `@crates/mdk-core/src/messages/error_handling.rs`:
- Around line 1042-1117: The test function
test_sanitize_error_reason_all_variants misses coverage for the new
Error::UnauthorizedProposalInCommit variant; add an assertion calling
MDK::<MdkMemoryStorage>::sanitize_error_reason(&Error::UnauthorizedProposalInCommit(some_value))
and assert it equals "authorization_failed" (choose any representative inner
value, e.g., 1 or a tuple expected by the variant) alongside the existing
CommitFromNonAdmin check so the sanitize_error_reason mapping for
UnauthorizedProposalInCommit is validated.
---
Nitpick comments:
In `@crates/mdk-core/src/messages/proposal.rs`:
- Around line 971-973: Several tests (e.g.,
ctf_nonadmin_add_proposal_smuggled_into_admin_commit and the tests around lines
you noted) have local `use` statements inside the test functions; move those
imports (for example `use crate::groups::NostrGroupDataUpdate` and the other
per-test `use` lines at 1091-1093 and 1201) up into the `#[cfg(test)] mod tests
{ ... }` module-level import block so all `use` statements are at the top of
their containing scope rather than inside the individual test functions.
🪄 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: 401e8976-71b9-49a9-8d3c-63be20c7bf81
📒 Files selected for processing (5)
crates/mdk-core/src/error.rscrates/mdk-core/src/groups.rscrates/mdk-core/src/messages/error_handling.rscrates/mdk-core/src/messages/proposal.rscrates/mdk-core/src/messages/validation.rs
…, propagate prune errors, strengthen tests Review pass on PR #317. Four still-valid findings applied: - Cover upgrade_group_capabilities with prune_unauthorized_pending_proposals so the GCE commit it produces cannot sweep non-admin proposals. - Propagate remove_pending_proposal storage failures from the prune helper instead of warn-and-continue: a silent failure would leave the smuggled proposal in the store for the next sweep, defeating the build-side guard. - Strengthen ctf_nonadmin_add_proposal_smuggled_into_admin_commit by asserting the rename actually landed on both replicas, so a regression that dropped the legitimate change along with the prune cannot pass. - Add sanitize_error_reason coverage for the new UnauthorizedProposalInCommit variant. Skipped (with reasons): - Non-admin self_update sweeping admin-authored proposals causing CommitFromNonAdmin on peers: pre-existing behavior, orthogonal to #106 (no privilege escalation). Belongs in a separate non-admin commit hygiene issue. - Move per-test `use` statements to the module-level import block: stylistic. Per-test `use` keeps each test self-contained; not a correctness issue.
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
1 participant
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-security#106.
A non-admin marmot in a group could call OpenMLS'
propose_add_memberdirectly, publish the resulting kind:445 event, and have it sit in every peer's pending-proposal store. The next admin commit (a rename, a self-update, an unrelated add) consumed the whole store with|_| trueand signed in the smuggled change. The admin saw "you renamed the group" in their UI while an attacker-controlled identity joined the encrypted group and received the exporter secret.Build side
prune_unauthorized_pending_proposalsdrains every non-admin Add, Remove-of-other, or GroupContextExtensions proposal from the pending store beforeadd_members,remove_members,update_group_data_extension, orself_updateconsume it. Self-scoped proposals (Update,SelfRemove,Remove(self)for the legacy leave path) stay in place, so non-admin self-leave still works.Receive side
validate_committed_proposal_authorshipwalks each staged commit's queued proposals and rejects any Add, Remove-of-other, or GCE whose proposer is not inadmin_pubkeys. This catches a non-conformant admin client that skips the build-side prune. The newUnauthorizedProposalInCommiterror maps to the existingauthorization_failedfailure category, so peers record the rejection and stay on the old epoch.Tests
ctf_nonadmin_add_proposal_smuggled_into_admin_commit: the reporter's reproducer. The assertion that Mallory is not a member now holds.receive_side_rejects_admin_commit_with_smuggled_nonadmin_add: a peer rejects a smuggled commit even when the committing admin client bypasses the prune.All 461 mdk-core lib tests pass;
just precommitclean.Per MIP-03 "Commit Messages", Add/Remove/GCE application is admin-only.
This PR fixes a critical vulnerability (marmot-security#106) where non-admin members could craft OpenMLS proposals that remained in the pending-proposal store and be accidentally or maliciously consumed by an admin-signed commit, allowing unauthorized Adds, Removes-of-others, and GroupContextExtensions to be applied. It implements build- and receive-side guards to prune or reject such unauthorized proposals, and records a clear error when a staged commit bundles an unauthorized proposal.
What changed:
Error::UnauthorizedProposalInCommit(String)to crates/mdk-core/src/error.rs to signal commits bundling unauthorized proposals.is_proposal_admin_authorized(),prune_unauthorized_pending_proposals(), andvalidate_committed_proposal_authorship()in crates/mdk-core/src/messages/validation.rs to implement per-proposal admin authorization checks, build-side pruning, and receive-side validation.get_unknown_extension_from_group_datatopub(crate).authorization_failedsanitized category in crates/mdk-core/src/messages/error_handling.rs so rejected commits are recorded consistently.Security impact:
Protocol changes:
API surface:
UnauthorizedProposalInCommit(String)added to theErrorenum.is_proposal_admin_authorized(),prune_unauthorized_pending_proposals(),validate_committed_proposal_authorship().get_unknown_extension_from_group_data()madepub(crate).Testing:
ctf_nonadmin_add_proposal_smuggled_into_admin_commit(build-side pruning) andreceive_side_rejects_admin_commit_with_smuggled_nonadmin_add(receive-side rejection andauthorization_failedrecording).just precommitis clean.