From 06766b408f03d9b10031577b900a8b7bc7654890 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 22:25:37 +0000 Subject: [PATCH] fix(backup): address CodeRabbit and CodeQL findings on PR #439 - first_time_install.sh: verify chown/chmod succeed and the final owner/group/mode on config_secrets.json before reporting success; exit with a clear error otherwise instead of swallowing failures. - api_v3.py: replace the predictable .writetest probe with an exclusive NamedTemporaryFile to avoid a race with concurrent resolvers; log the preferred/fallback export path and OSError when falling back to the reinstall-deleted directory. - api_v3.py: mark a restore as failed when plugin reinstalls fail, even if file restoration itself succeeded, so the endpoint no longer reports HTTP 200 success on a partial restore. - api_v3.py: stringify plugin IDs before joining them into the error message so a malformed backup's non-string plugin_id can't raise a TypeError and mask the detailed response. - backup_manager.py / api_v3.py: stop putting raw exception text (originating from a user-controlled backup file) into restore results returned to the client; log full details server-side instead. Addresses the CodeQL "stack trace information exposure" alert. - test coverage: add a test for get_plugin_info() resolving a manifest id, and assert the disabled restore_wifi path also skips and omits ytm_auth.json. --- first_time_install.sh | 20 +++++++++++++++++-- src/backup_manager.py | 30 +++++++++++++++++++++-------- test/test_backup_manager.py | 4 ++++ test/test_registry_id_resolution.py | 7 +++++++ web_interface/blueprints/api_v3.py | 28 ++++++++++++++++++++------- 5 files changed, 72 insertions(+), 17 deletions(-) diff --git a/first_time_install.sh b/first_time_install.sh index 4d643e4c..2256be52 100644 --- a/first_time_install.sh +++ b/first_time_install.sh @@ -1508,10 +1508,26 @@ if [ -f "$PROJECT_ROOT_DIR/config/config_secrets.json" ]; then if [ -z "$SECRETS_OWNER" ]; then SECRETS_OWNER="$ACTUAL_USER" fi + SECRETS_FILE="$PROJECT_ROOT_DIR/config/config_secrets.json" # A root-owned file is only correct when the writer really is root. - chown "$SECRETS_OWNER:$LEDMATRIX_GROUP" "$PROJECT_ROOT_DIR/config/config_secrets.json" || true + 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 + 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)" - chmod 640 "$PROJECT_ROOT_DIR/config/config_secrets.json" 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 b3cdb84b..e8d03b18 100644 --- a/src/backup_manager.py +++ b/src/backup_manager.py @@ -577,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. @@ -586,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") @@ -596,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") @@ -606,7 +611,10 @@ 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") @@ -618,7 +626,8 @@ def restore_backup( _copy_file(tmp_dir / _YTM_REL, project_root / _YTM_REL) result.restored.append("ytm_auth") except OSError as e: - result.errors.append(f"Failed to restore ytm_auth.json: {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") @@ -636,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(): @@ -657,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(): @@ -675,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/test/test_backup_manager.py b/test/test_backup_manager.py index 74134de8..e6533d69 100644 --- a/test/test_backup_manager.py +++ b/test/test_backup_manager.py @@ -283,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: diff --git a/test/test_registry_id_resolution.py b/test/test_registry_id_resolution.py index 85fb0084..3e5f2bfb 100644 --- a/test/test_registry_id_resolution.py +++ b/test/test_registry_id_resolution.py @@ -75,6 +75,13 @@ def _ids(entry: Optional[Dict[str, Any]]) -> Optional[str]: 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" diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index 7203d5ec..ae6eed8b 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -7862,14 +7862,18 @@ def _resolve_backup_export_dir() -> Path: export at all. """ preferred = PROJECT_ROOT.parent / "ledmatrix-backups" + fallback = PROJECT_ROOT / "config" / "backups" / "exports" try: preferred.mkdir(parents=True, exist_ok=True) - probe = preferred / ".writetest" - probe.write_text("", encoding="utf-8") - probe.unlink() + with tempfile.NamedTemporaryFile(dir=preferred, prefix=".writetest-"): + pass return preferred - except OSError: - return PROJECT_ROOT / "config" / "backups" / "exports" + 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() @@ -8022,7 +8026,15 @@ 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: @@ -8032,7 +8044,9 @@ def backup_restore(): # 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 = [p.get('plugin_id') for p in (result.plugins_failed or [])] + 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)}")