Enforce validation on fullname, username and email for increase secur…

…ity #224

README.md

@@ -131,6 +131,7 @@ This next release focus on two-factor-authentication as a measure to increase se
 * 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:
 

rdiffweb/controller/page_admin_users.py

@@ -74,20 +74,24 @@ class UserForm(CherryForm):
         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(
@@ -140,17 +144,6 @@ class UserForm(CherryForm):
         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

rdiffweb/controller/page_pref_general.py

@@ -19,23 +19,16 @@
 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')
@@ -45,14 +38,15 @@ class UserProfileForm(CherryForm):
         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'))

rdiffweb/controller/tests/test_page_admin_users.py

@@ -18,6 +18,7 @@
 from unittest.mock import ANY, MagicMock
 
 import cherrypy
+from parameterized import parameterized
 
 import rdiffweb.test
 from rdiffweb.core.model import UserObject
@@ -170,40 +171,58 @@ def test_add_edit_delete(self):
         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):
         """

rdiffweb/controller/tests/test_page_prefs_general.py

@@ -23,6 +23,7 @@
 from unittest.mock import MagicMock
 
 import cherrypy
+from parameterized import parameterized
 
 import rdiffweb.test
 from rdiffweb.core.model import RepoObject, UserObject
@@ -88,16 +89,31 @@ def test_change_username_noop(self):
         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
@@ -126,30 +142,31 @@ def test_change_email(self):
         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()

rdiffweb/core/model/_user.py

@@ -55,6 +55,7 @@ class UserObject(Base):
     __tablename__ = 'users'
     __table_args__ = {'sqlite_autoincrement': True}
 
+    # Value for role.
     ADMIN_ROLE = 0
     MAINTAINER_ROLE = 5
     USER_ROLE = 10
@@ -63,9 +64,15 @@ class UserObject(Base):
         '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="")