[FrameworkBundle] Added new "auto" mode for `framework.session.cookie_secure` to turn it on when https is used by nicolas-grekas · Pull Request #28244 · symfony/symfony

nicolas-grekas · 2018-08-22T20:04:27Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

I'm pretty sure we're many forgetting to make session cookies "secure".
Here is an "auto" mode that makes them secure automatically when the session is started on requests with the "https" scheme.

Pierstoval

I'm also using it by default on my projects because it's important, making it default seems to be a good option to me 👍

sroze · 2018-08-24T10:24:17Z

src/Symfony/Component/HttpKernel/EventListener/SessionListener.php

+            && ($storage = $this->container->get('session_storage')) instanceof NativeSessionStorage
+            && $this->container->get('request_stack')->getMasterRequest()->isSecure()
+        ) {
+            $storage->setOptions(array('cookie_secure' => true));


Might be a silly question but are we sure that this is called before the storage start?

We can throw a deprecation if that's not the case so that we ensure we can provide good security in 5.0. WDYT?

Yep, sounds good to me 👍

Also, this option should only be set when framework.session.cookie_secure === auto, not all the time. Looks like it is calling this option even if the option is set as false here.

Actually, we don't care: NativeSessionStorage::setOptions starts with:

if (headers_sent() || \PHP_SESSION_ACTIVE === session_status()) { return; }

But... we do care because if it is called after the start then the cookie_secure will not be configured which will end up having the secure cookie not configured 🤔

"auto" must mean "best effort".
If you want to be sure to have secure, just make your config explicit.

sroze · 2018-08-24T10:41:03Z

src/Symfony/Component/HttpKernel/EventListener/SessionListener.php

+            && ($storage = $this->container->get('session_storage')) instanceof NativeSessionStorage
+            && $this->container->get('request_stack')->getMasterRequest()->isSecure()
+        ) {
+            $storage->setOptions(array('cookie_secure' => true));


Yep, sounds good to me 👍

Also, this option should only be set when framework.session.cookie_secure === auto, not all the time. Looks like it is calling this option even if the option is set as false here.

nicolas-grekas · 2018-08-24T11:45:34Z

this option should only be set when framework.session.cookie_secure === auto

Which is already the case because "session_storage" is passed to "session.listener" only when framework.session.cookie_secure === auto. I added a note in the docblock of SessionListener about this.

ro0NL · 2018-08-24T12:06:31Z

...ymfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddSessionDomainConstraintPass.php

-        $container->findDefinition('security.http_utils')->addArgument(sprintf('{^%s$}i', $domainRegexp));
+        if ('auto' === ($sessionOptions['cookie_secure'] ?? null)) {
+            $secureDomainRegexp = sprintf('{^https://%s$}i', $domainRegexp);
+            $domainRegexp = 'https?://'.$domainRegexp;


moving the regex composition to runtime would solve #28051

worth to have look in this PR?

that's independent, it would deserve its own line in the changelog

fabpot

with minor typo to fix

fabpot · 2018-08-27T16:02:48Z

src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md

 * Deprecated auto-injection of the container in AbstractController instances, register them as service subscribers instead
 * Deprecated processing of services tagged `security.expression_language_provider` in favor of a new `AddExpressionLanguageProvidersPass` in SecurityBundle.
 * Enabled autoconfiguration for `Psr\Log\LoggerAwareInterface`
+ * Added new "auto" mode for `framework.session.cookie_secure` to turn it on when https is used


…_secure` to turn it on when https is used

fabpot · 2018-08-27T16:19:16Z

Thank you @nicolas-grekas.

….session.cookie_secure` to turn it on when https is used (nicolas-grekas) This PR was merged into the 4.2-dev branch. Discussion ---------- [FrameworkBundle] Added new "auto" mode for `framework.session.cookie_secure` to turn it on when https is used | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - I'm pretty sure we're many forgetting to make session cookies "secure". Here is an "auto" mode that makes them secure automatically when the session is started on requests with the "https" scheme. Commits ------- 4f7b41a [FrameworkBundle] Added new "auto" mode for `framework.session.cookie_secure` to turn it on when https is used

javiereguiluz · 2018-09-07T14:41:22Z

I've created symfony/symfony-docs#10284 to document this new feature. Please, don't forget to create a doc issue for every new feature, specially for important and security-related features like this one. Thanks!

@nicolas-grekas

…ty (yceruto) This PR was merged into the 4.2 branch. Discussion ---------- [HttpKernel] Fix get session when the request stack is empty | Q | A | ------------- | --- | Branch? | 4.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT This bug happen behind an exception on a kernel response event, when one collector (e.g. `RequestDataCollector`) is trying to get the request session and the request stack is currently empty. **Reproducer** https://github.com/yceruto/get-session-bug (`GET /`) See logs on terminal: ```bash Apr 15 20:29:03 |ERROR| PHP 2019-04-15T20:29:03-04:00 Call to a member function isSecure() on null Apr 15 20:29:03 |ERROR| PHP PHP Fatal error: Uncaught Symfony\Component\Debug\Exception\FatalThrowableError: Call to a member function isSecure() on null in /home/yceruto/demos/getsession/vendor/symfony/http-kernel/EventListener/SessionListener.php:43 Apr 15 20:29:03 |DEBUG| PHP Stack trace: Apr 15 20:29:03 |DEBUG| PHP #0 /home/yceruto/demos/getsession/vendor/symfony/http-kernel/EventListener/AbstractSessionListener.php(59): Symfony\Component\HttpKernel\EventListener\SessionListener->getSession() Apr 15 20:29:03 |DEBUG| PHP #1 /home/yceruto/demos/getsession/vendor/symfony/http-foundation/Request.php(707): Symfony\Component\HttpKernel\EventListener\AbstractSessionListener->Symfony\Component\HttpKernel\EventListener\{closure}() Apr 15 20:29:03 |DEBUG| PHP #2 /home/yceruto/demos/getsession/vendor/symfony/http-kernel/DataCollector/RequestDataCollector.php(65): Symfony\Component\HttpFoundation\Request->getSession() Apr 15 20:29:03 |DEBUG| PHP #3 /home/yceruto/demos/getsession/vendor/symfony/http-kernel/Profiler/Profiler.php(167): Symfony\Component\HttpKernel\DataCollector\RequestDataCollector->collect(Object(Symfony\Component\HttpFoundation\Request), Object(Symfony\Component\HttpFoundation\Respo in /home/yceruto/demos/getsession/vendor/symfony/http-kernel/EventListener/SessionListener.php on line 43 ``` Friendly ping @nicolas-grekas as author of the previous PR symfony/symfony#28244 Commits ------- d62ca37 Fix get session when the request stack is empty

nicolas-grekas added this to the next milestone Aug 22, 2018

carsonbot added Status: Needs Review FrameworkBundle Feature labels Aug 22, 2018

nicolas-grekas mentioned this pull request Aug 22, 2018

[symfony/framework-bundle] Secure session defaults with "cookie_secure: auto" and "cookie_samesite: lax" symfony/recipes#452

Merged

carsonbot removed the Status: Needs Review label Aug 22, 2018

maxhelias approved these changes Aug 22, 2018

View reviewed changes

carsonbot added the Status: Reviewed label Aug 22, 2018

nicolas-grekas force-pushed the session-secure branch from 65a6c9d to b695d91 Compare August 22, 2018 20:50

dunglas approved these changes Aug 22, 2018

View reviewed changes

nicolas-grekas force-pushed the session-secure branch from b695d91 to 99580ee Compare August 22, 2018 21:14

Pierstoval approved these changes Aug 23, 2018

View reviewed changes

sroze reviewed Aug 24, 2018

View reviewed changes

carsonbot removed the Status: Reviewed label Aug 24, 2018

sroze suggested changes Aug 24, 2018

View reviewed changes

carsonbot added the Status: Needs Work label Aug 24, 2018

nicolas-grekas force-pushed the session-secure branch from 99580ee to 3b161ff Compare August 24, 2018 11:44

nicolas-grekas force-pushed the session-secure branch from 3b161ff to 39d069a Compare August 24, 2018 11:46

ro0NL reviewed Aug 24, 2018

View reviewed changes

nicolas-grekas force-pushed the session-secure branch from 39d069a to 4a59670 Compare August 24, 2018 13:43

carsonbot removed the Status: Needs Work label Aug 24, 2018

sroze approved these changes Aug 24, 2018

View reviewed changes

carsonbot added the Status: Reviewed label Aug 24, 2018

fabpot approved these changes Aug 27, 2018

View reviewed changes

[FrameworkBundle] Added new "auto" mode for `framework.session.cookie…

4f7b41a

…_secure` to turn it on when https is used

nicolas-grekas force-pushed the session-secure branch from 4a59670 to 4f7b41a Compare August 27, 2018 16:14

fabpot merged commit 4f7b41a into symfony:master Aug 27, 2018

nicolas-grekas deleted the session-secure branch August 30, 2018 15:24

leofeyer mentioned this pull request Sep 2, 2018

"auto" mode for "remember_me.secure" #28338

Closed

javiereguiluz mentioned this pull request Sep 7, 2018

[FrameworkBundle] Added new "auto" mode for framework.session.cookie_secure symfony/symfony-docs#10283

Closed

nicolas-grekas mentioned this pull request Sep 7, 2018

Flag all cookies as Secure #26731

Closed

nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

This was referenced Nov 3, 2018

Release v4.2.0-BETA1 #29072

Closed

Release v4.2.0-BETA1 #29073

Merged

yceruto mentioned this pull request Apr 16, 2019

[HttpKernel] Fix get session when the request stack is empty #31121

Merged

Uh oh!

Conversation

nicolas-grekas commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pierstoval left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Aug 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Aug 27, 2018

Uh oh!

javiereguiluz commented Sep 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

nicolas-grekas commented Aug 22, 2018 •

edited

Loading

nicolas-grekas Aug 24, 2018 •

edited

Loading

nicolas-grekas Aug 24, 2018 •

edited

Loading