Use proto in ext inline install request and append recent navigations

To help diagnose the broken referrer chain problem to extension inline
install, we want to use Proto instead of JSON as the content format,
such that we can easily add more info to the request.

In addition, when referrer chain appears broken (completely empty, or
missing some entries), we are going to append recent navigation events
(quantity controlled by Finch) and send them for SBER/SCOUT users when
they are not in incognito.

Bug: 780532
Change-Id: I37ee0e6bf7a5e295640fca3eee86cb249595c7f5
Reviewed-on: https://chromium-review.googlesource.com/850726Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Commit-Queue: Jialiu Lin <jialiul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533146}

chrome/browser/extensions/BUILD.gn

@@ -849,6 +849,8 @@ static_library("extensions") {
     "//components/proxy_config",
     "//components/rappor",
     "//components/resources",
+    "//components/safe_browsing:csd_proto",
+    "//components/safe_browsing:features",
     "//components/safe_browsing/common:safe_browsing_prefs",
     "//components/safe_browsing/db:database_manager",
     "//components/search_engines",

chrome/browser/extensions/webstore_data_fetcher.cc

@@ -7,8 +7,10 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/metrics/field_trial_params.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/webstore_data_fetcher_delegate.h"
+#include "components/safe_browsing/features.h"
 #include "content/public/common/service_manager_connection.h"
 #include "extensions/common/extension_urls.h"
 #include "net/base/load_flags.h"
@@ -35,18 +37,22 @@ WebstoreDataFetcher::WebstoreDataFetcher(
       referrer_url_(referrer_url),
       id_(webstore_item_id),
       max_auto_retries_(0) {
+  upload_content_type_ =
+      base::FeatureList::IsEnabled(safe_browsing::kAppendRecentNavigationEvents)
+          ? "application/octet-stream"
+          : "application/json";
 }
 
 WebstoreDataFetcher::~WebstoreDataFetcher() {}
 
-void WebstoreDataFetcher::SetJsonPostData(const std::string& json) {
-  json_post_data_ = json;
+void WebstoreDataFetcher::SetPostData(const std::string& data) {
+  post_data_ = data;
 }
 
 void WebstoreDataFetcher::Start() {
   GURL webstore_data_url(extension_urls::GetWebstoreItemJsonDataURL(id_));
   net::URLFetcher::RequestType request_type =
-      json_post_data_.empty() ? net::URLFetcher::GET : net::URLFetcher::POST;
+      post_data_.empty() ? net::URLFetcher::GET : net::URLFetcher::POST;
   net::NetworkTrafficAnnotationTag traffic_annotation =
       net::DefineNetworkTrafficAnnotation("webstore_data_fetcher", R"(
         semantics {
@@ -84,10 +90,8 @@ void WebstoreDataFetcher::Start() {
   webstore_data_url_fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SAVE_COOKIES |
                                            net::LOAD_DISABLE_CACHE);
 
-  if (!json_post_data_.empty()) {
-    webstore_data_url_fetcher_->SetUploadData("application/json",
-                                              json_post_data_);
-  }
+  if (!post_data_.empty())
+    webstore_data_url_fetcher_->SetUploadData(upload_content_type_, post_data_);
 
   if (max_auto_retries_ > 0) {
     webstore_data_url_fetcher_->SetMaxRetriesOn5xx(max_auto_retries_);

chrome/browser/extensions/webstore_data_fetcher.h

@@ -37,9 +37,9 @@ class WebstoreDataFetcher : public base::SupportsWeakPtr<WebstoreDataFetcher>,
                       const std::string webstore_item_id);
   ~WebstoreDataFetcher() override;
 
-  // Makes this request use a POST instead of GET, and sends |json| in the
-  // body of the request. If |json| is empty, this is a no-op.
-  void SetJsonPostData(const std::string& json);
+  // Makes this request use a POST instead of GET, and sends |data| in the
+  // body of the request. If |data| is empty, this is a no-op.
+  void SetPostData(const std::string& data);
 
   void Start();
 
@@ -47,6 +47,8 @@ class WebstoreDataFetcher : public base::SupportsWeakPtr<WebstoreDataFetcher>,
     max_auto_retries_ = max_retries;
   }
 
+  std::string upload_content_type() const { return upload_content_type_; }
+
  private:
   void OnJsonParseSuccess(std::unique_ptr<base::Value> parsed_json);
   void OnJsonParseFailure(const std::string& error);
@@ -58,7 +60,8 @@ class WebstoreDataFetcher : public base::SupportsWeakPtr<WebstoreDataFetcher>,
   net::URLRequestContextGetter* request_context_;
   GURL referrer_url_;
   std::string id_;
-  std::string json_post_data_;
+  std::string post_data_;
+  std::string upload_content_type_;
 
   // For fetching webstore JSON data.
   std::unique_ptr<net::URLFetcher> webstore_data_url_fetcher_;

chrome/browser/extensions/webstore_inline_installer.cc

@@ -18,6 +18,7 @@
 #include "chrome/browser/ui/exclusive_access/fullscreen_controller.h"
 #include "chrome/common/pref_names.h"
 #include "components/prefs/pref_service.h"
+#include "components/safe_browsing/proto/csd.pb.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
@@ -118,7 +119,10 @@ bool WebstoreInlineInstaller::SafeBrowsingNavigationEventsEnabled() const {
   return SafeBrowsingNavigationObserverManager::IsEnabledAndReady(profile());
 }
 
-std::string WebstoreInlineInstaller::GetJsonPostData() {
+std::string WebstoreInlineInstaller::GetPostData(
+    const std::string& upload_content_type) {
+  DCHECK(upload_content_type == "application/octet-stream" ||
+         upload_content_type == "application/json");
   // web_contents() might return null during tab destruction. This object would
   // also be destroyed shortly thereafter but check to be on the safe side.
   if (!web_contents())
@@ -129,72 +133,10 @@ std::string WebstoreInlineInstaller::GetJsonPostData() {
   if (!profile()->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled))
     return std::string();
 
-  auto redirect_chain = std::make_unique<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());
-      }
-    }
-  }
-
-  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();
+  if (upload_content_type == "application/json")
+    return GetJsonPostData();
+  else
+    return GetProtoPostData();
 }
 
 bool WebstoreInlineInstaller::CheckRequestorAlive() const {
@@ -305,6 +247,91 @@ void WebstoreInlineInstaller::WebContentsDestroyed() {
   AbortInstall();
 }
 
+std::string WebstoreInlineInstaller::GetJsonPostData() {
+  auto redirect_chain = base::MakeUnique<base::ListValue>();
+
+  if (SafeBrowsingNavigationEventsEnabled()) {
+    scoped_refptr<SafeBrowsingNavigationObserverManager>
+        navigation_observer_manager = g_browser_process->safe_browsing_service()
+                                          ->navigation_observer_manager();
+
+    ReferrerChain referrer_chain;
+    SafeBrowsingNavigationObserverManager::AttributionResult result =
+        navigation_observer_manager->IdentifyReferrerChainByWebContents(
+            web_contents(), kExtensionReferrerUserGestureLimit,
+            &referrer_chain);
+    if (result !=
+        SafeBrowsingNavigationObserverManager::NAVIGATION_EVENT_NOT_FOUND) {
+      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());
+      }
+    }
+  }
+
+  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();
+}
+
+std::string WebstoreInlineInstaller::GetProtoPostData() {
+  if (!SafeBrowsingNavigationEventsEnabled())
+    return std::string();
+
+  scoped_refptr<SafeBrowsingNavigationObserverManager>
+      navigation_observer_manager = g_browser_process->safe_browsing_service()
+                                        ->navigation_observer_manager();
+
+  ReferrerChain referrer_chain;
+  SafeBrowsingNavigationObserverManager::AttributionResult result =
+      navigation_observer_manager->IdentifyReferrerChainByWebContents(
+          web_contents(), kExtensionReferrerUserGestureLimit, &referrer_chain);
+
+  // If the referrer chain is incomplete we'll append most recent navigations
+  // to referrer chain for diagnose purpose. This only happens if user is not
+  // in incognito mode and has opted into extended reporting to Scout reporting.
+  int recent_navigations_to_collect =
+      SafeBrowsingNavigationObserverManager::CountOfRecentNavigationsToAppend(
+          *profile(), result);
+  navigation_observer_manager->AppendRecentNavigations(
+      recent_navigations_to_collect, &referrer_chain);
+  safe_browsing::ExtensionWebStoreInstallRequest request;
+  request.mutable_referrer_chain()->Swap(&referrer_chain);
+  request.mutable_referrer_chain_options()->set_recent_navigations_to_collect(
+      recent_navigations_to_collect);
+
+  return request.SerializeAsString();
+}
+
 // static
 bool WebstoreInlineInstaller::IsRequestorURLInVerifiedSite(
     const GURL& requestor_url,

chrome/browser/extensions/webstore_inline_installer.h

@@ -53,7 +53,7 @@ class WebstoreInlineInstaller : public WebstoreStandaloneInstaller,
   virtual bool SafeBrowsingNavigationEventsEnabled() const;
 
   // Implementations WebstoreStandaloneInstaller Template Method's hooks.
-  std::string GetJsonPostData() override;
+  std::string GetPostData(const std::string& upload_content_type) override;
   bool CheckRequestorAlive() const override;
   const GURL& GetRequestorURL() const override;
   bool ShouldShowPostInstallUI() const override;
@@ -72,6 +72,11 @@ class WebstoreInlineInstaller : public WebstoreStandaloneInstaller,
       content::NavigationHandle* navigation_handle) override;
   void WebContentsDestroyed() override;
 
+  // Get Json string that contains extension install referrer chain info.
+  std::string GetJsonPostData();
+  // Get serialized ExtensionWebStoreInstallRequest protobuf.
+  std::string GetProtoPostData();
+
   // Checks whether the install is initiated by a page in a verified site
   // (which is at least a domain, but can also have a port or a path).
   static bool IsRequestorURLInVerifiedSite(const GURL& requestor_url,

chrome/browser/extensions/webstore_inline_installer_browsertest.cc

@@ -25,6 +25,8 @@
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/prefs/pref_service.h"
+#include "components/safe_browsing/features.h"
+#include "components/safe_browsing/proto/csd.pb.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test_utils.h"
 #include "extensions/browser/extension_registry.h"
@@ -408,22 +410,35 @@ class WebstoreInlineInstallerRedirectTest
   void ProcessServerRequest(
       const net::test_server::HttpRequest& request) override {
     cws_request_received_ = true;
+    if (request.content.empty())
+      return;
+
     if (request.content.find("redirect_chain") != std::string::npos) {
       std::unique_ptr<base::Value> contents =
           base::JSONReader::Read(request.content);
       ASSERT_EQ(base::Value::Type::DICTIONARY, contents->type());
       cws_request_json_data_ = base::DictionaryValue::From(std::move(contents));
+    } else {
+      cws_request_proto_ =
+          base::MakeUnique<safe_browsing::ExtensionWebStoreInstallRequest>();
+      if (!cws_request_proto_->ParseFromString(request.content))
+        cws_request_proto_.reset();
     }
   }
 
   bool cws_request_received_;
   std::unique_ptr<base::DictionaryValue> cws_request_json_data_;
+  std::unique_ptr<safe_browsing::ExtensionWebStoreInstallRequest>
+      cws_request_proto_;
 };
 
 // Test that an install from a page arrived at via redirects includes the
 // redirect information in the webstore request.
 IN_PROC_BROWSER_TEST_P(WebstoreInlineInstallerRedirectTest,
-                       IncludesRedirectData) {
+                       IncludesRedirectJsonData) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndDisableFeature(
+      safe_browsing::kAppendRecentNavigationEvents);
   const bool using_safe_browsing_tracker = GetParam();
   WebContents* web_contents =
       browser()->tab_strip_model()->GetActiveWebContents();
@@ -456,6 +471,7 @@ IN_PROC_BROWSER_TEST_P(WebstoreInlineInstallerRedirectTest,
 
   EXPECT_TRUE(cws_request_received_);
   ASSERT_NE(nullptr, cws_request_json_data_);
+  ASSERT_EQ(nullptr, cws_request_proto_);
 
   base::ListValue* redirect_list = nullptr;
   cws_request_json_data_->GetList("redirect_chain", &redirect_list);
@@ -480,6 +496,68 @@ IN_PROC_BROWSER_TEST_P(WebstoreInlineInstallerRedirectTest,
   }
 }
 
+// Test that an install from a page arrived at via redirects includes the
+// redirect information in the webstore request.
+IN_PROC_BROWSER_TEST_P(WebstoreInlineInstallerRedirectTest,
+                       IncludesRedirectProtoData) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeatureWithParameters(
+      safe_browsing::kAppendRecentNavigationEvents,
+      {{"recent_navigation_count", "3"}});
+  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);
+
+  net::HostPortPair host_port = embedded_test_server()->host_port_pair();
+
+  std::string final_url =
+      GenerateTestServerUrl(kAppDomain, "install.html").spec();
+  std::string redirect_url =
+      base::StringPrintf("http://%s:%d/server-redirect?%s", kRedirect2Domain,
+                         host_port.port(), final_url.c_str());
+  std::string install_url =
+      base::StringPrintf("http://%s:%d/server-redirect?%s", kRedirect1Domain,
+                         host_port.port(), redirect_url.c_str());
+  AutoAcceptInstall();
+  ui_test_utils::NavigateToURL(browser(), GURL(install_url));
+
+  RunTestAsync("runTest");
+  while (!ProgrammableInstallPrompt::Ready())
+    base::RunLoop().RunUntilIdle();
+  web_contents->Close();
+
+  EXPECT_TRUE(cws_request_received_);
+  ASSERT_EQ(nullptr, cws_request_json_data_);
+  if (!using_safe_browsing_tracker) {
+    ASSERT_EQ(nullptr, cws_request_proto_);
+    return;
+  }
+
+  ASSERT_NE(nullptr, cws_request_proto_);
+  ASSERT_EQ(1, cws_request_proto_->referrer_chain_size());
+
+  safe_browsing::ReferrerChainEntry referrer_entry =
+      cws_request_proto_->referrer_chain(0);
+
+  // Check that the expected domains are in the redirect list.
+  const std::set<std::string> expected_redirect_domains = {
+      kRedirect1Domain, kRedirect2Domain, kAppDomain};
+
+  EXPECT_EQ(final_url, referrer_entry.url());
+  EXPECT_EQ(safe_browsing::ReferrerChainEntry::CLIENT_REDIRECT,
+            referrer_entry.type());
+  EXPECT_EQ(3, referrer_entry.server_redirect_chain_size());
+  EXPECT_EQ(install_url, referrer_entry.server_redirect_chain(0).url());
+  EXPECT_EQ(redirect_url, referrer_entry.server_redirect_chain(1).url());
+  EXPECT_EQ(final_url, referrer_entry.server_redirect_chain(2).url());
+  EXPECT_TRUE(cws_request_proto_->referrer_chain_options()
+                  .has_recent_navigations_to_collect());
+}
+
 // Test that an install from a page arrived at via redirects does not include
 // redirect information when SafeBrowsing is disabled.
 IN_PROC_BROWSER_TEST_F(WebstoreInlineInstallerRedirectTest,
@@ -510,6 +588,7 @@ IN_PROC_BROWSER_TEST_F(WebstoreInlineInstallerRedirectTest,
 
   EXPECT_TRUE(cws_request_received_);
   ASSERT_EQ(nullptr, cws_request_json_data_);
+  ASSERT_EQ(nullptr, cws_request_proto_);
 }
 
 INSTANTIATE_TEST_CASE_P(NetRedirectTracking,

chrome/browser/extensions/webstore_standalone_installer.cc

@@ -69,9 +69,8 @@ void WebstoreStandaloneInstaller::BeginInstall() {
       GetRequestorURL(),
       id_));
 
-  std::string json_post_data = GetJsonPostData();
-  if (!json_post_data.empty())
-    webstore_data_fetcher_->SetJsonPostData(json_post_data);
+  webstore_data_fetcher_->SetPostData(
+      GetPostData(webstore_data_fetcher_->upload_content_type()));
 
   webstore_data_fetcher_->Start();
 }
@@ -157,7 +156,8 @@ WebstoreStandaloneInstaller::GetLocalizedExtensionForDisplay() {
   return localized_extension_for_display_.get();
 }
 
-std::string WebstoreStandaloneInstaller::GetJsonPostData() {
+std::string WebstoreStandaloneInstaller::GetPostData(
+    const std::string& upload_content_type_unused) {
   return std::string();
 }
 

chrome/browser/extensions/webstore_standalone_installer.h

@@ -88,9 +88,10 @@ class WebstoreStandaloneInstaller
   // Template Method's hooks to be implemented by subclasses.
 
   // Gives subclasses an opportunity to provide extra post data in the form of
-  // serialized JSON to the webstore data request before sending. The default
-  // implementation returns an empty string.
-  virtual std::string GetJsonPostData();
+  // serialized JSON or proto based on |upload_content_type| to the webstore
+  // data request before sending. The default implementation returns an empty
+  // string.
+  virtual std::string GetPostData(const std::string& upload_content_type);
 
   // Called at certain check points of the workflow to decide whether it makes
   // sense to proceed with installation. A requestor can be a website that

components/safe_browsing/proto/csd.proto

@@ -1146,3 +1146,18 @@ message NotificationImageReportRequest {
   // (even if the image URL is cross-origin). Otherwise a website could mislead
   // Safe Browsing into associating phishing image bitmaps with safe image URLs.
 }
+
+// Protobuf for Chrome extension webstore install request.
+message ExtensionWebStoreInstallRequest {
+  // If we can find the complete referrer chain, this field will contain URL
+  // transitions from landing referrer to event in reverse chronological
+  // order, i.e. event url comes first in this list, and landing referrer
+  // comes last.
+  // For Safe Browsing Extended Reporting or Scout users, if the referrer
+  // chain is empty or partially missing, we will add/append recent navigation
+  // events to this list. The type of these entries will be RECENT_NAVIGATION.
+  repeated ReferrerChainEntry referrer_chain = 1;
+
+  // Options and metadata about the above referrer chain.
+  optional ReferrerChainOptions referrer_chain_options = 2;
+}

testing/variations/fieldtrial_testing_config.json

@@ -2867,6 +2867,27 @@
             ]
         }
     ],
+    "ReferrerChainDiagnose": [
+        {
+            "platforms": [
+                "chromeos",
+                "linux",
+                "mac",
+                "win"
+            ],
+            "experiments": [
+                {
+                    "name": "Append5RecentNavigations",
+                    "params": {
+                        "recent_navigation_count": "5"
+                    },
+                    "enable_features": [
+                        "AppendRecentNavigationEvents"
+                    ]
+                }
+            ]
+        }
+    ],
     "RefreshTokenDeviceId": [
         {
             "platforms": [