bug #35605 [HttpFoundation][FrameworkBundle] fix support for samesite in session cookies (fabpot)

This PR was merged into the 3.4 branch. Discussion ---------- [HttpFoundation][FrameworkBundle] fix support for samesite in session cookies | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #35520 | License | MIT | Doc PR | - This PR cherry-picks #28168 on 3.4, with a rationale given by @ConneXNL in https://github.com/symfony/symfony/issues/35520#issuecomment-582296847: > I hope I am wrong but I see the impact of not making any changes to Symfony 3.4 will have a tons of sites break if we cannot set the cookie's samesite setting (in the framework session and remember me) before Chrome pushes this update. > > Very soon all existing cookies are no longer going to work with cross-domains if you do not specify 'None' for the cookie_samesite. All external APIs that use cookies and are running SF 3.4 will break and devs will have no quick solution to fix their auth process. > > If you are using PHP 7.4, yes you can most likely use ini_set to workaround this issue. > > However, ini_set('cookie_samesite') does not work in PHP Version <= 7.2. I am not even sure PHP 7.3 supports the value 'None' as php.watch/articles/PHP-Samesite-cookies says it has support for 'Lax' and 'Scrict'. > > This effectively means SF 3.4 on PHP 7.2 (or PHP 7.3) is no longer supported for cross domain APIs with cookies. People would have to either update PHP to 7.4 (if they even can?) or go to Symfony 4 (with a dead live site is going to be a complete disaster). > > Since the impact of the change that chrome is about to roll out is so fundamentally changing our way to set cookies, I consider configuring samesite configuration in the framework an absolute requirement, not a feature, especially since SF 3.4 is still supported. > > What am i missing? > > Note: SF3 HTTPFoundation already supports the new cookie settings, it's just the framework that doesn't support it. Our BC policy embeds the promise that one should be able to keep the same app on a newest infrastructure (eg that's why supporting a PHP version is a bug fix). I think we can consider this for browsers here also. WDYT? Commits ------- f46e6cb8a0 [HttpFoundation][FrameworkBundle] fix support for samesite in session cookies
2020-02-07 08:56:52 +01:00 · 2020-02-07 08:56:52 +01:00 · f350f532b7
commit f350f532b7
parent e41a31236a f46e6cb8a0
13 changed files with 195 additions and 38 deletions
--- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
+++ b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
@ -20,6 +20,7 @@ use Symfony\Component\Config\Definition\Builder\TreeBuilder;
 use Symfony\Component\Config\Definition\ConfigurationInterface;
 use Symfony\Component\DependencyInjection\Exception\LogicException;
 use Symfony\Component\Form\Form;
+use Symfony\Component\HttpFoundation\Cookie;
 use Symfony\Component\Lock\Lock;
 use Symfony\Component\Lock\Store\SemaphoreStore;
 use Symfony\Component\PropertyInfo\PropertyInfoExtractorInterface;
@ -490,6 +491,7 @@ class Configuration implements ConfigurationInterface
                        ->scalarNode('cookie_domain')->end()
                        ->booleanNode('cookie_secure')->end()
                        ->booleanNode('cookie_httponly')->defaultTrue()->end()
+                        ->enumNode('cookie_samesite')->values([null, Cookie::SAMESITE_LAX, Cookie::SAMESITE_STRICT])->defaultNull()->end()
                        ->booleanNode('use_cookies')->end()
                        ->scalarNode('gc_divisor')->end()
                        ->scalarNode('gc_probability')->defaultValue(1)->end()
--- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
+++ b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
@ -867,7 +867,7 @@ class FrameworkExtension extends Extension
        // session storage
        $container->setAlias('session.storage', $config['storage_id'])->setPrivate(true);
        $options = ['cache_limiter' => '0'];
-        foreach (['name', 'cookie_lifetime', 'cookie_path', 'cookie_domain', 'cookie_secure', 'cookie_httponly', 'use_cookies', 'gc_maxlifetime', 'gc_probability', 'gc_divisor', 'use_strict_mode'] as $key) {
+        foreach (['name', 'cookie_lifetime', 'cookie_path', 'cookie_domain', 'cookie_secure', 'cookie_httponly', 'cookie_samesite', 'use_cookies', 'gc_maxlifetime', 'gc_probability', 'gc_divisor'] as $key) {
            if (isset($config[$key])) {
                $options[$key] = $config[$key];
            }
--- a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php
+++ b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php
@ -436,6 +436,7 @@ class ConfigurationTest extends TestCase
                'storage_id' => 'session.storage.native',
                'handler_id' => 'session.handler.native_file',
                'cookie_httponly' => true,
+                'cookie_samesite' => null,
                'gc_probability' => 1,
                'save_path' => '%kernel.cache_dir%/sessions',
                'metadata_update_threshold' => '0',
--- a/src/Symfony/Component/HttpFoundation/Session/SessionUtils.php
+++ b/src/Symfony/Component/HttpFoundation/Session/SessionUtils.php
@ -0,0 +1,59 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpFoundation\Session;
+
+/**
+ * Session utility functions.
+ *
+ * @author Nicolas Grekas <p@tchwork.com>
+ * @author Rémon van de Kamp <rpkamp@gmail.com>
+ *
+ * @internal
+ */
+final class SessionUtils
+{
+    /**
+     * Find the session header amongst the headers that are to be sent, remove it, and return
+     * it so the caller can process it further.
+     */
+    public static function popSessionCookie($sessionName, $sessionId)
+    {
+        $sessionCookie = null;
+        $sessionCookiePrefix = sprintf(' %s=', urlencode($sessionName));
+        $sessionCookieWithId = sprintf('%s%s;', $sessionCookiePrefix, urlencode($sessionId));
+        $otherCookies = [];
+        foreach (headers_list() as $h) {
+            if (0 !== stripos($h, 'Set-Cookie:')) {
+                continue;
+            }
+            if (11 === strpos($h, $sessionCookiePrefix, 11)) {
+                $sessionCookie = $h;
+
+                if (11 !== strpos($h, $sessionCookieWithId, 11)) {
+                    $otherCookies[] = $h;
+                }
+            } else {
+                $otherCookies[] = $h;
+            }
+        }
+        if (null === $sessionCookie) {
+            return null;
+        }
+
+        header_remove('Set-Cookie');
+        foreach ($otherCookies as $h) {
+            header($h, false);
+        }
+
+        return $sessionCookie;
+    }
+}
--- a/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/AbstractSessionHandler.php
+++ b/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/AbstractSessionHandler.php
@ -11,6 +11,8 @@

 namespace Symfony\Component\HttpFoundation\Session\Storage\Handler;

+use Symfony\Component\HttpFoundation\Session\SessionUtils;
+
 /**
 * This abstract session handler provides a generic implementation
 * of the PHP 7.0 SessionUpdateTimestampHandlerInterface,
@ -27,7 +29,7 @@ abstract class AbstractSessionHandler implements \SessionHandlerInterface, \Sess
    private $igbinaryEmptyData;

    /**
-     * {@inheritdoc}
+     * @return bool
     */
    public function open($savePath, $sessionName)
    {
@ -62,7 +64,7 @@ abstract class AbstractSessionHandler implements \SessionHandlerInterface, \Sess
    abstract protected function doDestroy($sessionId);

    /**
-     * {@inheritdoc}
+     * @return bool
     */
    public function validateId($sessionId)
    {
@ -73,7 +75,7 @@ abstract class AbstractSessionHandler implements \SessionHandlerInterface, \Sess
    }

    /**
-     * {@inheritdoc}
+     * @return string
     */
    public function read($sessionId)
    {
@ -99,7 +101,7 @@ abstract class AbstractSessionHandler implements \SessionHandlerInterface, \Sess
    }

    /**
-     * {@inheritdoc}
+     * @return bool
     */
    public function write($sessionId, $data)
    {
@ -124,7 +126,7 @@ abstract class AbstractSessionHandler implements \SessionHandlerInterface, \Sess
    }

    /**
-     * {@inheritdoc}
+     * @return bool
     */
    public function destroy($sessionId)
    {
@ -135,32 +137,24 @@ abstract class AbstractSessionHandler implements \SessionHandlerInterface, \Sess
            if (!$this->sessionName) {
                throw new \LogicException(sprintf('Session name cannot be empty, did you forget to call "parent::open()" in "%s"?.', static::class));
            }
-            $sessionCookie = sprintf(' %s=', urlencode($this->sessionName));
-            $sessionCookieWithId = sprintf('%s%s;', $sessionCookie, urlencode($sessionId));
-            $sessionCookieFound = false;
-            $otherCookies = [];
-            foreach (headers_list() as $h) {
-                if (0 !== stripos($h, 'Set-Cookie:')) {
-                    continue;
-                }
-                if (11 === strpos($h, $sessionCookie, 11)) {
-                    $sessionCookieFound = true;
+            $cookie = SessionUtils::popSessionCookie($this->sessionName, $sessionId);

-                    if (11 !== strpos($h, $sessionCookieWithId, 11)) {
-                        $otherCookies[] = $h;
-                    }
+            /*
+             * We send an invalidation Set-Cookie header (zero lifetime)
+             * when either the session was started or a cookie with
+             * the session name was sent by the client (in which case
+             * we know it's invalid as a valid session cookie would've
+             * started the session).
+             */
+            if (null === $cookie || isset($_COOKIE[$this->sessionName])) {
+                if (\PHP_VERSION_ID < 70300) {
+                    setcookie($this->sessionName, '', 0, ini_get('session.cookie_path'), ini_get('session.cookie_domain'), filter_var(ini_get('session.cookie_secure'), FILTER_VALIDATE_BOOLEAN), filter_var(ini_get('session.cookie_httponly'), FILTER_VALIDATE_BOOLEAN));
                } else {
-                    $otherCookies[] = $h;
+                    $params = session_get_cookie_params();
+                    unset($params['lifetime']);
+                    setcookie($this->sessionName, '', $params);
                }
            }
-            if ($sessionCookieFound) {
-                header_remove('Set-Cookie');
-                foreach ($otherCookies as $h) {
-                    header($h, false);
-                }
-            } else {
-                setcookie($this->sessionName, '', 0, ini_get('session.cookie_path'), ini_get('session.cookie_domain'), filter_var(ini_get('session.cookie_secure'), FILTER_VALIDATE_BOOLEAN), filter_var(ini_get('session.cookie_httponly'), FILTER_VALIDATE_BOOLEAN));
-            }
        }

        return $this->newSessionId === $sessionId || $this->doDestroy($sessionId);
--- a/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php
+++ b/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php
@ -12,6 +12,7 @@
 namespace Symfony\Component\HttpFoundation\Session\Storage;

 use Symfony\Component\HttpFoundation\Session\SessionBagInterface;
+use Symfony\Component\HttpFoundation\Session\SessionUtils;
 use Symfony\Component\HttpFoundation\Session\Storage\Handler\StrictSessionHandler;
 use Symfony\Component\HttpFoundation\Session\Storage\Proxy\AbstractProxy;
 use Symfony\Component\HttpFoundation\Session\Storage\Proxy\SessionHandlerProxy;
@ -48,6 +49,11 @@ class NativeSessionStorage implements SessionStorageInterface
     */
    protected $metadataBag;

+    /**
+     * @var string|null
+     */
+    private $emulateSameSite;
+
    /**
     * Depending on how you want the storage driver to behave you probably
     * want to override this constructor entirely.
@ -67,6 +73,7 @@ class NativeSessionStorage implements SessionStorageInterface
     * cookie_lifetime, "0"
     * cookie_path, "/"
     * cookie_secure, ""
+     * cookie_samesite, null
     * entropy_file, ""
     * entropy_length, "0"
     * gc_divisor, "100"
@ -94,9 +101,7 @@ class NativeSessionStorage implements SessionStorageInterface
     * trans_sid_hosts, $_SERVER['HTTP_HOST']
     * trans_sid_tags, "a=href,area=href,frame=src,form="
     *
-     * @param array                         $options Session configuration options
-     * @param \SessionHandlerInterface|null $handler
-     * @param MetadataBag                   $metaBag MetadataBag
+     * @param AbstractProxy|\SessionHandlerInterface|null $handler
     */
    public function __construct(array $options = [], $handler = null, MetadataBag $metaBag = null)
    {
@ -150,6 +155,13 @@ class NativeSessionStorage implements SessionStorageInterface
            throw new \RuntimeException('Failed to start the session');
        }

+        if (null !== $this->emulateSameSite) {
+            $originalCookie = SessionUtils::popSessionCookie(session_name(), session_id());
+            if (null !== $originalCookie) {
+                header(sprintf('%s; SameSite=%s', $originalCookie, $this->emulateSameSite), false);
+            }
+        }
+
        $this->loadSession();

        return true;
@ -215,6 +227,13 @@ class NativeSessionStorage implements SessionStorageInterface
        // @see https://bugs.php.net/70013
        $this->loadSession();

+        if (null !== $this->emulateSameSite) {
+            $originalCookie = SessionUtils::popSessionCookie(session_name(), session_id());
+            if (null !== $originalCookie) {
+                header(sprintf('%s; SameSite=%s', $originalCookie, $this->emulateSameSite), false);
+            }
+        }
+
        return $isRegenerated;
    }

@ -223,6 +242,7 @@ class NativeSessionStorage implements SessionStorageInterface
     */
    public function save()
    {
+        // Store a copy so we can restore the bags in case the session was not left empty
        $session = $_SESSION;

        foreach ($this->bags as $bag) {
@ -248,7 +268,11 @@ class NativeSessionStorage implements SessionStorageInterface
            session_write_close();
        } finally {
            restore_error_handler();
-            $_SESSION = $session;
+
+            // Restore only if not empty
+            if ($_SESSION) {
+                $_SESSION = $session;
+            }
        }

        $this->closed = true;
@ -347,7 +371,7 @@ class NativeSessionStorage implements SessionStorageInterface

        $validOptions = array_flip([
            'cache_expire', 'cache_limiter', 'cookie_domain', 'cookie_httponly',
-            'cookie_lifetime', 'cookie_path', 'cookie_secure',
+            'cookie_lifetime', 'cookie_path', 'cookie_secure', 'cookie_samesite',
            'entropy_file', 'entropy_length', 'gc_divisor',
            'gc_maxlifetime', 'gc_probability', 'hash_bits_per_character',
            'hash_function', 'lazy_write', 'name', 'referer_check',
@ -360,6 +384,12 @@ class NativeSessionStorage implements SessionStorageInterface

        foreach ($options as $key => $value) {
            if (isset($validOptions[$key])) {
+                if ('cookie_samesite' === $key && \PHP_VERSION_ID < 70300) {
+                    // PHP < 7.3 does not support same_site cookies. We will emulate it in
+                    // the start() method instead.
+                    $this->emulateSameSite = $value;
+                    continue;
+                }
                ini_set('url_rewriter.tags' !== $key ? 'session.'.$key : $key, $value);
            }
        }
@ -381,7 +411,7 @@ class NativeSessionStorage implements SessionStorageInterface
     * @see https://php.net/sessionhandlerinterface
     * @see https://php.net/sessionhandler
     *
-     * @param \SessionHandlerInterface|null $saveHandler
+     * @param AbstractProxy|\SessionHandlerInterface|null $saveHandler
     *
     * @throws \InvalidArgumentException
     */
--- a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/Fixtures/storage.expected
+++ b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/Fixtures/storage.expected
@ -11,10 +11,11 @@ $_SESSION is not empty
 write
 destroy
 close
-$_SESSION is not empty
+$_SESSION is empty
 Array
 (
    [0] => Content-Type: text/plain; charset=utf-8
    [1] => Cache-Control: max-age=0, private, must-revalidate
+    [2] => Set-Cookie: sid=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0; path=/; secure; HttpOnly
 )
 shutdown
--- a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/Fixtures/with_cookie_and_session.expected
+++ b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/Fixtures/with_cookie_and_session.expected
@ -20,5 +20,6 @@ Array
    [0] => Content-Type: text/plain; charset=utf-8
    [1] => Cache-Control: max-age=10800, private, must-revalidate
    [2] => Set-Cookie: abc=def
+    [3] => Set-Cookie: sid=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0; path=/; secure; HttpOnly
 )
 shutdown
--- a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/Fixtures/with_samesite.expected
+++ b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/Fixtures/with_samesite.expected
@ -0,0 +1,16 @@
+open
+validateId
+read
+doRead: 
+read
+
+write
+doWrite: foo|s:3:"bar";
+close
+Array
+(
+    [0] => Content-Type: text/plain; charset=utf-8
+    [1] => Cache-Control: max-age=0, private, must-revalidate
+    [2] => Set-Cookie: sid=random_session_id; path=/; secure; HttpOnly; SameSite=lax
+)
+shutdown
--- a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/Fixtures/with_samesite.php
+++ b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/Fixtures/with_samesite.php
@ -0,0 +1,13 @@
+<?php
+
+require __DIR__.'/common.inc';
+
+use Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage;
+
+$storage = new NativeSessionStorage(['cookie_samesite' => 'lax']);
+$storage->setSaveHandler(new TestSessionHandler());
+$storage->start();
+
+$_SESSION = ['foo' => 'bar'];
+
+ob_start(function ($buffer) { return str_replace(session_id(), 'random_session_id', $buffer); });
--- a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/Fixtures/with_samesite_and_migration.expected
+++ b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/Fixtures/with_samesite_and_migration.expected
@ -0,0 +1,23 @@
+open
+validateId
+read
+doRead: 
+read
+destroy
+close
+open
+validateId
+read
+doRead: 
+read
+
+write
+doWrite: foo|s:3:"bar";
+close
+Array
+(
+    [0] => Content-Type: text/plain; charset=utf-8
+    [1] => Cache-Control: max-age=0, private, must-revalidate
+    [2] => Set-Cookie: sid=random_session_id; path=/; secure; HttpOnly; SameSite=lax
+)
+shutdown
--- a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/Fixtures/with_samesite_and_migration.php
+++ b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/Fixtures/with_samesite_and_migration.php
@ -0,0 +1,15 @@
+<?php
+
+require __DIR__.'/common.inc';
+
+use Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage;
+
+$storage = new NativeSessionStorage(['cookie_samesite' => 'lax']);
+$storage->setSaveHandler(new TestSessionHandler());
+$storage->start();
+
+$_SESSION = ['foo' => 'bar'];
+
+$storage->regenerate(true);
+
+ob_start(function ($buffer) { return preg_replace('~_sf2_meta.*$~m', '', str_replace(session_id(), 'random_session_id', $buffer)); });
--- a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/NativeSessionStorageTest.php
+++ b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/NativeSessionStorageTest.php
@ -36,7 +36,7 @@ class NativeSessionStorageTest extends TestCase
    protected function setUp()
    {
        $this->iniSet('session.save_handler', 'files');
-        $this->iniSet('session.save_path', $this->savePath = sys_get_temp_dir().'/sf2test');
+        $this->iniSet('session.save_path', $this->savePath = sys_get_temp_dir().'/sftest');
        if (!is_dir($this->savePath)) {
            mkdir($this->savePath);
        }
@ -167,6 +167,10 @@ class NativeSessionStorageTest extends TestCase
            'cookie_httponly' => false,
        ];

+        if (\PHP_VERSION_ID >= 70300) {
+            $options['cookie_samesite'] = 'lax';
+        }
+
        $this->getStorage($options);
        $temp = session_get_cookie_params();
        $gco = [];
@ -175,8 +179,6 @@ class NativeSessionStorageTest extends TestCase
            $gco['cookie_'.$key] = $value;
        }

-        unset($gco['cookie_samesite']);
-
        $this->assertEquals($options, $gco);
    }