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

Update update-notifier to v6 #1145

Closed
3 tasks done
MrArnoldPalmer opened this issue Jun 23, 2022 · 6 comments · Fixed by #1150
Closed
3 tasks done

Update update-notifier to v6 #1145

MrArnoldPalmer opened this issue Jun 23, 2022 · 6 comments · Fixed by #1150

Comments

@MrArnoldPalmer
Copy link

  • I have searched for similar issues
  • I am using the latest version of npm-check-updates
  • I am using node >= 14

update-notifier recently released v6 which upgrades it's dependency on got (deep down the tree) which currently has a CVE related.

v6 of update-notifier also migrates to pure ESM which would require consumers to also migrate. So upgrading requires NCU to use ESM and drop support for node 12 (which was already done), but also programmatic NCU consumers would have to migrate to ESM.

Not sure how interested you are in migrating NCU to pure ESM and/or if this CVE is a forcing function for that (NCU may not even be vulnerable I haven't found details to support one way or another) but I started making a PR to do the version upgrade but with the ESM changes it started growing quite large so I figured I should check first before dumping it here 😆

main...MrArnoldPalmer:chore/upgrade-notifier current state though build:options currently failing and I haven't dug in.

@raineorshine
Copy link
Owner

raineorshine commented Jun 23, 2022

Thanks for the information and initial investigation.

I've followed the dependencies down and npm-check-updates does not appear to be vulnerable.

$ npm ls got
npm-check-updates@14.1.0 /Users/raine/projects/npm-check-updates
└─┬ update-notifier@5.1.0
  └─┬ latest-version@5.1.0
    └─┬ package-json@6.5.0
      └── got@9.6.0

programmatic NCU consumers would have to migrate to ESM.

I am more hesitant about this part. I was just working on a project where we wanted to create a submodule with its own tsconfig that referenced typescript modules in the parent project. After several hours of trying different combinations of tsconfig properties and import/export syntaxes, I eventually had to give up. The error messages are inscrutable and even when you diligently remedy one error it seems to be the exact cause of another.

Sindre had to write a whole FAQ to facilitate this transition in his modules. This is why I no longer upgrade them.

The best move may be no move at all until pure ESM becomes easier for the average user.

@MrArnoldPalmer
Copy link
Author

I am more hesitant about this part.

Definitely makes sense. 👍🏻 Feel free to close this out or keep open for tracking as needed.

TY for the writeup re vulnerability of got that is very useful.

@JustusFluegel
Copy link
Contributor

JustusFluegel commented Jun 30, 2022

Small note from my side on this: As update-notifier is only used in the cli, which does not export anything, we could stay on cjs but still use update-notifier v6 by using dynamic imports like this:

// imports ...

async function cli() {
  const {default: updateNotifier} = await import("update-notifier");
  const notifier = updateNotifier({ pkg })
  if (notifier.update && notifier.update.latest !== pkg.version) {
    notifier.notify({ defer: false, isGlobal: true })
  }
  
  // the remaining code in cli.ts
}

void cli();

This would solve both problems as we can still use cjs for npm-check-updates but still use the ejs version of the package.
A minor caveat is that this would still require the engine version bump from >=14 to >=14.14 as ejs packages are only supported since this version.

@JustusFluegel
Copy link
Contributor

I would be open to create a new issue / pr if that is welcome.

@raineorshine
Copy link
Owner

That sounds like a great compromise. The bump to node>=14.4 is fine.

@raineorshine raineorshine reopened this Jun 30, 2022
@JustusFluegel
Copy link
Contributor

gimme a sec, i'll fork & pr

pflorek added a commit to pepperize/cdk-organizations that referenced this issue Jul 15, 2022
- version pin @types/prettier DefinitelyTyped/DefinitelyTyped#60314
- upgrade npm-check-updates raineorshine/npm-check-updates#1145 and projen/projen#1963
- upgrade aws-cdk-lib 2.32.0 https://github.com/aws/aws-cdk/releases/tag/v2.32.0
- use projenrc ts
- upgrade jsii 1.62.0 aws/jsii#3609
- upgrade jsii-docgen
pflorek added a commit to pepperize/cdk-organizations that referenced this issue Jul 15, 2022
- version pin @types/prettier DefinitelyTyped/DefinitelyTyped#60314
- upgrade npm-check-updates raineorshine/npm-check-updates#1145 and projen/projen#1963
- upgrade aws-cdk-lib 2.32.0 https://github.com/aws/aws-cdk/releases/tag/v2.32.0
- use projenrc ts
- upgrade jsii 1.62.0 aws/jsii#3609
- upgrade jsii-docgen
mergify bot pushed a commit to projen/projen that referenced this issue Jul 18, 2022
due to security vulnerabilities

See raineorshine/npm-check-updates#1145

Fixes #1963

---
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants