Merge pull request #3655 from rubygems/disable_multisource_improvements

Fix source priority for transitive dependencies and split lockfile rubygems source sections

(cherry picked from commit 4346a8b)

Author: deivid-rodriguez

.github/workflows/ruby-core.yml

@@ -46,15 +46,17 @@ jobs:
       - uses: actions/checkout@v2
         with:
           path: rubygems/rubygems
-      - name: Test RubyGems
+      - name: Sync tools
         run: |
           ruby tool/sync_default_gems.rb rubygems
-          make -j2 -s test-all TESTS="rubygems --no-retry"
+          ruby tool/sync_default_gems.rb bundler
+        working-directory: ruby/ruby
+      - name: Test RubyGems
+        run: make -j2 -s test-all TESTS="rubygems --no-retry"
         working-directory: ruby/ruby
         if: matrix.target == 'Rubygems'
       - name: Test Bundler
         run: |
-          ruby tool/sync_default_gems.rb bundler
           git checkout lib/bundler/bundler.gemspec
           git add .
           make test-bundler-parallel

bundler/lib/bundler/definition.rb

@@ -106,6 +106,19 @@ def initialize(lockfile, dependencies, sources, unlock, ruby_version = nil, opti
         @locked_platforms = []
       end
 
+      @locked_gem_sources = @locked_sources.select {|s| s.is_a?(Source::Rubygems) }
+      @disable_multisource = !Bundler.frozen_bundle? || @locked_gem_sources.none? {|s| s.remotes.size > 1 }
+
+      unless @disable_multisource
+        msg = "Your lockfile contains a single rubygems source section with multiple remotes, which is insecure. " \
+          "You should regenerate your lockfile in a non frozen environment."
+
+        Bundler::SharedHelpers.major_deprecation 2, msg
+
+        @sources.allow_multisource!
+        @locked_gem_sources.each(&:allow_multisource!)
+      end
+
       @unlock[:gems] ||= []
       @unlock[:sources] ||= []
       @unlock[:ruby] ||= if @ruby_version && locked_ruby_version_object
@@ -145,6 +158,14 @@ def gem_version_promoter
       end
     end
 
+    def disable_multisource?
+      @disable_multisource
+    end
+
+    def allow_multisource!
+      @disable_multisource = false
+    end
+
     def resolve_with_cache!
       raise "Specs already loaded" if @specs
       sources.cached!
@@ -264,7 +285,7 @@ def resolve
           # Run a resolve against the locally available gems
           Bundler.ui.debug("Found changes from the lockfile, re-resolving dependencies because #{change_reason}")
           expanded_dependencies = expand_dependencies(dependencies + metadata_dependencies, @remote)
-          Resolver.resolve(expanded_dependencies, index, source_requirements, last_resolve, gem_version_promoter, additional_base_requirements_for_resolve, platforms)
+          Resolver.resolve(expanded_dependencies, source_requirements, last_resolve, gem_version_promoter, additional_base_requirements_for_resolve, platforms)
         end
       end
     end
@@ -530,6 +551,9 @@ def find_indexed_specs(current_spec)
     attr_reader :sources
     private :sources
 
+    attr_reader :locked_gem_sources
+    private :locked_gem_sources
+
     def nothing_changed?
       !@source_changes && !@dependency_changes && !@new_platform && !@path_changes && !@local_changes && !@locked_specs_incomplete_for_platform
     end
@@ -654,21 +678,20 @@ def converge_path_sources_to_gemspec_sources
     end
 
     def converge_rubygems_sources
-      return false if Bundler.feature_flag.disable_multisource?
+      return false if disable_multisource?
 
-      changes = false
+      return false if locked_gem_sources.empty?
 
-      # Get the RubyGems sources from the Gemfile.lock
-      locked_gem_sources = @locked_sources.select {|s| s.is_a?(Source::Rubygems) }
       # Get the RubyGems remotes from the Gemfile
       actual_remotes = sources.rubygems_remotes
+      return false if actual_remotes.empty?
+
+      changes = false
 
       # If there is a RubyGems source in both
-      if !locked_gem_sources.empty? && !actual_remotes.empty?
-        locked_gem_sources.each do |locked_gem|
-          # Merge the remotes from the Gemfile into the Gemfile.lock
-          changes |= locked_gem.replace_remotes(actual_remotes, Bundler.settings[:allow_deployment_source_credential_changes])
-        end
+      locked_gem_sources.each do |locked_gem|
+        # Merge the remotes from the Gemfile into the Gemfile.lock
+        changes |= locked_gem.replace_remotes(actual_remotes, Bundler.settings[:allow_deployment_source_credential_changes])
       end
 
       changes
@@ -893,30 +916,18 @@ def source_requirements
       # 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)
-      default = sources.default_source
-      source_requirements = { :default => default }
-      default = nil unless Bundler.feature_flag.disable_multisource?
-      dependencies.each do |dep|
-        next unless source = dep.source || default
-        source_requirements[dep.name] = source
-      end
+      source_requirements = { :default => sources.default_source }.merge(dependency_source_requirements)
       metadata_dependencies.each do |dep|
         source_requirements[dep.name] = sources.metadata_source
       end
+      source_requirements[:global] = index unless disable_multisource?
       source_requirements[:default_bundler] = source_requirements["bundler"] || source_requirements[:default]
       source_requirements["bundler"] = sources.metadata_source # needs to come last to override
       source_requirements
     end
 
     def pinned_spec_names(skip = nil)
-      pinned_names = []
-      default = Bundler.feature_flag.disable_multisource? && sources.default_source
-      @dependencies.each do |dep|
-        next unless dep_source = dep.source || default
-        next if dep_source == skip
-        pinned_names << dep.name
-      end
-      pinned_names
+      dependency_source_requirements.reject {|_, source| source == skip }.keys
     end
 
     def requested_groups
@@ -973,5 +984,18 @@ 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 = disable_multisource? && sources.default_source
+        dependencies.each do |dep|
+          dep_source = dep.source || default
+          next unless dep_source
+          source_requirements[dep.name] = dep_source
+        end
+        source_requirements
+      end
+    end
   end
 end

bundler/lib/bundler/dsl.rb

@@ -24,6 +24,9 @@ def self.evaluate(gemfile, lockfile, unlock)
     def initialize
       @source               = nil
       @sources              = SourceList.new
+
+      @global_rubygems_sources = []
+
       @git_sources          = {}
       @dependencies         = []
       @groups               = []
@@ -45,6 +48,7 @@ def eval_gemfile(gemfile, contents = nil)
       @gemfiles << expanded_gemfile_path
       contents ||= Bundler.read_file(@gemfile.to_s)
       instance_eval(contents.dup.tap{|x| x.untaint if RUBY_VERSION < "2.7" }, gemfile.to_s, 1)
+      check_primary_source_safety
     rescue Exception => e # rubocop:disable Lint/RescueException
       message = "There was an error " \
         "#{e.is_a?(GemfileEvalError) ? "evaluating" : "parsing"} " \
@@ -164,8 +168,7 @@ def source(source, *args, &blk)
       elsif block_given?
         with_source(@sources.add_rubygems_source("remotes" => source), &blk)
       else
-        check_primary_source_safety(@sources)
-        @sources.global_rubygems_source = source
+        @global_rubygems_sources << source
       end
     end
 
@@ -183,24 +186,14 @@ def git_source(name, &block)
     end
 
     def path(path, options = {}, &blk)
-      unless block_given?
-        msg = "You can no longer specify a path source by itself. Instead, \n" \
-              "either use the :path option on a gem, or specify the gems that \n" \
-              "bundler should find in the path source by passing a block to \n" \
-              "the path method, like: \n\n" \
-              "    path 'dir/containing/rails' do\n" \
-              "      gem 'rails'\n" \
-              "    end\n\n"
-
-        raise DeprecatedError, msg if Bundler.feature_flag.disable_multisource?
-        SharedHelpers.major_deprecation(2, msg.strip)
-      end
-
       source_options = normalize_hash(options).merge(
         "path" => Pathname.new(path),
         "root_path" => gemfile_root,
         "gemspec" => gemspecs.find {|g| g.name == options["name"] }
       )
+
+      source_options["global"] = true unless block_given?
+
       source = @sources.add_path_source(source_options)
       with_source(source, &blk)
     end
@@ -279,6 +272,11 @@ def method_missing(name, *args)
       raise GemfileError, "Undefined local variable or method `#{name}' for Gemfile"
     end
 
+    def check_primary_source_safety
+      check_path_source_safety
+      check_rubygems_source_safety
+    end
+
     private
 
     def add_git_sources
@@ -440,25 +438,40 @@ def normalize_source(source)
       end
     end
 
-    def check_primary_source_safety(source_list)
-      return if source_list.rubygems_primary_remotes.empty? && source_list.global_rubygems_source.nil?
+    def check_path_source_safety
+      return if @sources.global_path_source.nil?
+
+      msg = "You can no longer specify a path source by itself. Instead, \n" \
+              "either use the :path option on a gem, or specify the gems that \n" \
+              "bundler should find in the path source by passing a block to \n" \
+              "the path method, like: \n\n" \
+              "    path 'dir/containing/rails' do\n" \
+              "      gem 'rails'\n" \
+              "    end\n\n"
 
-      if Bundler.feature_flag.disable_multisource?
+      SharedHelpers.major_deprecation(2, msg.strip)
+    end
+
+    def check_rubygems_source_safety
+      if @global_rubygems_sources.size <= 1
+        @sources.global_rubygems_source = @global_rubygems_sources.first
+        return
+      end
+
+      @global_rubygems_sources.each do |source|
+        @sources.add_rubygems_remote(source)
+      end
+
+      if Bundler.feature_flag.bundler_3_mode?
         msg = "This Gemfile contains multiple primary sources. " \
           "Each source after the first must include a block to indicate which gems " \
           "should come from that source"
-        unless Bundler.feature_flag.bundler_2_mode?
-          msg += ". To downgrade this error to a warning, run " \
-            "`bundle config unset disable_multisource`"
-        end
         raise GemfileEvalError, msg
       else
         Bundler::SharedHelpers.major_deprecation 2, "Your Gemfile contains multiple primary sources. " \
           "Using `source` more than once without a block is a security risk, and " \
           "may result in installing unexpected gems. To resolve this warning, use " \
-          "a block to indicate which gems should come from the secondary source. " \
-          "To upgrade this warning to an error, run `bundle config set --local " \
-          "disable_multisource true`."
+          "a block to indicate which gems should come from the secondary source."
       end
     end
 

bundler/lib/bundler/feature_flag.rb

@@ -32,7 +32,6 @@ def self.settings_method(name, key, &default)
     settings_flag(:cache_all) { bundler_3_mode? }
     settings_flag(:default_install_uses_path) { bundler_3_mode? }
     settings_flag(:deployment_means_frozen) { 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(:only_update_to_newer_versions) { bundler_3_mode? }

bundler/lib/bundler/inline.rb

@@ -50,6 +50,7 @@ def Bundler.root
     Bundler::Plugin.gemfile_install(&gemfile) if Bundler.feature_flag.plugins?
     builder = Bundler::Dsl.new
     builder.instance_eval(&gemfile)
+    builder.check_primary_source_safety
 
     Bundler.settings.temporary(:frozen => false) do
       definition = builder.to_definition(nil, true)

bundler/lib/bundler/lockfile_parser.rb

@@ -64,8 +64,6 @@ def initialize(lockfile)
       @state        = nil
       @specs        = {}
 
-      @rubygems_aggregate = Source::Rubygems.new
-
       if lockfile.match(/<<<<<<<|=======|>>>>>>>|\|\|\|\|\|\|\|/)
         raise LockfileError, "Your #{Bundler.default_lockfile.relative_path_from(SharedHelpers.pwd)} contains merge conflicts.\n" \
           "Run `git checkout HEAD -- #{Bundler.default_lockfile.relative_path_from(SharedHelpers.pwd)}` first to get a clean lock."
@@ -89,7 +87,6 @@ def initialize(lockfile)
           send("parse_#{@state}", line)
         end
       end
-      @sources << @rubygems_aggregate unless Bundler.feature_flag.disable_multisource?
       @specs = @specs.values.sort_by(&:identifier)
       warn_for_outdated_bundler_version
     rescue ArgumentError => e
@@ -134,16 +131,19 @@ def parse_source(line)
             @sources << @current_source
           end
         when GEM
-          if Bundler.feature_flag.disable_multisource?
+          source_remotes = Array(@opts["remote"])
+
+          if source_remotes.size == 1
             @opts["remotes"] = @opts.delete("remote")
             @current_source = TYPES[@type].from_lock(@opts)
-            @sources << @current_source
           else
-            Array(@opts["remote"]).each do |url|
-              @rubygems_aggregate.add_remote(url)
+            source_remotes.each do |url|
+              rubygems_aggregate.add_remote(url)
             end
-            @current_source = @rubygems_aggregate
+            @current_source = rubygems_aggregate
           end
+
+          @sources << @current_source
         when PLUGIN
           @current_source = Plugin.source_from_lock(@opts)
           @sources << @current_source
@@ -245,5 +245,9 @@ def parse_bundled_with(line)
     def parse_ruby(line)
       @ruby_version = line.strip
     end
+
+    def rubygems_aggregate
+      @rubygems_aggregate ||= Source::Rubygems.new
+    end
   end
 end

bundler/lib/bundler/man/bundle-config.1

@@ -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)\.
 .
@@ -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

bundler/lib/bundler/man/bundle-config.1.ronn

@@ -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
@@ -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`):

bundler/lib/bundler/plugin.rb

@@ -105,6 +105,7 @@ def gemfile_install(gemfile = nil, &inline)
         else
           builder.eval_gemfile(gemfile)
         end
+        builder.check_primary_source_safety
         definition = builder.to_definition(nil, true)
 
         return if definition.dependencies.empty?