403500 - any changes made to existing permissions in the permissions.xul window are not retained after the user closes that window

Status: RESOLVED DUPLICATE of bug 334363

(Firefox :: Settings UI, defect)

Description

[sdb]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.9) Gecko/20071025 Firefox/2.0.0.9
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.9) Gecko/20071025 Firefox/2.0.0.9

Anytime you open the chrome://browser/content/preferences/permissions.xul window and try to change an exception that is already listed in the tree it does not permanently update that permission nor does it trigger the perm-changed notification.  For example, if you have google.com listed in the exceptions as Allow and you manually type google.com in the 'Address of website' box and then click 'Allow for Session' only the tree cell text is updated.  It never actually changes the permission with the add(uri,'cookie',8) method and therefore never triggers the perm-changed observers.  If you then close the window and re-open it the google.com permission will be back to Allow instead of retaining the 'Allow for session' setting.

This bug appears when changing permissions for both cookies and images.

Reproducible: Always

Steps to Reproduce:
1. Open the Firefox options window.
2. Click the privacy tab.
3. Click exceptions.
4. Enter google.com in the 'Address of website' box and click Allow.
5. Close the window.
6. Click exceptions again.
7. Enter google.com in the 'Address of website' box and click 'Allow for session'.
8. You will notice that the text in the tree cell will change successfully to 'Allow for session'.
9. Close the window.
10. Click exceptions again.
11. You will notice that the text in the tree cell has now reverted back to Allow indicating that the permission was never actually changed.
Actual Results:  
Only the tree cell text in the permissions.xul window is updated instead of actually updating the permission.

Expected Results:  
The text in the tree cell should be changed as well as calling the add(uri,'cookie',8) method which would permanently change the permission and trigger the perm-changed observers.

This could potentially pose a significant security risk if a user believes that they have successfully changed a permission from Allow to Block when in fact the permission remains set as Allow.  Then if that user decides to visit the site that they believe was changed to 'Block' there is the possibility that either an unwanted image or unwanted cookie could be added to their pc.  At the very minimum if they believe that the cookies from a certain site are blocked and then they visit that site then a tracking cookie could be added to their pc.

Comment 1

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

There's nothing in this bug report that needs to be kept confidential. I agree that it could be confusing.

Comment 2

[sdb]

Attachment: The attachment includes a modified addPermission method for the permissions.js file

Sorry if the patch isn't in the correct format.  This is the first patch I've submitted.  I've attached a modified version of the addPermission method that is found in the permissions.js file.  The main change involved adding the following code to the if statement.

aCapability == this._permissions[i].perm

I've also removed two lines of unnecessary code and noted the reasons for removing them.

Comment 3

[sdb]

Attachment: This includes the same change as the previous attachment except I've uncommented two lines

I've uncommented two lines and clearly marked where the changes were made.

Comment 4

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

Thanks for looking into this Ron. It would be great if you could attach a "diff" rather than the modified file - there's some documentation describing how to generate a patch file at http://developer.mozilla.org/en/docs/Creating_a_patch if you're interested.

Comment 5

[sdb]

Attachment: proposed changes for the addPermission() method in the permissions.js file

I've attached the diff file describing the changes that need to be made to the permissions.js file.  There's really only one major change that needs to be made.  The rest of the changes involve adding a couple of additional comments since there are no comments currently included in the code explaining why two uri's are being created for the same host.

Comment 6

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

Comment on attachment 288551 [details] [diff] [review]
proposed changes for the addPermission() method in the permissions.js file

The next step would be to ask for review - http://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree (linked to from the previous doc) describes the process in general - since this is a Firefox patch you'll need to ask for review from one of the Firefox peers, according to the guidelines at http://www.mozilla.org/projects/firefox/review.html . Since I'm a browser peer and happened to notice this bug, I'll take a look! :)

Comment 7

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

Comment on attachment 288551 [details] [diff] [review]
proposed changes for the addPermission() method in the permissions.js file

One style nit on the comments: please put a space after the comment marker, and use "// " for short multiline comments.

This code is dreadfully hard to follow - the "Permission" object it uses to track items is confusing, with it's "capability" property which appears to just be a display-friendly string that describes it's "perm" property, which itself is a value that maps to an nsIPermission's "capability" attribute.

>Index: mozilla/browser/components/preferences/permissions.js

>     // check whether the permission already exists, if not, add it
>     var exists = false;
>     for (var i = 0; i < this._permissions.length; ++i) {
>-      if (this._permissions[i].rawHost == host) {
>+      //do not change exists to true unless both the hosts and capabilities match
>+      if (this._permissions[i].rawHost == host &&
>+          this._permissions[i].perm == aCapability) {
>         exists = true;
>         this._permissions[i].capability = capabilityString;
>         this._permissions[i].perm = aCapability;
>         break;

The last assignment in this block is kind of redundant given the new check you're adding. I'm not sure this is correct though, see below.

>     if (!exists) {
>-      host = (host.charAt(0) == ".") ? host.substring(1,host.length) : host;
>+      //check if user entered a host with a leading dot
>+      host = (host.charAt(0) == ".") ? host.substr(1) : host;
>+
>+      /* we need to create a new uri since the previous one may have been
>+      initialized with a leading dot in the host - http://.example.com */
>       var uri = ioService.newURI("http://" + host, null, null);
>+
>       this._pm.add(uri, this._type, aCapability);
>     }

I think what this code wants to do is call add regardless of whether the entry exists already, and just use "exists" to determine whether to use "host" or "this._permissions[i].rawHost". The string munging of "host" is really strange, given that host comes from the result of newURI earlier in the function. I think it might just be a mistake based on other code in this file needing to do that, but I'm too tired to figure it out at the moment. Given the comment you're adding maybe you know what it's about and can explain? When would we see a leading do in the host like that one?

Comment 8

[sdb]

host = (host.charAt(0) == ".") ? host.substr(1) : host;
var uri = ioService.newURI("http://" + host, null, null);

If the two lines above are removed then it's possible for the same host to appear in the permissions list twice.  The script automatically removes leading dots from all hosts before they're displayed in the tree, but it's still possible for hosts to be added to the hostperm.1 file with a leading dot.  For some reason when you create a new uri with newURI('http://.example.com',null,null) it doesn't throw an error.  And when you retrieve the host name from that new uri with uri.host it still contains the leading dot.  Perhaps that part of the nsIURI interface should be changed since .example.com is not a valid host name.

example of hostperm.1 file
google.com 8
.google.com 8

Those two hosts would appear in the permissions tree as:

google.com Allow for session
google.com Allow for session

I seem to remember in previous versions of Firefox prior to v2.0 that the permissions were displayed in the permissions window exactly as they appeared in the hostperm.1 file.  So those two lines of code may have been recent additions to the script.  Although if those lines were added as a precaution to prevent users from adding hosts like .example.com wouldn't they be better served as part of the add() method in the nsIPermissionManager interface?  In the current version it's still possible to add a host with a leading dot to the hostperm.1 file with the nsIPermissionManager add method.  And unfortunately if a host like .example.com does get added to the hostperm.1 file most users will never realize it since it will always appear differently in the permissions window.

this._permissions[i].capability = capabilityString;
this._permissions[i].perm = aCapability;

Regarding the other two lines of code above, the only explanation I can think of for including that code is if a permission is added from some other part of the browser (perhaps an extension) and the permissions window needs to be updated with the correct text (capabilityString) whenever a 'perm-changed' notification is sent.  Although I haven't verified this so it's just a guess.

Comment 9

[sdb]

Attachment: proposed changes for the permissions.js file

After quite a bit of testing it turns out that the two lines of code below are in fact unnecessary.  As a test I commented out both lines and tried changing existing permissions displayed in that window from an extension.  As a result the permission text was changed successfully regardless of whether the code below was included.  So it apparently has nothing to do with the perm-changed observer.

this._permissions[i].capability = capabilityString;
this._permissions[i].perm = aCapability;

I've resubmitted a new patch that removes the code listed above and also includes a couple of other modifications to clean up the code a bit.  I did notice one other issue as I was testing the script.  The observer in the permissions window does not currently deal with deleted permissions.  While that probably will not affect the majority of Firefox users there are extensions that allow you to manipulate the permissions from a UI.  And since the observer ignores deleted permissions then the permissions tree is never updated when a permission is removed from a different UI.  Unfortunately I really don't have time to add that feature since it would require recoding a number of methods in the gPermissionManager and gTreeUtils methods, but at some point it really needs to be included in that script so I've made a note of it in the file.

I've listed the steps below on how to reproduce a bug that causes multiple permissions for the same host to appear in the permissions window.

1. Comment out the code listed below
//host = (host.charAt(0) == ".") ? host.substr(1) : host;
//var uri = ioService.newURI("http://" + host, null, null);
2. Open the cookies exceptions window
3. Type mozilla.org and click Allow
4. Type .mozilla.org and click Allow
5. You'll notice that two mozilla.org hosts now appear in the tree

Let me know if I need to make any changes to the patch file.  And many thanks for taking the time to review this bug. :) Cheers.

Comment 10

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

Ron: you can set the "review" flag to "?" on the attachment "Details" page, and enter my email address in the field, to put the new patch in my review queue.

Comment 11

[sdb]

Attachment: proposed changes for the permissions.js file

Slight change made to the observer.

Comment 12

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

Looks like this problem has already been reported as bug 334363. Ron, could you move your latest patch there? I'm sorry for not having noticed this earlier, but it really would be best to keep this tracked in one place, and bug 334363 is already in the "NEW" state and has some duplicates.