Skip to content

Commit

Permalink
getZMIMainFrameTarget: do not accept a 'tainted' came_from parameter.
Browse files Browse the repository at this point in the history
Do not accept a came_from path-only url starting with '//'.
Browsers can interpret the rest of the path as a domain.
  • Loading branch information
mauritsvanrees committed Sep 11, 2023
1 parent 9bc4959 commit b355046
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 3 deletions.
22 changes: 19 additions & 3 deletions src/OFS/Application.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
24 changes: 24 additions & 0 deletions src/OFS/tests/testApplication.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import unittest

from AccessControl.tainted import TaintedString
from Testing.ZopeTestCase import FunctionalTestCase


Expand Down Expand Up @@ -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),
Expand All @@ -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):

Expand Down

0 comments on commit b355046

Please sign in to comment.