jeremylong / DependencyCheck

OWASP dependency-check is a software composition analysis utility that detects publicly disclosed vulnerabilities in application dependencies.
https://owasp.org/www-project-dependency-check/
Apache License 2.0
6.27k stars 1.25k forks source link

SARIF "ruleId" for NPM findings is just a number; should be NPM- prefixed #4515

Open candrews opened 2 years ago

candrews commented 2 years ago

Describe the bug In SARIF report output, the "ruleId" for NPM findings is just a number. That make it hard to understand what database to look that number up in, adding the NPM- prefix would make the meaning of the number much more obvious.

For example, here's an excerpt from a SARIF report:

"results": [ {"ruleId": "1070273","level": "warning","message": {"text": "1070273 - ansi-regex is vulnerable to Inefficient Regular Expression Complexity"},"partialFingerprints": {"vulnerabilityHash": "ecc887814803262bb6af684fd4dd8791"},"locations": [{"physicalLocation": {"artifactLocation": {"uri": "\/src\/frontend\/yarn.lock?ansi-regex","index": 16094 }},"logicalLocations": [{"fullyQualifiedName": "pkg:npm\/ansi-regex@3.0.0"}]}]}]

In the HTML report, the information presented is much more helpful/readable: image

Note that the SARIF report generator already has code to special handle NPM and add the NPM- prefix in another place: https://github.com/jeremylong/DependencyCheck/blob/07bd05189ee4a3635365bd72e8d265c4ea49385e/core/src/main/resources/templates/sarifReport.vsl#L23

Version of dependency-check used cli version 7.1.0

Log file n/a

To Reproduce Steps to reproduce the behavior:

  1. Run OWASP dependency check on a project that has NPM findings
  2. View the SARIF report

Expected behavior The ruleId field should contain a full reference uniquely identifying the finding; part of that unique identifier should be the database (in this case NPM).

Additional context Add any other context about the problem here.

candrews commented 2 years ago

Perhaps a better approach would be to set the vulnerability's name to something more logical than just the id at https://github.com/jeremylong/DependencyCheck/blob/56058e5dc3bd44d261f0a81a69be130dfd143823/core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractNpmAnalyzer.java#L455

To be specific, instead of:

vuln.setName(String.valueOf(advisory.getId()));

How about:

vuln.setName("NPM-" + advisory.getId() + ": " + advisory.getTitle());