Detect chained string methods on interpolated commands in exec/child_process rules#52707
Conversation
…process rules Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ments are flagged Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR Triage\n\n- Category: feature\n- Risk: low\n- Priority: medium\n- Score: 47/100 (impact 30 + urgency 10 + quality 7)\n- Recommended action:
|
|
👋 Great work on the eslint-factory improvements! This PR looks ready for review. The refactoring cleanly centralizes command detection logic ( The changes:
This aligns with the project's agentic development process (core team using Copilot agent) and adds genuine security value by catching previously-missed edge cases.
|
There was a problem hiding this comment.
Pull request overview
Extends command-injection lint rules to detect interpolated commands wrapped in common string transformations.
Changes:
- Centralizes dynamic-command analysis in shared utilities.
- Recursively analyzes chained string methods and their arguments.
- Adds rule tests and documentation.
Show a summary per file
| File | Description |
|---|---|
command-initializer-utils.ts |
Adds shared recursive command analysis. |
no-exec-interpolated-command.ts |
Uses the shared analyzer. |
no-exec-interpolated-command.test.ts |
Tests transformed exec commands. |
no-child-process-interpolated-command.ts |
Uses the shared analyzer. |
no-child-process-interpolated-command.test.ts |
Tests transformed child-process commands. |
README.md |
Documents recognized chained methods. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (2)
eslint-factory/src/rules/command-initializer-utils.ts:125
- Function-valued replacements remain an escape:
exec.exec("git X".replace("X", () =>${branch}), [])is not reported becausegetDynamicCommandKindreceives anArrowFunctionExpressionand returns null, although its interpolated return value becomes command text. Inspect return expressions of replacement callbacks forreplace/replaceAll.
const argKind = getDynamicCommandKind(arg, sourceCode, seen);
eslint-factory/README.md:722
- This supported-method list omits
toLocaleLowerCaseandtoLocaleUpperCase, although both are included inSTRING_TRANSFORM_METHODS, so users cannot tell that these calls are also inspected.
- `` exec.exec(`git checkout ${branchName}`.trim()) `` — chained string-normalizing methods (`trim`, `trimStart`, `trimEnd`, `toLowerCase`, `toUpperCase`, `replace`, `replaceAll`, `normalize`) are unwrapped before the check.
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
| for (const arg of transform.args) { | ||
| if (arg.type === AST_NODE_TYPES.SpreadElement) continue; | ||
| const argKind = getDynamicCommandKind(arg, sourceCode, seen); | ||
| if (argKind) return argKind; |
| - `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`, `replace`, `replaceAll`, `normalize`) are unwrapped before the check. |
|
@copilot Please address the current review feedback and then run the Open review feedback to resolve:
After updates, summarize what changed and any remaining blockers for maintainers.
|
… case methods Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 3ba... (latest push):
No remaining blockers from these items. The 5 pre-existing |
PR Triage
Automated triage by PR Triage Agent.
|
no-exec-interpolated-commandandno-child-process-interpolated-commandonly recognized a bare interpolated template literal or dynamic+concatenation as the command argument, so a routine normalization step defeated both rules:The argument node is a
CallExpression, not aTemplateLiteral, andresolveWriteOnceInitializerChainonly unwraps identifier indirection.Changes
isStaticExpression/isDynamicStringConcatenation/getDynamicCommandKindtrio intocommand-initializer-utils.ts; both rules now callgetDynamicCommandKind(expr, sourceCode), which applies the write-once initializer resolution internally.getDynamicCommandKindrecursively descends through calls totrim,trimStart,trimEnd,toLowerCase,toUpperCase,toLocaleLowerCase,toLocaleUpperCase,replace,replaceAll, andnormalize, resolving identifiers at each step. The reported kind is the underlying shape (interpolated template literal/dynamic string concatenation), so existing message data is unchanged..replace()can inject dynamic content via their replacement argument, so arguments are inspected as well andisStaticExpressiontreats a transform call as static only when the receiver and all arguments are static. This also preserves the previous behavior where"a".replace(x) + "b"is dynamic..trim()- and.toLowerCase()-chained templates, chained dynamic concatenation, identifier-then-.trim(), and a dynamic.replace()argument; valid cases assert chained methods on fully static strings/concatenations stay unflagged.Note: the 5 failing tests in
require-fs-io-try-catch.test.tspredate this branch and are untouched here.