Fix: Secure delete_upload against path traversal #41
Conversation
WalkthroughRefactors delete_upload in services/file_service.py to perform safe, resolved path handling within UPLOAD_DIR, verify targets are files, and add logging for attempts outside the upload directory, non-file targets, success, not found, and other exceptions. Imports pathlib.Path and updates docstring. Signature remains unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant FS as FileService.delete_upload()
participant P as Path/FS
participant L as Logger
C->>FS: delete_upload(name_or_path)
FS->>P: Resolve UPLOAD_DIR
FS->>P: Build target_path (abs or relative to UPLOAD_DIR)
FS->>P: Resolve target_path
FS->>P: Check target_path is within UPLOAD_DIR
alt Target outside UPLOAD_DIR
FS->>L: warn("Outside upload dir")
FS-->>C: False
else Within UPLOAD_DIR
FS->>P: is_file?
alt Not a regular file
FS->>L: warn("Not a file")
FS-->>C: False
else Is a file
FS->>P: unlink()
FS->>L: info("Deleted")
FS-->>C: True
end
end
opt Exceptions
alt FileNotFoundError
FS->>L: info("Not found")
FS-->>C: False
else Other Exception
FS->>L: exception("Deletion failed")
FS-->>C: False
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
services/file_service.py (3)
236-247: Use Path API consistently for absolute checkMinor polish: prefer Path(...).is_absolute() over os.path.isabs for consistency with pathlib usage.
Apply:
- if os.path.isabs(name_or_path): + if Path(name_or_path).is_absolute(): target_path = Path(name_or_path) else: # Construct path relative to UPLOAD_DIR target_path = Path(UPLOAD_DIR) / name_or_path
262-267: Tighten control flow per TRY300Move the trailing return into the except or drop the final return. This satisfies the linter and clarifies flow.
Apply:
- except Exception: - logger.exception("Failed to delete upload: %s", name_or_path) - return False + except Exception: + logger.exception("Failed to delete upload: %s", name_or_path) + return False
248-252: Fallback for Python < 3.9: confirm minimum Python version or add try/exceptUse
Path.relative_towithValueErrorfallback unless you guarantee Python ≥ 3.9:- if not target_resolved.is_relative_to(upload_dir_resolved): + try: + target_resolved.relative_to(upload_dir_resolved) + except ValueError: logger.warning("Attempt to delete file outside UPLOAD_DIR: %s", name_or_path) return FalseEnsure your project metadata or CI specifies Python ≥ 3.9.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/file_service.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.13.3)
services/file_service.py
261-261: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (2)
services/file_service.py (2)
12-12: Import looks goodUsing pathlib.Path is appropriate here.
232-234: Docstring and logger init LGTMClear intent and consistent logging setup.
| # 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 |
There was a problem hiding this comment.
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.
| # 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)
2 participants
This change addresses a critical security vulnerability in the
delete_uploadfunction withinservices/file_service.py. Previously, the function accepted absolute paths, allowing for potential path traversal attacks where an attacker could delete arbitrary files outside the intendedUPLOAD_DIR.Changes Made:
delete_uploadfunction now strictly resolves the requested file path against theUPLOAD_DIR.UPLOAD_DIRand the target path are resolved usingPath.resolve()to normalize paths and handle symlinks.UPLOAD_DIR.is_file()) to prevent accidental deletion of directories or following malicious symlinks.Falseimmediately, preventing unauthorized file operations.Motivation:
To enhance the security posture of the application by preventing unauthorized file deletion and ensuring that file operations are strictly confined to the designated upload directory.
Summary by CodeRabbit