diff --git a/README.md b/README.md index ccc12332..a23b6720 100644 --- a/README.md +++ b/README.md @@ -365,6 +365,12 @@ sudo bash ./first_time_install.sh This single script installs services, dependencies, configures permissions and sudoers, and validates the setup. +It finishes by asking whether to reboot. If you run it non-interactively — piped, over a script, or with `-y` — there is no one to ask, so **it reboots immediately without prompting**. Pass `--no-reboot-prompt` to install without rebooting: + +```bash +sudo bash ./first_time_install.sh -y --no-reboot-prompt +``` + diff --git a/first_time_install.sh b/first_time_install.sh index 5515544b..2256be52 100644 --- a/first_time_install.sh +++ b/first_time_install.sh @@ -1480,27 +1480,54 @@ if [ -f "$PROJECT_ROOT_DIR/config/config.json" ]; then fi # Set proper permissions for secrets file (restrictive: owner rw, group r) -# If service runs as root, set ownership to root so it can read as owner -# Otherwise, use ACTUAL_USER and rely on group membership +# Owned by whoever WRITES the file, which is the web interface. +# +# This used to read the User= of ledmatrix.service — the display service — +# and, finding root, hand the file to root:ledmatrix 640. But the display +# service only ever reads secrets, and root can read any file regardless of +# mode. The account that *writes* them is the web interface: it saves config +# edits and performs backup restores, and it deliberately does not run as root +# (a web server should not). So a root-owned, group-read-only file left the web +# UI unable to write its own secrets, and restoring a backup failed with +# "Permission denied: config_secrets.json" while every other file in the same +# backup restored fine. +# +# Owning by the writer keeps the tighter 640 rather than loosening to +# group-writable, and root still reads it as superuser. if [ -f "$PROJECT_ROOT_DIR/config/config_secrets.json" ]; then - # Check if service runs as root (from service file or template) - SERVICE_USER="root" - if [ -f "/etc/systemd/system/ledmatrix.service" ]; then - SERVICE_USER=$(grep "^User=" /etc/systemd/system/ledmatrix.service | cut -d'=' -f2 || echo "root") - elif [ -f "$PROJECT_ROOT_DIR/systemd/ledmatrix.service" ]; then - SERVICE_USER=$(grep "^User=" "$PROJECT_ROOT_DIR/systemd/ledmatrix.service" | cut -d'=' -f2 || echo "root") + # The web service is the writer; fall back to the display service, then to + # the installing user, so an unusual layout still lands somewhere sensible. + SECRETS_OWNER="" + for unit in "/etc/systemd/system/ledmatrix-web.service" \ + "$PROJECT_ROOT_DIR/systemd/ledmatrix-web.service"; do + if [ -f "$unit" ]; then + SECRETS_OWNER=$(grep -m1 "^User=" "$unit" | cut -d'=' -f2) + [ -n "$SECRETS_OWNER" ] && break + fi + done + if [ -z "$SECRETS_OWNER" ]; then + SECRETS_OWNER="$ACTUAL_USER" fi - - if [ "$SERVICE_USER" = "root" ]; then - # Service runs as root - set ownership to root so it can read as owner - chown "root:$LEDMATRIX_GROUP" "$PROJECT_ROOT_DIR/config/config_secrets.json" || true - echo "✓ Secrets file permissions set (root:ledmatrix for root service)" - else - # Service runs as regular user - use ACTUAL_USER and rely on group membership - chown "$ACTUAL_USER:$LEDMATRIX_GROUP" "$PROJECT_ROOT_DIR/config/config_secrets.json" || true - echo "✓ Secrets file permissions set ($ACTUAL_USER:ledmatrix)" + SECRETS_FILE="$PROJECT_ROOT_DIR/config/config_secrets.json" + # A root-owned file is only correct when the writer really is root. + if ! chown "$SECRETS_OWNER:$LEDMATRIX_GROUP" "$SECRETS_FILE"; then + echo "✗ ERROR: Failed to set ownership on $SECRETS_FILE to $SECRETS_OWNER:$LEDMATRIX_GROUP" >&2 + echo " Try: sudo chown $SECRETS_OWNER:$LEDMATRIX_GROUP $SECRETS_FILE" >&2 + exit 1 fi - chmod 640 "$PROJECT_ROOT_DIR/config/config_secrets.json" + if ! chmod 640 "$SECRETS_FILE"; then + echo "✗ ERROR: Failed to set permissions on $SECRETS_FILE to 640" >&2 + echo " Try: sudo chmod 640 $SECRETS_FILE" >&2 + exit 1 + fi + ACTUAL_OWNERSHIP=$(stat -c '%U:%G' "$SECRETS_FILE" 2>/dev/null || echo "unknown") + ACTUAL_MODE=$(stat -c '%a' "$SECRETS_FILE" 2>/dev/null || echo "unknown") + if [ "$ACTUAL_OWNERSHIP" != "$SECRETS_OWNER:$LEDMATRIX_GROUP" ] || [ "$ACTUAL_MODE" != "640" ]; then + echo "✗ ERROR: $SECRETS_FILE ended up as $ACTUAL_OWNERSHIP mode $ACTUAL_MODE, expected $SECRETS_OWNER:$LEDMATRIX_GROUP mode 640" >&2 + echo " The web interface may be unable to read or write config_secrets.json." >&2 + exit 1 + fi + echo "✓ Secrets file owned by the web service user ($SECRETS_OWNER:$LEDMATRIX_GROUP, mode 640)" fi # Set proper permissions for YTM auth file (readable by all users including root service) diff --git a/src/backup_manager.py b/src/backup_manager.py index c54ce81e..e8d03b18 100644 --- a/src/backup_manager.py +++ b/src/backup_manager.py @@ -16,6 +16,7 @@ import json import logging import os import shutil +import stat import socket import tempfile import zipfile @@ -82,6 +83,10 @@ BUNDLED_FONTS: frozenset[str] = frozenset({ _CONFIG_REL = Path("config/config.json") _SECRETS_REL = Path("config/config_secrets.json") _WIFI_REL = Path("config/wifi_config.json") +# Sits in config/ next to the three above and is pure user state — a +# YouTube Music session that has to be re-authenticated by hand if lost. +# It was omitted from backups, so a restore silently signed the user out. +_YTM_REL = Path("config/ytm_auth.json") _FONTS_REL = Path("assets/fonts") _PLUGIN_UPLOADS_REL = Path("assets/plugins") _STATE_REL = Path("data/plugin_state.json") @@ -303,6 +308,9 @@ def create_backup( if (project_root / _WIFI_REL).exists(): zf.write(project_root / _WIFI_REL, _WIFI_REL.as_posix()) contents.append("wifi") + if (project_root / _YTM_REL).exists(): + zf.write(project_root / _YTM_REL, _YTM_REL.as_posix()) + contents.append("ytm_auth") # User-uploaded fonts. user_fonts = iter_user_fonts(project_root) @@ -348,6 +356,7 @@ def preview_backup_contents(project_root: Path) -> Dict[str, Any]: "has_config": (project_root / _CONFIG_REL).exists(), "has_secrets": (project_root / _SECRETS_REL).exists(), "has_wifi": (project_root / _WIFI_REL).exists(), + "has_ytm_auth": (project_root / _YTM_REL).exists(), "user_fonts": [p.name for p in iter_user_fonts(project_root)], "plugin_uploads": len(iter_plugin_uploads(project_root)), "plugins": list_installed_plugins(project_root), @@ -429,6 +438,8 @@ def validate_backup(zip_path: Path) -> Tuple[bool, str, Dict[str, Any]]: detected.append("secrets") if _WIFI_REL.as_posix() in names: detected.append("wifi") + if _YTM_REL.as_posix() in names: + detected.append("ytm_auth") if any(n.startswith(_FONTS_REL.as_posix() + "/") for n in names): detected.append("fonts") if any( @@ -481,8 +492,61 @@ def _extract_zip_safe(zip_path: Path, dest_dir: Path) -> None: def _copy_file(src: Path, dst: Path) -> None: + """Replace ``dst`` with ``src``, atomically, without needing to own ``dst``. + + ``shutil.copy2`` opens the destination for writing, so it needs write + permission on the *existing file*. Several config files are installed + root-owned and group-readable while the web interface — which is what runs + a restore — deliberately runs as a non-root user. Restoring those failed + with EACCES even though the account could create files in the same + directory perfectly well. + + Writing a temporary file alongside and renaming over the target needs only + directory permission, which the web user has. It is also atomic: a crash + mid-restore can no longer leave a half-written config behind. + + The destination's existing mode is preserved when there is one, so + restoring secrets does not silently widen them to the umask default. + """ dst.parent.mkdir(parents=True, exist_ok=True) - shutil.copy2(src, dst) + + existing_mode: Optional[int] = None + existing_owner: Optional[Tuple[int, int]] = None + if dst.exists(): + try: + info = dst.stat() + existing_mode = stat.S_IMODE(info.st_mode) + existing_owner = (info.st_uid, info.st_gid) + except OSError: + existing_mode = None + existing_owner = None + + fd, tmp_name = tempfile.mkstemp(dir=str(dst.parent), prefix=f".{dst.name}.", suffix=".tmp") + os.close(fd) + tmp_path = Path(tmp_name) + try: + shutil.copyfile(src, tmp_path) + if existing_mode is not None: + os.chmod(tmp_path, existing_mode) + else: + shutil.copymode(src, tmp_path) + if existing_owner is not None: + # Replacing a file creates a new inode owned by whoever is running, + # which would silently move a root-owned config to the web user. + # Carry the previous owner across when the OS permits it — only + # root can hand a file to another user, so this is best-effort and + # a plain restore as the web user simply keeps its own ownership. + try: + os.chown(tmp_path, existing_owner[0], existing_owner[1]) + except (OSError, PermissionError): + pass + os.replace(tmp_path, dst) + except BaseException: + try: + tmp_path.unlink() + except OSError: + pass + raise def restore_backup( @@ -513,7 +577,8 @@ def restore_backup( try: _extract_zip_safe(Path(zip_path), tmp_dir) except (ValueError, zipfile.BadZipFile, OSError) as e: - result.errors.append(f"Failed to extract backup: {e}") + logger.error("[Backup] Failed to extract backup: %s", e, exc_info=True) + result.errors.append("Failed to extract backup") return result # Main config. @@ -522,7 +587,8 @@ def restore_backup( _copy_file(tmp_dir / _CONFIG_REL, project_root / _CONFIG_REL) result.restored.append("config") except OSError as e: - result.errors.append(f"Failed to restore config.json: {e}") + logger.error("[Backup] Failed to restore config.json: %s", e, exc_info=True) + result.errors.append("Failed to restore config.json") elif (tmp_dir / _CONFIG_REL).exists(): result.skipped.append("config") @@ -532,7 +598,10 @@ def restore_backup( _copy_file(tmp_dir / _SECRETS_REL, project_root / _SECRETS_REL) result.restored.append("secrets") except OSError as e: - result.errors.append(f"Failed to restore config_secrets.json: {e}") + logger.error( + "[Backup] Failed to restore config_secrets.json: %s", e, exc_info=True + ) + result.errors.append("Failed to restore config_secrets.json") elif (tmp_dir / _SECRETS_REL).exists(): result.skipped.append("secrets") @@ -542,10 +611,26 @@ def restore_backup( _copy_file(tmp_dir / _WIFI_REL, project_root / _WIFI_REL) result.restored.append("wifi") except OSError as e: - result.errors.append(f"Failed to restore wifi_config.json: {e}") + logger.error( + "[Backup] Failed to restore wifi_config.json: %s", e, exc_info=True + ) + result.errors.append("Failed to restore wifi_config.json") elif (tmp_dir / _WIFI_REL).exists(): result.skipped.append("wifi") + # YouTube Music session. Follows restore_wifi rather than getting its + # own flag: it is device-local auth in the same sense, and a separate + # toggle for one file would be noise in the restore dialog. + if options.restore_wifi and (tmp_dir / _YTM_REL).exists(): + try: + _copy_file(tmp_dir / _YTM_REL, project_root / _YTM_REL) + result.restored.append("ytm_auth") + except OSError as e: + logger.error("[Backup] Failed to restore ytm_auth.json: %s", e, exc_info=True) + result.errors.append("Failed to restore ytm_auth.json") + elif (tmp_dir / _YTM_REL).exists(): + result.skipped.append("ytm_auth") + # User fonts — skip anything that collides with a bundled font. tmp_fonts = tmp_dir / _FONTS_REL if options.restore_fonts and tmp_fonts.exists(): @@ -560,7 +645,10 @@ def restore_backup( _copy_file(font, project_root / _FONTS_REL / font.name) restored_count += 1 except OSError as e: - result.errors.append(f"Failed to restore font {font.name}: {e}") + logger.error( + "[Backup] Failed to restore font %s: %s", font.name, e, exc_info=True + ) + result.errors.append(f"Failed to restore font {font.name}") if restored_count: result.restored.append(f"fonts ({restored_count})") elif tmp_fonts.exists(): @@ -581,7 +669,8 @@ def restore_backup( _copy_file(src, project_root / rel) count += 1 except OSError as e: - result.errors.append(f"Failed to restore {rel}: {e}") + logger.error("[Backup] Failed to restore %s: %s", rel, e, exc_info=True) + result.errors.append(f"Failed to restore {rel}") if count: result.restored.append(f"plugin_uploads ({count})") elif tmp_uploads.exists(): @@ -599,7 +688,8 @@ def restore_backup( if isinstance(p, dict) and p.get("plugin_id") ] except (OSError, json.JSONDecodeError) as e: - result.errors.append(f"Could not read plugins.json: {e}") + logger.error("[Backup] Could not read plugins.json: %s", e, exc_info=True) + result.errors.append("Could not read plugins.json") result.success = not result.errors return result diff --git a/src/plugin_system/store_manager.py b/src/plugin_system/store_manager.py index 8e27ab8f..5cf31cf8 100644 --- a/src/plugin_system/store_manager.py +++ b/src/plugin_system/store_manager.py @@ -1143,7 +1143,7 @@ class PluginStoreManager: """ registry = self.fetch_registry() plugins = registry.get('plugins', []) or [] - plugin_info = next((p for p in plugins if p['id'] == plugin_id), None) + plugin_info = self._match_registry_entry(plugins, plugin_id) if not plugin_info: return None @@ -1183,6 +1183,37 @@ class PluginStoreManager: return plugin_info + @staticmethod + def _match_registry_entry(plugins: List[Dict], plugin_id: str) -> Optional[Dict]: + """Find a registry entry by its id, or by the directory it installs to. + + Four shipped plugins have a registry ``id`` that differs from the ``id`` + in their own manifest: ``weather`` installs to ``plugins/ledmatrix-weather``, + and likewise stocks, music and leaderboard. Installation already prefers + the manifest id for the directory name, so on disk, in ``config.json`` + and in a backup manifest those plugins are called ``ledmatrix-weather``. + + Only the registry calls them ``weather``, and nothing resolved that in + reverse: restoring a backup asked the store for ``ledmatrix-weather`` + and got "Plugin not found in registry", silently dropping four enabled + plugins from a restored device. + + Matching ``plugin_path`` fixes it without renaming any published id, + which would orphan ``plugin_state.json`` entries keyed on the old ones. + Exact id always wins, so an entry whose *path* happens to collide with + another entry's id cannot shadow it. + """ + if not plugin_id: + return None + exact = next((p for p in plugins if p.get('id') == plugin_id), None) + if exact is not None: + return exact + for entry in plugins: + path = (entry.get('plugin_path') or '').rstrip('/') + if path and path.rsplit('/', 1)[-1] == plugin_id: + return entry + return None + def get_registry_info(self, plugin_id: str) -> Optional[Dict]: """ Get plugin information from the registry cache only (no GitHub API calls). @@ -1198,7 +1229,7 @@ class PluginStoreManager: """ registry = self.fetch_registry() plugins = registry.get('plugins', []) or [] - return next((p for p in plugins if p.get('id') == plugin_id), None) + return self._match_registry_entry(plugins, plugin_id) def install_plugin(self, plugin_id: str, branch: Optional[str] = None) -> bool: """Install a plugin, keeping any existing install until the new one is diff --git a/test/test_backup_manager.py b/test/test_backup_manager.py index fef10d82..e6533d69 100644 --- a/test/test_backup_manager.py +++ b/test/test_backup_manager.py @@ -3,6 +3,7 @@ from __future__ import annotations import json +import stat import zipfile from pathlib import Path @@ -41,6 +42,13 @@ def _make_project(root: Path) -> Path: json.dumps({"ap_mode": {"ssid": "LEDMatrix"}}), encoding="utf-8", ) + # Device-local auth that lives in config/ like the three above. It was + # omitted from backups, so a restore silently signed the user out of + # YouTube Music and they had to re-authenticate by hand. + (root / "config" / "ytm_auth.json").write_text( + json.dumps({"token": "YTM-TOKEN"}), + encoding="utf-8", + ) fonts = root / "assets" / "fonts" fonts.mkdir(parents=True) @@ -240,6 +248,10 @@ def test_restore_roundtrip(project: Path, empty_project: Path, tmp_path: Path) - restored_secrets = json.loads((empty_project / "config" / "config_secrets.json").read_text()) assert restored_secrets["ledmatrix-weather"]["api_key"] == "SECRET" + assert "ytm_auth" in result.restored + restored_ytm = json.loads((empty_project / "config" / "ytm_auth.json").read_text()) + assert restored_ytm["token"] == "YTM-TOKEN" + # User font restored, bundled font untouched. assert (empty_project / "assets" / "fonts" / "my-custom-font.ttf").read_bytes() == b"\x00\x01USER" assert (empty_project / "assets" / "fonts" / "5x7.bdf").read_text() == "BUNDLED" @@ -271,6 +283,10 @@ def test_restore_honors_options(project: Path, empty_project: Path, tmp_path: Pa assert result.plugins_to_install == [] assert "secrets" in result.skipped assert "wifi" in result.skipped + # ytm_auth rides on restore_wifi rather than its own flag -- disabling + # wifi restore must not leave a stale session token behind. + assert "ytm_auth" in result.skipped + assert not (empty_project / "config" / "ytm_auth.json").exists() def test_restore_rejects_malicious_zip(empty_project: Path, tmp_path: Path) -> None: @@ -282,3 +298,39 @@ def test_restore_rejects_malicious_zip(empty_project: Path, tmp_path: Path) -> N # validate_backup catches it before extraction. assert not result.success assert any("unsafe" in e.lower() for e in result.errors) + + +def test_restore_over_a_file_the_user_cannot_write( + project: Path, empty_project: Path, tmp_path: Path +) -> None: + """Restore must not need write permission on the destination *file*. + + Reproduces what a fresh install leaves behind: config files owned by root + and only group-readable, while the web interface that performs the restore + runs as a non-root user. shutil.copy2 opens the destination for writing and + failed with EACCES; writing alongside and renaming needs only directory + permission, which that account has. + + Simulated here by making the destination read-only — the owner cannot + open it for writing either, but can still replace it within its directory. + """ + zip_path = create_backup(project, output_dir=tmp_path / "exports") + + # Pre-existing, read-only destinations. + (empty_project / "config").mkdir(parents=True, exist_ok=True) + for name in ("config.json", "config_secrets.json", "wifi_config.json", "ytm_auth.json"): + target = empty_project / "config" / name + target.write_text("{}", encoding="utf-8") + target.chmod(0o444) + + 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"] + + # 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 diff --git a/test/test_registry_id_resolution.py b/test/test_registry_id_resolution.py new file mode 100644 index 00000000..3e5f2bfb --- /dev/null +++ b/test/test_registry_id_resolution.py @@ -0,0 +1,114 @@ +"""A plugin must be findable in the registry by the id it calls itself. + +Four shipped plugins have a registry ``id`` that differs from the ``id`` in +their own ``manifest.json``: + + directory / manifest.json id registry id + ledmatrix-weather weather + ledmatrix-stocks stocks + ledmatrix-music music + ledmatrix-leaderboard leaderboard + +The installer already knows about this: it deliberately names the install +directory after the *manifest* id (store_manager, "Use manifest ID for +directory name"), and warns when the two disagree. So on disk, in +``config.json`` and in a backup manifest, these plugins are called +``ledmatrix-weather``. Only the registry calls them ``weather``. + +Nothing resolved that in reverse. Asking the store to install +``ledmatrix-weather`` -- which is exactly what restoring a backup does -- +failed with "Plugin not found in registry", and four enabled plugins went +missing from a restored device with no error surfaced to the user. + +Renaming the registry ids would orphan existing ``plugin_state.json`` entries +keyed on the old ones, so the lookup resolves ``plugin_path`` instead: the +registry already records ``plugins/ledmatrix-weather``, which is unambiguous +and needs no published identity to change. +""" + +import os +import sys +from typing import Any, Dict, List, Optional + +import pytest + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) + +from src.plugin_system.store_manager import PluginStoreManager # noqa: E402 + + +# Shaped like the real registry: id and plugin_path basename disagree for the +# first entry, agree for the second. +REGISTRY: Dict[str, List[Dict[str, Any]]] = { + "plugins": [ + { + "id": "weather", + "name": "Weather", + "plugin_path": "plugins/ledmatrix-weather", + "repo": "https://github.com/ChuckBuilds/ledmatrix-plugins", + }, + { + "id": "ledmatrix-flights", + "name": "Flights", + "plugin_path": "plugins/ledmatrix-flights", + "repo": "https://github.com/ChuckBuilds/ledmatrix-plugins", + }, + { + "id": "third-party", + "name": "Third Party", + "plugin_path": "", + "repo": "https://github.com/someone/thing", + }, + ] +} + + +@pytest.fixture +def store(monkeypatch: pytest.MonkeyPatch) -> PluginStoreManager: + manager = PluginStoreManager.__new__(PluginStoreManager) + monkeypatch.setattr(manager, "fetch_registry", lambda *a, **k: REGISTRY, raising=False) + return manager + + +def _ids(entry: Optional[Dict[str, Any]]) -> Optional[str]: + return entry.get("id") if entry else None + + +class TestRegistryLookupByManifestId: + def test_get_plugin_info_resolves_manifest_id(self, store: PluginStoreManager) -> None: + """get_plugin_info() delegates to the same lookup as get_registry_info().""" + assert ( + _ids(store.get_plugin_info("ledmatrix-weather", fetch_latest_from_github=False)) + == "weather" + ) + + def test_exact_registry_id_still_resolves(self, store: PluginStoreManager) -> None: + assert _ids(store.get_registry_info("weather")) == "weather" + + def test_manifest_id_resolves_via_plugin_path(self, store: PluginStoreManager) -> None: + """The case that broke restore: asked by the name on disk.""" + assert _ids(store.get_registry_info("ledmatrix-weather")) == "weather", ( + "a plugin installed as 'ledmatrix-weather' could not be found in a " + "registry that lists it under plugin_path plugins/ledmatrix-weather") + + def test_matching_id_and_path_unaffected(self, store: PluginStoreManager) -> None: + assert _ids(store.get_registry_info("ledmatrix-flights")) == "ledmatrix-flights" + + def test_unknown_plugin_still_returns_none(self, store: PluginStoreManager) -> None: + assert store.get_registry_info("no-such-plugin") is None + + def test_empty_plugin_path_is_not_a_wildcard(self, store: PluginStoreManager) -> None: + """Third-party entries carry plugin_path "" — that must not match "".""" + assert store.get_registry_info("") is None + + def test_exact_id_wins_over_a_path_match(self, monkeypatch: pytest.MonkeyPatch) -> None: + """If some other entry's path collides with a real id, id wins.""" + registry = { + "plugins": [ + {"id": "decoy", "plugin_path": "plugins/weather"}, + {"id": "weather", "plugin_path": "plugins/ledmatrix-weather"}, + ] + } + manager = PluginStoreManager.__new__(PluginStoreManager) + monkeypatch.setattr(manager, "fetch_registry", lambda *a, **k: registry, raising=False) + assert _ids(manager.get_registry_info("weather")) == "weather" diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index 95f7ddb1..56f91a8b 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -7833,7 +7833,35 @@ def clear_old_errors(): # Backup / Restore # --------------------------------------------------------------------------- -_BACKUP_EXPORT_DIR = PROJECT_ROOT / "config" / "backups" / "exports" +def _resolve_backup_export_dir() -> Path: + """Where exported backups live: beside the install, not inside it. + + They used to be written to ``/config/backups/exports``. That is + inside the directory a reinstall deletes, so the documented recovery path + -- export a backup, then reinstall -- destroyed the backup it had just + told the user to make. Anyone who downloaded the ZIP was fine; anyone + relying on the on-device copy was not. + + Falls back to the old location when the parent directory is not writable, + so an unusual layout degrades to previous behaviour instead of failing to + export at all. + """ + preferred = PROJECT_ROOT.parent / "ledmatrix-backups" + fallback = PROJECT_ROOT / "config" / "backups" / "exports" + try: + preferred.mkdir(parents=True, exist_ok=True) + with tempfile.NamedTemporaryFile(dir=preferred, prefix=".writetest-"): + pass + return preferred + except OSError as e: + logger.warning( + f"[Backup] Export dir {preferred} is not writable ({e}); " + f"falling back to {fallback}, which a reinstall will delete" + ) + return fallback + + +_BACKUP_EXPORT_DIR = _resolve_backup_export_dir() def _safe_backup_path(filename: str) -> Path: @@ -7983,11 +8011,36 @@ def backup_restore(): else: result.plugins_failed.append({'plugin_id': pid, 'error': 'Store manager unavailable'}) except Exception as pe: - result.plugins_failed.append({'plugin_id': pid, 'error': str(pe)}) + logger.error( + "[Backup] Failed to reinstall plugin %r: %s", pid, pe, exc_info=True + ) + result.plugins_failed.append({'plugin_id': pid, 'error': 'Installation failed; see server logs'}) + + # A restore that dropped files can still report success if the only + # failures were plugin reinstalls, since those don't touch result.errors. + if result.plugins_failed: + result.success = False data = result.to_dict() if not result.success: - return jsonify({'status': 'error', 'message': 'Restore had errors', 'data': data}), 500 + # Name what failed, and what nonetheless landed. A restore is + # partial far more often than it is total -- a fresh install can + # leave config_secrets.json unwritable by the web service, so + # config restores and secrets do not. "Restore had errors" alone + # left the user unable to tell a wholly failed restore from one + # that quietly dropped their API keys. + failed_plugins = [ + str(p.get('plugin_id')) for p in (result.plugins_failed or []) if p.get('plugin_id') + ] + parts = [] + if result.restored: + parts.append(f"restored: {', '.join(result.restored)}") + if result.errors: + parts.append(f"failed: {'; '.join(result.errors)}") + if failed_plugins: + parts.append(f"plugins not reinstalled: {', '.join(failed_plugins)}") + message = 'Restore incomplete — ' + ('. '.join(parts) if parts else 'see logs') + return jsonify({'status': 'error', 'message': message, 'data': data}), 500 return jsonify({'status': 'success', 'data': data}) except Exception as e: logger.error("backup_restore failed: %s", e, exc_info=True)