Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[SecurityBundle] Fixlogout.csrf_token_generator default value#48341
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
chalasr commentedNov 26, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Can you please give more insights about what's causing the bug at hand (e.g. which PR introduced it and why)? |
chalasr commentedNov 26, 2022
Introducing deprecations is preferably avoided at this stage, we need a strong reason for doing so. |
MatTheCat commentedNov 26, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The issue stems from the tokenmanager service ID configuration node being called
Okay so changing the default value only should be good. Do you think the configuration could be renamed on |
chalasr commentedNov 26, 2022
Thank you. I'll let@wouterj answer your questions as I didn't follow this change. |
wouterj commentedNov 26, 2022
This config option has been there since Symfony 2.4 (#9587), so this is not something we should rush for 6.2 imho. Also, this does not seem to be related to#48339 to me, as the issue talks about an error between 6.2.0-beta3 and 6.2.0-rc1. Nothing changed in relation to this config option between those releases. |
MatTheCat commentedNov 26, 2022
Okay, I’ll update this PR to only change the default value.
Indeed, but I was able to reproduce the issue against |
logout.csrf_token_generator default value and rename itlogout.csrf_token_managerlogout.csrf_token_generator default value
wouterj left a comment
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.
Ah, I didn't see you also changed the default value. Yeah, this seems like a 6.2 bug.
Thanks for adding a test + fix!
src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/Logout/config_csrf_enabled.yml OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
MatTheCat commentedNov 26, 2022
Yeah sorry I tend to do many things at once in my PRs 😬 Would a deprecation be ok for |
fabpot commentedNov 26, 2022
Thank you@MatTheCat. |
Uh oh!
There was an error while loading.Please reload this page.
The tokenmanager service ID configuration node is called
csrf_token_generator. As such it has been wrongly assumed in#46580security.csrf.token_generatorwas a good default value, whereassecurity.csrf.token_managershould be used (this is reflected bythe documentation).csrf_token_generatorshould ideally be deprecated and renamedcsrf_token_manager.