Don't send or save cookies in V4GetHashProtocolManager

Cookies are not needed for full hash lookups, so we can stop sending and
saving them. This CL makes this change and adds a test that we don't save
cookies.

Bug: 1049833
Change-Id: Id32c3d4007ae7936da5ec7bbae543b819ae7bf63
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2122452
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Reviewed-by: Xinghui Lu <xinghuilu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#794637}

chrome/browser/safe_browsing/safe_browsing_service.cc

@@ -384,9 +384,9 @@ void SafeBrowsingService::OnProfileWillBeDestroyed(Profile* profile) {
 }
 
 void SafeBrowsingService::CreateServicesForProfile(Profile* profile) {
+  services_delegate_->CreateSafeBrowsingNetworkContext(profile);
   services_delegate_->CreatePasswordProtectionService(profile);
   services_delegate_->CreateTelemetryService(profile);
-  services_delegate_->CreateSafeBrowsingNetworkContext(profile);
   observed_profiles_.Add(profile);
 }
 

chrome/browser/safe_browsing/v4_embedded_test_server_browsertest.cc

@@ -7,6 +7,10 @@
 
 #include "base/feature_list.h"
 #include "base/macros.h"
+#include "base/test/scoped_feature_list.h"
+#include "base/time/time.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/in_process_browser_test.h"
@@ -20,9 +24,13 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/test_utils.h"
+#include "net/cookies/canonical_cookie.h"
 #include "net/dns/mapped_host_resolver.h"
 #include "net/dns/mock_host_resolver.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "services/network/public/mojom/network_context.mojom-forward.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
 
 namespace {
 
@@ -35,6 +43,25 @@ bool IsShowingInterstitial(content::WebContents* contents) {
           nullptr);
 }
 
+std::vector<net::CanonicalCookie> GetCookies(
+    network::mojom::NetworkContext* network_context) {
+  base::RunLoop run_loop;
+  std::vector<net::CanonicalCookie> cookies;
+  mojo::Remote<network::mojom::CookieManager> cookie_manager_remote;
+  network_context->GetCookieManager(
+      cookie_manager_remote.BindNewPipeAndPassReceiver());
+  cookie_manager_remote->GetAllCookies(base::BindOnce(
+      [](base::RunLoop* run_loop,
+         std::vector<net::CanonicalCookie>* out_cookies,
+         const std::vector<net::CanonicalCookie>& cookies) {
+        *out_cookies = cookies;
+        run_loop->Quit();
+      },
+      &run_loop, &cookies));
+  run_loop.Run();
+  return cookies;
+}
+
 }  // namespace
 
 namespace safe_browsing {
@@ -57,9 +84,12 @@ class V4EmbeddedTestServerBrowserTest : public InProcessBrowserTest {
     v4_db_factory_ = v4_db_factory.get();
     V4Database::RegisterDatabaseFactoryForTest(std::move(v4_db_factory));
 
-    ASSERT_TRUE(embedded_test_server()->InitializeAndListen());
+    secure_embedded_test_server_ = std::make_unique<net::EmbeddedTestServer>(
+        net::EmbeddedTestServer::Type::TYPE_HTTPS);
+
     InProcessBrowserTest::SetUp();
   }
+
   void TearDown() override {
     InProcessBrowserTest::TearDown();
     V4Database::RegisterStoreFactoryForTest(nullptr);
@@ -73,6 +103,9 @@ class V4EmbeddedTestServerBrowserTest : public InProcessBrowserTest {
     v4_db_factory_->MarkPrefixAsBad(list_id, full_hash);
   }
 
+ protected:
+  std::unique_ptr<net::EmbeddedTestServer> secure_embedded_test_server_;
+
  private:
   std::unique_ptr<net::MappedHostResolver> mapped_host_resolver_;
 
@@ -83,6 +116,8 @@ class V4EmbeddedTestServerBrowserTest : public InProcessBrowserTest {
 };
 
 IN_PROC_BROWSER_TEST_F(V4EmbeddedTestServerBrowserTest, SimpleTest) {
+  ASSERT_TRUE(embedded_test_server()->InitializeAndListen());
+
   const char kMalwarePage[] = "/safe_browsing/malware.html";
   const GURL bad_url = embedded_test_server()->GetURL(kMalwarePage);
 
@@ -107,6 +142,8 @@ IN_PROC_BROWSER_TEST_F(V4EmbeddedTestServerBrowserTest, SimpleTest) {
 
 IN_PROC_BROWSER_TEST_F(V4EmbeddedTestServerBrowserTest,
                        WrongFullHash_NoInterstitial) {
+  ASSERT_TRUE(embedded_test_server()->InitializeAndListen());
+
   const char kMalwarePage[] = "/safe_browsing/malware.html";
   const GURL bad_url = embedded_test_server()->GetURL(kMalwarePage);
 
@@ -132,4 +169,52 @@ IN_PROC_BROWSER_TEST_F(V4EmbeddedTestServerBrowserTest,
   EXPECT_FALSE(IsShowingInterstitial(contents));
 }
 
+class V4EmbeddedTestServerWithSeparateNetworkContexts
+    : public V4EmbeddedTestServerBrowserTest {
+ public:
+  V4EmbeddedTestServerWithSeparateNetworkContexts() {
+    scoped_feature_list_.Reset();
+    scoped_feature_list_.InitWithFeatures(
+        {kSafeBrowsingSeparateNetworkContexts}, {});
+  }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_F(V4EmbeddedTestServerWithSeparateNetworkContexts,
+                       DoesNotSaveCookies) {
+  ASSERT_TRUE(secure_embedded_test_server_->InitializeAndListen());
+  const char kMalwarePage[] = "/safe_browsing/malware.html";
+  const GURL bad_url = secure_embedded_test_server_->GetURL(kMalwarePage);
+
+  ThreatMatch match;
+  FullHash full_hash = V4ProtocolManagerUtil::GetFullHash(bad_url);
+  LocallyMarkPrefixAsBad(bad_url, GetUrlMalwareId());
+  match.set_platform_type(GetUrlMalwareId().platform_type());
+  match.set_threat_entry_type(ThreatEntryType::URL);
+  match.set_threat_type(ThreatType::MALWARE_THREAT);
+  match.mutable_threat()->set_hash(full_hash);
+  match.mutable_cache_duration()->set_seconds(0);
+
+  std::map<GURL, safe_browsing::ThreatMatch> response_map{{bad_url, match}};
+  StartRedirectingV4RequestsForTesting(
+      response_map, secure_embedded_test_server_.get(),
+      /*delay_map=*/std::map<GURL, base::TimeDelta>(),
+      /*serve_cookies=*/true);
+  secure_embedded_test_server_->StartAcceptingConnections();
+
+  EXPECT_EQ(GetCookies(
+                g_browser_process->safe_browsing_service()->GetNetworkContext())
+                .size(),
+            0u);
+
+  ui_test_utils::NavigateToURL(browser(), bad_url);
+
+  EXPECT_EQ(GetCookies(
+                g_browser_process->safe_browsing_service()->GetNetworkContext())
+                .size(),
+            0u);
+}
+
 }  // namespace safe_browsing

components/safe_browsing/core/db/BUILD.gn

@@ -132,6 +132,7 @@ static_library("v4_get_hash_protocol_manager") {
     ":util",
     ":v4_protocol_manager_util",
     "//base",
+    "//components/safe_browsing/core:features",
     "//components/safe_browsing/core:webui_proto",
     "//components/safe_browsing/core/common:thread_utils",
     "//net",

components/safe_browsing/core/db/v4_embedded_test_server_util.cc

@@ -60,6 +60,7 @@ std::vector<HashPrefix> GetPrefixesForRequest(const GURL& url) {
 std::unique_ptr<net::test_server::HttpResponse> HandleFullHashRequest(
     const std::map<GURL, ThreatMatch>& response_map,
     const std::map<GURL, base::TimeDelta>& delay_map,
+    bool serve_cookies,
     const net::test_server::HttpRequest& request) {
   if (!(net::test_server::ShouldHandle(request, "/v4/fullHashes:find")))
     return nullptr;
@@ -95,6 +96,9 @@ std::unique_ptr<net::test_server::HttpResponse> HandleFullHashRequest(
       (delay ? std::make_unique<net::test_server::DelayedHttpResponse>(*delay)
              : std::make_unique<net::test_server::BasicHttpResponse>());
   http_response->set_content(serialized_response);
+  if (serve_cookies)
+    http_response->AddCustomHeader("Set-Cookie",
+                                   "name=value; SameSite=None; Secure");
   return http_response;
 }
 
@@ -103,13 +107,14 @@ std::unique_ptr<net::test_server::HttpResponse> HandleFullHashRequest(
 void StartRedirectingV4RequestsForTesting(
     const std::map<GURL, ThreatMatch>& response_map,
     net::test_server::EmbeddedTestServer* embedded_test_server,
-    const std::map<GURL, base::TimeDelta>& delay_map) {
+    const std::map<GURL, base::TimeDelta>& delay_map,
+    bool serve_cookies) {
   // Static so accessing the underlying buffer won't cause use-after-free.
   static std::string url_prefix;
   url_prefix = embedded_test_server->GetURL("/v4").spec();
   SetSbV4UrlPrefixForTesting(url_prefix.c_str());
-  embedded_test_server->RegisterRequestHandler(
-      base::BindRepeating(&HandleFullHashRequest, response_map, delay_map));
+  embedded_test_server->RegisterRequestHandler(base::BindRepeating(
+      &HandleFullHashRequest, response_map, delay_map, serve_cookies));
 }
 
 }  // namespace safe_browsing

components/safe_browsing/core/db/v4_embedded_test_server_util.h

@@ -23,11 +23,13 @@ namespace safe_browsing {
 // 1. Rewrites the global V4 server URL prefix to point to the test server.
 // 2. Registers the FullHash request handler with the server.
 // 3. (Optionally) associates some delay with the resulting http response.
+// 4. (Optionally) attaches a cookie to the response.
 void StartRedirectingV4RequestsForTesting(
     const std::map<GURL, ThreatMatch>& response_map,
     net::test_server::EmbeddedTestServer* embedded_test_server,
     const std::map<GURL, base::TimeDelta>& delay_map =
-        std::map<GURL, base::TimeDelta>());
+        std::map<GURL, base::TimeDelta>(),
+    bool serve_cookies = false);
 
 }  // namespace safe_browsing
 

components/safe_browsing/core/db/v4_get_hash_protocol_manager.cc

@@ -17,6 +17,7 @@
 #include "base/trace_event/trace_event.h"
 #include "base/trace_event/traced_value.h"
 #include "components/safe_browsing/core/common/thread_utils.h"
+#include "components/safe_browsing/core/features.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_status_code.h"
@@ -339,6 +340,10 @@ void V4GetHashProtocolManager::GetFullHashes(
                        &resource_request->headers);
 
   resource_request->load_flags = net::LOAD_DISABLE_CACHE;
+  if (base::FeatureList::IsEnabled(kSafeBrowsingSeparateNetworkContexts)) {
+    resource_request->load_flags |=
+        net::LOAD_DO_NOT_SAVE_COOKIES | net::LOAD_DO_NOT_SEND_COOKIES;
+  }
   std::unique_ptr<network::SimpleURLLoader> owned_loader =
       network::SimpleURLLoader::Create(std::move(resource_request),
                                        traffic_annotation);