diff --git a/server/mergin/auth/forms.py b/server/mergin/auth/forms.py index 30a615fa..f4baa69e 100644 --- a/server/mergin/auth/forms.py +++ b/server/mergin/auth/forms.py @@ -20,13 +20,22 @@ def username_validation(form, field): - from ..sync.utils import is_name_allowed + from ..sync.utils import ( + has_valid_characters, + has_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 = [ + has_valid_characters(field.data), + has_valid_first_character(field.data), + is_reserved_word(field.data), + is_valid_filename(field.data), + ] + for error in errors: + if error: + raise ValidationError(error) class PasswordValidator: diff --git a/server/mergin/sync/forms.py b/server/mergin/sync/forms.py index 275380b1..f5aa484e 100644 --- a/server/mergin/sync/forms.py +++ b/server/mergin/sync/forms.py @@ -6,6 +6,29 @@ from wtforms.validators import DataRequired from flask_wtf import FlaskForm +from mergin.sync.utils import ( + is_reserved_word, + has_valid_characters, + is_valid_filename, + 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 = [ + has_valid_characters(name), + has_valid_first_character(name), + is_reserved_word(name), + is_valid_filename(name), + ] + for error in errors: + if error: + 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..0dd3d3a9 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, ) @@ -179,10 +180,11 @@ def add_project(namespace): # noqa: E501 """ request.json["name"] = request.json["name"].strip() - if not is_name_allowed(request.json["name"]): + validation_error = project_name_validation(request.json["name"]) + if validation_error: abort( 400, - "Please don't start project name with . and use only alphanumeric or these -._! characters in project name.", + validation_error, ) if request.is_json: @@ -1194,6 +1196,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 +1208,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.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 d5d36b11..e95029e1 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,11 @@ def delete_project_now(id): def update_project(id): """Rename project""" project = require_project_by_uuid(id, ProjectPermissions.Update) - new_name = request.json["name"] - - if not is_name_allowed(new_name): + new_name = request.json["name"].strip() + validation_error = project_name_validation(new_name) + if validation_error: return ( - jsonify( - code="InvalidProjectName", detail="Entered project name is invalid" - ), + 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 b4120acc..13374ce6 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,36 @@ 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) -> str | None: + """Check if name is reserved in system""" + reserved = r"^support$|^helpdesk$|^merginmaps$|^lutraconsulting$|^mergin$|^lutra$|^input$|^admin$|^sales$" + if re.match(reserved, name) is not None: + return "The provided value is invalid." + return 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 has_valid_characters(name: str) -> str | None: + """Check if name contains only valid characters""" + if re.match(r"^[\w\s\-\.]+$", name) is None: + return "Please use only alphanumeric or the following -_. characters." + return None + + +def has_valid_first_character(name: str) -> str | None: + """Check if name contains only valid characters in first position""" + 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) -> str | None: + """Check if name contains only valid characters for filename""" + error = None + try: + validate_filename(name) + except ValidationError: + 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..8d25093f 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), ] 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 6141e573..4b982a7f 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, + has_valid_characters, + has_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,19 @@ 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] + assert ( + not ( + has_valid_characters(name) + and has_valid_first_character(name) + and is_valid_filename(name) + and is_reserved_word(name) + ) + == expected + )