Repository navigation
Conversation
![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 5 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/io.py" line_range="360" />
<code_context>
+ show_downloading_label=False,
+ )
+ if target_path.exists():
+ shutil.copymode(target_path, staging_path)
+ staging_path.replace(target_path)
if show_progress:
</code_context>
<issue_to_address>
**Downloaded bytes gain elevated privileges**
When a privileged downloader replaces a destination with setuid or setgid bits, `shutil.copymode` copies those bits to the staging file, and `staging_path.replace` publishes the downloaded contents with them, creating a setuid/setgid executable from untrusted bytes.
Clear setuid and setgid bits from the staging file before publishing it.
</issue_to_address>
### Comment 2
<location path="astrbot/core/utils/io.py" line_range="361" />
<code_context>
+ )
+ if target_path.exists():
+ shutil.copymode(target_path, staging_path)
+ staging_path.replace(target_path)
if show_progress:
print()
</code_context>
<issue_to_address>
**Read-only destinations become replaceable**
When the destination is non-writable but its parent directory is writable, `download_file` writes only to the staging file, then replaces the destination using the parent directory's permissions. On POSIX, a caller that cannot write an existing file can still overwrite it when the parent directory is writable, bypassing file-level write protection.
Check and enforce the existing destination's write authorization before publishing, rejecting the download when the caller could not write the destination directly.
</issue_to_address>
### Comment 3
<location path="astrbot/core/utils/io.py" line_range="360" />
<code_context>
+ show_downloading_label=False,
+ )
+ if target_path.exists():
+ shutil.copymode(target_path, staging_path)
+ staging_path.replace(target_path)
if show_progress:
</code_context>
<issue_to_address>
**Existing access metadata is lost**
When the destination relies on ownership, ACLs, or extended attributes beyond its permission bits, `shutil.copymode` copies only mode bits, and `staging_path.replace` installs a new inode without the destination's other access metadata, changing who can access the completed download.
Preserve the destination's ownership, ACLs, and extended attributes when publishing the downloaded file.
Also at `astrbot/core/utils/io.py:361-362`.
</issue_to_address>
### Comment 4
<location path="astrbot/core/utils/io.py" line_range="361" />
<code_context>
+ )
+ if target_path.exists():
+ shutil.copymode(target_path, staging_path)
+ staging_path.replace(target_path)
if show_progress:
print()
</code_context>
<issue_to_address>
**Older download replaces newer result**
When two successful downloads to the same destination finish in the reverse order from their start, the later-finishing call replaces the destination regardless of which download was started most recently, so an older response can overwrite a newer completed file and callers see stale content.
Serialize publication per destination or reject publication from calls superseded by a newer request.
</issue_to_address>
### Comment 5
<location path="astrbot/core/utils/io.py" line_range="304-306" />
<code_context>
+ # Resolve symlinks as open(path, "wb") does, and stage on the same filesystem
+ # so replacement is atomic. Each call owns its own temporary directory.
+ target_path = Path(path).resolve()
+ with tempfile.TemporaryDirectory(
+ prefix=".astrbot-download-", dir=target_path.parent
+ ) as staging_dir:
+ staging_path = Path(staging_dir) / "download.part"
+ try:
</code_context>
<issue_to_address>
**Writable file cannot be updated**
When the destination is writable but its parent directory does not permit creating entries, `TemporaryDirectory` raises `PermissionError` before the download because it must create a sibling entry, so the existing file is not updated.
Use a staging approach that does not require creating an entry in the destination's parent directory.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 5 findings to address first, and a completed download is published by replacing the destination, so if the received file is wrong, the previous file’s contents are no longer available at that path; reverting will not restore them. The damage is bounded and usually repairable by downloading the file again.
Blocking findings: astrbot/core/utils/io.py:360, astrbot/core/utils/io.py:361, astrbot/core/utils/io.py:360, astrbot/core/utils/io.py:361, astrbot/core/utils/io.py:306
| show_downloading_label=False, | ||
| ) | ||
| if target_path.exists(): | ||
| shutil.copymode(target_path, staging_path) |
There was a problem hiding this comment.
🟠 High · Downloaded bytes gain elevated privileges
When a privileged downloader replaces a destination with setuid or setgid bits, shutil.copymode copies those bits to the staging file, and staging_path.replace publishes the downloaded contents with them, creating a setuid/setgid executable from untrusted bytes.
Clear setuid and setgid bits from the staging file before publishing it.
Prompt for AI agents
In `astrbot/core/utils/io.py` at line 360:
**Downloaded bytes gain elevated privileges**
When a privileged downloader replaces a destination with setuid or setgid bits, `shutil.copymode` copies those bits to the staging file, and `staging_path.replace` publishes the downloaded contents with them, creating a setuid/setgid executable from untrusted bytes.
Clear setuid and setgid bits from the staging file before publishing it.| ) | ||
| if target_path.exists(): | ||
| shutil.copymode(target_path, staging_path) | ||
| staging_path.replace(target_path) |
There was a problem hiding this comment.
🟡 Medium · Read-only destinations become replaceable
When the destination is non-writable but its parent directory is writable, download_file writes only to the staging file, then replaces the destination using the parent directory's permissions. On POSIX, a caller that cannot write an existing file can still overwrite it when the parent directory is writable, bypassing file-level write protection.
Check and enforce the existing destination's write authorization before publishing, rejecting the download when the caller could not write the destination directly.
Prompt for AI agents
In `astrbot/core/utils/io.py` at line 361:
**Read-only destinations become replaceable**
When the destination is non-writable but its parent directory is writable, `download_file` writes only to the staging file, then replaces the destination using the parent directory's permissions. On POSIX, a caller that cannot write an existing file can still overwrite it when the parent directory is writable, bypassing file-level write protection.
Check and enforce the existing destination's write authorization before publishing, rejecting the download when the caller could not write the destination directly.| show_downloading_label=False, | ||
| ) | ||
| if target_path.exists(): | ||
| shutil.copymode(target_path, staging_path) |
There was a problem hiding this comment.
🟡 Medium · Existing access metadata is lost
When the destination relies on ownership, ACLs, or extended attributes beyond its permission bits, shutil.copymode copies only mode bits, and staging_path.replace installs a new inode without the destination's other access metadata, changing who can access the completed download.
Preserve the destination's ownership, ACLs, and extended attributes when publishing the downloaded file.
Also at astrbot/core/utils/io.py:361-362.
Prompt for AI agents
In `astrbot/core/utils/io.py` at line 360:
**Existing access metadata is lost**
When the destination relies on ownership, ACLs, or extended attributes beyond its permission bits, `shutil.copymode` copies only mode bits, and `staging_path.replace` installs a new inode without the destination's other access metadata, changing who can access the completed download.
Preserve the destination's ownership, ACLs, and extended attributes when publishing the downloaded file.
Also at `astrbot/core/utils/io.py:361-362`.| ) | ||
| if target_path.exists(): | ||
| shutil.copymode(target_path, staging_path) | ||
| staging_path.replace(target_path) |
There was a problem hiding this comment.
🟡 Medium · Older download replaces newer result
When two successful downloads to the same destination finish in the reverse order from their start, the later-finishing call replaces the destination regardless of which download was started most recently, so an older response can overwrite a newer completed file and callers see stale content.
Serialize publication per destination or reject publication from calls superseded by a newer request.
Prompt for AI agents
In `astrbot/core/utils/io.py` at line 361:
**Older download replaces newer result**
When two successful downloads to the same destination finish in the reverse order from their start, the later-finishing call replaces the destination regardless of which download was started most recently, so an older response can overwrite a newer completed file and callers see stale content.
Serialize publication per destination or reject publication from calls superseded by a newer request.| with tempfile.TemporaryDirectory( | ||
| prefix=".astrbot-download-", dir=target_path.parent | ||
| ) as staging_dir: |
There was a problem hiding this comment.
🟡 Medium · Writable file cannot be updated
When the destination is writable but its parent directory does not permit creating entries, TemporaryDirectory raises PermissionError before the download because it must create a sibling entry, so the existing file is not updated.
Use a staging approach that does not require creating an entry in the destination's parent directory.
Prompt for AI agents
In `astrbot/core/utils/io.py` at lines 304-306:
**Writable file cannot be updated**
When the destination is writable but its parent directory does not permit creating entries, `TemporaryDirectory` raises `PermissionError` before the download because it must create a sibling entry, so the existing file is not updated.
Use a staging approach that does not require creating an entry in the destination's parent directory.
The shared
io.download_file()helper opens the destination with"wb"before the response body finishes. A disconnect, timeout, cancellation, or progress callback failure leaves a partial file behind and destroys any previously complete download at that path.Modifications
Path.replace()after the response and progress callbacks complete.Existing PRs #9459 and #10125 address TLS policy and SSRF respectively; this change addresses destination integrity and leaves network policy unchanged. No new dependencies, public API signatures, or frontend changes.
Screenshots or Test Results
Windows, Python 3.12:
tests/unit/test_io_download_file.pytests/test_media_utils.pytests/unit/test_file_message_component.pytests/test_updater_socks.pytests/unit/test_diagnostic_redaction.pyuv run ruff format .— 524 files unchanged.uv run ruff check .— passed.tests/unit/test_io_download_file.py— passed.uvx --from pyupgrade==3.21.0 pyupgrade --py310-plus astrbot/core/utils/io.py tests/unit/test_io_download_file.py— passed.git diff --check— passed.The related tests were run with the
pytest.main(...), stream flush andos._exit(...)pattern already used byscripts/run_pytests_ci.sh. The full repository suite and other operating systems were not run locally.Checklist
Summary by Sourcery
Make file downloads publish atomically so failed or cancelled transfers leave no partial files and do not corrupt existing results.
Bug Fixes:
Enhancements:
Tests: