334363 - can't change existing (cookie, popup blocking, xpi install, images) permissions for site, need to remove and then re-add

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[bugs1m.wisefool]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2

If you already have a site listed under "Exceptions" for cookie permissions, an attempt to change that permission won't work unless you Remove the site from the permissions list first, and then re-add it with the new permission.
If you re-enter the site's domain while it's still on the list, and click a new permission level, the UI will tell you that the permission level has changed, but if you close the window and come back, you can see that it's the same as before.

Reproducible: Always

Steps to Reproduce:
1. Open Tools -> Options, then Privacy -> Cookies: click on "Exceptions"
2. Add "1foobar.net" as "Block"
3. Type in "1foobar.net" again, but click "Allow for Session" this time
4. Click Close.
5. Re-open Exceptions.

Actual Results:  
"1foobar.net" is set to "Block".

Expected Results:  
"1foobar.net" should be set to "Allow for Session".

Comment 1

[Steve England [:stevee]]

Reporter, do you still see this problem with the latest Firefox 2? If not, can you please close this bug as WORKSFORME. Thanks!

Comment 2

[Steve England [:stevee]]

No response from reporter re: comment 1 -->INCOMPLETE.
Reporter, if you still see this problem with the latest release of Firefox 2, please reopen this bug. Thanks!

Comment 3

[u279076]

I cannot reproduce this with the information supplied.  Verified INCOMPLETE.

Comment 4

[bugs1m.wisefool]

Hi, I just found that this got responses today. I can still reproduce this bug with Firefox 3 alpha 7, actually. It still happens, even in Safe Mode. I only have Adblock Plus, ChatZilla, and NoScript running on it lately anyway.

Comment 7

[[e-mail]]

confirmed on current trunk; probably exists on branches too. the problem is here:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/browser/components/preferences/permissions.js&rev=1.10#136

if the permission already exists, nsIPermissionManager::Add() is never called, so the backend never gets the update.

Comment 10

[sdb]

Attachment: proposed changes for the permissions.js file

Comment 11

[[e-mail]]

Comment on attachment 289158 [details] [diff] [review]
proposed changes for the permissions.js file

>+    // check whether the permission already exists
>+    var exists = this.testPermission(host,aCapability);

hmm, could you call this something like permissionExists() instead? testPermission is a method on nsIPermissionManager, which is confusing.

>-      host = (host.charAt(0) == ".") ? host.substring(1,host.length) : host;

it's not your fault, but... the rawHost/host business in this file is very confusing, and wrong. i think the person who wrote this thought that nsIPermissionManager makes some kind of distinction between hosts with a leading dot, which isn't true. what we really want to do here is have a sanitization method that pulls out spaces, leading/trailing dots, "*." wildcards etc (bug 259445) before even constructing an nsIURI object to use.

extra points for writing a patch to do that, though it doesn't have to be part of this bug ;)

>+      // implementations will have to move into here. NOTE: Extension developers
>+      // may implement UI's to remove permissions so this observer really needs
>+      // to listen for 'deleted' notifications and remove corresponding tree items

nice catch. any chance you could file a followup bug for this?

Comment 12

[sdb]

Attachment: proposed changes for the permissions.js file

Each time I examine the permissions.js file I seem to be finding more and more problems with the script.  The patch I've added will resolve a few of the bugs, but in my opinion this entire window (permissions.xul/permissions.js/permissionsutils.js) needs to be recoded.  There are also a few problems with the nsIPermissionManager interface that need to be fixed as well.  I've described a few of the issues below.  I haven't filed a new bug report yet regarding the deleted notifications.  I thought it might be a good idea to just file a bug report about the entire window explaining why it should be redesigned and recoded.  Let me know what you think.

nsIPermissionManager
It does not currently do a case-sensitive comparison when testing a permission.  If a host happens to make it's way into the hostperm.1 file with a capital letter (Google.com) then testing for google.com will return 0 for unknown action.  While I agree that this would be a rare occurrence it is possible for the user to edit the hostperm.1 file directly and there are also a few programs available that add exceptions to Firefox by manipulating the hostperm.1 file directly.  SpywareBlaster in particular causes problems because it adds a few hosts that include capital letters.  To avoid that problem I think the nsIPermissionManager interface should store the user data in a sqlite database instead of just a text file making it more difficult to edit it directly.  And it should also perform a case-sensitive comparison in the testPermission() method.  Another problem is that users can add hosts that include non-alphanumeric characters before and after the host name (e.g. .google.com, *.google.com), but when the user opens the view exceptions window all of those characters are removed so it appears to the user as if there are duplicates of the same host listed.  I think it would be a good idea to automatically strip all non-alphanumeric characters from each host name in the add() method before they are added to the database.

nsIURI
This really isn't a major issue, but when you create a nsIURI for a file:/// url the host property is always null.  Since the add() method of nsIPermissionManager will add scheme:file to the database for file urls I think it would be helpful to automatically set the nsIURI.host to scheme:file for local file urls.

permissions.xul window
The biggest issue with this window is that the 'address of website' textbox allows the user to enter either a host or a url.  By allowing both urls and hosts to be entered it makes it nearly impossible to determine which characters to allow and which to discard.  Some characters such as underscores are valid in the path while not valid in the domain name.  So to ensure that the host name is valid in DNS the textbox should only accept host names and not urls.  If that feature is changed then you could simply add a onkeyup event to the textbox and automatically remove any invalid characters immediately after the user enters them.  It might also be a good idea to include a drop down menulist element right before the textbox that includes different schemes (http://,file:///) which would indicate to the user that only the host name should be entered.  In the current version it is not possible to add exceptions for local files.  The latest patch includes support for that feature but if the user could simply select file:/// from a drop down menu it would simplify the process.  Another problem is that you can't change permissions for hosts listed in the tree by clicking on them.  It would make more sense to enable the allow/block buttons anytime a user clicked on an item in the tree.  That way instead of having to manually type the host the user could simply click on a host name and then click allow/block to change the permission.  And finally there's the deleted notifications which I've noted in the patch.  With all of these issues I think it would be better to simply start over instead of trying to patch the current script.

Comment 13

[sdb]

Just to clarify one of my comments above, the nsIPermissionManager does perform a case-sensitive comparison in the testPermission() method.  However it needs to perform a lowercase case-sensitive comparison to avoid instances where a host appears in the hostperm.1 file with capital letters.

Example:
if (host.toLowerCase()==host.toLowerCase()) return capability;

Comment 14

[sdb]

Attachment: updated patch for permissions.js file

I've added some additional regex code to eliminate any possible errors that might occur when a user enters a full url instead of the host.  While there may be a more efficient way to code the regex commands it may end up being helpful to future developers to have each regex command on a separate line along with an explanation of why it's necessary.

Comment 15

[[e-mail]]

(In reply to comment #12)

sorry for the delay - i haven't looked at your patch in detail yet, but by way of reply:

> nsIPermissionManager
> It does not currently do a case-sensitive comparison when testing a permission.

you mean case-insensitive, right? and, it actually does - nsIURI normalizes the host when it's created, so e.g. "Google.com" will end up as "google.com". (try typing Google.com into the urlbar.) UTF8 hosts are similarly normalized to a consistent representation. the only remaining problem, then, is people editing the hostperm.1 file directly - but we just moved to permissions.sqlite on the trunk. ;)

> the same host listed.  I think it would be a good idea to automatically strip
> all non-alphanumeric characters from each host name in the add() method before
> they are added to the database.

yep, this needs to happen somewhere. we could do it inside add(), i suppose, but i was thinking we'd do it in the js code which is what deals with user input. i mean, we don't want to penalize nsIPermissionManager consumers who know they have valid urls (e.g. "block this current page" kinda features) by messing with them, right?

adding a sanitizeHost() function to the js and encapsulating everything in there would be a good way to do this.

> nsIURI
> This really isn't a major issue, but when you create a nsIURI for a file:///
> url the host property is always null.  Since the add() method of
> nsIPermissionManager will add scheme:file to the database for file urls I think
> it would be helpful to automatically set the nsIURI.host to scheme:file for
> local file urls.

it can be null for other URI schemes too, i forget which. however, we just removed the "scheme:" feature on trunk, and add() will now throw failure on a null host, so it's no longer really an issue. (does the script check for failure in add(), and handle that suitably? i.e. by not adding the permission to the tree view? ;)

> permissions.xul window
> The biggest issue with this window is that the 'address of website' textbox
> allows the user to enter either a host or a url.  By allowing both urls and

hmm. you raise some good points about entering paths, schemes etc. we can try to make it more obvious in the UI that only a host is expected, but in the end we still need to deal with wacky input. there's a neat answer, though - nsIURI does all this parsing for us (scheme, path etc). how about we have sanitizeHost() take in the string and return a URI, and inside the guts it does this:

1) sniff for a scheme, perhaps by scanning the string until it hits the first non-alphabetical character [a-z, A-Z], then seeing if the next part is "://". remove it.
2) sniff for leading dots, "*.", etc and remove them.
3) prepend "http://".
4) create the nsIURI, which will do the rest of the parsing for us. (we need a scheme to do this, otherwise it'll throw.)

wrt your other points, modifying permissions in the tree and deletion notifications, those sound like good things to fix. i'd also really, really, really like to see that rawHost stuff just die (it's in other places in the tree too; i'll post bonsai links once i've found them all).

if you think that needs a rewrite, we're certainly not averse to those, but smaller patches are better ;)

Comment 16

[[e-mail]]

(In reply to comment #15)
> 1) sniff for a scheme, perhaps by scanning the string until it hits the first
> non-alphabetical character [a-z, A-Z], then seeing if the next part is "://".
> remove it.
> 2) sniff for leading dots, "*.", etc and remove them.
> 3) prepend "http://".
> 4) create the nsIURI, which will do the rest of the parsing for us. (we need a
> scheme to do this, otherwise it'll throw.)

just an idea, a more robust way to do all this might be to create two nsIURI's, one to parse the scheme out, then we grab the host, sanitize that ourselves, and use it to create a second (sanitized) nsIURI. kinda roundabout, but it eliminates URI parsing on our end, which is always a good thing...

Comment 17

[[e-mail]]

Comment on attachment 290510 [details] [diff] [review]
updated patch for permissions.js file

>+    // remove all non-alphanumeric characters from the entire string with
>+    // the exception of : / (schemes, paths) and - . (valid in DNS)
>+    var host = textbox.value.replace(/[^a-zA-Z0-9\:\/\-\.]/g,"");

what about UTF8 hosts? this will break them.

i think the best thing to do is avoid as much URI parsing as possible, i.e. something like what i suggested in comment 16, and just have a simple regex or two to strip leading and trailing dots and wildcards.

Comment 18

[sdb]

(In reply to comment #15)
> it can be null for other URI schemes too, i forget which. however, we just
> removed the "scheme:" feature on trunk, and add() will now throw failure on a
> null host, so it's no longer really an issue.

Does this mean that it's no longer possible to add allow/block cookie permissions for local file:/// urls?  Many users have cookies disabled by default and then enable cookies on a per site basis by adding allow/block permissions.  The ability to add an Allow cookie permission for scheme:file was incredibly helpful as it allowed web developers to test scripts locally (document.cookie = 'name=value') with cookies turned off.  The last patch that I posted includes support for adding scheme:file permissions.  If it's no longer possible to add permissions for scheme:file then there are a few lines of code that will need to be removed.

I hope that an alternative method of adding permissions for file:/// urls is implemented if the scheme:file feature has been removed.  That was a great feature to have available especially for web developers.

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Smaller fix

Folks, this is a simple patch that fixes the issue I reported in bug 407258. From reading this bug I guess there are more problems in this file, but this patch at least fixes the issue as currently summarized.

Comment 21

[[e-mail]]

Comment on attachment 292102 [details] [diff] [review]
Smaller fix

r=dwitte, very elegant ;)

Comment 22

[[e-mail]]

(In reply to comment #18)
> Does this mean that it's no longer possible to add allow/block cookie
> permissions for local file:/// urls?

yes, that's true - it was done because the scheme blocking functionality is counterintuitive when the user is expecting to have blocking by a particular host or spec - "hey, i just clicked 'deny cookies' on my file:// url, why are none of my files allowed to set cookies now?". it was basically a hack. i agree it'd be great to have another way to do it, but that's food for another bug.

anyway, i love the work you're doing here, but let's get bent's minimal fix in and move this cleanup stuff to another bug. could you file one, and cc me please?

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 292102 [details] [diff] [review]
Smaller fix

Rather small change that allows permission type to be modified.

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 292102 [details] [diff] [review]
Smaller fix

a=beltzner for 1.9

Comment 25

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fixed on trunk.