Skip to content

Commit

Permalink
Make ObjectManager's get and __getitem__ return only "items".
Browse files Browse the repository at this point in the history
No longer return attributes / methods from the class or from acquisition.
Thanks to Richard Mitchell at Netsight for the report.
  • Loading branch information
tseaver committed Feb 14, 2012
1 parent 3950231 commit 8c5288f
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 6 deletions.
4 changes: 4 additions & 0 deletions doc/CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ http://docs.zope.org/zope2/releases/.
Bugs Fixed
++++++++++

- Ensure that ObjectManager's ``get`` and ``__getitem__`` methods return only
"items" (no attributes / methods from the class or from acquisition).
Thanks to Richard Mitchell at Netsight for the report.

- Removed HTML tags from exception text of ``Unauthorized`` exception
because these tags get escaped since CVE-2010-1104 (see 2.13.12) got
fixed.
Expand Down
16 changes: 10 additions & 6 deletions src/OFS/ObjectManager.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import re
import sys
import time
from types import NoneType

from AccessControl import ClassSecurityInfo
from AccessControl.class_init import InitializeClass
Expand Down Expand Up @@ -757,12 +758,13 @@ def __delitem__(self, name):
return self.manage_delObjects(ids=[name])

def __getitem__(self, key):
v=self._getOb(key, None)
if v is not None: return v
if hasattr(self, 'REQUEST'):
request=self.REQUEST
if key in self:

This comment has been minimized.

Copy link
@malthe

malthe Jun 29, 2016

Contributor

This is pretty inefficient because of how __contains__ is currently implemented.

It would be good to fix __contains__ so that it doesn't have to do so much work. It currently pulls all ids out and then checks for an occurances in that list:

    def __contains__(self, name):
        return name in self.objectIds()

    def objectIds(self, spec=None):
        # Returns a list of subobject ids of the current object.
        # If 'spec' is specified, returns objects whose meta_type
        # matches 'spec'.
        if spec is not None:
            if type(spec) is str:
                spec=[spec]
            set=[]
            for ob in self._objects:
                if ob['meta_type'] in spec:
                    set.append(ob['id'])
            return set
        return [ o['id']  for o in self._objects ]

This comment has been minimized.

Copy link
@tseaver

tseaver Jun 29, 2016

Author Member

Sure -- of course, using OFS.Folder for any container big enough to notice the inefficiency is already contra-indicated. :)

This comment has been minimized.

Copy link
@davisagli

davisagli Jun 29, 2016

Member

Right. BTreeFolder2Base in Products.BTreeFolder2 overrides __contains__:

    def __contains__(self, name):
        return name in self._tree
return self._getOb(key, None)
request = getattr(self, 'REQUEST', None)
if not isinstance(request, (str, NoneType)):
method=request.get('REQUEST_METHOD', 'GET')
if request.maybe_webdav_client and not method in ('GET', 'POST'):
if (request.maybe_webdav_client and
method not in ('GET', 'POST')):
return NullResource(self, key, request).__of__(self)
raise KeyError, key

Expand All @@ -783,7 +785,9 @@ def __nonzero__(self):

security.declareProtected(access_contents_information, 'get')
def get(self, key, default=None):
return self._getOb(key, default)
if key in self:
return self._getOb(key, default)
return default

security.declareProtected(access_contents_information, 'keys')
def keys(self):
Expand Down
1 change: 1 addition & 0 deletions src/OFS/tests/testApplication.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ def test___bobo_traverse__attribute_hit(self):
def test___bobo_traverse__attribute_miss_key_hit(self):
app = self._makeOne()
app._getOb = lambda x, y: x
app._objects = [{'id': 'OTHER', 'meta_type': None}]
request = {}
self.assertEqual(app.__bobo_traverse__(request, 'OTHER'), 'OTHER')

Expand Down
20 changes: 20 additions & 0 deletions src/OFS/tests/testObjectManager.py
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,22 @@ def test_nonzero(self):
om = self._makeOne()
self.assertTrue(om)

def test___getitem___miss(self):
om = self._makeOne()
self.assertRaises(KeyError, om.__getitem__, 'nonesuch')

def test___getitem___miss_w_non_instance_attr(self):
om = self._makeOne()
self.assertRaises(KeyError, om.__getitem__, 'get')

def test___getitem___hit(self):
om = self._makeOne()
si1 = SimpleItem('1')
om['1'] = si1
got = om['1']
self.assertTrue(got.aq_self is si1)
self.assertTrue(got.aq_parent is om)

def test_get_miss_wo_default(self):
om = self._makeOne()
self.assertEqual(om.get('nonesuch'), None)
Expand All @@ -421,6 +437,10 @@ def test_get_miss_w_default(self):
obj = object()
self.assertTrue(om.get('nonesuch', obj) is obj)

def test_get_miss_w_non_instance_attr(self):
om = self._makeOne()
self.assertEqual(om.get('get'), None)

def test_get_hit(self):
om = self._makeOne()
si1 = SimpleItem('1')
Expand Down

0 comments on commit 8c5288f

Please sign in to comment.