Add support for SameSite session cookies #215
Conversation
|
Maybe add sfWebResponse->setCookie to be feature complete? |
Starting from PHP 7.3 there's native support for SameSite cookies (RFC6265bis) which requires using a new session_get_cookie_params() parameter syntax
There was a problem hiding this comment.
Hello,
Comments
Taking Symfony as reference and to not reinvent the wheel.
I suggest to do the same kind of implementation.
- With constant
- When same site is
nulldo not add on cookie parameters.
Also the implementation is not complete as it is missing on test component (sfTesterResponse).
Add it for PHP < 7.3
To implement this feature fully for all PHP version cookies should be sent with header() ref.
But it will need to use Symfony\Component\HttpFoundation\Cookie that's a bit maybe a good idea use it if the class exists. And add symfony/http-foundation on composer suggestion.
| @@ -288,7 +288,7 @@ public function normalizeHeaderName($name) | |||
| // ->setCookie() ->getCookies() | |||
| $t->diag('->setCookie() ->getCookies()'); | |||
| $response->setCookie('foo', 'bar'); | |||
| $t->is($response->getCookies(), array('foo' => array('name' => 'foo', 'value' => 'bar', 'expire' => null, 'path' => '/', 'domain' => '', 'secure' => false, 'httpOnly' => false)), '->setCookie() adds a cookie for the response'); | |||
| $t->is($response->getCookies(), array('foo' => array('name' => 'foo', 'value' => 'bar', 'expire' => null, 'path' => '/', 'domain' => '', 'secure' => false, 'httpOnly' => false, 'samesite' => '')), '->setCookie() adds a cookie for the response'); | |||
There was a problem hiding this comment.
This modification tell that there is a BC break.
As the feature is only for PHP >= 7.3 then current tests should not be touch.
But add a test to target specific version.
There was a problem hiding this comment.
This is not a BC break. The existence of ['samesite' => ''] in the output of getCookies() does not affect behavior - it gets ignored by setrawcookie and doesn't get sent in the header.
<?php
// Falsy values are simply ignored - don't get sent in the header by PHP
setrawcookie('samesite_null', 'testing', [
'samesite' => null
]);
setrawcookie('samesite_emptystring', 'testing', [
'samesite' => ''
]);
setrawcookie('samesite_false', 'testing', [
'samesite' => false
]);
// Other values, whether legit or not, do get sent
setrawcookie('samesite_gibberish', 'testing', [
'samesite' => 'asqewrqewrq'
]);
setrawcookie('samesite_none', 'testing', [
'samesite' => 'none'
]);
?>
Here's the body content❯ curl -I http://localhost/cookie.php
HTTP/1.1 200 OK
Date: Thu, 05 Oct 2023 23:33:55 GMT
Server: Apache/2.4.57 (Unix) PHP/7.4.33
X-Powered-By: PHP/7.4.33
Set-Cookie: samesite_null=testing
Set-Cookie: samesite_emptystring=testing
Set-Cookie: samesite_false=testing
Set-Cookie: samesite_gibberish=testing; SameSite=asqewrqewrq
Set-Cookie: samesite_none=testing; SameSite=none
Content-Type: text/html; charset=UTF-8
There was a problem hiding this comment.
BC break does not impact PHP >= 7.3 but for PHP < 7.3.
What is the problem to have behaviour depending on PHP version?
There was a problem hiding this comment.
The behavior does depend on PHP version: https://github.com/FriendsOfSymfony1/symfony1/pull/215/files#diff-5aa2248855efa9ca1219f04cb7575c93a0a1f4af85e30ff32013bf84afcbf0d5R370-R381
In PHP < 7.3, it calls session_set_cookie_params in the original form. In PHP >= 7.3, it uses the new form (passing options as an array, including the samesite setting which is a no-op unless a value has been explicitly set). Yes, $response->getCookies() returns something different, but I don't think returning an extra key in an array from a getter of an internal property should be considered a BC break.
There was a problem hiding this comment.
What I think
As you said: The behaviour does depend on PHP version.
And tests are a proof that behaviour work as it should.
The tests should expose this behaviour.
getCookies() is parts of the public interface, not the private one.
So it should not lie on what value is passed to PHP internal functions.
It is just my opinion.
Between: the property $cookies is protected, so part of the public interface.
In practice
Let's say that we can spy arguments passed to functions:
session_set_cookie_paramssetrawcookie
On those tests, we will see something like that:
// test/unit/**
if (PHP_VERSION_ID < 70300) {
// behaviour without samesite
givenCookie(array(
'name' => 'some cookie',
...
));
whenSendHttpHeaders();
assertCookies(array(
'cookie name' => array(
'name' => 'some cookie',
...
)
));
assertSetrawcookieWasCalledWith('some cookie', ..., array(
...
));
} else {
// behaviour with samesite
givenCookie(array(
'name' => 'some cookie',
...,
'samesite' => 'samesite_value'
));
whenSendHttpHeaders();
assertCookies(array(
'cookie name' => array(
'name' => 'some cookie',
...
'samesite' => 'samesite_value'
)
));
assertSetrawcookieWasCalledWith('some cookie', ..., array(
...,
'samesite' => 'samesite_value'
));
}
|
It looks like this PR was never merged. @alquerci does this just need a PHP-7.3-specific test in sfWebResponseTest.php? |
@mkopinsky yes @thePanz can you tell us what is the minimum PHP version supported now ? |
|
@alquerci @mkopinsky (damn, that's quite an old ticket 😅 ) My plan is to move to PHP v7.4 as min version as a first step. WDYT? |
|
@thePanz I think that make sf1 compatible with latest PHP version is another topic than removing support for old PHP versions. What problem will be fixed by removing all PHP v5.x and 7.x support? My point of viewWe should be able to have information about the usage of sf1 in terms of, at least:
Because the segment of sf1 usage is the main target of the purpose of the support provided by FriendsOfSymfony1. The main goal is to provide an easy path to migrate to the latest Symfony LTS version, or at least the first Symfony2 version. Nothing new as FriendsOfSymfony1 organization what next? #201 Provide a list of instructions how to switch from symfony 1.4.20 to FriendsOfSymfony1 #244 Do you agree with that goal? What I know
Hypothesis
That's why, like a production management, we need to know users of sf1. |
Who wants to migrate to an unsupported outdated version of symfony? |
Starting from PHP 7.3 there's native support for SameSite cookies (RFC6265bis) which requires using a new session_get_cookie_params() parameter syntax.