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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

henrycatalinismith
Copy link

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
Copy link
Author

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
Copy link
Contributor

Marsup commented Feb 16, 2018

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

@henrycatalinismith
Copy link
Author

Sure thing! Have a great weekend 👍

@scott113341
Copy link

scott113341 commented Feb 25, 2018

@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: mozilla/hawk#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
Copy link
Contributor

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
Copy link
Member

nlf commented Feb 25, 2018

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
Copy link

@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:

  • The 3.1.x branch of hawk is in maintenance mode, and relies on hoek@2.x.x. Normally, we could just bump that to hoek@5.x.x, but there have been breaking changes to hoek along the way. Additionally, hawk@3.1.x has declared in its engines that it is compatible with Node >=0.10.40.
  • So, a hawk maintainer's options are:
    • Release a new version of hawk@3.1.x that increases the minimum version of Node required to 4/6/8.x and bumps to hoek@4.x.x or hoek@5.x.x. However, this is extremely undesirable because it violates semver by requiring multiple major version increases in the Node runtime in a patch release.
    • OR monkey-patch/write hacks to fix the vulnerability in hoek within the hawk package. It is clearly not optimal for dozens of projects to do this.
    • OR fork the hoek project, backport the security patch, and use this new patched version. It is clearly not optimal for dozens of projects to do this.
    • OR immediately announce that hawk@3.1.x is EOL and will no longer receive security patches, including a known, publicly disclosed vulnerability. It is clearly not optimal for dozens of projects to do this.

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
Copy link
Member

nlf commented Feb 26, 2018

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
Copy link
Member

nlf commented Feb 26, 2018

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
Copy link
Contributor

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
Copy link
Member

nlf commented Feb 26, 2018

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

@hapijs hapijs locked as too heated and limited conversation to collaborators Feb 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants