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

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

Merged
merged 2 commits into from Jul 7, 2023

Conversation

Primexz
Copy link
Contributor

@Primexz Primexz commented Jun 23, 2023

fix some vulns

@netlify
Copy link

netlify bot commented Jun 23, 2023

Deploy Preview for nodemon ready!

Name Link
🔨 Latest commit 3681000
🔍 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
Copy link

FBNitro commented Jun 23, 2023

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

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

This comment was marked as resolved.

@Primexz
Copy link
Contributor Author

Primexz commented Jun 26, 2023

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

remy commented Jun 26, 2023

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

wellwelwel commented Jun 26, 2023

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:

  • semver.simplifyRange
  • semver.premajor
  • semver.preminor
  • semver.prepatch
  • semver.literals

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

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.

package.json Outdated Show resolved Hide resolved
@wellwelwel
Copy link

Thanks @Primexz 🚀

package.json Outdated Show resolved Hide resolved
@Cellule
Copy link
Contributor

Cellule commented Jul 5, 2023

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

@remy remy changed the title bump simple-update-notifier & semver Address vuln in semver (bump simple-update-notifier & semver) Jul 7, 2023
@remy remy merged commit 6bb8766 into remy:main Jul 7, 2023
5 of 6 checks passed
@remy
Copy link
Owner

remy commented Jul 7, 2023

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

github-actions bot commented Jul 8, 2023

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants