480892 - ".foo.com" cookies have inconsistent UI and behaviour (don't write exceptions for .foo.tld)

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Chris Lawson (gone)]

Again, I'll use the Freep example.

STR:

1) Visit http://www.freep.com/
2) Preferences -> Privacy -> Show Cookies...
3) (optional) Filter on "freep" for clarity.
4) Control-click on the ".freep.com" entry.

ER: Menu options use ".freep.com" text.

AR: Leading dot is ignored in contextual menu so items use "freep.com". The leading dot is, however, still written to permissions back end (which behaviour is also incorrect according to dwitte).

The contextual menu entry reads "Remove and Block Cookies from Sites Matching freep.com", but it doesn't remove all the cookies that phrasing would imply. At minimum, I would expect it to remove all the ".freep.com" cookies, and you could certainly argue that it ought to remove *all* cookies for the freep.com domain, including the sitelife.freep.com cookie, the www.freep.com cookie, etc.

Our behaviour should be the following:

1) The CM text is fine. Ignoring the leading dot is the right thing to do here.
2) Do not write leading dots to the permissions back-end. It will soon sanitize this input anyway, so writing them will be pointless. (See bug 480891.)
3) Remove all (*.)foo.com cookies when "remove and block all" is chosen from the CM on a .foo.com cookie.

(Funny -- the "Allow all foo.com cookies" item treats leading dots properly.)

Comment 1

[Chris Lawson (gone)]

(3) is gonna be a little tricky because nsCookieService doesn't have any way to remove all cookies matching a given host. nsCookieService can only delete cookies one at a time or all at once.

Comment 3

[Chris Lawson (gone)]

Attachment: fix v1.0

This refactors some of the other code to make everything in here a bit simpler. Smokey tested it and found that it doesn't seem to break anything, and it fixes the bug as described.

I really need to take a look at bug 480891 at some point, though, because then we could mostly dispense with this permissionsBlockingNameForCookieHostname silliness.

Comment 4

[Chris Lawson (gone)]

Oh, but this doesn't fix (3) in comment 0, 'cause like I said, that's a lot trickier. I wouldn't be opposed to fixing that in its own separate bug.

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #4)
> Oh, but this doesn't fix (3) in comment 0, 'cause like I said, that's a lot
> trickier. I wouldn't be opposed to fixing that in its own separate bug.

It should be its own bug; I filed 521170.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 405202 [details] [diff] [review]
fix v1.0

hendy said he'd try to look at this and then send sr? to smorgan.

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

There's been some comment cleanup in this file, but the code looks identical, so if this patch passes muster, I'd like to see about getting this fix in 1.6.11 for 10.3.9 users, since our behavior is very broken where users most need it to work (the "remove and block" case).

Comment 8

[Christopher Henderson]

Comment on attachment 405202 [details] [diff] [review]
fix v1.0

//-                   forHost:host
//+                    forHost:[self permissionsBlockingNameForCookieHostname:curSite]
//                       type:CHPermissionTypeCookie];

Fix alignment here.

We don't need the check for [inHostname length] > 1. In the case of ".", we send the permissions manager "http://", which has no effect on the permissions.

r+ with those changes.

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 405202 [details] [diff] [review]
fix v1.0

I can make hendy's review-comment changes on checkin if Stuart has nothing else and Chris is still flying.

Comment 10

[Stuart Morgan]

Comment on attachment 405202 [details] [diff] [review]
fix v1.0

sr=smorgan with the changes above and one more: I don't like relying on things working out if we feed an empty host in, so we should get [self permissionsBlockingNameForCookieHostname:curSite]
, check it for non-zero length, and only then make the permissions-setting call.

Comment 11

[Chris Lawson (gone)]

Attachment: fix v1.01

Updated per r/sr comments, ready for chicken.

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed on cvs trunk and CAMINO_2_0_BRANCH.

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: 1_8 branch patch to accomodate comment cleanup :P

This is the 1_8 version; we had comment cleanup in the middle of the patched area, which kept the original patch from applying :P

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed the 1_8 patch on the MOZILLA_1_8_BRANCH.