feature symfony#28244 [FrameworkBundle] Added new "auto" mode for framework.session.cookie_secure to turn it on when https is used (nicolas-grekas)

fabpot · fabpot · commit 74461cc2db84 · 2018-08-27T18:19:09.000+02:00
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
diff --git a/src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md b/src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md
@@ -9,6 +9,7 @@ CHANGELOG
  * 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
 
 4.1.0
 -----
diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
@@ -482,7 +482,7 @@ private function addSessionSection(ArrayNodeDefinition $rootNode)
                         ->scalarNode('cookie_lifetime')->end()
                         ->scalarNode('cookie_path')->end()
                         ->scalarNode('cookie_domain')->end()
-                        ->booleanNode('cookie_secure')->end()
+                        ->enumNode('cookie_secure')->values(array(true, false, 'auto'))->end()
                         ->booleanNode('cookie_httponly')->defaultTrue()->end()
                         ->booleanNode('use_cookies')->end()
                         ->scalarNode('gc_divisor')->end()
diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
@@ -765,6 +765,14 @@ private function registerSessionConfiguration(array $config, ContainerBuilder $c
             }
         }
 
+        if ('auto' === ($options['cookie_secure'] ?? null)) {
+            $locator = $container->getDefinition('session_listener')->getArgument(0);
+            $locator->setValues($locator->getValues() + array(
+                'session_storage' => new Reference('session.storage', ContainerInterface::IGNORE_ON_INVALID_REFERENCE),
+                'request_stack' => new Reference('request_stack'),
+            ));
+        }
+
         $container->setParameter('session.storage.options', $options);
 
         // session handler (the internal callback registered with PHP session management)
diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd b/src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd
@@ -112,7 +112,7 @@
         <xsd:attribute name="cookie-lifetime" type="xsd:string" />
         <xsd:attribute name="cookie-path" type="xsd:string" />
         <xsd:attribute name="cookie-domain" type="xsd:string" />
-        <xsd:attribute name="cookie-secure" type="xsd:boolean" />
+        <xsd:attribute name="cookie-secure" type="cookie_secure" />
         <xsd:attribute name="cookie-httponly" type="xsd:boolean" />
         <xsd:attribute name="use-cookies" type="xsd:boolean" />
         <xsd:attribute name="cache-limiter" type="xsd:string" />
@@ -329,6 +329,16 @@
         </xsd:sequence>
     </xsd:complexType>
 
+    <xsd:simpleType name="cookie_secure">
+        <xsd:restriction base="xsd:string">
+            <xsd:enumeration value="true" />
+            <xsd:enumeration value="false" />
+            <xsd:enumeration value="1" />
+            <xsd:enumeration value="0" />
+            <xsd:enumeration value="auto" />
+        </xsd:restriction>
+    </xsd:simpleType>
+
     <xsd:simpleType name="workflow_type">
         <xsd:restriction base="xsd:string">
             <xsd:enumeration value="state_machine" />
diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/session_cookie_secure_auto.php b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/session_cookie_secure_auto.php
@@ -0,0 +1,8 @@
+<?php
+
+$container->loadFromExtension('framework', array(
+    'session' => array(
+        'handler_id' => null,
+        'cookie_secure' => 'auto',
+    ),
+));
diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/session_cookie_secure_auto.xml b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/session_cookie_secure_auto.xml
@@ -0,0 +1,12 @@
+<?xml version="1.0" ?>
+
+<container xmlns="http://symfony.com/schema/dic/services"
+    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xmlns:framework="http://symfony.com/schema/dic/symfony"
+    xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd
+                        http://symfony.com/schema/dic/symfony http://symfony.com/schema/dic/symfony/symfony-1.0.xsd">
+
+    <framework:config>
+        <framework:session handler-id="null" cookie-secure="auto" />
+    </framework:config>
+</container>
diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/session_cookie_secure_auto.yml b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/session_cookie_secure_auto.yml
@@ -0,0 +1,4 @@
+framework:
+    session:
+        handler_id: ~
+        cookie_secure: auto
diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php
@@ -423,6 +423,9 @@ public function testNullSessionHandler()
         $this->assertTrue($container->hasDefinition('session'), '->registerSessionConfiguration() loads session.xml');
         $this->assertNull($container->getDefinition('session.storage.native')->getArgument(1));
         $this->assertNull($container->getDefinition('session.storage.php_bridge')->getArgument(0));
+
+        $expected = array('session', 'initialized_session');
+        $this->assertEquals($expected, array_keys($container->getDefinition('session_listener')->getArgument(0)->getValues()));
     }
 
     public function testRequest()
@@ -1243,6 +1246,14 @@ public function testLoggerAwareRegistration()
         $this->assertSame('logger', (string) $calls[0][1][0], 'Argument should be a reference to "logger"');
     }
 
+    public function testSessionCookieSecureAuto()
+    {
+        $container = $this->createContainerFromFile('session_cookie_secure_auto');
+
+        $expected = array('session', 'initialized_session', 'session_storage', 'request_stack');
+        $this->assertEquals($expected, array_keys($container->getDefinition('session_listener')->getArgument(0)->getValues()));
+    }
+
     protected function createContainer(array $data = array())
     {
         return new ContainerBuilder(new ParameterBag(array_merge(array(
diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddSessionDomainConstraintPass.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddSessionDomainConstraintPass.php
@@ -32,8 +32,17 @@ public function process(ContainerBuilder $container)
 
         $sessionOptions = $container->getParameter('session.storage.options');
         $domainRegexp = empty($sessionOptions['cookie_domain']) ? '%s' : sprintf('(?:%%s|(?:.+\.)?%s)', preg_quote(trim($sessionOptions['cookie_domain'], '.')));
-        $domainRegexp = (empty($sessionOptions['cookie_secure']) ? 'https?://' : 'https://').$domainRegexp;
 
-        $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;
+        } else {
+            $secureDomainRegexp = null;
+            $domainRegexp = (empty($sessionOptions['cookie_secure']) ? 'https?://' : 'https://').$domainRegexp;
+        }
+
+        $container->findDefinition('security.http_utils')
+            ->addArgument(sprintf('{^%s$}i', $domainRegexp))
+            ->addArgument($secureDomainRegexp);
     }
 }
diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Compiler/AddSessionDomainConstraintPassTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Compiler/AddSessionDomainConstraintPassTest.php
@@ -96,6 +96,26 @@ public function testNoSession()
         $this->assertTrue($utils->createRedirectResponse($request, 'http://pirate.com/foo')->isRedirect('http://pirate.com/foo'));
     }
 
+    public function testSessionAutoSecure()
+    {
+        $container = $this->createContainer(array('cookie_domain' => '.symfony.com.', 'cookie_secure' => 'auto'));
+
+        $utils = $container->get('security.http_utils');
+        $request = Request::create('/', 'get');
+
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://symfony.com/blog')->isRedirect('https://symfony.com/blog'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://www.symfony.com/blog')->isRedirect('https://www.symfony.com/blog'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'http://symfony.com/blog')->isRedirect('http://symfony.com/blog'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'http://pirate.com/foo')->isRedirect('http://localhost/'));
+
+        $container->get('router.request_context')->setScheme('https');
+
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://symfony.com/blog')->isRedirect('https://symfony.com/blog'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://www.symfony.com/blog')->isRedirect('https://www.symfony.com/blog'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'http://symfony.com/blog')->isRedirect('http://localhost/'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'http://pirate.com/foo')->isRedirect('http://localhost/'));
+    }
+
     private function createContainer($sessionStorageOptions)
     {
         $container = new ContainerBuilder();
@@ -121,7 +141,7 @@ private function createContainer($sessionStorageOptions)
         );
 
         $ext = new FrameworkExtension();
-        $ext->load(array('framework' => array('csrf_protection' => false)), $container);
+        $ext->load(array('framework' => array('csrf_protection' => false, 'router' => array('resource' => 'dummy'))), $container);
 
         $ext = new SecurityExtension();
         $ext->load($config, $container);
diff --git a/src/Symfony/Component/HttpKernel/EventListener/SessionListener.php b/src/Symfony/Component/HttpKernel/EventListener/SessionListener.php
@@ -12,10 +12,15 @@
 namespace Symfony\Component\HttpKernel\EventListener;
 
 use Psr\Container\ContainerInterface;
+use Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage;
 
 /**
  * Sets the session in the request.
  *
+ * When the passed container contains a "session_storage" entry which
+ * holds a NativeSessionStorage instance, the "cookie_secure" option
+ * will be set to true whenever the current master request is secure.
+ *
  * @author Fabien Potencier <fabien@symfony.com>
  *
  * @final
@@ -33,6 +38,13 @@ protected function getSession()
             return;
         }
 
+        if ($this->container->has('session_storage')
+            && ($storage = $this->container->get('session_storage')) instanceof NativeSessionStorage
+            && $this->container->get('request_stack')->getMasterRequest()->isSecure()
+        ) {
+            $storage->setOptions(array('cookie_secure' => true));
+        }
+
         return $this->container->get('session');
     }
 }
diff --git a/src/Symfony/Component/Security/Http/HttpUtils.php b/src/Symfony/Component/Security/Http/HttpUtils.php
@@ -30,22 +30,25 @@ class HttpUtils
     private $urlGenerator;
     private $urlMatcher;
     private $domainRegexp;
+    private $secureDomainRegexp;
 
     /**
-     * @param UrlGeneratorInterface                       $urlGenerator A UrlGeneratorInterface instance
-     * @param UrlMatcherInterface|RequestMatcherInterface $urlMatcher   The URL or Request matcher
-     * @param string|null                                 $domainRegexp A regexp that the target of HTTP redirections must match, scheme included
+     * @param UrlGeneratorInterface                       $urlGenerator       A UrlGeneratorInterface instance
+     * @param UrlMatcherInterface|RequestMatcherInterface $urlMatcher         The URL or Request matcher
+     * @param string|null                                 $domainRegexp       A regexp the target of HTTP redirections must match, scheme included
+     * @param string|null                                 $secureDomainRegexp A regexp the target of HTTP redirections must match when the scheme is "https"
      *
      * @throws \InvalidArgumentException
      */
-    public function __construct(UrlGeneratorInterface $urlGenerator = null, $urlMatcher = null, $domainRegexp = null)
+    public function __construct(UrlGeneratorInterface $urlGenerator = null, $urlMatcher = null, string $domainRegexp = null, string $secureDomainRegexp = null)
     {
         $this->urlGenerator = $urlGenerator;
         if (null !== $urlMatcher && !$urlMatcher instanceof UrlMatcherInterface && !$urlMatcher instanceof RequestMatcherInterface) {
             throw new \InvalidArgumentException('Matcher must either implement UrlMatcherInterface or RequestMatcherInterface.');
         }
         $this->urlMatcher = $urlMatcher;
         $this->domainRegexp = $domainRegexp;
+        $this->secureDomainRegexp = $secureDomainRegexp;
     }
 
     /**
@@ -59,6 +62,9 @@ public function __construct(UrlGeneratorInterface $urlGenerator = null, $urlMatc
      */
     public function createRedirectResponse(Request $request, $path, $status = 302)
     {
+        if (null !== $this->secureDomainRegexp && 'https' === $this->urlMatcher->getContext()->getScheme() && preg_match('#^https?://[^/]++#i', $path, $host) && !preg_match(sprintf($this->secureDomainRegexp, preg_quote($request->getHttpHost())), $host[0])) {
+            $path = '/';
+        }
         if (null !== $this->domainRegexp && preg_match('#^https?://[^/]++#i', $path, $host) && !preg_match(sprintf($this->domainRegexp, preg_quote($request->getHttpHost())), $host[0])) {
             $path = '/';
         }

Original file line number	Diff line number	Diff line change
`@@ -765,6 +765,14 @@ private function registerSessionConfiguration(array $config, ContainerBuilder $c`
`765`	`765`	`}`
`766`	`766`	`}`
`767`	`767`
	`768`	`+ if ('auto' === ($options['cookie_secure'] ?? null)) {`
	`769`	`+ $locator = $container->getDefinition('session_listener')->getArgument(0);`
	`770`	`+ $locator->setValues($locator->getValues() + array(`
	`771`	`+ 'session_storage' => new Reference('session.storage', ContainerInterface::IGNORE_ON_INVALID_REFERENCE),`
	`772`	`+ 'request_stack' => new Reference('request_stack'),`
	`773`	`+ ));`
	`774`	`+ }`
	`775`	`+`
`768`	`776`	`$container->setParameter('session.storage.options', $options);`
`769`	`777`
`770`	`778`	`// session handler (the internal callback registered with PHP session management)`