Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
8fb35c8
Add email validation function to forms.py
harminius Oct 25, 2024
dd4dfed
Add email address validation in registration
harminius Oct 25, 2024
ce879f7
black .
harminius Oct 25, 2024
7062f60
Do not allow non utf8 encoding and invalid characters in email address
harminius Nov 29, 2024
9758107
Revert unwanted changes
harminius Nov 29, 2024
cd664ae
Add valid email function
harminius Dec 9, 2024
5f0006f
Unify error messages for unsuported file type in public_api_controll…
harminius Feb 28, 2025
6ec65bb
Extend email validator with additional forbiddedn characters
harminius Feb 28, 2025
1a297c5
Merge pull request #388 from MerginMaps/master
MarcelGeo Feb 28, 2025
04ae6c3
rm dash from username in generate_username()
harminius Feb 28, 2025
5a35819
Fix migration file typo
harminius Feb 28, 2025
b2f25c4
Fix assert messages in tests.
harminius Feb 28, 2025
d4a86a5
black
harminius Feb 28, 2025
d682421
add required parameter in test
harminius Feb 28, 2025
1a8ad35
rm unused imports
harminius Feb 28, 2025
5d1767e
Clarify what email validation checks
harminius Mar 3, 2025
52cd307
Test validation not an endpoint
harminius Mar 3, 2025
e3f5117
Add db rollback on gevent timeout error
varmar05 Mar 3, 2025
01ec5d6
Merge pull request #308 from MerginMaps/enforce_valid_email_address_ce
harminius Mar 3, 2025
32005ef
Fix tests
varmar05 Mar 3, 2025
08f395e
black
varmar05 Mar 3, 2025
ec3dc14
Merge pull request #389 from MerginMaps/fix_rollback_error
MarcelGeo Mar 3, 2025
e878bb7
Fix generate_username to be case insensitive
varmar05 Mar 5, 2025
6bc4bef
Merge pull request #394 from MerginMaps/fix_username_generation
harminius Mar 5, 2025
99a4aac
Merge pull request #396 from MerginMaps/master
MarcelGeo Mar 6, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 24 additions & 4 deletions server/mergin/auth/forms.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -71,7 +91,7 @@ class RegisterUserForm(FlaskForm):
)
email = CustomStringField(
"Email Address",
validators=[DataRequired(), Email()],
validators=[DataRequired(), ExtendedEmail()],
)

def validate(self):
Expand All @@ -94,7 +114,7 @@ def validate(self):
class ResetPasswordForm(FlaskForm):
email = CustomStringField(
"Email Address",
[DataRequired(), Email()],
[DataRequired(), ExtendedEmail()],
)


Expand Down Expand Up @@ -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):
Expand Down
10 changes: 5 additions & 5 deletions server/mergin/auth/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
"""
),
Expand Down
16 changes: 14 additions & 2 deletions server/mergin/sync/public_api_controller.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import uuid
from datetime import datetime

import gevent
import psycopg2
from blinker import signal
from connexion import NoContent, request
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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()

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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()
Expand Down
1 change: 0 additions & 1 deletion server/mergin/sync/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
FileHistory,
PushChangeType,
ProjectRole,
ProjectUser,
)
from .workspace import WorkspaceRole
from ..app import DateTimeWithZ, ma
Expand Down
2 changes: 1 addition & 1 deletion server/mergin/sync/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
42 changes: 38 additions & 4 deletions server/mergin/tests/test_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -32,7 +30,6 @@
login_as_admin,
login,
upload_file_to_project,
test_project_dir,
)


Expand Down Expand Up @@ -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
]


Expand Down Expand Up @@ -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"""
Expand Down Expand Up @@ -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
42 changes: 41 additions & 1 deletion server/mergin/tests/test_middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand All @@ -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()
7 changes: 5 additions & 2 deletions server/mergin/tests/test_project_controller.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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."
)
Original file line number Diff line number Diff line change
@@ -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
Expand Down
Loading