CVE-2022-3438: URL Redirection to Untrusted Site ('Open Redirect') (CWE-601)
value: Open Redirect in GitHub repository ikus060/rdiffweb prior to 2.5.0a4.
CWE description: A web application accepts a user-controlled input that specifies a link to an external site, and uses that link in a Redirect. This simplifies phishing attacks.
CVSS v3 base score: 6.1
Published: 2022-10-10T12:15Z
Repository: https://github.com/ikus060/rdiffweb
Language: Python
Files changed: README.md, rdiffweb/controller/page_admin_users.py, rdiffweb/controller/page_pref_general.py, rdiffweb/controller/tests/test_page_admin_users.py, rdiffweb/controller/tests/test_page_prefs_general.py, rdiffweb/core/model/_user.py
Commit date: 2022-09-30 13:53:45 +0000
Commit message: Enforce validation on fullname, username and email for increase security #224

Diff: diff --git a/README.md b/README.md
index b5b1a3843..b237f3520 100644
--- a/README.md
+++ b/README.md
@@ -129,10 +129,11 @@ This next release focus on two-factor-authentication as a measure to increase se
 * Handle migration of older Rdiffweb database by adding the missing `repos.Encoding`, `repos.keepdays` and `users.role` columns #185
 * Replace deprecated references of `disutils.spawn.find_executable()` by `shutil.which()` #208
 * Add two-factor authentication with email verification #201
 * Generate a new session on login and 2FA #220
 * Enforce permission on /etc/rdiffweb configuration folder
+* Enforce validation on fullname, username and email
 
 Breaking changes:
 
 * Drop Ubuntu Hirsute & Impish (End-of-life)
 * `session-dir` is deprecated and should be replace by `rate-limit-dir`. User's session are stored in database.
diff --git a/rdiffweb/controller/page_admin_users.py b/rdiffweb/controller/page_admin_users.py
index b6d4e72da..953ee7193 100644
--- a/rdiffweb/controller/page_admin_users.py
+++ b/rdiffweb/controller/page_admin_users.py
@@ -72,24 +72,28 @@ class UserForm(CherryForm):
     username = StringField(
         _('Username'),
         validators=[
             validators.data_required(),
             validators.length(max=256, message=_('Username too long.')),
+            validators.length(min=3, message=_('Username too short.')),
+            validators.regexp(UserObject.PATTERN_USERNAME, message=_('Must not contain any special characters.')),
         ],
     )
     fullname = StringField(
         _('Fullname'),
         validators=[
             validators.optional(),
             validators.length(max=256, message=_('Fullname too long.')),
+            validators.regexp(UserObject.PATTERN_FULLNAME, message=_('Must not contain any special characters.')),
         ],
     )
     email = EmailField(
         _('Email'),
         validators=[
             validators.optional(),
             validators.length(max=256, message=_('Email too long.')),
+            validators.regexp(UserObject.PATTERN_EMAIL, message=_('Must be a valid email address.')),
         ],
     )
     password = PasswordField(
         _('Password'),
         validators=[validators.optional()],
@@ -138,21 +142,10 @@ class UserForm(CherryForm):
         validators=[validators.optional()],
         description=_("Disk spaces (in bytes) used by this user."),
         widget=widgets.HiddenInput(),
     )
 
-    def __init__(self, *args, **kwargs):
-        super().__init__(*args, **kwargs)
-        cfg = cherrypy.tree.apps[''].cfg
-        self.password.validators += [
-            validators.length(
-                min=cfg.password_min_length,
-                max=cfg.password_max_length,
-                message=_('Password must have between %(min)d and %(max)d characters.'),
-            )
-        ]
-
     def validate_role(self, field):
         # Don't allow the user to changes it's "role" state.
         currentuser = cherrypy.request.currentuser
         if self.username.data == currentuser.username and self.role.data != currentuser.role:
             raise ValueError(_('Cannot edit your own role.'))
diff --git a/rdiffweb/controller/page_pref_general.py b/rdiffweb/controller/page_pref_general.py
index 8988e792e..df2b731ca 100644
--- a/rdiffweb/controller/page_pref_general.py
+++ b/rdiffweb/controller/page_pref_general.py
@@ -17,44 +17,38 @@
 """
 Default preference page to show general user information. It allows user
 to change password ans refresh it's repository view.
 """
 
-import logging
-import re
-
 import cherrypy
 from wtforms.fields import HiddenField, PasswordField, StringField, SubmitField
 from wtforms.fields.html5 import EmailField
 from wtforms.validators import DataRequired, EqualTo, InputRequired, Length, Optional, Regexp
 
 from rdiffweb.controller import Controller, flash
 from rdiffweb.controller.form import CherryForm
+from rdiffweb.core.model import UserObject
 from rdiffweb.tools.i18n import gettext_lazy as _
 
-# Define the logger
-_logger = logging.getLogger(__name__)
-
-PATTERN_EMAIL = re.compile(r'[a-z0-9._%+-]+@[a-z0-9.-]+\.[a-z]{2,4}$')
-
 
 class UserProfileForm(CherryForm):
     action = HiddenField(default='set_profile_info')
     username = StringField(_('Username'), render_kw={'readonly': True})
     fullname = StringField(
         _('Fullname'),
         validators=[
             Optional(),
             Length(max=256, message=_('Fullname too long.')),
+            Regexp(UserObject.PATTERN_FULLNAME, message=_('Must not contain any special characters.')),
         ],
     )
     email = EmailField(
         _('Email'),
         validators=[
             DataRequired(),
-            Length(max=256, message=_("Invalid email.")),
-            Regexp(PATTERN_EMAIL, message=_("Invalid email.")),
+            Length(max=256, message=_("Email too long.")),
+            Regexp(UserObject.PATTERN_EMAIL, message=_("Must be a valid email address.")),
         ],
     )
     set_profile_info = SubmitField(_('Save changes'))
 
     def is_submitted(self):
diff --git a/rdiffweb/controller/tests/test_page_admin_users.py b/rdiffweb/controller/tests/test_page_admin_users.py
index c7331a742..c32a88f44 100644
--- a/rdiffweb/controller/tests/test_page_admin_users.py
+++ b/rdiffweb/controller/tests/test_page_admin_users.py
@@ -16,10 +16,11 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 from unittest.mock import ANY, MagicMock
 
 import cherrypy
+from parameterized import parameterized
 
 import rdiffweb.test
 from rdiffweb.core.model import UserObject
 
 
@@ -168,44 +169,62 @@ class AbstractAdminTest(rdiffweb.test.WebCase):
         self.listener.user_deleted.assert_called()
         self.assertStatus(200)
         self.assertInBody("User account removed.")
         self.assertNotInBody("test2")
 
-    def test_edit_fullname(self):
+    @parameterized.expand(
+        [
+            # Invalid
+            ('evil.com', False),
+            ('http://test', False),
+            ('email@test.test', False),
+            ('/test/', False),
+            # Valid
+            ('My fullname', True),
+            ('Test Test', True),
+            ('Éric Terrien-Pascal', True),
+            ("Tel'c", True),
+        ]
+    )
+    def test_edit_fullname_with_special_character(self, new_fullname, expected_valid):
         # Given an existing user
         # When updating the user's fullname
         self.getPage(
             "/admin/users/",
             method='POST',
-            body={'action': 'edit', 'username': self.USERNAME, 'fullname': 'My fullname'},
+            body={'action': 'edit', 'username': self.USERNAME, 'fullname': new_fullname},
         )
         self.assertStatus(200)
-        # Then user is updated successfully
-        self.assertInBody("User information modified successfully.")
-        # Then database is updated
-        obj = UserObject.query.filter(UserObject.username == self.USERNAME).first()
-        self.assertEqual('My fullname', obj.fullname)
-
-    def test_add_edit_delete_user_with_encoding(self):
-        """
-        Check creation of user with non-ascii char.
-        """
-        self._add_user("Éric", "éric@test.com", "pr3j5Dwi", "/home/", UserObject.USER_ROLE)
-        self.assertInBody("User added successfully.")
-        self.assertInBody("Éric")
-        self.assertInBody("éric@test.com")
-        # Update user
-        self._edit_user("Éric", "eric.létourno@test.com", "écureuil", "/tmp/", UserObject.ADMIN_ROLE)
-        self.assertInBody("User information modified successfully.")
-        self.assertInBody("Éric")
-        self.assertInBody("eric.létourno@test.com")
-        self.assertNotInBody("/home/")
-        self.assertInBody("/tmp/")
-
-        self._delete_user("Éric")
-        self.assertInBody("User account removed.")
-        self.assertNotInBody("Éric")
+        if expected_valid:
+            self.assertInBody("User information modified successfully.")
+            self.assertNotInBody("Fullname: Must not contain any special characters.")
+        else:
+            self.assertNotInBody("User information modified successfully.")
+            self.assertInBody("Fullname: Must not contain any special characters.")
+
+    @parameterized.expand(
+        [
+            # Invalid
+            ('http://username', False),
+            ('username@test.test', False),
+            ('/username/', False),
+            # Valid
+            ('username.com', True),
+            ('admin_user', True),
+            ('test.test', True),
+            ('test-test', True),
+        ]
+    )
+    def test_add_user_with_special_character(self, new_username, expected_valid):
+        self._add_user(new_username, "eric@test.com", "pr3j5Dwi", "/home/", UserObject.USER_ROLE)
+        self.assertStatus(200)
+        if expected_valid:
+            self.assertInBody("User added successfully.")
+            self.assertNotInBody("Username: Must not contain any special characters.")
+        else:
+            self.assertNotInBody("User added successfully.")
+            self.assertInBody("Username: Must not contain any special characters.")
 
     def test_add_user_with_empty_username(self):
         """
         Verify failure trying to create user without username.
         """
diff --git a/rdiffweb/controller/tests/test_page_prefs_general.py b/rdiffweb/controller/tests/test_page_prefs_general.py
index bfe152218..b46765d18 100644
--- a/rdiffweb/controller/tests/test_page_prefs_general.py
+++ b/rdiffweb/controller/tests/test_page_prefs_general.py
@@ -21,10 +21,11 @@ Created on Dec 26, 2015
 """
 
 from unittest.mock import MagicMock
 
 import cherrypy
+from parameterized import parameterized
 
 import rdiffweb.test
 from rdiffweb.core.model import RepoObject, UserObject
 
 
@@ -86,20 +87,35 @@ class PagePrefGeneralTest(rdiffweb.test.WebCase):
         # Then database is updated with fullname
         user = UserObject.query.filter(UserObject.username == self.USERNAME).first()
         self.assertIsNotNone(user)
         self.assertEqual("test@test.com", user.email)
 
-    def test_change_fullname(self):
+    @parameterized.expand(
+        [
+            # Invalid
+            ('@test.com', False),
+            ('test.com', False),
+            ('test@te_st.com', False),
+            ('test@test.com, test2@test.com', False),
+            # Valid
+            ('test', True),
+            ('My Fullname', True),
+        ]
+    )
+    def test_change_fullname(self, new_fullname, expected_valid):
         # Given an authenticated user
         # When update the fullname
-        self._set_profile_info("test@test.com", "My Fullname")
+        self._set_profile_info("test@test.com", new_fullname)
         self.assertStatus(200)
-        self.assertInBody("Profile updated successfully.")
-        # Then database is updated with fullname
-        self.assertInBody("My Fullname")
-        user = UserObject.query.filter(UserObject.username == self.USERNAME).first()
-        self.assertEqual("My Fullname", user.fullname)
+        if expected_valid:
+            self.assertInBody("Profile updated successfully.")
+            # Then database is updated with fullname
+            self.assertInBody(new_fullname)
+            user = UserObject.query.filter(UserObject.username == self.USERNAME).first()
+            self.assertEqual(new_fullname, user.fullname)
+        else:
+            self.assertNotInBody("Profile updated successfully.")
 
     def test_change_fullname_method_get(self):
         # Given an authenticated user
         # When trying to update full name using GET method
         self.getPage(self.PREFS + '?action=set_profile_info&email=test@test.com')
@@ -124,34 +140,35 @@ class PagePrefGeneralTest(rdiffweb.test.WebCase):
     def test_change_email(self):
         self._set_profile_info("test@test.com")
         self.assertStatus(200)
         self.assertInBody("Profile updated successfully.")
 
-    def test_change_email_with_invalid_email(self):
-        self._set_profile_info("@test.com")
-        self.assertStatus(200)
-        self.assertInBody("Invalid email")
-
-        self._set_profile_info("test.com")
-        self.assertStatus(200)
-        self.assertInBody("Invalid email")
-
-        self._set_profile_info("test")
-        self.assertStatus(200)
-        self.assertInBody("Invalid email")
-
-        self._set_profile_info("test@te_st.com")
-        self.assertStatus(200)
-        self.assertInBody("Invalid email")
-
-        self._set_profile_info("test@test.com, test2@test.com")
+    @parameterized.expand(
+        [
+            # Invalid
+            ('@test.com', False),
+            ('test.com', False),
+            ('test', False),
+            ('test@te_st.com', False),
+            ('test@test.com, test2@test.com', False),
+            # Valid
+            ('test@test.com', True),
+        ]
+    )
+    def test_change_email_with_invalid_email(self, new_email, expected_valid):
+        self._set_profile_info(new_email)
         self.assertStatus(200)
-        self.assertInBody("Invalid email")
+        if expected_valid:
+            self.assertInBody("Profile updated successfully.")
+            self.assertNotInBody("Must be a valid email address.")
+        else:
+            self.assertNotInBody("Profile updated successfully.")
+            self.assertInBody("Must be a valid email address.")
 
     def test_change_email_with_too_long(self):
         self._set_profile_info(("test1" * 50) + "@test.com")
-        self.assertInBody("Invalid email")
+        self.assertInBody("Email too long.")
 
     def test_change_password(self):
         self.listener.user_password_changed.reset_mock()
         # When udating user's password
         self._set_password(self.PASSWORD, "pr3j5Dwi", "pr3j5Dwi")
diff --git a/rdiffweb/core/model/_user.py b/rdiffweb/core/model/_user.py
index 0e8469794..441e10b3c 100644
--- a/rdiffweb/core/model/_user.py
+++ b/rdiffweb/core/model/_user.py
@@ -53,21 +53,28 @@ class DuplicateSSHKeyError(Exception):
 
 class UserObject(Base):
     __tablename__ = 'users'
     __table_args__ = {'sqlite_autoincrement': True}
 
+    # Value for role.
     ADMIN_ROLE = 0
     MAINTAINER_ROLE = 5
     USER_ROLE = 10
     ROLES = {
         'admin': ADMIN_ROLE,
         'maintainer': MAINTAINER_ROLE,
         'user': USER_ROLE,
     }
+    # Value for mfa field
     DISABLED_MFA = 0
     ENABLED_MFA = 1
 
+    # Regex pattern to be used for validation.
+    PATTERN_EMAIL = r"[a-z0-9._%+-]+@[a-z0-9.-]+\.[a-z]{2,4}$"
+    PATTERN_FULLNAME = r"""[^!"#$%&()*+,./:;<=>?@[\]_{|}~]+$"""
+    PATTERN_USERNAME = r"[a-zA-Z0-9_.\-]+$"
+
     userid = Column('UserID', Integer, primary_key=True)
     _username = Column('Username', String, nullable=False, unique=True)
     hash_password = Column('Password', String, nullable=False, default="")
     _user_root = Column('UserRoot', String, nullable=False, default="")
     _is_admin = deferred(

Vulnerable code:     def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        cfg = cherrypy.tree.apps[''].cfg
        self.password.validators += [
            validators.length(
                min=cfg.password_min_length,
                max=cfg.password_max_length,
                message=_('Password must have between %(min)d and %(max)d characters.'),
            )
        ]

import logging
import re

# Define the logger
_logger = logging.getLogger(__name__)

PATTERN_EMAIL = re.compile(r'[a-z0-9._%+-]+@[a-z0-9.-]+\.[a-z]{2,4}$')

            Length(max=256, message=_("Invalid email.")),
            Regexp(PATTERN_EMAIL, message=_("Invalid email.")),
    def test_edit_fullname(self):
            body={'action': 'edit', 'username': self.USERNAME, 'fullname': 'My fullname'},
        # Then user is updated successfully
        self.assertInBody("User information modified successfully.")
        # Then database is updated
        obj = UserObject.query.filter(UserObject.username == self.USERNAME).first()
        self.assertEqual('My fullname', obj.fullname)

    def test_add_edit_delete_user_with_encoding(self):
        """
        Check creation of user with non-ascii char.
        """
        self._add_user("Éric", "éric@test.com", "pr3j5Dwi", "/home/", UserObject.USER_ROLE)
        self.assertInBody("User added successfully.")
        self.assertInBody("Éric")
        self.assertInBody("éric@test.com")
        # Update user
        self._edit_user("Éric", "eric.létourno@test.com", "écureuil", "/tmp/", UserObject.ADMIN_ROLE)
        self.assertInBody("User information modified successfully.")
        self.assertInBody("Éric")
        self.assertInBody("eric.létourno@test.com")
        self.assertNotInBody("/home/")
        self.assertInBody("/tmp/")

        self._delete_user("Éric")
        self.assertInBody("User account removed.")
        self.assertNotInBody("Éric")
    def test_change_fullname(self):
        self._set_profile_info("test@test.com", "My Fullname")
        self.assertInBody("Profile updated successfully.")
        # Then database is updated with fullname
        self.assertInBody("My Fullname")
        user = UserObject.query.filter(UserObject.username == self.USERNAME).first()
        self.assertEqual("My Fullname", user.fullname)
    def test_change_email_with_invalid_email(self):
        self._set_profile_info("@test.com")
        self.assertStatus(200)
        self.assertInBody("Invalid email")

        self._set_profile_info("test.com")
        self.assertStatus(200)
        self.assertInBody("Invalid email")

        self._set_profile_info("test")
        self.assertStatus(200)
        self.assertInBody("Invalid email")

        self._set_profile_info("test@te_st.com")
        self.assertStatus(200)
        self.assertInBody("Invalid email")

        self._set_profile_info("test@test.com, test2@test.com")
        self.assertInBody("Invalid email")
        self.assertInBody("Invalid email")
Fixed code: * Enforce validation on fullname, username and email
            validators.length(min=3, message=_('Username too short.')),
            validators.regexp(UserObject.PATTERN_USERNAME, message=_('Must not contain any special characters.')),
            validators.regexp(UserObject.PATTERN_FULLNAME, message=_('Must not contain any special characters.')),
            validators.regexp(UserObject.PATTERN_EMAIL, message=_('Must be a valid email address.')),
from rdiffweb.core.model import UserObject
            Regexp(UserObject.PATTERN_FULLNAME, message=_('Must not contain any special characters.')),
            Length(max=256, message=_("Email too long.")),
            Regexp(UserObject.PATTERN_EMAIL, message=_("Must be a valid email address.")),
from parameterized import parameterized
    @parameterized.expand(
        [
            # Invalid
            ('evil.com', False),
            ('http://test', False),
            ('email@test.test', False),
            ('/test/', False),
            # Valid
            ('My fullname', True),
            ('Test Test', True),
            ('Éric Terrien-Pascal', True),
            ("Tel'c", True),
        ]
    )
    def test_edit_fullname_with_special_character(self, new_fullname, expected_valid):
            body={'action': 'edit', 'username': self.USERNAME, 'fullname': new_fullname},
        if expected_valid:
            self.assertInBody("User information modified successfully.")
            self.assertNotInBody("Fullname: Must not contain any special characters.")
        else:
            self.assertNotInBody("User information modified successfully.")
            self.assertInBody("Fullname: Must not contain any special characters.")

    @parameterized.expand(
        [
            # Invalid
            ('http://username', False),
            ('username@test.test', False),
            ('/username/', False),
            # Valid
            ('username.com', True),
            ('admin_user', True),
            ('test.test', True),
            ('test-test', True),
        ]
    )
    def test_add_user_with_special_character(self, new_username, expected_valid):
        self._add_user(new_username, "eric@test.com", "pr3j5Dwi", "/home/", UserObject.USER_ROLE)
        self.assertStatus(200)
        if expected_valid:
            self.assertInBody("User added successfully.")
            self.assertNotInBody("Username: Must not contain any special characters.")
        else:
            self.assertNotInBody("User added successfully.")
            self.assertInBody("Username: Must not contain any special characters.")
from parameterized import parameterized
    @parameterized.expand(
        [
            # Invalid
            ('@test.com', False),
            ('test.com', False),
            ('test@te_st.com', False),
            ('test@test.com, test2@test.com', False),
            # Valid
            ('test', True),
            ('My Fullname', True),
        ]
    )
    def test_change_fullname(self, new_fullname, expected_valid):
        self._set_profile_info("test@test.com", new_fullname)
        if expected_valid:
            self.assertInBody("Profile updated successfully.")
            # Then database is updated with fullname
            self.assertInBody(new_fullname)
            user = UserObject.query.filter(UserObject.username == self.USERNAME).first()
            self.assertEqual(new_fullname, user.fullname)
        else:
            self.assertNotInBody("Profile updated successfully.")
    @parameterized.expand(
        [
            # Invalid
            ('@test.com', False),
            ('test.com', False),
            ('test', False),
            ('test@te_st.com', False),
            ('test@test.com, test2@test.com', False),
            # Valid
            ('test@test.com', True),
        ]
    )
    def test_change_email_with_invalid_email(self, new_email, expected_valid):
        self._set_profile_info(new_email)
        if expected_valid:
            self.assertInBody("Profile updated successfully.")
            self.assertNotInBody("Must be a valid email address.")
        else:
            self.assertNotInBody("Profile updated successfully.")
            self.assertInBody("Must be a valid email address.")
        self.assertInBody("Email too long.")
    # Value for role.
    # Value for mfa field
    # Regex pattern to be used for validation.
    PATTERN_EMAIL = r"[a-z0-9._%+-]+@[a-z0-9.-]+\.[a-z]{2,4}$"
    PATTERN_FULLNAME = r"""[^!"#$%&()*+,./:;<=>?@[\]_{|}~]+$"""
    PATTERN_USERNAME = r"[a-zA-Z0-9_.\-]+$"