Skip to content
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

fix(npm-cli): count all instances of ignored advisories #5194

Merged
merged 13 commits into from
Mar 23, 2023

Conversation

B4ckslash
Copy link
Contributor

@B4ckslash B4ckslash commented Jan 12, 2023

What's the problem this PR addresses?

When a package was used in multiple dependency chains, ignoring the relevant advisory via --ignore or npmAuditIgnoreAdvisories only reduced the vulnerability count by 1, whereas every chain including the affected package counted towards the vulnerability count separately. Therefore yarn npm audit --recursive exits with a non-zero exit code, even though all found advisories were marked as ignored. This broke our CI pipeline, which relies on the exit code of the audit run to check if any vulnerabilities are present.

Closes #5104

How did you fix it?

Instead of reducing the vulnerability count by 1 per ignored advisory, it now gets reduced by the number of all affected paths of the ignored advisory entry.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@merceyz merceyz changed the title fix(core): Ignored advisories are actually ignored even if multiple paths are affected fix(core): count all instances of ignored advisories Jan 12, 2023
@B4ckslash
Copy link
Contributor Author

I selected the packages that need a release as best I could, but I am not 100% sure that the list is complete. If a declined package needs to be updated as well, let me know and I will update the file accordingly.

@B4ckslash
Copy link
Contributor Author

@merceyz any idea why that specific windows integration test would exceed the timeout in some tests?

@arcanis
Copy link
Member

arcanis commented Jan 12, 2023

Windows sometimes timeouts, we just retry it, it's fine as long as the failing tests don't look related to your changes.

The change seems globally fine, but can you add an extra test to avoid regressions?

@merceyz merceyz changed the title fix(core): count all instances of ignored advisories fix(npm-cli): count all instances of ignored advisories Jan 12, 2023
@B4ckslash
Copy link
Contributor Author

B4ckslash commented Jan 12, 2023

I guess I can, but given that there is basically no infrastructure around testing yarn npm audit (or did I just miss it?) that's gonna take a while. Just from a quick look around the integration tests I'd have to:

  • extend the integration test package server to handle audit requests,
  • add some fake packages to said server
  • add fake advisories for the new fake packages
  • write the actual tests

Could you perhaps greenlight this PR and I pinky-promise to create another PR with the tests? I understand if that's not an option, but otherwise this fix will take a fair while before being ready.

Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!


Re #5194 (comment)
Yeah, you shouldn't have to setup all the infrastructure required to test this.

@B4ckslash
Copy link
Contributor Author

So, uh.. how does this get merged then? :D

@B4ckslash
Copy link
Contributor Author

@merceyz sorry to ping you like this, but can this get merged?

@merceyz
Copy link
Member

merceyz commented Mar 23, 2023

I was giving @arcanis some time to object and since he hasn't I'm fine with merging this but the (unrelated) failing Netlify build is stopping me.

@arcanis arcanis merged commit 571a407 into yarnpkg:master Mar 23, 2023
merceyz pushed a commit that referenced this pull request Mar 23, 2023
Fix bug where ignored advisories would still count if multiple paths were affected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug?]: Ignoring NPM audit advisories fails for packages included multiple times
3 participants