Handlebars.js: What is the Node.js version requirement for 4.x?

[dougwilson]

[bcoe]

@ErisDS hey👋 I'm the maintainer of yargs. I saw the recent handlebars update that migrates from optimist to yargs (which is exciting) ... but, [email protected] requires Node 6, so might be breaking for some handlebars users (based on your engines field).

~yargs@15 should work for Node.js 6 (we haven't dropped Node 8 yet).~ I'm getting my versions mixed up, it's Node 8 I'm about to drop.

What are the version support goals for handlebars, I could work with you to target a slightly older version of yargs (but can't support all the way back to 0.4).

I might just be tempted to call this a major change for people.

[nknapp]

There are actually tests that should have failed in this case, but even though I can see the error messages hidden in the Travis log, the build itself did not fail.

The test tries to use the handlebars CLI with node versions starting at 0.10.

@ErisDS I didn't see this coming. Shall we revert?

[bcoe]

@nknapp @ErisDS I think it could be fairly disruptive to folks if you don't revert, what if you reverted, and made it a major with an engines update?

You could install yargs@14 which is not vulnerable, and supports back to Node 6.... But both Node 6 and 8 are EOL, and will no longer take patches. So I would put some thought into what your version support goals are (0.4 is unlikely to be able to be targeted by a patched version of optimist or yargs without a good chunk of work).

[nknapp]

@ErisDS I have fixed the integration tests in #1669. Do you want to review?

[ErisDS]

@dougwilson thanks for bringing this to our attention ❤️ Could you explain where and why you're running against Node v6 just so we have a better understanding?

Me and @nknapp have had a chat and think that it's time for Handlebars' extensive Node compatibility to end as there aren't enough maintainers and quite frankly, I doubt there's any need for support < v6.

What I'm going to do today is update the compatibility to be minimum Node.js v6 for now.

I'll also downgrade yargs to v14 and merge @nknapp's changes to fix the tests.

This should give us a working version that supports a sensible but very wide range of Node versions and doesn't have the minimist issue.

We'll support v6 & v8 for a short while longer and determine a strategy and timeline for dropping those internally that we are comfortable with, based on any feedback from the community.

@bcoe there's no expectation on our part that you would do any extra work to continue to support us on yargs v14. We really, really appreciate you showing up here today, joining the conversation and clarifying that yargs v14 would work and is secure. It has been immensely helpful ❤️

[tamebadger]

Also came here since we are experiencing issues on node v6.. our deps:

[email protected]
│ └─┬ [email protected]
│ └─┬ [email protected]
│ └── [email protected]

Thanks for the solution you are proposing @ErisDS, that should work for us too!

[ErisDS]

The changes discussed here have been made and shipped in 4.7.5. Node.js support is now set to v6+ for 4.x, yargs is pinned to v14.

Are you still using

The idea of this change, rather than going straight to v10+ is that it should be enough to provide realistic compatibility within the constraints we have.

I'm under instruction not to do major releases, so it's this or going back to having insecure dependencies unless someone else has a bright idea.

[ErisDS]

@dougwilson I'm not unsympathetic to that at all. I'm genuinely sorry that this has caused you so much pain.

Ultimately, all this new security reporting tooling is nice but it's definitely overbearing. Pretty much every piece of software I maintain now has multiple flags against it, largely for dev dependencies that affect no one. There's months of work needed to swap dependencies like was done here and I was incredibly grateful that in this case the work was done for us. So I think our pain is coming from the same place...

With this module, we're stuck between a rock and a hard place. Do we favour the people whose builds are failing on npm audit or do we favour people whose builds are failing because we fixed the audit issue by dropping compat for (very) out-of-date software?

Also, I understood from what you were saying that v6 support was going to be enough. Again, I'm sorry that it's not enough in your case. As I said I'm pretty out of ideas for what to do.

[bcoe]

@ErisDS thanks for your hard work here ❤️

I tend to agree with @dougwilson, that it's probably worth releasing this as a 5.x, with an explicit commit that updates the engines field (like this).

truly appreciate the hard work that you're doing for the community.

[ErisDS]

Let me also clarify that I made todays changes based on the thread here, believing v6 was going to be enough for everyone who spoke up. So I feel a bit aggrieved that I'm now being called unsympathetic.

It's difficult to advocate for a major bump to drop versions of node that are 2+ years past EOL. That's not to say I don't appreciate it's jarring and a lot of work for people, I do.

It might be that downgrading yargs further or using a different dependency altogether would solve all the problems, but I only have capacity for test+merge+release.

However I think spending time doing something like that is only deferring the real work to another day. If a worse security vulnerability comes along, the same thing could happen again.

[nknapp]

We have discussed this and agreed that we want to support Node.js LTS and current versions in general and that dropping older Node.js versions should not be considered to be a breaking change.

I think it was good to raise the issue (but it wasn't me who had the extra work due to the misunderstanding, so I'm only talking for myself here). But this topic would have come up anyway at some point and at least now, we have a common point-of-view towards this. It will be documented in the README as well, when #1671 will be merged.

So, even if the solution is not the one you wished for, some good has come out of it.

[wycats]

@dougwilson you don't need to apologize for raising the issue!

As @nknapp pointed out, had you not raised it, we wouldn't have clarified our policy. Hopefully clarifying the policy will help a future person like you avoid some frustration.

Seriously, great thanks for bringing up your real-world pain and helping us work through it.

[mcollina]

I would recommend against treating bumping the supported Node.js runtime anything but a breaking change. FWIW, Node.js does not do this.

[wesleytodd]

The conversation is a bit confusing now since @dougwilson removed his messages, but I want to also add that the inability to both bump the dependencies to a version without the minimist CVE and maintain semver with regards to node version support is NOT a problem with this package, it is a problem with the reporting on the minimist dep. We should fight for removal of that CVE over breaking an important dependency's ability to follow semver properly.

EDIT: to clarify my opinion here, I mean stop reporting on the CVE. If that is by recommending folks like Doug use an exclude in their projects, or if we can get it removed from npm audit results, then go ahead and keep old node version support in this major line. I am not trying to deny that the issues should be patched in the libraries, just that the reporting here causes more harm than good.

[jasnell]

It's a bit impossible to adequately weigh in on this issue but from the conversation it would appear to be a combination of issues stemming from the minimist fiasco and figuring out which Node.js LTS version to support?

To be honest, at this point, I'd be very happy for the CVE for minimist to be revisted and revoked. That said, as @mcollina recommends, bumping the minimally supported Node.js version should always be a breaking semver-major change.

[ErisDS]

The CVE for minimist is hugely disruptive. It has flagged almost every single package I maintain whilst (to the best of my knowledge so far) being exploitable in none of them.

[ErisDS]

To be honest, at this point, I'd be very happy for the CVE for minimist to be revisted and revoked.

Is this a genuine possibility?

[jasnell]

I don't know. It is worth looking into.

[wesleytodd]

@isaacs or @darcyclarke, is there something we can do on the registry reporting side here? Assuming that revoking the actual CVE is not an option (as it is probably valid), can we stop reporting on it in npm audit? What can be fixed from this seems to have been taken care of, but continuing to report on this is just causing unnecessary pain.

[darcyclarke]

@wesleytodd we've got an "Open RFC - Deep Dive" call queued up for today at 2pm ET about "resolutions". I imagine the conversation will touch on the existing proposal for us to support resolving/ignoring audit notices as well as the broader topic of resolving peer/unlisted deps. I encourage folks to join the call if their interested or just want to sit in.

Circling back to the question speicifically though, no I don't think there's anything we'd do on the registry side to revoke a valid CVE. Again, I believe a better way forward would be to provide a mechanism for the community - especially maintainers - to ignore/whitelist their usage of a dep against unnecessary noise.

[isaacs]

This particular CVE, combined with the fact that mkdirp 0.5.1's dep on minimist was pinned to an unpatched version, and that mkdirp is so widely used, highlighted a shortcoming in the current npm audit fix capabilities, where it is unable to properly diagnose the problem as being resolvable. (In this case, npm update mkdirp --depth=9999 would fix all cases that I've seen so far.)

The good news is that npm audit in npm v7 _will_ diagnose this properly, and thus npm audit fix in npm v7 will fix the situation in this class of cases.

Another aspect to this is that npm audit fix in v6 does not automatically fix some nested metadependency issues that are defined in a package-lock.json or npm-shrinkwrap.json file, and improperly implicates top level deps that originally triggered those metadeps to be loaded, even if they did so with a version range that includes the fix. So, it looks like nyc is wrong for depending on a vulnerable version of minimist, even though it's several levels deep, and fully upgradable.

One thing we are considering is actually creating an advisory against mkdirp 0.4.1 - 0.5.1, so that users of mkdirp will see that they can update, but it's not entirely clear that this will address the issue, and implementing this "meta-advisory" in a general way can be both tricky and extremely time consuming, given the scope of our registry and the interconnectedness of the dependency graph. There are technical solutions to be able to address this more elegantly, which we'll be exploring in the near future, I'm sure.

[wesleytodd]

Since this is dying down now, and is fixed in this library via #1672, it seems fine to forego more conversation here. In the future hopefully the changes in npm will help make this not as painful of an issue!

[ErisDS]

@wesleytodd totally get not carrying on the conversation here, but I'm still fighting the minimist CVE across countless packages, and so would be interested to know where I can follow along with what happens with this.