Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

fix bug 847190 - use django is_safe_url

  • Loading branch information...
commit 76b8b47d308be2deaf4a100cbaea6ad8f49fbe0d 1 parent 5428513
@groovecoder groovecoder authored
View
26 apps/users/tests/test_templates.py
@@ -12,9 +12,7 @@
import mock
from nose import SkipTest
from nose.tools import eq_
-from nose.plugins.attrib import attr
from pyquery import PyQuery as pq
-from test_utils import RequestFactory
from dekicompat.tests import (SINGLE_ACCOUNT_FIXTURE_XML,
mock_post_mindtouch_user,
@@ -28,7 +26,6 @@
from sumo.tests import post
from users.models import RegistrationProfile
from users.tests import TestCaseBase
-from users.views import _clean_next_url
class LoginTests(TestCaseBase):
@@ -109,17 +106,6 @@ def test_login_next_parameter_in_forms(self):
eq_(next, doc('input[name="next"]')[0].attrib['value'])
@mock.patch_object(Site.objects, 'get_current')
- def test_clean_url(self, get_current):
- '''Verify that protocol and domain get removed.'''
- get_current.return_value.domain = 'su.mo.com'
- r = RequestFactory().post('/users/login',
- {'next': 'https://su.mo.com/kb/new?f=b'})
- eq_('/kb/new?f=b', _clean_next_url(r))
- r = RequestFactory().post('/users/login',
- {'next': 'http://su.mo.com/kb/new'})
- eq_('/kb/new', _clean_next_url(r))
-
- @mock.patch_object(Site.objects, 'get_current')
def test_login_invalid_next_parameter(self, get_current):
'''Test with an invalid ?next=http://example.com parameter.'''
get_current.return_value.domain = 'testserver.com'
@@ -141,18 +127,6 @@ def test_login_invalid_next_parameter(self, get_current):
eq_(302, response.status_code)
eq_('http://testserver' + valid_next, response['location'])
- # http://bugzil.la/847190
- @attr('sec')
- @mock.patch_object(Site.objects, 'get_current')
- def test_clean_next_url_protocol_relative_redirect(self, get_current):
- '''Test with an XSS in ?next parameter.'''
- get_current.return_value.domain = 'testserver.com'
- redir_next = '%252f%252fgoo.gl/yY9B5&paddingpaddingpadding'
- redir_request = RequestFactory().get('/users/login',
- {'next': redir_next})
-
- eq_(None, _clean_next_url(redir_request))
-
def test_login_legacy_password(self):
'''Test logging in with a legacy md5 password.'''
legacypw = 'legacypass'
View
61 apps/users/tests/test_views.py
@@ -11,6 +11,7 @@
from nose.tools import eq_, ok_
from nose.plugins.attrib import attr
from pyquery import PyQuery as pq
+from test_utils import RequestFactory
from dekicompat.tests import (mock_mindtouch_login,
mock_missing_get_deki_user,
@@ -22,10 +23,11 @@
from dekicompat.backends import DekiUserBackend, MINDTOUCH_USER_XML
from notifications.tests import watch
+from sumo.helpers import urlparams
from sumo.tests import TestCase, LocalizingClient
from sumo.urlresolvers import reverse
from users.models import RegistrationProfile, EmailChange
-from users.views import SESSION_VERIFIED_EMAIL
+from users.views import SESSION_VERIFIED_EMAIL, _clean_next_url
from users.tests import get_deki_user_doc
@@ -184,6 +186,63 @@ def test_mindtouch_creds_create_user_and_profile(self, get_current):
doc = pq(response.content)
eq_('testaccount', doc.find('ul.user-state a:first').text())
+ @mock.patch_object(Site.objects, 'get_current')
+ def test_clean_next_url_request_properties(self, get_current):
+ '''_clean_next_url checks POST, GET, and REFERER'''
+ get_current.return_value.domain = 'dev.mo.org'
+
+ r = RequestFactory().get('/users/login', {'next': '/demos/submit'},
+ HTTP_REFERER='referer-trumped-by-get')
+ eq_('/demos/submit', _clean_next_url(r))
+ r = RequestFactory().post('/users/login', {'next': '/demos/submit'})
+ eq_('/demos/submit', _clean_next_url(r))
+ r = RequestFactory().get('/users/login', HTTP_REFERER='/demos/submit')
+ eq_('/demos/submit', _clean_next_url(r))
+
+ @mock.patch_object(Site.objects, 'get_current')
+ def test_clean_next_url_no_self_redirects(self, get_current):
+ '''_clean_next_url checks POST, GET, and REFERER'''
+ get_current.return_value.domain = 'dev.mo.org'
+
+ for next in [settings.LOGIN_URL, settings.LOGOUT_URL]:
+ r = RequestFactory().get('/users/login', {'next': next})
+ eq_(None, _clean_next_url(r))
+
+ @mock.patch_object(Site.objects, 'get_current')
+ def test_clean_next_url_invalid_next_parameter(self, get_current):
+ '''_clean_next_url cleans invalid urls'''
+ get_current.return_value.domain = 'dev.mo.org'
+
+ for next in self._invalid_nexts():
+ r = RequestFactory().get('/users/login', {'next': next})
+ eq_(None, _clean_next_url(r))
+
+ @mock.patch_object(Site.objects, 'get_current')
+ def test_login_invalid_next_parameter(self, get_current):
+ '''Test with an invalid ?next=http://example.com parameter.'''
+ get_current.return_value.domain = 'testserver.com'
+ valid_next = reverse('home', locale=settings.LANGUAGE_CODE)
+
+ for invalid_next in self._invalid_nexts():
+ # Verify that _valid_ next parameter is set in form hidden field.
+ response = self.client.get(urlparams(reverse('users.login'),
+ next=invalid_next))
+ eq_(200, response.status_code)
+ doc = pq(response.content)
+ eq_(valid_next, doc('input[name="next"]')[0].attrib['value'])
+
+ # Verify that it gets used on form POST.
+ response = self.client.post(reverse('users.login'),
+ {'username': 'testuser',
+ 'password': 'testpass',
+ 'next': invalid_next})
+ eq_(302, response.status_code)
+ eq_('http://testserver' + valid_next, response['location'])
+ self.client.logout()
+
+ def _invalid_nexts(self):
+ return ['http://foobar.com/evil/', '//goo.gl/y-bad']
+
class RegisterTestCase(TestCase):
fixtures = ['test_users.json']
View
52 apps/users/views.py
@@ -1,5 +1,4 @@
import os
-import urllib
import urlparse
from django.conf import settings
@@ -16,7 +15,7 @@
from django.views.decorators.csrf import csrf_exempt
from django.shortcuts import get_object_or_404
-from django.utils.http import base36_to_int
+from django.utils.http import base36_to_int, is_safe_url
from django_browserid.forms import BrowserIDForm
from django_browserid.auth import get_audience
@@ -600,37 +599,26 @@ def _clean_next_url(request):
elif 'HTTP_REFERER' in request.META:
url = request.META.get('HTTP_REFERER').decode('latin1', 'ignore')
else:
- url = None
-
- if url:
- parsed_url = urlparse.urlparse(url)
- # Don't redirect outside of site_domain.
- # Don't include protocol+domain, so if we are https we stay that way.
- # http://bugzil.la/847190
- relative_url_prefix = urllib.quote_plus(urllib.quote_plus('//'))
- if url.upper().startswith(relative_url_prefix):
- url = None
- if parsed_url.scheme:
- site_domain = Site.objects.get_current().domain
- url_domain = parsed_url.netloc
- if site_domain != url_domain:
- url = None
- else:
- url = u'?'.join([getattr(parsed_url, x) for x in
- ('path', 'query') if getattr(parsed_url, x)])
-
- # Don't redirect right back to login, logout, register, or change email
- # pages
- locale, register_url = split_path(reverse('users.browserid_register'))
- locale, change_email_url = split_path(
- reverse('users.change_email'))
- for looping_url in [settings.LOGIN_URL, settings.LOGOUT_URL,
- register_url, change_email_url]:
- if looping_url in parsed_url.path:
- url = None
+ return None
+
+ site = Site.objects.get_current()
+ if not is_safe_url(url, site.domain):
+ return None
+ parsed_url = urlparse.urlparse(url)
+
+ # Don't redirect right back to login, logout, register, or
+ # change email pages
+ locale, register_url = split_path(reverse(
+ 'users.browserid_register'))
+ locale, change_email_url = split_path(reverse(
+ 'users.change_email'))
+ LOOPING_NEXT_URLS = [settings.LOGIN_URL, settings.LOGOUT_URL,
+ register_url, change_email_url]
+ for looping_url in LOOPING_NEXT_URLS:
+ if looping_url in parsed_url.path:
+ return None
# TODO?HACK: can't use urllib.quote_plus because mod_rewrite quotes the
# next url value already.
- if url:
- url = url.replace(' ', '+')
+ url = url.replace(' ', '+')
return url
Please sign in to comment.
Something went wrong with that request. Please try again.