fix(tasks): address code review - SMTP TLS cert validation, rclone input validation, consolidated test fixture

Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com>

app/tasks/upload_to_user_integration.py

@@ -501,12 +501,17 @@ def _upload_email(file_path: str, cfg: dict[str, Any], creds: dict[str, Any], ta
     msg.attach(part)
 
     if use_tls:
+        import ssl
+
+        tls_context = ssl.create_default_context()
         with smtplib.SMTP(host, port, timeout=30) as smtp:
-            smtp.starttls()  # nosec B608
+            smtp.starttls(context=tls_context)
             if username and password:
                 smtp.login(username, password)
             smtp.sendmail(msg["From"], [recipient], msg.as_string())
     else:
+        # Plaintext SMTP — only use when explicitly configured and TLS is unavailable.
+        # Credentials and content will be transmitted without encryption.
         with smtplib.SMTP(host, port, timeout=30) as smtp:  # nosec B608
             if username and password:
                 smtp.login(username, password)
@@ -518,6 +523,9 @@ def _upload_email(file_path: str, cfg: dict[str, Any], creds: dict[str, Any], ta
 
 def _upload_rclone(file_path: str, cfg: dict[str, Any], creds: dict[str, Any], task_id: str) -> dict[str, Any]:
     """Copy *file_path* to an rclone remote using per-user rclone config."""
+    import re
+    import tempfile
+
     remote = cfg.get("remote") or ""
     folder = cfg.get("folder") or ""
     rclone_conf_text = creds.get("rclone_conf") or ""
@@ -528,9 +536,17 @@ def _upload_rclone(file_path: str, cfg: dict[str, Any], creds: dict[str, Any], t
     if not rclone_conf_text:
         raise ValueError("Rclone integration is missing rclone_conf in credentials")
 
-    # Write the user's rclone config to a temp file so we don't touch the system config
+    # Validate remote and folder to prevent shell metacharacter injection.
-    import tempfile
+    # rclone remote names are alphanumeric + hyphens/underscores followed by ':'.
+    # folder paths must not contain shell-dangerous characters.
+    _SAFE_REMOTE_RE = re.compile(r"^[A-Za-z0-9_\-]+:(/[A-Za-z0-9_.@\-/ ]*)?$")
+    _SAFE_FOLDER_RE = re.compile(r"^[A-Za-z0-9_.@\-/ ]*$")
+    if not _SAFE_REMOTE_RE.match(remote):
+        raise ValueError(f"Rclone remote contains unsafe characters: {remote!r}")
+    if folder and not _SAFE_FOLDER_RE.match(folder):
+        raise ValueError(f"Rclone folder contains unsafe characters: {folder!r}")
 
+    # Write the user's rclone config to a temp file so we don't touch the system config
     with tempfile.NamedTemporaryFile(mode="w", suffix=".conf", delete=False) as tmp_conf:
         tmp_conf.write(rclone_conf_text)
         conf_path = tmp_conf.name

tests/test_finalize_storage.py

@@ -19,16 +19,17 @@ def _make_file_record(file_id: int = 123, owner_id=None):
     return rec
 
 
+@pytest.fixture(autouse=True)
+def _patch_celery_background_tasks(mocker):
+    """Module-level autouse fixture: prevent lazy-imported Celery tasks from connecting to Redis."""
+    mocker.patch("app.tasks.compute_embedding.compute_document_embedding")
+    mocker.patch("app.tasks.convert_to_pdfa.convert_to_pdfa", create=True)
+
+
 @pytest.mark.unit
 class TestFinalizeDocumentStorage:
     """Tests for finalize_document_storage Celery task."""
 
-    @pytest.fixture(autouse=True)
-    def _patch_celery_tasks(self, mocker):
-        """Prevent all lazy-imported Celery tasks from actually connecting to Redis."""
-        mocker.patch("app.tasks.compute_embedding.compute_document_embedding")
-        mocker.patch("app.tasks.convert_to_pdfa.convert_to_pdfa", create=True)
-
     @patch("app.tasks.finalize_document_storage.notify_file_processed")
     @patch("app.tasks.finalize_document_storage.send_to_user_destinations")
     @patch("app.tasks.finalize_document_storage.send_to_all_destinations")
@@ -466,12 +467,6 @@ class TestFinalizeDocumentStorage:
 class TestFinalizeDocumentStorageUserRouting:
     """Tests for user-specific destination routing in finalize_document_storage."""
 
-    @pytest.fixture(autouse=True)
-    def _patch_celery_tasks(self, mocker):
-        """Prevent all lazy-imported Celery tasks from actually connecting to Redis."""
-        mocker.patch("app.tasks.compute_embedding.compute_document_embedding")
-        mocker.patch("app.tasks.convert_to_pdfa.convert_to_pdfa", create=True)
-
     @patch("app.tasks.finalize_document_storage.notify_file_processed")
     @patch("app.tasks.finalize_document_storage.send_to_user_destinations")
     @patch("app.tasks.finalize_document_storage.send_to_all_destinations")