fix(web): plugin action params, refused on-demand starts, pending-operation 500, double-click 409, binary static files

CHANGELOG.md

@@ -549,6 +549,45 @@ policies are unchanged.
   screen showed first and the game came after it. Each check also asks each
   plugin `has_live_content()` once, where a plugin registered under several
   modes used to be asked once per mode.
+- A plugin action whose params hold `true`, `false` or `null` runs again.
+  `/api/v3/plugins/action` wrote the params into the source of the wrapper
+  that runs the plugin's script, and those JSON words are not Python, so the
+  wrapper stopped with a NameError and the action answered "Action failed".
+  The plugin file manager's category toggle sends `"enabled": true`, so
+  turning a category on or off in of-the-day always failed. The params now
+  reach the wrapper on its stdin; the script still receives them as JSON on
+  its own stdin, as before.
+- An on-demand request that `/api/v3/display/on-demand/start` refuses no
+  longer runs later. With the display stopped the request goes to the
+  display's mailbox, and the display reads that mailbox for an hour without
+  looking at a request's age. So with "Start display service" unticked, the
+  answer was "Display service is not running", yet the next time the
+  display was started it ran that plugin, pinned if the request said so.
+  The same happened after "Failed to start display service". On either
+  refusal the route now takes its request back out of the mailbox, unless a
+  newer one has replaced it. A request the display acknowledges over the
+  control socket is now a success whatever systemd reports: a display run
+  by hand or in the emulator was told "not running" for a request it had
+  already taken, and with "Start display service" ticked the route tried to
+  start the service beside it.
+- `/api/v3/plugins/operation/<id>` reports a queued operation as `pending`
+  instead of answering 500. The queue keeps an operation's callback among
+  its parameters until it runs, and the status route tried to send that
+  function as JSON. An install queued behind another plugin's install
+  failed every status poll until the first one finished. Parameters whose
+  name starts with `_` are internal and are no longer in the answer.
+- A second click on Install while that plugin is still installing, or an
+  Uninstall during its install, now answers 409 "already has an install,
+  update or uninstall in progress" instead of 500 "An error occurred". The
+  first operation carried on either way. The uninstall route also stopped
+  recording a failed uninstall in the operation history for an uninstall
+  that never started.
+- `/api/v3/plugins/<plugin_id>/static/<path>` serves images and other
+  binary files. It opened every file as UTF-8 text, so a plugin's icon or
+  preview image answered 500 `UnicodeDecodeError`. Files are now sent as
+  they are on disk, an image with its own content type; HTML, JavaScript,
+  CSS, JSON and other text keep the types they had. The path checks are
+  unchanged.
 - The display schedule turns the panel off at exactly the end time. A window
   now runs from its start time up to, but not including, its end time: with
   07:00-23:00 the panel is on at 07:00 and off at 23:00. Before, the end

src/plugin_system/operation_types.py

@@ -48,12 +48,19 @@ class PluginOperation:
     completed_at: Optional[datetime] = None
 
     def to_dict(self) -> Dict[str, Any]:
-        """Convert operation to dictionary for serialization."""
+        """Convert operation to dictionary for serialization.
+
+        Parameters whose name starts with ``_`` are internal and left out:
+        PluginOperationQueue keeps the operation's callback there as
+        ``_callback`` until its worker runs it, and a pending operation's
+        status answered 500 because that function cannot be serialized.
+        """
         return {
             'operation_id': self.operation_id,
             'operation_type': self.operation_type.value,
             'plugin_id': self.plugin_id,
-            'parameters': self.parameters,
+            'parameters': {key: value for key, value in self.parameters.items()
+                           if not str(key).startswith('_')},
             'status': self.status.value,
             'progress': self.progress,
             'message': self.message,

test/test_api_v3_on_demand_restart.py

@@ -37,6 +37,7 @@
 START_URL = "/api/v3/display/on-demand/start"
 STOP_URL = "/api/v3/display/on-demand/stop"
 MAILBOX = "display_on_demand_request"
+DISPLAY = "web_interface.blueprints.api_v3.display"
 
 
 @pytest.fixture
@@ -150,6 +151,102 @@ def test_a_start_that_fails_is_reported(self, api_v3_client, service):
         assert response.get_json()["status"] == "error"
 
 
+class _Mailbox:
+    """The CacheManager calls the routes make, over a dict."""
+
+    def __init__(self):
+        self.entries = {}
+
+    def set(self, key, value, ttl=None):
+        self.entries[key] = value
+
+    def get(self, key, max_age=300, memory_ttl=None):
+        return self.entries.get(key)
+
+    def delete(self, key):
+        self.entries.pop(key, None)
+
+
+class TestARefusedStartLeavesNoRequestBehind:
+    """A start the route answers with an error must not run later.
+
+    The request was posted (to the mailbox, with the display stopped) before
+    the route refused it, and the display reads the mailbox for an hour
+    without looking at a request's age. So "Display service is not running"
+    (start_service off) or "Failed to start display service" left the
+    request waiting, and the next time the display started -- minutes later,
+    by hand -- it ran that plugin, pinned if the request said so.
+
+    A socket acknowledgement is the other side of it: the display answered,
+    so it is running and has the request, whatever systemd says (a display
+    run by hand or in the emulator has no active unit). That is a success,
+    not "not running", and no unit is started beside it.
+    """
+
+    @pytest.fixture
+    def mailbox(self, api_v3_module, service):
+        box = _Mailbox()
+        api_v3_module.api_v3.cache_manager = box
+        service["state"]["active"] = False
+        return box
+
+    @pytest.mark.parametrize("body", [
+        {"plugin_id": "weather", "start_service": False},
+        {"plugin_id": "weather"},                           # start_service defaults on
+    ])
+    def test_a_socket_ack_is_a_success_whatever_systemd_says(
+            self, api_v3_client, service, mailbox, body):
+        with patch(f"{DISPLAY}.control_client.on_demand_start",
+                   side_effect=lambda request_id, *a: {"accepted": True}):
+            response = api_v3_client.post(START_URL, json=body)
+        assert response.status_code == 200, response.get_json()
+        assert response.get_json()["data"]["transport"] == "socket"
+        assert MAILBOX not in mailbox.entries
+        assert _systemctl_verbs(service["systemctl"]) == [], (
+            "a unit was started beside a display that answered the socket")
+
+    def test_without_start_service_the_request_is_taken_back(
+            self, api_v3_client, service, mailbox):
+        response = api_v3_client.post(START_URL, json={
+            "plugin_id": "weather", "pinned": True, "start_service": False})
+        assert response.status_code == 400
+        assert response.get_json()["status"] == "error"
+        assert MAILBOX not in mailbox.entries
+
+    def test_a_start_that_fails_takes_its_request_back(self, api_v3_client, service, mailbox):
+        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 MAILBOX not in mailbox.entries
+
+    def test_a_newer_request_is_left_alone_on_the_400(self, api_v3_client, service, mailbox):
+        newer = {"request_id": "someone-else", "action": "start", "plugin_id": "clock"}
+
+        def stopped_and_another_post_lands(*args):
+            mailbox.entries[MAILBOX] = newer
+            return {"active": False}
+
+        with patch(f"{DISPLAY}._get_display_service_status",
+                   side_effect=stopped_and_another_post_lands):
+            response = api_v3_client.post(START_URL, json={
+                "plugin_id": "weather", "start_service": False})
+        assert response.status_code == 400
+        assert mailbox.entries[MAILBOX] is newer
+
+    def test_a_newer_request_is_left_alone_on_the_500(self, api_v3_client, service, mailbox):
+        newer = {"request_id": "someone-else", "action": "start", "plugin_id": "clock"}
+
+        def start_fails_after_another_post(args):
+            mailbox.entries[MAILBOX] = newer
+            return {"returncode": 1, "stdout": "", "stderr": "denied"}
+
+        service["systemctl"].side_effect = start_fails_after_another_post
+        response = api_v3_client.post(START_URL, json={"plugin_id": "weather"})
+        assert response.status_code == 500
+        assert mailbox.entries[MAILBOX] is newer
+
+
 class TestStop:
     def test_stop_posts_a_stop_request_and_leaves_the_service_running(
             self, api_v3_client, service):

test/test_api_v3_operation_status_pending.py

@@ -0,0 +1,83 @@
+"""GET /api/v3/plugins/operation/<id> answers for an operation still waiting.
+
+PluginOperationQueue keeps an operation's callback in its parameters, under
+``_callback``, until the worker takes it to run. PluginOperation.to_dict()
+returned the parameters as they were, so for a pending operation the route
+handed jsonify a function and answered 500 "A system error occurred". That
+is every poll of an install queued behind another plugin's: the second of
+two installs read as broken until the first one finished.
+"""
+
+import json
+import sys
+import threading
+from pathlib import Path
+
+import pytest
+
+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
+
+from src.plugin_system.operation_queue import PluginOperationQueue  # noqa: E402
+from src.plugin_system.operation_types import (  # noqa: E402
+    OperationType, PluginOperation,
+)
+
+
+def _callback(op):
+    return {"success": True, "message": "done"}
+
+
+class TestToDict:
+    def test_private_parameters_are_left_out(self):
+        op = PluginOperation(OperationType.INSTALL, "demo",
+                             parameters={"_callback": _callback, "branch": "main"})
+        assert op.to_dict()["parameters"] == {"branch": "main"}
+        json.dumps(op.to_dict())   # serializable
+
+    def test_the_operation_keeps_its_callback_for_the_worker(self):
+        op = PluginOperation(OperationType.INSTALL, "demo",
+                             parameters={"_callback": _callback})
+        op.to_dict()
+        assert op.parameters["_callback"] is _callback
+
+    def test_the_other_fields_are_unchanged(self):
+        op = PluginOperation(OperationType.UNINSTALL, "demo", operation_id="op-1")
+        assert op.to_dict() == {
+            "operation_id": "op-1", "operation_type": "uninstall", "plugin_id": "demo",
+            "parameters": {}, "status": "pending", "progress": 0.0, "message": "",
+            "error": None, "result": None,
+            "created_at": op.created_at.isoformat(), "started_at": None,
+            "completed_at": None,
+        }
+
+
+class TestTheRoute:
+    @pytest.fixture
+    def busy_queue(self, api_v3_module):
+        """A real queue whose worker is held by another plugin's operation."""
+        queue = PluginOperationQueue(max_history=10)
+        api_v3_module.api_v3.operation_queue = queue
+        started, release = threading.Event(), threading.Event()
+
+        def blocker(op):
+            started.set()
+            release.wait(10)
+            return {"success": True, "message": "done"}
+
+        queue.enqueue_operation(OperationType.INSTALL, "busy", operation_callback=blocker)
+        assert started.wait(5)
+        yield queue
+        release.set()
+        queue.shutdown()
+
+    def test_a_pending_operation_reports_pending(self, api_v3_client, busy_queue):
+        op_id = busy_queue.enqueue_operation(
+            OperationType.INSTALL, "demo", operation_callback=_callback)
+        response = api_v3_client.get(f"/api/v3/plugins/operation/{op_id}")
+        assert response.status_code == 200, response.get_json()
+        data = response.get_json()["data"]
+        assert data["status"] == "pending"
+        assert data["plugin_id"] == "demo"
+        assert "_callback" not in data["parameters"]

test/test_api_v3_plugin_action_params.py

@@ -0,0 +1,93 @@
+"""POST /api/v3/plugins/action hands ``params`` to the plugin's script intact.
+
+The route runs the script through a generated wrapper, and the params went
+into that wrapper as Python source: ``params = {json.dumps(params)}``. JSON is
+not Python. ``true``, ``false`` and ``null`` are undefined names there, so any
+params holding a boolean or a null died with a NameError before the script
+ran. The plugin file manager's category toggle sends ``{"category_name": ...,
+"enabled": true}``, so of-the-day's category toggle failed every time with
+"Action failed".
+
+The script's side of the contract is unchanged and pinned here too: the
+params arrive on stdin as one JSON document, LEDMATRIX_ROOT is set, and what
+the script prints to stdout is what the route parses.
+"""
+
+import json
+import subprocess
+import sys
+from pathlib import Path
+
+import pytest
+
+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
+
+ACTION_URL = "/api/v3/plugins/action"
+
+# The action script: report what it was handed, as JSON on stdout.
+ECHO_SCRIPT = (
+    "import json, os, sys\n"
+    "raw = sys.stdin.read()\n"
+    "print(json.dumps({'status': 'success', 'got': json.loads(raw),\n"
+    "                  'root': os.environ.get('LEDMATRIX_ROOT')}))\n"
+)
+
+
+@pytest.fixture
+def echo_plugin(tmp_path, api_v3_module, monkeypatch):
+    plugin_dir = tmp_path / "demo"
+    plugin_dir.mkdir()
+    (plugin_dir / "manifest.json").write_text(json.dumps({
+        "id": "demo",
+        "web_ui_actions": [{"id": "toggle", "type": "script", "script": "echo.py"}],
+    }), encoding="utf-8")
+    (plugin_dir / "echo.py").write_text(ECHO_SCRIPT, encoding="utf-8")
+    api_v3_module.api_v3.plugin_catalog.get_plugin_directory.return_value = str(plugin_dir)
+
+    # The route runs `python3`; use this interpreter, so the test does not
+    # depend on what that name resolves to here.
+    real_run = subprocess.run
+
+    def run(cmd, *args, **kwargs):
+        if isinstance(cmd, list) and cmd and cmd[0] == "python3":
+            cmd = [sys.executable] + cmd[1:]
+        return real_run(cmd, *args, **kwargs)
+
+    monkeypatch.setattr(subprocess, "run", run)
+    return plugin_dir
+
+
+@pytest.mark.parametrize("params", [
+    {"category_name": "jokes", "enabled": True},   # the file manager's toggle
+    {"category_name": "jokes", "enabled": False},
+    {"filename": None},
+    {"nested": {"list": [1, None, True, 2.5], "empty": {}}},
+    {"text": "café ✓ \U0001F600"},
+    {"text": "he said \"hi\" and 'bye' \\ ''' \"\"\" \n\t end"},
+], ids=["true", "false", "null", "nested", "unicode", "quotes"])
+def test_the_script_receives_the_params_it_was_sent(api_v3_client, echo_plugin, params):
+    response = api_v3_client.post(ACTION_URL, json={
+        "plugin_id": "demo", "action_id": "toggle", "params": params})
+    body = response.get_json()
+    assert response.status_code == 200, body
+    assert body["got"] == params
+
+
+def test_a_param_cannot_run_code_in_the_wrapper(api_v3_client, echo_plugin, tmp_path):
+    marker = tmp_path / "PWNED"
+    hostile = "\"}\nopen(%r, 'w').write('ran')\n#" % str(marker)
+    params = {"name": hostile, "flag": True}
+    response = api_v3_client.post(ACTION_URL, json={
+        "plugin_id": "demo", "action_id": "toggle", "params": params})
+    assert response.status_code == 200, response.get_json()
+    assert response.get_json()["got"] == params
+    assert not marker.exists(), "a param value ran as code"
+
+
+def test_the_script_still_gets_ledmatrix_root(api_v3_client, echo_plugin, api_v3_module):
+    response = api_v3_client.post(ACTION_URL, json={
+        "plugin_id": "demo", "action_id": "toggle", "params": {"enabled": True}})
+    assert response.status_code == 200, response.get_json()
+    assert response.get_json()["root"] == str(api_v3_module.PROJECT_ROOT)