diff --git a/examples/server-api/jupyterhub_config.py b/examples/server-api/jupyterhub_config.py index 110b6d10..8a0af9dd 100644 --- a/examples/server-api/jupyterhub_config.py +++ b/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", diff --git a/jupyterhub/app.py b/jupyterhub/app.py index 0d672533..74260101 100644 --- a/jupyterhub/app.py +++ b/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)") diff --git a/jupyterhub/proxy.py b/jupyterhub/proxy.py index 8d01debd..a70f6018 100644 --- a/jupyterhub/proxy.py +++ b/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. diff --git a/jupyterhub/tests/test_proxy.py b/jupyterhub/tests/test_proxy.py index a9caa3ea..d4e6b247 100644 --- a/jupyterhub/tests/test_proxy.py +++ b/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(