Move Secure cookie check into CanonicalCookie::IsSetPermittedInContext.

This cl also converts a portion of CanonicalCookie unit tests to use
matcher-based code, in order to be able to more easily assert multiple
things about multiple parts of the return value of
IsSetPermittedInContext. (E.g., being able to make assertions about both
the |status| and the |is_allowed_to_access_secure_cookies| fields of the
|CookieAccessResult|. This also has the benefit of being more
declarative and avoiding declaring temp variables, which are
error-prone. In particular, this fixes bugs where the wrong temp
variable was accidentally used (lines 2785, 2899).

This cl also fixes a bug (in test) where the LEGACY access semantics
were accidentally unused, with UNKNOWN being redundantly used instead
(lines 2967-2987).

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

net/cookies/canonical_cookie.cc

@@ -662,19 +662,29 @@ CookieAccessResult CanonicalCookie::IncludeForRequestURL(
   // insecure scheme, unless it is a localhost url, or the CookieAccessDelegate
   // otherwise denotes them as trustworthy
   // (`delegate_treats_url_as_trustworthy`).
-  if (IsSecure()) {
+  bool is_allowed_to_access_secure_cookies = false;
   CookieAccessScheme cookie_access_scheme =
       cookie_util::ProvisionalAccessScheme(url);
   if (cookie_access_scheme == CookieAccessScheme::kNonCryptographic &&
       params.delegate_treats_url_as_trustworthy) {
     cookie_access_scheme = CookieAccessScheme::kTrustworthy;
   }
-    if (cookie_access_scheme == CookieAccessScheme::kNonCryptographic) {
+  switch (cookie_access_scheme) {
+    case CookieAccessScheme::kNonCryptographic:
+      if (IsSecure())
         status.AddExclusionReason(CookieInclusionStatus::EXCLUDE_SECURE_ONLY);
-    } else if (cookie_access_scheme == CookieAccessScheme::kTrustworthy) {
+      break;
+    case CookieAccessScheme::kTrustworthy:
+      is_allowed_to_access_secure_cookies = true;
+      if (IsSecure()) {
         status.AddWarningReason(
-          CookieInclusionStatus::WARN_SECURE_ACCESS_GRANTED_NON_CRYPTOGRAPHIC);
+            CookieInclusionStatus::
+                WARN_SECURE_ACCESS_GRANTED_NON_CRYPTOGRAPHIC);
       }
+      break;
+    case CookieAccessScheme::kCryptographic:
+      is_allowed_to_access_secure_cookies = true;
+      break;
   }
   // Don't include cookies for requests that don't apply to the cookie domain.
   if (!IsDomainMatch(url.host()))
@@ -771,21 +781,56 @@ CookieAccessResult CanonicalCookie::IncludeForRequestURL(
 
   // TODO(chlily): Log metrics.
   return CookieAccessResult(effective_same_site, status,
-                            params.access_semantics);
+                            params.access_semantics,
+                            is_allowed_to_access_secure_cookies);
 }
 
 CookieAccessResult CanonicalCookie::IsSetPermittedInContext(
+    const GURL& source_url,
     const CookieOptions& options,
     const CookieAccessParams& params) const {
   CookieAccessResult access_result;
-  IsSetPermittedInContext(options, params, &access_result);
+  IsSetPermittedInContext(source_url, options, params, &access_result);
   return access_result;
 }
 
 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 &&
+      params.delegate_treats_url_as_trustworthy) {
+    access_scheme = CookieAccessScheme::kTrustworthy;
+  }
+
+  switch (access_scheme) {
+    case CookieAccessScheme::kNonCryptographic:
+      access_result->is_allowed_to_access_secure_cookies = false;
+      if (IsSecure()) {
+        access_result->status.AddExclusionReason(
+            CookieInclusionStatus::EXCLUDE_SECURE_ONLY);
+      }
+      break;
+
+    case CookieAccessScheme::kCryptographic:
+      // All cool!
+      access_result->is_allowed_to_access_secure_cookies = true;
+      break;
+
+    case CookieAccessScheme::kTrustworthy:
+      access_result->is_allowed_to_access_secure_cookies = true;
+      if (IsSecure()) {
+        // OK, but want people aware of this.
+        access_result->status.AddWarningReason(
+            CookieInclusionStatus::
+                WARN_SECURE_ACCESS_GRANTED_NON_CRYPTOGRAPHIC);
+      }
+      break;
+  }
+
   access_result->access_semantics = params.access_semantics;
   if (options.exclude_httponly() && IsHttpOnly()) {
     DVLOG(net::cookie_util::kVlogSetCookies)

net/cookies/canonical_cookie.h

@@ -312,11 +312,13 @@ class NET_EXPORT CanonicalCookie {
   // in a secure scheme, since whether the scheme is secure isn't part of
   // |options|.
   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 CookieOptions& options,
+  void IsSetPermittedInContext(const GURL& source_url,
+                               const CookieOptions& options,
                                const CookieAccessParams& params,
                                CookieAccessResult* access_result) const;
 

net/cookies/cookie_access_result.cc

@@ -11,10 +11,13 @@ CookieAccessResult::CookieAccessResult() = default;
 CookieAccessResult::CookieAccessResult(
     CookieEffectiveSameSite effective_same_site,
     CookieInclusionStatus status,
-    CookieAccessSemantics access_semantics)
+    CookieAccessSemantics access_semantics,
+    bool is_allowed_to_access_secure_cookies)
     : status(status),
       effective_same_site(effective_same_site),
-      access_semantics(access_semantics) {}
+      access_semantics(access_semantics),
+      is_allowed_to_access_secure_cookies(is_allowed_to_access_secure_cookies) {
+}
 
 CookieAccessResult::CookieAccessResult(CookieInclusionStatus status)
     : status(status) {}

net/cookies/cookie_access_result.h

@@ -5,6 +5,8 @@
 #ifndef NET_COOKIES_COOKIE_ACCESS_RESULT_H_
 #define NET_COOKIES_COOKIE_ACCESS_RESULT_H_
 
+#include <ostream>
+
 #include "net/base/net_export.h"
 #include "net/cookies/cookie_constants.h"
 #include "net/cookies/cookie_inclusion_status.h"
@@ -18,7 +20,8 @@ struct NET_EXPORT CookieAccessResult {
   CookieAccessResult();
   CookieAccessResult(CookieEffectiveSameSite effective_same_site,
                      CookieInclusionStatus status,
-                     CookieAccessSemantics access_semantics);
+                     CookieAccessSemantics access_semantics,
+                     bool is_allowed_to_access_secure_cookie);
 
   explicit CookieAccessResult(CookieInclusionStatus status);
 
@@ -34,8 +37,21 @@ struct NET_EXPORT CookieAccessResult {
   CookieEffectiveSameSite effective_same_site =
       CookieEffectiveSameSite::UNDEFINED;
   CookieAccessSemantics access_semantics = CookieAccessSemantics::UNKNOWN;
+  // Whether access to Secure cookies should be allowed. This is expected to be
+  // set based on the scheme of the source URL.
+  bool is_allowed_to_access_secure_cookies = false;
 };
 
+// Provided to allow gtest to create more helpful error messages, instead of
+// printing hex.
+inline void PrintTo(const CookieAccessResult& car, std::ostream* os) {
+  *os << "{ { ";
+  PrintTo(car.status, os);
+  *os << " }, " << static_cast<int>(car.effective_same_site) << ", "
+      << static_cast<int>(car.access_semantics) << ", "
+      << car.is_allowed_to_access_secure_cookies << " }";
+}
+
 }  // namespace net
 
 #endif  // NET_COOKIES_COOKIE_ACCESS_RESULT_H_

net/cookies/cookie_inclusion_status.h

@@ -292,6 +292,12 @@ NET_EXPORT inline std::ostream& operator<<(std::ostream& os,
   return os << status.GetDebugString();
 }
 
+// Provided to allow gtest to create more helpful error messages, instead of
+// printing hex.
+inline void PrintTo(const CookieInclusionStatus& cis, std::ostream* os) {
+  *os << cis;
+}
+
 }  // namespace net
 
 #endif  // NET_COOKIES_COOKIE_INCLUSION_STATUS_H_

net/cookies/cookie_monster.cc

@@ -1190,37 +1190,6 @@ void CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc,
   bool delegate_treats_url_as_trustworthy =
       cookie_access_delegate() &&
       cookie_access_delegate()->ShouldTreatUrlAsTrustworthy(source_url);
-  CookieAccessScheme access_scheme =
-      cookie_util::ProvisionalAccessScheme(source_url);
-  if (access_scheme == CookieAccessScheme::kNonCryptographic &&
-      delegate_treats_url_as_trustworthy) {
-    access_scheme = CookieAccessScheme::kTrustworthy;
-  }
-
-  bool allowed_to_set_secure_cookie = false;
-  switch (access_scheme) {
-    case CookieAccessScheme::kNonCryptographic:
-      if (cc->IsSecure()) {
-        access_result.status.AddExclusionReason(
-            CookieInclusionStatus::EXCLUDE_SECURE_ONLY);
-      }
-      break;
-
-    case CookieAccessScheme::kCryptographic:
-      // All cool!
-      allowed_to_set_secure_cookie = true;
-      break;
-
-    case CookieAccessScheme::kTrustworthy:
-      allowed_to_set_secure_cookie = true;
-      if (cc->IsSecure()) {
-        // OK, but want people aware of this.
-        access_result.status.AddWarningReason(
-            CookieInclusionStatus::
-                WARN_SECURE_ACCESS_GRANTED_NON_CRYPTOGRAPHIC);
-      }
-      break;
-  }
 
   if (!IsCookieableScheme(source_url.scheme())) {
     access_result.status.AddExclusionReason(
@@ -1230,7 +1199,7 @@ void CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc,
   const std::string key(GetKey(cc->Domain()));
 
   cc->IsSetPermittedInContext(
-      options,
+      source_url, options,
       CookieAccessParams(GetAccessSemanticsForCookie(*cc),
                          delegate_treats_url_as_trustworthy),
       &access_result);
@@ -1248,8 +1217,9 @@ void CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc,
   // deletes an existing cookie, so any ExclusionReasons in |status| that would
   // prevent such deletion should be finalized beforehand.
   MaybeDeleteEquivalentCookieAndUpdateStatus(
-      key, *cc, allowed_to_set_secure_cookie, options.exclude_httponly(),
-      already_expired, &creation_date_to_inherit, &access_result.status);
+      key, *cc, access_result.is_allowed_to_access_secure_cookies,
+      options.exclude_httponly(), already_expired, &creation_date_to_inherit,
+      &access_result.status);
 
   if (access_result.status.HasExclusionReason(
           CookieInclusionStatus::EXCLUDE_OVERWRITE_SECURE) ||
@@ -1427,10 +1397,12 @@ void CookieMonster::InternalDeleteCookie(CookieMap::iterator it,
     store_->DeleteCookie(*cc);
   }
   change_dispatcher_.DispatchChange(
-      CookieChangeInfo(*cc,
+      CookieChangeInfo(
+          *cc,
           CookieAccessResult(CookieEffectiveSameSite::UNDEFINED,
                              CookieInclusionStatus(),
-                                          GetAccessSemanticsForCookie(*cc)),
+                             GetAccessSemanticsForCookie(*cc),
+                             true /* is_allowed_to_access_secure_cookies */),
           mapping.cause),
       mapping.notify);
 

services/network/public/cpp/cookie_manager_mojom_traits.cc

@@ -457,7 +457,8 @@ bool StructTraits<
   if (!c.ReadAccessSemantics(&access_semantics))
     return false;
 
-  *out = {effective_same_site, status, access_semantics};
+  *out = {effective_same_site, status, access_semantics,
+          c.is_allowed_to_access_secure_cookies()};
 
   return true;
 }

services/network/public/cpp/cookie_manager_mojom_traits.h

@@ -217,6 +217,10 @@ struct StructTraits<network::mojom::CookieAccessResultDataView,
       const net::CookieAccessResult& c) {
     return c.access_semantics;
   }
+  static bool is_allowed_to_access_secure_cookies(
+      const net::CookieAccessResult& c) {
+    return c.is_allowed_to_access_secure_cookies;
+  }
   static bool Read(network::mojom::CookieAccessResultDataView access_result,
                    net::CookieAccessResult* out);
 };

services/network/public/cpp/cookie_manager_mojom_traits_unittest.cc

@@ -118,7 +118,7 @@ TEST(CookieManagerTraitsTest, Roundtrips_CookieInclusionStatus) {
           invalid, copied));
 }
 
-TEST(CookieManagerTraitsTest, Rountrips_CookieAccessResult) {
+TEST(CookieManagerTraitsTest, Roundtrips_CookieAccessResult) {
   net::CookieAccessResult original = net::CookieAccessResult(
       net::CookieEffectiveSameSite::LAX_MODE,
       net::CookieInclusionStatus(
@@ -126,7 +126,8 @@ TEST(CookieManagerTraitsTest, Rountrips_CookieAccessResult) {
               EXCLUDE_SAMESITE_UNSPECIFIED_TREATED_AS_LAX,
           net::CookieInclusionStatus::
               WARN_SAMESITE_UNSPECIFIED_CROSS_SITE_CONTEXT),
-      net::CookieAccessSemantics::LEGACY);
+      net::CookieAccessSemantics::LEGACY,
+      true /* is_allowed_to_access_secure_cookies */);
   net::CookieAccessResult copied;
 
   EXPECT_TRUE(mojo::test::SerializeAndDeserialize<mojom::CookieAccessResult>(
@@ -139,12 +140,14 @@ TEST(CookieManagerTraitsTest, Rountrips_CookieAccessResult) {
   EXPECT_TRUE(copied.status.HasExactlyWarningReasonsForTesting(
       {net::CookieInclusionStatus::
            WARN_SAMESITE_UNSPECIFIED_CROSS_SITE_CONTEXT}));
+  EXPECT_EQ(original.is_allowed_to_access_secure_cookies,
+            copied.is_allowed_to_access_secure_cookies);
 }
 
 TEST(CookieManagerTraitsTest, Rountrips_CookieWithAccessResult) {
   net::CanonicalCookie original_cookie(
       "A", "B", "x.y", "/path", base::Time(), base::Time(), base::Time(),
-      /* secure = */ true, /* http_only = */ false,
+      /* secure = */ true, /* httponly = */ false,
       net::CookieSameSite::NO_RESTRICTION, net::COOKIE_PRIORITY_LOW, false);
 
   net::CookieWithAccessResult original = {original_cookie,
@@ -172,10 +175,10 @@ TEST(CookieManagerTraitsTest, Rountrips_CookieWithAccessResult) {
   EXPECT_EQ(original.access_result.status, copied.access_result.status);
 }
 
-TEST(CookieManagerTraitsTest, Rountrips_CookieAndLineWithAccessResult) {
+TEST(CookieManagerTraitsTest, Roundtrips_CookieAndLineWithAccessResult) {
   net::CanonicalCookie original_cookie(
       "A", "B", "x.y", "/path", base::Time(), base::Time(), base::Time(),
-      /* secure = */ true, /* http_only = */ false,
+      /* secure = */ true, /* httponly = */ false,
       net::CookieSameSite::NO_RESTRICTION, net::COOKIE_PRIORITY_LOW, false);
 
   net::CookieAndLineWithAccessResult original(original_cookie, "cookie-string",
@@ -374,14 +377,15 @@ TEST(CookieManagerTraitsTest, Roundtrips_FullPartyContext) {
 TEST(CookieManagerTraitsTest, Roundtrips_CookieChangeInfo) {
   net::CanonicalCookie original_cookie(
       "A", "B", "x.y", "/path", base::Time(), base::Time(), base::Time(),
-      /* secure = */ false, /* http_only = */ false,
+      /* secure = */ false, /* httponly = */ false,
       net::CookieSameSite::UNSPECIFIED, net::COOKIE_PRIORITY_LOW, false);
 
   net::CookieChangeInfo original(
       original_cookie,
       net::CookieAccessResult(net::CookieEffectiveSameSite::UNDEFINED,
                               net::CookieInclusionStatus(),
-                              net::CookieAccessSemantics::LEGACY),
+                              net::CookieAccessSemantics::LEGACY,
+                              false /* is_allowed_to_access_secure_cookies */),
       net::CookieChangeCause::EXPLICIT);
 
   net::CookieChangeInfo copied;

services/network/public/mojom/cookie_manager.mojom

@@ -156,10 +156,12 @@ struct CookieAndLineWithAccessResult {
   CookieAccessResult access_result;
 };
 
+// See net/cookies/cookie_access_result.{cc,h} for documentation.
 struct CookieAccessResult {
   CookieEffectiveSameSite effective_same_site;
   CookieAccessSemantics access_semantics;
   CookieInclusionStatus status;
+  bool is_allowed_to_access_secure_cookies;
 };
 
 struct CookieWithAccessResult {