Serious bug in (un)restrictedTraverse

Bug #143190 reported by ChrisW
2
Affects Status Importance Assigned to Milestone
Zope 2
Invalid
Medium
Unassigned

Bug Description

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

Tags: bug zope
Revision history for this message
Andreas Jung (ajung) wrote :

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.

Revision history for this message
Florent Guillaume (efge) wrote :

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...

Revision history for this message
Florent Guillaume (efge) wrote :

Changes: submitter email, edited transcript, new comment

Fixed typo in entry #3

Revision history for this message
ChrisW (chris-simplistix) wrote :

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?

Revision history for this message
Clemens Robbenhaar (crobbenhaar) wrote :

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)

Revision history for this message
Florent Guillaume (efge) wrote :

This will probably be fixed if bug 1713 is fixed.
http://www.zope.org/Collectors/Zope/1713

Revision history for this message
ChrisW (chris-simplistix) wrote :

Are you going to fix Traversable.py as well?
That's the problem this issue is talking about...

Revision history for this message
Tres Seaver (tseaver) wrote :

We need failing tests which the proposed patches fix. Also, attaching the patch as a file will help get it attention in Launchpad.

Changed in zope2:
status: New → Triaged
Revision history for this message
Colin Watson (cjwatson) wrote :

The zope2 project on Launchpad has been archived at the request of the Zope developers (see https://answers.launchpad.net/launchpad/+question/683589 and https://answers.launchpad.net/launchpad/+question/685285). If this bug is still relevant, please refile it at https://github.com/zopefoundation/zope2.

Changed in zope2:
status: Triaged → Invalid
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.