Closed
Bug 1266012
Opened 9 years ago
Closed 8 years ago
Implement identity indication for moz-extension:// scheme
Categories
(WebExtensions :: Frontend, defect, P3)
WebExtensions
Frontend
Tracking
(firefox55 fixed)
People
(Reporter: bugzilla-ng, Assigned: mattw, Mentored)
References
()
Details
(Whiteboard: [design-decision-approved] triaged)
Attachments
(3 files, 8 obsolete files)
(deleted),
text/x-review-board-request
|
mixedpuppy
:
review+
|
Details |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
designakt
:
ui-review+
|
Details |
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•9 years ago
|
||
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)
Comment 2•9 years ago
|
||
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•9 years 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•9 years 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•9 years ago
|
Whiteboard: [design-decision-needed] triaged
Comment 5•8 years ago
|
||
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
Comment 6•8 years ago
|
||
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)
Comment 7•8 years ago
|
||
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)
Updated•8 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•8 years ago
|
Assignee: nobody → amckay
Comment 8•8 years ago
|
||
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)
Comment 9•8 years ago
|
||
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)
Comment 10•8 years ago
|
||
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)
Comment 11•8 years 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)
Comment 12•8 years ago
|
||
(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)
Comment 13•8 years ago
|
||
(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•8 years 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•8 years ago
|
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Priority: -- → P3
Comment 15•8 years ago
|
||
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•8 years ago
|
||
Rough, work in progress, no tests.
Updated•8 years ago
|
Assignee: amckay → mwein
Updated•8 years ago
|
webextensions: --- → +
Comment 17•8 years ago
|
||
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
Comment 18•8 years ago
|
||
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•8 years ago
|
Attachment #8862973 -
Flags: feedback?(jhofmann)
Assignee | ||
Comment 20•8 years ago
|
||
Assignee | ||
Comment 21•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8863817 -
Flags: ui-review?(mjaritz)
Assignee | ||
Updated•8 years ago
|
Attachment #8863825 -
Flags: ui-review?(mjaritz)
Comment 22•8 years 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•8 years 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 24•8 years ago
|
||
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•8 years 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•8 years 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 27•8 years ago
|
||
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-
Comment 28•8 years ago
|
||
Here is a mock-up of the changes I suggested in my previous comment.
Comment 29•8 years ago
|
||
Updated to the right state of the (i) icon for an open panel.
Attachment #8864561 -
Attachment is obsolete: true
Comment 30•8 years ago
|
||
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•8 years ago
|
Attachment #8743195 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8796343 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8807291 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(jmoradi)
Assignee | ||
Updated•8 years ago
|
Attachment #8863817 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8863825 -
Attachment is obsolete: true
Assignee | ||
Comment 31•8 years ago
|
||
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•8 years 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.
Comment 33•8 years ago
|
||
(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•8 years ago
|
||
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-
Comment 35•8 years ago
|
||
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•8 years ago
|
Attachment #8866981 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Assignee | ||
Comment 37•8 years ago
|
||
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 39•8 years ago
|
||
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•8 years 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•8 years 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•8 years 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) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 48•8 years 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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Updated•2 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•