Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow permission requests within the same eTLD+1 #13564

Closed
wants to merge 1 commit into from
Closed

Allow permission requests within the same eTLD+1 #13564

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions browser/brave_wallet/solana_provider_renderer_browsertest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
21 changes: 21 additions & 0 deletions browser/permissions/brave_wallet_permission_context_browsertest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,4 +174,25 @@ IN_PROC_BROWSER_TEST_F(BraveWalletPermissionContextBrowserTest,
EXPECT_TRUE(was_called);
}

IN_PROC_BROWSER_TEST_F(BraveWalletPermissionContextBrowserTest,
GetAllowedAccountsAllowETLDP1Iframe) {
std::vector<std::string> 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<std::string>()));
content::RunAllTasksUntilIdle();
EXPECT_TRUE(was_called);
}

} // namespace permissions
81 changes: 75 additions & 6 deletions browser/permissions/permission_manager_browsertest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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);
darkdh marked this conversation as resolved.
Show resolved Hide resolved
https_server()->ServeFilesFromSourceDirectory(GetChromeTestDataDir());
ASSERT_TRUE(https_server()->Start());

Expand All @@ -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();
}
Expand All @@ -119,11 +136,12 @@ class PermissionManagerBrowserTest : public InProcessBrowserTest {
raw_ptr<PermissionManager> 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();
Expand Down Expand Up @@ -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<std::string> addresses;
Expand Down Expand Up @@ -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);
Expand All @@ -341,8 +359,59 @@ IN_PROC_BROWSER_TEST_F(PermissionManagerBrowserTest,
}
}

IN_PROC_BROWSER_TEST_F(PermissionManagerBrowserTest,
RequestPermissionsAllowETLDP1) {
struct {
std::vector<std::string> 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<PermissionRequestManagerObserver>(
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<ContentSetting>& 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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls use base::RunLoop instead and call Quit in the lambda.

Copy link
Member Author

@bbondy bbondy Jun 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3e1a254

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 {
Expand Down
5 changes: 4 additions & 1 deletion chromium_src/chrome/browser/permissions/chrome_permissions_client.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just wanted to double check - embedding_origin is the top level (main frame) origin as opposed to the parent origin? ex: if a.com embeds b.com which embeds c.com and the requesting origin is c.com, embedding origin is a.com not b.com.

return true;
}

Expand Down
20 changes: 14 additions & 6 deletions components/permissions/contexts/brave_wallet_permission_context.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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<ContentSetting>());
return;
}
Expand Down Expand Up @@ -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() &&
darkdh marked this conversation as resolved.
Show resolved Hide resolved
!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<std::string>());
return;
}
Expand Down
10 changes: 8 additions & 2 deletions renderer/brave_wallet/brave_wallet_render_frame_observer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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;
}

Expand Down
7 changes: 7 additions & 0 deletions renderer/test/js_ethereum_provider_browsertest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}