[Static Themes] Unsupported image formats are not detected by the linter at static theme submission

[AlexandraMoga]

STR:

1. Log in to AMO -dev
2. Submit a static theme and make sure to incude an unsupported image format under 'images' in your manifest - i.e. a .tif file
3. Observe the validation results

Actual result:
There are no errors generated when a static theme package contains an unsupported image format

Expected result:
The linter should detect when an image format doesn't match the static theme requirements specified in the guideline - ST guideline

Notes:

• when a theme with an unsupported image file is submitted, a preview will be generated but the image file can't be inspected in Rev Tools and the image is not applied in the browser when the theme is installed
• here is an example:
  Rev Tools
  https://reviewers.addons-dev.allizom.org/en-US/reviewers/review/st-theme-zip-a
  Listing page
  https://addons-dev.allizom.org/en-US/firefox/addon/st-theme-zip-a/
• reproduced on AMO -dev with FF60, WIn10x64

[eviljeff]

see https://bugzilla.mozilla.org/show_bug.cgi?id=1500501

[eviljeff]

From #2243 (comment)

There is already validation for the icons property happening in src/parsers/manifestjson.js -> validateIcon which also validates the images by actually opening the first part of them and reading their size.

So that functionality can be re-used for this purpose. See validateIcons for a bit more information on this, all that should be generalized into a validateImage function that has the ability to check arbitrary sizes (for icons) as well.

This may not necessarily a good first bug but given that most of the functionality is already there and needs to be restructured it may as well be.

[muffinresearch]

@rpl hi - would you have the bandwidth to help out with this?

[AlexandraMoga]

@rpl Hi, I guess this can be tested when a new linter version will be released. Do you know when this is going to happen?
Thanks

[rpl]

@AlexandraMoga I just created a new release 1.4.0 and the new version should be available on npm pretty soon, I briefly chatted about this with @eviljeff over IRC and you should be able to test the new linting rules once the new addons-linter release is going to be part of the next deploy.

[eviljeff]

addons-linter has been updated on addons-server, so it's available on -dev.

[AlexandraMoga]

I have tried various static theme submissions, both valid and invalid and the linter was triggering the expected validation results:

[1] error thrown for missing background images, wrong or missing extensions

[2] error thrown for missing header images, wrong or missing extensions

[3] error thrown for unsupported files added as header or background images

@eviljeff , @jvillalobos

Should we worry about unnecessary files - i.e. a .js file intentionally or unintentionally added inside a static theme package?
I know that for other extension types, the linter will raise a warning for such files and will bring the extension to the reviewers' attention.
However, static theme review pages will only show the images included inside the package, which means that other files are left undetected.

Here is an example:
https://reviewers.addons-dev.allizom.org/en-US/reviewers/review/st-size5656

[jvillalobos]

Because of the add-on type, Firefox won't run any JS files, so any attack should be fruitless. However, if people are trying to upload malicious themes, it's best that we know or don't allow it. So, I agree that they should be detected.

[AlexandraMoga]

I have filed #2282 as a follow up and I will mark this issue as verified fixed on AMo -dev with FF63, Win10x64