255441 - Blocking a site that begins with dot does not work in cookie exceptions

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Travis Cobbs]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040803 Firefox/0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040803 Firefox/0.8

I periodically go through my stored cookies looking for sites I want to block. 
When I find one, I copy the Host: field into another window for temporary
storage, and do this for each one.

Next, I go into the Cookie Exceptions panel, and copy and paste each host into
the "Address of website:" field, and then click block.  The problem is that if
the text begins with a . (for instance .reference.com), it goes into the list as
doubleclick.net and says blocked, but DOES NOT BLOCK.

Reproducible: Always
Steps to Reproduce:
1. Go to Options->Privacy->Cookies->Exceptions...
2. Type .reference.com into the "Address of website:" field (any site name
beginning with '.' will do.
3. Click Block.  Notice that reference.com shows up on the list (not .reference.com)
4. Make sure to remove any reference.com cookies from the stored cookies list.
5. Go to www.dictionary.com.  Notice that a new cookie from .reference.com is
accepted and shows up in the stored cookies list.
Actual Results:  
My Stored Cookies showed a .reference.com cookie.  If I removed that cookie and
go back to www.dictionary.com, it reappears.

Expected Results:  
It should either block the cookie, or give an error when an attempt is made to
add .reference.com to the exceptions list (stating that site names cannot begin
with '.').  For obvious reasons, blocking the cookie is preferable.

Comment 1

[Travis Cobbs]

(In reply to comment #0)
In my steps, I forgot to mention that you apparently have to look something up
at dictionary.com for the cookie to be set.

Comment 2

[Ganesh]

Attachment: patch for review.

This is a bug on Linux aswell. Attached here the patch for the same.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #2)
> Created an attachment (id=171602) [edit]
> patch for review.

You should fix the indentation and follow the same format ("if(" or "if (").
Also, use diff -up8 to make the patch.

I doubt that popping up a dialog is what is wanted here though. Stripping off
the leading period would probably be better.

Comment 4

[Mike Connor [:mconnor]]

Comment on attachment 171602 [details] [diff] [review]
patch for review.

strip the period, if anything.

Comment 5

[Ganesh]

stripping off of . is already there.say if I add .moneycontrol.com
(moneycontrol.com)and go to www.moneycontrol.com the cookies are accepted (both
firefox and latest mozilla). But if I add www.moneycontrol.com for bocking
cookies and go the site then the cookies are really blocked from this.

I believe if we don't need a popup message we probably need to add  the entry as
www.moneycontrol.com.

Comment 6

[Mike Connor [:mconnor]]

.moneycontrol.com probably gets saved to the permissions file as
.moneycontrol.com, and this will break domainwalking.  If you block
moneycontrol.com it should block all domains ending in moneycontrol.com
(www.moneycontrol.com, foo.moneycontrol.com, bar.baz.moneycontrol.com, etc)

We strip the period, incorrectly, in the display list, but if hostperm.1 shows
it as .moneycontrol.com we need to strip.

Comment 7

[Ganesh]

Attachment: recent patch

I made the changes in extensions/cookie/nsPermissionManager.cpp this works fine
when tested. Please let me know your comments.

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 171849 [details] [diff] [review]
recent patch

this is the wrong place to strip the period.  if there's a "right place" to
strip the period in the permission manager backend, it'd be in Add(), since
once we store the right value, we don't need to reparse it.

But I don't think that's the right fix.  I'd rather just filter this in the UI
frontend, we only need to change this in one place for Firefox.

The backend doesn't need to know about bad user input, the front end should
handle parsing/fixup before anything gets passed to the permmgr.

Comment 9

[Ganesh]

Attachment: changes in CookieExceptions.js

Comment 10

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 171958 [details] [diff] [review]
changes in CookieExceptions.js

Mike, is this the right way then?

Comment 11

[Mike Connor [:mconnor]]

Comment on attachment 171958 [details] [diff] [review]
changes in CookieExceptions.js

right code, but I'd like to see a patch done right, style-wise.

diff -up8 please.  Context is good.

>     for (var i = 0; i < this._addedPermissions.length; ++i) {
>       var p = this._addedPermissions[i];
>+      if(p.host.charAt(0) == ".")
Space after the if statement, please.
>+	p.host= p.host.substring(1,p.host.length); 
Spaces, not tabs, 2-space indenting.
Space after p.host
Space after the comma.

Prevailing style is important, its not so fun to do patch cleanup before
checkin.

Comment 12

[Wolfgang Rosenauer [:wolfiR]]

Attachment: formatted patch

Comment 13

[Mike Connor [:mconnor]]

Comment on attachment 174363 [details] [diff] [review]
formatted patch

you still missed the diff -up8, but that doesn't matter for this patch.

Thanks!

Comment 14

[Wolfgang Rosenauer [:wolfiR]]

Mike, you gave your r+. Would you please drive it into trunk?

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 174363 [details] [diff] [review]
formatted patch

a=me, and FYI you can leave off the second argument to substring (and to slice)
to get the same effect.

/be

Comment 16

[Mike Connor [:mconnor]]

Comment on attachment 174363 [details] [diff] [review]
formatted patch

after all that, it looks like ben fixed this in the new extensions window! can
someone else verify and resolve the bug if this is the case?

Comment 17

[:Gavin Sharp [email: [e-mail]]]

This does indeed work now, thanks to the new pref dialog landing and
_makeStrippedHost().

Comment 18

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs,
filter on "beltznerLovesGoats" to get rid of this mass change