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

[plugin-npm-cli]: Add ability to exclude packages, or ignore specific advisories in yarn npm audit #4356

Merged
merged 10 commits into from
May 22, 2022

Conversation

hughdavenport
Copy link
Contributor

@hughdavenport hughdavenport commented Apr 13, 2022

What's the problem this PR addresses?
Closes #4355

...

How did you fix it?
This PR adds a --exclude flag to the yarn npm audit command in the
nmp-cli plugin. This flag can be passed multiple times, and any package
listed will be removed from the list of packages audited.

This PR also adds a --ignore flag to yarn npm audit, which is an array
of ID's to ignore from the audit report.

In addition, the ID is presented in the tree output (as well as the JSON).
...

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.

@arcanis
Copy link
Member

arcanis commented Apr 13, 2022

Syntax-wise I'd prefer if we had --include / --exclude flags, wdyt?

https://github.com/yarnpkg/berry/blob/master/packages/plugin-workspace-tools/sources/commands/foreach.ts#L98-L104

@merceyz
Copy link
Member

merceyz commented Apr 13, 2022

This would probably be better suited as a configuration setting rather than a CLI flag.

@hughdavenport
Copy link
Contributor Author

--exclude works for me. I have pushed a new patch for that. It opens up the possibility of --ignore for actual advisories (similar to how python's safety uses --ignore).

I used cli flag to stay consistent with other audit tools out there. Potentially having both cli flag and configuration setting would benefit more?

@hughdavenport hughdavenport changed the title [plugin-npm-cli]: Add --ignore flag to yarn npm audit [plugin-npm-cli]: Add --exclude flag to yarn npm audit Apr 13, 2022
@hughdavenport hughdavenport changed the title [plugin-npm-cli]: Add --exclude flag to yarn npm audit [plugin-npm-cli]: Add ability to exclude packages, or ignore specific advisories in yarn npm audit Apr 14, 2022
@hughdavenport
Copy link
Contributor Author

I've pushed all the features I was thinking of to this PR. I'm happy to do any further cleanup (such as adding these flags as configuration settings as well) if you would like.

Let me know what you think :)

@jdanil
Copy link
Contributor

jdanil commented Apr 14, 2022

This looks like a great addition. We've been running yarn's audit via audit-ci just so we can filter out some advisories.

My initial thoughts would have been for this to be exposed as configuration (as @merceyz suggested) instead of (or in addition to) CLI options.

Either way, looking forward to being able to drop one more dependency :)

@hughdavenport
Copy link
Contributor Author

Thanks @jdanil. Is there any examples of where configuration options have been used in the past that you can point me at? I can then update this to use both CLI flags and configuration options.

@jdanil
Copy link
Contributor

jdanil commented Apr 14, 2022

Yup sure. Here is an example where audit was updated to support a custom registry.

#3583

Also, don't forget to update the yarnrc schema like I did 😅

#3621

@hughdavenport
Copy link
Contributor Author

Apologies for the delay. I was on vacation.

I've pushed a patch to load these from a configuration file as well as via cli flags. Let me know if you need anything more to get this through :)

@arcanis
Copy link
Member

arcanis commented Apr 26, 2022

Made a few stylistic updates but overall looks good to me, thanks! Since audit isn't currently tested I won't block on adding ones, but, if you're interesting in potential followups, adding a few tests on this command would be very appreciated 😃

@hughdavenport
Copy link
Contributor Author

Ah yes I was thinking of tests before I left on vacation. I'll make up some and push those today. Good to have them there for future people to build off :).

@hughdavenport
Copy link
Contributor Author

hughdavenport commented Apr 27, 2022

Can someone point me in the direction of getting the build to work after adding micromatch. I got around it by commenting out the import before running yarn. I had a look at commit c4abfbf which introduced globbing into plugin-workspace-tool, and looks like I've done everything it does. I'm at a bit of a loss.
Worked it out :)

Will work on tests another day.

@hughdavenport hughdavenport force-pushed the yarn-npm-audit-ignores branch 5 times, most recently from 6e1de9a to 781c8bc Compare May 3, 2022 05:41
hughdavenport and others added 6 commits May 10, 2022 14:40
This patch adds a `--exclude` flag to the `yarn npm audit` command in the
`nmp-cli` plugin. This flag can be passed multiple times, and any package
listed will be removed from the list of packages audited.
This patch adds a `--ignore` flag to `yarn npm audit`, which is an array
of ID's to ignore from the audit report.

In addition, the ID is presented in the tree output (as well as the JSON).
Adds configuration options to specify packages to exclude from `yarn npm audit`
and to specify advisories to ignore from the results.
@hughdavenport
Copy link
Contributor Author

Hey @arcanis @merceyz @jdanil,

I've made all the changes suggested, and also made a few test cases (though could definitely do more). I'm not overly familiar with Jest for testing so I think I may leave it as is. Would you be happy merging this as is, and I can make another PR when I find motivation to finish the rest of the test cases. I'd be keen to get this into production so that we can use it in our pipelines :).

Cheers,

Hugh

@hughdavenport
Copy link
Contributor Author

Hi folks, just checking in to see what needs doing to get this merged.

Cheers,

Hugh

@arcanis
Copy link
Member

arcanis commented May 22, 2022

I think it should be good for this iteration, thanks! If you find time for a followup, implementing the integration tests would be really nice 😃

@arcanis arcanis merged commit 0a2261d into yarnpkg:master May 22, 2022
@quinnturner
Copy link

Author of audit-ci here, glad Yarn made progress here! This makes Yarn the first package manager to natively support allowlisting advisories.

One decision that audit-ci made in its recent v6.0.0 release was to use GitHub advisory identifiers rather than NPM identifiers (the number identifier used in this PR). Off the top of my head, I forget which package managers this applied to, but we've noticed that NPM identifiers are unstable. The same GitHub advisory will NPM change identifiers for no reason. This makes allowlisting/excluding those advisories potentially unstable. I am unsure whether this instability affects Yarn Berry but it might be worth reviewing.

Similarly, GitHub identifiers are a bit more useful now that NPM's advisory database redirects to GitHub's advisory database. Personally, I would recommend supporting the GitHub identifier and suggest it over the NPM identifier.

@hughdavenport hughdavenport deleted the yarn-npm-audit-ignores branch May 30, 2022 23:24
@hughdavenport
Copy link
Contributor Author

Hey folks,

I'm just wondering whether I missed something from this MR or not. This doesn't seem to have landed in the stable release yet (or I may just misunderstand how the releases work). Can someone let me know what needs doing to get this into stable?

Cheers,

Hugh

@merceyz
Copy link
Member

merceyz commented Sep 12, 2022

This doesn't seem to have landed in the stable release yet [...] Can someone let me know what needs doing to get this into stable?

We started work on v4 and only backported bugfixes, not new features, though considering we haven't had a feature release in about seven months we should probably reconsider that.

@hughdavenport
Copy link
Contributor Author

Ah OK. If I had known that when I started the PR process I probably wouldn't have contributed anything and instead done something in-house to meet the same objectives. Perhaps being upfront with contributors about timelines would be nice next time. Not everyone runs bleeding edge. I hope that some day my team will be able to use this feature :)

@hughdavenport
Copy link
Contributor Author

@merceyz have you had any more consideration in doing a feature release? I'm hoping that I'll be able to make use of this PR that I spent time contributing to your project.

@arcanis
Copy link
Member

arcanis commented Oct 20, 2022

@hughdavenport you could very well use the RCs, which are just as stable as the 3.x release (only difference being that we have a couple more breaking changes to land, so in theory further upgrades may worth a look at the migration notes; in practice those changes will be rare).

As you know we're currently working on a major release, and any cherry-picking takes resources we'd prefer to spend on progressing towards the major. That's why we publish the RCs, to provide a reasonable middle ground for people wanting to use the bleeding edge features.

Comment on lines +2 to +3
"@yarnpkg/cli": patch
"@yarnpkg/plugin-npm-cli": patch
Copy link
Member

@merceyz merceyz Nov 3, 2022

Choose a reason for hiding this comment

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

These should have been marked as minor as this added a feature.

merceyz pushed a commit that referenced this pull request Nov 3, 2022
… advisories in `yarn npm audit` (#4356)

* [plugin-npm-cli]: Add ability to exclude packages from `yarn npm audit`

This patch adds a `--exclude` flag to the `yarn npm audit` command in the
`nmp-cli` plugin. This flag can be passed multiple times, and any package
listed will be removed from the list of packages audited.

* [plugin-npm-cli] Add ability to ignore advisories in `yarn npm audit`

This patch adds a `--ignore` flag to `yarn npm audit`, which is an array
of ID's to ignore from the audit report.

In addition, the ID is presented in the tree output (as well as the JSON).

* Version bump

* chore: Fix types

* [plugin-npm-cli] Add configuration options for --exclude and --ignore

Adds configuration options to specify packages to exclude from `yarn npm audit`
and to specify advisories to ignore from the results.

* Update audit.ts

* Update audit.ts

* [plugin-npm-cli] Update docs

* [plugin-npm-cli] Add support for glob patterns in --exclude and --ignore

* [plugin-npm-cli] Add some unit tests and stubs for integration tests

Co-authored-by: Maël Nison <nison.mael@gmail.com>
@merceyz
Copy link
Member

merceyz commented Nov 16, 2022

@hughdavenport This has now been backported and released in v3.3.0.

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.

[Feature] Add support to ignore packages in yarn npm audit
5 participants