diff --git a/integration_tests/test_subprocess_shell_false.py b/integration_tests/test_subprocess_shell_false.py new file mode 100644 index 000000000..1397f8e06 --- /dev/null +++ b/integration_tests/test_subprocess_shell_false.py @@ -0,0 +1,18 @@ +from core_codemods.subprocess_shell_false import SubprocessShellFalse +from integration_tests.base_test import ( + BaseIntegrationTest, + original_and_expected_from_code_path, +) + + +class TestSubprocessShellFalse(BaseIntegrationTest): + codemod = SubprocessShellFalse + code_path = "tests/samples/subprocess_run.py" + original_code, expected_new_code = original_and_expected_from_code_path( + code_path, [(1, "subprocess.run(\"echo 'hi'\", shell=False)\n")] + ) + expected_diff = "--- \n+++ \n@@ -1,3 +1,3 @@\n import subprocess\n-subprocess.run(\"echo 'hi'\", shell=True)\n+subprocess.run(\"echo 'hi'\", shell=False)\n \n" + expected_line_change = "2" + change_description = SubprocessShellFalse.CHANGE_DESCRIPTION + # expected because output code points to fake file + allowed_exceptions = (FileNotFoundError,) diff --git a/src/codemodder/scripts/generate_docs.py b/src/codemodder/scripts/generate_docs.py index e2e77fb63..f0742ec01 100644 --- a/src/codemodder/scripts/generate_docs.py +++ b/src/codemodder/scripts/generate_docs.py @@ -178,6 +178,10 @@ class DocMetadata: importance="Low", guidance_explained="Since literals and new objects have their own identities, comparisons against them using `is` operators are most likely a bug and thus we deem the change safe.", ), + "subprocess-shell-false": DocMetadata( + importance="Medium", + guidance_explained="There are valid use cases for `shell=True` such as executing a validated string command or using shell functionality like globs or wildcard.", + ), } diff --git a/src/core_codemods/__init__.py b/src/core_codemods/__init__.py index a4e1cdba0..cceb8428d 100644 --- a/src/core_codemods/__init__.py +++ b/src/core_codemods/__init__.py @@ -38,6 +38,7 @@ from .sql_parameterization import SQLQueryParameterization from .exception_without_raise import ExceptionWithoutRaise from .literal_or_new_object_identity import LiteralOrNewObjectIdentity +from .subprocess_shell_false import SubprocessShellFalse registry = CodemodCollection( origin="pixee", @@ -75,6 +76,7 @@ WithThreadingLock, SQLQueryParameterization, SecureFlaskSessionConfig, + SubprocessShellFalse, FileResourceLeak, DjangoReceiverOnTop, NumpyNanEquality, diff --git a/src/core_codemods/docs/pixee_python_subprocess-shell-false.md b/src/core_codemods/docs/pixee_python_subprocess-shell-false.md new file mode 100644 index 000000000..b6b269f7c --- /dev/null +++ b/src/core_codemods/docs/pixee_python_subprocess-shell-false.md @@ -0,0 +1,12 @@ +This codemod sets the `shell` keyword argument to `False` in `subprocess` module function calls that have set it to `True`. + +Setting `shell=True` will execute the provided command through the system shell which can lead to shell injection vulnerabilities. + +The changes from this codemod look like this: + +```diff +- x = foo() +- if x is not None: ++ if (x := foo()) is not None: + print(x) +``` diff --git a/src/core_codemods/subprocess_shell_false.py b/src/core_codemods/subprocess_shell_false.py new file mode 100644 index 000000000..21ecc5cfa --- /dev/null +++ b/src/core_codemods/subprocess_shell_false.py @@ -0,0 +1,31 @@ +import libcst as cst +from libcst import matchers +from codemodder.codemods.api import BaseCodemod, ReviewGuidance +from codemodder.codemods.utils_mixin import NameResolutionMixin +from codemodder.codemods.api.helpers import NewArg + + +class SubprocessShellFalse(BaseCodemod, NameResolutionMixin): + NAME = "subprocess-shell-false" + SUMMARY = "Use `shell=False` in `subprocess` Function Calls" + REVIEW_GUIDANCE = ReviewGuidance.MERGE_AFTER_CURSORY_REVIEW + DESCRIPTION = "Set `shell` keyword argument to `False`" + REFERENCES: list = [] + 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 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")): + self.report_change(original_node) + new_args = self.replace_args( + original_node, + [NewArg(name="shell", value="False", add_if_missing=False)], + ) + return self.update_arg_target(updated_node, new_args) + return original_node diff --git a/tests/codemods/test_subprocess_shell_false.py b/tests/codemods/test_subprocess_shell_false.py new file mode 100644 index 000000000..782169008 --- /dev/null +++ b/tests/codemods/test_subprocess_shell_false.py @@ -0,0 +1,54 @@ +import pytest +from core_codemods.subprocess_shell_false import SubprocessShellFalse +from tests.codemods.base_codemod_test import BaseCodemodTest + +each_func = pytest.mark.parametrize( + "func", ["check_output", "check_call", "run", "call", "Popen"] +) + + +class TestSubprocessShellFalse(BaseCodemodTest): + codemod = SubprocessShellFalse + + def test_name(self): + assert self.codemod.name() == "subprocess-shell-false" + + @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) +""" + + 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) +""" + + 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) +""" + 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) +""" + self.run_and_assert(tmpdir, input_code, input_code) + assert len(self.file_context.codemod_changes) == 0 diff --git a/tests/samples/subprocess_run.py b/tests/samples/subprocess_run.py new file mode 100644 index 000000000..70bf26f10 --- /dev/null +++ b/tests/samples/subprocess_run.py @@ -0,0 +1,2 @@ +import subprocess +subprocess.run("echo 'hi'", shell=True)