diff --git a/jupyterhub/handlers/base.py b/jupyterhub/handlers/base.py index 07c7026e..91bf4a4e 100644 --- a/jupyterhub/handlers/base.py +++ b/jupyterhub/handlers/base.py @@ -654,8 +654,42 @@ class BaseHandler(RequestHandler): next_url = url_path_join(self.hub.base_url, 'spawn') else: next_url = url_path_join(self.hub.base_url, 'home') + + next_url = self.append_query_parameters(next_url, exclude=['next']) return next_url + def append_query_parameters(self, url, exclude=None): + """Append the current request's query parameters to the given URL. + + Supports an extra optional parameter ``exclude`` that when provided must + contain a list of parameters to be ignored, i.e. these parameters will + not be added to the URL. + + This is important to avoid infinite loops with the next parameter being + added over and over, for instance. + + The default value for ``exclude`` is an array with "next". This is useful + as most use cases in JupyterHub (all?) won't want to include the next + parameter twice (the next parameter is added elsewhere to the query + parameters). + + :param str url: a URL + :param list exclude: optional list of parameters to be ignored, defaults to + a list with "next" (to avoid redirect-loops) + :rtype (str) + """ + if exclude is None: + exclude = ['next'] + if self.request.query: + query_string = [ + param + for param in parse_qsl(self.request.query) + if param[0] not in exclude + ] + if query_string: + url = url_concat(url, query_string) + return url + async def auth_to_user(self, authenticated, user=None): """Persist data from .authenticate() or .refresh_user() to the User database diff --git a/jupyterhub/handlers/pages.py b/jupyterhub/handlers/pages.py index ffc44bb5..ea0c1d34 100644 --- a/jupyterhub/handlers/pages.py +++ b/jupyterhub/handlers/pages.py @@ -285,6 +285,8 @@ class SpawnHandler(BaseHandler): self.hub.base_url, "spawn-pending", user.escaped_name, server_name ) + pending_url = self.append_query_parameters(pending_url, exclude=['next']) + if self.get_argument('next', None): # preserve `?next=...` through spawn-pending pending_url = url_concat(pending_url, {'next': self.get_argument('next')}) diff --git a/jupyterhub/tests/test_pages.py b/jupyterhub/tests/test_pages.py index a1825d43..74de03a5 100644 --- a/jupyterhub/tests/test_pages.py +++ b/jupyterhub/tests/test_pages.py @@ -9,6 +9,7 @@ from urllib.parse import urlparse import pytest from bs4 import BeautifulSoup from tornado import gen +from tornado.escape import url_escape from tornado.httputil import url_concat from .. import orm @@ -516,6 +517,58 @@ async def test_user_redirect_deprecated(app, username): ) +@pytest.mark.parametrize( + 'url, params, redirected_url, form_action', + [ + ( + # spawn?param=value + # will encode given parameters for an unauthenticated URL in the next url + # the next parameter will contain the app base URL (replaces BASE_URL in tests) + 'spawn', + [('param', 'value')], + '/hub/login?next={{BASE_URL}}hub%2Fspawn%3Fparam%3Dvalue', + '/hub/login?next={{BASE_URL}}hub%2Fspawn%3Fparam%3Dvalue', + ), + ( + # login?param=fromlogin&next=encoded(/hub/spawn?param=value) + # will drop parameters given to the login page, passing only the next url + 'login', + [('param', 'fromlogin'), ('next', '/hub/spawn?param=value')], + '/hub/login?param=fromlogin&next=%2Fhub%2Fspawn%3Fparam%3Dvalue', + '/hub/login?next=%2Fhub%2Fspawn%3Fparam%3Dvalue', + ), + ( + # login?param=value&anotherparam=anothervalue + # will drop parameters given to the login page, and use an empty next url + 'login', + [('param', 'value'), ('anotherparam', 'anothervalue')], + '/hub/login?param=value&anotherparam=anothervalue', + '/hub/login?next=', + ), + ( + # login + # simplest case, accessing the login URL, gives an empty next url + 'login', + [], + '/hub/login', + '/hub/login?next=', + ), + ], +) +async def test_login_page(app, url, params, redirected_url, form_action): + url = url_concat(url, params) + r = await get_page(url, app) + redirected_url = redirected_url.replace('{{BASE_URL}}', url_escape(app.base_url)) + assert r.url.endswith(redirected_url) + # now the login.html rendered template must include the given parameters in the form + # action URL, including the next URL + page = BeautifulSoup(r.text, "html.parser") + form = page.find("form", method="post") + action = form.attrs['action'] + form_action = form_action.replace('{{BASE_URL}}', url_escape(app.base_url)) + assert action.endswith(form_action) + + async def test_login_fail(app): name = 'wash' base_url = public_url(app) @@ -546,26 +599,29 @@ async def test_login_strip(app): @pytest.mark.parametrize( - 'running, next_url, location', + 'running, next_url, location, params', [ # default URL if next not specified, for both running and not - (True, '', ''), - (False, '', ''), + (True, '', '', None), + (False, '', '', None), # next_url is respected - (False, '/hub/admin', '/hub/admin'), - (False, '/user/other', '/hub/user/other'), - (False, '/absolute', '/absolute'), - (False, '/has?query#andhash', '/has?query#andhash'), + (False, '/hub/admin', '/hub/admin', None), + (False, '/user/other', '/hub/user/other', None), + (False, '/absolute', '/absolute', None), + (False, '/has?query#andhash', '/has?query#andhash', None), # next_url outside is not allowed - (False, 'relative/path', ''), - (False, 'https://other.domain', ''), - (False, 'ftp://other.domain', ''), - (False, '//other.domain', ''), - (False, '///other.domain/triple', ''), - (False, '\\\\other.domain/backslashes', ''), + (False, 'relative/path', '', None), + (False, 'https://other.domain', '', None), + (False, 'ftp://other.domain', '', None), + (False, '//other.domain', '', None), + (False, '///other.domain/triple', '', None), + (False, '\\\\other.domain/backslashes', '', None), + # params are handled correctly + (True, '/hub/admin', 'hub/admin?left=1&right=2', [('left', 1), ('right', 2)]), + (False, '/hub/admin', 'hub/admin?left=1&right=2', [('left', 1), ('right', 2)]), ], ) -async def test_login_redirect(app, running, next_url, location): +async def test_login_redirect(app, running, next_url, location, params): cookies = await app.login_user('river') user = app.users['river'] if location: @@ -577,6 +633,8 @@ async def test_login_redirect(app, running, next_url, location): location = ujoin(app.base_url, 'hub/spawn') url = 'login' + if params: + url = url_concat(url, params) if next_url: if '//' not in next_url and next_url.startswith('/'): next_url = ujoin(app.base_url, next_url, '')