Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't clear first party storage area if browser is launched in incognito mode from command line #24543

Conversation

Ilie-Lesan
Copy link
Contributor

When opening the browser in incognito mode from command line, we should prevent deleting the origins marked with "Forget me when I close this site".

The reason for this is that the incognito window does not re-create the lifetime of TLD(TLDEphemeralLifetimeCreated never gets called because the default profile browser windows are not restored in incognito mode) from the main profile and as a result, they get scheduled for deletion.

Resolves brave/brave-browser#39107

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@goodov
Copy link
Member

goodov commented Jul 10, 2024

Thank you! Having a browser test for this would be nice. Can you try to add one please?

You can use this test as a starting point, you might need to use PRE_ tests to open few websites first.

IN_PROC_BROWSER_TEST_F(EphemeralStorageForgetByDefaultBrowserTest,
ForgetFirstPartyInheritedInIncognito) {
Browser* incognito_browser = CreateIncognitoBrowser(nullptr);
EXPECT_EQ(
brave_shields::GetCookieControlType(
HostContentSettingsMapFactory::GetForProfile(
incognito_browser->profile()),
CookieSettingsFactory::GetForProfile(incognito_browser->profile())
.get(),
a_site_ephemeral_storage_url_),
brave_shields::ControlType::BLOCK_THIRD_PARTY);
brave_shields::SetForgetFirstPartyStorageEnabled(
content_settings(), true, a_site_ephemeral_storage_url_);
EXPECT_TRUE(brave_shields::GetForgetFirstPartyStorageEnabled(
HostContentSettingsMapFactory::GetForProfile(
incognito_browser->profile()),
a_site_ephemeral_storage_url_));
ASSERT_TRUE(ui_test_utils::NavigateToURL(incognito_browser,
a_site_ephemeral_storage_url_));
auto* incognito_web_contents =
incognito_browser->tab_strip_model()->GetActiveWebContents();
// We set a value in the page where all the frames are first-party.
SetAndCheckValuesInFrames(incognito_web_contents, "a.com", "from=a.com");
// After keepalive values should be cleared.
ASSERT_TRUE(ui_test_utils::NavigateToURL(incognito_browser,
b_site_ephemeral_storage_url_));
WaitForCleanupAfterKeepAlive(incognito_browser);
ASSERT_TRUE(ui_test_utils::NavigateToURL(incognito_browser,
a_site_ephemeral_storage_url_));
ExpectValuesFromFramesAreEmpty(FROM_HERE,
GetValuesFromFrames(incognito_web_contents));
}

@@ -327,6 +327,13 @@ void EphemeralStorageService::ScheduleFirstPartyStorageAreasCleanupOnStartup() {

void EphemeralStorageService::CleanupFirstPartyStorageAreasOnStartup() {
DCHECK(!context_->IsOffTheRecord());

Profile* profile = Profile::FromBrowserContext(context_);
if (profile->HasAnyOffTheRecordProfile()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe the better approach here is to detect if any "normal" profile windows are opened or not.

Copy link
Contributor Author

@Ilie-Lesan Ilie-Lesan Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion, I will try to follow it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Please have a look

@Ilie-Lesan
Copy link
Contributor Author

Thank you! Having a browser test for this would be nice. Can you try to add one please?

You can use this test as a starting point, you might need to use PRE_ tests to open few websites first.

IN_PROC_BROWSER_TEST_F(EphemeralStorageForgetByDefaultBrowserTest,
ForgetFirstPartyInheritedInIncognito) {
Browser* incognito_browser = CreateIncognitoBrowser(nullptr);
EXPECT_EQ(
brave_shields::GetCookieControlType(
HostContentSettingsMapFactory::GetForProfile(
incognito_browser->profile()),
CookieSettingsFactory::GetForProfile(incognito_browser->profile())
.get(),
a_site_ephemeral_storage_url_),
brave_shields::ControlType::BLOCK_THIRD_PARTY);
brave_shields::SetForgetFirstPartyStorageEnabled(
content_settings(), true, a_site_ephemeral_storage_url_);
EXPECT_TRUE(brave_shields::GetForgetFirstPartyStorageEnabled(
HostContentSettingsMapFactory::GetForProfile(
incognito_browser->profile()),
a_site_ephemeral_storage_url_));
ASSERT_TRUE(ui_test_utils::NavigateToURL(incognito_browser,
a_site_ephemeral_storage_url_));
auto* incognito_web_contents =
incognito_browser->tab_strip_model()->GetActiveWebContents();
// We set a value in the page where all the frames are first-party.
SetAndCheckValuesInFrames(incognito_web_contents, "a.com", "from=a.com");
// After keepalive values should be cleared.
ASSERT_TRUE(ui_test_utils::NavigateToURL(incognito_browser,
b_site_ephemeral_storage_url_));
WaitForCleanupAfterKeepAlive(incognito_browser);
ASSERT_TRUE(ui_test_utils::NavigateToURL(incognito_browser,
a_site_ephemeral_storage_url_));
ExpectValuesFromFramesAreEmpty(FROM_HERE,
GetValuesFromFrames(incognito_web_contents));
}

Yes, I will try to write a browsing test. Thanks!

@Ilie-Lesan
Copy link
Contributor Author

Thank you! Having a browser test for this would be nice. Can you try to add one please?

You can use this test as a starting point, you might need to use PRE_ tests to open few websites first.

IN_PROC_BROWSER_TEST_F(EphemeralStorageForgetByDefaultBrowserTest,
ForgetFirstPartyInheritedInIncognito) {
Browser* incognito_browser = CreateIncognitoBrowser(nullptr);
EXPECT_EQ(
brave_shields::GetCookieControlType(
HostContentSettingsMapFactory::GetForProfile(
incognito_browser->profile()),
CookieSettingsFactory::GetForProfile(incognito_browser->profile())
.get(),
a_site_ephemeral_storage_url_),
brave_shields::ControlType::BLOCK_THIRD_PARTY);
brave_shields::SetForgetFirstPartyStorageEnabled(
content_settings(), true, a_site_ephemeral_storage_url_);
EXPECT_TRUE(brave_shields::GetForgetFirstPartyStorageEnabled(
HostContentSettingsMapFactory::GetForProfile(
incognito_browser->profile()),
a_site_ephemeral_storage_url_));
ASSERT_TRUE(ui_test_utils::NavigateToURL(incognito_browser,
a_site_ephemeral_storage_url_));
auto* incognito_web_contents =
incognito_browser->tab_strip_model()->GetActiveWebContents();
// We set a value in the page where all the frames are first-party.
SetAndCheckValuesInFrames(incognito_web_contents, "a.com", "from=a.com");
// After keepalive values should be cleared.
ASSERT_TRUE(ui_test_utils::NavigateToURL(incognito_browser,
b_site_ephemeral_storage_url_));
WaitForCleanupAfterKeepAlive(incognito_browser);
ASSERT_TRUE(ui_test_utils::NavigateToURL(incognito_browser,
a_site_ephemeral_storage_url_));
ExpectValuesFromFramesAreEmpty(FROM_HERE,
GetValuesFromFrames(incognito_web_contents));
}

@goodov Can you help me a bit with the testing? It looks like reproducing this scenario in the test is a bit hard.
I've tried multiple solutions but no one worked well. Can you have a look and tell me what I'm not doing right? Thanks!
https://gist.github.com/tech-zilla/0c58a249ae26c80cc296ce8fdf0f34a4

@@ -327,6 +339,11 @@ void EphemeralStorageService::ScheduleFirstPartyStorageAreasCleanupOnStartup() {

void EphemeralStorageService::CleanupFirstPartyStorageAreasOnStartup() {
DCHECK(!context_->IsOffTheRecord());
Profile* profile = Profile::FromBrowserContext(context_);
if (!DoesProfileHaveAnyBrowserWindow(profile)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, now I'm thinking: shouldn't we instead start the cleanup process timer when a regular profile window is created (or exists) for context_?

right now if you open a normal window with an existing incognito window, the cleanup won't run.

Copy link
Contributor Author

@Ilie-Lesan Ilie-Lesan Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I saw in the code, there are two situations that trigger the cleanup.

  1. At startup, there is a 5-second keep-alive timer after which the cleanup is executed(so if you are quick enough to enter the URL in the address bar, the cleanup is aborted)
  2. After closing a tab, there is a 30-second grace period (this option is enabled by default) after which all data cleanup is started.

Are you running in one of these situations? Can you please add some steps to reproduce? Thanks!

@goodov
Copy link
Member

goodov commented Jul 15, 2024

Can you help me a bit with the testing? It looks like reproducing this scenario in the test is a bit hard.
I've tried multiple solutions but no one worked well. Can you have a look and tell me what I'm not doing right? Thanks!

You need to create a separate test fixture with SetUpCommandLine override. If it's a non-PRE test, then you should add switches::kIncognito.

something like:

 void SetUpCommandLine(base::CommandLine* command_line) override {
    EphemeralStorageForgetByDefaultBrowserTest::SetUpCommandLine(command_line);
    std::string test_name =
        ::testing::UnitTest::GetInstance()->current_test_info()->name();
    const bool is_pre_test = test_name.starts_with("PRE_");
    if (!is_pre_test) {
      command_line->AppendSwitch(switches::kIncognito);
    }
  }

in non-PRE test you need to call WaitForCleanupAfterKeepAlive with a non-incognito profile and after that ensure that data (you can check cookies) is not removed there.

@goodov
Copy link
Member

goodov commented Jul 25, 2024

@tech-zilla

+ npm run presubmit
Running Python 3 presubmit upload checks ...
  checking for commit objects in tree took a long time: 0.5s
** Presubmit Warnings: 2 **
Found 1 lines longer than 80 characters (first 5 shown).
  browser/ephemeral_storage/ephemeral_storage_browsertest.cc, line 312, 84 chars
Found line ending with white spaces in:
***************
browser/ephemeral_storage/ephemeral_storage_forget_by_default_browsertest.cc:612
***************
** Presubmit ERRORS: 2 **
The brave directory requires source formatting. Please run: npm run presubmit -- --fix.
--- browser/ephemeral_storage/ephemeral_storage_browsertest.cc  (before formatting)
+++ browser/ephemeral_storage/ephemeral_storage_browsertest.cc  (after formatting)
@@ -309,7 +309,8 @@
   return content::EvalJs(host, "document.cookie");
 }
-size_t EphemeralStorageBrowserTest::WaitForCleanupAfterKeepAlive(Profile* profile) {
+size_t EphemeralStorageBrowserTest::WaitForCleanupAfterKeepAlive(
+    Profile* profile) {
   if (!profile) {
     profile = browser()->profile();
   }
@@ -322,8 +323,7 @@
   // 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 =
-      profile->GetBrowsingDataRemover();
+  content::BrowsingDataRemover* remover = profile->GetBrowsingDataRemover();
   remover->AddObserver(&data_remover_observer);
   remover->RemoveAndReply(base::Time(), base::Time::Max(), 0, 0,
                           &data_remover_observer);
You added one or more #includes that violate checkdeps rules.
  brave/components/ephemeral_storage/ephemeral_storage_service.cc
      Illegal include: "chrome/browser/profiles/profile.h"
    Because of "-chrome" from brave's include_rules.
  brave/components/ephemeral_storage/ephemeral_storage_service.cc
      Illegal include: "chrome/browser/ui/browser.h"
    Because of "-chrome" from brave's include_rules.
  brave/components/ephemeral_storage/ephemeral_storage_service.cc
      Illegal include: "chrome/browser/ui/browser_finder.h"
    Because of "-chrome" from brave's include_rules.
  brave/components/ephemeral_storage/ephemeral_storage_service.cc
      Illegal include: "chrome/browser/ui/browser_window.h"
    Because of "-chrome" from brave's include_rules.
Presubmit checks took 2.6s to calculate.
There were Python 3 presubmit errors.
null
Process exited with code 1

you need to move the added function into EphemeralStorageServiceDelegate and format things.

make sure to rebase before doing that.

Copy link
Member

@goodov goodov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very good job! thank you! Let's fix few things and I trigger CI.

return;
}
}
is_window_visible = false;
Copy link
Member

@goodov goodov Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just return a boolean from the function, I don't see why you need an optional returned as a mutable parameter here.

if you need to have a default implementation for the virtual function in .cc in this case (clang checks may ask for it), just add a .cc file and implement it, no big deal.

@@ -4,12 +4,12 @@
* You can obtain one at https://mozilla.org/MPL/2.0/. */

#include "brave/browser/ephemeral_storage/ephemeral_storage_browsertest.h"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please run npm run presubmit --fix locally before pushing things. I think the format checks won't like the removal of this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still seeing some warnings after adding the space, that's why I didn't remove it
image

}
}

static bool IsPreTest() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think this should be called IsPreTestToEnableIncognito, because you have 2 PRE_ tests, but you're actually interested in one of them.

@Ilie-Lesan
Copy link
Contributor Author

thanks for the support during the code review!

@goodov
Copy link
Member

goodov commented Oct 31, 2024

@Ilie-Lesan

======= Failed test run #1 ==========
  FAILURE
  ------- Stdout: -------
  [ RUN      ] EphemeralStorageServiceForgetFirstPartyTest.CleanupOnRestart
  ../../brave/browser/ephemeral_storage/ephemeral_storage_service_unittest.cc:313: Failure
  Expected equality of these values:
    profile_.GetPrefs()->GetList(kFirstPartyStorageOriginsToCleanup).size()
      Which is: 1
    0u
      Which is: 0
  Stack trace:
  #0 0x5b4256d1a4ec ephemeral_storage::EphemeralStorageServiceForgetFirstPartyTest_CleanupOnRestart_Test::TestBody() [../../brave/browser/ephemeral_storage/ephemeral_storage_service_unittest.cc:311:5]
  ../../brave/browser/ephemeral_storage/ephemeral_storage_service_unittest.cc:309: Failure
  Actual function call count doesn't match EXPECT_CALL(*mock_delegate_, CleanupFirstPartyStorageArea(ephemeral_domain))...
           Expected: to be called once
             Actual: never called - unsatisfied and active
  Stack trace:
  #0 0x5b425e5e361b testing::internal::GoogleTestFailureReporter::ReportFailure() [../../third_party/googletest/src/googlemock/src/gmock-internal-utils.cc:108:47]
  #1 0x5b425e5e6dba testing::internal::UntypedFunctionMockerBase::VerifyAndClearExpectationsLocked() [../../third_party/googletest/src/googlemock/include/gmock/internal/gmock-internal-utils.h:259:27]
  #2 0x5b425e5e77e3 testing::Mock::VerifyAndClearExpectationsLocked() [../../third_party/googletest/src/googlemock/src/gmock-spec-builders.cc:655:17]
  #3 0x5b425e5e7681 testing::Mock::VerifyAndClearExpectations() [../../third_party/googletest/src/googlemock/src/gmock-spec-builders.cc:624:10]
  #4 0x5b4256d17e08 ephemeral_storage::(anonymous namespace)::ScopedVerifyAndClearExpectations::~ScopedVerifyAndClearExpectations() [../../brave/browser/ephemeral_storage/ephemeral_storage_service_unittest.cc:32:5]
  #5 0x5b4256d1a556 ephemeral_storage::EphemeralStorageServiceForgetFirstPartyTest_CleanupOnRestart_Test::TestBody() [../../brave/browser/ephemeral_storage/ephemeral_storage_service_unittest.cc:314:3]
  [  FAILED  ] EphemeralStorageServiceForgetFirstPartyTest.CleanupOnRestart (244 ms)
  [4509/7015] EphemeralStorageServiceForgetFirstPartyTest.CleanupOnRestart (244 ms)

@Ilie-Lesan
Copy link
Contributor Author

@Ilie-Lesan

======= Failed test run #1 ==========
  FAILURE
  ------- Stdout: -------
  [ RUN      ] EphemeralStorageServiceForgetFirstPartyTest.CleanupOnRestart
  ../../brave/browser/ephemeral_storage/ephemeral_storage_service_unittest.cc:313: Failure
  Expected equality of these values:
    profile_.GetPrefs()->GetList(kFirstPartyStorageOriginsToCleanup).size()
      Which is: 1
    0u
      Which is: 0
  Stack trace:
  #0 0x5b4256d1a4ec ephemeral_storage::EphemeralStorageServiceForgetFirstPartyTest_CleanupOnRestart_Test::TestBody() [../../brave/browser/ephemeral_storage/ephemeral_storage_service_unittest.cc:311:5]
  ../../brave/browser/ephemeral_storage/ephemeral_storage_service_unittest.cc:309: Failure
  Actual function call count doesn't match EXPECT_CALL(*mock_delegate_, CleanupFirstPartyStorageArea(ephemeral_domain))...
           Expected: to be called once
             Actual: never called - unsatisfied and active
  Stack trace:
  #0 0x5b425e5e361b testing::internal::GoogleTestFailureReporter::ReportFailure() [../../third_party/googletest/src/googlemock/src/gmock-internal-utils.cc:108:47]
  #1 0x5b425e5e6dba testing::internal::UntypedFunctionMockerBase::VerifyAndClearExpectationsLocked() [../../third_party/googletest/src/googlemock/include/gmock/internal/gmock-internal-utils.h:259:27]
  #2 0x5b425e5e77e3 testing::Mock::VerifyAndClearExpectationsLocked() [../../third_party/googletest/src/googlemock/src/gmock-spec-builders.cc:655:17]
  #3 0x5b425e5e7681 testing::Mock::VerifyAndClearExpectations() [../../third_party/googletest/src/googlemock/src/gmock-spec-builders.cc:624:10]
  #4 0x5b4256d17e08 ephemeral_storage::(anonymous namespace)::ScopedVerifyAndClearExpectations::~ScopedVerifyAndClearExpectations() [../../brave/browser/ephemeral_storage/ephemeral_storage_service_unittest.cc:32:5]
  #5 0x5b4256d1a556 ephemeral_storage::EphemeralStorageServiceForgetFirstPartyTest_CleanupOnRestart_Test::TestBody() [../../brave/browser/ephemeral_storage/ephemeral_storage_service_unittest.cc:314:3]
  [  FAILED  ] EphemeralStorageServiceForgetFirstPartyTest.CleanupOnRestart (244 ms)
  [4509/7015] EphemeralStorageServiceForgetFirstPartyTest.CleanupOnRestart (244 ms)

@goodov Completely forgot about this PR. I will have a look next week

Brandon-T and others added 17 commits January 2, 2025 19:20
…ch (brave#26531)

* Add Smart-Proxy Routing Toggle
* Add VPN Kill-Switch Toggle
* Upgrade to GuardianConnect 2.0.1
This introduces a closer degree of parity with other browsers by supporting the de-facto standard of using Ctrl+Shift+S to create a screenshot.

Resolves brave/brave-browser#42682
Create & use a frozen copy of window.origin.
fix brave/brave-browser#42693

When sidebar menu is handled, it should check previous menu state.
If prev entry is separator, it should be removed before sidebar menu
because sidebar menu will add it's own top & bottom separators.
Bumps [path-to-regexp](https://github.com/pillarjs/path-to-regexp) and [express](https://github.com/expressjs/express). These dependencies needed to be updated together.

Updates `path-to-regexp` from 0.1.10 to 0.1.12
- [Release notes](https://github.com/pillarjs/path-to-regexp/releases)
- [Changelog](https://github.com/pillarjs/path-to-regexp/blob/master/History.md)
- [Commits](pillarjs/path-to-regexp@v0.1.10...v0.1.12)

Updates `express` from 4.21.1 to 4.21.2
- [Release notes](https://github.com/expressjs/express/releases)
- [Changelog](https://github.com/expressjs/express/blob/4.21.2/History.md)
- [Commits](expressjs/express@4.21.1...4.21.2)

---
updated-dependencies:
- dependency-name: path-to-regexp
  dependency-type: indirect
- dependency-name: express
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
* Disabled cosmetic filters on speedreader pages.
@Ilie-Lesan
Copy link
Contributor Author

Messed up the branch after wrong rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet