Movatterモバイル変換


[0]ホーム

URL:


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

Sign up
Appearance settings

[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

Merged
fabpot merged 1 commit intosymfony:6.2fromMatTheCat:ticket_48339
Nov 26, 2022

Conversation

@MatTheCat
Copy link
Contributor

@MatTheCatMatTheCat commentedNov 26, 2022
edited
Loading

QA
Branch?6.2
Bug fix?yes
New feature?no
Deprecations?no
TicketsFix#48339
LicenseMIT
Doc PRN/A

The tokenmanager service ID configuration node is calledcsrf_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 bythe documentation).

csrf_token_generator should ideally be deprecated and renamedcsrf_token_manager.

Tona-Mangobyte reacted with thumbs up emoji
@chalasr
Copy link
Member

chalasr commentedNov 26, 2022
edited
Loading

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

MatTheCat reacted with thumbs up emoji

@chalasr
Copy link
Member

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

@MatTheCat
Copy link
ContributorAuthor

MatTheCat commentedNov 26, 2022
edited
Loading

The issue stems from the tokenmanager service ID configuration node being calledcsrf_token_generator. As such it has been wrongly assumed in#46580 (and inthe blog post)security.csrf.token_generator was a correct value, whereassecurity.csrf.token_manager should be used (this is reflected bythe 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 on6.3?

@chalasr
Copy link
Member

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

@wouterj
Copy link
Member

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.

@wouterjwouterj modified the milestones:6.2,6.3Nov 26, 2022
@MatTheCat
Copy link
ContributorAuthor

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 against6.2.0-BETA1 and the error really comes from thecsrf_token_generator value set tosecurity.csrf.token_generator whenenable_csrf is true.

@MatTheCatMatTheCat changed the title[SecurityBundle] Fixlogout.csrf_token_generator default value and rename itlogout.csrf_token_manager[SecurityBundle] Fixlogout.csrf_token_generator default valueNov 26, 2022
@wouterjwouterj modified the milestones:6.3,6.2Nov 26, 2022
Copy link
Member

@wouterjwouterj 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
ContributorAuthor

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

Would a deprecation be ok for6.3?

@fabpot
Copy link
Member

Thank you@MatTheCat.

@fabpotfabpot merged commit25026b3 intosymfony:6.2Nov 26, 2022
@MatTheCatMatTheCat deleted the ticket_48339 branchNovember 26, 2022 16:49
@fabpotfabpot mentioned this pull requestNov 28, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@wouterjwouterjwouterj approved these changes

@chalasrchalasrAwaiting requested review from chalasrchalasr is a code owner

Assignees

No one assigned

Projects

None yet

Milestone

6.2

Development

Successfully merging this pull request may close these issues.

5 participants

@MatTheCat@chalasr@wouterj@fabpot@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp