Refactor transmission of security of source and http_only mods in cookie_monster.cc.

net/cookies/cookie_monster.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Portions of this code based on Mozilla:
 //   (netwerk/cookie/src/nsCookieService.cpp)
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
  * The contents of this file are subject to the Mozilla Public License Version
@@ skipping 747 matching lines @@
   this->cookie_monster()->ComputeCookieDiff(&old_cookies, &list_,
                                             &positive_diff, &negative_diff);
 
   for (CookieList::const_iterator it = negative_diff.begin();
        it != negative_diff.end(); ++it) {
     this->cookie_monster()->DeleteCanonicalCookie(*it);
   }
 
   bool result = true;
   if (positive_diff.size() > 0)
-    result = this->cookie_monster()->SetCanonicalCookies(list_);
+    result = this->cookie_monster()->SetAllCookies(list_);
 
   if (!callback_.is_null())
     callback_.Run(result);
 }
 
 // Task class for GetCookiesWithOptions call.
 class CookieMonster::GetCookiesWithOptionsTask : public CookieMonsterTask {
  public:
   GetCookiesWithOptionsTask(CookieMonster* cookie_monster,
                             const GURL& url,
@@ skipping 300 matching lines @@
   }
 
   // Validate consistency of passed arguments.
   if (ParsedCookie::ParseTokenString(name) != name ||
       ParsedCookie::ParseValueString(value) != value ||
       ParsedCookie::ParseValueString(domain) != domain ||
       ParsedCookie::ParseValueString(path) != path) {
     return false;
   }
 
-  // Validate passed arguments against URL.
-  if (secure && !url.SchemeIsCryptographic())
-    return false;
-
   std::string cookie_domain;
   if (!cookie_util::GetCookieDomainWithString(url, domain, &cookie_domain))
     return false;
 
   std::string cookie_path = CanonicalCookie::CanonPathWithString(url, path);
   if (!path.empty() && cookie_path != path)
     return false;
 
   // Canonicalize path again to make sure it escapes characters as needed.
   url::Component path_component(0, cookie_path.length());
   url::RawCanonOutputT<char> canon_path;
   url::Component canon_path_component;
   url::CanonicalizePath(cookie_path.data(), path_component, &canon_path,
                         &canon_path_component);
   cookie_path = std::string(canon_path.data() + canon_path_component.begin,
                             canon_path_component.len);
 
   std::unique_ptr<CanonicalCookie> cc(base::MakeUnique<CanonicalCookie>(
       name, value, cookie_domain, cookie_path, actual_creation_time,
       expiration_time, last_access_time, secure, http_only, same_site,
       priority));
 
-  CookieOptions options;
-  options.set_include_httponly();
-  options.set_same_site_cookie_mode(
-      CookieOptions::SameSiteCookieMode::INCLUDE_STRICT_AND_LAX);
-  return SetCanonicalCookie(std::move(cc), url, options);
+  return SetCanonicalCookie(std::move(cc), url.SchemeIsCryptographic(), true);
 }
 
 CookieList CookieMonster::GetAllCookies() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // This function is being called to scrape the cookie list for management UI
   // or similar.  We shouldn't show expired cookies in this list since it will
   // just be confusing to users, and this function is called rarely enough (and
   // is already slow enough) that it's OK to take the time to garbage collect
   // the expired cookies now.
@@ skipping 298 matching lines @@
   // Even if a key is expired, insert it so it can be garbage collected,
   // removed, and sync'd.
   CookieItVector cookies_with_control_chars;
 
   for (auto& cookie : cookies) {
     int64_t cookie_creation_time = cookie->CreationDate().ToInternalValue();
 
     if (creation_times_.insert(cookie_creation_time).second) {
       CanonicalCookie* cookie_ptr = cookie.get();
       CookieMap::iterator inserted = InternalInsertCookie(
-          GetKey(cookie_ptr->Domain()), std::move(cookie), GURL(), false);
+          GetKey(cookie_ptr->Domain()), std::move(cookie), false);
       const Time cookie_access_time(cookie_ptr->LastAccessDate());
       if (earliest_access_time_.is_null() ||
           cookie_access_time < earliest_access_time_)
         earliest_access_time_ = cookie_access_time;
 
       if (ContainsControlCharacter(cookie_ptr->Name()) ||
           ContainsControlCharacter(cookie_ptr->Value())) {
         cookies_with_control_chars.push_back(inserted);
       }
     } else {
@@ skipping 193 matching lines @@
     // time if we've been requested to do so.
     if (options.update_access_time()) {
       InternalUpdateCookieAccessTime(cc, current);
     }
     cookies->push_back(cc);
   }
 }
 
 bool CookieMonster::DeleteAnyEquivalentCookie(const std::string& key,
                                               const CanonicalCookie& ecc,
-                                              const GURL& source_url,
+                                              bool source_secure,
                                               bool skip_httponly,
                                               bool already_expired) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   bool found_equivalent_cookie = false;
   bool skipped_httponly = false;
   bool skipped_secure_cookie = false;
 
   histogram_cookie_delete_equivalent_->Add(COOKIE_DELETE_EQUIVALENT_ATTEMPT);
 
   for (CookieMapItPair its = cookies_.equal_range(key);
        its.first != its.second;) {
     CookieMap::iterator curit = its.first;
     CanonicalCookie* cc = curit->second.get();
     ++its.first;
 
     // 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-ietf-httpbis-cookie-alone
-    if (cc->IsSecure() && !source_url.SchemeIsCryptographic() &&
+    if (cc->IsSecure() && !source_secure &&
         ecc.IsEquivalentForSecureCookieMatching(*cc)) {
       skipped_secure_cookie = true;
       histogram_cookie_delete_equivalent_->Add(
           COOKIE_DELETE_EQUIVALENT_SKIPPING_SECURE);
       // 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;
 
@@ skipping 20 matching lines @@
       }
       found_equivalent_cookie = true;
     }
   }
   return skipped_httponly || skipped_secure_cookie;
 }
 
 CookieMonster::CookieMap::iterator CookieMonster::InternalInsertCookie(
     const std::string& key,
     std::unique_ptr<CanonicalCookie> cc,
-    const GURL& source_url,
     bool sync_to_store) {
   DCHECK(thread_checker_.CalledOnValidThread());
   CanonicalCookie* cc_ptr = cc.get();
 
   if ((cc_ptr->IsPersistent() || persist_session_cookies_) && store_.get() &&
       sync_to_store)
     store_->AddCookie(*cc_ptr);
   CookieMap::iterator inserted =
       cookies_.insert(CookieMap::value_type(key, std::move(cc)));
   if (delegate_.get()) {
     delegate_->OnCookieChanged(*cc_ptr, false,
                                CookieStore::ChangeCause::INSERTED);
   }
 
   // See InitializeHistograms() for details.
   int32_t type_sample = cc_ptr->SameSite() != CookieSameSite::NO_RESTRICTION
                             ? 1 << COOKIE_TYPE_SAME_SITE
                             : 0;
   type_sample |= cc_ptr->IsHttpOnly() ? 1 << COOKIE_TYPE_HTTPONLY : 0;
   type_sample |= cc_ptr->IsSecure() ? 1 << COOKIE_TYPE_SECURE : 0;
   histogram_cookie_type_->Add(type_sample);
 
-  // Histogram the type of scheme used on URLs that set cookies. This
-  // intentionally includes cookies that are set or overwritten by
-  // http:// URLs, but not cookies that are cleared by http:// URLs, to
-  // understand if the former behavior can be deprecated for Secure
-  // cookies.
-  if (!source_url.is_empty()) {
-    CookieSource cookie_source_sample;
-    if (source_url.SchemeIsCryptographic()) {
-      cookie_source_sample =
-          cc_ptr->IsSecure()
-              ? COOKIE_SOURCE_SECURE_COOKIE_CRYPTOGRAPHIC_SCHEME
-              : COOKIE_SOURCE_NONSECURE_COOKIE_CRYPTOGRAPHIC_SCHEME;
-    } else {
-      cookie_source_sample =
-          cc_ptr->IsSecure()
-              ? COOKIE_SOURCE_SECURE_COOKIE_NONCRYPTOGRAPHIC_SCHEME
-              : COOKIE_SOURCE_NONSECURE_COOKIE_NONCRYPTOGRAPHIC_SCHEME;
-    }
-    histogram_cookie_source_scheme_->Add(cookie_source_sample);
-  }
-
   RunCookieChangedCallbacks(*cc_ptr, CookieStore::ChangeCause::INSERTED);
 
   return inserted;
 }
 
 bool CookieMonster::SetCookieWithCreationTimeAndOptions(
     const GURL& url,
     const std::string& cookie_line,
     const Time& creation_time_or_null,
     const CookieOptions& options) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   VLOG(kVlogSetCookies) << "SetCookie() line: " << cookie_line;
 
   Time creation_time = creation_time_or_null;
   if (creation_time.is_null()) {
     creation_time = CurrentTime();
     last_time_seen_ = creation_time;
   }
 
   std::unique_ptr<CanonicalCookie> cc(
       CanonicalCookie::Create(url, cookie_line, creation_time, options));
 
   if (!cc.get()) {
     VLOG(kVlogSetCookies) << "WARNING: Failed to allocate CanonicalCookie";
     return false;
   }
-  return SetCanonicalCookie(std::move(cc), url, options);
+  return SetCanonicalCookie(std::move(cc), url.SchemeIsCryptographic(),
+                            !options.exclude_httponly());
 }
 
 bool CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc,
-                                       const GURL& source_url,
-                                       const CookieOptions& options) {
+                                       bool secure_source,
+                                       bool modify_http_only) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  if (cc->IsSecure() && !secure_source)
+    return false;
+
   Time creation_time = cc->CreationDate();
   const std::string key(GetKey(cc->Domain()));
   bool already_expired = cc->IsExpired(creation_time);
 
-  if (DeleteAnyEquivalentCookie(key, *cc, source_url,
-                                options.exclude_httponly(), already_expired)) {
+  if (DeleteAnyEquivalentCookie(key, *cc, secure_source, !modify_http_only,
+                                already_expired)) {
     std::string error;
     error =
         "SetCookie() not clobbering httponly cookie or secure cookie for "
         "insecure scheme";
 
     VLOG(kVlogSetCookies) << error;
     return false;
   }
 
   VLOG(kVlogSetCookies) << "SetCookie() key: " << key
                         << " cc: " << cc->DebugString();
 
   // Realize that we might be setting an expired cookie, and the only point
   // was to delete the cookie which we've already done.
   if (!already_expired) {
     // See InitializeHistograms() for details.
     if (cc->IsPersistent()) {
       histogram_expiration_duration_minutes_->Add(
           (cc->ExpiryDate() - creation_time).InMinutes());
     }
 
-    InternalInsertCookie(key, std::move(cc), source_url, true);
+    // Histogram the type of scheme used on URLs that set cookies. This
+    // intentionally includes cookies that are set or overwritten by
+    // http:// URLs, but not cookies that are cleared by http:// URLs, to
+    // understand if the former behavior can be deprecated for Secure
+    // cookies.
+    CookieSource cookie_source_sample =
+        (secure_source
+             ? (cc->IsSecure()
+                    ? COOKIE_SOURCE_SECURE_COOKIE_CRYPTOGRAPHIC_SCHEME
+                    : COOKIE_SOURCE_NONSECURE_COOKIE_CRYPTOGRAPHIC_SCHEME)
+             : (cc->IsSecure()
+                    ? COOKIE_SOURCE_SECURE_COOKIE_NONCRYPTOGRAPHIC_SCHEME
+                    : COOKIE_SOURCE_NONSECURE_COOKIE_NONCRYPTOGRAPHIC_SCHEME));
+    histogram_cookie_source_scheme_->Add(cookie_source_sample);
+
+    InternalInsertCookie(key, std::move(cc), true);
   } else {
     VLOG(kVlogSetCookies) << "SetCookie() not storing already expired cookie.";
   }
 
   // We assume that hopefully setting a cookie will be less common than
   // querying a cookie.  Since setting a cookie can put us over our limits,
   // 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);
 
   return true;
 }
 
-bool CookieMonster::SetCanonicalCookies(const CookieList& list) {
+bool CookieMonster::SetAllCookies(const CookieList& list) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  for (const auto& cookie : list) {
+    const std::string key(GetKey(cookie.Domain()));
+    Time creation_time = cookie.CreationDate();
+    bool already_expired = cookie.IsExpired(creation_time);
 
-  CookieOptions options;
-  options.set_include_httponly();
+    bool result =
+        DeleteAnyEquivalentCookie(key, cookie, false, false, already_expired);
+    DCHECK(!result);

[mmenke, 2017/06/16 16:19:48]

Why can we DCHECK on this?  The old method returned false when this happened
(Like when an input cookie was secure).

[Randy Smith (Not in Mondays), 2017/06/16 20:53:58]

Yes, for SetAllCookies I'm explicitly not keeping behavior as per the
conversation with droger@ (owner of only consumer) and the changes in the header
documentation comment.  But you're quite right that this is a bug--my intent had
been to give DeleteAnyEquivalentCookie full privileges and have it just run
rampant through the input cookie list, but that requires "true, false"
(source_secure true, skip_httponly false) and without that the DCHECK is
incorrect.  Done.

 
-  for (const auto& cookie : list) {
-    // Use an empty GURL.  This method does not support setting secure cookies.
-    if (!SetCanonicalCookie(base::MakeUnique<CanonicalCookie>(cookie), GURL(),
-                            options)) {
-      return false;
+    if (already_expired)
+      continue;
+
+    if (cookie.IsPersistent()) {
+      histogram_expiration_duration_minutes_->Add(
+          (cookie.ExpiryDate() - creation_time).InMinutes());
     }
+
+    InternalInsertCookie(key, base::MakeUnique<CanonicalCookie>(cookie), true);

[mmenke, 2017/06/16 16:19:48]

This doesn't run garbage collection.  The old method did.  Do we care?

[Randy Smith (Not in Mondays), 2017/06/16 20:53:58]

I consider it easier to add GC back in than to answer your philosophical
question :-} (but also :-|--I certainly don't care a *lot* about the behavior of
this method.)  I hadn't intended to change the behavior in removing it. 
 Done.

   }
 
+  // TODO(rdsmith): If this function always returns the same value, it
+  // shouldn't have a return value.  But it should also be deleted (see
+  // https://codereview.chromium.org/2882063002/#msg64), which would
+  // solve the return value problem.
   return true;
 }
 
 void CookieMonster::InternalUpdateCookieAccessTime(CanonicalCookie* cc,
                                                    const Time& current) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // Based off the Mozilla code.  When a cookie has been accessed recently,
   // don't bother updating its access time again.  This reduces the number of
   // updates we do during pageload, which in turn reduces the chance our storage
@@ skipping 581 matching lines @@
        it != hook_map_.end(); ++it) {
     std::pair<GURL, std::string> key = it->first;
     if (cookie.IncludeForRequestURL(key.first, opts) &&
         cookie.Name() == key.second) {
       it->second->Notify(cookie, cause);
     }
   }
 }
 
 }  // namespace net