Permalink
Browse files

[bug 849480] Simplify get_next_url.

- Improve test coverage of sumo.utils.get_next_url.
- Simplify implementation, since we no longer need to implement
  is_safe_url ourselves.
- Drop untested semantics/checks, like LOGIN_URL.
- Early returns are nice and clean.
  • Loading branch information...
jsocol committed Apr 1, 2013
1 parent 744e3d0 commit 6fd4384f4551c292c973a144b01231d6119fd7ea
Showing with 36 additions and 25 deletions.
  1. +33 −8 apps/sumo/tests/test__utils.py
  2. +3 −17 apps/sumo/utils.py
@@ -43,18 +43,43 @@ def tearDown(self):
self.patcher.stop()
super(GetNextUrlTests, self).tearDown()
- def test_https(self):
- """Verify that protocol and domain get removed for https."""
- r = self.r.post('/users/login',
- {'next': 'https://su.mo.com/kb/new?f=b'})
- eq_('/kb/new?f=b', get_next_url(r))
+ def test_query_string(self):
+ """Query-strings remain intact."""
+ r = self.r.get('/', {'next': '/new?f=b'})
+ eq_('/new?f=b', get_next_url(r))
- def test_http(self):
- """Verify that protocol and domain get removed for http."""
+ def test_good_host_https(self):
+ """Full URLs work with valid hosts."""
r = self.r.post('/users/login',
- {'next': 'http://su.mo.com/kb/new'})
+ {'next': 'https://su.mo.com/kb/new'})
+ eq_('https://su.mo.com/kb/new', get_next_url(r))
+
+ def test_post(self):
+ """'next' in POST overrides GET."""
+ r = self.r.post('/?next=/foo', {'next': '/bar'})
+ eq_('/bar', get_next_url(r))
+
+ def test_get(self):
+ """'next' can be a query-string parameter."""
+ r = self.r.get('/users/login', {'next': '/kb/new'})
eq_('/kb/new', get_next_url(r))
+ def test_referer(self):
+ """Use HTTP referer if nothing else."""
+ r = self.r.get('/')
+ r.META['HTTP_REFERER'] = 'http://su.mo.com/new'
+ eq_('http://su.mo.com/new', get_next_url(r))
+
+ def test_bad_host_https(self):
+ r = self.r.get('/', {'next': 'https://example.com'})
+ eq_(None, get_next_url(r))
+
+ def test_bad_host_protocol_relative(self):
+ """Protocol-relative URLs do not let bad hosts through."""
+ r = self.r.get('/', {'next': '//example.com'})
+ eq_(None, get_next_url(r))
+
+
class JSONTests(TestCase):
def test_truncated_noop(self):
"""Make sure short enough things are unmodified."""
View
@@ -5,7 +5,7 @@
from django.contrib.sites.models import Site
from django.db import models
from django.db.models.signals import pre_delete
-from django.utils.http import urlencode
+from django.utils.http import urlencode, is_safe_url
from sumo import paginator
@@ -117,22 +117,8 @@ def get_next_url(request):
else:
url = request.META.get('HTTP_REFERER')
- if url:
- parsed_url = urlparse.urlparse(url)
- # Don't redirect outside of SUMO.
- # Don't include protocol+domain, so if we are https we stay that way.
- 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 or logout page
- if parsed_url.path in [settings.LOGIN_URL, settings.LOGOUT_URL]:
- url = None
+ if not is_safe_url(url, Site.objects.get_current().domain):
+ return None
return url

0 comments on commit 6fd4384

Please sign in to comment.