[vishalshetagar]

Description of the fix

js/command-line-injection now reports remote input that reaches the argument array of spawn/execFile when the shell option is set to true or a nonempty string, including their synchronous variants.

const cp = require("child_process");
const app = require("express")();

app.get("/a", (req, res) => {
  cp.exec("echo hello " + req.query.name); // reported
  res.send("ok");
});

app.get("/b", (req, res) => {
  cp.spawn("echo", ["hello", req.query.name], { shell: true }); // now reported
  res.send("ok");
});

app.get("/c", (req, res) => {
  cp.spawn("echo", ["hello", req.query.name], { shell: "/bin/sh" }); // now reported
  res.send("ok");
});

All three routes are now reported by js/command-line-injection. Argument arrays in these examples with shell: false, shell: "", or no shell option remain unreported.

For child_process.spawn, spawnSync, execFile, and execFileSync, the command-injection sink modeling reuses UnsafeShellCommandConstruction::ShellTrueCommandExecutionSink. The sink is the argument array at the execution call, so array mutations must reach that call to be reported. executesArrayAsShell also recognizes nonempty string-valued shell paths and checks for local writes that overwrite the shell setting before execution.

The child_process model distinguishes argument arrays, options objects, and callbacks. It handles execFile calls with or without a callback, excludes options-only calls from argument-array sinks, and preserves js/useless-use-of-cat findings when callbacks cannot be resolved locally.

Testing

Regression fixtures and expected results cover boolean and string-valued shell options, synchronous and promisified calls, callback-free execFile, forwarded arrays/options, and push/unshift. Negative cases cover disabled, empty, omitted, and overwritten shell options, options-only calls, constant arguments, and array mutations after execution.

The CommandInjection, UnsafeShellCommandConstruction, UselessUseOfCat, and NodeJSLib test suites pass.

Fixes #22825.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[asgerf]

Looks harmless to merge, but without any tests it's hard to be confident that this PR actually does what is claimed by the change note.

[asgerf]

There's a line earlier in this predicate (line 807) that reads:

not result.getALocalSource() instanceof DataFlow::ArrayCreationNode and // looks like argumentlist

This seems to be at odds with what this PR is claiming to solve. It seems that this filter should be disabled when shell=true.

It would also be appropriate with unit tests for this.

[Copilot]

🟡 Changes recommended

The changes can produce false positives and suppress existing findings by misclassifying options and callbacks, counting post-execution array mutations, and honoring overwritten shell values, with regression coverage also missing.

5 open issues

• Distinguish trailing callbacks from exec options · New
• Exclude options-only overloads from argument-array sinks · New
• Report array mutations only when values reach the execution · New
• Resolve the shell option value at the invocation · New
• Add regression fixtures for shell options and argument-array sinks · New

_{Review effort: Max}

[vishalshetagar]

Thanks @asgerf, I’ve pushed an update with regression tests and expected results.

The array filter is in getOptionsArg(), so I’ve kept it to distinguish the options object from the argument array. The new tests verify that getArgumentList() still recognizes arrays when shell is enabled.

I also addressed Copilot’s findings around unresolved callbacks, options-only calls, array mutations after execution, and overwritten shell settings. All four affected test suites pass. Would appreciate another look!

[asgerf]

Suggested change

	private predicate isOptionsObject(DataFlow::Node node) {
	pragma[inline]
	private predicate isOptionsObject(DataFlow::Node node) {

This predicate does not bind this so it needs to be inlined

[asgerf]

Suggested change

	* Command injection modeling now recognizes argument arrays passed to `spawn` and `execFile` when the `shell` option is enabled, including string-valued shell paths and calls to `execFile` without a callback. The model distinguishes argument arrays, options objects, and callbacks, and accounts for local writes to the `shell` option before execution.
	* The command injection queries now properly recognize argument arrays passed to `spawn` and `execFile` as sinks when the `shell` option is enabled.

[vishalshetagar]

Thanks @asgerf, I’ve applied both suggestions: added pragma[inline] to isOptionsObject and updated the change note to your wording. The NodeJSLib tests and formatting checks pass.

[asgerf]

Thanks for the contribution @vishalshetagar, and for addressing all my comments! Let's merge this.

[vishalshetagar]

Thanks @asgerf for reviewing and merging this, and for the guidance along the way! Glad I could contribute.