0

Reland "Enable SafeBrowsingAsyncRealTimeCheck by default on Desktop"

This is a reland of commit c52ea2979e

The original CL was reverted in https://crrev.com/c/5528858 because the
DontContainPrerenderingInfoInThreatReport test was failing on Chrome
bots. This was caused by test setup, and this CL fixes that.

That test had been added in https://crrev.com/c/2948322 to confirm
that prerender URLs are not attached to CSBRRs logged for the main
frame. It was simulating this scenario with a client-side phishing
warning that was determined pre-commit due to the test setup, and it
would still trigger attaching URLs from the DOM in the CSBRR since
client-side phishing warnings always returned false for the function
IsMainPageLoadBlocked. This test setup of a client-side phishing warning
being determined pre-commit is not possible in production code, and
stopped working with async checks being enabled because
IsMainPageLoadBlocked (now called IsMainPageLoadPending) is no longer
hard-coded to return false for client-side phishing warnings.

This CL replaces that test with a version that instead uses a post-
commit async check to confirm that prerender URLs are not attached,
since that is possible in production code. I have confirmed that undoing
the threat_details.cc crrev.com/c/2948322 fix causes the new test to
fail as well.

This CL also involves some refactoring for the new test:
 - SBBPAsyncChecksTimingTest and SBBPAsyncChecksPrerenderingTest inherit
   from SBBPAsyncChecksTimingTestBase. Only SBBPAsyncChecksTimingTest
   extends WithParamInterface<bool> as well to test both true and false
   for check_complete_after_navigation_finish. The split is done because
   the new test within SBBPAsyncChecksPrerenderingTest does not use this
   parameter.
 - A new SBBPAsyncChecksTestBase base class is introduced that
   SBBPAsyncChecksTest and SBBPAsyncChecksTimingTestBase inherit from.
   Only SBBPAsyncChecksTest extends WithParamInterface<bool> as well to
   test async checks enabled and disabled. SBBPAsyncChecksTimingTest was
   previously overriding the feature list to enable async checks, and
   used the param for check_complete_after_navigation_finish instead.

Original change's description:
> Enable SafeBrowsingAsyncRealTimeCheck by default on Desktop
>
> Bug: 40941453
> Change-Id: Ia89befcdcce75f39aa11ec9f87754b86ba371afe
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5522265
> Reviewed-by: Varun Khaneja <vakh@chromium.org>
> Commit-Queue: Varun Khaneja <vakh@chromium.org>
> Auto-Submit: thefrog <thefrog@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1298313}

Bug: 40941453
Change-Id: I21573d3ee95e4465d401b304d61794e319f95043
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5539814
Reviewed-by: Xinghui Lu <xinghuilu@chromium.org>
Commit-Queue: thefrog <thefrog@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1301593}
This commit is contained in:
thefrog
2024-05-15 21:43:45 +00:00
committed by Chromium LUCI CQ
parent 9246edc6de
commit d2317b8430
4 changed files with 181 additions and 130 deletions
chrome/browser/safe_browsing
components/safe_browsing/core
testing/variations

@ -3250,21 +3250,10 @@ IN_PROC_BROWSER_TEST_F(SafeBrowsingBlockingPageEnhancedProtectionMessageTest,
browser(), "enhanced-protection-message"));
}
class SafeBrowsingBlockingPageAsyncChecksTest
: public InProcessBrowserTest,
public testing::WithParamInterface<bool> {
class SafeBrowsingBlockingPageAsyncChecksTestBase
: public InProcessBrowserTest {
public:
SafeBrowsingBlockingPageAsyncChecksTest() = default;
void SetUp() override {
bool is_async_check_enabled = GetParam();
if (is_async_check_enabled) {
feature_list_.InitAndEnableFeature(kSafeBrowsingAsyncRealTimeCheck);
} else {
feature_list_.InitAndDisableFeature(kSafeBrowsingAsyncRealTimeCheck);
}
InProcessBrowserTest::SetUp();
}
SafeBrowsingBlockingPageAsyncChecksTestBase() = default;
void SetUpOnMainThread() override {
host_resolver()->AddRule("*", "127.0.0.1");
@ -3309,6 +3298,23 @@ class SafeBrowsingBlockingPageAsyncChecksTest
}
TestSafeBrowsingServiceFactory factory_;
};
class SafeBrowsingBlockingPageAsyncChecksTest
: public SafeBrowsingBlockingPageAsyncChecksTestBase,
public testing::WithParamInterface<bool> {
public:
SafeBrowsingBlockingPageAsyncChecksTest() = default;
void SetUp() override {
bool is_async_check_enabled = GetParam();
if (is_async_check_enabled) {
feature_list_.InitAndEnableFeature(kSafeBrowsingAsyncRealTimeCheck);
} else {
feature_list_.InitAndDisableFeature(kSafeBrowsingAsyncRealTimeCheck);
}
SafeBrowsingBlockingPageAsyncChecksTestBase::SetUp();
}
private:
base::test::ScopedFeatureList feature_list_;
@ -3371,29 +3377,29 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageAsyncChecksTest,
}
}
class SafeBrowsingBlockingPageAsyncChecksTimingTest
: public SafeBrowsingBlockingPageAsyncChecksTest {
class SafeBrowsingBlockingPageAsyncChecksTimingTestBase
: public SafeBrowsingBlockingPageAsyncChecksTestBase {
public:
SafeBrowsingBlockingPageAsyncChecksTimingTest() = default;
SafeBrowsingBlockingPageAsyncChecksTimingTestBase() = default;
void SetUp() override {
feature_list_.InitWithFeatures(
{kSafeBrowsingAsyncRealTimeCheck,
kCreateWarningShownClientSafeBrowsingReports},
{kRedWarningSurvey});
InProcessBrowserTest::SetUp();
SafeBrowsingBlockingPageAsyncChecksTestBase::SetUp();
}
void TearDown() override {
RealTimeUrlLookupServiceFactory::GetInstance()
->SetURLLoaderFactoryForTesting(nullptr);
InProcessBrowserTest::TearDown();
SafeBrowsingBlockingPageAsyncChecksTestBase::TearDown();
ThreatDetails::RegisterFactory(nullptr);
}
void CreatedBrowserMainParts(
content::BrowserMainParts* browser_main_parts) override {
SafeBrowsingBlockingPageAsyncChecksTest::CreatedBrowserMainParts(
SafeBrowsingBlockingPageAsyncChecksTestBase::CreatedBrowserMainParts(
browser_main_parts);
ThreatDetails::RegisterFactory(&details_factory_);
}
@ -3514,22 +3520,23 @@ class SafeBrowsingBlockingPageAsyncChecksTimingTest
return final_url;
}
// The following events happen in sequence:
// 1. Navigation finished.
// 2. Safe Browsing checks complete.
GURL SetupWarningShownAfterFinishNavigationAndNavigate(
std::vector<UrlAndIsUnsafe> url_and_server_redirects) {
CHECK(!url_and_server_redirects.empty());
GURL original_url = embedded_test_server()->GetURL(
url_and_server_redirects.front().relative_url);
GURL final_url = embedded_test_server()->GetURL(
url_and_server_redirects.back().relative_url);
void NavigateAndAwaitNavigationFinished(std::string relative_url) {
GURL original_url = embedded_test_server()->GetURL(relative_url);
content::TestNavigationManager navigation_manager(
browser()->tab_strip_model()->GetActiveWebContents(), original_url);
ui_test_utils::NavigateToURLWithDisposition(
browser(), original_url, WindowOpenDisposition::CURRENT_TAB,
ui_test_utils::BROWSER_TEST_NO_WAIT);
EXPECT_TRUE(navigation_manager.WaitForNavigationFinished());
}
// This is expected to be used after NavigateAndAwaitNavigationFinished has
// completed navigating to the same URLs. This method expects that at least
// one of the URLs is unsafe.
GURL ReturnUrlRealTimeVerdictsForUnsafeChain(
std::vector<UrlAndIsUnsafe> url_and_server_redirects) {
GURL final_url = embedded_test_server()->GetURL(
url_and_server_redirects.back().relative_url);
// At this point, the navigation has finished but the async check has not
// yet completed.
@ -3559,23 +3566,15 @@ class SafeBrowsingBlockingPageAsyncChecksTimingTest
return final_url;
}
GURL SetupPostCommitInterstitialAndNavigate(
std::vector<UrlAndIsUnsafe> url_and_server_redirects,
base::OnceClosure report_sent_callback) {
// Call SetupUrlRealTimeVerdictInCacheManager with a random URL to ensure
// RealTimeUrlLookupServiceBase::CanCheckUrl returns true so the real time
// check is performed.
SetupUrlRealTimeVerdictInCacheManager(
GURL("https://random.url"), browser()->profile(), /*is_unsafe=*/false);
SetReportSentCallback(std::move(report_sent_callback));
bool check_complete_after_navigation_finish = GetParam();
if (check_complete_after_navigation_finish) {
return SetupWarningShownAfterFinishNavigationAndNavigate(
url_and_server_redirects);
} else {
return SetupWarningShownBetweenProcessResponseAndFinishNavigationAndNavigate(
url_and_server_redirects);
}
// The following events happen in sequence:
// 1. Navigation finished.
// 2. Safe Browsing checks complete.
GURL SetupWarningShownAfterFinishNavigationAndNavigate(
std::vector<UrlAndIsUnsafe> url_and_server_redirects) {
CHECK(!url_and_server_redirects.empty());
NavigateAndAwaitNavigationFinished(
url_and_server_redirects.front().relative_url);
return ReturnUrlRealTimeVerdictsForUnsafeChain(url_and_server_redirects);
}
void SetReportSentCallback(base::OnceClosure callback) {
@ -3597,7 +3596,6 @@ class SafeBrowsingBlockingPageAsyncChecksTimingTest
}
base::HistogramTester histogram_tester_;
TestThreatDetailsFactory details_factory_;
private:
@ -3605,6 +3603,132 @@ class SafeBrowsingBlockingPageAsyncChecksTimingTest
base::test::ScopedFeatureList feature_list_;
};
class SafeBrowsingBlockingPageAsyncChecksTimingTest
: public SafeBrowsingBlockingPageAsyncChecksTimingTestBase,
public testing::WithParamInterface<bool> {
public:
SafeBrowsingBlockingPageAsyncChecksTimingTest() = default;
GURL SetupPostCommitInterstitialAndNavigate(
std::vector<UrlAndIsUnsafe> url_and_server_redirects,
base::OnceClosure report_sent_callback) {
// Call SetupUrlRealTimeVerdictInCacheManager with a random URL to ensure
// RealTimeUrlLookupServiceBase::CanCheckUrl returns true so the real time
// check is performed.
SetupUrlRealTimeVerdictInCacheManager(
GURL("https://random.url"), browser()->profile(), /*is_unsafe=*/false);
SetReportSentCallback(std::move(report_sent_callback));
bool check_complete_after_navigation_finish = GetParam();
if (check_complete_after_navigation_finish) {
return SetupWarningShownAfterFinishNavigationAndNavigate(
url_and_server_redirects);
} else {
return SetupWarningShownBetweenProcessResponseAndFinishNavigationAndNavigate(
url_and_server_redirects);
}
}
};
class SafeBrowsingBlockingPageAsyncChecksPrerenderingTest
: public SafeBrowsingBlockingPageAsyncChecksTimingTestBase {
public:
SafeBrowsingBlockingPageAsyncChecksPrerenderingTest() = default;
void SetUpCommandLine(base::CommandLine* command_line) override {
SafeBrowsingBlockingPageAsyncChecksTimingTestBase::SetUpCommandLine(
command_line);
// |prerender_helper_| has a ScopedFeatureList so we needed to delay its
// creation until now because
// SafeBrowsingBlockingPageAsyncChecksTimingTestBase also uses a
// ScopedFeatureList and initialization order matters.
prerender_helper_ = std::make_unique<
content::test::PrerenderTestHelper>(base::BindRepeating(
&SafeBrowsingBlockingPageAsyncChecksPrerenderingTest::GetWebContents,
base::Unretained(this)));
}
void SetUpOnMainThread() override {
prerender_helper_->RegisterServerRequestMonitor(embedded_test_server());
SafeBrowsingBlockingPageAsyncChecksTimingTestBase::SetUpOnMainThread();
}
content::test::PrerenderTestHelper& prerender_helper() {
return *prerender_helper_;
}
content::WebContents* GetWebContents() {
return browser()->tab_strip_model()->GetActiveWebContents();
}
private:
std::unique_ptr<content::test::PrerenderTestHelper> prerender_helper_;
};
// Test that prerendering doesn't affect the primary frame's threat report.
IN_PROC_BROWSER_TEST_F(
SafeBrowsingBlockingPageAsyncChecksPrerenderingTest,
PostCommitInterstitialReportThreatDetails_DontContainPrerenderingInfo) {
EnableAsyncCheck();
// Navigate to unsafe page, but don't yet return unsafe for the Safe Browsing
// lookup.
auto threat_report_sent_runner = std::make_unique<base::RunLoop>();
SetReportSentCallback(threat_report_sent_runner->QuitClosure());
// Call SetupUrlRealTimeVerdictInCacheManager with a random URL to ensure
// RealTimeUrlLookupServiceBase::CanCheckUrl returns true so the real time
// check is performed.
SetupUrlRealTimeVerdictInCacheManager(
GURL("https://random.url"), browser()->profile(), /*is_unsafe=*/false);
std::vector<UrlAndIsUnsafe> url_and_server_redirects = {
{kMaliciousPage, /* is_unsafe */ true}};
NavigateAndAwaitNavigationFinished(
url_and_server_redirects.front().relative_url);
// Set up prerendering.
GURL prerender_url = embedded_test_server()->GetURL("/title1.html");
SetupUrlRealTimeVerdictInCacheManager(prerender_url, browser()->profile(),
/*is_unsafe=*/false);
int host_id = prerender_helper().AddPrerender(prerender_url);
content::RenderFrameHost* prerender_render_frame_host =
prerender_helper().GetPrerenderedMainFrameHost(host_id);
EXPECT_NE(prerender_render_frame_host, nullptr);
EXPECT_EQ(prerender_url, prerender_render_frame_host->GetLastCommittedURL());
// Return unsafe for the Safe Browsing lookup, which displays a post-commit
// interstitial.
GURL url = ReturnUrlRealTimeVerdictsForUnsafeChain(url_and_server_redirects);
ThreatDetails* threat_details = details_factory_.get_details();
EXPECT_TRUE(threat_details != nullptr);
// Proceed through the warning.
EXPECT_TRUE(ClickAndWaitForDetach(browser(), "proceed-link"));
AssertNoInterstitial(browser()); // Assert the interstitial is gone
EXPECT_TRUE(IsExtendedReportingEnabled(*browser()->profile()->GetPrefs()));
EXPECT_EQ(url, browser()
->tab_strip_model()
->GetActiveWebContents()
->GetLastCommittedURL());
threat_report_sent_runner->Run();
std::string serialized = GetReportSent();
ClientSafeBrowsingReportRequest report;
ASSERT_TRUE(report.ParseFromString(serialized));
// Verify the report is complete.
EXPECT_TRUE(report.complete());
// The threat report should not contain the prerender information.
EXPECT_NE(prerender_url.spec(), report.page_url());
EXPECT_NE(prerender_url.spec(), report.url());
for (const auto& resource : report.resources()) {
EXPECT_NE(prerender_url.spec(), resource.url());
}
// We don't check the specific size of resources here. The size can be either
// 1 or 2 depending on whether DOM details have been collected when we
// proceed.
ASSERT_NE(0, report.resources_size());
}
INSTANTIATE_TEST_SUITE_P(CheckCompleteAfterNavigationFinish,
SafeBrowsingBlockingPageAsyncChecksTimingTest,
testing::Bool());
@ -4315,80 +4439,6 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingPrerenderBrowserTest, UnsafePrerender) {
/*PrerenderFinalStatus::kBlockedByClient=*/28, 1);
}
class SafeBrowsingThreatDetailsPrerenderBrowserTest
: public SafeBrowsingPrerenderBrowserTest {};
INSTANTIATE_TEST_SUITE_P(
All,
SafeBrowsingThreatDetailsPrerenderBrowserTest,
// We simulate a SBThreatType::SB_THREAT_TYPE_URL_CLIENT_SIDE_PHISHING to
// trigger DOM detail collection.
testing::Combine(
testing::Values(SBThreatType::SB_THREAT_TYPE_URL_CLIENT_SIDE_PHISHING),
testing::Bool())); // If isolate all sites for testing.
// Test that the prerendering doesn't affect on the primary's threat report.
IN_PROC_BROWSER_TEST_P(SafeBrowsingThreatDetailsPrerenderBrowserTest,
DontContainPrerenderingInfoInThreatReport) {
SetExtendedReportingPrefForTests(browser()->profile()->GetPrefs(), true);
const bool expect_threat_details =
SafeBrowsingBlockingPage::ShouldReportThreatDetails(GetThreatType());
auto threat_report_sent_runner = std::make_unique<base::RunLoop>();
if (expect_threat_details) {
SetReportSentCallback(threat_report_sent_runner->QuitClosure());
}
// Navigate to a safe page which contains multiple potential DOM details
// on the primary page. (Despite the name, kMaliciousPage is not the page
// flagged as bad in this test.)
GURL primary_url = embedded_test_server()->GetURL(kMaliciousPage);
ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), primary_url));
EXPECT_EQ(nullptr, details_factory_.get_details());
// Navigate to a different page on the prerendering page.
GURL prerender_url = embedded_test_server()->GetURL("/title1.html");
int host_id = prerender_helper().AddPrerender(prerender_url);
content::RenderFrameHost* prerender_render_frame_host =
prerender_helper().GetPrerenderedMainFrameHost(host_id);
EXPECT_NE(prerender_render_frame_host, nullptr);
EXPECT_EQ(prerender_url, prerender_render_frame_host->GetLastCommittedURL());
// Start navigation to bad page (kEmptyPage), which will be blocked before it
// is committed.
SetupWarningAndNavigate(browser());
ThreatDetails* threat_details = details_factory_.get_details();
EXPECT_EQ(expect_threat_details, threat_details != nullptr);
// Proceed through the warning.
EXPECT_TRUE(ClickAndWaitForDetach("proceed-link"));
AssertNoInterstitial(); // Assert the interstitial is gone
EXPECT_TRUE(IsExtendedReportingEnabled(*browser()->profile()->GetPrefs()));
EXPECT_EQ(primary_url, browser()
->tab_strip_model()
->GetActiveWebContents()
->GetLastCommittedURL());
if (expect_threat_details) {
threat_report_sent_runner->Run();
std::string serialized = GetReportSent();
ClientSafeBrowsingReportRequest report;
ASSERT_TRUE(report.ParseFromString(serialized));
// Verify the report is complete.
EXPECT_TRUE(report.complete());
// The threat report should not contain the prerender information.
EXPECT_NE(prerender_url.spec(), report.page_url());
EXPECT_NE(prerender_url.spec(), report.url());
ASSERT_EQ(3, report.resources_size());
for (const auto& resource : report.resources()) {
EXPECT_NE(prerender_url.spec(), resource.url());
}
}
}
class SafeBrowsingBlockingPageDelayedWarningPrerenderingBrowserTest
: public SafeBrowsingBlockingPageDelayedWarningBrowserTest {
public:

@ -134,7 +134,7 @@ class VerdictCacheManager : public history::HistoryServiceObserver,
private:
friend class ::SafeBrowsingServiceTest;
friend class SafeBrowsingBlockingPageAsyncChecksTest;
friend class SafeBrowsingBlockingPageAsyncChecksTestBase;
friend class SafeBrowsingBlockingPageRealTimeUrlCheckTest;
friend class SafeBrowsingBlockingPageHashRealTimeCheckTest;
friend class VerdictCacheManagerTest;

@ -236,7 +236,13 @@ constexpr base::FeatureParam<int> kReferrerChainEventMaximumCount{
BASE_FEATURE(kSafeBrowsingAsyncRealTimeCheck,
"SafeBrowsingAsyncRealTimeCheck",
base::FEATURE_DISABLED_BY_DEFAULT);
#if BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC) || BUILDFLAG(IS_LINUX) || \
BUILDFLAG(IS_CHROMEOS)
base::FEATURE_ENABLED_BY_DEFAULT
#else
base::FEATURE_DISABLED_BY_DEFAULT
#endif
);
#if BUILDFLAG(IS_ANDROID)
BASE_FEATURE(kSafeBrowsingCallNewGmsApiOnStartup,

@ -17075,12 +17075,7 @@
"SafeBrowsingAsyncRealTimeCheck": [
{
"platforms": [
"android",
"chromeos",
"chromeos_lacros",
"linux",
"mac",
"windows"
"android"
],
"experiments": [
{