Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2246613003:
Update cookie value and attribute setting behavior to match other UAs (Closed)
DescriptionUpdate cookie value and attribute setting behavior to match other UAs
This is an attempt to bring Chrome's cookie behavior in line with the
majority of other major UAs, and thus are de facto standards. The
specific updates are:
1. An empty domain attribute value should be ignored as a
non-attribute rather than setting an empty attribute value.
2. An empty cookie line (or a cookie line with ignored characters)
should be treated as setting an empty value for the empty-key.
BUG=601786
Committed: https://crrev.com/03e6ff8c614040f60d1a1d818411870ddb3b90e4
Cr-Commit-Position: refs/heads/master@{#412608}
Patch Set 1 #Patch Set 2 : Updated ParsedCookie unittests #
Total comments: 20
Patch Set 3 : Fixes from mmenke #
Total comments: 10
Patch Set 4 : More fixes #Patch Set 5 : iOS and unit test fixes #
Total comments: 8
Patch Set 6 : Fixes #Patch Set 7 : Rebase on ToT #Patch Set 8 : Simplification #
Messages
Total messages: 29 (11 generated)
jww@chromium.org changed reviewers: + mmenke@chromium.org
mmenke@, can you take a look at this changes? Thanks!
Description was changed from
==========
Update cookie value and attribute setting behavior to match other UAs
This is an attempt to bring Chrome's cookie behavior in line with the
majority of other major UAs, and thus are de facto standards. The
specific updates are:
1. An empty domain attribute value should be ignored as a
non-attribute rather than setting an empty attribute value.
2. An empty cookie line (or a cookie line with ignored characters)
should be treated as setting an empty value for the empty-key.
BUG=601786
==========
to
==========
Update cookie value and attribute setting behavior to match other UAs
This is an attempt to bring Chrome's cookie behavior in line with the
majority of other major UAs, and thus are de facto standards. The
specific updates are:
1. An empty domain attribute value should be ignored as a
non-attribute rather than setting an empty attribute value.
2. An empty cookie line (or a cookie line with ignored characters)
should be treated as setting an empty value for the empty-key.
BUG=601786
==========
I believe the cookie_store_unittest tests are run on iOS as well, using iOS's cookie store. Don't be surprised if we run into some issues there. https://codereview.chromium.org/2246613003/diff/20001/net/cookies/cookie_stor... File net/cookies/cookie_store_unittest.h (right): https://codereview.chromium.org/2246613003/diff/20001/net/cookies/cookie_stor... net/cookies/cookie_store_unittest.h:601: this->SetCookie(cs, url_foobar, "a=1; domain=.yo.foo.bar.com; domain=")); Why does this fail? Aren't we ignoring the "domain=" at the end? https://codereview.chromium.org/2246613003/diff/20001/net/cookies/parsed_cook... File net/cookies/parsed_cookie.cc (right): https://codereview.chromium.org/2246613003/diff/20001/net/cookies/parsed_cook... net/cookies/parsed_cookie.cc:370: pairs_.push_back(TokenValuePair("", "")); 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. https://codereview.chromium.org/2246613003/diff/20001/net/cookies/parsed_cook... net/cookies/parsed_cookie.cc:377: if (!did_parse_token && pair_num != 0) Think this is worth a comment (Just "Allow first token to be empty" is fine). https://codereview.chromium.org/2246613003/diff/20001/net/cookies/parsed_cook... net/cookies/parsed_cookie.cc:377: if (!did_parse_token && pair_num != 0) 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. https://codereview.chromium.org/2246613003/diff/20001/net/cookies/parsed_cook... net/cookies/parsed_cookie.cc:390: it = did_parse_token ? token_start : start; 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 != '=') { .... https://codereview.chromium.org/2246613003/diff/20001/net/cookies/parsed_cook... net/cookies/parsed_cookie.cc:435: } else if (pairs_[i].first == kDomainTokenName && pairs_[i].second != "") { 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). https://codereview.chromium.org/2246613003/diff/20001/net/cookies/parsed_cook... File net/cookies/parsed_cookie_unittest.cc (right): https://codereview.chromium.org/2246613003/diff/20001/net/cookies/parsed_cook... net/cookies/parsed_cookie_unittest.cc:36: EXPECT_TRUE(pc7.IsValid()); We should also check ";" and "=" (With no extra stuff on the end). https://codereview.chromium.org/2246613003/diff/20001/net/cookies/parsed_cook... net/cookies/parsed_cookie_unittest.cc:36: EXPECT_TRUE(pc7.IsValid()); We should check the actual value, too, right? Also should test that they have path and secure set (Except in the two cases I suggested adding) https://codereview.chromium.org/2246613003/diff/20001/net/cookies/parsed_cook... net/cookies/parsed_cookie_unittest.cc:36: EXPECT_TRUE(pc7.IsValid()); optional: Suggest merging ValidOnlyWhitespace and ValidEmpty with this test, too, since they're so closely related. https://codereview.chromium.org/2246613003/diff/20001/net/cookies/parsed_cook... net/cookies/parsed_cookie_unittest.cc:315: EXPECT_FALSE(pc.SetName(std::string())); Hrm...we allow unnamed cookies, but SetName doesn't work for them? Guess that's another issue.
Good call about the iOS tests; I look forward to fixing those failures :-P I didn't agree with all of your comments, so definitely let me know what you think. Thanks! https://codereview.chromium.org/2246613003/diff/20001/net/cookies/cookie_stor... File net/cookies/cookie_store_unittest.h (right): https://codereview.chromium.org/2246613003/diff/20001/net/cookies/cookie_stor... net/cookies/cookie_store_unittest.h:601: this->SetCookie(cs, url_foobar, "a=1; domain=.yo.foo.bar.com; domain=")); On 2016/08/15 21:11:03, mmenke wrote: > Why does this fail? Aren't we ignoring the "domain=" at the end? No, that's part of the issue and difference in behavior that we're changing. We had been treating this as setting the domain attribute to an empty value, so it used to succeed. Now, with my changes, the empty domain attribute will be ignored, so the first one will take precedence, and has an invalid value. https://codereview.chromium.org/2246613003/diff/20001/net/cookies/parsed_cook... File net/cookies/parsed_cookie.cc (right): https://codereview.chromium.org/2246613003/diff/20001/net/cookies/parsed_cook... net/cookies/parsed_cookie.cc:370: pairs_.push_back(TokenValuePair("", "")); On 2016/08/15 21:11:03, mmenke wrote: > 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. Early return sgtm. Done. https://codereview.chromium.org/2246613003/diff/20001/net/cookies/parsed_cook... net/cookies/parsed_cookie.cc:377: if (!did_parse_token && pair_num != 0) On 2016/08/15 21:11:03, mmenke wrote: > 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. 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. https://codereview.chromium.org/2246613003/diff/20001/net/cookies/parsed_cook... net/cookies/parsed_cookie.cc:390: it = did_parse_token ? token_start : start; On 2016/08/15 21:11:03, mmenke wrote: > 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 != '=') { > .... 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. https://codereview.chromium.org/2246613003/diff/20001/net/cookies/parsed_cook... net/cookies/parsed_cookie.cc:435: } else if (pairs_[i].first == kDomainTokenName && pairs_[i].second != "") { On 2016/08/15 21:11:03, mmenke wrote: > 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. 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). > > Unless this is really what all other browsers do (Both for domain and for > everything else). 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. https://codereview.chromium.org/2246613003/diff/20001/net/cookies/parsed_cook... File net/cookies/parsed_cookie_unittest.cc (right): https://codereview.chromium.org/2246613003/diff/20001/net/cookies/parsed_cook... net/cookies/parsed_cookie_unittest.cc:36: EXPECT_TRUE(pc7.IsValid()); On 2016/08/15 21:11:03, mmenke wrote: > optional: Suggest merging ValidOnlyWhitespace and ValidEmpty with this test, > too, since they're so closely related. Good calls all around. I've done all of your suggestions: * Added versions of all the tests without the attributes * Added value checking for each of them * Moved ValidOnlyWhitespace and ValidEmpty into TestEmpty https://codereview.chromium.org/2246613003/diff/20001/net/cookies/parsed_cook... net/cookies/parsed_cookie_unittest.cc:315: EXPECT_FALSE(pc.SetName(std::string())); On 2016/08/15 21:11:03, mmenke wrote: > Hrm...we allow unnamed cookies, but SetName doesn't work for them? Guess that's > another issue. Good catch! Indeed, that doesn't make sense, so I've fixed that and updated the tests.
Note that I'll want to see a green iOS try run before I sign off on anything. https://codereview.chromium.org/2246613003/diff/20001/net/cookies/cookie_stor... File net/cookies/cookie_store_unittest.h (right): https://codereview.chromium.org/2246613003/diff/20001/net/cookies/cookie_stor... net/cookies/cookie_store_unittest.h:601: this->SetCookie(cs, url_foobar, "a=1; domain=.yo.foo.bar.com; domain=")); On 2016/08/16 02:24:23, jww wrote: > On 2016/08/15 21:11:03, mmenke wrote: > > Why does this fail? Aren't we ignoring the "domain=" at the end? > > No, that's part of the issue and difference in behavior that we're changing. We > had been treating this as setting the domain attribute to an empty value, so it > used to succeed. Now, with my changes, the empty domain attribute will be > ignored, so the first one will take precedence, and has an invalid value. Oh, I missed that the previous line was also EXPECT_FALSE (I had thought it was EXPECT_TRUE, which was confusing me). https://codereview.chromium.org/2246613003/diff/20001/net/cookies/parsed_cook... File net/cookies/parsed_cookie.cc (right): https://codereview.chromium.org/2246613003/diff/20001/net/cookies/parsed_cook... net/cookies/parsed_cookie.cc:377: if (!did_parse_token && pair_num != 0) On 2016/08/16 02:24:23, jww wrote: > On 2016/08/15 21:11:03, mmenke wrote: > > 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. > > 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. 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. https://codereview.chromium.org/2246613003/diff/20001/net/cookies/parsed_cook... net/cookies/parsed_cookie.cc:435: } else if (pairs_[i].first == kDomainTokenName && pairs_[i].second != "") { On 2016/08/16 02:24:23, jww wrote: > On 2016/08/15 21:11:03, mmenke wrote: > > 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. > 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). > > > > Unless this is really what all other browsers do (Both for domain and for > > everything else). > 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. 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. https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... File net/cookies/parsed_cookie.cc (right): https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... net/cookies/parsed_cookie.cc:397: it = start; nit: Use braces in else/if. Also, another option is to, outside this big block, set token_start to start, if !did_parse_token, but not sure that's any better - does have the advantage of putting all code that checks did_parse_token in one place (And you could remove two of the did_parse_token checks). Anyhow, up to you. https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... File net/cookies/parsed_cookie_unittest.cc (right): https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... net/cookies/parsed_cookie_unittest.cc:23: struct { const https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... net/cookies/parsed_cookie_unittest.cc:27: } test_cookie_lines[]{ kTestCookieLines https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... net/cookies/parsed_cookie_unittest.cc:35: {"\t;", "", false}, {"\t; path=/; secure;", "/", true}}; Suggest a blank line, for readability. https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... net/cookies/parsed_cookie_unittest.cc:36: for (size_t i = 0; i < arraysize(test_cookie_lines); i++) { Can just do a range-based loop (Or does it require that we have a named type? If so, I'd just add a name for the type in the array) for (const auto& test : test_cookie_lines)
Understood re: iOS. Will run those tests now and will get back to you once they're green. https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... File net/cookies/parsed_cookie.cc (right): https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... net/cookies/parsed_cookie.cc:397: it = start; On 2016/08/16 15:06:47, mmenke wrote: > nit: Use braces in else/if. > > Also, another option is to, outside this big block, set token_start to start, if > !did_parse_token, but not sure that's any better - does have the advantage of > putting all code that checks did_parse_token in one place (And you could remove > two of the did_parse_token checks). Anyhow, up to you. Did both. https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... File net/cookies/parsed_cookie_unittest.cc (right): https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... net/cookies/parsed_cookie_unittest.cc:23: struct { On 2016/08/16 15:06:47, mmenke wrote: > const Done. https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... net/cookies/parsed_cookie_unittest.cc:27: } test_cookie_lines[]{ On 2016/08/16 15:06:47, mmenke wrote: > kTestCookieLines Done. https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... net/cookies/parsed_cookie_unittest.cc:35: {"\t;", "", false}, {"\t; path=/; secure;", "/", true}}; On 2016/08/16 15:06:47, mmenke wrote: > Suggest a blank line, for readability. Done. https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... net/cookies/parsed_cookie_unittest.cc:36: for (size_t i = 0; i < arraysize(test_cookie_lines); i++) { On 2016/08/16 15:06:47, mmenke wrote: > Can just do a range-based loop (Or does it require that we have a named type? > If so, I'd just add a name for the type in the array) > > for (const auto& test : test_cookie_lines) Done.
On 2016/08/16 18:18:17, jww wrote: > Understood re: iOS. Will run those tests now and will get back to you once > they're green. > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > File net/cookies/parsed_cookie.cc (right): > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > net/cookies/parsed_cookie.cc:397: it = start; > On 2016/08/16 15:06:47, mmenke wrote: > > nit: Use braces in else/if. > > > > Also, another option is to, outside this big block, set token_start to start, > if > > !did_parse_token, but not sure that's any better - does have the advantage of > > putting all code that checks did_parse_token in one place (And you could > remove > > two of the did_parse_token checks). Anyhow, up to you. > > Did both. > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > File net/cookies/parsed_cookie_unittest.cc (right): > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > net/cookies/parsed_cookie_unittest.cc:23: struct { > On 2016/08/16 15:06:47, mmenke wrote: > > const > > Done. > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > net/cookies/parsed_cookie_unittest.cc:27: } test_cookie_lines[]{ > On 2016/08/16 15:06:47, mmenke wrote: > > kTestCookieLines > > Done. > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > net/cookies/parsed_cookie_unittest.cc:35: {"\t;", "", false}, {"\t; path=/; > secure;", "/", true}}; > On 2016/08/16 15:06:47, mmenke wrote: > > Suggest a blank line, for readability. > > Done. > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > net/cookies/parsed_cookie_unittest.cc:36: for (size_t i = 0; i < > arraysize(test_cookie_lines); i++) { > On 2016/08/16 15:06:47, mmenke wrote: > > Can just do a range-based loop (Or does it require that we have a named type? > > If so, I'd just add a name for the type in the array) > > > > for (const auto& test : test_cookie_lines) > > Done. The iOS situation is more complicated than I thought it would be. Because we're not actually using our own cookline parser, and we're relying entirely on NSHTTPCookie, we're reliant on how iOS decides to parse cookies (see https://cs.chromium.org/chromium/src/ios/net/cookies/cookie_store_ios.mm?rcl=... for where this happens). And, unfortunately, OS disagrees with the majority of other browsers on several of these issues, for example, how to treat empty-key cookies (it ignores them, as per http://inikulin.github.io/cookie-compat/#NAME0028, which is technically spec-compliant, but is not what any other UA does). mmenke@, any suggestions on how to handle this? Should we try to hack our way around the iOS behavior by partially parsing our own cookies? Should we leave iOS as-is, and run a separate set of unit tests on it? Should we just give up on these changes?
On 2016/08/16 21:09:36, jww wrote: > On 2016/08/16 18:18:17, jww wrote: > > Understood re: iOS. Will run those tests now and will get back to you once > > they're green. > > > > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > > File net/cookies/parsed_cookie.cc (right): > > > > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > > net/cookies/parsed_cookie.cc:397: it = start; > > On 2016/08/16 15:06:47, mmenke wrote: > > > nit: Use braces in else/if. > > > > > > Also, another option is to, outside this big block, set token_start to > start, > > if > > > !did_parse_token, but not sure that's any better - does have the advantage > of > > > putting all code that checks did_parse_token in one place (And you could > > remove > > > two of the did_parse_token checks). Anyhow, up to you. > > > > Did both. > > > > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > > File net/cookies/parsed_cookie_unittest.cc (right): > > > > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > > net/cookies/parsed_cookie_unittest.cc:23: struct { > > On 2016/08/16 15:06:47, mmenke wrote: > > > const > > > > Done. > > > > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > > net/cookies/parsed_cookie_unittest.cc:27: } test_cookie_lines[]{ > > On 2016/08/16 15:06:47, mmenke wrote: > > > kTestCookieLines > > > > Done. > > > > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > > net/cookies/parsed_cookie_unittest.cc:35: {"\t;", "", false}, {"\t; path=/; > > secure;", "/", true}}; > > On 2016/08/16 15:06:47, mmenke wrote: > > > Suggest a blank line, for readability. > > > > Done. > > > > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > > net/cookies/parsed_cookie_unittest.cc:36: for (size_t i = 0; i < > > arraysize(test_cookie_lines); i++) { > > On 2016/08/16 15:06:47, mmenke wrote: > > > Can just do a range-based loop (Or does it require that we have a named > type? > > > If so, I'd just add a name for the type in the array) > > > > > > for (const auto& test : test_cookie_lines) > > > > Done. > > The iOS situation is more complicated than I thought it would be. Because we're > not actually using our own cookline parser, and we're relying entirely on > NSHTTPCookie, we're reliant on how iOS decides to parse cookies (see > https://cs.chromium.org/chromium/src/ios/net/cookies/cookie_store_ios.mm?rcl=... > for where this happens). And, unfortunately, OS disagrees with the majority of > other browsers on several of these issues, for example, how to treat empty-key > cookies (it ignores them, as per > http://inikulin.github.io/cookie-compat/#NAME0028, which is technically > spec-compliant, but is not what any other UA does). > > mmenke@, any suggestions on how to handle this? Should we try to hack our way > around the iOS behavior by partially parsing our own cookies? Should we leave > iOS as-is, and run a separate set of unit tests on it? Should we just give up on > these changes? I'd say just disable the tests on iOS, and file a bug to reference, so there's a record of iOS being borked.
On 2016/08/16 21:12:53, mmenke wrote: > On 2016/08/16 21:09:36, jww wrote: > > On 2016/08/16 18:18:17, jww wrote: > > > Understood re: iOS. Will run those tests now and will get back to you once > > > they're green. > > > > > > > > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > > > File net/cookies/parsed_cookie.cc (right): > > > > > > > > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > > > net/cookies/parsed_cookie.cc:397: it = start; > > > On 2016/08/16 15:06:47, mmenke wrote: > > > > nit: Use braces in else/if. > > > > > > > > Also, another option is to, outside this big block, set token_start to > > start, > > > if > > > > !did_parse_token, but not sure that's any better - does have the advantage > > of > > > > putting all code that checks did_parse_token in one place (And you could > > > remove > > > > two of the did_parse_token checks). Anyhow, up to you. > > > > > > Did both. > > > > > > > > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > > > File net/cookies/parsed_cookie_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > > > net/cookies/parsed_cookie_unittest.cc:23: struct { > > > On 2016/08/16 15:06:47, mmenke wrote: > > > > const > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > > > net/cookies/parsed_cookie_unittest.cc:27: } test_cookie_lines[]{ > > > On 2016/08/16 15:06:47, mmenke wrote: > > > > kTestCookieLines > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > > > net/cookies/parsed_cookie_unittest.cc:35: {"\t;", "", false}, {"\t; path=/; > > > secure;", "/", true}}; > > > On 2016/08/16 15:06:47, mmenke wrote: > > > > Suggest a blank line, for readability. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > > > net/cookies/parsed_cookie_unittest.cc:36: for (size_t i = 0; i < > > > arraysize(test_cookie_lines); i++) { > > > On 2016/08/16 15:06:47, mmenke wrote: > > > > Can just do a range-based loop (Or does it require that we have a named > > type? > > > > If so, I'd just add a name for the type in the array) > > > > > > > > for (const auto& test : test_cookie_lines) > > > > > > Done. > > > > The iOS situation is more complicated than I thought it would be. Because > we're > > not actually using our own cookline parser, and we're relying entirely on > > NSHTTPCookie, we're reliant on how iOS decides to parse cookies (see > > > https://cs.chromium.org/chromium/src/ios/net/cookies/cookie_store_ios.mm?rcl=... > > for where this happens). And, unfortunately, OS disagrees with the majority of > > other browsers on several of these issues, for example, how to treat empty-key > > cookies (it ignores them, as per > > http://inikulin.github.io/cookie-compat/#NAME0028, which is technically > > spec-compliant, but is not what any other UA does). > > > > mmenke@, any suggestions on how to handle this? Should we try to hack our way > > around the iOS behavior by partially parsing our own cookies? Should we leave > > iOS as-is, and run a separate set of unit tests on it? Should we just give up > on > > these changes? > > I'd say just disable the tests on iOS, and file a bug to reference, so there's a > record of iOS being borked. Sounds good.
On 2016/08/16 21:53:37, jww wrote: > On 2016/08/16 21:12:53, mmenke wrote: > > On 2016/08/16 21:09:36, jww wrote: > > > On 2016/08/16 18:18:17, jww wrote: > > > > Understood re: iOS. Will run those tests now and will get back to you once > > > > they're green. > > > > > > > > > > > > > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > > > > File net/cookies/parsed_cookie.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > > > > net/cookies/parsed_cookie.cc:397: it = start; > > > > On 2016/08/16 15:06:47, mmenke wrote: > > > > > nit: Use braces in else/if. > > > > > > > > > > Also, another option is to, outside this big block, set token_start to > > > start, > > > > if > > > > > !did_parse_token, but not sure that's any better - does have the > advantage > > > of > > > > > putting all code that checks did_parse_token in one place (And you could > > > > remove > > > > > two of the did_parse_token checks). Anyhow, up to you. > > > > > > > > Did both. > > > > > > > > > > > > > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > > > > File net/cookies/parsed_cookie_unittest.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > > > > net/cookies/parsed_cookie_unittest.cc:23: struct { > > > > On 2016/08/16 15:06:47, mmenke wrote: > > > > > const > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > > > > net/cookies/parsed_cookie_unittest.cc:27: } test_cookie_lines[]{ > > > > On 2016/08/16 15:06:47, mmenke wrote: > > > > > kTestCookieLines > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > > > > net/cookies/parsed_cookie_unittest.cc:35: {"\t;", "", false}, {"\t; > path=/; > > > > secure;", "/", true}}; > > > > On 2016/08/16 15:06:47, mmenke wrote: > > > > > Suggest a blank line, for readability. > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/2246613003/diff/40001/net/cookies/parsed_cook... > > > > net/cookies/parsed_cookie_unittest.cc:36: for (size_t i = 0; i < > > > > arraysize(test_cookie_lines); i++) { > > > > On 2016/08/16 15:06:47, mmenke wrote: > > > > > Can just do a range-based loop (Or does it require that we have a named > > > type? > > > > > If so, I'd just add a name for the type in the array) > > > > > > > > > > for (const auto& test : test_cookie_lines) > > > > > > > > Done. > > > > > > The iOS situation is more complicated than I thought it would be. Because > > we're > > > not actually using our own cookline parser, and we're relying entirely on > > > NSHTTPCookie, we're reliant on how iOS decides to parse cookies (see > > > > > > https://cs.chromium.org/chromium/src/ios/net/cookies/cookie_store_ios.mm?rcl=... > > > for where this happens). And, unfortunately, OS disagrees with the majority > of > > > other browsers on several of these issues, for example, how to treat > empty-key > > > cookies (it ignores them, as per > > > http://inikulin.github.io/cookie-compat/#NAME0028, which is technically > > > spec-compliant, but is not what any other UA does). > > > > > > mmenke@, any suggestions on how to handle this? Should we try to hack our > way > > > around the iOS behavior by partially parsing our own cookies? Should we > leave > > > iOS as-is, and run a separate set of unit tests on it? Should we just give > up > > on > > > these changes? > > > > I'd say just disable the tests on iOS, and file a bug to reference, so there's > a > > record of iOS being borked. > > Sounds good. mmenke@, looks like tests are passing, so let me know how it looks. Thanks!
The CQ bit was checked by jww@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Still also skeptical that the empty string check for the domain (And *only* the domain) is correct. https://codereview.chromium.org/2246613003/diff/80001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_cookie_helper_unittest.cc (right): https://codereview.chromium.org/2246613003/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_cookie_helper_unittest.cc:289: helper->AddChangedCookie(origin, origin, "C=not http; HttpOnly", Why is this an invalid cookie? I'm not seeing why these are treated as non-HTTP cookies. https://codereview.chromium.org/2246613003/diff/80001/net/cookies/cookie_stor... File net/cookies/cookie_store_unittest.h (right): https://codereview.chromium.org/2246613003/diff/80001/net/cookies/cookie_stor... net/cookies/cookie_store_unittest.h:1422: #endif Looks like Windows doesn't like this (I guess I don't really blame it). May have to just make the test exist, but do nothing, on iOS. https://codereview.chromium.org/2246613003/diff/80001/net/cookies/parsed_cook... File net/cookies/parsed_cookie.cc (right): https://codereview.chromium.org/2246613003/diff/80001/net/cookies/parsed_cook... net/cookies/parsed_cookie.cc:388: if (!did_parse_token || it == end || *it != '=') { Hrm..!did_parse_token isn't needed here...Though suppose it may be clearer to keep it? Not sure.
FWIW, regarding the empty string for domain, whether an attribute should accept the empty string is already case-by-case according to the spec (https://tools.ietf.org/html/rfc6265#section-4.1.1). For example, domain and max-age should not, syntactically, allow empty strings, while path should. On the other hand, I'm not adding anything special for max-age because I believe Chrome's behavior matches the other UAs at this point, and this is pragmatically meant to match their behavior. Perhaps we should file a bug to confirm what other UAs do for other attributes? https://codereview.chromium.org/2246613003/diff/80001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_cookie_helper_unittest.cc (right): https://codereview.chromium.org/2246613003/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_cookie_helper_unittest.cc:289: helper->AddChangedCookie(origin, origin, "C=not http; HttpOnly", On 2016/08/17 16:23:17, mmenke wrote: > Why is this an invalid cookie? I'm not seeing why these are treated as non-HTTP > cookies. To accept HTTP-only cookies, you have to set a special net::CookieOption, which by default is set to reject HTTP-only cookies. So in this case, we didn't set that option (see like 285), so we expect HttpOnly cookies to be rejected. https://codereview.chromium.org/2246613003/diff/80001/net/cookies/cookie_stor... File net/cookies/cookie_store_unittest.h (right): https://codereview.chromium.org/2246613003/diff/80001/net/cookies/cookie_stor... net/cookies/cookie_store_unittest.h:1422: #endif On 2016/08/17 16:23:17, mmenke wrote: > Looks like Windows doesn't like this (I guess I don't really blame it). May > have to just make the test exist, but do nothing, on iOS. Done. https://codereview.chromium.org/2246613003/diff/80001/net/cookies/parsed_cook... File net/cookies/parsed_cookie.cc (right): https://codereview.chromium.org/2246613003/diff/80001/net/cookies/parsed_cook... net/cookies/parsed_cookie.cc:388: if (!did_parse_token || it == end || *it != '=') { On 2016/08/17 16:23:17, mmenke wrote: > Hrm..!did_parse_token isn't needed here...Though suppose it may be clearer to > keep it? Not sure. I believe it is needed. The prior if-block only breaks if pair_num != 0, so !did_parse_token can still be true if pair_num == 0.
LGTM https://codereview.chromium.org/2246613003/diff/80001/net/cookies/parsed_cook... File net/cookies/parsed_cookie.cc (right): https://codereview.chromium.org/2246613003/diff/80001/net/cookies/parsed_cook... net/cookies/parsed_cookie.cc:388: if (!did_parse_token || it == end || *it != '=') { On 2016/08/17 16:57:08, jww wrote: > On 2016/08/17 16:23:17, mmenke wrote: > > Hrm..!did_parse_token isn't needed here...Though suppose it may be clearer to > > keep it? Not sure. > > I believe it is needed. The prior if-block only breaks if pair_num != 0, so > !did_parse_token can still be true if pair_num == 0. Right, but then it == end. The check for whether ParseToken should return false actually explicitly checks if it == end. More intuitively, it ParseToken returns false, the string doesn't have "=" anywhere in it, so *it can't be "=".
https://codereview.chromium.org/2246613003/diff/80001/net/cookies/parsed_cook... File net/cookies/parsed_cookie.cc (right): https://codereview.chromium.org/2246613003/diff/80001/net/cookies/parsed_cook... net/cookies/parsed_cookie.cc:388: if (!did_parse_token || it == end || *it != '=') { On 2016/08/17 17:56:33, mmenke wrote: > On 2016/08/17 16:57:08, jww wrote: > > On 2016/08/17 16:23:17, mmenke wrote: > > > Hrm..!did_parse_token isn't needed here...Though suppose it may be clearer > to > > > keep it? Not sure. > > > > I believe it is needed. The prior if-block only breaks if pair_num != 0, so > > !did_parse_token can still be true if pair_num == 0. > > Right, but then it == end. The check for whether ParseToken should return false > actually explicitly checks if it == end. More intuitively, it ParseToken > returns false, the string doesn't have "=" anywhere in it, so *it can't be "=". Ah, great point! I'll actually simplify some of this, then.
jww@chromium.org changed reviewers: + thestig@chromium.org
thestig@, would you mind OWNER reviewing browsing_data?
lgtm
The CQ bit was checked by jww@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2246613003/#ps140001 (title: "Simplification")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
Update cookie value and attribute setting behavior to match other UAs
This is an attempt to bring Chrome's cookie behavior in line with the
majority of other major UAs, and thus are de facto standards. The
specific updates are:
1. An empty domain attribute value should be ignored as a
non-attribute rather than setting an empty attribute value.
2. An empty cookie line (or a cookie line with ignored characters)
should be treated as setting an empty value for the empty-key.
BUG=601786
==========
to
==========
Update cookie value and attribute setting behavior to match other UAs
This is an attempt to bring Chrome's cookie behavior in line with the
majority of other major UAs, and thus are de facto standards. The
specific updates are:
1. An empty domain attribute value should be ignored as a
non-attribute rather than setting an empty attribute value.
2. An empty cookie line (or a cookie line with ignored characters)
should be treated as setting an empty value for the empty-key.
BUG=601786
==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from
==========
Update cookie value and attribute setting behavior to match other UAs
This is an attempt to bring Chrome's cookie behavior in line with the
majority of other major UAs, and thus are de facto standards. The
specific updates are:
1. An empty domain attribute value should be ignored as a
non-attribute rather than setting an empty attribute value.
2. An empty cookie line (or a cookie line with ignored characters)
should be treated as setting an empty value for the empty-key.
BUG=601786
==========
to
==========
Update cookie value and attribute setting behavior to match other UAs
This is an attempt to bring Chrome's cookie behavior in line with the
majority of other major UAs, and thus are de facto standards. The
specific updates are:
1. An empty domain attribute value should be ignored as a
non-attribute rather than setting an empty attribute value.
2. An empty cookie line (or a cookie line with ignored characters)
should be treated as setting an empty value for the empty-key.
BUG=601786
Committed: https://crrev.com/03e6ff8c614040f60d1a1d818411870ddb3b90e4
Cr-Commit-Position: refs/heads/master@{#412608}
==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/03e6ff8c614040f60d1a1d818411870ddb3b90e4 Cr-Commit-Position: refs/heads/master@{#412608} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
