222343 - cookies migrated from IE may be effective duplicates of extant cookies

Status: RESOLVED FIXED

(Core Graveyard :: Profile: Migration, defect)

Description

[Dan M]

(Filed because of bug 185689 comment 14 -- anybody with better knowledge of the
situation do please feel free to comment / slap down / fix / close / take the
day off):

Admittedly I've never seen IE cookie file format documentation. This is not from
want of trying. And I don't have write access to a spread of domains from which
I can directly test my claim. But as far as I can tell IE draws no distinction
between cookies set by a request-host to a superdomain and cookies set by a
request-host in the superdomain. That is,

  b.com calls document.cookie="name=value"   and
  a.b.com calls document.cookie="name=value;domain=.b.com"

-- same thing. In either case, you get a cookie with the domain of b.com. The
distinction is important to Mozilla but maybe not to IE. (It's rumoured there's
a clause in RFC2109 which specifies a distinction, but I can't find it.)

But the point is, IE cookie files seem to make no such distinction. So there is
no way I know of to know during migration that a particular cookie is a
domain-type cookie. So the profile migration code treats all migrated cookies as
non-domain-type cookies.

That opens us up to a duplicate cookie problem similar to the one in bug 145492.
nsCookieService doesn't consider a .b.com cookie a duplicate of a b.com cookie
even if everything else important (name and path) match. Migrated cookies which
arguably should be duplicates won't be detected as such.

What's the right thing to do, then? The distinction Mozilla makes between
superdomain-type cookies and non-domain-type cookies is that it hides
non-domain-type cookies from subdomains. That is, a (non-domain-type) cookie
with a domain of b.com will be hidden from a host at a.b.com, while a
(superdomain-type) cookie with the domain of .b.com will be visible to host
a.b.com. If I'm correct that IE doesn't draw a distinction between the two, and
it seems not to judging from its cookie files, then IE won't hide b.com cookies
from a.b.com as Mozilla does. That'd be a compatibility problem. I wonder if
it's obscure enough that no one has noticed?

The correct thing to do depends on IE's treatment of cookies. If it makes no
distinction then likely it's treating all cookies the same way we treat
superdomain-type cookies, and we should migrate all cookies as superdomain-type
cookies. (And there's also a general compatibility problem that should be
addressed in another bug.)

If however IE does make a distinction then we should figure out how the
distinction is stored in the cookie file and carry it over while migrating. If
IE makes this distinction but there's no indication in the cookie file, which
would be odd, then we should migrate all cookies as non-domain-type cookies but
to avoid migrated duplicates, we should delete any extant superdomain-type
cookies which would otherwise match a newly migrated one.

Comment 1

[[e-mail]]

very nice report danm :)

at the least we should change the migrator to set domain cookies by default. i'm
not sure about IE's ability to set non-domain cookies (which i just call host
cookies). anyone willing to test this?

i'm tempted to just ignore the duplicate cookie issue (how much of a problem
would it be, anyway?), but if we really care, it's simple to fix. we can just
add a call to nsICookieManager::Remove() into the migrator code, to remove the
host cookie if it exists.

Comment 2

[Dan M]

bug 145492 comment 3 mentions a case where duplicate cookies cause a problem. As
you say it's easy to fix.

If IE's cookies are all host cookies or all domain cookies, we have an
interoperability problem that's bigger than this little migration bug. It'd be
good to know. Right now we assume all migrated cookies are host cookies.
Anything else will want an interface change to nsICookieManager2. But we should
get it right, whichever it is.

Comment 3

[[e-mail]]

hmm, what interoperability problem do you speak of? you mean the one that we
support host cookies, and IE does not? that's a feature :)

>Anything else will want an interface change to nsICookieManager2.

actually, nsICookieManager2::Add() doesn't assume a host cookie... it assumes a
domain cookie at present :/

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

what's the format of the 'host' param at
http://lxr.mozilla.org/seamonkey/source/extensions/tridentprofile/src/nsTridentPreferencesWin.cpp#599
 ? if it has a leading dot, everything's hunky-dory. if it doesn't, we're
broken, badly. what the Add() code should be doing, is checking to see if
aDomain has a leading dot, and making it a domain cookie if it does. (that's
official RFC2109 behavior).

Comment 4

[Dan M]

We DO assume an added cookie is a domain-cookie. Huh.

The format of the |host| parameter at nsTridentPreferencesWin.cpp#599 is
whatever comes from the IE cookie file. No leading dot.

You seem to be saying that you're already aware that all IE cookies are the
domain-type. I'm uncertain though because in comment 1 you seemed uncertain. If
this is a known thing, then we should add the leading dot to migrated cookies,
let potential duplicates take care of themselves, and dust our hands of the
migration issue.

However: if this domain thing is so important, it seems that parameter would be
a useful addition to nsICookieManager2::Add, though there's no immediate use if
IE doesn't have host cookies.

However continues: really? It doesn't seem like an interoperability problem to
you? Here's how I imagine things playing out: it seems like most websites are
written with only IE in mind, and I get the impression this leading dot thing
isn't well understood. There are probably only twelve websites in existence
which use domain cookies, but I imagine all but two of them have cookies which
are visible throughout their domain on IE but not on Mozilla. I'm surprised that
doesn't cause problems with Mozilla on those sites.

Comment 5

[[e-mail]]

>The format of the |host| parameter at nsTridentPreferencesWin.cpp#599 is
>whatever comes from the IE cookie file. No leading dot.

ok, bad. this will mess up the cookie list somewhat. we need to fix that.

>You seem to be saying that you're already aware that all IE cookies are the
>domain-type.

i'm not sure, but i've seen evidence elsewhere that would support this. we still
need to test, or something ;)

>let potential duplicates take care of themselves, and dust our hands of the
>migration issue.

we'd still need to Remove() any existing host cookie, i'd say...

>it seems that parameter would be a useful addition to nsICookieManager2::Add

yeah, either it can be a PRBool inparam or we can just make it be specified by
the presence of a leading dot on the aDomain param. i favor the latter since
it's less code.

>Here's how I imagine things playing out: it seems like most websites are
>written with only IE in mind, and I get the impression this leading dot thing
>isn't well understood.

actually the leading dot thing isn't important in terms of Set-Cookie headers...
whether you include it or not doesn't matter. (although i should mention that
before i rewrote cookies, it did - so all sites designed with older mozillae in
mind _will_ be aware of it. and we seem to work okay, so...). anyway, we have
seen bugs filed because of this; but i'd be loath to rip out support for host
cookies just because IE didn't see fit to support it. it's an evang issue in
those cases.

tons of sites use the "domain=" attribute, and they all have a leading dot like
they should.

Comment 6

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

A quick testcase showed me that IE stores the same cookie for cookies with or
without domain= attribute. So you can't see the difference from looking at the
.txt files.

testcase (in php):
<?
header("Set-Cookie: withdom=1;domain=yellow.exedo.nl;expires=".(time()+3600));
header("Set-Cookie: withoutdom=1;expires=".(time()+3600));
header("Set-Cookie: alldom=1;domain=exedo.nl;expires=".(time()+3600));
?>

or try at http://yellow.exedo.nl/~michiel/cookietestcase/domcookie.php

The first two cookies give the same (modulo the cookie name ofcourse) in the
.txt file.

Comment 7

[Dan M]

I believe I have this figured out after a visit to my friend Apache and his
sister the hosts file. The answer isn't simple. IE 6 does make a distinction
between host and domain cookies, and this distinction is stored in files between
sessions, but it's encoded in the index file, not the cookies text files
themselves. As far as I know the index file format is binary and undocumented,
so there's no way for us to know which kind of cookie it is while migrating. May
as well pick one.

The difference in cookie treatment between browsers goes like this. From a
subdomain, they're identical. In both browsers, a cookie without an explicit
domain specified is not visible in a superdomain; a cookie set to the exact
domain is not visible in a superdomain; a cookie can be set to a superdomain and
it is visible in that superdomain.

From a superdomain, IE and Mozilla differ. Neither browser will accept a cookie
set to a subdomain. However in IE both a cookie without an explicit domain and a
cookie set to the exact domain are visible from a subdomain, while in Mozilla
only the domain cookie is visible in the subdomain: the host cookie from the
superdomain is not visible. Note that in IE while the superdomain host cookie is
visible in the subdomain, it cannot be overwritten from the subdomain.

IE also considers as identical, host cookies and cookies with an explicit domain
the same as the host. That is, a host cookie is replaced by a domain cookie
whose domain is the host domain and vice versa. Mozilla treats them as different
cookies. This aspect of cookie management is covered in RFC2109 section 4.3.3
and yeah I agree it's unclear but I favour IE's interpretation. This is more or
less bug 217179.

By the way both browsers allow a cookie to be set to a domain without a leading
dot. They treat it as if it had the leading dot. That's a violation of RFC2109
section 4.3.2. But we already knew that.

So there's your differences. I think there are interoperability issues here and
suggest we switch to follow the "unofficial" IE standard, despite the protest I
hear from the RFC corner. I'll file a separate bug which I expect will be shot
up but at least it'll be on record.

But to the point of this bug: We should treat all migrated cookies as domain
cookies, because we can't tell the difference and IE treats both host and domain
cookies more the way Mozilla treats domain cookies. My reading of this is that
it means prepending a dot to migrated host names and explicitly deleting any
extant Mozilla host cookie of the same name because of bug 217179.

I'll take this bug and post a patch soonish.

Comment 8

[Dan M]

Attachment: fix domain cookies, hostless cookies, valueless cookies

Spruce up cookie migration. This patch adds a leading dot to all domains
migrated from IE as discussed above, and removes any extant host cookie as
discussed above. Also it fixes an an undiscussed problem where a cookie with no
host but a path of "/" (which probably never happens) gets "/" as the host. It
also omits cookies with a zero-length value (that's bug 222343) and fixes a
nasty frameshift error suffered by the line-end scanning code under zero-length
fields, without losing its ability to handle all the standard line-end formats.
Seems like a good thing.

Comment 9

[[e-mail]]

>It also omits cookies with a zero-length value (that's bug 222343)

which bug? :)

Comment 10

[Dan M]

Attachment: fix domain cookies, hostless cookies, valueless cookies

oops

Comment 11

[Dan M]

I rely on Cheerios to stave off senility. Comments 8 and 9, I meant bug 223029.

Comment 12

[Darin Fisher]

Comment on attachment 133767 [details] [diff] [review]
fix domain cookies, hostless cookies, valueless cookies

looks good to me.  sr=darin

Comment 13

[[e-mail]]

Comment on attachment 133767 [details] [diff] [review]
fix domain cookies, hostless cookies, valueless cookies

>+    int hostLength = path - host;
>+    if (hostLength > hostCopyEnd - hostCopyConstructor)
>+      hostLength = hostCopyEnd - hostCopyConstructor;
>+    PL_strncpy(hostCopyConstructor, host, hostLength);
>+    hostCopyConstructor += hostLength;
>+
>+    *hostCopyConstructor = '\0';

PL_strncpy will copy until it finds a null terminator or until it reaches the
nth character. it also returns a pointer one past the end of the copied data,
suitable for nullterminating. so, just do this instead:

    *PL_strncpy(hostCopyConstructor, host, hostCopyEnd - hostCopyConstructor) =
'\0';

which eliminates hostLength. for extra points, eliminate hostCopyEnd and just
pass |sHostnameLengthLimit - 1| as the third arg (i'm sure we don't care about
wasting space for 1 extra char.) 

with that, r=dwitte. nice patch!

Comment 14

[Dan M]

I've never liked to put my faith in the return values of strcpy functions. Just
don't like 'em. Dunno why. Maybe some bad library bit me once. There's another
thing, though. Actually PL_strncpy returns a pointer to the beginning of the
destination buffer, in this case it's just exactly hostCopyConstructor. So that
really isn't a help.

I could use PL_strncpyz but that method is wrong for me at the limit. It assumes
the third argument is the destination buffer size and what I need is the source
buffer size so that doesn't work for me unless I tweak the length parameter and
insert temporary nulls into the source string and did I mention it won't null
terminate if the third parameter is 0 and I really do want the string to be null
terminated when I'm all done with it and this is all kind of implied by the
interface comments but it's not explicit so the trust just isn't there and it's
more work anyway and I'm rather proud of this run-on so I don't much like that
function, either.

Also, for the length argument I need hostLength which maxes out at
sHostnameLengthLimit, plus or minus a byte, not the limit itself, since I
haven't bothered internally null-terminating the source string. This could be
reworked a bit but it'd end up pretty much the same.

So I can't use your suggestion without modification and the code as it is has a
tight control and especially a kind of symmetry that make me happy so I think it
should stay the way it is.

No one zinged me for my hostCopyConstructor pun! Dudes, it's a pun! I deserve to
be slapped for that one on general principles.

Comment 15

[[e-mail]]

>Actually PL_strncpy returns a pointer to the beginning of the destination buffer

indeed. pass the Cheerios, please.

Comment 16

[[e-mail]]

Attachment: improve nsICookieManager2::Add (checked in)

just a small patch to make ::Add() inparams be consistent with the "new" cookie
interface nsICookie2, and also to make it respect the "domainness" of the
cookie. orthogonal to danm's patch.

Comment 17

[Darin Fisher]

Comment on attachment 133875 [details] [diff] [review]
improve nsICookieManager2::Add (checked in)

sr=darin

Comment 18

[Dan M]

Comment on attachment 133875 [details] [diff] [review]
improve nsICookieManager2::Add (checked in)

That's the weirdest thing I've seen all day, attaching that patch to this bug,
but OK. r=me.

Comment 19

[[e-mail]]

well, the fix for Add() incorrectly assuming the input has a leading dot is
related to the "assume domain cookie" portion of this bug (your patch just
happens to mask that problem)... and the rest of the patch was just cleanup.
besides, you haven't lent me the cheerios yet :)

Comment 20

[[e-mail]]

Comment on attachment 133875 [details] [diff] [review]
improve nsICookieManager2::Add (checked in)

checked this one in on trunk.

Comment 21

[Dan M]

everything's checked in on the trunk now