Entries previsouly ignored showing up as new #56
Comments
|
We're seeing this across multiple projects and vulnerabilities, with the IDs ( Here is an example from the last 48 hours with one vulnerability (oldest to newest): 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. |
|
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 See the same problem and same proposed solution here: IBM/audit-ci#211 |
|
I do see |
|
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": {...}
} |
|
I see now. The output is much different. |
|
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. 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. |
|
@ruyadorno @darcyclarke help! 😱 |
|
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 |
|
npm support Ticket ID: 1541272 email thread id GO8PEM-Z34EY @MylesBorins some folks suggested I could let you know this is happening. |
|
@naugtur we are aware and researching internally to figure out what exactly is going on |
|
Thanks @MylesBorins ! |
|
@MylesBorins , @naugtur can you please advise if there is any update? |
|
I am not aware of any updates. But I have no insight into npm/github. 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) |
|
@naugtur That sounds like an interesting long-term solution. In the short-term, would you consider switching to using |
|
+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. |
|
For some reason I thought both are not stable. 🤦 |
|
GitHub advisory ID will be stable. I'm working on getting rid of the mutability of the npm advisory ID. |
|
@creising @adammwood @adevine can any one of you confirm you're no longer having issues? I'd close this one. |
|
Was there a new version released with a fix? |
|
@creising release was not necessary. The issue was in data supplied from npm |
|
@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. |
|
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. |
|
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. |
|
Here's a sample config: The following occurs when checking audit (moment of execution ~2022-12-28 4:45PM MST): This isn't an exhaustive analysis but it sure feels like these ID's are once again unstable. |
|
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. |
|
There are four different CVEs reported against jsonwebtoken <= 8.5.1: https://github.com/advisories?query=type%3Areviewed+jsonwebtoken |
|
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:
It still smells to me like this is an upstream issue, but I have to defer to you as a domain expert here. |
|
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. |
|
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: When I ran resolve-audit this morning (because it was being reported that these 2 were failing again) it added these 2 entries: (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. |
|
@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?
|
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. |
|
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? |
|
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 |
|
I notice that there is a link to click for history. The history of GHSA-hjrf-2m68-5959 lists 3 changes: Dates of changes: 12/21, 12/22, 12/29 |
|
@adevine Are the dates above matching your observations? If so, it's expected behavior. |
|
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 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. |
|
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 For example, right now if I run |
|
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 |
|
I'd accept a PR allowing you to specify what constitutes a key in some config somewhere (tbd) that defaults to backwards-compatible. |
|
As for In every suspected case, I do see a production dependency when I run:
(deprecated? |
|
I do remember |
Thank you @joebowbeer! This was indeed the missing link for me, but the behavior here makes me think that And thanks @naugtur , I'll go the route you suggest. Although, somewhat ironically, as I was looking for the bug about |
|
@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. |
Hey, don't suppose there is any update on this? After having just ignored the new IDs for the FWIW generally we're ignoring an issue because we can't resolve it yet, updates to the advisory won't really change this. |
|
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. |
|
@naugtur My proposal would only be what @adevine has already proposed further up:
In response to this, you suggested making it configurable:
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? |
We seem to be hitting a condition with vulnerabilities that were previously ignored showing up as new in our
audit-resolve.jsonfile. It looks like the ID is used as part of the key in theaudit-resolve.jsonfile, 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.jsonfile. Maybe thegithub_advisory_idis more stable?1030656|ember-cli>markdown-it-terminal>markdown-it1051572|ember-cli>markdown-it-terminal>markdown-itThank you for a such a useful tool!
NPM version: 6.14.12
npm-audit-resolver version: 3.0.0-6
The text was updated successfully, but these errors were encountered: