Commit 850e436e authored by Vincent Pelletier's avatar Vincent Pelletier

Products.ERP5Form.Form: Let NotFound exceptions propagate during rendering

Allows triggering the regular not-found error handling path from within
the rendering of a form.
parent 5f259059
...@@ -48,7 +48,7 @@ from Products.ERP5Type.Globals import DTMLFile, get_request ...@@ -48,7 +48,7 @@ from Products.ERP5Type.Globals import DTMLFile, get_request
from AccessControl import Unauthorized, ClassSecurityInfo from AccessControl import Unauthorized, ClassSecurityInfo
from DateTime import DateTime from DateTime import DateTime
from ZODB.POSException import ConflictError from ZODB.POSException import ConflictError
from zExceptions import Redirect from zExceptions import Redirect, NotFound
from Acquisition import aq_base, aq_get from Acquisition import aq_base, aq_get
from Products.PageTemplates.Expressions import SecureModuleImporter from Products.PageTemplates.Expressions import SecureModuleImporter
from zExceptions import Forbidden from zExceptions import Forbidden
...@@ -222,7 +222,7 @@ class TALESValue(StaticValue): ...@@ -222,7 +222,7 @@ class TALESValue(StaticValue):
kw['CONTEXTS'] = kw kw['CONTEXTS'] = kw
try: try:
value = self.tales_expr.__of__(field)(**kw) value = self.tales_expr.__of__(field)(**kw)
except (ConflictError, RuntimeError, Redirect): except (ConflictError, RuntimeError, Redirect, NotFound):
raise raise
except: except:
# We add this safety exception to make sure we always get # We add this safety exception to make sure we always get
......
  • This caused a failure in Zope4, because since Zope4, Unauthorized during path traversal are converted to NotFound ( https://github.com/zopefoundation/Zope/blob/4.8.7/src/Products/PageTemplates/Expressions.py#L110-L112 ).

    I did not check exactly what "path traversal" covers but requests to https://erp5/foo_module as anonymous user redirect to login page when not logged in and show a "Site Error: unauthorized" when already logged in.

    The failure happens with a listbox editable cell using cell/getId for TALES default, which is a path expression and path traversal happens. The failing test is a zelenium test, portal_tests/renderjs_ui_relation_field_zuite/testAccessUnauthorizedRelationValue, on this test there is a document where user has View permission but not Access permission, so the cell/getId cause an Unauthorized on Zope2 and on Zope4 it causes an NotFound which is not catched and end up with a site error page.

    I'm certainly not writing this to report something wrong with this commit and actually I have been thinking that we should also let Unauthorized propagate here. Some time ago, we had cases of users entering some forms in URL and seeing some things that they are not supposed to see, some field of the form have unauthorized as expected but some others do not and information is leaked, which is a problem that would not happen if we were re-raising Unauthorized here.

    I just wanted to say that this might have more impact than initially expected, but probably the impact is positive anyway.

    @arnau for this Zope4 failure, I'm changing the TALES to cell/getId | nothing ( this is a "try: except: return None" in TALES path syntax).

  • mentioned in commit fc17c19b

    Toggle commit list
  • mentioned in commit 5c40e95a

    Toggle commit list
  • mentioned in commit 38fbf5b8

    Toggle commit list
  • mentioned in merge request !1545 (merged)

    Toggle commit list
  • Personally I would recommend to stop using paths in TALES expressions (independently of this issue), because they are just slower (ex: handling the duality getitem/getattr). This is yet another example of how this behaves differently from python: expressions and python scripts.

    But I do realize that there are so many everywhere that we will never get rid of all.

    There is one special case where I do not know of a python: equivalent: the | exception catching mechanism. But maybe such cases would be better served by moving to a python script ? (more readable, finer exception filtering support)

    I'm changing the TALES to cell/getId | nothing

    So, what about python: cell.getId() ?

    EDIT:

    actually I have been thinking that we should also let Unauthorized propagate here

    +1, and IMHO this requires handling the Unauthorized within the relation string field rather than relying (as is the case in Zope2) on the exception being caught somewhere up the stack and "the right thing" being done for rendering the field.

    Edited by Vincent Pelletier
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment