Skip to content

Commit

Permalink
add review updates
Browse files Browse the repository at this point in the history
  • Loading branch information
clavedeluna committed Jan 2, 2024
1 parent 4209b70 commit ed87f3d
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 23 deletions.
27 changes: 24 additions & 3 deletions src/core_codemods/subprocess_shell_false.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,39 @@ class SubprocessShellFalse(BaseCodemod, NameResolutionMixin):
SUMMARY = "Use `shell=False` in `subprocess` Function Calls"
REVIEW_GUIDANCE = ReviewGuidance.MERGE_AFTER_CURSORY_REVIEW
DESCRIPTION = "Set `shell` keyword argument to `False`"
REFERENCES: list = []
REFERENCES = [
{
"url": "https://docs.python.org/3/library/subprocess.html#security-considerations",
"description": "",
},
{
"url": "https://en.wikipedia.org/wiki/Code_injection#Shell_injection",
"description": "",
},
{
"url": "https://stackoverflow.com/a/3172488",
"description": "",
},
]
SUBPROCESS_FUNCS = [
f"subprocess.{func}"
for func in {"run", "call", "check_output", "check_call", "Popen"}
]

def leave_Call(self, original_node: cst.Call, updated_node: cst.Call):
if not self.filter_by_path_includes_or_excludes(
self.node_position(original_node)
):
return

Check warning on line 36 in src/core_codemods/subprocess_shell_false.py

View check run for this annotation

Codecov / codecov/patch

src/core_codemods/subprocess_shell_false.py#L36

Added line #L36 was not covered by tests

if self.find_base_name(original_node.func) in self.SUBPROCESS_FUNCS:
for arg in original_node.args:
if matchers.matches(
arg.keyword, matchers.Name("shell")
) and matchers.matches(arg.value, matchers.Name("True")):
arg,
matchers.Arg(
keyword=matchers.Name("shell"), value=matchers.Name("True")
),
):
self.report_change(original_node)
new_args = self.replace_args(
original_node,
Expand Down
44 changes: 24 additions & 20 deletions tests/codemods/test_subprocess_shell_false.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,40 +15,44 @@ def test_name(self):

@each_func
def test_import(self, tmpdir, func):
input_code = f"""import subprocess
subprocess.{func}(args, shell=True)
"""
expexted_output = f"""import subprocess
subprocess.{func}(args, shell=False)
"""

input_code = f"""
import subprocess
subprocess.{func}(args, shell=True)
"""
expexted_output = f"""
import subprocess
subprocess.{func}(args, shell=False)
"""
self.run_and_assert(tmpdir, input_code, expexted_output)
assert len(self.file_context.codemod_changes) == 1

@each_func
def test_from_import(self, tmpdir, func):
input_code = f"""from subprocess import {func}
{func}(args, shell=True)
"""
expexted_output = f"""from subprocess import {func}
{func}(args, shell=False)
"""

input_code = f"""
from subprocess import {func}
{func}(args, shell=True)
"""
expexted_output = f"""
from subprocess import {func}
{func}(args, shell=False)
"""
self.run_and_assert(tmpdir, input_code, expexted_output)
assert len(self.file_context.codemod_changes) == 1

@each_func
def test_no_shell(self, tmpdir, func):
input_code = f"""import subprocess
subprocess.{func}(args, timeout=1)
"""
input_code = f"""
import subprocess
subprocess.{func}(args, timeout=1)
"""
self.run_and_assert(tmpdir, input_code, input_code)
assert len(self.file_context.codemod_changes) == 0

@each_func
def test_shell_False(self, tmpdir, func):
input_code = f"""import subprocess
subprocess.{func}(args, shell=False)
"""
input_code = f"""
import subprocess
subprocess.{func}(args, shell=False)
"""
self.run_and_assert(tmpdir, input_code, input_code)
assert len(self.file_context.codemod_changes) == 0

0 comments on commit ed87f3d

Please sign in to comment.