Implement identity indication for moz-extension:// scheme

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: Frontend
P3
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: bugzilla-ng, Assigned: mattw, Mentored)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [design-decision-approved] triaged, URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 8 obsolete attachments)

(Reporter)

Description

a year ago
Created attachment 8743195 [details]
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.
This does seem like a good idea, but I'm not really sure how much it would increase security.

Thoughts, Markus, Kev?
Flags: needinfo?(mjaritz)
Flags: needinfo?(kev)
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.
Flags: needinfo?(mjaritz)

Comment 3

a year ago
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.
Flags: needinfo?(kev)

Comment 4

a year ago
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.
Flags: needinfo?(jmoradi)

Updated

a year ago
Whiteboard: [design-decision-needed] triaged
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.
Whiteboard: [design-decision-needed] triaged → [design-decision-approved] triaged
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.
Flags: needinfo?(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.
Flags: needinfo?(dveditz)
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

10 months ago
Assignee: nobody → amckay

Updated

10 months ago
Depends on: 1306498

Comment 8

10 months ago
Created attachment 8796343 [details]
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.
Flags: needinfo?(mjaritz)
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:  ?
Flags: needinfo?(mjaritz)
Flags: needinfo?(mheubusch)
Flags: needinfo?(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.
Flags: needinfo?(dveditz)

Updated

10 months ago
Depends on: 1307841

Comment 11

10 months ago
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."
Flags: needinfo?(mheubusch) → needinfo?(mjaritz)
(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.
Flags: needinfo?(mjaritz)
(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.
Flags: needinfo?(mheubusch)

Comment 14

10 months ago
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.
Flags: needinfo?(mheubusch) → needinfo?(mjaritz)

Updated

10 months ago
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Priority: -- → P3
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.
Flags: needinfo?(mjaritz)

Comment 16

9 months ago
Created attachment 8807291 [details] [diff] [review]
identity.patch

Rough, work in progress, no tests.

Updated

7 months ago
Assignee: amckay → mwein

Updated

4 months ago
Blocks: 1351165

Updated

4 months ago
webextensions: --- → +
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.
Mentor: jhofmann@mozilla.com
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 hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8862973 - Flags: feedback?(jhofmann)
(Assignee)

Comment 20

3 months ago
Created attachment 8863817 [details]
identity_indication_screenshot.png
(Assignee)

Comment 21

3 months ago
Created attachment 8863825 [details]
identity_indication_with_long_addon_title_screenshot.png
(Assignee)

Updated

3 months ago
Attachment #8863817 - Flags: ui-review?(mjaritz)
(Assignee)

Updated

3 months ago
Attachment #8863825 - Flags: ui-review?(mjaritz)

Comment 22

3 months ago
mozreview-review
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
Attachment #8862973 - Flags: review?(mixedpuppy)

Comment 23

3 months ago
mozreview-review
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 on attachment 8862973 [details]
Bug 1266012 - Add identity indication for the moz-extensions scheme

Seems fine from identity UI side.
Attachment #8862973 - Flags: feedback?(jhofmann) → feedback+

Comment 25

3 months ago
mozreview-review
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

3 months ago
mozreview-review
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 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.)
Attachment #8863817 - Flags: ui-review?(mjaritz) → ui-review-
Created attachment 8864561 [details]
identity_indication_screenshot_mod.png

Here is a mock-up of the changes I suggested in my previous comment.
Created attachment 8864577 [details]
identity_indication_screenshot_mod.png

Updated to the right state of the (i) icon for an open panel.
Attachment #8864561 - Attachment is obsolete: true
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.
Attachment #8863825 - Flags: ui-review?(mjaritz) → ui-review-
(Assignee)

Updated

3 months ago
Attachment #8743195 - Attachment is obsolete: true
(Assignee)

Updated

3 months ago
Attachment #8796343 - Attachment is obsolete: true
(Assignee)

Updated

3 months ago
Attachment #8807291 - Attachment is obsolete: true
(Assignee)

Updated

3 months ago
Flags: needinfo?(jmoradi)
(Assignee)

Updated

3 months ago
Attachment #8863817 - Attachment is obsolete: true
(Assignee)

Updated

3 months ago
Attachment #8863825 - Attachment is obsolete: true
(Assignee)

Comment 31

3 months ago
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.
Attachment #8866981 - Flags: ui-review?(mjaritz)
(Assignee)

Comment 32

3 months ago
mozreview-review-reply
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.
(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 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.
Attachment #8866981 - Flags: ui-review?(mjaritz) → ui-review-
Created attachment 8867145 [details]
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.
Attachment #8864577 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8866981 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Comment 37

2 months ago
Created attachment 8867716 [details]
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.
Attachment #8867716 - Flags: ui-review?(mjaritz)
Comment hidden (mozreview-request)
Comment on attachment 8867716 [details]
identity_indication_screenshot.png

Looks great. Thanks.
Attachment #8867716 - Flags: ui-review?(mjaritz) → ui-review+
Comment hidden (mozreview-request)
(Assignee)

Comment 41

2 months ago
mozreview-review
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

2 months ago
mozreview-review
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.
Attachment #8862973 - Flags: review?(mixedpuppy) → review+
(Assignee)

Comment 43

2 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Blocks: 1365355
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 48

2 months ago
Pushed by mwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/18c74031bd89
Add identity indication for the moz-extensions scheme r=mixedpuppy

Comment 49

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/18c74031bd89
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1365911
Depends on: 1366242
You need to log in before you can comment on or make changes to this bug.