From bcb0525fe07eb16f48188c45446a8f3bc5f91fcc Mon Sep 17 00:00:00 2001 From: brave-builds Date: Thu, 7 Jul 2022 21:36:18 +0000 Subject: [PATCH] Uplift of #14031 (squashed) to beta --- .../browser/brave_shields_util.cc | 24 +++++++++++++++++-- .../brave_content_settings_pref_provider.cc | 23 ++++++++++++++++-- .../brave_content_settings_pref_provider.h | 2 ++ ...content_settings_pref_provider_unittest.cc | 22 +++++++++++++++++ 4 files changed, 67 insertions(+), 4 deletions(-) diff --git a/components/brave_shields/browser/brave_shields_util.cc b/components/brave_shields/browser/brave_shields_util.cc index 85be175370b0..355cedfff1e7 100644 --- a/components/brave_shields/browser/brave_shields_util.cc +++ b/components/brave_shields/browser/brave_shields_util.cc @@ -7,7 +7,9 @@ #include +#include "base/debug/dump_without_crashing.h" #include "base/feature_list.h" +#include "base/logging.h" #include "base/notreached.h" #include "base/strings/string_number_conversions.h" #include "brave/components/brave_shields/browser/brave_shields_p3a.h" @@ -181,12 +183,30 @@ void SetBraveShieldsEnabled(HostContentSettingsMap* map, if (url.is_valid() && !url.SchemeIsHTTPOrHTTPS()) return; - DCHECK(!url.is_empty()) << "url for shields setting cannot be blank"; + if (url.is_empty()) { + LOG(ERROR) << "url for shields setting cannot be blank"; + return; + } auto primary_pattern = GetPatternFromURL(url); - if (!primary_pattern.IsValid()) + if (primary_pattern.MatchesAllHosts()) { + LOG(ERROR) << "Url for shields setting cannot be blank or result in a " + "wildcard content setting."; + +#if DCHECK_IS_ON() + DCHECK(false); +#else + base::debug::DumpWithoutCrashing(); +#endif return; + } + + if (!primary_pattern.IsValid()) { + DLOG(ERROR) << "Invalid primary pattern for Url: " + << url.possibly_invalid_spec(); + return; + } map->SetContentSettingCustomScope( primary_pattern, ContentSettingsPattern::Wildcard(), diff --git a/components/content_settings/core/browser/brave_content_settings_pref_provider.cc b/components/content_settings/core/browser/brave_content_settings_pref_provider.cc index 05641fb21116..6c9a011923ec 100644 --- a/components/content_settings/core/browser/brave_content_settings_pref_provider.cc +++ b/components/content_settings/core/browser/brave_content_settings_pref_provider.cc @@ -157,6 +157,10 @@ void BravePrefProvider::MigrateShieldsSettings(bool incognito) { *shields_cookies); } + // Fix any wildcard entries that could cause issues like + // https://github.com/brave/brave-browser/issues/23113 + EnsureNoWildcardEntries(); + // Prior to Chromium 88, we used the "plugins" ContentSettingsType along with // ResourceIdentifiers to store our settings, which we need to migrate now // first of all, before attempting any other migration. @@ -168,6 +172,17 @@ void BravePrefProvider::MigrateShieldsSettings(bool incognito) { MigrateShieldsSettingsV2ToV3(); } +void BravePrefProvider::EnsureNoWildcardEntries() { + // ContentSettingsType::BRAVE_SHIELDS should not have wildcard entries, i.e. + // there is no global disabled value. + // TODO(petemill): This should also be done for the other shields + // content settings types, and we can use default boolean prefs to represent + // defaults, e.g. `profile.default_content_setting_values.https_everywhere`. + SetWebsiteSetting(ContentSettingsPattern::Wildcard(), + ContentSettingsPattern::Wildcard(), + ContentSettingsType::BRAVE_SHIELDS, base::Value(), {}); +} + void BravePrefProvider::MigrateShieldsSettingsFromResourceIds() { BravePrefProvider::CopyPluginSettingsForMigration(prefs_); @@ -552,8 +567,12 @@ void BravePrefProvider::UpdateCookieRules(ContentSettingsType content_type, // Adding shields down rules (they always override cookie rules). for (const auto& shield_rule : shield_rules) { - // There is no global shields rule - DCHECK(!shield_rule.primary_pattern.MatchesAllHosts()); + // There is no global shields rule, so if we have one ignore it. It would + // get replaced with EnsureNoWildcardEntries(). + if (shield_rule.primary_pattern.MatchesAllHosts()) { + LOG(ERROR) << "Found a wildcard shields rule which matches all hosts."; + continue; + } // Shields down. if (ValueToContentSetting(shield_rule.value) == CONTENT_SETTING_BLOCK) { diff --git a/components/content_settings/core/browser/brave_content_settings_pref_provider.h b/components/content_settings/core/browser/brave_content_settings_pref_provider.h index ac9d1490bdf9..fe60f5daabf4 100644 --- a/components/content_settings/core/browser/brave_content_settings_pref_provider.h +++ b/components/content_settings/core/browser/brave_content_settings_pref_provider.h @@ -58,7 +58,9 @@ class BravePrefProvider : public PrefProvider, TestShieldsSettingsMigrationFromResourceIDs); FRIEND_TEST_ALL_PREFIXES(BravePrefProviderTest, TestShieldsSettingsMigrationFromUnknownSettings); + FRIEND_TEST_ALL_PREFIXES(BravePrefProviderTest, EnsureNoWildcardEntries); void MigrateShieldsSettings(bool incognito); + void EnsureNoWildcardEntries(); void MigrateShieldsSettingsFromResourceIds(); void MigrateShieldsSettingsFromResourceIdsForOneType( const std::string& preference_path, diff --git a/components/content_settings/core/browser/brave_content_settings_pref_provider_unittest.cc b/components/content_settings/core/browser/brave_content_settings_pref_provider_unittest.cc index 94457f233927..9463cc109e17 100644 --- a/components/content_settings/core/browser/brave_content_settings_pref_provider_unittest.cc +++ b/components/content_settings/core/browser/brave_content_settings_pref_provider_unittest.cc @@ -13,6 +13,7 @@ #include "brave/components/content_settings/core/browser/brave_content_settings_pref_provider.h" #include "brave/components/content_settings/core/browser/brave_content_settings_utils.h" #include "chrome/test/base/testing_profile.h" +#include "components/content_settings/core/browser/content_settings_pref.h" #include "components/content_settings/core/browser/content_settings_registry.h" #include "components/content_settings/core/common/content_settings.h" #include "components/content_settings/core/common/content_settings_pattern.h" @@ -611,4 +612,25 @@ TEST_F(BravePrefProviderTest, TestShieldsSettingsMigrationV2toV3) { provider.ShutdownOnUIThread(); } +TEST_F(BravePrefProviderTest, EnsureNoWildcardEntries) { + BravePrefProvider provider( + testing_profile()->GetPrefs(), false /* incognito */, + true /* store_last_modified */, false /* restore_session */); + ShieldsEnabledSetting shields_enabled_settings(&provider); + GURL example_url("https://example.com"); + shields_enabled_settings.CheckSettingsAreDefault(example_url); + // Set wildcard entry + auto pattern = ContentSettingsPattern::Wildcard(); + provider.SetWebsiteSetting(pattern, pattern, + ContentSettingsType::BRAVE_SHIELDS, + base::Value(CONTENT_SETTING_ALLOW), {}); + // Verify global has changed + shields_enabled_settings.CheckSettingsWouldAllow(example_url); + // Remove wildcards + provider.EnsureNoWildcardEntries(); + // Verify global has reset + shields_enabled_settings.CheckSettingsAreDefault(example_url); + provider.ShutdownOnUIThread(); +} + } // namespace content_settings