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.

Sign up for GitHub

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

Add support for SameSite session cookies #215

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support for SameSite session cookies #215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions lib/response/sfWebResponse.class.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,10 +158,11 @@ public function isHeaderOnly()
* @param string $domain Domain name
* @param bool $secure If secure
* @param bool $httpOnly If uses only HTTP
* @param string $samesite SameSite cookies
*
* @throws <b>sfException</b> If fails to set the cookie
*/
public function setCookie($name, $value, $expire = null, $path = '/', $domain = '', $secure = false, $httpOnly = false)
public function setCookie($name, $value, $expire = null, $path = '/', $domain = '', $secure = false, $httpOnly = false, $samesite = '')
{
if ($expire !== null)
{
Expand All @@ -187,6 +188,7 @@ public function setCookie($name, $value, $expire = null, $path = '/', $domain =
'domain' => $domain,
'secure' => $secure ? true : false,
'httpOnly' => $httpOnly,
'samesite' => $samesite
);
}

Expand Down Expand Up @@ -365,7 +367,18 @@ public function sendHttpHeaders()
// cookies
foreach ($this->cookies as $cookie)
{
setrawcookie($cookie['name'], $cookie['value'], $cookie['expire'], $cookie['path'], $cookie['domain'], $cookie['secure'], $cookie['httpOnly']);
if (PHP_VERSION_ID < 70300) {
setrawcookie($cookie['name'], $cookie['value'], $cookie['expire'], $cookie['path'], $cookie['domain'], $cookie['secure'], $cookie['httpOnly']);
} else {
setrawcookie($cookie['name'], $cookie['value'], array(
'expires' => $cookie['expire'],
'path' => $cookie['path'],
'domain' => $cookie['domain'],
'secure' => $cookie['secure'],
'httpOnly' => $cookie['httpOnly'],
'samesite' => $cookie['samesite'],
));
}

if ($this->options['logging'])
{
Expand Down
15 changes: 14 additions & 1 deletion lib/storage/sfSessionStorage.class.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ public function initialize($options = null)
'session_cookie_domain' => $cookieDefaults['domain'],
'session_cookie_secure' => $cookieDefaults['secure'],
'session_cookie_httponly' => isset($cookieDefaults['httponly']) ? $cookieDefaults['httponly'] : false,
'session_cookie_samesite' => isset($cookieDefaults['samesite']) ? $cookieDefaults['samesite'] : '',
'session_cache_limiter' => null,
), $options);

Expand All @@ -83,7 +84,19 @@ public function initialize($options = null)
$domain = $this->options['session_cookie_domain'];
$secure = $this->options['session_cookie_secure'];
$httpOnly = $this->options['session_cookie_httponly'];
session_set_cookie_params($lifetime, $path, $domain, $secure, $httpOnly);
$samesite = $this->options['session_cookie_samesite'];
if (PHP_VERSION_ID < 70300) {
session_set_cookie_params($lifetime, $path, $domain, $secure, $httpOnly);
} else {
session_set_cookie_params(array(
'lifetime' => $lifetime,
'path' => $path,
'domain' => $domain,
'secure' => $secure,
'httponly' => $httpOnly,
'samesite' => $samesite
));
}

if (null !== $this->options['session_cache_limiter'])
{
Expand Down
2 changes: 1 addition & 1 deletion test/unit/response/sfWebResponseTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

BC break does not impact PHP >= 7.3 but for PHP < 7.3.

What is the problem to have behaviour depending on PHP version?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@alquerci alquerci Oct 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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_params
  • setrawcookie

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'
    ));
}


// ->setHeaderOnly() ->getHeaderOnly()
$t->diag('->setHeaderOnly() ->isHeaderOnly()');
Expand Down