fix: work through the remaining review findings on the editor and bridge

allow_insecure_mqtt (CWE-319, requested): a password with TLS disabled crosses
the network in cleartext. Refused now rather than merely warned about -- but
refused, not forbidden, because unencrypted MQTT on a trusted LAN is a normal
deliberate setup. allow_insecure_mqtt is the explicit acknowledgement, defaults
false, and is coerced like the other booleans so the string "false" cannot
switch the guard off.

starlark.py:796 -- the supported service runs Flask threaded, so two start
requests could each see running=False, each launch an editor, and the second
state write replace the first PID, orphaning a process that holds the display
down with nothing recording it. The check-launch-write sequence now takes a
module-level lock.

starlark.py:848 -- if the state write failed the route returned success with an
editor running and no PID recorded: status and stop both reported no session
while the display stayed down until the timeout expired. It now terminates the
process group and returns an error.

starlark.py:890 -- SIGKILL gives the script's EXIT trap no chance to run, so
nothing hands the display back, yet the response said "the display is
restarting". After an escalation the display is now restarted explicitly, and a
failure to do so returns an error naming the manual step instead of a success.

pixlet_config_editor.sh:184 -- find_pixlet supports Darwin but macOS ships no
timeout(1); GNU coreutils installs it as gtimeout. Resolved up front so the
failure lands before the display is stopped rather than after.

pixlet_config_editor.sh:154 -- wildcard, loopback and an explicit interface
address are three cases, not two. Collapsing the last two printed a URL saying
"localhost" whenever PIXLET_EDITOR_HOST named a LAN address.

tools.html:1254 -- escHtml does not encode single quotes, and the app id was
interpolated into an inline onclick="startPixletEditor('...')", so a directory
containing an apostrophe could break out of the JS string and run script. The
handler binds with addEventListener and reads the id from dataset, where it is
only ever parsed as an HTML attribute.

Tests: test_mqtt_bridge_config_endpoint.py grows to 23 cases covering the opt-in
in both directions. The tools DOM suite gains three guards asserting the edit
buttons carry no inline onclick and pass the id via dataset -- those need jsdom
and did not run here, so CI verifies them.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

scripts/utils/pixlet_config_editor.sh

@@ -104,6 +104,29 @@ PIXLET_BIN=$(find_pixlet) || {
     exit 1
 }
 
+# find_pixlet supports Darwin, so this script has to as well. macOS ships no
+# timeout(1); GNU coreutils installs it as gtimeout. Resolve whichever exists
+# and fail here with instructions rather than at the invocation far below,
+# where the failure would land after the display has already been stopped.
+find_timeout() {
+    local candidate
+    for candidate in timeout gtimeout; do
+        if command -v "$candidate" >/dev/null 2>&1; then
+            command -v "$candidate"
+            return 0
+        fi
+    done
+    return 1
+}
+
+TIMEOUT_BIN=$(find_timeout) || {
+    echo "Neither 'timeout' nor 'gtimeout' was found on PATH."
+    echo "This script needs one to bound the editing session."
+    echo "On macOS, install GNU coreutils:"
+    echo "  brew install coreutils"
+    exit 1
+}
+
 CONFIG_FILE="$APP_DIR/config.json"
 if [ -f "$CONFIG_FILE" ]; then
     cp "$CONFIG_FILE" "$CONFIG_FILE.backup"
@@ -148,11 +171,14 @@ if [ "$DISPLAY_WAS_RUNNING" = true ]; then
     sudo systemctl stop ledmatrix
 fi
 
-if [ "$BIND_HOST" = "0.0.0.0" ]; then
-    REACH_HOST="$(hostname).local"
-else
-    REACH_HOST="localhost"
-fi
+# Wildcard, loopback and an explicit interface address are three different
+# cases. Collapsing the last two into "localhost" printed a URL pointing at the
+# user's own machine whenever PIXLET_EDITOR_HOST named a LAN address.
+case "$BIND_HOST" in
+    0.0.0.0|::|"")            REACH_HOST="$(hostname).local" ;;
+    127.0.0.1|::1|localhost)  REACH_HOST="localhost" ;;
+    *)                        REACH_HOST="$BIND_HOST" ;;
+esac
 
 echo ""
 echo "Editing:  $APP_ID"
@@ -181,7 +207,7 @@ cd "$APP_DIR"
 # --foreground: stay in this script's process group so one signal reaches the
 # whole session. Backgrounded + `wait` so the EXIT trap can run while the child
 # is still alive; a foreground child would leave bash waiting on it instead.
-timeout --foreground "$EDITOR_TIMEOUT" "$PIXLET_BIN" serve "$(basename "$STAR_FILE")" \
+"$TIMEOUT_BIN" --foreground "$EDITOR_TIMEOUT" "$PIXLET_BIN" serve "$(basename "$STAR_FILE")" \
     --host "$BIND_HOST" \
     --port "$PORT" \
     --no-browser \

test/js/dom/test_tools_sections.js

@@ -86,6 +86,17 @@ const get = p => new Promise((res, rej) =>
   ok('editor lists the apps on disk',
      appIds.every(id => $('pixlet-editor-body').textContent.includes(id)), appIds);
   ok('no session banner while idle', !$('pixlet-countdown'));
+  // escHtml does not encode single quotes, so an app id interpolated into an
+  // inline onclick="startPixletEditor('...')" could break out of the JS string
+  // and run script. The id must reach the handler through dataset instead.
+  const editBtns = [...window.document.querySelectorAll('[id^="btn-pixlet-edit-"]')];
+  ok('edit buttons exist', editBtns.length > 0, editBtns.length);
+  ok('edit buttons carry no inline onclick',
+     editBtns.every(b => !b.getAttribute('onclick')),
+     editBtns.map(b => b.getAttribute('onclick')));
+  ok('edit buttons pass the app id via dataset',
+     editBtns.every(b => appIds.includes(b.dataset.appId)),
+     editBtns.map(b => b.dataset.appId));
   ok('warns that the display stops',
      window.document.body.textContent.includes('display stops while a session is open'));
 

test/test_mqtt_bridge_config_endpoint.py

@@ -70,7 +70,10 @@ class TestABodyItCannotApplyIsRejected:
 
 class TestClearPasswordNeedsARealBoolean:
     def _seed(self, cfg, password="hunter2"):
-        cfg.write_text(json.dumps({"mqtt_password": password}), encoding="utf-8")
+        # TLS on so these cases exercise clear_password alone: a stored password
+        # with TLS off is refused by the CWE-319 guard, which is a separate test.
+        cfg.write_text(json.dumps({"mqtt_password": password, "mqtt_tls": True}),
+                       encoding="utf-8")
 
     def _stored(self, cfg):
         return json.loads(cfg.read_text(encoding="utf-8")).get("mqtt_password")
@@ -95,3 +98,41 @@ class TestClearPasswordNeedsARealBoolean:
         self._seed(cfg)
         assert c.put(URL, json={"clear_password": truthy}).status_code == 200
         assert self._stored(cfg) is None
+
+
+class TestCleartextCredentialsNeedAnExplicitOptIn:
+    """CWE-319. A password with TLS off crosses the network in the clear.
+
+    Refused rather than forbidden: unencrypted MQTT on a trusted LAN is a normal
+    deliberate setup, so allow_insecure_mqtt is the explicit acknowledgement.
+    """
+
+    def test_a_password_without_tls_is_refused(self, client):
+        c, _ = client
+        r = c.put(URL, json={"mqtt_password": "hunter2", "mqtt_tls": False})
+        assert r.status_code == 400
+        assert "allow_insecure_mqtt" in r.get_json()["message"]
+
+    def test_the_opt_in_allows_it(self, client):
+        c, cfg = client
+        r = c.put(URL, json={"mqtt_password": "hunter2", "mqtt_tls": False,
+                             "allow_insecure_mqtt": True})
+        assert r.status_code == 200, r.get_json()
+        assert json.loads(cfg.read_text(encoding="utf-8"))["mqtt_password"] == "hunter2"
+
+    def test_tls_on_needs_no_opt_in(self, client):
+        c, _ = client
+        r = c.put(URL, json={"mqtt_password": "hunter2", "mqtt_tls": True})
+        assert r.status_code == 200, r.get_json()
+
+    def test_no_password_is_unaffected(self, client):
+        c, _ = client
+        assert c.put(URL, json={"mqtt_tls": False}).status_code == 200
+
+    def test_the_opt_in_is_a_real_boolean(self, client):
+        """"false" must not switch the guard off, same as clear_password."""
+        c, _ = client
+        r = c.put(URL, json={"mqtt_password": "hunter2", "mqtt_tls": False,
+                             "allow_insecure_mqtt": "false"})
+        assert r.status_code == 400
+

web_interface/blueprints/api_v3/__init__.py

@@ -1945,6 +1945,9 @@ _MQTT_BRIDGE_DEFAULTS = {
     'mqtt_topic': 'ledmatrix/command',
     'mqtt_tls': False,
     'mqtt_tls_insecure': False,
+    # Opt-in acknowledgement that credentials may cross an untrusted
+    # network in cleartext. Off by default: the save is refused instead.
+    'allow_insecure_mqtt': False,
     'ledmatrix_api_base': 'http://localhost:5000',
     'request_timeout': 15,
     'on_demand_duration': None,
@@ -2015,7 +2018,7 @@ def _coerce_mqtt_bridge_value(key: str, raw: Any) -> Tuple[Any, Optional[str]]:
         if not 1 <= duration <= 86400:
             return None, 'On-demand duration must be between 1 and 86400 seconds'
         return duration, None
-    if key in ('mqtt_tls', 'mqtt_tls_insecure'):
+    if key in ('mqtt_tls', 'mqtt_tls_insecure', 'allow_insecure_mqtt'):
         return bool(raw) if isinstance(raw, bool) else str(raw).lower() in ('1', 'true', 'yes', 'on'), None
     if key == 'log_level':
         level = str(raw or '').upper()

web_interface/blueprints/api_v3/misc.py

@@ -472,9 +472,21 @@ def update_mqtt_bridge_config():
         else:
             config['mqtt_password'] = existing_password
 
-        if config.get('mqtt_password') and not config.get('mqtt_tls'):
-            logger.warning('MQTT bridge: a password is set without TLS; '
-                           'credentials will cross the network in cleartext')
+        # CWE-319: a password with TLS off is sent in the clear. On a trusted
+        # LAN that is a normal, deliberate setup, so this is refused rather
+        # than forbidden -- allow_insecure_mqtt is the explicit acknowledgement.
+        insecure = bool(config.get('mqtt_password')) and not config.get('mqtt_tls')
+        if insecure and not config.get('allow_insecure_mqtt'):
+            return jsonify({
+                'status': 'error',
+                'message': 'MQTT credentials would cross the network in cleartext '
+                           'with TLS disabled. Enable mqtt_tls, or set '
+                           'allow_insecure_mqtt to accept that on a trusted network.'
+            }), 400
+        if insecure:
+            logger.warning('MQTT bridge: a password is set without TLS and '
+                           'allow_insecure_mqtt is on; credentials will cross the '
+                           'network in cleartext')
 
         _MQTT_BRIDGE_DIR.mkdir(parents=True, exist_ok=True)
         # Write via a temp file in the same directory so a crash mid-write

web_interface/blueprints/api_v3/starlark.py

@@ -4,6 +4,7 @@ Routes decorate the shared `api_v3` Blueprint from ._common, so their
 endpoint names are unchanged by living here.
 """
 import signal
+import threading
 
 from web_interface.blueprints.api_v3 import (
     PROJECT_ROOT, Path, _PIXLET_EDITOR_DEFAULT_PORT,
@@ -771,6 +772,28 @@ def get_pixlet_editor_status():
         return jsonify({'status': 'error', 'message': 'Could not read editor status',
                         'details': describe_exception(e)}), 500
 
+#: Flask runs threaded in the supported service, so two start requests can each
+#: observe running=False, each launch an editor, and the second state write
+#: replace the first PID -- orphaning a process that holds the display down with
+#: nothing left recording it. The check-launch-write sequence takes this lock.
+_EDITOR_START_LOCK = threading.Lock()
+
+
+def _terminate_editor_process(pid, wait_s=5):
+    """Signal an editor's process group and wait briefly for it to exit."""
+    try:
+        os.killpg(os.getpgid(pid), signal.SIGTERM)
+    except (ProcessLookupError, PermissionError, OSError):
+        with contextlib.suppress(ProcessLookupError, PermissionError):
+            os.kill(pid, signal.SIGTERM)
+    deadline = _pkg.time.time() + wait_s
+    while _pkg.time.time() < deadline and _pixlet_editor_alive(pid):
+        _pkg.time.sleep(0.25)
+    if _pixlet_editor_alive(pid):
+        with contextlib.suppress(ProcessLookupError, PermissionError, OSError):
+            os.killpg(os.getpgid(pid), signal.SIGKILL)
+
+
 @api_v3.route('/starlark/editor/start', methods=['POST'])
 def start_pixlet_editor():
     """Start an editing session for one app."""
@@ -792,60 +815,69 @@ def start_pixlet_editor():
             return jsonify({'status': 'error',
                             'message': 'Pixlet is not installed - install it first'}), 503
 
-        current = _pixlet_editor_status()
-        if current.get('running'):
-            return jsonify({'status': 'error',
-                            'message': f"An editor session for '{current.get('app_id')}' is "
-                                       f"already running; stop it first"}), 409
+        with _EDITOR_START_LOCK:
+            current = _pixlet_editor_status()
+            if current.get('running'):
+                return jsonify({'status': 'error',
+                                'message': f"An editor session for '{current.get('app_id')}' is "
+                                           f"already running; stop it first"}), 409
 
-        try:
-            timeout_s = int(data.get('timeout') or _PIXLET_EDITOR_DEFAULT_TIMEOUT)
-        except (TypeError, ValueError):
-            return jsonify({'status': 'error', 'message': 'timeout must be a whole number'}), 400
-        if not 60 <= timeout_s <= _PIXLET_EDITOR_MAX_TIMEOUT:
-            return jsonify({'status': 'error',
-                            'message': f'timeout must be between 60 and '
-                                       f'{_PIXLET_EDITOR_MAX_TIMEOUT} seconds'}), 400
-        try:
-            port = int(data.get('port') or _PIXLET_EDITOR_DEFAULT_PORT)
-        except (TypeError, ValueError):
-            return jsonify({'status': 'error', 'message': 'port must be a whole number'}), 400
-        if not 1024 <= port <= 65535:
-            return jsonify({'status': 'error', 'message': 'port must be between 1024 and 65535'}), 400
+            try:
+                timeout_s = int(data.get('timeout') or _PIXLET_EDITOR_DEFAULT_TIMEOUT)
+            except (TypeError, ValueError):
+                return jsonify({'status': 'error', 'message': 'timeout must be a whole number'}), 400
+            if not 60 <= timeout_s <= _PIXLET_EDITOR_MAX_TIMEOUT:
+                return jsonify({'status': 'error',
+                                'message': f'timeout must be between 60 and '
+                                           f'{_PIXLET_EDITOR_MAX_TIMEOUT} seconds'}), 400
+            try:
+                port = int(data.get('port') or _PIXLET_EDITOR_DEFAULT_PORT)
+            except (TypeError, ValueError):
+                return jsonify({'status': 'error', 'message': 'port must be a whole number'}), 400
+            if not 1024 <= port <= 65535:
+                return jsonify({'status': 'error', 'message': 'port must be between 1024 and 65535'}), 400
 
-        env = dict(os.environ)
-        env['PIXLET_EDITOR_PORT'] = str(port)
-        env['PIXLET_EDITOR_TIMEOUT'] = str(timeout_s)
-        # A browser reaching this endpoint is remote by definition, so the
-        # session needs to listen on more than loopback to be usable at all --
-        # but only as a *default*. An operator who has already set
-        # PIXLET_EDITOR_HOST (e.g. to keep it loopback-only even from the web
-        # UI) must not have that overridden here.
-        env.setdefault('PIXLET_EDITOR_HOST', '0.0.0.0')
+            env = dict(os.environ)
+            env['PIXLET_EDITOR_PORT'] = str(port)
+            env['PIXLET_EDITOR_TIMEOUT'] = str(timeout_s)
+            # A browser reaching this endpoint is remote by definition, so the
+            # session needs to listen on more than loopback to be usable at all --
+            # but only as a *default*. An operator who has already set
+            # PIXLET_EDITOR_HOST (e.g. to keep it loopback-only even from the web
+            # UI) must not have that overridden here.
+            env.setdefault('PIXLET_EDITOR_HOST', '0.0.0.0')
 
-        log_path = Path(tempfile.gettempdir()) / 'ledmatrix_pixlet_editor.log'
-        log_handle = open(log_path, 'w', encoding='utf-8')  # noqa: SIM115 - owned by the child
-        try:
-            # start_new_session so the script leads its own process group: the
-            # stop route signals the group, which is what lets the EXIT trap run
-            # and hand the display back.
-            process = subprocess.Popen(  # nosec B603 - fixed script path, validated app_id
-                ['/bin/bash', str(_PIXLET_EDITOR_SCRIPT), app_dir.name],
-                cwd=str(PROJECT_ROOT), env=env,
-                stdout=log_handle, stderr=subprocess.STDOUT,
-                start_new_session=True)
-        finally:
-            log_handle.close()
+            log_path = Path(tempfile.gettempdir()) / 'ledmatrix_pixlet_editor.log'
+            log_handle = open(log_path, 'w', encoding='utf-8')  # noqa: SIM115 - owned by the child
+            try:
+                # start_new_session so the script leads its own process group: the
+                # stop route signals the group, which is what lets the EXIT trap run
+                # and hand the display back.
+                process = subprocess.Popen(  # nosec B603 - fixed script path, validated app_id
+                    ['/bin/bash', str(_PIXLET_EDITOR_SCRIPT), app_dir.name],
+                    cwd=str(PROJECT_ROOT), env=env,
+                    stdout=log_handle, stderr=subprocess.STDOUT,
+                    start_new_session=True)
+            finally:
+                log_handle.close()
 
-        now = _pkg.time.time()
-        state = {'pid': process.pid, 'app_id': app_dir.name, 'port': port,
-                 'timeout': timeout_s, 'started_at': now, 'deadline': now + timeout_s,
-                 'host': env['PIXLET_EDITOR_HOST'], 'log': str(log_path)}
-        try:
-            with open(_PIXLET_EDITOR_STATE, 'w', encoding='utf-8') as handle:
-                json.dump(state, handle)
-        except OSError as err:
-            logger.warning('Started an editor session but could not record it: %s', err)
+            now = _pkg.time.time()
+            state = {'pid': process.pid, 'app_id': app_dir.name, 'port': port,
+                     'timeout': timeout_s, 'started_at': now, 'deadline': now + timeout_s,
+                     'host': env['PIXLET_EDITOR_HOST'], 'log': str(log_path)}
+            try:
+                with open(_PIXLET_EDITOR_STATE, 'w', encoding='utf-8') as handle:
+                    json.dump(state, handle)
+            except OSError as err:
+                # The process is up but nothing records its PID: status and stop
+                # would both report no session while the display stays down until
+                # the timeout expires. Take the editor with us instead.
+                logger.error('Started an editor session but could not record it: %s', err)
+                _terminate_editor_process(process.pid)
+                return jsonify({'status': 'error',
+                                'message': 'Could not record the editor session; '
+                                           'the editor was stopped.',
+                                'details': describe_exception(err)}), 500
 
         logger.info('Pixlet editor started for %s on port %s (pid %s, %ss limit)',
                     app_dir.name, port, process.pid, timeout_s)
@@ -883,13 +915,36 @@ def stop_pixlet_editor():
         while _pkg.time.time() < deadline and _pixlet_editor_alive(pid):
             _pkg.time.sleep(0.25)
 
+        escalated = False
         if _pixlet_editor_alive(pid):
-            logger.warning('Editor session %s ignored SIGTERM; sending SIGKILL. The display '
-                           'may need a manual restart.', pid)
+            logger.warning('Editor session %s ignored SIGTERM; sending SIGKILL.', pid)
             with contextlib.suppress(ProcessLookupError, PermissionError, OSError):
                 os.killpg(os.getpgid(pid), signal.SIGKILL)
+            escalated = True
 
         _clear_pixlet_editor_state()
+
+        if escalated:
+            # SIGKILL gives the script's EXIT trap no chance to run, so nothing
+            # has handed the display back. Reporting "the display is restarting"
+            # here was simply untrue: restart it, and if that fails say so
+            # rather than leave the panel dark behind a success response.
+            result = _run_systemctl_command(
+                ['sudo', 'systemctl', 'start', 'ledmatrix.service'])
+            if result.get('returncode') != 0:
+                logger.error('Display restart after SIGKILL failed: %s',
+                             (result.get('stderr') or '').strip())
+                return jsonify({
+                    'status': 'error',
+                    'message': 'Editor force-stopped, but the display could not be '
+                               'restarted automatically - start it manually.',
+                    'details': (result.get('stderr') or '').strip(),
+                    'data': {'running': False}}), 500
+            return jsonify({'status': 'success',
+                            'message': 'Editor force-stopped; the display has been '
+                                       'restarted.',
+                            'data': {'running': False}})
+
         return jsonify({'status': 'success',
                         'message': 'Editor stopped; the display is restarting.',
                         'data': {'running': False}})

web_interface/templates/v3/partials/tools.html

@@ -1251,7 +1251,7 @@
                 </div>
                 <button id="btn-pixlet-edit-${escHtml(a.id)}"
                         ${(!a.editable || status.running) ? 'disabled' : ''}
-                        onclick="startPixletEditor('${escHtml(a.id)}')"
+                        data-pixlet-edit data-app-id="${escHtml(a.id)}"
                         class="shrink-0 inline-flex items-center px-3 py-2 border border-gray-300 text-sm font-medium rounded-md text-gray-700 bg-white hover:bg-gray-50 disabled:opacity-50 disabled:cursor-not-allowed">
                     <i class="fas fa-sliders-h mr-2"></i>Edit
                 </button>
@@ -1263,6 +1263,15 @@
 
         body.innerHTML = active + `<div class="mt-2">${rows}</div>
             <div id="result-pixlet" class="hidden"></div>`;
+
+        // Bound here rather than via an inline onclick: escHtml does not encode
+        // single quotes, and the id used to be interpolated into a single-quoted
+        // JS string, so an app directory containing an apostrophe could break
+        // out of it and run script. Through dataset the value is only ever
+        // parsed as an HTML attribute, never as JavaScript.
+        body.querySelectorAll('[data-pixlet-edit]').forEach(btn => {
+            btn.addEventListener('click', () => startPixletEditor(btn.dataset.appId));
+        });
     }
 
     window.loadPixletEditor = function() {