Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added check to make sure icon has a valid extension #1558

Merged
merged 11 commits into from Sep 28, 2017

Conversation

Projects
None yet
3 participants
@ravneette
Copy link
Contributor

commented Sep 22, 2017

Fixes #1328

  • This PR relates to an existing open issue and there are no existing
    PRs open for the same issue.
  • Add Fixes #ISSUENUM at the top of your PR.
  • Add a description of the the changes introduced in this PR.
  • The change has been successfully run locally.
  • Add tests to cover the changes added in this PR.

Added a check to make sure that the icon has a valid extension. Gives a warning in case the extension of the image doesn't match a valid image format.
The valid extensions included for now are : jpg, jpeg, webp, giff, png, svg

@coveralls

This comment has been minimized.

Copy link

commented Sep 22, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 231f6cc on ravneette:icon-extension into 0088789 on mozilla:master.

1 similar comment
@coveralls

This comment has been minimized.

Copy link

commented Sep 22, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 231f6cc on ravneette:icon-extension into 0088789 on mozilla:master.

@EnTeQuAk

This comment has been minimized.

Copy link
Member

commented Sep 22, 2017

Thanks @ravneette for that patch!

For some reason I thought I answered to #1328 (comment) but apparently I didn't so here is what I wanted to comment on that issue:

I do think eventually we should verify that the image can be opened and check for potential image sizes. I think looking at mstriemer@5cbdaae#diff-baa432d9354e3de899d28a2638d1499eR31 and the patch in general for inspiration is a great idea.

Otherwise this is a great start and we can improve on that in a separate patch so let's start with a short review for this patch:

  • You added jpg, png and svg to the list of allowed files. Let's maybe add at least jpeg (as an often used alias for jpg), webp and giff to the list. I don't know yet what Firefox supports but we can figure that out somewhere to make sure we don't allow anything Firefox doesn't support (e.g tiff images are where I'm not so sure)
  • It's best to compare a lower cased file extension to the list to make sure we match all file names, lower or upper cased
  • This obviously needs tests but I think you're already working on them?

Otherwise, keep on going!

@EnTeQuAk

This comment has been minimized.

Copy link
Member

commented Sep 22, 2017

Worth noting that someone else is working on checking the size of icon files in #1555 so it's best to try to have your work as separated as possible.

@coveralls

This comment has been minimized.

Copy link

commented Sep 22, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling a3eeaa6 on ravneette:icon-extension into 0088789 on mozilla:master.

1 similar comment
@coveralls

This comment has been minimized.

Copy link

commented Sep 22, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling a3eeaa6 on ravneette:icon-extension into 0088789 on mozilla:master.

@ravneette

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2017

Thanks for the review @EnTeQuAk
Will make sure to keep my work disjoint from #1555 .
Added the other extensions and shifted to making the comparison among lowercase strings.
Also, do you think I should add this as a warning or as an error?
Working on writing tests now.

@coveralls

This comment has been minimized.

Copy link

commented Sep 22, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 86c7355 on ravneette:icon-extension into 0088789 on mozilla:master.

@coveralls

This comment has been minimized.

Copy link

commented Sep 22, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling eac86f3 on ravneette:icon-extension into 0088789 on mozilla:master.

@EnTeQuAk

This comment has been minimized.

Copy link
Member

commented Sep 22, 2017

Will make sure to keep my work disjoint from #1555 .

Cool, thanks!

Added the other extensions and shifted to making the comparison among lowercase strings. Also, do you think I should add this as a warning or as an error?

I don't think it's worth an error yet. If we are going to check for validity of images, e.g check that they can be opened and that they represent what they should, we can start raising errors if that check fails.

But for now we are only checking file-extensions so that should not be too agressive.

@ravneette

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2017

Great!
I restricted the code to throw a warning instead of an error.

@EnTeQuAk
Copy link
Member

left a comment

Just a few more minor changes, otherwise this already looks quite good.

code: 'WRONG_ICON_EXTENSION',
legacyCode: null,
message: _('Consider Adding another icon'),
description: sprintf('Icons Should be of type jpg, jpeg, webp, giff, png or svg.'),

This comment has been minimized.

Copy link
@EnTeQuAk

EnTeQuAk Sep 25, 2017

Member

This should not use sprintf since you're not using format-strings. You can just use _('') to mark the string as translatable.

Also, I'd write something like Icons should be one of JPG, WebP, GIFF, PNG or SVG which is what we want eventually.

const json = validManifestJSON({
icons: {
32: 'icons/icon-32.txt',
64: 'icons/icon-64.html',

This comment has been minimized.

Copy link
@EnTeQuAk

EnTeQuAk Sep 25, 2017

Member

Let's add one or two valid icons so that we can verify we don't match types we're not supposed to match.

@ravneette ravneette force-pushed the ravneette:icon-extension branch from 8796ede to 92b649f Sep 25, 2017

@coveralls

This comment has been minimized.

Copy link

commented Sep 25, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 92b649f on ravneette:icon-extension into b67aaa1 on mozilla:master.

@ravneette ravneette force-pushed the ravneette:icon-extension branch from 92b649f to 1746fa4 Sep 25, 2017

@coveralls

This comment has been minimized.

Copy link

commented Sep 25, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 1746fa4 on ravneette:icon-extension into b67aaa1 on mozilla:master.

@ravneette

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2017

Hey @EnTeQuAk
Modified the test and removed sprintf

export const WRONG_ICON_EXTENSION = {
code: 'WRONG_ICON_EXTENSION',
legacyCode: null,
message: _('Consider Adding another icon'),

This comment has been minimized.

Copy link
@EnTeQuAk

EnTeQuAk Sep 26, 2017

Member

Sorry, I didn't notice this earlier. This message is prominently shown to developers so it should be more explicit.

Something along the lines of Unsupported image extension and then the description goes into the details.

Mind changing that?

@ravneette

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2017

Ya sorry my bad on that.
Should've been more explicit.
Changed it to "Unsupported image extension"

@coveralls

This comment has been minimized.

Copy link

commented Sep 26, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 219721f on ravneette:icon-extension into b67aaa1 on mozilla:master.

@ravneette ravneette force-pushed the ravneette:icon-extension branch from 219721f to 9cc8fff Sep 27, 2017

@coveralls

This comment has been minimized.

Copy link

commented Sep 27, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 9cc8fff on ravneette:icon-extension into e8200f5 on mozilla:master.

@coveralls

This comment has been minimized.

Copy link

commented Sep 27, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 815f7df on ravneette:icon-extension into e8200f5 on mozilla:master.

1 similar comment
@coveralls

This comment has been minimized.

Copy link

commented Sep 27, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 815f7df on ravneette:icon-extension into e8200f5 on mozilla:master.

@EnTeQuAk
Copy link
Member

left a comment

Looks great, thanks a lot for all that work!

I'm about to open a follow up issue that actually opens these files and checks if they're corrupt. This is a good start.

@EnTeQuAk EnTeQuAk merged commit d3e429f into mozilla:master Sep 28, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@ravneette

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2017

Hey @EnTeQuAk , thanks for the guidance.

Since you mentioned the follow up, I've been brainstorming on the same. Drawing inspiration from mstriemer's work mstriemer/addons-linter@5cbdaae , I'm using the concept of ImageMetadata errors (Here's some progress on the same https://github.com/ravneette/addons-linter/commits/detect-corrupt-icons). Do you think this will work?

@ravneette ravneette referenced this pull request Oct 2, 2017

Merged

Added check to add warning in case of corrupt image file #1581

3 of 3 tasks complete

EnTeQuAk added a commit that referenced this pull request Nov 2, 2017

Fix regression in icon validation.
Fixes #6806

* icon size and format functions did not open the correct file
  directly from the XPI
* tests didn't use the actual test-fixtures
  and thus validated the wrong thing
* file path was not correctly fowarded to warning

This was a regression from #1581 and #1558
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.