hapijs / hoek

Node utilities shared among the extended hapi universe
Other
480 stars 171 forks source link

Backport 'skip assignment to __proto__' to v2.x.x #234

Closed henrycatalinismith closed 6 years ago

henrycatalinismith commented 6 years ago

Hej hej! I've tried my hand at backporting the proto vuln fix to v2.x.x! 😇

Our project seems to have a lot of dependencies pointing at v2.x.x versions of this package so this seems like it might be the fastest way to resolve the vulnerability from our perspective! Let me know what you think though.

If you look real close at the unit test you'll see it actually makes slightly different assertions than the tests added for the v4.x.x version of the fix. For some reason I couldn't get the v4.x.x test to pass so I tweaked it slightly until it worked. Here are the assertions side-by-side so that you can easily compare.

v4.x.x v2.x.x
expect(b).to.equal({ ok: 'value' }); expect(Object.keys(b).length).to.equal(1);
expect(b.test).to.equal(undefined); expect(Object.keys(b)[0]).to.equal('ok');
expect(b.test).to.equal(undefined);

Hope this is okay but let me know if not, or if you need anything more! ❤️

henrycatalinismith commented 6 years ago

Oh BTW I've sent this PR against master because there was no v2.x.x branch in this repo that I could find so in terms of GitHub features I didn't know how else to express this request! But obviously I'm not really asking you to merge this into master!! Argh bit confusing!

Marsup commented 6 years ago

Then maybe the problem is your dependencies pointing at a 3.5 years old dependency. Can't you PR those to upgrade ?

henrycatalinismith commented 6 years ago

Sure thing! Have a great weekend 👍

scott113341 commented 6 years ago

@Marsup it is unfortunately not possible to bump the hoek version in all packages that specify hoek@2.x.x as a dependency.

This is because hoek@2.x.x specifies a node engine of >=0.10.40, whereas hoek@4.2.1 (the lowest version with the backport applied) requires node >=4.0.0. Earlier today I ran into this issue when trying to bump the hoek dependency in the hawk project as you suggested: https://github.com/hueniverse/hawk/pull/236.

I completely understand that supporting very old versions of software is not desirable. However, taking the time to apply this security backport will help keep the JS community secure, especially since this version of hoek is relied upon by many projects (for better or for worse), and updating dependencies is not always trivial or even possible.

Thank you @hnrysmth for taking the time to write and submit this PR, and thank you @Marsup for developing and maintaining this library.

kanongil commented 6 years ago

This seems redundant. If you aren’t already using node v4+, then you have bigger issues. Any package that relies on hoek should require a stable node release.

The JS community will be better served by package authors updating accordingly.

nlf commented 6 years ago

Node 0.10 is well beyond it’s end of life. Node 4 is approaching its end of life in a matter of months. I have absolutely zero interest in maintaining or patching a library for a version of node no one has any business using. The answer remains a no, if you are truly limited and cannot update past node 0.10 then you likely have larger problems than what this patch is fixing.

scott113341 commented 6 years ago

@kanongil and @nlf, thank you for taking the time to respond.

I don't disagree that Node 0.10 is most definitely end of life; that's been the case since 2016-10-31. However, this backport is NOT intended whatsoever to service people only using EOL versions of Node; this security vulnerability affects all versions of Node (including active and LTS versions) equally.

Allow me to reiterate: this backport has nothing to do with Node 0.10, but rather the difficulty of getting dozens of other projects to update their version of hoek to patched versions > 4.2.0 < 5.0.0 || >= 5.0.3. Here is an example where it is impossible to simply upgrade hoek:

Given this, I think it still may be overall better for the JS community to accept this PR to secure hoek@2.x.x; it would make it extremely easy for maintainers to eliminate this vulnerability from their packages.

I respectfully request that you reconsider your decision to not accept this PR. Thank you for your hard work developing and maintaining this library, and for taking the time out of your weekend to discuss.

nlf commented 6 years ago

I respectfully decline. The correct solution here is for hawk to release a new version using an updated hoek and for hawk’s dependents to use that updated version. I personally will not maintain a release this old, for any reason. There exists no good reason for anyone to continue using hoek@2.x. That’s my stance. I’m not changing it.

nlf commented 6 years ago

And to address your comment about what’s best for the JS community, that would without doubt be updating your dependencies. Patching a nearly 3 year old release to allow people to continue using libraries that neglect their own maintenance serves no one.

hueniverse commented 6 years ago

I have no intention of releasing old and unsupported versions of hawk. I also fully supports @nlf position here. I do not care about the overall state of the JS community. I care about people who are actively using my code in a responsible way which means, keeping up to date with no more than ONE major version behind master.

If you don't like this policy, I suggest you stop using my code. It's really that simple. The health of the ecosystem depends on people being responsible for their code, and not by putting pressure on the few that actively maintain their own work to fill in the gaps.

nlf commented 6 years ago

I’m locking this issue now so I don’t have to continue talking in circles