From 9d3bc55c18088af685f4f4f79f62870e11ab44d5 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Thu, 12 Feb 2026 11:59:23 -0500 Subject: [PATCH] fix: post-merge monorepo hardening and cleanup (#239) * fix: address PR review nitpicks for monorepo hardening - Add docstring note about regex limitation in parse_json_with_trailing_commas - Abort on zip-slip in ZIP installer instead of skipping (consistent with API installer) - Use _safe_remove_directory for non-git plugin reinstall path - Use segment-wise encodeURIComponent for View button URL encoding Co-Authored-By: Claude Opus 4.6 * fix: check _safe_remove_directory result before reinstalling plugin Avoid calling install_plugin into a partially-removed directory by checking the boolean return of _safe_remove_directory, mirroring the guard already used in the git-remote migration path. Co-Authored-By: Claude Opus 4.6 * fix: normalize subpath prefix and add zip-slip guard to download installer - Strip trailing slashes from plugin_subpath before building the tree filter prefix, preventing double-slash ("subpath//") that would cause file_entries to silently miss all matches. - Add zip-slip protection to _install_via_download (extractall path), matching the guard already present in _install_from_monorepo_zip. Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Chuck Co-authored-by: Claude Opus 4.6 --- scripts/setup_plugin_repos.py | 6 +++++- src/plugin_system/store_manager.py | 23 +++++++++++++++++----- web_interface/static/v3/plugins_manager.js | 2 +- 3 files changed, 24 insertions(+), 7 deletions(-) diff --git a/scripts/setup_plugin_repos.py b/scripts/setup_plugin_repos.py index 07ec2d48..060637b9 100755 --- a/scripts/setup_plugin_repos.py +++ b/scripts/setup_plugin_repos.py @@ -18,7 +18,11 @@ MONOREPO_PLUGINS = PROJECT_ROOT.parent / "ledmatrix-plugins" / "plugins" def parse_json_with_trailing_commas(text: str) -> dict: - """Parse JSON that may have trailing commas.""" + """Parse JSON that may have trailing commas. + + Note: The regex also matches commas inside string values (e.g., "hello, }"). + This is fine for manifest files but may corrupt complex JSON with such patterns. + """ text = re.sub(r",\s*([}\]])", r"\1", text) return json.loads(text) diff --git a/src/plugin_system/store_manager.py b/src/plugin_system/store_manager.py index 1321233f..ac801f24 100644 --- a/src/plugin_system/store_manager.py +++ b/src/plugin_system/store_manager.py @@ -1235,7 +1235,7 @@ class PluginStoreManager: return False # Step 2: Filter for files in the target subdirectory - prefix = f"{plugin_subpath}/" + prefix = f"{plugin_subpath.strip('/')}/" file_entries = [ entry for entry in tree_data.get('tree', []) if entry['path'].startswith(prefix) and entry['type'] == 'blob' @@ -1348,9 +1348,10 @@ class PluginStoreManager: if not member_dest.is_relative_to(temp_extract_resolved): self.logger.error( f"Zip-slip detected: member {member!r} resolves outside " - f"temp directory, skipping" + f"temp directory, aborting" ) - continue + shutil.rmtree(temp_extract, ignore_errors=True) + return False zip_ref.extract(member, temp_extract) source_plugin_dir = temp_extract / root_dir / plugin_subpath @@ -1410,8 +1411,18 @@ class PluginStoreManager: # Find the root directory in the zip root_dir = zip_contents[0].split('/')[0] - # Extract to temp location + # Extract to temp location with zip-slip protection temp_extract = Path(tempfile.mkdtemp()) + temp_extract_resolved = temp_extract.resolve() + for member in zip_ref.namelist(): + member_dest = (temp_extract / member).resolve() + if not member_dest.is_relative_to(temp_extract_resolved): + self.logger.error( + f"Zip-slip detected: member {member!r} resolves outside " + f"temp directory, aborting" + ) + shutil.rmtree(temp_extract, ignore_errors=True) + return False zip_ref.extractall(temp_extract) # Move contents from root_dir to target @@ -2044,7 +2055,9 @@ class PluginStoreManager: self.logger.info(f"Plugin {plugin_id} not installed via git; re-installing latest archive") # Remove directory and reinstall fresh - shutil.rmtree(plugin_path, ignore_errors=True) + if not self._safe_remove_directory(plugin_path): + self.logger.error(f"Failed to remove old plugin directory for {plugin_id}") + return False return self.install_plugin(plugin_id) except Exception as e: diff --git a/web_interface/static/v3/plugins_manager.js b/web_interface/static/v3/plugins_manager.js index 2ea8a874..0c693cf5 100644 --- a/web_interface/static/v3/plugins_manager.js +++ b/web_interface/static/v3/plugins_manager.js @@ -5289,7 +5289,7 @@ function renderPluginStore(plugins) { -