From e7bb3956bccefd1c609b4ef9092ebe409656a7c7 Mon Sep 17 00:00:00 2001 From: Mario Ruci Date: Thu, 23 Jul 2026 09:48:09 +0200 Subject: [PATCH 1/3] fix(upgrader): reject invalid disaster recovery committee quorum set_committee now rejects quorum == 0 (would approve recovery with no votes) and quorum greater than the number of committee members (can never be met), alongside the existing empty-committee check. Co-Authored-By: Claude Opus 4.8 --- core/upgrader/impl/src/errors/mod.rs | 9 +++++ .../impl/src/services/disaster_recovery.rs | 34 +++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/core/upgrader/impl/src/errors/mod.rs b/core/upgrader/impl/src/errors/mod.rs index 706c62818..09678603b 100644 --- a/core/upgrader/impl/src/errors/mod.rs +++ b/core/upgrader/impl/src/errors/mod.rs @@ -5,6 +5,7 @@ pub enum UpgraderApiError { Unauthorized, DisasterRecoveryInProgress, EmptyCommittee, + InvalidQuorum, Unexpected(String), } @@ -31,6 +32,14 @@ impl From for ApiError { message: Some("Committee cannot be empty.".to_owned()), details: None, }, + UpgraderApiError::InvalidQuorum => ApiError { + code: "INVALID_QUORUM".to_owned(), + message: Some( + "Committee quorum must be greater than 0 and at most the number of committee members." + .to_owned(), + ), + details: None, + }, UpgraderApiError::Unexpected(err) => ApiError { code: "UNEXPECTED_ERROR".to_owned(), message: Some(err), diff --git a/core/upgrader/impl/src/services/disaster_recovery.rs b/core/upgrader/impl/src/services/disaster_recovery.rs index 9e6670c43..912fb6ade 100644 --- a/core/upgrader/impl/src/services/disaster_recovery.rs +++ b/core/upgrader/impl/src/services/disaster_recovery.rs @@ -124,6 +124,12 @@ impl DisasterRecoveryService { return Err(UpgraderApiError::EmptyCommittee.into()); } + // A quorum of 0 would approve disaster recovery with no votes; a quorum larger than the + // committee could never be met. Both are misconfigurations that must be rejected. + if committee.quorum == 0 || committee.quorum as usize > committee.users.len() { + return Err(UpgraderApiError::InvalidQuorum.into()); + } + value.committee = Some(committee.clone()); // only retain recovery requests from committee members @@ -572,6 +578,34 @@ mod tests { } } + #[tokio::test] + async fn set_committee_rejects_invalid_quorum() { + let dr = DisasterRecoveryService { + installer: Arc::new(TestInstaller::default()), + storage: Default::default(), + logger: Default::default(), + }; + + // mock_committee has 3 members. + let mut committee = mock_committee(); + + committee.quorum = 0; + dr.set_committee(committee.clone()) + .expect_err("quorum of 0 must be rejected"); + + committee.quorum = committee.users.len() as u16 + 1; + dr.set_committee(committee.clone()) + .expect_err("quorum greater than the number of members must be rejected"); + + committee.quorum = committee.users.len() as u16; + dr.set_committee(committee.clone()) + .expect("quorum equal to the number of members must be accepted"); + + committee.quorum = 1; + dr.set_committee(committee) + .expect("a positive quorum within the committee size must be accepted"); + } + #[tokio::test] async fn test_request_recovery() { let dr = DisasterRecoveryService { From da60d96f4255bd3d1f4f54f49932258d1171508c Mon Sep 17 00:00:00 2001 From: Mario Ruci Date: Thu, 23 Jul 2026 11:06:23 +0200 Subject: [PATCH 2/3] Adjust test Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- core/upgrader/impl/src/services/disaster_recovery.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/core/upgrader/impl/src/services/disaster_recovery.rs b/core/upgrader/impl/src/services/disaster_recovery.rs index 912fb6ade..6c425ecec 100644 --- a/core/upgrader/impl/src/services/disaster_recovery.rs +++ b/core/upgrader/impl/src/services/disaster_recovery.rs @@ -590,13 +590,16 @@ mod tests { let mut committee = mock_committee(); committee.quorum = 0; - dr.set_committee(committee.clone()) + let err = dr + .set_committee(committee.clone()) .expect_err("quorum of 0 must be rejected"); + assert_eq!(err.code, "INVALID_QUORUM".to_string()); committee.quorum = committee.users.len() as u16 + 1; - dr.set_committee(committee.clone()) + let err = dr + .set_committee(committee.clone()) .expect_err("quorum greater than the number of members must be rejected"); - + assert_eq!(err.code, "INVALID_QUORUM".to_string()); committee.quorum = committee.users.len() as u16; dr.set_committee(committee.clone()) .expect("quorum equal to the number of members must be accepted"); From 9443125e7fd25c3fa31d8c8f8120db471d3a68f7 Mon Sep 17 00:00:00 2001 From: Mario Ruci Date: Thu, 23 Jul 2026 11:09:05 +0200 Subject: [PATCH 3/3] fix(upgrader): validate DR quorum against unique committee members Addresses review feedback: the quorum was checked against committee.users.len(), but membership is deduplicated by user id (recovery requests are keyed by user id and retained via a HashSet of ids). Duplicate entries could let a quorum larger than the number of unique voters pass validation, making it impossible to reach. Validate against the unique member count (reusing the set already built for request retention). Co-Authored-By: Claude Opus 4.8 --- .../impl/src/services/disaster_recovery.rs | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/core/upgrader/impl/src/services/disaster_recovery.rs b/core/upgrader/impl/src/services/disaster_recovery.rs index 6c425ecec..92e39bba1 100644 --- a/core/upgrader/impl/src/services/disaster_recovery.rs +++ b/core/upgrader/impl/src/services/disaster_recovery.rs @@ -124,9 +124,13 @@ impl DisasterRecoveryService { return Err(UpgraderApiError::EmptyCommittee.into()); } - // A quorum of 0 would approve disaster recovery with no votes; a quorum larger than the - // committee could never be met. Both are misconfigurations that must be rejected. - if committee.quorum == 0 || committee.quorum as usize > committee.users.len() { + // Membership is deduplicated by user id elsewhere in the service (and requests are keyed by + // user id), so the quorum must be validated against the number of unique voters, not the + // length of the (possibly duplicate-containing) users vec. A quorum of 0 would approve + // disaster recovery with no votes; a quorum larger than the unique membership could never + // be met. Both are misconfigurations that must be rejected. + let committee_set: HashSet<_> = committee.users.iter().map(|user| user.id).collect(); + if committee.quorum == 0 || committee.quorum as usize > committee_set.len() { return Err(UpgraderApiError::InvalidQuorum.into()); } @@ -134,7 +138,6 @@ impl DisasterRecoveryService { // only retain recovery requests from committee members // who are in the new committee - let committee_set: HashSet<_> = committee.users.iter().map(|user| user.id).collect(); value .recovery_requests .retain(|request| committee_set.contains(&request.user_id)); @@ -607,6 +610,16 @@ mod tests { committee.quorum = 1; dr.set_committee(committee) .expect("a positive quorum within the committee size must be accepted"); + + // Duplicate members must not inflate the effective quorum ceiling: 3 vec entries but only + // 2 unique voters, so a quorum of 3 is unreachable and must be rejected. + let mut committee = mock_committee(); + committee.users[2].id = committee.users[1].id; + committee.quorum = 3; + let err = dr + .set_committee(committee) + .expect_err("quorum greater than the number of unique members must be rejected"); + assert_eq!(err.code, "INVALID_QUORUM".to_string()); } #[tokio::test]