145492 - Sending out duplicate cookies for domains with and without period

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect)

Description

[Stephen P. Morse]

BACKGROUND:

If a set-cookie header received from a site contains a domain attribute and the 
domain does not start with a dot, that is in violation of the cookie spec.  The 
netscape browsers used to store such cookies just as they were received from the 
site -- i.e., without the dot.  But for reasons discussed in bug 130752, we 
recently changed that and now automatically append a dot before storing the 
cookie if it is missing.


PROBLEM:

Old versions of the browser (prior to the fix in bug 130752) would store cookies 
without the leading dot.  New versions of the browser (post bug 130752) would 
store cookies with the leading dot, resulting in two cookies that are identical 
except for the dot.  On future returns to the site, both cookies are being sent.

See bug 130752 comment 16 for a case in which the problem occurs.


SOLUTIONS:

This isn't going to be pretty.  Here are some alternatives along the the pros 
and cons of each.

1 Fix the cookie file every time it is read in so that it does not contain any 
cookies that differ only in the leading dot.  The disadvantage is that we are 
going to be adding time to every browser startup just to fix a problem that once 
existed in legacy code.

2. Same as 1 but keep track of the fact that such fix-up occured in the past and 
don't waste time redoing it ever again.  Disadvantage is that the code for the 
fixup will exist forever adding bloat to the browser even though it is executed 
only once.

3. Write an external fix-up utility that cleans up this error from legacy cookie 
files.  Disadvantage is that the burdon now falls on the user to run this 
utility and most users won't know that they have to do so.

Comment 1

[Stephen P. Morse]

One more solution is to release-note this and tell the users to use the cookie 
manager to locate and delete such duplicate cookies.  That also has the 
disadvantage of putting the burden on the user.

Comment 2

[Peter Trudelle]

Nav triage team needs info: Why would users care about, or even notice 2 cookies
being sent?

Comment 3

[Stephen P. Morse]

Below is praveen's comment from bug 130752 which is what started the whole 
thing.  He says that this confuses the site.  Reading between the lines I assume 
that the problem is the following:

1. user visits site with old browser, gets cookie with no dot
2. user updates browser
3. user visits site again and browser sends old cookie
4. site sees old cookie and decides that it needs to be updated
5. site sends back a revised value with no dot
6. browser now appends dot and considers this as a separate cookie
7. user visits site again and browser sends both cookies
8. site sees the old (unmodified) cookie and decides that it needs to be updated

So at this time we get back to the condition in step 5 and will keep cycling 
through steps 5, 6, 7, and 8.  The site keeps attempting to update the cookie 
and keeps seeing the original cookie that hasn't been updated.  As far as tht 
site is concerned, the users state has never advanced.

Praveen, have I captured the essense of the problem correctly?  Is this indeed 
what is causing SNS not to work?

--------

[praveen's comment in bug 130752]

Sorry just noticed - this fix is causing problems if the users have cookies from 
the previous version  of browser. For example, if you run Netscape 4.7 and login 
to my.screenname.aol.com - SNS sets all the cookies (including permamnent 
cookies) on the screenname.aol.com domain and the browser accepts them. At that 
point if the user upgrades to the new version of mozilla (RC2), all the cookies 
are carried over. If you login to SNS again from the new browser - now you will 
have 2 sets of cookies - one on ".screenname.aol.com" and the other on 
"screenname.aol.com" (old cookies). This is confusing SNS because the browser is 
sending both cookies if you goto my.screenname.aol.com.
So I think you guys should do something to support the users with old cookies. 
May be automatically add '.' to the migrated cookies or just wipe them off.
thanks much
- Praveen

Comment 4

[Praveen Alavilli]

yes - that's correct - SNS writes some permanent cookies to remember some user 
information. So if a user using a old browser updates to the new version  and 
visits SNS again - SNS tries to update those cookies (refresh for the same/new 
user who gets authenticated). The new cookies instead of overwriting the old 
ones (as Morse described) exist as a different set of cookies. After that if the 
 user comes back to SNS - SNS gets both cookies and depending upon the order in 
which they are sent - it may either get old / new cookies. If it gets old 
cookies - the whole process starts again.

Comment 5

[Stephen P. Morse]

Giving some more thought to the various alternatives that I presented, here is 
how I would fix this if given the nsbeta1+

1. As each cookie is read in, check for a domain cookie not having a leading 
dot.

 - if leading dot is found, add cookie to cookie list as normal
 - if no leading dot is found, insert a dot and add cookie to a temporary list

2. After all cookies are read in, fetch the cookies from the temporary list and 
add them to the cookie list, testing for duplicates.  Code that tests for 
duplicates before adding cookies already exists.

This way we don't waste any time (other than checking for the leading dot) each 
time we read in a good cookie file.  And most of the added code necessary for 
rejecting duplicate cookies from a bad cookie list already exists so there is 
minimal bloat added.

Comment 6

[Samir Gehani]

Nav triage team: nsbeta1+, adt2

Comment 7

[Jaime Rodriguez, Jr.]

This needs to be addressed for 1.0.1 RC1. - Changing impact to [ADT1 RTM]. See
bugscape 15899 (http://bugscape.mcom.com/show_bug.cgi?id=15899) for more details

Comment 8

[Bob Clary [:bc] (inactive)]

*** Bug 144652 has been marked as a duplicate of this bug. ***

Comment 9

[Kathleen :Brade]

I defer to all of the experts in this bug (I'm no cookie expert).  However, can
we just send only one cookie (preferring the one with a dot over the one without
the dot)?  If that would work then we wouldn't really have to worry about
"cleanup" (although it'd be good to do).

Comment 10

[Jaime Rodriguez, Jr.]

Changing impact back to adt2 RTM, as the bugscape bug 15899
(http://bugscape.mcom.com/show_bug.cgi?id=15899) was found to be invalid.

Comment 11

[Stephen P. Morse]

brade: To send just one cookie means that every time we generate a list of 
cookies to be sent we need to make sure that the list contains no duplicates.  
This will be adding overhead to every page fetch forever and ever.  If the 
cleanup is done, we add the overhead only once.  Furthermore, the cleanup as I 
described above would take no more time (and less code) than the check for 
duplicate cookies at page-fetch time.

Comment 12

[Stephen P. Morse]

On second thought (actually third thought), there's an even simpler fix for 
this.  When reading in the cookies, if we find a bad legacy cookie (one whose 
domain name doesn't start with a dot, we simply ignore it.  That way there is 
never any such duplicate cookies in the list.

The advantage is that there is almost no overhead (except for a trivial test) 
added because of this legacy cookie.  The disadvantage is that the user who has 
such legacy cookies would have to obtain those cookies again (might involve 
relogging in at some affected website).  But that would be a one-time thing and 
the user would probably never even notice it.

Comment 13

[Stephen P. Morse]

Attachment: discard bad legacy cookies

Comment 14

[Stephen P. Morse]

Ignore prevous attachment.  I attached the wrong diff.  Trying again.

Comment 15

[Stephen P. Morse]

Attachment: discard bad legacy cookies (the correct diff file this time)

Comment 16

[Stephen P. Morse]

cc'ing alecf for super-review

Comment 17

[Scott Collins]

Comment on attachment 84787 [details] [diff] [review]
discard bad legacy cookies (the correct diff file this time)

sr=scc.  This fix looks good to me; just one question: is the cookie file
rewritten to disk each session?  That is, does skipping this cookie on a read
eventually end up eliminating the cookie from the file for next time?

Comment 18

[Kathleen :Brade]

Comment on attachment 84787 [details] [diff] [review]
discard bad legacy cookies (the correct diff file this time)

r=brade
Do we need to worry about sites that have non-compliant cookies?

Comment 19

[Stephen P. Morse]

Thanks Scott.

The cookie file is rewritten if any cookies in it change.  So, assuming that the 
user does some surfing, his cookie file will get rewritten and these legacy 
cookies will never again be seen.

However, no rewrite is done if the only change made to the set of collected 
cookies is the rejection of a legacy cookie.  I could add a few more lines of 
code to force a rewrite at that time, but I wanted to keep the patch as short as 
possible to avoid adding bloat just to fix legacy problems.  

Comment 20

[Stephen P. Morse]

brade: I'm not sure I understand what you mean by "sites that have non-compliant 
cookies".  Are you referring to sites that set cookies with a domain attribute 
not starting with a dot?  If so, we automatically append the dot before entering 
the cookie into our cookie list.  And it was because we started doing this that 
we got ourselves into this duplicate-cookie situation.

Comment 21

[Jaime Rodriguez, Jr.]

Let's get this one checked into the trunk asap, and tested, so we can take it on
the 1.0 branch next week. thanks!

Comment 22

[Scott Collins]

Re-writing the cookie file at the next new cookie sounds fine to me.  I don't
think there's any need to add code to your current patch.  Simpler is usually
better.

Comment 23

[Stephen P. Morse]

Patch checked in on trunk

Comment 24

[Jaime Rodriguez, Jr.]

tever- Can you verufy this on the trunk, and check to see it caused no
regressions? thanks!

Comment 25

[Tom Everingham]

verified on trunk - duplicate cookies with un-dotted domains are eliminated from
the cookie list, not seeing any regressions

tested 05/24 trunk builds - winNT4, linux rh6, mac osX

Comment 26

[Peter Trudelle]

adt1.0.0+ per ADT, okay to land on branch post once you have driver's approval.

Comment 27

[Matthias Versen [:Matti]]

*** Bug 147131 has been marked as a duplicate of this bug. ***

Comment 28

[Matthias Versen [:Matti]]

*** Bug 132169 has been marked as a duplicate of this bug. ***

Comment 29

[Eric Krock]

*** Bug 146007 has been marked as a duplicate of this bug. ***

Comment 30

[Matthias Versen [:Matti]]

*** Bug 147925 has been marked as a duplicate of this bug. ***

Comment 31

[scottputterman]

changing to adt1.0.1+ for checkin to the 1.0 branch for the Mozilla1.0.1
milestone.  Please get drivers approval before checking in.

Comment 32

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 84787 [details] [diff] [review]
discard bad legacy cookies (the correct diff file this time)

please check into the 1.0.1 branch ASAP. once landed remove the
mozilla1.0.1+ keyword and add the fixed1.0.1 keyword

Comment 33

[Susie Wyshak]

Test results with commercial 2002-05-30-04-trunk for windows 32 bit:
Not fixed: Using a profile I'd used in Gecko 11/28, I still fail to sign in and
am redirected to the sign in page again.

(I am reopening this since it looks like this fix was primarily for SNS fix
purposes.)

Both screenname.aol.com cookies with a . and without are being saved.

When I first to the signin page in 5/30 build, the user name is filled in,
password is not. 
1 of the 6 screenname.aol.com cookies has a leading  dot.

Enter password.
Signin page refreshes with empty login and password fields.

7 of the 11 screenname.aol.com cookies have a leading dot.

Restarted browser, still cannot log in.

I hope this info helps. 

Comment 34

[Susie Wyshak]

PS I used brand new profiles just for testing this.

Comment 35

[Stephen P. Morse]

Can you attach screenshots of your cookie-manager dialog so I can see 
what cookies you have.  Open the cookie manager and give me a screenshot of the 
cookies listed.  Also, select each cookie one at a time and give me a screenshot 
of the cookie details for each one.

Also if you can attach you cookies.txt file, both before and after the browser 
run, that might be helpful.

Comment 36

[Stephen P. Morse]

Are you sure you are not confusing host cookies with domain cookies.

Comment 37

[Susie Wyshak]

Sent stuff to Steve offline.

Comment 38

[Susie Wyshak]

Retested with commercial 20020604-04 trunk for windows 32 bit.

FIXED! Able to sign into webmail in 6/4 build after signing in with 11/28/01 
build.

Verified that only screename cookies with . are present in my cookies file 
(This wasn't the case in the 5/30 build)

Comment 39

[Jaime Rodriguez, Jr.]

tever - can you pls verify this one on the 1.0 branch asap? thanks!

Comment 40

[Tom Everingham]

actually, I couldn't duplicate the problem Susie was seeing with screename
cookies on 5/30 builds so it would be good to have her check the branch too.

It still looks like its working to me.  Using a perl script to set an undotted
domain cookie results in a dotted domain in the cookie list.  Duplicate cookies
with undotted domains appear to be replaced.  Checked todays branch on winNT and
mac osX.

Comment 41

[Susie Wyshak]

worksforme with Gecko/20020605 branch also (tested using a new profile)

Comment 42

[Tom Everingham]

thx, marking verified per comment #38

Comment 43

[Jaime Rodriguez, Jr.]

tever - pls verify this fix on the branch, then replace fixed1.0.1, with
verified1.0.1. thanks!

Comment 44

[Tom Everingham]

verified branch

Comment 45

[Kathleen :Brade]

*** Bug 148806 has been marked as a duplicate of this bug. ***

Comment 46

[Michael Lefevre]

*** Bug 144362 has been marked as a duplicate of this bug. ***