fix: prevent path traversal in file uploads

[bugkeep]

Fixes #7745.

• Sanitize uploaded filename (handles both / and \ separators)
• Enforce that the resolved save path stays within attachments_dir
• Add unit tests for filename sanitization

Summary by Sourcery

Harden file upload handling to prevent directory traversal and ensure attachments are stored within the configured attachments directory.

Bug Fixes:

• Sanitize uploaded filenames to strip path components and unsafe traversal patterns across POSIX and Windows-style paths.
• Validate the resolved file path is under the attachments directory before saving uploaded files, rejecting invalid filenames instead.

Tests:

• Add unit tests covering filename sanitization and traversal stripping for various path formats and empty values.

[sourcery-ai]

Hey - I've found 2 issues, and left some high level feedback:

• In _sanitize_upload_filename, consider rejecting or fully stripping any "\x00" characters anywhere in the string (not just leading/trailing) to avoid potential edge cases with embedded NUL bytes in filenames.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `_sanitize_upload_filename`, consider rejecting or fully stripping any `"\x00"` characters anywhere in the string (not just leading/trailing) to avoid potential edge cases with embedded NUL bytes in filenames.

## Individual Comments

### Comment 1
<location path="astrbot/dashboard/routes/chat.py" line_range="36-40" />
<code_context>
 SSE_HEARTBEAT = ": heartbeat\n\n"


+def _sanitize_upload_filename(filename: str | None) -> str:
+    if not filename:
+        return f"{uuid.uuid4()!s}"
+    normalized = filename.replace("\\", "/")
+    name = PurePosixPath(normalized).name.strip().strip("\x00")
+    if name in ("", ".", ".."):
+        return f"{uuid.uuid4()!s}"
</code_context>
<issue_to_address>
**🚨 issue (security):** Null byte sanitization only strips leading/trailing occurrences and may leave embedded nulls in the filename.

`strip("\x00")` only removes nulls at the start/end of the basename, so something like `"evil\x00.txt"` would keep the embedded null. Depending on how `file.save` handles this, it could lead to unexpected behavior. Consider using `replace("\x00", "")` (and any other needed normalization) so nulls are removed from all positions in the filename.
</issue_to_address>

### Comment 2
<location path="astrbot/dashboard/routes/chat.py" line_range="361-362" />
<code_context>
-        path = os.path.join(self.attachments_dir, filename)
-        await file.save(path)
+        attachments_dir = Path(self.attachments_dir).resolve(strict=False)
+        file_path = (attachments_dir / filename).resolve(strict=False)
+        if not file_path.is_relative_to(attachments_dir):
+            return Response().error("Invalid filename").__dict__
+
</code_context>
<issue_to_address>
**question (bug_risk):** Use of `Path.is_relative_to` constrains the minimum supported Python version.

This API is only available in Python 3.9+, so on 3.8 or older this will raise an `AttributeError` at runtime. If you still support those versions, emulate the check instead (e.g. `attachments_dir in file_path.parents`). Otherwise, just confirm that 3.9+ is the documented minimum in your compatibility matrix.
</issue_to_address>

---

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.}

[sourcery-ai]

🚨 issue (security): Null byte sanitization only strips leading/trailing occurrences and may leave embedded nulls in the filename.

strip("\x00") only removes nulls at the start/end of the basename, so something like "evil\x00.txt" would keep the embedded null. Depending on how file.save handles this, it could lead to unexpected behavior. Consider using replace("\x00", "") (and any other needed normalization) so nulls are removed from all positions in the filename.

[sourcery-ai]

question (bug_risk): Use of Path.is_relative_to constrains the minimum supported Python version.

This API is only available in Python 3.9+, so on 3.8 or older this will raise an AttributeError at runtime. If you still support those versions, emulate the check instead (e.g. attachments_dir in file_path.parents). Otherwise, just confirm that 3.9+ is the documented minimum in your compatibility matrix.

[gemini-code-assist]

Code Review

This pull request introduces filename sanitization and path traversal protection for file uploads in the chat dashboard. It adds a _sanitize_upload_filename utility to handle potentially malicious filenames and updates the post_file endpoint to verify that saved files remain within the designated attachments directory. Unit tests for the sanitization logic were also included. A review comment suggests a more robust approach to null byte removal by replacing them throughout the filename instead of only stripping them from the ends.

[gemini-code-assist]

The current implementation uses .strip("\x00"), which only removes null bytes from the beginning and end of the filename. If a null byte is present in the middle of the filename (e.g., "test.php\x00.jpg"), it will remain. While Python's file APIs typically raise a ValueError when encountering embedded null characters, it is more robust to remove them entirely to prevent potential issues with other system APIs or unexpected behavior. Using .replace("\x00", "") followed by .strip() ensures that null bytes are removed from anywhere in the string and that any resulting trailing whitespace is cleaned up. Additionally, ensure this functionality is accompanied by corresponding unit tests.

Suggested change

	name = PurePosixPath(normalized).name.strip().strip("\x00")
	name = PurePosixPath(normalized).name.replace("\x00", "").strip()

References

1. New functionality, such as handling attachments, should be accompanied by corresponding unit tests.