Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.x] Cleanup session config #5261

Merged
merged 2 commits into from Mar 13, 2020
Merged

[7.x] Cleanup session config #5261

merged 2 commits into from Mar 13, 2020

Conversation

mathieutu
Copy link
Contributor

@mathieutu mathieutu commented Mar 13, 2020

Hi,
Several changes reverted from #5157 (Symfony 5 update):

I'm doing an other PR to the upgrade guide in the doc to update that.

Thanks,
Matt'


EDIT: Updated to reflect actual PR by @GrahamCampbell

@taylorotwell taylorotwell merged commit f93f4ad into laravel:master Mar 13, 2020
@taylorotwell
Copy link
Member

Keeping same-site lax as it is a more secure default for session cookies IMO.

@GrahamCampbell GrahamCampbell changed the title Fix session config changes [7.x] Fix session config changes Mar 13, 2020
@GrahamCampbell
Copy link
Member

Just to clarify, this PR actually changes nothing at all...

@GrahamCampbell
Copy link
Member

Nothing was reverted, only a redundant explicit default was removed.

@GrahamCampbell GrahamCampbell changed the title [7.x] Fix session config changes [7.x] Cleanup session config Mar 13, 2020
@mfn
Copy link
Contributor

mfn commented Mar 14, 2020

Nothing was reverted, only a redundant explicit default was removed.

Does that mean the entry in the upgrade guide may not necessary?

https://laravel.com/docs/7.x/upgrade#symfony-5-related-upgrades

Next, please update your session configuration file's secure option to have a fallback value of null:

thx

@GrahamCampbell
Copy link
Member

Nothing was reverted, only a redundant explicit default was removed.

No I mean that this PR did not revert anything, not that nothing was changed between 6 and 7.

@GrahamCampbell
Copy link
Member

That line is still needed. env('SESSION_SECURE_COOKIE', null) is the same as env('SESSION_SECURE_COOKIE'), so this PR can be viewed as just a refactoring.

@mathieutu
Copy link
Contributor Author

@GrahamCampbell The PR doesn't revert anything and is just about documentation now, because of the Taylor commit, and you know it. My PR was not about refactoring this little env default, but reverting the 'lax' default value.

I understand the wish to keep it: the lax value is following a new security choice, but it apparently wasn't an upgrade need (you know, editing my message and crossing the point doesn't make it less relevant).

So please, do not feel hurt about that, you made an amazing job updating theses packages, so no need to ruin everything by being rude 🙂.

Thanks for your work and see you on the next PR 😉.

@GrahamCampbell
Copy link
Member

GrahamCampbell commented Mar 16, 2020

My comment wasn't directed at you, and wasn't meant to be rude. It was meant to help people who landed on this PR and got confused by the PR title. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants