Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (seehttps://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
• Features and deprecations must be submitted against the 5.x branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

I think this PR works in your case because your php.ini file hascookie_secure=on and this PR prevent overriding the .ini value with"auto".

But 'auto' is not a valid value for this ini setting. Yes, settingcookie_secure=auto via ini_set does have the same effect as setting it to false/off/0, but the question is whether this is an intended behavior. For this I agree with@stof that the value 'auto' shouldn't be passed as-is to the session storage.

But what if your .ini is set toon and you call the page withhttp does it removes thecookie_secure flag? Or what if the .ini is set tooff and the page is called withhttps is it re-added?

You are correct that this PR missed the scenario when the page is accessed through http, not https. The code only sets cookie_secure totrue for https, but didn't set it tofalse for http traffic. In this case theonKernelRequest callback could be changed to something like this:

if ($this->container->has('session_storage')            && ($storage =$this->container->get('session_storage'))instanceof NativeSessionStorage            && ($masterRequest =$this->container->get('request_stack')->getMasterRequest())        ) {$storage->setOptions(['cookie_secure' =>$masterRequest->isSecure()]);        }

I believe we should disable the .ini setting when value is auto then the SessionListener will re-enabled it if the request is secured.

Personally I don't have preference on whether the setting is set in one place, or is first disabled then enabled if the traffic in https. But again, I think the value passed to ini_set shouldn't be 'auto'.

Are you sure that your Code/EventListener has a lower priority than the SessionListener and you are not reading the session before the flag is correctly sets?
Note: In Symfony 5.3, injecting the session in your service will be deprecated and you'll have to use the RequestStack to get a session. So this will not be an issue anymore, as you won't be able to manipulate the session before the SessionListener initialize the storage and inject the session in the request.

I need to confess that my experience with Symfony internals isn't deep enough for me to guarantee that people will be immune from similar situations. They may still able to register an event listener which accesses the session before SessionListener is called, causing it to fail to properly set thecookie_secure flag (actually there might even be a need to tell the developer that the service cannot set the flag properly, given this is a security related thing). But, for my case, what I did was just registering a guard authenticator which accesses session data in thesupports() method. I didn't change the priority of any of the involved services. And by default the authenticator'ssupports() is called beforeSessionListener.

And by default the authenticator'ssupports() is called beforeSessionListener.

no it is not. And that's precisely why this PR will fix the issue by making sure that theauto value is resolved by the timeSessionListener is called (rather than by the time the session is retrieved from the Request object for the first time)

@stof is right. I forgot to update my mind when writing the reply.

The PR has been updated. In this PR,

1. Instead of lettingNativeSessionStorage to check for thisauto value in thecookie_secure key and act differently, the framework will create another set of options that does not contain thesecure_cookie option if it is set toauto, and theNativeSessionStorage will receive this special set of options in the constructor.

2. TheSessionListener class is updated to set thecookie_secure value properly inonKernelRequest, no matter the request is secure or not. This fixes the issue in the previous PR thatcookie_secure won't be set if the request is not secure.

Two related notes:

1. The scope of the issue is actually larger then I anticipated. Just create a new application,add a simple controller action that accesses the session. The auto secure cookie won't work because of the reason explained by@stof.

2. I have also verifitied that after the PR, user can still "invalidate" theSessionListener by creating an event listener that accesses the session with priority equal to or higher than that ofSessionListenerlike this.

Thank you@tamcy.

2\. I have also verifitied that after the PR, user can still "invalidate" the `SessionListener` by creating an event listener that accesses the session with priority equal to or higher than that of `SessionListener` [like this](https://github.com/tamcy/symfony-secure-cookie-test-4.4/blob/master/src/EventListener/NullifyAutoCookieSecureEventListener.php).

this is fine to me. On future versions where accessing the session as a service is not possible anymore, you won't be able to access the session before it is initialized in the Request by SessionListener (which is also why it runs at priority 100)

@nicolas-grekas Thank you. May I know if it is intentional to use my first pull request, rather than the second one? I ask this because besides the difference in approach, the second PR includes a fix in theSessionListener which is missing in the first PR. In the first (merged) PR, the code reads:

if ($this->container->has('session_storage')            && ($storage =$this->container->get('session_storage'))instanceof NativeSessionStorage            && ($masterRequest =$this->container->get('request_stack')->getMasterRequest())            &&$masterRequest->isSecure()        ) {$storage->setOptions(['cookie_secure' =>true]);        }

In this code, if the request is not https, the storage option will not be set. Consequently, the default PHP value will be used. If the default ini value is set to On (forcing secure cookie), the cookie will not work in an auto environment. Which is why the the code in the second PR, the code is changed to:

if ($this->container->has('session_storage')            && ($storage =$this->container->get('session_storage'))instanceof NativeSessionStorage            && ($masterRequest =$this->container->get('request_stack')->getMasterRequest())        ) {$storage->setOptions(['cookie_secure' =>$masterRequest->isSecure()]);        }

Please advise what could be done. Shall I create another PR for this? Thank you!

Yes, I reverted that part because I thought it was not desired: the goal here is to upgrade to ssl when possible. Never to downgrade to no-ssl, which is what the change you're mentioning would do to me.

I see, thank you, this makes sense. Do we need a warning to users though? As this kind of downgrade does happen before the patch (the value is always set to "auto" at first, which effectively turns off the secure cookie flag).