From edf39ee8e7abc545ad1b60619bf0a221a3765541 Mon Sep 17 00:00:00 2001 From: sergei Date: Mon, 10 Apr 2023 14:40:01 +0300 Subject: [PATCH 1/3] Use frame url for scripts without sources --- ...ields_web_contents_observer_browsertest.cc | 29 +++++++++++++++++++ .../brave_content_settings_agent_impl.cc | 8 ++++- test/data/load_js_dataurls.html | 9 ++++++ 3 files changed, 45 insertions(+), 1 deletion(-) create mode 100644 test/data/load_js_dataurls.html diff --git a/browser/brave_shields/brave_shields_web_contents_observer_browsertest.cc b/browser/brave_shields/brave_shields_web_contents_observer_browsertest.cc index 54f6058ffd96..c4dcc5d66a0d 100644 --- a/browser/brave_shields/brave_shields_web_contents_observer_browsertest.cc +++ b/browser/brave_shields/brave_shields_web_contents_observer_browsertest.cc @@ -277,4 +277,33 @@ IN_PROC_BROWSER_TEST_F(BraveShieldsWebContentsObserverBrowserTest, EXPECT_EQ(brave_shields_web_contents_observer()->block_javascript_count(), 0); } +IN_PROC_BROWSER_TEST_F(BraveShieldsWebContentsObserverBrowserTest, + JavaScriptAllowedDataUrls) { + const GURL& url = GURL("a.com"); + + // Start with JavaScript blocking initially disabled. + ContentSetting block_javascript_setting = + content_settings()->GetContentSetting(url, url, + ContentSettingsType::JAVASCRIPT); + EXPECT_EQ(CONTENT_SETTING_ALLOW, block_javascript_setting); + // Enable JavaScript blocking globally now. + content_settings()->SetContentSettingCustomScope( + ContentSettingsPattern::Wildcard(), ContentSettingsPattern::Wildcard(), + ContentSettingsType::JAVASCRIPT, CONTENT_SETTING_BLOCK); + block_javascript_setting = content_settings()->GetContentSetting( + url, url, ContentSettingsType::JAVASCRIPT); + EXPECT_EQ(CONTENT_SETTING_BLOCK, block_javascript_setting); + + // Load a simple HTML that attempts to load some JavaScript with data urls. + auto page_url = + embedded_test_server()->GetURL("a.com", "/load_js_dataurls.html"); + EXPECT_TRUE(ui_test_utils::NavigateToURL(browser(), page_url)); + EXPECT_TRUE(WaitForLoadStop(GetWebContents())); + EXPECT_EQ(brave_shields_web_contents_observer()->block_javascript_count(), 3); + auto blocked_list = GetBlockedJsList(); + EXPECT_EQ(blocked_list.size(), 1u); + EXPECT_EQ(GURL(blocked_list.front()), + GURL(url::Origin::Create(page_url).Serialize())); +} + } // namespace brave_shields diff --git a/components/content_settings/renderer/brave_content_settings_agent_impl.cc b/components/content_settings/renderer/brave_content_settings_agent_impl.cc index 8bc23d95ae33..96476fb19e64 100644 --- a/components/content_settings/renderer/brave_content_settings_agent_impl.cc +++ b/components/content_settings/renderer/brave_content_settings_agent_impl.cc @@ -224,7 +224,13 @@ bool BraveContentSettingsAgentImpl::AllowStorageAccessSync( bool BraveContentSettingsAgentImpl::AllowScriptFromSource( bool enabled_per_settings, const blink::WebURL& script_url) { - const GURL secondary_url(script_url); + GURL secondary_url(script_url); + // For scripts w/o sources it should report the domain / site used for + // executing the frame (which most, but not all, of the time will just be from + // document.location + if (secondary_url.SchemeIs(url::kDataScheme)) { + secondary_url = render_frame()->GetWebFrame()->GetDocument().Url(); + } bool allow = ContentSettingsAgentImpl::AllowScriptFromSource( enabled_per_settings, script_url); diff --git a/test/data/load_js_dataurls.html b/test/data/load_js_dataurls.html new file mode 100644 index 000000000000..279b342d35a3 --- /dev/null +++ b/test/data/load_js_dataurls.html @@ -0,0 +1,9 @@ +load some js code + + + + + + From 237bffd4b0f76c5c7a90fbd74513486052df34bd Mon Sep 17 00:00:00 2001 From: sergei Date: Mon, 10 Apr 2023 18:59:44 +0300 Subject: [PATCH 2/3] Use GetSecurityOrigin --- .../renderer/brave_content_settings_agent_impl.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/components/content_settings/renderer/brave_content_settings_agent_impl.cc b/components/content_settings/renderer/brave_content_settings_agent_impl.cc index 96476fb19e64..af51343335cd 100644 --- a/components/content_settings/renderer/brave_content_settings_agent_impl.cc +++ b/components/content_settings/renderer/brave_content_settings_agent_impl.cc @@ -229,7 +229,9 @@ bool BraveContentSettingsAgentImpl::AllowScriptFromSource( // executing the frame (which most, but not all, of the time will just be from // document.location if (secondary_url.SchemeIs(url::kDataScheme)) { - secondary_url = render_frame()->GetWebFrame()->GetDocument().Url(); + secondary_url = + url::Origin(render_frame()->GetWebFrame()->GetSecurityOrigin()) + .GetURL(); } bool allow = ContentSettingsAgentImpl::AllowScriptFromSource( enabled_per_settings, script_url); From 943c7b39abcb69b9742198e11cf7abf48031c994 Mon Sep 17 00:00:00 2001 From: sergei Date: Mon, 10 Apr 2023 21:04:14 +0300 Subject: [PATCH 3/3] Added test for subframe --- ...ields_web_contents_observer_browsertest.cc | 28 ++++++++++++++++--- .../brave_content_settings_agent_impl.cc | 2 +- test/data/load_js_dataurls.html | 4 +-- test/data/load_js_dataurls.js | 14 ++++++++++ 4 files changed, 41 insertions(+), 7 deletions(-) create mode 100644 test/data/load_js_dataurls.js diff --git a/browser/brave_shields/brave_shields_web_contents_observer_browsertest.cc b/browser/brave_shields/brave_shields_web_contents_observer_browsertest.cc index c4dcc5d66a0d..e873c7abf53e 100644 --- a/browser/brave_shields/brave_shields_web_contents_observer_browsertest.cc +++ b/browser/brave_shields/brave_shields_web_contents_observer_browsertest.cc @@ -286,6 +286,7 @@ IN_PROC_BROWSER_TEST_F(BraveShieldsWebContentsObserverBrowserTest, content_settings()->GetContentSetting(url, url, ContentSettingsType::JAVASCRIPT); EXPECT_EQ(CONTENT_SETTING_ALLOW, block_javascript_setting); + // Enable JavaScript blocking globally now. content_settings()->SetContentSettingCustomScope( ContentSettingsPattern::Wildcard(), ContentSettingsPattern::Wildcard(), @@ -299,11 +300,30 @@ IN_PROC_BROWSER_TEST_F(BraveShieldsWebContentsObserverBrowserTest, embedded_test_server()->GetURL("a.com", "/load_js_dataurls.html"); EXPECT_TRUE(ui_test_utils::NavigateToURL(browser(), page_url)); EXPECT_TRUE(WaitForLoadStop(GetWebContents())); + EXPECT_EQ(brave_shields_web_contents_observer()->block_javascript_count(), 4); + brave_shields_web_contents_observer()->Reset(); + // Allow subframe script and check we still block his data urls. + std::string subframe_script = + url::Origin::Create(page_url).Serialize() + "/load_js_dataurls.js"; + brave_shields_web_contents_observer()->AllowScriptsOnce( + std::vector({subframe_script})); + ClearAllResourcesList(); + GetWebContents()->GetController().Reload(content::ReloadType::NORMAL, true); + EXPECT_TRUE(WaitForLoadStop(GetWebContents())); + EXPECT_EQ(GetBlockedJsList().size(), 1u); + EXPECT_EQ(GetAllowedJsList().size(), 1u); EXPECT_EQ(brave_shields_web_contents_observer()->block_javascript_count(), 3); - auto blocked_list = GetBlockedJsList(); - EXPECT_EQ(blocked_list.size(), 1u); - EXPECT_EQ(GURL(blocked_list.front()), - GURL(url::Origin::Create(page_url).Serialize())); + brave_shields_web_contents_observer()->Reset(); + + // Allow all scripts for domain. + brave_shields_web_contents_observer()->AllowScriptsOnce( + std::vector({url::Origin::Create(page_url).Serialize()})); + ClearAllResourcesList(); + GetWebContents()->GetController().Reload(content::ReloadType::NORMAL, true); + EXPECT_TRUE(WaitForLoadStop(GetWebContents())); + + EXPECT_EQ(GetAllowedJsList().size(), 2u); + EXPECT_EQ(brave_shields_web_contents_observer()->block_javascript_count(), 0); } } // namespace brave_shields diff --git a/components/content_settings/renderer/brave_content_settings_agent_impl.cc b/components/content_settings/renderer/brave_content_settings_agent_impl.cc index af51343335cd..91cd8b56c924 100644 --- a/components/content_settings/renderer/brave_content_settings_agent_impl.cc +++ b/components/content_settings/renderer/brave_content_settings_agent_impl.cc @@ -228,7 +228,7 @@ bool BraveContentSettingsAgentImpl::AllowScriptFromSource( // For scripts w/o sources it should report the domain / site used for // executing the frame (which most, but not all, of the time will just be from // document.location - if (secondary_url.SchemeIs(url::kDataScheme)) { + if (secondary_url.SchemeIsLocal()) { secondary_url = url::Origin(render_frame()->GetWebFrame()->GetSecurityOrigin()) .GetURL(); diff --git a/test/data/load_js_dataurls.html b/test/data/load_js_dataurls.html index 279b342d35a3..5cde73be67e7 100644 --- a/test/data/load_js_dataurls.html +++ b/test/data/load_js_dataurls.html @@ -3,7 +3,7 @@ - - + + diff --git a/test/data/load_js_dataurls.js b/test/data/load_js_dataurls.js new file mode 100644 index 000000000000..95b1141881e0 --- /dev/null +++ b/test/data/load_js_dataurls.js @@ -0,0 +1,14 @@ +/* Copyright (c) 2023 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +var iframe = document.createElement('IFRAME'); +iframe.id = iframe.name = 'test_iframe'; +iframe.src = 'about:blank'; +document.body.appendChild(iframe); + +var frame = window.frames['test_iframe']; +frame.document.open(); +frame.document.write(''); +frame.document.close();