From 15cc6926a4d2affd617222cb2c353a460c954b3b Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Thu, 17 Sep 2026 08:48:21 -0400 Subject: [PATCH 1/3] fix(web): keep plugin asset and action routes inside their directories POST /plugins/assets/upload, GET /plugins/assets/list and POST /plugins/assets/delete joined the request's plugin_id onto assets/plugins unchecked, so '../../config' created, wrote, listed and deleted outside it. #561 guarded only the route that serves the files. All three now go through path_safety.resolve_under and answer 400 for anything but a plain name, and delete only unlinks a metadata path that resolves into that plugin's uploads directory. PluginManager.get_plugin_directory refuses ids that are not one plain path segment, so /plugins/action (which runs a manifest script from the returned directory) and every other caller get the guard; the action route also rejects such ids up front, covering its no-manager fallback. Co-Authored-By: Claude Opus 5 --- src/plugin_system/plugin_manager.py | 12 +- test/test_path_traversal_guards.py | 199 +++++++++++++++++++++ web_interface/blueprints/api_v3/plugins.py | 46 ++++- 3 files changed, 249 insertions(+), 8 deletions(-) diff --git a/src/plugin_system/plugin_manager.py b/src/plugin_system/plugin_manager.py index c587acc8..a79ec867 100644 --- a/src/plugin_system/plugin_manager.py +++ b/src/plugin_system/plugin_manager.py @@ -23,6 +23,7 @@ from src.plugin_system.plugin_loader import PluginLoader from src.plugin_system.plugin_executor import PluginExecutor from src.plugin_system.plugin_state import PluginStateManager, PluginState from src.plugin_system.schema_manager import SchemaManager, normalize_legacy_booleans +from src.common.path_safety import safe_path_component from src.common.permission_utils import ( ensure_directory_permissions, get_plugin_dir_mode @@ -743,11 +744,20 @@ class PluginManager: Returns: Directory path as string or None if not found + + ``plugin_id`` often comes straight from a request, so anything that is + not one plain path segment (``..``, ``a/b``, an absolute path) is + refused instead of being joined onto ``plugins_dir``. The join is not + resolved further: dev plugins are symlinks into ``plugins_dir``. """ with self._discovery_lock: if hasattr(self, 'plugin_directories') and plugin_id in self.plugin_directories: return str(self.plugin_directories[plugin_id]) - + + plugin_id = safe_path_component(plugin_id) + if plugin_id is None: + return None + plugin_dir = self.plugins_dir / plugin_id if plugin_dir.exists(): return str(plugin_dir) diff --git a/test/test_path_traversal_guards.py b/test/test_path_traversal_guards.py index 9aaa04e4..bce107cc 100644 --- a/test/test_path_traversal_guards.py +++ b/test/test_path_traversal_guards.py @@ -315,3 +315,202 @@ class TestPluginVersionLookup: assert module._get_plugin_version("..") == "" finally: module.api_v3.plugin_store_manager = original + + +PNG = b"\x89PNG\r\n\x1a\n" + b"0" * 20 + + +class TestPluginAssetRoutes: + """/api/v3/plugins/assets/upload, /list and /delete + + All three joined a request-supplied plugin_id straight onto + assets/plugins, so '../../config' created an uploads directory under + config/, wrote images and .metadata.json there, listed it, and deleted + whatever file a metadata entry's path named. #561 fixed only the route + that serves the uploaded files. + """ + + @pytest.fixture + def project(self, tmp_path, api_v3_module, monkeypatch): + import web_interface.blueprints.api_v3.plugins as plugins_module + + monkeypatch.setattr(plugins_module, "PROJECT_ROOT", tmp_path) + (tmp_path / "config").mkdir() + secrets = tmp_path / "config" / "config_secrets.json" + secrets.write_text('{"api_key":"hunter2"}', encoding="utf-8") + return tmp_path, secrets + + def _upload(self, client, plugin_id): + import io + + return client.post( + "/api/v3/plugins/assets/upload", + data={"plugin_id": plugin_id, "files": (io.BytesIO(PNG), "x.png")}, + content_type="multipart/form-data", + ) + + def _tree(self, root): + return sorted(str(p.relative_to(root)) for p in root.rglob("*")) + + def test_an_ordinary_upload_list_and_delete_still_work(self, api_v3_client, project): + root, _ = project + response = self._upload(api_v3_client, "static-image") + assert response.status_code == 200, response.get_json() + uploaded = response.get_json()["uploaded_files"][0] + assert uploaded["path"].startswith("assets/plugins/static-image/uploads/") + stored = root / uploaded["path"] + assert stored.exists() + + listed = api_v3_client.get( + "/api/v3/plugins/assets/list", query_string={"plugin_id": "static-image"} + ) + assert listed.status_code == 200 + assert [a["id"] for a in listed.get_json()["data"]["assets"]] == [uploaded["id"]] + + deleted = api_v3_client.post( + "/api/v3/plugins/assets/delete", + json={"plugin_id": "static-image", "image_id": uploaded["id"]}, + ) + assert deleted.status_code == 200 + assert not stored.exists() + + @pytest.mark.parametrize("plugin_id", [ + "../../config", "..", "a/b", "/abs", "x" + BACKSLASH + "y", "C:", + ]) + def test_a_traversing_upload_writes_nothing(self, api_v3_client, project, plugin_id): + root, _ = project + before = self._tree(root) + response = self._upload(api_v3_client, plugin_id) + assert response.status_code == 400 + assert self._tree(root) == before + + def test_a_traversing_list_reads_nothing(self, api_v3_client, project): + root, _ = project + outside = root / "config" / "uploads" + outside.mkdir() + (outside / ".metadata.json").write_text( + json.dumps({"i": {"id": "i", "path": "leaked"}}), encoding="utf-8" + ) + response = api_v3_client.get( + "/api/v3/plugins/assets/list", query_string={"plugin_id": "../../config"} + ) + assert response.status_code == 400 + assert b"leaked" not in response.data + + def test_a_traversing_delete_deletes_nothing(self, api_v3_client, project): + root, secrets = project + outside = root / "config" / "uploads" + outside.mkdir() + (outside / ".metadata.json").write_text( + json.dumps({"i": {"id": "i", "path": "config/config_secrets.json"}}), + encoding="utf-8", + ) + response = api_v3_client.post( + "/api/v3/plugins/assets/delete", + json={"plugin_id": "../../config", "image_id": "i"}, + ) + assert response.status_code == 400 + assert secrets.exists(), "file outside assets/plugins was deleted" + + @pytest.mark.parametrize("stored_path", [ + "config/config_secrets.json", + "assets/plugins/static-image/uploads/../../../../config/config_secrets.json", + "assets/plugins/other/uploads/x.png", + None, + ]) + def test_a_metadata_path_outside_the_uploads_is_never_unlinked( + self, api_v3_client, project, stored_path + ): + root, secrets = project + uploads = root / "assets" / "plugins" / "static-image" / "uploads" + uploads.mkdir(parents=True) + other = root / "assets" / "plugins" / "other" / "uploads" + other.mkdir(parents=True) + (other / "x.png").write_bytes(PNG) + (uploads / ".metadata.json").write_text( + json.dumps({"i": {"id": "i", "path": stored_path}}), encoding="utf-8" + ) + response = api_v3_client.post( + "/api/v3/plugins/assets/delete", + json={"plugin_id": "static-image", "image_id": "i"}, + ) + assert response.status_code == 200 + assert secrets.exists(), "file named by tampered metadata was deleted" + assert (other / "x.png").exists(), "another plugin's upload was deleted" + # The bad entry is still dropped, so the UI can get rid of it. + assert json.loads((uploads / ".metadata.json").read_text(encoding="utf-8")) == {} + + +class TestPluginActionDirectory: + """POST /api/v3/plugins/action runs a script named by the manifest in + get_plugin_directory(plugin_id), and plugin_id is the request body's. + get_plugin_directory joined it onto plugins_dir unchecked, so + '../elsewhere' ran a script from any directory holding a manifest. + """ + + @pytest.fixture + def tree(self, tmp_path): + import threading + + from src.plugin_system.plugin_manager import PluginManager + + plugins = tmp_path / "plugin-repos" + (plugins / "demo").mkdir(parents=True) + (plugins / "ledmatrix-legacy").mkdir() + outside = tmp_path / "elsewhere" + outside.mkdir() + (outside / "manifest.json").write_text(json.dumps({ + "web_ui_actions": [{"id": "go", "type": "script", "script": "s.py"}], + }), encoding="utf-8") + marker = tmp_path / "PWNED" + (outside / "s.py").write_text( + "open(%r, 'w').write('ran')\n" % str(marker), encoding="utf-8" + ) + manager = MagicMock() + manager.plugins_dir = plugins + manager._discovery_lock = threading.Lock() + manager.plugin_directories = {} + manager.get_plugin_directory = ( + lambda pid: PluginManager.get_plugin_directory(manager, pid) + ) + return manager, plugins, marker + + def test_real_plugin_directories_are_still_found(self, tree): + manager, plugins, _ = tree + assert manager.get_plugin_directory("demo") == str(plugins / "demo") + assert manager.get_plugin_directory("legacy") == str(plugins / "ledmatrix-legacy") + assert manager.get_plugin_directory("nope") is None + + def test_a_discovered_plugin_is_returned_from_the_registry(self, tree, tmp_path): + manager, _, _ = tree + manager.plugin_directories = {"linked": tmp_path / "somewhere"} + assert manager.get_plugin_directory("linked") == str(tmp_path / "somewhere") + + @pytest.mark.parametrize("plugin_id", [ + "..", "../elsewhere", "a/b", "/abs", "x" + BACKSLASH + "..", "", + ]) + def test_get_plugin_directory_refuses_anything_but_a_plain_name(self, tree, plugin_id): + manager, _, _ = tree + assert manager.get_plugin_directory(plugin_id) is None + + def test_the_action_endpoint_runs_nothing_outside_the_plugins_dir( + self, api_v3_client, api_v3_module, tree + ): + manager, _, marker = tree + api_v3_module.api_v3.plugin_manager = manager + response = api_v3_client.post( + "/api/v3/plugins/action", + json={"plugin_id": "../elsewhere", "action_id": "go", "params": {}}, + ) + assert response.status_code == 400 + assert not marker.exists(), "script outside the plugins directory ran" + + def test_the_fallback_without_a_plugin_manager_is_guarded_too( + self, api_v3_client, api_v3_module + ): + api_v3_module.api_v3.plugin_manager = None + response = api_v3_client.post( + "/api/v3/plugins/action", + json={"plugin_id": "../elsewhere", "action_id": "go", "params": {}}, + ) + assert response.status_code == 400 diff --git a/web_interface/blueprints/api_v3/plugins.py b/web_interface/blueprints/api_v3/plugins.py index c9643e54..87d1cbd2 100644 --- a/web_interface/blueprints/api_v3/plugins.py +++ b/web_interface/blueprints/api_v3/plugins.py @@ -2832,6 +2832,12 @@ def execute_plugin_action(): 'received': {'plugin_id': plugin_id, 'action_id': action_id, 'has_params': bool(action_params)} }), 400 + # plugin_id comes from the request body and picks the directory whose + # manifest names the script to run, so it must be a plain name. + plugin_id = safe_path_component(plugin_id) + if plugin_id is None: + return jsonify({'status': 'error', 'message': 'Invalid plugin_id'}), 400 + # Get plugin directory if api_v3.plugin_manager: plugin_dir = api_v3.plugin_manager.get_plugin_directory(plugin_id) @@ -3303,6 +3309,15 @@ def authenticate_ytm(): except Exception as e: logger.error('Error in authenticate_ytm', exc_info=True) return jsonify({'status': 'error', 'message': 'An error occurred; see logs for details', 'details': describe_exception(e)}), 500 +def _plugin_uploads_dir(plugin_id): + """assets/plugins//uploads for a request-supplied id, or None. + + Same guard as the serving route in app.py: one plain path segment, + resolved and contained under assets/plugins. + """ + return resolve_under(PROJECT_ROOT / 'assets' / 'plugins', plugin_id, 'uploads') + + @api_v3.route('/plugins/assets/upload', methods=['POST']) def upload_plugin_asset(): """Upload asset files for a plugin""" @@ -3322,8 +3337,13 @@ def upload_plugin_asset(): if len(files) > 10: return jsonify({'status': 'error', 'message': 'Maximum 10 files per upload'}), 400 - # Setup plugin assets directory - assets_dir = PROJECT_ROOT / 'assets' / 'plugins' / plugin_id / 'uploads' + # Setup plugin assets directory. plugin_id is a form field: without + # the guard '../../config' created, listed and wrote into directories + # outside assets/plugins (the serving route was fixed in #561). + assets_dir = _plugin_uploads_dir(plugin_id) + if assets_dir is None: + return jsonify({'status': 'error', 'message': 'Invalid plugin_id'}), 400 + plugin_id = safe_path_component(plugin_id) assets_dir.mkdir(parents=True, exist_ok=True) # Load metadata file @@ -3958,7 +3978,9 @@ def delete_plugin_asset(): return jsonify({'status': 'error', 'message': 'plugin_id and image_id are required'}), 400 # Get asset directory - assets_dir = PROJECT_ROOT / 'assets' / 'plugins' / plugin_id / 'uploads' + assets_dir = _plugin_uploads_dir(plugin_id) + if assets_dir is None: + return jsonify({'status': 'error', 'message': 'Invalid plugin_id'}), 400 metadata_file = assets_dir / '.metadata.json' if not metadata_file.exists(): @@ -3971,9 +3993,17 @@ def delete_plugin_asset(): if image_id not in metadata: return jsonify({'status': 'error', 'message': 'Image not found'}), 404 - # Delete file - file_path = PROJECT_ROOT / metadata[image_id]['path'] - if file_path.exists(): + # Delete file. The stored path is data, not a trusted location: only + # unlink it when it resolves to a file directly inside this plugin's + # uploads. An entry pointing anywhere else is dropped from the + # metadata without touching the file it names. + entry = metadata[image_id] if isinstance(metadata[image_id], dict) else {} + parts = safe_relative_parts(entry.get('path')) + file_path = resolve_under(PROJECT_ROOT, *parts) if parts else None + if file_path is None or file_path.parent != assets_dir: + logger.warning('Asset %s has a path outside its uploads directory; ' + 'removing the entry without deleting a file', image_id) + elif file_path.exists(): file_path.unlink() # Remove from metadata @@ -3997,7 +4027,9 @@ def list_plugin_assets(): return jsonify({'status': 'error', 'message': 'plugin_id is required'}), 400 # Get asset directory - assets_dir = PROJECT_ROOT / 'assets' / 'plugins' / plugin_id / 'uploads' + assets_dir = _plugin_uploads_dir(plugin_id) + if assets_dir is None: + return jsonify({'status': 'error', 'message': 'Invalid plugin_id'}), 400 metadata_file = assets_dir / '.metadata.json' if not metadata_file.exists(): From 68f267143b76be70b5bf8cd5cea1d64c8ed8206a Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Thu, 17 Sep 2026 08:50:09 -0400 Subject: [PATCH 2/3] fix(web): escape registry and upload values in plugin manager inline handlers The store, saved-repository and custom-registry buttons built onclick='...(${JSON.stringify(id)})...'. JSON.stringify leaves ' alone, so a custom registry entry whose id contained ' closed the attribute and added its own handler. One helper, jsStringAttr(), now HTML-escapes the JSON literal for every one of those handlers, and the store View button opens only http(s) repo links. The live window.updateImageList (plugins_manager.js loads last, so its copy wins over the file-upload widget's) wrote the uploaded file's original name, path and ids into markup raw; they are escaped now. Co-Authored-By: Claude Opus 5 --- test/js/README.md | 1 + test/js/run_all.js | 2 +- test/js/unit/test_inline_handler_escaping.js | 215 +++++++++++++++++++ web_interface/static/v3/plugins_manager.js | 39 ++-- 4 files changed, 242 insertions(+), 15 deletions(-) create mode 100644 test/js/unit/test_inline_handler_escaping.js diff --git a/test/js/README.md b/test/js/README.md index 9d7c3b95..c9eae3b9 100644 --- a/test/js/README.md +++ b/test/js/README.md @@ -40,6 +40,7 @@ nothing is listening, so it stays useful in a bare checkout. | `unit/test_style_editor_element_keys.js` | no | `elementKeys()`/`styleRows()`/`positionRows()` from `widgets/style-editor.js`: every `customization.layout` entry gets exactly one row -- paired with its style element through core's `x-layout-key` (so `score` belongs to `score_text`, not a second row), or a position row of its own, leaves included -- since the widget claims the whole `layout` block from the generic fallback renderer | | `unit/test_style_editor_layout_leaf_columns.js` | no | `columnsFor()` from `widgets/style-editor.js`: a layout-only key whose own value is a leaf (no x/y sub-object, e.g. a `show_logo` toggle) gets a self-keyed column instead of a blank, uneditable row | | `unit/test_style_editor_layout_leaf_collision.js` | no | `columnsFor()` from `widgets/style-editor.js`: a layout-only leaf key still gets its own column even when its name collides with an unrelated element's style sub-field or another layout axis's sub-field | +| `unit/test_inline_handler_escaping.js` | no | The store, saved-repository and custom-registry inline `onclick` handlers and the live `window.updateImageList` from `plugins_manager.js`: a registry id, URL or uploaded file name carrying `'`, `"` or entities adds no attributes and reaches the handler intact, and the store's View button opens only http(s) links | | `dom/test_installed_dom.js` | yes | The toolbar in a real DOM: pill/search/sort interaction, the HTMX partial re-swap, and a `getComputedStyle` check that `.filter-pill[data-active]` really matches the emitted markup | | `dom/test_store_dom.js` | yes | Store pagination, per-page, category, tri-state Installed button, and persistence across a re-boot, against the live registry | | `dom/test_no_double_fetch.js` | yes | Loads the **whole** `plugins_manager.js` and counts requests: typing in the store search must filter the cached list, not refetch `/api/v3/plugins/store/list` | diff --git a/test/js/run_all.js b/test/js/run_all.js index bf9ca625..abf0fff7 100755 --- a/test/js/run_all.js +++ b/test/js/run_all.js @@ -18,7 +18,7 @@ const UNIT = ['unit/test_list_filter.js', 'unit/test_render_cards.js', 'unit/test_html_escaping.js', 'unit/test_style_editor_element_keys.js', 'unit/test_style_editor_layout_leaf_columns.js', 'unit/test_style_editor_layout_leaf_collision.js', - 'unit/test_update_all.js']; + 'unit/test_update_all.js', 'unit/test_inline_handler_escaping.js']; const DOM = ['dom/test_installed_dom.js', 'dom/test_store_dom.js', 'dom/test_no_double_fetch.js', 'dom/test_tools_sections.js']; diff --git a/test/js/unit/test_inline_handler_escaping.js b/test/js/unit/test_inline_handler_escaping.js new file mode 100644 index 00000000..984d4299 --- /dev/null +++ b/test/js/unit/test_inline_handler_escaping.js @@ -0,0 +1,215 @@ +// Registry- and upload-supplied strings must not escape the inline handlers +// plugins_manager.js builds for them. +// +// The store, saved-repository and custom-registry renderers wrote +// +// - @@ -4102,9 +4104,9 @@ function renderSavedRepositories(repositories) { return; } - // Helper function to escape for JavaScript strings + // JS string literal for an inline handler; see jsStringAttr const escapeJs = (text) => { - return JSON.stringify(text || ''); + return jsStringAttr(text || ''); }; container.innerHTML = repositories.map(repo => { @@ -4498,9 +4500,9 @@ function renderCustomRegistryPlugins(plugins, registryUrl) { .replace(/'/g, '''); }; - // Helper function to escape for JavaScript strings + // JS string literal for an inline handler; see jsStringAttr const escapeJs = (text) => { - return JSON.stringify(text || ''); + return jsStringAttr(text || ''); }; container.innerHTML = plugins.map(plugin => { @@ -4617,6 +4619,15 @@ function escapeAttribute(text) { .replace(/>/g, '>'); } +// A quoted JS string literal that is safe inside an inline handler attribute +// (onclick='f(${jsStringAttr(id)})' or onclick="..."). JSON.stringify alone +// makes a valid JS string but leaves ' and & untouched, so a registry entry +// id containing ' closed a single-quoted attribute and added its own +// handlers. The browser decodes the entities before the JS is parsed. +function jsStringAttr(value) { + return escapeAttribute(JSON.stringify(value == null ? '' : String(value))); +} + // Format date for display function formatDate(dateString) { if (!dateString) return 'Unknown'; @@ -5104,11 +5115,11 @@ window.updateImageList = function(fieldId, images) { const scheduleSummary = hasSchedule ? (window.getScheduleSummary ? window.getScheduleSummary(imgSchedule) : 'Scheduled') : 'Always shown'; return ` -
+
- ${img.filename || ''} @@ -5116,7 +5127,7 @@ window.updateImageList = function(fieldId, images) {
-

${img.original_filename || img.filename || 'Image'}

+

${escapeHtml(img.original_filename || img.filename || 'Image')}

${window.formatFileSize ? window.formatFileSize(img.size || 0) : (Math.round((img.size || 0) / 1024) + ' KB')} • ${window.formatDate ? window.formatDate(img.uploaded_at) : (img.uploaded_at || '')}

${scheduleSummary} @@ -5125,14 +5136,14 @@ window.updateImageList = function(fieldId, images) {

- +
`; }).join(''); From d1c28e66161613b173ce06c7299047ea22b014e8 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Thu, 17 Sep 2026 08:50:30 -0400 Subject: [PATCH 3/3] docs(changelog): note plugin asset, action and inline handler guards Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea497352..43354296 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -112,6 +112,26 @@ Web interface: re-sent with backoff instead of being counted as failed and skipped — that is how a disabled plugin with an update waiting was silently left out. +Security (request paths and inline handlers, siblings of #561): + +- `POST /api/v3/plugins/assets/upload`, `GET .../assets/list` and + `POST .../assets/delete` validate `plugin_id` with `src/common/path_safety` + and answer 400 otherwise. A `plugin_id` of `../../config` used to create an + `uploads/` directory outside `assets/plugins`, write images and + `.metadata.json` there, list it, and delete whatever file a metadata entry + named. Delete now unlinks only a path that resolves inside that plugin's + uploads directory (any other entry is dropped without touching a file). +- `PluginManager.get_plugin_directory()` returns `None` for anything but a + plain name, so `POST /api/v3/plugins/action` can no longer run a manifest + script from a directory outside the plugins directory (`../elsewhere`); the + route also rejects such ids with 400. +- Plugin Store, saved-repository and custom-registry buttons escape registry + values for their inline `onclick` handlers (`jsStringAttr` in + `plugins_manager.js`). An entry id containing `'` used to close the attribute + and add its own script. The store's View button opens only `http(s)` links. +- The uploaded-images list escapes each file's original name, path and ids; a + name like `.png` was inserted as markup. + Plugin system: - A plugin no longer starts with a schema warning and a degraded flag because