Skip to content

[Security/Http] Fix compat of persistent remember-me with legacy tokens #49103

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

Conversation

nicolas-grekas
Copy link
Member

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #49100
License MIT
Doc PR -

In #49078, we changed the format of remember-me tokens, effectively invalidating them all.
While the invalidation is intentional for signature-based remember-me handlers, persistent remember-me handlers could accept both legacy and updated tokens.
This PR fixes compat with legacy tokens for persistent remember-me handlers.

@carsonbot carsonbot added this to the 5.4 milestone Jan 25, 2023
@nicolas-grekas nicolas-grekas changed the title Update CHANGELOG for 5.4.19 [Security/Http] Fix compat of persistent remember-me with legacy tokens Jan 25, 2023
@@ -45,7 +44,6 @@ public function __construct(TokenProviderInterface $tokenProvider, string $secre
}
$this->tokenProvider = $tokenProvider;
$this->tokenVerifier = $tokenVerifier;
$this->secret = $secret;
Copy link
Member Author

Choose a reason for hiding this comment

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

unused property after #49078

Copy link
Member

Choose a reason for hiding this comment

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

should we deprecate the constructor argument ?

Copy link
Member Author

Choose a reason for hiding this comment

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

we could but not in 5.4

Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

How long should they be supported?

@nicolas-grekas
Copy link
Member Author

We can keep that logic forever, we don't really care IMHO.

@eexit
Copy link

eexit commented Jan 25, 2023

Hi @nicolas-grekas,

Thanks for the quick fix.
I'm struggling with testing your PR. I use Symfony Flex and after adding this into my composer.json, it looks like I still can't install the forked bundled:

"repositories": [
    {
        "_comment": "Testing: https://github.com/symfony/symfony/pull/49103",
        "type": "git",
        "url": "https://github.com/nicolas-grekas/symfony.git"
    }
]

Doing this command does not work: composer require -W "symfony/security-bundle:dev-sec-http-legacy-persistent-token" --prefer-source.

composer require -W "symfony/security-bundle:dev-sec-http-legacy-persistent-token" --prefer-source
./composer.json has been updated
Running composer update symfony/security-bundle --with-all-dependencies
Loading composer repositories with package information
Info from https://repo.packagist.org/: #StandWithUkraine
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires symfony/security-bundle dev-sec-http-legacy-persistent-token, found symfony/security-bundle[2.0.7, ..., 2.8.x-dev, v3.0.0-BETA1, ..., 3.4.x-dev, v4.0.0-BETA1, ..., 4.4.x-dev, v5.0.0-BETA1, ..., 5.4.x-dev, v6.0.0-BETA1, ..., 6.3.x-dev] but it does not match the constraint.
  Problem 2
    - easycorp/easyadmin-bundle is locked to version v*** and an update of this package was not requested.
    - easycorp/easyadmin-bundle v*** requires symfony/security-bundle ^5.4|^6.0 -> found symfony/security-bundle[v5.4.0-BETA1, ..., 5.4.x-dev, v6.0.0-BETA1, ..., 6.3.x-dev] but it conflicts with your root composer.json require (dev-sec-http-legacy-persistent-token).


Installation failed, reverting ./composer.json and ./composer.lock to their original content.

It looks like composer is ignoring your repo and I have no idea why.
Would you mind giving some insights on how to test this?

Thanks!

@nicolas-grekas nicolas-grekas merged commit 96cdc5c into symfony:5.4 Jan 25, 2023
@nicolas-grekas nicolas-grekas deleted the sec-http-legacy-persistent-token branch January 25, 2023 18:24
fabpot added a commit that referenced this pull request Apr 8, 2023
…stentRememberMeHandler constructor (xabbuh)

This PR was merged into the 6.3 branch.

Discussion
----------

[Security] deprecate the $secret argument of the PersistentRememberMeHandler constructor

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Tickets       | #49103 (comment)
| License       | MIT
| Doc PR        |

Commits
-------

bf81d66 deprecate the $secret argument of the PersistentRememberMeHandler constructor
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