From 0fb106f55be460f3c5826c9a0846910a6621cbf5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 16 Oct 2020 17:42:21 +0200 Subject: [PATCH] Make `queryStorage` and `storagePairs` unsafe RPC functions The RPC calls can be rather expensive and can easily bring a RPC node in some problems ;) --- client/rpc-api/src/state/error.rs | 2 ++ client/rpc/src/state/mod.rs | 18 ++++++++++-- client/rpc/src/state/tests.rs | 49 ++++++++++++++++++++++++++----- client/service/src/builder.rs | 7 ++++- 4 files changed, 64 insertions(+), 12 deletions(-) diff --git a/client/rpc-api/src/state/error.rs b/client/rpc-api/src/state/error.rs index 2fcca3c343f09..1c22788062c7b 100644 --- a/client/rpc-api/src/state/error.rs +++ b/client/rpc-api/src/state/error.rs @@ -51,6 +51,8 @@ pub enum Error { /// Maximum allowed value max: u32, }, + /// Call to an unsafe RPC was denied. + UnsafeRpcCalled(crate::policy::UnsafeRpcError), } impl std::error::Error for Error { diff --git a/client/rpc/src/state/mod.rs b/client/rpc/src/state/mod.rs index 01c7c5f1eb40c..8573b3cf82551 100644 --- a/client/rpc/src/state/mod.rs +++ b/client/rpc/src/state/mod.rs @@ -28,7 +28,7 @@ use std::sync::Arc; use jsonrpc_pubsub::{typed::Subscriber, SubscriptionId, manager::SubscriptionManager}; use rpc::{Result as RpcResult, futures::{Future, future::result}}; -use sc_rpc_api::state::ReadProof; +use sc_rpc_api::{DenyUnsafe, state::ReadProof}; use sc_client_api::light::{RemoteBlockchain, Fetcher}; use sp_core::{Bytes, storage::{StorageKey, PrefixedStorageKey, StorageData, StorageChangeSet}}; use sp_version::RuntimeVersion; @@ -171,6 +171,7 @@ pub trait StateBackend: Send + Sync + 'static pub fn new_full( client: Arc, subscriptions: SubscriptionManager, + deny_unsafe: DenyUnsafe, ) -> (State, ChildState) where Block: BlockT + 'static, @@ -185,7 +186,7 @@ pub fn new_full( self::state_full::FullState::new(client.clone(), subscriptions.clone()) ); let backend = Box::new(self::state_full::FullState::new(client, subscriptions)); - (State { backend }, ChildState { backend: child_backend }) + (State { backend, deny_unsafe }, ChildState { backend: child_backend }) } /// Create new state API that works on light node. @@ -194,6 +195,7 @@ pub fn new_light>( subscriptions: SubscriptionManager, remote_blockchain: Arc>, fetcher: Arc, + deny_unsafe: DenyUnsafe, ) -> (State, ChildState) where Block: BlockT + 'static, @@ -217,12 +219,14 @@ pub fn new_light>( remote_blockchain, fetcher, )); - (State { backend }, ChildState { backend: child_backend }) + (State { backend, deny_unsafe }, ChildState { backend: child_backend }) } /// State API with subscriptions support. pub struct State { backend: Box>, + /// Whether to deny unsafe calls + deny_unsafe: DenyUnsafe, } impl StateApi for State @@ -249,6 +253,10 @@ impl StateApi for State key_prefix: StorageKey, block: Option, ) -> FutureResult> { + if let Err(err) = self.deny_unsafe.check_if_safe() { + return Box::new(result(Err(err.into()))) + } + self.backend.storage_pairs(block, key_prefix) } @@ -292,6 +300,10 @@ impl StateApi for State from: Block::Hash, to: Option ) -> FutureResult>> { + if let Err(err) = self.deny_unsafe.check_if_safe() { + return Box::new(result(Err(err.into()))) + } + self.backend.query_storage(from, to, keys) } diff --git a/client/rpc/src/state/tests.rs b/client/rpc/src/state/tests.rs index b6677a1f2ffb4..d145ac5e5510b 100644 --- a/client/rpc/src/state/tests.rs +++ b/client/rpc/src/state/tests.rs @@ -32,6 +32,7 @@ use substrate_test_runtime_client::{ sp_consensus::BlockOrigin, runtime, }; +use sc_rpc_api::DenyUnsafe; use sp_runtime::generic::BlockId; use crate::testing::TaskExecutor; use futures::{executor, compat::Future01CompatExt}; @@ -58,7 +59,11 @@ fn should_return_storage() { .add_extra_storage(b":map:acc2".to_vec(), vec![1, 2, 3]) .build(); let genesis_hash = client.genesis_hash(); - let (client, child) = new_full(Arc::new(client), SubscriptionManager::new(Arc::new(TaskExecutor))); + let (client, child) = new_full( + Arc::new(client), + SubscriptionManager::new(Arc::new(TaskExecutor)), + DenyUnsafe::No, + ); let key = StorageKey(KEY.to_vec()); assert_eq!( @@ -96,7 +101,11 @@ fn should_return_child_storage() { .add_child_storage(&child_info, "key", vec![42_u8]) .build()); let genesis_hash = client.genesis_hash(); - let (_client, child) = new_full(client, SubscriptionManager::new(Arc::new(TaskExecutor))); + let (_client, child) = new_full( + client, + SubscriptionManager::new(Arc::new(TaskExecutor)), + DenyUnsafe::No, + ); let child_key = prefixed_storage_key(); let key = StorageKey(b"key".to_vec()); @@ -131,7 +140,11 @@ fn should_return_child_storage() { fn should_call_contract() { let client = Arc::new(substrate_test_runtime_client::new()); let genesis_hash = client.genesis_hash(); - let (client, _child) = new_full(client, SubscriptionManager::new(Arc::new(TaskExecutor))); + let (client, _child) = new_full( + client, + SubscriptionManager::new(Arc::new(TaskExecutor)), + DenyUnsafe::No, + ); assert_matches!( client.call("balanceOf".into(), Bytes(vec![1,2,3]), Some(genesis_hash).into()).wait(), @@ -145,7 +158,11 @@ fn should_notify_about_storage_changes() { { let mut client = Arc::new(substrate_test_runtime_client::new()); - let (api, _child) = new_full(client.clone(), SubscriptionManager::new(Arc::new(TaskExecutor))); + let (api, _child) = new_full( + client.clone(), + SubscriptionManager::new(Arc::new(TaskExecutor)), + DenyUnsafe::No, + ); api.subscribe_storage(Default::default(), subscriber, None.into()); @@ -179,7 +196,11 @@ fn should_send_initial_storage_changes_and_notifications() { { let mut client = Arc::new(substrate_test_runtime_client::new()); - let (api, _child) = new_full(client.clone(), SubscriptionManager::new(Arc::new(TaskExecutor))); + let (api, _child) = new_full( + client.clone(), + SubscriptionManager::new(Arc::new(TaskExecutor)), + DenyUnsafe::No, + ); let alice_balance_key = blake2_256(&runtime::system::balance_of_key(AccountKeyring::Alice.into())); @@ -217,7 +238,11 @@ fn should_send_initial_storage_changes_and_notifications() { #[test] fn should_query_storage() { fn run_tests(mut client: Arc, has_changes_trie_config: bool) { - let (api, _child) = new_full(client.clone(), SubscriptionManager::new(Arc::new(TaskExecutor))); + let (api, _child) = new_full( + client.clone(), + SubscriptionManager::new(Arc::new(TaskExecutor)), + DenyUnsafe::No, + ); let mut add_block = |nonce| { let mut builder = client.new_block(Default::default()).unwrap(); @@ -434,7 +459,11 @@ fn should_split_ranges() { #[test] fn should_return_runtime_version() { let client = Arc::new(substrate_test_runtime_client::new()); - let (api, _child) = new_full(client.clone(), SubscriptionManager::new(Arc::new(TaskExecutor))); + let (api, _child) = new_full( + client.clone(), + SubscriptionManager::new(Arc::new(TaskExecutor)), + DenyUnsafe::No, + ); let result = "{\"specName\":\"test\",\"implName\":\"parity-test\",\"authoringVersion\":1,\ \"specVersion\":2,\"implVersion\":2,\"apis\":[[\"0xdf6acb689907609b\",3],\ @@ -457,7 +486,11 @@ fn should_notify_on_runtime_version_initially() { { let client = Arc::new(substrate_test_runtime_client::new()); - let (api, _child) = new_full(client.clone(), SubscriptionManager::new(Arc::new(TaskExecutor))); + let (api, _child) = new_full( + client.clone(), + SubscriptionManager::new(Arc::new(TaskExecutor)), + DenyUnsafe::No, + ); api.subscribe_runtime_version(Default::default(), subscriber); diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index 0a8111710b30b..3b60db7ec5854 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -753,13 +753,18 @@ fn gen_handler( subscriptions.clone(), remote_blockchain.clone(), on_demand, + deny_unsafe, ); (chain, state, child_state) } else { // Full nodes let chain = sc_rpc::chain::new_full(client.clone(), subscriptions.clone()); - let (state, child_state) = sc_rpc::state::new_full(client.clone(), subscriptions.clone()); + let (state, child_state) = sc_rpc::state::new_full( + client.clone(), + subscriptions.clone(), + deny_unsafe, + ); (chain, state, child_state) };