Skip to content

Commit

Permalink
MPArch: Treat fenced frames as subresources in SubresourceFilter
Browse files Browse the repository at this point in the history 
[Picking up adithyas@'s CL: https://crrev.com/c/3296548]

This CL changes CSFTM to not create throttle managers for fenced frames
and makes them use their embedding page's throttle manager instead. It
also updates other some of the throttles to handle fenced frame roots
correctly (and treat them like subframes instead of main frames).

A few notes about this patch:

1) It does not treat portals as subresources yet (we aren't sure
   what the right behaviour for them is yet, and this patch maintains
   existing behaviour for them)
2) This patch doesn't fix ad tagging for fenced frames (this will be
   done in a separate patch) and needs some browser and renderer side
   changes
3) It does not fix activation inheritance for fenced frames

Design doc: https://docs.google.com/document/d/1tD8ZatT4PbIRdA2LzIUWCjLoOmKYCMnv526XEiwdy5g/edit?usp=sharing

* test_render_view_host.cc:

Without this, some tests were crashing due to not having a ScreenInfo
while performing a SynchronizeVisualProperties, see also:
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_widget_host_view_child_frame.cc;l=65;drc=46aacb37f89802901db6d8cb19e3e2ec01408ec4


Bug: 1263541
Change-Id: I0f30c5aec4e0ecb002769b08d2d142a12e1112e6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3488833
Reviewed-by: Kevin McNee <mcnee@chromium.org>
Reviewed-by: Adithya Srinivasan <adithyas@chromium.org>
Reviewed-by: John Delaney <johnidel@chromium.org>
Reviewed-by: Alex Turner <alexmt@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#985298}
  • Loading branch information
@bokand
bokand authored and Chromium LUCI CQ committed Mar 25, 2022
1 parent 695d4b8 commit 5a0d4e5
Show file tree
Hide file tree
Showing 26 changed files with 719 additions and 185 deletions.
3 changes: 3 additions & 0 deletions chrome/browser/devtools/protocol/page_handler.cc
View file
Expand Up @@ -57,6 +57,9 @@ protocol::Response PageHandler::Enable() {

protocol::Response PageHandler::Disable() {
enabled_ = false;
// TODO(bokan): This is inadvertently called from a FencedFrame as it has a
// PageHandler that gets destroyed when the main frame is refreshed.
// ToggleAdBlocking should be a no-op for non-primary pages.
ToggleAdBlocking(false /* enable */);
SetSPCTransactionMode(protocol::Page::SetSPCTransactionMode::ModeEnum::None);
// Do not mark the command as handled. Let it fall through instead, so that
Expand Down
214 changes: 213 additions & 1 deletion chrome/browser/subresource_filter/subresource_filter_fenced_frame_browsertest.cc
View file
Expand Up @@ -4,8 +4,12 @@

#include "chrome/browser/subresource_filter/subresource_filter_browser_test_harness.h"

#include "base/strings/pattern.h"
#include "base/test/bind.h"
#include "chrome/test/base/ui_test_utils.h"
#include "components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h"
#include "components/subresource_filter/core/browser/subresource_filter_constants.h"
#include "components/subresource_filter/core/common/test_ruleset_utils.h"
#include "content/public/test/browser_test.h"
#include "content/public/test/browser_test_utils.h"
#include "content/public/test/fenced_frame_test_util.h"
Expand All @@ -14,11 +18,21 @@
#include "testing/gtest/include/gtest/gtest.h"
#include "url/gurl.h"

using content::RenderFrameHost;
using ::testing::_;
using ::testing::Mock;

namespace subresource_filter {

namespace {
// This string comes from GetErrorStringForDisallowedLoad() in
// blink/renderer/core/loader/subresource_filter.cc
constexpr const char kBlinkDisallowSubframeConsoleMessageFormat[] =
"Chrome blocked resource %s on this site because this site tends to show "
"ads that interrupt, distract, mislead, or prevent user control. Learn "
"more at https://www.chromestatus.com/feature/5738264052891648";
} // namespace

// Tests that AddMessageToConsole() is not called from NavigationConsoleLogger
// with a fenced frame to ensure that it works only with the outermost main
// frame.
Expand All @@ -42,7 +56,7 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterFencedFrameBrowserTest,
// Load a fenced frame.
ConfigureURLWithWarning(fenced_frame_url,
{safe_browsing::SubresourceFilterType::BETTER_ADS});
content::RenderFrameHost* fenced_frame_host =
RenderFrameHost* fenced_frame_host =
fenced_frame_test_helper().CreateFencedFrame(
web_contents()->GetMainFrame(), fenced_frame_url);
ASSERT_EQ(0u, console_observer.messages().size());
Expand All @@ -53,4 +67,202 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterFencedFrameBrowserTest,
ASSERT_EQ(0u, console_observer.messages().size());
}

IN_PROC_BROWSER_TEST_F(SubresourceFilterFencedFrameBrowserTest,
CollapseBlockedFencedFrame) {
const GURL kTopLevelUrl(GetTestUrl("title1.html"));
ConfigureAsPhishingURL(kTopLevelUrl);

const GURL kUrlWithIncludedScript(
GetTestUrl("subresource_filter/frame_with_included_script.html"));
const GURL kUrlWithAllowedScript(
GetTestUrl("subresource_filter/frame_with_allowed_script.html"));

// Block documents that end in "included_script.html".
proto::UrlRule rule = testing::CreateSuffixRule("included_script.html");
ASSERT_NO_FATAL_FAILURE(SetRulesetWithRules({rule}));

ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), kTopLevelUrl));

// Load an unblocked document into the fenced frame, ensure it isn't
// collapsed.
RenderFrameHost* fenced_frame_root =
fenced_frame_test_helper().CreateFencedFrame(
web_contents()->GetMainFrame(), kUrlWithAllowedScript);
EXPECT_EQ("300x150", EvalJs(web_contents()->GetMainFrame(), R"JS(
let ff = document.querySelector('fencedframe');
`${ff.clientWidth}x${ff.clientHeight}`;
)JS"));

fenced_frame_root = fenced_frame_test_helper().NavigateFrameInFencedFrameTree(
fenced_frame_root, kUrlWithIncludedScript,
/*expected_error_code=*/net::ERR_ABORTED);

EXPECT_EQ("0x0", EvalJs(web_contents()->GetMainFrame(), R"JS(
let ff = document.querySelector('fencedframe');
`${ff.clientWidth}x${ff.clientHeight}`;
)JS"));
}

// Test that filtering resources inside a fencedframe works when the outer page
// is activated.
IN_PROC_BROWSER_TEST_F(SubresourceFilterFencedFrameBrowserTest,
OutermostFrameActivation) {
const std::string kMessageFilter =
base::StringPrintf(kBlinkDisallowSubframeConsoleMessageFormat, "*");
content::WebContentsConsoleObserver console_observer(web_contents());
console_observer.SetPattern(kMessageFilter);

const GURL kTopLevelUrl(GetTestUrl("title1.html"));
ConfigureAsPhishingURL(kTopLevelUrl);
ASSERT_NO_FATAL_FAILURE(
SetRulesetToDisallowURLsWithPathSuffix("included_script.js"));

// Navigate to a phishing page and load a page with ad script into a
// fencedframe.
ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), kTopLevelUrl));
RenderFrameHost* fenced_frame_root =
fenced_frame_test_helper().CreateFencedFrame(
web_contents()->GetMainFrame(),
GetTestUrl("/subresource_filter/frame_with_included_script.html"));

// Ensure the disallowed script was blocked.
EXPECT_FALSE(WasParsedScriptElementLoaded(fenced_frame_root));

// Ensure content settings has seen the ad as blocked (i.e. the UI was
// shown).
EXPECT_TRUE(AdsBlockedInContentSettings(web_contents()->GetMainFrame()));
EXPECT_FALSE(AdsBlockedInContentSettings(fenced_frame_root));

// Console message for subframe blocking should be displayed.
EXPECT_TRUE(base::MatchPattern(
console_observer.GetMessageAt(0u),
base::StringPrintf(kBlinkDisallowSubframeConsoleMessageFormat,
"*included_script.js")));
}

// Tests that navigations of a fenced frame are correctly blocked when the
// outer page is activated and the fenced frame URL matches a blocklist or
// allowlist rule.
IN_PROC_BROWSER_TEST_F(SubresourceFilterFencedFrameBrowserTest,
FencedFrameLoadFiltering) {
const GURL kTopLevelUrl(GetTestUrl("title1.html"));
ConfigureAsPhishingURL(kTopLevelUrl);

const GURL kUrlWithIncludedScript(
GetTestUrl("subresource_filter/frame_with_included_script.html"));
const GURL kUrlWithAllowedScript(
GetTestUrl("subresource_filter/frame_with_allowed_script.html"));
const std::string kAllowlistedDomain = "allowlisted.example";
const GURL kAllowlistedUrlWithIncludedScript(embedded_test_server()->GetURL(
kAllowlistedDomain,
"/subresource_filter/frame_with_included_script.html"));

// Block documents that end in "included_script.html", unless the document is
// loaded from an allowlisted domain. This enables the part of this test
// disallowing a load only after a redirect.
ASSERT_NO_FATAL_FAILURE(SetRulesetWithRules(
{testing::CreateSuffixRule("included_script.html"),
testing::CreateAllowlistSubstringRule(kAllowlistedDomain)}));

ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), kTopLevelUrl));

// Loading a document into a fenced frame that's on the blocklist should
// cancel the load.
RenderFrameHost* fenced_frame_root =
fenced_frame_test_helper().CreateFencedFrame(
web_contents()->GetMainFrame(), kUrlWithIncludedScript,
/*expected_error_code=*/net::ERR_ABORTED);
EXPECT_FALSE(WasParsedScriptElementLoaded(fenced_frame_root));

// Now navigate the fenced frame to a non-blocked document and ensure that
// the load is successful and the frame gets restored (no longer collapsed).
fenced_frame_root = fenced_frame_test_helper().NavigateFrameInFencedFrameTree(
fenced_frame_root, kUrlWithAllowedScript);
EXPECT_TRUE(WasParsedScriptElementLoaded(fenced_frame_root));

// Navigate to a URL that has the blocked suffix but whose domain is
// allowlisted. This should not be filtered.
fenced_frame_root = fenced_frame_test_helper().NavigateFrameInFencedFrameTree(
fenced_frame_root, kAllowlistedUrlWithIncludedScript);
EXPECT_TRUE(WasParsedScriptElementLoaded(fenced_frame_root));
EXPECT_EQ(kAllowlistedUrlWithIncludedScript,
fenced_frame_root->GetLastCommittedURL());

// Finally, navigate the fenced frame to an allowlisted URL that redirects to
// the blocked page and verify that the navigation is cancelled.
const GURL kRedirectingUrl(embedded_test_server()->GetURL(
kAllowlistedDomain, "/server-redirect?" + kUrlWithIncludedScript.spec()));
fenced_frame_root = fenced_frame_test_helper().NavigateFrameInFencedFrameTree(
fenced_frame_root, kRedirectingUrl,
/*expected_error_code=*/net::ERR_ABORTED);

EXPECT_FALSE(WasParsedScriptElementLoaded(fenced_frame_root));
EXPECT_EQ(kUrlWithIncludedScript, fenced_frame_root->GetLastCommittedURL());
}

// Same as above test but tests filtering of navigations occurring inside
// subframes embedded within in a fenced frame.
IN_PROC_BROWSER_TEST_F(SubresourceFilterFencedFrameBrowserTest,
LoadFilteringNestedInFencedFrame) {
const GURL kTopLevelUrl(GetTestUrl("title1.html"));
ConfigureAsPhishingURL(kTopLevelUrl);

const GURL kFencedFrameUrl(
GetTestUrl("subresource_filter/included_script_in_iframe.html"));
const GURL kUrlWithIncludedScript(
GetTestUrl("subresource_filter/frame_with_included_script.html"));
const GURL kUrlWithAllowedScript(
GetTestUrl("subresource_filter/frame_with_allowed_script.html"));
const std::string kAllowlistedDomain = "allowlisted.example";
const GURL kAllowlistedUrlWithIncludedScript(embedded_test_server()->GetURL(
kAllowlistedDomain,
"/subresource_filter/frame_with_included_script.html"));

// Block documents that end in "included_script.html", unless the document is
// loaded from an allowlisted domain. This enables the third part of this
// test disallowing a load only after the first redirect.
ASSERT_NO_FATAL_FAILURE(SetRulesetWithRules(
{testing::CreateSuffixRule("included_script.html"),
testing::CreateAllowlistSubstringRule(kAllowlistedDomain)}));

ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), kTopLevelUrl));
RenderFrameHost* fenced_frame_root =
fenced_frame_test_helper().CreateFencedFrame(
web_contents()->GetMainFrame(), kFencedFrameUrl);
RenderFrameHost* subframe = content::FrameMatchingPredicate(
fenced_frame_root->GetPage(),
base::BindRepeating(&content::FrameMatchesName, "subframe"));

// The fenced frame document initially has an iframe with
// frame_with_included_script.html so it should be blocked.
EXPECT_FALSE(WasParsedScriptElementLoaded(subframe));
EXPECT_TRUE(subframe->IsErrorDocument());

// Now navigate the subframe to a non-blocked document and ensure that
// the load is successful and the frame gets restored (no longer collapsed).
subframe = fenced_frame_test_helper().NavigateFrameInFencedFrameTree(
subframe, kUrlWithAllowedScript);
EXPECT_TRUE(WasParsedScriptElementLoaded(subframe));
EXPECT_FALSE(subframe->IsErrorDocument());
EXPECT_EQ(kUrlWithAllowedScript, subframe->GetLastCommittedURL());

// Navigate to a URL that has the blocked suffix but whose domain is
// allowlisted. This should not be filtered.
subframe = fenced_frame_test_helper().NavigateFrameInFencedFrameTree(
subframe, kAllowlistedUrlWithIncludedScript);
EXPECT_TRUE(WasParsedScriptElementLoaded(subframe));
EXPECT_EQ(kAllowlistedUrlWithIncludedScript, subframe->GetLastCommittedURL());

// Finally, navigate the subframe to an allowlisted URL that redirects to
// the blocked page and verify that the navigation is cancelled.
const GURL kRedirectingUrl(embedded_test_server()->GetURL(
kAllowlistedDomain, "/server-redirect?" + kUrlWithIncludedScript.spec()));
subframe = fenced_frame_test_helper().NavigateFrameInFencedFrameTree(
subframe, kRedirectingUrl, /*expected_error_code=*/net::ERR_ABORTED);

EXPECT_FALSE(WasParsedScriptElementLoaded(subframe));
EXPECT_TRUE(subframe->IsErrorDocument());
EXPECT_EQ(kUrlWithIncludedScript, subframe->GetLastCommittedURL());
}

} // namespace subresource_filter
2 changes: 1 addition & 1 deletion chrome/browser/subresource_filter/subresource_filter_prerender_browsertest.cc
View file
Expand Up @@ -248,7 +248,7 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterPrerenderingBrowserTest,
EXPECT_CALL(observer, OnPageActivationComputed(_, _)).Times(0);
prerender_helper_.NavigatePrimaryPage(kPrerenderingUrl);

EXPECT_TRUE(AdsBlockedInContentSettings(web_contents()->GetMainFrame()));
ASSERT_EQ(web_contents()->GetMainFrame(), prerender_rfh);
EXPECT_TRUE(AdsBlockedInContentSettings(prerender_rfh));
}
}
Expand Down
16 changes: 12 additions & 4 deletions components/blocked_content/safe_browsing_triggered_popup_blocker_unittest.cc
View file
Expand Up @@ -21,6 +21,7 @@
#include "components/content_settings/browser/page_specific_content_settings.h"
#include "components/content_settings/browser/test_page_specific_content_settings_delegate.h"
#include "components/content_settings/core/browser/host_content_settings_map.h"
#include "components/subresource_filter/content/browser/content_subresource_filter_web_contents_helper.h"
#include "components/subresource_filter/content/browser/fake_safe_browsing_database_manager.h"
#include "components/subresource_filter/content/browser/subresource_filter_observer_manager.h"
#include "components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h"
Expand Down Expand Up @@ -137,10 +138,17 @@ class SafeBrowsingTriggeredPopupBlockerTestBase
protected:
std::unique_ptr<content::NavigationThrottle> CreateThrottle(
content::NavigationHandle* handle) {
return std::make_unique<
subresource_filter::SubresourceFilterSafeBrowsingActivationThrottle>(
handle, /*delegate=*/nullptr, content::GetIOThreadTaskRunner({}),
fake_safe_browsing_database_);
// Activation is only computed when navigating a subresource filter root
// (see content_subresource_filter_throttle_manager.h for the definition of
// a root).
if (subresource_filter::IsInSubresourceFilterRoot(handle)) {
return std::make_unique<
subresource_filter::SubresourceFilterSafeBrowsingActivationThrottle>(
handle, /*delegate=*/nullptr, content::GetIOThreadTaskRunner({}),
fake_safe_browsing_database_);
}

return nullptr;
}

base::test::ScopedFeatureList scoped_feature_list_;
Expand Down
26 changes: 14 additions & 12 deletions ...ents/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc
View file
Expand Up @@ -10,6 +10,7 @@
#include "base/callback.h"
#include "base/memory/ptr_util.h"
#include "components/subresource_filter/content/browser/async_document_subresource_filter.h"
#include "components/subresource_filter/content/browser/content_subresource_filter_web_contents_helper.h"
#include "content/public/browser/navigation_handle.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/web_contents.h"
Expand All @@ -18,20 +19,20 @@ namespace subresource_filter {

// static
std::unique_ptr<ActivationStateComputingNavigationThrottle>
ActivationStateComputingNavigationThrottle::CreateForMainFrame(
ActivationStateComputingNavigationThrottle::CreateForRoot(
content::NavigationHandle* navigation_handle) {
DCHECK(navigation_handle->IsInMainFrame());
DCHECK(IsInSubresourceFilterRoot(navigation_handle));
return base::WrapUnique(new ActivationStateComputingNavigationThrottle(
navigation_handle, absl::optional<mojom::ActivationState>(), nullptr));
}

// static
std::unique_ptr<ActivationStateComputingNavigationThrottle>
ActivationStateComputingNavigationThrottle::CreateForSubframe(
ActivationStateComputingNavigationThrottle::CreateForChild(
content::NavigationHandle* navigation_handle,
VerifiedRuleset::Handle* ruleset_handle,
const mojom::ActivationState& parent_activation_state) {
DCHECK(!navigation_handle->IsInMainFrame());
DCHECK(!IsInSubresourceFilterRoot(navigation_handle));
DCHECK_NE(mojom::ActivationLevel::kDisabled,
parent_activation_state.activation_level);
DCHECK(ruleset_handle);
Expand All @@ -55,7 +56,7 @@ void ActivationStateComputingNavigationThrottle::
NotifyPageActivationWithRuleset(
VerifiedRuleset::Handle* ruleset_handle,
const mojom::ActivationState& page_activation_state) {
DCHECK(navigation_handle()->IsInMainFrame());
DCHECK(IsInSubresourceFilterRoot(navigation_handle()));
DCHECK_NE(mojom::ActivationLevel::kDisabled,
page_activation_state.activation_level);
parent_activation_state_ = page_activation_state;
Expand All @@ -81,7 +82,7 @@ ActivationStateComputingNavigationThrottle::WillProcessResponse() {
// If no parent activation, this is main frame that was never notified of
// activation.
if (!parent_activation_state_) {
DCHECK(navigation_handle()->IsInMainFrame());
DCHECK(IsInSubresourceFilterRoot(navigation_handle()));
DCHECK(!async_filter_);
DCHECK(!ruleset_handle_);
return content::NavigationThrottle::PROCEED;
Expand All @@ -92,14 +93,14 @@ ActivationStateComputingNavigationThrottle::WillProcessResponse() {
// finish, or start a new check now if there was no previous speculative
// check.
if (async_filter_ && async_filter_->has_activation_state()) {
if (navigation_handle()->IsInMainFrame())
if (IsInSubresourceFilterRoot(navigation_handle()))
UpdateWithMoreAccurateState();
return content::NavigationThrottle::PROCEED;
}
DCHECK(!deferred_);
deferred_ = true;
if (!async_filter_) {
DCHECK(navigation_handle()->IsInMainFrame());
DCHECK(IsInSubresourceFilterRoot(navigation_handle()));
CheckActivationState();
}
return content::NavigationThrottle::DEFER;
Expand All @@ -115,8 +116,9 @@ void ActivationStateComputingNavigationThrottle::CheckActivationState() {
AsyncDocumentSubresourceFilter::InitializationParams params;
params.document_url = navigation_handle()->GetURL();
params.parent_activation_state = parent_activation_state_.value();
if (!navigation_handle()->IsInMainFrame()) {
content::RenderFrameHost* parent = navigation_handle()->GetParentFrame();
if (!IsInSubresourceFilterRoot(navigation_handle())) {
content::RenderFrameHost* parent =
navigation_handle()->GetParentFrameOrOuterDocument();
DCHECK(parent);
params.parent_document_origin = parent->GetLastCommittedOrigin();
}
Expand All @@ -135,7 +137,7 @@ void ActivationStateComputingNavigationThrottle::CheckActivationState() {
void ActivationStateComputingNavigationThrottle::OnActivationStateComputed(
mojom::ActivationState state) {
if (deferred_) {
if (navigation_handle()->IsInMainFrame())
if (IsInSubresourceFilterRoot(navigation_handle()))
UpdateWithMoreAccurateState();
Resume();
}
Expand All @@ -145,7 +147,7 @@ void ActivationStateComputingNavigationThrottle::UpdateWithMoreAccurateState() {
// This method is only needed for main frame navigations that are notified of
// page activation more than once. Even for those that are updated once, it
// should be a no-op.
DCHECK(navigation_handle()->IsInMainFrame());
DCHECK(IsInSubresourceFilterRoot(navigation_handle()));
DCHECK(parent_activation_state_);
DCHECK(async_filter_);
async_filter_->UpdateWithMoreAccurateState(*parent_activation_state_);
Expand Down

0 comments on commit 5a0d4e5

Please sign in to comment.