fix(security): refuse unsafe plugin ids, keep secrets private, validate request bodies (#643)

* fix(security): refuse unsafe plugin ids, keep secrets private, validate bodies - install_from_url and the registry install's manifest rename refuse a plugin id that is not a single safe name (no ../ out of plugins_dir). - Uninstall and config reset refuse core config sections and ids with path parts; uninstall of a plugin whose directory is gone still works. - separate_secrets checks a field's own x-secret marker before recursing, so object/array secrets no longer land in config.json. - Backup restore creates missing secrets/wifi/ytm files with mode 640; export skips non-object manifests and no longer collides on same-second exports. - SYSTEM_FONTS includes every bundled font from BUNDLED_FONTS. - Raw config/secrets saves and validate_request_json require a JSON object. - A blank max_dynamic_duration_seconds keeps the stored value; other values are validated to 30-1800 instead of raising a 500. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(security): validate the id before install_plugin moves anything; claim backup names atomically - install_plugin set aside plugins_dir / plugin_id before any id check, so "../x" moved a directory outside the plugins dir (the rollback moved it back, but only if the install path got that far) - two exports finishing in the same second could both see a free name and the later os.replace destroyed the first archive; the name is now claimed with O_EXCL before the archive is swapped in Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-04 14:25:08 +00:00 · 2026-09-28 08:24:43 -04:00
parent da5937da3d
commit bcef1957a9
15 changed files with 508 additions and 13 deletions
@@ -0,0 +1,56 @@
+"""DELETE /fonts/<name> must refuse every font the repository ships.
+
+The api's SYSTEM_FONTS was a hand-written list that had drifted from
+backup_manager.BUNDLED_FONTS: MatrixChunky8X, MatrixLight6X, MatrixLight8X and
+ic8x8u were missing, so deleting them removed git-tracked files (and the next
+`git pull` either restored them or conflicted).
+"""
+
+import os
+import sys
+from pathlib import Path
+from unittest.mock import patch
+
+import pytest
+
+sys.path.insert(0, str(Path(__file__).parent.parent))
+
+from src.backup_manager import BUNDLED_FONTS  # noqa: E402
+from test._api_v3_test_helpers import api_v3_client, api_v3_module  # noqa: F401,E402
+from web_interface.blueprints.api_v3 import SYSTEM_FONTS  # noqa: E402
+
+SHIPPED_FONT_FILES = sorted(
+    name for name in BUNDLED_FONTS if name.lower().endswith(('.ttf', '.otf', '.bdf')))
+
+
+def test_every_bundled_font_is_a_system_font():
+    stems = {os.path.splitext(name)[0].lower() for name in SHIPPED_FONT_FILES}
+    assert stems <= SYSTEM_FONTS, stems - SYSTEM_FONTS
+
+
+@pytest.fixture
+def fonts_root(tmp_path):
+    fonts = tmp_path / "assets" / "fonts"
+    fonts.mkdir(parents=True)
+    with patch("web_interface.blueprints.api_v3.fonts.PROJECT_ROOT", tmp_path):
+        yield fonts
+
+
+@pytest.mark.parametrize("filename", ["MatrixChunky8X.bdf", "MatrixLight6X.bdf",
+                                      "MatrixLight8X.bdf", "ic8x8u.bdf"])
+def test_the_previously_unprotected_fonts_cannot_be_deleted(api_v3_client, fonts_root, filename):
+    (fonts_root / filename).write_text("BUNDLED")
+
+    response = api_v3_client.delete(f"/api/v3/fonts/{Path(filename).stem}")
+
+    assert response.status_code == 403
+    assert (fonts_root / filename).exists()
+
+
+def test_a_user_font_can_still_be_deleted(api_v3_client, fonts_root):
+    (fonts_root / "my-upload.ttf").write_bytes(b"USER")
+
+    response = api_v3_client.delete("/api/v3/fonts/my-upload")
+
+    assert response.status_code == 200, response.get_json()
+    assert not (fonts_root / "my-upload.ttf").exists()
@@ -414,3 +414,28 @@ def test_pi5_form_warns_about_a_stored_unsupported_row_address_type(display_page
    board(PI5_MODEL)
    body = display_page(_config_with(hardware={'row_address_type': 5}))
    assert "Your saved row address type (5) can't be used on this Raspberry Pi 5" in body
+
+
+@pytest.mark.parametrize('value,stored', [(180, 180), ('600', 600), (30, 30), ('1800', 1800)])
+def test_max_dynamic_duration_in_range_is_saved(api_v3_client, saved, value, stored):
+    response = _post(api_v3_client, {'max_dynamic_duration_seconds': value})
+    assert response.status_code == 200, response.get_data(as_text=True)[:200]
+    assert saved['config']['display']['dynamic_duration']['max_duration_seconds'] == stored
+
+
+@pytest.mark.parametrize('value', ['', '  ', None])
+def test_a_blank_max_dynamic_duration_keeps_the_stored_cap(api_v3_client, api_v3_module, saved, value):
+    """A cleared box posts "": int("") was a 500 that lost the whole Display save."""
+    api_v3_module.api_v3.config_manager.load_config.return_value = {
+        'display': {'dynamic_duration': {'max_duration_seconds': 240}}}
+    response = _post(api_v3_client, {'max_dynamic_duration_seconds': value, 'brightness': 50})
+    assert response.status_code == 200, response.get_data(as_text=True)[:200]
+    assert saved['config']['display']['dynamic_duration']['max_duration_seconds'] == 240
+    assert saved['config']['display']['hardware']['brightness'] == 50
+
+
+@pytest.mark.parametrize('value', ['abc', 29, 1801, '12.5', True])
+def test_an_invalid_max_dynamic_duration_is_a_400(api_v3_client, saved, value):
+    response = _post(api_v3_client, {'max_dynamic_duration_seconds': value})
+    assert response.status_code == 400
+    assert 'config' not in saved
@@ -95,6 +95,31 @@ class TestMissingBodyGivesTheDeclaredError:
        assert response.status_code == 400


+class TestNonObjectBodyIsRefused:
+    """A JSON array parses and is truthy, so it got past "No data provided".
+
+    The raw editors then wrote it over config.json / config_secrets.json, and
+    validate_request_json checked ``field in data`` against a list -- so
+    ``["plugin_id"]`` passed and the handler raised TypeError.
+    """
+
+    @pytest.mark.parametrize("url", [
+        "/api/v3/config/raw/main",
+        "/api/v3/config/raw/secrets",
+    ])
+    def test_raw_config_saves_refuse_an_array(self, api_v3_client, api_v3_module, url):
+        response = api_v3_client.post(url, json=["display", "schedule"])
+
+        assert response.status_code == 400
+        api_v3_module.api_v3.config_manager.save_raw_file_content.assert_not_called()
+
+    def test_validate_request_json_refuses_an_array(self, api_v3_client, api_v3_module):
+        response = api_v3_client.post("/api/v3/plugins/uninstall", json=["plugin_id"])
+
+        assert response.status_code == 400
+        assert "object" in response.get_json()["message"]
+
+
 class TestNoBodyReadContradictsItsOwnGuard:
    PKG = Path(__file__).parent.parent / "web_interface/blueprints/api_v3"

@@ -417,3 +417,93 @@ def test_restore_still_carries_the_previous_owner_across(
    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)}
+
+
+def test_restore_onto_a_fresh_device_keeps_secrets_private(
+    project: Path, empty_project: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    """With no existing file to take a mode from, the restored file took the
+    extracted temp file's umask mode -- 0o644, so config_secrets.json,
+    wifi_config.json and ytm_auth.json came back world-readable.
+
+    Recorded through os.chmod because Windows cannot represent 0o640.
+    """
+    zip_path = create_backup(project, output_dir=tmp_path / "exports")
+    chmods = []
+    real_chmod = os.chmod
+
+    def recording_chmod(path, mode, *args, **kwargs):
+        chmods.append((Path(path).name.lstrip("."), mode))
+        return real_chmod(path, mode, *args, **kwargs)
+
+    monkeypatch.setattr(os, "chmod", recording_chmod)
+
+    result = restore_backup(zip_path, empty_project, RestoreOptions(
+        restore_fonts=False, restore_plugin_uploads=False, reinstall_plugins=False,
+    ))
+
+    assert result.success, result.errors
+    private = {name.split(".json")[0]: mode for name, mode in chmods
+               if name.startswith(("config_secrets.json", "wifi_config.json", "ytm_auth.json"))}
+    assert private == {"config_secrets": 0o640, "wifi_config": 0o640, "ytm_auth": 0o640}
+    assert all(mode != 0o640 for name, mode in chmods if name.startswith("config.json"))
+
+
+def test_a_manifest_that_is_not_an_object_is_skipped(project: Path) -> None:
+    broken = project / "plugin-repos" / "broken"
+    broken.mkdir()
+    (broken / "manifest.json").write_text("[1, 2]", encoding="utf-8")
+
+    ids = [p["plugin_id"] for p in list_installed_plugins(project)]
+
+    assert "my-plugin" in ids and "broken" not in ids
+
+
+def test_same_second_exports_do_not_overwrite_each_other(
+    project: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    from datetime import datetime as real_datetime
+
+    class FrozenDatetime:
+        @staticmethod
+        def now(*a, **k):
+            return real_datetime(2026, 1, 2, 3, 4, 5)
+
+    monkeypatch.setattr(backup_manager, "datetime", FrozenDatetime)
+    out = tmp_path / "exports"
+
+    first = create_backup(project, output_dir=out)
+    second = create_backup(project, output_dir=out)
+
+    assert first != second
+    assert first.exists() and second.exists()
+    assert second.name == first.name[:-len(".zip")] + "-2.zip"
+    assert not list(out.glob("*.tmp"))
+
+
+def test_export_name_is_claimed_atomically(
+    project: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    # Two exports racing both see "no such file" before either publishes.
+    # Simulate that by making exists() always say no: the name must still be
+    # claimed exclusively, so the second export gets -2 instead of replacing
+    # the first archive.
+    from datetime import datetime as real_datetime
+
+    class FrozenDatetime:
+        @staticmethod
+        def now(*a, **k):
+            return real_datetime(2026, 1, 2, 3, 4, 5)
+
+    monkeypatch.setattr(backup_manager, "datetime", FrozenDatetime)
+    out = tmp_path / "exports"
+    first = create_backup(project, output_dir=out)
+    first_bytes = first.read_bytes()
+
+    monkeypatch.setattr(Path, "exists", lambda self: False)
+    second = create_backup(project, output_dir=out)
+    monkeypatch.undo()
+
+    assert second != first
+    assert first.read_bytes() == first_bytes
+    assert zipfile.is_zipfile(second)
@@ -0,0 +1,94 @@
+"""A plugin id from a request or a downloaded manifest cannot escape plugins_dir.
+
+install_from_url joined ``plugins_dir / plugin_id`` -- plugin_id coming from
+the request body or the downloaded manifest -- then removed whatever was
+there and moved the download onto it, so ``"../x"`` deleted and replaced a
+directory beside plugins_dir. _install_plugin_impl did the same when it
+renamed the install to the manifest's id.
+"""
+
+import json
+
+import pytest
+
+from src.plugin_system.store_manager import PluginStoreManager
+
+MANIFEST = {
+    "id": "good-plugin", "name": "Good", "class_name": "P",
+    "display_modes": ["good"], "version": "1.0.0",
+}
+
+
+@pytest.fixture
+def store(tmp_path, monkeypatch):
+    mgr = PluginStoreManager(
+        plugins_dir=str(tmp_path / "plugins"),
+        uninstalled_registry_path=str(tmp_path / "uninstalled.json"))
+    mgr.plugins_dir.mkdir(parents=True, exist_ok=True)
+    monkeypatch.setattr(mgr, "_install_dependencies", lambda *a, **k: True)
+    mgr.manifest = dict(MANIFEST)
+
+    def fake_clone(repo_url, target, branches):
+        target = type(mgr.plugins_dir)(target)
+        target.mkdir(parents=True, exist_ok=True)
+        (target / "manifest.json").write_text(json.dumps(mgr.manifest))
+        (target / "manager.py").write_text("class P: pass\n")
+        return "main"
+
+    monkeypatch.setattr(mgr, "_install_via_git", fake_clone)
+    return mgr
+
+
+@pytest.fixture
+def victim(tmp_path):
+    # A sibling of plugins_dir that a traversal id would target.
+    d = tmp_path / "victim"
+    d.mkdir()
+    (d / "keep.txt").write_text("precious")
+    return d
+
+
+def test_install_from_url_rejects_a_traversal_id_from_the_request(store, victim):
+    result = store.install_from_url("https://github.com/x/y", plugin_id="../victim")
+
+    assert result["success"] is False
+    assert "Invalid plugin ID" in result["error"]
+    assert (victim / "keep.txt").read_text() == "precious"
+
+
+def test_install_from_url_rejects_a_traversal_id_from_the_manifest(store, victim):
+    store.manifest["id"] = "../victim"
+
+    result = store.install_from_url("https://github.com/x/y")
+
+    assert result["success"] is False
+    assert (victim / "keep.txt").read_text() == "precious"
+
+
+def test_install_from_url_still_installs_a_normal_id(store):
+    result = store.install_from_url("https://github.com/x/y", plugin_id="ledmatrix-weather")
+
+    assert result["success"] is True
+    assert (store.plugins_dir / "ledmatrix-weather" / "manifest.json").exists()
+
+
+def test_registry_install_rejects_a_traversal_manifest_id(store, victim, monkeypatch):
+    store.manifest["id"] = "../victim"
+    monkeypatch.setattr(store, "get_plugin_info", lambda *a, **k: {
+        "id": "good-plugin", "repo": "https://github.com/x/y"})
+
+    assert store._install_plugin_impl("good-plugin") is False
+    assert (victim / "keep.txt").read_text() == "precious"
+    assert not (store.plugins_dir / "good-plugin").exists()
+
+
+def test_install_plugin_rejects_a_traversal_id_before_touching_disk(store, victim, monkeypatch):
+    # install_plugin moves an existing plugins_dir / plugin_id aside before
+    # installing; for "../victim" that is a directory outside plugins_dir.
+    # A later rollback may move it back, so assert nothing was touched at all.
+    touched = []
+    monkeypatch.setattr(store, "_set_aside", lambda *a: touched.append("set_aside"))
+    monkeypatch.setattr(store, "_install_plugin_impl", lambda *a, **k: touched.append("install"))
+    assert store.install_plugin("../victim") is False
+    assert touched == []
+    assert (victim / "keep.txt").read_text() == "precious"
@@ -391,5 +391,55 @@ class TestReconcileEndpointPayload(unittest.TestCase):
        self._reconciler_instance.reconcile_state.assert_called_once_with(force=True)


+class TestNonPluginIdsAreRefused(unittest.TestCase):
+    """Uninstall and config reset key config.json by the request's plugin_id.
+
+    ``{"plugin_id": "display"}`` deleted the core display section and
+    answered success; a reset overwrote it with a plugin schema's defaults.
+    """
+
+    def setUp(self):
+        self.client, self.mod, _cleanup = _make_client()
+        self.addCleanup(_cleanup)
+        self.api_v3 = self.mod.api_v3
+        self.api_v3.plugin_manager.plugin_manifests = {'thing': {'id': 'thing'}}
+
+    def _post(self, url, body):
+        return self.client.post(url, data=json.dumps(body),
+                                content_type='application/json')
+
+    def test_uninstall_refuses_core_sections_and_traversal(self):
+        for bad in ('display', 'schedule', 'plugin_system', 'github', '../x', 'a/b', 7):
+            with self.subTest(plugin_id=bad):
+                response = self._post('/api/v3/plugins/uninstall', {'plugin_id': bad})
+                self.assertEqual(response.status_code, 400)
+        self.api_v3.config_manager.cleanup_plugin_config.assert_not_called()
+        self.api_v3.plugin_store_manager.uninstall_plugin.assert_not_called()
+
+    def test_uninstall_still_cleans_a_plugin_whose_directory_is_gone(self):
+        # Not among the discovered manifests, but a plugin-shaped id: the
+        # config cleanup must still run.
+        self.api_v3.plugin_store_manager.uninstall_plugin.return_value = True
+        response = self._post('/api/v3/plugins/uninstall', {'plugin_id': 'gone-plugin'})
+
+        self.assertEqual(response.status_code, 200, response.get_json())
+        self.api_v3.config_manager.cleanup_plugin_config.assert_called_once_with(
+            'gone-plugin', remove_secrets=True)
+
+    def test_secrets_only_core_key_is_allowed_when_a_plugin_has_that_id(self):
+        self.api_v3.plugin_manager.plugin_manifests = {'youtube': {'id': 'youtube'}}
+        self.api_v3.plugin_store_manager.uninstall_plugin.return_value = True
+        response = self._post('/api/v3/plugins/uninstall', {'plugin_id': 'youtube'})
+
+        self.assertEqual(response.status_code, 200, response.get_json())
+
+    def test_reset_refuses_core_sections(self):
+        response = self._post('/api/v3/plugins/config/reset', {'plugin_id': 'display'})
+
+        self.assertEqual(response.status_code, 400)
+        self.api_v3.schema_manager.generate_default_config.assert_not_called()
+        self.api_v3.config_manager.save_raw_file_content.assert_not_called()
+
+
 if __name__ == '__main__':
    unittest.main()
@@ -124,6 +124,23 @@ class TestSeparateSecrets:
        assert regular == {"accounts": [{"name": "a"}, "oddball"]}
        assert secrets == {"accounts": [{"token": "ta"}, {}]}

+    def test_secret_field_holding_an_object_or_array_goes_to_secrets(self):
+        # x-secret on the field itself, not its children: the dict/list type
+        # check used to win, and the whole value landed in config.json.
+        props = {
+            "oauth": {"type": "object", "x-secret": True},
+            "cookies": {"type": "array", "x-secret": True},
+            "city": {"type": "string"},
+        }
+        config = {"oauth": {"refresh": "r3fr3sh"}, "cookies": ["c1", "c2"],
+                  "city": "Austin"}
+        regular, secrets = separate_secrets(config, find_secret_fields(props))
+        assert regular == {"city": "Austin"}
+        assert secrets == {"oauth": {"refresh": "r3fr3sh"}, "cookies": ["c1", "c2"]}
+        # ...which is what the API already masks for those fields.
+        masked = mask_secret_fields(config, props)
+        assert masked["oauth"] == "" and masked["cookies"] == ""
+
    def test_array_without_secret_paths_stays_regular(self):
        config = {"teams": ["DAL", "HOU"]}
        regular, secrets = separate_secrets(config, {"api_key"})