Add IncludeForRequestURL method to CanonicalCookie

Add IncludeForRequestURL method to CanonicalCookie.

Move the code for checking whether to include a cookie or not from the CookieMonster to the new method in CanonicalCookie. This means the new method IncludeForRequestURL returns true when the cookie should be included for the given |url| and cookie options.
Change the CookieMonster to use the IncludeForRequestURL method of CanonicalCookie to test whether a cookie should be included or not.

TEST=CanonicalCookieTest.IncludeForRequestURL  (net_unittests)
BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172668

[markusheintz_, 2012-11-29 15:16:14]

Hey Erik here is a rough sketch of who the CanonicalCookie UI could be changed.
WDYT?

The CL is missing unitestest yet.

https://codereview.chromium.org/11308272/diff/1/net/cookies/cookie_monster.cc
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/11308272/diff/1/net/cookies/cookie_monster.cc...
net/cookies/cookie_monster.cc:1619: continue;
I think I should even move the if above to the new "IncludeForRequest" method
and pass the options as additional param.

[erikwright (departed), 2012-11-30 05:15:31]

SGTM.

[markusheintz_, 2012-12-05 19:33:01]

Sorry for the delay on this one.

Could you please review the final version of this CL? Thanks a lot.

[erikwright (departed), 2012-12-05 19:43:36]

https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
File net/cookies/canonical_cookie.cc (right):

https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
net/cookies/canonical_cookie.cc:380: // unsecure scheme.
unsecure -> insecure

https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
net/cookies/canonical_cookie.cc:386: // Don't include cookies for requests that
incomplete comment.

https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
File net/cookies/canonical_cookie_unittest.cc (right):

https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
net/cookies/canonical_cookie_unittest.cc:266: options));
indent (also further in this method).

https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
net/cookies/canonical_cookie_unittest.cc:272:
cookie.reset(CanonicalCookie::Create(url, "A=2; Path=/foo/bar", creation_time,
is it legal to set a path that is not equal to or a child of the origin URL? If
not, this test is misleading (even if the test for that is done elsewhere).

Also, if not, is it more consistent to do that in CanonicalCookie::Create?

https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
net/cookies/canonical_cookie_unittest.cc:280:
cookie.reset(CanonicalCookie::Create(secure_url, "A=2; Secure", creation_time,
Same question. Presumably insecure URLs can't set secure cookies - if that's
right, can you confirm where the logic for that currently lives and whether it's
consistent with your changes?

https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
net/cookies/canonical_cookie_unittest.cc:286: // Test that http only cookies are
only inlucded if the include httponly flag
inlucded (typo)

https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
net/cookies/canonical_cookie_unittest.cc:290: CanonicalCookie::Create(url, "A=2;
HttpOnly", creation_time, options));
Same question about the options - do we use the options to prevent an HttpOnly
cookie from being set by Javascript, and if so, is that prevention occurring in
the correct place?

[markusheintz_, 2012-12-06 10:10:43]

https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
File net/cookies/canonical_cookie.cc (right):

https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
net/cookies/canonical_cookie.cc:380: // unsecure scheme.
On 2012/12/05 19:43:36, erikwright wrote:
> unsecure -> insecure

Done.

https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
net/cookies/canonical_cookie.cc:386: // Don't include cookies for requests that
On 2012/12/05 19:43:36, erikwright wrote:
> incomplete comment.

Done.

https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
File net/cookies/canonical_cookie_unittest.cc (right):

https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
net/cookies/canonical_cookie_unittest.cc:266: options));
On 2012/12/05 19:43:36, erikwright wrote:
> indent (also further in this method).
Done

Sorry, I renamed the Include.. method and forgot to fix the formatting.

https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
net/cookies/canonical_cookie_unittest.cc:272:
cookie.reset(CanonicalCookie::Create(url, "A=2; Path=/foo/bar", creation_time,
On 2012/12/05 19:43:36, erikwright wrote:
> is it legal to set a path that is not equal to or a child of the origin URL? 

The path of the URL "http://www.example.com" is "/". So "/foo/bar/" should be a
child of "/". Maybe I confuse something here.

Anyway. Currently Chrome happily accepts such cookies. I tried it with some test
server I quickly put together.
Chrome even accepts cookies set by http://www.example.com/test1
that have the cookie path  "Path=/foo/bar/blablabla"

Section 5.2.4 "The Path Attribute" of RCF 6265 says:

"If the attribute-name case-insensitively matches the string 'Path', the user
agent MUST process the cookie-av as follows. If the attribute-value is empty or
if the first character of the attribute-value is not %x2F ("/"):
      Let cookie-path be the default-path.
   Otherwise:
      Let cookie-path be the attribute-value.
   Append an attribute to the cookie-attribute-list with an attribute-name of
Path and an attribute-value of cookie-path."


Section 5.3 "Storage Model" of RFC 6265 says:

"7. If the cookie-attribute-list contains an attribute with an attribute-name of
"Path", set the cookie's path to attribute-        value of the last attribute
in the cookie-attribute-list with an attribute-name of "Path". Otherwise, set
the cookie's path to the default-path of the request-uri."

Step seven of the algorithm used for storing cookies does not require that the
cookie path and the URL path must be related in any way.

Please double check the algorithm Section 5.3 "Storage Model" of RFC 6265 just
in case I missed someting. But I think this is a totally valid case.

I probably should add a test case to the "CanonicalCookieTest.Create" test to
document this.



> If
> not, this test is misleading (even if the test for that is done elsewhere).
> 
> Also, if not, is it more consistent to do that in CanonicalCookie::Create?

https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
net/cookies/canonical_cookie_unittest.cc:280:
cookie.reset(CanonicalCookie::Create(secure_url, "A=2; Secure", creation_time,
On 2012/12/05 19:43:36, erikwright wrote:
> Same question. Presumably insecure URLs can't set secure cookies - if that's
> right, can you confirm where the logic for that currently lives and whether
it's
> consistent with your changes?

The way I read the spec is: It is possible to set secure cookies by insecure
URLs (See 8. of Section 5.3 of RFC 2656 ).

I also verified that Chrome currently allows that.

I should actually add a test case for this if I already haven't.

https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
net/cookies/canonical_cookie_unittest.cc:286: // Test that http only cookies are
only inlucded if the include httponly flag
On 2012/12/05 19:43:36, erikwright wrote:
> inlucded (typo)

Done.

https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
net/cookies/canonical_cookie_unittest.cc:290: CanonicalCookie::Create(url, "A=2;
HttpOnly", creation_time, options));
On 2012/12/05 19:43:36, erikwright wrote:
> Same question about the options - do we use the options to prevent an HttpOnly
> cookie from being set by Javascript, and if so, is that prevention occurring
in
> the correct place?

Yes we use the options to prevent an HttpOnly cookie from being set or read by
JavaScript. By default http only cookies are not included (see
net/cookies/cookie_option.*). For Cookies set by JavaScript the code path goes
through src/content/browser/render_host/render_message_filter.cc:515. 
In src/net/url_request/url_request_http_job.cc:627-629 we set cookies received
via http requests and user a CookieOptions object with the include httponly flag
set to true (see line 611).

So I think this check is happening in the right place because otherwise we have
no way to check inside the CookieStore or CanonicalCookie through which code
path a cookie was set.

[markusheintz_, 2012-12-10 16:19:42]

ping. I addressed your comments. Could you give this CL another look? 

On 2012/12/06 10:10:43, markusheintz_ wrote:
>
https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
> File net/cookies/canonical_cookie.cc (right):
> 
>
https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
> net/cookies/canonical_cookie.cc:380: // unsecure scheme.
> On 2012/12/05 19:43:36, erikwright wrote:
> > unsecure -> insecure
> 
> Done.
> 
>
https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
> net/cookies/canonical_cookie.cc:386: // Don't include cookies for requests
that
> On 2012/12/05 19:43:36, erikwright wrote:
> > incomplete comment.
> 
> Done.
> 
>
https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
> File net/cookies/canonical_cookie_unittest.cc (right):
> 
>
https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
> net/cookies/canonical_cookie_unittest.cc:266: options));
> On 2012/12/05 19:43:36, erikwright wrote:
> > indent (also further in this method).
> Done
> 
> Sorry, I renamed the Include.. method and forgot to fix the formatting.
> 
>
https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
> net/cookies/canonical_cookie_unittest.cc:272:
> cookie.reset(CanonicalCookie::Create(url, "A=2; Path=/foo/bar", creation_time,
> On 2012/12/05 19:43:36, erikwright wrote:
> > is it legal to set a path that is not equal to or a child of the origin URL?

> 
> The path of the URL "http://www.example.com" is "/". So "/foo/bar/" should be
a
> child of "/". Maybe I confuse something here.
> 
> Anyway. Currently Chrome happily accepts such cookies. I tried it with some
test
> server I quickly put together.
> Chrome even accepts cookies set by http://www.example.com/test1
> that have the cookie path  "Path=/foo/bar/blablabla"
> 
> Section 5.2.4 "The Path Attribute" of RCF 6265 says:
> 
> "If the attribute-name case-insensitively matches the string 'Path', the user
> agent MUST process the cookie-av as follows. If the attribute-value is empty
or
> if the first character of the attribute-value is not %x2F ("/"):
>       Let cookie-path be the default-path.
>    Otherwise:
>       Let cookie-path be the attribute-value.
>    Append an attribute to the cookie-attribute-list with an attribute-name of
> Path and an attribute-value of cookie-path."
> 
> 
> Section 5.3 "Storage Model" of RFC 6265 says:
> 
> "7. If the cookie-attribute-list contains an attribute with an attribute-name
of
> "Path", set the cookie's path to attribute-        value of the last attribute
> in the cookie-attribute-list with an attribute-name of "Path". Otherwise, set
> the cookie's path to the default-path of the request-uri."
> 
> Step seven of the algorithm used for storing cookies does not require that the
> cookie path and the URL path must be related in any way.
> 
> Please double check the algorithm Section 5.3 "Storage Model" of RFC 6265 just
> in case I missed someting. But I think this is a totally valid case.
> 
> I probably should add a test case to the "CanonicalCookieTest.Create" test to
> document this.
> 
> 
> 
> > If
> > not, this test is misleading (even if the test for that is done elsewhere).
> > 
> > Also, if not, is it more consistent to do that in CanonicalCookie::Create?
> 
>
https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
> net/cookies/canonical_cookie_unittest.cc:280:
> cookie.reset(CanonicalCookie::Create(secure_url, "A=2; Secure", creation_time,
> On 2012/12/05 19:43:36, erikwright wrote:
> > Same question. Presumably insecure URLs can't set secure cookies - if that's
> > right, can you confirm where the logic for that currently lives and whether
> it's
> > consistent with your changes?
> 
> The way I read the spec is: It is possible to set secure cookies by insecure
> URLs (See 8. of Section 5.3 of RFC 2656 ).
> 
> I also verified that Chrome currently allows that.
> 
> I should actually add a test case for this if I already haven't.
> 
>
https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
> net/cookies/canonical_cookie_unittest.cc:286: // Test that http only cookies
are
> only inlucded if the include httponly flag
> On 2012/12/05 19:43:36, erikwright wrote:
> > inlucded (typo)
> 
> Done.
> 
>
https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
> net/cookies/canonical_cookie_unittest.cc:290: CanonicalCookie::Create(url,
"A=2;
> HttpOnly", creation_time, options));
> On 2012/12/05 19:43:36, erikwright wrote:
> > Same question about the options - do we use the options to prevent an
HttpOnly
> > cookie from being set by Javascript, and if so, is that prevention occurring
> in
> > the correct place?
> 
> Yes we use the options to prevent an HttpOnly cookie from being set or read by
> JavaScript. By default http only cookies are not included (see
> net/cookies/cookie_option.*). For Cookies set by JavaScript the code path goes
> through src/content/browser/render_host/render_message_filter.cc:515. 
> In src/net/url_request/url_request_http_job.cc:627-629 we set cookies received
> via http requests and user a CookieOptions object with the include httponly
flag
> set to true (see line 611).
> 
> So I think this check is happening in the right place because otherwise we
have
> no way to check inside the CookieStore or CanonicalCookie through which code
> path a cookie was set.

[erikwright (departed), 2012-12-11 14:09:21]

LGTM. Thank you!

https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
File net/cookies/canonical_cookie_unittest.cc (right):

https://codereview.chromium.org/11308272/diff/9002/net/cookies/canonical_cook...
net/cookies/canonical_cookie_unittest.cc:272:
cookie.reset(CanonicalCookie::Create(url, "A=2; Path=/foo/bar", creation_time,
On 2012/12/06 10:10:43, markusheintz_ wrote:
> On 2012/12/05 19:43:36, erikwright wrote:
> > is it legal to set a path that is not equal to or a child of the origin URL?

> 
> The path of the URL "http://www.example.com" is "/". So "/foo/bar/" should be
a
> child of "/". Maybe I confuse something here.

No, I was confused, I meant not a parent, but your response answers my intended
question.

> 
> Anyway. Currently Chrome happily accepts such cookies. I tried it with some
test
> server I quickly put together.
> Chrome even accepts cookies set by http://www.example.com/test1
> that have the cookie path  "Path=/foo/bar/blablabla"
> 
> Section 5.2.4 "The Path Attribute" of RCF 6265 says:
> 
> "If the attribute-name case-insensitively matches the string 'Path', the user
> agent MUST process the cookie-av as follows. If the attribute-value is empty
or
> if the first character of the attribute-value is not %x2F ("/"):
>       Let cookie-path be the default-path.
>    Otherwise:
>       Let cookie-path be the attribute-value.
>    Append an attribute to the cookie-attribute-list with an attribute-name of
> Path and an attribute-value of cookie-path."
> 
> 
> Section 5.3 "Storage Model" of RFC 6265 says:
> 
> "7. If the cookie-attribute-list contains an attribute with an attribute-name
of
> "Path", set the cookie's path to attribute-        value of the last attribute
> in the cookie-attribute-list with an attribute-name of "Path". Otherwise, set
> the cookie's path to the default-path of the request-uri."
> 
> Step seven of the algorithm used for storing cookies does not require that the
> cookie path and the URL path must be related in any way.
> 
> Please double check the algorithm Section 5.3 "Storage Model" of RFC 6265 just
> in case I missed someting. But I think this is a totally valid case.
> 
> I probably should add a test case to the "CanonicalCookieTest.Create" test to
> document this.
> 
> 
> 
> > If
> > not, this test is misleading (even if the test for that is done elsewhere).
> > 
> > Also, if not, is it more consistent to do that in CanonicalCookie::Create?
>

[commit-bot: I haz the power, 2012-12-12 13:25:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markusheintz@chromium.org/11308272/19001

[commit-bot: I haz the power, 2012-12-12 19:54:17]

Retried try job too often on win_rel for step(s) base_unittests,
cacheinvalidation_unittests, check_deps, content_unittests, crypto_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, net_unittests, ppapi_unittests, printing_unittests,
remoting_unittests, sql_unittests, sync_unit_tests, unit_tests

[M-A Ruel, 2012-12-12 20:24:19]

On 2012/12/12 19:54:17, I haz the power (commit-bot) wrote:
> Retried try job too often on win_rel for step(s) base_unittests,
> cacheinvalidation_unittests, check_deps, content_unittests, crypto_unittests,
> gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
> media_unittests, net_unittests, ppapi_unittests, printing_unittests,
> remoting_unittests, sql_unittests, sync_unit_tests, unit_tests

This is wrong. I'll take a look later to figure out why it happened. I just
checked the CQ bit again to see. Will debug a tad later tonight.

[commit-bot: I haz the power, 2012-12-12 20:27:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markusheintz@chromium.org/11308272/19001

[commit-bot: I haz the power, 2012-12-12 21:56:57]

Change committed as 172668