zopefoundation / Zope

Zope is an open-source web application server.
https://zope.readthedocs.io
Other
352 stars 99 forks source link

Cookie path validation check is incomplete and leads to errors #1052

Closed wohnlice closed 2 years ago

wohnlice commented 2 years ago

BUG/PROBLEM REPORT (OR OTHER COMMON ISSUE)

What I did:

Originally reported here as I thought this was an issue with plone.app.drafts/plone.app.mosaic. https://github.com/plone/plone.app.drafts/issues/13

Create a Plone (5.2.8) site with plone.app.mosaic. Navigate to the add new News Item page e.g. "/Plone/++add++News Item". Toggle focus on a field to trigger the XHR @@z3cform_validate_field

What I expect to happen:

@@z3cform_validate_field returns with HTTP code 200

What actually happened:

@@z3cform_validate_field returns with HTTP code 500

2022-07-26 14:25:15,994 ERROR   [waitress:426][waitress-0] Exception while serving /Plone/need-to-know/++add++News Item/@@z3cform_validate_field
Traceback (most recent call last):
  File "c:\users\wohnlice\plone\eggs\waitress-2.1.1-py3.8.egg\waitress\channel.py", line 426, in service
    task.service()
  File "c:\users\wohnlice\plone\eggs\waitress-2.1.1-py3.8.egg\waitress\task.py", line 168, in service
    self.execute()
  File "c:\users\wohnlice\plone\eggs\waitress-2.1.1-py3.8.egg\waitress\task.py", line 434, in execute
    app_iter = self.channel.server.application(environ, start_response)
  File "c:\users\wohnlice\plone\eggs\paste-3.5.0-py3.8.egg\paste\translogger.py", line 69, in __call__
    return self.application(environ, replacement_start_response)
  File "c:\users\wohnlice\plone\eggs\zope-4.8.1-py3.8.egg\ZPublisher\httpexceptions.py", line 30, in __call__
    return self.application(environ, start_response)
  File "c:\users\wohnlice\plone\eggs\zope-4.8.1-py3.8.egg\ZPublisher\WSGIPublisher.py", line 371, in publish_module
    response = _publish(request, new_mod_info)
  File "c:\users\wohnlice\plone\eggs\zope-4.8.1-py3.8.egg\ZPublisher\HTTPResponse.py", line 1060, in finalize
    return '%s %s' % (self.status, self.errmsg), self.listHeaders()
  File "c:\users\wohnlice\plone\eggs\zope-4.8.1-py3.8.egg\ZPublisher\HTTPResponse.py", line 1068, in listHeaders
    result.extend(super(WSGIResponse, self).listHeaders())
  File "c:\users\wohnlice\plone\eggs\zope-4.8.1-py3.8.egg\ZPublisher\HTTPResponse.py", line 725, in listHeaders
    result.extend(self._cookie_list())
  File "c:\users\wohnlice\plone\eggs\zope-4.8.1-py3.8.egg\ZPublisher\HTTPResponse.py", line 694, in _cookie_list
    cookie = "%s=%s" % (name, dump(name, attrs["value"]))
KeyError: 'value'

Process finished with exit code 0

What version of Python and Zope/Addons I am using:

Plone 5.2.8, plone.app.mosaic 2.2.3

The issue stems from this regex and the following function: https://github.com/zopefoundation/Zope/blame/4.8.1/src/ZPublisher/cookie.py#L254. The path_safe regex does not allow for valid characters like "-" and "+", possibly others. So what happens is this function is passed an already quoted path value that has one or more of "-" and "+", in addition to a space already quoted as "%20". Because "+" isn't caught by the regex, path_safe finds no match. But because it does have "%", path_converter function raises a ValueError. The end result is ZPublisher creates an empty cookie value and @@z3cform_validate_field fails to parse this cookie properly.

To me this does not look like an issue with either plone.app.z3cform or plone.app.drafts (by way of plone.app.mosaic) but is rather an issue with ZPublisher doing some weird stuff. One solution would be to improve that regex. But if the goal is simply to avoid quoting an already quoted value, perhaps the regex should be ditched in favor of something like the following:

if unquote(value) == value:
  return quote(value)
return value

This handles cases where value is either quoted or not, but doesn't delve into the mechanics of what quote/unquote does.

d-maurer commented 2 years ago

Would you check #1053 to verify that it resolves this issue?

wohnlice commented 2 years ago

I tested #1053 and it does resolve this issue.

d-maurer commented 2 years ago

wohnlice wrote at 2022-7-27 07:52 -0700:

I tested #1053 and it does resolve this issue.

Thank you for your bug report and your help to resolve it!

d-maurer commented 2 years ago

Fixed via #1053 and #1054.