add subprocess shell=False codemod

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,)

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.",
+    ),
 }
 
 

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,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)
+```

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

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

tests/samples/subprocess_run.py

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