"Cannot change the ID of an active session." after upgrading to 5.4

[devnix]

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 running 5.4.* and branch 5.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

[javiereguiluz]

Thanks for creating such a great issue report, with a bug reproducer and insights about the possible cause of this issue!

[romaricdrigon]

Same issue here - we have random session failures, and the patch solves it.

Related to #44546 I believe

[thinkawitch]

edited: my mistake, posted in wrong window.

[devnix]

Hi @thinkawitch, I think your PR is not related to this one.

[alexander-schranz]

@devnix Thx for the reproducer 👍 great issue.
Can you give #44634 a try? To check if that would fix your issue in your project also?

[devnix]

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

[alexander-schranz]

@devnix thank you for testing it out so quickly! I will try to have a look at the other issue at the evening.

[alexander-schranz]

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

[devnix]

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


Which logic seems pretty incorrect to me for the php bridge driver, as the session may be shared with a legacy application. In my case, there are data stored into the session by the legacy app, and Symfony is removing the PHPSESSID cookie without hesitation.

[devnix]

It's still not replicated in the reproducer because $session->isStarted() evaluates to false: https://github.com/symfony/symfony/blob/v5.4.0/src/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.php#L109

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.

[alexander-schranz]

@devnix you can also use the https://github.com/symfony/demo as a start point. That already has security in it.

[devnix]

Reproduced! devnix/symfony-sessions-messed-up@00f37fc

Enabling a - { path: ^/, roles: PUBLIC_ACCESS } line in access_control starts this behavior if Symfony session bags are empty. In that case, Symfony will destroy the whole session cookie, even if it is used by other applications.

[siguycmo]

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:

if (PHP_SESSION_NONE === session_status()) {
    session_start();
}

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.

[alexander-schranz]

@siguycmo would be great if you could give the changes in #44634 a try to know if that would fix it in your case too.

[chrisNemo]

In my case, I replaced both Files of #44634 and it worked perfectly.
thx

[alexander-schranz]

@chrisNemo Thx for the feedback!

[mbtreetime]

@siguycmo would be great if you could give the changes in #44634 a try to know if that would fix it in your case too.

#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

[siguycmo]

@siguycmo would be great if you could give the changes in #44634 a try to know if that would fix it in your case too.

@alexander-schranz @mbtreetime The changes in #44634 did not work for me, I think my legacy app starting the session was my issue.

[alexander-schranz]

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

[devnix]

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

[mbtreetime]

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 isStarted() method that returns !empty($_SESSION) and explicitly make sure session is started in Kernel.php.

composer.json

  "autoload": {
    "psr-4": {
      "Symfony\\": "classes/Symfony/"
    },
    "exclude-from-classmap": ["vendor/symfony/http-foundation/Session/Storage/PhpBridgeSessionStorage.php"]
  }

classes/symfony/http-foundation/Session/Storage/PhpBridgeSessionStorage.php

diff --git a/vendor/symfony/http-foundation/Session/Storage/PhpBridgeSessionStorage.php [b/classes/Symfony/Component/HttpFoundation/Session/Storage/PhpBridgeSessionStorage.php](url)
--- a/vendor/symfony/http-foundation/Session/Storage/PhpBridgeSessionStorage.php	
+++ b/classes/Symfony/Component/HttpFoundation/Session/Storage/PhpBridgeSessionStorage.php	(date 1641424542633)
@@ -61,10 +61,4 @@
         // reconnect the bags to the session
         $this->loadSession();
     }
-
-    
-    public function isStarted()
-    {
-        return !empty($_SESSION);
-    }
 }

adding session_start() to Kernel.php::boot() and setting a variable in order to pass !empty($_SESSION)

        // Start session for legacy compatibility
        if (\PHP_SESSION_NONE === session_status()) {
            session_start();
        }
        $_SESSION['started'] = true;