Do not allow wildcard content settings entries for "shields enabled" #14031

petemill · 2022-07-01T19:51:03Z

Also remove any existing entries that may mistakingly be there

Resolves brave/brave-browser#23214

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:

Add an entry for shields down for any site
Close Brave and open Default/Preferences file
in braveShield section, change the site.com,* to *,*
Open and close Brave
*,* entry for braveShield should be removed

We don't current know how that entry gets created in normal use so there is no test plan for preventing it, but the code no longer allows default entries to be added for shields

boocmp · 2022-07-02T06:54:22Z

components/content_settings/core/browser/brave_content_settings_pref_provider.cc

@@ -168,6 +172,17 @@ void BravePrefProvider::MigrateShieldsSettings(bool incognito) {
  MigrateShieldsSettingsV2ToV3();
 }

+void BravePrefProvider::EnsureNoWildcardEntries() {


It seems you also need to add one 'if' in SetWebsiteSettingInternal. You can check it by clicking on 'Reset Settings'

one if for what? Not sure what Reset Settings has to do with this?

everything seems to be working correctly, I'm not understanding what the issue is here

components/content_settings/core/browser/brave_content_settings_pref_provider_unittest.cc

Also remove any existing entries that may mistakingly be there

LaurenWags · 2022-07-07T20:25:48Z

Verified using:

Brave | 1.43.6 Chromium: 103.0.5060.114 (Official Build) nightly (x86_64)
-- | --
Revision | a1c2360c5b02a6d4d6ab33796ad8a268a6128226-refs/branch-heads/5060@{#1124}
OS | macOS Version 12.4 (Build 21F79)

Reproduced the issue using STR from Do not allow wildcard content settings entries for "shields enabled" #14031 (comment) with 1.41.94.
Confirmed after "upgrading" (renaming profile to Nightly) and launching with 1.43.6 the issue no longer reproduced - every site I visited had shields up.
Closed Nightly and viewed the Preferences file
Confirmed the wildcard *,* was removed from braveShield section of Preferences file

Also tried the following scenario:

Installed 1.41.94
Visited several sites and modified shield configurations (some up, some down, some with various changes to individual settings like Aggressively Block Ads, Aggressively Block Fingerprinting, etc)
Closed 1.41.94
Modified Preferences file as per Do not allow wildcard content settings entries for "shields enabled" #14031 (comment) for just one of the entries
Relaunched 1.41.94 and experienced the issue
"Upgraded" (renamed profile to Nightly) and launched with 1.43.6
Confirmed new sites visited did not reproduce the issue and shields were up for them
Confirmed previously set shield settings (up, down, modifications to individual settings) were now showing as expected
Closed Nightly and viewed the Preferences file
Confirmed the wildcard *,* was removed from braveShield section of Preferences file

…-content-setting Do not allow wildcard content settings entries for "shields enabled"

kjozwiak · 2022-07-10T00:44:28Z

Using the STR/Cases outlined via #14031 (comment), reproduced the original issue using 1.41.94 Chromium: 103.0.5060.114. Once *,* was added into the Preference file, Brave Shields stopped working and was disabled for every website.

Verification PASSED on Win 11 x64 using the following build(s):

Brave | 1.43.9 Chromium: 103.0.5060.114 (Official Build) nightly (64-bit) 
--- | ---
Revision | a1c2360c5b02a6d4d6ab33796ad8a268a6128226-refs/branch-heads/5060@{#1124}
OS | Windows 11 Version 21H2 (Build 22000.739)

Test Case #1 - Basic/Simple check using STR/Cases from #14031 (comment)

installed 1.43.9 Chromium: 103.0.5060.114
visited https://videocardz.com
closed Brave and opened AppData\Local\BraveSoftware\Brave-Browser-Nightly\User Data\Default\Preferences
changed "videocardz.com,*" -> "*,*" via the braveShields section
relaunched the browser and ensured that Brave shields was enabled under https://videocardz.com
visited several other websites and ensured that Brave shields was still enabled

Test Case #2 - Upgrading (renaming the affected profile to Brave-Browser-Nightly)

installed 1.41.94 Chromium: 103.0.5060.114
visited https://videocardz.com and several other websites
closed Brave and opened AppData\Local\BraveSoftware\Brave-Browser\User Data\Default\Preferences
changed "videocardz.com,*" -> "*,*" via the braveShields section
re-launched the browser and reproduced the issue re: shields being disabled
opened ~10 websites and closed the browser (each website had shields disabled due to the issue)
renamed the Brave-Browser profile to Brave-Browser-Nightly
launched using 1.43.9 Chromium: 103.0.5060.114
ensured that Brave shields was enabled for all the opened tabs including https://videocardz.com
ensured "*,*" was removed from AppData\Local\BraveSoftware\Brave-Browser-Nightly\User Data\Default\Preferences

Test Case #3 - Clean Upgrading (no issue present when upgrading)

Quick check to ensure that shields settings are being retained on profiles that are not experiencing the issue

installed 1.41.94 Chromium: 103.0.5060.114
visited several websites and changed their Brave shields settings (Settings that were changed/modified):
- Aggressively block trackers & ads, Block trackers & ads, Allow all trackers & ads
- Upgrade connections to HTTPS, Block Scripts
- Aggressively block fingerprinting, Block fingerprinting, Allow fingerprinting
- Block all cookies, Block cross-site cookies, Allow all cookies
renamed the Brave-Browser profile to Brave-Browser-Nightly
launched using 1.43.9 Chromium: 103.0.5060.114
ensured that all the shields settings were retained without any issues

Test Case #4 - Upgrading affected profile

installed 1.41.94 Chromium: 103.0.5060.114
visited several websites and changed their Brave shields settings (Settings that were changed/modified):
- Aggressively block trackers & ads, Block trackers & ads, Allow all trackers & ads
- Upgrade connections to HTTPS, Block Scripts
- Aggressively block fingerprinting, Block fingerprinting, Allow fingerprinting
- Block all cookies, Block cross-site cookies, Allow all cookies
closed Brave and opened AppData\Local\BraveSoftware\Brave-Browser\User Data\Default\Preferences
changed one of the settings to "*,*" via the braveShields section
launched the browser and verified the that issue was reproducible
renamed the Brave-Browser profile to Brave-Browser-Nightly
launched using 1.43.9 Chromium: 103.0.5060.114
ensured that all the shields settings were retained without any issues and the issue wasn't occurring

petemill requested review from bridiver, nullhook, mkarolin and boocmp July 1, 2022 19:51

petemill self-assigned this Jul 1, 2022

petemill mentioned this pull request Jul 1, 2022

Shields should use GetURL for content settings #14032

Closed

25 tasks

petemill force-pushed the shields-enabled-migrate-wildcard-content-setting branch from 6880632 to 0cc33c2 Compare July 1, 2022 23:12

boocmp reviewed Jul 2, 2022

View reviewed changes

bridiver reviewed Jul 2, 2022

View reviewed changes

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

bridiver approved these changes Jul 3, 2022

View reviewed changes

boocmp approved these changes Jul 4, 2022

View reviewed changes

petemill and others added 2 commits July 4, 2022 10:43

Do not allow wildcard content settings entries for "shields enabled"

5c64850

Also remove any existing entries that may mistakingly be there

dump without crashing for shields wildcard setting

4b02a84

bridiver force-pushed the shields-enabled-migrate-wildcard-content-setting branch from 174a4b3 to 4b02a84 Compare July 4, 2022 17:43

bridiver merged commit 82a983c into master Jul 6, 2022

bridiver deleted the shields-enabled-migrate-wildcard-content-setting branch July 6, 2022 18:52

bridiver added this to the 1.43.x - Nightly milestone Jul 6, 2022

brave-builds pushed a commit that referenced this pull request Jul 7, 2022

Uplift of #14031 (squashed) to beta

bcb0525

brave-builds mentioned this pull request Jul 7, 2022

Do not allow wildcard content settings entries for "shields enabled" (uplift to 1.42.x) #14073

Merged

7 tasks

bridiver mentioned this pull request Jul 7, 2022

Do not allow wildcard content settings entries for "shields enabled" (uplift to 1.41.x) #14074

Merged

7 tasks

bridiver added a commit that referenced this pull request Jul 7, 2022

Merge pull request #14031 from brave/shields-enabled-migrate-wildcard…

dabf000

…-content-setting Do not allow wildcard content settings entries for "shields enabled"

MadhaviSeelam mentioned this pull request Jul 11, 2022

default shield-on option lost brave/brave-browser#23988

Closed

GeetaSarvadnya mentioned this pull request Jul 12, 2022

default shield-on option lost since a recent update brave/brave-browser#23214

Closed

kjozwiak mentioned this pull request Aug 23, 2022

Ensure no wildcard in BRAVE_SHIELDS. #14592

Merged

25 tasks

stephendonner mentioned this pull request Aug 23, 2022

Brave shields always down by default on sites, but enabled in global settings brave/brave-browser#24514

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not allow wildcard content settings entries for "shields enabled" #14031

Do not allow wildcard content settings entries for "shields enabled" #14031

petemill commented Jul 1, 2022 •

edited by LaurenWags

Loading

boocmp Jul 2, 2022

bridiver Jul 2, 2022

bridiver Jul 3, 2022

LaurenWags commented Jul 7, 2022

kjozwiak commented Jul 10, 2022

Do not allow wildcard content settings entries for "shields enabled" #14031

Do not allow wildcard content settings entries for "shields enabled" #14031

Conversation

petemill commented Jul 1, 2022 • edited by LaurenWags Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

boocmp Jul 2, 2022

Choose a reason for hiding this comment

bridiver Jul 2, 2022

Choose a reason for hiding this comment

bridiver Jul 3, 2022

Choose a reason for hiding this comment

LaurenWags commented Jul 7, 2022

kjozwiak commented Jul 10, 2022

petemill commented Jul 1, 2022 •

edited by LaurenWags

Loading