fix(security): enhance path traversal protection in file uploads
- Import and use sanitize_filename utility in ui_upload endpoint - Enhance sanitize_filename to handle Windows-style paths (backslashes) - Add protection against path traversal patterns (..) - Replace all path separators with underscores - Add comprehensive security tests for Windows-style paths and mixed separators - All existing tests pass with improved security This addresses the "Uncontrolled data used in path expression" code scanning alert by ensuring all user-provided filenames are properly sanitized before being used in any file operations or stored in the database. Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com>
This commit is contained in:
+5
-1
@@ -19,6 +19,7 @@ from app.models import FileRecord, ProcessingLog
|
||||
from app.tasks.convert_to_pdf import convert_to_pdf
|
||||
from app.tasks.process_document import process_document
|
||||
from app.utils.file_status import get_files_processing_status
|
||||
from app.utils.filename_utils import sanitize_filename
|
||||
|
||||
# Set up logging
|
||||
logger = logging.getLogger(__name__)
|
||||
@@ -657,7 +658,10 @@ async def ui_upload(request: Request, file: UploadFile = File(...)):
|
||||
workdir = settings.workdir
|
||||
|
||||
# Extract just the filename without any path components to prevent path traversal
|
||||
safe_filename = os.path.basename(file.filename)
|
||||
# First, use basename to remove any directory components
|
||||
base_filename = os.path.basename(file.filename)
|
||||
# Then sanitize the filename to remove special characters and ensure filesystem compatibility
|
||||
safe_filename = sanitize_filename(base_filename)
|
||||
|
||||
# Generate a unique filename with UUID to prevent overwriting and filename conflicts
|
||||
unique_id = str(uuid.uuid4())
|
||||
|
||||
@@ -71,24 +71,38 @@ def get_unique_filename(original_path, check_exists_func=None):
|
||||
|
||||
def sanitize_filename(filename):
|
||||
"""
|
||||
Sanitize a filename to ensure it's valid across different file systems.
|
||||
Sanitize a filename to ensure it's valid across different file systems
|
||||
and prevent path traversal attacks.
|
||||
|
||||
Args:
|
||||
filename (str): The filename to sanitize
|
||||
|
||||
Returns:
|
||||
str: A sanitized filename
|
||||
|
||||
Security:
|
||||
- Removes path separators (/ and \)
|
||||
- Prevents path traversal patterns (..)
|
||||
- Replaces problematic characters with underscores
|
||||
- Ensures compatibility across Windows, Linux, and macOS
|
||||
"""
|
||||
# First, replace all path separators (both Unix and Windows style) with underscores
|
||||
sanitized = filename.replace("/", "_").replace("\\", "_")
|
||||
|
||||
# Replace characters that are problematic in various filesystems
|
||||
# Keep only alphanumeric, dash, underscore, period, and space
|
||||
sanitized = re.sub(r"[^\w\-\. ]", "_", filename)
|
||||
sanitized = re.sub(r"[^\w\-\. ]", "_", sanitized)
|
||||
|
||||
# Remove or replace path traversal patterns
|
||||
# Replace consecutive dots with a single underscore to prevent .. patterns
|
||||
sanitized = re.sub(r"\.\.+", "_", sanitized)
|
||||
|
||||
# Replace multiple spaces/underscores with single ones
|
||||
sanitized = re.sub(r"__+", "_", sanitized)
|
||||
sanitized = re.sub(r" +", " ", sanitized)
|
||||
|
||||
# Trim leading/trailing spaces and periods which cause issues in Windows
|
||||
sanitized = sanitized.strip(". ")
|
||||
# Trim leading/trailing spaces, periods, and underscores which cause issues in Windows
|
||||
sanitized = sanitized.strip(". _")
|
||||
|
||||
# Ensure the filename isn't empty after sanitization
|
||||
if not sanitized or sanitized == ".":
|
||||
|
||||
@@ -272,10 +272,54 @@ class TestUploadSecurity:
|
||||
assert response.status_code == 200
|
||||
data = response.json()
|
||||
|
||||
# Original filename should be preserved (sanitized by basename)
|
||||
assert data["original_filename"] == special_filename
|
||||
# Original filename should be sanitized (special chars replaced with underscores)
|
||||
assert "!" not in data["original_filename"]
|
||||
assert "@" not in data["original_filename"]
|
||||
assert "#" not in data["original_filename"]
|
||||
# Spaces and basic characters should be preserved
|
||||
assert "file" in data["original_filename"]
|
||||
assert ".pdf" in data["original_filename"]
|
||||
# Stored filename should have UUID and extension
|
||||
assert data["stored_filename"].endswith(".pdf")
|
||||
|
||||
def test_path_traversal_prevention_windows_style(self, client: TestClient, mock_celery_tasks):
|
||||
"""Test that Windows-style path traversal attempts are prevented."""
|
||||
# Try Windows-style path with backslashes
|
||||
malicious_filename = "..\\..\\..\\windows\\system32\\config.pdf"
|
||||
pdf_content = b"%PDF-1.4\n%EOF"
|
||||
|
||||
response = client.post(
|
||||
"/api/ui-upload",
|
||||
files={"file": (malicious_filename, io.BytesIO(pdf_content), "application/pdf")}
|
||||
)
|
||||
|
||||
assert response.status_code == 200
|
||||
data = response.json()
|
||||
|
||||
# Backslashes should be removed or replaced
|
||||
assert "\\" not in data["original_filename"]
|
||||
assert ".." not in data["original_filename"]
|
||||
# Should contain sanitized version
|
||||
assert "config.pdf" in data["original_filename"]
|
||||
|
||||
def test_path_traversal_prevention_mixed_separators(self, client: TestClient, mock_celery_tasks):
|
||||
"""Test handling of filenames with mixed path separators."""
|
||||
malicious_filename = "../path\\to/file.pdf"
|
||||
pdf_content = b"%PDF-1.4\n%EOF"
|
||||
|
||||
response = client.post(
|
||||
"/api/ui-upload",
|
||||
files={"file": (malicious_filename, io.BytesIO(pdf_content), "application/pdf")}
|
||||
)
|
||||
|
||||
assert response.status_code == 200
|
||||
data = response.json()
|
||||
|
||||
# Should only keep the filename without any path components
|
||||
assert "/" not in data["original_filename"]
|
||||
assert "\\" not in data["original_filename"]
|
||||
assert ".." not in data["original_filename"]
|
||||
assert "file.pdf" in data["original_filename"]
|
||||
|
||||
|
||||
@pytest.mark.integration
|
||||
|
||||
Reference in New Issue
Block a user