From 1d51efe4c7414a34056a978d758e0d54e66e1644 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Thu, 17 Sep 2026 09:26:21 -0400 Subject: [PATCH 1/3] fix(backup): restore over existing files on hosts without os.chown (#592) _copy_file() replaces each restored file and then carries the previous owner across with os.chown. On Windows os.chown does not exist and st_uid/st_gid are 0 rather than absent, so the ownership branch always ran and raised AttributeError. That is not an OSError, so it escaped every per-section handler in restore_backup(): a restore over any existing config aborted at config.json and restored nothing. Skip the ownership step where os.chown is missing, as auto_update_setup.py already does. No change on POSIX. test_restore_over_a_file_the_user_cannot_write simulates root-owned files with chmod 0o444; on Windows that sets the read-only attribute, which blocks any rename over the file, so it is skipped there. The modes the app writes (0o644/0o640/0o600) replace fine on Windows. Co-authored-by: Claude Opus 5 --- CHANGELOG.md | 6 ++++ src/backup_manager.py | 5 +++- test/test_backup_manager.py | 58 +++++++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea497352..51752b5f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -134,6 +134,12 @@ Core: from every config load and could not `import web_interface.app` at all. `ensure_shared_group_ownership()` now returns immediately when `os.geteuid` or `os.chown` is missing. No behaviour change on the Pi. +- Restoring a backup on Windows no longer fails over files that already exist. + The restore carries each replaced file's owner across with `os.chown`, which + does not exist on Windows; the `AttributeError` escaped the per-file error + handling, so the restore stopped at `config.json` with nothing restored. The + ownership step is now skipped where `os.chown` is missing. No behaviour + change on the Pi. ## 3.4.0 diff --git a/src/backup_manager.py b/src/backup_manager.py index e8d03b18..13114114 100644 --- a/src/backup_manager.py +++ b/src/backup_manager.py @@ -530,12 +530,15 @@ def _copy_file(src: Path, dst: Path) -> None: os.chmod(tmp_path, existing_mode) else: shutil.copymode(src, tmp_path) - if existing_owner is not None: + if existing_owner is not None and hasattr(os, 'chown'): # Replacing a file creates a new inode owned by whoever is running, # which would silently move a root-owned config to the web user. # Carry the previous owner across when the OS permits it — only # root can hand a file to another user, so this is best-effort and # a plain restore as the web user simply keeps its own ownership. + # os.chown does not exist on Windows (where st_uid/st_gid are just + # 0); looking it up there raises AttributeError, which no caller + # catches, so every restore over an existing file aborted. try: os.chown(tmp_path, existing_owner[0], existing_owner[1]) except (OSError, PermissionError): diff --git a/test/test_backup_manager.py b/test/test_backup_manager.py index e6533d69..fb603d43 100644 --- a/test/test_backup_manager.py +++ b/test/test_backup_manager.py @@ -3,9 +3,12 @@ from __future__ import annotations import json +import os import stat +import sys import zipfile from pathlib import Path +from unittest.mock import MagicMock import pytest @@ -300,6 +303,11 @@ def test_restore_rejects_malicious_zip(empty_project: Path, tmp_path: Path) -> N assert any("unsafe" in e.lower() for e in result.errors) +@pytest.mark.skipif( + sys.platform == "win32", + reason="simulates root-owned POSIX files with chmod 0o444, which on Windows sets the " + "read-only attribute, and Windows refuses to rename over a read-only file", +) def test_restore_over_a_file_the_user_cannot_write( project: Path, empty_project: Path, tmp_path: Path ) -> None: @@ -334,3 +342,53 @@ def test_restore_over_a_file_the_user_cannot_write( # The destination's mode is preserved rather than widened to the umask. assert stat.S_IMODE((empty_project / "config" / "config_secrets.json").stat().st_mode) == 0o444 + + +def _existing_config(empty_project: Path) -> None: + (empty_project / "config").mkdir(parents=True, exist_ok=True) + for name in ("config.json", "config_secrets.json", "wifi_config.json", "ytm_auth.json"): + (empty_project / "config" / name).write_text("{}", encoding="utf-8") + + +def test_restore_over_existing_files_without_os_chown( + project: Path, empty_project: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Restore must work where the OS has no file ownership API (Windows). + + Replacing a file tries to carry its previous owner across with os.chown. + That name does not exist on Windows, and the AttributeError is not an + OSError, so it escaped every per-section handler: restoring over any + existing config aborted the whole restore and left the old files in place. + """ + zip_path = create_backup(project, output_dir=tmp_path / "exports") + _existing_config(empty_project) + monkeypatch.delattr(os, "chown", raising=False) + + result = restore_backup(zip_path, empty_project, RestoreOptions()) + + assert result.success, result.errors + for section in ("config", "secrets", "wifi", "ytm_auth"): + assert section in result.restored, f"{section} not restored: {result.errors}" + restored = json.loads((empty_project / "config" / "config.json").read_text()) + assert restored["my-plugin"]["favorites"] == ["A", "B"] + + +def test_restore_still_carries_the_previous_owner_across( + project: Path, empty_project: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Where os.chown exists, the replaced file keeps the old file's owner.""" + zip_path = create_backup(project, output_dir=tmp_path / "exports") + _existing_config(empty_project) + target = empty_project / "config" / "config.json" + old = target.stat() + chown = MagicMock() + monkeypatch.setattr(os, "chown", chown, raising=False) + + result = restore_backup(zip_path, empty_project, RestoreOptions( + restore_secrets=False, restore_wifi=False, + restore_fonts=False, restore_plugin_uploads=False, reinstall_plugins=False, + )) + + assert result.success, result.errors + owners = {(c.args[1], c.args[2]) for c in chown.call_args_list} + assert owners == {(old.st_uid, old.st_gid)} From f4750388954140b4f786277f96165cec663df96e Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Thu, 17 Sep 2026 12:38:39 -0400 Subject: [PATCH 2/3] fix(cache): web UI can read what the display service caches again (#593) * fix(cache): web UI can read what the display service caches again ledmatrix-web.service carried CacheDirectory=ledmatrix. With User= set to the installing user, systemd re-owns /var/cache/ledmatrix and everything in it to that user and its primary group whenever the directory's owner differs -- for a directory root created, on the first start. That erased the root:ledmatrix setgid layout the installers set up, so every file the display service (root) wrote afterwards was root:root 0660 and unreadable by the web interface: WARNING - Permission denied loading cache for display_current_state ... Since #547 install_service.sh renders the web unit from the template, so every fresh install hit this. Measured on one rig: 392 unreadable files, and the web UI's display status, on-demand state and plugin health empty. Existing installs only receive `git pull`, never a reinstalled unit, so the fix for them is in the code the root display service runs: - DiskCache.set gives each file the directory's group (when the directory is group-writable) and 0660 on the open descriptor before the rename, independent of setgid. This also closes a window where a fresh file was visible as mkstemp's 0600. - DiskCache.share_existing_files repairs files an older version left behind, once per process from the cleanup thread. It works through O_NOFOLLOW descriptors and skips hard links and other users' files: the directory is writable by the web user, and root must not be steered into changing a file outside it. For new installs, the web unit drops CacheDirectory=/CacheDirectoryMode=, and install_web_service.sh stops replacing an existing directory's ledmatrix group with the user's group. Co-Authored-By: Claude Opus 5 * fix(web): on-demand and current-display status read the display's latest state Found testing the cache-permission fix on a rig: once the web interface could read display_on_demand_state at all, /display/on-demand/status kept answering "active" for over 100 seconds while the file on disk said "idle". Both status routes read the display service's keys through the web process's memory tier, which serves the first copy it read for the full max_age (120s). Read them with memory_ttl=0, as every other cross-process reader (plugin health/metrics, the on-demand mailbox) already does. Co-Authored-By: Claude Opus 5 * fix(install): re-group the cache dir whenever the web user is outside its group install_web_service.sh replaced an existing cache directory's group only when it was root's. A directory in any other group the web user is not a member of -- root:ledmatrix, for a user who is not in ledmatrix -- was left alone, and every file root wrote there stayed unreadable to the web interface. Replace the group whenever the installing user is not in it. A directory whose group the user is already in (ledmatrix, or the user's own group where CacheDirectory= left it) is still left as it is: re-grouping a working directory strands the files already in it on the old group. When the group does change and root-owned JSON files carrying the old group are present, try-restart ledmatrix.service so DiskCache.share_existing_files re-groups them through its symlink- and hard-link-safe path, rather than a recursive chgrp. Verified under WSL's systemd for seven directory states (user group, ledmatrix member, ledmatrix non-member with and without root files, root:root, missing, unnamed gid); the previous version left the non-member case unchanged. Addresses CodeRabbit review on #593. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- scripts/install/install_service.sh | 2 +- scripts/install/install_web_service.sh | 50 ++-- src/cache/disk_cache.py | 147 ++++++++++-- src/cache_manager.py | 8 + systemd/ledmatrix-web.service | 10 +- test/test_cache_shared_group.py | 267 +++++++++++++++++++++ test/test_web_display_state_freshness.py | 62 +++++ web_interface/blueprints/api_v3/display.py | 9 +- 8 files changed, 517 insertions(+), 38 deletions(-) create mode 100644 test/test_cache_shared_group.py create mode 100644 test/test_web_display_state_freshness.py diff --git a/scripts/install/install_service.sh b/scripts/install/install_service.sh index 07a53832..0d88891e 100755 --- a/scripts/install/install_service.sh +++ b/scripts/install/install_service.sh @@ -70,7 +70,7 @@ echo "Installing LEDMatrix Web Interface service (ledmatrix-web.service)..." # Rendered from systemd/ledmatrix-web.service, the same template # install_web_service.sh uses. This was an inline heredoc until it drifted from # the template: it had lost Wants=network-online.target, RestartSec, -# SyslogIdentifier, CacheDirectory and Environment=USE_THREADING. Because +# SyslogIdentifier and Environment=USE_THREADING. Because # src/startup_validator.py compares the installed unit against the template, # every boot warned "re-run install_service.sh" -- and doing so reinstalled the # same stale copy, so the warning could never clear. diff --git a/scripts/install/install_web_service.sh b/scripts/install/install_web_service.sh index 40d5378c..0c4c9908 100755 --- a/scripts/install/install_web_service.sh +++ b/scripts/install/install_web_service.sh @@ -32,7 +32,7 @@ fi # Render the unit from systemd/ledmatrix-web.service. That template is the # only description of the unit; this script used to carry its own heredoc copy, # and install_service.sh a third, which is how the installed unit on real rigs -# ended up missing RestartSec, SyslogIdentifier and CacheDirectory while +# ended up missing RestartSec and SyslogIdentifier while # src/startup_validator.py warned about drift on every boot. TEMPLATE="$PROJECT_ROOT_DIR/systemd/ledmatrix-web.service" if [ ! -f "$TEMPLATE" ]; then @@ -62,31 +62,47 @@ for VERIFY_UNIT in ledmatrix-update-verify.service ledmatrix-update-verify.path; fi done -# Ensure cache directory exists with proper permissions -# This is a fallback for older systemd versions that don't support CacheDirectory -# Systemd 239+ will automatically create it via CacheDirectory directive +# Shared cache directory. The display service (root) and this web service both +# write here and read each other's files, which are created 0660, so the two +# share it through the directory's group: ledmatrix when the installing user +# is in it (first_time_install.sh / setup_cache.sh set that up), otherwise the +# user's own group. setgid makes new files inherit that group. +# +# An existing directory keeps its group whenever the web user can read through +# it -- ledmatrix, or the user's own group where systemd's old CacheDirectory= +# left it -- because re-grouping a working directory strands every file already +# in it on the old group. Only a group the user is not in (root's, or ledmatrix +# for a user outside it) is replaced. This used to force the user's group on +# every run, replacing the ledmatrix group setup_cache.sh had set. echo "Setting up cache directory..." CACHE_DIR="/var/cache/ledmatrix" +USER_GROUPS=$(id -nG "$ACTUAL_USER" 2>/dev/null | tr ' ' '\n') +if printf '%s\n' "$USER_GROUPS" | grep -qx ledmatrix; then + CACHE_GROUP="ledmatrix" +else + CACHE_GROUP=$(id -gn "$ACTUAL_USER" 2>/dev/null || echo root) +fi if [ ! -d "$CACHE_DIR" ]; then mkdir -p "$CACHE_DIR" - # Set group ownership to allow both root and web user access - # Try to use ACTUAL_USER's group, fallback to root if that fails - if getent group "$ACTUAL_USER" > /dev/null 2>&1; then - chown root:"$ACTUAL_USER" "$CACHE_DIR" 2>/dev/null || chown root:root "$CACHE_DIR" - else - chown root:root "$CACHE_DIR" - fi - chmod 775 "$CACHE_DIR" + chown root:"$CACHE_GROUP" "$CACHE_DIR" 2>/dev/null || true echo "✓ Cache directory created: $CACHE_DIR" else - # Ensure permissions are correct - chmod 775 "$CACHE_DIR" 2>/dev/null || true - # Try to set group ownership if possible - if getent group "$ACTUAL_USER" > /dev/null 2>&1; then - chown root:"$ACTUAL_USER" "$CACHE_DIR" 2>/dev/null || true + DIR_GROUP=$(stat -c %G "$CACHE_DIR" 2>/dev/null) + if ! printf '%s\n' "$USER_GROUPS" | grep -qx "$DIR_GROUP"; then + if chgrp "$CACHE_GROUP" "$CACHE_DIR" 2>/dev/null; then + echo "✓ Cache directory group changed from $DIR_GROUP to $CACHE_GROUP" + # Files already there keep the old group. The display service + # re-groups its own files when it starts (DiskCache.share_existing_files, + # which refuses symlinks and hard links); a recursive chgrp here + # would not. try-restart does nothing if the service is not running. + if find "$CACHE_DIR" -maxdepth 1 -name '*.json' -user root ! -group "$CACHE_GROUP" -print -quit 2>/dev/null | grep -q .; then + systemctl try-restart ledmatrix.service 2>/dev/null || true + fi + fi fi echo "✓ Cache directory exists: $CACHE_DIR" fi +chmod 2775 "$CACHE_DIR" 2>/dev/null || true # Reload systemd to recognize the new service echo "Reloading systemd..." diff --git a/src/cache/disk_cache.py b/src/cache/disk_cache.py index c64c8632..87c3d7a9 100644 --- a/src/cache/disk_cache.py +++ b/src/cache/disk_cache.py @@ -7,6 +7,7 @@ Handles persistent disk-based caching with atomic writes and error recovery. import json import math import os +import stat import time import tempfile import logging @@ -139,6 +140,65 @@ else: return json.loads(raw) +# SHARING CACHE FILES BETWEEN THE TWO SERVICES +# -------------------------------------------- +# The display service runs as root and the web interface as the installing +# user, and the web interface reads records only the display writes +# (display_current_state, display_on_demand_state, plugin_metrics:*). Files are +# written 0660, so the web interface can read one only through its group. +# +# The installers rely on the directory's setgid bit to set that group. That is +# not something the cache can count on: systemd's CacheDirectory=, which +# ledmatrix-web.service carried until Sept 2026, re-owns the directory and +# everything in it to the web user and its primary group whenever the +# directory's owner does not match, and the setgid layout never survives that. +# From then on every file root creates is root:root 0660, unreadable by the web +# interface. Measured on one rig: 365 such files, and the web UI's display +# status, on-demand state and plugin health all silently empty. +# +# So a cache file takes its group from the directory explicitly, whether or +# not setgid is set. Only a group-writable directory counts as shared: that +# group can already replace any file in it, so reading them grants nothing new. +# +# Everything here works on an open descriptor, never a path. The directory is +# writable by the web user, so between a path check and a path operation that +# user could put a symlink in the file's place, and root would then chown and +# chmod whatever it points at. + +_CACHE_FILE_MODE = 0o660 + + +def _shared_group(directory: str) -> Optional[int]: + """The group a cache file in ``directory`` should carry, if it is shared.""" + try: + st = os.stat(directory) + except OSError: + return None + if not st.st_mode & stat.S_IWGRP: + return None + return st.st_gid + + +def _share_open_file(fd: int, group: Optional[int]) -> None: + """Make an open cache file readable by the other service. Best effort.""" + fchmod = getattr(os, 'fchmod', None) # absent on Windows before 3.13 + if fchmod is not None: + try: + fchmod(fd, _CACHE_FILE_MODE) + except OSError: + pass + fchown = getattr(os, 'fchown', None) # absent on Windows + if fchown is None or group is None: + return + try: + if os.fstat(fd).st_gid != group: + fchown(fd, -1, group) + except OSError: + # Not a member of the directory's group and not root: nothing to do, + # and the file keeps the group it was created with. + pass + + class DiskCache: """Manages persistent disk-based cache.""" @@ -350,13 +410,12 @@ class DiskCache: try: with os.fdopen(fd, 'wb') as tmp_file: tmp_file.write(payload) + # Before the rename, not after: mkstemp + # creates the file 0600, and a reader that + # opened it in between was refused. + _share_open_file(tmp_file.fileno(), _shared_group(tmp_dir)) os.replace(tmp_path, cache_path) self._write_digests[key] = digest - # Set proper permissions: 660 (rw-rw----) for group-readable cache files - try: - os.chmod(cache_path, 0o660) # nosec B103 - intentional; web UI and service share a group - except OSError: - pass # Non-critical if chmod fails finally: if os.path.exists(tmp_path): try: @@ -368,12 +427,8 @@ class DiskCache: try: with open(cache_path, 'wb') as cache_file: cache_file.write(payload) + _share_open_file(cache_file.fileno(), _shared_group(tmp_dir)) self._write_digests[key] = digest - # Set proper permissions: 660 (rw-rw----) for group-readable cache files - try: - os.chmod(cache_path, 0o660) # nosec B103 - intentional; web UI and service share a group - except OSError: - pass # Non-critical if chmod fails self.logger.debug("Wrote cache for %s directly (non-atomic)", key) except (IOError, OSError, PermissionError) as write_error: # If direct write also fails, try fallback location @@ -398,11 +453,7 @@ class DiskCache: fallback_path = os.path.join(fallback_dir, os.path.basename(cache_path)) with open(fallback_path, 'wb') as tmp_file: tmp_file.write(payload) - # Set proper permissions: 660 (rw-rw----) for group-readable cache files - try: - os.chmod(fallback_path, 0o660) # nosec B103 - intentional; web UI and service share a group - except OSError: - pass # Non-critical if chmod fails + _share_open_file(tmp_file.fileno(), _shared_group(fallback_dir)) self.logger.debug("Cache wrote to fallback location: %s", fallback_path) return # Successfully wrote to fallback, exit gracefully except (IOError, OSError, PermissionError) as e2: @@ -459,6 +510,72 @@ class DiskCache: def get_cache_dir(self) -> Optional[str]: """Get the cache directory path.""" return self.cache_dir + + def share_existing_files(self) -> int: + """Give cache files already on disk the group set() now gives new ones. + + set() fixes every file it writes from now on; this repairs the ones an + older version left behind as root:root, which the web interface cannot + read until each key happens to be rewritten -- and some, like a + plugin's metrics, may not be for a long time. Meant to run once per + process, off the startup path. + + Only this process's own regular files are touched, and each one through + a descriptor opened with O_NOFOLLOW and checked for a single link: the + directory is writable by the web user, and a root process must not be + steered into changing a file outside it. + + Returns: + Number of files whose group or mode was changed. + """ + fchown = getattr(os, 'fchown', None) + geteuid = getattr(os, 'geteuid', None) + nofollow = getattr(os, 'O_NOFOLLOW', None) + if not self.cache_dir or fchown is None or geteuid is None or nofollow is None: + return 0 + group = _shared_group(self.cache_dir) + if group is None: + return 0 + euid = geteuid() + + changed = 0 + try: + entries = list(os.scandir(self.cache_dir)) + except OSError as e: + self.logger.debug("Could not scan %s to share cache files: %s", self.cache_dir, e) + return 0 + for entry in entries: + if not entry.name.endswith('.json'): + continue + try: + st = entry.stat(follow_symlinks=False) + except OSError: + continue + if (not stat.S_ISREG(st.st_mode) or st.st_uid != euid + or (st.st_gid == group and stat.S_IMODE(st.st_mode) == _CACHE_FILE_MODE)): + continue + try: + fd = os.open(entry.path, os.O_RDONLY | nofollow | getattr(os, 'O_NONBLOCK', 0)) + except OSError: + continue + try: + st = os.fstat(fd) + if not stat.S_ISREG(st.st_mode) or st.st_uid != euid or st.st_nlink != 1: + continue + _share_open_file(fd, group) + st = os.fstat(fd) + if st.st_gid == group and stat.S_IMODE(st.st_mode) == _CACHE_FILE_MODE: + changed += 1 + except OSError: + continue + finally: + os.close(fd) + if changed: + self.logger.info( + "Made %d cache file(s) in %s readable by the directory's group " + "(gid %d) so the web interface can read them", + changed, self.cache_dir, group) + return changed @staticmethod def _is_orphaned_temp(filename: str) -> bool: diff --git a/src/cache_manager.py b/src/cache_manager.py index 67624de1..dc7cebdc 100644 --- a/src/cache_manager.py +++ b/src/cache_manager.py @@ -762,6 +762,14 @@ class CacheManager: self.logger.info("Disk cache cleanup thread started (interval: %d hours)", self._disk_cleanup_interval_hours) + # Repair files an older version wrote unreadable by the web + # interface (see disk_cache.py, "SHARING CACHE FILES"). Once per + # directory per process, which is what this thread already is. + try: + self._disk_cache_component.share_existing_files() + except Exception as e: + self.logger.error("Error sharing existing cache files: %s", e, exc_info=True) + # Run initial cleanup on startup (deferred from __init__ to avoid blocking) try: self.logger.debug("Running initial disk cache cleanup") diff --git a/systemd/ledmatrix-web.service b/systemd/ledmatrix-web.service index 3b74b569..98178e71 100644 --- a/systemd/ledmatrix-web.service +++ b/systemd/ledmatrix-web.service @@ -25,9 +25,13 @@ RestartSec=10 StandardOutput=syslog StandardError=syslog SyslogIdentifier=ledmatrix-web -# Automatically create and manage cache directory -CacheDirectory=ledmatrix -CacheDirectoryMode=0775 +# No CacheDirectory=. /var/cache/ledmatrix is shared with ledmatrix.service, +# which runs as root, and the installers set it up as root:ledmatrix 2775 so +# both services can read what the other writes. CacheDirectory= makes systemd +# re-own that directory and everything in it to this unit's User= and that +# user's primary group whenever the owner differs -- which, for a directory +# root created, is on the first start. That erased the shared group, and from +# then on the web interface could not read a single file the display wrote. [Install] WantedBy=multi-user.target diff --git a/test/test_cache_shared_group.py b/test/test_cache_shared_group.py new file mode 100644 index 00000000..4d0d4a8c --- /dev/null +++ b/test/test_cache_shared_group.py @@ -0,0 +1,267 @@ +"""Tests that the web interface can read what the display service caches. + +The display service runs as root and the web interface as the installing user; +cache files are 0660, so the web interface reads them only through their group. +The installers made that the directory's group via setgid, but the web unit's +CacheDirectory= let systemd re-own /var/cache/ledmatrix to the web user and its +primary group on first start, erasing it. Every file root wrote afterwards was +root:root 0660: + + WARNING - Permission denied loading cache for display_current_state from + /var/cache/ledmatrix/display_current_state.json: [Errno 13] Permission denied + +Measured on one rig: 365 unreadable files, and the web UI's display status, +on-demand state and plugin health all silently empty. + +Most of these need POSIX ownership calls, so they skip on Windows. The ones +that only make sense as root (another user reading, another user's files) skip +without it; run them with `sudo python3 -m pytest test/test_cache_shared_group.py`. +""" + +import os +import shutil +import stat +import tempfile +import time +from pathlib import Path + +import pytest + +from src.cache import disk_cache as disk_cache_module +from src.cache.disk_cache import DiskCache + +posix_only = pytest.mark.skipif( + not hasattr(os, 'fchown') or not hasattr(os, 'geteuid'), + reason="needs POSIX file ownership") + +IS_ROOT = hasattr(os, 'geteuid') and os.geteuid() == 0 +root_only = pytest.mark.skipif(not IS_ROOT, reason="needs root") + +# Ids nothing on a test host is likely to use, for root-only scenarios. +OTHER_GID = 48213 +OTHER_UID = 48214 + + +def _another_group(): + """A group this process may chown its files to, other than its own.""" + if IS_ROOT: + return OTHER_GID + for gid in os.getgroups(): + if gid != os.getegid(): + return gid + pytest.skip("process belongs to no supplementary group") + + +def _shared_dir(path, gid, mode=0o775): + """The rig's broken layout: group-writable, no setgid, a foreign group.""" + os.chown(path, -1, gid) + os.chmod(path, mode) + assert not os.stat(path).st_mode & stat.S_ISGID + return path + + +@posix_only +class TestNewFilesTakeTheDirectoryGroup: + def test_without_setgid(self, tmp_path): + gid = _another_group() + cache = DiskCache(str(_shared_dir(tmp_path, gid))) + + cache.set('display_current_state', {'mode': 'clock'}) + + st = os.stat(tmp_path / 'display_current_state.json') + assert st.st_gid == gid + assert stat.S_IMODE(st.st_mode) == 0o660 + + def test_the_file_is_never_visible_with_a_narrower_mode(self, tmp_path, monkeypatch): + """mkstemp creates 0600; the rename used to publish it before the chmod.""" + gid = _another_group() + cache = DiskCache(str(_shared_dir(tmp_path, gid))) + seen = [] + real_replace = os.replace + + def spy(src, dst): + st = os.stat(src) + seen.append((stat.S_IMODE(st.st_mode), st.st_gid)) + return real_replace(src, dst) + + monkeypatch.setattr(disk_cache_module.os, 'replace', spy) + cache.set('k', {'v': 1}) + + assert seen == [(0o660, gid)] + + def test_a_rewrite_fixes_a_file_left_with_the_wrong_group(self, tmp_path): + gid = _another_group() + cache = DiskCache(str(_shared_dir(tmp_path, gid))) + path = tmp_path / 'k.json' + path.write_text('{}') + os.chown(path, -1, os.getegid()) + + cache.set('k', {'v': 2}) + + assert os.stat(path).st_gid == gid + + def test_the_direct_write_fallback_shares_too(self, tmp_path, monkeypatch): + gid = _another_group() + cache = DiskCache(str(_shared_dir(tmp_path, gid))) + + def no_temp(*args, **kwargs): + raise OSError("no temp files") + + monkeypatch.setattr(disk_cache_module.tempfile, 'mkstemp', no_temp) + cache.set('k', {'v': 3}) + + st = os.stat(tmp_path / 'k.json') + assert (stat.S_IMODE(st.st_mode), st.st_gid) == (0o660, gid) + + def test_a_directory_not_shared_with_its_group_is_left_alone(self, tmp_path): + gid = _another_group() + cache = DiskCache(str(_shared_dir(tmp_path, gid, mode=0o755))) + + cache.set('k', {'v': 4}) + + assert os.stat(tmp_path / 'k.json').st_gid == os.getegid() + + +@posix_only +class TestExistingFilesAreRepaired: + def test_a_root_style_file_becomes_readable(self, tmp_path): + gid = _another_group() + cache = DiskCache(str(_shared_dir(tmp_path, gid))) + path = tmp_path / 'display_on_demand_state.json' + path.write_text('{}') + os.chown(path, -1, os.getegid()) + os.chmod(path, 0o600) + + assert cache.share_existing_files() == 1 + + st = os.stat(path) + assert (stat.S_IMODE(st.st_mode), st.st_gid) == (0o660, gid) + + def test_files_already_shared_are_not_counted(self, tmp_path): + gid = _another_group() + cache = DiskCache(str(_shared_dir(tmp_path, gid))) + cache.set('k', {'v': 1}) + + assert cache.share_existing_files() == 0 + + def test_only_json_files(self, tmp_path): + gid = _another_group() + cache = DiskCache(str(_shared_dir(tmp_path, gid))) + other = tmp_path / 'tile.png' + other.write_bytes(b'x') + os.chown(other, -1, os.getegid()) + + cache.share_existing_files() + + assert os.stat(other).st_gid == os.getegid() + + def test_a_symlink_is_not_followed(self, tmp_path): + """The directory is writable by the web user; root must not be aimed elsewhere.""" + gid = _another_group() + cache_dir = tmp_path / 'cache' + cache_dir.mkdir() + _shared_dir(cache_dir, gid) + outside = tmp_path / 'outside' + outside.write_text('secret') + os.chown(outside, -1, os.getegid()) + os.chmod(outside, 0o600) + (cache_dir / 'evil.json').symlink_to(outside) + + assert DiskCache(str(cache_dir)).share_existing_files() == 0 + + st = os.stat(outside) + assert (stat.S_IMODE(st.st_mode), st.st_gid) == (0o600, os.getegid()) + + def test_a_hard_linked_file_is_skipped(self, tmp_path): + gid = _another_group() + cache_dir = tmp_path / 'cache' + cache_dir.mkdir() + _shared_dir(cache_dir, gid) + outside = tmp_path / 'outside' + outside.write_text('secret') + os.chown(outside, -1, os.getegid()) + os.chmod(outside, 0o600) + os.link(outside, cache_dir / 'linked.json') + + assert DiskCache(str(cache_dir)).share_existing_files() == 0 + assert stat.S_IMODE(os.stat(outside).st_mode) == 0o600 + + @root_only + def test_another_users_file_is_skipped(self, tmp_path): + cache = DiskCache(str(_shared_dir(tmp_path, OTHER_GID))) + path = tmp_path / 'theirs.json' + path.write_text('{}') + os.chown(path, OTHER_UID, 0) + os.chmod(path, 0o600) + + assert cache.share_existing_files() == 0 + assert os.stat(path).st_gid == 0 + + +@posix_only +@root_only +def test_a_non_root_member_of_the_group_can_read_what_root_wrote(): + """The rig, end to end: root writes, the web user reads.""" + # Not tmp_path: pytest's root-owned base directory is 0700, which the + # reader could not traverse no matter what the cache file's group was. + base = tempfile.mkdtemp() + try: + _root_writes_web_user_reads(Path(base)) + finally: + shutil.rmtree(base, ignore_errors=True) + + +def _root_writes_web_user_reads(base): + os.chmod(base, 0o755) + cache_dir = base / 'cache' + cache_dir.mkdir() + # What systemd's CacheDirectory= left behind: the web user's own group. + os.chown(cache_dir, OTHER_UID, OTHER_GID) + os.chmod(cache_dir, 0o775) + DiskCache(str(cache_dir)).set('display_current_state', {'mode': 'clock'}) + + pid = os.fork() + if pid == 0: # pragma: no cover - child + code = 1 + try: + os.setgroups([]) + os.setgid(OTHER_GID) + os.setuid(OTHER_UID) + with open(cache_dir / 'display_current_state.json', 'rb') as f: + code = 0 if b'clock' in f.read() else 2 + except PermissionError: + code = 13 + finally: + os._exit(code) + _, status = os.waitpid(pid, 0) + assert os.WEXITSTATUS(status) == 0, "web user could not read the display's cache file" + + +def test_the_web_unit_does_not_let_systemd_re_own_the_cache(): + template = Path(__file__).resolve().parent.parent / 'systemd' / 'ledmatrix-web.service' + directives = [line.strip() for line in template.read_text(encoding='utf-8').splitlines() + if line.strip() and not line.strip().startswith('#')] + offending = [d for d in directives if d.startswith(('CacheDirectory', 'StateDirectory'))] + assert not offending, ( + f"{offending}: systemd re-owns that directory to the web user, and the " + "display service (root) shares it") + + +def test_the_cleanup_thread_repairs_existing_files(tmp_path, monkeypatch): + from src.cache_manager import CacheManager + + calls = [] + monkeypatch.setattr(CacheManager, '_get_writable_cache_dir', lambda self: str(tmp_path)) + monkeypatch.setattr(DiskCache, 'share_existing_files', + lambda self: calls.append(self.cache_dir) or 0) + CacheManager._cleanup_owners.clear() + manager = CacheManager() + try: + deadline = time.monotonic() + 5 + while not calls and time.monotonic() < deadline: + time.sleep(0.01) + finally: + manager.stop_cleanup_thread() + CacheManager._cleanup_owners.clear() + + assert calls == [str(tmp_path)] diff --git a/test/test_web_display_state_freshness.py b/test/test_web_display_state_freshness.py new file mode 100644 index 00000000..3349db00 --- /dev/null +++ b/test/test_web_display_state_freshness.py @@ -0,0 +1,62 @@ +"""Tests that the web UI reports the display's state as it is now. + +The display service writes display_on_demand_state and display_current_state; +the web interface is a different process and can only see them on disk. Its +reads went through the cache's memory tier, which keeps the first copy read +for the full max_age (120s). Measured on a rig once the web interface could +read these files at all: /display/on-demand/status said "active" for over 100 +seconds while the file on disk had said "idle" the whole time. +""" + +import json + +import pytest +from flask import Flask + +import web_interface.blueprints.api_v3 as api_pkg +from src.cache_manager import CacheManager + + +@pytest.fixture +def two_processes(tmp_path, monkeypatch): + """A display-side and a web-side cache over one directory, as on a rig.""" + monkeypatch.setattr(CacheManager, '_get_writable_cache_dir', lambda self: str(tmp_path)) + monkeypatch.setattr(CacheManager, 'start_cleanup_thread', lambda self: None) + display, web = CacheManager(), CacheManager() + monkeypatch.setattr(api_pkg, 'cache_manager', web) + monkeypatch.setattr(api_pkg, '_get_display_service_status', lambda: {'active': True}) + return display + + +@pytest.fixture +def client(): + app = Flask(__name__) + app.config['TESTING'] = True + if 'api_v3' not in app.blueprints: + app.register_blueprint(api_pkg.api_v3, url_prefix='/api/v3') + with app.test_client() as c: + yield c + + +def _get(client, url): + response = client.get(url) + assert response.status_code == 200 + return json.loads(response.data)['data'] + + +def test_on_demand_status_sees_the_stop_immediately(client, two_processes): + two_processes.set('display_on_demand_state', {'active': True, 'status': 'active'}) + assert _get(client, '/api/v3/display/on-demand/status')['state']['status'] == 'active' + + two_processes.set('display_on_demand_state', {'active': False, 'status': 'idle'}) + + assert _get(client, '/api/v3/display/on-demand/status')['state']['status'] == 'idle' + + +def test_current_status_sees_the_mode_change_immediately(client, two_processes): + two_processes.set('display_current_state', {'mode': 'odds_ticker'}) + assert _get(client, '/api/v3/display/current-status')['mode'] == 'odds_ticker' + + two_processes.set('display_current_state', {'mode': 'stocks'}) + + assert _get(client, '/api/v3/display/current-status')['mode'] == 'stocks' diff --git a/web_interface/blueprints/api_v3/display.py b/web_interface/blueprints/api_v3/display.py index afb2f5d9..aebc0546 100644 --- a/web_interface/blueprints/api_v3/display.py +++ b/web_interface/blueprints/api_v3/display.py @@ -138,7 +138,11 @@ def get_on_demand_status(): """Return the current on-demand display state.""" try: cache = _ensure_cache_manager() - state = cache.get('display_on_demand_state', max_age=120) + # memory_ttl=0: the display service writes this key, so only the file + # is current. This process's memory tier would keep serving the first + # copy it read for the full max_age -- "active" for two minutes after + # the display had already stopped. + state = cache.get('display_on_demand_state', max_age=120, memory_ttl=0) if state is None: state = { 'active': False, @@ -304,7 +308,8 @@ def get_current_display_status(): """ try: cache = _ensure_cache_manager() - state = cache.get('display_current_state', max_age=120) + # memory_ttl=0: written by the display service; see get_on_demand_status. + state = cache.get('display_current_state', max_age=120, memory_ttl=0) if state is None: state = { 'mode': None, From 7e5967e16038d304f2c5864a54c793b39c26726a Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Thu, 17 Sep 2026 12:38:55 -0400 Subject: [PATCH 3/3] fix(web): find installed plugins before anything has discovered them (#594) The web process discovers plugins lazily: plugin_manifests is empty until some endpoint calls discover_plugins(). Three routes consulted it without discovering, so they misbehaved for as long as nothing else had run -- which, after every ledmatrix-web restart, is until someone opens the dashboard: - POST /display/on-demand/start answered 404 "Plugin not found" (or "Mode not found"). Measured on a rig: 404 for over three minutes after a web restart, until GET /plugins/installed ran. The browser UI loads the plugin list first, so API-only callers (the Home Assistant MQTT bridge, scripts) are the ones who hit it. - POST /plugins/toggle answered 404 "Plugin not found". - POST /config/main did not recognise a plugin section, so it skipped secret separation and merged the section as-is: the plugin's API key was written to config.json in plain text instead of config_secrets.json. Add _discovered_plugin_manifests(), which discovers when nothing has been yet, and rescans once when a specific plugin id (or, for on-demand by mode, a mode) is not found, so a plugin installed since the last scan is found too. _installed_plugin_ids() now uses it. Co-authored-by: Claude Opus 5 --- test/test_api_v3_lazy_plugin_discovery.py | 161 ++++++++++++++++++++ web_interface/blueprints/api_v3/__init__.py | 40 +++-- web_interface/blueprints/api_v3/config.py | 7 +- web_interface/blueprints/api_v3/display.py | 8 +- web_interface/blueprints/api_v3/plugins.py | 2 +- 5 files changed, 205 insertions(+), 13 deletions(-) create mode 100644 test/test_api_v3_lazy_plugin_discovery.py diff --git a/test/test_api_v3_lazy_plugin_discovery.py b/test/test_api_v3_lazy_plugin_discovery.py new file mode 100644 index 00000000..8a9b5aa7 --- /dev/null +++ b/test/test_api_v3_lazy_plugin_discovery.py @@ -0,0 +1,161 @@ +"""Tests that api_v3 routes find installed plugins before anything else has. + +The web process discovers plugins lazily (web_interface/app.py): nothing scans +the plugins directory at startup, and plugin_manifests stays empty until some +endpoint calls discover_plugins(). Routes that consulted plugin_manifests +without discovering therefore misbehaved for as long as nothing else had run. +Measured on a rig after restarting ledmatrix-web: + + POST /api/v3/display/on-demand/start {"plugin_id": "ledmatrix-stocks"} + -> 404 "Plugin ledmatrix-stocks not found", for over three minutes, + until GET /api/v3/plugins/installed happened to discover plugins. + +The browser UI loads the plugin list first, so people rarely saw it; API-only +callers (the Home Assistant MQTT bridge, scripts) saw it after every restart. +/plugins/toggle answered the same 404, and /config/main was worse: an +undiscovered plugin section skipped secret separation and wrote its API key +into config.json in plain text. + +A real PluginManager over a temporary plugins directory, so "empty until +discovered" is the real behaviour rather than a mock's. +""" + +import json +import sys +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + +sys.path.insert(0, str(Path(__file__).parent.parent)) + +from src.config_manager import ConfigManager # noqa: E402 +from src.plugin_system.plugin_manager import PluginManager # noqa: E402 +from test._api_v3_test_helpers import api_v3_client, api_v3_module # noqa: F401,E402 + +PLUGIN_ID = 'ledmatrix-stocks' +MODE = 'stocks' + + +def _install(plugins_dir, plugin_id, modes, schema=None): + plugin_dir = plugins_dir / plugin_id + plugin_dir.mkdir(parents=True) + (plugin_dir / 'manifest.json').write_text(json.dumps({ + 'id': plugin_id, 'name': plugin_id, 'version': '1.0.0', + 'entry_point': 'manager.py', 'class_name': 'Plugin', + 'display_modes': modes, + })) + (plugin_dir / 'manager.py').write_text('') + if schema is not None: + (plugin_dir / 'config_schema.json').write_text(json.dumps(schema)) + + +@pytest.fixture +def plugins_dir(tmp_path): + path = tmp_path / 'plugin-repos' + path.mkdir() + _install(path, PLUGIN_ID, [MODE], schema={ + 'type': 'object', + 'properties': { + 'enabled': {'type': 'boolean'}, + 'symbols': {'type': 'string'}, + 'api_key': {'type': 'string', 'x-secret': True}, + }, + }) + return path + + +@pytest.fixture +def fresh_web_process(api_v3_module, plugins_dir): + """The web process right after a restart: nothing discovered yet.""" + manager = PluginManager(plugins_dir=str(plugins_dir)) + assert not manager.plugin_manifests + api_v3_module.api_v3.plugin_manager = manager + return manager + + +@pytest.fixture +def display_service(): + """Keep on-demand start away from systemctl and the real cache.""" + with patch('web_interface.blueprints.api_v3.display._ensure_cache_manager') as cache, \ + patch('web_interface.blueprints.api_v3.display._get_display_service_status') as status: + cache.return_value = MagicMock() + status.return_value = {'active': True} + yield cache.return_value + + +def _start(client, **body): + body.setdefault('start_service', False) + return client.post('/api/v3/display/on-demand/start', json=body) + + +class TestOnDemandStart: + def test_by_plugin_id(self, api_v3_client, fresh_web_process, display_service): + response = _start(api_v3_client, plugin_id=PLUGIN_ID) + + assert response.status_code == 200, response.get_json() + request = display_service.set.call_args.args[1] + assert (request['plugin_id'], request['mode']) == (PLUGIN_ID, MODE) + + def test_by_mode_alone(self, api_v3_client, fresh_web_process, display_service): + response = _start(api_v3_client, mode=MODE) + + assert response.status_code == 200, response.get_json() + assert display_service.set.call_args.args[1]['plugin_id'] == PLUGIN_ID + + def test_a_plugin_installed_since_the_last_scan( + self, api_v3_client, fresh_web_process, plugins_dir, display_service): + fresh_web_process.discover_plugins() + _install(plugins_dir, 'ledmatrix-weather', ['weather']) + + assert _start(api_v3_client, plugin_id='ledmatrix-weather').status_code == 200 + assert _start(api_v3_client, mode=MODE).status_code == 200 + + def test_a_mode_installed_since_the_last_scan( + self, api_v3_client, fresh_web_process, plugins_dir, display_service): + fresh_web_process.discover_plugins() + _install(plugins_dir, 'ledmatrix-weather', ['weather']) + + response = _start(api_v3_client, mode='weather') + + assert response.status_code == 200, response.get_json() + + def test_an_unknown_plugin_is_still_not_found( + self, api_v3_client, fresh_web_process, display_service): + assert _start(api_v3_client, plugin_id='no-such-plugin').status_code == 404 + assert _start(api_v3_client, mode='no-such-mode').status_code == 404 + display_service.set.assert_not_called() + + +def test_toggle_finds_the_plugin(api_v3_client, api_v3_module, fresh_web_process): + config_manager = api_v3_module.api_v3.config_manager + config_manager.load_config.return_value = {PLUGIN_ID: {'enabled': False}} + config_manager.save_config_atomic.return_value = MagicMock( + status=MagicMock(value='success')) + + response = api_v3_client.post('/api/v3/plugins/toggle', + json={'plugin_id': PLUGIN_ID, 'enabled': True}) + + assert response.status_code == 200, response.get_json() + saved = config_manager.save_config_atomic.call_args.args[0] + assert saved[PLUGIN_ID]['enabled'] is True + + +def test_main_config_save_keeps_a_plugin_secret_out_of_config_json( + api_v3_client, api_v3_module, fresh_web_process, tmp_path): + config_file = tmp_path / 'config.json' + config_file.write_text('{}') + secrets_file = tmp_path / 'config_secrets.json' + config_manager = ConfigManager(config_path=str(config_file), + secrets_path=str(secrets_file)) + config_manager.template_path = str(tmp_path / 'no-template.json') + api_v3_module.api_v3.config_manager = config_manager + + response = api_v3_client.post('/api/v3/config/main', json={ + PLUGIN_ID: {'symbols': 'AAPL', 'api_key': 's3cret-key'}, + }) + + assert response.status_code == 200, response.get_json() + assert 's3cret' not in config_file.read_text() + assert json.loads(secrets_file.read_text())[PLUGIN_ID]['api_key'] == 's3cret-key' + assert json.loads(config_file.read_text())[PLUGIN_ID]['symbols'] == 'AAPL' diff --git a/web_interface/blueprints/api_v3/__init__.py b/web_interface/blueprints/api_v3/__init__.py index 93ff7703..4b3b4fd2 100644 --- a/web_interface/blueprints/api_v3/__init__.py +++ b/web_interface/blueprints/api_v3/__init__.py @@ -568,21 +568,41 @@ def _installed_plugin_ids(): enumerate the installed plugins and read each one's persisted summary by ID instead of relying on the tracker's in-memory `get_all_*` view. """ - pm = api_v3.plugin_manager - manifests = getattr(pm, 'plugin_manifests', None) - if not manifests: - # Only pay for a discovery scan when we haven't discovered anything yet; - # subsequent polls reuse the already-populated manifest map. - try: - pm.discover_plugins() - except Exception: - logger.debug('discover_plugins failed while listing plugin ids', exc_info=True) - manifests = getattr(pm, 'plugin_manifests', None) + manifests = _discovered_plugin_manifests() try: return list(manifests.keys()) if manifests else [] except Exception: logger.debug('listing plugin_manifests failed while building plugin ids', exc_info=True) return [] +def _discovered_plugin_manifests(plugin_id=None, rescan=False): + """The plugin manager's manifests, discovering plugins first if needed. + + The web process discovers plugins lazily (see app.py): nothing scans at + startup, so plugin_manifests is empty until some endpoint calls + discover_plugins(). A route that looks a plugin up without coming through + here answers "not found" for every installed plugin until something else + has run -- after a web restart, POST /display/on-demand/start returned 404 + for minutes on a real rig, and only API-only callers ever noticed. + + Scans when nothing is discovered yet, when ``plugin_id`` is given and not + among the manifests (it may have been installed since the last scan), or + when ``rescan`` is set (for lookups that are not by id, such as a mode). + Otherwise the existing map is reused, so a steady stream of requests + for known plugins costs nothing. + + Returns the manifest map, or {} when there is no plugin manager. + """ + pm = api_v3.plugin_manager + if pm is None: + return {} + manifests = getattr(pm, 'plugin_manifests', None) + if not manifests or rescan or (plugin_id is not None and plugin_id not in manifests): + try: + pm.discover_plugins() + except Exception: + logger.warning('Plugin discovery failed', exc_info=True) + manifests = getattr(pm, 'plugin_manifests', None) + return manifests or {} def _do_transactional_uninstall(plugin_id, preserve_config): """Execute an uninstall with snapshot-based rollback. diff --git a/web_interface/blueprints/api_v3/config.py b/web_interface/blueprints/api_v3/config.py index d1bd34d7..2aa9e3b2 100644 --- a/web_interface/blueprints/api_v3/config.py +++ b/web_interface/blueprints/api_v3/config.py @@ -960,9 +960,14 @@ def save_main_config(): # Any key that matches a plugin ID should be saved as plugin config # This includes proper secret field handling from schema plugin_keys_to_remove = [] + # Discovered first: a plugin key not recognised here skips secret + # separation below and falls through to the generic merge, which wrote + # the plugin's API key into config.json in plain text whenever nothing + # had yet discovered plugins in this process (any save after a restart). + plugin_manifests = _pkg._discovered_plugin_manifests() for key in data: # Check if this key is a plugin ID - if api_v3.plugin_manager and key in api_v3.plugin_manager.plugin_manifests: + if api_v3.plugin_manager and key in plugin_manifests: plugin_id = key plugin_config = data[key] diff --git a/web_interface/blueprints/api_v3/display.py b/web_interface/blueprints/api_v3/display.py index aebc0546..7b632c94 100644 --- a/web_interface/blueprints/api_v3/display.py +++ b/web_interface/blueprints/api_v3/display.py @@ -178,14 +178,20 @@ def start_on_demand_display(): resolved_mode = mode if api_v3.plugin_manager: - if resolved_plugin and resolved_plugin not in api_v3.plugin_manager.plugin_manifests: + if resolved_plugin and resolved_plugin not in _pkg._discovered_plugin_manifests(resolved_plugin): return jsonify({'status': 'error', 'message': f'Plugin {resolved_plugin} not found'}), 404 if resolved_plugin and not resolved_mode: modes = api_v3.plugin_manager.get_plugin_display_modes(resolved_plugin) resolved_mode = modes[0] if modes else resolved_plugin elif resolved_mode and not resolved_plugin: + _pkg._discovered_plugin_manifests() resolved_plugin = api_v3.plugin_manager.find_plugin_for_mode(resolved_mode) + if not resolved_plugin: + # Not among what was discovered: the plugin that declares + # it may have been installed since. Scan once more. + _pkg._discovered_plugin_manifests(rescan=True) + resolved_plugin = api_v3.plugin_manager.find_plugin_for_mode(resolved_mode) if not resolved_plugin: return jsonify({'status': 'error', 'message': f'Mode {resolved_mode} not found'}), 404 diff --git a/web_interface/blueprints/api_v3/plugins.py b/web_interface/blueprints/api_v3/plugins.py index c9643e54..fd8f6aa3 100644 --- a/web_interface/blueprints/api_v3/plugins.py +++ b/web_interface/blueprints/api_v3/plugins.py @@ -453,7 +453,7 @@ def toggle_plugin(): return _toggle_starlark_app(plugin_id[len('starlark:'):], enabled) # Check if plugin exists in manifests (discovered but may not be loaded) - if plugin_id not in api_v3.plugin_manager.plugin_manifests: + if plugin_id not in _pkg._discovered_plugin_manifests(plugin_id): return jsonify({'status': 'error', 'message': 'Plugin not found'}), 404 # Update config (this is what the display controller reads)