Closed
Bug 277033
Opened 20 years ago
Closed 20 years ago
cookie parsing of Path and Max-Age attributes doesn't handle quoted values
Categories
(Core :: Networking: Cookies, defect)
Core
Networking: Cookies
Tracking
()
RESOLVED
FIXED
People
(Reporter: tom, Assigned: dwitte)
Details
Attachments
(3 files, 1 obsolete file)
6.04 KB,
text/plain
|
Details | |
3.13 KB,
text/plain
|
Details | |
1.37 KB,
patch
|
mvl
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
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•20 years ago
|
||
(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?
Reporter | ||
Comment 2•20 years ago
|
||
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.
Reporter | ||
Comment 3•20 years ago
|
||
After requests one and two, the browser was restarted to clear session cookies. This attachment is the cookie.log for the third request.
Reporter | ||
Comment 4•20 years ago
|
||
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•20 years ago
|
||
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.
Assignee | ||
Comment 6•20 years ago
|
||
...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 :)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 7•20 years ago
|
||
we want to look at the last char, not the string terminator...
Assignee: darin → dwitte
Assignee | ||
Updated•20 years ago
|
Attachment #171476 -
Flags: superreview?(darin)
Attachment #171476 -
Flags: review?(mvl)
Reporter | ||
Comment 8•20 years ago
|
||
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•20 years ago
|
||
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?
Assignee | ||
Comment 10•20 years ago
|
||
(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.
Reporter | ||
Comment 11•20 years ago
|
||
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?
Assignee | ||
Comment 12•20 years ago
|
||
Attachment #171476 -
Attachment is obsolete: true
Attachment #171569 -
Flags: review?(mvl)
Comment 13•20 years ago
|
||
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 :/)
Attachment #171569 -
Flags: review?(mvl) → review+
Assignee | ||
Updated•20 years ago
|
Attachment #171476 -
Flags: superreview?(darin)
Attachment #171476 -
Flags: review?(mvl)
Assignee | ||
Comment 14•20 years ago
|
||
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.
Attachment #171569 -
Flags: superreview?(darin)
Reporter | ||
Comment 15•20 years ago
|
||
(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)?
Assignee | ||
Comment 16•20 years ago
|
||
(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.
Updated•20 years ago
|
Attachment #171569 -
Flags: superreview?(darin) → superreview+
Assignee | ||
Comment 17•20 years ago
|
||
checked in, thanks guys!
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•