Setting shell=False requires the command to be an list of strings

[drdavella]

Right now our codemod does not account for the fact that in most cases for shell=False to work the command needs to be a list of strings rather than a single string.

For example, the following is valid:

subprocess.run("echo foo", shell=True)

Whereas the following is not:

subprocess.run("echo foo", shell=False)
FileNotFoundError: [Errno 2] No such file or directory: 'echo foo'

Instead, it needs to be the following:

subprocess.run(["echo", "foo"], shell=False)

We have a few options here but the most correct is probably to do the following:

1. Detect whether the first argument to the subprocess call is a string or array
2. In cases where this is not possible, do not make a modification
3. If the argument is an array, make the change
4. If the argument is a string, split the string on empty spaces and convert to an array

[clavedeluna]

@drdavella just to be clear since you did not specify the codemod, it's subprocess-shell-false you're talking about, not sandbox-process-creation

[clavedeluna]

This is a good refinement, so I'm bringing this test case that I thought about to see how we want to handle it.

Say we want to run ls -l | grep '.txt' with subprocess. This has a pipe which is a shell feature. The way you'd write the code for this would be
subprocess.run("ls -l | grep '.txt'", shell=True) which would correctly ls any .txt files. If we follow the suggestions in the issue, we would end up converting this to subprocess.run(["ls", "-l", "|", "grep", "'.txt'"], shell=False) which breaks entirely.

There are more complicated ways to handle shell features like a pipe, we could use Popen and subprocess.PIPE, but that deviates from the purpose of this codemod I feel like.

So what do we want to do? Do we want to check if strings like "|" or other shell feature strings are present and not change it?

[drdavella]

@clavedeluna that's a really good question. There are definitely valid cases for using shell=True. Maybe the safest heuristic is to simply ignore entirely any cases that have string literals instead of arrays.

[clavedeluna]

Would be a similar logic to #263
So perhaps the most refined heuristic would be to check if:

1. if argument is str literal, subprocess.run("echo foo", shell=True), don't do anything
   1a. similarly, if arg is a var pointing to str literal

cmd = "echo foo"
subprocess.run(cmd, shell=True)

don't do anything

2. if argument is a list, check if all list elements resolve to a str literal, if so, don't do anything
3. else, run the codemod

[LucasFaudman]

subprocess actually does not require a list of strings when run with shell=False, it requires that the first arg be a string |bytes that is a path which exists and resolves to a valid executable.

Args are handled as a list regardless of the arg type.
When shell=True ['/usr/bin/sh' , '-c'] is just added to the front of the args list , unless the executable kwarg is set then it over rides args[0]

So to fully solve this issue two things are needed:

1. The type of the first arg.
2. The value of the executable kwarg if set to determine if it is a shell. If it is, this is the same as shell=True

If the type can be determined to be a string or bytes, then arg -> shlex.split(arg) could be a simple way to improve the codeMod.

Thanks guys have a great weekend!

In 3.11 this is at line 1800 of subprocess.py

            if isinstance(args, (str, bytes)):
                args = [args] # String or bytes arg are allowed when shell=True, string or bytes are then placed in list
            elif isinstance(args, os.PathLike):
                if shell:
                    raise TypeError('path-like args is not allowed when '
                                    'shell is true')
                args = [args]
            else:
                args = list(args)

            if shell:
                # On Android the default shell is at '/system/bin/sh'.
                unix_shell = ('/system/bin/sh' if
                          hasattr(sys, 'getandroidapilevel') else '/bin/sh')
                args = [unix_shell, "-c"] + args
                if executable:
                    args[0] = executable

            if executable is None:
                executable = args[0]