diff --git a/server/mergin/auth/forms.py b/server/mergin/auth/forms.py index a5def163..b4c2a14d 100644 --- a/server/mergin/auth/forms.py +++ b/server/mergin/auth/forms.py @@ -1,7 +1,7 @@ # Copyright (C) Lutra Consulting Limited # # SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial - +import re import safe from flask_wtf import FlaskForm from sqlalchemy import func @@ -42,6 +42,26 @@ def username_validation(form, field): raise ValidationError(error) +class ExtendedEmail(Email): + """Custom email validation extending WTForms email validation. + WTForms checks for + 1. spaces, + 2. special characters ,:;()<>[]\" + 3, multiple @ symbols, + 4, leading, trailing, or consecutive dots in the local part + 5, invalid domain part - missing top level domain (user@example), consecutive dots + Custom check for additional invalid characters disallows |'— because they make our email sending service to fail + """ + + def __call__(self, form, field): + super().__call__(form, field) + + if re.search(r"[|'—]", field.data): + raise ValidationError( + f"Email address '{field.data}' contains an invalid character." + ) + + class PasswordValidator: def __init__(self, min_length=8): self.min_length = min_length @@ -71,7 +91,7 @@ class RegisterUserForm(FlaskForm): ) email = CustomStringField( "Email Address", - validators=[DataRequired(), Email()], + validators=[DataRequired(), ExtendedEmail()], ) def validate(self): @@ -94,7 +114,7 @@ def validate(self): class ResetPasswordForm(FlaskForm): email = CustomStringField( "Email Address", - [DataRequired(), Email()], + [DataRequired(), ExtendedEmail()], ) @@ -129,7 +149,7 @@ class UserProfileDataForm(UpdateForm): receive_notifications = BooleanField("Receive notifications", [Optional()]) first_name = CustomStringField("First Name", [Optional()]) last_name = CustomStringField("Last Name", [Optional()]) - email = CustomStringField("Email", [Optional(), Email()]) + email = CustomStringField("Email", [Optional(), ExtendedEmail()]) class ApiLoginForm(LoginForm): diff --git a/server/mergin/auth/models.py b/server/mergin/auth/models.py index 85741470..e580e814 100644 --- a/server/mergin/auth/models.py +++ b/server/mergin/auth/models.py @@ -199,7 +199,7 @@ def generate_username(cls, email: str) -> Optional[str]: username = email.split("@")[0].strip().lower() # remove forbidden chars username = re.sub( - r"[\@\#\$\%\^\&\*\(\)\{\}\[\]\?\'\"`,;\:\+\=\~\\\/\|\<\>]", "", username + r"[\@\#\$\%\^\&\*\(\)\{\}\[\]\?\'\"`,;\:\+\=\~\\\/\|\<\>—]", "", username ).ljust(4, "0") # additional check for reserved words username = f"{username}0" if is_reserved_word(username) else username @@ -210,12 +210,12 @@ def generate_username(cls, email: str) -> Optional[str]: text( """ SELECT - replace(username, :username, '0')::int AS suffix + replace(lower(username), :username, '0')::int AS suffix FROM "user" WHERE - username = :username OR - username SIMILAR TO :username_like - ORDER BY replace(username, :username, '0')::int DESC + lower(username) = :username OR + lower(username) SIMILAR TO :username_like + ORDER BY replace(lower(username), :username, '0')::int DESC LIMIT 1; """ ), diff --git a/server/mergin/sync/public_api_controller.py b/server/mergin/sync/public_api_controller.py index 5aada5b6..2a4829b9 100644 --- a/server/mergin/sync/public_api_controller.py +++ b/server/mergin/sync/public_api_controller.py @@ -13,6 +13,7 @@ import uuid from datetime import datetime +import gevent import psycopg2 from blinker import signal from connexion import NoContent, request @@ -824,7 +825,7 @@ def project_push(namespace, project_name): abort( 400, f"Unsupported file type detected: {item.path}. " - f"Please remove the file or try compressing it into a ZIP file before uploading", + f"Please remove the file or try compressing it into a ZIP file before uploading.", ) # changes' files must be unique @@ -951,6 +952,9 @@ def project_push(namespace, project_name): f"Failed to upload a new project version using transaction id: {upload.id}: {str(err)}" ) abort(422, "Failed to upload a new project version. Please try later.") + except gevent.timeout.Timeout: + db.session.rollback() + raise finally: upload.clear() @@ -1056,7 +1060,11 @@ def push_finish(transaction_id): continue if not is_supported_type(dest_file): logging.info(f"Rejecting blacklisted file: {dest_file}") - abort(400, f"Unsupported file type detected: {f.path}") + abort( + 400, + f"Unsupported file type detected: {f.path}. " + f"Please remove the file or try compressing it into a ZIP file before uploading.", + ) if expected_size != os.path.getsize(dest_file): logging.error( @@ -1165,6 +1173,10 @@ def push_finish(transaction_id): f"transaction id: {transaction_id}.: {str(err)}" ) abort(422, "Failed to create new version: {}".format(str(err))) + # catch exception during pg transaction so we can rollback and prevent PendingRollbackError during upload clean up + except gevent.timeout.Timeout: + db.session.rollback() + raise finally: # remove artifacts upload.clear() diff --git a/server/mergin/sync/schemas.py b/server/mergin/sync/schemas.py index 0d8a11d5..75b6f09e 100644 --- a/server/mergin/sync/schemas.py +++ b/server/mergin/sync/schemas.py @@ -16,7 +16,6 @@ FileHistory, PushChangeType, ProjectRole, - ProjectUser, ) from .workspace import WorkspaceRole from ..app import DateTimeWithZ, ma diff --git a/server/mergin/sync/utils.py b/server/mergin/sync/utils.py index 38e2fd68..8ee0f480 100644 --- a/server/mergin/sync/utils.py +++ b/server/mergin/sync/utils.py @@ -285,7 +285,7 @@ def has_valid_characters(name: str) -> str | 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: + if re.match(r"^[\s.].*$", name) is not None: return f"Value can not start with space or dot." return None diff --git a/server/mergin/tests/test_auth.py b/server/mergin/tests/test_auth.py index f70dbd53..c8b9a682 100644 --- a/server/mergin/tests/test_auth.py +++ b/server/mergin/tests/test_auth.py @@ -3,16 +3,14 @@ # SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial from datetime import datetime, timedelta -import os import time import pytest import json from flask import url_for -from itsdangerous import URLSafeTimedSerializer from sqlalchemy import desc from unittest.mock import patch -from mergin.tests import test_workspace +from ..auth.forms import ResetPasswordForm from ..auth.app import generate_confirmation_token, confirm_token from ..auth.models import User, UserProfile, LoginHistory from ..auth.tasks import anonymize_removed_users @@ -32,7 +30,6 @@ login_as_admin, login, upload_file_to_project, - test_project_dir, ) @@ -118,6 +115,8 @@ def test_logout(client): ), # tests with upper case, but email already exists (" mergin@mergin.com ", "#pwd123", 400), # invalid password ("verylonglonglonglonglonglonglongemail@example.com", "#pwd1234", 201), + ("us.er@mergin.com", "#pwd1234", 201), # dot is allowed + ("us er@mergin.com", "#pwd1234", 400), # space is disallowed ] @@ -857,6 +856,13 @@ def test_username_generation(client): == "verylonglonglonglonglo" ) + # test username generation with existing user, case insensitive + user = add_user("Testuser") + assert User.generate_username("Testuser@example.com") == "testuser1" + assert User.generate_username("testuser@example.com") == "testuser1" + user = add_user("testuser1") + assert User.generate_username("Testuser@example.com") == "testuser2" + def test_server_usage(client): """Test server usage endpoint""" @@ -889,3 +895,31 @@ def test_server_usage(client): assert resp.json["editors"] == 1 assert resp.json["users"] == 1 assert resp.json["projects"] == 1 + + +user_data = [ + ("user1", True), # no problem + ("日人日本人", True), # non-ascii character + ("usér", True), # non-ascii character + ("user\\", False), # disallowed character + ("user\260", True), # non-ascii character (°) + ("user|", False), # vertical bar + ("us er", False), # space in the middle + ("us,er", False), # comma + ("us—er", False), # dash + ("us'er", False), # apostrophe + (" user", True), # starting with space (will be stripped) + ("us.er", True), # dot in the middle + (".user", False), # starting with dot +] + + +@pytest.mark.parametrize("username,is_valid", user_data) +def test_email_format_validation(app, username, is_valid): + """Test email form format validation""" + email = username + "@email.com" + with app.test_request_context(method="POST"): + form = ResetPasswordForm( + data={"email": email} + ) # ResetPasswordForm has only email field + assert form.validate() == is_valid diff --git a/server/mergin/tests/test_middleware.py b/server/mergin/tests/test_middleware.py index 09f4bf05..508334d0 100644 --- a/server/mergin/tests/test_middleware.py +++ b/server/mergin/tests/test_middleware.py @@ -3,9 +3,11 @@ # SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial import gevent +import psycogreen.gevent import pytest +import sqlalchemy -from ..app import create_simple_app, GeventTimeoutMiddleware +from ..app import create_simple_app, GeventTimeoutMiddleware, db from ..config import Configuration @@ -31,3 +33,41 @@ def ping(): if use_middleware else 200 ) + + +def test_catch_timeout(): + """Test proper handling of gevent timeout with db.session.rollback""" + psycogreen.gevent.patch_psycopg() + Configuration.GEVENT_WORKER = True + Configuration.GEVENT_REQUEST_TIMEOUT = 1 + application = create_simple_app() + + def unhandled(): + try: + db.session.execute("SELECT pg_sleep(1.1);") + finally: + db.session.execute("SELECT 1;") + return "" + + def timeout(): + try: + db.session.execute("SELECT pg_sleep(1.1);") + except gevent.timeout.Timeout: + db.session.rollback() + raise + finally: + db.session.execute("SELECT 1;") + return "" + + application.add_url_rule("/unhandled", "unhandled", unhandled) + application.add_url_rule("/timeout", "timeout", timeout) + app_context = application.app_context() + app_context.push() + + assert application.test_client().get("/timeout").status_code == 504 + + # in case of missing rollback sqlalchemy would raise error + with pytest.raises(sqlalchemy.exc.PendingRollbackError): + application.test_client().get("/unhandled") + + db.session.rollback() diff --git a/server/mergin/tests/test_project_controller.py b/server/mergin/tests/test_project_controller.py index ac839d95..9761ea05 100644 --- a/server/mergin/tests/test_project_controller.py +++ b/server/mergin/tests/test_project_controller.py @@ -2576,7 +2576,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" @@ -2613,4 +2613,7 @@ def test_supported_file_upload(client): # Unsupported file type is revealed when reconstructed from chunks - based on the mime type - and upload is refused resp = client.post(f"/v1/project/push/finish/{upload.id}") assert resp.status_code == 400 - assert resp.json["detail"] == f"Unsupported file type detected: {spoof_name}" + 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." + ) diff --git a/server/migrations/community/1ab5b02ce532_version_author_name_to_user_id.py b/server/migrations/community/1ab5b02ce532_version_author_name_to_user_id.py index 93747d70..99d2f337 100644 --- a/server/migrations/community/1ab5b02ce532_version_author_name_to_user_id.py +++ b/server/migrations/community/1ab5b02ce532_version_author_name_to_user_id.py @@ -1,4 +1,4 @@ -"""Migrage project version author name to user.id +"""Migrate project version author name to user.id Revision ID: 1ab5b02ce532 Revises: 1c23e3be03a3