diff --git a/eslint-factory/README.md b/eslint-factory/README.md index 6ae7017e0d9..b7c27f069e6 100644 --- a/eslint-factory/README.md +++ b/eslint-factory/README.md @@ -20,6 +20,7 @@ This project hosts custom ESLint linters for `/actions/setup/js`. |---|---| | [`no-core-exportvariable-non-string`](#no-core-exportvariable-non-string) | Require explicit string values for `core.exportVariable` calls | | [`no-core-setoutput-non-string`](#no-core-setoutput-non-string) | Require explicit string values for `core.setOutput` calls | +| [`no-child-process-interpolated-command`](#no-child-process-interpolated-command) | Disallow interpolated command strings in shell-evaluated `child_process` calls | | [`no-github-request-interpolated-route`](#no-github-request-interpolated-route) | Disallow interpolated route arguments in Octokit `.request()` calls | | [`no-json-stringify-error`](#no-json-stringify-error) | Disallow `JSON.stringify()` on caught error variables | | [`no-throw-plain-object`](#no-throw-plain-object) | Disallow throwing plain object literals | @@ -432,6 +433,33 @@ Prefer `@actions/core` logging methods (`core.info`, `core.debug`) over `console `console.error` and `console.warn` write to **`process.stderr`**, while `core.error` and `core.warning` emit GitHub Actions workflow commands to **`process.stdout`**. For processes that own stdout as a data/protocol channel — such as stdio MCP servers and transports — replacing stderr logging with stdout logging would corrupt the JSON-RPC stream. Because the stream change is not behavior-preserving, the rule never reports `console.error` or `console.warn` and offers no suggestion to replace them. +### `no-child-process-interpolated-command` + +Disallow interpolated template literals and dynamic string concatenation as command arguments to shell-evaluated `child_process` calls. + +Why: command strings evaluated by a shell (`exec`, `execSync`, `spawn` / `spawnSync` with `shell: true`, and `execFile` / `execFileSync` with `shell: true`) can become shell-injection vectors when command content is assembled dynamically. + +**Detected forms (when bound to `child_process` / `node:child_process`):** +- `const { execSync } = require("child_process"); execSync(\`git checkout ${branch}\`)` +- `const cp = require("child_process"); cp.exec("git checkout " + branch)` +- `const run = cp.execSync; run("git checkout " + branch)` — member alias call. +- `spawn(\`git checkout ${branch}\`, { shell: true })` — shell-enabled spawn. +- `execFileSync("git " + branch, ["status"], { shell: true })` — shell-enabled execFileSync. +- `spawn("git checkout " + branch, ...opts)` — spread options are treated conservatively as potentially shell-enabled. +- ESM imports are recognized (`import { execSync } from "node:child_process"`). + +**Not flagged:** +- Fully static command strings (`"git status"`, `` `git status` ``, and fully static `+` concatenations). +- `spawn(cmd, [args])` / `spawnSync(cmd, [args])` without `shell: true`. +- `execFile` / `execFileSync` without `shell: true`. + +**Out of scope:** github-script's injected `exec.exec(...)` / `exec.getExecOutput(...)`. Those are covered by `no-exec-interpolated-command`, which targets `@actions/exec` argument-splitting correctness rather than shell injection. + +**Safer alternatives:** +- Use a static executable and pass arguments as an array (`execFileSync("git", ["checkout", branch])`). +- Avoid `shell: true` unless strictly required. + + ### `require-execsync-try-catch` Require `execSync` calls sourced from `child_process` to be wrapped in `try/catch`. diff --git a/eslint-factory/eslint.config.cjs b/eslint-factory/eslint.config.cjs index 13d5de678bf..1a0acbd9d3b 100644 --- a/eslint-factory/eslint.config.cjs +++ b/eslint-factory/eslint.config.cjs @@ -34,6 +34,7 @@ module.exports = [ "gh-aw-custom/prefer-core-logging": "warn", "gh-aw-custom/no-core-error-then-process-exit": "warn", "gh-aw-custom/no-core-error-then-process-exitcode": "warn", + "gh-aw-custom/no-child-process-interpolated-command": "warn", "gh-aw-custom/no-exec-interpolated-command": "warn", "gh-aw-custom/require-execsync-try-catch": "warn", "gh-aw-custom/require-execfilesync-try-catch": "warn", diff --git a/eslint-factory/src/index.ts b/eslint-factory/src/index.ts index 8fb6c35ac74..f788b205aa8 100644 --- a/eslint-factory/src/index.ts +++ b/eslint-factory/src/index.ts @@ -20,6 +20,7 @@ import { requireNewUrlTryCatchRule } from "./rules/require-new-url-try-catch"; import { preferCoreLoggingRule } from "./rules/prefer-core-logging"; import { noCoreErrorThenProcessExitRule } from "./rules/no-core-error-then-process-exit"; import { noCoreErrorThenProcessExitCodeRule } from "./rules/no-core-error-then-process-exitcode"; +import { noChildProcessInterpolatedCommandRule } from "./rules/no-child-process-interpolated-command"; import { noExecInterpolatedCommandRule } from "./rules/no-exec-interpolated-command"; import { requireExecSyncTryCatchRule } from "./rules/require-execsync-try-catch"; import { requireExecFileSyncTryCatchRule } from "./rules/require-execfilesync-try-catch"; @@ -55,6 +56,7 @@ const plugin = { "prefer-core-logging": preferCoreLoggingRule, "no-core-error-then-process-exit": noCoreErrorThenProcessExitRule, "no-core-error-then-process-exitcode": noCoreErrorThenProcessExitCodeRule, + "no-child-process-interpolated-command": noChildProcessInterpolatedCommandRule, "no-exec-interpolated-command": noExecInterpolatedCommandRule, "require-execsync-try-catch": requireExecSyncTryCatchRule, "require-execfilesync-try-catch": requireExecFileSyncTryCatchRule, diff --git a/eslint-factory/src/rules/no-child-process-interpolated-command.test.ts b/eslint-factory/src/rules/no-child-process-interpolated-command.test.ts new file mode 100644 index 00000000000..6fc44573b0d --- /dev/null +++ b/eslint-factory/src/rules/no-child-process-interpolated-command.test.ts @@ -0,0 +1,79 @@ +import { RuleTester } from "eslint"; +import { describe, it } from "vitest"; +import { noChildProcessInterpolatedCommandRule } from "./no-child-process-interpolated-command"; + +const ruleTester = new RuleTester({ + languageOptions: { + ecmaVersion: "latest", + sourceType: "commonjs", + }, +}); + +describe("no-child-process-interpolated-command", () => { + it("flags dynamic child_process command strings for shell-evaluated methods", () => { + ruleTester.run("no-child-process-interpolated-command", noChildProcessInterpolatedCommandRule, { + valid: [ + { code: `const { execSync } = require("child_process"); execSync("git status");` }, + { code: `const cp = require("child_process"); cp.execSync(\`git status\`);` }, + { code: `const cp = require("child_process"); cp.spawn("git", ["status"]);` }, + { code: `const { spawn } = require("child_process"); spawn(\`git \${branch}\`, ["status"]);` }, + { code: `const { execFileSync } = require("child_process"); execFileSync("git", ["status"], { shell: false });` }, + { code: `exec.exec(\`git checkout \${branch}\`, []);` }, + { + code: `import { exec } from "node:child_process"; exec("git status");`, + languageOptions: { sourceType: "module" }, + }, + ], + invalid: [ + { + code: `const { execSync } = require("child_process"); execSync(\`git checkout \${branch}\`);`, + errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "execSync" } }], + }, + { + code: `const cp = require("child_process"); const run = cp.execSync; run("git checkout " + branch);`, + errors: [{ messageId: "interpolatedCommand", data: { kind: "dynamic string concatenation", method: "execSync" } }], + }, + { + code: `const cp = require("node:child_process"); cp.exec(\`git checkout \${branch}\`);`, + errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "exec" } }], + }, + { + code: `const { spawn } = require("child_process"); spawn(\`git checkout \${branch}\`, { shell: true });`, + errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "spawn" } }], + }, + { + code: `const { spawn } = require("child_process"); const opts = { shell: true }; spawn(\`git checkout \${branch}\`, opts);`, + errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "spawn" } }], + }, + { + code: `const { spawn } = require("child_process"); spawn(\`git checkout \${branch}\`, { shell: "/bin/bash" });`, + errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "spawn" } }], + }, + { + code: `const { spawn } = require("child_process"); const opts = [{ shell: true }]; spawn("git checkout " + branch, ...opts);`, + errors: [{ messageId: "interpolatedCommand", data: { kind: "dynamic string concatenation", method: "spawn" } }], + }, + { + code: `const { spawnSync } = require("child_process"); spawnSync("git checkout " + branch, ["--"], { shell: true });`, + errors: [{ messageId: "interpolatedCommand", data: { kind: "dynamic string concatenation", method: "spawnSync" } }], + }, + { + code: `const { execFileSync } = require("child_process"); execFileSync(\`git \${branch}\`, ["status"], { shell: true });`, + errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "execFileSync" } }], + }, + { + code: `const { execFile } = require("child_process"); execFile("git " + branch, { shell: true });`, + errors: [{ messageId: "interpolatedCommand", data: { kind: "dynamic string concatenation", method: "execFile" } }], + }, + { + code: `import { execSync, "exec" as run } from "child_process"; execSync(\`git checkout \${branch}\`); run("git checkout " + branch);`, + languageOptions: { sourceType: "module" }, + errors: [ + { messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "execSync" } }, + { messageId: "interpolatedCommand", data: { kind: "dynamic string concatenation", method: "exec" } }, + ], + }, + ], + }); + }); +}); diff --git a/eslint-factory/src/rules/no-child-process-interpolated-command.ts b/eslint-factory/src/rules/no-child-process-interpolated-command.ts new file mode 100644 index 00000000000..2f3f4fd98a6 --- /dev/null +++ b/eslint-factory/src/rules/no-child-process-interpolated-command.ts @@ -0,0 +1,182 @@ +import { AST_NODE_TYPES, ESLintUtils, TSESLint, TSESTree } from "@typescript-eslint/utils"; +import { isChildProcessImportBinding, isChildProcessObjectBinding, isRequireChildProcess } from "./try-catch-rule-utils"; + +const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`); + +type SourceCodeScope = ReturnType; +type ChildProcessMethod = "exec" | "execSync" | "spawn" | "spawnSync" | "execFile" | "execFileSync"; +const SHELL_CONDITIONAL_METHODS = new Set(["spawn", "spawnSync", "execFile", "execFileSync"]); + +function isStaticExpression(node: TSESTree.Expression): boolean { + if (node.type === AST_NODE_TYPES.Literal) return true; + if (node.type === AST_NODE_TYPES.TemplateLiteral) return node.expressions.length === 0; + if (node.type === AST_NODE_TYPES.BinaryExpression && node.operator === "+") { + return isStaticExpression(node.left) && isStaticExpression(node.right); + } + return false; +} + +function isDynamicStringConcatenation(node: TSESTree.Expression): boolean { + return node.type === AST_NODE_TYPES.BinaryExpression && node.operator === "+" && !isStaticExpression(node); +} + +function getDynamicCommandKind(node: TSESTree.Expression): string | null { + if (node.type === AST_NODE_TYPES.TemplateLiteral && node.expressions.length > 0) return "interpolated template literal"; + if (isDynamicStringConcatenation(node)) return "dynamic string concatenation"; + return null; +} + +function getImportSpecifierName(node: TSESTree.ImportSpecifier): string | null { + if (node.imported.type === AST_NODE_TYPES.Identifier) return node.imported.name; + if (node.imported.type === AST_NODE_TYPES.Literal && typeof node.imported.value === "string") return node.imported.value; + return null; +} + +function getShellPropertyValue(optionsArg: TSESTree.ObjectExpression): boolean { + for (const prop of optionsArg.properties) { + if (prop.type !== AST_NODE_TYPES.Property || prop.computed) continue; + + 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 || typeof prop.value.value === "string"); + } + + return false; +} + +function resolveObjectExpression(arg: TSESTree.CallExpressionArgument, scopeNode: TSESTree.Node, sourceCode: TSESLint.SourceCode): TSESTree.ObjectExpression | null { + if (arg.type === AST_NODE_TYPES.ObjectExpression) return arg; + if (arg.type !== AST_NODE_TYPES.Identifier) return null; + + let scope: SourceCodeScope | null = sourceCode.getScope(scopeNode); + while (scope) { + const variable = scope.set.get(arg.name); + if (variable && variable.defs.length > 0) { + for (const def of variable.defs) { + if (def.type !== "Variable") continue; + const declarator = def.node as TSESTree.VariableDeclarator; + if (declarator.id.type !== AST_NODE_TYPES.Identifier || declarator.id.name !== arg.name) continue; + if (declarator.init?.type === AST_NODE_TYPES.ObjectExpression) return declarator.init; + } + return null; + } + scope = scope.upper; + } + + return null; +} + +function isShellTrueOption(optionsArg: TSESTree.CallExpressionArgument | undefined, scopeNode: TSESTree.Node, sourceCode: TSESLint.SourceCode): boolean { + if (!optionsArg) return false; + if (optionsArg.type === AST_NODE_TYPES.SpreadElement) return true; // Conservative: spread arguments are treated as possibly shell-enabled. + + const resolvedObject = resolveObjectExpression(optionsArg, scopeNode, sourceCode); + if (!resolvedObject) return false; + return getShellPropertyValue(resolvedObject); +} + +function requiresShellTrue(method: ChildProcessMethod): boolean { + return SHELL_CONDITIONAL_METHODS.has(method); +} + +function hasShellTrueOptions(node: TSESTree.CallExpression, method: ChildProcessMethod, sourceCode: TSESLint.SourceCode): boolean { + if (!requiresShellTrue(method)) return true; + return isShellTrueOption(node.arguments[1], node, sourceCode) || isShellTrueOption(node.arguments[2], node, sourceCode); +} + +function isChildProcessMethodBinding(method: ChildProcessMethod, identifierName: string, scopeNode: TSESTree.Node, sourceCode: TSESLint.SourceCode): boolean { + let scope: SourceCodeScope | null = sourceCode.getScope(scopeNode); + while (scope) { + const variable = scope.set.get(identifierName); + if (variable && variable.defs.length > 0) { + for (const def of variable.defs) { + if (isChildProcessImportBinding(def) && def.node.type === AST_NODE_TYPES.ImportSpecifier) { + const importedName = getImportSpecifierName(def.node); + if (importedName === method) return true; + } + + if (def.type !== "Variable") continue; + + const declarator = def.node as TSESTree.VariableDeclarator; + if (declarator.id.type === AST_NODE_TYPES.ObjectPattern && isRequireChildProcess(declarator.init)) { + for (const prop of declarator.id.properties) { + if (prop.type !== AST_NODE_TYPES.Property || prop.key.type !== AST_NODE_TYPES.Identifier || prop.key.name !== method) continue; + const boundName = prop.value.type === AST_NODE_TYPES.Identifier ? prop.value.name : null; + if (boundName === identifierName) return true; + } + } + + if (declarator.id.type === AST_NODE_TYPES.Identifier && declarator.init?.type === AST_NODE_TYPES.MemberExpression) { + const init = declarator.init; + if (!init.computed && init.object.type === AST_NODE_TYPES.Identifier && isChildProcessObjectBinding(init.object.name, init.object, sourceCode) && init.property.type === AST_NODE_TYPES.Identifier && init.property.name === method) { + return true; + } + } + } + return false; + } + scope = scope.upper; + } + return false; +} + +function resolveChildProcessMethod(node: TSESTree.CallExpression, sourceCode: TSESLint.SourceCode): ChildProcessMethod | null { + const callee = node.callee; + if (callee.type === AST_NODE_TYPES.Identifier) { + const methods: ChildProcessMethod[] = ["exec", "execSync", "spawn", "spawnSync", "execFile", "execFileSync"]; + for (const method of methods) { + if (isChildProcessMethodBinding(method, callee.name, callee, sourceCode)) return method; + } + return null; + } + + 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 (!isChildProcessObjectBinding(callee.object.name, callee.object, sourceCode)) return null; + + const method = callee.property.name; + return method === "exec" || method === "execSync" || method === "spawn" || method === "spawnSync" || method === "execFile" || method === "execFileSync" ? method : null; +} + +export const noChildProcessInterpolatedCommandRule = createRule({ + name: "no-child-process-interpolated-command", + meta: { + type: "problem", + docs: { + description: + "Disallow interpolated template literals or dynamic string concatenation as child_process command arguments for shell-evaluated execution paths. " + + "Dynamic command strings can become shell-injection vectors. Prefer static command names with argument arrays and avoid shell: true.", + }, + schema: [], + messages: { + interpolatedCommand: + "Avoid passing a {{kind}} as the command to child_process.{{method}} — shell-evaluated command strings can enable shell injection. " + + "Prefer a static executable plus an argument array (for example, execFileSync(cmd, [arg1, arg2])) and avoid shell: true when possible.", + }, + }, + defaultOptions: [], + create(context) { + const sourceCode = context.sourceCode; + + return { + CallExpression(node) { + const method = resolveChildProcessMethod(node, sourceCode); + if (!method) return; + if (!hasShellTrueOptions(node, method, sourceCode)) return; + + const firstArg = node.arguments[0]; + if (!firstArg || firstArg.type === AST_NODE_TYPES.SpreadElement) return; + + const kind = getDynamicCommandKind(firstArg); + if (!kind) return; + + context.report({ + node: firstArg, + messageId: "interpolatedCommand", + data: { kind, method }, + }); + }, + }; + }, +});