Merge pull request #2233 from plone/plone-hotfix20171128-isURLInPorta…

…l-4.3 Improved isURLInPortal according to PloneHotfix20171128. [4.3]
plone · Dec 1, 2017 · 7db5b2c · 7db5b2c
2 parents 4ac76b7 + 37981ae
commit 7db5b2c
Show file tree

Hide file tree

Showing 3 changed files with 77 additions and 8 deletions.
diff --git a/Products/CMFPlone/URLTool.py b/Products/CMFPlone/URLTool.py
@@ -1,3 +1,4 @@
+from HTMLParser import HTMLParser
 from Products.CMFCore.URLTool import URLTool as BaseTool
 from Products.CMFCore.utils import getToolByName
 from AccessControl import ClassSecurityInfo
@@ -9,6 +10,25 @@
 import re
 
 
+hp = HTMLParser()
+# These schemas are allowed in full urls to consider them in the portal:
+# A mailto schema is an obvious sign of a url that is not in the portal.
+# This is a whitelist.
+ALLOWED_SCHEMAS = [
+    'https',
+    'http',
+]
+# These bad parts are not allowed in urls that are in the portal:
+# This is a blacklist.
+BAD_URL_PARTS = [
+    '\\\\',
+    '<script',
+    '%3cscript',
+    'javascript:',
+    'javascript%3a',
+]
+
+
 class URLTool(PloneBaseTool, BaseTool):
 
     meta_type = 'Plone URL Tool'
@@ -31,16 +51,24 @@ def isURLInPortal(self, url, context=None):
         # sanitize url
         url = re.sub('^[\x00-\x20]+', '', url).strip()
         cmp_url = url.lower()
-        if ('\\\\' in cmp_url or
-                '<script' in cmp_url or
-                '%3cscript' in cmp_url or
-                'javascript:' in cmp_url or
-                'javascript%3a' in cmp_url):
-            return False
+        for bad in BAD_URL_PARTS:
+            if bad in cmp_url:
+                return False
 
         p_url = self()
 
-        _, u_host, u_path, _, _, _ = urlparse(url)
+        schema, u_host, u_path, _, _, _ = urlparse(url)
+        if schema and schema not in ALLOWED_SCHEMAS:
+            # Redirecting to 'data:' may be harmful,
+            # and redirecting to 'mailto:' or 'ftp:' is silly.
+            return False
+
+        # Someone may be doing tricks with escaped html code.
+        unescaped_url = hp.unescape(url)
+        if unescaped_url != url:
+            if not self.isURLInPortal(unescaped_url):
+                return False
+
         if not u_host and not u_path.startswith('/'):
             if context is None:
                 return True  # old behavior

diff --git a/Products/CMFPlone/tests/testURLTool.py b/Products/CMFPlone/tests/testURLTool.py
@@ -124,3 +124,43 @@ def test_double_back_slash(self):
         url_tool = self._makeOne()
         iURLiP = url_tool.isURLInPortal
         self.assertFalse(iURLiP('\\\\www.example.com'))
+
+    def test_regression_absolute_url_in_portal(self):
+        url_tool = self._makeOne()
+        iURLiP = url_tool.isURLInPortal
+        self.assertTrue(iURLiP(url_tool()))
+        self.assertTrue(iURLiP(url_tool() + '/shrubbery?knights=ni#ekki-ekki'))
+
+    def test_mailto_simple_not_in_portal(self):
+        url_tool = self._makeOne()
+        iURLiP = url_tool.isURLInPortal
+        self.assertFalse(iURLiP(
+            'mailto:someone@example.org')
+        )
+
+    def test_mailto_complex_not_in_portal(self):
+        url_tool = self._makeOne()
+        iURLiP = url_tool.isURLInPortal
+        self.assertFalse(iURLiP(
+            'mailto&#58;192&#46;168&#46;163&#46;154&#58;8080&#47;Plone&apos;'
+            '&quot;&gt;&lt;html&gt;&lt;svg&#32;onload&#61;alert&#40;document'
+            '&#46;domain&#41;&gt;&lt;&#47;html&gt;')
+        )
+
+    def test_data_not_in_portal(self):
+        url_tool = self._makeOne()
+        iURLiP = url_tool.isURLInPortal
+        self.assertFalse(iURLiP(
+            'data:text/html%3bbase64,PHNjcmlwdD5hbGVydCgnWFNTJyk8L3NjcmlwdD4K')
+        )
+
+    def test_double_slash(self):
+        # I wondered if this might be a problem after reading
+        # https://bugs.python.org/issue23505
+        # Apparently not, but let's test it.
+        url_tool = self._makeOne()
+        iURLiP = url_tool.isURLInPortal
+        self.assertFalse(iURLiP(
+            '//www.google.com'))
+        self.assertFalse(iURLiP(
+            '////www.google.com'))
diff --git a/docs/CHANGES.rst b/docs/CHANGES.rst
@@ -19,7 +19,8 @@ New features:
 
 Bug fixes:
 
-- *add item here*
+- Improved isURLInPortal according to PloneHotfix20171128.
+  Accept only http/https, and doubly check escaped urls.  [maurits]
 
 
 4.3.16 (2017-09-08)