AlertifySessionHandler trigger a session_start even on stateless URL

[damienalexandre]

I'm not very familiar with this bundle but I encountered an issue while working on a project where it's used to display notifications.

I had a simple page hit by a monitoring application (like a curl call to a page). As the time passed, my session table was becoming very large so I started digging.

Even if my simple page was outside a Security Firewall (using a security: false firewall), a session was still assigned to my page. That meant every hit was creating a new session where I don't wanted to.

This is because AlertifyBundle only look at the Response:

https://github.com/Troopers/AlertifyBundle/blob/eded39af141d056627718d2ba7dee4e458fb7187/EventListener/AlertifyListener.php#L66-L79

If there is a </body> tag, alertifySessionHandler gets called:

https://github.com/Troopers/AlertifyBundle/blob/eded39af141d056627718d2ba7dee4e458fb7187/Handler/AlertifySessionHandler.php#L43

And this simple $session->getFlashBag()->all(); trigger a session start, whatever we want to do.

I think this may be a bug, as I don't see a valid usage of Flash Message outside of pages where a session is already started.

Suggestion

I think you should check for session existence before trying to load data from it.

A example implementation could be:

    protected function injectAlertify(Response $response, Request $request)
    {
        $content = $response->getContent();
        $endBodyPos = strripos($content, '</body>');
        $hasBody = false !== $endBodyPos;
        $hasMetaRefresh = false !== strripos($content, 'http-equiv="refresh"');
        $forceInject = $response->headers->get('X-Inject-Alertify', false);
        $isRedirectResponse = $response instanceof RedirectResponse;
+        $isSessionStarted = $this->session->isStarted();

+        if ($hasBody && $isSessionStarted && !$hasMetaRefresh && !$isRedirectResponse || $forceInject) {
-        if ($hasBody && !$hasMetaRefresh && !$isRedirectResponse || $forceInject) {
            if ($response->getStatusCode() === 204) {
                throw new IncompatibleStatusCodeException();
            }
            $alertify = $this->alertifySessionHandler->handle($this->session, $this->twig);
            if ($endBodyPos) {
                $content = substr($content, 0, $endBodyPos).$alertify.substr($content, $endBodyPos);
            } else {
                $content .= $alertify;
            }
            $response->setContent($content);
        }
    }

What do you think? Does that sound reasonable according to the bundle features? If you agree I would be pleased to submit a Pull Request.

Cheers.

[paulandrieux]

Hi Damien !

Symfony docs mention this issue: https://symfony.com/doc/current/session.html#avoid-starting-sessions-for-anonymous-users

A fix compliant to this doc could be something like:

    protected function injectAlertify(Response $response, Request $request)
    {
        $content = $response->getContent();
        $endBodyPos = strripos($content, '</body>');
        $hasBody = false !== $endBodyPos;
        $hasMetaRefresh = false !== strripos($content, 'http-equiv="refresh"');
        $forceInject = $response->headers->get('X-Inject-Alertify', false);
        $isRedirectResponse = $response instanceof RedirectResponse;
+        $hasPreviousSession = $request->hasPreviousSession();

+        if ($hasBody && $hasPreviousSession && !$hasMetaRefresh && !$isRedirectResponse || $forceInject) {
-        if ($hasBody && !$hasMetaRefresh && !$isRedirectResponse || $forceInject) {
            if ($response->getStatusCode() === 204) {
                throw new IncompatibleStatusCodeException();
            }
            $alertify = $this->alertifySessionHandler->handle($this->session, $this->twig);
            if ($endBodyPos) {
                $content = substr($content, 0, $endBodyPos).$alertify.substr($content, $endBodyPos);
            } else {
                $content .= $alertify;
            }
            $response->setContent($content);
        }
    }

What do you think ?

[damienalexandre]

Yes absolutely, hasPreviousSession() is smarter :+1: and will fix my issue too :+1:

[paulandrieux]

Released :tada: https://github.com/Troopers/AlertifyBundle/releases/tag/3.0.13

[damienalexandre]

Thank you!

[remyvanlerberghe]

@paulandrieux Can you release it on Packagist as well please ? Thank you!