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 <noreply@anthropic.com>
2026-10-13 02:26:37 +00:00 · 2026-09-17 08:50:09 -04:00
parent 15cc6926a4
commit 68f267143b
4 changed files with 242 additions and 15 deletions
@@ -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` |
@@ -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'];

@@ -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);
@@ -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('');