Update cookie value and attribute setting behavior to match other UAs

net/cookies/parsed_cookie.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 345 matching lines @@
 
   // Ok, here we go.  We should be expecting to be starting somewhere
   // before the cookie line, not including any header name...
   std::string::const_iterator start = cookie_line.begin();
   std::string::const_iterator it = start;
 
   // TODO(erikwright): Make sure we're stripping \r\n in the network code.
   // Then we can log any unexpected terminators.
   std::string::const_iterator end = FindFirstTerminator(cookie_line);
 
+  // For an empty |cookie_line|, add an empty-key with an empty value, which
+  // has the effect of clearing any prior setting of the empty-key. This is done
+  // to match the behavior of other browsers. See https://crbug.com/601786.
+  if (it == end)
+    pairs_.push_back(TokenValuePair("", ""));

[mmenke, 2016/08/15 21:11:03]

Maybe add an early return?  Doesn't make a difference, but makes it clear that
there's nothing else do do at this point.

Alternatively, if there's a concern that we may eventually do post-processing
here, which should apply to this entry, should use an if / else, to again, make
clear this branch is exclusive from the below code.

[jww, 2016/08/16 02:24:23]

Early return sgtm. Done.

+
   for (int pair_num = 0; pair_num < kMaxPairs && it != end; ++pair_num) {
     TokenValuePair pair;
 
     std::string::const_iterator token_start, token_end;
-    if (!ParseToken(&it, end, &token_start, &token_end))
+    bool did_parse_token = ParseToken(&it, end, &token_start, &token_end);
+    if (!did_parse_token && pair_num != 0)

[mmenke, 2016/08/15 21:11:03]

Think this is worth a comment (Just "Allow first token to be empty" is fine).

[mmenke, 2016/08/15 21:11:03]

What about if other tokens are empty?  Do other browsers do the same thing as we
do?  Not suggesting other changes here, just wondering if that area has been
explored.

[jww, 2016/08/16 02:24:23]

Comment added.

For tokens other than the first, I think they are allowed to be empty, but they
must be parsable. This particular change is to handle invalid tokens, not empty
ones (so the case of just having the character '\t' in the cookie_line), which
other browsers have made count as being empty.

In any case, I think the chart the reporter put together covers a lot of these
cases (http://inikulin.github.io/cookie-compat/). The spec actually disallows
what this change implements, but it's just meant to match other UA behavior.

[mmenke, 2016/08/16 15:06:47]

Ahh...The "break;" was concerning me.  I had assumed ParseToken returned false
on something like ";", so "foo=bar;;domain=.foo.com" would end up ignoring the
domain (And the same behavior with anything sufficiently ugly as the middle
parameter), but looks like ParseToken only returns false if there's only
whitespace remaining, not at all the behavior I expected.

       break;
 
-    if (it == end || *it != '=') {
+    if (!did_parse_token || it == end || *it != '=') {
       // We have a token-value, we didn't have any token name.
       if (pair_num == 0) {
         // For the first time around, we want to treat single values
         // as a value with an empty name. (Mozilla bug 169091).
         // IE seems to also have this behavior, ex "AAA", and "AAA=10" will
         // set 2 different cookies, and setting "BBB" will then replace "AAA".
         pair.first = "";
         // Rewind to the beginning of what we thought was the token name,
         // and let it get parsed as a value.
-        it = token_start;
+        it = did_parse_token ? token_start : start;

[mmenke, 2016/08/15 21:11:03]

Having this extra logic seems a little ugly.

I'd suggest putting all the !did_parse_toke logic up with the ParseToken call.

if (!ParseToken(....)) {
  ...
} else if (it == end || *id != '=') {
....

[jww, 2016/08/16 02:24:23]

I agree that this is ugly, but from looking at this for a while, I think it's
actually the *least* ugly we can (i.e. the least repeated code). If I split it
the way you suggest above, that will necessitate repeating the body of 'if
(pair_num == 0) { ...' in both branches that you suggest.

If I'm missing something, though, I'm happy to clean it up however you suggest.
I got rid of the terinary in the latest patchset and tried to clarify the
comment; hopefully that will make it easier to understand.

       } else {
         // Any not-first attribute we want to treat a value as a
         // name with an empty value...  This is so something like
         // "secure;" will get parsed as a Token name, and not a value.
         pair.first = std::string(token_start, token_end);
       }
     } else {
       // We have a TOKEN=VALUE.
       pair.first = std::string(token_start, token_end);
       ++it;  // Skip past the '='.
@@ skipping 20 matching lines @@
     pairs_.push_back(pair);
 
     // We've processed a token/value pair, we're either at the end of
     // the string or a ValueSeparator like ';', which we want to skip.
     if (it != end)
       ++it;
   }
 }
 
 void ParsedCookie::SetupAttributes() {
-  // Ignore Set-Cookie directive where name and value are both empty.
-  if (pairs_[0].first.empty() && pairs_[0].second.empty()) {
-    pairs_.clear();
-    return;
-  }
-
   // We skip over the first token/value, the user supplied one.
   for (size_t i = 1; i < pairs_.size(); ++i) {
     if (pairs_[i].first == kPathTokenName) {
       path_index_ = i;
-    } else if (pairs_[i].first == kDomainTokenName) {
+    } else if (pairs_[i].first == kDomainTokenName && pairs_[i].second != "") {

[mmenke, 2016/08/15 21:11:03]

Here's something weird:

For all the other tokens, we unconditionally take the last value.  For domain,
we take the last one that isn't "".  I'd actually be more comfortable if we took
the last domain, unconditionally.  i.e., if second is "", then set domain_index_
to the default value (0?), overwriting the previous value.

Unless this is really what all other browsers do (Both for domain and for
everything else).

[jww, 2016/08/16 02:24:23]

This wouldn't match the other UAs, unfortunately, which would defeat the purpose
of this patchset (which is a fine decision if we decide to go that route).
I tried to clarify that on the bug, because I can't tell from the chart what the
other UAs do for attributes such as Expires. I agree that it's weird, but I'd
also rather get Chrome to conform with the (admittedly quirky) behavior of the
other UAs here. So, yeah, this is a hack, but it also seems to be what they do.

[mmenke, 2016/08/16 15:06:47]

Sorry, should have been clearer.  Looking at this code, and this asymmetry, I
find it very hard to believe this is actually what other UAs do.  Because it's
bizarre, and I doubt this is actually the consensus behavior.  Before landing
this, I'd like to determine if this check should be done for more / most of
these.

       domain_index_ = i;
     } else if (pairs_[i].first == kExpiresTokenName) {
       expires_index_ = i;
     } else if (pairs_[i].first == kMaxAgeTokenName) {
       maxage_index_ = i;
     } else if (pairs_[i].first == kSecureTokenName) {
       secure_index_ = i;
     } else if (pairs_[i].first == kHttpOnlyTokenName) {
       httponly_index_ = i;
     } else if (pairs_[i].first == kSameSiteTokenName) {
@@ skipping 59 matching lines @@
       --*indexes[i];
   }
   pairs_.erase(pairs_.begin() + index);
 }
 
 bool ParsedCookie::IsSameSiteAttributeValid() const {
   return same_site_index_ == 0 || SameSite() != CookieSameSite::DEFAULT_MODE;
 }
 
 }  // namespace