security: validate package names before installation

Author: geritwagner

colrev/package_manager/check.py

@@ -9,6 +9,7 @@
 from importlib import import_module
 from importlib import util
 from pathlib import Path
+import re
 
 import toml
 
@@ -104,10 +105,25 @@
 }
 
 
+def _validate_package_name(package_name: str) -> str:
+    if not package_name:
+        raise ValueError("Invalid package name: package name must not be empty.")
+    if package_name.startswith("-"):
+        raise ValueError("Invalid package name: package name must not start with '-'.")
+    if not re.match(r"^[A-Za-z0-9][A-Za-z0-9._-]*$", package_name):
+        raise ValueError(
+            "Invalid package name: only letters, digits, dots, underscores, and hyphens are allowed."
+        )
+    return package_name
+
+
 def _check_package_installed(data: dict) -> bool:
-    package_name = data["project"]["name"]
+    package_name = _validate_package_name(data["project"]["name"])
     try:
-        subprocess.check_output([sys.executable, "-m", "pip", "show", package_name])
+        # package_name is validated above and shell=True is not used.
+        subprocess.check_output(  # nosec B603
+            [sys.executable, "-m", "pip", "show", package_name]
+        )
     except subprocess.CalledProcessError:
         print(
             f"Navigate to {Path.cwd()} and run: "

colrev/package_manager/package_manager.py

@@ -13,11 +13,21 @@
 import colrev.exceptions as colrev_exceptions
 import colrev.package_manager.colrev_internal_packages
 import colrev.package_manager.package
+from colrev.package_manager.colrev_internal_packages import get_internal_packages_dict
 from colrev.constants import Colors
 from colrev.constants import EndpointType
 from colrev.constants import Filepaths
 
 
+def _validate_internal_package_selection(
+    *, selected_package: str, internal_packages_dict: dict[str, str]
+) -> None:
+    if selected_package not in internal_packages_dict:
+        raise ValueError(
+            f"Installation rejected: unknown internal package '{selected_package}'."
+        )
+
+
 def _run_uv_pip_list() -> subprocess.CompletedProcess[str]:
     """Return package information reported by ``uv pip list``.
 
@@ -190,16 +200,19 @@ def install(
 
         # print(package_manager)
 
-        internal_packages_dict = (
-            colrev.package_manager.colrev_internal_packages.get_internal_packages_dict()
-        )
+        internal_packages_dict = get_internal_packages_dict()
 
         if len(packages) == 1 and packages[0] == "all_internal_packages":
             packages = list(internal_packages_dict.keys())
 
         # Install internal colrev packages first
         colrev_packages = []
         for package in packages:
+            if package.startswith("colrev."):
+                _validate_internal_package_selection(
+                    selected_package=package,
+                    internal_packages_dict=internal_packages_dict,
+                )
             if package in internal_packages_dict:
                 colrev_packages.append(package)
         packages = [p for p in packages if p not in colrev_packages]

tests/1_env/package_manager_security_test.py

@@ -0,0 +1,72 @@
+#!/usr/bin/env python
+"""Security tests for package-manager subprocess arguments."""
+
+from __future__ import annotations
+
+from unittest.mock import MagicMock
+
+import pytest
+
+from colrev.package_manager import check
+from colrev.package_manager.package_manager import PackageManager
+
+
+@pytest.mark.parametrize(
+    "package_name",
+    ["colrev", "my-package", "my_package", "my.package", "p1._-name"],
+)
+def test_validate_package_name_accepts_valid_names(package_name: str) -> None:
+    assert check._validate_package_name(package_name) == package_name
+
+
+@pytest.mark.parametrize("package_name", ["", "-help", "bad name", "name;rm -rf /"])
+def test_validate_package_name_rejects_invalid_names(package_name: str) -> None:
+    with pytest.raises(ValueError):
+        check._validate_package_name(package_name)
+
+
+def test_check_package_installed_validates_package_name_before_subprocess(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    check_output_mock = MagicMock()
+    monkeypatch.setattr(check.subprocess, "check_output", check_output_mock)
+
+    result = check._check_package_installed({"project": {"name": "colrev"}})
+
+    assert result is True
+    check_output_mock.assert_called_once()
+
+
+def test_install_accepts_internal_package_and_calls_subprocess(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    run_mock = MagicMock()
+    monkeypatch.setattr(
+        "colrev.package_manager.package_manager.subprocess.run", run_mock
+    )
+    monkeypatch.setattr(
+        "colrev.package_manager.package_manager.get_internal_packages_dict",
+        MagicMock(return_value={"colrev.allowed": "colrev-allowed"}),
+    )
+
+    PackageManager().install(packages=["colrev.allowed"], upgrade=False)
+
+    run_mock.assert_called_once_with(["pip", "install", "colrev-allowed"], check=True)
+
+
+def test_install_rejects_unknown_internal_package_before_subprocess(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    run_mock = MagicMock()
+    monkeypatch.setattr(
+        "colrev.package_manager.package_manager.subprocess.run", run_mock
+    )
+    monkeypatch.setattr(
+        "colrev.package_manager.package_manager.get_internal_packages_dict",
+        MagicMock(return_value={"colrev.allowed": "colrev-allowed"}),
+    )
+
+    with pytest.raises(ValueError):
+        PackageManager().install(packages=["colrev.unknown"], upgrade=False)
+
+    run_mock.assert_not_called()