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

Use file session driver again #5201

Merged
merged 2 commits into from Jan 14, 2020
Merged

Use file session driver again #5201

merged 2 commits into from Jan 14, 2020

Conversation

@aimeos
Copy link
Contributor

aimeos commented Jan 8, 2020

The maximum amount of cookie data per domain is ~4k bytes and this change returns empty data if more data is stored in the session. In our case, the session contains the shopping cart data, which are almost always above the limit

aimeos added 2 commits Jan 8, 2020
@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Jan 8, 2020

Change it in your own file. I like this default better.

@aimeos

This comment has been minimized.

Copy link
Contributor Author

aimeos commented Jan 8, 2020

@taylorotwell Please be aware that the default cookie session storage is also a security problem because users can change the session data on their local machine. This can lead to privilege escalation and remote data injection. There are good reasons why session data isn't stored at the client machine by default and that Laravel shouldn't do this by default too regardless of personal preference!

@narwy

This comment has been minimized.

Copy link
Contributor

narwy commented Jan 10, 2020

I do agree with the default being file

@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Jan 13, 2020

@aimeos They can't change the cookie (or any other Laravel cookie) on their local machine. They are signed and encrypted.

@narwy

This comment has been minimized.

Copy link
Contributor

narwy commented Jan 13, 2020

@taylorotwell But can be decrypted if someone gains access to your secret 🤷‍♂

@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Jan 13, 2020

If they gain access to your secret, you have big problems regardless of your driver. They can modify your session identifier cookie to another user even with the file driver at that point.

@narwy

This comment has been minimized.

Copy link
Contributor

narwy commented Jan 13, 2020

@taylorotwell But surely you'd minimise potential risk if it's file based by default.

@aimeos

This comment has been minimized.

Copy link
Contributor Author

aimeos commented Jan 13, 2020

@taylorotwell OK, then security may not be a big problem. Nevertheless, the maximum limit of all cookies for a domain (not for each cookie) is still 4096 bytes and that can be reached easily. Laravel was always known for defaults that work for 95% of all applications out of the box. Now it may be working for 70-80% only and it's frustrating for users if something doesn't work without a hint why this is the case.

@alec-lefors

This comment has been minimized.

Copy link

alec-lefors commented Jan 13, 2020

This new default throws 419s on all POST requests if the cookie is too big and was very hard to debug. It’s a shame a default setting doesn’t have better error handling other than 419s and the occasional apache error when the requests get ridiculously long.

@driesvints driesvints reopened this Jan 14, 2020
@driesvints driesvints merged commit 9d0862b into laravel:master Jan 14, 2020
1 check passed
1 check passed
continuous-integration/styleci/pr The analysis has passed
Details
@driesvints

This comment has been minimized.

Copy link
Member

driesvints commented Jan 14, 2020

After talking this over with @taylorotwell, we've decided to revert this change. Thanks for all the input on this 👍

@Tjoosten Tjoosten mentioned this pull request Jan 14, 2020
@IlyaVorozhbit

This comment has been minimized.

Copy link

IlyaVorozhbit commented on f44f065 Jan 17, 2020

Why it not squashed?
I have seen it a second time, at least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.