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.

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

Fix dependency confusion issues with implicit dependencies #4609

Merged
merged 21 commits into from
May 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
1d1d8a8
Don't use global merged source in `bundle outdated`
deivid-rodriguez Mar 14, 2021
f73dd4f
No longer need to keep the global index in an instance variable
deivid-rodriguez Mar 20, 2021
33214a7
Consistently pass artifice values as strings
deivid-rodriguez May 13, 2021
8a246d5
Migrate sources specs to use the compact index
deivid-rodriguez Mar 5, 2021
eb3e401
Improve block variable name
deivid-rodriguez May 18, 2021
bfc5533
Don't exclude bundler in unmet dependencies check
deivid-rodriguez May 18, 2021
b39eed0
Remove inaccurate comment
deivid-rodriguez May 18, 2021
eda0c2f
Use `sources.default_source` directly
deivid-rodriguez May 18, 2021
a33cfa9
Remove unnecessary code
deivid-rodriguez May 19, 2021
6e4738d
Assign source requirements less times
deivid-rodriguez May 19, 2021
363665e
Improve naming in `SourceList`
deivid-rodriguez May 19, 2021
69054b5
Tweak some specs to not use the deprecated `--deployment` flag
deivid-rodriguez May 19, 2021
a18b9db
Rename `dependency_source_requirements`
deivid-rodriguez May 19, 2021
77fc277
Hold names, not dependencies, in a variable called "names"
deivid-rodriguez May 19, 2021
1004332
Fix typo
deivid-rodriguez May 21, 2021
151ceb5
Remove unnecessary loop
deivid-rodriguez May 21, 2021
cdad84c
Move source requirement logic to a separate class
deivid-rodriguez May 20, 2021
77204b4
Move rubygems aggregate source to a separate class
deivid-rodriguez May 20, 2021
32ac0ba
Fix dependency confusion issues
deivid-rodriguez May 19, 2021
9dfae92
Refactor "gem not found in source" resolver errors
deivid-rodriguez May 20, 2021
d06d4e5
More cleanups in the resolver
deivid-rodriguez May 20, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions Manifest.txt
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,9 @@ bundler/lib/bundler/source/path.rb
bundler/lib/bundler/source/path/installer.rb
bundler/lib/bundler/source/rubygems.rb
bundler/lib/bundler/source/rubygems/remote.rb
bundler/lib/bundler/source/rubygems_aggregate.rb
bundler/lib/bundler/source_list.rb
bundler/lib/bundler/source_map.rb
bundler/lib/bundler/spec_set.rb
bundler/lib/bundler/stub_specification.rb
bundler/lib/bundler/templates/.document
Expand Down
1 change: 1 addition & 0 deletions bundler/lib/bundler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ module Bundler
autoload :SharedHelpers, File.expand_path("bundler/shared_helpers", __dir__)
autoload :Source, File.expand_path("bundler/source", __dir__)
autoload :SourceList, File.expand_path("bundler/source_list", __dir__)
autoload :SourceMap, File.expand_path("bundler/source_map", __dir__)
autoload :SpecSet, File.expand_path("bundler/spec_set", __dir__)
autoload :StubSpecification, File.expand_path("bundler/stub_specification", __dir__)
autoload :UI, File.expand_path("bundler/ui", __dir__)
Expand Down
17 changes: 7 additions & 10 deletions bundler/lib/bundler/cli/outdated.rb
Original file line number Diff line number Diff line change
Expand Up @@ -146,17 +146,14 @@ def nothing_outdated_message
end

def retrieve_active_spec(definition, current_spec)
if strict
active_spec = definition.find_resolved_spec(current_spec)
else
active_specs = definition.find_indexed_specs(current_spec)
if !current_spec.version.prerelease? && !options[:pre] && active_specs.size > 1
active_specs.delete_if {|b| b.respond_to?(:version) && b.version.prerelease? }
end
active_spec = active_specs.last
end
active_spec = definition.resolve.find_by_name_and_platform(current_spec.name, current_spec.platform)
return active_spec if strict

active_spec
active_specs = active_spec.source.specs.search(current_spec.name).select {|spec| spec.match_platform(current_spec.platform) }.sort_by(&:version)
if !current_spec.version.prerelease? && !options[:pre] && active_specs.size > 1
active_specs.delete_if {|b| b.respond_to?(:version) && b.version.prerelease? }
end
active_specs.last
end

def print_gems(gems_list)
Expand Down
89 changes: 14 additions & 75 deletions bundler/lib/bundler/definition.rb
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,6 @@ def missing_specs?
Bundler.ui.debug "The definition is missing #{missing.map(&:full_name)}"
true
rescue BundlerError => e
@index = nil
@resolve = nil
@specs = nil
@gem_version_promoter = nil
Expand Down Expand Up @@ -287,50 +286,6 @@ def resolve
end
end

def index
Copy link

@rmacklin rmacklin Jun 8, 2021

Choose a reason for hiding this comment

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

@deivid-rodriguez, it seems that the removal of this index method broke https://rubygems.org/gems/gemsurance as that method was used here, resulting in a NoMethodError on bundler v2.2.18:

undefined method `index' for #<Bundler::Definition:0x00007fd7dd23a958> (NoMethodError)

Given that this was removed in a patch version of bundler (the method existed in bundler v2.2.17), I'm guessing it wasn't considered public API in the first place. But I figured I'd mention it in case it was okay to use the index method externally and this breakage was actually unintended.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hei @rmacklin, thanks for letting me know.

Yeah, this is not public API. The method was using a merged index (gems coming from different sources being used indifferently), which was the underlying problem causing "dependency confusion". So I decided to remove it since I think it was making bundle outdated suggest unexpected (potentially insecure) gems.

I guess gemasurance can adapt 1d1d8a8 to its needs: use the same source as the currently resolved spec instead of using a merged source (note that commit caused a regression fixed here: 64168eb.

Copy link

@rmacklin rmacklin Jun 10, 2021

Choose a reason for hiding this comment

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

Thanks for the confirmation!

I think we may be able to replace gemsurance with a combination of bundler-audit and bundle outdated

@index ||= Index.build do |idx|
dependency_names = @dependencies.map(&:name)

sources.all_sources.each do |source|
source.dependency_names = dependency_names - pinned_spec_names(source)
idx.add_source source.specs
dependency_names.concat(source.unmet_deps).uniq!
end

double_check_for_index(idx, dependency_names)
end
end

# Suppose the gem Foo depends on the gem Bar. Foo exists in Source A. Bar has some versions that exist in both
# sources A and B. At this point, the API request will have found all the versions of Bar in source A,
# but will not have found any versions of Bar from source B, which is a problem if the requested version
# of Foo specifically depends on a version of Bar that is only found in source B. This ensures that for
# each spec we found, we add all possible versions from all sources to the index.
def double_check_for_index(idx, dependency_names)
pinned_names = pinned_spec_names
loop do
idxcount = idx.size

names = :names # do this so we only have to traverse to get dependency_names from the index once
unmet_dependency_names = lambda do
return names unless names == :names
new_names = sources.all_sources.map(&:dependency_names_to_double_check)
return names = nil if new_names.compact!
names = new_names.flatten(1).concat(dependency_names)
names.uniq!
names -= pinned_names
names
end

sources.all_sources.each do |source|
source.double_check_for(unmet_dependency_names)
end

break if idxcount == idx.size
end
end
private :double_check_for_index

def has_rubygems_remotes?
sources.rubygems_sources.any? {|s| s.remotes.any? }
end
Expand Down Expand Up @@ -539,14 +494,6 @@ def most_specific_locked_platform
end
end

def find_resolved_spec(current_spec)
specs.find_by_name_and_platform(current_spec.name, current_spec.platform)
end

def find_indexed_specs(current_spec)
index[current_spec.name].select {|spec| spec.match_platform(current_spec.platform) }.sort_by(&:version)
end

attr_reader :sources
private :sources

Expand All @@ -563,6 +510,10 @@ def unlocking?

private

def precompute_source_requirements_for_indirect_dependencies?
sources.non_global_rubygems_sources.all?(&:dependency_api_available?) && sources.no_aggregate_global_source?
end

def current_ruby_platform_locked?
return false unless generic_local_platform == Gem::Platform::RUBY

Expand Down Expand Up @@ -688,9 +639,9 @@ def converge_rubygems_sources
changes = false

# If there is a RubyGems source in both
locked_gem_sources.each do |locked_gem|
locked_gem_sources.each do |locked_gem_source|
# Merge the remotes from the Gemfile into the Gemfile.lock
changes |= locked_gem.replace_remotes(actual_remotes, Bundler.settings[:allow_deployment_source_credential_changes])
changes |= locked_gem_source.replace_remotes(actual_remotes, Bundler.settings[:allow_deployment_source_credential_changes])
end

changes
Expand Down Expand Up @@ -909,26 +860,22 @@ def expand_dependency_with_platforms(dep, platforms)
end

def source_requirements
# Load all specs from remote sources
index

# Record the specs available in each gem's source, so that those
# specs will be available later when the resolver knows where to
# look for that gemspec (or its dependencies)
source_requirements = { :default => sources.default_source }.merge(dependency_source_requirements)
source_requirements = if precompute_source_requirements_for_indirect_dependencies?
{ :default => sources.default_source }.merge(source_map.all_requirements)
else
{ :default => Source::RubygemsAggregate.new(sources, source_map) }.merge(source_map.direct_requirements)
end
metadata_dependencies.each do |dep|
source_requirements[dep.name] = sources.metadata_source
end
source_requirements[:global] = index unless Bundler.feature_flag.disable_multisource?
source_requirements[:default_bundler] = source_requirements["bundler"] || source_requirements[:default]
source_requirements[:default_bundler] = source_requirements["bundler"] || sources.default_source
source_requirements["bundler"] = sources.metadata_source # needs to come last to override
source_requirements
end

def pinned_spec_names(skip = nil)
dependency_source_requirements.reject {|_, source| source == skip }.keys
end

def requested_groups
groups - Bundler.settings[:without] - @optional_groups + Bundler.settings[:with]
end
Expand Down Expand Up @@ -984,16 +931,8 @@ def equivalent_rubygems_remotes?(source)
Bundler.settings[:allow_deployment_source_credential_changes] && source.equivalent_remotes?(sources.rubygems_remotes)
end

def dependency_source_requirements
@dependency_source_requirements ||= begin
source_requirements = {}
default = sources.default_source
dependencies.each do |dep|
dep_source = dep.source || default
source_requirements[dep.name] = dep_source
end
source_requirements
end
def source_map
@source_map ||= SourceMap.new(sources, dependencies)
end
end
end
1 change: 0 additions & 1 deletion bundler/lib/bundler/feature_flag.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ def self.settings_method(name, key, &default)
settings_flag(:auto_clean_without_path) { bundler_3_mode? }
settings_flag(:cache_all) { bundler_3_mode? }
settings_flag(:default_install_uses_path) { bundler_3_mode? }
settings_flag(:disable_multisource) { bundler_3_mode? }
settings_flag(:forget_cli_options) { bundler_3_mode? }
settings_flag(:global_gem_cache) { bundler_3_mode? }
settings_flag(:path_relative_to_cwd) { bundler_3_mode? }
Expand Down
3 changes: 1 addition & 2 deletions bundler/lib/bundler/index.rb
Original file line number Diff line number Diff line change
Expand Up @@ -122,10 +122,9 @@ def spec_names
names
end

# returns a list of the dependencies
def unmet_dependency_names
dependency_names.select do |name|
name != "bundler" && search(name).empty?
search(name).empty?
end
end

Expand Down
2 changes: 1 addition & 1 deletion bundler/lib/bundler/man/bundle-add.1
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
.\" generated with Ronn/v0.7.3
.\" http://github.com/rtomayko/ronn/tree/0.7.3
.
.TH "BUNDLE\-ADD" "1" "April 2021" "" ""
.TH "BUNDLE\-ADD" "1" "May 2021" "" ""
.
.SH "NAME"
\fBbundle\-add\fR \- Add gem to the Gemfile and run bundle install
Expand Down
2 changes: 1 addition & 1 deletion bundler/lib/bundler/man/bundle-binstubs.1
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
.\" generated with Ronn/v0.7.3
.\" http://github.com/rtomayko/ronn/tree/0.7.3
.
.TH "BUNDLE\-BINSTUBS" "1" "April 2021" "" ""
.TH "BUNDLE\-BINSTUBS" "1" "May 2021" "" ""
.
.SH "NAME"
\fBbundle\-binstubs\fR \- Install the binstubs of the listed gems
Expand Down
2 changes: 1 addition & 1 deletion bundler/lib/bundler/man/bundle-cache.1
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
.\" generated with Ronn/v0.7.3
.\" http://github.com/rtomayko/ronn/tree/0.7.3
.
.TH "BUNDLE\-CACHE" "1" "April 2021" "" ""
.TH "BUNDLE\-CACHE" "1" "May 2021" "" ""
.
.SH "NAME"
\fBbundle\-cache\fR \- Package your needed \fB\.gem\fR files into your application
Expand Down
2 changes: 1 addition & 1 deletion bundler/lib/bundler/man/bundle-check.1
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
.\" generated with Ronn/v0.7.3
.\" http://github.com/rtomayko/ronn/tree/0.7.3
.
.TH "BUNDLE\-CHECK" "1" "April 2021" "" ""
.TH "BUNDLE\-CHECK" "1" "May 2021" "" ""
.
.SH "NAME"
\fBbundle\-check\fR \- Verifies if dependencies are satisfied by installed gems
Expand Down
2 changes: 1 addition & 1 deletion bundler/lib/bundler/man/bundle-clean.1
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
.\" generated with Ronn/v0.7.3
.\" http://github.com/rtomayko/ronn/tree/0.7.3
.
.TH "BUNDLE\-CLEAN" "1" "April 2021" "" ""
.TH "BUNDLE\-CLEAN" "1" "May 2021" "" ""
.
.SH "NAME"
\fBbundle\-clean\fR \- Cleans up unused gems in your bundler directory
Expand Down
8 changes: 1 addition & 7 deletions bundler/lib/bundler/man/bundle-config.1
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
.\" generated with Ronn/v0.7.3
.\" http://github.com/rtomayko/ronn/tree/0.7.3
.
.TH "BUNDLE\-CONFIG" "1" "April 2021" "" ""
.TH "BUNDLE\-CONFIG" "1" "May 2021" "" ""
.
.SH "NAME"
\fBbundle\-config\fR \- Set bundler configuration options
Expand Down Expand Up @@ -56,9 +56,6 @@ Executing \fBbundle config unset \-\-local <name> <value>\fR will delete the con
.P
Executing bundle with the \fBBUNDLE_IGNORE_CONFIG\fR environment variable set will cause it to ignore all configuration\.
.
.P
Executing \fBbundle config set \-\-local disable_multisource true\fR upgrades the warning about the Gemfile containing multiple primary sources to an error\. Executing \fBbundle config unset disable_multisource\fR downgrades this error to a warning\.
.
.SH "REMEMBERING OPTIONS"
Flags passed to \fBbundle install\fR or the Bundler runtime, such as \fB\-\-path foo\fR or \fB\-\-without production\fR, are remembered between commands and saved to your local application\'s configuration (normally, \fB\./\.bundle/config\fR)\.
.
Expand Down Expand Up @@ -184,9 +181,6 @@ The following is a list of all configuration keys and their purpose\. You can le
\fBdisable_local_revision_check\fR (\fBBUNDLE_DISABLE_LOCAL_REVISION_CHECK\fR): Allow Bundler to use a local git override without checking if the revision present in the lockfile is present in the repository\.
.
.IP "\(bu" 4
\fBdisable_multisource\fR (\fBBUNDLE_DISABLE_MULTISOURCE\fR): When set, Gemfiles containing multiple sources will produce errors instead of warnings\. Use \fBbundle config unset disable_multisource\fR to unset\.
.
.IP "\(bu" 4
\fBdisable_shared_gems\fR (\fBBUNDLE_DISABLE_SHARED_GEMS\fR): Stop Bundler from accessing gems installed to RubyGems\' normal location\.
.
.IP "\(bu" 4
Expand Down
8 changes: 0 additions & 8 deletions bundler/lib/bundler/man/bundle-config.1.ronn
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,6 @@ configuration only from the local application.
Executing bundle with the `BUNDLE_IGNORE_CONFIG` environment variable set will
cause it to ignore all configuration.

Executing `bundle config set --local disable_multisource true` upgrades the warning about
the Gemfile containing multiple primary sources to an error. Executing `bundle
config unset disable_multisource` downgrades this error to a warning.

## REMEMBERING OPTIONS

Flags passed to `bundle install` or the Bundler runtime, such as `--path foo` or
Expand Down Expand Up @@ -178,10 +174,6 @@ learn more about their operation in [bundle install(1)](bundle-install.1.html).
* `disable_local_revision_check` (`BUNDLE_DISABLE_LOCAL_REVISION_CHECK`):
Allow Bundler to use a local git override without checking if the revision
present in the lockfile is present in the repository.
* `disable_multisource` (`BUNDLE_DISABLE_MULTISOURCE`):
When set, Gemfiles containing multiple sources will produce errors
instead of warnings.
Use `bundle config unset disable_multisource` to unset.
* `disable_shared_gems` (`BUNDLE_DISABLE_SHARED_GEMS`):
Stop Bundler from accessing gems installed to RubyGems' normal location.
* `disable_version_check` (`BUNDLE_DISABLE_VERSION_CHECK`):
Expand Down
2 changes: 1 addition & 1 deletion bundler/lib/bundler/man/bundle-doctor.1
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
.\" generated with Ronn/v0.7.3
.\" http://github.com/rtomayko/ronn/tree/0.7.3
.
.TH "BUNDLE\-DOCTOR" "1" "April 2021" "" ""
.TH "BUNDLE\-DOCTOR" "1" "May 2021" "" ""
.
.SH "NAME"
\fBbundle\-doctor\fR \- Checks the bundle for common problems
Expand Down
2 changes: 1 addition & 1 deletion bundler/lib/bundler/man/bundle-exec.1
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
.\" generated with Ronn/v0.7.3
.\" http://github.com/rtomayko/ronn/tree/0.7.3
.
.TH "BUNDLE\-EXEC" "1" "April 2021" "" ""
.TH "BUNDLE\-EXEC" "1" "May 2021" "" ""
.
.SH "NAME"
\fBbundle\-exec\fR \- Execute a command in the context of the bundle
Expand Down
2 changes: 1 addition & 1 deletion bundler/lib/bundler/man/bundle-gem.1
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
.\" generated with Ronn/v0.7.3
.\" http://github.com/rtomayko/ronn/tree/0.7.3
.
.TH "BUNDLE\-GEM" "1" "April 2021" "" ""
.TH "BUNDLE\-GEM" "1" "May 2021" "" ""
.
.SH "NAME"
\fBbundle\-gem\fR \- Generate a project skeleton for creating a rubygem
Expand Down
2 changes: 1 addition & 1 deletion bundler/lib/bundler/man/bundle-info.1
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
.\" generated with Ronn/v0.7.3
.\" http://github.com/rtomayko/ronn/tree/0.7.3
.
.TH "BUNDLE\-INFO" "1" "April 2021" "" ""
.TH "BUNDLE\-INFO" "1" "May 2021" "" ""
.
.SH "NAME"
\fBbundle\-info\fR \- Show information for the given gem in your bundle
Expand Down
2 changes: 1 addition & 1 deletion bundler/lib/bundler/man/bundle-init.1
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
.\" generated with Ronn/v0.7.3
.\" http://github.com/rtomayko/ronn/tree/0.7.3
.
.TH "BUNDLE\-INIT" "1" "April 2021" "" ""
.TH "BUNDLE\-INIT" "1" "May 2021" "" ""
.
.SH "NAME"
\fBbundle\-init\fR \- Generates a Gemfile into the current working directory
Expand Down
2 changes: 1 addition & 1 deletion bundler/lib/bundler/man/bundle-inject.1
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
.\" generated with Ronn/v0.7.3
.\" http://github.com/rtomayko/ronn/tree/0.7.3
.
.TH "BUNDLE\-INJECT" "1" "April 2021" "" ""
.TH "BUNDLE\-INJECT" "1" "May 2021" "" ""
.
.SH "NAME"
\fBbundle\-inject\fR \- Add named gem(s) with version requirements to Gemfile
Expand Down
2 changes: 1 addition & 1 deletion bundler/lib/bundler/man/bundle-install.1
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
.\" generated with Ronn/v0.7.3
.\" http://github.com/rtomayko/ronn/tree/0.7.3
.
.TH "BUNDLE\-INSTALL" "1" "April 2021" "" ""
.TH "BUNDLE\-INSTALL" "1" "May 2021" "" ""
.
.SH "NAME"
\fBbundle\-install\fR \- Install the dependencies specified in your Gemfile
Expand Down
2 changes: 1 addition & 1 deletion bundler/lib/bundler/man/bundle-list.1
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
.\" generated with Ronn/v0.7.3
.\" http://github.com/rtomayko/ronn/tree/0.7.3
.
.TH "BUNDLE\-LIST" "1" "April 2021" "" ""
.TH "BUNDLE\-LIST" "1" "May 2021" "" ""
.
.SH "NAME"
\fBbundle\-list\fR \- List all the gems in the bundle
Expand Down
2 changes: 1 addition & 1 deletion bundler/lib/bundler/man/bundle-lock.1
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
.\" generated with Ronn/v0.7.3
.\" http://github.com/rtomayko/ronn/tree/0.7.3
.
.TH "BUNDLE\-LOCK" "1" "April 2021" "" ""
.TH "BUNDLE\-LOCK" "1" "May 2021" "" ""
.
.SH "NAME"
\fBbundle\-lock\fR \- Creates / Updates a lockfile without installing
Expand Down
2 changes: 1 addition & 1 deletion bundler/lib/bundler/man/bundle-open.1
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
.\" generated with Ronn/v0.7.3
.\" http://github.com/rtomayko/ronn/tree/0.7.3
.
.TH "BUNDLE\-OPEN" "1" "April 2021" "" ""
.TH "BUNDLE\-OPEN" "1" "May 2021" "" ""
.
.SH "NAME"
\fBbundle\-open\fR \- Opens the source directory for a gem in your bundle
Expand Down