Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

context.run(): Expose API to pass command and separate args instead of a single string #977

Open
ibc opened this issue Nov 27, 2023 · 2 comments

Comments

@ibc
Copy link

ibc commented Nov 27, 2023

Currently context.run(command: str, **kwargs: Any) gives very limited chances to the user when dealing with paths with whitespaces or special chars.

mkdir "123\ foo\ \"bar"

Imagine that folder whose name contains whitespaces and double quote symbol. Passing it as argument to a shell command within a interpolated string is basically hell.

In Rust and Node you can spawn a separate command by specifying the command itself and then each argument separately (into an array of strings). This is very convenient to NOT having to deal with string interpolation. Example:

child_process.spawnSync(command, [ args ], ...);

However in Invoke all command arguments must be interpolated into a single command string. I don't know if this is a Python limitation. I just hope that Python exposes an API to specify the command and each separate argument to spawn. And if so, IMHO it would make sense that Invoke exposes it.

@lieryan
Copy link

lieryan commented Dec 19, 2023

Python's subprocesses.Popen() (and all the other convenience methods in the standard library that internally uses it as well as the low-level function calls to exec/spawn processes in os) in the standard library all do have support for spawning subprocesses using a list and in fact the list argument is the recommended way to spawn subprocesses, if you have to spawn subprocesses. The list argument is also necessary to call a multi-call binary (e.g. busybox) which is basically impossible to use in invoke. Using a shell to spawn subprocesses is a major security risk (and also it's bad for performance) and context.run() basically forces you to write insecure tasks.

Many security scanners such as bandit would flag use of subprocesses with the shell/string argument as high security risk. Using context.run() somewhat bypasses the scanners that don't recognise the invoke library, but all of the security risk is pretty much still there, and in fact, worse, since there isn't any way to actually do anything safely with context.run().

I'm actually rather surprised by the complete lack of consideration to do any sort of secure coding in invoke.run(). While there are many use case of invoke where shell security may not be important, there is not even any way for people who need to call subprocesses with semi/untrusted data using context.run().

@achekery
Copy link

Submitted #987 for this qol change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants