security: triage the CodeQL backlog — 129 alerts, three of them live (#561)

* fix(web): escape quotes in every HTML escaper, not just & < > The escapers are all `div.textContent = x; return div.innerHTML`. That round-trip escapes &, < and > -- the only characters the HTML serializer must escape in a text node -- and leaves quotes alone. Every widget then interpolates the result into a quoted attribute value: value="${escapeHtml(v)}" title="${escapeHtml(v)}" so a value of `x" onmouseover="alert(1)` closes the attribute and adds an event handler of its own. CodeQL reported this 83 times (js/incomplete-html-attribute-sanitization) across the widget files. It is one bug, not 83: the widgets each carry a standalone fallback that did escape quotes, but they all prefer BaseWidget.escapeHtml when window.BaseWidget exists -- which it always does in the shipped page -- so the correct fallbacks were dead code and the incomplete shared one ran. Fixed at each source instead of at the call sites. app-shell.js already documented this exact gap in a comment and worked around it by building DOM nodes by hand; that workaround stays (setting a property cannot be got wrong), the comment is now accurate. cache.html's delete button interpolated the cache key into `onclick="deleteCacheFile('...')"`. Escaping cannot help there -- the browser HTML-decodes the attribute before parsing it as JS, so `'` becomes a real `'` again -- so the key moves to a data-cache-key attribute that the handler reads back. url-input.js additionally wrote a value straight into an <a href> after validating it against a schema-supplied protocol list, and that list accepted any RFC 3986 scheme -- "javascript" included. Scriptable schemes (javascript, data, vbscript, blob, filesystem) are now refused both when the list is normalised and when a URL is checked against it, and the render path routes its href through the same check instead of emitting whatever was stored (js/xss-through-dom). test/js/unit/test_html_escaping.js reads each escaper out of the shipped file and runs it, so losing the quote handling again fails a test rather than a scan. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(security): stop request-supplied names from reaching paths outside their base Three of the py/path-injection alerts were live, not lint: * GET /api/v3/plugins/<plugin_id>/static/<path:file_path> read any file whose resolved path *string-prefixed* the plugin directory. Flask's default converter forbids a slash but not dots, and get_plugin_directory('..') returned the parent of the plugins directory because it exists -- so every file under the project root then prefixed that directory, config/config_secrets.json included. The prefix check was also wrong on its own terms: with plugin dir "plugin-repos/foo", "../foo-evil/x" resolves to "plugin-repos/foo-evil/x", whose string does start with "plugin-repos/foo". * POST /api/v3/plugins/of-the-day/json/delete interpolated the request body's file_id into f"{file_id}.json" and unlinked it, unvalidated. A file_id of "../../../../etc/something" deleted that file. This is the one finding in the batch that destroyed data rather than exposing it. * POST /api/v3/cache/delete passed the body's key through CacheManager.clear_cache to DiskCache, which joined it as a filename and called os.remove. Same shape, same result. The guard goes in DiskCache.get_cache_path, the single choke point get/set/clear share, so every caller is covered rather than just this route. Real keys are the stems of files already flat in the cache directory -- that is how list_cache_files derives them -- so nothing legitimate is turned away. The rest of the cluster (web_interface/app.py's asset route, the plugin update handler, _get_plugin_version, the plugin-schema read in config.py) was guarded in ways that held, but each had grown its own version of the check. They now go through one helper, src/common/path_safety.py, which returns the *sanitised value* rather than a verdict -- so a caller cannot validate one string and open another, which is how the two real bugs above were shaped. Also: WiFiManager.connect_to_network took the SSID and password straight from POST /api/v3/wifi/connect into nmcli's argv. There is no shell there, so CodeQL's py/command-line-injection alert overstates the risk -- but nmcli reads a leading "-" as an option, so an SSID of "--ask" asks nmcli to run differently rather than to join a network. Both values are now checked for shape (802.11's 32-octet SSID limit, WPA's 8-63 char passphrase or 64-char hex key, no control characters, no leading dash) before any subprocess runs. test/test_path_traversal_guards.py asserts on the filesystem, not just the status code: a handler that returns 403 and deletes the file anyway would pass the weaker check. Twelve of its cases fail against the unpatched code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(web): refuse a plugin id that is not a plain name, don't truncate it pages_v3 and scripts/dev_server.py ran request ids through os.path.basename and carried on with what came out, so "../weather" rendered the config form for "weather". Nothing escaped the plugins directory -- the relative_to guards held -- but the handler answered a request nobody made, and validating one string while the filesystem sees another is the shape both live traversals earlier in this branch had. Same treatment as the rest: safe_path_component rejects rather than truncates, resolve_under returns the path it checked, and the call sites use what those return. The three handlers that had hand-rolled resolve-and-relative_to blocks lose about twenty lines to the shared one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(web): say what the plugin web_ui iframe actually is The docstring claimed the fragment runs "in a sandboxed iframe". The iframe in plugin_config.html carries no sandbox attribute, so the fragment runs with the interface's own origin. That is fine -- the file belongs to an installed plugin, and an installed plugin already runs Python on the device, so the trust boundary is install rather than this route -- but a comment promising containment that is not there is worse than no comment. This is the context for the py/reflective-xss alert on this handler. Also drops the now-unused os/os.path imports. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(web): inline url-input's scheme guard at the previewLink.href sink CodeQL flagged this line as a new high-severity js/xss-through-dom alert on this PR even though it is already covered by SCRIPTABLE_SCHEMES: the guard reached the sink through safeHref -> isValidUrl, two function calls away, which its DOM-based-XSS sanitizer recognition does not trace. Behavior is unchanged -- same scheme check, same SCRIPTABLE_SCHEMES list, same allowedProtocols gate -- just inlined directly above the previewLink.href assignment it guards, so the barrier is visible in the same scope as the sink. Added a regression test that runs the shipped onInput handler (not just the extracted helpers) against a mocked DOM, so a future change that reintroduces an unguarded previewLink.href assignment fails here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(security): address CodeRabbit findings on the CodeQL triage PR - src/wifi_manager.py: reject non-ASCII WPA-PSK passphrases before any credential-saving or connect flow runs. NetworkManager only accepts printable ASCII passphrases (or a 64-char hex key); a non-ASCII value was previously saved/attempted before nmcli itself rejected it. - web_interface/blueprints/api_v3/config.py: fail closed when the plugin config schema path can't be resolved under the plugins directory (e.g. a symlinked plugin dir). Previously this fell through with secret_fields left empty, so submitted credentials for that plugin were saved as ordinary, unencrypted configuration. - web_interface/static/v3/js/widgets/plugin-file-manager.js: stop splicing the JSON day/column key into an inline oninput="..." handler string. escHtml() escapes quotes for a normal HTML attribute, but the browser HTML-decodes the attribute before running it as script, which undoes that escaping and lets a crafted column name (e.g. from an uploaded JSON file) break out of the JS string and execute. Cell edits now travel through data-day/data-col attributes read by one delegated 'input' listener instead. While in this file: fixed 6 pre-existing missing-')' typos on multi-line safeSetHTML(...) calls (already flagged by Biome in this PR's own CodeRabbit run as syntax errors blocking its lint pass). These predate this PR (present on main too) but made the whole file fail to parse in any JS engine, which is a bigger problem than the XSS finding itself and directly touches the same lines. Added/extended regression tests for each fix; full suites pass (pytest: 4580 passed, 62 skipped; JS: 84 assertions). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2026-10-04 14:25:08 +00:00 · 2026-09-11 15:32:03 -04:00
parent 5137e86d16
commit f6367d63ae
27 changed files with 1520 additions and 170 deletions
@@ -602,9 +602,9 @@
                        };
                        // Build the <i class="..."> + label as DOM nodes so a
                        // hostile plugin.icon (e.g. containing a quote) can't
-                        // break out of the attribute. escapeHtml only escapes
-                        // <, >, &, not ", so attribute-context interpolation
-                        // would be unsafe.
+                        // break out of the attribute. escapeHtml now escapes
+                        // quotes too, but setting the property directly cannot
+                        // be got wrong at all, so it stays.
                        const iconEl = document.createElement('i');
                        iconEl.className = plugin.icon || 'fas fa-puzzle-piece';
                        const labelNode = document.createTextNode(plugin.name || plugin.id);
@@ -643,10 +643,15 @@
                    }
                },

+                // Quotes too, so the result is safe inside a quoted attribute
+                // value -- the textContent/innerHTML round-trip alone only
+                // escapes &, < and >.
                escapeHtml(text) {
                    const div = document.createElement('div');
                    div.textContent = text;
-                    return div.innerHTML;
+                    return div.innerHTML
+                        .replace(/"/g, '&quot;')
+                        .replace(/'/g, '&#39;');
                },

                async refreshPlugins() {
@@ -260,6 +260,9 @@ function closeErrorModal() {

 /**
 * Escape HTML to prevent XSS.
+ *
+ * Quotes are escaped as well so the result is safe inside a quoted attribute
+ * value -- the textContent/innerHTML round-trip alone only escapes &, < and >.
 */
 function escapeHtml(text) {
    if (typeof text !== 'string') {
@@ -267,7 +270,9 @@ function escapeHtml(text) {
    }
    const div = document.createElement('div');
    div.textContent = text;
-    return div.innerHTML;
+    return div.innerHTML
+        .replace(/"/g, '&quot;')
+        .replace(/'/g, '&#39;');
 }

 /**
@@ -118,15 +118,27 @@
        /**
         * Escape HTML to prevent XSS
         * Always escapes the input, even for non-strings, by coercing to string first
+         *
+         * The result is safe in text content AND inside quoted attribute values.
+         * The textContent/innerHTML round-trip only escapes `&`, `<` and `>` --
+         * the HTML serializer leaves quotes alone because they are harmless in a
+         * text node. Every widget here interpolates the result into attributes
+         * (`value="${escapeHtml(v)}"`), where an unescaped `"` closes the
+         * attribute and lets the value inject its own, so the quotes have to go
+         * too. Each widget's standalone fallback already did this; the shared
+         * implementation they all prefer did not.
+         *
         * @param {*} text - Text to escape (will be coerced to string)
-         * @returns {string} Escaped text
+         * @returns {string} Escaped text, safe for text and attribute contexts
         */
        escapeHtml(text) {
            // Always coerce to string first, then escape
            const textStr = String(text);
            const div = document.createElement('div');
            div.textContent = textStr;
-            return div.innerHTML;
+            return div.innerHTML
+                .replace(/"/g, '&quot;')
+                .replace(/'/g, '&#39;');
        }
        
        /**
@@ -50,10 +50,14 @@
            }
            
            // Escape HTML to prevent XSS (for HTML contexts)
+            // Quotes too: the result lands in quoted attribute values below, and
+            // the textContent/innerHTML round-trip only escapes &, < and >.
            const escapeHtml = (text) => {
                const div = document.createElement('div');
-                div.textContent = text;
-                return div.innerHTML;
+                div.textContent = String(text ?? '');
+                return div.innerHTML
+                    .replace(/"/g, '&quot;')
+                    .replace(/'/g, '&#39;');
            };
            
            // Use validated/sanitized hex for style attribute and input values
@@ -804,10 +804,14 @@
        const schedule = image.schedule || { enabled: false, mode: 'always', start_time: '08:00', end_time: '18:00', days: {} };
        
        // Escape HTML helper
+        // Quotes too: the result lands in quoted attribute values below, and the
+        // textContent/innerHTML round-trip only escapes &, < and >.
        const escapeHtml = (text) => {
            const div = document.createElement('div');
-            div.textContent = text;
-            return div.innerHTML;
+            div.textContent = String(text ?? '');
+            return div.innerHTML
+                .replace(/"/g, '&quot;')
+                .replace(/'/g, '&#39;');
        };
        
        // Use sanitizedId for all ID references in the schedule HTML
@@ -738,10 +738,15 @@
            delete btn._jfmOrigText;
        }

+        // Quotes too: the result lands in quoted attribute values (title=,
+        // data-cat=, pattern=, ...), and the textContent/innerHTML round-trip
+        // only escapes &, < and >.
        _esc(str) {
            const d = document.createElement('div');
            d.textContent = String(str ?? '');
-            return d.innerHTML;
+            return d.innerHTML
+                .replace(/"/g, '&quot;')
+                .replace(/'/g, '&#39;');
        }

        _fmtSize(bytes) {
@@ -206,10 +206,15 @@
        else console.log(`[PFM][${type}] ${msg}`);
    }

+    // Quotes too: the result lands in quoted attribute values (id=, value=,
+    // data-col=), and the textContent/innerHTML round-trip only escapes
+    // &, < and >.
    function escHtml(s) {
        const d = document.createElement('div');
        d.textContent = String(s ?? '');
-        return d.innerHTML;
+        return d.innerHTML
+            .replace(/"/g, '&quot;')
+            .replace(/'/g, '&#39;');
    }

    function formatSize(bytes) {
@@ -383,7 +388,7 @@
                <button class="pfm-btn pfm-btn-primary" id="${escHtml(fieldId)}_save_btn">
                    <i class="fas fa-save mr-1"></i>Save
                </button>
-            </div>`;
+            </div>`);
        overlay.appendChild(modal);
        // Bind events after DOM insertion — filename captured in closure, not in HTML.
        modal.querySelector(`#${CSS.escape(fieldId)}_modal_close`).addEventListener('click', () => window._pfmCloseModal(fieldId));
@@ -411,7 +416,7 @@
                <textarea id="${escHtml(fieldId)}_json_ta" rows="20"
                    style="width:100%;font-family:monospace;font-size:.75rem;border:1px solid #d1d5db;border-radius:.375rem;padding:.5rem;"
                >${escHtml(JSON.stringify(content, null, 2))}</textarea>
-                <div id="${escHtml(fieldId)}_json_err" style="color:#dc2626;font-size:.75rem;margin-top:.25rem;"></div>`;
+                <div id="${escHtml(fieldId)}_json_err" style="color:#dc2626;font-size:.75rem;margin-top:.25rem;"></div>`);
        }
    };

@@ -431,6 +436,20 @@
        if (!entries.length) { container.textContent = 'No entries.'; return; }

        const cols = Object.keys(entries[0][1]);
+
+        // Delegated listener: day/col reach _pfmCellEdit only through data-*
+        // attributes, never through a JS string spliced into an inline
+        // handler -- the browser HTML-decodes attribute values before
+        // running them as script, which undoes escHtml's quote escaping and
+        // reopens the exact injection it exists to close. `container` is a
+        // fresh element per modal open (see _pfmOpenEdit), so this attaches
+        // exactly once per table, even though buildPage() re-renders below.
+        container.addEventListener('input', (e) => {
+            const cell = e.target.closest('input[data-day], textarea[data-day]');
+            if (!cell) return;
+            window._pfmCellEdit(fieldId, cell.dataset.day, cell.dataset.col, cell.value);
+        });
+
        const MS_PER_DAY = 86400 * 1000; // eslint-disable-line no-magic-numbers -- 86400s/day is not magic
        const todayDoy = Math.ceil((new Date() - new Date(new Date().getFullYear(), 0, 0)) / MS_PER_DAY);
        const total = entries.length;
@@ -459,18 +478,16 @@
                        </thead>
                        <tbody>
                            ${pageEntries.map(([day, val]) => `
-                            <tr data-day="${day}" class="${parseInt(day) === todayDoy ? 'today-row' : ''}">
+                            <tr data-day="${escHtml(day)}" class="${parseInt(day) === todayDoy ? 'today-row' : ''}">
                                <td class="pfm-day-col" style="user-select:none;">${escHtml(day)}</td>
                                ${cols.map(col => {
                                    const v = val[col] ?? '';
                                    const isLong = String(v).length > 60 || col === 'description' || col === 'definition' || col === 'content';
                                    return isLong
-                                        ? `<td><textarea data-day="${day}" data-col="${escHtml(col)}" rows="2"
-                                            oninput="window._pfmCellEdit('${fieldId}','${day}','${escHtml(col)}',this.value)"
+                                        ? `<td><textarea data-day="${escHtml(day)}" data-col="${escHtml(col)}" rows="2"
                                            >${escHtml(String(v))}</textarea></td>`
-                                        : `<td><input type="text" data-day="${day}" data-col="${escHtml(col)}"
-                                            value="${escHtml(String(v))}"
-                                            oninput="window._pfmCellEdit('${fieldId}','${day}','${escHtml(col)}',this.value)"></td>`;
+                                        : `<td><input type="text" data-day="${escHtml(day)}" data-col="${escHtml(col)}"
+                                            value="${escHtml(String(v))}"></td>`;
                                }).join('')}
                            </tr>`).join('')}
                        </tbody>
@@ -489,7 +506,7 @@
                                ${page >= totalPages ? 'disabled' : ''}
                                onclick="window._pfmTablePage('${fieldId}',${page + 1})">Next ›</button>
                    </div>
-                </div>`;
+                </div>`);
            st._tablePage = page;
            st._tableEntries = entries;
            st._tableCols = cols;
@@ -578,7 +595,7 @@
                <button class="pfm-btn pfm-btn-danger" id="${escHtml(fieldId)}_del_confirm">
                    <i class="fas fa-trash mr-1"></i>Delete
                </button>
-            </div>`;
+            </div>`);
        overlay.appendChild(modal);
        modal.querySelector(`#${CSS.escape(fieldId)}_del_close`).addEventListener('click', () => window._pfmCloseModal(fieldId));
        modal.querySelector(`#${CSS.escape(fieldId)}_del_cancel`).addEventListener('click', () => window._pfmCloseModal(fieldId));
@@ -631,7 +648,7 @@
                    <i class="fas fa-plus mr-1"></i>Create
                </button>
            </div>
-            </div>`;
+            </div>`);
        overlay.appendChild(modal);
        modal.querySelector(`#${CSS.escape(fieldId)}_cre_close`).addEventListener('click', () => window._pfmCloseModal(fieldId));
        modal.querySelector(`#${CSS.escape(fieldId)}_cre_cancel`).addEventListener('click', () => window._pfmCloseModal(fieldId));
@@ -784,7 +801,7 @@
                    <div class="pfm-grid">
                        <div class="pfm-empty"><i class="fas fa-spinner fa-spin"></i>Loading…</div>
                    </div>
-                </div>`;
+                </div>`);

            loadFiles(fieldId);
        },
@@ -54,9 +54,16 @@
    // RFC 3986 scheme pattern: starts with letter, then letters/digits/+/./-
    const RFC_SCHEME_PATTERN = /^[A-Za-z][A-Za-z0-9+.-]*$/;

+    // Schemes that execute script when navigated to. These can never be
+    // allowed, whatever a schema's allowedProtocols asks for: the validated
+    // value is written straight into an <a href>, so allowing "javascript"
+    // here would turn a config field into script execution.
+    const SCRIPTABLE_SCHEMES = ['javascript', 'data', 'vbscript', 'blob', 'filesystem'];
+
    /**
     * Normalize and validate protocol list against RFC 3986 scheme pattern.
     * Accepts schemes like "http", "https", "git+ssh", "android-app", etc.
+     * Scriptable schemes are dropped -- see SCRIPTABLE_SCHEMES.
     * @param {Array|string} protocols - Protocol list (array or comma-separated string)
     * @returns {Array} Normalized lowercase protocols, defaults to ['http', 'https']
     */
@@ -70,15 +77,25 @@
        const normalized = list
            .map(p => String(p).trim())
            .filter(p => RFC_SCHEME_PATTERN.test(p))
-            .map(p => p.toLowerCase());
+            .map(p => p.toLowerCase())
+            .filter(p => !SCRIPTABLE_SCHEMES.includes(p));
        return normalized.length > 0 ? normalized : ['http', 'https'];
    }

+    /**
+     * True when `string` parses as a URL whose scheme is allowed AND is not
+     * one that executes script. The scriptable check is repeated here rather
+     * than trusted to normalizeProtocols so that a caller passing its own
+     * protocol list cannot re-open the hole.
+     */
    function isValidUrl(string, allowedProtocols) {
        try {
            const url = new URL(string);
+            const protocol = url.protocol.replace(':', '').toLowerCase();
+            if (SCRIPTABLE_SCHEMES.includes(protocol)) {
+                return false;
+            }
            if (allowedProtocols && allowedProtocols.length > 0) {
-                const protocol = url.protocol.replace(':', '').toLowerCase();
                return allowedProtocols.includes(protocol);
            }
            return true;
@@ -87,6 +104,15 @@
        }
    }

+    /**
+     * A value safe to use as an <a href>: the URL itself when it validates,
+     * and '' otherwise. Keeps the "render an href for whatever is stored"
+     * path from emitting a javascript: URL that was never validated.
+     */
+    function safeHref(value, allowedProtocols) {
+        return isValidUrl(value, allowedProtocols) ? value : '';
+    }
+
    window.LEDMatrixWidgets.register('url-input', {
        name: 'URL Input Widget',
        version: '1.0.0',
@@ -139,7 +165,7 @@
                html += `
                    <div id="${fieldId}_preview" class="mt-2 ${currentValue && isValidUrl(currentValue, allowedProtocols) ? '' : 'hidden'}">
                        <a id="${fieldId}_preview_link"
-                           href="${escapeHtml(currentValue)}"
+                           href="${escapeHtml(safeHref(currentValue, allowedProtocols))}"
                           target="_blank"
                           rel="noopener noreferrer"
                           class="text-sm text-blue-600 hover:text-blue-800 flex items-center">
@@ -231,10 +257,23 @@
                const protocols = normalizeProtocols(widgetEl?.dataset.protocols);

                if (previewEl && previewLink) {
-                    if (value && isValidUrl(value, protocols)) {
+                    // Scheme checked inline, right where the value reaches the DOM sink,
+                    // rather than through safeHref/isValidUrl -- CodeQL's DOM-based-XSS
+                    // sanitizer recognition does not trace a boolean-returning helper two
+                    // calls deep, so it kept flagging this assignment even though the
+                    // scriptable-scheme check (see SCRIPTABLE_SCHEMES) already covered it.
+                    let scheme = '';
+                    try {
+                        scheme = value ? new URL(value).protocol.replace(':', '').toLowerCase() : '';
+                    } catch (_) {
+                        scheme = '';
+                    }
+                    const schemeIsSafe = !!scheme && !SCRIPTABLE_SCHEMES.includes(scheme) && protocols.includes(scheme);
+                    if (schemeIsSafe) {
                        previewLink.href = value;
                        previewEl.classList.remove('hidden');
                    } else {
+                        previewLink.removeAttribute('href');
                        previewEl.classList.add('hidden');
                    }
                }
@@ -4370,12 +4370,15 @@ function renderCustomRegistryPlugins(plugins, registryUrl) {
        return;
    }

-    // Escape HTML helper
+    // Escape HTML helper. Quotes too: the result lands in quoted attribute
+    // values, and the textContent/innerHTML round-trip only escapes &, < and >.
    const escapeHtml = (text) => {
        if (!text) return '';
        const div = document.createElement('div');
        div.textContent = text;
-        return div.innerHTML;
+        return div.innerHTML
+            .replace(/"/g, '&quot;')
+            .replace(/'/g, '&#39;');
    };

    // Helper function to escape for JavaScript strings
@@ -4471,11 +4474,15 @@ function isGithubUrl(url) {
    }
 }

-// Utility function to escape HTML
+// Utility function to escape HTML. Quotes too: most call sites interpolate the
+// result into a quoted attribute value, and the textContent/innerHTML
+// round-trip only escapes &, < and >.
 function escapeHtml(text) {
    const div = document.createElement('div');
    div.textContent = text;
-    return div.innerHTML;
+    return div.innerHTML
+        .replace(/"/g, '&quot;')
+        .replace(/'/g, '&#39;');
 }

 // Utility function to escape text for use in HTML attributes
@@ -5623,11 +5630,15 @@ document.addEventListener('htmx:afterSettle', function() {
    let starlarkDataLoaded = false;

    // ── Helpers ─────────────────────────────────────────────────────────────
+    // Quotes too: the result lands in quoted attribute values (data-app-id=,
+    // title=), and the textContent/innerHTML round-trip only escapes &, < and >.
    function escapeHtml(str) {
        if (!str) return '';
        const div = document.createElement('div');
        div.textContent = str;
-        return div.innerHTML;
+        return div.innerHTML
+            .replace(/"/g, '&quot;')
+            .replace(/'/g, '&#39;');
    }

    function isStarlarkInstalled(appId) {