Invalid request token error when sending invalid form again

[fritzmg]

Affected version(s)

4.13, 5.3

Description

Unfortunately there is still another regression with the changes from #6801, #6959 and #7052. There can be situations where you encounter an "invalid request token" error during regular use. See the following reproduction:

1. Create a form.
2. Activate Disable HTML5 validation.
3. Add a mandatory text form field.
4. Add the security question form field.
5. Add a submit button.
6. Add the form to a page.
7. Open the page in the front end without any pre-existing cookies.
8. Submit the form without any input in the text field - you will see the form again with the error message next to the mandatory field.
9. Immediately submit the form again (within 5 seconds I think).
10. You will see the invalid request token error message.

Contao\CoreBundle\Exception\InvalidRequestTokenException:
Invalid CSRF token. Please reload the page and try again.

  at vendor/contao/contao/core-bundle/src/EventListener/RequestTokenListener.php:102

I notice the following behaviour:

1. The initial GET request to the page with the form does not respond with any cookies, which is expected.
2. The POST request that will show the form again (as the form does not validate) responds with a PHPSESSID cookie, which is expected as the form's values are stored in the session for a short period. However, there is no CSRF token cookie in the response, which I think is unexpected. Whenever there is a session cookie there should always be a CSRF token cookie.
3. Contao' security form field will make an AJAX request to _contao/captcha/…. There will be a CSRF token cookie in the response - but there might also be a PHPSESSID=delete instruction in the response.
4. There will be a second AJAX request to _contao/captcha/… which then also deletes the CSRF token cookie.

I think overall the core issue is that the CSRF token cookie is missing in the response of the POST request.

[fritzmg]

I think our checks for an empty session in various places (#6801, #6959 and #7052) are flawed, at least with the default NativeSessionStorage. These are my findings so far:

1. When a form is submitted a session is always started here (via LazySessionAccess), regardless of whether anything actually needs to be stored in the session:

   contao/core-bundle/src/Resources/contao/forms/Form.php

   Line 628 in eb6c497

   $_SESSION['FORM_DATA'] = \is_array($_SESSION['FORM_DATA'] ?? null) ? $_SESSION['FORM_DATA'] : array();

2. If NativeSessionStorage is in use, this will execute session_start().
3. If I understand this correctly, session_start() will always immediately send the PHPSESSID cookie (depending on the PHP configuration of course), bypassing Symfony's response object - again regardless of whether anything is actually stored in the session or not.
4. Our CsrfTokenCookieSubscriber would first check whether there are any cookies in the response, but there will be none in the response object (despite the PHPSESSID cookie having been sent already).
5. Next our CsrfTokenCookieSubscriber now checks whether the session is started and not empty. It is started - but it will always be empty, even if there was a field that was valid: our session check does not pick up on the data present in $_SESSION['FORM_DATA']. This needs further investigation.

My thoughts:

• We need to revert Handle empty sessions in the CSRF cookie subscriber #6801, Do not make a response private if the session is empty #6959 and Skip token validation if the session is empty and new #7052 as the empty session check is unreliable for at least the NativeSessionStorage in general and due to the $_SESSION['FORM_DATA'] being ignored.
• Once we fix the latter may be not all of the aforementioned changes need to be reverted (e.g. the changes to the MakeResponsePrivate listener could then stay I suppose).
• We could of course make an adjustment that always only checks for $session->isStarted() if the session is an instance of NativeSessionStorage (since the PHPSESSID cookie will always be present then). But can we actually be sure that other session storage solutions are not affected by similar issues? I have no experience with other session storages within Contao.

[ausi]

I think our checks for an empty session in various places (#6801, #6959 and #7052) are flawed, at least with the default NativeSessionStorage.

I think checking for an empty session is generally correct, but we should also check if PHP has sent a PHPSESSID cookie itself, see #7213

See the following reproduction:

I was not able to reproduce this in Contao 5.3.

[fritzmg]

I was not able to reproduce this in Contao 5.3.

You are right, it does indeed not happen in Contao 5.3. At least not this way - because we only write to the session there when the form validates (and when we do, we do it properly).

I tried to force the issue in Contao 5 with something like this:

// src/EventListener/PrepareFormDataListener.php
namespace App\EventListener;

use Contao\CoreBundle\DependencyInjection\Attribute\AsHook;
use Symfony\Component\HttpFoundation\RequestStack;

#[AsHook('prepareFormData')]
class PrepareFormDataListener
{
    public function __construct(private readonly RequestStack $requestStack)
    {
    }

    public function __invoke(): void
    {
        func_get_arg(3)->addError('Nope!');
        $this->requestStack->getCurrentRequest()->getSession()->start();
    }
}

But weirdly no PHPSESSID cookie is sent there - even though I manually force the session to start 🤔

[ausi]

I tried to force the issue in Contao 5 with something like this:

You also need to make the native $_SESSION non-empty in order to trigger it, e.g. like so:

$this->requestStack->getCurrentRequest()->getSession()->start();
$_SESSION['FOO'] = 'bar';

Otherwise the session cookie gets cleared here: https://github.com/symfony/symfony/blob/95a9bd778786d3d6593ba214299b472e27fc308e/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/AbstractSessionHandler.php#L80

This way I was able to reproduce the issue in Contao 5, and #7213 also fixes it 🎉

[fritzmg]

Closing in favor of #7213