zope 2.12.0b1 does not use standard_error_message

Bug #372632 reported by Jürgen Herrmann
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Zope 2
Fix Released
Medium
Sidnei da Silva

Bug Description

running zope 2.12.0b1:

standard_error_message is not used anymore, instead zope fires a hardcoded error message found in ZPublisher/HTTPResponse.py, lines 640 and following. comment states "XXX could this try to use standard_error_message somehow?". i tried to hack a little on this bug, but failed as i cannot acquire standard_error_message from within HTTPResponse. any suggestions?

Probably this also has security implications, because the hardcoded error message spits out a traceback and so possibly reveals code to the outside!

Revision history for this message
Jürgen Herrmann (xlhost) wrote :

i worked myself throught the ZPublisher code in the last hour or so. i found out that neither request nor response do have information about the traversed object. ZPublisher/Publish.py, method publish_module_standard() calls response.exception() around lines 206-214. so probably we'd have to pass in information how to get to the standard_error_message template there?

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

You're currently looking at what's happening in Zope 2.12 without any understanding of what used to happen.
Please find out how this worked in Zope 2.(9|10|11) and then find out what changed.
The code surrounding this is all old and very very nasty ;-)

Revision history for this message
Jürgen Herrmann (xlhost) wrote :

i tracked this down to OFS/SimpleItem.py, lines 240++

handle_errors is 0, because RESPONSE's attribute handle_errors is set to it's constructors kw arg debug=0 (ZPublisher/Publish.py, line 194)

ctor is None because:

(Pdb) getattr(error_type, '__init__').im_func

*** AttributeError: 'wrapper_descriptor' object has no attribute 'im_func'

(Pdb) getattr(getattr(error_type, '__init__', None), 'im_func', None)

None

if i disable the test for can_raise and handle_errors, everything looks fine here, standard_error_message is back :)

Revision history for this message
Jürgen Herrmann (xlhost) wrote :

btw this code is *new* and nasty ;)

i cannot find anything similar in zope 2.11.2

Revision history for this message
Tres Seaver (tseaver) wrote : Re: [Bug 372632] Re: zope 2.12.0b1 does not use standard_error_message

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jürgen Herrmann wrote:
> btw this code is *new* and nasty ;)
>
> i cannot find anything similar in zope 2.11.2
>

 assignee sidnei

 $ cd projects/Zope-CVS/Zope-trunk
 $ svn blame src/OFS/SimpleItem.py | xargs grep -C 10 im_func
 sidnei ....
 ....

Tres.
- --
===================================================================
Tres Seaver +1 540-429-0999 <email address hidden>
Palladion Software "Excellence by Design" http://palladion.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFKAgK1+gerLs4ltQ4RAuoFAKCQncaUhSFFtAAGur8WTE2W9mBtUwCgz2kb
b5EbLp7nBIUQp/k0RJ3AF3w=
=Of2a
-----END PGP SIGNATURE-----

Changed in zope2:
assignee: nobody → Sidnei da Silva (sidnei)
Revision history for this message
ChrisW (chris-simplistix) wrote :

Yeah, just been chatting with Sidnei about this.

The offending revision is r92767.

Apparently the change is "something to do with handling exceptions with a view. ie, if you register a view for BadRequest in your example, it will get called", but the commit message doesn't give much hint as to how the two are related.

Anyway, Sidnei has promised to fix this "just not right now." Although he's also said that if it's not fixed soon enough we're welcome to knock him on the head ;-)

Changed in zope2:
status: New → Confirmed
Revision history for this message
Sidnei da Silva (sidnei) wrote :

I believe this issue has been fixed now. Please let me know if there are further problems.

Changed in zope2:
importance: Undecided → Medium
status: Confirmed → Fix Committed
Revision history for this message
ChrisW (chris-simplistix) wrote :

Okay, just for clarification, Sidnei's fixes were:

http://zope3.pov.lt/trac/changeset/99805
http://zope3.pov.lt/trac/changeset/99806

A few comments:

- The original commits in r92767 shows no signs of unit tests being written. Why was this?

- r99806 also includes no tests. If this commit was needed, where's the test to prove it?

- Some light testing suggests the patches do fix the issue

- Zope 2 no longer support Python 2.4, so I'm not sure we need to special cases for Python 2.4

I'm setting the issue back to "In Progress" until we get more tests for the original changes and r99806.

Changed in zope2:
status: Fix Committed → In Progress
Revision history for this message
Sidnei da Silva (sidnei) wrote :

On Fri, May 8, 2009 at 4:49 AM, ChrisW <email address hidden> wrote:
> Okay, just for clarification, Sidnei's fixes were:
>
> http://zope3.pov.lt/trac/changeset/99805
> http://zope3.pov.lt/trac/changeset/99806
>
> A few comments:
>
> - The original commits in r92767 shows no signs of unit tests being
> written. Why was this?

I believe if you revert r92767 there are tests in Five that break, so
it was a fix for a newer version of Five that got integrated later.

> - r99806 also includes no tests. If this commit was needed, where's the
> test to prove it?

It's a fix for Python 2.4, the test added in 99805 passed on 2.6 but
failed on 2.4

> - Some light testing suggests the patches do fix the issue
>
> - Zope 2 no longer support Python 2.4, so I'm not sure we need to
> special cases for Python 2.4

We don't need to intentionally break it either *wink*.

> I'm setting the issue back to "In Progress" until we get more tests for
> the original changes and r99806.

I will leave it up to you to verify that you're happy with the
response and close the issue then.

--
Sidnei da Silva
Canonical Ltd.
 Landscape · Changing the way you manage your systems
http://landscape.canonical.com

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

Sidnei da Silva wrote:
>> - The original commits in r92767 shows no signs of unit tests being
>> written. Why was this?
>
> I believe if you revert r92767 there are tests in Five that break, so
> it was a fix for a newer version of Five that got integrated later.

As long as there are *some* tests that fail, that'd be better than
nothing...

Can you confirm which tests fail when you revert r92767?

>> - Zope 2 no longer support Python 2.4, so I'm not sure we need to
>> special cases for Python 2.4
>
> We don't need to intentionally break it either *wink*.

I don't think it's an intentional break, but ugly ahcks just to support
2.4 have little point as the Zope2 egg as a whole won't run under
Python2.4 anymore...

Chris

--
Simplistix - Content Management, Zope & Python Consulting
            - http://www.simplistix.co.uk

Revision history for this message
Jürgen Herrmann (xlhost) wrote :

> I don't think it's an intentional break, but ugly ahcks just to support
> 2.4 have little point as the Zope2 egg as a whole won't run under
> Python2.4 anymore...

actually i had an easy_install'd zope 2.12.0 beta running with python2.4 just fine.

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

Jürgen Herrmann wrote:
>> I don't think it's an intentional break, but ugly ahcks just to support
>> 2.4 have little point as the Zope2 egg as a whole won't run under
>> Python2.4 anymore...
>
> actually i had an easy_install'd zope 2.12.0 beta running with python2.4
> just fine.

I'll remain to be convinced that you tried out *all* the functionality
of Zope 2 with that beta.

"starting up" and "compatible with" are two very different things...

Chris

--
Simplistix - Content Management, Zope & Python Consulting
            - http://www.simplistix.co.uk

Revision history for this message
Jürgen Herrmann (xlhost) wrote :

Andreas Jung told me:
> Unsupported Python version. See
> http://docs.zope.org/zope2/releases/2.12/INSTALL.html
so i jumped forward to 2.6, followed by a big internal code cleanup. so i'll not be the person to convince you :)

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

Jürgen Herrmann wrote:
> Andreas Jung told me:
>> Unsupported Python version. See
>> http://docs.zope.org/zope2/releases/2.12/INSTALL.html
> so i jumped forward to 2.6, followed by a big internal code cleanup. so i'll not be the person to convince you :)

Indeed, and I'm likely to remove that 2.4 section before the next beta...

Chris

--
Simplistix - Content Management, Zope & Python Consulting
            - http://www.simplistix.co.uk

Revision history for this message
David Glick (davisagli) wrote :

There is still a regression in this code compared to 2.10. In 2.10, an Unauthorized exception raised while publishing an object ended up being re-raised by SimpleItem's raise_standardErrorMessage, then caught by the general exception handler in ZPublisher/Publish.py's publish_module_standard, which resulted in calling request.response.exception()...this is where PluggableAuthService's 'challenge' plugin hooks in, which is the mechanism by which, in Plone at least, accessing an unauthorized object results in a redirection to the login screen.

In Zope 2.12.0b3, raise_standardErrorMessage determines that the handle_errors flag is True, so it returns the rendered standard_error_message rather than reraising the error...so that is what the response renders and it never gets a chance to process the exception.

I don't understand the purpose of why the handle_errors flag was added in the first place, so I'm having a hard time figuring out the correct way to test and resolve this. Can anyone shed some light on it?

Revision history for this message
Tres Seaver (tseaver) wrote : Re: [zope2-tracker] [Bug 372632] Re: zope 2.12.0b1 does not use standard_error_message

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

David Glick wrote:
> There is still a regression in this code compared to 2.10. In 2.10, an
> Unauthorized exception raised while publishing an object ended up being
> re-raised by SimpleItem's raise_standardErrorMessage, then caught by the
> general exception handler in ZPublisher/Publish.py's
> publish_module_standard, which resulted in calling
> request.response.exception()...this is where PluggableAuthService's
> 'challenge' plugin hooks in, which is the mechanism by which, in Plone
> at least, accessing an unauthorized object results in a redirection to
> the login screen.
>
> In Zope 2.12.0b3, raise_standardErrorMessage determines that the
> handle_errors flag is True, so it returns the rendered
> standard_error_message rather than reraising the error...so that is what
> the response renders and it never gets a chance to process the
> exception.
>
> I don't understand the purpose of why the handle_errors flag was added
> in the first place, so I'm having a hard time figuring out the correct
> way to test and resolve this. Can anyone shed some light on it?

I think Hanno's recent changes have fixed this regression. David, can
you test against the 2.12 branch and the trunk?

Tres.
- --
===================================================================
Tres Seaver +1 540-429-0999 <email address hidden>
Palladion Software "Excellence by Design" http://palladion.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFKZIGC+gerLs4ltQ4RAuWkAKCMiXGqU/2RuPA3kcLl3IOTunemfACfQQcs
CeO8OHzUb9LYc2mLRsDvsL8=
=qsi4
-----END PGP SIGNATURE-----

Revision history for this message
David Glick (davisagli) wrote : Re: [zope2-tracker] [Bug 372632] Re: zope 2.12.0b1 does not use standard_error_message

On Jul 20, 2009, at 7:38 AM, Tres Seaver wrote:
> I think Hanno's recent changes have fixed this regression. David, can
> you test against the 2.12 branch and the trunk?

No, it's still an issue. I have been tracking the 2.12 branch (and
have no reason to think trunk would be different) as Hanno first made
the ZPublisherExceptionHook re-raise Unauthorized exceptions
immediately, then reverted that change because of it meaning that
Unauthorized exceptions wouldn't get logged to the error_log.
Following that initial change, Unauthorized exceptions were getting re-
raised and handled by the response object's exception method as
desired (though without getting recorded in the error_log). Now that
the change has been reverted, Unauthorized exceptions result in the
standard_error_message.

I worked around this issue for Plone (4.0 branch) by registering a
view of Unauthorized exceptions which simply re-raises the original
exception. This happens after the error is logged but before the
standard_error_message is raised, so resolves the issue pretty decently.

peace,
David

Revision history for this message
Simon Michael (simon) wrote :

I think I hit the same issue (described above by David) in Zwiki. Before 2.12, if a standard_error_message object in a folder rendered nothing, zope would invoke the one in the next folder up. In 2.12.0 it just stops, I had to explicitly re-raise it
(<dtml-else><dtml-with container><dtml-var standard_error_message></dtml-with> seems to work).

The new behaviour seems fine, but probably needs to be added to the release notes.

Revision history for this message
yuppie (yuppie3) wrote :

The issue mentioned in comment #17 still exists.

See also https://mail.zope.org/pipermail/zope-dev/2010-April/040084.html

and the raise_standardErrorMessage cleanups I made today:

http://svn.zope.org/?rev=110801&view=rev
http://svn.zope.org/?rev=110806&view=rev

Revision history for this message
yuppie (yuppie3) wrote :

The original issue was fixed long ago.

The issue mentioned in comments #15ff. is now also fixed:
http://svn.zope.org/?rev=110974&view=rev
http://svn.zope.org/?rev=110976&view=rev

There might still be related issues, please open a new bug if you find one.

Changed in zope2:
status: In Progress → Fix Committed
Changed in zope2:
milestone: none → 2.12.5
Changed in zope2:
status: Fix Committed → Fix Released
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.