Support array args on windows

Implements correct quoting and escaping of arguments on windows.

That means that this works right now:

```ruby
filename = "c:\program files"
shell_out("dir", filename);
```

So all the defensive coding around quotes-around-filepaths -- which were
all actually buggy even when they worked (trailing backslashes would
fail) -- are unnecessary and arguments can just be passed in as an array
and this code will sort it out.

We rely on the existing determination of if metacharacters mean it needs
to run under cmd and if it needs the ^ quoting which seems to be well
tested, and which all runs after this does.

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

Rakefile

@@ -3,7 +3,7 @@ require "rspec/core/rake_task"
 
 Bundler::GemHelper.install_tasks name: "mixlib-shellout"
 
-task default: [:style, :spec]
+task default: [:spec, :style]
 
 desc "Run specs"
 RSpec::Core::RakeTask.new(:spec) do |spec|

lib/mixlib/shellout/windows.rb

@@ -2,7 +2,7 @@
 # Author:: Daniel DeLeo (<dan@chef.io>)
 # Author:: John Keiser (<jkeiser@chef.io>)
 # Author:: Ho-Sheng Hsiao (<hosh@chef.io>)
-# Copyright:: Copyright (c) 2011-2016 Chef Software, Inc.
+# Copyright:: Copyright (c) 2011-2019, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -66,7 +66,7 @@ def run_command
           #
           # Set cwd, environment, appname, etc.
           #
-          app_name, command_line = command_to_run(command)
+          app_name, command_line = command_to_run(combine_args(*command))
           create_process_args = {
             app_name: app_name,
             command_line: command_line,
@@ -196,6 +196,46 @@ def consume_output(open_streams, stdout_read, stderr_read)
         true
       end
 
+      # Use to support array passing semantics on windows
+      #
+      # 1.  strings with whitespace or quotes in them need quotes around them.
+      # 2.  interior quotes need to get backslash escaped (parser needs to know when it really ends).
+      # 3.  random backlsashes in paths themselves remain untouched.
+      # 4.  if the argument must be quoted by #1 and terminates in a sequence of backslashes then all the backlashes must themselves
+      #     be backslash excaped (double the backslashes).
+      # 5.  if an interior quote that must be escaped by #2 has a sequence of backslashes before it then all the backslashes must
+      #     themselves be backslash excaped along with the backslash ecape of the interior quote (double plus one backslashes).
+      #
+      # And to restate.  We are constructing a string which will be parsed by the windows parser into arguments, and we want those
+      # arguments to match the *args array we are passed here.  So call the windows parser operation A then we need to apply A^-1 to
+      # our args to construct the string so that applying A gives windows back our *args.
+      #
+      # And when the windows parser sees a series of backslashes followed by a double quote, it has to determine if that double quote
+      # is terminating or not, and how many backslashes to insert in the args.  So what it does is divide it by two (rounding down) to
+      # get the number of backslashes to insert.  Then if it is even the double quotes terminate the argument.  If it is even the
+      # double quotes are interior double quotes (the extra backslash quotes the double quote).
+      #
+      # We construct the inverse operation so interior double quotes preceeded by N backslashes get 2N+1 backslashes in front of the quote,
+      # while trailing N backslashes get 2N backslashes in front of the quote that terminates the argument.
+      #
+      # see: https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/
+      #
+      # @api private
+      # @param args [Array<String>] array of command arguments
+      # @return String
+      def combine_args(*args)
+        return args[0] if args.length == 1
+        args.map do |arg|
+          if arg =~ /[ \t\n\v"]/
+            arg = arg.gsub(/(\\*)"/, '\1\1\"') # interior quotes with N preceeding backslashes need 2N+1 backslashes
+            arg = arg.sub(/(\\+)$/, '\1\1') # trailing N backslashes need to become 2N backslashes
+            "\"#{arg}\""
+          else
+            arg
+          end
+        end.join(" ")
+      end
+
       def command_to_run(command)
         return run_under_cmd(command) if should_run_under_cmd?(command)
 

spec/mixlib/shellout/windows_spec.rb

@@ -307,4 +307,88 @@ def self.with_command(cmd, filename: nil, search: false, directory: false, &exam
       end
     end
   end
+
+  context "#combine_args" do
+    let(:shell_out) { Mixlib::ShellOut.new }
+    subject { shell_out.send(:combine_args, *largs) }
+
+    def self.with_args(*args, &example)
+      context "with command #{args}" do
+        let(:largs) { args }
+        it(&example)
+      end
+    end
+
+    with_args("echo", "%PATH%") do
+      is_expected.to eql(%q{echo %PATH%})
+    end
+
+    with_args("echo %PATH%") do
+      is_expected.to eql(%q{echo %PATH%})
+    end
+
+    # Note carefully for the following that single quotes in ruby support '\\' as an escape sequence for a single
+    # literal backslash.  It is not mandatory to always use this since '\d' does not escape the 'd' and is literally
+    # a backlash followed by an 'd'.  However, in the following all backslashes are escaped for consistency.  Otherwise
+    # it becomes prohibitively confusing to track when you need and do not need the escape the backslash (particularly
+    # when the literal string has a trailing backslash such that '\\' must be used instead of '\' which would escape
+    # the intended terminating single quote or %q{\} which escapes the terminating delimiter).
+
+    with_args("child.exe", "argument1", "argument 2", '\\some\\path with\\spaces') do
+      is_expected.to eql('child.exe argument1 "argument 2" "\\some\\path with\\spaces"')
+    end
+
+    with_args("child.exe", "argument1", 'she said, "you had me at hello"', '\\some\\path with\\spaces') do
+      is_expected.to eql('child.exe argument1 "she said, \\"you had me at hello\\"" "\\some\\path with\\spaces"')
+    end
+
+    with_args("child.exe", "argument1", 'argument\\\\"2\\\\"', "argument3", "argument4") do
+      is_expected.to eql('child.exe argument1 "argument\\\\\\\\\\"2\\\\\\\\\\"" argument3 argument4')
+    end
+
+    with_args("child.exe", '\\some\\directory with\\spaces\\', "argument2") do
+      is_expected.to eql('child.exe "\\some\\directory with\\spaces\\\\" argument2')
+    end
+
+    with_args("child.exe", '\\some\\directory with\\\\\\spaces\\\\\\', "argument2") do
+      is_expected.to eql('child.exe "\\some\\directory with\\\\\\spaces\\\\\\\\\\\\" argument2')
+    end
+  end
+
+  context "#run_command" do
+    let(:shell_out) { Mixlib::ShellOut.new(*largs) }
+    subject { shell_out.send(:run_command) }
+
+    def self.with_args(*args, &example)
+      context "with command #{args}" do
+        let(:largs) { args }
+        it(&example)
+      end
+    end
+
+    with_args("echo", "FOO") do
+      is_expected.not_to be_error
+    end
+
+    # Test box is going to have to have c:\program files on it, which is perhaps brittle in principle, but
+    # I don't know enough windows to come up with a less brittle test.  Fix it if you've got a better idea.
+    # The tests need to fail though if the argument is not quoted correctly so using `echo` would be poor
+    # because `echo FOO BAR` and `echo "FOO BAR"` aren't any different.
+
+    with_args('dir c:\\program files') do
+      is_expected.to be_error
+    end
+
+    with_args('dir "c:\\program files"') do
+      is_expected.not_to be_error
+    end
+
+    with_args("dir", 'c:\\program files') do
+      is_expected.not_to be_error
+    end
+
+    with_args("dir", 'c:\\program files\\') do
+      is_expected.not_to be_error
+    end
+  end
 end