Use new SafeBrowsing redirect tracking code in CWS pings.

The SafeBrowsing code tracks more types of redirects than the /net code which strictly adheres to spec and so can drop various types of redirects which can obscure the referrer chain.

BUG=685905

Review-Url: https://codereview.chromium.org/2779643002
Cr-Commit-Position: refs/heads/master@{#468389}

chrome/browser/extensions/webstore_inline_installer.cc

@@ -9,8 +9,11 @@
 #include "base/json/json_writer.h"
 #include "base/strings/stringprintf.h"
 #include "base/values.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/webstore_data_fetcher.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h"
+#include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/exclusive_access/fullscreen_controller.h"
 #include "chrome/common/pref_names.h"
@@ -20,6 +23,14 @@
 #include "content/public/browser/web_contents.h"
 
 using content::WebContents;
+using safe_browsing::SafeBrowsingNavigationObserverManager;
+using safe_browsing::ReferrerChain;
+
+namespace {
+
+// The number of user gestures to trace back for CWS pings.
+const int kExtensionReferrerUserGestureLimit = 2;
+}
 
 namespace extensions {
 
@@ -103,6 +114,10 @@ bool WebstoreInlineInstaller::IsRequestorPermitted(
   return true;
 }
 
+bool WebstoreInlineInstaller::SafeBrowsingNavigationEventsEnabled() const {
+  return SafeBrowsingNavigationObserverManager::IsEnabledAndReady(profile());
+}
+
 std::string WebstoreInlineInstaller::GetJsonPostData() {
   // web_contents() might return null during tab destruction. This object would
   // also be destroyed shortly thereafter but check to be on the safe side.
@@ -114,32 +129,71 @@ std::string WebstoreInlineInstaller::GetJsonPostData() {
   if (!profile()->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled))
     return std::string();
 
-  content::NavigationController& navigation_controller =
-      web_contents()->GetController();
-  content::NavigationEntry* navigation_entry =
-      navigation_controller.GetLastCommittedEntry();
-
-  if (navigation_entry) {
-    const std::vector<GURL>& redirect_urls =
-        navigation_entry->GetRedirectChain();
-
-    if (!redirect_urls.empty()) {
-      base::DictionaryValue dictionary;
-      dictionary.SetString("id", id());
-      dictionary.SetString("referrer", requestor_url_.spec());
-      std::unique_ptr<base::ListValue> redirect_chain =
-          base::MakeUnique<base::ListValue>();
+  auto redirect_chain = base::MakeUnique<base::ListValue>();
+
+  if (SafeBrowsingNavigationEventsEnabled()) {
+    // If we have it, use the new referrer checker.
+    safe_browsing::SafeBrowsingService* safe_browsing_service =
+        g_browser_process->safe_browsing_service();
+    // May be null in some tests.
+    if (!safe_browsing_service)
+      return std::string();
+
+    scoped_refptr<SafeBrowsingNavigationObserverManager>
+        navigation_observer_manager =
+            safe_browsing_service->navigation_observer_manager();
+    // This may be null if the navigation observer manager feature is
+    // disabled by experiment.
+    if (!navigation_observer_manager)
+      return std::string();
+
+    ReferrerChain referrer_chain;
+    SafeBrowsingNavigationObserverManager::AttributionResult result =
+        navigation_observer_manager->IdentifyReferrerChainByWebContents(
+            web_contents(), kExtensionReferrerUserGestureLimit,
+            &referrer_chain);
+    if (result !=
+        SafeBrowsingNavigationObserverManager::NAVIGATION_EVENT_NOT_FOUND) {
+      // For now the CWS post data is JSON encoded. Consider moving it to a
+      // proto.
+      for (const auto& referrer_chain_entry : referrer_chain) {
+        // Referrer chain entries are a list of URLs in reverse chronological
+        // order, so the final URL is the last thing in the list and the initial
+        // landing page is the first thing in the list.
+        // Furthermore each entry may contain a series of server redirects
+        // stored in the same order.
+        redirect_chain->AppendString(referrer_chain_entry.url());
+        for (const auto& server_side_redirect :
+             referrer_chain_entry.server_redirect_chain()) {
+          redirect_chain->AppendString(server_side_redirect.url());
+        }
+      }
+    }
+  } else {
+    content::NavigationController& navigation_controller =
+        web_contents()->GetController();
+    content::NavigationEntry* navigation_entry =
+        navigation_controller.GetLastCommittedEntry();
+    if (navigation_entry) {
+      const std::vector<GURL>& redirect_urls =
+          navigation_entry->GetRedirectChain();
       for (const GURL& url : redirect_urls) {
         redirect_chain->AppendString(url.spec());
       }
-      dictionary.Set("redirect_chain", std::move(redirect_chain));
-
-      std::string json;
-      base::JSONWriter::Write(dictionary, &json);
-      return json;
     }
   }
 
+  if (!redirect_chain->empty()) {
+    base::DictionaryValue dictionary;
+    dictionary.SetString("id", id());
+    dictionary.SetString("referrer", requestor_url_.spec());
+    dictionary.Set("redirect_chain", std::move(redirect_chain));
+
+    std::string json;
+    base::JSONWriter::Write(dictionary, &json);
+    return json;
+  }
+
   return std::string();
 }
 

chrome/browser/extensions/webstore_inline_installer.h

@@ -49,6 +49,9 @@ class WebstoreInlineInstaller : public WebstoreStandaloneInstaller,
 
   ~WebstoreInlineInstaller() override;
 
+  // Returns whether to use the new navigation event tracker.
+  virtual bool SafeBrowsingNavigationEventsEnabled() const;
+
   // Implementations WebstoreStandaloneInstaller Template Method's hooks.
   std::string GetJsonPostData() override;
   bool CheckRequestorAlive() const override;

chrome/browser/extensions/webstore_inline_installer_browsertest.cc

@@ -118,7 +118,8 @@ class WebstoreInlineInstallerForTest : public WebstoreInlineInstaller {
                                  content::RenderFrameHost* host,
                                  const std::string& extension_id,
                                  const GURL& requestor_url,
-                                 const Callback& callback)
+                                 const Callback& callback,
+                                 bool enable_safebrowsing_redirects)
       : WebstoreInlineInstaller(
             contents,
             host,
@@ -139,6 +140,10 @@ class WebstoreInlineInstallerForTest : public WebstoreInlineInstaller {
     return WebstoreInlineInstaller::CheckRequestorAlive();
   }
 
+  bool SafeBrowsingNavigationEventsEnabled() const override {
+    return enable_safebrowsing_redirects_;
+  }
+
   // Tests that care about the actual arguments to the install callback can use
   // this to receive a copy in |install_result_target|.
   void set_install_result_target(
@@ -166,13 +171,22 @@ class WebstoreInlineInstallerForTest : public WebstoreInlineInstaller {
   // arguments.
   std::unique_ptr<InstallResult>* install_result_target_;
 
+  // This can be set by tests that want to use the new SafeBrowsing redirect
+  // tracker.
+  bool enable_safebrowsing_redirects_;
+
   ProgrammableInstallPrompt* programmable_prompt_;
 };
 
 class WebstoreInlineInstallerForTestFactory :
     public WebstoreInlineInstallerFactory {
  public:
-  WebstoreInlineInstallerForTestFactory() : last_installer_(nullptr) {}
+  WebstoreInlineInstallerForTestFactory()
+      : last_installer_(nullptr), enable_safebrowsing_redirects_(false) {}
+  explicit WebstoreInlineInstallerForTestFactory(
+      bool enable_safebrowsing_redirects)
+      : last_installer_(nullptr),
+        enable_safebrowsing_redirects_(enable_safebrowsing_redirects) {}
   ~WebstoreInlineInstallerForTestFactory() override {}
 
   WebstoreInlineInstallerForTest* last_installer() { return last_installer_; }
@@ -184,13 +198,16 @@ class WebstoreInlineInstallerForTestFactory :
       const GURL& requestor_url,
       const WebstoreStandaloneInstaller::Callback& callback) override {
     last_installer_ = new WebstoreInlineInstallerForTest(
-        contents, host, webstore_item_id, requestor_url, callback);
+        contents, host, webstore_item_id, requestor_url, callback,
+        enable_safebrowsing_redirects_);
     return last_installer_;
   }
 
  private:
   // The last installer that was created.
   WebstoreInlineInstallerForTest* last_installer_;
+
+  bool enable_safebrowsing_redirects_;
 };
 
 IN_PROC_BROWSER_TEST_F(WebstoreInlineInstallerTest,
@@ -373,7 +390,9 @@ IN_PROC_BROWSER_TEST_F(WebstoreInlineInstallerTest, DoubleInlineInstallTest) {
   RunTest("runTest");
 }
 
-class WebstoreInlineInstallerRedirectTest : public WebstoreInlineInstallerTest {
+class WebstoreInlineInstallerRedirectTest
+    : public WebstoreInlineInstallerTest,
+      public ::testing::WithParamInterface<bool> {
  public:
   WebstoreInlineInstallerRedirectTest() : cws_request_received_(false) {}
   ~WebstoreInlineInstallerRedirectTest() override {}
@@ -401,8 +420,16 @@ class WebstoreInlineInstallerRedirectTest : public WebstoreInlineInstallerTest {
 
 // Test that an install from a page arrived at via redirects includes the
 // redirect information in the webstore request.
-IN_PROC_BROWSER_TEST_F(WebstoreInlineInstallerRedirectTest,
+IN_PROC_BROWSER_TEST_P(WebstoreInlineInstallerRedirectTest,
                        IncludesRedirectData) {
+  const bool using_safe_browsing_tracker = GetParam();
+  WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  TabHelper* tab_helper = TabHelper::FromWebContents(web_contents);
+  WebstoreInlineInstallerForTestFactory* factory =
+      new WebstoreInlineInstallerForTestFactory(using_safe_browsing_tracker);
+  tab_helper->SetWebstoreInlineInstallerFactoryForTests(factory);
+
   // Hand craft a url that will cause the test server to issue redirects.
   const std::vector<std::string> redirects = {kRedirect1Domain,
                                               kRedirect2Domain};
@@ -419,7 +446,11 @@ IN_PROC_BROWSER_TEST_F(WebstoreInlineInstallerRedirectTest,
 
   AutoAcceptInstall();
   ui_test_utils::NavigateToURL(browser(), install_url);
-  RunTest("runTest");
+
+  RunTestAsync("runTest");
+  while (!ProgrammableInstallPrompt::Ready())
+    base::RunLoop().RunUntilIdle();
+  web_contents->Close();
 
   EXPECT_TRUE(cws_request_received_);
   ASSERT_NE(nullptr, cws_request_json_data_);
@@ -429,15 +460,21 @@ IN_PROC_BROWSER_TEST_F(WebstoreInlineInstallerRedirectTest,
   ASSERT_NE(nullptr, redirect_list);
 
   // Check that the expected domains are in the redirect list.
-  const std::vector<std::string> expected_redirect_domains = {
+  const std::set<std::string> expected_redirect_domains = {
       kRedirect1Domain, kRedirect2Domain, kAppDomain};
-  ASSERT_EQ(expected_redirect_domains.size(), redirect_list->GetSize());
-  int i = 0;
+
+  // The SafeBrowsing tracker has a much more liberal definition of "redirect"
+  // and it may (based on timing) pick up additional navigations that occur
+  // shortly before the navigation we mainly care about here. Be somewhat
+  // permissive in what we accept as redirect results.
+  ASSERT_LE(expected_redirect_domains.size(), redirect_list->GetSize());
+
   for (const auto& value : *redirect_list) {
     std::string value_string;
     ASSERT_TRUE(value.GetAsString(&value_string));
     GURL redirect_url(value_string);
-    EXPECT_EQ(expected_redirect_domains[i++], redirect_url.host());
+    EXPECT_TRUE(expected_redirect_domains.find(redirect_url.host()) !=
+                expected_redirect_domains.end());
   }
 }
 
@@ -473,6 +510,13 @@ IN_PROC_BROWSER_TEST_F(WebstoreInlineInstallerRedirectTest,
   ASSERT_EQ(nullptr, cws_request_json_data_);
 }
 
+INSTANTIATE_TEST_CASE_P(NetRedirectTracking,
+                        WebstoreInlineInstallerRedirectTest,
+                        testing::Values(false));
+INSTANTIATE_TEST_CASE_P(SafeBrowsingRedirectTracking,
+                        WebstoreInlineInstallerRedirectTest,
+                        testing::Values(true));
+
 class WebstoreInlineInstallerListenerTest : public WebstoreInlineInstallerTest {
  public:
   WebstoreInlineInstallerListenerTest() {}