Skip to content
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

[Security] Remember_me cookie doesn't get deleted correctly #35198

Closed
Claicon opened this issue Jan 3, 2020 · 4 comments
Closed

[Security] Remember_me cookie doesn't get deleted correctly #35198

Claicon opened this issue Jan 3, 2020 · 4 comments

Comments

@Claicon
Copy link

Claicon commented Jan 3, 2020

Symfony version(s) affected: 4.4.2

Description
The remember_me cookie should get deleted after User is deauthenticated with EquatableInterface or normal hasUserChanged after changing roles of the user while in an active session. It works correctly when doing that to the username, password or isActive/banned flag as an example, but doesn't work if roles are changing. After changing the roles a second time it works. I thought this was fixed with #34671 but doesn't work for me with roles.

How to reproduce

  1. login with a user and the remember_me cookie active.
  2. change role of the user (with EquatableInterface or normal hasUserChanged covers role-changes too).
  3. check if user is still logged in / in an active session.

Additional context
Here are the logs directly after I changed the role and reloaded the page. The remember-me cookie is being cleared but there's a new remember-me cookie detected afterwards and accepted again and session is still kinda active (is_fully_authenticated() returns false, is_authenticated() returns true).

[2020-01-02 13:08:48] security.DEBUG: Read existing security token from the session. {"key":"_security_main","token_class":"Symfony\\Component\\Security\\Guard\\Token\\PostAuthenticationGuardToken"} []
[2020-01-02 13:08:48] security.DEBUG: Cannot refresh token because user has changed. {"username":"Claicon","provider":"Symfony\\Bridge\\Doctrine\\Security\\User\\EntityUserProvider"} []
[2020-01-02 13:08:48] security.DEBUG: Token was deauthenticated after trying to refresh it. [] []
[2020-01-02 13:08:48] security.DEBUG: Clearing remember-me cookie. {"name":"REMEMBERME"} []
[2020-01-02 13:08:48] security.DEBUG: Checking for guard authentication credentials. {"firewall_key":"main","authenticators":1} []
[2020-01-02 13:08:48] security.DEBUG: Checking support on guard authenticator. {"firewall_key":"main","authenticator":"App\\Security\\LoginFormAuthenticator"} []
[2020-01-02 13:08:48] security.DEBUG: Guard authenticator does not support the request. {"firewall_key":"main","authenticator":"App\\Security\\LoginFormAuthenticator"} []
[2020-01-02 13:08:48] security.DEBUG: Remember-me cookie detected. [] []
[2020-01-02 13:08:48] security.INFO: Remember-me cookie accepted. [] []
[2020-01-02 13:08:48] security.DEBUG: Populated the token storage with a remember-me token. [] []
[2020-01-02 13:08:48] security.DEBUG: Stored the security token in the session. {"key":"_security_main"} []

After changing the role AGAIN and reloading the page it seems to be fine, remember-me cookie is cleared and token populated with an anonymous Token (as it should be).

[2020-01-02 13:15:31] security.DEBUG: Read existing security token from the session. {"key":"_security_main","token_class":"Symfony\\Component\\Security\\Core\\Authentication\\Token\\RememberMeToken"} []
[2020-01-02 13:15:31] security.DEBUG: Cannot refresh token because user has changed. {"username":"Claicon","provider":"Symfony\\Bridge\\Doctrine\\Security\\User\\EntityUserProvider"} []
[2020-01-02 13:15:31] security.DEBUG: Token was deauthenticated after trying to refresh it. [] []
[2020-01-02 13:15:31] security.DEBUG: Clearing remember-me cookie. {"name":"REMEMBERME"} []
[2020-01-02 13:15:31] security.DEBUG: Checking for guard authentication credentials. {"firewall_key":"main","authenticators":1} []
[2020-01-02 13:15:31] security.DEBUG: Checking support on guard authenticator. {"firewall_key":"main","authenticator":"App\\Security\\LoginFormAuthenticator"} []
[2020-01-02 13:15:31] security.DEBUG: Guard authenticator does not support the request. {"firewall_key":"main","authenticator":"App\\Security\\LoginFormAuthenticator"} []
[2020-01-02 13:15:31] security.INFO: Populated the TokenStorage with an anonymous Token. [] []
@chalasr
Copy link
Member

chalasr commented Jan 3, 2020

@Claicon Could you please try the following patch?

diff --git a/src/Symfony/Component/Security/Http/RememberMe/PersistentTokenBasedRememberMeServices.php b/src/Symfony/Component/Security/Http/RememberMe/PersistentTokenBasedRememberMeServices.php
index 61b1257b4a..98e8756e33 100644
--- a/src/Symfony/Component/Security/Http/RememberMe/PersistentTokenBasedRememberMeServices.php
+++ b/src/Symfony/Component/Security/Http/RememberMe/PersistentTokenBasedRememberMeServices.php
@@ -52,6 +52,8 @@ class PersistentTokenBasedRememberMeServices extends AbstractRememberMeServices
             list($series) = $parts;
             $this->tokenProvider->deleteTokenBySeries($series);
         }
+
+        $request->cookies->remove($this->options['name']);
     }
 
     /**

@Claicon
Copy link
Author

Claicon commented Jan 3, 2020

@chalasr No it's still the same with the patch.

Why is it only when changing roles? I share my code just in case I'm doing something wrong here, my App only got one role so I just get it from a user field (getTypeName() is e.g. ROLE_USER) but that shouldn't be a problem here as I have seen this quite a few times.

/**
* @see UserInterface
*/
public function getRoles(): array
{
    $roles[] = $this->role->getTypeName();

    return array_unique($roles);
}

Can someone confirm this behaviour too so I can rule out that it is my code?

Thanks!

@chalasr
Copy link
Member

chalasr commented Jan 5, 2020

Confirmed, I'm on it.

@chalasr
Copy link
Member

chalasr commented Jan 7, 2020

See #35239

nicolas-grekas added a commit that referenced this issue Jan 8, 2020
…eing accepted (chalasr)

This PR was merged into the 3.4 branch.

Discussion
----------

[Security\Http] Prevent canceled remember-me cookie from being accepted

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #35198
| License       | MIT
| Doc PR        | -

`RememberMeServices::autoLogin()` only checks that the cookie exists in `$request->cookies` while `loginFail()` only alter `$request->attributes` (which allows child implementations to read the canceled cookie for e.g. removing a persistent one).
This makes `autoLogin()` checks for `request->attributes` first, which fixes the linked issue.

Failure expected on deps=high build.

Commits
-------

9b711b8 [Security] Prevent canceled remember-me cookie from being accepted
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants