-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Migrate NSP Analyzer to Node Audit Analyzer #1366
Comments
This will likely need to be a phased migration as the current Node Audit API still has dependencies on Node Security Platform resources.
|
Dynamically creating package.json from fragments will no longer work. The Audit API requires a valid package-lock.json to be submitted. The POST body can either be plaintext json or a gziped version of the lockfile. https://docs.npmjs.com/cli/audit The lockfile is partially scrubbed and the following fragment is added to it: "metadata": {
"npm_version": "6.1.0",
"node_version": "v10.5.0",
"platform": "linux"
} In addition, the Documentation for the Audit API does not exist yet. https://github.com/npm/registry/issues/355 The response from the Audit API contains
|
This enhancement may need to come sooner rather than later. Although NSP is being shutdown in September, it appears that NSP is not being kept up-to-date. For example, a malicious version of eslint-scope is able to be found using |
When handling this migration, will it be possible to support suppression of specific advisories (a-la #892)? |
@pioto The migration will simply be to swap out nsp check for npm audit API compatibility so that Dependency-Check users can continue scanning Node projects without interruption. #892 requires modifications to the suppression schema for it to support 'source' (as in the source of vulnerability intelligence / NSP, NVD, etc). It will also require us to go back to all previous core suppressions and modify those to support source as well as provide backward compatibility for suppressions that do not specify a source. The Javascript to generate suppressions snippets from HTML reports will also need to be updated to support source. So #892 is a fairly big chunk of work by itself. PRs are always welcome ;-) |
Migration Branch: https://github.com/jeremylong/DependencyCheck/tree/npmaudit |
@jeremylong The migration is nearly complete. I've run into a problem though. The Node Audit analyzer uses package-lock.json, whereas the old NSP Analyzer used package.json. Code in the NodePackageAnalyzer is removing the dependency from the engine so the Node Audit analyzer is never called. If I comment out the following line, everything works as expected. So basically, we have two analyzers now, that both use the same file filters, and the Node Package analyzer executes before the Node Audit analyzer does thus causing the issue. Not sure how you want to handle that. Other than that, I believe the migration is complete. I've tested it, it works. One related issue though. NPM does not provide CVSS scores or vectors, so all the severities are 'unscored'. The RetireJS and the Node Audit analyzer both set the unscored severity, but there's nothing in the reports to account for that. This leads to all NPM findings being low severity for example because it's CVSS score is 0. The reports may need some refatoring.... Or we could make a convenience method in the Vulnerability class to handle that logic for us so the reports could be simplified. |
Update the check to see if we should delete the file to check if the NspAnalyzer is enabled and then only delete the package-lock.json if the NSP analyzer is disabled. private boolean isNspEnabled(Engine engine) {
for (Analyzer a : engine.getAnalyzers()) {
if (a instanceof NspAnalyzer) {
return a.isEnabled();
}
}
return false;
}
@Override
protected void analyzeDependency(Dependency dependency, Engine engine) throws AnalysisException {
if (isNspEnabled(engine) && !PACKAGE_LOCK_JSON.equals(dependency.getFileName())) {
engine.removeDependency(dependency);
}
... |
actually, its the opposite. I don't want to delete the dependency if the NspAnalyzer (now called NodeAuditAnalyzer) is enabled - it needs it. And if I reverse this logic, it causes all sorts of other problems with the NodePackageAnalyzer. |
Steve - just updated the branch. Did that solve the problems with NodePackageAnalyzer you were seeing? |
Hi. This is a really important feature when running as part of a CI job and you cant upgrade all your packages to newer versions. Is the documentation up to date or the feature still not implemented? |
Documentation is up-to-date in the npmaudit branch, not the master branch. npm audit has a lot of features outside of just vulnerability identification. And after chatting with their security team, more features are planned. The goal of the Dependency-Check NodeAuditAnalyzer is not to reproduce all the functionality of npm audit, but to identify vulnerable dependencies, which it does. Pull requests for additional features (like scrubbing) that are outside the scope of the migration are welcome. Dependency-Check already has an 'excludes' feature, but I believe it only applies to files, not virtual dependencies. |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
According to https://blog.npmjs.org/post/175511531085/the-node-security-platform-service-is-shutting, NSP will be shutting down September 30, 2018.
What we know:
After investigating the NPM AUDIT API, it is safe to assume that:
For organizations that rely on stable Node.js distributions, using Dependency-Check for vulnerability identification will be the only alternative.
Related: DependencyTrack/dependency-track#173
The text was updated successfully, but these errors were encountered: