Skip to content

fix: delete orphaned SignatureKeyPair when clearing a self_update commit#197

Merged
erskingardner merged 2 commits into
masterfrom
fix/clear-pending-commit-self-update
Feb 23, 2026
Merged

fix: delete orphaned SignatureKeyPair when clearing a self_update commit#197
erskingardner merged 2 commits into
masterfrom
fix/clear-pending-commit-self-update

Conversation

@erskingardner

Copy link
Copy Markdown
Member

Summary

When self_update() creates a pending commit, it eagerly stores a new SignatureKeyPair in the provider before calling OpenMLS. If the Kind:445 publish fails and clear_pending_commit() is called to roll back, the MLS group reverts to the old signer — but the new keypair previously remained in storage indefinitely.

This violates the OpenMLS documentation, which explicitly states that the application is responsible for deleting the new keypair when discarding a self-update commit. In persistent storage (SQLite), repeated failed self-update publishes would accumulate unreachable private key material.

Changes

  • clear_pending_commit: Before clearing, detects whether the pending commit is a self-update (using the same update_path_leaf_node heuristic as merge_pending_commit). If so, captures the new leaf's public key and calls SignatureKeyPair::delete() after the rollback.

  • test_self_update_then_merge_no_orphan: Verifies the happy path — after self_update + merge_pending_commit, the new keypair is present before merge and remains present after (it is now the active signer).

  • test_self_update_then_clear_removes_orphaned_keypair: Verifies the fix — after self_update + clear_pending_commit, the new keypair is present before clearing and absent after. Also verifies the group can successfully retry self_update + merge_pending_commit.

When self_update() stores a new SignatureKeyPair before creating a pending
commit, and that commit is then rolled back via clear_pending_commit(), the
new keypair previously remained in storage indefinitely — accumulating
unreachable private key material with each failed publish attempt.

clear_pending_commit() now detects self-update commits (using the same
update_path_leaf_node heuristic as merge_pending_commit), extracts the
new leaf's public key, and calls SignatureKeyPair::delete() after the
rollback, as required by the OpenMLS documentation.

Two new tests cover this:
- test_self_update_then_merge_no_orphan: verifies the happy path leaves
  the new key in storage as the active signer after a successful merge
- test_self_update_then_clear_removes_orphaned_keypair: verifies the
  orphaned key is deleted after a clear, and the group can retry
@coderabbitai

coderabbitai Bot commented Feb 23, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉


ℹ️ Recent review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 06a0636 and b94f595.

📒 Files selected for processing (2)
  • crates/mdk-core/CHANGELOG.md
  • crates/mdk-core/src/groups.rs

📝 Walkthrough

Walkthrough

This PR addresses orphaned private key material accumulation by enhancing the clear_pending_commit function to detect self_update commits, extract the newly-stored signing key's public key, and delete the associated orphaned SignatureKeyPair from storage. Related tests verify the cleanup behavior works correctly.

Changes

Cohort / File(s) Summary
Documentation
crates/mdk-core/CHANGELOG.md
Added Fixed entry documenting the new cleanup behavior for orphaned SignatureKeyPair deletion when self_update pending commits are rolled back.
Core Implementation & Tests
crates/mdk-core/src/groups.rs
Enhanced clear_pending_commit to detect self_update pending commits, extract the new signing key from the staged commit, and delete the orphaned SignatureKeyPair from storage. Added test_self_update_then_merge_no_orphan to verify key rotation persists post-merge, and test_self_update_then_clear_removes_orphaned_keypair to verify cleanup and retry flow work correctly.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

security, mls-protocol, storage

Suggested reviewers

  • dannym-arx
  • mubarakcoded
  • jgmontoya
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: fixing cleanup of orphaned SignatureKeyPair when rolling back a self_update commit, which matches the core objective and changes in the PR.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
No Sensitive Identifier Leakage ✅ Passed PR implements clear_pending_commit() with orphaned keypair cleanup without leaking sensitive identifiers through logs, errors, or debug output.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/clear-pending-commit-self-update

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown

✅ Coverage: 91.04% → 91.08% (+0.04%)

@github-actions

Copy link
Copy Markdown

✅ Coverage: 91.04% → 91.09% (+0.05%)

@erskingardner
erskingardner merged commit 58f7fed into master Feb 23, 2026
15 checks passed
@erskingardner
erskingardner deleted the fix/clear-pending-commit-self-update branch February 23, 2026 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants