Return a list instead of string for disablerepo command

The current `get_rhel_disable_repos_command` is returning a list of disablerepo commands to be passed down to run_subprocess functions. This is insecure as it contains white spaces inbetween the commands. This patch changes this to return them in a list instead.
oamg · May 23, 2024 · bd2154a · bd2154a
1 parent 523b84b
commit bd2154a
Show file tree

Hide file tree

Showing 5 changed files with 40 additions and 23 deletions.
diff --git a/convert2rhel/actions/system_checks/is_loaded_kernel_latest.py b/convert2rhel/actions/system_checks/is_loaded_kernel_latest.py
@@ -51,10 +51,9 @@ def run(self):  # pylint: disable= too-many-return-statements
             "repoquery",
             "--setopt=exclude=",
             "--quiet",
-            disable_repo_command,
-            "--qf",
-            "C2R\\t%{BUILDTIME}\\t%{VERSION}-%{RELEASE}\\t%{REPOID}",
         ]
+        cmd.extend(disable_repo_command)
+        cmd.extend(["--qf", "C2R\\t%{BUILDTIME}\\t%{VERSION}-%{RELEASE}\\t%{REPOID}"])
 
         # For Oracle/CentOS Linux 8 the `kernel` is just a meta package, instead,
         # we check for `kernel-core`. But 7 releases, the correct way to check is

diff --git a/convert2rhel/pkghandler.py b/convert2rhel/pkghandler.py
@@ -402,7 +402,10 @@ def _get_package_repositories(pkgs, disable_repos=None):
     # If needed, disable some repos for the repoquery
     disable_repo_command = repo.get_rhel_disable_repos_command(disable_repos)
 
-    cmd = ["repoquery", "--quiet", "-q", disable_repo_command] + pkgs + ["--qf", query_format]
+    cmd = ["repoquery", "--quiet", "-q"]
+    cmd.extend(disable_repo_command)
+    cmd.extend(pkgs)
+    cmd.extend(["--qf", query_format])
 
     output, retcode = utils.run_subprocess(
         cmd,

diff --git a/convert2rhel/repo.py b/convert2rhel/repo.py
@@ -78,12 +78,12 @@ def get_rhel_disable_repos_command(disable_repos):
     :param disable_repos: List of repo IDs to disable
     :type disable_repos: List[str]
     :return: String for disabling the rhel and user provided repositories while performing checks.
-    :rtype: str
+    :rtype: list[str]
     """
     if not disable_repos:
-        return ""
+        return []
 
-    disable_repo_command = " ".join("--disablerepo=" + repo for repo in disable_repos)
+    disable_repo_command = ["".join("--disablerepo=" + repo) for repo in disable_repos]
 
     return disable_repo_command
 

diff --git a/convert2rhel/unit_tests/actions/system_checks/is_loaded_kernel_latest_test.py b/convert2rhel/unit_tests/actions/system_checks/is_loaded_kernel_latest_test.py
@@ -766,14 +766,37 @@ def test_is_loaded_kernel_latest_system_exit(self, monkeypatch, is_loaded_kernel
 
     @centos7
     @pytest.mark.parametrize(
-        ("enablerepos", "disable_repo_cmd"),
+        ("enablerepos", "expected_cmd"),
         (
-            ([], "--disablerepo=rhel*"),
-            (["test-repo"], "--disablerepo=rhel* --disablerepo=test-repo"),
+            (
+                [],
+                [
+                    "repoquery",
+                    "--setopt=exclude=",
+                    "--quiet",
+                    "--disablerepo=rhel*",
+                    "--qf",
+                    "C2R\\t%{BUILDTIME}\\t%{VERSION}-%{RELEASE}\\t%{REPOID}",
+                    "kernel",
+                ],
+            ),
+            (
+                ["test-repo"],
+                [
+                    "repoquery",
+                    "--setopt=exclude=",
+                    "--quiet",
+                    "--disablerepo=rhel*",
+                    "--disablerepo=test-repo",
+                    "--qf",
+                    "C2R\\t%{BUILDTIME}\\t%{VERSION}-%{RELEASE}\\t%{REPOID}",
+                    "kernel",
+                ],
+            ),
         ),
     )
     def test_is_loaded_kernel_latest_disable_repos(
-        self, monkeypatch, enablerepos, is_loaded_kernel_latest_action, pretend_os, disable_repo_cmd
+        self, monkeypatch, enablerepos, expected_cmd, is_loaded_kernel_latest_action, pretend_os
     ):
         """Test if the --disablerepo part of the command is built propertly."""
         run_subprocess = mock.Mock(return_value=[None, None])
@@ -788,14 +811,6 @@ def test_is_loaded_kernel_latest_disable_repos(
         is_loaded_kernel_latest_action.run()
 
         run_subprocess.assert_called_with(
-            [
-                "repoquery",
-                "--setopt=exclude=",
-                "--quiet",
-                disable_repo_cmd,
-                "--qf",
-                "C2R\\t%{BUILDTIME}\\t%{VERSION}-%{RELEASE}\\t%{REPOID}",
-                "kernel",
-            ],
+            expected_cmd,
             print_output=False,
         )
diff --git a/convert2rhel/unit_tests/repo_test.py b/convert2rhel/unit_tests/repo_test.py
@@ -92,9 +92,9 @@ def test_get_rhel_repos_to_disable(monkeypatch, enablerepo, disablerepos):
 @pytest.mark.parametrize(
     ("disable_repos", "command"),
     (
-        ([], ""),
-        (["test-repo"], "--disablerepo=test-repo"),
-        (["rhel*", "test-repo"], "--disablerepo=rhel* --disablerepo=test-repo"),
+        ([], []),
+        (["test-repo"], ["--disablerepo=test-repo"]),
+        (["rhel*", "test-repo"], ["--disablerepo=rhel*", "--disablerepo=test-repo"]),
     ),
 )
 def test_get_rhel_disable_repos_command(disable_repos, command):