codemod to set shell=False for subprocess commands

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,2 +1,2 @@\n import subprocess\n-subprocess.run(\"echo 'hi'\", shell=True)\n+subprocess.run(\"echo 'hi'\", shell=False)\n"
+    expected_line_change = "2"
+    change_description = SubprocessShellFalse.CHANGE_DESCRIPTION
+    # expected because output code points to fake file
+    allowed_exceptions = (FileNotFoundError,)

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="High",
+        guidance_explained="In most cases setting `shell=False` is correct and leads to much safer code. However there are valid use cases for `shell=True` when using shell functionality like pipes or wildcard is required. In such cases it is important to run only trusted, validated commands.",
+    ),
 }
 
 

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,

src/core_codemods/docs/pixee_python_subprocess-shell-false.md

@@ -0,0 +1,11 @@
+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. In the worst case this can give an attacker the ability to run arbitrary commands on your system. In most cases using `shell=False` is sufficient and leads to much safer code.
+
+The changes from this codemod look like this:
+
+```diff
+ import subprocess
+- subprocess.run("echo 'hi'", shell=True)
++ subprocess.run("echo 'hi'", shell=False)
+```

src/core_codemods/subprocess_shell_false.py

@@ -0,0 +1,52 @@
+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 = [
+        {
+            "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 original_node
+
+        if self.find_base_name(original_node.func) in self.SUBPROCESS_FUNCS:
+            for arg in original_node.args:
+                if matchers.matches(
+                    arg,
+                    matchers.Arg(
+                        keyword=matchers.Name("shell"), 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

tests/codemods/test_subprocess_shell_false.py

@@ -0,0 +1,58 @@
+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

tests/samples/subprocess_run.py

@@ -0,0 +1,2 @@
+import subprocess
+subprocess.run("echo 'hi'", shell=True)