FEAT: Add support for samesite attribute to session cookies #9842
Conversation
| @@ -329,10 +329,27 @@ public function start(HTTPRequest $request) | |||
|
|
|||
| session_start(); | |||
|
|
|||
| $sameSite = null; | |||
| $sessionParams = session_get_cookie_params(); | |||
There was a problem hiding this comment.
Does this work? It's calling session_set_cookie_params above without samesite.
From my testing, the above call removes the samesite, and the below Cookie::set will only get called if you also have a non-zero $timeout configured.
There was a problem hiding this comment.
@madmatt matt-in-a-hatt had an unanswered question about this above
| ) { | ||
| // if headers aren't sent, we can set the cookie | ||
| if (!headers_sent($file, $line)) { | ||
| return setcookie($name, $value, $expiry, $path, $domain, $secure, $httpOnly); | ||
| if (version_compare(PHP_VERSION, '7.3.0') >= 0) { |
There was a problem hiding this comment.
No need for this check, minimum version of framework is currently 7.3
| ] | ||
| ); | ||
| } else { | ||
| // We can't set the SameSite cookie, if it isn't the default value of null then throw an error |
| @@ -329,10 +329,27 @@ public function start(HTTPRequest $request) | |||
|
|
|||
| session_start(); | |||
|
|
|||
| $sameSite = null; | |||
| $sessionParams = session_get_cookie_params(); | |||
There was a problem hiding this comment.
@madmatt matt-in-a-hatt had an unanswered question about this above
| $sameSite = null; | ||
| $sessionParams = session_get_cookie_params(); | ||
|
|
||
| // If samesite param is set, then we can pass it into the Cookie::set call below as we are on >= PHP 7.3 |
|
@madmatt @emteknetnz @matt-in-a-hat It'd be great if this PR could be fixed up so it can be merged. Incorporating it requires very little changes but makes SilverStripe a lot more flexible in headless environments :) Happy to fork it and open a new PR, but I just thought I'd give this a push because it looks like it's nearly there. |
|
@NikxDa Doesn't look like there's any movement here - are you still interested in forking and opening a new PR for this? |
|
Closing in favour of #10335 which avoids breaking changes this PR would introduce. |
Relates to #9440.
Expected behaviour
Silverstripe CMS should support the new SameSite cookie attribute for all cookies, as well as session cookies.
Observed behaviour
Silverstripe CMS doesn't properly support this attribute. There are two issues:
Cookie::set()logic doesn't have support for the SameSite attribute.ini_set()but it only works for the very firstSet-Cookieheader - from then on the framework sets the cookie without the flag.I think this fix is backwards compatible. It allows the attribute to be set, but only on PHP 7.3+ because it can't be set on older versions of PHP than that (without manually setting HTTP headers).
This doesn't take on any of the complexity that #9548 discussed, just the simplest possible thing to ensure that the samesite attribute is consistently set on session cookies.