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

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:

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

@drdavella

@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

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?

@clavedeluna

@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.

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

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

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

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:

The type of the first arg.
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]

Navigation Menu

Repository navigation

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

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Activity

clavedeluna commented on Mar 18, 2024

clavedeluna commented on Mar 18, 2024

drdavella commented on Mar 19, 2024

clavedeluna commented on Mar 19, 2024

LucasFaudman commented on Apr 20, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

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