Implements strict secure cookies as the default behavior in //net

net/cookies/cookie_monster.cc

Index: net/cookies/cookie_monster.cc
diff --git a/net/cookies/cookie_monster.cc b/net/cookies/cookie_monster.cc
index b656d83e6c078a0594dfd3c0f76d87ca0ea78488..ad56044a057031fe6483002bd272e80629272963 100644
--- a/net/cookies/cookie_monster.cc
+++ b/net/cookies/cookie_monster.cc
@@ -406,7 +406,6 @@ class CookieMonster::SetCookieWithDetailsTask : public CookieMonsterTask {
                            bool secure,
                            bool http_only,
                            CookieSameSite same_site,
-                           bool enforce_strict_secure,
                            CookiePriority priority,
                            const SetCookiesCallback& callback)
       : CookieMonsterTask(cookie_monster),
@@ -421,7 +420,6 @@ class CookieMonster::SetCookieWithDetailsTask : public CookieMonsterTask {
         secure_(secure),
         http_only_(http_only),
         same_site_(same_site),
-        enforce_strict_secure_(enforce_strict_secure),
         priority_(priority),
         callback_(callback) {}
 
@@ -443,7 +441,6 @@ class CookieMonster::SetCookieWithDetailsTask : public CookieMonsterTask {
   bool secure_;
   bool http_only_;
   CookieSameSite same_site_;
-  bool enforce_strict_secure_;
   CookiePriority priority_;
   SetCookiesCallback callback_;
 
@@ -453,8 +450,7 @@ class CookieMonster::SetCookieWithDetailsTask : public CookieMonsterTask {
 void CookieMonster::SetCookieWithDetailsTask::Run() {
   bool success = this->cookie_monster()->SetCookieWithDetails(
       url_, name_, value_, domain_, path_, creation_time_, expiration_time_,
-      last_access_time_, secure_, http_only_, same_site_,
-      enforce_strict_secure_, priority_);
+      last_access_time_, secure_, http_only_, same_site_, priority_);
   if (!callback_.is_null())
     callback_.Run(success);
 }
@@ -847,13 +843,11 @@ void CookieMonster::SetCookieWithDetailsAsync(
     bool secure,
     bool http_only,
     CookieSameSite same_site,
-    bool enforce_strict_secure,
     CookiePriority priority,
     const SetCookiesCallback& callback) {
   scoped_refptr<SetCookieWithDetailsTask> task = new SetCookieWithDetailsTask(
       this, url, name, value, domain, path, creation_time, expiration_time,
-      last_access_time, secure, http_only, same_site, enforce_strict_secure,
-      priority, callback);
+      last_access_time, secure, http_only, same_site, priority, callback);
   DoCookieTaskForURL(task, url);
 }
 
@@ -1040,7 +1034,6 @@ bool CookieMonster::SetCookieWithDetails(const GURL& url,
                                          bool secure,
                                          bool http_only,
                                          CookieSameSite same_site,
-                                         bool enforce_strict_secure,
                                          CookiePriority priority) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
@@ -1059,7 +1052,7 @@ bool CookieMonster::SetCookieWithDetails(const GURL& url,
 
   std::unique_ptr<CanonicalCookie> cc(CanonicalCookie::Create(
       url, name, value, domain, path, actual_creation_time, expiration_time,
-      secure, http_only, same_site, enforce_strict_secure, priority));
+      secure, http_only, same_site, priority));
 
   if (!cc.get())
     return false;
@@ -1071,8 +1064,6 @@ bool CookieMonster::SetCookieWithDetails(const GURL& url,
   options.set_include_httponly();
   options.set_same_site_cookie_mode(
       CookieOptions::SameSiteCookieMode::INCLUDE_STRICT_AND_LAX);
-  if (enforce_strict_secure)
-    options.set_enforce_strict_secure();
   return SetCanonicalCookie(std::move(cc), url, options);
 }
 
@@ -1608,8 +1599,7 @@ bool CookieMonster::DeleteAnyEquivalentCookie(const std::string& key,
                                               const CanonicalCookie& ecc,
                                               const GURL& source_url,
                                               bool skip_httponly,
-                                              bool already_expired,
-                                              bool enforce_strict_secure) {
+                                              bool already_expired) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   bool found_equivalent_cookie = false;
@@ -1624,15 +1614,13 @@ bool CookieMonster::DeleteAnyEquivalentCookie(const std::string& key,
     CanonicalCookie* cc = curit->second.get();
     ++its.first;
 
-    // If strict secure cookies is being enforced, then the equivalency
-    // requirements are looser. 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.
+    // 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.
     //
     // See: https://tools.ietf.org/html/draft-west-leave-secure-cookies-alone

[Mike West, 2017/01/14 06:24:01]

Nit: We should update these references to
https://tools.ietf.org/html/draft-ietf-httpbis-cookie-alone.

[jww, 2017/01/24 18:09:25]

Done.

-    if (enforce_strict_secure && cc->IsSecure() &&
-        !source_url.SchemeIsCryptographic() &&
+    if (cc->IsSecure() && !source_url.SchemeIsCryptographic() &&
         ecc.IsEquivalentForSecureCookieMatching(*cc)) {
       skipped_secure_cookie = true;
       histogram_cookie_delete_equivalent_->Add(
@@ -1757,16 +1745,11 @@ bool CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc,
   bool already_expired = cc->IsExpired(creation_time);
 
   if (DeleteAnyEquivalentCookie(key, *cc, source_url,
-                                options.exclude_httponly(), already_expired,
-                                options.enforce_strict_secure())) {
+                                options.exclude_httponly(), already_expired)) {
     std::string error;
-    if (options.enforce_strict_secure()) {
-      error =
-          "SetCookie() not clobbering httponly cookie or secure cookie for "
-          "insecure scheme";
-    } else {
-      error = "SetCookie() not clobbering httponly cookie";
-    }
+    error =
+        "SetCookie() not clobbering httponly cookie or secure cookie for "
+        "insecure scheme";
 
     VLOG(kVlogSetCookies) << error;
     return false;
@@ -1794,7 +1777,7 @@ bool CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc,
   // make sure that we garbage collect...  We can also make the assumption that
   // if a cookie was set, in the common case it will be used soon after,
   // and we will purge the expired cookies in GetCookies().
-  GarbageCollect(creation_time, key, options.enforce_strict_secure());
+  GarbageCollect(creation_time, key);
 
   return true;
 }
@@ -1867,8 +1850,7 @@ void CookieMonster::InternalDeleteCookie(CookieMap::iterator it,
 // Domain expiry behavior is unchanged by key/expiry scheme (the
 // meaning of the key is different, but that's not visible to this routine).
 size_t CookieMonster::GarbageCollect(const Time& current,
-                                     const std::string& key,
-                                     bool enforce_strict_secure) {
+                                     const std::string& key) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   size_t num_deleted = 0;
@@ -1926,11 +1908,6 @@ size_t CookieMonster::GarbageCollect(const Time& current,
 
       size_t quota = 0;
       for (const auto& purge_round : purge_rounds) {
-        // Only observe the non-secure purge rounds if strict secure cookies is
-        // enabled.
-        if (!enforce_strict_secure && purge_round.protect_secure_cookies)
-          continue;
-
         // Adjust quota according to the priority of cookies. Each round should
         // protect certain number of cookies in order to avoid starvation.
         // For example, when each round starts to remove cookies, the number of
@@ -1983,27 +1960,22 @@ size_t CookieMonster::GarbageCollect(const Time& current,
       size_t purge_goal = cookie_its.size() - (kMaxCookies - kPurgeCookies);
       DCHECK(purge_goal > kPurgeCookies);
 
-      if (enforce_strict_secure) {
-        CookieItVector secure_cookie_its;
-        CookieItVector non_secure_cookie_its;
-        SplitCookieVectorIntoSecureAndNonSecure(cookie_its, &secure_cookie_its,
-                                                &non_secure_cookie_its);
-        size_t non_secure_purge_goal =
-            std::min<size_t>(purge_goal, non_secure_cookie_its.size() - 1);
-
-        size_t just_deleted = GarbageCollectLeastRecentlyAccessed(
-            current, safe_date, non_secure_purge_goal, non_secure_cookie_its);
-        num_deleted += just_deleted;
-
-        if (just_deleted < purge_goal) {
-          size_t secure_purge_goal = std::min<size_t>(
-              purge_goal - just_deleted, secure_cookie_its.size() - 1);
-          num_deleted += GarbageCollectLeastRecentlyAccessed(
-              current, safe_date, secure_purge_goal, secure_cookie_its);
-        }
-      } else {
+      CookieItVector secure_cookie_its;
+      CookieItVector non_secure_cookie_its;
+      SplitCookieVectorIntoSecureAndNonSecure(cookie_its, &secure_cookie_its,
+                                              &non_secure_cookie_its);
+      size_t non_secure_purge_goal =
+          std::min<size_t>(purge_goal, non_secure_cookie_its.size() - 1);
+
+      size_t just_deleted = GarbageCollectLeastRecentlyAccessed(
+          current, safe_date, non_secure_purge_goal, non_secure_cookie_its);
+      num_deleted += just_deleted;
+
+      if (just_deleted < purge_goal && secure_cookie_its.size() > 0) {
+        size_t secure_purge_goal = std::min<size_t>(
+            purge_goal - just_deleted, secure_cookie_its.size() - 1);
         num_deleted += GarbageCollectLeastRecentlyAccessed(
-            current, safe_date, purge_goal, cookie_its);
+            current, safe_date, secure_purge_goal, secure_cookie_its);

[Mike West, 2017/01/14 06:24:01]

Hrm. I don't think we ever updated
https://tools.ietf.org/html/draft-west-cookie-priority-00 with the implications
for `secure`. I'm not sure it's worth your time to tackle that in the last two
weeks you're here, but maybe you could sketch out a summary in a doc somewhere
so I have something to work from? :)

[jww, 2017/01/24 18:09:25]

Shared a doc with you:
https://docs.google.com/document/d/1fI_vpJ_mF5K_52c5o39wfyVCuPStrswc7E2bFB_o0...

       }
     }
   }