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

Avoid regenerating the remember me token if it is still fresh#40972

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:5.xfromSeldaek:patch-12
May 7, 2021

Conversation

@Seldaek
Copy link
Member

QA
Branch?5.x
Bug fix?~yes
New feature?no?
Deprecations?no
TicketsRefs#40971
LicenseMIT
Doc PR

Please see#40971 for more information about the context of this change.

As it was discussed in#18384 - regenerating the remember me token/cookie is done to avoid old cookies being stolen and reused, this is a valid concern (although cookie theft is much harder these days with httpOnly and secure flags) and a good security practice, but if the token was refreshed very recently it seems a bit overkill to refresh it again, it leads to more DB writes, and for us who are trying to support concurrent re-authenticating requests it is causing further problems if every request triggers a new token update.

I'd be happy to also update this in the old PersistentTokenBasedRememberMeServices if needed, but I find that it is perhaps better to just do this in the new auth system as it was until 5.3 considered experimental.

kevinpapst reacted with thumbs up emoji
Copy link
Member

@chalasrchalasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks sensible to me, thanks. Fixes#28314 also?

@Seldaek
Copy link
MemberAuthor

No it makes it possible/easier to fix that issue of parallel reauth requests but this in itself is not enough to fix it. I'll try and send another PR to add event hooks or an optional interface perhaps allowing the remember me token storage to implement a fix, then we have a chance of fixing this by default in 6 perhaps.

@fabpot
Copy link
Member

Thank you@Seldaek.

@fabpotfabpot merged commit883899e intosymfony:5.xMay 7, 2021
@fabpotfabpot mentioned this pull requestMay 9, 2021
@SeldaekSeldaek deleted the patch-12 branchMay 11, 2021 08:59
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@chalasrchalasrchalasr approved these changes

@wouterjwouterjAwaiting requested review from wouterj

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Seldaek@fabpot@chalasr@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp