Entries previsouly ignored showing up as new

[creising]

We seem to be hitting a condition with vulnerabilities that were previously ignored showing up as new in our audit-resolve.json file. It looks like the ID is used as part of the key in the audit-resolve.json file, but lately it looks like these IDs are changing (daily over the last couple of days) which causes our build to keep breaking.

I know this tool is not responsible for the IDs, but I was curious if anyone else was having this issue and/or if there is any plan to refine how the key is generated in the audit-resolve.json file. Maybe the github_advisory_id is more stable?

• Entry from March 8th: 1030656|ember-cli>markdown-it-terminal>markdown-it
• Entry needed for same ignored entry for March 9th: 1051572|ember-cli>markdown-it-terminal>markdown-it

Thank you for a such a useful tool!

NPM version: 6.14.12 npm-audit-resolver version: 3.0.0-6

[adammwood]

We're seeing this across multiple projects and vulnerabilities, with the IDs (source property) changing every few hours, and came here to raise an issue to see if a different/stable ID could be used.

Here is an example from the last 48 hours with one vulnerability (oldest to newest):

[high] node-fetch: node-fetch is vulnerable ... (1028029)
[high] node-fetch: node-fetch is vulnerable ... (1030641)
[high] node-fetch: node-fetch is vulnerable ... (1033253)
[high] node-fetch: node-fetch is vulnerable ... (1038477)
[high] node-fetch: node-fetch is vulnerable ... (1043701)

Note the IDs are constantly incrementing.

Unless this is an issue with NPM/GitHub that can be resolved (not sure where this would be raised), I agree that a more stable identifier needs to be used as it's rendering this project unusable.

Happy to help on a change if it would be useful and if there's consensus on a solution.

[adammwood]

Having looked into this further, we initially thought that this was related to the switch of NPM to GitHub Advisory Database, but dismissed this as it occurred months ago and we've only started seeing this in the last few weeks.

I've then seen that the GitHub Advisory Database repository was created in the last few weeks and has commits every few hours. It may be that they regenerate all their identifiers on any commit/build now or similar.

Either way, it seems that the current source property that's used is no longer stable. The only reference I can see to the GitHub reference in the output of npm audit --json is in the via.url property, so it would need to be parsed from that. Although I'm not familiar with what other via options are possible (if any) with npm audit, so this would need confirming. I can see that github_advisory_id is returned from Yarn's audit functionality but not NPMs.

See the same problem and same proposed solution here: https://github.com/IBM/audit-ci/issues/211

[creising]

I do see github_advisory_id from NPM when I run npm audit --json so I think it will work if everyone thinks that is the right path forward.

[adammwood]

I think we may be seeing something different due to NPM versions and the NPM 7 rewrite of audits.

Example output from NPM 8.1.0:

{
  "auditReportVersion": 2,
  "vulnerabilities": {
    "quill": {
      "name": "quill",
      "severity": "moderate",
      "isDirect": false,
      "via": [
        {
          "source": 1054693,
          "name": "quill",
          "dependency": "quill",
          "title": "Cross-site Scripting in quill",
          "url": "https://github.com/advisories/GHSA-4943-9vgg-gr5r",
          "severity": "moderate",
          "range": "<=1.3.7"
        }
      ],
      "effects": [
        "react-quill"
      ],
      "range": "<=1.3.7",
      "nodes": [
        "node_modules/quill"
      ],
      "fixAvailable": {
        "name": "react-quill",
        "version": "0.0.2",
        "isSemVerMajor": true
      }
    },
    "react-quill": {
      "name": "react-quill",
      "severity": "moderate",
      "isDirect": true,
      "via": [
        "quill"
      ],
      "effects": [],
      "range": ">=0.0.3",
      "nodes": [
        "node_modules/react-quill"
      ],
      "fixAvailable": {
        "name": "react-quill",
        "version": "0.0.2",
        "isSemVerMajor": true
      }
    }
  },
  "metadata": {...}
}

[creising]

I see now. The output is much different.

[naugtur]

npm 7+ is using a totally different endpoint from the registry server.

Looks like the change originates from folding the npm advisory into github advisory. It might be a breaking change for all users. I'm gonna try to reach out to npm folks

and btw audit-resolver v3.0.0-7+ is only going to work with npm v8 and some later versions of 7. there's some versions of npm7 that wont work. should still support v6, but moving from one to another may be a breaking change for your decisions file.

[naugtur]

@ruyadorno @darcyclarke help! :scream: Is there a way to convert the old ids to the new ids?

[adevine]

Thanks very much all for the info in this thread. This is hitting us as well.

FWIW, we are also seeing a lot of audit failures when we run with the --production flag that we should not, because the dependencies only come through under devDependencies (transitive deps from our devDependencies, that is). This is solely a bug with NPM, NOT npm-audit-resolver, but this thread was helpful because I believe some of the recent npm audit changes must have caused this.

[naugtur]

npm support Ticket ID: 1541272

email thread id GO8PEM-Z34EY

@MylesBorins some folks suggested I could let you know this is happening.

[MylesBorins]

@naugtur we are aware and researching internally to figure out what exactly is going on

[naugtur]

Thanks @MylesBorins !
Don't hesitate to loop me in if there's anything I can do to help.

[johnz]

@MylesBorins , @naugtur can you please advise if there is any update?

[naugtur]

I am not aware of any updates. But I have no insight into npm/github.
Is anybody monitoring if this is still occurring? I switched to other aspects of supply chain security a few months back and npm audit is not part of my daily routine right now.

I'm pondering a rewrite where I skip the dependency on npm/yarn for audit and compile the audit results from github vulnerability database directly, using other identifiers, like the CVE number. It may be prohibitively expensive to do though and would benefit from doing everything from scratch (because a migration path from broken resolve files doesn't make sense anyway)
With the current situation the only valuable part of npm-audit-resolver is the user interface code. I'd throw away everything else if I decided to do the above. There's a design in my head. Next step: feasibility study. If that goes well, I'd feel comfortable starting this project if I had 2-3 people with a weekly quota of 6-8 hours to work on it. At this time I'm not sure I'd have room to commit to a weekly quota myself.

[adammwood]

@naugtur That sounds like an interesting long-term solution.

In the short-term, would you consider switching to using github_advisory_id as the identifier as mentioned earlier in this issue, which would presumably need to go into a major release as a breaking change? This approach has already been adopted by similar projects hitting this issue, e.g. https://github.com/IBM/audit-ci/pull/217. If so, and if it's something you would accept a PR for, it may be something we can assign resources to.

[adevine]

+1 to @adammwood's suggestion.

One point of clarification. I obviously don't know if others have the same opinion, but the only issue for us is that the npm IDs are not stable. We have no problem throwing away all of our audit resolver files and starting over (i.e. things aren't working for us now anyways so we're doing audit failure checks manually), so literally anything that would be unlikely to change, even like a hash of the issue title, would work for us.

[naugtur]

For some reason I thought both are not stable. :facepalm: I'll merge a PR for that change right away (or implement myself whenever I get an afternoon to myself)

[MylesBorins]

GitHub advisory ID will be stable. I'm working on getting rid of the mutability of the npm advisory ID.

[naugtur]

@creising @adammwood @adevine can any one of you confirm you're no longer having issues? I'd close this one.

[creising]

Was there a new version released with a fix?

[naugtur]

@creising release was not necessary. The issue was in data supplied from npm

[wokkaflokka]

@naugtur responding on behalf of @adevine to your question from 2022-06-13 -- yes, this seemed to resolve for our project for the past ~6 months.

We are observing behavior this week that may indicate this behavior has regressed. However, I haven't done enough diligence to say this with complete confidence ... I'm still looking into it.

[naugtur]

That'd be bad timing, as I was hoping to finally release the v3 and get to work on a brand new way of doing the checking.

[wokkaflokka]

Let me take a deeper look at the issues we're seeing -- I'll respond here by 5pm MST with my findings. Hopefully it's not a regression in the npm data source.

[wokkaflokka]

Here's a sample config:

{
  "decisions": {
    "1054663|firebase>@firebase/analytics>@firebase/component>@firebase/util": {
      "decision": "ignore",
      "madeAt": 1646842490288,
      "expiresAt": 1649434452188
    },
    "1054172|firebase>@firebase/analytics>@firebase/component>@firebase/util>@firebase/app>@firebase/database>@firebase/firestore>node-fetch": {
      "decision": "ignore",
      "madeAt": 1646842496080,
      "expiresAt": 1649434452188
    },
    "1054183|firebase-admin>node-forge": {
      "decision": "ignore",
      "madeAt": 1646842497494,
      "expiresAt": 1649434452188
    },
    "1054287|swagger2openapi>better-ajv-errors>jsonpointer": {
      "decision": "ignore",
      "madeAt": 1646842498850,
      "expiresAt": 1649434452188
    },
    "1054685|swagger2openapi>better-ajv-errors>jsonpointer>oas-validator>ajv": {
      "decision": "ignore",
      "madeAt": 1646842500173,
      "expiresAt": 1649434452188
    },
    "1054109|urijs": {
      "decision": "ignore",
      "madeAt": 1646842501645,
      "expiresAt": 1649434452188
    },
    "1054113|urijs": {
      "decision": "ignore",
      "madeAt": 1646842503398,
      "expiresAt": 1649434452188
    },
    "1070480|firebase-admin>dicer": {
      "decision": "ignore",
      "madeAt": 1658757840161,
      "expiresAt": 1661349828635
    },
    "1075704|@google-cloud/pubsub>google-gax>protobufjs": {
      "decision": "ignore",
      "madeAt": 1659021126685
    },
    "1084677|xml-crypto>@xmldom/xmldom": {
      "decision": "ignore",
      "madeAt": 1665524576309
    },
    "1085242|firebase-admin>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1671728035974,
      "expiresAt": 1674320022857
    },
    "1085242|firebase-functions>firebase-admin>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1671728035974,
      "expiresAt": 1674320022857
    },
    "1085242|twilio>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1671728035974,
      "expiresAt": 1674320022857
    },
    "1085243|firebase-admin>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1671728039600,
      "expiresAt": 1674320022857
    },
    "1085243|firebase-functions>firebase-admin>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1671728039600,
      "expiresAt": 1674320022857
    },
    "1085243|twilio>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1671728039600,
      "expiresAt": 1674320022857
    },
    "1085244|firebase-admin>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1671728041727,
      "expiresAt": 1674320022857
    },
    "1085244|firebase-functions>firebase-admin>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1671728041727,
      "expiresAt": 1674320022857
    },
    "1085244|twilio>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1671728041727,
      "expiresAt": 1674320022857
    },
    "1085245|firebase-admin>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1671728044554,
      "expiresAt": 1674320022857
    },
    "1085245|firebase-functions>firebase-admin>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1671728044554,
      "expiresAt": 1674320022857
    },
    "1085245|twilio>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1671728044554,
      "expiresAt": 1674320022857
    },
    "1085248|firebase-admin>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1671743398973,
      "expiresAt": 1674335375940
    },
    "1085248|firebase-functions>firebase-admin>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1671743398973,
      "expiresAt": 1674335375940
    },
    "1085248|twilio>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1671743398973,
      "expiresAt": 1674335375940
    },
    "1085250|firebase-admin>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1671743398973,
      "expiresAt": 1674335375940
    },
    "1085250|firebase-functions>firebase-admin>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1671743398973,
      "expiresAt": 1674335375940
    },
    "1085250|twilio>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1671743398973,
      "expiresAt": 1674335375940
    },
    "1085251|firebase-admin>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1671743398973,
      "expiresAt": 1674335375940
    },
    "1085251|twilio>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1671743398973,
      "expiresAt": 1674335375940
    }
  },
  "rules": {},
  "version": 1
}

The following occurs when checking audit (moment of execution ~2022-12-28 4:45PM MST):

(base) wokkaflokka@wokkaflokka % npx check-audit --omit=dev  
>>>> npm audit --json --omit dev
Total of 4 actions to process
--------------------------------------------------
[high] jsonwebtoken: jsonwebtoken has insecure input validation in jwt.verify function (1085261)
  twilio>jsonwebtoken 
--------------------------------------------------
 😱   Unresolved issues found!
--------------------------------------------------

This isn't an exhaustive analysis but it sure feels like these ID's are once again unstable.

[joebowbeer]

I doubt they are unstable given my experience that every time I check at the source (GitHub Advisory of reviewed vulnerabilities) there is in fact a new vulnerability.

[joebowbeer]

There are four different CVEs reported against jsonwebtoken <= 8.5.1:

https://github.com/advisories?query=type%3Areviewed+jsonwebtoken

[wokkaflokka]

Yes, but my N coworkers keep having to ignore day after today (even today, even right now it's complaining despite a commit around 11am CST adding an ignore directive), and several of them indicated they ignored for 30 days (7 days ago). A 30 day ignore in the past week shouldn't notify until late January.

In any case:

1. Greatly appreciate your work and this repo.
2. I'm on vacation and only touching base for this short window of time. I could totally be wrong, you likely know significantly more in this domain. I just wanted to follow up as I said I would.

It still smells to me like this is an upstream issue, but I have to defer to you as a domain expert here.

[joebowbeer]

Another way to look at jsonwebtoken is to see that there are 4 separate advisories posted in the source code repo:

https://github.com/auth0/node-jsonwebtoken/security/advisories

All reported in the last week by a maintainer of the code.

[adevine]

FWIW, I have verified the upstream IDs are unstable. Yesterday (2022-12-29) throughout the day out builds passed, and then late yesterday evening started failing due to changes in the jsonwebtoken vuln IDs. That is, we previously had these entries in audit-resolve.json:

    "1085251|twilio>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1671743398973,
      "expiresAt": 1674335375940
    },
    "1085261|twilio>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1672249670936,
      "expiresAt": 1674841660819
    },

When I ran resolve-audit this morning (because it was being reported that these 2 were failing again) it added these 2 entries:

    "1085291|twilio>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1672416390477,
      "expiresAt": 1675008376923
    },
    "1085292|twilio>jsonwebtoken": {
      "decision": "ignore",
      "madeAt": 1672416398816,
      "expiresAt": 1675008376923
    }

(notice that the "madeAt" in these 2 entries is before the "expiresAt" of the previous ones).

I know jsonwebtoken has had a bunch of new vulns reported recently, but something with them is causing the IDs to be unstable when they were previously ignored.

[joebowbeer]

@adevine I don't understand your comment about madeAt

There are currently four different reviewed advisories for jsonwebtoken and you listed four different indices above.

What is the expected result?

Is there a way to match these indices with the GitHub advisory?

Here are the four advisories. Note that two were updated 18 hours ago. Does the index change every time they are updated?

• https://github.com/advisories/GHSA-27h2-hvpr-p74q (updated 2 days ago)
• https://github.com/advisories/GHSA-8cf7-32gw-wr33 (updated last week)
• https://github.com/advisories/GHSA-qwph-4952-7xr6 (updated 18 hours ago)
• https://github.com/advisories/GHSA-hjrf-2m68-5959 (updated 18 hours ago)

[adevine]

Does the index change every time they are updated?

Yes, that is indeed the issue we are seeing. Every time an advisory is updated the index changes so it starts failing again, even if it was previously ignored.

In the examples I gave, I didn't highlight the OTHER entries that we had to add for jsonwebtoken (which correspond to the first 2 bullet points in your list), just the ones that had changed and started failing again yesterday evening when they should have been ignored. My comment about the "madeAt" was just to highlight that I had to add those entries in the second block before the entries in the first block I posted expired, point being I shouldn't have had to enter them again (or, at least, shouldn't have had to if the IDs were stable) because the original ignore decisions hadn't yet expired.

[joebowbeer]

Thinking aloud: Should the index change if the issue is updated?

For example, if the threat level changes (esp. upgraded), or the affected version range changes, or the fixed version changes, isn't a re-review warranted?

[naugtur]

If there's a meaningful change in the report, it'd make sense to update the id. But it seems odd that they'd keep doing that every day for a while

[joebowbeer]

I notice that there is a link to click for history. The history of GHSA-hjrf-2m68-5959 lists 3 changes:

https://github.com/github/advisory-database/commits/main/advisories/github-reviewed/2022/12/GHSA-hjrf-2m68-5959/GHSA-hjrf-2m68-5959.json

Dates of changes: 12/21, 12/22, 12/29

[naugtur]

@adevine Are the dates above matching your observations? If so, it's expected behavior. I'd welcome an issue in this repo to extend the output with more meaningful info helping with such situations - like "an entry for this package exists in your rules for a different advisory. it might be an update to the details" with both IDs listed.

[adevine]

Thanks very much @naugtur and @joebowbeer for your help. Yes, the changes to align with those dates, but FWIW I strongly believe this should not be the desired behavior (though I don't necessarily think it should be up to this repo to fix this issue).

To Joe's previous thought: "Thinking aloud: Should the index change if the issue is updated?"

Given how I've seen issues being updated in the past, I believe the answer to this should be no. If an issue is updated to the point that it has enough different about it that warrants downstream users to re-evaluate, my 2c is that it should get a new CVE.

The problem now is that about 99% plus of the advisories we get are false positives. Now, I don't think that's unusual. I would expect that the vast majority of advisories aren't necessarily relevant to how any particular package is using a dependency. Thus, I think that in-and-of itself is acceptable, but when I want to ignore a particular CVE in my repo, I don't think I should have to re-ignore it every day or so. It leads to it being even MORE difficult to find the truly critical issues from the noise. (As an aside, I still can't figure out why the --production or --omit=dev flag isn't being honored by npm audit the way it should).

Case in point, our json5 advisory started alerting again this morning despite the fact that there was no substantive change in the advisory: https://github.com/github/advisory-database/commits/main/advisories/github-reviewed/2022/12/GHSA-9c47-m6qq-7p4h/GHSA-9c47-m6qq-7p4h.json

Others may disagree, but for my code I'm going to look for a solution where I can consider the CVE identifier as the ID and only fail our build if I get a new CVE.

[naugtur]

Oh there is a way to make audit consider only prod dependencies AFAIR. I think it was --only=prod but I didn't use it much.

[adevine]

Oh there is a way to make audit consider only prod dependencies AFAIR. I think it was --only=prod but I didn't use it much.

Yep, we use that, but it doesn't work (and I haven't figured out the rhyme or reason as to why it sometimes doesn't work). Note this is solely an npm audit issue.

For example, right now if I run npm audit --only=prod or npm audit --omit=dev (when I run the first version I get console output that says I should use the --omit=dev form), I get a failure on the json5 package. But then when I run npm ls json5, my packages that depend on json5 are all only in my devDependencies.

[adevine]

One last note on this, as I see where the issue is now arising: https://github.com/naugtur/npm-audit-resolver/blob/v3.0.0-7/src/pkgmanagers/npm.js#L21

That is, instead of using ${via.source}|${via.name} for the key, my plan is to fork this repo (and happy to submit a PR back if so desired) and instead use ${via.url}|${via.name} for the key (or perhaps just parse out the last part of the URL that has the GHSA ID and use that). The reason being that, in my opinion, I don't want to have to re-ignore if the source changes but the GitHub Security Advisory ID doesn't change. Of course, this isn't backwards compatible for existing audit-resolve files, but for me I'm fine with making this change for us once, re-generating the audit-resolve file, and then have it be more stable going forward.

[naugtur]

I'd accept a PR allowing you to specify what constitutes a key in some config somewhere (tbd) that defaults to backwards-compatible. it could go into the audit-resolve.json in the rules section for example. That way you could also specify nothing as the first segment and start ignoring entire packages (not great, but some people want that)

[joebowbeer]

As for --production not working, I haven't seen a repro of this in many months.

In every suspected case, I do see a production dependency when I run:

npm ls <pkg> --production

(deprecated? --only=prod?)

[naugtur]

I do remember npm audit at least initially supporting a different set of dependency type selectors (prod/dev/etc) than the rest of npm commands, but the ones that worked in audit worked for me.

[adevine]

In every suspected case, I do see a production dependency when I run: npm ls <pkg> --production

Thank you @joebowbeer! This was indeed the missing link for me, but the behavior here makes me think that npm ls is busted, or at least works in very unexpected ways. That is, when I run just npm ls json5, I only get paths that point to devDependencies. When I run npm ls json5 --production or npm ls json5 --omit=dev (--production and --omit=dev are synonyms), only then do I see the production dependency path show up. It's bizarre that adding the --omit=dev flag would cause additional dependency paths to show up that weren't returned when I passed no flags. Seems to me this is happening in cases where I have a parent dependency that exists in both prod and dev chains. My npm version is 8.11.0, I'll try searching the npm repo to see if a bug for this has already been reported.

And thanks @naugtur , I'll go the route you suggest. Although, somewhat ironically, as I was looking for the bug about npm ls I saw this submitted issue about npm audit in v8, https://github.com/npm/cli/issues/4161, which means that the audit output format seems to have changed unexpectedly yet again.

[stevendarby]

@adevine did you get anywhere with a PR to allow customising the ID? I agree that what is currently happening is not desirable behaviour. Considering forking to hardcode in using the URL, as per your suggestion.

[stevendarby]

@MylesBorins

GitHub advisory ID will be stable. I'm working on getting rid of the mutability of the npm advisory ID.

Hey, don't suppose there is any update on this? After having just ignored the new IDs for the semver issue & adding new ignores for a more recent tough-cookie issue, we run the CI again and find tough-cookie has been updated again meaning it has a new ID and the ignore does not work.

FWIW generally we're ignoring an issue because we can't resolve it yet, updates to the advisory won't really change this.

[naugtur]

All audit processing is happening one way (get the entire audit, transform it as needed to store in a shape that's easy to process) and only then the lookup by IDs happens, so it should be a matter of providing alternative ways to process the audit result to use different fields for ID. If you have specific proposals which field to use (and find that field reliably unique and not missing sometimes) we can give it a go and implement it on a branch. Create a new issue and link it here if you want to pursue that.

[stevendarby]

@naugtur My proposal would only be what @adevine has already proposed further up:

my plan is to fork this repo (and happy to submit a PR back if so desired) and instead use ${via.url}|${via.name} for the key (or perhaps just parse out the last part of the URL that has the GHSA ID and use that).

In response to this, you suggested making it configurable:

I'd accept a PR allowing you to specify what constitutes a key in some config somewhere (tbd) that defaults to backwards-compatible.

I can raise an issue to propose what @adevine suggested but - just to check before I do that - would you not make the same suggestion again?