1187697 - Provide better explanation for why manual review is required

Status: RESOLVED INCOMPLETE

(addons.mozilla.org Graveyard :: Add-on Validation, defect)

Description

[robertknight]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20150718030211

Steps to reproduce:

Uploaded extension set to be unlisted. The validation report came back with a collection of warnings - some trivial, some less so (all warnings are unfortunately shown as having the same level of severity).

A message was displayed at the bottom of step 2 of the submission process - "Looks like your add-on requires a manual review before it can be signed."

It isn't clear though _why_ a manual review is required. Does that happen if the signing process generates _any_ warnings?

Many of those warnings come from external libs (Q, jQuery) which are going to be a PITA to resolve.

For our Chrome extension we plan to do automated deployments using the Chrome web store APIs once our internal integration tests have run, which means that we can deliver frequent updates and respond to user needs quickly. We'd like Firefox users to have an equally good experience and equally frequent updates. For that reason manual reviews are a complete non-starter.


Actual results:

Validation report: https://addons.mozilla.org/en-US/developers/upload/838f90847c7243d79b2033e94feccf19

(This is a very early version of the extension that we are developing, it is of limited use but we're testing the deployment pipeline)

Comment 1

[robertknight]

> We'd like Firefox users to have an equally good experience and equally frequent updates. For that reason manual reviews are a complete non-starter.

That sounds quite negative, but I should add that I completely support the motivation for requiring extensions to be signed and appreciate that malicious and/or poorly performing addons have been a significant problem for Firefox users in the past.

Comment 2

[Jorge Villalobos [:jorgev] (he/him)]

This may be a dupe of bug 1172030, but since there's already a PR for that one, it's possible there are things here that need to be treated separately.

Comment 3

[robertknight]

Hi Jorge,

Thanks for the response. bug 1172030 covers the UX issues with the Add On submission tool, specifically:

- I did not see the 'Signing Severity' messages at all
- Even if I had, I'd still prefer that the reasons for failing validation are explicit in the final summary message ("Your addon requires manual review because of N warnings")

There are a few things I'm not clear on though:

- Does the add-on validation apply the same standards of validation all JS shipped in the add-on? Many of the warnings are in well-known third party libraries. One approach we could take would be to move such code to a background page and minimize the amount of code in the main add-on script that has privileged access to Firefox, essentially sandboxing all the third party libs. That will only work if the add-on verifier is more lenient amount the level of validation applied to content scripts though.

- The warnings mentioning security issues could use more explanation. For example, the one about passing a non-function expression to setTimeout().

Comment 4

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to robertknight from comment #3)
> - Does the add-on validation apply the same standards of validation all JS
> shipped in the add-on? Many of the warnings are in well-known third party
> libraries. One approach we could take would be to move such code to a
> background page and minimize the amount of code in the main add-on script
> that has privileged access to Firefox, essentially sandboxing all the third
> party libs. That will only work if the add-on verifier is more lenient
> amount the level of validation applied to content scripts though.

The validator doesn't show warnings for libraries it's aware of. The library validation code is due an overhaul, which should come relatively soon. For now it's a bit hit or miss.

> - The warnings mentioning security issues could use more explanation. For
> example, the one about passing a non-function expression to setTimeout().

That's something that I think will be part of today's validator update. That one specifically matter because JS strings aren't being validated, and setTimeout (and others) can be used to execute JS strings.

Comment 5

[robertknight]

>  That one specifically matter because JS strings aren't being validated,
> and setTimeout (and others) can be used to execute JS strings.

Could this be dealt with by locking down the JS environment itself - eg. disabling various methods of executing code that is not part of the bundle, such as eval(), setTimeout() with a string.

See https://developer.chrome.com/extensions/sandboxingEval for how they handle eval. I presume setTimeout() with a string is handled in the same way though I haven't verified this.

Comment 6

[robertknight]

Indeed, the default CSP for Chrome extension background pages has more detail: https://developer.chrome.com/extensions/contentSecurityPolicy

Re-using CSP here is a nice approach since web developers are already familiar with it.

Having recently observed developers new to writing extensions working with Chrome and Firefox, one thing that soon became apparent is that having the privileged environment (background pages in Chrome) work in an environment that is much more like a regular page (ie. with a DOM) is much easier than Firefox's pseudo-CommonJS environment.