Merge origin/main (#592, #593, #594) into the September 16 core audit fixes

config.py conflict: #594 discovers plugins before /config/main matches plugin
keys; this branch routes those sections through the /plugins/config
preparation (F13). Kept both.

#594's secret-separation test posts a plugin section to /config/main, which
this branch now validates through api_v3.schema_manager; the test gets a real
SchemaManager over its temporary plugins directory instead of the helper's
mock. It still fails if the discovery call is reverted.

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

test/test_api_v3_lazy_plugin_discovery.py

@@ -0,0 +1,165 @@
+"""Tests that api_v3 routes find installed plugins before anything else has.
+
+The web process discovers plugins lazily (web_interface/app.py): nothing scans
+the plugins directory at startup, and plugin_manifests stays empty until some
+endpoint calls discover_plugins(). Routes that consulted plugin_manifests
+without discovering therefore misbehaved for as long as nothing else had run.
+Measured on a rig after restarting ledmatrix-web:
+
+    POST /api/v3/display/on-demand/start {"plugin_id": "ledmatrix-stocks"}
+    -> 404 "Plugin ledmatrix-stocks not found", for over three minutes,
+       until GET /api/v3/plugins/installed happened to discover plugins.
+
+The browser UI loads the plugin list first, so people rarely saw it; API-only
+callers (the Home Assistant MQTT bridge, scripts) saw it after every restart.
+/plugins/toggle answered the same 404, and /config/main was worse: an
+undiscovered plugin section skipped secret separation and wrote its API key
+into config.json in plain text.
+
+A real PluginManager over a temporary plugins directory, so "empty until
+discovered" is the real behaviour rather than a mock's.
+"""
+
+import json
+import sys
+from pathlib import Path
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+sys.path.insert(0, str(Path(__file__).parent.parent))
+
+from src.config_manager import ConfigManager  # noqa: E402
+from src.plugin_system.plugin_manager import PluginManager  # noqa: E402
+from src.plugin_system.schema_manager import SchemaManager  # noqa: E402
+from test._api_v3_test_helpers import api_v3_client, api_v3_module  # noqa: F401,E402
+
+PLUGIN_ID = 'ledmatrix-stocks'
+MODE = 'stocks'
+
+
+def _install(plugins_dir, plugin_id, modes, schema=None):
+    plugin_dir = plugins_dir / plugin_id
+    plugin_dir.mkdir(parents=True)
+    (plugin_dir / 'manifest.json').write_text(json.dumps({
+        'id': plugin_id, 'name': plugin_id, 'version': '1.0.0',
+        'entry_point': 'manager.py', 'class_name': 'Plugin',
+        'display_modes': modes,
+    }))
+    (plugin_dir / 'manager.py').write_text('')
+    if schema is not None:
+        (plugin_dir / 'config_schema.json').write_text(json.dumps(schema))
+
+
+@pytest.fixture
+def plugins_dir(tmp_path):
+    path = tmp_path / 'plugin-repos'
+    path.mkdir()
+    _install(path, PLUGIN_ID, [MODE], schema={
+        'type': 'object',
+        'properties': {
+            'enabled': {'type': 'boolean'},
+            'symbols': {'type': 'string'},
+            'api_key': {'type': 'string', 'x-secret': True},
+        },
+    })
+    return path
+
+
+@pytest.fixture
+def fresh_web_process(api_v3_module, plugins_dir):
+    """The web process right after a restart: nothing discovered yet."""
+    manager = PluginManager(plugins_dir=str(plugins_dir))
+    assert not manager.plugin_manifests
+    api_v3_module.api_v3.plugin_manager = manager
+    return manager
+
+
+@pytest.fixture
+def display_service():
+    """Keep on-demand start away from systemctl and the real cache."""
+    with patch('web_interface.blueprints.api_v3.display._ensure_cache_manager') as cache, \
+         patch('web_interface.blueprints.api_v3.display._get_display_service_status') as status:
+        cache.return_value = MagicMock()
+        status.return_value = {'active': True}
+        yield cache.return_value
+
+
+def _start(client, **body):
+    body.setdefault('start_service', False)
+    return client.post('/api/v3/display/on-demand/start', json=body)
+
+
+class TestOnDemandStart:
+    def test_by_plugin_id(self, api_v3_client, fresh_web_process, display_service):
+        response = _start(api_v3_client, plugin_id=PLUGIN_ID)
+
+        assert response.status_code == 200, response.get_json()
+        request = display_service.set.call_args.args[1]
+        assert (request['plugin_id'], request['mode']) == (PLUGIN_ID, MODE)
+
+    def test_by_mode_alone(self, api_v3_client, fresh_web_process, display_service):
+        response = _start(api_v3_client, mode=MODE)
+
+        assert response.status_code == 200, response.get_json()
+        assert display_service.set.call_args.args[1]['plugin_id'] == PLUGIN_ID
+
+    def test_a_plugin_installed_since_the_last_scan(
+            self, api_v3_client, fresh_web_process, plugins_dir, display_service):
+        fresh_web_process.discover_plugins()
+        _install(plugins_dir, 'ledmatrix-weather', ['weather'])
+
+        assert _start(api_v3_client, plugin_id='ledmatrix-weather').status_code == 200
+        assert _start(api_v3_client, mode=MODE).status_code == 200
+
+    def test_a_mode_installed_since_the_last_scan(
+            self, api_v3_client, fresh_web_process, plugins_dir, display_service):
+        fresh_web_process.discover_plugins()
+        _install(plugins_dir, 'ledmatrix-weather', ['weather'])
+
+        response = _start(api_v3_client, mode='weather')
+
+        assert response.status_code == 200, response.get_json()
+
+    def test_an_unknown_plugin_is_still_not_found(
+            self, api_v3_client, fresh_web_process, display_service):
+        assert _start(api_v3_client, plugin_id='no-such-plugin').status_code == 404
+        assert _start(api_v3_client, mode='no-such-mode').status_code == 404
+        display_service.set.assert_not_called()
+
+
+def test_toggle_finds_the_plugin(api_v3_client, api_v3_module, fresh_web_process):
+    config_manager = api_v3_module.api_v3.config_manager
+    config_manager.load_config.return_value = {PLUGIN_ID: {'enabled': False}}
+    config_manager.save_config_atomic.return_value = MagicMock(
+        status=MagicMock(value='success'))
+
+    response = api_v3_client.post('/api/v3/plugins/toggle',
+                                  json={'plugin_id': PLUGIN_ID, 'enabled': True})
+
+    assert response.status_code == 200, response.get_json()
+    saved = config_manager.save_config_atomic.call_args.args[0]
+    assert saved[PLUGIN_ID]['enabled'] is True
+
+
+def test_main_config_save_keeps_a_plugin_secret_out_of_config_json(
+        api_v3_client, api_v3_module, fresh_web_process, plugins_dir, tmp_path):
+    # /config/main validates a plugin section like POST /plugins/config does,
+    # so it needs a real schema manager rather than the helper's mock.
+    api_v3_module.api_v3.schema_manager = SchemaManager(plugins_dir=plugins_dir)
+    config_file = tmp_path / 'config.json'
+    config_file.write_text('{}')
+    secrets_file = tmp_path / 'config_secrets.json'
+    config_manager = ConfigManager(config_path=str(config_file),
+                                   secrets_path=str(secrets_file))
+    config_manager.template_path = str(tmp_path / 'no-template.json')
+    api_v3_module.api_v3.config_manager = config_manager
+
+    response = api_v3_client.post('/api/v3/config/main', json={
+        PLUGIN_ID: {'symbols': 'AAPL', 'api_key': 's3cret-key'},
+    })
+
+    assert response.status_code == 200, response.get_json()
+    assert 's3cret' not in config_file.read_text()
+    assert json.loads(secrets_file.read_text())[PLUGIN_ID]['api_key'] == 's3cret-key'
+    assert json.loads(config_file.read_text())[PLUGIN_ID]['symbols'] == 'AAPL'

test/test_backup_manager.py

@@ -3,9 +3,12 @@
 from __future__ import annotations
 
 import json
+import os
 import stat
+import sys
 import zipfile
 from pathlib import Path
+from unittest.mock import MagicMock
 
 import pytest
 
@@ -300,6 +303,11 @@ def test_restore_rejects_malicious_zip(empty_project: Path, tmp_path: Path) -> N
     assert any("unsafe" in e.lower() for e in result.errors)
 
 
+@pytest.mark.skipif(
+    sys.platform == "win32",
+    reason="simulates root-owned POSIX files with chmod 0o444, which on Windows sets the "
+    "read-only attribute, and Windows refuses to rename over a read-only file",
+)
 def test_restore_over_a_file_the_user_cannot_write(
     project: Path, empty_project: Path, tmp_path: Path
 ) -> None:
@@ -334,3 +342,53 @@ def test_restore_over_a_file_the_user_cannot_write(
 
     # The destination's mode is preserved rather than widened to the umask.
     assert stat.S_IMODE((empty_project / "config" / "config_secrets.json").stat().st_mode) == 0o444
+
+
+def _existing_config(empty_project: Path) -> None:
+    (empty_project / "config").mkdir(parents=True, exist_ok=True)
+    for name in ("config.json", "config_secrets.json", "wifi_config.json", "ytm_auth.json"):
+        (empty_project / "config" / name).write_text("{}", encoding="utf-8")
+
+
+def test_restore_over_existing_files_without_os_chown(
+    project: Path, empty_project: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    """Restore must work where the OS has no file ownership API (Windows).
+
+    Replacing a file tries to carry its previous owner across with os.chown.
+    That name does not exist on Windows, and the AttributeError is not an
+    OSError, so it escaped every per-section handler: restoring over any
+    existing config aborted the whole restore and left the old files in place.
+    """
+    zip_path = create_backup(project, output_dir=tmp_path / "exports")
+    _existing_config(empty_project)
+    monkeypatch.delattr(os, "chown", raising=False)
+
+    result = restore_backup(zip_path, empty_project, RestoreOptions())
+
+    assert result.success, result.errors
+    for section in ("config", "secrets", "wifi", "ytm_auth"):
+        assert section in result.restored, f"{section} not restored: {result.errors}"
+    restored = json.loads((empty_project / "config" / "config.json").read_text())
+    assert restored["my-plugin"]["favorites"] == ["A", "B"]
+
+
+def test_restore_still_carries_the_previous_owner_across(
+    project: Path, empty_project: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    """Where os.chown exists, the replaced file keeps the old file's owner."""
+    zip_path = create_backup(project, output_dir=tmp_path / "exports")
+    _existing_config(empty_project)
+    target = empty_project / "config" / "config.json"
+    old = target.stat()
+    chown = MagicMock()
+    monkeypatch.setattr(os, "chown", chown, raising=False)
+
+    result = restore_backup(zip_path, empty_project, RestoreOptions(
+        restore_secrets=False, restore_wifi=False,
+        restore_fonts=False, restore_plugin_uploads=False, reinstall_plugins=False,
+    ))
+
+    assert result.success, result.errors
+    owners = {(c.args[1], c.args[2]) for c in chown.call_args_list}
+    assert owners == {(old.st_uid, old.st_gid)}

test/test_cache_shared_group.py

@@ -0,0 +1,267 @@
+"""Tests that the web interface can read what the display service caches.
+
+The display service runs as root and the web interface as the installing user;
+cache files are 0660, so the web interface reads them only through their group.
+The installers made that the directory's group via setgid, but the web unit's
+CacheDirectory= let systemd re-own /var/cache/ledmatrix to the web user and its
+primary group on first start, erasing it. Every file root wrote afterwards was
+root:root 0660:
+
+    WARNING - Permission denied loading cache for display_current_state from
+    /var/cache/ledmatrix/display_current_state.json: [Errno 13] Permission denied
+
+Measured on one rig: 365 unreadable files, and the web UI's display status,
+on-demand state and plugin health all silently empty.
+
+Most of these need POSIX ownership calls, so they skip on Windows. The ones
+that only make sense as root (another user reading, another user's files) skip
+without it; run them with `sudo python3 -m pytest test/test_cache_shared_group.py`.
+"""
+
+import os
+import shutil
+import stat
+import tempfile
+import time
+from pathlib import Path
+
+import pytest
+
+from src.cache import disk_cache as disk_cache_module
+from src.cache.disk_cache import DiskCache
+
+posix_only = pytest.mark.skipif(
+    not hasattr(os, 'fchown') or not hasattr(os, 'geteuid'),
+    reason="needs POSIX file ownership")
+
+IS_ROOT = hasattr(os, 'geteuid') and os.geteuid() == 0
+root_only = pytest.mark.skipif(not IS_ROOT, reason="needs root")
+
+# Ids nothing on a test host is likely to use, for root-only scenarios.
+OTHER_GID = 48213
+OTHER_UID = 48214
+
+
+def _another_group():
+    """A group this process may chown its files to, other than its own."""
+    if IS_ROOT:
+        return OTHER_GID
+    for gid in os.getgroups():
+        if gid != os.getegid():
+            return gid
+    pytest.skip("process belongs to no supplementary group")
+
+
+def _shared_dir(path, gid, mode=0o775):
+    """The rig's broken layout: group-writable, no setgid, a foreign group."""
+    os.chown(path, -1, gid)
+    os.chmod(path, mode)
+    assert not os.stat(path).st_mode & stat.S_ISGID
+    return path
+
+
+@posix_only
+class TestNewFilesTakeTheDirectoryGroup:
+    def test_without_setgid(self, tmp_path):
+        gid = _another_group()
+        cache = DiskCache(str(_shared_dir(tmp_path, gid)))
+
+        cache.set('display_current_state', {'mode': 'clock'})
+
+        st = os.stat(tmp_path / 'display_current_state.json')
+        assert st.st_gid == gid
+        assert stat.S_IMODE(st.st_mode) == 0o660
+
+    def test_the_file_is_never_visible_with_a_narrower_mode(self, tmp_path, monkeypatch):
+        """mkstemp creates 0600; the rename used to publish it before the chmod."""
+        gid = _another_group()
+        cache = DiskCache(str(_shared_dir(tmp_path, gid)))
+        seen = []
+        real_replace = os.replace
+
+        def spy(src, dst):
+            st = os.stat(src)
+            seen.append((stat.S_IMODE(st.st_mode), st.st_gid))
+            return real_replace(src, dst)
+
+        monkeypatch.setattr(disk_cache_module.os, 'replace', spy)
+        cache.set('k', {'v': 1})
+
+        assert seen == [(0o660, gid)]
+
+    def test_a_rewrite_fixes_a_file_left_with_the_wrong_group(self, tmp_path):
+        gid = _another_group()
+        cache = DiskCache(str(_shared_dir(tmp_path, gid)))
+        path = tmp_path / 'k.json'
+        path.write_text('{}')
+        os.chown(path, -1, os.getegid())
+
+        cache.set('k', {'v': 2})
+
+        assert os.stat(path).st_gid == gid
+
+    def test_the_direct_write_fallback_shares_too(self, tmp_path, monkeypatch):
+        gid = _another_group()
+        cache = DiskCache(str(_shared_dir(tmp_path, gid)))
+
+        def no_temp(*args, **kwargs):
+            raise OSError("no temp files")
+
+        monkeypatch.setattr(disk_cache_module.tempfile, 'mkstemp', no_temp)
+        cache.set('k', {'v': 3})
+
+        st = os.stat(tmp_path / 'k.json')
+        assert (stat.S_IMODE(st.st_mode), st.st_gid) == (0o660, gid)
+
+    def test_a_directory_not_shared_with_its_group_is_left_alone(self, tmp_path):
+        gid = _another_group()
+        cache = DiskCache(str(_shared_dir(tmp_path, gid, mode=0o755)))
+
+        cache.set('k', {'v': 4})
+
+        assert os.stat(tmp_path / 'k.json').st_gid == os.getegid()
+
+
+@posix_only
+class TestExistingFilesAreRepaired:
+    def test_a_root_style_file_becomes_readable(self, tmp_path):
+        gid = _another_group()
+        cache = DiskCache(str(_shared_dir(tmp_path, gid)))
+        path = tmp_path / 'display_on_demand_state.json'
+        path.write_text('{}')
+        os.chown(path, -1, os.getegid())
+        os.chmod(path, 0o600)
+
+        assert cache.share_existing_files() == 1
+
+        st = os.stat(path)
+        assert (stat.S_IMODE(st.st_mode), st.st_gid) == (0o660, gid)
+
+    def test_files_already_shared_are_not_counted(self, tmp_path):
+        gid = _another_group()
+        cache = DiskCache(str(_shared_dir(tmp_path, gid)))
+        cache.set('k', {'v': 1})
+
+        assert cache.share_existing_files() == 0
+
+    def test_only_json_files(self, tmp_path):
+        gid = _another_group()
+        cache = DiskCache(str(_shared_dir(tmp_path, gid)))
+        other = tmp_path / 'tile.png'
+        other.write_bytes(b'x')
+        os.chown(other, -1, os.getegid())
+
+        cache.share_existing_files()
+
+        assert os.stat(other).st_gid == os.getegid()
+
+    def test_a_symlink_is_not_followed(self, tmp_path):
+        """The directory is writable by the web user; root must not be aimed elsewhere."""
+        gid = _another_group()
+        cache_dir = tmp_path / 'cache'
+        cache_dir.mkdir()
+        _shared_dir(cache_dir, gid)
+        outside = tmp_path / 'outside'
+        outside.write_text('secret')
+        os.chown(outside, -1, os.getegid())
+        os.chmod(outside, 0o600)
+        (cache_dir / 'evil.json').symlink_to(outside)
+
+        assert DiskCache(str(cache_dir)).share_existing_files() == 0
+
+        st = os.stat(outside)
+        assert (stat.S_IMODE(st.st_mode), st.st_gid) == (0o600, os.getegid())
+
+    def test_a_hard_linked_file_is_skipped(self, tmp_path):
+        gid = _another_group()
+        cache_dir = tmp_path / 'cache'
+        cache_dir.mkdir()
+        _shared_dir(cache_dir, gid)
+        outside = tmp_path / 'outside'
+        outside.write_text('secret')
+        os.chown(outside, -1, os.getegid())
+        os.chmod(outside, 0o600)
+        os.link(outside, cache_dir / 'linked.json')
+
+        assert DiskCache(str(cache_dir)).share_existing_files() == 0
+        assert stat.S_IMODE(os.stat(outside).st_mode) == 0o600
+
+    @root_only
+    def test_another_users_file_is_skipped(self, tmp_path):
+        cache = DiskCache(str(_shared_dir(tmp_path, OTHER_GID)))
+        path = tmp_path / 'theirs.json'
+        path.write_text('{}')
+        os.chown(path, OTHER_UID, 0)
+        os.chmod(path, 0o600)
+
+        assert cache.share_existing_files() == 0
+        assert os.stat(path).st_gid == 0
+
+
+@posix_only
+@root_only
+def test_a_non_root_member_of_the_group_can_read_what_root_wrote():
+    """The rig, end to end: root writes, the web user reads."""
+    # Not tmp_path: pytest's root-owned base directory is 0700, which the
+    # reader could not traverse no matter what the cache file's group was.
+    base = tempfile.mkdtemp()
+    try:
+        _root_writes_web_user_reads(Path(base))
+    finally:
+        shutil.rmtree(base, ignore_errors=True)
+
+
+def _root_writes_web_user_reads(base):
+    os.chmod(base, 0o755)
+    cache_dir = base / 'cache'
+    cache_dir.mkdir()
+    # What systemd's CacheDirectory= left behind: the web user's own group.
+    os.chown(cache_dir, OTHER_UID, OTHER_GID)
+    os.chmod(cache_dir, 0o775)
+    DiskCache(str(cache_dir)).set('display_current_state', {'mode': 'clock'})
+
+    pid = os.fork()
+    if pid == 0:  # pragma: no cover - child
+        code = 1
+        try:
+            os.setgroups([])
+            os.setgid(OTHER_GID)
+            os.setuid(OTHER_UID)
+            with open(cache_dir / 'display_current_state.json', 'rb') as f:
+                code = 0 if b'clock' in f.read() else 2
+        except PermissionError:
+            code = 13
+        finally:
+            os._exit(code)
+    _, status = os.waitpid(pid, 0)
+    assert os.WEXITSTATUS(status) == 0, "web user could not read the display's cache file"
+
+
+def test_the_web_unit_does_not_let_systemd_re_own_the_cache():
+    template = Path(__file__).resolve().parent.parent / 'systemd' / 'ledmatrix-web.service'
+    directives = [line.strip() for line in template.read_text(encoding='utf-8').splitlines()
+                  if line.strip() and not line.strip().startswith('#')]
+    offending = [d for d in directives if d.startswith(('CacheDirectory', 'StateDirectory'))]
+    assert not offending, (
+        f"{offending}: systemd re-owns that directory to the web user, and the "
+        "display service (root) shares it")
+
+
+def test_the_cleanup_thread_repairs_existing_files(tmp_path, monkeypatch):
+    from src.cache_manager import CacheManager
+
+    calls = []
+    monkeypatch.setattr(CacheManager, '_get_writable_cache_dir', lambda self: str(tmp_path))
+    monkeypatch.setattr(DiskCache, 'share_existing_files',
+                        lambda self: calls.append(self.cache_dir) or 0)
+    CacheManager._cleanup_owners.clear()
+    manager = CacheManager()
+    try:
+        deadline = time.monotonic() + 5
+        while not calls and time.monotonic() < deadline:
+            time.sleep(0.01)
+    finally:
+        manager.stop_cleanup_thread()
+        CacheManager._cleanup_owners.clear()
+
+    assert calls == [str(tmp_path)]

test/test_web_display_state_freshness.py

@@ -0,0 +1,62 @@
+"""Tests that the web UI reports the display's state as it is now.
+
+The display service writes display_on_demand_state and display_current_state;
+the web interface is a different process and can only see them on disk. Its
+reads went through the cache's memory tier, which keeps the first copy read
+for the full max_age (120s). Measured on a rig once the web interface could
+read these files at all: /display/on-demand/status said "active" for over 100
+seconds while the file on disk had said "idle" the whole time.
+"""
+
+import json
+
+import pytest
+from flask import Flask
+
+import web_interface.blueprints.api_v3 as api_pkg
+from src.cache_manager import CacheManager
+
+
+@pytest.fixture
+def two_processes(tmp_path, monkeypatch):
+    """A display-side and a web-side cache over one directory, as on a rig."""
+    monkeypatch.setattr(CacheManager, '_get_writable_cache_dir', lambda self: str(tmp_path))
+    monkeypatch.setattr(CacheManager, 'start_cleanup_thread', lambda self: None)
+    display, web = CacheManager(), CacheManager()
+    monkeypatch.setattr(api_pkg, 'cache_manager', web)
+    monkeypatch.setattr(api_pkg, '_get_display_service_status', lambda: {'active': True})
+    return display
+
+
+@pytest.fixture
+def client():
+    app = Flask(__name__)
+    app.config['TESTING'] = True
+    if 'api_v3' not in app.blueprints:
+        app.register_blueprint(api_pkg.api_v3, url_prefix='/api/v3')
+    with app.test_client() as c:
+        yield c
+
+
+def _get(client, url):
+    response = client.get(url)
+    assert response.status_code == 200
+    return json.loads(response.data)['data']
+
+
+def test_on_demand_status_sees_the_stop_immediately(client, two_processes):
+    two_processes.set('display_on_demand_state', {'active': True, 'status': 'active'})
+    assert _get(client, '/api/v3/display/on-demand/status')['state']['status'] == 'active'
+
+    two_processes.set('display_on_demand_state', {'active': False, 'status': 'idle'})
+
+    assert _get(client, '/api/v3/display/on-demand/status')['state']['status'] == 'idle'
+
+
+def test_current_status_sees_the_mode_change_immediately(client, two_processes):
+    two_processes.set('display_current_state', {'mode': 'odds_ticker'})
+    assert _get(client, '/api/v3/display/current-status')['mode'] == 'odds_ticker'
+
+    two_processes.set('display_current_state', {'mode': 'stocks'})
+
+    assert _get(client, '/api/v3/display/current-status')['mode'] == 'stocks'