On Wed, May 20, 2015 at 9:22 AM, Thomas De Schampheleire <[email protected]> wrote: > # HG changeset patch > # User Thomas De Schampheleire <[email protected]> > # Date 1432065035 -7200 > # Tue May 19 21:50:35 2015 +0200 > # Node ID cedc3ee5ef792a77a515997c90b38099f4688166 > # Parent 579110ca5178f13254e7e4c7b6043767a11b92a2 > login: preserve GET arguments throughout login redirection (issue #104) > > When redirecting a user to the login page and while handling this login and > redirecting to the original page, the GET arguments passed to the original > URL are lost through the login redirection process. > > For example, when creating a pull request for a specific revision from the > repository changelog, there are rev_start and rev_end arguments passed in > the URL. Through the login redirection, they are lost. > > Fix the issue by passing along the GET arguments to the login page, in the > login form action, and when redirecting back to the original page. > Tests are added to cover these cases. > > diff --git a/kallithea/controllers/login.py b/kallithea/controllers/login.py > --- a/kallithea/controllers/login.py > +++ b/kallithea/controllers/login.py > @@ -100,7 +100,12 @@ class LoginController(BaseController): > log.error('Suspicious NETLOC detected %s for url %s server url ' > 'is: %s' % (parsed.netloc, parsed, server_parsed)) > came_from = url('home') > - return came_from > + return came_from.encode('ascii') > + > + def _redirect_to_origin(self, origin, headers=None): > + '''redirect to the original page, preserving any get arguments > given''' > + request.GET.pop('came_from', None) > + raise HTTPFound(location=url(origin, **request.GET), headers=headers) > > def index(self): > _default_came_from = url('home') > @@ -112,7 +117,7 @@ class LoginController(BaseController): > > # redirect if already logged in > if self.authuser.is_authenticated and not_default and ip_allowed: > - raise HTTPFound(location=c.came_from) > + return self._redirect_to_origin(c.came_from) > > if request.POST: > # import Login Form validator class > @@ -124,7 +129,8 @@ class LoginController(BaseController): > headers = self._store_user_in_session( > username=c.form_result['username'], > remember=c.form_result['remember']) > - raise HTTPFound(location=c.came_from, headers=headers) > + return self._redirect_to_origin(c.came_from, headers) > + > except formencode.Invalid, errors: > defaults = errors.value > # remove password from filling in form again > @@ -157,7 +163,8 @@ class LoginController(BaseController): > > if auth_info: > headers = > self._store_user_in_session(auth_info.get('username')) > - raise HTTPFound(location=c.came_from, headers=headers) > + return self._redirect_to_origin(c.came_from, headers) > + > return render('/login.html') > > @HasPermissionAnyDecorator('hg.admin', 'hg.register.auto_activate', > diff --git a/kallithea/lib/auth.py b/kallithea/lib/auth.py > --- a/kallithea/lib/auth.py > +++ b/kallithea/lib/auth.py > @@ -711,7 +711,7 @@ def redirect_to_login(message=None): > if message: > h.flash(h.literal(message), category='warning') > log.debug('Redirecting to login page, origin: %s' % p) > - return redirect(url('login_home', came_from=p)) > + return redirect(url('login_home', came_from=p, **request.GET)) > > class LoginRequired(object): > """ > diff --git a/kallithea/templates/login.html b/kallithea/templates/login.html > --- a/kallithea/templates/login.html > +++ b/kallithea/templates/login.html > @@ -16,7 +16,7 @@ > %endif > </div> > <div class="panel-body inner"> > - ${h.form(h.url.current(came_from=c.came_from))} > + ${h.form(h.url.current(**request.GET))} > <div class="form"> > <i class="icon-lock"></i> > <!-- fields --> > diff --git a/kallithea/tests/functional/test_login.py > b/kallithea/tests/functional/test_login.py > --- a/kallithea/tests/functional/test_login.py > +++ b/kallithea/tests/functional/test_login.py > @@ -96,6 +96,42 @@ class TestLoginController(TestController > response.mustcontain('invalid user name') > response.mustcontain('invalid password') > > + # verify that get arguments are correctly passed along login redirection > + > + def test_redirection_to_login_form_preserves_get_args(self): > + with fixture.anon_access(False): > + response = self.app.get(url(controller='summary', action='index', > + repo_name=HG_REPO, > + foo='one', bar='two')) > + self.assertEqual(response.status, '302 Found') > + self.assertIn('foo=one&bar=two', response.location) > + > + def test_login_form_preserves_get_args(self): > + response = self.app.get(url(controller='login', action='index', > + came_from = '/_admin/users', > + foo='one', bar='two')) > + self.assertIn('foo=one&bar=two', response.form.action) > + > + def test_redirection_after_successful_login_preserves_get_args(self): > + response = self.app.post(url(controller='login', action='index', > + came_from = '/_admin/users', > + foo='one', bar='two'), > + {'username': 'test_admin', > + 'password': 'test12'}) > + self.assertEqual(response.status, '302 Found') > + self.assertIn('foo=one&bar=two', response.location) > + > + def test_login_form_after_incorrect_login_preserves_get_args(self): > + response = self.app.post(url(controller='login', action='index', > + came_from = '/_admin/users', > + foo='one', bar='two'), > + {'username': 'error', > + 'password': 'test12'}) > + > + response.mustcontain('invalid user name') > + response.mustcontain('invalid password') > + self.assertIn('foo=one&bar=two', response.form.action) > + > > #========================================================================== > # REGISTRATIONS > > #==========================================================================
Question: are there security implications with this change? I think it should be OK because the get dictionary is only passed to url() to create URLs that the user could have typed directly. _______________________________________________ kallithea-general mailing list [email protected] http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
