Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
eslint-factory: add
no-child-process-interpolated-commandto catch shell-injection command strings #47555New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
eslint-factory: add
no-child-process-interpolated-commandto catch shell-injection command strings #47555Changes from all commits
f95eed348f67c6c83b351ec499487069326539b5be0de203bFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rule is not activated in
eslint.config.cjs: registering the rule inindex.tsdoesn't enable it — the config file must also include an entry, otherwise the rule never runs onactions/setup/js.💡 Fix
Add to
eslint-factory/eslint.config.cjsinside therulesblock:Without this, the rule is dead code in production even though it tests fine in isolation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd] The test suite is a single
itblock with 14 cases, all testing one axis at a time. There are no tests for the variable-options false-negative (const opts = { shell: true }), theshell: 'sh'string value (truthy but not=== true), orexecFilewhen arguments come in position 1 vs position 2. Splitting into focuseditblocks per scenario would make failures much easier to pinpoint.💡 Suggested structure
Document known limitations as skipped/todo tests so they act as regression anchors.
@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing invalid ESM test cases.
The test suite only validates a single valid ESM case but contains no invalid ESM examples. The ESM import path through
isChildProcessImportBindingis a separate code branch from therequirepath, so it deserves at least one invalid case (e.g.import { execSync } from "child_process"; execSync(\git checkout ${branch}`)`) to confirm that branch is actually exercised.@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd]
isShellTrueOptionaccepts onlyshell: true(boolean). A call withshell: 'sh'orshell: '/bin/bash'is also shell-evaluated but will not be flagged, creating a silent bypass.💡 Suggested fix
Change the value check to treat any truthy shell value as shell-enabled:
Add a test case:
spawn(\cmd ${x}`, { shell: 'sh' })` should be invalid.@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spread element in options position silently skips the shell check.
isShellTrueOptionreturnsfalseearly forSpreadElement, sospawn(cmd, ...opts)orspawn(cmd, args, ...opts)is treated as non-shell and skipped. If a spread contains{ shell: true }, the rule will silently miss the violation. The safer default would be to treat an unknown spread as possibly shell-enabled (returntruefrom the spread branch) so the rule errs on the side of flagging rather than silently passing.@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String-literal ESM import specifier missed:
import { 'exec' as run } from 'child_process'(ES2022 module string names) is silently skipped —run(\git ${cmd}`)` would not be flagged.💡 Details and fix
At line ~60, the import binding check does:
String-name imports (
import { 'exec' as run }) use anAST_NODE_TYPES.Literalnode forimported. Fix:This is an edge case, but it's a silent false negative in a security rule.