CSRF exception for LogoutListener Firewall isn't handled #36814
Comments
|
Currently I'm using this workaround: In services:
App\Listener\LogoutListener:
tags:
- { name: kernel.event_listener, event: kernel.exception, method: onKernelException }
<?php
namespace App\Listener;
use Symfony\Component\HttpKernel\Event\ExceptionEvent;
use Symfony\Component\Security\Core\Exception\LogoutException;
use Symfony\Component\HttpFoundation\RedirectResponse;
class LogoutListener
{
public function onKernelException(ExceptionEvent $event)
{
$exception = $event->getThrowable();
do {
if ($exception instanceof LogoutException) {
$this->handleLogoutException($event, $exception);
return;
}
} while (null !== $exception = $exception->getPrevious());
}
private function handleLogoutException(ExceptionEvent $event, LogoutException $exception): void
{
// CSRF errors are normal so for those we just redirect page
if ($exception->getMessage() !== 'Invalid CSRF token.') {
return;
}
try {
$event->setResponse(new RedirectRespon
8000
se('/'));
$event->allowCustomResponseCode();
} catch (\Exception $e) {
$event->setThrowable($e);
}
}
} |
|
Thanks for filling in a detailed bug report @davispuh! In the PR adding this feature many years ago (ref #3007), it was succesfully transformed into an access denied response. In 46071f3, this code was refactored, but it seems like a mistake was made (the access denied response was moved in front of some nested if's, but the one for logout needs to be kept if I'm correct). So I would argue that this is a bug in all currently maintained versions of Symfony. @chalasr do you agree? |
|
8 years later, ... no idea :) |
|
If no Symfony test fails when making the change, I'm 👍 for changing it and writing a regression test. If it causes bad bugs, it'll probably be catched in a 5.1.0 RC release |
This PR was merged into the 3.4 branch. Discussion ---------- [Security] Fixed handling of CSRF logout error | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #36814 | License | MIT | Doc PR | - 8 years ago, a typo was made while refactoring the `ExceptionListener`, loosing this logic (46071f3). I think we should fix it. The `LogoutException` is a very generic name for something only used when the CSRF token is invalid. Should we match the exception message to make sure only this CSRF error is transformed into 403? I didn't yet do it because any usage of `LogoutException` would have resulted in 500, which always is worse than 403. Commits ------- 50348f2 Fixed handling of CSRF logout error
|
I still had to do something similar to handle the error more gracefully. I don't know if there's a way this could be done better in the framework, but right now getting a 403 error page when you are trying to logout with an expired session (but remember-me logs you back in.. the CSRF remains invalid tho) is not a great user experience. Here if anyone is interested composer/packagist@4697125 |
Symfony version(s) affected: v5.0.8 (and git master)
Description
Currently when you use CSRF validation for logout then when invalid CSRF is submitted HTTP 500 will be returned and exception logged.
But this isn't correct because maybe user has wrong state (in which case we should inform them to refresh token) or even if it's attacker then we don't care about it (it's not server error, so HTTP 500 is wrong) as it's expected behavior.
How to reproduce
In
security.yamlThen make request to logout with wrong CSRF token.
Possible Solution
In Firewall
ExceptionListenerthere ishandleLogoutExceptionit could check if it's CSRF exception and set response to redirect.Additional context
Log
The text was updated successfully, but these errors were encountered: