Merge remote-tracking branch 'origin/main' into claude/fix-cache-write-dedup

# Conflicts:
#	CHANGELOG.md

CHANGELOG.md

@@ -19,30 +19,6 @@ accepts both, but the store flags the old spelling as deprecated
 
 ## Unreleased
 
-### Security
-
-- The web interface refuses state-changing requests (`POST`, `PUT`, `PATCH`,
-  `DELETE`) sent by another website's page. Any site a LAN user visited could
-  make their browser submit a plain HTML form to `http://<pi>:5000` -- CORS
-  does not stop such a request, only hides its answer -- and
-  `/api/v3/system/action` accepted form bodies, so that page could reboot or
-  power off the Pi, pull code, or reach any other mutating route. A request
-  whose `Origin` (or, without one, `Referer`) is not the host it was sent to,
-  or is `null`, now gets 403 `CROSS_SITE_REQUEST`
-  (`web_interface/origin_guard.py`). `/api/v3/system/action` also refuses a
-  form-encoded or `text/plain` body (415) unless it carries HTMX's
-  `HX-Request` header; every caller in the interface already sends JSON.
-- **Behaviour change for API scripts:** clients that send no `Origin` or
-  `Referer` -- curl, Python `requests`, Home Assistant, the MQTT bridge --
-  are unaffected. A browser page served from a *different* origin (a
-  dashboard or userscript on another host) can no longer call the mutating
-  API; call it server-side instead. Anyone posting a form body to
-  `system/action` must switch to JSON. Behind a reverse proxy, forward the
-  original `Host`, port included (`proxy_set_header Host $http_host;`;
-  nginx's `$host` drops the port); `X-Forwarded-Host` is not trusted. A
-  TLS-terminating proxy needs nothing more: a portless `Host` matches an
-  `https://` page.
-
 ### Fixes
 
 - On-demand no longer restarts a running display. `POST
@@ -54,6 +30,27 @@ accepts both, but the store flags the old spelling as deprecated
   mid-screen and mid-Vegas included, so the route now only starts the service
   when it is not running. `POST /display/on-demand/stop` reads
   `stop_service` as a boolean, so `"false"` no longer stops the service.
+- On-demand works for a disabled plugin. The display only loads enabled
+  plugins, so "Preview on display" on a disabled plugin's config page (which
+  says the plugin will be enabled for the preview) failed with
+  `invalid-mode`. The display now loads the plugin live for the session,
+  without writing `enabled` to `config.json`, and unloads it when on-demand
+  is stopped, expires or moves to another plugin. A plugin that fails to
+  load reports on-demand status `error` with `load-failed`. A session
+  restored after a restart unloads its disabled plugin the same way; it used
+  to stay loaded until the next restart.
+- A stop request now clears an on-demand error. After a failed request,
+  `/display/on-demand/status` kept reporting `status: error` for up to two
+  minutes even after a stop.
+- Re-saving unchanged data through `CacheManager.set` no longer rewrites its
+  cache file. The disk cache already skipped a payload identical to the last
+  one written, but `set()` stamps every record with the current time, so the
+  skip never fired and every plugin rewrote its unchanged API data to the SD
+  card on every update cycle. Records are now compared without that
+  timestamp, and a skipped write moves the file's mtime to it instead; reads
+  treat a record as fresh from the later of the two, so it expires exactly
+  when the rewrite would have. Changed data or a changed `ttl` still writes,
+  and so does a file another process has replaced since.
 
 ## 3.7.0
 

SECURITY.md

@@ -63,14 +63,6 @@ are intentional rather than vulnerabilities:
 
 - **No web UI authentication.** The web interface assumes the network
   it's running on is trusted. Don't expose port 5000 to the internet.
-  "Trusted network" does not mean "trusted websites", though: any page
-  a LAN user opens could make their browser POST to the Pi. So the
-  interface refuses a `POST`/`PUT`/`PATCH`/`DELETE` whose `Origin` (or
-  `Referer`) header names another site (`web_interface/origin_guard.py`),
-  and `/api/v3/system/action` only accepts JSON or HTMX requests. Tools
-  that send neither header (curl, Home Assistant, the MQTT bridge) are
-  unaffected. Not covered: DNS rebinding, and anyone who can reach the
-  port directly.
 - **Plugins run unsandboxed.** Installed plugins execute in the same
   Python process as the display loop with full file-system and
   network access. Review plugin code (especially third-party plugins

docs/ARCHITECTURE.md

@@ -84,7 +84,11 @@ then normal rotation.
 - **On-demand.** A request from the web interface pins one plugin (or mode)
   for a duration. `_activate_on_demand()` / `_clear_on_demand()`; the
   session is saved under `display_on_demand_config` so it survives a
-  restart. It also keeps the display on during scheduled off hours.
+  restart. It also keeps the display on during scheduled off hours. A
+  request for a plugin that is disabled in config loads it live
+  (`_load_plugin_for_on_demand()`, `load_plugin(force_enabled=True)`)
+  without writing `config.json`; the main loop unloads it once on-demand
+  moves off it (`_release_on_demand_plugins()`).
 - **Live priority.** `_check_live_priority()` looks for a plugin whose
   `has_live_priority()` and `has_live_content()` are both true and switches
   to it, rotating between several live games.

docs/REST_API_REFERENCE.md

@@ -18,17 +18,6 @@ top level instead of under `data` (install-from-url, registry-from-url, the
 auth endpoints, upload endpoints, `system/git-info`, `system/check-update`),
 the entry below says so.
 
-**Cross-site requests are refused.** A `POST`, `PUT`, `PATCH` or `DELETE`
-carrying an `Origin` header (or, without one, a `Referer`) that is not the
-host the request was sent to gets `403` with `"error_code":
-"CROSS_SITE_REQUEST"`; so does `Origin: null`. This stops other websites from
-driving the Pi through a LAN user's browser. Scripts, curl, Home Assistant and
-the MQTT bridge send neither header and are unaffected. A browser page on
-another origin (a dashboard you host elsewhere, say) can no longer call the
-API; call it server-side instead. Behind a reverse proxy, pass the original
-`Host` through, port included (nginx: `proxy_set_header Host $http_host;`;
-`$host` drops the port) -- `X-Forwarded-Host` is not read.
-
 ## Table of Contents
 
 - [Configuration](#configuration)
@@ -1399,10 +1388,7 @@ Fetches `origin` and lists branches to switch to: `current`, `upstream`,
 
 **POST** `/api/v3/system/action`
 
-Execute system-level actions. Send JSON (`Content-Type: application/json`).
+Execute system-level actions. JSON or form data.
-A form-encoded or `text/plain` body is accepted only with an `HX-Request`
-header (HTMX sends it; a cross-site HTML form cannot) and is otherwise
-refused with `415`.
 
 **Request Body**:
 ```json

docs/WEB_INTERFACE_GUIDE.md

@@ -412,16 +412,6 @@ The API blueprint (`web_interface/blueprints/api_v3/`) is registered at
 - No authentication is currently implemented
 - Recommended for trusted networks only
 
-**Other websites:**
-- A web page you open elsewhere could otherwise make your browser send
-  commands to the Pi (reboot, update, config changes). The interface refuses
-  any change request whose `Origin`/`Referer` header names a different site
-  (403 `CROSS_SITE_REQUEST`), so use the interface from its own address.
-- Scripts, curl, Home Assistant and the MQTT bridge send no such header and
-  keep working. Behind a reverse proxy, forward the original `Host` header
-  with its port (nginx: `proxy_set_header Host $http_host;` -- `$host`
-  drops the port).
-
 **Best Practices:**
 1. Run on a private network (not exposed to internet)
 2. Use a firewall to restrict access if needed

src/cache/disk_cache.py

@@ -14,7 +14,7 @@ import tempfile
 import logging
 import threading
 import zlib
-from typing import Dict, Any, Optional, Protocol
+from typing import Dict, Any, Optional, Protocol, Tuple
 from datetime import datetime
 
 from src.common.path_safety import safe_path_component
@@ -111,18 +111,66 @@ _HEAD_RE = re.compile(
 )
 
 
-def _stale_from_head(head: bytes, max_age: Optional[int], now: float) -> bool:
+def _head_timestamp(head: bytes) -> Optional[Tuple[float, int]]:
+    """A header-first record's timestamp and the offset just past it.
+
+    None when the record does not start with a finite numeric timestamp.
+    """
+    match = _HEAD_RE.match(head)
+    if not match:
+        return None
+    try:
+        timestamp = float(match.group(1))
+    except ValueError:
+        return None
+    if not math.isfinite(timestamp):
+        return None
+    return timestamp, match.end(1)
+
+
+# FRESHNESS OF A SKIPPED WRITE
+# ----------------------------
+# CacheManager.set stamps every record with time.time(), so re-saving
+# unchanged data produced a different payload every time and DiskCache.set's
+# identical-payload skip never fired: every plugin rewrote its unchanged API
+# data to the SD card every update cycle. set() now compares header-first
+# records without their timestamp, and on a skip moves the file's mtime to
+# the timestamp the skipped record carried instead of rewriting it. So the
+# file's mtime is when its content was last saved, and a header-first
+# record is as fresh as the later of its embedded timestamp and its mtime.
+#
+# A real write sets the mtime to the embedded timestamp too, so mtime is
+# never later than the timestamp for a record written with an old one on
+# purpose -- only a skip can move it forward.
+
+
+def _refreshed_at(timestamp: float, mtime: float) -> float:
+    """When a header-first record was last saved, embedded time or mtime.
+
+    An mtime within a second of the timestamp is the write that carried it
+    (float rounding, or an older file whose mtime was not set to match),
+    not a skipped rewrite, and leaves the record as written.
+    """
+    return mtime if mtime > timestamp + 1.0 else timestamp
+
+
+def _stale_from_head(head: bytes, max_age: Optional[int], now: float,
+                     refreshed: Optional[float] = None) -> bool:
     """True when a record's header alone shows it has expired.
 
     Mirrors the expiry rule in DiskCache.get: a per-entry ttl wins over the
     caller's max_age, and no limit at all means never stale. False whenever the
     header cannot be read, so the full parse decides as it always did.
+    ``refreshed`` is the file's mtime: a skipped rewrite advances it rather
+    than the embedded timestamp (see "FRESHNESS OF A SKIPPED WRITE").
     """
     match = _HEAD_RE.match(head)
     if not match:
         return False
     try:
         timestamp = float(match.group(1))
+        if refreshed is not None:
+            timestamp = max(timestamp, refreshed)
         limit = max_age
         if match.group(2) is not None:
             ttl = float(match.group(2))
@@ -248,11 +296,13 @@ class DiskCache:
         self.cache_dir = cache_dir
         self.logger = logger or logging.getLogger(__name__)
         self._lock = threading.Lock()
-        # key -> adler32 of the last payload successfully written to the
+        # key -> ((length, adler32) of the last content written to the
-        # primary cache path; lets set() skip rewriting identical data
+        # primary cache path, (st_ino, st_size) of the file it left); lets
-        # (per-process only — worst case another process rewrites, never
+        # set() skip rewriting identical data. The file identity catches
-        # a missed write). Guarded by _lock.
+        # another process -- the web interface writes and clears keys too --
-        self._write_digests: Dict[str, int] = {}
+        # having replaced the file since, which would otherwise make the skip
+        # a missed write. Per-process only. Guarded by _lock.
+        self._write_digests: Dict[str, Tuple[Tuple[int, int], Tuple[int, int]]] = {}
     
     def get_cache_path(self, key: str) -> Optional[str]:
         """
@@ -306,7 +356,12 @@ class DiskCache:
                     # records (a season schedule is re-fetched when its cache
                     # expires), and parsing 53MB to throw it away held the GIL
                     # for ~1.8s -- a visible freeze on the panel.
-                    if _stale_from_head(f.read(_HEAD_BYTES), max_age, time.time()):
+                    head = f.read(_HEAD_BYTES)
+                    stamp = _head_timestamp(head)
+                    fresh_at = None
+                    if stamp is not None:
+                        fresh_at = _refreshed_at(stamp[0], os.fstat(f.fileno()).st_mtime)
+                    if _stale_from_head(head, max_age, time.time(), fresh_at):
                         return None
                     f.seek(0)
                     record = _loads(f.read())
@@ -315,6 +370,11 @@ class DiskCache:
             record_ts = None
             if isinstance(record, dict):
                 record_ts = record.get('timestamp')
+                if fresh_at is not None and fresh_at > stamp[0]:
+                    # A skipped rewrite refreshed this record (see "FRESHNESS
+                    # OF A SKIPPED WRITE"); hand callers the time it was last
+                    # saved, as the rewrite would have.
+                    record['timestamp'] = record_ts = fresh_at
             if record_ts is None:
                 try:
                     record_ts = os.path.getmtime(cache_path)
@@ -403,24 +463,37 @@ class DiskCache:
             self.logger.warning("Cache data for key '%s' not serializable: %s", key, e)
             return
 
-        digest = zlib.adler32(payload)
+        # A header-first record is compared without its timestamp, which
+        # CacheManager.set changes on every call (see "FRESHNESS OF A SKIPPED
+        # WRITE"). The length rides along with adler32, which is weak on its
+        # own for short payloads, and a collision here is a missed write.
+        stamp = _head_timestamp(payload[:_HEAD_BYTES])
+        stamped_at = stamp[0] if stamp is not None else None
+        content = memoryview(payload)[stamp[1]:] if stamp is not None else payload
+        digest = (len(content), zlib.adler32(content))
 
         try:
             # Atomic write to avoid partial/corrupt files
             with self._lock:
-                # Skip the disk entirely when this exact payload was already
+                # Skip the disk entirely when this content was already
                 # written for this key (plugins re-save unchanged API data
                 # every update cycle — each write is real SD-card wear).
-                # Refresh the file mtime so records that rely on it for TTL
+                # Move the file mtime instead, so the record stays as fresh as
-                # (no embedded 'timestamp') don't expire early; a metadata
+                # the rewrite would have left it; a metadata touch is
-                # touch is journal-cheap compared to rewriting the data.
+                # journal-cheap compared to rewriting the data.
-                if self._write_digests.get(key) == digest:
+                known = self._write_digests.get(key)
+                if known is not None and known[0] == digest:
                     try:
-                        os.utime(cache_path, None)
+                        st = os.stat(cache_path)
-                        return
+                        if (st.st_ino, st.st_size) == known[1]:
+                            os.utime(cache_path, None if stamped_at is None
+                                     else (stamped_at, stamped_at))
+                            return
                     except OSError:
-                        # File vanished or perms changed — fall through and write
+                        pass
-                        self._write_digests.pop(key, None)
+                    # File vanished, was replaced by another process, or its
+                    # times cannot be set — fall through and write
+                    self._write_digests.pop(key, None)
 
                 tmp_dir = os.path.dirname(cache_path)
                 # Try to create temp file in cache directory first
@@ -458,7 +531,7 @@ class DiskCache:
                                 # 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
+                            self._remember_write(key, cache_path, digest, stamped_at)
                         finally:
                             if os.path.exists(tmp_path):
                                 try:
@@ -471,7 +544,7 @@ class DiskCache:
                             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
+                            self._remember_write(key, cache_path, digest, stamped_at)
                             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
@@ -520,6 +593,26 @@ class DiskCache:
             )
             return  # Exit gracefully without raising exception
     
+    def _remember_write(self, key: str, cache_path: str,
+                        digest: Tuple[int, int], stamped_at: Optional[float]) -> None:
+        """Record a completed write so an identical set() can skip the disk.
+
+        Caller holds _lock. A header-first record's mtime is set to its
+        timestamp, so only a skipped rewrite ever moves it later (see
+        "FRESHNESS OF A SKIPPED WRITE").
+        """
+        try:
+            if stamped_at is not None:
+                os.utime(cache_path, (stamped_at, stamped_at))
+            st = os.stat(cache_path)
+        except OSError:
+            # Written but not stamped (another user's file, on the direct
+            # write path): mtime is the write time, which _refreshed_at reads
+            # as the write itself. Remember nothing; the next set() writes.
+            self._write_digests.pop(key, None)
+            return
+        self._write_digests[key] = (digest, (st.st_ino, st.st_size))
+
     def clear(self, key: Optional[str] = None) -> None:
         """
         Clear cache entry or all entries.

src/display_controller.py

@@ -29,7 +29,7 @@ import threading
 import types
 from collections import deque
 from contextlib import contextmanager
-from typing import Dict, Any, List, Optional, Callable, Tuple
+from typing import Dict, Any, List, Optional, Callable, Set, Tuple
 from datetime import datetime
 from concurrent.futures import ThreadPoolExecutor, as_completed  # pylint: disable=no-name-in-module
 import pytz
@@ -266,6 +266,10 @@ class DisplayController:
         self.on_demand_last_error: Optional[str] = None
         self.on_demand_last_event: Optional[str] = None
         self.on_demand_schedule_override = False
+        # Plugins that are disabled in config and loaded only because an
+        # on-demand request named them. The main loop unloads each one once
+        # on-demand has moved off it (_release_on_demand_plugins).
+        self._on_demand_loaded_plugins: Set[str] = set()
         self.rotation_resume_index: Optional[int] = None
         # Saved rotation position when a live-priority plugin preempts the
         # rotation, so it resumes where it left off (not after the live plugin)
@@ -369,7 +373,11 @@ class DisplayController:
                 """Load a single plugin and return result."""
                 plugin_load_start = time.time()
                 try:
-                    if self.plugin_manager.load_plugin(plugin_id):
+                    if plugin_id in self._on_demand_loaded_plugins:
+                        loaded = self.plugin_manager.load_plugin(plugin_id, force_enabled=True)
+                    else:
+                        loaded = self.plugin_manager.load_plugin(plugin_id)
+                    if loaded:
                         plugin_load_time = time.time() - plugin_load_start
                         return {
                             'success': True,
@@ -1475,8 +1483,13 @@ class DisplayController:
         On-demand still resumes on its saved mode; this only widens what gets
         loaded, so normal rotation has somewhere to return to when it ends.
         A plugin that is disabled in config but named by the on-demand request
-        is still enabled and added, since otherwise the mode being resumed
+        is still loaded, since otherwise the mode being resumed would have
-        would have nothing behind it.
+        nothing behind it. It is tracked as loaded for on-demand only, the
+        same as one loaded live by _activate_on_demand, so it is unloaded
+        when the session ends instead of staying loaded until the next
+        restart. Its config section is not touched: setting ``enabled`` in
+        self.config wrote into the dict config_manager caches and returns to
+        every later load_config() in this process.
         """
         enabled_plugins = [p for p in discovered_plugins
                            if self.config.get(p, {}).get('enabled', False)]
@@ -1491,11 +1504,10 @@ class DisplayController:
             logger.warning("Falling back to normal mode (all enabled plugins)")
             return enabled_plugins
 
-        if not self.config.get(on_demand_plugin_id, {}).get('enabled', False):
+        if on_demand_plugin_id not in enabled_plugins:
-            logger.info("Temporarily enabling plugin '%s' for on-demand mode", on_demand_plugin_id)
+            logger.info("Loading disabled plugin '%s' for on-demand mode only", on_demand_plugin_id)
-            self.config.setdefault(on_demand_plugin_id, {})['enabled'] = True
+            self._on_demand_loaded_plugins.add(on_demand_plugin_id)
-            if on_demand_plugin_id not in enabled_plugins:
+            enabled_plugins.append(on_demand_plugin_id)
-                enabled_plugins.append(on_demand_plugin_id)
 
         # Restore on-demand state from the cached request so it resumes.
         self.on_demand_active = True
@@ -1591,6 +1603,11 @@ class DisplayController:
                 logger.debug("Stop request %s received but on-demand is not active", request_id)
                 # Still update request_id to acknowledge the request
                 self.on_demand_request_id = request_id
+                if self.on_demand_status == 'error':
+                    # A failed request left status 'error' published, and
+                    # without this the status route kept reporting it until
+                    # the state aged out (120s) or another request came in.
+                    self._clear_on_demand(reason='requested-stop')
             # Stop requests are deliberately exempt from the request_id/
             # processed_id guards above, so that a second click stops a mode
             # that a race left running. Consuming the mailbox is therefore the
@@ -1757,10 +1774,136 @@ class DisplayController:
                    plugin_id, ordered_modes, self.on_demand_mode_index, 
                    ordered_modes[self.on_demand_mode_index] if ordered_modes else 'N/A')
 
+    def _load_plugin_for_on_demand(self, plugin_id: str) -> bool:
+        """Load an installed plugin that isn't running so on-demand can show it.
+
+        This process only loads the plugins enabled in config, so a request
+        for a disabled one -- the config page's "Preview on display" button
+        offers it on every plugin -- failed with "invalid-mode" while the UI
+        said the plugin would be enabled for the session. Nothing did that
+        short of a restart, and restarts no longer happen on a request.
+
+        Loads through the same path as a live enable (load_plugin, then
+        _register_loaded_plugin), with force_enabled so the instance runs
+        enabled while config.json keeps saying disabled. The plugin is
+        recorded in _on_demand_loaded_plugins, and the main loop unloads it
+        once on-demand moves off it (_release_on_demand_plugins).
+
+        Returns False after publishing an error when the load fails. A
+        plugin that isn't installed returns True without loading anything:
+        the mode checks that follow report it as they always have.
+        """
+        if self.plugin_manager is None:
+            return True
+        try:
+            known = self.plugin_manager.discovered_plugin_ids()
+        except AttributeError:
+            known = set(getattr(self.plugin_manager, 'plugin_manifests', ()) or ())
+        if plugin_id not in known:
+            # Installed after this process scanned: the web process checked
+            # its own, fresher list before posting the request.
+            try:
+                known = set(self.plugin_manager.discover_plugins())
+            except Exception:  # pylint: disable=broad-except
+                logger.exception("On-demand: plugin discovery failed")
+                known = set()
+            if plugin_id not in known:
+                return True
+
+        logger.info("On-demand: loading disabled plugin '%s' for this session only", plugin_id)
+        self._on_demand_loaded_plugins.add(plugin_id)
+        try:
+            loaded = self.plugin_manager.load_plugin(plugin_id, force_enabled=True)
+            if loaded:
+                modes = self._register_loaded_plugin(plugin_id)
+                logger.info("On-demand: loaded plugin '%s' (modes: %s)", plugin_id, modes)
+        except Exception:  # pylint: disable=broad-except
+            logger.exception("On-demand: error loading plugin '%s'", plugin_id)
+            loaded = False
+        if not loaded:
+            # Stays in _on_demand_loaded_plugins so the main loop removes
+            # whatever part of it did get registered.
+            logger.error("On-demand: could not load plugin '%s'", plugin_id)
+            self._set_on_demand_error("load-failed")
+            return False
+        return True
+
+    def _release_on_demand_plugins(self) -> None:
+        """Unload plugins loaded only for on-demand that it has moved off.
+
+        Runs from the main loop, right after its own on-demand poll, not
+        where on-demand ends: a stop, an expiry or the next request is often
+        read from inside a render loop or a dwell sleep, where the plugin
+        being released may still be on the stack mid-display(). Unloading
+        goes through _unregister_plugin, as a live disable does, and nothing
+        is written to config.json.
+
+        A plugin the user enabled in the meantime stays loaded and takes its
+        place in the rotation, which is what the reconcile that the enable
+        queued would have done.
+        """
+        if self.plugin_manager is None:  # plugin system failed after startup restore
+            self._on_demand_loaded_plugins.clear()
+            return
+        keep = self.on_demand_plugin_id if self.on_demand_active else None
+        releasable = [p for p in self._on_demand_loaded_plugins if p != keep]
+        if not releasable:
+            return
+        try:
+            config = self.config_service.get_config()
+        except Exception as e:  # pylint: disable=broad-except
+            logger.warning("On-demand release: falling back to cached config: %s", e)
+            config = self.config
+        previous_mode = self.current_display_mode
+        for plugin_id in releasable:
+            self._on_demand_loaded_plugins.discard(plugin_id)
+            section = config.get(plugin_id)
+            if isinstance(section, dict) and section.get('enabled', False):
+                logger.info("On-demand: keeping plugin '%s' loaded; it was enabled "
+                            "while on-demand showed it", plugin_id)
+                continue
+            if (plugin_id in self.plugin_display_modes
+                    or self.plugin_manager.get_plugin(plugin_id) is not None):
+                logger.info("On-demand: unloading plugin '%s'; it is disabled in config",
+                            plugin_id)
+                self._unregister_plugin(plugin_id)
+        if not self.on_demand_active:
+            # Only outside a session: rotation_resume_index points into
+            # available_modes until the session ends.
+            self._apply_plugin_rotation_order()
+        self._resync_mode_index_after_change(previous_mode)
+        if self.current_display_mode != previous_mode:
+            self.force_change = True
+
+    def _rotation_index_outside_on_demand(self, start: int) -> Optional[int]:
+        """First index from `start` (wrapping) whose mode is not owned by a
+        plugin loaded only for on-demand, or None if every mode is.
+
+        Ending a session must not resume the rotation onto the plugin that
+        is about to be unloaded. A live load appends that plugin's modes
+        after the saved resume index, but a session restored after a
+        restart has no saved index and its plugin was ordered in with the
+        rest -- the rotation resumed onto it, and a stop read during its own
+        screen changed nothing on the panel until that screen ended.
+        """
+        if not self._on_demand_loaded_plugins:
+            return start
+        on_demand_only = {mode for plugin_id in self._on_demand_loaded_plugins
+                          for mode in self.plugin_display_modes.get(plugin_id, [])}
+        count = len(self.available_modes)
+        for step in range(count):
+            index = (start + step) % count
+            if self.available_modes[index] not in on_demand_only:
+                return index
+        return None
+
     def _activate_on_demand(self, request: Dict[str, Any]) -> None:
         """Activate on-demand mode for a specific plugin display."""
         plugin_id = request.get('plugin_id')
         mode = request.get('mode')
+        if (plugin_id and plugin_id not in self.plugin_display_modes
+                and not self._load_plugin_for_on_demand(plugin_id)):
+            return
         resolved_mode = self._resolve_mode_for_plugin(plugin_id, mode)
 
         if not resolved_mode:
@@ -1866,6 +2009,15 @@ class DisplayController:
                 self.on_demand_last_event = 'stop-request-ignored'  # Already idle
                 self._publish_on_demand_state()
             return
+        if not self.on_demand_active and self.on_demand_status == 'error':
+            # _set_on_demand_error already ended any session and dropped
+            # rotation_resume_index; the full clear below would only move
+            # the rotation and force a redraw. Just drop the error.
+            self.on_demand_status = 'idle'
+            self.on_demand_last_error = None
+            self.on_demand_last_event = reason or 'cleared'
+            self._publish_on_demand_state()
+            return
 
         self._reset_on_demand_fields()
         self.on_demand_status = 'idle'
@@ -1875,17 +2027,27 @@ class DisplayController:
         # Clear on-demand configuration from cache
         self.cache_manager.clear_cache('display_on_demand_config')
 
-        if self.rotation_resume_index is not None and self.available_modes:
+        if self.available_modes:
-            self.current_mode_index = self.rotation_resume_index % len(self.available_modes)
+            saved = self.rotation_resume_index
-            self.current_display_mode = self.available_modes[self.current_mode_index]
+            # Default to the current index if no resume index
-            logger.info("Resuming rotation from saved index %d: mode '%s'", 
+            start = saved if saved is not None else self.current_mode_index
-                       self.rotation_resume_index, self.current_display_mode)
+            index = self._rotation_index_outside_on_demand(start % len(self.available_modes))
-        elif self.available_modes:
+            if index is None:
-            # Default to first mode if no resume index
+                # Every mode belongs to a plugin loaded only for on-demand,
-            self.current_mode_index = self.current_mode_index % len(self.available_modes)
+                # which the main loop is about to unload; it then idles.
-            self.current_display_mode = self.available_modes[self.current_mode_index]
+                self.current_mode_index = 0
-            logger.info("Resuming rotation to mode '%s' (index %d)", 
+                self.current_display_mode = None
-                       self.current_display_mode, self.current_mode_index)
+                logger.info("No enabled mode to resume rotation to")
+            elif saved is not None:
+                self.current_mode_index = index
+                self.current_display_mode = self.available_modes[index]
+                logger.info("Resuming rotation from saved index %d: mode '%s'",
+                            saved, self.current_display_mode)
+            else:
+                self.current_mode_index = index
+                self.current_display_mode = self.available_modes[index]
+                logger.info("Resuming rotation to mode '%s' (index %d)",
+                            self.current_display_mode, self.current_mode_index)
         else:
             logger.warning("No available modes to resume rotation to")
 
@@ -2082,6 +2244,14 @@ class DisplayController:
                 # Handle on-demand commands before rendering
                 self._poll_on_demand_requests()
                 self._check_on_demand_expiration()
+                # Unload plugins loaded only to show them on-demand once it
+                # has moved off them. Here, where no display() is on the
+                # stack; one ended from inside a screen is caught here on
+                # the next pass.
+                if self._on_demand_loaded_plugins:
+                    self._release_on_demand_plugins()
+                    if not self.available_modes:
+                        continue  # it was all there was; idle as above
                 self._tick_plugin_updates()
                 
                 # Clean up expired WiFi status messages
@@ -3099,6 +3269,11 @@ class DisplayController:
                     prepared = prepare(_pid, new_config) if callable(prepare) else None
                     if isinstance(prepared, dict):
                         new_config = prepared
+                    if _pid in self._on_demand_loaded_plugins:
+                        # Saved while on-demand shows it: config.json still
+                        # says disabled, and on_config_change would switch
+                        # the instance off mid-session.
+                        new_config = {**new_config, 'enabled': True}
                     _plugin.on_config_change(new_config)
                     logger.debug("Plugin %s notified of config change", _pid)
                 except Exception as e:

src/plugin_system/plugin_manager.py

@@ -296,7 +296,7 @@ class PluginManager:
 
         return plugin_ids
 
-    def load_plugin(self, plugin_id: str) -> bool:
+    def load_plugin(self, plugin_id: str, force_enabled: bool = False) -> bool:
         """
         Load a plugin by ID.
         
@@ -310,6 +310,10 @@ class PluginManager:
         
         Args:
             plugin_id: Plugin identifier
+            force_enabled: Run the plugin enabled even though config.json has
+                it disabled. On-demand uses this to show a disabled plugin
+                (DisplayController._load_plugin_for_on_demand). Only the
+                instance's config says enabled; config.json is not written.
             
         Returns:
             True if loaded successfully, False otherwise
@@ -376,6 +380,12 @@ class PluginManager:
             # (prepare_plugin_config). In memory only: config.json is written
             # by saves, never by loading a plugin.
             config = self.prepare_plugin_config(plugin_id, config, schema=schema)
+            if force_enabled:
+                # A copy: prepare_plugin_config can hand back the section from
+                # config_manager's cached config, and setting the flag there
+                # would read as enabled to everything else in this process.
+                config = dict(config)
+                config['enabled'] = True
             
             # Use PluginLoader to load plugin
             plugin_instance, _module = self.plugin_loader.load_plugin(

test/conftest.py

@@ -16,7 +16,54 @@ if str(project_root) not in sys.path:
     sys.path.insert(0, str(project_root))
 
 
+class _DisarmStartupReconciliation:
+    """Import hook: every ``web_interface.app`` this process builds starts disarmed.
+
+    app.py wires itself to the checkout's real config/config.json and
+    plugin-repos/ at import, and its before_request hook launches startup
+    reconciliation on the first request any test sends. Reconciliation
+    reinstalls every configured plugin missing on disk from the live store,
+    so a full run downloaded basketball-scoreboard, calendar,
+    football-scoreboard, leaderboard and ledmatrix-stocks into the real
+    plugin-repos/ (not gitignored), minutes in, from a daemon thread no test
+    waits on. Setting ``_reconciliation_started`` is the app's own run-once
+    latch; doing it as the module finishes executing covers fixtures that
+    import the app lazily and send a request at once, and ``importlib.reload``.
+    StateReconciliation itself stays fully testable.
+    """
+
+    _MODULE = "web_interface.app"
+
+    def find_spec(self, fullname, path, target=None):
+        if fullname != self._MODULE:
+            return None
+        import importlib.machinery
+        spec = importlib.machinery.PathFinder.find_spec(fullname, path, target)
+        if spec is None or spec.loader is None:
+            return spec
+        exec_module = spec.loader.exec_module
+
+        def exec_disarmed(module):
+            exec_module(module)
+            module._reconciliation_started = True
+
+        spec.loader.exec_module = exec_disarmed
+        return spec
+
+
+_DISARM_HOOK = _DisarmStartupReconciliation()
+
+
 def pytest_configure(config):
+    sys.meta_path.insert(0, _DISARM_HOOK)
+    app_module = sys.modules.get(_DisarmStartupReconciliation._MODULE)
+    if app_module is not None:
+        app_module._reconciliation_started = True
+
+    _point_emulator_at_raw_adapter(config)
+
+
+def _point_emulator_at_raw_adapter(config):
     """Point the emulator at a per-process config that binds no socket.
 
     Six test modules set EMULATOR=true and build a real DisplayManager. The
@@ -63,7 +110,9 @@ def pytest_configure(config):
 
 
 def pytest_unconfigure(config):
-    """Remove the throwaway emulator config written by pytest_configure."""
+    """Undo pytest_configure: the import hook and the throwaway emulator config."""
+    if _DISARM_HOOK in sys.meta_path:
+        sys.meta_path.remove(_DISARM_HOOK)
     tmp_dir = getattr(config, "_ledmatrix_emulator_tmp", None)
     if tmp_dir is not None:
         import shutil

test/test_cache_write_dedup.py

@@ -0,0 +1,158 @@
+"""Re-saving unchanged data through CacheManager.set does not rewrite the file.
+
+Regression under test: DiskCache.set skipped the disk when a payload matched
+the last one written for the key, but CacheManager.set stamps every record
+with time.time(), so no two payloads ever matched and every plugin rewrote its
+unchanged API data to the SD card on every update cycle. The skip now ignores
+the timestamp, and a skipped write moves the file's mtime instead -- so the
+entry must stay exactly as fresh as the rewrite would have left it.
+"""
+
+import os
+import time
+from unittest.mock import patch
+
+import pytest
+
+from src.cache import disk_cache as disk_cache_module
+from src.cache.disk_cache import DiskCache
+from src.cache_manager import CacheManager
+
+class Clock:
+    def __init__(self):
+        self.now = time.time()
+
+    def __call__(self):
+        return self.now
+
+
+@pytest.fixture
+def clock(monkeypatch):
+    fake = Clock()
+    monkeypatch.setattr(time, "time", fake)
+    return fake
+
+
+@pytest.fixture
+def writes(monkeypatch):
+    """Paths DiskCache.set actually replaced (its atomic write path)."""
+    replaced = []
+    real = os.replace
+
+    def counting(src, dst, *args, **kwargs):
+        replaced.append(os.path.basename(dst))
+        return real(src, dst, *args, **kwargs)
+
+    monkeypatch.setattr(disk_cache_module.os, "replace", counting)
+    return replaced
+
+
+def _manager(cache_dir):
+    with patch('src.cache_manager.CacheManager._get_writable_cache_dir',
+               return_value=str(cache_dir)):
+        manager = CacheManager()
+    manager.stop_cleanup_thread()
+    return manager
+
+
+@pytest.fixture
+def cm(tmp_path):
+    return _manager(tmp_path)
+
+
+DATA = {"events": [{"id": n, "name": "x" * 20} for n in range(50)]}
+
+
+def test_resaving_unchanged_data_does_not_rewrite_the_file(cm, clock, writes):
+    cm.set("scores", DATA)
+    path = cm._get_cache_path("scores")
+    first = os.stat(path)
+    with open(path, "rb") as f:
+        first_bytes = f.read()
+
+    for _ in range(5):
+        clock.now += 60
+        cm.set("scores", DATA)
+
+    assert writes == ["scores.json"]
+    after = os.stat(path)
+    assert after.st_ino == first.st_ino
+    with open(path, "rb") as f:
+        assert f.read() == first_bytes
+    # The file records when its content was last saved.
+    assert after.st_mtime == pytest.approx(clock.now, abs=1e-3)
+
+
+def test_skipped_writes_keep_the_entry_fresh(cm, clock, tmp_path):
+    cm.set("scores", DATA)
+    for _ in range(4):          # re-saved unchanged every 200s
+        clock.now += 200
+        cm.set("scores", DATA)
+    # 800s past the only real write, well beyond max_age=300 of it.
+    assert cm.get("scores", max_age=300) == DATA
+
+    # A reader with no memory tier -- the web interface, or this service
+    # after a restart -- sees the same freshness from disk.
+    other = _manager(tmp_path)
+    record = other.get_cached_data("scores", max_age=300)
+    assert record is not None and record["data"] == DATA
+    assert record["timestamp"] == pytest.approx(clock.now, abs=1e-3)
+    assert DiskCache(str(tmp_path)).get("scores", max_age=300) is not None
+
+    # Freshness is the last save, not forever.
+    clock.now += 301
+    assert DiskCache(str(tmp_path)).get("scores", max_age=300) is None
+    assert _manager(tmp_path).get("scores", max_age=300) is None
+
+
+def test_skipped_writes_keep_a_ttl_entry_fresh(cm, clock, tmp_path):
+    cm.set("odds", DATA, ttl=120)
+    for _ in range(3):
+        clock.now += 100
+        cm.set("odds", DATA, ttl=120)
+    assert _manager(tmp_path).get("odds", max_age=10) == DATA
+
+
+def test_changed_data_writes(cm, clock, writes, tmp_path):
+    cm.set("scores", DATA)
+    clock.now += 60
+    cm.set("scores", {"events": []})
+    assert writes == ["scores.json", "scores.json"]
+    assert _manager(tmp_path).get("scores", max_age=300) == {"events": []}
+
+
+def test_changed_ttl_writes(cm, clock, writes, tmp_path):
+    cm.set("scores", DATA, ttl=60)
+    clock.now += 10
+    cm.set("scores", DATA, ttl=600)
+    clock.now += 10
+    cm.set("scores", DATA)
+    assert writes == ["scores.json"] * 3
+    record = _manager(tmp_path).get_cached_data("scores", max_age=300)
+    assert "ttl" not in record
+
+
+def test_a_record_written_old_stays_old(tmp_path, clock):
+    # Only a skip may move freshness forward: a record saved with an old
+    # timestamp on purpose is not made fresh by the write's own mtime.
+    disk = DiskCache(str(tmp_path))
+    disk.set("k", {"timestamp": clock.now - 600, "data": DATA})
+    assert disk.get("k", max_age=300) is None
+
+
+def test_a_file_replaced_by_another_process_is_rewritten(tmp_path, clock):
+    ours, theirs = DiskCache(str(tmp_path)), DiskCache(str(tmp_path))
+    ours.set("k", {"timestamp": clock.now, "data": "ours"})
+    clock.now += 10
+    theirs.set("k", {"timestamp": clock.now, "data": "theirs"})
+    clock.now += 10
+    ours.set("k", {"timestamp": clock.now, "data": "ours"})
+    assert DiskCache(str(tmp_path)).get("k", max_age=300)["data"] == "ours"
+
+
+def test_a_cleared_file_is_rewritten(cm, clock, tmp_path):
+    cm.set("scores", DATA)
+    os.remove(cm._get_cache_path("scores"))   # e.g. the web UI's delete
+    clock.now += 10
+    cm.set("scores", DATA)
+    assert _manager(tmp_path).get("scores", max_age=300) == DATA

test/test_on_demand_disabled_plugin.py

@@ -0,0 +1,423 @@
+"""On-demand for a plugin that is installed but disabled in config.
+
+The display process only loads enabled plugins, so a request for a disabled
+one -- "Preview on display" offers it on every plugin's config page, with a
+note that the plugin will be enabled for the preview -- failed with
+"invalid-mode". Nothing loaded it short of a restart, and the on-demand
+route no longer restarts the service.
+
+The display now loads such a plugin live for the session (force_enabled, so
+config.json keeps saying disabled) and the main loop unloads it once
+on-demand moves off it: a stop, an expiry, or a request for another plugin.
+
+Also here: a stop sent after a failed request clears the error instead of
+leaving status 'error' published until the state ages out.
+"""
+
+import time
+from unittest.mock import MagicMock
+
+import pytest
+
+from src.plugin_system.plugin_manager import PluginManager
+from src.plugin_system.plugin_state import PluginState
+
+
+def _make_plugin(modes):
+    plugin = MagicMock()
+    plugin.modes = list(modes)
+    return plugin
+
+
+@pytest.fixture
+def controller(test_display_controller):
+    """An idle controller running 'clock', with 'preview-me' installed but disabled."""
+    c = test_display_controller
+    clock = _make_plugin(['clock'])
+    preview = _make_plugin(['preview_a', 'preview_b'])
+    instances = {'clock': clock}
+    catalogue = {'clock': clock, 'preview-me': preview}
+
+    def load_plugin(plugin_id, force_enabled=False):
+        instances[plugin_id] = catalogue[plugin_id]
+        return True
+
+    def unload_plugin(plugin_id):
+        return instances.pop(plugin_id, None) is not None
+
+    pm = c.plugin_manager
+    pm.discovered_plugin_ids.return_value = set(catalogue)
+    pm.discover_plugins.return_value = list(catalogue)
+    pm.plugin_manifests = {}
+    pm.load_plugin = MagicMock(side_effect=load_plugin)
+    pm.unload_plugin = MagicMock(side_effect=unload_plugin)
+    pm.get_plugin.side_effect = instances.get
+
+    config = {'clock': {'enabled': True}, 'preview-me': {'enabled': False}}
+    c.config_service.get_config = lambda: config
+    c.config_manager.save_config = MagicMock()
+    c.cache_manager.set = MagicMock()
+    c.cache_manager.clear_cache = MagicMock()
+
+    c._register_loaded_plugin('clock')
+    c.current_mode_index = 0
+    c.current_display_mode = 'clock'
+    c.test_config = config
+    c.test_instances = instances
+    return c
+
+
+def _start(c, plugin_id='preview-me', mode=None, **extra):
+    request = {'request_id': 'r-' + plugin_id, 'action': 'start',
+               'plugin_id': plugin_id, 'mode': mode or plugin_id}
+    request.update(extra)
+    c._activate_on_demand(request)
+
+
+class TestLoadingForOnDemand:
+    def test_a_disabled_plugin_is_loaded_and_shown(self, controller):
+        _start(controller)
+
+        controller.plugin_manager.load_plugin.assert_called_once_with(
+            'preview-me', force_enabled=True)
+        assert controller.on_demand_active is True
+        assert controller.on_demand_status == 'active'
+        assert controller.on_demand_plugin_id == 'preview-me'
+        assert controller.current_display_mode == 'preview_a'
+        assert controller.plugin_display_modes['preview-me'] == ['preview_a', 'preview_b']
+
+    def test_config_json_is_not_written(self, controller):
+        _start(controller)
+
+        controller.config_manager.save_config.assert_not_called()
+        assert controller.test_config['preview-me'] == {'enabled': False}
+
+    def test_a_requested_mode_is_honoured(self, controller):
+        _start(controller, mode='preview_b')
+        assert controller.current_display_mode == 'preview_b'
+
+    def test_an_enabled_plugin_is_not_reloaded(self, controller):
+        _start(controller, plugin_id='clock')
+
+        controller.plugin_manager.load_plugin.assert_not_called()
+        assert controller.on_demand_active is True
+        assert controller._on_demand_loaded_plugins == set()
+
+    def test_a_plugin_that_is_not_installed_is_not_loaded(self, controller):
+        _start(controller, plugin_id='uninstalled')
+
+        controller.plugin_manager.load_plugin.assert_not_called()
+        assert controller.on_demand_status == 'error'
+        assert controller.on_demand_last_error == 'invalid-mode'
+
+    def test_a_plugin_installed_after_startup_is_found_by_rescanning(self, controller):
+        controller.plugin_manager.discovered_plugin_ids.return_value = {'clock'}
+
+        _start(controller)
+
+        controller.plugin_manager.discover_plugins.assert_called()
+        assert controller.on_demand_active is True
+
+
+class TestLoadFailures:
+    def test_a_failed_load_reports_load_failed(self, controller):
+        controller.plugin_manager.load_plugin = MagicMock(return_value=False)
+
+        _start(controller)
+
+        assert controller.on_demand_active is False
+        assert controller.on_demand_status == 'error'
+        assert controller.on_demand_last_error == 'load-failed'
+        assert 'preview_a' not in controller.available_modes
+        published = controller.cache_manager.set.call_args_list[-1]
+        assert published.args[0] == 'display_on_demand_state'
+        assert published.args[1]['status'] == 'error'
+        assert published.args[1]['error'] == 'load-failed'
+
+    def test_a_load_that_raises_reports_load_failed(self, controller):
+        controller.plugin_manager.load_plugin = MagicMock(side_effect=ImportError('no module'))
+
+        _start(controller)
+
+        assert controller.on_demand_status == 'error'
+        assert controller.on_demand_last_error == 'load-failed'
+
+    def test_a_failed_load_leaves_the_rotation_alone(self, controller):
+        controller.plugin_manager.load_plugin = MagicMock(return_value=False)
+
+        _start(controller)
+        controller._release_on_demand_plugins()
+
+        assert controller.available_modes == ['clock']
+        assert controller.current_display_mode == 'clock'
+        assert controller._on_demand_loaded_plugins == set()
+        controller.plugin_manager.unload_plugin.assert_not_called()
+
+    def test_a_plugin_that_loads_but_has_no_modes_is_unloaded_again(self, controller):
+        """Registered, then the activation fails: the release removes it."""
+        controller._on_demand_modes_for_plugin = MagicMock(return_value=[])
+
+        _start(controller)
+        assert controller.on_demand_last_error == 'no-modes'
+        controller._release_on_demand_plugins()
+
+        controller.plugin_manager.unload_plugin.assert_called_once_with('preview-me')
+        assert controller.available_modes == ['clock']
+
+
+class TestReleasingThePlugin:
+    def test_it_stays_loaded_while_on_demand_shows_it(self, controller):
+        _start(controller)
+        controller._release_on_demand_plugins()
+
+        controller.plugin_manager.unload_plugin.assert_not_called()
+        assert 'preview_a' in controller.plugin_modes
+
+    def test_a_stop_unloads_it_and_resumes_the_rotation(self, controller):
+        _start(controller)
+        controller._clear_on_demand(reason='requested-stop')
+        # Deferred to the main loop: the stop may be read mid-display().
+        controller.plugin_manager.unload_plugin.assert_not_called()
+
+        controller._release_on_demand_plugins()
+
+        controller.plugin_manager.unload_plugin.assert_called_once_with('preview-me')
+        assert controller.available_modes == ['clock']
+        assert 'preview-me' not in controller.plugin_display_modes
+        assert 'preview_a' not in controller.plugin_modes
+        assert controller.current_display_mode == 'clock'
+        assert controller._on_demand_loaded_plugins == set()
+        assert controller.test_config['preview-me'] == {'enabled': False}
+
+    def test_expiry_unloads_it(self, controller):
+        _start(controller, duration=30)
+        controller.on_demand_expires_at = time.time() - 1
+        controller._check_on_demand_expiration()
+        controller._release_on_demand_plugins()
+
+        assert controller.on_demand_last_event == 'expired'
+        controller.plugin_manager.unload_plugin.assert_called_once_with('preview-me')
+
+    def test_a_request_for_another_plugin_unloads_it(self, controller):
+        _start(controller)
+        _start(controller, plugin_id='clock')
+        controller._release_on_demand_plugins()
+
+        controller.plugin_manager.unload_plugin.assert_called_once_with('preview-me')
+        assert controller.on_demand_active is True
+        assert controller.on_demand_plugin_id == 'clock'
+        assert controller.current_display_mode == 'clock'
+
+    def test_a_failed_request_that_ends_the_session_unloads_it(self, controller):
+        _start(controller)
+        _start(controller, plugin_id='uninstalled')
+        controller._release_on_demand_plugins()
+
+        controller.plugin_manager.unload_plugin.assert_called_once_with('preview-me')
+        assert controller.current_display_mode == 'clock'
+        assert controller.force_change is True
+
+    def test_a_plugin_enabled_during_the_session_stays_loaded(self, controller):
+        _start(controller)
+        controller.test_config['preview-me'] = {'enabled': True}
+        controller._clear_on_demand(reason='requested-stop')
+        controller._release_on_demand_plugins()
+
+        controller.plugin_manager.unload_plugin.assert_not_called()
+        assert 'preview_a' in controller.available_modes
+        assert controller._on_demand_loaded_plugins == set()
+
+    def test_the_main_loop_releases_right_after_its_own_poll(self, controller):
+        """A stop read by the main loop unloads before the next screen, not
+        one screen later. That poll runs with no display() on the stack."""
+        import inspect
+        source = inspect.getsource(type(controller).run)
+        poll = source.index('self._check_on_demand_expiration()')
+        release = source.index('self._release_on_demand_plugins()')
+        render = source.index('self._tick_plugin_updates()')
+        assert poll < release < render
+
+    def test_a_reconcile_that_runs_first_unloads_it_the_same_way(self, controller):
+        """A reconcile queued during the session runs at the top of the loop,
+        before the release: it removes the plugin itself (not in the enabled
+        set) and the release is then a no-op."""
+        _start(controller)
+        controller._clear_on_demand(reason='requested-stop')
+
+        controller._reconcile_enabled_plugins()
+        controller._release_on_demand_plugins()
+
+        controller.plugin_manager.unload_plugin.assert_called_once_with('preview-me')
+        assert controller.available_modes == ['clock']
+        assert controller._on_demand_loaded_plugins == set()
+
+    def test_a_config_save_mid_session_keeps_the_instance_enabled(self, controller):
+        """on_config_change would otherwise read enabled: false and switch it off."""
+        controller.config_service.subscribe = MagicMock()
+        _start(controller)
+        callback = controller._plugin_config_callbacks['preview-me']
+        controller.plugin_manager.prepare_plugin_config = None
+
+        callback({}, {'enabled': False, 'color': 'red'})
+
+        plugin = controller.plugin_modes['preview_a']
+        plugin.on_config_change.assert_called_once_with({'enabled': True, 'color': 'red'})
+
+
+class TestRestoredSession:
+    """A restart during a session for a disabled plugin restores it the same way."""
+
+    def test_the_plugin_is_tracked_and_config_is_left_alone(self, test_display_controller):
+        c = test_display_controller
+        c.config.update({'clock': {'enabled': True}, 'disabled-one': {'enabled': False}})
+
+        selected = c._select_startup_plugins(
+            ['clock', 'disabled-one'], {'plugin_id': 'disabled-one', 'mode': 'x'})
+
+        assert 'disabled-one' in selected
+        assert c._on_demand_loaded_plugins == {'disabled-one'}
+        assert c.config['disabled-one']['enabled'] is False
+
+
+class TestResumingAfterTheSession:
+    """Ending a session never resumes the rotation onto the plugin that is
+    about to be unloaded."""
+
+    def _restored_session(self, c, other_modes=('clock',)):
+        """As after a restart: no saved resume index, and the plugin's modes
+        ordered in ahead of the rest (load order is not deterministic)."""
+        c._on_demand_loaded_plugins.add('preview-me')
+        c.plugin_manager.load_plugin('preview-me', force_enabled=True)
+        c._register_loaded_plugin('preview-me')
+        c.available_modes = ['preview_a', 'preview_b'] + list(other_modes)
+        c.on_demand_active = True
+        c.on_demand_status = 'active'
+        c.on_demand_plugin_id = 'preview-me'
+        c.on_demand_modes = ['preview_a', 'preview_b']
+        c.rotation_resume_index = None
+        c.current_mode_index = 0
+        c.current_display_mode = 'preview_a'
+
+    def test_a_restored_session_resumes_on_an_enabled_mode(self, controller):
+        self._restored_session(controller)
+
+        controller._clear_on_demand(reason='requested-stop')
+
+        assert controller.current_display_mode == 'clock'
+        controller._release_on_demand_plugins()
+        assert controller.available_modes == ['clock']
+        assert controller.current_display_mode == 'clock'
+
+    def test_with_nothing_else_enabled_the_display_goes_idle(self, controller):
+        controller._unregister_plugin('clock')
+        self._restored_session(controller, other_modes=())
+
+        controller._clear_on_demand(reason='requested-stop')
+        assert controller.current_display_mode is None
+
+        controller._release_on_demand_plugins()
+        assert controller.available_modes == []
+        assert controller.current_display_mode is None
+
+    def test_a_saved_resume_index_is_still_used(self, controller):
+        c = controller
+        c.available_modes = ['clock', 'other']
+        c.plugin_modes['other'] = MagicMock()
+        c.current_mode_index = 1
+        c.current_display_mode = 'other'
+
+        _start(c)
+        c._clear_on_demand(reason='requested-stop')
+
+        assert c.current_display_mode == 'other'
+
+
+class TestStopClearsAnError:
+    def _post_stop(self, c):
+        stop = {'request_id': 'S1', 'action': 'stop'}
+        c._last_on_demand_poll = None
+        c.cache_manager.get = MagicMock(
+            side_effect=lambda key, *a, **kw:
+                stop if key == 'display_on_demand_request' else None)
+        c.cache_manager.delete = MagicMock()
+        c._poll_on_demand_requests()
+
+    def test_a_stop_after_a_failed_request_clears_the_error(self, controller):
+        _start(controller, plugin_id='uninstalled')
+        assert controller.on_demand_status == 'error'
+
+        self._post_stop(controller)
+
+        assert controller.on_demand_status == 'idle'
+        assert controller.on_demand_last_error is None
+        state = controller.cache_manager.set.call_args_list[-1].args[1]
+        assert state['status'] == 'idle'
+        assert state['error'] is None
+
+    def test_clearing_the_error_leaves_the_rotation_alone(self, controller):
+        _start(controller, plugin_id='uninstalled')
+        controller.force_change = False
+
+        self._post_stop(controller)
+
+        assert controller.current_display_mode == 'clock'
+        assert controller.force_change is False
+
+    def test_a_stop_while_idle_is_still_just_acknowledged(self, controller):
+        controller._clear_on_demand = MagicMock()
+
+        self._post_stop(controller)
+
+        assert controller.on_demand_status == 'idle'
+        assert controller.on_demand_request_id == 'S1'
+        controller._clear_on_demand.assert_not_called()
+
+
+class TestForceEnabledLoad:
+    """PluginManager.load_plugin(force_enabled=True) runs the plugin enabled
+    without touching the config it read."""
+
+    class _Plugin:
+        def __init__(self, config):
+            self.config = config
+            self.enabled_calls = 0
+
+        def on_enable(self):
+            self.enabled_calls += 1
+
+    @pytest.fixture
+    def pm(self, tmp_path):
+        plugins_dir = tmp_path / 'plugins'
+        (plugins_dir / 'demo').mkdir(parents=True)
+        manager = PluginManager(plugins_dir=str(plugins_dir))
+        manager.plugin_manifests['demo'] = {'id': 'demo', 'name': 'Demo'}
+        manager.schema_manager = MagicMock()
+        manager.schema_manager.get_schema_path.return_value = None
+        # Hand the section back as-is, as the fallback path can: the copy in
+        # load_plugin is what keeps the cached config clean.
+        manager.schema_manager.prepare_plugin_config.side_effect = (
+            lambda pid, cfg, schema=None, changed_paths=None: cfg)
+        manager.plugin_loader = MagicMock()
+        manager.plugin_loader.find_plugin_directory.return_value = plugins_dir / 'demo'
+        manager.plugin_loader.load_plugin.side_effect = (
+            lambda **kw: (self._Plugin(kw['config']), None))
+        manager.config_manager = MagicMock()
+        manager.cached_config = {'demo': {'enabled': False, 'color': 'red'}}
+        manager.config_manager.load_config.return_value = manager.cached_config
+        return manager
+
+    def test_a_disabled_plugin_loads_disabled_by_default(self, pm):
+        assert pm.load_plugin('demo') is True
+        assert pm.plugins['demo'].enabled_calls == 0
+        assert pm.state_manager.get_state('demo') == PluginState.DISABLED
+
+    def test_force_enabled_runs_it_enabled(self, pm):
+        assert pm.load_plugin('demo', force_enabled=True) is True
+        plugin = pm.plugins['demo']
+        assert plugin.config == {'enabled': True, 'color': 'red'}
+        assert plugin.enabled_calls == 1
+        assert pm.state_manager.get_state('demo') == PluginState.ENABLED
+
+    def test_force_enabled_does_not_touch_the_cached_config(self, pm):
+        pm.load_plugin('demo', force_enabled=True)
+        assert pm.cached_config['demo'] == {'enabled': False, 'color': 'red'}

test/test_on_demand_pinning_and_restart.py

@@ -164,12 +164,14 @@ class TestRestartDoesNotStarveTheOtherPlugins:
         assert controller.on_demand_mode == 'app_a'
         assert controller.on_demand_pinned is True
 
-    def test_a_disabled_on_demand_plugin_is_enabled_and_loaded(self, controller):
+    def test_a_disabled_on_demand_plugin_is_still_loaded(self, controller):
-        """Otherwise the mode being resumed has nothing behind it."""
+        """Otherwise the mode being resumed has nothing behind it. It loads
+        for on-demand only; its config section is left disabled."""
         selected = controller._select_startup_plugins(
             self.DISCOVERED, {'plugin_id': 'disabled-one', 'mode': 'x'})
         assert 'disabled-one' in selected
-        assert controller.config['disabled-one']['enabled'] is True
+        assert controller._on_demand_loaded_plugins == {'disabled-one'}
+        assert controller.config['disabled-one']['enabled'] is False
 
     def test_an_unknown_on_demand_plugin_falls_back_to_normal(self, controller):
         selected = controller._select_startup_plugins(

test/test_web_origin_guard.py

@@ -1,285 +0,0 @@
-"""State-changing requests from another website's page are refused.
-
-The interface has no login and was defended only by "it is on the LAN". But
-any site a LAN user opens can make their browser POST to http://<pi>:5000: a
-plain HTML form is a CORS "simple" request, so it arrives and runs even though
-the attacking page never sees the answer. /api/v3/system/action accepted
-form-encoded bodies and reboots, powers off and pulls code.
-
-web_interface/origin_guard.py refuses POST/PUT/PATCH/DELETE whose Origin (or
-Referer) is not this server's own host, and /system/action only takes a
-form-encoded body from HTMX (a cross-site form cannot set HX-Request).
-Requests with neither Origin nor Referer are not from a browser -- curl, Home
-Assistant, the MQTT bridge -- and still pass.
-"""
-
-import subprocess
-from unittest.mock import patch
-
-import pytest
-from flask import Flask, jsonify
-
-from test._api_v3_test_helpers import (  # noqa: F401 - fixture
-    api_v3_module, build_app,
-)
-from web_interface import origin_guard
-
-# Flask's test client addresses requests to Host: localhost.
-SELF = 'http://localhost'
-EVIL = 'http://evil.example'
-
-
-# --- The guard itself, on a throwaway app ---------------------------------
-
-@pytest.fixture
-def probe():
-    app = Flask(__name__)
-    app.config['TESTING'] = True
-    origin_guard.init_app(app)
-
-    @app.route('/change', methods=['GET', 'POST', 'PUT', 'PATCH', 'DELETE'])
-    def change():
-        return jsonify({'status': 'success'})
-
-    return app.test_client()
-
-
-@pytest.mark.parametrize('method', ['post', 'put', 'patch', 'delete'])
-def test_a_cross_site_origin_is_refused_for_every_changing_method(probe, method):
-    resp = getattr(probe, method)('/change', headers={'Origin': EVIL})
-    assert resp.status_code == 403
-    body = resp.get_json()
-    assert body['status'] == 'error'
-    assert body['error_code'] == 'CROSS_SITE_REQUEST'
-    assert body['details'].startswith('Origin ')
-    # The attacker-chosen origin is logged, never echoed back in the body.
-    assert 'evil.example' not in resp.get_data(as_text=True)
-
-
-@pytest.mark.parametrize('origin', [
-    SELF,
-    'http://LOCALHOST',          # host case is not significant
-    'http://localhost:80',       # explicit default port
-    'https://localhost:80',      # TLS proxy passing Host through: scheme ignored
-])
-def test_the_interfaces_own_origin_passes(probe, origin):
-    assert probe.post('/change', headers={'Origin': origin}).status_code == 200
-
-
-def test_the_host_the_browser_used_is_what_counts(probe):
-    # Whatever name or address the user typed -- mDNS name, LAN IP, or the
-    # access-point address the captive portal answers on.
-    for host in ('ledpi.local:5000', '192.168.1.40:5000', '192.168.4.1',
-                 '[fe80::1]:5000'):
-        resp = probe.post('/change', headers={
-            'Host': host, 'Origin': f'http://{host}'})
-        assert resp.status_code == 200, host
-
-
-def test_captive_portal_via_port_80_redirect_passes(probe):
-    # iptables REDIRECT 80 -> 5000 keeps the Host the browser sent, which
-    # carries no port; the page's Origin carries none either.
-    resp = probe.post('/change', headers={
-        'Host': '192.168.4.1', 'Origin': 'http://192.168.4.1'})
-    assert resp.status_code == 200
-
-
-def test_the_same_host_on_another_port_is_another_site(probe):
-    resp = probe.post('/change', headers={
-        'Host': 'ledpi.local:5000', 'Origin': 'http://ledpi.local:8080'})
-    assert resp.status_code == 403
-
-
-def test_an_https_page_behind_a_tls_terminating_proxy_passes(probe):
-    # nginx terminates TLS and forwards a portless Host to the plain-http
-    # upstream: the browser's Origin is https (443), Flask sees http (80).
-    resp = probe.post('/change', headers={
-        'Host': 'pi.example', 'Origin': 'https://pi.example'})
-    assert resp.status_code == 200
-    resp = probe.post('/change', headers={
-        'Host': 'pi.example', 'Referer': 'https://pi.example/v3'})
-    assert resp.status_code == 200
-
-
-def test_a_portless_host_still_refuses_a_nondefault_port(probe):
-    # Only the standard port of either scheme counts as "no port".
-    for origin in ('https://pi.example:8443', 'http://pi.example:5000',
-                   'http://pi.example:443', 'https://evil.example'):
-        resp = probe.post('/change', headers={
-            'Host': 'pi.example', 'Origin': origin})
-        assert resp.status_code == 403, origin
-
-
-def test_an_explicit_host_port_must_match_exactly(probe):
-    # A Host with a port (the proxy forwards $http_host) is compared as is.
-    assert probe.post('/change', headers={
-        'Host': 'pi.example:8443',
-        'Origin': 'https://pi.example:8443'}).status_code == 200
-    assert probe.post('/change', headers={
-        'Host': 'pi.example:8443',
-        'Origin': 'https://pi.example'}).status_code == 403
-
-
-def test_a_refusal_logs_only_the_site_never_the_referer_path(probe, caplog):
-    # A Referer's path and query can carry tokens.
-    with caplog.at_level('WARNING', logger='web_interface.origin_guard'):
-        resp = probe.post('/change', headers={
-            'Referer': EVIL + '/page?token=s3cret#frag'})
-    assert resp.status_code == 403
-    logged = caplog.text
-    assert 'evil.example' in logged
-    assert 's3cret' not in logged
-    assert '/page' not in logged
-
-
-def test_a_refusal_log_cannot_be_forged_with_newlines(probe, caplog):
-    with caplog.at_level('WARNING', logger='web_interface.origin_guard'):
-        probe.post('/change%0D%0AFAKE', headers={'Origin': EVIL})
-    assert len(caplog.records) == 1
-    message = caplog.records[0].getMessage()
-    assert '\n' not in message and '\r' not in message
-    assert 'FAKE' in message  # the path was logged, escaped
-
-
-def test_no_origin_and_no_referer_passes(probe):
-    # curl, Home Assistant, the MQTT bridge: not a browser.
-    assert probe.post('/change').status_code == 200
-    assert probe.post('/change', json={'action': 'x'}).status_code == 200
-
-
-def test_a_null_origin_is_refused(probe):
-    # Sandboxed iframes and file:// pages send "Origin: null".
-    resp = probe.post('/change', headers={'Origin': 'null'})
-    assert resp.status_code == 403
-    assert 'null' in resp.get_json()['details']
-
-
-def test_the_referer_is_checked_when_origin_is_absent(probe):
-    assert probe.post('/change', headers={
-        'Referer': EVIL + '/attack.html'}).status_code == 403
-    assert probe.post('/change', headers={
-        'Referer': SELF + '/v3'}).status_code == 200
-
-
-def test_origin_wins_over_referer(probe):
-    resp = probe.post('/change', headers={
-        'Origin': EVIL, 'Referer': SELF + '/'})
-    assert resp.status_code == 403
-
-
-@pytest.mark.parametrize('value', [
-    'not a url', 'ftp://localhost', 'http://', 'http://localhost:notaport',
-])
-def test_an_unreadable_origin_is_refused(probe, value):
-    assert probe.post('/change', headers={'Origin': value}).status_code == 403
-
-
-def test_gets_are_never_checked(probe):
-    assert probe.get('/change', headers={'Origin': EVIL}).status_code == 200
-    assert probe.get('/change', headers={'Origin': 'null'}).status_code == 200
-
-
-# --- /api/v3/system/action -------------------------------------------------
-
-@pytest.fixture
-def api_client(api_v3_module):  # noqa: F811 - pytest fixture injection
-    app = build_app(api_v3_module.api_v3)
-    origin_guard.init_app(app)
-    return app.test_client()
-
-
-def _ok(args, **kwargs):
-    return subprocess.CompletedProcess(args, 0, stdout='', stderr='')
-
-
-def test_a_cross_site_form_post_never_reaches_the_reboot(api_client):
-    with patch('subprocess.run', side_effect=_ok) as run:
-        resp = api_client.post('/api/v3/system/action',
-                               data={'action': 'reboot_system'},
-                               headers={'Origin': EVIL})
-    assert resp.status_code == 403
-    run.assert_not_called()
-
-
-def test_a_form_post_without_hx_request_is_refused_even_without_origin(api_client):
-    # Belt and braces: a browser whose Origin/Referer never arrived (a
-    # privacy proxy stripping both) still cannot send the form.
-    with patch('subprocess.run', side_effect=_ok) as run:
-        resp = api_client.post('/api/v3/system/action',
-                               data={'action': 'reboot_system'})
-    assert resp.status_code == 415
-    assert 'JSON' in resp.get_json()['message']
-    run.assert_not_called()
-
-
-def test_a_text_plain_body_is_refused(api_client):
-    # enctype="text/plain" is the other cross-site form encoding.
-    with patch('subprocess.run', side_effect=_ok) as run:
-        resp = api_client.post('/api/v3/system/action',
-                               data='{"action": "reboot_system"}',
-                               content_type='text/plain')
-    assert resp.status_code == 415
-    run.assert_not_called()
-
-
-def test_an_htmx_form_post_from_the_interface_runs(api_client):
-    with patch('subprocess.run', side_effect=_ok) as run:
-        resp = api_client.post('/api/v3/system/action',
-                               data={'action': 'stop_display'},
-                               headers={'Origin': SELF, 'HX-Request': 'true'})
-    assert resp.status_code == 200
-    assert resp.get_json()['status'] == 'success'
-    assert run.call_args[0][0] == ['sudo', 'systemctl', 'stop', 'ledmatrix.service']
-
-
-def test_a_same_origin_json_post_runs(api_client):
-    # What every button and fetch() in the interface sends.
-    with patch('subprocess.run', side_effect=_ok):
-        resp = api_client.post('/api/v3/system/action',
-                               json={'action': 'stop_display'},
-                               headers={'Origin': SELF})
-    assert resp.status_code == 200
-    assert resp.get_json()['status'] == 'success'
-
-
-def test_a_json_post_with_no_origin_runs(api_client):
-    # The MQTT bridge, Home Assistant, curl.
-    with patch('subprocess.run', side_effect=_ok):
-        resp = api_client.post('/api/v3/system/action',
-                               json={'action': 'stop_display'})
-    assert resp.status_code == 200
-
-
-def test_an_empty_json_body_still_asks_for_an_action(api_client):
-    resp = api_client.post('/api/v3/system/action', json={})
-    assert resp.status_code == 400
-    assert resp.get_json()['message'] == 'Action required'
-
-
-def test_a_json_body_that_is_not_an_object_asks_for_an_action(api_client):
-    resp = api_client.post('/api/v3/system/action', json=['reboot_system'])
-    assert resp.status_code == 400
-
-
-# --- The real app ----------------------------------------------------------
-
-def test_the_real_app_has_the_guard():
-    import web_interface.app as web_app
-    web_app.app.config['TESTING'] = True
-    with patch('subprocess.run', side_effect=_ok) as run, \
-            web_app.app.test_client() as c:
-        resp = c.post('/api/v3/system/action',
-                      json={'action': 'reboot_system'},
-                      headers={'Origin': EVIL})
-    assert resp.status_code == 403
-    assert resp.get_json()['error_code'] == 'CROSS_SITE_REQUEST'
-    run.assert_not_called()
-
-
-def test_the_real_app_leaves_gets_alone():
-    import web_interface.app as web_app
-    web_app.app.config['TESTING'] = True
-    with web_app.app.test_client() as c:
-        resp = c.get('/api/v3/no-such-endpoint-for-origin-test',
-                     headers={'Origin': EVIL})
-    assert resp.status_code == 404

test/web_interface/test_startup_reconciliation_disarmed.py

@@ -0,0 +1,23 @@
+"""
+No test may start the real app's startup reconciliation.
+
+web_interface/app.py reads the checkout's real config/config.json and
+plugin-repos/ at import, and its first request launches a reconciliation
+thread that reinstalls every configured-but-missing plugin from the live
+store. A full suite run on a dev checkout used to leave whole plugins
+untracked in plugin-repos/ that way. test/conftest.py disarms the run-once
+latch on every import of the module; this pins that.
+"""
+
+from unittest.mock import MagicMock, patch
+
+
+def test_a_request_to_the_imported_app_launches_no_reconciliation():
+    import web_interface.app as web_app
+
+    assert web_app._reconciliation_started is True
+
+    with patch.object(web_app, "threading", MagicMock()) as threading_mock:
+        web_app.app.test_client().get("/favicon.ico")
+
+    threading_mock.Thread.assert_not_called()

web_interface/app.py

@@ -55,17 +55,10 @@ app = Flask(__name__)
 app.secret_key = os.urandom(24)
 config_manager = ConfigManager()
 
-# Cross-site request forgery: the UI has no login, and being "only on the LAN"
+# No CSRF protection: the UI is meant for the local network, where anyone who
-# does not keep other websites out. Any page a LAN user opens can make their
+# can forge a request can also send it directly, and neither the HTMX forms
-# browser POST to this server -- a plain HTML form is not blocked by CORS -- so
+# nor the fetch() calls carry a token. Exposing the UI beyond the LAN needs
-# a hostile site could reboot the Pi, pull code or rewrite the config through
+# CSRF tokens added to both first.
-# the user's browser. web_interface/origin_guard.py (registered below) refuses
-# POST/PUT/PATCH/DELETE whose Origin (or, failing that, Referer) is not this
-# server's own host; requests with neither header (curl, Home Assistant, the
-# MQTT bridge) are not from a browser and pass. There are no CSRF tokens:
-# neither the HTMX forms nor the fetch() calls carry one. Anyone who can reach
-# the port directly can still use the API, so exposing the UI beyond a trusted
-# network still needs real authentication.
 
 # Initialize rate limiting (prevent accidental abuse, not security)
 try:
@@ -409,11 +402,6 @@ def success_txt():
 from web_interface import request_logging
 request_logging.init_app(app)
 
-# Refuse state-changing requests sent by another website's page (see the
-# cross-site note near the top of this file).
-from web_interface import origin_guard
-origin_guard.init_app(app)
-
 # Global error handlers
 @app.errorhandler(404)
 def not_found_error(error):

web_interface/blueprints/api_v3/display.py

@@ -180,9 +180,9 @@ def start_on_demand_display():
             if not resolved_plugin:
                 return jsonify({'status': 'error', 'message': f'Mode {resolved_mode} not found'}), 404
 
-    # Note: On-demand can work with disabled plugins - the display controller
+    # On-demand works with disabled plugins: the running display loads one
-    # will temporarily enable them during initialization if needed
+    # for the session and unloads it afterwards, leaving config.json alone
-    # We don't block the request here, but log it for debugging
+    # (DisplayController._load_plugin_for_on_demand). Logged for debugging.
     if api_v3.config_manager and resolved_plugin:
         config = api_v3.config_manager.load_config()
         plugin_config = config.get(resolved_plugin, {})

web_interface/blueprints/api_v3/system.py

@@ -383,27 +383,16 @@ def _perform_core_update_locked(stash_local_changes=True):
 def execute_system_action():
     """Execute system actions (start/stop/reboot/etc)"""
     try:
-        data = request.get_json(silent=True)
+        # HTMX sends data as form data, not JSON
-        if data is None and not request.is_json:
+        data = request.get_json(silent=True) or {}
-            # Every caller in the interface sends JSON (the Quick Actions
+        if not data:
-            # buttons use HTMX's json-enc). A form-encoded body is what a
+            # Try to get from form data if JSON fails
-            # cross-site HTML form can send without a CORS preflight, and
-            # this route reboots, powers off and pulls code, so it is only
-            # accepted from HTMX: a cross-site form cannot set HX-Request.
-            # This backs up the app-wide Origin check (origin_guard.py).
-            if not request.headers.get('HX-Request'):
-                return jsonify({
-                    'status': 'error',
-                    'message': ('Send the action as JSON '
-                                '(Content-Type: application/json), '
-                                'e.g. {"action": "restart_display_service"}'),
-                }), 415
             data = {
                 'action': request.form.get('action'),
                 'mode': request.form.get('mode')
             }
 
-        if not isinstance(data, dict) or not data.get('action'):
+        if not data or 'action' not in data:
             return jsonify({'status': 'error', 'message': 'Action required'}), 400
 
         action = data['action']

web_interface/origin_guard.py

@@ -1,191 +0,0 @@
-"""
-Cross-site request guard for the web interface.
-
-The threat: the interface has no login, and "it is only on the LAN" does not
-keep other websites out of it. Any page a person on the LAN opens in their
-browser can make that browser send a request to ``http://<pi>:5000``. A plain
-HTML form POST (``application/x-www-form-urlencoded``, ``multipart/form-data``
-or ``text/plain``) is a "simple" request: CORS does not preflight it and does
-not stop it from arriving, it only hides the response from the page. So a
-hostile or compromised site could reboot the Pi, pull code, install or remove
-plugins or rewrite the config, without the user ever seeing the interface.
-
-The defence here needs no tokens and no frontend change. Browsers attach an
-``Origin`` header to every cross-site POST (and to same-origin ones in all
-current browsers), and it cannot be set or removed by page script. So for any
-state-changing method:
-
-* ``Origin`` present -> it must name this server's own host, else 403.
-  ``Origin: null`` (a sandboxed iframe, a ``file://`` page, some cross-site
-  redirect chains) is never this server, so it is refused too.
-* ``Origin`` absent, ``Referer`` present -> the same check on the Referer.
-* neither -> allowed. That is curl, Home Assistant, the MQTT bridge and every
-  other script: not a browser, so not a confused deputy. A browser making a
-  cross-site request always sends ``Origin``.
-
-"This server's own host" is the ``Host`` header the request arrived with, so
-it follows whatever name or address the user typed: ``ledpi.local:5000``,
-``192.168.1.40:5000``, or ``192.168.4.1`` in access-point mode (the captive
-portal's port 80 -> 5000 redirect keeps the Host the browser sent, and the
-setup page's fetches go back to that same host).
-
-The scheme is deliberately not compared, only host and port. The claimed
-value's default port comes from its own scheme. A ``Host`` without a port
-means "the default port of whatever scheme the browser used", and that scheme
-is not always the one Flask sees: a TLS-terminating reverse proxy makes the
-browser say ``https://pi.example`` (443) while Flask sees ``http`` (80). So a
-portless ``Host`` accepts either default. An attacker cannot use that gap,
-because to match they would need to serve a page from this same host on its
-standard port. The app does not use ``ProxyFix`` and so does not trust
-``X-Forwarded-Host`` or ``X-Forwarded-Proto``: a proxy that rewrites ``Host``
-to the upstream address (nginx's default ``proxy_pass`` does) must be
-configured to pass the original one, port included
-(``proxy_set_header Host $http_host;`` -- nginx's ``$host`` drops the port).
-
-Not covered: DNS rebinding (an attacker's hostname re-pointed at the Pi is
-"same origin" to the browser), and anyone who can reach the port directly.
-Neither is new; the interface is still meant for a trusted network.
-"""
-import logging
-from urllib.parse import urlsplit, urlunsplit
-
-from flask import Flask, jsonify, request
-
-logger = logging.getLogger('web_interface.origin_guard')
-
-#: Methods that change state and so must come from this interface's own pages.
-STATE_CHANGING_METHODS = frozenset({'POST', 'PUT', 'PATCH', 'DELETE'})
-
-_DEFAULT_PORTS = {'http': 80, 'https': 443}
-
-
-def _authority(netloc: str):
-    """``(hostname, port)`` for an authority; port is None when it has none.
-
-    Lower-cases the host and drops a trailing dot, so ``Pi.local.`` and
-    ``pi.local`` compare equal. None if the authority is unreadable.
-    """
-    try:
-        parts = urlsplit(f'//{netloc}')
-        hostname = parts.hostname
-        port = parts.port
-    except ValueError:
-        # A malformed port or bracketed address.
-        return None
-    if not hostname:
-        return None
-    return hostname.lower().rstrip('.'), port
-
-
-def _url_host_port(url: str):
-    """``(hostname, port, default_port)`` for an Origin or Referer, or None.
-
-    ``port`` is the explicit port or, failing that, the URL scheme's default,
-    which is also returned as ``default_port``.
-    """
-    try:
-        parts = urlsplit(url.strip())
-    except ValueError:
-        return None
-    scheme = parts.scheme.lower()
-    if scheme not in _DEFAULT_PORTS or not parts.netloc:
-        return None
-    authority = _authority(parts.netloc.rsplit('@', 1)[-1])
-    if authority is None:
-        return None
-    hostname, port = authority
-    default_port = _DEFAULT_PORTS[scheme]
-    return hostname, default_port if port is None else port, default_port
-
-
-def _names_this_server(claimed) -> bool:
-    """Whether a claimed ``(hostname, port, default_port)`` is this request's
-    own ``Host``."""
-    own = _authority(request.host)
-    if own is None:
-        return False
-    hostname, port = own
-    claimed_host, claimed_port, claimed_default = claimed
-    if claimed_host != hostname:
-        return False
-    if port is not None:
-        return claimed_port == port
-    # A portless Host is the default port of the scheme the browser used.
-    # Behind a TLS-terminating proxy that is https/443 while Flask sees
-    # http/80, so accept the default of either scheme.
-    return claimed_port in (claimed_default,
-                            _DEFAULT_PORTS.get(request.scheme))
-
-
-def _loggable(value: str) -> str:
-    """Just the ``scheme://host[:port]`` of an Origin/Referer, for the log.
-
-    A Referer's path and query can carry tokens or other private data, and
-    only the site matters when reading a refusal.
-    """
-    try:
-        parts = urlsplit(value.strip())
-        netloc = parts.netloc.rsplit('@', 1)[-1]
-    except ValueError:
-        return '<unreadable>'
-    if not parts.scheme or not netloc:
-        return '<unreadable>'
-    return urlunsplit((parts.scheme, netloc, '', '', ''))
-
-
-def check_request_origin():
-    """None when the request may proceed, else the reason it may not.
-
-    The reason is a short phrase for the log and the error message.
-    """
-    if request.method not in STATE_CHANGING_METHODS:
-        return None
-
-    origin = request.headers.get('Origin')
-    if origin is not None:
-        header, value = 'Origin', origin
-    else:
-        referer = request.headers.get('Referer')
-        if referer is None:
-            # Not a browser (curl, Home Assistant, the MQTT bridge, scripts).
-            return None
-        header, value = 'Referer', referer
-
-    if value.strip().lower() == 'null':
-        return f'{header} is "null" (sandboxed or file:// page)'
-
-    claimed = _url_host_port(value)
-    if claimed is None:
-        return f'{header} header is not a valid http(s) URL'
-    if not _names_this_server(claimed):
-        # The claimed value is attacker-chosen: the hook logs it, but the
-        # reason (echoed in the 403 body) never repeats it.
-        return header + ' names a different host than this interface'
-    return None
-
-
-def init_app(app: Flask) -> None:
-    """Refuse state-changing requests that another website's page sent."""
-
-    @app.before_request
-    def _refuse_cross_site_requests():
-        reason = check_request_origin()
-        if reason is None:
-            return None
-        # Only the site each header names, never a Referer's path or query
-        # (which can carry tokens); %r keeps CR/LF from forging log lines.
-        origin = request.headers.get('Origin')
-        referer = request.headers.get('Referer')
-        logger.warning("Refused cross-site %s %r: %s (Origin=%r, Referer=%r)",
-                       request.method, request.path, reason,
-                       None if origin is None else _loggable(origin),
-                       None if referer is None else _loggable(referer))
-        return jsonify({
-            'status': 'error',
-            'error_code': 'CROSS_SITE_REQUEST',
-            'message': ('Refused: this request came from another website, not '
-                        'from the LEDMatrix interface. Open the interface '
-                        'directly (the address in your browser bar must be the '
-                        'same one the request goes to) and try again.'),
-            'details': reason,
-        }), 403