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

Monorepo: Fix npm audit security vulnerabilities (nyc / verdaccio / tap-spec / npm-auth-to-token) #1537

Closed
holgerd77 opened this issue Oct 21, 2021 · 7 comments

Comments

@holgerd77
Copy link
Member

Doing an npm i from the root directory on master currently gives the following result:

grafik

Many of these vulnerabilities are linked to outdated versions of nyc (see also #832) and verdaccio, so we should give updates of these two libraries some priority in some first round.

While both of the libraries are dev dependencies, generally having a large number of vulnerabilities shown on npm audit also comes with some greater risk of overlooking some more severe vulnerabilities.

Mid-term we should optimally come to a situation where we just regularly update and get to a somewhat 0-vulnerabilities situation on all levels.

@holgerd77 holgerd77 changed the title Monorepo: Fix npm audit security vulnerabilities Monorepo: Fix npm audit security vulnerabilities (nyc / verdaccio) Oct 21, 2021
@emersonmacro emersonmacro self-assigned this Oct 28, 2021
@emersonmacro
Copy link
Contributor

@holgerd77 I made a PR to upgrade verdaccio and nyc. After that, npm still shows some vulnerabilities: 10 vulnerabilities (5 moderate, 5 high). I tried looking into them a bit but the fixes weren't immediately clear to me.

@holgerd77
Copy link
Member Author

holgerd77 commented Nov 16, 2021

Ok, looking into the remaining vulnerabilities here. One other high severe vulnerability is coming from tap-spec, which is a dev dependency in the VM package.json and is used for the formatter dev command for VM testing which routes to the formatTest.js script in the vm/scripts folder, where tap-spec is used as the default formatter (this was already somewhat hard to find out).

My first thought was that we might want to remove this formatter script all together since I guess this is rarely (not at all?) used. But on another thought it should already be enough to remove tap-spec as a default and make this script more generic. If one then wants to use this with tap-spec it is always possible to install globally e.g..

(so, some other context on tap-spec: package seems no to be maintained any more, last release is from over 3 years ago, so not possible to update (already on latest version) and therefore would be generally desirable to get rid of this package).

@holgerd77 holgerd77 changed the title Monorepo: Fix npm audit security vulnerabilities (nyc / verdaccio) Monorepo: Fix npm audit security vulnerabilities (nyc / verdaccio / tap-spec) Nov 16, 2021
@holgerd77
Copy link
Member Author

Second high severe vulnerability is coming from npm-auth-to-token, an extremely obscure dev dependency which we should definitely remove. This has been published four years ago and there is neither a README nor a matching code/GitHub site available.

Dependency is used in the E2E publish script (for doing the Hardhat tests) for logging in to npm, I guess this should be possible to be relatively easily achieved in a more standard way? 🤔

@holgerd77 holgerd77 changed the title Monorepo: Fix npm audit security vulnerabilities (nyc / verdaccio / tap-spec) Monorepo: Fix npm audit security vulnerabilities (nyc / verdaccio / tap-spec / npm-auth-to-token) Nov 16, 2021
@emersonmacro
Copy link
Contributor

@holgerd77 I have a PR up for the first vulnerability. I found a more up-to-date fork of tap-spec. A fresh npm i after that change shows just 2 high vulnerabilities, which I think is down to the npm-auth-to-token vulnerability. I'll try to tackle that next.

@emersonmacro
Copy link
Contributor

Update: I found a workaround for npm login functionality in the e2e-publish script that does not require an external library, so the PR removes npm-auth-to-token as well. After those two changes, a fresh npm i shows zero high severity vulnerabilities! 🎉

@holgerd77
Copy link
Member Author

Update: I found a workaround for npm login functionality in the e2e-publish script that does not require an external library, so the PR removes npm-auth-to-token as well. After those two changes, a fresh npm i shows zero high severity vulnerabilities! 🎉

That's super cool, thanks! 🙂 👍

@holgerd77
Copy link
Member Author

Most of this has been addressed, will close.

Nevertheless: fixes on vulnerabilities from npm audit are always welcome!

michaelbromley added a commit to vendure-ecommerce/vendure that referenced this issue Nov 17, 2023
The `npm-auth-to-token` dependency is probably breaking. It has no
readme or repo link: https://www.npmjs.com/package/npm-auth-to-token
and has not been updated for 6 years.

The issue is discussed here: ethereumjs/ethereumjs-monorepo#1537
and I am attempting the fix as seen here: https://github.com/ethereumjs/ethereumjs-monorepo/pull/1579/files#diff-02c8f04118065b423b5b599fe96dc405107c1a84c7840d9ec3b04627021aef69
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants