eslint-factory: add no-child-process-interpolated-command to catch shell-injection command strings#47555
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
no-child-process-interpolated-command to catch shell-injection command strings
There was a problem hiding this comment.
Pull request overview
Adds a security-focused ESLint rule for detecting shell-injection-prone child_process command interpolation.
Changes:
- Implements binding and shell-option detection.
- Adds rule tests and plugin registration.
- Documents supported and excluded patterns.
Show a summary per file
| File | Description |
|---|---|
no-child-process-interpolated-command.ts |
Implements the rule. |
no-child-process-interpolated-command.test.ts |
Tests command patterns and bindings. |
src/index.ts |
Registers the plugin rule. |
README.md |
Documents usage and scope. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Medium
| "prefer-core-logging": preferCoreLoggingRule, | ||
| "no-core-error-then-process-exit": noCoreErrorThenProcessExitRule, | ||
| "no-core-error-then-process-exitcode": noCoreErrorThenProcessExitCodeRule, | ||
| "no-child-process-interpolated-command": noChildProcessInterpolatedCommandRule, |
|
|
||
| function hasShellTrueOptions(node: TSESTree.CallExpression, method: ChildProcessMethod): boolean { | ||
| if (!requiresShellTrue(method)) return true; | ||
| return isShellTrueOption(node.arguments[1]) || isShellTrueOption(node.arguments[2]); |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47555 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 81/100 — Excellent
📊 Metrics (14 scenarios covered)
Detailed AnalysisTest:
Strengths:
Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes on correctness gaps and a missing activation step.
📋 Key Themes & Highlights
Issues
- Rule never activates — registered in the plugin but absent from
eslint.config.cjs; the linter will never fire on the target codebase. shell: 'sh'bypass —isShellTrueOptionchecks only=== true; string shell values (valid per Node.js docs) are silently ignored.- Variable options false-negative —
const opts = { shell: true }; spawn(cmd, opts)is not flagged because only inline object literals are inspected. - Test structure — a single monolithic
itblock makes failures hard to diagnose; edge cases for the above gaps are absent.
Positive Highlights
- ✅ Thorough binding-resolution logic covering require, ESM imports, destructuring, and member aliases.
- ✅ Clean scope boundary: spawn/execFile only flagged when
shell: trueis explicitly set. - ✅ Clear README section distinguishing this rule from
no-exec-interpolated-command. - ✅ Good coverage of the happy path — 7 valid + 7 invalid cases all pass.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 41.7 AIC · ⌖ 4.94 AIC · ⊞ 6.7K
Comment /matt to run again
|
|
||
| function hasShellTrueOptions(node: TSESTree.CallExpression, method: ChildProcessMethod): boolean { | ||
| if (!requiresShellTrue(method)) return true; | ||
| return isShellTrueOption(node.arguments[1]) || isShellTrueOption(node.arguments[2]); |
There was a problem hiding this comment.
[/tdd] isShellTrueOption only handles inline object literals — const opts = { shell: true }; spawn(cmd, opts) is not flagged even though the call is shell-evaluated. This creates a false-negative that lets injection slip through.
💡 Suggested approach
Resolve the identifier to its initialiser using scope analysis, similar to how isChildProcessObjectBinding resolves cp bindings. If the variable's init is an ObjectExpression, inspect it the same way as an inline literal.
@copilot please address this.
| }); | ||
|
|
||
| describe("no-child-process-interpolated-command", () => { | ||
| it("flags dynamic child_process command strings for shell-evaluated methods", () => { |
There was a problem hiding this comment.
[/tdd] The test suite is a single it block with 14 cases, all testing one axis at a time. There are no tests for the variable-options false-negative (const opts = { shell: true }), the shell: 'sh' string value (truthy but not === true), or execFile when arguments come in position 1 vs position 2. Splitting into focused it blocks per scenario would make failures much easier to pinpoint.
💡 Suggested structure
describe('shell option detection', () => {
it('flags spawn with inline { shell: true }', ...)
it('does not flag spawn with variable opts containing { shell: true } (known limitation)', ...)
it('does not flag spawn with { shell: "sh" } — only boolean true is recognised', ...)
})
describe('execFile argument positions', () => {
it('checks position 1 (no args array)', ...)
it('checks position 2 (with args array)', ...)
})Document known limitations as skipped/todo tests so they act as regression anchors.
@copilot please address this.
|
|
||
| for (const prop of optionsArg.properties) { | ||
| if (prop.type !== AST_NODE_TYPES.Property || prop.computed) continue; | ||
|
|
There was a problem hiding this comment.
[/tdd] isShellTrueOption accepts only shell: true (boolean). A call with shell: 'sh' or shell: '/bin/bash' is also shell-evaluated but will not be flagged, creating a silent bypass.
💡 Suggested fix
Change the value check to treat any truthy shell value as shell-enabled:
const val = prop.value;
if (val.type === AST_NODE_TYPES.Literal) {
return !!val.value; // true, 'sh', '/bin/bash' all truthy
}Add a test case: spawn(\cmd ${x}`, { shell: 'sh' })` should be invalid.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — Good security intent, but multiple correctness gaps must be addressed before this protects anything in production.
Blocking issues (4)
Critical: Rule is never activated
Adding it to index.ts registers it in the plugin object, but eslint.config.cjs is what actually runs against source. The rule is missing from the config, so it currently has zero effect on actions/setup/js.
High: Variable-reference options bypass
const opts = { shell: true }; spawn(\git ${branch}`, [], opts)is not flagged.isShellTrueOptiononly sees inlineObjectExpression` nodes. For a security rule, a trivially-avoidable false negative is a significant gap. At minimum, document the limitation; ideally flag any non-literal options argument as potentially shell-enabled.
High: shell: '/bin/bash' (truthy string) not detected
Node.js accepts a shell path string as the shell option. prop.value.value === true misses it. Change to !!prop.value.value to cover all truthy literals.
Medium: String-literal ESM import specifier missed
import { 'exec' as run } from 'child_process' (ES2022) produces a Literal imported node, which falls through to null and is never matched.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 50.2 AIC · ⌖ 5.04 AIC · ⊞ 5.7K
Comment /review to run again
| const keyName = prop.key.type === AST_NODE_TYPES.Identifier ? prop.key.name : prop.key.type === AST_NODE_TYPES.Literal ? prop.key.value : null; | ||
| if (keyName !== "shell") continue; | ||
|
|
||
| return prop.value.type === AST_NODE_TYPES.Literal && prop.value.value === true; |
There was a problem hiding this comment.
shell: true check misses truthy string values: { shell: '/bin/bash' } also evaluates via shell but is not flagged, leaving a gap in injection detection.
💡 Details and fix
Node.js accepts any truthy value for shell — including a string path to the shell binary. The current check prop.value.value === true only matches the boolean true:
// Not flagged today:
spawn(`git checkout ${branch}`, { shell: '/bin/bash' }); // shell injection riskBroaden the check to cover any truthy shell value:
return prop.value.type === AST_NODE_TYPES.Literal && !!prop.value.value;This catches shell: true, shell: '/bin/sh', shell: '/bin/bash', etc.
| "no-child-process-interpolated-command": noChildProcessInterpolatedCommandRule, | ||
| "no-exec-interpolated-command": noExecInterpolatedCommandRule, | ||
| "require-execsync-try-catch": requireExecSyncTryCatchRule, | ||
| "require-execfilesync-try-catch": requireExecFileSyncTryCatchRule, |
There was a problem hiding this comment.
Rule is not activated in eslint.config.cjs: registering the rule in index.ts doesn't enable it — the config file must also include an entry, otherwise the rule never runs on actions/setup/js.
💡 Fix
Add to eslint-factory/eslint.config.cjs inside the rules block:
"gh-aw-custom/no-child-process-interpolated-command": "warn",Without this, the rule is dead code in production even though it tests fine in isolation.
|
|
||
| function hasShellTrueOptions(node: TSESTree.CallExpression, method: ChildProcessMethod): boolean { | ||
| if (!requiresShellTrue(method)) return true; | ||
| return isShellTrueOption(node.arguments[1]) || isShellTrueOption(node.arguments[2]); |
There was a problem hiding this comment.
Variable-reference options object silently bypasses detection: const opts = { shell: true }; spawn(\git ${branch}`, [], opts)is not flagged becauseisShellTrueOptiononly handles inlineObjectExpression` nodes.
💡 Details
isShellTrueOption immediately returns false for any non-ObjectExpression argument, including Identifier references to option objects. This is a false negative for the rule's core security purpose — a developer can trivially avoid the lint check by hoisting the options:
const opts = { shell: true };
spawn(`rm -rf ${userInput}`, [], opts); // NOT flaggedFull resolution (resolving the variable through scope) is non-trivial, but the limitation should at minimum be documented in the rule description and/or a code comment so future maintainers know the boundary.
Alternatively, for the shell: true check, emit a warning whenever the options argument is an Identifier or other non-literal expression (conservative over-flagging is preferable to under-flagging for injection rules).
| for (const def of variable.defs) { | ||
| if (isChildProcessImportBinding(def) && def.node.type === AST_NODE_TYPES.ImportSpecifier) { | ||
| const importedName = def.node.imported.type === AST_NODE_TYPES.Identifier ? def.node.imported.name : null; | ||
| if (importedName === method) return true; |
There was a problem hiding this comment.
String-literal ESM import specifier missed: import { 'exec' as run } from 'child_process' (ES2022 module string names) is silently skipped — run(\git ${cmd}`)` would not be flagged.
💡 Details and fix
At line ~60, the import binding check does:
const importedName = def.node.imported.type === AST_NODE_TYPES.Identifier
? def.node.imported.name
: null; // <-- string literal specifier falls through as nullString-name imports (import { 'exec' as run }) use an AST_NODE_TYPES.Literal node for imported. Fix:
const importedName =
def.node.imported.type === AST_NODE_TYPES.Identifier
? def.node.imported.name
: def.node.imported.type === AST_NODE_TYPES.Literal
? String(def.node.imported.value)
: null;This is an edge case, but it's a silent false negative in a security rule.
There was a problem hiding this comment.
Review: no-child-process-interpolated-command
Good addition — shell-injection via interpolated command strings is a real risk, and the rule logic, binding resolution, and docs are well-structured. A few blocking issues need to be addressed before merging.
Blocking
-
Rule never activates (pre-existing comment, line 59 of
index.ts): The rule is registered in the plugin map but never added toeslint-factory/eslint.config.cjs. Without an entry there, the rule won't run overactions/setup/js. -
Variable-reference shell options not detected (pre-existing comment, line 50 of rule):
isShellTrueOptiononly handles inline{ shell: true }object literals.const opts = { shell: true }; spawn(cmd, opts)silently passes. -
Spread options silently skipped (new comment, line 46 of rule): A
SpreadElementin the options position is treated as non-shell and bypassed.spawn(cmd ${x}, ...opts)is missed entirely.
Non-blocking
- No invalid ESM test (new comment, line 29 of test): The ESM import branch is only exercised by a single valid case; an invalid ESM case should be added to confirm coverage.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 42.9 AIC · ⌖ 4.7 AIC · ⊞ 5K
| ], | ||
| invalid: [ | ||
| { | ||
| code: `const { execSync } = require("child_process"); execSync(\`git checkout \${branch}\`);`, |
There was a problem hiding this comment.
Missing invalid ESM test cases.
The test suite only validates a single valid ESM case but contains no invalid ESM examples. The ESM import path through isChildProcessImportBinding is a separate code branch from the require path, so it deserves at least one invalid case (e.g. import { execSync } from "child_process"; execSync(\git checkout ${branch}`)`) to confirm that branch is actually exercised.
@copilot please address this.
|
|
||
| function requiresShellTrue(method: ChildProcessMethod): boolean { | ||
| return SHELL_CONDITIONAL_METHODS.has(method); | ||
| } |
There was a problem hiding this comment.
Spread element in options position silently skips the shell check.
isShellTrueOption returns false early for SpreadElement, so spawn(cmd, ...opts) or spawn(cmd, args, ...opts) is treated as non-shell and skipped. If a spread contains { shell: true }, the rule will silently miss the violation. The safer default would be to treat an unknown spread as possibly shell-enabled (return true from the spread branch) so the rule errs on the side of flagging rather than silently passing.
@copilot please address this.
🔍 PR Triage — §30008120537
Notes: New eslint rule
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the unresolved rule-feedback items in commit c83b351. I enabled I ran local validation ( |
…g note Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
eslint-factorypreviously enforced interpolated-command safety only for github-script’s injectedexecAPI (@actions/execarg-splitting). This change adds dedicated coverage forchild_processshell-evaluated command strings, where interpolation is a shell-injection risk.What this adds
no-child-process-interpolated-command+concatenation when passed as the command string to:exec,execSyncspawn,spawnSyncwhenshell: trueexecFile,execFileSyncwhenshell: trueBinding resolution covered
require("child_process")andrequire("node:child_process"){ execSync })cp.execSync(...))const run = cp.execSync; run(...))Scope boundaries
spawn/spawnSyncwithout shell options.execFile/execFileSyncwithoutshell: true.exec.exec(...)/exec.getExecOutput(...); that remains underno-exec-interpolated-command.Plugin/docs wiring
eslint-factory/src/index.tsno-exec-interpolated-command