From bcef1957a9a373dca0efedd0b5176a23cea48972 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Mon, 28 Sep 2026 08:24:43 -0400 Subject: [PATCH] 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 * 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 --------- Co-authored-by: Claude Opus 5.5 --- CHANGELOG.md | 10 ++ src/backup_manager.py | 43 ++++++++- src/plugin_system/store_manager.py | 22 +++++ src/web_interface/api_helpers.py | 12 ++- src/web_interface/secret_helpers.py | 11 ++- test/test_api_v3_bundled_fonts_protected.py | 56 +++++++++++ test/test_api_v3_display_hardware.py | 25 +++++ test/test_api_v3_optional_body.py | 25 +++++ test/test_backup_manager.py | 90 ++++++++++++++++++ test/test_store_plugin_id_traversal.py | 94 +++++++++++++++++++ test/test_uninstall_and_reconcile_endpoint.py | 50 ++++++++++ test/web_interface/test_secret_helpers.py | 17 ++++ web_interface/blueprints/api_v3/__init__.py | 35 ++++++- web_interface/blueprints/api_v3/config.py | 23 ++++- web_interface/blueprints/api_v3/plugins.py | 8 +- 15 files changed, 508 insertions(+), 13 deletions(-) create mode 100644 test/test_api_v3_bundled_fonts_protected.py create mode 100644 test/test_store_plugin_id_traversal.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 68713d0c..b9efc067 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,16 @@ accepts both, but the store flags the old spelling as deprecated ## Unreleased +- Security and input-validation fixes: + - Installing from a URL (and a registry install whose manifest renames the plugin) refuses a plugin id that isn't a single safe name, so `../x` can no longer delete and replace a directory outside the plugins directory. + - Plugin uninstall and config reset refuse core config sections (`display`, `schedule`, ...) and ids with path parts. Uninstall still cleans the config of a plugin whose directory is already gone. + - A config field marked `x-secret` whose value is an object or array is saved to `config_secrets.json`, not to `config.json` in plain text. + - Restoring a backup onto a device without `config_secrets.json`, `wifi_config.json` or `ytm_auth.json` creates them with mode 640 instead of world-readable 644. + - Backup export skips a plugin `manifest.json` that isn't a JSON object instead of failing, and two exports in the same second no longer share a temp file or overwrite each other (the second gets a `-2` suffix). + - Every font that ships in `assets/fonts/` is protected from deletion; `MatrixChunky8X`, `MatrixLight6X`, `MatrixLight8X` and `ic8x8u` could be deleted from the Fonts tab. + - The raw config and secrets editors, and endpoints using `validate_request_json`, answer 400 for a JSON body that isn't an object. + - A blank Max Dynamic Duration keeps the stored value instead of failing the Display save with a 500; other values must be whole seconds from 30 to 1800. + - Fixes found testing on a Pi: - Stopping `ledmatrix.service` runs the controller's cleanup (SIGTERM now takes the Ctrl-C path). - The Logs tab's "Now showing" no longer reads "unknown" when one screen stays up longer than 2 minutes. diff --git a/src/backup_manager.py b/src/backup_manager.py index 490db4dd..895ac886 100644 --- a/src/backup_manager.py +++ b/src/backup_manager.py @@ -102,6 +102,12 @@ _SINGLE_FILE_SECTIONS: Tuple[Tuple[str, Path, str], ...] = ( ("ytm_auth", _YTM_REL, "restore_wifi"), ) +#: Sections holding credentials. Restored onto a device that has no copy yet, +#: they would otherwise take the extracted temp file's umask mode (0o644, +#: world-readable); 0o640 matches what config_manager_atomic gives secrets. +_PRIVATE_SECTION_RELS = frozenset({_SECRETS_REL, _WIFI_REL, _YTM_REL}) +_PRIVATE_FILE_MODE = 0o640 + MANIFEST_NAME = "manifest.json" PLUGINS_MANIFEST_NAME = "plugins.json" @@ -235,6 +241,10 @@ def list_installed_plugins(project_root: Path) -> List[Dict[str, Any]]: data = json.load(f) except (OSError, json.JSONDecodeError): continue + # Valid JSON that is not an object (a list, a bare string) would + # raise AttributeError on .get() and abort the whole export. + if not isinstance(data, dict): + continue plugin_id = data.get("id") or entry.name if plugin_id not in plugins: plugins[plugin_id] = { @@ -310,7 +320,11 @@ def create_backup( contents: List[str] = [] # Stream directly to a temp file so we never hold the whole ZIP in memory. - tmp_path = zip_path.with_suffix(".zip.tmp") + # The name is unique per call: a fixed ".tmp" was shared by two + # exports started in the same second, which then wrote the same file. + fd, tmp_name = tempfile.mkstemp(dir=str(output_dir), prefix=f".{zip_name}.", suffix=".tmp") + os.close(fd) + tmp_path = Path(tmp_name) try: with zipfile.ZipFile(tmp_path, "w", compression=zipfile.ZIP_DEFLATED) as zf: for section, rel, _flag in _SINGLE_FILE_SECTIONS: @@ -347,7 +361,24 @@ def create_backup( manifest = _build_manifest(contents) zf.writestr(MANIFEST_NAME, json.dumps(manifest, indent=2)) - os.replace(tmp_path, zip_path) + # Same-second exports share a timestamp; number the later one rather + # than replacing the backup the first one just returned. The name is + # claimed with an exclusive create (O_EXCL fails if it exists), so two + # exports finishing together can't both pick the same free name; the + # replace then swaps the finished archive in over our own placeholder. + suffix = 2 + while True: + try: + os.close(os.open(zip_path, os.O_CREAT | os.O_EXCL | os.O_WRONLY, 0o600)) + break + except FileExistsError: + zip_path = output_dir / f"{Path(zip_name).stem}-{suffix}.zip" + suffix += 1 + try: + os.replace(tmp_path, zip_path) + except BaseException: + zip_path.unlink(missing_ok=True) + raise except Exception: tmp_path.unlink(missing_ok=True) raise @@ -493,7 +524,7 @@ def _extract_zip_safe(zip_path: Path, dest_dir: Path) -> None: shutil.copyfileobj(src, dst, length=64 * 1024) -def _copy_file(src: Path, dst: Path) -> None: +def _copy_file(src: Path, dst: Path, new_mode: Optional[int] = None) -> None: """Replace ``dst`` with ``src``, atomically, without needing to own ``dst``. ``shutil.copy2`` opens the destination for writing, so it needs write @@ -509,6 +540,7 @@ def _copy_file(src: Path, dst: Path) -> None: The destination's existing mode is preserved when there is one, so restoring secrets does not silently widen them to the umask default. + When there is none, ``new_mode`` (if given) is used instead of ``src``'s. """ dst.parent.mkdir(parents=True, exist_ok=True) @@ -530,6 +562,8 @@ def _copy_file(src: Path, dst: Path) -> None: shutil.copyfile(src, tmp_path) if existing_mode is not None: os.chmod(tmp_path, existing_mode) + elif new_mode is not None: + os.chmod(tmp_path, new_mode) else: shutil.copymode(src, tmp_path) if existing_owner is not None and hasattr(os, 'chown'): @@ -593,7 +627,8 @@ def restore_backup( result.skipped.append(section) continue try: - _copy_file(tmp_dir / rel, project_root / rel) + _copy_file(tmp_dir / rel, project_root / rel, + new_mode=_PRIVATE_FILE_MODE if rel in _PRIVATE_SECTION_RELS else None) result.restored.append(section) except OSError as e: logger.error("[Backup] Failed to restore %s: %s", rel.name, e, exc_info=True) diff --git a/src/plugin_system/store_manager.py b/src/plugin_system/store_manager.py index 33f8ec86..47f64842 100644 --- a/src/plugin_system/store_manager.py +++ b/src/plugin_system/store_manager.py @@ -1178,6 +1178,13 @@ class PluginStoreManager: other's freshly installed copy. The lock is reentrant because the rollback path already holds it when it calls in here. """ + # Before anything touches the filesystem: plugin_id comes from the + # request body, and the set-aside below moves plugins_dir / plugin_id + # -- which for "../x" is a directory outside the plugins directory. + if not self._is_valid_plugin_id(plugin_id): + self.logger.error(f"Refusing to install invalid plugin id: {plugin_id!r}") + return False + with self._get_reinstall_lock(plugin_id): plugin_path = self.plugins_dir / plugin_id if not plugin_path.exists(): @@ -1345,6 +1352,13 @@ class PluginStoreManager: self.logger.error("Plugin manifest missing 'id' field") self._safe_remove_directory(plugin_path) return False + # The manifest id becomes a directory name below (and the old + # directory is removed to make room), so a downloaded manifest + # saying "../x" must not steer that outside plugins_dir. + if not self._is_valid_plugin_id(manifest_plugin_id): + self.logger.error(f"Plugin manifest has an invalid 'id': {manifest_plugin_id!r}") + self._safe_remove_directory(plugin_path) + return False # If manifest ID doesn't match directory name, rename directory to match manifest if manifest_plugin_id != plugin_id: @@ -1524,6 +1538,14 @@ class PluginStoreManager: 'success': False, 'error': 'No plugin ID found in manifest' } + # plugin_id names the directory that is removed and then replaced + # below, and it comes from the request body or a downloaded + # manifest -- so "../x" would reach outside plugins_dir. + if not self._is_valid_plugin_id(plugin_id): + return { + 'success': False, + 'error': f'Invalid plugin ID: {plugin_id!r}' + } # Validate manifest has required fields required_fields = ['id', 'name', 'class_name', 'display_modes'] diff --git a/src/web_interface/api_helpers.py b/src/web_interface/api_helpers.py index dc92b249..c3bba7f2 100644 --- a/src/web_interface/api_helpers.py +++ b/src/web_interface/api_helpers.py @@ -129,7 +129,17 @@ def validate_request_json(required_fields: list, data: Optional[Dict] = None) -> "Request body must be valid JSON", status_code=400 ) - + + # A JSON array passes the check above, and ``field in data`` then tests + # list membership: ["plugin_id"] "had" every required field and the + # handler's data['plugin_id'] raised TypeError -- a 500, not a 400. + if not isinstance(data, dict): + return None, error_response( + ErrorCode.INVALID_INPUT, + "Request body must be a JSON object", + status_code=400 + ) + missing_fields = [field for field in required_fields if field not in data] if missing_fields: return None, error_response( diff --git a/src/web_interface/secret_helpers.py b/src/web_interface/secret_helpers.py index 9aa2eccc..2680591c 100644 --- a/src/web_interface/secret_helpers.py +++ b/src/web_interface/secret_helpers.py @@ -64,7 +64,14 @@ def separate_secrets( secrets: Dict[str, Any] = {} for key, value in config.items(): full_path = f"{prefix}.{key}" if prefix else key - if isinstance(value, dict): + # The field's own x-secret marker is checked before its type. A secret + # whose value is an object or array used to fall into the recursion + # below, where none of its children are marked, and was written to + # config.json in plain text. mask_secret_fields already checks the + # marker first, so this also matches what the API masks. + if full_path in secret_paths: + secrets[key] = value + elif isinstance(value, dict): nested_regular, nested_secrets = separate_secrets(value, secret_paths, full_path) if nested_regular: regular[key] = nested_regular @@ -94,8 +101,6 @@ def separate_secrets( secrets[key] = sec_items else: regular[key] = value - elif full_path in secret_paths: - secrets[key] = value else: regular[key] = value return regular, secrets diff --git a/test/test_api_v3_bundled_fonts_protected.py b/test/test_api_v3_bundled_fonts_protected.py new file mode 100644 index 00000000..07ac6ee2 --- /dev/null +++ b/test/test_api_v3_bundled_fonts_protected.py @@ -0,0 +1,56 @@ +"""DELETE /fonts/ 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() diff --git a/test/test_api_v3_display_hardware.py b/test/test_api_v3_display_hardware.py index 6e31bc81..245df46c 100644 --- a/test/test_api_v3_display_hardware.py +++ b/test/test_api_v3_display_hardware.py @@ -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 diff --git a/test/test_api_v3_optional_body.py b/test/test_api_v3_optional_body.py index 921538c1..4950b880 100644 --- a/test/test_api_v3_optional_body.py +++ b/test/test_api_v3_optional_body.py @@ -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" diff --git a/test/test_backup_manager.py b/test/test_backup_manager.py index 0097c213..e468990e 100644 --- a/test/test_backup_manager.py +++ b/test/test_backup_manager.py @@ -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) diff --git a/test/test_store_plugin_id_traversal.py b/test/test_store_plugin_id_traversal.py new file mode 100644 index 00000000..0e507a69 --- /dev/null +++ b/test/test_store_plugin_id_traversal.py @@ -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" diff --git a/test/test_uninstall_and_reconcile_endpoint.py b/test/test_uninstall_and_reconcile_endpoint.py index 34d2021b..27494e28 100644 --- a/test/test_uninstall_and_reconcile_endpoint.py +++ b/test/test_uninstall_and_reconcile_endpoint.py @@ -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() diff --git a/test/web_interface/test_secret_helpers.py b/test/web_interface/test_secret_helpers.py index a9eb4a7a..9230c7fe 100644 --- a/test/web_interface/test_secret_helpers.py +++ b/test/web_interface/test_secret_helpers.py @@ -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"}) diff --git a/web_interface/blueprints/api_v3/__init__.py b/web_interface/blueprints/api_v3/__init__.py index 46eade4f..abf0397e 100644 --- a/web_interface/blueprints/api_v3/__init__.py +++ b/web_interface/blueprints/api_v3/__init__.py @@ -50,7 +50,9 @@ from src.web_interface.validators import ( validate_file_upload ) from src.common.permission_utils import install_requirements_file -from src.common.path_safety import resolve_under +from src.common.path_safety import resolve_under, safe_path_component +from src.core_config_keys import CORE_CONFIG_KEYS, CORE_SECRETS_KEYS +from src.backup_manager import BUNDLED_FONTS as _BUNDLED_FONTS from src.device_location import DeviceLocationResolver, apply_device_location _SUDO = shutil.which('sudo') _JOURNALCTL = shutil.which('journalctl') @@ -112,7 +114,15 @@ SYSTEM_FONTS = frozenset([ '10x20', 'matrixchunky8', 'matrixlight6', 'tom-thumb', 'clr6x12', 'helvr12', 'texgyre-27' -]) +]) | frozenset( + # Every font the repository ships, from the list backups already keep in + # sync with assets/fonts/. The hand-written names above had drifted from + # it (MatrixChunky8X, MatrixLight6X, MatrixLight8X and ic8x8u were + # missing), so DELETE /fonts/ removed git-tracked fonts. Keys are + # the lowercased file stem, which is what the catalog and delete compare. + os.path.splitext(_name)[0].lower() for _name in _BUNDLED_FONTS + if _name.lower().endswith(('.ttf', '.otf', '.bdf')) +) api_v3 = Blueprint('api_v3', __name__) @@ -592,6 +602,27 @@ def _installed_plugin_ids(): instead of relying on the tracker's in-memory `get_all_*` view. """ return list(_discovered_plugin_manifests()) +def _non_plugin_id_error(plugin_id): + """Error response when ``plugin_id`` cannot name a plugin, else None. + + Uninstall and config reset take the id from the request body and delete + or overwrite ``config[plugin_id]`` -- so ``{"plugin_id": "display"}`` + removed the core display section and reported success. Core sections are + never plugin ids. The secrets-only core keys (``github`` holds the Plugin + Store token) are refused too, unless a plugin by that id is really + installed. Uninstall deliberately does not require the plugin to be + installed: it must still clean the config of one whose directory is gone. + """ + if safe_path_component(plugin_id) is None: + return error_response(ErrorCode.INVALID_INPUT, + f'Invalid plugin id: {plugin_id!r}', status_code=400) + if plugin_id in CORE_CONFIG_KEYS or ( + plugin_id in CORE_SECRETS_KEYS + and plugin_id not in _discovered_plugin_manifests(plugin_id)): + return error_response(ErrorCode.INVALID_INPUT, + f"'{plugin_id}' is a core configuration section, not a plugin", + status_code=400) + return None def _discovered_plugin_manifests(plugin_id=None, rescan=False): """The plugin manager's manifests, discovering plugins first if needed. diff --git a/web_interface/blueprints/api_v3/config.py b/web_interface/blueprints/api_v3/config.py index 00355af1..bdd97207 100644 --- a/web_interface/blueprints/api_v3/config.py +++ b/web_interface/blueprints/api_v3/config.py @@ -687,10 +687,21 @@ def save_main_config(): _set_checkbox(current_config['display'], 'use_short_date_format', 'use_short_date_format') # Handle dynamic duration settings - if 'max_dynamic_duration_seconds' in data: + # The Display form posts this on every save, as "" when the box + # was cleared; int("") was a 500 that lost the whole save. Blank + # keeps the stored cap, and anything else is held to the form's + # 30-1800 range instead of raising. + max_dynamic = data.get('max_dynamic_duration_seconds') + if max_dynamic is None or (isinstance(max_dynamic, str) and not max_dynamic.strip()): + max_dynamic = None + else: + error = _hardware_int_error('max_dynamic_duration_seconds', 30, 1800) + if error: + return error + if max_dynamic is not None: if 'dynamic_duration' not in current_config['display']: current_config['display']['dynamic_duration'] = {} - current_config['display']['dynamic_duration']['max_duration_seconds'] = int(data['max_dynamic_duration_seconds']) + current_config['display']['dynamic_duration']['max_duration_seconds'] = int(max_dynamic) # Handle double-sided display settings double_sided_fields = ['double_sided_enabled', 'double_sided_copies', 'double_sided_axis'] @@ -1155,6 +1166,10 @@ def save_raw_main_config(): return jsonify({'status': 'error', 'message': 'Invalid JSON in request body'}), 400 if not data: return jsonify({'status': 'error', 'message': 'No data provided'}), 400 + # A JSON array or string parses fine and would be written over + # config.json as-is, leaving a file nothing can load. + if not isinstance(data, dict): + return jsonify({'status': 'error', 'message': 'Configuration must be a JSON object'}), 400 was_auto_update_enabled = False try: @@ -1217,6 +1232,10 @@ def save_raw_secrets_config(): return jsonify({'status': 'error', 'message': 'Invalid JSON in request body'}), 400 if not data: return jsonify({'status': 'error', 'message': 'No data provided'}), 400 + # strip_masked_values/deep_merge below expect an object; anything + # else was a 500 at best and a replaced secrets file at worst. + if not isinstance(data, dict): + return jsonify({'status': 'error', 'message': 'Secrets configuration must be a JSON object'}), 400 # The GET above masks what it returns, and this endpoint's only client # reads the whole file, edits one field and posts all of it back. So diff --git a/web_interface/blueprints/api_v3/plugins.py b/web_interface/blueprints/api_v3/plugins.py index 6fce4411..e8682ed5 100644 --- a/web_interface/blueprints/api_v3/plugins.py +++ b/web_interface/blueprints/api_v3/plugins.py @@ -6,7 +6,7 @@ so their endpoint names are unchanged by living here. from web_interface.blueprints.api_v3 import ( ErrorCode, OperationType, PROJECT_ROOT, Path, Response, _CALENDAR_LIST_MAX_PAGES, _RENDERED_SECTION_FIELD, _SKIP_FIELD, _coerce_to_bool, - _do_transactional_uninstall, _enhance_schema_with_core_properties, + _do_transactional_uninstall, _enhance_schema_with_core_properties, _non_plugin_id_error, _filter_config_by_schema, _get_plugin_version, _get_schema_property, _hidden_array_item_property, _installed_plugin_ids, _is_plugin_update_available, _plugin_directory, @@ -1107,6 +1107,9 @@ def uninstall_plugin(): plugin_id = data['plugin_id'] preserve_config = data.get('preserve_config', False) + id_error = _non_plugin_id_error(plugin_id) + if id_error: + return id_error # Both queued and direct paths use the same transactional helper so # snapshot/rollback behaviour is consistent regardless of deployment. @@ -2338,6 +2341,9 @@ def reset_plugin_config(): if not plugin_id: return jsonify({'status': 'error', 'message': 'plugin_id required'}), 400 + id_error = _non_plugin_id_error(plugin_id) + if id_error: + return id_error # Get schema manager instance schema_mgr = api_v3.schema_manager