nexedi / erp5

I tried to find a way to allow some web sections to change the SameSite value (in order to not break officejs).

But it seems setAuthCookie is called before registering the ERP5JS web section in the request, and I could not find a way to fetch the web section.

Which also means it is not possible to surcharge setAuthCookie via the SkinSelection RJS.

For now, I don't know how to prevent breaking OfficeJS, except by patching setAuthCookie on the OfficeJS data backends, but this will apply the SameSite value to all ERP5 urls, which will lead to the same issue for public web sites.

Any idea is welcome

/cc @jerome @vpelletier

Maybe one way could be to use a custom login page that would also post a hidden field like __ac_cookie_same_site=None and have setAuthCookie look in request.

added 1 commit

• 827dc213 - erp5_core: allow instances to surchage the SameSite value

Compare with previous version

post a hidden field [...]

This means signing the hidden field value too, as the client value should not be trusted.

For now, after discussing with Vincent, I added a new script to allow instances to configure the SameSite value based on the HTTP Host.

Please also note that using ERP5JS is perfectly possible with the Strict value (as the html page is a static and published web page).

Both dict.get and HTTPRequest.get have the sane default value of None, so the second argument can be dropped.

Going futher, I think this script (not only this lookup, but also the existing SERVER_URL one scratch this part: SERVER_URL is not always part of environ, and is only guaranteed to be in other) should actually not look into the whole request, but only into REQUEST.environ. Which is a dict, making the default value even more reliable.

All discussed here was applied, marking resolved.

post a hidden field [...]

Yes, forget this idea. I was thinking that we can trust the login page, because if some attacker could alter the login page then we don't have much left to protect, but I missed the point that all this was nonsense because we can just add the logic in the action script. The approach you took looks good.

Please also note that using ERP5JS is perfectly possible with the Strict value (as the html page is a static and published web page).

For the records, google login integration is known not to work with Strict and needs Lax, so using Lax by default seems good.

Shouldn't we name the script with a bit more context, something like ERP5Site_getAuthCookieSameSite ?

BTW, as I'm working on tests I found product/ERP5Security/tests/testERP5Security.py which has TestAuthenticationCookie.testCookieAttributes, which checks Secure, HTTPOnly. You may want to extend it so it also covers SameSite.

You may want to extend it so it also covers SameSite.

if so, then maybe apply the remaining patches from !802 (diffs)

added 15 commits

• 827dc213...b41f0737 - 10 commits from branch nexedi:master
• 1a8ab34b - erp5_core: Use SameSite=Lax cookie
• 4cec64b8 - erp5_core: allow instances to surchage the SameSite value
• f0f1e99d - core: set SameSite=Lax on authentication cookie
• 1af5c7df - erp5_core: rename
• e3903b74 - erp5_core: coding style

Compare with previous version

added 1 commit

• 0386a23c - erp5_core: server_url is not in environ

Compare with previous version

Tests were OK.

With you approval, I'll squash the commits and merge.

I just have one small concern is that this ERP5Site_getAuthCookieSameSite script accepts host, port and path and its a customisation point, so it looks we can customize based on the port or path, but here we only pass the host.

Maybe we should pass the full request ?

here we only pass the host.

I think this is a bug indeed, it should provide all 3.

Maybe we should pass the full request ?

The advantage of passing only those 3 values is that it discourages applying conditions which will anyway not be visible to the browser via the cookie parameters. Which would then cause different behaviours depending on what the user browsed in which order and with which headers (or whatever the script would look for in the request).

I'm experimenting passing: scheme, hostname, port and path.

As we are aware of browser's compatibility issues with the SAME_SITE parameter, it could also be useful to pass the User-Agent, as optional parameter.

BTW, the REQUEST's HTTP_HOST seems to only contain local zope connection information. So, instead, I'm using now the ACTUAL_URL key.

I'm experimenting passing: scheme

I noticed this may be missing, but wondered if it was intentional (it should probably only matter for Secure flag). No problem adding it for me.

it could also be useful to pass the User-Agent

It's unfortunate, but it makes sense to add it yes.

as optional parameter.

Just a nitpick: As you are defining the API with the expectation that caller should not be customised, this means the caller must always provide this value (otherwise it means it must be customised, which defeat the purpose, or it must introspect the callee, which I think is unnecessary complication). So "optional" does not really have a meaning on this side of the call...

I believe everything discussed here was applied. Marking resolved.

With you approval, I'll squash the commits and merge.

Besides the "only host is given to ERP5Site_getAuthCookieSameSite" issue (so passing the 2 missing parameters), it looks all good to me.

added 14 commits

• 0386a23c...1764c526 - 7 commits from branch nexedi:master
• f987eff3 - erp5_core: Use SameSite=Lax cookie
• b74a06d2 - erp5_core: allow instances to surchage the SameSite value
• ecf62bfe - core: set SameSite=Lax on authentication cookie
• 8e4cf2c9 - erp5_core: rename
• c67b8d52 - erp5_core: coding style
• c34b4b2b - erp5_core: server_url is not in environ
• 958ef680 - erp5_core: parse actual_url

Compare with previous version

added 18 commits

• 958ef680...522ae495 - 10 commits from branch nexedi:master
• 8c65094a - erp5_core: Use SameSite=Lax cookie
• c1bc605a - erp5_core: allow instances to surchage the SameSite value
• a5b56fc6 - core: set SameSite=Lax on authentication cookie
• 8cffae81 - erp5_core: rename
• 14924440 - erp5_core: coding style
• 0f61a159 - erp5_core: server_url is not in environ
• d1393a56 - erp5_core: parse actual_url
• c4abf636 - erp5_core: user agent parameter

Compare with previous version

added 10 commits

• c4abf636...94053d30 - 9 commits from branch nexedi:master
• 4db8fd54 - core: set SameSite=Lax on authentication cookie

Compare with previous version

unmarked as a Work In Progress

@jerome @vpelletier I remove the WIP prefix, as I do not plan to change anything.

Feel free to merge if you're OK or request improvements.

resolved all discussions

I remove the WIP prefix, as I do not plan to change anything.

Thanks, I reviewed the code for places which still had unresolved discussions, and I believe all are resolved.

I'll leave it open for a bit more in case there is disagreement about resolution.

For me it's also OK, maybe @romain it's better you merge. There's no written policy but it might be better that we merge our own merge requests rather than wait for someone else to merge.

There's one thing though, not every body knew this so I prefer to be explicit: this is a merge request with only one commit and the commit message is different from the merge request description. This version of gitlab is modified to add an "Apply patch" button and remove the "Merge" button. If you click "Apply patch", the commit that gitlab will apply on master will use the message from the merge request description, but I think the message from the commit is more complete, so we should either: edit the MR description with the commit message and hit "Apply patch" OR push this commit on master branch using git command line

added 3 commits

• 4db8fd54...317c3bd1 - 2 commits from branch nexedi:master
• c4bd6cd2 - core: set SameSite=Lax on authentication cookie

Compare with previous version

merged