Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Add SameSite cookies to FrameWorkBundle#28168

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromrpkamp:session-samesite
Sep 4, 2018

Conversation

@rpkamp
Copy link
Contributor

@rpkamprpkamp commentedAug 8, 2018•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes, and added to changeloghttps://github.com/symfony/symfony/pull/28168/files#diff-276f5b13978c2ce3f555b9603f44801aR21
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#27631
LicenseMIT
Doc PRsymfony/symfony-docs#10202

Usessession.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

dkarlovi reacted with thumbs up emoji
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

no need for samesite here also?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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 value0 is incorrect, that should be e.g.time() - 3600.

Copy link
Member

@nicolas-grekasnicolas-grekasAug 16, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Fromthe manual on setcookie:

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

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

No problem, I'll do that later.

Copy link
Contributor

@apfelboxapfelboxAug 16, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

@rpkamprpkampAug 16, 2018•
edited
Loading

Choose a reason for hiding this comment

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

So I tested this locally and the results are not as expected. When I send a cookie with an empty value and anexpires of 0 it actuallydoes 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 httpfalse and you delete it withhttp 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 aSet-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 addini_get('session.cookie_samesite') to the call if it's PHP >= 7.3 just to be sure?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

too many zeros: 70300

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Nice catch, will fix that 👍

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed

@rpkamp
Copy link
ContributorAuthor

I've added thecookie_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.

* Find the session header amongst the headers that are to be sent, remove it, and return
* it so the caller can process it further.
*/
publicstaticfunctionpopSessionCookie(string$sessionName,string$sessionId): ?string
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

foreach ($optionsas$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
Copy link
Contributor

Choose a reason for hiding this comment

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

(obvious but anyway:)PHP < 7.3

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Indeed. Fixed.

guilliamxavier reacted with thumbs up emoji
@rpkamprpkampforce-pushed thesession-samesite branch 2 times, most recently from156c923 tob0cce6cCompareAugust 19, 2018 07:28
/**
* Session utility functions.
*
* @author Nicolas Grekas <p@tchwork.com>
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

nicolas-grekas reacted with thumbs up emoji
@rpkamp
Copy link
ContributorAuthor

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

xabbuh reacted with thumbs up emoji

->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()

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

@rpkamprpkampAug 23, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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!

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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?

Copy link
Member

@nicolas-grekasnicolas-grekasAug 28, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

nicolas-grekas reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

Uses `session.cookie_samesite` for PHP >= 7.3. For PHP < 7.3 it firstdoes a session_start(), find the emitted header, changes it, and emitsit again with the value for SameSite added.
@rpkamp
Copy link
ContributorAuthor

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

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

Nice (yes, master is broken for now.)

Copy link
Member

@fabpotfabpot left a comment

Choose a reason for hiding this comment

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

with a small comment regarding CS

@fabpot
Copy link
Member

Thank you@rpkamp.

rpkamp reacted with laugh emoji

@fabpotfabpot merged commit4091feb intosymfony:masterSep 4, 2018
fabpot added a commit that referenced this pull requestSep 4, 2018
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 changeloghttps://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#10202Uses `session.cookie_samesite` for PHP >= 7.3. For PHP < 7.3 it firstdoes a session_start(), find the emitted header, changes it, and emitsit 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
Copy link
Member

Recipe update insymfony/recipes#452

rpkamp reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedSep 13, 2018•
edited
Loading

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

@rpkamp
Copy link
ContributorAuthor

rpkamp commentedSep 13, 2018•
edited
Loading

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

Note that this also means that currently theAbstractTestSessionListener is usingnone 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 inphp.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
Copy link
Member

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

@rpkamp
Copy link
ContributorAuthor

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

@tcz
Copy link

tcz commentedOct 2, 2018

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

@rpkamprpkamp deleted the session-samesite branchOctober 3, 2018 07:22
@nicolas-grekasnicolas-grekas modified the milestones:next,4.2Nov 1, 2018
This was referencedNov 3, 2018
@jayesbe
Copy link

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 theSameSite attribute. A future release of Chrome will only deliver cookies with cross-site requests if they are set withSameSite=None andSecure.

@nicolas-grekas
Copy link
Member

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
Copy link

@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.

fabpot added a commit that referenced this pull requestFeb 7, 2020
… 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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@stofstofstof left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+2 more reviewers

@apfelboxapfelboxapfelbox left review comments

@guilliamxavierguilliamxavierguilliamxavier left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

9 participants

@rpkamp@fabpot@nicolas-grekas@tcz@jayesbe@stof@apfelbox@guilliamxavier@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp