fix: ensure that npm dependencies retain their "production" grouping #939
Conversation
G-Rath
force-pushed
the
groups-are-unique
branch
from
fc018d5
to
336c337
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #939 +/- ##
==========================================
+ Coverage 63.62% 63.69% +0.06%
==========================================
Files 146 146
Lines 11947 11961 +14
==========================================
+ Hits 7601 7618 +17
+ Misses 3878 3876 -2
+ Partials 468 467 -1 ☔ View full report in Codecov by Sentry. |
| // the merge happens almost as you'd expect, except that if either given packages | ||
| // belong to no groups, then that is the result since it indicates the package | ||
| // is implicitly a production dependency. | ||
| func mergeNpmDepsGroups(a, b PackageDetails) []string { |
There was a problem hiding this comment.
it seems we only need DepGroups so can the func be
func mergeNpmDepGroups(a, b []string) []string
?
There was a problem hiding this comment.
It could but that'd mean every caller of this function would have to explicitly access DepGroups themselves - so it's cleaner for us to take PackageDetails and do that accessing ourselves.
|
|
||
| for name, detail := range m1 { | ||
| details[name] = detail | ||
| type npmPackageDetailsMap map[string]PackageDetails |
There was a problem hiding this comment.
maybe add a comment stating that the key is name and version
oliverchang
requested review from
michaelkedar
and removed request for
josieang and
hogo6002
There was a problem hiding this comment.
I'll defer this to @michaelkedar, who is our npm expert. WDYT of this approach?
|
Looking a bit into this, there's another awkward edge case with optional prod dependencies & non-optional dev dependencies (
https://docs.npmjs.com/cli/v9/configuring-npm/package-lock-json#packages Our groups parser doesn't seem to get this correct at the moment: osv-scanner/pkg/lockfile/parse-npm-lock.go Lines 125 to 137 in 9702901 So I guess we ought to differentiate between "optional in prod, required in dev" ( This whole thing kind of comes to some thoughts I've been having on counting vulnerabilities and how or whether we should be deduplicating vulnerabilities in an npm lockfile in the first place. I feel like it'd generally be clearer to have one entry per vulnerability ID, with a complete list of instances of affected packages. |
|
I don't think I disagree with that but frankly it makes my head spin a bit 😅 Personally I feel like it's probably safest to at least start by just treating groups as part of the uniqueness of a package - I'm pretty sure for most ecosystems that won't change anything because you can only have instance of a package regardless of group, and for ecosystems like NPM it'd mean you would just end up with some more packages in the output. Then we can work on how to make the output smarter without worrying about the current version of the cli giving false negatives in the meantime |
|
note to self: consider exploring if an empty string could be useful here - i.e. if a package is not optional or dev, then its (though thinking about it, I think that could just lead back to this same solution - I think either way, its probably correct more as it needs to be handled within the extractor rather than at the outputter level right now) |
This resolves an inconsistency in the scanner output for npm packages that appear in the same tree multiple times but in different groups; this happens because the table outputter deduplicates on groups on the assumption that a package can only appear in a single group which is incorrect for the NPM ecosystem.
To address this, I've introduced an internal map type that ensures groups are merged when a package is added, with the twist that if either instance of a package being merged is in no groups then that is the result of the merge because implicitly that means an instance of the package is in the "production" group which takes priority over the other groups.
This is something that should most likely be improved on the future, but right now this fix should be good enough to ship since afaik it doesn't impact any other ecosystem and groups are something of a PoC given we can't resolve that information richly enough across all ecosystems yet.
I think this technically could impact
pnpmas well, but none of our fixtures gave a good indicator and the latest lockfile version doesn't have that information anymore so frankly I'm not worrying about it at this point.Resolves #924