Closed Bug 1332061 Opened 3 years ago Closed 3 years ago

Add unsigned warning to webextension permission dialog

Categories

(Toolkit :: Add-ons Manager, defect, P1)

51 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox55 --- verified

People

(Reporter: aswan, Assigned: aswan)

References

Details

(Whiteboard: [permissions] triaged)

Attachments

(3 files)

This is follow-up from https://bugzilla.mozilla.org/show_bug.cgi?id=1308295#c32
Flags: needinfo?(emanuela)
Whiteboard: [permissions]
Priority: -- → P1
Whiteboard: [permissions] → [permissions] triaged
Assignee: nobody → aswan
Let's display the warning copy between the intro text and the list.

Visual adjustment:

— add-on's logo in grayscale with a 50% opacity

— warning icon (https://firefoxux.github.io/firefox-svg-icons/icons/alert/warning.svg) on the bottom right of the add-on's logo

The warning copy must be reviewed by Scott.
Flags: needinfo?(emanuela)
Scott, can you review the copy?  I'm skeptical of repeating the add-on name 3 times, some names are pretty long and this could get out of hand...
Flags: needinfo?(sdevaney)
Agree. But I'm confused about the origin of portions of the copy. Where did they "... only if you are sure of what you are doing" come from? That messaging is vague and unsettling. Here's what I'd propose for the full copy: 

Would you like to add [name of add-on] to Firefox? 

This extension is not verified by Firefox. Please review its permission requests. 

It's able to:
Flags: needinfo?(sdevaney)
Wait, are we switching from "It requires your permission to:" back to "It's able to:" ?
We're also toning down the message from the previous:
Caution: This site would like to install an unverified add-on in #1. Proceed at your own risk.
In the interest of getting this string right the first time, if the string from comment 3 is the proposal, I'd like to run it by security before updating.
Flags: needinfo?(sdevaney)
Apologies, aswan, I was going off the copy in the design mock and didn't connect it to what we had arrived at previously. How does this look? 

Would you like to add [name of add-on] to Firefox? 

Caution: This site would like to install an unverified add-on. Proceed at your own risk. 

It requires your permission to:
Flags: needinfo?(sdevaney)
Sorry to be so pedantic but these strings are a pain to change later.  I want to push back a little on "this site".  I have no data to back this up but I suspect that a major fraction of installations of unsigned add-ons are from local files which makes the language about "this site" confusing.
Can we say "Caution: This addon is unverified.  Proceed at your own risk"?
Flags: needinfo?(sdevaney)
Makes total sense. Let's go with:

Would you like to add [name of add-on] to Firefox? 

Caution: This add-on is unverified. Proceed at your own risk. 

It requires your permission to:
Flags: needinfo?(sdevaney)
I assume the brackets around the name won't actually appear, though I kind of wish they would. We've had a long history of people making spoofing demonstrations (and occasionally actual attacks) when they can insert random text into a dialog. The earlier form of this that repeated the name three times was kind of ugly, but at least the same spoofing fragment wouldn't grammatically work in all of those places.

  Would you like to add Firefox features? Click 'Add' to enable the ability to install add-ons to Firefox?

Kind of confusing, maybe people wouldn't notice the extra '?' at the end. Not clear what's going on but looks harmless so OK.

Making it bold as in the screen shots helps, but might be too subtle in some fonts. Sticking the name in the middle of a sentence might also be grammatically awkward for translators, if add-on names could be interpreted as gendered or some other attribute that requires matching forms (number?).

You folks seem sold on using 'add' vs 'install' but to me that underplays the danger people are undertaking. Especially for unverified add-ons we haven't reviewed! "add and subtract" are conceptually wedded, makes me think I can just un-add things I don't like and there's no problem -- but at that point it's too late if it's malicious stuff.
The summary of this bug says "add unsigned warning"... are these really unsigned?! Later you say "unverified" which I'm assuming means unreviewed (but signed), but it's not clear.
Flags: needinfo?(aswan)
(In reply to Daniel Veditz [:dveditz] from comment #9)
> The summary of this bug says "add unsigned warning"... are these really
> unsigned?! Later you say "unverified" which I'm assuming means unreviewed
> (but signed), but it's not clear.

No, this is about a relatively uncommon scenario: user is using Aurora, Nightly, or an unbranded build, and has the preference flipped to disable signing enforcement.  The "unverified" language mirrors what we current display in the notification in this case:
http://searchfox.org/mozilla-central/rev/672c83ed65da286b68be1d02799c35fdd14d0134/browser/locales/en-US/chrome/browser/browser.properties#156

I had also understood that we wouldn't include the brackets but would put the extension name in bold with the surrounding text in normal weight.  We can add brackets if that's desirable though I anticipate pushback from the l10n folks if we try that.  As for the bold/normal distinction being too subtle in some fonts, is there a way for users to change the font used in system dialogs?  And I agree that using the name again (once instead of twice as in the mockup) in the warning message could mitigate the spoofing issue, I'm happy to do that if that's the consensus.

In any case, I think the wording of the message is the big thing here, I have no objection to making it more severe, though I think that the fact the user is using a non-release/beta build and has presumably chosen to flip the signing pref means they've already indicated a willingness to run unsigned code.

Lets take the time now to get this right, changing strings later causes a lot of unnecessary churn for translators.  Actually, I don't know what your plans are for bug 1316460 but a comprehensive review of the strings in all the notifications seems like an obvious thing to include there, do you want to take this discussion over there or are your concerns limited to the unsigned extensions scenario?
Flags: needinfo?(aswan) → needinfo?(dveditz)
So granting that the user in this case is using a nightly build and has intentionally enabled unsigned installs, the question is whether the dialog makes it clear enough that any given install is, in fact, an unsigned install. I think it needs to look more different from the normal dialog because in a user's mind

 1. I click the install button
 2. the expected install prompt comes up
 3. I automatically click the OK button

it could be pretty habitual and only after the click does the user think "wait, what?"

tweaking the wording won't make a whole lot of difference if the dialog basically "looks the same" -- it's not going to get read. Could we drop the add-on supplied logo entirely? It's an unsigned install, we can't trust the logo. On a malicious add-on the logo will be whatever might make the add-on seem more trustworthy. Grey scale helps; does it help enough?

What if you just show a larger warning icon in that spot?

I'm not asking that the name be enclosed in brackets, just trying to understand the screenshot.

I don't like replacing "install" with "add to". Maybe the distinction doesn't mean much to average people, but "add" makes it seem encapsulated and unthreatening. 

"only if you're sure what you're doing" puts the burden on the wrong party. The user knows what they're doing, they're installing something. What they _ought_ to think about is the source of the thing they're installing. "only if you trust the source of this add-on" would be better. The "Proceed at your own risk" idea from the linked properties file is more in the right direction, or "Caution, malicious add-ons can steal your private information or compromise your computer". In the unsigned add-on case this dialog needs to go from being a confirmation to being a warning.
Flags: needinfo?(dveditz)
Blocks: 1342142
Here's a proposal based on Dan's most recent comment.  Emanuela, Scott, is this okay for you guys?
Flags: needinfo?(sdevaney)
Flags: needinfo?(emanuela)
I'm OK with the copy reflected in the screen shot. 

On the point about the use of "Add", that convention is employed in a number of places so I think it makes sense to remain consistent with it.
Flags: needinfo?(sdevaney)
aswan: I still think we should go with the addon logo in greyscale + the warning badge.

I don't agree in general with Dan's comment because it's not taking into consideration the specific kind of users we are dealing here.
Flags: needinfo?(emanuela)
I can see both sides here, but I'm going to side with Dan on this one, its important to show this big and clearly that its a problem. This only hits users on Nightly or Dev Edition who have turned off signing and not release users (since they can't). Let's please use the standard built in icon for this though: https://firefoxux.github.io/firefox-svg-icons/icons/alert/warning.svg
Attachment #8835635 - Flags: review?(florian)
Comment on attachment 8835635 [details]
Bug 1332061 Add warning for unsigned extensions

https://reviewboard.mozilla.org/r/111312/#review121022

::: browser/base/content/test/webextensions/browser.ini:26
(Diff revision 3)
>  [browser_permissions_addons_search.js]
>  [browser_permissions_installTrigger.js]
>  [browser_permissions_local_file.js]
>  [browser_permissions_mozAddonManager.js]
> +[browser_permissions_unsigned.js]
> +skip=if = require_signing

The syntax seems wrong here, other files use:
skip-if = require_signing
Attachment #8835635 - Flags: review?(florian) → review+
Comment on attachment 8835635 [details]
Bug 1332061 Add warning for unsigned extensions

https://reviewboard.mozilla.org/r/111312/#review121022

> The syntax seems wrong here, other files use:
> skip-if = require_signing

Ah, thanks for catching that, I guess that typo turns it into a valid, but useless, line so nothing fails (until this hits beta if it had landed like that...)
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/782f34bf8bb5
Add warning for unsigned extensions r=florian
https://hg.mozilla.org/mozilla-central/rev/782f34bf8bb5
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Verified as fixed on Firefox 55.0a1 (2017-03-17) under Windows 10 64-bit, Ubuntu 16.04 32-bit and Mac Os X 10.12.1. "Caution" pop-up is successfully displayed while installing unverified extensions: https://www.screencast.com/t/Mrhh3nuc1tZ , https://www.screencast.com/t/Gb81QTuj .
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.