1253771 - Disabling mixed content blocking results in no indication of cert override

Status: VERIFIED FIXED

(Firefox :: General, defect, P3)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Screenshot

For sites with both mixed active content and a user-overridden cert, when mixed content blocking is disabled, there is no indication at all in the control center UI that the cert was overridden. Not sure what the right thing to do is here. Showing text for both will be difficult in the small popup panel. Screenshot attached, though FWIW it's basically the same UI that would be displayed if there was no cert exception.

Comment 1

[Paul Silaghi, QA [:pauly]]

Also, the "Remove exception" button disappears after disabling the protection.

Comment 2

[Johann Hofmann [:johannh]]

Attachment: Bug 1253771 - Add previous state info to mixed content callback.

Review commit: https://reviewboard.mozilla.org/r/49171/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49171/

Comment 3

[Johann Hofmann [:johannh]]

Comment on attachment 8745921 [details]
Bug 1253771 - Add previous state info to mixed content callback.

I'm still figuring out how to use MozReview so bare with me and please notify me if something is terribly wrong :)

Tanvi, would you like to take a quick preliminary look at this? I'm testing this now and will update the patch later for review.

Comment 4

[Johann Hofmann [:johannh]]

Attachment: Without additional UI work

We also need to discuss how it's supposed to look. I attached a screenshot of how it looks with only the minimal changes in platform and browser.js.

To me that actually looks fine, considering this is a pretty uncommon use case and I can't think of a better alternative.

Comment 5

[Tanvi Vyas[:tanvi]]

(In reply to Johann Hofmann [:johannh] from comment #4)
> Created attachment 8745943 [details]
> Without additional UI work
> 
> We also need to discuss how it's supposed to look. I attached a screenshot
> of how it looks with only the minimal changes in platform and browser.js.
> 
Why is some text grey and some black?  Also the https is underlined instead of struck through.

Comment 6

[Tanvi Vyas[:tanvi]]

https://reviewboard.mozilla.org/r/49171/#review47103

::: browser/themes/shared/identity-block/identity-block.inc.css:171
(Diff revision 1)
>  #urlbar[pageproxystate="valid"] > #identity-box.verifiedIdentity > #connection-icon {
>    list-style-image: url(chrome://browser/skin/identity-secure.svg);
>    visibility: visible;
>  }
>  
> -#urlbar[pageproxystate="valid"] > #identity-box.insecureLoginForms > #connection-icon,
> +#urlbar[pageproxystate="valid"] > #identity-box.mixedActiveBlocked > #connection-icon {

Why are you reordering these?  Other than the reorder, is there any other change here?

Comment 7

[Tanvi Vyas[:tanvi]]

https://reviewboard.mozilla.org/r/49171/#review47105

::: dom/security/nsMixedContentBlocker.cpp:832
(Diff revision 1)
>        }
>        rootDoc->SetHasMixedDisplayContentLoaded(true);
>  
>        if (rootHasSecureConnection) {
> -        if (rootDoc->GetHasMixedActiveContentLoaded()) {
> +        // reset state security flag
> +        state = state >> 4 << 4;

I'm not sure we always want to change state this way.  For example, see line 844.  In that case, we are on an http page so state is not broken (since that state is reserved for https pages).

I wonder if a run through try would have caught this.  Can you run "./mach test mcb" locally with this patch applied.

You only want to do this bit flipping of states when we had initially not used the "state" variable at all and overridden its value.

Comment 8

[Tanvi Vyas[:tanvi]]

https://reviewboard.mozilla.org/r/49171/#review47923

::: browser/base/content/browser.js:6744
(Diff revision 1)
>  
>      if (this._isSecureInternalUI) {
>        this._identityBox.className = "chromeUI";
>        let brandBundle = document.getElementById("bundle_brand");
>        icon_label = brandBundle.getString("brandShorterName");
>      } else if (this._uriHasHost && this._isEV) {

Do we need an "if (!this._isCertUserOverridden) {" in this block too?

::: browser/base/content/browser.js:6752
(Diff revision 1)
>          this._identityBox.classList.add("mixedActiveBlocked");
>        }
>  
>        // If it's identified, then we can populate the dialog with credentials
>        let iData = this.getIdentityData();
>        tooltip = gNavigatorBundle.getFormattedString("identity.identified.verifier",

It would probably go around this tooltip.

Comment 9

[Tanvi Vyas[:tanvi]]

Comment on attachment 8745921 [details]
Bug 1253771 - Add previous state info to mixed content callback.

(In reply to Tanvi Vyas [:tanvi] from comment #7)
> https://reviewboard.mozilla.org/r/49171/#review47105
> 
> ::: dom/security/nsMixedContentBlocker.cpp:832
> (Diff revision 1)
> >        }
> >        rootDoc->SetHasMixedDisplayContentLoaded(true);
> >  
> >        if (rootHasSecureConnection) {
> > -        if (rootDoc->GetHasMixedActiveContentLoaded()) {
> > +        // reset state security flag
> > +        state = state >> 4 << 4;
> 
> I'm not sure we always want to change state this way.  For example, see line
> 844.  In that case, we are on an http page so state is not broken (since
> that state is reserved for https pages).
> 
> I wonder if a run through try would have caught this.  Can you run "./mach
> test mcb" locally with this patch applied.
> 
> You only want to do this bit flipping of states when we had initially not
> used the "state" variable at all and overridden its value.

Actually taking a second look at this, you do only override it when rootHasSecureConnection.  So it is probably okay.

feedback+ for now, but I'd like to see another version and responses to my comments.  Thanks Johann!

Comment 10

[Johann Hofmann [:johannh]]

Comment on attachment 8745921 [details]
Bug 1253771 - Add previous state info to mixed content callback.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49171/diff/1-2/

Comment 11

[Johann Hofmann [:johannh]]

New patch with tests and your comments fixed. The tests actually exposed two more functions that were also overriding the state and I patched those up as well.

I refactored another test file (browser_addCertException.js) because it shared basically all its helper functions with my test and put those in head.js

Comment 12

[Johann Hofmann [:johannh]]

Ash, do you have time to take a look at the attachment "Without additional UI work" (https://bug1253771.bmoattachments.org/attachment.cgi?id=8745943) and give your opinion on whether that looks okay (given that it's an edge case)?

Thanks you! :)

Comment 13

[Johann Hofmann [:johannh]]

> Why is some text grey and some black?  Also the https is underlined instead of struck through.

The grey and black seems to be a design inconsistency. https being underlined seems to be correct, although I don't like it either. What's that supposed to say?

> Why are you reordering these?  Other than the reorder, is there any other change here?

Reordering so that the warning icon from an overriden certificate does not override the higher severity icon from mixed active content. No other change.

Comment 14

[Ash Grigas]

(In reply to Johann Hofmann [:johannh] from comment #12)
> Ash, do you have time to take a look at the attachment "Without additional
> UI work" (https://bug1253771.bmoattachments.org/attachment.cgi?id=8745943)
> and give your opinion on whether that looks okay (given that it's an edge
> case)?
> 
> Thanks you! :)

Why do we need two buttons? Can't we just say enable protection like it is here?
https://www.dropbox.com/s/19kmn4c7s9tgyci/Screenshot%202016-05-25%2010.57.27.png?dl=0

Comment 15

[Johann Hofmann [:johannh]]

(In reply to agrigas from comment #14)
> 
> Why do we need two buttons? Can't we just say enable protection like it is
> here?
> https://www.dropbox.com/s/19kmn4c7s9tgyci/Screenshot%202016-05-25%2010.57.27.
> png?dl=0

These are two different actions. This screen is shown when the user has mixed active content AND an overridden certificate, and it wouldn't make sense to have one button to trigger both.

Comment 16

[Ash Grigas]

(In reply to Johann Hofmann [:johannh] from comment #15)
> (In reply to agrigas from comment #14)
> > 
> > Why do we need two buttons? Can't we just say enable protection like it is
> > here?
> > https://www.dropbox.com/s/19kmn4c7s9tgyci/Screenshot%202016-05-25%2010.57.27.
> > png?dl=0
> 
> These are two different actions. This screen is shown when the user has
> mixed active content AND an overridden certificate, and it wouldn't make
> sense to have one button to trigger both.

Ok - all text under the horizontal rule should be gray. Then I guess its good to go. The phrasing 'remove exception' sounds a bit odd to me since its a double negative but can't think of a better way to phrase it...

Comment 17

[Ash Grigas]

(In reply to agrigas from comment #16)
> (In reply to Johann Hofmann [:johannh] from comment #15)
> > (In reply to agrigas from comment #14)
> > > 
> > > Why do we need two buttons? Can't we just say enable protection like it is
> > > here?
> > > https://www.dropbox.com/s/19kmn4c7s9tgyci/Screenshot%202016-05-25%2010.57.27.
> > > png?dl=0
> > 
> > These are two different actions. This screen is shown when the user has
> > mixed active content AND an overridden certificate, and it wouldn't make
> > sense to have one button to trigger both.
> 
> Ok - all text under the horizontal rule should be gray. Then I guess its
> good to go. The phrasing 'remove exception' sounds a bit odd to me since its
> a double negative but can't think of a better way to phrase it...

Also the https shouldnt have a line under it...

Comment 18

[Johann Hofmann [:johannh]]

(In reply to agrigas from comment #17)
> Also the https shouldnt have a line under it...

When I visit a site like https://mixed-script.badssl.com/ and turn off protection then it has that line on my Aurora/DevEdition and Nightly, but not on stable Firefox. Is that a bug? We should really fix it then.

Comment 19

[Ash Grigas]

(In reply to Johann Hofmann [:johannh] from comment #18)
> (In reply to agrigas from comment #17)
> > Also the https shouldnt have a line under it...
> 
> When I visit a site like https://mixed-script.badssl.com/ and turn off
> protection then it has that line on my Aurora/DevEdition and Nightly, but
> not on stable Firefox. Is that a bug? We should really fix it then.

i think that is a bug - that was never part of the design. I'm flagging tanvi who worked on the changes with me to weigh in...

Comment 20

[Johann Hofmann [:johannh]]

Tanvi: after a quick chat on IRC we agreed that it might be best to leave the style as it is now. So this would be ready for review :)

Comment 21

[Tanvi Vyas[:tanvi]]

(In reply to Johann Hofmann [:johannh] from comment #18)
> (In reply to agrigas from comment #17)
> > Also the https shouldnt have a line under it...
> 
> When I visit a site like https://mixed-script.badssl.com/ and turn off
> protection then it has that line on my Aurora/DevEdition and Nightly, but
> not on stable Firefox. Is that a bug? We should really fix it then.

That seems like a bad regression!  We do not want to underline https when protection is disabled!  It is supposed to be struck through.  On Firefox 46 release, I see a struck through https in the url bar when we disable protection.  But on Nightly, I see an underlined https in the url bar.  That is no good!

https://people.mozilla.com/~tvyas/mixedcontent.html

Comment 22

[Tanvi Vyas[:tanvi]]

(In reply to Johann Hofmann [:johannh] from comment #20)
> Tanvi: after a quick chat on IRC we agreed that it might be best to leave
> the style as it is now. So this would be ready for review :)

What do you mean?  Shouldn't we flip the order and have the mixed content text first?  And shouldn't we fix the text coloring issues?

Comment 23

[Johann Hofmann [:johannh]]

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #22)
> (In reply to Johann Hofmann [:johannh] from comment #20)
> > Tanvi: after a quick chat on IRC we agreed that it might be best to leave
> > the style as it is now. So this would be ready for review :)
> 
> What do you mean?  Shouldn't we flip the order and have the mixed content
> text first?  And shouldn't we fix the text coloring issues?

The certificate info text is always black, the mixed content text is always gray, by design. Adding an exception here seems messier to me than having two text colors on the same page.

What's the point of flipping the order?

Putting the certificate override text to the bottom and making it grey would very much aid in hiding it. I personally think that an overridden certificate is more rare, important and actionable info than mixed content blocking (the mixed content part is also way larger). Maybe I'm wrong about that.

Comment 24

[Tanvi Vyas[:tanvi]]

Spoke to Johann and I will review this later.  If the priority increases, let me know and I will do it sooner than later.  Thanks!

Comment 25

[Tanvi Vyas[:tanvi]]

Comment on attachment 8745921 [details]
Bug 1253771 - Add previous state info to mixed content callback.

https://reviewboard.mozilla.org/r/49171/#review60160

Sorry for the long delay here Johann!  This looks good to me.  I would recommend a second reviewer to take a second look at the /browser code.

::: browser/themes/shared/identity-block/identity-block.inc.css:171
(Diff revision 2)
>  #urlbar[pageproxystate="valid"] > #identity-box.verifiedIdentity > #connection-icon {
>    list-style-image: url(chrome://browser/skin/identity-secure.svg);
>    visibility: visible;
>  }
>  
> -#urlbar[pageproxystate="valid"] > #identity-box.insecureLoginForms > #connection-icon,
> +#urlbar[pageproxystate="valid"] > #identity-box.mixedActiveBlocked > #connection-icon {

Why did you need to reorganize this section?

Comment 26

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #25)
> ::: browser/themes/shared/identity-block/identity-block.inc.css:171
> (Diff revision 2)
> >  #urlbar[pageproxystate="valid"] > #identity-box.verifiedIdentity > #connection-icon {
> >    list-style-image: url(chrome://browser/skin/identity-secure.svg);
> >    visibility: visible;
> >  }
> >  
> > -#urlbar[pageproxystate="valid"] > #identity-box.insecureLoginForms > #connection-icon,
> > +#urlbar[pageproxystate="valid"] > #identity-box.mixedActiveBlocked > #connection-icon {
> 
> Why did you need to reorganize this section?

Ignore this part, it was already answered in comment 13.  Thanks!

Comment 27

[Johann Hofmann [:johannh]]

Comment on attachment 8745921 [details]
Bug 1253771 - Add previous state info to mixed content callback.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49171/diff/2-3/

Comment 28

[Johann Hofmann [:johannh]]

Florian, whenever you have the time, could you take a quick look at the browser code? Thanks!

Comment 29

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=33797a678ed7

Comment 30

[Florian Quèze [:florian]]

Comment on attachment 8745921 [details]
Bug 1253771 - Add previous state info to mixed content callback.

https://reviewboard.mozilla.org/r/49171/#review60312

r=me with the following trivial comments addressed.

::: browser/base/content/test/general/browser_mixed_content_cert_override.js:23
(Diff revision 3)
> +  return window.getComputedStyle(document.getElementById("connection-icon")).listStyleImage;
> +}
> +
> +function checkIdentityPopup(icon) {
> +  gIdentityHandler.refreshIdentityPopup();
> +  is(getConnectionIcon(), icon);

Looks like this should test for 'url("' + icon + '"), or maybe even include the chrome://browser/skin/ and the .svg parts.

::: browser/base/content/test/general/browser_mixed_content_cert_override.js:36
(Diff revision 3)
> +  // check that a warning is shown when loading a page with mixed content and an overridden certificate
> +  yield loadBadCertPage(MIXED_CONTENT_URL);
> +  checkIdentityPopup('url("chrome://browser/skin/identity-mixed-passive-loaded.svg")');
> +
> +  // check that the crossed out icon is shown when disabling mixed content protection
> +  gIdentityHandler.disableMixedContentProtection()

nit: missing ;

::: browser/base/content/test/general/head.js:1105
(Diff revision 3)
>      }, false, true);
>    });
>  }
> +
> +function* loadBadCertPage(url) {
> +  let exceptionDialogURI = "chrome://pippki/content/exceptionDialog.xul";

const. This was actually a constant in the code before you moved it.

::: browser/base/content/test/general/head.js:1106
(Diff revision 3)
>    });
>  }
> +
> +function* loadBadCertPage(url) {
> +  let exceptionDialogURI = "chrome://pippki/content/exceptionDialog.xul";
> +  let exceptionDialogResolved = new Promise(function(resolve){

nit: missing space between ) and {

::: browser/base/content/test/general/head.js:1116
(Diff revision 3)
> +        if (aTopic == "cert-exception-ui-ready") {
> +          Services.obs.removeObserver(this, "cert-exception-ui-ready");
> +          let certExceptionDialog = getCertExceptionDialog(exceptionDialogURI);
> +          ok(certExceptionDialog, "found exception dialog");
> +          executeSoon(function() {
> +            resolve();

Why is resolve() before the click on the button?

::: browser/base/content/test/general/head.js:1141
(Diff revision 3)
> +// Utility function to get a handle on the certificate exception dialog.
> +// Modified from toolkit/components/passwordmgr/test/prompt_common.js
> +function getCertExceptionDialog(aLocation) {
> +  let wm = Cc["@mozilla.org/appshell/window-mediator;1"]
> +             .getService(Ci.nsIWindowMediator);
> +  let enumerator = wm.getXULWindowEnumerator(null);

Services.wm

Comment 31

[Johann Hofmann [:johannh]]

Comment on attachment 8745921 [details]
Bug 1253771 - Add previous state info to mixed content callback.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49171/diff/3-4/

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/d13afbb70d7b
Add previous state info to mixed content callback. r=tanvi r=florian

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d13afbb70d7b

Comment 34

[Paul Silaghi, QA [:pauly]]

"You have added a security exception for this site" text and the "Remove exception" button displayed.
Verified fixed FX 50.0a1 (2016-07-12) Win 7.