getZMIMainFrameTarget: do not accept a 'tainted' came_from parameter. (…

…#1152)

Do not accept a came_from path-only url starting with '//' either.
Browsers can interpret the rest of the path as a domain.

src/OFS/Application.py

@@ -24,6 +24,7 @@
 from AccessControl.class_init import InitializeClass
 from AccessControl.Permission import ApplicationDefaultPermissions
 from AccessControl.Permissions import view_management_screens
+from AccessControl.tainted import TaintedString
 from Acquisition import aq_base
 from App import FactoryDispatcher
 from App.ApplicationManager import ApplicationManager
@@ -122,15 +123,30 @@ def getZMIMainFrameTarget(self, REQUEST):
         if not came_from:
             return default
 
+        # When came_from contains suspicious code, it will not be a string,
+        # but an instance of AccessControl.tainted.TaintedString.
+        # Passing this to urlparse, gives:
+        # AttributeError: 'str' object has no attribute 'decode'
+        # This is good, but let's check explicitly.
+        if isinstance(came_from, TaintedString):
+            return default
+        try:
+            parsed_came_from = urlparse(came_from)
+        except AttributeError:
+            return default
         parsed_parent_url = urlparse(parent_url)
-        parsed_came_from = urlparse(came_from)
 
         # Only allow a passed-in ``came_from`` URL if it is local (just a path)
         # or if the URL scheme and hostname are the same as our own
-        if (not parsed_came_from.scheme and not parsed_came_from.netloc) or \
-           (parsed_parent_url.scheme == parsed_came_from.scheme
+        if (parsed_parent_url.scheme == parsed_came_from.scheme
                 and parsed_parent_url.netloc == parsed_came_from.netloc):
             return came_from
+        if (not parsed_came_from.scheme and not parsed_came_from.netloc):
+            # This is only a path.  But some paths can be misinterpreted
+            # by browsers.
+            if parsed_came_from.path.startswith("//"):
+                return default
+            return came_from
 
         return default
 

src/OFS/tests/testApplication.py

@@ -1,5 +1,6 @@
 import unittest
 
+from AccessControl.tainted import TaintedString
 from Testing.ZopeTestCase import FunctionalTestCase
 
 
@@ -145,6 +146,12 @@ def test_getZMIMainFrameTarget(self):
             self.assertEqual(app.getZMIMainFrameTarget(request),
                              '/new/path')
 
+            # Tainted local path.  came_from can be marked as 'tainted' if it
+            # suspicious contents.  It is not accepted then.
+            request['came_from'] = TaintedString('/new/path')
+            self.assertEqual(app.getZMIMainFrameTarget(request),
+                             f'{URL1}/manage_workspace')
+
             # came_from URL outside our own server
             request['came_from'] = 'https://www.zope.dev/index.html'
             self.assertEqual(app.getZMIMainFrameTarget(request),
@@ -160,6 +167,23 @@ def test_getZMIMainFrameTarget(self):
             self.assertEqual(app.getZMIMainFrameTarget(request),
                              f'{URL1}/added/path')
 
+            # Anything beginning with '<script>' should already be marked as
+            # 'tainted'
+            request['came_from'] = TaintedString(
+                '<script>alert("hi");</script>'
+            )
+            self.assertEqual(app.getZMIMainFrameTarget(request),
+                             f'{URL1}/manage_workspace')
+
+            # double slashes as path should not be accepted.
+            # Try a few forms.
+            request['came_from'] = '//www.example.org'
+            self.assertEqual(app.getZMIMainFrameTarget(request),
+                             f'{URL1}/manage_workspace')
+            request['came_from'] = '////www.example.org'
+            self.assertEqual(app.getZMIMainFrameTarget(request),
+                             f'{URL1}/manage_workspace')
+
 
 class ApplicationPublishTests(FunctionalTestCase):