Merge audit16-core/security into the September 16 core audit fixes

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 `<img src=x onerror=...>.png` was inserted as markup.
+
 Plugin system:
 
 - A plugin no longer starts with a schema warning and a degraded flag because

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)

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` |

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'];
 

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
+//
+//     <button onclick='... installFromCustomRegistry(${JSON.stringify(id)}) ...'>
+//
+// JSON.stringify makes a valid JS string but leaves `'` alone, so an entry id
+// of  x' onmouseover='alert(1)  closed the single-quoted attribute and added a
+// handler of its own. The live window.updateImageList (plugins_manager.js loads
+// last, so its copy beats the file-upload widget's) put the uploaded file's
+// original name into the markup unescaped.
+//
+// Each case renders with the shipped function, parses the tag the way a browser
+// does (quoted attribute values, entities decoded), and checks two things: no
+// attribute appeared that the template did not write, and the decoded handler
+// runs and passes the hostile value through intact as data.
+
+const fs = require('fs');
+const path = require('path');
+
+const SRC = fs.readFileSync(
+  path.resolve(__dirname, '../../../web_interface/static/v3/plugins_manager.js'), 'utf8');
+
+let pass = 0, fail = 0;
+const ok = (label, cond, extra) => cond
+  ? (pass++, console.log('  ok   ' + label))
+  : (fail++, console.log('  FAIL ' + label + (extra !== undefined ? '  -> ' + JSON.stringify(extra).slice(0, 400) : '')));
+
+function extract(opener) {
+  const start = SRC.indexOf(opener);
+  if (start < 0) { console.error('FAIL: cannot find ' + JSON.stringify(opener)); process.exit(1); }
+  let depth = 0;
+  for (let j = SRC.indexOf('{', start); j < SRC.length; j++) {
+    if (SRC[j] === '{') depth++;
+    else if (SRC[j] === '}' && --depth === 0) return SRC.slice(start, j + 1);
+  }
+  console.error('FAIL: unbalanced braces after ' + opener); process.exit(1);
+}
+
+// ── DOM shim ───────────────────────────────────────────────────────────────
+class FakeEl {
+  constructor() { this.innerHTML = ''; this.value = ''; this.textContent = ''; }
+}
+class TextEl {
+  set textContent(v) { this._t = String(v == null ? '' : v); }
+  get innerHTML() {
+    return (this._t || '').replace(/&/g, '&amp;').replace(/</g, '&lt;').replace(/>/g, '&gt;');
+  }
+}
+const els = {};
+global.document = {
+  getElementById: id => (els[id] ||= new FakeEl()),
+  createElement: () => new TextEl(),
+};
+global.window = global;
+global.pluginLog = () => {};
+global.isStorePluginInstalled = () => false;
+global.isNewPlugin = () => false;
+global.formatDate = () => '';
+global.setGridHtmlIfChanged = (container, html) => { container.innerHTML = html; };
+
+// eslint-disable-next-line no-eval
+eval([
+  'function escapeHtml(text) {', 'function escapeAttribute(text) {', 'function jsStringAttr(value) {',
+  'function renderPluginStore(plugins) {', 'function renderSavedRepositories(repositories) {',
+  'function renderCustomRegistryPlugins(plugins, registryUrl) {',
+].map(extract).join('\n') + '\nglobal.jsStringAttr = jsStringAttr; global.escapeHtml = escapeHtml;'
+  + '\nglobal.renderPluginStore = renderPluginStore; global.renderSavedRepositories = renderSavedRepositories;'
+  + '\nglobal.renderCustomRegistryPlugins = renderCustomRegistryPlugins; global.escapeAttribute = escapeAttribute;');
+// eslint-disable-next-line no-eval
+eval(extract('window.updateImageList = function(fieldId, images) {'));
+
+// ── minimal HTML start-tag tokenizer ───────────────────────────────────────
+function decodeEntities(s) {
+  return s.replace(/&(#x[0-9a-f]+|#\d+|quot|amp|lt|gt|apos);/gi, (m, e) => {
+    const k = e.toLowerCase();
+    if (k === 'quot') return '"';
+    if (k === 'amp') return '&';
+    if (k === 'lt') return '<';
+    if (k === 'gt') return '>';
+    if (k === 'apos') return "'";
+    return String.fromCodePoint(k[1] === 'x' ? parseInt(k.slice(2), 16) : parseInt(k.slice(1), 10));
+  });
+}
+function tags(html, name) {
+  const out = [];
+  let i = 0;
+  while ((i = html.indexOf('<' + name, i)) >= 0) {
+    let j = i + name.length + 1;
+    const attrs = [];
+    for (;;) {
+      while (/\s/.test(html[j])) j++;
+      if (html[j] === '>' || j >= html.length) break;
+      if (html[j] === '/') { j++; continue; }
+      let n = '';
+      while (j < html.length && !/[\s=>]/.test(html[j])) n += html[j++];
+      let v = '';
+      while (/\s/.test(html[j])) j++;
+      if (html[j] === '=') {
+        j++;
+        while (/\s/.test(html[j])) j++;
+        const q = html[j];
+        if (q === '"' || q === "'") {
+          const end = html.indexOf(q, j + 1);
+          v = html.slice(j + 1, end); j = end + 1;
+        } else {
+          while (j < html.length && !/[\s>]/.test(html[j])) v += html[j++];
+        }
+      }
+      attrs.push([n.toLowerCase(), decodeEntities(v)]);
+    }
+    out.push(attrs);
+    i = j;
+  }
+  return out;
+}
+const names = attrs => attrs.map(a => a[0]);
+const attr = (attrs, n) => (attrs.find(a => a[0] === n) || [])[1];
+
+// Run a decoded inline handler with window/document stubs; return the calls.
+function runHandler(code) {
+  const calls = [];
+  const win = new Proxy({}, {
+    get: (_, prop) => (...args) => { calls.push([prop, ...args]); },
+    has: () => true,
+  });
+  const doc = { getElementById: () => ({ value: '' }) };
+  // eslint-disable-next-line no-new-func
+  new Function('window', 'document', 'console', code)(win, doc, { error() {} });
+  return calls;
+}
+
+const SQ = "x' onmouseover='alert(1)";
+const DQ = 'x" onmouseover="alert(1)';
+const AMP = 'x&#39; onmouseover=&#39;alert(1)';
+
+console.log('\n1. custom registry install button');
+for (const hostile of [SQ, DQ, AMP]) {
+  renderCustomRegistryPlugins([{ id: hostile, name: 'n', plugin_path: hostile }], hostile);
+  const buttons = tags(els['custom-registry-grid'].innerHTML, 'button');
+  const b = buttons.find(a => attr(a, 'onclick'));
+  ok(`${JSON.stringify(hostile)}: only onclick and class attributes`,
+     b && names(b).join(',') === 'onclick,class', b && names(b));
+  let calls = [];
+  try { calls = runHandler(attr(b, 'onclick')); } catch (e) { calls = [['threw', String(e)]]; }
+  ok(`${JSON.stringify(hostile)}: handler passes id, url and path intact`,
+     calls.length === 1 && calls[0][0] === 'installFromCustomRegistry'
+       && calls[0][1] === hostile && calls[0][2] === hostile && calls[0][3] === hostile, calls);
+}
+
+console.log('\n2. store install and view buttons');
+for (const hostile of [SQ, DQ, AMP]) {
+  renderPluginStore([{ id: hostile, name: 'n', repo: 'https://github.com/o/r', plugin_path: 'p' }]);
+  const buttons = tags(els['plugin-store-grid'].innerHTML, 'button');
+  ok(`${JSON.stringify(hostile)}: two buttons, no extra attributes`,
+     buttons.length === 2 && buttons.every(b => names(b).every(n => ['onclick', 'class', 'disabled'].includes(n))),
+     buttons.map(names));
+  let calls = [];
+  try { calls = runHandler(attr(buttons[0], 'onclick')); } catch (e) { calls = [['threw', String(e)]]; }
+  ok(`${JSON.stringify(hostile)}: install handler gets the id intact`,
+     calls.length === 1 && calls[0][0] === 'installPlugin' && calls[0][1] === hostile, calls);
+}
+
+console.log('\n3. store view button only opens http(s) links');
+renderPluginStore([{ id: 'a', name: 'n', repo: 'https://github.com/o/r', plugin_path: 'plugins/a' }]);
+{
+  const view = tags(els['plugin-store-grid'].innerHTML, 'button')[1];
+  const calls = runHandler(attr(view, 'onclick'));
+  ok('https repo opens the plugin tree',
+     calls.length === 1 && calls[0][0] === 'open' && calls[0][1] === 'https://github.com/o/r/tree/main/plugins/a', calls);
+}
+renderPluginStore([{ id: 'a', name: 'n', repo: 'javascript:alert(document.domain)//' }]);
+{
+  const view = tags(els['plugin-store-grid'].innerHTML, 'button')[1];
+  const calls = runHandler(attr(view, 'onclick'));
+  ok('javascript: repo opens nothing', calls.length === 0, calls);
+  ok('javascript: repo button is disabled', names(view).includes('disabled'), names(view));
+}
+
+console.log('\n4. saved repository remove button');
+for (const hostile of [SQ, DQ, AMP]) {
+  renderSavedRepositories([{ url: hostile, name: hostile }]);
+  const b = tags(els['saved-repositories-list'].innerHTML, 'button')[0];
+  ok(`${JSON.stringify(hostile)}: no extra attributes`,
+     b && names(b).join(',') === 'onclick,class,title,aria-label', b && names(b));
+  let calls = [];
+  try { calls = runHandler(attr(b, 'onclick')); } catch (e) { calls = [['threw', String(e)]]; }
+  ok(`${JSON.stringify(hostile)}: handler gets the url intact`,
+     calls.length === 1 && calls[0][0] === 'removeSavedRepository' && calls[0][1] === hostile, calls);
+}
+
+console.log('\n5. live window.updateImageList escapes the uploaded file name');
+window.getUploadConfig = () => ({ plugin_id: SQ });
+window.currentPluginConfig = null;
+{
+  const name = '<img src=x onerror=alert(1)>' + DQ + '.png';
+  window.updateImageList('f', [{ id: SQ, path: 'assets/x".png', filename: DQ, original_filename: name, size: 1 }]);
+  const html = els['f_image_list'].innerHTML;
+  ok('no raw markup from original_filename', !html.includes('<img src=x onerror'), html);
+  const imgs = tags(html, 'img');
+  ok('one <img>, only the template attributes',
+     imgs.length === 1 && names(imgs[0]).join(',') === 'src,alt,loading,decoding,class,onerror', imgs.map(names));
+  ok('alt carries the stored filename as text', attr(imgs[0], 'alt') === DQ, imgs[0]);
+  const buttons = tags(html, 'button');
+  ok('two buttons, no extra attributes',
+     buttons.length === 2 && buttons.every(b => names(b).join(',') === 'type,onclick,class,title,aria-label'),
+     buttons.map(names));
+  const del = runHandler(attr(buttons[1], 'onclick'));
+  ok('delete handler gets field, image and plugin ids intact',
+     del.length === 1 && del[0][0] === 'deleteUploadedImage' && del[0][1] === 'f' && del[0][2] === SQ && del[0][3] === SQ, del);
+}
+
+console.log(`\n${pass} passed, ${fail} failed\n`);
+process.exit(fail ? 1 : 0);

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

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/<plugin_id>/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():

web_interface/static/v3/plugins_manager.js

@@ -3893,13 +3893,15 @@ function renderPluginStore(plugins) {
         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 || '');
     };
 
     setGridHtmlIfChanged(container, plugins.map(plugin => {
         const installed = isStorePluginInstalled(plugin);
+        // Registry data: only open real web links, never javascript: URLs.
+        const repoLink = plugin.repo && /^https?:\/\//i.test(plugin.repo) ? plugin.repo : '';
         return `
         <div class="plugin-card">
             <div class="flex items-start justify-between mb-4">
@@ -3941,7 +3943,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 ${installed ? 'bg-gray-500 hover:bg-gray-600' : 'bg-green-600 hover:bg-green-700'} text-white px-4 py-2 rounded-md text-sm flex-1 font-semibold">
                         <i class="fas ${installed ? 'fa-redo' : 'fa-download'} mr-2"></i>${installed ? 'Reinstall' : 'Install'}
                     </button>
-                    <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'}">
+                    <button onclick='${repoLink ? `window.open(${escapeJs(plugin.plugin_path ? repoLink + "/tree/" + encodeURIComponent(plugin.default_branch || plugin.branch || "main") + "/" + plugin.plugin_path.split("/").map(encodeURIComponent).join("/") : repoLink)}, "_blank")` : `void(0)`}' ${repoLink ? '' : 'disabled'} class="btn bg-gray-600 hover:bg-gray-700 text-white px-4 py-2 rounded-md text-sm flex-1 font-semibold${repoLink ? '' : ' opacity-50 cursor-not-allowed'}">
                         <i class="fas fa-external-link-alt mr-2"></i>View
                     </button>
                 </div>
@@ -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, '&#39;');
     };
 
-    // 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, '&gt;');
 }
 
+// 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 `
-            <div id="img_${img.id || idx}" class="bg-gray-50 p-3 rounded-lg border border-gray-200">
+            <div id="img_${escapeAttribute(img.id || idx)}" class="bg-gray-50 p-3 rounded-lg border border-gray-200">
                 <div class="flex items-center justify-between mb-2">
                     <div class="flex items-center space-x-3 flex-1">
-                        <img src="/${img.path || ''}"
-                             alt="${img.filename || ''}"
+                        <img src="/${escapeAttribute(String(img.path || '').replace(/^\/+/, ''))}"
+                             alt="${escapeAttribute(img.filename || '')}"
                              loading="lazy" decoding="async"
                              class="w-16 h-16 object-cover rounded"
                              onerror="this.style.display='none'; this.nextElementSibling.style.display='block';">
@@ -5116,7 +5127,7 @@ window.updateImageList = function(fieldId, images) {
                             <i class="fas fa-image text-gray-400"></i>
                         </div>
                         <div class="flex-1 min-w-0">
-                            <p class="text-sm font-medium text-gray-900 truncate">${img.original_filename || img.filename || 'Image'}</p>
+                            <p class="text-sm font-medium text-gray-900 truncate">${escapeHtml(img.original_filename || img.filename || 'Image')}</p>
                             <p class="text-xs text-gray-500">${window.formatFileSize ? window.formatFileSize(img.size || 0) : (Math.round((img.size || 0) / 1024) + ' KB')} • ${window.formatDate ? window.formatDate(img.uploaded_at) : (img.uploaded_at || '')}</p>
                             <p class="text-xs text-blue-600 mt-1">
                                 <i class="fas fa-clock mr-1"></i>${scheduleSummary}
@@ -5125,14 +5136,14 @@ window.updateImageList = function(fieldId, images) {
                     </div>
                     <div class="flex items-center space-x-2 ml-4">
                         <button type="button"
-                                onclick="window.openImageSchedule('${fieldId}', '${img.id}', ${idx})"
+                                onclick="window.openImageSchedule(${jsStringAttr(fieldId)}, ${jsStringAttr(img.id)}, ${idx})"
                                 class="text-blue-600 hover:text-blue-800 p-2"
                                 title="Schedule this image"
                                 aria-label="Schedule image ${escapeAttribute(img.original_filename || img.filename || '')}">
                             <i class="fas fa-calendar-alt" aria-hidden="true"></i>
                         </button>
                         <button type="button"
-                                onclick="window.deleteUploadedImage('${fieldId}', '${img.id}', '${pluginId}')"
+                                onclick="window.deleteUploadedImage(${jsStringAttr(fieldId)}, ${jsStringAttr(img.id)}, ${jsStringAttr(pluginId)})"
                                 class="text-red-600 hover:text-red-800 p-2"
                                 title="Delete image"
                                 aria-label="Delete image ${escapeAttribute(img.original_filename || img.filename || '')}">
@@ -5141,7 +5152,7 @@ window.updateImageList = function(fieldId, images) {
                     </div>
                 </div>
                 <!-- Schedule widget will be inserted here when opened -->
-                <div id="schedule_${img.id || idx}" class="hidden mt-3 pt-3 border-t border-gray-300"></div>
+                <div id="schedule_${escapeAttribute(img.id || idx)}" class="hidden mt-3 pt-3 border-t border-gray-300"></div>
             </div>
             `;
         }).join('');