8000 RememberMe cookie should only contain the bare minimum of details · Issue #42349 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

RememberMe cookie should only contain the bare minimum of details #42349

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.

Sign up for GitHub

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

Jump to bottom
Closed
zerkms opened this issue Aug 2, 2021 · 17 comments · May be fixed by #59232
Closed

RememberMe cookie should only contain the bare minimum of details #42349

zerkms opened this issue Aug 2, 2021 · 17 comments · May be fixed by #59232
Labels
Security

Comments

@zerkms
Copy link
Contributor
zerkms commented Aug 2, 2021

Description

As I mentioned at #40145 (comment) at this moment remember-me cookie exposes a userFqcn. It leaks implementation details of the application. And it's not used anywhere.

I understand now it's too late - but what was the reason to include it originally?

Example
@xabbuh xabbuh added the Security label Aug 4, 2021
@vasilvestre vasilvestre mentioned this issue Sep 13, 2021
Closed
@carsonbot
Copy link

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

@zerkms
Copy link
Contributor Author
zerkms commented Feb 6, 2022

Has this been resolved?

Nope.

@carsonbot carsonbot removed the Stalled label Feb 6, 2022
@nicolas-grekas
Copy link
Member

Would you mind investigating? If it's not used let's remove it indeed.

@wouterj
Copy link
Member
wouterj commented Feb 6, 2022

I am all in for removing, but me and @vasilvestre have tried to do so in #43006 and we both cannot find a way to remove it in a backwards compatible matter.

@carsonbot
Copy link

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

@wouterj wouterj closed this as completed Sep 4, 2022
@zerkms
Copy link
Contributor Author
zerkms commented Sep 4, 2022 •
edited
Loading

@wouterj how about removing it in a backward incompatible manner? There is Symfony 7, where it can be done.

When I reported it - php 6.0 was not released, so it could have been done back then. But nonetheless, what's bad in waiting another year and fixing it, instead of dragging it til the heat death of the universe?

@wouterj
Copy link
Member
wouterj commented Sep 4, 2022

Any change must have a smooth upgrade path in Symfony. This means that the only BC break allowed in a new major is removing code that was deprecated (and properly reported as such) in the previous minor version.

Besides that, this is a minor thing. It would have been nice to clean it up, but it's not worth massively complex BC layers.

@zerkms
Copy link
Contributor Author
zerkms commented Sep 4, 2022 •
edited
Loading

RememberMe is a short living cookie, it's not a broken experience if a user needs to reauthenticate.

No need for any BC layers - just release it and let users reauthenticate.

RememberMe was broken in some way or another for 2 years, and it was not a problem.

@vasilvestre
Copy link

RememberMe is a short living cookie, it's not a broken experience if a user needs to reauthenticate.

No need for any BC layers - just release it and let users reauthenticate.

RememberMe was broken in some way or another for 2 years, and it was not a problem.

May be wrong but it breaks your code (that's what BC is about). Having users disconnected is not important, but breaking BC policy is afaik

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Sep 6, 2022 •
edited
Loading

My advice: deprecate RememberMeDetails::getUserFqcn().

We still have to accept the fqcn as constructor argument but in Symfony 7.1, we'll be able to deprecate passing 4 arguments to the constructor, and we'll be able to generate cookies with no FQCN without breaking anything.

PR welcome.

@nicolas-grekas nicolas-grekas reopened this Sep 6, 2022
@carsonbot carsonbot removed the Stalled label Sep 6, 2022
@nicolas-grekas
Copy link
Member

Same for PersistentToken::getClass() I suppose.
Note that I reopened; but we might close again if nobody works on it ;)

@nicolas-grekas
Copy link
Member

Let's to it the reverse way: closing, but PR still welcome if someone wants to dive this topic.

@zerkms
Copy link
Contributor Author
zerkms commented Sep 6, 2022

@vasilvestre

May be wrong but it breaks your code (that's what BC is about)

it does not.

@zerkms
Copy link
Contributor Author
zerkms commented Sep 6, 2022

@nicolas-grekas

but in Symfony 7.1, we'll be able to deprecate passing 4 arguments to the constructor

is it not possible to deprecate it now in 6.x and entirely remove in 7.x?

@vasilvestre
Copy link

Same for PersistentToken::getClass() I suppose. Note that I reopened; but we might close again if nobody works on it ;)

Just to be clear, we should remove the usage of the argument but still allow it despite the deprecation and remove it later ?

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Sep 7, 2022 •
edited
Loading

Because we just deprecate the method, we still need a way to make it return something useful. That's what the argument is for.
Maybe we could deprecate passing it in the same run, but then this makes an abrupt BC layer, where everything must be fixed together. This might not be possible unless you're in control of everything (including deps, in theory at least.)

@phucwan91
Copy link
Contributor

Im wondering, why we don't simply md5 or hash the $userFqcn 🤔

@thereisnobugs thereisnobugs mentioned this issue Dec 17, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Security
Projects
None yet
Milestone
No milestone
7 participants
@nicolas-grekas @zerkms @wouterj @xabbuh @carsonbot @vasilvestre @phucwan91
0