fix: reject invalid backup uploads

[VectorPeak]

Uploaded backup files were previously accepted based mainly on the .zip filename suffix. That allowed invalid archives to enter the backups directory even when they were not usable AstrBot backups.

Modifications / 改动点

What Problem This Solves

Before this change, the backup upload path mainly gated uploaded files by filename shape: a file ending in .zip could be saved into the backups directory before AstrBot checked whether it was a usable AstrBot backup archive.

In practice, the old acceptance boundary was closer to this shape: the filename suffix was checked, then the upload was persisted.

if not file.filename or not file.filename.endswith(".zip"):
    raise BackupServiceError("请上传 ZIP 格式的备份文件")

await self._save_upload(file, zip_path)

That is different from a valid backup. The ZIP file is only the container; the minimum AstrBot backup contract is that the archive must contain a readable manifest.json whose JSON value is an object. When an arbitrary invalid ZIP, or a ZIP without a usable manifest, was accepted by the upload path, later backup consumers could not treat it like a real backup.

The existing backup listing path already depends on that manifest contract:

manifest = self.get_backup_manifest(file_path)
if not manifest:
    continue

So an invalid upload could appear to succeed because the filename ended with .zip, but the saved file could then be skipped by list_backups(), unusable for restore/import paths that rely on the same manifest contract, and left behind as an orphaned archive in the backups directory.

This PR moves that failure to the upload boundary. Direct uploads and completed chunked uploads are now validated after the ZIP is saved or merged, but before the upload is accepted:

await self._save_upload(file, zip_path)
self.validate_uploaded_backup(zip_path)

The validation reuses the existing manifest read path and accepts only a readable JSON object:

with zipfile.ZipFile(zip_path, "r") as zf:
    if "manifest.json" in zf.namelist():
        manifest = json.loads(zf.read("manifest.json").decode("utf-8"))
        return manifest if isinstance(manifest, dict) else None

Archives without a readable JSON-object manifest.json now fail fast with BackupServiceError instead of being accepted first and failing later in backup consumers.

Changes

• Add BackupService.validate_uploaded_backup() to reject uploaded backup archives when manifest.json is missing, unreadable, or not a JSON object.
• Tighten get_backup_manifest() so it only returns a readable dictionary manifest and logs non-object manifest payloads at debug level.
• Validate direct uploads after saving the file and remove the saved ZIP if validation fails.
• Keep cleanup secondary to the primary failure: if deleting a failed direct-upload ZIP raises, log the cleanup failure without masking the original validation error.
• Validate chunked uploads after merging all chunks and before marking the archive as uploaded.
• On invalid chunked uploads, clean up the upload session, temporary chunk directory, and merged ZIP output before re-raising the validation failure.
• Add regression coverage in tests/test_backup.py for invalid direct upload cleanup and invalid merged archive cleanup after chunked upload completion, including the chunk session/chunk directory cleanup assertions.

Evidence

• backup_service.py now validates uploaded backups through the readable dict manifest path before accepting them.
• Direct upload failure deletes the newly saved ZIP before re-raising the validation error, while cleanup errors are logged without hiding the original failure.
• Chunked upload failure validates the merged ZIP before mark_backup_as_uploaded(), removes the upload session and chunk directory on validation failure, and still removes the merged ZIP output through the existing failure cleanup path.
• tests/test_backup.py covers both invalid direct ZIP uploads and invalid chunked ZIP uploads, including cleanup assertions for the saved/merged archive and the chunked upload session state.
• This uses the existing zipfile / json manifest-read path and does not add new dependencies. It verifies the minimum readable-dict manifest.json contract; it does not attempt to fully validate restore completeness or every backup payload file.

Possible call chain / impact

Direct upload endpoint
  -> BackupService.upload_backup(...)
  -> _save_upload(...)
  -> validate_uploaded_backup(...)
  -> get_backup_manifest(...)
  -> reject unreadable / missing / non-object manifest
  -> remove newly saved ZIP without masking the primary error

Chunked upload completion endpoint
  -> BackupService.upload_complete(...)
  -> merge received chunks into backup ZIP
  -> validate_uploaded_backup(...)
  -> reject unreadable / missing / non-object manifest
  -> cleanup upload session and temporary chunk directory
  -> remove merged ZIP output

New invalid uploads now fail fast with BackupServiceError instead of being accepted as unusable backup archives. Existing legacy invalid backup files are still handled by the existing list_backups() skip behavior; this PR changes the upload boundary, not historical cleanup or full backup restore validation.

• This is NOT a breaking change. / 这不是一个破坏性变更。

Screenshots or Test Results / 运行截图或测试结果

Before this change, a file named bad.zip containing invalid ZIP bytes could be saved by the upload path. The archive would later be skipped by list_backups() because its manifest could not be read, so the upload could appear successful while leaving an unusable backup file behind.

After this change, both direct uploads and chunked uploads fail fast with BackupServiceError when the saved or merged archive does not contain a readable manifest.json object. The invalid saved or merged file is removed before returning the error, and invalid chunked uploads also clear their upload session and temporary chunk directory.

uv run ruff format --check astrbot/dashboard/services/backup_service.py tests/test_backup.py
# 2 files already formatted

uv run ruff check astrbot/dashboard/services/backup_service.py tests/test_backup.py
# All checks passed!

uv run pytest tests/test_backup.py -k "BackupUploadValidation or pre_check_invalid_zip or pre_check_missing_manifest" -q
# 4 passed, 60 deselected, 1 warning in 3.08s

---

Checklist / 检查清单

• 😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
  / 如果 PR 中有新加入的功能，已经通过 Issue / 邮件等方式和作者讨论过。
  Not applicable: this is a bug fix, not a new feature.

• 👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
  / 我的更改经过了良好的测试，并已在上方提供了“验证步骤”和“运行截图”。

• 🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in requirements.txt and pyproject.toml.
  / 我确保没有引入新依赖库，或者引入新依赖库的同时将其添加到 requirements.txt 和 pyproject.toml 文件相应位置。

• 😮 My changes do not introduce malicious code.
  / 我的更改没有引入恶意代码。

Summary by Sourcery

Validate uploaded backup archives and ensure invalid ZIP files are rejected and cleaned up for both direct and chunked uploads.

Bug Fixes:

• Reject backup uploads whose archives lack a readable manifest.json object instead of silently accepting unusable backups.
• Ensure invalid direct backup uploads are cleaned up by deleting the partially saved ZIP file on failure.
• Ensure invalid chunked backup uploads are validated after merge and cleaned up by removing the merged ZIP file on failure.

Tests:

• Add async tests covering rejection and cleanup behavior for invalid direct backup uploads.
• Add async tests covering rejection and cleanup behavior for invalid chunked backup uploads.

[gemini-code-assist]

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

[sourcery-ai]

Hey - I've left some high level feedback:

• In upload_complete, if validate_uploaded_backup raises (e.g., invalid zip/manifest), the merged output_path file and upload session/chunk directory are left behind; consider mirroring the cleanup logic used in upload_backup so the merged file and session state are removed on validation failure.
• validate_uploaded_backup currently treats any non-dict manifest the same as a missing/invalid manifest; if you expect other JSON shapes (like lists or primitive types) it may be worth logging or surfacing a more specific error to aid debugging rather than silently returning None.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `upload_complete`, if `validate_uploaded_backup` raises (e.g., invalid zip/manifest), the merged `output_path` file and upload session/chunk directory are left behind; consider mirroring the cleanup logic used in `upload_backup` so the merged file and session state are removed on validation failure.
- `validate_uploaded_backup` currently treats any non-dict manifest the same as a missing/invalid manifest; if you expect other JSON shapes (like lists or primitive types) it may be worth logging or surfacing a more specific error to aid debugging rather than silently returning `None`.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[VectorPeak]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces validation for uploaded backup files by verifying the presence and format of manifest.json in both single-file and chunked upload flows, along with corresponding unit tests. The review feedback identifies a resource leak in chunked uploads where validation failure bypasses session cleanup, warns about potential exception masking during file deletion, and suggests adding assertions to verify session cleanup in tests.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

If validate_uploaded_backup raises a BackupServiceError, the execution jumps to the outer except Exception: block, which deletes the merged ZIP file but bypasses await self.cleanup_upload_session(upload_id). This leaves the temporary chunk files and the session in memory until they expire, causing a resource leak. Wrapping the validation in a try...except block ensures the session is cleaned up immediately on validation failure.

Suggested change

	self.validate_uploaded_backup(output_path)
	try:
	self.validate_uploaded_backup(output_path)
	except Exception:
	await self.cleanup_upload_session(upload_id)
	raise

[VectorPeak]

Addressed in commit 525f573. upload_complete() now wraps upload validation so invalid merged archives call cleanup_upload_session(upload_id) before re-raising, and the existing outer failure path still removes the merged ZIP output. The focused regression test now also asserts that the upload session is removed and the temporary chunk directory no longer exists.

[VectorPeak]

Addressed in commit 525f573. upload_complete() now wraps upload validation so invalid merged archives call cleanup_upload_session(upload_id) before re-raising, and the existing outer failure path still removes the merged ZIP output. The focused regression test now also asserts that the upload session is removed and the temporary chunk directory no longer exists.

[gemini-code-assist]

If os.remove(zip_path) raises an exception (such as PermissionError or FileNotFoundError), it will mask the original exception (e.g., BackupServiceError from validation failure). Wrapping the cleanup in a nested try...except block ensures that any secondary cleanup errors are logged but do not obscure the primary error.

        except Exception:
            try:
                if os.path.exists(zip_path):
                    os.remove(zip_path)
            except Exception as e:
                logger.warning(f"清理失败的备份文件失败: {e}")
            raise

[VectorPeak]

Addressed in commit 525f573. Direct upload cleanup now runs in a nested try/except, logs cleanup failures, and then re-raises the original validation/save failure so a secondary os.remove() error does not mask the primary error.

[gemini-code-assist]

Add an assertion to verify that the chunked upload session is successfully cleaned up from service.upload_sessions when validation fails.

Suggested change

	assert not (backup_dir / "bad.zip").exists()
	assert not (backup_dir / "bad.zip").exists()
	assert "upload-1" not in service.upload_sessions

[VectorPeak]

Added in commit 525f573. The invalid chunked upload regression now asserts both upload-1 not in service.upload_sessions and that the temporary chunk directory was removed.