Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Security] Skip clearing CSRF Token on stateless logout#50312
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| @@ -31,6 +32,10 @@ public function __construct(ClearableTokenStorageInterface $csrfTokenStorage) | |||
| public function onLogout(LogoutEvent $event): void | |||
| { | |||
| if ($this->csrfTokenStorage instanceof SessionTokenStorage && !$event->getRequest()->hasPreviousSession()) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What about fixing it inSessionTokenStorage instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
UsingSessionTokenStorage without a session has been deprecated in 5.x:
symfony/src/Symfony/Component/Security/Csrf/TokenStorage/SessionTokenStorage.php
Lines 95 to 108 in8c637a5
| publicfunctionclear() | |
| { | |
| $session =$this->getSession(); | |
| foreach (array_keys($session->all())as$key) { | |
| if (str_starts_with($key,$this->namespace.'/')) { | |
| $session->remove($key); | |
| } | |
| } | |
| } | |
| /** | |
| * @throws SessionNotFoundException | |
| */ | |
| privatefunctiongetSession():SessionInterface |
Ideally this listener shouldn't be registered for stateless firewalls, problem is that it's not a per-firewall listener but a global one. We should probably change that in another (feature) PR.
Any way to test this? |
Thank you@chalasr. |
Sure, at least something preventing regressions. I'll do! |
…tateless logout (chalasr)This PR was merged into the 6.2 branch.Discussion----------[Security] Test `CsrfTokenClearingLogoutListener` with stateless logout| Q | A| ------------- | ---| Branch? | 6.2| Bug fix? | no| New feature? | no| Deprecations? | no| Tickets |#50312 (comment)| License | MIT| Doc PR | -Commits-------099ba75 [Security] Test `CsrfTokenClearingLogoutListener` with stateless logout
Not targeting 5.4 LTS as the bug is only breaking on 6.3 although it does exist on prior versions.