G-Rath / osv-detector

MIT License
61 stars 8 forks source link

Handle gems being present multiple times with different platforms #38

Open G-Rath opened 2 years ago

G-Rath commented 2 years ago

Gems can be in the lockfile multiple times if they have different platforms - we're actually already supporting parsing this out, but currently we ignore the platform; this results in the gem being added & reported twice.

I think the main thing to do is decide what the best behaviour actually is - we could add the platform back into the version (so it'll be parsed as a build string), but it might be fine to just drop it too.

For now this shouldn't be critical as it just means we report a gem twice if it has vulnerabilities.

Patch with reproduction ``` Index: detector/parsers/parse-gemfile-lock.go IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/detector/parsers/parse-gemfile-lock.go b/detector/parsers/parse-gemfile-lock.go --- a/detector/parsers/parse-gemfile-lock.go (revision fd42ffc0018908084de1f6b27b7cca08eabd07e0) +++ b/detector/parsers/parse-gemfile-lock.go (date 1646523300530) @@ -66,6 +66,7 @@ } if len(spaces) == 4 { + fmt.Printf("%s@%s: %s\n", results[2], results[3], results[4]) parser.addDependency(results[2], results[3]) } } Index: detector/parsers/parse-gemfile-lock_test.go IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/detector/parsers/parse-gemfile-lock_test.go b/detector/parsers/parse-gemfile-lock_test.go --- a/detector/parsers/parse-gemfile-lock_test.go (revision fd42ffc0018908084de1f6b27b7cca08eabd07e0) +++ b/detector/parsers/parse-gemfile-lock_test.go (date 1646523244139) @@ -619,3 +619,36 @@ }, }) } + +func TestParseGemfileLock_SameGemDifferentPlatforms(t *testing.T) { + t.Parallel() + + packages, err := parsers.ParseGemfileLock("fixtures/bundler/same-gem-different-platforms.lock") + + if err != nil { + t.Errorf("Got unexpected error: %v", err) + } + + expectPackages(t, packages, []parsers.PackageDetails{ + { + Name: "nokogiri", + Version: "1.10.9", + Ecosystem: parsers.BundlerEcosystem, + }, + { + Name: "nokogiri", + Version: "1.10.9-x86-mingw32", + Ecosystem: parsers.BundlerEcosystem, + }, + { + Name: "rails-dom-testing", + Version: "2.0.3", + Ecosystem: parsers.BundlerEcosystem, + }, + { + Name: "xpath", + Version: "3.2.0", + Ecosystem: parsers.BundlerEcosystem, + }, + }) +} Index: detector/parsers/fixtures/bundler/same-gem-different-platforms.lock IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/detector/parsers/fixtures/bundler/same-gem-different-platforms.lock b/detector/parsers/fixtures/bundler/same-gem-different-platforms.lock new file mode 100644 --- /dev/null (date 1646523080168) +++ b/detector/parsers/fixtures/bundler/same-gem-different-platforms.lock (date 1646523080168) @@ -0,0 +1,29 @@ +GEM + remote: https://rubygems.org/ + specs: + nokogiri (1.10.9) + mini_portile2 (~> 2.4.0) + nokogiri (1.10.9-x86-mingw32) + mini_portile2 (~> 2.4.0) + rails-dom-testing (2.0.3) + activesupport (>= 4.2.0) + nokogiri (>= 1.6) + xpath (3.2.0) + nokogiri (~> 1.8) + +PLATFORMS + ruby + x86-mingw32 + +DEPENDENCIES + bootstrap_form (>= 4.0.0) + byebug + capybara (~> 3.30) + devise + tzinfo-data + uglifier (>= 1.3.0) + web-console (>= 3.3.0) + webpacker (>= 4.0.x) + +BUNDLED WITH + 2.0.1 ```
G-Rath commented 2 years ago

Actually I just realised bundler-audit has the exact same behaviour, so this really shouldn't be a huge issue - still, worth seeing if we could handle it smarter.