461627 - Hide the UI for saving certificate exceptions permanently in Private Browsing mode

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[(no longer active)]

Based on bug 460342 comment 3, this bug is filed in order to discuss whether we need to hide the UI for saving certificate exceptions while in Private Browsing mode.  These exceptions, if saved permanently, may enable others to determine which sites you've been visiting in a private session, and I think we need to avoid this.

There are two approaches here: first one being handling this in the backend like my initial patches used to do, by treating "permanent" permission entries as "temporary" when the private browsing mode was active, second being hiding the checkbox for storing the exception permanently inside the certificate exceptions dialog.

Comments welcome.

Comment 1

[Mike Connor [:mconnor]]

The second approach is much less invasive, and I'd prefer to simply do that.  I think if you modify the backend, then the prefwindow becomes mysteriously impermanent in places.

Comment 2

[(no longer active)]

OK, I'll give it a go.

Comment 3

[(no longer active)]

Attachment: Patch (v1)

Simple patch to hide and uncheck the checkbox when the dialog is opened in the private mode.

Comment 4

[(no longer active)]

Kai: ping?

Comment 5

[(no longer active)]

Comment on attachment 344821 [details] [diff] [review]
Patch (v1)

Switching the review request to johnath in hopes of a quick review.  :-)

johnath: this should be a fairly easy review.

Comment 6

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

Ideally it seems like nsICertOverrideService should also fail to add the exception in PB mode (at least with aTemporary = false), even if we hide the UI here. Followup bug?

Comment 7

[(no longer active)]

We discussed this with mconnor, and he feels that a UI-only change is enough here to minimize changes to core code.

For reference, my previous implementation of private browsing used to handle this in the override service by forcing all entries to be temporary while inside the private browsing mode.  See attachment 301098 [details] [diff] [review] for example.

Comment 8

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

(In reply to comment #7)
> We discussed this with mconnor, and he feels that a UI-only change is enough
> here to minimize changes to core code.

For now, sure. But the right thing in the longer term is to do both, in my opinion. That is why I suggested a followup bug.

Comment 9

[Johnathan Nightingale [:johnath]]

Comment on attachment 344821 [details] [diff] [review]
Patch (v1)

I would not expect it to behave this way.  I would expect private browsing mode to change the default for the checkbox to unchecked, and *maybe* even disable the checkbox to give users the feedback that private browsing was preventing permanent exceptions, but I wouldn't expect the checkbox to disappear.

If there's an argument somewhere that I missed about why hiding it was better, please point me to it, but in other cases we have made the opposite decision (disabling a menu item, say, instead of just removing it).

It's Kai's component, but you can have my r+ now for a version that disables the checkbox instead of hiding it, or you can point me to an argument in favour of removing it, and I'll take a look between flights today.

Comment 10

[(no longer active)]

Hiding it was what mconnor suggested, but I also think that disabling it might be more appropriate.  I guess if mconnor doesn't object, we can take a patch which disables the checkbox for now.

Comment 11

[(no longer active)]

Oh, and please note that we're hiding UI elements in at least one more place: bug 461625.  mconnor: what do you think?  It sounds to me that maybe we should disable those as well...

Comment 12

[Mike Connor [:mconnor]]

Yeah, we should be disabling.  Except in the context menu case.

Comment 13

[(no longer active)]

Attachment: Patch (v1.1)

Disable the checkbox instead of hiding it.

Comment 14

[Johnathan Nightingale [:johnath]]

Comment on attachment 346019 [details] [diff] [review]
Patch (v1.1)

>@@ -76,16 +77,27 @@ function initExceptionDialog() {
>+  // The Private Browsing service might not be available
>+  try {
>+    var pb = Components.classes["@mozilla.org/privatebrowsing;1"].
>+             getService(Components.interfaces.nsIPrivateBrowsingService);
>+    if (pb.privateBrowsingEnabled) {
>+      var permanentCheckbox = document.getElementById("permanent");
>+      permanentCheckbox.setAttribute("disabled", "true");
>+      permanentCheckbox.removeAttribute("checked");
>+    }
>+  } catch (ex) {}

I think the comment here should reflect the reason we're including this block of code, not the reason it's in a try catch.  E.g.

// If the Private Browsing service is available and the mode is active,
// don't store permanent exceptions, since they would persist after 
// private browsing mode was disabled.

r+ with the comment, though I'm not technically a module peer here.

Comment 15

[Reed Loden [:reed]]

This is not "ready for landing". As johnath pointed out, he's not a valid reviewer for this code.

Comment 16

[(no longer active)]

(In reply to comment #15)
> This is not "ready for landing". As johnath pointed out, he's not a valid
> reviewer for this code.

Oh, sorry about that.  mconnor mentioned that johnath can review this on IRC, hence the confusion.

Comment 17

[(no longer active)]

Landed with mconnor's approval:

http://hg.mozilla.org/mozilla-central/rev/c504956ed84b

Marcia: needs a Litmus test.  I can probably get around to write an automated test for it as well if you feel it's worth it.

Comment 18

[Marcia Knous [:marcia]]

This doesn't seem to be working right for me using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081104 Minefield/3.1b2pre. In PB mode I can still see the entire UI and add the certificate exception.

Comment 19

[Marcia Knous [:marcia]]

Ok, after revisiting this I see that the checkbox that says "Permanently store this exception" is supposed to be disabled. Using today's nightly build, the checkbox is not disabled, and I can permanently store an exception in PB mode and have it show in regular mode. Reopening....

Comment 20

[Kai Engert (:KaiE:)]

I'm ok with both hiding or disabling, and I'm fine with Jonath's review for this code.

Although I would have preferred a more general approach to private browsing, that disables all storage on some lower I/O layer, I understand that won't happen, and we're going to make individual UI aware of PB where needed.

The patch has a comment that says "service might not be available".
Is there a specific reason *why* not?
Should that potential/likely reason get added to the comment?

Could the same reason be the cause for Marcia's test failure?

Comment 21

[Kai Engert (:KaiE:)]

(disabling the checkbox has the advantage that we can visually verify that the checkbox is unchecked during testing, because you haven't changed the code that actually evaluates the state of checkbox. as a general advice I'd prefer that tests "to store or not to store" happens right before storing, instead of relying of some earlier indirect event (like setting a checkbox to disabled/unchecked))

Comment 22

[(no longer active)]

Attachment: Patch (v2)

OK, in the haste of landing private browsing, I blindly assumed that if the patch v1.0 worked with hidden, v1.1 should work with s/hidden/disabled/, but I was wrong. :(

The problem is that the disabled property is set to true in updateCertStatus, which was overriding my code.

So I moved this to updateCertStatus, and also changed the checkbox to be unchecked by default in the xul file.  This won't change the interaction outside of PB mode (inPrivateBrowsingMode will be false, so checked will become true) and correctly unchecks and disables the checkbox in PB mode (test it this time).

So, Kai can you review this please?

Comment 23

[(no longer active)]

Comment on attachment 346671 [details] [diff] [review]
Patch (v2)

(In reply to comment #21)
> (disabling the checkbox has the advantage that we can visually verify that the
> checkbox is unchecked during testing, because you haven't changed the code that
> actually evaluates the state of checkbox. as a general advice I'd prefer that
> tests "to store or not to store" happens right before storing, instead of
> relying of some earlier indirect event (like setting a checkbox to
> disabled/unchecked))

Oh, sorry didn't see this comment before preparing the patch.  New patch on its way.

Comment 24

[(no longer active)]

Attachment: Patch (v2.1)

This is nearly the same as the previous patch, only in addition to handling the UI, I explicitly disable storing the override permanently inside the PB mode.

Comment 25

[(no longer active)]

(In reply to comment #20)
> The patch has a comment that says "service might not be available".
> Is there a specific reason *why* not?
> Should that potential/likely reason get added to the comment?

I edited that comment upon landing as per johnath, but what it means that currently the PB service resides in browser/, so anything which may be used where browser/ doesn't exist must expect this service not to be present all the time.  This is of course going to change with bug 462832, so I thought it's not worth explaining this in the patch...

Comment 26

[(no longer active)]

Comment on attachment 346674 [details] [diff] [review]
Patch (v2.1)

Johnath: can you take a look at this?  It would be great if we can take this for Beta 2...

Comment 27

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

Comment on attachment 346674 [details] [diff] [review]
Patch (v2.1)

We can do this after beta, and I don't want to rush it.

Comment 28

[Mike Connor [:mconnor]]

I am worried that with this patch, we'll also lose permanent exceptions set via the prefwindow, where everything else set there will persist...

Comment 29

[(no longer active)]

Attachment: Patch (v2.2)

(In reply to comment #28)
> I am worried that with this patch, we'll also lose permanent exceptions set via
> the prefwindow, where everything else set there will persist...

This new patch handles this as well.  It only disables the checkbox control when inside the private browsing mode, and if the dialog has been invoked from the certificate error page.

Requesting review on browser/ part (trivial one-liner) from mconnor, and on the security/ part from kaie.

Comment 30

[Kai Engert (:KaiE:)]

Comment on attachment 348760 [details] [diff] [review]
Patch (v2.2)

+      document.getElementById("permanent").disabled = inPrivateBrowsing;
+      document.getElementById("permanent").checked = !inPrivateBrowsing;

please change to 

  var pe = document.getElementById("permanent");
  pe.disabled = inPrivateBrowsing;
  pe.checked = !inPrivateBrowsing;

r=kaie with that change

Comment 31

[(no longer active)]

Attachment: Patch (v2.3)

Made the requested change in exceptionDialog.js.

Comment 32

[(no longer active)]

Comment on attachment 357098 [details] [diff] [review]
Patch (v2.3)

Switching review to gavin, since mconnor seems super-busy.  Gavin: it's a one-liner change to review in browser.js.  :-)

Comment 33

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

Comment on attachment 357098 [details] [diff] [review]
Patch (v2.3)

>diff --git a/security/manager/pki/resources/content/exceptionDialog.js b/security/manager/pki/resources/content/exceptionDialog.js

>+function inPrivateBrowsingMode() {

>+  if (args && args[0] && args[0].handlePrivateBrowsing) {
>+    // detect if the private browsing mode is active
>+    try {
>+      var pb = Components.classes["@mozilla.org/privatebrowsing;1"].
>+               getService(Components.interfaces.nsIPrivateBrowsingService);
>+      return pb.privateBrowsingEnabled;

If someone requested that we handlePrivateBrowsing, and we can't get the private browsing service, shouldn't we fail? Or at least print an error to the console? I really hate all these try/catches, I sure wish bug 462832 was fixed! :)

Comment 34

[(no longer active)]

(In reply to comment #33)
> If someone requested that we handlePrivateBrowsing, and we can't get the
> private browsing service, shouldn't we fail? Or at least print an error to the
> console?

I think it would be best to print an error to the console.  I'll do so upon landing.

> I really hate all these try/catches, I sure wish bug 462832 was fixed!
> :)

Me too.  The problem is, that bug would need some refactoring, and I still have quite a few PB patches in my mq which need to land before I can fix bug 462832... :/

Comment 35

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/496d8d83f310

Comment 36

[Marcia Knous [:marcia]]

Does this need to go on the 1.9.1 branch as well?

Comment 37

[Marcia Knous [:marcia]]

Verified fixed on the trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090211 Minefield/3.2a1pre and : Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090211 Minefield/3.2a1pre. The "Permanently Store this Exception" is disabled in the UI while in the PB mode.

Comment 38

[(no longer active)]

(In reply to comment #36)
> Does this need to go on the 1.9.1 branch as well?

Yes, I've requested approval for the patch to land on 1.9.1.

Comment 39

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

Comment on attachment 357098 [details] [diff] [review]
Patch (v2.3)

Can you add a test for this and then poke me on IRC? I'll approve it then.

Comment 40

[(no longer active)]

Attachment: Test

Automated test for the patch landed for this bug.

Comment 41

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

Comment on attachment 369838 [details] [diff] [review]
Test

If for some reason the test fails and win.gCert is never set, this test will fail to cancel the timer or close its windows, which isn't optimal.

I'd much prefer adding either an event or observer notification in updateCertStatus to avoid having to use a polling timer here.

Comment 42

[(no longer active)]

Attachment: Test (v2)

Addressed review comments.

Comment 43

[(no longer active)]

Test landed as <http://hg.mozilla.org/mozilla-central/rev/b4f1f0e4f332>.

Comment 44

[(no longer active)]

Attachment: Combined 1.9.1 patch

Comment 45

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

Comment on attachment 392176 [details] [diff] [review]
Combined 1.9.1 patch

Approved for 1.9.1.3. a=ss

Comment 46

[Justin Dolske [:Dolske]]

Pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4c10a82ac37b

Comment 47

[Al Billings [:abillings - ex-MoCo]]

Verified fixed in 1.9.1.3 as well with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.3pre) Gecko/20090817 Shiretoko/3.5.3pre (.NET CLR 3.5.30729).