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

"Cookie encryption improvements" in 7.22.0+ are a breaking change #33940

Closed
qooplmao opened this issue Aug 19, 2020 · 4 comments
Closed

"Cookie encryption improvements" in 7.22.0+ are a breaking change #33940

qooplmao opened this issue Aug 19, 2020 · 4 comments

Comments

@qooplmao
Copy link

qooplmao commented Aug 19, 2020

  • Laravel Version: >=7.22.0

Description:

In #33662 a prefix is added to cookie values. When the cookie is then decrypted the prefix is checked and set or the cookie nulled. This means that session id's stored using the <7.22 format will no longer be valid and the session will be lost.

I have a legacy (5.6) application and a new (7.25) application that share a session cookie, with the login being handled by the legacy app. On updating from 7.20 to 7.25 I kept getting logged out when going to any routes on the new app.

I would class this as a breaking change so would assume this would/should be moved to version 8, or at least have the usage of the prefix be configurable with the non-prefixed version being the default.

Steps To Reproduce:

  • Create a Laravel application with using version 7.20.0 of laravel/framework
  • Login and go to a route that requires authentication
  • Update laravel/framework to >=7.22.0
  • Refresh page
  • User will now be logged out of application and forwarded to login page
@morloderex
Copy link
Contributor

This is a breaking change. However it is breaking but it also fixes a security vulnerability prior to 7.22, and given that this is the case I would argue that it should not be reverted as security vulnerabilities should be patched regardless of any breaking changes.

However I do believe the Laravel team has patched the same issues for 5.6 also. But if I remember correctly it was also stated as a breaking changes in the change log.

@qooplmao
Copy link
Author

Yeah, no worries. I would argue that any breaking change, security fix or otherwise, should require some kind of mitigation or opt-in until the next major but I would guess that I would be in the minority.

5.6 hit the end of life in February of 2019 so I wouldn't expect any patches to be in there. The latest tagged version (5.6.40) doesn't have the updated code in place but it's not complicated to mitigate and/or port the changes.

Closing as it's a known BC.

@driesvints
Copy link
Member

I have a legacy (5.6) application and a new (7.25) application that share a session cookie

Even besides the breaking change it might be good for you to know that we don't support this. So it's at your own risk that this breaks (just saying).

@qooplmao
Copy link
Author

Even besides the breaking change it might be good for you to know that we don't support this. So it's at your own risk that this breaks (just saying).

I understand 100% but thanks for the heads-up.

The legacy app is such a brittle mess, coupled with the fact that most of the functionality is being deprecated anyway, that updating in place isn't really a viable, or productive, option. This was the lowest impact way to be able to run both versions in tandem rather than having to have a complete rebuild on day 1. It's definitely not something that I want to be handling for the longterm.

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

No branches or pull requests

3 participants