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

fix: Replace update notifier with simplified deps #2033

Merged
merged 7 commits into from
Jul 5, 2022

Conversation

alexbrazier
Copy link
Contributor

@alexbrazier alexbrazier commented Jun 24, 2022

Demo

image

@netlify
Copy link

netlify bot commented Jun 24, 2022

Deploy Preview for nodemon ready!

Name Link
🔨 Latest commit 65432a6
🔍 Latest deploy log https://app.netlify.com/sites/nodemon/deploys/62bafb3c30e36400090079c9
😎 Deploy Preview https://deploy-preview-2033--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 settings.

@remy
Copy link
Owner

remy commented Jun 24, 2022

this is good, but I'm wary your windows test didn't pass: https://github.com/alexbrazier/simple-update-notifier/runs/7033149767?check_suite_focus=true

@alexbrazier
Copy link
Contributor Author

alexbrazier commented Jun 24, 2022

this is good, but I'm wary your windows test didn't pass: https://github.com/alexbrazier/simple-update-notifier/runs/7033149767?check_suite_focus=true

@remy thanks for looking into it! I actually force pushed that update as the isTTY check was causing the update checker not to run on CI so had to update a flag to force it to run during the tests. That is original test run from the old commit and the latest one has all three passing https://github.com/alexbrazier/simple-update-notifier/actions/runs/2554631152

Edit: Looks like I was looking at the wrong build you referenced - that failure was an old one caused by the single quotes that Windows wasn't liking and has since been fixed

@zorn-v
Copy link

zorn-v commented Jun 26, 2022

Seems it is not compatible with node 8.10.0 (minimum required version for nodemon) because for await
https://github.com/alexbrazier/simple-update-notifier/blob/91085738fbd27138dc510069d663e013e81af1eb/src/getDistVersion.ts#L10

@alexbrazier
Copy link
Contributor Author

Seems it is not compatible with node 8.10.0 (minimum required version for nodemon) because for await
https://github.com/alexbrazier/simple-update-notifier/blob/91085738fbd27138dc510069d663e013e81af1eb/src/getDistVersion.ts#L10

Thanks @zorn-v, I've now pushed an update to fix that

@remy
Copy link
Owner

remy commented Jun 26, 2022

@alexbrazier worth wrapping this in a try/catch - https://github.com/alexbrazier/simple-update-notifier/blob/master/src/getDistVersion.ts#L13= - I can't remember what happens if this fails inside of the deep event handler - might be worth injecting some bad code to see how it behaves.

@alexbrazier
Copy link
Contributor Author

@alexbrazier worth wrapping this in a try/catch - https://github.com/alexbrazier/simple-update-notifier/blob/master/src/getDistVersion.ts#L13= - I can't remember what happens if this fails inside of the deep event handler - might be worth injecting some bad code to see how it behaves.

Sure, something like this? https://github.com/alexbrazier/simple-update-notifier/pull/2/files

@MaximMaximS
Copy link

MaximMaximS commented Jun 27, 2022

Tbh @remy, you should probably choose this or #2035, because it is pretty bad to have a random vulnerability in a project, which is used by a lot of people. When using npm, the alert is kinda discouraging.

@oldium
Copy link

oldium commented Jun 27, 2022

@alexbrazier have you thought about using semver module instead of implementing your own version check?

@alexbrazier
Copy link
Contributor Author

@alexbrazier have you thought about using semver module instead of implementing your own version check?

I was originally trying to keep it simple but now the version check has become more complicated than expected and still not perfect so happy to swap it out for semver if that is preferred?

@remy
Copy link
Owner

remy commented Jun 27, 2022

Tbh @remy, you should probably choose this or #2035,

I'm letting @alexbrazier have some time to settle the code base. It's not a straight up simple change and a few extra days to help iron out issues will be worthwhile in the long run.

Plus, the reality of the vuln in nodemon has near zero impact. Of course it's not zero, but there's no path to exploit the actual vuln through nodemon (partly because it's fired outside of calling your code - which would take advantage of the vuln, and partly because nodemon in a lot of cases forks the sub process, so it never has access to any of the nodemon code - and thusly the vuln on got).

The issue isn't being ignored, @alexbrazier has kindly and valiantly taken up the mantle to solve this dependency once and for all.

@alexbrazier
Copy link
Contributor Author

Thanks @remy!

As the last couple of changes have been around some issues with the semver comparison and there were still some edge cases I've decided to just switch to using semver to make it more reliable. Should hopefully be good to go now unless anyone has any other comments.

@zorn-v
Copy link

zorn-v commented Jun 28, 2022

semver 7.3.7 has

  "engines": {
    "node": ">=10"
  }

There is "semver": "^5.7.1" in nodemon deps

@alexbrazier
Copy link
Contributor Author

semver 7.3.7 has

  "engines": {
    "node": ">=10"
  }

There is "semver": "^5.7.1" in nodemon deps

Hmm, I've tested with node 8.10.0 and it seems to work as expected:

image

@remy
Copy link
Owner

remy commented Jun 28, 2022

semver 7.3.7 has

  "engines": {
    "node": ">=10"
  }

There is "semver": "^5.7.1" in nodemon deps

Hmm, I've tested with node 8.10.0 and it seems to work as expected:

image

Are there any warnings during install if you're using node 8? It could be just that.

@alexbrazier
Copy link
Contributor Author

Are there any warnings during install if you're using node 8? It could be just that.

Nothing when installing with npm, but seems there is an issue with yarn
image

@Axent96
Copy link

Axent96 commented Jun 28, 2022

related to #2039 & #2040

@zorn-v
Copy link

zorn-v commented Jun 28, 2022

Are there any warnings during install if you're using node 8? It could be just that.

Nothing when installing with npm, but seems there is an issue with yarn image

You can use ~7.0.0 to avoid then

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