Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set SameSite=Lax Cookie Attribute and Upgrade Requirements to PHP 7.4 #3994

Merged
merged 6 commits into from Aug 21, 2023

Conversation

splitbrain
Copy link
Collaborator

This implements #2849 and switches all cookie setting to the new named options array style introduced in PHP 7.3. Subsequently it updates the minimum requirements to PHP 7.4 which is probably the only PHP 7 version still widely in use anyway.

Since this has been the default in Chrome for a while, no sideeffects
are to be expected.
updates may also be caused by the php platform version increase in the last
commit
Copy link
Collaborator

@michitux michitux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In principle I agree with this and this looks good.

Note that SameSite=Lax may break authentication workflows where an external site redirects back to DokuWiki with a POST request. For this reason, Chrome's SameSite=Lax by default behavior sends cookies that are less than two minutes old in top-level cross-origin POST requests (see, e.g., PortSwigger). According to SameSite Updates, this should be temporary but I couldn't find any information about this actually being phased out.

Therefore, it might be interesting to make this configurable with an appropriate security warning. I guess it could in particular be interesting to allow returning to the browser's default behavior but also maybe to allow setting Strict for extra protection or even None if it should, e.g., be possible to embed private images on another domain (but this is already broken in Chrome today, so if nobody complained, it can't be that important).

I'm not saying you should add the configuration option, I just want to make you aware of the potential breakages, I don't know if this actually affects anything in DokuWiki.

As mentioned in
#3994 (review)
there might be occasions when users might want to change the policy to a
stricter one or the somewhat more lenient Lax implementation of current
browsers.
@splitbrain splitbrain merged commit a493dd6 into master Aug 21, 2023
4 of 12 checks passed
@splitbrain splitbrain deleted the cookie branch August 21, 2023 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants