Fix escaped quotes bug in Command#group #425
Conversation
Command#group incorrectly escapes double quotes, resulting in a syntax error when specifying the group a command should be executed as. This issue manifested when user command quotes [changed from double quotes to single quotes](capistrano@fbe1775#diff-4eaa2c55e6802df7b49a8e7a2f7584d5). This fix removes the double quote escaping in order to prevent a syntax error when executing the command.
|
Could I get a second opinion on this please either @mattbrictson or @will-in-wi it looks OK for me, but the escaped shell escaping thing always blows my mind. |
|
I think this makes sense. Shell escaping is tricky to follow. |
There was a problem hiding this comment.
I've never used sg and so I'm not sure I can 100% predict the consequences of these changes. But I agree the current behavior is broken and this looks like a good fix. Also, nice tests! 💯
My only concern is the code and the tests are a bit hard to follow due to the various levels of Ruby string-escaping that is layered on top of the shell-escaping that is being implemented. See my suggestions below for possible improvements in that regard..
lib/sshkit/command.rb
Outdated
| @@ -182,7 +182,7 @@ def umask(&_block) | |||
|
|
|||
| def group(&_block) | |||
| return yield unless options[:group] | |||
| "sg #{options[:group]} -c \\\"%s\\\"" % %Q{#{yield}} | |||
| "sg #{options[:group]} -c \"%s\"" % %Q{#{yield}} | |||
There was a problem hiding this comment.
Since we're already looking at this part of the code, I think there are some additional changes we can do to make this much easier to read.
First, I want to point out that:
%Q{#{yield}}is the same as"#{yield}""#{yield}"is the same asyield.to_s- We always pass a string in the block, so
yield.to_sis the same asyield
Taking this further:
"... %s ..." % yieldis the same as"... #{yield} ...""... \" ..."can be written without escapes as e.g.%Q(... " ...)
And so this whole line could be much simplified to:
%Q(sg #{options[:group]} -c "#{yield}")
test/unit/test_command.rb
Outdated
| @@ -102,12 +102,12 @@ def test_working_as_a_given_user | |||
|
|
|||
| 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 | |||
There was a problem hiding this comment.
I think a single-quoted string would improve readability here.
assert_equal 'sg devvers -c "/usr/bin/env whoami"', c.to_command
test/unit/test_command.rb
Outdated
| end | ||
|
|
||
| def test_working_as_a_given_user_and_group | ||
| c = Command.new(:whoami, user: :anotheruser, group: :devvers) | ||
| assert_equal "sudo -u anotheruser -- sh -c 'sg devvers -c \\\"/usr/bin/env whoami\\\"'", c.to_command | ||
| assert_equal "sudo -u anotheruser -- sh -c 'sg devvers -c \"/usr/bin/env whoami\"'", c.to_command |
There was a problem hiding this comment.
%Q might help here, too.
assert_equal %Q(sudo -u anotheruser -- sh -c 'sg devvers -c "/usr/bin/env whoami"'), c.to_command
|
Excellent feedback @mattbrictson thanks for doing the legwork to figure out how we can trip off four years of oxidation from this dark corner of the code. |
|
Hi @mattbrictson Great feedback! I've incorporate the suggested changes, and things seem a lot more readable now. Mind taking another look? |
|
Looks good, thanks! 🙌 I'll squash this down to one commit and merge the PR. |
## [1.17.0][] (2018-07-07) * [#430](capistrano/sshkit#430): [Feature] Command Argument STDOUT/capistrano.log Hiding - [@NorseGaud](https://github.com/NorseGaud) ## [1.16.1][] (2018-05-20) * [#425](capistrano/sshkit#425): Command#group incorrectly escapes double quotes, resulting in a a syntax error when specifying the group execution using `as`. This issue manifested when user command quotes changed from double quotes to single quotes. This fix removes the double quote escaping - [@pblesi](https://github.com/pblesi).
## [1.18.0][] (2018-10-21) * [#435](capistrano/sshkit#435): Consistent verbosity configuration #capture and #test methods - [@NikolayRys](https://github.com/NikolayRys) ## [1.17.0][] (2018-07-07) * [#430](capistrano/sshkit#430): [Feature] Command Argument STDOUT/capistrano.log Hiding - [@NorseGaud](https://github.com/NorseGaud) ## [1.16.1][] (2018-05-20) * [#425](capistrano/sshkit#425): Command#group incorrectly escapes double quotes, resulting in a a syntax error when specifying the group execution using `as`. This issue manifested when user command quotes changed from double quotes to single quotes. This fix removes the double quote escaping - [@pblesi](https://github.com/pblesi).
Command#group incorrectly escapes double quotes, resulting in a syntax
error when specifying the group a command should be executed as. This
issue manifested when user command quotes changed from double quotes to
single quotes. This fix removes the double quote escaping in order to prevent a syntax error when executing the command.
This addresses issue #424