When writing a custom operation, I was happy to see that commands could either be specified as a full string (to be passed to a shell as-is), or as a StringCommand, where you pass the individual pieces/arguments of the command as an array.
In most contexts, this means that such an array is passed along to the final executable as-is (i.e. your array is passed to the execve syscall, without being processed by a shell).
However, I've found that pyinfra always condenses this array into a shell commandline and passes it to a shell for interpretation. Furthermore, by default, the arguments are not quoted or escaped, meaning that the shell does additional processing, like word splitting, command interpolation, pipeline construction, etc. This is of course convenient in some cases, but to me it is very surprising that this is the default. When using arguments that contain anything that is interpreted by the shell (spaces, pipes, &&, ;, $, etc.), the result is not what you would expect when passing arbitrary strings, leading to potentially dangerous situations (security issues when untrusted data is handled, or accidentally overwriting files when > is used in an argument somewhere).
This can be mitigated by wrapping such arguments in the QuoteString class, but that is errorprone and easy to forget.
Suggested solution
Quoting strings should be the default. Having an UnquotedString (or so) to explicitly indicate arguments that are not to be quoted because they contain shell constructs (and have been manually checked for safety) still allows expressing everything that is currently possible.
Ideally, any command that contains only quoted strings would not even be serialized into a shell command but just passed as an array all the way to the final execve call, but I suspect not all connectors can support this, and it might also be tricky with su/sudo and other wrappers (but when handled carefully, it should be possible).
Compatbility
Simply changing the default will of course break compatibility. But what could be done is introducing a new class Command with the new semantics to replace StringCommand (tbh I'm not so sure what the name StringCommand should mean anyway, so maybe good to replace it). Then the StringCommand class could become a subclass of Command that simply wraps all of its plain string arguments in UnquotedString and passes them to the Command constructor).
Alternatively, StringCommand could be kept as-is and a new wrapper function make_command (or whatever) could be created and documented as the default, which wraps the StringCommand constructor and wraps all plain string arguments in QuoteString.
In any case, calling the StringCommand constructor directly can then produce a deprecation warning (maybe not immediately, but in a few releases) and eventually maybe be removed.
What would you think of this?
When writing a custom operation, I was happy to see that commands could either be specified as a full string (to be passed to a shell as-is), or as a
StringCommand, where you pass the individual pieces/arguments of the command as an array.In most contexts, this means that such an array is passed along to the final executable as-is (i.e. your array is passed to the
execvesyscall, without being processed by a shell).However, I've found that pyinfra always condenses this array into a shell commandline and passes it to a shell for interpretation. Furthermore, by default, the arguments are not quoted or escaped, meaning that the shell does additional processing, like word splitting, command interpolation, pipeline construction, etc. This is of course convenient in some cases, but to me it is very surprising that this is the default. When using arguments that contain anything that is interpreted by the shell (spaces, pipes,
&&,;,$, etc.), the result is not what you would expect when passing arbitrary strings, leading to potentially dangerous situations (security issues when untrusted data is handled, or accidentally overwriting files when>is used in an argument somewhere).This can be mitigated by wrapping such arguments in the
QuoteStringclass, but that is errorprone and easy to forget.Suggested solution
Quoting strings should be the default. Having an
UnquotedString(or so) to explicitly indicate arguments that are not to be quoted because they contain shell constructs (and have been manually checked for safety) still allows expressing everything that is currently possible.Ideally, any command that contains only quoted strings would not even be serialized into a shell command but just passed as an array all the way to the final
execvecall, but I suspect not all connectors can support this, and it might also be tricky with su/sudo and other wrappers (but when handled carefully, it should be possible).Compatbility
Simply changing the default will of course break compatibility. But what could be done is introducing a new class
Commandwith the new semantics to replaceStringCommand(tbh I'm not so sure what the nameStringCommandshould mean anyway, so maybe good to replace it). Then theStringCommandclass could become a subclass ofCommandthat simply wraps all of its plain string arguments inUnquotedStringand passes them to theCommandconstructor).Alternatively,
StringCommandcould be kept as-is and a new wrapper functionmake_command(or whatever) could be created and documented as the default, which wraps theStringCommandconstructor and wraps all plain string arguments inQuoteString.In any case, calling the
StringCommandconstructor directly can then produce a deprecation warning (maybe not immediately, but in a few releases) and eventually maybe be removed.What would you think of this?