506446 - Clear site preferences (recent or on shutdown) removes default allowed sites

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Black_Ps`]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1

Cleaning private data including site preferences will delete exception for installing addons from addons.mozilla.org and update.mozilla.org is this a bug ?

Reproducible: Always

Steps to Reproduce:
1.Clean private data including site preferences
2.Go to Tools > options security > exceptions , is empty.
3.

Comment 1

[Michael Ryan]

Confirmed.

Comment 2

[Nochum Sossonko [:Natch]]

This sounds pretty bad, did it happen previously (3.0)?

Comment 3

[Black_Ps`]

Only 3.5 has this problem.

Comment 4

[Michael Ryan]

The ability to clear site preferences wasn't in 3.0, so naturally this issue would not be present.

I'm marking this as blocking bug 380852, since that was the bug that exposed this in the UI.

Comment 5

[Samuel Sidler (old account; do not CC)]

Johnath: Is this expected?

Comment 6

[Johnathan Nightingale [:johnath]]

(In reply to comment #5)
> Johnath: Is this expected?

Hmm - nope, this feels like a bug to me. The control was put in place for users to express a privacy choice, to tell us to forget something we'd learned about their browsing habits -- the default exceptions for addons and updates don't reveal any privacy-sensitive information, so I don't think a user would expect them to go away.

Myk, would you agree?

Comment 7

[Black_Ps`]

I think when this bug will be fixed a patch to add the exceptions back will be welcome as many people i think they don't even know that exceptions are empty.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #6)
> Myk, would you agree?

Yes, I agree.  Someone probing a user's browsing habits with those preferences present would be unable to distinguish between "explicitly trusts those sites" and "trusts them by default," so there's no privacy need for us to remove them.

Indeed, removing them identifies the user as probably having cleared their site preferences, since they almost certainly never get cleared otherwise, and we don't want to leak that information unnecessarily.

In general, clearing site preferences should return them to their default values, whether that's "no preferences set" or "the default preferences".

Comment 9

[Johnathan Nightingale [:johnath]]

So, right now sanitize.js just calls nsIPermissionManager's removeAll() method:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.js#357

I could probably be persuaded that sanitize.js should handle this, but my gut reaction is that removeAll() should special case the built-ins and skip over them. I think remove() should still allow a person to remove individual entries including the built-ins, but removeAll() is likely to be understood by most callers as "get back to default", not "break AMO". The IDL comment should be updated to reflect this, too.

Myk, if you agree, I think the patch is probably straightforward, but that permissionManager is your baby, so I wouldn't want to presume.  :)

Comment 10

[Dave Townsend [:mossop]]

These items aren't exactly built in though. They come from the preferences xpinstall.whitelist.add and xpinstall.whitelist.add.103. We should probably just reset those preferences when clearing the permission manager, then when permissions are next checked the defaults will be read in again.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #9)
> So, right now sanitize.js just calls nsIPermissionManager's removeAll() method:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.js#357
> 
> I could probably be persuaded that sanitize.js should handle this, but my gut
> reaction is that removeAll() should special case the built-ins and skip over
> them. I think remove() should still allow a person to remove individual entries
> including the built-ins, but removeAll() is likely to be understood by most
> callers as "get back to default", not "break AMO". The IDL comment should be
> updated to reflect this, too.

I agree, although perhaps this also warrants a name change to something like "resetAll".


> Myk, if you agree, I think the patch is probably straightforward, but that
> permissionManager is your baby, so I wouldn't want to presume.  :)

Actually, the permission manager is part of the NetLib module <http://www.mozilla.org/owners.html#netlib>, which is owned by biesi, with Darin Fisher and bz as peers, and dwitte being the last person to touch nsIPermissionManager.idl (back in 2007).

cc:ing those folks, although I'm not sure how involved they are in the module these days.  biesi, Darin, bz, and dwitte: can you weigh in on Johnathan's proposed solution to this bug?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Thing is...  The permissions manager has no idea about these "built-ins".  They're just added to it at install time like any other permission (see permissions.js _updatePermissions function).

I'm not sure how you were planning to teach permission manager about these, and in theory I have no problem with it being able to tell apart "default" and "user-set" permissions.  Depending on how it's done, it could be a good idea!

dwitte is more familiar with this code than I am, for what it's worth.

Comment 13

[[e-mail]]

What bz said. Either the app listens for the "permissions cleared" notification and re-adds AMO exceptions and such, or we could look at distinguishing permissions by who added them. This would require a schema change in permissions.sqlite, but that's no big deal.

Comment 14

[Black_Ps`]

Firefox 3.6 has the same problem as 3.5,i mean this bug.

Comment 15

[Matthew N. [:MattN]]

This breaks UITour and Lightweight Theme previews worse than XPI installation because functionality is broken for them instead of just another security prompt.

Gavin, can we get this on the radar soon? This has been biting myself and other users for years (but I never knew the cause). This is likely a factor in why up to 9% of users who saw the whatsnew page in the last 30 days didn't get a UITour callback to indicate whether Sync was setup (see bug 1048689 comment #14). As a result, the button to promote Sync didn't appear on the page.

Comment 16

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

I can add to an upcoming iteration. What's a good strategy for fixing this?

Comment 17

[Matthew N. [:MattN]]

There are a few different ways I can think of fixing this (roughly in order of preference):

A) Add a new modification time column and use a special value e.g. -1 to indicate that it was preloaded. (By default the current time would be recorded). This also helps fix bug 771630 because we could just remove permissions that have a modification time in the time window specified. I realize it's not perfect for that other bug but I think it's "good enough" and better than deleting all permissions.

B) Add a new expiration type e.g. EXPIRE_STICKY or EXPIRE_PRELOADED and don't delete them with removeAll.

C) Use a special expiration time that would also need to be checked for EXPIRE_NEVER too in removeAll.

D) All interested consumer listen for the "cleared" observer notification from startup forward and do their PermissionsUtils.importFromPrefs call again (comment 13). I'm not sure if us setting the prefs to  "" now will complicate this or if we already use the default pref branch.

E) Add a single-purpose new column to indicate that the permission is built-in.

F) Somehow use a different appId for permissions that are related to chrome access (e.g. UITour, XPI, etc.)? Don't clear them with the CPD feature.

The solution may want to keep "Forget about this site" in mind and consider still allowing explicit removals from the permission manager dialog and only special-casing removeAll.

Comment 18

[Matthew N. [:MattN]]

Note: This affects both "Clear Recent History" and "Clear History When Firefox Closes".

Comment 19

[:Felipe Gomes (needinfo for replies!)]

I'm planning on going with fix A from Matt's comment 17

Comment 20

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-506446-part-1-add-concept-of-sticky-permissions.patch

I've experimented with an approach and I'd like some feedback.  The approach is:

* Have nsIPermissionManager::Add and AddFromPrincipal take a new optional arg - a flag to indicate if the permission is "sticky" or not (with the default obviously being *not sticky*.)

* The default permissions added via preferences will use this flag to indicate the permissions are sticky (but that part is not in this patch)

* The ::removeAll() method now handles these sticky permissions as a special case so they aren't removed (actually removed then re-added for the in-memory copy).  Note that ::remove() and ::removeFromPrincipal() *do* still remove sticky permissions, so they can be explicitly removed, just not by removeAll() - this could be changed, but it doesn't seem worthwhile to me.

The part I expect to be more controversial:

* The "stickyness" of a permission is handled by adding a new field to nsIPermission (and new column to the DB) - a timestamp of when the permission was added, with -1 being a special-case to flag the item as "sticky".  This will ultimately allow us to fix bug 771630 by allowing us to only delete permissions created after a specified time.

Thus, removeAll() currently deletes any permission with an additionTime >= 0.  In bug 771630, we can expect the addition of a removal function (or a new optional param to removeAll) that allows you to specify the lower-bound of the additionTimes to delete.

The existing tests I ran all pass with this - it does need some additional tests, but I figured I'd get early feedback.  I've nominated bsmedberg and ehsan as they were the authors/reviewers of the most recent nsIPermissionManager API change, but all feedback is obviously welcome.

Comment 21

[Benjamin Smedberg]

Why are we mixing defaults with user values at all? Can we have an entirely separate table of defaults and just do a two-stage lookup (user site pref if found, default site pref if not, then the global default)?

That way the defaults don't end up in the profile at all and we don't have to worry about upgrading them if they change in the future, and all the other complexity here.

Comment 22

[(no longer active)]

Comment on attachment 8476474 [details] [diff] [review]
0001-Bug-506446-part-1-add-concept-of-sticky-permissions.patch

Review of attachment 8476474 [details] [diff] [review]:
-----------------------------------------------------------------

I think a better approach is to store modification times for these permissions.  Or do what bsmedberg suggests.  But we definitely don't want to complicate the interface of the permission manager like this, it's already way too complicated.

Comment 23

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #22)
> I think a better approach is to store modification times for these
> permissions.

I'd be fine with treating the new column as "modification time" instead of "addition time", but this alone wouldn't remove much complexity - we'd still need to have all the schema upgrade code etc, and given bug 771630, code that looks at this new field when doing removals.
 
> Or do what bsmedberg suggests.

I fear that will increase the complexity even more and isn't going to help us avoid the schema change, nor avoid the date-based removal code - it will just push it into a different bug that I'm hoping to fix in the same cycle.  I'll defer more discussion on that until I understand the other points.

> But we definitely don't want
> to complicate the interface of the permission manager like this, it's
> already way too complicated.

Can you please elaborate on this?  Are you referring to the signature change of the existing methods, which I could solve by adding a new method specific to this use-case?  Or is there other complexity you think unnecessary given what we are trying to do in both this bug and bug 771630?

Comment 24

[Benjamin Smedberg]

Comment on attachment 8476474 [details] [diff] [review]
0001-Bug-506446-part-1-add-concept-of-sticky-permissions.patch

I'm not sure I understand how my approach is more complexity: if I were the module owner of this, I think would insist on having completely separate layers for defaults and for user preferences as a design decision.

In any case, I want a clearly-defined behavior for what happens when defaults change, whether that involves recording defaults with a special flag in the database or loading them separately.

Comment 25

[(no longer active)]

I am much more worried about the changes to the nsIPermissionManager API than any of the internal details (the upgrade to the schema etc.)  The latter is something that we try to get right once, but changing the nsIPermissionManager API is something that all consumers of it need to deal with forever.

Also, thinking a bit more about this, maintaining a modification time won't be enough for the clear recent history use case, unless we actually start to maintain the full history of the changes.  For example, let's say that I granted a permission to a web page yesterday, and I just revoked it.  Will the revocation just change a last modified timestamp on the permission entry?  If yes, clear recent history won't be able to revert the change properly because the information on *what* changed at the last modified timestamp is lost.

Based on that, I think we actually have two problems to solve.  One is addressing the clear recent history use case, the other being addressing the clear site prefs use case (this bug.)  For the latter, I now think that Benjamin's idea is the way to go.  For the former, we really need to think about the exact UX we want to support, and we need to determine answers as to what should happen in cases such as the above.

Comment 26

[Matthew N. [:MattN]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #25)
> Also, thinking a bit more about this, maintaining a modification time won't
> be enough for the clear recent history use case, unless we actually start to
> maintain the full history of the changes.  For example, let's say that I
> granted a permission to a web page yesterday, and I just revoked it.  Will
> the revocation just change a last modified timestamp on the permission
> entry?  If yes, clear recent history won't be able to revert the change
> properly because the information on *what* changed at the last modified
> timestamp is lost.

I already mentioned this in bug 771630 comment 7 but I don't think it's a blocker for the permission change since using just the modification time is much better than the current state of losing ALL permission data. Secondly, most of our permissions have a sane defaults and include prompts to let the user re-set the permission the next time it's used after being cleared instead of reverted. Thirdly, I think it's rare for someone to do a 180 and switch a permission from always allow to always deny since normally you find the domain permission unnecessary or you don't. Again the worst case for the people who use this feature is that we go back to the default (usually prompting). Fourthly, I believe not keeping the change history for this feature is on par with the other checkboxes for Clear Recent History e.g. Cookies and Form History. We don't revert cookie values or form history metadata columns to their previous values when the feature is used.

Comment 27

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-proof-of-concept-patch-for-reading-default-permissio.patch

I've been wondering if we can avoid the database completely for these default permissions.  One obvious reason is that the front-end goes to pains to ensure these default permissions are only loaded from prefs once - however, if something goes wrong and the database gets clobbered (which can happen in nsPermissionManager if it finds a database it doesn't like) the front-end doesn't know and the permissions remain lost.  Eg, see bug 1050080 which wants to re-add these permissions once this bug is fixed.

nsPermissionManager already has code to import preferences from a text file.  This could be leveraged to import a default set of permissions from within the app directory.  Whenever the database is initialized, this file would be read and added to to the in-memory copy of the permissions database - they'd never need to be written to the disk copy.

Best I can tell from trawling over the code, doing things this way would involve reading the following 4 lines from disk:

--8<--
host	uitour	1	www.mozilla.org
host	uitour	1	support.mozilla.org
host	install	1	addons.mozilla.org
host	install	1	marketplace.firefox.com
--8<--
(and maybe some #comment lines if we choose)

And we'd be able to remove one .jsm file and a number of default preferences.  It would involve a small amount of main-thread IO during startup, but there may be other ways to store this to mitigate that - in theory, we could read this from anything that can support nsILineInputStream.

The robustness of this approach appeals, but there may be something I'm missing.  I've a proof-of-concept patch that I've attached, and it seems fairly light-weight.  Is this worth exploring further?

Comment 28

[Benjamin Smedberg]

Comment on attachment 8481124 [details] [diff] [review]
0001-proof-of-concept-patch-for-reading-default-permissio.patch

I like the approach. The only thing I'm concerned about in terms of the actual patch is that we document _DoImport better; I can tell from reading the patch that `conn` is null when we're importing defaults, but that might not be obvious to future engineers and in the method itself it's rather obscure. At least add a big scary comment.

Comment 29

[Mark Hammond [:markh] [:mhammond]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #28)
> Comment on attachment 8481124 [details] [diff] [review]
> 0001-proof-of-concept-patch-for-reading-default-permissio.patch
> 
> I like the approach.

Thanks!  While looking at removing PermissionsUtils.jsm, I came across toolkit/mozapps/extensions/test/xpcshell/test_permissions.js.  This has the comment:

// Checks that permissions set in preferences are correctly imported but can
// be removed by the user.

and it explicitly tests that when nsIPermissionManager::RemoveAll() is called, all such defaults are removed.  That behaviour leads explicitly to this bug - but the approach I'm taking here means it becomes impossible for the user to remove the permission to install apps from, eg, addons.mozilla.org

This seems to leave us in a catch 22 - on one hand we want to give the user the choice to remove such permissions, but on the other hand we are giving the user a gun pointed at their foot by using RemoveAll the way we are - user's simply aren't going to expect a "clear recent history" -> "everything" removes builtin permissions.

Any thoughts on how to resolve this dilemma?  My thoughts:

* Don't expose a way for the user to remove these builtin permissions.
* Have a hidden pref to allow us to skip loading the file with defaults - you get all builtin defaults, or you get none.
* Have hidden pref with the names of files that should be loaded as defaults.  The default value for this pref would be 2 files - one for xpinstall, one for uitour.  This becomes complex if we try and handle path names.

I personally think the first is fine but I may underestimate the shouting from the vocal minority this might cause...

Comment 30

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mark Hammond [:markh] from comment #29)
> but the approach I'm taking here means it becomes impossible for
> the user to remove the permission to install apps from, eg,
> addons.mozilla.org

... and I just remembered we expose UI which explicitly allows the user to remove, eg, addons.mozilla.org as a trusted site for addon installation - so I guess we need to discuss this a little more.  Removing needinfo for now...

Comment 31

[Benjamin Smedberg]

> // Checks that permissions set in preferences are correctly imported but can
> // be removed by the user.
> 
> and it explicitly tests that when nsIPermissionManager::RemoveAll() is
> called, all such defaults are removed.  That behaviour leads explicitly to
> this bug - but the approach I'm taking here means it becomes impossible for
> the user to remove the permission to install apps from, eg,
> addons.mozilla.org

We do expect that users have the ability to explicitly override defaults. I'm not convinced that .removeAll is the right API for this, however: I'd suggest that in the UI, the user just sets an alternate setting for the domain in question, and so the user permission will override the default permission.

If the user *clears* preferences for a site, or for all sites, I believe we should revert to the Firefox defaults.

But I don't know how the existing UI elements map to setting user-site-prefs versus clearing site prefs.

Comment 32

[Matthew N. [:MattN]]

Comment on attachment 8481124 [details] [diff] [review]
0001-proof-of-concept-patch-for-reading-default-permissio.patch

Clearing flag since we had an in-person discussion.

Comment 33

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-506446-part-1-nsPermissionManager-now-reads-a-fi.patch

This patch is similar to the previous version, but allows these "default" permissions to be overridden by the existing nsIPermissionsManager methods.  How it works:

* A file with permission defaults is read as the DB is initialized.  (Note this patch still reads a file, but I intend to change this to load a resource:// URI to avoid the extra file opening at startup.)  Permissions read from this file are added to the in-memory copy of the permissions with a special ID of -1, but *not* written to the DB.

* If pm.add() is called for a builtin permission, then we write a row to the DB as normal, overriding the default.

* If pm.remove() is called for one of these default permissions, we write an entry to the DB with a permission of UNKNOWN_ACTION.  Before this patch, that value is never written to the DB, so this is a significant change.  This is so future tests against that permission return this UNKNOWN_ACTION value rather than the default, and so the UNKNOWN_ACTION value is persisted across restarts.  Note that the permissions enumerator does *not* return this value - so to consumers of the iterators it appears as though this permission doesn't exist at all.

* pm.removeAll() removes all rows and overrides, thereby re-instating the default permissions.  IOW, .removeAll() is more of a .resetAll().

* The patch has reasonably extensive tests.

I hope to have the patch updated to read the defaults from a resource:// URI in the next few days, at which time I'll be asking bsmedberg for review, but posting the WIP here now incase anyone wants to have a look...

Comment 34

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-506446-part-2-replace-all-permissions-via-prefs-.patch

This patch removes all the code that does preference-based-defaults and adds those defaults to the file read by nsPermissionManager.  This defaults file will eventually end up as a resource:// URI, but I figure it's worth getting feedback now.

Comment 35

[Matthew N. [:MattN]]

Comment on attachment 8485070 [details] [diff] [review]
0002-Bug-506446-part-2-replace-all-permissions-via-prefs-.patch

Review of attachment 8485070 [details] [diff] [review]:
-----------------------------------------------------------------

Once this lands we should send an email to the enterprise mailing list to let organizations who currently remove our default permissions to know about the new way. I think this patch belongs in bug 1050080.

Comment 36

[Mark Hammond [:markh] [:mhammond]]

I've got the code working that reads the default permissions from a resource:// URI, however, I'd like some feedback on where in the tree to place this magic - we might as well get the bike-shedding done before I put the new patch up :)

My current patch has:

* A new file browser/app/default_permissions - this is the text file in the format supported by the permission manager.

* A new file browser/app/jar.mn which has the entry:
   defaults/default_permissions (default_permissions)

* nsPermissionsManager looks for a pref 'permissions.manager.defaultsUrl', which defaults to 'resource://app/chrome/browser/defaults/default_permissions'

I'd like feedback on:

* The name of the file - maybe it should be default_permissions.txt?  Or something else entirely?

* The location of this file - is browser/app the best place, even though that means a new jar.mn?  Or would browser/base/content be better?

* Does 'defaults/default_permissions' creating a new logical directory named 'defaults' make sense?  Or should it just be in the top-level, making the resource URL resource://app/chrome/browser/default_permissions ?

* The preference name and anything else.

Benjamin, flagging you for feedback, but I'll take it from anyone ;)

Comment 37

[Benjamin Smedberg]

> * A new file browser/app/default_permissions - this is the text file in the
> format supported by the permission manager.
> * A new file browser/app/jar.mn which has the entry:
>    defaults/default_permissions (default_permissions)

That's fine.

> * The name of the file - maybe it should be default_permissions.txt?  Or
> something else entirely?

Don't care.

> * The location of this file - is browser/app the best place, even though
> that means a new jar.mn?  Or would browser/base/content be better?

browser/app is close to browser/app/profile where the Firefox default prefs file lives, so ok. (Probably firefox.js should be in browser/app not browser/app/profile anyway.

> * Does 'defaults/default_permissions' creating a new logical directory named
> 'defaults' make sense?  Or should it just be in the top-level, making the
> resource URL resource://app/chrome/browser/default_permissions ?

Don't care. The file name already has "default" in it, so the extra dir isn't necessary, but there is no performance or other overhead.

Comment 38

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-506446-nsPermissionManager-now-reads-a-file-with.patch

This patch is the same as described in detail in comment 33, but with the following changes:

* The defaults are read from a URL instead of a file. This URL can be specified by a preference 'permissions.manager.defaultsUrl' with the default being 'resource://app/chrome/browser/default_permissions'.  The file behind this URL is included in the patch, but the file contains only comments - no defaults are actually added.  There's nothing to check the URL is sane (which could be an issue - the pref only exists for testing)

* The line-reading code in the import function has been replaced with NS_ReadLine() - only files have nsILineInputString. 

* The new test was orange due to the permission manager using async DB updates, so sometimes the test would see "stale" data for the async completed.  I fixed this by having the check against the DB retry if it finds an unexpected value and giving up after 20 * 100ms retries.

Try at https://tbpl.mozilla.org/?tree=Try&rev=a3a9210acc3e - note this also includes the "part 2", which as Matt correctly points out, belongs in bug 1050080.

Comment 39

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/fx-team/rev/4223912e9f76

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4223912e9f76

Comment 41

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.8.5 using latest Nightly 35.0a1 (buildID: 20140914030209).