bug #27467 [HttpKernel] fix session tracking in surrogate master requests (nicolas-grekas)

This PR was merged into the 3.4 branch. Discussion ---------- [HttpKernel] fix session tracking in surrogate master requests | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Spotted while looking at ESI fragments resolved by`HttpCache`: right now when the master request starts the session, fragments are not cacheable anymore, even when they do not use the session. Commits ------- 146e01cb44 [HttpKernel] fix session tracking in surrogate master requests
2018-06-19 13:58:43 +02:00 · 2018-06-19 13:58:43 +02:00 · 18026dcc83
commit 18026dcc83
parent 1f439e5845 146e01cb44
5 changed files with 77 additions and 13 deletions
--- a/src/Symfony/Component/HttpFoundation/Session/Session.php
+++ b/src/Symfony/Component/HttpFoundation/Session/Session.php
@ -29,7 +29,7 @@ class Session implements SessionInterface, \IteratorAggregate, \Countable
    private $flashName;
    private $attributeName;
    private $data = array();
-    private $hasBeenStarted;
+    private $usageIndex = 0;

    /**
     * @param SessionStorageInterface $storage    A SessionStorageInterface instance
@ -54,6 +54,8 @@ class Session implements SessionInterface, \IteratorAggregate, \Countable
     */
    public function start()
    {
+        ++$this->usageIndex;
+
        return $this->storage->start();
    }

@ -142,13 +144,13 @@ class Session implements SessionInterface, \IteratorAggregate, \Countable
    }

    /**
-     * @return bool
+     * @return int
     *
     * @internal
     */
-    public function hasBeenStarted()
+    public function getUsageIndex()
    {
-        return $this->hasBeenStarted;
+        return $this->usageIndex;
    }

    /**
@ -158,6 +160,7 @@ class Session implements SessionInterface, \IteratorAggregate, \Countable
     */
    public function isEmpty()
    {
+        ++$this->usageIndex;
        foreach ($this->data as &$data) {
            if (!empty($data)) {
                return false;
@ -182,6 +185,8 @@ class Session implements SessionInterface, \IteratorAggregate, \Countable
     */
    public function migrate($destroy = false, $lifetime = null)
    {
+        ++$this->usageIndex;
+
        return $this->storage->regenerate($destroy, $lifetime);
    }

@ -190,6 +195,8 @@ class Session implements SessionInterface, \IteratorAggregate, \Countable
     */
    public function save()
    {
+        ++$this->usageIndex;
+
        $this->storage->save();
    }

@ -230,6 +237,8 @@ class Session implements SessionInterface, \IteratorAggregate, \Countable
     */
    public function getMetadataBag()
    {
+        ++$this->usageIndex;
+
        return $this->storage->getMetadataBag();
    }

@ -238,7 +247,7 @@ class Session implements SessionInterface, \IteratorAggregate, \Countable
     */
    public function registerBag(SessionBagInterface $bag)
    {
-        $this->storage->registerBag(new SessionBagProxy($bag, $this->data, $this->hasBeenStarted));
+        $this->storage->registerBag(new SessionBagProxy($bag, $this->data, $this->usageIndex));
    }

    /**
--- a/src/Symfony/Component/HttpFoundation/Session/SessionBagProxy.php
+++ b/src/Symfony/Component/HttpFoundation/Session/SessionBagProxy.php
@ -20,13 +20,13 @@ final class SessionBagProxy implements SessionBagInterface
 {
    private $bag;
    private $data;
-    private $hasBeenStarted;
+    private $usageIndex;

-    public function __construct(SessionBagInterface $bag, array &$data, &$hasBeenStarted)
+    public function __construct(SessionBagInterface $bag, array &$data, &$usageIndex)
    {
        $this->bag = $bag;
        $this->data = &$data;
-        $this->hasBeenStarted = &$hasBeenStarted;
+        $this->usageIndex = &$usageIndex;
    }

    /**
@ -34,6 +34,8 @@ final class SessionBagProxy implements SessionBagInterface
     */
    public function getBag()
    {
+        ++$this->usageIndex;
+
        return $this->bag;
    }

@ -42,6 +44,8 @@ final class SessionBagProxy implements SessionBagInterface
     */
    public function isEmpty()
    {
+        ++$this->usageIndex;
+
        return empty($this->data[$this->bag->getStorageKey()]);
    }

@ -58,7 +62,7 @@ final class SessionBagProxy implements SessionBagInterface
     */
    public function initialize(array &$array)
    {
-        $this->hasBeenStarted = true;
+        ++$this->usageIndex;
        $this->data[$this->bag->getStorageKey()] = &$array;

        $this->bag->initialize($array);
@ -77,6 +81,8 @@ final class SessionBagProxy implements SessionBagInterface
     */
    public function clear()
    {
+        ++$this->usageIndex;
+
        return $this->bag->clear();
    }
 }
--- a/src/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.php
+++ b/src/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.php
@ -14,6 +14,7 @@ namespace Symfony\Component\HttpKernel\EventListener;
 use Symfony\Component\HttpFoundation\Session\Session;
 use Symfony\Component\HttpFoundation\Session\SessionInterface;
 use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
+use Symfony\Component\HttpKernel\Event\FinishRequestEvent;
 use Symfony\Component\HttpKernel\Event\GetResponseEvent;
 use Symfony\Component\HttpKernel\KernelEvents;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
@ -25,6 +26,8 @@ use Symfony\Component\EventDispatcher\EventSubscriberInterface;
 */
 abstract class AbstractSessionListener implements EventSubscriberInterface
 {
+    private $sessionUsageStack = array();
+
    public function onKernelRequest(GetResponseEvent $event)
    {
        if (!$event->isMasterRequest()) {
@ -33,6 +36,7 @@ abstract class AbstractSessionListener implements EventSubscriberInterface

        $request = $event->getRequest();
        $session = $this->getSession();
+        $this->sessionUsageStack[] = $session instanceof Session ? $session->getUsageIndex() : null;
        if (null === $session || $request->hasSession()) {
            return;
        }
@ -50,7 +54,7 @@ abstract class AbstractSessionListener implements EventSubscriberInterface
            return;
        }

-        if ($session->isStarted() || ($session instanceof Session && $session->hasBeenStarted())) {
+        if ($session instanceof Session ? $session->getUsageIndex() !== end($this->sessionUsageStack) : $session->isStarted()) {
            $event->getResponse()
                ->setPrivate()
                ->setMaxAge(0)
@ -58,12 +62,23 @@ abstract class AbstractSessionListener implements EventSubscriberInterface
        }
    }

+    /**
+     * @internal
+     */
+    public function onFinishRequest(FinishRequestEvent $event)
+    {
+        if ($event->isMasterRequest()) {
+            array_pop($this->sessionUsageStack);
+        }
+    }
+
    public static function getSubscribedEvents()
    {
        return array(
            KernelEvents::REQUEST => array('onKernelRequest', 128),
            // low priority to come after regular response listeners, same as SaveSessionListener
            KernelEvents::RESPONSE => array('onKernelResponse', -1000),
+            KernelEvents::FINISH_REQUEST => array('onFinishRequest'),
        );
    }

--- a/src/Symfony/Component/HttpKernel/Tests/EventListener/SessionListenerTest.php
+++ b/src/Symfony/Component/HttpKernel/Tests/EventListener/SessionListenerTest.php
@ -18,6 +18,7 @@ use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\HttpFoundation\Session\Session;
 use Symfony\Component\HttpKernel\Event\GetResponseEvent;
 use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
+use Symfony\Component\HttpKernel\Event\FinishRequestEvent;
 use Symfony\Component\HttpKernel\EventListener\AbstractSessionListener;
 use Symfony\Component\HttpKernel\EventListener\SessionListener;
 use Symfony\Component\HttpKernel\HttpKernelInterface;
@ -58,8 +59,7 @@ class SessionListenerTest extends TestCase
    public function testResponseIsPrivate()
    {
        $session = $this->getMockBuilder(Session::class)->disableOriginalConstructor()->getMock();
-        $session->expects($this->once())->method('isStarted')->willReturn(false);
-        $session->expects($this->once())->method('hasBeenStarted')->willReturn(true);
+        $session->expects($this->exactly(2))->method('getUsageIndex')->will($this->onConsecutiveCalls(0, 1));

        $container = new Container();
        $container->set('session', $session);
@ -76,4 +76,38 @@ class SessionListenerTest extends TestCase
        $this->assertTrue($response->headers->hasCacheControlDirective('must-revalidate'));
        $this->assertSame('0', $response->headers->getCacheControlDirective('max-age'));
    }
+
+    public function testSurrogateMasterRequestIsPublic()
+    {
+        $session = $this->getMockBuilder(Session::class)->disableOriginalConstructor()->getMock();
+        $session->expects($this->exactly(4))->method('getUsageIndex')->will($this->onConsecutiveCalls(0, 1, 1, 1));
+
+        $container = new Container();
+        $container->set('session', $session);
+
+        $listener = new SessionListener($container);
+        $kernel = $this->getMockBuilder(HttpKernelInterface::class)->disableOriginalConstructor()->getMock();
+
+        $request = new Request();
+        $response = new Response();
+        $response->setCache(array('public' => true, 'max_age' => '30'));
+        $listener->onKernelRequest(new GetResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST));
+        $this->assertTrue($request->hasSession());
+
+        $subRequest = clone $request;
+        $this->assertSame($request->getSession(), $subRequest->getSession());
+        $listener->onKernelRequest(new GetResponseEvent($kernel, $subRequest, HttpKernelInterface::MASTER_REQUEST));
+        $listener->onKernelResponse(new FilterResponseEvent($kernel, $subRequest, HttpKernelInterface::MASTER_REQUEST, $response));
+        $listener->onFinishRequest(new FinishRequestEvent($kernel, $subRequest, HttpKernelInterface::MASTER_REQUEST));
+
+        $this->assertFalse($response->headers->hasCacheControlDirective('private'));
+        $this->assertFalse($response->headers->hasCacheControlDirective('must-revalidate'));
+        $this->assertSame('30', $response->headers->getCacheControlDirective('max-age'));
+
+        $listener->onKernelResponse(new FilterResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST, $response));
+
+        $this->assertTrue($response->headers->hasCacheControlDirective('private'));
+        $this->assertTrue($response->headers->hasCacheControlDirective('must-revalidate'));
+        $this->assertSame('0', $response->headers->getCacheControlDirective('max-age'));
+    }
 }
--- a/src/Symfony/Component/HttpKernel/composer.json
+++ b/src/Symfony/Component/HttpKernel/composer.json
@ -18,7 +18,7 @@
    "require": {
        "php": "^5.5.9|>=7.0.8",
        "symfony/event-dispatcher": "~2.8|~3.0|~4.0",
-        "symfony/http-foundation": "^3.4.4|^4.0.4",
+        "symfony/http-foundation": "~3.4.12|~4.0.12|^4.1.1",
        "symfony/debug": "~2.8|~3.0|~4.0",
        "symfony/polyfill-ctype": "~1.8",
        "psr/log": "~1.0"