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

fix: Make sure Xpi class detects and throws an explicit error on invalid chars in zip entries filenames. #190

Conversation

rpl
Copy link
Member

@rpl rpl commented Sep 29, 2021

  • Enable yauzl strictFileNames option to detect invalid characters (but still require yauzl to send us zip entries filenames and content as strings, to keep that part unchanged)
  • Define more explicit Error classes to detect the two error conditions we explicitly expect and want to customize addons-linter errors for without resorting to expect that the error type has to be part of the error message (so that the addons-linter will be able to check the error.name property instead)
  • Add test coverage for the zip files with an invalid chars (in particular the '\' used as a separator)

NOTE:

@codecov
Copy link

codecov bot commented Sep 29, 2021

Codecov Report

Merging #190 (e2e673e) into master (fd91bdf) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #190      +/-   ##
==========================================
+ Coverage   97.37%   97.42%   +0.05%     
==========================================
  Files          10       11       +1     
  Lines         419      428       +9     
  Branches       90       90              
==========================================
+ Hits          408      417       +9     
  Misses         11       11              
Impacted Files Coverage Δ
src/errors.ts 100.00% <100.00%> (ø)
src/io/xpi.ts 96.00% <100.00%> (+0.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd91bdf...e2e673e. Read the comment docs.

Copy link
Member

@willdurand willdurand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+wc, thanks

src/io/xpi.ts Show resolved Hide resolved
@rpl rpl force-pushed the fix/throw-an-error-on-invalid-chars-in-zip-filenames branch from f05e950 to c66e871 Compare September 29, 2021 19:08
@willdurand
Copy link
Member

@rpl are we ready to merge this patch and release addons-scanner-utils v5?

@rpl
Copy link
Member Author

rpl commented Oct 4, 2021

@rpl are we ready to merge this patch and release addons-scanner-utils v5?

yes, based on what we discussed last week, this should be ready to merge as is (and the linter PR is the part of this change that needs some more tweaks).

@willdurand
Copy link
Member

this should be ready to merge as is

Are you able to merge PRs in this repo?

@rpl
Copy link
Member Author

rpl commented Oct 4, 2021

@willdurand nope, it looks that my github permissions associated to this repo do not allow me to merge PRs at the moment.

@willdurand
Copy link
Member

@willdurand nope, it looks that my github permissions associated to this repo do not allow me to merge PRs at the moment.

ok, that explains why you didn't merge this PR before I suppose. Sorry for the confusion.

I'll merge this PR myself and cut a new release.

(FWIW, you probably have "triage" permission on this repo because we try not to give "write" or more to everyone)

@willdurand willdurand merged commit 661d216 into mozilla:master Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants