From b89653c836ad3c9efb330b76fb2097e4fb8506ff Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Fri, 10 Jul 2026 15:17:25 -0400 Subject: [PATCH] fix(security): replace basename-only sanitiser with a trusted-enumeration check The previous commit's os.path.basename() + os.path.join() pattern (which a pre-existing code comment claimed CodeQL recognises as a sanitiser) did not actually clear the alert -- the next CodeQL run still flagged the same 2 sink lines, plus a new one at the os.path.join() call itself. Taking a substring of tainted data apparently isn't treated as a barrier by this query, whatever the comment assumed. Replaced it with find_trusted_subdir(): enumerate the trusted plugins_dir via os.scandir() and only use a name that scandir itself produced, matched by equality against the caller's requested name. The path is then built from that enumerated entry, not from the caller's string -- a value sourced from iterating a trusted, non-tainted directory carries no taint regardless of what it happens to equal, which is a stronger and more conventional allowlist-style barrier than string-stripping. Applied identically in both PluginLoader.install_dependencies() and StoreManager._install_dependencies(), sharing one implementation. Re-verified: all 65 tests across test_plugin_loader.py (20, including the 2 new security regression tests), test_store_manager_caches.py (35), test_plugin_system.py (10) pass, plus the full CI plugin-safety suite (test_harness.py/test_visual_rendering.py/test_plugin_matrix.py: 52 passed, 2 pre-existing skips). --- src/plugin_system/plugin_loader.py | 50 +++++++++++++++++++++--------- src/plugin_system/store_manager.py | 32 +++++++++---------- 2 files changed, 51 insertions(+), 31 deletions(-) diff --git a/src/plugin_system/plugin_loader.py b/src/plugin_system/plugin_loader.py index 951eb63d..e5ac854b 100644 --- a/src/plugin_system/plugin_loader.py +++ b/src/plugin_system/plugin_loader.py @@ -93,6 +93,27 @@ def requirements_are_satisfied(requirements_file: str) -> bool: return True +def find_trusted_subdir(trusted_dir: str, name: str) -> Optional[str]: + """Return `name` if it names an actual subdirectory of trusted_dir, else None. + + Used as a containment check for a directory name derived from untrusted + input (a manifest-declared plugin id, an externally-supplied plugin + path): the returned value always comes from enumerating trusted_dir + itself via os.scandir(), so a caller that builds a path by joining + trusted_dir with this return value is joining against a name the + filesystem produced under a trusted root -- not the caller's original + string, which could otherwise smuggle a traversal sequence through. + """ + try: + with os.scandir(trusted_dir) as entries: + for entry in entries: + if entry.name == name and entry.is_dir(): + return entry.name + except OSError: + pass + return None + + class PluginLoader: """Handles plugin module loading and class instantiation.""" @@ -200,9 +221,9 @@ class PluginLoader: except (json.JSONDecodeError, Exception) as e: self.logger.debug("Skipping %s due to manifest error: %s", item.name, e) continue - + return None - + def install_dependencies( self, plugin_dir: Path, @@ -233,25 +254,24 @@ class PluginLoader: # Resolve to a canonical absolute path (normalises .. and symlinks) plugin_dir_real = os.path.realpath(str(plugin_dir)) - - # Reconstruct the plugin path from a trusted base + a sanitised - # directory name. os.path.basename() is CodeQL's recognised - # py/path-injection sanitiser: it strips all directory components - # so the result cannot contain traversal sequences. Joining it - # with the resolved, trusted plugins_dir produces a path that - # CodeQL considers untainted. plugins_dir_real = os.path.realpath(str(plugins_dir)) - safe_dir_name = os.path.basename(plugin_dir_real) - if not safe_dir_name: - self.logger.error("Could not determine plugin directory name for %s", plugin_id) - return False - safe_plugin_dir = os.path.join(plugins_dir_real, safe_dir_name) - if not os.path.isdir(safe_plugin_dir): + requested_name = os.path.basename(plugin_dir_real) + + # Match the requested directory against an entry actually enumerated + # from the trusted plugins_dir, and build the path from that entry -- + # not from requested_name. A name that came out of os.scandir() on a + # trusted root carries no taint regardless of what the caller asked + # for, so this is a real containment guarantee (an allowlist check + # against a trusted source), not a string-sanitisation of untrusted + # input that a static analyzer has to trust blindly. + matched_name = find_trusted_subdir(plugins_dir_real, requested_name) + if matched_name is None: self.logger.error( "Plugin directory for %s not found inside plugins dir", plugin_id ) return False + safe_plugin_dir = os.path.join(plugins_dir_real, matched_name) requirements_file = os.path.join(safe_plugin_dir, "requirements.txt") if not os.path.isfile(requirements_file): diff --git a/src/plugin_system/store_manager.py b/src/plugin_system/store_manager.py index 863afa76..059a1343 100644 --- a/src/plugin_system/store_manager.py +++ b/src/plugin_system/store_manager.py @@ -25,7 +25,9 @@ import logging from urllib.parse import urlparse from src.common.permission_utils import sudo_remove_directory, install_requirements_file -from src.plugin_system.plugin_loader import requirements_has_real_deps, requirements_are_satisfied +from src.plugin_system.plugin_loader import ( + requirements_has_real_deps, requirements_are_satisfied, find_trusted_subdir +) try: from jsonschema import Draft7Validator, ValidationError @@ -1908,24 +1910,22 @@ class PluginStoreManager: True if successful or no requirements file """ # Reconstruct the plugin path from the trusted self.plugins_dir base + - # a sanitised directory name rather than trusting plugin_path directly - # -- callers ultimately derive it from a plugin-supplied manifest "id" - # field (see install_plugin_from_url), so without this a malicious - # manifest could point requirements_file outside plugins_dir. - # os.path.basename() is CodeQL's recognised py/path-injection - # sanitiser: it strips all directory components so the result cannot - # contain traversal sequences, matching the pattern already used in - # PluginLoader.install_dependencies(). + # an entry actually enumerated from it, rather than trusting + # plugin_path directly -- callers ultimately derive it from a + # plugin-supplied manifest "id" field (see install_plugin_from_url), + # so without this a malicious manifest could point requirements_file + # outside plugins_dir. find_trusted_subdir()'s return value always + # comes from os.scandir() on the trusted root, so building the path + # from it (not from the caller's string) is a real containment + # guarantee, matching the pattern in PluginLoader.install_dependencies(). plugin_dir_real = os.path.realpath(str(plugin_path)) plugins_dir_real = os.path.realpath(str(self.plugins_dir)) - safe_dir_name = os.path.basename(plugin_dir_real) - if not safe_dir_name: - self.logger.error("Could not determine plugin directory name for dependency install") - return False - safe_plugin_path = Path(os.path.join(plugins_dir_real, safe_dir_name)) - if not safe_plugin_path.is_dir(): - self.logger.error("Plugin directory not found inside plugins dir: %s", safe_plugin_path) + requested_name = os.path.basename(plugin_dir_real) + matched_name = find_trusted_subdir(plugins_dir_real, requested_name) + if matched_name is None: + self.logger.error("Plugin directory not found inside plugins dir for dependency install") return False + safe_plugin_path = Path(os.path.join(plugins_dir_real, matched_name)) requirements_file = safe_plugin_path / "requirements.txt"