diff --git a/app/api/files.py b/app/api/files.py index 58e0d834..e90e8d3d 100644 --- a/app/api/files.py +++ b/app/api/files.py @@ -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()) diff --git a/app/utils/filename_utils.py b/app/utils/filename_utils.py index b4cf5ad5..a491613a 100644 --- a/app/utils/filename_utils.py +++ b/app/utils/filename_utils.py @@ -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 == ".": diff --git a/tests/test_file_upload.py b/tests/test_file_upload.py index 07243a47..24feacb1 100644 --- a/tests/test_file_upload.py +++ b/tests/test_file_upload.py @@ -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