fix and unify shell escaping for user/group/directory #453 (#459)

* fix and unify shell escaping for user/group/directory

* avoid duplicate calls to env methods

* Improve CHANGELOG description

Author: mattbrictson

CHANGELOG.md

@@ -6,7 +6,8 @@ appear at the top.
 ## [Unreleased][]
 
   * Your contribution here!
-* [#455](https://github.com/capistrano/sshkit/pull/455): Ensure UUID of commands are stable in logging - [@lazyatom](https://github.com/lazyatom)
+  * [#455](https://github.com/capistrano/sshkit/pull/455): Ensure UUID of commands are stable in logging - [@lazyatom](https://github.com/lazyatom)
+  * [#453](https://github.com/capistrano/sshkit/pull/453): `as` and `within` now properly escape their user/group/path arguments, and the command nested within an `as` block is now properly escaped before passing to `sh -c`. In the unlikely case that you were manually escaping commands passed to SSHKit as a workaround, you will no longer need to do this. See [#458](https://github.com/capistrano/sshkit/issues/458) for examples of what has been fixed. - [@grosser](https://github.com/grosser)
 
 ## [1.18.2][] (2019-02-03)
 

lib/sshkit/backends/abstract.rb

@@ -1,3 +1,5 @@
+require 'shellwords'
+
 module SSHKit
 
   module Backend
@@ -81,12 +83,12 @@ def execute(*args)
       def within(directory, &_block)
         (@pwd ||= []).push directory.to_s
         execute <<-EOTEST, verbosity: Logger::DEBUG
-          if test ! -d #{File.join(@pwd)}
-            then echo "Directory does not exist '#{File.join(@pwd)}'" 1>&2
+          if test ! -d #{File.join(@pwd).shellescape}
+            then echo "Directory does not exist '#{File.join(@pwd).shellescape}'" 1>&2
             false
           fi
-          EOTEST
-          yield
+        EOTEST
+        yield
       ensure
         @pwd.pop
       end
@@ -108,8 +110,8 @@ def as(who, &_block)
           @group = nil
         end
         execute <<-EOTEST, verbosity: Logger::DEBUG
-          if ! sudo -u #{@user} whoami > /dev/null
-            then echo "You cannot switch to user '#{@user}' using sudo, please check the sudoers file" 1>&2
+          if ! sudo -u #{@user.to_s.shellescape} whoami > /dev/null
+            then echo "You cannot switch to user '#{@user.to_s.shellescape}' using sudo, please check the sudoers file" 1>&2
             false
           fi
         EOTEST

lib/sshkit/command.rb

@@ -1,5 +1,6 @@
 require 'digest/sha1'
 require 'securerandom'
+require 'shellwords'
 
 # @author Lee Hambley
 module SSHKit
@@ -142,7 +143,7 @@ def should_map?
 
     def within(&_block)
       return yield unless options[:in]
-      "cd #{options[:in]} && #{yield}"
+      "cd #{options[:in].shellescape} && #{yield}"
     end
 
     def environment_hash
@@ -158,13 +159,15 @@ def environment_string
     end
 
     def with(&_block)
-      return yield unless environment_hash.any?
-      "( export #{environment_string} ; #{yield} )"
+      env_string = environment_string
+      return yield if env_string.empty?
+      "( export #{env_string} ; #{yield} )"
     end
 
     def user(&_block)
       return yield unless options[:user]
-      "sudo -u #{options[:user]} #{environment_string + " " unless environment_string.empty?}-- sh -c '#{yield}'"
+      env_string = environment_string
+      "sudo -u #{options[:user].to_s.shellescape} #{env_string + " " unless env_string.empty?}-- sh -c #{yield.shellescape}"
     end
 
     def in_background(&_block)
@@ -179,7 +182,7 @@ def umask(&_block)
 
     def group(&_block)
       return yield unless options[:group]
-      %Q(sg #{options[:group]} -c "#{yield}")
+      "sg #{options[:group].to_s.shellescape} -c #{yield.shellescape}"
       # We could also use the so-called heredoc format perhaps:
       #"newgrp #{options[:group]} <<EOC \\\"%s\\\" EOC" % %Q{#{yield}}
     end

test/unit/test_command.rb

@@ -51,13 +51,13 @@ def test_including_the_env_with_string_keys
     def test_double_quotes_are_escaped_in_env
       SSHKit.config = nil
       c = Command.new(:rails, 'server', env: {foo: 'asdf"hjkl'})
-      assert_equal %{( export FOO="asdf\\\"hjkl" ; /usr/bin/env rails server )}, c.to_command
+      assert_equal %{( export FOO="asdf\\"hjkl" ; /usr/bin/env rails server )}, c.to_command
     end
 
     def test_percentage_symbol_handled_in_env
       SSHKit.config = nil
       c = Command.new(:rails, 'server', env: {foo: 'asdf%hjkl'}, user: "anotheruser")
-      assert_equal %{( export FOO="asdf%hjkl" ; sudo -u anotheruser FOO=\"asdf%hjkl\" -- sh -c '/usr/bin/env rails server' )}, c.to_command
+      assert_equal %{( export FOO="asdf%hjkl" ; sudo -u anotheruser FOO=\"asdf%hjkl\" -- sh -c /usr/bin/env\\ rails\\ server )}, c.to_command
     end
 
     def test_including_the_env_doesnt_addressively_escape
@@ -84,6 +84,11 @@ def test_working_in_a_given_directory
       assert_equal "cd /opt/sites && /usr/bin/env ls -l", c.to_command
     end
 
+    def test_working_in_a_given_weird_directory
+      c = Command.new(:ls, '-l', in: "/opt/sites and stuff")
+      assert_equal "cd /opt/sites\\ and\\ stuff && /usr/bin/env ls -l", c.to_command
+    end
+
     def test_working_in_a_given_directory_with_env
       c = Command.new(:ls, '-l', in: "/opt/sites", env: {a: :b})
       assert_equal %{cd /opt/sites && ( export A="b" ; /usr/bin/env ls -l )}, c.to_command
@@ -97,17 +102,27 @@ def test_having_a_host_passed
 
     def test_working_as_a_given_user
       c = Command.new(:whoami, user: :anotheruser)
-      assert_equal "sudo -u anotheruser -- sh -c '/usr/bin/env whoami'", c.to_command
+      assert_equal "sudo -u anotheruser -- sh -c /usr/bin/env\\ whoami", c.to_command
+    end
+
+    def test_working_as_a_given_weird_user
+      c = Command.new(:whoami, user: "mr space |")
+      assert_equal "sudo -u mr\\ space\\ \\| -- sh -c /usr/bin/env\\ whoami", c.to_command
     end
 
     def test_working_as_a_given_group
       c = Command.new(:whoami, group: :devvers)
-      assert_equal 'sg devvers -c "/usr/bin/env whoami"', c.to_command
+      assert_equal 'sg devvers -c /usr/bin/env\\ whoami', c.to_command
+    end
+
+    def test_working_as_a_given_weird_group
+      c = Command.new(:whoami, group: "space | group")
+      assert_equal "sg space\\ \\|\\ group -c /usr/bin/env\\ whoami", c.to_command
     end
 
     def test_working_as_a_given_user_and_group
       c = Command.new(:whoami, user: :anotheruser, group: :devvers)
-      assert_equal %Q(sudo -u anotheruser -- sh -c 'sg devvers -c "/usr/bin/env whoami"'), c.to_command
+      assert_equal %Q(sudo -u anotheruser -- sh -c sg\\ devvers\\ -c\\ /usr/bin/env\\\\\\ whoami), c.to_command
     end
 
     def test_umask
@@ -125,13 +140,13 @@ def test_umask_with_working_directory
     def test_umask_with_working_directory_and_user
       SSHKit.config.umask = '007'
       c = Command.new(:touch, 'somefile', in: '/var', user: 'alice')
-      assert_equal "cd /var && umask 007 && sudo -u alice -- sh -c '/usr/bin/env touch somefile'", c.to_command
+      assert_equal "cd /var && umask 007 && sudo -u alice -- sh -c /usr/bin/env\\ touch\\ somefile", c.to_command
     end
 
     def test_umask_with_env_and_working_directory_and_user
       SSHKit.config.umask = '007'
       c = Command.new(:touch, 'somefile', user: 'bob', env: {a: 'b'}, in: '/var')
-      assert_equal %{cd /var && umask 007 && ( export A="b" ; sudo -u bob A="b" -- sh -c '/usr/bin/env touch somefile' )}, c.to_command
+      assert_equal %{cd /var && umask 007 && ( export A="b" ; sudo -u bob A="b" -- sh -c /usr/bin/env\\ touch\\ somefile )}, c.to_command
     end
 
     def test_verbosity_defaults_to_logger_info