From 804408428010e00ac42835fff9617e0a77953991 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Thu, 16 Jul 2026 14:17:44 -0400 Subject: [PATCH] =?UTF-8?q?fix(web):=20address=20CodeRabbit=20review=20?= =?UTF-8?q?=E2=80=94=20validation,=20a11y,=20perf,=20and=20privacy=20fixes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verified each finding against current code. Fixed: - api_v3: plugin_rotation_order is now strictly validated (JSON list of strings, 400 with a descriptive message otherwise) and popped from the payload before any further handling. - display_controller: _apply_plugin_rotation_order defensively ignores a non-list value (keeps the existing rotation, logs a warning) and drops non-string entries; new logs carry the [DisplayController] prefix. Unit-tested both defensive paths. - app.py: snapshot-read handler narrowed to OSError with debug logging; flask-compress ImportError now emits one structured warning with the install remedy. - htmx-config: the response-error logger prints form FIELD NAMES only - values (API keys, passwords) never reach the console. - plugin-order-list: saved order/exclusions normalized with Array.isArray (a saved "null" previously crashed .forEach); each row gained keyboard/touch-accessible move-up/move-down buttons (HTML5 drag events don't fire on most mobile browsers) that reorder and syncInputs() immediately alongside native drag. - app-shell: window.installedPlugins setter always takes the new list (same-ID metadata/enabled updates were silently dropped); tab rebuild stays gated on ID changes. LED dot renderer reads the frame with ONE getImageData call instead of one per pixel (~9,200/frame at 192x48). - plugins_manager: togglePlugin returns its request promise resolving the API outcome; the install flow now shows the "installed and enabled" toast (with Restart Now) only after enablement succeeds, and a warning without a restart offer when it fails. - a11y: hamburger aria-label flips Open/Close with drawer state; both Advanced-section toggle buttons declare aria-controls/aria-expanded and the shared toggleSection() keeps aria-expanded in sync; move buttons have per-plugin aria-labels. - Rotation/Vegas order-list bootstraps cap their retries (~5s) and show a reload hint instead of spinning forever; Alpine app-state lookups prefer [x-data="app()"] with a generic fallback. Skipped, with reasons: - executePluginAction arg order: caller (plugin_config.html) already passes (actionId, index, pluginId) matching the signature exactly. - generateFieldHtml XSS, entity-unescape blocks, dotToNested pollution, and "app.loadInstalledPlugins" in app-shell: all inside the legacy client-side config cluster whose entry points are shadowed by plugins_manager.js / replaced by server-rendered forms (zero live callers, verified) - queued for wholesale deletion in the follow-up rather than patching dead code. - custom-feeds-helpers.js findings (3): file was deleted in a prior commit. - console.error/warn override removal and afterSwap script re-execution removal: deliberate pre-existing workarounds every partial's inline init currently depends on; reworking them safely needs isolated testing (follow-up), and the error suppression is already double-gated (insertBefore AND htmx match). - "move durations bootstrap into a bundle": inline partial-scoped init is the established pattern for HTMX partials in this codebase. Validation: all 40 web tests pass; py_compile on all touched Python; all touched templates parse; rotation-order defensive paths unit-tested. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ --- src/display_controller.py | 10 ++++- web_interface/app.py | 12 ++++-- web_interface/blueprints/api_v3.py | 25 +++++++------ web_interface/static/v3/app.js | 4 +- web_interface/static/v3/js/app-shell.js | 26 ++++++++++--- web_interface/static/v3/js/htmx-config.js | 19 +++++++--- .../static/v3/js/widgets/plugin-order-list.js | 29 +++++++++++++++ web_interface/static/v3/plugins_manager.js | 37 ++++++++++++------- web_interface/templates/v3/base.html | 1 + .../templates/v3/partials/display.html | 18 ++++++--- .../templates/v3/partials/durations.html | 14 +++++-- .../templates/v3/partials/overview.html | 2 +- .../templates/v3/partials/plugin_config.html | 2 + 13 files changed, 148 insertions(+), 51 deletions(-) diff --git a/src/display_controller.py b/src/display_controller.py index 21fd79cf..a4834102 100644 --- a/src/display_controller.py +++ b/src/display_controller.py @@ -2866,6 +2866,14 @@ class DisplayController: get_ordered_plugins() semantics for the primary rotation. """ configured = (self.config.get("display", {}) or {}).get("plugin_rotation_order", []) or [] + # Defensive: hand-edited or migrated configs may hold a non-list or + # non-string entries; keep the existing rotation rather than applying + # a garbage order. + if not isinstance(configured, list): + logger.warning("[DisplayController] Ignoring invalid plugin_rotation_order (not a list): %r", + type(configured).__name__) + return + configured = [p for p in configured if isinstance(p, str)] if not configured or not self.available_modes: return @@ -2882,7 +2890,7 @@ class DisplayController: new_modes.append(mode) if new_modes != self.available_modes: self.available_modes = new_modes - logger.info("Applied plugin rotation order %s -> modes: %s", + logger.info("[DisplayController] Applied plugin rotation order %s -> modes: %s", configured, self.available_modes) def _resync_mode_index_after_change(self, previous_mode: Optional[str]) -> None: diff --git a/web_interface/app.py b/web_interface/app.py index e40fac9a..74ebcfe0 100644 --- a/web_interface/app.py +++ b/web_interface/app.py @@ -67,7 +67,11 @@ try: Compress(app) except ImportError: - pass + logging.getLogger(__name__).warning( + "flask-compress not installed - responses will be served uncompressed. " + "Install it with the Tools tab's 'Install Base Requirements' button or " + "'pip install flask-compress'." + ) # Import cache functions from separate module to avoid circular imports @@ -682,8 +686,10 @@ def display_preview_generator(): } last_modified = current_modified yield preview_data - except Exception: # nosec B110 - transient read error (e.g. file rotated away); skip this update - pass + except OSError: + # Transient filesystem race (file rotated/replaced + # between mtime check and read); skip this update. + app.logger.debug("Preview snapshot read failed; skipping frame", exc_info=True) else: # No snapshot available yield { diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index 3d0545bb..01fa6e8f 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -961,21 +961,22 @@ def save_main_config(): return jsonify({"status": "error", "message": "sync_follower_position must be left or right"}), 400 current_config["sync"]["follower_position"] = pos_val - # Handle primary rotation order (JSON array of plugin ids; same - # parse/validate pattern as vegas_plugin_order above) + # Handle primary rotation order: must be a JSON array of plugin-id + # strings. Reject anything else with a 400 rather than silently + # coercing, so a buggy client can't clear or corrupt the saved order. if 'plugin_rotation_order' in data: + raw_order = data.pop('plugin_rotation_order') try: - if isinstance(data['plugin_rotation_order'], str): - parsed = json.loads(data['plugin_rotation_order']) - else: - parsed = data['plugin_rotation_order'] - if 'display' not in current_config: - current_config['display'] = {} - current_config['display']['plugin_rotation_order'] = ( - list(parsed) if isinstance(parsed, (list, tuple)) else [] - ) + parsed = json.loads(raw_order) if isinstance(raw_order, str) else raw_order except (json.JSONDecodeError, TypeError, ValueError): - logger.warning("Malformed plugin_rotation_order ignored") + return jsonify({'status': 'error', + 'message': 'plugin_rotation_order must be valid JSON'}), 400 + if not isinstance(parsed, list) or not all(isinstance(p, str) for p in parsed): + return jsonify({'status': 'error', + 'message': 'plugin_rotation_order must be a list of plugin-id strings'}), 400 + if 'display' not in current_config: + current_config['display'] = {} + current_config['display']['plugin_rotation_order'] = parsed # Handle display durations duration_fields = [k for k in data.keys() if k.endswith('_duration') or k in ['default_duration', 'transition_duration']] diff --git a/web_interface/static/v3/app.js b/web_interface/static/v3/app.js index f9660787..c84cb4b0 100644 --- a/web_interface/static/v3/app.js +++ b/web_interface/static/v3/app.js @@ -395,7 +395,7 @@ window.updateFloatingPreviewVisibility = function(tab) { if (!panel || !toggle) return; let active = tab; if (!active) { - const el = document.querySelector('[x-data]'); + const el = document.querySelector('[x-data="app()"]') || document.querySelector('[x-data]'); const data = el && el._x_dataStack && el._x_dataStack[0]; active = data && data.activeTab; } @@ -449,7 +449,7 @@ window.updateNavAriaCurrent = function(tab) { // opening the drawer moves focus to its first tab. (function() { function appData() { - const el = document.querySelector('[x-data]'); + const el = document.querySelector('[x-data="app()"]') || document.querySelector('[x-data]'); return el && el._x_dataStack && el._x_dataStack[0]; } document.addEventListener('keydown', function(e) { diff --git a/web_interface/static/v3/js/app-shell.js b/web_interface/static/v3/js/app-shell.js index e3857de6..0abfada8 100644 --- a/web_interface/static/v3/js/app-shell.js +++ b/web_interface/static/v3/js/app-shell.js @@ -361,12 +361,15 @@ const newPlugins = value || []; const oldIds = (_installedPluginsValue || []).map(p => p.id).sort().join(','); const newIds = newPlugins.map(p => p.id).sort().join(','); - - // Only update if plugin list actually changed + + // Always take the new list — same-ID updates + // still carry changed metadata/enabled state. + _installedPluginsValue = newPlugins; + this.installedPlugins = newPlugins; + // Only rebuild the tab row when the ID set + // actually changed. if (oldIds !== newIds) { debugLog('window.installedPlugins changed:', newPlugins.length, 'plugins'); - _installedPluginsValue = newPlugins; - this.installedPlugins = newPlugins; this.updatePluginTabs(); } }, @@ -1983,12 +1986,23 @@ ctx.fillStyle = 'rgb(0, 0, 0)'; ctx.fillRect(0, 0, ledCanvas.width, ledCanvas.height); + // Read the whole frame once instead of one getImageData call per + // pixel (a 192x48 panel would otherwise issue ~9,200 calls per + // frame — noticeably heavy on phones). + let frame; + try { + frame = offCtx.getImageData(0, 0, logicalWidth, logicalHeight).data; + } catch (e) { + console.error('Failed to read offscreen canvas pixels:', e); + return; + } + // Draw circular dots for each LED pixel let drawn = 0; for (let y = 0; y < logicalHeight; y++) { for (let x = 0; x < logicalWidth; x++) { - const pixel = offCtx.getImageData(x, y, 1, 1).data; - const r = pixel[0], g = pixel[1], b = pixel[2], a = pixel[3]; + const i = (y * logicalWidth + x) * 4; + const r = frame[i], g = frame[i + 1], b = frame[i + 2], a = frame[i + 3]; // Skip fully transparent or black pixels to reduce overdraw if (a === 0 || (r|g|b) === 0) continue; diff --git a/web_interface/static/v3/js/htmx-config.js b/web_interface/static/v3/js/htmx-config.js index 62da9584..23a3c8fd 100644 --- a/web_interface/static/v3/js/htmx-config.js +++ b/web_interface/static/v3/js/htmx-config.js @@ -130,14 +130,16 @@ responseText: xhr?.responseText }); - // For form submissions, log the form data + // For form submissions, log field names only — values + // may contain API keys, passwords, or other secrets + // that must never reach the console. if (target && target.tagName === 'FORM') { const formData = new FormData(target); - const formPayload = Object.create(null); - for (const [key, value] of formData.entries()) { - formPayload[key] = value; + const fieldNames = []; + for (const [key] of formData.entries()) { + fieldNames.push(key); } - console.error('Form payload:', formPayload); + console.error('Form fields (values redacted):', fieldNames); // Try to parse error response for validation details if (xhr?.responseText) { @@ -243,5 +245,12 @@ icon.classList.remove('fa-chevron-down'); icon.classList.add('fa-chevron-right'); } + + // Keep assistive tech in sync: any toggle button that declares + // aria-controls for this section mirrors the expanded state. + const controlBtn = document.querySelector(`[aria-controls="${sectionId}"]`); + if (controlBtn) { + controlBtn.setAttribute('aria-expanded', String(isHidden)); + } }; })(); diff --git a/web_interface/static/v3/js/widgets/plugin-order-list.js b/web_interface/static/v3/js/widgets/plugin-order-list.js index 95275982..f4610b90 100644 --- a/web_interface/static/v3/js/widgets/plugin-order-list.js +++ b/web_interface/static/v3/js/widgets/plugin-order-list.js @@ -115,6 +115,10 @@ } catch (e) { console.error('Error parsing saved plugin order:', e); } + // JSON.parse can succeed and still return null/objects + // (e.g. a saved value of "null"); normalize to arrays. + if (!Array.isArray(currentOrder)) currentOrder = []; + if (!Array.isArray(excluded)) excluded = []; // Saved order first, then any newly enabled plugins. const orderedPlugins = []; @@ -173,6 +177,31 @@ row.appendChild(badge); } + // Up/down buttons: touch- and keyboard-accessible + // reordering alongside native drag-and-drop (HTML5 drag + // events don't fire on most mobile browsers). + const pluginLabel = plugin.name || plugin.id; + [['up', 'fa-chevron-up', `Move ${pluginLabel} up`], + ['down', 'fa-chevron-down', `Move ${pluginLabel} down`]].forEach(([dir, iconCls, ariaLabel]) => { + const moveBtn = document.createElement('button'); + moveBtn.type = 'button'; + moveBtn.className = 'plugin-order-move text-gray-400 hover:text-gray-700 px-2 py-1'; + moveBtn.setAttribute('aria-label', ariaLabel); + const moveIcon = document.createElement('i'); + moveIcon.className = `fas ${iconCls} text-xs`; + moveBtn.appendChild(moveIcon); + moveBtn.addEventListener('click', function() { + if (dir === 'up' && row.previousElementSibling) { + container.insertBefore(row, row.previousElementSibling); + } else if (dir === 'down' && row.nextElementSibling) { + container.insertBefore(row.nextElementSibling, row); + } + syncInputs(); + moveBtn.focus(); + }); + row.appendChild(moveBtn); + }); + container.appendChild(row); }); diff --git a/web_interface/static/v3/plugins_manager.js b/web_interface/static/v3/plugins_manager.js index 9b740c81..fb7b0ae4 100644 --- a/web_interface/static/v3/plugins_manager.js +++ b/web_interface/static/v3/plugins_manager.js @@ -317,7 +317,7 @@ window.togglePlugin = window.togglePlugin || function(pluginId, enabled) { showNotification(`${action.charAt(0).toUpperCase() + action.slice(1)} ${pluginName}...`, 'info'); } - fetch('/api/v3/plugins/toggle', { + return fetch('/api/v3/plugins/toggle', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ plugin_id: pluginId, enabled: enabled }) @@ -361,6 +361,9 @@ window.togglePlugin = window.togglePlugin || function(pluginId, enabled) { if (wrapperDiv) { wrapperDiv.classList.remove('opacity-50', 'pointer-events-none'); } + // Resolve the outcome so callers (e.g. the install flow) can chain + // on whether enabling actually succeeded. + return data; }) .catch(error => { // Verify this error is for the latest request (prevent race conditions) @@ -3889,19 +3892,27 @@ window.installPlugin = function(pluginId, branch = null) { .then(data => { showNotification(data.message, data.status); if (data.status === 'success') { - // Enable immediately so install -> enable is one step, then nudge - // for the display restart that's needed before it shows on the - // matrix (persistent toast; duration 0 = stays until dismissed). - window.togglePlugin(pluginId, true); - showNotification( - `${pluginId} installed and enabled — restart the display to show it`, - { - type: 'success', - duration: 0, - actionLabel: 'Restart Now', - onAction: () => restartDisplay() + // Enable immediately so install -> enable is one step; only nudge + // for a restart once enablement actually succeeded (persistent + // toast; duration 0 = stays until dismissed). + Promise.resolve(window.togglePlugin(pluginId, true)).then(toggleResult => { + if (toggleResult && toggleResult.status === 'success') { + showNotification( + `${pluginId} installed and enabled — restart the display to show it`, + { + type: 'success', + duration: 0, + actionLabel: 'Restart Now', + onAction: () => restartDisplay() + } + ); + } else { + showNotification( + `${pluginId} installed, but enabling it failed — use its toggle in the plugin list`, + 'warning' + ); } - ); + }); // Refresh installed plugins list, then re-render store to update badges loadInstalledPlugins(); setTimeout(() => applyStoreFiltersAndSort(true), 500); diff --git a/web_interface/templates/v3/base.html b/web_interface/templates/v3/base.html index 7be1384b..d54d8cc1 100644 --- a/web_interface/templates/v3/base.html +++ b/web_interface/templates/v3/base.html @@ -302,6 +302,7 @@ @click="mobileNavOpen = !mobileNavOpen" :aria-expanded="mobileNavOpen ? 'true' : 'false'" aria-controls="site-nav" + :aria-label="mobileNavOpen ? 'Close navigation menu' : 'Open navigation menu'" aria-label="Open navigation menu" title="Menu"> diff --git a/web_interface/templates/v3/partials/display.html b/web_interface/templates/v3/partials/display.html index 8199d10d..463da515 100644 --- a/web_interface/templates/v3/partials/display.html +++ b/web_interface/templates/v3/partials/display.html @@ -137,6 +137,8 @@