validate proxy.extra_routes

- add trailing slash if missing, and warn
- raise if leading slash is wrong (must not be present with host routing, must be present otherwise)

examples/server-api/jupyterhub_config.py

@@ -2,6 +2,8 @@
 # 1. start/stop servers, and
 # 2. access the server API
 
+c = get_config()  # noqa
+
 c.JupyterHub.load_roles = [
     {
         "name": "launcher",

jupyterhub/app.py

@@ -711,11 +711,14 @@ class JupyterHub(Application):
         """,
     ).tag(config=True)
 
-    def _subdomain_host_changed(self, name, old, new):
+    @validate("subdomain_host")
+    def _validate_subdomain_host(self, proposal):
+        new = proposal.value
         if new and '://' not in new:
             # host should include '://'
             # if not specified, assume https: You have to be really explicit about HTTP!
-            self.subdomain_host = 'https://' + new
+            new = 'https://' + new
+        return new
 
     domain = Unicode(help="domain name, e.g. 'example.com' (excludes protocol, port)")
 

jupyterhub/proxy.py

@@ -23,12 +23,23 @@ import signal
 import time
 from functools import wraps
 from subprocess import Popen
-from urllib.parse import quote
+from urllib.parse import quote, urlparse
 from weakref import WeakKeyDictionary
 
 from tornado.httpclient import AsyncHTTPClient, HTTPError, HTTPRequest
 from tornado.ioloop import PeriodicCallback
-from traitlets import Any, Bool, Dict, Instance, Integer, Unicode, default, observe
+from traitlets import (
+    Any,
+    Bool,
+    Dict,
+    Instance,
+    Integer,
+    TraitError,
+    Unicode,
+    default,
+    observe,
+    validate,
+)
 from traitlets.config import LoggingConfigurable
 
 from jupyterhub.traitlets import Command
@@ -111,7 +122,8 @@ class Proxy(LoggingConfigurable):
     )
 
     extra_routes = Dict(
-        {},
+        key_trait=Unicode(),
+        value_trait=Unicode(),
         config=True,
         help="""
         Additional routes to be maintained in the proxy.
@@ -130,6 +142,51 @@ class Proxy(LoggingConfigurable):
         """,
     )
 
+    @validate("extra_routes")
+    def _validate_extra_routes(self, proposal):
+        extra_routes = {}
+        # check routespecs for leading/trailing slashes
+        for routespec, target in proposal.value.items():
+            if not isinstance(routespec, str):
+                raise TraitError(
+                    f"Proxy.extra_routes keys must be str, got {routespec!r}"
+                )
+            if not isinstance(target, str):
+                raise TraitError(
+                    f"Proxy.extra_routes values must be str, got {target!r}"
+                )
+            if not routespec.endswith("/"):
+                # trailing / is unambiguous, so we can add it
+                self.log.warning(
+                    f"Adding missing trailing '/' to c.Proxy.extra_routes {routespec} -> {routespec}/"
+                )
+                routespec += "/"
+
+            if self.app.subdomain_host:
+                # subdomain routing must _not_ start with /
+                if routespec.startswith("/"):
+                    raise ValueError(
+                        f"Proxy.extra_routes missing host component in {routespec} (must not have leading '/') when using `JupyterHub.subdomain_host = {self.app.subdomain_host!r}`"
+                    )
+
+            else:
+                # no subdomains, must start with /
+                # this is ambiguous with host routing, so raise instead of warn
+                if not routespec.startswith("/"):
+                    raise ValueError(
+                        f"Proxy.extra_routes routespec {routespec} missing leading '/'."
+                    )
+
+            # validate target URL?
+            target_url = urlparse(target.lower())
+            if target_url.scheme not in {"http", "https"} or not target_url.netloc:
+                raise ValueError(
+                    f"Proxy.extra_routes target {routespec}={target!r} doesn't look like a URL (should have http[s]://...)"
+                )
+            extra_routes[routespec] = target
+
+        return extra_routes
+
     def start(self):
         """Start the proxy.
 

jupyterhub/tests/test_proxy.py

@@ -6,6 +6,7 @@ from subprocess import Popen
 from urllib.parse import quote, urlparse
 
 import pytest
+from traitlets import TraitError
 from traitlets.config import Config
 
 from ..utils import url_path_join as ujoin
@@ -193,23 +194,96 @@ async def test_check_routes(app, username, disable_check_routes):
     assert before == after
 
 
-async def test_extra_routes(app):
+@pytest.mark.parametrize(
+    "routespec",
+    [
+        '/has%20space/foo/',
+        '/missing-trailing/slash',
+        '/has/@/',
+        '/has/' + quote('üñîçø∂é'),
+        'host.name/path/',
+        'other.host/path/no/slash',
+    ],
+)
+async def test_extra_routes(app, routespec):
     proxy = app.proxy
     # When using host_routing, it's up to the admin to
     # provide routespecs that have a domain in them.
     # We don't explicitly validate that here.
-    if app.subdomain_host:
-        route_spec = 'example.com/test-extra-routes/'
-    else:
-        route_spec = '/test-extra-routes/'
+    if app.subdomain_host and routespec.startswith("/"):
+        routespec = 'example.com/' + routespec
+    elif not app.subdomain_host and not routespec.startswith("/"):
+        pytest.skip("requires subdomains")
+    validated_routespec = routespec
+    if not routespec.endswith("/"):
+        validated_routespec = routespec + "/"
     target = 'http://localhost:9999/test'
-    proxy.extra_routes = {route_spec: target}
+    proxy.extra_routes = {routespec: target}
 
     await proxy.check_routes(app.users, app._service_map)
 
     routes = await app.proxy.get_all_routes()
-    assert route_spec in routes
-    assert routes[route_spec]['target'] == target
+    print(routes)
+    assert validated_routespec in routes
+    assert routes[validated_routespec]['target'] == target
+    assert routes[validated_routespec]['data']['extra']
+
+
+@pytest.mark.parametrize(
+    "needs_subdomain, routespec, expected",
+    [
+        (False, "/prefix/", "/prefix/"),
+        (False, "/prefix", "/prefix/"),
+        (False, "prefix/", ValueError),
+        (True, "/prefix/", ValueError),
+        (True, "example.com/prefix/", "example.com/prefix/"),
+        (True, "example.com/prefix", "example.com/prefix/"),
+        (False, 100, TraitError),
+    ],
+)
+def test_extra_routes_validate_routespec(
+    request, app, needs_subdomain, routespec, expected
+):
+    save_host = app.subdomain_host
+    request.addfinalizer(lambda: setattr(app, "subdomain_host", save_host))
+    if needs_subdomain:
+        app.subdomain_host = "localhost.jovyan.org"
+    else:
+        app.subdomain_host = ""
+
+    proxy = app.proxy
+
+    extra_routes = {routespec: "https://127.0.0.1"}
+    if isinstance(expected, type) and issubclass(expected, BaseException):
+        with pytest.raises(expected):
+            proxy.extra_routes = extra_routes
+        return
+    proxy.extra_routes = extra_routes
+    assert list(proxy.extra_routes) == [expected]
+
+
+@pytest.mark.parametrize(
+    "target, expected",
+    [
+        ("http://host", "http://host"),
+        ("https://host", "https://host"),
+        ("/missing-host", ValueError),
+        ("://missing-scheme", ValueError),
+        (100, TraitError),
+    ],
+)
+def test_extra_routes_validate_target(app, target, expected):
+    proxy = app.proxy
+    routespec = "/prefix/"
+    if app.subdomain_host:
+        routespec = f"host.tld{routespec}"
+    extra_routes = {routespec: target}
+    if isinstance(expected, type) and issubclass(expected, BaseException):
+        with pytest.raises(expected):
+            proxy.extra_routes = extra_routes
+        return
+    proxy.extra_routes = extra_routes
+    assert list(proxy.extra_routes.values()) == [expected]
 
 
 @pytest.mark.parametrize(