Show Safety Tip on Safe Browsing delayed warnings

This CL adds a Finch feature to show a "Suspicious site" Safety Tip
when there is a Safe Browsing delayed warning, i.e. when a page is
known to be malicious but Chrome is delaying the Safe Browsing warning
until user interaction.

This will allow us to measure 2 things:
- How a Safety Tip influences user behavior on known-bad sites
- How omnibox UI treatments influence user behavior when there is
  something (a Safety Tip) drawing their attention to the omnibox

This is implemented by checking if a delayed warning is present (and
the relevant Finch feature is enabled) inside
ReputationService::GetReputationStatusWithEngagedSites().

There's a bit of trickiness with the Finch features. Delayed warning
Safety Tips are controlled with their own separate Finch feature, so
that they can be controlled independently of the regular Safety Tips
feature. This is so that we can have simultaneous experiments where
one enables regular Safety Tips and another enables delayed warning
Safety Tips. (We aren't allowed to run multiple simultaneous
experiments that both enable the same feature.) Running simultaneous
experiments like this could mean that we end up with some users in
both experiments, but this should be rare, and even rarer still that a
user encounters both different types of Safety Tips (regular and
delayed warnings), so this shouldn't affect our metrics.

For convenience, I've reused the suspicious sites Safety Tips variant
-- both the UI and the SafetyTipStatus -- rather than introduce a new
type of Safety Tip. This seemed okay to me because we're not really
actively using/studying the suspicious site variant, so I figured we
can reuse it for this purpose. This does mean that we'll want to make
sure to turn off any existing Finch config for the suspicious site
Safety Tip before we run delayed warnings Safety Tip experiments.

Bug: 1146471
Change-Id: Ie98a12d0f95b1e4313493697d218385edb58212a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2523674Reviewed-by: Daniel Rubery <drubery@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Reviewed-by: Joe DeBlasio <jdeblasio@chromium.org>
Commit-Queue: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#825418}

chrome/browser/reputation/reputation_service.cc

@@ -17,10 +17,12 @@
 #include "chrome/browser/profiles/incognito_helpers.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/reputation/local_heuristics.h"
+#include "chrome/browser/safe_browsing/user_interaction_observer.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/keyed_service/content/browser_context_keyed_service_factory.h"
 #include "components/lookalikes/core/lookalike_url_util.h"
 #include "components/reputation/core/safety_tips_config.h"
+#include "components/security_state/core/features.h"
 #include "components/security_state/core/security_state.h"
 #include "components/url_formatter/spoof_checks/top_domains/top500_domains.h"
 #include "services/metrics/public/cpp/ukm_source_id.h"
@@ -105,19 +107,23 @@ ReputationService* ReputationService::Get(Profile* profile) {
 }
 
 void ReputationService::GetReputationStatus(const GURL& url,
+                                            content::WebContents* web_contents,
                                             ReputationCheckCallback callback) {
   DCHECK(url.SchemeIsHTTPOrHTTPS());
 
   LookalikeUrlService* service = LookalikeUrlService::Get(profile_);
   if (service->EngagedSitesNeedUpdating()) {
-    service->ForceUpdateEngagedSites(
-        base::BindOnce(&ReputationService::GetReputationStatusWithEngagedSites,
-                       weak_factory_.GetWeakPtr(), url, std::move(callback)));
+    service->ForceUpdateEngagedSites(base::BindOnce(
+        &ReputationService::GetReputationStatusWithEngagedSites,
+        weak_factory_.GetWeakPtr(), url,
+        !!safe_browsing::SafeBrowsingUserInteractionObserver::FromWebContents(
+            web_contents),
+        std::move(callback)));
     // If the engaged sites need updating, there's nothing to do until callback.
     return;
   }
 
-  GetReputationStatusWithEngagedSites(url, std::move(callback),
+  GetReputationStatusWithEngagedSites(url, web_contents, std::move(callback),
                                       service->GetLatestEngagedSites());
 }
 
@@ -142,6 +148,7 @@ void ReputationService::SetSensitiveKeywordsForTesting(
 
 void ReputationService::GetReputationStatusWithEngagedSites(
     const GURL& url,
+    bool has_delayed_warning,
     ReputationCheckCallback callback,
     const std::vector<DomainInfo>& engaged_sites) {
   const DomainInfo navigated_domain = GetDomainInfo(url);
@@ -219,6 +226,25 @@ void ReputationService::GetReputationStatusWithEngagedSites(
     done_checking_reputation_status = true;
   }
 
+  // 6. This case is an experimental variation on Safe Browsing delayed warnings
+  // (https://crbug.com/1057157) to measure the effect of simplified domain
+  // display (https://crbug.com/1090393). In this experiment, Chrome delays Safe
+  // Browsing warnings until user interaction to see if the simplified domain
+  // display UI treatment affects how people interact with the page. In this
+  // variation, Chrome shows a Safety Tip on such pages, to try to isolate the
+  // effect of the UI treatment to when people's attention is drawn to the
+  // omnibox.
+  if (has_delayed_warning &&
+      base::FeatureList::IsEnabled(
+          security_state::features::kSafetyTipUIOnDelayedWarning)) {
+    // Intentionally don't check |done_checking_reputation_status| here, as we
+    // want this Safety Tip to take precedence. In this case, where there is a
+    // delayed Safe Browsing warning, we know the page is actually suspicious.
+    result.safety_tip_status = SafetyTipStatus::kBadReputation;
+    result.triggered_heuristics.blocklist_heuristic_triggered = true;
+    done_checking_reputation_status = true;
+  }
+
   if (IsIgnored(url)) {
     if (result.safety_tip_status == SafetyTipStatus::kBadReputation) {
       result.safety_tip_status = SafetyTipStatus::kBadReputationIgnored;

chrome/browser/reputation/reputation_service.h

@@ -67,7 +67,9 @@ class ReputationService : public KeyedService {
   // will be called regardless of whether |url| is flagged or
   // not. (Specifically, |callback| will be called with SafetyTipStatus::kNone
   // if the url is not flagged).
-  void GetReputationStatus(const GURL& url, ReputationCheckCallback callback);
+  void GetReputationStatus(const GURL& url,
+                           content::WebContents* web_contents,
+                           ReputationCheckCallback callback);
 
   // Returns whether the user has dismissed a similar warning, and thus no
   // warning should be shown for the provided url.
@@ -90,9 +92,12 @@ class ReputationService : public KeyedService {
 
  private:
   // Callback once we have up-to-date |engaged_sites|. Performs checks on the
-  // navigated |url|. Displays the warning when needed.
+  // navigated |url|. |has_delayed_warning| is true if the relevant WebContents
+  // is currently delaying a Safe Browsing warning (an experiment described in
+  // https://crbug.com/1057157). Displays the Safety Tip warning when needed.
   void GetReputationStatusWithEngagedSites(
       const GURL& url,
+      bool has_delayed_warning,
       ReputationCheckCallback callback,
       const std::vector<DomainInfo>& engaged_sites);
 

chrome/browser/reputation/reputation_web_contents_observer.cc

@@ -196,10 +196,17 @@ void RecordSafetyTipStatusWithInitiatorOriginInfo(
 
 // Returns whether a safety tip should be shown, according to finch.
 bool IsSafetyTipEnabled(security_state::SafetyTipStatus status) {
-  if (!base::FeatureList::IsEnabled(security_state::features::kSafetyTipUI)) {
+  if (!security_state::IsSafetyTipUIFeatureEnabled()) {
     return false;
   }
-  if (status == security_state::SafetyTipStatus::kBadReputation) {
+  // Safety Tips can be enabled for Safe Browsing delayed warnings
+  // (https://crbug.com/1146471) or independently. When enabled independently,
+  // there are a variety of triggers that can be toggled: a suspicious site
+  // check and various lookalike URL checks. We check the suspicious site
+  // trigger here; lookalike URL parameters are checked when computing whether
+  // the URL is a lookalike.
+  if (status == security_state::SafetyTipStatus::kBadReputation &&
+      base::FeatureList::IsEnabled(security_state::features::kSafetyTipUI)) {
     return kEnableSuspiciousSiteChecks.Get();
   }
   return true;
@@ -303,10 +310,11 @@ void ReputationWebContentsObserver::MaybeShowSafetyTip(
 
   ReputationService* service = ReputationService::Get(profile_);
   service->GetReputationStatus(
-      url, base::BindOnce(
-               &ReputationWebContentsObserver::HandleReputationCheckResult,
-               weak_factory_.GetWeakPtr(), navigation_source_id,
-               called_from_visibility_check, record_ukm_if_tip_not_shown));
+      url, web_contents(),
+      base::BindOnce(
+          &ReputationWebContentsObserver::HandleReputationCheckResult,
+          weak_factory_.GetWeakPtr(), navigation_source_id,
+          called_from_visibility_check, record_ukm_if_tip_not_shown));
 }
 
 void ReputationWebContentsObserver::HandleReputationCheckResult(

chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc

@@ -43,6 +43,7 @@
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/views/page_info/page_info_bubble_view_base.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
@@ -74,6 +75,7 @@
 #include "components/security_interstitials/core/metrics_helper.h"
 #include "components/security_interstitials/core/unsafe_resource.h"
 #include "components/security_interstitials/core/urls.h"
+#include "components/security_state/core/features.h"
 #include "components/security_state/core/security_state.h"
 #include "components/strings/grit/components_strings.h"
 #include "components/unified_consent/pref_names.h"
@@ -1859,6 +1861,10 @@ class SafeBrowsingBlockingPageDelayedWarningBrowserTest
   SafeBrowsingBlockingPageDelayedWarningBrowserTest() = default;
 
   void SetUp() override {
+    std::vector<base::Feature> additional_enabled_features;
+    std::vector<base::Feature> additional_disabled_features;
+    GetAdditionalFeatures(&additional_enabled_features,
+                          &additional_disabled_features);
     if (warning_on_mouse_click_enabled()) {
       const std::map<std::string, std::string> parameters{{"mouse", "true"}};
       std::vector<base::test::ScopedFeatureList::FeatureAndParams>
@@ -1868,12 +1874,21 @@ class SafeBrowsingBlockingPageDelayedWarningBrowserTest
                                blink::features::kPortals, {}),
                            base::test::ScopedFeatureList::FeatureAndParams(
                                blink::features::kPortalsCrossOrigin, {})};
-      scoped_feature_list_.InitWithFeaturesAndParameters(enabled_features, {});
+      for (const auto& feature : additional_enabled_features) {
+        enabled_features.push_back(
+            base::test::ScopedFeatureList::FeatureAndParams(feature, {}));
+      }
+      scoped_feature_list_.InitWithFeaturesAndParameters(
+          enabled_features, additional_disabled_features);
     } else {
-      scoped_feature_list_.InitWithFeatures(
-          /*enabled_features=*/{kDelayedWarnings, blink::features::kPortals,
-                                blink::features::kPortalsCrossOrigin},
-          /*disabled_features=*/{});
+      std::vector<base::Feature> enabled_features = {
+          kDelayedWarnings, blink::features::kPortals,
+          blink::features::kPortalsCrossOrigin};
+      for (const auto& feature : additional_enabled_features) {
+        enabled_features.push_back(feature);
+      }
+      scoped_feature_list_.InitWithFeatures(enabled_features,
+                                            additional_disabled_features);
     }
     InProcessBrowserTest::SetUp();
   }
@@ -1975,6 +1990,11 @@ class SafeBrowsingBlockingPageDelayedWarningBrowserTest
   }
 
  protected:
+  // Subclasses can override to enable/disable features in SetUp().
+  virtual void GetAdditionalFeatures(
+      std::vector<base::Feature>* enabled_features,
+      std::vector<base::Feature>* disabled_features) {}
+
   // Initiates a download and waits for it to be completed or cancelled.
   static void DownloadAndWaitForNavigation(Browser* browser) {
     content::WebContents* contents =
@@ -2603,6 +2623,93 @@ INSTANTIATE_TEST_SUITE_P(
         testing::Values(false, true), /* IsolateAllSitesForTesting */
         testing::Values(false, true) /* Show warning on mouse click */));
 
+class SafeBrowsingBlockingPageDelayedWarningWithSafetyTipBrowserTest
+    : public SafeBrowsingBlockingPageDelayedWarningBrowserTest {
+ public:
+  SafeBrowsingBlockingPageDelayedWarningWithSafetyTipBrowserTest() = default;
+  SafeBrowsingBlockingPageDelayedWarningWithSafetyTipBrowserTest(
+      const SafeBrowsingBlockingPageDelayedWarningWithSafetyTipBrowserTest&) =
+      delete;
+  SafeBrowsingBlockingPageDelayedWarningWithSafetyTipBrowserTest& operator=(
+      const SafeBrowsingBlockingPageDelayedWarningWithSafetyTipBrowserTest&) =
+      delete;
+
+ protected:
+  void GetAdditionalFeatures(
+      std::vector<base::Feature>* enabled_features,
+      std::vector<base::Feature>* disabled_features) override {
+    enabled_features->push_back(
+        security_state::features::kSafetyTipUIOnDelayedWarning);
+    // Explicitly disable the main Safety Tip feature. This feature is used to
+    // enable Safety Tips independently of delayed warnings, so that we can
+    // have one experiment studying regular Safety Tips running at the same time
+    // as another experiment studying delayed warnings Safety Tips. We want to
+    // test that delayed warnings Safety Tips can be enabled independently
+    // without requiring the main Safety Tip feature as well.
+    disabled_features->push_back(security_state::features::kSafetyTipUI);
+  }
+};
+
+// Tests that when delayed warnings and Safety Tips for delayed warnings are
+// enabled, a safety tip is shown on page load, and then the delayed warning on
+// interaction.
+IN_PROC_BROWSER_TEST_P(
+    SafeBrowsingBlockingPageDelayedWarningWithSafetyTipBrowserTest,
+    ShowSafetyTipBeforeInterstitial) {
+  base::HistogramTester histograms;
+  NavigateAndAssertNoInterstitial();
+
+  // Check that a Safety Tip is showing.
+  EXPECT_EQ(PageInfoBubbleViewBase::BUBBLE_SAFETY_TIP,
+            PageInfoBubbleViewBase::GetShownBubbleType());
+  histograms.ExpectUniqueSample("Security.SafetyTips.SafetyTipShown",
+                                security_state::SafetyTipStatus::kBadReputation,
+                                1);
+
+  // After typing in the page, the delayed warning should show and the Safety
+  // Tip should be dismissed.
+  EXPECT_TRUE(TypeAndWaitForInterstitial(browser()));
+  EXPECT_EQ(PageInfoBubbleViewBase::BUBBLE_NONE,
+            PageInfoBubbleViewBase::GetShownBubbleType());
+}
+
+// Tests that when delayed warnings and Safety Tips for delayed warnings are
+// enabled, the safety tip for the delayed warning takes precedence over a
+// lookalike URL safety tip.
+IN_PROC_BROWSER_TEST_P(
+    SafeBrowsingBlockingPageDelayedWarningWithSafetyTipBrowserTest,
+    SafetyTipLookalike) {
+  base::HistogramTester histograms;
+  // Use a domain that is 1 edit distance away from a top 500 domain.
+  const GURL lookalike_url =
+      embedded_test_server()->GetURL("gooogle.com", "/iframe.html");
+  SetURLThreatType(lookalike_url, SB_THREAT_TYPE_URL_PHISHING);
+  ui_test_utils::NavigateToURL(browser(), lookalike_url);
+  AssertNoInterstitial(browser(), true);
+
+  // Check that a Safety Tip is showing and that is not recorded as a lookalike,
+  // but rather as a bad reputation safety tip (the type we use for delayed
+  // warnings).
+  EXPECT_EQ(PageInfoBubbleViewBase::BUBBLE_SAFETY_TIP,
+            PageInfoBubbleViewBase::GetShownBubbleType());
+  histograms.ExpectUniqueSample("Security.SafetyTips.SafetyTipShown",
+                                security_state::SafetyTipStatus::kBadReputation,
+                                1);
+
+  // After typing in the page, the delayed warning should show and the Safety
+  // Tip should be dismissed.
+  EXPECT_TRUE(TypeAndWaitForInterstitial(browser()));
+  EXPECT_EQ(PageInfoBubbleViewBase::BUBBLE_NONE,
+            PageInfoBubbleViewBase::GetShownBubbleType());
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    SafeBrowsingBlockingPageDelayedWarningWithSafetyTipBrowserTest,
+    SafeBrowsingBlockingPageDelayedWarningWithSafetyTipBrowserTest,
+    testing::Combine(
+        testing::Values(false, true), /* IsolateAllSitesForTesting */
+        testing::Values(false, true) /* Show warning on mouse click */));
+
 // Test that SafeBrowsingBlockingPage properly decodes IDN URLs that are
 // displayed.
 class SafeBrowsingBlockingPageIDNTest

components/page_info/page_info.cc

@@ -48,7 +48,6 @@
 #include "components/safe_browsing/content/password_protection/password_protection_service.h"
 #include "components/safe_browsing/core/proto/csd.pb.h"
 #include "components/security_interstitials/content/stateful_ssl_host_state_delegate.h"
-#include "components/security_state/core/features.h"
 #include "components/signin/public/identity_manager/account_info.h"
 #include "components/ssl_errors/error_info.h"
 #include "components/strings/grit/components_chromium_strings.h"
@@ -794,7 +793,7 @@ void PageInfo::ComputeUIInputs(const GURL& url) {
 
   safety_tip_info_ = visible_security_state.safety_tip_info;
 #if defined(OS_ANDROID)
-  if (base::FeatureList::IsEnabled(security_state::features::kSafetyTipUI)) {
+  if (security_state::IsSafetyTipUIFeatureEnabled()) {
     // identity_status_description_android_ is only displayed on Android when
     // the user taps "Details" link on the page info. Reuse the description from
     // page info UI.
@@ -1023,7 +1022,7 @@ void PageInfo::PresentSiteIdentity() {
   info.connection_status_description = UTF16ToUTF8(site_connection_details_);
   info.identity_status = site_identity_status_;
   info.safe_browsing_status = safe_browsing_status_;
-  if (base::FeatureList::IsEnabled(security_state::features::kSafetyTipUI)) {
+  if (security_state::IsSafetyTipUIFeatureEnabled()) {
     info.safety_tip_info = safety_tip_info_;
   }
 #if defined(OS_ANDROID)

components/security_state/core/features.cc

@@ -21,5 +21,8 @@ const base::Feature kLegacyTLSWarnings{"LegacyTLSWarnings",
 const base::Feature kSafetyTipUI{"SafetyTip",
                                  base::FEATURE_DISABLED_BY_DEFAULT};
 
+const base::Feature kSafetyTipUIOnDelayedWarning{
+    "SafetyTipUIOnDelayedWarning", base::FEATURE_DISABLED_BY_DEFAULT};
+
 }  // namespace features
 }  // namespace security_state

components/security_state/core/features.h

@@ -44,6 +44,12 @@ extern const base::Feature kLegacyTLSWarnings;
 COMPONENT_EXPORT(SECURITY_STATE_FEATURES)
 extern const base::Feature kSafetyTipUI;
 
+// This feature enables Safety Tip warnings on pages where there is a delayed
+// Safe Browsing warning. Has no effect unless safe_browsing::kDelayedWarnings
+// is also enabled. Can be enabled independently of kSafetyTipUI.
+COMPONENT_EXPORT(SECURITY_STATE_FEATURES)
+extern const base::Feature kSafetyTipUIOnDelayedWarning;
+
 }  // namespace features
 }  // namespace security_state
 

components/security_state/core/security_state.cc

@@ -328,4 +328,9 @@ bool ShouldShowDangerTriangleForWarningLevel() {
   return true;
 }
 
+bool IsSafetyTipUIFeatureEnabled() {
+  return base::FeatureList::IsEnabled(features::kSafetyTipUI) ||
+         base::FeatureList::IsEnabled(features::kSafetyTipUIOnDelayedWarning);
+}
+
 }  // namespace security_state

components/security_state/core/security_state.h

@@ -269,6 +269,10 @@ bool IsSHA1InChain(const VisibleSecurityState& visible_security_state);
 // info to danger triangle as part of an experiment (crbug.com/997972).
 bool ShouldShowDangerTriangleForWarningLevel();
 
+// Returns true if Safety Tip UI should be shown because a relevant field trial
+// is enabled.
+bool IsSafetyTipUIFeatureEnabled();
+
 }  // namespace security_state
 
 #endif  // COMPONENTS_SECURITY_STATE_CORE_SECURITY_STATE_H_