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

Consider removing/replacing update-notifier #1961

Closed
Labels

Comments

@abalabahaha
Copy link

This is the dependency tree I got from installing nodemon in an empty project:

└─┬ [email protected]
  ├─┬ [email protected]
  │ ├─┬ [email protected]
  │ │ ├── [email protected] deduped
  │ │ └── [email protected]
  │ ├─┬ [email protected]
  │ │ └─┬ [email protected]
  │ │   └─┬ [email protected]
  │ │     └── [email protected]
  │ ├── [email protected]
  │ ├─┬ [email protected]
  │ │ └── [email protected] deduped
  │ ├─┬ [email protected]
  │ │ └── [email protected]
  │ ├─┬ [email protected]
  │ │ └── [email protected]
  │ ├── [email protected]
  │ └─┬ [email protected]
  │   └── [email protected] deduped
  ├─┬ [email protected]
  │ └── [email protected]
  ├── [email protected]
  ├─┬ [email protected]
  │ └─┬ [email protected]
  │   ├── [email protected]
  │   └── [email protected]
  ├── [email protected]
  ├── [email protected]
  ├─┬ [email protected]
  │ └── [email protected]
  ├─┬ [email protected]
  │ └─┬ [email protected]
  │   └── [email protected]
  ├── [email protected]
  └─┬ [email protected]
    ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ │ └── [email protected] deduped
    │ ├── [email protected]
    │ ├── [email protected] deduped
    │ ├── [email protected]
    │ ├─┬ [email protected]
    │ │ ├── [email protected]
    │ │ ├── [email protected]
    │ │ └─┬ [email protected]
    │ │   └── [email protected]
    │ ├── [email protected]
    │ ├─┬ [email protected]
    │ │ └── [email protected] deduped
    │ └─┬ [email protected]
    │   ├── [email protected] deduped
    │   ├── [email protected] deduped
    │   └── [email protected] deduped
    ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ │ └─┬ [email protected]
    │ │   └── [email protected]
    │ └─┬ [email protected]
    │   └── [email protected]
    ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ │ └── [email protected]
    │ ├── [email protected]
    │ ├─┬ [email protected]
    │ │ └── [email protected]
    │ ├─┬ [email protected]
    │ │ └── [email protected]
    │ ├─┬ [email protected]
    │ │ ├── [email protected]
    │ │ ├── [email protected]
    │ │ ├── [email protected]
    │ │ └─┬ [email protected]
    │ │   └── [email protected] deduped
    │ └── [email protected] deduped
    ├── [email protected]
    ├── [email protected]
    ├─┬ [email protected]
    │ └── [email protected]
    ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ │ └── [email protected]
    │ └── [email protected]
    ├── [email protected]
    ├── [email protected]
    ├─┬ [email protected]
    │ └─┬ [email protected]
    │   ├─┬ [email protected]
    │   │ ├── @sindresorhus/[email protected]
    │   │ ├─┬ @szmarczak/[email protected]
    │   │ │ └── [email protected]
    │   │ ├─┬ [email protected]
    │   │ │ ├─┬ [email protected]
    │   │ │ │ └── [email protected] deduped
    │   │ │ ├─┬ [email protected]
    │   │ │ │ └── [email protected] deduped
    │   │ │ ├── [email protected]
    │   │ │ ├─┬ [email protected]
    │   │ │ │ └── [email protected]
    │   │ │ ├── [email protected]
    │   │ │ ├── [email protected]
    │   │ │ └─┬ [email protected]
    │   │ │   └── [email protected] deduped
    │   │ ├─┬ [email protected]
    │   │ │ └── [email protected] deduped
    │   │ ├── [email protected]
    │   │ ├─┬ [email protected]
    │   │ │ └─┬ [email protected]
    │   │ │   ├─┬ [email protected]
    │   │ │   │ └── [email protected] deduped
    │   │ │   └─┬ [email protected]
    │   │ │     └── [email protected]
    │   │ ├── [email protected]
    │   │ ├── [email protected]
    │   │ ├── [email protected]
    │   │ ├── [email protected]
    │   │ └─┬ [email protected]
    │   │   └── [email protected]
    │   ├─┬ [email protected]
    │   │ └─┬ [email protected]
    │   │   ├── [email protected]
    │   │   ├── [email protected]
    │   │   ├── [email protected]
    │   │   └── [email protected]
    │   ├─┬ [email protected]
    │   │ └── [email protected] deduped
    │   └── [email protected]
    ├─┬ [email protected]
    │ └── [email protected]
    ├─┬ [email protected]
    │ └── [email protected]
    ├─┬ [email protected]
    │ └─┬ [email protected]
    │   └── [email protected]
    └── [email protected]

update-notifier pulls in more dependencies than the rest of nodemon.

I personally think that the drawbacks from keeping outweigh the benefits:

  1. The attack surface is increased, and having many dependencies makes it harder to vet the nodemon dependency tree.
    • Over the years, several vulnerability reports for nodemon dependencies have come from the update-notifier tree. Not all of the vulnerabilities affect nodemon, but again, it's not easy to immediately tell.
    • Dependency chain attacks are becoming more common.
    • Currently, npm outdated reports that about 70 requirements are at least 1 major version behind. I'm sure that some of the updates are not currently necessary, but perhaps they may soon.
  2. In the ~8 years since update-notifier was added, the ecosystem has improved visibility and handling of dependency updates. We have npm outdated and yarn upgrade-interactive, and npx nodemon without an explicit dependency will try to run the latest version by default.

Thoughts?

@remy
Copy link
Owner

remy commented Dec 7, 2021

I don't disagree, but you're point (2) doesn't address what update notifier solves: automatic notification of upgrade.

All other solutions require the user to specifically ask whether there's an upgrade (potentially one that does fix a vuln).

I strongly believe that this project needs something like upgrade notifier, though I'm not convinced it requires that particular dep or any of it's complexity...

@aledalgrande
Copy link

Just my 2c, but I agree with OP. Currently update-notifier has vulnerabilities in its dependencies, some that don't seem to be actively maintained (e.g. nexdrew/ansi-align#61).

Yes, it adds the ability to automatically notify, but we have other specialized tools nowadays for that that run in CI. What we don't have right now is the ability to update that ansi dep that has a security issue. Removing so many dependencies from Nodemon would increase security considerably.

Let me know what you think and if you need help.

@remy
Copy link
Owner

remy commented Jan 4, 2022

I've already agreed that I'd rather not have the dependency. What I'd need to do to remove the dep is to have a bespoke, nodemon specific system to notify of updates. In my head that would probably check an npm endpoint for the current version and compare to the local version, along with a backoff (i.e. do it once a week, or even opt-out), then show a notice.

@jimmywarting
Copy link

jimmywarting commented Feb 14, 2022

👍 on removing it

it is a waste of install time of having to download and exec all of this every time some CI needs to build.

@remy
Copy link
Owner

remy commented Feb 14, 2022

@jimmywarting it's a waste of time including nodemon as part of your CI dependencies… seems like something you wouldn't want to do.

@jimmywarting
Copy link

nodemon is in there with eslint and everything else in dev devdependencies, everytime a PR comes in it must download all dev dependencies as well...

i think a dependabot or something is a better solution.
another thing that you can do is marking older version on npm as deprecated and tag a older release with something like

v1.x is deprecated and don't get any more security patches, update to v2

that way we will get the notification too.

@jimmywarting
Copy link

jimmywarting commented Feb 18, 2022

I would rather have something as small and dependency free version like this running each and every time than having to load 84 modules and risk getting any security issues

const fs = require('node:fs/promise')
const https = require('node:https')

https.get('https://registry.npmjs.org/nodemon', async response => {
    let body = ''
    for await (const chunk of response) body += chunk
    const json = JSON.parse(body)
    const latest = json['dist-tags']['latest'].split('-')[0]
    const pkg = await fs.readFile(`${__dirname}/../../package.json`, 'utf8')
    const pkgJson = JSON.parse(pkg)
    if (latest.localeCompare(pkgJson.version, 'en', {numeric: true}) === -1) {
        console.info(`New version of nodemon is available update to ${latest}`)
    }
})

// haven't tested this code but should work

but preferable nothing at all - just run npm deprecate [email protected] new version available, update to 2.1.0

every sub dependency that you have no control over is a potentially security risk

@remy
Copy link
Owner

remy commented Feb 18, 2022

Pop it in a PR (and probably test!), but yeah, I already agreed: #1961 (comment)

@danepowell
Copy link

Another vulnerability introduced by update-notifier and unlikely to be resolved on its own: #2023

@lorand-horvath
Copy link

lorand-horvath commented Jun 24, 2022

It's worth mentioning that upgrading to a newer version of update-notifier@6 in [email protected] https://github.com/yeoman/update-notifier/releases/tag/v6.0.0 didn't work because @sindresorhus has chosen to completely switch to ES modules, dropping support for CommonJS modules, more on that here https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c - very interesting read! So packages dependent on anything newer from his repos are subject to this problem of incompatibility.

Specifically, update-notifier@5 has a transient dep on got@9 which I reported here sindresorhus/got#2067 but I don't think the vulnerability fix will be backported to got v9 from the ESM-only v12.

@Axent96
Copy link

Axent96 commented Jun 28, 2022

related #2039 & #2040

@DesignByOnyx
Copy link

For those worried about CI and other environments where you need dev dependencies but not things like nodemon, this is what I use optionalDependecies for. I consider optional deps to be "dev dependencies which are not required for building or running tests". Nodemon is usually the number 1 candidate for this.

# make nodemon optional
npm i nodemon -O

Then in your CI/CD/Docker/Whatever:

npm ci --no-optional

@remy remy closed this as completed in #2033 Jul 5, 2022
remy pushed a commit that referenced this issue Jul 5, 2022
Closes #1961
Closes #2028

- Fixes security issue with got (CVE-2022-33987)
- Replace update-notifier with simple-update-notifier which does the same thing but has one dependency (semver) rather than several
- Same caching settings as update-notifier

Congratulations and thanks to @alexbrazier 👏 🥇 ❤️
@github-actions
Copy link

github-actions bot commented Jul 5, 2022

🎉 This issue has been resolved in version 2.0.19 🎉

The release is available on:

Your semantic-release bot 📦🚀

@aledalgrande
Copy link

thank you @remy!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment