Skip to content

Commit

Permalink
Reject Solana SignTransaction/SignAllTransactions when blockhash is i…
Browse files Browse the repository at this point in the history 
…nvalid
  • Loading branch information
@yrliou
yrliou committed Apr 22, 2023
1 parent ac70214 commit bf6ffdf
Show file tree
Hide file tree
Showing 9 changed files with 246 additions and 50 deletions.
12 changes: 9 additions & 3 deletions browser/brave_content_browser_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -339,14 +339,20 @@ void MaybeBindSolanaProvider(
return;
}

auto* json_rpc_service =
brave_wallet::JsonRpcServiceFactory::GetServiceForContext(
frame_host->GetBrowserContext());
if (!json_rpc_service) {
return;
}

content::WebContents* web_contents =
content::WebContents::FromRenderFrameHost(frame_host);
mojo::MakeSelfOwnedReceiver(
std::make_unique<brave_wallet::SolanaProviderImpl>(
keyring_service, brave_wallet_service, tx_service,
keyring_service, brave_wallet_service, tx_service, json_rpc_service,
std::make_unique<brave_wallet::BraveWalletProviderDelegateImpl>(
web_contents, frame_host),
user_prefs::UserPrefs::Get(web_contents->GetBrowserContext())),
web_contents, frame_host)),
std::move(receiver));
}

Expand Down
79 changes: 63 additions & 16 deletions browser/brave_wallet/solana_provider_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@
#include "base/memory/raw_ptr.h"
#include "base/path_service.h"
#include "base/strings/strcat.h"
#include "base/strings/string_util.h"
#include "base/test/bind.h"
#include "base/test/scoped_feature_list.h"
#include "base/test/values_test_util.h"
#include "brave/browser/brave_wallet/brave_wallet_service_factory.h"
#include "brave/browser/brave_wallet/brave_wallet_tab_helper.h"
#include "brave/browser/brave_wallet/json_rpc_service_factory.h"
Expand Down Expand Up @@ -228,21 +230,6 @@ constexpr char kSignedTxArrayStrV0[] =
"62,117,14,95,59,44,129,146,205,25,85,231,59,33,111,45,217,138,53,56,53,66,"
"159,240,201,66,1,1,0";

std::unique_ptr<net::test_server::HttpResponse> HandleRequest(
const net::test_server::HttpRequest& request) {
std::unique_ptr<net::test_server::BasicHttpResponse> http_response(
new net::test_server::BasicHttpResponse());
http_response->set_code(net::HTTP_OK);
http_response->set_content_type("text/html");
std::string request_path = request.GetURL().path();
http_response->set_content(R"({
"jsonrpc": "2.0",
"id": 1,
"result": "ns1aBL6AowxpiPzQL3ZeBK1RpCSLq1VfhqNw9KFSsytayARYdYrqrmbmhaizUTTkT4SXEnjnbVmPBrie3o9yuyB"
})");
return std::move(http_response);
}

// signMessage
constexpr char kMessage[] = "bravey baby!";
constexpr char kEncodedMessage[] = "98,114,97,118,121,32,98,97,98,121,33";
Expand Down Expand Up @@ -305,6 +292,13 @@ bool WaitForWalletBubble(content::WebContents* web_contents) {

return tab_helper->IsShowingBubble();
}

void CloseWalletBubble(content::WebContents* web_contents) {
auto* tab_helper =
brave_wallet::BraveWalletTabHelper::FromWebContents(web_contents);
tab_helper->CloseBubble();
}

} // namespace

class SolanaProviderTest : public InProcessBrowserTest {
Expand Down Expand Up @@ -348,7 +342,8 @@ class SolanaProviderTest : public InProcessBrowserTest {
tx_service_ = TxServiceFactory::GetServiceForContext(browser()->profile());
tx_service_->AddObserver(observer()->GetReceiver());

StartRPCServer(base::BindRepeating(&HandleRequest));
StartRPCServer(base::BindRepeating(&SolanaProviderTest::HandleRequest,
base::Unretained(this)));

// load solana web3 script
if (g_provider_solana_web3_script->empty()) {
Expand All @@ -357,6 +352,38 @@ class SolanaProviderTest : public InProcessBrowserTest {
}
}

std::unique_ptr<net::test_server::HttpResponse> HandleRequest(
const net::test_server::HttpRequest& request) {
std::unique_ptr<net::test_server::BasicHttpResponse> http_response(
new net::test_server::BasicHttpResponse());
http_response->set_code(net::HTTP_OK);
http_response->set_content_type("text/html");
std::string request_path = request.GetURL().path();

auto body = base::test::ParseJsonDict(request.content);
auto* method = body.FindString("method");
EXPECT_TRUE(method);
if (*method == "isBlockhashValid") {
std::string reply = R"({
"jsonrpc": "2.0",
"id": 1,
"result": {
"value": {valid}
}
})";
base::ReplaceFirstSubstringAfterOffset(
&reply, 0, "{valid}", mock_blockhash_is_valid_ ? "true" : "false");
http_response->set_content(reply);
} else {
http_response->set_content(R"({
"jsonrpc": "2.0",
"id": 1,
"result": "ns1aBL6AowxpiPzQL3ZeBK1RpCSLq1VfhqNw9KFSsytayARYdYrqrmbmhaizUTTkT4SXEnjnbVmPBrie3o9yuyB"
})");
}
return std::move(http_response);
}

void StartRPCServer(
const net::EmbeddedTestServer::HandleRequestCallback& callback) {
https_server_for_rpc()->SetSSLConfig(net::EmbeddedTestServer::CERT_OK);
Expand Down Expand Up @@ -534,12 +561,14 @@ class SolanaProviderTest : public InProcessBrowserTest {

void CallSolanaSignMessage(const std::string& message,
const std::string& encoding) {
CloseWalletBubble(web_contents());
ASSERT_TRUE(ExecJs(web_contents(),
base::StringPrintf(R"(solanaSignMessage('%s', '%s'))",
message.c_str(), encoding.c_str())));
}

void CallSolanaRequest(const std::string& json) {
CloseWalletBubble(web_contents());
ASSERT_TRUE(
ExecJs(web_contents(),
base::StringPrintf(R"(solanaRequest(%s))", json.c_str())));
Expand All @@ -557,6 +586,7 @@ class SolanaProviderTest : public InProcessBrowserTest {
const std::string& pubkey = "",
const std::string& signature_array_string = "",
bool v0 = false) {
CloseWalletBubble(web_contents());
const std::string script =
pubkey.empty()
? base::StringPrintf(
Expand All @@ -583,6 +613,7 @@ class SolanaProviderTest : public InProcessBrowserTest {
const std::string& pubkey = "",
const std::string& signature_array_string = "",
bool v0 = false) {
CloseWalletBubble(web_contents());
const std::string script =
pubkey.empty()
? base::StringPrintf(
Expand All @@ -604,6 +635,7 @@ class SolanaProviderTest : public InProcessBrowserTest {
const std::string& pubkey = "",
const std::string& signature_array_string = "",
bool v0 = false) {
CloseWalletBubble(web_contents());
const std::string script =
pubkey.empty()
? base::StringPrintf(
Expand Down Expand Up @@ -672,6 +704,7 @@ class SolanaProviderTest : public InProcessBrowserTest {
protected:
raw_ptr<BraveWalletService> brave_wallet_service_ = nullptr;
raw_ptr<KeyringService> keyring_service_ = nullptr;
bool mock_blockhash_is_valid_ = true;

private:
TestTxServiceObserver observer_;
Expand Down Expand Up @@ -1133,6 +1166,13 @@ IN_PROC_BROWSER_TEST_F(SolanaProviderTest, SignTransaction) {
true, request_index++, nullptr, absl::nullopt);
WaitForResultReady();
EXPECT_EQ(GetSignTransactionResult(), kSignedTxArrayStrV0);

// Test blockhash is invalid.
mock_blockhash_is_valid_ = false;
CallSolanaSignTransaction(kUnsignedTxArrayStr);
WaitForResultReady();
EXPECT_EQ(GetSignTransactionResult(),
l10n_util::GetStringUTF8(IDS_WALLET_INVALID_BLOCKHASH_ERROR));
}

IN_PROC_BROWSER_TEST_F(SolanaProviderTest, SignAllTransactions) {
Expand Down Expand Up @@ -1184,6 +1224,13 @@ IN_PROC_BROWSER_TEST_F(SolanaProviderTest, SignAllTransactions) {
true, request_index++, absl::nullopt, absl::nullopt);
WaitForResultReady();
EXPECT_EQ(GetSignAllTransactionsResult(), "success");

// Test blockhash is invalid.
mock_blockhash_is_valid_ = false;
CallSolanaSignAllTransactions(kUnsignedTxArrayStr, kSignedTxArrayStr);
WaitForResultReady();
EXPECT_EQ(GetSignAllTransactionsResult(),
l10n_util::GetStringUTF8(IDS_WALLET_INVALID_BLOCKHASH_ERROR));
}

IN_PROC_BROWSER_TEST_F(SolanaProviderTest, Request) {
Expand Down
49 changes: 39 additions & 10 deletions browser/brave_wallet/solana_provider_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@
#include "content/public/test/browser_task_environment.h"
#include "content/public/test/test_web_contents_factory.h"
#include "content/test/test_web_contents.h"
#include "services/data_decoder/public/cpp/test_support/in_process_data_decoder.h"
#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
#include "services/network/test/test_url_loader_factory.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "ui/base/l10n/l10n_util.h"

Expand Down Expand Up @@ -98,7 +101,10 @@ class TestEventsListener : public mojom::SolanaEventsListener {

class SolanaProviderImplUnitTest : public testing::Test {
public:
SolanaProviderImplUnitTest() {
SolanaProviderImplUnitTest()
: shared_url_loader_factory_(
base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
&url_loader_factory_)) {
feature_list_.InitAndEnableFeature(
brave_wallet::features::kBraveWalletSolanaFeature);
}
Expand All @@ -121,6 +127,18 @@ class SolanaProviderImplUnitTest : public testing::Test {
permissions::PermissionRequestManager::CreateForWebContents(web_contents());
json_rpc_service_ =
JsonRpcServiceFactory::GetServiceForContext(browser_context());
json_rpc_service_->SetAPIRequestHelperForTesting(
shared_url_loader_factory_);

// Return true for checking blockhash.
url_loader_factory_.SetInterceptor(base::BindLambdaForTesting(
[&](const network::ResourceRequest& request) {
url_loader_factory_.ClearResponses();
url_loader_factory_.AddResponse(
request.url.spec(),
R"({"jsonrpc": "2.0", "id": 1, "result": { "value": true }})");
}));

keyring_service_ =
KeyringServiceFactory::GetServiceForContext(browser_context());
brave_wallet_service_ =
Expand All @@ -133,10 +151,9 @@ class SolanaProviderImplUnitTest : public testing::Test {
PermissionManagerFactory::GetInstance()->BuildServiceInstanceFor(
browser_context()))));
provider_ = std::make_unique<SolanaProviderImpl>(
keyring_service_, brave_wallet_service_, tx_service_,
keyring_service_, brave_wallet_service_, tx_service_, json_rpc_service_,
std::make_unique<brave_wallet::BraveWalletProviderDelegateImpl>(
web_contents(), web_contents()->GetPrimaryMainFrame()),
profile_.GetPrefs());
web_contents(), web_contents()->GetPrimaryMainFrame()));
observer_ = std::make_unique<TestEventsListener>();
provider_->Init(observer_->GetReceiver());
}
Expand Down Expand Up @@ -372,9 +389,12 @@ class SolanaProviderImplUnitTest : public testing::Test {
}));

if (run_notify) {
brave_wallet_service_->NotifySignTransactionRequestProcessed(
approve, brave_wallet_service_->sign_transaction_id_ - 1,
std::move(hw_sig), err_in);
brave_wallet_service_->SetSignTransactionRequestAddedCallbackForTesting(
base::BindLambdaForTesting([&]() {
brave_wallet_service_->NotifySignTransactionRequestProcessed(
approve, brave_wallet_service_->sign_transaction_id_ - 1,
std::move(hw_sig), err_in);
}));
}

run_loop.Run();
Expand Down Expand Up @@ -412,9 +432,15 @@ class SolanaProviderImplUnitTest : public testing::Test {
}));

if (run_notify) {
brave_wallet_service_->NotifySignAllTransactionsRequestProcessed(
approve, brave_wallet_service_->sign_all_transactions_id_ - 1,
std::move(hw_sigs), err_in);
brave_wallet_service_
->SetSignAllTransactionsRequestAddedCallbackForTesting(
base::BindLambdaForTesting([&]() {
brave_wallet_service_
->NotifySignAllTransactionsRequestProcessed(
approve,
brave_wallet_service_->sign_all_transactions_id_ - 1,
std::move(hw_sigs), err_in);
}));
}

run_loop.Run();
Expand Down Expand Up @@ -470,6 +496,9 @@ class SolanaProviderImplUnitTest : public testing::Test {
content::TestWebContentsFactory factory_;
TestingProfile profile_;
base::test::ScopedFeatureList feature_list_;
data_decoder::test::InProcessDataDecoder in_process_data_decoder_;
network::TestURLLoaderFactory url_loader_factory_;
scoped_refptr<network::SharedURLLoaderFactory> shared_url_loader_factory_;
};

TEST_F(SolanaProviderImplUnitTest, Connect) {
Expand Down
7 changes: 6 additions & 1 deletion components/brave_wallet/browser/brave_wallet_service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
#include "brave/components/brave_wallet/browser/brave_wallet_service.h"

#include <map>
#include <utility>
#include <vector>

#include "base/metrics/histogram_macros.h"
Expand Down Expand Up @@ -1340,6 +1339,9 @@ void BraveWalletService::AddSignTransactionRequest(
}
sign_transaction_requests_.push_back(std::move(request));
sign_transaction_callbacks_.push_back(std::move(callback));
if (sign_tx_request_added_cb_for_testing_) {
std::move(sign_tx_request_added_cb_for_testing_).Run();
}
}

void BraveWalletService::AddSignAllTransactionsRequest(
Expand All @@ -1350,6 +1352,9 @@ void BraveWalletService::AddSignAllTransactionsRequest(
}
sign_all_transactions_requests_.push_back(std::move(request));
sign_all_transactions_callbacks_.push_back(std::move(callback));
if (sign_all_txs_request_added_cb_for_testing_) {
std::move(sign_all_txs_request_added_cb_for_testing_).Run();
}
}

void BraveWalletService::AddSuggestTokenRequest(
Expand Down
13 changes: 13 additions & 0 deletions components/brave_wallet/browser/brave_wallet_service.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include <memory>
#include <string>
#include <utility>
#include <vector>

#include "base/containers/circular_deque.h"
Expand Down Expand Up @@ -258,6 +259,15 @@ class BraveWalletService : public KeyedService,

BraveWalletP3A* GetBraveWalletP3A();

void SetSignTransactionRequestAddedCallbackForTesting(
base::OnceClosure callback) {
sign_tx_request_added_cb_for_testing_ = std::move(callback);
}
void SetSignAllTransactionsRequestAddedCallbackForTesting(
base::OnceClosure callback) {
sign_all_txs_request_added_cb_for_testing_ = std::move(callback);
}

protected:
// For tests
BraveWalletService();
Expand Down Expand Up @@ -313,6 +323,9 @@ class BraveWalletService : public KeyedService,
void CancelAllGetEncryptionPublicKeyCallbacks();
void CancelAllDecryptCallbacks();

base::OnceClosure sign_tx_request_added_cb_for_testing_;
base::OnceClosure sign_all_txs_request_added_cb_for_testing_;

int sign_message_id_ = 0;
int sign_transaction_id_ = 0;
int sign_all_transactions_id_ = 0;
Expand Down
Loading

0 comments on commit bf6ffdf

Please sign in to comment.