Skip to content

Fix: Secure delete_upload against path traversal #41

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 9, 2025
Merged

Fix: Secure delete_upload against path traversal #41

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 34 additions & 9 deletions services/file_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import tempfile
import contextlib
import logging
from pathlib import Path
import hashlib
import asyncio
import mimetypes
Expand Down Expand Up @@ -228,17 +229,41 @@ def list_uploads() -> List[Dict[str, Any]]:


def delete_upload(name_or_path: str) -> bool:
"""Delete an upload by filename or absolute path. Returns True if deleted."""
if os.path.isabs(name_or_path):
path = name_or_path
else:
path = os.path.join(UPLOAD_DIR, name_or_path)
"""Delete an upload by filename or absolute path, ensuring it's within UPLOAD_DIR."""
logger = logging.getLogger(__name__)

try:
if os.path.exists(path):
os.remove(path)
return True
# Resolve UPLOAD_DIR and the target path
upload_dir_resolved = Path(UPLOAD_DIR).resolve()

# If name_or_path is absolute, ensure it's within UPLOAD_DIR
if os.path.isabs(name_or_path):
target_path = Path(name_or_path)
else:
# Construct path relative to UPLOAD_DIR
target_path = Path(UPLOAD_DIR) / name_or_path

target_resolved = target_path.resolve()

# 1. Ensure the resolved target starts with the resolved UPLOAD_DIR path
if not target_resolved.is_relative_to(upload_dir_resolved):
logger.warning("Attempt to delete file outside UPLOAD_DIR: %s", name_or_path)
return False

# 2. Verify the target is a regular file and not a directory or symlink pointing outside
if not target_resolved.is_file():
logger.warning("Attempt to delete non-file or invalid file type: %s", name_or_path)
return False

# If all checks pass, remove the file
os.remove(target_resolved)
logger.info("Successfully deleted upload: %s", name_or_path)
return True
Comment on lines +253 to +261
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid deleting symlink targets; unlink the symlink itself (safer semantics)

Current logic resolves then deletes the resolved path. If the path provided is a symlink inside UPLOAD_DIR to another file in UPLOAD_DIR, this will delete the target file and leave the symlink, which is surprising and risky. After validating the resolved path is within UPLOAD_DIR, prefer unlinking the symlink itself; otherwise delete the regular file.

Apply:

-        # 2. Verify the target is a regular file and not a directory or symlink pointing outside
-        if not target_resolved.is_file():
-            logger.warning("Attempt to delete non-file or invalid file type: %s", name_or_path)
-            return False
-
-        # If all checks pass, remove the file
-        os.remove(target_resolved)
-        logger.info("Successfully deleted upload: %s", name_or_path)
-        return True
+        # 2. Determine deletion behavior:
+        #    - If the provided path is a symlink, unlink the symlink itself (safe: does not touch target).
+        #    - Otherwise, ensure the resolved target is a regular file and remove it.
+        if target_path.is_symlink():
+            target_path.unlink()
+            logger.info("Successfully deleted upload symlink: %s", name_or_path)
+            return True
+
+        if not target_resolved.is_file():
+            logger.warning("Attempt to delete non-file or invalid file type: %s", name_or_path)
+            return False
+
+        os.remove(target_resolved)
+        logger.info("Successfully deleted upload: %s", name_or_path)
+        return True
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# 2. Verify the target is a regular file and not a directory or symlink pointing outside
if not target_resolved.is_file():
logger.warning("Attempt to delete non-file or invalid file type: %s", name_or_path)
return False
# If all checks pass, remove the file
os.remove(target_resolved)
logger.info("Successfully deleted upload: %s", name_or_path)
return True
# 2. Determine deletion behavior:
# - If the provided path is a symlink, unlink the symlink itself (safe: does not touch target).
# - Otherwise, ensure the resolved target is a regular file and remove it.
if target_path.is_symlink():
target_path.unlink()
logger.info("Successfully deleted upload symlink: %s", name_or_path)
return True
if not target_resolved.is_file():
logger.warning("Attempt to delete non-file or invalid file type: %s", name_or_path)
return False
os.remove(target_resolved)
logger.info("Successfully deleted upload: %s", name_or_path)
return True
🧰 Tools
🪛 Ruff (0.13.3)

261-261: Consider moving this statement to an else block

(TRY300)

except FileNotFoundError:
logger.info("Attempted to delete non-existent file: %s", name_or_path)
return False
except Exception:
logging.getLogger(__name__).exception("Failed to delete upload: %s", path)
logger.exception("Failed to delete upload: %s", name_or_path)
return False


Expand Down