The CodeQL Alert: Shell Command Built From Environment Values

CodeQL alert #178, "Shell command built from environment values," pointed at eight lines in e2e/scripts/cleanup.js — a script that runs after the E2E suite to drop test data from the database. The instinct when reading "the value came from process.env" is to think it's safe, since you control the environment. The instinct is wrong: anything that lands in a shell string without escaping is a shell injection risk regardless of where the value originated, including your own .env file.

exec() hands a string to /bin/sh; execFile() hands an argv array straight to the binary.

The vulnerable code

const { exec } = require('child_process');
const execPromise = util.promisify(exec);

const dbPassword = process.env.DB_PASSWORD || '';
const command = `PGPASSWORD="${dbPassword}" psql -h ${dbHost} -p ${dbPort} -U ${dbUser} -d ${dbName} -f ${sqlPath}`;
await execPromise(command);

Five separate values — dbPassword, dbHost, dbPort, dbUser, dbName — are interpolated directly into a string that exec() then hands to /bin/sh -c. exec() always spawns a shell; that's its entire contract. Any of those five values containing a shell metacharacter (;, ` `, $(), |`, unbalanced quotes) doesn't just corrupt the command — it terminates it and starts a new one, running with whatever privileges the Node process has.

In this specific case the values come from CI environment variables the project author controls, so the realistic exploit path is narrow. That's exactly why it's worth taking seriously anyway: "the values are trusted today" is a claim about the current codebase, not an invariant the language enforces. A future refactor that lets DB_HOST come from a config file uploaded through an admin panel would turn this into an actual injection point without anyone touching this file again.

The fix (commit abdd43a)

const { exec, execFile } = require('child_process');
const execFilePromise = util.promisify(execFile);

const env = { ...process.env, PGPASSWORD: dbPassword };
const args = ['-h', dbHost, '-p', dbPort, '-U', dbUser, '-d', dbName, '-f', sqlPath];
await execFilePromise('psql', args, { env });

Three changes, in order of importance:

1. execFile instead of exec — no shell is spawned at all. psql is invoked directly with an argv array, so there's no string for a metacharacter to break out of.
2. Arguments passed as array elements, not string-concatenated — each element is handed to the psql process as a single argv entry regardless of its contents.
3. PGPASSWORD passed through the env option instead of prefixed into the command string — it never touches a shell parser at all, and it stops showing up in ps aux output on systems where that matters.

The general rule

exec() and execSync() are for commands that are entirely static strings you wrote yourself, with no interpolation. The moment a command needs a variable in it — a hostname, a filename, a password — execFile() (or spawn(), which has the same argv-array contract) is the only safe choice, independent of whether that variable's ultimate source is a form field, a database row, or your own .env file. CodeQL's rule name — "shell command built from environment values" — undersells the fix slightly: the real fix isn't sanitizing the environment value, it's removing the shell from the picture entirely.

Series: Fishing Tracker Pro. Next: why the API gets rate-limited before anyone has ever tried to attack it.