Move cookieable-scheme check into CanonicalCookie.

This aims for a simpler method signature for
CanonicalCookie::IsSetPermittedInContext, and aims to have all exclusion
reasons (except those set by MaybeDeleteEquivalentCookieAndUpdateStatus)
set by CanonicalCookie itself.

Change-Id: Id0673d11795241f10728d90424612b1bc0ab9883
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2611764Reviewed-by: Lily Chen <chlily@chromium.org>
Commit-Queue: Chris Fredrickson <cfredric@chromium.org>
Cr-Commit-Position: refs/heads/master@{#840347}

net/cookies/canonical_cookie.cc

@@ -788,17 +788,14 @@ CookieAccessResult CanonicalCookie::IncludeForRequestURL(
 CookieAccessResult CanonicalCookie::IsSetPermittedInContext(
     const GURL& source_url,
     const CookieOptions& options,
-    const CookieAccessParams& params) const {
+    const CookieAccessParams& params,
+    const std::vector<std::string>& cookieable_schemes) const {
   CookieAccessResult access_result;
-  IsSetPermittedInContext(source_url, options, params, &access_result);
-  return access_result;
-}
+  if (!base::Contains(cookieable_schemes, source_url.scheme())) {
+    access_result.status.AddExclusionReason(
+        CookieInclusionStatus::EXCLUDE_NONCOOKIEABLE_SCHEME);
+  }
 
-void CanonicalCookie::IsSetPermittedInContext(
-    const GURL& source_url,
-    const CookieOptions& options,
-    const CookieAccessParams& params,
-    CookieAccessResult* access_result) const {
   CookieAccessScheme access_scheme =
       cookie_util::ProvisionalAccessScheme(source_url);
   if (access_scheme == CookieAccessScheme::kNonCryptographic &&
@@ -808,34 +805,34 @@ void CanonicalCookie::IsSetPermittedInContext(
 
   switch (access_scheme) {
     case CookieAccessScheme::kNonCryptographic:
-      access_result->is_allowed_to_access_secure_cookies = false;
+      access_result.is_allowed_to_access_secure_cookies = false;
       if (IsSecure()) {
-        access_result->status.AddExclusionReason(
+        access_result.status.AddExclusionReason(
             CookieInclusionStatus::EXCLUDE_SECURE_ONLY);
       }
       break;
 
     case CookieAccessScheme::kCryptographic:
       // All cool!
-      access_result->is_allowed_to_access_secure_cookies = true;
+      access_result.is_allowed_to_access_secure_cookies = true;
       break;
 
     case CookieAccessScheme::kTrustworthy:
-      access_result->is_allowed_to_access_secure_cookies = true;
+      access_result.is_allowed_to_access_secure_cookies = true;
       if (IsSecure()) {
         // OK, but want people aware of this.
-        access_result->status.AddWarningReason(
+        access_result.status.AddWarningReason(
             CookieInclusionStatus::
                 WARN_SECURE_ACCESS_GRANTED_NON_CRYPTOGRAPHIC);
       }
       break;
   }
 
-  access_result->access_semantics = params.access_semantics;
+  access_result.access_semantics = params.access_semantics;
   if (options.exclude_httponly() && IsHttpOnly()) {
     DVLOG(net::cookie_util::kVlogSetCookies)
         << "HttpOnly cookie not permitted in script context.";
-    access_result->status.AddExclusionReason(
+    access_result.status.AddExclusionReason(
         CookieInclusionStatus::EXCLUDE_HTTP_ONLY);
   }
 
@@ -847,7 +844,7 @@ void CanonicalCookie::IsSetPermittedInContext(
       SameSite() == CookieSameSite::NO_RESTRICTION && !IsSecure()) {
     DVLOG(net::cookie_util::kVlogSetCookies)
         << "SetCookie() rejecting insecure cookie with SameSite=None.";
-    access_result->status.AddExclusionReason(
+    access_result.status.AddExclusionReason(
         CookieInclusionStatus::EXCLUDE_SAMESITE_NONE_INSECURE);
   }
   // Log whether a SameSite=None cookie is Secure or not.
@@ -862,11 +859,11 @@ void CanonicalCookie::IsSetPermittedInContext(
           ? options.same_site_cookie_context().context()
           : options.same_site_cookie_context().GetContextForCookieInclusion();
 
-  access_result->effective_same_site =
+  access_result.effective_same_site =
       GetEffectiveSameSite(params.access_semantics);
-  DCHECK(access_result->effective_same_site !=
+  DCHECK(access_result.effective_same_site !=
          CookieEffectiveSameSite::UNDEFINED);
-  switch (access_result->effective_same_site) {
+  switch (access_result.effective_same_site) {
     case CookieEffectiveSameSite::STRICT_MODE:
       // This intentionally checks for `< SAME_SITE_LAX`, as we allow
       // `SameSite=Strict` cookies to be set for top-level navigations that
@@ -876,7 +873,7 @@ void CanonicalCookie::IsSetPermittedInContext(
         DVLOG(net::cookie_util::kVlogSetCookies)
             << "Trying to set a `SameSite=Strict` cookie from a "
                "cross-site URL.";
-        access_result->status.AddExclusionReason(
+        access_result.status.AddExclusionReason(
             CookieInclusionStatus::EXCLUDE_SAMESITE_STRICT);
       }
       break;
@@ -888,13 +885,13 @@ void CanonicalCookie::IsSetPermittedInContext(
           DVLOG(net::cookie_util::kVlogSetCookies)
               << "Cookies with no known SameSite attribute being treated as "
                  "lax; attempt to set from a cross-site URL denied.";
-          access_result->status.AddExclusionReason(
+          access_result.status.AddExclusionReason(
               CookieInclusionStatus::
                   EXCLUDE_SAMESITE_UNSPECIFIED_TREATED_AS_LAX);
         } else {
           DVLOG(net::cookie_util::kVlogSetCookies)
               << "Trying to set a `SameSite=Lax` cookie from a cross-site URL.";
-          access_result->status.AddExclusionReason(
+          access_result.status.AddExclusionReason(
               CookieInclusionStatus::EXCLUDE_SAMESITE_LAX);
         }
       }
@@ -904,17 +901,19 @@ void CanonicalCookie::IsSetPermittedInContext(
   }
 
   ApplySameSiteCookieWarningToStatus(
-      SameSite(), access_result->effective_same_site, IsSecure(),
-      options.same_site_cookie_context(), &access_result->status,
+      SameSite(), access_result.effective_same_site, IsSecure(),
+      options.same_site_cookie_context(), &access_result.status,
       true /* is_cookie_being_set */);
 
-  if (access_result->status.IsInclude()) {
+  if (access_result.status.IsInclude()) {
     UMA_HISTOGRAM_ENUMERATION("Cookie.IncludedResponseEffectiveSameSite",
-                              access_result->effective_same_site,
+                              access_result.effective_same_site,
                               CookieEffectiveSameSite::COUNT);
   }
 
   // TODO(chlily): Log metrics.
+
+  return access_result;
 }
 
 std::string CanonicalCookie::DebugString() const {

net/cookies/canonical_cookie.h

@@ -311,13 +311,8 @@ class NET_EXPORT CanonicalCookie {
   CookieAccessResult IsSetPermittedInContext(
       const GURL& source_url,
       const CookieOptions& options,
-      const CookieAccessParams& params) const;
-
-  // Overload that updates an existing |status| rather than returning a new one.
-  void IsSetPermittedInContext(const GURL& source_url,
-                               const CookieOptions& options,
-                               const CookieAccessParams& params,
-                               CookieAccessResult* access_result) const;
+      const CookieAccessParams& params,
+      const std::vector<std::string>& cookieable_schemes) const;
 
   std::string DebugString() const;
 

net/cookies/cookie_monster.cc

@@ -479,12 +479,6 @@ void CookieMonster::SetPersistSessionCookies(bool persist_session_cookies) {
   persist_session_cookies_ = persist_session_cookies;
 }
 
-bool CookieMonster::IsCookieableScheme(const std::string& scheme) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
-  return base::Contains(cookieable_schemes_, scheme);
-}
-
 const char* const CookieMonster::kDefaultCookieableSchemes[] = {"http", "https",
                                                                 "ws", "wss"};
 const int CookieMonster::kDefaultCookieableSchemesCount =
@@ -1175,24 +1169,17 @@ void CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc,
                                        SetCookiesCallback callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  CookieAccessResult access_result;
-
   bool delegate_treats_url_as_trustworthy =
       cookie_access_delegate() &&
       cookie_access_delegate()->ShouldTreatUrlAsTrustworthy(source_url);
 
-  if (!IsCookieableScheme(source_url.scheme())) {
-    access_result.status.AddExclusionReason(
-        CookieInclusionStatus::EXCLUDE_NONCOOKIEABLE_SCHEME);
-  }
-
-  const std::string key(GetKey(cc->Domain()));
-
-  cc->IsSetPermittedInContext(
+  CookieAccessResult access_result = cc->IsSetPermittedInContext(
       source_url, options,
       CookieAccessParams(GetAccessSemanticsForCookie(*cc),
                          delegate_treats_url_as_trustworthy),
-      &access_result);
+      cookieable_schemes_);
+
+  const std::string key(GetKey(cc->Domain()));
 
   base::Time creation_date = cc->CreationDate();
   if (creation_date.is_null()) {

net/cookies/cookie_monster.h

@@ -190,10 +190,6 @@ class NET_EXPORT CookieMonster : public CookieStore {
   // (i.e. as part of the instance initialization process).
   void SetPersistSessionCookies(bool persist_session_cookies);
 
-  // Determines if the scheme of the URL is a scheme that cookies will be
-  // stored for.
-  bool IsCookieableScheme(const std::string& scheme);
-
   // The default list of schemes the cookie monster can handle.
   static const char* const kDefaultCookieableSchemes[];
   static const int kDefaultCookieableSchemesCount;