Merge pull request #534 from MerginMaps/refuse_trailing_whitespace_in_path

Refuse trailing whitespace in path

Author: MarcelGeo

server/mergin/sync/files.py

@@ -13,10 +13,10 @@
 
 from .utils import (
     is_file_name_blacklisted,
-    is_qgis,
     is_supported_extension,
     is_valid_path,
     is_versioned_file,
+    has_trailing_space,
 )
 from ..app import DateTimeWithZ, ma
 
@@ -212,14 +212,21 @@ def validate(self, data, **kwargs):
 
             if not is_valid_path(file_path):
                 raise ValidationError(
-                    f"Unsupported file name detected: {file_path}. Please remove the invalid characters."
+                    f"Unsupported file name detected: '{file_path}'. Please remove the invalid characters."
                 )
 
             if not is_supported_extension(file_path):
                 raise ValidationError(
-                    f"Unsupported file type detected: {file_path}. "
+                    f"Unsupported file type detected: '{file_path}'. "
                     f"Please remove the file or try compressing it into a ZIP file before uploading.",
                 )
+        # new checks must restrict only new files not to block existing projects
+        for file in data["added"]:
+            file_path = file["path"]
+            if has_trailing_space(file_path):
+                raise ValidationError(
+                    f"Folder name contains a trailing space. Please remove the space in: '{file_path}'."
+                )
 
 
 class ProjectFileSchema(FileSchema):

server/mergin/sync/public_api_controller.py

@@ -55,9 +55,7 @@
 from .files import (
     ProjectFileChange,
     ChangesSchema,
-    UploadFileSchema,
     ProjectFileSchema,
-    FileSchema,
     files_changes_from_upload,
     mergin_secure_filename,
 )
@@ -83,17 +81,11 @@
     generate_checksum,
     Toucher,
     get_x_accel_uri,
-    is_file_name_blacklisted,
     get_ip,
     get_user_agent,
     generate_location,
     is_valid_uuid,
-    is_versioned_file,
-    get_project_path,
     get_device_id,
-    is_valid_path,
-    is_supported_type,
-    is_supported_extension,
     get_mimetype,
     wkb2wkt,
 )
@@ -980,7 +972,7 @@ def push_finish(transaction_id):
         if len(unsupported_files):
             abort(
                 400,
-                f"Unsupported file type detected: {unsupported_files[0]}. "
+                f"Unsupported file type detected: '{unsupported_files[0]}'. "
                 f"Please remove the file or try compressing it into a ZIP file before uploading.",
             )
 

server/mergin/sync/utils.py

@@ -25,6 +25,7 @@
 )
 import magic
 from flask import current_app
+from pathlib import Path
 
 
 def generate_checksum(file, chunk_size=4096):
@@ -100,11 +101,6 @@ def is_qgis(path: str) -> bool:
     return ext.lower() in [".qgs", ".qgz"]
 
 
-def int_version(version):
-    """Convert v<n> format of version to integer representation."""
-    return int(version.lstrip("v")) if re.match(r"v\d", version) else None
-
-
 def is_versioned_file(file):
     """Check if file is compatible with geodiff lib and hence suitable for versioning."""
     diff_extensions = [".gpkg", ".sqlite"]
@@ -338,14 +334,19 @@ def files_size():
 def is_valid_path(filepath: str) -> bool:
     """Check filepath and filename for invalid characters, absolute path or path traversal"""
     return (
-        not len(re.split(r"\.[/\\]", filepath)) > 1  # ./ or .\
+        not re.search(r"\.[/\\]", filepath)  # ./ or .\
         and is_valid_filepath(filepath)  # invalid characters in filepath, absolute path
         and is_valid_filename(
             os.path.basename(filepath)
         )  # invalid characters in filename, reserved filenames
     )
 
 
+def has_trailing_space(filepath: str) -> bool:
+    """Check filepath for trailing spaces that makes the project impossible to download on Windows"""
+    return any(part != part.rstrip() for part in Path(filepath).parts)
+
+
 def is_supported_extension(filepath) -> bool:
     """Check whether file's extension is supported."""
     ext = os.path.splitext(filepath)[1].lower()

server/mergin/tests/test_project_controller.py

@@ -2495,7 +2495,7 @@ def test_filepath_manipulation(client):
     assert resp.status_code == 400
     assert (
         resp.json["detail"]
-        == f"Unsupported file name detected: {manipulated_path}. Please remove the invalid characters."
+        == f"Unsupported file name detected: '{manipulated_path}'. Please remove the invalid characters."
     )
 
 
@@ -2528,7 +2528,7 @@ def test_supported_file_upload(client):
     assert resp.status_code == 400
     assert (
         resp.json["detail"]
-        == f"Unsupported file type detected: {script_filename}. Please remove the file or try compressing it into a ZIP file before uploading."
+        == f"Unsupported file type detected: '{script_filename}'. Please remove the file or try compressing it into a ZIP file before uploading."
     )
     # Extension spoofing to trick the validator
     spoof_name = "script.gpkg"
@@ -2567,7 +2567,7 @@ def test_supported_file_upload(client):
     assert resp.status_code == 400
     assert (
         resp.json["detail"]
-        == f"Unsupported file type detected: {spoof_name}. Please remove the file or try compressing it into a ZIP file before uploading."
+        == f"Unsupported file type detected: '{spoof_name}'. Please remove the file or try compressing it into a ZIP file before uploading."
     )
 
 

server/mergin/tests/test_utils.py

@@ -5,14 +5,14 @@
 import base64
 from datetime import datetime
 import json
-import os
 import pytest
 from flask import url_for, current_app
 from sqlalchemy import desc
 import os
 from unittest.mock import patch
 from pathvalidate import sanitize_filename
 from pygeodiff import GeoDiff
+from pathlib import PureWindowsPath
 
 from ..utils import save_diagnostic_log_file
 
@@ -24,6 +24,7 @@
     is_valid_path,
     get_x_accel_uri,
     wkb2wkt,
+    has_trailing_space,
 )
 from ..auth.models import LoginHistory, User
 from . import json_headers
@@ -228,6 +229,24 @@ def test_is_valid_path(client, filepath, allow):
     assert is_valid_path(filepath) == allow
 
 
+trailing_spaces_paths = [
+    ("photos /lutraHQ.jpg", "posix", True),
+    ("photo s/ lutraHQ.jpg", "posix", False),
+    ("assets\photos \lutraHQ.jpg", "windows", True),
+    ("assets\  photos\lutraHQ.jpg", "windows", False),
+]
+
+
+@pytest.mark.parametrize("path,path_platform,result", trailing_spaces_paths)
+def test_has_trailing_space(path, path_platform, result):
+    if path_platform == "windows":
+        # we must mock Path to instantiate as Windows path
+        with patch("mergin.sync.utils.Path", PureWindowsPath):
+            assert has_trailing_space(path) is result
+    else:
+        assert has_trailing_space(path) is result
+
+
 def test_get_x_accell_uri(client):
     """Test get_x_accell_uri"""
     client.application.config["LOCAL_PROJECTS"] = "/data/"