Closed Bug 11630 Opened 25 years ago Closed 25 years ago

[BLOCKER] cookie problem - unable to set multiple cookies in one response

Categories

(Core :: Networking, defect, P3)

Product:
Core
Component:
Networking
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: hshaw, Assigned: jud)

References

(
URL
)

Details

The following needs to be fixed to enable e-commerce sites.

The parsing code in

mozilla/extensions/cookie/nsCookie.cpp:cookie_SetCookieString()

assumes only one cookie will be set.

During login/logout, the CDW site sets 4 or more cookies, whose
HTTP headers look like

  Set-Cookie: USER%5FNAME=HSHAW; path=/
  Set-Cookie: ID=2Q7P7Q8X; expires=Wed, 01-Jan-2020 06:00:00 GMT; path=/
  Set-Cookie: company=01; path=/
  Set-Cookie: program=0; path=/

By the time this information gets passed to cookie_SetCookieString()
it looks like

  USER%5FNAME=HSHAW; path=/, ID=2Q7P7Q8X; expires=Wed, 01-Jan-2020 06:00:00
GMT; path=/, company=01; path=/, program=0; path=/

The parsing code in cookie_SetCookieString() will only recognize the
first cookie value, in this case, USER%5FNAME=HSHAW

The parsing code needs to be modified to recognize a comman separated
list of cookies.

As a side note, the comma separated list that necko is passing to the
cookie code is valid cookie syntax per RFC 2109

http://andrew2.andrew.cmu.edu/rfc/rfc2109.html

- Hubie
Summary: cookie problem - unable to set multiple cookies in one request → cookie problem - unable to set multiple cookies in one response
Do I conclude that in this case netlib was making four calls to
cookie_SetCookieString whereas in necko you have changed this to be one
concatenated call instead?
Am I correct in saying that this is for http cookies only -- javascript cookies
will never need this parsing.  In that case the additional parsing should not be
done in cookie_SetCookieString (which is already cumbersome enough) but rather
in the public routine COOKIE_SetCookieStringFromHttp.  That public routine is
currently doing some pre-processing and then calling the local
cookie_SetCookieString routine.  Right now the public routine is calling the
local routine once, but under this change it should call it once for each of the
comma-separated strings.

Furthermore, the public routine itself is already doing some processing of the
fields (the expire field in particular), so this further necessitates that the
change be done in the public routine and not in the private one.
Steve, Set-Cookie headers can contain multiple cookies. It's conceivable that
js cookie setting could do the same (if the api permits it). I think this
parsing change should occur in the core parsing code.
Judson is right, it could have came directly from the server in comma separated
form in one 'Set-Cookie' header as opposed to coming from Necko.  If there is a
way to set multiple cookies at once in Javascript, then it needs to be in a
routine used by both http and javascript, unless Javascript internally
serialized the cookie setting and calls you multiple times, once for each
cookie.
If what you are saying is correct, then this bug existed in our browser from day
1 and is a problem in our current 4.x browser as well.  Is that correct?  Can
you give me the URL of a site that sends back the cookies in such a format?
Does the 4.x browser handle that site correctly?
Off the top of my head I don't have a site which does this from the server
side, but I do remember writing server side code that did it this way
(multiple cookies for a single Set-Cookie header) back in the netscape 1.0
days.

If you look at section 4.2.2 in the cookie RFC, it
mentions that a comma separated list of cookies is
valid syntax.

http://andrew2.andrew.cmu.edu/rfc/rfc2109.html

It's possible that 4.x doesn't process this correctly,
but I haven't checked.  You can setup a test case using
the data I provided and see how 4.x behaves.
The point I'm trying to make is that if this is the way things always worked,
then this bug is of very low priority.  Upon first reading of this bug report,
it appeared as though this was a change that necko was making in that it batched
up several cookie requests and sent them all in one string to the cookie module.
In that case this would have been a high priority bug and possiblhy an M9
showstoper.  But now I realize that this is not the case and it is very low
priority.
Status: NEW → ASSIGNED
Target Milestone: M12
Setting target milestone to M12
> Upon first reading of this bug report, it appeared as though this was
> a change that necko was making in that it batched up several cookie
> requests and sent them all in one string to the cookie module.

This is exactly what happens (necko batches).  We were just pointing out
that, in addition, even if necko didn't do it, it could naturally occur.

I didn't look at what netlib used to do, so I can't comment on whether
the batching is a necko innovation or not.

> In that case this would have been a high priority bug and possiblhy
> an M9 showstoper.

I think it's more important than M12.  Hofmann and you guys are a
better judge of whether it's M9 material.

- Hubie
OK, I think I have a trivial fix for this problem.  But I don't have any test
cases to try it on.  Could one of you generate a URL that I can use for testing.

Also, if someone would like to code-review the change, I'd appreciate it.
Here's the code that I will add to the head of COOKIE_SetCookieStringFromHttp.
Note that I simply have the routine call on itself recursively.

  /* allow for multiple cookies separated by commas */
  char *comma = PL_strchr(setCookieHeader, ',');
  if(comma) {
    *comma = '\0';
    COOKIE_SetCookieStringFromHttp(curURL, setCookieHeader, server_date);
    *comma = ',';
    setCookieHeader = comma+1;
  }
Oops, let's try again.  Here's a revised change.

 /* allow for multiple cookies separated by commas */
  char *comma = PL_strchr(setCookieHeader, ',');
  if(comma) {
    *comma = '\0';
    COOKIE_SetCookieStringFromHttp(curURL, setCookieHeader, server_date);
    *comma = ',';
    COOKIE_SetCookieStringFromHttp(curURL, comma+1, server_date);
    return;
  }
The rfc 2109 says that the Set-Cookie response header comprises the token Set-
Cookie:, followed by a comma-separated list of one or more cookies. But the
expired attribute could also have a a comma in its value. for e.g.
Set-Cookie: ID=2Q7P7Q8X; expires=Wed, 01-Jan-2020 06:00:00 GMT; path=/.
We need to handle this case too.
Now that's interesting.  The latest cookie spec doesn't even have an expires
atrribute.  Instead it has a max-age attribute which simply takes a numeric
value (number of seconds) and so contains no commas.

So which spec are we implementing to?
> 10.1.2 Expires and Max-Age
>
> Netscape's original proposal defined an Expires header that took a
> date value in a fixed-length variant format in place of Max-Age:
>
>   Wdy, DD-Mon-YY HH:MM:SS GMT
>
> Note that the Expires date format contains embedded spaces, and that
> "old" cookies did not have quotes around values. Clients that implement
> to this specification should be aware of "old" cookies and Expires.

There is a note regarding the Expires header.

IMO the important thing is cookies that are in use today and that
necko is passing to the cookie module work.  That's what we should
concentrate on right now.

Bringing the cookie parsing up to spec is not as important right
now, especially if nobody (or very few people) use the new syntax.

Neeti is right, solely searching for comma is not a good way
to separate the different cookies.  Unfortunately due to the
way the original cookie spec was designed, parsing comma
separated cookies is not as easy as you'd believe.

It's significantly easier to process the cookies before they
get merged together.
After a lengthy discussion about this with Hubie, I have now come to the
conclusion that the cookie module should not be changed and that necko should
not batch cookies up to be passed as a single string.  Let me give my
justification.

First, I agree that we are not compliance with the cookie spec which says that
sites can send a concatenated cookie.  But that's how the 4.x browser works and
nobody has complained about it -- so apparently there is no site for which we
fail (if there was, the site probably dropped the concatenated cookie.

Second, if we make the change to our cookie parsing, then we would break a site
that sends a cookie having the name A and the value "B, C=D".  In other words,
the cookie string would be of the form "A=B, C=D".  Can a site currently send
such a cookie, even though the spec says that space and comma would need to be
quoted in order to appear within a cookie value?  The answer is yes -- Hubie and
I looked at our own cookie files and he found examples of commas in values and I
found examples of spaces (and the space examples came from mozilla.org).  So
currently a site could be sending out comma-space in the cookie value and we
would be intepreting it as the site intended.  By changing the cookie parsing
now, we would break that site.

I've already gone through a year-long exersize of fixing the cookie
implementation so that the host a.b.co.nz cannot set a cookie for .co.nz
(because that was in violation of the cookie spec).  The downside was that we
broke yahoo.mail who indeed was violating the cookie spec and consequently we
had to back out the change.  I'd hate to repeat this exersize again.  The safest
solution is for necko to change and not concatenate cookies.
Well, although I've already given arguments as to why we shouldn't change the
cookie module, here is a cleaner version of the code that I would use if we
decide to do otherwise.  It's yet another revision of my patch above but uses a
while loop so that the level of recursion is never greater than one.  Of course
the test for comma would have to be more complicated to reject for the comma
within the expires field, as Neeti pointed out.

  /* allow for multiple cookies separated by commas */
  char *comma;
  while if(comma = PL_strchr(setCookieHeader, ',')) {
    *comma = '\0';
    COOKIE_SetCookieStringFromHttp(curURL, setCookieHeader, server_date);
    *comma = ',';
    setCookieHeader = comma+1;
  }
Assignee: neeti → valeski
Status: ASSIGNED → NEW
Since nobody from necko has given arguments agains fixing this in necko rather
than in cookie code, I'm assigning this bug to necko team and changing the
component to necko.  Basically necko should not be concatening cookie strings.
Assignee: valeski → morse
see my previous comments
Component: Cookies → Necko
Target Milestone: M12 → M9
Judson, did you see my comments?  How do you propose we change the cookie code
and not break existing sites?  The only safe solution that I can think of is not
to accept concatenated cookies, just as we always have been doing.  Or

I don't want to play football with this bug and pass it back and forth.
So please present a safe counter-proposal if you can, and I will gladly
implement it.  Otherwise please change necko.
Guys is this M9 material. Looks like M10'able.
Does necko always batch cookie requests for sites that set more than one
cookie? If so, then until this bug is fixed we will break every site that sets
more than one cookie.  How can we ship M9 with such a bug.  Or am I
misunderstanding something here?
Yes, currently necko passes multiple set-cookie request headers as a
single ',' separated string of multiple cookies.  Any site that returns
more than one Set-Cookie header will have difficulty getting the cookies
set.

With the assumptions of the current cookie parsing code, a-v pairs like
'expires=' and 'domain=' could be associated with the wrong cookie variable.

This makes shopping at many sites somewhat difficult.
*** Bug 12005 has been marked as a duplicate of this bug. ***
Assignee: morse → dp
I got the overall picture here.

Necko is putting together multiple cookie headers into a single one because it
makes life simpler for it. Is that true. If so, fine. The cookie module can un
strip each one.

But shouldn't necko be quoting the individual headers to protect for the
separator that *necko* is adding.

If we agree to all the above, then the fix would look like:
Cookie module: unquote and strip individual cookie headers
Necko: quote when putting together multiple headers
There are two cookie specs: The netscape cookie spec (the only one currently
implemented (and only partially implemented at that)), and 2109. The only 2109
implementations are test beds. Although we should be ready for 2109 as it fixes
some important holes in the old netscape cookie spec, we need to make sure we
don't break the world with a change in our cookie parsing.

Our original (4.x-) cookie implementation couldn't handle multiple cookies in a
single set-cookie header. Subsequently we weren't coded to "spec".

Necko condenses multiple headers into a single header using HTTP header
condensing rules and delimiter tokens. We did this not for ease of coding in
necko, but to follow http header parsing more closely.

I know how dirty the cookie setting code is, but we should fix this there. We
wouldn't be breaking existing functionality, and we'd be providing a cookie
implementation that is more up to date.

I'm not clear on the problem w/ handling the comma seperated list. We should
never hit the expires/max-age because the list is separated from the attributes
by the ';' token.
Jud could you give parsing logic for a simplified form of hubie's example:

  Set-Cookie: USER%5FNAME=HSHAW; path=/
  Set-Cookie: ID=2Q7P7Q8X; expires=Wed, 01-Jan-2020 06:00:00 GMT; path=/
  Set-Cookie: company=01; path=/

USER%5FNAME=HSHAW; path=/, ID=2Q7P7Q8X; expires=Wed, 01-Jan-2020 06:00:00 GMT;
path=/, company=01; path=/

When would the parsing logic look for ; and when for ,  If the cookie module
knew that from the above, it would be glad to implement it. The best I can come
up with is the largest pattern that matches [^=,;]+=[^=]+[,;]

One premise I am running with is breaking old behaviour would be bad. I dont
know if , in the expires is valid per spec or not. But if 4.6 supported it, we
should try our best to.
Steve mentioned this somewhere above...

The difficulty with parsing based on ',' is that the value a cookie
may have, includes space and comma as valid characters.

By examining our cookie file it appears that sites use space and ','
in their cookie values without quoting them as the current spec mandates.

Without quoting the cookie value it is impossible to deterministically
tell whether the ',' is part of the cookie value, or a separator between
multiple cookies.

With some heuristics and assumptions, we could probably get away with
very good approximation of the old parsing code, but there are cases
(possibly artificial?) where it is still ambiguous.

If sites quoted there cookie values, there wouldn't be any ambiguity
problem, but most sites don't do this (at least that's the appearance
based on our cookie file)

Also, to address DP, the current cookie spec allows for servers to
set multiple cookies in a single header by separating them by commas.
They could naturally occur in this form even if necko did something
different.  The counter-argument is most sites don't seem to be setting
cookies this way.

The comma is valid in the expires field, but that shouldn't be
a problem for parsing, just makes it more complicated.  The main
problem is the cookie value isn't usually quoted, and a comma
in the cookie value would be impossible to distinguish from a
multiple cookie separator unless you use heuristics and make
assumptions

So, either solve it with cookie parsing or solve it by changing
what necko passes on, but do it some way, because setting multiple
cookies is essential for lots of sites where you login, including
e-commerce.

- Hubie
Don't talk about the expires field, that's not the issue.  Hubie just explained
it correctly.  The problem is in the cookie value itself.  See my example above.
Jud, do you understand the issues. Would you prefer to talk this out. Let me
know.

If necko gave every cookie header separately, then we can retain the old parsing
code in the cookie module. There is a lot of value to doing that. We pretty much
ASSURE backward compatibility. Fixing multiple cookies in a header would be a
feature that the cookie module could then implement.
dp, I agree with everything you just said except for the last sentence.  How can
the cookie module add this as a feature?  To do so would break backwards
compatibility.

It's the same as the issue with a.b.co.nz setting cookies for the .co.nz domain.
This is in violation of the cookie spec, but to fix it in the browser will (and
did) break backwards compatibility and we were forced to back it out.
Steve i hear you. let us postpone that discussion to later. I assure you that
our priorities will be:

1. Maintain backwards compatibility
2. Implement additional cookie features

I want to get point (1) fixed for M9. We are broken now.
Just to clarify, in case this is still not clear.  Judson wrote above:

   "I know how dirty the cookie setting code is, but we should fix this there."

The issue has nothing at all to do with how clean or dirty the cookie code is.
In fact, the patch would be very clean, as I described above.  THIS IS NOT THE
ISSUE!!!

  "We wouldn't be breaking existing functionality,"

We absolutely would.  We would break any site that is currently violating the
cookie spec and setting a cookie named "A" to have the value "B, C=D".

  "and we'd be providing a cookie implementation that is more up to date."

We have already declined from making our implementation be "more up to date" in
regard to domain cookies because it broke backwards compatibility.  This will do
the same.
Assignee: dp → valeski
I talked to jud. We decided the safest and surest way to fix this would be to
not condense cookie headers.

Jud was going to talk talk to gagan and rpotts to take care of this.
Reassigning. (thanks jud).

I think we got to fix this for M9.
*** Bug 12093 has been marked as a duplicate of this bug. ***
*** Bug 12093 has been marked as a duplicate of this bug. ***
*** Bug 12093 has been marked as a duplicate of this bug. ***
*** Bug 12093 has been marked as a duplicate of this bug. ***
*** Bug 12093 has been marked as a duplicate of this bug. ***
*** Bug 12093 has been marked as a duplicate of this bug. ***
Summary: cookie problem - unable to set multiple cookies in one response → [BLOCKER] cookie problem - unable to set multiple cookies in one response
Severity: normal → blocker
"QUIT MESSING AROUND AND FIX THIS SPANKY" - (tm) Jevering
Status: NEW → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
fix checked in 8/20/99, 11:38am pac time, into SeaMonkey_M9_BRANCH.

I made two changes:
1. I checked in Steve's code to recursively set cookies parsed out of a new line
delimited cookie list.
2. HTTP still condenses the cookie headers, but I've special cased (this really
sucks) "set-cookie" so that it uses the new line char when appending/condensing
multiple set-cookie headers.
Jud, I haven't seen your changes, but from your description, it sounds like it
should work.  Of course if you took my code as presented in the bug report, I
presume you fixed up the error that I just noticed -- I had "while if(" when of
course I meant just "while(".
Status: RESOLVED → VERIFIED
I verified the basic functionality of this using bugzilla, which sets both the
login name and password as a cookie in one response. It now works on the M9
branch, all platforms, though continues to be broken on M10 branch, which is to
be expected. Marking verified.
Bulk move of all Necko (to be deleted component) bugs to new Networking

component.
You need to log in before you can comment on or make changes to this bug.