Improve regex management and simplify urls check

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:
+        if '%' in path:
-            re.compile(path)
+            logger.warning("/!\\ Packagers! You are probably using a lua regex. You should use a PCRE regex instead.")
-        except sre_constants.error:
+        else:
-            raise YunohostError('invalid_regex', regex=path)
+            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('/')
+            path = '/' + url.split('/', 1)[1].rstrip('/')
-            return domain + '/' + path
+            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):

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)
+        url = _check_and_sanitize_permission_path(url, app_main_path, permission)
-        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])
         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)
+                ur = _check_and_sanitize_permission_path(ur, app_main_path, permission)
-                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))
                 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

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_sanitize_permission_path("/wiki/", maindomain + '/path', 'test_permission.main') == "/wiki"
-    assert _check_and_normalize_permission_path("/") == "/"
+    assert _check_and_sanitize_permission_path("/", maindomain + '/path', 'test_permission.main') == "/"
-    assert _check_and_normalize_permission_path("//salut/") == "/salut"
+    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_sanitize_permission_path(maindomain + "/hey/", maindomain + '/path', 'test_permission.main') == maindomain + "/hey"
-    assert _check_and_normalize_permission_path(maindomain + "//") == maindomain + "/"
+    assert _check_and_sanitize_permission_path(maindomain + "//", maindomain + '/path', 'test_permission.main') == maindomain + "/"
-    assert _check_and_normalize_permission_path(maindomain + "/") == 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_sanitize_permission_path("re:/yolo.*/", maindomain + '/path', 'test_permission.main') == "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:/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_sanitize_permission_path("re:" + maindomain + "/yolo.*/", maindomain + '/path', 'test_permission.main') == "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 + "/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')