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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

---------

Co-authored-by: Chuck <chuck@example.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

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)
 

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:

web_interface/static/v3/plugins_manager.js

@@ -5289,7 +5289,7 @@ function renderPluginStore(plugins) {
                     <button onclick='if(window.installPlugin){const branchInput = document.getElementById("branch-input-${plugin.id.replace(/[^a-zA-Z0-9]/g, '-')}"); window.installPlugin(${escapeJs(plugin.id)}, branchInput?.value?.trim() || null)}else{console.error("installPlugin not available")}' class="btn bg-green-600 hover:bg-green-700 text-white px-4 py-2 rounded-md text-sm flex-1 font-semibold">
                         <i class="fas fa-download mr-2"></i>Install
                     </button>
-                    <button onclick='${plugin.repo ? `window.open(${escapeJs(plugin.plugin_path ? plugin.repo + "/tree/" + (plugin.default_branch || plugin.branch || "main") + "/" + encodeURI(plugin.plugin_path) : plugin.repo)}, "_blank")` : `void(0)`}' ${plugin.repo ? '' : 'disabled'} class="btn bg-gray-600 hover:bg-gray-700 text-white px-4 py-2 rounded-md text-sm flex-1 font-semibold${plugin.repo ? '' : ' opacity-50 cursor-not-allowed'}">
+                    <button onclick='${plugin.repo ? `window.open(${escapeJs(plugin.plugin_path ? plugin.repo + "/tree/" + encodeURIComponent(plugin.default_branch || plugin.branch || "main") + "/" + plugin.plugin_path.split("/").map(encodeURIComponent).join("/") : plugin.repo)}, "_blank")` : `void(0)`}' ${plugin.repo ? '' : 'disabled'} class="btn bg-gray-600 hover:bg-gray-700 text-white px-4 py-2 rounded-md text-sm flex-1 font-semibold${plugin.repo ? '' : ' opacity-50 cursor-not-allowed'}">
                         <i class="fas fa-external-link-alt mr-2"></i>View
                     </button>
                 </div>