fix(server): validate cosigner_commitments against initial account state - #380
Conversation
/configure accepted the client-declared auth.cosigner_commitments after only proving the requesting key exists in the account state, so the stored list — the authorization source of truth for every later request (Auth::verify) — could name keys that are not signers of the account. Extract the signer set from initial_state via should_update_auth and reject the configuration when the declared list does not match it exactly. A state with no extractable signer set skips the check, matching canonicalization semantics; every real multisig-guardian account carries a non-empty signer map, so the check always runs for real accounts. The switch/abandon e2e tests onboarded 2-of-2 accounts declaring only one cosigner — exactly the drift now rejected — and register the full signer set instead. Closes #102
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAccount configuration now validates declared cosigner commitments against commitments extracted from the submitted account state. The change adds a public ChangesCosigner commitment validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/server/src/services/configure_account.rs (1)
782-828: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a reordered-commitments rejection test.
The implementation uses
Authequality, so ordering is enforced. The current tests cover different and extra commitments, but not the required case where the same commitments appear in a different order. Add a two-signer test that returns[first, second]fromshould_update_authand submits[second, first]. AssertInvalidInputand no state persistence.🤖 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/server/src/services/configure_account.rs` around lines 782 - 828, Add a two-signer test alongside test_configure_account_rejects_injected_extra_cosigner that generates two commitments, configures should_update_auth to return them in [first, second] order, and submits the same commitments as [second, first]. Assert configure_account returns Err(GuardianError::InvalidInput(_)) and storage_backend records no state submission.
🤖 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.
Nitpick comments:
In `@crates/server/src/services/configure_account.rs`:
- Around line 782-828: Add a two-signer test alongside
test_configure_account_rejects_injected_extra_cosigner that generates two
commitments, configures should_update_auth to return them in [first, second]
order, and submits the same commitments as [second, first]. Assert
configure_account returns Err(GuardianError::InvalidInput(_)) and
storage_backend records no state submission.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 90489350-3848-4a14-a67f-e491517cd044
📒 Files selected for processing (5)
crates/server/src/metadata/auth/mod.rscrates/server/src/services/configure_account.rscrates/server/src/testing/e2e/abandon_candidate.rscrates/server/src/testing/e2e/configure_account.rscrates/server/src/testing/e2e/switch_guardian_canonicalization.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Strengthens the server-side /configure flow by ensuring the client-declared cosigner commitments exactly match the signer set embedded in the submitted initial account state, preventing unauthorized commitments from being persisted into the account’s long-lived authorization configuration.
Changes:
- Add
/configurevalidation that compares declaredauth.cosigner_commitmentsagainst signer commitments extracted frominitial_stateviaNetworkClient::should_update_auth. - Add a shared
Auth::cosigner_commitments()accessor to standardize commitment list access across schemes. - Update/extend E2E and service tests to cover cosigner mismatch and ensure updated onboarding flows provide full signer lists.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/server/src/services/configure_account.rs | Enforces cosigner commitment validation against extracted signer set; adds multiple unit tests for mismatch/error/skip semantics. |
| crates/server/src/metadata/auth/mod.rs | Adds Auth::cosigner_commitments() accessor used by the new validation and error reporting. |
| crates/server/src/testing/e2e/configure_account.rs | Adds an integration-style rejection test using real signer extraction (should_update_auth) via an integration network client wrapper. |
| crates/server/src/testing/e2e/switch_guardian_canonicalization.rs | Updates onboarding in E2E flow to register the full cosigner set to satisfy the new /configure validation. |
| crates/server/src/testing/e2e/abandon_candidate.rs | Updates test setup to configure accounts with the full cosigner set consistent with new validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| GuardianError::NetworkError(format!( | ||
| "Failed to extract signer commitments from initial state: {e}" | ||
| )) | ||
| })?; |
| return Err(GuardianError::InvalidInput(format!( | ||
| "cosigner_commitments do not match the signer set in initial_state: expected {:?}, provided {:?}", | ||
| expected_auth.cosigner_commitments(), | ||
| params.auth.cosigner_commitments() | ||
| ))); |
zeljkoX
left a comment
zeljkoX
left a comment
There was a problem hiding this comment.
Thanks for working on this one.
Looks good.
Few nits:
test_configure_account_with_real_miden_accountstill uses a mock path where should_update_auth() returns Ok(None). Consequently, it succeeds without proving that the supplied commitments match the account state and would continue passing if the new validation were removed.- Update
spec/processes.mdto state that, for MultisigGuardian accounts, auth.cosigner_commitments must exactly match the signer map extracted from initial_state, including its canonical order. This is a user-visible auth behavior change and helps explain otherwise confusing InvalidInput responses. - Add
test_rejects_reordered_commitmentsto lock in the deliberately order-sensitive behavior. - Consider validating commitments at ingestion as canonical 0x-prefixed, lowercase, 32-byte hex and explicitly rejecting empty or duplicate lists. Current exact string equality works with the SDKs but can produce confusing failures for manual clients.
- Consider mapping malformed client-supplied initial_state to InvalidInput rather than NetworkError. This is consistent with existing behavior, so it need not block this PR.
… spec doc - Reject non-canonical (not 0x + 64 lowercase hex), empty, or duplicate cosigner_commitments at ingestion so manual clients get a named error instead of a confusing set-mismatch. - Lock in order-sensitive comparison with test_rejects_reordered_commitments. - Exercise the real extraction path in test_configure_account_with_real_miden_account: the declared set is the fixture account's actual 3-signer map (keys.json, map order) and the credential is signed by fixture signer 1, so the test fails if the validation is removed or the order convention drifts. - Document the exact-match requirement in spec/processes.md.
github-project-automation
Bot
moved this from Review
to Release Candidate
in OZ Development for Miden
Closes #102. Supersedes #109, which predates the
GuardianErrormigration and no longer applies.Problem
/configureaccepted the client-declaredauth.cosigner_commitmentsafter only verifying that the requesting key exists in the account state (validate_credential). The rest of the list was stored unvalidated — and that stored list is the authorization source of truth for every subsequent request (Auth::verifyauthorizes any key whose commitment appears in it). A configure call could therefore register commitments that are not signers of the account.Fix
After the guardian-binding check, extract the signer set from
initial_state(via the existingNetworkClient::should_update_auth, which reads theopenzeppelin::multisig::signer_public_keysmap) and reject withInvalidInputwhen the declared list does not match it exactly (same set and order — the SDK derives the list from the same map walk, so honest clients are unaffected).Ok(None)(no extractable signer set) skips the check, matching the canonicalization-side semantics ofshould_update_auth. Every real multisig-guardian account carries a non-empty signer map —MultisigGuardianConfigwrites all signers, including 1-of-1 — so the check always runs for real accounts.Summary by CodeRabbit
New Features
Bug Fixes