[Zope-dev] Traversal issue which affects Five

[Alec Mitchell]

Hi all,

It seems that the way OFS.Traversable.restrictedTraverse() handles
security checking on objects with __bobo_traverse__ methods is
considerably different from the way it normally checks security.  The
result is that traversal cannot obtain attributes using acquisition
from objects that are marked <five:traversable>.  In the normal case,
security is checked using guarded_getattr, which gets an attribute and
checks the permissions on the retrieved object in its original
context.  For __bobo_traverse__methods which return simple properties
(say strings), it is impossible to determine the container from which
the returned attribute originates, so unless the attribute was not
acquired, an Unauthorized error will always be raised.

Objects that are Five Traversable always have __bobo_traverse__
methods which attempt to mimic normal traversal, which effectively
means that the security checks end up preventing acquisition of simple
properties using traversal from ever working on these objects (say
using a TAL path expression 'context/attribute' which you expect to be
acquired).  Unfortunately, because Five has no control over the
security checks done during traversal, this cannot be fixed directly
in Five.  However, IMHO fixing this makes sense for Zope itself,
provided there aren't any undesirable consequences.  I propose that if
the validation of a __bobo_traverse result raises Unauthorized, that
we make one last check to see if the result o 'guarded_getattr(obj,
name)' is identical to the result of the __bobo_traverse__ call and
allow it if that's the case.  Here is my proposed patch against Zope
2.9 trunk:

--- Traversable.py      (revision 66323)
+++ Traversable.py      (working copy)
@@ -201,9 +201,18 @@
                         else:
                             # Can't determine container
                             container = _none
-                        if not securityManager.validate(
-                            obj, container, name, next):
-                            raise Unauthorized, name
+                        try:
+                            if not securityManager.validate(
+                                    obj, container, name, next):
+                                raise Unauthorized, name
+                        except Unauthorized:
+                            # If next is a simple unwrapped property, it's
+                            # parentage is indeterminate, but it may have been
+                            # acquired safely.  In this case validate
will raise
+                            # an error, and we can check that our value was
+                            # acquired safely.
+                            if guarded_getattr(obj, name, marker) is not next:
+                                raise Unauthorized, name
                 else:
                     if restricted:
                         next = guarded_getattr(obj, name, marker)

At the moment Plone 2.5 is really struggling with this issue, and it
would be wonderful if a fix for this could go into Zope 2.8 and 2.9
soon.  I'm happy to write tests for this, I just want to make sure
that I'm not proposing something really wrong/inappropriate here. 
Generally, the validate() call should return a True or False value, so
this change should have little performance impact except in the case
where 'container == _none' and validate would otherwise raise a very
unhelpful unauthorized error.  Your feedback is much appreciated.

Alec Mitchell