Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[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

Merged
merged 1 commit into from
Nov 26, 2022

Conversation

MatTheCat
Copy link
Contributor

@MatTheCat MatTheCat commented Nov 26, 2022

Q A
Branch? 6.2
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #48339
License MIT
Doc PR N/A

The token manager service ID configuration node is called csrf_token_generator. As such it has been wrongly assumed in #46580 security.csrf.token_generator was a good default value, whereas security.csrf.token_manager should be used (this is reflected by the documentation).

csrf_token_generator should ideally be deprecated and renamed csrf_token_manager.

@chalasr
Copy link
Member

chalasr commented Nov 26, 2022

Can you please give more insights about what's causing the bug at hand (e.g. which PR introduced it and why)?

@chalasr
Copy link
Member

chalasr commented Nov 26, 2022

Introducing deprecations is preferably avoided at this stage, we need a strong reason for doing so.

@MatTheCat
Copy link
Contributor Author

MatTheCat commented Nov 26, 2022

The issue stems from the token manager service ID configuration node being called csrf_token_generator. As such it has been wrongly assumed in #46580 (and in the blog post) security.csrf.token_generator was a correct value, whereas security.csrf.token_manager should be used (this is reflected by the documentation).

Introducing deprecations is preferably avoided at this stage, we need a strong reason for doing so.

Okay so changing the default value only should be good. Do you think the configuration could be renamed on 6.3?

@chalasr
Copy link
Member

chalasr commented Nov 26, 2022

Thank you. I'll let @wouterj answer your questions as I didn't follow this change.

@wouterj
Copy link
Member

wouterj commented Nov 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.

@wouterj wouterj modified the milestones: 6.2, 6.3 Nov 26, 2022
@MatTheCat
Copy link
Contributor Author

this is not something we should rush for 6.2 imho

Okay, I’ll update this PR to only change the default value.

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.

Indeed, but I was able to reproduce the issue against 6.2.0-BETA1 and the error really comes from the csrf_token_generator value set to security.csrf.token_generator when enable_csrf is true.

@MatTheCat MatTheCat changed the title [SecurityBundle] Fix logout.csrf_token_generator default value and rename it logout.csrf_token_manager [SecurityBundle] Fix logout.csrf_token_generator default value Nov 26, 2022
@wouterj wouterj modified the milestones: 6.3, 6.2 Nov 26, 2022
Copy link
Member

@wouterj wouterj left a 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!

@MatTheCat
Copy link
Contributor Author

Yeah sorry I tend to do many things at once in my PRs 😬

Would a deprecation be ok for 6.3?

@fabpot
Copy link
Member

fabpot commented Nov 26, 2022

Thank you @MatTheCat.

@fabpot fabpot merged commit 25026b3 into symfony:6.2 Nov 26, 2022
@MatTheCat MatTheCat deleted the ticket_48339 branch November 26, 2022 16:49
@fabpot fabpot mentioned this pull request Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.