Remove lodash dependency; fix VS build config #5
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi, could we merge this PR? Lodash 3 is vulnerable and triggering alarms in scanning tools. |
Author
|
For what it's worth, I ended up creating my own package. It might be of use to you. Here it is: https://www.npmjs.com/package/file-icon-info |
|
Thanks for your reply @avocadianmage , could you please confirm if they are fully compatible? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The primary reason I'm making this pull request is that this package pulls in lodash as a dependency, and the particular version it pulls in has security vulnerability CVE-2018-3721. While from a practical standpoint, this is unlikely to be exploited in the context of the icon-extractor package, it also produces annoying NPM warnings and github alerts when attempting to commit code with icon-extractor as a dependency.
My original intent was to simply commit a fix updating the dependency to the latest version of lodash. However, after a quick look at the code, I found that lodash was only being used a couple of times for some fairly straightforward looping and string checking, so I just removed the package as a dependency and replaced these two calls with standard Javascript.
In the process of testing my changes locally, I ran into a couple bumps compiling and running the C# project which would have required manual file manipulation to fix. So to save time if someone else is cloning this repo, I've also fixed these issues:
copycommand in the post-build actions doesn't create new directories in the path (ex:bin, which doesn't exist in the repo) and thus fails.Newtonsoft.Json.dllisn't also copied, the executable won't run from that location.I just eliminated the post-build action, and set the output path of the project to the appropriate directory, which fixes both of these issues.
Finally, I updated .gitignore to the standard recommended for C# projects, and removed everything else (such as
node_modulessince there's no dependencies anymore). Also committingpackage-lock.jsonas standardly advised.Would love to get this merged in and published, so I can keep using it. It's been a helpful package and I'd rather not completely fork it off if we can keep things consolidated here. Cheers.