Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Security] Do not try to clear CSRF on stateless request #54742

Open
wants to merge 3 commits into
base: 6.4
Choose a base branch
from
Open

[Security] Do not try to clear CSRF on stateless request #54742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1d10333
[Security] Do not try to clear CSRF on stateless request
Seb33300 Apr 26, 2024
3d4b434
Add test to clear CSRF on stateless request
Seb33300 Apr 26, 2024
e7323b3
Fix LogoutTest
Seb33300 Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion ...ny/Bundle/SecurityBundle/Tests/Functional/app/LogoutWithoutSessionInvalidation/config.yml
View file
Open in desktop
Expand Up @@ -21,4 +21,3 @@ security:
secret: secret
logout:
invalidate_session: false
stateless: true
chalasr marked this conversation as resolved.
Show resolved Hide resolved
7 changes: 6 additions & 1 deletion src/Symfony/Component/Security/Http/EventListener/CsrfTokenClearingLogoutListener.php
View file
Open in desktop
Expand Up @@ -32,7 +32,12 @@ public function __construct(ClearableTokenStorageInterface $csrfTokenStorage)

public function onLogout(LogoutEvent $event): void
{
if ($this->csrfTokenStorage instanceof SessionTokenStorage && !$event->getRequest()->hasPreviousSession()) {
$request = $event->getRequest();

if (
$this->csrfTokenStorage instanceof SessionTokenStorage
&& ($request->attributes->getBoolean('_stateless') || !$request->hasPreviousSession())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why do we have a previous request in the first place in your situation?
If there is a previous session, we should clean the session even of this contradicts the stateless flag, or we might create a security issue, or?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our API and website are sharing the same domain.
API is accessible from domain.com/api/
The user can be authenticated on the website using session and also using JWT on the API on the same device.

When the user already has a session on the website and try to logout from the API, it results in this log message.
(On the API we use the logout endpoint to delete refresh tokens of the user in our db)
Because the logout listener is only checking if the request ha a previous session when all other listeners also check if the request is stateless.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, this is also how works others logout listeners.
If the user logout on a stateless request (and also has a session on the same device), his session is not cleared.
So we should also clear the CSRF token too on statleless requests.

) {
return;
}

Expand Down
24 changes: 23 additions & 1 deletion ...mfony/Component/Security/Http/Tests/EventListener/CsrfTokenClearingLogoutListenerTest.php
View file
Open in desktop
Expand Up @@ -15,13 +15,14 @@
use Symfony\Component\HttpFoundation\Exception\SessionNotFoundException;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\HttpFoundation\Session\Session;
use Symfony\Component\Security\Csrf\TokenStorage\SessionTokenStorage;
use Symfony\Component\Security\Http\Event\LogoutEvent;
use Symfony\Component\Security\Http\EventListener\CsrfTokenClearingLogoutListener;

class CsrfTokenClearingLogoutListenerTest extends TestCase
{
public function testSkipsClearingSessionTokenStorageOnStatelessRequest()
public function testSkipsClearingSessionTokenStorageOnRequestWithoutSession()
{
try {
(new CsrfTokenClearingLogoutListener(
Expand All @@ -33,4 +34,25 @@ public function testSkipsClearingSessionTokenStorageOnStatelessRequest()

$this->addToAssertionCount(1);
}

public function testSkipsClearingSessionTokenStorageOnStatelessRequest()
{
$session = new Session();

// Create a stateless request with a previous session
$request = new Request();
$request->setSession($session);
$request->cookies->set($session->getName(), 'previous_session');
$request->attributes->set('_stateless', true);

try {
(new CsrfTokenClearingLogoutListener(
new SessionTokenStorage(new RequestStack())
))->onLogout(new LogoutEvent($request, null));
} catch (SessionNotFoundException) {
$this->fail('clear() must not be called if the request is stateless');
}

$this->addToAssertionCount(1);
}
}