834830 - Strike through https in location bar for sites that have mixed active content loaded.

Status: RESOLVED FIXED

(Firefox :: Security, defect)

Description

[Tanvi Vyas[:tanvi]]

In the mixed content blocker design at http://people.mozilla.com/~lco/ProjectSPF/Mixed_Content/Mixed_Content_Spec/, when the user overrides mixed active content blocking (or if the blocking pref is disabled), a triangle icon replaces the globe/lock (bug 822366 does this) and the https appears in the location bar with a strike through.

This bug is to add the strikethrough.

Comment 1

[Johnathan Nightingale [:johnath]]

I'm concerned that a strikethrough of "https" is a pretty jargon-y way to express that there are problems with the site. I've always thought that chrome's approach there was over-technical.

Have we evaluated other ways of expressing that the page load lacks integrity (e.g. disappearing the protocol signifier completely) that might remove the deceptive "https" cue for people who do know to look for it?

I'm not veto-ing the work here, nor am I a designer, but the approach is surprising and I wanted to make sure we've considered and addressed/rejected the concern. It's plausible that we already have, but the linked PDF doesn't seem to anticipate it, and does talk about using https-with-strikethrough as a user signal.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Johnathan Nightingale [:johnath] from comment #1)
> Have we evaluated other ways of expressing that the page load lacks
> integrity (e.g. disappearing the protocol signifier completely) that might
> remove the deceptive "https" cue for people who do know to look for it?

Channeling Limi: people don't notice the lack of an indicator.

One very positive thing about the crossed-out https in Chrome: it makes the "this page is broken because it was trying to do something dangerous" indicator bigger and more noticeable. If we're going to break pages with mixed content blocker, I'd rather have a bigger finger to point at the website to lesson the likelihood that the user will think Firefox just doesn't work.

But, I am concerned about one aspect of the usability of Chrome's implementation of this. Normally, when we show an indicator, the user can click on the indicator to get more information or do something about the indication. But, clicking on the crossed-out "https" in Chrome doesn't do anything except select the text in the address bar.

One thing to keep in mind: If a MitM can inject his own script, then he can effectively undo all the encryption of the page. For example, he could use XHR to send the whole page--including password and credit card details in forms--to another site. I think that any kind of mixed content that can do that is deserving of more severe security indicators than we would use for, e.g. passive mixed content.

Comment 3

[Johnathan Nightingale [:johnath]]

(In reply to Brian Smith (:bsmith) from comment #2)
> (In reply to Johnathan Nightingale [:johnath] from comment #1)
> > Have we evaluated other ways of expressing that the page load lacks
> > integrity (e.g. disappearing the protocol signifier completely) that might
> > remove the deceptive "https" cue for people who do know to look for it?
> 
> Channeling Limi: people don't notice the lack of an indicator.

I think in general that is a deeply true statement, and I don't object at all to an indicator of broken SSL. I think the specific one under discussion here, the one chrome uses, is jargon-heavy and not something for general consumption. I know that you and Larissa and others know that, though, so I infer that we're doing this explicitly for the subset of our users who know, or have been trained, to look for the https protocol string as a signal of secure-ness. For that group, the strikethrough makes more sense, but I think that group in particular has been trained to look for a thing, and so taking that thing away (or using some other indicator of brokenness) ought to address the problem, too.

> One very positive thing about the crossed-out https in Chrome: it makes the
> "this page is broken because it was trying to do something dangerous"
> indicator bigger and more noticeable. If we're going to break pages with
> mixed content blocker, I'd rather have a bigger finger to point at the
> website to lesson the likelihood that the user will think Firefox just
> doesn't work.

I completely agree that insecure-but-should-be-secure pages need a problem indicator, but I don't think "<strike>https</strike>" says "insecure" to most humans any more than "https" says "secure." Hence our turbulent love affair with the padlock.

> But, I am concerned about one aspect of the usability of Chrome's
> implementation of this. Normally, when we show an indicator, the user can
> click on the indicator to get more information or do something about the
> indication. But, clicking on the crossed-out "https" in Chrome doesn't do
> anything except select the text in the address bar.

I do like giving people the option to drill in, I agree, and disappearing the indicator would disappear the affordance. I suspect that you and I are both in some insane minority in terms of people who actually DO interact with Larry and security status, but I'm still partial to supporting click-me-to-elaborate.

> One thing to keep in mind: If a MitM can inject his own script, then he can
> effectively undo all the encryption of the page. For example, he could use
> XHR to send the whole page--including password and credit card details in
> forms--to another site. I think that any kind of mixed content that can do
> that is deserving of more severe security indicators than we would use for,
> e.g. passive mixed content.

For sure, though drawing the line might be tricky (a rogue CSS file can do surprising things).

Comment 4

[Larissa Co [:lco]]

Initially, dolske, shorlander and I had discussed treating the site like any non-SSL site (i.e. showing a globe and no http label), but I realized that the reason we wanted to show a warning icon + crossed out https was to remind the user that they weren't protected. 

What about taking Johnath's suggestion of removing the signifier (the https) but retaining the warning icon (yellow triangle)? This might eliminate both concerns of being too technical and being misleadingly secure. The warning icon will also provide a place for people looking for more information to view the Larry menu

Comment 5

[Jesse Ruderman]

Please don't hide "https" when security is broken.  That breaks copy-paste, confuses bug diagnosis, and makes the problem look less severe than it is (accidentally loading an HTTP page is recoverable; loading a compromised HTTPS page allows various persistent attacks).

We could cross out "https".  We might want additional UI for less technical users, but that's not this bug.

Comment 6

[Larissa Co [:lco]]

Just wanted to emphasize that the state where mixed active content is loaded is NOT the default state. By default, when the user enters a site with mixed active content, we block the script and show the lock icon + https because the user is currently protected.

It's only when the user has voluntarily chosen the option in the doorhanger to allow mixed scripts that we display the warning indicators that are being discussed in this bug.

My point is that in the default state, we protect the user. Here, we are discussing a state that is visible presumably in only a small number of cases to hopefully more technical users. 

Jesse, I don't know if that addresses your concerns about bug diagnosis and making the problem look less severe? I'm not sure I understand what you mean by breaking copy-paste (wouldn't that happen if the https was crossed-out anyway?)

Comment 7

[Jesse Ruderman]

My copy-paste concern: when a user tries to share the URL of an https page, the browser shouldn't silently hack off the "https", causing the recipients to load an http page.

Crossing "https" out wouldn't break copy-paste, but hiding it would break it in at least some cases.

Comment 8

[Jesse Ruderman]

> Here, we are discussing a state that is visible presumably in only a small number  
> of cases to hopefully more technical users. 

I'm not sure about that.  Anyone who installed the Pocket bookmarklet before Nov 2012 will get a mixed content warning when trying to save an article from an https site.  More technical users are more likely to update or edit their bookmarklet rather than click through the warning.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Jesse Ruderman from comment #7)
> My copy-paste concern: when a user tries to share the URL of an https page,
> the browser shouldn't silently hack off the "https", causing the recipients
> to load an http page.

I agree that muddying the distinction between https and http in the address bar has a lot of significant side effects.

Also, I think we're overlooking an important (but too subtle) cue from Chrome's UI: When they cross out the "https" in red, they also have a broken lock with a small *red* shield in it next to it. The color helps associate the lock with the crossed-out https. It wouldn't make sense to have a *yellow* triangle and a *red* strike-through on the "https", especially since the struck-through "https" is not clickable on its own to get help on WTF is going on.

Would it make it easier to decide what to do here if we had the telemetry first? One thing to keep in mind is that IE and Chrome have helped to eradicate a lot of mixed active content from the internet already in the last couple of years. That may mean that it is more reasonable to have a louder UI for this then we would otherwise be comfortable with, if very few people will ever see it.

Comment 10

[Justin Dolske [:Dolske]]

I'm not sure that "hiding 'https' on broken sites breaks cut'n'paste" is necessarily a blocker -- we're getting into edge case territory. (We could also hide the display, but add it back when the field is focused/editable, it makes things jump around but as I said we're in edge-case territory).

Comment 11

[Johnathan Nightingale [:johnath]]

(In reply to Larissa Co [:lco] from comment #6)
> Just wanted to emphasize that the state where mixed active content is loaded
> is NOT the default state. By default, when the user enters a site with mixed
> active content, we block the script and show the lock icon + https because
> the user is currently protected.
> 
> It's only when the user has voluntarily chosen the option in the doorhanger
> to allow mixed scripts that we display the warning indicators that are being
> discussed in this bug.
> 
> My point is that in the default state, we protect the user. Here, we are
> discussing a state that is visible presumably in only a small number of
> cases to hopefully more technical users. 

This is a good point that I'd overlooked. Concerns about a jargon-heavy indicator matter less if we expect our users to be privy to the jargon. I yield the floor to UX on whether we think "users who enable active mixed content" and "users who understand SSL" overlap sufficiently so as to make the jargon concern moot.

Comment 12

[Frederik Braun [:freddy]]

We're going for the shield now, so I suppose this can be resolved?

Comment 13

[Tanvi Vyas[:tanvi]]

The sh(In reply to Frederik Braun [:freddyb] from comment #12)
> We're going for the shield now, so I suppose this can be resolved?

The shield appears when Mixed Active Content is blocked.  When the user "Disables Protection" the shield disappears and a yellow triangle appears instead of the lock icon with "https" right next to it:
https://people.mozilla.com/~tvyas/FigureD.jpg

Originally, the design included the yellow triangle with a strikethrough on the "https".  This bug is to implement the strikethrough.  I don't have the bandwidth or expertise to do this right now, so if anyone else does, please feel free to take the bug ;)  Thanks!

Comment 14

[sjw]

I would take it if someone could show me how to write a patch... I think this is a really easy one.

Comment 15

[Tanvi Vyas[:tanvi]]

(In reply to sjw from comment #14)
> I would take it if someone could show me how to write a patch... I think
> this is a really easy one.

From talking to Jared (jaws@), this might be more complicated than it seems.

What do you mean by show you how to write a patch?

Take a look at https://developer.mozilla.org/en-US/docs/Introduction , if you haven't already.

Thanks!

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Yeah, I don't see how this can be accomplished since we use a ::selection to apply the background and color of the text.

Dao, do you have any ideas here?

Comment 17

[sjw]

(In reply to Tanvi Vyas [:tanvi] from comment #15)
> What do you mean by show you how to write a patch?
> 
> Take a look at https://developer.mozilla.org/en-US/docs/Introduction , if
> you haven't already.
> 
> Thanks!

I know this page, thanks.
My English skills are bad, so the best way for me is learning by doing. I know how to develop something, but I've no idea how the System here works.

Comment 18

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #16)
> Dao, do you have any ideas here?

Nope, and I think we should just hide https:// here.

Comment 19

[sjw]

The current UI struck through the shield symbol.

Comment 20

[Tanvi Vyas[:tanvi]]

The shield is now struck through when we disable protection.  Would be nice to also strike through the https but it might not be worth the implementation headache to do that.  Jared says it would be pretty tough.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-834830-Add-nsISelectionController.SELECTION_URLS.patch

Since bug 451833 landed selection types still aren't easily configurable, probably because it has never been a priority. So let's add another one :)

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-834830-Strike-through-https-in-location-bar-for-.patch

Use the new selection type and strike out the "https" part of a URL if mixed active content is loaded.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Screenshot

Aislinn, what do you think? I made the line twice as thick as it would usually be because it's otherwise relatively hard to spot with a grey font.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8630345 [details] [diff] [review]
0001-Bug-834830-Add-nsISelectionController.SELECTION_URLS.patch

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

Bleh.

Comment 25

[Dão Gottwald [:dao]]

Comment on attachment 8630346 [details] [diff] [review]
0002-Bug-834830-Strike-through-https-in-location-bar-for-.patch

>+          let isMixedActiveContentLoaded = gBrowser.securityUI.state &
>+            Ci.nsIWebProgressListener.STATE_LOADED_MIXED_ACTIVE_CONTENT;
>+
>+          // Strike out the "https" part if mixed active content is loaded.
>+          if (value.startsWith("https:") && isMixedActiveContentLoaded) {

The way you wrote it, both value.startsWith("https:") and gBrowser.securityUI.state & Ci.nsIWebProgressListener.STATE_LOADED_MIXED_ACTIVE_CONTENT are executed all the time. The cheaper check should come first and the other one should only happen when needed.

It also looks like you'll need to make sure the URL actually represents the current page, e.g. by checking pageproxystate.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-834830-Strike-through-https-in-location-bar-for-.patch, v2

Added a focus handler that calls formatValue() to clear the current URLSTRIKEOUT selection so that when focusing the url bar it's not striked out. Normal selection does override font colors but not text decoration unfortunately, seems not too easy to solve on the platform side.

The condition now also checks the "pageproxystate", the securityUI state is checked lazily.

Comment 27

[Dave Garrett]

Comment on attachment 8630440 [details] [diff] [review]
0002-Bug-834830-Strike-through-https-in-location-bar-for-.patch, v2

>From ff3a03b337c5311d218ba8334aab662a67dc5812 Mon Sep 17 00:00:00 2001
>+    // Make sure the "https" part of the URL is striked out our not,
>+    // depending on the current mixed active content blocking state.
>+    gURLBar.formatValue();
>+

"our" -> "or"

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dave Garrett from comment #27)
> >+    // Make sure the "https" part of the URL is striked out our not,
> 
> "our" -> "or"

Fixed locally, thanks!

Comment 29

[Ash Grigas]

(In reply to Tim Taubert [:ttaubert] from comment #23)
> Created attachment 8630347 [details]
> Screenshot
> 
> Aislinn, what do you think? I made the line twice as thick as it would
> usually be because it's otherwise relatively hard to spot with a grey font.

That looks good to me!

Comment 30

[Dão Gottwald [:dao]]

Comment on attachment 8630440 [details] [diff] [review]
0002-Bug-834830-Strike-through-https-in-location-bar-for-.patch, v2

>+      <handler event="focus"><![CDATA[
>+        this.formatValue();
>+      ]]></handler>
>+
>       <handler event="blur"><![CDATA[
>         this._clearNoActions();
>         this.formatValue();
>       ]]></handler>

You need to make sure you're getting the focus event for the input field rather than one of the other textbox children such as the identity block. This is actually already broken in the blur handler.

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-834830-Strike-through-https-in-location-bar-for-.patch, v3

Comment 32

[Dão Gottwald [:dao]]

Comment on attachment 8630951 [details] [diff] [review]
0002-Bug-834830-Strike-through-https-in-location-bar-for-.patch, v3

>+      <handler event="focus"><![CDATA[
>+        if (event.target == this) {
>+          this.formatValue();
>+        }
>+      ]]></handler>
>+
>       <handler event="blur"><![CDATA[
>-        this._clearNoActions();
>-        this.formatValue();
>+        if (event.target == this) {
>+          this._clearNoActions();
>+          this.formatValue();
>+        }
>       ]]></handler>

This helps for non-anonymous children but can't distinguish between anonymous ones. Please check event.originalTarget == this.inputField instead. r=me with that

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #32)
> This helps for non-anonymous children but can't distinguish between
> anonymous ones. Please check event.originalTarget == this.inputField
> instead. r=me with that

Will do, thanks!

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/12536dbd1274
https://hg.mozilla.org/integration/fx-team/rev/53a5746425f9

Comment 35

[Tim Taubert [:ttaubert] (inactive)]

Backed out due to test failures:

https://hg.mozilla.org/integration/fx-team/rev/741bb6d3448b
https://hg.mozilla.org/integration/fx-team/rev/35010b71836c

Turns out that since .formatValue() is called more often the "this.editor is null" error that has been around for a while now breaks things. We'll need to fix bug 950661 at the same time.

Comment 36

[Tim Taubert [:ttaubert] (inactive)]

Attached a patch to bug 950661.

Comment 37

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c1b7e593fe66
https://hg.mozilla.org/integration/fx-team/rev/02a87dc3c5fb

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c1b7e593fe66
https://hg.mozilla.org/mozilla-central/rev/02a87dc3c5fb

Comment 40

[Tanvi Vyas[:tanvi]]

Nice job Tim!