Add IncludeForRequestURL method to CanonicalCookie

net/cookies/canonical_cookie_unittest.cc

Index: net/cookies/canonical_cookie_unittest.cc
diff --git a/net/cookies/canonical_cookie_unittest.cc b/net/cookies/canonical_cookie_unittest.cc
index b46a3f2fa7950ab1b125f58190e02934f666d6ba..c21a39395d225e202d753cb66879b2a55318d101 100644
--- a/net/cookies/canonical_cookie_unittest.cc
+++ b/net/cookies/canonical_cookie_unittest.cc
@@ -250,4 +250,48 @@ TEST(CanonicalCookieTest, IsOnPath) {
   EXPECT_TRUE(cookie->IsOnPath("/test/sample/bar.html"));
 }
 
+TEST(CanonicalCookieTest, IncludeForRequestURL) {
+  GURL url("http://www.example.com");
+  base::Time creation_time = base::Time::Now();
+  CookieOptions options;
+
+  scoped_ptr<CanonicalCookie> cookie(
+      CanonicalCookie::Create(url, "A=2", creation_time, options));
+  EXPECT_TRUE(cookie->IncludeForRequestURL(url, options));
+  EXPECT_TRUE(cookie->IncludeForRequestURL(
+      GURL("http://www.example.com/foo/bar"), options));
+  EXPECT_TRUE(cookie->IncludeForRequestURL(
+      GURL("https://www.example.com/foo/bar"), options));
+  EXPECT_FALSE(cookie->IncludeForRequestURL(GURL("https://sub.example.com"),
+                                         options));

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

indent (also further in this method).

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

Done

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

+  EXPECT_FALSE(cookie->IncludeForRequestURL(GURL("https://sub.www.example.com"),
+                                         options));
+
+  // Test that cookie with a cookie path that does not match the url path are
+  // not included.
+  cookie.reset(CanonicalCookie::Create(url, "A=2; Path=/foo/bar", creation_time,

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

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?

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

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.

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

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

+                                       options));
+  EXPECT_FALSE(cookie->IncludeForRequestURL(url, options));
+  EXPECT_TRUE(cookie->IncludeForRequestURL(
+      GURL("http://www.example.com/foo/bar/index.html"), options));
+
+  // Test that a secure cookie is not included for a non secure URL.
+  GURL secure_url("https://www.example.com");
+  cookie.reset(CanonicalCookie::Create(secure_url, "A=2; Secure", creation_time,

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

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?

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

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.

+                                       options));
+  EXPECT_TRUE(cookie->IsSecure());
+  EXPECT_TRUE(cookie->IncludeForRequestURL(secure_url, options));
+  EXPECT_FALSE(cookie->IncludeForRequestURL(url, options));
+
+  // Test that http only cookies are only inlucded if the include httponly flag

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

inlucded (typo)

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

Done.

+  // is set on the cookie options.
+  options.set_include_httponly();
+  cookie.reset(
+      CanonicalCookie::Create(url, "A=2; HttpOnly", creation_time, options));

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

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]

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.

+  EXPECT_TRUE(cookie->IsHttpOnly());
+  EXPECT_TRUE(cookie->IncludeForRequestURL(url, options));
+  options.set_exclude_httponly();
+  EXPECT_FALSE(cookie->IncludeForRequestURL(url, options));
+}
+
 }  // namespace net