rick/LEDMatrix
1
0
Fork 0
mirror of https://github.com/ChuckBuilds/LEDMatrix.git synced 2026-04-29 20:13:00 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(backup): address PR review findings

Browse Source 
- backup_manager: read plugin state from "states" key (not "plugins") to
  match the actual plugin_state.json format written by state_manager
- backup_manager: stream ZIP directly to a temp file instead of building
  it in an io.BytesIO buffer to avoid OOM on Raspberry Pi
- backup_manager: tighten plugin-uploads path validation in validate_backup
  and restore_backup to require "/uploads/" in the path, rejecting any
  non-uploads files smuggled under assets/plugins/
- api_v3: enforce 200 MB upload limit by streaming in chunks rather than
  relying on validate_file_upload (which only checks the filename)
- api_v3: replace bool() with _coerce_to_bool() for RestoreOptions fields
  so string "false" is not treated as truthy
- api_v3: capture and log _save_config_atomic return value instead of
  discarding it; log rather than silence font-cache and config-reload errors
- backup_restore.html: track inspectedFile so runRestore always applies to
  the file the user inspected, not a subsequently selected file; clear on
  input change or clearRestore()
- backup_restore.html: throw on non-success restore payload so errors are
  surfaced via the error notification path instead of yellow "warnings"
- test: update fixture to use correct "states" key structure; import
  SCHEMA_VERSION constant instead of hardcoding 1; rename unused err -> _err

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Chuck
2026-04-27 10:10:01 -04:00
parent a84b65fffb
commit b609b9e9e1
4 changed files with 65 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files

20
src/backup_manager.py
View File

@@ -12,7 +12,6 @@ used from scripts.
from __future__ import annotations
import io
import json
import logging
import os
@@ -189,7 +188,7 @@ def list_installed_plugins(project_root: Path) -> List[Dict[str, Any]]:
try:
with state_file.open("r", encoding="utf-8") as f:
state = json.load(f)
raw_plugins = state.get("plugins", {}) if isinstance(state, dict) else {}
raw_plugins = state.get("states", {}) if isinstance(state, dict) else {}
if isinstance(raw_plugins, dict):
for plugin_id, info in raw_plugins.items():
if not isinstance(info, dict):
@@ -290,9 +289,9 @@ def create_backup(
contents: List[str] = []
# Build bundle in memory first, then atomically write to final path.
buffer = io.BytesIO()
with zipfile.ZipFile(buffer, "w", compression=zipfile.ZIP_DEFLATED) as zf:
# Stream directly to a temp file so we never hold the whole ZIP in memory.
tmp_path = zip_path.with_suffix(".zip.tmp")
with zipfile.ZipFile(tmp_path, "w", compression=zipfile.ZIP_DEFLATED) as zf:
# Config files.
if (project_root / _CONFIG_REL).exists():
zf.write(project_root / _CONFIG_REL, _CONFIG_REL.as_posix())
@@ -333,9 +332,6 @@ def create_backup(
manifest = _build_manifest(contents, project_root)
zf.writestr(MANIFEST_NAME, json.dumps(manifest, indent=2))
# Write atomically.
tmp_path = zip_path.with_suffix(".zip.tmp")
tmp_path.write_bytes(buffer.getvalue())
os.replace(tmp_path, zip_path)
logger.info("Created backup %s (%d bytes)", zip_path, zip_path.stat().st_size)
return zip_path
@@ -429,7 +425,10 @@ def validate_backup(zip_path: Path) -> Tuple[bool, str, Dict[str, Any]]:
detected.append("wifi")
if any(n.startswith(_FONTS_REL.as_posix() + "/") for n in names):
detected.append("fonts")
if any(n.startswith(_PLUGIN_UPLOADS_REL.as_posix() + "/") for n in names):
if any(
n.startswith(_PLUGIN_UPLOADS_REL.as_posix() + "/") and "/uploads/" in n
for n in names
):
detected.append("plugin_uploads")
plugins: List[Dict[str, Any]] = []
@@ -569,6 +568,9 @@ def restore_backup(
for name in files:
src = Path(root) / name
rel = src.relative_to(tmp_dir)
if "/uploads/" not in rel.as_posix():
result.errors.append(f"Rejected unexpected plugin path: {rel}")
continue
try:
_copy_file(src, project_root / rel)
count += 1

12
test/test_backup_manager.py
View File

@@ -12,6 +12,7 @@ import pytest
from src import backup_manager
from src.backup_manager import (
BUNDLED_FONTS,
SCHEMA_VERSION,
RestoreOptions,
create_backup,
list_installed_plugins,
@@ -66,10 +67,11 @@ def _make_project(root: Path) -> Path:
(root / "data" / "plugin_state.json").write_text(
json.dumps(
{
"plugins": {
"version": 1,
"states": {
"my-plugin": {"version": "1.2.3", "enabled": True},
"other-plugin": {"version": "0.1.0", "enabled": False},
}
},
}
),
encoding="utf-8",
@@ -204,7 +206,7 @@ def test_validate_backup_bad_schema_version(tmp_path: Path) -> None:
def test_validate_backup_rejects_zip_traversal(tmp_path: Path) -> None:
zip_path = tmp_path / "malicious.zip"
with zipfile.ZipFile(zip_path, "w") as zf:
zf.writestr("manifest.json", json.dumps({"schema_version": 1, "contents": []}))
zf.writestr("manifest.json", json.dumps({"schema_version": SCHEMA_VERSION, "contents": []}))
zf.writestr("../../etc/passwd", "x")
ok, err, _ = validate_backup(zip_path)
assert not ok
@@ -214,7 +216,7 @@ def test_validate_backup_rejects_zip_traversal(tmp_path: Path) -> None:
def test_validate_backup_not_a_zip(tmp_path: Path) -> None:
p = tmp_path / "nope.zip"
p.write_text("hello", encoding="utf-8")
ok, err, _ = validate_backup(p)
ok, _err, _ = validate_backup(p)
assert not ok
@@ -275,7 +277,7 @@ def test_restore_honors_options(project: Path, empty_project: Path, tmp_path: Pa
def test_restore_rejects_malicious_zip(empty_project: Path, tmp_path: Path) -> None:
zip_path = tmp_path / "bad.zip"
with zipfile.ZipFile(zip_path, "w") as zf:
zf.writestr("manifest.json", json.dumps({"schema_version": 1, "contents": []}))
zf.writestr("manifest.json", json.dumps({"schema_version": SCHEMA_VERSION, "contents": []}))
zf.writestr("../escape.txt", "x")
result = restore_backup(zip_path, empty_project, RestoreOptions())
# validate_backup catches it before extraction.

34
web_interface/blueprints/api_v3.py
View File

@@ -1232,8 +1232,20 @@ def _save_uploaded_backup_to_temp() -> Tuple[Optional[Path], Optional[Tuple[Resp
fd, tmp_name = _tempfile.mkstemp(prefix='ledmatrix_upload_', suffix='.zip')
os.close(fd)
tmp_path = Path(tmp_name)
max_bytes = 200 * 1024 * 1024
try:
upload.save(str(tmp_path))
written = 0
with open(tmp_path, 'wb') as fh:
while True:
chunk = upload.stream.read(65536)
if not chunk:
break
written += len(chunk)
if written > max_bytes:
fh.close()
tmp_path.unlink(missing_ok=True)
return None, (jsonify({'status': 'error', 'message': 'Backup file exceeds 200 MB limit'}), 413)
fh.write(chunk)
except Exception:
tmp_path.unlink(missing_ok=True)
logger.exception("[Backup] Failed to save uploaded backup")
@@ -1286,12 +1298,12 @@ def backup_restore():
return jsonify({'status': 'error', 'message': 'Invalid options JSON'}), 400
opts = backup_manager.RestoreOptions(
restore_config=bool(opts_dict.get('restore_config', True)),
restore_secrets=bool(opts_dict.get('restore_secrets', True)),
restore_wifi=bool(opts_dict.get('restore_wifi', True)),
restore_fonts=bool(opts_dict.get('restore_fonts', True)),
restore_plugin_uploads=bool(opts_dict.get('restore_plugin_uploads', True)),
reinstall_plugins=bool(opts_dict.get('reinstall_plugins', True)),
restore_config=_coerce_to_bool(opts_dict.get('restore_config', True)),
restore_secrets=_coerce_to_bool(opts_dict.get('restore_secrets', True)),
restore_wifi=_coerce_to_bool(opts_dict.get('restore_wifi', True)),
restore_fonts=_coerce_to_bool(opts_dict.get('restore_fonts', True)),
restore_plugin_uploads=_coerce_to_bool(opts_dict.get('restore_plugin_uploads', True)),
reinstall_plugins=_coerce_to_bool(opts_dict.get('reinstall_plugins', True)),
)
# Snapshot current config through the atomic manager so the pre-restore
@@ -1299,7 +1311,9 @@ def backup_restore():
if api_v3.config_manager and opts.restore_config:
try:
current = api_v3.config_manager.load_config()
_save_config_atomic(api_v3.config_manager, current, create_backup=True)
snapshot_ok, snapshot_err = _save_config_atomic(api_v3.config_manager, current, create_backup=True)
if not snapshot_ok:
logger.warning("[Backup] Pre-restore snapshot failed: %s (continuing)", snapshot_err)
except Exception:
logger.warning("[Backup] Pre-restore snapshot failed (continuing)", exc_info=True)
@@ -1337,7 +1351,7 @@ def backup_restore():
from web_interface.cache import delete_cached
delete_cached('fonts_catalog')
except Exception:
pass
logger.warning("[Backup] Failed to clear font cache", exc_info=True)
# Reload config_manager state so the UI picks up the new values
# without a full service restart.
@@ -1348,7 +1362,7 @@ def backup_restore():
try:
api_v3.config_manager.load_config()
except Exception:
pass
logger.warning("[Backup] Could not reload config after restore", exc_info=True)
except Exception:
logger.warning("[Backup] Could not reload config after restore", exc_info=True)

32
web_interface/templates/v3/partials/backup_restore.html
View File

@@ -117,6 +117,8 @@
<script>
(function () {
let inspectedFile = null;
function notify(message, kind) {
if (typeof showNotification === 'function') {
showNotification(message, kind || 'info');
@@ -245,12 +247,14 @@
notify('Choose a backup file first', 'error');
return;
}
const file = input.files[0];
const fd = new FormData();
fd.append('backup_file', input.files[0]);
fd.append('backup_file', file);
try {
const res = await fetch('/api/v3/backup/validate', { method: 'POST', body: fd });
const payload = await res.json();
if (payload.status !== 'success') throw new Error(payload.message || 'Validation failed');
inspectedFile = file;
renderRestorePreview(payload.data);
} catch (err) {
notify('Invalid backup: ' + err.message, 'error');
@@ -273,15 +277,15 @@
}
function clearRestore() {
inspectedFile = null;
document.getElementById('restore-preview').classList.add('hidden');
document.getElementById('restore-result').classList.add('hidden');
document.getElementById('restore-file-input').value = '';
}
async function runRestore() {
const input = document.getElementById('restore-file-input');
if (!input.files || !input.files[0]) {
notify('Choose a backup file first', 'error');
if (!inspectedFile) {
notify('Inspect the file before restoring', 'error');
return;
}
if (!confirm('Restore from this backup? Current configuration will be overwritten.')) return;
@@ -295,7 +299,7 @@
reinstall_plugins: document.getElementById('opt-reinstall').checked,
};
const fd = new FormData();
fd.append('backup_file', input.files[0]);
fd.append('backup_file', inspectedFile);
fd.append('options', JSON.stringify(options));
const btn = document.getElementById('run-restore-btn');
@@ -304,13 +308,16 @@
try {
const res = await fetch('/api/v3/backup/restore', { method: 'POST', body: fd });
const payload = await res.json();
if (payload.status !== 'success') {
const msgs = (payload.data?.errors || []).join('; ');
throw new Error(payload.message || msgs || 'Restore had errors');
}
const data = payload.data || {};
const result = document.getElementById('restore-result');
const ok = payload.status === 'success';
result.className = (ok ? 'bg-green-50 border-green-200 text-green-800' : 'bg-yellow-50 border-yellow-200 text-yellow-800') + ' border rounded-md p-4';
result.className = 'bg-green-50 border-green-200 text-green-800 border rounded-md p-4';
result.classList.remove('hidden');
result.innerHTML = `
<h3 class="font-medium mb-2">${ok ? 'Restore complete' : 'Restore finished with warnings'}</h3>
<h3 class="font-medium mb-2">Restore complete</h3>
<div><strong>Restored:</strong> ${(data.restored || []).map(escapeHtml).join(', ') || 'none'}</div>
<div><strong>Skipped:</strong> ${(data.skipped || []).map(escapeHtml).join(', ') || 'none'}</div>
<div><strong>Plugins installed:</strong> ${(data.plugins_installed || []).map(escapeHtml).join(', ') || 'none'}</div>
@@ -318,7 +325,7 @@
<div><strong>Errors:</strong> ${(data.errors || []).map(escapeHtml).join('; ') || 'none'}</div>
<p class="mt-2">Restart the display service to apply all changes.</p>
`;
notify(ok ? 'Restore complete' : 'Restore finished with warnings', ok ? 'success' : 'info');
notify('Restore complete', 'success');
} catch (err) {
notify('Restore failed: ' + err.message, 'error');
} finally {
@@ -334,6 +341,13 @@
window.clearRestore = clearRestore;
window.runRestore = runRestore;
// Clear inspection state whenever the user picks a new file.
document.getElementById('restore-file-input').addEventListener('change', function () {
inspectedFile = null;
document.getElementById('restore-preview').classList.add('hidden');
document.getElementById('restore-result').classList.add('hidden');
});
// Initial load.
loadPreview();
loadBackupList();