Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support commands/users/groups/directories with spaces or other shell characters #451

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

grosser
Copy link
Contributor

@grosser grosser commented Feb 18, 2019

host = SSHKit::Host.new(hostname: "localhost")
SSHKit::Coordinator.new([host]).each { capture "sh", "-c", "touch 1 && echo 1" }.first.value

this should execute "sh -c touch\ 1 &&\ echo\ 1" and not sh -c touch 1 and echo 1
... same issue for directories/users/groups
... also making user and group use the same form of escaping
... I'm pretty sure nohup should also do sh -c to avoid funky issues

/fyi @eatwithforks @adammw @jonmoter this is why we saw crazy issues :D

... cap-devs, please let me know if this looks correct, or if I understood something wrong

@grosser grosser force-pushed the grosser/escape branch 2 times, most recently from f0124b7 to 18e44f9 Compare February 18, 2019 23:33
@grosser grosser changed the title support commands/users/groups with spaces or other strange characters support commands/users/groups/directories with spaces or other shell characters Feb 18, 2019
@mattbrictson
Copy link
Member

This is a breaking change to the behavior of SSHKit. I won't be able to accept this PR as is.

However there are still improvements here that I can definitely get behind, if you'd like to split them off into another PR. For example I like replacing the sprintf("...%s", yield) statements with the much more readable "...#{yield}" template strings.

Also, how about adding to the documentation instead of changing the behavior? For example the docs could offer advice about using .shellescape when dealing with strings that are not shell-safe. Like:

capture "sh", "-c", "touch 1 && echo 1".shellescape

@grosser
Copy link
Contributor Author

grosser commented Feb 19, 2019

Afaik ruby convention is that passing arrays to system calls is supposed to escape them, that's how system/exec/sh/IO.popen etc work. Not doing that is also violating the users expectation:

foo "bar baz" is
call the binary foo with the argument "bar baz" and not call the binary foo with the argument "bar" and "baz"

Agreed that this is not a simple patch, and will need a minor or major bump, but I think it's the obvious/expected way it should work, otherwise it's just "random bugs" since things like adding a user/group will suddenly change what the command means. (also user already had a poor mans escaping for the wrapped command, so at least this can be seen as a bugfix release)

@grosser
Copy link
Contributor Author

grosser commented Feb 19, 2019

#452 for just the sprintf change

@grosser
Copy link
Contributor Author

grosser commented Feb 19, 2019

/cc @leehambley since your name is all over command.rb :D

@grosser
Copy link
Contributor Author

grosser commented Feb 19, 2019

#453 for just the things I'd consider bugfixes

@mattbrictson
Copy link
Member

Afaik ruby convention is that passing arrays to system calls is supposed to escape them, that's how system/exec/sh/IO.popen etc work

I don't think the comparison holds up exactly. In Ruby's system, for example, a string is interpreted by a shell, whereas an array of arguments is executed directly, without a shell. In SSHKit, commands are always executed by a shell.

Arguments about "correctness" aside, SSHKit is a mature library (it has been >5 years since 1.0.0) and as a maintainer I personally place more value on keeping existing users happy and free from surprising changes. I will defer to @leehambley since he probably has more experience in this particular area.

Thanks for splitting up into separate PRs. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants