@alexander-schranz maybe you could review this please ?

@simonchrz sorry for the noice. Overall I think we should need to duplicate here some thing from theNativeSessionStorage to solve this problem correctly.

I would go with the following in the__construct method so we are sure the value inside$this->sessionOptions is always the correct one:

$this->sessionOptions = [];foreach (session_get_cookie_params()as$key =>$value) {$this->sessionOptions['cookie_' .$key] =>$value;}foreach ($sessionOptionsas$key =>$value) {// do the same logic as in the NativeSessionStorage// @see https://github.com/symfony/symfony/blob/v5.4.0/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php#L392-L394if (isset($validOptions[$key])) {if ('cookie_secure' ===$key &&'auto' ===$value) {continue;        }$this->sessionOptions[$key] =$value;   }}

We need to check if symfony is forcing some specific values inhttps://github.com/symfony/symfony/blob/v5.4.0/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php specially if samesite work correctly still for lower PHP Versions.

@simonchrz Are you able to write a test for your change? I really want to make sure, we don't break this behavior again.

@alexander-schranz i've followed your suggestion, please double check. 👍
@derrabus added a test, too.

@simonchrz as I see there is also issue aboutcookie_secure: auto not correctly working. And this PR also seems to fix this. Can you add an additional test case wherecookie_secure: auto is set by symfony and php has it set to false/off, before it seems to be always true in auto mode.

@simonchrz@alexander-schranz I don't think this completely fixes the problem with theauto setting. ThemergePhpSessionOptions thing gets called too early for that.

In a normal Symfony request lifecycle the session listener is one of the first things that gets called on thekernel.request event which means it's one of the first listeners that gets constructed. TheonKernelRequest method sets the session factory callback which gets triggered the first time somebody tries to access the session. That callback will then eventually call\Symfony\Component\HttpFoundation\Session\Storage\SessionStorageFactoryInterface::createStorage whose implementations then setcookie_secure option totrue if the framework configuration value wasauto ortrue and the request is secure ->https://github.com/symfony/symfony/blob/v5.4.1/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorageFactory.php#L44

Since themergePhpSessionOptions method was already called in the constructor it did not take into consideration the PHP ini settings change that the session storage factory (potentially) made so by the time$sessionCookieSecure = $this->sessionOptions['cookie_secure'] ?? false; executes on thekernel.response event$this->sessionOptions['cookie_secure'] will befalse instead oftrue (in the case when the request was secure, when PHP has it set tofalse andauto was specified in the framework configuration).

The fix for this would probably be to not callmergePhpSessionOptions in the constructor and instead of accessing$this->sessionOptions directly in theonKernelResponse a getter can be called which would trigger themergePhpSessionOptions logic. That way we ensure that that logic is called at the last possible moment and with that we ensure thatsession_get_cookie_params() returns the correct/expected values (as those values can be changed between the constructor and thekernel.response event handling, as described above).

@X-Coder264 Thank you for the detailed response and testing this out. Was not aware of it that it was currently possible to change the Storage Options from outside. Then it definitly make sense that we move the call ofmergePhpSessionOptions from__construct where$this->sessionOptions is currently use by doing:

$sessionOptions =$this->getMergedPhpSessionOptions();$sessionCookiePath =$sessionOptions['cookie_path'] ??'/';// ...

Would be interesting how we could create a functional test case for this. To avoid this errors in the future.

@simonchrz sorry for the circumstances. And really thank you for working on this.

Changes looks good. Thank you 👍 . Still a test for symfony using cookie_secureauto with native secure disabled should be added, or did I miss that one?

nope, i'm actually struggling with adding this one... maybe you could jump in writing this test ? :-)

@simonchrz Thx, no problem. Will try to have a look at the evening at it.

I can confirm this fixes#44541

@simonchrz This two additonal test cases cover thecookie_secureauto behaviour:

'set_cookiesecure_auto_by_symfony_false_by_php' => ['phpSessionOptions' => ['secure' =>false],'sessionOptions' => ['cookie_path' =>'/test/','cookie_httponly' =>'auto','cookie_secure' =>'auto','cookie_samesite' => Cookie::SAMESITE_LAX],'expectedSessionOptions' => ['cookie_path' =>'/test/','cookie_domain' =>'','cookie_secure' =>false,'cookie_httponly' =>true,'cookie_samesite' => Cookie::SAMESITE_LAX],            ],'set_cookiesecure_auto_by_symfony_true_by_php' => ['phpSessionOptions' => ['secure' =>true],'sessionOptions' => ['cookie_path' =>'/test/','cookie_httponly' =>'auto','cookie_secure' =>'auto','cookie_samesite' => Cookie::SAMESITE_LAX],'expectedSessionOptions' => ['cookie_path' =>'/test/','cookie_domain' =>'','cookie_secure' =>true,'cookie_httponly' =>true,'cookie_samesite' => Cookie::SAMESITE_LAX],            ],

@alexander-schranz thanks for the hint == i've just added them to the provider

Why isn't 5.3 affected?

issue comes from refactoring on#41390 which isn't backmerged to 5.3 for some reason...

I am on version 5.4.1 and I guess. that I am affected by the Invalid CSRF problems, I have had to put in all my options csrf_ * => false to force the operation, any idea when I can update simfony with composer to return to normality?

i guess with 5.4.2 ? ;)

I think the real question is why wasn't this problem caught on such a mature project?

🤷

@nicolas-grekas any idea how to fix thos 8.0, macos-11 test of messenger component ? they seem to have some kind of disk I/O issues ?https://github.com/symfony/symfony/runs/4569511187?check_suite_focus=true#step:9:2464

any idea how to fix thos 8.0, macos-11 test of messenger component ?

That test is not related to these changes, is it? You don't need to bother then. The macOS test suite is relatively new and we still have some flaky tests there.

Thank you@simonchrz.

@simonchrz Thank you for fixing this, great work 👍

@simonchrz Thank you for fixing this, great work 👍

a great job let's wait for the 5.4.2 release to pull the XD, I think I'll have to revert some changes.