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: update lodash to 4.17.11 to fix vulenrabilities #473

Closed
wants to merge 5 commits into from

Conversation

kzmv
Copy link

@kzmv kzmv commented Oct 25, 2018

Update lodash to fix vulnerabilities.

Description

Updated lodash to 4.17.11 to fix proto buffer vulnerabilities.

I had issues committing due to npx xo throwing errors for unrelated to the changes issues so I used --no-verify on commit.

Motivation and Context

Usage examples

How Has This Been Tested?

Ran tests. There were 4 tests failing when the branch was checked out and they continue to fail. All other tests pass after lodash import paths were refactored.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@kzmv
Copy link
Author

kzmv commented Oct 25, 2018

The build isn't passing. I will take a look in the morning

@byCedric
Copy link
Member

byCedric commented Oct 26, 2018

Hi George! Thanks for the security patch. I think your commit should start with fix( instead of bug(. After all, you are describing what happens when the commit is merged, and I don't think you are adding bugs 😉 I also think the scope should be either one of the sub-packages name or none. But those can be changed later!

Could you explain why you switched to a full lodash import everywhere, instead of partial (only what's actually used) imports? As far as I can tell, the vulnerability only exists in 3 methods of lodash, right?

... vulnerability via defaultsDeep, merge, and mergeWith function. ...

I'll try to do a full security check this evening (CEST) too 😄

@kzmv
Copy link
Author

kzmv commented Oct 26, 2018

Hey Cedric.
Lodash hasn't maintained the modularized versions of the library for 2 years. The single library is the direction the lodash maintainers have taken, in fact in v5 I understand they'll remove submodules entirely (everything will import from the root namespace) and depend on babel and webpack wizardry to treeshake.
https://github.com/lodash/lodash/wiki/Roadmap#v500-2018

If lodash has to be upgraded for some of the libraries we might as well upgrade the rest, I believe.

@byCedric
Copy link
Member

Ah I see, that's a really good explanation 😄 So this PR is only for updating Lodash across all packages right? (And with that, fix some security issues related to lodash too)

For @marionebl, this is what they say in their Roadmap.

Discontinue per method packages in favor of modular lodash

@kzmv
Copy link
Author

kzmv commented Oct 26, 2018

Correct, all I'm doing is bumping versions and changing import paths.

I'm having issues with npx xo resolving locally the package libraries. I don't have much experience with npx or xo so I'm not sure what is driving their validation. Tests seem to be passing though. Any advice?

@byCedric
Copy link
Member

I will take a look this evening (CEST) 😄 I'll get back at you about xo and npx!

@kzmv
Copy link
Author

kzmv commented Oct 26, 2018

I fond the issue. I had some stale lodash. packages and tests were running because of that. Paths are wrong so I will be updating the request later today.

@byCedric
Copy link
Member

Hi George! Great job in finding out about the issue 😄 Do you still need some help with this PR? I did a full audit for all other (sub)packages and created a PR to fix 2/3 issues. Thanks for helping keep Commitlint safe! 😄

@kzmv
Copy link
Author

kzmv commented Oct 27, 2018

Sorry didn't have time yesterday. I will be taking a look in a bit and committing the fixes on my side.

@byCedric
Copy link
Member

No worries, whenever you have time! If you are stuck or want someone to check, let me know 😄

@kzmv
Copy link
Author

kzmv commented Oct 27, 2018

The build is crashing due to npx xo validation but I don't believe I've introduced this, yet it passes for your pull request, any idea why?

@@ -72,6 +72,7 @@
"@commitlint/load": "^7.2.1",
"babel-runtime": "^6.23.0",
"chalk": "^2.0.0",
"lodash": "^4.17.11",
"lodash.camelcase": "4.3.0",
Copy link
Member

Choose a reason for hiding this comment

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

Also some old lodash modules right? 😄

@@ -75,6 +75,7 @@
},
"dependencies": {
"babel-runtime": "6.26.0",
"lodash": "^4.17.11",
"lodash.merge": "4.6.1",
Copy link
Member

Choose a reason for hiding this comment

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

Let's burn the lodash modules! 😬

@@ -73,6 +73,7 @@
},
"dependencies": {
"babel-runtime": "^6.23.0",
"chalk": "^2.0.1"
"chalk": "^2.0.1",
"lodash": "^4.17.11"
Copy link
Member

Choose a reason for hiding this comment

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

In this package, lodash is used as devdependency. I think it's best to keep it there (and remove the old lodash.includes of course 😄

@@ -78,6 +78,7 @@
"@commitlint/ensure": "^7.2.0",
"@commitlint/message": "^7.1.2",
"@commitlint/to-lines": "^7.1.2",
"babel-runtime": "^6.23.0"
"babel-runtime": "^6.23.0",
"lodash": "^4.17.11"
Copy link
Member

Choose a reason for hiding this comment

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

Same here, I found lodash.values in the dev-dependencies. I would say, remove that one and move lodash to devdependencies 😄

@byCedric
Copy link
Member

byCedric commented Oct 27, 2018

Sorry, it took me a while! I'm not sure why it's not popping up in my PR. Maybe xo only checks edited packages. I think this is something for @marionebl, we either need to ignore the function with xo or fix it. But fixing it requires a lot of refactoring 😅 Let's leave it like this until he comes back from his holiday.

Meanwhile, I also double checked your changes and started a review. Found some old modules in some package files, if you can clean it up we only have to fix the xo issue then! 😄 It will be easier for Mario to check and take action then 😄

Again, thanks a lot! <3

@kzmv
Copy link
Author

kzmv commented Nov 7, 2018

I think the packages look good at the moment but the build is failing due to the commitlint check and the xo check.

I'm happy to close this request and create a new one if needed but I'm not sure if that is going to solve the xo checks.

@marionebl marionebl changed the title bug(commitlint): update lodash to 4.17.11 to fix vulenrabilities fix: update lodash to 4.17.11 to fix vulenrabilities Nov 25, 2018
@marionebl
Copy link
Contributor

Follow up at #497, thank you folks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants