Skip to content

Add npm audit to test scripts #1028

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

Closed
SkeLLLa opened this issue Apr 30, 2019 · 3 comments
Closed

Add npm audit to test scripts #1028

SkeLLLa opened this issue Apr 30, 2019 · 3 comments

Comments

@SkeLLLa
Copy link

SkeLLLa commented Apr 30, 2019

Problem

Typedoc 3rd party dependencies can cause vulnerability issues. And those issues present in typedoc repo for quiet long period of time without being fixed.

Suggested Solution

Add npm audit to test or lint scripts. Or even setup recommit hooks that will run audit command.

As an alternative some audit service (e.g. snyk.io) could be integrated with this service and it will make automatic PRs that will fix such vulnerabilities.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Apr 30, 2019

I don't think it makes sense to run the audit every time something is tested or linted. Those scripts already take much longer than ideal to run, and updating dependencies shouldn't need to be the first step in fixing a bug.

Regarding automatic PRs - we stopped using Greenkeeper when its PRs turned into noise. #833

It does make sense to me to run a vulnerability audit before publishing a new version, so it might make sense to run it in the prepublishOnly npm hook.

@SkeLLLa
Copy link
Author

SkeLLLa commented Apr 30, 2019

I can't agree with you regarding that fact that time running tests is more valuable than user's security.
Npm doesn't provide enough security and there are lot's of issues discovered:
https://medium.com/the-node-js-collection/node-js-security-wg-january-2019-619ef804cd6d
https://medium.com/@liran.tal/malicious-modules-what-you-need-to-know-when-installing-npm-packages-12b2f56d3685
That just couple recent examples.

Now let's observe some stats. According to github stats this repo has 2 commits in a week average. Comparing to 100k installs in a week according to npm. So in another words running npm audit in lint or test stage will cost less than a couple of minutes in a week, but it may prevent 100k applications and libs that are using it from being compromised. So IMHO those couple of minutes completely worth it.

As for automatic greenkepper - snyk vunerability tests are completely different. They doesn't make PR unless vunerability is detected in modules used directly in typedoc or indirectly. So there will be no "noise", only security alerts.

However making audit prepublish only hook will not help a lot wit current release frequency. If new releases have frequency at least 1 per week it could work, but as far as I can see there more than one month passed since last release and latest stable version 0.14.2 was released 4 month ago.

There's also another option that will help users deal with typedoc audit issues by themselves. The solution is to remove package-lock.json in this repo. This will allow npm audit to fix issues within safe update semver versions of packages that typedoc depends on.

@aciccarello
Copy link
Collaborator

Thanks for the suggestion however I don't believe this would be a helpful solution to avoid security issues. My concern is that it would make it harder to make fixes and release updates (that usually include fixes for old security issues) because new security issues were opened on a dependency. Sometimes it isn't possible to clear all security issues immediately.

I recognize that security audit results can be concerning however not all flagged issues are relevant for a build time project such as typedoc which generates static content. We do take the security issues seriously however npm audit is a warning system and should not be automatically considered a blocker.

My understanding is that the package-lock.json does not apply when typedoc is installed as a dependency. Npm should still update versions within semver where appropriate when installing typedoc as a dependency. If anything, removing the lock file might lead to security issues being less vulnerable because they don't appear on GitHub security alerts.

Again, thank you for raising your concern. However, in my estimation, your proposed solution likely would not help the project. Feel free to continue to discuss but for now I'll close this issue.

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

No branches or pull requests

3 participants