From ee426c478c05df9b959c7775c92cbf3a85de8293 Mon Sep 17 00:00:00 2001 From: Ilie Lesan Date: Fri, 3 Jan 2025 14:47:32 +0200 Subject: [PATCH 1/2] Don't clear cookies if the profile doesn't have any browser window associated --- ...rave_ephemeral_storage_service_delegate.cc | 15 +++++ ...brave_ephemeral_storage_service_delegate.h | 1 + .../ephemeral_storage_browsertest.cc | 12 ++-- .../ephemeral_storage_browsertest.h | 2 +- ...l_storage_forget_by_default_browsertest.cc | 56 ++++++++++++++++++- .../ephemeral_storage_service_unittest.cc | 9 +-- components/ephemeral_storage/BUILD.gn | 1 + .../ephemeral_storage_service.cc | 5 ++ .../ephemeral_storage_service_delegate.cc | 14 +++++ .../ephemeral_storage_service_delegate.h | 1 + 10 files changed, 104 insertions(+), 12 deletions(-) create mode 100644 components/ephemeral_storage/ephemeral_storage_service_delegate.cc diff --git a/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.cc b/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.cc index 00c6a0f83faa..d8256a791d1c 100644 --- a/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.cc +++ b/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.cc @@ -9,6 +9,10 @@ #include "brave/components/brave_shields/content/browser/brave_shields_util.h" #include "chrome/browser/browsing_data/chrome_browsing_data_remover_constants.h" +#include "chrome/browser/profiles/profile.h" +#include "chrome/browser/ui/browser.h" +#include "chrome/browser/ui/browser_finder.h" +#include "chrome/browser/ui/browser_window.h" #include "components/content_settings/core/browser/host_content_settings_map.h" #include "content/public/browser/browser_context.h" #include "content/public/browser/browsing_data_filter_builder.h" @@ -111,4 +115,15 @@ void BraveEphemeralStorageServiceDelegate::CleanupFirstPartyStorageArea( origin_type, std::move(filter_builder)); } +bool BraveEphemeralStorageServiceDelegate::DoesProfileHaveAnyBrowserWindow() + const { + Profile* profile = Profile::FromBrowserContext(context_); + for (Browser* browser : chrome::FindAllBrowsersWithProfile(profile)) { + if (browser->window()) { + return true; + } + } + return false; +} + } // namespace ephemeral_storage diff --git a/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.h b/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.h index d1024699350b..ae30d3501c88 100644 --- a/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.h +++ b/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.h @@ -33,6 +33,7 @@ class BraveEphemeralStorageServiceDelegate void CleanupTLDEphemeralArea(const TLDEphemeralAreaKey& key) override; void CleanupFirstPartyStorageArea( const std::string& registerable_domain) override; + bool DoesProfileHaveAnyBrowserWindow() const override; private: raw_ptr context_ = nullptr; diff --git a/browser/ephemeral_storage/ephemeral_storage_browsertest.cc b/browser/ephemeral_storage/ephemeral_storage_browsertest.cc index 916fb7ccb046..cd78f76b3a36 100644 --- a/browser/ephemeral_storage/ephemeral_storage_browsertest.cc +++ b/browser/ephemeral_storage/ephemeral_storage_browsertest.cc @@ -308,12 +308,13 @@ content::EvalJsResult EphemeralStorageBrowserTest::GetCookiesInFrame( return content::EvalJs(host, "document.cookie"); } -size_t EphemeralStorageBrowserTest::WaitForCleanupAfterKeepAlive(Browser* b) { - if (!b) { - b = browser(); +size_t EphemeralStorageBrowserTest::WaitForCleanupAfterKeepAlive( + Profile* profile) { + if (!profile) { + profile = browser()->profile(); } const size_t fired_cnt = EphemeralStorageServiceFactory::GetInstance() - ->GetForContext(b->profile()) + ->GetForContext(profile) ->FireCleanupTimersForTesting(); // NetworkService closes existing connections when a data removal action @@ -321,8 +322,7 @@ size_t EphemeralStorageBrowserTest::WaitForCleanupAfterKeepAlive(Browser* b) { // failures when the timing is "just right". Do a no-op removal here to make // sure the queued Ephemeral Storage cleanup was complete. BrowsingDataRemoverObserver data_remover_observer; - content::BrowsingDataRemover* remover = - b->profile()->GetBrowsingDataRemover(); + content::BrowsingDataRemover* remover = profile->GetBrowsingDataRemover(); remover->AddObserver(&data_remover_observer); remover->RemoveAndReply(base::Time(), base::Time::Max(), 0, 0, &data_remover_observer); diff --git a/browser/ephemeral_storage/ephemeral_storage_browsertest.h b/browser/ephemeral_storage/ephemeral_storage_browsertest.h index 4f54fb626bdc..c6f99437495e 100644 --- a/browser/ephemeral_storage/ephemeral_storage_browsertest.h +++ b/browser/ephemeral_storage/ephemeral_storage_browsertest.h @@ -88,7 +88,7 @@ class EphemeralStorageBrowserTest : public InProcessBrowserTest { StorageType storage_type); void SetCookieInFrame(content::RenderFrameHost* host, std::string cookie); content::EvalJsResult GetCookiesInFrame(content::RenderFrameHost* host); - size_t WaitForCleanupAfterKeepAlive(Browser* browser = nullptr); + size_t WaitForCleanupAfterKeepAlive(Profile* profile = nullptr); void ExpectValuesFromFramesAreEmpty(const base::Location& location, const ValuesFromFrames& values); void ExpectValuesFromFrameAreEmpty(const base::Location& location, diff --git a/browser/ephemeral_storage/ephemeral_storage_forget_by_default_browsertest.cc b/browser/ephemeral_storage/ephemeral_storage_forget_by_default_browsertest.cc index 47a3992eed3f..da2be302ff51 100644 --- a/browser/ephemeral_storage/ephemeral_storage_forget_by_default_browsertest.cc +++ b/browser/ephemeral_storage/ephemeral_storage_forget_by_default_browsertest.cc @@ -10,6 +10,7 @@ #include "chrome/browser/content_settings/host_content_settings_map_factory.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/ui/browser.h" +#include "chrome/common/chrome_switches.h" #include "chrome/test/base/ui_test_utils.h" #include "components/content_settings/core/browser/cookie_settings.h" #include "components/content_settings/core/browser/host_content_settings_map.h" @@ -206,7 +207,7 @@ IN_PROC_BROWSER_TEST_F(EphemeralStorageForgetByDefaultBrowserTest, // After keepalive values should be cleared. ASSERT_TRUE(ui_test_utils::NavigateToURL(incognito_browser, b_site_ephemeral_storage_url_)); - WaitForCleanupAfterKeepAlive(incognito_browser); + WaitForCleanupAfterKeepAlive(incognito_browser->profile()); ASSERT_TRUE(ui_test_utils::NavigateToURL(incognito_browser, a_site_ephemeral_storage_url_)); @@ -570,4 +571,57 @@ IN_PROC_BROWSER_TEST_F(EphemeralStorageForgetByDefaultDisabledBrowserTest, EXPECT_EQ(1u, GetAllCookies().size()); } +class EphemeralStorageForgetByDefaultIncognitoBrowserTest + : public EphemeralStorageForgetByDefaultBrowserTest { + public: + EphemeralStorageForgetByDefaultIncognitoBrowserTest() { + scoped_feature_list_.InitAndEnableFeature( + net::features::kBraveForgetFirstPartyStorage); + } + ~EphemeralStorageForgetByDefaultIncognitoBrowserTest() override = default; + + void SetUpCommandLine(base::CommandLine* command_line) override { + EphemeralStorageForgetByDefaultBrowserTest::SetUpCommandLine(command_line); + if (IsPreTestToEnableIncognito()) { + command_line->AppendSwitch(switches::kIncognito); + } + } + + static bool IsPreTestToEnableIncognito() { + const testing::TestInfo* const test_info = + testing::UnitTest::GetInstance()->current_test_info(); + return base::StartsWith(test_info->name(), "PRE_DontForgetFirstParty"); + } + + private: + base::test::ScopedFeatureList scoped_feature_list_; +}; + +IN_PROC_BROWSER_TEST_F(EphemeralStorageForgetByDefaultIncognitoBrowserTest, + PRE_PRE_DontForgetFirstPartyIfNoBrowserWindowIsActive) { + const GURL a_site_set_cookie_url( + "https://a.com/set-cookie?name=acom;path=/" + ";SameSite=None;Secure;Max-Age=600"); + brave_shields::SetForgetFirstPartyStorageEnabled(content_settings(), true, + a_site_set_cookie_url); + + // Cookies should NOT exist for a.com. + EXPECT_EQ(0u, GetAllCookies().size()); + + EXPECT_TRUE(LoadURLInNewTab(a_site_set_cookie_url)); + + // Cookies SHOULD exist for a.com. + EXPECT_EQ(1u, GetAllCookies().size()); +} + +IN_PROC_BROWSER_TEST_F(EphemeralStorageForgetByDefaultIncognitoBrowserTest, + PRE_DontForgetFirstPartyIfNoBrowserWindowIsActive) { + EXPECT_TRUE(browser()->profile()->IsOffTheRecord()); + WaitForCleanupAfterKeepAlive(browser()->profile()->GetOriginalProfile()); +} + +IN_PROC_BROWSER_TEST_F(EphemeralStorageForgetByDefaultIncognitoBrowserTest, + DontForgetFirstPartyIfNoBrowserWindowIsActive) { + EXPECT_EQ(1u, GetAllCookies().size()); +} } // namespace ephemeral_storage diff --git a/browser/ephemeral_storage/ephemeral_storage_service_unittest.cc b/browser/ephemeral_storage/ephemeral_storage_service_unittest.cc index e145e4c4bdcf..5c3a222e596c 100644 --- a/browser/ephemeral_storage/ephemeral_storage_service_unittest.cc +++ b/browser/ephemeral_storage/ephemeral_storage_service_unittest.cc @@ -301,16 +301,17 @@ TEST_F(EphemeralStorageServiceForgetFirstPartyTest, CleanupOnRestart) { 1u); } - // Cleanup should happen in 5 seconds after the startup. + // No cleanup should happen at the restart when the profile has no browser window associated with it. { ScopedVerifyAndClearExpectations verify(mock_delegate_); ScopedVerifyAndClearExpectations verify_observer(&mock_observer_); - EXPECT_CALL(*mock_delegate_, - CleanupFirstPartyStorageArea(ephemeral_domain)); task_environment_.FastForwardBy(base::Seconds(5)); EXPECT_EQ( profile_.GetPrefs()->GetList(kFirstPartyStorageOriginsToCleanup).size(), - 0u); + 1u); + EXPECT_EQ( + mock_delegate_->DoesProfileHaveAnyBrowserWindow(), + false); } } diff --git a/components/ephemeral_storage/BUILD.gn b/components/ephemeral_storage/BUILD.gn index 1749037d4fa0..ddd4e06e0aa0 100644 --- a/components/ephemeral_storage/BUILD.gn +++ b/components/ephemeral_storage/BUILD.gn @@ -8,6 +8,7 @@ static_library("ephemeral_storage") { "ephemeral_storage_pref_names.h", "ephemeral_storage_service.cc", "ephemeral_storage_service.h", + "ephemeral_storage_service_delegate.cc", "ephemeral_storage_service_delegate.h", "ephemeral_storage_service_observer.h", "ephemeral_storage_types.h", diff --git a/components/ephemeral_storage/ephemeral_storage_service.cc b/components/ephemeral_storage/ephemeral_storage_service.cc index 5711652feba1..08adc7fbc40b 100644 --- a/components/ephemeral_storage/ephemeral_storage_service.cc +++ b/components/ephemeral_storage/ephemeral_storage_service.cc @@ -327,6 +327,11 @@ void EphemeralStorageService::ScheduleFirstPartyStorageAreasCleanupOnStartup() { void EphemeralStorageService::CleanupFirstPartyStorageAreasOnStartup() { DCHECK(!context_->IsOffTheRecord()); + + if (!delegate_->DoesProfileHaveAnyBrowserWindow()) { + first_party_storage_areas_to_cleanup_on_startup_.clear(); + return; + } ScopedListPrefUpdate pref_update(prefs_, kFirstPartyStorageOriginsToCleanup); for (const auto& url_to_cleanup : first_party_storage_areas_to_cleanup_on_startup_) { diff --git a/components/ephemeral_storage/ephemeral_storage_service_delegate.cc b/components/ephemeral_storage/ephemeral_storage_service_delegate.cc new file mode 100644 index 000000000000..f3f13f71993a --- /dev/null +++ b/components/ephemeral_storage/ephemeral_storage_service_delegate.cc @@ -0,0 +1,14 @@ +/* Copyright (c) 2024 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 "brave/components/ephemeral_storage/ephemeral_storage_service_delegate.h" + +namespace ephemeral_storage { + +bool EphemeralStorageServiceDelegate::DoesProfileHaveAnyBrowserWindow() const { + return false; +} + +} // namespace ephemeral_storage diff --git a/components/ephemeral_storage/ephemeral_storage_service_delegate.h b/components/ephemeral_storage/ephemeral_storage_service_delegate.h index 399ae1291745..c2f4323a24be 100644 --- a/components/ephemeral_storage/ephemeral_storage_service_delegate.h +++ b/components/ephemeral_storage/ephemeral_storage_service_delegate.h @@ -24,6 +24,7 @@ class EphemeralStorageServiceDelegate { // Cleanups non-ephemeral first party storage areas (cache, dom storage). virtual void CleanupFirstPartyStorageArea( const std::string& registerable_domain) {} + virtual bool DoesProfileHaveAnyBrowserWindow() const; }; } // namespace ephemeral_storage From 3f30bccc37c2bfe34d55b8bfc701f521e8a9596c Mon Sep 17 00:00:00 2001 From: Ilie Lesan Date: Fri, 3 Jan 2025 14:49:27 +0200 Subject: [PATCH 2/2] Run "npm run presubmit -- --fix" --- .../ephemeral_storage_forget_by_default_browsertest.cc | 1 - .../ephemeral_storage_service_unittest.cc | 7 +++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/browser/ephemeral_storage/ephemeral_storage_forget_by_default_browsertest.cc b/browser/ephemeral_storage/ephemeral_storage_forget_by_default_browsertest.cc index da2be302ff51..d806f4df46c1 100644 --- a/browser/ephemeral_storage/ephemeral_storage_forget_by_default_browsertest.cc +++ b/browser/ephemeral_storage/ephemeral_storage_forget_by_default_browsertest.cc @@ -4,7 +4,6 @@ * You can obtain one at https://mozilla.org/MPL/2.0/. */ #include "brave/browser/ephemeral_storage/ephemeral_storage_browsertest.h" - #include "brave/components/brave_shields/content/browser/brave_shields_util.h" #include "chrome/browser/content_settings/cookie_settings_factory.h" #include "chrome/browser/content_settings/host_content_settings_map_factory.h" diff --git a/browser/ephemeral_storage/ephemeral_storage_service_unittest.cc b/browser/ephemeral_storage/ephemeral_storage_service_unittest.cc index 5c3a222e596c..7696e59c4526 100644 --- a/browser/ephemeral_storage/ephemeral_storage_service_unittest.cc +++ b/browser/ephemeral_storage/ephemeral_storage_service_unittest.cc @@ -301,7 +301,8 @@ TEST_F(EphemeralStorageServiceForgetFirstPartyTest, CleanupOnRestart) { 1u); } - // No cleanup should happen at the restart when the profile has no browser window associated with it. + // No cleanup should happen at the restart when the profile has no browser + // window associated with it. { ScopedVerifyAndClearExpectations verify(mock_delegate_); ScopedVerifyAndClearExpectations verify_observer(&mock_observer_); @@ -309,9 +310,7 @@ TEST_F(EphemeralStorageServiceForgetFirstPartyTest, CleanupOnRestart) { EXPECT_EQ( profile_.GetPrefs()->GetList(kFirstPartyStorageOriginsToCleanup).size(), 1u); - EXPECT_EQ( - mock_delegate_->DoesProfileHaveAnyBrowserWindow(), - false); + EXPECT_EQ(mock_delegate_->DoesProfileHaveAnyBrowserWindow(), false); } }