Curl cookie handling bug with spaces

[cromestant]

Hello,
I just ran into a wonky behavior in curl 's cookie handling when spaces exist in the cookie:
here is a gist with the trace:https://gist.github.com/cromestant/9452ad9912c27882867c

to reproduce I created two PHP files

//cookie.php

<?php
header("Set-Cookie: CHARZID=this is ma cookie; path=/");
header("Location: http://www.google.com");

and
//cookies2.php

<?php
header("Set-Cookie: CHARZID = this is ma cookie; path=/");
header("Location: http://www.google.com");

the only difference is the space in the cookie name=value .
I looked at the RFC and it seems like a valid cookie header.

If you curl it and use a cookie jar you will see that cookie 2 does not set the value
you can test here:
http://test dot tigocloud dot net slash cookie.php
and cookie2.php

[cromestant]

from IRC #curlusers
gevaerts: http://paste.debian.net/164323/ makes this work again
[12:05pm] gevaerts: That's a (very) partial revert of 7c21c1c

[gevaerts]

The following patch fixes this for me (but probably breaks the test suite and maybe some actual real things as well):

diff --git a/lib/cookie.c b/lib/cookie.c
index f1450e9..71a02db 100644
--- a/lib/cookie.c
+++ b/lib/cookie.c
@@ -407,7 +407,7 @@ Curl_cookie_add(struct SessionHandle *data,
     do {
       /* we have a <what>=<this> pair or a stand-alone word here */
       name[0]=what[0]=0; /* init the buffers */
-      if(1 <= sscanf(ptr, "%" MAX_NAME_TXT "[^;\r\n =]=%"
+      if(1 <= sscanf(ptr, "%" MAX_NAME_TXT "[^;\r\n=]=%"
                      MAX_COOKIE_LINE_TXT "[^;\r\n]",
                      name, what)) {
         /* Use strstore() below to properly deal with received cookie

From looking at the code, I think it worked before 7c21c1c (I can't easily test, autoconf issues with older builds). The code that's supposed to strip such whitespace is still there, so I assume the intent is to still support such whitespace.

[bagder]

Thanks, I'll write up a test case (or perhaps amend an existing) for this to first repeat it and then make sure it verifies the fix - while not breaking any existing test!

[bagder]

Unfortunately, only this patch is not enough. It breaks test 31. I'll work on a minor improvement.

[bagder]

Please try out this version. The fix is very similar to @gevaerts', and I also extended the test case to make sure it works with a name with a trailing space.

[bagder]

Presumed to be fixed, closing this issue!

[cromestant]

I can confirm upon building and testing that the issue is resolved.
Awesome turnaround!

[bagder]

Thanks for confirming

[markstos]

From reading the cookie grammar in RFC 6265, it appears that whitespace is not allowed around the "=" sign, or within the value.

Should Curl be accepting invalid cookies, or emitting a warning or error when it encounters an invalid formatted cookie?

[cromestant]

I might be minterpreting this, but I see

set-cookie-string = cookie-pair *( ";" SP cookie-av )

so cookie pair:

cookie-pair = cookie-name "=" cookie-value

cookie name is defined as token which is defined in
http://tools.ietf.org/html/rfc2616#section-2.2

which clearly includes spaces

and cookie value is defined as cookie octed with and without double quotes.

cookie-octet = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
; US-ASCII characters excluding CTLs,
; whitespace DQUOTE, comma, semicolon,
; and backslash

So in looking at it I do see whitepaces as valid...

On Tue, Apr 14, 2015 at 9:47 AM, Mark Stosberg notifications@github.com
wrote:

From reading the cookie grammar in RFC 6265
http://tools.ietf.org/html/rfc6265#section-4.1.1, it appears that
whitespace is not allowed around the "=" sign, or within the value.

Should Curl be accepting invalid cookies, or emitting a warning or error
when it encounters an invalid formatted cookie?

—
Reply to this email directly or view it on GitHub
#195 (comment).

MSc. Charles M. Romestant F.

Merci de penser à l'environnement avant d'imprimer cet e-mail
Please think about the environment before you print this e-mail
Por favor piense en el medio ambiente antes de imprimir este e-mail

[cromestant]

emphasis on whitepsace in cookie-octet added by my gmail sending.. sorryabout that

[markstos]

As you posted above, the cookie-octet is to contain: "US-ASCII characters excluding CTLs, whitespace DQUOTE, comma, semicolon, and backslash".

I read that as excluding whitespace in the cookie values.

[markstos]

The Mozilla Cookie Documentation is also clear that unencoded whitespace is now allowed in cookies. (Search for "white" on that page).

[cromestant]

I agree, I misread,
however looking at the documentation available, I see a few RFCs and
discussions that are very unclear (at best) but mostly conflicting
http://www.ietf.org/rfc/rfc2965.txt specifies "non-whitespace" characters,
but then states:

Attributes (names) (attr) are case-insensitive. White space is
permitted between tokens. Note that while the above syntax
description shows value as optional, most attrs require them.

"white spaces are permitted between tokens" which if looking at the
language def previously stated:

av-pair = attr ["=" value] ; optional value
attr = token

value = token | quoted-string

could mean that between the name and = and value... as name and value
are token..

In the end the thing that preocupies me the most:

• many apps send the space, and I've looked at lots of cookie parsing
  code that trims the spaces ( so expects them).[1]
• Curl code seems to be a reference on this issue, and CURL's doc
  states "no white spaces" (http://curl.haxx.se/rfc/cookie_spec.html)
  yet the code was in, and then removed and now back into the tested
  version.

=> we could break compatibility with this

So I 'd say leaving it in even though it is not "proper".

[1] Chromium code has this function:

void ParseRequestCookieLine(const std::string& header_value,
ParsedRequestCookies* parsed_cookies) {
std::string::const_iterator i = header_value.begin();
while (i != header_value.end()) {
// Here we are at the beginning of a cookie.

// Eat whitespace.
while (i != header_value.end() && *i == ' ') ++i;
if (i == header_value.end()) return;

On Tue, Apr 14, 2015 at 10:39 AM, Mark Stosberg notifications@github.com
wrote:

The Mozilla Cookie Documentation
https://developer.mozilla.org/en-US/docs/Web/API/Document/cookie is
also clear that unencoded whitespace is now allowed in cookies. (Search for
"white" on that page).

—
Reply to this email directly or view it on GitHub
#195 (comment).

MSc. Charles M. Romestant F.

Merci de penser à l'environnement avant d'imprimer cet e-mail
Please think about the environment before you print this e-mail
Por favor piense en el medio ambiente antes de imprimir este e-mail

[bagder]

"CURL's doc" as mentioned there is not at all curl's doc, it is the original Netscape spec for cookies and we use that only as a historic reference now (we host it only because the original hosting place went away and it would otherwise be gone). We have RFC 6265 to follow these days. I believe we now do like the browsers do.

RFC 2965 is not a spec anyone follows, and curl doesn't either. Ignore that.

[cromestant]

Great discussion, but what is the "official" policy on this in the end?
in or out, or in with a warning?

On Wed, Apr 15, 2015 at 2:48 PM, Daniel Stenberg notifications@github.com
wrote:

"CURL's doc" as mentioned there is not at all curl's doc, it is the
original Netscape spec for cookies and we use that only as a historic
reference now (we host it only because the original hosting place went away
and it would otherwise be gone). We have RFC 6265 to follow these days. I
believe we now do like the browsers do.

RFC 2965 is not a spec anyone follows, and curl doesn't either. Ignore
that.

—
Reply to this email directly or view it on GitHub
#195 (comment).

MSc. Charles M. Romestant F.

Merci de penser à l'environnement avant d'imprimer cet e-mail
Please think about the environment before you print this e-mail
Por favor piense en el medio ambiente antes de imprimir este e-mail

[bagder]

The policy is to A) follow the spec but also to B) do how "the others" do it - for maximum compatibility. Like the browsers. And they should all adhere to RFC 6265. We don't really have warnings in libcurl.

[cromestant]

great!
thanks for all.

On Wed, Apr 15, 2015 at 3:10 PM, Daniel Stenberg notifications@github.com
wrote:

The policy is to A) follow the spec but also to B) do how "the others" do
it - for maximum compatibility. Like the browsers. And they should all
adhere to RFC 6265. We don't really have warnings in libcurl.

—
Reply to this email directly or view it on GitHub
#195 (comment).

MSc. Charles M. Romestant F.

Merci de penser à l'environnement avant d'imprimer cet e-mail
Please think about the environment before you print this e-mail
Por favor piense en el medio ambiente antes de imprimir este e-mail