277033 - cookie parsing of Path and Max-Age attributes doesn't handle quoted values

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Tom Jackson]

I've tested this on the latest versions of Mozila (Trunk) on Linux and Firefox 
on Windows(1.0).  
 
If a server sends a Set-Cookie header of the form: 
 
mycookie=myvalue; Path="/"; Max-Age="100"; 
 
Several problems develop.  
First Problem: The browser saves the path attribute with the double quotes 
included. This makes the Path _never_ match a future request, so the cookie is 
never sent back to the server.  
Second Problem: The browser ignores the Max-Age value, so the cookie is viewed 
as a session cookie and isn't saved to disk.  
 
Although the Max-Age attribute really doesn't need to be quoted, the path 
component is an http_URL, which might contain things which need quoting, like 
';' or ','.   
 
If needed I can create a url which creates the bug, however it is easy to see 
the bug working if you can setup a server to send the above cookie. Once the 
cookie is sent.  
 
For testing, I would recommend leaving off the Max-Age attribute, so the 
browser stores the cookie. Once the cookie has been set, you can view the 
cookie information and _note_ the path is set with surrounding double quotes.  
 
Compare this result with valid cookies send from other sites.  
 
The original cookie spec is RFC-2109, a working link is: 
ftp://ftp.isi.edu/in-notes/rfc2109.txt 
 
If someone points me to the code, I could review it, and maybe produce a 
patch. 
 
Thanks!

Comment 1

[Frank Wein [:mcsmurf]]

(In reply to comment #0)
> mycookie=myvalue; Path="/"; Max-Age="100"; 
>  
> Several problems develop.  
> First Problem: The browser saves the path attribute with the double quotes 
> included. This makes the Path _never_ match a future request, so the cookie is 
> never sent back to the server.  

wfm, Mozilla sends back the unquoted path (so Path="/" is the same as Path=/ for
the cookie reponse from Mozilla).

> Second Problem: The browser ignores the Max-Age value, so the cookie is viewed 
> as a session cookie and isn't saved to disk.  

Same handling as Path.

> Although the Max-Age attribute really doesn't need to be quoted, the path 
> component is an http_URL, which might contain things which need quoting, like 
> ';' or ','.   

Also works here.

Can you create a cookie log, see
http://www.mozilla.org/projects/netlib/cookies/cookie-log.html for a how-to?

Comment 2

[Tom Jackson]

Attachment: requests one and two

I setup four cookies to cover all the cases. This attachment shows the cookie
log for these two requests. Browser was setup to accept all cookies, and all
cookies were removed prior to the test. The browser was restarted before the
requests were performed.

Comment 3

[Tom Jackson]

Attachment: request three, after browser restart

After requests one and two, the browser was restarted to clear session cookies.
This attachment is the cookie.log for the third request.

Comment 4

[Tom Jackson]

Was there any additional comment after I uploaded the cookie logs? They seem 
to confirm the behavior I was describing: only one cookie, with path and 
max-age unquoted, works as it should. All other cookies are incorrectly 
handled by mozilla.  

Comment 5

[Michiel van Leeuwen (email: mvl+moz@)]

There is code to include the quotes in the parse results:
http://lxr.mozilla.org/seamonkey/source/netwerk/cookie/src/nsCookieService.cpp#1441

I'm not sure what we should do. If the cookie value is quoted, should we send it
back unquoted? If not, we need to special case that.

Comment 6

[[e-mail]]

...and there is code to remove the quotes for this case, too:

http://lxr.mozilla.org/seamonkey/source/netwerk/cookie/src/nsCookieService.cpp#1441

I stared at it for a sec before realizing there is a bug in that code. That
string fu can be tricky :)

Comment 7

[[e-mail]]

Attachment: patch

we want to look at the last char, not the string terminator...

Comment 8

[Tom Jackson]

I haven't verified if the patch works, but it looks like you check the last    
char, at tempend, and then include that char in the following Rebind  
operation. Wouldn't that produce a result like /path/" instead of /path/?  
 
Shouldn't you back up one more char from the ending quote? 
 

Comment 9

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 171476 [details] [diff] [review]
patch

>+                              && *--tokenValue.EndReading(tempEnd) == '"') {

That line is pretty confusing. I need to think real hard to find the right
order in which the operators are called here. Can you expand it a bit, into 2
steps or something?
And where will tempEnd point to? Still the end of the string?

>+      tokenValue.Rebind(++tempBegin, tempEnd);

If so, doesn't this still include the quote?

Comment 10

[[e-mail]]

(In reply to comment #9)
> That line is pretty confusing. I need to think real hard to find the right
> order in which the operators are called here. Can you expand it a bit, into 2
> steps or something?
> And where will tempEnd point to? Still the end of the string?

Yeah, it's nicely minimalist ;) EndReading() returns a reference to an iterator
- the returned iterator and the outparam are the same, and the -- operation
occurs on that referenced iterator.

> If so, doesn't this still include the quote?

Nope.

Comment 11

[Tom Jackson]

One other thing is that when I viewed the cookie data, the path had quotes at 
the beginning and the end. Would this code address this, or just remove the 
ending quote? 

Comment 12

[[e-mail]]

Attachment: patch 2

Comment 13

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 171569 [details] [diff] [review]
patch 2

It is still a bit confusing, but better :)
(have to remember that the second param of Rebind point to the char after the
string. if i still got this right :/)

Comment 14

[[e-mail]]

Comment on attachment 171569 [details] [diff] [review]
patch 2

tom, this will fix the bug, because the |if| branch to deal with the quoted
case will now be taken.

Comment 15

[Tom Jackson]

(In reply to comment #14) 
> (From update of attachment 171569 [details] [diff] [review] [edit]) 
> tom, this will fix the bug, because the |if| branch to deal with the quoted 
> case will now be taken. 
 
Okay, thanks. It was not at all obvious where the problem was when I looked at 
the code, until you realize that the cookie value itself is stored with quotes. 
So, maybe one last question: is this going to lead to problems when sending the 
cookie back? Also, how can I test the bug fix? Does this fix make it into the 
nightly build at some point, or is there some other method I should use (or 
just grab via cvs and build)? 
 
 
 

Comment 16

[[e-mail]]

(In reply to comment #15)
> So, maybe one last question: is this going to lead to problems when sending the 
> cookie back?

No, read the code if you want to understand it :)

> Also, how can I test the bug fix? Does this fix make it into the 
> nightly build at some point, or is there some other method I should use (or 
> just grab via cvs and build)? 

It's already been tested by mvl, but you can test by pulling the trunk via cvs,
applying this patch, and building. When it gets checked in, I will close this bug.

Comment 17

[[e-mail]]

checked in, thanks guys!