Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse code

Fixed #18856 -- Ensured that redirects can't be poisoned by malicious…

… users.
  • Loading branch information...
commit a2f2a399566dd68ce7e312fff5a5ba857066797d 1 parent 0cdfa76
Florian Apolloner authored November 17, 2012
52  django/contrib/auth/views.py
@@ -7,7 +7,7 @@
7 7
 from django.core.urlresolvers import reverse
8 8
 from django.http import HttpResponseRedirect, QueryDict
9 9
 from django.template.response import TemplateResponse
10  
-from django.utils.http import base36_to_int
  10
+from django.utils.http import base36_to_int, is_safe_url
11 11
 from django.utils.translation import ugettext as _
12 12
 from django.shortcuts import resolve_url
13 13
 from django.views.decorators.debug import sensitive_post_parameters
@@ -37,18 +37,12 @@ def login(request, template_name='registration/login.html',
37 37
     if request.method == "POST":
38 38
         form = authentication_form(data=request.POST)
39 39
         if form.is_valid():
40  
-            # Use default setting if redirect_to is empty
41  
-            if not redirect_to:
42  
-                redirect_to = settings.LOGIN_REDIRECT_URL
43  
-            redirect_to = resolve_url(redirect_to)
44  
-
45  
-            netloc = urlparse(redirect_to)[1]
46  
-            # Heavier security check -- don't allow redirection to a different
47  
-            # host.
48  
-            if netloc and netloc != request.get_host():
  40
+
  41
+            # Ensure the user-originating redirection url is safe.
  42
+            if not is_safe_url(url=redirect_to, host=request.get_host()):
49 43
                 redirect_to = resolve_url(settings.LOGIN_REDIRECT_URL)
50 44
 
51  
-            # Okay, security checks complete. Log the user in.
  45
+            # Okay, security check complete. Log the user in.
52 46
             auth_login(request, form.get_user())
53 47
 
54 48
             if request.session.test_cookie_worked():
@@ -82,27 +76,27 @@ def logout(request, next_page=None,
82 76
     Logs out the user and displays 'You are logged out' message.
83 77
     """
84 78
     auth_logout(request)
85  
-    redirect_to = request.REQUEST.get(redirect_field_name, '')
86  
-    if redirect_to:
87  
-        netloc = urlparse(redirect_to)[1]
  79
+
  80
+    if redirect_field_name in request.REQUEST:
  81
+        next_page = request.REQUEST[redirect_field_name]
88 82
         # Security check -- don't allow redirection to a different host.
89  
-        if not (netloc and netloc != request.get_host()):
90  
-            return HttpResponseRedirect(redirect_to)
  83
+        if not is_safe_url(url=next_page, host=request.get_host()):
  84
+            next_page = request.path
91 85
 
92  
-    if next_page is None:
93  
-        current_site = get_current_site(request)
94  
-        context = {
95  
-            'site': current_site,
96  
-            'site_name': current_site.name,
97  
-            'title': _('Logged out')
98  
-        }
99  
-        if extra_context is not None:
100  
-            context.update(extra_context)
101  
-        return TemplateResponse(request, template_name, context,
102  
-                                current_app=current_app)
103  
-    else:
  86
+    if next_page:
104 87
         # Redirect to this page until the session has been cleared.
105  
-        return HttpResponseRedirect(next_page or request.path)
  88
+        return HttpResponseRedirect(next_page)
  89
+
  90
+    current_site = get_current_site(request)
  91
+    context = {
  92
+        'site': current_site,
  93
+        'site_name': current_site.name,
  94
+        'title': _('Logged out')
  95
+    }
  96
+    if extra_context is not None:
  97
+        context.update(extra_context)
  98
+    return TemplateResponse(request, template_name, context,
  99
+        current_app=current_app)
106 100
 
107 101
 
108 102
 def logout_then_login(request, login_url=None, current_app=None, extra_context=None):
8  django/contrib/comments/views/comments.py
@@ -44,9 +44,6 @@ def post_comment(request, next=None, using=None):
44 44
         if not data.get('email', ''):
45 45
             data["email"] = request.user.email
46 46
 
47  
-    # Check to see if the POST data overrides the view's next argument.
48  
-    next = data.get("next", next)
49  
-
50 47
     # Look up the object we're trying to comment about
51 48
     ctype = data.get("content_type")
52 49
     object_pk = data.get("object_pk")
@@ -100,7 +97,7 @@ def post_comment(request, next=None, using=None):
100 97
             template_list, {
101 98
                 "comment": form.data.get("comment", ""),
102 99
                 "form": form,
103  
-                "next": next,
  100
+                "next": data.get("next", next),
104 101
             },
105 102
             RequestContext(request, {})
106 103
         )
@@ -131,7 +128,8 @@ def post_comment(request, next=None, using=None):
131 128
         request=request
132 129
     )
133 130
 
134  
-    return next_redirect(data, next, comment_done, c=comment._get_pk_val())
  131
+    return next_redirect(request, fallback=next or 'comments-comment-done',
  132
+        c=comment._get_pk_val())
135 133
 
136 134
 comment_done = confirmation_view(
137 135
     template="comments/posted.html",
10  django/contrib/comments/views/moderation.py
@@ -10,7 +10,6 @@
10 10
 from django.views.decorators.csrf import csrf_protect
11 11
 
12 12
 
13  
-
14 13
 @csrf_protect
15 14
 @login_required
16 15
 def flag(request, comment_id, next=None):
@@ -27,7 +26,8 @@ def flag(request, comment_id, next=None):
27 26
     # Flag on POST
28 27
     if request.method == 'POST':
29 28
         perform_flag(request, comment)
30  
-        return next_redirect(request.POST.copy(), next, flag_done, c=comment.pk)
  29
+        return next_redirect(request, fallback=next or 'comments-flag-done',
  30
+            c=comment.pk)
31 31
 
32 32
     # Render a form on GET
33 33
     else:
@@ -54,7 +54,8 @@ def delete(request, comment_id, next=None):
54 54
     if request.method == 'POST':
55 55
         # Flag the comment as deleted instead of actually deleting it.
56 56
         perform_delete(request, comment)
57  
-        return next_redirect(request.POST.copy(), next, delete_done, c=comment.pk)
  57
+        return next_redirect(request, fallback=next or 'comments-delete-done',
  58
+            c=comment.pk)
58 59
 
59 60
     # Render a form on GET
60 61
     else:
@@ -81,7 +82,8 @@ def approve(request, comment_id, next=None):
81 82
     if request.method == 'POST':
82 83
         # Flag the comment as approved.
83 84
         perform_approve(request, comment)
84  
-        return next_redirect(request.POST.copy(), next, approve_done, c=comment.pk)
  85
+        return next_redirect(request, fallback=next or 'comments-approve-done',
  86
+            c=comment.pk)
85 87
 
86 88
     # Render a form on GET
87 89
     else:
17  django/contrib/comments/views/utils.py
@@ -9,25 +9,26 @@
9 9
     from urllib import urlencode
10 10
 
11 11
 from django.http import HttpResponseRedirect
12  
-from django.core import urlresolvers
13  
-from django.shortcuts import render_to_response
  12
+from django.shortcuts import render_to_response, resolve_url
14 13
 from django.template import RequestContext
15 14
 from django.core.exceptions import ObjectDoesNotExist
16 15
 from django.contrib import comments
  16
+from django.utils.http import is_safe_url
17 17
 
18  
-def next_redirect(data, default, default_view, **get_kwargs):
  18
+def next_redirect(request, fallback, **get_kwargs):
19 19
     """
20 20
     Handle the "where should I go next?" part of comment views.
21 21
 
22  
-    The next value could be a kwarg to the function (``default``), or a
23  
-    ``?next=...`` GET arg, or the URL of a given view (``default_view``). See
  22
+    The next value could be a
  23
+    ``?next=...`` GET arg or the URL of a given view (``fallback``). See
24 24
     the view modules for examples.
25 25
 
26 26
     Returns an ``HttpResponseRedirect``.
27 27
     """
28  
-    next = data.get("next", default)
29  
-    if next is None:
30  
-        next = urlresolvers.reverse(default_view)
  28
+    next = request.POST.get('next')
  29
+    if not is_safe_url(url=next, host=request.get_host()):
  30
+        next = resolve_url(fallback)
  31
+
31 32
     if get_kwargs:
32 33
         if '#' in next:
33 34
             tmp = next.rsplit('#', 1)
12  django/utils/http.py
@@ -227,3 +227,15 @@ def same_origin(url1, url2):
227 227
     """
228 228
     p1, p2 = urllib_parse.urlparse(url1), urllib_parse.urlparse(url2)
229 229
     return (p1.scheme, p1.hostname, p1.port) == (p2.scheme, p2.hostname, p2.port)
  230
+
  231
+def is_safe_url(url, host=None):
  232
+    """
  233
+    Return ``True`` if the url is a safe redirection (i.e. it doesn't point to
  234
+    a different host).
  235
+
  236
+    Always returns ``False`` on an empty url.
  237
+    """
  238
+    if not url:
  239
+        return False
  240
+    netloc = urllib_parse.urlparse(url)[1]
  241
+    return not netloc or netloc == host
11  django/views/i18n.py
@@ -9,6 +9,7 @@
9 9
 from django.utils.encoding import smart_text
10 10
 from django.utils.formats import get_format_modules, get_format
11 11
 from django.utils._os import upath
  12
+from django.utils.http import is_safe_url
12 13
 from django.utils import six
13 14
 
14 15
 def set_language(request):
@@ -22,11 +23,11 @@ def set_language(request):
22 23
     redirect to the page in the request (the 'next' parameter) without changing
23 24
     any state.
24 25
     """
25  
-    next = request.REQUEST.get('next', None)
26  
-    if not next:
27  
-        next = request.META.get('HTTP_REFERER', None)
28  
-    if not next:
29  
-        next = '/'
  26
+    next = request.REQUEST.get('next')
  27
+    if not is_safe_url(url=next, host=request.get_host()):
  28
+        next = request.META.get('HTTP_REFERER')
  29
+        if not is_safe_url(url=next, host=request.get_host()):
  30
+            next = '/'
30 31
     response = http.HttpResponseRedirect(next)
31 32
     if request.method == 'POST':
32 33
         lang_code = request.POST.get('language', None)
7  tests/regressiontests/comment_tests/tests/comment_view_tests.py
@@ -222,6 +222,13 @@ def testCommentNext(self):
222 222
         match = re.search(r"^http://testserver/somewhere/else/\?c=\d+$", location)
223 223
         self.assertTrue(match != None, "Unexpected redirect location: %s" % location)
224 224
 
  225
+        data["next"] = "http://badserver/somewhere/else/"
  226
+        data["comment"] = "This is another comment with an unsafe next url"
  227
+        response = self.client.post("/post/", data)
  228
+        location = response["Location"]
  229
+        match = post_redirect_re.match(location)
  230
+        self.assertTrue(match != None, "Unsafe redirection to: %s" % location)
  231
+
225 232
     def testCommentDoneView(self):
226 233
         a = Article.objects.get(pk=1)
227 234
         data = self.getValidData(a)
90  tests/regressiontests/comment_tests/tests/moderation_view_tests.py
@@ -4,7 +4,6 @@
4 4
 from django.contrib.comments import signals
5 5
 from django.contrib.comments.models import Comment, CommentFlag
6 6
 from django.contrib.contenttypes.models import ContentType
7  
-from django.conf import settings
8 7
 
9 8
 from . import CommentTestCase
10 9
 
@@ -30,6 +29,30 @@ def testFlagPost(self):
30 29
         self.assertEqual(c.flags.filter(flag=CommentFlag.SUGGEST_REMOVAL).count(), 1)
31 30
         return c
32 31
 
  32
+    def testFlagPostNext(self):
  33
+        """
  34
+        POST the flag view, explicitly providing a next url.
  35
+        """
  36
+        comments = self.createSomeComments()
  37
+        pk = comments[0].pk
  38
+        self.client.login(username="normaluser", password="normaluser")
  39
+        response = self.client.post("/flag/%d/" % pk, {'next': "/go/here/"})
  40
+        self.assertEqual(response["Location"],
  41
+            "http://testserver/go/here/?c=1")
  42
+
  43
+    def testFlagPostUnsafeNext(self):
  44
+        """
  45
+        POSTing to the flag view with an unsafe next url will ignore the
  46
+        provided url when redirecting.
  47
+        """
  48
+        comments = self.createSomeComments()
  49
+        pk = comments[0].pk
  50
+        self.client.login(username="normaluser", password="normaluser")
  51
+        response = self.client.post("/flag/%d/" % pk,
  52
+            {'next': "http://elsewhere/bad"})
  53
+        self.assertEqual(response["Location"],
  54
+            "http://testserver/flagged/?c=%d" % pk)
  55
+
33 56
     def testFlagPostTwice(self):
34 57
         """Users don't get to flag comments more than once."""
35 58
         c = self.testFlagPost()
@@ -49,7 +72,7 @@ def testFlagAnon(self):
49 72
     def testFlaggedView(self):
50 73
         comments = self.createSomeComments()
51 74
         pk = comments[0].pk
52  
-        response = self.client.get("/flagged/", data={"c":pk})
  75
+        response = self.client.get("/flagged/", data={"c": pk})
53 76
         self.assertTemplateUsed(response, "comments/flagged.html")
54 77
 
55 78
     def testFlagSignals(self):
@@ -101,6 +124,33 @@ def testDeletePost(self):
101 124
         self.assertTrue(c.is_removed)
102 125
         self.assertEqual(c.flags.filter(flag=CommentFlag.MODERATOR_DELETION, user__username="normaluser").count(), 1)
103 126
 
  127
+    def testDeletePostNext(self):
  128
+        """
  129
+        POSTing the delete view will redirect to an explicitly provided a next
  130
+        url.
  131
+        """
  132
+        comments = self.createSomeComments()
  133
+        pk = comments[0].pk
  134
+        makeModerator("normaluser")
  135
+        self.client.login(username="normaluser", password="normaluser")
  136
+        response = self.client.post("/delete/%d/" % pk, {'next': "/go/here/"})
  137
+        self.assertEqual(response["Location"],
  138
+            "http://testserver/go/here/?c=1")
  139
+
  140
+    def testDeletePostUnsafeNext(self):
  141
+        """
  142
+        POSTing to the delete view with an unsafe next url will ignore the
  143
+        provided url when redirecting.
  144
+        """
  145
+        comments = self.createSomeComments()
  146
+        pk = comments[0].pk
  147
+        makeModerator("normaluser")
  148
+        self.client.login(username="normaluser", password="normaluser")
  149
+        response = self.client.post("/delete/%d/" % pk,
  150
+            {'next': "http://elsewhere/bad"})
  151
+        self.assertEqual(response["Location"],
  152
+            "http://testserver/deleted/?c=%d" % pk)
  153
+
104 154
     def testDeleteSignals(self):
105 155
         def receive(sender, **kwargs):
106 156
             received_signals.append(kwargs.get('signal'))
@@ -116,13 +166,13 @@ def receive(sender, **kwargs):
116 166
     def testDeletedView(self):
117 167
         comments = self.createSomeComments()
118 168
         pk = comments[0].pk
119  
-        response = self.client.get("/deleted/", data={"c":pk})
  169
+        response = self.client.get("/deleted/", data={"c": pk})
120 170
         self.assertTemplateUsed(response, "comments/deleted.html")
121 171
 
122 172
 class ApproveViewTests(CommentTestCase):
123 173
 
124 174
     def testApprovePermissions(self):
125  
-        """The delete view should only be accessible to 'moderators'"""
  175
+        """The approve view should only be accessible to 'moderators'"""
126 176
         comments = self.createSomeComments()
127 177
         pk = comments[0].pk
128 178
         self.client.login(username="normaluser", password="normaluser")
@@ -134,7 +184,7 @@ def testApprovePermissions(self):
134 184
         self.assertEqual(response.status_code, 200)
135 185
 
136 186
     def testApprovePost(self):
137  
-        """POSTing the delete view should mark the comment as removed"""
  187
+        """POSTing the approve view should mark the comment as removed"""
138 188
         c1, c2, c3, c4 = self.createSomeComments()
139 189
         c1.is_public = False; c1.save()
140 190
 
@@ -146,6 +196,36 @@ def testApprovePost(self):
146 196
         self.assertTrue(c.is_public)
147 197
         self.assertEqual(c.flags.filter(flag=CommentFlag.MODERATOR_APPROVAL, user__username="normaluser").count(), 1)
148 198
 
  199
+    def testApprovePostNext(self):
  200
+        """
  201
+        POSTing the approve view will redirect to an explicitly provided a next
  202
+        url.
  203
+        """
  204
+        c1, c2, c3, c4 = self.createSomeComments()
  205
+        c1.is_public = False; c1.save()
  206
+
  207
+        makeModerator("normaluser")
  208
+        self.client.login(username="normaluser", password="normaluser")
  209
+        response = self.client.post("/approve/%d/" % c1.pk,
  210
+            {'next': "/go/here/"})
  211
+        self.assertEqual(response["Location"],
  212
+            "http://testserver/go/here/?c=1")
  213
+
  214
+    def testApprovePostUnsafeNext(self):
  215
+        """
  216
+        POSTing to the approve view with an unsafe next url will ignore the
  217
+        provided url when redirecting.
  218
+        """
  219
+        c1, c2, c3, c4 = self.createSomeComments()
  220
+        c1.is_public = False; c1.save()
  221
+
  222
+        makeModerator("normaluser")
  223
+        self.client.login(username="normaluser", password="normaluser")
  224
+        response = self.client.post("/approve/%d/" % c1.pk,
  225
+            {'next': "http://elsewhere/bad"})
  226
+        self.assertEqual(response["Location"],
  227
+            "http://testserver/approved/?c=%d" % c1.pk)
  228
+
149 229
     def testApproveSignals(self):
150 230
         def receive(sender, **kwargs):
151 231
             received_signals.append(kwargs.get('signal'))
17  tests/regressiontests/views/tests/i18n.py
@@ -25,13 +25,28 @@ class I18NTests(TestCase):
25 25
     """ Tests django views in django/views/i18n.py """
26 26
 
27 27
     def test_setlang(self):
28  
-        """The set_language view can be used to change the session language"""
  28
+        """
  29
+        The set_language view can be used to change the session language.
  30
+
  31
+        The user is redirected to the 'next' argument if provided.
  32
+        """
29 33
         for lang_code, lang_name in settings.LANGUAGES:
30 34
             post_data = dict(language=lang_code, next='/views/')
31 35
             response = self.client.post('/views/i18n/setlang/', data=post_data)
32 36
             self.assertRedirects(response, 'http://testserver/views/')
33 37
             self.assertEqual(self.client.session['django_language'], lang_code)
34 38
 
  39
+    def test_setlang_unsafe_next(self):
  40
+        """
  41
+        The set_language view only redirects to the 'next' argument if it is
  42
+        "safe".
  43
+        """
  44
+        lang_code, lang_name = settings.LANGUAGES[0]
  45
+        post_data = dict(language=lang_code, next='//unsafe/redirection/')
  46
+        response = self.client.post('/views/i18n/setlang/', data=post_data)
  47
+        self.assertEqual(response['Location'], 'http://testserver/')
  48
+        self.assertEqual(self.client.session['django_language'], lang_code)
  49
+
35 50
     def test_setlang_reversal(self):
36 51
         self.assertEqual(reverse('set_language'), '/views/i18n/setlang/')
37 52
 

0 notes on commit a2f2a39

Please sign in to comment.
Something went wrong with that request. Please try again.