diff --git a/eslint-factory/README.md b/eslint-factory/README.md index 26a2c5f9a04..d404861e485 100644 --- a/eslint-factory/README.md +++ b/eslint-factory/README.md @@ -600,9 +600,11 @@ Why: command strings evaluated by a shell (`exec`, `execSync`, `spawn` / `spawnS - `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"`). +- `` execSync(`git checkout ${branch}`.trim()) `` — chained string-normalizing methods (`trim`, `trimStart`, `trimEnd`, `toLowerCase`, `toUpperCase`, `toLocaleLowerCase`, `toLocaleUpperCase`, `replace`, `replaceAll`, `normalize`) are unwrapped before the check. +- `` execSync("git checkout PLACEHOLDER".replace("PLACEHOLDER", () => branch)) `` — a `.replace()` / `.replaceAll()` replacer callback's return value is also inspected. **Not flagged:** -- Fully static command strings (`"git status"`, `` `git status` ``, and fully static `+` concatenations). +- Fully static command strings (`"git status"`, `` `git status` ``, and fully static `+` concatenations), including when a string-normalizing method is chained onto them. - `spawn(cmd, [args])` / `spawnSync(cmd, [args])` without `shell: true`. - `execFile` / `execFileSync` without `shell: true`. @@ -718,9 +720,11 @@ Disallow passing an interpolated template literal or dynamic string concatenatio **Detected forms:** - `` exec.exec(`git checkout ${branchName}`) `` — interpolated template literal. - `exec.exec("git " + branchName)` — dynamic string concatenation. +- `` exec.exec(`git checkout ${branchName}`.trim()) `` — chained string-normalizing methods (`trim`, `trimStart`, `trimEnd`, `toLowerCase`, `toUpperCase`, `toLocaleLowerCase`, `toLocaleUpperCase`, `replace`, `replaceAll`, `normalize`) are unwrapped before the check. +- `` exec.exec("git checkout PLACEHOLDER".replace("PLACEHOLDER", () => branchName)) `` — a `.replace()` / `.replaceAll()` replacer callback's return value is also inspected. **Not flagged:** -- Static command strings, including string concatenation of only static expressions. +- Static command strings, including string concatenation of only static expressions and chained string-normalizing methods on them. - Arguments passed correctly via the `args` array. ### `no-setfailed-then-exit-zero` diff --git a/eslint-factory/src/rules/command-initializer-utils.ts b/eslint-factory/src/rules/command-initializer-utils.ts index eaf2d5418b4..ba123d1d9c1 100644 --- a/eslint-factory/src/rules/command-initializer-utils.ts +++ b/eslint-factory/src/rules/command-initializer-utils.ts @@ -46,3 +46,152 @@ export function resolveWriteOnceInitializerChain(expression: TSESTree.Expression } return candidate; } + +/** + * String methods that return a normalized copy of their receiver. Chaining one + * of these after a command string (for example `` `git checkout ${branch}`.trim() ``) + * keeps the interpolated value in the resulting command, so the receiver must be + * inspected instead of the outer call expression. + */ +const STRING_TRANSFORM_METHODS = new Set(["trim", "trimStart", "trimEnd", "toLowerCase", "toUpperCase", "toLocaleLowerCase", "toLocaleUpperCase", "replace", "replaceAll", "normalize"]); + +/** + * When the node is a call to a string-normalizing method (for example + * `.trim()` or `.toLowerCase()`), returns the receiver expression and the call + * arguments so callers can inspect the underlying command string. Returns null + * otherwise. + */ +function getStringTransformCall(node: TSESTree.Expression): { receiver: TSESTree.Expression; args: TSESTree.CallExpressionArgument[] } | null { + if (node.type !== AST_NODE_TYPES.CallExpression) return null; + const callee = node.callee; + if (callee.type !== AST_NODE_TYPES.MemberExpression || callee.computed) return null; + if (callee.property.type !== AST_NODE_TYPES.Identifier || !STRING_TRANSFORM_METHODS.has(callee.property.name)) return null; + return { receiver: callee.object, args: node.arguments }; +} + +/** + * Returns true when the node is a purely static expression (no runtime + * interpolation): a literal, a no-expression template literal, a binary `+` of + * two static expressions, or a string-normalizing method call whose receiver + * and arguments are all static (for example `.replace()` can inject dynamic + * content through its replacement argument). + */ +export 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); + } + const transform = getStringTransformCall(node); + if (transform) { + if (!isStaticExpression(transform.receiver)) return false; + return transform.args.every(arg => arg.type !== AST_NODE_TYPES.SpreadElement && isStaticExpression(arg)); + } + return false; +} + +/** + * Returns true when the node is a dynamic string concatenation (binary `+` + * that is not entirely static). + */ +export function isDynamicStringConcatenation(node: TSESTree.Expression): boolean { + return node.type === AST_NODE_TYPES.BinaryExpression && node.operator === "+" && !isStaticExpression(node); +} + +/** + * Collects the argument expressions of every `return` statement reachable + * from `node` without crossing into a nested function boundary. Used to + * inspect what a `.replace()` / `.replaceAll()` replacer callback can inject + * into the resulting command string. + */ +function collectReturnArguments(node: TSESTree.Statement, results: TSESTree.Expression[]): void { + switch (node.type) { + case AST_NODE_TYPES.ReturnStatement: + if (node.argument) results.push(node.argument); + return; + case AST_NODE_TYPES.BlockStatement: + for (const stmt of node.body) collectReturnArguments(stmt, results); + return; + case AST_NODE_TYPES.IfStatement: + collectReturnArguments(node.consequent, results); + if (node.alternate) collectReturnArguments(node.alternate, results); + return; + case AST_NODE_TYPES.ForStatement: + case AST_NODE_TYPES.ForInStatement: + case AST_NODE_TYPES.ForOfStatement: + case AST_NODE_TYPES.WhileStatement: + case AST_NODE_TYPES.DoWhileStatement: + collectReturnArguments(node.body, results); + return; + case AST_NODE_TYPES.TryStatement: + collectReturnArguments(node.block, results); + if (node.handler) collectReturnArguments(node.handler.body, results); + if (node.finalizer) collectReturnArguments(node.finalizer, results); + return; + case AST_NODE_TYPES.SwitchStatement: + for (const switchCase of node.cases) { + for (const stmt of switchCase.consequent) collectReturnArguments(stmt, results); + } + return; + case AST_NODE_TYPES.LabeledStatement: + collectReturnArguments(node.body, results); + return; + default: + // Do not descend into nested function/arrow expressions or other + // statement kinds that cannot contain a same-scope `return`. + return; + } +} + +/** + * When `node` is a function or arrow expression (for example a `.replace()` / + * `.replaceAll()` replacer callback), returns the dynamic kind of any value it + * can return: the expression body of an arrow function, or the argument of + * any `return` statement reachable without crossing a nested function + * boundary. Returns null when `node` is not a function/arrow expression or + * none of its return values are dynamic. + */ +function getDynamicKindFromCallbackReturn(node: TSESTree.Node, sourceCode: TSESLint.SourceCode, seen: Set): string | null { + if (node.type !== AST_NODE_TYPES.ArrowFunctionExpression && node.type !== AST_NODE_TYPES.FunctionExpression) return null; + if (node.body.type !== AST_NODE_TYPES.BlockStatement) return getDynamicCommandKind(node.body, sourceCode, seen); + + const returnArguments: TSESTree.Expression[] = []; + collectReturnArguments(node.body, returnArguments); + for (const argument of returnArguments) { + const kind = getDynamicCommandKind(argument, sourceCode, seen); + if (kind) return kind; + } + return null; +} + +/** + * Returns the display kind string for the problematic command expression, or + * null when the expression is not one of the flagged shapes. + * + * Write-once local bindings and chained string-normalizing calls (for example + * `` `git checkout ${branch}`.trim() ``) are unwrapped before the check so the + * underlying command string is inspected. For calls that accept arguments (for + * example `.replace(pattern, value)`), the arguments are inspected as well, + * including a replacer callback's return value (for example + * `.replace(pattern, () => branch)`). + */ +export function getDynamicCommandKind(expression: TSESTree.Expression, sourceCode: TSESLint.SourceCode, seen: Set = new Set()): string | null { + const candidate = resolveWriteOnceInitializerChain(expression, sourceCode); + if (seen.has(candidate)) return null; + seen.add(candidate); + + if (candidate.type === AST_NODE_TYPES.TemplateLiteral && candidate.expressions.length > 0) return "interpolated template literal"; + if (isDynamicStringConcatenation(candidate)) return "dynamic string concatenation"; + + const transform = getStringTransformCall(candidate); + if (!transform) return null; + + const receiverKind = getDynamicCommandKind(transform.receiver, sourceCode, seen); + if (receiverKind) return receiverKind; + for (const arg of transform.args) { + if (arg.type === AST_NODE_TYPES.SpreadElement) continue; + const argKind = getDynamicCommandKind(arg, sourceCode, seen) ?? getDynamicKindFromCallbackReturn(arg, sourceCode, seen); + if (argKind) return argKind; + } + return null; +} 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 index 74d9f682a7d..e5c49c40877 100644 --- a/eslint-factory/src/rules/no-child-process-interpolated-command.test.ts +++ b/eslint-factory/src/rules/no-child-process-interpolated-command.test.ts @@ -33,12 +33,43 @@ describe("no-child-process-interpolated-command", () => { code: `import { execSync } from "child_process"; import { cmd } from "./cmd"; execSync(cmd);`, languageOptions: { sourceType: "module" }, }, + // Static command with a chained string method — still static, safe + { code: `const { execSync } = require("child_process"); execSync("git status".trim());` }, + // Fully static concatenation with a chained string method — safe + { code: `const { execSync } = require("child_process"); execSync(("git" + " status").toLowerCase());` }, + // Static replacer callback with a fully static return value — safe + { code: `const { execSync } = require("child_process"); execSync("git-status".replace("-", () => " "));` }, ], invalid: [ { code: `const { execSync } = require("child_process"); execSync(\`git checkout \${branch}\`);`, errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "execSync" } }], }, + // Chained .trim() must not defeat the check + { + code: `const { execSync } = require("child_process"); execSync(\`git checkout \${branch}\`.trim());`, + errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "execSync" } }], + }, + // Chained .toLowerCase() must not defeat the check + { + code: `const { execSync } = require("child_process"); execSync(\`git log --author=\${author}\`.toLowerCase());`, + errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "execSync" } }], + }, + // Chained string method on a dynamic concatenation is also flagged + { + code: `const { execSync } = require("child_process"); execSync(("git checkout " + branch).trim());`, + errors: [{ messageId: "interpolatedCommand", data: { kind: "dynamic string concatenation", method: "execSync" } }], + }, + // Replacer callback returning a dynamic template literal is also flagged + { + code: `const { execSync } = require("child_process"); execSync("git checkout PLACEHOLDER".replace("PLACEHOLDER", () => \`\${branch}-x\`));`, + errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "execSync" } }], + }, + // Replacer callback with a block body / return statement is also flagged + { + code: `const { execSync } = require("child_process"); execSync("git checkout PLACEHOLDER".replaceAll("PLACEHOLDER", function() { return "x-" + branch; }));`, + errors: [{ messageId: "interpolatedCommand", data: { kind: "dynamic string concatenation", method: "execSync" } }], + }, { code: `function run(x) { const { execSync } = require("child_process"); const cmd = \`git log --author=\${x}\`; execSync(cmd); }`, errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "execSync" } }], diff --git a/eslint-factory/src/rules/no-child-process-interpolated-command.ts b/eslint-factory/src/rules/no-child-process-interpolated-command.ts index 68106cc1cd8..b41d91c1ec8 100644 --- a/eslint-factory/src/rules/no-child-process-interpolated-command.ts +++ b/eslint-factory/src/rules/no-child-process-interpolated-command.ts @@ -1,5 +1,5 @@ import { AST_NODE_TYPES, ESLintUtils, TSESLint, TSESTree } from "@typescript-eslint/utils"; -import { resolveWriteOnceInitializerChain } from "./command-initializer-utils"; +import { getDynamicCommandKind } from "./command-initializer-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}`); @@ -8,25 +8,6 @@ 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; @@ -176,8 +157,7 @@ export const noChildProcessInterpolatedCommandRule = createRule({ const firstArg = node.arguments[0]; if (!firstArg || firstArg.type === AST_NODE_TYPES.SpreadElement) return; - const candidate = resolveWriteOnceInitializerChain(firstArg as TSESTree.Expression, sourceCode); - const kind = getDynamicCommandKind(candidate); + const kind = getDynamicCommandKind(firstArg as TSESTree.Expression, sourceCode); if (!kind) return; context.report({ diff --git a/eslint-factory/src/rules/no-exec-interpolated-command.test.ts b/eslint-factory/src/rules/no-exec-interpolated-command.test.ts index 944eede606e..3b1c45bd1a8 100644 --- a/eslint-factory/src/rules/no-exec-interpolated-command.test.ts +++ b/eslint-factory/src/rules/no-exec-interpolated-command.test.ts @@ -47,6 +47,14 @@ describe("no-exec-interpolated-command", () => { { code: `(function(cmd) { exec.exec(cmd, []); })("git");` }, // Cross-function binding is intentionally out of scope { code: "function outer(branch) { const cmd = `git checkout ${branch}`; function inner() { exec.exec(cmd, []); } }" }, + // Static template literal with a chained string method — still static, safe + { code: "exec.exec(`git`.trim(), [branch]);" }, + // Fully static concatenation with a chained string method — safe + { code: `exec.exec(("git" + " checkout").toLowerCase(), [branch]);` }, + // Static replace with static arguments — safe + { code: `exec.exec("git-checkout".replace("-", " "), [branch]);` }, + // Static replace with a fully static replacer callback — safe + { code: `exec.exec("git-checkout".replace("-", () => " "), [branch]);` }, ], invalid: [ // Template literal with interpolation as command @@ -99,6 +107,41 @@ describe("no-exec-interpolated-command", () => { code: `const cmd = "git checkout " + branchName; exec.exec(cmd, []);`, errors: [{ messageId: "interpolatedCommand", data: { kind: "dynamic string concatenation", method: "exec" } }], }, + // Chained .trim() must not defeat the check + { + code: "exec.exec(`git checkout ${branch}`.trim(), []);", + errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "exec" } }], + }, + // Chained .toLowerCase() must not defeat the check + { + code: "exec.exec(`git log --author=${author}`.toLowerCase(), []);", + errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "exec" } }], + }, + // Chained string methods on a dynamic concatenation are also flagged + { + code: `exec.exec(("git checkout " + branchName).trim(), []);`, + errors: [{ messageId: "interpolatedCommand", data: { kind: "dynamic string concatenation", method: "exec" } }], + }, + // Chained string method on a variable holding a dynamic command + { + code: "function run(branch) { const cmd = `git checkout ${branch}`; exec.exec(cmd.trim(), []); }", + errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "exec" } }], + }, + // Dynamic replacement argument on a static receiver is also flagged + { + code: 'exec.exec("git checkout PLACEHOLDER".replace("PLACEHOLDER", `${branch}-x`), []);', + errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "exec" } }], + }, + // Replacer callback returning a dynamic template literal is also flagged + { + code: 'exec.exec("git checkout PLACEHOLDER".replace("PLACEHOLDER", () => `${branch}-x`), []);', + errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "exec" } }], + }, + // Replacer callback with a block body / return statement is also flagged + { + code: 'exec.exec("git checkout PLACEHOLDER".replaceAll("PLACEHOLDER", function() { return "x-" + branch; }), []);', + errors: [{ messageId: "interpolatedCommand", data: { kind: "dynamic string concatenation", method: "exec" } }], + }, // Chained aliases are also flagged when they resolve to a dynamic command { code: "function run(branch) { const dynamic = `git checkout ${branch}`; const cmd = dynamic; exec.exec(cmd, []); }", diff --git a/eslint-factory/src/rules/no-exec-interpolated-command.ts b/eslint-factory/src/rules/no-exec-interpolated-command.ts index 778790550eb..c00d33cb21d 100644 --- a/eslint-factory/src/rules/no-exec-interpolated-command.ts +++ b/eslint-factory/src/rules/no-exec-interpolated-command.ts @@ -1,41 +1,9 @@ import { AST_NODE_TYPES, ESLintUtils, TSESTree } from "@typescript-eslint/utils"; -import { resolveWriteOnceInitializerChain } from "./command-initializer-utils"; +import { getDynamicCommandKind } from "./command-initializer-utils"; const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`); type ExecMethodName = "exec" | "getExecOutput"; -/** - * Returns true when the node is a purely static expression (no runtime - * interpolation): a literal, a no-expression template literal, or a binary - * `+` of two static expressions. - */ -function isStaticExpression(node: TSESTree.Expression): boolean { - if (node.type === "Literal") return true; - if (node.type === "TemplateLiteral") return node.expressions.length === 0; - if (node.type === "BinaryExpression" && node.operator === "+") { - return isStaticExpression(node.left) && isStaticExpression(node.right); - } - return false; -} - -/** - * Returns true when the node is a dynamic string concatenation (binary `+` - * that is not entirely static). - */ -function isDynamicStringConcatenation(node: TSESTree.Expression): boolean { - return node.type === "BinaryExpression" && node.operator === "+" && !isStaticExpression(node); -} - -/** - * Returns the display kind string for the problematic first argument, or null - * when the argument is not one of the flagged shapes. - */ -function getDynamicCommandKind(node: TSESTree.Expression): string | null { - if (node.type === "TemplateLiteral" && node.expressions.length > 0) return "interpolated template literal"; - if (isDynamicStringConcatenation(node)) return "dynamic string concatenation"; - return null; -} - /** * Returns true when the call expression looks like `exec.exec(...)` or * `exec.getExecOutput(...)` — the `exec` global injected by github-script. @@ -86,8 +54,7 @@ export const noExecInterpolatedCommandRule = createRule({ const firstArg = node.arguments[0]; if (!firstArg || firstArg.type === AST_NODE_TYPES.SpreadElement) return; - const candidate = resolveWriteOnceInitializerChain(firstArg as TSESTree.Expression, sourceCode); - const kind = getDynamicCommandKind(candidate); + const kind = getDynamicCommandKind(firstArg as TSESTree.Expression, sourceCode); if (!kind) return; context.report({