From 0ab3192b81c98f1ba75cc6e29bcad6f6bb7f699d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josu=C3=A9=20Tille?= Date: Wed, 22 Apr 2020 15:49:16 +0200 Subject: [PATCH] Improve regex management and simplify urls check --- src/yunohost/domain.py | 73 +++++++++++++++++++++---------- src/yunohost/permission.py | 39 +++-------------- src/yunohost/tests/test_appurl.py | 44 ++++++++++++------- 3 files changed, 83 insertions(+), 73 deletions(-) diff --git a/src/yunohost/domain.py b/src/yunohost/domain.py index 3848213d2..8ef7bb390 100644 --- a/src/yunohost/domain.py +++ b/src/yunohost/domain.py @@ -395,7 +395,7 @@ def _normalize_domain_path(domain, path): return domain, path -def _check_and_normalize_permission_path(url): +def _check_and_sanitize_permission_path(url, app_main_path, permission): """ Check and normalize the urls passed for all permissions Also check that the Regex is valid @@ -416,48 +416,77 @@ def _check_and_normalize_permission_path(url): """ import re, sre_constants - # Uri without domain - if url.startswith('re:/'): - regex = url[4:] - # check regex - try: - re.compile(regex) - except sre_constants.error: - raise YunohostError('invalid_regex', regex=regex) - return url - - if url.startswith('/'): - return '/' + url.strip("/") - # Uri with domain domains = domain_list()['domains'] + # regex without domain + if url.startswith('re:/'): + regex = url[4:] + # check regex if it's a PCRE regex, if it's a lua regex just print a warning + # I don't how how to validate a regex + if '%' in regex: + logger.warning("/!\\ Packagers! You are probably using a lua regex. You should use a PCRE regex instead.") + else: + try: + re.compile(regex) + except sre_constants.error: + raise YunohostError('invalid_regex', regex=regex) + return url + + # regex with domain if url.startswith('re:'): if '/' not in url: raise YunohostError('regex_with_only_domain') domain = url[3:].split('/')[0] path = '/' + url[3:].split('/', 1)[1] - if domain not in domains: + if domain.replace('%', '').replace('\\', '') not in domains: raise YunohostError('domain_named_unknown', domain=domain) - try: - re.compile(path) - except sre_constants.error: - raise YunohostError('invalid_regex', regex=path) + if '%' in path: + logger.warning("/!\\ Packagers! You are probably using a lua regex. You should use a PCRE regex instead.") + else: + try: + re.compile(path) + except sre_constants.error: + raise YunohostError('invalid_regex', regex=path) return 're:' + domain + path + # uris without domain + if url.startswith('/'): + sanitized_url = '/' + url.strip("/") + domain = app_main_path.split('/')[0] + path = ('/' + app_main_path.split('/')[1]) if '/' in app_main_path else '/' + + # uris with domain else: domain = url.split('/')[0] if domain not in domains: raise YunohostError('domain_named_unknown', domain=domain) if '/' in url: - path = url.split('/', 1)[1].rstrip('/') - return domain + '/' + path + path = '/' + url.split('/', 1)[1].rstrip('/') + sanitized_url = domain + path else: - return domain + sanitized_url = domain + path = '/' + + conflicts = _get_conflicting_apps(domain, path, ignore_app=permission.split('.')[0]) + + if conflicts: + apps = [] + for path, app_id, app_label in conflicts: + apps.append(" * {domain:s}{path:s} → {app_label:s} ({app_id:s})".format( + domain=domain, + path=path, + app_id=app_id, + app_label=app_label, + )) + + raise YunohostError('app_location_unavailable', apps="\n".join(apps)) + + return sanitized_url def _build_dns_conf(domain, ttl=3600): diff --git a/src/yunohost/permission.py b/src/yunohost/permission.py index 387b04858..e3d912047 100644 --- a/src/yunohost/permission.py +++ b/src/yunohost/permission.py @@ -364,7 +364,7 @@ def permission_url(operation_logger, permission, """ from yunohost.app import app_setting from yunohost.utils.ldap import _get_ldap_interface - from yunohost.domain import _check_and_normalize_permission_path, _get_conflicting_apps + from yunohost.domain import _check_and_sanitize_permission_path, _get_conflicting_apps ldap = _get_ldap_interface() # By default, manipulate main permission @@ -389,26 +389,12 @@ def permission_url(operation_logger, permission, if url is None: url = existing_permission["url"] else: - url = _check_and_normalize_permission_path(url) - domain, path = _get_full_url(url, app_main_path).split('/', 1) - domain = domain[3:] if domain.startswith("re:") else domain - conflicts = _get_conflicting_apps(domain, path, ignore_app=permission.split('.')[0]) + url = _check_and_sanitize_permission_path(url, app_main_path, permission) + if url.startswith('re:') and existing_permission['show_tile']: logger.warning(m18n.n('regex_incompatible_with_tile', regex=url, permission=permission)) show_tile = False - if conflicts: - apps = [] - for path, app_id, app_label in conflicts: - apps.append(" * {domain:s}{path:s} → {app_label:s} ({app_id:s})".format( - domain=domain, - path=path, - app_id=app_id, - app_label=app_label, - )) - - raise YunohostError('app_location_unavailable', apps="\n".join(apps)) - current_additional_urls = existing_permission["additional_urls"] new_additional_urls = copy.copy(current_additional_urls) @@ -417,22 +403,7 @@ def permission_url(operation_logger, permission, if ur in current_additional_urls: logger.warning(m18n.n('additional_urls_already_added', permission=permission, url=ur)) else: - ur = _check_and_normalize_permission_path(ur) - domain, path = _get_full_url(ur, app_main_path).split('/', 1) - domain = domain[3:] if domain.startswith("re:") else domain - conflicts = _get_conflicting_apps(domain, path, ignore_app=permission.split('.')[0]) - - if conflicts: - apps = [] - for path, app_id, app_label in conflicts: - apps.append(" * {domain:s}{path:s} → {app_label:s} ({app_id:s})".format( - domain=domain, - path=path, - app_id=app_id, - app_label=app_label, - )) - - raise YunohostError('app_location_unavailable', apps="\n".join(apps)) + ur = _check_and_sanitize_permission_path(ur, app_main_path, permission) new_additional_urls += [ur] if remove_url: @@ -657,6 +628,6 @@ def _get_full_url(url, app_main_path): if url.startswith('/'): return app_main_path + url.rstrip("/") if url.startswith('re:/'): - return 're:' + app_main_path + url[3:] + return 're:' + app_main_path.replace('.', '\\.') + url[3:] else: return url diff --git a/src/yunohost/tests/test_appurl.py b/src/yunohost/tests/test_appurl.py index 3b7af4d92..e7df12a12 100644 --- a/src/yunohost/tests/test_appurl.py +++ b/src/yunohost/tests/test_appurl.py @@ -2,7 +2,7 @@ import pytest from yunohost.utils.error import YunohostError from yunohost.app import app_install, app_remove -from yunohost.domain import _get_maindomain, domain_url_available, _normalize_domain_path, _check_and_normalize_permission_path +from yunohost.domain import _get_maindomain, domain_url_available, _normalize_domain_path, _check_and_sanitize_permission_path # Get main domain maindomain = _get_maindomain() @@ -63,40 +63,50 @@ def test_registerurl_baddomain(): def test_normalize_permission_path(): # Relative path - assert _check_and_normalize_permission_path("/wiki/") == "/wiki" - assert _check_and_normalize_permission_path("/") == "/" - assert _check_and_normalize_permission_path("//salut/") == "/salut" + assert _check_and_sanitize_permission_path("/wiki/", maindomain + '/path', 'test_permission.main') == "/wiki" + assert _check_and_sanitize_permission_path("/", maindomain + '/path', 'test_permission.main') == "/" + assert _check_and_sanitize_permission_path("//salut/", maindomain + '/path', 'test_permission.main') == "/salut" # Full path - assert _check_and_normalize_permission_path(maindomain + "/hey/") == maindomain + "/hey" - assert _check_and_normalize_permission_path(maindomain + "//") == maindomain + "/" - assert _check_and_normalize_permission_path(maindomain + "/") == maindomain + "/" + assert _check_and_sanitize_permission_path(maindomain + "/hey/", maindomain + '/path', 'test_permission.main') == maindomain + "/hey" + assert _check_and_sanitize_permission_path(maindomain + "//", maindomain + '/path', 'test_permission.main') == maindomain + "/" + assert _check_and_sanitize_permission_path(maindomain + "/", maindomain + '/path', 'test_permission.main') == maindomain + "/" # Relative Regex - assert _check_and_normalize_permission_path("re:/yolo.*/") == "re:/yolo.*/" - assert _check_and_normalize_permission_path("re:/y.*o(o+)[a-z]*/bo\1y") == "re:/y.*o(o+)[a-z]*/bo\1y" + assert _check_and_sanitize_permission_path("re:/yolo.*/", maindomain + '/path', 'test_permission.main') == "re:/yolo.*/" + assert _check_and_sanitize_permission_path("re:/y.*o(o+)[a-z]*/bo\1y", maindomain + '/path', 'test_permission.main') == "re:/y.*o(o+)[a-z]*/bo\1y" # Full Regex - assert _check_and_normalize_permission_path("re:" + maindomain + "/yolo.*/") == "re:" + maindomain + "/yolo.*/" - assert _check_and_normalize_permission_path("re:" + maindomain + "/y.*o(o+)[a-z]*/bo\1y") == "re:" + maindomain + "/y.*o(o+)[a-z]*/bo\1y" + assert _check_and_sanitize_permission_path("re:" + maindomain + "/yolo.*/", maindomain + '/path', 'test_permission.main') == "re:" + maindomain + "/yolo.*/" + assert _check_and_sanitize_permission_path("re:" + maindomain + "/y.*o(o+)[a-z]*/bo\1y", maindomain + '/path', 'test_permission.main') == "re:" + maindomain + "/y.*o(o+)[a-z]*/bo\1y" def test_normalize_permission_path_with_bad_regex(): # Relative Regex with pytest.raises(YunohostError): - _check_and_normalize_permission_path("re:/yolo.*[1-7]^?/") + _check_and_sanitize_permission_path("re:/yolo.*[1-7]^?/", maindomain + '/path', 'test_permission.main') with pytest.raises(YunohostError): - _check_and_normalize_permission_path("re:/yolo.*[1-7](]/") + _check_and_sanitize_permission_path("re:/yolo.*[1-7](]/", maindomain + '/path', 'test_permission.main') # Full Regex with pytest.raises(YunohostError): - _check_and_normalize_permission_path("re:" + maindomain + "/yolo?+/") + _check_and_sanitize_permission_path("re:" + maindomain + "/yolo?+/", maindomain + '/path', 'test_permission.main') with pytest.raises(YunohostError): - _check_and_normalize_permission_path("re:" + maindomain + "/yolo[1-9]**/") + _check_and_sanitize_permission_path("re:" + maindomain + "/yolo[1-9]**/", maindomain + '/path', 'test_permission.main') def test_normalize_permission_path_with_unknown_domain(): with pytest.raises(YunohostError): - _check_and_normalize_permission_path("shouldntexist.tld/hey") + _check_and_sanitize_permission_path("shouldntexist.tld/hey", maindomain + '/path', 'test_permission.main') with pytest.raises(YunohostError): - _check_and_normalize_permission_path("re:shouldntexist.tld/hey.*") + _check_and_sanitize_permission_path("re:shouldntexist.tld/hey.*", maindomain + '/path', 'test_permission.main') + + +def test_normalize_permission_path_conflicting_path(): + app_install("./tests/apps/register_url_app_ynh", + args="domain=%s&path=%s" % (maindomain, "/url/registerapp"), force=True) + + with pytest.raises(YunohostError): + _check_and_sanitize_permission_path("/registerapp", maindomain + '/url', 'test_permission.main') + with pytest.raises(YunohostError): + _check_and_sanitize_permission_path(maindomain + "/url/registerapp", maindomain + '/path', 'test_permission.main')