From cc3fbdd6b5081a90b45dd782732777c403cf5fa9 Mon Sep 17 00:00:00 2001 From: Dannon Baker Date: Fri, 29 Jan 2021 09:08:55 -0500 Subject: [PATCH 1/5] bugfix: username matching in routes -- use same RE as for validation. --- lib/galaxy/webapps/galaxy/buildapp.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/lib/galaxy/webapps/galaxy/buildapp.py b/lib/galaxy/webapps/galaxy/buildapp.py index 6e2e75d1ade..be16db58a59 100644 --- a/lib/galaxy/webapps/galaxy/buildapp.py +++ b/lib/galaxy/webapps/galaxy/buildapp.py @@ -16,6 +16,7 @@ import galaxy.model.mapping import galaxy.web.framework import galaxy.webapps.base.webapp from galaxy import util +from galaxy.security.validate_user_input import VALID_PUBLICNAME_RE from galaxy.util import asbool from galaxy.util.properties import load_app_properties from galaxy.web.framework.middleware.batch import BatchMiddleware @@ -90,12 +91,14 @@ def app_factory(global_conf, load_app_kwds=None, **kwargs): webapp.add_route('/display_application/{dataset_id}/{app_name}/{link_name}/{user_id}/{app_action}/{action_param}/{action_param_extra:.+?}', controller='dataset', action='display_application', dataset_id=None, user_id=None, app_name=None, link_name=None, app_action=None, action_param=None, action_param_extra=None) - webapp.add_route('/u/{username}/d/{slug}/{filename}', controller='dataset', action='display_by_username_and_slug', filename=None) - webapp.add_route('/u/{username}/p/{slug}', controller='page', action='display_by_username_and_slug') - webapp.add_route('/u/{username}/h/{slug}', controller='history', action='display_by_username_and_slug') - webapp.add_route('/u/{username}/w/{slug}', controller='workflow', action='display_by_username_and_slug') - webapp.add_route('/u/{username}/w/{slug}/{format}', controller='workflow', action='display_by_username_and_slug') - webapp.add_route('/u/{username}/v/{slug}', controller='visualization', action='display_by_username_and_slug') + + USERNAME_REQS = {'username': VALID_PUBLICNAME_RE.pattern.strip("^$")} # Strip start/end from compiled RE pattern + webapp.add_route('/u/{username}/d/{slug}/{filename}', controller='dataset', action='display_by_username_and_slug', filename=None, requirements=USERNAME_REQS) + webapp.add_route('/u/{username}/p/{slug}', controller='page', action='display_by_username_and_slug', requirements=USERNAME_REQS) + webapp.add_route('/u/{username}/h/{slug}', controller='history', action='display_by_username_and_slug', requirements=USERNAME_REQS) + webapp.add_route('/u/{username}/w/{slug}', controller='workflow', action='display_by_username_and_slug', requirements=USERNAME_REQS) + webapp.add_route('/u/{username}/w/{slug}/{format}', controller='workflow', action='display_by_username_and_slug', requirements=USERNAME_REQS) + webapp.add_route('/u/{username}/v/{slug}', controller='visualization', action='display_by_username_and_slug', requirements=USERNAME_REQS) # TODO: Refactor above routes into external method to allow testing in # isolation as well. From 7f5b54a6511f9b67293ed6b67c90ce713c903478 Mon Sep 17 00:00:00 2001 From: Dannon Baker Date: Fri, 29 Jan 2021 09:18:11 -0500 Subject: [PATCH 2/5] Use substitution regex that I didn't notice before instead of the extra stripping --- lib/galaxy/webapps/galaxy/buildapp.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/webapps/galaxy/buildapp.py b/lib/galaxy/webapps/galaxy/buildapp.py index be16db58a59..be1479f2c20 100644 --- a/lib/galaxy/webapps/galaxy/buildapp.py +++ b/lib/galaxy/webapps/galaxy/buildapp.py @@ -16,7 +16,7 @@ import galaxy.model.mapping import galaxy.web.framework import galaxy.webapps.base.webapp from galaxy import util -from galaxy.security.validate_user_input import VALID_PUBLICNAME_RE +from galaxy.security.validate_user_input import VALID_PUBLICNAME_SUB from galaxy.util import asbool from galaxy.util.properties import load_app_properties from galaxy.web.framework.middleware.batch import BatchMiddleware @@ -92,7 +92,7 @@ def app_factory(global_conf, load_app_kwds=None, **kwargs): controller='dataset', action='display_application', dataset_id=None, user_id=None, app_name=None, link_name=None, app_action=None, action_param=None, action_param_extra=None) - USERNAME_REQS = {'username': VALID_PUBLICNAME_RE.pattern.strip("^$")} # Strip start/end from compiled RE pattern + USERNAME_REQS = {'username': VALID_PUBLICNAME_SUB.pattern} webapp.add_route('/u/{username}/d/{slug}/{filename}', controller='dataset', action='display_by_username_and_slug', filename=None, requirements=USERNAME_REQS) webapp.add_route('/u/{username}/p/{slug}', controller='page', action='display_by_username_and_slug', requirements=USERNAME_REQS) webapp.add_route('/u/{username}/h/{slug}', controller='history', action='display_by_username_and_slug', requirements=USERNAME_REQS) From 0b84678f43d3333236467fcdb0dbb58b5facdf60 Mon Sep 17 00:00:00 2001 From: Dannon Baker Date: Fri, 29 Jan 2021 12:02:53 -0500 Subject: [PATCH 3/5] Add a couple basic tests for username/email validation --- .../data/security/test_validate_user_input.py | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/test/unit/data/security/test_validate_user_input.py b/test/unit/data/security/test_validate_user_input.py index 63f9b829625..3800357494a 100644 --- a/test/unit/data/security/test_validate_user_input.py +++ b/test/unit/data/security/test_validate_user_input.py @@ -1,4 +1,8 @@ -from galaxy.security.validate_user_input import extract_domain +from galaxy.security.validate_user_input import ( + extract_domain, + validate_email_str, + validate_publicname_str +) def test_extract_full_domain(): @@ -10,3 +14,20 @@ def test_extract_base_domain(): # Use case: ignore subdomains to filter out disposable email addresses assert extract_domain('jack@foo.com', base_only=True) == 'foo.com' assert extract_domain('jack@foo.bar.com', base_only=True) == 'bar.com' + + +def test_validate_username(): + assert validate_publicname_str('testuser') == '' + assert validate_publicname_str('test.user') == '' + assert validate_publicname_str('test-user') == '' + assert validate_publicname_str('test@user') != '' + assert validate_publicname_str('test user') != '' + + +def test_validate_email(): + assert validate_email_str('test@foo.com') == '' + assert validate_email_str('test-dot.user@foo.com') == '' + assert validate_email_str('test@com') != '' + assert validate_email_str('@not-a-domain') != '' + too_long_email = "N" * 255 + "@foo.com" + assert validate_email_str(too_long_email) != '' From 6c155bf2ef7a9ad037537513fad1a813cb0c8f65 Mon Sep 17 00:00:00 2001 From: Dannon Baker Date: Fri, 29 Jan 2021 12:05:41 -0500 Subject: [PATCH 4/5] Standardize test format --- .../data/security/test_validate_user_input.py | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/test/unit/data/security/test_validate_user_input.py b/test/unit/data/security/test_validate_user_input.py index 3800357494a..e25be2a52cb 100644 --- a/test/unit/data/security/test_validate_user_input.py +++ b/test/unit/data/security/test_validate_user_input.py @@ -1,33 +1,33 @@ from galaxy.security.validate_user_input import ( extract_domain, validate_email_str, - validate_publicname_str + validate_publicname_str, ) def test_extract_full_domain(): - assert extract_domain('jack@foo.com') == 'foo.com' - assert extract_domain('jack@foo.bar.com') == 'foo.bar.com' + assert extract_domain("jack@foo.com") == "foo.com" + assert extract_domain("jack@foo.bar.com") == "foo.bar.com" def test_extract_base_domain(): # Use case: ignore subdomains to filter out disposable email addresses - assert extract_domain('jack@foo.com', base_only=True) == 'foo.com' - assert extract_domain('jack@foo.bar.com', base_only=True) == 'bar.com' + assert extract_domain("jack@foo.com", base_only=True) == "foo.com" + assert extract_domain("jack@foo.bar.com", base_only=True) == "bar.com" def test_validate_username(): - assert validate_publicname_str('testuser') == '' - assert validate_publicname_str('test.user') == '' - assert validate_publicname_str('test-user') == '' - assert validate_publicname_str('test@user') != '' - assert validate_publicname_str('test user') != '' + assert validate_publicname_str("testuser") == "" + assert validate_publicname_str("test.user") == "" + assert validate_publicname_str("test-user") == "" + assert validate_publicname_str("test@user") != "" + assert validate_publicname_str("test user") != "" def test_validate_email(): - assert validate_email_str('test@foo.com') == '' - assert validate_email_str('test-dot.user@foo.com') == '' - assert validate_email_str('test@com') != '' - assert validate_email_str('@not-a-domain') != '' + assert validate_email_str("test@foo.com") == "" + assert validate_email_str("test-dot.user@foo.com") == "" + assert validate_email_str("test@com") != "" + assert validate_email_str("@not-a-domain") != "" too_long_email = "N" * 255 + "@foo.com" - assert validate_email_str(too_long_email) != '' + assert validate_email_str(too_long_email) != "" From 329cee9fc5a2614d565c7a00f8c6152634b76c17 Mon Sep 17 00:00:00 2001 From: Dannon Baker Date: Fri, 29 Jan 2021 12:06:36 -0500 Subject: [PATCH 5/5] Drop local methods (which diverged) for checking email, username from users api --- lib/galaxy/webapps/galaxy/api/users.py | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/lib/galaxy/webapps/galaxy/api/users.py b/lib/galaxy/webapps/galaxy/api/users.py index c48381ffbdd..8dd4a781cd4 100644 --- a/lib/galaxy/webapps/galaxy/api/users.py +++ b/lib/galaxy/webapps/galaxy/api/users.py @@ -416,7 +416,7 @@ class UserAPIController(BaseAPIController, UsesTagsMixin, BaseUIController, Uses # Update email if 'email' in payload: email = payload.get('email') - message = self._validate_email(email) or validate_email(trans, email, user) + message = validate_email(trans, email, user) if message: raise exceptions.RequestParameterInvalidException(message) if user.email != email: @@ -441,7 +441,7 @@ class UserAPIController(BaseAPIController, UsesTagsMixin, BaseUIController, Uses # Update public name if 'username' in payload: username = payload.get('username') - message = self._validate_publicname(username) or validate_publicname(trans, username, user) + message = validate_publicname(trans, username, user) if message: raise exceptions.RequestParameterInvalidException(message) if user.username != username: @@ -581,19 +581,6 @@ class UserAPIController(BaseAPIController, UsesTagsMixin, BaseUIController, Uses else: raise exceptions.ObjectAttributeInvalidException("This type is not supported. Given object_type: %s" % object_type) - def _validate_email(self, email): - ''' Validate email and username using regex ''' - if email == '' or not isinstance(email, str): - return 'Please provide your email address.' - if not re.match(r'^(([^<>()[\]\.,;:\s@"]+(\.[^<>()[\]\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$', email): - return 'Please provide your valid email address.' - if len(email) > 255: - return 'Email cannot be more than 255 characters in length.' - - def _validate_publicname(self, username): - if not re.match(r'^[a-z0-9\-]{3,255}$', username): - return 'Public name must contain only lowercase letters, numbers and "-". It also has to be shorter than 255 characters but longer than 2.' - @expose_api def get_password(self, trans, id, payload=None, **kwd): """