fix and unify shell escaping for user/group/directory

[grosser]

• support user/group/directories with spaces
• fixing the broken user/group escaping

... not touching env vars since I think someone might be abusing this for $ var injection, needs more testing

[grosser]

added another commit to avoid calling env methods multiple times

[grosser]

good to go ?

[grosser]

@mattbrictson

[grosser]

@mattbrictson is there anything missing ?

[mattbrictson]

Sorry for the radio silence! To be honest I have not been able to find much time in recent weeks for Capistrano PR reviews and so I am prioritizing those that are low-risk, high-value, and well-documented.

This PR is tricky because it definitely changes existing behavior (as evidenced by the fact that you needed to change existing tests) and I don't want to merge without be absolutely certain all the bases are covered.

I would be more comfortable merging this if the PR description (or alternatively, a bug report linked to this PR) explained in more detail what problem this PR is solving with some examples of errors or unwanted behavior. Also, considering there is some risk of changing existing behavior, it would be nice to explain who might be affected by these changes, and what they could to do mitigate any problems when upgrading. A more detailed CHANGELOG entry that summarizes this would also help.

I know this might be discouraging but given the large number of users of Capistrano and the fact that the project support 100% by volunteers, I tend to be very conservative about merging PRs unless there are good supporting docs and migration paths.

[grosser]

examples:

within

require 'sshkit'

include SSHKit::DSL

on 'server' do
  within 'space here' do
    puts capture "pwd"
  end
end

pwd stderr: bash: line 0: cd: space: No such file or directory

user part 1

on 'bastion1.use1.zdsystest.com' do
  as 'space here' do
    puts capture "echo 1"
  end
end

if ! sudo -u space here whoami > /dev/null; then echo "You cannot switch to user 'space here' using sudo, please check the sudoers file" 1>&2; false; fi stderr: sudo: unknown user: space

user part 2

on 'bastion1.use1.zdsystest.com' do
  as 'nobody' do
    puts capture :echo, "'"
  end
end

echo stderr: bash: -c: line 0: unexpected EOF while looking for matching `''

group part 1

on 'bastion1.use1.zdsystest.com' do
  as group: 'space here' do
    puts capture :echo, '1'
  end
end

echo stderr: sudo: no tty present and no askpass program specified

group part 2

on 'bastion1.use1.zdsystest.com' do
  as group: 'nobody' do
    puts capture :echo, '"'
  end
end

echo stderr: bash: -c: line 0: unexpected EOF while looking for matching `"'

Also in general, this is a gem that connects to maybe hundreds of live servers to execute commands, which is terrifying, it should be built on a solid foundation and not "this kinda seems to work, why change it", shellwords is a builtin library for exactly this usecase. Whatever shady code worked before because it "hacked this hack with another hack" is wrong & dangerous.

The changelog warning could read "if you replied on strange quoting to break out of the surrounding sh -c then this will no longer work" / "if you put env vars into usernames then we will escape them now" ... but afaik it should not break anything for anyone doing regular command execution.

[grosser]

in general it seems like a bad idea to use these user/within apis anyways since they add the overhead of another remote execution (check user exists / check dir exists) to each execution.

[mattbrictson]

I will clean up the CHANGELOG, fix the conflicts and get this merged in. Thanks for the examples. If you don't mind I will move them to their own issue to make them more discoverable.

I hope our future interactions on GitHub will not be so abrasive! 🙂🙏

[grosser]

thx!

…

On Wed, Mar 6, 2019 at 9:47 PM Matt Brictson ***@***.***> wrote: I will clean up the CHANGELOG, fix the conflicts and get this merged in. Thanks for the examples. If you don't mind I will move them to their own issue to make them more discoverable. I hope our future interactions on GitHub will not be so abrasive! 🙂🙏 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#453 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAsZ6E_aSVxd0UkIVAzVL2WOBw5LGV8ks5vUKflgaJpZM4bB8kv> .

[mattbrictson]

Merged via #459

[grosser]

awesome! :D

I'd say let's get this out in a release and if it does not cause complaints within a month then revisit #451