Check isBraveWallet before updating properties as mitigation before gin::Wrappable migration. (uplift to 1.43.x)

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<v8::Context> context,
     v8::Local<v8::Object> ethereum_obj) {
   v8::Local<v8::Primitive> 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<v8::Value> 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,27 +256,32 @@ void JSEthereumProvider::UpdateAndBindJSProperties(
       chain_id_uint256 <= (uint256_t)std::numeric_limits<uint64_t>::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();
   }
 
   // Note this does not return the selected account, but it returns the
   // 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();
   }
 }

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
     }
   })
 

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",

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')";