osv-scanner: fix reports of vulnerabilities when multiple versions of @grafana packages are installed
#419
Conversation
github-project-automation
bot
moved this to 📬 Triage
in Plugins Platform / Grafana Community
academo
moved this from 📬 Triage
to 🔬 In review
in Plugins Platform / Grafana Community
academo
requested review from
leventebalogh,
oshirohugo,
toddtreece and
wbrowne
academo
force-pushed
the
academo/fix-osv-scanner-multi-version-dependency
branch
from
b11b4e0 to
ca6ab91
Compare
…er-multi-version-dependency
| for _, pkg := range allPackages { | ||
| if GrafanaPackages[pkg.Name] && !processedNames[pkg.Name] { | ||
| processedNames[pkg.Name] = true | ||
|
|
||
| allVersions := make([]lockfile.PackageDetails, 0) | ||
| for _, p := range allPackages { | ||
| if p.Name == pkg.Name { | ||
| allVersions = append(allVersions, p) | ||
| } | ||
| } | ||
|
|
||
| // should never happen | ||
| if len(allVersions) == 0 { | ||
| continue | ||
| } |
There was a problem hiding this comment.
probably we don't need to worry about performance here but better code would be: rather than iterating twice over allPackages, would be to create a map packagesToProcess[string]struct{Name,Versions} and then you iterate over that map to process them
| func expandDependenciesRecursively( | ||
| topLevel *lockfile.PackageFlattened, | ||
| allPackages []lockfile.PackageDetails, | ||
| ) { | ||
| for i := 0; i < len(topLevel.Dependencies); i++ { | ||
| if topLevel.Dependencies[i].Processed { | ||
| continue | ||
| } | ||
|
|
||
| topLevel.Dependencies[i].Processed = true | ||
| depName := topLevel.Dependencies[i].Package.Name | ||
|
|
||
| var depPackage *lockfile.PackageDetails | ||
| for j := range allPackages { | ||
| if allPackages[j].Name == depName { | ||
| depPackage = &allPackages[j] | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if depPackage == nil { | ||
| continue | ||
| } | ||
|
|
||
| for _, subDep := range depPackage.Dependencies { | ||
| // do not add them twice | ||
| if !dependencyExists(subDep.Name, topLevel.Dependencies) { | ||
| state := lockfile.DependencyState{ | ||
| Package: subDep, | ||
| Processed: false, | ||
| } | ||
| topLevel.Dependencies = append(topLevel.Dependencies, state) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
honestly, this function is really hard to read or understand (I tried for several minutes and failed 😅 ). It's mixing npm variable names (dep, package, lockfile), with iterative vars (i, j), modifying the input (topLevel) in several places, breaking the loop in other places... I don't have a suggestion because I don't know what this is doing but I'd try to make it more modular, with more comments and avoid modifying inputs.
There was a problem hiding this comment.
I know! this code is absurdly complex but, it is harder to make it more readable because the structure of lock files is deep and recursive so one have to create code like this.
I will add more comments to the code so it is easier to follow up
There was a problem hiding this comment.
do you really need a 25k file for this? It's impossible to know which parts are important, I would rather see a fake file with only the nested cases you are interested in testing.
There was a problem hiding this comment.
likely not, but it is a real lock file from a real plugin that presented this issue.
the problem is that lock files tend to be huge because they are not meant to read manually and the dependency I found that had issues has hundreds of transitive dependencies that inflate this file.
There was a problem hiding this comment.
yeah, my point is that for this to be readable the input of a test needs to be readable. Right now is: I give you this obscure blob and you have to trust me that the expected output is that it founds dompurify in this specific format. In the other hand, if this file is 10 lines long, I'm able to see where dompurify is and how the test is processing it.
Also, if I need to add something else to the logic and test it with something this file doesn't have, I wouldn't be able to modify this package-lock.json because I don't know which parts are important, so I'd need to add multi-version-npm-2, with another 25k lines of code and again that file becomes immutable for the next person.
This is just a general comment, I'm not a fan of test files representing real stuff vs minimal (fake) input that easily represents what the test is meant to cover.
There was a problem hiding this comment.
I removed most of the bulk and left it to the bare minimum but it is still far from "readable" it just now contains the minimum necessary to trigger the issues we try to test
github-project-automation
bot
moved this from 🔬 In review
to 🚀 Shipped
in Plugins Platform / Grafana Community
2 participants
Fixes: #418
The logic of the filtering remains the same but it is now expanded to find all transitive dependencies of all versions of a package inside the grafana dependencies list
e.g.
@grafana/data@v1brings dependencies:a,b, andc@grafana/data@v2brings dependencies:a,e, andfin the old version we'd only report
a,b, andcas transitive dependencies of@grafana/datawith this changes we'll report:a,b,c,eandf