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

RESOLVED FIXED

Status

()

Firefox
Preferences
RESOLVED FIXED
14 years ago
12 years ago

People

(Reporter: Travis Cobbs, Assigned: Ganesh)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

14 years ago
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.
(Reporter)

Comment 1

14 years ago
(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.
(Assignee)

Comment 2

14 years ago
Created attachment 171602 [details] [diff] [review]
patch for review.

This is a bug on Linux aswell. Attached here the patch for the same.
(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.
OS: Windows 2000 → All
Hardware: PC → All
Comment on attachment 171602 [details] [diff] [review]
patch for review.

strip the period, if anything.
Attachment #171602 - Flags: review-
(Assignee)

Comment 5

14 years ago
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.
.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.
(Assignee)

Comment 7

14 years ago
Created attachment 171849 [details] [diff] [review]
recent patch

I made the changes in extensions/cookie/nsPermissionManager.cpp this works fine
when tested. Please let me know your comments.
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.
Attachment #171849 - Flags: review-
(Assignee)

Comment 9

14 years ago
Created attachment 171958 [details] [diff] [review]
changes in CookieExceptions.js
Comment on attachment 171958 [details] [diff] [review]
changes in CookieExceptions.js

Mike, is this the right way then?
Attachment #171958 - Flags: review?(mconnor)
Assignee: firefox → rganesan
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Attachment #171958 - Flags: review?(mconnor) → review-
Created attachment 174363 [details] [diff] [review]
formatted patch
Attachment #171602 - Attachment is obsolete: true
Attachment #171849 - Attachment is obsolete: true
Attachment #171958 - Attachment is obsolete: true
Attachment #174363 - Flags: review?(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!
Attachment #174363 - Flags: review?(mconnor) → review+
Mike, you gave your r+. Would you please drive it into trunk?

Updated

13 years ago
Attachment #174363 - Flags: approval-aviary1.1a?
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
Attachment #174363 - Flags: approval-aviary1.1a? → approval-aviary1.1a+
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?
This does indeed work now, thanks to the new pref dialog landing and
_makeStrippedHost().
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs,
filter on "beltznerLovesGoats" to get rid of this mass change
QA Contact: mconnor → preferences
You need to log in before you can comment on or make changes to this bug.