refactor(web): build the logged origin with urlunsplit, not an f-string

Semgrep's directly-returned-format-string rule read the helper as a Flask route returning a formatted string. Same output.

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

CHANGELOG.md

@@ -19,6 +19,42 @@ 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
+  /display/on-demand/start` treated `start_service` (on by default, and what
+  "Preview on display", the on-demand dialog and the MQTT bridge all send) as
+  "restart": it stopped the service, waited 1.5s and started it again, so
+  every request reloaded every plugin and left the panel blank for seconds.
+  The running display already reads the request within a quarter of a second,
+  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.
+
 ## 3.7.0
 
 Sports consolidation stage 3 (#672). No behaviour change: nothing in core

SECURITY.md

@@ -63,6 +63,14 @@ 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

@@ -52,8 +52,10 @@ each other. They share three things:
 | Preview viewer marker | `/tmp/led_matrix_preview_viewer` | web, while a preview is open | display: writes full-rate snapshots only while it is fresh |
 | Hardware init status | `/tmp/led_matrix_hw_status.json` | display | web: `/api/v3/hardware/status` |
 
-The on-demand start route also restarts `ledmatrix.service` by default so the
+The on-demand start route starts `ledmatrix.service` when it is not running
-request takes effect straight away.
+(`start_service`, on by default) but never restarts a running one: the display
+reads the mailbox every `ON_DEMAND_POLL_INTERVAL` (0.25s), from its dwell
+sleep, its render loops and Vegas's interrupt check as well as the main loop.
 
 ## Display loop
 

docs/REST_API_REFERENCE.md

@@ -18,6 +18,17 @@ 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)
@@ -390,7 +401,7 @@ Request a specific plugin to display on-demand.
 - `mode` (string, optional): Display mode name (plugin_id inferred if not provided)
 - `duration` (number, optional): Duration in seconds (0 = until stopped)
 - `pinned` (boolean, optional): Pin display (pause rotation)
-- `start_service` (boolean, optional): (Re)start the display service so it picks the request up (default: true)
+- `start_service` (boolean, optional): Start the display service if it is not running (default: true). A running service is never restarted: it picks the request up within about a quarter of a second. When false and the service is stopped, the route returns 400.
 
 **Response**:
 ```json
@@ -1388,7 +1399,10 @@ Fetches `origin` and lists branches to switch to: `current`, `upstream`,
 
 **POST** `/api/v3/system/action`
 
-Execute system-level actions. JSON or form data.
+Execute system-level actions. Send JSON (`Content-Type: application/json`).
+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,6 +412,16 @@ 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

test/test_api_v3_on_demand_restart.py

@@ -1,25 +1,27 @@
-"""Regression test: POST /display/on-demand/start restarting a running
+"""POST /display/on-demand/start and /stop must not restart a running display.
-service must not import a name that does not exist.
 
-display.py has `import web_interface.blueprints.api_v3 as _pkg` and reads
+The start route used to treat ``start_service`` (default True, and what both
-mutable, test-patched attributes back through it (`_pkg.time.time()`,
+the web UI and the MQTT bridge send) as "restart": with the service running it
-`_pkg._get_starlark_plugin()`, ...) rather than binding them by value, per
+ran ``systemctl stop``, slept 1.5s and started it again. Every on-demand or
-the package's own docstring. One spot went further and wrote a genuine
+"Preview on display" click therefore cold-restarted the display process --
-`import` *statement* against that alias --
+every plugin reloaded, the panel blank for seconds -- to deliver a request the
+running process polls for every ON_DEMAND_POLL_INTERVAL anyway (see
+test_on_demand_mailbox.py and test_display_pending_changes.py for the display
+side: the mailbox is read mid-dwell, mid-screen and mid-Vegas-iteration).
 
-    import _pkg.time as time_module
+The restart did not buy anything either: a freshly started display restores
+only the on-demand session it saved itself (``display_on_demand_config``), so
+the new request reached it through the same mailbox, one cold start later.
 
--- but `_pkg` is a local name bound by `import ... as _pkg` in this module,
+This file previously pinned that restart path (it guarded a broken
-not a real top-level package, so `import _pkg.time` is not something Python
+``import _pkg.time`` inside it). The path is gone; these tests pin its
-can resolve; it raises ModuleNotFoundError. That line only runs when the
+replacement: a running service is left alone, a stopped one is started (only
-display service is already running and the caller also asked to (re)start
+when start_service is set), and the request lands in the mailbox either way.
-it, so this endpoint failed on exactly the restart path -- the one where a
-cache write recording the new on-demand request had already happened.
 
-The route wraps its body in `except Exception`, so the failure reached the
+The service helpers are patched where they run. display.py binds
-caller as a handled 500 with a generic message, not an unhandled crash --
+_get_display_service_status by value, while _ensure_display_service_running
-but a 500 all the same on a request that should have restarted the service
+(in the package __init__) looks it up in its own module, so both are patched;
-and reported success.
+_run_systemctl_command is the one place a systemctl command is issued.
 """
 
 import sys
@@ -32,60 +34,137 @@ sys.path.insert(0, str(Path(__file__).parent.parent))
 
 from test._api_v3_test_helpers import api_v3_client, api_v3_module  # noqa: F401,E402
 
-URL = "/api/v3/display/on-demand/start"
+START_URL = "/api/v3/display/on-demand/start"
+STOP_URL = "/api/v3/display/on-demand/stop"
+MAILBOX = "display_on_demand_request"
 
 
 @pytest.fixture
-def restart_path(api_v3_module):
+def service(api_v3_module):
-    """Force the `service_was_running and start_service` branch.
+    """A display service whose state the test sets; records systemctl calls.
 
-    plugin_manager and config_manager are set to None so the route takes
+    plugin_manager and config_manager are None so the route skips plugin
-    the simplest path to that branch rather than tripping over unrelated
+    resolution (not what is under test here). The cache is the blueprint's
-    MagicMock plumbing. The cache is the blueprint's cache_manager, which
+    MagicMock cache_manager, so mailbox writes are visible as set() calls.
-    api_v3_module already set to a MagicMock. _get_display_service_status,
-    _stop_display_service and _ensure_display_service_running are bound by
-    value in display.py (see its own docstring), so they are patched on
-    that submodule rather than on the package.
     """
     api_v3_module.api_v3.plugin_manager = None
     api_v3_module.api_v3.config_manager = None
+    state = {"active": True}
 
-    with patch("web_interface.blueprints.api_v3.display._get_display_service_status") as get_status, \
+    def status():
-         patch("web_interface.blueprints.api_v3.display._stop_display_service") as stop_service, \
+        return {"active": state["active"]}
-         patch("web_interface.blueprints.api_v3.display._ensure_display_service_running") as ensure_running:
+
-        # Active before the request: service_was_running becomes True.
+    def systemctl(args):
-        get_status.return_value = {"active": True}
+        if args[-2:] == ["start", "ledmatrix.service"]:
-        ensure_running.return_value = {"active": True}
+            state["active"] = True
+        elif args[-2:] == ["stop", "ledmatrix.service"]:
+            state["active"] = False
+        return {"returncode": 0, "stdout": "", "stderr": ""}
+
+    with patch("web_interface.blueprints.api_v3._get_display_service_status",
+               side_effect=status), \
+         patch("web_interface.blueprints.api_v3.display._get_display_service_status",
+               side_effect=status), \
+         patch("web_interface.blueprints.api_v3._run_systemctl_command",
+               side_effect=systemctl) as run_systemctl, \
+         patch("web_interface.blueprints.api_v3.display._stop_display_service") as stop_service:
         yield {
-            "get_status": get_status,
+            "state": state,
+            "systemctl": run_systemctl,
             "stop_service": stop_service,
-            "ensure_running": ensure_running,
+            "cache": api_v3_module.api_v3.cache_manager,
         }
 
 
-class TestRestartingARunningService:
+def _mailbox_writes(cache):
-    def test_it_does_not_500(self, api_v3_client, restart_path):
+    return [c.args[1] for c in cache.set.call_args_list if c.args and c.args[0] == MAILBOX]
-        response = api_v3_client.post(
-            URL, json={"plugin_id": "weather", "start_service": True})
-        body = response.get_json()
-        assert response.status_code == 200, body
-        assert body["status"] == "success", body
 
-    def test_the_service_is_actually_stopped_and_restarted(
-            self, api_v3_client, restart_path):
-        api_v3_client.post(
-            URL, json={"plugin_id": "weather", "start_service": True})
-        restart_path["stop_service"].assert_called_once()
-        restart_path["ensure_running"].assert_called_once()
 
-    def test_a_service_that_was_not_running_is_not_stopped_first(
+def _systemctl_verbs(run_systemctl):
-            self, api_v3_client, restart_path):
+    return [c.args[0][-2] for c in run_systemctl.call_args_list]
-        # The buggy import sits inside `if service_was_running and
+
-        # start_service`, so it only ever fired on the restart path --
+
-        # this is the other side of that branch, unaffected either way,
+class TestStartWhileTheServiceIsRunning:
-        # kept here so the branch condition itself stays covered.
+    @pytest.mark.parametrize("body", [
-        restart_path["get_status"].return_value = {"active": False}
+        {"plugin_id": "weather"},                           # "Preview on display", MQTT
-        response = api_v3_client.post(
+        {"plugin_id": "weather", "start_service": True},    # on-demand modal, box ticked
-            URL, json={"plugin_id": "weather", "start_service": True})
+        {"plugin_id": "weather", "start_service": "true"},
+    ])
+    def test_the_service_is_not_stopped_or_restarted(self, api_v3_client, service, body):
+        response = api_v3_client.post(START_URL, json=body)
         assert response.status_code == 200, response.get_json()
-        restart_path["stop_service"].assert_not_called()
+        assert response.get_json()["status"] == "success"
+        service["stop_service"].assert_not_called()
+        assert _systemctl_verbs(service["systemctl"]) == [], (
+            "a running display service was sent a systemctl command")
+
+    def test_the_request_is_posted_for_the_running_display(self, api_v3_client, service):
+        response = api_v3_client.post(
+            START_URL, json={"plugin_id": "weather", "mode": "weather_current",
+                             "duration": 60, "pinned": True})
+        data = response.get_json()["data"]
+        writes = _mailbox_writes(service["cache"])
+        assert len(writes) == 1
+        assert writes[0]["action"] == "start"
+        assert writes[0]["request_id"] == data["request_id"]
+        assert writes[0]["plugin_id"] == "weather"
+        assert writes[0]["mode"] == "weather_current"
+        assert writes[0]["duration"] == 60
+        assert writes[0]["pinned"] is True
+
+    def test_the_response_reports_the_service_was_not_started(self, api_v3_client, service):
+        data = api_v3_client.post(START_URL, json={"plugin_id": "weather"}).get_json()["data"]
+        assert data["service"]["active"] is True
+        assert data["service"]["started"] is False
+
+    def test_it_answers_without_the_old_restart_pause(self, api_v3_client, service):
+        # The restart slept 1.5s; nothing here should sleep at all.
+        with patch("time.sleep") as sleep:
+            api_v3_client.post(START_URL, json={"plugin_id": "weather"})
+        sleep.assert_not_called()
+
+
+class TestStartWhileTheServiceIsStopped:
+    def test_start_service_starts_it_once_and_never_stops_it(self, api_v3_client, service):
+        service["state"]["active"] = False
+        response = api_v3_client.post(START_URL, json={"plugin_id": "weather"})
+        assert response.status_code == 200, response.get_json()
+        assert _systemctl_verbs(service["systemctl"]) == ["start"]
+        service["stop_service"].assert_not_called()
+        # Written before the start, so the new process finds it on its first poll.
+        assert len(_mailbox_writes(service["cache"])) == 1
+
+    def test_without_start_service_it_is_left_stopped(self, api_v3_client, service):
+        service["state"]["active"] = False
+        response = api_v3_client.post(
+            START_URL, json={"plugin_id": "weather", "start_service": "false"})
+        assert response.status_code == 400
+        assert _systemctl_verbs(service["systemctl"]) == []
+
+    def test_a_start_that_fails_is_reported(self, api_v3_client, service):
+        service["state"]["active"] = False
+        service["systemctl"].side_effect = lambda args: {
+            "returncode": 1, "stdout": "", "stderr": "denied"}
+        response = api_v3_client.post(START_URL, json={"plugin_id": "weather"})
+        assert response.status_code == 500
+        assert response.get_json()["status"] == "error"
+
+
+class TestStop:
+    def test_stop_posts_a_stop_request_and_leaves_the_service_running(
+            self, api_v3_client, service):
+        response = api_v3_client.post(STOP_URL, json={})
+        assert response.status_code == 200, response.get_json()
+        writes = _mailbox_writes(service["cache"])
+        assert [w["action"] for w in writes] == ["stop"]
+        service["stop_service"].assert_not_called()
+        assert _systemctl_verbs(service["systemctl"]) == []
+
+    def test_a_string_false_stop_service_does_not_stop_it(self, api_v3_client, service):
+        # bool("false") is True: the flag was read raw and stopped the service.
+        api_v3_client.post(STOP_URL, json={"stop_service": "false"})
+        service["stop_service"].assert_not_called()
+
+    def test_stop_service_true_still_stops_it(self, api_v3_client, service):
+        api_v3_client.post(STOP_URL, json={"stop_service": True})
+        service["stop_service"].assert_called_once()

test/test_web_origin_guard.py

@@ -0,0 +1,285 @@
+"""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

web_interface/app.py

@@ -55,10 +55,17 @@ app = Flask(__name__)
 app.secret_key = os.urandom(24)
 config_manager = ConfigManager()
 
-# No CSRF protection: the UI is meant for the local network, where anyone who
+# Cross-site request forgery: the UI has no login, and being "only on the LAN"
-# can forge a request can also send it directly, and neither the HTMX forms
+# does not keep other websites out. Any page a LAN user opens can make their
-# nor the fetch() calls carry a token. Exposing the UI beyond the LAN needs
+# browser POST to this server -- a plain HTML form is not blocked by CORS -- so
-# CSRF tokens added to both first.
+# a hostile site could reboot the Pi, pull code or rewrite the config through
+# 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:
@@ -402,6 +409,11 @@ 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

@@ -192,8 +192,9 @@ def start_on_demand_display():
                 resolved_plugin,
             )
 
-    # Set the on-demand request in cache FIRST (before starting service)
+    # Post the request to the mailbox the display process polls
-    # This ensures the request is available when the service starts/restarts
+    # (DisplayController._poll_on_demand_requests). Written before any
+    # service start, so a freshly started display finds it on its first poll.
     cache = _cache_manager()
     request_id = data.get('request_id') or str(uuid.uuid4())
     request_payload = {
@@ -207,18 +208,7 @@ def start_on_demand_display():
     }
     cache.set('display_on_demand_request', request_payload)
 
-    # Check if display service is running (or will be started)
     service_status = _get_display_service_status()
-    service_was_running = service_status.get('active', False)
-        
-    # Stop the display service first to ensure clean state when we will restart it
-    if service_was_running and start_service:
-        import time as time_module
-        logger.debug("Stopping display service before starting on-demand mode")
-        _stop_display_service()
-        # Wait a brief moment for the service to fully stop
-        time_module.sleep(1.5)
-        logger.debug("Display service stopped, now starting with on-demand request")
 
     if not service_status.get('active') and not start_service:
         return jsonify({
@@ -227,6 +217,18 @@ def start_on_demand_display():
             'service_status': service_status
         }), 400
 
+    # start_service means "start it if it is not running", as the UI's
+    # checkbox says; _ensure_display_service_running leaves a running service
+    # alone. This used to stop a running service, sleep 1.5s and start it
+    # again, so every on-demand or "Preview on display" click -- and every
+    # MQTT on-demand command, which posts here with the default -- cold-
+    # restarted the display process: every plugin reloaded and the panel was
+    # blank for seconds. The restart bought nothing. The running process
+    # reads this mailbox every ON_DEMAND_POLL_INTERVAL (0.25s), from its
+    # dwell sleep, its render loops and Vegas's interrupt check as well as
+    # the main loop, and a restarted one got the request the same way: the
+    # startup path only restores a session the display itself saved
+    # (display_on_demand_config), so it loaded nothing it would not have had.
     service_result = None
     if start_service:
         service_result = _ensure_display_service_running()
@@ -237,9 +239,6 @@ def start_on_demand_display():
                 'message': 'Failed to start display service. Please check service logs or start it manually.',
                 'service_result': service_result
             }), 500
-            
-        # Service was restarted (or started fresh) with on-demand request in cache
-        # The display controller will read the request during initialization or when it polls
 
     response_data = {
         'request_id': request_id,
@@ -254,10 +253,12 @@ def start_on_demand_display():
 def stop_on_demand_display():
     """Request the display controller to stop on-demand mode."""
     data = request.get_json(silent=True) or {}
-    stop_service = data.get('stop_service', False)
+    # _coerce_to_bool: bool("false") is True, which stopped the service.
+    stop_service = _coerce_to_bool(data.get('stop_service', False))
 
-    # Set the stop request in cache FIRST
+    # The running display reads the stop from the mailbox within
-    # The display controller will poll this and restart without the on-demand filter
+    # ON_DEMAND_POLL_INTERVAL and resumes normal rotation in place
+    # (_clear_on_demand); nothing is restarted.
     cache = _cache_manager()
     request_id = data.get('request_id') or str(uuid.uuid4())
     request_payload = {
@@ -266,10 +267,7 @@ def stop_on_demand_display():
         'timestamp': _pkg.time.time()
     }
     cache.set('display_on_demand_request', request_payload)
-        
+
-    # Note: The display controller's _clear_on_demand() will handle the restart
-    # to restore normal operation with all plugins
-        
     service_result = None
     if stop_service:
         service_result = _stop_display_service()

web_interface/blueprints/api_v3/system.py

@@ -383,16 +383,27 @@ def _perform_core_update_locked(stash_local_changes=True):
 def execute_system_action():
     """Execute system actions (start/stop/reboot/etc)"""
     try:
-        # HTMX sends data as form data, not JSON
+        data = request.get_json(silent=True)
-        data = request.get_json(silent=True) or {}
+        if data is None and not request.is_json:
-        if not data:
+            # Every caller in the interface sends JSON (the Quick Actions
-            # Try to get from form data if JSON fails
+            # buttons use HTMX's json-enc). A form-encoded body is what a
+            # 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 data or 'action' not in data:
+        if not isinstance(data, dict) or not data.get('action'):
             return jsonify({'status': 'error', 'message': 'Action required'}), 400
 
         action = data['action']

web_interface/origin_guard.py

@@ -0,0 +1,191 @@
+"""
+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