- Fix remaining open redirect sources

CHANGES.rst

@@ -4,6 +4,8 @@ Change Log
 2.6.1 (unreleased)
 ------------------
 
+- Fix remaining open redirect sources
+
 
 2.6.0 (2021-02-23)
 ------------------

src/Products/PluggableAuthService/PluggableAuthService.py

@@ -73,6 +73,7 @@
 from .utils import classImplements
 from .utils import createKeywords
 from .utils import createViewName
+from .utils import url_local
 
 
 security = ModuleSecurityInfo(
@@ -1153,7 +1154,7 @@ def logout(self, REQUEST):
         # credentials can be acted upon to e.g. go back to the login page
         referrer = REQUEST.get('HTTP_REFERER')  # optional header
         if referrer:
-            REQUEST['RESPONSE'].redirect(referrer)
+            REQUEST['RESPONSE'].redirect(url_local(referrer))
 
     @security.public
     def resetCredentials(self, request, response):

src/Products/PluggableAuthService/plugins/CookieAuthHelper.py

@@ -28,8 +28,6 @@
 import six
 from six.moves.urllib.parse import quote
 from six.moves.urllib.parse import unquote
-from six.moves.urllib.parse import urlparse
-from six.moves.urllib.parse import urlunparse
 
 from AccessControl.class_init import InitializeClass
 from AccessControl.Permissions import view
@@ -45,6 +43,7 @@
 from ..interfaces.plugins import ILoginPasswordHostExtractionPlugin
 from ..plugins.BasePlugin import BasePlugin
 from ..utils import classImplements
+from ..utils import url_local
 
 
 class ICookieAuthHelper(Interface):
@@ -229,9 +228,7 @@ def unauthorized(self):
 
             # Sanitize the return URL ``came_from`` and only allow local URLs
             # to prevent an open exploitable redirect issue
-            if came_from:
-                parsed = urlparse(came_from)
-                came_from = urlunparse(('', '') + parsed[2:])
+            came_from = url_local(came_from)
 
             if '?' in url:
                 sep = '&'
@@ -279,7 +276,7 @@ def login(self):
         if pas_instance is not None:
             pas_instance.updateCredentials(request, response, login, password)
 
-        came_from = request.form['came_from']
+        came_from = url_local(request.form['came_from'])
 
         return response.redirect(came_from)
 

src/Products/PluggableAuthService/plugins/tests/test_CookieAuthHelper.py

@@ -175,6 +175,31 @@ def test_resetCredentials(self):
         helper.resetCredentials(request, response)
         self.assertEqual(len(response.cookies), 0)
 
+    def test_login_redirect(self):
+        helper = self._makeOne()
+        response = FauxCookieResponse()
+        request = FauxSettableRequest(RESPONSE=response)
+        helper.REQUEST = request
+
+        # came_from empty, redirect is empty as well
+        url = ''
+        request.form['came_from'] = url
+        helper.login()
+        self.assertEqual(response.headers['Location'], url)
+
+        # came_from is site-local, redirect won't change the URL
+        url = '/foo?arg1=val1&arg2=val2'
+        request.form['came_from'] = url
+        helper.login()
+        self.assertEqual(response.headers['Location'], url)
+
+        # Protocol and host parts will be chopped off
+        url = 'http://evil.com/foo?arg1=val1&arg2=val2'
+        request.form['came_from'] = url
+        helper.login()
+        self.assertEqual(response.headers['Location'],
+                         '/foo?arg1=val1&arg2=val2')
+
     def test_loginWithoutCredentialsUpdate(self):
         helper = self._makeOne()
         response = FauxCookieResponse()

src/Products/PluggableAuthService/tests/test_utils.py

@@ -275,6 +275,24 @@ def w_optional_request(foo, bar, REQUEST=None):
         self.assertEqual(wrapped(foo=None, bar=None, REQUEST=req), 42)
 
 
+class Test_utils(unittest.TestCase):
+
+    def test_url_local(self):
+        from ..utils import url_local
+
+        # Protocol and host will be removed
+        url_input = 'https://evil-site.com/exploit?arg1=val1&arg2=val2'
+        self.assertEqual(url_local(url_input), '/exploit?arg1=val1&arg2=val2')
+
+        # Site-local paths are unchanged
+        url_input = '/local/path?arg1=val1&arg2=val2'
+        self.assertEqual(url_local(url_input), url_input)
+
+        # Empty paths are unchanged
+        for url_input in ('', None):
+            self.assertEqual(url_local(url_input), url_input)
+
+
 def _createHashedValue(items):
     from hashlib import sha1 as sha
 

src/Products/PluggableAuthService/utils.py

@@ -19,6 +19,8 @@
 from hashlib import sha1
 
 import six
+from six.moves.urllib.parse import urlparse
+from six.moves.urllib.parse import urlunparse
 
 from AccessControl import ClassSecurityInfo
 # BBB import
@@ -187,3 +189,15 @@ def csrf_only(wrapped):
     exec('\n'.join(lines), g, l_copy)
 
     return functools.wraps(wrapped)(l_copy['wrapper'])
+
+
+def url_local(url):
+    """Helper to convert a URL into a site-local URL
+
+    This function removes the protocol and host parts of a URL in order to
+    prevent open redirect issues.
+    """
+    if url:
+        parsed = urlparse(url)
+        url = urlunparse(('', '') + parsed[2:])
+    return url