From 7d801b4f72f9e85716fb16521db1b4ded3655797 Mon Sep 17 00:00:00 2001 From: sergerad Date: Thu, 25 Jun 2026 13:09:27 +1200 Subject: [PATCH 1/2] Add bound validation for storage map request --- crates/rpc/src/server/api.rs | 7 +- crates/rpc/src/server/api/get_account.rs | 129 ++++++++++++++++++++--- crates/rpc/src/tests.rs | 19 ++++ crates/utils/src/limiter.rs | 15 +++ 4 files changed, 156 insertions(+), 14 deletions(-) diff --git a/crates/rpc/src/server/api.rs b/crates/rpc/src/server/api.rs index 6fe2cb411..50c44f6de 100644 --- a/crates/rpc/src/server/api.rs +++ b/crates/rpc/src/server/api.rs @@ -27,6 +27,7 @@ use miden_node_utils::limiter::{ QueryParamNoteTagLimit, QueryParamNullifierPrefixLimit, QueryParamStorageMapKeyTotalLimit, + QueryParamStorageMapSlotLimit, }; use miden_node_utils::lru_cache::LruCache; use miden_node_utils::retry::{self, Retryable}; @@ -386,6 +387,7 @@ static RPC_LIMITS: LazyLock = LazyLock::new(|| { use QueryParamNoteTagLimit as NoteTag; use QueryParamNullifierPrefixLimit as NullifierPrefix; use QueryParamStorageMapKeyTotalLimit as StorageMapKeyTotal; + use QueryParamStorageMapSlotLimit as StorageMapSlot; proto::rpc::RpcLimits { endpoints: std::collections::HashMap::from([ @@ -401,7 +403,10 @@ static RPC_LIMITS: LazyLock = LazyLock::new(|| { ("GetNotesById".into(), endpoint_limits(&[(NoteId::PARAM_NAME, NoteId::LIMIT)])), ( "GetAccount".into(), - endpoint_limits(&[(StorageMapKeyTotal::PARAM_NAME, StorageMapKeyTotal::LIMIT)]), + endpoint_limits(&[ + (StorageMapKeyTotal::PARAM_NAME, StorageMapKeyTotal::LIMIT), + (StorageMapSlot::PARAM_NAME, StorageMapSlot::LIMIT), + ]), ), ]), } diff --git a/crates/rpc/src/server/api/get_account.rs b/crates/rpc/src/server/api/get_account.rs index 914623acd..fae90da3d 100644 --- a/crates/rpc/src/server/api/get_account.rs +++ b/crates/rpc/src/server/api/get_account.rs @@ -1,3 +1,5 @@ +use std::collections::HashSet; + use miden_node_proto::domain::account::{ AccountRequest, AccountResponse, @@ -6,7 +8,7 @@ use miden_node_proto::domain::account::{ }; use miden_node_proto::generated as proto; use miden_node_store::GetAccountError; -use miden_node_utils::limiter::QueryParamStorageMapKeyTotalLimit; +use miden_node_utils::limiter::{QueryParamStorageMapKeyTotalLimit, QueryParamStorageMapSlotLimit}; use miden_node_utils::tracing::OpenTelemetrySpanExt; use tonic::Status; use tracing::{Span, debug, info_span}; @@ -35,20 +37,10 @@ impl proto::server::rpc_api::GetAccount for RpcService { span.set_attribute("block.number", block); } - // Validate total storage map key limit before forwarding to store + // Validate storage map request limits before forwarding to store. if let Some(details) = &request.details { let _span = info_span!(target: COMPONENT, "validate_storage_map_keys").entered(); - let total_keys: usize = match &details.storage_request { - AccountStorageRequest::None | AccountStorageRequest::AllStorageMaps => 0, - AccountStorageRequest::Explicit(requests) => requests - .iter() - .filter_map(|request| match &request.slot_data { - SlotData::All => None, - SlotData::MapKeys(items) => Some(items.len()), - }) - .sum(), - }; - check::(total_keys)?; + validate_storage_request(&details.storage_request)?; } let account_data = @@ -60,6 +52,46 @@ impl proto::server::rpc_api::GetAccount for RpcService { // HELPERS // ================================================================================================ +/// Validates the storage map request limits before forwarding it to the store. +/// +/// Only the [`AccountStorageRequest::Explicit`] variant carries a user-controlled list of +/// slots. [`AccountStorageRequest::AllStorageMaps`] is expanded by the store from the account's +/// actual storage layout (and bounded by a response budget), so it needs no validation here. +fn validate_storage_request(storage_request: &AccountStorageRequest) -> Result<(), Status> { + let AccountStorageRequest::Explicit(requests) = storage_request else { + return Ok(()); + }; + + // Bound the number of requested slots. `all-entries` requests are not counted by the per-key + // limit below, so without this an unbounded (or duplicated) list of `all-entries` slots could + // force the store into arbitrarily many forest lookups and database reconstructions, a + // denial-of-service vector. + check::(requests.len())?; + + // Reject duplicate slots: requesting the same slot more than once is redundant and would + // otherwise multiply the store-side work for that slot. + let mut seen = HashSet::with_capacity(requests.len()); + for request in requests { + if !seen.insert(&request.slot_name) { + return Err(Status::invalid_argument(format!( + "duplicate storage map slot in request: {}", + request.slot_name + ))); + } + } + + let total_keys: usize = requests + .iter() + .filter_map(|request| match &request.slot_data { + SlotData::All => None, + SlotData::MapKeys(items) => Some(items.len()), + }) + .sum(); + check::(total_keys)?; + + Ok(()) +} + fn get_account_error_to_status(err: GetAccountError) -> Status { let message = err.to_string(); match err { @@ -71,3 +103,74 @@ fn get_account_error_to_status(err: GetAccountError) -> Status { | GetAccountError::BlockPruned(_) => Status::invalid_argument(message), } } + +// TESTS +// ================================================================================================ + +#[cfg(test)] +mod tests { + use miden_node_proto::domain::account::StorageMapRequest; + use miden_node_utils::limiter::QueryParamLimiter; + use miden_protocol::account::{StorageMapKey, StorageSlotName}; + use tonic::Code; + + use super::*; + + fn slot_request(name: &str, slot_data: SlotData) -> StorageMapRequest { + StorageMapRequest { + slot_name: StorageSlotName::new(name).unwrap(), + slot_data, + } + } + + #[test] + fn none_and_all_storage_maps_requests_are_always_valid() { + validate_storage_request(&AccountStorageRequest::None).unwrap(); + validate_storage_request(&AccountStorageRequest::AllStorageMaps).unwrap(); + } + + #[test] + fn explicit_request_within_limits_is_valid() { + let requests = vec![ + slot_request("a::0", SlotData::All), + slot_request("a::1", SlotData::MapKeys(vec![StorageMapKey::from_index(1)])), + ]; + + validate_storage_request(&AccountStorageRequest::Explicit(requests)).unwrap(); + } + + #[test] + fn too_many_all_entries_slots_are_rejected() { + // `all-entries` requests are not counted by the per-key limit, so this must be caught by + // the per-slot limit. + let requests = (0..=QueryParamStorageMapSlotLimit::LIMIT) + .map(|index| slot_request(&format!("a::{index}"), SlotData::All)) + .collect(); + + let status = validate_storage_request(&AccountStorageRequest::Explicit(requests)) + .expect_err("request exceeding the slot limit must be rejected"); + assert_eq!(status.code(), Code::OutOfRange); + } + + #[test] + fn duplicate_slots_are_rejected() { + let requests = + vec![slot_request("a::0", SlotData::All), slot_request("a::0", SlotData::All)]; + + let status = validate_storage_request(&AccountStorageRequest::Explicit(requests)) + .expect_err("duplicate slot must be rejected"); + assert_eq!(status.code(), Code::InvalidArgument); + } + + #[test] + fn too_many_map_keys_are_rejected() { + let keys: Vec<_> = (0..=QueryParamStorageMapKeyTotalLimit::LIMIT) + .map(|index| StorageMapKey::from_index(index as u32)) + .collect(); + let requests = vec![slot_request("a::0", SlotData::MapKeys(keys))]; + + let status = validate_storage_request(&AccountStorageRequest::Explicit(requests)) + .expect_err("request exceeding the key limit must be rejected"); + assert_eq!(status.code(), Code::OutOfRange); + } +} diff --git a/crates/rpc/src/tests.rs b/crates/rpc/src/tests.rs index 51571d432..e33ccf161 100644 --- a/crates/rpc/src/tests.rs +++ b/crates/rpc/src/tests.rs @@ -29,6 +29,8 @@ use miden_node_utils::limiter::{ QueryParamNoteIdLimit, QueryParamNoteTagLimit, QueryParamNullifierPrefixLimit, + QueryParamStorageMapKeyTotalLimit, + QueryParamStorageMapSlotLimit, }; use miden_protocol::Word; use miden_protocol::account::delta::AccountUpdateDetails; @@ -869,6 +871,23 @@ async fn get_limits_endpoint() { QueryParamNoteIdLimit::PARAM_NAME, QueryParamNoteIdLimit::LIMIT ); + + // Verify GetAccount endpoint advertises both the per-key and per-slot storage map limits. + let get_account = limits.endpoints.get("GetAccount").expect("GetAccount should exist"); + assert_eq!( + get_account.parameters.get(QueryParamStorageMapKeyTotalLimit::PARAM_NAME), + Some(&(QueryParamStorageMapKeyTotalLimit::LIMIT as u32)), + "GetAccount {} limit should be {}", + QueryParamStorageMapKeyTotalLimit::PARAM_NAME, + QueryParamStorageMapKeyTotalLimit::LIMIT + ); + assert_eq!( + get_account.parameters.get(QueryParamStorageMapSlotLimit::PARAM_NAME), + Some(&(QueryParamStorageMapSlotLimit::LIMIT as u32)), + "GetAccount {} limit should be {}", + QueryParamStorageMapSlotLimit::PARAM_NAME, + QueryParamStorageMapSlotLimit::LIMIT + ); } #[tokio::test] diff --git a/crates/utils/src/limiter.rs b/crates/utils/src/limiter.rs index c68b9cf53..3e860da55 100644 --- a/crates/utils/src/limiter.rs +++ b/crates/utils/src/limiter.rs @@ -118,3 +118,18 @@ impl QueryParamLimiter for QueryParamStorageMapKeyTotalLimit { const PARAM_NAME: &str = "storage_map_key"; const LIMIT: usize = 64; } + +/// Used for the following RPC endpoints +/// * `get_account` +/// +/// Capped at the maximum number of storage slots an account can have +/// ([`AccountStorage::MAX_NUM_STORAGE_SLOTS`]). An explicit storage request can never legitimately +/// reference more distinct slots than that, so this bounds the per-request work (forest lookups and +/// database reconstructions) regardless of how many `all-entries` slots are requested. Without this +/// cap, `all-entries` requests are otherwise uncounted by +/// [`QueryParamStorageMapKeyTotalLimit`] and could be repeated without bound. +pub struct QueryParamStorageMapSlotLimit; +impl QueryParamLimiter for QueryParamStorageMapSlotLimit { + const PARAM_NAME: &str = "storage_map_slot"; + const LIMIT: usize = miden_protocol::account::AccountStorage::MAX_NUM_STORAGE_SLOTS; +} From 7d54ae59b90c3e31251aebbe71d03b10fadd2e5b Mon Sep 17 00:00:00 2001 From: sergerad Date: Mon, 29 Jun 2026 09:58:38 +1200 Subject: [PATCH 2/2] Fix param name --- crates/utils/src/limiter.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/utils/src/limiter.rs b/crates/utils/src/limiter.rs index 3e860da55..4f5f6b670 100644 --- a/crates/utils/src/limiter.rs +++ b/crates/utils/src/limiter.rs @@ -130,6 +130,6 @@ impl QueryParamLimiter for QueryParamStorageMapKeyTotalLimit { /// [`QueryParamStorageMapKeyTotalLimit`] and could be repeated without bound. pub struct QueryParamStorageMapSlotLimit; impl QueryParamLimiter for QueryParamStorageMapSlotLimit { - const PARAM_NAME: &str = "storage_map_slot"; + const PARAM_NAME: &str = "storage_maps"; const LIMIT: usize = miden_protocol::account::AccountStorage::MAX_NUM_STORAGE_SLOTS; }