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: GH-935 replaced compare-func with native sort method #937

Merged
merged 1 commit into from
Apr 1, 2020

Conversation

breautek
Copy link
Contributor

Platforms affected

Android

Motivation and Context

compare-func & its sub-dependency dot-prop are two unmaintained packages with reported vulnerabilities as documented in #935

Closes #935

Description

I removed the compare-func function and replaced its one and only usage in ProjectBuilder class using the native array sort method.

Testing

The sort method was already tested, and the new sort implementation still passes the tests.

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@breautek
Copy link
Contributor Author

In draft because there is an issue preventing tests from running successfully on NodeJS 12.

Copy link
Member

@timbru31 timbru31 left a comment

Choose a reason for hiding this comment

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

Thanks Norman, LGTM

@breautek
Copy link
Contributor Author

Thanks Norman, LGTM

Should I wait for #938 before I merge? Not sure what would be acceptable here, given the circumstances...

@breautek breautek marked this pull request as ready for review April 1, 2020 03:05
@breautek breautek closed this Apr 1, 2020
@breautek breautek reopened this Apr 1, 2020
@erisu
Copy link
Member

erisu commented Apr 1, 2020

You will need to rebase first.

@codecov-io
Copy link

Codecov Report

Merging #937 into master will increase coverage by 0.15%.
The diff coverage is 93.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #937      +/-   ##
==========================================
+ Coverage   65.27%   65.42%   +0.15%     
==========================================
  Files          21       21              
  Lines        1817     1828      +11     
==========================================
+ Hits         1186     1196      +10     
- Misses        631      632       +1     
Impacted Files Coverage Δ
...n/templates/cordova/lib/builders/ProjectBuilder.js 73.21% <93.75%> (+1.23%) ⬆️

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 fb26050...6c8bfc5. Read the comment docs.

@breautek breautek merged commit 8ab1dbc into apache:master Apr 1, 2020
raphinesse added a commit to raphinesse/cordova-android that referenced this pull request Oct 19, 2020
This reverts the fileSorter to the state from before apache#937, but using our
own simple re-implementation of `compare-func`.
raphinesse added a commit to raphinesse/cordova-android that referenced this pull request Nov 19, 2020
This reverts the fileSorter to the state from before apache#937, but using our
own simple re-implementation of `compare-func`.
raphinesse added a commit to raphinesse/cordova-android that referenced this pull request Nov 21, 2020
This reverts the fileSorter to the state from before apache#937, but using our
own simple re-implementation of `compare-func`.
raphinesse added a commit that referenced this pull request Nov 21, 2020
* refactor(ProjectBuilder): less repetitive fileSorter

This reverts the fileSorter to the state from before #937, but using our
own simple re-implementation of `compare-func`.

* fix(ProjectBuilder): apply sort RegExp to basename only

* refactor(ProjectBuilder): use fast-glob instead of hand-rolled equivalent

* refactor(ProjectBuilder): factor out common isPathArchSpecific

* refactor(ProjectBuilder): use includes instead of indexOf

* refactor(ProjectBuilder): move sorting into findOutputFilesHelper

* refactor(ProjectBuilder): simplify findOutputFiles signature
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.

Prototype pollution in dot-prop
4 participants