8000 Something Wrong with AbstractSessionListener · Issue #37113 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

Something Wrong with AbstractSessionListener #37113

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

Closed
tugrul opened this issue Jun 5, 2020 · 19 comments
Closed

Something Wrong with AbstractSessionListener #37113

tugrul opened this issue Jun 5, 2020 · 19 comments

Comments

@tugrul
Copy link
tugrul commented Jun 5, 2020

Symfony version(s) affected: 5.0.9

Description
AbstractSessionListener always modify Cache-Control header even there is no data inside of session storage.

How to reproduce
Simply create a controller with a hello world response action.

Possible Solution
We can check is there some data in session store instead of checking session mutation.

Additional context
I inspect the issue with debugging and you can see below.

session_store_no_cache

@tugrul
Copy link
Author
tugrul commented Jun 5, 2020

Is there a way to disable AbstractSessionListener class from autoconfiguration?

I tried to disable on services.yaml but not works.

services:
    Symfony\Component\HttpKernel\EventListener\AbstractSessionListener:
        autoconfigure: false

edit: I also tried to disable SessionListener but doesn't work.

services:
    Symfony\Component\HttpKernel\EventListener\SessionListener:
        autoconfigure: false

@antalaron
Copy link
Contributor

Maybe I misunderstand something, but the PHP Doc of the AbstractSessionListener doesn't help you?

Sets the session onto the request on the "kernel.request" event and saves
it on the "kernel.response" event.

In addition, if the session has been started it overrides the Cache-Control
header in such a way that all caching is disabled in that case.
If you have a scenario where caching responses with session information in
them makes sense, you can disable this behaviour by setting the header
AbstractSessionListener::NO_AUTO_CACHE_CONTROL_HEADER on the response.

@tugrul
Copy link
Author
tugrul commented Jun 7, 2020

@antalaron yes I missed following lines. Something can break if I disable SessionListener. I just wanted to disable SessionListener to workaround Cache-Control problem.

AbstractSessionListener::NO_AUTO_CACHE_CONTROL_HEADER is not an elegant solution and I have to wait for a new release that fixed the problem.

@nicolas-grekas
Copy link
Member

Thanks for the report.
Even if the session is empty, the fact that it accessed is enough to make the page not-cacheable.
This is what this listener guards for.
NO_AUTO_CACHE_CONTROL_HEADER is the way to go, but only in use cases where the page is really cacheable, despite the session being used. There is only one case I know about, which is when values shared by groups of users are accessed.
But reading the empty session is not such a case.

I suspect you have a template that tries to display session flash bags. That's enough to make a page uncacheable.

@tugrul
Copy link
Author
tugrul commented Jun 8, 2020

@nicolas-grekas thank you for your response. Session wont empty in any regular Symfony application and it is always going to break Cache-Control header. Nowadays there is no sharp borderline of web applications, they can serve static and dynamic content in same time.

Session object's existence is insignificant if it not send session cookie to client and we can consider the content as static in this scenario. Also we can manage this cases in Varnish or NGINX. I can give directive NGINX to bypass cache if there is a cookie but I can't create caching strategy URL by URL in NGINX configuration. Only I can create caching strategy URL by URL in Symfony's side using Cache-Control header.

NO_AUTO_CACHE_CONTROL_HEADER is not a solution because I have to add it to every controllers' actions this or I should create kernel onresponse listener to add it to every response object.

You can observe same problem for following sample it not use twig, session or flashbag.

namespace App\Controller;

use Sensio\Bundle\FrameworkExtraBundle\Configuration\Cache;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\Routing\Annotation\Route;

class IndexController extends AbstractController
{
    /**
     * @Route("/", name="index")
     * @Cache(smaxage=3600)
     */
    public function indexAction()
    {
        return new Response('test');
    }
}

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Jun 8, 2020

Session wont empty in any regular Symfony application and it is always going to break Cache-Control header

Why so? With your example, the session is not used at all.
Or maybe you have a firewall enabled, in which case this is the reason why.

In Symfony 5.1, we added a way to more easily know where the session is started/used. You might want to check https://symfony.com/blog/new-in-symfony-5-1-routing-improvements, that might help understand what's going on.

@tugrul
Copy link
Author
tugrul commented Jun 8, 2020

@nicolas-grekas yes there are guard authenticators in firewall rules but there is no access control rules for the action. Also there is no user authenticated and there is no session cookie.

And I found where is the session is initialized.

Session requested by RequestDataCollector

debug_1

Then initalized by session factory callback.

debug_2

@tugrul
Copy link
Author
tugrul commented Jun 8, 2020

$session->isEmpty() can solve the issue by my tests.

It returns true on first request to following action. Because there is no data and no session cookie even session object is initialized somewhere. This is cachable response.

public function indexAction()
{
    return new Response('test');
}

Then I add reference to session and set some value to session. It returns false on second request to following action. This is uncacheable response.

public function indexAction(SessionInterface $session)
{
    $session->set('some', 'value');
    return new Response('test');
}

Then I convert back to previous action for third request and it returns false again because there is a session cookie and session values by previous request. This is uncachable response.

@mpdude
Copy link
Contributor
mpdude commented Jun 11, 2020

@tugrul I don't know if you can go back that far, but could you try to check if everything works as you'd expect with Symfony 4.3.11 and then "something" happens as of Symfony 4.4.0?

@mpdude
Copy link
Contributor
mpdude commented Jun 11, 2020

Seems to be related to #33663. I have an app where a controller like the one shown above...

namespace App\Controller;

use Sensio\Bundle\FrameworkExtraBundle\Configuration\Cache;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\Routing\Annotation\Route;

class IndexController extends AbstractController
{
    /**
     * @Route("/", name="index")
     * @Cache(smaxage=3600)
     */
    public function indexAction()
    {
        return new Response('test');
    }
}

will be public with Symfony 4.3.11, but become private in Symfony 4.4. Upon inspection I see that AbstractSessionListener::onKernelResponse() activates the private "protection" mechanism because of something with the UsageTrackingTokenStorage.

I guess it's because the application has the following (very defaults!) in security.yml:

security:
    # https://symfony.com/doc/current/security.html#where-do-users-come-from-user-providers
    providers:
        in_memory: { memory: null }
    firewalls:
        dev:
            pattern: ^/(_(profiler|wdt)|css|images|js)/
            security: false
        main:
            anonymous: true

            # activate different ways to authenticate
            # https://symfony.com/doc/current/security.html#firewalls-authentication

            # https://symfony.com/doc/current/security/impersonating_user.html
            # switch_user: true

    # Easy way to control access for large sections of your site
    # Note: Only the *first* access control that matches will be used
    access_control:
        # - { path: ^/admin, roles: ROLE_ADMIN }
        # - { path: ^/profile, roles: ROLE_USER }

Basically, that should just be a flex-provided, almost clean setup.

Maybe @nicolas-grekas could advise if that is in fact intended and necessary behavior or worth further investigation? Also CC @Toflar because you've struggled with this in #33663 as well.

Update: #34220 seems to be the original report.

@nicolas-grekas
Copy link
Member

See my comment in #34220 (comment)

@tugrul
Copy link
Author
tugrul commented Jun 11, 2020

@mpdude I don't have an idea for previous versions but this broke caching strategy of my proxy cache.

@nicolas-grekas stateless it not a solution because actions sometimes have states and sometimes have not states. This feature is good by default but it not gives any other option also it works wrong by my idea. I wish opt-out easily then I'm able to make safe using NGINX cache directives. I wrote an ass-backwards event handler to change behavior of this feature.

namespace App\EventSubscriber;

use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpKernel\Event\ResponseEvent;
use Symfony\Component\HttpKernel\EventListener\AbstractSessionListener;

use Symfony\Component\HttpFoundation\Session\Session;
use Symfony\Component\HttpKernel\KernelEvents;

class SessionCacheControlSubscriber implements EventSubscriberInterface
{
    public function onKernelResponse(ResponseEvent $event)
    {

        if (!defined(AbstractSessionListener::class . '::NO_AUTO_CACHE_CONTROL_HEADER')) {
            return;
        }

        $request = $event->getRequest();

        // https://github.com/symfony/symfony/issues/37113#issuecomment-643176155
        if (!$request->hasSession()) {
            return;
        }

        $session = $request->getSession();

        // existence of isEmpty function is not guarantee because it isn't in SessionInterface contract
        if (!($session instanceof Session) || !method_exists($session, 'isEmpty')) {

            $fields = $session->all();

            foreach ($fields as &$field) {
                if (!empty($field)) {
                    return;
                }
            }

        } elseif (!$session->isEmpty()) {
            return;
        }

        $response = $event->getResponse();
        $response->headers->set(AbstractSessionListener::NO_AUTO_CACHE_CONTROL_HEADER, true);
    }

    public static function getSubscribedEvents()
    {
        return [
            KernelEvents::RESPONSE => ['onKernelResponse', -999],
        ];
    }
}

@nicolas-grekas
Copy link
Member

actions sometimes have states and sometimes have not states.

Can you explain what you mean? Do you have an example?

@tugrul
Copy link
Author
tugrul commented Jun 11, 2020

actions sometimes have states and sometimes have not states.

Can you explain what you mean? Do you have an example?

For example there is a BlogPostController::showAction and it renders blog post content and comments of this content. It is stateless when you are using as anonymous. Then you wanted to write a comment this blog post and scroll down. Consider to see a message "You must sign in to write a comment" with Login and Register buttons. Then you click to login button it redirected to login form. Then you fill your credentials and logged in and redirected back to BlogPostController::showAction. You are not anonymous and comment form appears below. In this time it has state because CSRF token is generated for comment form also it is going to show flash messages.

@Toflar
Copy link
Contributor
Toflar commented Jun 12, 2020

The fact that you're accessing the session will start it.
When you do $session->all() or $session->isEmpty() etc. the session must be started to actually be able to provide you with the answer. It doesn't know if it's empty unless you start it :)

You can check if there even was a session cookie provided before you access the session. This way you can prevent it from starting:

if (!$request->hasPreviousSession()) {
    return;
}

if ($request->getSession()->isEmpty()) {
    // ...
}

@tugrul
Copy link
Author
tugrul commented Jun 12, 2020

@Toflar if there is a session without a session cookie on first request and will broke my event handler. I prefer to add $request->hasSession().

@tugrul
Copy link
Author
tugrul commented Jun 12, 2020

@nicolas-grekas I created a PoC. It shows when an action stateless and has state same time. tugrul/slcc-poc@17f59f4

You can simply enable/disable App\EventSubscriber\SessionCacheControlSubscriber to see difference.

@nicolas-grekas
Copy link
Member

Closing as the behavior is correct, as I explained previously.
In 5.2, #36364 will help understand where the session is started so that it should be easier to understand the behavior.

adamfranco added a commit to middlebury/coursecatalog that referenced this issue Feb 19, 2025
adamfranco added a commit to middlebury/coursecatalog that referenced this issue Feb 19, 2025
@adamfranco
Copy link

For those stumbling on this issue later, I also ran into this issue in my application and found @tugrul 's proof of concept incredibly helpful. While I appreciate that the default behavior may be desired as a safe baseline, the lack of documentation of how to successfully work around it makes it difficult to figure out how to write Symfony applications that leverage reverse-proxy caching for anonymous requests that use the same routes as authenticated requests.

My implementation based on @tugrul's for Symofony 6.4 is documented in Best practice to allow reverse-proxy caching of anonymous responses?.

Would there be support for further fleshing out the documentation on [HTTP Caching and User Sessions](https://symfony.com/doc/current/http_cache.html#http-caching-and-user-sessions) to include a working onKernelResponse listener?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants
0