Add a SetCanonicalCookie method for CookieMonster.

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 f2bd3e4558249f5b0651df1e7d3ab4fe537bd882..2f7da0619af82338fdc8ec49b42ae9e3c008bbe5 100644
--- a/net/cookies/cookie_monster.cc
+++ b/net/cookies/cookie_monster.cc
@@ -62,6 +62,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/time.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/cookies/canonical_cookie.h"
 #include "net/cookies/cookie_util.h"
@@ -678,6 +679,42 @@ int CookieMonster::DeleteCanonicalCookieTask::RunDeleteTask() {
   return this->cookie_monster()->DeleteCanonicalCookie(cookie_);
 }
 
+// Task class for SetCanonicalCookie call.
+class CookieMonster::SetCanonicalCookieTask : public CookieMonsterTask {
+ public:
+  SetCanonicalCookieTask(CookieMonster* cookie_monster,
+                         std::unique_ptr<CanonicalCookie> cookie,
+                         bool secure_source,
+                         bool modify_http_only,
+                         const SetCookiesCallback& callback)
+      : CookieMonsterTask(cookie_monster),
+        cookie_(std::move(cookie)),
+        secure_source_(secure_source),
+        modify_http_only_(modify_http_only),
+        callback_(callback) {}
+
+  // CookieMonsterTask:
+  void Run() override;
+
+ protected:
+  ~SetCanonicalCookieTask() override {}
+
+ private:
+  std::unique_ptr<CanonicalCookie> cookie_;
+  bool secure_source_;
+  bool modify_http_only_;
+  SetCookiesCallback callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(SetCanonicalCookieTask);
+};
+
+void CookieMonster::SetCanonicalCookieTask::Run() {
+  bool result = this->cookie_monster()->SetCanonicalCookie(
+      std::move(cookie_), secure_source_, modify_http_only_);
+  if (!callback_.is_null())
+    callback_.Run(result);
+}
+
 // Task class for SetCookieWithOptions call.
 class CookieMonster::SetCookieWithOptionsTask : public CookieMonsterTask {
  public:
@@ -749,7 +786,7 @@ void CookieMonster::SetAllCookiesTask::Run() {
 
   bool result = true;
   if (positive_diff.size() > 0)
-    result = this->cookie_monster()->SetCanonicalCookies(list_);
+    result = this->cookie_monster()->SetCanonicalCookiesInternal(list_);
 
   if (!callback_.is_null())
     callback_.Run(result);
@@ -882,6 +919,23 @@ void CookieMonster::SetAllCookiesAsync(const CookieList& list,
   DoCookieTask(task);
 }
 
+void CookieMonster::SetCanonicalCookieAsync(
+    const CanonicalCookie& cookie,
+    bool secure_source,
+    bool modify_http_only,
+    const SetCookiesCallback& callback) {
+  DCHECK(cookie.IsCanonical());
+  scoped_refptr<SetCanonicalCookieTask> task = new SetCanonicalCookieTask(
+      this, base::MakeUnique<CanonicalCookie>(cookie), secure_source,
+      modify_http_only, callback);
+
+  // TODO(rdsmith): Switch to DoCookieTaskForURL (or the equivalent).
+  // This is tricky because we don't have the scheme in this routine
+  // and DoCookieTaskForURL uses cookie_util::GetEffectiveDomain(scheme, host)
+  // to generate the database key to block behind.
+  DoCookieTask(task);
+}
+
 void CookieMonster::SetCookieWithOptionsAsync(
     const GURL& url,
     const std::string& cookie_line,
@@ -1048,16 +1102,6 @@ bool CookieMonster::SetCookieWithDetails(const GURL& url,
   if (!HasCookieableScheme(url))
     return false;
 
-  // TODO(mmenke): This class assumes each cookie to have a unique creation
-  // time. Allowing the caller to set the creation time violates that
-  // assumption. Worth fixing? Worth noting that time changes between browser
-  // restarts can cause the same issue.
-  base::Time actual_creation_time = creation_time;
-  if (creation_time.is_null()) {
-    actual_creation_time = CurrentTime();
-    last_time_seen_ = actual_creation_time;
-  }
-
   // Validate consistency of passed arguments.
   if (ParsedCookie::ParseTokenString(name) != name ||
       ParsedCookie::ParseValueString(value) != value ||
@@ -1066,10 +1110,6 @@ bool CookieMonster::SetCookieWithDetails(const GURL& url,
     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;
@@ -1088,15 +1128,10 @@ bool CookieMonster::SetCookieWithDetails(const GURL& url,
                             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));
+      name, value, cookie_domain, cookie_path, 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() {
@@ -1415,7 +1450,7 @@ void CookieMonster::StoreLoadedCookies(
     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, false);
       const Time cookie_access_time(cookie_ptr->LastAccessDate());
       if (earliest_access_time_.is_null() ||
           cookie_access_time < earliest_access_time_)
@@ -1629,7 +1664,7 @@ void CookieMonster::FindCookiesForKey(const std::string& key,
 
 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());
@@ -1652,7 +1687,7 @@ bool CookieMonster::DeleteAnyEquivalentCookie(const std::string& key,
     // 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(
@@ -1693,7 +1728,7 @@ bool CookieMonster::DeleteAnyEquivalentCookie(const std::string& key,
 CookieMonster::CookieMap::iterator CookieMonster::InternalInsertCookie(
     const std::string& key,
     std::unique_ptr<CanonicalCookie> cc,
-    const GURL& source_url,
+    bool source_secure,
     bool sync_to_store) {
   DCHECK(thread_checker_.CalledOnValidThread());
   CanonicalCookie* cc_ptr = cc.get();
@@ -1721,21 +1756,15 @@ CookieMonster::CookieMap::iterator CookieMonster::InternalInsertCookie(
   // 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);
-  }
+  CookieSource cookie_source_sample =
+      (source_secure
+           ? (cc_ptr->IsSecure()
+                  ? COOKIE_SOURCE_SECURE_COOKIE_CRYPTOGRAPHIC_SCHEME
+                  : COOKIE_SOURCE_NONSECURE_COOKIE_CRYPTOGRAPHIC_SCHEME)
+           : (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);
 
@@ -1764,20 +1793,34 @@ bool CookieMonster::SetCookieWithCreationTimeAndOptions(
     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());

[mmenke, 2017/05/22 18:51:27]

This is a web-visible behavior change, right?  This path didn't used to prevent
non-secure URLs from setting secure cookies, it only prevented from overwriting
non-secure cookies.

[mmenke, 2017/05/22 19:07:04]

If this is a web-visible behavior change, this should be done in a way it can be
independently reverted, and we should have Mike West on board, and maybe even an
intent email.  I actually think the current behavior is bizarre (And I think it
violate's Mike Wests's own spec (Google leave secure cookies alone)), but we got
plenty of complains about it.  We may see even more about this change, just
after people fixed their code to work with the new behavior.

[Randy Smith (Not in Mondays), 2017/05/22 21:54:03]

Aw, drat, it looks like I refactored a check from SetCookieWithDetails down into
the guts of the system.  Right you are.  

(Presuming you meant "prevented non-secure URLs from overwriting secure
cookies".)
I don't intend any web visible change.  I'll refactor to preserve the old
behavior.  You can either wait for that for your review or review now, as you
choose.

[Randy Smith (Not in Mondays), 2017/05/25 20:45:35]

Matt:  I was digging into this issue more closely (basically, writing tests to
enforce the behavior at the CookieMonster API level for those interfaces that
can set secure cookies from non-secure origins) and discovered that the
interface I *thought* could (SetCookieWithOptions) actually can't because the
CanonicalCookie constructor it uses does the check.  This makes me suspect that
there aren't any interfaces that allow this; SetCookieWithDetails has the check
explicitly.  I'll do a more thorough exploration of the source, but: Do you know
of any existing external interfaces you think should have the odd behavior? 
Because if there aren't any I'm inclined to refactor it down below
SetCanonicalCookie as in my first patchset.

[Randy Smith (Not in Mondays), 2017/05/25 20:59:25]

Followup: The only place where this might be a behavior change is iOS, and
comments in the code suggest that it's not expected to work.  SetCanonicalCookie
(which would allow it) is according to codesearch called from three places:
* SetCookieWithDetails: Explicitly disallowed.
* SetCookieWithCreationTimeAndOptions: Uses a CC constructor that disallows
* SetCanonicalCookies: Ok, there's a behavior change here, but I think the
current behavior is unintended.  This is called from the SetAllCookiesAsync()
interface, which doesn't take a URL.  It deletes all the cookies *not* in the
new list, then sets all the ones that are.  So it'll currently succeed in
setting a secure cookie, but if there's an existing one it'll fail.  But there's
a comment in the source 

// Use an empty GURL.  This method does not support setting secure cookies.

And the method is only called outside of tests on IOS when copying system
cookies into the cookie monster in CookieStoreIOS::WriteToCookieMonster.  So
changing this would be a behavior change in a corner case, with comments
suggesting it wasn't intended to be supported.  Oh joy.

droger@: Any opinions on whether or not this is needed for the IOS Case
mentioned above?  What currently happens is that secure cookies that aren't
replacing existing secure cookies of the same name in the cookie monster will be
store, but secure cookies that collide with already existing secure cookies will
be rejected.  If I make the change I want to,
WriteToCookieMonster/SetAllCookiesAsync would simply refuse to write secure
cookies.  WDYT?

 }
 
 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());
 
-  Time creation_time = cc->CreationDate();
+  // TODO(mmenke): This class assumes each cookie to have a unique creation
+  // time. Allowing the caller to set the creation time violates that
+  // assumption. Worth fixing? Worth noting that time changes between browser
+  // restarts can cause the same issue.
+  base::Time creation_date = cc->CreationDate();
+  if (cc->CreationDate().is_null()) {
+    creation_date = CurrentTime();
+    cc->SetCreationDate(creation_date);
+    last_time_seen_ = creation_date;
+  }
+
   const std::string key(GetKey(cc->Domain()));
-  bool already_expired = cc->IsExpired(creation_time);
+  bool already_expired = cc->IsExpired(creation_date);
+
+  if (!secure_source && cc->IsSecure())
+    return false;
 
-  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 "
@@ -1796,10 +1839,10 @@ bool CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc,
     // See InitializeHistograms() for details.
     if (cc->IsPersistent()) {
       histogram_expiration_duration_minutes_->Add(
-          (cc->ExpiryDate() - creation_time).InMinutes());
+          (cc->ExpiryDate() - creation_date).InMinutes());
     }
 
-    InternalInsertCookie(key, std::move(cc), source_url, true);
+    InternalInsertCookie(key, std::move(cc), secure_source, true);
   } else {
     VLOG(kVlogSetCookies) << "SetCookie() not storing already expired cookie.";
   }
@@ -1809,12 +1852,12 @@ 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);
+  GarbageCollect(creation_date, key);
 
   return true;
 }
 
-bool CookieMonster::SetCanonicalCookies(const CookieList& list) {
+bool CookieMonster::SetCanonicalCookiesInternal(const CookieList& list) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   CookieOptions options;
@@ -1822,8 +1865,8 @@ bool CookieMonster::SetCanonicalCookies(const CookieList& list) {
 
   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)) {
+    if (!SetCanonicalCookie(base::MakeUnique<CanonicalCookie>(cookie), false,
+                            true)) {
       return false;
     }
   }
@@ -2255,7 +2298,7 @@ void CookieMonster::InitializeHistograms() {
       "Cookie.Type", 1, (1 << COOKIE_TYPE_LAST_ENTRY) - 1,
       1 << COOKIE_TYPE_LAST_ENTRY, base::Histogram::kUmaTargetedHistogramFlag);
   histogram_cookie_source_scheme_ = base::LinearHistogram::FactoryGet(
-      "Cookie.CookieSourceScheme", 1, COOKIE_SOURCE_LAST_ENTRY - 1,
+      "Cookie.CookieSourceScheme2", 1, COOKIE_SOURCE_LAST_ENTRY - 1,
       COOKIE_SOURCE_LAST_ENTRY, base::Histogram::kUmaTargetedHistogramFlag);
   histogram_cookie_delete_equivalent_ = base::LinearHistogram::FactoryGet(
       "Cookie.CookieDeleteEquivalent", 1,