feat: support vulnerabilities.ignore in package overrides
#1268
Conversation
| | UNKNOWN | | https://github.com/flutter/buildroot.git | | fixtures/locks-insecure/osv-scanner-flutter-deps.json | | ||
| | UNKNOWN | | https://github.com/brendan-duncan/archive.git | | fixtures/locks-insecure/osv-scanner-flutter-deps.json | | ||
| | UNKNOWN | | https://chromium.googlesource.com/chromium/src | | fixtures/locks-insecure/osv-scanner-flutter-deps.json | |
There was a problem hiding this comment.
note: I realised as part of this that git-based dependencies are a bit of an edge case for the more composition-based config override logic
internally they don't have an ecosystem which is reflected in the license violation table but the vulnerability table uses GIT instead, but then ecosystem = "GIT" does not match these while ecosystem = "" just ignores everything.
I don't know if its worth fixing but we could account for this by mapping a value of GIT as a special case when checking for overrides; alternatively (and maybe the better short-term change) we could change the vulnerabilities table output to not have this logic so it at least matches the licenses table...
| ignore = true # Ignore this package completely, including both reporting vulnerabilities and license violations | ||
| vulnerability.ignore = true # Ignore vulnerabilities for this package, while still checking the license (if not also ignored) | ||
| license.ignore = true # Ignore the license of the package, while still checking for vulnerabilities (if not also ignored) | ||
| license.override = ["MIT", "0BSD"] # Override the license of the package, if it is not ignored from license scanning completely |
There was a problem hiding this comment.
note: I think overall this reads better using the term "reporting" and "checking"
imo that reduces the expectation that these options mean the underlying actions for the matched packages don't happen at all, as that's not the case (i.e we will always include the package in the API calls to determine if there are vulnerabilities, it's just if there are we ignore them)
There was a problem hiding this comment.
Huh I thought #1206 ignored packages from being sent to the API in the first place.
There was a problem hiding this comment.
oh yeah you're right I think, though I still think my wording here is fine because really that's an optimization - our public contract in this sense I think is still (at least right now) "we won't report" not "we don't check at all"
| // ShouldIgnorePackageVulnerabilities determines if the given package should have its vulnerabilities ignored based on override entries in the config | ||
| func (c *Config) ShouldIgnorePackageVulnerabilities(pkg models.PackageVulns) bool { | ||
| overrides, _ := c.filterPackageVersionEntries(pkg, func(e PackageOverrideEntry) bool { | ||
| return e.Vulnerability.Ignore | ||
| }) | ||
|
|
||
| return overrides | ||
| } |
There was a problem hiding this comment.
note: the names here don't really make much sense but I think that's a bigger issue that should be tackled at the package level
I also didn't keep the interface matching the other functions because I couldn't think of any unsurprising way of leveraging "overrides for vulnerabilities" like with licenses, and I feel like having this be as specific as possible should make it easier to do non-breaking changes in future
i.e. my hope is that if in future we do think up more uses for this struct, we'll be able to introduce ShouldOverridePackageVulnerabilities and co, and just deprecate this method
Given this is a public method, I am happy to switch this to returning the override entry to be on the safe side anyway given there's no major downside to doing that straight away even if we never use it
| pkg.Groups = grouper.Group(grouper.ConvertVulnerabilityToIDAliases(pkg.Vulnerabilities)) | ||
| for i, group := range pkg.Groups { | ||
| pkg.Groups[i].MaxSeverity = output.MaxSeverity(group, pkg) | ||
| configToUse := configManager.Get(r, rawPkg.Source.Path) |
There was a problem hiding this comment.
note: it might make sense to pull this out of the condition since we do the exact same thing for license scanning below
but I've purposely not done that for now as I think the performance impact will be pretty minimal, it should be easily doable in a follow-up PR, and there might be a reason why we don't want to do that...
G-Rath
force-pushed
the
config/vuln-ignore
branch
from
fa2e338
to
cca673b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1268 +/- ##
==========================================
- Coverage 68.53% 68.46% -0.08%
==========================================
Files 175 175
Lines 16804 16811 +7
==========================================
- Hits 11516 11509 -7
- Misses 4661 4672 +11
- Partials 627 630 +3 ☔ View full report in Codecov by Sentry. |
G-Rath
force-pushed
the
config/vuln-ignore
branch
3 times, most recently
from
c5e1817
to
a46e383
Compare
G-Rath
force-pushed
the
config/vuln-ignore
branch
from
a46e383
to
860e6af
Compare
This implements the ability to ignore vulnerabilities in a matching group of packages while still reporting license violations, as the inverse to
license.ignore.Resolves #1226