Skip to content

fix(eslint): detect inline require('child_process').<method>() in no-child-process-interpolated-command #49069

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.

Sign up for GitHub

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

Merged
pelikhan merged 2 commits into from
Jul 30, 2026
+22 −1
Merged

fix(eslint): detect inline require('child_process').<method>() in no-child-process-interpolated-command #49069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e02b248
Initial plan
Copilot Jul 30, 2026
0d28aac
fix: detect inline require('child_process').<method>() calls in no-ch…
Copilot Jul 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions eslint-factory/src/rules/no-child-process-interpolated-command.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ describe("no-child-process-interpolated-command", () => {
{ code: `const { execSync } = require("child_process"); let cmd = \`git checkout \${branch}\`; cmd = "git status"; execSync(cmd);` },
{ code: `const { execSync } = require("child_process"); (function(cmd) { execSync(cmd); })("git status");` },
{ code: `exec.exec(\`git checkout \${branch}\`, []);` },
{ code: `require("child_process").execSync("git status");` },
{ code: `require("node:child_process").execSync(\`git status\`);` },
{
code: `import { exec } from "node:child_process"; exec("git status");`,
languageOptions: { sourceType: "module" },
Expand Down Expand Up @@ -89,6 +91,18 @@ describe("no-child-process-interpolated-command", () => {
{ messageId: "interpolatedCommand", data: { kind: "dynamic string concatenation", method: "exec" } },
],
},
{
code: `require("child_process").execSync(\`rm -rf \${userInput}\`);`,
errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "execSync" } }],
},
{
code: `require("node:child_process").execSync("rm -rf " + userInput);`,
errors: [{ messageId: "interpolatedCommand", data: { kind: "dynamic string concatenation", method: "execSync" } }],
},
{
code: `require("child_process").spawn(\`git checkout \${branch}\`, { shell: true });`,
errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "spawn" } }],

@github-actions github-actions Bot Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The new invalid test cases cover exec/execSync and spawn, but execFile, execFileSync, and spawnSync with inline require() are not exercised. Since the method list on line 140 now handles all six methods for the inline-require path, missing tests leave that coverage gap invisible.

💡 Suggested additional test cases 
{
  code: `require("child_process").execFile(\`git \${branch}\`, { shell: true });`,
  errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "execFile" } }],
},
{
  code: `require("child_process").spawnSync("cmd " + arg, ["--"], { shell: true });`,
  errors: [{ messageId: "interpolatedCommand", data: { kind: "dynamic string concatenation", method: "spawnSync" } }],
},

@copilot please address this.

},
],
});
});
Expand Down
9 changes: 8 additions & 1 deletion eslint-factory/src/rules/no-child-process-interpolated-command.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,14 @@ function resolveChildProcessMethod(node: TSESTree.CallExpression, sourceCode: TS
}

if (callee.type !== AST_NODE_TYPES.MemberExpression || callee.computed) return null;
if (callee.object.type !== AST_NODE_TYPES.Identifier || callee.property.type !== AST_NODE_TYPES.Identifier) return null;
if (callee.property.type !== AST_NODE_TYPES.Identifier) return null;

if (callee.object.type === AST_NODE_TYPES.CallExpression && isRequireChildProcess(callee.object)) {

@github-actions github-actions Bot Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reusing isRequireChildProcess here inherits its known weakness: it matches any require identifier by name with no scope/binding check, so a shadowed require (e.g. a function parameter named require) is misidentified as the real Node.js require.

💡 Details

This extends the same flawed name-matching helper already noted in an earlier review comment on line 141 (for the identifier-binding path) to a new call site — doubling the false-positive exposure. Example:

function run(require, input) {
  require("child_process").execSync(`echo ${input}`);
}

A proper fix would have isRequireChildProcess verify the require identifier actually resolves to the unbound global, not just match by name.

const method = callee.property.name;
return method === "exec" || method === "execSync" || method === "spawn" || method === "spawnSync" || method === "execFile" || method === "execFileSync" ? method : null;

@github-actions github-actions Bot Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method-validation ternary here is identical to the one on line 147. Both inline the same 6 methods that also appear in the methods array on line 128. Consider extracting a shared constant — e.g. const CHILD_PROCESS_METHOD_SET = new Set<ChildProcessMethod>(["exec", "execSync", "spawn", "spawnSync", "execFile", "execFileSync"]) — and replacing both ternaries with CHILD_PROCESS_METHOD_SET.has(method) ? (method as ChildProcessMethod) : null to keep the list in one place and avoid drift if methods are added later.

@copilot please address this.

@github-actions github-actions Bot Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method allow-list ternary is now duplicated verbatim on lines 140 and 147, so a future edit to the supported method set only touching one copy silently reintroduces a gap.

💡 Details

Both branches repeat:

return method === "exec" || method === "execSync" || method === "spawn" || method === "spawnSync" || method === "execFile" || method === "execFileSync" ? method : null;

A ChildProcessMethod[] list (already declared for the identifier-callee branch at line 128) or a shared Set<string>/helper checked against ChildProcessMethod would remove the duplication and guarantee both paths stay in sync, e.g.:

const CHILD_PROCESS_METHOD_NAMES = new Set<ChildProcessMethod>(["exec","execSync","spawn","spawnSync","execFile","execFileSync"]);
function asChildProcessMethod(name: string): ChildProcessMethod | null {
  return CHILD_PROCESS_METHOD_NAMES.has(name as ChildProcessMethod) ? (name as ChildProcessMethod) : null;
}

@github-actions github-actions Bot Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The method-name whitelist on this line is duplicated verbatim on line 147 — both paths use the same inline ternary. If a new method is added to one but not the other, the rule silently misfires.

💡 Suggested refactor

SHELL_CONDITIONAL_METHODS already uses a Set; follow the same pattern here:

const CHILD_PROCESS_METHODS = new Set<string>(["exec", "execSync", "spawn", "spawnSync", "execFile", "execFileSync"]);

// replace both ternaries with:
return CHILD_PROCESS_METHODS.has(method) ? (method as ChildProcessMethod) : null;

This keeps both code paths in sync automatically.

@copilot please address this.

}
Comment on lines +138 to +141

if (callee.object.type !== AST_NODE_TYPES.Identifier) return null;
if (!isChildProcessObjectBinding(callee.object.name, callee.object, sourceCode)) return null;

const method = callee.property.name;
Expand Down
Loading