[ZCM] [ZC] 1534/ 6 Comment "Serious bug in (un)restrictedTraverse"

[Collector: Zope Bugs, Features, and Patches ...]

Issue #1534 Update (Comment) "Serious bug in (un)restrictedTraverse"
 Status Pending, Zope/bug medium
To followup, visit:
  http://zope.org/Collectors/Zope/1534

==============================================================
= Comment - Entry #6 by camil7 on Nov 26, 2004 7:48 am

About the:

+ except (AttributeError,KeyError): 

please at least add "ValueError", "IndexError" 

(they may happen in cases when sequences stored 
as attributes of persistent objects are traversed)
________________________________________
= Edit - Entry #5 by chrisw on Nov 26, 2004 7:36 am

 Changes: revised title, new comment

On inspection of the code, it appears this is actually a bug in unrestrictedTraverse, and a nasty one at that. From Zope 2.7.3's Traversable.py:

Line 137 raises unauthorized if restricted and the user can't access the zodb root.

Line 141 - Line 197 then raises lots of Unauthorized's, which then get caught by the blanket except on line 198.

I put forward two patches:

--- Traversable.py      Sun Oct 24 11:39:28 2004
+++ Traversable.py.new  Fri Nov 26 12:31:51 2004
@@ -194,7 +194,7 @@

             return object

-        except ConflictError: raise
+        except (ConflictError,Unauthorized): raise
         except:
             if default==_marker: raise
             return default

...or maybe...

--- Traversable.py      Sun Oct 24 11:39:28 2004
+++ Traversable.py.new  Fri Nov 26 12:32:47 2004
@@ -194,8 +194,8 @@

             return object

-        except ConflictError: raise
-        except:
+        except (AttributeError,KeyError):
+            # should any other exceptions be caught?
             if default==_marker: raise
             return default

...which I'd prefer, unless anyone can see any problems?
________________________________________
= Edit - Entry #4 by efge on Nov 12, 2004 12:06 pm

 Changes: submitter email, edited transcript, new comment

Fixed typo in entry #3
________________________________________
= Assign - Entry #3 by efge on Nov 12, 2004 12:03 pm

 Status: Accepted => Pending

 Supporters removed: Caseman


IMO moving from unrestrictedTraverse to restrictedTraverse was a mistake, as all unrestricted code that expected that to work now fails silently. There may be security concerns, but all objects that have to be protected have adequate protections on themeselves anyway (I hope).

Of course the API was underspecified in the first place, and there should be a _getObject for restricted code to call...

________________________________________
= Comment - Entry #2 by ajung on Oct 13, 2004 4:42 am

AFAIK Casey is no longer working for ZC and I don't know if he is still working on Zope. Maybe this issue must be resolved by someone else.
________________________________________
= Request - Entry #1 by chrisw on Oct 13, 2004 4:33 am

 Status: Pending => Accepted

 Supporters added: Caseman

ZCatalog brains' getObject method is now very simply implemented, but in a way that can lead to end user confusion.

The intent is to return None for a brain that no longer maps to an object, but the impementation is such that it also returns None when the user doing the getObject isn't allowed to access the object being mapped to.

This in itself is confusing, an Unauthorized should be raised instead of None being returned.

However, more seriously, it means that unrestricted code that calls getObject no longer works since there may not be an active security manager.

FWIW, the (ugly) workaround I'm currently using is:

container.unrestrictedTraverse(brain.getPath(),None)

...in place of any brain.getObject()'s
==============================================================