From 7aa85e8b51744c27e91072dd839ce4daa9c27be3 Mon Sep 17 00:00:00 2001 From: brave-builds Date: Tue, 9 Aug 2022 17:39:07 +0000 Subject: [PATCH] Uplift of #14539 (squashed) to beta --- .../renderer/js_ethereum_provider.cc | 52 ++++++++++++---- .../resources/ethereum_provider.js | 8 --- renderer/test/BUILD.gn | 1 + .../test/js_ethereum_provider_browsertest.cc | 62 +++++++++++++++++++ 4 files changed, 104 insertions(+), 19 deletions(-) diff --git a/components/brave_wallet/renderer/js_ethereum_provider.cc b/components/brave_wallet/renderer/js_ethereum_provider.cc index 03190cb9e537..7223d35fe96b 100644 --- a/components/brave_wallet/renderer/js_ethereum_provider.cc +++ b/components/brave_wallet/renderer/js_ethereum_provider.cc @@ -177,7 +177,19 @@ void JSEthereumProvider::CreateEthereumObject( .Check(); } ethereum_obj - ->Set(context, gin::StringToSymbol(isolate, "_metamask"), metamask_obj) + ->DefineOwnProperty(context, + gin::StringToSymbol(isolate, "isBraveWallet"), + v8::True(isolate), v8::ReadOnly) + .Check(); + // isMetaMask shuld be writable because of + // https://github.com/brave/brave-browser/issues/22213 + ethereum_obj + ->DefineOwnProperty(context, gin::StringToSymbol(isolate, "isMetaMask"), + v8::True(isolate)) + .Check(); + ethereum_obj + ->DefineOwnProperty(context, gin::StringToSymbol(isolate, "_metamask"), + metamask_obj, v8::ReadOnly) .Check(); BindFunctionsToObject(isolate, context, ethereum_obj, metamask_obj); UpdateAndBindJSProperties(isolate, context, ethereum_obj); @@ -218,9 +230,22 @@ void JSEthereumProvider::UpdateAndBindJSProperties( v8::Local context, v8::Local ethereum_obj) { v8::Local undefined(v8::Undefined(isolate)); + + // Skip window.ethereum is not owned by Brave. Note this is not accurate since + // anyone can fake window.ethereum.isBraveWallet. We don't need this kind of + // check when we migrate to gin::Wrappable because we won't need to grab + // window.ethereum from global space to update properties and provider object + // owned property getter will be bound to a native function. + v8::Local is_brave_wallet; + if (!GetProperty(context, ethereum_obj, u"isBraveWallet") + .ToLocal(&is_brave_wallet) || + !is_brave_wallet->BooleanValue(isolate)) { + return; + } + ethereum_obj - ->Set(context, gin::StringToSymbol(isolate, "chainId"), - gin::StringToV8(isolate, chain_id_)) + ->DefineOwnProperty(context, gin::StringToSymbol(isolate, "chainId"), + gin::StringToV8(isolate, chain_id_), v8::ReadOnly) .Check(); // We have no easy way to convert a uin256 to a decimal number string yet @@ -231,13 +256,16 @@ void JSEthereumProvider::UpdateAndBindJSProperties( chain_id_uint256 <= (uint256_t)std::numeric_limits::max()) { uint64_t networkVersion = (uint64_t)chain_id_uint256; ethereum_obj - ->Set(context, gin::StringToSymbol(isolate, "networkVersion"), - gin::StringToV8(isolate, std::to_string(networkVersion))) + ->DefineOwnProperty( + context, gin::StringToSymbol(isolate, "networkVersion"), + gin::StringToV8(isolate, std::to_string(networkVersion)), + v8::ReadOnly) .Check(); } else { ethereum_obj - ->Set(context, gin::StringToSymbol(isolate, "networkVersion"), - undefined) + ->DefineOwnProperty(context, + gin::StringToSymbol(isolate, "networkVersion"), + undefined, v8::ReadOnly) .Check(); } @@ -245,13 +273,15 @@ void JSEthereumProvider::UpdateAndBindJSProperties( // first connected account that was given permissions. if (first_allowed_account_.empty()) { ethereum_obj - ->Set(context, gin::StringToSymbol(isolate, "selectedAddress"), - undefined) + ->DefineOwnProperty(context, + gin::StringToSymbol(isolate, "selectedAddress"), + undefined, v8::ReadOnly) .Check(); } else { ethereum_obj - ->Set(context, gin::StringToSymbol(isolate, "selectedAddress"), - gin::StringToV8(isolate, first_allowed_account_)) + ->DefineOwnProperty( + context, gin::StringToSymbol(isolate, "selectedAddress"), + gin::StringToV8(isolate, first_allowed_account_), v8::ReadOnly) .Check(); } } diff --git a/components/brave_wallet/resources/ethereum_provider.js b/components/brave_wallet/resources/ethereum_provider.js index 67b66f2b106f..6ddb2fbb9e4f 100644 --- a/components/brave_wallet/resources/ethereum_provider.js +++ b/components/brave_wallet/resources/ethereum_provider.js @@ -25,14 +25,6 @@ removeAllListeners: { value: BraveWeb3ProviderEventEmitter.removeAllListeners, writable: false - }, - isMetaMask: { - value: true, - writable: true // https://github.com/brave/brave-browser/issues/22213 - }, - isBraveWallet: { - value: true, - writable: false } }) diff --git a/renderer/test/BUILD.gn b/renderer/test/BUILD.gn index 4ac0b247295d..14be5286b609 100644 --- a/renderer/test/BUILD.gn +++ b/renderer/test/BUILD.gn @@ -21,6 +21,7 @@ source_set("browser_tests") { deps = [ "//base/test:test_support", + "//brave/browser/brave_wallet", "//brave/common", "//brave/components/brave_wallet/browser", "//brave/components/brave_wallet/browser:utils", diff --git a/renderer/test/js_ethereum_provider_browsertest.cc b/renderer/test/js_ethereum_provider_browsertest.cc index 6dc897ba89c4..bc20acfb9bae 100644 --- a/renderer/test/js_ethereum_provider_browsertest.cc +++ b/renderer/test/js_ethereum_provider_browsertest.cc @@ -5,7 +5,9 @@ #include "base/path_service.h" #include "base/strings/stringprintf.h" +#include "brave/browser/brave_wallet/json_rpc_service_factory.h" #include "brave/components/brave_wallet/browser/brave_wallet_utils.h" +#include "brave/components/brave_wallet/browser/json_rpc_service.h" #include "brave/components/constants/brave_paths.h" #include "build/build_config.h" #include "chrome/browser/ui/browser.h" @@ -23,6 +25,15 @@ #include "url/gurl.h" namespace { +std::string NonWriteableScriptProperty(const std::string& property) { + return base::StringPrintf( + R"(window.ethereum.%s = "brave" + if (window.ethereum.%s === "brave") + window.domAutomationController.send(false) + else + window.domAutomationController.send(true))", + property.c_str(), property.c_str()); +} std::string NonWriteableScriptMethod(const std::string& provider, const std::string& method) { return base::StringPrintf( @@ -35,6 +46,9 @@ std::string NonWriteableScriptMethod(const std::string& provider, } } // namespace +// TODO(darkdh): Move this browser test to //brave/browser/brave_wallet/ because +// it has layer violation (//chrome/browser, +// //brave/components/brave_wallet/browser and //brave/browser) class JSEthereumProviderBrowserTest : public InProcessBrowserTest { public: JSEthereumProviderBrowserTest() @@ -86,6 +100,11 @@ class JSEthereumProviderBrowserTest : public InProcessBrowserTest { ASSERT_TRUE(content::WaitForLoadStop(web_contents())); } + brave_wallet::JsonRpcService* GetJsonRpcService() { + return brave_wallet::JsonRpcServiceFactory::GetInstance() + ->GetServiceForContext(browser()->profile()); + } + protected: net::test_server::EmbeddedTestServerHandle test_server_handle_; content::ContentMockCertVerifier mock_cert_verifier_; @@ -155,6 +174,14 @@ IN_PROC_BROWSER_TEST_F(JSEthereumProviderBrowserTest, NonWritable) { const GURL url = https_server_.GetURL("/simple.html"); ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), url)); + // window.ethereum.* (properties) + for (const std::string& property : {"isBraveWallet", "_metamask", "chainId", + "networkVersion", "selectedAddress"}) { + SCOPED_TRACE(property); + auto result = EvalJs(web_contents(), NonWriteableScriptProperty(property), + content::EXECUTE_SCRIPT_USE_MANUAL_REPLY); + EXPECT_EQ(base::Value(true), result.value) << result.error; + } // window.ethereum.* (methods) for (const std::string& method : {"on", "emit", "removeListener", "removeAllListeners", "request", @@ -202,6 +229,41 @@ IN_PROC_BROWSER_TEST_F(JSEthereumProviderBrowserTest, NonConfigurable) { EXPECT_TRUE(content::EvalJs(main_frame(), overwrite).ExtractBool()); } +IN_PROC_BROWSER_TEST_F(JSEthereumProviderBrowserTest, OnlyWriteOwnProperty) { + brave_wallet::SetDefaultEthereumWallet( + browser()->profile()->GetPrefs(), + brave_wallet::mojom::DefaultWallet::BraveWallet); + const GURL url = https_server_.GetURL("/simple.html"); + ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), url)); + + const std::string get_chain_id = "window.ethereum.chainId"; + + ASSERT_EQ(content::EvalJs(main_frame(), get_chain_id).ExtractString(), "0x1"); + + GetJsonRpcService()->SetNetwork("0x3", brave_wallet::mojom::CoinType::ETH, + false); + // Needed so ChainChangedEvent observers run + base::RunLoop().RunUntilIdle(); + EXPECT_EQ(content::EvalJs(main_frame(), get_chain_id).ExtractString(), "0x3"); + + brave_wallet::SetDefaultEthereumWallet( + browser()->profile()->GetPrefs(), + brave_wallet::mojom::DefaultWallet::BraveWalletPreferExtension); + ReloadAndWaitForLoadStop(); + ASSERT_EQ(content::EvalJs( + main_frame(), + "window.ethereum = {chainId: '0x89'}; window.ethereum.chainId") + .ExtractString(), + "0x89"); + + GetJsonRpcService()->SetNetwork("0x4", brave_wallet::mojom::CoinType::ETH, + false); + // Needed so ChainChangedEvent observers run + base::RunLoop().RunUntilIdle(); + EXPECT_EQ(content::EvalJs(main_frame(), get_chain_id).ExtractString(), + "0x89"); +} + IN_PROC_BROWSER_TEST_F(JSEthereumProviderBrowserTest, Iframe3P) { constexpr char kEvalEthereumUndefined[] = R"(typeof window.ethereum === 'undefined')";