Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Added new detect-child-process rule (#252) #855

Merged
merged 2 commits into from Apr 24, 2019
Merged

Added new detect-child-process rule (#252) #855

merged 2 commits into from Apr 24, 2019

Conversation

soon
Copy link
Contributor

@soon soon commented Apr 4, 2019

PR checklist

Overview of change:

Added new detect-child-process rule.

Is there anything you'd like reviewers to focus on?

Spelling (docs), Rule meta (not sure about issueClass, severity and level).

@JoshuaKGoldberg JoshuaKGoldberg added the PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon! label Apr 5, 2019
JoshuaKGoldberg
JoshuaKGoldberg suggested changes Apr 15, 2019
View reviewed changes
Copy link

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start! Some structural & documentation feedback.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/detectChildProcessRule.ts Show resolved Hide resolved
src/detectChildProcessRule.ts Outdated Show resolved Hide resolved
src/detectChildProcessRule.ts Outdated Show resolved Hide resolved
tests/detect-child-process/test.ts.lint Show resolved Hide resolved
src/detectChildProcessRule.ts Outdated Show resolved Hide resolved
tests/detect-child-process/test.ts.lint Show resolved Hide resolved
@JoshuaKGoldberg
Copy link

Sorry for taking so long to get to this! I'm happy to discuss here or on gitter.im if any of the feedback doesn't make sense!

@JoshuaKGoldberg JoshuaKGoldberg added PR: Waiting for Author Changes have been requested that the pull request author should address. and removed PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon! labels Apr 15, 2019
@soon
Copy link
Contributor Author

soon commented Apr 16, 2019 •
edited

@JoshuaKGoldberg Thanks for your feedback! I've just updated the code, please review

Do we need to handle require(['child_process'])? I've never used this form of require before, but I found some tests including require with array as the first argument for other checks

@JoshuaKGoldberg
Copy link

Do we need to handle require(['child_process'])? I've never used this form of require before, but I found some tests including require with array as the first argument for other checks

Ooh, good question. No, let's ignore that for now. Direct AMD modules are falling out of favor and most rules don't bother with them.

@JoshuaKGoldberg JoshuaKGoldberg added PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon! and removed PR: Waiting for Author Changes have been requested that the pull request author should address. labels Apr 19, 2019
@soon
Copy link
Contributor Author

soon commented Apr 23, 2019

@JoshuaKGoldberg Anything else should be changed in this PR?

JoshuaKGoldberg
JoshuaKGoldberg approved these changes Apr 24, 2019
View reviewed changes
Copy link

@JoshuaKGoldberg JoshuaKGoldberg left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Just a couple of nitpicks on the metadata text that I'll merge in. Looks like I don't have permissions. @soon would you mind committing the suggestions? This is good to go otherwise! 🚀

README.md Outdated
</td>
<td>
Detects usages of child_process and especially child_process.exec() with a non-literal first argument.
It is dangerous to pass a string constructed at runtime as the first argument to the child_process.exec().
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
It is dangerous to pass a string constructed at runtime as the first argument to the child_process.exec().
It is dangerous to pass a string constructed at runtime as the first argument to <code>child_process.exec()</code>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

README.md Outdated
<td>
Detects usages of child_process and especially child_process.exec() with a non-literal first argument.
It is dangerous to pass a string constructed at runtime as the first argument to the child_process.exec().
<code>child_process.exec(cmd)</code> runs <code>cmd</code> as a shell command which allows attacker to execute malicious code injected into <code>cmd</code> string.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<code>child_process.exec(cmd)</code> runs <code>cmd</code> as a shell command which allows attacker to execute malicious code injected into <code>cmd</code> string.
<code>child_process.exec(cmd)</code> runs <code>cmd</code> as a shell command which could allow an attacker to execute malicious code injected into <code>cmd</code>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@soon
Copy link
Contributor Author

soon commented Apr 24, 2019

@JoshuaKGoldberg Commited your changes

JoshuaKGoldberg
JoshuaKGoldberg approved these changes Apr 24, 2019
View reviewed changes
Copy link

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic, thanks @soon! 🎉

@JoshuaKGoldberg JoshuaKGoldberg merged commit b819421 into microsoft:master Apr 24, 2019
@IllusionMH IllusionMH added this to the 6.2.0-beta milestone May 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JoshuaKGoldberg JoshuaKGoldberg JoshuaKGoldberg approved these changes

Assignees
No one assigned
Labels
PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon!
Projects
None yet
Milestone
6.2.0-beta
Development

Successfully merging this pull request may close these issues.

new security rule: detect-child-process
3 participants
@soon @JoshuaKGoldberg @IllusionMH