fix(backup): address second round of PR review findings

- api_v3: guard opts_dict with isinstance check after json.loads so a
  non-object JSON payload (null, array, etc.) returns a 400 instead of a
  500 AttributeError
- backup_manager: wrap tmp ZIP creation and os.replace in try/except so
  the .zip.tmp temp file is always removed on any failure
- backup_manager: replace hardcoded Path("/tmp/_zip_check") sentinel in
  validate_backup with a proper tempfile.TemporaryDirectory() so path
  traversal checks are portable and leave no artifacts
- backup_restore.html: detect partial-success responses (plugins_failed or
  errors non-empty) even when status is 'success' and render yellow/warning
  styling and notify instead of green

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Chuck
2026-04-27 12:03:47 -04:00
parent b609b9e9e1
commit 3d44e15a0d
3 changed files with 60 additions and 49 deletions

View File

@@ -291,6 +291,7 @@ def create_backup(
# Stream directly to a temp file so we never hold the whole ZIP in memory. # Stream directly to a temp file so we never hold the whole ZIP in memory.
tmp_path = zip_path.with_suffix(".zip.tmp") tmp_path = zip_path.with_suffix(".zip.tmp")
try:
with zipfile.ZipFile(tmp_path, "w", compression=zipfile.ZIP_DEFLATED) as zf: with zipfile.ZipFile(tmp_path, "w", compression=zipfile.ZIP_DEFLATED) as zf:
# Config files. # Config files.
if (project_root / _CONFIG_REL).exists(): if (project_root / _CONFIG_REL).exists():
@@ -333,6 +334,9 @@ def create_backup(
zf.writestr(MANIFEST_NAME, json.dumps(manifest, indent=2)) zf.writestr(MANIFEST_NAME, json.dumps(manifest, indent=2))
os.replace(tmp_path, zip_path) os.replace(tmp_path, zip_path)
except Exception:
tmp_path.unlink(missing_ok=True)
raise
logger.info("Created backup %s (%d bytes)", zip_path, zip_path.stat().st_size) logger.info("Created backup %s (%d bytes)", zip_path, zip_path.stat().st_size)
return zip_path return zip_path
@@ -391,6 +395,8 @@ def validate_backup(zip_path: Path) -> Tuple[bool, str, Dict[str, Any]]:
return False, "Backup is missing manifest.json", {} return False, "Backup is missing manifest.json", {}
total = 0 total = 0
with tempfile.TemporaryDirectory() as _sandbox:
sandbox = Path(_sandbox)
for info in zf.infolist(): for info in zf.infolist():
if info.file_size > _MAX_MEMBER_BYTES: if info.file_size > _MAX_MEMBER_BYTES:
return False, f"Member {info.filename} is too large", {} return False, f"Member {info.filename} is too large", {}
@@ -398,7 +404,7 @@ def validate_backup(zip_path: Path) -> Tuple[bool, str, Dict[str, Any]]:
if total > _MAX_TOTAL_BYTES: if total > _MAX_TOTAL_BYTES:
return False, "Backup exceeds maximum allowed size", {} return False, "Backup exceeds maximum allowed size", {}
# Safety: reject members with unsafe paths up front. # Safety: reject members with unsafe paths up front.
if _safe_extract_path(Path("/tmp/_zip_check"), info.filename) is None: if _safe_extract_path(sandbox, info.filename) is None:
return False, f"Unsafe path in backup: {info.filename}", {} return False, f"Unsafe path in backup: {info.filename}", {}
try: try:

View File

@@ -1296,6 +1296,8 @@ def backup_restore():
opts_dict = json.loads(raw_opts) opts_dict = json.loads(raw_opts)
except json.JSONDecodeError: except json.JSONDecodeError:
return jsonify({'status': 'error', 'message': 'Invalid options JSON'}), 400 return jsonify({'status': 'error', 'message': 'Invalid options JSON'}), 400
if not isinstance(opts_dict, dict):
return jsonify({'status': 'error', 'message': 'options must be an object'}), 400
opts = backup_manager.RestoreOptions( opts = backup_manager.RestoreOptions(
restore_config=_coerce_to_bool(opts_dict.get('restore_config', True)), restore_config=_coerce_to_bool(opts_dict.get('restore_config', True)),

View File

@@ -313,11 +313,14 @@
throw new Error(payload.message || msgs || 'Restore had errors'); throw new Error(payload.message || msgs || 'Restore had errors');
} }
const data = payload.data || {}; const data = payload.data || {};
const hasPartial = (data.plugins_failed || []).length > 0 || (data.errors || []).length > 0;
const result = document.getElementById('restore-result'); const result = document.getElementById('restore-result');
result.className = 'bg-green-50 border-green-200 text-green-800 border rounded-md p-4'; result.className = (hasPartial
? 'bg-yellow-50 border-yellow-200 text-yellow-800'
: 'bg-green-50 border-green-200 text-green-800') + ' border rounded-md p-4';
result.classList.remove('hidden'); result.classList.remove('hidden');
result.innerHTML = ` result.innerHTML = `
<h3 class="font-medium mb-2">Restore complete</h3> <h3 class="font-medium mb-2">${hasPartial ? 'Restore complete with warnings' : 'Restore complete'}</h3>
<div><strong>Restored:</strong> ${(data.restored || []).map(escapeHtml).join(', ') || 'none'}</div> <div><strong>Restored:</strong> ${(data.restored || []).map(escapeHtml).join(', ') || 'none'}</div>
<div><strong>Skipped:</strong> ${(data.skipped || []).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> <div><strong>Plugins installed:</strong> ${(data.plugins_installed || []).map(escapeHtml).join(', ') || 'none'}</div>
@@ -325,7 +328,7 @@
<div><strong>Errors:</strong> ${(data.errors || []).map(escapeHtml).join('; ') || 'none'}</div> <div><strong>Errors:</strong> ${(data.errors || []).map(escapeHtml).join('; ') || 'none'}</div>
<p class="mt-2">Restart the display service to apply all changes.</p> <p class="mt-2">Restart the display service to apply all changes.</p>
`; `;
notify('Restore complete', 'success'); notify(hasPartial ? 'Restore complete with warnings' : 'Restore complete', hasPartial ? 'warning' : 'success');
} catch (err) { } catch (err) {
notify('Restore failed: ' + err.message, 'error'); notify('Restore failed: ' + err.message, 'error');
} finally { } finally {