[iOS] Use a persistent cookie store for Safe Browsing

This makes SafeBrowsingService use a persistent cookie store,
and clears this cookie store whenever cookies are cleared for
"all time". This makes the behavior on iOS consistent with
the behavior of Safe Browsing on other platforms.

Change-Id: I37fec41b6929386ecf2ce1e29fdbb8e95ac89383
Bug: 1103219
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2291694
Commit-Queue: Ali Juma <ajuma@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796057}

ios/chrome/browser/browsing_data/BUILD.gn

@@ -45,6 +45,7 @@ source_set("browsing_data") {
     "//components/open_from_clipboard",
     "//components/password_manager/core/browser",
     "//components/prefs",
+    "//components/safe_browsing/core:features",
     "//components/sessions",
     "//components/signin/ios/browser",
     "//components/translate/core/browser:browser",
@@ -57,6 +58,7 @@ source_set("browsing_data") {
     "//ios/chrome/browser/language",
     "//ios/chrome/browser/passwords",
     "//ios/chrome/browser/reading_list:reading_list_remover",
+    "//ios/chrome/browser/safe_browsing",
     "//ios/chrome/browser/search_engines",
     "//ios/chrome/browser/sessions",
     "//ios/chrome/browser/sessions:serialisation",

ios/chrome/browser/browsing_data/browsing_data_remover_impl.mm

@@ -33,6 +33,7 @@
 #include "components/open_from_clipboard/clipboard_recent_content.h"
 #include "components/password_manager/core/browser/password_store.h"
 #include "components/prefs/pref_service.h"
+#include "components/safe_browsing/core/features.h"
 #include "components/search_engines/template_url_service.h"
 #include "components/sessions/core/tab_restore_service.h"
 #include "components/signin/ios/browser/account_consistency_service.h"
@@ -52,6 +53,7 @@
 #include "ios/chrome/browser/language/url_language_histogram_factory.h"
 #include "ios/chrome/browser/passwords/ios_chrome_password_store_factory.h"
 #include "ios/chrome/browser/reading_list/reading_list_remover_helper.h"
+#import "ios/chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "ios/chrome/browser/search_engines/template_url_service_factory.h"
 #include "ios/chrome/browser/sessions/ios_chrome_tab_restore_service_factory.h"
 #import "ios/chrome/browser/sessions/session_service_ios.h"
@@ -294,14 +296,24 @@ void BrowsingDataRemoverImpl::RemoveImpl(base::Time delete_begin,
 
   if (IsRemoveDataMaskSet(mask, BrowsingDataRemoveMask::REMOVE_COOKIES)) {
     base::RecordAction(base::UserMetricsAction("ClearBrowsingData_Cookies"));
+    net::CookieDeletionInfo::TimeRange deletion_time_range =
+        net::CookieDeletionInfo::TimeRange(delete_begin, delete_end);
     base::PostTask(
         FROM_HERE, task_traits,
         base::BindOnce(
-            &ClearCookies, context_getter_,
-            net::CookieDeletionInfo::TimeRange(delete_begin, delete_end),
+            &ClearCookies, context_getter_, deletion_time_range,
             base::BindOnce(base::IgnoreResult(&base::TaskRunner::PostTask),
                            current_task_runner, FROM_HERE,
                            CreatePendingTaskCompletionClosure())));
+    if (base::FeatureList::IsEnabled(
+            safe_browsing::kSafeBrowsingAvailableOnIOS) &&
+        !browser_state_->IsOffTheRecord()) {
+      GetApplicationContext()->GetSafeBrowsingService()->ClearCookies(
+          deletion_time_range,
+          base::BindOnce(base::IgnoreResult(&base::TaskRunner::PostTask),
+                         current_task_runner, FROM_HERE,
+                         CreatePendingTaskCompletionClosure()));
+    }
   }
 
   // There is no need to clean the remaining types of data for off-the-record

ios/chrome/browser/net/BUILD.gn

@@ -32,7 +32,6 @@ source_set("net") {
     "//components/update_client",
     "//ios/chrome/browser",
     "//ios/chrome/browser/browser_state",
-    "//ios/chrome/browser/browsing_data",
     "//ios/net",
     "//ios/web/common",
     "//net",

ios/chrome/browser/safe_browsing/BUILD.gn

@@ -52,6 +52,7 @@ source_set("safe_browsing") {
     "//ios/chrome/browser/browser_state",
     "//ios/chrome/browser/content_settings",
     "//ios/chrome/browser/history",
+    "//ios/chrome/browser/net",
     "//ios/chrome/browser/prerender",
     "//ios/chrome/browser/signin",
     "//ios/chrome/browser/sync",
@@ -176,6 +177,8 @@ source_set("unit_tests") {
     "//ios/chrome/test:test_support",
     "//ios/web/public",
     "//ios/web/public/test",
+    "//net:test_support",
+    "//net/traffic_annotation:test_support",
     "//services/network/public/cpp",
     "//testing/gmock",
     "//testing/gtest",

ios/chrome/browser/safe_browsing/fake_safe_browsing_service.h

@@ -32,6 +32,8 @@ class FakeSafeBrowsingService : public SafeBrowsingService {
       web::WebState* web_state) override;
   bool CanCheckUrl(const GURL& url) const override;
   scoped_refptr<network::SharedURLLoaderFactory> GetURLLoaderFactory() override;
+  void ClearCookies(const net::CookieDeletionInfo::TimeRange& creation_range,
+                    base::OnceClosure callback) override;
 
  protected:
   ~FakeSafeBrowsingService() override;

ios/chrome/browser/safe_browsing/fake_safe_browsing_service.mm

@@ -85,3 +85,9 @@ FakeSafeBrowsingService::GetURLLoaderFactory() {
   return base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
       &url_loader_factory_);
 }
+
+void FakeSafeBrowsingService::ClearCookies(
+    const net::CookieDeletionInfo::TimeRange& creation_range,
+    base::OnceClosure callback) {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
+}

ios/chrome/browser/safe_browsing/safe_browsing_service.h

@@ -5,7 +5,9 @@
 #ifndef IOS_CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_SERVICE_H_
 #define IOS_CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_SERVICE_H_
 
+#include "base/callback.h"
 #include "base/memory/ref_counted.h"
+#include "net/cookies/cookie_deletion_info.h"
 #include "url/gurl.h"
 
 class PrefService;
@@ -58,6 +60,12 @@ class SafeBrowsingService
   virtual scoped_refptr<network::SharedURLLoaderFactory>
   GetURLLoaderFactory() = 0;
 
+  // Clears cookies if the given deletion time range is for "all time". Calls
+  // the given |callback| once deletion is complete.
+  virtual void ClearCookies(
+      const net::CookieDeletionInfo::TimeRange& creation_range,
+      base::OnceClosure callback) = 0;
+
  protected:
   SafeBrowsingService() = default;
   virtual ~SafeBrowsingService() = default;

ios/chrome/browser/safe_browsing/safe_browsing_service_impl.h

@@ -44,6 +44,8 @@ class SafeBrowsingServiceImpl : public SafeBrowsingService {
       web::WebState* web_state) override;
   bool CanCheckUrl(const GURL& url) const override;
   scoped_refptr<network::SharedURLLoaderFactory> GetURLLoaderFactory() override;
+  void ClearCookies(const net::CookieDeletionInfo::TimeRange& creation_range,
+                    base::OnceClosure callback) override;
 
  private:
   // A helper class for enabling/disabling Safe Browsing and maintaining state
@@ -63,7 +65,8 @@ class SafeBrowsingServiceImpl : public SafeBrowsingService {
     void Initialize(
         scoped_refptr<SafeBrowsingServiceImpl> safe_browsing_service,
         mojo::PendingReceiver<network::mojom::NetworkContext>
-            network_context_receiver);
+            network_context_receiver,
+        const base::FilePath& safe_browsing_data_path);
 
     // Disables Safe Browsing, and destroys the network context and URL loader
     // factory used by the SafeBrowsingDatabaseManager.
@@ -72,6 +75,9 @@ class SafeBrowsingServiceImpl : public SafeBrowsingService {
     // Enables or disables Safe Browsing database updates and lookups.
     void SetSafeBrowsingEnabled(bool enabled);
 
+    // Clears all cookies. Calls the given |callback| when deletion is complete.
+    void ClearAllCookies(base::OnceClosure callback);
+
    private:
     friend base::RefCountedThreadSafe<IOThreadEnabler>;
     ~IOThreadEnabler();
@@ -80,8 +86,9 @@ class SafeBrowsingServiceImpl : public SafeBrowsingService {
     // queries.
     void StartSafeBrowsingDBManager();
 
-    // Constructs a URLRequestContext.
-    void SetUpURLRequestContext();
+    // Constructs a URLRequestContext, using the given path as the location for
+    // the cookie store.
+    void SetUpURLRequestContext(const base::FilePath& safe_browsing_data_path);
 
     // Constructs a SharedURLLoaderFactory.
     void SetUpURLLoaderFactory(

ios/chrome/browser/safe_browsing/safe_browsing_service_impl.mm

@@ -18,11 +18,14 @@
 #include "components/safe_browsing/core/features.h"
 #include "components/safe_browsing/core/realtime/url_lookup_service.h"
 #import "ios/chrome/browser/browser_state/chrome_browser_state.h"
+#import "ios/chrome/browser/net/cookie_util.h"
 #import "ios/chrome/browser/safe_browsing/real_time_url_lookup_service_factory.h"
 #import "ios/chrome/browser/safe_browsing/url_checker_delegate_impl.h"
+#import "ios/net/cookies/system_cookie_store.h"
 #include "ios/web/public/thread/web_task_traits.h"
 #include "ios/web/public/thread/web_thread.h"
 #import "ios/web/public/web_state.h"
+#include "net/cookies/cookie_store.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_builder.h"
 #include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
@@ -61,7 +64,8 @@ void SafeBrowsingServiceImpl::Initialize(PrefService* prefs,
       FROM_HERE, {web::WebThread::IO},
       base::BindOnce(&IOThreadEnabler::Initialize, io_thread_enabler_,
                      base::WrapRefCounted(this),
-                     network_context_client_.BindNewPipeAndPassReceiver()));
+                     network_context_client_.BindNewPipeAndPassReceiver(),
+                     safe_browsing_data_path));
 
   // Watch for changes to the Safe Browsing opt-out preference.
   pref_change_registrar_ = std::make_unique<PrefChangeRegistrar>();
@@ -111,6 +115,21 @@ SafeBrowsingServiceImpl::GetURLLoaderFactory() {
   return shared_url_loader_factory_;
 }
 
+void SafeBrowsingServiceImpl::ClearCookies(
+    const net::CookieDeletionInfo::TimeRange& creation_range,
+    base::OnceClosure callback) {
+  if (creation_range.start() == base::Time() &&
+      creation_range.end() == base::Time::Max()) {
+    base::PostTask(
+        FROM_HERE,
+        {web::WebThread::IO, base::TaskShutdownBehavior::BLOCK_SHUTDOWN},
+        base::BindOnce(&IOThreadEnabler::ClearAllCookies, io_thread_enabler_,
+                       std::move(callback)));
+  } else {
+    base::PostTask(FROM_HERE, {web::WebThread::IO}, std::move(callback));
+  }
+}
+
 void SafeBrowsingServiceImpl::SetUpURLLoaderFactory(
     mojo::PendingReceiver<network::mojom::URLLoaderFactory> receiver) {
   DCHECK_CURRENTLY_ON(web::WebThread::UI);
@@ -146,8 +165,9 @@ SafeBrowsingServiceImpl::IOThreadEnabler::~IOThreadEnabler() = default;
 void SafeBrowsingServiceImpl::IOThreadEnabler::Initialize(
     scoped_refptr<SafeBrowsingServiceImpl> safe_browsing_service,
     mojo::PendingReceiver<network::mojom::NetworkContext>
-        network_context_receiver) {
-  SetUpURLRequestContext();
+        network_context_receiver,
+    const base::FilePath& safe_browsing_data_path) {
+  SetUpURLRequestContext(safe_browsing_data_path);
   std::vector<std::string> cors_exempt_header_list;
   network_context_ = std::make_unique<network::NetworkContext>(
       /*network_service=*/nullptr, std::move(network_context_receiver),
@@ -178,6 +198,15 @@ void SafeBrowsingServiceImpl::IOThreadEnabler::SetSafeBrowsingEnabled(
     safe_browsing_db_manager_->StopOnIOThread(shutting_down_);
 }
 
+void SafeBrowsingServiceImpl::IOThreadEnabler::ClearAllCookies(
+    base::OnceClosure callback) {
+  DCHECK_CURRENTLY_ON(web::WebThread::IO);
+  net::CookieStore* cookie_store = url_request_context_->cookie_store();
+  cookie_store->DeleteAllAsync(base::BindOnce(
+      [](base::OnceClosure callback, uint32_t) { std::move(callback).Run(); },
+      std::move(callback)));
+}
+
 void SafeBrowsingServiceImpl::IOThreadEnabler::StartSafeBrowsingDBManager() {
   DCHECK_CURRENTLY_ON(web::WebThread::IO);
 
@@ -195,12 +224,23 @@ void SafeBrowsingServiceImpl::IOThreadEnabler::StartSafeBrowsingDBManager() {
                                              config);
 }
 
-void SafeBrowsingServiceImpl::IOThreadEnabler::SetUpURLRequestContext() {
+void SafeBrowsingServiceImpl::IOThreadEnabler::SetUpURLRequestContext(
+    const base::FilePath& safe_browsing_data_path) {
   DCHECK_CURRENTLY_ON(web::WebThread::IO);
 
-  // This uses an in-memory non-persistent cookie store. The Safe Browsing V4
-  // Update API does not depend on cookies.
   net::URLRequestContextBuilder builder;
+  base::FilePath cookie_file_path(safe_browsing_data_path.value() +
+                                  safe_browsing::kCookiesFile);
+  std::unique_ptr<net::CookieStore> cookie_store =
+      cookie_util::CreateCookieStore(
+          cookie_util::CookieStoreConfig(
+              cookie_file_path,
+              cookie_util::CookieStoreConfig::RESTORED_SESSION_COOKIES,
+              cookie_util::CookieStoreConfig::COOKIE_MONSTER,
+              /*crypto_delegate=*/nullptr),
+          /*system_cookie_store=*/nullptr, net::NetLog::Get());
+
+  builder.SetCookieStore(std::move(cookie_store));
   url_request_context_ = builder.Build();
 }
 

ios/chrome/browser/safe_browsing/safe_browsing_service_unittest.mm

@@ -8,6 +8,7 @@
 #include "base/path_service.h"
 #include "base/run_loop.h"
 #include "base/task/post_task.h"
+#include "base/test/bind_test_util.h"
 #include "base/test/scoped_feature_list.h"
 #include "components/prefs/pref_service.h"
 #include "components/safe_browsing/core/browser/safe_browsing_url_checker_impl.h"
@@ -37,6 +38,11 @@
 #include "ios/web/public/test/web_task_environment.h"
 #include "ios/web/public/thread/web_task_traits.h"
 #include "ios/web/public/thread/web_thread.h"
+#include "net/test/embedded_test_server/default_handlers.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/cpp/simple_url_loader.h"
 #include "testing/platform_test.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
@@ -195,6 +201,7 @@ class SafeBrowsingServiceTest : public PlatformTest {
   web::WebTaskEnvironment task_environment_;
   scoped_refptr<SafeBrowsingService> safe_browsing_service_;
   std::unique_ptr<TestChromeBrowserState> browser_state_;
+  base::ScopedTempDir temp_dir_;
 
  private:
   void MarkUrlAsMalwareOnIOThread(const GURL& bad_url) {
@@ -218,8 +225,6 @@ class SafeBrowsingServiceTest : public PlatformTest {
     v4_get_hash_factory_->AddToFullHashCache(full_hash_info);
   }
 
-  base::ScopedTempDir temp_dir_;
-
   // Owned by V4Database.
   safe_browsing::TestV4DatabaseFactory* v4_db_factory_;
   // Owned by V4GetHashProtocolManager.
@@ -301,3 +306,119 @@ TEST_F(SafeBrowsingServiceTest, RealTimeSafeAndUnsafePages) {
   EXPECT_FALSE(client.result_pending());
   EXPECT_FALSE(client.url_is_unsafe());
 }
+
+// Verifies that cookies are persisted across instantiations of
+// SafeBrowsingServiceImpl.
+TEST_F(SafeBrowsingServiceTest, PersistentCookies) {
+  net::EmbeddedTestServer server(net::EmbeddedTestServer::TYPE_HTTPS);
+  net::test_server::RegisterDefaultHandlers(&server);
+  ASSERT_TRUE(server.Start());
+  std::string cookie = "test=123";
+  std::unique_ptr<network::ResourceRequest> resource_request =
+      std::make_unique<network::ResourceRequest>();
+
+  // Set a cookie that expires in an hour.
+  resource_request->url = server.GetURL("/set-cookie?" + cookie +
+                                        ";max-age=3600;SameSite=None;Secure");
+  std::unique_ptr<network::SimpleURLLoader> url_loader =
+      network::SimpleURLLoader::Create(std::move(resource_request),
+                                       TRAFFIC_ANNOTATION_FOR_TESTS);
+  base::RunLoop run_loop1;
+  url_loader->DownloadHeadersOnly(
+      safe_browsing_service_->GetURLLoaderFactory().get(),
+      base::BindLambdaForTesting(
+          [&](scoped_refptr<net::HttpResponseHeaders> headers) {
+            run_loop1.Quit();
+          }));
+  run_loop1.Run();
+
+  // Destroy and re-create safe_browsing_service_, and verify that the cookie
+  // is still set.
+  safe_browsing_service_->ShutDown();
+  base::RunLoop().RunUntilIdle();
+  safe_browsing_service_ = base::MakeRefCounted<SafeBrowsingServiceImpl>();
+  safe_browsing_service_->Initialize(browser_state_->GetPrefs(),
+                                     temp_dir_.GetPath());
+  base::RunLoop().RunUntilIdle();
+
+  resource_request = std::make_unique<network::ResourceRequest>();
+  resource_request->url = server.GetURL("/echoheader?Cookie");
+  url_loader = network::SimpleURLLoader::Create(std::move(resource_request),
+                                                TRAFFIC_ANNOTATION_FOR_TESTS);
+  base::RunLoop run_loop2;
+  url_loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+      safe_browsing_service_->GetURLLoaderFactory().get(),
+      base::BindLambdaForTesting([&](std::unique_ptr<std::string> body) {
+        EXPECT_NE(std::string::npos, body->find(cookie));
+        run_loop2.Quit();
+      }));
+  run_loop2.Run();
+}
+
+// Verifies that cookies are cleared when ClearCookies() is called with a
+// time range of all-time, but not otherwise.
+TEST_F(SafeBrowsingServiceTest, ClearCookies) {
+  net::EmbeddedTestServer server(net::EmbeddedTestServer::TYPE_HTTPS);
+  net::test_server::RegisterDefaultHandlers(&server);
+  ASSERT_TRUE(server.Start());
+  std::string cookie = "test=123";
+  std::unique_ptr<network::ResourceRequest> resource_request =
+      std::make_unique<network::ResourceRequest>();
+
+  // Set a cookie that expires in an hour.
+  resource_request->url = server.GetURL("/set-cookie?" + cookie +
+                                        ";max-age=3600;SameSite=None;Secure");
+  std::unique_ptr<network::SimpleURLLoader> url_loader =
+      network::SimpleURLLoader::Create(std::move(resource_request),
+                                       TRAFFIC_ANNOTATION_FOR_TESTS);
+  base::RunLoop run_loop1;
+  url_loader->DownloadHeadersOnly(
+      safe_browsing_service_->GetURLLoaderFactory().get(),
+      base::BindLambdaForTesting(
+          [&](scoped_refptr<net::HttpResponseHeaders> headers) {
+            run_loop1.Quit();
+          }));
+  run_loop1.Run();
+
+  // Call ClearCookies() with a non-all-time time range, and verify that the
+  // cookie is still set.
+  base::RunLoop run_loop2;
+  safe_browsing_service_->ClearCookies(
+      net::CookieDeletionInfo::TimeRange(base::Time(), base::Time::Now()),
+      base::BindLambdaForTesting([&]() { run_loop2.Quit(); }));
+  run_loop2.Run();
+
+  resource_request = std::make_unique<network::ResourceRequest>();
+  resource_request->url = server.GetURL("/echoheader?Cookie");
+  url_loader = network::SimpleURLLoader::Create(std::move(resource_request),
+                                                TRAFFIC_ANNOTATION_FOR_TESTS);
+  base::RunLoop run_loop3;
+  url_loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+      safe_browsing_service_->GetURLLoaderFactory().get(),
+      base::BindLambdaForTesting([&](std::unique_ptr<std::string> body) {
+        EXPECT_NE(std::string::npos, body->find(cookie));
+        run_loop3.Quit();
+      }));
+  run_loop3.Run();
+
+  // Call ClearCookies() with a time range of all-time, and verify that the
+  // cookie is no longer set.
+  base::RunLoop run_loop4;
+  safe_browsing_service_->ClearCookies(
+      net::CookieDeletionInfo::TimeRange(base::Time(), base::Time::Max()),
+      base::BindLambdaForTesting([&]() { run_loop4.Quit(); }));
+  run_loop4.Run();
+
+  resource_request = std::make_unique<network::ResourceRequest>();
+  resource_request->url = server.GetURL("/echoheader?Cookie");
+  url_loader = network::SimpleURLLoader::Create(std::move(resource_request),
+                                                TRAFFIC_ANNOTATION_FOR_TESTS);
+  base::RunLoop run_loop5;
+  url_loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+      safe_browsing_service_->GetURLLoaderFactory().get(),
+      base::BindLambdaForTesting([&](std::unique_ptr<std::string> body) {
+        EXPECT_EQ(std::string::npos, body->find(cookie));
+        run_loop5.Quit();
+      }));
+  run_loop5.Run();
+}