fix: secure project update temp staging#9083
Conversation
dosubot
Bot
added
size:L
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The core update download progress callback now uses
self._make_progress_callback(progress_id, "core", 45, 45), which prevents the overall percentage from advancing beyond 45; consider restoring the original end value (e.g., 90) for correct progress reporting. - You unconditionally chmod the
updatesparent directory to0o700aftermkdir(exist_ok=True), which could unexpectedly change permissions on a pre-existing directory; if that's a concern, consider differentiating between newly created and existing directories or validating ownership/perms before chmod.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The core update download progress callback now uses `self._make_progress_callback(progress_id, "core", 45, 45)`, which prevents the overall percentage from advancing beyond 45; consider restoring the original end value (e.g., 90) for correct progress reporting.
- You unconditionally chmod the `updates` parent directory to `0o700` after `mkdir(exist_ok=True)`, which could unexpectedly change permissions on a pre-existing directory; if that's a concern, consider differentiating between newly created and existing directories or validating ownership/perms before chmod.
## Individual Comments
### Comment 1
<location path="astrbot/dashboard/services/update_service.py" line_range="202" />
<code_context>
- progress_callback=self._make_progress_callback(
+ update_temp_parent.mkdir(mode=0o700, parents=True, exist_ok=True)
+ update_temp_parent.chmod(0o700)
+ with tempfile.TemporaryDirectory(
+ prefix="project-update-",
+ dir=update_temp_parent,
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the verification logic and the whole download–verify–apply update workflow into separate helper methods to reduce nesting inside `_run_update_project` while preserving current behavior.
The added `TemporaryDirectory` context and inner `_verify_update_packages` do increase local nesting. You can keep all behavior (including secure temp dir and automatic cleanup) while reducing complexity by (1) extracting the verification helper, and (2) moving the monolithic update sequence into a dedicated helper that’s called from inside the `with` block.
**1. Move `_verify_update_packages` out of `_run_update_project`**
Make it a small private method; this trims the body of `_run_update_project` and avoids inner function nesting:
```python
def _verify_update_packages(self, dashboard_zip_path: Path, core_zip_path: Path) -> None:
for zip_path in (dashboard_zip_path, core_zip_path):
with zipfile.ZipFile(zip_path, "r") as archive:
corrupt_member = archive.testzip()
if corrupt_member:
raise UpdateServiceError(f"更新包校验失败: {corrupt_member}")
```
Usage in `_run_update_project`:
```python
self._set_update_stage(progress_id, "verify", "running", "下载完成，正在校验更新包...", 90)
await asyncio.to_thread(self._verify_update_packages, dashboard_zip_path, core_zip_path)
self._set_update_stage(progress_id, "verify", "done", "更新包校验完成。", 91)
```
**2. Extract the update sequence into a helper called from the temp-dir context**
Keep `TemporaryDirectory` but reduce indentation by moving the “download → verify → apply → dependencies → reboot” pipeline into a separate method. The `with` block only sets up paths and calls the helper.
```python
async def _perform_project_update(
self,
progress_id: str,
dashboard_zip_path: Path,
core_zip_path: Path,
latest: bool,
version: str,
proxy: str,
reboot: bool,
) -> None:
self._set_update_stage(progress_id, "dashboard", "running", "正在下载 WebUI...", 0)
await self.download_dashboard(
path=str(dashboard_zip_path),
latest=latest,
version=version,
proxy=proxy,
progress_callback=self._make_progress_callback(progress_id, "dashboard", 0, 45),
extract=False,
)
self._set_update_stage(progress_id, "dashboard", "done", "WebUI 下载完成。", 45)
self._set_update_stage(progress_id, "core", "running", "正在下载 AstrBot 项目代码...", 45)
core_zip_actual = Path(
await self.astrbot_updator.download_update_package(
latest=latest,
version=version,
proxy=proxy,
path=core_zip_path,
progress_callback=self._make_progress_callback(progress_id, "core", 45, 45),
)
)
core_zip_path = core_zip_actual # keep existing behavior
# ... verify, apply, dependencies, reboot and progress updates as in current code ...
```
Then `_run_update_project` becomes:
```python
async def _run_update_project(
self,
progress_id: str,
version: str,
latest: bool,
reboot: bool,
proxy: str | None,
) -> None:
update_temp_parent = Path(get_astrbot_temp_path()) / "updates"
try:
update_temp_parent.mkdir(mode=0o700, parents=True, exist_ok=True)
update_temp_parent.chmod(0o700)
with tempfile.TemporaryDirectory(
prefix="project-update-",
dir=update_temp_parent,
) as update_temp_dir_name:
update_temp_dir = Path(update_temp_dir_name)
update_token = uuid.uuid4().hex
dashboard_zip_path = update_temp_dir / f"{update_token}-dashboard.zip"
core_zip_path = update_temp_dir / f"{update_token}-core.zip"
await self._perform_project_update(
progress_id=progress_id,
dashboard_zip_path=dashboard_zip_path,
core_zip_path=core_zip_path,
latest=latest,
version=version,
proxy=proxy or "",
reboot=reboot,
)
except asyncio.CancelledError:
...
except Exception as exc:
...
```
This keeps the secure temp-dir semantics and automatic cleanup, but flattens `_run_update_project` into “setup temp dir + call helper + error handling,” making the core workflow easier to read and test.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| progress_callback=self._make_progress_callback( | ||
| update_temp_parent.mkdir(mode=0o700, parents=True, exist_ok=True) | ||
| update_temp_parent.chmod(0o700) | ||
| with tempfile.TemporaryDirectory( |
There was a problem hiding this comment.
issue (complexity): Consider extracting the verification logic and the whole download–verify–apply update workflow into separate helper methods to reduce nesting inside _run_update_project while preserving current behavior.
The added TemporaryDirectory context and inner _verify_update_packages do increase local nesting. You can keep all behavior (including secure temp dir and automatic cleanup) while reducing complexity by (1) extracting the verification helper, and (2) moving the monolithic update sequence into a dedicated helper that’s called from inside the with block.
1. Move _verify_update_packages out of _run_update_project
Make it a small private method; this trims the body of _run_update_project and avoids inner function nesting:
def _verify_update_packages(self, dashboard_zip_path: Path, core_zip_path: Path) -> None:
for zip_path in (dashboard_zip_path, core_zip_path):
with zipfile.ZipFile(zip_path, "r") as archive:
corrupt_member = archive.testzip()
if corrupt_member:
raise UpdateServiceError(f"更新包校验失败: {corrupt_member}")Usage in _run_update_project:
self._set_update_stage(progress_id, "verify", "running", "下载完成，正在校验更新包...", 90)
await asyncio.to_thread(self._verify_update_packages, dashboard_zip_path, core_zip_path)
self._set_update_stage(progress_id, "verify", "done", "更新包校验完成。", 91)2. Extract the update sequence into a helper called from the temp-dir context
Keep TemporaryDirectory but reduce indentation by moving the “download → verify → apply → dependencies → reboot” pipeline into a separate method. The with block only sets up paths and calls the helper.
async def _perform_project_update(
self,
progress_id: str,
dashboard_zip_path: Path,
core_zip_path: Path,
latest: bool,
version: str,
proxy: str,
reboot: bool,
) -> None:
self._set_update_stage(progress_id, "dashboard", "running", "正在下载 WebUI...", 0)
await self.download_dashboard(
path=str(dashboard_zip_path),
latest=latest,
version=version,
proxy=proxy,
progress_callback=self._make_progress_callback(progress_id, "dashboard", 0, 45),
extract=False,
)
self._set_update_stage(progress_id, "dashboard", "done", "WebUI 下载完成。", 45)
self._set_update_stage(progress_id, "core", "running", "正在下载 AstrBot 项目代码...", 45)
core_zip_actual = Path(
await self.astrbot_updator.download_update_package(
latest=latest,
version=version,
proxy=proxy,
path=core_zip_path,
progress_callback=self._make_progress_callback(progress_id, "core", 45, 45),
)
)
core_zip_path = core_zip_actual # keep existing behavior
# ... verify, apply, dependencies, reboot and progress updates as in current code ...Then _run_update_project becomes:
async def _run_update_project(
self,
progress_id: str,
version: str,
latest: bool,
reboot: bool,
proxy: str | None,
) -> None:
update_temp_parent = Path(get_astrbot_temp_path()) / "updates"
try:
update_temp_parent.mkdir(mode=0o700, parents=True, exist_ok=True)
update_temp_parent.chmod(0o700)
with tempfile.TemporaryDirectory(
prefix="project-update-",
dir=update_temp_parent,
) as update_temp_dir_name:
update_temp_dir = Path(update_temp_dir_name)
update_token = uuid.uuid4().hex
dashboard_zip_path = update_temp_dir / f"{update_token}-dashboard.zip"
core_zip_path = update_temp_dir / f"{update_token}-core.zip"
await self._perform_project_update(
progress_id=progress_id,
dashboard_zip_path=dashboard_zip_path,
core_zip_path=core_zip_path,
latest=latest,
version=version,
proxy=proxy or "",
reboot=reboot,
)
except asyncio.CancelledError:
...
except Exception as exc:
...This keeps the secure temp-dir semantics and automatic cleanup, but flattens _run_update_project into “setup temp dir + call helper + error handling,” making the core workflow easier to read and test.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the update process in update_service.py to use tempfile.TemporaryDirectory for managing temporary update files securely and automatically. Feedback was provided regarding two main issues: a potential security vulnerability where update_temp_parent could be a symlink (meaning chmod would follow it and modify permissions of the target file/directory), and a potential reliability issue where automatic cleanup by tempfile.TemporaryDirectory might raise exceptions (e.g., due to file locks on Windows) and propagate, causing a successful update to be marked as failed. Explicitly handling cleanup in a try...finally block is recommended.
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.
| "overall_percent": 100, | ||
| }, | ||
| ) | ||
| logger.info(message) |
There was a problem hiding this comment.
Using with tempfile.TemporaryDirectory(...) means any exception raised during the automatic cleanup (e.g., due to file locks on Windows or antivirus interference) will propagate out of the block and be caught by the outer except Exception as exc block. This would mark a successful update as failed in the UI, leading to a bad user experience. Handling the cleanup explicitly in a try...finally block and catching any cleanup errors allows us to log them as warnings without failing the entire update process.
logger.info(message)
finally:
try:
update_temp_dir_obj.cleanup()
except Exception as cleanup_exc:
logger.warning(f"清理更新临时目录失败: {cleanup_exc}")
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
LGTM |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
astrbot-docs | f6a1fc3 | Commit Preview URL Branch Preview URL |
Jun 30 2026, 06:16 AM |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The progress callback for the core package download now uses
start=45, end=45, which prevents the overall progress from advancing during that stage; consider restoring the previous range (e.g. 45–90) so users see meaningful progress. - In the setup of
update_temp_parent, only symlinks are unlinked; if an attacker or misconfiguration creates a regular file at that path,mkdirwill fail—consider handling non-directory cases explicitly (e.g. unlinking or raising a clear error) beforemkdir.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The progress callback for the core package download now uses `start=45, end=45`, which prevents the overall progress from advancing during that stage; consider restoring the previous range (e.g. 45–90) so users see meaningful progress.
- In the setup of `update_temp_parent`, only symlinks are unlinked; if an attacker or misconfiguration creates a regular file at that path, `mkdir` will fail—consider handling non-directory cases explicitly (e.g. unlinking or raising a clear error) before `mkdir`.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
* fix: secure project update temp staging * Update astrbot/dashboard/services/update_service.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * fix: close update staging temp context --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Motivation
get_astrbot_system_tmp_path) that could be pre-created or symlinked by an attacker, allowing replacement of verified ZIPs before application.Description
get_astrbot_temp_path()andtempfile.TemporaryDirectoryso each update run gets an isolated directory.updatesparent directory is created with restrictive permissions usingmkdir(mode=0o700, ...)andchmod(0o700)before creating the per-update temp dir, and placedashboard/coreZIPs inside that per-update directory.import tempfileand adjust the_run_update_projectflow to use the scopedTemporaryDirectory(automatic cleanup) and remove the previous manualfinallyZIP cleanup block while preserving the download, verification (zipfile.testzip()), apply, extraction, dependency install, and restart behavior.Testing
uv run --no-sync ruff format .anduv run --no-sync ruff check .and they passed.uv run --no-sync python -m py_compile astrbot/dashboard/services/update_service.pyand it succeeded.git diff --checkto validate no leftover whitespace/errors and it passed.Codex Task
Summary by Sourcery
Secure the project update staging flow by isolating update archives in a per-run temporary directory under AstrBot’s data temp path with restricted permissions and relying on automatic cleanup.
Bug Fixes:
Enhancements: