Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2959123002:
Allow paths constructed from URLs rather as well as cookie attributes. (Closed)
|
Created:
3 years, 5 months ago by Randy Smith (Not in Mondays) Modified:
3 years, 5 months ago Reviewers:
mmenke CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow paths constructed from URLs rather as well as cookie attributes.
BUG=734355
R=mmenke@chromium.org
Review-Url: https://codereview.chromium.org/2959123002
Cr-Commit-Position: refs/heads/master@{#483114}
Committed: https://chromium.googlesource.com/chromium/src/+/81abc648e801e288b9c43a5c409d2d7285444f98
Patch Set 1 #Patch Set 2 : Remove failing path tests. #
Total comments: 4
Patch Set 3 : Remove test for space in path--GURL escapes those. #
Messages
Total messages: 21 (11 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: This is what I get for not following thoughts to their logical conclusions :-}; see https://codereview.chromium.org/2898953008/diff/200001/net/cookies/canonical_.... Turns out that cookies with paths constructed from URLs can be "non-canonical" by the old definition. I've relaxed the constraints. PTAL?
https://codereview.chromium.org/2959123002/diff/20001/net/cookies/canonical_c... File net/cookies/canonical_cookie_unittest.cc (right): https://codereview.chromium.org/2959123002/diff/20001/net/cookies/canonical_c... net/cookies/canonical_cookie_unittest.cc:64: cookie = CanonicalCookie::Create(GURL("http://fool/xyzzy wwwww/"), "A=B", Doesn't GURL precent-escape spaces in paths?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2959123002/diff/20001/net/cookies/canonical_c... File net/cookies/canonical_cookie_unittest.cc (right): https://codereview.chromium.org/2959123002/diff/20001/net/cookies/canonical_c... net/cookies/canonical_cookie_unittest.cc:64: cookie = CanonicalCookie::Create(GURL("http://fool/xyzzy wwwww/"), "A=B", On 2017/06/28 00:26:20, mmenke wrote: > Doesn't GURL precent-escape spaces in paths? Yeah, it does :-J. Ok. What I think that means in terms of restrictions: * A cookie attribute value can create a value with a space (not that this makes sense for the reason you mention, but it could be done), but not a space at the beginning or end. * A path created from a URL will not have a space in it period. So I think the canonical test should be "no space at beginning or end, but spaces in middle allowed" with positive and negative tests as appropriate in CreationCornerCases and IsCanonical. SGTY?
https://codereview.chromium.org/2959123002/diff/20001/net/cookies/canonical_c... File net/cookies/canonical_cookie_unittest.cc (right): https://codereview.chromium.org/2959123002/diff/20001/net/cookies/canonical_c... net/cookies/canonical_cookie_unittest.cc:64: cookie = CanonicalCookie::Create(GURL("http://fool/xyzzy wwwww/"), "A=B", On 2017/06/28 14:58:05, Randy Smith (Not in Mondays) wrote: > On 2017/06/28 00:26:20, mmenke wrote: > > Doesn't GURL precent-escape spaces in paths? > > Yeah, it does :-J. Ok. What I think that means in terms of restrictions: > * A cookie attribute value can create a value with a space (not that this makes > sense for the reason you mention, but it could be done), but not a space at the > beginning or end. > * A path created from a URL will not have a space in it period. > > So I think the canonical test should be "no space at beginning or end, but > spaces in middle allowed" with positive and negative tests as appropriate in > CreationCornerCases and IsCanonical. SGTY? I'm wondering if it's worth the effort - I don't think there's a security issue with paths that can never actually be matched? There may also be weird restrictions (Other characters that can't be in values, and are percent-escaped in URLs).
https://codereview.chromium.org/2959123002/diff/20001/net/cookies/canonical_c... File net/cookies/canonical_cookie_unittest.cc (right): https://codereview.chromium.org/2959123002/diff/20001/net/cookies/canonical_c... net/cookies/canonical_cookie_unittest.cc:64: cookie = CanonicalCookie::Create(GURL("http://fool/xyzzy wwwww/"), "A=B", On 2017/06/28 17:13:48, mmenke wrote: > On 2017/06/28 14:58:05, Randy Smith (Not in Mondays) wrote: > > On 2017/06/28 00:26:20, mmenke wrote: > > > Doesn't GURL precent-escape spaces in paths? > > > > Yeah, it does :-J. Ok. What I think that means in terms of restrictions: > > * A cookie attribute value can create a value with a space (not that this > makes > > sense for the reason you mention, but it could be done), but not a space at > the > > beginning or end. > > * A path created from a URL will not have a space in it period. > > > > So I think the canonical test should be "no space at beginning or end, but > > spaces in middle allowed" with positive and negative tests as appropriate in > > CreationCornerCases and IsCanonical. SGTY? > > I'm wondering if it's worth the effort - I don't think there's a security issue > with paths that can never actually be matched? There may also be weird > restrictions (Other characters that can't be in values, and are percent-escaped > in URLs). Sure. My goal at the moment is just to get the fuzzer to shut up. So combining your response and that goal: I'll just remove the check against cookie value parsing for path from the IsCanonical check?
On 2017/06/28 17:41:21, Randy Smith (Not in Mondays) wrote: > https://codereview.chromium.org/2959123002/diff/20001/net/cookies/canonical_c... > File net/cookies/canonical_cookie_unittest.cc (right): > > https://codereview.chromium.org/2959123002/diff/20001/net/cookies/canonical_c... > net/cookies/canonical_cookie_unittest.cc:64: cookie = > CanonicalCookie::Create(GURL("http://fool/xyzzy wwwww/"), "A=B", > On 2017/06/28 17:13:48, mmenke wrote: > > On 2017/06/28 14:58:05, Randy Smith (Not in Mondays) wrote: > > > On 2017/06/28 00:26:20, mmenke wrote: > > > > Doesn't GURL precent-escape spaces in paths? > > > > > > Yeah, it does :-J. Ok. What I think that means in terms of restrictions: > > > * A cookie attribute value can create a value with a space (not that this > > makes > > > sense for the reason you mention, but it could be done), but not a space at > > the > > > beginning or end. > > > * A path created from a URL will not have a space in it period. > > > > > > So I think the canonical test should be "no space at beginning or end, but > > > spaces in middle allowed" with positive and negative tests as appropriate in > > > CreationCornerCases and IsCanonical. SGTY? > > > > I'm wondering if it's worth the effort - I don't think there's a security > issue > > with paths that can never actually be matched? There may also be weird > > restrictions (Other characters that can't be in values, and are > percent-escaped > > in URLs). > > Sure. My goal at the moment is just to get the fuzzer to shut up. So combining > your response and that goal: I'll just remove the check against cookie value > parsing for path from the IsCanonical check? Yes
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...
Ok, went with the pretty simple changes. PTAL?
On 2017/06/28 18:12:51, Randy Smith (Not in Mondays) wrote: > Ok, went with the pretty simple changes. PTAL? LGTM!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rdsmith@chromium.org
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": 40001, "attempt_start_ts": 1498678095949760,
"parent_rev": "8e15cc7cbff20011dbd0152d7ea141e5443b2241", "commit_rev":
"81abc648e801e288b9c43a5c409d2d7285444f98"}
Message was sent while issue was closed.
Description was changed from ========== Allow paths constructed from URLs rather as well as cookie attributes. BUG=734355 R=mmenke@chromium.org ========== to ========== Allow paths constructed from URLs rather as well as cookie attributes. BUG=734355 R=mmenke@chromium.org Review-Url: https://codereview.chromium.org/2959123002 Cr-Commit-Position: refs/heads/master@{#483114} Committed: https://chromium.googlesource.com/chromium/src/+/81abc648e801e288b9c43a5c409d... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/81abc648e801e288b9c43a5c409d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
