search

google / osv-scanner

Vulnerability scanner written in Go which uses the data provided by https://osv.dev
https://google.github.io/osv-scanner/
Apache License 2.0
6.02k stars 337 forks source link

fix: only care about ecosystem suffix if present in both ecosystems when determining equality #1007

Closed G-Rath closed 1 week ago

G-Rath commented 1 month ago

This changes how we compare ecosystems so that the suffix is only considered when it is present for both ecosystems being compared, since we can't reliably extract that.

Resolves #769

codecov-commenter commented 1 month ago

Codecov Report

Attention: Patch coverage is 75.00000% with 3 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@46aee59). Learn more about missing BASE report.

Files Patch % Lines
internal/utility/vulns/vulnerability.go 75.00% 2 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1007 +/- ## ======================================= Coverage ? 65.30% ======================================= Files ? 150 Lines ? 12535 Branches ? 0 ======================================= Hits ? 8186 Misses ? 3884 Partials ? 465 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

G-Rath commented 3 weeks ago

Turns out we're not accurately sorting the table output at least for local - I'll tackle that somewhere...

G-Rath commented 2 weeks ago

I cannot for the life of me replicate that sorting difference locally 😕

hogo6002 commented 2 weeks ago

I cannot for the life of me replicate that sorting difference locally 😕

I tried running update_snaps for this branch locally, and it did change the order, but it's still a little different from the Ubuntu results. image

G-Rath commented 2 weeks ago

Yeah, it seems that every couple of hours the order changes - I'm not sure if its coinciding with something on the osv.dev side like work being landed or if it's just some clock-shift based randomness, but it seems to be "hours" based rather than "seconds" based...

hogo6002 commented 2 weeks ago

Yeah, it seems that every couple of hours the order changes - I'm not sure if its coinciding with something on the osv.dev side like work being landed or if it's just some clock-shift based randomness, but it seems to be "hours" based rather than "seconds" based...

Yeah I see what you mean. I just tested again and the result shows different.

hogo6002 commented 2 weeks ago

I had a look with @michaelkedar on this issue. We noticed that the result changes after the exporter cron job publishes a new all.zip. We think this is because the exporter runs in parallel (https://github.com/google/osv.dev/pull/2167), which may cause the order to be slightly different each time. To solve this, I guess we need to either sort the all.zip on the OSV.dev side or order all vulnerabilities alphabetically on the OSV-Scanner side. What do you think @another-rex

G-Rath commented 2 weeks ago

oohh good find - fwiw locally I've been playing around with sorting the table rows within packages by their url though that got blocked by it being an interface{}; I've not gotten back to it yet so might be an easy fix

another-rex commented 2 weeks ago

Hmm... is this behaviour new? All.zip is built concurrently, but that has always been the case if I understand correctly.

hogo6002 commented 2 weeks ago

Hmm... is this behaviour new? All.zip is built concurrently, but that has always been the case if I understand correctly.

I don't think this is a new behaviour. Even though all.zip is built concurrently, the files are usually modified in alphabetical order, just a few are out of place. That's probably why most of our tests are passing. But this PR has a bunch of vulnerabilities with adjacency IDs (like 9840, 9841, 9842, and 9843), which is probably why it's failing. But I am not 100% sure if this is the issue as the modified order doesn't really align with the scanner output order. image

another-rex commented 1 week ago

I think this just needs a flag change in the test to match the new download-offline-db flag and should be good to merge.