TQ
dev.com

Blog about software development

Subscribe

Proposal to fix a 2012 bug in Symfony

28 May 2022 - by 'Maurits van der Schee'

When your Symfony (PHP web framework) project uses AJAX requests and sessions (logging in) you may run into this 2012 bug where Symfony does not lock the session allowing for data loss on concurrent AJAX requests. I fixed the bug in 2014 in the SncRedisBundle, but that merge was reverted last year, creating an issue for some high traffic sites. In this post I propose a better solution for Symfony.

Quick workaround

The quick workaround is to use the NativeFileSessionHandler class, which uses the session storage (handler) in the php.ini (using session.save_handler and session.save_path) that does store session files on disk. As long as sessions are not working reliable (due to lack of locking) I advice to stay away from Symfony's Redis and Memcache support for session storage and use this native variant (that does support locking).

ini_set('session.save_handler', 'files');
ini_set('session.save_path', sys_get_temp_dir());

See: https://github.com/symfony/symfony/blob/6.1/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/NativeFileSessionHandler.php

Performance impact

Some people argue that locking is bad for performance and they are right. It is bad to lock a session on a single action as this would prevent the same session to run another action in parallel (using another apache/php thread). You will mainly notice this is an AJAX heavy application as HTML requests are serialized by your browser anyway. This locking is only needed when the action modifies the session data, which is something we do not know in advance (or do we?). Hence the following proposal, please read on.

Proposal (read-only sessions)

What I think would be an optimal solution for both performance and correctness: I suggest we look at the verb (GET or not) to indicate that the action (may) write to the session (on POST, PUT or DELETE) or when it should be read-only (GET). If the action may write to the session we open the session (identified by the session identifier) with a lock and write it back after the action completes. If it is a GET we open the session in read-only mode (read stale while locked) and throw an exception when people write to it. Note that the session cleanup must take GET requests (with read-only session) into account by writing an updated timestamp in the session store.

Two new session primitives

If you want to implement this optimally you need (additional to session locking) support for reading without locking and for updating the timestamp without locking (to avoid session timeouts). In Symfony the session timestamp can be updated using the updateTimestamp function when the session is read (and not written to). Although I did find the updateTimestamp method, it should not be implemented by writing the data as that defeats the purpose. I also did not find where that method was actually used in the framework. The PHP standard library is lacking the required functionality.

PHP core team got it wrong

Updating the timestamp is a pitfall as the in PHP 7 added read_and_close flag to the session_start function showed (not updating the timestamp), which PHP also does not do when session.lazy_write is set to 1 (the default). This does not affect Debian based systems as they manage file based PHP sessions using a script in /etc/cron.d/php that calls /usr/lib/php/sessionclean every half hour that touches all active sessions in /var/lib/php/sessions and removes session files that are too old. Note that read_and_close still blocks and is therefor of little use. We actually need a read_dont_lock (read stale) option.

See: https://stackoverflow.com/questions/37789172/php-session-randomly-dies-when-read-and-close-is-active

Idempotency exceptions

Proposed solution: When people write to the session (or to the database) on a GET request you throw an error unless an annotation is set (to allow non-idempotent behavior). This requires an alternative to the "stateless" annotation of the route, called "idempotent", which is less strict and is used to mark idempotency exceptions (non-idempotency on GET requests or idempotency on non-GET requests).

// src/Controller/MainController.php
namespace App\Controller;

use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\Routing\Annotation\Route;

class MainController extends AbstractController
{
    /**
     * @Route("/", name="homepage", idempotent=false)
     */
    public function homepage()
    {
        // ...
    }
}

Note that the idempotent=false annotation is part of this proposal and not actually supported by Symfony (yet).

See: https://symfony.com/doc/current/routing.html#stateless-routes

Warnings and errors write to the session

Symfony promotes the use of a session stored "FlashBag" to transfer warnings and errors from one route to the next. This allows for the "Post/Redirect/Get" (PRG) pattern, having different routes for the form retrieval and the form submission action. This may seem to simplify the controller implementation, but I'm not even sure that it does. From a performance perspective I'm not a fan of this pattern as it increases the number of session writes.

See: https://stackoverflow.com/questions/10827242/understanding-the-post-redirect-get-pattern

CSRF tokens write to the session

Cross Site Request Forgery (CSRF) protection avoids blind form submission in case of XSS vulnerabilites. This type of form submission protection uses CSRF tokens that writes to the session on every form retrieval request (in Symfony). Although that may certainly be true for Symfony, that is not a necessity for CSRF protection, as you can read here:

CSRF tokens should be generated on the server-side. They can be generated once per user session or for each request. Per-request tokens are more secure than per-session tokens as the time range for an attacker to exploit the stolen tokens is minimal. However, this may result in usability concerns.

See: https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#synchronizer-token-pattern

Conclusion

Writing reliably to the session requires session locking. Symfony does not lock sessions in it's session implementation(s), while PHP natively does. Session locking has performance impact and the number of session locks should be reduced. PHP (core) does not properly support read-only sessions (allowing stale reads) nor updating the session timestamp. Symfony on the other hand is not promoting idempotency with respect to session storage in it's Flashbag and CSRF implementations, causing even more problems. Proper session locking seems to be a problem people don't understand very well. I hope this post helps.

See: https://github.com/symfony/symfony/issues/4976

Enjoy!


PS: Liked this article? Please share it on Facebook, Twitter or LinkedIn.