✨ Add --exclude-packges Flag to osv-scanner CLI

[toddtee]

This PR adds an --exclude-packages flag to the osv-scanner CLI. This flag allows users to specify a list of regex patterns to which matches of collected package names will be matched against.

If the packages are found to match the regex patterns, they will be excluded from the query to the OSV database.

Exclusions work for both lockfile and SBOM package definitions.

Excluded-packages flag will address issue #151.

[G-Rath]

This is on the right track - I've done a general review but not gone over every line yet as I think the next main question is about if the library should be accepting a slice of regexp or strings, as changing that will probably mean other code can be changed which'd make other parts of my review moot 😅

[G-Rath]

I'm softly leaning towards saying we should be taking a slice of regexps here because that is the most accurate (and thus least-error-prone) representation of the data; the reason I'm not completely sold is that I expect a number of library consumers are probably taking user input aka dynamic regexps, but for us it should mean the library doesn't have to handle "what if the string isn't a valid regexp"...

This would be a reason to keep exclusions as a public package as then downstream users could just pull that in like the CLI does rather than have to implement their own parser (then again they could also just copy and paste, which is the Go way 🤷)

[toddtee]

Hey @G-Rath, given in my brain, the user was always going to be input from the user, I naturally just assumed it was going to be a list of string. If it were going to be regexp, we would still need to take the list of string from the flag and parse them as regexp before we assign them to the ScannerActions struct? 🤔

If that is the case, kind of feels like we are just swapping the ordering of how we process it. I might be missing the point though 😅

[G-Rath]

What you've said is correct but the code base is made up of both the CLI and the library - and it's the CLI that handles the fact the input is coming in as a string (which is because that's just how data comes in from terminals)

The library on the other hand can just accept regexp so it doesn't need to be responsible for handling the edge cases that come with parsing strings into regexp

[toddtee]

Thanks @G-Rath that makes sense. Let me shuffle some things and see what happens. 🔀

[toddtee]

Updated @G-Rath.

The ScannerActions struct now has the Exclusions field as a []*regex.regexp.

[oliverchang]

Thanks for this! Our team has considered this in the past as a config file rule (https://google.github.io/osv-scanner/configuration/).

Could we understand if a config file would also work for your use case? Or are there reasons why you'd prefer a CLI flag?

[hayleycd]

Docs look good with a couple of comments.

[toddtee]

Hi @oliverchang, thanks for checking out my PR!

Could we understand if a config file would also work for your use case? Or are there reasons why you'd prefer a CLI flag?

Ideally, I think having the exclusions parameter both as a CLI flag and a configuration parameter would be ideal.

I like the idea of having the CLI flag as I personally believe that CLI tools should have good levels of interactivity for a human user. For example, if I as a developer want to quickly update a project and I have a particular package that is giving me issues that I am blocked on updating or don't want to query for whatever reason, it is nice to have the flag as a quick option to interact with the tool as I need it. Or I could even use the flag to test the scan results for when I am developing the regex patterns that will eventually end up in the configuration file! 🤣

However... let's now talk automation, not human interaction. If I incorporate the osv-scanner as part of my project's CI/CD pipeline or automated security checks, having the flag available is good, however having a configuration parameter is better. That way, maintainers of the project can configure what exclusions they like and perhaps even have comments in the config file as to why they are there. I see the configuration parameter for more permanent and/or governing exclusion purposes.

TL;DR, I see the CLI flag for quick developer use for once-offs or short term purposes and the configuration file more for automated processes and more permanent solutions.

I don't know how challenging incorporating the configuration file into my current offering would be in the short term...

However, as I am a supporter of one PR including one change only. I would propose that this PR maintain adding the functionality of a CLI flag exclusively, and perhaps create a related Issue and follow-up PR to add exclusions as a configuration option as well.

The benefit of this is the tool can still offer the ability to exclude packages from the query in the short term and deliver value to those that need it. A configuration parameter will extend that functionality offering further.

I hope that answers your question 🙏

[oliverchang]

Hi @oliverchang, thanks for checking out my PR!

Could we understand if a config file would also work for your use case? Or are there reasons why you'd prefer a CLI flag?

Ideally, I think having the exclusions parameter both as a CLI flag and a configuration parameter would be ideal.

I like the idea of having the CLI flag as I personally believe that CLI tools should have good levels of interactivity for a human user. For example, if I as a developer want to quickly update a project and I have a particular package that is giving me issues that I am blocked on updating or don't want to query for whatever reason, it is nice to have the flag as a quick option to interact with the tool as I need it. Or I could even use the flag to test the scan results for when I am developing the regex patterns that will eventually end up in the configuration file! 🤣

Thanks, that makes sense. We've generalised shied away from having multiple ways to achieve the same thing out of a desire to keep the CLI simple to understand, but having a way to quickly iterate/test does make sense.

Could we perhaps rename the flag to --exclude-packages though? --exclude is a little ambiguous on its own.

@another-rex @G-Rath Do you think there's value in maintaining this as a CLI flag?

However... let's now talk automation, not human interaction. If I incorporate the osv-scanner as part of my project's CI/CD pipeline or automated security checks, having the flag available is good, however having a configuration parameter is better. That way, maintainers of the project can configure what exclusions they like and perhaps even have comments in the config file as to why they are there. I see the configuration parameter for more permanent and/or governing exclusion purposes.

Big +1 to this. Having this stored as the repo would help maintain state around exclusions and reduce duplicate triage efforts for the project.

TL;DR, I see the CLI flag for quick developer use for once-offs or short term purposes and the configuration file more for automated processes and more permanent solutions.

I don't know how challenging incorporating the configuration file into my current offering would be in the short term...

However, as I am a supporter of one PR including one change only. I would propose that this PR maintain adding the functionality of a CLI flag exclusively, and perhaps create a related Issue and follow-up PR to add exclusions as a configuration option as well.

Yep! I've filed #670 for this.

The benefit of this is the tool can still offer the ability to exclude packages from the query in the short term and deliver value to those that need it. A configuration parameter will extend that functionality offering further.

I hope that answers your question 🙏

[toddtee]

@oliverchang, I think --exclude-packages is better as it is more explicit to what the functionality actually is. I have updated the flag and documentations to reflect this. 👍

[G-Rath]

Do you think there's value in maintaining this as a CLI flag?

I'm not sure - I'm not getting super strong "omg yes this'd be great" vibes from my gut, but that doesn't meant it still wouldn't be useful.

However...

For example, if I as a developer want to quickly update a project and I have a particular package that is giving me issues that I am blocked on updating or don't want to query for whatever reason, it is nice to have the flag as a quick option to interact with the tool as I need it

I'm not sure that is as useful as you think: since we're agreed that it's best to capture this in config long-term, in this situation you'd be running the scanner locally in most cases, meaning you could just mentally ignore the results too - I know that is not as nice as having them omitted but not sure if its "support a new flag forever" nice.

I could even use the flag to test the scan results for when I am developing the regex patterns that will eventually end up in the configuration file!

That does seem like a useful reason to have the flag, though I don't think it would be that much overhead to just be using the config file; that would also mean less risk that a specific shell might get confused by the regexp.

---

I don't think this should be taken as "never will we ever have this flag", but it feels like it would probably be best to start life via the config file