Merge refactor/api-v3-blueprint-split into feat/tools-mqtt-bridge-pixlet-editor-on-split

PR #554 was branched from #553 before its CodeRabbit-fix commit
(4e91cf1) landed, leaving GitHub reporting mergeable_state: dirty and
the branch missing eight fixes to shared api_v3 package code —
including a guaranteed ModuleNotFoundError on display.py's on-demand
restart path, a credential-redaction gap, and an unguarded manifest
read-modify-write race.

Resolved the two import-list conflicts (backup.py, starlark.py) by
unioning both branches' additions; the rest of the merge was clean.
Verified the merged tree keeps every usage of the fix branch's new
helpers (_starlark_manifest_lock, _coerce_to_bool) intact, the URL map
fixture still lists all 117 routes, and the full suite passes with only
the two pre-existing tzdata-related failures (unrelated, present on
origin/main too).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GwNEnaWDTzDbFE2M7MDjaq

test/test_api_v3_font_upload_size.py

@@ -0,0 +1,60 @@
+"""Regression test: POST /fonts/upload must enforce its own stated size limit.
+
+validate_file_upload(filename, max_size_mb=10, allowed_extensions=[...]) reads
+like it checks the upload's size, but it only ever validated the filename
+(traversal characters, extension) -- max_size_mb was accepted and silently
+ignored. Nothing else in the handler checked the actual upload size either,
+so it saved whatever was posted to assets/fonts/<family><ext> regardless of
+size, unlike the sibling .star and plugin-asset upload routes, which check
+`file.tell()` against a stated limit before saving.
+"""
+
+import io
+import sys
+from pathlib import Path
+from unittest.mock import patch
+
+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
+
+URL = "/api/v3/fonts/upload"
+TEN_MB = 10 * 1024 * 1024
+
+
+@pytest.fixture
+def fonts_root(tmp_path):
+    # PROJECT_ROOT is bound by value in fonts.py, so it is patched there.
+    with patch("web_interface.blueprints.api_v3.fonts.PROJECT_ROOT", tmp_path):
+        yield tmp_path
+
+
+def upload(client, content, filename="myfont.ttf", family="myfont"):
+    data = {
+        "font_file": (io.BytesIO(content), filename),
+        "font_family": family,
+    }
+    return client.post(URL, data=data, content_type="multipart/form-data")
+
+
+class TestFontUploadSizeLimit:
+    def test_oversized_font_is_rejected(self, api_v3_client, fonts_root):
+        response = upload(api_v3_client, b"x" * (TEN_MB + 1))
+        assert response.status_code == 400
+        assert "too large" in response.get_json()["message"].lower()
+        assert not (fonts_root / "assets" / "fonts" / "myfont.ttf").exists(), (
+            "an oversized font was saved to disk before being rejected")
+
+    def test_a_font_right_at_the_limit_is_accepted(self, api_v3_client, fonts_root):
+        response = upload(api_v3_client, b"x" * TEN_MB,
+                          filename="atlimit.ttf", family="atlimit")
+        assert response.status_code == 200, response.get_json()
+        assert (fonts_root / "assets" / "fonts" / "atlimit.ttf").exists()
+
+    def test_an_ordinary_small_font_is_still_accepted(self, api_v3_client, fonts_root):
+        response = upload(api_v3_client, b"fake font bytes",
+                          filename="small.ttf", family="small")
+        assert response.status_code == 200, response.get_json()
+        assert (fonts_root / "assets" / "fonts" / "small.ttf").read_bytes() == b"fake font bytes"

test/test_api_v3_on_demand_restart.py

@@ -0,0 +1,93 @@
+"""Regression test: POST /display/on-demand/start restarting a running
+service must not import a name that does not exist.
+
+display.py has `import web_interface.blueprints.api_v3 as _pkg` and reads
+mutable, test-patched attributes back through it (`_pkg.time.time()`,
+`_pkg._get_starlark_plugin()`, ...) rather than binding them by value, per
+the package's own docstring. One spot went further and wrote a genuine
+`import` *statement* against that alias --
+
+    import _pkg.time as time_module
+
+-- but `_pkg` is a local name bound by `import ... as _pkg` in this module,
+not a real top-level package, so `import _pkg.time` is not something Python
+can resolve; it raises ModuleNotFoundError. That line only runs when the
+display service is already running and the caller also asked to (re)start
+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
+caller as a handled 500 with a generic message, not an unhandled crash --
+but a 500 all the same on a request that should have restarted the service
+and reported success.
+"""
+
+import sys
+from pathlib import Path
+from unittest.mock import MagicMock, patch
+
+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
+
+URL = "/api/v3/display/on-demand/start"
+
+
+@pytest.fixture
+def restart_path(api_v3_module):
+    """Force the `service_was_running and start_service` branch.
+
+    plugin_manager and config_manager are set to None so the route takes
+    the simplest path to that branch rather than tripping over unrelated
+    MagicMock plumbing; _ensure_cache_manager, _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
+
+    with patch("web_interface.blueprints.api_v3.display._ensure_cache_manager") as ensure_cache, \
+         patch("web_interface.blueprints.api_v3.display._get_display_service_status") as get_status, \
+         patch("web_interface.blueprints.api_v3.display._stop_display_service") as stop_service, \
+         patch("web_interface.blueprints.api_v3.display._ensure_display_service_running") as ensure_running:
+        ensure_cache.return_value = MagicMock()
+        # Active before the request: service_was_running becomes True.
+        get_status.return_value = {"active": True}
+        ensure_running.return_value = {"active": True}
+        yield {
+            "ensure_cache": ensure_cache,
+            "get_status": get_status,
+            "stop_service": stop_service,
+            "ensure_running": ensure_running,
+        }
+
+
+class TestRestartingARunningService:
+    def test_it_does_not_500(self, api_v3_client, restart_path):
+        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(
+            self, api_v3_client, restart_path):
+        # 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,
+        # kept here so the branch condition itself stays covered.
+        restart_path["get_status"].return_value = {"active": False}
+        response = api_v3_client.post(
+            URL, json={"plugin_id": "weather", "start_service": True})
+        assert response.status_code == 200, response.get_json()
+        restart_path["stop_service"].assert_not_called()

test/test_api_v3_schedule_error_messages.py

@@ -0,0 +1,54 @@
+"""Regression test: per-day schedule validation errors must say "time", not
+"_pkg.time".
+
+The split into a package rewrote every bare `time` reference that needed to
+read through the shared module as `_pkg.time` (see the package's own
+docstring on why -- tests patch it, so it has to be read back live rather
+than bound by value). That rewrite was mechanical and matched the substring
+"time" inside string literals and comments too, so the user-facing message
+
+    "Invalid start time for {day}: ..."
+
+came out as
+
+    "Invalid start _pkg.time for {day}: ..."
+
+in both POST /config/schedule and POST /config/dim-schedule, for both the
+start and end time of a per-day entry.
+"""
+
+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
+
+
+@pytest.mark.parametrize("url", [
+    "/api/v3/config/schedule",
+    "/api/v3/config/dim-schedule",
+])
+class TestPerDayTimeErrorsAreNotCorrupted:
+    def test_invalid_start_time_message(self, api_v3_client, api_v3_module, url):
+        response = api_v3_client.post(url, json={
+            "mode": "per_day",
+            "monday_start": "not-a-time",
+        })
+        assert response.status_code == 400
+        message = response.get_json()["message"]
+        assert "_pkg" not in message, message
+        assert message.startswith("Invalid start time for monday:"), message
+
+    def test_invalid_end_time_message(self, api_v3_client, api_v3_module, url):
+        response = api_v3_client.post(url, json={
+            "mode": "per_day",
+            "monday_start": "07:00",
+            "monday_end": "not-a-time",
+        })
+        assert response.status_code == 400
+        message = response.get_json()["message"]
+        assert "_pkg" not in message, message
+        assert message.startswith("Invalid end time for monday:"), message

test/test_api_v3_wifi_endpoints.py

@@ -229,6 +229,61 @@ class TestRadio:
         assert api_v3_client.post(self.URL, json={"enabled": True}).status_code == 500
 
 
+class TestAutoEnableApMode:
+    URL = "/api/v3/wifi/ap/auto-enable"
+
+    def test_requires_the_field(self, api_v3_client, wifi_manager):
+        response = api_v3_client.post(self.URL, json={})
+        assert response.status_code == 400
+
+    @pytest.mark.parametrize("raw,expected", [
+        (True, True), (False, False),
+        ("true", True), ("True", True), ("1", True), ("yes", True),
+        ("false", False), ("False", False), ("0", False), ("no", False),
+        (1, True), (0, False),
+    ])
+    def test_value_is_coerced_not_just_truthy(
+            self, api_v3_client, wifi_manager, raw, expected):
+        # Regression: bool(data['auto_enable_ap_mode']) meant a caller who
+        # sent the JSON string "false" got it stored as True — bool("false")
+        # is True, since any non-empty string is truthy.
+        wifi_manager.config = {}
+        response = api_v3_client.post(self.URL, json={"auto_enable_ap_mode": raw})
+        assert response.status_code == 200, response.get_json()
+        assert response.get_json()["data"]["auto_enable_ap_mode"] is expected
+        assert wifi_manager.config["auto_enable_ap_mode"] is expected
+
+    def test_a_string_false_does_not_enable_it(self, api_v3_client, wifi_manager):
+        # The exact shape of the bug.
+        wifi_manager.config = {}
+        api_v3_client.post(self.URL, json={"auto_enable_ap_mode": "false"})
+        assert wifi_manager.config["auto_enable_ap_mode"] is False
+
+
+class TestRadioEnabledAndForceAcceptIntegers:
+    """`{"enabled": 1}` / `{"enabled": 0}` used to be mishandled: the old
+    coercion was `raw is True or (isinstance(raw, str) and ...)`, and
+    `1 is True` is False in Python -- an int is never the `True` singleton
+    even though it equals it -- so a plain integer fell through to False
+    regardless of its value.
+    """
+    URL = "/api/v3/wifi/radio"
+
+    @pytest.mark.parametrize("raw,expected", [(1, True), (0, False)])
+    def test_enabled_as_an_integer(self, api_v3_client, wifi_manager, raw, expected):
+        wifi_manager.set_wifi_radio.return_value = (True, "ok", None)
+        wifi_manager.get_wifi_radio_state.return_value = {}
+        api_v3_client.post(self.URL, json={"enabled": raw})
+        wifi_manager.set_wifi_radio.assert_called_once_with(expected, force=False)
+
+    @pytest.mark.parametrize("raw,expected", [(1, True), (0, False)])
+    def test_force_as_an_integer(self, api_v3_client, wifi_manager, raw, expected):
+        wifi_manager.set_wifi_radio.return_value = (True, "ok", None)
+        wifi_manager.get_wifi_radio_state.return_value = {}
+        api_v3_client.post(self.URL, json={"enabled": True, "force": raw})
+        wifi_manager.set_wifi_radio.assert_called_once_with(True, force=expected)
+
+
 class TestNoRealNetworking:
     def test_wifi_manager_is_never_constructed_for_real(self, api_v3_client):
         # Guard against a future refactor moving the import to module level,

test/test_config_main_redacts_secrets.py

@@ -103,6 +103,43 @@ def test_non_dict_input_passes_through():
     assert _redact_credentials(None) is None
 
 
+def test_a_list_of_bare_credentials_is_redacted():
+    """A credential-named key whose value is a list of scalars, not a list
+    of named-field dicts, used to pass through untouched: the dict branch
+    recursed into `v` on its own value only, so a bare string list item had
+    no field name to test and fell through the base case unredacted.
+    """
+    config = {"tokens": ["abc123", "def456"], "timezone": "America/New_York"}
+    out = _redact_credentials(config)
+    assert out["tokens"] == ["", ""]
+    assert out["timezone"] == "America/New_York"
+
+
+def test_a_list_nested_inside_a_credential_named_container_is_also_blanked():
+    """The list fix applies at any depth under a credential-named key, not
+    only when the list is the key's direct value.
+    """
+    config = {"auth": {"backup_tokens": ["a", "b"], "note": "keep"}}
+    out = _redact_credentials(config)
+    assert out["auth"]["backup_tokens"] == ["", ""]
+    assert out["auth"]["note"] == "keep"
+
+
+def test_credential_shaped_container_with_named_fields_still_only_blanks_those():
+    """Unchanged behaviour for the case the container test above already
+    covers: a dict whose sub-keys are semantically named fields is walked
+    normally, not blanket-blanked, so an ordinary field next to a
+    credential-named one survives.
+    """
+    config = {"secrets": {"api_key": "k", "note": "keep", "list": ["a", "b"]}}
+    out = _redact_credentials(config)
+    assert out["secrets"]["api_key"] == ""
+    assert out["secrets"]["note"] == "keep"
+    # "list" isn't itself credential-named, so it is walked, not blanked --
+    # but it holds no credential-named field either, so it survives whole.
+    assert out["secrets"]["list"] == ["a", "b"]
+
+
 def test_the_endpoint_itself_redacts():
     """Through the view function, not the helper.
 

test/web_interface/test_api_v3_backup_restore.py

@@ -140,6 +140,41 @@ class TestRequestValidation:
         assert restore.call_args[0][2].restore_config is True
 
 
+class TestOptionsAreBooleanAware:
+    """Regression: bool("false") is True in Python.
+
+    Every restore flag used bare bool() coercion, so a caller that sends its
+    options as JSON strings rather than real booleans -- a form field, a
+    hand-built request -- had `{"restore_secrets": "false"}` restore secrets
+    anyway, the opposite of what was asked. Fixed with the same
+    string-aware `_coerce_to_bool` already used for checkbox-style config
+    fields elsewhere in this package (config.py, plugins.py).
+    """
+
+    @pytest.mark.parametrize("raw,expected", [
+        ("false", False), ("False", False), ("FALSE", False),
+        ("0", False),
+        ("true", True), ("True", True), ("1", True),
+    ])
+    def test_string_valued_flags_are_parsed_not_just_truthy(
+            self, client, restore, raw, expected):
+        post(client, options=json.dumps({"restore_secrets": raw}))
+        assert restore.call_args[0][2].restore_secrets is expected
+
+    def test_a_string_false_does_not_restore_secrets(self, client, restore):
+        # The exact shape of the bug: a truthy non-empty string coerced by
+        # bare bool() to True regardless of its contents.
+        post(client, options=json.dumps({"restore_secrets": "false"}))
+        assert restore.call_args[0][2].restore_secrets is False
+
+    def test_real_json_booleans_still_work(self, client, restore):
+        post(client, options=json.dumps({"restore_secrets": False,
+                                          "restore_config": True}))
+        options = restore.call_args[0][2]
+        assert options.restore_secrets is False
+        assert options.restore_config is True
+
+
 class TestSuccess:
     def test_success_returns_the_result(self, client, restore):
         restore.return_value = FakeResult(success=True, restored=["config", "secrets"])

test/web_interface/test_calendar_oauth_endpoints.py

@@ -363,6 +363,22 @@ class TestDiagnosticsAreRedacted:
         assert 'hunter2' not in error, error
         assert '<redacted>' in error, error
 
+    def test_script_stderr_is_redacted_in_the_log_too(self, tmp_path, caplog):
+        # Regression: the return value went through redact_text (asserted
+        # above), but the logger.error call right next to it logged `raw`
+        # verbatim -- a script that handles OAuth client secrets and can
+        # quote them in its stderr, landing unredacted in the log (CWE-532).
+        script = tmp_path / 'calendar_registration.py'
+        script.write_text(
+            'import sys\n'
+            'sys.stderr.write("boom client_secret=hunter2 more\\n")\n',
+            encoding='utf-8')
+        with caplog.at_level('ERROR', logger=mod.logger.name):
+            mod._run_calendar_registration(tmp_path, '')
+        logged = '\n'.join(r.getMessage() for r in caplog.records)
+        assert 'hunter2' not in logged, logged
+        assert '<redacted>' in logged, logged
+
     def test_a_failing_script_payload_is_redacted(self, client):
         (client.plugin_dir / 'credentials.json').write_text('{}', encoding='utf-8')
         (client.plugin_dir / 'calendar_registration.py').write_text(

test/web_interface/test_starlark_pixlet_routes.py

@@ -395,6 +395,132 @@ class TestTheManifestStaysRelocatable:
         assert self._install(tmp_path)['star_file'] == 'demo.star'
 
 
+class TestManifestLockPreventsLostUpdates:
+    """The standalone manifest fallback (no plugin instance loaded) reads,
+    mutates and writes manifest.json with no coordination across requests.
+    Each write is atomic on its own (temp file + rename), but two concurrent
+    read-modify-write cycles can still race: both read the same starting
+    manifest, and the second write silently discards whatever the first one
+    added. _starlark_manifest_lock closes that window -- mirrors
+    StarlarkAppsPlugin._update_manifest_safe, which already does this when
+    the plugin instance is loaded.
+    """
+
+    @pytest.fixture
+    def starlark_dir(self, tmp_path, monkeypatch):
+        from web_interface.blueprints import api_v3 as module
+        apps_dir = tmp_path / "starlark-apps"
+        apps_dir.mkdir()
+        monkeypatch.setattr(module, '_STARLARK_APPS_DIR', apps_dir)
+        monkeypatch.setattr(module, '_STARLARK_MANIFEST_FILE', apps_dir / 'manifest.json')
+        module._write_starlark_manifest({'apps': {}})
+        return apps_dir
+
+    def test_two_concurrent_updates_are_both_kept(self, starlark_dir):
+        import threading
+        import time as _time
+
+        from web_interface.blueprints import api_v3 as module
+
+        def add_app(app_id):
+            with module._starlark_manifest_lock():
+                manifest = module._read_starlark_manifest()
+                # Widen the window between read and write. Without the lock
+                # both threads read here before either writes, and whichever
+                # writes second overwrites the other's addition; with the
+                # lock, the second thread cannot even start its read until
+                # the first has written and released.
+                _time.sleep(0.05)
+                manifest.setdefault('apps', {})[app_id] = {'enabled': True}
+                module._write_starlark_manifest(manifest)
+
+        threads = [threading.Thread(target=add_app, args=(app_id,))
+                   for app_id in ('a', 'b')]
+        for t in threads:
+            t.start()
+        for t in threads:
+            t.join(timeout=5)
+
+        manifest = module._read_starlark_manifest()
+        assert set(manifest['apps']) == {'a', 'b'}, (
+            "a concurrent update was lost: %r" % (manifest,))
+
+    def test_the_lock_is_reentrant_safe_across_sequential_calls(self, starlark_dir):
+        """Not reentrant within one thread -- just that using it twice in a
+        row (the ordinary case: one request, then the next) works cleanly
+        and does not leak the lock file descriptor or leave it locked."""
+        from web_interface.blueprints import api_v3 as module
+
+        for app_id in ('first', 'second'):
+            with module._starlark_manifest_lock():
+                manifest = module._read_starlark_manifest()
+                manifest.setdefault('apps', {})[app_id] = {'enabled': True}
+                module._write_starlark_manifest(manifest)
+
+        manifest = module._read_starlark_manifest()
+        assert set(manifest['apps']) == {'first', 'second'}
+
+
+class TestConfigAndManifestStayInSync:
+    """Standalone-mode PUT /starlark/apps/<id>/config (no plugin instance
+    loaded) writes config.json and then the manifest. If the manifest write
+    fails after config.json was already written, the two disagree about
+    what was saved unless config.json is rolled back.
+    """
+
+    @pytest.fixture
+    def app_dir(self, tmp_path, monkeypatch):
+        from web_interface.blueprints import api_v3 as module
+        apps_dir = tmp_path / "starlark-apps"
+        apps_dir.mkdir()
+        monkeypatch.setattr(module, '_STARLARK_APPS_DIR', apps_dir)
+        monkeypatch.setattr(module, '_STARLARK_MANIFEST_FILE', apps_dir / 'manifest.json')
+        one_app_dir = apps_dir / 'demo'
+        one_app_dir.mkdir()
+        module._write_starlark_manifest({'apps': {'demo': {'name': 'Demo', 'enabled': True}}})
+        return one_app_dir
+
+    def test_manifest_write_failure_rolls_back_an_existing_config_json(self, client, app_dir):
+        config_file = app_dir / 'config.json'
+        config_file.write_text(json.dumps({'existing': 'value'}))
+
+        with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=None), \
+             patch('web_interface.blueprints.api_v3._write_starlark_manifest', return_value=False):
+            resp = client.put('/api/v3/starlark/apps/demo/config',
+                              json={'new_field': 'x'})
+
+        assert resp.status_code == 500
+        assert json.loads(config_file.read_text()) == {'existing': 'value'}, (
+            "config.json kept the new value even though the manifest write "
+            "that was supposed to follow it failed")
+
+    def test_manifest_write_failure_removes_a_freshly_created_config_json(self, client, app_dir):
+        config_file = app_dir / 'config.json'
+        assert not config_file.exists()
+
+        with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=None), \
+             patch('web_interface.blueprints.api_v3._write_starlark_manifest', return_value=False):
+            resp = client.put('/api/v3/starlark/apps/demo/config',
+                              json={'new_field': 'x'})
+
+        assert resp.status_code == 500
+        assert not config_file.exists(), (
+            "config.json was left behind even though the manifest write "
+            "that was supposed to follow it failed")
+
+    def test_success_updates_both_config_and_manifest(self, client, app_dir):
+        from web_interface.blueprints import api_v3 as module
+
+        with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=None):
+            resp = client.put('/api/v3/starlark/apps/demo/config',
+                              json={'new_field': 'x'})
+
+        assert resp.status_code == 200, resp.get_json()
+        assert json.loads((app_dir / 'config.json').read_text())['new_field'] == 'x'
+        manifest = json.loads(module._STARLARK_MANIFEST_FILE.read_text())
+        assert manifest['apps']['demo']['config']['new_field'] == 'x'
+
+
 # ---------------------------------------------------------------------------
 # The store loaded, then stopped loading, and nothing anywhere said why.
 #

web_interface/blueprints/api_v3/__init__.py

@@ -16,6 +16,8 @@ The route modules are imported at the *bottom*: they import names from here, so
 everything they need has to exist first.
 """
 from flask import Blueprint, request, jsonify, Response
+import contextlib
+import fcntl
 import json
 import signal
 import contextlib
@@ -296,12 +298,31 @@ def _redact_credentials(value):
     that round-trips this response cannot erase a secret it never saw.
     """
     if isinstance(value, dict):
-        return {k: ("" if _looks_like_a_credential(k) and not isinstance(v, (dict, list))
+        return {k: (_blank_credential_value(v) if _looks_like_a_credential(k)
                     else _redact_credentials(v))
                 for k, v in value.items()}
     if isinstance(value, list):
         return [_redact_credentials(item) for item in value]
     return value
+def _blank_credential_value(value):
+    """Blank a value that sits under a credential-shaped key.
+
+    A dict is still walked -- `secrets: {api_key: ..., note: ...}` is a
+    section name, not a value to blank in one go, so sub-fields that are not
+    themselves credential-named survive (see
+    test_a_credential_shaped_container_is_still_walked). A scalar list item
+    has no field name to test, though: `_redact_credentials` previously
+    recursed into a list the same way it recurses into a dict, and a scalar
+    is returned unchanged by the base case, so a credential-named key whose
+    value was a bare list of secrets (e.g. `tokens: ["a", "b"]`) passed
+    through with nothing redacted. Blank every scalar reached under this key,
+    at any depth, instead.
+    """
+    if isinstance(value, dict):
+        return _redact_credentials(value)
+    if isinstance(value, list):
+        return [_blank_credential_value(item) for item in value]
+    return ""
 def _validate_time_format(time_str):
     """Validate time format is HH:MM"""
     try:
@@ -1235,12 +1256,11 @@ def _run_calendar_registration(plugin_dir: Path, stdin_payload: str):
             return payload, None
 
     raw = (result.stderr or result.stdout or '').strip()
-    # The unredacted text goes to the log, where it is worth having in full.
-    # What comes back over HTTP is redacted: this is a script that handles
-    # OAuth client secrets, and its stderr can quote them.
+    # Redacted in the log too: this is a script that handles OAuth client
+    # secrets, and its stderr can quote them verbatim (CWE-532).
     if raw:
         logger.error('calendar_registration.py failed (exit %s): %s',
-                     result.returncode, raw)
+                     result.returncode, redact_text(raw))
     return None, 'Authentication script produced no result%s' % (
         ': %s' % redact_text(raw) if raw else '')
 def _resolve_backup_export_dir() -> Path:
@@ -1323,6 +1343,31 @@ def _find_pixlet_binary(explicit_path: Optional[str] = None) -> Optional[str]:
                     return str(bundled)
                 logger.warning("Pixlet bundled binary still not executable after chmod (%s); falling back to PATH", bundled)
     return shutil.which("pixlet")
+@contextlib.contextmanager
+def _starlark_manifest_lock():
+    """Hold an exclusive lock across a standalone starlark-manifest read-modify-write.
+
+    StarlarkAppsPlugin._update_manifest_safe (plugin-repos/starlark-apps/manager.py)
+    already does this -- fcntl.flock held for the whole read-modify-write cycle --
+    when the plugin instance is loaded. These routes fall back to reading and
+    writing manifest.json directly when it is not, and did so with no lock: each
+    _write_starlark_manifest() call is atomic on its own (temp file + rename), but
+    two concurrent requests can each read the manifest, mutate their own copy, and
+    write it back, and the second write silently discards the first's change.
+
+    Callers should do their read, mutation and _write_starlark_manifest() call
+    entirely inside the `with` block, mirroring the plugin's lock scope.
+    """
+    _STARLARK_APPS_DIR.mkdir(parents=True, exist_ok=True)
+    lock_fd = os.open(str(_STARLARK_MANIFEST_FILE), os.O_CREAT | os.O_RDWR, 0o644)
+    try:
+        fcntl.flock(lock_fd, fcntl.LOCK_EX)
+        try:
+            yield
+        finally:
+            fcntl.flock(lock_fd, fcntl.LOCK_UN)
+    finally:
+        os.close(lock_fd)
 def _read_starlark_manifest() -> Dict[str, Any]:
     """Read the starlark-apps manifest.json directly from disk."""
     try:
@@ -1665,22 +1710,23 @@ def _install_star_file(app_id: str, star_file_path: str, metadata: Dict[str, Any
     with open(config_path, 'w') as f:
         json.dump(default_config, f, indent=2)
 
-    manifest = _read_starlark_manifest()
-    manifest.setdefault('apps', {})[app_id] = {
-        'name': metadata.get('name', app_id),
-        'enabled': True,
-        'render_interval': metadata.get('render_interval', 300),
-        'display_duration': metadata.get('display_duration', 15),
-        'config': metadata.get('config', {}),
-        # The filename, not the full path. Readers join this to the app's own
-        # directory and fall back to a bare '<app_id>.star', so an absolute
-        # value gave the key two meanings -- and Path.__truediv__ discards the
-        # left side when the right is absolute, which pinned the manifest to
-        # whatever PROJECT_ROOT installed it. Moving or redeploying the install
-        # then left the app unable to find its own file.
-        'star_file': dest.name,
-    }
-    return _write_starlark_manifest(manifest)
+    with _starlark_manifest_lock():
+        manifest = _read_starlark_manifest()
+        manifest.setdefault('apps', {})[app_id] = {
+            'name': metadata.get('name', app_id),
+            'enabled': True,
+            'render_interval': metadata.get('render_interval', 300),
+            'display_duration': metadata.get('display_duration', 15),
+            'config': metadata.get('config', {}),
+            # The filename, not the full path. Readers join this to the app's own
+            # directory and fall back to a bare '<app_id>.star', so an absolute
+            # value gave the key two meanings -- and Path.__truediv__ discards the
+            # left side when the right is absolute, which pinned the manifest to
+            # whatever PROJECT_ROOT installed it. Moving or redeploying the install
+            # then left the app unable to find its own file.
+            'star_file': dest.name,
+        }
+        return _write_starlark_manifest(manifest)
 def _starlark_virtual_plugins() -> list:
     """Installed Starlark apps, shaped like plugin entries.
 
@@ -1757,14 +1803,15 @@ def _toggle_starlark_app(app_id: str, enabled: bool):
         # Only now is the in-memory copy allowed to disagree with disk.
         plugin.apps[safe_id].manifest['enabled'] = enabled
     else:
-        manifest = _read_starlark_manifest()
-        app_data = manifest.get('apps', {}).get(safe_id)
-        if not app_data:
-            return jsonify({'status': 'error',
-                            'message': f'Starlark app not found: {safe_id}'}), 404
-        app_data['enabled'] = enabled
-        if not _write_starlark_manifest(manifest):
-            return jsonify({'status': 'error', 'message': 'Failed to save manifest'}), 500
+        with _starlark_manifest_lock():
+            manifest = _read_starlark_manifest()
+            app_data = manifest.get('apps', {}).get(safe_id)
+            if not app_data:
+                return jsonify({'status': 'error',
+                                'message': f'Starlark app not found: {safe_id}'}), 404
+            app_data['enabled'] = enabled
+            if not _write_starlark_manifest(manifest):
+                return jsonify({'status': 'error', 'message': 'Failed to save manifest'}), 500
 
     return jsonify({'status': 'success',
                     'message': f"Starlark app {'enabled' if enabled else 'disabled'}",

web_interface/blueprints/api_v3/backup.py

@@ -4,8 +4,9 @@ Routes decorate the shared `api_v3` Blueprint from ._common, so their
 endpoint names are unchanged by living here.
 """
 from web_interface.blueprints.api_v3 import (
-    PROJECT_ROOT, Path, _safe_backup_path, api_v3, datetime, json, jsonify,
-    logger, os, plugin_store_manager, request, tempfile,
+    PROJECT_ROOT, Path, _coerce_to_bool, _safe_backup_path, api_v3,
+    datetime, json, jsonify, logger, os, plugin_store_manager, request,
+    tempfile,
 )
 import web_interface.blueprints.api_v3 as _pkg
 # Read through the module rather than bound by value: tests patch these
@@ -111,13 +112,17 @@ def backup_restore():
                 'status': 'error',
                 'message': 'Invalid options: expected a JSON object',
             }), 400
+        # _coerce_to_bool (not bare bool()) because a request can send these
+        # as JSON strings: bool("false") is True in Python, so a caller who
+        # explicitly asked to skip secrets would have had them restored
+        # anyway.
         options = RestoreOptions(
-            restore_config=bool(opts_dict.get('restore_config', True)),
-            restore_secrets=bool(opts_dict.get('restore_secrets', True)),
-            restore_wifi=bool(opts_dict.get('restore_wifi', True)),
-            restore_fonts=bool(opts_dict.get('restore_fonts', True)),
-            restore_plugin_uploads=bool(opts_dict.get('restore_plugin_uploads', True)),
-            reinstall_plugins=bool(opts_dict.get('reinstall_plugins', True)),
+            restore_config=_coerce_to_bool(opts_dict.get('restore_config', True)),
+            restore_secrets=_coerce_to_bool(opts_dict.get('restore_secrets', True)),
+            restore_wifi=_coerce_to_bool(opts_dict.get('restore_wifi', True)),
+            restore_fonts=_coerce_to_bool(opts_dict.get('restore_fonts', True)),
+            restore_plugin_uploads=_coerce_to_bool(opts_dict.get('restore_plugin_uploads', True)),
+            reinstall_plugins=_coerce_to_bool(opts_dict.get('reinstall_plugins', True)),
         )
         with tempfile.NamedTemporaryFile(suffix='.zip', delete=False) as tmp:
             tmp_path = tmp.name

web_interface/blueprints/api_v3/config.py

@@ -84,7 +84,7 @@ def save_schedule_config():
             start_time = data.get('start_time', '07:00')
             end_time = data.get('end_time', '23:00')
 
-            # Validate _pkg.time formats
+            # Validate time formats
             is_valid, error_msg = _validate_time_format(start_time)
             if not is_valid:
                 return error_response(
@@ -148,12 +148,12 @@ def save_schedule_config():
                     else:
                         end_time = '23:00'
 
-                    # Validate _pkg.time formats
+                    # Validate time formats
                     is_valid, error_msg = _validate_time_format(start_time)
                     if not is_valid:
                         return error_response(
                             ErrorCode.VALIDATION_ERROR,
-                            f"Invalid start _pkg.time for {day}: {error_msg}",
+                            f"Invalid start time for {day}: {error_msg}",
                             status_code=400
                         )
 
@@ -161,7 +161,7 @@ def save_schedule_config():
                     if not is_valid:
                         return error_response(
                             ErrorCode.VALIDATION_ERROR,
-                            f"Invalid end _pkg.time for {day}: {error_msg}",
+                            f"Invalid end time for {day}: {error_msg}",
                             status_code=400
                         )
 
@@ -313,7 +313,7 @@ def save_dim_schedule_config():
             start_time = data.get('start_time', '20:00')
             end_time = data.get('end_time', '07:00')
 
-            # Validate _pkg.time formats
+            # Validate time formats
             is_valid, error_msg = _validate_time_format(start_time)
             if not is_valid:
                 return error_response(
@@ -365,12 +365,12 @@ def save_dim_schedule_config():
                     start_time = data.get(start_key) or '20:00'
                     end_time = data.get(end_key) or '07:00'
 
-                    # Validate _pkg.time formats
+                    # Validate time formats
                     is_valid, error_msg = _validate_time_format(start_time)
                     if not is_valid:
                         return error_response(
                             ErrorCode.VALIDATION_ERROR,
-                            f"Invalid start _pkg.time for {day}: {error_msg}",
+                            f"Invalid start time for {day}: {error_msg}",
                             status_code=400
                         )
 
@@ -378,7 +378,7 @@ def save_dim_schedule_config():
                     if not is_valid:
                         return error_response(
                             ErrorCode.VALIDATION_ERROR,
-                            f"Invalid end _pkg.time for {day}: {error_msg}",
+                            f"Invalid end time for {day}: {error_msg}",
                             status_code=400
                         )
 

web_interface/blueprints/api_v3/display.py

@@ -227,7 +227,7 @@ def start_on_demand_display():
         
         # Stop the display service first to ensure clean state when we will restart it
         if service_was_running and start_service:
-            import _pkg.time as time_module
+            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

web_interface/blueprints/api_v3/fonts.py

@@ -168,15 +168,30 @@ def upload_font():
         if font_file.filename == '':
             return jsonify({'status': 'error', 'message': 'No file selected'}), 400
 
-        # Validate filename
+        # Validate filename. validate_file_upload takes max_size_mb but only
+        # checks the filename/extension with it -- it never looks at the
+        # actual upload size, so the size limit below is enforced separately
+        # before the file is saved (same pattern as the .star upload above).
+        MAX_FONT_SIZE_MB = 10
         is_valid, error_msg = validate_file_upload(
             font_file.filename,
-            max_size_mb=10,
+            max_size_mb=MAX_FONT_SIZE_MB,
             allowed_extensions=['.ttf', '.otf', '.bdf']
         )
         if not is_valid:
             return jsonify({'status': 'error', 'message': error_msg}), 400
 
+        # Check file size (stated limit is MAX_FONT_SIZE_MB)
+        font_file.seek(0, 2)  # Seek to end
+        file_size = font_file.tell()
+        font_file.seek(0)  # Reset to beginning
+        max_font_size_bytes = MAX_FONT_SIZE_MB * 1024 * 1024
+        if file_size > max_font_size_bytes:
+            return jsonify({
+                'status': 'error',
+                'message': f'File too large (max {MAX_FONT_SIZE_MB}MB, got {file_size / 1024 / 1024:.1f}MB)'
+            }), 400
+
         font_family = request.form.get('font_family', '')
 
         if not font_family:

web_interface/blueprints/api_v3/starlark.py

@@ -10,9 +10,10 @@ from web_interface.blueprints.api_v3 import (
     _find_pixlet_binary, _install_star_file, _pixlet_editor_alive,
     _pixlet_editor_status, _read_pixlet_editor_state,
     _standalone_render_starlark_app, _starlark_github_token,
-    _validate_and_sanitize_app_id, _validate_starlark_app_path,
-    _validate_timing_value, api_v3, contextlib, describe_exception, json,
-    jsonify, logger, os, request, shutil, signal, subprocess, tempfile,
+    _starlark_manifest_lock, _validate_and_sanitize_app_id,
+    _validate_starlark_app_path, _validate_timing_value, api_v3, contextlib,
+    describe_exception, json, jsonify, logger, os, request, shutil, signal,
+    subprocess, tempfile,
 )
 import web_interface.blueprints.api_v3 as _pkg
 # Read through the module rather than bound by value: tests patch these
@@ -299,9 +300,10 @@ def uninstall_starlark_app(app_id):
             import shutil
             if app_dir.exists():
                 shutil.rmtree(app_dir)
-            manifest = _pkg._read_starlark_manifest()
-            manifest.get('apps', {}).pop(app_id, None)
-            success = _pkg._write_starlark_manifest(manifest)
+            with _starlark_manifest_lock():
+                manifest = _pkg._read_starlark_manifest()
+                manifest.get('apps', {}).pop(app_id, None)
+                success = _pkg._write_starlark_manifest(manifest)
 
         if success:
             return jsonify({'status': 'success', 'message': f'App uninstalled: {app_id}'})
@@ -435,52 +437,70 @@ def update_starlark_app_config(app_id):
             else:
                 return jsonify({'status': 'error', 'message': 'Failed to save configuration'}), 500
 
-        # Standalone: update both config.json and manifest
-        manifest = _pkg._read_starlark_manifest()
-        app_data = manifest.get('apps', {}).get(app_id)
-        if not app_data:
-            return jsonify({'status': 'error', 'message': f'App not found: {app_id}'}), 404
+        # Standalone: update both config.json and manifest. Both live under
+        # the manifest lock (serialized against every other standalone
+        # manifest read-modify-write -- see _starlark_manifest_lock), and if
+        # the manifest write fails after config.json was already written,
+        # config.json is rolled back so the two do not end up out of sync.
+        with _starlark_manifest_lock():
+            manifest = _pkg._read_starlark_manifest()
+            app_data = manifest.get('apps', {}).get(app_id)
+            if not app_data:
+                return jsonify({'status': 'error', 'message': f'App not found: {app_id}'}), 404
 
-        # Extract timing keys (they go in manifest, not config.json)
-        render_interval = data.pop('render_interval', None)
-        display_duration = data.pop('display_duration', None)
+            # Extract timing keys (they go in manifest, not config.json)
+            render_interval = data.pop('render_interval', None)
+            display_duration = data.pop('display_duration', None)
 
-        # Update manifest with timing values
-        if render_interval is not None:
-            app_data['render_interval'] = render_interval
-        if display_duration is not None:
-            app_data['display_duration'] = display_duration
+            # Update manifest with timing values
+            if render_interval is not None:
+                app_data['render_interval'] = render_interval
+            if display_duration is not None:
+                app_data['display_duration'] = display_duration
 
-        # Load current config from config.json. app_dir is the path
-        # _validate_starlark_app_path checked, not a fresh join.
-        config_file = app_dir / "config.json"
-        current_config = {}
-        if config_file.exists():
+            # Load current config from config.json. app_dir is the path
+            # _validate_starlark_app_path checked, not a fresh join.
+            config_file = app_dir / "config.json"
+            existed_before = config_file.exists()
+            previous_config_bytes = None
+            current_config = {}
+            if existed_before:
+                try:
+                    previous_config_bytes = config_file.read_bytes()
+                    current_config = json.loads(previous_config_bytes)
+                except Exception as e:
+                    logger.warning(f"Failed to load config for {app_id}: {e}")
+
+            # Update config with new values (excluding timing keys)
+            current_config.update(data)
+
+            # Write updated config to config.json
             try:
-                with open(config_file, 'r') as f:
-                    current_config = json.load(f)
+                with open(config_file, 'w') as f:
+                    json.dump(current_config, f, indent=2)
             except Exception as e:
-                logger.warning(f"Failed to load config for {app_id}: {e}")
+                logger.error(f"Failed to save config.json for {app_id}: {e}")
+                logger.exception("Failed to save Starlark configuration for %r", app_id)
+                return jsonify({'status': 'error', 'message': 'Failed to save configuration',
+                                'details': describe_exception(e)}), 500
 
-        # Update config with new values (excluding timing keys)
-        current_config.update(data)
+            # Also update manifest for backward compatibility
+            app_data.setdefault('config', {}).update(data)
 
-        # Write updated config to config.json
-        try:
-            with open(config_file, 'w') as f:
-                json.dump(current_config, f, indent=2)
-        except Exception as e:
-            logger.error(f"Failed to save config.json for {app_id}: {e}")
-            logger.exception("Failed to save Starlark configuration for %r", app_id)
-            return jsonify({'status': 'error', 'message': 'Failed to save configuration',
-                            'details': describe_exception(e)}), 500
+            if _pkg._write_starlark_manifest(manifest):
+                return jsonify({'status': 'success', 'message': 'Configuration updated', 'config': current_config})
 
-        # Also update manifest for backward compatibility
-        app_data.setdefault('config', {}).update(data)
-
-        if _pkg._write_starlark_manifest(manifest):
-            return jsonify({'status': 'success', 'message': 'Configuration updated', 'config': current_config})
-        else:
+            # The manifest write failed after config.json was already
+            # written -- roll config.json back rather than leave the two
+            # disagreeing about what was saved.
+            try:
+                if existed_before and previous_config_bytes is not None:
+                    config_file.write_bytes(previous_config_bytes)
+                elif not existed_before:
+                    config_file.unlink(missing_ok=True)
+            except OSError:
+                logger.exception(
+                    "Failed to roll back config.json for %r after manifest write failure", app_id)
             return jsonify({'status': 'error', 'message': 'Failed to save manifest'}), 500
 
     except Exception as e:
@@ -508,19 +528,20 @@ def toggle_starlark_app(app_id):
             return jsonify({'status': 'success', 'message': f"App {'enabled' if enabled else 'disabled'}", 'enabled': enabled})
 
         # Standalone: update manifest directly
-        manifest = _pkg._read_starlark_manifest()
-        app_data = manifest.get('apps', {}).get(app_id)
-        if not app_data:
-            return jsonify({'status': 'error', 'message': f'App not found: {app_id}'}), 404
+        with _starlark_manifest_lock():
+            manifest = _pkg._read_starlark_manifest()
+            app_data = manifest.get('apps', {}).get(app_id)
+            if not app_data:
+                return jsonify({'status': 'error', 'message': f'App not found: {app_id}'}), 404
 
-        enabled = data.get('enabled')
-        if enabled is None:
-            enabled = not app_data.get('enabled', True)
-        app_data['enabled'] = enabled
-        if _pkg._write_starlark_manifest(manifest):
-            return jsonify({'status': 'success', 'message': f"App {'enabled' if enabled else 'disabled'}", 'enabled': enabled})
-        else:
-            return jsonify({'status': 'error', 'message': 'Failed to save'}), 500
+            enabled = data.get('enabled')
+            if enabled is None:
+                enabled = not app_data.get('enabled', True)
+            app_data['enabled'] = enabled
+            if _pkg._write_starlark_manifest(manifest):
+                return jsonify({'status': 'success', 'message': f"App {'enabled' if enabled else 'disabled'}", 'enabled': enabled})
+            else:
+                return jsonify({'status': 'error', 'message': 'Failed to save'}), 500
 
     except Exception as e:
         logger.exception("[Starlark] toggle_starlark_app failed")

web_interface/blueprints/api_v3/wifi.py

@@ -8,6 +8,36 @@ from web_interface.blueprints.api_v3 import (
 )
 
 
+def _parse_bool_ish(value, default=False):
+    """Coerce a JSON value that is supposed to be a boolean.
+
+    A JSON boolean arrives as a real Python bool, but these routes are a
+    public HTTP contract and not every caller sends one. `bool(value)` gets
+    two common cases wrong: `bool("false")` is True (a non-empty string is
+    always truthy), and a plain int does not match an `is True` check
+    (`1 is True` is False, since `True` is a distinct singleton from the int
+    `1`) -- so a caller sending `{"enabled": 1}` was silently treated as
+    False. Recognizes a real bool, "true"/"false"/"1"/"0"/"yes"/"no"
+    case-insensitively, and int 1/0; anything else falls back to `default`.
+    """
+    if isinstance(value, bool):
+        return value
+    if isinstance(value, str):
+        lowered = value.strip().lower()
+        if lowered in ('true', '1', 'yes'):
+            return True
+        if lowered in ('false', '0', 'no'):
+            return False
+        return default
+    if isinstance(value, int):
+        if value == 1:
+            return True
+        if value == 0:
+            return False
+        return default
+    return default
+
+
 # WiFi Management Endpoints
 @api_v3.route('/wifi/status', methods=['GET'])
 def get_wifi_status():
@@ -273,7 +303,7 @@ def set_auto_enable_ap_mode():
                 'message': 'auto_enable_ap_mode is required'
             }), 400
 
-        auto_enable = bool(data['auto_enable_ap_mode'])
+        auto_enable = _parse_bool_ish(data['auto_enable_ap_mode'], default=False)
 
         wifi_manager = WiFiManager()
         wifi_manager.config["auto_enable_ap_mode"] = auto_enable
@@ -330,13 +360,12 @@ def set_wifi_radio():
                 'message': 'enabled is required'
             }), 400
 
-        # Parse defensively: bool("false") is True, so mirror the string-aware
-        # coercion used for `force` — the endpoint is a public contract, not just
-        # the shipped UI (which always sends real JSON booleans).
-        _enabled_raw = data['enabled']
-        enabled = _enabled_raw is True or (isinstance(_enabled_raw, str) and _enabled_raw.lower() in ('true', '1', 'yes'))
-        _force_raw = data.get('force', False)
-        force = _force_raw is True or (isinstance(_force_raw, str) and _force_raw.lower() in ('true', '1', 'yes'))
+        # Parse defensively: bool("false") is True and a plain int never
+        # matches `is True`, so `_parse_bool_ish` handles bool, string and
+        # int 1/0 — the endpoint is a public contract, not just the shipped
+        # UI (which always sends real JSON booleans).
+        enabled = _parse_bool_ish(data['enabled'], default=False)
+        force = _parse_bool_ish(data.get('force', False), default=False)
 
         wifi_manager = WiFiManager()
         success, message, reason = wifi_manager.set_wifi_radio(enabled, force=force)