1194561 - Change globe to broken lock for insecure resources

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect)

Description

[Richard Barnes [:rbarnes]]

Currently the network monitor shows the globe icon for nonsecure resources that are loaded.  It should instead show the broken lock icon that we currently show for pages with un-blocked active mixed content -- clearly a page with no security is no better than a secure one that includes active mixed content.

This does cause inconsistency between the icon in the URL bar and the icon in the network monitor, but I don't think this is a problem.  The only reason we don't show the broken lock in the URL bar is that it would appear to often and desensitize users.  We shouldn't worry about the corresponding phenomenon with developers -- we want to be in their faces about being insecure, since they can fix it.

Comment 1

[Richard Barnes [:rbarnes]]

Attachment: Appearance before patch (globe icon)

Comment 2

[Richard Barnes [:rbarnes]]

Attachment: Appearance after patch (broken lock icon)

Comment 3

[Richard Barnes [:rbarnes]]

Attachment: Appearance before patch (globe icon)

Comment 4

[Richard Barnes [:rbarnes]]

Attachment: bug-1194561.0.patch

This patch just changes the icon link for insecure content to point to the broken lock instead of the globe.

Comment 5

[Sami Jaktholm]

That won't work as the same icon is shown for broken HTTPS connections. See http://i.imgur.com/wasHvjW.png or live at https://badssl.com/.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8647850 [details] [diff] [review]
bug-1194561.0.patch

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

I was not fully aware that we already used this broken link icon for another purpose in the net monitor when I first replied to the list.

Let's discuss this further on the list (I just replied there myself).

Comment 7

[April King [:April]]

It should be noted that the network monitor also only shows a globe for resources that generate a mixed content warning in the URL bar.  It would be nice if they could be tied together.

Anyways, I'm relatively fine with the patch, and I think developers probably could use a kick in the pants.  Perhaps we could simply start with using the sad gray triangle for http/mixed, the broken lock for broken https, and the green lock for working https?  That way we don't overload the broken https lock?

Either way, this bug/patch gets my delicious-low-hanging-fruit vote.

Comment 8

[Richard Barnes [:rbarnes]]

Attachment: before.jpg

Comment 9

[Richard Barnes [:rbarnes]]

Attachment: after.jpg

Comment 10

[Richard Barnes [:rbarnes]]

Attachment: bug-1194561.1.patch

I think this patch addresses the comments raised on the mailing list.  The different error cases are clearly separated.  See the attached before and after pictures for an illustration of the clearer iconography.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8649062 [details] [diff] [review]
bug-1194561.1.patch

Jeff, what do you think about this revised version?  See the before / after images in the bug to compare.

I am tempted to accept this, as we're not losing data by merging states and it seems like an overall improvement to me.

On the whole, I think this discussion has highlighted that we really need:

* Text tooltips to precisely explain the meaning of each icon
* UX should spend some time thinking deeply about the specific icons we should use

But, I think those could be handled as follow ups to this.

The one unresolved issue I see from our IRC discussion is the treatment of localhost URLs.  This current version gives HTTP to localhost a broken lock.  Do you consider this issue a blocker to making a change here, after reviewing the comparison images?

Comment 12

[Richard Barnes [:rbarnes]]

On localhost: FWIW, I looked at whether it would be easy to give localhost URLs different treatment, and it didn't seem to be the case.  The securityState element is updated independently of the domain/url elements, so its not easy to see what domain you're talking about when you choose which security state to display.

Given that it's a bit of a corner case and will require some refactoring, I would suggest punting to a follow-up.

Comment 13

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #11)
> Comment on attachment 8649062 [details] [diff] [review]
> bug-1194561.1.patch
> 
> Jeff, what do you think about this revised version?  See the before / after
> images in the bug to compare.

Sure, the before/after look much more varied. I'm not keen on the icons used, logged bug 1195839 as a followup.

Comment 14

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Richard Barnes [:rbarnes] from comment #12)
> On localhost: FWIW, I looked at whether it would be easy to give localhost
> URLs different treatment, and it didn't seem to be the case.  The
> securityState element is updated independently of the domain/url elements,
> so its not easy to see what domain you're talking about when you choose
> which security state to display.

I would want the different treatment for any connection made to 127.0.0.1, or also possibly connections made to the system's own LAN ip address?

> Given that it's a bit of a corner case and will require some refactoring, I
> would suggest punting to a follow-up.

I agree.

Comment 15

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Richard Barnes [:rbarnes] from comment #12)
...
> Given that it's a bit of a corner case and will require some refactoring, I
> would suggest punting to a follow-up.

To be clear - do you expect to work on the followup, and land it in the current cycle?

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Richard Barnes [:rbarnes] from comment #12)
> On localhost: FWIW, I looked at whether it would be easy to give localhost
> URLs different treatment, and it didn't seem to be the case.  The
> securityState element is updated independently of the domain/url elements,
> so its not easy to see what domain you're talking about when you choose
> which security state to display.

Even though securityState is listed separately in this switch statement [0], I think that we could get domain/url somehow from requestItem.attachment after that loop finishes.

[0]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/netmonitor-view.js#1429

Comment 17

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Comment on attachment 8649062 [details] [diff] [review]
bug-1194561.1.patch

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

This seems fine as long as someone is going to work on the localhost case, see my needinfo for Richard.

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Waiting on a reply to Jeff's ni? before proceeding with review.

Comment 19

[Richard Barnes [:rbarnes]]

If the localhost stuff is doable, it makes sense to do it here.  I'll take the suggestion from :bgrins and see what I can do.  So hold off on review for the moment.

Comment 20

[Brian Grinstead [:bgrins]]

To be more specific about this, I think from updateMenuView [0] we could access aItem.attachment.remoteAddress when securityState is being updated.  Or is it the domain name / URL that we care about here?


[0]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/netmonitor-view.js#1595

Comment 21

[Richard Barnes [:rbarnes]]

Attachment: bug-1194561.2.patch

This patch now marks all requests to localhost, 127.0.0.0/8, and ::1/128 as secure.  Note that the check is based on the name in the URL, not the resolved IP address.  The latter is dangerous due to DNS risks, so we can't mark it as secure.  It will get a marking based on its actual security state.

I'm honestly not super happy re-using the secure icon/tooltip for localhost, but I can live with it.  If we can come up with a better answer, it should be easy to change given the way local stuff is split out here.  (It will require l10n, however.)

Comment 22

[Richard Barnes [:rbarnes]]

Attachment: before.png

Comment 23

[Richard Barnes [:rbarnes]]

Attachment: after.png

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to Richard Barnes [:rbarnes] from comment #21)
> I'm honestly not super happy re-using the secure icon/tooltip for localhost,
> but I can live with it.  If we can come up with a better answer, it should
> be easy to change given the way local stuff is split out here.  (It will
> require l10n, however.)

Maybe we could stick with the globe icon for now for localhost?

Comment 25

[Tim Nguyen :ntim]

(In reply to Richard Barnes [:rbarnes] from comment #21)
> Created attachment 8650095 [details] [diff] [review]
> bug-1194561.2.patch
> 
> This patch now marks all requests to localhost, 127.0.0.0/8, and ::1/128 as
> secure.  Note that the check is based on the name in the URL, not the
> resolved IP address.  The latter is dangerous due to DNS risks, so we can't
> mark it as secure.  It will get a marking based on its actual security state.
What about `file://` URIs ?

> I'm honestly not super happy re-using the secure icon/tooltip for localhost,
> but I can live with it.  If we can come up with a better answer, it should
> be easy to change given the way local stuff is split out here.  (It will
> require l10n, however.)

I would use something generic for localhost. If the dev sees a green lock icon for all of requests, he will think everything is alright. Although, we shouldn't send that message for localhost, as the dev may deploy his app on non-secure http later. So we should simply show a generic icon (globe) in my opinion.

Comment 26

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] (mostly away until 26 August) from comment #25)
> (In reply to Richard Barnes [:rbarnes] from comment #21)
> > Created attachment 8650095 [details] [diff] [review]
> > bug-1194561.2.patch
> > 
> > This patch now marks all requests to localhost, 127.0.0.0/8, and ::1/128 as
> > secure.  Note that the check is based on the name in the URL, not the
> > resolved IP address.  The latter is dangerous due to DNS risks, so we can't
> > mark it as secure.  It will get a marking based on its actual security state.
> What about `file://` URIs ?

I believe file:// URIs aren't shown (see Bug 1193811)

Comment 27

[Richard Barnes [:rbarnes]]

(In reply to Brian Grinstead [:bgrins] from comment #26)
> I believe file:// URIs aren't shown (see Bug 1193811)

Confirmed.  It's the *network* panel, after all :)


(In reply to Tim Nguyen [:ntim] (mostly away until 26 August) from comment #25)
> I would use something generic for localhost. If the dev sees a green lock
> icon for all of requests, he will think everything is alright. Although, we
> shouldn't send that message for localhost, as the dev may deploy his app on
> non-secure http later. So we should simply show a generic icon (globe) in my
> opinion.

I had thought about the question of whether there was a problem of consistency between localhost testing and eventual deployment.  I still think the lock icon is the right answer if we're going to do anything special for localhost. 

If we really want things to look the same on localhost vs. deployment, we should just remove the special-casing for localhost altogether.  If we're going to special-case localhost, it should reflect the security status we actually assign the request, which, by spec, should be the same as HTTPS -- thus the lock.

In brief, the globe is never the answer.  It is content-free.

Comment 28

[April King [:April]]

I have the perfect solution for localhost files, and one that will require zero additional UI resources: let's just use the home icon for it.  :)

Comment 29

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8650095 [details] [diff] [review]
bug-1194561.2.patch

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

I believe you'll need to update some tests, for example browser/devtools/netmonitor/test/browser_net_security-state.js at least.

Thanks for working on the localhost case!  Like Brian and Tim, I am a bit wary of using the green lock for localhost when HTTP is used, mainly because of web platform features like service workers that don't currently work over HTTP today, so the green lock could surprise developers in that case.  But perhaps the intent of the localhost exception in powerful features is that platform features like this *should* work on localhost over HTTP?  If that's the intent, then the green lock seems more reasonable to me.

In any case, we'll hopefully review the icons again in bug 1195839.

::: browser/themes/shared/devtools/netmonitor.inc.css
@@ +144,5 @@
>    -moz-margin-end: 4px;
>  }
>  
>  .security-state-insecure {
> +  list-style-image: url(chrome://browser/skin/identity-mixed-active-loaded.svg);

(Maybe this should have cursor: pointer too?)

Comment 30

[Brian Grinstead [:bgrins]]

(In reply to Richard Barnes [:rbarnes] from comment #27)
> I had thought about the question of whether there was a problem of
> consistency between localhost testing and eventual deployment.  I still
> think the lock icon is the right answer if we're going to do anything
> special for localhost. 
> 
> If we really want things to look the same on localhost vs. deployment, we
> should just remove the special-casing for localhost altogether.  If we're
> going to special-case localhost, it should reflect the security status we
> actually assign the request, which, by spec, should be the same as HTTPS --
> thus the lock.

If I understand correctly from https://groups.google.com/d/msg/mozilla.dev.developer-tools/y3MqhAGLMjg/RXEh3fX0BwAJ, localhost is considered "potentially trustworthy" (https://w3c.github.io/webappsec/specs/powerfulfeatures/#is-origin-trustworthy), which seems distinct from HTTPs by spec.

> In brief, the globe is never the answer.  It is content-free.

At least it matches what's visible in the URL bar (for now) and doesn't require coming up with a new icon.  If (when) we move away from the globe in the URL bar, we could adjust to match whatever is used there for localhost requests. The home icon could also work.

Comment 31

[Richard Barnes [:rbarnes]]

Attachment: bug-1194561.3.patch

Comment 32

[Richard Barnes [:rbarnes]]

Comment on attachment 8651983 [details] [diff] [review]
bug-1194561.3.patch

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #29)
> Comment on attachment 8650095 [details] [diff] [review]
> bug-1194561.2.patch
> 
> Review of attachment 8650095 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I believe you'll need to update some tests, for example
> browser/devtools/netmonitor/test/browser_net_security-state.js at least.

The tests don't look at the icons, so there's no breakage in the current tests.  I added a test for the localhost case.

> Thanks for working on the localhost case!  Like Brian and Tim, I am a bit
> wary of using the green lock for localhost when HTTP is used, mainly because
> of web platform features like service workers that don't currently work over
> HTTP today, so the green lock could surprise developers in that case.  But
> perhaps the intent of the localhost exception in powerful features is that
> platform features like this *should* work on localhost over HTTP?  If that's
> the intent, then the green lock seems more reasonable to me.

In response to this and :bgrins, I went ahead and used the globe.


> ::: browser/themes/shared/devtools/netmonitor.inc.css
> @@ +144,5 @@
> >    -moz-margin-end: 4px;
> >  }
> >  
> >  .security-state-insecure {
> > +  list-style-image: url(chrome://browser/skin/identity-mixed-active-loaded.svg);
> 
> (Maybe this should have cursor: pointer too?)

Done.

Comment 33

[Tim Nguyen :ntim]

Comment on attachment 8651983 [details] [diff] [review]
bug-1194561.3.patch

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

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +1620,5 @@
>          let domain = $(".requests-menu-domain", target);
>          domain.setAttribute("value", hostPort);
>          domain.setAttribute("tooltiptext", hostPort);
> +
> +        // Mark local hosts specially, and treat them as secure, where "local" is

Not sure if the "treat them as secure" still applies here.

::: browser/themes/shared/devtools/netmonitor.css
@@ +144,5 @@
>    -moz-margin-end: 4px;
>  }
>  
>  .security-state-insecure {
> +  cursor: pointer;

Instead of duplicating those cursor: pointer; declarations, can you move them in the rule above ?

Comment 34

[Richard Barnes [:rbarnes]]

Attachment: after.png

Comment 35

[Richard Barnes [:rbarnes]]

Attachment: after.png

Comment 36

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8651983 [details] [diff] [review]
bug-1194561.3.patch

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

Great, this seems good to me!

Please fix up the nits :ntim pointed out, but no need for another review.

Comment 37

[Richard Barnes [:rbarnes]]

Attachment: bug-1194561.4.patch r=jryans

With :ntim nits addressed

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf655c6d3c62

Comment 39

[Tanvi Vyas[:tanvi]]

It might be good to get some documentation on this.

States:

1. HTTPS OK - green lock
2. HTTPS weak - grey lock with yellow warning triangle
3. HTTPS failed - grey warning triangle
4. HTTP OK - struck-through lock
5. localhost - globe

I'm not sure that the distinction between 2 and 3 is necessary.  We don't use the gray warning triangle in the urlbar anymore, so it doesn't match any state we already have.  Moreover, if the red square indicates connection failure, couldn't we just reuse the grey lock with yellow warning triangle.

What is the icon when mixed active content is loaded?  Grey lock with yellow triangle in the net monitor and grey lock with strike through in the url bar?

Comment 40

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Yes, let's document for sure.  Bug 1195839 is already filed to review the icons soonish, so that seems like a good place for further fine tuning.

(In reply to Tanvi Vyas [:tanvi] from comment #39)
> What is the icon when mixed active content is loaded?  Grey lock with yellow
> triangle in the net monitor and grey lock with strike through in the url bar?

Net monitor is only examines each request, so it is not paying attention to the overall possibly "mixed" state of the page.

It's currently the reverse of what you said in my testing: with mixed content (try mixed.badssl.com), URL bar get gray lock with yellow triangle, and then in the net monitor you have a green lock for the page itself, and a struck-through lock for the HTTP image.

I am sure there ways to improve all around.  Please comment in bug 1195839 with icon improvement ideas!

Comment 41

[Ryan VanderMeulen [:RyanVM]]

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

Comment 42

[Will Bamberg [:wbamberg]]

Docs: https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Security_icons

Does this cover it? I didn't regenerate all the other screenshots, because it seems likely that this will change again soon.

Comment 43

[Tanvi Vyas[:tanvi]]

(In reply to Will Bamberg [:wbamberg] from comment #42)
> Docs:
> https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Security_icons
> 

As I mentioned in Comment 39, the distinction between weak and failed is going to be hard to detect.  Since the icon for weak is the new icon for weak in the url bar (FF42+) and the icon for failed used to mean weak (FF26?-FF41).  Perhaps the red square is all you need and you can use the same icon in this case?  Or you could use a red warning triangle to match the red square for connection failure?

Comment 44

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Tanvi Vyas [:tanvi] from comment #43)
> (In reply to Will Bamberg [:wbamberg] from comment #42)
> > Docs:
> > https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Security_icons
> > 
> 
> As I mentioned in Comment 39, the distinction between weak and failed is
> going to be hard to detect.  Since the icon for weak is the new icon for
> weak in the url bar (FF42+) and the icon for failed used to mean weak
> (FF26?-FF41).  Perhaps the red square is all you need and you can use the
> same icon in this case?  Or you could use a red warning triangle to match
> the red square for connection failure?

Will is documenting the current state after this bug.  Like I said in comment 40, we have bug 1195839 for further icon tweaks, so I think this belongs over there (unless I am misunderstanding).

Comment 45

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Will Bamberg [:wbamberg] from comment #42)
> Docs:
> https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Security_icons
> 
> Does this cover it? I didn't regenerate all the other screenshots, because
> it seems likely that this will change again soon.

I believe this correctly documents the current state, thanks!

Comment 46

[Tanvi Vyas[:tanvi]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #44)
> Will is documenting the current state after this bug.  Like I said in
> comment 40, we have bug 1195839 for further icon tweaks, so I think this
> belongs over there (unless I am misunderstanding).

Sorry, you are right!  I didn't look at the bug carefully.