Skip to content

Commit f350f53

Browse files
fabpotfabpot
committed
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 #35520 (comment): > 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 ------- f46e6cb [HttpFoundation][FrameworkBundle] fix support for samesite in session cookies
2 parents e41a312 + f46e6cb commit f350f53

File tree

13 files changed

+196
-39
lines changed

13 files changed

+196
-39
lines changed

‎src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
use Symfony\Component\Config\Definition\ConfigurationInterface;
2121
use Symfony\Component\DependencyInjection\Exception\LogicException;
2222
use Symfony\Component\Form\Form;
23+
use Symfony\Component\HttpFoundation\Cookie;
2324
use Symfony\Component\Lock\Lock;
2425
use Symfony\Component\Lock\Store\SemaphoreStore;
2526
use Symfony\Component\PropertyInfo\PropertyInfoExtractorInterface;
@@ -490,6 +491,7 @@ private function addSessionSection(ArrayNodeDefinition $rootNode)
490491
->scalarNode('cookie_domain')->end()
491492
->booleanNode('cookie_secure')->end()
492493
->booleanNode('cookie_httponly')->defaultTrue()->end()
494+
->enumNode('cookie_samesite')->values([null, Cookie::SAMESITE_LAX, Cookie::SAMESITE_STRICT])->defaultNull()->end()
493495
->booleanNode('use_cookies')->end()
494496
->scalarNode('gc_divisor')->end()
495497
->scalarNode('gc_probability')->defaultValue(1)->end()

‎src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -867,7 +867,7 @@ private function registerSessionConfiguration(array $config, ContainerBuilder $c
867867
// session storage
868868
$container->setAlias('session.storage', $config['storage_id'])->setPrivate(true);
869869
$options = ['cache_limiter' => '0'];
870-
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) {
870+
foreach (['name', 'cookie_lifetime', 'cookie_path', 'cookie_domain', 'cookie_secure', 'cookie_httponly', 'cookie_samesite', 'use_cookies', 'gc_maxlifetime', 'gc_probability', 'gc_divisor'] as $key) {
871871
if (isset($config[$key])) {
872872
$options[$key] = $config[$key];
873873
}

‎src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -436,6 +436,7 @@ protected static function getBundleDefaultConfig()
436436
'storage_id' => 'session.storage.native',
437437
'handler_id' => 'session.handler.native_file',
438438
'cookie_httponly' => true,
439+
'cookie_samesite' => null,
439440
'gc_probability' => 1,
440441
'save_path' => '%kernel.cache_dir%/sessions',
441442
'metadata_update_threshold' => '0',

‎src/Symfony/Component/HttpFoundation/Session/SessionUtils.php

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Component\HttpFoundation\Session;
13+
14+
/**
15+
* Session utility functions.
16+
*
17+
* @author Nicolas Grekas <p@tchwork.com>
18+
* @author Rémon van de Kamp <rpkamp@gmail.com>
19+
*
20+
* @internal
21+
*/
22+
final class SessionUtils
23+
{
24+
/**
25+
* Find the session header amongst the headers that are to be sent, remove it, and return
26+
* it so the caller can process it further.
27+
*/
28+
public static function popSessionCookie($sessionName, $sessionId)
29+
{
30+
$sessionCookie = null;
31+
$sessionCookiePrefix = sprintf(' %s=', urlencode($sessionName));
32+
$sessionCookieWithId = sprintf('%s%s;', $sessionCookiePrefix, urlencode($sessionId));
33+
$otherCookies = [];
34+
foreach (headers_list() as $h) {
35+
if (0 !== stripos($h, 'Set-Cookie:')) {
36+
continue;
37+
}
38+
if (11 === strpos($h, $sessionCookiePrefix, 11)) {
39+
$sessionCookie = $h;
40+
41+
if (11 !== strpos($h, $sessionCookieWithId, 11)) {
42+
$otherCookies[] = $h;
43+
}
44+
} else {
45+
$otherCookies[] = $h;
46+
}
47+
}
48+
if (null === $sessionCookie) {
49+
return null;
50+
}
51+
52+
header_remove('Set-Cookie');
53+
foreach ($otherCookies as $h) {
54+
header($h, false);
55+
}
56+
57+
return $sessionCookie;
58+
}
59+
}

‎src/Symfony/Component/HttpFoundation/Session/Storage/Handler/AbstractSessionHandler.php

Lines changed: 22 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@
1111

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

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

2931
/**
30-
* {@inheritdoc}
32+
* @return bool
3133
*/
3234
public function open($savePath, $sessionName)
3335
{
@@ -62,7 +64,7 @@ abstract protected function doWrite($sessionId, $data);
6264
abstract protected function doDestroy($sessionId);
6365

6466
/**
65-
* {@inheritdoc}
67+
* @return bool
6668
*/
6769
public function validateId($sessionId)
6870
{
@@ -73,7 +75,7 @@ public function validateId($sessionId)
7375
}
7476

7577
/**
76-
* {@inheritdoc}
78+
* @return string
7779
*/
7880
public function read($sessionId)
7981
{
@@ -99,7 +101,7 @@ public function read($sessionId)
99101
}
100102

101103
/**
102-
* {@inheritdoc}
104+
* @return bool
103105
*/
104106
public function write($sessionId, $data)
105107
{
@@ -124,7 +126,7 @@ public function write($sessionId, $data)
124126
}
125127

126128
/**
127-
* {@inheritdoc}
129+
* @return bool
128130
*/
129131
public function destroy($sessionId)
130132
{
@@ -135,31 +137,23 @@ public function destroy($sessionId)
135137
if (!$this->sessionName) {
136138
throw new \LogicException(sprintf('Session name cannot be empty, did you forget to call "parent::open()" in "%s"?.', static::class));
137139
}
138-
$sessionCookie = sprintf(' %s=', urlencode($this->sessionName));
139-
$sessionCookieWithId = sprintf('%s%s;', $sessionCookie, urlencode($sessionId));
140-
$sessionCookieFound = false;
141-
$otherCookies = [];
142-
foreach (headers_list() as $h) {
143-
if (0 !== stripos($h, 'Set-Cookie:')) {
144-
continue;
145-
}
146-
if (11 === strpos($h, $sessionCookie, 11)) {
147-
$sessionCookieFound = true;
148-
149-
if (11 !== strpos($h, $sessionCookieWithId, 11)) {
150-
$otherCookies[] = $h;
151-
}
140+
$cookie = SessionUtils::popSessionCookie($this->sessionName, $sessionId);
141+
142+
/*
143+
* We send an invalidation Set-Cookie header (zero lifetime)
144+
* when either the session was started or a cookie with
145+
* the session name was sent by the client (in which case
146+
* we know it's invalid as a valid session cookie would've
147+
* started the session).
148+
*/
149+
if (null === $cookie || isset($_COOKIE[$this->sessionName])) {
150+
if (\PHP_VERSION_ID < 70300) {
151+
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));
152152
} else {
153-
$otherCookies[] = $h;
154-
}
155-
}
156-
if ($sessionCookieFound) {
157-
header_remove('Set-Cookie');
158-
foreach ($otherCookies as $h) {
159-
header($h, false);
153+
$params = session_get_cookie_params();
154+
unset($params['lifetime']);
155+
setcookie($this->sessionName, '', $params);
160156
}
161-
} else {
162-
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));
163157
}
164158
}
165159

‎src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php

Lines changed: 36 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
namespace Symfony\Component\HttpFoundation\Session\Storage;
1313

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

52+
/**
53+
* @var string|null
54+
*/
55+
private $emulateSameSite;
56+
5157
/**
5258
* Depending on how you want the storage driver to behave you probably
5359
* want to override this constructor entirely.
@@ -67,6 +73,7 @@ class NativeSessionStorage implements SessionStorageInterface
6773
* cookie_lifetime, "0"
6874
* cookie_path, "/"
6975
* cookie_secure, ""
76+
* cookie_samesite, null
7077
* entropy_file, ""
7178
* entropy_length, "0"
7279
* gc_divisor, "100"
@@ -94,9 +101,7 @@ class NativeSessionStorage implements SessionStorageInterface
94101
* trans_sid_hosts, $_SERVER['HTTP_HOST']
95102
* trans_sid_tags, "a=href,area=href,frame=src,form="
96103
*
97-
* @param array $options Session configuration options
98-
* @param \SessionHandlerInterface|null $handler
99-
* @param MetadataBag $metaBag MetadataBag
104+
* @param AbstractProxy|\SessionHandlerInterface|null $handler
100105
*/
101106
public function __construct(array $options = [], $handler = null, MetadataBag $metaBag = null)
102107
{
@@ -150,6 +155,13 @@ public function start()
150155
throw new \RuntimeException('Failed to start the session');
151156
}
152157

158+
if (null !== $this->emulateSameSite) {
159+
$originalCookie = SessionUtils::popSessionCookie(session_name(), session_id());
160+
if (null !== $originalCookie) {
161+
header(sprintf('%s; SameSite=%s', $originalCookie, $this->emulateSameSite), false);
162+
}
163+
}
164+
153165
$this->loadSession();
154166

155167
return true;
@@ -215,6 +227,13 @@ public function regenerate($destroy = false, $lifetime = null)
215227
// @see https://bugs.php.net/70013
216228
$this->loadSession();
217229

230+
if (null !== $this->emulateSameSite) {
231+
$originalCookie = SessionUtils::popSessionCookie(session_name(), session_id());
232+
if (null !== $originalCookie) {
233+
header(sprintf('%s; SameSite=%s', $originalCookie, $this->emulateSameSite), false);
234+
}
235+
}
236+
218237
return $isRegenerated;
219238
}
220239

@@ -223,6 +242,7 @@ public function regenerate($destroy = false, $lifetime = null)
223242
*/
224243
public function save()
225244
{
245+
// Store a copy so we can restore the bags in case the session was not left empty
226246
$session = $_SESSION;
227247

228248
foreach ($this->bags as $bag) {
@@ -248,7 +268,11 @@ public function save()
248268
session_write_close();
249269
} finally {
250270
restore_error_handler();
251-
$_SESSION = $session;
271+
272+
// Restore only if not empty
273+
if ($_SESSION) {
274+
$_SESSION = $session;
275+
}
252276
}
253277

254278
$this->closed = true;
@@ -347,7 +371,7 @@ public function setOptions(array $options)
347371

348372
$validOptions = array_flip([
349373
'cache_expire', 'cache_limiter', 'cookie_domain', 'cookie_httponly',
350-
'cookie_lifetime', 'cookie_path', 'cookie_secure',
374+
'cookie_lifetime', 'cookie_path', 'cookie_secure', 'cookie_samesite',
351375
'entropy_file', 'entropy_length', 'gc_divisor',
352376
'gc_maxlifetime', 'gc_probability', 'hash_bits_per_character',
353377
'hash_function', 'lazy_write', 'name', 'referer_check',
@@ -360,6 +384,12 @@ public function setOptions(array $options)
360384

361385
foreach ($options as $key => $value) {
362386
if (isset($validOptions[$key])) {
387+
if ('cookie_samesite' === $key && \PHP_VERSION_ID < 70300) {
388+
// PHP < 7.3 does not support same_site cookies. We will emulate it in
389+
// the start() method instead.
390+
$this->emulateSameSite = $value;
391+
continue;
392+
}
363393
ini_set('url_rewriter.tags' !== $key ? 'session.'.$key : $key, $value);
364394
}
365395
}
@@ -381,7 +411,7 @@ public function setOptions(array $options)
381411
* @see https://php.net/sessionhandlerinterface
382412
* @see https://php.net/sessionhandler
383413
*
384-
* @param \SessionHandlerInterface|null $saveHandler
414+
* @param AbstractProxy|\SessionHandlerInterface|null $saveHandler
385415
*
386416
* @throws \InvalidArgumentException
387417
*/

‎src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/Fixtures/storage.expected

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,11 @@ $_SESSION is not empty
1111
write
1212
destroy
1313
close
14-
$_SESSION is not empty
14+
$_SESSION is empty
1515
Array
1616
(
1717
[0] => Content-Type: text/plain; charset=utf-8
1818
[1] => Cache-Control: max-age=0, private, must-revalidate
19+
[2] => Set-Cookie: sid=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0; path=/; secure; HttpOnly
1920
)
2021
shutdown

‎src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/Fixtures/with_cookie_and_session.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,5 +20,6 @@ Array
2020
[0] => Content-Type: text/plain; charset=utf-8
2121
[1] => Cache-Control: max-age=10800, private, must-revalidate
2222
[2] => Set-Cookie: abc=def
23+
[3] => Set-Cookie: sid=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0; path=/; secure; HttpOnly
2324
)
2425
shutdown

‎src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/Fixtures/with_samesite.expected

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
open
2+
validateId
3+
read
4+
doRead:
5+
read
6+
7+
write
8+
doWrite: foo|s:3:"bar";
9+
close
10+
Array
11+
(
12+
[0] => Content-Type: text/plain; charset=utf-8
13+
[1] => Cache-Control: max-age=0, private, must-revalidate
14+
[2] => Set-Cookie: sid=random_session_id; path=/; secure; HttpOnly; SameSite=lax
15+
)
16+
shutdown

‎src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/Fixtures/with_samesite.php

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<?php
2+
3+
require __DIR__.'/common.inc';
4+
5+
use Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage;
6+
7+
$storage = new NativeSessionStorage(['cookie_samesite' => 'lax']);
8+
$storage->setSaveHandler(new TestSessionHandler());
9+
$storage->start();
10+
11+
$_SESSION = ['foo' => 'bar'];
12+
13+
ob_start(function ($buffer) { return str_replace(session_id(), 'random_session_id', $buffer); });

0 commit comments

Comments
 (0)