I've been doing a few systems administration tasks with Python recently, and shell command invocation directly via the subprocess module is annoyingly clunky (even with the new convenience APIs).

Since subprocess needs to avoid the shell by default for security reasons, I suggest we add the following simple helper functions to shutil:

    def call(cmd, *args, **kwds):
        if args or kwds:
            cmd = cmd.format(*args, **kwds)
        return subprocess.call(cmd, shell=True)

    def check_call(cmd, *args, **kwds):
        if args or kwds:
            cmd = cmd.format(*args, **kwds)
        return subprocess.check_call(cmd, shell=True)

    def check_output(cmd, *args, **kwds):
        if args or kwds:
            cmd = cmd.format(*args, **kwds)
        return subprocess.check_output(cmd, shell=True)


Initially posted at:
http://code.activestate.com/recipes/577891-simple-invocation-of-shell-commands/

Of course, even if others agree in principle, documentation and tests are still needed before this change can go anywhere.

These feel like a shell injection waiting to happen to me.

And that's exactly the problem - a web developer's or security auditor's "shell injection" is a system administrator's "this language sucks".

These wrappers are the kind of thing you want for shell invocations when using Python as a replacement for a shell script or rewriting something that was originally written in Perl, but they're a terrible idea if anything you're interpolating came from an untrusted data source.

Currently, requiring "shell=True" in the arguments to the subprocess calls is considered a sufficient deterrent against people doing the wrong thing. I'm suggesting that requiring "import shutil" instead of "import subprocess" may be a similarly acceptable compromise that better serves the system administrators that choose to use Python for system automation tasks.

Perhaps a better idea would be to use different names, so it's clearer at the point of invocation that the shell is being invoked (and hence shell injection attacks are a potential concern). For example:

  shell_call
  check_shell_call
  check_shell_output

That would make large applications easier to audit (just search for 'shell_') while still making life easier for sysadmins.

Is format() really the best choice here, considering that {}s already have a meaning in the shell?

Of the 3 available options (mod style, string.Template and str.format), yes, str.format is the best choice.

If people want the shell meaning of the braces, they can escape them by doubling them up in the command string.

Why not keeping these helpers in subprocess?

Initially, because I was suggesting the names shadow the subprocess convenience functions so they *had* to live in a different namespace.

However, even after changing the names to explicitly include "shell", I'd like to keep them away from the general subprocess functionality - these wrappers are more convenient for shell operations than the subprocess ones, but it's that very convenience that makes them potentially dangerous in larger applications that may be interpolating data that untrusted users can manipulate.

Since the intended audience is system administrators working on shell-like operations, the shell utility module seems like an appropriate place for them. Both the "import shutil" and the "shell" in the names would then serve as red flags on a code review or security audit.

Hum, in:

return_code = shellcmd.shell_call('ls -l {}', dirname)
listing = shellcmd.check_shell_output('ls -l {}', dirname)

...how do you know that dirname doesn't need some kind of escaping?
This is not only a security issue, but a bug. Even if security doesn't matter on your system, your script will still break and/or do unexpected things.

Also, I don't really understand how your recipe improves things. You're just saving one call to .format(). You would probably have the same saving by using the % operator.

It's a flow thing. This idea was kicked off by the process of translating a large Perl script to Python and paying attention to what the translation made *worse*.

One of the big things it made worse was the translation of "qx" (quoted executable) strings. In Perl, those are almost as neat and tidy as if you were writing directly in the shell:

    qx|ls -l $dirname|

The thought process isn't "build this command and then execute it", it's "run this shell command".

Yes, you have to be careful that "dirname" is legal in the shell, but that usually isn't a big problem in practice, because dirname came from a previous listdir call, or you otherwise know that it's valid to interpolate it into the command (and if it isn't, then the bug lies in the way 'dirname' was populated, not in this operation).

Now, Python's never going to have implicit string interpolation, and that's fine - we have explicit interpolation instead. A direct translation of the above to idiomatic Python 2.7 code looks like the following:

    subprocess.check_output("ls -l {}".format(dirname), shell=True)

Now, if I'm doing system administration tasks (like the script I was translating), then I'm going to be doing that kind of thing a *lot*. I'm also likely to be a sysadmin rather than a programmer, so rather than going "Oh, I can write a helper function for this", my natural reaction is going to be "I'm going to use a language that doesn't get in my way so much".

This proposal is aimed directly at making Python a better language for systems administration by making shell invocation almost as easy as it is in Perl:

    shutil.check_shell_output("ls -l {}", dirname)

Heck, if someone really wanted to, they could even do:

    qx = shutil.check_shell_output
    qx("ls -l {}", dirname)

However, this is also why I *don't* want these methods in subprocess - they put the burden on the user to think about their data as if they were writing shell scripts, because that data is going to get passed straight to the shell for execution without any additional escaping. That's a reasonable idea for a shell utility in shutil, it's not reasonable for a general purpose subprocess manipulation utility in subprocess.

> Yes, you have to be careful that "dirname" is legal in the shell, but
> that usually isn't a big problem in practice, because dirname came
> from a previous listdir call, or you otherwise know that it's valid to
> interpolate it into the command

I don't understand. os.listdir() doesn't escape filenames for you.

Having been bitten several times by almost-working shell commands which
would crash when one of the 10000 files being processed had a space in
it (ironically, I think that was in the Python source tree with some of
the old Mac directories), I think we really don't want to publish a
function which encourages people to pass unescaped arguments to the
shell.

That's a fair point, but I think it actually *improves* the argument for better helper functions, since we can have them automatically invoke shlex.quote() on all of the arguments:

    def _shell_format(cmd, args, kwds):
        args = map(shlex.quote, args)
        kwds = {k:shlex.quote(v) for k, v in kwds}
        return cmd.format(*args, **kwds)

    def _invoke_shell(func, cmd, args, kwds):
        return func(_shell_format(cmd, args, kwds), shell=True)

    def format_shell_call(cmd, *args, kwds):
        return _shell_format(cmd, args, kwds)

    def shell_call(cmd, *args, **kwds):
        return _invoke_shell(subprocess.call, cmds, args, kwds)

    def check_shell_call(cmd, *args, **kwds):
        return _invoke_shell(subprocess.check_call, cmds, args, kwds)

    def check_shell_output(cmd, *args, **kwds):
        return _invoke_shell(subprocess.check_output, cmds, args, kwds)

> That's a fair point, but I think it actually *improves* the argument
> for better helper functions

Agreed :)

> [snip rationale about why shutil and not subprocess]
I’m convinced (with one nit: sh in the shutil name does not ring a security alarm for me, as I understand it as “shell-like conveniences in nice, dont-do-nasty-things-with-stings Python” :) but the shell in check_shell_call does warn).

Automatic call of shlex.quote is an argument in favor of the new helpers.

s/stings/strings/

Unfortunately, I don't think including implicit shlex.quote() calls is going to have the effect I was originally looking for:

>>> subprocess.call("du -hs ../py*", shell=True)
593M    ../py3k
577M    ../py3k_pristine
479M    ../python27
300M    ../python31
381M    ../python32
288K    ../python_swallowed_whole
0

>>> subprocess.call("du -hs {}".format(shlex.quote("../py*")), shell=True)
du: cannot access `../py*': No such file or directory
1

However, tinkering with this did provide some other "feels like using the shell" examples:

>>> subprocess.call("du -hs ~/devel", shell=True)
4.5G    /home/ncoghlan/devel
0

>>> subprocess.call(["du","-hs","~/devel"])
du: cannot access `~/devel': No such file or directory
1

(I'm using the existing subprocess calls rather than the proposed API, since I'm playing with this on the current hg tip without making any changes)

Considering this further, I've realised that the idea of implicit quoting for this style of helper function is misguided on another level - the parameters to be interpolated may not even be strings yet, so attempting to quote them would fail:

>>> subprocess.call("exit {}".format(1), shell=True)
1
>>> shlex.quote(1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/ncoghlan/devel/py3k/Lib/shlex.py", line 285, in quote
    if _find_unsafe(s) is None:
TypeError: expected string or buffer

I'll note that none of these problems will be unique to the new convenience API - they're all a general reflection of the impedance mismatch between typical shell interfaces and filenames that can contain spaces.

I discovered a couple of APIs that were moved from the commands module to the subprocess module in 3.0:
http://docs.python.org/dev/library/subprocess#subprocess.getstatusoutput

However, they have issues, especially on Windows: http://bugs.python.org/issue10197

So part of this patch would include deprecating those two interfaces in favour of the new shutil ones - the existing APIs break the subprocess promise of never invoking the shell implicitly.

After a bit of thought, I realised I could use the string.Formatter API to implement a custom formatter for the shell command helpers that auto-escapes whitespace while leaving the other shell metacharacters alone (so you can still interpolate paths containing wildcards, etc).

People that want to bypass the autoescaping of whitespace can do the interpolation prior to the shell call, those that also want to escape shell metacharacters can use shlex.quote explicitly.

Diff can be seen here:
https://bitbucket.org/ncoghlan/cpython_sandbox/changeset/f19accc9a91b

I realised I could use the convert_field() option in the custom formatter to choose between several interpolation quoting options:

  default - str + shutil.quote_ascii_whitespace
  !q - str + shlex.quote
  !u - unquoted (i.e. no conversion, str.format default behaviour)
  !s - str (as usual)
  !r - repr (as usual)

The most recent commit also exposes public APIs for the formatting aspects (shutil.quote_ascii_whitespace, shutil.shell_format, shutil.shell_format_map)

Some examples:

>>> import shutil
>>> shutil.shell_call("du -hs {}", "../py*")
594M    ../py3k
579M    ../py3k_pristine
480M    ../python27
301M    ../python31
382M    ../python32
288K    ../python_swallowed_whole
0
>>> shutil.shell_call("du -hs {!q}", "../py*")
du: cannot access `../py*': No such file or directory
1
>>> shutil.shell_call("ls {}", "no file")
ls: cannot access no file: No such file or directory
2
>>> shutil.shell_call("ls {!u}", "no file")
ls: cannot access no: No such file or directory
ls: cannot access file: No such file or directory
2

The custom formatter idea sounds brilliant.  Can you test that auto-escaping of spaces works well with glob patterns?

>  default - str + shutil.quote_ascii_whitespace
>  !q - str + shlex.quote
>  !u - unquoted (i.e. no conversion, str.format default behaviour)

The default doesn't look very understandable to me. Why would you quote only some characters and not all of them?

The first version I wrote *did* automatically invoke shlex.quote on all interpolated values, but that breaks wildcard handling. You can see that in the examples I posted above. With the default whitespace escaping (which allows spaces in filenames), wildcard matching still works (thus the list of directories matching the "../py*" pattern), but with full quoting it breaks (thus the "nothing named '../py*'" result).

So I figured the simplest default was "you can have spaces in your filenames, but otherwise you can do what you want". Now that I have the 'unquoted' conversion specifier, I'm open to tweaking that scheme to allow the same chars that shlex.quote does *plus* specifically the wildcard matching chars (i.e. '*', '?').

>  With the default whitespace escaping (which allows spaces in
> filenames), wildcard matching still works (thus the list of
> directories matching the "../py*" pattern), but with full quoting it
> breaks (thus the "nothing named '../py*'" result).

My question is why it would be a good idea to make a difference between
whitespace and other characters. If you use a wildcard pattern,
generally it won't contain spaces at all, so you don't have to quote it.
If you are injecting a normal filename, noticing that whitespace gets
quoted may get you a false sense of security until somebody injects a
wildcard character that won't get quoted.

So what I'm saying is that a middleground between quoting and no quoting
is dangerous and not very useful.

Yeah, I was thinking about this a bit more and realised that I'd rejected the "quote everything by default" approach before I had the idea of providing a custom conversion specifier to disable the implicit string conversion and quoting.

So perhaps a better alternative would be:

  default - str + shlex.quote
  !u - unquoted (i.e. normal str.format default behaviour)

When you have a concise way to explicitly bypass it, making the default behaviour as safe as possible seems like a good way to go.

I’m not sure my question was well phrased.

If I have these files:
  spam.py
  ham.py
  foo bar.py

will a pattern of '*.py' match all of them with your functions, even the one with an embedded space?

After working on the documentation updates to the subprocess module to emphasise the convenience functions, I've now changed my mind and think it makes sense to keep these additions in that module.

Now that the shell helpers will default to using shlex.quote() and require the "!u" conversion specifier to bypass that behaviour, and also will have distinct names from the ordinary non-shell based versions, my original concerns that lead to me wanting to keep them out of subprocess no longer apply. That means the general preference that subprocess be our "one stop shop" for explicit subprocess invocation can once again take precedence.

I'm actually planning to change the API a bit though, dropping "shell_format" and "shell_format_map" in favour of the following helper class:

    class ShellCommand:
        def __init__(self, command, *, **callkwds):
            self.command = command
            self.callkwds = callkwds
        def format(self, *args, **kwds):
            return _ShellFormatter().vformat(_fmt, args, kwds)
        def format_map(self, mapping):
            return _ShellFormatter().vformat(_fmt, (), mapping)
        def call(self, *args, **kwds):
            if args or kwds:
                cmd = _ShellFormatter().vformat(cmd, args, kwds)
            return subprocess.call(cmd, shell=True, **self.callkwds)
        def check_call(self, *args, **kwds):
            if args or kwds:
                cmd = _ShellFormatter().vformat(cmd, args, kwds)
            return subprocess.check_call(cmd, shell=True, **self.callkwds)
    def shell_output(cmd, *args, **kwds):
        if args or kwds:
            cmd = _ShellFormatter().vformat(cmd, args, kwds)
        data = subprocess.check_output(cmd, shell=True, universal_newlines=True)
        if data[-1:] == "\n":
            data = data[:-1]
        return data

The module level helpers would then just become re module style wrappers:

    def shell_call(command, *args, **kwds):
        return ShellCommand(command).shell_call(*args, **kwds)
    def check_shell_call(command, *args, **kwds):
        return ShellCommand(command).check_shell_call(*args, **kwds)
    def shell_output(command, *args, **kwds):
        return ShellCommand(command).shell_output(*args, **kwds)

Éric, to answer your question, with the planned version of the new API, a "*.py" value interpolated with "{!u}" should indeed pick up all of those files, since the wildcard will be passed unmodified to the underlying shell, and all shells I am aware of will accept "foo bar.py" as a match for "*.py".

>        def __init__(self, command, *, **callkwds):
Is the '*' marker needed?

>            self.callkwds = callkwds
These aren’t used in the module-level functions.  What is the use case?

If you forgive me for the nitpick, the docstrings have too much indenting.

> a "*.py" value interpolated with "{!u}" should indeed pick up all of those files,
> since the wildcard will be passed unmodified to the underlying shell
Great.

(This patch will also serve as a nice example of creating a string formatter.  I’ve seen the PyCon video about them, but I thought I was missing a piece since I didn’t see how you hook the format function to your custom formatter; it looks like the answer is that you don’t.)

I like Nick's original idea for a handful of convenience functions but want to caution against adding a bunch of tools that start guessing at what you want.  Adding automatic wildcard expansion, shell quoting/splitting and whatnot can make the module more arcane and harder to learn and remember.  

Please adopt a conservation approach adding the minimal set that has been shown to be necessary.  We can take these tools away if they turn out to have been a bad idea.  It is much easier to add cruft than to take it away.   In the itertools module, a number of tools started their life as just a recipe in the docs and only became builtin after having proved their worth and having proved their API.

Similar to what I did with walkdir, I plan to publish a shellcommand module on PyPI to give people a chance to look at the API and experiment with it without having to patch shutil in the standard library.

The only aspect I'm 100% sold on at the moment for the stdlib version is the use of a custom formatter to implement automatic invocation of shlex.quote(). It makes doing the right thing to avoid shell injection attacks (and whitespace problems in filenames) the path of least resistance: if you use shell_call/shell_check_call/shell_output and let them do the interpolation, then your call will be safe by default and the only thing you have to avoid is using an explicit conversion specifier that will bypass the quoting (i.e. '!a', '!r', '!s' or the custom '!u'). Since fear of encouraging shell injection vulnerabilities has historically been a big issue when it comes to shell invocation from the subprocess module, I think this part is mandatory. Once you do that, then '!u' is needed as a consequence, since sometimes you *will* want to pass unquoted values through in order to use standard numeric formatting options, or just to allow metacharacters to be interpreted by the shell, etc.

Interface wise, I'm still leaning towards just the _ShellFormatter class being private (due to its thread safety limitations), with ShellCommand and the three module level helpers making up the initial public API.

This isn't just speculation - by making the choice to use the *args and **kwds slots of the helper functions for string interpolation, it means they're no longer available for arguments to the Popen constructor.

In my own use case, one of the big things I want to do is invoke shell_call() with stdout and stderr redirected to a log file. Without ShellCommand exposed to let you customise the arguments to Popen, you end up having to essentially reimplement everything the helpers otherwise provide. With ShellCommand, you're able to still handle those less common use cases *without* having to reinvent the wheel on the formatting front:

    cmd = ShellCommand("cat {}", stdout=logfile, stderr=STDOUT)
    cmd.shell_call(cmd_arg)

For simple use cases where you *don't* want to tinker with the Popen args, though, the module level helpers will be the preferred option:

    data = shell_output("cat {}", cmd_arg).decode("utf-8")

The PyPI package to prototype the API details is now available:

http://pypi.python.org/pypi/shell_command
http://shell-command.readthedocs.org
https://bitbucket.org/ncoghlan/shell_command/

For a different take on this concept:
http://julialang.org/manual/running-external-programs/

Bumping the target version for this issue to 3.4.

There's a lot of active development in the shell convenience function space at the moment, including my shell_command, Vinay Sajip's sarge, Kenneth Reitz's envoy and the possibility of doing something inspired by Julia's approach to this problem.

I'm not convinced enough that shell_command gets the security trade-offs right to push for this in the two months we have left before 3.3b1 (plus I have other things I want to work on). In the meantime, "pip install shell_command" is fairly straightforward (or similar for sarge, envoy or one of the other shell helper libraries).

Even more libraries in this vein:
http://plumbum.readthedocs.org/en/latest/
http://amoffat.github.com/sh/

There's also https://pypi.python.org/pypi/sarge

One other thing I *often* want to do when scripting commands is to capture output, but provide some form of "progress" reporting (something like a dot per line of output, usually). That's annoyingly verbose with subprocess.

Having reviewed this issue, I don't see any point to leaving it open.  If any additional shell command helpers are added to the stdlib, it should be an ab-initio discussion based on what exists in the field, and start on python-ideas.  But given current sentiments about the stdlib, I doubt this is something we are likely to do in any case.