-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[SecurityBundle] Fix logout.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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
2d29981
to
63acf45
Compare
Can you please give more insights about what's causing the bug at hand (e.g. which PR introduced it and why)? |
Introducing deprecations is preferably avoided at this stage, we need a strong reason for doing so. |
63acf45
to
0e429a7
Compare
The issue stems from the token manager 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 |
Thank you. I'll let @wouterj answer your questions as I didn't follow this change. |
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. |
Okay, I’ll update this PR to only change the default value.
Indeed, but I was able to reproduce the issue against |
0e429a7
to
1a0cf86
Compare
logout.csrf_token_generator
default value and rename it logout.csrf_token_manager
logout.csrf_token_generator
default value
1a0cf86
to
74b52a7
Compare
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
Outdated
Show resolved
Hide resolved
74b52a7
to
df539e2
Compare
Yeah sorry I tend to do many things at once in my PRs 😬 Would a deprecation be ok for |
Thank you @MatTheCat. |
The token manager service ID configuration node is called
csrf_token_generator
. As such it has been wrongly assumed in #46580security.csrf.token_generator
was a good default value, whereassecurity.csrf.token_manager
should be used (this is reflected by the documentation).csrf_token_generator
should ideally be deprecated and renamedcsrf_token_manager
.