Remove cookies in CertificateReportingServiceFactory

This CL prepares the CertificateReportingServiceFactory for the launch
of Safe Browsing per-profile NetworkContexts. Since it does not need
cookies, we will be using the profile-wide SharedURLLoaderFactory, and
setting the credentials_mode to omit credentials.

Bug: 1049833
Change-Id: If479c178501f93523697248bc484e28ab515aa0b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2357465
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800386}

chrome/browser/net/chrome_report_sender.cc

@@ -88,6 +88,7 @@ void SendReport(
   resource_request->url = report_uri;
   resource_request->method = "POST";
   resource_request->load_flags = net::ReportSender::kLoadFlags;
+  resource_request->credentials_mode = network::mojom::CredentialsMode::kOmit;
 
   auto loader = network::SimpleURLLoader::Create(std::move(resource_request),
                                                  traffic_annotation);

chrome/browser/net/chrome_report_sender.h

@@ -17,7 +17,7 @@ class SharedURLLoaderFactory;
 }
 
 // Similar to net::ReportSender but uses network::SimpleURLLoader under the
-// hood.
+// hood. Reports sent with this function do not save or send credentials.
 void SendReport(
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
     net::NetworkTrafficAnnotationTag traffic_annotation,

chrome/browser/net/chrome_report_sender_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/net/chrome_report_sender.h"
+
+#include <vector>
+
+#include "base/bind_helpers.h"
+#include "base/memory/scoped_refptr.h"
+#include "content/public/test/browser_task_environment.h"
+#include "net/base/load_flags.h"
+#include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
+#include "services/network/public/cpp/resource_request.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/mojom/url_loader.mojom.h"
+#include "services/network/public/mojom/url_response_head.mojom.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+// A fake SharedURLLoaderFactory that always returns 200 OK.
+class FakeSharedURLLoaderFactory : public network::SharedURLLoaderFactory {
+ public:
+  FakeSharedURLLoaderFactory() = default;
+
+  const std::vector<network::ResourceRequest> resource_requests() {
+    return resource_requests_;
+  }
+
+ private:
+  ~FakeSharedURLLoaderFactory() override = default;
+
+  // network::SharedURLLoaderFactory
+  void CreateLoaderAndStart(
+      mojo::PendingReceiver<network::mojom::URLLoader> receiver,
+      int32_t routing_id,
+      int32_t request_id,
+      uint32_t options,
+      const network::ResourceRequest& url_request,
+      mojo::PendingRemote<network::mojom::URLLoaderClient> client,
+      const net::MutableNetworkTrafficAnnotationTag& traffic_annotation)
+      override {
+    resource_requests_.push_back(url_request);
+    mojo::Remote<network::mojom::URLLoaderClient> client_remote(
+        std::move(client));
+    auto head = network::mojom::URLResponseHead::New();
+    head->headers =
+        net::HttpResponseHeaders::TryToCreate("HTTP/1.1 200 OK\n\n");
+    head->mime_type = "text/html";
+    client_remote->OnReceiveResponse(std::move(head));
+    client_remote->OnComplete(network::URLLoaderCompletionStatus());
+  }
+
+  void Clone(mojo::PendingReceiver<network::mojom::URLLoaderFactory> receiver)
+      override {
+    NOTREACHED();
+  }
+
+  std::unique_ptr<network::PendingSharedURLLoaderFactory> Clone() override {
+    NOTREACHED();
+    return nullptr;
+  }
+
+  std::vector<network::ResourceRequest> resource_requests_;
+};
+
+}  // namespace
+
+TEST(ChromeReportSenderTest, DoesNotSaveOrSendCookies) {
+  content::BrowserTaskEnvironment task_environment;
+  scoped_refptr<FakeSharedURLLoaderFactory> test_loader_factory =
+      base::MakeRefCounted<FakeSharedURLLoaderFactory>();
+  GURL report_url("https://example.com/");
+
+  base::RunLoop run_loop;
+  SendReport(test_loader_factory, TRAFFIC_ANNOTATION_FOR_TESTS, report_url,
+             "application/octet-stream", "report contents",
+             run_loop.QuitClosure(), base::DoNothing());
+  run_loop.Run();
+
+  ASSERT_EQ(test_loader_factory->resource_requests().size(), 1u);
+  EXPECT_EQ(test_loader_factory->resource_requests()[0].credentials_mode,
+            network::mojom::CredentialsMode::kOmit);
+}
+
+TEST(ChromeReportSenderTest, UploadsToUrl) {
+  content::BrowserTaskEnvironment task_environment;
+  scoped_refptr<FakeSharedURLLoaderFactory> test_loader_factory =
+      base::MakeRefCounted<FakeSharedURLLoaderFactory>();
+  GURL report_url("https://example.com/");
+
+  base::RunLoop run_loop;
+  SendReport(test_loader_factory, TRAFFIC_ANNOTATION_FOR_TESTS, report_url,
+             "application/octet-stream", "report contents",
+             run_loop.QuitClosure(), base::DoNothing());
+  run_loop.Run();
+
+  ASSERT_EQ(test_loader_factory->resource_requests().size(), 1u);
+  EXPECT_EQ(test_loader_factory->resource_requests()[0].url, report_url);
+}
+
+TEST(ChromeReportSenderTest, UsesPostMethod) {
+  content::BrowserTaskEnvironment task_environment;
+  scoped_refptr<FakeSharedURLLoaderFactory> test_loader_factory =
+      base::MakeRefCounted<FakeSharedURLLoaderFactory>();
+  GURL report_url("https://example.com/");
+
+  base::RunLoop run_loop;
+  SendReport(test_loader_factory, TRAFFIC_ANNOTATION_FOR_TESTS, report_url,
+             "application/octet-stream", "report contents",
+             run_loop.QuitClosure(), base::DoNothing());
+  run_loop.Run();
+
+  ASSERT_EQ(test_loader_factory->resource_requests().size(), 1u);
+  EXPECT_EQ(test_loader_factory->resource_requests()[0].method,
+            net::HttpRequestHeaders::kPostMethod);
+}
+
+TEST(ChromeReportSenderTest, SkipsCache) {
+  content::BrowserTaskEnvironment task_environment;
+  scoped_refptr<FakeSharedURLLoaderFactory> test_loader_factory =
+      base::MakeRefCounted<FakeSharedURLLoaderFactory>();
+  GURL report_url("https://example.com/");
+
+  base::RunLoop run_loop;
+  SendReport(test_loader_factory, TRAFFIC_ANNOTATION_FOR_TESTS, report_url,
+             "application/octet-stream", "report contents",
+             run_loop.QuitClosure(), base::DoNothing());
+  run_loop.Run();
+
+  ASSERT_EQ(test_loader_factory->resource_requests().size(), 1u);
+  EXPECT_EQ(test_loader_factory->resource_requests()[0].load_flags,
+            net::LOAD_BYPASS_CACHE | net::LOAD_DISABLE_CACHE);
+}

chrome/browser/net/trial_comparison_cert_verifier_controller_unittest.cc

@@ -347,7 +347,7 @@ TEST_F(TrialComparisonCertVerifierControllerTest, OfficialBuildTrialEnabled) {
   std::vector<std::string> full_reports;
   reporting_service_test_helper()->WaitForRequestsDestroyed(
       ReportExpectation::Successful({{"127.0.0.1", RetryStatus::NOT_RETRIED}}),
-      &full_reports);
+      &full_reports, nullptr);
 
   ASSERT_EQ(1U, full_reports.size());
 
@@ -449,7 +449,7 @@ TEST_F(TrialComparisonCertVerifierControllerTest,
   reporting_service_test_helper()->WaitForRequestsDestroyed(
       ReportExpectation::Successful({{"127.0.0.1", RetryStatus::NOT_RETRIED},
                                      {"127.0.0.2", RetryStatus::NOT_RETRIED}}),
-      &full_reports);
+      &full_reports, nullptr);
 
   ASSERT_EQ(2U, full_reports.size());
 

chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc

@@ -38,6 +38,7 @@
 #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/network/public/cpp/resource_request.h"
 #include "url/scheme_host_port.h"
 
 using certificate_reporting_test_utils::CertificateReportingServiceTestHelper;
@@ -717,4 +718,29 @@ IN_PROC_BROWSER_TEST_F(CertificateReportingServiceBrowserTest, Delayed_Reset) {
       2 /* submitted */, 0 /* failed */, 1 /* successful */, 0 /* dropped */);
 }
 
+IN_PROC_BROWSER_TEST_F(CertificateReportingServiceBrowserTest,
+                       OmitsCredentials) {
+  SetExpectedHistogramCountOnTeardown(0);
+
+  certificate_reporting_test_utils::SetCertReportingOptIn(
+      browser(), certificate_reporting_test_utils::EXTENDED_REPORTING_OPT_IN);
+  // Make all reports succeed.
+  test_helper()->SetFailureMode(certificate_reporting_test_utils::
+                                    ReportSendingResult::REPORTS_SUCCESSFUL);
+
+  // Trigger a report
+  std::vector<network::ResourceRequest> full_requests;
+  SendReport("report0");
+  test_helper()->WaitForRequestsDestroyed(
+      ReportExpectation::Successful({{"report0", RetryStatus::NOT_RETRIED}}),
+      nullptr, &full_requests);
+
+  ASSERT_EQ(full_requests.size(), 1u);
+  EXPECT_EQ(full_requests[0].credentials_mode,
+            network::mojom::CredentialsMode::kOmit);
+  // report0 was successfully submitted.
+  event_histogram_tester()->SetExpectedValues(
+      1 /* submitted */, 0 /* failed */, 1 /* successful */, 0 /* dropped */);
+}
+
 }  // namespace safe_browsing

chrome/browser/safe_browsing/certificate_reporting_service_factory.cc

@@ -5,12 +5,14 @@
 #include "chrome/browser/safe_browsing/certificate_reporting_service_factory.h"
 
 #include "base/bind_helpers.h"
+#include "base/feature_list.h"
 #include "base/time/default_clock.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/certificate_reporting_service.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/safe_browsing/core/features.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 
 namespace {
@@ -70,7 +72,7 @@ void CertificateReportingServiceFactory::SetServiceResetCallbackForTesting(
 
 void CertificateReportingServiceFactory::SetURLLoaderFactoryForTesting(
     scoped_refptr<network::SharedURLLoaderFactory> factory) {
-  url_loader_factory_ = factory;
+  test_url_loader_factory_ = factory;
 }
 
 CertificateReportingServiceFactory::CertificateReportingServiceFactory()
@@ -87,20 +89,27 @@ CertificateReportingServiceFactory::CertificateReportingServiceFactory()
 CertificateReportingServiceFactory::~CertificateReportingServiceFactory() {}
 
 KeyedService* CertificateReportingServiceFactory::BuildServiceInstanceFor(
-    content::BrowserContext* profile) const {
+    content::BrowserContext* browser_context) const {
   safe_browsing::SafeBrowsingService* safe_browsing_service =
       g_browser_process->safe_browsing_service();
   // In unit tests the safe browsing service can be null, if this happens,
   // return null instead of crashing.
   if (!safe_browsing_service)
     return nullptr;
+  Profile* profile = Profile::FromBrowserContext(browser_context);
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory;
+  if (test_url_loader_factory_.get()) {
+    url_loader_factory = test_url_loader_factory_;
+  } else if (base::FeatureList::IsEnabled(
+                 safe_browsing::kSafeBrowsingRemoveCookies)) {
+    url_loader_factory = profile->GetURLLoaderFactory();
+  } else {
+    url_loader_factory = safe_browsing_service->GetURLLoaderFactory();
+  }
   return new CertificateReportingService(
-      safe_browsing_service,
-      url_loader_factory_.get() ? url_loader_factory_
-                                : safe_browsing_service->GetURLLoaderFactory(),
-      static_cast<Profile*>(profile), server_public_key_,
-      server_public_key_version_, max_queued_report_count_, queued_report_ttl_,
-      clock_, service_reset_callback_);
+      safe_browsing_service, url_loader_factory, static_cast<Profile*>(profile),
+      server_public_key_, server_public_key_version_, max_queued_report_count_,
+      queued_report_ttl_, clock_, service_reset_callback_);
 }
 
 content::BrowserContext*

chrome/browser/safe_browsing/certificate_reporting_service_factory.h

@@ -61,7 +61,7 @@ class CertificateReportingServiceFactory
   base::TimeDelta queued_report_ttl_;
   size_t max_queued_report_count_;
   base::Callback<void()> service_reset_callback_;
-  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
+  scoped_refptr<network::SharedURLLoaderFactory> test_url_loader_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(CertificateReportingServiceFactory);
 };

chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc

@@ -16,6 +16,7 @@
 #include "content/public/test/test_utils.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "net/http/http_response_headers.h"
+#include "services/network/public/cpp/resource_request.h"
 #include "services/network/public/mojom/url_response_head.mojom.h"
 #include "services/network/test/test_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -50,13 +51,16 @@ std::string GetReportContents(const network::ResourceRequest& request,
 void WaitReports(
     certificate_reporting_test_utils::RequestObserver* observer,
     const certificate_reporting_test_utils::ReportExpectation& expectation,
-    std::vector<std::string>* full_reports) {
+    std::vector<std::string>* full_reports,
+    std::vector<network::ResourceRequest>* full_requests) {
   observer->Wait(expectation.num_reports());
   EXPECT_EQ(expectation.successful_reports, observer->successful_reports());
   EXPECT_EQ(expectation.failed_reports, observer->failed_reports());
   EXPECT_EQ(expectation.delayed_reports, observer->delayed_reports());
   if (full_reports)
     *full_reports = observer->full_reports();
+  if (full_requests)
+    *full_requests = observer->full_requests();
   observer->ClearObservedReports();
 }
 
@@ -88,13 +92,15 @@ void RequestObserver::Wait(unsigned int num_events_to_wait_for) {
   }
 }
 
-void RequestObserver::OnRequest(const std::string& serialized_report,
+void RequestObserver::OnRequest(const network::ResourceRequest& url_request,
+                                const std::string& serialized_report,
                                 ReportSendingResult report_type) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   CertificateErrorReport report;
   EXPECT_TRUE(report.InitializeFromString(serialized_report));
 
   full_reports_.push_back(serialized_report);
+  full_requests_.push_back(url_request);
 
   switch (report_type) {
     case REPORTS_SUCCESSFUL:
@@ -143,6 +149,11 @@ const std::vector<std::string>& RequestObserver::full_reports() const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   return full_reports_;
 }
+const std::vector<network::ResourceRequest>& RequestObserver::full_requests()
+    const {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  return full_requests_;
+}
 
 void RequestObserver::ClearObservedReports() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ -150,6 +161,7 @@ void RequestObserver::ClearObservedReports() {
   failed_reports_.clear();
   delayed_reports_.clear();
   full_reports_.clear();
+  full_requests_.clear();
 }
 
 ReportExpectation::ReportExpectation() {}
@@ -228,7 +240,8 @@ void CertificateReportingServiceTestHelper::ResumeDelayedRequest() {
   EXPECT_EQ(REPORTS_DELAY, expected_report_result_);
   if (delayed_client_) {
     SendResponse(std::move(delayed_client_), delayed_result_ == REPORTS_FAIL);
-    request_destroyed_observer_.OnRequest(delayed_report_, delayed_result_);
+    request_destroyed_observer_.OnRequest(delayed_request_, delayed_report_,
+                                          delayed_result_);
   }
 }
 
@@ -243,24 +256,28 @@ uint32_t CertificateReportingServiceTestHelper::server_public_key_version()
 
 void CertificateReportingServiceTestHelper::WaitForRequestsCreated(
     const ReportExpectation& expectation) {
-  WaitReports(&request_created_observer_, expectation, nullptr);
+  WaitReports(&request_created_observer_, expectation, nullptr, nullptr);
 }
 
 void CertificateReportingServiceTestHelper::WaitForRequestsCreated(
     const ReportExpectation& expectation,
-    std::vector<std::string>* full_reports) {
-  WaitReports(&request_created_observer_, expectation, full_reports);
+    std::vector<std::string>* full_reports,
+    std::vector<network::ResourceRequest>* full_requests) {
+  WaitReports(&request_created_observer_, expectation, full_reports,
+              full_requests);
 }
 
 void CertificateReportingServiceTestHelper::WaitForRequestsDestroyed(
     const ReportExpectation& expectation) {
-  WaitReports(&request_destroyed_observer_, expectation, nullptr);
+  WaitReports(&request_destroyed_observer_, expectation, nullptr, nullptr);
 }
 
 void CertificateReportingServiceTestHelper::WaitForRequestsDestroyed(
     const ReportExpectation& expectation,
-    std::vector<std::string>* full_reports) {
-  WaitReports(&request_destroyed_observer_, expectation, full_reports);
+    std::vector<std::string>* full_reports,
+    std::vector<network::ResourceRequest>* full_requests) {
+  WaitReports(&request_destroyed_observer_, expectation, full_reports,
+              full_requests);
 }
 
 void CertificateReportingServiceTestHelper::ExpectNoRequests(
@@ -308,12 +325,12 @@ void CertificateReportingServiceTestHelper::CreateLoaderAndStart(
     const net::MutableNetworkTrafficAnnotationTag& traffic_annotation) {
   const std::string serialized_report =
       GetReportContents(url_request, server_private_key_);
-  request_created_observer_.OnRequest(serialized_report,
+  request_created_observer_.OnRequest(url_request, serialized_report,
                                       expected_report_result_);
 
   if (expected_report_result_ == REPORTS_FAIL) {
     SendResponse(std::move(client), true);
-    request_destroyed_observer_.OnRequest(serialized_report,
+    request_destroyed_observer_.OnRequest(url_request, serialized_report,
                                           expected_report_result_);
     return;
   }
@@ -322,12 +339,13 @@ void CertificateReportingServiceTestHelper::CreateLoaderAndStart(
     DCHECK(!delayed_client_) << "Supports only one delayed request at a time";
     delayed_client_ = std::move(client);
     delayed_report_ = serialized_report;
+    delayed_request_ = url_request;
     delayed_result_ = expected_report_result_;
     return;
   }
 
   SendResponse(std::move(client), false);
-  request_destroyed_observer_.OnRequest(serialized_report,
+  request_destroyed_observer_.OnRequest(url_request, serialized_report,
                                         expected_report_result_);
 }
 

chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h

@@ -15,6 +15,7 @@
 #include "content/public/test/browser_task_environment.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
+#include "services/network/public/cpp/resource_request.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/network/public/mojom/url_loader.mojom-forward.h"
 
@@ -74,7 +75,8 @@ class RequestObserver {
 
   // Called when a request created or destroyed, depending on whichever one this
   // class observes.
-  void OnRequest(const std::string& serialized_report,
+  void OnRequest(const network::ResourceRequest& url_request,
+                 const std::string& serialized_report,
                  ReportSendingResult report_type);
 
   // These must be called on the UI thread.
@@ -82,6 +84,7 @@ class RequestObserver {
   const ObservedReportMap& failed_reports() const;
   const ObservedReportMap& delayed_reports() const;
   const std::vector<std::string>& full_reports() const;
+  const std::vector<network::ResourceRequest>& full_requests() const;
   void ClearObservedReports();
 
  private:
@@ -94,6 +97,7 @@ class RequestObserver {
   ObservedReportMap delayed_reports_;
 
   std::vector<std::string> full_reports_;
+  std::vector<network::ResourceRequest> full_requests_;
 };
 
 // Class to wait for the CertificateReportingService to reset.
@@ -130,11 +134,15 @@ class CertificateReportingServiceTestHelper
   void ResumeDelayedRequest();
 
   void WaitForRequestsCreated(const ReportExpectation& expectation);
-  void WaitForRequestsCreated(const ReportExpectation& expectation,
-                              std::vector<std::string>* full_reports);
+  void WaitForRequestsCreated(
+      const ReportExpectation& expectation,
+      std::vector<std::string>* full_reports,
+      std::vector<network::ResourceRequest>* full_requests);
   void WaitForRequestsDestroyed(const ReportExpectation& expectation);
-  void WaitForRequestsDestroyed(const ReportExpectation& expectation,
-                                std::vector<std::string>* full_reports);
+  void WaitForRequestsDestroyed(
+      const ReportExpectation& expectation,
+      std::vector<std::string>* full_reports,
+      std::vector<network::ResourceRequest>* full_requests);
 
   // Checks that all requests are destroyed and that there are no in-flight
   // reports in |service|.
@@ -168,6 +176,7 @@ class CertificateReportingServiceTestHelper
 
   mojo::PendingRemote<network::mojom::URLLoaderClient> delayed_client_;
   std::string delayed_report_;
+  network::ResourceRequest delayed_request_;
   ReportSendingResult delayed_result_;
 
   RequestObserver request_created_observer_;

chrome/test/BUILD.gn

@@ -3347,6 +3347,7 @@ test("unit_tests") {
     "../browser/navigation_predictor/navigation_predictor_renderer_warmup_client_unittest.cc",
     "../browser/navigation_predictor/navigation_predictor_unittest.cc",
     "../browser/net/chrome_network_delegate_unittest.cc",
+    "../browser/net/chrome_report_sender_unittest.cc",
     "../browser/net/dns_probe_runner_unittest.cc",
     "../browser/net/dns_probe_service_factory_unittest.cc",
     "../browser/net/file_downloader_unittest.cc",