B/C break in http-kernel / EventListener / AbstractSessionListener in Symfony 3.4 TLS #29151
Comments
|
Hi @nicolas-grekas, Thank you for linking the issues, my google-fu apparently failed me while researching this initially. I agree that this is a good default, however let me explain why I do consider this a B/C break in 3.4/LTS. As per comments in other tickets, HttpKernel is not aware of application-specific logic (hence the reasonable default), but consider a controller method as follows: <?php
namespace AppBundle\Controller;
class HomepageController extends \Symfony\Bundle\FrameworkBundle\Controller\Controller
{
public function indexAction()
{
$viewData = [];
// some business logic
$response = $this->render('index.html.twig', $viewData);
// user-agnostic page (even for authenticated users) - we can set long-term cache headers here
$response->setPublic();
$response->setMaxAge(86400);
$response->setSharedMaxAge(86400);
return $response;
}
}The above scenario is perfectly valid, but is of course specific per application. The change introduced to $event->getResponse()
->setPrivate()
->setMaxAge(0)
->headers->addCacheControlDirective('must-revalidate');... thus breaking what was working fine prior to 3.4. Note that I'm not arguing that the new default is incorrect (on the contrary). I'm arguing that we should not override headers set by the application - we should do so only when no values were set, i.e. modify $response = $event->getResponse();
if(!$reponse->headers->hasCacheControlDirective())
{
$response
->setPrivate()
->setMaxAge(0)
->headers->addCacheControlDirective('must-revalidate');
}Thank you |
|
Doing so can lead to severe security issues: a user-specific value can be put in a shared HTTP cache. That's why it shouldn't be that easy to override the behavior. |
|
I don't disagree. Severity will vary depending on the application, i.e. in our case a loose flash message (with no PII) is the only thing I can image that could ever end there (and even that would be extremely rare). So if we don't want to backport |
|
Using a decorating listener yes, see FriendsOfSymfony/FOSHttpCacheBundle#435 |
|
That's awesome! Thank you Nicolas. |
|
Just a note for others: I have an action similar to the one presented in: #29151 (comment) . The action itself does not use session but because of a firewall configuration in my I made a workaround by disabling the security for the action: |
Uh oh!
There was an error while loading. Please reload this page.
Symfony version(s) affected: 3.4.x
Description
Symfony 3.4 changed behaviour of
AbstractSessionListenerwhen a session cookie is present (regardless of authentication state).Old behaviour was to respect
Cache-Controlheaders set on response. New behaviour introduces a B/C break that overridesCache-Controlheaders in a way that cannot be easily changed. This can have a non-trivial performance impact as it prevents websites to set proper cache headers even on user-agnostic content.A fix for this was introduced in Symfony 4.1, but no solution is easily available for Symfony 3.4.
How to reproduce
Cache-Control: public, s-maxage=86400on responseCache-Control: max-age=0, must-revalidate, private, s-maxage=1234Possible Solution
One of:
Additional context
Respective code:
The text was updated successfully, but these errors were encountered: