-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
"Cannot change the ID of an active session." after upgrading to 5.4 #44553
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
Comments
Thanks for creating such a great issue report, with a bug reproducer and insights about the possible cause of this issue! |
Same issue here - we have random session failures, and the patch solves it. Related to #44546 I believe |
edited: my mistake, posted in wrong window. |
Hi @thinkawitch, I think your PR is not related to this one. |
Hi @alexander-schranz, thank you! It solves the first part of the issue, which is the title of the ticket. The problem is that the PHPSESSID cookie is regenerated every two requests. I think this is the responsible part of that behavior: https://github.com/symfony/symfony/blob/v5.4.0/src/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.php#L136-L183 |
@devnix thank you for testing it out so quickly! I will try to have a look at the other issue at the evening. |
@devnix I'm not able to reproduce to lose sessions after every second requests tested on http and https: Do you have any hints? PHP Version, PHP Session settings?, PHP Cookie Settings? |
@alexander-schranz I'm still trying to replicate on my reproducer, but right now it's only happening only on my project. I'll let you know if I archieve to isolate it. Anyway, while debugging, I see that the cookie is explicitly removed if the session is considered empty: |
It's still not replicated in the reproducer because In my project is happening because of the security bundle starting the session, so I'm trying to add the same behavior in the reproducer repository. |
@devnix you can also use the https://github.com/symfony/demo as a start point. That already has security in it. |
Reproduced! devnix/symfony-sessions-messed-up@00f37fc Enabling a |
I have also started getting this issue on symfony 5.4 after upgrading from 5.2, used in a legacy system that already handled sessions using native PHP functions. It was happening for me everytime when I would logout. I managed to get this resolved by removing a session check in my legacy app which looked like this:
I know this wont work for everyone, but it may be worth checking that the legacy app isnt also starting the session before symfony. And of course this may still be an issue with symfony core. |
In my case, I replaced both Files of #44634 and it worked perfectly. |
@chrisNemo Thx for the feedback! |
#44634 changes to AbstractSessionListener.php also fixed my legacy app with the same error message. Too bad this didn't make it into 5.4.2, hopefully it's merged soon, I was upgrading our legacy app which uses the Legacy Route Loader approach from 5.3 to 5.4 |
@alexander-schranz @mbtreetime The changes in #44634 did not work for me, I think my legacy app starting the session was my issue. |
@siguycmo would it be possible that you provide a reproducer: https://symfony.com/doc/current/contributing/code/reproducer.html. Which I can test then, that the legacy app is starting the session should with the PR be fine. Do you use the Session PHP Bridge in your case? |
@alexander-schranz I was about to post that my reproducer keeps refreshing the PHPSESSID, but I missed this: devnix/symfony-sessions-messed-up@4c4a55b Now it seems like it works, but if you comment the new line, you will see a new PHPSESSID for each request, not sure if it would be a desired behavior, but the bug would look fixed! |
Since there doesn't seem to be a resolution forthcoming in #44634 I'm considering just making a copy of PhpBridgeSessionStorage.php and adding an composer.json
classes/symfony/http-foundation/Session/Storage/PhpBridgeSessionStorage.php
adding
|
… started php sessions (alexander-schranz) This PR was squashed before being merged into the 5.4 branch. Discussion ---------- [HttpKernel] Fix compatibility with php bridge and already started php sessions | Q | A | ------------- | --- | Branch? | 5.4 for bug fixes <!-- see below --> | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | Fix #44553, Fix #44616 <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead --> | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> Fix compatibility to PHPBridge with new session handling. TODO: - [x] Test Commits ------- 4052ec1 [HttpKernel] Fix compatibility with php bridge and already started php sessions
Symfony version(s) affected
5.4.0, 5.4.1
Description
We have a legacy application that we are slowly migrating to Symfony, which we did following this guide with good results since 5.0: https://symfony.com/doc/current/session/php_bridge.html
Since we upgraded to 5.4.0 from 5.3.*, we are observing a
Cannot change the ID of an active session.
intermitently.How to reproduce
I've been able to reproduce in an isolated project: https://github.com/devnix/symfony-sessions-messed-up
master
branch is running5.4.*
and branch5.3
is showing how it works.Possible Solution
I've been able to track down the code affecting the sessions this way here: b3e4f66
https://github.com/symfony/symfony/blob/v5.4.0/src/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.php#L72-L87
https://github.com/symfony/symfony/blob/v5.4.0/src/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.php#L136-L183
Monkey patching the class without these portions of code makes it work again.
I understand that this is a neat solution for new runtimes like Roadrunner, but it's breaking compatibility with how sessions worked. Maybe the solution would be to be able to opt-in to this functionality? Should the documentation about migrating legacy applications be updated?
Additional Context
No response
The text was updated successfully, but these errors were encountered: