1266012 - Implement identity indication for moz-extension:// scheme

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect, P3)

Description

[bugzilla-ng]

Attachment: Possible identity indication example for a moz-extension: page

From https://discourse.mozilla-community.org/t/webextensions-anti-phishing-for-add-on-pages/8060

moz-extension:// URIs include random strings. Although this is good for privacy, it is not nice for UX. An add-on can handle security-sensitive things. Users cannot remember or recognize trusted URIs. Teaching about the moz-extension scheme is not enough: users have different expectation for each add-on. The add-on responsible for a moz-extension: page must be clear.

We have Identity box. We use that for indicating EV certs, for example. We can use that to address this problem, too.

__________________________________________________________________________________
[Generic "extension" icon] | Extension ([Add-on Name]) | moz-extension://<uuid>/
----------------------------------------------------------------------------------

This should be easy, but will greatly increase security against identity spoofing.

Comment 1

[Kris Maglione [:kmag]]

This does seem like a good idea, but I'm not really sure how much it would increase security.

Thoughts, Markus, Kev?

Comment 2

[Markus Jaritz [:designakt] (UX)]

I think this is a very good suggestion. I especially like that this proposal is re-using the identity box as it is already a known patter for users.
I think that increased visibility of what add-on is responsible for that page will help security, as it will link the add-on to the content, and maybe remind the user about an add-on they had forgotten about, or maybe aren't even aware of.

Comment 3

[Kev Needham [:kev]]

Adding Javaun as well, because this crosses a couple of domains (I swear I'm not trying to make an Awesomebar pun), and I think the identity box is his domain.

The Identity box is used to identify the creator of the page content (who vs. where), so the question for me here is are we look to identify what the content is, or who (which entity) created it. Agree it could be useful for helping users identify unfamiliar content (similar to some about: URIs), but should we be looking at author or type? It is a bit of a different paradigm.

I like the idea, but I'd like to understand if it'd be more consistent with identifying the source of the content directly (named addon) vs. a generic addon indicator.

Comment 4

[Kev Needham [:kev]]

Javaun - just to put this on your radar (I think Identity box is you) - what's the approach you'd recommend here, and is identifying content such as specific add-ons something that fits in with goals with identity? We have no way to validate the identity of the author (we don't use author-specific certs), so the goal would be "this content is from (this) add-on", with the info box pointing to the add-on manager or the installed add-on specifically, with a little extra metadata about the add-on.

Comment 5

[Andy McKay]

Chatted to a few people about this one and it got a broad thumbs up as a good idea.

We thought as first pass it could just show an add-ons logo (the jigsaw peice), the name of the add-on and that's about it. I don't know if it needs much more UX than that, but Markus if you want to do more, please let me know.

Comment 6

[Markus Jaritz [:designakt] (UX)]

Great idea.
Maybe instead of only the add-on name, we can add "extension" as a qualifier like in the initial proposal so that the name will not be mistaken for a trusted entity or even a message from Firefox approving this page. This is something where :dveditz might have some security input.

Comment 7

[Daniel Veditz [:dveditz]]

You could, but I suspect it would be uglier than it would be helpful. Ideally the plug-piece (or other) icon conveys it's an extension well enough, and users can click on it and get a longer explanation in the drop-down. e.g. about: pages say the URL and "This is a secure Firefox page". In this case it would be the name of the add-on on the first line--repeated from the box, but we should stick to the format users expect in the drop-down--and something like "This is an extension-generated page".

We should always use the same icon, of course! It would be spoofy to allow extensions to provide their own icons and users wouldn't learn the association between a consistent icon and extension content.

The add-on's name might itself be spoofy but that's malicious behavior. We should be able to catch it for AMO-hosted add-ons, and for signed non-hosted add-ons that's the least of the malicious things it could do. If it's a minor problem we blocklist, and if it's a major problem we need to re-think our policies on signing non-hosted content.

Comment 8

[Andy McKay]

Attachment: Screenshot 2016-09-29 18.41.39.png

Attached a version of how this could look. The only add-on customizable part of this is the "Add-on Title". I note that the green text is used in the identity part in other places, but it fits well with the green of the icon.

Any suggestions on that appreciated.

Comment 9

[Markus Jaritz [:designakt] (UX)]

Looks good. In the URL-bar I would however use Extension instead of Add-on. Same in the second line on the panel as only extensions can create such pages, not all add-ons.
I like that you follow the pattern used with Firefox pages. And green matches great with the icon. However I am not sure about the color. Firefox pages are orange, and only secure pages are green. I wonder if this might suggest a false sense of safety with extension pages. If so we might just use black text. Wonder what :dveditz thinks.

And for the copy of the second line in the panel I would like to get input from Michelle:
This page is loaded from an extension.
This is an extension-generated page.
This is a page provided by an extension.

For reference: Currently this line is used to indicate how secure a page is.
HTTPS:      Secure Connection
HTTP:       Connection is Not Secure
about:home: This is a secure Firefox page.
extension:  ?

Comment 10

[Daniel Veditz [:dveditz]]

My only (minor) security concern is about the use of green text, which looks like a trusted EV page. That's not the lock icon, though. If the text is the fixed string "Add-on" (or "Extension") it should be OK, I only start to worry if that's add-on settable text. Markus notes "Firefox" pages are orange, but Nightly pages are black text and that could work fine, too.

From a power-user POV I'd find putting the name in the URL bar more useful than relegating it to the drop-down, and it doesn't really matter if it pushes more of the real URL out of view because the moz-extension://<random> junk is mostly meaningless. If you do then it's more important not to use green text, and maybe to use a fixed prefix like "Add-on: ". However, it certainly simplifies the security spoofing worries if you ignore my desires and go with the fixed text in your screenshot; I recommend you go with simple first.

Most places in our UI (e.g. the Tool and Hamburger menus, our AMO site) use the word "Add-on" rather than "Extension". It will be less confusing to users to stick with that. Our internal distinction between types of add-ons are unknown or confusing to users.

Comment 11

[mheubusch]

So, making sure I understand the issue.  

Is the point to communicate Reliability "the information on this page was created by the add-on, so take if for what it's worth" or is it Security "the page itself was generated by the add-on, not by Firefox, so you're on your own here if anything happens"  

If the former, I'd say "This content is published by an add-on."

If the latter, I'd say "This page is generated by an add-on."

Comment 12

[Markus Jaritz [:designakt] (UX)]

(In reply to mheubusch from comment #11)
> 
> ... Security "the page itself was generated by the add-on, not by Firefox, 
> so you're on your own here if anything happens"  
> ...
> I'd say "This page is generated by an add-on."

As this panel is about security we will go with "generated". Thanks.

Comment 13

[Markus Jaritz [:designakt] (UX)]

(In reply to Daniel Veditz [:dveditz] from comment #10)
> Most places in our UI (e.g. the Tool and Hamburger menus, our AMO site) use
> the word "Add-on" rather than "Extension". It will be less confusing to
> users to stick with that. Our internal distinction between types of add-ons
> are unknown or confusing to users.

Michelle, the question if we should use "Add-on" or "Extension" has come up here.
From the Firefox Voice and Tone Guide I understand that we should always use the most distinct term for a given description. Which here would be "Extension".
https://docs.google.com/document/d/1SjIg4ccoZvfTA6bph1er0mBIyMHbRxlXztMYpy-eYuA/edit#heading=h.z8j563g1zrme
Can you please comment on that.

Comment 14

[mheubusch]

Hi Markus - yes, I saw that and didn't call out my rationale. Sorry. I do think we should go with add-ons in this case and probably need to modify the style guide advice.  Per the thread with Scott DeVaney in the disco pane copy doc (here: https://docs.google.com/document/d/1rGBaMwpr_qbdah_LE1oZkTVIPZKNEAwRfH0zRbd0YaY/edit#) I think we are tending to use add-ons more now as it is the predominant term and are attempting to only use extension as a distinguisher when necessary.  I also note that the icon in the designs you've created is the one we use to indicate add-ons in the browser pancake menu panel so wanted to train users to recognize it as such.  Let me know if you want to discuss in greater detail - I can set up a Vidyo meeting.

Comment 15

[Markus Jaritz [:designakt] (UX)]

Let's chat outside of this bug about general application of those terms.
For here we have a decision to label it add-ons. Thanks.

Comment 16

[Andy McKay]

Attachment: identity.patch

Rough, work in progress, no tests.

Comment 17

[Johann Hofmann [:johannh]]

fwiw I'm happy to help move this forward if you need someone for feedback/review/advice.

Also, not sure if you're aware, but Chrome has this now.

Comment 18

[Jonathan Kingston [:jkt] he/him]

I would be pro hiding the "moz-extension://uuid/" also like we do for "http" as I think this is clear what has loaded it and I'm not convinced it needs to be navigable either.

This mock up seems fine:
https://bug1266012.bmoattachments.org/attachment.cgi?id=8743195

Adding the info in the control centre would also help users who might be confused also.

Comment 19

[Matthew Wein [:mattw]]

Attachment: Bug 1266012 - Add identity indication for the moz-extensions scheme

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

Comment 20

[Matthew Wein [:mattw]]

Attachment: identity_indication_screenshot.png

Comment 21

[Matthew Wein [:mattw]]

Attachment: identity_indication_with_long_addon_title_screenshot.png

Comment 22

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8862973 [details]
Bug 1266012 - Add identity indication for the moz-extensions scheme

https://reviewboard.mozilla.org/r/134838/#review138600

::: browser/base/content/browser.js:6816
(Diff revision 1)
> +      Cu.reportError("Extension cannot be found in AddonPolicyService.");
> +    }
> +  }
> +
> +  if (extensionId) {
> +    let extension = GlobalManager.extensionMap.get(extensionId)

I'd rather not import globalmanager this way.  The name could be retrieved with:

let addon = await AddonManager.getAddonByID(extensionId);
return addon.name

Comment 23

[Johann Hofmann [:johannh]]

Comment on attachment 8862973 [details]
Bug 1266012 - Add identity indication for the moz-extensions scheme

https://reviewboard.mozilla.org/r/134838/#review138796

::: browser/themes/shared/jar.inc.mn:32
(Diff revision 1)
>  * skin/classic/browser/controlcenter/arrow-subview.svg         (../shared/controlcenter/arrow-subview.svg)
>  * skin/classic/browser/controlcenter/arrow-subview-back.svg    (../shared/controlcenter/arrow-subview-back.svg)
>  * skin/classic/browser/controlcenter/conn-not-secure.svg       (../shared/controlcenter/conn-not-secure.svg)
>  * skin/classic/browser/controlcenter/connection.svg            (../shared/controlcenter/connection.svg)
>  * skin/classic/browser/controlcenter/mcb-disabled.svg          (../shared/controlcenter/mcb-disabled.svg)
> +  skin/classic/browser/controlcenter/extension.svg             (../shared/controlcenter/extension.svg)

Did you forget to check this file in?

Comment 24

[Johann Hofmann [:johannh]]

Comment on attachment 8862973 [details]
Bug 1266012 - Add identity indication for the moz-extensions scheme

Seems fine from identity UI side.

Comment 25

[Johann Hofmann [:johannh]]

Comment on attachment 8862973 [details]
Bug 1266012 - Add identity indication for the moz-extensions scheme

https://reviewboard.mozilla.org/r/134838/#review138802

::: browser/components/controlcenter/content/panel.inc.xul:36
(Diff revision 1)
>            <description class="identity-popup-connection-secure"
>                         when-connection="secure secure-ev">&identity.connectionSecure;</description>
>            <description when-connection="chrome">&identity.connectionInternal;</description>
>            <description when-connection="file">&identity.connectionFile;</description>
> +          <description value="&identity.extensionPage;"
> +                       when-connection="extension"/>

Please move the value attribute inside the description tag (like above). This will make the text wrap properly AFAIK.

Comment 26

[Johann Hofmann [:johannh]]

Comment on attachment 8862973 [details]
Bug 1266012 - Add identity indication for the moz-extensions scheme

https://reviewboard.mozilla.org/r/134838/#review138804

::: browser/themes/shared/identity-block/icons.inc.css:22
(Diff revision 1)
>    list-style-image: url(chrome://browser/skin/identity-icon.svg#hover@iconVariant@);
>  }
>  
> +#identity-box.extensionPage:hover > #identity-icon:not(.no-hover),
> +#identity-box.extensionPage[open=true] > #identity-icon {
> +  list-style-image: url(chrome://browser/skin/controlcenter/extension.svg);

Please check with UX that they really intend to leave this without any hover state. If yes, you should probably be able to merge this rule with the one you added above.

Comment 27

[Markus Jaritz [:designakt] (UX)]

Comment on attachment 8863817 [details]
identity_indication_screenshot.png

This looks nice.
Checking with Michelle I learned that we should use "Extension" instead of "Add-on" in that case. (Our guidelines on usage of those words changed since her last comment on that bug.)

(In reply to Johann Hofmann [:johannh] from comment #26)
> Please check with UX that they really intend to leave this without any hover
> state. If yes, you should probably be able to merge this rule with the one
> you added above.

And through Johann's comment I realized that we do not offer the (i) we use on websites to indicate additional information. Would be great to add that. (Also resolves his question as that has a hover state.)

Comment 28

[Markus Jaritz [:designakt] (UX)]

Attachment: identity_indication_screenshot_mod.png

Here is a mock-up of the changes I suggested in my previous comment.

Comment 29

[Markus Jaritz [:designakt] (UX)]

Attachment: identity_indication_screenshot_mod.png

Updated to the right state of the (i) icon for an open panel.

Comment 30

[Markus Jaritz [:designakt] (UX)]

Comment on attachment 8863825 [details]
identity_indication_with_long_addon_title_screenshot.png

How you deal with long names is great. Does this also carry over to windows with truncation in the middle of the name?
ui-r - for the comments I left earlier. Thanks for your work.

Comment 31

[Matthew Wein [:mattw]]

Attachment: identity_indication_screenshot.png

Note that I've changed the font color to green so you could see what that would look like. Let me know if you'd like to stick with green text, otherwise I'll switch it back to the default text color.

Comment 32

[Matthew Wein [:mattw]]

Comment on attachment 8862973 [details]
Bug 1266012 - Add identity indication for the moz-extensions scheme

https://reviewboard.mozilla.org/r/134838/#review138804

> Please check with UX that they really intend to leave this without any hover state. If yes, you should probably be able to merge this rule with the one you added above.

This is no longer an issue because the default extension icon will now be shown next to the identity icon which will have the hover state.

Comment 33

[Johann Hofmann [:johannh]]

(In reply to Matthew Wein [:mattw] from comment #31)
> Created attachment 8866981 [details]
> identity_indication_screenshot.png
> 
> Note that I've changed the font color to green so you could see what that
> would look like. Let me know if you'd like to stick with green text,
> otherwise I'll switch it back to the default text color.

I agree with dveditz in comment 10, green text should probably be reserved for EV certificates.

Comment 34

[Markus Jaritz [:designakt] (UX)]

Comment on attachment 8866981 [details]
identity_indication_screenshot.png

(In reply to Johann Hofmann [:johannh] from comment #33)
> (In reply to Matthew Wein [:mattw] from comment #31)
> > Created attachment 8866981 [details]
> > identity_indication_screenshot.png
> > 
> > Note that I've changed the font color to green so you could see what that
> > would look like. Let me know if you'd like to stick with green text,
> > otherwise I'll switch it back to the default text color.
> 
> I agree with dveditz in comment 10, green text should probably be reserved
> for EV certificates.

Please switch back to the default black/dark color we use. I'd totally give it a ui-r+ if it wasn't for that color. Everything else looks great. Thanks.

Comment 35

[Markus Jaritz [:designakt] (UX)]

Attachment: identity_indication_screenshot_mod.png

Looking at this again I would love if you could also make the puzzle piece grey so that we exclusively use green for indicating https / certified pages. Thanks.

Comment 36

[Matthew Wein [:mattw]]

Comment on attachment 8862973 [details]
Bug 1266012 - Add identity indication for the moz-extensions scheme

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

Comment 37

[Matthew Wein [:mattw]]

Attachment: identity_indication_screenshot.png

Sounds great, thanks for sharing your reasoning. I've updated the patch to use the default black/dark color for the text and made the default extension icon grey -- I've updated the screenshot to reflect this.

Comment 38

[Matthew Wein [:mattw]]

Comment on attachment 8862973 [details]
Bug 1266012 - Add identity indication for the moz-extensions scheme

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

Comment 39

[Markus Jaritz [:designakt] (UX)]

Comment on attachment 8867716 [details]
identity_indication_screenshot.png

Looks great. Thanks.

Comment 40

[Matthew Wein [:mattw]]

Comment on attachment 8862973 [details]
Bug 1266012 - Add identity indication for the moz-extensions scheme

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

Comment 41

[Matthew Wein [:mattw]]

Comment on attachment 8862973 [details]
Bug 1266012 - Add identity indication for the moz-extensions scheme

https://reviewboard.mozilla.org/r/134838/#review142638

::: browser/themes/shared/controlcenter/extension.svg:7
(Diff revision 4)
> -  <defs>
> -    <style>
> +#include ../icon-colors.inc.svg
> +  <path class="fieldtext" d="M42,62c2.2,0,4-1.8,4-4l0-14.2c0,0,0.4-3.7,2.8-3.7c2.4,0,2.2,3.9,6.7,3.9c2.3,0,6.2-1.2,6.2-8.2 c0-7-3.9-7.9-6.2-7.9c-4.5,0-4.3,3.7-6.7,3.7c-2.4,0-2.8-3.8-2.8-3.8V22c0-2.2-1.8-4-4-4H31.5c0,0-3.4-0.6-3.4-3 c0-2.4,3.8-2.6,3.8-7.1c0-2.3-1.3-5.9-8.3-5.9s-8,3.6-8,5.9c0,4.5,3.4,4.7,3.4,7.1c0,2.4-3.4,3-3.4,3H6c-2.2,0-4,1.8-4,4l0,7.8 c0,0-0.4,6,4.4,6c3.1,0,3.2-4.1,7.3-4.1c2,0,4,1.9,4,6c0,4.2-2,6.3-4,6.3c-4,0-4.2-4.1-7.3-4.1c-4.8,0-4.4,5.8-4.4,5.8L2,58 c0,2.2,1.8,4,4,4H19c0,0,6.3,0.4,6.3-4.4c0-3.1-4-3.6-4-7.7c0-2,2.2-4.5,6.4-4.5c4.2,0,6.6,2.5,6.6,4.5c0,4-3.9,4.6-3.9,7.7 c0,4.9,6.3,4.4,6.3,4.4H42z"/>
> -      .style-puzzle-piece {
> -        fill: url('#gradient-linear-puzzle-piece');
> -      }
> -    </style>
> -    <linearGradient id="gradient-linear-puzzle-piece" x1="0%" y1="0%" x2="0%" y2="100%">
> -      <stop offset="0%" stop-color="#66cc52" stop-opacity="1"/>
> -      <stop offset="100%" stop-color="#60bf4c" stop-opacity="1"/>
> -    </linearGradient>
> -  </defs>
> -  <path class="style-puzzle-piece" d="M42,62c2.2,0,4-1.8,4-4l0-14.2c0,0,0.4-3.7,2.8-3.7c2.4,0,2.2,3.9,6.7,3.9c2.3,0,6.2-1.2,6.2-8.2 c0-7-3.9-7.9-6.2-7.9c-4.5,0-4.3,3.7-6.7,3.7c-2.4,0-2.8-3.8-2.8-3.8V22c0-2.2-1.8-4-4-4H31.5c0,0-3.4-0.6-3.4-3 c0-2.4,3.8-2.6,3.8-7.1c0-2.3-1.3-5.9-8.3-5.9s-8,3.6-8,5.9c0,4.5,3.4,4.7,3.4,7.1c0,2.4-3.4,3-3.4,3H6c-2.2,0-4,1.8-4,4l0,7.8 c0,0-0.4,6,4.4,6c3.1,0,3.2-4.1,7.3-4.1c2,0,4,1.9,4,6c0,4.2-2,6.3-4,6.3c-4,0-4.2-4.1-7.3-4.1c-4.8,0-4.4,5.8-4.4,5.8L2,58 c0,2.2,1.8,4,4,4H19c0,0,6.3,0.4,6.3-4.4c0-3.1-4-3.6-4-7.7c0-2,2.2-4.5,6.4-4.5c4.2,0,6.6,2.5,6.6,4.5c0,4-3.9,4.6-3.9,7.7 c0,4.9,6.3,4.4,6.3,4.4H42z"/>

The diff makes it look like I changed a lot here, but I did here is replace the code which colors the icon green with code that colors it grey. I followed the examples for the other SVG icons, which accomplish this by importing icon-colors.inc.svg and using the "fieldtext" class.

Comment 42

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8862973 [details]
Bug 1266012 - Add identity indication for the moz-extensions scheme

https://reviewboard.mozilla.org/r/134838/#review142692

r+ with comment addressed or explained.

::: browser/themes/shared/identity-block/identity-block.inc.css:169
(Diff revision 4)
>    visibility: collapse;
>  }
>  
> +/* EXTENSION ICON */
> +
> +#extension-icon {

any reason you duplicated instead of “#extension-icon, #connection-icon”?  If not combine them.

Comment 43

[Matthew Wein [:mattw]]

Comment on attachment 8862973 [details]
Bug 1266012 - Add identity indication for the moz-extensions scheme

https://reviewboard.mozilla.org/r/134838/#review142692

> any reason you duplicated instead of “#extension-icon, #connection-icon”?  If not combine them.

Nope, I think I just got led into creating a separate rule because of the large comment above the connection icon. I'll make this update before checking in.

Comment 44

[Matthew Wein [:mattw]]

Comment on attachment 8862973 [details]
Bug 1266012 - Add identity indication for the moz-extensions scheme

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

Comment 45

[Matthew Wein [:mattw]]

Comment on attachment 8862973 [details]
Bug 1266012 - Add identity indication for the moz-extensions scheme

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134838/diff/5-6/

Comment 46

[Matthew Wein [:mattw]]

Comment on attachment 8862973 [details]
Bug 1266012 - Add identity indication for the moz-extensions scheme

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134838/diff/6-7/

Comment 47

[Matthew Wein [:mattw]]

Comment on attachment 8862973 [details]
Bug 1266012 - Add identity indication for the moz-extensions scheme

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134838/diff/7-8/

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/18c74031bd89
Add identity indication for the moz-extensions scheme r=mixedpuppy

Comment 49

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/18c74031bd89