diff --git a/Addon.py b/Addon.py index cda23a2d..786c118a 100644 --- a/Addon.py +++ b/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" ) diff --git a/AddonCatalog.py b/AddonCatalog.py index a2138e76..fe960a1a 100644 --- a/AddonCatalog.py +++ b/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) diff --git a/AddonCatalogCacheCreator.py b/AddonCatalogCacheCreator.py index be8b8bb7..36e0bed2 100644 --- a/AddonCatalogCacheCreator.py +++ b/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) diff --git a/AddonManagerTest/app/test_addon.py b/AddonManagerTest/app/test_addon.py index a3006c03..7ccc1f87 100644 --- a/AddonManagerTest/app/test_addon.py +++ b/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 = ( + '\n' + ']>\n' + '&payload;\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 "] diff --git a/AddonManagerTest/app/test_cmake_file_lists.py b/AddonManagerTest/app/test_cmake_file_lists.py index 22add35e..4fa18ce6 100644 --- a/AddonManagerTest/app/test_cmake_file_lists.py +++ b/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, diff --git a/AddonManagerTest/app/test_dependency_installer.py b/AddonManagerTest/app/test_dependency_installer.py index 9786d601..0d7db259 100644 --- a/AddonManagerTest/app/test_dependency_installer.py +++ b/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 diff --git a/AddonManagerTest/app/test_python_deps.py b/AddonManagerTest/app/test_python_deps.py index 0aa197ed..993c216f 100644 --- a/AddonManagerTest/app/test_python_deps.py +++ b/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 diff --git a/AddonManagerTest/app/test_uninstaller.py b/AddonManagerTest/app/test_uninstaller.py index 995009b6..479a1242 100644 --- a/AddonManagerTest/app/test_uninstaller.py +++ b/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: diff --git a/AddonManagerTest/app/test_utilities.py b/AddonManagerTest/app/test_utilities.py index a18d8940..0eb2b6e2 100644 --- a/AddonManagerTest/app/test_utilities.py +++ b/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 diff --git a/AddonManagerTest/gui/test_uninstaller_gui.py b/AddonManagerTest/gui/test_uninstaller_gui.py index ea9e440d..d3b0d2e1 100644 --- a/AddonManagerTest/gui/test_uninstaller_gui.py +++ b/AddonManagerTest/gui/test_uninstaller_gui.py @@ -20,6 +20,8 @@ ################################################################################ import functools +import os +import tempfile import unittest from unittest.mock import MagicMock, patch @@ -41,7 +43,7 @@ ) from AddonManagerTest.app.mocks import MockAddon, MockMacro -from addonmanager_uninstaller_gui import AddonUninstallerGUI +from addonmanager_uninstaller_gui import AddonUninstallerGUI, UninstallScriptDialog translate = fci.translate @@ -175,3 +177,103 @@ def test_finalize(self): self.uninstaller_gui.worker_thread = MockThread() self.uninstaller_gui._finalize() self.assertIn("finished", self.signals_caught) + + def setup_installation_with_script(self, temp_dir) -> str: + """Create a fake installed addon whose uninstall.py writes a marker file when run. + Returns the path of the marker file.""" + addon_path = os.path.join(temp_dir, self.addon_to_remove.name) + os.makedirs(addon_path) + marker_file = os.path.join(temp_dir, "RAN_UNINSTALL_SCRIPT.txt") + double_escaped = marker_file.replace("\\", "\\\\") + with open(os.path.join(addon_path, "uninstall.py"), "w", encoding="utf-8") as f: + f.write(f"""# Mock uninstall script +with open('{double_escaped}', "w", encoding="utf-8") as f: + f.write("File created by uninstall.py from unit tests") +""") + self.uninstaller_gui.uninstaller.installation_path = temp_dir + return marker_file + + def test_uninstall_script_no_prompt_without_script(self): + with tempfile.TemporaryDirectory() as temp_dir: + os.makedirs(os.path.join(temp_dir, self.addon_to_remove.name)) + self.uninstaller_gui.uninstaller.installation_path = temp_dir + with patch("addonmanager_uninstaller_gui.UninstallScriptDialog") as mock_dialog: + self.uninstaller_gui._handle_uninstall_script() + mock_dialog.assert_not_called() + self.assertFalse(self.uninstaller_gui.uninstaller.should_run_uninstall_script) + + def test_uninstall_script_no_prompt_for_macro(self): + macro_addon = MockAddon() + macro_addon.macro = MockMacro() + uninstaller_gui = AddonUninstallerGUI(macro_addon) + with patch("addonmanager_uninstaller_gui.UninstallScriptDialog") as mock_dialog: + uninstaller_gui._handle_uninstall_script() + mock_dialog.assert_not_called() + + def test_uninstall_script_prompt_declined(self): + with tempfile.TemporaryDirectory() as temp_dir: + marker_file = self.setup_installation_with_script(temp_dir) + dialog_watcher = DialogWatcher( + "AddonManager_RunUninstallScriptDialog", QtWidgets.QDialogButtonBox.No + ) + self.uninstaller_gui._handle_uninstall_script() + self.assertTrue(dialog_watcher.dialog_found, "Failed to find the expected dialog box") + self.assertTrue(dialog_watcher.button_found, "Failed to find the expected button") + self.assertFalse( + os.path.exists(marker_file), "Ran the uninstall script without permission" + ) + self.assertFalse(self.uninstaller_gui.uninstaller.should_run_uninstall_script) + + def test_uninstall_script_prompt_accepted(self): + with tempfile.TemporaryDirectory() as temp_dir: + marker_file = self.setup_installation_with_script(temp_dir) + dialog_watcher = DialogWatcher( + "AddonManager_RunUninstallScriptDialog", QtWidgets.QDialogButtonBox.Yes + ) + self.uninstaller_gui._handle_uninstall_script() + self.assertTrue(dialog_watcher.dialog_found, "Failed to find the expected dialog box") + self.assertTrue(dialog_watcher.button_found, "Failed to find the expected button") + self.assertTrue( + os.path.exists(marker_file), "Failed to run the approved uninstall script" + ) + self.assertFalse(self.uninstaller_gui.uninstaller.should_run_uninstall_script) + + +class TestUninstallScriptDialog(unittest.TestCase): + + MODULE = "test_uninstaller_gui" # file name without extension + + def setUp(self): + self.dialog = UninstallScriptDialog("Mock Addon", os.path.join("path", "uninstall.py")) + + def tearDown(self): + self.dialog.close() + del self.dialog # Immediately destroy the widget so no top-level window leaks + + def test_run_button_requests_run(self): + dialog_watcher = DialogWatcher( + "AddonManager_RunUninstallScriptDialog", QtWidgets.QDialogButtonBox.Yes + ) + self.dialog.exec() + self.assertTrue(dialog_watcher.dialog_found, "Failed to find the expected dialog box") + self.assertTrue(dialog_watcher.button_found, "Failed to find the expected button") + self.assertTrue(self.dialog.run_requested) + + def test_do_not_run_button_does_not_request_run(self): + dialog_watcher = DialogWatcher( + "AddonManager_RunUninstallScriptDialog", QtWidgets.QDialogButtonBox.No + ) + self.dialog.exec() + self.assertTrue(dialog_watcher.dialog_found, "Failed to find the expected dialog box") + self.assertTrue(dialog_watcher.button_found, "Failed to find the expected button") + self.assertFalse(self.dialog.run_requested) + + def test_open_button_opens_editor_without_closing_dialog(self): + with patch("addonmanager_uninstaller_gui.open_file_in_text_editor") as mock_open: + self.dialog.show() + self.dialog.open_button.click() + self.assertTrue(self.dialog.isVisible(), "Open button should not close the dialog") + self.dialog.skip_button.click() + self.assertFalse(self.dialog.isVisible()) + mock_open.assert_called_once_with(self.dialog.script_path) + self.assertFalse(self.dialog.run_requested) diff --git a/Resources/translations/run_translation_cycle.py b/Resources/translations/run_translation_cycle.py index f623761a..d2e5a1e3 100644 --- a/Resources/translations/run_translation_cycle.py +++ b/Resources/translations/run_translation_cycle.py @@ -30,7 +30,10 @@ import os import shutil import stat -import subprocess + +# Audited: fixed lrelease/lupdate commands operating on local repository files, no shell +# (added nosec B404) +import subprocess # nosec B404 import sys import tempfile import time @@ -88,8 +91,9 @@ def _make_api_req(self, url, extra_headers=None, method="GET", data=None): parsed_url = urlparse(url) if parsed_url.scheme != "https": raise Exception("API requests must be made over HTTPS") - request = Request(url, headers=headers, method=method, data=data) - request_result = urlopen(request) + # Audited: this code does not accept non-HTTPS URL schemes (added nosec B310) + request = Request(url, headers=headers, method=method, data=data) # nosec B310 + request_result = urlopen(request) # nosec B310 if request_result.getcode() >= 300: print(f"Failed to make API request {url}: return code {request_result.getcode()}") raise Exception("Failed to make API request") @@ -144,7 +148,8 @@ def download(self, build_id): if parsed_url.scheme != "https": raise Exception("API requests must be made over HTTPS") - urlretrieve(response["url"], filename) + # Audited: this code does not accept non-HTTPS URL schemes (added nosec B310) + urlretrieve(response["url"], filename) # nosec B310 print("download of " + filename + " complete") def build(self): @@ -194,7 +199,7 @@ def process_single_translation_file(source_path: str, target_path: str): print("Generating qm file for", basename, "...") try: - subprocess.run( + subprocess.run( # nosec B603 B607 [ "lrelease", new_path, @@ -357,7 +362,7 @@ def run_and_download_build(crowdin_updater: CrowdinUpdater): "-ts", os.path.join(TS_FILE_PATH, CROWDIN_FILE_NAME), ] - result = subprocess.run( + result = subprocess.run( # nosec B603 args, timeout=30, check=True, diff --git a/addonmanager_dependency_installer.py b/addonmanager_dependency_installer.py index 288cdba5..da892fad 100644 --- a/addonmanager_dependency_installer.py +++ b/addonmanager_dependency_installer.py @@ -22,7 +22,10 @@ """Class to manage installation of sets of Python dependencies.""" import os -import subprocess + +# Audited: subprocess is used only for its types; commands run through the audited wrappers +# in addonmanager_utilities (added nosec B404) +import subprocess # nosec B404 from typing import List import addonmanager_freecad_interface as fci diff --git a/addonmanager_git.py b/addonmanager_git.py index 61593cc5..faeb74d1 100644 --- a/addonmanager_git.py +++ b/addonmanager_git.py @@ -26,7 +26,9 @@ import os import platform import shutil -import subprocess + +# Audited: subprocess calls use fixed argument lists and no shell (added nosec B404) +import subprocess # nosec B404 from typing import Callable, List, Dict, Optional import time @@ -466,8 +468,11 @@ def _git_is_real() -> bool: on the Mac actually requires us to check for that installation.""" try: # Get the path to git from xcrun + # Audited: fixed arguments, no shell, no untrusted input (added nosec B603, B607) git_path = ( - subprocess.check_output(["xcrun", "--find", "git"], stderr=subprocess.DEVNULL) + subprocess.check_output( # nosec B603 B607 + ["xcrun", "--find", "git"], stderr=subprocess.DEVNULL + ) .decode() .strip() ) @@ -475,7 +480,8 @@ def _git_is_real() -> bool: return False # Check if running git triggers version output - result = subprocess.run( + # Audited: runs the git that xcrun reported, fixed arguments, no shell (added nosec B603) + result = subprocess.run( # nosec B603 [git_path, "--version"], stdout=subprocess.PIPE, stderr=subprocess.DEVNULL ) return result.returncode == 0 diff --git a/addonmanager_python_deps.py b/addonmanager_python_deps.py index fbe45b05..d6bf800c 100644 --- a/addonmanager_python_deps.py +++ b/addonmanager_python_deps.py @@ -27,7 +27,10 @@ import os import re import shutil -import subprocess + +# Audited: subprocess is used only for its CalledProcessError exception type; commands run +# through the audited wrappers in addonmanager_utilities (added nosec B404) +import subprocess # nosec B404 from typing import Callable, Dict, Iterable, List, TypedDict, Optional, Set from enum import Enum from addonmanager_metadata import Version diff --git a/addonmanager_uninstaller.py b/addonmanager_uninstaller.py index ba6ab012..89ae3d87 100644 --- a/addonmanager_uninstaller.py +++ b/addonmanager_uninstaller.py @@ -84,6 +84,10 @@ class AddonUninstaller(QtCore.QObject): uninstaller = AddonInstaller(addon_to_remove) uninstaller.run() + If the addon provides an "uninstall.py" script it is executed as part of run() by + default. Set should_run_uninstall_script to False before calling run() to skip the + script (the GUI wrapper does this, asking the user for permission first and running + the script itself only when the user approves). """ # Signals: success and failure Emitted when the installation process is complete. @@ -101,6 +105,7 @@ def __init__(self, addon: Addon): self.addon_to_remove = addon self.installation_path = fci.DataPaths().mod_dir self.macro_installation_path = fci.DataPaths().macro_dir + self.should_run_uninstall_script = True def run(self) -> bool: """Remove an addon. Returns True if the addon was removed cleanly, or False @@ -116,7 +121,8 @@ def run(self) -> bool: path_to_remove, self.installation_path ): try: - self.run_uninstall_script(path_to_remove) + if self.should_run_uninstall_script: + self.run_uninstall_script(path_to_remove) self.remove_extra_files(path_to_remove) success = utils.rmdir(path_to_remove) if ( @@ -149,7 +155,8 @@ def run_uninstall_script(path_to_remove): # pylint: disable=broad-exception-caught try: with open(uninstall_script, encoding="utf-8") as f: - exec(f.read()) + # This use of exec() is behind an explicit user opt-in dialog (added nosec B102) + exec(f.read()) # nosec B102 except Exception: fci.Console.PrintError( translate( diff --git a/addonmanager_uninstaller_gui.py b/addonmanager_uninstaller_gui.py index 31421d60..d41bff75 100644 --- a/addonmanager_uninstaller_gui.py +++ b/addonmanager_uninstaller_gui.py @@ -21,6 +21,12 @@ """GUI functions for uninstalling an Addon or Macro.""" +import os +import platform + +# Audited: subprocess calls use fixed argument lists and no shell (added nosec B404) +import subprocess # nosec B404 + import addonmanager_freecad_interface as fci from Widgets.addonmanager_utility_dialogs import MessageDialog @@ -40,6 +46,68 @@ translate = fci.translate +def open_file_in_text_editor(path: str) -> None: + """Open the given file in a text editor chosen by the operating system. Deliberately avoids + QDesktopServices.openUrl, because on some platforms the default action for a Python file is + to execute it rather than to display it.""" + # Audited: fixed editor commands with the file path passed as an argument, never to a shell + # (added nosec B606, B603, B607) + system = platform.system() + if system == "Windows": + try: + os.startfile(path, "edit") # nosec B606 + except OSError: + subprocess.Popen(["notepad.exe", path]) # nosec B603 B607 + elif system == "Darwin": + subprocess.Popen(["open", "-t", path]) # nosec B603 B607 + else: + subprocess.Popen(["xdg-open", path]) # nosec B603 B607 + + +class UninstallScriptDialog(QtWidgets.QDialog): + """Asks the user whether the addon's uninstall script should be run. Offers to open the + script in a text editor so it can be reviewed before deciding. The default action is to not + run the script. After the dialog closes, run_requested is True only if the user explicitly + chose to run the script.""" + + def __init__(self, addon_display_name: str, script_path: str, parent=None): + super().__init__(parent) + self.script_path = script_path + self.run_requested = False + self.setObjectName("AddonManager_RunUninstallScriptDialog") + self.setWindowTitle(translate("AddonsInstaller", "Run Uninstall Script?")) + layout = QtWidgets.QVBoxLayout(self) + message = translate( + "AddonsInstaller", + "{} includes an uninstall script, intended to let the addon clean up after itself " + "when it is removed (for example, by removing its saved preferences). The script is " + "provided by the addon itself, not by the Addon Manager, and can run arbitrary code: " + "you may review it before deciding whether to run it.", + ).format(addon_display_name) + label = QtWidgets.QLabel(message) + label.setWordWrap(True) + layout.addWidget(label) + self.button_box = QtWidgets.QDialogButtonBox() + self.open_button = self.button_box.addButton(QtWidgets.QDialogButtonBox.Open) + self.open_button.setText(translate("AddonsInstaller", "Open Script in Editor…")) + self.run_button = self.button_box.addButton(QtWidgets.QDialogButtonBox.Yes) + self.run_button.setText(translate("AddonsInstaller", "Run Script")) + self.skip_button = self.button_box.addButton(QtWidgets.QDialogButtonBox.No) + self.skip_button.setText(translate("AddonsInstaller", "Do Not Run")) + self.skip_button.setDefault(True) + self.open_button.clicked.connect(self._open_script_in_editor) + self.run_button.clicked.connect(self._run_clicked) + self.skip_button.clicked.connect(self.reject) + layout.addWidget(self.button_box) + + def _run_clicked(self): + self.run_requested = True + self.accept() + + def _open_script_in_editor(self): + open_file_in_text_editor(self.script_path) + + class AddonUninstallerGUI(QtCore.QObject): """User interface for uninstalling an Addon: asks for confirmation, displays a progress dialog, displays completion and/or error dialogs, and emits the finished() signal when all work is @@ -75,6 +143,7 @@ def run(self): self._finalize() return + self._handle_uninstall_script() self.dialog_timer.start() self._run_uninstaller() @@ -92,6 +161,25 @@ def _confirm_uninstallation(self) -> bool: ) return confirm == QtWidgets.QMessageBox.Yes + def _handle_uninstall_script(self): + """If the addon provides an uninstall script, ask the user whether to run it. When the + user approves, the script is run here, on the main GUI thread, so that scripts that show + dialogs of their own work as expected. In either case the core uninstaller is told not to + run the script itself.""" + if not isinstance(self.uninstaller, AddonUninstaller): + return + self.uninstaller.should_run_uninstall_script = False + addon_path = os.path.join(self.uninstaller.installation_path, self.addon_to_remove.name) + script_path = os.path.join(addon_path, "uninstall.py") + if not os.path.isfile(script_path): + return + dialog = UninstallScriptDialog( + self.addon_to_remove.display_name, script_path, parent=utils.get_main_am_window() + ) + dialog.exec() + if dialog.run_requested: + AddonUninstaller.run_uninstall_script(addon_path) + def _show_progress_dialog(self): self.progress_dialog = QtWidgets.QMessageBox( QtWidgets.QMessageBox.NoIcon, diff --git a/addonmanager_utilities.py b/addonmanager_utilities.py index 53dde117..7d739c66 100644 --- a/addonmanager_utilities.py +++ b/addonmanager_utilities.py @@ -33,7 +33,10 @@ import queue import shutil import stat -import subprocess + +# Audited: the subprocess wrappers below run argument-list commands with no shell; executables +# are resolved locally and caller data appears only as arguments (added nosec B404) +import subprocess # nosec B404 import sys import threading import time @@ -671,12 +674,14 @@ def blocking_get(url: str, method=None) -> bytes: if hasattr(p, "data"): p = p.data() elif requests and method is None or method == "requests": - response = requests.get(url, timeout=10.0) + # Audited: this code does not accept non-HTTPS URL schemes (added nosec B310) + response = requests.get(url, timeout=10.0) # nosec B310 if response.status_code == 200: p = response.content else: ctx = ssl.create_default_context() - with urllib.request.urlopen(url, context=ctx) as f: + # Audited: this code does not accept non-HTTPS URL schemes (added nosec B310) + with urllib.request.urlopen(url, context=ctx) as f: # nosec B310 p = f.read() return p @@ -691,7 +696,10 @@ def run_interruptable_subprocess( # Added in Python 3.7 -- only used on Windows creation_flags = subprocess.CREATE_NO_WINDOW try: - p = subprocess.Popen( + # Audited: args is an argument list run with no shell; callers pass locally-resolved + # executables (git, pip) with untrusted data only ever appearing as arguments + # (added nosec B603) + p = subprocess.Popen( # nosec B603 args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, @@ -743,7 +751,8 @@ def run_monitored_subprocess( if hasattr(subprocess, "CREATE_NO_WINDOW"): creation_flags = subprocess.CREATE_NO_WINDOW try: - process = subprocess.Popen( + # Audited: args is an argument list run with no shell, as above (added nosec B603) + process = subprocess.Popen( # nosec B603 args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, @@ -820,7 +829,8 @@ def _kill_process_tree(process: subprocess.Popen) -> None: git clone: they go on downloading, and they keep its output pipe open.""" if sys.platform == "win32": try: - subprocess.run( + # Audited: fixed system command with a numeric PID argument (added nosec B603, B607) + subprocess.run( # nosec B603 B607 ["taskkill", "/F", "/T", "/PID", str(process.pid)], capture_output=True, check=False, diff --git a/addonmanager_workers_startup.py b/addonmanager_workers_startup.py index 3934b6cf..b312c4a3 100644 --- a/addonmanager_workers_startup.py +++ b/addonmanager_workers_startup.py @@ -29,7 +29,12 @@ import os from types import SimpleNamespace from typing import List, Optional, Tuple -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 from PySideWrapper import QtCore @@ -257,7 +262,7 @@ def _serves_package_xml(self, url: str) -> bool: return False try: MetadataReader.from_bytes(data) - except (XmlParseError, RuntimeError): + except (XmlParseError, DefusedXmlException, RuntimeError): return False return True @@ -282,7 +287,7 @@ def _collect_addon_metadata(cls, name: str, get_file) -> Optional[CatalogEntryMe if package_xml: try: parsed_metadata = MetadataReader.from_bytes(package_xml) - except (XmlParseError, RuntimeError) as e: + except (XmlParseError, DefusedXmlException, RuntimeError) as e: parsed_metadata = None fci.Console.PrintWarning( translate(