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

Reduce the usage of lodash (round 2) #145

Merged
merged 19 commits into from
Mar 9, 2021

Conversation

alexbjorlig
Copy link
Contributor

@alexbjorlig alexbjorlig commented Feb 12, 2021

Objective --> remove lodash security warnings

The primary objective of this PR is to remove security warnings when using the library (maybe we can setup snyk.io later to warn about this?)

What changed

Simply upgrading lodash was not easy at all, and gave a tremendous amount of errors because a lot of functions were re-named or their API was modified.

In response to this, I converted must lodash functions to just using Node.js.

However there are 2 functions left:

  • _.get()
  • _.defaultsDeep()

I think get can be easily removed, when we upgrade the repo to Node.js v14, because then we can use ?. and ?? syntax. I'm not so sure about defaultsDeep.

But I decided to finish up the PR because, hey it works and Lodash is updated to the latest version 💃🏾 🚀

Positive side-effects

I actually think the source code will be more easy to read with the reduced usage of lodash.

Closes #135
Closes #142

@alexbjorlig
Copy link
Contributor Author

alexbjorlig commented Feb 12, 2021

@addaleax I'm fighting with this one 😳

Do you know what this install error from Github Actions could indicate?

Screen Shot 2021-02-12 at 13 30 00

@alexbjorlig
Copy link
Contributor Author

I think the issues could be related to the pre-commit package, that did not get updates for 6 years 🤯. Is this package/pattern with a pre-commit something you would like to keep, or should I remove it?

@addaleax
Copy link
Contributor

I think the issues could be related to the pre-commit package, that did not get updates for 6 years exploding_head. Is this package/pattern with a pre-commit something you would like to keep, or should I remove it?

I think we want to keep it – generally, most of the @mongodb-js org’s repositories have this in one form or another. You could maybe try aligning this with https://github.com/mongodb-js/compass/blob/master/package.json in this regard? It’s not using the pre-commit package itself, so that might be something that could be replaced somehow.

@alexbjorlig
Copy link
Contributor Author

@addaleax I pushed another commit to align with the compass setup, but I get a very similar error

npm ERR! /usr/bin/git ls-remote -h -t ssh://git@github.com/mongodb-js/pre-commit.git
npm ERR! 
npm ERR! Warning: Permanently added the RSA host key for IP address '192.30.255.112' to the list of known hosts.
npm ERR! git@github.com: Permission denied (publickey).
npm ERR! fatal: Could not read from remote repository.
npm ERR! 
npm ERR! Please make sure you have the correct access rights
npm ERR! and the repository exists.

Seems like a security issue. Should the github action somehow include a token or what? (can't find a reference for such a token in the compass repo)

@addaleax
Copy link
Contributor

Hm yeah, I guess github rejects git+ssh for unknown users even if the requested resource is public? That’s weird, yup, especially considering that the Travis builds passed in the past

@alexbjorlig
Copy link
Contributor Author

alexbjorlig commented Feb 12, 2021

Hm yeah, I guess github rejects git+ssh for unknown users even if the requested resource is public? That’s weird, yup, especially considering that the Travis builds passed in the past

I "fixed" the issue by following the tip here. Found some failing tests, after upgrading the BSON library. And finally made the github action workflow pass 🚀 🔥

Sorry for the alle the files changed, but I hope it's ok to review. I will be quick to resolve any questions/comments/change requests.

Let me know if I should update anything @addaleax - so happy if we can remove all the security warnings on NPM install 🚀

@alexbjorlig
Copy link
Contributor Author

alexbjorlig commented Feb 15, 2021

Hi @addaleax - hope you had a great weekend.
When this PR is ready to merge, I would be happy to take a look at #127.

I would also like to know what you and the other maintainers think about converting the project to typescript? In my opinion it is just another factor that makes the library 5% more easy to consume (and maintain) 🧐

@addaleax addaleax merged commit 88b20c2 into mongodb-js:master Mar 9, 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.

What is the Zuul package provided in the package.json as devDependency? Security issues
2 participants