Inserts semicolon on line breaks
atombender opened this issue · 16 comments
Test case:
require 'tty-command'
command = %{bash -c "echo 'a\nb' >/tmp/myfile"}
# This inserts the semicolon
puts "---1"
cmd = TTY::Command.new(printer: :pretty)
cmd.run(command)
puts File.read("/tmp/myfile")
# This works
puts "---2"
system(command)
puts File.read("/tmp/myfile")This prints:
---1
[8c782eed] Running bash -c "echo 'a; b' >/tmp/myfile"
[8c782eed] Finished in 0.005 seconds with exit status 0 (successful)
a; b
---2
a
b
As you can see, the line break is replaced with ;, even though it's in the middle of a string argument.
Also tried escaping with $'\n', but it's still broken up.
I don't think tty-command should interfere with the contents of the command line like this.
While running bash like this is of little use, we use this occasionally to wrap the subshell in sudo, among other good reasons.
Is there a way to prevent it from doing it? I tried the vararg syntax, but that leads to fail (I don't know what kind of mangling it's doing here, some kind of attempt to escape the contents?):
cmd = TTY::Command.new(printer: :pretty)
cmd.run(:bash, "-c", "echo 'a\nb'")
Output:
[6caaddb3] Running bash -c 'echo 'a
b''a
b'''
[6caaddb3] a
[6caaddb3] sh: line 1: ba: command not found
[6caaddb3] sh: -c: line 2: unexpected EOF while looking for matching `''
[6caaddb3] sh: -c: line 3: syntax error: unexpected end of file
[6caaddb3] Finished in 0.007 seconds with exit status 2 (failed)
/Users/alex/.rbenv/versions/2.1.10/lib/ruby/gems/2.1.0/gems/tty-command-0.3.2/lib/tty/command.rb:91:in `run': Running `bash -c 'echo 'a (TTY::Command::ExitError)
b''a
b'''` failed with
exit status: 2
stdout: a
stderr: sh: line 1: ba: command not found
sh: -c: line 2: unexpected EOF while looking for matching `''
sh: -c: line 3: syntax error: unexpected end of file
from test.rb:16:in `<main>'
Thanks for using the library. I had to jog my memory how it all works 😄
Basically, if you pass command as a single argument, any newline is replaced with colon as the command is assumed to be made up of many parts and logically separated see test
The actual method that performs sanitanization of command input is here
In order to execute command the way you have intended you would need to do the following:
cmd = TTY::Command.new(printer: :pretty)
cmd.run(:echo, "a\nb", :out => 'tmp/myfile')This has the added safety benefit of ensuring that arguments are escaped and helps kicking the bash habit.
Thanks. That doesn't work, because the whole point is that I'm passing a quoted string to bash (partly so that bash can evaluate whatever is inside using bash rules).
If I were running echo directly, no problem: But I need to pass a string containing a line break to bash. So there's one level of indirection here, and the library is not understanding that the line break is already inside a string.
In particular, my real use case needs to do this because it's using sudo to run the actual command:
sudo bash -c <command>
I can't run sudo echo, obviously, because echo is not a binary, it's a special shell command.
For now, I think my workaround is to pass a script on stdin to bash, to avoid this mangling.
I think that kind of optimistic parsing of input is bad form generally. The library doesn't understand quoted strings, so it shouldn't attempt to change them.
I totally see you point and I'm not precious about this behaviour. With a hindsight that's probably too much guessing on part of library. Lesson learnt. Do you have time to work on this? I'm rather swamped for next few days but if you submit PR I promise to review and release quickly.
I think instead of completely dropping this feature I suggest we add command option such as :sanitize that user has to specifically opt into to enable this behaviour:
cmd.run("while test 1\n sleep 1\n done", sanitize: true)
# => while test 1; sleep 1; doneI'm aiming at most intuitive solution. Any thoughts?
The point is, if you don't drop the current behaviour, you're still doing it wrong. The only safe way to replace line break with ";" is to parse bash's grammar to know whether a line break is terminating an expression or is inside a string. Bash grammar is complicated — you'd need to be aware of things like $'\n', "here docs", etc. And even then, that would be specific to bash. Other shells have their own grammars.
Anyway, are you sure that the semicolon is needed? spawn("echo a\necho b") works correctly here.
Completely agree, lexing bash scripts is beyond the scope. Now thinking about it and checking the test again, the original intention was to process here docs this way. Therefore, maybe we keep the option specific to this case?
But are you sure it's needed? This works fine here:
spawn(%{cat <<END | wc -l
a
b
c
END
})
Prints 3.
It was more to do with collapsing here docs for logging purposes, otherwise they could expand the output in unpredictable ways.
Isn't that a presentation concern that affects the logger, not the spawning logic? You can collapse the log output however you want, but you'll want to run the raw command.
Agree. I think what needs to be done here is that we don't introduce any options or delete stuff etc... Instead the Cmd has to be slightly changed to separate to_command and to_s calls more cleanly. So that to_command which gets passed to spawn is the actual unsanitized command and the to_s deals with pretty output.
Having said all that, I think allowing this behaviour in logs is also treacherous. I quite often copy commands from logs to run them separately. Therefore having a command that has been artificially mingled may introduce other issues. Ok, I think we should just drop this feature all together.
👍
Do you have time to submit PR? I'm currently working on content for two presentations that need finishing this weekend. Busy times. But I will try to release new version as soon as I can.
Released v0.4.0.
Thanks for taking the time, which I didn't have. 👍