Mike Perry Reporter
	Description • 10 years ago

In Tor Browser, we had to patch the Permissions Manager to make it memory only, to avoid leaking site information to disk.

Our patch provides a global pref to make the Permissions Manager memory only. It does not support Private Browsing Mode notifications or window detection.

This patch is against FF24ESR.

	(no longer active)
	Comment 1 • 10 years ago

This is intentional.  The permission manager is used for explicit permissions for the website that are directly granted or denied by the user through explicit actions, and therefore we persist them in private browsing mode just like we preserve bookmarks and downloads.

	Mike Perry Reporter
	Comment 2 • 10 years ago

Hrmm.. Are you sure that site permissions should be the same between private and non-private browsing mode though?

Unlike bookmarks and downloads, they are effectively exposed to content, and control how it behaves. For example, it seems like it would be quite common for users to grant maps.google.com access to their location and offline storage in normal mode, but perhaps not want those permissions to apply to their private browsing interactions with maps.google.com.

	(no longer active)
	Comment 3 • 10 years ago

(In reply to comment #2)
> Hrmm.. Are you sure that site permissions should be the same between private
> and non-private browsing mode though?
> 
> Unlike bookmarks and downloads, they are effectively exposed to content, and
> control how it behaves. For example, it seems like it would be quite common for
> users to grant maps.google.com access to their location and offline storage in
> normal mode, but perhaps not want those permissions to apply to their private
> browsing interactions with maps.google.com.

Hmm, why do you think that those permissions should not be honored in private browsing mode?

	Mike Perry Reporter
	Comment 4 • 10 years ago

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailacopolypse) from comment #3)
> (In reply to comment #2)
> > Hrmm.. Are you sure that site permissions should be the same between private
> > and non-private browsing mode though?
> > 
> > Unlike bookmarks and downloads, they are effectively exposed to content, and
> > control how it behaves. For example, it seems like it would be quite common for
> > users to grant maps.google.com access to their location and offline storage in
> > normal mode, but perhaps not want those permissions to apply to their private
> > browsing interactions with maps.google.com.
> 
> Hmm, why do you think that those permissions should not be honored in
> private browsing mode?

Primarily because I think a person's relationship with a site is different depending on if they are browsing it privately or not. I've seen people use PB/Incognito mode specifically for the purpose of not having certain things directly recorded in the same Google account they have open in normal mode, or just to use a different Google account.

To me, this change in relationship to a site depending on PBM/non-PBM suggests that people would likely not want their site permissions to apply across both modes, either.

I also don't really like the idea of a site prompting a user for something that could generate a disk record in PBM. Many users will just click OK without thinking about the consequences. Personally, I would have expected PBM permissions to persist for that session, just like everything else in the mode. Especially for choices made in the door-hanger prompts based on site request.

	Tim Taubert [:ttaubert] (inactive)
	Comment 5 • 10 years ago

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailacopolypse) from comment #1)
> This is intentional.  The permission manager is used for explicit
> permissions for the website that are directly granted or denied by the user
> through explicit actions, and therefore we persist them in private browsing
> mode just like we preserve bookmarks and downloads.

It looks like in bug 461625 we explicitly disabled UI that accesses the permission manager to not change permissions permanently from private windows. Bug 970675 was wontfixed referring to bug 461625, it seems like we should either allow permanent permission changes from private windows or not, but not both.

I see that bug 970675 is about explicitly allowing a thing in private windows whereas this bug is rather about not inheriting non-private window permissions. It feels very similar to me though and I think it would be good to streamline our behavior.

With the approach that Mike suggests, we could probably revert the changes from bug 461625 and allow users to change permissions in private windows without persisting them, i.e. bug 970675. At the same time we would also re-ask for permissions in private windows to ensure the user grants them to sites re-visited in private mode as well.

	Mike Perry Reporter
	Comment 6 • 10 years ago

So what is the way forward here? Can this 'clean slate for site Permissions in PBM' idea be made part of bug 970675's implementation, or can we reopen this bug?

Many things in Private Browsing Mode are already a 'clean slate' from non-private browsing rather than append-only on normal mode tracking data, and I think many users expect this behavior. It is certainly a key part of the model we are striving for with Tor Browser, and anything less is rather a blocker for us towards being able to recommend the use of vanilla Firefox with Tor (at least, if we shoot for the 'Install this addon to make PBM really private' use case).

	(no longer active)
	Comment 7 • 10 years ago

Sorry I dropped the ball here, Mike.  (Please use the needinfo? flag, that way I'm usually very responsive, but I'm continually unable to keep up with my bugmail... :/)

(In reply to Mike Perry from comment #4)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailacopolypse) from comment #3)
> > (In reply to comment #2)
> > > Hrmm.. Are you sure that site permissions should be the same between private
> > > and non-private browsing mode though?
> > > 
> > > Unlike bookmarks and downloads, they are effectively exposed to content, and
> > > control how it behaves. For example, it seems like it would be quite common for
> > > users to grant maps.google.com access to their location and offline storage in
> > > normal mode, but perhaps not want those permissions to apply to their private
> > > browsing interactions with maps.google.com.
> > 
> > Hmm, why do you think that those permissions should not be honored in
> > private browsing mode?
> 
> Primarily because I think a person's relationship with a site is different
> depending on if they are browsing it privately or not. I've seen people use
> PB/Incognito mode specifically for the purpose of not having certain things
> directly recorded in the same Google account they have open in normal mode,
> or just to use a different Google account.
> 
> To me, this change in relationship to a site depending on PBM/non-PBM
> suggests that people would likely not want their site permissions to apply
> across both modes, either.

I understand this, but this is a misconception of the user which we should arguably try to correct.  I mean, a simple ETag-based attack would allow websites to track you across private/public windows.  I'm not sure I understand why you think this user perception is correct.

> I also don't really like the idea of a site prompting a user for something
> that could generate a disk record in PBM. Many users will just click OK
> without thinking about the consequences. Personally, I would have expected
> PBM permissions to persist for that session, just like everything else in
> the mode. Especially for choices made in the door-hanger prompts based on
> site request.

We don't really prompt, our default bookmark UI is one-click.  But really, bookmarking something without remembering it doesn't really mean much.  (Note that we *do* protect against marking the bookmark as visited in your history obviously.)

(In reply to Tim Taubert [:ttaubert] from comment #5)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailacopolypse) from comment #1)
> > This is intentional.  The permission manager is used for explicit
> > permissions for the website that are directly granted or denied by the user
> > through explicit actions, and therefore we persist them in private browsing
> > mode just like we preserve bookmarks and downloads.
> 
> It looks like in bug 461625 we explicitly disabled UI that accesses the
> permission manager to not change permissions permanently from private
> windows. Bug 970675 was wontfixed referring to bug 461625, it seems like we
> should either allow permanent permission changes from private windows or
> not, but not both.

Tim, bug 461625 only disabled some of the UI which would allow you to set permissions.  Have you read bug 461625 comment 14 for example?  Our stance on this has not changed so far (not to say that I'm not open to a change if a good case can be made for it.)

> I see that bug 970675 is about explicitly allowing a thing in private
> windows whereas this bug is rather about not inheriting non-private window
> permissions. It feels very similar to me though and I think it would be good
> to streamline our behavior.

See the last paragraph of bug 970675 comment 5?  What I said there was that I'm fine with a patch which keep track of only that menu item while the window is open without touching the permission manager.  Note that the items which we have disabled in the UI are the ones which can potentially be confusing to the user as they're not obviously asking for a permanent change to be remembered (as opposed to the bookmark case above, for example.)

> With the approach that Mike suggests, we could probably revert the changes
> from bug 461625 and allow users to change permissions in private windows
> without persisting them, i.e. bug 970675. At the same time we would also
> re-ask for permissions in private windows to ensure the user grants them to
> sites re-visited in private mode as well.

I agree that if we decide to take a patch in this bug, bug 461625 needs to be backed out and bug 970675 would be automatically fixed.

(In reply to Mike Perry from comment #6)
> So what is the way forward here? Can this 'clean slate for site Permissions
> in PBM' idea be made part of bug 970675's implementation, or can we reopen
> this bug?

FWIW, no matter what we end up deciding here, the scope of bug 970675 is much more limited than this (since there I'm suggesting to not touch the permission service as part of the command handler for that menu item in private windows.)

> Many things in Private Browsing Mode are already a 'clean slate' from
> non-private browsing rather than append-only on normal mode tracking data,
> and I think many users expect this behavior. It is certainly a key part of
> the model we are striving for with Tor Browser, and anything less is rather
> a blocker for us towards being able to recommend the use of vanilla Firefox
> with Tor (at least, if we shoot for the 'Install this addon to make PBM
> really private' use case).

Hmm, we actually never ended up discussing the specific use cases here, I just realized.  Do you care about any specific parts of our UI which use the permission manager, or are you actually worried about the API surface of the permission manager service?  The patch you provided handles the latter, but I'm not sure if that's the intention.

Also, I don't really know if I have much more to add to my side of the argument.  To be honest, I think there is one hole in my argument, let me describe that.  Back when we made this decision originally, we had the global private browsing mode implementation.  One of the things we wanted to accomplish back then was making it possible to manipulate these permissions in the Options dialog.  With per-window private browsing, through not showing any PB theme in the Options dialog, we've basically conveyed the non-private nature of that dialog, so we're stuck with some of the per-window permission setting UI (the ones touched in bug 461625) which are disabled in private windows, and a bunch of others (such as the doorhanger UIs) which can set the permissions.)  That sort of swings me to Mike's side of the argument...

Josh, what do you think about this?  (Mike, Josh is a peer of the private browsing module.)

	(no longer active)
	Comment 8 • 10 years ago

(Reopening at least for the ongoing discussion)

	Arthur Edelstein [:arthur] Assignee
	Comment 9 • 9 years ago

Here's the same patch updated to mozilla-central, which introduces a pref, "permissions.memory_only" that prevents the Permissions manager from writing to disk (regardless of whether we are in PBM). This works well for Tor Browser and could be useful in a future profile-based "Tor mode". Would this approach be acceptable?

	(no longer active)
	Comment 10 • 9 years ago

Yes, I think the general approach of making this depend on a pref is acceptable, but why do you need to handle dynamic changes to the pref?  Is that something that the Tor Browser needs?  I would be much more comfortable if we didn't do this, since it can break the consumers of the permission manager in unexpected ways...

	Arthur Edelstein [:arthur] Assignee
	Comment 11 • 9 years ago

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #10)
> Yes, I think the general approach of making this depend on a pref is
> acceptable, but why do you need to handle dynamic changes to the pref?  Is
> that something that the Tor Browser needs?  I would be much more comfortable
> if we didn't do this, since it can break the consumers of the permission
> manager in unexpected ways...

Good point -- as far as I can tell, a static pref will suit Tor Browser's needs.

	Arthur Edelstein [:arthur] Assignee
	Comment 12 • 9 years ago

Here's a new version of the patch, where the pref is only read once when the permissions manager starts up.

	(no longer active)
	Comment 13 • 9 years ago

Comment on attachment 8620595 [details] [diff] [review]
0001-Bug-967812-Pref-to-make-Permissions-Manager-memory-o.patch

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

r=me with the following nits addressed.  Thanks!

::: extensions/cookie/nsPermissionManager.cpp
@@ +385,4 @@
>  
>  nsPermissionManager::nsPermissionManager()
>   : mLargestID(0)
> + , mMemoryDBOnly(false)

Nit: please keep the order of initialization consistent with the order in which the members are declared in the class definition.  Otherwise, the compiler will emit warnings about this, and that can break build configurations which treat warnings as errors.

@@ +427,5 @@
>  {
>    nsresult rv;
>  
> +  mMemoryDBOnly = mozilla::Preferences::GetBool("permissions.memory_only", false);
> + 

Nit: please remove the trailing whitespace.

::: extensions/cookie/nsPermissionManager.h
@@ +327,5 @@
>    nsCOMPtr<mozIStorageAsyncStatement> mStmtDelete;
>    nsCOMPtr<mozIStorageAsyncStatement> mStmtUpdate;
>  
> +  bool mMemoryDBOnly;
> +  

Nit: please rename this to mMemoryOnlyDB.  Also, please remove the trailing whitespace.  :-)

	Arthur Edelstein [:arthur] Assignee
	Comment 14 • 9 years ago

Thanks for the review! I have made all requested changes in this new version of the patch.

Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f69aee249f9

	Pulsebot
	Comment 15 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/9361bef1aefe

	Phil Ringnalda (:philor)
	Comment 16 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/9361bef1aefe