[PATCH] login: preserve GET arguments throughout login redirection (issue #104)
Mads Kiilerich
mads at kiilerich.com
Wed May 20 05:56:09 EDT 2015
On 05/20/2015 09:27 AM, Thomas De Schampheleire wrote:
> On Wed, May 20, 2015 at 9:22 AM, Thomas De Schampheleire
> <patrickdepinguin at gmail.com> wrote:
>> # HG changeset patch
>> # User Thomas De Schampheleire <thomas.de.schampheleire at gmail.com>
>> # 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
>> #==========================================================================
Yeah - tests! :-)
> 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.
I cannot imagine that. We have the existing "not a good idea" if people
pass API keys in the url but this doesn't make it any worse.
/Mads
More information about the kallithea-general
mailing list