Merge pull request #167 from christianlouis/copilot/fix-uncontrolled-data-alert

Harden filename sanitization against path traversal attacks

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())

app/utils/filename_utils.py

@@ -70,25 +70,39 @@ def get_unique_filename(original_path, check_exists_func=None):
 
 
 def sanitize_filename(filename):
-    """
+    r"""
-    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 specifically '..' to prevent path traversal while preserving single dots
+    sanitized = sanitized.replace("..", "_")
 
     # 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
+    # Trim leading/trailing spaces, periods, and underscores which cause issues in Windows
-    sanitized = sanitized.strip(". ")
+    sanitized = sanitized.strip(". _")
 
     # Ensure the filename isn't empty after sanitization
     if not sanitized or sanitized == ".":

tests/test_file_upload.py

@@ -9,6 +9,7 @@ Tests cover:
 - Security issues (path traversal)
 - Error handling
 """
+
 import io
 import os
 import pytest
@@ -21,8 +22,10 @@ from fastapi.testclient import TestClient
 def mock_celery_tasks():
     """Mock all Celery tasks to prevent execution."""
     # Patch the entire task object where it's used (in app.api.files)
-    with patch("app.api.files.process_document") as mock_process_task, \
+    with (
-         patch("app.api.files.convert_to_pdf") as mock_convert_task:
+        patch("app.api.files.process_document") as mock_process_task,
+        patch("app.api.files.convert_to_pdf") as mock_convert_task,
+    ):
 
         # Setup default return values for .delay()
         mock_task = MagicMock()
@@ -30,10 +33,7 @@ def mock_celery_tasks():
         mock_process_task.delay.return_value = mock_task
         mock_convert_task.delay.return_value = mock_task
 
-        yield {
+        yield {"process_document": mock_process_task.delay, "convert_to_pdf": mock_convert_task.delay}
-            "process_document": mock_process_task.delay,
-            "convert_to_pdf": mock_convert_task.delay
-        }
 
 
 @pytest.mark.integration
@@ -43,10 +43,7 @@ class TestValidFileUploads:
     def test_upload_valid_pdf(self, client: TestClient, sample_pdf_path: str, mock_celery_tasks):
         """Test uploading a valid PDF file."""
         with open(sample_pdf_path, "rb") as f:
-            response = client.post(
+            response = client.post("/api/ui-upload", files={"file": ("document.pdf", f, "application/pdf")})
-                "/api/ui-upload",
-                files={"file": ("document.pdf", f, "application/pdf")}
-            )
 
         assert response.status_code == 200
         data = response.json()
@@ -71,8 +68,7 @@ class TestValidFileUploads:
         """Test uploading a valid text file."""
         text_content = b"This is a test text file.\nWith multiple lines."
         response = client.post(
-            "/api/ui-upload",
+            "/api/ui-upload", files={"file": ("document.txt", io.BytesIO(text_content), "text/plain")}
-            files={"file": ("document.txt", io.BytesIO(text_content), "text/plain")}
         )
 
         assert response.status_code == 200
@@ -92,10 +88,7 @@ class TestValidFileUploads:
             b"\xff\xd9"
         )
 
-        response = client.post(
+        response = client.post("/api/ui-upload", files={"file": ("image.jpg", io.BytesIO(jpeg_content), "image/jpeg")})
-            "/api/ui-upload",
-            files={"file": ("image.jpg", io.BytesIO(jpeg_content), "image/jpeg")}
-        )
 
         assert response.status_code == 200
         data = response.json()
@@ -114,8 +107,7 @@ class TestValidFileUploads:
         )
 
         response = client.post(
-            "/api/ui-upload",
+            "/api/ui-upload", files={"file": ("screenshot.png", io.BytesIO(png_content), "image/png")}
-            files={"file": ("screenshot.png", io.BytesIO(png_content), "image/png")}
         )
 
         assert response.status_code == 200
@@ -127,17 +119,17 @@ class TestValidFileUploads:
     def test_upload_office_document_docx(self, client: TestClient, mock_celery_tasks):
         """Test uploading a Word document."""
         # Create minimal DOCX content (ZIP file with proper structure)
-        docx_content = (
+        docx_content = b"PK\x03\x04\x14\x00\x00\x00\x08\x00" + b"\x00" * 50
-            b"PK\x03\x04\x14\x00\x00\x00\x08\x00" + b"\x00" * 50
-        )
 
         response = client.post(
             "/api/ui-upload",
-            files={"file": (
+            files={
+                "file": (
                     "report.docx",
                     io.BytesIO(docx_content),
-                "application/vnd.openxmlformats-officedocument.wordprocessingml.document"
+                    "application/vnd.openxmlformats-officedocument.wordprocessingml.document",
-            )}
+                )
+            },
         )
 
         assert response.status_code == 200
@@ -152,10 +144,7 @@ class TestValidFileUploads:
         """Test uploading a CSV file."""
         csv_content = b"name,age,city\nJohn,30,NYC\nJane,25,LA\n"
 
-        response = client.post(
+        response = client.post("/api/ui-upload", files={"file": ("data.csv", io.BytesIO(csv_content), "text/csv")})
-            "/api/ui-upload",
-            files={"file": ("data.csv", io.BytesIO(csv_content), "text/csv")}
-        )
 
         assert response.status_code == 200
         data = response.json()
@@ -177,8 +166,7 @@ class TestInvalidFileUploads:
             mock_getsize.return_value = 501 * 1024 * 1024  # 501MB
 
             response = client.post(
-                "/api/ui-upload",
+                "/api/ui-upload", files={"file": ("huge.pdf", io.BytesIO(large_content), "application/pdf")}
-                files={"file": ("huge.pdf", io.BytesIO(large_content), "application/pdf")}
             )
 
             assert response.status_code == 413  # Request Entity Too Large
@@ -189,8 +177,7 @@ class TestInvalidFileUploads:
         exe_content = b"MZ\x90\x00"  # PE header
 
         response = client.post(
-            "/api/ui-upload",
+            "/api/ui-upload", files={"file": ("program.exe", io.BytesIO(exe_content), "application/x-msdownload")}
-            files={"file": ("program.exe", io.BytesIO(exe_content), "application/x-msdownload")}
         )
 
         # Per the code, unsupported types get a warning but are still processed
@@ -200,10 +187,7 @@ class TestInvalidFileUploads:
 
     def test_upload_empty_file(self, client: TestClient, mock_celery_tasks):
         """Test uploading an empty file."""
-        response = client.post(
+        response = client.post("/api/ui-upload", files={"file": ("empty.txt", io.BytesIO(b""), "text/plain")})
-            "/api/ui-upload",
-            files={"file": ("empty.txt", io.BytesIO(b""), "text/plain")}
-        )
 
         # Empty files are accepted and queued for processing
         assert response.status_code == 200
@@ -230,8 +214,7 @@ class TestUploadSecurity:
         pdf_content = b"%PDF-1.4\n%EOF"
 
         response = client.post(
-            "/api/ui-upload",
+            "/api/ui-upload", files={"file": (malicious_filename, io.BytesIO(pdf_content), "application/pdf")}
-            files={"file": (malicious_filename, io.BytesIO(pdf_content), "application/pdf")}
         )
 
         assert response.status_code == 200
@@ -248,8 +231,7 @@ class TestUploadSecurity:
         pdf_content = b"%PDF-1.4\n%EOF"
 
         response = client.post(
-            "/api/ui-upload",
+            "/api/ui-upload", files={"file": (malicious_filename, io.BytesIO(pdf_content), "application/pdf")}
-            files={"file": (malicious_filename, io.BytesIO(pdf_content), "application/pdf")}
         )
 
         assert response.status_code == 200
@@ -265,18 +247,69 @@ class TestUploadSecurity:
         pdf_content = b"%PDF-1.4\n%EOF"
 
         response = client.post(
-            "/api/ui-upload",
+            "/api/ui-upload", files={"file": (special_filename, io.BytesIO(pdf_content), "application/pdf")}
-            files={"file": (special_filename, io.BytesIO(pdf_content), "application/pdf")}
         )
 
         assert response.status_code == 200
         data = response.json()
 
-        # Original filename should be preserved (sanitized by basename)
+        # Original filename should be sanitized (special chars replaced with underscores)
-        assert data["original_filename"] == special_filename
+        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()
+
+        # On Unix systems, os.path.basename doesn't recognize Windows backslashes,
+        # so the full string goes to sanitize_filename which then:
+        # 1. Replaces backslashes with underscores
+        # 2. Replaces .. with underscores
+        # 3. Strips leading/trailing underscores
+        # Result: "windows_system32_config.pdf"
+        expected_filename = "windows_system32_config.pdf"
+        assert data["original_filename"] == expected_filename
+        # Verify no dangerous characters remain
+        assert "\\" not in data["original_filename"]
+        assert ".." not in data["original_filename"]
+        assert "/" not 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()
+
+        # os.path.basename recognizes Unix / separators and extracts "file.pdf"
+        # sanitize_filename then ensures it's safe (already is in this case)
+        expected_filename = "file.pdf"
+        assert data["original_filename"] == expected_filename
+        # Verify no dangerous characters remain
+        assert "/" not in data["original_filename"]
+        assert "\\" not in data["original_filename"]
+        assert ".." not in data["original_filename"]
+
 
 @pytest.mark.integration
 class TestUploadErrorHandling:
@@ -288,8 +321,7 @@ class TestUploadErrorHandling:
             pdf_content = b"%PDF-1.4\n%EOF"
 
             response = client.post(
-                "/api/ui-upload",
+                "/api/ui-upload", files={"file": ("test.pdf", io.BytesIO(pdf_content), "application/pdf")}
-                files={"file": ("test.pdf", io.BytesIO(pdf_content), "application/pdf")}
             )
 
             assert response.status_code == 500
@@ -304,10 +336,7 @@ class TestUploadErrorHandling:
         # The endpoint should still handle the error gracefully
         # In this case, the exception will propagate
         with pytest.raises(Exception):
-            client.post(
+            client.post("/api/ui-upload", files={"file": ("test.pdf", io.BytesIO(pdf_content), "application/pdf")})
-                "/api/ui-upload",
-                files={"file": ("test.pdf", io.BytesIO(pdf_content), "application/pdf")}
-            )
 
 
 @pytest.mark.integration
@@ -320,13 +349,11 @@ class TestUploadFilenameHandling:
 
         # Upload same file twice
         response1 = client.post(
-            "/api/ui-upload",
+            "/api/ui-upload", files={"file": ("same.pdf", io.BytesIO(pdf_content), "application/pdf")}
-            files={"file": ("same.pdf", io.BytesIO(pdf_content), "application/pdf")}
         )
 
         response2 = client.post(
-            "/api/ui-upload",
+            "/api/ui-upload", files={"file": ("same.pdf", io.BytesIO(pdf_content), "application/pdf")}
-            files={"file": ("same.pdf", io.BytesIO(pdf_content), "application/pdf")}
         )
 
         assert response1.status_code == 200
@@ -346,8 +373,7 @@ class TestUploadFilenameHandling:
         content = b"Some content"
 
         response = client.post(
-            "/api/ui-upload",
+            "/api/ui-upload", files={"file": ("NOEXTENSION", io.BytesIO(content), "application/octet-stream")}
-            files={"file": ("NOEXTENSION", io.BytesIO(content), "application/octet-stream")}
         )
 
         assert response.status_code == 200
@@ -366,8 +392,7 @@ class TestUploadMimeTypeDetection:
         pdf_content = b"%PDF-1.4\n%EOF"
 
         response = client.post(
-            "/api/ui-upload",
+            "/api/ui-upload", files={"file": ("doc.pdf", io.BytesIO(pdf_content), "application/octet-stream")}
-            files={"file": ("doc.pdf", io.BytesIO(pdf_content), "application/octet-stream")}
         )
 
         assert response.status_code == 200
@@ -380,8 +405,7 @@ class TestUploadMimeTypeDetection:
         image_content = b"\x00\x01\x02\x03"
 
         response = client.post(
-            "/api/ui-upload",
+            "/api/ui-upload", files={"file": ("photo.jpg", io.BytesIO(image_content), "application/octet-stream")}
-            files={"file": ("photo.jpg", io.BytesIO(image_content), "application/octet-stream")}
         )
 
         assert response.status_code == 200