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

zipObjectDeep: fix prototype pollution #4759

Merged
merged 1 commit into from Jul 2, 2020

Conversation

@JackuB
Copy link

@JackuB JackuB commented May 7, 2020

Following PR #4745 and this comment #4744 (comment).

Here is the patch for prototype pollution in zipObjectDeep.

@jsf-clabot
Copy link

@jsf-clabot jsf-clabot commented May 7, 2020

CLA assistant check
All committers have signed the CLA.

lodash.js Outdated Show resolved Hide resolved
@JackuB JackuB force-pushed the snyk-labs:patch-SNYK-JS-LODASH-567746 branch from 7816228 to 787b42b May 11, 2020
@mdeknowis
Copy link

@mdeknowis mdeknowis commented May 12, 2020

Is this PR the "better" fix? #4745

@phoydar
Copy link

@phoydar phoydar commented May 12, 2020

Would love to see this merged soon. My company is getting hit with security vulnerability notifications because of this issue and it would be a large effort to manually patch in all of our repos.

@falsyvalues falsyvalues mentioned this pull request May 12, 2020
@shura-sparrow
Copy link

@shura-sparrow shura-sparrow commented May 15, 2020

Please, merge this soon, our project suffer terribly :(

@dariadomagala
Copy link

@dariadomagala dariadomagala commented May 18, 2020

Hello team, do you plan to merge it anytime soon? It's really blocking us.

@n0fixedab0de
Copy link

@n0fixedab0de n0fixedab0de commented May 18, 2020

Heya, desperate for a fix too. Release is about to be blocked without it.

@deepikascetest
Copy link

@deepikascetest deepikascetest commented May 18, 2020

Is this going to be merged soon and new release will happen? We are blocked by this vulnerability.

@pratikgolchha
Copy link

@pratikgolchha pratikgolchha commented May 19, 2020

Please merge this fix asap. We need this patch.

@rahuldev247
Copy link

@rahuldev247 rahuldev247 commented May 20, 2020

Please merge this fix ASAP!! there are a lot of companies getting security alerts due to this.

@YanVulich
Copy link

@YanVulich YanVulich commented May 20, 2020

Any updates when it will be merged?

@steelscott
Copy link

@steelscott steelscott commented May 20, 2020

Does anyone know if this project is still being maintained? Discussion like the above popping up about multiple security issues, plus the absence of releases for nearly a year, have me wondering if this project is actually defunct with none of us being let in on the secret.

@kelset
Copy link

@kelset kelset commented May 21, 2020

folks, I would recommend you relax: the library is clearly not dead (there have been commits in the last few weeks), and the main maintainer has replied to the other PR on this topic less than a month ago -> #4745 (comment)

That said, while I'm also affected by the security alert and I'm frustrated that Lodash didn't have a new release in a while, we are all living in covid-19 times and this can take a toll on how much time people can dedicated to OSS.

I can assure you that seeing comments like the ones above are real turnoffs for any open source maintainer so please try to be kind and comprehensive. If this issue is bothering you all so much just fork and apply the patch in your branch until a new release is out. Or ask what you can do to help.

@steelscott
Copy link

@steelscott steelscott commented May 21, 2020

@kelset Thank you for your reply. I'm not looking to get into a big argument or anything, as I greatly appreciate all the work the many contributors have put into Lodash, and we're all on edge from the current state of the world. However, I think your assessment of the situation is missing a couple of things: This has been an issue even before this current PR and before Covid-19, and plenty of people have been volunteering to help. If there is anything specific people need to help with more, please tell us what it is.

@nickswope expressed it very well in this much-upvoted post from earlier in the year regarding another security issue: #4518 (comment)

I won't have anything else to say on this topic, as my goal isn't to harp on anything or to make anyone feel bad, but please understand some people are starting to lose confidence in the project. Unfortunately, because it is a dependency of practically everything, there is little recourse other than everyone forking for patches and adding extra processes to their workflow.

But in the end, it's "just" software, and I hope everyone is well. :)

@kelset
Copy link

@kelset kelset commented May 22, 2020

If there is anything specific people need to help with more, please tell us what it is.

I'm in your same position, I'm not a maintainer for this project - but I'm an OSS maintainer for other projects so I know how it feels. If you have time and/or energy to dedicate maybe something to do would be to reach out directly to the actual maintainers and ask what can be done. I was planning to do so the other day but I'm actually already too busy to take this up 😓

Neumann-Nils added a commit to Neumann-Nils/lodash that referenced this pull request May 25, 2020
Following the approach in lodash#4759
@Soontao
Copy link

@Soontao Soontao commented May 25, 2020

Hello @jdalton

I'm sorry to contact you directly, but I believe you are the owner of this repository.

This issue is a bug-fix of a HIGH level vulnerability, It seems to have been reviewed and passed the unit test, but no person to merge it & publish a patch version, I'm not sure what happened, maybe maintainers have some internal discussion ? And, if you are busy on something, who are the leader of this project now ?

Thanks

@rajivshah3
Copy link

@rajivshah3 rajivshah3 commented Jun 1, 2020

@falsyvalues I noticed you're also a member of the lodash org, is there anything you can do to expedite the review of this PR? If you don't have time please let me know if I can help in any way.

@falsyvalues
Copy link
Member

@falsyvalues falsyvalues commented Jun 3, 2020

Hi @rajivshah3!
While this looks good to me, I'm not the one who is responsible for release process.

@adelyafatykhova
Copy link

@adelyafatykhova adelyafatykhova commented Jun 4, 2020

Chiming in with how important this is to be fixed, we have a strict security validation process for our release that will start a week from now and if it's not fixed we need to replace lodash.

@lodash lodash deleted a comment from martinoppitz Jun 4, 2020
@matthieu-foucault
Copy link

@matthieu-foucault matthieu-foucault commented Jul 2, 2020

Following up on workaround to make yarn audit pass. I forked the snyk-labs repo and added a yarn resolution pointing to this PR's branch.
@rally25rs I think that there is a caveat with the step of "bumping the version in the package.json": bumping the version number to 4.17.16 did not work for me, but setting it to 4.17.16-rc.1 did. I suspect that dependencies with prerelease versions (-rc, -alpha, etc) are ignored when running yarn audit. If anyone has the time to confirm that behavior that would be great!

@rally25rs
Copy link

@rally25rs rally25rs commented Jul 2, 2020

@matthieu-foucault under the hood yarn audit takes the package info and sends it to the audit http endpoint on the npm registry. It could be the case that npm ignores non-production versioning. When I tried it earlier I used 4.17.16-alpha.1

matthieu-foucault added a commit to bcgov/cas-ciip-portal that referenced this pull request Jul 2, 2020
Update resolution to point to the next patch version
once lodash/lodash#4759 is merged
@jdalton jdalton merged commit c84fe82 into lodash:4.17.15-post Jul 2, 2020
1 check passed
1 check passed
licence/cla Contributor License Agreement is signed.
Details
pxwise added a commit to pxwise/lodash that referenced this pull request Jul 2, 2020
@NomaanAhmed
Copy link

@NomaanAhmed NomaanAhmed commented Jul 3, 2020

Really great that this PR was merged - any timeline on when a new release will be published on npm?

@bnjmnt4n bnjmnt4n added the bug label Jul 3, 2020
yumetodo added a commit to yumetodo/google-photos-album-image-url-fetch that referenced this pull request Jul 5, 2020
note: npm will report lodash has prototype pollution

That is already fixed at lodash/lodash#4759
I'm waiting next release of lodash.
thetombomb91 added a commit to thetombomb91/UserViewer that referenced this pull request Jul 5, 2020
@neagle
Copy link

@neagle neagle commented Jul 6, 2020

I certainly don't mean to derail this thread, but I wonder if others might be having trouble applying the workaround that @rally25rs very helpfully posted above. When I add the resolutions key suggested above pointing to either a forked version of lodash with the version number bumped, or just to this branch (now that the PR has been merged), I get an error when I try to build from babel, which I'm guessing is just the first dependency I have trying to use lodash, and probably means that nothing is getting lodash.

My package.json snippet:

  "resolutions": {
    "**/lodash": "lodash/lodash#4.17.15-post"
  },

My error:

Error: Cannot find module 'lodash/uniq'
Require stack:
- /app/node_modules/@babel/cli/lib/babel/options.js
...

It looks like there might be action on getting this published to npm soon, in which case this workaround was unnecessary, but I was wondering if anyone who has the workaround working for transitive deps might be able to point out what I'm doing wrong, or share how they got it to work. Thanks!

@matthieu-foucault
Copy link

@matthieu-foucault matthieu-foucault commented Jul 7, 2020

@neagle I ended up with the same issue. I suspect that lodash needs to be packaged in some way for any lodash/* import to be available. We decided to ignore our failing audit for now and wait to see if the new version gets published soon. You could try to publish it to npm under your own scope and edit the resolution accordingly. That might fix it.

@JackuB JackuB deleted the snyk-labs:patch-SNYK-JS-LODASH-567746 branch Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.