Merge pull request #2905 from graphite-project/copilot/fix-issue-2871

Fix open redirect vulnerability in account login/logout views

Author: deniszh

webapp/graphite/account/views.py

@@ -19,16 +19,28 @@
     from django.urls import reverse
 from django.http import HttpResponseRedirect
 from django.shortcuts import render
+from django.utils.http import url_has_allowed_host_and_scheme
 from graphite.user_util import getProfile, isAuthenticated
 
 
+def _get_safe_next_page(request, param_value):
+    """Return param_value if it is a safe local redirect target, otherwise the browser URL."""
+    if url_has_allowed_host_and_scheme(
+        url=param_value,
+        allowed_hosts={request.get_host()},
+        require_https=request.is_secure(),
+    ):
+        return param_value
+    return reverse('browser')
+
+
 def loginView(request):
     username = request.POST.get('username')
     password = request.POST.get('password')
     if request.method == 'GET':
-        nextPage = request.GET.get('nextPage', reverse('browser'))
+        nextPage = _get_safe_next_page(request, request.GET.get('nextPage', reverse('browser')))
     else:
-        nextPage = request.POST.get('nextPage', reverse('browser'))
+        nextPage = _get_safe_next_page(request, request.POST.get('nextPage', reverse('browser')))
     if username and password:
         user = authenticate(username=username,password=password)
         if user is None:
@@ -43,7 +55,7 @@ def loginView(request):
 
 
 def logoutView(request):
-    nextPage = request.GET.get('nextPage', reverse('browser'))
+    nextPage = _get_safe_next_page(request, request.GET.get('nextPage', reverse('browser')))
     logout(request)
     return HttpResponseRedirect(nextPage)
 
@@ -60,5 +72,5 @@ def updateProfile(request):
     if profile:
         profile.advancedUI = request.POST.get('advancedUI','off') == 'on'
         profile.save()
-    nextPage = request.POST.get('nextPage', reverse('browser'))
+    nextPage = _get_safe_next_page(request, request.POST.get('nextPage', reverse('browser')))
     return HttpResponseRedirect(nextPage)

webapp/tests/test_account.py

@@ -0,0 +1,71 @@
+# -*- coding: utf-8 -*-
+
+from django.contrib.auth.models import User
+from django.urls import reverse
+from .base import TestCase
+
+
+class LogoutViewTest(TestCase):
+    def test_logout_default_redirect(self):
+        """Logout without nextPage redirects to the browser."""
+        url = reverse('account_logout')
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual(response['Location'], reverse('browser'))
+
+    def test_logout_safe_relative_redirect(self):
+        """Logout with a safe relative nextPage redirects there."""
+        url = reverse('account_logout')
+        response = self.client.get(url, {'nextPage': '/graphite/'})
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual(response['Location'], '/graphite/')
+
+    def test_logout_open_redirect_blocked(self):
+        """Logout with an external nextPage falls back to the browser URL."""
+        url = reverse('account_logout')
+        response = self.client.get(url, {'nextPage': 'http://evil.example.com'})
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual(response['Location'], reverse('browser'))
+
+    def test_logout_open_redirect_blocked_https(self):
+        """Logout with an external HTTPS nextPage falls back to the browser URL."""
+        url = reverse('account_logout')
+        response = self.client.get(url, {'nextPage': 'https://evil.example.com/path'})
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual(response['Location'], reverse('browser'))
+
+
+class LoginViewTest(TestCase):
+    def setUp(self):
+        self.user = User.objects.create_user(
+            username='testuser', password='testpass', is_active=True
+        )
+
+    def test_login_open_redirect_blocked_get(self):
+        """GET login page with external nextPage falls back to the browser URL."""
+        url = reverse('account_login')
+        response = self.client.get(url, {'nextPage': 'http://evil.example.com'})
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, reverse('browser'))
+
+    def test_login_open_redirect_blocked_post(self):
+        """POST login with external nextPage falls back to the browser URL."""
+        url = reverse('account_login')
+        response = self.client.post(url, {
+            'username': 'testuser',
+            'password': 'testpass',
+            'nextPage': 'http://evil.example.com',
+        })
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual(response['Location'], reverse('browser'))
+
+    def test_login_safe_relative_redirect(self):
+        """POST login with a safe relative nextPage redirects there."""
+        url = reverse('account_login')
+        response = self.client.post(url, {
+            'username': 'testuser',
+            'password': 'testpass',
+            'nextPage': '/graphite/',
+        })
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual(response['Location'], '/graphite/')