Repository: jupyterhub/jupyterhub
Pull request: Escape named server name
This escapes the server name when used in paths, but keeps the original server name in the database.

Closes https://github.com/jupyterhub/jupyterhub/issues/3144

An alternative to this implementation is to require that server names are url-safe and error if they're not, i.e. the client UI would be responsible for converting the name submitted by a server to it's URL safe form before POSTing it to JupyterHub. However this leads to more complications since if the client encodes the name before submission (e.g. in javascript), then it'll be encoded a second time when it's sent in the URL request, and the JupyterHub database will always store an encoded name instead of the original.

- [x] This works in manual testing, ~but needs an automated test~ I've added some tests, though I couldn't work out how to trigger the `'/' in server_name` error in a test. I can trigger it in the browser, but if I add to the existing test it's accepted- is there a Mock getting in the way somewhere?

Problem statement:
Named Server names are not URL escaped
### Bug description
<!-- Use this section to clearly and concisely describe the bug. -->
A user created a named server with a name that contains a whitespace, this caused a malformed URL and subsequently crashed jupyterhub when it runs add_route()

#### Expected behaviour
<!-- Tell us what you thought would happen. -->
JupyterHub is able to communicated with all spawned JupyterServer


#### Actual behaviour
<!-- Tell us what actually happens. -->
Already started Jupyter servers remain running, but JupyterHub failed to restart. 

### How to reproduce
<!-- Use this section to describe the steps that a user would take to experience this bug. -->

1. Launch a Jupyter server as normal. 
2. Launch an additional named server with white space in name. 

### Your personal set up
<!-- Tell us a little about the system you're using. -->

 - OS: CentOS 8.2
 <!-- [e.g. linux, OSX] -->
 - Version:
 <!-- e.g. jupyterhub --version. --->
 - Configuration:
 <!-- Be careful not to share any sensitive information. --->

Hints from issue discussion:
Thank you for opening your first issue in this project! Engagement like this is essential for open source projects! :hugs:
<br>If you haven't done so already, check out [Jupyter's Code of Conduct](https://github.com/jupyter/governance/blob/master/conduct/code_of_conduct.md). Also, please try to follow the issue template as it helps other other community members to contribute more effectively.
![welcome](https://raw.githubusercontent.com/jupyterhub/.github/master/images/welcome.jpg)
You can meet the other [Jovyans](https://jupyter.readthedocs.io/en/latest/community/content-community.html?highlight=jovyan#what-is-a-jovyan) by joining our [Discourse forum](http://discourse.jupyter.org/). There is also an intro thread there where you can stop by and say Hi! :wave:
<br>Welcome to the Jupyter community! :tada:

Thanks for @linhbngo for reporting this. I can reproduce this issue!

In our case the bug appeared when a user created a named server with a space in its name.
Interestingly, the bug did not cause a hard crash of the hub as long it was running.

Only when restarted, and the hub syncs its routes with the CHP, it became stuck in a crash-loop.
This is still happening and very easy to reproduce.

Commit message: Ban `/` from server-name

Patch to review:
diff --git a/jupyterhub/handlers/pages.py b/jupyterhub/handlers/pages.py
index b7aad49cb9..b154b0d8db 100644
--- a/jupyterhub/handlers/pages.py
+++ b/jupyterhub/handlers/pages.py
@@ -118,6 +118,9 @@ def get(self, user_name=None, server_name=''):
 
     @needs_scope("servers")
     async def _get(self, user_name, server_name):
+        if '/' in server_name:
+            raise web.HTTPError(400, f"Invalid server_name: {server_name}")
+
         for_user = user_name
 
         user = current_user = self.current_user
diff --git a/jupyterhub/proxy.py b/jupyterhub/proxy.py
index f09bda83ef..8d01debd0c 100644
--- a/jupyterhub/proxy.py
+++ b/jupyterhub/proxy.py
@@ -36,7 +36,7 @@
 from . import utils
 from .metrics import CHECK_ROUTES_DURATION_SECONDS, PROXY_POLL_DURATION_SECONDS
 from .objects import Server
-from .utils import AnyTimeoutError, exponential_backoff, url_path_join
+from .utils import AnyTimeoutError, exponential_backoff, url_escape_path, url_path_join
 
 
 def _one_at_a_time(method):
@@ -295,7 +295,9 @@ async def delete_user(self, user, server_name=''):
         """Remove a user's server from the proxy table."""
         routespec = user.proxy_spec
         if server_name:
-            routespec = url_path_join(user.proxy_spec, server_name, '/')
+            routespec = url_path_join(
+                user.proxy_spec, url_escape_path(server_name), '/'
+            )
         self.log.info("Removing user %s from proxy (%s)", user.name, routespec)
         await self.delete_route(routespec)
 
diff --git a/jupyterhub/spawner.py b/jupyterhub/spawner.py
index 76f9ad7798..0e1a5f6424 100644
--- a/jupyterhub/spawner.py
+++ b/jupyterhub/spawner.py
@@ -43,6 +43,7 @@
     exponential_backoff,
     maybe_future,
     random_port,
+    url_escape_path,
     url_path_join,
 )
 
@@ -871,7 +872,7 @@ def get_env(self):
             env['JUPYTERHUB_COOKIE_OPTIONS'] = json.dumps(self.cookie_options)
         env['JUPYTERHUB_HOST'] = self.hub.public_host
         env['JUPYTERHUB_OAUTH_CALLBACK_URL'] = url_path_join(
-            self.user.url, self.name, 'oauth_callback'
+            self.user.url, url_escape_path(self.name), 'oauth_callback'
         )
 
         env['JUPYTERHUB_OAUTH_SCOPES'] = json.dumps(self.oauth_scopes)
diff --git a/jupyterhub/tests/test_named_servers.py b/jupyterhub/tests/test_named_servers.py
index 70564fc1a1..3dbabbf7db 100644
--- a/jupyterhub/tests/test_named_servers.py
+++ b/jupyterhub/tests/test_named_servers.py
@@ -2,12 +2,12 @@
 import asyncio
 import json
 from unittest import mock
-from urllib.parse import urlencode, urlparse
+from urllib.parse import unquote, urlencode, urlparse
 
 import pytest
 from tornado.httputil import url_concat
 
-from ..utils import url_path_join
+from ..utils import url_escape_path, url_path_join
 from .mocking import FormSpawner, public_url
 from .test_api import TIMESTAMP, add_user, api_request, fill_user, normalize_user
 from .utils import async_requests, get_page
@@ -83,25 +83,44 @@ async def test_default_server(app, named_servers):
     )
 
 
-async def test_create_named_server(app, named_servers):
+@pytest.mark.parametrize(
+    'servername,escapedname,caller_escape',
+    [
+        ('trevor', 'trevor', False),
+        ('$p~c|a! ch@rs', '%24p~c%7Ca%21%20ch@rs', False),
+        ('$p~c|a! ch@rs', '%24p~c%7Ca%21%20ch@rs', True),
+    ],
+)
+async def test_create_named_server(
+    app, named_servers, servername, escapedname, caller_escape
+):
     username = 'walnut'
     user = add_user(app.db, app, name=username)
     # assert user.allow_named_servers == True
     cookies = await app.login_user(username)
-    servername = 'trevor'
-    r = await api_request(app, 'users', username, 'servers', servername, method='post')
+    request_servername = servername
+    if caller_escape:
+        request_servername = url_escape_path(servername)
+
+    r = await api_request(
+        app, 'users', username, 'servers', request_servername, method='post'
+    )
     r.raise_for_status()
     assert r.status_code == 201
     assert r.text == ''
 
     url = url_path_join(public_url(app, user), servername, 'env')
+    expected_url = url_path_join(public_url(app, user), escapedname, 'env')
     r = await async_requests.get(url, cookies=cookies)
     r.raise_for_status()
-    assert r.url == url
+    # requests doesn't fully encode the servername: "$p~c%7Ca!%20ch@rs".
+    # Since this is the internal requests representation and not the JupyterHub
+    # representation it just needs to be equivalent.
+    assert unquote(r.url) == unquote(expected_url)
     env = r.json()
     prefix = env.get('JUPYTERHUB_SERVICE_PREFIX')
     assert prefix == user.spawners[servername].server.base_url
-    assert prefix.endswith(f'/user/{username}/{servername}/')
+    assert prefix.endswith(f'/user/{username}/{escapedname}/')
 
     r = await api_request(app, 'users', username)
     r.raise_for_status()
@@ -121,7 +140,7 @@ async def test_create_named_server(app, named_servers):
                     'pending': None,
                     'ready': True,
                     'progress_url': 'PREFIX/hub/api/users/{}/servers/{}/progress'.format(
-                        username, servername
+                        username, escapedname
                     ),
                     'state': {'pid': 0},
                     'user_options': {},
diff --git a/jupyterhub/user.py b/jupyterhub/user.py
index ae360ed794..9fd298090a 100644
--- a/jupyterhub/user.py
+++ b/jupyterhub/user.py
@@ -17,7 +17,13 @@
 from .metrics import RUNNING_SERVERS, TOTAL_USERS
 from .objects import Server
 from .spawner import LocalProcessSpawner
-from .utils import AnyTimeoutError, make_ssl_context, maybe_future, url_path_join
+from .utils import (
+    AnyTimeoutError,
+    make_ssl_context,
+    maybe_future,
+    url_escape_path,
+    url_path_join,
+)
 
 # detailed messages about the most common failure-to-start errors,
 # which manifest timeouts during start
@@ -410,7 +416,9 @@ def _new_spawner(self, server_name, spawner_class=None, **kwargs):
             hub=self.settings.get('hub'),
             authenticator=self.authenticator,
             config=self.settings.get('config'),
-            proxy_spec=url_path_join(self.proxy_spec, server_name, '/'),
+            proxy_spec=url_path_join(
+                self.proxy_spec, url_escape_path(server_name), '/'
+            ),
             _deprecated_db_session=self.db,
             oauth_client_id=client_id,
             cookie_options=self.settings.get('cookie_options', {}),
@@ -494,7 +502,7 @@ def server(self):
     @property
     def escaped_name(self):
         """My name, escaped for use in URLs, cookies, etc."""
-        return quote(self.name, safe='@~')
+        return url_escape_path(self.name)
 
     @property
     def json_escaped_name(self):
@@ -543,13 +551,13 @@ def server_url(self, server_name=''):
         if not server_name:
             return self.url
         else:
-            return url_path_join(self.url, server_name)
+            return url_path_join(self.url, url_escape_path(server_name))
 
     def progress_url(self, server_name=''):
         """API URL for progress endpoint for a server with a given name"""
         url_parts = [self.settings['hub'].base_url, 'api/users', self.escaped_name]
         if server_name:
-            url_parts.extend(['servers', server_name, 'progress'])
+            url_parts.extend(['servers', url_escape_path(server_name), 'progress'])
         else:
             url_parts.extend(['server/progress'])
         return url_path_join(*url_parts)
@@ -623,7 +631,7 @@ async def spawn(self, server_name='', options=None, handler=None):
         if handler:
             await self.refresh_auth(handler)
 
-        base_url = url_path_join(self.base_url, server_name) + '/'
+        base_url = url_path_join(self.base_url, url_escape_path(server_name)) + '/'
 
         orm_server = orm.Server(base_url=base_url)
         db.add(orm_server)
@@ -678,7 +686,8 @@ async def spawn(self, server_name='', options=None, handler=None):
             oauth_client = oauth_provider.add_client(
                 client_id,
                 api_token,
-                url_path_join(self.url, server_name, 'oauth_callback'),
+                url_path_join(self.url, url_escape_path(server_name), 'oauth_callback'),
+                # url_path_join(self.url, server_name, 'oauth_callback'),
                 allowed_roles=allowed_roles,
                 description="Server at %s"
                 % (url_path_join(self.base_url, server_name) + '/'),
@@ -785,7 +794,9 @@ async def spawn(self, server_name='', options=None, handler=None):
                     oauth_provider.add_client(
                         client_id,
                         spawner.api_token,
-                        url_path_join(self.url, server_name, 'oauth_callback'),
+                        url_path_join(
+                            self.url, url_escape_path(server_name), 'oauth_callback'
+                        ),
                     )
                     db.commit()
 
diff --git a/jupyterhub/utils.py b/jupyterhub/utils.py
index 44bdaef985..8855e7ea9c 100644
--- a/jupyterhub/utils.py
+++ b/jupyterhub/utils.py
@@ -19,6 +19,7 @@
 from datetime import datetime, timezone
 from hmac import compare_digest
 from operator import itemgetter
+from urllib.parse import quote
 
 from async_generator import aclosing
 from sqlalchemy.exc import SQLAlchemyError
@@ -371,6 +372,11 @@ def compare_token(compare, token):
     return False
 
 
+def url_escape_path(value):
+    """Escape a value to be used in URLs, cookies, etc."""
+    return quote(value, safe='@~')
+
+
 def url_path_join(*pieces):
     """Join components of url into a relative url.
 


Code review comments:
diff_hunk: @@ -118,6 +118,9 @@ def get(self, user_name=None, server_name=''):
 
     @needs_scope("servers")
     async def _get(self, user_name, server_name):
+        if '/' in server_name:
+            raise web.HTTPError(400, f"Invalid server_name: {server_name}")
path: jupyterhub/handlers/pages.py
text: @user1:
This check is only on GET not on POST, which is why the test is passing. The test doesn't use the single-server GET request, it users the _full user_ (all servers) GET request. It might make sense to move it to the centralized `BaseHandler.spawn_single_user`, or on the User's `new_spawner`. Since there are quite a few places that would also need this check, but they all go through `spawn_single_user` eventually.