Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
39 changes: 38 additions & 1 deletion eslint-factory/src/rules/no-exec-interpolated-command.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ describe("no-exec-interpolated-command", () => {
{ code: "exec.exec(`git`, [`checkout`, branch]);" },
// getExecOutput with static command
{ code: `exec.getExecOutput("git", ["rev-parse", "--abbrev-ref", "HEAD"], opts);` },
// Command variable (identifier) — not a string literal, out of scope
// Command variable (identifier) — no definition in scope, cannot resolve, not flagged
{ code: `exec.exec(myCommand, [arg1]);` },
// Single-word static template literal — no interpolation
{ code: "exec.exec(`git`, [branch]);" },
Expand All @@ -35,6 +35,18 @@ describe("no-exec-interpolated-command", () => {
{ code: `exec(\`git checkout \${branch}\`);` },
// Spread first argument is intentionally out of scope
{ code: `exec.exec(...args);` },
// Variable holds a static string — safe, must not be flagged
{ code: `const cmd = "git"; exec.exec(cmd, [branch]);` },
// Variable holds a static template literal — safe
{ code: "const cmd = `git`; exec.exec(cmd, [branch]);" },
// Reassigned variable — skipped to avoid false positives
{ code: `let cmd = "git"; cmd = "other"; exec.exec(cmd, [branch]);` },
// Dynamic initializer with reassignment — must still be skipped
{ code: "let cmd = `git checkout ${branch}`; cmd = 'git'; exec.exec(cmd, []);" },
// Parameter as command — skipped (def.type === "Parameter")
{ 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, []); } }" },
],
invalid: [
// Template literal with interpolation as command
Expand Down Expand Up @@ -67,6 +79,31 @@ describe("no-exec-interpolated-command", () => {
code: "exec.exec(`git am --3way ${patchPath}`, [], opts);",
errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "exec" } }],
},
// Variable holds an interpolated template literal — must be flagged (indirection)
{
code: "const cmd = `git checkout ${branch}`; exec.exec(cmd, []);",
errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "exec" } }],
},
// Same-function indirection should still be flagged with function-boundary resolution
{
code: "function run(branch) { const cmd = `git checkout ${branch}`; exec.exec(cmd, []); }",
errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "exec" } }],
},
// Nested block in same function should still resolve and be flagged
{
code: "function run(branch) { const cmd = `git checkout ${branch}`; if (ok) { exec.exec(cmd, []); } }",
errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "exec" } }],
},
// Variable holds dynamic string concatenation — must be flagged (indirection)
{
code: `const cmd = "git checkout " + branchName; exec.exec(cmd, []);`,
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, []); }",
errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "exec" } }],
},
],

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] Missing boundary test: two-level indirection (e.g. const a = \git ${x}`; const b = a; exec.exec(b, [])`) is intentionally out of scope, but there is no test documenting this limit.

💡 Suggested valid test
// Two-level indirection — single-level resolution only, not flagged
{ code: "const a = `git ${x}`; const b = a; exec.exec(b, []);", },

Without a test, the intended boundary is invisible to reviewers of future follow-up PRs.

@copilot please address this.

});
});
Expand Down
50 changes: 48 additions & 2 deletions eslint-factory/src/rules/no-exec-interpolated-command.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { AST_NODE_TYPES, ESLintUtils, TSESTree } from "@typescript-eslint/utils";
import { AST_NODE_TYPES, ESLintUtils, TSESLint, TSESTree } from "@typescript-eslint/utils";

const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`);
type ExecMethodName = "exec" | "getExecOutput";
Expand Down Expand Up @@ -75,6 +75,43 @@ export const noExecInterpolatedCommandRule = createRule({
},
defaultOptions: [],
create(context) {
const sourceCode = context.sourceCode;

/**
* When `identifier` is a write-once local variable binding, returns its
* initializer expression so the caller can apply further checks. Returns
* null for parameters, imports, multiply-assigned vars, and vars with no

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.

Scope walk climbs past function boundaries — module-level variable initializers are not statically safe.

💡 Details and suggested fix

The while (scope !== null) loop walks up through function and module scopes. A module-level let or var can be mutated at runtime, so its initializer is not a reliable proxy for its value at call time.

The existing isWrite() && !ref.init guard only detects reassignments visible in the same scope analysis graph; external mutation (e.g., from an async callback) is invisible to this analysis.

Fix: stop climbing at the enclosing function boundary so only local block-scoped bindings are resolved.

// stop at function/module boundaries
if (scope.type === 'function' || scope.type === 'module') {
  break;
}
scope = scope.upper;

* initializer.
*/
function resolveInitializer(identifier: TSESTree.Identifier): TSESTree.Expression | null {
const startScope = sourceCode.getScope(identifier);
const functionScope = startScope.variableScope;
// Only resolve within a concrete function boundary (function declaration,
// function expression, or arrow function). Module/global scopes are
// intentionally skipped because those bindings are not a stable proxy for
// runtime values at call time.
if (functionScope.type !== "function") return null;

let scope: TSESLint.Scope.Scope | null = startScope;
// Stay inside the same function's nested block scopes; do not cross to
// enclosing function/module scopes.
while (scope !== null && scope.variableScope === functionScope) {
const variable = scope.set.get(identifier.name);
if (variable !== undefined) {
// Only accept simple, single-definition Variable bindings.
if (variable.defs.length !== 1) return null;
const def = variable.defs[0];
if (def.type !== "Variable") return null;
// Reject re-assigned bindings (write references that are not the initializer).
if (variable.references.some(ref => ref.isWrite() && !ref.init)) return null;
const declarator = def.node as TSESTree.VariableDeclarator;
return declarator.init ?? null;
}
scope = scope.upper;
}
return null;
}

return {
CallExpression(node) {
const method = resolveExecMethod(node);
Expand All @@ -83,7 +120,16 @@ export const noExecInterpolatedCommandRule = createRule({
const firstArg = node.arguments[0];
if (!firstArg || firstArg.type === AST_NODE_TYPES.SpreadElement) return;

const kind = getDynamicCommandKind(firstArg);
let candidate: TSESTree.Expression = firstArg as TSESTree.Expression;
const seen = new Set<TSESTree.Identifier>();
while (candidate.type === AST_NODE_TYPES.Identifier && !seen.has(candidate)) {
seen.add(candidate);
const resolved = resolveInitializer(candidate);
if (!resolved) break;
candidate = resolved;
}

const kind = getDynamicCommandKind(candidate);
if (!kind) return;

context.report({
Expand Down