Add SameSite cookies to FrameWorkBundle #28168

rpkamp · 2018-08-08T20:26:31Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes, and added to changelog https://github.com/symfony/symfony/pull/28168/files#diff-276f5b13978c2ce3f555b9603f44801aR21
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#27631
License	MIT
Doc PR	symfony/symfony-docs#10202

Uses session.cookie_samesite for PHP >= 7.3. For PHP < 7.3 it first
does a session_start(), find the emitted header, changes it, and emits
it again with the value for SameSite added.

I also tried it in a minimal Symfony 4.1 app, and works there too:

nicolas-grekas

missing in NativeSessionStorageTest.php:

@@ -170,6 +170,9 @@ class NativeSessionStorageTest extends TestCase
             'cookie_secure' => true,
             'cookie_httponly' => false,
         );
+        if (\PHP_VERSION_ID >= 70300) {
+            $options['cookie_samesite'] = '';
+        }
 
         $this->getStorage($options);

nicolas-grekas · 2018-08-15T17:30:41Z

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

-                }
-            } else {
+            $cookie = HeaderUtils::popSessionCookie($this->sessionName, $sessionId);
+            if (null === $cookie) {
                setcookie($this->sessionName, '', 0, ini_get('session.cookie_path'), ini_get('session.cookie_domain'), ini_get('session.cookie_secure'), ini_get('session.cookie_httponly'));


no need for samesite here also?

This sets the session id to an empty value in the cookie, meaning the session won't work. I don't think adding SameSite here would add anything.

Looking at this some more, isn't this code supposed to remove the cookie entirely? In that case the value 0 is incorrect, that should be e.g. time() - 3600.

isn't this code supposed to remove the cookie entirely

indeed, my bad

the value 0 is incorrect

I don't understand why 0 is invalid here. It's a timestamp in the past that's fine for removing a cookie AFAIK

From the manual on setcookie:

If set to 0, or omitted, the cookie will expire at the end of the session (when the browser closes).

oups, would you mind opening a PR on 3.4 where this has been introduced?

No problem, I'll do that later.

Do we actually need a calculated value here, or wouldn't 1 be sufficient? (Edit: maybe with a comment explaining it)

I'll test it locally to be sure, but it seems to me that 1 would work 👍

So I tested this locally and the results are not as expected. When I send a cookie with an empty value and an expires of 0 it actually does delete the cookie. Turns out further down the manual page it says:

If the value argument is an empty string, or FALSE, and all other arguments match a previous call to setcookie, then the cookie with the specified name will be deleted from the remote client. This is internally achieved by setting value to 'deleted' and expiration time to one year in past.

Which is actually only partly true from what I've seen as:

It doesn't set the expiration time to one year in the past, it sets it to 1970-01-01 00:00:01

The value of $httponly doesn't seem to matter; If a cookie was set with http false and you delete it with http true it gets deleted all the same, even though the documentation suggests it would not

If I create a cookie with SameSite=lax and remove it without any SameSite option, it still deletes the cookie

It does however always send a Set-Cookie header, it's just that it doesn't get picked up by the browser as a command to delete the previous cookie.

I've only tested in chrome, so the actual behaviour of when the header is honoured and when not might vary between browsers as well.

Maybe it would be best to add ini_get('session.cookie_samesite') to the call if it's PHP >= 7.3 just to be sure?

I've added the samesite parameter for PHP >= 7.3 just to be sure.

nicolas-grekas · 2018-08-15T17:31:21Z

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

@@ -359,6 +374,12 @@ public function setOptions(array $options)

        foreach ($options as $key => $value) {
            if (isset($validOptions[$key])) {
+                if ('cookie_samesite' === $key && \PHP_VERSION_ID < 703000) {


too many zeros: 70300

Nice catch, will fix that 👍

rpkamp · 2018-08-16T17:08:07Z

I've added the cookie_samesite option to \Symfony\Component\HttpFoundation\Tests\Session\Storage\NativeSessionStorageTest, only I've set it to the value of "lax" instead of "" as I think that's a more solid test; any string can be empty, getting a specific value is harder.

stof · 2018-08-16T17:34:19Z

src/Symfony/Component/HttpFoundation/HeaderUtils.php

+     * 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(string $sessionName, string $sessionId): ?string


This method is really different from the other ones, as it does not deal with a header value, but with PHP headers themselves. So I would not expose it here as a public utility. I would keep it as @internal somewhere else (especially given than one of the 2 places using it will disappear when dropping support for PHP 7.2 and older)

Good point. Any suggestion on where to keep it? Maybe introduce a Symfony\HttpFoundation\Session\SessionUtils class?

@stof I've created a new Symfony\HttpFoundation\Session\SessionUtils class, marked it as internal and moved it there. Does that seem better to you?

guilliamxavier · 2018-08-17T08:11:02Z

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

@@ -359,6 +374,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


(obvious but anyway:) PHP < 7.3

Indeed. Fixed.

rpkamp · 2018-08-19T07:36:33Z

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

+/**
+ * Session utility functions.
+ *
+ * @author Nicolas Grekas <p@tchwork.com>


@nicolas-grekas I've added you here since this was originally implemented by you but I've moved it here. Is that ok?

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

rpkamp · 2018-08-20T11:52:45Z

I've just added the change to CHANGELOG.md and created a PR for the docs: symfony/symfony-docs#10202

nicolas-grekas · 2018-08-22T20:34:30Z

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

@@ -482,6 +483,7 @@ private function addSessionSection(ArrayNodeDefinition $rootNode)
                        ->scalarNode('cookie_domain')->end()
                        ->booleanNode('cookie_secure')->end()
                        ->booleanNode('cookie_httponly')->defaultTrue()->end()
+                        ->enumNode('cookie_samesite')->values(array(null, Cookie::SAMESITE_LAX, Cookie::SAMESITE_STRICT))->defaultNull()->end()


what about setting it to lax by default? this would improve the security of all Symfony apps by default
alternatively, we should make it the default in the flex recipe

That would be a BC break as it could break some apps that post across domains by design.

Setting it as default in the flex recipe sounds good though!

Does this use case really exist? It would be great to improve the security of everyone by default if not. Worth thinking twice here.

Maybe just put it in the flex recipe for now and default it to Lax in the framework for Symfony 5?

If we really think this is a BC break, we cannot change it in 5 without first issuing a notice in 4...
Can you add a strong recommendation in the upgrading file also btw?

@rpkamp, could you please add a note in the upgrading files (4.2&5.0) to suggest enabling the lax mode?

Yes, I will do that later today or else tomorrow.

@nicolas-grekas done. There are no UPGRADE-4.2.md yet, so I've created one.

Never mind, there are already was an UPGRADE-4.2.md on master, I've just rebased against master.

Uses `session.cookie_samesite` for PHP >= 7.3. For PHP < 7.3 it first does a session_start(), find the emitted header, changes it, and emits it again with the value for SameSite added.

rpkamp · 2018-08-28T18:34:58Z

I'm not sure why Travis is failing now. It seems to be missing the symfony/var-dumper package. Maybe that's broken on master now?

nicolas-grekas

Nice (yes, master is broken for now.)

fabpot

with a small comment regarding CS

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

fabpot · 2018-09-04T08:28:56Z

Thank you @rpkamp.

This PR was merged into the 4.2-dev branch. Discussion ---------- Add SameSite cookies to FrameWorkBundle | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes, and added to changelog https://github.com/symfony/symfony/pull/28168/files#diff-276f5b13978c2ce3f555b9603f44801aR21 | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #27631 | License | MIT | Doc PR | symfony/symfony-docs#10202 Uses `session.cookie_samesite` for PHP >= 7.3. For PHP < 7.3 it first does a session_start(), find the emitted header, changes it, and emits it again with the value for SameSite added. I also tried it in a minimal Symfony 4.1 app, and works there too: ![screenshot from 2018-08-08 21-39-10](https://user-images.githubusercontent.com/1059790/43864708-b7437978-9b60-11e8-81dd-b41f1a5afb52.png) Commits ------- 4091feb Add SameSite cookies to FrameWorkBundle

nicolas-grekas · 2018-09-04T08:41:33Z

Recipe update in symfony/recipes#452

nicolas-grekas · 2018-09-13T18:39:33Z

@rpkamp I just found that src/Symfony/Component/HttpKernel/EventListener/AbstractTestSessionListener.php contains an occurrence of session_get_cookie_params() that may miss a BC/FC layer for the samesite support! Would you mind having a look please? See

symfony/src/Symfony/Component/HttpKernel/EventListener/AbstractTestSessionListener.php

Line 75 in 8ab7077

$params = session_get_cookie_params();

rpkamp · 2018-09-13T19:18:09Z

It would be quite easy to make it compatible with PHP 7.3 as the value for samesite will be returned from the session_get_cookie_params function in that case, but getting the cookie_samesite framework option (or rather the %session.storage.options% kernel parameter) into the AbstractTestSessionListener class would be rather hard, as we would have to add a method getSessionParameters() to the AbstractTestSessionListener class (BC break!), pass the parameter to the TestSessionListener constructor (another BC break), and use that get to the samesite option.

Note that this also means that currently the AbstractTestSessionListener is using none of the options set in the framework configuration whatsoever, as it doesn't have access to them. So maybe that is what should be fixed entirely, and use all values set in there (if any) instead of relying on the values in php.ini, which might differ with what was set in the framework settings? It would make for a more realistic test setup.

Or maybe all of this isn't really an issue as test drivers probably don't know how to handle samesite anyway, and if they do almost nobody is doing cross site testing? It certainly would be the edgiest of edge cases if the lack of samesite in that class would cause a false positive in tests for someone IMHO.

nicolas-grekas · 2018-09-13T20:41:06Z

What about passing the session options as a constructor argument to AbstractTestSessionListener?
I feel like this would be enough, isn't it?

rpkamp · 2018-09-14T04:49:12Z

No, because TestSessionListener extends that abstract class so it has to be in the constructor of that class as well.

tcz · 2018-10-02T20:35:00Z

Any chance this will be posted to Symfony 3.x? If not, any workaround?

jayesbe · 2020-01-29T12:49:56Z

I realise this is a merged issue.. but 3.4 LTS is still considered a maintained branch.. SameSite is now a requirement.

A cookie associated with a cross-site resource at was set without the SameSite attribute. A future release of Chrome will only deliver cookies with cross-site requests if they are set with SameSite=None and Secure.

nicolas-grekas · 2020-01-29T13:56:01Z

we default to samesite=lax now, don't we?
if you think we should discuss something on the topic, please open an issue with the details as comments on a closed PR will be forgotten for sure.

jayesbe · 2020-01-30T10:49:15Z

@nicolas-grekas thanks.

My project is on Symfony 3.4 LTS

This

  session:
      # storage_id: app.dynamic.session.storage
      handler_id: ~
      name: "%session.name%"
      cookie_secure: true
      cookie_httponly: true
      cookie_lifetime: 2630000 # 1-Month
      cookie_samesite: strict

results in

Unrecognized option "cookie_samesite" under "framework.session"

Will open a new issue now.

… 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

carsonbot added Status: Needs Review Feature labels Aug 8, 2018

rpkamp force-pushed the session-samesite branch from 2051275 to fa6ce44 Compare August 8, 2018 20:29

rpkamp mentioned this pull request Aug 8, 2018

[Security] Ability to set SameSite attribute for session and remember me cookie #27631

Closed

rpkamp force-pushed the session-samesite branch 2 times, most recently from 4505b9a to 559036a Compare August 8, 2018 21:04

nicolas-grekas added this to the next milestone Aug 9, 2018

nicolas-grekas reviewed Aug 15, 2018

View reviewed changes

rpkamp force-pushed the session-samesite branch from 559036a to 44c8458 Compare August 16, 2018 17:06

stof reviewed Aug 16, 2018

View reviewed changes

guilliamxavier reviewed Aug 17, 2018

View reviewed changes

rpkamp force-pushed the session-samesite branch 2 times, most recently from 156c923 to b0cce6c Compare August 19, 2018 07:28

rpkamp commented Aug 19, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Aug 19, 2018

nicolas-grekas approved these changes Aug 19, 2018

View reviewed changes

rpkamp force-pushed the session-samesite branch from b0cce6c to 69b17f1 Compare August 20, 2018 11:19

fabpot requested changes Aug 20, 2018

View reviewed changes

src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Reviewed labels Aug 20, 2018

rpkamp mentioned this pull request Aug 20, 2018

Add documentation on cookie_samesite setting in FrameworkBundle symfony/symfony-docs#10202

Closed

nicolas-grekas reviewed Aug 22, 2018

View reviewed changes

rpkamp force-pushed the session-samesite branch from 69b17f1 to 432f3be Compare August 28, 2018 18:15

Add SameSite cookies to FrameWorkBundle

4091feb

Uses `session.cookie_samesite` for PHP >= 7.3. For PHP < 7.3 it first does a session_start(), find the emitted header, changes it, and emits it again with the value for SameSite added.

rpkamp force-pushed the session-samesite branch from 432f3be to 4091feb Compare August 28, 2018 18:18

nicolas-grekas approved these changes Aug 28, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Aug 28, 2018

fabpot approved these changes Sep 4, 2018

View reviewed changes

src/Symfony/Component/HttpFoundation/Session/SessionUtils.php Show resolved Hide resolved

fabpot merged commit 4091feb into symfony:master Sep 4, 2018

nicolas-grekas mentioned this pull request Sep 4, 2018

[symfony/framework-bundle] Secure session defaults with "cookie_secure: auto" and "cookie_samesite: lax" symfony/recipes#452

Merged

rpkamp deleted the session-samesite branch October 3, 2018 07:22

nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

This was referenced Nov 3, 2018

Release v4.2.0-BETA1 #29072

Closed

Release v4.2.0-BETA1 #29073

Merged

rpkamp mentioned this pull request Jul 9, 2019

[HttpFoundation] Clear invalid session cookie #32455

Merged

nicolas-grekas mentioned this pull request Feb 5, 2020

[HttpFoundation][FrameworkBundle] fix support for samesite in session cookies #35605

Merged

Add SameSite cookies to FrameWorkBundle #28168

Add SameSite cookies to FrameWorkBundle #28168

Conversation

rpkamp commented Aug 8, 2018 • edited

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Aug 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apfelbox Aug 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpkamp Aug 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpkamp commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpkamp commented Aug 20, 2018

Choose a reason for hiding this comment

rpkamp Aug 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Aug 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpkamp commented Aug 28, 2018

nicolas-grekas left a comment

Choose a reason for hiding this comment

fabpot left a comment

Choose a reason for hiding this comment

fabpot commented Sep 4, 2018

nicolas-grekas commented Sep 4, 2018

nicolas-grekas commented Sep 13, 2018 • edited

rpkamp commented Sep 13, 2018 • edited

nicolas-grekas commented Sep 13, 2018

rpkamp commented Sep 14, 2018

tcz commented Oct 2, 2018

jayesbe commented Jan 29, 2020

nicolas-grekas commented Jan 29, 2020

jayesbe commented Jan 30, 2020

rpkamp commented Aug 8, 2018 •

edited

nicolas-grekas Aug 16, 2018 •

edited

apfelbox Aug 16, 2018 •

edited

rpkamp Aug 16, 2018 •

edited

rpkamp Aug 23, 2018 •

edited

nicolas-grekas Aug 28, 2018 •

edited

nicolas-grekas commented Sep 13, 2018 •

edited

rpkamp commented Sep 13, 2018 •

edited