Refactor CookieMonster::MaybeDeleteEquivalentCookieAndUpdateStatus

This change straightens out some of the convoluted logic in this method,
so that hopefully it is simpler and easier to understand.

Bug: None
Change-Id: I913e98c04c3f9f3a8c6727a25c6c0f4675695c77
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1877664
Commit-Queue: Lily Chen <chlily@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#710884}

net/cookies/cookie_monster.cc

@@ -1012,109 +1012,100 @@ void CookieMonster::FilterCookiesWithOptions(
 
 void CookieMonster::MaybeDeleteEquivalentCookieAndUpdateStatus(
     const std::string& key,
-    const CanonicalCookie& ecc,
+    const CanonicalCookie& cookie_being_set,
     bool source_secure,
     bool skip_httponly,
     bool already_expired,
     base::Time* creation_date_to_inherit,
     CanonicalCookie::CookieInclusionStatus* status) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!status->HasExclusionReason(
+      CanonicalCookie::CookieInclusionStatus::EXCLUDE_OVERWRITE_SECURE));
+  DCHECK(!status->HasExclusionReason(
+      CanonicalCookie::CookieInclusionStatus::EXCLUDE_OVERWRITE_HTTP_ONLY));
 
   bool found_equivalent_cookie = false;
-  bool skipped_httponly = false;
-  bool skipped_secure_cookie = false;
-
-  CookieMap::iterator cookie_it_to_possibly_delete = cookies_.end();
+  CookieMap::iterator maybe_delete_it = cookies_.end();
   CanonicalCookie* cc_skipped_secure = nullptr;
-  for (CookieMapItPair its = cookies_.equal_range(key);
-       its.first != its.second;) {
-    auto curit = its.first;
-    CanonicalCookie* cc = curit->second.get();
-    ++its.first;
 
+  // Check every cookie matching this domain key for equivalence.
+  CookieMapItPair range_its = cookies_.equal_range(key);
+  for (auto cur_it = range_its.first; cur_it != range_its.second; ++cur_it) {
+    CanonicalCookie* cc = cur_it->second.get();
+
+    // Evaluate "Leave Secure Cookies Alone":
     // If the cookie is being set from an insecure scheme, then if a cookie
     // already exists with the same name and it is Secure, then the cookie
     // should *not* be updated if they domain-match and ignoring the path
-    // attribute.
+    // attribute. This notion of equivalence is slightly more inclusive than the
+    // usual IsEquivalent() check.
     //
     // See: https://tools.ietf.org/html/draft-ietf-httpbis-cookie-alone
     if (cc->IsSecure() && !source_secure &&
-        ecc.IsEquivalentForSecureCookieMatching(*cc)) {
-      skipped_secure_cookie = true;
+        cookie_being_set.IsEquivalentForSecureCookieMatching(*cc)) {
+      // Hold onto this for additional Netlogging later if we end up preserving
+      // a would-have-been-deleted cookie because of this.
       cc_skipped_secure = cc;
       net_log_.AddEvent(NetLogEventType::COOKIE_STORE_COOKIE_REJECTED_SECURE,
                         [&](NetLogCaptureMode capture_mode) {
                           return NetLogCookieMonsterCookieRejectedSecure(
-                              cc, &ecc, capture_mode);
+                              cc_skipped_secure, &cookie_being_set,
+                              capture_mode);
                         });
-      // If the cookie is equivalent to the new cookie and wouldn't have been
-      // skipped for being HTTP-only, record that it is a skipped secure cookie
-      // that would have been deleted otherwise.
-      if (ecc.IsEquivalent(*cc)) {
-        found_equivalent_cookie = true;
-
-        // Would also have skipped for being httponly, so make a note of that.
-        if (skip_httponly && cc->IsHttpOnly())
-          skipped_httponly = true;
-      }
-    } else if (ecc.IsEquivalent(*cc)) {
+      status->AddExclusionReason(
+          CanonicalCookie::CookieInclusionStatus::EXCLUDE_OVERWRITE_SECURE);
+    }
+
+    if (cookie_being_set.IsEquivalent(*cc)) {
       // We should never have more than one equivalent cookie, since they should
-      // overwrite each other, unless secure cookies require secure scheme is
-      // being enforced. In that case, cookies with different paths might exist
-      // and be considered equivalent.
+      // overwrite each other.
       CHECK(!found_equivalent_cookie)
           << "Duplicate equivalent cookies found, cookie store is corrupted.";
-      DCHECK(cookie_it_to_possibly_delete == cookies_.end());
+      DCHECK(maybe_delete_it == cookies_.end());
+      found_equivalent_cookie = true;
+
+      // The |cookie_being_set| is rejected for trying to overwrite an httponly
+      // cookie when it should not be able to.
       if (skip_httponly && cc->IsHttpOnly()) {
-        skipped_httponly = true;
         net_log_.AddEvent(
             NetLogEventType::COOKIE_STORE_COOKIE_REJECTED_HTTPONLY,
             [&](NetLogCaptureMode capture_mode) {
-              return NetLogCookieMonsterCookieRejectedHttponly(cc, &ecc,
-                                                               capture_mode);
+              return NetLogCookieMonsterCookieRejectedHttponly(
+                  cc, &cookie_being_set, capture_mode);
             });
+        status->AddExclusionReason(CanonicalCookie::CookieInclusionStatus::
+                                       EXCLUDE_OVERWRITE_HTTP_ONLY);
       } else {
-        cookie_it_to_possibly_delete = curit;
+        maybe_delete_it = cur_it;
       }
-      found_equivalent_cookie = true;
     }
   }
 
-  if (cookie_it_to_possibly_delete != cookies_.end()) {
-    CanonicalCookie* cc_to_possibly_delete =
-        cookie_it_to_possibly_delete->second.get();
-    // 1) If a secure cookie was encountered (and left alone), don't actually
-    // modify any of the pre-existing cookies. Only delete if no secure cookies
-    // were skipped. 2) Only delete if the status of the current cookie-addition
-    // is "include", so that we don't throw out a valid cookie for a bad cookie.
-    if (!skipped_secure_cookie && status->IsInclude()) {
-      if (cc_to_possibly_delete->Value() == ecc.Value()) {
-        *creation_date_to_inherit = cc_to_possibly_delete->CreationDate();
-      }
-      InternalDeleteCookie(cookie_it_to_possibly_delete, true,
+  if (maybe_delete_it != cookies_.end()) {
+    CanonicalCookie* maybe_delete_cc = maybe_delete_it->second.get();
+    if (status->IsInclude()) {
+      if (maybe_delete_cc->Value() == cookie_being_set.Value())
+        *creation_date_to_inherit = maybe_delete_cc->CreationDate();
+      InternalDeleteCookie(maybe_delete_it, true,
                            already_expired ? DELETE_COOKIE_EXPIRED_OVERWRITE
                                            : DELETE_COOKIE_OVERWRITE);
-    } else if (skipped_secure_cookie) {
-      // If any secure cookie was skipped, preserve the pre-existing cookie.
+    } else if (status->HasExclusionReason(
+                   CanonicalCookie::CookieInclusionStatus::
+                       EXCLUDE_OVERWRITE_SECURE)) {
+      // Log that we preserved a cookie that would have been deleted due to
+      // Leave Secure Cookies Alone. This arbitrarily only logs the last
+      // |cc_skipped_secure| that we were left with after the for loop, even if
+      // there were multiple matching Secure cookies that were left alone.
       DCHECK(cc_skipped_secure);
       net_log_.AddEvent(
           NetLogEventType::COOKIE_STORE_COOKIE_PRESERVED_SKIPPED_SECURE,
           [&](NetLogCaptureMode capture_mode) {
             return NetLogCookieMonsterCookiePreservedSkippedSecure(
-                cc_skipped_secure, cc_to_possibly_delete, &ecc, capture_mode);
+                cc_skipped_secure, maybe_delete_cc, &cookie_being_set,
+                capture_mode);
           });
     }
   }
-
-  if (skipped_httponly) {
-    status->AddExclusionReason(
-        CanonicalCookie::CookieInclusionStatus::EXCLUDE_OVERWRITE_HTTP_ONLY);
-  }
-
-  if (skipped_secure_cookie) {
-    status->AddExclusionReason(
-        CanonicalCookie::CookieInclusionStatus::EXCLUDE_OVERWRITE_SECURE);
-  }
 }
 
 CookieMonster::CookieMap::iterator CookieMonster::InternalInsertCookie(

net/cookies/cookie_monster.h

@@ -397,18 +397,24 @@ class NET_EXPORT CookieMonster : public CookieStore {
                                 CookieStatusList* included_cookies,
                                 CookieStatusList* excluded_cookies);
 
-  // Delete any cookies that are equivalent to |ecc| (same path, domain, etc).
+  // Possibly delete an existing cookie equivalent to |cookie_being_set| (same
+  // path, domain, and name).
+  //
   // |source_secure| indicates if the source may override existing secure
-  // cookies.
+  // cookies. If the source is not secure, and there is an existing "equivalent"
+  // cookie that is Secure, that cookie will be preserved, under "Leave Secure
+  // Cookies Alone" (see
+  // https://tools.ietf.org/html/draft-ietf-httpbis-cookie-alone-01).
+  // ("equivalent" here is in quotes because the equivalency check for the
+  // purposes of preserving existing Secure cookies is slightly more inclusive.)
   //
-  // If |skip_httponly| is true, httponly cookies will not be deleted.  The
-  // return value will be true if |skip_httponly| skipped an httponly cookie or
-  // the cookie to delete was Secure and the scheme of |ecc| is insecure.  |key|
-  // is the key to find the cookie in cookies_; see the comment before the
+  // If |skip_httponly| is true, httponly cookies will not be deleted even if
+  // they are equivalent.
+  // |key| is the key to find the cookie in cookies_; see the comment before the
   // CookieMap typedef for details.
   //
-  // If a cookie is deleted, and its value matches |ecc|'s value, then
-  // |creation_date_to_inherit| will be set to that cookie's creation date.
+  // If a cookie is deleted, and its value matches |cookie_being_set|'s value,
+  // then |creation_date_to_inherit| will be set to that cookie's creation date.
   //
   // The cookie will not be deleted if |*status| is not "include" when calling
   // the function. The function will update |*status| with exclusion reasons if
@@ -417,7 +423,7 @@ class NET_EXPORT CookieMonster : public CookieStore {
   // NOTE: There should never be more than a single matching equivalent cookie.
   void MaybeDeleteEquivalentCookieAndUpdateStatus(
       const std::string& key,
-      const CanonicalCookie& ecc,
+      const CanonicalCookie& cookie_being_set,
       bool source_secure,
       bool skip_httponly,
       bool already_expired,

net/cookies/cookie_monster_unittest.cc

@@ -2746,6 +2746,105 @@ TEST_F(CookieMonsterTest, CookieSourceHistogram) {
       CookieMonster::COOKIE_SOURCE_NONSECURE_COOKIE_NONCRYPTOGRAPHIC_SCHEME, 1);
 }
 
+TEST_F(CookieMonsterTest, MaybeDeleteEquivalentCookieAndUpdateStatus) {
+  scoped_refptr<MockPersistentCookieStore> store(new MockPersistentCookieStore);
+  std::unique_ptr<CookieMonster> cm(new CookieMonster(store.get(), &net_log_));
+
+  // Set a secure, httponly cookie from a secure origin
+  auto preexisting_cookie = CanonicalCookie::Create(
+      https_www_foo_.url(), "A=B;Secure;HttpOnly", base::Time::Now(),
+      base::nullopt /* server_time */);
+  CanonicalCookie::CookieInclusionStatus status =
+      SetCanonicalCookieReturnStatus(cm.get(), std::move(preexisting_cookie),
+                                     "https", true /* can_modify_httponly */);
+  ASSERT_TRUE(status.IsInclude());
+
+  // Set a new cookie with a different name. Should work because cookies with
+  // different names are not considered equivalent nor "equivalent for secure
+  // cookie matching".
+  // Same origin:
+  EXPECT_TRUE(SetCookie(cm.get(), https_www_foo_.url(), "B=A;"));
+  // Different scheme, same domain:
+  EXPECT_TRUE(SetCookie(cm.get(), http_www_foo_.url(), "C=A;"));
+
+  // Set a non-Secure cookie from an insecure origin that is
+  // equivalent to the pre-existing Secure cookie.
+  auto bad_cookie =
+      CanonicalCookie::Create(http_www_foo_.url(), "A=D", base::Time::Now(),
+                              base::nullopt /* server_time */);
+  // Allow modifying HttpOnly, so that we don't skip preexisting cookies for
+  // being HttpOnly.
+  status = SetCanonicalCookieReturnStatus(
+      cm.get(), std::move(bad_cookie), "http", true /* can_modify_httponly */);
+  EXPECT_TRUE(status.HasExactlyExclusionReasonsForTesting(
+      {CanonicalCookie::CookieInclusionStatus::EXCLUDE_OVERWRITE_SECURE}));
+  // The preexisting cookie should still be there.
+  EXPECT_THAT(GetCookiesWithOptions(cm.get(), https_www_foo_.url(),
+                                    CookieOptions::MakeAllInclusive()),
+              ::testing::HasSubstr("A=B"));
+
+  auto entries = net_log_.GetEntries();
+  size_t skipped_secure_netlog_index = ExpectLogContainsSomewhere(
+      entries, 0, NetLogEventType::COOKIE_STORE_COOKIE_REJECTED_SECURE,
+      NetLogEventPhase::NONE);
+  EXPECT_FALSE(LogContainsEntryWithTypeAfter(
+      entries, 0, NetLogEventType::COOKIE_STORE_COOKIE_REJECTED_HTTPONLY));
+  ExpectLogContainsSomewhereAfter(
+      entries, skipped_secure_netlog_index,
+      NetLogEventType::COOKIE_STORE_COOKIE_PRESERVED_SKIPPED_SECURE,
+      NetLogEventPhase::NONE);
+
+  net_log_.Clear();
+
+  // Set a non-secure cookie from an insecure origin that matches the name of an
+  // already existing cookie but is not equivalent. This should fail since it's
+  // trying to shadow a secure cookie.
+  bad_cookie = CanonicalCookie::Create(
+      http_www_foo_.url(), "A=E; path=/some/path", base::Time::Now(),
+      base::nullopt /* server_time */);
+  // Allow modifying HttpOnly, so that we don't skip preexisting cookies for
+  // being HttpOnly.
+  status = SetCanonicalCookieReturnStatus(
+      cm.get(), std::move(bad_cookie), "http", true /* can_modify_httponly */);
+  EXPECT_TRUE(status.HasExactlyExclusionReasonsForTesting(
+      {CanonicalCookie::CookieInclusionStatus::EXCLUDE_OVERWRITE_SECURE}));
+  // The preexisting cookie should still be there.
+  EXPECT_THAT(GetCookiesWithOptions(cm.get(), https_www_foo_.url(),
+                                    CookieOptions::MakeAllInclusive()),
+              ::testing::HasSubstr("A=B"));
+
+  entries = net_log_.GetEntries();
+  skipped_secure_netlog_index = ExpectLogContainsSomewhere(
+      entries, 0, NetLogEventType::COOKIE_STORE_COOKIE_REJECTED_SECURE,
+      NetLogEventPhase::NONE);
+  EXPECT_FALSE(LogContainsEntryWithTypeAfter(
+      entries, 0, NetLogEventType::COOKIE_STORE_COOKIE_REJECTED_HTTPONLY));
+  // There wasn't actually a strictly equivalent cookie that we would have
+  // deleted.
+  EXPECT_FALSE(LogContainsEntryWithTypeAfter(
+      entries, skipped_secure_netlog_index,
+      NetLogEventType::COOKIE_STORE_COOKIE_PRESERVED_SKIPPED_SECURE));
+
+  net_log_.Clear();
+
+  // Test skipping equivalent cookie for HttpOnly only.
+  bad_cookie = CanonicalCookie::Create(https_www_foo_.url(), "A=E; Secure",
+                                       base::Time::Now(),
+                                       base::nullopt /* server_time */);
+  status =
+      SetCanonicalCookieReturnStatus(cm.get(), std::move(bad_cookie), "https",
+                                     false /* can_modify_httponly */);
+  EXPECT_TRUE(status.HasExactlyExclusionReasonsForTesting(
+      {CanonicalCookie::CookieInclusionStatus::EXCLUDE_OVERWRITE_HTTP_ONLY}));
+
+  entries = net_log_.GetEntries();
+  ExpectLogContainsSomewhere(
+      entries, 0, NetLogEventType::COOKIE_STORE_COOKIE_REJECTED_HTTPONLY,
+      NetLogEventPhase::NONE);
+  EXPECT_FALSE(LogContainsEntryWithTypeAfter(
+      entries, 0, NetLogEventType::COOKIE_STORE_COOKIE_REJECTED_SECURE));
+}
+
 // Test skipping a cookie in MaybeDeleteEquivalentCookieAndUpdateStatus for
 // multiple reasons (Secure and HttpOnly).
 TEST_F(CookieMonsterTest, SkipDontOverwriteForMultipleReasons) {
@@ -2771,6 +2870,14 @@ TEST_F(CookieMonsterTest, SkipDontOverwriteForMultipleReasons) {
   EXPECT_TRUE(status.HasExactlyExclusionReasonsForTesting(
       {CanonicalCookie::CookieInclusionStatus::EXCLUDE_OVERWRITE_SECURE,
        CanonicalCookie::CookieInclusionStatus::EXCLUDE_OVERWRITE_HTTP_ONLY}));
+
+  auto entries = net_log_.GetEntries();
+  ExpectLogContainsSomewhere(
+      entries, 0, NetLogEventType::COOKIE_STORE_COOKIE_REJECTED_SECURE,
+      NetLogEventPhase::NONE);
+  ExpectLogContainsSomewhere(
+      entries, 0, NetLogEventType::COOKIE_STORE_COOKIE_REJECTED_HTTPONLY,
+      NetLogEventPhase::NONE);
 }
 
 // Test that when we check for equivalent cookies, we don't remove any if the