From 0684cbc8cd5379d6204612d81fa883de7caa3e7c Mon Sep 17 00:00:00 2001
From: "marcel.kocisek" <marcel.kocisek@lutraconsulting.co.uk>
Date: Tue, 28 Jan 2025 10:15:37 +0100
Subject: [PATCH 1/3] Added: util methods to validate names - use pathvalidate
 lib for filenmaes like names (project, workspace) - new error messages for
 validation Removed: one regexp for validation

---
 server/mergin/auth/forms.py                   | 25 +++++++++---
 server/mergin/sync/forms.py                   | 24 ++++++++++++
 server/mergin/sync/public_api_controller.py   | 23 ++++++-----
 .../mergin/sync/public_api_v2_controller.py   | 14 ++++---
 server/mergin/sync/utils.py                   | 38 ++++++++++++-------
 server/mergin/tests/test_utils.py             | 36 ++++++++++++++----
 6 files changed, 116 insertions(+), 44 deletions(-)

diff --git a/server/mergin/auth/forms.py b/server/mergin/auth/forms.py
index 30a615fa..bd5e960a 100644
--- a/server/mergin/auth/forms.py
+++ b/server/mergin/auth/forms.py
@@ -20,13 +20,26 @@
 
 
 def username_validation(form, field):
-    from ..sync.utils import is_name_allowed
+    from ..sync.utils import (
+        is_valid_character,
+        is_valid_first_character,
+        is_valid_filename,
+        is_reserved_word,
+    )
 
-    if field.data and (not is_name_allowed(field.data) or "@" in field.data):
-        raise ValidationError(
-            f"Please don't start username with . and "
-            f"use only alphanumeric or these -._! characters in {field.name}."
-        )
+    errors = [
+        "Please use only alphanumeric or these -_. characters.",
+        "User name cannot start with space or dot.",
+        "User name contains not allowed word.",
+    ]
+    validations = [
+        is_valid_character(field.data),
+        is_valid_first_character(field.data),
+        not is_reserved_word(field.data) and is_valid_filename(field.data),
+    ]
+    for index, error in enumerate(errors):
+        if not validations[index]:
+            raise ValidationError(error)
 
 
 class PasswordValidator:
diff --git a/server/mergin/sync/forms.py b/server/mergin/sync/forms.py
index 275380b1..1e9bfaa5 100644
--- a/server/mergin/sync/forms.py
+++ b/server/mergin/sync/forms.py
@@ -6,6 +6,30 @@
 from wtforms.validators import DataRequired
 from flask_wtf import FlaskForm
 
+from mergin.sync.utils import (
+    is_valid_character,
+    is_valid_filename,
+    is_valid_first_character,
+)
+
+
+def project_name_validation(name: str) -> str | None:
+    """Check whether project name is valid"""
+    errors = [
+        "Please use only alphanumeric or these -_. characters.",
+        "Project name cannot start with space or dot.",
+        "Project name contains not allowed word.",
+    ]
+    validations = [
+        is_valid_character(name),
+        is_valid_first_character(name),
+        is_valid_filename(name),
+    ]
+    for index, error in enumerate(errors):
+        if not validations[index]:
+            return error
+    return
+
 
 class IntegerListField(Field):
     def _value(self):
diff --git a/server/mergin/sync/public_api_controller.py b/server/mergin/sync/public_api_controller.py
index 6a39e441..488d0ad9 100644
--- a/server/mergin/sync/public_api_controller.py
+++ b/server/mergin/sync/public_api_controller.py
@@ -34,6 +34,8 @@
 from sqlalchemy.orm import load_only
 from werkzeug.exceptions import HTTPException
 
+from mergin.sync.forms import project_name_validation
+
 from .interfaces import WorkspaceRole
 from ..app import db
 from ..auth import auth_required
@@ -83,7 +85,6 @@
     is_valid_uuid,
     gpkg_wkb_to_wkt,
     is_versioned_file,
-    is_name_allowed,
     get_project_path,
     get_device_id,
 )
@@ -177,12 +178,15 @@ def add_project(namespace):  # noqa: E501
 
     :rtype: None
     """
-    request.json["name"] = request.json["name"].strip()
+    name = request.json["name"].strip()
 
-    if not is_name_allowed(request.json["name"]):
+    if not name:
+        abort(400, "Project name cannot be empty")
+    validation = project_name_validation(name)
+    if validation:
         abort(
             400,
-            "Please don't start project name with . and use only alphanumeric or these -._! characters in project name.",
+            validation,
         )
 
     if request.is_json:
@@ -1194,6 +1198,8 @@ def clone_project(namespace, project_name):  # noqa: E501
     dest_ns = request.json.get("namespace", cp_workspace_name).strip()
     dest_project = request.json.get("project", cloned_project.name).strip()
     ws = current_app.ws_handler.get_by_name(dest_ns)
+    if not dest_project:
+        abort(400, "Project name cannot be empty")
     if not ws:
         if dest_ns == current_user.username:
             abort(
@@ -1204,12 +1210,9 @@ def clone_project(namespace, project_name):  # noqa: E501
             abort(404, "Workspace does not exist")
     if not ws.user_has_permissions(current_user, "admin"):
         abort(403, "You do not have permissions for this workspace")
-
-    if not is_name_allowed(dest_project):
-        abort(
-            400,
-            "Please don't start project name with . and use only alphanumeric or these -._! characters in project name.",
-        )
+    validation = project_name_validation(dest_project)
+    if validation:
+        abort(400, validation)
 
     _project = Project.query.filter_by(name=dest_project, workspace_id=ws.id).first()
     if _project:
diff --git a/server/mergin/sync/public_api_v2_controller.py b/server/mergin/sync/public_api_v2_controller.py
index d5d36b11..82dfc4a5 100644
--- a/server/mergin/sync/public_api_v2_controller.py
+++ b/server/mergin/sync/public_api_v2_controller.py
@@ -7,6 +7,8 @@
 from flask import abort, jsonify
 from flask_login import current_user
 
+from mergin.sync.forms import project_name_validation
+
 from .schemas import ProjectMemberSchema
 from .workspace import WorkspaceRole
 from ..app import db
@@ -15,7 +17,6 @@
 from .models import Project, ProjectRole, ProjectMember
 from .permissions import ProjectPermissions, require_project_by_uuid
 from .private_api_controller import project_access_granted
-from .utils import is_name_allowed
 
 
 @auth_required
@@ -45,13 +46,14 @@ def delete_project_now(id):
 def update_project(id):
     """Rename project"""
     project = require_project_by_uuid(id, ProjectPermissions.Update)
-    new_name = request.json["name"]
+    new_name = request.json["name"].strip()
 
-    if not is_name_allowed(new_name):
+    if not new_name:
+        abort(400, "Project name cannot be empty")
+    validation = project_name_validation(new_name)
+    if validation:
         return (
-            jsonify(
-                code="InvalidProjectName", detail="Entered project name is invalid"
-            ),
+            jsonify(code="InvalidProjectName", detail=validation),
             400,
         )
     new_name_exists = Project.query.filter_by(
diff --git a/server/mergin/sync/utils.py b/server/mergin/sync/utils.py
index b4120acc..bf26689e 100644
--- a/server/mergin/sync/utils.py
+++ b/server/mergin/sync/utils.py
@@ -15,6 +15,7 @@
 from flask import Request
 from typing import Optional
 from sqlalchemy import text
+from pathvalidate import validate_filename, ValidationError
 
 
 def generate_checksum(file, chunk_size=4096):
@@ -261,22 +262,31 @@ def convert_byte(size_bytes, unit):
     return size_bytes
 
 
-def is_name_allowed(string):
-    """Check if string is just has whitelisted character
+def is_reserved_word(name: str) -> bool:
+    """Check if name is reserved in system"""
+    reserved = r"^support$|^helpdesk$|^merginmaps$|^lutraconsulting$|^mergin$|^lutra$|^input$|^admin$|^sales$"
+    return re.match(reserved, name) is not None
 
-    :param string: string to be checked.
-    :type string: str
 
-    :return: boolean of has just whitelisted character
-    :rtype: bool
-    """
-    return (
-        re.match(
-            r".*[\@\#\$\%\^\&\*\(\)\{\}\[\]\?\'\"`,;\:\+\=\~\\\/\|\<\>].*|^[\s^\.].*$|^CON$|^PRN$|^AUX$|^NUL$|^COM\d$|^LPT\d|^support$|^helpdesk$|^merginmaps$|^lutraconsulting$|^mergin$|^lutra$|^input$|^admin$|^sales$|^$",
-            string,
-        )
-        is None
-    )
+def is_valid_character(name: str) -> bool:
+    """Check if name contains only valid characters"""
+    return re.match(r"^[\w\s\-\.]+$", name) is not None
+
+
+def is_valid_first_character(name: str) -> bool:
+    """Check if name contains only valid characters in first position"""
+    return re.match(r"^[\s^\.].*$", name) is None
+
+
+def is_valid_filename(name: str) -> bool:
+    """Check if name contains only valid characters for filename"""
+    is_valid = True
+    try:
+        validate_filename(name)
+    except ValidationError:
+        is_valid = False
+
+    return is_valid
 
 
 def workspace_names(workspaces):
diff --git a/server/mergin/tests/test_utils.py b/server/mergin/tests/test_utils.py
index 6141e573..8d6f2f81 100644
--- a/server/mergin/tests/test_utils.py
+++ b/server/mergin/tests/test_utils.py
@@ -11,7 +11,14 @@
 from unittest.mock import MagicMock
 
 from ..app import db
-from ..sync.utils import parse_gpkgb_header_size, gpkg_wkb_to_wkt, is_name_allowed
+from ..sync.utils import (
+    parse_gpkgb_header_size,
+    gpkg_wkb_to_wkt,
+    is_reserved_word,
+    is_valid_character,
+    is_valid_first_character,
+    is_valid_filename,
+)
 from ..auth.models import LoginHistory, User
 from . import json_headers
 from .utils import login
@@ -137,13 +144,15 @@ def test_is_name_allowed():
         ("Pro123ject", True),
         ("123PROJECT", True),
         ("PROJECT", True),
-        ("project ", True),
+        # Not valid filename
+        ("project ", False),
         ("pro ject", True),
         ("proj-ect", True),
         ("-project", True),
         ("proj_ect", True),
         ("proj.ect", True),
-        ("proj!ect", True),
+        # We are repmoving ! from valids
+        ("proj!ect", False),
         (" project", False),
         (".project", False),
         ("proj~ect", False),
@@ -182,14 +191,15 @@ def test_is_name_allowed():
         ("NUL", False),
         ("NULL", True),
         ("PRN", False),
-        ("LPT0", False),
+        # is not reserved word
+        ("LPT0", True),
         ("lpt0", True),
         ("LPT1", False),
-        ("lpt1", True),
+        ("lpt1", False),
         ("COM1", False),
-        ("com1", True),
+        ("com1", False),
         ("AUX", False),
-        ("AuX", True),
+        ("AuX", False),
         ("projAUXect", True),
         ("CONproject", True),
         ("projectCON", True),
@@ -204,7 +214,17 @@ def test_is_name_allowed():
         ("sales", False),
         ("", False),
         ("    ", False),
+        ("😄guy", False),
+        ("会社", True),
     ]
 
     for t in test_cases:
-        assert is_name_allowed(t[0]) == t[1]
+        name = t[0]
+        expected = t[1]
+        print(is_reserved_word(name))
+        assert (
+            is_valid_character(name)
+            and is_valid_first_character(name)
+            and is_valid_filename(name)
+            and not is_reserved_word(name)
+        ) == expected

From 94e9bc72494d47e0196b3dc272d350c35a41c491 Mon Sep 17 00:00:00 2001
From: "marcel.kocisek" <marcel.kocisek@lutraconsulting.co.uk>
Date: Tue, 28 Jan 2025 12:18:20 +0100
Subject: [PATCH 2/3] Address comments @varmar05

---
 server/mergin/auth/forms.py                   | 20 ++++++--------
 server/mergin/sync/forms.py                   | 21 +++++++--------
 server/mergin/sync/public_api_controller.py   | 10 +++----
 server/mergin/sync/public_api_v2.yaml         | 10 +++----
 .../mergin/sync/public_api_v2_controller.py   |  9 +++----
 server/mergin/sync/utils.py                   | 27 +++++++++++--------
 .../mergin/tests/test_project_controller.py   |  2 ++
 server/mergin/tests/test_public_api_v2.py     |  2 +-
 server/mergin/tests/test_utils.py             | 17 +++++++-----
 9 files changed, 58 insertions(+), 60 deletions(-)

diff --git a/server/mergin/auth/forms.py b/server/mergin/auth/forms.py
index bd5e960a..f4baa69e 100644
--- a/server/mergin/auth/forms.py
+++ b/server/mergin/auth/forms.py
@@ -21,24 +21,20 @@
 
 def username_validation(form, field):
     from ..sync.utils import (
-        is_valid_character,
-        is_valid_first_character,
+        has_valid_characters,
+        has_valid_first_character,
         is_valid_filename,
         is_reserved_word,
     )
 
     errors = [
-        "Please use only alphanumeric or these -_. characters.",
-        "User name cannot start with space or dot.",
-        "User name contains not allowed word.",
+        has_valid_characters(field.data),
+        has_valid_first_character(field.data),
+        is_reserved_word(field.data),
+        is_valid_filename(field.data),
     ]
-    validations = [
-        is_valid_character(field.data),
-        is_valid_first_character(field.data),
-        not is_reserved_word(field.data) and is_valid_filename(field.data),
-    ]
-    for index, error in enumerate(errors):
-        if not validations[index]:
+    for error in errors:
+        if error:
             raise ValidationError(error)
 
 
diff --git a/server/mergin/sync/forms.py b/server/mergin/sync/forms.py
index 1e9bfaa5..f5aa484e 100644
--- a/server/mergin/sync/forms.py
+++ b/server/mergin/sync/forms.py
@@ -7,26 +7,25 @@
 from flask_wtf import FlaskForm
 
 from mergin.sync.utils import (
-    is_valid_character,
+    is_reserved_word,
+    has_valid_characters,
     is_valid_filename,
-    is_valid_first_character,
+    has_valid_first_character,
 )
 
 
 def project_name_validation(name: str) -> str | None:
     """Check whether project name is valid"""
+    if not name.strip():
+        return "Project name cannot be empty"
     errors = [
-        "Please use only alphanumeric or these -_. characters.",
-        "Project name cannot start with space or dot.",
-        "Project name contains not allowed word.",
-    ]
-    validations = [
-        is_valid_character(name),
-        is_valid_first_character(name),
+        has_valid_characters(name),
+        has_valid_first_character(name),
+        is_reserved_word(name),
         is_valid_filename(name),
     ]
-    for index, error in enumerate(errors):
-        if not validations[index]:
+    for error in errors:
+        if error:
             return error
     return
 
diff --git a/server/mergin/sync/public_api_controller.py b/server/mergin/sync/public_api_controller.py
index 488d0ad9..0dd3d3a9 100644
--- a/server/mergin/sync/public_api_controller.py
+++ b/server/mergin/sync/public_api_controller.py
@@ -178,15 +178,13 @@ def add_project(namespace):  # noqa: E501
 
     :rtype: None
     """
-    name = request.json["name"].strip()
+    request.json["name"] = request.json["name"].strip()
 
-    if not name:
-        abort(400, "Project name cannot be empty")
-    validation = project_name_validation(name)
-    if validation:
+    validation_error = project_name_validation(request.json["name"])
+    if validation_error:
         abort(
             400,
-            validation,
+            validation_error,
         )
 
     if request.is_json:
diff --git a/server/mergin/sync/public_api_v2.yaml b/server/mergin/sync/public_api_v2.yaml
index 3d47ca4e..04dbce61 100644
--- a/server/mergin/sync/public_api_v2.yaml
+++ b/server/mergin/sync/public_api_v2.yaml
@@ -66,8 +66,6 @@ paths:
                     example: InvalidProjectName
                   detail:
                     type: string
-                    enum:
-                      - "Entered project name is invalid"
                     example: "Entered project name is invalid"
         "401":
           $ref: "#/components/responses/Unauthorized"
@@ -267,7 +265,7 @@ components:
       example: writer
     Role:
       allOf:
-        - $ref: '#/components/schemas/ProjectRole'
+        - $ref: "#/components/schemas/ProjectRole"
           nullable: false
           description: combination of workspace role and project role
     ProjectMember:
@@ -284,8 +282,8 @@ components:
           format: email
           example: john.doe@example.com
         workspace_role:
-          $ref: '#/components/schemas/WorkspaceRole'
+          $ref: "#/components/schemas/WorkspaceRole"
         project_role:
-          $ref: '#/components/schemas/ProjectRole'
+          $ref: "#/components/schemas/ProjectRole"
         role:
-          $ref: '#/components/schemas/Role'
+          $ref: "#/components/schemas/Role"
diff --git a/server/mergin/sync/public_api_v2_controller.py b/server/mergin/sync/public_api_v2_controller.py
index 82dfc4a5..e95029e1 100644
--- a/server/mergin/sync/public_api_v2_controller.py
+++ b/server/mergin/sync/public_api_v2_controller.py
@@ -47,13 +47,10 @@ def update_project(id):
     """Rename project"""
     project = require_project_by_uuid(id, ProjectPermissions.Update)
     new_name = request.json["name"].strip()
-
-    if not new_name:
-        abort(400, "Project name cannot be empty")
-    validation = project_name_validation(new_name)
-    if validation:
+    validation_error = project_name_validation(new_name)
+    if validation_error:
         return (
-            jsonify(code="InvalidProjectName", detail=validation),
+            jsonify(code="InvalidProjectName", detail=validation_error),
             400,
         )
     new_name_exists = Project.query.filter_by(
diff --git a/server/mergin/sync/utils.py b/server/mergin/sync/utils.py
index bf26689e..13374ce6 100644
--- a/server/mergin/sync/utils.py
+++ b/server/mergin/sync/utils.py
@@ -262,31 +262,36 @@ def convert_byte(size_bytes, unit):
     return size_bytes
 
 
-def is_reserved_word(name: str) -> bool:
+def is_reserved_word(name: str) -> str | None:
     """Check if name is reserved in system"""
     reserved = r"^support$|^helpdesk$|^merginmaps$|^lutraconsulting$|^mergin$|^lutra$|^input$|^admin$|^sales$"
-    return re.match(reserved, name) is not None
+    if re.match(reserved, name) is not None:
+        return "The provided value is invalid."
+    return None
 
 
-def is_valid_character(name: str) -> bool:
+def has_valid_characters(name: str) -> str | None:
     """Check if name contains only valid characters"""
-    return re.match(r"^[\w\s\-\.]+$", name) is not None
+    if re.match(r"^[\w\s\-\.]+$", name) is None:
+        return "Please use only alphanumeric or the following -_. characters."
+    return None
 
 
-def is_valid_first_character(name: str) -> bool:
+def has_valid_first_character(name: str) -> str | None:
     """Check if name contains only valid characters in first position"""
-    return re.match(r"^[\s^\.].*$", name) is None
+    if re.match(r"^[\s^\.].*$", name) is not None:
+        return f"Value can not start with space or dot."
+    return None
 
 
-def is_valid_filename(name: str) -> bool:
+def is_valid_filename(name: str) -> str | None:
     """Check if name contains only valid characters for filename"""
-    is_valid = True
+    error = None
     try:
         validate_filename(name)
     except ValidationError:
-        is_valid = False
-
-    return is_valid
+        error = "The provided value is invalid."
+    return error
 
 
 def workspace_names(workspaces):
diff --git a/server/mergin/tests/test_project_controller.py b/server/mergin/tests/test_project_controller.py
index 76c94d89..b7d97e65 100644
--- a/server/mergin/tests/test_project_controller.py
+++ b/server/mergin/tests/test_project_controller.py
@@ -386,6 +386,7 @@ def test_get_projects_by_names(client):
     ({"name": "foo/bar", "template": test_project}, 400),  # invalid project name
     ({"name": "ba%r", "template": test_project}, 400),  # invalid project name
     ({"name": "bar*", "template": test_project}, 400),  # invalid project name
+    ({"name": "  ", "template": test_project}, 400),  # empty
     ({"name": "support", "template": test_project}, 400),  # forbidden project name
     ({"name": test_project}, 409),
 ]
@@ -423,6 +424,7 @@ def test_add_project(client, app, data, expected):
     )
     assert resp.status_code == expected
     if expected == 200:
+        print(resp.data)
         project = Project.query.filter_by(
             name=data["name"].strip(), workspace_id=test_workspace_id
         ).first()
diff --git a/server/mergin/tests/test_public_api_v2.py b/server/mergin/tests/test_public_api_v2.py
index a17d1696..2d88d652 100644
--- a/server/mergin/tests/test_public_api_v2.py
+++ b/server/mergin/tests/test_public_api_v2.py
@@ -64,7 +64,7 @@ def test_rename_project(client):
     assert response.status_code == 400
     assert response.json["code"] == "InvalidProjectName"
     response = client.patch(
-        f"v2/projects/{project.id}", json={"name": " new_project_name"}
+        f"v2/projects/{project.id}", json={"name": ".new_project_name"}
     )
     assert response.status_code == 400
     assert response.json["code"] == "InvalidProjectName"
diff --git a/server/mergin/tests/test_utils.py b/server/mergin/tests/test_utils.py
index 8d6f2f81..69492b63 100644
--- a/server/mergin/tests/test_utils.py
+++ b/server/mergin/tests/test_utils.py
@@ -15,8 +15,8 @@
     parse_gpkgb_header_size,
     gpkg_wkb_to_wkt,
     is_reserved_word,
-    is_valid_character,
-    is_valid_first_character,
+    has_valid_characters,
+    has_valid_first_character,
     is_valid_filename,
 )
 from ..auth.models import LoginHistory, User
@@ -223,8 +223,11 @@ def test_is_name_allowed():
         expected = t[1]
         print(is_reserved_word(name))
         assert (
-            is_valid_character(name)
-            and is_valid_first_character(name)
-            and is_valid_filename(name)
-            and not is_reserved_word(name)
-        ) == expected
+            not (
+                has_valid_characters(name)
+                and has_valid_first_character(name)
+                and is_valid_filename(name)
+                and is_reserved_word(name)
+            )
+            == expected
+        )

From f1e757731e188ba060c7bcc01db72e6c38204945 Mon Sep 17 00:00:00 2001
From: "marcel.kocisek" <marcel.kocisek@lutraconsulting.co.uk>
Date: Tue, 28 Jan 2025 12:24:24 +0100
Subject: [PATCH 3/3] Clenup: print

---
 server/mergin/tests/test_project_controller.py | 1 -
 server/mergin/tests/test_utils.py              | 1 -
 2 files changed, 2 deletions(-)

diff --git a/server/mergin/tests/test_project_controller.py b/server/mergin/tests/test_project_controller.py
index b7d97e65..8d25093f 100644
--- a/server/mergin/tests/test_project_controller.py
+++ b/server/mergin/tests/test_project_controller.py
@@ -424,7 +424,6 @@ def test_add_project(client, app, data, expected):
     )
     assert resp.status_code == expected
     if expected == 200:
-        print(resp.data)
         project = Project.query.filter_by(
             name=data["name"].strip(), workspace_id=test_workspace_id
         ).first()
diff --git a/server/mergin/tests/test_utils.py b/server/mergin/tests/test_utils.py
index 69492b63..4b982a7f 100644
--- a/server/mergin/tests/test_utils.py
+++ b/server/mergin/tests/test_utils.py
@@ -221,7 +221,6 @@ def test_is_name_allowed():
     for t in test_cases:
         name = t[0]
         expected = t[1]
-        print(is_reserved_word(name))
         assert (
             not (
                 has_valid_characters(name)