Patrick Gerken
2005-11-13 03:35:01 UTC
Hello,
there is a Bug in ZSQLCatalog.getobject()
It first tries to find an object via unrestrictedTraverse. Then it
checks if it got an object, and it it got an object, it tries to
retrieve the object via resolve_url().
If ever unrestrictedTraverse failed where it should not have failed,
this bug would have shown itself on the radar!
But somebody must have put it in there for a reason, so I did not
delete it out of the code.
We found this bug, because our product code was not called by an
authenticated user, and authenticated itself with
AccessControl.newSecurityManager('some manager id').
resolve_url() does its own security checks by checking only the
BasicAuth data in the http header, which we never set in our product.
So in our case, which is a very uncommon one, unrestrictedTraverse
returns an object successfully. Because of the bug in the if
statement, resolve_url() tries to retrieve the object again via
ZPublisher.HTTPRequest.resolve_url(), and that one does not like us,
because we set the BasicAuth header, and instead of returning an
object pulls our god given right to be manager and throws an error.
Finding this out was REALLY hard by the way, because
ZSQLCatalog.resolve_url() ate caught all exceptions ate them and just
returned None, without giving any hint that something went wrong.
The applied patch patches the methods resolve_url() and getobject().
getobject() has no try: except: block any more. All method calls or
methods got modified so that they return None if an object could not
be found.
resolve_url() has one try: except block.
But it now only catches a NotFound exception.
Sadly, this put spotlight on a Zope Bug. There it manages to raise an
error in a raise expression, which results in an raising another
exception instead of the NotFound one. So resolve_url also catches
Exceptions unconditionally, since I was unable to find out which
exception got raised. But in this except clause I explicitly check,
that the error was caused by a missing document, and if not raise an
error.
If you are curious about the Zope bug, look here:
http://www.zope.org/Collectors/Zope/1944 . As soon as the patch is
applied, the second except clause can be deleted.
So, the patch changes the following behaviour.
If getobject() does something which raises an error, the error gets
passed to you. Trying to access a non existing object DOES NOT RAISE
AN ERROR.
if resolve_url() does something which raises an error, the error gets
passed to you. Trying to access a non existing object DOES NOT RAISE
AN ERROR.
I believe, that the calling resolve_url() in getobject() is absolutely
unnecessary, and propose to kick it out of the code. If it would be
needed some times, people would have complained, because they got a
None instead of the object.
But maybe, someone can show, in which case unrestrictedTraverse does
not return an object, but should, and resolve_url() does return such
an object.
No unit tests were added, No unit test was run, I just tested the code
with the debugger. If somebody can provide the scripts to run the unit
tests of ZSQLCatalog out of the box I will happily provide a unittest.
To apply the attached patch, enter the directory where your
ZSQLCatalog.py file resides, and enter the following:
patch ZSQLCatalog.py < /some/path/ZSQLCatalog.py.patch
If I made myself too unclear in what the problem is, please forgive me.
I will use the late time as a lame excuse about the time I am writing
it and explain myself better if it is needed ;-)
Patrick
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ZSQLCatalog.py.patch
Type: application/octet-stream
Size: 2685 bytes
Desc: not available
URL: <http://mail.tiolive.com/pipermail/erp5-dev/attachments/20051113/e8cab5cb/attachment.obj>
there is a Bug in ZSQLCatalog.getobject()
It first tries to find an object via unrestrictedTraverse. Then it
checks if it got an object, and it it got an object, it tries to
retrieve the object via resolve_url().
If ever unrestrictedTraverse failed where it should not have failed,
this bug would have shown itself on the radar!
But somebody must have put it in there for a reason, so I did not
delete it out of the code.
We found this bug, because our product code was not called by an
authenticated user, and authenticated itself with
AccessControl.newSecurityManager('some manager id').
resolve_url() does its own security checks by checking only the
BasicAuth data in the http header, which we never set in our product.
So in our case, which is a very uncommon one, unrestrictedTraverse
returns an object successfully. Because of the bug in the if
statement, resolve_url() tries to retrieve the object again via
ZPublisher.HTTPRequest.resolve_url(), and that one does not like us,
because we set the BasicAuth header, and instead of returning an
object pulls our god given right to be manager and throws an error.
Finding this out was REALLY hard by the way, because
ZSQLCatalog.resolve_url() ate caught all exceptions ate them and just
returned None, without giving any hint that something went wrong.
The applied patch patches the methods resolve_url() and getobject().
getobject() has no try: except: block any more. All method calls or
methods got modified so that they return None if an object could not
be found.
resolve_url() has one try: except block.
But it now only catches a NotFound exception.
Sadly, this put spotlight on a Zope Bug. There it manages to raise an
error in a raise expression, which results in an raising another
exception instead of the NotFound one. So resolve_url also catches
Exceptions unconditionally, since I was unable to find out which
exception got raised. But in this except clause I explicitly check,
that the error was caused by a missing document, and if not raise an
error.
If you are curious about the Zope bug, look here:
http://www.zope.org/Collectors/Zope/1944 . As soon as the patch is
applied, the second except clause can be deleted.
So, the patch changes the following behaviour.
If getobject() does something which raises an error, the error gets
passed to you. Trying to access a non existing object DOES NOT RAISE
AN ERROR.
if resolve_url() does something which raises an error, the error gets
passed to you. Trying to access a non existing object DOES NOT RAISE
AN ERROR.
I believe, that the calling resolve_url() in getobject() is absolutely
unnecessary, and propose to kick it out of the code. If it would be
needed some times, people would have complained, because they got a
None instead of the object.
But maybe, someone can show, in which case unrestrictedTraverse does
not return an object, but should, and resolve_url() does return such
an object.
No unit tests were added, No unit test was run, I just tested the code
with the debugger. If somebody can provide the scripts to run the unit
tests of ZSQLCatalog out of the box I will happily provide a unittest.
To apply the attached patch, enter the directory where your
ZSQLCatalog.py file resides, and enter the following:
patch ZSQLCatalog.py < /some/path/ZSQLCatalog.py.patch
If I made myself too unclear in what the problem is, please forgive me.
I will use the late time as a lame excuse about the time I am writing
it and explain myself better if it is needed ;-)
Patrick
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ZSQLCatalog.py.patch
Type: application/octet-stream
Size: 2685 bytes
Desc: not available
URL: <http://mail.tiolive.com/pipermail/erp5-dev/attachments/20051113/e8cab5cb/attachment.obj>