Repository: taskcluster/tc-admin (language: python)

Issue:
Strip `/` from rootUrl, be resilient to its presence
The most recent versions are susceptible to errors when a rootUrl has a `/` at the end.  Technically rootUrls should not have a trailing `/`, but in practice we should be resilient to the presence of `/`.


Discussion / hints:

Fix patch:
diff --git a/tcadmin/util/root_url.py b/tcadmin/util/root_url.py
index 11dca162..efa13e5b 100644
--- a/tcadmin/util/root_url.py
+++ b/tcadmin/util/root_url.py
@@ -5,12 +5,21 @@
 # obtain one at http://mozilla.org/MPL/2.0/.
 
 import os
+import re
 import click
 
 from ..appconfig import AppConfig
 
 
 _root_url = None
+_url_re = re.compile("^https?://[a-z0-9.-]+/?$")
+
+
+def _normalize(root_url):
+    """Normalize a rootUrl, stripping trailing `/` and checking its format."""
+    if not _url_re.match(root_url):
+        raise click.UsageError("Given root URL {root_url} is not a valid root URL")
+    return root_url.rstrip('/')
 
 
 async def root_url():
@@ -27,9 +36,9 @@ async def root_url():
         if "TASKCLUSTER_ROOT_URL" in os.environ:
             if os.environ["TASKCLUSTER_ROOT_URL"] != root_url:
                 raise click.UsageError(f"TASKCLUSTER_ROOT_URL does not match {root_url}")
-        _root_url = root_url
+        _root_url = _normalize(root_url)
     else:
         if "TASKCLUSTER_ROOT_URL" not in os.environ:
             raise click.UsageError("TASKCLUSTER_ROOT_URL must be set")
-        _root_url = os.environ["TASKCLUSTER_ROOT_URL"]
+        _root_url = _normalize(os.environ["TASKCLUSTER_ROOT_URL"])
     return _root_url


Test patch:
diff --git a/tcadmin/tests/test_util_root_url.py b/tcadmin/tests/test_util_root_url.py
index 89a2a0b3..cd5c62e2 100644
--- a/tcadmin/tests/test_util_root_url.py
+++ b/tcadmin/tests/test_util_root_url.py
@@ -23,6 +23,12 @@ async def test_root_url_from_env(appconfig, root_url, monkeypatch):
     assert await root_url() == "https://tc-testing.example.com"
 
 
+@pytest.mark.asyncio
+async def test_root_url_from_env_trailing_slash(appconfig, root_url, monkeypatch):
+    monkeypatch.setenv("TASKCLUSTER_ROOT_URL", "https://tc-testing.example.com/")
+    assert await root_url() == "https://tc-testing.example.com"  # slash not present
+
+
 @pytest.mark.asyncio
 async def test_root_url_not_set(appconfig, root_url, monkeypatch):
     monkeypatch.delenv("TASKCLUSTER_ROOT_URL", raising=False)
@@ -37,6 +43,13 @@ async def test_root_url_from_appconfig_str(appconfig, root_url, monkeypatch):
     assert await root_url() == "https://tc-testing.example.com"
 
 
+@pytest.mark.asyncio
+async def test_root_url_from_appconfig_str_trailing_slash(appconfig, root_url, monkeypatch):
+    monkeypatch.delenv("TASKCLUSTER_ROOT_URL", raising=False)
+    appconfig.root_url = "https://tc-testing.example.com/"
+    assert await root_url() == "https://tc-testing.example.com"  # slash not present
+
+
 @pytest.mark.asyncio
 async def test_root_url_from_appconfig_fn(appconfig, root_url, monkeypatch):
     monkeypatch.delenv("TASKCLUSTER_ROOT_URL", raising=False)
@@ -54,3 +67,10 @@ async def test_root_url_from_appconfig_diferent(appconfig, root_url, monkeypatch
     appconfig.root_url = "https://something-else.example.com"
     with pytest.raises(click.UsageError):
         await root_url()
+
+
+@pytest.mark.asyncio
+async def test_root_url_invalid(root_url, monkeypatch):
+    monkeypatch.setenv("TASKCLUSTER_ROOT_URL", "https://tc-testing.example.com/some/subpath")
+    with pytest.raises(click.UsageError):
+        await root_url()