don't set shell=True on subprocess calls

[dimbleby]

When poetry executes subprocesses, it sets shell=True, concatenates the list of command elements into a single string, and makes an unconvincing try at escaping the components. See eg #6848.

I don't know what the point is in any of this. Surely it's simpler not to create a shell, and to let subprocess take care of arguments.

On the other hand I'm ony testing on Linux, and - I'm not sure - it may be that the unit tests mock this part sufficiently thoroughly that making them pass doesn't reveal very much about the real-life behaviour.

So I thought I'd see if either the pipeline or a reviewer can tell me why this doesn't work.

[YDX-2147483647]

Just FYI:

• list_to_shell_command was introduced in 598503 to fix poetry install fails because a space in path to env #3013.

• Python doc on security considerations for subprocess:

  subprocess.run will never implicitly call a system shell. This means that all characters (including shell metacharacters) can safely be passed to child processes.

  If the shell is invoked explicitly (shell=True), it is the application’s responsibility to ensure that all whitespace and metacharacters are quoted appropriately to avoid shell injection vulnerabilities. On some platforms (Unix), it is possible to use shlex.quote() for this escaping.

[dimbleby]

in fact list_to_shell_command was introduced a couple of years earlier than that, at 58dbca4

however I think we are agreeing that a more reliable way of dealing with special characters is to let subprocess handle them, so it's all good

[radoering]

There is the following paragraph in the docs:

If shell is True, the specified command will be executed through the shell. This can be useful if you are using Python primarily for the enhanced control flow it offers over most system shells and still want convenient access to other shell features such as shell pipes, filename wildcards, environment variable expansion, and expansion of ~ to a user’s home directory.

Maybe, that's relevant for _run?

[dimbleby]

Happy to hear about and discuss examples where this makes a negative difference; hard, though, to give a useful answer to "maybe"...

[radoering]

It seems Env._run() is never called with user input. (When calling poetry run ..., Env.execute() is called.) Thus, I assume the changes in this PR should be fine if we don't use paths with something like ~ or %appdata% internally.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.