Skip to content

[HttpKernel] Fix session handling: decouple "save" from setting response "private" #25699

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 10, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions src/Symfony/Component/HttpFoundation/Session/Session.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class Session implements SessionInterface, \IteratorAggregate, \Countable
private $flashName;
private $attributeName;
private $data = array();
private $hasBeenStarted;

/**
* @param SessionStorageInterface $storage A SessionStorageInterface instance
Expand Down Expand Up @@ -140,6 +141,16 @@ public function count()
return count($this->getAttributeBag()->all());
}

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

/**
* @return bool
*
Expand Down Expand Up @@ -227,7 +238,7 @@ public function getMetadataBag()
*/
public function registerBag(SessionBagInterface $bag)
{
$this->storage->registerBag(new SessionBagProxy($bag, $this->data));
$this->storage->registerBag(new SessionBagProxy($bag, $this->data, $this->hasBeenStarted));
}

/**
Expand Down Expand Up @@ -257,6 +268,6 @@ public function getFlashBag()
*/
private function getAttributeBag()
{
return $this->storage->getBag($this->attributeName)->getBag();
return $this->getBag($this->attributeName);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,13 @@ final class SessionBagProxy implements SessionBagInterface
{
private $bag;
private $data;
private $hasBeenStarted;

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

/**
Expand Down Expand Up @@ -56,6 +58,7 @@ public function getName()
*/
public function initialize(array &$array)
{
$this->hasBeenStarted = true;
$this->data[$this->bag->getStorageKey()] = &$array;

$this->bag->initialize($array);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@

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\GetResponseEvent;
use Symfony\Component\HttpKernel\KernelEvents;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
Expand All @@ -38,10 +40,30 @@ public function onKernelRequest(GetResponseEvent $event)
$request->setSession($session);
}

public function onKernelResponse(FilterResponseEvent $event)
{
if (!$event->isMasterRequest()) {
return;
}

if (!$session = $event->getRequest()->getSession()) {
return;
}

if ($session->isStarted() || ($session instanceof Session && $session->hasBeenStarted())) {
$event->getResponse()
->setPrivate()
->setMaxAge(0)
->headers->addCacheControlDirective('must-revalidate');
}
}

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),
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,17 @@ public function onKernelResponse(FilterResponseEvent $event)
return;
}

$session = $event->getRequest()->getSession();
if ($session && $session->isStarted()) {
if (!$session = $event->getRequest()->getSession()) {
return;
}

if ($wasStarted = $session->isStarted()) {
$session->save();
if (!$session instanceof Session || !\method_exists($session, 'isEmpty') || !$session->isEmpty()) {
$params = session_get_cookie_params();
$event->getResponse()->headers->setCookie(new Cookie($session->getName(), $session->getId(), 0 === $params['lifetime'] ? 0 : time() + $params['lifetime'], $params['path'], $params['domain'], $params['secure'], $params['httponly']));
}
}

if ($session instanceof Session ? !$session->isEmpty() : $wasStarted) {
$params = session_get_cookie_params();
$event->getResponse()->headers->setCookie(new Cookie($session->getName(), $session->getId(), 0 === $params['lifetime'] ? 0 : time() + $params['lifetime'], $params['path'], $params['domain'], $params['secure'], $params['httponly']));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,6 @@ public function onKernelResponse(FilterResponseEvent $event)
$session = $event->getRequest()->getSession();
if ($session && $session->isStarted()) {
$session->save();
$event->getResponse()
->setPrivate()
->setMaxAge(0)
->headers->addCacheControlDirective('must-revalidate');
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public function testOnlyTriggeredOnMasterRequest()
$listener->onKernelResponse($event);
}

public function testSessionSavedAndResponsePrivate()
public function testSessionSaved()
{
$listener = new SaveSessionListener();
$kernel = $this->getMockBuilder(HttpKernelInterface::class)->disableOriginalConstructor()->getMock();
Expand All @@ -45,9 +45,5 @@ public function testSessionSavedAndResponsePrivate()
$request->setSession($session);
$response = new Response();
$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'));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\HttpKernel\Tests\EventListener;

use PHPUnit\Framework\TestCase;
use Symfony\Component\DependencyInjection\Container;
use Symfony\Component\HttpFoundation\Request;
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\EventListener\AbstractSessionListener;
use Symfony\Component\HttpKernel\EventListener\SessionListener;
use Symfony\Component\HttpKernel\HttpKernelInterface;

class SessionListenerTest extends TestCase
{
public function testOnlyTriggeredOnMasterRequest()
{
$listener = $this->getMockForAbstractClass(AbstractSessionListener::class);
$event = $this->getMockBuilder(GetResponseEvent::class)->disableOriginalConstructor()->getMock();
$event->expects($this->once())->method('isMasterRequest')->willReturn(false);
$event->expects($this->never())->method('getRequest');

// sub request
$listener->onKernelRequest($event);
}

public function testSessionIsSet()
{
$session = $this->getMockBuilder(Session::class)->disableOriginalConstructor()->getMock();

$container = new Container();
$container->set('session', $session);

$request = new Request();
$listener = new SessionListener($container);

$event = $this->getMockBuilder(GetResponseEvent::class)->disableOriginalConstructor()->getMock();
$event->expects($this->once())->method('isMasterRequest')->willReturn(true);
$event->expects($this->once())->method('getRequest')->willReturn($request);

$listener->onKernelRequest($event);

$this->assertTrue($request->hasSession());
$this->assertSame($session, $request->getSession());
}

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);

$container = new Container();
$container->set('session', $session);

$listener = new SessionListener($container);
$kernel = $this->getMockBuilder(HttpKernelInterface::class)->disableOriginalConstructor()->getMock();

$request = new Request();
$response = new Response();
$listener->onKernelRequest(new GetResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST));
$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'));
}
}
2 changes: 1 addition & 1 deletion src/Symfony/Component/HttpKernel/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -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.3.11|~4.0",
"symfony/http-foundation": "^3.4.4|^4.0.4",
"symfony/debug": "~2.8|~3.0|~4.0",
"psr/log": "~1.0"
},
Expand Down