Metric for whether a pop up navigated to a safe browsing site.

Introduces a new SafeBrowsingStatus field to Popup.Closed
indicating the safe browsing status of a pop-up's navigation.

This is checked using the sub resource filter observer's
OnSafeBrowsingChecksComplete callback, similar to
safe_browsing_triggered_popup_blocker.

BUG=1044601

Change-Id: I012fbbde60741d30bad74457ddd3058f2fd909eb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2085161
Commit-Queue: Justin Miron <justinmiron@google.com>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#747702}

chrome/browser/ui/blocked_content/popup_tracker.cc

@@ -33,12 +33,20 @@ PopupTracker::~PopupTracker() = default;
 PopupTracker::PopupTracker(content::WebContents* contents,
                            content::WebContents* opener)
     : content::WebContentsObserver(contents),
+      scoped_observer_(this),
       visibility_tracker_(
           base::DefaultTickClock::GetInstance(),
           contents->GetVisibility() != content::Visibility::HIDDEN),
       opener_source_id_(ukm::GetSourceIdForWebContentsDocument(opener)) {
   if (auto* popup_opener = PopupOpenerTabHelper::FromWebContents(opener))
     popup_opener->OnOpenedPopup(this);
+
+  auto* observer_manager =
+      subresource_filter::SubresourceFilterObserverManager::FromWebContents(
+          contents);
+  if (observer_manager) {
+    scoped_observer_.Add(observer_manager);
+  }
 }
 
 void PopupTracker::WebContentsDestroyed() {
@@ -74,6 +82,7 @@ void PopupTracker::WebContentsDestroyed() {
         .SetUserInitiatedClose(web_contents()->GetClosedByUserGesture())
         .SetTrusted(is_trusted_)
         .SetNumInteractions(capped_interactions)
+        .SetSafeBrowsingStatus(static_cast<int>(safe_browsing_status_))
         .Record(ukm::UkmRecorder::Get());
   }
 }
@@ -110,4 +119,29 @@ void PopupTracker::DidGetUserInteraction(
   num_interactions_++;
 }
 
+// This method will always be called before the DidFinishNavigation associated
+// with this handle.
+// The exception is a navigation restoring a page from back-forward cache --
+// in that case don't issue any requests, therefore we don't get any
+// safe browsing callbacks. See the comment above for the mitigation.
+void PopupTracker::OnSafeBrowsingChecksComplete(
+    content::NavigationHandle* navigation_handle,
+    const SafeBrowsingCheckResults& results) {
+  DCHECK(navigation_handle->IsInMainFrame());
+  safe_browsing_status_ = PopupSafeBrowsingStatus::kSafe;
+  for (const auto& result : results) {
+    if (result.threat_type ==
+            safe_browsing::SBThreatType::SB_THREAT_TYPE_URL_PHISHING ||
+        result.threat_type == safe_browsing::SBThreatType::
+                                  SB_THREAT_TYPE_URL_CLIENT_SIDE_PHISHING ||
+        result.threat_type ==
+            safe_browsing::SBThreatType::SB_THREAT_TYPE_SUBRESOURCE_FILTER)
+      safe_browsing_status_ = PopupSafeBrowsingStatus::kUnsafe;
+  }
+}
+
+void PopupTracker::OnSubresourceFilterGoingAway() {
+  scoped_observer_.RemoveAll();
+}
+
 WEB_CONTENTS_USER_DATA_KEY_IMPL(PopupTracker)

chrome/browser/ui/blocked_content/popup_tracker.h

@@ -7,7 +7,10 @@
 
 #include "base/macros.h"
 #include "base/optional.h"
+#include "base/scoped_observer.h"
 #include "base/time/time.h"
+#include "components/subresource_filter/content/browser/subresource_filter_observer.h"
+#include "components/subresource_filter/content/browser/subresource_filter_observer_manager.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 #include "services/metrics/public/cpp/ukm_source_id.h"
@@ -17,14 +20,23 @@ namespace content {
 class WebContents;
 }
 
-
 // This class tracks new popups, and is used to log metrics on the visibility
 // time of the first document in the popup.
 // TODO(csharrison): Consider adding more metrics like total visibility for the
 // lifetime of the WebContents.
 class PopupTracker : public content::WebContentsObserver,
-                     public content::WebContentsUserData<PopupTracker> {
+                     public content::WebContentsUserData<PopupTracker>,
+                     public subresource_filter::SubresourceFilterObserver {
  public:
+  // These values are persisted to logs. Entries should not be renumbered and
+  // numeric values should never be reused.
+  enum class PopupSafeBrowsingStatus {
+    kNoValue = 0,
+    kSafe = 1,
+    kUnsafe = 2,
+    kMaxValue = kUnsafe,
+  };
+
   static PopupTracker* CreateForWebContents(content::WebContents* contents,
                                             content::WebContents* opener);
   ~PopupTracker() override;
@@ -43,6 +55,16 @@ class PopupTracker : public content::WebContentsObserver,
   void OnVisibilityChanged(content::Visibility visibility) override;
   void DidGetUserInteraction(const blink::WebInputEvent::Type type) override;
 
+  // subresource_filter::SubresourceFilterObserver:
+  void OnSafeBrowsingChecksComplete(
+      content::NavigationHandle* navigation_handle,
+      const SafeBrowsingCheckResults& results) override;
+  void OnSubresourceFilterGoingAway() override;
+
+  ScopedObserver<subresource_filter::SubresourceFilterObserverManager,
+                 subresource_filter::SubresourceFilterObserver>
+      scoped_observer_;
+
   // Will be unset until the first navigation commits. Will be set to the total
   // time the contents was visible at commit time.
   base::Optional<base::TimeDelta> first_load_visible_time_start_;
@@ -62,6 +84,11 @@ class PopupTracker : public content::WebContentsObserver,
 
   bool is_trusted_ = false;
 
+  // Whether the pop-up navigated to a site on the safe browsing list. Set when
+  // the safe browsing checks complete.
+  PopupSafeBrowsingStatus safe_browsing_status_ =
+      PopupSafeBrowsingStatus::kNoValue;
+
   WEB_CONTENTS_USER_DATA_KEY_DECL();
 
   DISALLOW_COPY_AND_ASSIGN(PopupTracker);

chrome/browser/ui/blocked_content/popup_tracker_browsertest.cc

@@ -7,23 +7,29 @@
 #include <memory>
 #include <string>
 
+#include "base/path_service.h"
 #include "base/supports_user_data.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "build/build_config.h"
 #include "chrome/browser/content_settings/tab_specific_content_settings.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/safe_browsing/test_safe_browsing_database_helper.h"
 #include "chrome/browser/ui/blocked_content/popup_blocker_tab_helper.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "components/safe_browsing/core/db/v4_embedded_test_server_util.h"
+#include "components/safe_browsing/core/db/v4_test_util.h"
 #include "components/ukm/test_ukm_recorder.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.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"
 #include "services/metrics/public/cpp/ukm_builders.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -43,6 +49,7 @@ const char kUkmEngagementTime[] = "EngagementTime";
 const char kUkmUserInitiatedClose[] = "UserInitiatedClose";
 const char kUkmTrusted[] = "Trusted";
 const char kUkmNumInteractions[] = "NumInteractions";
+const char kUkmSafeBrowsingStatus[] = "SafeBrowsingStatus";
 }  // namespace
 
 using UkmEntry = ukm::builders::Popup_Closed;
@@ -300,3 +307,172 @@ IN_PROC_BROWSER_TEST_F(PopupTrackerBrowserTest, NoOpener_NoTracker) {
   EXPECT_FALSE(PopupTracker::FromWebContents(new_contents));
   EXPECT_EQ(0u, GetNumPopupUkmEntries());
 }
+
+// Tests for the subresource_filter popup blocker.
+class SafeBrowsingPopupTrackerBrowserTest : public PopupTrackerBrowserTest {
+ public:
+  SafeBrowsingPopupTrackerBrowserTest() = default;
+  ~SafeBrowsingPopupTrackerBrowserTest() override = default;
+
+  void SetUp() override {
+    database_helper_ = CreateTestDatabase();
+    PopupTrackerBrowserTest::SetUp();
+  }
+
+  void SetUpOnMainThread() override {
+    base::FilePath test_data_dir;
+    base::PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir);
+    embedded_test_server()->ServeFilesFromDirectory(test_data_dir);
+    host_resolver()->AddRule("*", "127.0.0.1");
+    content::SetupCrossSiteRedirector(embedded_test_server());
+
+    ASSERT_TRUE(embedded_test_server()->Start());
+  }
+
+  void TearDown() override {
+    InProcessBrowserTest::TearDown();
+    database_helper_.reset();
+  }
+
+  virtual std::unique_ptr<TestSafeBrowsingDatabaseHelper> CreateTestDatabase() {
+    std::vector<safe_browsing::ListIdentifier> list_ids = {
+        safe_browsing::GetUrlSubresourceFilterId()};
+    return std::make_unique<TestSafeBrowsingDatabaseHelper>(
+        std::make_unique<safe_browsing::TestV4GetHashProtocolManagerFactory>(),
+        std::move(list_ids));
+  }
+
+  void ConfigureAsList(const GURL& url,
+                       const safe_browsing::ListIdentifier& list_identifier) {
+    safe_browsing::ThreatMetadata metadata;
+    database_helper_->AddFullHashToDbAndFullHashCache(url, list_identifier,
+                                                      metadata);
+  }
+
+  TestSafeBrowsingDatabaseHelper* database_helper() {
+    return database_helper_.get();
+  }
+
+ private:
+  std::unique_ptr<TestSafeBrowsingDatabaseHelper> database_helper_;
+
+  DISALLOW_COPY_AND_ASSIGN(SafeBrowsingPopupTrackerBrowserTest);
+};
+
+// Pop-ups closed before navigation has finished will receive no safe browsing
+// status.
+IN_PROC_BROWSER_TEST_F(SafeBrowsingPopupTrackerBrowserTest,
+                       PopupClosedBeforeNavigationFinished_LoggedAsNoValue) {
+  const GURL first_url = embedded_test_server()->GetURL("/title1.html");
+  ui_test_utils::NavigateToURL(browser(), first_url);
+
+  const GURL unsafe_url = embedded_test_server()->GetURL("/slow");
+  ConfigureAsList(unsafe_url, safe_browsing::GetUrlSocEngId());
+
+  content::TestNavigationObserver navigation_observer(nullptr, 1);
+  navigation_observer.StartWatchingNewWebContents();
+
+  EXPECT_TRUE(
+      content::ExecJs(browser()->tab_strip_model()->GetActiveWebContents(),
+                      "window.open('/slow?1000')"));
+
+  EXPECT_EQ(2, browser()->tab_strip_model()->count());
+  content::WebContents* popup =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_TRUE(PopupTracker::FromWebContents(popup));
+
+  // Close the popup and check metric.
+  int active_index = browser()->tab_strip_model()->active_index();
+  content::WebContentsDestroyedWatcher destroyed_watcher(popup);
+  browser()->tab_strip_model()->CloseWebContentsAt(
+      active_index, TabStripModel::CLOSE_USER_GESTURE);
+  destroyed_watcher.Wait();
+
+  auto* entry = ExpectAndGetEntry(first_url);
+  test_ukm_recorder_->ExpectEntryMetric(
+      entry, kUkmSafeBrowsingStatus,
+      static_cast<int>(PopupTracker::PopupSafeBrowsingStatus::kNoValue));
+}
+
+IN_PROC_BROWSER_TEST_F(SafeBrowsingPopupTrackerBrowserTest,
+                       SafePopup_LoggedAsSafe) {
+  const GURL first_url = embedded_test_server()->GetURL("/title1.html");
+  ui_test_utils::NavigateToURL(browser(), first_url);
+
+  content::TestNavigationObserver navigation_observer(nullptr, 1);
+  navigation_observer.StartWatchingNewWebContents();
+
+  const GURL second_url = embedded_test_server()->GetURL("/title2.html");
+  EXPECT_TRUE(
+      content::ExecJs(browser()->tab_strip_model()->GetActiveWebContents(),
+                      "window.open('/title2.html')"));
+  navigation_observer.Wait();
+
+  EXPECT_EQ(2, browser()->tab_strip_model()->count());
+  content::WebContents* popup =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_TRUE(PopupTracker::FromWebContents(popup));
+
+  // Close the popup and check metric.
+  int active_index = browser()->tab_strip_model()->active_index();
+  content::WebContentsDestroyedWatcher destroyed_watcher(popup);
+  browser()->tab_strip_model()->CloseWebContentsAt(
+      active_index, TabStripModel::CLOSE_USER_GESTURE);
+  destroyed_watcher.Wait();
+
+  auto* entry = ExpectAndGetEntry(first_url);
+  test_ukm_recorder_->ExpectEntryMetric(
+      entry, kUkmSafeBrowsingStatus,
+      static_cast<int>(PopupTracker::PopupSafeBrowsingStatus::kSafe));
+}
+
+IN_PROC_BROWSER_TEST_F(SafeBrowsingPopupTrackerBrowserTest,
+                       PhishingPopup_LoggedAsUnsafe) {
+  const GURL first_url = embedded_test_server()->GetURL("/title1.html");
+  ui_test_utils::NavigateToURL(browser(), first_url);
+
+  // Associate each domain with a separate safe browsing ListIdentifier to
+  // exercise the set of lists.
+  std::vector<std::pair<std::string, safe_browsing::ListIdentifier>>
+      domain_list_pairs = {
+          {"a.com", safe_browsing::GetUrlSocEngId()},
+          {"b.com", safe_browsing::GetUrlSubresourceFilterId()}};
+
+  // For each pair, configure the local safe browsing database and open a
+  // pop-up to the url.
+  for (const auto& domain_list_pair : domain_list_pairs) {
+    const GURL unsafe_url =
+        embedded_test_server()->GetURL(domain_list_pair.first, "/title2.html");
+    ConfigureAsList(unsafe_url, domain_list_pair.second);
+
+    content::TestNavigationObserver navigation_observer(nullptr, 1);
+    navigation_observer.StartWatchingNewWebContents();
+
+    EXPECT_TRUE(
+        content::ExecJs(browser()->tab_strip_model()->GetActiveWebContents(),
+                        "window.open('" + unsafe_url.spec() + "')"));
+    navigation_observer.Wait();
+
+    EXPECT_EQ(2, browser()->tab_strip_model()->count());
+    content::WebContents* popup =
+        browser()->tab_strip_model()->GetActiveWebContents();
+    EXPECT_TRUE(PopupTracker::FromWebContents(popup));
+
+    // Close the popup and check metric.
+    int active_index = browser()->tab_strip_model()->active_index();
+    content::WebContentsDestroyedWatcher destroyed_watcher(popup);
+    browser()->tab_strip_model()->CloseWebContentsAt(
+        active_index, TabStripModel::CLOSE_USER_GESTURE);
+    destroyed_watcher.Wait();
+  }
+
+  // The URL should have 4 pop-up entries with an unsafe safe browsing status.
+  auto entries = test_ukm_recorder_->GetEntriesByName(
+      ukm::builders::Popup_Closed::kEntryName);
+  EXPECT_EQ(2u, entries.size());
+  for (auto* entry : entries) {
+    test_ukm_recorder_->ExpectEntryMetric(
+        entry, kUkmSafeBrowsingStatus,
+        static_cast<int>(PopupTracker::PopupSafeBrowsingStatus::kUnsafe));
+  }
+}

tools/metrics/histograms/enums.xml

@@ -51981,6 +51981,13 @@ Called by update_net_trust_anchors.py.-->
   <int value="3" label="Popup clicked through (abusive)"/>
 </enum>
 
+<enum name="PopupSafeBrowsingStatus">
+  <int value="0" label="Popup has no safe-browsing status"/>
+  <int value="1"
+      label="Popup does not navigate to an unsafe safe-browsing site"/>
+  <int value="2" label="Popup navigates to an unsafe safe-browsing site"/>
+</enum>
+
 <enum name="PortalDetectionMultiProbeResult">
   <int value="0" label="Undefined result"/>
   <int value="1" label="HTTPS traffic blocked, no HTTP redirect"/>

tools/metrics/ukm/ukm.xml

@@ -8359,6 +8359,11 @@ be describing additional metrics about the same event.
       start, or raw key down event.
     </summary>
   </metric>
+  <metric name="SafeBrowsingStatus" enum="PopupSafeBrowsingStatus">
+    <summary>
+      The site's safe browsing status if the pop-up's navigation commits.
+    </summary>
+  </metric>
   <metric name="Trusted">
     <summary>
       Boolean value representing whether the popup was opened via some trusted