fix(eslint-factory): resolve single-level variable indirection in no-exec-interpolated-command#47544
Conversation
…exec-interpolated-command Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Adds single-level variable resolution to detect dynamically constructed commands passed to exec.exec().
Changes:
- Resolves write-once variable initializers before command analysis.
- Adds valid and invalid variable-indirection tests.
Show a summary per file
| File | Description |
|---|---|
no-exec-interpolated-command.ts |
Adds initializer resolution. |
no-exec-interpolated-command.test.ts |
Tests variable-based commands. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Medium
| const declarator = def.node as TSESTree.VariableDeclarator; | ||
| return declarator.init ?? null; | ||
| } | ||
| scope = scope.upper; |
| // 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]);` }, |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47544 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The implementation looks correct and well-tested.
resolveInitializerproperly walks the scope chain and returnsnullfor parameters, imports, multiply-assigned vars, and unresolvable identifiers- The fallback
resolveInitializer(firstArg) ?? firstArgpreserves existing detection behavior for direct calls ref.isWrite() && !ref.initcorrectly distinguishes reassignments from initializers- Test cases cover all new branches (static-string variables, reassigned vars, parameters, interpolated via variable, concatenation via variable)
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 18.2 AIC · ⌖ 7.93 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (6 test cases)
Verdict✅ Approved. All 6 new test cases are focused behavioral contracts that enforce the vulnerability model (single-level variable indirection resolution in exec commands). Tests comprehensively cover:
The fix resolves the core vulnerability: preventing dangerous shell interpolation patterns even when hidden behind a single variable reference.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — 2 minor suggestions, no blockers.
📋 Key Themes & Highlights
Key Themes
- Missing boundary test: the intentional single-level-only scope is undocumented by a test, making the limit invisible to future contributors.
- Inline complexity: the resolution + fallback on one line reduces readability.
Positive Highlights
- ✅ Root cause correctly addressed — scope-chain walk resolves write-once variable initializers without false positives on reassigned vars, parameters, or imports.
- ✅ Conservative guards (
defs.length !== 1,def.type !== "Variable", write-only ref check) prevent over-flagging. - ✅ Good test coverage of the new happy paths and the new invalid cases.
- ✅ PR description is clear and matches the implementation exactly.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 33 AIC · ⌖ 4.71 AIC · ⊞ 6.7K
Comment /matt to run again
| code: `const cmd = "git checkout " + branchName; exec.exec(cmd, []);`, | ||
| errors: [{ messageId: "interpolatedCommand", data: { kind: "dynamic string concatenation", method: "exec" } }], | ||
| }, | ||
| ], |
There was a problem hiding this comment.
[/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.
| // Resolve a single level of variable indirection so that | ||
| // const cmd = `git checkout ${branch}`; exec.exec(cmd, []); | ||
| // is flagged in the same way as the direct form. | ||
| const candidate = firstArg.type === AST_NODE_TYPES.Identifier ? (resolveInitializer(firstArg) ?? firstArg) : (firstArg as TSESTree.Expression); |
There was a problem hiding this comment.
[/codebase-design] This line is doing three things at once (type narrowing, resolution, fallback). Extracting to a named variable would make the intent clearer and easier to maintain.
💡 Suggested refactor
const resolvedArg =
firstArg.type === AST_NODE_TYPES.Identifier
? (resolveInitializer(firstArg) ?? firstArg)
: firstArg;
const kind = getDynamicCommandKind(resolvedArg as TSESTree.Expression);Minor, but the current one-liner hides the fallback logic on an already-busy line.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — two correctness gaps must be fixed before merge
Blocking issues
- Chained aliasing bypasses the rule (high) —
resolveInitializeris called exactly once, soconst a = cmd; const cmd = \git ${x}`` is a silent false negative. Either loop until stable or explicitly document the known limitation. - Scope walk climbs into module/global scope (medium) — resolving a module-level variable's initializer is not a safe proxy for its runtime value; the mutation guard cannot see async or cross-module writes. Restrict resolution to local function-scoped bindings.
The one-level indirection check is the correct direction, but the above gaps undermine its security guarantee.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 40.2 AIC · ⌖ 6.5 AIC · ⊞ 5.7K
Comment /review to run again
| // Resolve a single level of variable indirection so that | ||
| // const cmd = `git checkout ${branch}`; exec.exec(cmd, []); | ||
| // is flagged in the same way as the direct form. | ||
| const candidate = firstArg.type === AST_NODE_TYPES.Identifier ? (resolveInitializer(firstArg) ?? firstArg) : (firstArg as TSESTree.Expression); |
There was a problem hiding this comment.
Single-level indirection only — chained aliasing silently bypasses the rule.
💡 Details and suggested fix
resolveInitializer is called exactly once on arguments[0]. If a user writes:
const bad = \`git checkout ${branch}\`;
const cmd = bad; // one extra alias
exec.exec(cmd, []); // NOT flaggedThe rule resolves cmd → bad (an Identifier), then calls getDynamicCommandKind(bad) which returns null because bad is an identifier, not a template literal.
Fix: loop until stable, or add an explicit TODO comment documenting the known gap.
let candidate: TSESTree.Expression =
firstArg.type === AST_NODE_TYPES.Identifier
? (resolveInitializer(firstArg) ?? firstArg)
: firstArg;
while (candidate.type === AST_NODE_TYPES.Identifier) {
const resolved = resolveInitializer(candidate);
if (!resolved) break;
candidate = resolved;
}| /** | ||
| * 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 |
There was a problem hiding this comment.
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;| // Resolve a single level of variable indirection so that | ||
| // const cmd = `git checkout ${branch}`; exec.exec(cmd, []); | ||
| // is flagged in the same way as the direct form. | ||
| const candidate = firstArg.type === AST_NODE_TYPES.Identifier ? (resolveInitializer(firstArg) ?? firstArg) : (firstArg as TSESTree.Expression); |
There was a problem hiding this comment.
Dense ternary with unnecessary cast — split for readability.
💡 Suggested fix
The candidate line crams an as TSESTree.Expression cast into the else branch that serves no purpose — after the SpreadElement guard above, firstArg is already narrowed to TSESTree.Expression. The cast masks the type narrowing and makes the code harder to review.
Prefer an explicit if/else:
let candidate: TSESTree.Expression;
if (firstArg.type === AST_NODE_TYPES.Identifier) {
candidate = resolveInitializer(firstArg) ?? firstArg;
} else {
candidate = firstArg;
}
|
@copilot please run the Unresolved review feedback:
|
…ries Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the unresolved blocking feedback in commit Implemented:
Validation run:
CI on this new head is stale until a maintainer re-triggers workflows. |
no-exec-interpolated-commandonly inspected the syntactic shape ofarguments[0], so storing a dynamic command in a variable before passing it toexec.exec()fully evaded detection.Changes
no-exec-interpolated-command.ts: AddedresolveInitializer()insidecreate()that walks the scope chain and returns a variable's initializer when the binding is a write-onceVariabledefinition (parameters, imports, multiply-assigned vars, and unresolvable identifiers are all skipped). Whenarguments[0]is anIdentifier, the resolved initializer is checked instead of the identifier itself.no-exec-interpolated-command.test.ts: Added newinvalidcases for the variable-indirection pattern and newvalidcases confirming static-string variables, reassigned variables, and function parameters are not flagged.Now flagged
Still valid