Bandit audit

Addon.py

@@ -29,7 +29,12 @@
 from typing import Dict, Set, List, Optional
 from threading import Lock
 from enum import IntEnum, auto
-from xml.etree.ElementTree import ParseError as XmlParseError
+
+# Audited: only the exception class is imported, for catching errors raised by defusedxml,
+# which re-exports this same class. All parsing is done by defusedxml. (added nosec B405)
+from xml.etree.ElementTree import ParseError as XmlParseError  # nosec B405
+
+from defusedxml import DefusedXmlException
 
 try:
     import importlib.metadata as importlib_metadata
@@ -341,7 +346,7 @@ def load_metadata_file(self, file: str) -> None:
         if os.path.exists(file):
             try:
                 metadata = MetadataReader.from_file(file)
-            except XmlParseError:
+            except (XmlParseError, DefusedXmlException):
                 fci.Console.PrintWarning(
                     "An invalid or corrupted package.xml file was found in the cache for"
                 )
@@ -360,7 +365,7 @@ def _load_installed_metadata(self) -> None:
         if os.path.isfile(installed_metadata_path):
             try:
                 self.installed_metadata = MetadataReader.from_file(installed_metadata_path)
-            except XmlParseError:
+            except (XmlParseError, DefusedXmlException):
                 fci.Console.PrintWarning(
                     "An invalid or corrupted package.xml file was found in installation of"
                 )

AddonCatalog.py

@@ -25,7 +25,12 @@
 import base64
 import datetime
 import os
-from xml.etree.ElementTree import ParseError as XmlParseError
+
+# Audited: only the exception class is imported, for catching errors raised by defusedxml,
+# which re-exports this same class. All parsing is done by defusedxml. (added nosec B405)
+from xml.etree.ElementTree import ParseError as XmlParseError  # nosec B405
+
+from defusedxml import DefusedXmlException
 from dataclasses import dataclass
 import json
 from hashlib import sha256
@@ -157,7 +162,7 @@ def instantiate_addon(self, addon_id: str) -> Addon:
         if self.metadata:
             try:
                 self._load_addon_metadata(addon, self.metadata)
-            except XmlParseError:
+            except (XmlParseError, DefusedXmlException):
                 fci.Console.PrintWarning(
                     "An invalid or corrupted package.xml file was installed "
                     f"for {addon.display_name}\n"
@@ -171,7 +176,7 @@ def instantiate_addon(self, addon_id: str) -> Addon:
             try:
                 package_file = os.path.join(fci.DataPaths().mod_dir, addon_id, "package.xml")
                 addon.installed_metadata = MetadataReader.from_file(package_file)
-            except (FileNotFoundError, XmlParseError, RuntimeError):
+            except (FileNotFoundError, XmlParseError, DefusedXmlException, RuntimeError):
                 pass  # If there was an error, just ignore it, no metadata is not fatal
 
             most_recent_mtime = AddonCatalogEntry.most_recent_mtime(addon_id)

AddonCatalogCacheCreator.py

@@ -37,9 +37,18 @@
 import os
 import re
 import requests
-import subprocess
+
+# Audited: all subprocess calls in this module are fixed git argument lists run with no shell;
+# the variable arguments (url, branch, name) come from the addon index this tool exists to
+# process (added nosec B404, and B603/B607 at the call sites)
+import subprocess  # nosec B404
 from typing import List
-from xml.etree.ElementTree import ParseError as XmlParseError
+
+# Audited: only the exception class is imported, for catching errors raised by defusedxml,
+# which re-exports this same class. All parsing is done by defusedxml. (added nosec B405)
+from xml.etree.ElementTree import ParseError as XmlParseError  # nosec B405
+
+from defusedxml import DefusedXmlException
 import zipfile
 
 import AddonCatalog
@@ -256,7 +265,7 @@ def get_git_info(
         results = []
         for cmd in (hash_cmd, tag_cmd):
             try:
-                result = subprocess.run(
+                result = subprocess.run(  # nosec B603
                     cmd,
                     capture_output=True,
                     text=True,
@@ -319,7 +328,7 @@ def generate_cache_entry_from_package_xml(
             metadata = addonmanager_metadata.MetadataReader.from_bytes(
                 cache_entry.package_xml.encode("utf-8")
             )
-        except XmlParseError:
+        except (XmlParseError, DefusedXmlException):
             print(f"ERROR: Failed to parse XML from {path_to_package_xml}")
             return None
         except RuntimeError:
@@ -503,7 +512,7 @@ def clone_or_update(self, name: str, url: str, branch: str) -> None:
                 name,
             ]
             try:
-                completed_process = subprocess.run(command, timeout=CLONE_TIMEOUT)
+                completed_process = subprocess.run(command, timeout=CLONE_TIMEOUT)  # nosec B603
             except subprocess.TimeoutExpired:
                 self.clone_errors[name] = f"Timed out after {CLONE_TIMEOUT} seconds."
                 raise RuntimeError(f"Clone of {url} timed out.")
@@ -544,18 +553,22 @@ def sparse_clone(self, name: str, url: str, branch: str, files: List[str]) -> No
             os.makedirs(clone_path)
             os.chdir(clone_path)
             try:
-                subprocess.run(["git", "init", "--quiet"], check=True)
-                subprocess.run(["git", "remote", "add", "origin", url], check=True)
-                subprocess.run(["git", "config", "core.sparsecheckout", "true"], check=True)
+                subprocess.run(["git", "init", "--quiet"], check=True)  # nosec B603 B607
+                subprocess.run(
+                    ["git", "remote", "add", "origin", url], check=True
+                )  # nosec B603 B607
+                subprocess.run(  # nosec B603 B607
+                    ["git", "config", "core.sparsecheckout", "true"], check=True
+                )
                 with open(".git/info/sparse-checkout", "w") as f:
                     f.write("\n".join(files))
                     f.write("\n")  # So we are safe appending later
-                subprocess.run(
+                subprocess.run(  # nosec B603 B607
                     ["git", "fetch", "--depth=1", "origin", branch],
                     check=True,
                     timeout=CLONE_TIMEOUT,
                 )
-                subprocess.run(["git", "checkout", branch], check=True)
+                subprocess.run(["git", "checkout", branch], check=True)  # nosec B603 B607
             except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e:
                 self.clone_errors[name] = str(e)
                 print(f"ERROR: {e}")
@@ -565,13 +578,17 @@ def sparse_clone(self, name: str, url: str, branch: str, files: List[str]) -> No
             cwd = os.getcwd()
             os.chdir(os.path.join(cwd, name))
             try:
-                subprocess.run(
+                subprocess.run(  # nosec B603 B607
                     ["git", "fetch", "--force", "--depth=1", "origin", branch],
                     check=True,
                     timeout=CLONE_TIMEOUT,
                 )
-                subprocess.run(["git", "reset", "--hard", "FETCH_HEAD", "--quiet"], check=True)
-                subprocess.run(["git", "clean", "-x", "-f", "-d", "--quiet"], check=True)
+                subprocess.run(  # nosec B603 B607
+                    ["git", "reset", "--hard", "FETCH_HEAD", "--quiet"], check=True
+                )
+                subprocess.run(  # nosec B603 B607
+                    ["git", "clean", "-x", "-f", "-d", "--quiet"], check=True
+                )
             except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e:
                 self.clone_errors[name] = str(e)
                 print(f"ERROR: {e}")
@@ -587,7 +604,7 @@ def add_to_sparse_clone(self, name: str, files: List[str]) -> None:
             f.write("\n".join(files))
             f.write("\n")  # So we are safe appending later
         try:
-            subprocess.run(["git", "read-tree", "-m", "-u", "HEAD"], check=True)
+            subprocess.run(["git", "read-tree", "-m", "-u", "HEAD"], check=True)  # nosec B603 B607
         except subprocess.CalledProcessError as e:
             self.clone_errors[name] = str(e)
             print(f"ERROR: {e}")
@@ -622,7 +639,9 @@ def fetch_and_reset(name: str, url: str, branch: str) -> None:
         if any of the git calls fails."""
 
         try:
-            completed_process = subprocess.run(["git", "fetch", "--force"], timeout=CLONE_TIMEOUT)
+            completed_process = subprocess.run(  # nosec B603 B607
+                ["git", "fetch", "--force"], timeout=CLONE_TIMEOUT
+            )
         except subprocess.TimeoutExpired:
             raise RuntimeError(f"git fetch for {name} timed out after {CLONE_TIMEOUT} seconds")
         if completed_process.returncode != 0:
@@ -631,11 +650,15 @@ def fetch_and_reset(name: str, url: str, branch: str) -> None:
         git_ref_type = CacheWriter.determine_git_ref_type(name, url, branch)
         reset_target = f"origin/{branch}" if git_ref_type == GitRefType.BRANCH else branch
 
-        completed_process = subprocess.run(["git", "reset", "--hard", reset_target, "--quiet"])
+        completed_process = subprocess.run(  # nosec B603 B607
+            ["git", "reset", "--hard", reset_target, "--quiet"]
+        )
         if completed_process.returncode != 0:
             raise RuntimeError(f"git reset failed for {name} ref {reset_target}")
 
-        completed_process = subprocess.run(["git", "clean", "-x", "-f", "-d", "--quiet"])
+        completed_process = subprocess.run(  # nosec B603 B607
+            ["git", "clean", "-x", "-f", "-d", "--quiet"]
+        )
         if completed_process.returncode != 0:
             raise RuntimeError(f"git clean failed for {name}")
 
@@ -644,16 +667,16 @@ def determine_git_ref_type(name: str, _url: str, branch: str) -> GitRefType:
         """Determine if the given branch, tag, or hash is a tag, branch, or hash. Returns the type
         if determinable, otherwise raises a RuntimeError."""
         command = ["git", "show-ref", "--verify", f"refs/remotes/origin/{branch}"]
-        completed_process = subprocess.run(command, capture_output=True)
+        completed_process = subprocess.run(command, capture_output=True)  # nosec B603
         if completed_process.returncode == 0:
             return GitRefType.BRANCH
         command = ["git", "show-ref", "--tags"]
-        completed_process = subprocess.run(command, capture_output=True)
+        completed_process = subprocess.run(command, capture_output=True)  # nosec B603
         completed_process_output = completed_process.stdout.decode("utf-8")
         if branch in completed_process_output:
             return GitRefType.TAG
         command = ["git", "rev-parse", branch]
-        completed_process = subprocess.run(command)
+        completed_process = subprocess.run(command)  # nosec B603
         if completed_process.returncode == 0:
             return GitRefType.HASH
         raise RuntimeError(
@@ -665,7 +688,7 @@ def determine_git_ref_type(name: str, _url: str, branch: str) -> GitRefType:
     def determine_last_commit_time() -> datetime.datetime:
         """Executed on the current working directory. Returns the time of the last commit."""
         command = ["git", "log", "-1", "--format=%cd", "--date=iso-strict"]
-        completed_process = subprocess.run(command, capture_output=True)
+        completed_process = subprocess.run(command, capture_output=True)  # nosec B603
         completed_process_output = completed_process.stdout.decode("utf-8").strip()
         try:
             dt = datetime.datetime.fromisoformat(completed_process_output)

AddonManagerTest/app/test_addon.py

@@ -47,6 +47,25 @@ def test_display_name(self):
         self.assertEqual(addon.name, "FreeCAD")
         self.assertEqual(addon.display_name, "Test Workbench")
 
+    def test_load_metadata_file_ignores_xml_with_entity_declaration(self):
+        addon = Addon(
+            "FreeCAD",
+            "https://github.com/FreeCAD/FreeCAD",
+            Addon.Status.NOT_INSTALLED,
+            "master",
+        )
+        xml_with_entity = (
+            '<?xml version="1.0" encoding="UTF-8"?>\n'
+            '<!DOCTYPE package [<!ENTITY payload "expanded">]>\n'
+            '<package format="1"><name>&payload;</name></package>\n'
+        )
+        with tempfile.TemporaryDirectory() as temp_dir:
+            file_path = os.path.join(temp_dir, "package.xml")
+            with open(file_path, "w", encoding="utf-8") as f:
+                f.write(xml_with_entity)
+            addon.load_metadata_file(file_path)
+        self.assertIsNone(addon.metadata)
+
     def test_git_url_cleanup(self):
         base_url = "https://github.com/FreeCAD/FreeCAD"
         test_urls = [f"  {base_url}  ", f"{base_url}.git", f"  {base_url}.git  "]

AddonManagerTest/app/test_cmake_file_lists.py

@@ -28,7 +28,9 @@
 
 import os
 import re
-import subprocess
+
+# Audited: only runs a fixed git command against this repository (added nosec B404)
+import subprocess  # nosec B404
 import unittest
 
 REPO_ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), "..", ".."))
@@ -71,16 +73,16 @@
 def files_listed_in_cmake(cmake_path):
     """Return the set of file names referenced inside any SET() block.
 
-    Tokens are file names when they contain a dot or are the literal LICENSE;
+    Words are file names when they contain a dot or are the literal LICENSE;
     the SET variable names (for example AddonManager_SRCS) have neither and are
     skipped."""
     with open(cmake_path, "r", encoding="utf-8") as cmake_file:
         contents = cmake_file.read()
     listed = set()
     for block in re.findall(r"SET\s*\((.*?)\)", contents, re.DOTALL | re.IGNORECASE):
-        for token in block.split():
-            if "." in token or token == "LICENSE":
-                listed.add(token)
+        for word in block.split():
+            if "." in word or word == "LICENSE":
+                listed.add(word)
     return listed
 
 
@@ -91,7 +93,8 @@ def tracked_files_in(relative_directory):
     artifacts (cache archives, the CatalogCache and FreeCAD-macros trees, build
     output) from masquerading as un-registered source files."""
     prefix = "" if relative_directory == "." else relative_directory.replace(os.sep, "/") + "/"
-    output = subprocess.run(
+    # Audited: fixed git command, no shell, local path prefix (added nosec B603, B607)
+    output = subprocess.run(  # nosec B603 B607
         ["git", "ls-files", "-z", f"{prefix}*"],
         cwd=REPO_ROOT,
         capture_output=True,

AddonManagerTest/app/test_dependency_installer.py

@@ -21,7 +21,9 @@
 
 import functools
 import os
-import subprocess
+
+# Audited: used only for its types in test mocks; nothing is executed (added nosec B404)
+import subprocess  # nosec B404
 import tempfile
 from time import sleep
 import unittest

AddonManagerTest/app/test_python_deps.py

@@ -20,7 +20,9 @@
 ################################################################################
 
 import os
-import subprocess
+
+# Audited: used only for its types in test mocks; nothing is executed (added nosec B404)
+import subprocess  # nosec B404
 import tempfile
 import unittest
 from unittest.mock import MagicMock, patch

AddonManagerTest/app/test_uninstaller.py

@@ -163,6 +163,20 @@ def func(self, *args):
             _ = self.test_object.run()
             self.assertTrue(interceptor.called, "Failed to call uninstall script")
 
+    @patch("addonmanager_uninstaller.InstallationManifest")
+    def test_uninstall_skips_script_when_disabled(self, mock_install_manifest):
+        """Tests that run() does not call the uninstall.py script when it has been disabled"""
+
+        calls = []
+        with tempfile.TemporaryDirectory() as temp_dir:
+            toplevel_path = self.setup_dummy_installation(temp_dir)
+            self.test_object.run_uninstall_script = lambda *args: calls.append(args)
+            self.test_object.should_run_uninstall_script = False
+            _ = self.test_object.run()
+            self.assertFalse(calls, "Called uninstall script even though it was disabled")
+            self.assertFalse(os.path.exists(toplevel_path), "Failed to remove the addon")
+            self.assertIn("success", self.signals_caught)
+
     def test_remove_extra_files_no_digest(self):
         """Tests that a lack of digest file is not an error, and nothing gets removed"""
         with tempfile.TemporaryDirectory() as temp_dir:

AddonManagerTest/app/test_utilities.py

@@ -24,7 +24,10 @@
 import unittest
 from unittest.mock import MagicMock, patch, mock_open
 import os
-import subprocess
+
+# Audited: subprocess is used only for its types; its execution entry points are mocked
+# (added nosec B404)
+import subprocess  # nosec B404
 import sys
 
 from AddonManagerTest.app.mocks import MockAddon as Addon