From df1ef781a96ce0cbe6e8fdfa895524f81e68d0ab Mon Sep 17 00:00:00 2001 From: "Brian R. Bondy" Date: Tue, 31 May 2022 15:19:02 -0400 Subject: [PATCH] Allow permission requests within the same eTLD+1 --- .../solana_provider_renderer_browsertest.cc | 9 +++ ...e_wallet_permission_context_browsertest.cc | 21 +++++ .../permission_manager_browsertest.cc | 81 +++++++++++++++++-- .../permissions/chrome_permissions_client.cc | 5 +- .../brave_wallet_permission_context.cc | 20 +++-- .../brave_wallet_render_frame_observer.cc | 10 ++- .../test/js_ethereum_provider_browsertest.cc | 7 ++ 7 files changed, 138 insertions(+), 15 deletions(-) diff --git a/browser/brave_wallet/solana_provider_renderer_browsertest.cc b/browser/brave_wallet/solana_provider_renderer_browsertest.cc index 53bdf1572aa3..c25a84d68ae2 100644 --- a/browser/brave_wallet/solana_provider_renderer_browsertest.cc +++ b/browser/brave_wallet/solana_provider_renderer_browsertest.cc @@ -1033,4 +1033,13 @@ IN_PROC_BROWSER_TEST_F(SolanaProviderRendererTest, Block3PIframe) { iframe_rfh = ChildFrameAt(main_frame, 0); ASSERT_TRUE(iframe_rfh); EXPECT_FALSE(content::EvalJs(iframe_rfh, kEvalSolana).ExtractBool()); + + // same eTLD+1 + GURL iframe_url_etldp1( + embedded_test_server()->GetURL("dapps.please.a.com", "/")); + EXPECT_TRUE( + NavigateIframeToURL(web_contents(browser()), "test", iframe_url_etldp1)); + iframe_rfh = ChildFrameAt(main_frame, 0); + ASSERT_TRUE(iframe_rfh); + EXPECT_FALSE(content::EvalJs(iframe_rfh, kEvalSolana).ExtractBool()); } diff --git a/browser/permissions/brave_wallet_permission_context_browsertest.cc b/browser/permissions/brave_wallet_permission_context_browsertest.cc index f32c2cebf712..00bda0584d19 100644 --- a/browser/permissions/brave_wallet_permission_context_browsertest.cc +++ b/browser/permissions/brave_wallet_permission_context_browsertest.cc @@ -174,4 +174,25 @@ IN_PROC_BROWSER_TEST_F(BraveWalletPermissionContextBrowserTest, EXPECT_TRUE(was_called); } +IN_PROC_BROWSER_TEST_F(BraveWalletPermissionContextBrowserTest, + GetAllowedAccountsAllowETLDP1Iframe) { + std::vector addresses = { + "0xaf5Ad1E10926C0Ee4af4eDAC61DD60E853753f8A", + "0xaf5Ad1E10926C0Ee4af4eDAC61DD60E853753f8B"}; + + GURL top_url(https_server()->GetURL("a.com", "/iframe.html")); + ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), top_url)); + GURL iframe_url(https_server()->GetURL("c.b.a.com", "/")); + EXPECT_TRUE(NavigateIframeToURL(web_contents(), "test", iframe_url)); + + bool was_called = false; + auto* iframe_rfh = ChildFrameAt(web_contents()->GetMainFrame(), 0); + BraveWalletPermissionContext::GetAllowedAccounts( + ContentSettingsType::BRAVE_ETHEREUM, iframe_rfh, addresses, + base::BindOnce(&OnGetAllowedAccountsResult, &was_called, true, + std::vector())); + content::RunAllTasksUntilIdle(); + EXPECT_TRUE(was_called); +} + } // namespace permissions diff --git a/browser/permissions/permission_manager_browsertest.cc b/browser/permissions/permission_manager_browsertest.cc index 8767dbe5b2f9..30e87291a336 100644 --- a/browser/permissions/permission_manager_browsertest.cc +++ b/browser/permissions/permission_manager_browsertest.cc @@ -7,6 +7,7 @@ #include "base/command_line.h" #include "base/feature_list.h" #include "base/memory/raw_ptr.h" +#include "base/test/bind.h" #include "base/test/scoped_feature_list.h" #include "brave/components/brave_wallet/browser/permission_utils.h" #include "brave/components/brave_wallet/common/features.h" @@ -29,6 +30,7 @@ #include "content/public/browser/web_contents.h" #include "content/public/test/browser_test.h" #include "content/public/test/browser_test_utils.h" +#include "content/public/test/content_mock_cert_verifier.h" #include "content/public/test/test_utils.h" #include "net/dns/mock_host_resolver.h" #include "net/test/embedded_test_server/embedded_test_server.h" @@ -81,7 +83,7 @@ class PermissionManagerBrowserTest : public InProcessBrowserTest { void SetUpOnMainThread() override { InProcessBrowserTest::SetUpOnMainThread(); host_resolver()->AddRule("*", "127.0.0.1"); - https_server_.SetSSLConfig(net::EmbeddedTestServer::CERT_TEST_NAMES); + mock_cert_verifier_.mock_cert_verifier()->set_default_result(net::OK); https_server()->ServeFilesFromSourceDirectory(GetChromeTestDataDir()); ASSERT_TRUE(https_server()->Start()); @@ -98,6 +100,21 @@ class PermissionManagerBrowserTest : public InProcessBrowserTest { return HostContentSettingsMapFactory::GetForProfile(browser()->profile()); } + void SetUpCommandLine(base::CommandLine* command_line) override { + InProcessBrowserTest::SetUpCommandLine(command_line); + mock_cert_verifier_.SetUpCommandLine(command_line); + } + + void SetUpInProcessBrowserTestFixture() override { + InProcessBrowserTest::SetUpInProcessBrowserTestFixture(); + mock_cert_verifier_.SetUpInProcessBrowserTestFixture(); + } + + void TearDownInProcessBrowserTestFixture() override { + mock_cert_verifier_.TearDownInProcessBrowserTestFixture(); + InProcessBrowserTest::TearDownInProcessBrowserTestFixture(); + } + content::WebContents* web_contents() { return browser()->tab_strip_model()->GetActiveWebContents(); } @@ -119,11 +136,12 @@ class PermissionManagerBrowserTest : public InProcessBrowserTest { raw_ptr permission_manager_ = nullptr; private: + content::ContentMockCertVerifier mock_cert_verifier_; base::test::ScopedFeatureList scoped_feature_list_; }; IN_PROC_BROWSER_TEST_F(PermissionManagerBrowserTest, RequestPermissions) { - const GURL& url = https_server()->GetURL("a.test", "/empty.html"); + const GURL& url = https_server()->GetURL("a.com", "/empty.html"); ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), url)); auto* permission_request_manager = GetPermissionRequestManager(); @@ -238,7 +256,7 @@ IN_PROC_BROWSER_TEST_F(PermissionManagerBrowserTest, RequestPermissions) { IN_PROC_BROWSER_TEST_F(PermissionManagerBrowserTest, RequestPermissionsTabClosed) { - const GURL& url = https_server()->GetURL("a.test", "/empty.html"); + const GURL& url = https_server()->GetURL("a.com", "/empty.html"); struct { std::vector addresses; @@ -321,9 +339,9 @@ IN_PROC_BROWSER_TEST_F(PermissionManagerBrowserTest, "JDqrvDz8d8tFCADashbUKQDKfJZFobNy13ugN65t1wvV"}, ContentSettingsType::BRAVE_SOLANA}}; - GURL top_url(https_server()->GetURL("a.test", "/iframe.html")); + GURL top_url(https_server()->GetURL("a.com", "/iframe.html")); ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), top_url)); - GURL iframe_url(https_server()->GetURL("b.test", "/")); + GURL iframe_url(https_server()->GetURL("b.com", "/")); EXPECT_TRUE(NavigateIframeToURL(web_contents(), "test", iframe_url)); auto* iframe_rfh = ChildFrameAt(web_contents()->GetMainFrame(), 0); @@ -341,8 +359,59 @@ IN_PROC_BROWSER_TEST_F(PermissionManagerBrowserTest, } } +IN_PROC_BROWSER_TEST_F(PermissionManagerBrowserTest, + RequestPermissionsAllowETLDP1) { + struct { + std::vector addresses; + ContentSettingsType type; + } cases[] = {{{"0xaf5Ad1E10926C0Ee4af4eDAC61DD60E853753f8A", + "0xaf5Ad1E10926C0Ee4af4eDAC61DD60E853753f8B"}, + ContentSettingsType::BRAVE_ETHEREUM}, + {{"BrG44HdsEhzapvs8bEqzvkq4egwevS3fRE6ze2ENo6S8", + "JDqrvDz8d8tFCADashbUKQDKfJZFobNy13ugN65t1wvV"}, + ContentSettingsType::BRAVE_SOLANA}}; + + GURL top_url(https_server()->GetURL("a.com", "/iframe.html")); + ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), top_url)); + GURL iframe_url(https_server()->GetURL("c.b.a.com", "/")); + EXPECT_TRUE(NavigateIframeToURL(web_contents(), "test", iframe_url)); + WaitForLoadStop(web_contents()); + + auto* iframe_rfh = ChildFrameAt(web_contents()->GetMainFrame(), 0); + auto* permission_request_manager = GetPermissionRequestManager(); + + for (const auto& current_case : cases) { + SCOPED_TRACE(testing::PrintToString(current_case.addresses)); + auto observer = std::make_unique( + permission_request_manager); + + base::RunLoop run_loop; + BraveWalletPermissionContext::RequestPermissions( + current_case.type, iframe_rfh, current_case.addresses, + // This callback is not called until after a response is given + base::BindLambdaForTesting( + [&](const std::vector& responses) { + EXPECT_FALSE(responses.empty()); + run_loop.Quit(); + })); + + RequestType request_type = + ContentSettingsTypeToRequestType(current_case.type); + // Needed for the request to be considered to be in progress + content::RunAllTasksUntilIdle(); + EXPECT_TRUE(BraveWalletPermissionContext::HasRequestsInProgress( + iframe_rfh, request_type)); + EXPECT_TRUE(observer->IsShowingBubble()); + EXPECT_FALSE(IsPendingGroupedRequestsEmpty(current_case.type)); + permissions::BraveWalletPermissionContext::Cancel(web_contents()); + run_loop.Run(); + EXPECT_FALSE(BraveWalletPermissionContext::HasRequestsInProgress( + iframe_rfh, request_type)); + } +} + IN_PROC_BROWSER_TEST_F(PermissionManagerBrowserTest, GetCanonicalOrigin) { - const GURL& url = https_server()->GetURL("a.test", "/empty.html"); + const GURL& url = https_server()->GetURL("a.com", "/empty.html"); ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), url)); struct { diff --git a/chromium_src/chrome/browser/permissions/chrome_permissions_client.cc b/chromium_src/chrome/browser/permissions/chrome_permissions_client.cc index 9ec5dd97d519..011bde0df854 100644 --- a/chromium_src/chrome/browser/permissions/chrome_permissions_client.cc +++ b/chromium_src/chrome/browser/permissions/chrome_permissions_client.cc @@ -15,6 +15,7 @@ #include "build/build_config.h" #include "components/permissions/permission_request.h" #include "components/permissions/request_type.h" +#include "net/base/registry_controlled_domains/registry_controlled_domain.h" #include "url/origin.h" #if BUILDFLAG(IS_ANDROID) @@ -39,7 +40,9 @@ bool ChromePermissionsClient::BraveCanBypassEmbeddingOriginCheck( permissions::ContentSettingsTypeToRequestType(type), url::Origin::Create(requesting_origin), &original_requesting_origin, nullptr) && - original_requesting_origin == url::Origin::Create(embedding_origin)) { + net::registry_controlled_domains::SameDomainOrHost( + original_requesting_origin, url::Origin::Create(embedding_origin), + net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES)) { return true; } diff --git a/components/permissions/contexts/brave_wallet_permission_context.cc b/components/permissions/contexts/brave_wallet_permission_context.cc index 89b4f36cb6ab..6861dc383a41 100644 --- a/components/permissions/contexts/brave_wallet_permission_context.cc +++ b/components/permissions/contexts/brave_wallet_permission_context.cc @@ -19,6 +19,7 @@ #include "components/permissions/permissions_client.h" #include "content/public/browser/render_frame_host.h" #include "content/public/browser/web_contents.h" +#include "net/base/registry_controlled_domains/registry_controlled_domain.h" #include "third_party/blink/public/mojom/permissions_policy/permissions_policy_feature.mojom.h" #include "url/origin.h" @@ -163,9 +164,12 @@ void BraveWalletPermissionContext::RequestPermissions( } auto* web_contents = content::WebContents::FromRenderFrameHost(rfh); - // Fail the request came from 3p origin. - if (web_contents->GetMainFrame()->GetLastCommittedOrigin() != - rfh->GetLastCommittedOrigin()) { + // Fail the request came from a different eTLD+1 origin. + if (rfh->GetParent() && + !net::registry_controlled_domains::SameDomainOrHost( + web_contents->GetMainFrame()->GetLastCommittedOrigin(), + rfh->GetLastCommittedOrigin(), + net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES)) { std::move(callback).Run(std::vector()); return; } @@ -211,9 +215,13 @@ void BraveWalletPermissionContext::GetAllowedAccounts( } auto* web_contents = content::WebContents::FromRenderFrameHost(rfh); - // Fail the request came from 3p origin. - if (web_contents->GetMainFrame()->GetLastCommittedOrigin() != - rfh->GetLastCommittedOrigin()) { + // Fail the request came from a different eTLD+1 origin. is missing for this + // function + if (rfh->GetParent() && + !net::registry_controlled_domains::SameDomainOrHost( + web_contents->GetMainFrame()->GetLastCommittedOrigin(), + rfh->GetLastCommittedOrigin(), + net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES)) { std::move(callback).Run(false, std::vector()); return; } diff --git a/renderer/brave_wallet/brave_wallet_render_frame_observer.cc b/renderer/brave_wallet/brave_wallet_render_frame_observer.cc index 9e219ace5a11..eadc0e03e3d2 100644 --- a/renderer/brave_wallet/brave_wallet_render_frame_observer.cc +++ b/renderer/brave_wallet/brave_wallet_render_frame_observer.cc @@ -11,6 +11,7 @@ #include "brave/components/brave_wallet/common/features.h" #include "content/public/common/isolated_world_ids.h" #include "content/public/renderer/render_frame.h" +#include "net/base/registry_controlled_domains/registry_controlled_domain.h" #include "third_party/abseil-cpp/absl/types/optional.h" #include "third_party/blink/public/web/web_local_frame.h" @@ -45,9 +46,14 @@ void BraveWalletRenderFrameObserver::DidCreateScriptContext( js_ethereum_provider_.reset(); return; } - // Wallet provider objects won't be generated for third party iframe + + // Wallet provider objects won't be generated for different eTLD+1s if (!render_frame()->IsMainFrame() && - render_frame()->GetWebFrame()->IsCrossOriginToOutermostMainFrame()) { + !net::registry_controlled_domains::SameDomainOrHost( + url::Origin( + render_frame()->GetWebFrame()->Top()->GetSecurityOrigin()), + url::Origin(render_frame()->GetWebFrame()->GetSecurityOrigin()), + net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES)) { return; } diff --git a/renderer/test/js_ethereum_provider_browsertest.cc b/renderer/test/js_ethereum_provider_browsertest.cc index d338123e209e..68d63f16f40e 100644 --- a/renderer/test/js_ethereum_provider_browsertest.cc +++ b/renderer/test/js_ethereum_provider_browsertest.cc @@ -227,4 +227,11 @@ IN_PROC_BROWSER_TEST_F(JSEthereumProviderBrowserTest, Block3PIframe) { iframe_rfh = ChildFrameAt(main_frame(), 0); ASSERT_TRUE(iframe_rfh); EXPECT_FALSE(content::EvalJs(iframe_rfh, kEvalEthereum).ExtractBool()); + + // same eTLD+1 + GURL iframe_url_etldp1(https_server_.GetURL("c.b.a.com", "/")); + EXPECT_TRUE(NavigateIframeToURL(web_contents(), "test", iframe_url_etldp1)); + iframe_rfh = ChildFrameAt(main_frame(), 0); + ASSERT_TRUE(iframe_rfh); + EXPECT_FALSE(content::EvalJs(iframe_rfh, kEvalEthereum).ExtractBool()); }