mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-01 08:48:05 +00:00
fix(web): address CodeRabbit review — validation, a11y, perf, and privacy fixes
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
This commit is contained in:
ChuckBuilds
co-authored by
Claude Sonnet 5
Claude Sonnet 5
parent
aec1368d63
commit
8044084280
@@ -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:
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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']
|
||||
parsed = json.loads(raw_order) if isinstance(raw_order, str) else raw_order
|
||||
except (json.JSONDecodeError, TypeError, ValueError):
|
||||
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'] = (
|
||||
list(parsed) if isinstance(parsed, (list, tuple)) else []
|
||||
)
|
||||
except (json.JSONDecodeError, TypeError, ValueError):
|
||||
logger.warning("Malformed plugin_rotation_order ignored")
|
||||
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']]
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -362,11 +362,14 @@
|
||||
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
|
||||
if (oldIds !== newIds) {
|
||||
debugLog('window.installedPlugins changed:', newPlugins.length, 'plugins');
|
||||
// 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');
|
||||
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;
|
||||
|
||||
@@ -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));
|
||||
}
|
||||
};
|
||||
})();
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
|
||||
|
||||
@@ -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,10 +3892,11 @@ 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);
|
||||
// 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`,
|
||||
{
|
||||
@@ -3902,6 +3906,13 @@ window.installPlugin = function(pluginId, branch = null) {
|
||||
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);
|
||||
|
||||
@@ -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">
|
||||
<i class="fas fa-bars" aria-hidden="true"></i>
|
||||
|
||||
@@ -137,6 +137,8 @@
|
||||
<div class="nested-section border border-gray-300 rounded-lg mt-4">
|
||||
<button type="button"
|
||||
class="w-full bg-gray-100 hover:bg-gray-200 px-4 py-3 flex items-center justify-between text-left transition-colors rounded-t-lg"
|
||||
aria-controls="display-section-advanced-hardware"
|
||||
aria-expanded="false"
|
||||
onclick="toggleSection('display-section-advanced-hardware')">
|
||||
<div class="flex-1">
|
||||
<h4 class="font-semibold text-gray-900">
|
||||
@@ -642,12 +644,18 @@ if (typeof window.fixInvalidNumberInputs !== 'function') {
|
||||
// Initialize plugin order list via the shared drag-and-drop module
|
||||
// (static/v3/js/widgets/plugin-order-list.js) — the same component the
|
||||
// Durations tab uses for the primary rotation order.
|
||||
function initPluginOrderList() {
|
||||
if (!document.getElementById('vegas_plugin_order')) return;
|
||||
function initPluginOrderList(attempt) {
|
||||
const container = document.getElementById('vegas_plugin_order');
|
||||
if (!container) return;
|
||||
if (!window.PluginOrderList) {
|
||||
// Widget script is deferred; retry briefly if this partial's
|
||||
// inline script runs first.
|
||||
setTimeout(initPluginOrderList, 100);
|
||||
// Widget script is deferred; retry briefly, then surface a real
|
||||
// error instead of waiting forever.
|
||||
if ((attempt || 0) < 50) {
|
||||
setTimeout(function() { initPluginOrderList((attempt || 0) + 1); }, 100);
|
||||
} else {
|
||||
container.textContent = 'Could not load the reorder widget — reload the page to try again.';
|
||||
container.className = 'text-sm text-red-500';
|
||||
}
|
||||
return;
|
||||
}
|
||||
window.PluginOrderList.init({
|
||||
|
||||
@@ -79,10 +79,18 @@
|
||||
(function () {
|
||||
// Shared drag-and-drop plugin list (static/v3/js/widgets/plugin-order-list.js,
|
||||
// same module the Vegas Scroll section uses).
|
||||
function initRotationOrderList() {
|
||||
if (!document.getElementById('rotation_plugin_order')) return;
|
||||
function initRotationOrderList(attempt) {
|
||||
const container = document.getElementById('rotation_plugin_order');
|
||||
if (!container) return;
|
||||
if (!window.PluginOrderList) {
|
||||
setTimeout(initRotationOrderList, 100);
|
||||
// Widget script is deferred; retry briefly, then surface a real
|
||||
// error instead of showing "Loading…" forever.
|
||||
if ((attempt || 0) < 50) {
|
||||
setTimeout(function() { initRotationOrderList((attempt || 0) + 1); }, 100);
|
||||
} else {
|
||||
container.textContent = 'Could not load the reorder widget — reload the page to try again.';
|
||||
container.className = 'text-sm text-red-500';
|
||||
}
|
||||
return;
|
||||
}
|
||||
window.PluginOrderList.init({
|
||||
|
||||
@@ -152,7 +152,7 @@
|
||||
if (nowDone) maybeAutoHide();
|
||||
return;
|
||||
}
|
||||
var appEl = document.querySelector('[x-data]');
|
||||
var appEl = document.querySelector('[x-data="app()"]') || document.querySelector('[x-data]');
|
||||
var data = appEl && appEl._x_dataStack && appEl._x_dataStack[0];
|
||||
if (data) {
|
||||
data.activeTab = btn.dataset.tab;
|
||||
|
||||
@@ -1038,6 +1038,8 @@
|
||||
<div class="nested-section border border-gray-300 rounded-lg mb-4">
|
||||
<button type="button"
|
||||
class="w-full bg-gray-100 hover:bg-gray-200 px-4 py-3 flex items-center justify-between text-left transition-colors rounded-t-lg"
|
||||
aria-controls="{{ adv_section_id }}"
|
||||
aria-expanded="false"
|
||||
onclick="toggleSection('{{ adv_section_id }}')">
|
||||
<div class="flex-1">
|
||||
<h4 class="font-semibold text-gray-900">
|
||||
|
||||
Reference in New Issue
Block a user