jeemok / better-npm-audit

The goal of this project is to provide additional features on top of the existing npm audit options
https://www.npmjs.com/package/better-npm-audit
MIT License
118 stars 26 forks source link

Update 'isExcepted' to check for CVE id #73

Closed kyleclark1824 closed 2 years ago

kyleclark1824 commented 2 years ago

Making this PR because every time a vulnerability get's updated the ID seems to change. So I constantly have to update my .nsprc file with the new ID.

It looks like the JSON output from the audit contains a cves property with the GitHub CVE ID's. So would something like this work?

So if we don't have a match to the cur.id check the cve id's. Not ideal having numeric a non-numeric ID's but that should allow the CVE ID to be used in the .nsprc as well as the other standard ID's.

Related issue: https://github.com/jeemok/better-npm-audit/issues/60

leedm777 commented 2 years ago

I stumbled across this looking for alternatives due to IBM/audit-ci#211.

I think you're going to need to look at the advisory URL; not just the CVEs. There are at least some vulnerabilities that don't get assigned CVEs. See this comment on the audit-ci issue:

Does every vulnerability in the database have a CVE? It looks like here's maybe a few examples of ones that don't have CVEs: GHSA-r8hm-w5f7-wj39, GHSA-pjwm-rvh2-c87w, GHSA-xx4c-jj58-r7x6

kyleclark1824 commented 2 years ago

@leedm777, good point. Maybe even the github_advisory_id would be a good place to check. I'm just not positive which "ID's" change as things get updated.

leedm777 commented 2 years ago

@kyleclark1824 ,

Maybe even the github_advisory_id would be a good place to check.

I don't see auditReportVersion in the v2 output given by npm v8. Closest I see is via.url.

I'm just not positive which "ID's" change as things get updated.

That's entirely unclear, and probably an unintended consequence. The changing ids feels like a bug, but I'm not even sure where to report it.

What I observe changing is the via.source field from the v2 format, which looks like it's the same thing as the id field in the old format.

guillermaster commented 2 years ago

@kyleclark1824 the solution looks good, it will be very useful to have it merged soon. For some odd reason, those are IDs being currently used are changing frequently. @jeemok do you mind reviewing this PR?

leedm777 commented 2 years ago

This patch only fixes npm v6 handling. npm v7 and newer doesn't even list the CVE in the JSON output; at least not with npm v8.3.1 😕

kyle-clark1824 commented 2 years ago

This patch only fixes npm v6 handling. npm v7 and newer doesn't even list the CVE in the JSON output; at least not with npm v8.3.1 😕

Yeah @leedm777, I was worried about that as well. This "fix" shouldn't hurt in those cases but a full fix would definitely be required. I'll see if I can find the JSON output for the other versions and add a more robust fix. Probably poke at it tonight.

I'm not seeing auditReportVersion in v 6.x either. Ideally there is a good way to determine what version is being run, then use that info to determine how to set isExcepted. But I'll take a look, worst case we can just have some more conditionals to handle additional cases.

kyle-clark1824 commented 2 years ago

@leedm777, I added some more conditions. The issue with auditReportVersion2 looks like more inconsistent data. Sometimes the via array is just a single string. In those cases I don't see any sort of Identifier.

I updated to check 4 places:

  "@angular-devkit/build-angular": {
      "name": "@angular-devkit/build-angular",
      "severity": "high",
      "isDirect": true,
      "via": [
        "webpack-dev-server"
      ],
      "effects": [],
      "range": "0.8.8 - 0.1102.17 || 0.1200.0-next.0 - 12.2.14 || 13.0.0-next.0 - 13.0.0-rc.3",
      "nodes": [
        "node_modules/@angular-devkit/build-angular"
      ],
      "fixAvailable": {
        "name": "@angular-devkit/build-angular",
        "version": "12.2.16",
        "isSemVerMajor": false
      }
    }
I don't really see any way to capture ID's when the output looks like that (other than `name`?).
jeemok commented 2 years ago

hey @kyle-clark1824, thanks for initiating this change! let's work on this in a separate branch beta, I'll focus on v6 support first and try to cover different version later

jeemok commented 2 years ago

let me merge this in first and help with the eslint issue and adding unit tests, etc.

kyle-clark1824 commented 2 years ago

Sounds good @jeemok thanks!