From 87e4f458fb72df7ace625f066c6bb7235ea9d013 Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 17 Nov 2020 11:00:37 +0100 Subject: [PATCH] only preserve params when ?next= is not specified --- jupyterhub/handlers/base.py | 11 +++- jupyterhub/tests/test_pages.py | 96 +++++++++++++++++++++++++++++++--- 2 files changed, 99 insertions(+), 8 deletions(-) diff --git a/jupyterhub/handlers/base.py b/jupyterhub/handlers/base.py index c8b2dc9e..94cf27bf 100644 --- a/jupyterhub/handlers/base.py +++ b/jupyterhub/handlers/base.py @@ -634,6 +634,12 @@ class BaseHandler(RequestHandler): next_url, ) + # this is where we know if next_url is coming from ?next= param or we are using a default url + if next_url: + next_url_from_param = True + else: + next_url_from_param = False + if not next_url: # custom default URL, usually passed because user landed on that page but was not logged in if default: @@ -659,7 +665,10 @@ class BaseHandler(RequestHandler): else: next_url = url_path_join(self.hub.base_url, 'home') - next_url = self.append_query_parameters(next_url, exclude=['next']) + if not next_url_from_param: + # when a request made with ?next=... assume all the params have already been encoded + # otherwise, preserve params from the current request across the redirect + next_url = self.append_query_parameters(next_url, exclude=['next']) return next_url def append_query_parameters(self, url, exclude=None): diff --git a/jupyterhub/tests/test_pages.py b/jupyterhub/tests/test_pages.py index 51b5b258..fd4ab168 100644 --- a/jupyterhub/tests/test_pages.py +++ b/jupyterhub/tests/test_pages.py @@ -31,7 +31,7 @@ async def test_root_no_auth(app): url = ujoin(public_host(app), app.hub.base_url) r = await async_requests.get(url) r.raise_for_status() - assert r.url == ujoin(url, 'login') + assert r.url == url_concat(ujoin(url, 'login'), dict(next=app.hub.base_url)) async def test_root_auth(app): @@ -622,9 +622,16 @@ async def test_login_strip(app): (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)]), + # params are handled correctly (ignored if ?next= specified) + ( + True, + '/hub/admin?left=1&right=2', + 'hub/admin?left=1&right=2', + {"left": "abc"}, + ), + (False, '/hub/admin', 'hub/admin', [('left', 1), ('right', 2)]), + (True, '', '', {"keep": "yes"}), + (False, '', '', {"keep": "yes"}), ], ) async def test_login_redirect(app, running, next_url, location, params): @@ -633,10 +640,15 @@ async def test_login_redirect(app, running, next_url, location, params): if location: location = ujoin(app.base_url, location) elif running: + # location not specified, location = user.url + if params: + location = url_concat(location, params) else: # use default url location = ujoin(app.base_url, 'hub/spawn') + if params: + location = url_concat(location, params) url = 'login' if params: @@ -655,7 +667,73 @@ async def test_login_redirect(app, running, next_url, location, params): r = await get_page(url, app, cookies=cookies, allow_redirects=False) r.raise_for_status() assert r.status_code == 302 - assert location == r.headers['Location'] + assert r.headers["Location"] == location + + +@pytest.mark.parametrize( + 'location, next, extra_params', + [ + ( + "{base_url}hub/spawn?a=5", + None, + {"a": "5"}, + ), # no ?next= given, preserve params + ("/x", "/x", {"a": "5"}), # ?next=given, params ignored + ( + "/x?b=10", + "/x?b=10", + {"a": "5"}, + ), # ?next=given with params, additional params ignored + ], +) +async def test_next_url(app, user, location, next, extra_params): + params = {} + if extra_params: + params.update(extra_params) + if next: + params["next"] = next + url = url_concat("/", params) + cookies = await app.login_user("monster") + + # location can be a string template + location = location.format(base_url=app.base_url) + + r = await get_page(url, app, cookies=cookies, allow_redirects=False) + r.raise_for_status() + assert r.status_code == 302 + assert r.headers["Location"] == location + + +async def test_next_url_params_sequence(app, user): + """Test each step of / -> login -> spawn + + and whether they preserve url params + """ + params = {"xyz": "5"} + # first request: root page, with params, not logged in + r = await get_page("/?xyz=5", app, allow_redirects=False) + r.raise_for_status() + location = r.headers["Location"] + + # next page: login + cookies = await app.login_user(user.name) + assert location == url_concat( + ujoin(app.base_url, "/hub/login"), {"next": ujoin(app.base_url, "/hub/?xyz=5")} + ) + r = await async_requests.get( + public_host(app) + location, cookies=cookies, allow_redirects=False + ) + r.raise_for_status() + location = r.headers["Location"] + + # after login, redirect back + assert location == ujoin(app.base_url, "/hub/?xyz=5") + r = await async_requests.get( + public_host(app) + location, cookies=cookies, allow_redirects=False + ) + r.raise_for_status() + location = r.headers["Location"] + assert location == ujoin(app.base_url, "/hub/spawn?xyz=5") async def test_auto_login(app, request): @@ -669,14 +747,18 @@ async def test_auto_login(app, request): ) # no auto_login: end up at /hub/login r = await async_requests.get(base_url) - assert r.url == public_url(app, path='hub/login') + assert r.url == url_concat( + public_url(app, path="hub/login"), {"next": app.hub.base_url} + ) # enable auto_login: redirect from /hub/login to /hub/dummy authenticator = Authenticator(auto_login=True) authenticator.login_url = lambda base_url: ujoin(base_url, 'dummy') with mock.patch.dict(app.tornado_settings, {'authenticator': authenticator}): r = await async_requests.get(base_url) - assert r.url == public_url(app, path='hub/dummy') + assert r.url == url_concat( + public_url(app, path="hub/dummy"), {"next": app.hub.base_url} + ) async def test_auto_login_logout(app):