fix(eslint): detect inline require('child_process').<method>() in no-child-process-interpolated-command - #49069
Conversation
…ild-process-interpolated-command Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Not ready to approve
Inline detection incorrectly reports calls where require is locally shadowed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Fixes inline child_process require calls being missed by the ESLint rule.
Changes:
- Resolves methods called directly from
require("child_process"). - Adds static and dynamic inline-require test cases.
File summaries
| File | Description |
|---|---|
eslint-factory/src/rules/no-child-process-interpolated-command.ts |
Adds inline-require method resolution. |
eslint-factory/src/rules/no-child-process-interpolated-command.test.ts |
Tests static, interpolated, concatenated, and shell-enabled calls. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| if (callee.object.type === AST_NODE_TYPES.CallExpression && isRequireChildProcess(callee.object)) { | ||
| const method = callee.property.name; | ||
| return method === "exec" || method === "execSync" || method === "spawn" || method === "spawnSync" || method === "execFile" || method === "execFileSync" ? method : null; | ||
| } |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @-
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(eslint): detect inline require('child_process').<method>()
The fix is correct and well-targeted. The root cause (the early-return guarding against non-Identifier objects in resolveChildProcessMethod) is addressed cleanly, and the new test cases cover the three meaningful new shapes (template literal, string concat, spawn with shell).
One non-blocking suggestion: the method-membership ternary is duplicated verbatim at lines 140 and 147, and the same 6 methods are already listed in the methods array on line 128. Extracting a shared Set<ChildProcessMethod> would consolidate the single source of truth and prevent future drift.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 23.5 AIC · ⌖ 4.86 AIC · ⊞ 5.3K
|
|
||
| if (callee.object.type === AST_NODE_TYPES.CallExpression && isRequireChildProcess(callee.object)) { | ||
| const method = callee.property.name; | ||
| return method === "exec" || method === "execSync" || method === "spawn" || method === "spawnSync" || method === "execFile" || method === "execFileSync" ? method : null; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Verdict: COMMENT
Small, well-tested fix for the inline-require detection gap, but it copy-pastes an existing weak binding check and duplicates a method-validation ternary.
💡 Themes
- The new inline-require branch reuses
isRequireChildProcess, which resolvesrequireby name only (no scope check), extending an already-known false-positive risk (shadowedrequire) to a second call site. - The allowed-method-name check (
exec/execSync/spawn/spawnSync/execFile/execFileSync) is now duplicated verbatim across two branches instead of factored into one list/helper, risking future drift. - Test coverage for the new cases (execSync, string concat, spawn with shell:true) is solid and directly validates the fix.
🔎 Code quality review by PR Code Quality Reviewer · auto · 17.9 AIC · ⌖ 4.68 AIC · ⊞ 7.8K
Comment /review to run again
| 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)) { |
There was a problem hiding this comment.
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.
|
|
||
| if (callee.object.type === AST_NODE_TYPES.CallExpression && isRequireChildProcess(callee.object)) { | ||
| const method = callee.property.name; | ||
| return method === "exec" || method === "execSync" || method === "spawn" || method === "spawnSync" || method === "execFile" || method === "execFileSync" ? method : null; |
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 31.2 AIC · ⌖ 4.94 AIC · ⊞ 7K
Comment /matt to run again
|
|
||
| if (callee.object.type === AST_NODE_TYPES.CallExpression && isRequireChildProcess(callee.object)) { | ||
| const method = callee.property.name; | ||
| return method === "exec" || method === "execSync" || method === "spawn" || method === "spawnSync" || method === "execFile" || method === "execFileSync" ? method : null; |
There was a problem hiding this comment.
[/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.
| }, | ||
| { | ||
| code: `require("child_process").spawn(\`git checkout \${branch}\`, { shell: true });`, | ||
| errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "spawn" } }], |
There was a problem hiding this comment.
[/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.
Triage Summary
Small, well-tested eslint rule fix for inline require(child_process) detection. Not draft, CI green (30/30 checks passing). Good candidate for auto-merge.
|
|
@copilot run pr-finisher skill |
|
🎉 This pull request is included in a new release. Release: |
no-child-process-interpolated-commandmissed the inline-require call shaperequire('child_process').execSync(...)becauseresolveChildProcessMethodrequiredcallee.objectto be anIdentifier— but for inline requires it's aCallExpression, so the rule never fired.Changes
no-child-process-interpolated-command.ts— inresolveChildProcessMethod, split the existingcallee.object.type !== Identifierearly-return into two checks: first test whethercallee.objectis aCallExpressionmatchingisRequireChildProcessand resolve the method directly; otherwise fall through to the existing identifier-binding path.no-child-process-interpolated-command.test.ts— new invalid cases forrequire('child_process').execSync(\...`),require('node:child_process').execSync("..." + x), andrequire('child_process').spawn(`...`, { shell: true })`; new valid cases confirming static-command inline requires produce no false positives.