Chef-13: shell_out PATH fixes and path_sanity changes

Turns out polluting PATH with a global path_sanity actually tends
to cause problems via picking up stuff from e.g. embedded/bin
when users don't want it.  This change unsets the default setting
to add path sanity.

We already have the difference between internal and external uses
of shell_out, so what this does is take the internal use of
shell_out and always apply the PATH -- so anything calling
shell_out gets path_sanity.

It also modifies path_sanity to prepend the ruby bin and the
embedded bin into the PATH.  When we need those for internal use
we really want those first.

Users who don't want path sanity at all can do:

shell_out_with_systems_locale(*whatever)

or:

shell_out(*whatever, env: { "PATH" => ENV['PATH'] })

Next PR I want to add:

shell_out(*whatever, internal: false)

Default will still be true though.

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>

5 files changed, 42 insertions, 142 deletions

diff --git a/chef-config/lib/chef-config/config.rb b/chef-config/lib/chef-config/config.rb
index c63098930b..0666dd869d 100644
--- a/chef-config/lib/chef-config/config.rb
+++ b/chef-config/lib/chef-config/config.rb
@@ -250,7 +250,7 @@ module ChefConfig
     default(:policy_path) { derive_path_from_chef_repo_path("policies") }
 
     # Turn on "path sanity" by default. See also: http://wiki.opscode.com/display/chef/User+Environment+PATH+Sanity
-    default :enforce_path_sanity, true
+    default :enforce_path_sanity, false
 
     # Formatted Chef Client output is a beta feature, disabled by default:
     default :formatter, "null"
diff --git a/lib/chef/mixin/path_sanity.rb b/lib/chef/mixin/path_sanity.rb
index 6a8e017bcd..c441d0770a 100644
--- a/lib/chef/mixin/path_sanity.rb
+++ b/lib/chef/mixin/path_sanity.rb
@@ -1,6 +1,6 @@
 #
 # Author:: Seth Chisamore (<schisamo@chef.io>)
-# Copyright:: Copyright 2011-2016, Chef Software Inc.
+# Copyright:: Copyright 2011-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -22,25 +22,23 @@ class Chef
 
       def enforce_path_sanity(env = ENV)
         if Chef::Config[:enforce_path_sanity]
-          env["PATH"] = "" if env["PATH"].nil?
-          path_separator = Chef::Platform.windows? ? ";" : ":"
-          existing_paths = env["PATH"].split(path_separator)
-          # ensure the Ruby and Gem bindirs are included
-          # mainly for 'full-stack' Chef installs
-          paths_to_add = []
-          paths_to_add << ruby_bindir unless sane_paths.include?(ruby_bindir)
-          paths_to_add << gem_bindir unless sane_paths.include?(gem_bindir)
-          paths_to_add << sane_paths if sane_paths
-          paths_to_add.flatten!.compact!
-          paths_to_add.each do |sane_path|
-            unless existing_paths.include?(sane_path)
-              env_path = env["PATH"].dup
-              env_path << path_separator unless env["PATH"].empty?
-              env_path << sane_path
-              env["PATH"] = env_path.encode("utf-8", invalid: :replace, undef: :replace)
-            end
-          end
+          env["PATH"] = sanitized_path(env)
+        end
+      end
+
+      def sanitized_path(env = ENV)
+        env_path = env["PATH"].nil? ? "" : env["PATH"].dup
+        path_separator = Chef::Platform.windows? ? ";" : ":"
+        # ensure the Ruby and Gem bindirs are included
+        # mainly for 'full-stack' Chef installs
+        new_paths = env_path.split(path_separator)
+        [ ruby_bindir, gem_bindir ].compact.each do |path|
+          new_paths = [ path ] + new_paths unless new_paths.include?(path)
+        end
+        sane_paths.each do |path|
+          new_paths << path unless new_paths.include?(path)
         end
+        new_paths.join(path_separator).encode("utf-8", invalid: :replace, undef: :replace)
       end
 
       private
diff --git a/lib/chef/mixin/shell_out.rb b/lib/chef/mixin/shell_out.rb
index 18bd067989..1bcf6d8861 100644
--- a/lib/chef/mixin/shell_out.rb
+++ b/lib/chef/mixin/shell_out.rb
@@ -16,11 +16,12 @@
 # limitations under the License.
 
 require "mixlib/shellout"
-require "chef/deprecated"
+require "chef/mixin/path_sanity"
 
 class Chef
   module Mixin
     module ShellOut
+      include Chef::Mixin::PathSanity
 
       # PREFERRED APIS:
       #
@@ -108,6 +109,7 @@ class Chef
           "LC_ALL" => Chef::Config[:internal_locale],
           "LANGUAGE" => Chef::Config[:internal_locale],
           "LANG" => Chef::Config[:internal_locale],
+          "PATH" => sanitized_path,
         }.update(options[env_key] || {})
         shell_out_command(*args, **options)
       end
@@ -129,28 +131,6 @@ class Chef
         cmd
       end
 
-      DEPRECATED_OPTIONS =
-        [ [:command_log_level,   :log_level],
-          [:command_log_prepend, :log_tag] ]
-
-      # CHEF-3090: Deprecate command_log_level and command_log_prepend
-      # Patterned after https://github.com/chef/chef/commit/e1509990b559984b43e428d4d801c394e970f432
-      def run_command_compatible_options(command_args)
-        return command_args unless command_args.last.is_a?(Hash)
-
-        my_command_args = command_args.dup
-        my_options = my_command_args.last
-
-        DEPRECATED_OPTIONS.each do |old_option, new_option|
-          # Edge case: someone specifies :command_log_level and 'command_log_level' in the option hash
-          next unless value = my_options.delete(old_option) || my_options.delete(old_option.to_s)
-          deprecate_option old_option, new_option
-          my_options[new_option] = value
-        end
-
-        my_command_args
-      end
-
       # Helper for sublcasses to convert an array of string args into a string.  It
       # will compact nil or empty strings in the array and will join the array elements
       # with spaces, without introducing any double spaces for nil/empty elements.
@@ -186,16 +166,12 @@ class Chef
       private
 
       def shell_out_command(*command_args)
-        cmd = Mixlib::ShellOut.new(*run_command_compatible_options(command_args))
+        cmd = Mixlib::ShellOut.new(*command_args)
         cmd.live_stream ||= io_for_live_stream
         cmd.run_command
         cmd
       end
 
-      def deprecate_option(old_option, new_option)
-        Chef.deprecated :internal_api, "Chef::Mixin::ShellOut option :#{old_option} is deprecated. Use :#{new_option}"
-      end
-
       def io_for_live_stream
         if STDOUT.tty? && !Chef::Config[:daemon] && Chef::Log.debug?
           STDOUT
diff --git a/spec/unit/mixin/path_sanity_spec.rb b/spec/unit/mixin/path_sanity_spec.rb
index 675b5722be..c9ed4e2595 100644
--- a/spec/unit/mixin/path_sanity_spec.rb
+++ b/spec/unit/mixin/path_sanity_spec.rb
@@ -1,6 +1,6 @@
 #
 # Author:: Seth Chisamore (<schisamo@chef.io>)
-# Copyright:: Copyright 2011-2016, Chef Software Inc.
+# Copyright:: Copyright 2011-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -41,19 +41,19 @@ describe Chef::Mixin::PathSanity do
     it "adds all useful PATHs even if environment is an empty hash" do
       env = {}
       @sanity.enforce_path_sanity(env)
-      expect(env["PATH"]).to eq("#{@ruby_bindir}:#{@gem_bindir}:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin")
+      expect(env["PATH"]).to eq("#{@gem_bindir}:#{@ruby_bindir}:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin")
     end
 
     it "adds all useful PATHs that are not yet in PATH to PATH" do
       env = { "PATH" => "" }
       @sanity.enforce_path_sanity(env)
-      expect(env["PATH"]).to eq("#{@ruby_bindir}:#{@gem_bindir}:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin")
+      expect(env["PATH"]).to eq("#{@gem_bindir}:#{@ruby_bindir}:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin")
     end
 
     it "does not re-add paths that already exist in PATH" do
       env = { "PATH" => "/usr/bin:/sbin:/bin" }
       @sanity.enforce_path_sanity(env)
-      expect(env["PATH"]).to eq("/usr/bin:/sbin:/bin:#{@ruby_bindir}:#{@gem_bindir}:/usr/local/sbin:/usr/local/bin:/usr/sbin")
+      expect(env["PATH"]).to eq("#{@gem_bindir}:#{@ruby_bindir}:/usr/bin:/sbin:/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin")
     end
 
     it "creates path with utf-8 encoding" do
@@ -65,7 +65,7 @@ describe Chef::Mixin::PathSanity do
     it "adds the current executing Ruby's bindir and Gem bindir to the PATH" do
       env = { "PATH" => "" }
       @sanity.enforce_path_sanity(env)
-      expect(env["PATH"]).to eq("#{@ruby_bindir}:#{@gem_bindir}:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin")
+      expect(env["PATH"]).to eq("#{@gem_bindir}:#{@ruby_bindir}:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin")
     end
 
     it "does not create entries for Ruby/Gem bindirs if they exist in SANE_PATH or PATH" do
@@ -75,7 +75,7 @@ describe Chef::Mixin::PathSanity do
       allow(RbConfig::CONFIG).to receive(:[]).with("bindir").and_return(ruby_bindir)
       env = { "PATH" => gem_bindir }
       @sanity.enforce_path_sanity(env)
-      expect(env["PATH"]).to eq("/yo/gabba/gabba:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin")
+      expect(env["PATH"]).to eq("/usr/bin:/yo/gabba/gabba:/usr/local/sbin:/usr/local/bin:/usr/sbin:/sbin:/bin")
     end
 
     it "builds a valid windows path" do
@@ -86,7 +86,7 @@ describe Chef::Mixin::PathSanity do
       allow(ChefConfig).to receive(:windows?).and_return(true)
       env = { "PATH" => 'C:\Windows\system32;C:\mr\softie' }
       @sanity.enforce_path_sanity(env)
-      expect(env["PATH"]).to eq("C:\\Windows\\system32;C:\\mr\\softie;#{ruby_bindir};#{gem_bindir}")
+      expect(env["PATH"]).to eq("#{gem_bindir};#{ruby_bindir};C:\\Windows\\system32;C:\\mr\\softie")
     end
   end
 end
diff --git a/spec/unit/mixin/shell_out_spec.rb b/spec/unit/mixin/shell_out_spec.rb
index aa38639c1c..1ea7db87a4 100644
--- a/spec/unit/mixin/shell_out_spec.rb
+++ b/spec/unit/mixin/shell_out_spec.rb
@@ -21,93 +21,13 @@
 #
 
 require "spec_helper"
+require "chef/mixin/path_sanity"
 
 describe Chef::Mixin::ShellOut do
+  include Chef::Mixin::PathSanity
+
   let(:shell_out_class) { Class.new { include Chef::Mixin::ShellOut } }
   subject(:shell_out_obj) { shell_out_class.new }
-  describe "#run_command_compatible_options" do
-    subject { shell_out_obj.run_command_compatible_options(command_args) }
-    let(:command_args) { [ cmd, options ] }
-    let(:cmd) { "echo '#{rand(1000)}'" }
-
-    let(:output) { StringIO.new }
-    let!(:capture_log_output) { Chef::Log.logger = Logger.new(output) }
-    let(:assume_deprecation_log_level) { allow(Chef::Log).to receive(:level).and_return(:warn) }
-
-    before do
-      Chef::Config[:treat_deprecation_warnings_as_errors] = false
-    end
-
-    context "without options" do
-      let(:command_args) { [ cmd ] }
-
-      it "should not edit command args" do
-        is_expected.to eql(command_args)
-      end
-    end
-
-    context "without deprecated options" do
-      let(:options) { { :environment => environment } }
-      let(:environment) { { "LC_ALL" => "C", "LANG" => "C", "LANGUAGE" => "C" } }
-
-      it "should not edit command args" do
-        is_expected.to eql(command_args)
-      end
-    end
-
-    def self.should_emit_deprecation_warning_about(old_option, new_option)
-      it "should emit a deprecation warning" do
-        assume_deprecation_log_level && capture_log_output
-        subject
-        expect(output.string).to match Regexp.escape(old_option.to_s)
-        expect(output.string).to match Regexp.escape(new_option.to_s)
-      end
-    end
-
-    context "with :command_log_level option" do
-      let(:options) { { :command_log_level => command_log_level } }
-      let(:command_log_level) { :warn }
-
-      it "should convert :command_log_level to :log_level" do
-        is_expected.to eql [ cmd, { :log_level => command_log_level } ]
-      end
-
-      should_emit_deprecation_warning_about :command_log_level, :log_level
-    end
-
-    context "with :command_log_prepend option" do
-      let(:options) { { :command_log_prepend => command_log_prepend } }
-      let(:command_log_prepend) { "PROVIDER:" }
-
-      it "should convert :command_log_prepend to :log_tag" do
-        is_expected.to eql [ cmd, { :log_tag => command_log_prepend } ]
-      end
-
-      should_emit_deprecation_warning_about :command_log_prepend, :log_tag
-    end
-
-    context "with 'command_log_level' option" do
-      let(:options) { { "command_log_level" => command_log_level } }
-      let(:command_log_level) { :warn }
-
-      it "should convert 'command_log_level' to :log_level" do
-        is_expected.to eql [ cmd, { :log_level => command_log_level } ]
-      end
-
-      should_emit_deprecation_warning_about :command_log_level, :log_level
-    end
-
-    context "with 'command_log_prepend' option" do
-      let(:options) { { "command_log_prepend" => command_log_prepend } }
-      let(:command_log_prepend) { "PROVIDER:" }
-
-      it "should convert 'command_log_prepend' to :log_tag" do
-        is_expected.to eql [ cmd, { :log_tag => command_log_prepend } ]
-      end
-
-      should_emit_deprecation_warning_about :command_log_prepend, :log_tag
-    end
-  end
 
   context "when testing individual methods" do
     before(:each) do
@@ -127,13 +47,13 @@ describe Chef::Mixin::ShellOut do
       describe "when the last argument is a Hash" do
         describe "and environment is an option" do
           it "should not change environment language settings when they are set to nil" do
-            options = { :environment => { "LC_ALL" => nil, "LANGUAGE" => nil, "LANG" => nil } }
+            options = { :environment => { "LC_ALL" => nil, "LANGUAGE" => nil, "LANG" => nil, "PATH" => nil } }
             expect(shell_out_obj).to receive(:shell_out_command).with(cmd, options).and_return(true)
             shell_out_obj.shell_out(cmd, options)
           end
 
           it "should not change environment language settings when they are set to non-nil" do
-            options = { :environment => { "LC_ALL" => "en_US.UTF-8", "LANGUAGE" => "en_US.UTF-8", "LANG" => "en_US.UTF-8" } }
+            options = { :environment => { "LC_ALL" => "en_US.UTF-8", "LANGUAGE" => "en_US.UTF-8", "LANG" => "en_US.UTF-8", "PATH" => "foo:bar:baz" } }
             expect(shell_out_obj).to receive(:shell_out_command).with(cmd, options).and_return(true)
             shell_out_obj.shell_out(cmd, options)
           end
@@ -146,6 +66,7 @@ describe Chef::Mixin::ShellOut do
                 "LC_ALL"   => Chef::Config[:internal_locale],
                 "LANG"     => Chef::Config[:internal_locale],
                 "LANGUAGE" => Chef::Config[:internal_locale],
+                "PATH"     => sanitized_path,
               },
             }).and_return(true)
             shell_out_obj.shell_out(cmd, options)
@@ -159,6 +80,7 @@ describe Chef::Mixin::ShellOut do
                 "LC_ALL"   => Chef::Config[:internal_locale],
                 "LANG"     => Chef::Config[:internal_locale],
                 "LANGUAGE" => Chef::Config[:internal_locale],
+                "PATH"     => sanitized_path,
               },
             }).and_return(true)
             shell_out_obj.shell_out(cmd, options)
@@ -168,13 +90,13 @@ describe Chef::Mixin::ShellOut do
 
         describe "and env is an option" do
           it "should not change env when langauge options are set to nil" do
-            options = { :env => { "LC_ALL" => nil, "LANG" => nil, "LANGUAGE" => nil } }
+            options = { :env => { "LC_ALL" => nil, "LANG" => nil, "LANGUAGE" => nil, "PATH" => nil } }
             expect(shell_out_obj).to receive(:shell_out_command).with(cmd, options).and_return(true)
             shell_out_obj.shell_out(cmd, options)
           end
 
           it "should not change env when language options are set to non-nil" do
-            options = { :env => { "LC_ALL" => "de_DE.UTF-8", "LANG" => "de_DE.UTF-8", "LANGUAGE" => "de_DE.UTF-8" } }
+            options = { :env => { "LC_ALL" => "de_DE.UTF-8", "LANG" => "de_DE.UTF-8", "LANGUAGE" => "de_DE.UTF-8", "PATH" => "foo:bar:baz" } }
             expect(shell_out_obj).to receive(:shell_out_command).with(cmd, options).and_return(true)
             shell_out_obj.shell_out(cmd, options)
           end
@@ -187,6 +109,7 @@ describe Chef::Mixin::ShellOut do
                 "LC_ALL"   => Chef::Config[:internal_locale],
                 "LANG"     => Chef::Config[:internal_locale],
                 "LANGUAGE" => Chef::Config[:internal_locale],
+                "PATH"     => sanitized_path,
               },
             }).and_return(true)
             shell_out_obj.shell_out(cmd, options)
@@ -200,6 +123,7 @@ describe Chef::Mixin::ShellOut do
                 "LC_ALL"   => Chef::Config[:internal_locale],
                 "LANG"     => Chef::Config[:internal_locale],
                 "LANGUAGE" => Chef::Config[:internal_locale],
+                "PATH"     => sanitized_path,
               },
             }).and_return(true)
             shell_out_obj.shell_out(cmd, options)
@@ -216,6 +140,7 @@ describe Chef::Mixin::ShellOut do
                 "LC_ALL"   => Chef::Config[:internal_locale],
                 "LANG"     => Chef::Config[:internal_locale],
                 "LANGUAGE" => Chef::Config[:internal_locale],
+                "PATH"     => sanitized_path,
               },
             }).and_return(true)
             shell_out_obj.shell_out(cmd, options)
@@ -230,6 +155,7 @@ describe Chef::Mixin::ShellOut do
               "LC_ALL"   => Chef::Config[:internal_locale],
               "LANG"     => Chef::Config[:internal_locale],
               "LANGUAGE" => Chef::Config[:internal_locale],
+              "PATH"     => sanitized_path,
             },
           }).and_return(true)
           shell_out_obj.shell_out(cmd)