remy / nodemon

Monitor for any changes in your node.js application and automatically restart the server - perfect for development
http://nodemon.io/
MIT License
26.21k stars 1.72k forks source link

Address vuln in semver (bump simple-update-notifier & semver) #2118

Closed Primexz closed 1 year ago

Primexz commented 1 year ago

fix some vulns

netlify[bot] commented 1 year ago

Deploy Preview for nodemon ready!

Name Link
Latest commit 3681000cffdb314b6520af0872da450066755739
Latest deploy log https://app.netlify.com/sites/nodemon/deploys/649ae37ae917860008d43ab1
Deploy Preview https://deploy-preview-2118--nodemon.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

FBNitro commented 1 year ago

Not an issue for me, but... semver 7.x needs node 10+. The nodemon package.json says: "engines": { "node": ">=8.10.0" },

Which means this could be a breaking change for 'someone' out there.

aren55555 commented 1 year ago

Support for Node 8 ended 3 years and 5 months ago (31 Dec 2019). Even support for Node 10 ended 2 years ago (30 Apr 2021). See https://endoflife.date/nodejs

The package.json should be updated to something a lot more recent, and hopefully at least in the "Security Support" phase. It doesn't do anyone any good to support versions of Node that are no longer maintained and potentially insecure.

wellwelwel commented 1 year ago

@Primexz, these changes will not fix the problem since the simple-update-notifier dependency has the same problem.

See: simple-update-notifier PR #19

The nodemon depends on this being fixed in the simple-update-notifier first, probably in a version ^2.0.0.


@aren55555, regardless of whether it's an unsupported version of NodeJS, following the release semantics, it would probably be the ideal way to release a major version of nodemon like ^3.0.0.

Then, indicate the support starting at Node 10 in: https://github.com/remy/nodemon/blob/6787871d521eef65c2bc7a62234e3736bf6fcc35/package.json#L11-L13


Related:

Primexz commented 1 year ago

Then we wait until a fix is merged into the master of simple-update-notifier. If a fix is available there we can continue with this PR.

remy commented 1 year ago

Firstly, sorry for late replies. Gmail has been slurping a lot of legit emails into spam, so I've missed a lot (including replies to emails I've sent my own family).

Regarding support for older node versions that @aren55555 mentions, I'll explain: node 8 was easy to support for a long time with little to no changes. Since I come from the web and backward compatibility is the gold standard, and when it was easy to do, I did.

I'd only drop an older version of node of there was a really good reason (like the version of node flat out couldn't support some feature).

I'm happy to drop node 8 if semver simply can't support it, but I'll only go to the lowest common version (which I'd expect is 10, but again, happy to push higher if it's required).

Some people are in companies that can't easily upgrade, and it's important to me to try to help them as long as I can.

Aside, I'm sure the author of the updater script will be happy to merge a pr - the module development was driven by stale support in the original updater and nodemon relying on it.

(Completely aside, I'd love to know what changed in semver that meant they had to drop support for node 8 - I'm guessing native await support, but I'll have to take a gander).

Thanks to everyone bringing this up.

wellwelwel commented 1 year ago

I think it's can be good:

I tested the most common methods from semver 7.5.3, then I got "breaks" on Node 8 using:

But when I use any other method, every line works fine (including the satisfies, the only method used by nodemon and gt, the only method used in simple-update-notifier).

Since the nodemon doesn't uses any method that can breaks on Node 8, I think it don't needs a break change.

@remy, I would like your opinion about that.

alexbrazier commented 1 year ago

Version 2.0.0 of simple-update-notifier has just been release which updates to the latest semver versions.

It is a major version update because it officially drops support for node 8, however it still appears to work with node 8, but you'll get an error when running yarn install on node 8.

For this PR you might also want to update the package.json to "node": ">=10" to match simple-update-notifier and semver.

Node 8 workaround

Run yarn with yarn install --ignore-engines to ignore node 8 error and it should still work. I don't think you get an error when using npm install.

wellwelwel commented 1 year ago

Thanks @Primexz 🚀

Cellule commented 1 year ago

Can we get this merged and published ? Would love to get rid of semver's vulnerability warnings in my project

remy commented 1 year ago

I've merged this - along with some other changes at the same time. Tests are doing something weird, but I've validated them manually and I'm happy.

The whole CI system is a little brittle (due to the way the tests need to constantly spawn processes), but if it releases you should see this in nodemon@3. The first major release in a long time (and hopefully the last, I really don't like breaking changes, but it absolutely makes sense to finally drop official support for node@8).

github-actions[bot] commented 1 year ago

:tada: This PR is included in version 3.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: