From 4d440be93adcfd5e4ee0bdc7acd1c3260625728f Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Tue, 4 Sep 2018 10:27:39 +0200 Subject: [PATCH] [HttpFoundation][FrameworkBundle] fix support for samesite in session cookies --- Session/SessionUtils.php | 59 +++++++++++++++++++ .../Handler/AbstractSessionHandler.php | 50 +++++++--------- Session/Storage/NativeSessionStorage.php | 42 +++++++++++-- .../Storage/Handler/Fixtures/storage.expected | 3 +- .../Fixtures/with_cookie_and_session.expected | 1 + .../Handler/Fixtures/with_samesite.expected | 16 +++++ .../Handler/Fixtures/with_samesite.php | 13 ++++ .../with_samesite_and_migration.expected | 23 ++++++++ .../Fixtures/with_samesite_and_migration.php | 15 +++++ .../Storage/NativeSessionStorageTest.php | 8 ++- 10 files changed, 192 insertions(+), 38 deletions(-) create mode 100644 Session/SessionUtils.php create mode 100644 Tests/Session/Storage/Handler/Fixtures/with_samesite.expected create mode 100644 Tests/Session/Storage/Handler/Fixtures/with_samesite.php create mode 100644 Tests/Session/Storage/Handler/Fixtures/with_samesite_and_migration.expected create mode 100644 Tests/Session/Storage/Handler/Fixtures/with_samesite_and_migration.php diff --git a/Session/SessionUtils.php b/Session/SessionUtils.php new file mode 100644 index 000000000..04a25f716 --- /dev/null +++ b/Session/SessionUtils.php @@ -0,0 +1,59 @@ + + * + * 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 + * @author Rémon van de Kamp + * + * @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; + } +} diff --git a/Session/Storage/Handler/AbstractSessionHandler.php b/Session/Storage/Handler/AbstractSessionHandler.php index 84ba0ebba..ec59d895c 100644 --- a/Session/Storage/Handler/AbstractSessionHandler.php +++ b/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 protected function doWrite($sessionId, $data); abstract protected function doDestroy($sessionId); /** - * {@inheritdoc} + * @return bool */ public function validateId($sessionId) { @@ -73,7 +75,7 @@ public function validateId($sessionId) } /** - * {@inheritdoc} + * @return string */ public function read($sessionId) { @@ -99,7 +101,7 @@ public function read($sessionId) } /** - * {@inheritdoc} + * @return bool */ public function write($sessionId, $data) { @@ -124,7 +126,7 @@ public function write($sessionId, $data) } /** - * {@inheritdoc} + * @return bool */ public function destroy($sessionId) { @@ -135,31 +137,23 @@ public function destroy($sessionId) 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; - - if (11 !== strpos($h, $sessionCookieWithId, 11)) { - $otherCookies[] = $h; - } + $cookie = SessionUtils::popSessionCookie($this->sessionName, $sessionId); + + /* + * 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; - } - } - if ($sessionCookieFound) { - header_remove('Set-Cookie'); - foreach ($otherCookies as $h) { - header($h, false); + $params = session_get_cookie_params(); + unset($params['lifetime']); + setcookie($this->sessionName, '', $params); } - } 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)); } } diff --git a/Session/Storage/NativeSessionStorage.php b/Session/Storage/NativeSessionStorage.php index cbf7cadc7..7502897a4 100644 --- a/Session/Storage/NativeSessionStorage.php +++ b/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 @@ public function start() 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 @@ public function regenerate($destroy = false, $lifetime = null) // @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 @@ public function regenerate($destroy = false, $lifetime = null) */ 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 @@ public function save() 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 @@ public function setOptions(array $options) $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 @@ public function setOptions(array $options) 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 @@ public function setOptions(array $options) * @see https://php.net/sessionhandlerinterface * @see https://php.net/sessionhandler * - * @param \SessionHandlerInterface|null $saveHandler + * @param AbstractProxy|\SessionHandlerInterface|null $saveHandler * * @throws \InvalidArgumentException */ diff --git a/Tests/Session/Storage/Handler/Fixtures/storage.expected b/Tests/Session/Storage/Handler/Fixtures/storage.expected index 4533a10a1..05a5d5d0b 100644 --- a/Tests/Session/Storage/Handler/Fixtures/storage.expected +++ b/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 diff --git a/Tests/Session/Storage/Handler/Fixtures/with_cookie_and_session.expected b/Tests/Session/Storage/Handler/Fixtures/with_cookie_and_session.expected index 5de2d9e39..63078228d 100644 --- a/Tests/Session/Storage/Handler/Fixtures/with_cookie_and_session.expected +++ b/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 diff --git a/Tests/Session/Storage/Handler/Fixtures/with_samesite.expected b/Tests/Session/Storage/Handler/Fixtures/with_samesite.expected new file mode 100644 index 000000000..d20fb88ec --- /dev/null +++ b/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 diff --git a/Tests/Session/Storage/Handler/Fixtures/with_samesite.php b/Tests/Session/Storage/Handler/Fixtures/with_samesite.php new file mode 100644 index 000000000..fc2c41828 --- /dev/null +++ b/Tests/Session/Storage/Handler/Fixtures/with_samesite.php @@ -0,0 +1,13 @@ + 'lax']); +$storage->setSaveHandler(new TestSessionHandler()); +$storage->start(); + +$_SESSION = ['foo' => 'bar']; + +ob_start(function ($buffer) { return str_replace(session_id(), 'random_session_id', $buffer); }); diff --git a/Tests/Session/Storage/Handler/Fixtures/with_samesite_and_migration.expected b/Tests/Session/Storage/Handler/Fixtures/with_samesite_and_migration.expected new file mode 100644 index 000000000..8b5fc08bd --- /dev/null +++ b/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 diff --git a/Tests/Session/Storage/Handler/Fixtures/with_samesite_and_migration.php b/Tests/Session/Storage/Handler/Fixtures/with_samesite_and_migration.php new file mode 100644 index 000000000..a28b6fedf --- /dev/null +++ b/Tests/Session/Storage/Handler/Fixtures/with_samesite_and_migration.php @@ -0,0 +1,15 @@ + '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)); }); diff --git a/Tests/Session/Storage/NativeSessionStorageTest.php b/Tests/Session/Storage/NativeSessionStorageTest.php index 9ce8108da..d2cf32452 100644 --- a/Tests/Session/Storage/NativeSessionStorageTest.php +++ b/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 @@ public function testCookieOptions() '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 @@ public function testCookieOptions() $gco['cookie_'.$key] = $value; } - unset($gco['cookie_samesite']); - $this->assertEquals($options, $gco); }