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

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.

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; }

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.

see my previous comments

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. ***

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.

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. ***

"QUIT MESSING AROUND AND FIX THIS SPANKY" - (tm) Jevering

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(".

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.