Addons-server: Use static analysis flags in post-review queue prioritization

[EnTeQuAk]

For what's needed in addons-linter:

• eval, evalInSandbox, document.write - mozilla/addons-linter#1047 - code: DANGEROUS_EVAL
• setInterval, and on* attributes - mozilla/addons-linter#1206 - code: NO_IMPLIED_EVAL
• <script> tag with a remote URL - https://github.com/mozilla/addons-linter/issues/1321 - needs to be implemented
• Unsafe DOM: innerHTML - mozilla/addons-linter#1187 - code: UNSAFE_VAR_ASSIGNMENT
• Custom CSP - mozilla/addons-linter#562 - code: MANIFEST_CSP
• Plain HTTP for sensitive operations -mozilla/addons-linter #259 - needs to be implemented
• Synchronous (non-local) HTTP requests - https://github.com/mozilla/addons-linter/issues/1322 - needs to be implemented
• Violate the Mozilla Conditions of Use - mozilla/addons-linter#1285 - needs to be implemented
• Require permissions that are not used - mozilla/addons-linter#1286 - needs to be implemented, might be hard to do, easy to fool (imho)
• Native messaging - needs issue filed
• Libraries and other unreadable code - https://github.com/mozilla/addons-linter/issues/1323 - needs to be implemented, vague description

In case there's already a code you can iterate through an add-ons validation messages and filter for that code which internally is the message id

[diox]

Note: nativeMessaging does not need a linter issue for now, I'll just detect it from the permission list.

[diox]

Closing to be verified by QA, but it's still missing some flags that have not been implemented in the linter yet. See https://github.com/mozilla/addons-server/issues/5475#issuecomment-306451131 for the list of missing "needs to be implemented in linter" flags

[ValentinaPC]

@diox : I have the same question, like the one I've asked you in #5473, but now about "Custom CSP" flag... how it is possible to have an add-on that uses a CSP, auto-approved?
I am very confused.

[diox]

@ValentinaPC it's not currently possible. It will be in the future though (because at some point in the near future we'll allow all webextensions to be auto-approved once they pass validation), and that's why it's one of the factors in the weight.

[ValentinaPC]

@ValentinaPC it's not currently possible.

@diox : Can you help me with what is possible to be tested from that list?
Because I've tried with eval and using content_scripts in manifest interacts again with auto-approvals (https://addons-dev.allizom.org/en-US/editors/review/webext-eval) and weight cannot be calculated.
Thanks!!!

[diox]

Using content scripts is fine, it's just content scripts for all URLs that leads to no auto-approval right now.

From the list, here is what's currently implemented:

• eval, evalInSandbox, document.write
• setInterval, and on* attributes
• Unsafe DOM: innerHTML
• Custom CSP but you can't test it with auto-approval
• Native messaging but you can't test it with auto-approval

[ValentinaPC]

Verified this on AMO-dev FF53(Win 7) and obtained the following results:

• eval - https://addons-dev.allizom.org/en-US/developers/addon/webext-eval-test/edit - no changes for weight
• document.write - https://addons-dev.allizom.org/en-US/developers/addon/webext-eval-test/edit - no changes for weight

@diox : I guess that something happened while working on https://github.com/mozilla/addons-server/issues/5520 in what concerns the weight values.

[diox]

The linter is not returning what I expect. Re-opening.

[diox]

Depends on https://github.com/mozilla/addons-linter/issues/1355

[diox]

668f524afd33c7df9c909ce085d36917f27eef20 should have fixed that. Note that you'll need to upload new versions of your add-ons so that they're validated again. I did that for Webext-eval-test already, but feel free to do it again :)

[ValentinaPC]

Verified all 7 scenarios using AMO-dev FF54(Win 7) and obtained the following results:
1. eval - https://addons-dev.allizom.org/en-US/developers/addon/webext-eval-harm/edit
- warning at validation

- weight incremented with 20

1. evalInSandbox - https://addons-dev.allizom.org/en-US/developers/addon/webext-evalinsandbox/edit
2. no warning at validation
   

3. weight not incremented

4. document.write - https://addons-dev.allizom.org/en-US/developers/addon/webext-docwrite/edit

5. warning at validation
   

6. weight not incremented

7. setInterval - https://addons-dev.allizom.org/en-US/developers/addon/webext-setinterval/edit

8. no warning at validation
   

9. weight not incremented

10. innerHTML - https://addons-dev.allizom.org/en-US/developers/addon/webext-innerhtml/edit

11. warning at validation
    

12. weight incremented with 20

13. Custom CSP - https://addons-dev.allizom.org/en-US/developers/addon/webext-csp/edit

14. warning at validation
    

15. weight is incremented with 30

16. Native messaging - https://addons-dev.allizom.org/en-US/developers/addon/webext-nativeperm/edit

17. no warning at validation
18. weight is incremented with 20

Screenshots from Reviewer Tools:

@diox - What should we do about 2, 3, 4?

[diox]

I'll look into those.

[ValentinaPC]

I'll look into those.

Anything I can help, just give me a ping.
Hope the add-ons I've created are using - evalInSandbox, document.write, setInterval - correctly.
Thanks!

[diox]

Regarding setInterval, it's because your add-on was doing a setInterval() call with a function, which is not dangerous. It's only setInterval("some string") that is triggers the implied eval warning and weight, so that seems to behaves as expected.

The other points are valid, I'll fix them. (evalInSandbox might require a linter change)

Edit: Actually, evalInSandbox is not available for webextensions so that can be ignored. The only thing left is document.write().

[ValentinaPC]

Submitted the webextension (using document.write()) https://addons-dev.allizom.org/en-US/editors/review/webext-docwrite2 and got auto-approved after few minutes, but no trace of this webext in auto approved queue.... should I wait longer?
Thanks!

[diox]

I do see it in the list, it has a weight of 60.

[ValentinaPC]

Yes, seems like document.write() is working as expected.
Postfix screenshot:

@diox But, I am not able to obtain a correct weight for a webextension that uses setInterval("some string")
Created:

• https://addons-dev.allizom.org/en-US/editors/review/webext-setinterval5
• https://addons-dev.allizom.org/en-US/editors/review/webext-setinterval4
  If are not created correctly could you give me a hand? maybe an example.
  Thanks!

[diox]

In both these extensions setInterval() seems to be used with a function and not a string:

• https://addons-dev.allizom.org/en-US/firefox/files/browse/253338/file/enable.js#L5

setInterval(function () {
        cur = div + 1;
        $('#records_crawled').text(cur);
        div = cur;
        console.log(div);
    }, 2000);

• https://addons-dev.allizom.org/en-US/firefox/files/browse/253337/file/enable.js#L28

setInterval(randomQuote, 10000);

Instead you want something like setInterval("randomQuote()", 10000) for instance.

[ValentinaPC]

So.... I've tried using your example and I have two news: a bad one and a good one.

1. The good one is that a warning was generated while submitting a webextesion that uses setInterval
   
2. The bad one is that the weight is not incremented with 5
   

[diox]

Sigh, I suck.

[ValentinaPC]

Sigh, I suck.

Be positive! :)

[ValentinaPC]

Verified as fixed on AMO-dev FF54(Win 7). Using setInterval increments weight with 5, as expected.
Postfix screenshot:

All possible scenarios were checked and this bug can be marked as verified-fixed.