Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2898953008:
Implement and test CanonicalCookie::IsCanonical() (Closed)
|
Created:
3 years, 6 months ago by Randy Smith (Not in Mondays) Modified:
3 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement and test CanonicalCookie::IsCanonical()
This is done for data validation including but not limited to the
cookies servicification effort.
BUG=721395, 723734
R=mmenke@chromium.org
Review-Url: https://codereview.chromium.org/2898953008
Cr-Commit-Position: refs/heads/master@{#480238}
Committed: https://chromium.googlesource.com/chromium/src/+/1e64af8eed07578f01b933d654180659c0fe962c
Patch Set 1 #
Total comments: 13
Patch Set 2 : Sync'd to p477328. #Patch Set 3 : Incorporated comments. #Patch Set 4 : Conditionalize correctly on TypeParam for IOS tests. #Patch Set 5 : Added tweaks to handle empty name behavior differences on iOS. #Patch Set 6 : Add test param to Android Webview cookie test. #
Total comments: 38
Patch Set 7 : Incorporated comments. #Patch Set 8 : Defined IsCanonical() in terms of CC::Create(). #Patch Set 9 : Sync'd to p497116. #
Total comments: 1
Patch Set 10 : Remove leftover logging. #
Total comments: 6
Patch Set 11 : Incorporated comments. #
Total comments: 9
Patch Set 12 : Next round of comments incorporated. #Patch Set 13 : Removed vestigial code. #
Dependent Patchsets: Messages
Total messages: 80 (44 generated)
The CQ bit was checked by rdsmith@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...
Matt: When I engaged with the question of how to slice https://codereview.chromium.org/2882063002, I realized that it most reasonably sliced into four different CLs: Adding CanonicalCookie::IsCanonical() (this one), the secure/http_only refactor you mentioned, fixes to sqlite, and adding the CM method. I decided that the effort to actually slice that thin given that I was going to be breaking it apart wasn't particular worse for four than two, so I went for four. Hopefully all of these will be easier to review.
On 2017/05/25 19:38:10, Randy Smith (Not in Mondays) wrote: > Matt: When I engaged with the question of how to slice > https://codereview.chromium.org/2882063002, I realized that it most reasonably > sliced into four different CLs: Adding CanonicalCookie::IsCanonical() (this > one), the secure/http_only refactor you mentioned, fixes to sqlite, and adding > the CM method. I decided that the effort to actually slice that thin given that > I was going to be breaking it apart wasn't particular worse for four than two, > so I went for four. Hopefully all of these will be easier to review. Awesome! Thanks so much.
https://codereview.chromium.org/2898953008/diff/1/net/cookies/canonical_cooki... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/2898953008/diff/1/net/cookies/canonical_cooki... net/cookies/canonical_cookie.cc:404: bool CanonicalCookie::IsCanonical() const { Is there a single method I should be comparing this logic to? https://codereview.chromium.org/2898953008/diff/1/net/cookies/canonical_cooki... net/cookies/canonical_cookie.cc:405: if (name_.empty()) Don't we support "Set-Cookie: =foo" and "Set-Cookie: foo", which both do the same thing? https://codereview.chromium.org/2898953008/diff/1/net/cookies/canonical_cooki... net/cookies/canonical_cookie.cc:417: CanonicalizeHost(domain_, &ignored).empty()) { What if CanonicalizeHost(domain_) != domain_? https://codereview.chromium.org/2898953008/diff/1/net/cookies/canonical_cooki... net/cookies/canonical_cookie.cc:422: return false; Do we always set path to be non-empty, even when none is provided? Regardless, this should at least be (path_.empty() || path_[0] != '/')
https://codereview.chromium.org/2898953008/diff/1/net/cookies/canonical_cooki... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/2898953008/diff/1/net/cookies/canonical_cooki... net/cookies/canonical_cookie.cc:405: if (name_.empty()) On 2017/05/25 19:49:56, mmenke wrote: > Don't we support "Set-Cookie: =foo" and "Set-Cookie: foo", which both do the > same thing? Fun table that indicates we support both of these: http://inikulin.github.io/cookie-compat/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2898953008/diff/1/net/cookies/canonical_cookie.h File net/cookies/canonical_cookie.h (right): https://codereview.chromium.org/2898953008/diff/1/net/cookies/canonical_cooki... net/cookies/canonical_cookie.h:149: bool IsCanonical() const; We should DCHECK on this at the end of CanonicalCookie::Create (Since it should only be returning canonical cookies). That should hopefully help us catch most false negatives, though doesn't help much with potential false positives... Maybe we could create a fuzzer for that? Call CanonicalCookie::Create, and if it fails, manually construct a CanonicalCookie with the unsafe constructor that looks much like what Create would have returned on success, and make sure it's not canonical?
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Incorporated comments; PTAL. https://codereview.chromium.org/2898953008/diff/1/net/cookies/canonical_cooki... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/2898953008/diff/1/net/cookies/canonical_cooki... net/cookies/canonical_cookie.cc:404: bool CanonicalCookie::IsCanonical() const { On 2017/05/25 19:49:56, mmenke wrote: > Is there a single method I should be comparing this logic to? I modeled it after SetCookieWithDetails minus the checks against URL. https://codereview.chromium.org/2898953008/diff/1/net/cookies/canonical_cooki... net/cookies/canonical_cookie.cc:405: if (name_.empty()) On 2017/05/25 21:25:21, mmenke wrote: > On 2017/05/25 19:49:56, mmenke wrote: > > Don't we support "Set-Cookie: =foo" and "Set-Cookie: foo", which both do the > > same thing? > > Fun table that indicates we support both of these: > http://inikulin.github.io/cookie-compat/ Nice catch; my bad. This snuck in a couple of CLs ago (see DCHECKs added in CanonicalCookie::Create in https://codereview.chromium.org/2861063003) and caused http://crbug.com/723734. I've removed the check above and add some tests to make sure this remains true, at least until we explicitly try to change the behavior. https://codereview.chromium.org/2898953008/diff/1/net/cookies/canonical_cooki... net/cookies/canonical_cookie.cc:417: CanonicalizeHost(domain_, &ignored).empty()) { On 2017/05/25 19:49:56, mmenke wrote: > What if CanonicalizeHost(domain_) != domain_? Oh, tricky. So as noted above I'm copying this logic from SetCookieWithDetails and subtracting out the verification against the URL. SetCookieWithDetails (in GetCookieDomainWithSring()) has the above check, and then just adopts the domain returned by CanonicalizeHost() (prefixed with a "." if is isn't already there :-} :-J). I think modifying the incoming domain is incompatible with the sense of the CC constructor API. So I think I agree with you that CanonicalizeHost(domain_) != domain_ should be a failure from this function. But that's depending on a property of CanonicalizeHost() we haven't really depended on before (loosely its idempotence). I think I'll put that in, and presume that if there's a problem it'll show up in testing, but if you think of any gotchas in this space, please raise a flag. Done. https://codereview.chromium.org/2898953008/diff/1/net/cookies/canonical_cooki... net/cookies/canonical_cookie.cc:417: CanonicalizeHost(domain_, &ignored).empty()) { Also note that the DCHECK(IsCanonical()) you suggested for CC::Create turned up the fact that we were (in the case of strange schemes, specifically "foo:" in the tests) creating cookies with empty domains. I went back and read SetCookieWithDetails() more carefully and realized that it actually *allowed* empty domains despite the line "if (domain.empty()) return false;" part way through :-J. I've filed http://crbug.com/730633 to track the WTF aspects of this, and modified the logic above to allow empty domains. https://codereview.chromium.org/2898953008/diff/1/net/cookies/canonical_cooki... net/cookies/canonical_cookie.cc:422: return false; On 2017/05/25 19:49:56, mmenke wrote: > Do we always set path to be non-empty, even when none is provided? Yes, at least at a couple of places in the stack--see CanonicalCookie::CanonPathWithString. > Regardless, this should at least be (path_.empty() || path_[0] != '/') Right! Done. https://codereview.chromium.org/2898953008/diff/1/net/cookies/canonical_cookie.h File net/cookies/canonical_cookie.h (right): https://codereview.chromium.org/2898953008/diff/1/net/cookies/canonical_cooki... net/cookies/canonical_cookie.h:149: bool IsCanonical() const; On 2017/05/26 17:02:17, mmenke wrote: > We should DCHECK on this at the end of CanonicalCookie::Create (Since it should > only be returning canonical cookies). That should hopefully help us catch most > false negatives, though doesn't help much with potential false positives... > Maybe we could create a fuzzer for that? Call CanonicalCookie::Create, and if > it fails, manually construct a CanonicalCookie with the unsafe constructor that > looks much like what Create would have returned on success, and make sure it's > not canonical? DCHECK added. I agree that a fuzzer might be a way to catch false positives, but that would be a fair amount of work. How important do you think it is? (I tried to add some tests for CC::Create paralleling the ones for IsCanonical(), but the parsing of the cookie line defeated my attempts.)
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rdsmith@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2898953008/diff/1/net/cookies/canonical_cookie.h File net/cookies/canonical_cookie.h (right): https://codereview.chromium.org/2898953008/diff/1/net/cookies/canonical_cooki... net/cookies/canonical_cookie.h:149: bool IsCanonical() const; On 2017/06/07 23:31:40, Randy Smith (Not in Mondays) wrote: > On 2017/05/26 17:02:17, mmenke wrote: > > We should DCHECK on this at the end of CanonicalCookie::Create (Since it > should > > only be returning canonical cookies). That should hopefully help us catch > most > > false negatives, though doesn't help much with potential false positives... > > Maybe we could create a fuzzer for that? Call CanonicalCookie::Create, and if > > it fails, manually construct a CanonicalCookie with the unsafe constructor > that > > looks much like what Create would have returned on success, and make sure it's > > not canonical? > > DCHECK added. I agree that a fuzzer might be a way to catch false positives, > but that would be a fair amount of work. How important do you think it is? > > (I tried to add some tests for CC::Create paralleling the ones for > IsCanonical(), but the parsing of the cookie line defeated my attempts.) So I'm very shaky on the accuracy of this function. I assume the intention is to validate things the network process receives, which is why I want some way to automatically validate that this is actually correct. My opinion is we either need to add automatic validation (DCHECKs, fuzzers, or something), or I'm not going to be able to sign off on this without a fair bit of digging. At which point, I'll probably also once again suggest we also validate it with DCHECKs, though I may have better informed suggestions on how to do that.
On 2017/06/08 18:00:23, mmenke wrote: > https://codereview.chromium.org/2898953008/diff/1/net/cookies/canonical_cookie.h > File net/cookies/canonical_cookie.h (right): > > https://codereview.chromium.org/2898953008/diff/1/net/cookies/canonical_cooki... > net/cookies/canonical_cookie.h:149: bool IsCanonical() const; > On 2017/06/07 23:31:40, Randy Smith (Not in Mondays) wrote: > > On 2017/05/26 17:02:17, mmenke wrote: > > > We should DCHECK on this at the end of CanonicalCookie::Create (Since it > > should > > > only be returning canonical cookies). That should hopefully help us catch > > most > > > false negatives, though doesn't help much with potential false positives... > > > Maybe we could create a fuzzer for that? Call CanonicalCookie::Create, and > if > > > it fails, manually construct a CanonicalCookie with the unsafe constructor > > that > > > looks much like what Create would have returned on success, and make sure > it's > > > not canonical? > > > > DCHECK added. I agree that a fuzzer might be a way to catch false positives, > > but that would be a fair amount of work. How important do you think it is? > > > > (I tried to add some tests for CC::Create paralleling the ones for > > IsCanonical(), but the parsing of the cookie line defeated my attempts.) > > So I'm very shaky on the accuracy of this function. > > I assume the intention is to validate things the network process receives, which > is why I want some way to automatically validate that this is actually correct. > > My opinion is we either need to add automatic validation (DCHECKs, fuzzers, or > something), or I'm not going to be able to sign off on this without a fair bit > of digging. At which point, I'll probably also once again suggest we also > validate it with DCHECKs, though I may have better informed suggestions on how > to do that. Ahh, you added the DCHECK on construction, didn't realize that. So that seems to address my biggest concern. There may still be things it falsely claims are canonical. I want to think a bit about that before signing off.
The CQ bit was checked by rdsmith@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by rdsmith@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: This issue passed the CQ dry run.
rdsmith@chromium.org changed reviewers: + droger@chromium.org
David, would you review the PS 4->5 diff?
lgtm
A Mojo cookie hookup to the renderer process will doubless need a security review, once we're actually using this logic. https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:243: std::unique_ptr<CanonicalCookie> cc(base::WrapUnique(new CanonicalCookie( base::MakeUnique (pre-existing issue) https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:248: DCHECK(cc->IsCanonical()); Random comment: Come to think of it, we do have some fuzzer coverage of this DCHECK, in the URLRequest fuzzer, though it can't set secure or extension cookies. https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:410: ParsedCookie::ParseValueString(path_) != path_) { I don't think this is enough. In particular, disallow CRs/LFs and embedded NULLs, which all seems like they could be rather problematic. What happens if someone tries to set such cookies via document.cookie? https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:419: // the absence of a domain attribute), but are needed for chrome are -> is. Also, should Chrome be capitalized, or be chrome/ instead? Doesn't really matter. https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:421: if (!url::HostIsIPAddress(domain_) && !domain_.empty() && instead of url::HostIsIPAddress, can't we just grab this from CanonHostInfo::IsIPAddress(), to save some work? https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:422: canonical_domain != domain_) { If the URL is an IP address, shouldn't canonical_domain == domain_? My only concern here is domains that can be expressed in multiple ways. i.e. [::0] is the same as [0::0] or [0:0:0:0:0:0:0:0:0] (Plus some :0's). Just because domain_ is an IP address doesn't mean it can necessarily be part of a canonical cookie. https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:426: if (path_.empty() || path_[0] != '/') Hrm...I guess we do nothing about invalid characters anywhere in the cookie code, unless I'm missing something? So we depend on the site to do any normalization when they specify a path (i.e., escape rules, like " " -> "%20) https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:429: return true; What if there's no creation time, or the creation time is far in the future? The main concern is that may much with GC, or that a compromised renderer could create a forever cookie, that will never expire, due to always passing the 30-day checks. Of course, just dropping them could cause problems if the clock changes, or clocks are off. Are there cases where consumers really want to set a cookie with a weird creation time? If not, can just have the API to set a CanonicalCookie overwrite the time itself. I'm less concerned about funky expiration times, since that's what GC is for, and that's based mostly on creation time. We'll presumably want to overwrite last_access_data. The only other fields that look to be of potential concern when set by a compromised renderer are domain and secure, but those can already be set be such a renderer, so that's nothing new. https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... File net/cookies/canonical_cookie_unittest.cc (right): https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie_unittest.cc:600: EXPECT_TRUE(CanonicalCookie("A", "B", "x.y", "/path", base::Time(), Some of these should use IP addresses, both v4 and v6 (I think the v6 format used by the cookie store/url::CanonicalHost includes braces?) https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie_unittest.cc:600: EXPECT_TRUE(CanonicalCookie("A", "B", "x.y", "/path", base::Time(), Also, if we choose to anything about the base last-access times, should test that, too. https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie_unittest.cc:610: EXPECT_FALSE(CanonicalCookie("A ", "B", "x.y", "/path", base::Time(), I think these each need comments - fixuring out which field is wrong is a bit of a where's waldo search.
https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:410: ParsedCookie::ParseValueString(path_) != path_) { On 2017/06/09 18:25:49, mmenke wrote: > I don't think this is enough. > > In particular, disallow CRs/LFs and embedded NULLs, which all seems like they > could be rather problematic. What happens if someone tries to set such cookies > via document.cookie? Also, may be worth testing if there are any paths where we let web platform consumers set cookies with semi-colons or commas (or quotes?) in cookie name or value strings.
Completely agreed about the security review for the renderer->Mojo switch, but that's several CLs down the pike. Even the CL that creates the mojo service won't shift the renderer over, and that's the furthest one I've started working on yet. https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:243: std::unique_ptr<CanonicalCookie> cc(base::WrapUnique(new CanonicalCookie( On 2017/06/09 18:25:49, mmenke wrote: > base::MakeUnique (pre-existing issue) Done. https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:248: DCHECK(cc->IsCanonical()); On 2017/06/09 18:25:49, mmenke wrote: > Random comment: Come to think of it, we do have some fuzzer coverage of this > DCHECK, in the URLRequest fuzzer, though it can't set secure or extension > cookies. Acknowledged. https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:410: ParsedCookie::ParseValueString(path_) != path_) { On 2017/06/09 18:25:49, mmenke wrote: > I don't think this is enough. > > In particular, disallow CRs/LFs and embedded NULLs, which all seems like they > could be rather problematic. What happens if someone tries to set such cookies > via document.cookie? Sorry, could you say a bit more? I may be confused, but it looks to me as if ParseTokenString(x) will indeed return something that != x if x contains CRs, LFs, or nulls. I've added a couple of tests in cc_unittest.cc to confirm this, and cookies constructed with those characters in the name aren't canonical. https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:410: ParsedCookie::ParseValueString(path_) != path_) { On 2017/06/09 19:00:47, mmenke wrote: > On 2017/06/09 18:25:49, mmenke wrote: > > I don't think this is enough. > > > > In particular, disallow CRs/LFs and embedded NULLs, which all seems like they > > could be rather problematic. What happens if someone tries to set such > cookies > > via document.cookie? > > Also, may be worth testing if there are any paths where we let web platform > consumers set cookies with semi-colons or commas (or quotes?) in cookie name or > value strings. See comments elsewhere about currently web platform consumers (Set-Cookie: header and document.cookie) using SetCookieWithOptions() and hence going through parsing. But I'm not sure what you mean by "testing"? How would one test that? https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:419: // the absence of a domain attribute), but are needed for chrome On 2017/06/09 18:25:49, mmenke wrote: > are -> is. Also, should Chrome be capitalized, or be chrome/ instead? Doesn't > really matter. Done. https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:421: if (!url::HostIsIPAddress(domain_) && !domain_.empty() && On 2017/06/09 18:25:49, mmenke wrote: > instead of url::HostIsIPAddress, can't we just grab this from > CanonHostInfo::IsIPAddress(), to save some work? Done. https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:422: canonical_domain != domain_) { On 2017/06/09 18:25:49, mmenke wrote: > If the URL is an IP address, shouldn't canonical_domain == domain_? > > My only concern here is domains that can be expressed in multiple ways. i.e. > [::0] is the same as [0::0] or [0:0:0:0:0:0:0:0:0] (Plus some :0's). Just > because domain_ is an IP address doesn't mean it can necessarily be part of a > canonical cookie. Is the issue what "canonical" means, i.e. that you could have two equivalent hosts that are IP addresses and represented differently, and hence they wouldn't look like the same cookie? If so, yes, that's an issue, but it's an issue that exists in the current code, and I'm not particularly inclined to shave the yak unless you think it's specifically important? https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:426: if (path_.empty() || path_[0] != '/') On 2017/06/09 18:25:49, mmenke wrote: > Hrm...I guess we do nothing about invalid characters anywhere in the cookie > code, unless I'm missing something? So we depend on the site to do any > normalization when they specify a path (i.e., escape rules, like " " -> "%20) That sounds right to me, though if there's anything this exercise has taught me it's that I'm not very well grounded in all the nuances of the cookie code. There's some protection on the path through which web sites set cookies (because it uses SetCookieWithOptions(), which means the cookie code is parsing the cookie line, and that parsing presumably doesn't something about invalid characters). And direct renderer setting of cookies uses the same interface (as makes sense for setting through document.cookies). Which means that we're going to need to add that validation before we switch the renderer pathway over to Mojo, since currently the parsing's done in the browser and that'll move it to the renderer. I'd rather do that in a separate CL (so this is pretty purely a refactor of what we currently have), but I'll take a note that it'll need to be done. https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:429: return true; On 2017/06/09 18:25:49, mmenke wrote: > What if there's no creation time, or the creation time is far in the future? > The main concern is that may much with GC, or that a compromised renderer could > create a forever cookie, that will never expire, due to always passing the > 30-day checks. As noted above, I'd like to put in the Mojo-required renderer checks in a separate CL that's more closely associated with the shift to mojo, and leave this as being just a refactor of/based on the current code. Is that reasonable, or do you think that this CL is incomplete without those checks? > Of course, just dropping them could cause problems if the clock changes, or > clocks are off. Are there cases where consumers really want to set a cookie > with a weird creation time? If not, can just have the API to set a > CanonicalCookie overwrite the time itself. I'm less concerned about funky > expiration times, since that's what GC is for, and that's based mostly on > creation time. > > We'll presumably want to overwrite last_access_data. The only other fields that > look to be of potential concern when set by a compromised renderer are domain > and secure, but those can already be set be such a renderer, so that's nothing > new. Some of the issues you're raising are handled in https://codereview.chromium.org/2882063002; specifically, in that CL a null creation time is taken as "set the creation time when you push the cookie into the monster". (And CC already has a method for overriding the last access time.) Historically, renderers (and the net) haven't had a way of setting creation time, so my inclination is to have a specific check on reception of renderer messages for a null creation time (in a later CL). Let me know if you don't think that addresses the set of issues you're concerned about. ETA: Thinking about this from a slightly more general perspective, I don't think that variations in creation time or last access time will affect whether a cookie is *canonical*; it's that we want to have different privileges able to set those values or not. Which may mean that we'll want a null last access time to have a specific ("Figure it out for me") meaning, and then assert that the renderer must always provide a null last access time. From that perspective, the validation around illegal characters that we talk about above *should* be part of the definition of a canonical cookie, and if you want I'm willing to expand this CL to take care of that if you'd like, though I still think it's reasonable to have it in a different CL when the privilege issue comes up (mostly because CookieMonster::SetCookieWithDetails() does nothing in this space, and as I mention elsewhere, that's my initial model for IsCanonical() and SetCanonicalCookie()). https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... File net/cookies/canonical_cookie_unittest.cc (right): https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie_unittest.cc:600: EXPECT_TRUE(CanonicalCookie("A", "B", "x.y", "/path", base::Time(), On 2017/06/09 18:25:49, mmenke wrote: > Some of these should use IP addresses, both v4 and v6 (I think the v6 format > used by the cookie store/url::CanonicalHost includes braces?) Done, though give them a scan--I have no confidence I've explored the space thoroughly. https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie_unittest.cc:600: EXPECT_TRUE(CanonicalCookie("A", "B", "x.y", "/path", base::Time(), On 2017/06/09 18:25:49, mmenke wrote: > Also, if we choose to anything about the base last-access times, should test > that, too. Current belief is that we're not going to do anything about that in this CL; will test if we end up doing something. https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie_unittest.cc:610: EXPECT_FALSE(CanonicalCookie("A ", "B", "x.y", "/path", base::Time(), On 2017/06/09 18:25:49, mmenke wrote: > I think these each need comments - fixuring out which field is wrong is a bit of > a where's waldo search. Completely fair--even writing them was that. I developed a discipline of always copying the "EXPECT_TRUE" at the top and changing it. Done.
Sorry, also: Comments incorporated, PTAL? & I'm not running try jobs yet as that'll require merging past the test tweak CLs I recently landed, which'll be a bit of a pain; obviously, I won't land without getting back to green try jobs.
https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:410: ParsedCookie::ParseValueString(path_) != path_) { On 2017/06/09 19:38:32, Randy Smith (Not in Mondays) wrote: > On 2017/06/09 18:25:49, mmenke wrote: > > I don't think this is enough. > > > > In particular, disallow CRs/LFs and embedded NULLs, which all seems like they > > could be rather problematic. What happens if someone tries to set such > cookies > > via document.cookie? > > Sorry, could you say a bit more? I may be confused, but it looks to me as if > ParseTokenString(x) will indeed return something that != x if x contains CRs, > LFs, or nulls. I've added a couple of tests in cc_unittest.cc to confirm this, > and cookies constructed with those characters in the name aren't canonical. Oops, I missed the FindFirstTerminator call, and was digging into ParseToken instead. https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:410: ParsedCookie::ParseValueString(path_) != path_) { On 2017/06/09 19:38:32, Randy Smith (Not in Mondays) wrote: > On 2017/06/09 19:00:47, mmenke wrote: > > On 2017/06/09 18:25:49, mmenke wrote: > > > I don't think this is enough. > > > > > > In particular, disallow CRs/LFs and embedded NULLs, which all seems like > they > > > could be rather problematic. What happens if someone tries to set such > > cookies > > > via document.cookie? > > > > Also, may be worth testing if there are any paths where we let web platform > > consumers set cookies with semi-colons or commas (or quotes?) in cookie name > or > > value strings. > > See comments elsewhere about currently web platform consumers (Set-Cookie: > header and document.cookie) using SetCookieWithOptions() and hence going through > parsing. But I'm not sure what you mean by "testing"? How would one test that? If they go through SetCookieWithOptions, that's enough for me. So yea, I think we should ban ";", at least, in name and value (Not sure about commas or quotes - depends on whether they're allowed in set-cookie lines). One would test it by trying to set a cookie with the values, to see if it's possible (Reading specs wouldn't be enough, given our....flexible relationship to the spec). https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:422: canonical_domain != domain_) { On 2017/06/09 19:38:32, Randy Smith (Not in Mondays) wrote: > On 2017/06/09 18:25:49, mmenke wrote: > > If the URL is an IP address, shouldn't canonical_domain == domain_? > > > > My only concern here is domains that can be expressed in multiple ways. i.e. > > [::0] is the same as [0::0] or [0:0:0:0:0:0:0:0:0] (Plus some :0's). Just > > because domain_ is an IP address doesn't mean it can necessarily be part of a > > canonical cookie. > > Is the issue what "canonical" means, i.e. that you could have two equivalent > hosts that are IP addresses and represented differently, and hence they wouldn't > look like the same cookie? If so, yes, that's an issue, but it's an issue that > exists in the current code, and I'm not particularly inclined to shave the yak > unless you think it's specifically important? I'm not sure if canonicalizing domain names canonicalizes IPs or not. This is entirely dependent on what CanonicalizeHost does. Just remove the IP address check, and I think we're fine, regardless? Hrm...It looks like CanonicalCookie deliberately skips canonicalization when the domain is an IP address, sorry, I missed that, so this looks right. My concern is that CanonicalCookie::Create may have a different notion of what's canonical than this function. Ideally, this would only say true iff such a cookie could be created with CanonicalCookie::Create(), in my opinion. Do you have a different notion of what this should do?
https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:422: canonical_domain != domain_) { On 2017/06/09 19:53:59, mmenke wrote: > On 2017/06/09 19:38:32, Randy Smith (Not in Mondays) wrote: > > On 2017/06/09 18:25:49, mmenke wrote: > > > If the URL is an IP address, shouldn't canonical_domain == domain_? > > > > > > My only concern here is domains that can be expressed in multiple ways. > i.e. > > > [::0] is the same as [0::0] or [0:0:0:0:0:0:0:0:0] (Plus some :0's). Just > > > because domain_ is an IP address doesn't mean it can necessarily be part of > a > > > canonical cookie. > > > > Is the issue what "canonical" means, i.e. that you could have two equivalent > > hosts that are IP addresses and represented differently, and hence they > wouldn't > > look like the same cookie? If so, yes, that's an issue, but it's an issue > that > > exists in the current code, and I'm not particularly inclined to shave the yak > > unless you think it's specifically important? > > I'm not sure if canonicalizing domain names canonicalizes IPs or not. This is > entirely dependent on what CanonicalizeHost does. Just remove the IP address > check, and I think we're fine, regardless? > > Hrm...It looks like CanonicalCookie deliberately skips canonicalization when the > domain is an IP address, sorry, I missed that, so this looks right. > > My concern is that CanonicalCookie::Create may have a different notion of what's > canonical than this function. Ideally, this would only say true iff such a > cookie could be created with CanonicalCookie::Create(), in my opinion. Do you > have a different notion of what this should do? So...this is really weird: CanonicalCookie::Create quite deliberately does not canonicalize hosts that are IP address, so it would allow cookies to be set on a domain of "[0::1]", but GURL *does* canonicalize domains. So normally, it's impossible to set a cookie on the domain "[0::1]", because it will be normalized to "[::1]" by GURL. However, this code will let cookies be set on the domain.
Matt: I'd value your reactions to my responses below while I execute on this CL in parallel (see CC def response). Note that I haven't responded to all of your comments (specifically, I'm taking the SetCookieWithOptions discussion as moot given the definition of IsCanonical() discussion). https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:422: canonical_domain != domain_) { On 2017/06/09 19:53:59, mmenke wrote: > On 2017/06/09 19:38:32, Randy Smith (Not in Mondays) wrote: > > On 2017/06/09 18:25:49, mmenke wrote: > > > If the URL is an IP address, shouldn't canonical_domain == domain_? > > > > > > My only concern here is domains that can be expressed in multiple ways. > i.e. > > > [::0] is the same as [0::0] or [0:0:0:0:0:0:0:0:0] (Plus some :0's). Just > > > because domain_ is an IP address doesn't mean it can necessarily be part of > a > > > canonical cookie. > > > > Is the issue what "canonical" means, i.e. that you could have two equivalent > > hosts that are IP addresses and represented differently, and hence they > wouldn't > > look like the same cookie? If so, yes, that's an issue, but it's an issue > that > > exists in the current code, and I'm not particularly inclined to shave the yak > > unless you think it's specifically important? > > I'm not sure if canonicalizing domain names canonicalizes IPs or not. This is > entirely dependent on what CanonicalizeHost does. Just remove the IP address > check, and I think we're fine, regardless? > > Hrm...It looks like CanonicalCookie deliberately skips canonicalization when the > domain is an IP address, sorry, I missed that, so this looks right. > > My concern is that CanonicalCookie::Create may have a different notion of what's > canonical than this function. Ideally, this would only say true iff such a > cookie could be created with CanonicalCookie::Create(), in my opinion. Do you > have a different notion of what this should do? That's an excellent clarifying question. I think that the set of canonical cookies as defined by IsCanonical() should be the set createable by CanonicalCookie::Create closed over what happens when cookies get put into and removed from the cookie store. I'll take a stab at defining that more precisely: * Everything CC::Create can produce except * Creation times may be arbitrary or null (that's looking forward; null means "figure it out for me when entering it into the store") * Access and modification times must be null or after creation times unless creation time is null, in which case they must be null. I'll take a stab at bringing IsCanonical() in line with the above (and dealing with any test failures :-}), but I'd value your reaction to the definition above in the meantime. https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:422: canonical_domain != domain_) { On 2017/06/09 20:05:04, mmenke wrote: > On 2017/06/09 19:53:59, mmenke wrote: > > On 2017/06/09 19:38:32, Randy Smith (Not in Mondays) wrote: > > > On 2017/06/09 18:25:49, mmenke wrote: > > > > If the URL is an IP address, shouldn't canonical_domain == domain_? > > > > > > > > My only concern here is domains that can be expressed in multiple ways. > > i.e. > > > > [::0] is the same as [0::0] or [0:0:0:0:0:0:0:0:0] (Plus some :0's). Just > > > > because domain_ is an IP address doesn't mean it can necessarily be part > of > > a > > > > canonical cookie. > > > > > > Is the issue what "canonical" means, i.e. that you could have two equivalent > > > hosts that are IP addresses and represented differently, and hence they > > wouldn't > > > look like the same cookie? If so, yes, that's an issue, but it's an issue > > that > > > exists in the current code, and I'm not particularly inclined to shave the > yak > > > unless you think it's specifically important? > > > > I'm not sure if canonicalizing domain names canonicalizes IPs or not. This is > > entirely dependent on what CanonicalizeHost does. Just remove the IP address > > check, and I think we're fine, regardless? > > > > Hrm...It looks like CanonicalCookie deliberately skips canonicalization when > the > > domain is an IP address, sorry, I missed that, so this looks right. > > > > My concern is that CanonicalCookie::Create may have a different notion of > what's > > canonical than this function. Ideally, this would only say true iff such a > > cookie could be created with CanonicalCookie::Create(), in my opinion. Do you > > have a different notion of what this should do? > > So...this is really weird: CanonicalCookie::Create quite deliberately does not > canonicalize hosts that are IP address, so it would allow cookies to be set on a > domain of "[0::1]", but GURL *does* canonicalize domains. So normally, it's > impossible to set a cookie on the domain "[0::1]", because it will be normalized > to "[::1]" by GURL. However, this code will let cookies be set on the domain. Yeah, that fits with my understanding, and it sounds wrong to me, but I'm not inclined to change it; it's a pre-existing to this CL wart. Let me know if that bothers you.
https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:422: canonical_domain != domain_) { On 2017/06/11 15:35:24, Randy Smith (Not in Mondays) wrote: > On 2017/06/09 19:53:59, mmenke wrote: > > On 2017/06/09 19:38:32, Randy Smith (Not in Mondays) wrote: > > > On 2017/06/09 18:25:49, mmenke wrote: > > > > If the URL is an IP address, shouldn't canonical_domain == domain_? > > > > > > > > My only concern here is domains that can be expressed in multiple ways. > > i.e. > > > > [::0] is the same as [0::0] or [0:0:0:0:0:0:0:0:0] (Plus some :0's). Just > > > > because domain_ is an IP address doesn't mean it can necessarily be part > of > > a > > > > canonical cookie. > > > > > > Is the issue what "canonical" means, i.e. that you could have two equivalent > > > hosts that are IP addresses and represented differently, and hence they > > wouldn't > > > look like the same cookie? If so, yes, that's an issue, but it's an issue > > that > > > exists in the current code, and I'm not particularly inclined to shave the > yak > > > unless you think it's specifically important? > > > > I'm not sure if canonicalizing domain names canonicalizes IPs or not. This is > > entirely dependent on what CanonicalizeHost does. Just remove the IP address > > check, and I think we're fine, regardless? > > > > Hrm...It looks like CanonicalCookie deliberately skips canonicalization when > the > > domain is an IP address, sorry, I missed that, so this looks right. > > > > My concern is that CanonicalCookie::Create may have a different notion of > what's > > canonical than this function. Ideally, this would only say true iff such a > > cookie could be created with CanonicalCookie::Create(), in my opinion. Do you > > have a different notion of what this should do? > > That's an excellent clarifying question. I think that the set of canonical > cookies as defined by IsCanonical() should be the set createable by > CanonicalCookie::Create closed over what happens when cookies get put into and > removed from the cookie store. I'll take a stab at defining that more > precisely: > * Everything CC::Create can produce except > * Creation times may be arbitrary or null (that's looking forward; null means > "figure it out for me when entering it into the store") > * Access and modification times must be null or after creation times unless > creation time is null, in which case they must be null. > > I'll take a stab at bringing IsCanonical() in line with the above (and dealing > with any test failures :-}), but I'd value your reaction to the definition above > in the meantime. I think that's reasonable, and should be documented. Note that this would include canonicalizing IP address host names, since CanonicalCookie::Create takes a GURL, rather than a string hostname. https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:422: canonical_domain != domain_) { On 2017/06/11 15:35:24, Randy Smith (Not in Mondays) wrote: > On 2017/06/09 20:05:04, mmenke wrote: > > On 2017/06/09 19:53:59, mmenke wrote: > > > On 2017/06/09 19:38:32, Randy Smith (Not in Mondays) wrote: > > > > On 2017/06/09 18:25:49, mmenke wrote: > > > > > If the URL is an IP address, shouldn't canonical_domain == domain_? > > > > > > > > > > My only concern here is domains that can be expressed in multiple ways. > > > i.e. > > > > > [::0] is the same as [0::0] or [0:0:0:0:0:0:0:0:0] (Plus some :0's). > Just > > > > > because domain_ is an IP address doesn't mean it can necessarily be part > > of > > > a > > > > > canonical cookie. > > > > > > > > Is the issue what "canonical" means, i.e. that you could have two > equivalent > > > > hosts that are IP addresses and represented differently, and hence they > > > wouldn't > > > > look like the same cookie? If so, yes, that's an issue, but it's an issue > > > that > > > > exists in the current code, and I'm not particularly inclined to shave the > > yak > > > > unless you think it's specifically important? > > > > > > I'm not sure if canonicalizing domain names canonicalizes IPs or not. This > is > > > entirely dependent on what CanonicalizeHost does. Just remove the IP > address > > > check, and I think we're fine, regardless? > > > > > > Hrm...It looks like CanonicalCookie deliberately skips canonicalization when > > the > > > domain is an IP address, sorry, I missed that, so this looks right. > > > > > > My concern is that CanonicalCookie::Create may have a different notion of > > what's > > > canonical than this function. Ideally, this would only say true iff such a > > > cookie could be created with CanonicalCookie::Create(), in my opinion. Do > you > > > have a different notion of what this should do? > > > > So...this is really weird: CanonicalCookie::Create quite deliberately does > not > > canonicalize hosts that are IP address, so it would allow cookies to be set on > a > > domain of "[0::1]", but GURL *does* canonicalize domains. So normally, it's > > impossible to set a cookie on the domain "[0::1]", because it will be > normalized > > to "[::1]" by GURL. However, this code will let cookies be set on the domain. > > Yeah, that fits with my understanding, and it sounds wrong to me, but I'm not > inclined to change it; it's a pre-existing to this CL wart. Let me know if that > bothers you. I agree this CL shouldn't try to change behavior, or work around existing warts, however, only allowing cookies to be set on normalized IP addresses *does* seem to be existing behavior, just because CanonicalCookie::Create takes a GURL, rather than a hostname. https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:429: return true; On 2017/06/09 19:38:32, Randy Smith (Not in Mondays) wrote: > On 2017/06/09 18:25:49, mmenke wrote: > > What if there's no creation time, or the creation time is far in the future? > > The main concern is that may much with GC, or that a compromised renderer > could > > create a forever cookie, that will never expire, due to always passing the > > 30-day checks. > > As noted above, I'd like to put in the Mojo-required renderer checks in a > separate CL that's more closely associated with the shift to mojo, and leave > this as being just a refactor of/based on the current code. Is that reasonable, > or do you think that this CL is incomplete without those checks? I think that's fine. Part of the trouble I'm having with this CL is the code isn't even used, so I have to speculate on how it will be used (Or punt that part of the review until it's actually used, which means I'll be reviewing a lot of code that the CL doesn't actually modify, which seems kinda funky). > > Of course, just dropping them could cause problems if the clock changes, or > > clocks are off. Are there cases where consumers really want to set a cookie > > with a weird creation time? If not, can just have the API to set a > > CanonicalCookie overwrite the time itself. I'm less concerned about funky > > expiration times, since that's what GC is for, and that's based mostly on > > creation time. > > > > We'll presumably want to overwrite last_access_data. The only other fields > that > > look to be of potential concern when set by a compromised renderer are domain > > and secure, but those can already be set be such a renderer, so that's nothing > > new. > > Some of the issues you're raising are handled in > https://codereview.chromium.org/2882063002; specifically, in that CL a null > creation time is taken as "set the creation time when you push the cookie into > the monster". (And CC already has a method for overriding the last access > time.) > > Historically, renderers (and the net) haven't had a way of setting creation > time, so my inclination is to have a specific check on reception of renderer > messages for a null creation time (in a later CL). Let me know if you don't > think that addresses the set of issues you're concerned about. I think this is reasonable. We should be very clear in function docs just what this does and does not check, though. > ETA: Thinking about this from a slightly more general perspective, I don't think > that variations in creation time or last access time will affect whether a > cookie is *canonical*; it's that we want to have different privileges able to > set those values or not. Which may mean that we'll want a null last access time > to have a specific ("Figure it out for me") meaning, and then assert that the > renderer must always provide a null last access time. > > From that perspective, the validation around illegal characters that we talk > about above *should* be part of the definition of a canonical cookie, and if you > want I'm willing to expand this CL to take care of that if you'd like, though I > still think it's reasonable to have it in a different CL when the privilege > issue comes up (mostly because CookieMonster::SetCookieWithDetails() does > nothing in this space, and as I mention elsewhere, that's my initial model for > IsCanonical() and SetCanonicalCookie()). I'd prefer to do it in this CL, mostly because I think it's easier to make sure we have good testing of this method on a CL that just adds this method, rather than in a CL that does other stuff, and it also highlights any issue with the checks earlier, so we can fix them now, if need be, rather than having to deal with that in a CL where changes here aren't the focus.
https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:429: return true; On 2017/06/12 15:27:00, mmenke wrote: > On 2017/06/09 19:38:32, Randy Smith (Not in Mondays) wrote: > > On 2017/06/09 18:25:49, mmenke wrote: > > > What if there's no creation time, or the creation time is far in the future? > > > > The main concern is that may much with GC, or that a compromised renderer > > could > > > create a forever cookie, that will never expire, due to always passing the > > > 30-day checks. > > > > As noted above, I'd like to put in the Mojo-required renderer checks in a > > separate CL that's more closely associated with the shift to mojo, and leave > > this as being just a refactor of/based on the current code. Is that > reasonable, > > or do you think that this CL is incomplete without those checks? > > I think that's fine. Part of the trouble I'm having with this CL is the code > isn't even used, so I have to speculate on how it will be used (Or punt that > part of the review until it's actually used, which means I'll be reviewing a lot > of code that the CL doesn't actually modify, which seems kinda funky). > > > > Of course, just dropping them could cause problems if the clock changes, or > > > clocks are off. Are there cases where consumers really want to set a cookie > > > with a weird creation time? If not, can just have the API to set a > > > CanonicalCookie overwrite the time itself. I'm less concerned about funky > > > expiration times, since that's what GC is for, and that's based mostly on > > > creation time. > > > > > > We'll presumably want to overwrite last_access_data. The only other fields > > that > > > look to be of potential concern when set by a compromised renderer are > domain > > > and secure, but those can already be set be such a renderer, so that's > nothing > > > new. > > > > Some of the issues you're raising are handled in > > https://codereview.chromium.org/2882063002; specifically, in that CL a null > > creation time is taken as "set the creation time when you push the cookie into > > the monster". (And CC already has a method for overriding the last access > > time.) > > > > Historically, renderers (and the net) haven't had a way of setting creation > > time, so my inclination is to have a specific check on reception of renderer > > messages for a null creation time (in a later CL). Let me know if you don't > > think that addresses the set of issues you're concerned about. > > I think this is reasonable. We should be very clear in function docs just what > this does and does not check, though. > > > ETA: Thinking about this from a slightly more general perspective, I don't > think > > that variations in creation time or last access time will affect whether a > > cookie is *canonical*; it's that we want to have different privileges able to > > set those values or not. Which may mean that we'll want a null last access > time > > to have a specific ("Figure it out for me") meaning, and then assert that the > > renderer must always provide a null last access time. > > > > From that perspective, the validation around illegal characters that we talk > > about above *should* be part of the definition of a canonical cookie, and if > you > > want I'm willing to expand this CL to take care of that if you'd like, though > I > > still think it's reasonable to have it in a different CL when the privilege > > issue comes up (mostly because CookieMonster::SetCookieWithDetails() does > > nothing in this space, and as I mention elsewhere, that's my initial model for > > IsCanonical() and SetCanonicalCookie()). > > I'd prefer to do it in this CL, mostly because I think it's easier to make sure > we have good testing of this method on a CL that just adds this method, rather > than in a CL that does other stuff, and it also highlights any issue with the > checks earlier, so we can fix them now, if need be, rather than having to deal > with that in a CL where changes here aren't the focus. Another option would be to land this largely as-is, and then fix it up in another CL that does nothing else, which I'm also fine with.
Responding to your comments, but no code changes, and mostly nothing you need to respond to until I finish the code changes. The only thing I'll call your attention to is the wart that I'm attempting to dodge fixing in CC::Create(). Arguably what I should do is split off yet another CL to fix that wart, and then get back to working on this one--let me know if you'd strongly like me to do that, but given how many different CLs this attempt has sparked so far, I'd still prefer to ignore it. (Yaks. Why did it have to be Yaks?) https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:422: canonical_domain != domain_) { On 2017/06/11 15:35:24, Randy Smith (Not in Mondays) wrote: > On 2017/06/09 19:53:59, mmenke wrote: > > On 2017/06/09 19:38:32, Randy Smith (Not in Mondays) wrote: > > > On 2017/06/09 18:25:49, mmenke wrote: > > > > If the URL is an IP address, shouldn't canonical_domain == domain_? > > > > > > > > My only concern here is domains that can be expressed in multiple ways. > > i.e. > > > > [::0] is the same as [0::0] or [0:0:0:0:0:0:0:0:0] (Plus some :0's). Just > > > > because domain_ is an IP address doesn't mean it can necessarily be part > of > > a > > > > canonical cookie. > > > > > > Is the issue what "canonical" means, i.e. that you could have two equivalent > > > hosts that are IP addresses and represented differently, and hence they > > wouldn't > > > look like the same cookie? If so, yes, that's an issue, but it's an issue > > that > > > exists in the current code, and I'm not particularly inclined to shave the > yak > > > unless you think it's specifically important? > > > > I'm not sure if canonicalizing domain names canonicalizes IPs or not. This is > > entirely dependent on what CanonicalizeHost does. Just remove the IP address > > check, and I think we're fine, regardless? > > > > Hrm...It looks like CanonicalCookie deliberately skips canonicalization when > the > > domain is an IP address, sorry, I missed that, so this looks right. > > > > My concern is that CanonicalCookie::Create may have a different notion of > what's > > canonical than this function. Ideally, this would only say true iff such a > > cookie could be created with CanonicalCookie::Create(), in my opinion. Do you > > have a different notion of what this should do? > > That's an excellent clarifying question. I think that the set of canonical > cookies as defined by IsCanonical() should be the set createable by > CanonicalCookie::Create closed over what happens when cookies get put into and > removed from the cookie store. I'll take a stab at defining that more > precisely: > * Everything CC::Create can produce except > * Creation times may be arbitrary or null (that's looking forward; null means > "figure it out for me when entering it into the store") > * Access and modification times must be null or after creation times unless > creation time is null, in which case they must be null. Ignore the above about modification times; they don't exist. I am not planning to put any constraints on expiry times; they can exist even if the creation time is null (through use of expires rather than max age). > I'll take a stab at bringing IsCanonical() in line with the above (and dealing > with any test failures :-}), but I'd value your reaction to the definition above > in the meantime. Just a quick note: As best I can tell, CanonicalCookie::Create() relies on creation_time being non-null (see the call to CanonExpiration()) but does not assert or document that it so relies. I don't plan on trying to fix CanonicalCookie::Create() in this CL, but I do plan to allow canonical cookies to have times as noted above despite the slightly strange behavior of CC::Create(). I don't like the inconsistency, but neither do I want to get caught in too large a yak shave, and I think null creation times for CCs are a fine thing, and could be useful--specifically, it allows only a single CC copy on setting a CC on the cookie store while making sure that creation times are unique within a cookie store. https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:429: return true; On 2017/06/12 15:27:00, mmenke wrote: > On 2017/06/09 19:38:32, Randy Smith (Not in Mondays) wrote: > > On 2017/06/09 18:25:49, mmenke wrote: > > > What if there's no creation time, or the creation time is far in the future? > > > > The main concern is that may much with GC, or that a compromised renderer > > could > > > create a forever cookie, that will never expire, due to always passing the > > > 30-day checks. > > > > As noted above, I'd like to put in the Mojo-required renderer checks in a > > separate CL that's more closely associated with the shift to mojo, and leave > > this as being just a refactor of/based on the current code. Is that > reasonable, > > or do you think that this CL is incomplete without those checks? > > I think that's fine. Part of the trouble I'm having with this CL is the code > isn't even used, so I have to speculate on how it will be used (Or punt that > part of the review until it's actually used, which means I'll be reviewing a lot > of code that the CL doesn't actually modify, which seems kinda funky). I see that as part of the basic philosophical tension between doing a lot of small CLs vs. one big CL. I'm happy to go with your preference, but I don't see a way to provide cake for both having and eating :-} :-J. I'll note that *some* of how this CL will be used (not the renderer checks, which are down the line) will be occurring in https://codereview.chromium.org/2882063002, and I'd actually be happy to duck back and forth between the two CLs based on your prompting if you'd like to review them in parallel. Let me know if you want to do that, and I'll bring them both to "reviewable level" before I next ping you on either. But for the renderer checks, yeah, I think you'll need to use your imagination or review this code again when we get to those CLs (though I don't intend to put any of the renderer specific checks in the main cookie code). I'd suggest thinking of this CL as if its consumers are all privileged, and we'll deal with the unprivileged consumers later, and separately. > > > > Of course, just dropping them could cause problems if the clock changes, or > > > clocks are off. Are there cases where consumers really want to set a cookie > > > with a weird creation time? If not, can just have the API to set a > > > CanonicalCookie overwrite the time itself. I'm less concerned about funky > > > expiration times, since that's what GC is for, and that's based mostly on > > > creation time. > > > > > > We'll presumably want to overwrite last_access_data. The only other fields > > that > > > look to be of potential concern when set by a compromised renderer are > domain > > > and secure, but those can already be set be such a renderer, so that's > nothing > > > new. > > > > Some of the issues you're raising are handled in > > https://codereview.chromium.org/2882063002; specifically, in that CL a null > > creation time is taken as "set the creation time when you push the cookie into > > the monster". (And CC already has a method for overriding the last access > > time.) > > > > Historically, renderers (and the net) haven't had a way of setting creation > > time, so my inclination is to have a specific check on reception of renderer > > messages for a null creation time (in a later CL). Let me know if you don't > > think that addresses the set of issues you're concerned about. > > I think this is reasonable. We should be very clear in function docs just what > this does and does not check, though. > > > ETA: Thinking about this from a slightly more general perspective, I don't > think > > that variations in creation time or last access time will affect whether a > > cookie is *canonical*; it's that we want to have different privileges able to > > set those values or not. Which may mean that we'll want a null last access > time > > to have a specific ("Figure it out for me") meaning, and then assert that the > > renderer must always provide a null last access time. > > > > From that perspective, the validation around illegal characters that we talk > > about above *should* be part of the definition of a canonical cookie, and if > you > > want I'm willing to expand this CL to take care of that if you'd like, though > I > > still think it's reasonable to have it in a different CL when the privilege > > issue comes up (mostly because CookieMonster::SetCookieWithDetails() does > > nothing in this space, and as I mention elsewhere, that's my initial model for > > IsCanonical() and SetCanonicalCookie()). > > I'd prefer to do it in this CL, mostly because I think it's easier to make sure > we have good testing of this method on a CL that just adds this method, rather > than in a CL that does other stuff, and it also highlights any issue with the > checks earlier, so we can fix them now, if need be, rather than having to deal > with that in a CL where changes here aren't the focus. Yeah, I had basically come to this same conclusion, free-running on defining IsCanonical() based on CC::Create(). Accepted. https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:429: return true; On 2017/06/12 15:28:48, mmenke wrote: > On 2017/06/12 15:27:00, mmenke wrote: > > On 2017/06/09 19:38:32, Randy Smith (Not in Mondays) wrote: > > > On 2017/06/09 18:25:49, mmenke wrote: > > > > What if there's no creation time, or the creation time is far in the > future? > > > > > > The main concern is that may much with GC, or that a compromised renderer > > > could > > > > create a forever cookie, that will never expire, due to always passing the > > > > 30-day checks. > > > > > > As noted above, I'd like to put in the Mojo-required renderer checks in a > > > separate CL that's more closely associated with the shift to mojo, and leave > > > this as being just a refactor of/based on the current code. Is that > > reasonable, > > > or do you think that this CL is incomplete without those checks? > > > > I think that's fine. Part of the trouble I'm having with this CL is the code > > isn't even used, so I have to speculate on how it will be used (Or punt that > > part of the review until it's actually used, which means I'll be reviewing a > lot > > of code that the CL doesn't actually modify, which seems kinda funky). > > > > > > Of course, just dropping them could cause problems if the clock changes, > or > > > > clocks are off. Are there cases where consumers really want to set a > cookie > > > > with a weird creation time? If not, can just have the API to set a > > > > CanonicalCookie overwrite the time itself. I'm less concerned about funky > > > > expiration times, since that's what GC is for, and that's based mostly on > > > > creation time. > > > > > > > > We'll presumably want to overwrite last_access_data. The only other > fields > > > that > > > > look to be of potential concern when set by a compromised renderer are > > domain > > > > and secure, but those can already be set be such a renderer, so that's > > nothing > > > > new. > > > > > > Some of the issues you're raising are handled in > > > https://codereview.chromium.org/2882063002; specifically, in that CL a null > > > creation time is taken as "set the creation time when you push the cookie > into > > > the monster". (And CC already has a method for overriding the last access > > > time.) > > > > > > Historically, renderers (and the net) haven't had a way of setting creation > > > time, so my inclination is to have a specific check on reception of renderer > > > messages for a null creation time (in a later CL). Let me know if you don't > > > think that addresses the set of issues you're concerned about. > > > > I think this is reasonable. We should be very clear in function docs just > what > > this does and does not check, though. > > > > > ETA: Thinking about this from a slightly more general perspective, I don't > > think > > > that variations in creation time or last access time will affect whether a > > > cookie is *canonical*; it's that we want to have different privileges able > to > > > set those values or not. Which may mean that we'll want a null last access > > time > > > to have a specific ("Figure it out for me") meaning, and then assert that > the > > > renderer must always provide a null last access time. > > > > > > From that perspective, the validation around illegal characters that we talk > > > about above *should* be part of the definition of a canonical cookie, and if > > you > > > want I'm willing to expand this CL to take care of that if you'd like, > though > > I > > > still think it's reasonable to have it in a different CL when the privilege > > > issue comes up (mostly because CookieMonster::SetCookieWithDetails() does > > > nothing in this space, and as I mention elsewhere, that's my initial model > for > > > IsCanonical() and SetCanonicalCookie()). > > > > I'd prefer to do it in this CL, mostly because I think it's easier to make > sure > > we have good testing of this method on a CL that just adds this method, rather > > than in a CL that does other stuff, and it also highlights any issue with the > > checks earlier, so we can fix them now, if need be, rather than having to deal > > with that in a CL where changes here aren't the focus. > > Another option would be to land this largely as-is, and then fix it up in > another CL that does nothing else, which I'm also fine with. Not really any point; let's keep the CL conceptually coherent. I've done enough salami slicing, at least IMO.
The CQ bit was checked by rdsmith@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...
Matt: I think I've done a reasonable job at defining CanonicalCookie::IsCanonical() in comments and bringing it into alignment with CanonicalCookie::Create. PTAL? https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:422: canonical_domain != domain_) { On 2017/06/12 15:27:00, mmenke wrote: > On 2017/06/11 15:35:24, Randy Smith (Not in Mondays) wrote: > > On 2017/06/09 19:53:59, mmenke wrote: > > > On 2017/06/09 19:38:32, Randy Smith (Not in Mondays) wrote: > > > > On 2017/06/09 18:25:49, mmenke wrote: > > > > > If the URL is an IP address, shouldn't canonical_domain == domain_? > > > > > > > > > > My only concern here is domains that can be expressed in multiple ways. > > > i.e. > > > > > [::0] is the same as [0::0] or [0:0:0:0:0:0:0:0:0] (Plus some :0's). > Just > > > > > because domain_ is an IP address doesn't mean it can necessarily be part > > of > > > a > > > > > canonical cookie. > > > > > > > > Is the issue what "canonical" means, i.e. that you could have two > equivalent > > > > hosts that are IP addresses and represented differently, and hence they > > > wouldn't > > > > look like the same cookie? If so, yes, that's an issue, but it's an issue > > > that > > > > exists in the current code, and I'm not particularly inclined to shave the > > yak > > > > unless you think it's specifically important? > > > > > > I'm not sure if canonicalizing domain names canonicalizes IPs or not. This > is > > > entirely dependent on what CanonicalizeHost does. Just remove the IP > address > > > check, and I think we're fine, regardless? > > > > > > Hrm...It looks like CanonicalCookie deliberately skips canonicalization when > > the > > > domain is an IP address, sorry, I missed that, so this looks right. > > > > > > My concern is that CanonicalCookie::Create may have a different notion of > > what's > > > canonical than this function. Ideally, this would only say true iff such a > > > cookie could be created with CanonicalCookie::Create(), in my opinion. Do > you > > > have a different notion of what this should do? > > > > That's an excellent clarifying question. I think that the set of canonical > > cookies as defined by IsCanonical() should be the set createable by > > CanonicalCookie::Create closed over what happens when cookies get put into and > > removed from the cookie store. I'll take a stab at defining that more > > precisely: > > * Everything CC::Create can produce except > > * Creation times may be arbitrary or null (that's looking forward; null means > > "figure it out for me when entering it into the store") > > * Access and modification times must be null or after creation times unless > > creation time is null, in which case they must be null. > > > > I'll take a stab at bringing IsCanonical() in line with the above (and dealing > > with any test failures :-}), but I'd value your reaction to the definition > above > > in the meantime. > > I think that's reasonable, and should be documented. Note that this would > include canonicalizing IP address host names, since CanonicalCookie::Create > takes a GURL, rather than a string hostname. Huh. Good point. I think the only code modification required for this is to remove the check above for HostIsIPAddress(); no matter what the domain must equal the canonicalized domain (even if it's empty, since canonicalizing an empty domain just returns an empty domain). I'll put tests in for both these cases.
Will do. I'll plan to take a look tomorrow. Heading out early today - puff of smoke, which I think was from my printer, set off my fire alarm early in the morning (Well, early in the morning for me). On 2017/06/13 20:37:35, Randy Smith (Not in Mondays) wrote: > Matt: I think I've done a reasonable job at defining > CanonicalCookie::IsCanonical() in comments and bringing it into alignment with > CanonicalCookie::Create. PTAL? > > https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... > File net/cookies/canonical_cookie.cc (right): > > https://codereview.chromium.org/2898953008/diff/100001/net/cookies/canonical_... > net/cookies/canonical_cookie.cc:422: canonical_domain != domain_) { > On 2017/06/12 15:27:00, mmenke wrote: > > On 2017/06/11 15:35:24, Randy Smith (Not in Mondays) wrote: > > > On 2017/06/09 19:53:59, mmenke wrote: > > > > On 2017/06/09 19:38:32, Randy Smith (Not in Mondays) wrote: > > > > > On 2017/06/09 18:25:49, mmenke wrote: > > > > > > If the URL is an IP address, shouldn't canonical_domain == domain_? > > > > > > > > > > > > My only concern here is domains that can be expressed in multiple > ways. > > > > i.e. > > > > > > [::0] is the same as [0::0] or [0:0:0:0:0:0:0:0:0] (Plus some :0's). > > Just > > > > > > because domain_ is an IP address doesn't mean it can necessarily be > part > > > of > > > > a > > > > > > canonical cookie. > > > > > > > > > > Is the issue what "canonical" means, i.e. that you could have two > > equivalent > > > > > hosts that are IP addresses and represented differently, and hence they > > > > wouldn't > > > > > look like the same cookie? If so, yes, that's an issue, but it's an > issue > > > > that > > > > > exists in the current code, and I'm not particularly inclined to shave > the > > > yak > > > > > unless you think it's specifically important? > > > > > > > > I'm not sure if canonicalizing domain names canonicalizes IPs or not. > This > > is > > > > entirely dependent on what CanonicalizeHost does. Just remove the IP > > address > > > > check, and I think we're fine, regardless? > > > > > > > > Hrm...It looks like CanonicalCookie deliberately skips canonicalization > when > > > the > > > > domain is an IP address, sorry, I missed that, so this looks right. > > > > > > > > My concern is that CanonicalCookie::Create may have a different notion of > > > what's > > > > canonical than this function. Ideally, this would only say true iff such > a > > > > cookie could be created with CanonicalCookie::Create(), in my opinion. Do > > you > > > > have a different notion of what this should do? > > > > > > That's an excellent clarifying question. I think that the set of canonical > > > cookies as defined by IsCanonical() should be the set createable by > > > CanonicalCookie::Create closed over what happens when cookies get put into > and > > > removed from the cookie store. I'll take a stab at defining that more > > > precisely: > > > * Everything CC::Create can produce except > > > * Creation times may be arbitrary or null (that's looking forward; null > means > > > "figure it out for me when entering it into the store") > > > * Access and modification times must be null or after creation times unless > > > creation time is null, in which case they must be null. > > > > > > I'll take a stab at bringing IsCanonical() in line with the above (and > dealing > > > with any test failures :-}), but I'd value your reaction to the definition > > above > > > in the meantime. > > > > I think that's reasonable, and should be documented. Note that this would > > include canonicalizing IP address host names, since CanonicalCookie::Create > > takes a GURL, rather than a string hostname. > > Huh. Good point. I think the only code modification required for this is to > remove the check above for HostIsIPAddress(); no matter what the domain must > equal the canonicalized domain (even if it's empty, since canonicalizing an > empty domain just returns an empty domain). I'll put tests in for both these > cases.
https://codereview.chromium.org/2898953008/diff/160001/net/cookies/canonical_... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/2898953008/diff/160001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:418: (creation_date_.is_null() || creation_date_ > last_access_date_)) { I'll call out that, given that we allow callers to set creation date, I'm not sure about this test; setting a creation date in the future would be a very easy way to fail this check after a cookie is laundered through the store. So it probably makes sense to remove the comparison. Let me know what you think.
The CQ bit was checked by rdsmith@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: This issue passed the CQ dry run.
This looks pretty reasonable to me. Want to take a more careful look tomorrow, particularly around path and domain logic. https://codereview.chromium.org/2898953008/diff/180001/net/cookies/canonical_... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/2898953008/diff/180001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:418: (creation_date_.is_null() || creation_date_ > last_access_date_)) { On 2017/06/13 21:05:04, Randy Smith (Not in Mondays) wrote: > I'll call out that, given that we allow callers to set creation date, I'm not > sure about this test; setting a creation date in the future would be a very easy > way to fail this check after a cookie is laundered through the store. So it > probably makes sense to remove the comparison. Let me know what you think. Also, if there's a time change, creation date could be before last_access_date....yea, I think it makes sense to remove it. https://codereview.chromium.org/2898953008/diff/180001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:436: if (GetCookiePrefix(name_) == COOKIE_PREFIX_HOST && Check if IsSecure(), and also check if IsSecure() if the prefix is CanonicalCookie::COOKIE_PREFIX_SECURE? https://codereview.chromium.org/2898953008/diff/180001/net/cookies/canonical_... File net/cookies/canonical_cookie_unittest.cc (right): https://codereview.chromium.org/2898953008/diff/180001/net/cookies/canonical_... net/cookies/canonical_cookie_unittest.cc:47: TEST(CanonicalCookie, CreateConstraints) { What's this supposed to check? Space in the name? I'm not sure how that follows from "CreateConstraints". Maybe add a comment? Or call it "SpaceInName"?
Oh, and what about domains that are completely invalid? "@:ßÞ"
On 2017/06/15 21:44:37, mmenke wrote: > Oh, and what about domains that are completely invalid? > > "@:ßÞ" (From a security standpoint, I don't care about these, just wondering if we should do anything about them)
On 2017/06/15 21:44:37, mmenke wrote: > Oh, and what about domains that are completely invalid? > > "@:ßÞ" I think that's taken care of by CanonicalizeHost, which reaches deep into url: and I believe returns an empty domain (which won't match the input domain, failing the canonical check). I've added a test in IsCanonical() using some garbage.
Comments incorporated; PTAL. https://codereview.chromium.org/2898953008/diff/180001/net/cookies/canonical_... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/2898953008/diff/180001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:418: (creation_date_.is_null() || creation_date_ > last_access_date_)) { On 2017/06/15 21:30:24, mmenke wrote: > On 2017/06/13 21:05:04, Randy Smith (Not in Mondays) wrote: > > I'll call out that, given that we allow callers to set creation date, I'm not > > sure about this test; setting a creation date in the future would be a very > easy > > way to fail this check after a cookie is laundered through the store. So it > > probably makes sense to remove the comparison. Let me know what you think. > > Also, if there's a time change, creation date could be before > last_access_date....yea, I think it makes sense to remove it. Done. I left the requirement that last_access_date_ be null if creation date is. https://codereview.chromium.org/2898953008/diff/180001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:436: if (GetCookiePrefix(name_) == COOKIE_PREFIX_HOST && On 2017/06/15 21:30:24, mmenke wrote: > Check if IsSecure(), Whoops, didn't realize host cookies had to be secure. Done. > and also check if IsSecure() if the prefix is > CanonicalCookie::COOKIE_PREFIX_SECURE? Also good catch. Done. (I may have been over clever in how I wrote the tests--feel free to tell me to break it out into clear, separate clauses.) https://codereview.chromium.org/2898953008/diff/180001/net/cookies/canonical_... File net/cookies/canonical_cookie_unittest.cc (right): https://codereview.chromium.org/2898953008/diff/180001/net/cookies/canonical_... net/cookies/canonical_cookie_unittest.cc:47: TEST(CanonicalCookie, CreateConstraints) { On 2017/06/15 21:30:24, mmenke wrote: > What's this supposed to check? Space in the name? I'm not sure how that > follows from "CreateConstraints". Maybe add a comment? Or call it > "SpaceInName"? Changed to "SpaceInName". (When I first wrote it I thought I'd be putting in more tests to make sure that Create() obeyed what I thought of as its constraints, thus the original name. I may change it in some fashion if I end up putting in more tests later in this review.)
The CQ bit was checked by rdsmith@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...
rdsmith@chromium.org changed reviewers: + sgurun@chromium.org
Also, sgurun@, PTAL at android_webview/?
https://codereview.chromium.org/2898953008/diff/200001/net/cookies/canonical_... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/2898953008/diff/200001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:412: !ParsedCookie::IsValidCookieAttributeValue(domain_) || Should we just rely on CanonicalizeHost() to do all the sanity checking on the domain name? Not sure if we also need the two domain checks here. Also worth noting that domain_ can be taken from the URL, instead of as part of a parameter. https://codereview.chromium.org/2898953008/diff/200001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:438: // Fall through; host cookies must also be secure. I suggest not falling through, and trusting in the optimizer. I think falling through is much more likely to lead to regressions on modification. https://codereview.chromium.org/2898953008/diff/200001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:441: return false; Add tests for this, for both SECURE and HOST cookies? https://codereview.chromium.org/2898953008/diff/200001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:443: default:; I don't think that semi-colon is needed?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rdsmith@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...
Next round; PTAL? https://codereview.chromium.org/2898953008/diff/200001/net/cookies/canonical_... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/2898953008/diff/200001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:412: !ParsedCookie::IsValidCookieAttributeValue(domain_) || On 2017/06/16 15:36:03, mmenke wrote: > Should we just rely on CanonicalizeHost() to do all the sanity checking on the > domain name? Not sure if we also need the two domain checks here. > Also worth > noting that domain_ can be taken from the URL, instead of as part of a > parameter. Hmmm. I think your second point is the key one, specifically that along the CC::Create() path, we might end up with a domain that hasn't gone through the above checks because it was never part of a cookie attribute value, so these checks might mark as non-canonical a cookie produced by CC::Create(). Ok, removing checks with a comment. As per our offline conversation, I'm not going to worry about the implications of this for paths--it's both more complicated, and not as important, since we don't send paths back to the server. (For the record, if there isn't a path attribute, the path we set is from the URL to the rightmost '/'; see CanonPathWithString() in canonical_cookie.cc.) https://codereview.chromium.org/2898953008/diff/200001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:438: // Fall through; host cookies must also be secure. On 2017/06/16 15:36:03, mmenke wrote: > I suggest not falling through, and trusting in the optimizer. I think falling > through is much more likely to lead to regressions on modification. Oh, you never let me have any fun. Done. (This comment should not be interpreted as in any way, shape, or form *disagreeing* with you :-}.) https://codereview.chromium.org/2898953008/diff/200001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:441: return false; On 2017/06/16 15:36:03, mmenke wrote: > Add tests for this, for both SECURE and HOST cookies? Done. https://codereview.chromium.org/2898953008/diff/200001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:443: default:; On 2017/06/16 15:36:03, mmenke wrote: > I don't think that semi-colon is needed? I put in a "break" for consistency (which you may consider worse :-}).
LGTM! https://codereview.chromium.org/2898953008/diff/200001/net/cookies/canonical_... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/2898953008/diff/200001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:443: default:; On 2017/06/16 19:40:05, Randy Smith (Not in Mondays) wrote: > On 2017/06/16 15:36:03, mmenke wrote: > > I don't think that semi-colon is needed? > > I put in a "break" for consistency (which you may consider worse :-}). I'm perfectly happy with a break.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
On 2017/06/16 19:48:04, mmenke wrote: > LGTM! Thank you! > > https://codereview.chromium.org/2898953008/diff/200001/net/cookies/canonical_... > File net/cookies/canonical_cookie.cc (right): > > https://codereview.chromium.org/2898953008/diff/200001/net/cookies/canonical_... > net/cookies/canonical_cookie.cc:443: default:; > On 2017/06/16 19:40:05, Randy Smith (Not in Mondays) wrote: > > On 2017/06/16 15:36:03, mmenke wrote: > > > I don't think that semi-colon is needed? > > > > I put in a "break" for consistency (which you may consider worse :-}). > > I'm perfectly happy with a break.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
aw lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rdsmith@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2898953008/#ps240001 (title: "Removed vestigial code.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1497658815087960,
"parent_rev": "4c7e1ad4f62150f722f97c80acc457faf8f947f1", "commit_rev":
"1e64af8eed07578f01b933d654180659c0fe962c"}
Message was sent while issue was closed.
Description was changed from ========== Implement and test CanonicalCookie::IsCanonical() This is done for data validation including but not limited to the cookies servicification effort. BUG=721395, 723734 R=mmenke@chromium.org ========== to ========== Implement and test CanonicalCookie::IsCanonical() This is done for data validation including but not limited to the cookies servicification effort. BUG=721395, 723734 R=mmenke@chromium.org Review-Url: https://codereview.chromium.org/2898953008 Cr-Commit-Position: refs/heads/master@{#480238} Committed: https://chromium.googlesource.com/chromium/src/+/1e64af8eed07578f01b933d65418... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/1e64af8eed07578f01b933d65418... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
