Issue 6464: Do not forget about shields while calculating cookie rules.

Fix brave/brave-browser#6464
brave · Oct 14, 2019 · 4060967 · 4060967
1 parent 442f247
commit 4060967
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 10 deletions.
diff --git a/browser/net/brave_network_delegate_browsertest.cc b/browser/net/brave_network_delegate_browsertest.cc
@@ -53,6 +53,12 @@ class BraveNetworkDelegateBrowserTest : public InProcessBrowserTest {
                                         top_level_page_url_);
   }
 
+  void ShieldsDown() {
+    brave_shields::SetBraveShieldsEnabled(browser()->profile(),
+                                          false,
+                                          top_level_page_url_);
+  }
+
  protected:
   GURL url_;
   GURL nested_iframe_script_url_;
@@ -81,6 +87,14 @@ IN_PROC_BROWSER_TEST_F(BraveNetworkDelegateBrowserTest, Iframe3PCookieAllowed) {
   EXPECT_FALSE(cookie.empty());
 }
 
+IN_PROC_BROWSER_TEST_F(BraveNetworkDelegateBrowserTest, Iframe3PShieldsDown) {
+  ShieldsDown();
+  ui_test_utils::NavigateToURL(browser(), url_);
+  const std::string cookie =
+      content::GetCookies(browser()->profile(), GURL("http://c.com/"));
+  EXPECT_FALSE(cookie.empty());
+}
+
 // Fetching not just a frame, but some other resource.
 IN_PROC_BROWSER_TEST_F(BraveNetworkDelegateBrowserTest,
                        IframeJs3PCookieBlocked) {

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
@@ -63,8 +63,7 @@ class BraveShieldsRuleIterator : public RuleIterator {
 
 
 bool IsActive(const Rule& cookie_rule,
-              const std::vector<Rule>& shield_rules,
-              bool* shields_down_for_site) {
+              const std::vector<Rule>& shield_rules) {
   // don't include default rules in the iterator
   if (cookie_rule.primary_pattern == ContentSettingsPattern::Wildcard() &&
       (cookie_rule.secondary_pattern == ContentSettingsPattern::Wildcard() ||
@@ -86,8 +85,6 @@ bool IsActive(const Rule& cookie_rule,
       if (primary_compare == ContentSettingsPattern::IDENTITY ||
           primary_compare == ContentSettingsPattern::SUCCESSOR) {
         // TODO(bridiver) - move this logic into shields_util for allow/block
-        *shields_down_for_site =
-            ValueToContentSetting(&shield_rule.value) == CONTENT_SETTING_BLOCK;
         return
             ValueToContentSetting(&shield_rule.value) != CONTENT_SETTING_BLOCK;
       }
@@ -224,6 +221,7 @@ void BravePrefProvider::UpdateCookieRules(ContentSettingsType content_type,
   while (brave_shields_iterator && brave_shields_iterator->HasNext()) {
     shield_rules.push_back(CloneRule(brave_shields_iterator->Next()));
   }
+
   brave_shields_iterator.reset();
 
   // add brave cookies after checking shield status
@@ -234,20 +232,30 @@ void BravePrefProvider::UpdateCookieRules(ContentSettingsType content_type,
 
   auto old_rules = std::move(brave_cookie_rules_[incognito]);
 
+  // Matching cookie rules against shield rules.
   while (brave_cookies_iterator && brave_cookies_iterator->HasNext()) {
     auto rule = brave_cookies_iterator->Next();
-    bool shields_down_for_site = false;
-    if (IsActive(rule, shield_rules, &shields_down_for_site)) {
+    if (IsActive(rule, shield_rules)) {
       rules.push_back(CloneRule(rule, true));
       brave_cookie_rules_[incognito].push_back(CloneRule(rule, true));
-    } else if (shields_down_for_site) {
+    }
+  }
+
+  // Adding shields down rules (they always override cookie rules).
+  for (const auto& shield_rule : shield_rules) {
+    // Skip the default rule.
+    if (shield_rule.primary_pattern.MatchesAllHosts()) {
+      continue;
+    }
+    // Shields down.
+    if (ValueToContentSetting(&shield_rule.value) == CONTENT_SETTING_BLOCK) {
       rules.push_back(
           Rule(ContentSettingsPattern::Wildcard(),
-               rule.primary_pattern,
+               shield_rule.primary_pattern,
                ContentSettingToValue(CONTENT_SETTING_ALLOW)->Clone()));
       brave_cookie_rules_[incognito].push_back(
           Rule(ContentSettingsPattern::Wildcard(),
-               rule.primary_pattern,
+               shield_rule.primary_pattern,
                ContentSettingToValue(CONTENT_SETTING_ALLOW)->Clone()));
     }
   }

diff --git a/renderer/brave_content_settings_observer_browsertest.cc b/renderer/brave_content_settings_observer_browsertest.cc
@@ -654,7 +654,7 @@ IN_PROC_BROWSER_TEST_F(BraveContentSettingsObserverBrowserTest,
 }
 
 IN_PROC_BROWSER_TEST_F(BraveContentSettingsObserverBrowserTest,
-    ShieldsDownAllowsCookies) {
+    ShieldsDownAllowBlockedCookies) {
   BlockCookies();
   ShieldsDown();
   NavigateToPageWithIframe();
@@ -665,6 +665,17 @@ IN_PROC_BROWSER_TEST_F(BraveContentSettingsObserverBrowserTest,
                COOKIE_STR);
 }
 
+IN_PROC_BROWSER_TEST_F(BraveContentSettingsObserverBrowserTest,
+    ShieldsDownAllowsCookies) {
+  ShieldsDown();
+  NavigateToPageWithIframe();
+  EXPECT_STREQ(ExecScriptGetStr(kCookieScript, contents()).c_str(), COOKIE_STR);
+  ASSERT_TRUE(NavigateIframeToURL(contents(), kIframeID, iframe_url()));
+  ASSERT_EQ(child_frame()->GetLastCommittedURL(), iframe_url());
+  EXPECT_STREQ(ExecScriptGetStr(kCookieScript, child_frame()).c_str(),
+               COOKIE_STR);
+}
+
 IN_PROC_BROWSER_TEST_F(BraveContentSettingsObserverBrowserTest,
     ShieldsUpBlockCookies) {
   BlockCookies();