Cookies #13126

boocmp · 2022-04-22T06:37:13Z

Resolves brave/brave-browser#8604,

The combo-box in the Shield pop-up window now shows the actual block status.
Allow all & block all states now produces only one rule in the content settings.
Shields rules (from the combobox) can now be removed by clicking on trash icon in settings.
The Shield down rule is now contained in a separate container and cannot be deleted in settings page.
The "http://firstpaty" placeholder has been removed and the order of the primary/secondary patterns is now correct (not reversed)
Removed default rules from BRAVE_COOKIES, values from settings are used instead.

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
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, npm run lint, 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:

https://drive.google.com/file/d/1nZhhHR0rOsfZzHa-C7TnIdpggOtkHwKO/view?usp=sharing
https://drive.google.com/file/d/1ZguyenegGPPJ7bXm4aR-o9sKSebnfe-b/view?usp=sharing
check_list_1-17.txt

components/content_settings/core/browser/brave_content_settings_pref_provider.cc

bridiver · 2022-04-27T04:23:19Z

components/brave_shields/browser/brave_shields_util.cc

+  ContentSetting general_setting = CONTENT_SETTING_DEFAULT;
+  ContentSetting first_party_setting = CONTENT_SETTING_DEFAULT;
+};
+
 // TODO(bridiver) - convert cookie settings to ContentSettingsType::COOKIES


I don't think this comment is relevant anymore since we can't use the upstream 3p setting to eliminate having two rules for block 3p

iefremov · 2022-05-10T22:58:28Z

@goodov ptal?

goodov · 2022-04-24T10:41:09Z

components/content_settings/core/browser/brave_content_settings_pref_provider.cc

+      };
+  const auto cookie_is_found_in =
+      [&rule_matcher](const std::vector<Rule>& rules) {
+        return rules.cend() != base::ranges::find_if(rules, rule_matcher);


goodov · 2022-04-24T10:52:32Z

components/content_settings/core/browser/brave_content_settings_pref_provider.cc

+    for (auto&& r : rules) {
+      cookie_rules_[incognito].SetValue(
+          r.primary_pattern, r.secondary_pattern, ContentSettingsType::COOKIES,
+          base::Time(), std::move(r.value), {r.expiration, r.session_model});


is base::Time() alright? won't it break some "delete all data for 1 hour" logic?

goodov · 2022-05-12T11:30:38Z

browser/net/brave_network_delegate_browsertest.cc

                                        brave_shields::ControlType::ALLOW, url);
  }

  void BlockThirdPartyCookies(const GURL url) {
    brave_shields::SetCookieControlType(
-        content_settings(), brave_shields::ControlType::BLOCK_THIRD_PARTY, url);
+        content_settings(), browser()->profile()->GetPrefs(),
+        brave_shields::ControlType::BLOCK_THIRD_PARTY, url);
  }

  void BlockCookies(const GURL url) {


const GURL url -> const GURL& url everywhere in this file

goodov · 2022-05-12T11:42:30Z

components/brave_shields/browser/brave_shields_util.cc

+      {"*://[*.]",
+       net::registry_controlled_domains::GetDomainAndRegistry(
+           host_pattern.GetHost(),
+           net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES),


this will return an empty string if the host is an IP address or if a registry is not found (ipfs cases?).

goodov · 2022-05-12T11:48:50Z

components/brave_shields/browser/brave_shields_util.cc

+      // first-party rule:
+      map->SetContentSettingCustomScope(primary_pattern, host_pattern,
+                                        ContentSettingsType::BRAVE_COOKIES,
+                                        GetDefaultAllowFromControlType(type));


a good comment for all cases here would be nice. currently it's unclear why we come up with primary_pattern and not stick with host_pattern only. why we use GetDefaultBlockFromControlType, GetDefaultAllowFromControlType, CONTENT_SETTING_DEFAULT in different Sets?

goodov · 2022-05-12T11:51:10Z

components/brave_shields/browser/brave_shields_util.cc


  RecordShieldsSettingChanged(local_state);
 }
+
+struct CookieRules {


is this an internal helper only? anonymous namespace then.
also should be a class with private members modifiable only with Merge.

goodov · 2022-05-12T12:15:00Z

components/brave_shields/browser/cookie_pref_service.h

+namespace content_settings {
+class CookieSettings;
+}
+
 namespace brave_shields {

 // sync brave plugin cookie settings with chromium cookie prefs


it doesn't do anything like that anymore.

goodov · 2022-05-12T12:16:36Z

components/brave_shields/browser/cookie_pref_service.cc

-  pref_change_registrar_.Add(
+CookiePrefService::CookiePrefService(PrefService* prefs) {
+  DCHECK(prefs);
+  prefs->SetDefaultPrefValue(


I think maybe we just should drop this class and do this with a patch or in some other place (maybe even create such place to be able to override any default chromium prefs in future).

goodov · 2022-05-12T12:18:30Z

components/content_settings/core/browser/brave_content_settings_pref_provider.cc

  for (const auto& shield_rule : shield_rules) {
    auto primary_compare =
-        shield_rule.primary_pattern.Compare(cookie_rule.primary_pattern);
+        shield_rule.primary_pattern.Compare(cookie_rule.secondary_pattern);
    // TODO(bridiver) - verify that SUCCESSOR is correct and not PREDECESSOR


what about this and next TODO? do they still make sense or we can drop them?

goodov · 2022-05-12T12:22:36Z

components/content_settings/core/browser/brave_content_settings_pref_provider.cc

+          .primary_pattern = ContentSettingsPattern::FromString(base::StrCat(
+          {"*://[*.]",
+           net::registry_controlled_domains::GetDomainAndRegistry(
+               new_rule.secondary_pattern.GetHost(),


same thing with ip address?

iefremov · 2022-06-15T17:56:01Z

components/content_settings/core/browser/brave_content_settings_pref_provider.cc

+    if (old_rule.primary_pattern == wildcard &&
+        (old_rule.secondary_pattern == wildcard ||
+         old_rule.secondary_pattern == first_party)) {
+      // Remove default rules from BRAVE_COKIES because it's already mapped to


BRAVE_COOKIES

iefremov · 2022-06-15T17:56:44Z

components/content_settings/core/browser/brave_content_settings_pref_provider.cc

+
+  if (content_type == ContentSettingsType::COOKIES) {
+    if (cookie_is_found_in(brave_shield_down_rules_[off_the_record_])) {
+      // Don't do anything with the generated Shiels-down rules. Unremovable


shields down

iefremov · 2022-06-15T18:02:13Z

@boocmp pls wait for the channel migration, then merge to 1.42

… consistent state. 2. The Brave shields settings now have a higher priority than Chromium settings. 3. If Brave shield is down, the cookie settings fallback to Chromium settings. 4. The Brave shields cookie settings can be edited on the cookie settings page. 5. Fixed the reverse order of patterns in BRAVE_COOKIES. 6. Default rules [*,*] was removed from BRAVE_COOKIES.

bridiver · 2022-08-01T21:46:04Z

components/brave_shields/browser/brave_shields_util.cc


-  ContentSetting fp_setting = map->GetContentSetting(
-      url, GURL("https://firstParty/"), ContentSettingsType::BRAVE_COOKIES);
+  auto result = CookieRules::Get(map, url, ContentSettingsType::COOKIES);


This should be BRAVE_COOKIES because otherwise the control will show the combined value of shield and setting cookies (incorrectly in many cases) and not be an accurate reflection of the actual setting. See brave/brave-browser#24321

boocmp requested a review from iefremov as a code owner April 22, 2022 06:37

boocmp requested a review from goodov April 22, 2022 06:52

boocmp force-pushed the cookies branch from 013ffe2 to 965b0dd Compare April 22, 2022 08:24

boocmp self-assigned this Apr 23, 2022

iefremov requested a review from bridiver April 26, 2022 12:45

bridiver reviewed Apr 27, 2022

View reviewed changes

components/content_settings/core/browser/brave_content_settings_pref_provider.cc Show resolved Hide resolved

bridiver reviewed Apr 27, 2022

View reviewed changes

goodov reviewed May 12, 2022

View reviewed changes

boocmp force-pushed the cookies branch 2 times, most recently from 3cf6935 to d90d07f Compare May 16, 2022 09:01

goodov approved these changes May 19, 2022

View reviewed changes

boocmp force-pushed the cookies branch from d90d07f to 55f33c9 Compare May 23, 2022 07:09

boocmp force-pushed the cookies branch 2 times, most recently from f93d923 to 34b1f12 Compare June 6, 2022 10:32

boocmp force-pushed the cookies branch 2 times, most recently from 5221a90 to 53146eb Compare June 15, 2022 06:58

iefremov reviewed Jun 15, 2022

View reviewed changes

iefremov approved these changes Jun 15, 2022

View reviewed changes

boocmp added 9 commits June 17, 2022 12:39

Removed unused code.

6ac8c90

AllowRefferers renamed to AreReferrersAllowed.

55db6bc

Simplified CookiePrefService.

2c333e9

Lint.

38101ae

Fixed cookie controls mode pref.

ad753cf

Allow all cookies when Shield is down.

a616023

Fixed tests & CookiePrefService more simplified.

e77485f

Removed unnecessary lambda args copy.

ae98d67

boocmp added 11 commits June 17, 2022 12:39

Removed unnecessary PostTask.

e408ef9

Update Advanced panel when toggle shields.

ad87656

Default settings support.

662e532

Tests.

ce79d07

Added migration for BRAVE_COOKIES from version 2 to 3.

31a5068

Lint.

24a3879

gn format.

6810f91

Changed rules container.

b500bbf

Review issues are adressed.

86d2a5a

Rebased.

db84215

Removed cookie_pref_service

3e3d5ea

boocmp force-pushed the cookies branch from 53146eb to 3e3d5ea Compare June 17, 2022 07:06

boocmp merged commit 835f7a7 into master Jun 17, 2022

boocmp deleted the cookies branch June 17, 2022 09:34

github-actions bot added this to the 1.42.x - Nightly milestone Jun 17, 2022

boocmp mentioned this pull request Jun 17, 2022

Broken component build on Linux & Android brave/brave-browser#23535

Closed

bsclifton mentioned this pull request Jun 21, 2022

Weird cookie issue - conflict with shields brave/brave-browser#23567

Closed

bridiver reviewed Aug 1, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cookies #13126

Cookies #13126

boocmp commented Apr 22, 2022 •

edited

Loading

bridiver Apr 27, 2022

iefremov commented May 10, 2022

goodov Apr 24, 2022

goodov Apr 24, 2022

goodov May 12, 2022

goodov May 12, 2022

goodov May 12, 2022

goodov May 12, 2022

goodov May 12, 2022

goodov May 12, 2022

goodov May 12, 2022

goodov May 12, 2022

iefremov Jun 15, 2022

iefremov Jun 15, 2022

iefremov commented Jun 15, 2022

bridiver Aug 1, 2022

Cookies #13126

Cookies #13126

Conversation

boocmp commented Apr 22, 2022 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Choose a reason for hiding this comment

iefremov commented May 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iefremov commented Jun 15, 2022

Choose a reason for hiding this comment

boocmp commented Apr 22, 2022 •

edited

Loading