[PATCH] login: always pass server-relative URLs in came_from
Søren Løvborg
sorenl at unity3d.com
Fri Sep 18 13:03:01 UTC 2015
Here's an alternative to Andrew's patch, which fixes both the prefix
problem and a few others.
There's a lot more churn, especially in the test suite, but all-in-all
the result is a 20-line reduction. A test that prefix redirects actually
work should probably be added; for now, it's been verified manually.
# HG changeset patch
# User Søren Løvborg <sorenl at unity3d.com>
# Date 1442577469 -7200
# Fri Sep 18 13:57:49 2015 +0200
# Branch stable
# Node ID 5d855cf256204208a7494c7206c4c6a84cb6b657
# Parent c79e4f89bfd30ce87f0e04024e990b88864d5ea4
login: always pass server-relative URLs in came_from
The login controller uses the came_from query argument to determine
the page to continue to after login.
Previously, came_from specified only the URL path (obtained using
h.url.current), and any URL query parameters were passed along as
separate (additional) URL query parameters; to obtain the final redirect
target, h.url was used to combine came_from with the request.GET.
As of this changeset, came_from specifies both the URL path and query
string (obtained using request.path_qs), which means that came_from can
be used directly as the redirect target (as always, WebOb handles the
task of expanding the server relative path to a fully qualified URL).
This fixes a number of issues with the existing implementation:
* Using h.url to combine came_from with query parameters caused the
SCRIPT_NAME to incorrectly be prepended to came_from, even though
it was already present. This was not a problem if the Kallithea
instance was served directly from the server root ('/') as is common,
but broke setups where Kallithea was served from a prefix.
* Even though only server-relative came_from URLs were ever generated,
the login controller allowed fully qualified URLs (URLs including
scheme and server). To avoid an open HTTP redirect (CWE-601), the code
included logic to prevent redirects to other servers. By requiring
server-relative URLs, this logic can simply be removed.
* The login code appended arbitrary, user-supplied query parameters to
URLs by calling the Routes URLGenerator (h.url) with user-supplied
keyword arguments. This construct is unfortunate, since url only
appends _unknown_ keyword arguments as query parameters, and the
parameter names could overlap with known keyword arguments, possibly
affecting the generated URL in various ways. This changeset removes
this usage from the login code, but other instances remain.
(In practice, the damage is apparently limited to causing an Internal
Server Error when going to e.g. "/_admin/login?host=foo", since WebOb
returns Unicode strings and URLGenerator only allows byte strings for
these keyword arguments.)
diff --git a/kallithea/controllers/login.py b/kallithea/controllers/login.py
--- a/kallithea/controllers/login.py
+++ b/kallithea/controllers/login.py
@@ -51,45 +51,28 @@
log = logging.getLogger(__name__)
+def is_server_relative(url):
+ url = urlparse.urlsplit(url)
+ return not url.scheme and not url.netloc
+
+
class LoginController(BaseController):
def __before__(self):
super(LoginController, self).__before__()
- def _validate_came_from(self, came_from):
- """Return True if came_from is valid and can and should be used"""
- if not came_from:
- return False
-
- parsed = urlparse.urlparse(came_from)
- server_parsed = urlparse.urlparse(url.current())
- allowed_schemes = ['http', 'https']
- if parsed.scheme and parsed.scheme not in allowed_schemes:
- log.error('Suspicious URL scheme detected %s for url %s',
- parsed.scheme, parsed)
- return False
- if server_parsed.netloc != parsed.netloc:
- log.error('Suspicious NETLOC detected %s for url %s server url '
- 'is: %s' % (parsed.netloc, parsed, server_parsed))
- return False
- return True
-
- def _redirect_to_origin(self, origin):
- '''redirect to the original page, preserving any get arguments given'''
- request.GET.pop('came_from', None)
- raise HTTPFound(location=url(origin, **request.GET))
-
def index(self):
- c.came_from = safe_str(request.GET.get('came_from', ''))
- if not self._validate_came_from(c.came_from):
- c.came_from = url('home')
+ c.came_from = safe_str(request.GET.get('came_from', '')) or url('home')
+ if not is_server_relative(c.came_from):
+ log.error('Invalid came_from (not server-relative): %r', c.came_from)
+ raise HTTPBadRequest()
not_default = self.authuser.username != User.DEFAULT_USER
ip_allowed = AuthUser.check_ip_allowed(self.authuser, self.ip_addr)
# redirect if already logged in
if self.authuser.is_authenticated and not_default and ip_allowed:
- return self._redirect_to_origin(c.came_from)
+ raise HTTPFound(location=c.came_from)
if request.POST:
# import Login Form validator class
@@ -119,7 +102,7 @@
else:
log_in_user(user, c.form_result['remember'],
is_external_auth=False)
- return self._redirect_to_origin(c.came_from)
+ raise HTTPFound(location=c.came_from)
return render('/login.html')
diff --git a/kallithea/lib/auth.py b/kallithea/lib/auth.py
--- a/kallithea/lib/auth.py
+++ b/kallithea/lib/auth.py
@@ -712,11 +712,12 @@
def redirect_to_login(message=None):
from kallithea.lib import helpers as h
- p = url.current()
+ p = request.path_qs
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, **request.GET))
+ return redirect(url('login_home', came_from=p))
+
class LoginRequired(object):
"""
diff --git a/kallithea/templates/base/base.html b/kallithea/templates/base/base.html
--- a/kallithea/templates/base/base.html
+++ b/kallithea/templates/base/base.html
@@ -294,7 +294,7 @@
<div id="quick_login">
%if c.authuser.username == 'default' or c.authuser.user_id is None:
<h4>${_('Login to Your Account')}</h4>
- ${h.form(h.url('login_home',came_from=h.url.current()))}
+ ${h.form(h.url('login_home', came_from=request.path_qs))}
<div class="form">
<div class="fields">
<div class="field">
diff --git a/kallithea/templates/changeset/changeset_file_comment.html b/kallithea/templates/changeset/changeset_file_comment.html
--- a/kallithea/templates/changeset/changeset_file_comment.html
+++ b/kallithea/templates/changeset/changeset_file_comment.html
@@ -87,7 +87,7 @@
${h.form('')}
<div class="clearfix">
<div class="comment-help">
- ${_('You need to be logged in to comment.')} <a href="${h.url('login_home',came_from=h.url.current())}">${_('Login now')}</a>
+ ${_('You need to be logged in to comment.')} <a href="${h.url('login_home', came_from=request.path_qs)}">${_('Login now')}</a>
</div>
</div>
<div class="comment-button">
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
@@ -1,6 +1,8 @@
# -*- coding: utf-8 -*-
import re
import time
+import urllib
+import urlparse
import mock
@@ -105,18 +107,14 @@
('file:///etc/passwd',),
('ftp://ftp.example.com',),
('http://other.example.com/bl%C3%A5b%C3%A6rgr%C3%B8d',),
+ ('//evil.example.com/',),
])
def test_login_bad_came_froms(self, url_came_from):
response = self.app.post(url(controller='login', action='index',
came_from=url_came_from),
{'username': TEST_USER_ADMIN_LOGIN,
- 'password': TEST_USER_ADMIN_PASS})
- self.assertEqual(response.status, '302 Found')
- self.assertEqual(response._environ['paste.testing_variables']
- ['tmpl_context'].came_from, '/')
- response = response.follow()
-
- self.assertEqual(response.status, '200 OK')
+ 'password': TEST_USER_ADMIN_PASS},
+ status=400)
def test_login_short_password(self):
response = self.app.post(url(controller='login', action='index'),
@@ -133,64 +131,62 @@
response.mustcontain('Invalid username or password')
- # verify that get arguments are correctly passed along login redirection
+ # verify that query string parameters survive login redirection
@parameterized.expand([
- ({'foo':'one', 'bar':'two'}, ('foo=one', 'bar=two')),
- ({'blue': u'blå'.encode('utf-8'), 'green':u'grøn'},
- ('blue=bl%C3%A5', 'green=gr%C3%B8n')),
+ ('?foo=one&bar=two',),
+ ('?blue=bl%C3%A5&green=gr%C3%B8n',),
])
- def test_redirection_to_login_form_preserves_get_args(self, args, args_encoded):
- with fixture.anon_access(False):
- response = self.app.get(url(controller='summary', action='index',
- repo_name=HG_REPO,
- **args))
- self.assertEqual(response.status, '302 Found')
- for encoded in args_encoded:
- self.assertIn(encoded, response.location)
+ def test_redirection_to_login_form_preserves_query(self, qs):
+ came_from = url('my_pullrequests') + qs
+ response = self.app.get(came_from)
+ self.assertEqual(response.status, '302 Found')
+
+ self.assertIn('came_from=' + urllib.quote(came_from, ''),
+ response.location)
@parameterized.expand([
- ({'foo':'one', 'bar':'two'}, ('foo=one', 'bar=two')),
- ({'blue': u'blå'.encode('utf-8'), 'green':u'grøn'},
- ('blue=bl%C3%A5', 'green=gr%C3%B8n')),
+ ('?foo=one&bar=two',),
+ ('?blue=bl%C3%A5&green=gr%C3%B8n',),
])
- def test_login_form_preserves_get_args(self, args, args_encoded):
+ def test_login_form_preserves_query(self, qs):
+ came_from = url('/_admin/users') + qs
response = self.app.get(url(controller='login', action='index',
- came_from = '/_admin/users',
- **args))
- for encoded in args_encoded:
- self.assertIn(encoded, response.form.action)
+ came_from=came_from))
+
+ self.assertIn('came_from=' + urllib.quote(came_from, ''),
+ response.form.action)
@parameterized.expand([
- ({'foo':'one', 'bar':'two'}, ('foo=one', 'bar=two')),
- ({'blue': u'blå'.encode('utf-8'), 'green':u'grøn'},
- ('blue=bl%C3%A5', 'green=gr%C3%B8n')),
+ ('?foo=one&bar=two',),
+ ('?blue=bl%C3%A5&green=gr%C3%B8n',),
])
- def test_redirection_after_successful_login_preserves_get_args(self, args, args_encoded):
+ def test_redirection_after_successful_login_preserves_query(self, qs):
+ came_from = url('/_admin/users') + qs
response = self.app.post(url(controller='login', action='index',
- came_from = '/_admin/users',
- **args),
+ came_from=came_from),
{'username': TEST_USER_ADMIN_LOGIN,
'password': TEST_USER_ADMIN_PASS})
self.assertEqual(response.status, '302 Found')
- for encoded in args_encoded:
- self.assertIn(encoded, response.location)
+
+ loc = urlparse.urlsplit(response.location)
+ path_qs = urlparse.urlunsplit(('', '', loc.path, loc.query, loc.fragment))
+ self.assertEqual(came_from, path_qs)
@parameterized.expand([
- ({'foo':'one', 'bar':'two'}, ('foo=one', 'bar=two')),
- ({'blue': u'blå'.encode('utf-8'), 'green':u'grøn'},
- ('blue=bl%C3%A5', 'green=gr%C3%B8n')),
+ ('?foo=one&bar=two',),
+ ('?blue=bl%C3%A5&green=gr%C3%B8n',),
])
- def test_login_form_after_incorrect_login_preserves_get_args(self, args, args_encoded):
+ def test_login_form_after_incorrect_login_preserves_query(self, qs):
+ came_from = url('/_admin/users') + qs
response = self.app.post(url(controller='login', action='index',
- came_from = '/_admin/users',
- **args),
+ came_from=came_from),
{'username': 'error',
'password': 'test12'})
response.mustcontain('Invalid username or password')
- for encoded in args_encoded:
- self.assertIn(encoded, response.form.action)
+ self.assertIn('came_from=' + urllib.quote(came_from, ''),
+ response.form.action)
#==========================================================================
# REGISTRATIONS
More information about the kallithea-general
mailing list