From 56de59362baf082acbb2d8f9f9649266f1171bec Mon Sep 17 00:00:00 2001 From: Shivan Kaul Sahib Date: Thu, 1 Sep 2022 10:57:39 -0700 Subject: [PATCH] Cookie expiry fixes (#14852) * Add cookie expiry browser tests * Remove cookie store patch * Add ModifyExpiration to account for JS cookie expiry --- .../cookie_expiry_browsertest.cc | 212 ++++++++++++++++++ browser/brave_shields/sources.gni | 1 + chromium_src/net/cookies/canonical_cookie.cc | 16 +- .../network/restricted_cookie_manager.cc | 23 ++ .../network/restricted_cookie_manager.h | 2 + .../modules/cookie_store/cookie_store.cc | 23 -- patches/net-cookies-canonical_cookie.cc.patch | 18 +- ...modules-cookie_store-cookie_store.cc.patch | 12 - test/BUILD.gn | 1 + 9 files changed, 256 insertions(+), 52 deletions(-) create mode 100644 browser/brave_shields/cookie_expiry_browsertest.cc delete mode 100644 chromium_src/third_party/blink/renderer/modules/cookie_store/cookie_store.cc delete mode 100644 patches/third_party-blink-renderer-modules-cookie_store-cookie_store.cc.patch diff --git a/browser/brave_shields/cookie_expiry_browsertest.cc b/browser/brave_shields/cookie_expiry_browsertest.cc new file mode 100644 index 00000000000..28e5b3e7fc1 --- /dev/null +++ b/browser/brave_shields/cookie_expiry_browsertest.cc @@ -0,0 +1,212 @@ +// Copyright (c) 2012 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 + +#include "base/bind.h" +#include "base/test/bind.h" + +#include "base/path_service.h" +#include "base/strings/stringprintf.h" +#include "brave/components/constants/brave_paths.h" +#include "build/build_config.h" +#include "chrome/browser/ui/browser.h" +#include "chrome/test/base/in_process_browser_test.h" +#include "chrome/test/base/ui_test_utils.h" +#include "content/public/browser/browser_context.h" +#include "content/public/browser/storage_partition.h" +#include "content/public/test/browser_test.h" +#include "content/public/test/content_mock_cert_verifier.h" +#include "net/dns/mock_host_resolver.h" +#include "net/test/embedded_test_server/default_handlers.h" +#include "net/test/embedded_test_server/embedded_test_server.h" + +namespace { + +constexpr base::TimeDelta k4YearsInDays = base::Days(1461); +// There might be a gap of a few milliseconds between setting the cookie and it +// getting stored. To prevent flapping tests, set this margin to be large +// (but we're still testing what we want to test) +// See: net/cookies/canonical_cookie_unittest.cc +constexpr base::TimeDelta kMarginForTesting = base::Seconds(5); + +} // namespace + +class CookieExpirationTest : public InProcessBrowserTest { + public: + void SetUpOnMainThread() override { + InProcessBrowserTest::SetUpOnMainThread(); + mock_cert_verifier_.mock_cert_verifier()->set_default_result(net::OK); + host_resolver()->AddRule("*", "127.0.0.1"); + https_server_ = std::make_unique( + net::test_server::EmbeddedTestServer::TYPE_HTTPS); + RegisterDefaultHandlers(https_server_.get()); + + brave::RegisterPathProvider(); + base::FilePath test_data_dir; + base::PathService::Get(brave::DIR_TEST_DATA, &test_data_dir); + https_server_->ServeFilesFromDirectory(test_data_dir); + + ASSERT_TRUE(https_server_->Start()); + } + + void SetUpCommandLine(base::CommandLine* command_line) override { + InProcessBrowserTest::SetUpCommandLine(command_line); + mock_cert_verifier_.SetUpCommandLine(command_line); + } + + void SetUpInProcessBrowserTestFixture() override { + InProcessBrowserTest::SetUpInProcessBrowserTestFixture(); + mock_cert_verifier_.SetUpInProcessBrowserTestFixture(); + } + + void TearDownInProcessBrowserTestFixture() override { + InProcessBrowserTest::TearDownInProcessBrowserTestFixture(); + mock_cert_verifier_.TearDownInProcessBrowserTestFixture(); + } + + // Set a cookie with JavaScript. + void JSDocumentCookieWriteCookie(Browser* browser, std::string age) { + std::string cookie_string = + base::StringPrintf("document.cookie = 'name=Test; %s'", age.c_str()); + ASSERT_TRUE(content::ExecJs( + browser->tab_strip_model()->GetActiveWebContents(), cookie_string)); + } + + void JSCookieStoreWriteCookie(Browser* browser, std::string expires_in_ms) { + ASSERT_TRUE(content::ExecJs( + browser->tab_strip_model()->GetActiveWebContents(), + base::StringPrintf("(async () => {" + "return await window.cookieStore.set(" + " { name: 'name'," + " value: 'Good'," + " expires: Date.now() + %s," + " });" + "})()", + expires_in_ms.c_str()))); + } + + std::vector GetAllCookiesDirect(Browser* browser) { + base::RunLoop run_loop; + + std::vector cookies_out; + browser->tab_strip_model() + ->GetActiveWebContents() + ->GetBrowserContext() + ->GetDefaultStoragePartition() + ->GetCookieManagerForBrowserProcess() + ->GetAllCookies(base::BindLambdaForTesting( + [&run_loop, + &cookies_out](const std::vector& cookies) { + cookies_out = cookies; + run_loop.Quit(); + })); + run_loop.Run(); + return cookies_out; + } + + protected: + std::unique_ptr https_server_; + + private: + content::ContentMockCertVerifier mock_cert_verifier_; +}; + +IN_PROC_BROWSER_TEST_F(CookieExpirationTest, + CheckExpiryForDocumentCookieLessThanMax) { + auto less_than_max = base::Days(2); + + GURL url = https_server_->GetURL("a.com", "/simple.html"); + ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), url)); + JSDocumentCookieWriteCookie( + browser(), "max-age=" + std::to_string(less_than_max.InSeconds())); + std::vector all_cookies = + GetAllCookiesDirect(browser()); + EXPECT_EQ(1u, all_cookies.size()); + for (const net::CanonicalCookie& cookie : all_cookies) { + EXPECT_LE((base::Time::Now() + less_than_max - cookie.ExpiryDate()), + kMarginForTesting); + } +} + +IN_PROC_BROWSER_TEST_F(CookieExpirationTest, + CheckExpiryForDocumentCookieMoreThanMax) { + GURL url = https_server_->GetURL("a.com", "/simple.html"); + ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), url)); + JSDocumentCookieWriteCookie( + browser(), "max-age=" + std::to_string(k4YearsInDays.InSeconds())); + std::vector all_cookies = + GetAllCookiesDirect(browser()); + EXPECT_EQ(1u, all_cookies.size()); + for (const net::CanonicalCookie& cookie : all_cookies) { + EXPECT_EQ((cookie.ExpiryDate() - cookie.CreationDate()).InDays(), 7); + } +} + +IN_PROC_BROWSER_TEST_F(CookieExpirationTest, + CheckExpiryForCookieStoreLessThanMax) { + auto less_than_max = base::Days(2); + GURL url = https_server_->GetURL("a.com", "/simple.html"); + ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), url)); + JSCookieStoreWriteCookie(browser(), + std::to_string(less_than_max.InMilliseconds())); + + std::vector all_cookies = + GetAllCookiesDirect(browser()); + EXPECT_EQ(1u, all_cookies.size()); + for (const net::CanonicalCookie& cookie : all_cookies) { + EXPECT_LE((base::Time::Now() + less_than_max - cookie.ExpiryDate()), + kMarginForTesting); + } +} + +IN_PROC_BROWSER_TEST_F(CookieExpirationTest, + CheckExpiryForCookieStoreMoreThanMax) { + GURL url = https_server_->GetURL("a.com", "/simple.html"); + ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), url)); + JSCookieStoreWriteCookie(browser(), + std::to_string(k4YearsInDays.InMilliseconds())); + + std::vector all_cookies = + GetAllCookiesDirect(browser()); + EXPECT_EQ(1u, all_cookies.size()); + for (const net::CanonicalCookie& cookie : all_cookies) { + EXPECT_EQ((cookie.ExpiryDate() - cookie.CreationDate()).InDays(), 7); + } +} + +IN_PROC_BROWSER_TEST_F(CookieExpirationTest, + CheckExpiryForHttpCookiesLessThanMax) { + auto less_than_max = base::Days(30); + std::string cookie_string = "/set-cookie?test=http;max-age=" + + std::to_string(less_than_max.InSeconds()); + + GURL url = https_server_->GetURL("a.com", cookie_string); + + ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), url)); + + std::vector all_cookies = + GetAllCookiesDirect(browser()); + EXPECT_EQ(1u, all_cookies.size()); + for (const net::CanonicalCookie& cookie : all_cookies) { + EXPECT_LE((base::Time::Now() + less_than_max - cookie.ExpiryDate()), + kMarginForTesting); + } +} + +IN_PROC_BROWSER_TEST_F(CookieExpirationTest, + CheckExpiryForHttpCookiesMoreThanMax) { + std::string cookie_string = + "test=http;max-age=" + std::to_string(k4YearsInDays.InSeconds()); + GURL url = https_server_->GetURL("a.com", "/set-cookie?" + cookie_string); + + ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), url)); + + std::vector all_cookies = + GetAllCookiesDirect(browser()); + EXPECT_EQ(1u, all_cookies.size()); + for (const net::CanonicalCookie& cookie : all_cookies) { + EXPECT_EQ((cookie.ExpiryDate() - cookie.CreationDate()).InDays(), 180); + } +} diff --git a/browser/brave_shields/sources.gni b/browser/brave_shields/sources.gni index d8e68aea117..3afe95656f3 100644 --- a/browser/brave_shields/sources.gni +++ b/browser/brave_shields/sources.gni @@ -21,6 +21,7 @@ brave_browser_brave_shields_deps = [ "//brave/components/brave_perf_predictor/browser", "//brave/components/brave_shields/browser", "//brave/components/brave_shields/common", + "//brave/components/constants", "//chrome/browser/profiles:profile", "//chrome/common", "//components/content_settings/core/browser", diff --git a/chromium_src/net/cookies/canonical_cookie.cc b/chromium_src/net/cookies/canonical_cookie.cc index 94a21c76e32..cb5d5093559 100644 --- a/chromium_src/net/cookies/canonical_cookie.cc +++ b/chromium_src/net/cookies/canonical_cookie.cc @@ -9,21 +9,21 @@ namespace { -// Javascript max expiration is handled by blink::CookieStore constexpr base::TimeDelta kMaxCookieExpiration = base::Days(30 * 6); // 6 months -base::Time BraveCanonExpiration(const base::Time& cookie_expires, - const base::Time& creation_time) { - const base::Time max_expiration = creation_time + kMaxCookieExpiration; +base::Time BraveCanonExpiration(const base::Time& expiry_date, + const base::Time& creation_date) { + const base::Time max_expiration = creation_date + kMaxCookieExpiration; - return std::min(cookie_expires, max_expiration); + return std::min(expiry_date, max_expiration); } } // namespace -#define BRAVE_CREATE \ - cookie_expires = BraveCanonExpiration(cookie_expires, creation_time); +#define BRAVE_CANONICAL_COOKIE_VALIDATE_AND_ADJUST_EXPIRY_DATE \ + if ((true)) \ + return BraveCanonExpiration(expiry_date, fixed_creation_date); #include "src/net/cookies/canonical_cookie.cc" -#undef BRAVE_CREATE +#undef BRAVE_CANONICAL_COOKIE_VALIDATE_AND_ADJUST_EXPIRY_DATE diff --git a/chromium_src/services/network/restricted_cookie_manager.cc b/chromium_src/services/network/restricted_cookie_manager.cc index 3a2028dbe2f..0d6635748b9 100644 --- a/chromium_src/services/network/restricted_cookie_manager.cc +++ b/chromium_src/services/network/restricted_cookie_manager.cc @@ -24,14 +24,37 @@ // for components/content_settings/core/common/cookie_settings_base.{h,cc}. #define IsFullCookieAccessAllowed IsEphemeralCookieAccessAllowed +#define FromStorage(NAME, VALUE, DOMAIN, PATH, CREATION, EXPIRY, LAST_ACCESS, \ + LAST_UPDATE, SECURE, HTTP_ONLY, SAME_SITE, PRIORITY, \ + SAME_PARTY, PARTITION, SOURCE_SCHEME, PORT) \ + FromStorage(NAME, VALUE, DOMAIN, PATH, CREATION, \ + ModifyExpiration(EXPIRY, CREATION), LAST_ACCESS, LAST_UPDATE, \ + SECURE, HTTP_ONLY, SAME_SITE, PRIORITY, SAME_PARTY, PARTITION, \ + SOURCE_SCHEME, PORT) + #include "src/services/network/restricted_cookie_manager.cc" #undef IsFullCookieAccessAllowed #undef AnnotateAndMoveUserBlockedCookies #undef IsCookieAccessible +namespace { + +constexpr base::TimeDelta kMaxCookieExpiration = + base::Days(7); // For JS cookies: CookieStore and document.cookie + +} // namespace + namespace network { +base::Time RestrictedCookieManager::ModifyExpiration( + const base::Time& expiry_date, + const base::Time& creation_date) const { + const base::Time max_expiration = creation_date + kMaxCookieExpiration; + + return std::min(expiry_date, max_expiration); +} + net::CookieOptions RestrictedCookieManager::MakeOptionsForSet( mojom::RestrictedCookieManagerRole role, const GURL& url, diff --git a/chromium_src/services/network/restricted_cookie_manager.h b/chromium_src/services/network/restricted_cookie_manager.h index dc1ce90dda1..967d8c4963d 100644 --- a/chromium_src/services/network/restricted_cookie_manager.h +++ b/chromium_src/services/network/restricted_cookie_manager.h @@ -14,6 +14,8 @@ #define RemoveChangeListener \ NotUsed() const {} \ + base::Time ModifyExpiration(const base::Time& expiry_date, \ + const base::Time& creation_date) const; \ net::CookieOptions MakeOptionsForSet( \ mojom::RestrictedCookieManagerRole role, const GURL& url, \ const net::SiteForCookies& site_for_cookies, \ diff --git a/chromium_src/third_party/blink/renderer/modules/cookie_store/cookie_store.cc b/chromium_src/third_party/blink/renderer/modules/cookie_store/cookie_store.cc deleted file mode 100644 index 0a4522d5fd0..00000000000 --- a/chromium_src/third_party/blink/renderer/modules/cookie_store/cookie_store.cc +++ /dev/null @@ -1,23 +0,0 @@ -/* Copyright (c) 2020 The Brave Authors. All rights reserved. - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this file, - * You can obtain one at https://mozilla.org/MPL/2.0/. */ - -#include - -#include "base/time/time.h" - -namespace { - -constexpr base::TimeDelta kJavascriptCookieExpiration = base::Days(7); - -base::Time BraveCanonExpiration(const base::Time& current) { - // creation time is always now for new JS cookies - return std::min(base::Time::Now() + kJavascriptCookieExpiration, current); -} - -} // namespace - -#define BRAVE_TO_CANONICAL_COOKIE expires = BraveCanonExpiration(expires); -#include "src/third_party/blink/renderer/modules/cookie_store/cookie_store.cc" -#undef BRAVE_TO_CANONICAL_COOKIE diff --git a/patches/net-cookies-canonical_cookie.cc.patch b/patches/net-cookies-canonical_cookie.cc.patch index 26bdf8e456e..15918fb1218 100644 --- a/patches/net-cookies-canonical_cookie.cc.patch +++ b/patches/net-cookies-canonical_cookie.cc.patch @@ -1,12 +1,12 @@ diff --git a/net/cookies/canonical_cookie.cc b/net/cookies/canonical_cookie.cc -index 91d210fa1683da314922a15734d4fabbde8ab873..1123de9ee42317726e3e0d8f4a37bd71a54202d7 100644 +index 91d210fa1683da314922a15734d4fabbde8ab873..20eb17aef84b0f3bf8182b62e6afe13b71aa8571 100644 --- a/net/cookies/canonical_cookie.cc +++ b/net/cookies/canonical_cookie.cc -@@ -666,6 +666,7 @@ std::unique_ptr CanonicalCookie::Create( - // Get the port, this will get a default value if a port isn't provided. - int source_port = ValidateAndAdjustSourcePort(url.EffectiveIntPort()); - -+ BRAVE_CREATE - std::unique_ptr cc = base::WrapUnique(new CanonicalCookie( - parsed_cookie.Name(), parsed_cookie.Value(), cookie_domain, cookie_path, - creation_time, cookie_expires, creation_time, +@@ -552,6 +552,7 @@ base::Time CanonicalCookie::ValidateAndAdjustExpiryDate( + // * network_handler.cc::MakeCookieFromProtocolValues + fixed_creation_date = base::Time::Now(); + } ++ BRAVE_CANONICAL_COOKIE_VALIDATE_AND_ADJUST_EXPIRY_DATE + if (base::FeatureList::IsEnabled(features::kClampCookieExpiryTo400Days)) { + base::Time maximum_expiry_date = fixed_creation_date + base::Days(400); + if (expiry_date > maximum_expiry_date) diff --git a/patches/third_party-blink-renderer-modules-cookie_store-cookie_store.cc.patch b/patches/third_party-blink-renderer-modules-cookie_store-cookie_store.cc.patch deleted file mode 100644 index 7e202f6900e..00000000000 --- a/patches/third_party-blink-renderer-modules-cookie_store-cookie_store.cc.patch +++ /dev/null @@ -1,12 +0,0 @@ -diff --git a/third_party/blink/renderer/modules/cookie_store/cookie_store.cc b/third_party/blink/renderer/modules/cookie_store/cookie_store.cc -index c4e35911f18c1690c1e212949a6ed86d5c517473..ee40afd0a3c91267e38f731ad518fd9d5dfe1015 100644 ---- a/third_party/blink/renderer/modules/cookie_store/cookie_store.cc -+++ b/third_party/blink/renderer/modules/cookie_store/cookie_store.cc -@@ -85,6 +85,7 @@ std::unique_ptr ToCanonicalCookie( - base::Time expires = options->hasExpiresNonNull() - ? base::Time::FromJavaTime(options->expiresNonNull()) - : base::Time(); -+ BRAVE_TO_CANONICAL_COOKIE - - String cookie_url_host = cookie_url.Host(); - String domain; diff --git a/test/BUILD.gn b/test/BUILD.gn index e47e6a7f785..68a7c319da3 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -657,6 +657,7 @@ if (!is_android) { "//brave/browser/brave_shields/ad_block_service_browsertest.cc", "//brave/browser/brave_shields/ad_block_service_browsertest.h", "//brave/browser/brave_shields/brave_shields_web_contents_observer_browsertest.cc", + "//brave/browser/brave_shields/cookie_expiry_browsertest.cc", "//brave/browser/brave_shields/cookie_pref_service_browsertest.cc", "//brave/browser/brave_shields/domain_block_page_browsertest.cc", "//brave/browser/brave_shields/websockets_pool_limit_browsertest.cc",