Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
218 changes: 211 additions & 7 deletions packages/rs-drive-proof-verifier/src/from_request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,9 @@ use drive::query::{
VotePollsByEndDateDriveQuery,
};

use crate::Error;
use crate::{proved_request_limit, Error};

const BINCODE_CONFIG: dpp::bincode::config::Configuration = dpp::bincode::config::standard();

/// Convert a gRPC request into a query object.
///
/// This trait is implemented on Drive queries that can be created from gRPC requests.
Expand Down Expand Up @@ -86,7 +85,7 @@ impl TryFromRequest<GetContestedResourceVoteStateRequest> for ContestedDocumentV
let result = match grpc_request.version.ok_or(Error::EmptyVersion)? {
get_contested_resource_vote_state_request::Version::V0(v) => {
ContestedDocumentVotePollDriveQuery {
limit: v.count.map(|v| v as u16),
limit: Some(proved_request_limit(v.count)?),
Comment thread
PastaPastaPasta marked this conversation as resolved.
vote_poll: ContestedDocumentResourceVotePoll {
contract_id: Identifier::from_bytes(&v.contract_id).map_err(|e| {
Error::RequestError {
Expand Down Expand Up @@ -196,7 +195,7 @@ impl TryFromRequest<GetContestedResourceIdentityVotesRequest>
}
})?,
offset: None,
limit: value.limit.map(|x| x as u16),
limit: Some(proved_request_limit(value.limit)?),
start_at,
order_ascending: value.order_ascending,
})
Expand Down Expand Up @@ -251,7 +250,7 @@ impl TryFromRequest<GetContestedResourceVotersForIdentityRequest>
error: format!("cannot decode contestant_id: {}", e),
}
})?,
limit: v.count.map(|v| v as u16),
limit: Some(proved_request_limit(v.count)?),
offset: None,
start_at: v
.start_at_identifier_info
Expand Down Expand Up @@ -321,7 +320,7 @@ impl TryFromRequest<GetContestedResourcesRequest> for VotePollsByDocumentTypeQue
.transpose()?,
start_index_values: bincode_decode_values(req.start_index_values.iter())?,
end_index_values: bincode_decode_values(req.end_index_values.iter())?,
limit: req.count.map(|v| v as u16),
limit: Some(proved_request_limit(req.count)?),
order_ascending: req.order_ascending,
},
};
Expand Down Expand Up @@ -367,7 +366,7 @@ impl TryFromRequest<GetVotePollsByEndDateRequest> for VotePollsByEndDateDriveQue
end_time: v
.end_time_info
.map(|v| (v.end_time_ms, v.end_time_included)),
limit: v.limit.map(|v| v as u16),
limit: Some(proved_request_limit(v.limit)?),
offset: v.offset.map(|v| v as u16),
order_ascending: v.ascending,
},
Expand Down Expand Up @@ -472,6 +471,15 @@ mod tests {
use dpp::identifier::Identifier;
use dpp::platform_value::Value;

fn sample_vote_poll() -> ContestedDocumentResourceVotePoll {
ContestedDocumentResourceVotePoll {
contract_id: Identifier::from_bytes(&[1u8; 32]).unwrap(),
document_type_name: "domain".to_string(),
index_name: "parentNameAndLabel".to_string(),
index_values: vec![Value::Text("dash".to_string())],
}
}

// ---------------------------------------------------------------
// Helper: to_bytes32
// ---------------------------------------------------------------
Expand Down Expand Up @@ -642,6 +650,202 @@ mod tests {
assert_eq!(roundtripped, id);
}

#[test]
fn test_contested_resources_request_preserves_omitted_limit_when_encoding() {
let query = VotePollsByDocumentTypeQuery {
contract_id: Identifier::from_bytes(&[1u8; 32]).unwrap(),
document_type_name: "domain".to_string(),
index_name: "parentNameAndLabel".to_string(),
start_index_values: vec![Value::Text("dash".to_string())],
end_index_values: vec![],
start_at_value: None,
limit: None,
order_ascending: true,
};

let request = query.try_to_request().expect("request should encode");
let proto::get_contested_resources_request::Version::V0(v0) =
request.version.expect("request should contain a version");

assert_eq!(v0.count, None);
}

#[test]
fn test_contested_resources_request_defaults_limit_when_decoding() {
let request: GetContestedResourcesRequest = GetContestedResourcesRequestV0 {
prove: true,
contract_id: Identifier::from_bytes(&[2u8; 32]).unwrap().to_vec(),
count: None,
document_type_name: "domain".to_string(),
end_index_values: vec![],
start_index_values: bincode_encode_values([&Value::Text("dash".to_string())])
.expect("start index values should encode"),
index_name: "parentNameAndLabel".to_string(),
order_ascending: true,
start_at_value_info: None,
}
.into();

let query =
VotePollsByDocumentTypeQuery::try_from_request(request).expect("request should decode");

assert_eq!(query.limit, Some(crate::DEFAULT_QUERY_LIMIT));
}

#[test]
fn test_contested_resource_vote_state_request_preserves_omitted_limit_when_encoding() {
let query = ContestedDocumentVotePollDriveQuery {
vote_poll: sample_vote_poll(),
result_type: ContestedDocumentVotePollDriveQueryResultType::Documents,
offset: None,
limit: None,
start_at: None,
allow_include_locked_and_abstaining_vote_tally: false,
};

let request = query.try_to_request().expect("request should encode");
let proto::get_contested_resource_vote_state_request::Version::V0(v0) =
request.version.expect("request should contain a version");

assert_eq!(v0.count, None);
}

#[test]
fn test_contested_resource_vote_state_request_defaults_limit_when_decoding() {
let request: GetContestedResourceVoteStateRequest =
proto::get_contested_resource_vote_state_request::GetContestedResourceVoteStateRequestV0 {
prove: true,
contract_id: sample_vote_poll().contract_id.to_vec(),
count: None,
document_type_name: "domain".to_string(),
index_name: "parentNameAndLabel".to_string(),
index_values: bincode_encode_values([&Value::Text("dash".to_string())])
.expect("index values should encode"),
result_type: get_contested_resource_vote_state_request_v0::ResultType::Documents.into(),
start_at_identifier_info: None,
allow_include_locked_and_abstaining_vote_tally: false,
}
.into();

let query = ContestedDocumentVotePollDriveQuery::try_from_request(request)
.expect("request should decode");

assert_eq!(query.limit, Some(crate::DEFAULT_QUERY_LIMIT));
}

#[test]
fn test_contested_resource_identity_votes_request_preserves_omitted_limit_when_encoding() {
let query = ContestedResourceVotesGivenByIdentityQuery {
identity_id: Identifier::from_bytes(&[9u8; 32]).unwrap(),
offset: None,
limit: None,
start_at: None,
order_ascending: true,
};

let request = query.try_to_request().expect("request should encode");
let proto::get_contested_resource_identity_votes_request::Version::V0(v0) =
request.version.expect("request should contain a version");

assert_eq!(v0.limit, None);
}

#[test]
fn test_contested_resource_identity_votes_request_defaults_limit_when_decoding() {
let request: GetContestedResourceIdentityVotesRequest =
proto::get_contested_resource_identity_votes_request::GetContestedResourceIdentityVotesRequestV0 {
prove: true,
identity_id: Identifier::from_bytes(&[8u8; 32]).unwrap().to_vec(),
offset: None,
limit: None,
start_at_vote_poll_id_info: None,
order_ascending: true,
}
.into();

let query = ContestedResourceVotesGivenByIdentityQuery::try_from_request(request)
.expect("request should decode");

assert_eq!(query.limit, Some(crate::DEFAULT_QUERY_LIMIT));
}

#[test]
fn test_contested_resource_voters_for_identity_request_preserves_omitted_limit_when_encoding() {
let query = ContestedDocumentVotePollVotesDriveQuery {
vote_poll: sample_vote_poll(),
contestant_id: Identifier::from_bytes(&[3u8; 32]).unwrap(),
offset: None,
limit: None,
start_at: None,
order_ascending: true,
};

let request = query.try_to_request().expect("request should encode");
let proto::get_contested_resource_voters_for_identity_request::Version::V0(v0) =
request.version.expect("request should contain a version");

assert_eq!(v0.count, None);
}

#[test]
fn test_contested_resource_voters_for_identity_request_defaults_limit_when_decoding() {
let request: GetContestedResourceVotersForIdentityRequest =
proto::get_contested_resource_voters_for_identity_request::GetContestedResourceVotersForIdentityRequestV0 {
prove: true,
contract_id: sample_vote_poll().contract_id.to_vec(),
document_type_name: "domain".to_string(),
index_name: "parentNameAndLabel".to_string(),
index_values: bincode_encode_values([&Value::Text("dash".to_string())])
.expect("index values should encode"),
contestant_id: Identifier::from_bytes(&[4u8; 32]).unwrap().to_vec(),
start_at_identifier_info: None,
count: None,
order_ascending: true,
}
.into();

let query = ContestedDocumentVotePollVotesDriveQuery::try_from_request(request)
.expect("request should decode");

assert_eq!(query.limit, Some(crate::DEFAULT_QUERY_LIMIT));
}

#[test]
fn test_vote_polls_by_end_date_request_preserves_omitted_limit_when_encoding() {
let query = VotePollsByEndDateDriveQuery {
start_time: Some((1000, true)),
end_time: Some((2000, false)),
limit: None,
offset: None,
order_ascending: true,
};

let request = query.try_to_request().expect("request should encode");
let proto::get_vote_polls_by_end_date_request::Version::V0(v0) =
request.version.expect("request should contain a version");

assert_eq!(v0.limit, None);
}

#[test]
fn test_vote_polls_by_end_date_request_defaults_limit_when_decoding() {
let request: GetVotePollsByEndDateRequest =
proto::get_vote_polls_by_end_date_request::GetVotePollsByEndDateRequestV0 {
prove: true,
start_time_info: None,
end_time_info: None,
limit: None,
offset: None,
ascending: true,
}
.into();

let query =
VotePollsByEndDateDriveQuery::try_from_request(request).expect("request should decode");

assert_eq!(query.limit, Some(crate::DEFAULT_QUERY_LIMIT));
}

// ---------------------------------------------------------------
// Error path: SingleDocumentByContender is rejected in try_to_request
// ---------------------------------------------------------------
Expand Down
35 changes: 35 additions & 0 deletions packages/rs-drive-proof-verifier/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,41 @@ pub mod from_request;
/// Implementation of unproved verification
pub mod unproved;

/// Default query limit applied by Platform when proved paginated requests omit `count` or `limit`.
///
/// Proof verification must mirror this behavior; otherwise a valid proof for a default-bounded
/// server query is reconstructed locally as an unbounded query and looks truncated.
pub(crate) use drive::config::DEFAULT_QUERY_LIMIT;

/// Parse a proved request's optional `count`/`limit`, applying Platform's default when omitted.
pub(crate) fn proved_request_limit(limit: Option<u32>) -> Result<u16, Error> {
limit.map_or(Ok(DEFAULT_QUERY_LIMIT), |limit| {
u16::try_from(limit).map_err(|_| Error::RequestError {
error: "query limit exceeds u16::MAX".to_string(),
})
})
}
Comment thread
PastaPastaPasta marked this conversation as resolved.
Comment on lines +24 to +37
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Verifier hardcodes server's runtime-configurable default_query_limit as a compile-time constant

proved_request_limit falls back to the compile-time constant drive::config::DEFAULT_QUERY_LIMIT (= 100 per packages/rs-drive/src/config.rs:16), but every server path this fix mirrors actually reads the runtime field self.config.drive.default_query_limit (e.g. packages/rs-drive-abci/src/query/voting/contested_resources/v0/mod.rs:133, .../contested_resource_vote_state/v0/mod.rs:130, .../contested_resource_voters_for_identity/v0/mod.rs:124, .../contested_resource_identity_votes/v0/mod.rs:44, .../vote_polls_by_end_date_query/v0/mod.rs:63, .../validator_queries/proposed_block_counts_by_range/v0/mod.rs:35, .../token_queries/token_pre_programmed_distributions/v0/mod.rs:40, .../group_queries/group_actions/v0/mod.rs:60). DriveConfig::default_query_limit is a serde-deserialized field on DriveConfig (packages/rs-drive/src/config.rs:44-52), so any node operator who overrides it in drive config will produce proofs the SDK reconstructs against the wrong range — the same Proof is missing data for query range failure this PR is meant to eliminate, just silent and undebuggable on the client side. This isn't a soundness issue (no proof forgery), but it is a liveness/availability footgun. Recommend either (i) treating default_query_limit as a consensus-pinned protocol constant via PlatformVersion (so it is impossible to misconfigure) and asserting agreement at config load, or (ii) returning the effective limit in ResponseMetadata and having the SDK use the server's declared value. At minimum, add a doc-comment invariant on DEFAULT_QUERY_LIMIT documenting that it must equal DriveConfig::default_query_limit for every node on the network.

source: ['claude-security-auditor', 'claude-rust-quality']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-drive-proof-verifier/src/lib.rs`:
- [SUGGESTION] lines 24-37: Verifier hardcodes server's runtime-configurable `default_query_limit` as a compile-time constant
  `proved_request_limit` falls back to the compile-time constant `drive::config::DEFAULT_QUERY_LIMIT` (`= 100` per `packages/rs-drive/src/config.rs:16`), but every server path this fix mirrors actually reads the runtime field `self.config.drive.default_query_limit` (e.g. `packages/rs-drive-abci/src/query/voting/contested_resources/v0/mod.rs:133`, `.../contested_resource_vote_state/v0/mod.rs:130`, `.../contested_resource_voters_for_identity/v0/mod.rs:124`, `.../contested_resource_identity_votes/v0/mod.rs:44`, `.../vote_polls_by_end_date_query/v0/mod.rs:63`, `.../validator_queries/proposed_block_counts_by_range/v0/mod.rs:35`, `.../token_queries/token_pre_programmed_distributions/v0/mod.rs:40`, `.../group_queries/group_actions/v0/mod.rs:60`). `DriveConfig::default_query_limit` is a serde-deserialized field on `DriveConfig` (`packages/rs-drive/src/config.rs:44-52`), so any node operator who overrides it in `drive` config will produce proofs the SDK reconstructs against the wrong range — the same `Proof is missing data for query range` failure this PR is meant to eliminate, just silent and undebuggable on the client side. This isn't a soundness issue (no proof forgery), but it is a liveness/availability footgun. Recommend either (i) treating `default_query_limit` as a consensus-pinned protocol constant via `PlatformVersion` (so it is impossible to misconfigure) and asserting agreement at config load, or (ii) returning the effective limit in `ResponseMetadata` and having the SDK use the server's declared value. At minimum, add a doc-comment invariant on `DEFAULT_QUERY_LIMIT` documenting that it must equal `DriveConfig::default_query_limit` for every node on the network.

Comment thread
PastaPastaPasta marked this conversation as resolved.
Comment thread
PastaPastaPasta marked this conversation as resolved.

// Needed for #[derive(PlatformSerialize, PlatformDeserialize)]
#[cfg(feature = "mocks")]
use dpp::serialization;

#[cfg(test)]
mod tests {
use super::{proved_request_limit, DEFAULT_QUERY_LIMIT};

#[test]
fn proved_request_limit_defaults_when_omitted() {
assert_eq!(proved_request_limit(None).unwrap(), DEFAULT_QUERY_LIMIT);
}

#[test]
fn proved_request_limit_preserves_explicit_value() {
assert_eq!(proved_request_limit(Some(7)).unwrap(), 7);
}

#[test]
fn proved_request_limit_rejects_u32_overflow() {
assert!(proved_request_limit(Some(u16::MAX as u32 + 1)).is_err());
}
Comment thread
PastaPastaPasta marked this conversation as resolved.
Comment thread
PastaPastaPasta marked this conversation as resolved.
}
4 changes: 2 additions & 2 deletions packages/rs-drive-proof-verifier/src/proof.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ pub mod token_total_supply;

use crate::from_request::TryFromRequest;
use crate::verify::verify_tenderdash_proof;
use crate::{types::*, ContextProvider, DataContractProvider, Error};
use crate::{proved_request_limit, types::*, ContextProvider, DataContractProvider, Error};
use dapi_grpc::platform::v0::get_evonodes_proposed_epoch_blocks_by_range_request::get_evonodes_proposed_epoch_blocks_by_range_request_v0::Start;
use dapi_grpc::platform::v0::get_identities_contract_keys_request::GetIdentitiesContractKeysRequestV0;
use dapi_grpc::platform::v0::get_path_elements_request::GetPathElementsRequestV0;
Expand Down Expand Up @@ -2362,7 +2362,7 @@ impl FromProof<platform::GetEvonodesProposedEpochBlocksByRangeRequest> for Propo
Some(index) => try_u32_to_u16(index)?,
None => try_u32_to_u16(mtd.epoch)?,
};
let checked_limit = limit.map(try_u32_to_u16).transpose()?;
let checked_limit = Some(proved_request_limit(limit)?);

let (root_hash, proposer_block_counts) = Drive::verify_epoch_proposers(
&proof.grovedb_proof,
Expand Down
6 changes: 3 additions & 3 deletions packages/rs-drive-proof-verifier/src/proof/groups.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::error::MapGroveDbError;
use crate::types::groups::{GroupActionSigners, GroupActions, Groups};
use crate::verify::verify_tenderdash_proof;
use crate::{ContextProvider, Error, FromProof};
use crate::{proved_request_limit, ContextProvider, Error, FromProof};
use dapi_grpc::platform::v0::{
get_group_action_signers_request, get_group_actions_request, get_group_info_request,
get_group_infos_request, GetGroupActionSignersRequest, GetGroupActionSignersResponse,
Expand Down Expand Up @@ -109,7 +109,7 @@ impl FromProof<GetGroupInfosRequest> for Groups {
)
});

let count = v0.count.map(|count| count as u16);
let count = Some(proved_request_limit(v0.count)?);

(contract_id, start_group_contract_position, count)
}
Expand Down Expand Up @@ -195,7 +195,7 @@ impl FromProof<GetGroupActionsRequest> for GroupActions {
let group_contract_position =
v0.group_contract_position as GroupContractPosition;

let count = v0.count.map(|count| count as u16);
let count = Some(proved_request_limit(v0.count)?);

let status = GroupActionStatus::try_from(v0.status).map_err(|error| {
Error::RequestError {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::error::MapGroveDbError;
use crate::verify::verify_tenderdash_proof;
use crate::{types::TokenPreProgrammedDistributions, ContextProvider, Error};
use crate::{proved_request_limit, types::TokenPreProgrammedDistributions, ContextProvider, Error};
use dapi_grpc::platform::v0::{
get_token_pre_programmed_distributions_request, GetTokenPreProgrammedDistributionsRequest,
GetTokenPreProgrammedDistributionsResponse, Proof, ResponseMetadata,
Expand Down Expand Up @@ -68,14 +68,7 @@ impl FromProof<GetTokenPreProgrammedDistributionsRequest> for TokenPreProgrammed
None => None,
};

let limit = req_v0
.limit
.map(|l| {
u16::try_from(l).map_err(|_| Error::RequestError {
error: "limit exceeds u16::MAX".into(),
})
})
.transpose()?;
let limit = Some(proved_request_limit(req_v0.limit)?);

let metadata = response
.metadata()
Expand Down
Loading
Loading