Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove platform duplicates #356

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Remove platform duplicates #356

wants to merge 3 commits into from

Conversation

ylecuyer
Copy link

Hello, lately we noticed some gems having duplicates in the report because of the various platform.

Here is an example with nokogiri:

    nokogiri (1.13.6)
      mini_portile2 (~> 2.8.0)
      racc (~> 1.4)
    nokogiri (1.13.6-x86_64-darwin)
      racc (~> 1.4)
    nokogiri (1.13.6-x86_64-linux)
      racc (~> 1.4)

And this is causing the duplicates.

As a fix, I suggest to filter and keep only the ruby platform for the check.

Yoann Lecuyer added 2 commits October 19, 2022 17:27
ylecuyer
ylecuyer commented Oct 19, 2022
View reviewed changes
spec/bundle/secure/Gemfile
@@ -1,4 +1,4 @@
source 'https://rubygems.org'

gem 'rails', '~> 5.2'
gem 'rails', '~> 7.0.4'
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need after changing the fixed commit of the advisory db

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we need to bump the rails version and the ruby-advisory-db commit, that can be done in a separate PR which I can immediately merge.

ylecuyer
ylecuyer commented Oct 19, 2022
View reviewed changes
spec/spec_helper.rb
@@ -12,7 +12,7 @@ module Fixtures
module Database
PATH = File.join(ROOT,'database')

COMMIT = '89cdde9a725bb6f8a483bca97c5da344e060ac61'
COMMIT = '137a425b9f4f30f895df8765b0e773400170803d'
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed to get latest advisory for nokogiri

@postmodern
Copy link
Member

postmodern commented Oct 19, 2022 •
edited
Loading

@ylecuyer maybe a better fix would be to print the gem's platform (if set) along with the gem name and version in the report? I think it would be bad if bundler-audit ignored an insecure gem in the Gemfile.lock because it's platform differs from the user's current ruby platform. This means bundle-audit would ensure the gems were secure for the user running it, but maybe not their coworker who's on a different ruby platform. By including the platform of the gem in the final report, we're not losing any information.

Also, under normal circumstances, you shouldn't have gems from multiple platforms in the Gemfile.lock.

source 'https://rubygems.org/'

gem 'nokogiri-ext'
gem 'nokogiri'

group :development do
  gem 'rake'
end

GEM
  remote: https://rubygems.org/
  specs:
    nokogiri (1.13.9-x86_64-linux)
      racc (~> 1.4)
    nokogiri-ext (0.1.0)
      nokogiri (~> 1.0)
    racc (1.6.0)
    rake (13.0.6)

PLATFORMS
  x86_64-linux

DEPENDENCIES
  nokogiri
  nokogiri-ext
  rake

BUNDLED WITH
   2.3.17

I suspect the multiple platforms are due to gems having explicit platform:/platforms: set in the Gemfile or multiple developers running bundle install/bundle update on different platforms then committing the Gemfile.lock?

postmodern
postmodern requested changes Oct 19, 2022
View reviewed changes
Copy link
Member

@postmodern postmodern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of these commits could be separate PRs. Also, I'm afraid that by filtering out other non-ruby platforms, bundler-audit might miss information vulnerabilities.

lib/bundler/audit/scanner.rb
@@ -224,7 +224,7 @@ def scan_specs(options={})
config.ignore
end

@lockfile.specs.each do |gem|
@lockfile.specs.select { |gem| gem.platform == "ruby" }.each do |gem|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would cause bundler-audit to ignore platform specific gems, potentially allowing vulnerabilities to slip by.

spec/scanner_spec.rb
@@ -67,7 +67,6 @@
subject { super().scan.to_a }

it "should print nothing when everything is fine" do
puts subject.inspect
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could totally be a separate PR that I could instantly merge.

spec/bundle/unpatched_gems/Gemfile.lock
mini_portile2 (~> 2.8.0)
racc (~> 1.4)
nokogiri (1.13.6-x86_64-linux)
racc (~> 1.4)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to test Gemfile.lock containing gems from multiple platforms or gems which explicit platforms set in the Gemfile, I think that should be a separate spec/bundle/ directory (ex: spec/bundle/unpatched_multi_platform/).

spec/bundle/secure/Gemfile
@@ -1,4 +1,4 @@
source 'https://rubygems.org'

gem 'rails', '~> 5.2'
gem 'rails', '~> 7.0.4'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we need to bump the rails version and the ruby-advisory-db commit, that can be done in a separate PR which I can immediately merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@postmodern postmodern postmodern requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ylecuyer @postmodern