Zope4: cookie value is no longer quoted.

bt5/erp5_oauth2_authorisation/TestTemplateItem/portal_components/test.erp5.testOAuth2Server.py

@@ -420,8 +420,10 @@ class TestOAuth2(ERP5TypeTestCase):
           if key == 'set-cookie':
             # XXX: minimal Set-Cookie parser
             cookie_name, cookie_body = value.split('=', 1)
-            assert cookie_body[0] == '"', repr(cookie_body)
-            cookie_value, cookie_attributes = cookie_body[1:].split('"', 1)
+            # RFC6265 makes quoting obsolete
+            # assert cookie_body[0] == '"', repr(cookie_body)
+            cookie_value, cookie_attributes = cookie_body.split(';', 1)
+            cookie_value = cookie_value.strip('"')
             cookie_value_dict = {
               'value': urllib.unquote(cookie_value),
             }

[Vincent Pelletier]

Please remove the old line instead of commenting it out.

The reference to the RFC can be moved in to the XXX comment above, to specify that this parser is targeting this version of the RFC.

After these minor changes, I'm ok for inclusion of this change in master (...assuming it does not break anything).

Side note: I notice there is a nice description of the algorithm for correctly parsing set-cookie value, and I am not following it... Gah. Anyway I do not think it needs to be RFC-perfect: Zope should not produce malformed cookies, so the test should not have to be too careful.

[Kazuhiko Shiozaki]

oops, thx for your review. I should delete the old line...

anyway this test still fails on zope4py2 branch even with this change. we should investigate further.

[Jérome Perrin]

I don't think we can have this exact change in master already, because this change only supports the "new style zope cookies" but with Zope 2 the cookies are still quoted.

I have tried a little bit as well and I was thinking maybe the part where we remove cookies in https://lab.nexedi.com/nexedi/erp5/blob/88ee0cb9990da6fd2769eb16ef63862d8f1a7f66/product/ERP5Security/ERP5OAuth2ResourceServerPlugin.py#L536-544 needs to be adjusted as well ( but in https://lab.nexedi.com/nexedi/erp5/blob/88ee0cb9990da6fd2769eb16ef63862d8f1a7f66/product/ERP5Security/ERP5OAuth2ResourceServerPlugin.py#L146 we seem to handle the non quoted case, so maybe not ).

Maybe we could use a library for the cookie handling. My guess was that we were not using https://docs.python.org/2.7/library/cookie.html#module-Cookie because it does not support the "SameSite" attribute on python 2. Maybe https://werkzeug.palletsprojects.com/en/latest/http/#cookies can be used.

[Vincent Pelletier]

anyway this test still fails on zope4py2 branch even with this change. we should investigate further.

Do you have a link to a test result so I can check ?

This test is very verbose when failing, because there are several values which are generated at the very start and only checked at the very end. So it looks scary, but in my experience this was necessary to have a chance to debug failures.

because this change only supports the "new style zope cookies" but with Zope 2 the cookies are still quoted.

Does it ? I see that it is stripping quotes, which is what the original code was doing. Only, now the quotes are optional. I was initially worried about strip doing too much, but the RFC says that DQUOTE are not allowed as part of the cookie value.

So, to me this change can and should be upstreamed, minus the commented-out code line, and ideally the RFC reference merged into the XXX comment a bit higher (the reference to quotes being obsolete is surprising if one does not check the commit history, so I think it is unnecessary).

the part where we remove cookies in needs to be adjusted as well

While I cannot tell at a glance if this is necessary, beware: this part processes data received from the browser. Maybe the browser could add quotes that Zope did not generate, so all syntaxes should be supported. Which is not to say that the current code supposed to handle all syntaxes is perfect...

Maybe we could use a library for the cookie handling.

Fine for me. The less code duplication, the better. As for which library to use, I do not have a concrete preference. Something which is heavily used, maintained by a large reputable group and not modified often (already stable) may be my criterion. What is Zope4 using ? Is it reusable ?

My guess was that we were not using cookie because it does not support the "SameSite" attribute on python 2.

Frankly, I do not remember why. Lack of SameSite support is definitely a blocker, so it is at least part of the reason.

[Jérome Perrin]

Thanks for feedback @vpelletier

Do you have a link to a test result so I can check ?

There's a failing test result at https://erp5js.nexedi.net/#/test_result_module/20221020-C64B563/177

the zope change which I suspect to have introduced the failure is https://github.com/zopefoundation/Zope/commit/12c78aa402716611537c10e5f7d24b039705a01e

because this change only supports the "new style zope cookies" but with Zope 2 the cookies are still quoted.

Does it ?

sorry ! it should support the old style cookies as well, I did not notice the

            cookie_value = cookie_value.strip('"')

so it should be OK. I pushed the commit (as is, without the extra changes we are discussing) on testrunner branch to confirm that ( https://lab.nexedi.com/nexedi/erp5/pipelines/24407 )

If we can find a way to make it work without rewriting parts with a cookie library it's probably easier, but for reference:

• Werkzeug is used by Flask (and maintained by Flask team). It's in ERP5 stack because Flask is a dependency of slapos.core ( and slapos.toolbox ).
• Zope seems to use https://github.com/zopefoundation/Zope/blob/68f0c122cf938c3e6184185cd8519e26ff7d0b14/src/ZPublisher/HTTPRequest.py#L1729-L1776 to parse the Cookie request header and https://github.com/zopefoundation/Zope/blob/68f0c122cf938c3e6184185cd8519e26ff7d0b14/src/ZPublisher/HTTPResponse.py#L696-L711 to serialize to the Set-Cookie response header.

[Vincent Pelletier]

Werkzeug is used by Flask (and maintained by Flask team). It's in ERP5 stack because Flask is a dependency of slapos.core ( and slapos.toolbox ).

How robust is flask nowadays ? IIRC the history was (as often):

• new framework appears, it is lightweight and fast, and advertised for proof-of-concept kind of use
• it gets popular, so it gets used way beyond its original scope, which makes it more popular, etc

Is it getting feature creep ? Is it keeping true to its original scope ? Or is it avoiding pitfalls and evolving into something robust enough to be exposed to the bad wide web ? I have totally not followed its evolution.

BTW, I just remembered a thing about the cookie parsing in ERP5OAuth2ResourceServerPlugin.__extractCookie: the biggest unusual thing in this code is that it has to remove the cookie from the request header, so that its value does not get leaked (ex: in error_log). So it:

• works on the whole raw header (request's environment)
• should be very careful to not alter any of the other cookies the header may contain, so I did not want to deserialise everything and then re-serialise all but one, as this may not be a perfectly bijective operation (minus the dropped cookie)

This will impose unusual requirements on a library, making the choice difficult.

[Jérome Perrin]

According to https://piptrends.com/top-packages werkzeug is in the top 25 of downloaded packages from pypi yesterday, so it's still popular and we can assume that the cookies API follow the latest specs, but of course it's python3 only since some time.

Anyway, my idea of using a cookie library was to simplify the test a bit but also exactly to change ERP5OAuth2ResourceServerPlugin.__extractCookie to deserialise to a dict, drop the key we don't want and re-serialise, which works only if we are using a library capable of doing this without loosing data and this means keeping the library up to date so that it supports upcoming changes with the cookie specs (python2 standard library is a good example of how this can fail, because it has API for this but without SameSite support).

We don't know if the current approach of splitting ourselves the header line directly will support upcoming cookies implementations, but since it was implemented this way on purpose and also because at this point we don't know know yet if the test failure is because of some incompatibility in ERP5OAuth2ResourceServerPlugin.__extractCookie, we should keep it like this and understand the failure first.

And yes, a well maintained library providing this exact feature of "remove a cookie from the raw cookie line without altering others" probably does not exist.

[Kazuhiko Shiozaki]

Do you have a link to a test result so I can check ?

There's a failing test result at https://erp5js.nexedi.net/#/test_result_module/20221020-C64B563/177

the zope change which I suspect to have introduced the failure is https://github.com/zopefoundation/Zope/commit/12c78aa402716611537c10e5f7d24b039705a01e

Should be fixed in 17346560.