fix: service control buttons and AP-mode SSH lockout post-install (#326)

* fix: service control buttons and AP-mode SSH lockout post-install

Two user-reported issues after fresh install:

1. All service buttons (Start/Stop/Restart Display, Restart Web Service)
   failed silently — only Reboot worked.

   Root cause: sudoers rules use `ledmatrix.service` (with suffix) but
   api_v3.py called `sudo systemctl start ledmatrix` (no suffix). sudo
   does exact string matching, so every service action was rejected with
   returncode=1. Also missing from sudoers: ledmatrix-web, journalctl,
   and is-active entries.

   Fix:
   - Add `.service` suffix to all 8 sudo systemctl call sites in
     api_v3.py (_ensure_display_service_running, _stop_display_service,
     and all execute_system_action branches).
   - Add timeout=15 to all subprocess.run calls in execute_system_action
     (previously could hang indefinitely).
   - Add missing sudoers rules to first_time_install.sh and
     configure_web_sudo.sh: ledmatrix-web.service start/stop/restart,
     is-active for both name forms, and journalctl -u/-t ledmatrix rules.

2. SSH and web UI became inaccessible after ~1 hour even though the
   display kept running.

   Root cause: wifi_monitor_daemon restarts NetworkManager after 5
   consecutive internet failures (~2.5 min). Each NM restart drops WiFi
   briefly. During that window check_and_manage_ap_mode() increments
   _disconnected_checks but the daemon never reset it after the restart.
   After 3 such NM-restart cycles, _disconnected_checks reached 3 and
   AP mode activated — changing the Pi from WiFi client to hotspot
   (192.168.4.1) and killing SSH on the old IP.

   Fix:
   - Reset wifi_manager._disconnected_checks = 0 in the daemon
     immediately after a successful NM restart so the brief drop it
     causes doesn't count toward AP-mode activation.
   - Increase _disconnected_checks_required from 3 to 6 (90s → 3min)
     as an additional buffer against transient network flaps.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* revert: restore AP-mode grace period to 90s (3 checks)

The counter reset after NM restart already fully prevents the SSH-lockout
cascade: _disconnected_checks can never accumulate across NM restarts
because it is reset to 0 before the next daemon iteration runs.

The 3→6 increase provided no additional fix for the described problem and
caused a UX regression: fresh Pi devices with no WiFi configured would
wait 3 minutes instead of 90 seconds for the LEDMatrix-Setup hotspot to
appear.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix: address five valid review findings; skip two

Fixed:
- march-madness/requirements.txt: Pillow>=10.3.0 (patches CVE-2024-28219;
  10.3.0 is the actual fix version — reviewer cited 12.2.0 but that risks
  breaking API changes without test coverage)
- wifi_monitor_daemon.py: add missing `import subprocess`; subprocess.run
  and CalledProcessError would NameError at runtime on the NM restart path
- wifi_manager.py: validate ap_idle_timeout_minutes before arithmetic —
  coerce to int, clamp 1–1440, fall back to 15 on bad config values
- wifi_manager.py: call _remove_nm_dnsmasq_captive_conf() on all three
  rollback paths in _enable_ap_mode_nmcli_hotspot() and in the top-level
  except block so stale dnsmasq drop-ins are never left behind
- api_v3.py: fix wrong_password prefix strip — removeprefix("wrong_password:")
  then lstrip() handles both "wrong_password: msg" and "wrong_password:msg"
- plugins_manager.js: add .catch() to loadInstalledPlugins().then() to
  surface failures instead of silently dropping unhandled rejections

Skipped:
- WiFiManager AP state persistence: architectural overhaul; _is_ap_mode_active()
  already derives from live system state, not in-memory variables
- Absolute subprocess paths in api_v3.py: paths vary by distro (/usr/bin vs
  /bin); web service has a normal PATH; sudoers already use resolved paths

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix: address five review findings (NM retry loop, start_display message, code quality)

- wifi_monitor_daemon: reset _consecutive_internet_failures = 0 in both
  NM-restart exception handlers; previously both left the counter at threshold,
  causing an immediate retry on the next iteration instead of waiting another
  full backoff period

- api_v3: fix start_display failure message — when mode is set and systemctl
  returns non-zero, message now includes the failure reason and a hint rather
  than always reporting success phrasing

- wifi_manager: move _redirect_backend from class variable to instance variable
  in __init__ alongside _ap_enabled_at; class-level default shadowed correctly
  in practice (single instance) but was misleading

- wifi_manager: narrow broad except Exception in _check_internet_connectivity
  to (subprocess.SubprocessError, OSError) for ping and OSError for HTTP
  (urllib.error.URLError is an OSError subclass in Python 3)

- wifi_manager: remove redundant local 'import re as _re' in _validate_ap_config;
  re is already imported at module level (line 37)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix: address five review findings (Pillow CVEs, daemon exception narrowing, timeout handling, plugin store)

- march-madness/requirements.txt: Pillow>=12.2.0 (patches CVE-2026-42308
  and CVE-2026-42310; previous floor of 10.3.0 was insufficient)

- wifi_monitor_daemon: narrow final except Exception to
  (subprocess.SubprocessError, OSError) so programming errors in the NM
  restart block are no longer silently swallowed

- api_v3/execute_system_action: add explicit subprocess.TimeoutExpired
  handler before the generic Exception catch; returns action-specific
  message with 'status','message','returncode','stdout','stderr' fields
  so the UI receives a precise, actionable payload instead of the generic
  'Failed to execute system action' string

- plugins_manager.js: move searchPluginStore into .finally() so the
  plugin store renders regardless of whether loadInstalledPlugins succeeds
  or fails; .catch() still logs the error

- first_time_install.sh: add safe_plugin_rm.sh NOPASSWD rule to the
  /tmp/ledmatrix_web_sudoers block; configure_web_sudo.sh had this rule
  but the standalone installer never granted it, leaving plugin removal
  broken after first-time install

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* refactor(api): resolve sudo/systemctl/reboot/poweroff paths at startup

Use shutil.which() with safe fallbacks for the four privileged binaries
instead of relying on bare names being resolved by the subprocess shell
search. Resolves paths once at module load rather than per-call.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Chuck <chuck@example.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

README.md

@@ -1,4 +1,5 @@
 # LEDMatrix
+[![Codacy Badge](https://app.codacy.com/project/badge/Grade/77fc9b446a5948e5b0aed7a7aaeb1bab)](https://app.codacy.com/gh/ChuckBuilds/LEDMatrix/dashboard?utm_source=gh&utm_medium=referral&utm_content=&utm_campaign=Badge_grade)
 ## Welcome to LEDMatrix! 
 Welcome to the LEDMatrix Project! This open-source project enables you to run an information-rich display on a Raspberry Pi connected to an LED RGB Matrix panel. Whether you want to see your calendar, weather forecasts, sports scores, stock prices, or any other information at a glance, LEDMatrix brings it all together.
 

first_time_install.sh

@@ -1110,6 +1110,7 @@ $ACTUAL_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart ledmatrix-web.service
 $ACTUAL_USER ALL=(ALL) NOPASSWD: $PYTHON_PATH $PROJECT_ROOT_DIR/display_controller.py
 $ACTUAL_USER ALL=(ALL) NOPASSWD: $BASH_PATH $PROJECT_ROOT_DIR/start_display.sh
 $ACTUAL_USER ALL=(ALL) NOPASSWD: $BASH_PATH $PROJECT_ROOT_DIR/stop_display.sh
+$ACTUAL_USER ALL=(ALL) NOPASSWD: $BASH_PATH $PROJECT_ROOT_DIR/scripts/fix_perms/safe_plugin_rm.sh *
 EOF
 if [ -n "$JOURNALCTL_PATH" ]; then
     cat >> /tmp/ledmatrix_web_sudoers << EOF

plugin-repos/march-madness/requirements.txt

@@ -1,4 +1,4 @@
 requests>=2.28.0
-Pillow>=10.3.0
+Pillow>=12.2.0
 pytz>=2022.1
 numpy>=1.24.0

scripts/utils/wifi_monitor_daemon.py

@@ -155,7 +155,7 @@ class WiFiMonitorDaemon:
                                 logger.error(f"NetworkManager restart failed (rc={e.returncode}); "
                                              "resetting failure counter to avoid tight retry loop")
                                 self._consecutive_internet_failures = 0
-                            except Exception as e:
+                            except (subprocess.SubprocessError, OSError) as e:
                                 logger.error(f"NetworkManager restart error: {e}; "
                                              "resetting failure counter to avoid tight retry loop")
                                 self._consecutive_internet_failures = 0

web_interface/blueprints/api_v3.py

@@ -2,6 +2,7 @@ from flask import Blueprint, request, jsonify, Response, send_from_directory
 import json
 import os
 import re
+import shutil
 import socket
 import sys
 import subprocess
@@ -16,6 +17,11 @@ from typing import Optional, Tuple, Dict, Any, Type
 
 logger = logging.getLogger(__name__)
 
+SUDO_BIN = shutil.which("sudo") or "/usr/bin/sudo"
+SYSTEMCTL_BIN = shutil.which("systemctl") or "/usr/bin/systemctl"
+REBOOT_BIN = shutil.which("reboot") or "/usr/sbin/reboot"
+POWEROFF_BIN = shutil.which("poweroff") or "/usr/sbin/poweroff"
+
 # Import new infrastructure
 from src.web_interface.api_helpers import success_response, error_response, validate_request_json
 from src.web_interface.errors import ErrorCode
@@ -218,7 +224,7 @@ def _ensure_display_service_running():
     if status.get('active'):
         status['started'] = False
         return status
-    result = _run_systemctl_command(['sudo', 'systemctl', 'start', 'ledmatrix.service'])
+    result = _run_systemctl_command([SUDO_BIN, SYSTEMCTL_BIN, 'start', 'ledmatrix.service'])
     service_status = _get_display_service_status()
     result['started'] = result.get('returncode') == 0
     result['active'] = service_status.get('active')
@@ -227,7 +233,7 @@ def _ensure_display_service_running():
 
 def _stop_display_service():
     """Stop the ledmatrix display service."""
-    result = _run_systemctl_command(['sudo', 'systemctl', 'stop', 'ledmatrix.service'])
+    result = _run_systemctl_command([SUDO_BIN, SYSTEMCTL_BIN, 'stop', 'ledmatrix.service'])
     status = _get_display_service_status()
     result['active'] = status.get('active')
     result['status'] = status
@@ -1716,7 +1722,7 @@ def execute_system_action():
             if mode:
                 # For on-demand modes, we would need to integrate with the display controller
                 # For now, just start the display service
-                result = subprocess.run(['sudo', 'systemctl', 'start', 'ledmatrix.service'],
+                result = subprocess.run([SUDO_BIN, SYSTEMCTL_BIN, 'start', 'ledmatrix.service'],
                                      capture_output=True, text=True, timeout=15)
                 return jsonify({
                     'status': 'success' if result.returncode == 0 else 'error',
@@ -1727,22 +1733,22 @@ def execute_system_action():
                     'stderr': result.stderr
                 })
             else:
-                result = subprocess.run(['sudo', 'systemctl', 'start', 'ledmatrix.service'],
+                result = subprocess.run([SUDO_BIN, SYSTEMCTL_BIN, 'start', 'ledmatrix.service'],
                                      capture_output=True, text=True, timeout=15)
         elif action == 'stop_display':
-            result = subprocess.run(['sudo', 'systemctl', 'stop', 'ledmatrix.service'],
+            result = subprocess.run([SUDO_BIN, SYSTEMCTL_BIN, 'stop', 'ledmatrix.service'],
                                  capture_output=True, text=True, timeout=15)
         elif action == 'enable_autostart':
-            result = subprocess.run(['sudo', 'systemctl', 'enable', 'ledmatrix.service'],
+            result = subprocess.run([SUDO_BIN, SYSTEMCTL_BIN, 'enable', 'ledmatrix.service'],
                                  capture_output=True, text=True, timeout=15)
         elif action == 'disable_autostart':
-            result = subprocess.run(['sudo', 'systemctl', 'disable', 'ledmatrix.service'],
+            result = subprocess.run([SUDO_BIN, SYSTEMCTL_BIN, 'disable', 'ledmatrix.service'],
                                  capture_output=True, text=True, timeout=15)
         elif action == 'reboot_system':
-            result = subprocess.run(['sudo', 'reboot'],
+            result = subprocess.run([SUDO_BIN, REBOOT_BIN],
                                  capture_output=True, text=True, timeout=10)
         elif action == 'shutdown_system':
-            result = subprocess.run(['sudo', 'poweroff'],
+            result = subprocess.run([SUDO_BIN, POWEROFF_BIN],
                                  capture_output=True, text=True, timeout=10)
         elif action == 'git_pull':
             # Use PROJECT_ROOT instead of hardcoded path
@@ -1824,10 +1830,10 @@ def execute_system_action():
                 'stderr': result.stderr
             })
         elif action == 'restart_display_service':
-            result = subprocess.run(['sudo', 'systemctl', 'restart', 'ledmatrix.service'],
+            result = subprocess.run([SUDO_BIN, SYSTEMCTL_BIN, 'restart', 'ledmatrix.service'],
                                  capture_output=True, text=True, timeout=15)
         elif action == 'restart_web_service':
-            result = subprocess.run(['sudo', 'systemctl', 'restart', 'ledmatrix-web.service'],
+            result = subprocess.run([SUDO_BIN, SYSTEMCTL_BIN, 'restart', 'ledmatrix-web.service'],
                                  capture_output=True, text=True, timeout=15)
         else:
             return jsonify({'status': 'error', 'message': f'Unknown action: {action}'}), 400
@@ -1840,6 +1846,13 @@ def execute_system_action():
             'stderr': result.stderr
         })
 
+    except subprocess.TimeoutExpired:
+        if action == 'start_display' and mode:
+            msg = f'Failed to start display in {mode} mode: timed out'
+        else:
+            msg = f'Action {action} timed out'
+        logger.warning("[System] execute_system_action timed out: action=%s", action)
+        return jsonify({'status': 'error', 'message': msg, 'returncode': -1, 'stdout': '', 'stderr': 'timeout'}), 500
     except Exception as e:
         logger.exception("[System] execute_system_action failed")
         return jsonify({'status': 'error', 'message': 'Failed to execute system action'}), 500

web_interface/blueprints/pages_v3.py

@@ -1,7 +1,6 @@
 from flask import Blueprint, render_template, request, redirect, url_for, flash, jsonify
 import json
 import logging
-from html import escape as html_escape
 from pathlib import Path
 from src.web_interface.secret_helpers import mask_secret_fields
 
@@ -355,7 +354,7 @@ def _load_plugin_config_partial(plugin_id):
             plugin_info = pages_v3.plugin_manager.get_plugin_info(plugin_id)
         
         if not plugin_info:
-            return f'<div class="text-red-500 p-4">Plugin "{html_escape(plugin_id)}" not found</div>', 404
+            return f'<div class="text-red-500 p-4">Plugin "{plugin_id}" not found</div>', 404
         
         # Get plugin instance (may be None if not loaded)
         plugin_instance = pages_v3.plugin_manager.get_plugin(plugin_id)

web_interface/static/v3/js/widgets/day-selector.js

@@ -91,7 +91,7 @@
             const xOptions = config['x-options'] || config['x_options'] || {};
             const requestedFormat = xOptions.format || 'long';
             // Validate format exists in DAY_LABELS, default to 'long' if not
-            const format = Object.prototype.hasOwnProperty.call(DAY_LABELS, requestedFormat) ? requestedFormat : 'long';
+            const format = DAY_LABELS.hasOwnProperty(requestedFormat) ? requestedFormat : 'long';
             const layout = xOptions.layout || 'horizontal';
             const showSelectAll = xOptions.selectAll !== false;
 

web_interface/static/v3/plugins_manager.js

@@ -1225,10 +1225,10 @@ function initializePlugins() {
     window.pluginManager._reswap = false;
     // Await the installed-plugins fetch so window.installedPlugins is populated before
     // searchPluginStore renders Installed/Reinstall badges against it.
-    loadInstalledPlugins().then(() => {
-        searchPluginStore(!isReswapWarm);
-    }).catch(err => {
+    loadInstalledPlugins().catch(err => {
         console.error('[PluginStore] loadInstalledPlugins failed:', err);
+    }).finally(() => {
+        searchPluginStore(!isReswapWarm);
     });
 
     // Setup search functionality (with guard against duplicate listeners)
@@ -4853,9 +4853,9 @@ window.executePluginAction = function(actionId, actionIndex, pluginIdParam = nul
                     showNotification(data.message || 'Action completed successfully!', 'success');
                 }
             } else {
-                statusDiv.innerHTML = `<div class="text-red-600"><i class="fas fa-exclamation-circle mr-2"></i>${data.message}</div>`;
+                statusDiv.innerHTML = `<div class="text-red-600"><i class="fas fa-exclamation-circle mr-2"></i>${escapeHtml(data.message || 'Error')}</div>`;
                 if (data.output) {
-                    statusDiv.innerHTML += `<pre class="mt-2 text-xs bg-red-50 p-2 rounded overflow-auto max-h-32">${data.output}</pre>`;
+                    statusDiv.innerHTML += `<pre class="mt-2 text-xs bg-red-50 p-2 rounded overflow-auto max-h-32">${escapeHtml(data.output)}</pre>`;
                 }
                 btn.innerHTML = originalText;
                 btn.disabled = false;
@@ -4899,8 +4899,8 @@ window.executePluginAction = function(actionId, actionIndex, pluginIdParam = nul
                         </div>
                         <div class="mb-3">
                             <p class="text-sm text-blue-700 mb-2">1. Click the link below to authorize:</p>
-                            <a href="${data.auth_url}" target="_blank" class="text-blue-600 hover:text-blue-800 underline break-all">
-                                ${data.auth_url}
+                            <a href="${data.auth_url && data.auth_url.startsWith('http') ? escapeHtml(data.auth_url) : '#'}" target="_blank" class="text-blue-600 hover:text-blue-800 underline break-all">
+                                ${escapeHtml(data.auth_url || '')}
                             </a>
                         </div>
                         <div class="mb-2">
@@ -4922,7 +4922,7 @@ window.executePluginAction = function(actionId, actionIndex, pluginIdParam = nul
                         <div class="text-green-900 font-medium mb-2">
                             <i class="fas fa-check-circle mr-2"></i>${data.message || 'Action completed successfully'}
                         </div>
-                        ${data.output ? `<pre class="mt-2 text-xs bg-green-50 p-2 rounded overflow-auto max-h-32">${data.output}</pre>` : ''}
+                        ${data.output ? `<pre class="mt-2 text-xs bg-green-50 p-2 rounded overflow-auto max-h-32">${escapeHtml(data.output)}</pre>` : ''}
                     </div>
                 `;
                 btn.innerHTML = originalText;
@@ -4935,9 +4935,9 @@ window.executePluginAction = function(actionId, actionIndex, pluginIdParam = nul
             statusDiv.innerHTML = `
                 <div class="bg-red-50 border border-red-200 rounded p-3">
                     <div class="text-red-900 font-medium mb-2">
-                        <i class="fas fa-exclamation-circle mr-2"></i>${data.message || 'Action failed'}
+                        <i class="fas fa-exclamation-circle mr-2"></i>${escapeHtml(data.message || 'Action failed')}
                     </div>
-                    ${data.output ? `<pre class="mt-2 text-xs bg-red-50 p-2 rounded overflow-auto max-h-32">${data.output}</pre>` : ''}
+                    ${data.output ? `<pre class="mt-2 text-xs bg-red-50 p-2 rounded overflow-auto max-h-32">${escapeHtml(data.output)}</pre>` : ''}
                 </div>
             `;
             btn.innerHTML = originalText;
@@ -8067,4 +8067,3 @@ setTimeout(function() {
         }
     });
 })();
-